Merge pull request #2056 from Tibsfox/fix/atomic-writes-1972

fix(core): extend atomicWriteFileSync to milestone, phase, and frontmatter

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

@@ -4,7 +4,7 @@
 
 const fs = require('fs');
 const path = require('path');
-const { safeReadFile, normalizeMd, output, error } = require('./core.cjs');
+const { safeReadFile, normalizeMd, output, error, atomicWriteFileSync } = require('./core.cjs');
 
 // ─── Parsing engine ───────────────────────────────────────────────────────────
 
@@ -337,7 +337,7 @@ function cmdFrontmatterSet(cwd, filePath, field, value, raw) {
   try { parsedValue = JSON.parse(value); } catch { parsedValue = value; }
   fm[field] = parsedValue;
   const newContent = spliceFrontmatter(content, fm);
-  fs.writeFileSync(fullPath, normalizeMd(newContent), 'utf-8');
+  atomicWriteFileSync(fullPath, normalizeMd(newContent));
   output({ updated: true, field, value: parsedValue }, raw, 'true');
 }
 
@@ -351,7 +351,7 @@ function cmdFrontmatterMerge(cwd, filePath, data, raw) {
   try { mergeData = JSON.parse(data); } catch { error('Invalid JSON for --data'); return; }
   Object.assign(fm, mergeData);
   const newContent = spliceFrontmatter(content, fm);
-  fs.writeFileSync(fullPath, normalizeMd(newContent), 'utf-8');
+  atomicWriteFileSync(fullPath, normalizeMd(newContent));
   output({ merged: true, fields: Object.keys(mergeData) }, raw, 'true');
 }
 

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

@@ -4,7 +4,7 @@
 
 const fs = require('fs');
 const path = require('path');
-const { escapeRegex, getMilestonePhaseFilter, extractOneLinerFromBody, normalizeMd, planningPaths, output, error } = require('./core.cjs');
+const { escapeRegex, getMilestonePhaseFilter, extractOneLinerFromBody, normalizeMd, planningPaths, output, error, atomicWriteFileSync } = require('./core.cjs');
 const { extractFrontmatter } = require('./frontmatter.cjs');
 const { writeStateMd, stateReplaceFieldWithFallback } = require('./state.cjs');
 
@@ -74,7 +74,7 @@ function cmdRequirementsMarkComplete(cwd, reqIdsRaw, raw) {
   }
 
   if (updated.length > 0) {
-    fs.writeFileSync(reqPath, reqContent, 'utf-8');
+    atomicWriteFileSync(reqPath, reqContent);
   }
 
   output({
@@ -178,21 +178,21 @@ function cmdMilestoneComplete(cwd, version, options, raw) {
     const existing = fs.readFileSync(milestonesPath, 'utf-8');
     if (!existing.trim()) {
       // Empty file — treat like new
-      fs.writeFileSync(milestonesPath, normalizeMd(`# Milestones\n\n${milestoneEntry}`), 'utf-8');
+      atomicWriteFileSync(milestonesPath, normalizeMd(`# Milestones\n\n${milestoneEntry}`));
     } else {
       // Insert after the header line(s) for reverse chronological order (newest first)
       const headerMatch = existing.match(/^(#{1,3}\s+[^\n]*\n\n?)/);
       if (headerMatch) {
         const header = headerMatch[1];
         const rest = existing.slice(header.length);
-        fs.writeFileSync(milestonesPath, normalizeMd(header + milestoneEntry + rest), 'utf-8');
+        atomicWriteFileSync(milestonesPath, normalizeMd(header + milestoneEntry + rest));
       } else {
         // No recognizable header — prepend the entry
-        fs.writeFileSync(milestonesPath, normalizeMd(milestoneEntry + existing), 'utf-8');
+        atomicWriteFileSync(milestonesPath, normalizeMd(milestoneEntry + existing));
       }
     }
   } else {
-    fs.writeFileSync(milestonesPath, normalizeMd(`# Milestones\n\n${milestoneEntry}`), 'utf-8');
+    atomicWriteFileSync(milestonesPath, normalizeMd(`# Milestones\n\n${milestoneEntry}`));
   }
 
   // Update STATE.md — use shared helpers that handle both **bold:** and plain Field: formats

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

@@ -4,7 +4,7 @@
 
 const fs = require('fs');
 const path = require('path');
-const { escapeRegex, loadConfig, normalizePhaseName, comparePhaseNum, findPhaseInternal, getArchivedPhaseDirs, generateSlugInternal, getMilestonePhaseFilter, stripShippedMilestones, extractCurrentMilestone, replaceInCurrentMilestone, toPosixPath, planningDir, withPlanningLock, output, error, readSubdirectories, phaseTokenMatches } = require('./core.cjs');
+const { escapeRegex, loadConfig, normalizePhaseName, comparePhaseNum, findPhaseInternal, getArchivedPhaseDirs, generateSlugInternal, getMilestonePhaseFilter, stripShippedMilestones, extractCurrentMilestone, replaceInCurrentMilestone, toPosixPath, planningDir, withPlanningLock, output, error, readSubdirectories, phaseTokenMatches, atomicWriteFileSync } = require('./core.cjs');
 const { extractFrontmatter } = require('./frontmatter.cjs');
 const { writeStateMd, readModifyWriteStateMd, stateExtractField, stateReplaceField, stateReplaceFieldWithFallback, updatePerformanceMetricsSection } = require('./state.cjs');
 
@@ -392,7 +392,7 @@ function cmdPhaseAdd(cwd, description, raw, customId) {
       updatedContent = rawContent + phaseEntry;
     }
 
-    fs.writeFileSync(roadmapPath, updatedContent, 'utf-8');
+    atomicWriteFileSync(roadmapPath, updatedContent);
     return { newPhaseId: _newPhaseId, dirName: _dirName };
   });
 
@@ -493,7 +493,7 @@ function cmdPhaseInsert(cwd, afterPhase, description, raw) {
     }
 
     const updatedContent = rawContent.slice(0, insertIdx) + phaseEntry + rawContent.slice(insertIdx);
-    fs.writeFileSync(roadmapPath, updatedContent, 'utf-8');
+    atomicWriteFileSync(roadmapPath, updatedContent);
     return { decimalPhase: _decimalPhase, dirName: _dirName };
   });
 
