Skip to content

Commit e36a61c

Browse files
jawwad-aliclaude
andcommitted
fix(workflows): use the bracket-aware scanner for the multi-arg filter check
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) <noreply@anthropic.com>
1 parent 7f16c1f commit e36a61c

2 files changed

Lines changed: 36 additions & 24 deletions

File tree

src/specify_cli/workflows/expressions.py

Lines changed: 8 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -366,25 +366,6 @@ def _find_top_level(text: str, token: str) -> int:
366366
return -1
367367

368368

369-
def _has_top_level_comma(text: str) -> bool:
370-
"""True when *text* has a comma outside any quoted span.
371-
372-
Every filter in this subset takes exactly one argument, but that argument may
373-
itself contain a comma -- ``join(", ")`` and ``default("a, b")`` are both
374-
valid -- so the check has to be quote-aware rather than a plain ``split``.
375-
"""
376-
quote = ""
377-
for ch in text:
378-
if quote:
379-
if ch == quote:
380-
quote = ""
381-
elif ch in ("'", '"'):
382-
quote = ch
383-
elif ch == ",":
384-
return True
385-
return False
386-
387-
388369
def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> Any:
389370
"""Apply a single pipe filter segment to *value*.
390371
@@ -426,8 +407,14 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An
426407
# None (silently wrong) and ``join(",", "extra")`` raise a message blaming
427408
# the separator rather than the extra argument. Fall through to the
428409
# unsupported-form error below instead, which names the filter and lists
429-
# the accepted forms. The check is quote-aware so ``join(", ")`` still works.
430-
if filter_match and _has_top_level_comma(filter_match.group(2)):
410+
# the accepted forms.
411+
#
412+
# Use ``_find_top_level``, the same scanner the operator splitting uses: it
413+
# skips commas inside quotes AND inside nested brackets, so a single
414+
# argument that happens to contain a comma still works -- ``join(", ")``,
415+
# ``default("a, b")``, and the list/dict literals the evaluator supports
416+
# (``default([1, 2])``, ``default({"a": 1, "b": 2})``).
417+
if filter_match and _find_top_level(filter_match.group(2), ",") != -1:
431418
filter_match = None
432419
if filter_match:
433420
fname = filter_match.group(1)

tests/test_workflows.py

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -735,10 +735,16 @@ def test_multi_argument_filter_call_fails_loudly(self):
735735
)
736736

737737
def test_single_argument_containing_a_comma_still_works(self):
738-
"""The multi-argument check must be quote-aware.
738+
"""The multi-argument check must skip quotes AND nested brackets.
739739

740-
`join(", ")` and `default("a, b")` are single arguments that happen to
741-
contain a comma, so a plain split would reject valid expressions.
740+
A single argument may legitimately contain a comma in two ways:
741+
742+
* inside quotes — `join(", ")`, `default("a, b")`
743+
* inside a bracketed literal — `default([1, 2])`, which the expression
744+
evaluator supports and which resolves to a real list
745+
746+
so the check uses `_find_top_level` (the same scanner the operator
747+
splitting uses) rather than a quote-only scan.
742748
"""
743749
from specify_cli.workflows.expressions import evaluate_expression
744750
from specify_cli.workflows.base import StepContext
@@ -750,6 +756,25 @@ def test_single_argument_containing_a_comma_still_works(self):
750756
evaluate_expression('{{ inputs.missing | default("a, b") }}', ctx)
751757
== "a, b"
752758
)
759+
# List literals: a comma inside brackets is not an argument separator.
760+
assert evaluate_expression(
761+
"{{ inputs.missing | default([1, 2]) }}", ctx
762+
) == [1, 2]
763+
assert evaluate_expression(
764+
"{{ inputs.missing | default([1,2]) }}", ctx
765+
) == [1, 2]
766+
assert evaluate_expression("{{ inputs.missing | default([]) }}", ctx) == []
767+
768+
def test_multi_argument_after_a_literal_is_still_rejected(self):
769+
"""A real second argument is rejected even when the first is a literal."""
770+
import pytest
771+
from specify_cli.workflows.expressions import evaluate_expression
772+
from specify_cli.workflows.base import StepContext
773+
774+
with pytest.raises(ValueError, match="unsupported form"):
775+
evaluate_expression(
776+
"{{ inputs.missing | default([1,2], 3) }}", StepContext(inputs={})
777+
)
753778

754779
def test_chained_filters_apply_left_to_right(self):
755780
# Filters chain: each filter's result feeds the next. `map` yields a

0 commit comments

Comments
 (0)