Treat an Escaped Backtick as a Literal, Not a Span Delimiter - #1180
Conversation
Qodo on the promotion PR #1179, and it is real. CODE_SPAN read a backslash-escaped backtick as an opening run, so it paired with the next real tick and masked everything between them. That is the silent direction: a genuine unknown section disappears and the digest closes the loop on a body nobody read. Reproduced against a control body that reports the section the escaped tick hid. Both runs now exclude a tick preceded by a backslash. The guard reads one backslash, so an escaped backslash under-masks rather than over-masks, which is the safe direction: a quoted tag reported as a section refuses loudly, where a hidden section does not. Also trimmed the CODE_SPAN comment from fifteen lines to four, which Qodo raised separately. The prose gate requires one sentence per line, so the lever is fewer sentences rather than tighter wrapping. A regression test covers the escaped tick and fails with the guard reverted.
PR Summary by QodoTreat Escaped Backticks as Literals in Code-Span Masking
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe review parser now excludes escaped backticks from inline-code span openings. It preserves closing-delimiter and paragraph-boundary behavior. Regression tests cover escaped delimiters, backslashes inside spans, and quoted markers. ChangesReview marker parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This is a localized correction to escaped-backtick handling with regression coverage; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟢 Approval recommended
The regex change matches the stated masking intent and is covered by a focused regression test that exercises the previously silent failure mode.
Pull request overview
Updates the PR review body parser’s inline-code masking so a backslash-escaped backtick is treated as a literal character (not the start/end of a code span), preventing real <summary> sections from being accidentally hidden during unrecognized-marker detection.
Changes:
- Tighten
CODE_SPANto ignore backticks preceded by a backslash when determining span boundaries. - Add a regression test proving an escaped backtick no longer suppresses a real
<summary>marker.
File summaries
| File | Description |
|---|---|
| scripts/pr_review.py | Adjusts the CODE_SPAN regex so ``` does not open/close a masked inline-code span. |
| scripts/tests/test_pr_review.py | Adds a regression test covering the escaped-backtick scenario that previously hid real sections. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Qodo on #1180, and it is right: guarding the closing run the same way as the opener skipped a valid close. A backslash is literal inside a code span rather than an escape, so `` `a\` `` is a span ending in a backslash. Reading its close as escaped let the span run on and mask the section after it, which is the same silent failure the opener guard was added to prevent. Reproduced before changing anything, against a body whose section disappears with the closer guarded and reports with it removed. The opener keeps the guard, since a backslash before an opening run does escape it. A regression test covers the span ending in a backslash and fails with the closer guard put back.
|
Answering both Qodo findings on head 2. Backslash skips valid span closer — real, fixed in
|
There was a problem hiding this comment.
🟡 Changes recommended
The updated CODE_SPAN regex also blocks legitimate code spans that end with a backslash before the closing backtick, which can reintroduce quoted-marker misparsing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The change is small, targeted, and backed by regression tests; only minor test-strictness nits were identified.
Review details
Suppressed comments (1)
scripts/tests/test_pr_review.py:1727
- Use assertEqual here so this test stays strict and detects accidental extra unrecognized markers in addition to verifying that the summary is not masked.
self.assertIn("summary: Hidden", pr_review.unrecognized_in(body))
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
Copilot on #1180, both findings taken. assertIn only proved the expected marker was present, so an extra one would pass unnoticed. Switched to assertEqual, as the nearby tests do, and it immediately earned its keep: two of the three expected lists were wrong. They named a `### Pull request overview` heading that OVERVIEW does not carry and that would be vetted anyway, so the tests were passing on a mental model of the output rather than the output. Added the case Copilot asked for, a span that both carries a marker and ends in a backslash. It is the other side of the close guard: the earlier test proves a valid close is recognized, this one proves recognizing it keeps the quoted marker masked. Each guard is now independently covered. Reverting the closer guard fails two tests, reverting the opener guard fails one.
|
Answering the suppressed finding on head
Applied in Recording what it caught, since the finding was better than a style note. Two of the three expected lists were wrong: they named a |
There was a problem hiding this comment.
🟢 Approval recommended
The regex change is narrowly scoped, aligns with the stated failure mode, and is covered by targeted regression tests for both the opener and closer edge cases.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Qodo: the new test docstrings narrate the parser's former failure, which couples them to an implementation that may be rewritten without the narration following. The staleness point is right, and it is separable from the convention. This file's docstrings say why a case exists rather than only what it asserts, and that stays. What changes is the tense: each now states what happens when the guard is absent, in the present, rather than what happened before it was added. The reader still learns why the case is there, and nothing goes stale if the regex is rewritten.
|
Revisiting the docstring finding, which I first answered as a decline. Acted on in
The staleness half is right. The docstrings were written in the past tense, describing what the parser did before each guard existed. That narration goes stale if the regex is rewritten and nobody updates the prose, and a stale explanation is worse than none because it is still believed. The convention half I still decline. This file's docstrings say why a case exists rather than only what it asserts, for example "A vetting list that stops reporting is indistinguishable from a body with nothing to report, which is the failure the list exists to make loud." Stripping these three to stated behaviour alone would make them the outliers, and that is a The two are separable, which I missed the first time. Each docstring now states, in the present, what happens when its guard is absent: The reader still learns why the case exists, and nothing goes stale if the implementation changes, since the sentence describes the behaviour the test pins rather than a past bug. Worth recording that keeping the failure legible is load-bearing here rather than decorative: it is what let a reviewer of this branch check whether each new test actually fails without its fix, which caught two that did not and two whose expected values were invented. |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, matches the stated defect, and is protected by targeted regression tests for both the opener and closer edge cases.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Answers a Qodo finding on the
develop -> mainpromotion PR #1179, against the code-span masking that landed in #1157.The Defect
CODE_SPANread a backslash-escaped backtick as an opening run, so it paired with the next real tick and masked everything between them.That is the silent direction, which is what makes it worth a fix cycle rather than a decline. A body carrying an escaped tick before a real section loses that section:
The section is real in both. Only the escaped tick hides it, and a hidden unknown section lets the digest close a loop on a body nobody read.
The Fix
Both runs now exclude a tick preceded by a backslash. The guard reads a single backslash, so an escaped backslash under-masks rather than over-masks. That is deliberate: a quoted tag reported as a section refuses loudly and costs a decline, where a hidden section costs nothing visible and is wrong.
Verified unchanged by the fix: a quoted
<summary>in prose is still masked, the joined-token case still holds, the paragraph bound still holds, and a multi-line span inside one paragraph is still masked.A regression test covers the escaped tick and fails with the guard reverted.
Also Here
Qodo separately called the
CODE_SPANcomment verbose at fifteen lines. Trimmed to four. Worth noting for the next reader: the prose gate requires one sentence per comment line, so the lever is fewer sentences rather than tighter wrapping.Declined on #1179, Recorded Here
Two Qodo findings on that PR are not acted on, with evidence in its own threads:
CODECOV_TOKENbe mapped into this repository's own validation callers. This repo is[source-only, docs]withrequiredSecrets: []and claims no codecov mechanism, so the mapping would register a secretspec/audit.pythen reports as stale. The real gap is downstream and is filed as WORKFLOW.md Section 4 Contract Claims Falsified by the Hub-Hosted Validator and Release Task #1152.Summary by CodeRabbit
Bug Fixes
Tests