One graph decides what runs before what - #466
Merged
Merged
Conversation
A template reference is a data dependency: {{ make.result.path }} cannot
exist until make has run. Three parts of the compiler agreed and none of them
told the scheduler.
DependencyValidator inferred edges from for_each/condition/while but never
looked inside parameters at all, and its regex recognised only six
hard-coded suffixes (result|output|data|content|status|metadata) -- so the
commonest reference in the language was invisible to the graph that checks
for cycles.
DataFlowValidator built a graph that did include parameters, logged it, and
threw it away.
YAMLCompiler._analyze_template found the same references with a third regex
and stored them on Task.template_metadata, where nothing scheduled from
them.
Task.dependencies came from the explicit key alone, so a pipeline could
validate and then fail at run time (#465): both steps landed in one execution
level, the producer's result was not in context, and the render failed.
core/dependency_graph.py is the single answer. Explicit dependencies,
template references from every nested field, and control-flow references go
into one graph; that graph is what the cycle check inspects and what every
Task.dependencies is built from. Appending edges inside _build_task would
have produced a schedule nobody validated -- the same shape as the bug.
Three extractors became one AST extractor in core/template_scope, which is
why the 're' module is now an unused import in dependency_validator.py.
Being
scope-aware, it excludes loop-local names for free.
ControlFlowCompiler._build_task has many exits, each building its task
through a different handler that reads dependencies off the definition. It is
now a thin wrapper applying the graph once, so control-flow steps are not the
one kind of step that silently keeps the old behaviour.
implicit_dependency is INFO-only. Inference is a supported way to write a
pipeline, not a mistake; the lint names the line to add for authors who want
the ordering written down.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #465 with option A: infer the dependency and schedule on it.
The defect
A template reference is a data dependency —
{{ make.result.path }}cannot exist untilmakehas run. Three parts of the compiler agreed about that and none of them told the scheduler:DependencyValidatorfor_each/condition/while/untilDataFlowValidator.data_flow_graph_analyze_template(regex)Task.template_metadata, scheduled from by nothingTask.dependenciescame from the explicit key alone. So a pipeline validated and then failed at run time:Worth calling out from the mapping:
DependencyValidator._extract_template_dependenciesmatched only six hard-coded suffixes (result|output|data|content|status|metadata), somake.pathwas not recognised as a task reference at all. The commonest reference in the language was invisible to the graph that checks for cycles.The fix
core/dependency_graph.pyis the single answer, following the review's constraints exactly:Task.dependenciesis built from it.Consolidation, not a fourth mechanism. Three extractors became one AST extractor in
core/template_scope, which is whyreis now an unused import independency_validator.py— the clearest confirmation the regex is gone. Being the scope-aware extractor from #461, it excludes loop-local names for free.ControlFlowCompiler._build_taskhas many exits — conditional, goto, for-each, action-loop — each building through a different handler that readsdependenciesoff the definition. It is now a thin wrapper applying the graph once, so control-flow steps are not the one kind of step silently keeping the old behaviour.The
implicit_dependencylintINFO severity, never failing. Inference is a supported way to write a pipeline, not a mistake, and failing here would break every pipeline relying on it. Carries stable structured fields:
code,severity,component,path,suggestions, and metadata withstep/referenced_step/parameter_path/origin.Two corrections to my own work
The reproduction I filed in #465 was partly wrong. It used
{{ make.path }}, which is not a field the filesystem tool returns — it returns{'result': {'path': ...}, 'success': ...}. That conflated the missing edge with a wrong field name and would have failed even with the bug fixed. I've corrected the issue.{{ make.path }}is now its own test asserting the opposite: it must still fail, because ordering a step correctly does not invent a field the producer never returned.A mutation passed for the wrong reason. Reverting
YAMLCompiler._build_taskfailed only 2 unit tests while the e2e reproduction still passed — because the CLI runs throughControlFlowCompiler, whose wrapper still applied the graph. The mutation was not a faithful inverse. With both compilers reverted, the e2e test fails as it should.Verification
{% if %}/filter/subscript/index/nested-list reference forms.Task.dependenciesignoring the graph; the graph skippingparameters(22 failures); the cycle check removed.Not in this PR
Warning visibility (the second half of #465) and the catalogue baseline rename — both from the review's sequence, both separate concerns.
🤖 Generated with Claude Code