From cad40fff8bbe36e8804ddb1bbdecbec9eb81252a Mon Sep 17 00:00:00 2001 From: Tibsfox Date: Fri, 10 Apr 2026 11:34:17 -0700 Subject: [PATCH 1/2] fix(core): extend atomicWriteFileSync to milestone, phase, and frontmatter (#1972) Replace 11 fs.writeFileSync calls with atomicWriteFileSync in three files that write to .planning/ artifacts (ROADMAP.md, REQUIREMENTS.md, MILESTONES.md, and frontmatter updates). This prevents partial writes from corrupting planning files on crash or power loss. Skipped low-risk writes: .gitkeep (empty files) and archive directory writes (new files, not read-modify-write). Files changed: - milestone.cjs: 5 sites (REQUIREMENTS.md, MILESTONES.md) - phase.cjs: 5 sites (ROADMAP.md, REQUIREMENTS.md) - frontmatter.cjs: 2 sites (arbitrary .planning/ files) Closes #1972 Co-Authored-By: Claude Opus 4.6 --- get-shit-done/bin/lib/frontmatter.cjs | 6 +++--- get-shit-done/bin/lib/milestone.cjs | 12 ++++++------ get-shit-done/bin/lib/phase.cjs | 12 ++++++------ 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/get-shit-done/bin/lib/frontmatter.cjs b/get-shit-done/bin/lib/frontmatter.cjs index f8e4ee7fc..ad567a034 100644 --- a/get-shit-done/bin/lib/frontmatter.cjs +++ b/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'); } diff --git a/get-shit-done/bin/lib/milestone.cjs b/get-shit-done/bin/lib/milestone.cjs index 181153f2c..7a023bb11 100644 --- a/get-shit-done/bin/lib/milestone.cjs +++ b/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 diff --git a/get-shit-done/bin/lib/phase.cjs b/get-shit-done/bin/lib/phase.cjs index e470eec08..bad0e9cbd 100644 --- a/get-shit-done/bin/lib/phase.cjs +++ b/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; } } From 2cd0e0d8f0098707172e83d0cd091c44f13e2845 Mon Sep 17 00:00:00 2001 From: Tibsfox Date: Sat, 11 Apr 2026 03:30:05 -0700 Subject: [PATCH 2/2] test(core): add atomic write coverage structural regression guard (#1972) Per CONTRIBUTING.md, enhancements require tests covering the enhanced behavior. This test structurally verifies that milestone.cjs, phase.cjs, and frontmatter.cjs do not contain bare fs.writeFileSync calls targeting .planning/ files. All such writes must route through atomicWriteFileSync. Allowed exceptions: .gitkeep writes (empty files) and archive directory writes (new files, not read-modify-write). This complements atomic-write.test.cjs which tests the helper itself. If someone later adds a bare writeFileSync to these files without using the atomic helper, this test will catch it. Review feedback on #2056 from @trek-e. --- tests/atomic-write-coverage.test.cjs | 95 ++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 tests/atomic-write-coverage.test.cjs diff --git a/tests/atomic-write-coverage.test.cjs b/tests/atomic-write-coverage.test.cjs new file mode 100644 index 000000000..533b1299c --- /dev/null +++ b/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` + ); + } + }); +});