fix: guard autoCommitDirtyState and restore cwd on MergeConflictError (#2929)
Three fixes to prevent cross-milestone contamination during parallel merge: 1. autoCommitDirtyState branch guard: only auto-commit when cwd is on the milestone branch, not the integration branch. Prevents committing dirty files from other milestones onto main. 2. process.chdir(previousCwd) before MergeConflictError throw: restores cwd so the next merge in a parallel sequence doesn't inherit a leaked cwd. 3. Pre-teardown auto-commit branch guard: skip the safety-net commit when cwd is on the integration branch, not the milestone branch. Supersedes #3130.
This commit is contained in:
Jeremy
parent
0c37a88024
commit
f6f15ddec6
2 changed files with 236 additions and 15 deletions
|
|
@ -1420,8 +1420,31 @@ export function mergeMilestoneToMain(
|
|||
const worktreeCwd = process.cwd();
|
||||
const milestoneBranch = autoWorktreeBranch(milestoneId);
|
||||
|
||||
// 1. Auto-commit dirty state in worktree before leaving
|
||||
autoCommitDirtyState(worktreeCwd);
|
||||
// 1. Auto-commit dirty state before leaving.
|
||||
// Guard: when we entered through an auto-worktree (originalBase is set),
|
||||
// only auto-commit when cwd is on the milestone branch. In parallel mode,
|
||||
// cwd may be on the integration branch after a prior merge's
|
||||
// MergeConflictError left cwd unrestored. Auto-committing on the
|
||||
// integration branch captures dirty files from OTHER milestones under a
|
||||
// misleading commit message, contaminating the main branch (#2929).
|
||||
//
|
||||
// When originalBase is null (branch mode, no worktree), autoCommitDirtyState
|
||||
// runs unconditionally — the caller is responsible for cwd placement.
|
||||
{
|
||||
let shouldAutoCommit = true;
|
||||
if (originalBase !== null) {
|
||||
try {
|
||||
const currentBranch = nativeGetCurrentBranch(worktreeCwd);
|
||||
shouldAutoCommit = currentBranch === milestoneBranch;
|
||||
} catch {
|
||||
// If we can't determine the branch, skip the auto-commit to be safe
|
||||
shouldAutoCommit = false;
|
||||
}
|
||||
}
|
||||
if (shouldAutoCommit) {
|
||||
autoCommitDirtyState(worktreeCwd);
|
||||
}
|
||||
}
|
||||
|
||||
// Reconcile worktree DB into main DB before leaving worktree context.
|
||||
// Skip when both paths resolve to the same physical file (shared WAL /
|
||||
|
|
@ -1778,6 +1801,12 @@ export function mergeMilestoneToMain(
|
|||
}
|
||||
}
|
||||
restoreShelter();
|
||||
// Restore cwd so the caller is not stranded on the integration branch.
|
||||
// Without this, the next mergeMilestoneToMain call in a parallel merge
|
||||
// sequence uses process.cwd() (now the project root) as worktreeCwd,
|
||||
// causing autoCommitDirtyState to commit unrelated milestone files to
|
||||
// the integration branch (#2929).
|
||||
process.chdir(previousCwd);
|
||||
throw new MergeConflictError(
|
||||
codeConflicts,
|
||||
"squash",
|
||||
|
|
@ -1975,23 +2004,34 @@ export function mergeMilestoneToMain(
|
|||
// 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`.
|
||||
//
|
||||
// Guard: only run when worktreeCwd is on the milestone branch (#2929).
|
||||
// In parallel mode or branch-mode merges, worktreeCwd may be the project
|
||||
// root on the integration branch. Committing dirty state there would
|
||||
// capture unrelated files from other milestones.
|
||||
if (existsSync(worktreeCwd)) {
|
||||
try {
|
||||
const dirtyCheck = nativeWorkingTreeStatus(worktreeCwd);
|
||||
if (dirtyCheck) {
|
||||
let preTeardownBranch: string | null = null;
|
||||
try { preTeardownBranch = nativeGetCurrentBranch(worktreeCwd); } catch { /* */ }
|
||||
const isOnMilestoneBranch = preTeardownBranch === milestoneBranch;
|
||||
|
||||
if (isOnMilestoneBranch) {
|
||||
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-dirty",
|
||||
worktreeCwd,
|
||||
status: dirtyCheck.slice(0, 200),
|
||||
phase: "pre-teardown-commit-error",
|
||||
error: String(e),
|
||||
});
|
||||
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),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,181 @@
|
|||
/**
|
||||
* merge-cwd-restore.test.ts — Regression tests for #2929.
|
||||
*
|
||||
* Verifies:
|
||||
* 1. MergeConflictError restores process.cwd() to the pre-merge directory.
|
||||
* 2. autoCommitDirtyState does not run on the integration branch when cwd
|
||||
* leaked there from a prior failed merge (parallel mode).
|
||||
*
|
||||
* Bug: PR #2298 added a stash lifecycle around mergeMilestoneToMain but the
|
||||
* MergeConflictError throw path omitted the process.chdir(previousCwd) that
|
||||
* the dirty-working-tree and divergence handlers both include. In parallel
|
||||
* merge sequences, this left cwd on the integration branch, causing the next
|
||||
* merge's autoCommitDirtyState to commit dirty files from OTHER milestones
|
||||
* onto main.
|
||||
*/
|
||||
|
||||
import test from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import {
|
||||
mkdtempSync,
|
||||
mkdirSync,
|
||||
writeFileSync,
|
||||
rmSync,
|
||||
realpathSync,
|
||||
} from "node:fs";
|
||||
import { join } from "node:path";
|
||||
import { tmpdir } from "node:os";
|
||||
import { execSync } from "node:child_process";
|
||||
|
||||
import { mergeMilestoneToMain } from "../../auto-worktree.ts";
|
||||
import { MergeConflictError } from "../../git-service.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(), "merge-cwd-restore-test-")),
|
||||
);
|
||||
run("git init -b main", dir);
|
||||
run("git config user.email test@test.com", dir);
|
||||
run("git config user.name Test", dir);
|
||||
writeFileSync(join(dir, "README.md"), "# test\n");
|
||||
writeFileSync(join(dir, ".gitignore"), ".gsd/worktrees/\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);
|
||||
return dir;
|
||||
}
|
||||
|
||||
function cleanup(dir: string): void {
|
||||
try {
|
||||
rmSync(dir, { recursive: true, force: true });
|
||||
} catch {
|
||||
/* */
|
||||
}
|
||||
}
|
||||
|
||||
function makeRoadmap(mid: string, title: string): string {
|
||||
return `# ${mid}: ${title}\n\n## Slices\n- [x] **S01: Test slice**\n`;
|
||||
}
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// Test 1: MergeConflictError restores cwd (#2929 bug 2)
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
test("#2929 bug 2 — MergeConflictError restores cwd to pre-merge directory", () => {
|
||||
const savedCwd = process.cwd();
|
||||
const repo = createTempRepo();
|
||||
|
||||
try {
|
||||
// Create milestone branch that modifies README.md
|
||||
run("git checkout -b milestone/M010", repo);
|
||||
writeFileSync(join(repo, "README.md"), "# M010 version\n");
|
||||
run("git add .", repo);
|
||||
run('git commit -m "M010 changes README"', repo);
|
||||
run("git checkout main", repo);
|
||||
|
||||
// Modify README.md on main to create a conflict
|
||||
writeFileSync(join(repo, "README.md"), "# main version (diverged)\n");
|
||||
run("git add .", repo);
|
||||
run('git commit -m "main diverges README"', repo);
|
||||
|
||||
// cwd must be repo root (simulates parallel-merge calling from project root)
|
||||
process.chdir(repo);
|
||||
const cwdBefore = process.cwd();
|
||||
|
||||
let caught: unknown = null;
|
||||
try {
|
||||
mergeMilestoneToMain(repo, "M010", makeRoadmap("M010", "Conflict test"));
|
||||
} catch (err) {
|
||||
caught = err;
|
||||
}
|
||||
|
||||
// Should have thrown a MergeConflictError
|
||||
assert.ok(caught instanceof MergeConflictError, "expected MergeConflictError");
|
||||
|
||||
// Critical: cwd must be restored to where it was before the merge
|
||||
const cwdAfter = process.cwd();
|
||||
assert.equal(
|
||||
cwdAfter,
|
||||
cwdBefore,
|
||||
"cwd should be restored after MergeConflictError — was left on integration branch before fix",
|
||||
);
|
||||
} finally {
|
||||
process.chdir(savedCwd);
|
||||
try {
|
||||
run("git reset --hard HEAD", repo);
|
||||
} catch {
|
||||
/* */
|
||||
}
|
||||
cleanup(repo);
|
||||
}
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// Test 2: autoCommitDirtyState skipped when on integration branch (#2929 bug 1)
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
test("#2929 bug 1 — autoCommitDirtyState does not commit on integration branch in worktree mode", () => {
|
||||
const savedCwd = process.cwd();
|
||||
const repo = createTempRepo();
|
||||
|
||||
try {
|
||||
// Create milestone branch with real work
|
||||
run("git checkout -b milestone/M010", repo);
|
||||
writeFileSync(join(repo, "m010.ts"), "export const m010 = true;\n");
|
||||
run("git add .", repo);
|
||||
run('git commit -m "M010 work"', repo);
|
||||
run("git checkout main", repo);
|
||||
|
||||
// Simulate the parallel-mode state: cwd is on main with dirty files
|
||||
// from another milestone (as if a prior merge's MergeConflictError
|
||||
// left cwd on main and syncStateToProjectRoot wrote these files).
|
||||
writeFileSync(join(repo, "dirty-from-m020.txt"), "should not be committed\n");
|
||||
|
||||
// Set up roadmap so mergeMilestoneToMain can find milestone metadata
|
||||
mkdirSync(join(repo, ".gsd", "milestones", "M010"), { recursive: true });
|
||||
writeFileSync(
|
||||
join(repo, ".gsd", "milestones", "M010", "M010-ROADMAP.md"),
|
||||
makeRoadmap("M010", "First milestone"),
|
||||
);
|
||||
|
||||
process.chdir(repo);
|
||||
|
||||
const result = mergeMilestoneToMain(
|
||||
repo,
|
||||
"M010",
|
||||
makeRoadmap("M010", "First milestone"),
|
||||
);
|
||||
|
||||
assert.ok(result.commitMessage.includes("M010"), "commit should be for M010");
|
||||
|
||||
// Verify the squash merge brought M010's work file
|
||||
const mergeLog = run("git log --oneline --diff-filter=A -- m010.ts", repo);
|
||||
assert.ok(mergeLog.length > 0, "m010.ts should be in a commit on main");
|
||||
|
||||
// The dirty file should NOT appear in the squash merge commit.
|
||||
const squashCommit = run("git log --format=%H --grep='GSD-Milestone: M010' -1", repo);
|
||||
assert.ok(squashCommit.length > 0, "should find the squash merge commit");
|
||||
const filesInSquash = run(`git diff-tree --no-commit-id --name-only -r ${squashCommit}`, repo);
|
||||
assert.ok(
|
||||
!filesInSquash.includes("dirty-from-m020.txt"),
|
||||
"dirty-from-m020.txt should NOT be in the squash merge commit",
|
||||
);
|
||||
} finally {
|
||||
process.chdir(savedCwd);
|
||||
try {
|
||||
run("git reset --hard HEAD", repo);
|
||||
} catch {
|
||||
/* */
|
||||
}
|
||||
cleanup(repo);
|
||||
}
|
||||
});
|
||||
Loading…
Add table
Reference in a new issue