Skip to content

Commit b4309cc

Browse files
committed
fix(workflows): evaluate chained comparisons pairwise like Python
The expression evaluator split a comparison at the first operator in _COMPARISON_OPERATORS table order and evaluated the whole remainder as the right operand, so a chained comparison such as `0 <= inputs.n <= 10` became `0 <= (inputs.n <= 10)` — a number compared against a boolean. Validation accepted the condition and nothing was raised at run time, so an `if`, `while` or `do-while` step silently took the wrong branch. Scan the expression left to right for every top-level comparison operator (longest operator first at each position, with the same quote/bracket skipping as _find_top_level) and evaluate the chain pairwise with an implicit `and`, the way Python and Jinja2 chain comparisons: `0 <= n <= 10` is `0 <= n and n <= 10`. Single comparisons keep their previous semantics, and every operand is evaluated before any pair is compared so validation probes visit the same leaves regardless of which pair fails first. Fixes #5071 Assisted-by: Claude Code (model: Claude Fable 5, supervised)
1 parent 7370e59 commit b4309cc

2 files changed

Lines changed: 121 additions & 24 deletions

File tree

‎src/specify_cli/workflows/expressions/engine_current.py‎

Lines changed: 83 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,57 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An
153153
# evaluator will actually split on.
154154
_COMPARISON_OPERATORS = ("!=", "==", ">=", "<=", ">", "<", " not in ", " in ")
155155

156+
# The chain scan tries operators at one position, not across the whole string,
157+
# so at a given index the longest operator must win: "<=" is one operator, not
158+
# "<" followed by a dangling "=". Derived from _COMPARISON_OPERATORS so the two
159+
# tables cannot drift.
160+
_CHAIN_SCAN_OPERATORS = tuple(sorted(_COMPARISON_OPERATORS, key=len, reverse=True))
161+
162+
163+
def _split_comparison_chain(expr: str) -> tuple[list[str], list[str]] | None:
164+
"""Split *expr* at every top-level comparison operator, left to right.
165+
166+
Returns ``(operands, ops)`` with ``len(operands) == len(ops) + 1``, or
167+
``None`` when *expr* holds no top-level comparison operator. Occurrences
168+
inside quoted strings or brackets are skipped with the same scanning rules
169+
as ``_find_top_level``, so this never splits where the single-operator
170+
search would not have.
171+
"""
172+
operands: list[str] = []
173+
ops: list[str] = []
174+
start = 0
175+
quote: str | None = None
176+
depth = 0
177+
i = 0
178+
n = len(expr)
179+
while i < n:
180+
ch = expr[i]
181+
if quote is not None:
182+
if ch == quote:
183+
quote = None
184+
elif ch in ("'", '"'):
185+
quote = ch
186+
elif ch in "([{":
187+
depth += 1
188+
elif ch in ")]}":
189+
depth = max(0, depth - 1)
190+
elif depth == 0:
191+
matched = next(
192+
(op for op in _CHAIN_SCAN_OPERATORS if expr.startswith(op, i)), None
193+
)
194+
if matched is not None:
195+
operands.append(expr[start:i])
196+
ops.append(matched)
197+
i += len(matched)
198+
start = i
199+
continue
200+
i += 1
201+
if not ops:
202+
return None
203+
operands.append(expr[start:])
204+
return operands, ops
205+
206+
156207
# Set only while `_collect_leaves` probes an expression; None everywhere else, so
157208
# a normal evaluation costs one `.get()`. A ContextVar rather than a module global
158209
# so concurrent probes cannot append into each other's list.
@@ -164,7 +215,8 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any:
164215
165216
Supports:
166217
- Dot-path access: ``steps.specify.output.file``
167-
- Comparisons: ``==``, ``!=``, ``>``, ``<``, ``>=``, ``<=``
218+
- Comparisons: ``==``, ``!=``, ``>``, ``<``, ``>=``, ``<=``, chained
219+
pairwise as in Python/Jinja2 (``0 <= inputs.n <= 10``)
168220
- Boolean operators: ``and``, ``or``, ``not``
169221
- ``in``, ``not in``
170222
- Pipe filters: ``| default('...')``, ``| join(', ')``, ``| contains('...')``, ``| from_json``, ``| map('...')``, ``| split(',')``, ``| upper``, ``| lower``, ``| length``, ``| to_json``
@@ -282,30 +334,37 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any:
282334
inner = _evaluate_simple_expression(expr[4:].strip(), namespace)
283335
return not bool(inner)
284336

285-
# Comparison operators (order matters — check multi-char ops first). Split at
286-
# the first top-level occurrence so an operator inside a quoted operand is
287-
# ignored.
288-
for op in _COMPARISON_OPERATORS:
289-
op_idx = _find_top_level(expr, op)
290-
if op_idx != -1:
291-
left = _evaluate_simple_expression(expr[:op_idx].strip(), namespace)
292-
right = _evaluate_simple_expression(expr[op_idx + len(op):].strip(), namespace)
337+
# Comparison operators. Scan left to right for EVERY top-level occurrence
338+
# (longest operator first at each position, so "<=" is not read as "<"),
339+
# then evaluate pairwise with an implicit `and`, the way Python and Jinja2
340+
# chain comparisons: `0 <= n <= 10` is `0 <= n and n <= 10`. Splitting at
341+
# the first operator in _COMPARISON_OPERATORS order instead evaluated
342+
# `0 <= (n <= 10)` — a number compared against a boolean — so a range
343+
# check silently took the wrong branch (#5071). Operators inside quoted or
344+
# bracketed operands are still ignored. Every operand is evaluated before
345+
# any pair is compared, so a validation probe visits the same leaves
346+
# whichever pair fails first.
347+
chain = _split_comparison_chain(expr)
348+
if chain is not None:
349+
operands, ops = chain
350+
values = [
351+
_evaluate_simple_expression(operand.strip(), namespace)
352+
for operand in operands
353+
]
354+
for left, op, right in zip(values, ops, values[1:]):
293355
if op == "==":
294-
return left == right
295-
if op == "!=":
296-
return left != right
297-
if op == ">":
298-
return _safe_compare(left, right, ">")
299-
if op == "<":
300-
return _safe_compare(left, right, "<")
301-
if op == ">=":
302-
return _safe_compare(left, right, ">=")
303-
if op == "<=":
304-
return _safe_compare(left, right, "<=")
305-
if op == " in ":
306-
return _safe_membership(left, right, negate=False)
307-
if op == " not in ":
308-
return _safe_membership(left, right, negate=True)
356+
ok = left == right
357+
elif op == "!=":
358+
ok = left != right
359+
elif op == " in ":
360+
ok = _safe_membership(left, right, negate=False)
361+
elif op == " not in ":
362+
ok = _safe_membership(left, right, negate=True)
363+
else:
364+
ok = _safe_compare(left, right, op)
365+
if not ok:
366+
return False
367+
return True
309368

310369
# Numeric literal
311370
try:

‎tests/test_workflows.py‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -504,6 +504,44 @@ def test_operator_splitting_is_quote_aware(self):
504504
assert evaluate_expression("{{ inputs.a == 9 or inputs.b == 2 }}", plain) is True
505505
assert evaluate_expression("{{ inputs.missing | default('a and b') }}", plain) == "a and b"
506506

507+
def test_chained_comparisons_evaluate_like_python(self):
508+
"""A chained comparison such as `0 <= n <= 10` evaluates pairwise with
509+
an implicit `and`, as Python and Jinja2 chain comparisons. Splitting at
510+
the first operator in table order instead evaluated `0 <= (n <= 10)` —
511+
a number compared against a boolean — so a range check silently took
512+
the wrong branch (#5071)."""
513+
from specify_cli.workflows.expressions import (
514+
evaluate_condition,
515+
evaluate_expression,
516+
)
517+
from specify_cli.workflows.base import StepContext
518+
519+
inside = StepContext(inputs={"n": 5})
520+
outside = StepContext(inputs={"n": 50})
521+
for cond in ("{{ 0 <= inputs.n <= 10 }}", "{{ 1 < inputs.n < 10 }}"):
522+
assert evaluate_condition(cond, inside) is True, cond
523+
assert evaluate_condition(cond, outside) is False, cond
524+
525+
# Mixed operators chain pairwise too: `a < b == c` is
526+
# `a < b and b == c`, membership included.
527+
ctx = StepContext(inputs={"a": 1, "b": 5, "c": 5, "items": [5, 9]})
528+
assert evaluate_expression("{{ inputs.a < inputs.b == inputs.c }}", ctx) is True
529+
assert evaluate_expression("{{ inputs.a < inputs.b == 6 }}", ctx) is False
530+
assert evaluate_expression("{{ inputs.a < inputs.b in inputs.items }}", ctx) is True
531+
532+
# A chain inside a boolean expression binds tighter than and/or.
533+
assert evaluate_condition("{{ 0 <= inputs.n <= 10 and inputs.n == 5 }}", inside) is True
534+
assert evaluate_condition("{{ 0 <= inputs.n <= 10 or inputs.n == 50 }}", outside) is True
535+
536+
# Regression: single comparisons and operator text inside quoted
537+
# operands are untouched.
538+
assert evaluate_expression("{{ inputs.a < inputs.b }}", ctx) is True
539+
assert evaluate_expression("{{ inputs.b < inputs.a }}", ctx) is False
540+
assert evaluate_expression("{{ '<=' == '<=' }}", StepContext()) is True
541+
assert evaluate_expression(
542+
"{{ inputs.x == 'a <= b' }}", StepContext(inputs={"x": "a <= b"})
543+
) is True
544+
507545
def test_pipe_detection_is_quote_aware(self):
508546
from specify_cli.workflows.expressions import evaluate_expression
509547
from specify_cli.workflows.base import StepContext

0 commit comments

Comments
 (0)