fix(workflows): raise a clear error, not a cryptic crash, on non-string filter args#3522
Open
Noor-ul-ain001 wants to merge 1 commit into
Open
fix(workflows): raise a clear error, not a cryptic crash, on non-string filter args#3522Noor-ul-ain001 wants to merge 1 commit into
Noor-ul-ain001 wants to merge 1 commit into
Conversation
…ng filter args
The `map`, `join`, and `contains` expression filters assumed their
argument was a string. A non-string argument — an authoring mistake such
as `| map(5)`, `| join(5)`, or `| contains(5)` — reached an operation
that only strings support and raised a cryptic exception that escaped the
evaluator entirely:
* `map(5)` -> `attr.split(".")` -> AttributeError
* `join(5)` -> `separator.join(...)` -> AttributeError
* `contains(5)` on a string value -> `x in str` -> TypeError
The engine wraps neither expression evaluation nor `step_impl.execute()`
in a try/except, so each of these took down the whole run with a message
that names none of the real problem.
Validate the argument type up front and raise a `ValueError` naming the
filter and the offending type instead, mirroring the strict argument
handling already in `from_json`. `contains` guards only the string-value
branch: for a list value, membership of any element type is legitimate
(`5 in [1, 2, 5]`), so that branch is intentionally left unguarded.
Co-authored-by: Claude Opus 4.8 (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.
Summary
The
map,join, andcontainsexpression filters assumed their argument was a string. A non-string argument — an authoring mistake such as| map(5),| join(5), or| contains(5)— reached an operation that only strings support and raised a cryptic exception that escaped the evaluator entirely:| map(5)attr.split(".")AttributeError: 'int' object has no attribute 'split'| join(5)separator.join(...)AttributeError: 'int' object has no attribute 'join'| contains(5)on a stringx in strTypeError: 'in <string>' requires string as left operand, not intThe engine wraps neither expression evaluation nor
step_impl.execute()in a try/except, so each of these took down the whole run with a message that names none of the real problem.Fix
Validate the argument type up front and raise a
ValueErrornaming the filter and the offending type instead, mirroring the strict argument handling already infrom_json.containsguards only the string-value branch: for a list value, membership of any element type is legitimate (5 in [1, 2, 5]), so that branch is intentionally left unguarded.Tests
Added to
TestExpressions:map/join/contains(on a string) with a non-string arg each raise a clearValueError.containson a list with a non-string arg stays valid (5 in [1,2,5]→ True).test_registered_filters_unaffectedregression coverage is unchanged.All 46
TestExpressionstests pass locally.Follows the same "fail loudly instead of crashing the run" pattern as the merged
from_json, non-listwait_for, and non-mappingcasesfixes.