@@ -607,7 +607,7 @@ function updateRoadmapAfterPhaseRemoval(roadmapPath, targetPhase, isDecimal, rem
       }
     }
 
-    fs.writeFileSync(roadmapPath, content, 'utf-8');
+    atomicWriteFileSync(roadmapPath, content);
   });
 }
 
@@ -783,7 +783,7 @@ function cmdPhaseComplete(cwd, phaseNum, raw) {
         roadmapContent = roadmapContent.replace(planCheckboxPattern, '$1x$2');
       }
 
-      fs.writeFileSync(roadmapPath, roadmapContent, 'utf-8');
+      atomicWriteFileSync(roadmapPath, roadmapContent);
 
       // Update REQUIREMENTS.md traceability for this phase's requirements
       const reqPath = path.join(planningDir(cwd), 'REQUIREMENTS.md');
@@ -816,7 +816,7 @@ function cmdPhaseComplete(cwd, phaseNum, raw) {
             );
           }
 
-          fs.writeFileSync(reqPath, reqContent, 'utf-8');
+          atomicWriteFileSync(reqPath, reqContent);
           requirementsUpdated = true;
         }
       }

tests/atomic-write-coverage.test.cjs

@@ -0,0 +1,95 @@
+/**
+ * Structural regression guard for atomic write usage (#1972).
+ *
+ * Ensures that milestone.cjs, phase.cjs, and frontmatter.cjs do NOT
+ * contain bare fs.writeFileSync calls targeting .planning/ files. All
+ * such writes must go through atomicWriteFileSync to prevent partial
+ * writes from corrupting planning artifacts on crash.
+ *
+ * Allowed exceptions:
+ *   - Writes to .gitkeep (empty files, no corruption risk)
+ *   - Writes to archive directories (new files, not read-modify-write)
+ *
+ * This test is structural — it reads the source files and parses for
+ * bare writeFileSync patterns. It complements functional tests in
+ * atomic-write.test.cjs which verify the helper itself.
+ */
+
+'use strict';
+
+const { test, describe } = require('node:test');
+const assert = require('node:assert/strict');
+const fs = require('node:fs');
+const path = require('node:path');
+
+const libDir = path.resolve(__dirname, '..', 'get-shit-done', 'bin', 'lib');
+
+/**
+ * Find all fs.writeFileSync(...) call sites in a file.
+ * Returns array of { line: number, text: string }.
+ */
+function findBareWrites(filePath) {
+  const content = fs.readFileSync(filePath, 'utf-8');
+  const lines = content.split('\n');
+  const hits = [];
+  for (let i = 0; i < lines.length; i++) {
+    if (/\bfs\.writeFileSync\s*\(/.test(lines[i])) {
+      hits.push({ line: i + 1, text: lines[i].trim() });
+    }
+  }
+  return hits;
+}
+
+/**
+ * Classify a bare write as allowed (archive, .gitkeep) or disallowed.
+ */
+function isAllowedException(lineText) {
+  // .gitkeep writes (empty file, no corruption risk)
+  if (/\.gitkeep/.test(lineText)) return true;
+  // Archive directory writes (new files, not read-modify-write)
+  if (/archiveDir/.test(lineText)) return true;
+  return false;
+}
+
+describe('atomic write coverage (#1972)', () => {
+  const targetFiles = ['milestone.cjs', 'phase.cjs', 'frontmatter.cjs'];
+
+  for (const file of targetFiles) {
+    test(`${file}: all fs.writeFileSync calls target allowed exceptions`, () => {
+      const filePath = path.join(libDir, file);
+      assert.ok(fs.existsSync(filePath), `${file} must exist at ${filePath}`);
+
+      const hits = findBareWrites(filePath);
+      const violations = hits.filter(h => !isAllowedException(h.text));
+
+      if (violations.length > 0) {
+        const report = violations.map(v => `  line ${v.line}: ${v.text}`).join('\n');
+        assert.fail(
+          `${file} contains ${violations.length} bare fs.writeFileSync call(s) targeting planning files.\n` +
+          `These should use atomicWriteFileSync instead:\n${report}`
+        );
+      }
+    });
+
+    test(`${file}: imports atomicWriteFileSync from core.cjs`, () => {
+      const filePath = path.join(libDir, file);
+      const content = fs.readFileSync(filePath, 'utf-8');
+      assert.match(
+        content,
+        /atomicWriteFileSync.*require\(['"]\.\/core\.cjs['"]\)|atomicWriteFileSync[^)]*\}\s*=\s*require\(['"]\.\/core\.cjs['"]\)/s,
+        `${file} must import atomicWriteFileSync from core.cjs`
+      );
+    });
+  }
+
+  test('all three files use atomicWriteFileSync at least once', () => {
+    for (const file of targetFiles) {
+      const content = fs.readFileSync(path.join(libDir, file), 'utf-8');
+      assert.match(
+        content,
+        /atomicWriteFileSync\s*\(/,
+        `${file} must contain at least one atomicWriteFileSync call`
+      );
+    }
+  });
+});