diff --git a/src/web/routes/system-routes.ts b/src/web/routes/system-routes.ts index 5d6b3c93..93c45ded 100644 --- a/src/web/routes/system-routes.ts +++ b/src/web/routes/system-routes.ts @@ -936,7 +936,47 @@ export function registerSystemRoutes( // ========== Settings ========== app.get('/api/settings', async () => { - return readJsonConfig(SETTINGS_PATH, 'settings', {}); + 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; }); app.put('/api/settings', async (req) => { 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 new file mode 100644 index 00000000..56221a08 --- /dev/null +++ b/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, 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(); + 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); + }); +}); diff --git a/test/routes/system-routes.test.ts b/test/routes/system-routes.test.ts index 3873f2f5..be690ce1 100644 --- a/test/routes/system-routes.test.ts +++ b/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(); }); });