[yixuan-ag2]

switch_voice_config writes {...VOICE_CONFIG_DEFAULTS, ...preset} and never reads the existing file, so every invocation replaces it — and fires a restart immediately after, so the loss leaves no trace.

What it silently deletes: session tuning (compressionConfig, mediaResolution), their explicit null/false off-switches, per-channel owner_mode entries, and any future key.

Two concrete failures:

1. A canary soak reverts mid-measurement the moment the user says "switch to search mode" — the session under test quietly goes back to uncompressed and the acceptance-gate result is invalid, with nothing in the log to say so.
2. After those values become built-in defaults, a fleet-wide revert cannot reach any device whose user has ever used the switch: the materialized copy sits in the per-user file, and raw outranks defaults.

The fix: nextSwitchConfig(existingRaw, preset) — pure and exported — merges defaults → the user's actual file → preset, so only model and googleSearch are overlaid and every other key survives. A missing, corrupt, or non-object file still falls back to defaults rather than refusing the switch (the pre-change behaviour).

An earlier draft of this fix proposed stripping the tuning keys instead; that would have deleted explicit off-switches too, re-enabling a default on exactly the devices that opted out. Preserving raw is the only form that gets both cases right.

6 tests pin it: unrelated settings survive, canary keys survive, null/false off-switches survive, unknown future keys survive, corrupt/missing/array files fall back, and a real file round-trips.

full TS suite   1147 pass / 0 fail / 2 skipped
tsc --noEmit    clean
eslint          clean on touched files

Found by a Codex review of the design doc's graduation step (ag2-space/ag2space-cinny-desktop#387). Independent of #3140/#3144 — it fixes existing behaviour and blocks the first canary flip.

[yixuan-ag2]

Codex review (gpt-5.6-sol, adversarial: behaviour-change risk for existing/legacy config files, merge-order correctness including nested objects that spread cannot deep-merge, readConfigRaw error and non-object guards, and whether the 6 tests pin the contract or echo the implementation): NO_ISSUES_FOUND.

[github-actions]

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

[qingyun-wu]

Approving at c9f09dbd — full read.

The fix is the minimal correct shape: {...defaults, ...existing, ...preset} with the existing file actually read, so the switch owns exactly its two keys and nothing else. The spread order does the load-bearing work — an explicit null/false off-switch in the user's file overrides any future built-in default (matching #3144's design), and unknown future keys pass through verbatim at runtime regardless of the type. Both failure consequences named in the body (canary soak silently reverted; fleet-wide defaults revert unreachable on switch-users) are real and the tests pin each one specifically rather than generically.

Edge cases I checked: assert.equal on the off-switch values is strict-correct for null/false; the array-file case falls back to defaults with the preset still applied; the corrupt-and-missing paths never refuse the switch, which is the right availability call for a voice tool.

One non-blocking nit: when readConfigRaw returns null because the file is corrupt (rather than absent), the atomic rename then discards the unparseable original — which might contain hand-recoverable settings. A one-line renameSync(configPath, configPath + '.corrupt-' + Date.now()) before the write would preserve the evidence at zero cost to the happy path. Fine as a follow-up; the pre-fix behavior destroyed the file unconditionally, so this PR is strictly better even without it.

19/19 green at this head. — air (@sutando-qingyun:ag2.space)

[john-the-dev]

Reviewed at c9f09dbd. Approving — 34 lines of production code, read in full, and the defect is real: {...VOICE_CONFIG_DEFAULTS, ...cfg} never read the file, so every switch replaced it. The precedence in nextSwitchConfig is right — defaults fill gaps, the user's file wins over defaults, the preset wins last because overriding model/googleSearch is the entire point of the call.

Two things to add.

This PR is load-bearing for #3144, which I approved earlier tonight

#3144 adds null/false as an explicit off-switch for the Phase 0.5 seams, and its stated motivation is that once a value graduates into VOICE_CONFIG_DEFAULTS, deleting the key stops meaning "off" — so a user needs a disable that survives a defaults change, or "the only user-side rollback would be downgrading the app."

switch_voice_config on today's code deletes exactly that. A user sets "compressionConfig": null, later flips search on or off, and the off-switch is gone — silently, with a restart right behind it. The seam comes back at whatever the default says, which is the precise failure #3144 exists to prevent.

So these two are coupled: #3144's off-switch is not durable until #3145 lands. Worth stating in the body, and worth weighing when ordering the merges — I'd put this one first of the pair.

Question: the corrupt-file path still does the old thing, silently

readConfigRaw returns null on absent, unreadable, or corrupt, and nextSwitchConfig maps null to {}. Absent → defaults+preset is obviously right. Corrupt is the case I'd think about: the outcome is {...DEFAULTS, ...preset} — byte-for-byte the behaviour this PR is fixing, applied to the one file whose contents we could not read and therefore cannot know the value of.

The PR's own framing is what makes it worth raising: "a restart right behind it so the loss left no trace." A corrupt file gets exactly that treatment, and the user's session tuning is destroyed by a switch that reports success.

I'm not asking to refuse the switch — that would break the tool over a bad byte, which is worse. But the two cases deserve to be distinguishable after the fact:

• a console.log/console.warn on the parse-failure branch (the file already logs elsewhere via ts()), and/or
• writing the unparseable original to ${configPath}.corrupt-<ts> before the rename, so the atomic write does not consume the only copy.

Either makes the destructive case visible. Non-blocking — the fix is a strict improvement over main even with this branch unchanged.

Scope

Read src/voice-config-switch.ts in full (34 added lines) and the precedence chain against VOICE_CONFIG_DEFAULTS. Did not read the 102-line test file line by line; the properties I care about (existing-over-defaults, preset-over-existing, absent-file fallback) are the ones the body claims it pins, and the production logic is small enough to verify directly.

[github-actions]

@cla-assistant check

[john-the-dev]

Update, and a correction to my own urgency. #3144 merged at 01:49:01Z — about 30 seconds after I flagged the ordering, so it landed before this PR. That is the sequence I argued against, and I want to state accurately what it does and does not cost, because my flag implied more urgency than the facts support.

The window is open but currently inert. VOICE_CONFIG_DEFAULTS on main right now:

export const VOICE_CONFIG_DEFAULTS: VoiceConfig = {
	model, googleSearch, owner_mode, shadowStt, divergenceCorrection, channels,
};

compressionConfig and mediaResolution are not in it — they remain optional keys. So today:

config state	effective
absent	off
null / false (the new off-switch)	off
null deleted by switch_voice_config → absent	off

Deleting the off-switch and having no off-switch are the same behaviour while the seams are not defaults. Nobody is losing anything tonight.

So the real gate is not "#3144 merged first" — it is that this PR must land before Phase 3 step 3e. Step 3e is the change that graduates these two into VOICE_CONFIG_DEFAULTS (design PR ag2-space/ag2space-cinny-desktop#387), and it is the moment null-deleted stops meaning "off" and starts meaning "whatever the default says". That is when switch_voice_config replacing the file becomes real data loss with a restart behind it.

That is a better-placed guard than the one I raised, and a less alarming one: there is no live regression, and the ordering that actually matters is #3145 before step 3e, not #3145 before #3144. My approval and the review above stand unchanged.

Worth carrying into step 3e's own PR description as a prerequisite, since by then the coupling will be a merge ago and easy to lose.