diff --git a/src/orchestrator/core/template_globals.py b/src/orchestrator/core/template_globals.py index e99a5ee..a4d1929 100644 --- a/src/orchestrator/core/template_globals.py +++ b/src/orchestrator/core/template_globals.py @@ -143,6 +143,8 @@ def find_global_misuse(ast: Any) -> List[GlobalMisuse]: """ from jinja2 import nodes + from .template_scope import shadowed_name_nodes + # A Name node is a legitimate use only when it is the thing being called. # Identity matters here, not the name: `{{ now() and now.foo }}` has two # Name nodes spelled the same, one valid and one not. @@ -154,21 +156,17 @@ def find_global_misuse(ast: Any) -> List[GlobalMisuse]: # `{% for now in items %}{{ now }}{% endfor %}` rebinds the name: the # target is a `store`, but the use inside the body is an ordinary `load` - # and is indistinguishable from ours without tracking scope. A template - # that binds the name anywhere is left alone entirely -- deliberately - # conservative, because a false rejection here is the exact failure the - # last three changes to this validator existed to remove. - shadowed = { - node.name - for node in ast.find_all(nodes.Name) - if getattr(node, "ctx", "load") != "load" - } + # and looks exactly like ours. Which uses the binding actually reaches is + # a question of scope -- a template-wide set of bound names would silence + # `{{ now }}` before and after that loop as well, where it really is our + # global and really is a misuse. + shadowed = shadowed_name_nodes(ast) misuse: List[GlobalMisuse] = [] seen = set() for name_node in ast.find_all(nodes.Name): spec = global_spec(name_node.name) - if spec is None or spec.name in shadowed: + if spec is None or id(name_node) in shadowed: continue if getattr(name_node, "ctx", "load") != "load": continue diff --git a/src/orchestrator/core/template_scope.py b/src/orchestrator/core/template_scope.py new file mode 100644 index 0000000..a9f728e --- /dev/null +++ b/src/orchestrator/core/template_scope.py @@ -0,0 +1,157 @@ +"""Which names in a template are the template's own. + +`{% for row in rows %}` introduces `row`. A validator that reports undefined +references must not report `row`, because the template defines it -- but it +must still report everything the template does *not* define, and the +difference is a matter of scope, not spelling. + +Both validators first approximated this with one template-wide set of every +bound name. That is sound for a name used only where it is bound and wrong +everywhere else, because binding a name *anywhere* silenced it *everywhere*:: + + {{ ghost.done }} <- undefined; reported nothing + {% for ghost in rows %} + {{ ghost.name }} <- the binding that silenced it + {% endfor %} + +The reference outside the loop is exactly the kind of typo a validator exists +to catch, and adding a loop elsewhere in the file made it disappear. A +false negative is worse than the false positive it was introduced to fix: +the false positive was loud. + +So scope is tracked as Jinja tracks it. A binding reaches: + +* the body of the construct that introduces it (`for`, `macro`, `call`, + `with`), and nothing outside that body; +* for `{% set %}`, the statements that *follow* it in the same block -- + Jinja evaluates a template top to bottom, and `{{ x }}{% set x = 1 %}` + really is an undefined reference followed by an assignment. + +`{% if %}` deliberately does not open a scope, because Jinja does not give it +one: a `{% set %}` inside a taken branch is visible after the `{% endif %}`. +Treating the binding as reaching the rest of the block is the conservative +reading -- it can only suppress a report, never invent one. + +Identity, not spelling, is the answer: `shadowed_name_nodes` returns the +`id()` of each `Name` node that refers to a template-local binding, so one +template can hold both a shadowed and an unshadowed use of the same word. +""" + +from __future__ import annotations + +from typing import Any, FrozenSet, MutableSet, Set + +from jinja2 import nodes + +#: Bound by Jinja inside a `{% for %}` body without appearing as a target. +LOOP_IMPLICIT: FrozenSet[str] = frozenset({"loop"}) + +#: Bound by Jinja inside a `{% macro %}` body without appearing in its +#: argument list. +MACRO_IMPLICIT: FrozenSet[str] = frozenset({"varargs", "kwargs", "caller"}) + + +def shadowed_name_nodes(ast: nodes.Node) -> FrozenSet[int]: + """The `id()` of every `Name` node that refers to a template-local binding. + + Callers keep their own traversal and ask this whether a particular node + is the template's own name or a reference to something outside it. + """ + shadowed: Set[int] = set() + _walk(ast, set(), shadowed) + return frozenset(shadowed) + + +def _target_names(target: Any) -> Set[str]: + """The names a binding site introduces. + + A target is a `Name`, or a tuple/list of them for `{% for k, v in ... %}`. + A namespace reference (`{% set ns.x = 1 %}`) binds no new name. + """ + if isinstance(target, nodes.Name): + return {target.name} + if isinstance(target, (nodes.Tuple, nodes.List)): + names: Set[str] = set() + for item in target.items: + names |= _target_names(item) + return names + return set() + + +def _walk_all(children: Any, bound: MutableSet[str], shadowed: MutableSet[int]) -> None: + for child in children: + if child is not None: + _walk(child, bound, shadowed) + + +def _walk(node: Any, bound: MutableSet[str], shadowed: MutableSet[int]) -> None: + """Visit `node`, recording uses of names bound in the enclosing scopes. + + `bound` is mutated in place by `{% set %}` and `{% import %}` so that the + binding reaches the statements after them; constructs that open a scope + pass a copy instead, so their bindings do not escape. + """ + if isinstance(node, nodes.Name): + if getattr(node, "ctx", "load") == "load" and node.name in bound: + shadowed.add(id(node)) + return + + if isinstance(node, nodes.For): + # The iterable is evaluated outside the loop; the filter test is not. + _walk(node.iter, bound, shadowed) + inner = set(bound) | _target_names(node.target) | LOOP_IMPLICIT + _walk_all([node.test, *node.body], inner, shadowed) + # `{% else %}` runs when the iterable was empty, so the target never + # took a value there. + _walk_all(node.else_, bound, shadowed) + return + + if isinstance(node, nodes.Macro): + _walk_all(node.defaults, bound, shadowed) + inner = set(bound) | {arg.name for arg in node.args} | MACRO_IMPLICIT + _walk_all(node.body, inner, shadowed) + # The macro's own name is callable after its definition. + bound.add(node.name) + return + + if isinstance(node, nodes.CallBlock): + _walk(node.call, bound, shadowed) + _walk_all(node.defaults, bound, shadowed) + inner = set(bound) | {arg.name for arg in node.args} + _walk_all(node.body, inner, shadowed) + return + + if isinstance(node, nodes.With): + _walk_all(node.values, bound, shadowed) + inner = set(bound) + for target in node.targets: + inner |= _target_names(target) + _walk_all(node.body, inner, shadowed) + return + + if isinstance(node, nodes.Assign): + # The value is evaluated before the name exists: in `{% set x = x %}` + # the right-hand `x` is whatever `x` meant before this statement. + _walk(node.node, bound, shadowed) + bound |= _target_names(node.target) + return + + if isinstance(node, nodes.AssignBlock): + _walk_all([*node.body, node.filter], bound, shadowed) + bound |= _target_names(node.target) + return + + if isinstance(node, nodes.Import): + _walk(node.template, bound, shadowed) + if node.target: + bound.add(node.target) + return + + if isinstance(node, nodes.FromImport): + _walk(node.template, bound, shadowed) + for name in node.names: + # `{% from 'x' import a as b %}` arrives as the pair ('a', 'b'). + bound.add(name[1] if isinstance(name, tuple) else name) + return + + _walk_all(list(node.iter_child_nodes()), bound, shadowed) diff --git a/src/orchestrator/validation/data_flow_validator.py b/src/orchestrator/validation/data_flow_validator.py index 6ca65a8..4c66a4f 100644 --- a/src/orchestrator/validation/data_flow_validator.py +++ b/src/orchestrator/validation/data_flow_validator.py @@ -12,9 +12,12 @@ import logging import re -from typing import Any, Dict, List, Optional, Set, Tuple, Union +from typing import Any, Dict, List, Optional, Set, Tuple from dataclasses import dataclass, field -from jinja2 import TemplateSyntaxError, Undefined, meta +# `jinja2.meta` was how references used to be found. It reports only top-level +# undeclared names, which is why the chain and subscript forms had to be +# recovered from the raw text afterwards; walking the AST replaced both. +from jinja2 import Undefined from ..core.runtime_context import ( BARE_RUNTIME_NAMES, @@ -22,6 +25,7 @@ RUNTIME_NAMESPACE, ) from ..core.template_sandbox import create_sandboxed_environment, pipeline_global_names +from ..core.template_scope import shadowed_name_nodes logger = logging.getLogger(__name__) @@ -447,13 +451,19 @@ def _extract_template_variables(self, template_str: str) -> List[str]: # simply has no references this validator can be sure of. return [] - # `{% for row in rows %}` binds `row`; it is not a reference to - # anything this validator tracks. - bound = { - node.name - for node in ast.find_all(nodes.Name) - if getattr(node, "ctx", "load") != "load" - } + # `{% for row in rows %}` binds `row` *inside its body*; it is not a + # reference to anything this validator tracks. Which uses the binding + # reaches is a question of scope, and getting that wrong in the + # permissive direction is worse than the false positive it replaced: + # + # {{ ghost.done }} <- undefined, and reported nothing + # {% for ghost in rows %} + # {{ ghost.name }} <- the binding that silenced it + # {% endfor %} + # + # A template-wide set of bound names made adding a loop anywhere in a + # file suppress that typo everywhere in it. + shadowed = shadowed_name_nodes(ast) # `a.b.c` is one reference, not three. Only the outermost node of a # chain is reported; the links inside it are skipped. @@ -470,16 +480,32 @@ def _extract_template_variables(self, template_str: str) -> List[str]: for node in ast.find_all((nodes.Name, nodes.Getattr, nodes.Getitem)): if id(node) in inner: continue - path = self._dotted_path(node) - if not path: + base_node = self._base_name(node) + if base_node is None or id(base_node) in shadowed: continue - base = path.split(".", 1)[0] - if base in bound or base in provided: + if base_node.name in provided: continue - variables.append(path) + path = self._dotted_path(node) + if path: + variables.append(path) return variables + def _base_name(self, node): + """The `Name` node a reference chain starts from. + + `a.b['c']` is a reference to `a`; identity of that particular node is + what decides whether this use is shadowed, since one template can hold + both a shadowed and an unshadowed use of the same word. + """ + from jinja2 import nodes + + while isinstance(node, (nodes.Getattr, nodes.Getitem)): + node = node.node + if isinstance(node, nodes.Name) and getattr(node, "ctx", "load") == "load": + return node + return None + def _dotted_path(self, node) -> Optional[str]: """`a.b`, `a['b']` and `a[0]` as the one name they refer to.""" from jinja2 import nodes diff --git a/tests/test_template_scope.py b/tests/test_template_scope.py new file mode 100644 index 0000000..2d023cc --- /dev/null +++ b/tests/test_template_scope.py @@ -0,0 +1,280 @@ +"""A binding reaches where Jinja says it reaches, and no further. + +Both validators used to answer "is this name the template's own?" with one +template-wide set of every bound name. That is right for a name used only +where it is bound and wrong everywhere else, because binding a name *anywhere* +silenced it *everywhere*:: + + {{ ghost.done }} <- undefined; reported nothing + {% for ghost in rows %} + {{ ghost.name }} <- the binding that silenced it + {% endfor %} + +The reference outside the loop is exactly the typo a validator exists to +catch, and adding an unrelated loop to the file made it vanish. That trade is +backwards: the false positive it replaced was loud, and a false negative is +silent. + +These tests pin scope from both ends -- what a binding covers, and what it +does not -- for the visitor and for both things that ask it. +""" + +import pytest + +from jinja2 import nodes + +from orchestrator.core.template_globals import NOT_CALLED, find_global_misuse +from orchestrator.core.template_manager import TemplateManager +from orchestrator.core.template_scope import shadowed_name_nodes +from orchestrator.validation.data_flow_validator import DataFlowValidator + +pytestmark = [pytest.mark.contract] + + +def _free_names(source): + """The names a template uses but does not define, by identity.""" + ast = TemplateManager().env.parse(source) + shadowed = shadowed_name_nodes(ast) + return [ + node.name + for node in ast.find_all(nodes.Name) + if getattr(node, "ctx", "load") == "load" and id(node) not in shadowed + ] + + +def _refs(source): + return DataFlowValidator()._extract_template_variables(source) + + +def _misuse(source): + ast = TemplateManager().env.parse(source) + return {m.code for m in find_global_misuse(ast) if m.severity == "error"} + + +# --------------------------------------------------------------------------- +# A loop binds its target, inside the loop +# --------------------------------------------------------------------------- + +def test_a_loop_target_is_the_template_s_own_inside_the_loop(): + assert _free_names("{% for row in rows %}{{ row.x }}{% endfor %}") == ["rows"] + + +def test_a_name_used_before_the_loop_that_binds_it_is_not_the_loop_s(): + """The regression. `ghost.done` is undefined and must be reported.""" + source = "{{ ghost.done }}{% for ghost in rows %}{{ ghost.name }}{% endfor %}" + assert _free_names(source) == ["ghost", "rows"] + assert "ghost.done" in _refs(source) + + +def test_a_name_used_after_the_loop_that_binds_it_is_not_the_loop_s(): + source = "{% for ghost in rows %}{{ ghost.n }}{% endfor %}{{ ghost.done }}" + assert _free_names(source) == ["rows", "ghost"] + assert "ghost.done" in _refs(source) + + +def test_an_inner_loop_s_target_does_not_escape_the_inner_loop(): + source = ( + "{% for a in xs %}" + "{% for b in ys %}{{ a }}{{ b }}{% endfor %}" + "{{ b }}" # the inner target, outside the inner loop + "{% endfor %}" + ) + assert _free_names(source) == ["xs", "ys", "b"] + + +def test_the_iterable_is_evaluated_outside_the_loop(): + """`{% for x in x %}` iterates the *outer* `x`; only the body sees the new one.""" + assert _free_names("{% for x in x %}{{ x }}{% endfor %}") == ["x"] + + +def test_a_loop_filter_sees_the_target(): + """`{% for x in xs if x.ok %}` -- the test runs with `x` bound.""" + assert _free_names("{% for x in xs if x.ok %}{{ x }}{% endfor %}") == ["xs"] + + +def test_the_else_branch_does_not_see_the_target(): + """`{% else %}` runs only when the iterable was empty, so the target + never took a value there.""" + source = "{% for x in xs %}{{ x }}{% else %}{{ x }}{% endfor %}" + assert _free_names(source) == ["xs", "x"] + + +def test_loop_itself_is_bound_only_inside_a_loop(): + source = "{% for i in xs %}{{ loop.index }}{% endfor %}{{ loop }}" + assert _free_names(source) == ["xs", "loop"] + + +def test_a_tuple_target_binds_every_name_in_it(): + source = "{% for k, v in pairs %}{{ k }}{{ v }}{% endfor %}{{ v }}" + assert _free_names(source) == ["pairs", "v"] + + +# --------------------------------------------------------------------------- +# `{% set %}` binds from where it appears +# --------------------------------------------------------------------------- + +def test_set_binds_the_statements_after_it_and_not_before(): + """Jinja evaluates top to bottom, so `{{ x }}{% set x = 1 %}` really is an + undefined reference followed by an assignment.""" + assert _free_names("{{ x }}{% set x = 1 %}{{ x }}") == ["x"] + + +def test_the_value_of_a_set_is_evaluated_before_the_name_exists(): + """In `{% set x = x %}` the right-hand `x` is whatever it meant before.""" + assert _free_names("{% set x = x %}{{ x }}") == ["x"] + + +def test_a_set_block_binds_after_the_block(): + assert _free_names("{% set x %}{{ y }}{% endset %}{{ x }}") == ["y"] + + +def test_a_set_inside_a_conditional_reaches_past_it(): + """`{% if %}` opens no scope in Jinja: a `{% set %}` in a taken branch is + visible after the `{% endif %}`. Treating it as bound can only suppress a + report, never invent one.""" + assert _free_names("{% if c %}{% set y = 1 %}{% endif %}{{ y }}") == ["c"] + + +def test_a_set_inside_a_loop_does_not_escape_it(): + source = "{% for i in xs %}{% set t = i %}{{ t }}{% endfor %}{{ t }}" + assert _free_names(source) == ["xs", "t"] + + +# --------------------------------------------------------------------------- +# Macros, calls and with-blocks +# --------------------------------------------------------------------------- + +def test_a_macro_argument_is_bound_only_in_its_body(): + source = "{% macro m(p) %}{{ p }}{% endmacro %}{{ p }}" + assert _free_names(source) == ["p"] + assert "p" in _refs(source) + + +def test_a_macro_name_is_callable_after_its_definition(): + assert _free_names("{% macro m(p) %}{{ p }}{% endmacro %}{{ m() }}") == [] + + +def test_a_macro_default_is_evaluated_outside_the_body(): + assert _free_names("{% macro m(p=q) %}{{ p }}{% endmacro %}") == ["q"] + + +def test_caller_is_bound_inside_a_macro(): + """Jinja provides `caller` to a macro invoked through `{% call %}`.""" + assert _free_names("{% macro m() %}{{ caller() }}{% endmacro %}") == [] + + +def test_a_with_block_binds_only_its_body(): + assert _free_names("{% with w = 1 %}{{ w }}{% endwith %}{{ w }}") == ["w"] + + +def test_a_call_block_argument_is_bound_only_in_its_body(): + source = "{% macro m() %}{{ caller(1) }}{% endmacro %}" + source += "{% call(v) m() %}{{ v }}{% endcall %}{{ v }}" + assert _free_names(source) == ["v"] + + +# --------------------------------------------------------------------------- +# The globals validator asks the same question +# --------------------------------------------------------------------------- + +def test_a_global_shadowed_by_a_loop_is_left_alone_inside_it(): + """A template that binds `now` is talking about its own variable.""" + assert _misuse("{% for now in items %}{{ now }}{% endfor %}") == set() + + +@pytest.mark.parametrize( + "source", + [ + # Ours, before the loop that rebinds the name. + "{{ now }}{% for now in items %}{{ now }}{% endfor %}", + # Ours, after it. + "{% for now in items %}{{ now }}{% endfor %}{{ now }}", + # Ours, outside the macro that takes the name as an argument. + "{% macro f(now) %}{{ now }}{% endmacro %}{{ now }}", + # Ours, before the `{% set %}`. + "{{ now }}{% set now = 'x' %}", + ], +) +def test_a_global_outside_the_binding_is_still_ours(source): + """The suppression must not extend past the binding. + + `{{ now }}` renders the repr of a live function object -- the defect #451 + exists to catch -- and a loop elsewhere in the file used to hide it. + """ + assert NOT_CALLED in _misuse(source), ( + f"{source} names our global outside any binding of it and was accepted" + ) + + +def test_a_shadowed_and_an_unshadowed_use_can_share_one_template(): + """Identity, not spelling: same word, two nodes, two answers.""" + source = "{% for now in items %}{{ now }}{% endfor %}{{ now.foo }}" + assert NOT_CALLED in _misuse(source) + + +# --------------------------------------------------------------------------- +# It reaches the product +# --------------------------------------------------------------------------- + +def _validate(source, tmp_path): + """Validate a real pipeline the way `orchestrator validate` does.""" + import asyncio + + from orchestrator.compiler.yaml_compiler import YAMLCompiler + + pipeline = f""" +id: probe +name: Probe +steps: + - id: write_it + tool: filesystem + action: write + parameters: + path: "./out.txt" + content: "{source}" +""" + try: + asyncio.run(YAMLCompiler().compile(pipeline, {})) + return None + except Exception as exc: # noqa: BLE001 - the message is the subject + return str(exc) + + +def test_a_reference_outside_the_loop_that_shadows_it_fails_validation(tmp_path): + """The whole point, through the compiler rather than the helper. + + Without this, deleting the scope check leaves every test above passing: + they exercise the function, not whether anything asks it. + + Asserting merely that validation *fails* proves nothing here, and the + first version of this test passed under the very mutation it existed to + catch. Two reasons: `rows` is undeclared, so the pipeline is refused + either way; and the template validator finds undeclared names with + `jinja2.meta`, which is scope-aware already and reports `ghost` on its + own. The data-flow validator is the one that was blind, and it has its + own phrasing -- so that is what this looks for. + """ + refused = _validate( + "{% for ghost in rows %}{{ ghost.name }}{% endfor %}{{ ghost.done }}", + tmp_path, + ) + assert refused is not None, ( + "a reference to a loop target from outside the loop compiled at all" + ) + assert "Undefined task reference: 'ghost'" in refused, ( + "the data-flow validator did not object to 'ghost' used outside the " + f"loop that binds it, so the scope check is not wired in: {refused}" + ) + + +def test_a_reference_inside_the_loop_still_validates(tmp_path): + """The false positive this whole line of work exists to avoid. + + `{{ row.name }}` inside `{% for row in ... %}` is correct and common; a + scope check that rejected it would be worse than no check at all. + """ + inputs = "{% for row in [1, 2] %}{{ row }}{% endfor %}" + assert _validate(inputs, tmp_path) is None, ( + f"a loop variable used inside its loop was refused: " + f"{_validate(inputs, tmp_path)}" + )