Skip to content

fix(finish): report MERGED as success instead of failing on branch cleanup - #697

Merged
NagyVikt merged 1 commit into
mainfrom
agent/claude/merged-pr-ends-finish-as-success-2026-08-07-13-17
Aug 7, 2026
Merged

fix(finish): report MERGED as success instead of failing on branch cleanup#697
NagyVikt merged 1 commit into
mainfrom
agent/claude/merged-pr-ends-finish-as-success-2026-08-07-13-17

Conversation

@NagyVikt

@NagyVikt NagyVikt commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • fix(finish): report MERGED as success instead of failing on branch cleanup

Test plan

  • verified locally

…eanup

`gx branch finish` could merge the PR and still exit 1. After the merge,
cleanup runs `git branch -d` on the source branch, which requires an
ancestor link to HEAD or full upstream coverage. A squash merge — the
default here (`gh pr merge --squash`) — leaves neither once the remote
branch is gone, so the refusal is the NORMAL post-merge outcome. It was
fatal, while every other cleanup step only warns.

Seen on #695: merge landed, branch had been rebased during finish,
`branch -d` said "not fully merged", run ended "branchFinish command
failed with status 1" and never said the merge had succeeded. That
output invites a force-push to fix a merge that already landed.

- announce `MERGED <source> -> <base>` + PR URL the moment the merge is
  confirmed, before any cleanup can bury it
- downgrade a refused local-branch delete to a warning
- when `branch -d` refuses, ask GitHub whether that exact head landed in
  a merged PR; force the delete only then, else keep the branch
- when a branch is kept, drop --delete-branches/--delete-remote-branches
  from the follow-up prune, which uses `git branch -D` and would destroy
  exactly what was protected
- report "kept source branch" vs the two existing "cleaned ..." wordings,
  both preserved verbatim

Test suite failing set md5-identical to main (41 = 41); new regression
test covers merge-lands-but-delete-refused.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@NagyVikt
NagyVikt marked this pull request as ready for review August 7, 2026 11:37
@NagyVikt

NagyVikt commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Decision: APPROVE — 0 CRITICAL, 0 HIGH, 0 MEDIUM, 2 LOW.

Caveat stated up front: this is a self-review by the change's author, not an independent one. The repo's automated review gate (gx review / the removed cr.yml) no longer dispatches, so it is the strongest review available here.

Makes a landed merge report as success. The root cause is that git branch -d requires an ancestor link to HEAD or full upstream coverage, and a squash merge — this flow's default via gh pr merge --squash — leaves neither once the remote branch is dropped. That refusal was fatal while every other post-merge cleanup step only warns.

Verified the premise rather than assuming it: in an isolated repo, a squash merge leaves f.txt byte-identical in main, and git branch -d still errors "not fully merged". So this fired on ordinary squash merges, not just the rebase case seen on #695.

The force-delete is gated on GitHub's merged-PR record for that exact head SHA, not on a patch-id guess. That distinction matters: git cherry reports + (no upstream equivalent) for every commit of a squash-merged multi-commit branch, so patch-id matching would have been wrong here.

Behavior tested against the real extracted function, both directions:

Case Result
Squash-merged, GitHub confirms head landed force-deleted, returns 0
Head in no merged PR (rebase / post-merge commits) branch kept, returns 1, caller warns and exits 0

Caught and fixed one self-inflicted regression mid-review: collapsing the two closing summaries lost the distinct branch/remote vs branch/worktree wordings that tests match on — 8 failures, now restored verbatim and re-verified.

LOW-1: read_merged_pr_for_head mutates PR_STATE / PR_MERGED_AT / pr_url as a side effect. Harmless today — the only consumer is the banner, which already printed by then (confirmed: no pr_url reads after line 1533) — but a latent trap if someone later reads those globals during cleanup.

LOW-2: when a branch is kept, the prune drops --delete-branches for all dirty lanes that run, not just the protected one. Conservative in the safe direction (branches survive rather than getting -D'd), and gx cleanup sweeps the rest.

Tests: 845 total, 41 failing — failing set md5-identical to main (f003cdc8…), zero new. New regression test covers merge-lands-but-delete-refused. openspec validate --specs 133/133; change validates --strict. Symlink parity OK.

@NagyVikt
NagyVikt merged commit cf4d000 into main Aug 7, 2026
3 of 4 checks passed
@NagyVikt
NagyVikt deleted the agent/claude/merged-pr-ends-finish-as-success-2026-08-07-13-17 branch August 7, 2026 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant