From 2f0c078cc266c603f226ac07e8184da84facab85 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 21 Mar 2026 14:50:58 -0400 Subject: [PATCH] fix: prevent silent data loss when milestone merge fails due to dirty working tree (#1752) Three bugs caused mergeMilestoneToMain to silently lose all milestone branch commits when git merge --squash was rejected by dirty/untracked .gsd/ files: 1. nativeMergeSquash catch block returned success:true when git rejected a merge pre-staging ("local changes would be overwritten") because the --diff-filter=U conflict check found no markers. Now detects dirty tree rejections via stderr and returns a __dirty_working_tree__ sentinel. 2. mergeMilestoneToMain deleted the milestone branch unconditionally even when nothingToCommit was true (empty squash). Now throws with a clear error and preserves the branch to prevent data loss. 3. clearProjectRootStateFiles only removed STATE.md, auto.lock, and META.json but left synced milestone dirs and runtime/units from syncStateToProjectRoot. These untracked files blocked squash merges. Now cleans all untracked files in synced .gsd/ directories before merge. Fixes #1738 Co-authored-by: Claude Opus 4.6 (1M context) --- src/resources/extensions/gsd/auto-worktree.ts | 101 +++++++--- .../extensions/gsd/native-git-bridge.ts | 19 +- .../auto-worktree-milestone-merge.test.ts | 189 +++++++++++------- 3 files changed, 203 insertions(+), 106 deletions(-) diff --git a/src/resources/extensions/gsd/auto-worktree.ts b/src/resources/extensions/gsd/auto-worktree.ts index 71be82765..35ef5dada 100644 --- a/src/resources/extensions/gsd/auto-worktree.ts +++ b/src/resources/extensions/gsd/auto-worktree.ts @@ -13,6 +13,7 @@ import { readdirSync, mkdirSync, realpathSync, + rmSync, unlinkSync, lstatSync as lstatSyncFn, } from "node:fs"; @@ -77,6 +78,41 @@ function clearProjectRootStateFiles(basePath: string, milestoneId: string): void /* non-fatal — file may not exist */ } } + + // Clean up entire synced milestone directory and runtime/units. + // syncStateToProjectRoot() copies these into the project root during + // execution. If they remain as untracked files when we attempt + // `git merge --squash`, git rejects the merge with "local changes would + // be overwritten", causing silent data loss (#1738). + const syncedDirs = [ + join(gsdDir, "milestones", milestoneId), + join(gsdDir, "runtime", "units"), + ]; + + for (const dir of syncedDirs) { + try { + if (existsSync(dir)) { + // Only remove files that are untracked by git — tracked files are + // managed by the branch checkout and should not be deleted. + const untrackedOutput = execFileSync( + "git", + ["ls-files", "--others", "--exclude-standard", dir], + { cwd: basePath, stdio: ["ignore", "pipe", "pipe"], encoding: "utf-8" }, + ).trim(); + if (untrackedOutput) { + for (const f of untrackedOutput.split("\n").filter(Boolean)) { + try { + unlinkSync(join(basePath, f)); + } catch { + /* non-fatal */ + } + } + } + } + } catch { + /* non-fatal — git command may fail if not in repo */ + } + } } // ─── Worktree ↔ Main Repo Sync (#1311) ────────────────────────────────────── @@ -962,6 +998,19 @@ export function mergeMilestoneToMain( const mergeResult = nativeMergeSquash(originalBasePath_, milestoneBranch); if (!mergeResult.success) { + // Dirty working tree — the merge was rejected before it started (e.g. + // untracked .gsd/ files left by syncStateToProjectRoot). Preserve the + // milestone branch so commits are not lost. + if (mergeResult.conflicts.includes("__dirty_working_tree__")) { + // Restore cwd so the caller is not stranded on the integration branch + process.chdir(previousCwd); + throw new GSDError( + GSD_GIT_ERROR, + `Squash merge of ${milestoneBranch} rejected: working tree has dirty or untracked files that conflict with the merge. ` + + `Clean the project root .gsd/ directory and retry.`, + ); + } + // Check for conflicts — use merge result first, fall back to nativeConflictFiles const conflictedFiles = mergeResult.conflicts.length > 0 @@ -1052,36 +1101,36 @@ export function mergeMilestoneToMain( } } - // 10. Remove worktree directory first (must happen before branch deletion) - // ONLY when a commit was actually produced — if nativeCommit returned null - // (nothing to commit), tearing down the worktree would destroy source code - // that was never merged (#1672). + // 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) { - // eslint-disable-next-line no-console - console.warn( - `[GSD] Warning: squash merge of ${milestoneBranch} produced nothing to commit. ` + - "Worktree and branch preserved to prevent data loss. " + - "Inspect the worktree manually and retry.", + 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.`, ); - } else { - try { - removeWorktree(originalBasePath_, milestoneId, { - branch: null as unknown as string, - deleteBranch: false, - }); - } catch { - // Best-effort -- worktree dir may already be gone - } - - // 11. Delete milestone branch (after worktree removal so ref is unlocked) - try { - nativeBranchDelete(originalBasePath_, milestoneBranch); - } catch { - // Best-effort - } } - // 12. Clear module state + // 11. Remove worktree directory first (must happen before branch deletion) + try { + removeWorktree(originalBasePath_, milestoneId, { + branch: null as unknown as string, + deleteBranch: false, + }); + } catch { + // Best-effort -- worktree dir may already be gone + } + + // 12. Delete milestone branch (after worktree removal so ref is unlocked) + try { + nativeBranchDelete(originalBasePath_, milestoneBranch); + } catch { + // Best-effort + } + + // 13. Clear module state originalBase = null; nudgeGitBranchCache(previousCwd); diff --git a/src/resources/extensions/gsd/native-git-bridge.ts b/src/resources/extensions/gsd/native-git-bridge.ts index 2048aa993..a8d9067d2 100644 --- a/src/resources/extensions/gsd/native-git-bridge.ts +++ b/src/resources/extensions/gsd/native-git-bridge.ts @@ -850,9 +850,22 @@ export function nativeMergeSquash(basePath: string, branch: string): GitMergeRes }); return { success: true, conflicts: [] }; } catch (err: unknown) { - // Check for conflicts — only treat as recoverable if actual conflict - // markers are present. Other failures (bad ref, corrupt repo, etc.) - // must propagate so callers don't assume the merge succeeded (#1672). + // Distinguish pre-merge rejections (dirty working tree) from actual + // content conflicts. When git rejects the merge before staging + // ("local changes would be overwritten"), there are no conflict markers + // to detect, so the old --diff-filter=U check would return an empty + // list and incorrectly report success (#1672, #1738). + const stderr = + err instanceof Error ? (err as Error & { stderr?: string }).stderr ?? err.message : String(err); + if ( + stderr.includes("local changes would be overwritten") || + stderr.includes("not possible because you have unmerged files") || + stderr.includes("overwritten by merge") + ) { + return { success: false, conflicts: ["__dirty_working_tree__"] }; + } + + // Check for real content conflicts const conflictOutput = gitExec(basePath, ["diff", "--name-only", "--diff-filter=U"], true); const conflicts = conflictOutput ? conflictOutput.split("\n").filter(Boolean) : []; if (conflicts.length > 0) { 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 218750e62..f823ceedc 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 @@ -181,8 +181,8 @@ async function main(): Promise { assertTrue(gitMsg.includes("- S01: Core API"), "git commit body has S01"); } - // ─── Test 3: Nothing to commit — no changes ──────────────────────── - console.log("\n=== nothing to commit — no changes ==="); + // ─── Test 3: Nothing to commit — preserves branch (#1738) ────────── + console.log("\n=== nothing to commit — preserves branch (#1738) ==="); { const repo = freshRepo(); const wtPath = createAutoWorktree(repo, "M030"); @@ -190,15 +190,21 @@ async function main(): Promise { // Don't add any slices/changes — milestone branch is identical to main const roadmap = makeRoadmap("M030", "Empty milestone", []); - // Should complete without throwing + // Should throw to prevent silent branch deletion when squash is empty let threw = false; + let errorMsg = ""; try { - const result = mergeMilestoneToMain(repo, "M030", roadmap); - assertTrue(typeof result.pushed === "boolean", "returns result even with nothing to commit"); - } catch { + mergeMilestoneToMain(repo, "M030", roadmap); + } catch (err: unknown) { threw = true; + errorMsg = err instanceof Error ? err.message : String(err); } - assertTrue(!threw, "does not throw on nothing-to-commit"); + 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"); // Main log unchanged (only init commit) const mainLog = run("git log --oneline main", repo); @@ -327,18 +333,8 @@ async function main(): Promise { } // ─── Test 7: Repo using `master` as default branch (#1668) ──────── - // - // Reproduces the exact failure mode from the bug report: a repo initialised - // with `master`, no GSD preferences file setting `main_branch`, and no - // META.json (so readIntegrationBranch returns null). Before the fix, - // mergeMilestoneToMain would fall back to the hardcoded string "main", - // attempt `git checkout main`, fail, and leave the user with a broken state - // and a confusing error. After the fix, nativeDetectMainBranch detects - // `master` and the squash-merge succeeds normally. console.log("\n=== master-branch repo — no META.json, no prefs (#1668) ==="); { - // Build a repo with `master` as the default branch (not `main`). - // Use -b master to override the system default (which may be `main`). const dir = realpathSync(mkdtempSync(join(tmpdir(), "wt-ms-master-test-"))); tempDirs.push(dir); run("git init -b master", dir); @@ -349,19 +345,14 @@ async function main(): Promise { writeFileSync(join(dir, ".gsd", "STATE.md"), "# State\n"); run("git add .", dir); run("git commit -m init", dir); - // Leave the default branch as `master` — do NOT run `git branch -M main` const defaultBranch = run("git rev-parse --abbrev-ref HEAD", dir); assertEq(defaultBranch, "master", "repo is on master branch"); - // Create a worktree for the milestone (creates milestone/M070 branch) const wtPath = createAutoWorktree(dir, "M070"); - addSliceToMilestone(dir, wtPath, "M070", "S01", "Master branch test", [ { file: "master-feature.ts", content: "export const masterFeature = true;\n", message: "add master feature" }, ]); - // No META.json written (simulates the captureIntegrationBranch failure - // described in the issue) — readIntegrationBranch will return null. const metaFile = join(dir, ".gsd", "milestones", "M070", "M070-META.json"); assertTrue(!existsSync(metaFile), "no META.json — integration branch not captured"); @@ -369,7 +360,6 @@ async function main(): Promise { { id: "S01", title: "Master branch test" }, ]); - // Should succeed: nativeDetectMainBranch detects `master` automatically. let threw = false; let errMsg = ""; try { @@ -378,11 +368,9 @@ async function main(): Promise { } catch (err) { threw = true; errMsg = err instanceof Error ? err.message : String(err); - console.error("Unexpected error:", err); } assertTrue(!threw, `should not throw on master-branch repo (got: ${errMsg})`); - // Verify the code landed on master and the milestone branch is gone const finalBranch = run("git rev-parse --abbrev-ref HEAD", dir); assertEq(finalBranch, "master", "repo is still on master after merge"); assertTrue(existsSync(join(dir, "master-feature.ts")), "feature merged to master"); @@ -390,88 +378,135 @@ async function main(): Promise { assertTrue(!branches.includes("milestone/M070"), "milestone branch deleted after merge"); } - // ─── Test 8: Worktree preserved when commit is empty (#1672) ────── - console.log("\n=== worktree preserved when commit is empty (#1672) ==="); + // ─── Test 8: #1738 Bug 1 — dirty working tree detected by nativeMergeSquash ── + console.log("\n=== #1738 bug 1: nativeMergeSquash detects dirty working tree ==="); + { + const { nativeMergeSquash } = await import("../native-git-bridge.ts"); + const repo = freshRepo(); + + run("git checkout -b milestone/M070", repo); + writeFileSync(join(repo, "feature.ts"), "export const feature = true;\n"); + run("git add .", repo); + run('git commit -m "add feature"', repo); + run("git checkout main", repo); + + writeFileSync(join(repo, "feature.ts"), "// local dirty version\n"); + + const result = nativeMergeSquash(repo, "milestone/M070"); + assertEq(result.success, false, "merge reports failure on dirty working tree"); + assertTrue( + result.conflicts.includes("__dirty_working_tree__"), + "conflicts include __dirty_working_tree__ sentinel", + ); + + run("git checkout -- . 2>/dev/null || true", repo); + run("rm -f feature.ts", repo); + } + + // ─── Test 9: #1738 Bug 2 — branch preserved on empty squash commit ── + console.log("\n=== #1738 bug 2: branch preserved when squash commit empty ==="); { const repo = freshRepo(); const wtPath = createAutoWorktree(repo, "M080"); - // Do NOT add any slices/changes — milestone branch is identical to main. - // This simulates the WSL stat-cache bug where autoCommitCurrentBranch - // skips commits, leaving the milestone branch identical to main. + // Make no changes — squash will produce nothing to commit const roadmap = makeRoadmap("M080", "Empty milestone", []); - // Capture console.warn to verify the warning is emitted - const warnings: string[] = []; - const origWarn = console.warn; - console.warn = (...args: unknown[]) => { - warnings.push(args.map(String).join(" ")); - }; - + let threw = false; try { mergeMilestoneToMain(repo, "M080", roadmap); - } finally { - console.warn = origWarn; + } 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"); } + assertTrue(threw, "#1738 throws to prevent silent data loss"); - // Milestone branch must still exist (not deleted) const branches = run("git branch", repo); assertTrue( branches.includes("milestone/M080"), - "milestone branch preserved when nothing was committed (#1672)", - ); - - // A warning should have been emitted - assertTrue( - warnings.some((w) => w.includes("nothing to commit")), - "emits warning about empty merge (#1672)", + "#1738 milestone branch NOT deleted on empty squash", ); } - // ─── Test 9: Worktree removed when commit succeeds (#1672) ────── - console.log("\n=== worktree removed when commit succeeds (#1672) ==="); + // ─── Test 10: #1738 Bug 3 — clearProjectRootStateFiles cleans synced dirs ── + console.log("\n=== #1738 bug 3: synced .gsd/ dirs cleaned before merge ==="); { const repo = freshRepo(); const wtPath = createAutoWorktree(repo, "M090"); - addSliceToMilestone(repo, wtPath, "M090", "S01", "Teardown test", [ - { file: "teardown.ts", content: "export const teardown = true;\n", message: "add teardown file" }, + addSliceToMilestone(repo, wtPath, "M090", "S01", "Sync test", [ + { file: "sync-test.ts", content: "export const sync = true;\n", message: "add sync-test" }, ]); - const roadmap = makeRoadmap("M090", "Teardown verification", [ - { id: "S01", title: "Teardown test" }, - ]); - - mergeMilestoneToMain(repo, "M090", roadmap); - - // Milestone branch must be deleted - const branches = run("git branch", repo); - assertTrue( - !branches.includes("milestone/M090"), - "milestone branch deleted after successful commit (#1672)", + // Simulate syncStateToProjectRoot: create untracked .gsd/ milestone files + const msDir = join(repo, ".gsd", "milestones", "M090", "slices", "S01"); + mkdirSync(msDir, { recursive: true }); + writeFileSync(join(msDir, "S01-PLAN.md"), "# synced plan\n"); + writeFileSync( + join(repo, ".gsd", "milestones", "M090", "M090-ROADMAP.md"), + "# synced roadmap\n", ); - // Worktree directory must be removed - const worktreeDir = join(repo, ".gsd", "worktrees", "M090"); - assertTrue(!existsSync(worktreeDir), "worktree directory removed after successful commit (#1672)"); + const runtimeDir = join(repo, ".gsd", "runtime", "units"); + mkdirSync(runtimeDir, { recursive: true }); + writeFileSync(join(runtimeDir, "unit-001.json"), '{"stale": true}'); - // File should be on main - assertTrue(existsSync(join(repo, "teardown.ts")), "teardown.ts merged to main (#1672)"); - } + const roadmap = makeRoadmap("M090", "Sync cleanup test", [ + { id: "S01", title: "Sync test" }, + ]); - // ─── Test 10: nativeMergeSquash throws on non-conflict failures (#1672) ─ - console.log("\n=== nativeMergeSquash throws on non-conflict failures (#1672) ==="); - { - const repo = freshRepo(); - - // Merge a nonexistent branch — a non-conflict failure that must throw let threw = false; try { - nativeMergeSquash(repo, "nonexistent-branch"); - } catch { + const result = mergeMilestoneToMain(repo, "M090", roadmap); + assertTrue( + result.commitMessage.includes("feat(M090)"), + "#1738 merge succeeds after cleaning synced dirs", + ); + } catch (err: unknown) { threw = true; + console.error("#1738 bug 3 regression:", err); } - assertTrue(threw, "nativeMergeSquash throws on nonexistent branch (#1672)"); + assertTrue(!threw, "#1738 merge does not fail on synced .gsd/ files"); + assertTrue(existsSync(join(repo, "sync-test.ts")), "sync-test.ts on main after merge"); + } + + // ─── Test 11: #1738 Bug 1+2 — dirty tree merge preserves branch end-to-end ── + console.log("\n=== #1738 e2e: dirty tree rejection preserves branch ==="); + { + const repo = freshRepo(); + const wtPath = createAutoWorktree(repo, "M100"); + + addSliceToMilestone(repo, wtPath, "M100", "S01", "E2E test", [ + { file: "e2e.ts", content: "export const e2e = true;\n", message: "add e2e" }, + ]); + + writeFileSync(join(repo, "e2e.ts"), "// conflicting local file\n"); + + const roadmap = makeRoadmap("M100", "E2E dirty tree", [ + { id: "S01", title: "E2E test" }, + ]); + + let threw = false; + let errorMsg = ""; + try { + mergeMilestoneToMain(repo, "M100", roadmap); + } catch (err: unknown) { + threw = true; + errorMsg = err instanceof Error ? err.message : String(err); + } + assertTrue(threw, "#1738 e2e: throws on dirty working tree"); + assertTrue( + errorMsg.includes("dirty") || errorMsg.includes("untracked") || errorMsg.includes("overwritten"), + "#1738 e2e: error identifies dirty tree cause", + ); + + const branches = run("git branch", repo); + assertTrue( + branches.includes("milestone/M100"), + "#1738 e2e: milestone branch preserved on dirty tree rejection", + ); } } finally {