From 60fa2936dd209e87e5bfca2d63b5fd9b297e7a7d Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 7 Apr 2026 17:27:20 -0400 Subject: [PATCH] fix(core): add atomicWriteFileSync to prevent truncated files on kill (#1943) Replaces direct fs.writeFileSync calls for STATE.md, ROADMAP.md, and config.json with write-to-temp-then-rename so a process killed mid-write cannot leave an unparseable truncated file. Falls back to direct write if rename fails (e.g. cross-device). Adds regression tests for the helper. Closes #1915 Co-authored-by: Claude Sonnet 4.6 --- get-shit-done/bin/lib/config.cjs | 8 ++-- get-shit-done/bin/lib/core.cjs | 33 +++++++++++++ get-shit-done/bin/lib/roadmap.cjs | 4 +- get-shit-done/bin/lib/state.cjs | 6 +-- tests/atomic-write.test.cjs | 78 +++++++++++++++++++++++++++++++ 5 files changed, 120 insertions(+), 9 deletions(-) create mode 100644 tests/atomic-write.test.cjs diff --git a/get-shit-done/bin/lib/config.cjs b/get-shit-done/bin/lib/config.cjs index 0cd64ab5e..cbbaf6440 100644 --- a/get-shit-done/bin/lib/config.cjs +++ b/get-shit-done/bin/lib/config.cjs @@ -4,7 +4,7 @@ const fs = require('fs'); const path = require('path'); -const { output, error, planningRoot, CONFIG_DEFAULTS } = require('./core.cjs'); +const { output, error, planningRoot, CONFIG_DEFAULTS, atomicWriteFileSync } = require('./core.cjs'); const { VALID_PROFILES, getAgentToModelMapForProfile, @@ -227,7 +227,7 @@ function cmdConfigNewProject(cwd, choicesJson, raw) { const config = buildNewProjectConfig(userChoices); try { - fs.writeFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8'); + atomicWriteFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8'); output({ created: true, path: '.planning/config.json' }, raw, 'created'); } catch (err) { error('Failed to write config.json: ' + err.message); @@ -261,7 +261,7 @@ function ensureConfigFile(cwd) { const config = buildNewProjectConfig({}); try { - fs.writeFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8'); + atomicWriteFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8'); return { created: true, path: '.planning/config.json' }; } catch (err) { error('Failed to create config.json: ' + err.message); @@ -318,7 +318,7 @@ function setConfigValue(cwd, keyPath, parsedValue) { // Write back try { - fs.writeFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8'); + atomicWriteFileSync(configPath, JSON.stringify(config, null, 2), 'utf-8'); return { updated: true, key: keyPath, value: parsedValue, previousValue }; } catch (err) { error('Failed to write config.json: ' + err.message); diff --git a/get-shit-done/bin/lib/core.cjs b/get-shit-done/bin/lib/core.cjs index 394f2eb66..e233a2728 100644 --- a/get-shit-done/bin/lib/core.cjs +++ b/get-shit-done/bin/lib/core.cjs @@ -1491,6 +1491,38 @@ function readSubdirectories(dirPath, sort = false) { } } +// ─── Atomic file writes ─────────────────────────────────────────────────────── + +/** + * Write a file atomically using write-to-temp-then-rename. + * + * On POSIX systems, `fs.renameSync` is atomic when the source and destination + * are on the same filesystem. This prevents a process killed mid-write from + * leaving a truncated file that is unparseable on next read. + * + * The temp file is placed alongside the target so it is guaranteed to be on + * the same filesystem (required for rename atomicity). The PID is embedded in + * the temp file name so concurrent writers use distinct paths. + * + * If `renameSync` fails (e.g. cross-device move), the function falls back to a + * direct `writeFileSync` so callers always get a best-effort write. + * + * @param {string} filePath Absolute path to write. + * @param {string|Buffer} content File content. + * @param {string} [encoding='utf-8'] Encoding passed to writeFileSync. + */ +function atomicWriteFileSync(filePath, content, encoding = 'utf-8') { + const tmpPath = filePath + '.tmp.' + process.pid; + try { + fs.writeFileSync(tmpPath, content, encoding); + fs.renameSync(tmpPath, filePath); + } catch (renameErr) { + // Clean up the temp file if rename failed, then fall back to direct write. + try { fs.unlinkSync(tmpPath); } catch { /* already gone or never created */ } + fs.writeFileSync(filePath, content, encoding); + } +} + module.exports = { output, error, @@ -1536,4 +1568,5 @@ module.exports = { readSubdirectories, getAgentsDir, checkAgentsInstalled, + atomicWriteFileSync, }; diff --git a/get-shit-done/bin/lib/roadmap.cjs b/get-shit-done/bin/lib/roadmap.cjs index 638f24897..4a7aff7e7 100644 --- a/get-shit-done/bin/lib/roadmap.cjs +++ b/get-shit-done/bin/lib/roadmap.cjs @@ -4,7 +4,7 @@ const fs = require('fs'); const path = require('path'); -const { escapeRegex, normalizePhaseName, planningPaths, withPlanningLock, output, error, findPhaseInternal, stripShippedMilestones, extractCurrentMilestone, replaceInCurrentMilestone, phaseTokenMatches } = require('./core.cjs'); +const { escapeRegex, normalizePhaseName, planningPaths, withPlanningLock, output, error, findPhaseInternal, stripShippedMilestones, extractCurrentMilestone, replaceInCurrentMilestone, phaseTokenMatches, atomicWriteFileSync } = require('./core.cjs'); /** * Search for a phase header (and its section) within the given content string. @@ -341,7 +341,7 @@ function cmdRoadmapUpdatePlanProgress(cwd, phaseNum, raw) { roadmapContent = roadmapContent.replace(planCheckboxPattern, '$1x$2'); } - fs.writeFileSync(roadmapPath, roadmapContent, 'utf-8'); + atomicWriteFileSync(roadmapPath, roadmapContent, 'utf-8'); }); output({ updated: true, diff --git a/get-shit-done/bin/lib/state.cjs b/get-shit-done/bin/lib/state.cjs index 1467cdf21..ed270b4d6 100644 --- a/get-shit-done/bin/lib/state.cjs +++ b/get-shit-done/bin/lib/state.cjs @@ -4,7 +4,7 @@ const fs = require('fs'); const path = require('path'); -const { escapeRegex, loadConfig, getMilestoneInfo, getMilestonePhaseFilter, normalizeMd, planningDir, planningPaths, output, error } = require('./core.cjs'); +const { escapeRegex, loadConfig, getMilestoneInfo, getMilestonePhaseFilter, normalizeMd, planningDir, planningPaths, output, error, atomicWriteFileSync } = require('./core.cjs'); const { extractFrontmatter, reconstructFrontmatter } = require('./frontmatter.cjs'); /** Shorthand — every state command needs this path */ @@ -845,7 +845,7 @@ function writeStateMd(statePath, content, cwd) { const synced = syncStateFrontmatter(content, cwd); const lockPath = acquireStateLock(statePath); try { - fs.writeFileSync(statePath, normalizeMd(synced), 'utf-8'); + atomicWriteFileSync(statePath, normalizeMd(synced), 'utf-8'); } finally { releaseStateLock(lockPath); } @@ -863,7 +863,7 @@ function readModifyWriteStateMd(statePath, transformFn, cwd) { const content = fs.existsSync(statePath) ? fs.readFileSync(statePath, 'utf-8') : ''; const modified = transformFn(content); const synced = syncStateFrontmatter(modified, cwd); - fs.writeFileSync(statePath, normalizeMd(synced), 'utf-8'); + atomicWriteFileSync(statePath, normalizeMd(synced), 'utf-8'); } finally { releaseStateLock(lockPath); } diff --git a/tests/atomic-write.test.cjs b/tests/atomic-write.test.cjs new file mode 100644 index 000000000..27fae200c --- /dev/null +++ b/tests/atomic-write.test.cjs @@ -0,0 +1,78 @@ +/** + * Tests for atomicWriteFileSync helper (issue #1915) + */ + +const { test, describe, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const { createTempDir, cleanup } = require('./helpers.cjs'); + +const CORE_PATH = path.join(__dirname, '..', 'get-shit-done', 'bin', 'lib', 'core.cjs'); + +describe('atomicWriteFileSync', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempDir(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('is exported from core.cjs', () => { + const core = require(CORE_PATH); + assert.strictEqual(typeof core.atomicWriteFileSync, 'function', 'atomicWriteFileSync must be exported'); + }); + + test('writes correct content to the target file', () => { + const { atomicWriteFileSync } = require(CORE_PATH); + const filePath = path.join(tmpDir, 'test.md'); + const content = '# Hello\nworld\n'; + + atomicWriteFileSync(filePath, content, 'utf-8'); + + const written = fs.readFileSync(filePath, 'utf-8'); + assert.strictEqual(written, content, 'written content must match'); + }); + + test('does not leave .tmp.* files after successful write', () => { + const { atomicWriteFileSync } = require(CORE_PATH); + const filePath = path.join(tmpDir, 'STATE.md'); + + atomicWriteFileSync(filePath, '# State\n', 'utf-8'); + + const entries = fs.readdirSync(tmpDir); + const tmpFiles = entries.filter(e => e.includes('.tmp.')); + assert.deepStrictEqual(tmpFiles, [], 'no .tmp.* files should remain after write'); + }); + + test('overwrites an existing file with new content', () => { + const { atomicWriteFileSync } = require(CORE_PATH); + const filePath = path.join(tmpDir, 'config.json'); + + atomicWriteFileSync(filePath, '{"first":true}', 'utf-8'); + atomicWriteFileSync(filePath, '{"second":true}', 'utf-8'); + + const written = fs.readFileSync(filePath, 'utf-8'); + assert.strictEqual(written, '{"second":true}', 'second write must replace first'); + }); + + test('cleans up stale tmp file if present before write', () => { + const { atomicWriteFileSync } = require(CORE_PATH); + const filePath = path.join(tmpDir, 'ROADMAP.md'); + // Place a stale tmp file matching the pattern used by atomicWriteFileSync + const staleTmp = filePath + '.tmp.' + process.pid; + fs.writeFileSync(staleTmp, 'stale content', 'utf-8'); + + atomicWriteFileSync(filePath, '# Roadmap\n', 'utf-8'); + + const entries = fs.readdirSync(tmpDir); + const tmpFiles = entries.filter(e => e.includes('.tmp.')); + assert.deepStrictEqual(tmpFiles, [], 'stale .tmp.* file must be gone after write'); + + const written = fs.readFileSync(filePath, 'utf-8'); + assert.strictEqual(written, '# Roadmap\n', 'target file must have correct content'); + }); +});