📋 Review Summary

This PR implements A1 from the side-channel coordination design (#4511), enabling in-session model switches (via /model slash command or plan-mode model switch) to reach attached clients by emitting current_model_update notifications over the extNotification side-channel. The implementation is well-designed, with proper suppression logic to prevent double-publishing during bridge-driven model roundtrips, and comprehensive test coverage.

🔍 General Feedback

• Clean architectural decision: Using the existing extNotification side-channel (same as MCP-budget events) rather than modifying the ACP SessionUpdate spec is the right call—minimizes spec churn and keeps changes localized.
• Good separation of concerns: Agent emits notification, bridge demuxes and promotes/suppresses based on roundtrip state—each component has a clear responsibility.
• Observability is solid: Structured logging with promoted / suppressed / dropped actions makes the demux behavior observable and debuggable.
• Fire-and-forget pattern: Matching the MCP-budget extNotification pattern (notification failures don't fail the switch) is appropriate for this advisory use case.
• Test coverage is comprehensive: Both promotion and suppression scenarios are covered, including the race condition where a bridge roundtrip is in flight.

🎯 Specific Feedback

🟢 Medium

• packages/acp-bridge/src/bridgeClient.ts:567 — The handleInSessionModelUpdate method catches and silently ignores bus errors (catch { /* bus closed */ }). While this matches the fire-and-forget intent, consider adding a debug-level log (even if suppressed in production) to aid debugging during development:

  } catch (err) {
    // Bus closed — advisory notification, no action needed.
    if (process.env.DEBUG) {
      writeStderrLine(`[demux] session=${sessionId} type=current_model_update action=failed reason=bus_closed`);
    }
  }

• packages/cli/src/acp-integration/session/Session.ts:1601 — The previousModelId capture is done before the switch, which is correct. However, the conditional spread ...(previousModelId && previousModelId !== parsed.modelId ? { previousModelId } : {}) could be simplified since previousModelId is already truthy from getModel(). Consider always including it for consistency:

  ...(previousModelId ? { previousModelId } : {}),

  This ensures the field is present even if the model somehow switches to the same value (edge case, but makes the notification schema more predictable).

🔵 Low

• packages/acp-bridge/src/bridge.ts:215 — The JSDoc comment for modelRoundtripInFlight is excellent and clear. Consider adding a similar inline comment at the two finally blocks (lines 1215 and 2884) where the flag is reset, just to make it obvious why the flag is being cleared in both success and failure paths:

  } finally {
    // A1: clear the roundtrip flag so subsequent in-session notifications are promoted.
    entry.modelRoundtripInFlight = false;
  }

• packages/acp-bridge/src/bridgeClient.ts:558 — The comment mentions "field mapping: currentModelId → data.modelId", but the actual mapping in the published event uses modelId: currentModelId. This is correct, but the comment could be slightly clearer:

  * to a `model_switched` bus event (maps `currentModelId` from the notification
  * to `modelId` in the event data).

• packages/cli/src/acp-integration/session/Session.test.ts:473 — The test assertion uses expect.objectContaining which is good, but consider also asserting the previousModelId field is present in the notification to ensure the full payload shape is verified:

  expect(mockClient.extNotification).toHaveBeenCalledWith(
    'qwen/notify/session/model-update',
    expect.objectContaining({
      v: 1,
      sessionId: 'test-session-id',
      currentModelId: 'qwen3-coder-plus',
      previousModelId: expect.any(String), // Verify previousModelId is included
    }),
  );

• packages/acp-bridge/src/bridge.test.ts:6027 — The test comment says "Hang the agent's unstable_setSessionModel" but the proxy implementation returns a promise that never resolves until releaseModel is called. This is a clever pattern, but consider naming the variable resolveModel instead of releaseModel to be more explicit about what the function does (it resolves the hanging promise).

✅ Highlights

• Excellent design doc reference: The PR description and inline comments consistently reference docs(design): daemon side-channel coordination (A1/A2/A4/A5) #4511 and the A1 initiative, making it easy to understand the broader context.
• Smart suppression mechanism: The modelRoundtripInFlight flag is a simple but effective solution to the double-publish problem—cleaner than complex deduplication logic.
• Comprehensive test scenarios: The suppression test (bridge.test.ts:6027-6107) is particularly well-crafted, using a hanging promise to simulate the race condition and verifying only the authoritative model_switched is published.
• Consistent patterns: Fire-and-forget notification pattern, structured logging format, and queue-based model change serialization all follow established patterns from the MCP-budget implementation.
• Clear commit scope: The PR explicitly scopes to "core A1 path + suppress + observability + tests" and calls out the follow-up work (post-roundtrip reconciliation and timeout-race staleness check), setting clear expectations.

⚠️ Cross-PR Conflict Detected — Large Conflict Group (5 PRs)

This PR is part of a conflict group with PR #4476, PR #4507, PR #4520, and PR #4545.

Key shared functions:

• publish / push — packages/acp-bridge/src/eventBus.ts
• setModel — packages/cli/src/acp-integration/session/Session.ts
• createInMemoryChannel — packages/acp-bridge/src/inMemoryChannel.ts
• shutdown — multiple files (DualOutputBridge.ts, agent-interactive.ts, config.ts, etc.)
• expectCompressBeforeSend — packages/cli/src/acp-integration/session/Session.test.ts

Recommendation: This PR adds in-session model switch which shares event bus and session management code with other PRs. Coordinate with the group before merging.

This conflict was detected automatically by CodeScope skills — structural analysis of call graphs and file modifications.

⚠️ Cross-PR Conflict Detected — Large Conflict Group (5 PRs)

This PR is part of a conflict group with PR #4476, PR #4507, PR #4520, and PR #4545.

Key shared functions:

• publish / push — packages/acp-bridge/src/eventBus.ts
• setModel — packages/cli/src/acp-integration/session/Session.ts
• createInMemoryChannel — packages/acp-bridge/src/inMemoryChannel.ts
• shutdown — multiple files (DualOutputBridge.ts, agent-interactive.ts, config.ts, etc.)
• expectCompressBeforeSend — packages/cli/src/acp-integration/session/Session.test.ts

Recommendation: This PR adds in-session model switch which shares event bus and session management code with other PRs. Coordinate with the group before merging.

This conflict was detected automatically by CodeScope skills — structural analysis of call graphs and file modifications.

Note: this PR merged before I pushed the applyModelServiceId publish-guard fix for your suggestion above, so it landed as a small follow-up PR instead.