Skip to content

Commit a324625

Browse files
committed
fix(workflows): chain comparisons like Python in expressions
A chained comparison such as 0 <= inputs.n <= 10 grouped left to right, comparing a boolean with the next operand, so the range check answered True for n = 50 and an if/while step silently took the wrong branch. Evaluate comparisons as Python and Jinja2 do: a OP b OP c is a OP b and b OP c, with the shared operand evaluated once and evaluation stopping at the first false pair, so a later operand is only reached when every pair before it held (the same guard behavior as and). Explicit parentheses still end a chain: (0 <= n) <= 10 compares the grouped boolean, exactly as in Python. The grammar is unchanged; cmp nodes flatten into one chain node the way and/or and filter chains already do, a parenthesized comparison stays one grouped operand, and validation still walks every operand. Fixes #5071 Assisted-by: Claude Code (model: Claude Fable 5, supervised)
1 parent 08252af commit a324625

3 files changed

Lines changed: 140 additions & 48 deletions

File tree

‎docs/workflows/expressions.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ Notes on the newer filters:
6464
6565
`and` and `or` return `true` or `false` and evaluate their right side only when the left side does not decide the result: `and` skips it when the left side is false, and `or` skips it when the left side is true. A condition can therefore guard a filter that would fail on an empty value. `{{ inputs.payload != '' and (inputs.payload | from_json | length) }}` is false for an empty optional `payload` instead of failing the run on invalid JSON. When the right side is evaluated, its errors still fail the run.
6666

67-
Operators bind from loosest to tightest as `or`, `and`, `not`, then the comparisons. All comparisons (`==`, `!=`, `>`, `<`, `>=`, `<=`, `in`, `not in`) share one level and are evaluated left to right; none has priority over another. `{{ a == b > 0 }}` therefore means `{{ (a == b) > 0 }}`. To combine comparisons, use parentheses or `and`/`or`.
67+
Operators bind from loosest to tightest as `or`, `and`, `not`, then the comparisons. All comparisons (`==`, `!=`, `>`, `<`, `>=`, `<=`, `in`, `not in`) share one level and chain as in Python and Jinja2: `{{ 0 <= inputs.n <= 10 }}` means `{{ 0 <= inputs.n and inputs.n <= 10 }}`, with the shared operand evaluated once and evaluation stopping at the first false pair — a later operand is evaluated only when every pair before it held, with the same guard behavior as `and`. Explicit parentheses end a chain: `{{ (0 <= inputs.n) <= 10 }}` compares the grouped `true`/`false` result with `10`, exactly as in Python.
6868

6969
The body of every `{{ }}` block must be a valid expression. Text that is not one, such as prose (`{{ some text }}`), a key with characters other than letters, digits, `_` and `-` (`{{ inputs.weird:key }}`), or arithmetic (`{{ count + 1 }}`), raises a `ValueError` naming the offending token. Numbers may be written as `5`, `-5`, `+5`, `1_000`, `0.5`, `.5`, `5.`, or `1.5e3`.
7070

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

Lines changed: 56 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,12 @@
44
parsed here. The language is the same with these deliberate differences, all
55
documented in https://github.github.io/spec-kit/workflows/expressions.html:
66
7-
* All comparison operators share one precedence level and group left to right
8-
(``a == b > 0`` is ``(a == b) > 0``). The former engine's operator scan order
9-
gave them accidental priorities; parenthesize to combine comparisons.
7+
* All comparison operators share one precedence level and chain like Python
8+
and Jinja2 (``0 <= n <= 10`` is ``0 <= n and n <= 10``, with ``n`` evaluated
9+
once and evaluation stopping at the first false pair). The former engine's
10+
operator scan order gave them accidental priorities and compared a boolean
11+
to the next operand; explicit parentheses still end a chain, so
12+
``(0 <= n) <= 10`` compares the grouped result, exactly as in Python.
1013
* Text that is not an expression -- prose inside ``{{ }}``, path keys with
1114
characters outside ``[\\w-]`` -- is a syntax error instead of a path lookup,
1215
which could resolve an existing key.
@@ -207,6 +210,13 @@ def path(self, token: Token) -> tuple[str, str]:
207210
return ("path", str(token))
208211

