fix: re.sub backslash-escape crash in PR comment section replace - #82
Merged
Conversation
insert_or_replace_section() passed generated section content directly as the replacement argument to re.sub(). re.sub interprets backslashes in a string replacement as escape/backreference syntax, so content containing sequences like "\x" or "\1" (e.g. raw test output, byte-string reprs) raised re.error: bad escape \x, reddening downstream CI. Observed deterministically on ovos-skill-volume PR #129's bus-coverage comment step. Fix by using a lambda replacement so the content is inserted verbatim.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesSection replacement safety
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
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
scripts/update_pr_comment.py,insert_or_replace_section()(line 1135), called:new_sectionis built bybuild_section()from arbitrary generated content (test output, coverage numbers, bus-message payloads, etc.) and passed tore.subas a string replacement.re.subinterprets backslashes in a string replacement as escape/backreference syntax, so content containing sequences like\xor\1raised:This reddened CI in downstream repos whenever a check's generated section content happened to contain a backslash sequence. Observed deterministically on
ovos-skill-volumePR #129'sbus-coveragecomment step, where the section content included raw\xbyte sequences from the bus message dump.Fix
Pass a callable (
lambda _match: new_section) as the replacement instead of a raw string, sore.subinserts the content verbatim without interpreting any backslash sequences in it. This was the onlyre.sub/re.subncall in the file with a dynamic replacement argument; audited the rest of the module and found no other instances.Test evidence
Added
TestInsertOrReplaceSectionBackslashSafetyintest/test_update_pr_comment.pycovering:\x00,\x01,\x4F\1,\g<name>)Red (before fix, on unmodified
insert_or_replace_section):Green (after fix):
Full existing suite (40 tests) still passes after the fix.
Summary by CodeRabbit
Bug Fixes
Tests
Independent review
Verified against current source (
origin/devdiff as posted):gh apithat this is the onlyre.sub/re.subncall with a dynamic replacement arg inscripts/update_pr_comment.py(grepped full file: only onere.subsite, at the fixed line).lambda _match: new_sectionfix is the textbook-correct way to avoidre.subinterpreting backslashes/backreferences in the replacement string — it does not change matching behavior (pattern/re.DOTALLuntouched), only how the replacement is substituted. No over-matching risk introduced.TestInsertOrReplaceSectionBackslashSafety) exercises both the replace-path and append-path with\x,\1,\g<name>sequences — this is a real regression test, not just a happy-path add.Issue list: none.
Verdict: MERGE
Safe for the human to merge? Yes — correctness fix, real regression test (fails before/passes after per PR's own red/green evidence), CI green, no other paths touched.