From a89c8be10165314745f1450552718fba9f64e613 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 31 Jul 2026 13:17:46 +0500 Subject: [PATCH] fix(workflows): refuse a filter mixed with a comparison operator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/specify_cli/workflows/expressions.py | 22 ++++++++++++- tests/test_workflows.py | 40 ++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index a38cd6cb68..ba6ffbb3db 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -465,7 +465,27 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: pipe_idx = _find_top_level(expr, "|") if pipe_idx != -1: segments = _split_top_level(expr, "|") - value = _evaluate_simple_expression(segments[0].strip(), namespace) + # The pipe is detected before the operators below, so a filter written on + # the right-hand operand of a comparison was applied to the comparison's + # BOOLEAN RESULT instead: `count > limit | default(5)` evaluated + # `count > limit` first and then `default` on the bool, which is a no-op, + # so the expression silently returned the comparison against the + # *unfiltered* operand. This is the mirror of a filter followed by a + # comparison (`default('7') > '5'`), which this module already refuses + # rather than guessing at the intended precedence. Refuse both the same + # way, so an ambiguous expression is reported instead of quietly + # producing the answer the author did not ask for. + head = segments[0].strip() + for _op in ("!=", "==", ">=", "<=", ">", "<", " not in ", " in ", + " or ", " and "): + if _find_top_level(head, _op) != -1: + raise ValueError( + f"ambiguous filter precedence in '{expr}': " + f"'| {segments[1].strip()}' would apply to the result of " + f"'{head}', not to an operand of '{_op.strip()}'. Filter the " + f"operand in its own expression instead." + ) + value = _evaluate_simple_expression(head, namespace) for segment in segments[1:]: value = _apply_filter(value, segment.strip(), namespace) return value diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 57536081ea..85b8938c7e 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -708,6 +708,46 @@ def test_filter_call_with_trailing_tokens_fails_loudly(self): StepContext(inputs={"tags": ["a", "b"]}), ) + def test_filter_on_a_comparison_operand_is_refused(self): + """A filter mixed with a comparison must be reported, not guessed at. + + The pipe is detected before the operators, so + `count > limit | default(5)` evaluated `count > limit` first and then + applied `default` to the resulting bool — a no-op — silently returning + the comparison against the *unfiltered* operand (False, where the author + meant `10 > 5` = True). + + This is the mirror of a filter followed by a comparison + (`default('7') > '5'`), which this module already refuses rather than + guessing at the intended precedence. Both are now refused the same way. + """ + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"count": 10, "name": "x"}) + with pytest.raises(ValueError, match="ambiguous filter precedence"): + evaluate_expression("{{ inputs.count > inputs.limit | default(5) }}", ctx) + with pytest.raises(ValueError, match="ambiguous filter precedence"): + evaluate_expression( + '{{ inputs.name == inputs.other | default("x") }}', ctx + ) + with pytest.raises(ValueError, match="ambiguous filter precedence"): + evaluate_expression("{{ inputs.a and inputs.b | default(1) }}", ctx) + + def test_plain_filters_and_chains_are_unaffected(self): + """Only a filter mixed with an operator is refused.""" + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"items": ["a", "b"], "count": 10, "name": "x"}) + assert evaluate_expression("{{ inputs.missing | default(5) }}", ctx) == 5 + assert evaluate_expression('{{ inputs.items | join(", ") }}', ctx) == "a, b" + assert evaluate_expression("{{ inputs.items | contains('a') }}", ctx) is True + # Operators without a filter, and filters without an operator, both fine. + assert evaluate_expression("{{ inputs.count > 5 }}", ctx) is True + assert evaluate_expression('{{ inputs.name == "x" }}', ctx) is True + def test_chained_filters_apply_left_to_right(self): # Filters chain: each filter's result feeds the next. `map` yields a # list and `join` is the only filter that renders a list to a string,