From 7ea7c821764557f7cef3b1436005e3dfa0389131 Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Mon, 3 Aug 2026 08:00:59 -0400 Subject: [PATCH] Read references with the parser, not with str.split (#104) `_extract_template_variables` chopped raw template text: `split('|')[0]`, then `split(' ')[0]`. That does not survive an expression, so anything with a space or a bracket in it came out mangled -- and the fragment was then looked up as a task id and reported against the pipeline: Undefined task reference: '(row' Undefined task reference: 'from_json)' Undefined task reference: 'analysis_topics[loop' Undefined task reference: '(fact_check_loop' None of those is a name anyone wrote. Jinja already knows what a reference is, so it is asked instead -- the same move #451 made for global calls. The AST also gives what text-chopping could not: which names a `{% for %}` binds, and where a chain like `a.b.c` starts and stops. This catches *more*, not less. `split(' ')[0]` on {{ 'x' if ghost_step.done else 'y' }} yielded `'x'` -- a literal, discarded -- so a reference to a step that does not exist went unchecked. It is an error now, and there is a test for it, because a 14-file jump in the catalogue is only trustworthy with one. Two smaller fixes in the same area: - `loop` is Jinja's own variable and `item` is ours, but the allowlist compared the *whole* reference. A bare `loop` passed while `{{ loop.index }}` -- the way anyone actually writes it -- was reported as an undefined task. It compares the base name now. - `{{ range(3) }}` names a function the environment provides, not a step. Jinja's globals and the pipeline's are both excluded. Measured: catalogue validating 34 -> 48 of 117; blocking suite 711 -> 725 passed, 0 failed. Three mutations, all killed: - restore the text-chopping extractor -> 7 tests - compare the whole reference to the loop -> 2 tests allowlist again - stop excluding environment-provided -> 2 tests names Co-Authored-By: Claude Opus 5 (1M context) --- .../validation/data_flow_validator.py | 126 +++++++++++++----- tests/test_triage_product_defects.py | 76 +++++++++++ 2 files changed, 165 insertions(+), 37 deletions(-) diff --git a/src/orchestrator/validation/data_flow_validator.py b/src/orchestrator/validation/data_flow_validator.py index 07c5ce9..70ec735 100644 --- a/src/orchestrator/validation/data_flow_validator.py +++ b/src/orchestrator/validation/data_flow_validator.py @@ -21,6 +21,11 @@ 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"} +) + #: `thing['key']` -> `thing.key`, so one spelling reaches the checks below. _SUBSCRIPT = re.compile(r"""\[\s*['"]([^'"]+)['"]\s*\]""") #: `thing[0]` -> `thing`; an element of a collection is not a separate name. @@ -414,43 +419,86 @@ def _analyze_template_string(self, return errors, warnings, dependencies def _extract_template_variables(self, template_str: str) -> List[str]: - """Extract all template variable references from a string.""" - variables = [] - - # Find all {{ variable }} patterns - matches = self.template_var_pattern.findall(template_str) - - for match in matches: - # Clean up the variable reference - var = match.strip() - - # Handle filters and complex expressions - # Take the base variable before any filters or operations - var = var.split('|')[0].strip() # Remove filters - var = var.split(' ')[0].strip() # Remove operations - - # Skip literals and complex expressions - if (not var.startswith('"') and - not var.startswith("'") and - not var.isdigit() and - not var.startswith('[') and - not var.startswith('{')): - variables.append(var) - - # Also check for Jinja2 control structures ({% %}) - control_pattern = re.compile(r'\{%\s*(?:for|if)\s+([^%]+)\s*%\}') - control_matches = control_pattern.findall(template_str) - - for match in control_matches: - # Extract variable references from control structures - # This is more complex parsing, simplified for now - parts = match.split() - for part in parts: - if '.' in part and not part.startswith('"') and not part.startswith("'"): - variables.append(part.strip()) - + """Every variable reference in a template, as dotted paths. + + This used to chop the raw text -- `split('|')[0]`, then `split(' ')[0]` + -- which does not survive an expression. Anything with a space or a + bracket in it came out mangled, and the mangled fragment was then + looked up as a task id: + + Undefined task reference: '(row' + Undefined task reference: 'from_json)' + Undefined task reference: 'analysis_topics[loop' + + None of those is a name anyone wrote. The parser knows what a + reference is, so it is asked instead -- the same move #451 made for + global calls. + """ + from jinja2 import nodes + + try: + ast = self.jinja_env.parse(template_str) + except Exception: + # Syntax is the template validator's business; a broken template + # 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" + } + + # `a.b.c` is one reference, not three. Only the outermost node of a + # chain is reported; the links inside it are skipped. + inner = { + id(node.node) + for node in ast.find_all((nodes.Getattr, nodes.Getitem)) + } + + # `{{ range(3) }}` names a function the environment provides, not a + # step. Jinja's own globals and the pipeline's both count. + provided = set(self.jinja_env.globals) | pipeline_global_names() + + variables: 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: + continue + base = path.split(".", 1)[0] + if base in bound or base in provided: + continue + variables.append(path) + return variables - + + 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 + + if isinstance(node, nodes.Name): + return node.name if getattr(node, "ctx", "load") == "load" else None + + if isinstance(node, nodes.Getattr): + base = self._dotted_path(node.node) + return f"{base}.{node.attr}" if base else None + + if isinstance(node, nodes.Getitem): + base = self._dotted_path(node.node) + if not base: + return None + key = node.arg + if isinstance(key, nodes.Const) and isinstance(key.value, str): + return f"{base}.{key.value}" + # An index selects an element of `base`; it is not its own name. + return base + + return None + def _validate_variable_reference(self, var_ref: str, task_id: str, @@ -463,7 +511,11 @@ def _validate_variable_reference(self, Returns dict with validation result and metadata. """ # Handle special variables - if var_ref in ['item', 'index', 'loop', 'iteration', 'is_first', 'is_last']: + # `loop` is Jinja's own, `item` and the rest are ours. The check used + # to compare the whole reference, so a bare `loop` passed while + # `{{ loop.index }}` -- the way anyone actually writes it -- was + # reported as an undefined *task*. + if var_ref.split(".", 1)[0] in LOOP_VARIABLES: return {"valid": True, "type": "loop_variable"} if var_ref.startswith('$'): diff --git a/tests/test_triage_product_defects.py b/tests/test_triage_product_defects.py index 0f8c623..5c69f21 100644 --- a/tests/test_triage_product_defects.py +++ b/tests/test_triage_product_defects.py @@ -181,3 +181,79 @@ def test_json_encode_is_the_same_function_as_to_json(): """ filters = TemplateManager().env.filters assert filters["json_encode"] is filters["to_json"] + + +# --------------------------------------------------------------------------- +# 4. References were extracted by chopping text +# --------------------------------------------------------------------------- + +def _refs(template): + return DataFlowValidator()._extract_template_variables(template) + + +@pytest.mark.parametrize( + "template,expected", + [ + # An expression with a space in it. `split(' ')[0]` cut here. + ("{{ zip(rows, cols) }}", {"zip", "rows", "cols"}), + ("{{ 'yes' if ready else 'no' }}", {"ready"}), + # A filter. `split('|')[0]` left the tail behind as its own "name". + ("{{ payload | from_json }}", {"payload"}), + # A subscript holding another reference. + ("{{ topics[loop.index] }}", {"topics", "loop.index"}), + # A chain is one reference, not three. + ("{{ a.b.c }}", {"a.b.c"}), + ], +) +def test_a_reference_survives_being_in_an_expression(template, expected): + """Chopping the text produced fragments nobody wrote. + + Undefined task reference: '(row' + Undefined task reference: 'from_json)' + Undefined task reference: 'analysis_topics[loop' + + Each was then looked up as a task id and reported against the pipeline. + """ + assert set(_refs(template)) == expected + + +def test_a_loop_target_is_not_a_reference(): + """`{% for row in rows %}` binds `row`; only `rows` is a reference.""" + assert set(_refs("{% for row in rows %}{{ row.x }}{% endfor %}")) == {"rows"} + + +@pytest.mark.parametrize("template", ["{{ range(3) }}", "{{ now() }}"]) +def test_a_function_the_environment_provides_is_not_a_task(template): + assert _refs(template) == [] + + +def test_a_broken_template_yields_no_references(): + """Syntax is the template validator's business. + + Guessing at references inside something that does not parse is how the + fragments above were invented. + """ + assert _refs("{{ unclosed ") == [] + + +@pytest.mark.parametrize("reference", ["loop.index", "item.name", "loop", "item"]) +def test_loop_variables_are_recognised_however_they_are_written(reference): + """The allowlist compared the *whole* reference, so a bare `loop` passed + while `{{ loop.index }}` -- the way anyone writes it -- did not.""" + assert _reference(reference)["valid"] is True + + +def test_a_typo_inside_an_expression_is_now_caught(): + """Parsing catches more than chopping did, not less. + + `split(' ')[0]` on `'x' if ghost_step.done else 'y'` yielded `'x'` -- a + literal, discarded -- so the reference to a step that does not exist was + never checked at all. A 14-file jump in the catalogue is only trustworthy + if it comes with this. + """ + refs = _refs("{{ 'x' if ghost_step.done else 'y' }}") + assert "ghost_step.done" in refs + + result = _reference("ghost_step.done") + assert result["valid"] is False + assert result["error_type"] == "undefined_task"