fix: prevent UAT stuck-loop and orphaned worktree after milestone completion (#3065)
Bug 1 -- UAT stuck-loop: syncProjectRootToWorktree used force:false for all milestone files, which preserved stale ASSESSMENT files in the worktree. When the project root had a passing verdict but the worktree retained a FAIL copy (or lost it during DB rebuild), checkNeedsRunUat found no passing verdict and re-dispatched run-uat indefinitely (x9). Fix: after the additive-only safeCopyRecursive, walk ASSESSMENT files in the project root and force-overwrite the worktree copy when the source contains a verdict field. This is safe because ASSESSMENT verdicts are only ever overwritten in a forward direction (FAIL -> PASS on retry). Bug 2 -- Orphaned worktree: removeWorktree silently swallowed failures from git worktree remove when untracked files (UAT-RESULT, ASSESSMENT) blocked removal. The .git/worktrees/<name> internal directory held a lock that also prevented the rmSync fallback from working. Fix: after both native removal attempts fail, explicitly remove the git internal .git/worktrees/<name> directory first, then retry rmSync on the worktree filesystem directory. Log a warning with manual cleanup instructions if the final attempt also fails. Closes #2821 Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
GitHub
parent
47c1b9cd7f
commit
37ec08d23c
3 changed files with 386 additions and 0 deletions
|
|
@ -102,6 +102,67 @@ function isSamePath(a: string, b: string): boolean {
|
|||
}
|
||||
}
|
||||
|
||||
// ─── ASSESSMENT Force-Sync Helper (#2821) ─────────────────────────────────
|
||||
|
||||
/** Regex matching YAML frontmatter `verdict:` field. */
|
||||
const VERDICT_RE = /verdict:\s*[\w-]+/i;
|
||||
|
||||
/**
|
||||
* Walk a milestone directory and force-overwrite ASSESSMENT files in the
|
||||
* destination when the source copy contains a `verdict:` field.
|
||||
*
|
||||
* This is the targeted fix for the UAT stuck-loop (#2821): the main
|
||||
* safeCopyRecursive uses force:false to protect worktree-authoritative
|
||||
* files (#1886), but ASSESSMENT files written by run-uat must be
|
||||
* forward-synced when the project root has a verdict. Without this,
|
||||
* the worktree retains a stale FAIL or missing ASSESSMENT and
|
||||
* checkNeedsRunUat re-dispatches run-uat indefinitely.
|
||||
*
|
||||
* Only overwrites when the source has a verdict — never clobbers a
|
||||
* worktree ASSESSMENT with a verdictless project-root copy.
|
||||
*/
|
||||
function forceOverwriteAssessmentsWithVerdict(
|
||||
srcMilestoneDir: string,
|
||||
dstMilestoneDir: string,
|
||||
): void {
|
||||
if (!existsSync(srcMilestoneDir)) return;
|
||||
|
||||
// Walk slices/<SID>/ looking for *-ASSESSMENT.md files
|
||||
const slicesDir = join(srcMilestoneDir, "slices");
|
||||
if (!existsSync(slicesDir)) return;
|
||||
|
||||
try {
|
||||
for (const sliceEntry of readdirSync(slicesDir, { withFileTypes: true })) {
|
||||
if (!sliceEntry.isDirectory()) continue;
|
||||
const srcSliceDir = join(slicesDir, sliceEntry.name);
|
||||
const dstSliceDir = join(dstMilestoneDir, "slices", sliceEntry.name);
|
||||
|
||||
try {
|
||||
for (const fileEntry of readdirSync(srcSliceDir, { withFileTypes: true })) {
|
||||
if (!fileEntry.isFile()) continue;
|
||||
if (!fileEntry.name.endsWith("-ASSESSMENT.md")) continue;
|
||||
|
||||
const srcFile = join(srcSliceDir, fileEntry.name);
|
||||
try {
|
||||
const srcContent = readFileSync(srcFile, "utf-8");
|
||||
if (!VERDICT_RE.test(srcContent)) continue; // no verdict in source — skip
|
||||
|
||||
// Source has a verdict — force-copy into worktree
|
||||
mkdirSync(dstSliceDir, { recursive: true });
|
||||
safeCopy(srcFile, join(dstSliceDir, fileEntry.name), { force: true });
|
||||
} catch {
|
||||
/* non-fatal per file */
|
||||
}
|
||||
}
|
||||
} catch {
|
||||
/* non-fatal per slice */
|
||||
}
|
||||
}
|
||||
} catch {
|
||||
/* non-fatal */
|
||||
}
|
||||
}
|
||||
|
||||
// ─── Module State ──────────────────────────────────────────────────────────
|
||||
|
||||
/** Original project root before chdir into auto-worktree. */
|
||||
|
|
@ -214,6 +275,19 @@ export function syncProjectRootToWorktree(
|
|||
{ force: false },
|
||||
);
|
||||
|
||||
// Force-sync ASSESSMENT files that have a verdict from project root (#2821).
|
||||
// The additive-only copy above preserves worktree-authoritative files, but
|
||||
// ASSESSMENT files are special: after run-uat writes a verdict and post-unit
|
||||
// syncs it to the project root, the worktree may retain a stale copy (e.g.
|
||||
// verdict:fail while the project root has verdict:pass from a retry). On
|
||||
// session resume the DB is rebuilt from disk, and if the stale ASSESSMENT
|
||||
// persists, checkNeedsRunUat finds no passing verdict → re-dispatches
|
||||
// run-uat indefinitely (stuck-loop ×9).
|
||||
forceOverwriteAssessmentsWithVerdict(
|
||||
join(prGsd, "milestones", milestoneId),
|
||||
join(wtGsd, "milestones", milestoneId),
|
||||
);
|
||||
|
||||
// Forward-sync completed-units.json from project root to worktree.
|
||||
// Project root is authoritative for completion state after crash recovery;
|
||||
// without this, the worktree re-dispatches already-completed units (#1886).
|
||||
|
|
|
|||
|
|
@ -0,0 +1,289 @@
|
|||
/**
|
||||
* uat-stuck-loop-orphaned-worktree.test.ts — Regression tests for #2821.
|
||||
*
|
||||
* Reproduces two cascading bugs:
|
||||
*
|
||||
* Bug 1 — UAT stuck-loop: syncProjectRootToWorktree uses force:false for
|
||||
* milestone files. When the project root has an ASSESSMENT with a verdict
|
||||
* but the worktree has a stale/empty ASSESSMENT (or none at all after DB
|
||||
* rebuild), the verdict is NOT synced into the worktree. checkNeedsRunUat
|
||||
* finds no verdict → re-dispatches run-uat indefinitely.
|
||||
*
|
||||
* Bug 2 — Orphaned worktree: removeWorktree silently swallows failures when
|
||||
* git worktree remove fails (untracked files, CWD inside worktree, etc.).
|
||||
* The worktree directory and branch persist on disk after teardown.
|
||||
* teardownAutoWorktree has a fallback rmSync but it also fails when the
|
||||
* git internal .git/worktrees/<name> directory holds a lock.
|
||||
*/
|
||||
|
||||
import { describe, test, beforeEach, afterEach } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import {
|
||||
mkdtempSync,
|
||||
mkdirSync,
|
||||
writeFileSync,
|
||||
rmSync,
|
||||
existsSync,
|
||||
readFileSync,
|
||||
} from "node:fs";
|
||||
import { join } from "node:path";
|
||||
import { tmpdir } from "node:os";
|
||||
import { execFileSync } from "node:child_process";
|
||||
|
||||
import { syncProjectRootToWorktree } from "../auto-worktree.ts";
|
||||
import {
|
||||
createWorktree,
|
||||
removeWorktree,
|
||||
worktreePath,
|
||||
} from "../worktree-manager.ts";
|
||||
|
||||
function git(args: string[], cwd: string): string {
|
||||
return execFileSync("git", args, {
|
||||
cwd,
|
||||
stdio: ["ignore", "pipe", "pipe"],
|
||||
encoding: "utf-8",
|
||||
}).trim();
|
||||
}
|
||||
|
||||
function makeBaseRepo(): string {
|
||||
const base = mkdtempSync(join(tmpdir(), "gsd-2821-"));
|
||||
git(["init", "-b", "main"], base);
|
||||
git(["config", "user.name", "Test"], base);
|
||||
git(["config", "user.email", "test@test.com"], base);
|
||||
writeFileSync(join(base, "README.md"), "# test\n");
|
||||
mkdirSync(join(base, ".gsd", "milestones", "M011"), { recursive: true });
|
||||
git(["add", "."], base);
|
||||
git(["commit", "-m", "init"], base);
|
||||
return base;
|
||||
}
|
||||
|
||||
// ─── Bug 1: ASSESSMENT force-sync ─────────────────────────────────────────
|
||||
|
||||
describe("#2821 Bug 1 — ASSESSMENT file force-synced on resume", () => {
|
||||
let mainBase: string;
|
||||
let wtBase: string;
|
||||
|
||||
beforeEach(() => {
|
||||
mainBase = mkdtempSync(join(tmpdir(), "gsd-2821-main-"));
|
||||
wtBase = mkdtempSync(join(tmpdir(), "gsd-2821-wt-"));
|
||||
mkdirSync(join(mainBase, ".gsd", "milestones", "M011", "slices", "S01"), {
|
||||
recursive: true,
|
||||
});
|
||||
mkdirSync(join(wtBase, ".gsd", "milestones", "M011", "slices", "S01"), {
|
||||
recursive: true,
|
||||
});
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
rmSync(mainBase, { recursive: true, force: true });
|
||||
rmSync(wtBase, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test("force-syncs ASSESSMENT with verdict from project root into worktree when worktree copy has no verdict", () => {
|
||||
// Project root has ASSESSMENT with a PASS verdict (written by run-uat, synced by post-unit)
|
||||
const prAssessment = join(
|
||||
mainBase,
|
||||
".gsd",
|
||||
"milestones",
|
||||
"M011",
|
||||
"slices",
|
||||
"S01",
|
||||
"S01-ASSESSMENT.md",
|
||||
);
|
||||
writeFileSync(
|
||||
prAssessment,
|
||||
"---\nverdict: pass\n---\n# S01 Assessment\nAll tests pass.\n",
|
||||
);
|
||||
|
||||
// Worktree has a stale ASSESSMENT with FAIL verdict (from the initial run-uat execution)
|
||||
const wtAssessment = join(
|
||||
wtBase,
|
||||
".gsd",
|
||||
"milestones",
|
||||
"M011",
|
||||
"slices",
|
||||
"S01",
|
||||
"S01-ASSESSMENT.md",
|
||||
);
|
||||
writeFileSync(
|
||||
wtAssessment,
|
||||
"---\nverdict: fail\n---\n# S01 Assessment\nSome tests fail.\n",
|
||||
);
|
||||
|
||||
syncProjectRootToWorktree(mainBase, wtBase, "M011");
|
||||
|
||||
// The worktree ASSESSMENT must now have the project root's PASS verdict
|
||||
const content = readFileSync(wtAssessment, "utf-8");
|
||||
assert.ok(
|
||||
content.includes("verdict: pass"),
|
||||
`Expected worktree ASSESSMENT to have verdict:pass after sync, got: ${content.slice(0, 100)}`,
|
||||
);
|
||||
});
|
||||
|
||||
test("force-syncs ASSESSMENT from project root when worktree has no ASSESSMENT at all", () => {
|
||||
// Project root has ASSESSMENT with verdict
|
||||
const prAssessment = join(
|
||||
mainBase,
|
||||
".gsd",
|
||||
"milestones",
|
||||
"M011",
|
||||
"slices",
|
||||
"S01",
|
||||
"S01-ASSESSMENT.md",
|
||||
);
|
||||
writeFileSync(
|
||||
prAssessment,
|
||||
"---\nverdict: pass\n---\n# S01 Assessment\n",
|
||||
);
|
||||
|
||||
// Worktree has NO ASSESSMENT (deleted during DB rebuild)
|
||||
// — file simply doesn't exist
|
||||
|
||||
syncProjectRootToWorktree(mainBase, wtBase, "M011");
|
||||
|
||||
const wtAssessment = join(
|
||||
wtBase,
|
||||
".gsd",
|
||||
"milestones",
|
||||
"M011",
|
||||
"slices",
|
||||
"S01",
|
||||
"S01-ASSESSMENT.md",
|
||||
);
|
||||
assert.ok(
|
||||
existsSync(wtAssessment),
|
||||
"ASSESSMENT should be copied to worktree when missing",
|
||||
);
|
||||
const content = readFileSync(wtAssessment, "utf-8");
|
||||
assert.ok(
|
||||
content.includes("verdict: pass"),
|
||||
`Synced ASSESSMENT should contain verdict:pass, got: ${content.slice(0, 100)}`,
|
||||
);
|
||||
});
|
||||
|
||||
test("does NOT overwrite worktree ASSESSMENT when project root has no verdict", () => {
|
||||
// Project root has ASSESSMENT without verdict (incomplete)
|
||||
const prAssessment = join(
|
||||
mainBase,
|
||||
".gsd",
|
||||
"milestones",
|
||||
"M011",
|
||||
"slices",
|
||||
"S01",
|
||||
"S01-ASSESSMENT.md",
|
||||
);
|
||||
writeFileSync(prAssessment, "# S01 Assessment\nIn progress...\n");
|
||||
|
||||
// Worktree has ASSESSMENT with verdict:fail
|
||||
const wtAssessment = join(
|
||||
wtBase,
|
||||
".gsd",
|
||||
"milestones",
|
||||
"M011",
|
||||
"slices",
|
||||
"S01",
|
||||
"S01-ASSESSMENT.md",
|
||||
);
|
||||
writeFileSync(
|
||||
wtAssessment,
|
||||
"---\nverdict: fail\n---\n# S01 Assessment\nSome tests fail.\n",
|
||||
);
|
||||
|
||||
syncProjectRootToWorktree(mainBase, wtBase, "M011");
|
||||
|
||||
// Worktree copy should NOT be overwritten by the verdictless project root copy
|
||||
const content = readFileSync(wtAssessment, "utf-8");
|
||||
assert.ok(
|
||||
content.includes("verdict: fail"),
|
||||
`Worktree ASSESSMENT should keep verdict:fail when project root has no verdict, got: ${content.slice(0, 100)}`,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── Bug 2: Orphaned worktree cleanup ─────────────────────────────────────
|
||||
|
||||
describe("#2821 Bug 2 — removeWorktree cleans up despite untracked files", () => {
|
||||
let base: string;
|
||||
|
||||
beforeEach(() => {
|
||||
base = makeBaseRepo();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
rmSync(base, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test("removes worktree directory even when it contains untracked files", () => {
|
||||
const info = createWorktree(base, "M011", {
|
||||
branch: "milestone/M011",
|
||||
});
|
||||
|
||||
// Simulate run-uat writing untracked files (S01-UAT-RESULT.md, ASSESSMENT)
|
||||
mkdirSync(
|
||||
join(info.path, ".gsd", "milestones", "M011", "slices", "S01"),
|
||||
{ recursive: true },
|
||||
);
|
||||
writeFileSync(
|
||||
join(
|
||||
info.path,
|
||||
".gsd",
|
||||
"milestones",
|
||||
"M011",
|
||||
"slices",
|
||||
"S01",
|
||||
"S01-UAT-RESULT.md",
|
||||
),
|
||||
"# UAT Result\nverdict: fail\n",
|
||||
);
|
||||
writeFileSync(
|
||||
join(
|
||||
info.path,
|
||||
".gsd",
|
||||
"milestones",
|
||||
"M011",
|
||||
"slices",
|
||||
"S01",
|
||||
"S01-ASSESSMENT.md",
|
||||
),
|
||||
"---\nverdict: fail\n---\n# Assessment\n",
|
||||
);
|
||||
|
||||
removeWorktree(base, "M011", {
|
||||
branch: "milestone/M011",
|
||||
deleteBranch: true,
|
||||
force: true,
|
||||
});
|
||||
|
||||
const wtDir = worktreePath(base, "M011");
|
||||
assert.ok(
|
||||
!existsSync(wtDir),
|
||||
`Worktree directory should be removed after teardown, but still exists at ${wtDir}`,
|
||||
);
|
||||
});
|
||||
|
||||
test("removes git internal worktree metadata after filesystem removal", () => {
|
||||
createWorktree(base, "M011", {
|
||||
branch: "milestone/M011",
|
||||
});
|
||||
|
||||
removeWorktree(base, "M011", {
|
||||
branch: "milestone/M011",
|
||||
deleteBranch: true,
|
||||
force: true,
|
||||
});
|
||||
|
||||
// The git internal worktree directory should be cleaned up
|
||||
const gitInternalWorktreeDir = join(base, ".git", "worktrees", "M011");
|
||||
assert.ok(
|
||||
!existsSync(gitInternalWorktreeDir),
|
||||
`Git internal worktree dir should be removed: ${gitInternalWorktreeDir}`,
|
||||
);
|
||||
|
||||
// The branch should be deleted
|
||||
const branches = git(["branch"], base);
|
||||
assert.ok(
|
||||
!branches.includes("milestone/M011"),
|
||||
"milestone/M011 branch should be deleted after removeWorktree",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
@ -365,6 +365,29 @@ export function removeWorktree(
|
|||
try { nativeWorktreeRemove(basePath, resolvedWtPath, true); } catch { /* may fail */ }
|
||||
}
|
||||
|
||||
// (#2821) If the worktree directory STILL exists after both native removal
|
||||
// attempts (e.g. untracked files like ASSESSMENT/UAT-RESULT prevent git
|
||||
// worktree remove), force-remove the git internal worktree metadata first,
|
||||
// then remove the filesystem directory. Without this, the .git/worktrees/<name>
|
||||
// lock prevents rmSync from cleaning up, and the orphaned worktree directory
|
||||
// causes every subsequent `/gsd auto` to re-enter the stale worktree.
|
||||
if (existsSync(resolvedWtPath)) {
|
||||
try {
|
||||
const wtInternalDir = join(basePath, ".git", "worktrees", name);
|
||||
if (existsSync(wtInternalDir)) {
|
||||
rmSync(wtInternalDir, { recursive: true, force: true });
|
||||
}
|
||||
rmSync(resolvedWtPath, { recursive: true, force: true });
|
||||
} catch {
|
||||
logWarning(
|
||||
"reconcile",
|
||||
`Worktree directory could not be removed after git internal cleanup: ${resolvedWtPath}. ` +
|
||||
`Manual cleanup: rm -rf "${resolvedWtPath.replaceAll("\\", "/")}"`,
|
||||
{ worktree: name },
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// Prune stale entries so git knows the worktree is gone
|
||||
nativeWorktreePrune(basePath);
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue