fix(workflows): make pipe-filter split quote/bracket-aware#3261
Closed
Quratulain-bilal wants to merge 1 commit into
Closed
fix(workflows): make pipe-filter split quote/bracket-aware#3261Quratulain-bilal wants to merge 1 commit into
Quratulain-bilal wants to merge 1 commit into
Conversation
_evaluate_simple_expression split on the first '|' with a naive
'if "|" in expr' / expr.split('|', 1), unlike the and/or/comparison
splits which use _find_top_level to ignore separators inside quoted
operands or nested brackets. so a '|' inside a quoted string - e.g. a
gate/when condition like {{ inputs.mode == 'read|write' }} - was treated
as a filter pipe and raised ValueError("unknown filter 'write'"),
crashing the whole expression. use _find_top_level(expr, '|') so only a
top-level pipe starts a filter. also fixes join('|') (pipe as the
separator argument). add a regression test mirroring the existing
quote-aware operator test.
Contributor
Author
|
closing as duplicate — this exact fix already landed in #3232 (same root cause, same _find_top_level approach). sorry for the noise. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a parsing bug in the workflow expression evaluator where _evaluate_simple_expression() would incorrectly treat a | that appears inside a quoted string (or nested brackets) as a filter separator, causing spurious “unknown filter …” errors and breaking valid comparisons like inputs.mode == 'read|write'.
Changes:
- Update pipe-filter splitting in
_evaluate_simple_expression()to split only on the first top-level|using_find_top_level(expr, "|"). - Add a regression test ensuring pipe splitting is quote-aware and that
join('|')continues to work correctly.
Show a summary per file
| File | Description |
|---|---|
| src/specify_cli/workflows/expressions.py | Make pipe-filter splitting quote/bracket-aware by splitting only at top-level ` |
| tests/test_workflows.py | Add regression test covering ` |
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: 0
- Review effort level: Low
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.
closes #3260
what
_evaluate_simple_expressionsplit on the first|with a naiveif "|" in expr/expr.split("|", 1). unlike the and/or/in/comparison splits (which use_find_top_levelto ignore separators inside quoted operands or nested brackets), the pipe split had no such guard. so a|inside a quoted string was treated as a filter pipe:this crashed any workflow gate/
whencondition or template comparing against a string literal containing|.fix
use
_find_top_level(expr, "|")so only a top-level pipe starts a filter, mirroring the existing operator handling. one-spot change. as a side effect it also fixesjoin('|')(pipe as the separator argument), which previously mis-split.tests
added
test_pipe_splitting_is_quote_awareintests/test_workflows.py, mirroring the existingtest_operator_splitting_is_quote_aware. verified it fails on the pre-fix code (ValueError: unknown filter 'write') and passes after. fulltests/test_workflows.py: 313 passed, 1 skipped.note: this is separate from #3208/#3228 (which fix the multi-expression
fullmatchpath inevaluate_expression); this touches the pipe split in_evaluate_simple_expression, a different spot.note: i used an ai assistant to help investigate and write this up.