Skip to content

Correction gate re-implements the expression grammar; let the evaluator report its own leaves #4274

Description

@ntdatt812

Context

In #4230 you said: "Lets get this one through the review and then if you can describe what specifically would make it easier / cleaner / faster then lets address that by describing it in an issue so we can asses it?" — this is that write-up. It has merged, so here it is.

The problem

_unresolvable_term answers one question: does every operand in this condition resolve to something? It answers it by walking the expression itself — filters, then or/and/not, then comparisons, then list literals, down to the leaves — and classifying each leaf as a literal or a path.

That walk is a second implementation of the parsing in _evaluate_simple_expression, kept in step with it by hand. Two helpers exist purely to restate rules the evaluator already has:

  • _looks_numeric mirrors the float()-only-when-a-dot-is-present rule, because a bare float() accepts 1e3 and the evaluator does not.
  • _is_literal mirrors the matching close is the final character string test, because startswith/endswith accepts 'a' 'b' and the evaluator does not.

Both docstrings say "mirror the evaluator exactly", which is the tell. When the two drift, nothing breaks loudly — the gate keeps answering, just wrongly, and the wrong answer is a paste-ready correction that silently inverts a condition.

That is what made #4230 take eight rounds. Sorting its findings by what actually failed:

Finding What the gate got wrong
inputs.a === inputs.b didn't see = inputs.b as an operand
bogus == 'x' only inspected one position
inputs.tag in ['x', 'y'] didn't recurse into list elements
inputs.tags | join(bogus) didn't treat a filter argument as an operand
newline before and split where the evaluator doesn't
1e3 classified a leaf the evaluator wouldn't
'a' 'b' classified a leaf the evaluator wouldn't
inputs.payload | from_json() filter form — caught by the evaluator probe
item[0] genuine namespace rule (item is the one root that may be a list)

Seven of nine are the same defect wearing different clothes: the gate disagreed with the evaluator about where the operands are. Each round fixed one shape. Nothing stops a tenth.

What would make it cleaner

_evaluate_simple_expression already has exactly one place where a substring stops being grammar and becomes a name to resolve — its final line, return _resolve_dot_path(namespace, expr). Literals never reach it. Operands, filter arguments and list elements all do, by construction.

So let the evaluator report the leaves it reaches, and the gate applies its rules to that list instead of re-deriving it. The gate then contains no grammar at all — only namespace knowledge, which is the part that genuinely belongs to it.

Prototype

I built it against main to check the idea survives contact, rather than describing something untested.

The evaluator side is three lines:

    # Variable reference (dot-path)
    if _LEAF_SINK is not None:
        _LEAF_SINK.append(expr)
    return _resolve_dot_path(namespace, expr)

The gate becomes a probe run plus a per-leaf check, and _unresolvable_leaf keeps only the namespace rules — root membership, path-segment shape, and the item index narrowing from the last round of #4230.

expressions.py                                    36 insertions, 80 deletions
tests/unit/test_condition_expression_block.py    286 passed, unchanged

Every case those eight rounds produced still gets the same verdict and the same reason string, with no test edited — which is the check that matters, since the tests are the accumulated record of the drift. _is_literal and _looks_numeric fall out entirely (only a test still imports _is_literal); that is another ~30 lines a real change would remove along with its test.

Wider run, measured on both sides:

tests/unit/test_condition_expression_block.py + test_workflows.py + test_extensions.py
  patched    22 failed, 1728 passed
  main       22 failed, 1728 passed      identical set, `diff` clean

Those 22 are the pre-existing symlink and bash-parity classes on unelevated Windows.

Mutation-checked, after confirming the edit applied: making the sink append a no-op fails 33 cases. The reporting line is load-bearing, not decoration.

What deliberately stays, and why

Not everything collapses, and I would rather say so than oversell this.

_ProbeNamespace answers every leaf lookup, so under a probe inputs.count+1 and inputs.some_value_not_known_yet both resolve. Only the shape of the segment separates a malformed path from a value that simply isn't known at validation time — so _PATH_SEGMENT has to stay. Root membership stays for the same reason. And item still needs its own line: _resolve_dot_path branches on isinstance(current, list) versus isinstance(current, dict), and no single probe object can be both, so the probe cannot represent the one root whose runtime type is Any.

That leaves three explicit rules, each about the namespace rather than about the grammar — and 1, 2, 4, 5 and 9 in the table above stop being possible rather than being fixed one at a time.

For assessment

Things I would want your call on before writing it up properly:

  1. Where the report lives. A module-level sink is the smallest change but it is global state; a ContextVar or an explicit on_leaf parameter threaded through _evaluate_simple_expression are both cleaner and both noisier at the call sites.
  2. Whether _resolve_dot_path is the better reporting point. It can distinguish unknown root from dead end at depth, which would let the gate drop _PATH_SEGMENT too — at the cost of touching the function every template render goes through.
  3. Whether the evaluator should carry this at all. It is diagnostics-only coupling in a hot path, and "no" is a reasonable answer — in which case the gate stays as it is and this issue is just the reason it looks the way it does.

Happy to open the PR if it is worth carrying; happy to leave it here if it isn't.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions