From 8574e3c8806bf202bdc8fc6f6a82457f242404fe Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Tue, 27 Oct 2026 01:24:15 +0100 Subject: [PATCH] fix: apply the review items for #719 at merge - src/web/settings-writer.ts:110: export assertSettingsWritable(), the writer's own read-for-write rule run on its own - src/web/routes/case-routes.ts:1860: an admin's rename checks settings.json before it ends any session or moves the folder, and answers 409 CONFLICT with the writer's message (placed before the session-starting check, so nothing is awaited between it and the lock) - test/routes/case-rename-routes.test.ts:314: the torn-file pin now expects 409 CONFLICT (no stoppedSessions, like the other pre-checks) - test/routes/case-rename-routes.test.ts:331: new test, a live session with stopSessions and a torn file: 409 CONFLICT, cleanupSession not called - docs/api-reference.md: the Settings paragraph says an admin's rename answers this refusal as 409 CONFLICT before ending sessions - src/hooks-config.ts:1161, src/prompt-cache-keepwarm.ts:341: comments describe the atomic writer, not an in-place rewrite Co-Authored-By: Claude Opus 5.5 --- docs/api-reference.md | 2 +- src/hooks-config.ts | 5 +++-- src/prompt-cache-keepwarm.ts | 8 +++++--- src/web/routes/case-routes.ts | 18 +++++++++++++++++- src/web/settings-writer.ts | 11 +++++++++++ test/routes/case-rename-routes.test.ts | 23 ++++++++++++++++++++--- 6 files changed, 57 insertions(+), 10 deletions(-) diff --git a/docs/api-reference.md b/docs/api-reference.md index 16836d2e..39681205 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. 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 `{}`. +`~/.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 an admin's `POST /api/cases/:name/rename` answers the same `409 CONFLICT`, checked before it ends any session or moves the folder. A missing or empty file still starts from `{}`. | Method | Path | Body | Notes | | ------ | ----------------------------- | ------------------------------------ | ----- | diff --git a/src/hooks-config.ts b/src/hooks-config.ts index f1bd7ebc..1cf2a9a2 100644 --- a/src/hooks-config.ts +++ b/src/hooks-config.ts @@ -1158,8 +1158,9 @@ export async function readKeepWarmSettings(): Promise { /** * The same two keys for the keep-warm service, which keeps what it reads until the next * settings change. A missing file is the defaults, but any other read or parse failure rejects - * rather than falling back: settings.json is rewritten in place, so a read can land mid-write, - * and a fallback kept from it would hold keep-warm off until the next keep-warm settings change. + * rather than falling back: settings.json is replaced atomically, so such a failure is a real + * read error or a file that does not parse (a hand edit), never a write in progress, and a + * fallback kept from it would hold keep-warm off until the next keep-warm settings change. */ export async function readKeepWarmSettingsOrThrow(): Promise { let text: string; diff --git a/src/prompt-cache-keepwarm.ts b/src/prompt-cache-keepwarm.ts index 2dd5e754..40080c34 100644 --- a/src/prompt-cache-keepwarm.ts +++ b/src/prompt-cache-keepwarm.ts @@ -338,9 +338,11 @@ export class PromptCacheKeepWarmService implements KeepWarmScheduler { try { settings = await this.readSettings(); } catch (err) { - // A read that lands mid-write is retried. A settings.json that stays broken (a hand edit - // with a trailing comma) fails every read until it is fixed, so it is logged once rather - // than once a minute per session, and only a session that could nudge retries. + // A failed read is retried (settings.json is replaced atomically, so a read never sees a + // half-written file; a failure is a read error or a file that does not parse). A settings.json + // that stays broken (a hand edit with a trailing comma) fails every read until it is fixed, so + // it is logged once rather than once a minute per session, and only a session that could + // nudge retries. const message = err instanceof Error ? err.message : String(err); if (message !== this.settingsFailure) { this.settingsFailure = message; diff --git a/src/web/routes/case-routes.ts b/src/web/routes/case-routes.ts index 4231185e..b0849767 100644 --- a/src/web/routes/case-routes.ts +++ b/src/web/routes/case-routes.ts @@ -78,7 +78,12 @@ import { parseBody, readJsonConfig, } from '../route-helpers.js'; -import { mutateSettingsFile, settingsWriteErrorResponse } from '../settings-writer.js'; +import { + assertSettingsWritable, + mutateSettingsFile, + SettingsWriteRefusedError, + settingsWriteErrorResponse, +} from '../settings-writer.js'; import { isMultiUserMode } from '../../config/multiuser.js'; import type { AuthUser } from '../../types.js'; import { SseEvent } from '../sse-events.js'; @@ -1850,6 +1855,17 @@ export function registerCaseRoutes( if (open.some((s) => !canAccessOwned(user, s.owner))) { return fail(403, ApiErrorCode.FORBIDDEN, "Another user's session has this case open"); } + // The admin's rename writes settings.json only after the sessions are ended and the folder moved, and + // the writer refuses a file that is not a JSON object: check it by the writer's own rule now, so no + // session is ended for a rename that cannot complete. Not between the starting check and the lock below. + if (isAdmin(req)) { + try { + await assertSettingsWritable(); + } catch (err) { + if (err instanceof SettingsWriteRefusedError) return fail(409, ApiErrorCode.CONFLICT, err.message); + throw err; + } + } // A session being created in the folder is not in ctx.sessions yet, so it could be neither listed nor // stopped: it would start in the old path after the move. Checked in the same tick that takes the lock // below (nothing is awaited in between), and the Session constructor refuses the folder from then on. diff --git a/src/web/settings-writer.ts b/src/web/settings-writer.ts index 24689291..d5bbaca4 100644 --- a/src/web/settings-writer.ts +++ b/src/web/settings-writer.ts @@ -100,6 +100,17 @@ export function mutateSettingsFile( return run; } +/** + * Rejects with `SettingsWriteRefusedError` when `mutateSettingsFile()` would refuse a write right + * now: the writer's own read-for-write rule, run on its own. For a route that does something it + * cannot take back (case rename ends sessions) before its settings write. A check only, outside + * the chain (writes are atomic, so it never reads a half-written file); the file can still change + * before the write. + */ +export async function assertSettingsWritable(): Promise { + await readSettingsForWrite(SETTINGS_PATH); +} + /** 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); diff --git a/test/routes/case-rename-routes.test.ts b/test/routes/case-rename-routes.test.ts index 8ef016da..2a28d08a 100644 --- a/test/routes/case-rename-routes.test.ts +++ b/test/routes/case-rename-routes.test.ts @@ -317,18 +317,35 @@ describe('POST /api/cases/:name/rename (a linked case)', () => { 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.statusCode).toBe(409); expect(res.json()).toEqual({ success: false, - errorCode: 'OPERATION_FAILED', + 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.`, - stoppedSessions: 0, }); 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).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');