-
Notifications
You must be signed in to change notification settings - Fork 56
feat(engine): add workflow.dir, workflow.file, workflow.name template variables #121
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
Changes from all commits
33a5a24
eaf6c9f
dd2c2e2
ec597f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,9 @@ def test_init_default_values(self) -> None: | |
| assert ctx.agent_outputs == {} | ||
| assert ctx.current_iteration == 0 | ||
| assert ctx.execution_history == [] | ||
| assert ctx.workflow_dir == "" | ||
| assert ctx.workflow_file == "" | ||
| assert ctx.workflow_name == "" | ||
|
|
||
| def test_set_workflow_inputs(self) -> None: | ||
| """Test setting workflow inputs.""" | ||
|
|
@@ -151,6 +154,50 @@ def test_last_only_mode_empty_history(self) -> None: | |
| assert "context" in agent_ctx | ||
|
|
||
|
|
||
| class TestWorkflowContextMetadata: | ||
| """Tests for workflow metadata (dir, file, name) in context.""" | ||
|
|
||
| def test_workflow_dir_file_name_in_accumulate_context(self) -> None: | ||
| """Test workflow.dir, workflow.file, workflow.name available in accumulate mode.""" | ||
| ctx = WorkflowContext( | ||
| workflow_dir="/home/user/workflows", | ||
| workflow_file="/home/user/workflows/main.yaml", | ||
| workflow_name="my-workflow", | ||
| ) | ||
| ctx.set_workflow_inputs({"key": "val"}) | ||
|
|
||
| agent_ctx = ctx.build_for_agent("agent", [], mode="accumulate") | ||
|
|
||
| assert agent_ctx["workflow"]["dir"] == "/home/user/workflows" | ||
| assert agent_ctx["workflow"]["file"] == "/home/user/workflows/main.yaml" | ||
| assert agent_ctx["workflow"]["name"] == "my-workflow" | ||
| assert agent_ctx["workflow"]["input"] == {"key": "val"} | ||
|
|
||
| def test_workflow_metadata_in_explicit_mode(self) -> None: | ||
| """Test workflow.dir/file/name available in explicit mode (not filtered).""" | ||
| ctx = WorkflowContext( | ||
| workflow_dir="/registry/twig", | ||
| workflow_file="/registry/twig/sdlc.yaml", | ||
| workflow_name="twig-sdlc", | ||
| ) | ||
|
|
||
| agent_ctx = ctx.build_for_agent("agent", [], mode="explicit") | ||
|
|
||
| assert agent_ctx["workflow"]["dir"] == "/registry/twig" | ||
| assert agent_ctx["workflow"]["file"] == "/registry/twig/sdlc.yaml" | ||
| assert agent_ctx["workflow"]["name"] == "twig-sdlc" | ||
|
|
||
| def test_empty_metadata_omitted(self) -> None: | ||
| """Test that empty workflow metadata fields are not included.""" | ||
| ctx = WorkflowContext() | ||
|
|
||
| agent_ctx = ctx.build_for_agent("agent", [], mode="accumulate") | ||
|
|
||
| assert "dir" not in agent_ctx["workflow"] | ||
| assert "file" not in agent_ctx["workflow"] | ||
| assert "name" not in agent_ctx["workflow"] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: add an engine-level wiring test. These tests verify def test_engine_populates_workflow_metadata(tmp_path):
wf_file = tmp_path / "wf.yaml"
wf_file.write_text("...") # or use a fixture config
engine = WorkflowEngine(config, workflow_path=wf_file)
assert engine.context.workflow_dir == str(tmp_path.resolve())
assert engine.context.workflow_file == str(wf_file.resolve())
assert engine.context.workflow_name == config.workflow.namewould catch any future regression in the wiring (e.g., if someone refactors
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair, there was no guard that |
||
|
|
||
|
|
||
| class TestWorkflowContextExplicitMode: | ||
| """Tests for explicit context mode.""" | ||
|
|
||
|
|
||
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.
Resume drops these fields.
The new
workflow_dir/file/namefields aren't included into_dict()(line 486) or restored byfrom_dict()(line 502). On resume,cli/run.py:1538callsengine.set_context(restored_context), which replaces the engine's context (built with the metadata in__init__) with one that has the metadata wiped to empty strings. After resume,{{ workflow.dir/file/name }}will silently disappear from templates — exactly the registry-based script-path scenario this PR exists for.Recommended fix: repopulate metadata inside
WorkflowEngine.set_context()fromself.workflow_path/self.config. Keeps absolute paths out of checkpoint files (which would otherwise become stale if the workflow moves) and keeps the source of truth co-located with the engine that knows the path.Please also add a regression test in
tests/test_engine/test_resume.py(or similar) asserting thatworkflow.dirsurvives a checkpoint round-trip + resume.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.
Yeah, you're right, the round trip drops them and
set_contextwas happily clobbering the engine's view. Fixed ineaf6c9f:set_contextnow repopulatesworkflow_dir/file/namefromself.workflow_pathandself.configafter the replace, so the engine stays the source of truth and we keep absolute paths out of the checkpoint. Addedtest_set_context_repopulates_workflow_metadatathat round-trips throughto_dict/from_dict, callsset_context, and verifies viabuild_for_agentthat templates actually resolve. Also covered the no-workflow_pathcase intest_set_context_without_workflow_path_still_sets_name.