From 5a38b9ab2dd65184a094bcbdcb7662dfc67b9b2f Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Tue, 4 Aug 2026 09:15:04 -0400 Subject: [PATCH] Accept loop variables in the spelling examples actually use (#469) The runtime binds each loop variable under two names: item and $item. The template validator knew only the $-prefixed set: self.loop_vars = {'$item', '$index', '$is_first', ...} so {{ item.name }} inside a for_each step -- the form every example in the catalogue writes -- was reported as "Undefined variable: 'item'". That is the same 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 exposed a second disagreement underneath. 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 stayed wrongly rejected. A validator that knows one spelling and a compiler that accepts another is #465 one layer down. Both name sets now live in core.template_globals with the rest of the pipeline language, and data_flow_validator imports the same object rather than keeping an equal copy. The gate that makes this safe already existed: in_loop_context. A loop variable outside a loop is still an error, and a pipeline declaring an input named item still wins over the loop reading. The prediction in issue #469 that this trade would introduce a false negative was wrong. Also fixes a destructive bug in the baseline writer found while updating it. The file is generated in full, header included, but its header had been extended by hand when examples/supported/ landed -- so --update silently deleted the paragraph distinguishing "passes validation" from "is a supported example". The header moves into BASELINE_HEADER, and a test now asserts the bytes on disk equal what --update would write, failing loudly on any hand-edit. Catalogue: 50 -> 52 validating, no listed example regressed. Blocking suite: 868 passed. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/catalogue_report.py | 28 ++- scripts/catalogue_validation_baseline.txt | 2 + src/orchestrator/core/template_globals.py | 36 ++++ .../validation/data_flow_validator.py | 8 +- .../validation/template_validator.py | 37 +++- tests/test_catalogue_report.py | 19 ++ tests/test_loop_variables.py | 180 ++++++++++++++++++ 7 files changed, 288 insertions(+), 22 deletions(-) create mode 100644 tests/test_loop_variables.py diff --git a/scripts/catalogue_report.py b/scripts/catalogue_report.py index 3e48bb6..0456002 100644 --- a/scripts/catalogue_report.py +++ b/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)) ) diff --git a/scripts/catalogue_validation_baseline.txt b/scripts/catalogue_validation_baseline.txt index 18db8ee..27d4f13 100644 --- a/scripts/catalogue_validation_baseline.txt +++ b/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 diff --git a/src/orchestrator/core/template_globals.py b/src/orchestrator/core/template_globals.py index a4d1929..352ea58 100644 --- a/src/orchestrator/core/template_globals.py +++ b/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} diff --git a/src/orchestrator/validation/data_flow_validator.py b/src/orchestrator/validation/data_flow_validator.py index 507e4a7..fd3fbfd 100644 --- a/src/orchestrator/validation/data_flow_validator.py +++ b/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*\]""") diff --git a/src/orchestrator/validation/template_validator.py b/src/orchestrator/validation/template_validator.py index 8208b07..530c7b4 100644 --- a/src/orchestrator/validation/template_validator.py +++ b/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,11 +363,15 @@ 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) @@ -365,8 +379,11 @@ def _validate_variables( 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 diff --git a/tests/test_catalogue_report.py b/tests/test_catalogue_report.py index 1675392..05a1f98 100644 --- a/tests/test_catalogue_report.py +++ b/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" + ) diff --git a/tests/test_loop_variables.py b/tests/test_loop_variables.py new file mode 100644 index 0000000..75598ab --- /dev/null +++ b/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:]