From a7a19cfcbc74c262b2267086ab47d9c4fb545014 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 31 Jul 2026 13:17:46 +0500 Subject: [PATCH 1/2] 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 38a29890ae..438e3ea42e 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 afd70adecf..461c7f1dbf 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -711,6 +711,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, From c99496068a1fab05cf89ff75bbc25378f1d4f240 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sat, 15 Aug 2026 16:52:46 +0500 Subject: [PATCH 2/2] fix(workflows): detect a leading unary `not` in the ambiguity check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/specify_cli/workflows/expressions.py | 31 +++++++++++++++++------- tests/test_workflows.py | 28 +++++++++++++++++++++ 2 files changed, 50 insertions(+), 9 deletions(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 438e3ea42e..b362d7664d 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -476,15 +476,28 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: # 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." - ) + # Unary ``not`` is a leading prefix, not an infix token, so it has no + # surrounding space for the scan below to match -- it has to be checked + # the same way the parser itself does (``expr.startswith("not ")``). + # Without this, ``not inputs.missing | default(1)`` still evaluated + # ``not inputs.missing`` first and applied the filter to that boolean, + # which is the exact mis-binding this guard exists to reject. + # (A ``not`` that follows ``and``/``or`` is already caught by those + # tokens below.) + _ambiguous_op = "not" if head.startswith("not ") else None + if _ambiguous_op is None: + for _op in ("!=", "==", ">=", "<=", ">", "<", " not in ", " in ", + " or ", " and "): + if _find_top_level(head, _op) != -1: + _ambiguous_op = _op.strip() + break + if _ambiguous_op is not None: + 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 '{_ambiguous_op}'. 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) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 461c7f1dbf..98d3915f78 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -738,6 +738,34 @@ def test_filter_on_a_comparison_operand_is_refused(self): with pytest.raises(ValueError, match="ambiguous filter precedence"): evaluate_expression("{{ inputs.a and inputs.b | default(1) }}", ctx) + def test_filter_after_a_unary_not_is_refused(self): + """Unary `not` mis-binds the same way and must be caught too. + + `not` is a leading prefix rather than an infix token (the parser tests + it with `expr.startswith("not ")`), so it has no surrounding space for + the operator scan to match. Without an explicit check, + `not inputs.missing | default(1)` evaluated `not inputs.missing` first + and applied `default` to that boolean — a no-op — silently returning + True where the author meant `not 1` = False. + """ + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"value": 0, "flag": True}) + with pytest.raises(ValueError, match="ambiguous filter precedence"): + evaluate_expression("{{ not inputs.missing | default(1) }}", ctx) + with pytest.raises(ValueError, match="ambiguous filter precedence"): + evaluate_expression("{{ not inputs.value | default(1) }}", ctx) + # A `not` that follows and/or is already caught by that token. + with pytest.raises(ValueError, match="ambiguous filter precedence"): + evaluate_expression( + "{{ inputs.flag and not inputs.value | default(1) }}", ctx + ) + # Plain unary `not`, with no filter, is untouched. + assert evaluate_expression("{{ not inputs.value }}", ctx) is True + assert evaluate_expression("{{ not inputs.flag }}", ctx) is False + 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