From 69fd5fa8361a2244b8da99190ddc5174b3a62aee Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 12:04:13 -0700 Subject: [PATCH 1/2] Name the Carried Cause in the COVERAGE_IS_PARTIAL Status Line On the carry paths no round covering the head states partial coverage, so the line no longer claims the head's review read fewer files. It leads with the reason the head reads as partial instead. Closes #3424 Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 12 ++++++++++-- tests/test_pr_review.py | 14 +++++++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index fc182598..09c6f0ff 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -994,9 +994,17 @@ def report_verdict(pr: dict) -> int: return 0 state, why = coverage(pr) if state == "partial": + if any(reading(n).coverage == "partial" for n in head_reviews(pr)): + lead = ( + "the review covering the head states it read fewer files than the pull request " + f"changed ({why}), so part of the diff has no review at all" + ) + else: + lead = ( + f"the head reads as partial because {why}, so part of the diff may have no review" + ) print( - f"status=COVERAGE_IS_PARTIAL the review covering the head states it read fewer files " - f"than the pull request changed ({why}), so part of the diff has no review at all. A " + f"status=COVERAGE_IS_PARTIAL {lead}. A " "re-request is not the remedy it reads as. Splitting is the remedy where it applies, " "and it does not apply to a promotion. Otherwise this is the maintainer's call" ) diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index 7f96954b..c597896b 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1989,12 +1989,24 @@ def test_a_head_round_stating_partial_coverage_exits_forty_two(self) -> None: self.assertEqual(42, self.status(payload([review(body=part)], files=["a.py"]))) out = self.out.getvalue() self.assertIn("coverage=PARTIAL ", out) - self.assertIn("status=COVERAGE_IS_PARTIAL", out) + self.assertIn("status=COVERAGE_IS_PARTIAL the review covering the head states it", out) def test_an_earlier_partial_carries_to_a_head_stating_none(self) -> None: part = f"{OVERVIEW}\n" rounds = [review(oid=OLD, body=part, rid="A"), review(body=OVERVIEW, rid="B")] self.assertEqual(42, self.status(payload(rounds, files=["a.py"]))) + line = next(ln for ln in self.out.getvalue().splitlines() if ln.startswith("status=")) + self.assertIn("because an earlier round states partial coverage", line) + self.assertNotIn("the review covering the head states", line) + + def test_an_earlier_partial_carried_past_an_unbelievable_head_says_so(self) -> None: + part = f"{OVERVIEW}\n" + odd = f"{OVERVIEW}\n" + rounds = [review(oid=OLD, body=part, rid="A"), review(body=odd, rid="B")] + self.assertEqual(42, self.status(payload(rounds, files=["a.py"]))) + line = next(ln for ln in self.out.getvalue().splitlines() if ln.startswith("status=")) + self.assertIn("a round on the head states a reading this cannot believe", line) + self.assertNotIn("the review covering the head states", line) def test_a_prose_partial_on_an_earlier_round_keeps_a_table_from_standing_in(self) -> None: part = f"{OVERVIEW}\nCopilot reviewed 1 out of 2 changed files.\n" From 81ca8d885568f56bf5ca8f2b350955db53af09e3 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 12:14:37 -0700 Subject: [PATCH 2/2] Key the Partial Status Line on coverage()'s Own Carry Reason The status line now tests the reason coverage() built rather than re-deriving which path it took, and the carry path names the remedy that clears it: a round on the head stating full coverage. Co-Authored-By: Claude Opus 5.5 --- scripts/pr_review.py | 24 +++++++++++++----------- scripts/pr_review_rounds.py | 3 ++- tests/test_pr_review.py | 3 +++ 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 09c6f0ff..3c056035 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -189,6 +189,7 @@ ) from pr_review_rounds import ( ACK_MARKER, + CARRIED_PARTIAL, CODERABBIT_LOGIN, FILES_UNREAD, KNOWN_REVIEWERS, @@ -994,20 +995,21 @@ def report_verdict(pr: dict) -> int: return 0 state, why = coverage(pr) if state == "partial": - if any(reading(n).coverage == "partial" for n in head_reviews(pr)): - lead = ( - "the review covering the head states it read fewer files than the pull request " - f"changed ({why}), so part of the diff has no review at all" + if why.startswith(CARRIED_PARTIAL): + print( + f"status=COVERAGE_IS_PARTIAL the head reads as partial because {why}, so part of " + "the diff may have no review. A round on the head stating full coverage clears " + "this, unless a round on it already states a reading this cannot believe. " + "Otherwise this is the maintainer's call" ) else: - lead = ( - f"the head reads as partial because {why}, so part of the diff may have no review" + print( + f"status=COVERAGE_IS_PARTIAL the review covering the head states it read fewer " + f"files than the pull request changed ({why}), so part of the diff has no review " + "at all. A re-request is not the remedy it reads as. Splitting is the remedy where " + "it applies, and it does not apply to a promotion. Otherwise this is the " + "maintainer's call" ) - print( - f"status=COVERAGE_IS_PARTIAL {lead}. A " - "re-request is not the remedy it reads as. Splitting is the remedy where it applies, " - "and it does not apply to a promotion. Otherwise this is the maintainer's call" - ) return 42 if flagged := uncounted_verdict(pr): print( diff --git a/scripts/pr_review_rounds.py b/scripts/pr_review_rounds.py index 8788670e..6275758c 100644 --- a/scripts/pr_review_rounds.py +++ b/scripts/pr_review_rounds.py @@ -31,6 +31,7 @@ ) TRUSTED_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) ACK_MARKER = "" +CARRIED_PARTIAL = "an earlier round states partial coverage" def reviewer_requested(pr: dict) -> bool: @@ -437,7 +438,7 @@ def coverage(pr: dict) -> tuple[str, str]: if worst else "no round on the head states any" ) - return "partial", f"an earlier round states partial coverage, and {none}" + return "partial", f"{CARRIED_PARTIAL}, and {none}" if worst: return worst why = "" diff --git a/tests/test_pr_review.py b/tests/test_pr_review.py index c597896b..a31a9cdb 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1990,6 +1990,7 @@ def test_a_head_round_stating_partial_coverage_exits_forty_two(self) -> None: out = self.out.getvalue() self.assertIn("coverage=PARTIAL ", out) self.assertIn("status=COVERAGE_IS_PARTIAL the review covering the head states it", out) + self.assertNotIn("may have no review", out) def test_an_earlier_partial_carries_to_a_head_stating_none(self) -> None: part = f"{OVERVIEW}\n" @@ -1998,6 +1999,8 @@ def test_an_earlier_partial_carries_to_a_head_stating_none(self) -> None: line = next(ln for ln in self.out.getvalue().splitlines() if ln.startswith("status=")) self.assertIn("because an earlier round states partial coverage", line) self.assertNotIn("the review covering the head states", line) + self.assertIn("A round on the head stating full coverage clears this", line) + self.assertNotIn("A re-request is not the remedy", line) def test_an_earlier_partial_carried_past_an_unbelievable_head_says_so(self) -> None: part = f"{OVERVIEW}\n"