Fix RegexExtract crashing/returning None on non-participating capture groups#15031
Fix RegexExtract crashing/returning None on non-participating capture groups#15031vidigoat wants to merge 1 commit into
Conversation
In the 'First Group' and 'All Groups' modes, RegexExtract called match.group(group_index) after only checking len(match.groups()), without checking that the group actually participated in the match. When the requested group is optional or on a non-taken alternation branch, match.group() is None: - 'First Group' returned None from a node whose output is a String, leaking a non-string value into downstream string nodes. - 'All Groups' appended None and then crashed in join() with an uncaught TypeError, aborting the workflow. Guard both branches with 'is not None' so a non-participating group yields an empty string. Normal matches are unaffected. Adds a regression test.
|
✅ All contributors have signed the CLA. Thank you! This PR is ready to be merged. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (8)
|
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately summarizes the main change: fixing RegexExtract handling of non-participating capture groups. |
| Description check | ✅ Passed | The description directly explains the bug, the fix, and the added tests, so it is clearly related to the changeset. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
I have read and agree to the Contributor License Agreement |
Problem
The
RegexExtractnode (Extract Text) crashes or leaks a non-string value when a requested capture group didn't participate in the match.In "First Group" and "All Groups" modes the code only checks
len(match.groups()) >= group_index, then callsmatch.group(group_index). When the group is optional or on a non-taken alternation branch,match.group()returnsNone:So a common pattern with an alternation or optional group either silently sends
Noneinto string-consuming nodes ("First Group") or hard-crashes prompt execution ("All Groups").Fix
Guard both branches with
is not None, so a non-participating group yields""(matching the node's existing "no match" behaviour). Normal matches are unchanged.Added
tests-unit/comfy_extras_test/nodes_string_test.pycovering both modes with non-participating groups plus the normal paths; it fails before this change (None return + TypeError) and passes after. The full test runs green against the real node.Disclosure: I used an AI assistant while working on this; I reproduced the bug, wrote the fix and test, and verified everything myself.