From e01ee8fe86b851ba8e3bbbd48f0901ae2db8d1d0 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sat, 22 Aug 2026 02:12:43 +0000 Subject: [PATCH] fix(land-and-deploy): MERGED recovery reconciles and reports remote-branch cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 4's merge commands carry --delete-branch, and the success path tells the user 'The branch has been cleaned up.' When gh exits non-zero AFTER GitHub already merged (routine in worktree layouts: gh's local cleanup runs git checkout and fails), the §4a-postfail MERGED recovery re-established everything EXCEPT the branch deletion — and said nothing about it, so the discrepancy was invisible. The MERGED path now reconciles: git ls-remote --heads distinguishes branch-already-gone (exit 0, empty → 'already cleaned up', idempotent on re-runs) from branch-survived (offer confirm-first deletion, matching the section's worktree posture; -d not -D for any local branch) from check-itself-failed (non-zero exit → 'couldn't verify', skip the offer — never read a failed check as a clean branch). Template + regenerated SKILL.md + test extensions land in one commit (the md-sync assertion goes red otherwise). Regression assertions (fail on v1.68.3.0: no delete-branch reconciliation existed in test/ at all) pin the ls-remote check, the confirm-first delete, and the absent-vs-failed distinction. Fixes #2656 --- land-and-deploy/SKILL.md | 13 +++++++++++++ land-and-deploy/SKILL.md.tmpl | 13 +++++++++++++ test/land-and-deploy-postfail.test.ts | 20 ++++++++++++++++++++ 3 files changed, 46 insertions(+) diff --git a/land-and-deploy/SKILL.md b/land-and-deploy/SKILL.md index b4121d972..a6079237d 100644 --- a/land-and-deploy/SKILL.md +++ b/land-and-deploy/SKILL.md @@ -1620,6 +1620,19 @@ Identify candidates: a worktree is stale if (a) it is checked out on the base br - If any candidate has uncommitted work: list the files, tell the user, and STOP worktree cleanup without removing anything. - Do NOT use `--force`. Do NOT remove the user's primary working tree. +Remote-branch reconciliation — the failed `gh pr merge` carried `--delete-branch`, and this recovery path must not silently drop that half. The success path above says "The branch has been cleaned up"; this path states the branch outcome explicitly instead of staying silent: + +```bash +BRANCH=$(gh pr view --json headRefName -q .headRefName) +git ls-remote --heads origin "$BRANCH" +``` + +Three outcomes — never read a failed check as a clean branch: + +- **Exit 0, empty output** — the remote branch is already gone (GitHub's post-merge deletion or a concurrent actor got there). Tell the user: "The remote branch has already been cleaned up." This makes re-runs of the recovery idempotent. +- **Exit 0, one ref line** — the branch survived: the failed merge command never reached its `--delete-branch` half. OFFER deletion, confirm-first (matching the worktree-cleanup posture above): "The remote branch `` still exists — the failed merge never ran its --delete-branch half. Delete it?" Only on confirmation: `git push origin --delete "$BRANCH"`. If a local branch of the same name exists, offer `git branch -d "$BRANCH"` alongside (`-d`, never `-D` — a non-fast-forwarded local branch is the user's call). +- **Non-zero exit** — the check ITSELF failed (network, auth). Tell the user: "Couldn't verify remote branch state — leaving it alone." and skip the deletion offer entirely; a failed check is unknown state, not a clean branch. + Record `MERGE_PATH=direct`, then continue to §4a (CI auto-deploy detection). **If `state == "OPEN"`:** diff --git a/land-and-deploy/SKILL.md.tmpl b/land-and-deploy/SKILL.md.tmpl index 4a4551038..b43fbf39d 100644 --- a/land-and-deploy/SKILL.md.tmpl +++ b/land-and-deploy/SKILL.md.tmpl @@ -700,6 +700,19 @@ Identify candidates: a worktree is stale if (a) it is checked out on the base br - If any candidate has uncommitted work: list the files, tell the user, and STOP worktree cleanup without removing anything. - Do NOT use `--force`. Do NOT remove the user's primary working tree. +Remote-branch reconciliation — the failed `gh pr merge` carried `--delete-branch`, and this recovery path must not silently drop that half. The success path above says "The branch has been cleaned up"; this path states the branch outcome explicitly instead of staying silent: + +```bash +BRANCH=$(gh pr view --json headRefName -q .headRefName) +git ls-remote --heads origin "$BRANCH" +``` + +Three outcomes — never read a failed check as a clean branch: + +- **Exit 0, empty output** — the remote branch is already gone (GitHub's post-merge deletion or a concurrent actor got there). Tell the user: "The remote branch has already been cleaned up." This makes re-runs of the recovery idempotent. +- **Exit 0, one ref line** — the branch survived: the failed merge command never reached its `--delete-branch` half. OFFER deletion, confirm-first (matching the worktree-cleanup posture above): "The remote branch `` still exists — the failed merge never ran its --delete-branch half. Delete it?" Only on confirmation: `git push origin --delete "$BRANCH"`. If a local branch of the same name exists, offer `git branch -d "$BRANCH"` alongside (`-d`, never `-D` — a non-fast-forwarded local branch is the user's call). +- **Non-zero exit** — the check ITSELF failed (network, auth). Tell the user: "Couldn't verify remote branch state — leaving it alone." and skip the deletion offer entirely; a failed check is unknown state, not a clean branch. + Record `MERGE_PATH=direct`, then continue to §4a (CI auto-deploy detection). **If `state == "OPEN"`:** diff --git a/test/land-and-deploy-postfail.test.ts b/test/land-and-deploy-postfail.test.ts index f89d77518..d4ea73aaf 100644 --- a/test/land-and-deploy-postfail.test.ts +++ b/test/land-and-deploy-postfail.test.ts @@ -87,6 +87,26 @@ describe("PR #1620 §4a-postfail in land-and-deploy template", () => { expect(body).toMatch(/continue to §4a/); }); + // #2656: the failed merge carried --delete-branch; the recovery path must + // reconcile the remote branch instead of silently dropping that half. + test("MERGED branch reconciles the remote branch (ls-remote, confirm-first delete)", () => { + const body = readTmpl(); + expect(body).toMatch(/git ls-remote --heads origin "\$BRANCH"/); + expect(body).toMatch(/gh pr view --json headRefName -q \.headRefName/); + expect(body).toMatch(/git push origin --delete "\$BRANCH"/); + // Confirm-first: deletion is offered, never unilateral. + expect(body).toMatch(/Delete it\?/); + }); + + test("MERGED branch reconciliation distinguishes branch-absent from check-failed", () => { + const body = readTmpl(); + // exit 0 + empty output = already clean (idempotent re-runs)... + expect(body).toMatch(/already been cleaned up/); + // ...non-zero exit = unknown state, never read as a clean branch. + expect(body).toMatch(/Couldn't verify remote branch state/); + expect(body).toMatch(/never read a failed check as a clean branch/); + }); + test("OPEN branch checks autoMergeRequest before treating as failure", () => { const body = readTmpl(); expect(body).toMatch(/gh pr view --json autoMergeRequest/);