-
Notifications
You must be signed in to change notification settings - Fork 625
fix(utils): match_regex_list crashes on an empty-string pattern #6505
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1740,7 +1740,7 @@ def match_regex_list( | |
| return False | ||
|
|
||
| for item_matcher in regex_list: | ||
| if not substring_matching and item_matcher[-1] != "$": | ||
| if not substring_matching and (not item_matcher or item_matcher[-1] != "$"): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Empty pattern matches all stringsMedium Severity The empty-string guard avoids Reviewed by Cursor Bugbot for commit d42a94a. Configure here. |
||
| item_matcher += "$" | ||
|
Comment on lines
+1743
to
1744
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: An empty string Suggested FixInstead of converting an empty Prompt for AI AgentAlso affects:
Did we get this right? 👍 / 👎 to inform future reviews. |
||
|
|
||
| matched = re.search(item_matcher, item) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -562,6 +562,14 @@ def test_match_regex_list(item, regex_list, expected_result): | |
| assert match_regex_list(item, regex_list) == expected_result | ||
|
|
||
|
|
||
| def test_match_regex_list_empty_string_pattern(): | ||
| # An empty-string pattern must not raise IndexError (regression test). | ||
| result = match_regex_list("anything", [""]) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than have this be a standalone test, this case can be incorporated into the test above ( This means that last 2 assertions around foo/foobar can also be removed since we already have test cases that check similar behaviour within the array (lines 553 and 554 above) |
||
| assert isinstance(result, bool) | ||
| assert match_regex_list("foobar", ["foo"]) is False | ||
| assert match_regex_list("foo", ["foo"]) is True | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "version,expected_result", | ||
| [ | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this fixes the index error, the result that's returned from this change means
match_regex_list("anything", [""])will returnTrue.An empty string likely represents a typo/mistake on the developers part, and so we should treat this as if
[]was passed in.This would mean that this conditional should look like the following instead: