From f36e1352d93ce25a562dc1cd5840c078fa6da37f Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 17:22:11 -0700 Subject: [PATCH 1/3] Fix two docstring/comment inaccuracies Copilot caught, trim two more comment blocks - The status docstring's cr_outside_diff paragraph claimed it prints only once CodeRabbit has raised a finding, but digest() also prints it (as cr_outside_diff=0+) once the reviews window is truncated, regardless of what is visible. Reworded to state both conditions. - TestOtherReviewers' docstring claimed 'identity and commit only', but its own rate-limit-marker tests read comment/review body content (the structural marker, not free-text prose). Reworded to name what it actually covers and point elsewhere for prose parsing, rather than contradict its own tests. - Local-strict-review (dispatched before this push) caught three follow-on issues in that same rewrite: a garden-path modifier ('trialing it on an untruncated window'), a first-pass reading of the TestOtherReviewers reword that said the opposite of what it meant, and QODO_REVIEW_HEADING/QODO_FINDING/ QODO_BADGE's comment trim (Qodo's other finding on this round) having dropped load-bearing rationale for two of the three. All reworded/restored. QODO_BADGE's own comment carries two constraints now, both load-bearing (confirmed by this exact regex's two prior real-bug fixes in this task): the glyph requirement, and why it is escaped rather than typed literally. --- scripts/pr_review.py | 22 +++++++++------------- scripts/tests/test_pr_review.py | 9 +++++---- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 20687ab7..2752592a 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -66,8 +66,11 @@ poll alone cannot see. `cr_outside_diff=N (on_head=X earlier=Y)` counts CodeRabbit's own "outside diff range" findings, collapsed into the review body rather than raised as an inline review comment because the finding sits on a line outside - the pull request's changed hunks. Printed only once CodeRabbit has raised one, on any - round, so a repository not trialing it stays silent. `qodo_open=N` counts Qodo's own + the pull request's changed hunks. Printed once CodeRabbit has raised one, on any + round, or once the reviews window is truncated, since an older round could then + still carry one unseen (`cr_outside_diff=0+`). Stays silent otherwise, on an + untruncated window where a repository is not trialing CodeRabbit at all. + `qodo_open=N` counts Qodo's own numbered findings that carry neither its `Resolved` nor `Dismissed` self-tracked badge: Qodo's formal review carries an empty body on every round observed, so its findings are read from its "Code Review by Qodo" PR-level comment instead, not @@ -216,19 +219,12 @@ # The service name is captured rather than assumed. # A second bot using the same auto-generated-comment convention is read without a new pattern, and one that does not use it stays unread rather than guessed at. RATE_LIMITED = re.compile(r"auto-generated comment:\s*rate limited by\s*(\S+?)\s*-->") -# Qodo's formal review object carries an empty body on every review checked, confirmed across roughly 60. -# Its findings ride a PR-level comment instead. -# Two are posted per round, "PR Summary by Qodo" (an overview, no findings) and this one, told apart by its own heading. -# Anchored to the `

` tag the heading itself wears, not a bare substring: the summary comment's own prose can mention the phrase without being the comment it names. +# Anchored to the `

