A CodeRabbit finding has recurred identically across three PRs touching merge-and-release/SKILL.md's and drive-pr/SKILL.md's branch cleanup steps: #936 (the original promotion review), #940 (the merge-and-release/drive-pr fix chain), and #945 (the next promotion). Declined each time with the same reasoning below, but it keeps resurfacing, so it needs an actual decision rather than another decline.
What the cleanup steps do today
Both skills' cleanup, after confirming (via gh pr list/gh pr view, matched on headRepository.nameWithOwner and headRefOid) that a feature branch's PR genuinely merged, delete the branch in two separate steps:
- Verify the local and/or remote branch tip still matches the confirmed
headRefOid (git rev-parse --verify, git ls-remote --heads --exit-code).
- Delete it (
git branch -D, git push origin --delete), as a separate, later command.
The finding
Between steps 1 and 2 there is a TOCTOU (time-of-check-to-time-of-use) window: a concurrent push could move the branch after verification but before deletion, and the plain delete would remove whatever is there at that moment, not what was verified. CodeRabbit's suggested fix (most detailed version, from #945):
Delete the local ref with an expected-OID operation such as git update-ref -d "refs/heads/<branch>" "<headRefOid>", and use an explicit expected-value lease for the remote delete; stop when the expected OID does not match or conditional deletion is unavailable. Before deletion, verify that both the fetch and resolved push URLs for origin refer to the intended repository, since they may point to different destinations.
The remote half of that is functionally a git push --force-with-lease=<ref>:<expected-oid> for a delete.
Why it's been declined each time
git-commit-conventions's "Never force push" section is a blanket, unconditional rule:
Do not run git push --force or git push --force-with-lease under any circumstances. Force pushing rewrites shared history and can cause data loss. This holds regardless of how confident the rewrite looks, a rejected push is recoverable, a force-pushed one is not.
Implementing CodeRabbit's suggestion means carving a narrow, explicit exception into that rule for this one cleanup case. That's a real policy change to a rule that exists because of a specific incident, not a mechanical bug fix an agent should decide unilaterally mid-PR. CodeRabbit's own severity marking on the #945 instance was "Heavy lift", agreeing it's more than a quick fix.
The practical risk is also narrow as things stand: the branch being deleted is one the current task exclusively owns per repo-worktree's isolation model, and deletion happens immediately after GitHub itself confirms the merge, so the race window is small and the actor who could win it would have to be pushing to a branch that just finished its own PR.
The decision needed
- Is the current two-step check-then-delete acceptable as-is (declining the finding permanently, with a note explaining why), or does the race genuinely warrant closing?
- If closing it: does
git-commit-conventions' blanket force-push prohibition get a narrow, explicitly-scoped exception for this one case (an OID-gated delete of a branch this task's own cleanup just verified), or is there a different mechanism that avoids touching that rule at all (e.g., a repository-side branch protection setting, a cleanup lock file, or something else)?
- Separately, and lower-stakes: should the cleanup also verify
origin's fetch and push URLs resolve to the intended owner/repo before deleting anything, independent of the OID question?
Filed at the maintainer's request after the finding recurred a third time, to review together before deciding.
A CodeRabbit finding has recurred identically across three PRs touching
merge-and-release/SKILL.md's anddrive-pr/SKILL.md's branch cleanup steps: #936 (the original promotion review), #940 (the merge-and-release/drive-pr fix chain), and #945 (the next promotion). Declined each time with the same reasoning below, but it keeps resurfacing, so it needs an actual decision rather than another decline.What the cleanup steps do today
Both skills' cleanup, after confirming (via
gh pr list/gh pr view, matched onheadRepository.nameWithOwnerandheadRefOid) that a feature branch's PR genuinely merged, delete the branch in two separate steps:headRefOid(git rev-parse --verify,git ls-remote --heads --exit-code).git branch -D,git push origin --delete), as a separate, later command.The finding
Between steps 1 and 2 there is a TOCTOU (time-of-check-to-time-of-use) window: a concurrent push could move the branch after verification but before deletion, and the plain delete would remove whatever is there at that moment, not what was verified. CodeRabbit's suggested fix (most detailed version, from #945):
The remote half of that is functionally a
git push --force-with-lease=<ref>:<expected-oid>for a delete.Why it's been declined each time
git-commit-conventions's "Never force push" section is a blanket, unconditional rule:Implementing CodeRabbit's suggestion means carving a narrow, explicit exception into that rule for this one cleanup case. That's a real policy change to a rule that exists because of a specific incident, not a mechanical bug fix an agent should decide unilaterally mid-PR. CodeRabbit's own severity marking on the #945 instance was "Heavy lift", agreeing it's more than a quick fix.
The practical risk is also narrow as things stand: the branch being deleted is one the current task exclusively owns per
repo-worktree's isolation model, and deletion happens immediately after GitHub itself confirms the merge, so the race window is small and the actor who could win it would have to be pushing to a branch that just finished its own PR.The decision needed
git-commit-conventions' blanket force-push prohibition get a narrow, explicitly-scoped exception for this one case (an OID-gated delete of a branch this task's own cleanup just verified), or is there a different mechanism that avoids touching that rule at all (e.g., a repository-side branch protection setting, a cleanup lock file, or something else)?origin's fetch and push URLs resolve to the intendedowner/repobefore deleting anything, independent of the OID question?Filed at the maintainer's request after the finding recurred a third time, to review together before deciding.