fix(settings): serialize settings.json writes so a quick-start cannot tear the file

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`.

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 |
 | ------ | ----------------------------- | ------------------------------------ | ----- |

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 `<data dir>/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.
 

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<string, unknown> | 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<string, unknown> | 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<Record<string, unknown>>(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<string, unknown> = {};
-      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);
     }
   });
 

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';

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<string, unknown> = {};
-        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));
       }
     }
 

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<string, unknown> = {};
-          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));
         }
       }