Promote the pr_review Status Text and Cleanup Fixes to Main

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 <clone>` 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.
 

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

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(
@@ -1638,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)

scripts/pr_review_rounds.py

@@ -31,6 +31,7 @@
 )
 TRUSTED_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"})
 ACK_MARKER = "<!-- fleet-shape-ack: fingerprint={} -->"
+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 = ""

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])
@@ -906,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 (

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<!-- fleet-review: reviewed=1 changed=3 findings=0 -->"
         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<!-- fleet-review: reviewed=1 changed=3 findings=0 -->"
+        odd = f"{OVERVIEW}\n<!-- fleet-review: reviewed=4 changed=3 findings=0 -->"
+        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"