From b36b9dbc77b96d799501719a5a7b2ef00e837f7a Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Tue, 4 Aug 2026 09:40:10 -0400 Subject: [PATCH] Give each loop construct its own bindings and scope (#472) Validation treated "inside a loop" as one boolean and one union of names. Both halves were wrong. The boolean reached every field of a loop-shaped step, including the iterable that introduces the loop, so this was accepted: - id: process for_each: "{{ item.children }}" # no item exists yet parameters: text: "{{ item.name }}" # correct The iterable must resolve before there is an item to bind. A loop variable in it can never work, and validation said nothing. The union gave every construct every name. `while` binds no `is_last`; only `create_parallel_queue` binds `queue`. So `{{ is_last }}` inside a `while` loop passed validation and then failed to render -- the false negative #470 introduced while removing a false positive, and which that PR's description claimed was impossible. The correction is posted on #469. core.loop_contracts holds one table per construct, each declaring the fields evaluated before the loop exists and the names bound inside it. The tables were read from the runtime -- context_manager, loops.py, parallel_queue_task and action_loop_context -- rather than from docs/loop_variables.md, which claims names the runtime does not bind in every construct. A rejection now names what the construct does bind, so it is actionable rather than a puzzle. template_globals derives its flat sets from the contracts instead of keeping a second copy: ALL_LOOP_VARIABLES is ALL_BINDINGS by identity, and LOOP_STEP_KEYS is tuple(LOOP_CONTRACTS), so a key cannot be recognised as a loop without declaring what it binds. Strengthening #470's alias test to require full validity, rather than merely the absence of errors naming `item`, immediately failed for all five constructs: it probed every one with `item`, which only passed because every construct saw the union. It now probes each construct with a name its own contract binds. Mutations: routing source fields through body scope fails exactly the two source-scope tests; giving every construct the union fails exactly the four per-construct tests. Blocking suite: 916 passed. Catalogue: all 52 still validate. Co-Authored-By: Claude Opus 5 (1M context) --- src/orchestrator/core/loop_contracts.py | 145 +++++++++++ src/orchestrator/core/template_globals.py | 45 ++-- .../validation/template_validator.py | 88 +++++-- tests/test_loop_contracts.py | 237 ++++++++++++++++++ tests/test_loop_variables.py | 59 +++-- 5 files changed, 509 insertions(+), 65 deletions(-) create mode 100644 src/orchestrator/core/loop_contracts.py create mode 100644 tests/test_loop_contracts.py diff --git a/src/orchestrator/core/loop_contracts.py b/src/orchestrator/core/loop_contracts.py new file mode 100644 index 0000000..eb4e380 --- /dev/null +++ b/src/orchestrator/core/loop_contracts.py @@ -0,0 +1,145 @@ +"""What each loop construct binds, and where those bindings reach. + +Validation treated "inside a loop" as one boolean and one union of names. Two +things follow from that, both wrong. + +**The source expression was validated in loop scope.** `_validate_object_templates` +computed `is_loop` from the step dictionary and passed it down to every child, +including the iterable itself:: + + - id: process + for_each: "{{ item.children }}" # accepted -- but no item exists yet + parameters: + text: "{{ item.name }}" # correct + +The iterable must resolve before there is an item to bind. A reference to a +loop variable in it can never work, and validation said nothing. + +**Every construct got every name.** `while` does not bind `item`; only +`create_parallel_queue` binds `queue`. Accepting the union meant +``{{ is_last }}`` inside a `while` loop passed validation and then failed to +render -- the false negative #470 introduced while removing a false positive. + +The tables below were read from the runtime rather than from +`docs/loop_variables.md`, which claims names the runtime does not bind in +every construct: + +`for_each` / `foreach` + `ControlSystem._render_task_templates` registers `item`, `index`, + `is_first` and `is_last` from `metadata["loop_context"]`; + `ContextManager.loop_context` supplies `item`, `index` and `loop_id`. +`while` + `WhileLoopHandler` builds `iteration`, `index`, `is_first`, `position`, + `loop_state` and `loop_id`. +`create_parallel_queue` + `ParallelQueueTask.get_template_variables` adds `queue`, `queue_size`, + `parallel_queue_id` and `parent_task` alongside the per-item names. +`action_loop` + `ActionLoopContext` exposes iteration state and previous-result metadata, + but no item: an action loop is not iterating a collection. + +Each construct's `source_fields` are evaluated before the loop exists, so they +are validated in the enclosing scope. Everything else in the step is body, and +sees that construct's bindings. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Any, Dict, FrozenSet, Optional, Tuple + +#: Jinja's own loop object, bound by `{% for %}` inside a template rather than +#: by a pipeline construct. Jinja's undeclared-name analysis already accounts +#: for it; it is listed so that a step-level loop does not report it either. +JINJA_LOOP: FrozenSet[str] = frozenset({"loop"}) + + +@dataclass(frozen=True) +class LoopContract: + """One loop construct: what it binds, and which fields see the bindings.""" + + key: str + #: Fields evaluated before the loop exists. Validated in the outer scope. + source_fields: Tuple[str, ...] + #: Names available to the body, in the bare spelling. The runtime also + #: registers a `$`-prefixed alias for each. + bindings: FrozenSet[str] + + def dollar_bindings(self) -> FrozenSet[str]: + return frozenset(f"${name}" for name in self.bindings) + + def all_bindings(self) -> FrozenSet[str]: + return self.bindings | self.dollar_bindings() | JINJA_LOOP + + +FOR_EACH = LoopContract( + key="for_each", + source_fields=("for_each", "foreach"), + bindings=frozenset({"item", "index", "is_first", "is_last"}), +) + +#: `foreach` is the same construct under the spelling the declarative engine +#: accepts. One contract, two keys -- the compiler treats them identically, so +#: validation must too. +FOREACH = LoopContract( + key="foreach", + source_fields=FOR_EACH.source_fields, + bindings=FOR_EACH.bindings, +) + +WHILE = LoopContract( + key="while", + source_fields=("while", "until"), + bindings=frozenset({ + "iteration", "index", "is_first", "position", "loop_state", "loop_id", + }), +) + +CREATE_PARALLEL_QUEUE = LoopContract( + key="create_parallel_queue", + source_fields=("create_parallel_queue",), + bindings=frozenset({ + "item", "index", "queue", "queue_size", "is_first", "is_last", + "parallel_queue_id", "parent_task", + }), +) + +ACTION_LOOP = LoopContract( + key="action_loop", + source_fields=("action_loop", "until"), + bindings=frozenset({ + "loop_id", "iteration", "is_first", "has_previous", "total_duration", + "termination_reason", + }), +) + +LOOP_CONTRACTS: Dict[str, LoopContract] = { + contract.key: contract + for contract in (FOR_EACH, FOREACH, WHILE, CREATE_PARALLEL_QUEUE, ACTION_LOOP) +} + +#: Every name any construct binds. Used where a construct is not known -- never +#: as a substitute for the per-construct set, which is what made +#: `{{ is_last }}` acceptable inside a `while` loop. +ALL_BINDINGS: FrozenSet[str] = frozenset().union( + *(contract.all_bindings() for contract in LOOP_CONTRACTS.values()) +) + + +def contract_for(step: Any) -> Optional[LoopContract]: + """The loop contract a step declares, or None if it is not a loop. + + Checked in declaration order of `LOOP_CONTRACTS` so a step carrying more + than one loop key resolves deterministically. + """ + if not isinstance(step, dict): + return None + for key, contract in LOOP_CONTRACTS.items(): + if key in step: + return contract + return None + + +def is_source_field(contract: Optional[LoopContract], field: str) -> bool: + """Whether `field` is evaluated before the loop's bindings exist.""" + return contract is not None and field in contract.source_fields diff --git a/src/orchestrator/core/template_globals.py b/src/orchestrator/core/template_globals.py index 352ea58..9cddd59 100644 --- a/src/orchestrator/core/template_globals.py +++ b/src/orchestrator/core/template_globals.py @@ -41,6 +41,8 @@ from dataclasses import dataclass from typing import Any, FrozenSet, List, Optional, Tuple +from .loop_contracts import ALL_BINDINGS, LOOP_CONTRACTS + @dataclass(frozen=True) class GlobalSpec: @@ -108,40 +110,33 @@ 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 bindings, projected from the per-construct contracts. +#: +#: These names were once written out here as one flat tuple, which said that +#: every loop binds the same thing. It does not: `while` binds no `item`, and +#: only `create_parallel_queue` binds `queue`. `core.loop_contracts` holds the +#: real tables, and anything needing "a name some loop binds" -- rather than +#: "a name *this* loop binds" -- reads the union below. +#: +#: Prefer `loop_contracts.contract_for(step)` wherever the construct is known. +#: The union cannot tell `{{ is_last }}` in a `create_parallel_queue` from the +#: same text in a `while` loop, and that distinction is the whole point. +LOOP_VARIABLES: FrozenSet[str] = frozenset( + name for name in ALL_BINDINGS if not name.startswith("$") ) -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 + name for name in ALL_BINDINGS if name.startswith("$") ) -ALL_LOOP_VARIABLES: FrozenSet[str] = LOOP_VARIABLES | DOLLAR_LOOP_VARIABLES +ALL_LOOP_VARIABLES: FrozenSet[str] = ALL_BINDINGS - -#: 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", -) +#: Step keys that make a step a loop. One per contract, so a construct cannot +#: be recognised as a loop without also declaring what it binds. +LOOP_STEP_KEYS: Tuple[str, ...] = tuple(LOOP_CONTRACTS) _BY_NAME = {spec.name: spec for spec in GLOBAL_SPECS} diff --git a/src/orchestrator/validation/template_validator.py b/src/orchestrator/validation/template_validator.py index 530c7b4..2fbb5b4 100644 --- a/src/orchestrator/validation/template_validator.py +++ b/src/orchestrator/validation/template_validator.py @@ -12,16 +12,16 @@ import logging import re -from typing import Any, Dict, List, Optional, Set, Union +from typing import Any, Dict, FrozenSet, List, Optional, Set, Union from dataclasses import dataclass from jinja2 import Environment, TemplateSyntaxError, meta from jinja2.sandbox import SandboxedEnvironment from ..core.runtime_context import BARE_RUNTIME_NAMES, RUNTIME_NAMESPACE +from ..core.loop_contracts import ALL_BINDINGS, contract_for, is_source_field 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 @@ -36,6 +36,22 @@ PARAMETER_NAMESPACES = frozenset({"parameters", "inputs"}) +def _binding_set(value: Union[bool, FrozenSet[str], None]) -> FrozenSet[str]: + """Normalise the loop-scope argument to a set of names. + + `True` and `False` are still accepted because the parameter began life as + a boolean. `True` means "inside some loop, construct unknown" and admits + the union -- the imprecision this module is moving away from, kept only so + an external caller that has not been updated does not start reporting + every loop variable as undefined. + """ + if value is True: + return ALL_BINDINGS + if not value: + return frozenset() + return frozenset(value) + + @dataclass class TemplateValidationError: """Represents a template validation error.""" @@ -140,20 +156,26 @@ def validate_template( available_context: Optional[Dict[str, Any]] = None, context_path: Optional[str] = None, step_ids: Optional[List[str]] = None, - in_loop_context: bool = False + loop_bindings: Union[bool, FrozenSet[str]] = frozenset(), ) -> TemplateValidationResult: """Validate a single template string. - + Args: template: Template string to validate available_context: Context variables available at compile time context_path: Path to this template (for error reporting) step_ids: List of step IDs in the pipeline - in_loop_context: Whether this template is inside a loop - + loop_bindings: The names this template's loop binds. Empty means + not inside a loop. `True` is accepted as "some loop, construct + unknown" and admits the union of every construct's bindings -- + which cannot tell `{{ is_last }}` in a parallel queue from the + same text in a `while` loop, so pass the real set where the + construct is known. + Returns: TemplateValidationResult with validation details """ + loop_bindings = _binding_set(loop_bindings) if available_context is None: available_context = {} if step_ids is None: @@ -197,7 +219,7 @@ def validate_template( # 2. Extract and validate variable references var_results = self._validate_variables( - template, available_context, context_path, step_ids, in_loop_context + template, available_context, context_path, step_ids, loop_bindings ) errors.extend(var_results['errors']) warnings.extend(var_results['warnings']) @@ -327,7 +349,7 @@ def _validate_variables( available_context: Dict[str, Any], context_path: Optional[str], step_ids: List[str], - in_loop_context: bool + loop_bindings: FrozenSet[str], ) -> Dict[str, Any]: """Validate variable references in template.""" errors = [] @@ -384,14 +406,35 @@ def _validate_variables( # 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: + if var_name in loop_bindings: + continue + if loop_bindings: + # Inside a loop, but not one that binds this name. + # `while` has no `item`; only a parallel queue has + # `queue`. Accepting the union here is what let + # `{{ is_last }}` pass inside a `while` loop and then + # fail to render. + bound = ", ".join( + sorted(n for n in loop_bindings if not n.startswith("$")) + ) errors.append(TemplateValidationError( template=template, - error_type="loop_variable_outside_loop", - message=f"Loop variable '{var_name}' used outside of loop context", + error_type="loop_variable_wrong_construct", + message=( + f"Loop variable '{var_name}' is not bound by this " + f"loop construct" + ), context_path=context_path, - suggestions=["Move this template inside a for_each loop"] + suggestions=[f"This loop binds: {bound}"], )) + continue + errors.append(TemplateValidationError( + template=template, + error_type="loop_variable_outside_loop", + message=f"Loop variable '{var_name}' used outside of loop context", + context_path=context_path, + suggestions=["Move this template inside a for_each loop"] + )) continue # Check if it's a step result reference @@ -556,14 +599,14 @@ def _validate_object_templates( warnings: List, used_variables: Set, undefined_variables: Set, - in_loop_context: bool = False + loop_bindings: FrozenSet[str] = frozenset(), ): """Recursively validate templates in an object.""" if isinstance(obj, str): # Check if this contains templates if '{{' in obj or '{%' in obj: result = self.validate_template( - obj, context, path, step_ids, in_loop_context + obj, context, path, step_ids, loop_bindings ) errors.extend(result.errors) warnings.extend(result.warnings) @@ -571,15 +614,22 @@ def _validate_object_templates( undefined_variables.update(result.undefined_variables) elif isinstance(obj, dict): - # Check if we're entering a loop context - is_loop = any(key in obj for key in LOOP_STEP_KEYS) - + # A loop's source expression is evaluated before the loop exists, + # so it does not see the loop's own bindings. Passing one boolean + # down to every field accepted `for_each: "{{ item.children }}"`, + # which can never resolve: there is no item until the iterable + # has been evaluated. + contract = contract_for(obj) + body_bindings = ( + loop_bindings | contract.all_bindings() if contract else loop_bindings + ) + for key, value in obj.items(): new_path = f"{path}.{key}" if path else key self._validate_object_templates( value, context, step_ids, new_path, errors, warnings, used_variables, undefined_variables, - in_loop_context or is_loop + loop_bindings if is_source_field(contract, key) else body_bindings, ) elif isinstance(obj, list): @@ -588,7 +638,7 @@ def _validate_object_templates( self._validate_object_templates( item, context, step_ids, new_path, errors, warnings, used_variables, undefined_variables, - in_loop_context + loop_bindings, ) def _is_step_result_reference(self, var_name: str, step_ids: List[str]) -> bool: diff --git a/tests/test_loop_contracts.py b/tests/test_loop_contracts.py new file mode 100644 index 0000000..24a75a6 --- /dev/null +++ b/tests/test_loop_contracts.py @@ -0,0 +1,237 @@ +"""A loop binds particular names, in particular places. + +Validation treated "inside a loop" as one boolean and one union of names, and +both halves were wrong. + +The boolean reached every field of a loop-shaped step, including the iterable +itself, so this was accepted:: + + - id: process + for_each: "{{ item.children }}" # no item exists yet + parameters: + text: "{{ item.name }}" + +The union meant every construct got every name, so ``{{ is_last }}`` inside a +`while` loop -- which binds no `is_last` -- passed validation and then failed +to render. That false negative arrived with #470, whose description claimed +the opposite; the correction is on #469. + +`core.loop_contracts` holds one table per construct, read from the runtime +rather than from `docs/loop_variables.md`, which claims names the runtime does +not bind in every construct. +""" + +import pytest + +from orchestrator.core.loop_contracts import ( + ACTION_LOOP, + ALL_BINDINGS, + CREATE_PARALLEL_QUEUE, + FOR_EACH, + LOOP_CONTRACTS, + WHILE, + contract_for, + is_source_field, +) +from orchestrator.validation.template_validator import TemplateValidator + +pytestmark = [pytest.mark.contract] + + +def _errors(step, extra_context=None): + result = TemplateValidator().validate_pipeline_templates( + {"id": "p", "steps": [step]}, + extra_context or {}, + ) + return [(e.error_type, e.context_path) for e in result.errors] + + +# --------------------------------------------------------------------------- +# Source expressions are outside the loop they introduce +# --------------------------------------------------------------------------- + +def test_a_loop_variable_in_the_iterable_is_rejected(): + """The reported case. The iterable must resolve before an item exists.""" + kinds = [k for k, _ in _errors( + {"id": "s", "for_each": "{{ item.children }}", "parameters": {"t": "x"}} + )] + assert "loop_variable_outside_loop" in kinds, kinds + + +def test_the_error_points_at_the_iterable_not_the_body(): + paths = [p for k, p in _errors( + {"id": "s", "for_each": "{{ item.children }}", "parameters": {"t": "x"}} + ) if k == "loop_variable_outside_loop"] + assert paths == ["steps[0].for_each"], paths + + +def test_the_same_name_is_accepted_in_the_body(): + """Source and body differ, so one template must not decide the other.""" + kinds = [k for k, _ in _errors( + {"id": "s", "for_each": "{{ rows }}", "parameters": {"t": "{{ item.name }}"}}, + {"rows": [1]}, + )] + assert kinds == [], kinds + + +@pytest.mark.parametrize("contract", sorted(set(LOOP_CONTRACTS.values()), key=lambda c: c.key)) +def test_every_construct_declares_its_source_fields(contract): + assert contract.source_fields, ( + f"{contract.key} declares no source field, so its iterable or " + f"condition would be validated in loop scope" + ) + for field in contract.source_fields: + assert is_source_field(contract, field) + + +# --------------------------------------------------------------------------- +# Constructs bind different names +# --------------------------------------------------------------------------- + +def test_a_name_another_construct_binds_is_rejected(): + """`while` has no `is_last`. Accepting the union is what let this pass.""" + kinds = [k for k, _ in _errors( + {"id": "s", "while": "{{ go }}", "parameters": {"t": "{{ is_last }}"}}, + {"go": True}, + )] + assert "loop_variable_wrong_construct" in kinds, kinds + + +def test_the_message_names_what_this_loop_does_bind(): + """A rejection that does not say what is available is a puzzle.""" + result = TemplateValidator().validate_pipeline_templates( + {"id": "p", "steps": [ + {"id": "s", "while": "{{ go }}", "parameters": {"t": "{{ is_last }}"}} + ]}, + {"go": True}, + ) + wrong = [e for e in result.errors if e.error_type == "loop_variable_wrong_construct"] + assert wrong and "iteration" in wrong[0].suggestions[0], wrong[0].suggestions + + +@pytest.mark.parametrize("name", sorted(WHILE.bindings)) +def test_while_accepts_what_while_binds(name): + kinds = [k for k, _ in _errors( + {"id": "s", "while": "{{ go }}", "parameters": {"t": "{{ %s }}" % name}}, + {"go": True}, + )] + assert kinds == [], f"`while` binds {name} at run time but validation rejected it: {kinds}" + + +@pytest.mark.parametrize("name", sorted(FOR_EACH.bindings)) +def test_for_each_accepts_what_for_each_binds(name): + kinds = [k for k, _ in _errors( + {"id": "s", "for_each": "{{ rows }}", "parameters": {"t": "{{ %s }}" % name}}, + {"rows": [1]}, + )] + assert kinds == [], kinds + + +def test_a_queue_only_name_is_rejected_in_a_for_each(): + kinds = [k for k, _ in _errors( + {"id": "s", "for_each": "{{ rows }}", "parameters": {"t": "{{ queue_size }}"}}, + {"rows": [1]}, + )] + assert "loop_variable_wrong_construct" in kinds, kinds + + +def test_a_queue_only_name_is_accepted_in_a_queue(): + kinds = [k for k, _ in _errors( + { + "id": "s", + "create_parallel_queue": {"on": "{{ rows }}"}, + "parameters": {"t": "{{ queue_size }}"}, + }, + {"rows": [1]}, + )] + assert kinds == [], kinds + + +def test_an_action_loop_binds_no_item(): + """An action loop repeats actions; it is not iterating a collection.""" + assert "item" not in ACTION_LOOP.bindings + kinds = [k for k, _ in _errors( + {"id": "s", "action_loop": [{"action": "noop"}], "parameters": {"t": "{{ item }}"}} + )] + assert "loop_variable_wrong_construct" in kinds, kinds + + +# --------------------------------------------------------------------------- +# Scope still behaves +# --------------------------------------------------------------------------- + +def test_a_loop_variable_outside_any_loop_is_still_rejected(): + kinds = [k for k, _ in _errors({"id": "s", "parameters": {"t": "{{ item }}"}})] + assert "loop_variable_outside_loop" in kinds, kinds + + +def test_a_declared_input_named_item_still_wins(): + kinds = [k for k, _ in _errors( + {"id": "s", "parameters": {"t": "{{ item }}"}}, {"item": "declared"} + )] + assert kinds == [], kinds + + +def test_a_nested_loop_sees_both_constructs(): + """An inner loop does not hide the outer one's bindings.""" + kinds = [k for k, _ in _errors( + { + "id": "outer", + "for_each": "{{ rows }}", + "steps": [{ + "id": "inner", + "while": "{{ go }}", + "parameters": {"t": "{{ item }} {{ iteration }}"}, + }], + }, + {"rows": [1], "go": True}, + )] + assert kinds == [], kinds + + +# --------------------------------------------------------------------------- +# One declaration +# --------------------------------------------------------------------------- + +def test_the_union_is_built_from_the_contracts(): + from orchestrator.core.template_globals import ALL_LOOP_VARIABLES + + assert ALL_LOOP_VARIABLES is ALL_BINDINGS, ( + "the flat list must be a projection of the contracts, not a second " + "copy that can drift" + ) + + +def test_every_loop_step_key_has_a_contract(): + from orchestrator.core.template_globals import LOOP_STEP_KEYS + + assert set(LOOP_STEP_KEYS) == set(LOOP_CONTRACTS), ( + "a key recognised as a loop without a contract would fall back to the " + "union, which is the imprecision this replaces" + ) + + +def test_foreach_is_the_same_construct_as_for_each(): + """The compiler treats them identically, so validation must too.""" + assert LOOP_CONTRACTS["foreach"].bindings == FOR_EACH.bindings + assert contract_for({"foreach": "x"}).bindings == FOR_EACH.bindings + + +def test_a_step_with_no_loop_key_has_no_contract(): + assert contract_for({"id": "s", "parameters": {}}) is None + assert not is_source_field(None, "for_each") + + +@pytest.mark.parametrize("contract", sorted(set(LOOP_CONTRACTS.values()), key=lambda c: c.key)) +def test_both_spellings_are_bound(contract): + """The runtime registers `item` and `$item` alike.""" + for name in contract.bindings: + assert f"${name}" in contract.all_bindings() + + +def test_the_parallel_queue_binds_more_than_the_others(): + """A guard on the tables being genuinely different -- if every construct + ended up with the same set, the per-construct split would be decorative.""" + assert CREATE_PARALLEL_QUEUE.bindings - WHILE.bindings + assert WHILE.bindings - CREATE_PARALLEL_QUEUE.bindings + assert FOR_EACH.bindings != WHILE.bindings diff --git a/tests/test_loop_variables.py b/tests/test_loop_variables.py index 75598ab..de8fa8d 100644 --- a/tests/test_loop_variables.py +++ b/tests/test_loop_variables.py @@ -30,6 +30,7 @@ import pytest +from orchestrator.core.loop_contracts import FOR_EACH, LOOP_CONTRACTS from orchestrator.core.template_globals import ( ALL_LOOP_VARIABLES, DOLLAR_LOOP_VARIABLES, @@ -44,8 +45,15 @@ def _check(template, context=None, in_loop=False): + """`in_loop` means a `for_each` specifically. + + Passing `True` would admit the union of every construct's bindings, + which is what let `{{ is_last }}` pass inside a `while` loop. Tests + that go through the imprecise path cannot notice when it is wrong. + """ + bindings = FOR_EACH.all_bindings() if in_loop else frozenset() result = TemplateValidator().validate_template( - template, context or {}, None, [], in_loop + template, context or {}, None, [], bindings ) return result.is_valid, [error.error_type for error in result.errors] @@ -54,8 +62,11 @@ def _check(template, context=None, in_loop=False): # The bare spelling is the one people write # --------------------------------------------------------------------------- -@pytest.mark.parametrize("name", sorted(LOOP_VARIABLES)) +@pytest.mark.parametrize("name", sorted(FOR_EACH.bindings)) def test_every_loop_variable_is_accepted_inside_a_loop(name): + """`FOR_EACH.bindings`, not the union: a `for_each` binds `item`, not + `queue_size`, and asserting over the union would pass for the wrong + reason.""" valid, errors = _check("{{ %s }}" % name, in_loop=True) assert valid, f"{name} is bound inside a loop and was rejected: {errors}" @@ -107,25 +118,31 @@ def test_the_dollar_scan_covers_only_the_dollar_spellings(): @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]}" + `for_each` rejects a pipeline the compiler is happy to run. + + Each construct is probed with a name it actually binds. The earlier + version of this test used `item` for all five, which only passed because + every construct saw the union -- `while` and `action_loop` bind no `item`. + Requiring full validity is what exposed that; the weaker "no error + mentions `item`" assertion had been satisfied by a pipeline that was + rejected for an unrelated reason. + """ + contract = LOOP_CONTRACTS[key] + bound = sorted(contract.bindings)[0] + result = TemplateValidator().validate_pipeline_templates( + { + "id": "p", + "steps": [{ + "id": "looped", + key: "{{ some_source }}", + "parameters": {"text": "{{ %s }}" % bound}, + }], + }, + {"some_source": [1]}, + ) + assert result.is_valid, ( + f"a step declaring '{key}' binds {bound}, but the step was rejected: " + f"{[(e.error_type, e.message) for e in result.errors]}" )