From 0982b2019c51c0a00b3e26c3a52d60c83a77b69b Mon Sep 17 00:00:00 2001 From: NagyVikt Date: Fri, 7 Aug 2026 13:36:21 +0200 Subject: [PATCH] fix(finish): report MERGED as success instead of failing on branch cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 -> ` + 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) --- .../.openspec.yaml | 2 + .../proposal.md | 45 ++++++++++ .../merged-pr-ends-finish-as-success/spec.md | 36 ++++++++ .../tasks.md | 34 +++++++ templates/scripts/agent-branch-finish.sh | 65 ++++++++++++-- test/finish.test.js | 89 +++++++++++++++++++ 6 files changed, 264 insertions(+), 7 deletions(-) create mode 100644 openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/.openspec.yaml create mode 100644 openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/proposal.md create mode 100644 openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/specs/merged-pr-ends-finish-as-success/spec.md create mode 100644 openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/tasks.md diff --git a/openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/.openspec.yaml b/openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/.openspec.yaml new file mode 100644 index 0000000..878dc31 --- /dev/null +++ b/openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-07 diff --git a/openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/proposal.md b/openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/proposal.md new file mode 100644 index 0000000..9294398 --- /dev/null +++ b/openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/proposal.md @@ -0,0 +1,45 @@ +## Why + +`gx branch finish` could merge the PR and still exit 1, so a successful ship +read as a failure. The merge lands, then cleanup runs `git branch -d` on the +source branch; `git branch -d` requires an ancestor link to HEAD or full +coverage by the branch's upstream, and a squash merge — this flow's default +(`gh pr merge --squash`) — creates neither once the remote branch is gone. The +refusal was fatal (`exit 1`), even though every other post-merge cleanup step +(remote delete, worktree prune) only warns. + +Observed on PR #695: the merge completed, the local branch had been rebased +during finish, `git branch -d` reported "not fully merged", and the run ended +`branchFinish command failed with status 1` with no statement that the merge +had succeeded. The visible outcome invited a force-push to "fix" a merge that +had already landed. + +## What Changes + +- Print a definitive `✅ MERGED -> ` line (plus the PR URL) as + soon as the merge is confirmed, before any cleanup runs, so the outcome + cannot be buried under cleanup warnings. +- Treat a refused post-merge local-branch delete as a warning, not a failure. + The merge already landed; cleanup leftovers are reported, never fatal. +- When `git branch -d` refuses, ask GitHub whether that exact head landed in a + merged PR (`read_merged_pr_for_head`). If it did — the ordinary squash-merge + case — force the delete. If it did not, keep the branch: its commits exist + nowhere else. +- When a branch is deliberately kept, skip `--delete-branches` / + `--delete-remote-branches` on the follow-up prune, which deletes with + `git branch -D` and would otherwise destroy exactly what was just protected. +- Report the real outcome in the closing summary ("kept source branch" vs the + existing "cleaned source branch/remote" and "cleaned source branch/worktree" + wordings, both preserved verbatim). + +## Impact + +- Affected surface: `templates/scripts/agent-branch-finish.sh` cleanup path + (symlinked as `scripts/agent-branch-finish.sh`); `test/finish.test.js`. +- Exit-status change: a merged PR whose local branch cannot be deleted now + exits 0 instead of 1. Callers that treated exit 1 as "merge failed" get a + more accurate signal; nothing that previously exited 0 changes. +- Risk: force-deleting a branch that did not really land. Mitigated by gating + the force on GitHub's merged-PR record for that exact head SHA, and by + keeping the branch plus its remote whenever that check does not confirm. +- No API, schema, or config surface changes. No version bump. diff --git a/openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/specs/merged-pr-ends-finish-as-success/spec.md b/openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/specs/merged-pr-ends-finish-as-success/spec.md new file mode 100644 index 0000000..333290d --- /dev/null +++ b/openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/specs/merged-pr-ends-finish-as-success/spec.md @@ -0,0 +1,36 @@ +## ADDED Requirements + +### Requirement: A completed merge SHALL be reported as success +`gx branch finish` SHALL announce the merge as soon as it is confirmed and +before any cleanup step runs, and SHALL NOT report a non-zero exit status for +a run whose merge landed. + +#### Scenario: Merge lands and cleanup is clean +- **WHEN** the PR merges and every cleanup step succeeds +- **THEN** the run prints `✅ MERGED -> ` and the PR URL +- **AND** the run exits 0. + +#### Scenario: Merge lands but the local branch cannot be deleted +- **WHEN** the PR merges and `git branch -d` refuses the source branch +- **THEN** the run prints `✅ MERGED -> ` before the refusal +- **AND** the run warns that the branch was kept, naming the inspect and + delete commands +- **AND** the run exits 0. + +### Requirement: A refused branch delete SHALL be resolved by merge evidence +When `git branch -d` refuses the source branch after a merge, the system SHALL +consult GitHub for a merged PR whose head SHA equals that branch's head, and +SHALL force the delete only when such a PR exists. + +#### Scenario: Squash merge left no ancestor link +- **WHEN** `git branch -d` refuses and a merged PR records this exact head SHA +- **THEN** the branch is force-deleted +- **AND** the closing summary reports the branch as cleaned. + +#### Scenario: Branch holds commits that never landed +- **WHEN** `git branch -d` refuses and no merged PR records this head SHA +- **THEN** the local branch is kept +- **AND** the remote branch is kept +- **AND** the follow-up prune is invoked without `--delete-branches` and + without `--delete-remote-branches` +- **AND** the closing summary reports the branch as kept. diff --git a/openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/tasks.md b/openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/tasks.md new file mode 100644 index 0000000..b07f97a --- /dev/null +++ b/openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/tasks.md @@ -0,0 +1,34 @@ +## Definition of Done + +This change is complete only when **all** of the following are true: + +- Every checkbox below is checked. +- The agent branch reaches `MERGED` state on `origin` and the PR URL + state are recorded in the completion handoff. +- If any step blocks (test failure, conflict, ambiguous result), append a `BLOCKED:` line under section 4 explaining the blocker and **STOP**. Do not tick remaining cleanup boxes; do not silently skip the cleanup pipeline. + +## Handoff + +- Handoff: change=`agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17`; branch=`agent/claude/merged-pr-ends-finish-as-success-2026-08-07-13-17`; scope=`gx branch finish: report MERGED as success; stop failing on post-merge branch cleanup`; action=`continue this sandbox or finish cleanup after a usage-limit/manual takeover`. +- Copy prompt: Continue `agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17` on branch `agent/claude/merged-pr-ends-finish-as-success-2026-08-07-13-17`. Work inside the existing sandbox, review `openspec/changes/agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17/tasks.md`, continue from the current state instead of creating a new sandbox, and when the work is done run `gx branch finish --branch agent/claude/merged-pr-ends-finish-as-success-2026-08-07-13-17 --base main --via-pr --wait-for-merge --cleanup`. + +## 1. Specification + +- [x] 1.1 Finalize proposal scope and acceptance criteria for `agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17`. +- [x] 1.2 Define normative requirements in `specs/merged-pr-ends-finish-as-success/spec.md`. + +## 2. Implementation + +- [x] 2.1 Implement scoped behavior changes. +- [x] 2.2 Add/update focused regression coverage. + +## 3. Verification + +- [x] 3.1 Run targeted project verification commands. +- [x] 3.2 Run `openspec validate agent-claude-merged-pr-ends-finish-as-success-2026-08-07-13-17 --type change --strict`. +- [x] 3.3 Run `openspec validate --specs`. + +## 4. Cleanup (mandatory; run before claiming completion) + +- [ ] 4.1 Run the cleanup pipeline: `gx branch finish --branch agent/claude/merged-pr-ends-finish-as-success-2026-08-07-13-17 --base main --via-pr --wait-for-merge --cleanup`. This handles commit -> push -> PR create -> merge wait -> worktree prune in one invocation. +- [ ] 4.2 Record the PR URL and final merge state (`MERGED`) in the completion handoff. +- [ ] 4.3 Confirm the sandbox worktree is gone (`git worktree list` no longer shows the agent path; `git branch -a` shows no surviving local/remote refs for the branch). diff --git a/templates/scripts/agent-branch-finish.sh b/templates/scripts/agent-branch-finish.sh index 781d116..419b890 100755 --- a/templates/scripts/agent-branch-finish.sh +++ b/templates/scripts/agent-branch-finish.sh @@ -1011,6 +1011,20 @@ delete_local_branch_for_cleanup() { return 0 fi + # `git branch -d` insists on an ancestor link to HEAD. A squash merge — the + # default this flow uses (`gh pr merge --squash`) — never creates one, so the + # refusal here is the NORMAL post-merge outcome, not a sign the work is + # unmerged. GitHub is the authority on that, so ask whether this exact head + # landed in a merged PR and only then force the delete. When it did not (a + # rebase during finish, or commits pushed after the merge), fall through and + # keep the branch rather than destroying commits that never landed. + if read_merged_pr_for_head "$(git -C "$repo_root" rev-parse "$branch" 2>/dev/null || true)"; then + if git -C "$repo_root" branch -D "$branch" >/dev/null 2>&1; then + echo "[agent-branch-finish] Local branch '${branch}' had no ancestor link to '${BASE_BRANCH}' (squash merge), but its head landed in a merged PR; deleted it." >&2 + return 0 + fi + fi + echo "$delete_output" >&2 return 1 } @@ -1509,6 +1523,16 @@ if [[ "$PUSH_ENABLED" -eq 1 ]]; then fi fi +# Reaching here means the merge landed. That is the outcome the caller cares +# about, so state it once, unmistakably, BEFORE any best-effort cleanup runs — +# otherwise a warning from worktree/branch teardown reads as the headline and +# the merge scrolls away. Everything below this line is cleanup: it can warn, +# it must not fail the run, because the work is already in the base branch. +echo "[agent-branch-finish] ✅ MERGED ${SOURCE_BRANCH} -> ${BASE_BRANCH} (${merge_status} flow)" +if [[ -n "$pr_url" ]]; then + echo "[agent-branch-finish] ✅ PR: ${pr_url}" +fi + run_guardex_cli locks release --branch "$SOURCE_BRANCH" >/dev/null 2>&1 || true base_worktree="$(get_worktree_for_branch "$BASE_BRANCH")" @@ -1549,11 +1573,19 @@ if [[ "$CLEANUP_AFTER_MERGE" -eq 1 ]]; then git -C "$repo_root" worktree remove "$source_worktree" --force >/dev/null 2>&1 || true fi + # The merge already landed (see the MERGED banner above), so a branch that + # refuses to delete is a leftover to report, not a reason to fail the run and + # make a successful ship look broken. Keep the branch AND its remote in that + # case: the commits it still holds exist nowhere else. + local_branch_cleaned=1 if ! delete_local_branch_for_cleanup "$SOURCE_BRANCH"; then - exit 1 + local_branch_cleaned=0 + echo "[agent-branch-finish] Warning: kept local branch '${SOURCE_BRANCH}' — it holds commits that never landed in '${BASE_BRANCH}' (rebased during finish, or pushed after the merge)." >&2 + echo "[agent-branch-finish] Inspect: git log ${BASE_BRANCH}..${SOURCE_BRANCH}" >&2 + echo "[agent-branch-finish] Delete once you are satisfied: git branch -D ${SOURCE_BRANCH}" >&2 fi - if [[ "$PUSH_ENABLED" -eq 1 && "$DELETE_REMOTE_BRANCH" -eq 1 ]]; then + if [[ "$local_branch_cleaned" -eq 1 && "$PUSH_ENABLED" -eq 1 && "$DELETE_REMOTE_BRANCH" -eq 1 ]]; then if git -C "$repo_root" ls-remote --exit-code --heads origin "$SOURCE_BRANCH" >/dev/null 2>&1; then remote_delete_output="" if ! remote_delete_output="$(git -C "$repo_root" push origin --delete "$SOURCE_BRANCH" 2>&1)"; then @@ -1567,9 +1599,18 @@ if [[ "$CLEANUP_AFTER_MERGE" -eq 1 ]]; then fi fi - prune_args=(--base "$BASE_BRANCH" --only-dirty-worktrees --delete-branches) - if [[ "$DELETE_REMOTE_BRANCH" -eq 1 ]]; then - prune_args+=(--delete-remote-branches) + # prune deletes with `git branch -D`, so handing it --delete-branches right + # after we deliberately kept a branch with unlanded commits would force-delete + # exactly what we just protected. Skip branch deletion for this run; the next + # finish or an explicit `gx cleanup` sweeps the rest. + prune_args=(--base "$BASE_BRANCH" --only-dirty-worktrees) + if [[ "$local_branch_cleaned" -eq 1 ]]; then + prune_args+=(--delete-branches) + if [[ "$DELETE_REMOTE_BRANCH" -eq 1 ]]; then + prune_args+=(--delete-remote-branches) + fi + else + echo "[agent-branch-finish] Skipping branch deletion during prune so '${SOURCE_BRANCH}' survives; sweep later with: gx cleanup --base ${BASE_BRANCH}" >&2 fi pivot_to_repo_root_before_prune @@ -1578,12 +1619,22 @@ if [[ "$CLEANUP_AFTER_MERGE" -eq 1 ]]; then echo "[agent-branch-finish] You can run manual cleanup: gx cleanup --base ${BASE_BRANCH}" >&2 fi + # Say what actually happened: claiming the branch was cleaned when it was + # deliberately kept is the same misreport in the other direction. The two + # cleaned wordings differ ("branch/remote" vs "branch/worktree") and callers + # match on them, so keep each one exactly as it was. + if [[ "$local_branch_cleaned" -eq 1 ]]; then + kept_branch_summary="" + else + kept_branch_summary="kept source branch (commits not in '${BASE_BRANCH}')" + fi + if [[ "$source_worktree" == "$current_worktree" && "$source_worktree" == "${agent_worktree_root}"/* && -d "$source_worktree" ]]; then - echo "[agent-branch-finish] Merged '${SOURCE_BRANCH}' into '${BASE_BRANCH}' via ${merge_status} flow and cleaned source branch/remote." + echo "[agent-branch-finish] Merged '${SOURCE_BRANCH}' into '${BASE_BRANCH}' via ${merge_status} flow and ${kept_branch_summary:-cleaned source branch/remote}." echo "[agent-branch-finish] Current worktree '${source_worktree}' still exists because it is the active shell cwd." >&2 echo "[agent-branch-finish] Leave this directory, then run: gx cleanup --base ${BASE_BRANCH}" >&2 else - echo "[agent-branch-finish] Merged '${SOURCE_BRANCH}' into '${BASE_BRANCH}' via ${merge_status} flow and cleaned source branch/worktree." + echo "[agent-branch-finish] Merged '${SOURCE_BRANCH}' into '${BASE_BRANCH}' via ${merge_status} flow and ${kept_branch_summary:-cleaned source branch/worktree}." fi else pivot_to_repo_root_before_prune diff --git a/test/finish.test.js b/test/finish.test.js index 010b22c..b252a19 100644 --- a/test/finish.test.js +++ b/test/finish.test.js @@ -691,6 +691,95 @@ fi ); }); +test('agent-branch-finish reports MERGED and succeeds when the post-merge local branch delete is refused', () => { + const repoDir = initRepo(); + seedCommit(repoDir); + attachOriginRemote(repoDir); + + let result = runNode(['setup', '--target', repoDir, '--no-global-install'], repoDir); + assert.equal(result.status, 0, result.stderr || result.stdout); + result = runCmd('git', ['add', '.'], repoDir); + assert.equal(result.status, 0, result.stderr); + result = runCmd('git', ['commit', '-m', 'apply gx setup'], repoDir, { + ALLOW_COMMIT_ON_PROTECTED_BRANCH: '1', + }); + assert.equal(result.status, 0, result.stderr); + result = runCmd('git', ['push', 'origin', 'dev'], repoDir); + assert.equal(result.status, 0, result.stderr); + + result = runCmd('git', ['checkout', '-b', 'agent/test-merged-branch-delete-refused'], repoDir); + assert.equal(result.status, 0, result.stderr); + commitFile(repoDir, 'agent-merged-delete-refused.txt', 'agent change\n', 'agent change'); + + // `pr merge` succeeds but leaves the local branch in place, and `pr list` + // reports no merged PR for this head — so `git branch -d` refuses (the + // branch's commits are not ancestors of dev) and the force-delete is not + // authorized. The merge still happened, so the run must still succeed. + const { fakePath: fakeGhPath } = createFakeGhScript(` +if [[ "$1" == "pr" && "$2" == "create" ]]; then + exit 0 +fi +if [[ "$1" == "pr" && "$2" == "view" ]]; then + if [[ " $* " == *" --json body "* ]]; then + echo "" + exit 0 + fi + if [[ " $* " == *" --json url "* ]]; then + echo "https://example.test/pr/merged-delete-refused" + exit 0 + fi + echo "unexpected gh pr view args: $*" >&2 + exit 1 +fi +if [[ "$1" == "pr" && "$2" == "list" ]]; then + echo "" + exit 0 +fi +if [[ "$1" == "pr" && "$2" == "merge" ]]; then + # A real squash merge lands the work as a brand new commit and (with + # --delete-branch) drops the remote branch. That leaves the local branch + # covered by neither dev nor its upstream, which is what makes 'git branch + # -d' refuse it. + git_bin="$(command -v git)" + "$git_bin" -C "${'${GUARDEX_TEST_REPO_DIR}'}" update-ref -d "refs/remotes/origin/$3" >/dev/null 2>&1 || true + exit 0 +fi +echo "unexpected gh args: $*" >&2 +exit 1 +`); + + const finish = runBranchFinish( + ['--branch', 'agent/test-merged-branch-delete-refused', '--mode', 'pr', '--cleanup'], + repoDir, + { + GUARDEX_GH_BIN: fakeGhPath, + GUARDEX_TEST_REPO_DIR: repoDir, + }, + ); + + // The merge is the outcome: it must be stated, and it must not exit non-zero + // just because the local branch could not be torn down afterwards. + assert.equal(finish.status, 0, finish.stderr || finish.stdout); + assert.match( + finish.stdout, + /✅ MERGED {2}agent\/test-merged-branch-delete-refused -> dev \(pr flow\)/, + ); + assert.match(finish.stdout, /✅ PR: https:\/\/example\.test\/pr\/merged-delete-refused/); + assert.match( + finish.stderr, + /Warning: kept local branch 'agent\/test-merged-branch-delete-refused'/, + ); + + // The branch it declined to delete must still be there — that is the whole + // point of declining. + const branchStillThere = runCmd( + 'git', + ['show-ref', '--verify', '--quiet', 'refs/heads/agent/test-merged-branch-delete-refused'], + repoDir, + ); + assert.equal(branchStillThere.status, 0, 'branch with unlanded commits must be kept'); +}); + test('agent-branch-finish cleanup tolerates an already-deleted local branch after gh delete warning', () => { const repoDir = initRepo(); seedCommit(repoDir);