fix(#1875): route installer settings/defaults writes through atomic, lock-guarded primitives (#3966)
* fix(#1875): route writeSettings through atomicWriteFileSync writeSettings is the sole writer of settings.json/settings.local.json for six runtimes and wrote them with a naked fs.writeFileSync. Hosts discard the entire settings file on any parse failure, so a crash mid-write cost the user every hook, permission, env var, and statusline they had — not just GSD's. Route it through the atomicWriteFileSync (temp+rename) already used elsewhere in the installer and already bound in this file. withWriteFailure in the migration integration harness matched only the final destination path, so an atomic write bypassed the injection entirely and turned a rollback assertion into a vacuous pass. It now also matches the .tmp- sibling. Refs open-gsd/gsd-core#1874 (F5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(#1875): add changeset fragment Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#1876): honor the readSettings null contract in the #338 local-merge leg readSettings returns null only for an unparseable file — its documented "preserve existing, don't touch" signal. The #338 migration coerced that null to {} and wrote the result back, so a settings.local.json with one stray comma lost all its non-GSD content on the next install. The guard stands the whole migration down rather than just the local write: skipping the merge while still stripping the shared file would destroy the GSD entries outright instead of relocating them. Aborting here reaches a pre-existing latent crash that the clobber had been masking. Both are fixed, with their own regression test: - the unparseable-settings guard returned bare `undefined` while all five sibling early exits return the full result shape, so installAllRuntimes' statusline lookup (results.find(r => r.runtime)) threw; - handleStatusline dereferences result.settings, which is null on every early exit, so the call site now falls through to the banner branch. Both crashes reproduce on unmodified next with a malformed settings.local.json and no migration involved. Refs open-gsd/gsd-core#1874 (F6) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(#1876): add changeset fragment Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#1877): lock and atomically write the machine-global ~/.gsd/defaults.json Every non-Claude install read-modify-writes ~/.gsd/defaults.json with no lock and two separate naked whole-file writes. The file is read by every runtime and project on the machine, so concurrent installs lost each other's key, and a crash in either write window truncated it — silently, because the read path swallows parse errors and treats a corrupt file as absent. Take the existing acquireInstallMigrationLock around the read-modify-write and apply both mutations in one atomicWriteFileSync. An install that changes nothing no longer rewrites the file at all. Existing semantics are unchanged: the explicit resolve_model_ids:true opt-in (#1569) and an existing "omit" are preserved, non-canonical values still default to "omit" (#1156), a pre-existing runtime string is preserved (#2395), the malformed-non-object recovery (#1657) stands, and both console lines still print when both keys change. The #2834 structural test sliced a fixed 1200-character window from the function source; the added lock comment pushed an asserted token past it. The window now tracks the function body, so a comment or guard cannot red it spuriously. Refs open-gsd/gsd-core#1874 (F18) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(#1877): add changeset fragment Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#1874): preserve target mode and create temp files exclusively in atomicWriteFileSync * fix(#1874): write the migration-lock payload through the exclusive descriptor * chore(#1874): bold-led changeset fragments + fragment for the hardening pass * chore(#1874): rename the shadowed lock-release catch binding * test(#1874): fix source-grep/try-finally violations, add missing fault-injection cases - drop the /TypeError/.test(stderr) source-grep assertion (exitCode already proves the installer didn't crash) - convert inline try/finally fs-mock restoration to t.after() across the F5/F18 test suites, per this repo's no-try/finally-in-test-body rule - add a rename-failure fault-injection case for atomicWriteFileSync - add a read-only-.gsd-directory fault-injection case for the F18 lock+write path - extract MAX_TEMP_FILE_ATTEMPTS constant, dedupe the partial-write-then-throw mock into a shared tests/helpers.cjs helper Found during this session's own Standards-axis code-review pass on resurrected PR #3385. * chore(#1874): reset changeset fragments to pr:0 placeholder The resurrected fragments carried the closed PR's number (3385). This is a new PR, so reset to the pr:0 placeholder and backfill the real number once gh pr create returns it, per CONTRIBUTING.md's PR Number Handling. * chore(#1874): backfill changeset PR number (#3966) --------- Co-authored-by: Richard Spiers <1355479+richardspiers@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
@@ -19,6 +19,8 @@ const fs = require('fs');
|
||||
const os = require('os');
|
||||
const path = require('path');
|
||||
|
||||
const { cleanup, mockPartialWriteThenThrow } = require('./helpers.cjs');
|
||||
|
||||
// ─── load real install.js exports once ───────────────────────────────────────
|
||||
//
|
||||
// install.js prints a banner at module-load time (outside its GSD_TEST_MODE
|
||||
@@ -40,7 +42,7 @@ let installExports;
|
||||
process.stdout.write = _origWrite;
|
||||
}
|
||||
}
|
||||
const { readSettings, stripJsonComments } = installExports;
|
||||
const { readSettings, writeSettings, stripJsonComments } = installExports;
|
||||
|
||||
// ─── tests ───────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -264,3 +266,160 @@ describe('readSettings: JSON null coalesced to empty, malformed warns (#1191)',
|
||||
assert.strictEqual(warnCalls.length, 0, 'no warning expected for absent file');
|
||||
});
|
||||
});
|
||||
|
||||
// ─── writeSettings durability (#1874 F5) ─────────────────────────────────────
|
||||
//
|
||||
// Claude Code discards the ENTIRE settings file on any parse failure, so a
|
||||
// truncated write costs the user every hook, permission, and statusline they
|
||||
// have — not just GSD's entries. writeSettings is the sole writer of this
|
||||
// surface for six runtimes, so it must never leave a partial file behind.
|
||||
|
||||
describe('writeSettings durability (#1874 F5)', () => {
|
||||
|
||||
// The user's existing settings: entries GSD does not own and must not lose.
|
||||
const PRIOR = JSON.stringify({
|
||||
permissions: { allow: ['Bash(npm test)'] },
|
||||
statusLine: { command: '/usr/local/bin/my-statusline' },
|
||||
env: { MY_TOKEN: 'keep-me' },
|
||||
}, null, 2) + '\n';
|
||||
|
||||
function withTmpDir(fn) {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-write-settings-'));
|
||||
try { return fn(dir); } finally { cleanup(dir); }
|
||||
}
|
||||
|
||||
test('a failure mid-write leaves the previous settings file intact', (t) => {
|
||||
withTmpDir((dir) => {
|
||||
const settingsPath = path.join(dir, 'settings.json');
|
||||
fs.writeFileSync(settingsPath, PRIOR);
|
||||
|
||||
// Simulate the crash window faithfully: the bytes that were written
|
||||
// before the failure DO land on disk, then the call fails. A mock that
|
||||
// merely throws would pass against a non-atomic writer, proving nothing.
|
||||
t.after(mockPartialWriteThenThrow(fs, undefined, 12, {
|
||||
code: 'ENOSPC',
|
||||
message: 'ENOSPC: no space left on device',
|
||||
}));
|
||||
|
||||
let threw = false;
|
||||
try {
|
||||
writeSettings(settingsPath, { hooks: { SessionStart: [] } });
|
||||
} catch {
|
||||
threw = true;
|
||||
}
|
||||
|
||||
assert.ok(threw, 'the write failure must propagate, not be swallowed');
|
||||
assert.strictEqual(
|
||||
fs.readFileSync(settingsPath, 'utf8'),
|
||||
PRIOR,
|
||||
'settings.json must be byte-identical to its pre-write contents'
|
||||
);
|
||||
// The real cost of the bug: a truncated file is discarded wholesale by
|
||||
// the host, so assert the user's non-GSD entries actually survive.
|
||||
const recovered = JSON.parse(fs.readFileSync(settingsPath, 'utf8'));
|
||||
assert.deepStrictEqual(recovered.permissions.allow, ['Bash(npm test)']);
|
||||
assert.strictEqual(recovered.env.MY_TOKEN, 'keep-me');
|
||||
});
|
||||
});
|
||||
|
||||
test('a failure mid-write leaves no temp file behind', (t) => {
|
||||
withTmpDir((dir) => {
|
||||
const settingsPath = path.join(dir, 'settings.json');
|
||||
fs.writeFileSync(settingsPath, PRIOR);
|
||||
|
||||
t.after(mockPartialWriteThenThrow(fs, undefined, 12, {
|
||||
code: 'EIO',
|
||||
message: 'simulated mid-write failure',
|
||||
}));
|
||||
try {
|
||||
writeSettings(settingsPath, { hooks: {} });
|
||||
} catch { /* expected */ }
|
||||
|
||||
assert.deepStrictEqual(
|
||||
fs.readdirSync(dir).sort(),
|
||||
['settings.json'],
|
||||
'no .tmp-* residue may survive a failed write'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
test('a successful write produces the same bytes as before (format contract)', () => {
|
||||
withTmpDir((dir) => {
|
||||
const settingsPath = path.join(dir, 'settings.json');
|
||||
const settings = { hooks: { SessionStart: [{ hooks: [{ command: 'x' }] }] }, env: { A: '1' } };
|
||||
|
||||
writeSettings(settingsPath, settings);
|
||||
|
||||
// Two-space indent + trailing newline is the on-disk contract other
|
||||
// tooling (and users' diffs) depend on; atomicity must not disturb it.
|
||||
assert.strictEqual(
|
||||
fs.readFileSync(settingsPath, 'utf8'),
|
||||
JSON.stringify(settings, null, 2) + '\n'
|
||||
);
|
||||
assert.deepStrictEqual(readSettings(settingsPath), settings, 'must round-trip through readSettings');
|
||||
assert.deepStrictEqual(fs.readdirSync(dir), ['settings.json'], 'no temp residue on success');
|
||||
});
|
||||
});
|
||||
|
||||
test('hardened permissions survive the rewrite', () => {
|
||||
withTmpDir((dir) => {
|
||||
const settingsPath = path.join(dir, 'settings.json');
|
||||
fs.writeFileSync(settingsPath, PRIOR);
|
||||
// 0o600 is the hardened-secrets posture: settings.json can carry env
|
||||
// tokens, and rename() would otherwise swap in a umask-default inode.
|
||||
fs.chmodSync(settingsPath, 0o600);
|
||||
|
||||
writeSettings(settingsPath, { hooks: {}, env: { MY_TOKEN: 'keep-me' } });
|
||||
|
||||
assert.deepStrictEqual(
|
||||
readSettings(settingsPath),
|
||||
{ hooks: {}, env: { MY_TOKEN: 'keep-me' } },
|
||||
'the write must land through a chmod-hardened target on every OS'
|
||||
);
|
||||
if (process.platform !== 'win32') {
|
||||
assert.strictEqual(
|
||||
fs.statSync(settingsPath).mode & 0o7777,
|
||||
0o600,
|
||||
'a pre-existing non-default mode must survive the temp+rename write'
|
||||
);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
test('the temp file is created exclusively — a pre-planted symlink is not followed', { skip: process.platform === 'win32' }, (t) => {
|
||||
withTmpDir((dir) => {
|
||||
const settingsPath = path.join(dir, 'settings.json');
|
||||
fs.writeFileSync(settingsPath, PRIOR);
|
||||
const victimPath = path.join(dir, 'victim');
|
||||
fs.writeFileSync(victimPath, 'victim-bytes');
|
||||
|
||||
// Squat every plausible near-future temp path with a symlink to the
|
||||
// victim; an O_EXCL writer must skip them all instead of writing
|
||||
// through one.
|
||||
const planted = [];
|
||||
const origWriteFileSync = fs.writeFileSync;
|
||||
fs.writeFileSync = (target, data, options) => {
|
||||
const resolved = String(target);
|
||||
if (/\.tmp-\d+-\d+$/.test(resolved) && !fs.existsSync(resolved)) {
|
||||
try {
|
||||
fs.symlinkSync(victimPath, resolved);
|
||||
planted.push(resolved);
|
||||
} catch { /* already there */ }
|
||||
}
|
||||
return origWriteFileSync(target, data, options);
|
||||
};
|
||||
t.after(() => { fs.writeFileSync = origWriteFileSync; });
|
||||
assert.throws(
|
||||
() => writeSettings(settingsPath, { hooks: {} }),
|
||||
(e) => e.code === 'EEXIST',
|
||||
'an exclusive create must refuse every squatted temp path'
|
||||
);
|
||||
|
||||
assert.strictEqual(fs.readFileSync(victimPath, 'utf8'), 'victim-bytes',
|
||||
'the symlink target must never receive the settings payload');
|
||||
assert.strictEqual(fs.readFileSync(settingsPath, 'utf8'), PRIOR,
|
||||
'the settings file must be untouched when every temp path is squatted');
|
||||
for (const link of planted) fs.unlinkSync(link);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user