diff --git a/scripts/pr_review.py b/scripts/pr_review.py index efbdb5d7..20687ab7 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -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 `
` - 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 [] @@ -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)): @@ -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) diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index a8728261..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) @@ -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"}, @@ -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) @@ -1013,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) @@ -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) @@ -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( @@ -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) @@ -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) @@ -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 = "

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) @@ -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 = "

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) @@ -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 `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) @@ -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)