From 5f5ea25d02f5c785212a3597b491d180a631ac32 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 12:27:53 -0700 Subject: [PATCH 1/3] Name the Carried Cause in the COVERAGE_IS_PARTIAL Status Line (#3479) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Answers #3424. `status=COVERAGE_IS_PARTIAL` said "the review covering the head states it read fewer files than the pull request changed" on the two carry paths in `coverage()` too. On those paths no round covering the head states partial coverage, and the bracketed reason said so. - `pr_review_rounds.py` now holds the carry reason's prefix as `CARRIED_PARTIAL`. `report_verdict` keys on it, so the line follows the reason `coverage()` gave rather than re-deriving which path it took. - On a carry path the line now leads with that reason and names the remedy that clears it: a round on the head stating full coverage, unless a round on it already states a reading this cannot believe. The direct path keeps its text and its remedy. - Tests: two carry-path cases (the head states nothing, and the head states `reviewed=4 changed=3`), each asserting the carry lead and remedy. The direct-path case now asserts its own lead and the absence of the carry wording. A mutation back to the old source fails both carry cases. ## Verification - Full suite: `uv run --no-project --with-requirements tests/requirements.txt python -m unittest discover -s tests`, 2948 tests run, OK (7 skipped). - Local strict review: two rounds. Round 1 raised two `introduced` findings, both fixed in 81ca8d88: the branch condition re-derived `coverage()`'s path, and the remedy tail contradicted the carry path. Round 2 raised none. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- scripts/pr_review.py | 22 ++++++++++++++++------ scripts/pr_review_rounds.py | 3 ++- tests/test_pr_review.py | 17 ++++++++++++++++- 3 files changed, 34 insertions(+), 8 deletions(-) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index fc182598..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,12 +995,21 @@ def report_verdict(pr: dict) -> int: return 0 state, why = coverage(pr) if state == "partial": - 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 " - "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" - ) + 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: + 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" + ) 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 7f96954b..a31a9cdb 100755 --- a/tests/test_pr_review.py +++ b/tests/test_pr_review.py @@ -1989,12 +1989,27 @@ 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) + self.assertNotIn("may have no review", 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) + 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" + 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 c0ff838274834c43ae19088f825d7bfd41318bec Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 12:51:58 -0700 Subject: [PATCH 2/3] Re-Read a Deleted Ref Before Reporting DELETE_NOT_CONFIRMED (#3480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Answers #3436. `cleanup_remote_delete` read the ref back once, right after `deleteRef`. GitHub's ref read can lag a delete that landed, so a landed delete could print `status=DELETE_NOT_CONFIRMED` and exit 2 before the local half ran. - A ref still at the deleted tip is now read again on `DELETE_SETTLE_DELAYS = (1, 2, 4)` through the existing `core.backoff` helper, so about seven seconds at most, before it is reported. Exit 2 still follows once that schedule is spent. - A read back at a tip other than the deleted one stops the loop and gets its own message ("a tip other than the one deleted, so something pushed to the branch"), since re-reading it settles nothing. - `scripts/README.md`'s `cleanup` paragraph says so. - Tests: a read lagging then gone settles to `DELETED` and exit 0. A ref present on every read exits 2 after exactly the scheduled reads. A settle read that does not answer exits 2. A new tip stops the loop and names that tip. The first two fail against the unfixed source. The existing suite patches the schedule to `()`, which reproduces the old single read. ## Known limits, not defects - A branch pushed back at the deleted tip inside the settle window cannot be told from lag. It ends `still reads at after the delete`, exit 2, which is the conservative outcome. - `scripts/handoff.py` reads state back once after a write in `create_label`, `close`, and `edit_issue`. No lag has been seen on label or issue reads, so these are the sites to name in one issue only if it is. ## Verification - Full suite: `uv run --no-project --with-requirements tests/requirements.txt python -m unittest discover -s tests`, 2952 tests run, OK (7 skipped), at 278e49ae. The commit after it changes message wording and a test name, and `test_pr_*.py` (628 tests) passes on it. - Local strict review: three passes. Round 1 raised three `introduced` findings (a README sentence over the length limit, no test for a settle read that does not answer, and the loop re-reading a ref at a new tip), all fixed in 278e49ae. Round 2 raised three (an overclaiming message, a stale docstring, a long line), fixed in 2c71411c. The third pass raised none. ## Pre-existing findings, not filed - `scripts/README.md:264`, `pre-existing`: five sentences this change did not edit run over the 25-word sentence-length check. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_cleanup.py | 20 +++++++++++++++++--- tests/test_pr_cleanup.py | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 4 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 638b9ae0..850f08f4 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -261,7 +261,7 @@ uv run --no-project --script scripts/pr_review.py cleanup 452 --repo ptr727/Proj uv run --no-project --script scripts/pr_review.py cleanup --sweep --repo ptr727/ProjectTemplate --checkout ~/repos/ProjectTemplate ``` -It deletes the head branch of a merged pull request, the verify-then-delete `drive-pr` and `merge-and-release` prescribe after a merge into `develop`. It is the one place holding the delete's checks, and no skill prescribes a hand-typed delete beside it, since the `gh-write-guard` hook denies `git push origin --delete` from a primary checkout and Claude Code's auto-mode classifier refuses the ref API delete on some days. A command matching a narrow `permissions.allow` rule is settled before that classifier runs, and the host-safety installer registers `Bash(uv run --no-project --script scripts/pr_review.py:*)`. That rule approves any arguments, so every check lives here. It takes the pull request number and `--repo`, never a branch name, and reads the branch from the pull request. It refuses unless the pull request is merged, its base is `develop`, its head repository is `--repo` itself, and the head branch is neither `develop`, `main`, nor the default branch read live. Past the owner check `pr_review.py` makes, `handoff.py`'s own write-scope check binds the owner to `registry/repos.json` and admits a repository only where it is registered or a fork. A branch any ruleset or branch protection covers is refused, and so is a branch another open pull request in `--repo` has as its head or base, each one named. The ref is read by its exact qualified name, and the delete is sent only where that read's tip is the pull request's `headRefOid`, by the node id the same read returned. The delete is confirmed by reading the ref back rather than by the mutation's own answer. Exit `0` is the branch deleted and confirmed gone, or already gone. Exit `1` is a refusal, a tip that moved after the merge among them. Exit `2` is a read that did not answer, the rule read included, a delete the read back could not confirm, or a usage error. Exit `64` is the owner check, run before the registry and before any read. +It deletes the head branch of a merged pull request, the verify-then-delete `drive-pr` and `merge-and-release` prescribe after a merge into `develop`. It is the one place holding the delete's checks, and no skill prescribes a hand-typed delete beside it, since the `gh-write-guard` hook denies `git push origin --delete` from a primary checkout and Claude Code's auto-mode classifier refuses the ref API delete on some days. A command matching a narrow `permissions.allow` rule is settled before that classifier runs, and the host-safety installer registers `Bash(uv run --no-project --script scripts/pr_review.py:*)`. That rule approves any arguments, so every check lives here. It takes the pull request number and `--repo`, never a branch name, and reads the branch from the pull request. It refuses unless the pull request is merged, its base is `develop`, its head repository is `--repo` itself, and the head branch is neither `develop`, `main`, nor the default branch read live. Past the owner check `pr_review.py` makes, `handoff.py`'s own write-scope check binds the owner to `registry/repos.json` and admits a repository only where it is registered or a fork. A branch any ruleset or branch protection covers is refused, and so is a branch another open pull request in `--repo` has as its head or base, each one named. The ref is read by its exact qualified name, and the delete is sent only where that read's tip is the pull request's `headRefOid`, by the node id the same read returned. The delete is confirmed by reading the ref back rather than by the mutation's own answer. That read can lag a landed delete, so a ref still at the deleted tip is read again for a few seconds first. Exit `0` is the branch deleted and confirmed gone, or already gone. Exit `1` is a refusal, a tip that moved after the merge among them. Exit `2` is a read that did not answer, the rule read included, a delete the read back could not confirm, or a usage error. Exit `64` is the owner check, run before the registry and before any read. With `--checkout` set to a primary clone, a local half follows a remote half that exited `0`. Every refusal is decided before anything is deleted, the remote branch included: the primary leaving the merged branch, a rebase or bisect paused on it elsewhere, and a tip that is neither the merged head nor behind it. A re-check after the delete can still refuse where a concurrent session changed the branch or a worktree in between, and the output names what already happened. The local half removes the worktrees holding the branch without force, deletes the local branch only at the merged head or an ancestor of it after a tip re-read shows it unchanged, and fast-forwards `develop`. `cleanup --sweep --checkout ` takes no pull request. It reports what is provably finished and removes it only with `--apply`. An item must be clean, idle for `--min-age-hours` (default 24), and not mid-operation. A branch is finished when a pull request from `--repo` merged it at its exact tip or past it, or when `origin/develop` or `origin/main` contains it. The sweep never touches the primary except to fast-forward a tracked-clean `develop`, and never touches permanent branches, locked worktrees, or worktrees with submodules. diff --git a/scripts/pr_cleanup.py b/scripts/pr_cleanup.py index f4a9b8f8..b316a6b9 100755 --- a/scripts/pr_cleanup.py +++ b/scripts/pr_cleanup.py @@ -91,6 +91,7 @@ mutation($refId:ID!){ deleteRef(input:{refId:$refId}){ clientMutationId }} """ +DELETE_SETTLE_DELAYS = (1, 2, 4) class CleanupUnread(Exception): @@ -286,6 +287,7 @@ def cleanup_remote_delete(owner: str, repo: str, plan: CleanupPlan) -> int: """Delete the remote branch `plan` verified and read it back. A delete that reports a failure may still have landed, so the read back decides either way. + A ref still at the deleted tip is read again on `DELETE_SETTLE_DELAYS` before it is reported. """ target = f"{owner}/{repo}" branch = plan.branch @@ -301,17 +303,29 @@ def cleanup_remote_delete(owner: str, repo: str, plan: CleanupPlan) -> int: print(f" the delete reported a failure: {exc}") sys.stderr.write(exc.detail) try: - after = cleanup_ref(owner, repo, branch) + after = core.backoff( + cleanup_ref(owner, repo, branch), + lambda: cleanup_ref(owner, repo, branch), + lambda ref: ref is not None and ref["target"]["oid"] == tip, + delays=DELETE_SETTLE_DELAYS, + ) except CleanupUnread as exc: print( f"status=DELETE_NOT_CONFIRMED the delete of {branch!r} on {target} was sent, and " f"reading the ref back did not answer: {exc}" ) return 2 + if after is not None and after["target"]["oid"] != tip: + print( + f"status=DELETE_NOT_CONFIRMED {branch!r} on {target} reads at " + f"{after['target']['oid']}, a tip other than the one deleted, so something pushed to " + "the branch" + ) + return 2 if after is not None: print( - f"status=DELETE_NOT_CONFIRMED {branch!r} on {target} still reads at " - f"{after['target']['oid']} after the delete" + f"status=DELETE_NOT_CONFIRMED {branch!r} on {target} still reads at {tip} " + "after the delete" ) return 2 print(f"status=DELETED branch={branch} oid={tip}, confirmed by reading the ref back") diff --git a/tests/test_pr_cleanup.py b/tests/test_pr_cleanup.py index 1f182d9b..134e6740 100755 --- a/tests/test_pr_cleanup.py +++ b/tests/test_pr_cleanup.py @@ -61,6 +61,7 @@ def setUp(self) -> None: self.err = self.enterContext(contextlib.redirect_stderr(io.StringIO())) self.enterContext(mock.patch.object(pr_review, "origin_owner", return_value="o")) self.enterContext(mock.patch.object(pr_review, "READ_RETRY_DELAYS", ())) + self.enterContext(mock.patch.object(pr_cleanup, "DELETE_SETTLE_DELAYS", ())) self.enterContext( mock.patch.object(pr_cleanup.handoff, "registry", return_value=("o", {"r"})) ) @@ -186,6 +187,38 @@ def test_a_ref_still_there_after_the_delete_is_not_reported_deleted(self) -> Non self.assertIn("status=DELETE_NOT_CONFIRMED", self.out.getvalue()) self.assertNotIn("status=DELETED", self.out.getvalue()) + def test_a_read_back_lagging_the_delete_settles_to_deleted(self) -> None: + self.wire(cleanup_pr(), refs=[CLEANUP_HEAD, CLEANUP_HEAD, None]) + with mock.patch.object(pr_cleanup, "DELETE_SETTLE_DELAYS", (0, 0)): + self.assertEqual(0, self.run_cleanup()) + self.assertIn("status=DELETED", self.out.getvalue()) + self.assertNotIn("status=DELETE_NOT_CONFIRMED", self.out.getvalue()) + + def test_a_ref_present_on_every_settle_read_still_exits_2(self) -> None: + self.wire(cleanup_pr(), refs=[CLEANUP_HEAD] * 4) + with mock.patch.object(pr_cleanup, "DELETE_SETTLE_DELAYS", (0, 0)): + self.assertEqual(2, self.run_cleanup()) + self.assertIn("status=DELETE_NOT_CONFIRMED", self.out.getvalue()) + reads = [v for q, v in self.docs if "qualifiedName" in q] + self.assertEqual(4, len(reads)) + + def test_a_settle_read_that_does_not_answer_is_not_reported_deleted(self) -> None: + unread = pr_cleanup.shared.GraphQLError("gh graphql failed rc=1") + self.wire(cleanup_pr(), refs=[CLEANUP_HEAD, CLEANUP_HEAD, unread]) + with mock.patch.object(pr_cleanup, "DELETE_SETTLE_DELAYS", (0, 0)): + self.assertEqual(2, self.run_cleanup()) + self.assertIn("reading the ref back did not answer", self.out.getvalue()) + self.assertNotIn("status=DELETED", self.out.getvalue()) + + def test_a_settle_read_at_a_new_tip_stops_and_names_that_tip(self) -> None: + pushed = "c" * 40 + self.wire(cleanup_pr(), refs=[CLEANUP_HEAD, pushed]) + with mock.patch.object(pr_cleanup, "DELETE_SETTLE_DELAYS", (0, 0)): + self.assertEqual(2, self.run_cleanup()) + out = self.out.getvalue() + self.assertIn(f"reads at {pushed}, a tip other than the one deleted", out) + self.assertNotIn("still reads at", out) + def test_a_read_back_that_does_not_answer_is_not_reported_deleted(self) -> None: unread = pr_cleanup.shared.GraphQLError("gh graphql failed rc=1") self.wire(cleanup_pr(), refs=[CLEANUP_HEAD, unread]) From 0da95bc958725cd19c1895c18f41936cb98a18a0 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 13:11:28 -0700 Subject: [PATCH 3/3] Load One Copy of the Gate When pr_review.py Dispatches cleanup (#3481) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Answers #3461. Run as a script, the gate is `__main__`, so `pr_review.py cleanup` imported `pr_cleanup`, whose `import pr_review as core` loaded a second copy of the gate with its own memoized readers and constants. - The `cleanup` dispatch in `main` now runs `sys.modules.setdefault("pr_review", sys.modules[__name__])` before `import pr_cleanup`, so that import resolves to the running module. When the gate is imported as `pr_review` (the tests), `setdefault` changes nothing. - This takes neither route the issue suggested. A separate entry point would break the `pr_review.py cleanup` command that the host-safety allow rule and the `drive-pr` and `merge-and-release` skills name. Moving the six helpers `pr_cleanup` takes into a new module would also move every test patch point, while adding no safety today: `pr_cleanup` is the only module importing `pr_review`. - Test: `TestCleanupLoadsOneGate` runs the real script as `python -X importtime scripts/pr_review.py cleanup --help`. It asserts `pr_cleanup` is imported and `pr_review` is not. It fails against the unfixed source. In script mode a second copy can only come from a fresh import under that name, so the import log answers the issue's `sys.modules` condition. ## Verification - Full suite: `uv run --no-project --with-requirements tests/requirements.txt python -m unittest discover -s tests`, 2953 tests run, OK (7 skipped). - Local strict review: one pass, no findings. It checked every launch mode (`python3`, `uv run --script`, `python -m`, import, `runpy`), every sibling's local imports for the same cycle, and the test's parsing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 --- scripts/pr_review.py | 1 + tests/test_pr_cleanup.py | 16 ++++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 3c056035..1fa20559 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -1648,6 +1648,7 @@ def main(argv: list[str] | None = None) -> int: for flag in ("--repo", "--checkout", "--min-age-hours"): pre.add_argument(flag) if pre.parse_known_args(args)[0].first == "cleanup": + sys.modules.setdefault("pr_review", sys.modules[__name__]) import pr_cleanup rest = list(args) diff --git a/tests/test_pr_cleanup.py b/tests/test_pr_cleanup.py index 134e6740..aa3976c9 100755 --- a/tests/test_pr_cleanup.py +++ b/tests/test_pr_cleanup.py @@ -939,6 +939,22 @@ def test_a_develop_held_by_another_worktree_stops_the_primary_leaving(self) -> N self.assertIn(CLEANUP_BRANCH, self.branches()) +class TestCleanupLoadsOneGate(unittest.TestCase): + def test_the_gate_run_as_a_script_is_not_imported_again_by_cleanup(self) -> None: + """`pr_review.py cleanup` runs the gate as `__main__`, and `pr_cleanup` must reuse it.""" + script = Path(__file__).resolve().parent.parent / "scripts" / "pr_review.py" + result = subprocess.run( + [sys.executable, "-X", "importtime", str(script), "cleanup", "--help"], + capture_output=True, + text=True, + encoding="utf-8", + check=True, + ) + imported = [ln.rpartition("|")[2].strip() for ln in result.stderr.splitlines()] + self.assertIn("pr_cleanup", imported) + self.assertNotIn("pr_review", imported) + + class TestCleanupOriginParsing(unittest.TestCase): def test_https_and_ssh_forms_name_the_repository_case_insensitively(self) -> None: for url in (