Infer dependencies only from fields the runtime renders

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),

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",
+})

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