fix(workflows): reject a multi-argument filter call in expressions - #3893
Open
jawwad-ali wants to merge 1 commit into
Open
fix(workflows): reject a multi-argument filter call in expressions#3893jawwad-ali wants to merge 1 commit into
jawwad-ali wants to merge 1 commit into
Conversation
`_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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
_apply_filterparses a filter call with:The entire captured argument text goes to
_evaluate_simple_expressionas a single expression. Every filter in this subset takes exactly one argument, so"1, 2"is not a valid expression — it evaluates toNone.Reproduction on current
main(81bf741)Two distinct failures:
defaultsilently returnsNone— the opposite of the filter's entire purpose, with no error. A workflow using it gets an empty interpolation and carries on.joinraises 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:
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: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
Noneor 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 existingTestExpressionssuite.Verification
test_multi_argument_filter_call_fails_loudlyfails on unpatchedsrcand passes with the fix.tests/test_workflows.py: 21 failed → 20 failed, 838 → 839 passed.mainbaseline captured on81bf741(all Windows symlink-privilege).uvx ruff@0.15.0 check src tests→ cleanTests 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.