diff --git a/src/resources/extensions/gsd/auto-dispatch.ts b/src/resources/extensions/gsd/auto-dispatch.ts index 1cc2f28a0..2e27f77d8 100644 --- a/src/resources/extensions/gsd/auto-dispatch.ts +++ b/src/resources/extensions/gsd/auto-dispatch.ts @@ -767,13 +767,17 @@ export const DISPATCH_RULES: DispatchRule[] = [ // Safety guard (#1703): verify the milestone produced implementation // artifacts (non-.gsd/ files). A milestone with only plan files and // zero implementation code should not be marked complete. - if (!hasImplementationArtifacts(basePath)) { + const artifactCheck = hasImplementationArtifacts(basePath); + if (artifactCheck === "absent") { return { action: "stop", reason: `Cannot complete milestone ${mid}: no implementation files found outside .gsd/. The milestone has only plan files — actual code changes are required.`, level: "error", }; } + if (artifactCheck === "unknown") { + logWarning("dispatch", `Implementation artifact check inconclusive for ${mid} — proceeding (git context unavailable)`); + } // Verification class compliance: if operational verification was planned, // ensure the validation output documents it before allowing completion. diff --git a/src/resources/extensions/gsd/auto-recovery.ts b/src/resources/extensions/gsd/auto-recovery.ts index 1557432df..5fa4a2d1d 100644 --- a/src/resources/extensions/gsd/auto-recovery.ts +++ b/src/resources/extensions/gsd/auto-recovery.ts @@ -61,13 +61,12 @@ export { resolveExpectedArtifactPath, diagnoseExpectedArtifact }; * in the git history. Uses `git log --name-only` to inspect all commits on the * current branch that touch files outside `.gsd/`. * - * Returns true if at least one non-`.gsd/` file was committed, false otherwise. - * Non-fatal: returns true on git errors to avoid blocking the pipeline when - * running outside a git repo (e.g., tests). + * Returns "present" if implementation files found, "absent" if only .gsd/ files, + * "unknown" if git is unavailable or check failed (callers decide how to handle). */ -export function hasImplementationArtifacts(basePath: string): boolean { +export function hasImplementationArtifacts(basePath: string): "present" | "absent" | "unknown" { try { - // Verify we're in a git repo — fail open if not + // Verify we're in a git repo try { execFileSync("git", ["rev-parse", "--is-inside-work-tree"], { cwd: basePath, @@ -76,7 +75,7 @@ export function hasImplementationArtifacts(basePath: string): boolean { }); } catch (e) { logWarning("recovery", `git rev-parse check failed: ${(e as Error).message}`); - return true; + return "unknown"; } // Strategy: check `git diff --name-only` against the merge-base with the @@ -86,19 +85,19 @@ export function hasImplementationArtifacts(basePath: string): boolean { const mainBranch = detectMainBranch(basePath); const changedFiles = getChangedFilesSinceBranch(basePath, mainBranch); - // No files changed at all — fail open (could be detached HEAD, single- + // No files changed at all — unknown (could be detached HEAD, single- // commit repo, or other edge case where git diff returns nothing). - if (changedFiles.length === 0) return true; + if (changedFiles.length === 0) return "unknown"; // Filter out .gsd/ files — only implementation files count. // If every changed file is under .gsd/, the milestone produced no // implementation code (#1703). const implFiles = changedFiles.filter(f => !f.startsWith(".gsd/") && !f.startsWith(".gsd\\")); - return implFiles.length > 0; + return implFiles.length > 0 ? "present" : "absent"; } catch (e) { - // Non-fatal — if git operations fail, don't block the pipeline + // Non-fatal — if git operations fail, return unknown so callers can decide logWarning("recovery", `implementation artifact check failed: ${(e as Error).message}`); - return true; + return "unknown"; } } @@ -395,7 +394,7 @@ export function verifyExpectedArtifact( // A milestone with only .gsd/ plan files and zero implementation code is // not genuinely complete — the LLM wrote plan files but skipped actual work. if (unitType === "complete-milestone") { - if (!hasImplementationArtifacts(base)) return false; + if (hasImplementationArtifacts(base) === "absent") return false; } return true; @@ -502,7 +501,7 @@ export function reconcileMergeState( if (conflictedFiles.length === 0) { // All conflicts resolved — finalize the merge/squash commit try { - const commitSha = nativeCommit(basePath, ""); // --no-edit equivalent: use empty message placeholder + const commitSha = nativeCommit(basePath, "chore(gsd): reconcile merge state"); if (commitSha) { const mode = hasMergeHead ? "merge" : "squash commit"; ctx.ui.notify(`Finalized leftover ${mode} from prior session.`, "info"); diff --git a/src/resources/extensions/gsd/auto-start.ts b/src/resources/extensions/gsd/auto-start.ts index fcc1be0ab..7870993ce 100644 --- a/src/resources/extensions/gsd/auto-start.ts +++ b/src/resources/extensions/gsd/auto-start.ts @@ -102,11 +102,8 @@ export interface BootstrapDeps { * concurrent session detected). Returns true when ready to dispatch. */ -/** Guard: tracks consecutive bootstrap attempts that found phase === "complete". - * Prevents the recursive dialog loop described in #1348 where - * bootstrapAutoSession → showSmartEntry → checkAutoStartAfterDiscuss → startAuto - * cycles indefinitely when the discuss workflow doesn't produce a milestone. */ -let _consecutiveCompleteBootstraps = 0; +// Guard constant for consecutive bootstrap attempts that found phase === "complete". +// Counter moved to AutoSession.consecutiveCompleteBootstraps so s.reset() clears it. const MAX_CONSECUTIVE_COMPLETE_BOOTSTRAPS = 2; export async function openProjectDbIfPresent(basePath: string): Promise { @@ -392,9 +389,9 @@ export async function bootstrapAutoSession( // Guard against recursive dialog loop (#1348): // If we've entered this branch multiple times in quick succession, // the discuss workflow isn't producing a milestone. Break the cycle. - _consecutiveCompleteBootstraps++; - if (_consecutiveCompleteBootstraps > MAX_CONSECUTIVE_COMPLETE_BOOTSTRAPS) { - _consecutiveCompleteBootstraps = 0; + s.consecutiveCompleteBootstraps++; + if (s.consecutiveCompleteBootstraps > MAX_CONSECUTIVE_COMPLETE_BOOTSTRAPS) { + s.consecutiveCompleteBootstraps = 0; ctx.ui.notify( "All milestones are complete and the discussion didn't produce a new one. " + "Run /gsd to start a new milestone manually.", @@ -413,7 +410,7 @@ export async function bootstrapAutoSession( postState.phase !== "complete" && postState.phase !== "pre-planning" ) { - _consecutiveCompleteBootstraps = 0; // Successfully advanced past "complete" + s.consecutiveCompleteBootstraps = 0; // Successfully advanced past "complete" state = postState; } else if ( postState.activeMilestone && @@ -492,7 +489,7 @@ export async function bootstrapAutoSession( } // Successfully resolved an active milestone — reset the re-entry guard - _consecutiveCompleteBootstraps = 0; + s.consecutiveCompleteBootstraps = 0; // ── Initialize session state ── s.active = true; diff --git a/src/resources/extensions/gsd/auto.ts b/src/resources/extensions/gsd/auto.ts index 081dd493c..a2e5c09d7 100644 --- a/src/resources/extensions/gsd/auto.ts +++ b/src/resources/extensions/gsd/auto.ts @@ -1141,9 +1141,9 @@ export async function startAuto( s.stepMode = meta.stepMode ?? requestedStepMode; s.autoStartTime = meta.autoStartTime || Date.now(); s.paused = true; - try { unlinkSync(pausedPath); } catch (err) { /* non-fatal */ - logWarning("session", `pause file cleanup failed: ${err instanceof Error ? err.message : String(err)}`, { file: "auto.ts" }); - } + // Don't delete pause file yet — defer until lock is acquired. + // If lock fails, the file must survive for retry. + s.pausedSessionFile = pausedPath; ctx.ui.notify( `Resuming paused custom workflow${meta.activeRunDir ? ` (${meta.activeRunDir})` : ""}.`, "info", @@ -1167,10 +1167,9 @@ export async function startAuto( s.stepMode = meta.stepMode ?? requestedStepMode; s.autoStartTime = meta.autoStartTime || Date.now(); s.paused = true; - // Clean up the persisted file — we're consuming it - try { unlinkSync(pausedPath); } catch (err) { /* non-fatal */ - logWarning("session", `pause file cleanup failed: ${err instanceof Error ? err.message : String(err)}`, { file: "auto.ts" }); - } + // Don't delete pause file yet — defer until lock is acquired. + // If lock fails, the file must survive for retry. + s.pausedSessionFile = pausedPath; ctx.ui.notify( `Resuming paused session for ${meta.milestoneId}${meta.worktreePath ? ` (worktree)` : ""}.`, "info", @@ -1187,10 +1186,21 @@ export async function startAuto( if (s.paused) { const resumeLock = acquireSessionLock(base); if (!resumeLock.acquired) { + // Reset paused state so isAutoPaused() doesn't stick true after lock failure. + // Pause file is preserved on disk for retry — not deleted. + s.paused = false; ctx.ui.notify(`Cannot resume: ${resumeLock.reason}`, "error"); return; } + // Lock acquired — now safe to delete the pause file + if (s.pausedSessionFile) { + try { unlinkSync(s.pausedSessionFile); } catch (err) { + logWarning("session", `pause file cleanup failed: ${err instanceof Error ? err.message : String(err)}`, { file: "auto.ts" }); + } + s.pausedSessionFile = null; + } + s.paused = false; s.active = true; s.verbose = verboseMode; diff --git a/src/resources/extensions/gsd/auto/session.ts b/src/resources/extensions/gsd/auto/session.ts index 7cb991511..b191d8591 100644 --- a/src/resources/extensions/gsd/auto/session.ts +++ b/src/resources/extensions/gsd/auto/session.ts @@ -138,6 +138,9 @@ export class AutoSession { // ── Dispatch circuit breakers ────────────────────────────────────── rewriteAttemptCount = 0; + /** Tracks consecutive bootstrap attempts that found phase === "complete". + * Moved from module-level to per-session so s.reset() clears it (#1348). */ + consecutiveCompleteBootstraps = 0; // ── Metrics ────────────────────────────────────────────────────────────── autoStartTime = 0; @@ -224,6 +227,7 @@ export class AutoSession { this.pendingQuickTasks = []; this.sidecarQueue = []; this.rewriteAttemptCount = 0; + this.consecutiveCompleteBootstraps = 0; this.lastToolInvocationError = null; this.isolationDegraded = false; this.milestoneMergedInPhases = false; diff --git a/src/resources/extensions/gsd/db-writer.ts b/src/resources/extensions/gsd/db-writer.ts index b5165ff77..14bcb75b5 100644 --- a/src/resources/extensions/gsd/db-writer.ts +++ b/src/resources/extensions/gsd/db-writer.ts @@ -345,8 +345,12 @@ export async function saveRequirementToDb( await saveFile(filePath, md); } catch (diskErr) { logError('manifest', 'disk write failed, rolling back DB row', { fn: 'saveRequirementToDb', error: String((diskErr as Error).message) }); - const rollbackAdapter = db._getAdapter(); - rollbackAdapter?.prepare('DELETE FROM requirements WHERE id = :id').run({ ':id': id }); + try { + const rollbackAdapter = db._getAdapter(); + rollbackAdapter?.prepare('DELETE FROM requirements WHERE id = :id').run({ ':id': id }); + } catch (rollbackErr) { + logError('manifest', 'SPLIT BRAIN: disk write failed AND DB rollback failed — DB has orphaned row', { fn: 'saveRequirementToDb', id, error: String((rollbackErr as Error).message) }); + } throw diskErr; } invalidateStateCache(); @@ -466,7 +470,11 @@ export async function saveDecisionToDb( await saveFile(filePath, md); } catch (diskErr) { logError('manifest', 'disk write failed, rolling back DB row', { fn: 'saveDecisionToDb', error: String((diskErr as Error).message) }); - adapter?.prepare('DELETE FROM decisions WHERE id = :id').run({ ':id': id }); + try { + adapter?.prepare('DELETE FROM decisions WHERE id = :id').run({ ':id': id }); + } catch (rollbackErr) { + logError('manifest', 'SPLIT BRAIN: disk write failed AND DB rollback failed — DB has orphaned row', { fn: 'saveDecisionToDb', id, error: String((rollbackErr as Error).message) }); + } throw diskErr; } // #2661: When a decision defers a slice, update the slice status in the DB diff --git a/src/resources/extensions/gsd/tests/integration/auto-recovery.test.ts b/src/resources/extensions/gsd/tests/integration/auto-recovery.test.ts index 11f5a3d48..610f4d442 100644 --- a/src/resources/extensions/gsd/tests/integration/auto-recovery.test.ts +++ b/src/resources/extensions/gsd/tests/integration/auto-recovery.test.ts @@ -684,7 +684,7 @@ function makeGitBase(): string { return base; } -test("hasImplementationArtifacts returns false when only .gsd/ files committed (#1703)", (t) => { +test("hasImplementationArtifacts returns 'absent' when only .gsd/ files committed (#1703)", (t) => { const base = makeGitBase(); t.after(() => cleanup(base)); @@ -697,10 +697,10 @@ test("hasImplementationArtifacts returns false when only .gsd/ files committed ( execFileSync("git", ["commit", "-m", "chore: add plan files"], { cwd: base, stdio: "ignore" }); const result = hasImplementationArtifacts(base); - assert.equal(result, false, "should return false when only .gsd/ files were committed"); + assert.equal(result, "absent", "should return 'absent' when only .gsd/ files were committed"); }); -test("hasImplementationArtifacts returns true when implementation files committed (#1703)", (t) => { +test("hasImplementationArtifacts returns 'present' when implementation files committed (#1703)", (t) => { const base = makeGitBase(); t.after(() => cleanup(base)); @@ -714,16 +714,16 @@ test("hasImplementationArtifacts returns true when implementation files committe execFileSync("git", ["commit", "-m", "feat: add feature"], { cwd: base, stdio: "ignore" }); const result = hasImplementationArtifacts(base); - assert.equal(result, true, "should return true when implementation files are present"); + assert.equal(result, "present", "should return 'present' when implementation files are present"); }); -test("hasImplementationArtifacts returns true on non-git directory (fail-open)", (t) => { +test("hasImplementationArtifacts returns 'unknown' on non-git directory (fail-open)", (t) => { const base = join(tmpdir(), `gsd-test-nogit-${randomUUID()}`); mkdirSync(base, { recursive: true }); t.after(() => cleanup(base)); const result = hasImplementationArtifacts(base); - assert.equal(result, true, "should return true (fail-open) in non-git directory"); + assert.equal(result, "unknown", "should return 'unknown' (fail-open) in non-git directory"); }); // ─── verifyExpectedArtifact: complete-milestone requires impl artifacts (#1703) ── diff --git a/src/resources/extensions/gsd/tests/wave3-session-regressions.test.ts b/src/resources/extensions/gsd/tests/wave3-session-regressions.test.ts new file mode 100644 index 000000000..821f79aa1 --- /dev/null +++ b/src/resources/extensions/gsd/tests/wave3-session-regressions.test.ts @@ -0,0 +1,47 @@ +// GSD State Machine — Wave 3 Session Regression Tests +// Validates tri-state hasImplementationArtifacts and AutoSession.consecutiveCompleteBootstraps. + +import { describe, test } from "node:test"; +import assert from "node:assert/strict"; +import { hasImplementationArtifacts } from "../auto-recovery.js"; +import { AutoSession } from "../auto/session.js"; + +// ── Fix 9: hasImplementationArtifacts returns tri-state ── + +describe("hasImplementationArtifacts tri-state return", () => { + test("returns 'unknown' for non-git directory", () => { + const result = hasImplementationArtifacts("/tmp/not-a-git-repo-" + Date.now()); + assert.strictEqual(result, "unknown"); + }); + + test("return type is one of present/absent/unknown", () => { + const result = hasImplementationArtifacts(process.cwd()); + assert.ok( + result === "present" || result === "absent" || result === "unknown", + `Expected present/absent/unknown, got: ${result}`, + ); + }); +}); + +// ── Fix 11: consecutiveCompleteBootstraps is per-session ── + +describe("AutoSession.consecutiveCompleteBootstraps", () => { + test("initial value is 0", () => { + const s = new AutoSession(); + assert.strictEqual(s.consecutiveCompleteBootstraps, 0); + }); + + test("reset() clears the counter", () => { + const s = new AutoSession(); + s.consecutiveCompleteBootstraps = 5; + s.reset(); + assert.strictEqual(s.consecutiveCompleteBootstraps, 0); + }); + + test("two sessions have independent counters", () => { + const s1 = new AutoSession(); + const s2 = new AutoSession(); + s1.consecutiveCompleteBootstraps = 3; + assert.strictEqual(s2.consecutiveCompleteBootstraps, 0); + }); +}); diff --git a/src/resources/extensions/gsd/tests/workflow-logger-audit.test.ts b/src/resources/extensions/gsd/tests/workflow-logger-audit.test.ts index 1859bf1d8..1c63b60bd 100644 --- a/src/resources/extensions/gsd/tests/workflow-logger-audit.test.ts +++ b/src/resources/extensions/gsd/tests/workflow-logger-audit.test.ts @@ -90,18 +90,21 @@ describe("workflow-logger audit persistence", () => { assert.ok(ctx, "context should exist"); assert.equal(ctx.fn, "saveDecisionToDb"); assert.equal(ctx.tool, "gsd_decision_save"); - assert.equal(ctx.error, undefined, "error key must be stripped from persisted context"); + assert.equal(ctx.error, "SQLITE_BUSY: database is locked", "error key should be preserved in persisted context"); assert.equal(ctx.file, undefined, "file key must be stripped from persisted context"); }); - test("persisted errors omit context when no safe keys present", () => { + test("persisted errors preserve error key but strip other unsafe keys", () => { logError("bootstrap", "ensureDbOpen failed", { error: "ENOENT", cwd: "/home/user/project", }); const lines = readAuditLines(tmp); assert.equal(lines.length, 1); - assert.equal(lines[0].context, undefined, "context should be omitted when no safe keys match"); + const ctx = lines[0].context as Record; + assert.ok(ctx, "context should exist when error key is present"); + assert.equal(ctx.error, "ENOENT", "error key should be preserved"); + assert.equal(ctx.cwd, undefined, "cwd key must be stripped"); }); test("mixed warnings and errors only persist errors", () => { diff --git a/src/resources/extensions/gsd/workflow-logger.ts b/src/resources/extensions/gsd/workflow-logger.ts index 77960f7ca..e4d62b39b 100644 --- a/src/resources/extensions/gsd/workflow-logger.ts +++ b/src/resources/extensions/gsd/workflow-logger.ts @@ -295,7 +295,7 @@ function _sanitizeForAudit(entry: LogEntry): LogEntry { }; if (entry.context) { // Allowlist: only persist known-safe structured keys - const SAFE_KEYS = new Set(["fn", "tool", "mid", "sid", "tid", "worktree"]); + const SAFE_KEYS = new Set(["fn", "tool", "mid", "sid", "tid", "worktree", "id", "error", "count"]); const filtered: Record = {}; for (const [k, v] of Object.entries(entry.context)) { if (SAFE_KEYS.has(k)) {