From 9cd8e8e45912ab773725ac75a77cf07df3c3079a Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 16:06:33 -0700 Subject: [PATCH 1/4] Dispose of Qodo findings on the promotion PR (#1068) Three of Qodo's findings on PR #1068's re-review of the develop diff were real, about content #1067 introduced: - marker_blocks' docstring recorded PR #1067's own shape history and its line-scanning implementation rather than its return contract. Trimmed, the essential reasoning kept as a one-line inline comment. - TestCodeRabbitOutsideDiff and TestQodoOpenFindings still carried one-line class-summary docstrings, which comment-and-doc-style forbids regardless of length. Removed, matching most other test classes in this file. - A test docstring used historical 'previously read' change-framing instead of a present-tense invariant. Rewritten. local-strict-review against this diff (dispatched before this push) caught three more real issues in my own first pass at these same trims, now fixed too: - The rewritten badge test docstring claimed 'word-boundaried' matching, but QODO_BADGE requires the check-mark/cross glyph, not a word boundary. Corrected to name the actual mechanism. - A new inline comment gave an inverted reason for scanning line by line ('the heading can sit nested inside a summary'), when the actual reason is the opposite: the heading sits in a wrapper's body, not its summary, which is exactly why reading only the summary misses it. - suppressed_blocks' docstring still pointed readers to marker_blocks for 'the shape history behind it' after that history was trimmed out, a dangling reference this diff itself created. Removed the dangling clause. Two more of Qodo's findings, and both of CodeRabbit's findings on this same round, target .agents/skills/drive-pr and .agents/skills/local-strict-review, carried content from #1065 rather than #1067. Left for separate disposition: CodeRabbit's two are declined in the promotion PR thread (local-strict-review's own 'Disposing of Findings' section already requires disposition before a PR opens, contrary to the finding's claim; making that a hard publish-blocking gate would reverse a deliberate, documented design choice), and Qodo's AGENTS.md-duplication finding is deferred to a filed issue rather than redesigned mid-review. --- scripts/pr_review.py | 29 +++++++---------------------- scripts/tests/test_pr_review.py | 9 +++------ 2 files changed, 10 insertions(+), 28 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index efbdb5d7..783a8a2e 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -1486,27 +1486,11 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F that heading through to the end of its own region. Shared by `suppressed_blocks` (Copilot's low-confidence findings) and `outside_diff_blocks` - (CodeRabbit's outside-diff-range findings): both are a review body collapsing real - findings into a block that raises no `reviewThreads` entry, so a thread poll alone reports a - clean pass over either. - - The section has worn three shapes so far, all against `SUPPRESSED`: its own `
` - wrapper, a bare heading in the body, and a Markdown heading nested inside the `Review - details` wrapper. The wrapper is the part that keeps moving, so each region is scanned line - by line for the heading rather than read for a wrapper's summary: a nested heading is not a - summary, and stripping the wrappers to look for it outside deletes the very region it sits - in. That is the pair that reported `suppressed=0` over a body carrying `### Suppressed - comments (2)`. - - The heading carries the match rather than the body text, since a review whose prose discusses - the marker's own wording is not itself carrying a block. A region ends the block, so a - section is not read on into the file table that follows it. - - `strip_blockquote` is read on a copy used only to find the heading line, never on what is - returned: CodeRabbit's own outside-diff section wraps its lines in a Markdown blockquote, - which `BLOCKQUOTE` strips for detection, but a finding quoting code (`>&2 echo`, `>> $LOG`) - would otherwise be corrupted by that same strip once it landed in the printed digest. - Left off by default, so `suppressed_blocks` reads exactly as it always has. + (CodeRabbit's outside-diff-range findings): both are a review body collapsing real findings + into a block that raises no `reviewThreads` entry, so a thread poll alone reports a clean + pass over either. + + `strip_blockquote` affects heading detection only, never the returned content. """ if not body: return [] @@ -1517,6 +1501,7 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F blocks = [] for region in regions: raw_lines = region.splitlines() + # Scanned line by line rather than read from a wrapper's own summary, since the heading can sit in the wrapper's body instead of its summary, where reading only the summary would miss it entirely. scan_lines = [BLOCKQUOTE.sub("", ln) for ln in raw_lines] if strip_blockquote else raw_lines for i, line in enumerate(scan_lines): if marker.search(line) and (HEADING.match(line) or COUNT.search(line)): @@ -1528,7 +1513,7 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F def suppressed_blocks(body: str) -> list[str]: """Copilot's own low-confidence findings, collapsed rather than raised as inline threads. - See `marker_blocks` for the shared reading and the shape history behind it. + See `marker_blocks` for the shared reading. """ return marker_blocks(body, SUPPRESSED) diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index a8728261..83941d4e 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -950,8 +950,6 @@ def cr_outside_diff_body_multi(findings: list[str], path: str = "a.py") -> str: class TestCodeRabbitOutsideDiff(GqlCase): - """CodeRabbit's own equivalent of the blind spot `TestSuppressed` covers for Copilot.""" - def cr_review(self, oid: str = HEAD, body: str = "") -> dict: return { "author": {"login": "coderabbitai"}, @@ -1055,8 +1053,6 @@ def qodo_review_body(resolved: bool = False, heading: str = "Bad naming") -> str class TestQodoOpenFindings(GqlCase): - """Qodo's own comment-only findings, its formal review carrying an empty body on every round.""" - def test_an_open_finding_counts_and_prints_without_its_nested_subsections(self) -> None: self.answer( payload( @@ -1120,8 +1116,9 @@ def test_the_newest_findings_comment_wins_on_a_re_reviewed_pull_request(self) -> self.assertIn("qodo_open=0", out) def test_a_finding_titled_about_the_badge_word_itself_is_not_read_as_carrying_it(self) -> None: - """An unanchored match previously read this script's own `isResolved` identifier, quoted - in a finding's title, as the badge, closing an open finding on its own title.""" + """The required glyph, not the bare word, keeps this script's own `isResolved` + identifier, quoted in a finding's title, from being read as the badge and closing an + open finding on its title.""" body = qodo_review_body(heading="isResolved handling is inconsistent") self.answer(payload([review()], comments=[comment(login="qodo-code-review", body=body)])) out, _ = pr_review.digest("o", "r", 7) From d0ceacafbf019167b2b68698cbe03c8b24fbeedb Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 16:10:05 -0700 Subject: [PATCH 2/4] Trim marker_blocks to bare contract, split an over-25-word test sentence Qodo's round-2 findings on PR #1069 itself: - marker_blocks' docstring still explained the reviewThreads blind spot after two prior trims. Removed the rationale sentence entirely; suppressed_blocks and outside_diff_blocks each already carry their own copy of it, so nothing is lost. - The rewritten badge test's invariant sentence ran 29 (Qodo's count) to 32 (mine) words, over comment-and-doc-style's 25-word cap (ASD-STE100, prose_lint.py --check sentence-length). Split into two sentences under the cap. --- scripts/pr_review.py | 8 ++------ scripts/tests/test_pr_review.py | 6 +++--- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 783a8a2e..4cb5a7a9 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -1485,12 +1485,8 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F """Return the review body's sections whose own heading matches `marker`, each sliced from that heading through to the end of its own region. - Shared by `suppressed_blocks` (Copilot's low-confidence findings) and `outside_diff_blocks` - (CodeRabbit's outside-diff-range findings): both are a review body collapsing real findings - into a block that raises no `reviewThreads` entry, so a thread poll alone reports a clean - pass over either. - - `strip_blockquote` affects heading detection only, never the returned content. + Shared by `suppressed_blocks` and `outside_diff_blocks`. `strip_blockquote` affects heading + detection only, never the returned content. """ if not body: return [] diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index 83941d4e..7113f6f4 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -1116,9 +1116,9 @@ def test_the_newest_findings_comment_wins_on_a_re_reviewed_pull_request(self) -> self.assertIn("qodo_open=0", out) def test_a_finding_titled_about_the_badge_word_itself_is_not_read_as_carrying_it(self) -> None: - """The required glyph, not the bare word, keeps this script's own `isResolved` - identifier, quoted in a finding's title, from being read as the badge and closing an - open finding on its title.""" + """The required glyph, not the bare word, decides. This script's own `isResolved` + identifier, quoted in a finding's title, is not the badge and does not close an open + finding.""" body = qodo_review_body(heading="isResolved handling is inconsistent") self.answer(payload([review()], comments=[comment(login="qodo-code-review", body=body)])) out, _ = pr_review.digest("o", "r", 7) From 6bfc8cd950ce02d62aaf88b0a8eb43acdf50b87d Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 16:11:24 -0700 Subject: [PATCH 3/4] Split an over-25-word inline comment into two sentences Qodo caught this in the same round as the marker_blocks/prose-cap findings already fixed in d0ceaca, but on a comment that commit did not touch: 35 words on one line, over the 25-word cap. Split into two short sentences, same information, one per line per the file's own comment-wrap convention. --- scripts/pr_review.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 4cb5a7a9..20687ab7 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -1497,7 +1497,8 @@ def marker_blocks(body: str, marker: re.Pattern[str], strip_blockquote: bool = F blocks = [] for region in regions: raw_lines = region.splitlines() - # Scanned line by line rather than read from a wrapper's own summary, since the heading can sit in the wrapper's body instead of its summary, where reading only the summary would miss it entirely. + # Scanned line by line rather than read from a wrapper's own summary. + # A heading can sit in the wrapper's body instead, where reading only the summary would miss it. scan_lines = [BLOCKQUOTE.sub("", ln) for ln in raw_lines] if strip_blockquote else raw_lines for i, line in enumerate(scan_lines): if marker.search(line) and (HEADING.match(line) or COUNT.search(line)): From 1501336cc55c39354b1aedb56dbc2c39434956a0 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 16:21:50 -0700 Subject: [PATCH 4/4] Move ten multi-line docstrings' closing quotes onto their own line Copilot's finding on one instance (test_pr_review.py, the isResolved-badge test docstring rewritten twice already this round) is real and matches the established convention every pre-existing multi-line docstring in this file already follows: closing """ on its own line, text never trailing on the same line as the last sentence. Swept the same fix across nine more instances this task's own earlier commits introduced with the same defect, rather than fixing only the one flagged. --- scripts/tests/test_pr_review.py | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index 7113f6f4..ae6137be 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -879,7 +879,8 @@ def test_a_human_review_carrying_the_phrase_is_not_a_copilot_finding(self) -> No def test_a_truncated_reviews_window_marks_suppressed_as_undercounting(self) -> None: """An older round old enough to fall out of the 100-review window can carry a finding of its own that `suppressed=` then has no way to read, the same blind spot `threads=` - already carries its own `+` marker for.""" + already carries its own `+` marker for. + """ self.answer(payload([review()], older_reviews=True)) out, _ = pr_review.digest("o", "r", 7) self.assertIn("suppressed=0+", out) @@ -1001,7 +1002,8 @@ def test_a_copilot_review_carrying_the_phrase_is_not_a_coderabbit_finding(self) def test_a_truncated_reviews_window_surfaces_cr_outside_diff_even_at_zero(self) -> None: """Silence on `cr_outside_diff` reads as "nothing to triage", so an older CodeRabbit round old enough to fall out of the window must not stay silent just because none of - the rounds still in view happen to carry a finding of their own.""" + the rounds still in view happen to carry a finding of their own. + """ self.answer(payload([review()], older_reviews=True)) out, _ = pr_review.digest("o", "r", 7) self.assertIn("cr_outside_diff=0+", out) @@ -1011,7 +1013,8 @@ def test_every_finding_in_a_multi_finding_section_is_captured(self) -> None: """The shape a lazy `
` pairing loses: PR #1053's own round nests a per-finding "Prompt for AI Agents" block, and a second finding sitting after that nested block's own close used to fall outside the captured region entirely, `cr_outside_diff=2` printing - only the first.""" + only the first. + """ body = cr_outside_diff_body_multi(["First off-by-one.", "Second off-by-one."]) self.answer(payload([review(), self.cr_review(body=body)])) out, _ = pr_review.digest("o", "r", 7) @@ -1023,7 +1026,8 @@ def test_a_finding_quoting_shell_redirects_is_not_corrupted_by_the_blockquote_st self, ) -> None: """`BLOCKQUOTE` is read on a copy for detection only, never on the returned block: a - naive strip once turned `>&2 echo` into `&2 echo` in the printed finding.""" + naive strip once turned `>&2 echo` into `&2 echo` in the printed finding. + """ body = cr_outside_diff_body(finding="Missing >&2 echo failed on error.") self.answer(payload([review(), self.cr_review(body=body)])) out, _ = pr_review.digest("o", "r", 7) @@ -1118,7 +1122,8 @@ def test_the_newest_findings_comment_wins_on_a_re_reviewed_pull_request(self) -> def test_a_finding_titled_about_the_badge_word_itself_is_not_read_as_carrying_it(self) -> None: """The required glyph, not the bare word, decides. This script's own `isResolved` identifier, quoted in a finding's title, is not the badge and does not close an open - finding.""" + finding. + """ body = qodo_review_body(heading="isResolved handling is inconsistent") self.answer(payload([review()], comments=[comment(login="qodo-code-review", body=body)])) out, _ = pr_review.digest("o", "r", 7) @@ -1129,7 +1134,8 @@ def test_qodo_open_is_unknown_rather_than_absent_where_its_comment_can_be_behind self, ) -> None: """`qodo_open` absent means "never commented", and a window with no Qodo comment in view - cannot tell that from Qodo's own comment simply sitting behind it.""" + cannot tell that from Qodo's own comment simply sitting behind it. + """ full = [comment(login="ptr727") for _ in range(pr_review.WINDOW)] self.answer(payload([review()], comments=full, older=True)) out, _ = pr_review.digest("o", "r", 7) @@ -1144,7 +1150,8 @@ def test_qodo_open_is_absent_rather_than_unknown_once_the_window_is_not_blind(se def test_qodo_open_is_unknown_where_only_the_paired_summary_comment_is_in_view(self) -> None: """A visible `PR Summary by Qodo` clears plain `window_blind`, which settles for any of Qodo's own comments, but says nothing about the findings comment specifically: an older - `Code Review by Qodo` can still be the one that fell out of the window.""" + `Code Review by Qodo` can still be the one that fell out of the window. + """ summary = "

PR Summary by Qodo

\n\nAdds a thing.\n" full = [comment(login="ptr727") for _ in range(pr_review.WINDOW - 1)] + [ comment(login="qodo-code-review", body=summary) @@ -1160,7 +1167,8 @@ def test_a_summary_comment_mentioning_the_findings_heading_in_prose_is_not_selec bare substring: a `PR Summary by Qodo` comment whose own prose happens to mention "Code Review by Qodo" must not be mistaken for the findings comment itself, which would read a genuinely hidden findings comment as `qodo_open=0` rather than - `unknown`.""" + `unknown`. + """ summary = "

PR Summary by Qodo

\n\nA Code Review by Qodo will follow shortly.\n" full = [comment(login="ptr727") for _ in range(pr_review.WINDOW - 1)] + [ comment(login="qodo-code-review", body=summary) @@ -1175,7 +1183,8 @@ def test_a_finding_titled_with_the_bare_badge_word_and_no_glyph_is_not_read_as_t ) -> None: """`QODO_BADGE` requires the check mark or cross Qodo's own badge carries, not just the word: a finding's own title quoting `Resolved` with no glyph is not Qodo's - badge, only something adjacent enough to be mistaken for it on the word alone.""" + badge, only something adjacent enough to be mistaken for it on the word alone. + """ body = qodo_review_body(heading="Resolved flag ignored on retry") self.answer(payload([review()], comments=[comment(login="qodo-code-review", body=body)])) out, _ = pr_review.digest("o", "r", 7) @@ -3781,7 +3790,8 @@ def test_status_documents_its_no_review_success_case(self) -> None: def test_status_documents_review_on_head_as_copilot_scoped(self) -> None: """`review_on_head=NO` alongside a genuine `other_reviewed` head is a scoping fact, not - a coverage gap, the exact confusion that reached this docstring as a filed issue.""" + a coverage gap, the exact confusion that reached this docstring as a filed issue. + """ doc = pr_review.__doc__ or "" self.assertIn('never "no review of any kind covers this head"', doc) self.assertIn("not a gap", doc)