From 8fa5b340c1c3b377588e572f6b5847c551954b32 Mon Sep 17 00:00:00 2001 From: JD Date: Mon, 26 Oct 2026 02:01:59 -0400 Subject: [PATCH] fix(settings): serialize settings.json writes so a quick-start cannot tear the file Quick-start saved lastUsedCase with an un-awaited, non-atomic read-modify-write, and the settings PUT, model config, case order, case rename, Ralph start and the boot tunnel reset each ran their own. Two overlapping truncate-then-write calls left one write's bytes over the tail of the other (the CI flake in test/quick-start.test.ts), and most writers then read the torn file as {} and wrote back only its own keys, erasing the rest of the user's settings. All of them now go through one writer that runs one change at a time, replaces the file atomically, and refuses (409 CONFLICT) to overwrite a file that does not parse. --- CLAUDE.md | 2 +- docs/api-reference.md | 2 +- docs/architecture-invariants.md | 2 +- src/web/routes/case-routes.ts | 49 ++-- src/web/routes/config-backup-routes.ts | 4 +- src/web/routes/ralph-routes.ts | 20 +- src/web/routes/session-routes.ts | 26 +-- src/web/routes/system-routes.ts | 43 +--- src/web/server.ts | 13 +- src/web/settings-writer.ts | 107 +++++++++ test/routes/case-rename-routes.test.ts | 61 ++++- .../system-routes-settings-admin-only.test.ts | 5 + ...es-settings-get-plan-usage-default.test.ts | 4 +- ...system-routes-settings-partial-put.test.ts | 5 + .../routes/system-routes-tunnel-guard.test.ts | 5 + test/routes/system-routes.test.ts | 5 + test/settings-admin-only-multiuser.test.ts | 6 +- .../settings-json-write-serialization.test.ts | 220 ++++++++++++++++++ 18 files changed, 458 insertions(+), 121 deletions(-) create mode 100644 src/web/settings-writer.ts create mode 100644 test/settings-json-write-serialization.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index bd2594acd..bc5459d9b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -302,7 +302,7 @@ One module per domain in `src/web/routes/` (plus a barrel; `ls src/web/routes/` - **API endpoint**: Types in `src/types/` domain file, route in `src/web/routes/*-routes.ts`. Return the `ApiResponse` envelope (`{ success: true, data }`; errors via `createErrorResponse()` with proper status code). Validate with Zod schemas in `schemas.ts`. - **SSE event**: Add to `src/web/sse-events.ts` + `SSE_EVENTS` in `constants.js`, emit via `broadcast()`, handle in `app.js` (`addListener(`) - **Session setting**: Add to `SessionState`, include in `session.toState()`, call `persistSessionState()` -- **App setting**: decide per-device vs synced first. Per-device keys go in the `displayKeys` set in settings-ui.js and must NOT be added to `SettingsUpdateSchema` (it is `.strict()`). ⚠️ Anything in `PUT /api/settings` that acts on a setting (the `toggleService` watcher calls) must resolve from **`merged`** (persisted + incoming), never from the raw request body: a partial PUT omits keys it doesn't intend to change, and `body.x ?? default` turns every omission into "apply the default" and silently resets live services. Pinned by `test/routes/system-routes-settings-partial-put.test.ts`. ⚠️ A synced setting is admin-only to write in multi-user mode (the PUT 403s a non-admin, whose browser does not send it): a new route that writes `settings.json` takes the same `isMultiUserMode() && !requireAdmin(req, reply)` gate. +- **App setting**: decide per-device vs synced first. Per-device keys go in the `displayKeys` set in settings-ui.js and must NOT be added to `SettingsUpdateSchema` (it is `.strict()`). ⚠️ Anything in `PUT /api/settings` that acts on a setting (the `toggleService` watcher calls) must resolve from **`merged`** (persisted + incoming), never from the raw request body: a partial PUT omits keys it doesn't intend to change, and `body.x ?? default` turns every omission into "apply the default" and silently resets live services. Pinned by `test/routes/system-routes-settings-partial-put.test.ts`. ⚠️ A synced setting is admin-only to write in multi-user mode (the PUT 403s a non-admin, whose browser does not send it): a new route that writes `settings.json` takes the same `isMultiUserMode() && !requireAdmin(req, reply)` gate. ⚠️ Every write to the server's `settings.json` goes through `mutateSettingsFile()` (`src/web/settings-writer.ts`: one at a time, tmp+rename, and a file that does not parse is refused, never read as `{}`), and a route awaits it before answering: two unserialized plain `writeFile`s tore the file, and the `{}` fallback then erased every other setting. The one direct writer is config-backup restore (`restoreConfigBackup()`, as for `clis.json`): it must work on a file that does not parse, which the writer refuses. Pinned by `test/settings-json-write-serialization.test.ts`. - **Hook event**: Add to `HookEventType`, add hook in `hooks-config.ts:generateHooksConfig()`, update `HookEventSchema` - **Mobile feature**: Add to relevant singleton, guard with `MobileDetection.isMobile()`. New header buttons must stay off phones (`test/mobile-header-buttons-policy.test.ts`). - **New test**: a test that constructs `WebServer` itself binds port 0 (`new WebServer(0, …)`, then `server.boundPort`), and a raw `http`/`net`/Fastify/`ws` server listens on `0` and reads `address().port`; mobile tests get theirs from `createTestServer()` and read `boundPort` too. `test/test-ports-guard.test.ts` refuses a fixed port in either. Route tests use `app.inject()` (no port needed); see `test/routes/_route-test-utils.ts`. diff --git a/docs/api-reference.md b/docs/api-reference.md index 95a28b178..16836d2e4 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -1213,7 +1213,7 @@ A local case whose folder is the top of a git repository can keep **worktrees** ## Settings -`~/.codeman/settings.json` is one file for the whole server: the synced App Settings (feature switches, the plan-usage collection switch `showPlanUsageLimits`, `runMode`, `respawnPresets`, `tunnelEnabled`, ...), plus `modelConfig`, `caseOrder` and `lastUsedCase`. Reads are open to every signed-in user; in multi-user mode a non-admin reads it without what is the admin's (`voiceSettings.apiKey`, the Deepgram key an admin saved, and the admin's case names in `caseOrder` and `lastUsedCase`). **In multi-user mode every write is admin only:** for a non-admin the routes below answer `403 FORBIDDEN` before the body is even validated, so nothing is written, no watcher is toggled and no tunnel is started. Single-user mode is unaffected. +`~/.codeman/settings.json` is one file for the whole server: the synced App Settings (feature switches, the plan-usage collection switch `showPlanUsageLimits`, `runMode`, `respawnPresets`, `tunnelEnabled`, ...), plus `modelConfig`, `caseOrder` and `lastUsedCase`. Reads are open to every signed-in user; in multi-user mode a non-admin reads it without what is the admin's (`voiceSettings.apiKey`, the Deepgram key an admin saved, and the admin's case names in `caseOrder` and `lastUsedCase`). **In multi-user mode every write is admin only:** for a non-admin the routes below answer `403 FORBIDDEN` before the body is even validated, so nothing is written, no watcher is toggled and no tunnel is started. Single-user mode is unaffected. Every write to the file (these routes, and the `lastUsedCase` that `POST /api/quick-start` and `POST /api/ralph-loop/start` record before they answer) runs one at a time and replaces the file atomically (mode `0600`); the one write outside that queue is a config-backup restore, which must work on a file that does not parse. A write to a `settings.json` that exists but is not a JSON object is refused rather than starting from `{}`, which would erase every other setting: the routes below answer `409 CONFLICT` with a message naming the fix and leave the file untouched, the two starts skip their `lastUsedCase`, and `POST /api/cases/:name/rename` fails and is undone like any other registry write failure. A missing or empty file still starts from `{}`. | Method | Path | Body | Notes | | ------ | ----------------------------- | ------------------------------------ | ----- | diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 7a743bb62..482a055d1 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -750,7 +750,7 @@ Matching semantics: a query containing `/` matches the relative path, otherwise ⚠️ **The origin is a random id in `/config-backup-id`, never the hostname.** Instances that share a folder (two machines on one synced folder, prod and a beta) must keep to their own snapshots, so the change check, list, prune, Compare and restore are all scoped to it; without that, each instance snapshotted on every tick and Restore offered the other machine's `users.json` and `.env`. The hostname was tried first and was wrong: a Docker Compose container gets a new one on every recreate, which unlisted every earlier snapshot, and since prune is origin-scoped they were never deleted either. The id is not the hook secret and is never tracked or restored. Listing never creates it (`readConfigBackupOrigin()`; only a pass or a restore does). A fresh or reinstalled data dir is a new owner; copying an `origin` from an old manifest into the file makes those snapshots visible again. -⚠️ **Retention survives a bad `settings.json`.** Each manifest records the retention in force. `runConfigBackup()` reads `settings.json` itself, because `readJsonConfig` answers `{}` for any failure, including a read that lands inside the non-atomic write of `PUT /api/settings`: a file that does not parse still gets its snapshot but prunes nothing, and a retention key ABSENT from the file (a reset) means what the newest manifest that recorded one holds (`withRecordedRetention()`, per key, in the folder the file names, so re-entering a custom folder after a reset finds that folder's own history). `GET /api/config-backups` reports the same values the pass uses, the form fills from them, and Save sends a retention key only when it was saved before or edited there (`_configBackupRetentionPayload()`), so merely displaying the recorded retention never writes it back. Restoring `settings.json` keeps the four `configBackup*` keys at their live values (an old `configBackupDir` would point the list at another folder and hide the undo copy just made), except a retention key the live file lacks, which keeps the backup's. While `settings.json` does not parse a pass reads it as `{}`, so its snapshot goes to the default folder; nothing is widened. +⚠️ **Retention survives a bad `settings.json`.** Each manifest records the retention in force. `runConfigBackup()` reads `settings.json` itself, because `readJsonConfig` answers `{}` for any failure, including a file that does not parse: a file that does not parse still gets its snapshot but prunes nothing, and a retention key ABSENT from the file (a reset) means what the newest manifest that recorded one holds (`withRecordedRetention()`, per key, in the folder the file names, so re-entering a custom folder after a reset finds that folder's own history). `GET /api/config-backups` reports the same values the pass uses, the form fills from them, and Save sends a retention key only when it was saved before or edited there (`_configBackupRetentionPayload()`), so merely displaying the recorded retention never writes it back. Restoring `settings.json` keeps the four `configBackup*` keys at their live values (an old `configBackupDir` would point the list at another folder and hide the undo copy just made), except a retention key the live file lacks, which keeps the backup's. While `settings.json` does not parse a pass reads it as `{}`, so its snapshot goes to the default folder; nothing is widened. ⚠️ **A custom folder is probed BOUNDED before anything reads it** (#516: it may be a network mount that stopped answering, and the GET runs every time App Settings opens). `probeBackupDir()` runs `probePathKind(dir, { pastCap: true })` before every pass and in every route, and `readEffectiveBackupSettings()` probes BEFORE the recorded-retention fallback lists the folder (that fallback once did a `readdirSync` first whenever only `configBackupDir` was set). `unknown` answers 503 `OPERATION_FAILED` through `describeUnknownPath()` and skips the pass; a missing custom folder is created only when its parent is present. The default folder needs no probe. `test/routes/config-backup-unreachable.test.ts` pins all four routes with a recording `node:fs` mock. diff --git a/src/web/routes/case-routes.ts b/src/web/routes/case-routes.ts index 7fbe7b5e2..4231185e1 100644 --- a/src/web/routes/case-routes.ts +++ b/src/web/routes/case-routes.ts @@ -78,6 +78,7 @@ import { parseBody, readJsonConfig, } from '../route-helpers.js'; +import { mutateSettingsFile, settingsWriteErrorResponse } from '../settings-writer.js'; import { isMultiUserMode } from '../../config/multiuser.js'; import type { AuthUser } from '../../types.js'; import { SseEvent } from '../sse-events.js'; @@ -1876,27 +1877,23 @@ export function registerCaseRoutes( } if (moveFolder) await moveCaseFolder(oldPath, newPath); const originalLinks = isLinked ? await readLinkedCases() : undefined; - // The settings.json content before this rename wrote it, set only once a write is attempted (an - // absent file reads as {}, which must not be written back as a file that never existed). - let originalSettings: Record | undefined; + // The settings.json values this rename replaced, set only once it wrote them. A failure puts back + // these keys alone, so a settings save that landed in between is kept. + let settingsUndo: Record | undefined; try { // settings.json is server-wide and caseOrder/lastUsedCase name the admin's cases, so only the // admin's rename touches it (a non-admin's own rename must not rewrite someone else's list). // Written before linked-cases.json so a failure here leaves the registry untouched. if (isAdmin(req)) { - const existing = await readJsonConfig>(SETTINGS_PATH, 'settings.json', {}); - const order = renameInOrder(existing.caseOrder, name, newName); - const last = existing.lastUsedCase === name ? newName : undefined; - if (order || last) { - originalSettings = existing; - await fs.writeFile( - SETTINGS_PATH, - JSON.stringify( - { ...existing, ...(order ? { caseOrder: order } : {}), ...(last ? { lastUsedCase: last } : {}) }, - null, - 2 - ) - ); + const { before, after } = await mutateSettingsFile((existing) => { + const order = renameInOrder(existing.caseOrder, name, newName); + const last = existing.lastUsedCase === name ? newName : undefined; + if (!order && !last) return undefined; + return { ...existing, ...(order ? { caseOrder: order } : {}), ...(last ? { lastUsedCase: last } : {}) }; + }); + if (after) { + const keys = ['caseOrder', 'lastUsedCase'].filter((k) => after[k] !== before[k]); + settingsUndo = Object.fromEntries(keys.map((k) => [k, before[k]])); } } if (originalLinks) { @@ -1913,9 +1910,8 @@ export function registerCaseRoutes( } catch (err) { // The folder moved but the registries did not follow: put settings.json and the folder back // rather than leave a case whose name and folder disagree. - if (originalSettings) { - await fs.writeFile(SETTINGS_PATH, JSON.stringify(originalSettings, null, 2)).catch(() => {}); - } + const undo = settingsUndo; + if (undo) await mutateSettingsFile((current) => ({ ...current, ...undo })).catch(() => {}); if (moveFolder) { const movedBack = await fs.rename(newPath, oldPath).then( () => true, @@ -1970,22 +1966,11 @@ export function registerCaseRoutes( const { order } = parseBody(CaseOrderSchema, req.body, 'Invalid order data'); try { - const dir = getDataDir(); - if (!existsSync(dir)) { - mkdirSync(dir, { recursive: true }); - } - let existing: Record = {}; - try { - existing = JSON.parse(await fs.readFile(SETTINGS_PATH, 'utf-8')); - } catch { - /* ignore */ - } - const merged = { ...existing, caseOrder: order }; - await fs.writeFile(SETTINGS_PATH, JSON.stringify(merged, null, 2)); + await mutateSettingsFile((current) => ({ ...current, caseOrder: order })); ctx.broadcast(SseEvent.CaseOrderChanged, { order }); return { success: true, data: { order } }; } catch (err) { - return createErrorResponse(ApiErrorCode.OPERATION_FAILED, getErrorMessage(err)); + return settingsWriteErrorResponse(err); } }); diff --git a/src/web/routes/config-backup-routes.ts b/src/web/routes/config-backup-routes.ts index 5cf78eba7..a59d852d4 100644 --- a/src/web/routes/config-backup-routes.ts +++ b/src/web/routes/config-backup-routes.ts @@ -128,8 +128,8 @@ export async function runConfigBackup( reason: string, opts: { force?: boolean } = {} ): Promise<{ result: ConfigBackupResult; removed: string[] } | null> { - // Read settings.json directly: readJsonConfig answers {} for ANY failure, so a syntax error or a read - // that lands inside PUT /api/settings's non-atomic write used to prune with the defaults (20 / 30 + // Read settings.json directly: readJsonConfig answers {} for ANY failure, so a syntax error (or, before + // settings writes became atomic, a read that landed inside one) used to prune with the defaults (20 / 30 // days) and drop the longer history the user asked to keep. const raw = await readSettingsForBackup(); const unreadable = raw === 'unreadable'; diff --git a/src/web/routes/ralph-routes.ts b/src/web/routes/ralph-routes.ts index 0f624a19b..034568729 100644 --- a/src/web/routes/ralph-routes.ts +++ b/src/web/routes/ralph-routes.ts @@ -5,7 +5,7 @@ */ import { FastifyInstance } from 'fastify'; -import { join, dirname, resolve, relative, isAbsolute } from 'node:path'; +import { join, resolve, relative, isAbsolute } from 'node:path'; import { existsSync, mkdirSync, writeFileSync } from 'node:fs'; import fs from 'node:fs/promises'; import { ApiErrorCode, createErrorResponse, getErrorMessage, type ApiResponse } from '../../types.js'; @@ -20,10 +20,10 @@ import { ownerFor, resolveCasesDir, sessionCapacityMessage, - SETTINGS_PATH, findSessionOrFail, parseBody, } from '../route-helpers.js'; +import { mutateSettingsFile } from '../settings-writer.js'; import { resolveClaudeModeForUsername } from '../../user-store.js'; import { writeHooksConfig, stripCaseEnvKeys } from '../../hooks-config.js'; import { generateClaudeMd } from '../../templates/claude-md.js'; @@ -470,18 +470,10 @@ export function registerRalphRoutes( // in single-user mode. if (isAdmin(req)) { try { - let settings: Record = {}; - try { - settings = JSON.parse(await fs.readFile(SETTINGS_PATH, 'utf-8')); - } catch { - /* ignore */ - } - settings.lastUsedCase = caseName; - const dir = dirname(SETTINGS_PATH); - if (!existsSync(dir)) mkdirSync(dir, { recursive: true }); - fs.writeFile(SETTINGS_PATH, JSON.stringify(settings, null, 2)).catch(() => {}); // Ignore - persisting lastUsedCase is non-critical - } catch { - /* non-critical */ + await mutateSettingsFile((settings) => ({ ...settings, lastUsedCase: caseName })); + } catch (err) { + // Non-critical: the loop starts regardless. + console.warn('[Server] Failed to save settings (lastUsedCase):', getErrorMessage(err)); } } diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 7569a4dcb..ff8a68908 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -114,11 +114,11 @@ import { resolveCasesDir, resolveParentSessionId, sessionCapacityMessage, - SETTINGS_PATH, validatePathWithinBase, waitCapacityResponse, abortOnClientHangUp, } from '../route-helpers.js'; +import { mutateSettingsFile } from '../settings-writer.js'; import { buildAgentCaseMarker, writeAgentCaseMarker } from '../../agent-case-marker.js'; import { agentInbox } from '../agent-inbox.js'; import { canUsernameRunPrivilegedCommands, resolveClaudeModeForUsername } from '../../user-store.js'; @@ -4576,28 +4576,14 @@ export function registerSessionRoutes( // settings.json, readable by every user through GET /api/settings, so a non-admin's // case name is not written there in multi-user mode (writes are admin-only, like // PUT /api/settings). isAdmin is always true in single-user mode. + // Awaited and serialized with every other settings.json writer (mutateSettingsFile): the + // un-awaited write this replaced could land after the response, over a settings save. if (isAdmin(req)) { try { - const settingsFilePath = SETTINGS_PATH; - let settings: Record = {}; - try { - settings = JSON.parse(await fs.readFile(settingsFilePath, 'utf-8')); - } catch (err) { - if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; - } - settings.lastUsedCase = caseName; - const dir = dirname(settingsFilePath); - if (!existsSync(dir)) { - mkdirSync(dir, { recursive: true }); - } - // Use async write to avoid blocking event loop - fs.writeFile(settingsFilePath, JSON.stringify(settings, null, 2)).catch((err) => { - // Non-critical but log for debugging - console.warn('[Server] Failed to save settings (lastUsedCase):', err); - }); + await mutateSettingsFile((settings) => ({ ...settings, lastUsedCase: caseName })); } catch (err) { - // Non-critical but log for debugging - console.warn('[Server] Failed to prepare settings update:', err); + // Non-critical: the session is already running. + console.warn('[Server] Failed to save settings (lastUsedCase):', getErrorMessage(err)); } } diff --git a/src/web/routes/system-routes.ts b/src/web/routes/system-routes.ts index 1ebbef006..8e1659f70 100644 --- a/src/web/routes/system-routes.ts +++ b/src/web/routes/system-routes.ts @@ -48,6 +48,7 @@ import { toggleService, SETTINGS_PATH, } from '../route-helpers.js'; +import { mutateSettingsFile, settingsWriteErrorResponse } from '../settings-writer.js'; import { SseEvent } from '../sse-events.js'; import { getInstallInfo, checkForUpdate, startUpdate, getUpdateStatusForApi } from '../self-update.js'; @@ -1032,11 +1033,10 @@ export function registerSystemRoutes( app.get('/api/settings', async (req) => { // A plain read. This route must NEVER write settings.json: readJsonConfig() - // answers `{}` for ANY read failure (a parse error, EACCES, EMFILE, a read - // that lands inside PUT's non-atomic write), not only for a missing file, - // and every page load calls this route, so a "persist the default when the - // key is absent" reconcile here replaced a whole settings file with one key - // on the first unlucky read. The plan-usage default is resolved by the + // answers `{}` for ANY read failure (a parse error, EACCES, EMFILE), not + // only for a missing file, and every page load calls this route, so a + // "persist the default when the key is absent" reconcile here replaced a + // whole settings file with one key on the first unlucky read. The plan-usage default is resolved by the // READERS instead: an absent `showPlanUsageLimits` means ON to // readPlanUsageTelemetryEnabled() (hooks-config.ts), the same way an absent // `workspaceHooksEnabled` means ON, and the client resolves its own display @@ -1096,21 +1096,11 @@ export function registerSystemRoutes( } try { - const dir = dirname(SETTINGS_PATH); - if (!existsSync(dir)) { - mkdirSync(dir, { recursive: true }); - } - let existing: Record = {}; - try { - existing = JSON.parse(await fs.readFile(SETTINGS_PATH, 'utf-8')); - } catch { - /* ignore */ - } // acknowledgeUnauthTunnel is an ACTION field (not a stored setting) — strip // it before persisting so settings.json stays clean. const { acknowledgeUnauthTunnel, ...settingsToStore } = settings; + const { before: existing } = await mutateSettingsFile((current) => ({ ...current, ...settingsToStore })); const merged = { ...existing, ...settingsToStore }; - await fs.writeFile(SETTINGS_PATH, JSON.stringify(merged, null, 2)); // tmux 3.7+ resizes tracked panes; older versions apply this to new panes. // Already-evicted history cannot be recovered on either version. @@ -1196,7 +1186,7 @@ export function registerSystemRoutes( return {}; } catch (err) { - return createErrorResponse(ApiErrorCode.OPERATION_FAILED, getErrorMessage(err)); + return settingsWriteErrorResponse(err); } }); @@ -1214,25 +1204,10 @@ export function registerSystemRoutes( const modelConfig = parseBody(ModelConfigUpdateSchema, req.body, 'Invalid model config') as Record; try { - let existingSettings: Record = {}; - try { - const content = await fs.readFile(SETTINGS_PATH, 'utf-8'); - existingSettings = JSON.parse(content); - } catch { - // File doesn't exist yet, start fresh - } - - existingSettings.modelConfig = modelConfig; - - const dir = dirname(SETTINGS_PATH); - if (!existsSync(dir)) { - mkdirSync(dir, { recursive: true }); - } - await fs.writeFile(SETTINGS_PATH, JSON.stringify(existingSettings, null, 2)); - + await mutateSettingsFile((current) => ({ ...current, modelConfig })); return {}; } catch (err) { - return createErrorResponse(ApiErrorCode.OPERATION_FAILED, getErrorMessage(err)); + return settingsWriteErrorResponse(err); } }); diff --git a/src/web/server.ts b/src/web/server.ts index 2b3e582c4..4cbb13852 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -174,6 +174,7 @@ import { MAX_PASTE_IMAGE_BYTES } from '../config/buffer-limits.js'; import { resolveTerminalHistoryConfig } from '../config/terminal-history.js'; import { SseEvent } from './sse-events.js'; import { MAX_ROUTE_PARAM_LENGTH } from './route-helpers.js'; +import { mutateSettingsFile } from './settings-writer.js'; import { getLatestPlanUsage, setLatestCodexPlanUsage, setLatestCopilotPlanUsage } from './plan-usage-latest.js'; import { telemetrySignature } from '../usage-telemetry.js'; import { readCodexPlanUsage, resolveCodexBinaryPath } from '../utils/codex-cli-resolver.js'; @@ -3346,16 +3347,12 @@ export class WebServer extends EventEmitter { // Tunnel only starts when user clicks the toggle in the UI — never on boot. // Reset persisted tunnelEnabled so the UI toggle reflects actual state. if (await this.isTunnelEnabled()) { - const settingsPath = dataPath('settings.json'); try { - const content = await fs.readFile(settingsPath, 'utf-8'); - const settings = JSON.parse(content); - settings.tunnelEnabled = false; - await fs.writeFile(settingsPath, JSON.stringify(settings, null, 2)); - } catch { - /* ignore */ + await mutateSettingsFile((settings) => ({ ...settings, tunnelEnabled: false })); + console.log('Cloudflare tunnel setting reset (tunnel only starts on explicit UI toggle)'); + } catch (err) { + console.warn('Could not reset the Cloudflare tunnel setting:', getErrorMessage(err)); } - console.log('Cloudflare tunnel setting reset (tunnel only starts on explicit UI toggle)'); } // Start team watcher for agent team awareness (always on — lightweight polling) diff --git a/src/web/settings-writer.ts b/src/web/settings-writer.ts new file mode 100644 index 000000000..246892911 --- /dev/null +++ b/src/web/settings-writer.ts @@ -0,0 +1,107 @@ +/** + * @fileoverview The writer of the server's `settings.json` (`SETTINGS_PATH`). + * + * Every read-modify-write of that file goes through `mutateSettingsFile()`, the same shape as + * `mutateRegistryFile()` (config/cli-registry/registry-writer.ts). The one direct writer is + * config-backup restore (`restoreConfigBackup()`, as for `clis.json`): it must work on a file that + * does not parse, which this writer refuses. + * + * - **Serialized.** Mutations run one at a time on a single promise chain, each one reading, + * changing and writing before the next starts. The writers used to run unserialized with a + * plain truncate-then-write `fs.writeFile`, and quick-start did not even await its + * `lastUsedCase` write, so a quick-start followed by a settings save could tear the file (one + * write's bytes over the tail of a longer one) or drop the other writer's key. + * - **Atomic.** Each write goes to its own temp file beside the target and is renamed over it, + * so a reader never sees a half-written file. + * - **Refuses a file it must not trust.** Only a missing (or empty) file starts from `{}`. A file + * that cannot be read or does not parse is left alone and the write throws + * `SettingsWriteRefusedError`: the writers used to read such a file as `{}` and write back only + * their own keys, which erased every other setting. + */ + +import { randomUUID } from 'node:crypto'; +import fs from 'node:fs/promises'; +import { dirname } from 'node:path'; +import { ApiErrorCode, createErrorResponse, getErrorMessage, type ApiResponse } from '../types.js'; +import { SETTINGS_PATH } from './route-helpers.js'; + +export type SettingsFile = Record; + +/** A write refused because the existing `settings.json` must not be overwritten. The message is user-facing. */ +export class SettingsWriteRefusedError extends Error { + constructor(message: string) { + super(message); + this.name = 'SettingsWriteRefusedError'; + } +} + +async function readSettingsForWrite(path: string): Promise { + let raw: string; + try { + raw = await fs.readFile(path, 'utf-8'); + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'ENOENT') return {}; + throw new SettingsWriteRefusedError( + `Cannot read ${path} (${(err as Error).message}), so this change was not saved.` + ); + } + // An empty file holds no settings to lose. + if (raw.trim() === '') return {}; + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + parsed = undefined; + } + // The parser's own message is not repeated: it can quote the file, which may hold an API key. + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { + throw new SettingsWriteRefusedError( + `${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.` + ); + } + return parsed as SettingsFile; +} + +async function writeSettingsFile(path: string, settings: SettingsFile): Promise { + // Through a symlink, as the plain writeFile this replaces did: the rename lands on the real file. + const target = await fs.realpath(path).catch(() => path); + await fs.mkdir(dirname(target), { recursive: true }); + const tmp = `${target}.${process.pid}.${randomUUID()}.tmp`; + try { + await fs.writeFile(tmp, JSON.stringify(settings, null, 2), { mode: 0o600 }); + await fs.rename(tmp, target); + } catch (err) { + await fs.rm(tmp, { force: true }).catch(() => {}); + throw err; + } +} + +let mutationChain: Promise = Promise.resolve(); + +/** + * Run one change to `settings.json`. `fn` receives the current settings and returns the settings + * to write, or `undefined` to write nothing. Resolves with the settings before and after the + * change (`after` is `undefined` when nothing was written). A refused or failed mutation rejects + * its own caller only; the chain keeps going. + */ +export function mutateSettingsFile( + fn: (current: SettingsFile) => SettingsFile | undefined +): Promise<{ before: SettingsFile; after: SettingsFile | undefined }> { + const run = mutationChain.then(async () => { + const before = await readSettingsForWrite(SETTINGS_PATH).catch((err: unknown) => { + console.warn(`[settings] write refused: ${getErrorMessage(err)}`); + throw err; + }); + const after = fn({ ...before }); + if (after) await writeSettingsFile(SETTINGS_PATH, after); + return { before, after }; + }); + mutationChain = run.catch(() => {}); + return run; +} + +/** A refused write answers 409 CONFLICT with the fix; any other failure stays OPERATION_FAILED. */ +export function settingsWriteErrorResponse(err: unknown): ApiResponse { + if (err instanceof SettingsWriteRefusedError) return createErrorResponse(ApiErrorCode.CONFLICT, err.message); + return createErrorResponse(ApiErrorCode.OPERATION_FAILED, getErrorMessage(err)); +} diff --git a/test/routes/case-rename-routes.test.ts b/test/routes/case-rename-routes.test.ts index 94a587215..8ef016da7 100644 --- a/test/routes/case-rename-routes.test.ts +++ b/test/routes/case-rename-routes.test.ts @@ -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,47 @@ 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(500); + expect(res.json()).toEqual({ + success: false, + errorCode: 'OPERATION_FAILED', + 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.`, + stoppedSessions: 0, + }); + 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 +371,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 () => { diff --git a/test/routes/system-routes-settings-admin-only.test.ts b/test/routes/system-routes-settings-admin-only.test.ts index 49ea02cf7..0980862ca 100644 --- a/test/routes/system-routes-settings-admin-only.test.ts +++ b/test/routes/system-routes-settings-admin-only.test.ts @@ -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), }, })); diff --git a/test/routes/system-routes-settings-get-plan-usage-default.test.ts b/test/routes/system-routes-settings-get-plan-usage-default.test.ts index f2496d722..2e6b9ab33 100644 --- a/test/routes/system-routes-settings-get-plan-usage-default.test.ts +++ b/test/routes/system-routes-settings-get-plan-usage-default.test.ts @@ -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 diff --git a/test/routes/system-routes-settings-partial-put.test.ts b/test/routes/system-routes-settings-partial-put.test.ts index 416b1d646..afe22c853 100644 --- a/test/routes/system-routes-settings-partial-put.test.ts +++ b/test/routes/system-routes-settings-partial-put.test.ts @@ -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), }, })); diff --git a/test/routes/system-routes-tunnel-guard.test.ts b/test/routes/system-routes-tunnel-guard.test.ts index 8da2b6740..a03e48db7 100644 --- a/test/routes/system-routes-tunnel-guard.test.ts +++ b/test/routes/system-routes-tunnel-guard.test.ts @@ -20,6 +20,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), }, })); diff --git a/test/routes/system-routes.test.ts b/test/routes/system-routes.test.ts index a5550aedc..7b402fcde 100644 --- a/test/routes/system-routes.test.ts +++ b/test/routes/system-routes.test.ts @@ -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), }, })); diff --git a/test/settings-admin-only-multiuser.test.ts b/test/settings-admin-only-multiuser.test.ts index 533007ae4..e2407d430 100644 --- a/test/settings-admin-only-multiuser.test.ts +++ b/test/settings-admin-only-multiuser.test.ts @@ -53,7 +53,7 @@ async function send(method: string, path: string, headers: Record }; } -/** 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) => boolean): Promise> { let last: Record = {}; for (let i = 0; i < 40; i++) { @@ -61,7 +61,7 @@ async function settleSettings(predicate: (s: Record) => 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'); diff --git a/test/settings-json-write-serialization.test.ts b/test/settings-json-write-serialization.test.ts new file mode 100644 index 000000000..70b283c13 --- /dev/null +++ b/test/settings-json-write-serialization.test.ts @@ -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; + +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 }; +} + +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 }); + }); + }); +});