[FFFengMJL]

Problem

The advisor has no "configured but opt-in" mode. enabled is exactly the provider/model gate, and a session with no explicit override is ON (override ?? true). To review only chosen sessions, the only workaround is clearing provider/model — which also removes the model, so every opt-in must re-pin a full pair with /advisor model set instead of a single /advisor on.

Closes #124.

Change

Add a persisted boolean key defaultEnabled (default true) to the §5.1 entry config:

• resolveAdvisorConfig normalizes it on every path (the model gate is unchanged); it is a new key, not in LEGACY_KEYS, and unknown-key strictness is untouched.
• Session resolution: effective enable = session override ?? defaultEnabled, AND the provider/model gate. An explicit /advisor off keeps today's "no reason" status; a default-off session reports disabled by default — enable it for this session with /advisor on.
• Live re-apply (bridge.onChange): the enable switch is reconciled for tracked sessions — a flip to false disposes their runtimes, a flip to true applies to sessions already tracked (a session with no runtime and no override resolves on its next event). The runtime-signature rebuild path is unchanged, and defaultEnabled is deliberately not part of that signature.
• Surfaces: web card checkbox, TUI /settings boolean, /advisor config readback (defaultEnabled: on|off), gateway get/set wire, docs and the frozen spec's §5.1 / §8.6 annotations.

Semantics and compatibility

• Default true keeps every existing profile byte-identical: no stored config change, no behavior change.
• The pair gate still applies first: a missing provider/model keeps its own disabled reason.
• Pinning a session model never toggles the switch (unchanged).
• Enabling a default-off session is /advisor on; the web session-header Advisor action still only pins the model (it has no enable control today), so no new RPC surface is added.

Verification

• pnpm typecheck — clean
• pnpm build — clean
• pnpm test — 645/645 (25 files), including new cases for:
  • config defaults / explicit false / both gate arms / near-miss key rejection;
  • gateway get carrying the key and a NON-default set({ defaultEnabled: false }) round-trip;
  • /advisor config rendering and live edit;
  • web card + store seed/setter/patch and no-churn behavior;
  • TUI section path/kind/copy;
  • live re-apply: flip to false stops a running session, flip back starts on the next event; defaultEnabled: false + /advisor on opts one session in while the persisted default stays false.
• Real dsh Loader / web-GUI host integration was not exercised here; the repo harness composes the real plugin wiring (composeLiveHarness) for the live cases.

[btspoony]

⛔ Verdict: blocked · Score 40% (fail)

Findings	Count
🔴 must-fix	1
🟠 should-fix	1
🔵 nit	2
❓ unverified	0

📋 Review

What this PR does: adds a persisted boolean defaultEnabled (schema default true) to the advisor entry config, so a session with no own enable override resolves as override ?? defaultEnabled ANDed with the existing provider/model gate. It wires the key through the resolver, the gateway get/set wire, /advisor config, the web card, TUI /settings, the live bridge.onChange re-apply, docs and the frozen spec, and keeps the default byte-identical for existing profiles.

Revalidated findings

none

Findings

🔴 A model reset that closes the pair gate leaves the old runtime running

• Evidence: src/index.ts:545 — applyRouteChange early-returns when !effectiveEnabled(sessionId), and this PR changed effectiveEnabled from override ?? true to override ?? defaultEnabled AND the provider/model gate (src/index.ts:373-386). resetModel (src/index.ts:907-912) clears the session pin first, so a session pinned to a valid pair while the global pair is missing (or invalid) now returns before disposeRuntime.
• Impact: /advisor model reset (or the web reset) reports the session as disabled while the old runtime keeps its in-flight call and pending backlog, and a later global-pair repair resumes that stale route before the next rebuild — the §5.3 "a route change aborts the old call and drops its backlog" contract is broken, and the session keeps emitting in a state the switch says is off.
• Merge class: must-fix
• Confidence: HIGH
• Fix sketch: dispose the runtime before the gate return (disposeRuntime(sessionId) when the effective switch is off, rebuild only when on), plus a regression test for "pin reset to a missing global pair aborts the old call".

🟠 An explicit /advisor on|off that matches the current state records no session override

• Evidence: src/commands.ts:705-712 (on returns when already enabled and not recovering), src/commands.ts:717-719 (off returns when already disabled), and src/index.ts:798-805 (the controller writes the override only when the effective switch changes).
• Impact: the new persisted default means "effectively on/off" and "explicitly chosen on/off" are no longer the same state. A session explicitly turned off under defaultEnabled:false stores no false override, so flipping the persisted default to true starts reviewing it; mirror case: an explicit on under true is lost when the default flips to false. The issue's requirement that the per-session override take precedence is only half satisfied.
• Merge class: should-fix
• Confidence: HIGH
• Fix sketch: always record the explicit value on an explicit on/off; keep start/stop plus the KD-5 recovery routing as a separate decision.

🔵 Delete the DOM-id pin in the new defaultEnabled card spec

• Evidence: tests/advisor-card.spec.tsx:895 — expect(toggle.id).toBe('advisor-default-enabled'); getByLabelText(en.defaultEnabled) on the line above already proves the label↔control association the user actually relies on.
• Impact: pins an internal wiring detail instead of product behavior. Non-product-assertion class: incidental / source-shape — delete, or replace with a product-behaviour assertion (no replacement needed; type/checked/hint assertions carry the behavior).
• Merge class: nit
• Confidence: HIGH
• Fix sketch: drop the single id assertion.

🔵 Card header comment states the fields in the wrong order

• Evidence: src/client/advisor-card.tsx:14-16 — the header lists "the paired immuneTurns/maxDeltaMessages numbers plus the proseFallback / nextPrompt / defaultEnabled checkboxes"; the DOM renders the three checkboxes (:369-422) first and the .numberFields group second (:423), which the per-field comment at :374-375 states correctly. This PR edited this comment block and kept the inversion.
• Impact: a comment that contradicts both the DOM and the adjacent comment; only maintenance cost.
• Merge class: nit
• Confidence: HIGH
• Fix sketch: swap the two phrases to match the render order.

Linked-issue AC

Closes #124 —

• effective enable = defaultEnabled when no session override, still behind the pair gate — met (src/index.ts:373-378).
• /advisor on|off override precedence — met (precedence holds); the same-value no-op gap is finding 🟠, not a second AC increment.
• Model pin does not toggle the switch — met (the pin only feeds the pair substitution in resolveSessionConfig).
• New sessions inherit the default — met (no override → defaultEnabled).
• defaultEnabled:false with no override reports a reason distinct from the missing-model reason — met (disabled by default — enable it for this session with /advisor on), and a missing pair still reports its own gate reason.
• Default true keeps existing profiles unchanged — met (schema default, not in LEGACY_KEYS, not in runtimeSignature).

✅ Verified

• Read the pinned diff pack (git diff origin/main...pr-126, 799 changed lines, 25 files) in full across three seats; opened every cited line in the review worktree.
• Traced applyRouteChange / resetModel / ensureRuntime / disposeRuntime / bridge.onChange and the /advisor command handler to confirm both findings' call paths.
• Client surface cross-checked against the nine-key census (src/config.ts, src/gateway.ts), locale parity, and the no-churn patchFor round-trip.
• No tests, build or typecheck were run (read-only review); the PR's own typecheck/build/test claims were not re-executed. The PR states handoff to a real dsh Loader / web-GUI host was not exercised — that gap is unchanged by this review, which is static.

🗑️ Considered & rejected

• Per-candidate sourceConfig() re-run in bridge.onChange: rejected — volatile commits are low-frequency and sourceConfig() reads in-memory bridge references; no measured cost at realistic session counts.
• Gateway readConfig fallback forwarding a non-boolean defaultEnabled: rejected — raw is typed AdvisorConfig, so raw?.defaultEnabled ?? true cannot widen the type.
• defaultEnabled reintroducing the removed config-level enabled: rejected — by design; the issue and the spec notes separate the global row toggle from the new-session initial state.

🛠️ Plan to fix

Expand fix plan

1. src/index.ts — applyRouteChange: dispose before the disabled return
   - `if (!effectiveEnabled(sessionId)) { disposeRuntime(sessionId); return }`
   - keep the unchanged-route short-circuit for the enabled case.
   - tests/settings-live.test.ts: assert that resetting a pinned session while the global pair is
     missing leaves no runtime (no further llm.stream call) and that a later pair repair builds a
     fresh runtime on the new route.

2. commands.ts / index.ts — record the explicit switch
   - `/advisor on` and `/advisor off`: write the override even when the effective value already
     matches, then run the existing start/stop/recovery decision.
   - tests/commands.test.ts: explicit off under defaultEnabled:false leaves a false override; explicit
     on under true leaves a true override; flipping the persisted default afterwards does not change
     either session.

3. tests/advisor-card.spec.tsx — delete line 895 (DOM id pin).

4. src/client/advisor-card.tsx — reorder the header comment phrases to match the DOM.

Verification: `pnpm typecheck && pnpm build && pnpm test` on the branch.

[FFFengMJL]

Addressed all four findings in 7de05e3:

• must-fix — applyRouteChange now disposes the session runtime before returning when the effective switch is off, so resetting a pin under a pairless global aborts the old call and drops its backlog. Regression test: reset-to-pairless makes no further model call, and a later pair repair builds a fresh runtime.
• should-fix — /advisor on|off always route through setEnabled, which now always records the explicit override (the reply text stays "already on/off"). Tests pin: explicit off under defaultEnabled:false survives a flip to true; explicit on under true survives a flip to false.
• nits — dropped the DOM-id assertion in the card spec; reordered the card header comment to match the render order.

Verification on the pushed head: pnpm typecheck clean, pnpm build clean, pnpm test 648/648 (25 files).

[FFFengMJL]

Follow-up on the always-record change (9b68a0e): routing an already-on /advisor on through setEnabled re-ran the KD-5 seed, and seedTo resets the session renderer's cursor/fold state — the next delta would lose the pre-command context. The seed now runs only on a real off→on transition (or a halted/quota recovery); the explicit override is still recorded unconditionally. Added a regression test: an idempotent on during a gated in-flight call leaves the backlog and emission guard intact (both deltas drain, duplicate note still suppressed). Full suite: 649/649.

[btspoony]

⚠️ Verdict: needs fixes · Score 88% (good)

Findings	Count
🔴 must-fix	0
🟠 should-fix	1
🔵 nit	0
❓ unverified	0

📋 Review

What this PR does: increment re-review of 24c4270..9b68a0e (178 changed lines, 9 files) on top of the settled first round. Two fix commits land: 7de05e3 disposes the runtime when a route change closes the pair gate and records the explicit /advisor on|off choice, and 9b68a0e keeps the KD-5 seed off an idempotent /advisor on. Both prior blocking findings are addressed; the round's only remaining issue is the regression test that is supposed to guard the first fix.

Revalidated findings

Prior finding	State	Evidence
🔴 A model reset that closes the pair gate leaves the old runtime running	fixed	src/index.ts:550-552 — the closed-gate branch now calls disposeRuntime before returning; src/advisor-runtime.ts:592-596 aborts the call and clears the queue. resetModel still clears the pin then calls applyRouteChange (:924-928).
🟠 An explicit /advisor on|off that matches the current state records no session override	fixed	src/commands.ts:713-715, :728-731 always route through setEnabled; src/index.ts:811-814 writes the override unconditionally; :823-826 seeds only on an off→on transition or a halted/quota recovery, so an idempotent on no longer resets the renderer.
🔵 DOM-id pin in the new card spec	fixed	tests/advisor-card.spec.tsx:890-898 — the id assertion is deleted; type/checked/hint remain.
🔵 Card header comment field order	fixed	src/client/advisor-card.tsx:14-15 now lists the checkboxes before the paired numbers, matching the DOM and the per-field comment at :374-375.

Findings

🟠 The closed-gate reset regression test cannot catch the regression it names

• Evidence: tests/session-model-override.test.ts:441-464 — the new test never blocks the adapter, never queues a backlog delta, and never asserts that the old note stops delivering; it only checks the request count going 1 → 1 → 2. The repair commits the same pair (stub/stub-model) the pin held, so the post-repair request cannot distinguish "old runtime survived" from "fresh runtime built". A closed gate already blocks new deltas from enqueueing (src/index.ts:483), so the middle count stays 1 either way.
• Impact: restoring the old applyRouteChange early return would still pass this test — the must-fix behaviour (abort the in-flight call, drop the backlog) ships with no regression defence. The production fix itself is correct; the guard is not.
• Merge class: should-fix
• Confidence: HIGH
• Fix sketch: reuse the blocking pattern already in this file (:723-738): block the adapter, queue a backlog delta, reset the pin to the missing global pair, release, then assert no note was delivered and no queued request fired.

Linked-issue AC

Closes #124 —

• effective enable = defaultEnabled when no session override, behind the pair gate — met.
• /advisor on|off override precedence — met; the increment now also records the explicit choice when the effective state already matches, so a later defaultEnabled flip cannot overturn it.
• Model pin does not toggle the switch — met.
• New sessions inherit the default — met.
• defaultEnabled:false with no override reports a reason distinct from the missing-model reason — met.
• Default true keeps existing profiles unchanged — met.

✅ Verified

• Read the increment pack (315 diff lines / 178 changed lines / 9 files) and verified both fix sites plus the new tests by opening them at head 9b68a0e.
• Traced the repaired paths: applyRouteChange closed-gate dispose, setEnabled unconditional override write with the separate runtime decision, ensureRuntime/disposeRuntime idempotence, and the onChange candidate loop's behaviour for a session whose runtime was kept.
• Confirmed the docs and spec sentence ("an explicit /advisor on|off always records the choice") matches the shipped code.
• No tests, build or typecheck were run (read-only review); the increment's own test claims were not re-executed.

🗑️ Considered & rejected

• FakeController setCalls assertions in the idempotent on/off test (tests/commands.test.ts:355, :361): rejected — they are the unit-layer observation of the exact contract this PR introduces ("an already-matching on/off must still reach setEnabled") and follow the file's existing setCalls convention (:316-343); product-level coverage elsewhere does not make them meaningless.
• Re-seeding the observer on an idempotent /advisor on: rejected — 9b68a0e deliberately skips it (!wasOn || recovering), which is the correct reading of the KD-5 seed contract.

🛠️ Plan to fix

Expand fix plan

1. tests/session-model-override.test.ts — make the closed-gate reset test discriminating
   - adapter.blockNext() before the pinned turn;
   - feed a second turn while the call is blocked (backlog);
   - `/advisor model reset` against the pairless global;
   - adapter.release(); assert the inject spy was never called and the backlog request never fired;
   - repair the global pair with a DIFFERENT pair, feed a new delta, assert one request on the new route.
2. pnpm typecheck && pnpm build && pnpm test

[btspoony]

⚠️ Verdict: needs fixes · Score 86% (good)

Corrects review 5597750819: the FakeController wiring assertions below are counted as a nit instead of being rejected.

Findings	Count
🔴 must-fix	0
🟠 should-fix	1
🔵 nit	1
❓ unverified	0

📋 Review

What this PR does: increment re-review of 24c4270..9b68a0e (178 changed lines, 9 files) on top of the settled first round. Two fix commits land: 7de05e3 disposes the runtime when a route change closes the pair gate and records the explicit /advisor on|off choice, and 9b68a0e keeps the KD-5 seed off an idempotent /advisor on. Both prior blocking findings are addressed; the round's only remaining issue is the regression test that is supposed to guard the first fix.

Revalidated findings

Prior finding	State	Evidence
🔴 A model reset that closes the pair gate leaves the old runtime running	fixed	src/index.ts:550-552 — the closed-gate branch now calls disposeRuntime before returning; src/advisor-runtime.ts:592-596 aborts the call and clears the queue. resetModel still clears the pin then calls applyRouteChange (:924-928).
🟠 An explicit /advisor on|off that matches the current state records no session override	fixed	src/commands.ts:713-715, :728-731 always route through setEnabled; src/index.ts:811-814 writes the override unconditionally; :823-826 seeds only on an off→on transition or a halted/quota recovery, so an idempotent on no longer resets the renderer.
🔵 DOM-id pin in the new card spec	fixed	tests/advisor-card.spec.tsx:890-898 — the id assertion is deleted; type/checked/hint remain.
🔵 Card header comment field order	fixed	src/client/advisor-card.tsx:14-15 now lists the checkboxes before the paired numbers, matching the DOM and the per-field comment at :374-375.

Findings

🟠 The closed-gate reset regression test cannot catch the regression it names

• Evidence: tests/session-model-override.test.ts:441-464 — the new test never blocks the adapter, never queues a backlog delta, and never asserts that the old note stops delivering; it only checks the request count going 1 → 1 → 2. The repair commits the same pair (stub/stub-model) the pin held, so the post-repair request cannot distinguish "old runtime survived" from "fresh runtime built". A closed gate already blocks new deltas from enqueueing (src/index.ts:483), so the middle count stays 1 either way.
• Impact: restoring the old applyRouteChange early return would still pass this test — the must-fix behaviour (abort the in-flight call, drop the backlog) ships with no regression defence. The production fix itself is correct; the guard is not.
• Merge class: should-fix
• Confidence: HIGH
• Fix sketch: reuse the blocking pattern already in this file (:723-738): block the adapter, queue a backlog delta, reset the pin to the missing global pair, release, then assert no note was delivered and no queued request fired.

🔵 Delete the FakeController setCalls wiring assertions in the idempotent on/off test

• Evidence: tests/commands.test.ts:355, :361 — expect(controller.setCalls.at(-1)).toMatchObject({ sessionId, enabled }). The asserted outcome ("the explicit choice survives a later defaultEnabled flip") is already proven at product level by the new tests/settings-live.test.ts cases, which commit a default flip and read /advisor status.
• Impact: a mock-seam call echo that pins the handler→fake call sequence instead of user-visible behaviour. Non-product-assertion class: incidental / wiring — delete, or replace with a product-behaviour assertion (the reply-text assertions on the lines above already carry the unit-level behaviour).
• Merge class: nit
• Confidence: HIGH
• Fix sketch: drop the two setCalls assertions; keep the onResult.text / offResult.text checks.

Linked-issue AC

Closes #124 —

• effective enable = defaultEnabled when no session override, behind the pair gate — met.
• /advisor on|off override precedence — met; the increment now also records the explicit choice when the effective state already matches, so a later defaultEnabled flip cannot overturn it.
• Model pin does not toggle the switch — met.
• New sessions inherit the default — met.
• defaultEnabled:false with no override reports a reason distinct from the missing-model reason — met.
• Default true keeps existing profiles unchanged — met.

✅ Verified

• Read the increment pack (315 diff lines / 178 changed lines / 9 files) and verified both fix sites plus the new tests by opening them at head 9b68a0e.
• Traced the repaired paths: applyRouteChange closed-gate dispose, setEnabled unconditional override write with the separate runtime decision, ensureRuntime/disposeRuntime idempotence, and the onChange candidate loop's behaviour for a session whose runtime was kept.
• Confirmed the docs and spec sentence ("an explicit /advisor on|off always records the choice") matches the shipped code.
• No tests, build or typecheck were run (read-only review); the increment's own test claims were not re-executed.

🗑️ Considered & rejected

• Re-seeding the observer on an idempotent /advisor on: rejected — 9b68a0e deliberately skips it (!wasOn || recovering), which is the correct reading of the KD-5 seed contract.

🛠️ Plan to fix

Expand fix plan

1. tests/session-model-override.test.ts — make the closed-gate reset test discriminating
   - adapter.blockNext() before the pinned turn;
   - feed a second turn while the call is blocked (backlog);
   - `/advisor model reset` against the pairless global;
   - adapter.release(); assert the inject spy was never called and the backlog request never fired;
   - repair the global pair with a DIFFERENT pair, feed a new delta, assert one request on the new route.
2. pnpm typecheck && pnpm build && pnpm test

[btspoony]

Taking over the remaining fixes from the review round.

Planned work on this branch:

1. tests/session-model-override.test.ts — make the closed-gate reset test discriminating (I-000044, should-fix): block the adapter, queue a backlog delta, reset the pin against the pairless global, release, assert the old note never delivers and no queued request fires; repair the global pair with a different model so the post-repair request proves a fresh runtime on the new route.
2. tests/commands.test.ts — drop the FakeController setCalls wiring assertions (I-000045, nit) at :355 and :361; the reply-text assertions and the new settings-live cases already carry the behaviour.

Both are test-only changes; no product code is touched. Verification: pnpm typecheck && pnpm build && pnpm test, plus a reverse check that restoring the old applyRouteChange early return now fails the regression test.

[btspoony]

✅ Verdict: ship it · Score 98% (mergeable)

Findings	Count
🔴 must-fix	0
🟠 should-fix	0
🔵 nit	1
❓ unverified	0

📋 Review

What this PR does: increment re-review of 9b68a0e..669e800 (25 changed lines, 2 test files) — the author's follow-up to round 2. It makes the closed-gate reset test actually discriminating (block the adapter, queue a backlog delta, reset against the pairless global, release, assert no delivery and no queued request, then repair with a different pair) and deletes the two FakeController.setCalls wiring assertions. Both round-2 findings are closed; the only remainder is one over-strong comment.

Revalidated findings

Prior finding	State	Evidence
🟠 The closed-gate reset regression test cannot catch the regression it names	fixed	tests/session-model-override.test.ts:449-467 blocks the first call, queues the second delta, resets, releases, then asserts inject was never called and adapter requests stayed at 1. textReply now receives plain text (:444, helper at :63-65), so a surviving runtime would deliver a parseable nit note and drain the backlog.
🔵 FakeController setCalls wiring assertions	fixed	tests/commands.test.ts:346-360 keeps only the already on / already off reply assertions; setCalls is still used legitimately elsewhere (:316,326,334-336,343), so no dead helper.

Findings

🔵 The repair step's comment overstates what that step proves

• Evidence: tests/session-model-override.test.ts:469-471 — "the repair pair differs from the pin, so a surviving runtime would keep calling the old one". It would not: entry.commit fires bridge.onChange, whose candidate loop compares runtime signatures and rebuilds a surviving runtime onto the new pair (src/index.ts:730-747). The requests[1] assertion therefore passes on both the "disposed at reset" and the "survived, then rebuilt by signature" paths.
• Impact: a comment that teaches the next editor a false causal story about the assertion. The guard itself is unaffected — both discriminating assertions (:466, :467) run before the repair, so the buggy version still fails at the delivery assertion.
• Merge class: nit
• Confidence: HIGH
• Fix sketch: reword to "repairing with a different pair proves the new route is served" and drop the claim that this step proves the dispose.

Linked-issue AC

Closes #124 — all six criteria remain met; this increment touches no product behaviour, so the round-2 AC mapping stands unchanged.

✅ Verified

• Read the increment pack (67 diff lines) and both seats opened the product paths it depends on: disposeRuntime/dispose() abort + queue clear (src/advisor-runtime.ts:592-596), drain continuing past a closed gate (:640-650), requests.push before the await (tests/session-model-override.test.ts:104-108), and delivery routing to inject for nit (src/delivery.ts:210).
• Confirmed both assertions are independently meaningful and fire before the repair step; the flush timer (20 ms) cannot mask a late delivery because the release path completes in a microtask.
• Confirmed isolation: fresh adapter per case, reviewer marker cleared in beforeEach, blockNext consumed by the first stream().
• No tests, build or typecheck were run in this review (read-only). The author's earlier run reports typecheck/build green and 649/649.

🗑️ Considered & rejected

• requests.length === 1 does not prove the queue array was emptied: rejected — the product-observable behaviour (backlog never reaches the adapter) is what the assertion covers, and dispose() aborts and clears together.
• flush (20 ms timer) could hide a late delivery: rejected — the released call settles in a microtask; no timer or I/O sits between release and the assertion.
• Self-review bias: mitigated by two independent read-only seats asked to judge the increment adversarially; both revalidated the prior findings from the product code rather than the commit message.

🛠️ Plan to fix

Expand fix plan

1. tests/session-model-override.test.ts:469-471 — reword the repair comment:
   keep "repairing the global pair builds a fresh runtime on the new route" and
   drop the "a surviving runtime would keep calling the old one" claim
   (onChange's signature compare would rebuild it too).
2. pnpm test (no product change; targeted file is enough, full suite before push)