From 9823fd2d2df3a3271750f3c57ed8788004f268b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?T=C3=82CHES?= Date: Thu, 26 Mar 2026 20:16:42 -0600 Subject: [PATCH] fix: resolve stash pop conflicts and stop swallowing merge errors (#2780) * fix: resolve stash pop conflicts and stop swallowing merge errors After a squash merge, `git stash pop` can conflict on `.gsd/` state files, leaving them in UU state that permanently blocks all subsequent milestone merges. The post-commit stash pop catch block now detects `.gsd/` conflicts, auto-resolves them by accepting the HEAD version (matching the existing merge-time policy), and drops the stash when safe. In phases.ts, three catch blocks only handled MergeConflictError and silently continued on any other error, allowing auto-mode to advance to the next milestone with unmerged work. All three now stop auto-mode and return a "merge-failed" break result for non-conflict errors. Closes #2766 Co-Authored-By: Claude Opus 4.6 (1M context) * test: add regression tests for stash pop conflict and error handling Co-Authored-By: Claude Opus 4.6 (1M context) * fix: use correct LogComponent type in stash pop handler Co-Authored-By: Claude Opus 4.6 (1M context) * fix: join file array for logWarning in stash pop handler 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 | 42 +++++- src/resources/extensions/gsd/auto/phases.ts | 24 +++- .../phases-merge-error-stops-auto.test.ts | 103 +++++++++++++++ .../gsd/tests/stash-pop-gsd-conflict.test.ts | 125 ++++++++++++++++++ 4 files changed, 291 insertions(+), 3 deletions(-) create mode 100644 src/resources/extensions/gsd/tests/phases-merge-error-stops-auto.test.ts create mode 100644 src/resources/extensions/gsd/tests/stash-pop-gsd-conflict.test.ts diff --git a/src/resources/extensions/gsd/auto-worktree.ts b/src/resources/extensions/gsd/auto-worktree.ts index 4727517dc..ca39f72b8 100644 --- a/src/resources/extensions/gsd/auto-worktree.ts +++ b/src/resources/extensions/gsd/auto-worktree.ts @@ -1499,7 +1499,47 @@ export function mergeMilestoneToMain( encoding: "utf-8", }); } catch { - // Stash pop conflict is non-fatal — stash entry persists for manual resolution. + // Stash pop after squash merge can conflict on .gsd/ state files that + // diverged between branches. Left unresolved, these UU entries block + // every subsequent merge. Auto-resolve them the same way we handle + // .gsd/ conflicts during the merge itself: accept HEAD (the just-committed + // version) and drop the now-applied stash. + const uu = nativeConflictFiles(originalBasePath_); + const gsdUU = uu.filter((f) => f.startsWith(".gsd/")); + const nonGsdUU = uu.filter((f) => !f.startsWith(".gsd/")); + + if (gsdUU.length > 0) { + for (const f of gsdUU) { + try { + // Accept the committed (HEAD) version of the state file + execFileSync("git", ["checkout", "HEAD", "--", f], { + cwd: originalBasePath_, + stdio: ["ignore", "pipe", "pipe"], + encoding: "utf-8", + }); + nativeAddPaths(originalBasePath_, [f]); + } catch { + // Last resort: remove the conflicted state file + nativeRmForce(originalBasePath_, [f]); + } + } + } + + if (nonGsdUU.length === 0) { + // All conflicts were .gsd/ files — safe to drop the stash + try { + execFileSync("git", ["stash", "drop"], { + cwd: originalBasePath_, + stdio: ["ignore", "pipe", "pipe"], + encoding: "utf-8", + }); + } catch { /* stash may already be consumed */ } + } else { + // Non-.gsd conflicts remain — leave stash for manual resolution + logWarning("reconcile", "Stash pop conflict on non-.gsd files after merge", { + files: nonGsdUU.join(", "), + }); + } } } diff --git a/src/resources/extensions/gsd/auto/phases.ts b/src/resources/extensions/gsd/auto/phases.ts index c785bf2c0..966247a5e 100644 --- a/src/resources/extensions/gsd/auto/phases.ts +++ b/src/resources/extensions/gsd/auto/phases.ts @@ -261,8 +261,14 @@ export async function runPreDispatch( await deps.stopAuto(ctx, pi, `Merge conflict on milestone ${s.currentMilestoneId}`); return { action: "break", reason: "merge-conflict" }; } - // Non-conflict merge errors — log and continue - logWarning("engine", "Milestone merge failed with non-conflict error", { milestone: s.currentMilestoneId!, error: String(mergeErr) }); + // Non-conflict merge errors — stop auto to avoid advancing with unmerged work + logError("engine", "Milestone merge failed with non-conflict error", { milestone: s.currentMilestoneId!, error: String(mergeErr) }); + ctx.ui.notify( + `Merge failed: ${mergeErr instanceof Error ? mergeErr.message : String(mergeErr)}. Resolve and run /gsd auto to resume.`, + "error", + ); + await deps.stopAuto(ctx, pi, `Merge error on milestone ${s.currentMilestoneId}: ${String(mergeErr)}`); + return { action: "break", reason: "merge-failed" }; } // PR creation (auto_pr) is handled inside mergeMilestoneToMain (#2302) @@ -355,6 +361,13 @@ export async function runPreDispatch( await deps.stopAuto(ctx, pi, `Merge conflict on milestone ${s.currentMilestoneId}`); return { action: "break", reason: "merge-conflict" }; } + logError("engine", "Milestone merge failed with non-conflict error", { milestone: s.currentMilestoneId!, error: String(mergeErr) }); + ctx.ui.notify( + `Merge failed: ${mergeErr instanceof Error ? mergeErr.message : String(mergeErr)}. Resolve and run /gsd auto to resume.`, + "error", + ); + await deps.stopAuto(ctx, pi, `Merge error on milestone ${s.currentMilestoneId}: ${String(mergeErr)}`); + return { action: "break", reason: "merge-failed" }; } // PR creation (auto_pr) is handled inside mergeMilestoneToMain (#2302) @@ -452,6 +465,13 @@ export async function runPreDispatch( await deps.stopAuto(ctx, pi, `Merge conflict on milestone ${s.currentMilestoneId}`); return { action: "break", reason: "merge-conflict" }; } + logError("engine", "Milestone merge failed with non-conflict error", { milestone: s.currentMilestoneId!, error: String(mergeErr) }); + ctx.ui.notify( + `Merge failed: ${mergeErr instanceof Error ? mergeErr.message : String(mergeErr)}. Resolve and run /gsd auto to resume.`, + "error", + ); + await deps.stopAuto(ctx, pi, `Merge error on milestone ${s.currentMilestoneId}: ${String(mergeErr)}`); + return { action: "break", reason: "merge-failed" }; } // PR creation (auto_pr) is handled inside mergeMilestoneToMain (#2302) diff --git a/src/resources/extensions/gsd/tests/phases-merge-error-stops-auto.test.ts b/src/resources/extensions/gsd/tests/phases-merge-error-stops-auto.test.ts new file mode 100644 index 000000000..5323d4ae4 --- /dev/null +++ b/src/resources/extensions/gsd/tests/phases-merge-error-stops-auto.test.ts @@ -0,0 +1,103 @@ +/** + * phases-merge-error-stops-auto.test.ts — Regression test for #2766. + * + * When mergeAndExit throws a non-MergeConflictError, the auto loop must + * stop instead of continuing with unmerged work. This test verifies that + * all catch blocks in auto/phases.ts that handle mergeAndExit errors + * call stopAuto and return { action: "break" } for non-conflict errors. + */ + +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { createTestContext } from "./test-helpers.ts"; + +const { assertTrue, report } = createTestContext(); + +const phasesPath = join(import.meta.dirname, "..", "auto", "phases.ts"); +const phasesSrc = readFileSync(phasesPath, "utf-8"); + +console.log("\n=== #2766: Non-MergeConflictError stops auto mode ==="); + +// ── Test 1: phases.ts calls logError for non-conflict merge errors ────── + +assertTrue( + phasesPath.length > 0 && phasesPath.endsWith("phases.ts"), + "phases.ts file exists and is readable", +); + +// Count all mergeAndExit catch blocks by finding "} catch (mergeErr)" patterns +const mergeErrCatches = [...phasesPath.matchAll(/\} catch \(mergeErr\)/g)]; +// Use the source itself for matching +const mergeErrCatchCount = [...phasesSrc.matchAll(/\} catch \(mergeErr\)/g)].length; +assertTrue( + mergeErrCatchCount >= 3, + `all mergeAndExit call sites have catch (mergeErr) blocks (found ${mergeErrCatchCount}, expected >= 3)`, +); + +// ── Test 2: Every mergeErr catch block handles non-MergeConflictError ─── + +// Find each catch block and verify it has the non-conflict error handling pattern +const catchPattern = /\} catch \(mergeErr\) \{/g; +let match; +let blocksWithNonConflictHandling = 0; +let blocksTotal = 0; + +while ((match = catchPattern.exec(phasesSrc)) !== null) { + blocksTotal++; + // Look at the ~800 chars after the catch to find both the MergeConflictError + // instanceof check AND the non-conflict handling + const afterCatch = phasesSrc.slice(match.index, match.index + 1200); + + const hasInstanceofCheck = afterCatch.includes("instanceof MergeConflictError"); + const hasNonConflictStop = afterCatch.includes('reason: "merge-failed"'); + const hasStopAuto = afterCatch.includes("stopAuto"); + const hasLogError = afterCatch.includes("logError"); + + if (hasInstanceofCheck && hasNonConflictStop && hasStopAuto && hasLogError) { + blocksWithNonConflictHandling++; + } +} + +assertTrue( + blocksWithNonConflictHandling === blocksTotal && blocksTotal >= 3, + `all ${blocksTotal} mergeAndExit catch blocks stop auto on non-conflict errors (${blocksWithNonConflictHandling}/${blocksTotal})`, +); + +// ── Test 3: Non-conflict handler returns break (does not continue) ────── + +// Verify the pattern: after the MergeConflictError instanceof block, +// the non-conflict path returns { action: "break", reason: "merge-failed" } +const mergeFailedReasons = [...phasesSrc.matchAll(/reason: "merge-failed"/g)].length; +assertTrue( + mergeFailedReasons >= 3, + `all catch blocks return reason: "merge-failed" (found ${mergeFailedReasons}, expected >= 3)`, +); + +// ── Test 4: Non-conflict handler notifies user ────────────────────────── + +// Each non-conflict block should call ctx.ui.notify with error severity +const notifyErrorPattern = /Merge failed:.*Resolve and run \/gsd auto to resume/g; +const notifyCount = [...phasesSrc.matchAll(notifyErrorPattern)].length; +assertTrue( + notifyCount >= 3, + `all catch blocks notify user about merge failure (found ${notifyCount}, expected >= 3)`, +); + +// ── Test 5: logError replaces logWarning for non-conflict merge errors ── + +// The old code used logWarning — verify logError is used instead +const logWarningMergePattern = /logWarning\(.*Milestone merge failed with non-conflict error/g; +const logWarningCount = [...phasesSrc.matchAll(logWarningMergePattern)].length; +assertTrue( + logWarningCount === 0, + "logWarning is no longer used for non-conflict merge errors (replaced by logError)", +); + +const logErrorMergePattern = /logError\(.*Milestone merge failed with non-conflict error/g; +const logErrorCount = [...phasesSrc.matchAll(logErrorMergePattern)].length; +assertTrue( + logErrorCount >= 3, + `logError is used for non-conflict merge errors (found ${logErrorCount}, expected >= 3)`, +); + +report(); diff --git a/src/resources/extensions/gsd/tests/stash-pop-gsd-conflict.test.ts b/src/resources/extensions/gsd/tests/stash-pop-gsd-conflict.test.ts new file mode 100644 index 000000000..89ad125ae --- /dev/null +++ b/src/resources/extensions/gsd/tests/stash-pop-gsd-conflict.test.ts @@ -0,0 +1,125 @@ +/** + * stash-pop-gsd-conflict.test.ts — Regression test for #2766. + * + * When a squash merge stash-pops and hits conflicts on .gsd/ state files, + * the UU entries block every subsequent merge. This test verifies that + * mergeMilestoneToMain auto-resolves .gsd/ conflicts by accepting HEAD + * and drops the stash, leaving the repo in a clean state. + */ + +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"; + +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-stashpop-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"), "version: 1\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`; +} + +test("#2766: stash pop conflict on .gsd/ files is auto-resolved", () => { + const repo = createTempRepo(); + try { + const wtPath = createAutoWorktree(repo, "M300"); + + // Add a slice with real code on the milestone branch + const normalizedPath = wtPath.replaceAll("\\", "/"); + const worktreeName = normalizedPath.split("/").pop() || "M300"; + const sliceBranch = `slice/${worktreeName}/S01`; + run(`git checkout -b "${sliceBranch}"`, wtPath); + writeFileSync(join(wtPath, "feature.ts"), "export const feature = true;\n"); + + // Modify .gsd/STATE.md on the milestone branch (diverges from main) + writeFileSync(join(wtPath, ".gsd", "STATE.md"), "version: 2-milestone\n"); + run("git add .", wtPath); + run('git commit -m "add feature and update state"', wtPath); + run("git checkout milestone/M300", wtPath); + run(`git merge --no-ff "${sliceBranch}" -m "merge S01: feature"`, wtPath); + + // Dirty .gsd/STATE.md in the main repo (stash will conflict on pop) + writeFileSync(join(repo, ".gsd", "STATE.md"), "version: 2-main-dirty\n"); + + const roadmap = makeRoadmap("M300", "Stash pop conflict test", [ + { id: "S01", title: "Feature" }, + ]); + + // mergeMilestoneToMain should succeed — .gsd/ conflict auto-resolved + const result = mergeMilestoneToMain(repo, "M300", roadmap); + assert.ok( + result.commitMessage.includes("GSD-Milestone: M300"), + "merge succeeds despite stash pop conflict on .gsd/ file", + ); + assert.ok(existsSync(join(repo, "feature.ts")), "milestone code merged to main"); + + // Verify repo is clean (no UU entries blocking future merges) + const status = run("git status --porcelain", repo); + assert.ok( + !status.includes("UU "), + "no unmerged (UU) entries remain after stash pop conflict resolution", + ); + + // Stash should be dropped (no remaining stash entries) + let stashList = ""; + try { stashList = run("git stash list", repo); } catch { /* empty stash */ } + assert.strictEqual(stashList, "", "stash is empty after .gsd/ conflict auto-resolution"); + } finally { + try { rmSync(repo, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 }); } catch { /* cleanup best-effort */ } + } +}); + +test("#2766: stash pop conflict on non-.gsd files preserves stash for manual resolution", () => { + const repo = createTempRepo(); + try { + const wtPath = createAutoWorktree(repo, "M301"); + + // Add a slice that modifies a file also dirty on main + const normalizedPath = wtPath.replaceAll("\\", "/"); + const worktreeName = normalizedPath.split("/").pop() || "M301"; + const sliceBranch = `slice/${worktreeName}/S01`; + run(`git checkout -b "${sliceBranch}"`, wtPath); + writeFileSync(join(wtPath, "README.md"), "# milestone version\n"); + run("git add .", wtPath); + run('git commit -m "update readme"', wtPath); + run("git checkout milestone/M301", wtPath); + run(`git merge --no-ff "${sliceBranch}" -m "merge S01: readme"`, wtPath); + + // Dirty README.md in the main repo — this will conflict on stash pop + // and is NOT a .gsd/ file, so it should be left for manual resolution + writeFileSync(join(repo, "README.md"), "# locally modified\n"); + + const roadmap = makeRoadmap("M301", "Non-gsd stash conflict", [ + { id: "S01", title: "Readme update" }, + ]); + + // The merge itself should still succeed (stash pop conflict is non-fatal) + const result = mergeMilestoneToMain(repo, "M301", roadmap); + assert.ok( + result.commitMessage.includes("GSD-Milestone: M301"), + "merge succeeds even with non-.gsd stash pop conflict", + ); + } finally { + try { rmSync(repo, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 }); } catch { /* cleanup best-effort */ } + } +});