refactor(workflows): let the evaluator report its own leaves (#4274) (#4460)

* 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.

* 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.

* 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.

* 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.

Author: ntdatt812

src/specify_cli/workflows/expressions.py

@@ -9,6 +9,7 @@
 
 import json
 import re
+from contextvars import ContextVar
 from typing import Any
 
 
@@ -134,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*.
 
@@ -143,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):
@@ -479,6 +489,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.
@@ -545,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
@@ -627,7 +661,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)
 
 
@@ -1047,8 +1086,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):
@@ -1097,123 +1137,51 @@ def _evaluator_rejects(text: str) -> str | None:
 
 
 
-def _looks_numeric(text: str) -> bool:
-    """Mirror the evaluator's numeric literal test exactly.
-
-    `_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
-
+def _collect_leaves(text: str) -> list[str]:
+    """Every substring *text* hands to the evaluator as a name to resolve.
 
-def _is_literal(text: str) -> bool:
-    """Mirror the evaluator's literal tests exactly.
+    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.
 
-    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, ".")
-    if not _PATH_SEGMENT.match(segments[0].strip()):
-        return f"{stripped!r} is not a name the evaluator can resolve"
+    segments = _split_top_level(leaf, ".")
+    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
     # list `_resolve_dot_path` indexes it and `item[0] == 'x'` resolves. Every
     # other root comes back from `_build_namespace` as a mapping, and the index
     # 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:
@@ -1222,11 +1190,38 @@ def _unresolvable_term(text: 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
 
 
+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.