feat(daemon): keep model & approval-mode state consistent across clients sharing a session#4613
Conversation
📋 Review SummaryThis PR implements a bridge side-channel state layer with three key features: state cache with atomic publish helpers (§2.3), post-roundtrip reconciliation (§2.2), A2 mode promotion, and A5 session snapshots. The implementation is well-structured with comprehensive test coverage. The code demonstrates strong attention to race condition prevention and backward compatibility. 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
chiga0
left a comment
There was a problem hiding this comment.
LGTM. Clean solution to the distributed state consistency problem.
Architecture highlights:
- §2.3 centralized publish helpers with atomic cache + generation counter — eliminates the 4 scattered publish sites cleanly
- §2.2
reconcileAfterRoundtripwith generation TOCTOU guard is a solid pattern — prevents clobbering newer changes during reconciliation - A2 suppress guard (
approvalModeRoundtripInFlight) correctly prevents race between bridge-driven and agent-initiated mode changes - A5
withSnapshotgenerator injectssession_snapshotafterreplay_completewith zero extra round-trips
Test coverage: 11 new tests (7 bridge + 4 SDK) covering publish helpers, promotion, suppression, malformed input, snapshot on/off, reducer seeding, and null preservation.
Nit (non-blocking): handleInSessionModeUpdate fallback path (bridgeClient.ts:686) publishes approval_mode_changed with only { sessionId, next } — missing previous and persisted fields compared to the main path. Since the onModePromoted callback is always provided by the bridge factory in production, this is a theoretical inconsistency only. Consider adding previous: entry.currentApprovalMode ?? 'default', persisted: false for consistency.
This review was generated by QoderWork AI
🔬 Local Verification ReportVerified by: @wenshao TypeScript Compilation
Test Results
New Test Cases Verification (PR-specific)Bridge — side-channel state layer (#4511):
SDK — session_snapshot (A5 #4511):
Regression Check
Notes
Verdict✅ Ready to merge. All 428 tests pass, no type errors, no regressions. The implementation is consistent with the design doc and test plan. |
wenshao
left a comment
There was a problem hiding this comment.
Follow-up review of the latest revision. One finding (the applyModelServiceId reconcile guard) is a still-open tail of the resolved bridge.ts:1405 thread — the succeeded gate was added to two of the three reconcile sites but not this one. Two further suggestions below. Lower-confidence items (cold-attach snapshot seeding, workspace-mirror replay on reconcile) are left out as they may be intended scope.
✅ Local verification report — PR #4613Built and tested locally in an isolated git worktree (tmux session), against the merged state (PR + current base), which is what matters for the merge decision. Environment
Build & typecheck — all clean
Test suites — all green, zero regressions
PR-specific tests confirmed executing & passingRan the new describe-blocks in isolation (
The runtime Notes / suggestions for follow-up (none blocking)
VerdictThe change builds, typechecks, and passes all relevant suites cleanly on the merged state, with focused tests covering every new code path on the bridge + SDK side. Code is well-reasoned with thorough inline rationale (TOCTOU guard, non-recursion lock, fresh-attach vs resume snapshot placement, malformed-input validation). Recommend merge, optionally after adding the small CLI-side Verified locally via tmux on the merged worktree; full suites (not just the new tests) were run to check for regressions. |
✅ Follow-up: closed the two test-coverage gaps from the prior reportWrote and ran 4 new tests locally (merged worktree, tmux) that close findings (1) and (2) from my previous comment, then proved they're non-vacuous with a negative control. New tests — all pass (full files: 424 passed, was 420)
Negative control (reverse-audit — passing tests ≠ correct tests)Reverted only 3/4 fail without the PR's source → the tests genuinely pin the new behavior, not a tautology. The 4th is an absence-assertion that is correct on both branches by design. Restored the PR source → back to 4/4 green. NetCombined with the prior run (acp-bridge 343, sdk 705, server 339, Session 81 — all green; typechecks clean; clean merge), the daemon side-channel state layer is now covered end-to-end on all three layers — SDK reducer, bridge wrapper/publish/reconcile, and the CLI server + Session emission boundary. Findings (1) and (2) are addressed; (3) (no real client consumer of A5 yet) and (4) (base-branch lockfile drift) remain informational. Merge recommendation stands. The 4 new tests are small and follow existing patterns in each file — happy to open them as a PR against the branch if useful, or they can be folded into #4613. |
doudouOUC
left a comment
There was a problem hiding this comment.
Code Review
中优先级问题（建议修复）
applyModelServiceId 在 roundtrip 失败时仍无条件触发 reconcile，与设计目标不一致
在 applyModelServiceId（attach 时带 modelServiceId 的路径）的 finally 块中，reconcileAfterRoundtrip 是无条件调用的：
} finally {
entry.modelRoundtripInFlight = false;
void reconcileAfterRoundtrip(entry, 'model');
}而在 HTTP setSessionModel 路径中，你专门引入了 let succeeded = false 标记，只在成功后才触发 reconcile。PR 的测试和注释都明确了设计意图：roundtrip 失败时不应 reconcile，以避免在 model_switch_failed 旁边再出现一条"纠正性 model_switched"。
applyModelServiceId 与这一目标不一致。当代理拒绝模型切换时，客户端可能先收到 model_switch_failed，随后又收到 reconcile 补发的 model_switched，语义混乱。
建议: 仿照 setSessionModel 的模式加入 succeeded 守卫，并补充一条对应的回归测试。
低优先级建议
BridgeClient 构造参数注释已过时
现有注释（如 bridgeClient.test.ts 中）写着 "the constructor takes 7 positional args"，但本 PR 新增了 onModelPromoted 与 onModePromoted 两个可选参数。建议更新为描述"尾部为可选注入回调"的语义性注释，避免硬编码参数数量。
设计亮点
- 状态缓存 + 代次计数器 + 非递归 guard 整体自洽，无递归爆炸或死锁风险
- model / approvalMode 两轴完全独立，互不干扰
- snapshot 注入对新连接（先 yield snapshot）和恢复连接（replay_complete 后注入）两种场景都正确处理
- SDK
isSessionSnapshotData严格校验string | null，防止非字符串写入 state - dual-emit legacy
session_update{current_mode_update}保障 IDE companion 向后兼容 - 测试覆盖了 publish、suppress、malformed 丢弃、reconcile drift/match/failure、snapshot fresh/resume 等关键路径
除 applyModelServiceId 的 reconcile 触发条件需对齐外，整体实现质量很高。
doudouOUC
left a comment
There was a problem hiding this comment.
复核结论（side-channel state consistency #4511）
前 3 轮 review 的 critical 项已逐条核对，当前 revision befb556d 基本都已修复：
- snapshot 对全新 SSE 连接前置注入（
lastEventId===undefined） - reconcile 的
rerun重触发，连续变更不再丢 reconcile - fallback publish 补全
previous/persisted modeMap未知 id 改为throw invalidParams- corrective publish 加结构化 stderr 日志
reconciliation_failed改 stderr（不再发 bus 事件）- SDK snapshot validator 校验 model/mode 的
string | null - 上轮提的
applyModelServiceId失败仍 reconcile —— 现在三个 reconcile 点都有succeeded门控
并发模型（generation 计数 + 非递归 flag + rerun 收敛 + suppress guard）也验证过，逻辑自洽：flag 不泄漏卡死、corrective publish 不自触发死循环、drift 不被永久遗漏。👍
本轮新发现 6 条（见行内评论）。其中 #1（A5 冷 attach snapshot）建议合并前处理，会架空该功能的主用例。
另两个 minor 放这里：
bridge.ts:1853-1855：reconcile catch 注释称 stale 状态「已由 reconnect 的state_resync_required覆盖」，但该事件仅在 reconnect（ring 淘汰 / epoch 重置）时触发，长连接不断线的 client 收不到 —— 代码 OK，注释需修正。bridgeClient.test.ts:20,23：注释仍写「the 7-arg constructor / 7 positional args」，但本 PR 新增了onModelPromoted/onModePromoted，构造器现为 9 个位置参数，注释已过时。
…nc, contract cleanup - bridge: seed snapshot caches (currentModelId/currentApprovalMode) from newSession/loadSession responses so a cold attach reports real state instead of null/null, with KNOWN_APPROVAL_MODES enum backstop - bridge: enum-validate the reconcile approvalMode branch and drop unknown modes with a logged reason - bridge: on a persisted approval-mode change, mirror the new workspace default into every peer SessionEntry cache so their GET status / session_snapshot stop reporting the pre-change mode - bridge/bridgeClient: remove try/catch wrappers around EventBus.publish() per its documented never-throws contract; drop misleading "bus closed" comments - cli/Session: log dropped advisory extNotifications via debugLogger.debug instead of swallowing silently - bridge.test: add failure-gating coverage for applyModelServiceId — a rejected attach-time model apply must not trigger reconcile (no status read) Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
left a comment
There was a problem hiding this comment.
[Critical] [test] Session 测试失败：Session.test.ts:2574
resets AUTO denial counters when a permission-request hook approves a denialTracking fallback prompt — spy setAutoModeDenialState 被调用 0 次。PR 在 Session.ts 中删除了 isDenialFallbackReason 导入和相关回调逻辑（含 setAutoModeDenialState(after) 调用），但未同步移除或更新对应测试用例。测试结果：93 passed / 1 failed。
(Posting as body-level comment because the failing test line is not in the PR diff.)
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
[Critical] [test] Session.test.ts:2574 — denial-counter reset test fails (93 passed, 1 failed)
The test resets AUTO denial counters when a permission-request hook approves a denialTracking fallback prompt asserts setAutoModeDenialState was called, but the spy is invoked 0 times. The PR's changes to Session.ts (adding setMode validation and extNotification) appear to have altered the prompt flow such that the denial-tracking fallback path is no longer exercised by this test scenario. The test needs to be updated to match the new code behavior, or the code change needs investigation.
(Posted as a body-level comment because the failing test line is not in this PR's diff.)
— qwen3.7-max via Qwen Code /review
1ae2bdf to
816bf3d
Compare
wenshao
left a comment
There was a problem hiding this comment.
[Critical] Session.test.ts:2574 — denial counter reset test still fails (93 passed / 1 failed):
FAIL Session > prompt > resets AUTO denial counters when a permission-request hook approves a denialTracking fallback prompt
AssertionError: expected "spy" to be called with arguments: [ { consecutiveBlock: +0, …(3) } ]
(Line 2574 is not in this PR's diff, so this comment is posted here instead of inline.)
Needs Human Review (low confidence):
bridge.ts:3474—setSessionApprovalModeunknown-mode path:succeeded=trueskips publish but the returned HTTP response claims the mode was set to the requested value. Reconcile fires but also drops the unknown mode, leaving a temporary three-way divergence between caller, cache, and agent. Self-healing when the agent returns to a known mode, but silent divergence persists while the agent stays in the unknown mode.events.ts:1762—session_snapshotreducer uses!= nullguard: when snapshot sendsnullforcurrentModelId/currentApprovalMode, the reducer preserves whatever the previous state had rather than clearing it. In practicenullonly occurs when the cache was never populated, so the base state typically has nothing stale to preserve.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
[Critical] Session.test.ts:2574 — denial-counter reset test fails (93 passed / 1 failed).
FAIL Session > resets AUTO denial counters when a permission-request hook approves a denialTracking fallback prompt
AssertionError: expected "spy" to be called with arguments: [ { consecutiveBlock: +0, …(3) } ]
setAutoModeDenialState is never called. This test was also failing in R16.
[Critical] daemonEvents.test.ts — SDK test suite cannot run: the events.ts syntax errors (see inline comment) cause esbuild transform failure, blocking all 0 tests from collecting.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
[Critical] Session.test.ts:2574 — Denial-counter reset test fails (93/1 passed, 1 failed).
Test: "resets AUTO denial counters when a permission-request hook approves a denialTracking fallback prompt"
Error: setAutoModeDenialState spy was never called (0 invocations).
Root cause: Session.ts:2406 (hook-approval path) calls onConfirm(ProceedOnce) but does NOT call this.config.setAutoModeDenialState(recordFallbackApprove(...)). That reset exists only in the requestPermission response path (line 2488-2489). When AUTO mode's denial tracking forces a fallback prompt and a permission-request hook approves it, the denial streak is never reset — permanently downgrading the session to manual approval until the mode is toggled.
Fix: add this.config.setAutoModeDenialState(recordFallbackApprove(this.config.getAutoModeDenialState())) before or after onConfirm in the hook-approval branch at line 2406.
(Posted as a body-level comment because the error is on a line not touched by this PR's diff.)
Deterministic analysis note: 29 tsc errors exist in server.ts and Session.ts but all are on lines NOT in this PR's diff — pre-existing issues on the daemon_mode_b_main base branch.
— qwen3.7-max via Qwen Code /review
- inject session_snapshot up front on fresh SSE connections (not only on resume) - reconcile only after a roundtrip that landed; guard generation TOCTOU with one bounded re-run and log skip/correct/fail transitions - drop unencodable reconciliation_failed bus event in favor of operator log (client path already covered by state_resync_required) - bridgeClient mode fallback emits previous/persisted; dual-emit session_update uses the canonical nested data.update shape - validate modeId at the setMode boundary; reject unknown modes - SDK session_snapshot validator type-checks currentModelId/currentApprovalMode - tests: fresh-connection snapshot + reconciliation drift/match/failure/roundtrip-failure
- applyModelServiceId: gate reconcile behind a `succeeded` flag so a rejected create/attach-time roundtrip can't pair a corrective model_switched with the model_switch_failed it just published; mirrors setSessionModel / setSessionApprovalMode. - in-session mode demux: validate currentModeId against the known approval-mode enum (lockstep with Session.setMode) before it fans out to SSE clients / the SDK reducer. - in-session mode demux: suppress the legacy session_update dual-emit on the exit_plan_mode path via a `legacyFrameSent` flag — sendUpdate already published that frame, so dual-emitting delivered it twice. The setMode path (no sendUpdate) keeps its dual-emit. - reconcile: emit a `reason=roundtrip_failed` skip log on all three failure paths so the skipped reconcile is greppable. - SDK: add session_snapshot to RESYNC_PASSTHROUGH_TYPES so a client that reconnects past ring eviction recovers currentModelId / approvalMode from the full-state frame instead of staying stale until loadSession. - tests: approvalMode reconcile drift + roundtrip-fail, generation rerun, unknown-mode enum drop, dual-emit shape + suppression, setMode extNotification + unknown-modeId rejection. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The existing A5 snapshot tests only seed currentModelId, leaving the publishApprovalModeChanged -> entry.currentApprovalMode -> snapshot pipeline uncovered at the bridge level. Add a test that promotes an in-session mode change before subscribing and asserts the snapshot carries the non-null currentApprovalMode. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…egacyFrameSent - bridgeClient.ts: the A2 comment claimed handleInSessionModeUpdate "mirrors handleInSessionModelUpdate exactly", but it diverges with enum validation and the legacy dual-emit. Reword to state the shared suppression pattern plus the two additions. - Session.test.ts: add coverage for sendCurrentModeUpdateNotification asserting the extNotification carries legacyFrameSent: true, so a regression dropping it (double legacy frame to the IDE companion) is caught. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…nc, contract cleanup - bridge: seed snapshot caches (currentModelId/currentApprovalMode) from newSession/loadSession responses so a cold attach reports real state instead of null/null, with KNOWN_APPROVAL_MODES enum backstop - bridge: enum-validate the reconcile approvalMode branch and drop unknown modes with a logged reason - bridge: on a persisted approval-mode change, mirror the new workspace default into every peer SessionEntry cache so their GET status / session_snapshot stop reporting the pre-change mode - bridge/bridgeClient: remove try/catch wrappers around EventBus.publish() per its documented never-throws contract; drop misleading "bus closed" comments - cli/Session: log dropped advisory extNotifications via debugLogger.debug instead of swallowing silently - bridge.test: add failure-gating coverage for applyModelServiceId — a rejected attach-time model apply must not trigger reconcile (no status read) Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…d cache doc - bridge: document that setSessionModel caches the raw model id and relies on the immediately-following reconcileAfterRoundtrip to correct any raw-vs-canonical drift (the bridge layer lacks access to the CLI's formatAcpModelId which requires authType) - bridge: fix stale reconcile-catch comment that referenced state_resync_required (long-lived SSE connections don't reconnect) - bridgeClient.test: update stale "7-arg constructor" comment to reflect the current 8-arg constructor Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
… assertions - sdk: bump MAX_DAEMON_BROWSER_BUNDLE_BYTES from 100 KiB to 105 KiB to accommodate session_snapshot type/validator/reducer additions (+1.2 KiB) - bridge: remove redundant entry! non-null assertions (already narrowed by if-guard at line 2708) - bridge: document setSessionModel raw-id cache + reconcile correction - bridge.test: add seedSnapshotCaches cold-attach test (newSession response seeds model+mode without intermediate notifications) - bridge.test: add peer cache sync test (persisted mode change updates peer snapshot) - bridge.test: add unknown-mode-drop test (reconcile drops agent- returned modes not in KNOWN_APPROVAL_MODES) Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…test, add resync passthrough test
- bridge.test: rewrite unknown-mode-drop test to trigger approvalMode
reconcile (via setSessionApprovalMode) instead of model reconcile
(via modelServiceId), which never entered the approvalMode branch
— the original was a false positive (F8E2h)
- bridge.test: fix misleading params.mode cast in peer-cache-sync test;
status RPC sends {sessionId} not {mode} — return fixed 'yolo' (F8E2o)
- sdk daemonEvents.test: add session_snapshot passthrough-during-resync
test (RESYNC_PASSTHROUGH_TYPES membership regression guard) (F8SOq)
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…assertion Add statusReads counter to the unknown-mode-drop test so it positively asserts that reconcile actually executed (status RPC was called), not just that no corrective event appeared. Without this, a future refactor disabling reconcileAfterRoundtrip would make the test pass vacuously. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…test, add resync passthrough test - bridge.test: restore missing closing braces for extractErrorCode describe/it blocks (lost during rebase conflict resolution) - sdk build.js: bump MAX_DAEMON_BROWSER_BUNDLE_BYTES from 106 to 107 KiB (actual bundle is 108595 bytes = ~106.1 KiB) Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…model reconcile - bridge: validate setSessionApprovalMode extMethod response against KNOWN_APPROVAL_MODES before publishing/broadcasting; drop with log if agent returns unknown mode (closes trust-boundary gap where handleInSessionModeUpdate and reconcile had guards but this path did not) - bridge: add typeof === 'string' guard to model reconcile branch so a non-string agent response (e.g. number) cannot pollute the cache and break downstream session_snapshot validation - bridge: add writeStderrLine to seedSnapshotCaches drop branches for operator observability parity with reconcile's drop logging Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- bridge: leave succeeded=false when agent returns unknown approval mode — skips pointless reconcile that would re-drop the same value - bridge: restore channel-overlap HAZARD comment on closeSession's channelInfoForEntry call (lost during reaper code removal) Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Three sites where session_snapshot was inserted immediately after session_rewound lost the preceding block's closing delimiter during rebase conflict resolution: type alias (missing >;), asKnownDaemonEvent case (missing : undefined;), and reducer case (missing };). Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…base artifacts) - bridge: remove session-reaper code (closeSessionImpl, startSession- Reaper, stopSessionReaper, constants) inadvertently included during rebase conflict resolution — not part of this PR's scope - events.ts: restore 2 missing delimiters (isSessionBranchedData closing brace, session_rewound type/case closers) lost when session_snapshot was inserted adjacent to session_branched blocks Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
db57e1f to
af76d3b
Compare
… silent success When the agent returns a mode not in KNOWN_APPROVAL_MODES, throw instead of returning a misleading success response. The previous behavior sent 200 OK echoing the requested mode while the cache and SSE bus still showed the old value — a three-way state divergence. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
left a comment
There was a problem hiding this comment.
R19: No high-confidence issues found after full multi-dimensional review (9 agents + reverse audit). The publish helpers, reconciliation logic, snapshot injection, and SDK reducer are all sound. Tests: 247/247 bridge, 87/87 SDK pass. One pre-existing test failure in Session.test.ts (denial-counter reset on hook-approved path) is not introduced by this PR — verified present on base branch. — qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
There was a problem hiding this comment.
Review Summary
整体实现质量很高，并发设计精巧（generation counter + in-flight guard + bounded rerun）。核心逻辑正确，无 critical 问题。以下 inline comments 为建议改进项，不 block 合并。
架构亮点：
- §2.3 publish helpers 原子化了 cache + generation + bus publish，消除了原先 4 处散布的 publish site
- §2.2 reconcile 的
rerun标志在 corrective publish 之前捕获，不会自触发循环 - A5 snapshot 对 fresh connection（先 yield）和 reconnect（replay_complete 后注入）两种场景都正确处理
EventBus.publishnever-throws 合约经代码验证属实，移除 try/catch 包装是正确的简化- legacy dual-emit 的
legacyFrameSentflag 优雅解决了 exit_plan_mode 路径的重复发送
测试覆盖： 新增 ~1580 行测试，覆盖了 publish、suppress、malformed drop、reconcile drift/match/failure/roundtrip-fail/generation-rerun、snapshot fresh/resume、SDK reducer seeding/null-preservation 等关键路径。
| 级别 | 数量 |
|---|---|
| Critical | 0 |
| Important (inline) | 6 |
| Suggestion | 4 |
Suggestions (non-inline)：
seedSnapshotCaches的 3 条防御分支（non-string model、empty model、unknown mode）只测了 happy path，建议补测sendCurrentModeUpdateNotification只测了ProceedAlways分支，RestorePrevious（读 config）和ProceedOnce未覆盖reconcileAfterRoundtripcatch block 对所有异常统一写[reconcile] ... action=failed，建议对非预期编程错误打不同 tag 方便告警- bridge.test.ts 新增 ~1389 行中测试 boilerplate（ChannelFactory 创建）重复度较高，考虑提取更多共享 helper
| const sessionId = params['sessionId']; | ||
| const currentModeId = params['currentModeId']; | ||
| if (typeof sessionId !== 'string' || typeof currentModeId !== 'string') { | ||
| return; |
There was a problem hiding this comment.
[Important] type guard 静默 return 无日志，与同方法内其他 drop 路径（unknown_mode、no_entry、bridge_roundtrip_in_flight）不一致，也与 prompt-suggestion handler 的 malformed 路径不一致。
建议加 writeStderrLine 保持可观测性：
if (typeof sessionId !== 'string' || typeof currentModeId !== 'string') {
writeStderrLine(
`[demux] session=${typeof sessionId === 'string' ? sessionId : '<missing>'} type=current_mode_update action=dropped reason=malformed`,
);
return;
}handleInSessionModelUpdate 有同样的 gap（pre-existing），可一并修复。
| // outside this set is dropped before it fans out to SSE clients / the SDK | ||
| // reducer. Keep the two in lockstep. Exported so the bridge's reconcile and | ||
| // snapshot-seed paths apply the same enum backstop to agent-supplied mode ids. | ||
| export const KNOWN_APPROVAL_MODES: ReadonlySet<string> = new Set([ |
There was a problem hiding this comment.
[Important] 此 Set 与 Session.ts 的 modeMap keys 需手动保持同步，当前值完全一致（plan, default, auto-edit, auto, yolo），但没有编译时保障。注释中的 "Keep the two in lockstep" 容易被忽略。
建议考虑：
- 将 mode string literals 提取为共享 const（放在
@qwen-code/qwen-code-core或bridgeTypes），或 - 至少添加一个测试断言二者相等
| ); | ||
| } finally { | ||
| entry[flagKey] = false; | ||
| if (rerun) void reconcileAfterRoundtrip(entry, target); |
There was a problem hiding this comment.
[Important] rerun 路径无最大深度限制。虽然被 modelChangeQueue/approvalModeQueue 串行化约束（livelock 极不可能），但在持续快速 model switch 场景下可能累积多层 pending status read。
建议加 retries 参数设上限（如 3 次），超出后 log 告警：
const reconcileAfterRoundtrip = async (
entry: SessionEntry,
target: 'model' | 'approvalMode',
retries = 0,
): Promise<void> => {
// ...
finally {
entry[flagKey] = false;
if (rerun) {
if (retries >= 3) {
writeStderrLine(`[reconcile] ... action=rerun_exhausted retries=${retries}`);
} else {
void reconcileAfterRoundtrip(entry, target, retries + 1);
}
}
}
};| if (peer.sessionId === entry.sessionId) { | ||
| continue; | ||
| } | ||
| peer.currentApprovalMode = response.current; |
There was a problem hiding this comment.
[Important] 直接写入 peer cache 但没有 bump peer.approvalModePublishGeneration。虽然不是功能 bug（peer 的 reconcile 会看到 cache == agent truth 然后 no-op），但违反了 generation counter 的语义（"每次 cache 变更都 bump"）。
建议要么也 bump generation 保持一致，要么加注释说明此处刻意不 bump 的原因。
| // the HTTP client thinks the mode changed while the cache and | ||
| // SSE bus still show the old value. | ||
| throw new Error( | ||
| `Agent returned unknown approval mode: ${JSON.stringify(response.current)}`, |
There was a problem hiding this comment.
[Important] 这个新增的 throw 路径（agent 返回 unknown approval mode 时抛异常）缺少对应的测试。现有 drops unknown agent-returned approval mode 测试只覆盖了 reconcile 路径的 unknown mode 丢弃，不是此处的 response 验证。
建议补充测试：FakeAgent 的 approval_mode ext 返回 { current: 'super-yolo' }，断言 setSessionApprovalMode rejects 且无 approval_mode_changed 事件。
| const model = value['currentModelId']; | ||
| const mode = value['currentApprovalMode']; | ||
| return ( | ||
| (model === null || typeof model === 'string') && |
There was a problem hiding this comment.
[Important] validator 只检查 typeof model === 'string'，允许空字符串通过。reducer 的 != null guard 会把 "" 写入 state.currentModelId。虽然 bridge 侧有 non-empty 检查（空字符串到不了 wire），但 SDK 作为最后防线应更严格。
建议改用已有的 isNonEmptyString：
(model === null || isNonEmptyString(model)) &&
(mode === null || isNonEmptyString(mode))
Problem
A single daemon session is shared by multiple clients (chat view, terminal view, IDE companion). Two pieces of side-channel state — the current model and the current approval mode — were not reliably kept in sync across those clients:
exit_plan_mode, user choosing ProceedAlways), only the legacycurrent_mode_updatenotification fired — it was never promoted to a realapproval_mode_changedbroadcast, so chat and terminal views disagreed.Solution
currentModelId/currentApprovalModeand per-axis generation counters onSessionEntry; route every broadcast throughpublishModelSwitched/publishApprovalModeChangedso cache update and publish happen together. Replaces the 4 scattered inline publish sites → exactly one broadcast per change.reconcileAfterRoundtripruns fire-and-forget in thefinallyofapplyModelServiceId/setSessionModel/setSessionApprovalMode, reads the agent's real state, and emits a corrective broadcast if it differs from cache. Guarded by a per-axis non-recursion lock and a generation TOCTOU check so reconciliation never re-triggers itself or clobbers a newer change.Session.setMode/sendCurrentModeUpdateNotificationemitqwen/notify/session/mode-update; the bridge promotes it toapproval_mode_changed(still dual-emitting legacysession_updatefor IDE-companion compat), with an in-flight suppress guard so it doesn't race an explicit switch.GET /session/:id/events?snapshot=1injects a syntheticsession_snapshotframe right afterreplay_complete, carrying the current model + approval mode so a client can rebuild side-channel state with zero extra round-trips.Benefit
Every client sharing a session now sees a consistent current model and approval mode: changes broadcast once, drift is self-corrected, in-session changes propagate to all views, and new/reconnecting clients get current state immediately on attach.
Changed files
packages/acp-bridge/src/bridge.tssubscribeEventspackages/acp-bridge/src/bridgeClient.tshandleInSessionModeUpdate,onModePromotedcallback,approvalModeRoundtripInFlightonBridgeClientSessionEntrypackages/acp-bridge/src/bridgeTypes.tssnapshot?: booleanonsubscribeEventsoptspackages/cli/src/acp-integration/session/Session.tsextNotification('qwen/notify/session/mode-update')insetMode+sendCurrentModeUpdateNotificationpackages/cli/src/serve/server.ts?snapshot=1query param parsingpackages/sdk-typescript/src/daemon/events.tssession_snapshotevent type, data interface, validator, reducerpackages/sdk-typescript/src/daemon/index.tsDaemonSessionSnapshotData/DaemonSessionSnapshotEventTest plan
publishModelSwitchedupdates cache and publishesmodel_switchedpublishApprovalModeChangedpublishesapproval_mode_changedonsetSessionApprovalModecurrent_mode_updatetoapproval_mode_changedwhen no bridge roundtrip in flightcurrent_mode_updatewhile bridge approval-mode roundtrip in flight?snapshot=1yieldssession_snapshotafterreplay_completesession_snapshotwhensnapshotis not setasKnownDaemonEventnarrowssession_snapshotcurrentModelIdandapprovalModefrom snapshotsession_snapshot(missing sessionId)🤖 Generated with Qwen Code