diff --git a/CLAUDE.md b/CLAUDE.md index b8afdd61..a9c052af 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -209,7 +209,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Auto-resume on usage limit** (opt-in per session, top of the Respawn tab): when Claude halts on a subscription limit, `usage-limit-patterns.ts` (pure, unit-tested) parses the reset time and `SessionAutoOps` arms a timer for reset+2min, then sends Esc + `continue`. ⚠️ Respawn cycles are blocked while paused (`isLimitPaused` guard in `onIdleDetected`), which is what prevents `/clear` from wiping the paused conversation. Claude-mode only. → [architecture-invariants#auto-resume-on-usage-limit](docs/architecture-invariants.md#auto-resume-on-usage-limit) -**Plan-usage chip** (`showPlanUsageLimits`, per-device: desktop default **ON**, handhelds OFF via the mobile block in `getDefaultSettings()`): resolve DISPLAY ONLY through `planUsageChipEnabled()` in settings-ui.js, which backs the two call sites that must never disagree (the App Settings checkbox, the chip's visibility). The SAME persisted setting also doubles as the server-side telemetry COLLECTION switch — `readPlanUsageTelemetryEnabled()` (hooks-config.ts) reads it fresh from `settings.json` at every claude session create/respawn (`TmuxManager.createSession`/`respawnPane`), so it applies uniformly to every claude-creation path (interactive Run, cron, Ralph Loop API, quick-start) with no per-session state and no per-request field — a Codeman restart cannot silently kill it (there is nothing per-session to lose). Claude data comes from Codeman's marked `statusLine.command` exporter (injected as an EPHEMERAL `claude --settings` CLI flag, never written to disk — see `resolveStatusLineCliCommand`), which POSTs `rate_limits` to `POST /api/status-telemetry`, never overwrites a user's hand-authored statusLine (it WRAPS it instead — `findEffectiveUserStatusLineCommand`), and prints the footer through. Main Codex usage comes from a read-only host `account/rateLimits/read` app-server poll at startup and every 5 minutes; exclude model-specific buckets such as Spark, and omit the Codex row when no signed-in limit is available. Distinct from auto-resume, which reacts to Claude's limit *message* rather than showing live %. → [architecture-invariants#plan-usage-chip-statusline-telemetry](docs/architecture-invariants.md#plan-usage-chip-statusline-telemetry), `docs/usage-limits-display-plan.md` +**Plan-usage chip** (`showPlanUsageLimits`, per-device: desktop default **ON**, handhelds OFF via the mobile block in `getDefaultSettings()`): resolve DISPLAY ONLY through `planUsageChipEnabled()` in settings-ui.js, which backs the two call sites that must never disagree (the App Settings checkbox, the chip's visibility). The SAME persisted setting also doubles as the server-side telemetry COLLECTION switch — `readPlanUsageTelemetryEnabled()` (hooks-config.ts) reads it fresh from `settings.json` at every claude session create/respawn (`TmuxManager.createSession`/`respawnPane`), so it applies uniformly to every claude-creation path (interactive Run, cron, Ralph Loop API, quick-start) with no per-session state and no per-request field — a Codeman restart cannot silently kill it (there is nothing per-session to lose). ⚠️ An ABSENT key reads as ON (mirroring `readWorkspaceHooksEnabled()`), so the default is resolved by the READER and `GET /api/settings` stays a plain read that never writes: a reconcile write there ran on every page load and could replace an unreadable `settings.json` with a one-key file. ⚠️ A save sends `showPlanUsageLimits` ONLY when it FLIPS the chip on that device (`planUsageCollectionFlip()` in settings-ui.js): the chip defaults OFF on handhelds, so a phone saving its font size used to persist `false` and switch collection off for every desktop. Claude data comes from Codeman's marked `statusLine.command` exporter (injected as an EPHEMERAL `claude --settings` CLI flag, never written to disk — see `resolveStatusLineCliCommand`), which POSTs `rate_limits` to `POST /api/status-telemetry`, never overwrites a user's hand-authored statusLine (it WRAPS it instead — `findEffectiveUserStatusLineCommand`), and prints the footer through. Main Codex usage comes from a read-only host `account/rateLimits/read` app-server poll at startup and every 5 minutes; exclude model-specific buckets such as Spark, and omit the Codex row when no signed-in limit is available. Distinct from auto-resume, which reacts to Claude's limit *message* rather than showing live %. → [architecture-invariants#plan-usage-chip-statusline-telemetry](docs/architecture-invariants.md#plan-usage-chip-statusline-telemetry), `docs/usage-limits-display-plan.md` **Orchestrator**: State machine that turns a user goal into a phased plan and drives it to completion: `idle → planning → approval → executing → verifying → (replanning) → completed/failed`. `OrchestratorLoop` (engine) delegates plan generation to `orchestrator-planner` and per-phase verification gates to `orchestrator-verifier`, executing phases via team agents/`task-queue`. State persists under the `orchestrator` key in `state.json`. Distinct from Ralph (single-session autonomous loop) — orchestrator coordinates multi-phase, multi-agent execution. See `docs/orchestrator-loop-architecture.md`. diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 167e358b..e515a26e 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -92,7 +92,7 @@ Tests: `test/docker-hosts.test.ts`, `test/docker-exec-options.test.ts`, `test/do ### Plan-usage chip (statusLine telemetry) -**Plan-usage chip** (`showPlanUsageLimits`, per-device: desktop default **ON** since 1.9.3, handhelds OFF) renders compact Claude and Codex provider rows. Claude Code (v2.1.80+) pipes a JSON blob to a configured `statusLine.command` on each render; on Pro/Max it carries a `rate_limits` object (`five_hour`/`seven_day` windows only — no Opus weekly field — each `{used_percentage 0-100, resets_at epoch-SECONDS}`). ⚠️ **Injected as an EPHEMERAL `claude --settings` CLI flag at spawn (2026-09-07), never written to disk** — `resolveStatusLineCliCommand()`/`ensureStatusLineExporterScript()` in `hooks-config.ts` (`generateStatusLineCommand()`/`applyStatusLineConfig()` remain, but only as the legacy disk-write self-heal path: a workspace an older Codeman build touched gets its stale `.claude/settings.local.json` entry stripped the first time a session starts there again). The exporter WRAPS a user's own real statusline (`findEffectiveUserStatusLineCommand()`, walking Claude Code's own settings precedence) rather than replacing it, and POSTs the `rate_limits` blob to `POST /api/status-telemetry`. That route (auth-exempt like `/api/hook-event` — localhost-only, hook-secret-gated whenever auth is active, COD-91) parses via `usage-telemetry.ts` (pure, unit-tested), broadcasts SSE `session:statusTelemetry` (de-duped per session by `telemetrySignature` since the statusline fires on every assistant message), and returns a compact plain-text footer for the exporter to **print-through** (foreground POST in the no-wrap branch so its own stdout becomes the footer, `|| echo codeman` on failure; backgrounded — `>/dev/null 2>&1 /dev/null 2>&1 **Status: SHIPPED — deployed to prod + pushed to master, not yet released (2026-06-14).** App Settings → Display → **Plan Usage Limits** (`showPlanUsageLimits`). **Default changed in 1.9.3: desktop now defaults ON, handhelds stay OFF, resolved via `planUsageChipEnabled()`.** The per-device notes further down describing it as opt-in/synced record the original 2026-06-14 shape, not current behavior. Commits `c82f6c8` (feature) → `4d9d93d` (end-to-end fixes) → `eae225b` (per-user reconcile) → `95fb5fc` (init-snapshot replay). Full suite green (2869), CI green. No changeset/version bump yet. > -> **2026-09-07 rework — the "Injection lifecycle" section below (disk-write reconcile via `applyStatusLineConfig`) is SUPERSEDED and describes the OLD mechanism, kept for history.** That disk write let a Codeman-marked `statusLine.command` in `.claude/settings.local.json` take precedence over the user's own global/project statusline for ANY `claude` run in that directory — including entirely outside Codeman — with no disclosure and no way to undo it (real bug, found 2026-08-31). The exporter is now injected as an EPHEMERAL `claude --settings` CLI flag at spawn (`resolveStatusLineCliCommand`/`ensureStatusLineExporterScript`, hooks-config.ts) — never written to disk — and it WRAPS the user's own real statusline (`findEffectiveUserStatusLineCommand`) rather than replacing it. `showPlanUsageLimits` now doubles as the telemetry COLLECTION switch too: `readPlanUsageTelemetryEnabled()` reads it fresh from `settings.json` at every claude session create/respawn (`TmuxManager.createSession`/`respawnPane`), so it applies uniformly across every claude-creation path — interactive Run, cron, the Ralph Loop API, quick-start — with no per-session state (a Codeman restart cannot silently kill it) and no per-request field on the wire at all. +> **2026-09-07 rework — the "Injection lifecycle" section below (disk-write reconcile via `applyStatusLineConfig`) is SUPERSEDED and describes the OLD mechanism, kept for history.** That disk write let a Codeman-marked `statusLine.command` in `.claude/settings.local.json` take precedence over the user's own global/project statusline for ANY `claude` run in that directory — including entirely outside Codeman — with no disclosure and no way to undo it (real bug, found 2026-08-31). The exporter is now injected as an EPHEMERAL `claude --settings` CLI flag at spawn (`resolveStatusLineCliCommand`/`ensureStatusLineExporterScript`, hooks-config.ts) — never written to disk — and it WRAPS the user's own real statusline (`findEffectiveUserStatusLineCommand`) rather than replacing it. `showPlanUsageLimits` now doubles as the telemetry COLLECTION switch too: `readPlanUsageTelemetryEnabled()` reads it fresh from `settings.json` at every claude session create/respawn (`TmuxManager.createSession`/`respawnPane`), so it applies uniformly across every claude-creation path — interactive Run, cron, the Ralph Loop API, quick-start — with no per-session state (a Codeman restart cannot silently kill it) and no per-request field on the wire at all. An absent key reads as ON (the reader resolves the default; `GET /api/settings` never writes), and a settings save carries the key only when it flips the chip on that device, so a handheld with the chip off cannot switch collection off for a desktop by saving something unrelated. The exporter prints nothing on failure rather than the bare word `codeman` (discussion #405). > > Two surfaces from one `statusLine` callback: > - **Header chip** (top-right) — account-wide **plan limits**: `5h 35% · 7d 38%`, per-window green/yellow/red. diff --git a/src/hooks-config.ts b/src/hooks-config.ts index e637a3cb..513dbc28 100644 --- a/src/hooks-config.ts +++ b/src/hooks-config.ts @@ -1044,10 +1044,21 @@ export async function ensureStatusLineExporterScript(): Promise { * to thread a request-time flag through: they all already construct a * session via TmuxManager.createSession/respawnPane, which reads this at * spawn time. + * + * An ABSENT key means ON, mirroring readWorkspaceHooksEnabled() above: the + * client shows the chip and its checkbox as already on for a desktop that has + * never touched the setting (planUsageChipEnabled() in settings-ui.js), and + * the exporter only ever posts to THIS Codeman over loopback, so the honest + * default for an install that never said otherwise is the one the user can + * see. Resolving the default here, in the reader, is what lets + * `GET /api/settings` stay a plain read: a reconcile write there ran on every + * page load and could replace an unreadable settings.json with a one-key + * file. Only an explicit `false` (a save that flipped the chip off on some + * device) turns collection off. */ export async function readPlanUsageTelemetryEnabled(): Promise { const settings = await readJsonConfig>(SETTINGS_PATH, 'settings.json', {}); - return settings.showPlanUsageLimits === true; + return settings.showPlanUsageLimits !== false; } /** diff --git a/src/web/public/settings-ui.js b/src/web/public/settings-ui.js index 31e029ce..92631b0f 100644 --- a/src/web/public/settings-ui.js +++ b/src/web/public/settings-ui.js @@ -2315,21 +2315,25 @@ Object.assign(CodemanApp.prototype, { // and syncing would leak mobile's hidden-checkbox false onto desktop); it's // also absent from SettingsUpdateSchema, which is .strict() — sending it // would 400 the whole settings PUT. - // showPlanUsageLimits is the ONE exception to "per-device keys never sync": - // its DISPLAY stays per-device (loadAppSettingsFromServer only seeds it into - // localStorage when a device has no value yet — same as every other display - // key), but it ALSO doubles as the server-side plan-usage telemetry + // showPlanUsageLimits is per-device for DISPLAY (loadAppSettingsFromServer + // only seeds it into localStorage when a device has no value yet, like every + // other display key) but ALSO doubles as the server-side plan-usage telemetry // COLLECTION switch (readPlanUsageTelemetryEnabled in hooks-config.ts, read - // fresh at every claude session create/respawn), so unlike the others it - // MUST flow through in `serverSettings` below on every save — including - // OFF, which used to be un-sendable under the old one-way "ENABLE only" - // action field this replaces. + // fresh at every claude session create/respawn). So it is stripped here like + // the others and re-added below ONLY when this save FLIPS it on this device + // (planUsageCollectionFlip): the chip defaults OFF on handhelds, so sending + // it on every save let a phone saving its font size persist `false` and + // switch collection off for every desktop, whose chip then went stale with + // no error anywhere. An explicit toggle on any device still writes it, in + // either direction. + const _chipFlip = this.planUsageCollectionFlip(_prev, settings.showPlanUsageLimits); const { localEchoEnabled: _leo, cjkInputEnabled: _cjk, extendedKeyboardBar: _ekb, skin: _skin, language: _language, + showPlanUsageLimits: _pul, showAttachmentsButton: _ahb, showFileViewerButton: _fvb, webglRendererEnabled: _wgl, @@ -2364,6 +2368,7 @@ Object.assign(CodemanApp.prototype, { try { const res = await this._apiPut('/api/settings', { ...serverSettings, + ...(_chipFlip !== undefined ? { showPlanUsageLimits: _chipFlip } : {}), notificationPreferences: notifPrefsToSave, voiceSettings, }); @@ -2637,6 +2642,18 @@ Object.assign(CodemanApp.prototype, { return s.showPlanUsageLimits ?? this.getDefaultSettings().showPlanUsageLimits ?? true; }, + // What a settings save tells the server about plan-usage COLLECTION: the new + // chip value when this save FLIPS it relative to what this device resolved + // before (stored value, else the per-device default), otherwise undefined, + // meaning "say nothing". The server reads an absent key as ON, so a device + // that never touched the chip leaves collection alone, and a handheld (chip + // default OFF) cannot switch it off for every desktop by saving its font + // size. Pure so test/plan-usage-collection-flip.test.ts can drive it. + planUsageCollectionFlip(prevSettings, now) { + const before = this.planUsageChipEnabled(prevSettings ?? {}); + return now === before ? undefined : now; + }, + applyHeaderVisibilitySettings() { const settings = this.loadAppSettingsFromStorage(); const defaults = this.getDefaultSettings(); diff --git a/src/web/routes/system-routes.ts b/src/web/routes/system-routes.ts index 93c45ded..53275f5e 100644 --- a/src/web/routes/system-routes.ts +++ b/src/web/routes/system-routes.ts @@ -936,47 +936,18 @@ export function registerSystemRoutes( // ========== Settings ========== app.get('/api/settings', async () => { - const settings = await readJsonConfig>(SETTINGS_PATH, 'settings', {}); - - // Plan-usage chip default reconciliation (PR #361 follow-up): the client's - // own default resolution (planUsageChipEnabled() in settings-ui.js) shows - // the header chip and the App Settings checkbox as already ON whenever this - // key has never been set — a discoverability default from 1.9.3, unrelated - // to consent. Meanwhile readPlanUsageTelemetryEnabled() (hooks-config.ts) - // deliberately treats an absent key as "no telemetry" (privacy: never POST - // usage data without an explicit persisted yes, pinned by its own unit - // tests). Nothing ever reconciled those two independent guesses, so a - // fresh install showed a checked box that silently did nothing until the - // user opened Settings and hit Save at least once — verified live: an - // install that had never touched this setting had NO showPlanUsageLimits - // key in settings.json, and its running Claude process's argv carried no - // --settings flag at all, i.e. zero telemetry ever collected. - // - // Resolve it ONCE, here, the first time anything reads settings: if the - // key is truly ABSENT (never explicit true or false), persist the same - // desktop-default-ON resolution the client already shows, so "chip visible" - // and "telemetry collected" become the same fact instead of two defaults - // that happen to disagree. readPlanUsageTelemetryEnabled()'s own - // absent-means-false contract is untouched — after this runs once the key - // is never absent again, so that branch stays correct in isolation (its - // unit tests keep passing unmodified) while being unreachable in practice - // for any install that has ever called this route. An explicit false the - // user sets afterward is respected forever; this only fires on true absence. - if (!('showPlanUsageLimits' in settings)) { - settings.showPlanUsageLimits = true; - try { - const dir = dirname(SETTINGS_PATH); - if (!existsSync(dir)) { - mkdirSync(dir, { recursive: true }); - } - await fs.writeFile(SETTINGS_PATH, JSON.stringify(settings, null, 2)); - } catch { - // Best-effort: the resolved default still reaches this response even - // if the write fails, so the caller sees consistent data either way. - } - } - - return settings; + // 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 + // 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 + // default through planUsageChipEnabled(). Pinned by + // test/routes/system-routes-settings-get-plan-usage-default.test.ts. + return readJsonConfig(SETTINGS_PATH, 'settings', {}); }); app.put('/api/settings', async (req) => { diff --git a/src/web/schemas.ts b/src/web/schemas.ts index 53abd24a..9e182f41 100644 --- a/src/web/schemas.ts +++ b/src/web/schemas.ts @@ -1299,7 +1299,9 @@ export const SettingsUpdateSchema = z // Doubles as the plan-usage telemetry COLLECTION switch, read fresh from // disk by readPlanUsageTelemetryEnabled() (hooks-config.ts) at every claude // session create/respawn — not just the chip's DISPLAY preference. See that - // function's doc comment for why one persisted field serves both. + // function's doc comment for why one persisted field serves both. Absent + // means ON there, and the client sends it only on a save that flips the + // chip (planUsageCollectionFlip in settings-ui.js), never on every save. showPlanUsageLimits: z.boolean().optional(), showRedrawButton: z.boolean().optional(), // Input diff --git a/test/hooks-config.test.ts b/test/hooks-config.test.ts index 1048486a..1f17d03d 100644 --- a/test/hooks-config.test.ts +++ b/test/hooks-config.test.ts @@ -1345,13 +1345,22 @@ describe('readPlanUsageTelemetryEnabled', () => { expect(await readPlanUsageTelemetryEnabled()).toBe(false); }); - it('defaults to false when the setting is absent or the file is missing', async () => { + it('defaults to true when the setting is absent or the file is missing (mirrors readWorkspaceHooksEnabled)', async () => { + // The desktop chip shows as ON for an install that never touched the + // setting, so collection must agree with it. Resolving the default HERE + // is what keeps GET /api/settings a plain read (see its route test). rmSync(SETTINGS_PATH, { force: true }); - expect(await readPlanUsageTelemetryEnabled()).toBe(false); + expect(await readPlanUsageTelemetryEnabled()).toBe(true); mkdirSync(join(SETTINGS_PATH, '..'), { recursive: true }); writeFileSync(SETTINGS_PATH, JSON.stringify({ someOtherSetting: true })); - expect(await readPlanUsageTelemetryEnabled()).toBe(false); + expect(await readPlanUsageTelemetryEnabled()).toBe(true); + }); + + it('only an explicit false turns collection off; junk values read as on', async () => { + mkdirSync(join(SETTINGS_PATH, '..'), { recursive: true }); + writeFileSync(SETTINGS_PATH, JSON.stringify({ showPlanUsageLimits: 'no' })); + expect(await readPlanUsageTelemetryEnabled()).toBe(true); }); it('never caches — a change on disk is visible on the very next call', async () => { diff --git a/test/plan-usage-collection-flip.test.ts b/test/plan-usage-collection-flip.test.ts new file mode 100644 index 00000000..776810a5 --- /dev/null +++ b/test/plan-usage-collection-flip.test.ts @@ -0,0 +1,78 @@ +/** + * `planUsageCollectionFlip()` in settings-ui.js: the one place that decides + * whether a settings save carries `showPlanUsageLimits` to the server. + * + * The chip is per-device for DISPLAY (desktop default ON, handhelds OFF) but + * the same persisted key is the server-side telemetry COLLECTION switch, read + * at every claude spawn. Sending it on every save let a phone saving its font + * size persist `false` and turn collection off for every desktop. So the save + * sends the key ONLY when it flips the chip relative to what the device had, + * and the server reads an absent key as ON. + */ +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import vm from 'node:vm'; +import { describe, expect, it } from 'vitest'; + +const SOURCE = readFileSync(resolve(import.meta.dirname, '../src/web/public/settings-ui.js'), 'utf8'); + +function loadSettingsUi(defaultChip: boolean) { + const CodemanApp = function CodemanApp(this: unknown) {}; + const context = vm.createContext({ + CodemanApp, + VoiceInput: {}, + localStorage: { getItem: () => null, setItem: () => {} }, + document: { getElementById: () => null }, + console, + }); + vm.runInContext(SOURCE, context, { filename: 'settings-ui.js' }); + const app = Object.create(CodemanApp.prototype) as { + getDefaultSettings: () => { showPlanUsageLimits: boolean }; + planUsageCollectionFlip: (prev: Record | null, now: boolean) => boolean | undefined; + }; + app.getDefaultSettings = () => ({ showPlanUsageLimits: defaultChip }); + return app; +} + +describe('planUsageCollectionFlip', () => { + it('says nothing when a desktop that never touched the chip saves with it still on', () => { + const desktop = loadSettingsUi(true); + expect(desktop.planUsageCollectionFlip({}, true)).toBeUndefined(); + expect(desktop.planUsageCollectionFlip(null, true)).toBeUndefined(); + }); + + it('says nothing when a handheld (chip default OFF) saves an unrelated setting', () => { + const phone = loadSettingsUi(false); + expect(phone.planUsageCollectionFlip({ terminalFontSize: 14 }, false)).toBeUndefined(); + expect(phone.planUsageCollectionFlip({ showPlanUsageLimits: false }, false)).toBeUndefined(); + }); + + it('sends false only on the save that turned the chip off', () => { + const desktop = loadSettingsUi(true); + expect(desktop.planUsageCollectionFlip({}, false)).toBe(false); + expect(desktop.planUsageCollectionFlip({ showPlanUsageLimits: true }, false)).toBe(false); + expect(desktop.planUsageCollectionFlip({ showPlanUsageLimits: false }, false)).toBeUndefined(); + }); + + it('sends true when any device, a handheld included, turns the chip on', () => { + const phone = loadSettingsUi(false); + expect(phone.planUsageCollectionFlip({}, true)).toBe(true); + expect(phone.planUsageCollectionFlip({ showPlanUsageLimits: false }, true)).toBe(true); + expect(phone.planUsageCollectionFlip({ showPlanUsageLimits: true }, true)).toBeUndefined(); + }); +}); + +describe('saveAppSettings wiring', () => { + it('strips showPlanUsageLimits from the synced payload and re-adds it only through the flip', () => { + const save = SOURCE.slice( + SOURCE.indexOf('async saveAppSettings()'), + SOURCE.indexOf('closeAppSettings()', SOURCE.indexOf('async saveAppSettings()')) + ); + // Stripped from serverSettings like the other per-device display keys. + expect(save).toMatch(/showPlanUsageLimits: _pul,/); + // Decided once against the device's prior settings, before they are overwritten. + expect(save).toMatch(/const _chipFlip = this\.planUsageCollectionFlip\(_prev, settings\.showPlanUsageLimits\);/); + // And only a real flip reaches the PUT body. + expect(save).toMatch(/\.\.\.\(_chipFlip !== undefined \? \{ showPlanUsageLimits: _chipFlip \} : \{\}\),/); + }); +}); 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 56221a08..f2496d72 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 @@ -1,21 +1,17 @@ /** - * @fileoverview GET /api/settings must reconcile `showPlanUsageLimits` the - * first time it is ever read, closing the gap left by PR #361. + * @fileoverview GET /api/settings is a plain read and must NEVER write + * settings.json. * - * planUsageChipEnabled() (settings-ui.js) shows the header chip and the App - * Settings checkbox as already ON whenever this key has never been set — a - * discoverability default from 1.9.3. readPlanUsageTelemetryEnabled() - * (hooks-config.ts) deliberately treats an absent key as "no telemetry" — - * a privacy default, pinned by its own unit tests. Nothing reconciled those - * two independent guesses, so a fresh install showed a checked box that - * silently collected nothing until the user opened Settings and hit Save - * at least once. + * 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 + * 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 + * to planUsageChipEnabled() on the client. * - * These tests pin the fix: GET /api/settings persists the resolved default - * ONCE when the key is truly absent, and never overwrites an explicit value - * either way afterward. - * - * Uses app.inject() — no real HTTP ports needed. Port: N/A. + * Uses app.inject() with a mocked filesystem. Port: N/A. */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; @@ -23,26 +19,24 @@ import { createRouteTestHarness, type RouteTestHarness } from './_route-test-uti import { registerSystemRoutes } from '../../src/web/routes/system-routes.js'; // vi.mock factories are hoisted above module-level consts, so the mutable -// persisted-settings fixture has to be built inside vi.hoisted(). -const { state } = vi.hoisted(() => ({ - // Mutated per-test to control what "disk" holds before the GET. - state: { persisted: {} as Record, exists: true }, +// "disk" fixture has to be built inside vi.hoisted(). +const { state, writeFile } = vi.hoisted(() => ({ + // `raw` is what readFile returns; `failWith` makes it throw with that code. + state: { raw: '{}', failWith: null as string | null }, + writeFile: vi.fn(async () => {}), })); vi.mock('node:fs/promises', () => ({ default: { readFile: vi.fn(async () => { - if (!state.exists) { - const err = new Error('ENOENT') as NodeJS.ErrnoException; - err.code = 'ENOENT'; + if (state.failWith) { + const err = new Error(state.failWith) as NodeJS.ErrnoException; + err.code = state.failWith; throw err; } - return JSON.stringify(state.persisted); - }), - writeFile: vi.fn(async (_path: string, content: string) => { - state.persisted = JSON.parse(content); - state.exists = true; + return state.raw; }), + writeFile, }, })); @@ -51,10 +45,13 @@ vi.mock('node:fs', async (importOriginal) => { return { ...actual, existsSync: vi.fn(() => true), mkdirSync: vi.fn(), readdirSync: vi.fn(() => []) }; }); -describe('GET /api/settings — showPlanUsageLimits default reconciliation', () => { +describe('GET /api/settings never writes settings.json', () => { let harness: RouteTestHarness; beforeEach(async () => { + state.raw = '{}'; + state.failWith = null; + writeFile.mockClear(); harness = await createRouteTestHarness(registerSystemRoutes); }); @@ -62,47 +59,56 @@ describe('GET /api/settings — showPlanUsageLimits default reconciliation', () await harness.app.close(); }); - it('persists the resolved default (true) the first time the key is absent', async () => { - state.persisted = { someOtherSetting: true }; - state.exists = true; + it('returns the file content unchanged when showPlanUsageLimits is absent, and writes nothing', async () => { + state.raw = JSON.stringify({ someOtherSetting: true }); const res = await harness.app.inject({ method: 'GET', url: '/api/settings' }); expect(res.statusCode).toBe(200); - expect(res.json().showPlanUsageLimits).toBe(true); - // Reconciliation actually reached disk, not just the response. - expect(state.persisted.showPlanUsageLimits).toBe(true); + expect(res.json()).toEqual({ someOtherSetting: true }); + expect(writeFile).not.toHaveBeenCalled(); }); - it('reconciles even when settings.json does not exist at all', async () => { - state.exists = false; + it('answers {} for a missing settings.json without creating one', async () => { + state.failWith = 'ENOENT'; const res = await harness.app.inject({ method: 'GET', url: '/api/settings' }); expect(res.statusCode).toBe(200); - expect(res.json().showPlanUsageLimits).toBe(true); - expect(state.persisted.showPlanUsageLimits).toBe(true); + expect(res.json()).toEqual({}); + expect(writeFile).not.toHaveBeenCalled(); }); - it('never overwrites an explicit false', async () => { - state.persisted = { showPlanUsageLimits: false }; - state.exists = true; + it('leaves an unreadable settings.json alone (EACCES is not "absent")', async () => { + state.failWith = 'EACCES'; + const quiet = vi.spyOn(console, 'error').mockImplementation(() => {}); const res = await harness.app.inject({ method: 'GET', url: '/api/settings' }); + quiet.mockRestore(); expect(res.statusCode).toBe(200); - expect(res.json().showPlanUsageLimits).toBe(false); - // Untouched — reconciliation must not have written anything. - expect(state.persisted.showPlanUsageLimits).toBe(false); + expect(res.json()).toEqual({}); + expect(writeFile).not.toHaveBeenCalled(); }); - it('never rewrites an explicit true', async () => { - state.persisted = { showPlanUsageLimits: true }; - state.exists = true; + it('leaves a garbage settings.json alone (a parse error is not "absent")', async () => { + state.raw = '{ "showPlanUsageLimits": tru'; + const quiet = vi.spyOn(console, 'error').mockImplementation(() => {}); const res = await harness.app.inject({ method: 'GET', url: '/api/settings' }); + quiet.mockRestore(); expect(res.statusCode).toBe(200); - expect(res.json().showPlanUsageLimits).toBe(true); + expect(res.json()).toEqual({}); + expect(writeFile).not.toHaveBeenCalled(); + }); + + it('passes an explicit value through either way', async () => { + for (const value of [true, false]) { + state.raw = JSON.stringify({ showPlanUsageLimits: value }); + const res = await harness.app.inject({ method: 'GET', url: '/api/settings' }); + expect(res.json().showPlanUsageLimits).toBe(value); + } + expect(writeFile).not.toHaveBeenCalled(); }); }); diff --git a/test/routes/system-routes.test.ts b/test/routes/system-routes.test.ts index be690ce1..aa1fae13 100644 --- a/test/routes/system-routes.test.ts +++ b/test/routes/system-routes.test.ts @@ -411,34 +411,34 @@ describe('system-routes', () => { // ========== GET /api/settings ========== describe('GET /api/settings', () => { - it('reconciles showPlanUsageLimits to true when the settings file does not exist', async () => { - // The chip/checkbox default to ON client-side (planUsageChipEnabled()) whenever - // this key is absent, but readPlanUsageTelemetryEnabled() deliberately treats - // absence as "no telemetry" — nothing reconciled those two defaults, so a fresh - // install showed a checked box that silently collected nothing. GET now persists - // the resolved default the first time anything reads settings. + // A plain read that never writes. The route briefly reconciled an absent + // showPlanUsageLimits to true on first read, but readJsonConfig() answers {} + // for ANY read failure and every page load hits this route, so one unlucky + // read replaced the whole file with a one-key file. The default now lives in + // readPlanUsageTelemetryEnabled() (absent means ON); see also + // system-routes-settings-get-plan-usage-default.test.ts. + it('answers {} for a missing settings file and creates nothing', async () => { mockedReadFile.mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })); + mockedWriteFile.mockClear(); const res = await harness.app.inject({ method: 'GET', url: '/api/settings' }); expect(res.statusCode).toBe(200); - expect(JSON.parse(res.body)).toEqual({ showPlanUsageLimits: true }); - // Reconciliation actually reached disk, not just the response. - expect(mockedWriteFile).toHaveBeenCalledWith( - expect.anything(), - JSON.stringify({ showPlanUsageLimits: true }, null, 2) - ); + expect(JSON.parse(res.body)).toEqual({}); + expect(mockedWriteFile).not.toHaveBeenCalled(); }); - it('reconciles showPlanUsageLimits to true when the file exists but omits it', async () => { + it('returns the file unchanged when showPlanUsageLimits is absent', async () => { const settings = { subagentTrackingEnabled: true, showSystemStats: false }; mockedReadFile.mockResolvedValue(JSON.stringify(settings) as never); + mockedWriteFile.mockClear(); const res = await harness.app.inject({ method: 'GET', url: '/api/settings' }); expect(res.statusCode).toBe(200); - expect(JSON.parse(res.body)).toEqual({ ...settings, showPlanUsageLimits: true }); + expect(JSON.parse(res.body)).toEqual(settings); + expect(mockedWriteFile).not.toHaveBeenCalled(); }); - it('never overwrites an explicit false', async () => { + it('passes an explicit false through untouched', async () => { const settings = { subagentTrackingEnabled: true, showPlanUsageLimits: false }; mockedReadFile.mockResolvedValue(JSON.stringify(settings) as never); mockedWriteFile.mockClear(); @@ -446,7 +446,6 @@ describe('system-routes', () => { const res = await harness.app.inject({ method: 'GET', url: '/api/settings' }); expect(res.statusCode).toBe(200); expect(JSON.parse(res.body)).toEqual(settings); - // No reconciliation write when the key is already explicit. expect(mockedWriteFile).not.toHaveBeenCalled(); }); });