fix(settings): reconcile showPlanUsageLimits default on first read

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 <noreply@anthropic.com>
This commit is contained in:
timkjr
2026-09-10 19:10:25 -05:00
co-authored by Claude Sonnet 5
parent d5b75af628
commit aeb55c92b0
3 changed files with 174 additions and 4 deletions
@@ -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);
});
});
+25 -3
View File
@@ -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();
});
});