From 1e091d2bcb0ffaa76045afaaee459c3e5b9be418 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 13 May 2026 20:46:02 -0400 Subject: [PATCH] refactor(shell-projection): remove deprecated wrappers + finalize ADRs (Phase 4, #3468) (#3484) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(shell-projection): remove deprecated wrappers + finalize ADRs (Phase 4, #3468) Final phase of the shell-command-projection expansion. Removes the legacy core.cjs wrappers (`atomicWriteFileSync`, `safeReadFile`, `normalizeMd`) now that every call site lives behind the seam, plus three Phase-3 stragglers (`graphify.cjs`, `template.cjs`, dead import in `profile-pipeline.cjs`). Documentation: - ADR-0009: addendum noting Phase 1–4 scope expansion (subprocess + file I/O ownership), supersession of "does not execute" constraint, and resolution of open Q4. - ADR-0010: status changed to Superseded by ADR-0009 with explanation. - CONTEXT.md "Shell Command Projection Module" entry already current from Phase 1 — no edit needed. Tests: - `tests/atomic-write.test.cjs` deleted — wrapper it tested is gone; `atomic-write-coverage.test.cjs` (Phase 3) covers platformWriteSync. - `tests/core.test.cjs::safeReadFile` + `::normalizeMd` describes deleted — wrappers are gone. - `tests/concurrency-safety.test.cjs` normalizeMd suite (behavioral / perf / snapshot) repointed via 2-line shim at the seam's `normalizeContent` — full regression coverage preserved. Test result: 9059/9041/18 — exact pre-Phase-4 baseline. All 18 failures are pre-existing path-with-spaces local-env issues. Closes #3468 Co-Authored-By: Claude Sonnet 4.6 * refactor(shell-projection): migrate remaining raw fs.writeFileSync sites (Phase 4, #3468) Sweeps the 7 raw fs.writeFileSync call sites that bypassed the seam through Phase 3, folding them into platformWriteSync. Net -14 lines: deletes the local writeFileAtomicSync helper in installer-migrations.cjs and collapses surface.cjs's manual tmp+rename into a single seam call. Sites migrated: - drift.cjs (1) — frontmatter write - learnings.cjs (1) — learning record JSON write - install-profiles.cjs (1) — profile marker write (collapsed redundant mkdir) - gsd2-import.cjs (1) — imported file write (collapsed redundant mkdir) - surface.cjs (1) — surface state write (replaced manual tmp+rename block) - installer-migrations.cjs (3) — journal init/finalize + rewrite-json action; deleted private writeFileAtomicSync helper and its three call sites Two sites intentionally retained outside the seam: - planning-workspace.cjs:241 — workspace lock (wx-flag atomic-create; previously excluded by Phase 3) - installer-migrations.cjs:220 — install migration lock (fd write into wx-opened handle) - writeInstallState (installer-migrations.cjs) — strict atomic contract for install state; the seam's fallback-to-direct-write on rename failure would silently violate the invariant that install state must never be left half-written. Inline tmp+rename with rethrow keeps the original guarantee. Tests: 9059 / 9041 / 18 — exactly the pre-Phase-4 baseline; 18 failures are the pre-existing path-with-spaces local-env issues, identical files as before. Co-Authored-By: Claude Opus 4.7 * fix(installer-migrations): use strict atomic write for rollback install-state restore The rollback path was restoring INSTALL_STATE via platformWriteSync, which falls back to a direct write on rename failure and would silently violate the half-written invariant that the install-state contract guarantees elsewhere. Extracts the strict tmp+rename logic from writeInstallState into a shared atomicWriteInstallState(configDir, content) helper and routes both writeInstallState and rollbackAppliedMigrationResult through it. Preserves the existing null-handling (rmSync when previousInstallStateBytes === null) and existing failure-collection (failures.push on caught errors). Byte-faithful restore: previousInstallStateBytes is written as-is (no JSON parse round-trip), preserving the exact prior file contents on restore. Co-Authored-By: Claude Opus 4.7 --------- Co-authored-by: Claude Sonnet 4.6 --- .changeset/shell-projection-cleanup.md | 5 + .../0009-shell-command-projection-module.md | 20 +++ docs/adr/0010-file-operation-engine-module.md | 7 +- get-shit-done/bin/lib/core.cjs | 155 ------------------ get-shit-done/bin/lib/drift.cjs | 3 +- get-shit-done/bin/lib/graphify.cjs | 7 +- get-shit-done/bin/lib/gsd2-import.cjs | 4 +- get-shit-done/bin/lib/install-profiles.cjs | 4 +- .../bin/lib/installer-migrations.cjs | 45 ++--- get-shit-done/bin/lib/learnings.cjs | 3 +- get-shit-done/bin/lib/profile-pipeline.cjs | 2 +- get-shit-done/bin/lib/surface.cjs | 9 +- get-shit-done/bin/lib/template.cjs | 5 +- tests/atomic-write.test.cjs | 78 --------- tests/concurrency-safety.test.cjs | 8 +- tests/core.test.cjs | 135 --------------- 16 files changed, 77 insertions(+), 413 deletions(-) create mode 100644 .changeset/shell-projection-cleanup.md delete mode 100644 tests/atomic-write.test.cjs diff --git a/.changeset/shell-projection-cleanup.md b/.changeset/shell-projection-cleanup.md new file mode 100644 index 000000000..a4d6feb83 --- /dev/null +++ b/.changeset/shell-projection-cleanup.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3468 +--- +Remove the now-unused legacy I/O wrappers (`atomicWriteFileSync`, `safeReadFile`, `normalizeMd`) from `core.cjs` after Phase 3 migrated every call site to the `shell-command-projection` seam. Migrates 3 stragglers Phase 3 missed (`graphify.cjs`, `template.cjs`, dead `safeReadFile` import in `profile-pipeline.cjs`). Wrapper-specific tests retired or repointed at the seam; behavioral / snapshot / perf regression coverage for markdown normalization preserved via `normalizeContent`. See #3468. diff --git a/docs/adr/0009-shell-command-projection-module.md b/docs/adr/0009-shell-command-projection-module.md index 425368d9b..3af096e8a 100644 --- a/docs/adr/0009-shell-command-projection-module.md +++ b/docs/adr/0009-shell-command-projection-module.md @@ -109,3 +109,23 @@ projectShellScript({ shell: 'cmd' | 'pwsh' | 'sh', executable, argsTemplate }) - Related bug history: `#2376`, `#2979`, `#3002`, `#3011`, `#3017`, `#3020`, `#3082`, `#3181`, `#3393`, `#3413` - See `0005-sdk-architecture-seam-map.md` - See `0008-installer-migration-module.md` + +## Update — 2026-05-13 (Phases 1–4 expansion, `#3465`–`#3468`) + +The seam grew beyond the original "rendering only" scope. The "does not become a generic command runner" and "does not replace safe internal subprocess APIs" constraints (Decision §17, Initial Scope §33) were intentionally superseded. + +**Scope now owned by `shell-command-projection.cjs`:** + +- runtime-aware command-text rendering (original ADR scope) +- subprocess dispatch — `execGit`, `execNpm`, `execTool`, `probeTty` (Phase 2, `#3466`) +- platform file I/O — `platformWriteSync`, `platformReadSync`, `platformEnsureDir`, `normalizeContent` (Phase 3, `#3467`) +- legacy wrappers `atomicWriteFileSync` / `safeReadFile` / `normalizeMd` removed from `core.cjs` (Phase 4, `#3468`) + +**Result-shape invariant:** all `exec*` return `{ exitCode, stdout, stderr }` and never throw on non-zero exit. Platform-conditional logic (`shell: process.platform === 'win32'`, `probeTty` Windows null return, `.md`-aware normalization) lives only at the seam. + +**Open question resolutions:** + +- Q4 (installer-only vs shared seam): **resolved — shared.** The seam lives in `get-shit-done/bin/lib/`, consumed by installer, planning workflow, and every fs/subprocess call site across the tool. +- Q1, Q2, Q3 (`hooks.shell_preference`, Windows Git Bash modeling, shim/script builder migration timing): unresolved, carried forward as projection-design concerns independent of the I/O expansion. + +See CONTEXT.md "Shell Command Projection Module" entry for the canonical current-state description. diff --git a/docs/adr/0010-file-operation-engine-module.md b/docs/adr/0010-file-operation-engine-module.md index a5bbd1b4a..295c219de 100644 --- a/docs/adr/0010-file-operation-engine-module.md +++ b/docs/adr/0010-file-operation-engine-module.md @@ -1,7 +1,12 @@ # File Operation Engine Module owns safe runtime/config file mutations -- **Status:** Proposed +- **Status:** Superseded by ADR-0009 (Shell Command Projection Module expansion, Phases 3–4, `#3467`–`#3468`) - **Date:** 2026-05-12 +- **Superseded:** 2026-05-13 + +> **Supersession note.** Rather than build a separate File Operation Engine, the file-mutation safety policy this ADR proposed was absorbed into the **Shell Command Projection Module** (ADR-0009). Phase 3 (`#3467`) added `platformWriteSync` / `platformReadSync` / `platformEnsureDir` / `normalizeContent` to that seam, owning atomic write (tmp+rename), `.md` normalization, and directory creation as a single platform-conditional surface. Phase 4 (`#3468`) removed the duplicated `atomicWriteFileSync` / `safeReadFile` / `normalizeMd` wrappers from `core.cjs`. The `applyFileMutationPlan` / typed plan IR design proposed below was not built — the simpler per-call seam proved sufficient for the actual drift sites. Lock-file lifecycle (Track B item 3) remains owned by `withPlanningLock` in `planning-workspace.cjs` because its `{ flag: 'wx' }` exclusive-create semantics differ from atomic-write rename semantics. + +--- We propose introducing a File Operation Engine Module that owns policy for managed file reads, writes, deletes, locks, backups, and rollbacks across installer, migration, and planning surfaces. Today, file mutation behavior is duplicated across `bin/install.js`, `get-shit-done/bin/lib/installer-migrations.cjs`, and multiple planning modules, with drift in atomic-write guarantees, path safety checks, and ownership classification. diff --git a/get-shit-done/bin/lib/core.cjs b/get-shit-done/bin/lib/core.cjs index 14cd62c5d..71bab815b 100644 --- a/get-shit-done/bin/lib/core.cjs +++ b/get-shit-done/bin/lib/core.cjs @@ -278,14 +278,6 @@ function error(message, reason = ERROR_REASON.UNKNOWN) { // ─── File & Config utilities ────────────────────────────────────────────────── -function safeReadFile(filePath) { - try { - return fs.readFileSync(filePath, 'utf-8'); - } catch { - return null; - } -} - /** * Canonical config defaults. Single source of truth — imported by config.cjs and verify.cjs. */ @@ -610,118 +602,6 @@ function isGitIgnored(cwd, targetPath) { return ignored; } -// ─── Markdown normalization ───────────────────────────────────────────────── - -/** - * Normalize markdown to fix common markdownlint violations. - * Applied at write points so GSD-generated .planning/ files are IDE-friendly. - * - * Rules enforced: - * MD022 — Blank lines around headings - * MD031 — Blank lines around fenced code blocks - * MD032 — Blank lines around lists - * MD012 — No multiple consecutive blank lines (collapsed to 2 max) - * MD047 — Files end with a single newline - */ -function normalizeMd(content) { - if (!content || typeof content !== 'string') return content; - - // Normalize line endings to LF for consistent processing - let text = content.replace(/\r\n/g, '\n'); - - const lines = text.split('\n'); - const result = []; - - // Pre-compute fence state in a single O(n) pass instead of O(n^2) per-line scanning - const fenceRegex = /^```/; - const insideFence = new Array(lines.length); - let fenceOpen = false; - for (let i = 0; i < lines.length; i++) { - if (fenceRegex.test(lines[i].trimEnd())) { - if (fenceOpen) { - // This is a closing fence — mark as NOT inside (it's the boundary) - insideFence[i] = false; - fenceOpen = false; - } else { - // This is an opening fence - insideFence[i] = false; - fenceOpen = true; - } - } else { - insideFence[i] = fenceOpen; - } - } - - for (let i = 0; i < lines.length; i++) { - const line = lines[i]; - const prev = i > 0 ? lines[i - 1] : ''; - const prevTrimmed = prev.trimEnd(); - const trimmed = line.trimEnd(); - const isFenceLine = fenceRegex.test(trimmed); - - // MD022: Blank line before headings (skip first line and frontmatter delimiters) - if (/^#{1,6}\s/.test(trimmed) && i > 0 && prevTrimmed !== '' && prevTrimmed !== '---') { - result.push(''); - } - - // MD031: Blank line before fenced code blocks (opening fences only) - if (isFenceLine && i > 0 && prevTrimmed !== '' && !insideFence[i] && (i === 0 || !insideFence[i - 1] || isFenceLine)) { - // Only add blank before opening fences (not closing ones) - if (i === 0 || !insideFence[i - 1]) { - result.push(''); - } - } - - // MD032: Blank line before lists (- item, * item, N. item, - [ ] item) - if (/^(\s*[-*+]\s|\s*\d+\.\s)/.test(line) && i > 0 && - prevTrimmed !== '' && !/^(\s*[-*+]\s|\s*\d+\.\s)/.test(prev) && - prevTrimmed !== '---') { - result.push(''); - } - - result.push(line); - - // MD022: Blank line after headings - if (/^#{1,6}\s/.test(trimmed) && i < lines.length - 1) { - const next = lines[i + 1]; - if (next !== undefined && next.trimEnd() !== '') { - result.push(''); - } - } - - // MD031: Blank line after closing fenced code blocks - if (/^```\s*$/.test(trimmed) && i > 0 && insideFence[i - 1] && i < lines.length - 1) { - const next = lines[i + 1]; - if (next !== undefined && next.trimEnd() !== '') { - result.push(''); - } - } - - // MD032: Blank line after last list item in a block - if (/^(\s*[-*+]\s|\s*\d+\.\s)/.test(line) && i < lines.length - 1) { - const next = lines[i + 1]; - if (next !== undefined && next.trimEnd() !== '' && - !/^(\s*[-*+]\s|\s*\d+\.\s)/.test(next) && - !/^\s/.test(next)) { - // Only add blank line if next line is not a continuation/indented line - result.push(''); - } - } - } - - text = result.join('\n'); - - // MD012: Collapse 3+ consecutive blank lines to 2 - text = text.replace(/\n{3,}/g, '\n\n'); - - // MD047: Ensure file ends with exactly one newline - text = text.replace(/\n*$/, '\n'); - - return text; -} - -// Default timeout for worktree-related git subprocess calls (matches worktree-safety.cjs). -// Prevents `git worktree list --porcelain` and similar calls from blocking the parent // ─── Common path helpers ────────────────────────────────────────────────────── /** @@ -1884,38 +1764,6 @@ 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); - } -} - /** * Format a Date as a fuzzy relative time string (e.g. "5 minutes ago"). * @param {Date} date @@ -1948,10 +1796,8 @@ module.exports = { ERROR_REASON, setJsonErrorMode, getJsonErrorMode, - safeReadFile, loadConfig, isGitIgnored, - normalizeMd, escapeRegex, normalizePhaseName, comparePhaseNum, @@ -1999,7 +1845,6 @@ module.exports = { readSubdirectories, getAgentsDir, checkAgentsInstalled, - atomicWriteFileSync, timeAgo, pruneOrphanedWorktrees, inspectWorktreeHealth, diff --git a/get-shit-done/bin/lib/drift.cjs b/get-shit-done/bin/lib/drift.cjs index c4639131a..084899f86 100644 --- a/get-shit-done/bin/lib/drift.cjs +++ b/get-shit-done/bin/lib/drift.cjs @@ -31,6 +31,7 @@ 'use strict'; const fs = require('node:fs'); +const { platformWriteSync } = require('./shell-command-projection.cjs'); // ─── Constants ─────────────────────────────────────────────────────────────── @@ -360,7 +361,7 @@ function writeMappedCommit(filePath, commitSha, isoDate) { const { data, body } = parseFrontmatter(content); data.last_mapped_commit = commitSha; if (isoDate) data.last_mapped_at = isoDate; - fs.writeFileSync(filePath, serializeFrontmatter(data, body)); + platformWriteSync(filePath, serializeFrontmatter(data, body)); } // ─── Exports ───────────────────────────────────────────────────────────────── diff --git a/get-shit-done/bin/lib/graphify.cjs b/get-shit-done/bin/lib/graphify.cjs index d9c1ed1ba..7dc6ecd1c 100644 --- a/get-shit-done/bin/lib/graphify.cjs +++ b/get-shit-done/bin/lib/graphify.cjs @@ -2,8 +2,7 @@ const fs = require('fs'); const path = require('path'); -const { execTool, execGit } = require('./shell-command-projection.cjs'); -const { atomicWriteFileSync } = require('./core.cjs'); +const { execTool, execGit, platformWriteSync } = require('./shell-command-projection.cjs'); // ─── Config Gate ───────────────────────────────────────────────────────────── @@ -523,7 +522,7 @@ function graphifyBuild(cwd) { /** * Write a diff snapshot after successful build (D-06). * Reads graph.json from .planning/graphs/ and writes .last-build-snapshot.json - * using atomicWriteFileSync for crash safety. + * using platformWriteSync for crash safety. * * @param {string} cwd - Working directory * @returns {object} @@ -541,7 +540,7 @@ function writeSnapshot(cwd) { }; const snapshotPath = path.join(cwd, '.planning', 'graphs', '.last-build-snapshot.json'); - atomicWriteFileSync(snapshotPath, JSON.stringify(snapshot, null, 2)); + platformWriteSync(snapshotPath, JSON.stringify(snapshot, null, 2)); return { saved: true, timestamp: snapshot.timestamp, diff --git a/get-shit-done/bin/lib/gsd2-import.cjs b/get-shit-done/bin/lib/gsd2-import.cjs index e8d602cdc..6b3a575de 100644 --- a/get-shit-done/bin/lib/gsd2-import.cjs +++ b/get-shit-done/bin/lib/gsd2-import.cjs @@ -19,6 +19,7 @@ const fs = require('node:fs'); const path = require('node:path'); +const { platformWriteSync } = require('./shell-command-projection.cjs'); // ─── Utilities ────────────────────────────────────────────────────────────── @@ -434,8 +435,7 @@ function buildPreview(gsd2Data, artifacts) { function writePlanningDir(artifacts, planningRoot) { for (const [rel, content] of artifacts) { const absPath = path.join(planningRoot, rel); - fs.mkdirSync(path.dirname(absPath), { recursive: true }); - fs.writeFileSync(absPath, content, 'utf8'); + platformWriteSync(absPath, content); } } diff --git a/get-shit-done/bin/lib/install-profiles.cjs b/get-shit-done/bin/lib/install-profiles.cjs index c0cfb7970..68c961eae 100644 --- a/get-shit-done/bin/lib/install-profiles.cjs +++ b/get-shit-done/bin/lib/install-profiles.cjs @@ -41,6 +41,7 @@ const fs = require('fs'); const path = require('path'); const os = require('os'); +const { platformWriteSync } = require('./shell-command-projection.cjs'); // --------------------------------------------------------------------------- // Profile definitions @@ -404,8 +405,7 @@ function readActiveProfile(runtimeConfigDir) { * @param {string} profileName e.g. 'core', 'standard', 'full' */ function writeActiveProfile(runtimeConfigDir, profileName) { - fs.mkdirSync(runtimeConfigDir, { recursive: true }); - fs.writeFileSync(path.join(runtimeConfigDir, PROFILE_MARKER_NAME), profileName + '\n', 'utf8'); + platformWriteSync(path.join(runtimeConfigDir, PROFILE_MARKER_NAME), profileName + '\n'); } // --------------------------------------------------------------------------- diff --git a/get-shit-done/bin/lib/installer-migrations.cjs b/get-shit-done/bin/lib/installer-migrations.cjs index c9d552984..18fe72be0 100644 --- a/get-shit-done/bin/lib/installer-migrations.cjs +++ b/get-shit-done/bin/lib/installer-migrations.cjs @@ -7,6 +7,7 @@ const { validateInstallerMigrationActions, validateInstallerMigrationRecord, } = require('./installer-migration-authoring.cjs'); +const { platformWriteSync } = require('./shell-command-projection.cjs'); const MANIFEST_NAME = 'gsd-file-manifest.json'; const INSTALL_STATE_NAME = 'gsd-install-state.json'; @@ -71,9 +72,24 @@ function readInstallState(configDir) { }; } -function writeInstallState(configDir, state) { +// Strict atomic write for the install state: must never be left half-written. +// Bypasses the seam because platformWriteSync falls back to a direct write on +// rename failure, which would silently violate this invariant. +function atomicWriteInstallState(configDir, content) { fs.mkdirSync(configDir, { recursive: true }); - writeFileAtomicSync(path.join(configDir, INSTALL_STATE_NAME), JSON.stringify(state, null, 2) + '\n'); + const filePath = path.join(configDir, INSTALL_STATE_NAME); + const tmpPath = `${filePath}.tmp-${process.pid}-${Date.now()}`; + try { + fs.writeFileSync(tmpPath, content, 'utf8'); + fs.renameSync(tmpPath, filePath); + } catch (error) { + try { fs.rmSync(tmpPath, { force: true }); } catch { /* best-effort */ } + throw error; + } +} + +function writeInstallState(configDir, state) { + atomicWriteInstallState(configDir, JSON.stringify(state, null, 2) + '\n'); return state; } @@ -263,20 +279,6 @@ function isStructurallyEmpty(value) { return typeof value === 'object' && Object.keys(value).length === 0; } -function writeFileAtomicSync(filePath, content) { - const tmpPath = `${filePath}.tmp-${process.pid}-${Date.now()}`; - try { - fs.writeFileSync(tmpPath, content, 'utf8'); - fs.renameSync(tmpPath, filePath); - } catch (error) { - try { - fs.rmSync(tmpPath, { force: true }); - } catch { - // best-effort cleanup only; preserve the original write failure - } - throw error; - } -} function journalAction(action, status, extras = {}) { const { value, ...safeAction } = action; @@ -424,8 +426,7 @@ function rollbackAppliedMigrationResult({ configDir, journal, journalPath, rollb if (previousInstallStateBytes === null) { fs.rmSync(path.join(configDir, INSTALL_STATE_NAME), { force: true }); } else { - fs.mkdirSync(configDir, { recursive: true }); - writeFileAtomicSync(path.join(configDir, INSTALL_STATE_NAME), previousInstallStateBytes); + atomicWriteInstallState(configDir, previousInstallStateBytes); } } catch (error) { failures.push({ relPath: INSTALL_STATE_NAME, error: error.message }); @@ -476,12 +477,12 @@ function applyInstallerMigrationPlan({ configDir, plan, now = () => new Date().t const rollback = []; const installStatePath = path.join(configDir, INSTALL_STATE_NAME); const previousInstallStateBytes = fs.existsSync(installStatePath) - ? fs.readFileSync(installStatePath) + ? fs.readFileSync(installStatePath, 'utf8') : null; try { fs.mkdirSync(path.dirname(journalPath), { recursive: true }); - fs.writeFileSync(journalPath, JSON.stringify(journal, null, 2) + '\n', 'utf8'); + platformWriteSync(journalPath, JSON.stringify(journal, null, 2) + '\n'); for (const action of plan.actions) { if ( @@ -517,7 +518,7 @@ function applyInstallerMigrationPlan({ configDir, plan, now = () => new Date().t rollbackRelPath: path.posix.join(rollbackRootRelPath, normalized), })); } else { - writeFileAtomicSync(fullPath, JSON.stringify(action.value, null, 2) + '\n'); + platformWriteSync(fullPath, JSON.stringify(action.value, null, 2) + '\n'); journal.actions.push(journalAction(action, 'rewritten', { rollbackRelPath: path.posix.join(rollbackRootRelPath, normalized), })); @@ -542,7 +543,7 @@ function applyInstallerMigrationPlan({ configDir, plan, now = () => new Date().t fs.rmSync(fullPath, { force: true }); } - fs.writeFileSync(journalPath, JSON.stringify(journal, null, 2) + '\n', 'utf8'); + platformWriteSync(journalPath, JSON.stringify(journal, null, 2) + '\n'); const state = readInstallState(configDir); const applied = appliedMigrationIds(state); diff --git a/get-shit-done/bin/lib/learnings.cjs b/get-shit-done/bin/lib/learnings.cjs index c8f052c7b..dc99db3c4 100644 --- a/get-shit-done/bin/lib/learnings.cjs +++ b/get-shit-done/bin/lib/learnings.cjs @@ -18,6 +18,7 @@ const path = require('path'); const crypto = require('crypto'); const os = require('os'); const { output, error: coreError } = require('./core.cjs'); +const { platformWriteSync } = require('./shell-command-projection.cjs'); // ─── Constants ─────────────────────────────────────────────────────────────── @@ -125,7 +126,7 @@ function learningsWrite(entry, opts) { content_hash: hash, }; - fs.writeFileSync(path.join(dir, `${id}.json`), JSON.stringify(record, null, 2), 'utf-8'); + platformWriteSync(path.join(dir, `${id}.json`), JSON.stringify(record, null, 2)); return { id, created: true, content_hash: hash }; } diff --git a/get-shit-done/bin/lib/profile-pipeline.cjs b/get-shit-done/bin/lib/profile-pipeline.cjs index acfc73d6a..137706947 100644 --- a/get-shit-done/bin/lib/profile-pipeline.cjs +++ b/get-shit-done/bin/lib/profile-pipeline.cjs @@ -12,7 +12,7 @@ const fs = require('fs'); const path = require('path'); const os = require('os'); const readline = require('readline'); -const { output, error, safeReadFile, reapStaleTempFiles } = require('./core.cjs'); +const { output, error, reapStaleTempFiles } = require('./core.cjs'); // ─── Session I/O Helpers ────────────────────────────────────────────────────── diff --git a/get-shit-done/bin/lib/surface.cjs b/get-shit-done/bin/lib/surface.cjs index 60ae71890..00168c5ce 100644 --- a/get-shit-done/bin/lib/surface.cjs +++ b/get-shit-done/bin/lib/surface.cjs @@ -20,6 +20,7 @@ const fs = require('fs'); const path = require('path'); const os = require('os'); +const { platformWriteSync } = require('./shell-command-projection.cjs'); const { readActiveProfile, @@ -74,17 +75,13 @@ function readSurface(runtimeConfigDir) { } /** - * Write the surface state atomically (write to tmp then rename). + * Write the surface state atomically via the platform seam (mkdir + tmp+rename). * * @param {string} runtimeConfigDir * @param {SurfaceState} surfaceState */ function writeSurface(runtimeConfigDir, surfaceState) { - fs.mkdirSync(runtimeConfigDir, { recursive: true }); - const finalPath = path.join(runtimeConfigDir, SURFACE_FILE_NAME); - const tmpPath = finalPath + '.tmp.' + process.pid; - fs.writeFileSync(tmpPath, JSON.stringify(surfaceState, null, 2) + '\n', 'utf8'); - fs.renameSync(tmpPath, finalPath); + platformWriteSync(path.join(runtimeConfigDir, SURFACE_FILE_NAME), JSON.stringify(surfaceState, null, 2) + '\n'); } // --------------------------------------------------------------------------- diff --git a/get-shit-done/bin/lib/template.cjs b/get-shit-done/bin/lib/template.cjs index 676396190..daaf8581f 100644 --- a/get-shit-done/bin/lib/template.cjs +++ b/get-shit-done/bin/lib/template.cjs @@ -4,7 +4,8 @@ const fs = require('fs'); const path = require('path'); -const { normalizePhaseName, findPhaseInternal, generateSlugInternal, normalizeMd, toPosixPath, output, error } = require('./core.cjs'); +const { normalizePhaseName, findPhaseInternal, generateSlugInternal, toPosixPath, output, error } = require('./core.cjs'); +const { platformWriteSync } = require('./shell-command-projection.cjs'); const { planningDir } = require('./planning-workspace.cjs'); const { reconstructFrontmatter } = require('./frontmatter.cjs'); @@ -219,7 +220,7 @@ function cmdTemplateFill(cwd, templateType, options, raw) { return; } - fs.writeFileSync(outPath, normalizeMd(fullContent), 'utf-8'); + platformWriteSync(outPath, fullContent); const relPath = toPosixPath(path.relative(cwd, outPath)); output({ created: true, path: relPath, template: templateType }, raw, relPath); } diff --git a/tests/atomic-write.test.cjs b/tests/atomic-write.test.cjs deleted file mode 100644 index 27fae200c..000000000 --- a/tests/atomic-write.test.cjs +++ /dev/null @@ -1,78 +0,0 @@ -/** - * 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'); - }); -}); diff --git a/tests/concurrency-safety.test.cjs b/tests/concurrency-safety.test.cjs index 5d09b9486..39a32e899 100644 --- a/tests/concurrency-safety.test.cjs +++ b/tests/concurrency-safety.test.cjs @@ -26,9 +26,11 @@ const { promisify } = require('util'); const { performance } = require('perf_hooks'); const { runGsdTools, createTempProject, cleanup, TOOLS_PATH } = require('./helpers.cjs'); -const { - normalizeMd, -} = require('../get-shit-done/bin/lib/core.cjs'); +const { normalizeContent } = require('../get-shit-done/bin/lib/shell-command-projection.cjs'); +// normalizeMd was removed from core.cjs (Phase 4 — issue #3468); the same algorithm now +// lives in the shell-command-projection seam. Wrap normalizeContent so existing +// behavioral / snapshot / perf assertions stay point-of-truth. +const normalizeMd = (input) => normalizeContent('test.md', input).content; const execAsync = promisify(exec); diff --git a/tests/core.test.cjs b/tests/core.test.cjs index 1ca6e50f8..699fdfce3 100644 --- a/tests/core.test.cjs +++ b/tests/core.test.cjs @@ -24,9 +24,7 @@ const { generateSlugInternal, normalizePhaseName, reapStaleTempFiles, - normalizeMd, comparePhaseNum, - safeReadFile, pathExistsInternal, getMilestoneInfo, getMilestonePhaseFilter, @@ -588,30 +586,6 @@ describe('generateSlugInternal', () => { // multi-level decimal, case-insensitive, directory-slug, and full sort order). // Removed duplicates here to keep a single authoritative test location. -// ─── safeReadFile ────────────────────────────────────────────────────────────── - -describe('safeReadFile', () => { - let tmpDir; - - beforeEach(() => { - tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-core-test-')); - }); - - afterEach(() => { - cleanup(tmpDir); - }); - - test('reads existing file', () => { - const filePath = path.join(tmpDir, 'test.txt'); - fs.writeFileSync(filePath, 'hello world'); - assert.strictEqual(safeReadFile(filePath), 'hello world'); - }); - - test('returns null for missing file', () => { - assert.strictEqual(safeReadFile('/nonexistent/path/file.txt'), null); - }); -}); - // ─── pathExistsInternal ──────────────────────────────────────────────────────── describe('pathExistsInternal', () => { @@ -1213,115 +1187,6 @@ describe('getMilestonePhaseFilter', () => { }); }); -// ─── normalizeMd ───────────────────────────────────────────────────────────── - -describe('normalizeMd', () => { - test('returns null/undefined/empty unchanged', () => { - assert.strictEqual(normalizeMd(null), null); - assert.strictEqual(normalizeMd(undefined), undefined); - assert.strictEqual(normalizeMd(''), ''); - }); - - test('MD022: adds blank lines around headings', () => { - const input = 'Some text\n## Heading\nMore text\n'; - const result = normalizeMd(input); - assert.ok(result.includes('\n\n## Heading\n\n'), 'heading should have blank lines around it'); - }); - - test('MD032: adds blank line before list after non-list content', () => { - const input = 'Some text\n- item 1\n- item 2\n'; - const result = normalizeMd(input); - assert.ok(result.includes('Some text\n\n- item 1'), 'list should have blank line before it'); - }); - - test('MD032: adds blank line after list before non-list content', () => { - const input = '- item 1\n- item 2\nSome text\n'; - const result = normalizeMd(input); - assert.ok(result.includes('- item 2\n\nSome text'), 'list should have blank line after it'); - }); - - test('MD032: does not add extra blank lines between list items', () => { - const input = '- item 1\n- item 2\n- item 3\n'; - const result = normalizeMd(input); - assert.ok(result.includes('- item 1\n- item 2\n- item 3'), 'consecutive list items should not get blank lines'); - }); - - test('MD031: adds blank lines around fenced code blocks', () => { - const input = 'Some text\n```js\ncode\n```\nMore text\n'; - const result = normalizeMd(input); - assert.ok(result.includes('Some text\n\n```js'), 'code block should have blank line before'); - assert.ok(result.includes('```\n\nMore text'), 'code block should have blank line after'); - }); - - test('MD012: collapses 3+ consecutive blank lines to 2', () => { - const input = 'Line 1\n\n\n\n\nLine 2\n'; - const result = normalizeMd(input); - assert.ok(!result.includes('\n\n\n'), 'should not have 3+ consecutive blank lines'); - assert.ok(result.includes('Line 1\n\nLine 2'), 'should collapse to double newline'); - }); - - test('MD047: ensures file ends with single newline', () => { - const input = 'Content'; - const result = normalizeMd(input); - assert.ok(result.endsWith('\n'), 'should end with newline'); - assert.ok(!result.endsWith('\n\n'), 'should not end with double newline'); - }); - - test('MD047: trims trailing multiple newlines', () => { - const input = 'Content\n\n\n'; - const result = normalizeMd(input); - assert.ok(result.endsWith('Content\n'), 'should end with single newline after content'); - }); - - test('preserves frontmatter delimiters', () => { - const input = '---\nkey: value\n---\n\n# Heading\n\nContent\n'; - const result = normalizeMd(input); - assert.ok(result.startsWith('---\n'), 'should preserve opening frontmatter'); - assert.ok(result.includes('---\n\n# Heading'), 'should preserve frontmatter closing'); - }); - - test('handles CRLF line endings', () => { - const input = 'Some text\r\n## Heading\r\nMore text\r\n'; - const result = normalizeMd(input); - assert.ok(!result.includes('\r'), 'should normalize to LF'); - assert.ok(result.includes('\n\n## Heading\n\n'), 'should add blank lines around heading'); - }); - - test('handles ordered lists', () => { - const input = 'Some text\n1. First\n2. Second\nMore text\n'; - const result = normalizeMd(input); - assert.ok(result.includes('Some text\n\n1. First'), 'ordered list should have blank line before'); - }); - - test('does not add blank line between table and list', () => { - const input = '| Col |\n|-----|\n| val |\n- item\n'; - const result = normalizeMd(input); - // Table rows start with |, should not add extra blank before list after table - assert.ok(result.includes('| val |\n\n- item'), 'list after table should have blank line'); - }); - - test('complex real-world STATE.md-like content', () => { - const input = [ - '# Project State', - '## Current Position', - 'Phase: 5 of 10', - 'Status: Executing', - '## Decisions', - '- Decision 1', - '- Decision 2', - '## Blockers', - 'None', - ].join('\n'); - const result = normalizeMd(input); - // Every heading should have blank lines around it - assert.ok(result.includes('\n\n## Current Position\n\n'), 'section heading needs blank lines'); - assert.ok(result.includes('\n\n## Decisions\n\n'), 'decisions heading needs blank lines'); - assert.ok(result.includes('\n\n## Blockers\n\n'), 'blockers heading needs blank lines'); - // List should have blank line before it - assert.ok(result.includes('\n\n- Decision 1'), 'list needs blank line before'); - }); -}); - // ─── Stale hook filter regression (#1200) ───────────────────────────────────── describe('stale hook filter', () => {