Merge pull request #3725 from jeremymcs/fix/state-machine-wave3-session

fix(gsd): session and recovery robustness (wave 3/5)

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.

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");

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<void> {
@@ -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;

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;

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;

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

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) ──

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);
+  });
+});

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<string, string>;
+    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", () => {

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<string, string> = {};
     for (const [k, v] of Object.entries(entry.context)) {
       if (SAFE_KEYS.has(k)) {