Skip to content

Commit a89c8be

Browse files
jawwad-aliclaude
andcommitted
fix(workflows): refuse a filter mixed with a comparison operator
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>
1 parent 81bf741 commit a89c8be

2 files changed

Lines changed: 61 additions & 1 deletion

File tree

src/specify_cli/workflows/expressions.py

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -465,7 +465,27 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any:
465465
pipe_idx = _find_top_level(expr, "|")
466466
if pipe_idx != -1:
467467
segments = _split_top_level(expr, "|")
468-
value = _evaluate_simple_expression(segments[0].strip(), namespace)
468+
# The pipe is detected before the operators below, so a filter written on
469+
# the right-hand operand of a comparison was applied to the comparison's
470+
# BOOLEAN RESULT instead: `count > limit | default(5)` evaluated
471+
# `count > limit` first and then `default` on the bool, which is a no-op,
472+
# so the expression silently returned the comparison against the
473+
# *unfiltered* operand. This is the mirror of a filter followed by a
474+
# comparison (`default('7') > '5'`), which this module already refuses
475+
# rather than guessing at the intended precedence. Refuse both the same
476+
# way, so an ambiguous expression is reported instead of quietly
477+
# producing the answer the author did not ask for.
478+
head = segments[0].strip()
479+
for _op in ("!=", "==", ">=", "<=", ">", "<", " not in ", " in ",
480+
" or ", " and "):
481+
if _find_top_level(head, _op) != -1:
482+
raise ValueError(
483+
f"ambiguous filter precedence in '{expr}': "
484+
f"'| {segments[1].strip()}' would apply to the result of "
485+
f"'{head}', not to an operand of '{_op.strip()}'. Filter the "
486+
f"operand in its own expression instead."
487+
)
488+
value = _evaluate_simple_expression(head, namespace)
469489
for segment in segments[1:]:
470490
value = _apply_filter(value, segment.strip(), namespace)
471491
return value

tests/test_workflows.py

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -708,6 +708,46 @@ def test_filter_call_with_trailing_tokens_fails_loudly(self):
708708
StepContext(inputs={"tags": ["a", "b"]}),
709709
)
710710

711+
def test_filter_on_a_comparison_operand_is_refused(self):
712+
"""A filter mixed with a comparison must be reported, not guessed at.
713+
714+
The pipe is detected before the operators, so
715+
`count > limit | default(5)` evaluated `count > limit` first and then
716+
applied `default` to the resulting bool — a no-op — silently returning
717+
the comparison against the *unfiltered* operand (False, where the author
718+
meant `10 > 5` = True).
719+
720+
This is the mirror of a filter followed by a comparison
721+
(`default('7') > '5'`), which this module already refuses rather than
722+
guessing at the intended precedence. Both are now refused the same way.
723+
"""
724+
import pytest
725+
from specify_cli.workflows.expressions import evaluate_expression
726+
from specify_cli.workflows.base import StepContext
727+
728+
ctx = StepContext(inputs={"count": 10, "name": "x"})
729+
with pytest.raises(ValueError, match="ambiguous filter precedence"):
730+
evaluate_expression("{{ inputs.count > inputs.limit | default(5) }}", ctx)
731+
with pytest.raises(ValueError, match="ambiguous filter precedence"):
732+
evaluate_expression(
733+
'{{ inputs.name == inputs.other | default("x") }}', ctx
734+
)
735+
with pytest.raises(ValueError, match="ambiguous filter precedence"):
736+
evaluate_expression("{{ inputs.a and inputs.b | default(1) }}", ctx)
737+
738+
def test_plain_filters_and_chains_are_unaffected(self):
739+
"""Only a filter mixed with an operator is refused."""
740+
from specify_cli.workflows.expressions import evaluate_expression
741+
from specify_cli.workflows.base import StepContext
742+
743+
ctx = StepContext(inputs={"items": ["a", "b"], "count": 10, "name": "x"})
744+
assert evaluate_expression("{{ inputs.missing | default(5) }}", ctx) == 5
745+
assert evaluate_expression('{{ inputs.items | join(", ") }}', ctx) == "a, b"
746+
assert evaluate_expression("{{ inputs.items | contains('a') }}", ctx) is True
747+
# Operators without a filter, and filters without an operator, both fine.
748+
assert evaluate_expression("{{ inputs.count > 5 }}", ctx) is True
749+
assert evaluate_expression('{{ inputs.name == "x" }}', ctx) is True
750+
711751
def test_chained_filters_apply_left_to_right(self):
712752
# Filters chain: each filter's result feeds the next. `map` yields a
713753
# list and `join` is the only filter that renders a list to a string,

0 commit comments

Comments
 (0)