-
Notifications
You must be signed in to change notification settings - Fork 53
fix(dsl): Enable shorthand template syntax with workflow directory resolution #582
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
4011fa5 to
05372cf
Compare
nfgrep
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, would await @juniper-shopify's input though.
Maybe update/add a little DSL example that leverages this?
juniper-shopify
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for doing this!
A more robust priority list of search locations will be helpful, and adding the workflow dir to the WorkflowContext will reduce the size of these method signatures that are already kind of too big
|
|
||
| #: (String, ?Hash) -> String | ||
| def template(path, args = {}) | ||
| unless File.exist?(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit: I'd prefer to use Pathname objects instead of doing manipulation on strings that represent paths. The template method itself should ideally have Pathname | String as the type of its path argument, and it should coerce both to a pathname. this just makes it easier to reason about any code in the codebase (dsl part of it at least) that deals with file paths
| end | ||
|
|
||
| #: (String, ?Hash) -> String | ||
| def template(path, args = {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should actually remove this method definition from cog_input_context when you add it here. it's getting overwritten by the binding, so it works, but it will be very confusing to have a defunct implementation lying around
| if @workflow_dir | ||
| # Try relative to workflow directory | ||
| relative_path = @workflow_dir.join("prompts/#{path}.md.erb") | ||
| path = relative_path.to_s if relative_path.exist? | ||
| else | ||
| # Fallback to current working directory | ||
| path = "prompts/#{path}.md.erb" | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has a regression from the existing logic in that it only considers paths inside a prompts/ directory, and only paths ending with .md.erb.
I think we should implement a priority stack of places to look for a matching file, roughly like this (assume path is a Pathname that is whatever the user provided):
path if path.absolute?@workflow_dir / pathif that.exist?@workflow_dir / "#{path}.erb"if that.exist?@workflow_dir / "#{path}.md.erb"if that.exist?- repeat 2 - 4 for
@workflow_dir / prompts - repeat 2 - 4 for
Pathname.pwd - repeat 2 - 4 for
Pathname.pwd / prompts
and then just use the first one that exists
| end | ||
|
|
||
| unless File.exist?(path) | ||
| raise CogInputContext::ContextNotFoundError, "The prompt #{path} could not be found" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
template will generally be used for prompts, but not exclusively. For example, I have a workflow where I'm using template to generate some JSON files containing filter expressions for a system I want an agent to search, and then telling the agent how to run searches with those files, instead of putting the filter expressions into the prompt itself.
| raise CogInputContext::ContextNotFoundError, "The prompt #{path} could not be found" | |
| raise CogInputContext::ContextNotFoundError, "The file '#{path}' could not be found" |
| #| WorkflowContext, | ||
| #| ?scope: Symbol?, | ||
| #| ?scope_value: untyped?, | ||
| #| ?scope_index: Integer | ||
| #| ?scope_index: Integer, | ||
| #| ?workflow_dir: Pathname? | ||
| #| ) -> void | ||
| def initialize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Re: lines +31 to +41]
add the workflow dir to the WorkflowContext, instead of passing it through explicitly. it should also be non-nilable
See this comment inline on Graphite.
| @cog_registry = cog_registry | ||
| @cogs = cogs | ||
| @workflow_context = workflow_context | ||
| @workflow_dir = workflow_dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the workflow dir to the WorkflowContext, instead of passing it through explicitly. it should also be non-nilable
| #: (String, ?Hash) -> String | ||
| def template(path, args = {}); end | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a good opportunity to add some method doc for this while we're at it!
you can use the /docs:write-comments custom claude command to automatically write these for you; then you'll just have the edit them and make sure they make sense. do something like this:
- add a comment on this method with some rough notes about what it does, how it works, and things you want to include in the doc. super rough. (sometimes you don't even need this, but this method has no analogue that claude can use to figure out what it does, so some explanation would be helpful)
- in an interactive claude, do
/docs:write-comments sorbet/rbi/shims/lib/roast/dsl/cog_input_context.rbiand it will do its thing - read what it wrote and tweak it.
- check that it didn't mess up any other comments, and revert anything if it did
| workflow_dir = Pathname.new(@workflow_dir) | ||
| manager = CogInputManager.new(@cog_registry, @cogs, @workflow_context, workflow_dir) | ||
|
|
||
| # Change to different directory to test the fix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: test comments should not refer to "fixes". they should simply describe the objective behaviour being tested
| test "template method fails with shorthand syntax when not in correct directory and no workflow_dir" do | ||
| manager = CogInputManager.new(@cog_registry, @cogs, @workflow_context, nil) | ||
|
|
||
| # This test demonstrates the bug when workflow_dir is not provided |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: same as above -- test comment shouldn't refer to a bug that no longer exists, just describe the expected behaviour it is testing.
(okay, to be fair, there are cases where I think it's useful to have some comments in a test case explaining the particulars of a bug. But, I'd reserve that explicitly for when the bug is very complicated and counterintuitive, and non-obvious to understand what could go wrong to someone with even medium familiarity with the component)

TL;DR
Fixed template resolution in workflows by making it aware of the workflow directory.
What changed?
templatemethod fromCogInputContexttoCogInputManagerto access workflow directory informationworkflow_dirparameter toCogInputManager,ExecutionManager, and system cogsHow to test?
promptsdirectorytemplate("my_template", { name: "Test" }))Why make this change?
Previously, the template method could only resolve templates relative to the current working directory, causing issues when workflows were run from a different directory than where they were defined. This change ensures templates are properly resolved relative to the workflow's location, making workflows more portable and robust.