209212
def group(self, inner: Any) -> Any:
213+
# Parentheses end a comparison chain: ``(0 <= n) <= 10`` compares the
214+
# grouped result instead of extending the chain, as in Python. The
215+
# marker is unwrapped by ``_eval`` and ``_visit``; every other node is
216+
# safe to splice in directly (re-grouping ``and``/``or`` operands does
217+
# not change their result).
218+
if inner[0] == "chain":
219+
return ("group", inner)
210220
return inner
211221

212222
def list_body(self, *items: Any) -> tuple[Any, ...]:
@@ -228,7 +238,14 @@ def filtered(self, head: Any, filt: Any) -> tuple[str, Any, tuple[Any, ...]]:
228238
return ("pipe", head, (filt,))
229239

230240
def cmp(self, left: Any, op: Token, right: Any) -> tuple[str, ...]:
231-
return ("cmp", " ".join(str(op).split()), left, right)
241+
# As for ``filtered`` and ``and_op``/``or_op``: the grammar nests a
242+
# chain to the left; flatten it into one node so ``a < b < c`` keeps
243+
# all its operands and operators side by side. ``("group", chain)``
244+
# does not flatten, so a parenthesized comparison is one operand.
245+
op_text = " ".join(str(op).split())
246+
if left[0] == "chain":
247+
return ("chain", left[1] + (right,), left[2] + (op_text,))
248+
return ("chain", (left, right), (op_text,))
232249

233250
def or_op(self, left: Any, _op: Token, right: Any) -> tuple[str, tuple[Any, ...]]:
234251
# As for ``filtered``: the grammar nests a chain to the left; flatten it
@@ -399,6 +416,19 @@ def _apply_filter(
399416
)
400417

401418

419+
def _compare_pair(left: Any, right: Any, op: str) -> bool:
420+
"""One comparison of a chain, with the shared operator semantics."""
421+
if op == "==":
422+
return _safe_equals(left, right, negate=False)
423+
if op == "!=":
424+
return _safe_equals(left, right, negate=True)
425+
if op == "in":
426+
return _safe_membership(left, right, negate=False)
427+
if op == "not in":
428+
return _safe_membership(left, right, negate=True)
429+
return _safe_compare(left, right, op)
430+
431+
402432
def _eval(node: Any, namespace_: dict[str, Any]) -> Any:
403433
"""Evaluate a tuple AST node against *namespace_*."""
404434
tag = node[0]
@@ -426,19 +456,23 @@ def _eval(node: Any, namespace_: dict[str, Any]) -> Any:
426456
if tag == "not":
427457
return not bool(_eval(node[1], namespace_))
428458

429-
if tag == "cmp":
430-
op = node[1]
431-
left = _eval(node[2], namespace_)
432-
right = _eval(node[3], namespace_)
433-
if op == "==":
434-
return _safe_equals(left, right, negate=False)
435-
if op == "!=":
436-
return _safe_equals(left, right, negate=True)
437-
if op == "in":
438-
return _safe_membership(left, right, negate=False)
439-
if op == "not in":
440-
return _safe_membership(left, right, negate=True)
441-
return _safe_compare(left, right, op)
459+
if tag == "group":
460+
return _eval(node[1], namespace_)
461+
462+
if tag == "chain":
463+
# Python/Jinja2 chained-comparison semantics: ``a < b < c`` is
464+
# ``a < b and b < c`` with ``b`` evaluated once, left to right,
465+
# stopping at the first false pair, so a later operand is only
466+
# evaluated when every pair before it held. Validation walks the
467+
# parse tree instead, so it still visits every operand.
468+
operands, ops = node[1], node[2]
469+
left = _eval(operands[0], namespace_)
470+
for op, operand in zip(ops, operands[1:]):
471+
right = _eval(operand, namespace_)
472+
if not _compare_pair(left, right, op):
473+
return False
474+
left = right
475+
return True
442476