` tag rather than a bare substring, since prose elsewhere can mention the phrase without being the comment it names. QODO_REVIEW_HEADING = re.compile(r"

\s*Code Review by Qodo\s*

", re.IGNORECASE) -# Qodo nests a Description/Code/Relevance/Evidence/Agent-prompt `` under each of its own numbered findings. -# Only the numbered heading itself is a finding, told apart from those by starting with `N.` the way none of the nested ones do. +# Only the numbered heading counts as a finding, told apart from Qodo's own nested sub-summaries by starting with `N.` QODO_FINDING = re.compile(r"\s*\d+\.\s") -# Qodo's own self-tracked disposition, re-checked against the current head on its own schedule. -# Present on a finding it has re-verified as addressed or intentionally dismissed, absent on one still open. -# A fast pre-triage signal per the runbook, not a substitute for reading the finding: spot-verify against `gh pr diff` rather than trusting it outright. -# The glyph is required rather than left unanchored, since a bare word match reads any finding whose own title quotes the identifier `Resolved` or `Dismissed` in its own `` tag as carrying the badge, closing an open finding on the strength of its own title. -# Escaped rather than typed literally (check mark U+2713, ballot x U+2717), keeping this source inside the ASCII charset rule that governs the repository. +# A finding's own title can quote `Resolved`/`Dismissed` without carrying the badge, so the glyph is required rather than just the word. +# Escaped (U+2713, U+2717) rather than typed literally, per the repository's ASCII charset rule. QODO_BADGE = re.compile(r"[^<]*(?:\u2713 Resolved|\u2717 Dismissed)[^<]*") # A round states how much of the diff it read on a line of its own. # A round that read part of it is the clean pass elsewhere, same commit and threads and digest. diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index ae6137be..6e38fad5 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -585,15 +585,16 @@ def test_a_body_is_flattened_and_bounded(self) -> None: class TestOtherReviewers(GqlCase): - """Thread resolution and head-presence, generalized past Copilot to the other review bots - this repository has trialed: identity and commit only, no prose parsed for either one here. + """Thread resolution, head-presence, and the rate-limit marker's own structural pattern, + generalized past Copilot to the other review bots this repository has trialed. Free-text + prose parsing for CodeRabbit's outside-diff findings and Qodo's comment-only findings lives + in `TestCodeRabbitOutsideDiff` and `TestQodoOpenFindings` below, not here. `status`'s `unresolved=0` used to hide a CodeRabbit/qodo thread that still blocked a ruleset-gated merge (PR #915, ptr727/ProjectTemplate), since only Copilot's own threads counted. Coverage and refusal reading stay Copilot-only: `review_on_head` above names Copilot's own coverage specifically, the reviewer this script requests and waits for, not - "no review of any kind covers this head" (#1066). CodeRabbit's outside-diff-range findings - and Qodo's comment-only findings are each read too, in their own shape, by `TestCodeRabbitOutsideDiff` and `TestQodoOpenFindings` below (#1058). + "no review of any kind covers this head" (#1066). """ def other_review(self, login: str, oid: str = HEAD, body: str = "") -> dict: From d2edf623c8a177be4bd5f1c11cb3feeec17c764e Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 17:26:50 -0700 Subject: [PATCH 2/3] Fix two more precision gaps Copilot caught in the same docstrings - cr_outside_diff's 'stays silent' sentence implied silence means CodeRabbit is not trialed, but it is equally silent whenever CodeRabbit is trialed and simply has not raised a finding yet, the common case. Reworded to name both. - other_review()'s pre-existing docstring said 'identity and commit only', but the helper takes and returns body content (used by the rate-limit-marker tests in this same class). This helper predates this task's own changes, but Copilot flagged it against the class docstring this PR's own prior commit touched, so fixed rather than declined on a technicality. --- scripts/pr_review.py | 4 ++-- scripts/tests/test_pr_review.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 2752592a..72d65f51 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -68,8 +68,8 @@ than raised as an inline review comment because the finding sits on a line outside the pull request's changed hunks. Printed once CodeRabbit has raised one, on any round, or once the reviews window is truncated, since an older round could then - still carry one unseen (`cr_outside_diff=0+`). Stays silent otherwise, on an - untruncated window where a repository is not trialing CodeRabbit at all. + still carry one unseen (`cr_outside_diff=0+`). Stays silent otherwise, whether that + silence means no finding was raised or CodeRabbit was never trialed at all. `qodo_open=N` counts Qodo's own numbered findings that carry neither its `Resolved` nor `Dismissed` self-tracked badge: Qodo's formal review carries an empty body on every round observed, so its diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index 6e38fad5..6df4ed4c 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -598,7 +598,7 @@ class TestOtherReviewers(GqlCase): """ def other_review(self, login: str, oid: str = HEAD, body: str = "") -> dict: - """A minimal review node for a tracked non-Copilot reviewer: identity and commit only.""" + """A review node for a tracked non-Copilot reviewer, `body` empty unless a case needs it.""" return { "author": {"login": login}, "state": "COMMENTED", From ef4048e18fb90633a0b193f17d57d599afa1c0dd Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 17:37:52 -0700 Subject: [PATCH 3/3] Reword QODO_FINDING's comment to avoid the letter-N misreading Copilot's suppressed finding, on head: 'N.' as a stand-in for a numbered heading can read as the literal letter N rather than a digit, since the regex is \d+\. Spelled out with an example instead. --- scripts/pr_review.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 72d65f51..489abdbb 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -221,7 +221,7 @@ RATE_LIMITED = re.compile(r"auto-generated comment:\s*rate limited by\s*(\S+?)\s*-->") # Anchored to the `

` tag rather than a bare substring, since prose elsewhere can mention the phrase without being the comment it names. QODO_REVIEW_HEADING = re.compile(r"

\s*Code Review by Qodo\s*

", re.IGNORECASE) -# Only the numbered heading counts as a finding, told apart from Qodo's own nested sub-summaries by starting with `N.` +# Only the numbered heading counts as a finding, told apart from Qodo's own nested sub-summaries by starting with a number and a period (`1.`, `2.`, ...). QODO_FINDING = re.compile(r"\s*\d+\.\s") # A finding's own title can quote `Resolved`/`Dismissed` without carrying the badge, so the glyph is required rather than just the word. # Escaped (U+2713, U+2717) rather than typed literally, per the repository's ASCII charset rule.