From b01aa0bc25cadc58fdfceb708027fd6f28ccd7c4 Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Tue, 4 Aug 2026 09:27:25 -0400 Subject: [PATCH] Infer dependencies only from fields the runtime renders Two steps that never interact failed to compile: - id: a name: "{{ b.result }}" - id: b name: "{{ a.result }}" Dependency cycle detected: a -> b -> a Task names are copied verbatim; nothing substitutes into them. Two inert strings produced a hard compile error. The cause was a blocklist introduced with the canonical graph in #466: build_dependency_graph scanned every key it did not specifically exclude, so it read name, description, metadata and anything else a step happened to carry. Since that graph now drives scheduling and execution levels as well as cycle detection, a spurious edge does not merely warn -- it serialises independent work, invents cycles, and misreports which dependencies were inferred. core.step_fields replaces it with an allowlist, and each entry is there because the runtime was read rather than guessed: parameters ControlSystem._render_task_templates deep-renders it action the same function renders it when it is a string location kept as location_template and resolved when the output is recorded, which is why examples write location: "./reports/{{ inputs.topic | slugify }}.md" Nested steps are recursed into and attributed to the enclosing step, a child not being scheduled independently of its parent. Control-flow keys are rendered too, but keep their separate, more precise origin. Both directions are failure modes that have already happened here, so both are pinned: scanning too much invents cycles, and scanning too little re-opens #465, where a step ran before the value it needed existed. The catalogue guards the first; examples/supported/, which actually run, guard the second. Reverting to the blocklist fails 7 of the 14 new tests, including the CLI-level reproduction. Blocking suite: 882 passed. Catalogue: all 52 still validate. Co-Authored-By: Claude Opus 5 (1M context) --- src/orchestrator/core/dependency_graph.py | 38 +++-- src/orchestrator/core/step_fields.py | 77 +++++++++ tests/test_renderable_fields.py | 189 ++++++++++++++++++++++ 3 files changed, 295 insertions(+), 9 deletions(-) create mode 100644 src/orchestrator/core/step_fields.py create mode 100644 tests/test_renderable_fields.py diff --git a/src/orchestrator/core/dependency_graph.py b/src/orchestrator/core/dependency_graph.py index 52438c8..fc5bf2b 100644 --- a/src/orchestrator/core/dependency_graph.py +++ b/src/orchestrator/core/dependency_graph.py @@ -35,6 +35,7 @@ from dataclasses import dataclass from typing import Any, Dict, Iterator, List, Set, Tuple +from .step_fields import NESTED_STEP_FIELDS, RENDERABLE_STEP_FIELDS from .template_scope import template_references #: Where an edge came from. Kept on the edge so a diagnostic can say *why* @@ -198,6 +199,27 @@ def _strings_in(value: Any, path: str = "") -> Iterator[Tuple[str, str]]: yield from _strings_in(item, f"{path}[{index}]") +def _renderable_values( + step: Dict[str, Any], prefix: str = "" +) -> Iterator[Tuple[str, Any, str]]: + """Every renderable field of a step, and of any steps nested inside it. + + A nested step is not scheduled on its own -- it runs as part of its + parent -- so its references are attributed to the enclosing step. + """ + for key in RENDERABLE_STEP_FIELDS: + if key in step: + yield key, step[key], f"{prefix}{key}" + + for key in NESTED_STEP_FIELDS: + children = step.get(key) + if not isinstance(children, (list, tuple)): + continue + for index, child in enumerate(children): + if isinstance(child, dict): + yield from _renderable_values(child, f"{prefix}{key}[{index}].") + + def _declared_dependencies(step: Dict[str, Any]) -> List[str]: """The `dependencies:` / `depends_on:` value, however it is written.""" raw = step.get("dependencies", step.get("depends_on", [])) @@ -253,18 +275,16 @@ def add(task: str, depends_on: str, origin: str, location: str) -> None: if base in known: add(task_id, base, CONTROL_FLOW, path) - # Everything else the step carries that may hold a template. `id`, - # `dependencies` and the control-flow keys above are excluded: the - # first two are not templates, and the third is already covered with - # a more precise origin. - for key, value in step.items(): - if key in ("id", "dependencies", "depends_on") or key in CONTROL_FLOW_KEYS: - continue - for text, path in _strings_in(value, key): + # Only the fields the runtime actually renders. A template in `name` + # or `description` is copied verbatim and orders nothing; scanning it + # invented cycles between steps that never interact. See + # `core.step_fields` for why each field is on the list. + for key, value, path in _renderable_values(step): + for text, text_path in _strings_in(value, path): for reference in template_references(text, env): base = reference.split(".", 1)[0] if base in known: - add(task_id, base, TEMPLATE, path) + add(task_id, base, TEMPLATE, text_path) return DependencyGraph( steps=tuple(step_ids), diff --git a/src/orchestrator/core/step_fields.py b/src/orchestrator/core/step_fields.py new file mode 100644 index 0000000..76cd01f --- /dev/null +++ b/src/orchestrator/core/step_fields.py @@ -0,0 +1,77 @@ +"""Which parts of a step are rendered at run time, and which are copied verbatim. + +A template that never renders cannot create a dependency. `name:` is copied +onto the task and shown in logs; nothing substitutes into it. So this pipeline +has no ordering constraint at all:: + + - id: a + name: "{{ b.result }}" + - id: b + name: "{{ a.result }}" + +The canonical dependency graph nevertheless read `{{ b.result }}` as "a needs +b", `{{ a.result }}` as "b needs a", and rejected the pipeline with +`Dependency cycle detected: a -> b -> a`. Two inert strings produced a hard +compile error. + +The cause was a blocklist. `build_dependency_graph` scanned every key it did +not specifically exclude, which meant it scanned `name`, `description`, +`metadata` and anything else a step happened to carry. Since that same graph +now drives scheduling and execution levels as well as cycle detection, a +spurious edge does not merely warn -- it serialises independent work, invents +cycles, and misreports which dependencies were inferred. + +The list below is an allowlist, and each entry is here because the runtime was +read rather than guessed: + +`parameters` + `ControlSystem._render_task_templates` deep-renders it. +`action` + Same function renders it when it is a string. +`location` + Kept as `location_template` on the task's output metadata and resolved + when the output is recorded, which is why real examples write + ``location: "./reports/{{ inputs.topic | slugify }}.md"``. + +Control-flow keys (`for_each`, `condition`, `while`, ...) are also rendered, +but they are inferred separately in `dependency_graph` so their edges carry +the more precise `control_flow` origin. + +Everything absent from this list is inert: a reference inside it orders +nothing. Adding a field here without first confirming that the runtime renders +it re-opens the false-cycle bug; omitting one the runtime *does* render +re-opens #465, where a step ran before the value it needed existed. The +catalogue and `examples/supported/` are the check on both directions -- +the former for false rejections, the latter because those pipelines actually +run. +""" + +from __future__ import annotations + +from typing import FrozenSet, Tuple + +#: Step fields whose templates are rendered at run time. A reference in one of +#: these is a real ordering constraint. +RENDERABLE_STEP_FIELDS: Tuple[str, ...] = ( + "parameters", + "action", + "location", +) + +#: Fields holding child steps. Their own renderable fields count, attributed to +#: the enclosing step -- a child is not scheduled independently of its parent. +NESTED_STEP_FIELDS: Tuple[str, ...] = ( + "steps", +) + +#: Named so a test can assert they are never scanned, and so the reason is +#: written down rather than implied by absence. +INERT_STEP_FIELDS: FrozenSet[str] = frozenset({ + "id", # the step's own name + "name", # human-readable label, copied verbatim + "description", # prose + "metadata", # arbitrary author data + "tool", # a registry key, not a template + "dependencies", # already read, with the `declared` origin + "depends_on", +}) diff --git a/tests/test_renderable_fields.py b/tests/test_renderable_fields.py new file mode 100644 index 0000000..39c561c --- /dev/null +++ b/tests/test_renderable_fields.py @@ -0,0 +1,189 @@ +"""A template that never renders cannot create a dependency. + +`build_dependency_graph` scanned every step key it did not specifically +exclude. `name:` was not excluded, so this pipeline -- two steps that never +interact -- failed to compile with `Dependency cycle detected: a -> b -> a`:: + + - id: a + name: "{{ b.result }}" + - id: b + name: "{{ a.result }}" + +Task names are copied verbatim. Nothing substitutes into them. And because the +canonical graph now drives scheduling as well as cycle detection, a spurious +edge does more than warn: it serialises independent steps and misreports which +dependencies were inferred. + +The fix is an allowlist (`core.step_fields`) rather than a blocklist. These +tests pin it in both directions, because both are bugs that have already +happened here: scanning too much invents cycles, and scanning too little +re-opens #465, where a step ran before the value it needed existed. +""" + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +from orchestrator.core.dependency_graph import build_dependency_graph +from orchestrator.core.step_fields import ( + INERT_STEP_FIELDS, + NESTED_STEP_FIELDS, + RENDERABLE_STEP_FIELDS, +) + +pytestmark = [pytest.mark.contract] + +REPO = Path(__file__).resolve().parent.parent + + +def _graph(*steps): + return build_dependency_graph({"id": "p", "steps": list(steps)}) + + +# --------------------------------------------------------------------------- +# Inert fields order nothing +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("field", sorted(INERT_STEP_FIELDS - {"id", "dependencies", "depends_on"})) +def test_a_template_in_an_inert_field_creates_no_edge(field): + graph = _graph( + {"id": "a", field: "{{ b.result }}", "parameters": {"x": 1}}, + {"id": "b", "parameters": {"x": 1}}, + ) + assert graph.dependencies_for("a") == [], ( + f"a template in `{field}` was read as a dependency, but the runtime " + f"never renders that field" + ) + + +def test_the_reported_false_cycle_is_not_a_cycle(): + """The exact reproduction from review.""" + graph = _graph( + {"id": "a", "name": "{{ b.result }}", "parameters": {"x": 1}}, + {"id": "b", "name": "{{ a.result }}", "parameters": {"x": 1}}, + ) + assert graph.cycles() == [] + assert graph.levels() == [["a", "b"]], ( + "two steps that never interact must be free to run concurrently" + ) + + +def test_an_inert_template_does_not_become_an_implicit_dependency_finding(): + """The lint would otherwise tell an author to write down an ordering that + does not exist.""" + graph = _graph( + {"id": "a", "description": "see {{ b.result }}", "parameters": {"x": 1}}, + {"id": "b", "parameters": {"x": 1}}, + ) + assert graph.inferred_only() == [] + + +# --------------------------------------------------------------------------- +# Renderable fields still order -- the #465 direction +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("field,value", [ + ("parameters", {"content": "{{ a.result }}"}), + ("action", "write {{ a.result }}"), + ("location", "./out/{{ a.result }}.md"), +]) +def test_a_template_in_a_renderable_field_orders_the_step(field, value): + """Dropping one of these re-opens #465: the step runs before the value it + needs exists, having passed validation.""" + graph = _graph( + {"id": "a", "parameters": {"x": 1}}, + {"id": "b", field: value}, + ) + assert graph.dependencies_for("b") == ["a"], ( + f"a reference in `{field}` is rendered at run time and must order the step" + ) + + +def test_a_nested_step_orders_its_parent(): + """A child step is not scheduled on its own, so its references belong to + the step that contains it.""" + graph = _graph( + {"id": "a", "parameters": {"x": 1}}, + { + "id": "loop", + "for_each": "[1, 2]", + "steps": [{"id": "inner", "parameters": {"c": "{{ a.result }}"}}], + }, + ) + assert graph.dependencies_for("loop") == ["a"] + + +def test_control_flow_references_keep_their_own_origin(): + """`for_each` is renderable too, but its edges are inferred separately so a + diagnostic can distinguish an iterable from a parameter.""" + graph = _graph( + {"id": "a", "parameters": {"x": 1}}, + {"id": "b", "for_each": "{{ a.result }}", "parameters": {"x": 1}}, + ) + assert graph.origins_for("b", "a") == {"control_flow"} + + +# --------------------------------------------------------------------------- +# The contract itself +# --------------------------------------------------------------------------- + +def test_no_field_is_both_renderable_and_inert(): + assert not (set(RENDERABLE_STEP_FIELDS) & INERT_STEP_FIELDS) + assert not (set(NESTED_STEP_FIELDS) & INERT_STEP_FIELDS) + + +def test_the_runtime_renders_every_field_claimed_renderable(): + """Behavioural, not a name check. + + `parameters` and `action` are rendered by `_render_task_templates`; + `location` is resolved from `location_template` when the output is + recorded. If a field is listed but nothing renders it, references inside + it order steps for no reason -- the bug this module fixes, in a new place. + """ + source = (REPO / "src" / "orchestrator" / "core" / "control_system.py").read_text() + for field in ("parameters", "action"): + assert f"rendered_task.{field}" in source, ( + f"`{field}` is on the renderable list but nothing in control_system " + f"renders it" + ) + + +# --------------------------------------------------------------------------- +# End to end +# --------------------------------------------------------------------------- + +@pytest.mark.e2e +def test_the_false_cycle_pipeline_compiles_through_the_cli(tmp_path): + """The unit test uses `build_dependency_graph` directly; the CLI runs the + control-flow compiler. #466 shipped with a mutation that only one of those + two caught, so both paths are exercised.""" + pipeline = tmp_path / "p.yaml" + pipeline.write_text( + "id: inert\n" + "name: Inert\n" + "steps:\n" + " - id: a\n" + ' name: "{{ b.result }}"\n' + " tool: filesystem\n" + " action: write\n" + " parameters: {path: './a.txt', content: 'a'}\n" + " - id: b\n" + ' name: "{{ a.result }}"\n' + " tool: filesystem\n" + " action: write\n" + " parameters: {path: './b.txt', content: 'b'}\n" + ) + env = dict(os.environ) + env["PYTHONPATH"] = str(REPO / "src") + os.pathsep + env.get("PYTHONPATH", "") + env["ORCHESTRATOR_AUTO_INSTALL"] = "0" + result = subprocess.run( + [sys.executable, "-m", "orchestrator.cli", "validate", str(pipeline)], + cwd=str(tmp_path), env=env, capture_output=True, text=True, timeout=300, + ) + assert result.returncode == 0, result.stdout[-800:] + result.stderr[-800:] + # Not a bare "cycle" search: pytest names tmp_path after the test, so the + # word appears in the pipeline's own path in the output. + assert "Dependency cycle detected" not in result.stdout + result.stderr