fix(#464): make phase completion planning writes transactional (#465)

* fix(#464): make phase completion planning writes transactional

* fix(#464): add phase completion changeset

* test(#464): avoid source-grep rollback assertion

* fix(#464): address phase completion rollback review

* fix(#191): remove retired sdk tsconfig reference

.changeset/jolly-wolves-hop.md

@@ -0,0 +1,5 @@
+---
+type: Fixed
+pr: 465
+---
+**`phase complete` no longer leaves roadmap and state inconsistent after a failed state publish** — phase completion now publishes planning files from one locked transaction and rolls back earlier writes when a later write fails.

.gitignore

@@ -89,3 +89,7 @@ claude-test-command.md
 .stryker-tmp/
 .stryker-incremental.json
 reports/mutation/
+
+# Local Crabbox machine configuration
+.crabbox.yaml
+.crabbox.local.yaml

get-shit-done/bin/lib/phase.cjs

@@ -18,7 +18,7 @@ const { escapeRegex, loadConfig, normalizePhaseName, phaseMarkdownRegexSource, c
 const { platformWriteSync, platformReadSync, platformEnsureDir } = require('./shell-command-projection.cjs');
 const { planningDir, withPlanningLock } = require('./planning-workspace.cjs');
 const { extractFrontmatter } = require('./frontmatter.cjs');
-const { writeStateMd, readModifyWriteStateMd, stateExtractField, stateReplaceField, stateReplaceFieldWithFallback, updatePerformanceMetricsSection } = require('./state.cjs');
+const { readModifyWriteStateMd, stateExtractField, stateReplaceField, stateReplaceFieldWithFallback, syncStateFrontmatter, withStateLock, updatePerformanceMetricsSection } = require('./state.cjs');
 const { formatGsdSlash, resolveRuntime } = require('./runtime-slash.cjs');
 // Pure-computation helpers for cmdPhaseComplete (issue #4 fix).
 const { deriveProgressFromRoadmap, clampPercent } = require('./phase-lifecycle.cjs');
@@ -1207,6 +1207,31 @@ function cmdPhaseRemove(cwd, targetPhase, options, raw) {
   }, raw);
 }
 
+function writePlanningFileSet(writes) {
+  const applied = [];
+  try {
+    for (const write of writes) {
+      if (write.before === write.after) continue;
+      platformWriteSync(write.filePath, write.after);
+      applied.push(write);
+    }
+  } catch (err) {
+    for (const write of applied.reverse()) {
+      try {
+        platformWriteSync(write.filePath, write.before);
+      } catch (rollbackErr) {
+        err.rollbackError = rollbackErr;
+        err.message += `\nWARNING: rollback failed while restoring ${write.filePath} ` +
+          `(${rollbackErr.message}). Planning files under .planning/ may be left in an ` +
+          `inconsistent, partially rolled back state. Inspect ROADMAP.md / REQUIREMENTS.md / ` +
+          `STATE.md before re-running phase complete.`;
+        break;
+      }
+    }
+    throw err;
+  }
+}
+
 function cmdPhaseComplete(cwd, phaseNum, raw) {
   if (!phaseNum) {
     error('phase number required for phase complete');
@@ -1249,10 +1274,22 @@ function cmdPhaseComplete(cwd, phaseNum, raw) {
     }
   } catch {}
 
-  // Update ROADMAP.md and REQUIREMENTS.md atomically under lock
+  let nextPhaseNum = null;
-  if (fs.existsSync(roadmapPath)) {
+  let nextPhaseName = null;
+  let isLastPhase = true;
+
+  // Update ROADMAP.md, REQUIREMENTS.md, and STATE.md from one locked snapshot.
+  // A previous split-lock sequence could publish ROADMAP/REQUIREMENTS and then
+  // fail before STATE advanced, leaving planning files disagreeing about the
+  // current phase.
   withPlanningLock(cwd, () => {
-      let roadmapContent = fs.readFileSync(roadmapPath, 'utf-8');
+    const runPhaseCompleteTransaction = () => {
+      const writes = [];
+      let roadmapContent = null;
+
+      if (fs.existsSync(roadmapPath)) {
+        const originalRoadmapContent = fs.readFileSync(roadmapPath, 'utf-8');
+        roadmapContent = originalRoadmapContent;
 
         // Checkbox: - [ ] Phase N: → - [x] Phase N: (...completed DATE)
         // #3537: padding-tolerant fragment so the caller-resolved padded id
@@ -1315,7 +1352,7 @@ function cmdPhaseComplete(cwd, phaseNum, raw) {
           roadmapContent = roadmapContent.replace(planCheckboxPattern, '$1x$2');
         }
 
-      platformWriteSync(roadmapPath, roadmapContent);
+        writes.push({ filePath: roadmapPath, before: originalRoadmapContent, after: roadmapContent });
 
         // Update REQUIREMENTS.md traceability for this phase's requirements
         const reqPath = path.join(planningDir(cwd), 'REQUIREMENTS.md');
@@ -1336,7 +1373,8 @@ function cmdPhaseComplete(cwd, phaseNum, raw) {
           // from being ticked off in REQUIREMENTS.md on phase completion.
           const reqMatch = sectionText.match(/\*\*Requirements:?\*\*[^\S\n]*:?[^\S\n]*([^\n]+)/i);
 
-        let reqContent = fs.readFileSync(reqPath, 'utf-8');
+          const originalReqContent = fs.readFileSync(reqPath, 'utf-8');
+          let reqContent = originalReqContent;
 
           if (reqMatch) {
             const reqIds = reqMatch[1].replace(/[\[\]]/g, '').split(/[,\s]+/).map(r => r.trim()).filter(Boolean);
@@ -1386,19 +1424,14 @@ function cmdPhaseComplete(cwd, phaseNum, raw) {
             );
           }
 
-        platformWriteSync(reqPath, reqContent);
+          writes.push({ filePath: reqPath, before: originalReqContent, after: reqContent });
           requirementsUpdated = true;
         }
-    });
       }
 
       // Find next phase — check both filesystem AND roadmap
       // Phases may be defined in ROADMAP.md but not yet scaffolded to disk,
       // so a filesystem-only scan would incorrectly report is_last_phase:true
-  let nextPhaseNum = null;
-  let nextPhaseName = null;
-  let isLastPhase = true;
-
       try {
         const isDirInMilestone = getMilestonePhaseFilter(cwd);
         const entries = fs.readdirSync(phasesDir, { withFileTypes: true });
@@ -1424,9 +1457,9 @@ function cmdPhaseComplete(cwd, phaseNum, raw) {
 
       // Fallback: if filesystem found no next phase, check ROADMAP.md
       // for phases that are defined but not yet planned (no directory on disk)
-  if (isLastPhase && fs.existsSync(roadmapPath)) {
+      if (isLastPhase && roadmapContent !== null) {
         try {
-      const roadmapForPhases = extractCurrentMilestone(fs.readFileSync(roadmapPath, 'utf-8'), cwd);
+          const roadmapForPhases = extractCurrentMilestone(roadmapContent, cwd);
           const phasePattern = /#{2,4}\s*Phase\s+(\d+[A-Z]?(?:\.\d+)*)\s*:\s*([^\n]+)/gi;
           let pm;
           while ((pm = phasePattern.exec(roadmapForPhases)) !== null) {
@@ -1440,11 +1473,11 @@ function cmdPhaseComplete(cwd, phaseNum, raw) {
         } catch { /* intentionally empty */ }
       }
 
-  // Update STATE.md atomically — hold lock across read-modify-write (#P4.4).
+      // Update STATE.md while the planning lock is still held.
-  // Previously read outside the lock; a crash between the ROADMAP update
-  // (locked above) and this write left ROADMAP/STATE inconsistent.
       if (fs.existsSync(statePath)) {
-    readModifyWriteStateMd(statePath, (stateContent) => {
+        const originalStateContent = platformReadSync(statePath) || '';
+        let stateContent = originalStateContent;
+
         // Update Current Phase — preserve "X of Y (Name)" compound format
         const phaseValue = nextPhaseNum || phaseNum;
         const existingPhaseField = stateExtractField(stateContent, 'Current Phase')
@@ -1480,25 +1513,17 @@ function cmdPhaseComplete(cwd, phaseNum, raw) {
         stateContent = stateReplaceFieldWithFallback(stateContent, 'Last Activity Description', null,
           `Phase ${phaseNum} complete${nextPhaseNum ? `, transitioned to Phase ${nextPhaseNum}` : ''}`);
 
-      // Update Completed Phases counter — derive from ROADMAP instead of blind +1.
+        // Update Completed Phases counter — derive from the same ROADMAP snapshot
-      // Fix for issue #4: the original code did parseInt(completedRaw, 10) + 1 on every
+        // that will be published in this transaction, not a separately-read file.
-      // call, making phase complete non-idempotent (double-call = double-increment).
-      // Now we read the freshly-updated ROADMAP to count Complete rows, then use
-      // deriveProgressFromRoadmap() from phase-lifecycle.generated.cjs (generated from
-      // sdk/src/query/phase-lifecycle.ts "Root cause 1 fix" block).
-      // References: issue #4, ADR-3524, gen-phase-lifecycle.mjs.
         const completedRaw = stateExtractField(stateContent, 'Completed Phases');
         if (completedRaw !== null) {
           // Derive from ROADMAP if available (idempotent); fall back to existing value.
           let newCompleted = parseInt(completedRaw, 10);
           let derivedTotalPhases = null;
-        if (fs.existsSync(roadmapPath)) {
+          if (roadmapContent !== null) {
-          try {
+            const derived = deriveProgressFromRoadmap(roadmapContent);
-            const freshRoadmap = fs.readFileSync(roadmapPath, 'utf-8');
-            const derived = deriveProgressFromRoadmap(freshRoadmap);
             if (derived.completedPhases !== null) newCompleted = derived.completedPhases;
             if (derived.totalPhases !== null) derivedTotalPhases = derived.totalPhases;
-          } catch { /* fall through to existing value */ }
           }
           stateContent = stateReplaceField(stateContent, 'Completed Phases', String(newCompleted)) || stateContent;
 
@@ -1518,11 +1543,21 @@ function cmdPhaseComplete(cwd, phaseNum, raw) {
 
         // Gate 4: Update Performance Metrics section (#1627)
         stateContent = updatePerformanceMetricsSection(stateContent, cwd, phaseNum, planCount, summaryCount);
+        stateContent = syncStateFrontmatter(stateContent, cwd);
 
-      return stateContent;
+        writes.push({ filePath: statePath, before: originalStateContent, after: stateContent });
-    }, cwd);
       }
 
+      writePlanningFileSet(writes);
+    };
+
+    if (fs.existsSync(statePath)) {
+      withStateLock(statePath, runPhaseCompleteTransaction);
+    } else {
+      runPhaseCompleteTransaction();
+    }
+  });
+
   // Auto-prune STATE.md on phase boundary when configured (#2087)
   let autoPruned = false;
   try {

get-shit-done/bin/lib/planning-workspace.cjs

@@ -100,7 +100,7 @@ function withPlanningLock(cwd, fn, clock) {
   // Ensure .planning/ exists
   try { platformEnsureDir(planningDir(cwd)); } catch { /* ok */ }
 
-  function runWithHeldLock() {
+  function acquireLock() {
     // Atomic create — fails if file exists
     fs.writeFileSync(lockPath, JSON.stringify({
       pid: process.pid,
@@ -109,8 +109,9 @@ function withPlanningLock(cwd, fn, clock) {
     }), { flag: 'wx' });
 
     _heldPlanningLocks.add(lockPath);
+  }
 
-    // Lock acquired — run the function
+  function runWithHeldLock() {
     try {
       return fn();
     } finally {
@@ -120,12 +121,16 @@ function withPlanningLock(cwd, fn, clock) {
   }
 
   while (clock.now() - start < lockTimeout) {
+    let lockWasAcquired = false;
     try {
+      acquireLock();
+      lockWasAcquired = true;
       return runWithHeldLock();
     } catch (err) {
       // Transient filesystem errors (Docker overlay-fs, NFS, OS signals, AV scanners)
       // are recoverable — wait and retry rather than propagating.
       // See PLANNING_LOCK_RETRY_ERRNOS for the full list and rationale.
+      if (lockWasAcquired) throw err;
       if (PLANNING_LOCK_RETRY_ERRNOS.has(err.code)) {
         clock.sleep(100);
         continue;
@@ -150,6 +155,7 @@ function withPlanningLock(cwd, fn, clock) {
 
   // Timeout — stale-lock recovery, then re-acquire atomically before entering critical section.
   try { fs.unlinkSync(lockPath); } catch { /* ok */ }
+  acquireLock();
   return runWithHeldLock();
 }
 

get-shit-done/bin/lib/state.cjs

@@ -1050,6 +1050,15 @@ function releaseStateLock(lockPath) {
   try { fs.unlinkSync(lockPath); } catch { /* lock already gone */ }
 }
 
+function withStateLock(statePath, fn) {
+  const lockPath = acquireStateLock(statePath);
+  try {
+    return fn();
+  } finally {
+    releaseStateLock(lockPath);
+  }
+}
+
 /**
  * Write STATE.md with synchronized YAML frontmatter.
  * All STATE.md writes should use this instead of raw writeFileSync.
@@ -2001,6 +2010,8 @@ module.exports = {
   releaseStateLock,
   writeStateMd,
   readModifyWriteStateMd,
+  syncStateFrontmatter,
+  withStateLock,
   updatePerformanceMetricsSection,
   cmdStateLoad,
   cmdStateGet,

tests/4-phase-complete-cjs-regression.test.cjs

@@ -41,6 +41,7 @@ const { cmdPhaseComplete } = phaseModule;
 /**
  * Creates a minimal fixture project with:
  *   - ROADMAP.md with a 4-column progress table (Phase | Plans | Status | Completed)
+ *   - REQUIREMENTS.md with a phase-scoped REQ-ID and Traceability row
  *   - STATE.md with Completed Phases: 0 and Total Phases: 2 (Progress 0%)
  *   - Phase 01 directory with one plan+summary (to satisfy phase complete guard)
  *   - Phase 02 directory (next phase)
@@ -61,6 +62,7 @@ function createFixture(prefix = 'gsd-4-regression-') {
     '',
     '### Phase 01: Foundation',
     '**Goal:** Build the foundation',
+    '**Requirements:** REQ-1',
     '**Plans:** 1 plans',
     '',
     '### Phase 02: API',
@@ -76,6 +78,22 @@ function createFixture(prefix = 'gsd-4-regression-') {
   ].join('\n');
   fs.writeFileSync(path.join(planningDir, 'ROADMAP.md'), roadmap);
 
+  const requirements = [
+    '# Requirements',
+    '',
+    '## Functional Requirements',
+    '',
+    '- [ ] **REQ-1** Foundation must be complete.',
+    '',
+    '## Traceability',
+    '',
+    '| Requirement | Phase | Status |',
+    '|-------------|-------|--------|',
+    '| REQ-1 | Phase 01 | Pending |',
+    '',
+  ].join('\n');
+  fs.writeFileSync(path.join(planningDir, 'REQUIREMENTS.md'), requirements);
+
   // STATE.md: Completed Phases: 0, Total Phases: 2, Progress: 0%
   // Uses body-field format (bold **Field:** value) so the CJS handler's
   // stateExtractField/stateReplaceField path is exercised.
@@ -111,6 +129,38 @@ function readStateMd(tmpDir) {
   return fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf8');
 }
 
+function roadmapCompletionSnapshot(roadmapContent) {
+  const snapshot = {
+    phaseCheckboxes: [],
+    progressRows: [],
+  };
+
+  for (const line of roadmapContent.split(/\r?\n/)) {
+    let match = line.match(/^- \[([ x])\] Phase ([^:]+): (.*)$/);
+    if (match) {
+      snapshot.phaseCheckboxes.push({
+        checked: match[1] === 'x',
+        phase: match[2].trim(),
+        title: match[3].replace(/\s+\(completed [^)]+\)$/, '').trim(),
+      });
+      continue;
+    }
+
+    match = line.match(/^\|\s*(\d+[A-Z]?(?:\.\d+)*)\.?\s*([^|]*)\|\s*([^|]*)\|\s*([^|]*)\|\s*([^|]*)\|$/i);
+    if (match) {
+      snapshot.progressRows.push({
+        phase: match[1].trim(),
+        title: match[2].trim(),
+        plans: match[3].trim(),
+        status: match[4].trim(),
+        completed: match[5].trim(),
+      });
+    }
+  }
+
+  return snapshot;
+}
+
 function extractField(stateContent, fieldName) {
   const escaped = fieldName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
   const boldMatch = stateContent.match(new RegExp(`\\*\\*${escaped}:\\*\\*[ \\t]*(.+)`, 'i'));
@@ -207,6 +257,102 @@ describe('issue #4 (CJS): cmdPhaseComplete — idempotency (blind-increment bug)
       `STATE after second call (body: ${completedAfter2Body}, fm: ${completedAfter2Fm}):\n${stateAfter2}`,
     );
   });
+
+  test('rolls back ROADMAP when STATE write fails during phase completion', (t) => {
+    const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md');
+    const reqPath = path.join(tmpDir, '.planning', 'REQUIREMENTS.md');
+    const statePath = path.join(tmpDir, '.planning', 'STATE.md');
+    const originalRoadmap = fs.readFileSync(roadmapPath, 'utf8');
+    const originalReq = fs.readFileSync(reqPath, 'utf8');
+    const originalState = fs.readFileSync(statePath, 'utf8');
+    const originalWriteFileSync = fs.writeFileSync;
+
+    t.mock.method(fs, 'writeFileSync', function injectedStateWriteFailure(target, ...args) {
+      const targetPath = String(target);
+      const isStatePublish = targetPath === statePath || targetPath === `${statePath}.tmp.${process.pid}`;
+      if (isStatePublish) {
+        const err = new Error('injected STATE.md write failure');
+        err.code = 'EIO';
+        throw err;
+      }
+      return originalWriteFileSync.call(this, target, ...args);
+    });
+
+    assert.throws(
+      () => capturePhaseComplete(tmpDir, '1'),
+      /injected STATE\.md write failure/,
+    );
+
+    const roadmapAfter = fs.readFileSync(roadmapPath, 'utf8');
+    const reqAfter = fs.readFileSync(reqPath, 'utf8');
+    const stateAfter = fs.readFileSync(statePath, 'utf8');
+
+    assert.deepEqual(
+      roadmapCompletionSnapshot(roadmapAfter),
+      roadmapCompletionSnapshot(originalRoadmap),
+      'ROADMAP.md should roll back to its original completion state',
+    );
+    assert.equal(reqAfter, originalReq, 'REQUIREMENTS.md should roll back when STATE.md write fails');
+    assert.equal(stateAfter, originalState, 'STATE.md should remain unchanged after injected write failure');
+  });
+
+  test('rolls back ROADMAP when REQUIREMENTS write fails during phase completion', (t) => {
+    const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md');
+    const reqPath = path.join(tmpDir, '.planning', 'REQUIREMENTS.md');
+    const originalRoadmap = fs.readFileSync(roadmapPath, 'utf8');
+    const originalReq = fs.readFileSync(reqPath, 'utf8');
+    const originalWriteFileSync = fs.writeFileSync;
+
+    t.mock.method(fs, 'writeFileSync', function injectedRequirementsWriteFailure(target, ...args) {
+      const targetPath = String(target);
+      if (targetPath === reqPath || targetPath === `${reqPath}.tmp.${process.pid}`) {
+        const err = new Error('injected REQUIREMENTS.md write failure');
+        err.code = 'EIO';
+        throw err;
+      }
+      return originalWriteFileSync.call(this, target, ...args);
+    });
+
+    assert.throws(
+      () => capturePhaseComplete(tmpDir, '1'),
+      /injected REQUIREMENTS\.md write failure/,
+    );
+
+    assert.deepEqual(
+      roadmapCompletionSnapshot(fs.readFileSync(roadmapPath, 'utf8')),
+      roadmapCompletionSnapshot(originalRoadmap),
+      'ROADMAP.md should roll back when the REQUIREMENTS write fails',
+    );
+    assert.equal(fs.readFileSync(reqPath, 'utf8'), originalReq, 'REQUIREMENTS.md should be unchanged');
+  });
+
+  test('reports rollback failure when restoring an earlier planning file fails', (t) => {
+    const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md');
+    const reqPath = path.join(tmpDir, '.planning', 'REQUIREMENTS.md');
+    const originalWriteFileSync = fs.writeFileSync;
+    let requirementsWriteFailed = false;
+
+    t.mock.method(fs, 'writeFileSync', function injectedRollbackFailure(target, ...args) {
+      const targetPath = String(target);
+      if (targetPath === reqPath || targetPath === `${reqPath}.tmp.${process.pid}`) {
+        requirementsWriteFailed = true;
+        const err = new Error('injected REQUIREMENTS.md write failure');
+        err.code = 'EIO';
+        throw err;
+      }
+      if (requirementsWriteFailed && (targetPath === roadmapPath || targetPath === `${roadmapPath}.tmp.${process.pid}`)) {
+        const err = new Error('injected ROADMAP.md rollback failure');
+        err.code = 'EIO';
+        throw err;
+      }
+      return originalWriteFileSync.call(this, target, ...args);
+    });
+
+    assert.throws(
+      () => capturePhaseComplete(tmpDir, '1'),
+      /injected REQUIREMENTS\.md write failure[\s\S]*WARNING: rollback failed while restoring[\s\S]*injected ROADMAP\.md rollback failure/,
+    );
+  });
 });
 
 // ── T2: Progress percent must never exceed 100% ──────────────────────────────

tests/bugs-1656-1657.test.cjs

@@ -90,4 +90,20 @@ describe('#1657 / #191: installer/package metadata retires sdk seam', () => {
       'root package.json files must not include sdk paths'
     );
   });
+
+  test('root tsconfig project references resolve to existing paths (#191)', () => {
+    const tsconfigPath = path.join(__dirname, '..', 'tsconfig.json');
+    if (!fs.existsSync(tsconfigPath)) return;
+
+    const tsconfig = JSON.parse(fs.readFileSync(tsconfigPath, 'utf-8'));
+    const references = tsconfig.references || [];
+    for (const ref of references) {
+      const refPath = String(ref.path || '');
+      assert.notEqual(refPath, 'sdk', 'root tsconfig must not reference retired sdk project');
+      assert.ok(
+        fs.existsSync(path.join(__dirname, '..', refPath)),
+        `root tsconfig reference must exist: ${refPath}`
+      );
+    }
+  });
 });

tests/planning-workspace.test.cjs

@@ -125,6 +125,25 @@ describe('planning-workspace: lock seam', () => {
       fs.rmSync(tmpDir, { recursive: true, force: true });
     }
   });
+
+  test('does not retry errors thrown by locked work', () => {
+    const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-planning-lock-work-error-'));
+    let attempts = 0;
+    try {
+      assert.throws(() => {
+        withPlanningLock(tmpDir, () => {
+          attempts += 1;
+          const err = new Error('write failed inside critical section');
+          err.code = 'EIO';
+          throw err;
+        });
+      }, /write failed inside critical section/);
+      assert.strictEqual(attempts, 1);
+      assert.ok(!fs.existsSync(path.join(tmpDir, '.planning', '.lock')));
+    } finally {
+      fs.rmSync(tmpDir, { recursive: true, force: true });
+    }
+  });
 });
 
 describe('core compatibility adapter: planning workspace functions', () => {

tsconfig.json

@@ -1,6 +1,4 @@
 {
   "files": [],
-  "references": [
+  "references": []
-    { "path": "sdk" }
-  ]
 }