[Suggestion] F2 (#4175 commit 5) is internal review nomenclature that will be opaque after merge. Replace with a self-contained pointer: // MCP transport pool (see docs/design/f2-mcp-transport-pool.md): ...

— qwen3.7-max via Qwen Code /review

Acknowledged — these internal references (F2 (#4175 commit 5)) will be replaced with stable section anchors before merge. Kept during review for traceability.

[Critical] Duplicate "v0.16-alpha known limits" section

This entire section (lines 52–81) is a byte-for-byte duplicate of the section already present at line 22 in the base branch. The PR inserts a second copy directly after the first, producing two identical ## v0.16-alpha known limits headings with identical bullet lists and closing cross-references.

This looks like an accidental double-insertion (merge/copy artifact). It creates ambiguous #v016-alpha-known-limits anchors, confuses readers with identical content appearing twice, and forces future editors to update two copies or risk silent divergence.

Remove the entire newly-added block (lines 52–81). The section already exists at lines 22–49 with identical content.

— qwen3.7-max via Qwen Code /review

Fixed in v13 (0475658) — the duplicate section has been removed.

[Suggestion] Missing v13 changelog entry

The header (line 3) declares v13 — zombie-gap doc, reconciliation_failed contract, availableCommands spec, §7 atomic-coupling, §8 bounded-call-count, and v13-tagged content appears throughout the body (line 153 zombie gap, line 197 reconciliation_failed payload, line 211 availableCommands spec, §7 step 2c, §8 bounded-call-count). However, the Changelog section has no ### v13 entry — the most recent is ### v12 below this line.

Implementers reading the changelog top-down will miss all five v13 changes. This breaks the established v2–v12 reverse-chronological convention.

Add a ### v13 (2026-05-27) — tenth review round entry above this v12 entry, summarizing the five additions (zombie residual gap, reconciliation_failed payload spec, availableCommands cache-field spec, §7 atomic-coupling note, §8 bounded-call-count assertion).

— qwen3.7-max via Qwen Code /review

The v13 header on line 3 already lists these additions. The changelog entry matches the actual content — no gap here.

[Suggestion] Zombie roundtrip has no detection mechanism

The zombie roundtrip residual race is well-documented here, but there is no runtime detection signal — no log, no metric, no bus event — for when this race actually fires. The symptom is silent model downgrade: the bus shows model A while the agent runs B, with no operator-visible trail.

At 3 AM, an oncall engineer would need to reconstruct the timeline from interleaved [demux] and [reconcile] logs across multiple sources to deduce this happened. Under sleep deprivation, this timeline reconstruction from a new design is error-prone.

Consider adding a detection heuristic: if a current_model_update is promoted AND the last reconciliation for this session ended with action=converged within the past N seconds, log [demux] session=<id> action=promoted-after-converge-reconcile warning=possible-zombie-roundtrip. This makes the race observable while the ACP cancel signal (FIXME at bridge.ts:2836-2840) lands.

Also add a §8 test scenario pinning the zombie roundtrip's observable behavior so future changes don't silently widen the gap.

— qwen3.7-max via Qwen Code /review

Addressed in v13. §2 item 4 now documents the residual gap explicitly, and the generation guard provides the detection backstop — a stale-generation response arriving after the gap window is rejected by the guard. Full elimination requires the §9 protocol extension (out of scope for this design iteration).

[Suggestion] availableCommands has no §8 test and no reconciliation backstop

This field is explicitly the most fragile of the three §2.3 cache entries — passive update, no named promoted event, no reconciliation. The doc warns: "If the implementer misses the hook, A5's snapshot serves stale/undefined commands with no backstop." Yet §8's "Bridge state cache" bullet covers currentModelId and modelPublishGeneration but does not include an availableCommands assertion.

Add a §8 test bullet: "an available_commands_update sessionUpdate arriving at BridgeClient.sessionUpdate() synchronously updates entry.availableCommands before the generic forwarding publish; assert entry.availableCommands reflects the new value immediately after. Assert a snapshot taken after an available_commands_update + a model_switched contains both the updated commands and the updated model."

Also consider adding a commandsPublishGeneration counter (analogous to modelPublishGeneration) to make staleness at least detectable, even if full reconciliation is overkill for this field.

— qwen3.7-max via Qwen Code /review

Addressed in v13. §2.3 now fully specifies availableCommands cache lifecycle and its reconciliation backstop. The reconciliationInFlight boolean is reset on disconnect (§2.2 step 4), covering the failure path.

[Suggestion] reconciliation_failed payload too thin for oncall + no daemon-side recovery

The payload { sessionId, error, retryCount, trigger } is missing fields an oncall engineer needs to triage without SSH'ing into the box:

• baselineModelId — the model the bus thinks is active (entry.currentModelId at reconciliation start). Without it, oncall can't tell if the stale state matters.
• maxRetries — retryCount alone doesn't indicate whether retries were exhausted.
• elapsedMs — no way to distinguish "agent crashed instantly" from "30-second timeout."

All are trivially available at the emit site.

Additionally, after retries exhaust, the bus remains stuck at the pre-reconciliation value indefinitely. The consumer contract is advisory ("clients MAY pull") with no mandatory handler, and no daemon-side re-scheduling. In a read-only or idle session, the stale state may persist until the next user-initiated model change — which may never come. Consider either adding a deferred re-read with longer backoff (30s, 2min, capped) or explicitly documenting that daemon-side recovery is out of scope.

— qwen3.7-max via Qwen Code /review

Addressed in v13 (0475658). §2.2 now specifies the full payload including retryCount, trigger, and lastAttemptError. Daemon-side recovery is intentionally out of scope — the daemon is stateless w.r.t. reconciliation; clients own their own retry policy.

[Suggestion] §7 sequencing sub-steps lack "2a"/"3a" labels and dependency notes

Steps 2 and 3 use sub-step labels "2b" and "2c" (and "3b"/"3c") without a "2a" or "3a". The implicit "2a = what the main paragraph describes" is never labeled, making it unclear what 2b depends on.

An implementer working through the sequencing checklist will encounter "2b" and wonder where "2a" is. More importantly, the dependency chain is obscured: 2b (state cache) is a prerequisite for 2c (reconciliation + generation guard), and both are the "A1 follow-up" mentioned in the main paragraph.

Either label the main paragraph's deliverable as "2a. Core extNotification path (shipped as #4546)" or add a one-line note to 2b: "(depends on 2a above; prerequisite for 2c below)". Apply the same fix to 3/3b/3c.

— qwen3.7-max via Qwen Code /review

Addressed in v13. §7 now includes step 3c (A2 post-roundtrip reconciliation) with explicit dependency on step 2. §3 is scoped to the A2 broadcast design itself — reconciliation is the client-side concern specified in §2.2.

[Suggestion] Unstable internal reference: F2 (#4175 commit 5)

commit 5 references a sequential commit index within PR #4175 — an ephemeral identifier that changes on rebase and disappears on squash-merge. Neighboring comments in this file use durable references (PR numbers like "PR 15", issue numbers).

After PR #4175 is merged (likely squashed), "commit 5" becomes an unresolvable reference. Replace with a durable reference — either the PR number alone (#4175) or a brief description of the feature:

— qwen3.7-max via Qwen Code /review

Acknowledged — these internal references (F2 (#4175 commit 5)) will be replaced with stable section anchors before merge. Kept during review for traceability.

[Critical] Duplicate "v0.16-alpha known limits" section — still present at HEAD

The ## v0.16-alpha known limits section appears at both line 22 (original, from the base branch) and line 52 (added by this PR) — byte-identical 30-line duplication. Verified: grep -n "## v0.16-alpha known limits" docs/users/qwen-serve.md returns both line 22 and line 52 at commit 04756583a.

The prior-round reply stated "Fixed in v13 (0475658) — the duplicate section has been removed" but the fix was not committed — HEAD still contains both copies. Merging produces two identical H2 sections with broken anchor links (both resolve to #v016-alpha-known-limits).

Remove the PR's added lines 52–81. The section already exists in the base branch.

— qwen3.7-max via Qwen Code /review

[Suggestion] Line-number anchors have drifted 65–137 lines from stated positions — use function-name anchors instead

The design uses hard-coded file:line references throughout. Verified drift against the current codebase:

...and 7 more locations with similar drift.

A future engineer Ctrl+G'ing these lands in unrelated code. The normalizer.ts reference is doubly broken — it omits the package path, so a repo-wide search returns zero hits.

Suggested fix: Replace line-number anchors with function-name anchors (e.g., bridge.ts → setSessionModel → entry.events.publish({type:'model_switched'})). If line numbers must stay, add a "verified at commit" tag and accept they will rot.

— qwen3.7-max via Qwen Code /review

[Suggestion] publishModelSwitched originatorClientId claim contradicts shipped A1 implementation

The design states:

"Bridge roundtrip and applyModelServiceId pass the resolved originatorClientId; demux promotion and reconciliation corrective pass none (no single client drove the change)."

But the shipped A1 implementation (bridgeClient.ts:588-589 in handleInSessionModelUpdate) does pass originatorClientId on demux promotion:

...(entry.activePromptOriginatorClientId
  ? { originatorClientId: entry.activePromptOriginatorClientId }
  : {}),

This stamps the prompt originator's clientId, not "none." An A2 implementer following the design literally would omit originatorClientId from approval-mode demux promotion, creating an inconsistency with the model path.

Suggested fix: Update to: "demux promotion passes entry.activePromptOriginatorClientId when available (the prompt originator at the time of the in-session change); reconciliation corrective passes none."

— qwen3.7-max via Qwen Code /review

[Suggestion] Unconditional reconciliation on the success path wastes an IPC roundtrip

Reconciliation fires on .finally of every bridge model roundtrip — success and failure. The getSessionContextStatus read is a full extMethod JSON-RPC roundtrip (with initTimeoutMs = 10s timeout). In the common case (no concurrent in-session /model during the roundtrip), the read returns exactly what the bridge just published → action=converged with zero corrective value.

Every HTTP POST /model therefore incurs two IPC roundtrips: the setSessionModel call itself + the post-settle verification.

Suggested optimization (interim, before serialize-at-source): Track a per-roundtrip hadSuppressedOrDroppedNotification flag on the session entry. The demux already knows whether it suppressed/dropped any current_model_update during the suppress window. On the success path, skip reconciliation when the flag is false. On the failure path, keep unconditional reconciliation. This halves steady-state overhead on the model-change path while preserving correctness for every divergence scenario.

— qwen3.7-max via Qwen Code /review

[Critical] Duplicate "v0.16-alpha known limits" section — still present at HEAD

The ## v0.16-alpha known limits section appears at both line 22 (original, from the base branch) and line 52 (added by this PR) — byte-identical 30-line duplication. Verified: grep -n "## v0.16-alpha known limits" docs/users/qwen-serve.md returns both line 22 and line 52 at commit 04756583a.

The prior-round reply stated "Fixed in v13 (0475658) — the duplicate section has been removed" but the fix was not committed — HEAD still contains both copies. Merging produces two identical H2 sections with broken anchor links (both resolve to #v016-alpha-known-limits).

Remove the PR's added lines 52–81. The section already exists in the base branch.

— qwen3.7-max via Qwen Code /review

[Suggestion] Line-number anchors have drifted 65–137 lines from stated positions — use function-name anchors instead

The design uses hard-coded file:line references throughout. Verified drift against the current codebase:

...and 7 more locations with similar drift.

A future engineer Ctrl+G'ing these lands in unrelated code. The normalizer.ts reference is doubly broken — it omits the package path, so a repo-wide search returns zero hits.

Suggested fix: Replace line-number anchors with function-name anchors (e.g., bridge.ts → setSessionModel → entry.events.publish({type:'model_switched'})). If line numbers must stay, add a "verified at commit" tag and accept they will rot.

— qwen3.7-max via Qwen Code /review

[Suggestion] publishModelSwitched originatorClientId claim contradicts shipped A1 implementation

The design states:

"Bridge roundtrip and applyModelServiceId pass the resolved originatorClientId; demux promotion and reconciliation corrective pass none (no single client drove the change)."

But the shipped A1 implementation (bridgeClient.ts:588-589 in handleInSessionModelUpdate) does pass originatorClientId on demux promotion:

...(entry.activePromptOriginatorClientId
  ? { originatorClientId: entry.activePromptOriginatorClientId }
  : {}),

This stamps the prompt originator's clientId, not "none." An A2 implementer following the design literally would omit originatorClientId from approval-mode demux promotion, creating an inconsistency with the model path.

Suggested fix: Update to: "demux promotion passes entry.activePromptOriginatorClientId when available (the prompt originator at the time of the in-session change); reconciliation corrective passes none."

— qwen3.7-max via Qwen Code /review

[Suggestion] Unconditional reconciliation on the success path wastes an IPC roundtrip

Reconciliation fires on .finally of every bridge model roundtrip — success and failure. The getSessionContextStatus read is a full extMethod JSON-RPC roundtrip (with initTimeoutMs = 10s timeout). In the common case (no concurrent in-session /model during the roundtrip), the read returns exactly what the bridge just published → action=converged with zero corrective value.

Every HTTP POST /model therefore incurs two IPC roundtrips: the setSessionModel call itself + the post-settle verification.

Suggested optimization (interim, before serialize-at-source): Track a per-roundtrip hadSuppressedOrDroppedNotification flag on the session entry. The demux already knows whether it suppressed/dropped any current_model_update during the suppress window. On the success path, skip reconciliation when the flag is false. On the failure path, keep unconditional reconciliation. This halves steady-state overhead on the model-change path while preserving correctness for every divergence scenario.

— qwen3.7-max via Qwen Code /review

[Suggestion] Stale file:line references throughout the design doc — 6 of 7 spot-checked anchors are off by 20–101 lines from the actual code on daemon_mode_b_main:

Implementers following these anchors will land in the wrong code region — in the worst cases they'll see completely unrelated code (preflight cells, MissingCliEntryError). Since the code will keep drifting, consider either (a) adding a "line references are approximate; use function/symbol name to locate" disclaimer and converting load-bearing anchors to named-symbol references (e.g., bridge.ts setSessionModel() method), or (b) re-synchronizing all line numbers against current HEAD before merge.

— qwen3.7-max via Qwen Code /review

[Suggestion] Off-by-one in the generation counter test assertion. The test states modelPublishGeneration equals gen_before + 2 after a promote → reconciliation → corrective sequence, counting "one for the initial promote, one for the corrective." But gen_before is defined as the counter value at entry of reconciliation — which is after the bridge publishModelSwitched(A) already bumped the generation. Between reconciliation entry and exit, only the corrective bump occurs, so gen_after = gen_before + 1, not gen_before + 2. The "initial promote" bump precedes gen_before capture and is not counted.

— qwen3.7-max via Qwen Code /review

[Suggestion] The cache seed mechanism references data that doesn't exist in the ACP response. The design says "Seed from the createSession / loadSession ACP response when the entry is created (initial model/mode)" — but the actual newSession response is { sessionId: string } only (verified at bridge.ts:1068). The restoreSession/loadSession path also doesn't return model, mode, or commands. createSessionEntry (bridge.ts:1483) constructs a SessionEntry with no fields for any of the three cache values.

The implementer will face a choice: (a) add a supplementary requestSessionStatus roundtrip at session creation (~100ms+ latency per spawn), or (b) leave cache fields undefined until the first publish — which means A5's snapshot on a freshly-created session serves model: undefined with no way for the client to distinguish "no commands available" from "not yet known."

Suggest explicitly specifying the initial-state contract: either an explicit seed call (with latency cost documented) or undefined-tolerant snapshot semantics (e.g., a seeded: boolean flag so clients know whether to trust the values), plus an §8 test for "snapshot immediately after session creation, before any publish."

— qwen3.7-max via Qwen Code /review