Skip to content

fix: fail loudly on an unknown workflow expression filter#3074

Merged
mnriem merged 3 commits into
github:mainfrom
doquanghuy:fix/3073-unknown-filter-fail-loud
Jun 22, 2026
Merged

fix: fail loudly on an unknown workflow expression filter#3074
mnriem merged 3 commits into
github:mainfrom
doquanghuy:fix/3073-unknown-filter-fail-loud

Conversation

@doquanghuy

@doquanghuy doquanghuy commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes #3073.

The expression evaluator's filter dispatch fell through to return value for any unregistered filter, so a typo'd or unsupported filter — e.g. {{ items | length }} — rendered the value unchanged with no error, and the run completed. A silent wrong result, with nothing pointing at the cause.

This makes the unknown-filter path fail loudly: an unrecognized filter raises a clear ValueError naming the offending filter and the valid ones, reusing the same strict-ValueError style already used for from_json (which raises on its mis-wired forms precisely to avoid "silently falling through to the unknown-filter path and returning the unparsed value"). It closes that path in general, consistent with the fail-loud direction of #2957 (fan-out items) and #2961 (from_json).

  • The five registered filters (default / join / map / contains / from_json) are unchanged — no behavior change for any valid usage.
  • Both forms of an unknown filter are caught: the no-arg form (| length) and the name(arg) form (| upper('x')), which previously also fell through silently.

Backward-compat note (calling it out honestly): this turns a previously silent author error into a loud one. Any workflow that today relies on an unknown filter being silently dropped would now error. That passthrough was never intentional (the from_json strictness comment already names it as a path to avoid), and a scan of the repo's own workflow/template YAMLs found no {{ … | <unknown-filter> }} usage, and the full suite stays green — but flagging it so you can weigh the direction. Happy to rework as a warning-instead-of-raise, or gate it, if you'd prefer.

Testing

  • Tested locally with uv run specify --help
  • Ran existing tests with uv sync && uv run pytest — full suite 3913 passed, 59 skipped
  • Tested with a sample project (if applicable) — ran a workflow whose shell step used {{ [1,2,3] | length }}: pre-fix it printed count=[1, 2, 3] and completed; post-fix the run fails loudly with unknown filter 'length'.

Tests in TestExpressions (tests/test_workflows.py):

  • test_filter_unknown_name_raises| length raises ValueError.
  • test_filter_unknown_name_with_args_raises| upper('x') raises (the name(arg) form).
  • test_registered_filter_unsupported_form_raises — a registered filter in an unsupported form (| join, | map with no arg) raises a misused-filter message, not "unknown filter".
  • test_registered_filters_unaffected — regression: all five registered filters still work, now including map('attr').

The two unknown-filter tests are red against main (DID NOT RAISE), green with the fix.

AI Disclosure

  • I did use AI assistance (describe below)

This change was authored with Claude Code: the AI drafted the fix and the tests and ran the suite. I discovered the behavior through end-to-end workflow-expression testing, verified the root cause and the red-against-main / green-with-fix tests myself, and reviewed the diff. The fix deliberately reuses the existing from_json error style rather than inventing a new one.

The expression evaluator's filter dispatch fell through to `return value`
for any unregistered filter, so a typo'd or unsupported filter such as
`{{ items | length }}` rendered the value unchanged with no error and the
run completed — a silent wrong result.

Raise a clear ValueError instead, naming the offending filter and the valid
ones, mirroring the strict handling already used for `from_json`. The five
registered filters (default/join/map/contains/from_json) are unchanged; the
`name(arg)` form of an unknown filter is now caught too.
@doquanghuy

Copy link
Copy Markdown
Contributor Author

@mnriem this is the fix for #3073 — fail-loud on unknown expression filters, reusing the from_json ValueError style; the five registered filters are unchanged and the full suite stays green. Happy to adjust the direction (warn instead of raise, or gate it) if you'd prefer.

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

This PR updates the workflow expression evaluator to fail loudly when encountering an unknown (or otherwise unhandled) pipe filter, preventing silent passthrough that can hide typos and produce incorrect workflow results.

Changes:

  • Change filter dispatch in evaluate_expression to raise ValueError instead of silently returning the unfiltered value for unrecognized filters.
  • Add tests asserting that unknown filters raise (both | name and | name(arg) forms).
  • Add a regression test intended to ensure registered filters keep working.
Show a summary per file
File Description
src/specify_cli/workflows/expressions.py Replaces the silent unknown-filter fallback with a ValueError that includes the offending filter and expected forms.
tests/test_workflows.py Adds coverage for unknown-filter failures and a regression test for existing filter behavior.

Copilot's findings

Tip

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

  • Files reviewed: 2/2 changed files
  • Comments generated: 2

Comment thread tests/test_workflows.py
Comment thread src/specify_cli/workflows/expressions.py Outdated
…er map

Address the review feedback on the unknown-filter fail-loud path:

- A *registered* filter used in an unsupported form (e.g. `| join` or
  `| map` with no argument) raised the misleading "unknown filter
  '<name>'" — the filter is registered, the syntax isn't. It now raises
  a message naming it as a known filter misused. A new
  `_REGISTERED_FILTERS` constant drives the distinction.
- `test_registered_filters_unaffected` now also exercises `map('attr')`,
  which it previously claimed to cover but didn't. Add
  `test_registered_filter_unsupported_form_raises` to pin the new path.
@doquanghuy

Copy link
Copy Markdown
Contributor Author

@mnriem — pushed d37e440 addressing the two Copilot review comments:

  1. Misleading "unknown filter" for a registered filter in an unsupported form (expressions.py). A registered filter used with the wrong syntax — e.g. | join or | map with no argument — hit the same error as a genuinely unknown name and was reported as unknown filter 'join', which is wrong (the filter is registered; the form isn't). The no-match path now splits: a name in the recognized set raises filter 'join' used in an unsupported form (got '| join'): expected …, while a truly unknown name keeps unknown filter '<name>': …. A new _REGISTERED_FILTERS constant drives the distinction.

  2. map was asserted-unaffected but untested. test_registered_filters_unaffected now exercises map('attr') ({{ rows | map('id') }} == ["a","b"]) alongside the other four. Added test_registered_filter_unsupported_form_raises covering the | join / | map no-arg path.

The five registered filters are unchanged; full suite 3913 passed, 59 skipped, ruff check clean. Happy to tune the unsupported-form wording if you'd prefer a per-filter expected form.

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.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 1

Comment thread src/specify_cli/workflows/expressions.py
Copilot review: the hint listed default('x') but omitted the valid
no-argument default form (| default), which this module supports.
@doquanghuy

Copy link
Copy Markdown
Contributor Author

@mnriem pushed e7e64ba — the filter-error hint now lists the no-arg default form too (default or default('x')), per the latest Copilot note. Suite green (3913 passed).

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.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new

@mnriem mnriem merged commit 1cb9359 into github:main Jun 22, 2026
11 checks passed
@mnriem

mnriem commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Thank you!

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.

[Bug]: workflow expression evaluator silently ignores unknown filters

3 participants