From 7f16c1f3519ae8b8e3a2bf1f3a52e50ed28532ad Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Fri, 31 Jul 2026 13:09:36 +0500 Subject: [PATCH 1/2] fix(workflows): reject a multi-argument filter call in expressions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_apply_filter` parses a call with `re.fullmatch(r"(\w+)\((.+)\)")` and hands the ENTIRE captured argument text to `_evaluate_simple_expression` as one expression. Every filter in this subset takes exactly one argument, so a two-argument call is not a valid expression and evaluates to None: {{ inputs.missing | default(1) }} -> 1 {{ inputs.missing | default(1, 2) }} -> None <-- silently wrong {{ inputs.name | join(",", "extra") }} -> ValueError: join: expected a string separator, got NoneType So `default` silently returns None instead of its default, and `join` raises a message blaming the separator rather than the extra argument. Fall through to the existing "unsupported form" error, which names the filter and lists the accepted forms: filter 'default' used in an unsupported form (got '| default(1, 2)'): ... The check is quote-aware, because a single argument may legitimately contain a comma — `join(", ")` and `default("a, b")` must keep working, so a plain split would reject valid expressions. Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/workflows/expressions.py | 29 +++++++++++++++++ tests/test_workflows.py | 40 ++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 38a29890ae..f5b520c595 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -366,6 +366,25 @@ def _find_top_level(text: str, token: str) -> int: return -1 +def _has_top_level_comma(text: str) -> bool: + """True when *text* has a comma outside any quoted span. + + Every filter in this subset takes exactly one argument, but that argument may + itself contain a comma -- ``join(", ")`` and ``default("a, b")`` are both + valid -- so the check has to be quote-aware rather than a plain ``split``. + """ + quote = "" + for ch in text: + if quote: + if ch == quote: + quote = "" + elif ch in ("'", '"'): + quote = ch + elif ch == ",": + return True + return False + + def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> Any: """Apply a single pipe filter segment to *value*. @@ -400,6 +419,16 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An # branch above. The greedy ``.+`` still handles literal ``)`` and ``|`` # inside quoted args. filter_match = re.fullmatch(r"(\w+)\((.+)\)", filter_expr) + # A multi-argument call is not a supported form: every filter here takes + # exactly one argument, and the whole captured argument text was handed to + # ``_evaluate_simple_expression`` as ONE expression. "1, 2" is not a valid + # expression, so it evaluated to None -- making ``default(1, 2)`` return + # None (silently wrong) and ``join(",", "extra")`` raise a message blaming + # the separator rather than the extra argument. Fall through to the + # unsupported-form error below instead, which names the filter and lists + # the accepted forms. The check is quote-aware so ``join(", ")`` still works. + if filter_match and _has_top_level_comma(filter_match.group(2)): + filter_match = None if filter_match: fname = filter_match.group(1) farg = _evaluate_simple_expression(filter_match.group(2).strip(), namespace) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index afd70adecf..78566d919c 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_multi_argument_filter_call_fails_loudly(self): + """A second argument must be reported, not silently mis-evaluated. + + The whole captured argument text was handed to + `_evaluate_simple_expression` as ONE expression. `"1, 2"` is not a valid + expression, so it evaluated to None — making `default(1, 2)` return None + (silently wrong) and `join(",", "extra")` raise a message blaming the + separator rather than the extra argument. + """ + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + with pytest.raises(ValueError, match="unsupported form"): + evaluate_expression( + "{{ inputs.missing | default(1, 2) }}", StepContext(inputs={}) + ) + with pytest.raises(ValueError, match="unsupported form"): + evaluate_expression( + '{{ inputs.tags | join(",", "extra") }}', + StepContext(inputs={"tags": ["a", "b"]}), + ) + + def test_single_argument_containing_a_comma_still_works(self): + """The multi-argument check must be quote-aware. + + `join(", ")` and `default("a, b")` are single arguments that happen to + contain a comma, so a plain split would reject valid expressions. + """ + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"tags": ["a", "b"]}) + assert evaluate_expression('{{ inputs.tags | join(", ") }}', ctx) == "a, b" + assert evaluate_expression('{{ inputs.tags | join(",") }}', ctx) == "a,b" + assert ( + evaluate_expression('{{ inputs.missing | default("a, b") }}', ctx) + == "a, b" + ) + 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 e36a61ce2a1b3874e5a4d45f9cc214e3e018ba01 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sat, 15 Aug 2026 16:42:55 +0500 Subject: [PATCH 2/2] fix(workflows): use the bracket-aware scanner for the multi-arg filter check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catch: my hand-rolled `_has_top_level_comma` was quote-aware but NOT bracket-aware, so it treated the comma inside a list literal as an argument separator. The evaluator supports list literals, so this rejected expressions that work on main today: main: {{ inputs.missing | default([1, 2]) }} -> [1, 2] with my PR: ValueError: filter 'default' used in an unsupported form That is a breaking change, not a fix. Drop the helper and use `_find_top_level`, the same scanner the operator splitting already uses — it skips commas inside quotes AND inside nested brackets. Verified: default([1, 2]) -> [1, 2] (restored) default([1,2]) -> [1, 2] (restored) default([]) -> [] (restored) join(", ") / default("a, b") -> unchanged default(1, 2) -> rejected (the actual bug) join(",", "extra") -> rejected default([1,2], 3) -> rejected (real 2nd arg after a literal) Dict literals resolve to None both before and after, matching main. Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/workflows/expressions.py | 29 ++++++---------------- tests/test_workflows.py | 31 +++++++++++++++++++++--- 2 files changed, 36 insertions(+), 24 deletions(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index f5b520c595..e30346f019 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -366,25 +366,6 @@ def _find_top_level(text: str, token: str) -> int: return -1 -def _has_top_level_comma(text: str) -> bool: - """True when *text* has a comma outside any quoted span. - - Every filter in this subset takes exactly one argument, but that argument may - itself contain a comma -- ``join(", ")`` and ``default("a, b")`` are both - valid -- so the check has to be quote-aware rather than a plain ``split``. - """ - quote = "" - for ch in text: - if quote: - if ch == quote: - quote = "" - elif ch in ("'", '"'): - quote = ch - elif ch == ",": - return True - return False - - def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> Any: """Apply a single pipe filter segment to *value*. @@ -426,8 +407,14 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An # None (silently wrong) and ``join(",", "extra")`` raise a message blaming # the separator rather than the extra argument. Fall through to the # unsupported-form error below instead, which names the filter and lists - # the accepted forms. The check is quote-aware so ``join(", ")`` still works. - if filter_match and _has_top_level_comma(filter_match.group(2)): + # the accepted forms. + # + # Use ``_find_top_level``, the same scanner the operator splitting uses: it + # skips commas inside quotes AND inside nested brackets, so a single + # argument that happens to contain a comma still works -- ``join(", ")``, + # ``default("a, b")``, and the list/dict literals the evaluator supports + # (``default([1, 2])``, ``default({"a": 1, "b": 2})``). + if filter_match and _find_top_level(filter_match.group(2), ",") != -1: filter_match = None if filter_match: fname = filter_match.group(1) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 78566d919c..ef4aa265e2 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -735,10 +735,16 @@ def test_multi_argument_filter_call_fails_loudly(self): ) def test_single_argument_containing_a_comma_still_works(self): - """The multi-argument check must be quote-aware. + """The multi-argument check must skip quotes AND nested brackets. - `join(", ")` and `default("a, b")` are single arguments that happen to - contain a comma, so a plain split would reject valid expressions. + A single argument may legitimately contain a comma in two ways: + + * inside quotes — `join(", ")`, `default("a, b")` + * inside a bracketed literal — `default([1, 2])`, which the expression + evaluator supports and which resolves to a real list + + so the check uses `_find_top_level` (the same scanner the operator + splitting uses) rather than a quote-only scan. """ from specify_cli.workflows.expressions import evaluate_expression from specify_cli.workflows.base import StepContext @@ -750,6 +756,25 @@ def test_single_argument_containing_a_comma_still_works(self): evaluate_expression('{{ inputs.missing | default("a, b") }}', ctx) == "a, b" ) + # List literals: a comma inside brackets is not an argument separator. + assert evaluate_expression( + "{{ inputs.missing | default([1, 2]) }}", ctx + ) == [1, 2] + assert evaluate_expression( + "{{ inputs.missing | default([1,2]) }}", ctx + ) == [1, 2] + assert evaluate_expression("{{ inputs.missing | default([]) }}", ctx) == [] + + def test_multi_argument_after_a_literal_is_still_rejected(self): + """A real second argument is rejected even when the first is a literal.""" + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + with pytest.raises(ValueError, match="unsupported form"): + evaluate_expression( + "{{ inputs.missing | default([1,2], 3) }}", StepContext(inputs={}) + ) def test_chained_filters_apply_left_to_right(self): # Filters chain: each filter's result feeds the next. `map` yields a