fix(workflows): refuse a filter mixed with a comparison operator instead of silently mis-binding it - #3894
Conversation
There was a problem hiding this comment.
Pull request overview
Rejects ambiguous filter/operator expressions instead of silently misapplying filters.
Changes:
- Detects comparison and boolean operators before filters.
- Adds regression and unaffected-behavior tests.
- Unary
notremains unhandled.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/workflows/expressions.py |
Adds ambiguous precedence validation. |
tests/test_workflows.py |
Adds filter precedence regression tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
mnriem
left a comment
There was a problem hiding this comment.
Address Copilot feedback
The pipe is detected before the boolean/comparison operators, so a filter
written on the right-hand operand was applied to the comparison's BOOLEAN
RESULT instead of to the operand:
{{ inputs.count > inputs.limit | default(5) }} -> False
With count=10 and limit missing, `count > limit` is evaluated first and
`default` is then applied to the resulting bool — a no-op, since a bool is
never empty — so the expression silently returns the comparison against
the *unfiltered* operand. The author meant `10 > 5` = True.
This module already refuses the mirror case rather than guessing:
{{ inputs.missing | default('7') > '5' }}
-> ValueError: filter 'default' used in an unsupported form
Same ambiguity, opposite handling. Refuse both the same way so an
ambiguous expression is reported instead of quietly producing the answer
the author did not ask for.
No legitimate expression is affected: applying `default` to a bool is a
no-op, and `join`/`map`/`contains` on a bool is an error, so there is no
working use of a filter on a comparison result.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review catch: unary `not` is a leading prefix, not an infix token, so it has
no surrounding space for the operator scan to match — the parser itself
tests it with `expr.startswith("not ")`. It was therefore absent from the
guard, and the mis-binding this PR exists to reject survived:
{{ not inputs.missing | default(1) }} -> True
`not inputs.missing` is evaluated first and `default` is applied to that
boolean (a no-op), so the expression silently returns True where the author
meant `not 1` = False.
Check the prefix the same way the parser does. A `not` that follows
`and`/`or` was already caught by those tokens. Verified:
not inputs.missing | default(1) -> refused (operand of 'not')
not inputs.value | default(1) -> refused (operand of 'not')
inputs.count > inputs.limit | default(5) -> refused (operand of '>')
inputs.flag and not inputs.value | default(1) -> refused (operand of 'and')
not inputs.value / not inputs.flag -> unchanged (True / False)
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a89c8be to
c994960
Compare
|
@mnriem — I believe this one is ready; the CHANGES_REQUESTED looks stale rather than outstanding. Timeline:
So the review that requested changes predates the fix by 8 days, and Copilot's own re-review afterwards came back clean. Current state: 0 unresolved threads, no suppressed low-confidence comments, MERGEABLE. For completeness on the substance — the gap was that the ambiguity scan matched infix tokens, so a leading unary still bound the filter to the boolean rather than being rejected. Fixed by matching the prefix form the parser itself uses ( No action needed from me that I can see, but happy to rebase or address anything else if you'd rather I did. |
|
Thank you! |
Problem
In
_evaluate_simple_expressionthe pipe is detected before the boolean and comparison operators:So a filter written on the right-hand operand of a comparison is applied to the comparison's boolean result, not to the operand.
Reproduction on current
main(81bf741)With
inputs.count = 10andinputs.limitmissing:count > limitis evaluated first, thendefaultis applied to the resulting bool — which is a no-op, because a bool is never empty. The expression silently returns the comparison against the unfiltered operand. The author meant10 > 5→True.This module already refuses the mirror case
A filter followed by a comparison is explicitly rejected rather than guessed at:
…and the test that pins it (
test_filter_call_with_trailing_tokens_fails_loudly) states the reasoning:Same ambiguity, opposite handling — one raises, the other quietly returns a wrong answer. This PR applies the decision that was already made to the mirror case; it is not proposing a new precedence rule.
Fix
When the segment before the first top-level pipe contains a top-level comparison or boolean operator, report it:
Why nothing legitimate breaks
There is no working use of a filter on a comparison result:
defaulton a bool is a no-op (a bool is neverNoneor empty)join/map/containson a bool is an errorSo every expression this now refuses was already producing either a wrong value or an error. Verified unaffected:
The operator scan uses the same
_find_top_levelhelper and the same token list as the operator parsing below it, so a>inside a quoted operand is not mistaken for one.Verification
test_filter_on_a_comparison_operand_is_refusedfails on unpatchedsrcand passes with the fix.tests/test_workflows.py: 21 failed → 20 failed, 838 → 839 passed.mainbaseline captured on81bf741(all Windows symlink-privilege).uvx ruff@0.15.0 check src tests→ cleanTests sit beside the existing filter-strictness tests, and a second test pins that plain filters, chains, and bare operators are untouched.
Written with assistance from Claude Code. Bug found, reproduced, and verified by me on current
main.