refactor(workflows): let the evaluator report its own leaves (#4274) - #4460
refactor(workflows): let the evaluator report its own leaves (#4274)#4460ntdatt812 wants to merge 3 commits into
Conversation
…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 github#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 github#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.
|
Thanks — this is the right root-cause fix: having the evaluator report its own leaves instead of |
mnriem
added
author-needs-disclosure
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.
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 github#4416) - when the evaluator stops treating something as a leaf, the gate stops checking it, with no gate edit (the grouped-operand shape from github#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.
|
Thanks — both points addressed, and the second one turned up something I had missed in my own PR. (1) AI disclosure — added to the PR body, and it applies to this comment too. I used Claude Code as a coding agent for the code, the tests and the measurements below, reviewing and directing throughout. Sorry for the omission on the original submission; an oversight rather than an attempt to hide it. (2) The coordination question. I did not want to answer this from reading, so I applied the evaluator-side hunks only of #4416 and #4417 on top of this branch — no edit to
So the answer is yes, but only after a fix I had to add. My refactor as you first saw it did not cover the negative-index case. The gate had stopped restating the operator grammar, but it still restated the shape of a path segment — The parenthesised case needed nothing — it already worked, because once the evaluator stops treating a group as a leaf, the leaf sink stops reporting it. One correction to my own framing, since it bears on your sequencing: this refactor does not fix their bugs. On tests: Happy to rebase whenever suits the order you pick. |
|
Thanks — the disclosure's added, and I really appreciate you actually running the #4416/#4417-hunks experiment rather than just asserting compatibility. That settles the sequencing: I'll land this refactor first, then #4416/#4417 rebase down to just their evaluator hunks. No worries on the original disclosure omission. Approving the CI run; once it's green I'll review for merge. |
mnriem
removed
the
author-needs-disclosure
There was a problem hiding this comment.
🟡 Changes recommended
Probe exceptions can prematurely stop traversal and hide later unresolved leaves.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Refactors condition validation to derive resolvable leaves directly from the expression evaluator, preventing duplicated grammar rules.
Changes:
- Adds ContextVar-based evaluator leaf collection.
- Shares indexed path-segment parsing between evaluation and validation.
- Adds regression and sink-isolation tests.
File summaries
| File | Description |
|---|---|
src/specify_cli/workflows/expressions.py |
Implements evaluator-driven leaf reporting and shared path validation. |
tests/unit/test_condition_expression_block.py |
Tests leaf collection, sink cleanup, and shared evaluator definitions. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| except Exception: # noqa: BLE001 - probe values, reported by _evaluator_rejects | ||
| pass |
Implements #4274, which was the write-up you asked for at the end of #4230.
The problem
_unresolvable_termanswers one question — does every operand in this condition resolve to something? — by walking the expression itself: filters, thenor/and/not, then comparisons, then list literals, down to the leaves.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, and both say so:_looks_numeric— "Mirror the evaluator's numeric literal test exactly." A barefloat()accepts1e3; the evaluator does not._is_literal— "Mirror the evaluator's literal tests exactly."startswith/endswithaccepts'a' 'b'; the evaluator does not.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. Seven of the nine findings in #4230 were that same defect in different clothes — the gate disagreeing with the evaluator about where the operands are. Each round fixed one shape; nothing stopped a tenth.
The change
_evaluate_simple_expressionhas 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. So let the evaluator report what it reaches:_collect_leavesruns the probe_evaluator_rejectsalready uses, with the sink armed._unresolvable_leafkeeps only the namespace rules — root membership, path-segment shape, and theitemindex narrowing from the last round of #4230. The gate now contains no grammar at all.A
ContextVarrather than a module global, so concurrent probes cannot append into each other's list; it isNoneoutside a probe, so a normal evaluation costs one.get().Two properties this rests on
Both are asserted rather than assumed, because if either changed the gate would go quietly blind rather than fail:
or/andare not short-circuited._evaluate_simple_expressionevaluates both sides and only then combines them, so a leaf is recorded whatever the other side is worth.test_both_sides_of_a_boolean_are_reportedpins it.A probe run can raise on its own placeholder values, which is what
_evaluator_rejectssorts out. The leaves seen before that point are real — the evaluator reached them — so they are kept rather than discarded. Discarding them would losebogusininputs.tags | join(bogus), which is the filter-argument case an earlier round of #4230 had to add by hand.test_leaves_seen_before_a_probe_error_are_keptpins it.Verification
expressions.py: 109 lines removed, 84 added.All 336 existing tests pass unchanged, including the 20 cases of
test_operands_must_be_literals_or_known_pathsthat took eight rounds to get right. That is the main evidence: the new gate agrees with the old one on every shape review found, without knowing about any of them.One test changed rather than passed:
test_literal_test_mirrors_the_evaluatortested the mirror, and the mirror is gone. It becomestest_literal_handling_comes_from_the_evaluatorand asserts the same knowledge —1e3is not a number to the evaluator,'a' 'b'is not one literal — through_unresolvable_term. That is the property that actually mattered; the old test could pass while the two had drifted.Five tests added for the mechanism itself. Checked by breaking it:
_unresolvable_termsuiteitem[0]narrowingtest_an_indexed_item_root_keeps_the_correctionuvx ruff@0.15.0 check src tests— clean. Wider run (tests/unit,test_workflows.py,test_extensions.py): 1988 passed, and the set of failing test names is identical before and after — 24 symlink tests that cannot run unprivileged on Windows.Update: the segment grammar was still duplicated, and #4416 / #4417 (2026-09-09)
c6c3ef7finishes what the first commit started. The gate had stopped restating the operator grammar, but it still restated the shape of a path segment:_PATH_SEGMENTand an inlinere.fullmatchboth described the index form that_resolve_dot_pathmatches with its own regex — three copies of one rule, kept in step by hand. That form is now named once as_INDEXED_SEGMENTbeside_resolve_dot_path, and the gate asks it. The regex is copied verbatim, so behaviour is unchanged; what changes is that widening indexing now reaches the gate for free.That was not cosmetic. I measured it before writing it:
c6c3ef7task_list[-1].fileNone'task_list[-1]' is not a valid path segment(inputs.a or inputs.b) and inputs.cFalseAnswering your question directly. I took the evaluator-side hunks only of #4416 and #4417 — no edit to
_unresolvable_term,_PATH_SEGMENTor anything else in the gate — and applied them on top of this branch:task_list[-1].file'b.md'task_list[0].file'a.md'(inputs.a or inputs.b) and inputs.cTrue(inputs.n)5inputs.a or inputs.b and inputs.cTrueSo yes: once this lands, both of @NgoQuocViet2001's PRs reduce to their evaluator hunks. The gate-side changes they each carry — #4416's widened
_PATH_SEGMENTandindexed_root, #4417's "mirror the evaluator's group unwrapping" block in_unresolvable_term— become unnecessary, because the gate no longer has an opinion of its own to keep in sync. Their diffs shrink and stop touching the region this PR rewrites, which should make the rebase mechanical rather than a merge argument.One correction to my own framing, since it matters for sequencing: this refactor does not fix their bugs.
task_list[-1]still resolves toNoneon this branch and(a or b) and cstill reads false — those are evaluator defects and their PRs are what fix them. What this branch changes is that the gate now agrees with the evaluator instead of holding a second opinion, so their one-line evaluator fixes are sufficient on their own.dd4b723adds two regression tests for exactly that property, because it is easy to claim and easy to lose — a fresh copy of the grammar in the gate would keep every other test green. One patches_INDEXED_SEGMENTand asserts the gate follows; the other makes the evaluator stop treating a group as a leaf and asserts the gate stops checking it. I verified both by reintroducing the drift (giving the gate its own segment regex again), which fails the first with the real message rather than an import error. I deliberately did not mirror their test bodies: assertingtask_list[-1] == 'b.md'here would be a test for a fix this PR does not contain, and would pass or fail based on whether their PR is present.tests/unit/test_condition_expression_block.py: 343 passed.tests/test_workflows.py+ that file: the set of failing test names is byte-identical before and after these two commits — 20 symlink tests that cannot run unprivileged on Windows.AI disclosure
Per CONTRIBUTING: this pull request was developed with AI assistance — I used Claude Code as a coding agent for the code, the tests and the measurements above, reviewing and directing it throughout, and the experiment applying #4416/#4417's hunks was run and checked by me. This comment and the PR body were also written with that assistance. Apologies for the omission on the original submission; it was an oversight, not an attempt to hide it.