443477
if tag in ("and", "or"):
444478
# Left to right, stopping at the first operand that decides the result,
@@ -551,9 +585,11 @@ def _visit(node: Any, paths: list[str] | None = None, filters: list[Any] | None
551585
elif tag in ("and", "or"):
552586
for operand in node[1]:
553587
_visit(operand, paths, filters)
554-
elif tag == "cmp":
555-
_visit(node[2], paths, filters)
556-
_visit(node[3], paths, filters)
588+
elif tag == "group":
589+
_visit(node[1], paths, filters)
590+
elif tag == "chain":
591+
for operand in node[1]:
592+
_visit(operand, paths, filters)
557593

558594

559595
def _collect_leaves(text: str) -> list[str]:

‎tests/test_workflows.py‎

Lines changed: 83 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1762,45 +1762,101 @@ def test_boolean_precedence_without_parentheses(self):
17621762
# Explicit grouping overrides the default and flips the result.
17631763
assert evaluate_expression("{{ (inputs.a or inputs.b) and inputs.c }}", ctx) is False
17641764

1765-
def test_comparisons_share_one_level_and_group_left_to_right(self):
1766-
"""No comparison operator has priority over another.
1767-
1768-
The former engine's operator scan order made ``==``/``!=`` bind looser
1769-
than ``<``/``>``/``in`` and grouped repeated comparisons from the right.
1770-
The grammar puts every comparison on one level, left-associative, so
1771-
``a == b > 0`` is ``(a == b) > 0``. Each case is checked against its
1772-
explicit grouping and against the other grouping, which gives a
1773-
different result, so the assertion fails if the precedence moves.
1765+
def test_comparisons_chain_like_python(self):
1766+
"""Comparisons share one level and chain as in Python and Jinja2 (#5071).
1767+
1768+
``a OP b OP c`` means ``a OP b and b OP c`` with the shared operand
1769+
evaluated once -- not ``(a OP b) OP c``. The former left-to-right
1770+
grouping compared a boolean with the next operand, so the range check
1771+
``0 <= inputs.n <= 10`` answered True for ``n = 50`` and an
1772+
``if``/``while`` step silently took the wrong branch. Every case is
1773+
asserted against Python's own result for the same expression.
17741774
"""
17751775
from specify_cli.workflows.expressions import evaluate_expression
17761776
from specify_cli.workflows.base import StepContext
17771777

17781778
ctx = StepContext(
1779-
inputs={"enabled": True, "count": 5, "mode": "x", "tags": ["x"]}
1780-
)
1781-
cases = [
1782-
("inputs.enabled == inputs.count > 0",
1783-
"(inputs.enabled == inputs.count) > 0",
1784-
"inputs.enabled == (inputs.count > 0)"),
1785-
("true == inputs.mode in inputs.tags",
1786-
"(true == inputs.mode) in inputs.tags",
1787-
"true == (inputs.mode in inputs.tags)"),
1788-
("inputs.enabled != inputs.count >= 10",
1789-
"(inputs.enabled != inputs.count) >= 10",
1790-
"inputs.enabled != (inputs.count >= 10)"),
1791-
("1 < 2 < 3", "(1 < 2) < 3", "1 < (2 < 3)"),
1792-
("3 > 2 > 1", "(3 > 2) > 1", "3 > (2 > 1)"),
1779+
inputs={"n": 50, "m": 5, "enabled": True, "count": 5,
1780+
"mode": "x", "tags": ["x"]}
1781+
)
1782+
chained = [
1783+
# The #5071 repro: out of range must be False.
1784+
("0 <= inputs.n <= 10", 0 <= 50 <= 10),
1785+
("0 <= inputs.m <= 10", 0 <= 5 <= 10),
1786+
("1 < 2 < 3", 1 < 2 < 3),
1787+
("3 > 2 > 1", 3 > 2 > 1),
1788+
("1 < 2 > 1", 1 < 2 > 1),
1789+
("'a' < 'b' < 'c'", "a" < "b" < "c"),
1790+
("inputs.count == 5 == 5", 5 == 5 == 5),
1791+
# Mixed operators chain pairwise instead of comparing a boolean.
1792+
("inputs.enabled == inputs.count > 0", True == 5 > 0),
1793+
("inputs.enabled != inputs.count >= 10", True != 5 >= 10),
1794+
("true == inputs.mode in inputs.tags", True == "x" in ["x"]),
1795+
# Four operands, three pairs.
1796+
("1 <= 1 == 1 != 2", 1 <= 1 == 1 != 2),
17931797
]
1794-
for plain, left, right in cases:
1795-
result = evaluate_expression("{{ " + plain + " }}", ctx)
1796-
assert result == evaluate_expression("{{ " + left + " }}", ctx), plain
1797-
assert result != evaluate_expression("{{ " + right + " }}", ctx), plain
1798+
for expr, want in chained:
1799+
assert evaluate_expression("{{ " + expr + " }}", ctx) is want, expr
1800+
# A chain is its explicit and-form.
1801+
assert evaluate_expression(
1802+
"{{ 0 <= inputs.n <= 10 }}", ctx
1803+
) == evaluate_expression("{{ 0 <= inputs.n and inputs.n <= 10 }}", ctx)
1804+
# Explicit parentheses end a chain, exactly as in Python: the grouped
1805+
# boolean is the operand of the next comparison.
1806+
assert evaluate_expression(
1807+
"{{ (0 <= inputs.n) <= 10 }}", ctx
1808+
) is ((0 <= 50) <= 10)
1809+
assert evaluate_expression("{{ 1 < (2 < 3) }}", ctx) is (1 < (2 < 3))
17981810
# Comparisons still bind tighter than not/and/or.
17991811
assert evaluate_expression("{{ not inputs.count > 9 }}", ctx) is True
18001812
assert evaluate_expression(
18011813
"{{ inputs.count > 9 or inputs.mode == 'x' }}", ctx
18021814
) is True
18031815

1816+
def test_chained_comparison_short_circuits_like_and(self):
1817+
"""A failing pair stops the chain before later operands run (#5071).
1818+
1819+
Mirrors the ``and`` guard: a later operand is evaluated only when
1820+
every pair before it held, so a chain can guard a filter that would
1821+
fail on an empty or invalid value. When the operand is reached, its
1822+
errors still fail the run.
1823+
"""
1824+
from specify_cli.workflows.expressions import evaluate_expression
1825+
from specify_cli.workflows.base import StepContext
1826+
1827+
ctx = StepContext(inputs={"payload": "not json", "n": 0})
1828+
# 5 <= 0 fails first, so the raising filter operand is never reached.
1829+
assert evaluate_expression(
1830+
"{{ 5 <= inputs.n <= (inputs.payload | from_json | length) }}", ctx
1831+
) is False
1832+
# A reached operand still raises.
1833+
with pytest.raises(ValueError, match="from_json"):
1834+
evaluate_expression(
1835+
"{{ 0 <= inputs.n <= (inputs.payload | from_json | length) }}",
1836+
ctx,
1837+
)
1838+
1839+
def test_chained_comparison_evaluates_the_shared_operand_once(self, monkeypatch):
1840+
"""``a <= b <= c`` resolves ``b`` once, as Python does (#5071).
1841+
1842+
The explicit and-form resolves it twice; the chain must not.
1843+
"""
1844+
import specify_cli.workflows.expressions.engine as engine_mod
1845+
from specify_cli.workflows.expressions import evaluate_expression
1846+
from specify_cli.workflows.base import StepContext
1847+
1848+
calls: list[str] = []
1849+
real = engine_mod._resolve_dot_path
1850+
1851+
def counting(namespace_, path):
1852+
calls.append(path)
1853+
return real(namespace_, path)
1854+
1855+
monkeypatch.setattr(engine_mod, "_resolve_dot_path", counting)
1856+
ctx = StepContext(inputs={"m": 5})
1857+
assert evaluate_expression("{{ 0 <= inputs.m <= 10 }}", ctx) is True
1858+
assert calls.count("inputs.m") == 1
1859+
18041860
def test_number_literal_forms(self):
18051861
"""Every number form the former engine read through ``int()``/``float()``.
18061862

0 commit comments

Comments
 (0)