From a7ad0caf9f09eca84d155b0f08531914a9439fa4 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 21 Mar 2026 15:23:50 -0400 Subject: [PATCH] fix(auto): verify merge anchored before worktree teardown (#1829) * fix(auto): verify merge anchored before worktree teardown Fixes #1792 Co-Authored-By: Claude Opus 4.6 (1M context) * fix: replace unconditional empty-commit throw with anchor check Step 10's unconditional throw on nothingToCommit conflicted with step 8b's smarter anchor check (#1792). Step 8b correctly distinguishes "genuinely safe empty" (code already on main) from "data loss risk" (unanchored code changes). Updated tests accordingly. Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) --- src/resources/extensions/gsd/auto-worktree.ts | 43 ++++++--- .../auto-worktree-milestone-merge.test.ts | 96 +++++++++++++++---- 2 files changed, 109 insertions(+), 30 deletions(-) diff --git a/src/resources/extensions/gsd/auto-worktree.ts b/src/resources/extensions/gsd/auto-worktree.ts index 4f34c2aef..e16998c1d 100644 --- a/src/resources/extensions/gsd/auto-worktree.ts +++ b/src/resources/extensions/gsd/auto-worktree.ts @@ -56,6 +56,7 @@ import { nativeRmForce, nativeBranchDelete, nativeBranchExists, + nativeDiffNumstat, } from "./native-git-bridge.js"; // ─── Module State ────────────────────────────────────────────────────────── @@ -932,7 +933,8 @@ function autoCommitDirtyState(cwd: string): boolean { * 9. Clear originalBase * * On merge conflict: throws MergeConflictError. - * On "nothing to commit" after squash: handles gracefully (no error). + * On "nothing to commit" after squash: safe only if milestone work is already + * on the integration branch. Throws if unanchored code changes would be lost. */ export function mergeMilestoneToMain( originalBasePath_: string, @@ -1064,6 +1066,31 @@ export function mergeMilestoneToMain( const commitResult = nativeCommit(originalBasePath_, commitMessage); const nothingToCommit = commitResult === null; + // 8b. Safety check (#1792): if nothing was committed, verify the milestone + // work is already on the integration branch before allowing teardown. + // Compare only non-.gsd/ paths — .gsd/ state files diverge normally and + // are auto-resolved during the squash merge. + if (nothingToCommit) { + const numstat = nativeDiffNumstat( + originalBasePath_, + mainBranch, + milestoneBranch, + ); + const codeChanges = numstat.filter( + (entry) => !entry.path.startsWith(".gsd/"), + ); + if (codeChanges.length > 0) { + // Milestone has unanchored code changes — abort teardown. + process.chdir(previousCwd); + throw new GSDError( + GSD_GIT_ERROR, + `Squash merge produced nothing to commit but milestone branch "${milestoneBranch}" ` + + `has ${codeChanges.length} code file(s) not on "${mainBranch}". ` + + `Aborting worktree teardown to prevent data loss.`, + ); + } + } + // 9. Auto-push if enabled let pushed = false; if (prefs.auto_push === true && !nothingToCommit) { @@ -1107,17 +1134,9 @@ export function mergeMilestoneToMain( } } - // 10. Guard: if squash produced nothing to commit, the milestone branch has - // changes that were not merged. Preserve the branch and worktree so - // commits are not silently lost (#1672, #1738). - if (nothingToCommit) { - process.chdir(previousCwd); - throw new GSDError( - GSD_GIT_ERROR, - `Squash merge of ${milestoneBranch} produced an empty commit — milestone branch preserved to prevent data loss. ` + - `Inspect the branch manually and retry.`, - ); - } + // 10. Guard removed — step 8b (#1792) now handles this with a smarter check: + // throws only when the milestone has unanchored code changes, passes + // through when the code is genuinely already on the integration branch. // 11. Remove worktree directory first (must happen before branch deletion) try { diff --git a/src/resources/extensions/gsd/tests/auto-worktree-milestone-merge.test.ts b/src/resources/extensions/gsd/tests/auto-worktree-milestone-merge.test.ts index f823ceedc..2af1d8697 100644 --- a/src/resources/extensions/gsd/tests/auto-worktree-milestone-merge.test.ts +++ b/src/resources/extensions/gsd/tests/auto-worktree-milestone-merge.test.ts @@ -182,7 +182,7 @@ async function main(): Promise { } // ─── Test 3: Nothing to commit — preserves branch (#1738) ────────── - console.log("\n=== nothing to commit — preserves branch (#1738) ==="); + console.log("\n=== nothing to commit — safe when no code changes (#1738, #1792) ==="); { const repo = freshRepo(); const wtPath = createAutoWorktree(repo, "M030"); @@ -190,7 +190,8 @@ async function main(): Promise { // Don't add any slices/changes — milestone branch is identical to main const roadmap = makeRoadmap("M030", "Empty milestone", []); - // Should throw to prevent silent branch deletion when squash is empty + // Should NOT throw — milestone branch is identical to main, nothing to lose. + // The anchor check (#1792) verifies no code files differ and passes through. let threw = false; let errorMsg = ""; try { @@ -199,12 +200,7 @@ async function main(): Promise { threw = true; errorMsg = err instanceof Error ? err.message : String(err); } - assertTrue(threw, "throws on nothing-to-commit to preserve branch"); - assertTrue(errorMsg.includes("empty commit"), "error message mentions empty commit"); - - // Milestone branch must still exist — not deleted - const branches = run("git branch", repo); - assertTrue(branches.includes("milestone/M030"), "milestone branch preserved when squash is empty"); + assertTrue(!threw, `safe empty milestone should not throw (got: ${errorMsg})`); // Main log unchanged (only init commit) const mainLog = run("git log --oneline main", repo); @@ -412,22 +408,17 @@ async function main(): Promise { // Make no changes — squash will produce nothing to commit const roadmap = makeRoadmap("M080", "Empty milestone", []); + // With the #1792 anchor check, empty milestones with no code changes + // are safe to proceed — no data to lose. let threw = false; + let errMsg = ""; try { mergeMilestoneToMain(repo, "M080", roadmap); } catch (err: unknown) { threw = true; - const msg = err instanceof Error ? err.message : String(err); - assertTrue(msg.includes("empty commit"), "#1738 error says empty commit"); - assertTrue(msg.includes("preserved"), "#1738 error says branch preserved"); + errMsg = err instanceof Error ? err.message : String(err); } - assertTrue(threw, "#1738 throws to prevent silent data loss"); - - const branches = run("git branch", repo); - assertTrue( - branches.includes("milestone/M080"), - "#1738 milestone branch NOT deleted on empty squash", - ); + assertTrue(!threw, `empty milestone with no code changes should not throw (got: ${errMsg})`); } // ─── Test 10: #1738 Bug 3 — clearProjectRootStateFiles cleans synced dirs ── @@ -509,6 +500,75 @@ async function main(): Promise { ); } + // ─── Test 12: Throw on unanchored code changes after empty commit (#1792) ─ + console.log("\n=== throw on unanchored code changes after empty commit (#1792) ==="); + { + const repo = freshRepo(); + const wtPath = createAutoWorktree(repo, "M120"); + + addSliceToMilestone(repo, wtPath, "M120", "S01", "Critical feature", [ + { file: "critical.ts", content: "export const critical = true;\n", message: "add critical feature" }, + ]); + + // Simulate: merge then revert — git considers branch "already merged" + // but code is NOT on main (reverted). + run(`git merge milestone/M120 --no-ff -m "merge M120"`, repo); + run("git revert HEAD --no-edit -m 1", repo); + + const roadmap = makeRoadmap("M120", "Critical milestone", [ + { id: "S01", title: "Critical feature" }, + ]); + + let threw = false; + let errMsg = ""; + try { + mergeMilestoneToMain(repo, "M120", roadmap); + } catch (err) { + threw = true; + errMsg = err instanceof Error ? err.message : String(err); + } + assertTrue(threw, "throws when milestone has unanchored code changes (#1792)"); + assertTrue( + errMsg.includes("code file(s) not on"), + "error message mentions unanchored code files (#1792)", + ); + + const branches = run("git branch", repo); + assertTrue( + branches.includes("milestone/M120"), + "milestone branch preserved when code is unanchored (#1792)", + ); + } + + // ─── Test 13: Safe teardown when nothing-to-commit and work already on main (#1792) ─ + console.log("\n=== safe teardown — nothing to commit, work already on main (#1792) ==="); + { + const repo = freshRepo(); + const wtPath = createAutoWorktree(repo, "M130"); + + addSliceToMilestone(repo, wtPath, "M130", "S01", "Already landed", [ + { file: "landed.ts", content: "export const landed = true;\n", message: "add landed feature" }, + ]); + + run("git merge --squash milestone/M130", repo); + run('git commit -m "pre-land milestone work"', repo); + + const roadmap = makeRoadmap("M130", "Pre-landed milestone", [ + { id: "S01", title: "Already landed" }, + ]); + + let threw = false; + let errMsg = ""; + try { + mergeMilestoneToMain(repo, "M130", roadmap); + } catch (err) { + threw = true; + errMsg = err instanceof Error ? err.message : String(err); + } + assertTrue(!threw, `safe nothing-to-commit should not throw (got: ${errMsg})`); + assertTrue(existsSync(join(repo, "landed.ts")), "landed.ts present on main"); + } + } finally { process.chdir(savedCwd); for (const d of tempDirs) {