Merge pull request #361 from timkjr/fix/statusline-injection-opt-out

fix(statusline): inject plan-usage telemetry via ephemeral CLI flag, never disk

test/routes/session-routes-workspace-hooks.test.ts

@@ -156,7 +156,7 @@ describe('POST /api/sessions workspace hooks', () => {
     const cwdSettings = join(process.cwd(), '.claude', 'settings.local.json');
     const before = existsSync(cwdSettings) ? await readFile(cwdSettings, 'utf-8') : null;
 
-    const res = await createSession({ name: 'hooks-no-dir', mode: 'claude', statusLineTelemetry: true });
+    const res = await createSession({ name: 'hooks-no-dir', mode: 'claude' });
     expect(res.statusCode).toBe(200);
 
     const after = existsSync(cwdSettings) ? await readFile(cwdSettings, 'utf-8') : null;
@@ -166,8 +166,9 @@ describe('POST /api/sessions workspace hooks', () => {
   it('never writes hooks for a remote attach (workingDir is a user@host pseudo-path)', async () => {
     // A claude-mode attachRemoteSession create overwrites workingDir with
     // `user@host:session` — locally a RELATIVE path, so a mkdir would create it
-    // as a junk directory under the server cwd. statusLineTelemetry rides along:
-    // applyStatusLineConfig mkdirs the same way and used to run for remote attaches.
+    // as a junk directory under the server cwd. The statusLine exporter rides
+    // along: applyStatusLineConfig mkdirs the same way and used to run for
+    // remote attaches.
     // SAFETY (2026-08-29): write straight to `getDataDir()` — `test/setup.ts`
     // already sandboxes the data dir for the whole file (temp HOME, inherited
     // CODEMAN_DATA_DIR stripped; same convention as the docker-hosts fixtures
@@ -188,7 +189,6 @@ describe('POST /api/sessions workspace hooks', () => {
     const res = await createSession({
       name: 'hooks-remote',
       mode: 'claude',
-      statusLineTelemetry: true,
       attachRemoteSession: { hostId: 'h1', remoteSessionName: 'codeman-ssh-abc123' },
     });
     expect(res.statusCode).toBe(200);

test/routes/system-routes-settings-get-plan-usage-default.test.ts

@@ -0,0 +1,108 @@
+/**
+ * @fileoverview GET /api/settings must reconcile `showPlanUsageLimits` the
+ * first time it is ever read, closing the gap left by PR #361.
+ *
+ * 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.
+ *
+ * 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.
+ */
+
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
+import { createRouteTestHarness, type RouteTestHarness } from './_route-test-utils.js';
+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 },
+}));
+
+vi.mock('node:fs/promises', () => ({
+  default: {
+    readFile: vi.fn(async () => {
+      if (!state.exists) {
+        const err = new Error('ENOENT') as NodeJS.ErrnoException;
+        err.code = 'ENOENT';
+        throw err;
+      }
+      return JSON.stringify(state.persisted);
+    }),
+    writeFile: vi.fn(async (_path: string, content: string) => {
+      state.persisted = JSON.parse(content);
+      state.exists = true;
+    }),
+  },
+}));
+
+vi.mock('node:fs', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('node:fs')>();
+  return { ...actual, existsSync: vi.fn(() => true), mkdirSync: vi.fn(), readdirSync: vi.fn(() => []) };
+});
+
+describe('GET /api/settings — showPlanUsageLimits default reconciliation', () => {
+  let harness: RouteTestHarness;
+
+  beforeEach(async () => {
+    harness = await createRouteTestHarness(registerSystemRoutes);
+  });
+
+  afterEach(async () => {
+    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;
+
+    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);
+  });
+
+  it('reconciles even when settings.json does not exist at all', async () => {
+    state.exists = false;
+
+    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);
+  });
+
+  it('never overwrites an explicit false', async () => {
+    state.persisted = { showPlanUsageLimits: false };
+    state.exists = true;
+
+    const res = await harness.app.inject({ method: 'GET', url: '/api/settings' });
+
+    expect(res.statusCode).toBe(200);
+    expect(res.json().showPlanUsageLimits).toBe(false);
+    // Untouched — reconciliation must not have written anything.
+    expect(state.persisted.showPlanUsageLimits).toBe(false);
+  });
+
+  it('never rewrites an explicit true', async () => {
+    state.persisted = { showPlanUsageLimits: true };
+    state.exists = true;
+
+    const res = await harness.app.inject({ method: 'GET', url: '/api/settings' });
+
+    expect(res.statusCode).toBe(200);
+    expect(res.json().showPlanUsageLimits).toBe(true);
+  });
+});

test/routes/system-routes-settings-partial-put.test.ts

@@ -4,7 +4,7 @@
  * The three service toggles (subagent watcher, workflow-run watcher, image
  * watcher) used to read the RAW REQUEST BODY with `??` defaults, so any key the
  * caller omitted was treated as "apply the default". A body of just
- * `{statusLineTelemetry:true}` therefore STARTED the subagent watcher (`?? true`)
+ * `{showPlanUsageLimits:true}` therefore STARTED the subagent watcher (`?? true`)
  * and STOPPED the workflow + image watchers (`?? false`), silently undoing the
  * persisted config. Nothing triggered it in practice only because every shipped
  * client sends a full settings payload rebuilt from the DOM.
@@ -91,8 +91,8 @@ describe('PUT /api/settings — partial body must not reset service toggles', ()
     const res = await harness.app.inject({
       method: 'PUT',
       url: '/api/settings',
-      // Action-only body: the exact shape that used to flip all three watchers.
-      payload: { statusLineTelemetry: true },
+      // Minimal single-key body: the exact shape that used to flip all three watchers.
+      payload: { showPlanUsageLimits: true },
     });
 
     expect(res.statusCode).toBe(200);

test/routes/system-routes.test.ts

@@ -411,21 +411,43 @@ describe('system-routes', () => {
   // ========== GET /api/settings ==========
 
   describe('GET /api/settings', () => {
-    it('returns empty object when settings file does not exist', async () => {
+    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.
       mockedReadFile.mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' }));
 
       const res = await harness.app.inject({ method: 'GET', url: '/api/settings' });
       expect(res.statusCode).toBe(200);
-      expect(JSON.parse(res.body)).toEqual({});
+      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)
+      );
     });
 
-    it('returns parsed settings when file exists', async () => {
+    it('reconciles showPlanUsageLimits to true when the file exists but omits it', async () => {
       const settings = { subagentTrackingEnabled: true, showSystemStats: false };
       mockedReadFile.mockResolvedValue(JSON.stringify(settings) as never);
 
+      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 });
+    });
+
+    it('never overwrites an explicit false', async () => {
+      const settings = { subagentTrackingEnabled: true, showPlanUsageLimits: 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);
+      // No reconciliation write when the key is already explicit.
+      expect(mockedWriteFile).not.toHaveBeenCalled();
     });
   });