From 4c26458895204cc0d20926fee1d71f4c3c5725f5 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Mon, 7 Sep 2026 15:56:59 +0700 Subject: [PATCH 1/4] refactor(workflows): let the evaluator report its own leaves (#4274) _unresolvable_term answered one question -- does every operand in this condition resolve to something? -- by walking the expression itself: filters, then or/and/not, then comparisons, then list literals, down to the leaves. That walk was a second implementation of the parsing in _evaluate_simple_expression, kept in step with it by hand. Two helpers existed only to restate rules the evaluator already had. _looks_numeric mirrored the float()-only-when-a-dot-is-present rule because a bare float() accepts 1e3 and the evaluator does not. _is_literal mirrored the matching-close-is-the-final-character string test because startswith/endswith accepts 'a' 'b' and the evaluator does not. Both docstrings said "mirror the evaluator exactly", which is the tell: when the two drift nothing breaks loudly, the gate just answers wrongly, and the wrong answer is a paste-ready correction that inverts a condition. Seven of the nine findings in #4230 were the same defect wearing different clothes -- the gate disagreeing with the evaluator about where the operands are. Each round fixed one shape. Nothing stopped a tenth. _evaluate_simple_expression has exactly one place where a substring stops being grammar and becomes a name to resolve: its final line, _resolve_dot_path. Literals return before it; operands, filter arguments and list elements all arrive there by construction. Record the leaf there, behind a ContextVar that is None outside a probe, and the gate applies namespace rules to that list instead of re-deriving it. It now contains no grammar at all. Two properties this rests on, both asserted rather than assumed: * or/and are not short-circuited -- both sides are evaluated and only then combined -- so a leaf is recorded whatever the other side is worth. If that ever changes the gate would go quietly blind, so there is a test for it. * A probe run can raise on its own placeholder values. The leaves seen before that point are real, so they are kept rather than discarded; discarding them would lose `bogus` in `inputs.tags | join(bogus)`, which an earlier round of #4230 had to add by hand. expressions.py is 109 lines lighter and 84 heavier. All 336 existing tests pass unchanged, including the 20 cases of test_operands_must_be_literals_or_known_paths that took eight rounds to get right. test_literal_test_mirrors_the_evaluator tested the mirror, so it becomes test_literal_handling_comes_from_the_evaluator and asserts the same knowledge about 1e3 and 'a' 'b' through the gate instead. Four mutations, each killed by the tests that should kill it -- removing the leaf report alone turns 38 red. ruff 0.15.0 clean. --- src/specify_cli/workflows/expressions.py | 170 +++++++----------- tests/unit/test_condition_expression_block.py | 71 +++++++- 2 files changed, 132 insertions(+), 109 deletions(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 198010838e..4a6abacdc8 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -9,6 +9,7 @@ import json import re +from contextvars import ContextVar from typing import Any @@ -479,6 +480,11 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An # evaluator will actually split on. _COMPARISON_OPERATORS = ("!=", "==", ">=", "<=", ">", "<", " not in ", " in ") +# Set only while `_collect_leaves` probes an expression; None everywhere else, so +# a normal evaluation costs one `.get()`. A ContextVar rather than a module global +# so concurrent probes cannot append into each other's list. +_leaf_sink: ContextVar[list[str] | None] = ContextVar("_leaf_sink", default=None) + def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: """Evaluate a simple expression against the namespace. @@ -627,7 +633,12 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: ] return items - # Variable reference (dot-path) + # Variable reference (dot-path). This is the one place a substring stops being + # grammar and becomes a name to resolve, so it is where a probe can learn what + # the evaluator will actually look up. Literals have all returned above. + sink = _leaf_sink.get() + if sink is not None: + sink.append(expr) return _resolve_dot_path(namespace, expr) @@ -1097,115 +1108,43 @@ def _evaluator_rejects(text: str) -> str | None: -def _looks_numeric(text: str) -> bool: - """Mirror the evaluator's numeric literal test exactly. +def _collect_leaves(text: str) -> list[str]: + """Every substring *text* hands to the evaluator as a name to resolve. - `_evaluate_simple_expression` only calls `float()` when a `.` is present and - `int()` otherwise, so `1e3` is not a number to it -- it falls through to a path - lookup and resolves to None. A bare `float()` here accepted `1e3` and the - correction turned a truthy condition false. - """ - try: - if "." in text: - float(text) - else: - int(text) - except (ValueError, TypeError): - return False - return True + Runs the same probe ``_evaluator_rejects`` uses, with the leaf sink armed. + Literals never reach the dot-path resolution, and operands, filter arguments + and list elements all do -- ``_evaluate_simple_expression`` evaluates both + sides of ``or``/``and`` eagerly rather than short-circuiting, so a leaf is + recorded whatever the other side is worth. - -def _is_literal(text: str) -> bool: - """Mirror the evaluator's literal tests exactly. - - The string case is the opening quote's *matching close being the final - character*, not first/last-character equality: `'a' 'b'` passes the latter but - is two literals to the evaluator, which falls through to a path lookup. + A probe run can still raise on its own placeholder values, which is what + ``_evaluator_rejects`` sorts out. The leaves seen before that point are real + -- the evaluator reached them -- so they are kept rather than discarded: + ``inputs.tags | join(bogus)`` records ``bogus`` and only then trips over the + placeholder handed to ``join``. """ - if text[:1] in ("'", '"') and text.find(text[0], 1) == len(text) - 1: - return True - return text.lower() in ("true", "false", "none", "null") or _looks_numeric(text) - + leaves: list[str] = [] + token = _leaf_sink.set(leaves) + try: + _evaluate_simple_expression( + text, {root: _ProbeNamespace() for root in _NAMESPACE_ROOTS} + ) + except Exception: # noqa: BLE001 - probe values, reported by _evaluator_rejects + pass + finally: + _leaf_sink.reset(token) + return leaves -def _unresolvable_term(text: str) -> str | None: - """The first operand in *text* the evaluator cannot resolve, or ``None``. - Walks operands the way ``_evaluate_simple_expression`` does -- filters, then - ``or``/``and``/``not``, then comparisons -- and checks each leaf. A leaf must be - a literal or a dotted path rooted in ``_NAMESPACE_ROOTS``. +def _unresolvable_leaf(leaf: str) -> str | None: + """Why the evaluator cannot resolve the name *leaf*, or ``None``. - Enumerating broken shapes is what made this take several rounds: each new gate - only knew the shapes named so far. ``inputs.a === inputs.b`` split cleanly on - ``==`` and looked complete, while the evaluator read ``= inputs.b`` as a path - and resolved it to ``None``; ``bogus == 'x'`` passed for the same reason one - level up. Recursing to the leaves covers both without naming either. + Namespace knowledge only. Everything about where operands live now comes from + the evaluator itself, so nothing here restates the grammar. """ - stripped = text.strip() - if not stripped: - return "an operand is empty" - - if _find_top_level(stripped, "|") != -1: - segments = _split_top_level(stripped, "|") - reason = _unresolvable_term(segments[0]) - if reason is not None: - return reason - # A filter argument is an ordinary operand to `_apply_filter`, which - # evaluates it with `_evaluate_simple_expression` like any other. Skipping - # it let `inputs.tags | join(bogus)` be offered as paste-ready: `bogus` is - # no namespace root, resolves to None, and the wrapped form then raises - # `join: expected a string separator, got NoneType`. Parse with the same - # pattern `_apply_filter` uses, so a form this does not recognize is left - # to the evaluator probe rather than guessed at here. - for segment in segments[1:]: - match = re.fullmatch(r"(\w+)\((.+)\)", segment.strip()) - if match is None: - continue - reason = _unresolvable_term(match.group(2)) - if reason is not None: - return reason - return None - - for op in (" or ", " and "): - idx = _find_top_level(stripped, op) - if idx != -1: - return _unresolvable_term(stripped[:idx]) or _unresolvable_term( - stripped[idx + len(op):] - ) - - if stripped.startswith("not "): - return _unresolvable_term(stripped[4:]) - - for op in _COMPARISON_OPERATORS: - idx = _find_top_level(stripped, op) - if idx != -1: - return _unresolvable_term(stripped[:idx]) or _unresolvable_term( - stripped[idx + len(op):] - ) - - if _is_literal(stripped): - return None - - # A list literal is a term the evaluator understands, and it recurses into the - # elements rather than resolving the brackets as a name. Not mirroring that - # denied the correction to `inputs.tag in ['x', 'y']` -- a condition wrapping - # repairs completely -- while reporting the list as an unresolvable name. The - # empty-segment skip matches `_evaluate_simple_expression`, which drops them so - # `[1, 2,]` is `[1, 2]` rather than `[1, 2, None]`. - if stripped.startswith("[") and stripped.endswith("]"): - inner = stripped[1:-1].strip() - if not inner: - return None - for element in _split_top_level_commas(inner): - if not element.strip(): - continue - reason = _unresolvable_term(element) - if reason is not None: - return reason - return None - - segments = _split_top_level(stripped, ".") + segments = _split_top_level(leaf, ".") if not _PATH_SEGMENT.match(segments[0].strip()): - return f"{stripped!r} is not a name the evaluator can resolve" + return f"{leaf!r} is not a name the evaluator can resolve" # `item` is the only root that is not always a mapping: `StepContext.item` is # `Any` and a fan-out assigns the item value itself, so when that value is a # list `_resolve_dot_path` indexes it and `item[0] == 'x'` resolves. Every @@ -1227,6 +1166,33 @@ def _unresolvable_term(text: str) -> str | None: return None +def _unresolvable_term(text: str) -> str | None: + """The first name in *text* the evaluator cannot resolve, or ``None``. + + Asks the evaluator which names it will look up, then applies the namespace + rules to those. The previous implementation derived the names itself by + re-walking the grammar -- filters, then ``or``/``and``/``not``, then + comparisons, then list literals -- and had to be kept in step with + ``_evaluate_simple_expression`` by hand. + + That is what made this take several rounds: each round fixed one shape the + walk disagreed about (``inputs.a === inputs.b``, ``bogus == 'x'``, list + elements, filter arguments, a newline before ``and``) and nothing stopped the + next one. Reading the leaves off the evaluator removes the class rather than + another instance of it: the two cannot disagree about where the operands are + when only one of them decides. + """ + stripped = text.strip() + if not stripped: + return "an operand is empty" + + for leaf in _collect_leaves(stripped): + reason = _unresolvable_leaf(leaf) + if reason is not None: + return reason + return None + + def _wrapping_would_not_repair(core: str) -> str | None: """Why wrapping *core* in ``{{ }}`` would not yield the expression intended. diff --git a/tests/unit/test_condition_expression_block.py b/tests/unit/test_condition_expression_block.py index 0739a2bc29..83c061e91e 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -15,8 +15,9 @@ _has_unbalanced_bracket, _has_incomplete_operand, _unresolvable_term, + _collect_leaves, + _leaf_sink, _evaluator_rejects, - _is_literal, _strip_stray_delimiters, _COMPARISON_OPERATORS, _WORD_OPERATORS, @@ -676,26 +677,36 @@ def test_filter_wiring_errors_are_still_rejections(condition): @pytest.mark.parametrize( - "condition,literal", + "condition,resolvable", [ ("42", True), ("3.14", True), ("-7", True), # `1e3` has no "." so the evaluator calls int() on it, which fails; it then - # falls through to a path lookup. float() alone accepted it here. + # falls through to a path lookup and resolves to None. The gate used to + # decide this for itself with a float() test that accepted `1e3`, and the + # correction turned a truthy condition false. Now the evaluator reaches the + # dot-path resolution with `1e3` and reports it, so the two cannot disagree. ("1e3", False), ("'one'", True), ('"one"', True), # Two literals, not one: the evaluator requires the opening quote's match to # be the final character, which first/last-character equality does not. ("'a' 'b'", False), - ("'a' == 'b'", False), + ("'a' == 'b'", True), ("true", True), - ("inputs.name", False), + ("inputs.name", True), + ("bogus", False), ], ) -def test_literal_test_mirrors_the_evaluator(condition, literal): - assert _is_literal(condition) is literal +def test_literal_handling_comes_from_the_evaluator(condition, resolvable): + """What `_is_literal` used to assert, asserted through the gate instead. + + The helper existed only to restate the evaluator's literal tests, and its test + could pass while the two had drifted. Asking whether the gate accepts the + condition tests the property that actually matters. + """ + assert (_unresolvable_term(condition) is None) is resolvable @pytest.mark.parametrize( @@ -827,3 +838,49 @@ def test_indexing_an_always_mapping_root_still_loses_the_correction(condition): ctx = StepContext(inputs={"a": 1}, item=["x", "y"]) assert CORRECTION_OFFERED not in format_condition_remediation(condition) assert evaluate_condition("{{ " + condition + " }}", ctx) is False + + +# --- what the leaf sink rests on ---------------------------------------------- + + +def test_the_sink_is_off_outside_a_probe(): + """A normal evaluation must not pay for, or be observed by, the gate.""" + assert _leaf_sink.get() is None + + evaluate_expression("{{ inputs.name }}", StepContext(inputs={"name": "x"})) + + assert _leaf_sink.get() is None + + +def test_the_sink_is_cleared_even_when_the_probe_raises(): + """`_collect_leaves` swallows probe errors; it must still reset the var.""" + _collect_leaves("inputs.tags | nosuchfilter") + + assert _leaf_sink.get() is None + + +def test_both_sides_of_a_boolean_are_reported(): + """The load-bearing property: `_evaluate_simple_expression` evaluates both + operands of `and`/`or` and only then combines them. If it ever + short-circuits, the gate would stop seeing the right-hand operand and go + quietly blind -- so assert it here rather than rely on it silently. + """ + assert _collect_leaves("inputs.a or bogus") == ["inputs.a", "bogus"] + assert _collect_leaves("false and bogus") == ["bogus"] + assert _unresolvable_term("false and bogus") is not None + + +def test_leaves_seen_before_a_probe_error_are_kept(): + """The probe hands `join` a placeholder and it raises. The leaves reached + before that are real, so discarding them would lose `bogus` -- the filter + argument case an earlier round of #4230 had to add by hand. + """ + assert "bogus" in _collect_leaves("inputs.tags | join(bogus)") + assert _unresolvable_term("inputs.tags | join(bogus)") is not None + + +def test_a_literal_never_reaches_the_resolver(): + """Why the gate needs no literal test of its own any more.""" + assert _collect_leaves("'a literal'") == [] + assert _collect_leaves("42") == [] + assert _collect_leaves("true") == [] From c6c3ef74392334cee084786c410f076d50771a79 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Wed, 9 Sep 2026 08:01:57 +0700 Subject: [PATCH 2/4] refactor(workflows): let _resolve_dot_path define the indexed segment The gate no longer restates the operator grammar, but it still restated the shape of a path segment: _PATH_SEGMENT and an inline fullmatch both described the index form that _resolve_dot_path matches with its own regex. Three copies of one rule, kept in step by hand -- the same drift this refactor set out to remove, one layer down. Name the form once as _INDEXED_SEGMENT beside _resolve_dot_path and have the gate ask it. Behaviour is unchanged: the regex is copied verbatim. What changes is that widening indexing now reaches the gate for free. --- src/specify_cli/workflows/expressions.py | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 4a6abacdc8..7d8aeab425 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -135,6 +135,15 @@ def _filter_from_json(value: Any) -> Any: _EXPR_PATTERN = re.compile(r"\{\{(.+?)\}\}") +# The one definition of an indexed path segment. _resolve_dot_path matches +# against it, and the condition gate below reuses it rather than describing the +# same shape a second time, so widening what indexing accepts cannot leave the +# evaluator and the gate disagreeing. +_INDEXED_SEGMENT = re.compile(r"^([\w-]+)\[(\d+)\]$") + +_PLAIN_SEGMENT = re.compile(r"^[\w-]+$") + + def _resolve_dot_path(obj: Any, path: str) -> Any: """Resolve a dotted path like ``steps.specify.output.file`` against *obj*. @@ -144,7 +153,7 @@ def _resolve_dot_path(obj: Any, path: str) -> Any: current = obj for part in parts: # Handle list indexing: name[0] - idx_match = re.match(r"^([\w-]+)\[(\d+)\]$", part) + idx_match = _INDEXED_SEGMENT.match(part) if idx_match: key, idx = idx_match.group(1), int(idx_match.group(2)) if isinstance(current, dict): @@ -1058,8 +1067,9 @@ def _has_incomplete_operand(text: str) -> bool: # None, so a correction built on one turns a truthy condition false. _NAMESPACE_ROOTS = ("inputs", "steps", "item", "fan_in", "context") -# Exactly what _resolve_dot_path accepts: a name, optionally one numeric index. -_PATH_SEGMENT = re.compile(r"^[\w-]+(\[\d+\])?$") +def _is_path_segment(segment: str) -> bool: + """Whether _resolve_dot_path can walk *segment*: a name, or a name it indexes.""" + return bool(_PLAIN_SEGMENT.match(segment) or _INDEXED_SEGMENT.match(segment)) class _ProbeNamespace(dict): @@ -1143,7 +1153,7 @@ def _unresolvable_leaf(leaf: str) -> str | None: the evaluator itself, so nothing here restates the grammar. """ segments = _split_top_level(leaf, ".") - if not _PATH_SEGMENT.match(segments[0].strip()): + if not _is_path_segment(segments[0].strip()): return f"{leaf!r} is not a name the evaluator can resolve" # `item` is the only root that is not always a mapping: `StepContext.item` is # `Any` and a fan-out assigns the item value itself, so when that value is a @@ -1152,7 +1162,7 @@ def _unresolvable_leaf(leaf: str) -> str | None: # branch returns None for those however it is written -- so the index is # stripped for `item` alone rather than for roots in general. root = segments[0].strip() - indexed_root = re.fullmatch(r"([\w-]+)\[\d+\]", root) + indexed_root = _INDEXED_SEGMENT.match(root) if indexed_root is not None and indexed_root.group(1) == "item": root = indexed_root.group(1) if root not in _NAMESPACE_ROOTS: @@ -1161,7 +1171,7 @@ def _unresolvable_leaf(leaf: str) -> str | None: f"({', '.join(_NAMESPACE_ROOTS)})" ) for segment in segments[1:]: - if not _PATH_SEGMENT.match(segment.strip()): + if not _is_path_segment(segment.strip()): return f"{segment.strip()!r} is not a valid path segment" return None From dd4b7233c908a4d94d560922e89279c3bbe1fe53 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Wed, 9 Sep 2026 08:05:18 +0700 Subject: [PATCH 3/4] test(workflows): pin that the gate reads the evaluator's definitions Two regression tests for the property this refactor is for, both of which a second copy of the grammar in the gate would break while every existing test stayed green: - widening _INDEXED_SEGMENT alone reaches the gate (the negative-index shape from #4416) - when the evaluator stops treating something as a leaf, the gate stops checking it, with no gate edit (the grouped-operand shape from #4417) Both were checked by reintroducing the drift: giving the gate its own segment regex again fails the first with the real message rather than an import error. --- tests/unit/test_condition_expression_block.py | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/unit/test_condition_expression_block.py b/tests/unit/test_condition_expression_block.py index 83c061e91e..410a014bf3 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -1,8 +1,11 @@ """A string condition with no ``{{ }}`` block is never evaluated (always true).""" +import re + import pytest import yaml +from specify_cli.workflows import expressions from specify_cli.workflows.base import StepContext from specify_cli.workflows.expressions import ( condition_has_malformed_expression_block, @@ -884,3 +887,51 @@ def test_a_literal_never_reaches_the_resolver(): assert _collect_leaves("'a literal'") == [] assert _collect_leaves("42") == [] assert _collect_leaves("true") == [] + + +# --- The gate reads the evaluator's definitions, it does not restate them --- +# +# The point of this refactor is that widening what the evaluator accepts reaches +# the validation gate for free. That is easy to claim and easy to lose: a second +# copy of the grammar in the gate keeps every existing test green while silently +# reintroducing the drift. These two pin the wiring by moving the evaluator's own +# definitions and asserting the gate follows. + + +def test_gate_reads_the_shared_indexed_segment_definition(monkeypatch): + """Widening `_INDEXED_SEGMENT` alone must reach the gate. + + `steps.…​.task_list[-1]` is rejected today because `_INDEXED_SEGMENT` — the + one place `_resolve_dot_path` says what an index looks like — accepts digits + only. Widening it there and nowhere else must be enough; if the gate keeps + its own copy of the shape (as `_PATH_SEGMENT` used to), this fails. + """ + path = "steps.tasks.output.task_list[-1].file" + assert expressions._unresolvable_term(path) is not None + + monkeypatch.setattr( + expressions, "_INDEXED_SEGMENT", re.compile(r"^([\w-]+)\[(-?\d+)\]$") + ) + assert expressions._unresolvable_term(path) is None + + +def test_gate_reports_the_leaves_the_evaluator_actually_reached(monkeypatch): + """The gate's operands come from the evaluator's own walk, not a second parse. + + If `_evaluate_simple_expression` stops treating something as a leaf — which + is what unwrapping a parenthesised group does — the gate stops checking it, + with no change to the gate itself. + """ + grouped = "(inputs.a or inputs.b) and inputs.c" + assert expressions._unresolvable_term(grouped) is not None + + real = expressions._evaluate_simple_expression + + def unwrapping(expr, namespace): + stripped = expr.strip() + if stripped.startswith("(") and stripped.endswith(")"): + return unwrapping(stripped[1:-1], namespace) + return real(expr, namespace) + + monkeypatch.setattr(expressions, "_evaluate_simple_expression", unwrapping) + assert expressions._unresolvable_term(grouped) is None From d829b7e596f5dda69a9075144ab202d53420b704 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Thu, 10 Sep 2026 08:55:24 +0700 Subject: [PATCH 4/4] fix(workflows): keep collecting leaves after a probe error The refactor stopped the leaf walk at the first exception a probe value raised, so every leaf further along the chain was lost. That is the one thing the collection exists to report, and it was a step backwards from the hand-written walk this PR replaces: inputs.blob | from_json | contains(bogus) origin/main reports 'bogus' this PR before the fix MISSED this PR after the fix reports 'bogus' from_json receives the probe placeholder mapping and raises; the walk ended there and contains(bogus) was never reached. Carry on past a failing filter while the sink is armed. _apply_filter evaluates a filter argument before it can raise on the value, so the failing segment's own leaves are already recorded; a fresh placeholder goes into the next filter, matching what the probe namespace hands out. Scoped to the probe: the sink is armed only by _collect_leaves, and _evaluator_rejects runs its own probe without it, so a mis-wired filter is still rejected and a real evaluation still raises rather than quietly returning the unfiltered value. --- src/specify_cli/workflows/expressions.py | 21 ++++++++- tests/unit/test_condition_expression_block.py | 45 +++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 7d8aeab425..fe8098f321 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -560,8 +560,27 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: f"operand in its own expression instead." ) value = _evaluate_simple_expression(head, namespace) + sink = _leaf_sink.get() for segment in segments[1:]: - value = _apply_filter(value, segment.strip(), namespace) + if sink is None: + value = _apply_filter(value, segment.strip(), namespace) + continue + # Probing. A filter handed a placeholder can raise on it -- from_json + # on a mapping is the common one -- and letting that end the walk + # hides every leaf further along the chain, which is the one thing + # this collection exists to report: `inputs.blob | from_json | + # contains(bogus)` recorded `inputs.blob` and stopped, so `bogus` + # was never offered to _unresolvable_leaf. _apply_filter evaluates a + # filter's argument before it can raise on the value, so the leaves + # of the failing segment are already recorded when we get here. + # Carry a fresh placeholder so the next filter sees the same kind of + # unknown the namespace hands out. Real evaluation is untouched: the + # sink is armed only by _collect_leaves, and _evaluator_rejects runs + # its own probe without it, so a mis-wired filter is still reported. + try: + value = _apply_filter(value, segment.strip(), namespace) + except Exception: # noqa: BLE001 - probe values, not the author's text + value = _ProbeNamespace() return value # Boolean operators — parse 'or' first (lower precedence) so that diff --git a/tests/unit/test_condition_expression_block.py b/tests/unit/test_condition_expression_block.py index 410a014bf3..7a30a2f42f 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -882,6 +882,51 @@ def test_leaves_seen_before_a_probe_error_are_kept(): assert _unresolvable_term("inputs.tags | join(bogus)") is not None +def test_a_probe_error_does_not_end_the_filter_chain(): + """Keeping the leaves seen so far is not enough -- the walk has to go on. + + `from_json` receives the probe's placeholder mapping and raises. Stopping + there loses every leaf further along the chain, which is the one thing this + collection exists to report, and it is a step *backwards* from the + hand-written walk this refactor replaces: that walk read `bogus` straight + out of its own grammar rules and reported it. + """ + expr = "inputs.blob | from_json | contains(bogus)" + + assert _collect_leaves(expr) == ["inputs.blob", "bogus"] + assert _unresolvable_term(expr) is not None + + +def test_every_later_link_of_a_chain_is_still_walked(): + """Not merely the next link: two failing filters must not hide the third.""" + expr = "inputs.blob | from_json | map(bogus) | join(alsobogus)" + + leaves = _collect_leaves(expr) + + assert "bogus" in leaves + assert "alsobogus" in leaves + assert _unresolvable_term(expr) is not None + + +def test_only_the_probe_carries_on_past_a_filter_error(): + """The continue-on-error is armed by the sink and nothing else. + + A real evaluation must still fail loudly: `_apply_filter` raises rather than + return the unfiltered value precisely so a mis-wired filter cannot become a + quietly wrong answer, and the probe must not soften that. + """ + context = StepContext(inputs={"blob": "not json", "tags": ["a"]}) + + with pytest.raises(ValueError, match="invalid JSON"): + evaluate_expression("{{ inputs.blob | from_json | contains('x') }}", context) + + with pytest.raises(ValueError, match="unknown filter"): + evaluate_expression("{{ inputs.tags | nosuchfilter }}", context) + + # And the rejection probe, which runs without the sink, still reports it. + assert _evaluator_rejects("inputs.tags | nosuchfilter") is not None + + def test_a_literal_never_reaches_the_resolver(): """Why the gate needs no literal test of its own any more.""" assert _collect_leaves("'a literal'") == []