Skip to content

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

Open
jawwad-ali wants to merge 1 commit 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 1 commit 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.

`_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>
@jawwad-ali
jawwad-ali requested a review from mnriem as a code owner July 31, 2026 08:09
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.

1 participant