Give each loop construct its own bindings and scope (#472)

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

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}
 

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,30 +599,37 @@ 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)
                 used_variables.update(result.used_variables)
                 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: