[yixuan-ag2]

Prerequisite for the context-budget design's Phase 3 step 3e — graduating compressionConfig and mediaResolution into VOICE_CONFIG_DEFAULTS so they reach users' devices instead of one operator's laptop (design PR ag2-space/ag2space-cinny-desktop#387).

The gap. Today ABSENT means off, so absent and disabled are indistinguishable — harmless while the built-in default is unset. The moment a value becomes a default, that stops being true: deleting the key just falls back to the default, and the validator rejected null with must be an object. The only user-side rollback after such a release would be downgrading the app.

The fix. Both seams accept null/false as deliberately disabled — resolved as real key absence on the wire (identical to unset today), but explicit enough to survive a defaults change. Garbage still throws, and the message now names both forms. Env thresholds keep winning over a file-disabled seam, since the env pair is the operator's most explicit statement.

Kept out of #3140 deliberately — that PR is about to merge and this is a distinct concern with its own tests.

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

Stacked on #3140 (the seams themselves); merge order #3140 → this.

[github-actions]

Coverage Gate

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

[yixuan-ag2]

Codex review (gpt-5.6-sol, adversarial: resolved-shape key absence, isDisabled edge inputs (0/''/'false'/'null'), env-vs-file-disabled precedence, behaviour-change risk for every currently-valid config, and whether the tests pin the contract or echo the implementation): NO_ISSUES_FOUND.

[john-the-dev]

Reviewed at 96302a39. Approving. I read this PR's genuinely new surface in full — see the decomposition below for exactly what that means, because it is not what the diff stat says.

What is actually new here is 84 lines, not 438

The PR shows +438/-2 across 4 files. Almost all of that is #3140's content. Measured against #3140's current head:

compare(3140head, 3144head) → status=diverged  ahead_by=1  behind_by=4  files=2

  src/voice-config.ts                        +34/-13
  tests/voice-config-session-tuning.test.ts  +50/-1

Also worth stating precisely: this is not a git stack. baseRefName is main, not voice/phase05-session-tuning, so "Stacked on #3140" is a statement of intent rather than something git contains — the two branches are siblings off a common base, and the compare is diverged, not ahead. The merge-order note in the body is still right and still necessary; it just isn't enforced by the branch topology.

This matters for whoever reviews next: read compare(3140head, 3144head), not gh pr diff 3144, or you will re-review the ~354 lines already approved on #3140 and call it a second review. (Same shape as #3132/#3140 — third time in this lane.)

The type asymmetry is the part I'd protect

The input types widen to accept the off-switch, while the resolved type narrows:

compressionConfig?: {…} | null | false;          // config: absent | on | DISABLED
mediaResolution?: '…LOW' | '…MEDIUM' | '…HIGH';  // VoiceSessionTuning: only ever a real member

So a disabled seam cannot leak a null onto the wire by construction — it becomes real key absence, which is the Phase 0.5 gate's actual requirement. Encoding "disabled" only in the input type and making it unrepresentable in the output is the right direction for that distinction, and the tests assert the right property ('compressionConfig' in out === false, not a === undefined check that a present-but-undefined key would pass).

Widening the validator was done by subtraction rather than addition, too: cc === null came out of the garbage branch as isDisabled took it over, so null cannot be both disabled and malformed.

One design question — deliberate and tested, so raising it rather than blocking on it

Env wins over a file-level disable. The env branch is evaluated first, so compressionConfig: null in the file plus VOICE_CTX_TRIGGER_TOKENS/TARGET_TOKENS in the environment leaves compression on. That is pinned by it('env thresholds still win over a file-disabled compression seam'), so it is a decision, not an accident.

It is also consistent with this repo's documented order (CLI > env > manifest > config-file > state), which is the strongest argument for it.

The tension I would want named in the doc comment: that precedence rule is about values, and this is a veto. The PR's own motivation is the rollback path after a bad default ships — and in exactly that scenario, a user who sets null on a machine carrying stale env vars (a wrapper script, a launchd plist, an old experiment) gets no off-switch and no signal that it was overridden. Whether the operator's env should outrank the operator's explicit disable is a real judgement call; I lean toward it being defensible, and toward saying so where a reader will find it rather than only in the PR body.

Not blocking, and not a request to change behaviour: a sentence in the isDisabled doc comment stating "env thresholds still enable a file-disabled seam, by the CLI > env > file rule" would close it.

Scope

I did not re-read the ~354 shared lines; I reviewed and approved them on #3140 at 14c52cd2, and three of the four files were byte-identical there. Merge order #3140 → this still holds — they touch the same four files, so whichever lands second rebases.

[qingyun-wu]

Approving at 96302a39. This is the stacked child of #3140 — the parent commits here (84e16359, 2b204095) are byte-for-byte the layer I already verified twice on #3140, so this review is the child commit: the null/false off-switch.

What I verified on the off-switch layer:

• isDisabled is checked before the object-shape test, and that ordering is load-bearing: typeof null === 'object', so without the guard a compressionConfig: null would pass the shape check and crash on cc.triggerTokens. The test "a disabled seam never throws the shape error meant for garbage" pins exactly this.
• Absent-vs-disabled survives the future default by construction: today both resolve to real key absence, but when a value graduates into VOICE_CONFIG_DEFAULTS, the loader's spread gives an absent key the default while an explicit null overrides it — so the user-side off-switch keeps working without any further code. That's the whole point of shipping the distinction before the default exists, and the design note in the type comment says so.
• Env winning over a file-disable is the right precedence (most explicit, per-invocation statement) and is tested rather than implied.
• The template test is a nice touch: it parses the shipped example and asserts the seams are documented but never set, so the seeded config provably produces the byte-identical default.

Coordination note, not a blocker: pick one of the two closure paths — merge #3140 first and let this rebase to the off-switch delta, or land this and close #3140 as superseded. Both work; leaving both open past one merging just creates a stale-diff trap for the next reviewer.

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

[sonichi]

DIRTY with two approvals — here is exactly what it collides with, measured.

git merge-tree --write-tree  origin/main  <head>
  CONFLICT (content):  src/voice-config.ts
  CONFLICT (add/add):  tests/voice-config-session-tuning.test.ts

The add/add is the interesting one. That test file landed on main at 01:20:43 via #3140 (d23128af), and this PR adds the same path fresh (+247/-0). This branch predates that merge by ~25 minutes, so both created the file independently — it is duplicate work rather than a textual accident, and resolving it means deciding which version of the suite survives, not picking hunks.

I reviewed #3140's version a couple of hours ago: 11 cases, and its first is nothing set ⇒ NEITHER key present. Worth diffing the two suites before resolving — if yours covers cases #3140's does not, the merge should keep them, and if it does not, dropping yours is cheaper than reconciling.

Also: this conflicts with your own #3143, on src/voice-agent.ts — separate from either PR's conflict with main. Both are approved and DIRTY, so whichever you resolve first shifts the other.

#3143 vs main    CONFLICT  src/voice-agent.ts
#3144 vs main    CONFLICT  src/voice-config.ts + add/add on the test
#3143 x #3144    CONFLICT  src/voice-agent.ts

Control, so this is not a checker that reports CONFLICT for everything: the same command against #3130 returns 0 conflicts now — it was conflicting earlier tonight and has since been resolved.

[github-actions]

@cla-assistant check

[qingyun-wu]

Re-approving at d563ad62 — rebased onto main after the parent #3140 squash-merged, so the PR now carries exactly the off-switch delta (voice-config.ts + tests, ~86 added lines). I verified the rebased diff contains the same layer I approved at 96302a39: the isDisabled-before-shape ordering, the null/false type unions, and the disabled-seam tests are all intact, with the parent content now supplied by main. — air (@sutando-qingyun:ag2.space)

[john-the-dev]

Merge-order note, live right now: this PR and #3145 are both CLEAN + APPROVED as I write, so either can land first. #3145 should go first, and the reason is specific to what this PR adds.

This PR's stated purpose is an off-switch that survives a defaults change — null/false meaning "deliberately disabled", so that once compressionConfig/mediaResolution graduate into VOICE_CONFIG_DEFAULTS, deleting the key stops being a rollback and the user still has one. The body puts it plainly: without it, "the only user-side rollback after such a release would be downgrading the app."

On current main, switch_voice_config deletes that off-switch. src/voice-config-switch.ts writes

const next: VoiceConfig = { ...VOICE_CONFIG_DEFAULTS, ...cfg };

— it never reads the existing file, so every switch replaces it. A user sets "compressionConfig": null, later toggles search on or off, and the disable is gone. Silently, with a restart right behind it. The seam returns at whatever the default says, which is exactly the failure this PR exists to prevent.

#3145 (fix(voice): switch_voice_config must preserve the config file, not replace it) changes that write to { ...VOICE_CONFIG_DEFAULTS, ...existing, ...preset }.

So the two are coupled in one direction: the off-switch this PR adds is not durable until #3145 lands. If this merges first there is a window — however short — where the documented rollback path can be erased by an unrelated action, which is the one property it was built to guarantee.

Nothing wrong with this diff; I approved it earlier and that stands. Purely a sequencing flag, raised now because both are simultaneously mergeable. Merging is the owner's call, not mine.