Skip to content

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

Open
jawwad-ali wants to merge 3 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 3 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
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 thread src/specify_cli/workflows/expressions.py Outdated

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.

🟢 Approval recommended

The bracket-aware validation correctly addresses the bug with comprehensive regression coverage.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@mnriem mnriem added the triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review label Sep 9, 2026
@mnriem

mnriem commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

This is a clean fix — green review, reproduced, disclosed, and CI passes. The only blocker is that the branch now conflicts with main (it's from late July and expressions.py has moved since). Please rebase on main and resolve. Heads up: there are three other in-flight PRs on expressions.py (#4460, #4416, #4417), so once you rebase, double-check your _apply_filter change still sits cleanly alongside those. Re-request review after the rebase.

@mnriem mnriem added author-awaiting Waiting on author response author-needs-rebase Branch conflicts with main — rebase/resolve before merge labels Sep 9, 2026
jawwad-ali and others added 3 commits September 11, 2026 20:08
`_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>
…omment

Addresses review feedback: `_evaluate_simple_expression` implements a list
literal branch only. A mapping such as `{"a": 1}` has no branch there and
falls through to dot-path resolution:

    list literal  [1, 2]     -> [1, 2]
    dict literal  {"a": 1}   -> None

So the comment's claim that the evaluator supports dict literals was wrong.

Comment-only; no behaviour change. `_find_top_level` is brace-aware as well as
bracket-aware (`_find_top_level('{"a": 1, "b": 2}', ',')` returns -1), so the
scanner treats such a comma as nested either way -- the example was simply
describing syntax the evaluator does not implement, which is exactly the kind
of thing a future change might have relied on.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jawwad-ali
jawwad-ali force-pushed the fix/expressions-multiarg-filter branch from c59cf00 to 5536c88 Compare September 11, 2026 15:11
@jawwad-ali

Copy link
Copy Markdown
Contributor Author

Rebased onto current mainMERGEABLE again.

The conflict came from my own merged #3894, which added test_filter_on_a_comparison_operand_is_refused and test_filter_after_a_unary_not_is_refused at the same insertion point in TestExpressions. Both sides were different tests in the same place, so I resolved by taking main's file wholesale and re-inserting only this PR's three unique tests, rather than hand-merging the interleaved hunks:

  • test_multi_argument_filter_call_fails_loudly
  • test_single_argument_containing_a_comma_still_works
  • test_multi_argument_after_a_literal_is_still_rejected

All five now coexist; the diff against main is purely additive (87 insertions, 0 deletions).

Re-verified after the rebase:

  • Fail-before/pass-after still holds — with expressions.py reverted to upstream/main, test_multi_argument_filter_call_fails_loudly fails; with the fix, 24 passed.
  • No duplicate test or class names introduced (that would silently shadow another PR's tests).
  • Full tests/test_workflows.py: 20 failed / 960 passed, against a clean-main baseline of 22 failed / 955 passed on the same machine — five more passing and no new failures. The remaining failures are the known Windows os.replace flakiness in that file, which rotates between runs and is unrelated to this change.
  • uvx ruff@0.15.0 check src tests clean.

@mnriem — the Copilot thread on this PR was addressed back in c59cf00 and Copilot's follow-up review on Sep 1 filed nothing further, so I believe the CHANGES_REQUESTED here predates the fix. Happy to take another look if anything is still outstanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author-awaiting Waiting on author response author-needs-rebase Branch conflicts with main — rebase/resolve before merge triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants