Accept loop variables in the spelling examples actually use (#469)

scripts/catalogue_report.py

@@ -172,16 +172,28 @@ def load_baseline() -> Optional[List[str]]:
     ]
 
 
+#: The whole file, header included, is generated. Editing the header by hand
+#: puts it one `--update` away from being silently discarded -- which is how
+#: the paragraph below was lost once already.
+BASELINE_HEADER = (
+    "# Examples that pass `orchestrator validate` -- and nothing more.\n"
+    "#\n"
+    "# Passing validation is not running, is not producing correct output,\n"
+    "# and is not being a supported example. examples/supported/ is the\n"
+    "# stronger contract: those are executed and their behaviour asserted.\n"
+    "#\n"
+    "# A file leaving this list is a regression and fails CI. A file\n"
+    "# joining it is an improvement; run:\n"
+    "#     python scripts/catalogue_report.py --update\n"
+    "#\n"
+    "# The count is deliberately not the contract -- see the module\n"
+    "# docstring in scripts/catalogue_report.py.\n"
+)
+
+
 def write_baseline(paths: List[str]) -> None:
     BASELINE.write_text(
-        "# Examples that pass `orchestrator validate`.\n"
-        "#\n"
-        "# A file leaving this list is a regression and fails CI. A file\n"
-        "# joining it is an improvement; run:\n"
-        "#     python scripts/catalogue_report.py --update\n"
-        "#\n"
-        "# The count is deliberately not the contract -- see the module\n"
-        "# docstring in scripts/catalogue_report.py.\n"
+        BASELINE_HEADER
         + "".join(f"{p}\n" for p in sorted(paths))
     )
 

scripts/catalogue_validation_baseline.txt

@@ -23,6 +23,7 @@ examples/enhanced/control_flow_conditional_enhanced.yaml
 examples/enhanced/control_flow_dynamic_enhanced.yaml
 examples/enhanced/data_processing_enhanced.yaml
 examples/enhanced/data_processing_pipeline_enhanced.yaml
+examples/enhanced/fact_checker_enhanced.yaml
 examples/enhanced/interactive_pipeline_enhanced.yaml
 examples/enhanced/llm_routing_pipeline_enhanced.yaml
 examples/enhanced/mcp_integration_pipeline_enhanced.yaml
@@ -36,6 +37,7 @@ examples/enhanced/statistical_analysis_enhanced.yaml
 examples/enhanced/terminal_automation_enhanced.yaml
 examples/enhanced/validation_pipeline_enhanced.yaml
 examples/enhanced/working_web_search_enhanced.yaml
+examples/fact_checker.yaml
 examples/interactive_pipeline.yaml
 examples/llm_routing_pipeline.yaml
 examples/mcp_integration_pipeline.yaml

src/orchestrator/core/template_globals.py

@@ -107,6 +107,42 @@ def arity(self) -> str:
 
 GLOBAL_NAMES: FrozenSet[str] = frozenset(spec.name for spec in GLOBAL_SPECS)
 
+
+#: Names the runtime binds inside a loop step -- `for_each`, `foreach`,
+#: `create_parallel_queue`, `action_loop` -- and inside that step's nested
+#: `steps:`. Declared here, with the globals, because they are part of the
+#: pipeline language rather than one validator's private knowledge. The
+#: data-flow validator knew the bare spelling and the template validator knew
+#: only the `$`-prefixed one, so `{{ item.name }}` -- the form every example
+#: actually uses -- was reported as an undefined variable (#469).
+LOOP_VARIABLE_NAMES: Tuple[str, ...] = (
+    "item", "index", "is_first", "is_last", "iteration", "loop",
+)
+
+LOOP_VARIABLES: FrozenSet[str] = frozenset(LOOP_VARIABLE_NAMES)
+
+#: The runtime registers both spellings. `$item` is not a name Jinja can
+#: parse, so it has to be matched as raw text rather than found in the AST --
+#: a substring hack that must stay confined to these.
+DOLLAR_LOOP_VARIABLES: FrozenSet[str] = frozenset(
+    f"${name}" for name in LOOP_VARIABLE_NAMES
+)
+
+ALL_LOOP_VARIABLES: FrozenSet[str] = LOOP_VARIABLES | DOLLAR_LOOP_VARIABLES
+
+
+#: Step keys that make a step a loop, and so bind `LOOP_VARIABLES` inside it
+#: and inside its nested `steps:`.
+#:
+#: The template validator recognised only `for_each` and `while`, so a step
+#: written with `foreach` -- the spelling the compiler accepts as an alias --
+#: was not treated as a loop, and its loop variables were reported as used
+#: outside a loop. One spelling known to the compiler and a different set
+#: known to the validator is the same class of disagreement as #465.
+LOOP_STEP_KEYS: Tuple[str, ...] = (
+    "for_each", "foreach", "while", "create_parallel_queue", "action_loop",
+)
+
 _BY_NAME = {spec.name: spec for spec in GLOBAL_SPECS}
 
 

src/orchestrator/validation/data_flow_validator.py

@@ -29,10 +29,10 @@
 
 logger = logging.getLogger(__name__)
 
-#: Names bound by a loop rather than by a step. `loop` is Jinja's own.
-LOOP_VARIABLES = frozenset(
-    {"item", "index", "loop", "iteration", "is_first", "is_last"}
-)
+#: Names bound by a loop rather than by a step. Declared with the rest of the
+#: pipeline language in `core.template_globals`, so this validator and the
+#: template validator cannot disagree about them again (#469).
+from ..core.template_globals import LOOP_VARIABLES  # noqa: E402
 
 #: `thing['key']` -> `thing.key`, so one spelling reaches the checks below.
 _SUBSCRIPT = re.compile(r"""\[\s*['"]([^'"]+)['"]\s*\]""")

src/orchestrator/validation/template_validator.py

@@ -18,7 +18,12 @@
 from jinja2.sandbox import SandboxedEnvironment
 
 from ..core.runtime_context import BARE_RUNTIME_NAMES, RUNTIME_NAMESPACE
-from ..core.template_globals import find_global_misuse
+from ..core.template_globals import (
+    ALL_LOOP_VARIABLES,
+    DOLLAR_LOOP_VARIABLES,
+    LOOP_STEP_KEYS,
+    find_global_misuse,
+)
 from ..core.template_sandbox import pipeline_global_names
 
 logger = logging.getLogger(__name__)
@@ -119,7 +124,12 @@ def __init__(self, debug_mode: bool = False):
         self.comment_pattern = re.compile(r'{#\s*([^#]+)\s*#}')
 
         # Loop variable patterns
-        self.loop_vars = {'$item', '$index', '$is_first', '$is_last', '$iteration', '$loop'}
+        # Both spellings. The runtime registers `item` and `$item` alike, and
+        # knowing only the `$` form meant `{{ item.name }}` -- the form every
+        # example actually uses -- was reported as an undefined variable
+        # (#469). Declared in `core.template_globals` so the data-flow
+        # validator and this one cannot drift apart about them.
+        self.loop_vars = ALL_LOOP_VARIABLES
         self.step_result_pattern = re.compile(r'([a-zA-Z_][a-zA-Z0-9_]*)\.(result|results|output|outputs|content|data)')
 
         logger.info("TemplateValidator initialized")
@@ -353,20 +363,27 @@ def _validate_variables(
             # Find all variable references
             var_names = meta.find_undeclared_variables(ast)
 
-            # Also look for loop variables manually (since they start with $)
-            loop_var_matches = []
-            for loop_var in self.loop_vars:
-                if loop_var in template:
-                    loop_var_matches.append(loop_var)
+            # `$item` is not a name Jinja can parse, so it never appears in
+            # the AST and has to be matched as raw text. That substring scan
+            # stays confined to the `$` spellings: applied to the bare names
+            # it would match `item` inside `items` and `item_count`, which is
+            # the text-matching class of bug #458 removed.
+            loop_var_matches = [
+                loop_var for loop_var in DOLLAR_LOOP_VARIABLES
+                if loop_var in template
+            ]
 
             # Combine both sets of variables
             all_var_names = set(var_names) | set(loop_var_matches)
 
             for var_name in all_var_names:
                 used_variables.add(var_name)
 
-                # Check if it's a loop variable
-                if var_name in self.loop_vars:
+                # Check if it's a loop variable. A pipeline that declares an
+                # input of its own called `item` means that input, so a
+                # declared name wins -- otherwise adding these would reject
+                # the pipeline that named its parameter after a loop word.
+                if var_name in self.loop_vars and var_name not in available_context:
                     if not in_loop_context:
                         errors.append(TemplateValidationError(
                             template=template,
@@ -555,7 +572,7 @@ def _validate_object_templates(
 
         elif isinstance(obj, dict):
             # Check if we're entering a loop context
-            is_loop = 'for_each' in obj or 'while' in obj
+            is_loop = any(key in obj for key in LOOP_STEP_KEYS)
 
             for key, value in obj.items():
                 new_path = f"{path}.{key}" if path else key

tests/test_catalogue_report.py

@@ -166,3 +166,22 @@ def test_the_list_is_not_empty():
     assert report.load_baseline(), (
         "an empty baseline would make the regression gate vacuous"
     )
+
+
+def test_updating_an_already_current_baseline_changes_nothing():
+    """`--update` rewrites the whole file, header included.
+
+    The header was edited by hand when examples/supported/ landed, so the
+    next `--update` silently deleted the paragraph explaining that passing
+    validation is not the same as being a supported example. Generated files
+    have no room for hand-written parts; this pins the two together.
+    """
+    current = report.BASELINE.read_text()
+    listed = report.load_baseline()
+
+    assert current == report.BASELINE_HEADER + "".join(
+        f"{path}\n" for path in sorted(listed)
+    ), (
+        "the baseline on disk is not what --update would write, so the next "
+        "--update will silently discard the difference"
+    )

tests/test_loop_variables.py

@@ -0,0 +1,180 @@
+"""A loop variable is a loop variable in both spellings, in every loop.
+
+`{{ item.name }}` inside a `for_each` step is bound by the runtime. The
+data-flow validator accepted it; the template validator knew only the
+`$`-prefixed spelling::
+
+    self.loop_vars = {'$item', '$index', '$is_first', '$is_last', ...}
+
+so the bare form -- the one every example in the catalogue actually uses --
+was reported as `Undefined variable: 'item'` (#469). That is the false
+positive class removed in #448 (`slugify`), #450 (`now`), #454
+(`execution.timestamp`) and #459 (`pipeline_id`): a name the runtime provides,
+rejected by validation.
+
+Adding the bare names then exposed a second disagreement. The walker decided a
+step was a loop with ``'for_each' in obj or 'while' in obj``, so a step
+written with ``foreach`` -- an alias the compiler accepts -- was not a loop as
+far as the validator was concerned, and its loop variables became "used
+outside of loop context". The message changed; the pipeline was still wrongly
+rejected.
+
+Both name sets are now declared once, in `core.template_globals`, with the
+rest of the pipeline language.
+"""
+
+import subprocess
+import sys
+import os
+from pathlib import Path
+
+import pytest
+
+from orchestrator.core.template_globals import (
+    ALL_LOOP_VARIABLES,
+    DOLLAR_LOOP_VARIABLES,
+    LOOP_STEP_KEYS,
+    LOOP_VARIABLES,
+)
+from orchestrator.validation.template_validator import TemplateValidator
+
+pytestmark = [pytest.mark.contract]
+
+REPO = Path(__file__).resolve().parent.parent
+
+
+def _check(template, context=None, in_loop=False):
+    result = TemplateValidator().validate_template(
+        template, context or {}, None, [], in_loop
+    )
+    return result.is_valid, [error.error_type for error in result.errors]
+
+
+# ---------------------------------------------------------------------------
+# The bare spelling is the one people write
+# ---------------------------------------------------------------------------
+
+@pytest.mark.parametrize("name", sorted(LOOP_VARIABLES))
+def test_every_loop_variable_is_accepted_inside_a_loop(name):
+    valid, errors = _check("{{ %s }}" % name, in_loop=True)
+    assert valid, f"{name} is bound inside a loop and was rejected: {errors}"
+
+
+def test_an_attribute_of_a_loop_variable_is_accepted():
+    """`{{ item.name }}` is how a loop over objects is actually written."""
+    assert _check("{{ item.name }}", in_loop=True)[0]
+
+
+def test_a_loop_variable_is_still_refused_outside_a_loop():
+    """The gate that makes accepting the bare names safe.
+
+    Without it, adding these names would trade a loud false positive for a
+    silent false negative -- the trade rejected in #461.
+    """
+    valid, errors = _check("{{ item.name }}", in_loop=False)
+    assert not valid
+    assert "loop_variable_outside_loop" in errors, errors
+
+
+def test_a_declared_input_named_item_wins():
+    """A pipeline may name a parameter after a loop word.
+
+    The loop check runs before the context lookup, so without this a pipeline
+    declaring `item` would be told its own parameter is a misplaced loop
+    variable.
+    """
+    assert _check("{{ item }}", {"item": "a declared input"}, in_loop=False)[0]
+
+
+@pytest.mark.parametrize("name", ["item_count", "items", "indexed", "iterations"])
+def test_a_name_that_merely_contains_a_loop_word_is_not_one(name):
+    """The `$` spellings are matched as raw text, because Jinja cannot parse
+    `$item`. Applying that substring scan to the bare names would match `item`
+    inside `items` -- the text-matching class of bug #458 removed."""
+    assert _check("{{ %s }}" % name, {name: 1}, in_loop=False)[0]
+
+
+def test_the_dollar_scan_covers_only_the_dollar_spellings():
+    """A guard on the above, at the source rather than through behaviour."""
+    assert all(name.startswith("$") for name in DOLLAR_LOOP_VARIABLES)
+    assert ALL_LOOP_VARIABLES == LOOP_VARIABLES | DOLLAR_LOOP_VARIABLES
+
+
+# ---------------------------------------------------------------------------
+# Every way of writing a loop counts as one
+# ---------------------------------------------------------------------------
+
+@pytest.mark.parametrize("key", LOOP_STEP_KEYS)
+def test_every_loop_step_key_establishes_loop_context(key):
+    """`foreach` is an alias the compiler accepts. A validator that knows only
+    `for_each` rejects a pipeline the compiler is happy to run."""
+    validator = TemplateValidator()
+    pipeline = {
+        "id": "p",
+        "steps": [{
+            "id": "looped",
+            key: "{{ some_source }}",
+            "parameters": {"text": "{{ item.name }}"},
+        }],
+    }
+    result = validator.validate_pipeline_templates(pipeline)
+    offending = [
+        error for error in result.errors
+        if error.error_type in ("loop_variable_outside_loop", "undefined_variable")
+        and "item" in error.message
+    ]
+    assert not offending, (
+        f"a step declaring '{key}' is a loop, but `item` was reported: "
+        f"{[e.message for e in offending]}"
+    )
+
+
+def test_jinja_s_own_loop_object_is_not_confused_with_ours():
+    """`{% for r in xs %}{{ loop.index }}{% endfor %}` binds `loop` itself.
+
+    Jinja's own analysis already excludes it, so `loop` being in our set must
+    not start reporting it.
+    """
+    assert _check(
+        "{% for r in xs %}{{ loop.index }}{% endfor %}", {"xs": []}, in_loop=False
+    )[0]
+
+
+# ---------------------------------------------------------------------------
+# One declaration
+# ---------------------------------------------------------------------------
+
+def test_both_validators_use_the_same_declaration():
+    """Two sets of loop names is what produced #469.
+
+    Identity, not equality: an equal copy can drift, and the point is that
+    there is nothing to drift from.
+    """
+    from orchestrator.validation.data_flow_validator import LOOP_VARIABLES as data_flow
+
+    assert data_flow is LOOP_VARIABLES
+
+
+def test_the_template_validator_uses_the_shared_set():
+    assert TemplateValidator().loop_vars == ALL_LOOP_VARIABLES
+
+
+# ---------------------------------------------------------------------------
+# The catalogue files this was found in
+# ---------------------------------------------------------------------------
+
+@pytest.mark.e2e
+@pytest.mark.parametrize(
+    "example", ["examples/fact_checker.yaml", "examples/enhanced/fact_checker_enhanced.yaml"]
+)
+def test_the_examples_that_exposed_this_now_validate(example):
+    """Found by triaging the catalogue's largest failure group rather than by
+    reading the code."""
+    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", example],
+        cwd=str(REPO), env=env, capture_output=True, text=True, timeout=300,
+    )
+    assert result.returncode == 0, result.stdout[-600:] + result.stderr[-600:]