Merge-base vs anchor-target mismatch. This PR's base is main, but every code anchor in the doc (bridge.ts, bridgeClient.ts, permissionMediator.ts, acp.d.ts, replay_complete) exists only on daemon_mode_b_main — none are present on main. Co-locating design docs on main is the established convention here (docs/design/ already has a channels/ subdir), so this is defensible, but the "Targets daemon_mode_b_main" line should be reconciled with the actual merge base — or the doc should land on the daemon branch alongside the code it annotates.

Framing drift between doc and PR body. This line states A4 (#4539) and A1 (#4546) are already implemented, but the PR description says "No implementation … before any code lands. Each item ships as its own implementation PR after this is approved." The doc is design-first for A2/A5 but retroactive for A1/A4. Please update the PR body so reviewers aren't reviewing already-merged decisions as if they were still open.

Changelog is ~90 of 398 lines (≈23%). Twelve rounds (v2–v13) of internal-review history will be frozen into the repo on a doc that has never been on main — git history already captures revisions. Several entries narrate corrections to earlier drafts of this same unmerged doc (e.g. v10 "§8 test contradicted v9"; v12 "stale-but-equal scenario was self-contradictory"). Recommend squashing to a short status block (current decisions + open items) before merge and leaving the round-by-round narrative in the PR thread.

Anchor rot. replay_complete does exist (good — the claim is correct), but it's at eventBus.ts:516 on daemon_mode_b_main, not :444 (the :437 neighbourhood is the false-caught-up comment). With ~40 file:line anchors in a long-lived doc this drift is inevitable; lean on the symbol name when symbol and line disagree.

Naming inconsistency — the exact "A1/A2 mix-up" this line claims to prevent. A1 here maps current_model_update → model.changed, while every other section (§2.1 promotion table, §8) maps it to the bus event model_switched; A2 in this same list uses the bus name approval_mode_changed. Either make A1 read model_switched for parity, or state explicitly that model.changed is the SDK-typed reducer event distinct from the model_switched bus event.

[Critical] A2 transport contradicts the shipped code. The doc repeatedly states A2 uses BridgeClient.sessionUpdate() for the demux (§0, §1.1 item 2b, §2.1, §6, §9 table). The shipped code uses extNotification() for both A1 and A2: Session.setMode sends extNotification('qwen/notify/session/mode-update', ...) (Session.ts:2520-2529) and the demux is in BridgeClient.extNotification() via handleInSessionModeUpdate (bridgeClient.ts:480-481). The "two insertion points / different transports" architecture described throughout the document does not exist.

Anyone reading the doc to implement follow-ups will look in the wrong method. The §2.1 demux contract's two-insertion-point framing is incorrect.

— qwen3.7-max via Qwen Code /review

[Critical] Failure-path reconciliation is described but not implemented. This section states reconciliation fires on "both the success and failure paths (a .finally on the roundtrip)" and calls the failure case "exactly when the bus is most likely diverged." The shipped code gates reconciliation on if (succeeded) at all three sites (bridge.ts:1563-1569, :3637-3644, :3866-3875), explicitly skipping reconciliation on failure with action=skipped reason=roundtrip_failed. The zombie roundtrip gap (§2 item 4, v13) is a direct consequence: without failure-path reconciliation, a timed-out model that completes late is never corrected.

Either implement failure-path reconciliation (remove the if (succeeded) guard), or update the doc to state reconciliation fires on success only and acknowledge the zombie roundtrip gap is a consequence of this decision.

— qwen3.7-max via Qwen Code /review

[Critical] Internal contradiction on previousModeId. This line says "Additive wire (current_mode_update reuse + previousModeId + scope)" but §3 point 3 (line 248) explicitly resolves that the agent does NOT send previousModeId — the bridge derives previous from its state cache. An implementor reading this line would add previousModeId to the wire format, contradicting the detailed design.

— qwen3.7-max via Qwen Code /review

[Critical] SessionEntry field declaration is incomplete. This bullet lists three fields (currentModelId, currentApprovalMode, availableCommands) but omits modelPublishGeneration and reconciliationInFlight, both required by §2.2. An implementor following this field list would miss the generation guard and the structural non-recursion guard, causing the §2.2 TOCTOU protection and re-entry protection to be absent.

— qwen3.7-max via Qwen Code /review

[Critical] reconciliationInFlight structural guard has no timeout. The flag is set before the async read and cleared in .finally. If getSessionContextStatus hangs indefinitely (agent stuck, transport half-open), the flag stays true forever and all future reconciliation is skipped (action=skipped-reentrant). The bounded-retry path covers error responses but not hangs. The authoritative correctness backstop is permanently disabled with no recovery.

Add a per-attempt timeout to the reconciliation getSessionContextStatus call (the bridge already uses withTimeout for model roundtrips), or clear reconciliationInFlight with a maximum-hold timer.

— qwen3.7-max via Qwen Code /review

[Suggestion] The zombie roundtrip residual race (§2 item 4, v13) has no corresponding test scenario in §8. The design documents a specific sequence (timeout → reconciliation converges → user switches to B → zombie completes → bus reverts) but §8 omits it entirely. Without a test, there is no regression guard when the ACP cancel fix eventually lands.

Add a §8 bullet asserting the current (broken) behavior as a documented test case, to be updated when the fix lands.

— qwen3.7-max via Qwen Code /review

[Suggestion] availableCommands cache has no reconciliation backstop, no test, and is not yet implemented in SessionEntry. Unlike model/mode (which have authoritative reconciliation), this passive cache field has a single population hook with no staleness detection. §8 has no test for cache population or snapshot inclusion.

Either add availableCommands to the reconciliation read (cheap — the status call already returns it), or add a §8 test and explicitly document undefined as a legal snapshot value.

— qwen3.7-max via Qwen Code /review

[Suggestion] A2 prerequisite text and §7 status markers are stale. approvalModeQueue already exists in the codebase (bridge.ts:262-263), A4 is shipped (#4539), and A1 is shipped (#4546). The "Hard prerequisite" section overstates what the queue alone achieves (reconciliation is the actual divergence closer — see §7 step 3c). §7 step 1 doesn't mark A4 as completed.

Update status markers: add "(Shipped as #4539)" to §7 step 1, update §3 to note the queue has landed, and restate A2's status as unblocked.

— qwen3.7-max via Qwen Code /review

[Critical] Generation-guard rerun behavior: doc says "skip" but implementation re-runs

The doc states reconciliation "skips the corrective if the generation advanced during the read." Verified: the implementation (bridge.ts:1876-1958) does something fundamentally different — it sets rerun = true, skips the stale read, then re-invokes reconcileAfterRoundtrip in the finally block after releasing the in-flight guard. This rerun is load-bearing correctness: without it, the newer change that landed during the read is never reconciled (its own reconciliation bailed on the in-flight guard). The doc contains zero mention of "rerun" or "re-run."

An implementor following this doc would skip on generation mismatch without re-running, silently breaking reconciliation under the exact race condition the guard is designed to handle.

Suggested fix: Add to §2.2: "When the generation guard fires (a newer publish landed during the async read), the current read is discarded (stale) and reconciliation re-runs once after releasing the in-flight guard. The re-run uses the post-publish generation as its new baseline, so it correctly detects further drift. The re-run is bounded to one retry per triggering settle event."

— qwen3.7-max via Qwen Code /review

[Critical] Four specified contract items are NOT implemented — doc doesn't acknowledge the divergence

Verified against the current codebase:

1. reconciliation_failed bus event (bridge.ts:1945): explicitly log-only — "not a known SDK event type." The doc specifies a full payload shape.
2. "1-2 bounded retries with short backoff": implementation does zero retries.
3. availableCommands on SessionEntry (§2.3): field does not exist in the current type.
4. scope: 'session' | 'workspace' discriminator (§3): not included in the publishApprovalModeChanged helper.

Each is a silent correctness gap between spec and reality. Future consumers built against this spec will wait for bus events that never arrive, assume retries that don't happen, and read cache fields that return undefined.

Suggested fix: Add explicit implementation-status markers next to each item — e.g., [deferred — SDK type gap], [pending — A5 follow-up], [deferred — A2 not yet promoted]. This prevents the design doc from becoming a lying specification.

— qwen3.7-max via Qwen Code /review

[Critical] Best-effort dedup (§2.1) specified in doc but missing from implementation

The doc states: "the demux drops a current_model_update whose currentModelId already equals entry.currentModelId." Verified: handleInSessionModelUpdate (bridgeClient.ts:730-771) performs only type validation, entry existence, and roundtrip-suppress checks — no value-equality comparison. Any current_model_update is unconditionally promoted.

Spurious promotions bump modelPublishGeneration without real state change, which can cause a subsequent reconciliation to falsely skip (action=skipped-newer-gen) when a real corrective is needed — amplifying the zombie roundtrip race and any future race depending on generation accuracy.

Suggested fix: Either add the dedup check in the implementation (if (currentModelId === entry.currentModelId) { log dropped; return; }), or update the doc to reflect current behavior and note it as a pending follow-up.

— qwen3.7-max via Qwen Code /review

[Suggestion] approvalModeQueue blocking status is stale

The doc says "approvalModeQueue does not exist in the codebase today" and A2 is "BLOCKED on PR #4510." Verified: approvalModeQueue exists at bridge.ts:268 with full usage at :3771, :3885, and tests at bridge.test.ts:4861. The blocking prerequisite appears to have been satisfied.

Suggested fix: Update §3 item 4 and §7 step 3 to reflect the current state — change "does not exist" / "BLOCKED" to "available since #4510" / "depends on approvalModeQueue."

— qwen3.7-max via Qwen Code /review

[Suggestion] Missing A2 reconciliation test scenarios in §8

§8 has 7 model-axis reconciliation test scenarios (corrective, converged, generation-skip, failure-path trigger, read-error, failure-path converged, non-recursion guard) but zero approval-mode equivalents. §2.2 states "the same reconciliation applies to A2 once approvalModeQueue exists" and §7 step 3c commits to approvalModePublishGeneration + publishApprovalModeChanged helper landing atomically with A2 promotion.

Suggested fix: Add a "Reconciliation (A2, approval-mode)" test group in §8 mirroring the model scenarios: corrective after concurrent in-session /approval-mode during bridge roundtrip, converged, generation-skip, publishApprovalModeChanged generation invariant. Note these are gated on A2 unblocking.

— qwen3.7-max via Qwen Code /review

[Suggestion] Missing gen_read in reconciliation observability format

The [reconcile] log format shows gen_before and gen_after but not the generation value observed when the async read returns. An oncall engineer investigating a skipped-newer-gen decision cannot tell what generation the guard actually observed — they only know "before was N, after was N" with no way to verify the skip was justified vs a bug that skipped incorrectly.

Suggested fix: Add gen_read=<K> to the format. For corrected and converged, gen_read = gen_before. For skipped-newer-gen, gen_read > gen_before, proving the skip was justified.

— qwen3.7-max via Qwen Code /review

[Suggestion] Zombie roundtrip description glosses over dedup interaction

The paragraph says the demux promotes the zombie because it's "not a dup" — but the §2.1 best-effort dedup mechanism IS relevant in the general case. If the zombie arrives before any intervening switch (cache still holds A), dedup catches it (A === A → dropped). The residual race is narrower than described: it only manifests when the zombie arrives AFTER the user's switch to B has completed publishModelSwitched (cache moved to B, so A !== B and dedup doesn't fire).

Suggested fix: Acknowledge that dedup catches the zombie in the no-intervening-switch case, and specify the precise residual window: "zombie arrives after B's settle and publishModelSwitched has moved the cache past A."

— qwen3.7-max via Qwen Code /review

[Critical] Systematically stale line-number anchors + phantom file reference

The anchor convention references httpAcpBridge.ts (described as a "101-LOC re-export shim") — this file does not exist anywhere in the repository. The bridge.ts LOC count ("3923-LOC") is also wrong (actual: 4971).

More broadly, virtually every line-number reference in this document is stale, typically off by hundreds to over a thousand lines. I verified ~30 references against the current codebase and none matched. Examples:

Impact: Implementers following these anchors will land on unrelated code. For a document whose value proposition is mapping coordination surfaces to code locations, this undermines the entire guide.

Suggested fix: Switch to function/method-name anchors (e.g., Session.setModel, BridgeClient.extNotification()) without line numbers. Line numbers are inherently fragile in a living codebase; symbol names survive refactoring.

— qwen3.7-max via Qwen Code /review

[Critical] publishModelSwitched helper signature does not match implementation

The v12 changelog claims the implemented signature is:

publishModelSwitched(entry, modelId, opts?: { originatorClientId?: string })

The actual code at bridge.ts:1899 uses a positional parameter:

const publishModelSwitched = (
  entry: SessionEntry,
  modelId: string,
  originatorClientId: string | undefined,
): void => {

An implementer following the documented signature would pass { originatorClientId: 'x' } where a string | undefined is expected. The truthy object would be used as originatorClientId in the published event, serialized as [object Object] — silent data corruption on the wire.

Suggested fix: Update the documented signature to match: publishModelSwitched(entry, modelId, originatorClientId: string | undefined).

— qwen3.7-max via Qwen Code /review

[Critical] Reconciliation generation-guard: doc says "skip" but code does "skip-and-rerun"

This section describes the generation guard as: reconciliation skips the corrective if the generation advanced during the read.

The actual implementation (bridge.ts:1956-2038) does more: when the generation advances, it sets rerun = true, returns early, then in finally calls reconcileAfterRoundtrip(entry, target) once more. This bounded rerun exists because the newer publish's own reconciliation bailed on the in-flight guard, so without a re-run the latest change would never be reconciled.

This is a key behavioral detail that is entirely absent from the document. Worse, the §8 test plan (line 374) asserts getSessionContextStatus is called exactly once per settle event — but the rerun path calls it twice.

Suggested fix: Document the bounded-rerun mechanism alongside the generation guard. Update the §8 test plan: the generation-skip scenario should expect two getSessionContextStatus calls (the stale read + the rerun), not one.

— qwen3.7-max via Qwen Code /review

[Suggestion] approvalModeQueue already exists — A2 is not blocked on this prerequisite

This section states "approvalModeQueue does not exist in the codebase today" and marks A2 as blocked on PR #4510. In reality, approvalModeQueue is already implemented:

• Defined on SessionEntry at bridge.ts:288
• Initialized at bridge.ts:2059
• Actively used for serialization at bridge.ts:4000 and bridge.ts:4114

The same claim appears in §7 sequencing (line 344: "BLOCKED on PR #4510") and §3 header (line 231: "blocked on #4510"). These should all be updated to reflect that the serialization infrastructure has landed.

— qwen3.7-max via Qwen Code /review