Skip to content

Trim implementation-detail docstrings Qodo flagged on PR #1068#1069

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.

Sign up for GitHub

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

Merged
ptr727 merged 4 commits into from
Aug 29, 2026
+27 −38
Merged

Trim implementation-detail docstrings Qodo flagged on PR #1068 #1069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9cd8e8e
Dispose of Qodo findings on the promotion PR (#1068)
ptr727 Aug 28, 2026
d0ceaca
Trim marker_blocks to bare contract, split an over-25-word test sentence
ptr727 Aug 28, 2026
6bfc8cd
Split an over-25-word inline comment into two sentences
ptr727 Aug 28, 2026
1501336
Move ten multi-line docstrings' closing quotes onto their own line
ptr727 Aug 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 5 additions & 23 deletions scripts/pr_review.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1485,28 +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.

The section has worn three shapes so far, all against `SUPPRESSED`: its own `<details>`
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.
Shared by `suppressed_blocks` and `outside_diff_blocks`. `strip_blockquote` affects heading
detection only, never the returned content.
"""
if not body:
return []
Expand All@@ -1517,6 +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.
# 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)):
Expand All@@ -1528,7 +1510,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)

Expand Down
37 changes: 22 additions & 15 deletions scripts/tests/test_pr_review.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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)
Expand DownExpand Up@@ -950,8 +951,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"},
Expand DownExpand Up@@ -1003,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)
Expand All@@ -1013,7 +1013,8 @@ def test_every_finding_in_a_multi_finding_section_is_captured(self) -> None:
"""The shape a lazy `<details>` 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)
Expand All@@ -1025,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)
Expand DownExpand Up@@ -1055,8 +1057,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(
Expand DownExpand Up@@ -1120,8 +1120,10 @@ 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, 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)
Expand All@@ -1132,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)
Expand All@@ -1147,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 = "<h3>PR Summary by Qodo</h3>\n\nAdds a thing.\n"
full = [comment(login="ptr727") for _ in range(pr_review.WINDOW - 1)] + [
comment(login="qodo-code-review", body=summary)
Expand All@@ -1163,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 = "<h3>PR Summary by Qodo</h3>\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)
Expand All@@ -1178,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 `<code>Resolved</code>` 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="<code>Resolved</code> flag ignored on retry")
self.answer(payload([review()], comments=[comment(login="qodo-code-review", body=body)]))
out, _ = pr_review.digest("o", "r", 7)
Expand DownExpand Up@@ -3784,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)
Expand Down