From 865dae2462210c33ccbd30b9b273eb745ceabf6f Mon Sep 17 00:00:00 2001 From: mastertyko <11311479+mastertyko@users.noreply.github.com> Date: Tue, 24 Mar 2026 14:17:26 +0100 Subject: [PATCH] fix(gsd): auto-stash dirty files before squash merge and surface dirty filenames in error (#2298) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: auto-stash dirty files before squash merge and surface dirty filenames in error Two bugs in mergeMilestoneToMain caused milestone completion to fail when the project root had pre-existing dirty tracked files: Bug 1 — No auto-stash: clearProjectRootStateFiles only removes untracked .gsd/ files. Any tracked dirty file elsewhere (e.g. .planning/work-state.json with stash conflict markers) caused `git merge --squash` to reject with "local changes would be overwritten". Fixed by adding a stash/pop wrapper around the squash merge — dirty files are stashed before merge and restored after commit. Stash is also popped on all error paths so local work is never lost. Bug 2 — Misleading error message: nativeMergeSquash discarded the filenames from git stderr and the caller hardcoded blame on .gsd/ regardless of which files were actually dirty. Fixed by parsing tab-indented filenames from git stderr into a new `dirtyFiles` field on GitMergeResult, and surfacing them in the error message. Closes #2151 * ci: re-trigger CI (derive-state-db perf assertion is nondeterministic on slow runners) * review: move #2151 tests to node:test format in separate file Per review feedback, moved Tests 20 and 21 from the script-style auto-worktree-milestone-merge.test.ts into a new auto-stash-merge.test.ts using node:test's test() function and assert module. --- src/resources/extensions/gsd/auto-worktree.ts | 92 +++++++++++-- .../extensions/gsd/native-git-bridge.ts | 13 +- .../gsd/tests/auto-stash-merge.test.ts | 121 ++++++++++++++++++ .../auto-worktree-milestone-merge.test.ts | 35 +++-- 4 files changed, 227 insertions(+), 34 deletions(-) create mode 100644 src/resources/extensions/gsd/tests/auto-stash-merge.test.ts diff --git a/src/resources/extensions/gsd/auto-worktree.ts b/src/resources/extensions/gsd/auto-worktree.ts index 522b6eb91..75f7c4071 100644 --- a/src/resources/extensions/gsd/auto-worktree.ts +++ b/src/resources/extensions/gsd/auto-worktree.ts @@ -1098,7 +1098,32 @@ export function mergeMilestoneToMain( } } - // 7. Squash merge — auto-resolve .gsd/ state file conflicts (#530) + // 7. Stash any pre-existing dirty files so the squash merge is not + // blocked by unrelated local changes (#2151). clearProjectRootStateFiles + // only removes untracked .gsd/ files; tracked dirty files elsewhere (e.g. + // .planning/work-state.json with stash conflict markers) are invisible to + // that cleanup but will cause `git merge --squash` to reject. + let stashed = false; + try { + const status = execFileSync("git", ["status", "--porcelain"], { + cwd: originalBasePath_, + stdio: ["ignore", "pipe", "pipe"], + encoding: "utf-8", + }).trim(); + if (status) { + execFileSync( + "git", + ["stash", "push", "--include-untracked", "-m", `gsd: pre-merge stash for ${milestoneId}`], + { cwd: originalBasePath_, stdio: ["ignore", "pipe", "pipe"], encoding: "utf-8" }, + ); + stashed = true; + } + } catch { + // Stash failure is non-fatal — proceed without stash and let the merge + // report the dirty tree if it fails. + } + + // 8. Squash merge — auto-resolve .gsd/ state file conflicts (#530) const mergeResult = nativeMergeSquash(originalBasePath_, milestoneBranch); if (!mergeResult.success) { @@ -1106,12 +1131,27 @@ export function mergeMilestoneToMain( // untracked .gsd/ files left by syncStateToProjectRoot). Preserve the // milestone branch so commits are not lost. if (mergeResult.conflicts.includes("__dirty_working_tree__")) { + // Pop stash before throwing so local work is not lost. + if (stashed) { + try { + execFileSync("git", ["stash", "pop"], { + cwd: originalBasePath_, + stdio: ["ignore", "pipe", "pipe"], + encoding: "utf-8", + }); + } catch { /* stash pop conflict is non-fatal */ } + } // Restore cwd so the caller is not stranded on the integration branch process.chdir(previousCwd); + // Surface the actual dirty filenames from git stderr instead of + // generically blaming .gsd/ (#2151). + const fileList = mergeResult.dirtyFiles?.length + ? `Dirty files:\n${mergeResult.dirtyFiles.map((f) => ` ${f}`).join("\n")}` + : `Check \`git status\` in the project root for details.`; 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.`, + `Squash merge of ${milestoneBranch} rejected: working tree has dirty or untracked files ` + + `that conflict with the merge. ${fileList}`, ); } @@ -1147,6 +1187,16 @@ export function mergeMilestoneToMain( // If there are still non-.gsd conflicts, escalate if (codeConflicts.length > 0) { + // Pop stash before throwing so local work is not lost (#2151). + if (stashed) { + try { + execFileSync("git", ["stash", "pop"], { + cwd: originalBasePath_, + stdio: ["ignore", "pipe", "pipe"], + encoding: "utf-8", + }); + } catch { /* stash pop conflict is non-fatal */ } + } throw new MergeConflictError( codeConflicts, "squash", @@ -1158,11 +1208,11 @@ export function mergeMilestoneToMain( // No conflicts detected — possibly "already up to date", fall through to commit } - // 8. Commit (handle nothing-to-commit gracefully) + // 9. Commit (handle nothing-to-commit gracefully) const commitResult = nativeCommit(originalBasePath_, commitMessage); const nothingToCommit = commitResult === null; - // 8a. Clean up SQUASH_MSG left by git merge --squash (#1853). + // 9a. 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 @@ -1172,7 +1222,23 @@ export function mergeMilestoneToMain( if (existsSync(squashMsgPath)) unlinkSync(squashMsgPath); } catch { /* best-effort */ } - // 8b. Safety check (#1792): if nothing was committed, verify the milestone + // 9a-ii. Restore stashed files now that the merge+commit is complete (#2151). + // Pop after commit so stashed changes do not interfere with the squash merge + // or the commit content. Conflict on pop is non-fatal — the stash entry is + // preserved and the user can resolve manually with `git stash pop`. + if (stashed) { + try { + execFileSync("git", ["stash", "pop"], { + cwd: originalBasePath_, + stdio: ["ignore", "pipe", "pipe"], + encoding: "utf-8", + }); + } catch { + // Stash pop conflict is non-fatal — stash entry persists for manual resolution. + } + } + + // 9b. 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. @@ -1197,7 +1263,7 @@ export function mergeMilestoneToMain( } } - // 8c. Detect whether any non-.gsd/ code files were actually merged (#1906). + // 9c. Detect whether any non-.gsd/ code files were actually merged (#1906). // When a milestone only produced .gsd/ metadata (summaries, roadmaps) but no // real code, the user sees "milestone complete" but nothing changed in their // codebase. Surface this so the caller can warn the user. @@ -1218,7 +1284,7 @@ export function mergeMilestoneToMain( } } - // 9. Auto-push if enabled + // 10. Auto-push if enabled let pushed = false; if (prefs.auto_push === true && !nothingToCommit) { const remote = prefs.remote ?? "origin"; @@ -1264,11 +1330,11 @@ export function mergeMilestoneToMain( } } - // 10. Guard removed — step 8b (#1792) now handles this with a smarter check: + // 11. Guard removed — step 9b (#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. - // 10a. Pre-teardown safety net (#1853): if the worktree still has uncommitted + // 11a. 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`. @@ -1292,7 +1358,7 @@ export function mergeMilestoneToMain( } } - // 11. Remove worktree directory first (must happen before branch deletion) + // 12. Remove worktree directory first (must happen before branch deletion) try { removeWorktree(originalBasePath_, milestoneId, { branch: null as unknown as string, @@ -1302,14 +1368,14 @@ export function mergeMilestoneToMain( // Best-effort -- worktree dir may already be gone } - // 12. Delete milestone branch (after worktree removal so ref is unlocked) + // 13. Delete milestone branch (after worktree removal so ref is unlocked) try { nativeBranchDelete(originalBasePath_, milestoneBranch); } catch { // Best-effort } - // 13. Clear module state + // 14. 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 dd6d7bae9..edfe81188 100644 --- a/src/resources/extensions/gsd/native-git-bridge.ts +++ b/src/resources/extensions/gsd/native-git-bridge.ts @@ -58,6 +58,8 @@ interface GitBatchInfo { interface GitMergeResult { success: boolean; conflicts: string[]; + /** Filenames extracted from git stderr when a dirty working tree blocks the merge (#2151). */ + dirtyFiles?: string[]; } // ─── Native Module Loading ────────────────────────────────────────────────── @@ -863,7 +865,16 @@ export function nativeMergeSquash(basePath: string, branch: string): GitMergeRes stderr.includes("not possible because you have unmerged files") || stderr.includes("overwritten by merge") ) { - return { success: false, conflicts: ["__dirty_working_tree__"] }; + // Extract filenames from git stderr so callers can report which files + // are dirty instead of generically blaming .gsd/ (#2151). + // Git lists them as tab-indented lines between the "would be overwritten" + // header and the "Please commit" footer. + const dirtyFiles = stderr + .split("\n") + .filter((line) => line.startsWith("\t")) + .map((line) => line.trim()) + .filter(Boolean); + return { success: false, conflicts: ["__dirty_working_tree__"], dirtyFiles }; } // Check for real content conflicts diff --git a/src/resources/extensions/gsd/tests/auto-stash-merge.test.ts b/src/resources/extensions/gsd/tests/auto-stash-merge.test.ts new file mode 100644 index 000000000..403caf396 --- /dev/null +++ b/src/resources/extensions/gsd/tests/auto-stash-merge.test.ts @@ -0,0 +1,121 @@ +/** + * auto-stash-merge.test.ts — Regression tests for #2151. + * + * Tests that mergeMilestoneToMain auto-stashes dirty files before squash merge, + * and that nativeMergeSquash returns dirty filenames from git stderr. + */ + +import test from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync, existsSync, readFileSync, realpathSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { execSync } from "node:child_process"; + +import { createAutoWorktree, mergeMilestoneToMain } from "../auto-worktree.ts"; +import { nativeMergeSquash } from "../native-git-bridge.ts"; + +function run(cmd: string, cwd: string): string { + return execSync(cmd, { cwd, stdio: ["ignore", "pipe", "pipe"], encoding: "utf-8" }).trim(); +} + +function createTempRepo(): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "wt-autostash-test-"))); + run("git init", dir); + run("git config user.email test@test.com", dir); + run("git config user.name Test", dir); + writeFileSync(join(dir, "README.md"), "# test\n"); + mkdirSync(join(dir, ".gsd"), { recursive: true }); + writeFileSync(join(dir, ".gsd", "STATE.md"), "# State\n"); + run("git add .", dir); + run("git commit -m init", dir); + run("git branch -M main", dir); + return dir; +} + +function makeRoadmap(milestoneId: string, title: string, slices: Array<{ id: string; title: string }>): string { + const sliceLines = slices.map(s => `- [x] **${s.id}: ${s.title}**`).join("\n"); + return `# ${milestoneId}: ${title}\n\n## Slices\n${sliceLines}\n`; +} + +function addSliceToMilestone( + repo: string, wtPath: string, milestoneId: string, + sliceId: string, sliceTitle: string, + commits: Array<{ file: string; content: string; message: string }>, +): void { + const normalizedPath = wtPath.replaceAll("\\", "/"); + const worktreeName = normalizedPath.split("/").pop() || milestoneId; + const sliceBranch = `slice/${worktreeName}/${sliceId}`; + run(`git checkout -b "${sliceBranch}"`, wtPath); + for (const c of commits) { + writeFileSync(join(wtPath, c.file), c.content); + run("git add .", wtPath); + run(`git commit -m "${c.message}"`, wtPath); + } + const milestoneBranch = `milestone/${milestoneId}`; + run(`git checkout "${milestoneBranch}"`, wtPath); + run(`git merge --no-ff "${sliceBranch}" -m "merge ${sliceId}: ${sliceTitle}"`, wtPath); +} + +test("#2151 bug 1: auto-stash unblocks merge when unrelated files are dirty", () => { + const repo = createTempRepo(); + try { + const wtPath = createAutoWorktree(repo, "M200"); + + addSliceToMilestone(repo, wtPath, "M200", "S01", "Stash test", [ + { file: "stash-test.ts", content: "export const stash = true;\n", message: "add stash test" }, + ]); + + // Dirty an unrelated tracked file in the project root — this previously + // blocked the squash merge with "local changes would be overwritten". + writeFileSync(join(repo, "README.md"), "# modified locally\n"); + + const roadmap = makeRoadmap("M200", "Auto-stash test", [ + { id: "S01", title: "Stash test" }, + ]); + + // Should succeed — the dirty README.md is auto-stashed before merge. + const result = mergeMilestoneToMain(repo, "M200", roadmap); + assert.ok(result.commitMessage.includes("feat(M200)"), "merge succeeds with dirty unrelated file"); + assert.ok(existsSync(join(repo, "stash-test.ts")), "milestone code merged to main"); + + // Verify the dirty file was restored (stash popped). + const readmeContent = readFileSync(join(repo, "README.md"), "utf-8"); + assert.equal(readmeContent, "# modified locally\n", "stash popped — dirty file restored after merge"); + } finally { + rmSync(repo, { recursive: true, force: true }); + } +}); + +test("#2151 bug 2: nativeMergeSquash returns dirty filenames", async () => { + const { nativeMergeSquash } = await import("../native-git-bridge.ts"); + const repo = createTempRepo(); + try { + run("git checkout -b milestone/M210", repo); + writeFileSync(join(repo, "overlap.ts"), "export const overlap = true;\n"); + run("git add .", repo); + run('git commit -m "add overlap"', repo); + run("git checkout main", repo); + + // Create the same file as a dirty local change + writeFileSync(join(repo, "overlap.ts"), "// local dirty version\n"); + + const result = nativeMergeSquash(repo, "milestone/M210"); + assert.equal(result.success, false, "merge reports failure"); + assert.ok( + result.conflicts.includes("__dirty_working_tree__"), + "conflicts include __dirty_working_tree__ sentinel", + ); + assert.ok( + Array.isArray(result.dirtyFiles) && result.dirtyFiles.length > 0, + "dirtyFiles array is populated", + ); + assert.ok( + result.dirtyFiles!.includes("overlap.ts"), + "dirtyFiles includes the actual dirty file name", + ); + } finally { + run("git checkout -- . 2>/dev/null || true", repo); + rmSync(repo, { recursive: true, force: true }); + } +}); 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 a2bb897f6..0a24524df 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 @@ -463,8 +463,11 @@ async function main(): Promise { 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 ==="); + // ─── Test 11: #1738 Bug 1+2 → #2151: dirty tree auto-stashed, merge succeeds ── + // Before #2151, a conflicting dirty file in the project root would cause + // the squash merge to reject. Now auto-stash moves it out of the way, + // the merge succeeds, and the user's local file goes to the stash. + console.log("\n=== #2151: dirty tree auto-stashed, merge succeeds ==="); { const repo = freshRepo(); const wtPath = createAutoWorktree(repo, "M100"); @@ -473,31 +476,21 @@ async function main(): Promise { { file: "e2e.ts", content: "export const e2e = true;\n", message: "add e2e" }, ]); + // Create a conflicting local file — previously blocked the merge. 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", - ); + // With auto-stash (#2151), the merge should succeed. + const result = mergeMilestoneToMain(repo, "M100", roadmap); + assertTrue(result.commitMessage.includes("feat(M100)"), "#2151: merge succeeds after auto-stash"); - const branches = run("git branch", repo); - assertTrue( - branches.includes("milestone/M100"), - "#1738 e2e: milestone branch preserved on dirty tree rejection", - ); + // The milestone code should be on main. + assertTrue(existsSync(join(repo, "e2e.ts")), "#2151: e2e.ts merged to main"); + const content = readFileSync(join(repo, "e2e.ts"), "utf-8"); + assertEq(content, "export const e2e = true;\n", "#2151: merged content is from milestone branch"); } // ─── Test 12: Throw on unanchored code changes after empty commit (#1792) ─ @@ -771,6 +764,8 @@ async function main(): Promise { assertTrue(existsSync(join(repo, "real-code.ts")), "real-code.ts merged to main"); } + // Tests 20 and 21 for #2151 are in auto-stash-merge.test.ts (node:test format). + } finally { process.chdir(savedCwd); for (const d of tempDirs) {