From 81f291842d0f09ad15e55b6748ab2347fc4d280d Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 12:30:18 -0700 Subject: [PATCH 1/3] Re-Read a Deleted Ref Before Reporting DELETE_NOT_CONFIRMED The ref read can lag a delete that landed, so cleanup reads a ref still present again on a short schedule through the existing backoff helper before reporting it. Exit 2 still follows once the schedule is spent. Closes #3436 Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_cleanup.py | 9 ++++++++- tests/test_pr_cleanup.py | 16 ++++++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 638b9ae08..5bca5e7a1 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, and a ref still present is read again over a few seconds before it is reported, since the read can lag a delete that landed. 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 f4a9b8f8b..f48882ff4 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 present is read again on `DELETE_SETTLE_DELAYS` before it is reported. """ target = f"{owner}/{repo}" branch = plan.branch @@ -301,7 +303,12 @@ 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, + delays=DELETE_SETTLE_DELAYS, + ) except CleanupUnread as exc: print( f"status=DELETE_NOT_CONFIRMED the delete of {branch!r} on {target} was sent, and " diff --git a/tests/test_pr_cleanup.py b/tests/test_pr_cleanup.py index 1f182d9b2..efc92ef17 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,21 @@ 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_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 278e49ae4da4ea1c37df9434e4d4bc013c3fe3ae Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 12:37:42 -0700 Subject: [PATCH 2/3] Stop the Settle Re-Read at a Tip Other Than the Deleted One A ref back at a new tip means the branch was pushed again, so the loop stops there and says so rather than reporting the delete as unlanded. A test now drives a settle read that does not answer, and the README sentence is split under the length limit. Co-Authored-By: Claude Opus 5.5 --- scripts/README.md | 2 +- scripts/pr_cleanup.py | 12 +++++++++--- tests/test_pr_cleanup.py | 17 +++++++++++++++++ 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 5bca5e7a1..850f08f47 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, and a ref still present is read again over a few seconds before it is reported, since the read can lag a delete that landed. 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 f48882ff4..df2ad59f2 100755 --- a/scripts/pr_cleanup.py +++ b/scripts/pr_cleanup.py @@ -306,7 +306,7 @@ def cleanup_remote_delete(owner: str, repo: str, plan: CleanupPlan) -> int: after = core.backoff( cleanup_ref(owner, repo, branch), lambda: cleanup_ref(owner, repo, branch), - lambda ref: ref is not None, + lambda ref: ref is not None and ref["target"]["oid"] == tip, delays=DELETE_SETTLE_DELAYS, ) except CleanupUnread as exc: @@ -315,10 +315,16 @@ def cleanup_remote_delete(owner: str, repo: str, plan: CleanupPlan) -> int: 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 the branch was " + "pushed again after the delete" + ) + 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 efc92ef17..c4ad1a33e 100755 --- a/tests/test_pr_cleanup.py +++ b/tests/test_pr_cleanup.py @@ -202,6 +202,23 @@ def test_a_ref_present_on_every_settle_read_still_exits_2(self) -> None: 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_says_the_branch_returned(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 2c71411c1f2760a4064deddcdb56bc88a2594a07 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 25 Oct 2026 12:40:09 -0700 Subject: [PATCH 3/3] Word the New-Tip Delete Message Without Assuming the Delete Landed A delete that reported a failure can also read back at another tip, so the message says only that something pushed to the branch. The docstring now names the deleted tip as what is re-read. Co-Authored-By: Claude Opus 5.5 --- scripts/pr_cleanup.py | 9 +++++---- tests/test_pr_cleanup.py | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/scripts/pr_cleanup.py b/scripts/pr_cleanup.py index df2ad59f2..b316a6b92 100755 --- a/scripts/pr_cleanup.py +++ b/scripts/pr_cleanup.py @@ -287,7 +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 present is read again on `DELETE_SETTLE_DELAYS` before it is reported. + 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 @@ -318,13 +318,14 @@ def cleanup_remote_delete(owner: str, repo: str, plan: CleanupPlan) -> int: 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 the branch was " - "pushed again after the delete" + 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 {tip} 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 c4ad1a33e..134e6740a 100755 --- a/tests/test_pr_cleanup.py +++ b/tests/test_pr_cleanup.py @@ -210,7 +210,7 @@ def test_a_settle_read_that_does_not_answer_is_not_reported_deleted(self) -> Non 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_says_the_branch_returned(self) -> None: + 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)):