fix(statusline): GET /api/settings never writes, and a save sends the collection switch only on a flip

Two follow-ups to #361's sticky telemetry switch.

GET /api/settings reconciled an absent showPlanUsageLimits by persisting
true, but readJsonConfig() answers {} for ANY read failure (a 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 route is a
plain read again and the default moved into the reader:
readPlanUsageTelemetryEnabled() treats an absent key as ON, the same way
readWorkspaceHooksEnabled() does, which is what the desktop chip already
shows for an install that never touched the setting.

saveAppSettings() sent showPlanUsageLimits on every save. The chip
defaults OFF on handhelds, so a phone saving its font size persisted
false and switched collection off for every desktop, whose chip then
went stale with no error anywhere. The key is now stripped like the
other per-device display keys and re-added only when the save FLIPS the
chip relative to what the device had (planUsageCollectionFlip), so an
explicit toggle on any device still writes it in either direction.

Tests pin both: the GET route with a mocked filesystem (absent, missing,
EACCES, garbage, explicit), the reader default, and the flip helper plus
its wiring in saveAppSettings.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

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 () => {

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<string, unknown> | 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 \} : \{\}\),/);
+  });
+});

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

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();
     });
   });