feat(daemon): add voterClientId to permission_resolved (A4)#4539
Conversation
Resolve the originator/voter ambiguity on permission_resolved without breaking wire or SDK consumers (design PR #4511, A4): - Wire: the mediator now emits data.voterClientId alongside the envelope originatorClientId on permission_resolved (same value, the resolving voter). Both are omitted together for no-voter resolutions (timer expiry, session-closed, loopback voter with no clientId). permission_already_ resolved is unchanged (deliberately stamps neither). - SDK: the normalizer exposes an optional voterClientId on the permission.resolved typed event, reading data.voterClientId and falling back to the envelope originatorClientId for daemons predating the field. originatorClientId stays available on the base (no rename, no break). voterClientId is the canonical, unambiguous name; originatorClientId on permission_resolved is kept as a deprecated alias (it means the voter here, unlike the prompt originator on permission_request). Tests: permissionMediator emits voterClientId (+ omits both with no voter); normalizer surfaces voterClientId from data, falls back to originatorClientId, omits it for no-voter. acp-bridge 297, sdk daemon-ui 186 pass.
📋 Review SummaryThis PR implements A4 from the side-channel coordination design (#4511), addressing the long-standing ambiguity where 🔍 General Feedback
🎯 Specific Feedback🔵 LowFile: const voterClientId =
getString(event.data, 'voterClientId') ?? base.originatorClientId;While File: ✅ Highlights
|
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
Add the distinguishing case wenshao asked for: client A submits the prompt (permission_request.originatorClientId === A) while a different client B casts the resolving vote (permission_resolved.voterClientId === B), and assert the two differ — the disambiguation A4 exists to enable. The prior tests only covered the same-client value.
- Concurrent-in-session-/model drift (Critical): add §2.2 post-roundtrip reconciliation — on roundtrip settle the bridge re-reads the agent's actual model and emits a corrective model_switched if divergent (in-session /model bypasses modelChangeQueue, so drop-when-suppressed could otherwise leave the bus on A while the session runs B). - IDE-companion lockstep (Critical): add a one-release dual-emit transition (publish both generic session_update and promoted approval_mode_changed) and enumerate the upstream dispatch sites (daemonIdeConnection.ts / DaemonChannelBridge.ts) that drop unknown types and also need handlers. - Specify the model_switched payload mapping (currentModelId→data.modelId, envelope sessionId→data.sessionId) — without it the SDK validator drops the promoted event and A1 is non-functional. - Require demux observability (structured log: promoted/dropped/suppressed/ generic) at every decision point. - Correct the reviewer's "replay_complete doesn't exist": it shipped in merged #4484 (eventBus.ts:444); A5 phase 2 introduces only session_snapshot. - First-attach no longer synthesizes replay_complete{0} (would widen that event's contract); session_snapshot is self-delimiting on first attach. - Tighten capture-at-emission to a synchronous read+publish block. - Specify the helper-generalization migration model; resolve Q3 (keep the extMethod bypass); add the A4 distinguishing test (done in #4539) to §8.
yiliang114
left a comment
There was a problem hiding this comment.
LGTM! ✅ The implementation is clean, fully additive, and well-tested.
One minor note: The JSDoc on PermissionResolutionRecord.resolverClientId (line 327) says the value is "replayed onto permission_already_resolved", but the actual vote() method intentionally does NOT stamp it there — the inline comment at the duplicate-vote path explains this is a deliberate pre-F3 compatibility choice. Since this PR introduces voterClientId as the canonical name, a reader might reasonably expect it to also appear on permission_already_resolved. Consider updating the JSDoc to match reality.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
🔍 TUI 真实测试验证报告 — PR #4539概要
静态验证
TUI 真实测试 (tmux real-user testing)使用 tmux 模拟真实用户在
关键验证点
结论✅ PASS — 推荐 merge。227 项单元测试全部通过,tmux 真实用户测试 8/8 场景全部通过, |
Squashed feature work from daemon_mode_b_main branch, rebased onto latest main to establish proper merge-base and clean PR diff. Original commits: - perf(core): F2 cleanup PR A — R9/W11/W12/R10 (post-merge follow-ups) (#4411) - refactor(acp-bridge): F1 test split — lift bridge.test.ts (6861 LOC) to acp-bridge (#4445) - fix(core): F2 cleanup PR B — self-heal observability (W133-a + W134) (#4460) - feat(sdk/daemon-ui): unified completeness follow-up to #4328 (#4353) - docs(serve): v0.16-alpha known limits + SDK QWEN_SERVER_TOKEN env fallback (PR 27) (#4473) - docs(deploy): local launch templates for v0.16-alpha (PR 30a) (#4483) - feat(daemon+sdk): cross-client real-time sync completeness (#4484) - feat(serve): add POST /session/:id/recap (#4504) - feat(daemon): add voterClientId to permission_resolved (A4) (#4539) - feat(serve): --allow-origin <pattern> CORS allowlist (T2.4 #4514) (#4527) - feat(daemon): in-session model switch reaches the bus (A1) (#4546) - feat(serve): prompt absolute deadline + SSE writer idle timeout (#4514 T2.9) (#4530) - Feat/daemon react cli (#4380)
* feat(daemon): add voterClientId to permission_resolved (A4) Resolve the originator/voter ambiguity on permission_resolved without breaking wire or SDK consumers (design PR #4511, A4): - Wire: the mediator now emits data.voterClientId alongside the envelope originatorClientId on permission_resolved (same value, the resolving voter). Both are omitted together for no-voter resolutions (timer expiry, session-closed, loopback voter with no clientId). permission_already_ resolved is unchanged (deliberately stamps neither). - SDK: the normalizer exposes an optional voterClientId on the permission.resolved typed event, reading data.voterClientId and falling back to the envelope originatorClientId for daemons predating the field. originatorClientId stays available on the base (no rename, no break). voterClientId is the canonical, unambiguous name; originatorClientId on permission_resolved is kept as a deprecated alias (it means the voter here, unlike the prompt originator on permission_request). Tests: permissionMediator emits voterClientId (+ omits both with no voter); normalizer surfaces voterClientId from data, falls back to originatorClientId, omits it for no-voter. acp-bridge 297, sdk daemon-ui 186 pass. * test(daemon): cover the prompt-originator vs voter distinction (A4) Add the distinguishing case wenshao asked for: client A submits the prompt (permission_request.originatorClientId === A) while a different client B casts the resolving vote (permission_resolved.voterClientId === B), and assert the two differ — the disambiguation A4 exists to enable. The prior tests only covered the same-client value. --------- Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com>
What this does
Implements A4 from the side-channel coordination design (#4511): resolves the long-standing originator/voter ambiguity on
permission_resolved, fully additively.permission_resolved.originatorClientIdhas always carried the voter, whilepermission_request.originatorClientIdcarries the prompt originator — a documented inconsistency that forced consumers to special-case the two events.data.voterClientIdalongside the envelopeoriginatorClientIdonpermission_resolved(same value — the resolving voter). Both are omitted together for no-voter resolutions (timer expiry, session-closed, loopback voter with no client id).permission_already_resolvedis unchanged (it deliberately stamps neither).voterClientIdon thepermission.resolvedtyped event, readingdata.voterClientIdand falling back to the envelopeoriginatorClientIdfor daemons predating the field.originatorClientIdstays available on the base — no rename, no break for existing consumers.voterClientIdis the canonical name going forward;originatorClientIdonpermission_resolvedis kept as a deprecated alias.Benefits
voterClientIdinstead of overloadingoriginatorClientId.lastReplayedEventIdaliasing pattern).Tests
permissionMediator: emits
voterClientIdon a voted resolution; omits both fields for a no-clientId voter. Normalizer: surfacesvoterClientIdfrom data; falls back tooriginatorClientId(old daemon); omits it for a no-voter resolution. acp-bridge 297 + sdk daemon-ui 186 pass.Design
First implementation PR of #4511 (A4 — additive, unblocked). A1/A2/A5 follow once the design is approved (A2 is additionally blocked on #4510's
approvalModeQueue).