From 747e29b9b40a828a16f06256770fa6e129ed38d4 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 21 Mar 2026 17:24:53 -0400 Subject: [PATCH] fix: clean up SQUASH_MSG after squash-merge and guard worktree teardown against uncommitted changes (#1868) Three changes to prevent data loss and persistent doctor errors in the worktree merge-back lifecycle: 1. After nativeCommit in mergeMilestoneToMain, explicitly delete .git/SQUASH_MSG. The native libgit2 path and git commit -F - on some versions do not auto-remove it, causing doctor to report corrupt_merge_state on every run. 2. Before worktree removal (step 11), check for uncommitted changes and force a final auto-commit if dirty. This prevents code files written by task agents from being destroyed by git worktree remove. 3. Invalidate the nativeHasChanges 10-second cache before the post-unit auto-commit in auto-post-unit.ts. A stale false result causes autoCommit to skip staging entirely. Fixes #1853 Co-authored-by: Claude Opus 4.6 (1M context) --- .../extensions/gsd/auto-post-unit.ts | 8 +++ src/resources/extensions/gsd/auto-worktree.ts | 35 +++++++++++ .../auto-worktree-milestone-merge.test.ts | 63 +++++++++++++++---- 3 files changed, 95 insertions(+), 11 deletions(-) diff --git a/src/resources/extensions/gsd/auto-post-unit.ts b/src/resources/extensions/gsd/auto-post-unit.ts index 4f60e801e..a841d8b22 100644 --- a/src/resources/extensions/gsd/auto-post-unit.ts +++ b/src/resources/extensions/gsd/auto-post-unit.ts @@ -59,6 +59,7 @@ import { existsSync, unlinkSync } from "node:fs"; import { join } from "node:path"; import { uncheckTaskInPlan } from "./undo.js"; import { atomicWriteSync } from "./atomic-write.js"; +import { _resetHasChangesCache } from "./native-git-bridge.js"; /** Throttle STATE.md rebuilds — at most once per 30 seconds */ const STATE_REBUILD_MIN_INTERVAL_MS = 30_000; @@ -156,6 +157,13 @@ export async function postUnitPreVerification(pctx: PostUnitContext, opts?: PreV } } + // Invalidate the nativeHasChanges cache before auto-commit (#1853). + // The cache has a 10-second TTL and is keyed by basePath. A stale + // `false` result causes autoCommit to skip staging entirely, leaving + // code files only in the working tree where they are destroyed by + // `git worktree remove --force` during teardown. + _resetHasChangesCache(); + const commitMsg = autoCommitCurrentBranch(s.basePath, s.currentUnit.type, s.currentUnit.id, taskContext); if (commitMsg) { ctx.ui.notify(`Committed: ${commitMsg.split("\n")[0]}`, "info"); diff --git a/src/resources/extensions/gsd/auto-worktree.ts b/src/resources/extensions/gsd/auto-worktree.ts index 1616ec77c..6e3d2815d 100644 --- a/src/resources/extensions/gsd/auto-worktree.ts +++ b/src/resources/extensions/gsd/auto-worktree.ts @@ -31,6 +31,7 @@ import { gsdRoot } from "./paths.js"; import { createWorktree, removeWorktree, + resolveGitDir, worktreePath, } from "./worktree-manager.js"; import { @@ -1142,6 +1143,16 @@ export function mergeMilestoneToMain( const commitResult = nativeCommit(originalBasePath_, commitMessage); const nothingToCommit = commitResult === null; + // 8a. Clean up SQUASH_MSG left by git merge --squash (#1853). + // git only removes SQUASH_MSG when the commit reads it directly (plain + // `git commit`). nativeCommit uses `-F -` (stdin) or libgit2, neither + // of which trigger git's SQUASH_MSG cleanup. If left on disk, doctor + // reports `corrupt_merge_state` on every subsequent run. + try { + const squashMsgPath = join(resolveGitDir(originalBasePath_), "SQUASH_MSG"); + if (existsSync(squashMsgPath)) unlinkSync(squashMsgPath); + } catch { /* best-effort */ } + // 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 @@ -1217,6 +1228,30 @@ export function mergeMilestoneToMain( // throws only when the milestone has unanchored code changes, passes // through when the code is genuinely already on the integration branch. + // 10a. Pre-teardown safety net (#1853): if the worktree still has uncommitted + // changes (e.g. nativeHasChanges cache returned stale false, or auto-commit + // silently failed), force one final commit so code is not destroyed by + // `git worktree remove --force`. + if (existsSync(worktreeCwd)) { + try { + const dirtyCheck = nativeWorkingTreeStatus(worktreeCwd); + if (dirtyCheck) { + debugLog("mergeMilestoneToMain", { + phase: "pre-teardown-dirty", + worktreeCwd, + status: dirtyCheck.slice(0, 200), + }); + nativeAddAllWithExclusions(worktreeCwd, RUNTIME_EXCLUSION_PATHS); + nativeCommit(worktreeCwd, "chore: pre-teardown auto-commit of uncommitted worktree changes"); + } + } catch (e) { + debugLog("mergeMilestoneToMain", { + phase: "pre-teardown-commit-error", + error: String(e), + }); + } + } + // 11. Remove worktree directory first (must happen before branch deletion) try { removeWorktree(originalBasePath_, milestoneId, { 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 d5dd4039b..0bbf0c39d 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 @@ -639,21 +639,17 @@ async function main(): Promise { { file: "base.ts", content: "export const base = true;\n", message: "add base" }, ]); - // Detach HEAD, then reset branch ref forward independently to create - // divergence (branch ref is NOT an ancestor of worktree HEAD). run("git checkout --detach HEAD", wtPath); writeFileSync(join(wtPath, "detached-work.ts"), "export const detached = true;\n"); run("git add .", wtPath); run('git commit -m "detached work"', wtPath); - // Now advance the branch ref on a different path (via the main repo) run("git checkout milestone/M150", repo); writeFileSync(join(repo, "diverged-work.ts"), "export const diverged = true;\n"); run("git add .", repo); run('git commit -m "diverged work on branch"', repo); run("git checkout main", repo); - // Move back to worktree cwd process.chdir(wtPath); const roadmap = makeRoadmap("M150", "Diverged milestone", [ @@ -669,16 +665,61 @@ async function main(): Promise { errMsg = err instanceof Error ? err.message : String(err); } assertTrue(threw, "throws when worktree HEAD diverged from branch ref (#1846)"); - assertTrue( - errMsg.includes("diverged"), - "error message mentions divergence (#1846)", - ); + assertTrue(errMsg.includes("diverged"), "error message mentions divergence (#1846)"); - // Branch must be preserved — no data loss const branches = run("git branch", repo); + assertTrue(branches.includes("milestone/M150"), "milestone branch preserved on divergence (#1846)"); + } + + // ─── Test 16: #1853 Bug 1 — SQUASH_MSG cleaned up after squash-merge ── + console.log("\n=== #1853 bug 1: SQUASH_MSG cleaned up after successful squash-merge ==="); + { + const repo = freshRepo(); + const wtPath = createAutoWorktree(repo, "M160"); + + addSliceToMilestone(repo, wtPath, "M160", "S01", "SQUASH_MSG cleanup test", [ + { file: "squash-cleanup.ts", content: "export const cleanup = true;\n", message: "add squash-cleanup" }, + ]); + + const roadmap = makeRoadmap("M160", "SQUASH_MSG cleanup", [ + { id: "S01", title: "SQUASH_MSG cleanup test" }, + ]); + + const squashMsgPath = join(repo, ".git", "SQUASH_MSG"); + writeFileSync(squashMsgPath, "leftover squash message\n"); + assertTrue(existsSync(squashMsgPath), "SQUASH_MSG planted before merge"); + + const result = mergeMilestoneToMain(repo, "M160", roadmap); + assertTrue(result.commitMessage.includes("feat(M160)"), "merge commit created"); + assertTrue( - branches.includes("milestone/M150"), - "milestone branch preserved on divergence (#1846)", + !existsSync(squashMsgPath), + "#1853: SQUASH_MSG must not persist after successful squash-merge", + ); + } + + // ─── Test 17: #1853 Bug 2 — uncommitted worktree code survives teardown ── + console.log("\n=== #1853 bug 2: uncommitted worktree changes committed before teardown ==="); + { + const repo = freshRepo(); + const wtPath = createAutoWorktree(repo, "M170"); + + addSliceToMilestone(repo, wtPath, "M170", "S01", "Teardown safety test", [ + { file: "safe-file.ts", content: "export const safe = true;\n", message: "add safe file" }, + ]); + + writeFileSync(join(wtPath, "uncommitted-agent-code.ts"), "export const lost = true;\n"); + + const roadmap = makeRoadmap("M170", "Teardown safety", [ + { id: "S01", title: "Teardown safety test" }, + ]); + + const result = mergeMilestoneToMain(repo, "M170", roadmap); + assertTrue(result.commitMessage.includes("feat(M170)"), "merge commit created"); + + assertTrue( + existsSync(join(repo, "uncommitted-agent-code.ts")), + "#1853: uncommitted worktree code must survive teardown", ); }