From aeb55c92b051c77bc25fca3d254bd724460fc74d Mon Sep 17 00:00:00 2001 From: timkjr Date: Thu, 10 Sep 2026 19:10:14 -0500 Subject: [PATCH] fix(settings): reconcile showPlanUsageLimits default on first read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit planUsageChipEnabled() (settings-ui.js) shows the header chip and the App Settings checkbox as already ON whenever showPlanUsageLimits 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 (never POST usage data without an explicit persisted yes). 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. Verified live: an install that had never touched this setting had no showPlanUsageLimits key in settings.json at all, and its running Claude process's argv carried no --settings flag — zero telemetry ever collected despite the chip rendering as enabled. GET /api/settings now persists the resolved default (true) the first time the key is truly absent — not explicit false — so "chip visible" and "telemetry collected" become the same fact. 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 while being unreachable in practice for any install that has ever called this route. An explicit false set afterward is respected forever. Co-Authored-By: Claude Sonnet 5 --- src/web/routes/system-routes.ts | 42 ++++++- ...es-settings-get-plan-usage-default.test.ts | 108 ++++++++++++++++++ test/routes/system-routes.test.ts | 28 ++++- 3 files changed, 174 insertions(+), 4 deletions(-) create mode 100644 test/routes/system-routes-settings-get-plan-usage-default.test.ts 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(); }); });