fix(condition): assert_in/assert_not_in wrongly reject falsy values present in the list - #218
Open
chuenchen309 wants to merge 1 commit into
Open
Conversation
…resent in the list _assert_in and _assert_not_in short-circuit on `if not value`, but this checks the truthiness of the value being tested for membership, not whether it's absent. A falsy value that is legitimately present in the expected list (e.g. an empty string in a picklist that allows an empty option) is silently rejected by "in" and silently accepted by "not in" before the actual membership check ever runs. Every other comparator in this file (_assert_equal, _assert_greater_than, etc.) guards with `if value is None`, not `if not value` -- these two are the only outliers. Fix: match the file's own established pattern, guard on `value is None` instead. Verified: new test (test_process_conditions_in_matches_falsy_value_present_in_list) red before fix, green after; full test_condition_processor.py suite (5 tests) plus tests/nodes/if_else/ (24 tests) pass; full test suite (604 tests) passes; ruff + ty clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
All contributors on this pull request have signed the CLA. |
Author
|
I have read the CLA Document and I hereby sign the CLA |
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.
Bug
_assert_in/_assert_not_inshort-circuit onif not value, but this checks the truthiness of the value being tested for membership, not whether it's absent. A falsy value that is legitimately present in the expected list (e.g. an empty string in a picklist that allows an empty option) is silently rejected byinand silently accepted bynot in, before the actual membership check ever runs.Every other comparator in this file (
_assert_equal,_assert_greater_than,_assert_null, etc.) guards withif value is None, notif not value—_assert_in/_assert_not_inare the only outliers.Fix
Match the file's own established pattern: guard on
value is Noneinstead ofnot value.Tests
test_process_conditions_in_matches_falsy_value_present_in_list(via the realConditionProcessor.process_conditionspath, matching this file's existing test style): red before the fix, green after.tests/utils/test_condition_processor.py(5 tests) +tests/nodes/if_else/(24 tests, the node that consumes this processor) pass.ruff check/ruff format --check/ty checkclean on the changed files.AI disclosure
Found, reproduced, and fixed with AI assistance (Claude Code), which noticed the inconsistency by comparing
_assert_in/_assert_not_in's guard against every other comparator function in the same file. I independently reproduced the pre-fix bug and the fix's correctness by running the tests directly before opening this PR.