From 68c1f0dce8c92655b250170f175efe73a24c499e Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Mon, 21 Sep 2026 18:54:14 -0700 Subject: [PATCH 1/5] feat(settings): log every persisted change, with the stack that caused it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A consent-adjacent flag changed itself on the QA box — betaFeaturesEnabled went true to false in a clean single-field write, no click, no onboarding, telemetry true throughout — and nothing in the app could say what wrote it. Every writer was then excluded by reading the code. The seed only writes when the value is absent and would have written true, since telemetry was true. The onboarding submit needs the first-use flow. The settings toggle emits only from a click, and its watcher deliberately does not. The whole-object saves write back what they loaded. So the writer is something a source search does not see, which is exactly the case a log has to cover: the useful question is not "which of the writers I know about ran" but "who ran". That is why this logs a STACK rather than a per-call-site reason tag. A tag can only annotate sites someone already thought of — here, precisely the set that has been ruled out. It would have printed the flip with no tag and left the question open. At `save` rather than `set`, because `set` is not the only writer: the seed and the directory-repair path both persist whole objects without going through it. Diffed against what is on disk, so a save that changes nothing says nothing, and a key being REMOVED is reported too — a deletion is what makes a later seed re-run and write a value nobody chose. `set-setting` additionally records the requesting renderer's URL, because for anything renderer-driven the stack stops at the IPC handler and every such write otherwise looks identical. Writing the tests surfaced two behaviours worth knowing, both now pinned: a single `set` can produce TWO writes, since `loadOutcome` repairs missing directories and saves before `set` saves again; and the first write on a sparse file materialises every default as a real change. Both are truthful, and a reader who does not expect them would misread the log. Diagnostic only — no behaviour changes. Co-Authored-By: Claude Opus 5 --- src/main/lib/ipc/registerSettingsHandlers.ts | 7 ++- src/main/settings.test.ts | 52 ++++++++++++++++++++ src/main/settings.ts | 46 +++++++++++++++++ 3 files changed, 104 insertions(+), 1 deletion(-) diff --git a/src/main/lib/ipc/registerSettingsHandlers.ts b/src/main/lib/ipc/registerSettingsHandlers.ts index c8d30106c..79ff483bf 100644 --- a/src/main/lib/ipc/registerSettingsHandlers.ts +++ b/src/main/lib/ipc/registerSettingsHandlers.ts @@ -328,8 +328,13 @@ export function registerSettingsHandlers(): void { ipcMain.handle('get-models-sections', () => buildModelsPayload()) ipcMain.handle('get-media-sections', () => buildMediaSections()) - ipcMain.handle('set-setting', (_event, key: string, value: unknown) => { + ipcMain.handle('set-setting', (event, key: string, value: unknown) => { recordIpcInvocation('set-setting', { key, value }) + // The settings log's stack stops at this handler for anything a renderer asked for, so + // record WHICH renderer asked. Without it every renderer-driven write looks identical. + console.log( + `Settings: set-setting '${key}' requested by ${event.sender.getURL() || ''}` + ) applySettingSet(key, value) }) diff --git a/src/main/settings.test.ts b/src/main/settings.test.ts index 8f706031f..ad90255ee 100644 --- a/src/main/settings.test.ts +++ b/src/main/settings.test.ts @@ -702,6 +702,58 @@ describe('locked settings.json served from .bak (issue #1367)', () => { }) }) +describe('persisted-write logging', () => { + it('names the key, the old and new value, and the caller', () => { + fs.mkdirSync(path.dirname(settingsPath), { recursive: true }) + fs.writeFileSync(settingsPath, JSON.stringify({ betaFeaturesEnabled: true })) + const log = vi.spyOn(console, 'log').mockImplementation(() => {}) + + settings.set('betaFeaturesEnabled', false) + + const line = log.mock.calls + .map((c) => String(c[0])) + .find((l) => l.startsWith('Settings: writing')) + expect(line, 'no write line logged').toBeDefined() + expect(line).toContain('betaFeaturesEnabled: true -> false') + // The point of the log: a stack, so an UNKNOWN writer is named. A per-call-site tag would + // only ever name the sites someone already thought to annotate. + expect(line!.split('\n').length).toBeGreaterThan(1) + log.mockRestore() + }) + + it('stays silent about a key whose value does not change', () => { + // Scoped to the key under test rather than to "no output at all": a `set` can persist + // more than the caller asked for. `loadOutcome` repairs missing directories and saves + // before `set` saves again, so a first write on a sparse file materialises every default + // as a real change — truthfully logged, and nothing to do with the key being set. + fs.mkdirSync(path.dirname(settingsPath), { recursive: true }) + fs.writeFileSync(settingsPath, JSON.stringify({ betaFeaturesEnabled: true })) + const log = vi.spyOn(console, 'log').mockImplementation(() => {}) + + settings.set('betaFeaturesEnabled', true) + + const lines = log.mock.calls.map((c) => String(c[0])).filter((l) => l.startsWith('Settings:')) + expect(lines.filter((l) => l.includes('betaFeaturesEnabled'))).toEqual([]) + log.mockRestore() + }) + + it('reports a key being removed rather than going quiet', () => { + // `set(key, undefined)` deletes, and a deletion is exactly what makes a later seed re-run + // and write a value nobody chose. A log that only reported value changes would miss the + // step that causes the next write. + fs.mkdirSync(path.dirname(settingsPath), { recursive: true }) + fs.writeFileSync(settingsPath, JSON.stringify({ betaFeaturesEnabled: true })) + const log = vi.spyOn(console, 'log').mockImplementation(() => {}) + + settings.set('betaFeaturesEnabled', undefined) + + // Every line, not just the first: the repair save above is logged before this one. + const lines = log.mock.calls.map((c) => String(c[0])).filter((l) => l.startsWith('Settings:')) + expect(lines.some((l) => l.includes('betaFeaturesEnabled: true -> '))).toBe(true) + log.mockRestore() + }) +}) + // The beta-features toggle is the gate for injecting core beta launch args. // It is deliberately NOT telemetry consent: gating on consent creates a trap // where a user hitting beta bugs escapes by disabling telemetry, killing the diff --git a/src/main/settings.ts b/src/main/settings.ts index 5e5e14258..c1ae0731a 100644 --- a/src/main/settings.ts +++ b/src/main/settings.ts @@ -628,7 +628,53 @@ function loadOutcome(): { settings: Settings; unreadable: boolean } { return { settings: result, unreadable } } +/** One line per key whose persisted value actually changes, with the stack that caused it. + * + * Written because a consent-adjacent flag changed itself on a QA box and nothing in the app + * could say what wrote it. Every candidate was excluded by reading the code, which is exactly + * the situation a log has to cover: the useful question is not "which of the writers I know + * about ran" but "who ran", and only a stack answers that. A per-call-site tag would have + * annotated the sites already ruled out and stayed silent on the one that matters. + * + * At `save`, not at `set`, because `set` is not the only writer — the seed and the + * directory-repair path both persist whole objects without going through it. + * + * Diffed against what is on disk, so a save that changes nothing says nothing. Settings are + * written on user actions rather than in loops, so the extra read is not a hot path. */ +function logPersistedChanges(next: Settings): void { + try { + const read = readFileSafe(dataPath) + let before: Record = {} + if (read.kind === 'data') { + const parsed: unknown = JSON.parse(read.data) + if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { + before = parsed as Record + } + } + const brief = (v: unknown): string => { + if (v === undefined) return '' + const text = JSON.stringify(v) ?? String(v) + return text.length > 120 ? `${text.slice(0, 117)}...` : text + } + const keys = new Set([...Object.keys(before), ...Object.keys(next as object)]) + const changes: string[] = [] + for (const key of keys) { + const a = before[key] + const b = (next as Record)[key] + if (JSON.stringify(a) === JSON.stringify(b)) continue + changes.push(`${key}: ${brief(a)} -> ${brief(b)}`) + } + if (changes.length === 0) return + // Frames 0-1 are this helper and `save`; the caller starts after them. + const stack = (new Error().stack ?? '').split('\n').slice(3, 9).join('\n') + console.log(`Settings: writing ${changes.join(', ')}\n${stack}`) + } catch { + // Diagnostics must never cost a write. A failure here is silent on purpose. + } +} + function save(settings: Settings): void { + logPersistedChanges(settings) writeFileSafe(dataPath, JSON.stringify(settings, null, 2), { backup: true }) } From 8a434628e60e08c9dcb28b00d6fe81ee42ce318f Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Mon, 21 Sep 2026 23:33:27 -0700 Subject: [PATCH 2/5] fix(settings): take the log's baseline from memory and never print values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found this in the wrong place and too loud. Both are fixed by the same change of source: the baseline now comes from the caller's already-loaded object instead of re-reading settings.json. Re-reading looked simpler and was wrong three ways. `readFileSafe` increments the process-wide `.bak`-fallback counter that telemetry reports, so a diagnostic was quietly moving a metric operators use. It blocks the main thread on `Atomics.wait` while retrying a locked file, so "the extra read is not a hot path" was measuring the wrong cost. And it cannot distinguish "no previous value" from "previous file unparseable", which would have dropped the log exactly when a malformed file is the interesting case. Reading memory has none of those properties. VALUES ARE NO LONGER PRINTED. These lines land in app.log, which users attach to support requests; `appLog` runs `scrubAll`, but that is a best-effort telemetry scrubber for known credential shapes, not a licence to write every setting a user has. Booleans and numbers are still logged exactly — they cannot carry a secret and they are the question this log exists to answer. Everything else is reduced to its shape (``, ``), which still says whether a key changed and into what kind of thing. A test pins that a mirror URL with embedded credentials does not appear. Also from review: - logged AFTER the write lands, since `writeFileSafe` can throw and a line claiming a value was written when it was not is worse than no line; - `event.sender.getURL()` guarded — it throws "Object has been destroyed" when the sender is torn down between invoke and dispatch, and a diagnostic must never be the reason the write it describes is lost; - object comparison is key-order insensitive, so a re-serialised object no longer reads as a real edit and puts a spurious writer in the log; - the directory-repair path snapshots before its substitutions, so it attributes its own writes instead of logging nothing. The earlier stack assertion could not fail — it counted newlines in a template that always contains one. It now matches the frame itself. Co-Authored-By: Claude Opus 5 --- src/main/lib/ipc/registerSettingsHandlers.ts | 15 ++- src/main/settings.test.ts | 85 ++++++++++---- src/main/settings.ts | 114 ++++++++++++------- 3 files changed, 153 insertions(+), 61 deletions(-) diff --git a/src/main/lib/ipc/registerSettingsHandlers.ts b/src/main/lib/ipc/registerSettingsHandlers.ts index 0de16a7d4..01b36d8f9 100644 --- a/src/main/lib/ipc/registerSettingsHandlers.ts +++ b/src/main/lib/ipc/registerSettingsHandlers.ts @@ -333,9 +333,18 @@ export function registerSettingsHandlers(): void { recordIpcInvocation('set-setting', { key, value }) // The settings log's stack stops at this handler for anything a renderer asked for, so // record WHICH renderer asked. Without it every renderer-driven write looks identical. - console.log( - `Settings: set-setting '${key}' requested by ${event.sender.getURL() || ''}` - ) + // + // Guarded: `getURL()` throws "Object has been destroyed" when the sender is torn down + // between `invoke` and dispatch — a popup closing right after a toggle does exactly that, + // and a diagnostic must never be the reason the write it describes is lost. + const origin = ((): string => { + try { + return event.sender.getURL() || '' + } catch { + return '' + } + })() + console.log(`Settings: set-setting ${JSON.stringify(key)} requested by ${origin}`) applySettingSet(key, value) }) diff --git a/src/main/settings.test.ts b/src/main/settings.test.ts index ad90255ee..66ef92d57 100644 --- a/src/main/settings.test.ts +++ b/src/main/settings.test.ts @@ -703,53 +703,98 @@ describe('locked settings.json served from .bak (issue #1367)', () => { }) describe('persisted-write logging', () => { - it('names the key, the old and new value, and the caller', () => { + const writeLines = (log: { mock: { calls: unknown[][] } }): string[] => + log.mock.calls + .map((call) => String(call[0])) + .filter((line) => line.startsWith('Settings: wrote')) + + it('names the key, the change, and the caller', () => { fs.mkdirSync(path.dirname(settingsPath), { recursive: true }) fs.writeFileSync(settingsPath, JSON.stringify({ betaFeaturesEnabled: true })) const log = vi.spyOn(console, 'log').mockImplementation(() => {}) settings.set('betaFeaturesEnabled', false) - const line = log.mock.calls - .map((c) => String(c[0])) - .find((l) => l.startsWith('Settings: writing')) - expect(line, 'no write line logged').toBeDefined() - expect(line).toContain('betaFeaturesEnabled: true -> false') + const line = writeLines(log).find((l) => l.includes('betaFeaturesEnabled')) + expect(line, 'no write line logged for the key').toBeDefined() + expect(line).toContain('"betaFeaturesEnabled": true -> false') // The point of the log: a stack, so an UNKNOWN writer is named. A per-call-site tag would - // only ever name the sites someone already thought to annotate. - expect(line!.split('\n').length).toBeGreaterThan(1) + // only ever name the sites someone already thought to annotate. Asserted on the frame + // marker rather than on line count, which the format guarantees either way. + expect(line).toMatch(/\| via .*settings\.ts/) log.mockRestore() }) it('stays silent about a key whose value does not change', () => { - // Scoped to the key under test rather than to "no output at all": a `set` can persist - // more than the caller asked for. `loadOutcome` repairs missing directories and saves - // before `set` saves again, so a first write on a sparse file materialises every default - // as a real change — truthfully logged, and nothing to do with the key being set. fs.mkdirSync(path.dirname(settingsPath), { recursive: true }) fs.writeFileSync(settingsPath, JSON.stringify({ betaFeaturesEnabled: true })) const log = vi.spyOn(console, 'log').mockImplementation(() => {}) settings.set('betaFeaturesEnabled', true) - const lines = log.mock.calls.map((c) => String(c[0])).filter((l) => l.startsWith('Settings:')) - expect(lines.filter((l) => l.includes('betaFeaturesEnabled'))).toEqual([]) + expect(writeLines(log).filter((l) => l.includes('betaFeaturesEnabled'))).toEqual([]) log.mockRestore() }) it('reports a key being removed rather than going quiet', () => { - // `set(key, undefined)` deletes, and a deletion is exactly what makes a later seed re-run - // and write a value nobody chose. A log that only reported value changes would miss the - // step that causes the next write. + // `set(key, undefined)` deletes, and a deletion is what makes a later seed re-run and + // write a value nobody chose. A log that only reported value changes would catch the + // effect and miss the cause. fs.mkdirSync(path.dirname(settingsPath), { recursive: true }) fs.writeFileSync(settingsPath, JSON.stringify({ betaFeaturesEnabled: true })) const log = vi.spyOn(console, 'log').mockImplementation(() => {}) settings.set('betaFeaturesEnabled', undefined) - // Every line, not just the first: the repair save above is logged before this one. - const lines = log.mock.calls.map((c) => String(c[0])).filter((l) => l.startsWith('Settings:')) - expect(lines.some((l) => l.includes('betaFeaturesEnabled: true -> '))).toBe(true) + expect(writeLines(log).some((l) => l.includes('"betaFeaturesEnabled": true -> '))).toBe( + true + ) + log.mockRestore() + }) + + it('describes a string value by shape instead of printing it', () => { + // These lines land in `app.log`, which users attach to support requests. A path or a + // mirror host must not be disclosed just because it changed; the shape still answers + // "did this key change, and into what kind of thing". + fs.mkdirSync(path.dirname(settingsPath), { recursive: true }) + fs.writeFileSync(settingsPath, JSON.stringify({ pypiMirror: 'https://old.example' })) + const log = vi.spyOn(console, 'log').mockImplementation(() => {}) + + settings.set('pypiMirror', 'https://user:hunter2@secret.example/simple') + + const line = writeLines(log).find((l) => l.includes('pypiMirror')) + expect(line).toBeDefined() + expect(line).toContain(' { + fs.mkdirSync(path.dirname(settingsPath), { recursive: true }) + fs.writeFileSync(settingsPath, JSON.stringify({ betaFeaturesEnabled: true })) + const log = vi.spyOn(console, 'log').mockImplementation(() => {}) + const realWrite = fs.writeFileSync.bind(fs) as typeof fs.writeFileSync + const write = vi.spyOn(fs, 'writeFileSync').mockImplementation((( + target: fs.PathOrFileDescriptor, + data: string | NodeJS.ArrayBufferView, + opts?: unknown + ) => { + if (String(target).startsWith(settingsPath)) throw new Error('fake ENOSPC') + return realWrite(target, data, opts as fs.WriteFileOptions) + }) as typeof fs.writeFileSync) + + try { + settings.set('betaFeaturesEnabled', false) + } catch { + // The write failing is the point; whether it propagates is not what this pins. + } finally { + // Restored in a `finally`: a leaked write mock fails every later test in the file with + // this test's fake error, which is a confusing way to learn about a missing cleanup. + write.mockRestore() + } + + expect(writeLines(log).filter((l) => l.includes('betaFeaturesEnabled'))).toEqual([]) log.mockRestore() }) }) diff --git a/src/main/settings.ts b/src/main/settings.ts index 704052da0..3ededfdb4 100644 --- a/src/main/settings.ts +++ b/src/main/settings.ts @@ -490,6 +490,9 @@ function loadOutcome(): { settings: Settings; unreadable: boolean } { } const result: Settings = { ...defaults, ...(parsed || {}) } let changed = false + // Snapshot before the directory substitutions below, so the repair path attributes its + // own writes rather than logging nothing. + const repairBaseline = structuredClone(result) // Drop legacy keys that no longer back any setting. `maxCachedFiles` was the // user-editable predecessor of `maxCachedDownloads`; its old value is @@ -631,58 +634,90 @@ function loadOutcome(): { settings: Settings; unreadable: boolean } { changed = true } } - if (changed && !unreadable) save(result) + if (changed && !unreadable) save(result, repairBaseline) return { settings: result, unreadable } } +/** Describe a value for the log WITHOUT disclosing it. + * + * These lines land in `app.log`, which users attach to support requests. `appLog` runs + * `scrubAll` over everything, but that is a best-effort telemetry scrubber for known + * credential shapes — it is not a licence to write every setting a user has. Paths, mirror + * hosts and anything else bespoke would go straight through it. + * + * Booleans and numbers are logged exactly, because they cannot carry a secret and they are + * what this log exists to explain — `betaFeaturesEnabled: true -> false` is the whole + * question. Everything else is reduced to its shape, which still answers "did this key + * change, and into what kind of thing", without printing the contents. */ +function describeForLog(v: unknown): string { + if (v === undefined) return '' + if (v === null) return 'null' + if (typeof v === 'boolean' || typeof v === 'number') return String(v) + if (typeof v === 'string') return `` + if (Array.isArray(v)) return `` + if (typeof v === 'object') return `` + return `<${typeof v}>` +} + +/** True when two persisted values differ, order-insensitively for objects. + * + * Comparing serializations would make a key-order change read as a real edit, which would + * put a spurious writer in the log — the opposite of useful when the log's whole job is + * attributing a change to a caller. */ +function sameValue(a: unknown, b: unknown): boolean { + if (a === b) return true + if (typeof a !== typeof b || a === null || b === null) return false + if (Array.isArray(a) !== Array.isArray(b)) return false + if (typeof a !== 'object') return false + const ao = a as Record + const bo = b as Record + const ak = Object.keys(ao) + const bk = Object.keys(bo) + if (ak.length !== bk.length) return false + return ak.every((k) => Object.prototype.hasOwnProperty.call(bo, k) && sameValue(ao[k], bo[k])) +} + /** One line per key whose persisted value actually changes, with the stack that caused it. * - * Written because a consent-adjacent flag changed itself on a QA box and nothing in the app - * could say what wrote it. Every candidate was excluded by reading the code, which is exactly + * Written because a consent-adjacent flag changed itself and nothing in the app could say + * what wrote it. Every candidate writer was excluded by reading the code, which is exactly * the situation a log has to cover: the useful question is not "which of the writers I know - * about ran" but "who ran", and only a stack answers that. A per-call-site tag would have - * annotated the sites already ruled out and stayed silent on the one that matters. - * - * At `save`, not at `set`, because `set` is not the only writer — the seed and the - * directory-repair path both persist whole objects without going through it. + * about ran" but "who ran", and only a stack answers that. * - * Diffed against what is on disk, so a save that changes nothing says nothing. Settings are - * written on user actions rather than in loops, so the extra read is not a hot path. */ -function logPersistedChanges(next: Settings): void { + * The baseline comes from the caller's already-loaded object, NOT from re-reading the file. + * Re-reading looked simpler and was wrong three ways: `readFileSafe` increments the + * process-wide `.bak`-fallback counter that telemetry reports, it blocks the main thread on + * `Atomics.wait` while retrying a locked file, and it cannot tell "no previous value" from + * "previous file unparseable". Reading memory has none of those costs. */ +function logPersistedChanges(before: Settings | undefined, next: Settings): void { try { - const read = readFileSafe(dataPath) - let before: Record = {} - if (read.kind === 'data') { - const parsed: unknown = JSON.parse(read.data) - if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { - before = parsed as Record - } - } - const brief = (v: unknown): string => { - if (v === undefined) return '' - const text = JSON.stringify(v) ?? String(v) - return text.length > 120 ? `${text.slice(0, 117)}...` : text - } - const keys = new Set([...Object.keys(before), ...Object.keys(next as object)]) + if (!before) return + const a = before as Record + const b = next as Record const changes: string[] = [] - for (const key of keys) { - const a = before[key] - const b = (next as Record)[key] - if (JSON.stringify(a) === JSON.stringify(b)) continue - changes.push(`${key}: ${brief(a)} -> ${brief(b)}`) + for (const key of new Set([...Object.keys(a), ...Object.keys(b)])) { + if (sameValue(a[key], b[key])) continue + changes.push(`${JSON.stringify(key)}: ${describeForLog(a[key])} -> ${describeForLog(b[key])}`) } if (changes.length === 0) return // Frames 0-1 are this helper and `save`; the caller starts after them. - const stack = (new Error().stack ?? '').split('\n').slice(3, 9).join('\n') - console.log(`Settings: writing ${changes.join(', ')}\n${stack}`) + const stack = (new Error().stack ?? '') + .split('\n') + .slice(3, 9) + .map((line) => line.trim()) + .join(' <- ') + console.log(`Settings: wrote ${changes.join(', ')} | via ${stack}`) } catch { - // Diagnostics must never cost a write. A failure here is silent on purpose. + // Diagnostics must never cost a write, and must never be the reason one is lost. } } -function save(settings: Settings): void { - logPersistedChanges(settings) +/** `before` is the caller's pre-mutation snapshot, used only for the change log. Logged AFTER + * the write lands: `writeFileSafe` can throw, and a line saying a value was written when it + * was not is worse than no line. */ +function save(settings: Settings, before?: Settings): void { writeFileSafe(dataPath, JSON.stringify(settings, null, 2), { backup: true }) + logPersistedChanges(before, settings) } /** Sentinel values for `autoLaunchOnStartup`. Any string OTHER than these @@ -732,12 +767,14 @@ export function set( (typeof value === 'string' && value.trim() === '' && EMPTY_STRING_MEANS_UNSET.has(key)) || (DEFAULT_VALUE_MEANS_UNSET.has(key) && value === DEFAULT_VALUE_MEANS_UNSET.get(key)) ) { + const before = structuredClone(settings) delete settings[key] - save(settings) + save(settings, before) return } + const before = structuredClone(settings) settings[key] = value - save(settings) + save(settings, before) } export function getAll(): Settings { @@ -759,8 +796,9 @@ export function resolveBetaFeaturesEnabled(): boolean { if (typeof stored === 'boolean') return stored if (unreadable) return false const seeded = settings.telemetryEnabled === true + const before = structuredClone(settings) settings.betaFeaturesEnabled = seeded - save(settings) + save(settings, before) return seeded } From 9c99a3b552a43cf709a316ab6b77647edd3c1a7a Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Tue, 22 Sep 2026 01:55:11 -0700 Subject: [PATCH 3/5] fix(settings): baseline the change log from disk, not the merged defaults MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit caught that the baseline was taken after defaults were merged in, so every key a sparse settings.json gains on its first real write was already present in the baseline and logged as no change. `set`, the beta seed and the directory-repair path all shared the gap. That matters more than the missing lines. The whole claim this log supports is "key X was written, by this caller" — and its contrapositive, that no line for X means X was not written. A baseline that already contains the defaults quietly breaks the second half, so an absence stops being evidence. Given the log exists because a flag changed itself and nothing could say what wrote it, an instrument that can silently omit writes is the wrong instrument. `loadOutcome` now also returns what it actually parsed from disk, and the three writers baseline from that. It is still memory rather than a re-read, so none of the reasons the earlier version stopped re-reading are reintroduced. Yes, this means the first write against a sparse file logs every default as a change. That is true — those keys really do reach disk for the first time — and a noisy truth beats a quiet omission in something whose job is attribution. Co-Authored-By: Claude Opus 5 --- src/main/settings.test.ts | 17 ++++++++++++++++ src/main/settings.ts | 42 +++++++++++++++++++++------------------ 2 files changed, 40 insertions(+), 19 deletions(-) diff --git a/src/main/settings.test.ts b/src/main/settings.test.ts index 66ef92d57..52cd43e85 100644 --- a/src/main/settings.test.ts +++ b/src/main/settings.test.ts @@ -752,6 +752,23 @@ describe('persisted-write logging', () => { log.mockRestore() }) + it('logs a key the file gains for the first time', () => { + // The baseline is what was on DISK, not the defaults-merged view. Baselining from the + // merged object would hide every key a sparse file gains on its first real write — they + // are already present in a merged baseline — and "no line for key X" would stop meaning + // "X was not written", which is the only claim this log exists to support. + fs.mkdirSync(path.dirname(settingsPath), { recursive: true }) + fs.writeFileSync(settingsPath, JSON.stringify({ betaFeaturesEnabled: true })) + const log = vi.spyOn(console, 'log').mockImplementation(() => {}) + + settings.set('telemetryEnabled', false) + + // `installDir` is a default the sparse file above does not contain, so this write is the + // first time it reaches disk. + expect(writeLines(log).some((l) => l.includes('installDir'))).toBe(true) + log.mockRestore() + }) + it('describes a string value by shape instead of printing it', () => { // These lines land in `app.log`, which users attach to support requests. A path or a // mirror host must not be disclosed just because it changed; the shape still answers diff --git a/src/main/settings.ts b/src/main/settings.ts index 3ededfdb4..da85afb80 100644 --- a/src/main/settings.ts +++ b/src/main/settings.ts @@ -463,13 +463,19 @@ function load(): Settings { * content is unknown, so saving anything derived from the stand-in would * overwrite the user's intact, newer settings (the failure environment of * issue #1367). */ -function loadOutcome(): { settings: Settings; unreadable: boolean } { +function loadOutcome(): { + settings: Settings + unreadable: boolean + /** Exactly what was parsed from disk, before defaults are merged in — the baseline the + * change log needs, so a key the file gains for the first time is reported as a change. */ + persisted: Record +} { maybeSeedFromEnv() let parsed: Record | null = null let unreadable = false const read = readFileSafe(dataPath) if (read.kind === 'unreadable') { - return { settings: { ...defaults }, unreadable: true } + return { settings: { ...defaults }, unreadable: true, persisted: {} } } if (read.kind === 'data') { unreadable = read.primaryUnreadable === true @@ -490,9 +496,6 @@ function loadOutcome(): { settings: Settings; unreadable: boolean } { } const result: Settings = { ...defaults, ...(parsed || {}) } let changed = false - // Snapshot before the directory substitutions below, so the repair path attributes its - // own writes rather than logging nothing. - const repairBaseline = structuredClone(result) // Drop legacy keys that no longer back any setting. `maxCachedFiles` was the // user-editable predecessor of `maxCachedDownloads`; its old value is @@ -634,8 +637,8 @@ function loadOutcome(): { settings: Settings; unreadable: boolean } { changed = true } } - if (changed && !unreadable) save(result, repairBaseline) - return { settings: result, unreadable } + if (changed && !unreadable) save(result, parsed ?? {}) + return { settings: result, unreadable, persisted: parsed ?? {} } } /** Describe a value for the log WITHOUT disclosing it. @@ -684,15 +687,19 @@ function sameValue(a: unknown, b: unknown): boolean { * the situation a log has to cover: the useful question is not "which of the writers I know * about ran" but "who ran", and only a stack answers that. * - * The baseline comes from the caller's already-loaded object, NOT from re-reading the file. + * The baseline is what was actually PARSED FROM DISK, not the defaults-merged view. Merging + * first would hide the keys a sparse file gains on its first real write: they are already + * present in a merged baseline, so nothing would be logged for them, and "no line for key X" + * would stop meaning "X was not written" — which is the only claim this log exists to + * support. It is still memory, not a re-read. * Re-reading looked simpler and was wrong three ways: `readFileSafe` increments the * process-wide `.bak`-fallback counter that telemetry reports, it blocks the main thread on * `Atomics.wait` while retrying a locked file, and it cannot tell "no previous value" from * "previous file unparseable". Reading memory has none of those costs. */ -function logPersistedChanges(before: Settings | undefined, next: Settings): void { +function logPersistedChanges(before: Record | undefined, next: Settings): void { try { if (!before) return - const a = before as Record + const a = before const b = next as Record const changes: string[] = [] for (const key of new Set([...Object.keys(a), ...Object.keys(b)])) { @@ -715,7 +722,7 @@ function logPersistedChanges(before: Settings | undefined, next: Settings): void /** `before` is the caller's pre-mutation snapshot, used only for the change log. Logged AFTER * the write lands: `writeFileSafe` can throw, and a line saying a value was written when it * was not is worse than no line. */ -function save(settings: Settings, before?: Settings): void { +function save(settings: Settings, before?: Record): void { writeFileSafe(dataPath, JSON.stringify(settings, null, 2), { backup: true }) logPersistedChanges(before, settings) } @@ -750,7 +757,7 @@ export function set( key: K, value: K extends KnownSettingKey ? KnownSettings[K] | undefined : unknown ): void { - const { settings, unreadable } = loadOutcome() + const { settings, unreadable, persisted } = loadOutcome() if (unreadable) { // Fail closed (issue #1367): settings.json exists but can't be read right // now, so `settings` holds bare defaults or stale .bak content. Persisting @@ -767,14 +774,12 @@ export function set( (typeof value === 'string' && value.trim() === '' && EMPTY_STRING_MEANS_UNSET.has(key)) || (DEFAULT_VALUE_MEANS_UNSET.has(key) && value === DEFAULT_VALUE_MEANS_UNSET.get(key)) ) { - const before = structuredClone(settings) delete settings[key] - save(settings, before) + save(settings, persisted) return } - const before = structuredClone(settings) settings[key] = value - save(settings, before) + save(settings, persisted) } export function getAll(): Settings { @@ -791,14 +796,13 @@ export function getAll(): Settings { * beta by revoking consent, taking the diagnostics with them. */ export function resolveBetaFeaturesEnabled(): boolean { - const { settings, unreadable } = loadOutcome() + const { settings, unreadable, persisted } = loadOutcome() const stored = settings.betaFeaturesEnabled if (typeof stored === 'boolean') return stored if (unreadable) return false const seeded = settings.telemetryEnabled === true - const before = structuredClone(settings) settings.betaFeaturesEnabled = seeded - save(settings, before) + save(settings, persisted) return seeded } From c8618d6185248e35e1f4c96bb7c61de722f470ba Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Tue, 22 Sep 2026 02:03:43 -0700 Subject: [PATCH 4/5] fix(settings): snapshot the disk baseline before null normalisation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `loadOutcome` strips `null`s the schema does not allow, and I was returning the object after that ran. So a key stored as `null` reached the change log as absent, and its line claimed ` -> value` when the truth was `null -> value`. Small, but the wrong kind of small for this file: the log's only job is to say what a write changed, and a baseline that has been quietly normalised makes it restate the state it is attributing against. The `null` case is also the one that matters most here — a stored `null` is what makes `resolveBetaFeaturesEnabled` treat the key as unset and re-seed it, which is one of the paths this log exists to catch in the act. Baseline now captured before the normalisation loop, with a test pinning that a stored `null` is reported as `null`. Raised by CodeRabbit. Co-Authored-By: Claude Opus 5 --- src/main/settings.test.ts | 15 +++++++++++++++ src/main/settings.ts | 10 ++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/main/settings.test.ts b/src/main/settings.test.ts index 52cd43e85..b61109f13 100644 --- a/src/main/settings.test.ts +++ b/src/main/settings.test.ts @@ -769,6 +769,21 @@ describe('persisted-write logging', () => { log.mockRestore() }) + it('reports a stored null as null, not as absent', () => { + // `loadOutcome` strips `null`s the schema does not allow, so the baseline is taken before + // that. Otherwise a key stored as `null` reads as never-present and the line claims + // ` -> value` — a log that restates the state it is attributing against. + fs.mkdirSync(path.dirname(settingsPath), { recursive: true }) + fs.writeFileSync(settingsPath, JSON.stringify({ betaFeaturesEnabled: null })) + const log = vi.spyOn(console, 'log').mockImplementation(() => {}) + + settings.set('betaFeaturesEnabled', true) + + const line = writeLines(log).find((l) => l.includes('betaFeaturesEnabled')) + expect(line).toContain('"betaFeaturesEnabled": null -> true') + log.mockRestore() + }) + it('describes a string value by shape instead of printing it', () => { // These lines land in `app.log`, which users attach to support requests. A path or a // mirror host must not be disclosed just because it changed; the shape still answers diff --git a/src/main/settings.ts b/src/main/settings.ts index da85afb80..e6a1df0ce 100644 --- a/src/main/settings.ts +++ b/src/main/settings.ts @@ -487,6 +487,12 @@ function loadOutcome(): { console.warn('Settings: failed to parse settings JSON:', (err as Error).message) } } + // Captured BEFORE the normalisation below, which deletes `null`s that the schema does not + // allow. The change log's baseline has to be what the file literally held: a key stored as + // `null` would otherwise read as absent, and the line for it would claim ` -> value` + // when the truth is `null -> value`. A log whose job is attribution should not quietly + // restate the state it is attributing against. + const persisted: Record = { ...(parsed ?? {}) } if (parsed) { for (const key of KNOWN_SETTING_KEYS) { if (parsed[key] === null && !isNullableKnownSettingKey(key)) { @@ -637,8 +643,8 @@ function loadOutcome(): { changed = true } } - if (changed && !unreadable) save(result, parsed ?? {}) - return { settings: result, unreadable, persisted: parsed ?? {} } + if (changed && !unreadable) save(result, persisted) + return { settings: result, unreadable, persisted } } /** Describe a value for the log WITHOUT disclosing it. From b4d5becec298238da722234cd36ccaa2f04738a3 Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Tue, 22 Sep 2026 02:11:09 -0700 Subject: [PATCH 5/5] fix(settings): log what reached disk, not what was in memory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `JSON.stringify` turns `NaN` and `Infinity` into `null` and drops `undefined` properties, so the object being saved and the bytes being written genuinely disagree. A renderer can set a key to `NaN` and the file receives `null`, while the log reported `NaN` — a value the file does not contain, in the one place whose entire purpose is to say what reached disk. The payload is now serialised once, written, and read back for the diff. The log describes the bytes. This is the fourth finding on this PR of the same shape, and the pattern is worth naming: every one has been the diagnostic describing something other than what happened — the wrong moment, the wrong baseline, a normalised baseline, and now the wrong representation. An instrument that is subtly wrong is worse than none, because it is believed. Raised by CodeRabbit as an outside-diff comment, which is the channel that only appears in the panel body. Co-Authored-By: Claude Opus 5 --- src/main/settings.test.ts | 16 ++++++++++++++++ src/main/settings.ts | 19 +++++++++++++++---- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/main/settings.test.ts b/src/main/settings.test.ts index b61109f13..7e8820e35 100644 --- a/src/main/settings.test.ts +++ b/src/main/settings.test.ts @@ -784,6 +784,22 @@ describe('persisted-write logging', () => { log.mockRestore() }) + it('reports what reached disk, not what was in memory', () => { + // `JSON.stringify` turns NaN into null. Logging the in-memory object would report a value + // the file does not contain, in the one place that exists to say what reached disk. + fs.mkdirSync(path.dirname(settingsPath), { recursive: true }) + fs.writeFileSync(settingsPath, JSON.stringify({ maxCachedDownloads: 1 })) + const log = vi.spyOn(console, 'log').mockImplementation(() => {}) + + settings.set('maxCachedDownloads', Number.NaN as unknown as number) + + const line = writeLines(log).find((l) => l.includes('maxCachedDownloads')) + expect(line).toBeDefined() + expect(line).toContain('-> null') + expect(line).not.toContain('NaN') + log.mockRestore() + }) + it('describes a string value by shape instead of printing it', () => { // These lines land in `app.log`, which users attach to support requests. A path or a // mirror host must not be disclosed just because it changed; the shape still answers diff --git a/src/main/settings.ts b/src/main/settings.ts index e6a1df0ce..02e703d30 100644 --- a/src/main/settings.ts +++ b/src/main/settings.ts @@ -702,11 +702,16 @@ function sameValue(a: unknown, b: unknown): boolean { * process-wide `.bak`-fallback counter that telemetry reports, it blocks the main thread on * `Atomics.wait` while retrying a locked file, and it cannot tell "no previous value" from * "previous file unparseable". Reading memory has none of those costs. */ -function logPersistedChanges(before: Record | undefined, next: Settings): void { +function logPersistedChanges( + before: Record | undefined, + writtenPayload: string +): void { try { if (!before) return const a = before - const b = next as Record + const parsed: unknown = JSON.parse(writtenPayload) + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) return + const b = parsed as Record const changes: string[] = [] for (const key of new Set([...Object.keys(a), ...Object.keys(b)])) { if (sameValue(a[key], b[key])) continue @@ -729,8 +734,14 @@ function logPersistedChanges(before: Record | undefined, next: * the write lands: `writeFileSafe` can throw, and a line saying a value was written when it * was not is worse than no line. */ function save(settings: Settings, before?: Record): void { - writeFileSafe(dataPath, JSON.stringify(settings, null, 2), { backup: true }) - logPersistedChanges(before, settings) + // Serialised once, and the log reads back THAT payload rather than the in-memory object. + // `JSON.stringify` turns `NaN` and `Infinity` into `null` and drops `undefined`, so the two + // genuinely disagree: a renderer can set a key to `NaN` and the file gets `null`. Logging + // the object would report a value the file does not contain — which is the one thing a + // change log must never do, since its whole purpose is to say what reached disk. + const payload = JSON.stringify(settings, null, 2) + writeFileSafe(dataPath, payload, { backup: true }) + logPersistedChanges(before, payload) } /** Sentinel values for `autoLaunchOnStartup`. Any string OTHER than these