Merge pull request #719: fix(settings): serialize settings.json writes so a quick-start cannot tear the file

Conflict in src/web/routes/system-routes.ts (PUT /api/settings) resolved by
keeping both sides: #718's line that never stores an unauthorized
tunnelEnabled:true runs first, then #719's serialized mutateSettingsFile()
write replaces the inline read and plain writeFile.

test/routes/system-routes-tunnel-guard.test.ts: #718's new tests asserted
plain writeFile calls on settings.json. With #719 every write goes to a temp
file renamed over it, so the assertions read writes through a settingsWrites()
helper (writeFile contents keyed by their rename), and the concurrent-stop
harness applies a write and the stop at the rename.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Codeman maintainer committed 2026-10-27 02:10:35 +01:00
commit 82ef8b1edd
20 files changed
+532 -131

No files matched your search

+75 -3
View File
@@ -10,6 +10,7 @@ import {
existsSync,
mkdirSync,
mkdtempSync,
readdirSync,
readFileSync,
realpathSync,
symlinkSync,
@@ -25,6 +26,7 @@ import { SETTINGS_PATH, resolveCasesDir } from '../../src/web/route-helpers.js';
import { caseSessionSettled, claudeProjectDirName } from '../../src/case-rename.js';
import { Session } from '../../src/session.js';
import { safeRmHomeTree } from '../mocks/index.js';
import { mutateSettingsFile } from '../../src/web/settings-writer.js';
const LINKED = () => dataPath('linked-cases.json');
const CASES_DIR = () => join(homedir(), 'codeman-cases');
@@ -286,6 +288,64 @@ describe('POST /api/cases/:name/rename (a linked case)', () => {
expect(settings()).toEqual({ caseOrder: ['a', 'proj', 'b'], lastUsedCase: 'proj' });
});
it('puts back only the keys it wrote, keeping a settings write made in between', async () => {
const { app } = await createRouteTestHarness(registerCaseRoutes);
const real = join(work, 'my-project');
mkdirSync(real);
writeFileSync(LINKED(), JSON.stringify({ proj: real }));
writeFileSync(SETTINGS_PATH, JSON.stringify({ caseOrder: ['a', 'proj', 'b'], lastUsedCase: 'proj' }));
chmodSync(LINKED(), 0o444);
// Another writer (a settings save) lands after the rename wrote settings.json and before the
// registry write fails.
const realWriteFile = fs.writeFile;
const spy = vi.spyOn(fs, 'writeFile').mockImplementation(async (file, data, options) => {
if (file === LINKED()) await mutateSettingsFile((current) => ({ ...current, agentSkillEnabled: true }));
return realWriteFile(file, data, options);
});
try {
const res = await rename(app, 'proj', { newName: 'renamed', renameFolder: true });
expect(res.statusCode).toBe(500);
} finally {
spy.mockRestore();
}
expect(settings()).toEqual({ caseOrder: ['a', 'proj', 'b'], lastUsedCase: 'proj', agentSkillEnabled: true });
});
it('refuses to rename while settings.json is not a JSON object, leaving the folder and the file alone', async () => {
const { app } = await createRouteTestHarness(registerCaseRoutes);
const oldDir = localCase('alpha');
const torn = '{\n "agentSkillEnabled": true\n}tail"\n}';
writeFileSync(SETTINGS_PATH, torn);
const res = await rename(app, 'alpha', { newName: 'beta' });
expect(res.statusCode).toBe(409);
expect(res.json()).toEqual({
success: false,
errorCode: 'CONFLICT',
error: `${SETTINGS_PATH} is not a valid JSON object, so this change was not saved (saving would have replaced every other setting in it). Fix or delete the file, or restore it from a config backup, then try again.`,
});
expect(existsSync(oldDir)).toBe(true);
expect(existsSync(join(CASES_DIR(), 'beta'))).toBe(false);
expect(readFileSync(SETTINGS_PATH, 'utf8')).toBe(torn);
});
it('checks settings.json before it stops any session, so none is ended for a rename that cannot complete', async () => {
const { app, ctx } = await createRouteTestHarness(registerCaseRoutes);
const oldDir = localCase('alpha');
const torn = '{\n "agentSkillEnabled": true\n}tail"\n}';
writeFileSync(SETTINGS_PATH, torn);
(ctx.sessions as Map<string, unknown>).set('s1', { id: 's1', name: 'w1', workingDir: oldDir });
const res = await rename(app, 'alpha', { newName: 'beta', stopSessions: true });
expect(res.statusCode).toBe(409);
expect(res.json().errorCode).toBe('CONFLICT');
expect(res.json().error).toContain('is not a valid JSON object');
expect(ctx.cleanupSession).not.toHaveBeenCalled();
expect(existsSync(oldDir)).toBe(true);
expect(existsSync(join(CASES_DIR(), 'beta'))).toBe(false);
expect(readFileSync(SETTINGS_PATH, 'utf8')).toBe(torn);
});
it('says where the folder is when it cannot be moved back', async () => {
const { app } = await createRouteTestHarness(registerCaseRoutes);
const real = join(work, 'my-project');
@@ -328,12 +388,24 @@ describe('POST /api/cases/:name/rename (a linked case)', () => {
mkdirSync(real);
writeFileSync(LINKED(), JSON.stringify({ proj: real, other: join(work, 'x') }));
writeFileSync(SETTINGS_PATH, JSON.stringify({ caseOrder: ['proj'] }));
chmodSync(SETTINGS_PATH, 0o444);
const res = await rename(app, 'proj', { newName: 'renamed', renameFolder: true });
chmodSync(SETTINGS_PATH, 0o644);
// settings.json is replaced by renaming a temp file over it, which a read-only file does not stop,
// so the failure is injected at that rename.
const realRename = fs.rename;
const spy = vi.spyOn(fs, 'rename').mockImplementation(async (from, to) => {
if (to === SETTINGS_PATH) throw Object.assign(new Error('permission denied'), { code: 'EACCES' });
return realRename(from, to);
});
let res;
try {
res = await rename(app, 'proj', { newName: 'renamed', renameFolder: true });
} finally {
spy.mockRestore();
}
expect(res.statusCode).toBe(500);
expect(existsSync(real)).toBe(true);
expect(linked()).toEqual({ proj: real, other: join(work, 'x') });
// The failed write leaves no temp file behind.
expect(readdirSync(dirname(SETTINGS_PATH)).filter((n) => n.startsWith('settings.json.'))).toEqual([]);
});
it('answers 404 for a linked case a non-admin cannot see in multi-user mode', async () => {
@@ -54,6 +54,11 @@ vi.mock('node:fs/promises', () => ({
default: {
readFile: vi.fn(async () => JSON.stringify(STORED)),
writeFile: vi.fn(async () => undefined),
// settings.json is written to a temp file and renamed over the real one (settings-writer.ts).
realpath: vi.fn(async (p: string) => p),
mkdir: vi.fn(async () => undefined),
rename: vi.fn(async () => undefined),
rm: vi.fn(async () => undefined),
},
}));
@@ -4,8 +4,8 @@
*
* PR #361 once made it reconcile `showPlanUsageLimits` on first read: if the
* key was absent, the route persisted `true`. But readJsonConfig() answers
* `{}` for ANY read failure (parse error, EACCES, EMFILE, a read landing inside
* PUT's non-atomic write), not only ENOENT, and every page load calls this
* `{}` for ANY read failure (parse error, EACCES, EMFILE, and at the time a read
* landing inside PUT's non-atomic write), not only ENOENT, and every page load calls this
* route, so one unlucky read replaced the whole settings file with a one-key
* file. The default now lives in the readers instead: an absent key means ON
* to readPlanUsageTelemetryEnabled() (pinned in test/hooks-config.test.ts) and
@@ -70,6 +70,11 @@ vi.mock('node:fs/promises', () => ({
default: {
readFile: vi.fn(async () => JSON.stringify(EXISTING_SETTINGS)),
writeFile: vi.fn(async () => undefined),
// settings.json is written to a temp file and renamed over the real one (settings-writer.ts).
realpath: vi.fn(async (p: string) => p),
mkdir: vi.fn(async () => undefined),
rename: vi.fn(async () => undefined),
rm: vi.fn(async () => undefined),
},
}));
+27 -5
View File
@@ -11,6 +11,11 @@ vi.mock('node:fs/promises', () => ({
default: {
readFile: vi.fn(async () => '{}'),
writeFile: vi.fn(async () => undefined),
// settings.json is written to a temp file and renamed over the real one (settings-writer.ts).
realpath: vi.fn(async (p: string) => p),
mkdir: vi.fn(async () => undefined),
rename: vi.fn(async () => undefined),
rm: vi.fn(async () => undefined),
},
}));
@@ -27,6 +32,18 @@ vi.mock('node:fs', async (importOriginal) => {
import fs from 'node:fs/promises';
import { SETTINGS_PATH } from '../../src/web/route-helpers.js';
const mockedWriteFile = vi.mocked(fs.writeFile);
const mockedRename = vi.mocked(fs.rename);
/**
* settings.json writes as [path, contents]. The writer (settings-writer.ts) writes each one to a
* temp file and renames it over settings.json, so a write counts once its rename lands.
*/
function settingsWrites(): [string, string][] {
return mockedRename.mock.calls.map(([from, to]) => {
const write = mockedWriteFile.mock.calls.find(([path]) => path === from);
return [String(to), String(write?.[1])];
});
}
const STATUS = { running: false, url: null };
@@ -94,7 +111,7 @@ describe('tunnel authentication policy', () => {
expect(res.statusCode).toBe(200);
expect(tunnel.start.mock.calls).toEqual([[harness.ctx.port, harness.ctx.https]]);
expect(tunnel.stop.mock.calls).toEqual([]);
expect(mockedWriteFile.mock.calls).toEqual([[SETTINGS_PATH, JSON.stringify({ tunnelEnabled: true }, null, 2)]]);
expect(settingsWrites()).toEqual([[SETTINGS_PATH, JSON.stringify({ tunnelEnabled: true }, null, 2)]]);
}
it('refuses the incident: network acknowledgement does not authorize a public tunnel', async () => {
@@ -154,7 +171,7 @@ describe('tunnel authentication policy', () => {
expect(res.statusCode).toBe(200);
expect(tunnel.start.mock.calls).toEqual([]);
expect(tunnel.stop.mock.calls).toEqual([[]]);
expect(mockedWriteFile.mock.calls).toEqual([[SETTINGS_PATH, JSON.stringify({ tunnelEnabled: false }, null, 2)]]);
expect(settingsWrites()).toEqual([[SETTINGS_PATH, JSON.stringify({ tunnelEnabled: false }, null, 2)]]);
});
it.each([
@@ -210,9 +227,13 @@ describe('tunnel enable authorization across a concurrent stop', () => {
expect(path).toBe(SETTINGS_PATH);
return JSON.stringify(storedSettings);
});
const pending = new Map<string, string>();
mockedWriteFile.mockImplementation(async (path, contents) => {
expect(path).toBe(SETTINGS_PATH);
storedSettings = JSON.parse(contents as string);
pending.set(String(path), contents as string);
});
mockedRename.mockImplementation(async (from, to) => {
expect(to).toBe(SETTINGS_PATH);
storedSettings = JSON.parse(pending.get(String(from)) as string);
// The stop becomes visible during the awaited write, regardless of isRunning() call count.
if (stopDuringWrite) running = false;
afterWrite?.();
@@ -229,6 +250,7 @@ describe('tunnel enable authorization across a concurrent stop', () => {
});
vi.mocked(fs.readFile).mockResolvedValue('{}');
mockedWriteFile.mockResolvedValue(undefined);
mockedRename.mockResolvedValue(undefined);
}
});
@@ -243,7 +265,7 @@ describe('tunnel enable authorization across a concurrent stop', () => {
const expected = { tunnelEnabled, subagentTrackingEnabled: false, autoNameSessions: false };
// Check every write so a transient unauthorized enable followed by a rollback also fails.
expect
.soft(mockedWriteFile.mock.calls.map(([path, contents]) => ({ path, settings: JSON.parse(contents as string) })))
.soft(settingsWrites().map(([path, contents]) => ({ path, settings: JSON.parse(contents) })))
.toEqual([{ path: SETTINGS_PATH, settings: expected }]);
expect.soft(storedSettings).toEqual(expected);
}
+5
View File
@@ -22,6 +22,11 @@ vi.mock('node:fs/promises', () => ({
default: {
readFile: vi.fn(async () => '{}'),
writeFile: vi.fn(async () => undefined),
// settings.json is written to a temp file and renamed over the real one (settings-writer.ts).
realpath: vi.fn(async (p: string) => p),
mkdir: vi.fn(async () => undefined),
rename: vi.fn(async () => undefined),
rm: vi.fn(async () => undefined),
},
}));
+3 -3
View File
@@ -53,7 +53,7 @@ async function send(method: string, path: string, headers: Record<string, string
return { status: res.status, body: (await res.json()) as Record<string, any> };
}
/** The quick-start write of lastUsedCase is fire-and-forget: poll for it briefly. */
/** Read settings.json until `predicate` holds (briefly), so a write that never comes fails on its value. */
async function settleSettings(predicate: (s: Record<string, unknown>) => boolean): Promise<Record<string, unknown>> {
let last: Record<string, unknown> = {};
for (let i = 0; i < 40; i++) {
@@ -61,7 +61,7 @@ async function settleSettings(predicate: (s: Record<string, unknown>) => boolean
last = JSON.parse(await readSettingsBytes());
if (predicate(last)) return last;
} catch {
// the write is not atomic: a read that lands between its truncate and its data is empty, so poll again
// not there yet, or not JSON: poll again
}
await new Promise((r) => setTimeout(r, 25));
}
@@ -127,7 +127,7 @@ describe('a non-admin and the server-wide settings.json', () => {
const mine = await send('POST', '/api/quick-start', bob, { caseName: 'bobproj' });
expect(mine.status).toBe(200);
expect(mine.body.success).toBe(true);
// Give a (wrong) fire-and-forget write every chance to land before checking.
// Give a (wrong) write every chance to land before checking.
await new Promise((r) => setTimeout(r, 200));
expect(JSON.parse(await readSettingsBytes()).lastUsedCase).toBe('rootcase');
@@ -0,0 +1,220 @@
/**
* @fileoverview Every writer of the server's settings.json is serialized, atomic, and refuses to
* overwrite a file that does not parse (live server, ephemeral port, real file on disk).
*
* The upstream CI flake in test/quick-start.test.ts ("accepts the setting via PUT, then injects")
* was a torn settings.json: `SyntaxError: Unexpected non-whitespace character after JSON at
* position 31`. Quick-start saved `lastUsedCase` with an un-awaited, non-atomic read-modify-write,
* PUT /api/settings ran its own unserialized one, and the two overlapping truncate-then-write
* calls left one write's bytes over the tail of the other. The PUT then read the torn file as `{}`
* and wrote back only its own keys, erasing every other setting.
*
* Writes to settings.json (and its temp files) are slowed down here, which is what a busy CI disk
* does: a writer that answers before its write lands, or two writers that both read before either
* writes, then fail deterministically instead of once in a while.
*/
import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest';
import fsp from 'node:fs/promises';
import {
lstatSync,
mkdirSync,
mkdtempSync,
readFileSync,
readlinkSync,
rmSync,
statSync,
symlinkSync,
writeFileSync,
} from 'node:fs';
import { tmpdir } from 'node:os';
import { join } from 'node:path';
import { WebServer } from '../src/web/server.js';
import { TmuxManager } from '../src/tmux-manager.js';
import { SETTINGS_PATH } from '../src/web/route-helpers.js';
import { getCasesDir } from '../src/config/cases-dir.js';
vi.spyOn(TmuxManager, 'isTmuxAvailable').mockReturnValue(true);
/** How long every write to settings.json (or a temp file beside it) is held before it starts. */
const SLOW_WRITE_MS = 200;
const realWriteFile = fsp.writeFile.bind(fsp);
vi.spyOn(fsp, 'writeFile').mockImplementation((async (file: unknown, data: unknown, options?: unknown) => {
if (typeof file === 'string' && file.startsWith(SETTINGS_PATH)) {
await new Promise((resolve) => setTimeout(resolve, SLOW_WRITE_MS));
}
return realWriteFile(file as string, data as string, options as undefined);
}) as typeof fsp.writeFile);
/** The bytes the CI run left behind: the PUT's 31-byte file over the tail of quick-start's longer one. */
const TORN = '{\n "agentSkillEnabled": true\n}' + 'rt-1761000000000"\n}';
const REFUSAL = {
success: false,
errorCode: 'CONFLICT',
error: `${SETTINGS_PATH} is not a valid JSON object, so this change was not saved (saving would have replaced every other setting in it). Fix or delete the file, or restore it from a config backup, then try again.`,
};
let server: WebServer;
const url = (p: string) => `http://localhost:${server.boundPort}${p}`;
const seed = (value: unknown) => writeFileSync(SETTINGS_PATH, JSON.stringify(value, null, 2));
const readSettings = () => JSON.parse(readFileSync(SETTINGS_PATH, 'utf-8')) as Record<string, unknown>;
async function send(method: string, path: string, body: unknown) {
const res = await fetch(url(path), {
method,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(body),
});
return { status: res.status, body: (await res.json()) as Record<string, unknown> };
}
let caseCounter = 0;
const newCaseName = () => `settings-race-${process.pid}-${++caseCounter}`;
beforeAll(async () => {
// The boot pass resets a persisted tunnelEnabled (the tunnel only starts from the UI); that write
// goes through the same writer and must keep every other key.
mkdirSync(join(SETTINGS_PATH, '..'), { recursive: true });
seed({ tunnelEnabled: true, caseOrder: ['boot'] });
server = new WebServer(0, false, true);
await server.start();
}, 30000);
afterAll(async () => {
await server?.stop();
vi.restoreAllMocks();
}, 30000);
describe('settings.json writes', () => {
it('resets tunnelEnabled at boot without dropping the other keys', () => {
expect(readSettings()).toEqual({ tunnelEnabled: false, caseOrder: ['boot'] });
});
describe('with a valid file', () => {
beforeEach(() => seed({ lastUsedCase: 'seeded' }));
it('answers a quick-start only after its lastUsedCase has landed', async () => {
const caseName = newCaseName();
const res = await send('POST', '/api/quick-start', { caseName });
expect(res.body.success).toBe(true);
expect(readSettings()).toEqual({ lastUsedCase: caseName });
});
it('answers a Ralph loop start only after its lastUsedCase has landed', async () => {
const caseName = newCaseName();
const res = await send('POST', '/api/ralph-loop/start', { caseName, taskDescription: 'x' });
expect(res.body.success).toBe(true);
expect(readSettings()).toEqual({ lastUsedCase: caseName });
});
it('keeps every key when quick-start, the settings PUT, model config and case order write at once', async () => {
const caseName = newCaseName();
const [quick, put, model, order] = await Promise.all([
send('POST', '/api/quick-start', { caseName }),
send('PUT', '/api/settings', { agentSkillEnabled: true }),
send('PUT', '/api/execution/model-config', { defaultModel: 'opus' }),
send('PUT', '/api/cases/order', { order: ['a', 'b'] }),
]);
expect(quick.body.success).toBe(true);
expect(put).toEqual({ status: 200, body: { success: true, data: {} } });
expect(model).toEqual({ status: 200, body: { success: true, data: {} } });
expect(order).toEqual({ status: 200, body: { success: true, data: { order: ['a', 'b'] } } });
expect(readSettings()).toEqual({
lastUsedCase: caseName,
agentSkillEnabled: true,
modelConfig: { defaultModel: 'opus' },
caseOrder: ['a', 'b'],
});
});
it('keeps a settings PUT that lands while a case rename rewrites caseOrder', async () => {
const oldName = newCaseName();
const newName = newCaseName();
mkdirSync(join(getCasesDir(), oldName), { recursive: true });
seed({ lastUsedCase: oldName, caseOrder: [oldName, 'other'] });
const [rename, put] = await Promise.all([
send('POST', `/api/cases/${oldName}/rename`, { newName }),
send('PUT', '/api/settings', { agentSkillEnabled: true }),
]);
expect(rename.body.success).toBe(true);
expect(put).toEqual({ status: 200, body: { success: true, data: {} } });
expect(readSettings()).toEqual({ lastUsedCase: newName, caseOrder: [newName, 'other'], agentSkillEnabled: true });
});
it('writes the file 0600, since it can hold an API key', async () => {
await send('PUT', '/api/settings', { agentSkillEnabled: true });
expect(statSync(SETTINGS_PATH).mode & 0o777).toBe(0o600);
});
});
it('starts from {} when the file is missing or empty', async () => {
rmSync(SETTINGS_PATH, { force: true });
expect(await send('PUT', '/api/settings', { agentSkillEnabled: true })).toEqual({
status: 200,
body: { success: true, data: {} },
});
expect(readFileSync(SETTINGS_PATH, 'utf-8')).toBe('{\n "agentSkillEnabled": true\n}');
writeFileSync(SETTINGS_PATH, ' \n');
await send('PUT', '/api/execution/model-config', { defaultModel: 'opus' });
expect(readSettings()).toEqual({ modelConfig: { defaultModel: 'opus' } });
});
it('writes through a symlinked settings.json to its target', async () => {
const elsewhere = mkdtempSync(join(tmpdir(), 'codeman-settings-link-'));
const target = join(elsewhere, 'settings.json');
writeFileSync(target, JSON.stringify({ lastUsedCase: 'linked' }));
rmSync(SETTINGS_PATH, { force: true });
symlinkSync(target, SETTINGS_PATH);
try {
await send('PUT', '/api/settings', { agentSkillEnabled: true });
expect(lstatSync(SETTINGS_PATH).isSymbolicLink()).toBe(true);
expect(readlinkSync(SETTINGS_PATH)).toBe(target);
expect(JSON.parse(readFileSync(target, 'utf-8'))).toEqual({ lastUsedCase: 'linked', agentSkillEnabled: true });
} finally {
rmSync(SETTINGS_PATH, { force: true });
rmSync(elsewhere, { recursive: true, force: true });
}
});
describe('refuses to overwrite a file that is not a JSON object, and leaves its bytes alone', () => {
for (const [label, bytes] of [
['a torn file', TORN],
['a JSON array', '[1, 2]'],
['JSON null', 'null'],
['a JSON string', '"settings"'],
] as const) {
it(`${label}: every settings write route answers 409 CONFLICT`, async () => {
writeFileSync(SETTINGS_PATH, bytes);
expect(await send('PUT', '/api/settings', { agentSkillEnabled: true })).toEqual({ status: 409, body: REFUSAL });
expect(await send('PUT', '/api/execution/model-config', { defaultModel: 'opus' })).toEqual({
status: 409,
body: REFUSAL,
});
expect(await send('PUT', '/api/cases/order', { order: ['a'] })).toEqual({ status: 409, body: REFUSAL });
expect(readFileSync(SETTINGS_PATH, 'utf-8')).toBe(bytes);
});
}
it('a torn file: quick-start and a Ralph loop start still succeed without touching it', async () => {
writeFileSync(SETTINGS_PATH, TORN);
expect((await send('POST', '/api/quick-start', { caseName: newCaseName() })).body.success).toBe(true);
expect(
(await send('POST', '/api/ralph-loop/start', { caseName: newCaseName(), taskDescription: 'x' })).body.success
).toBe(true);
expect(readFileSync(SETTINGS_PATH, 'utf-8')).toBe(TORN);
});
it('a torn file: a refusal does not wedge later writes once the file is fixed', async () => {
writeFileSync(SETTINGS_PATH, TORN);
expect((await send('PUT', '/api/settings', { agentSkillEnabled: true })).status).toBe(409);
seed({ lastUsedCase: 'fixed' });
expect(await send('PUT', '/api/settings', { agentSkillEnabled: true })).toEqual({
status: 200,
body: { success: true, data: {} },
});
expect(readSettings()).toEqual({ lastUsedCase: 'fixed', agentSkillEnabled: true });
});
});
});