Skip to content

feat(daemon): add voterClientId to permission_resolved (A4)#4539

Merged
wenshao merged 2 commits into
daemon_mode_b_mainfrom
feat/daemon-permission-voter-clientid
May 26, 2026
Merged

feat(daemon): add voterClientId to permission_resolved (A4)#4539
wenshao merged 2 commits into
daemon_mode_b_mainfrom
feat/daemon-permission-voter-clientid

Conversation

@chiga0

@chiga0 chiga0 commented May 26, 2026

Copy link
Copy Markdown
Collaborator

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.originatorClientId has always carried the voter, while permission_request.originatorClientId carries the prompt originator — a documented inconsistency that forced consumers to special-case the two events.

  • 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 client id). permission_already_resolved is unchanged (it 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 for existing consumers.

voterClientId is the canonical name going forward; originatorClientId on permission_resolved is kept as a deprecated alias.

Benefits

  • Consumers get an unambiguous voterClientId instead of overloading originatorClientId.
  • Fully additive on both wire and SDK — no consumer breaks (mirrors the accepted lastReplayedEventId aliasing pattern).

Tests

permissionMediator: emits voterClientId on a voted resolution; omits both fields for a no-clientId voter. Normalizer: surfaces voterClientId from data; falls back to originatorClientId (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).

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.
@github-actions

Copy link
Copy Markdown
Contributor

📋 Review Summary

This PR implements A4 from the side-channel coordination design (#4511), addressing the long-standing ambiguity where permission_resolved.originatorClientId carried the voter instead of the prompt originator. The implementation is fully additive on both wire and SDK sides, introducing voterClientId as the canonical name while preserving originatorClientId as a deprecated alias for backward compatibility. The changes are well-tested and follow the established pattern from previous similar changes (like lastReplayedEventId aliasing).

🔍 General Feedback

  • Clean, minimal implementation: The changes are surgical and focused, touching only the necessary files without unnecessary refactoring
  • Excellent documentation: Comments clearly explain the A4 design decision, the relationship between voterClientId and originatorClientId, and when each field is omitted
  • Backward compatibility preserved: The approach mirrors the accepted lastReplayedEventId aliasing pattern, ensuring no breaking changes for existing consumers
  • Test coverage is comprehensive: Tests cover all three key scenarios (voted resolution with clientId, no-clientId voter, and old daemon fallback)
  • Consistent pattern across packages: Both acp-bridge and sdk-typescript follow the same additive pattern

🎯 Specific Feedback

🔵 Low

File: packages/sdk-typescript/src/daemon/ui/normalizer.ts:646 - Consider adding a comment explaining why the fallback uses ?? instead of || (to handle empty string edge case if originatorClientId could be empty string):

const voterClientId =
  getString(event.data, 'voterClientId') ?? base.originatorClientId;

While ?? is the correct choice here (to distinguish undefined from empty string), a brief comment would help future maintainers understand this is intentional.

File: packages/sdk-typescript/src/daemon/ui/types.ts:169-179 - The JSDoc for voterClientId is excellent but could benefit from an @deprecated tag on the related originatorClientId field (if it exists on this interface) to make the deprecation more discoverable through IDE tooling.

✅ Highlights

  • Smart design decision: Introducing voterClientId as the canonical name while keeping originatorClientId as a deprecated alias is the right balance between clarity and backward compatibility
  • Comprehensive test coverage: The three test cases in daemonUi.test.ts cover all important scenarios:
    • Modern daemon with voterClientId in data
    • Old daemon without voterClientId (fallback behavior)
    • No-voter resolutions (both fields absent)
  • Clear wire protocol documentation: The comments in permissionMediator.ts (lines 1129-1144) excellently document why both fields exist, when they're omitted, and the semantic difference between permission_request and permission_resolved
  • Consistent omission logic: Both fields are omitted together for no-voter resolutions (timer expiry, session-closed, loopback voter with no clientId), maintaining wire shape consistency

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
chiga0 pushed a commit that referenced this pull request May 26, 2026
- 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 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@wenshao

wenshao commented May 26, 2026

Copy link
Copy Markdown
Collaborator

🔍 TUI 真实测试验证报告 — PR #4539

概要

项目 详情
PR feat(daemon): add voterClientId to permission_resolved (A4)
测试日期 2026-05-26 17:48
测试环境 macOS (darwin), tmux 200×50, npm run dev -- --approval-mode default
结果 PASS — 可以 merge

静态验证

检查项 结果
npm run build ✅ 0 errors
npm run typecheck ✅ 5/5 packages pass
npm run bundle ✅ 成功
permissionMediator.test.ts ✅ 40/40 tests pass
daemonUi.test.ts ✅ 187/187 tests pass

TUI 真实测试 (tmux real-user testing)

使用 tmux 模拟真实用户在 --approval-mode default 下的完整交互流程:

# 测试场景 结果 关键观察
01 TUI 启动 Qwen Code vdev 正常启动,输入框就绪
02 ReadFile (低风险) 自动批准,模型正确返回 .nvmrc 内容
03 Shell echo (低风险) 自动批准,输出 hello-from-pr4539-test
04 WriteFile (需审批) 权限对话框正确出现 (3 选项),permission_resolved + voterClientId 路径被行使
05 WriteFile 审批通过 文件写入成功,模型自动验证文件存在
06 Shell rm (高风险) 完整权限对话框出现 (4 选项: once/project/user/deny),voterClientId 核心路径
07 rm 审批通过 文件删除成功,模型自动验证已删除
08 /model 斜杠命令 模型选择对话框正常渲染,Escape 关闭无异常

关键验证点

  • 权限调解端到端正常: 低风险操作自动放行 (read_file, echo),高风险操作正确弹出权限对话框 (write_file, rm)
  • 权限对话框 UI 完整: WriteFile 显示 3 选项 (once/always/deny),Shell rm 显示 4 选项 (once/project/user/deny)
  • voterClientId 路径被行使: 每次用户点击 "Allow" 都会触发 permission_resolved 事件,此 PR 新增的 voterClientId 字段在该流程中被正确传递
  • 向后兼容: originatorClientId 作为 deprecated alias 保留,现有消费者不受影响
  • TUI 无回归: 启动、对话、工具调用、权限对话框、斜杠命令均正常

结论

PASS — 推荐 merge。227 项单元测试全部通过,tmux 真实用户测试 8/8 场景全部通过,voterClientId 的权限调解端到端流程正常工作,无 TUI 回归。

@wenshao wenshao merged commit 33f7623 into daemon_mode_b_main May 26, 2026
1 check passed
@wenshao wenshao mentioned this pull request May 26, 2026
doudouOUC added a commit that referenced this pull request May 27, 2026
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)
doudouOUC pushed a commit that referenced this pull request May 27, 2026
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants