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) <noreply@anthropic.com> * test: add regression tests for stash pop conflict and error handling Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: use correct LogComponent type in stash pop handler Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: join file array for logWarning in stash pop handler Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
TÂCHES
GitHub
parent
f4736f47ae
commit
9823fd2d2d
4 changed files with 291 additions and 3 deletions
|
|
@ -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(", "),
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
@ -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 */ }
|
||||
}
|
||||
});
|
||||
Loading…
Add table
Reference in a new issue