Skip to content

fix(workflows): reject a multi-argument filter call in expressions - #3893

Open
jawwad-ali wants to merge 2 commits into
github:mainfrom
jawwad-ali:fix/expressions-multiarg-filter
Open

fix(workflows): reject a multi-argument filter call in expressions#3893
jawwad-ali wants to merge 2 commits into
github:mainfrom
jawwad-ali:fix/expressions-multiarg-filter

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

_apply_filter parses a filter call with:

filter_match = re.fullmatch(r"(\w+)\((.+)\)", filter_expr)
...
farg = _evaluate_simple_expression(filter_match.group(2).strip(), namespace)

The entire captured argument text goes to _evaluate_simple_expression as a single expression. Every filter in this subset takes exactly one argument, so "1, 2" is not a valid expression — it evaluates to None.

Reproduction on current main (81bf741)

{{ 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

Two distinct failures:

  • default silently returns None — the opposite of the filter's entire purpose, with no error. A workflow using it gets an empty interpolation and carries on.
  • join raises a misleading error. It blames the separator ("expected a string separator, got NoneType") when the separator was fine and the real problem is the extra argument.

Fix

Fall through to the error this function already raises for a registered filter used wrongly:

filter 'default' used in an unsupported form (got '| default(1, 2)'):
  expected one of default or default('x'), join('sep'), map('attr'),
  contains('s'), or from_json

That message names the filter and lists the accepted forms, which is exactly the diagnostic the author needs.

The check has to be quote-aware

A single argument may legitimately contain a comma, so a plain split(",") would reject valid expressions. All of these must keep working, and are pinned by a new test:

{{ inputs.items | join(", ") }}          -> 'a, b'
{{ inputs.items | join(",") }}           -> 'a,b'
{{ inputs.missing | default("a, b") }}   -> 'a, b'

Hence _has_top_level_comma, which tracks quote state and only reports a comma outside a quoted span.

Breaking risk: the only expressions whose behaviour changes are multi-argument calls, which today either return None or raise a misleading error — neither is a form anyone can be relying on. Every single-argument form, the no-argument | default, and chained filters are unchanged; verified above and by the existing TestExpressions suite.

Verification

  • Fail-before / pass-after: test_multi_argument_filter_call_fails_loudly fails on unpatched src and passes with the fix. tests/test_workflows.py: 21 failed → 20 failed, 838 → 839 passed.
  • The remaining 20 are identical to the clean-main baseline captured on 81bf741 (all Windows symlink-privilege).
  • uvx ruff@0.15.0 check src tests → clean

Tests sit beside the existing filter-strictness tests (test_registered_filter_unsupported_form_raises, test_filter_call_with_trailing_tokens_fails_loudly), which established this exact "fail loudly rather than silently mis-evaluate" contract.


Written with assistance from Claude Code. Bug found, reproduced, and verified by me on current main.

@jawwad-ali
jawwad-ali requested a review from mnriem as a code owner July 31, 2026 08:09
@mnriem
mnriem requested a balanced review from Copilot August 7, 2026 16:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds strict validation for unsupported multi-argument workflow filter calls.

Changes:

  • Detects top-level commas in filter arguments.
  • Adds regression tests for multi-argument calls and quoted commas.
Show a summary per file
File Description
src/specify_cli/workflows/expressions.py Adds filter argument validation.
tests/test_workflows.py Tests invalid multiple arguments and valid quoted commas.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/specify_cli/workflows/expressions.py Outdated

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback

jawwad-ali and others added 2 commits August 15, 2026 16:20
`_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) <noreply@anthropic.com>
…r 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>
@jawwad-ali
jawwad-ali force-pushed the fix/expressions-multiarg-filter branch from 49ed86a to e36a61c Compare August 15, 2026 11:43
@mnriem
mnriem requested a balanced review from Copilot August 17, 2026 13:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment on lines +412 to +416
# 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})``).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants