fix(daemon): resolve ACP permission votes across connections - #5912
Conversation
|
Thanks for the PR! (Re-triage requested by @wenshao) Template looks good ✓ — all required sections present, bilingual, concrete reviewer test plan, honest Tested-on matrix. On direction: this fixes a real, independently reproducible bug — a permission vote posted on a different On approach: scope is tight — 5 files, +1812/-29, all in
Test count grew to 233 (26 permission-specific tests, up from 222 in the prior triage). Every error branch is covered: missing requestId, invalid outcome shapes, sessionId mismatch, unowned-connection rejection, bridge rejection with retry verification, bridge error mapping (400/403/404/409/500/501), cancel fallback, cross-connection malformed result, and the Moving on to code review and testing. 🔍 中文说明感谢贡献！（应 @wenshao 要求重新 triage） 模板完整 ✓ —— 所有章节齐全、双语、reviewer test plan 具体可执行、Tested-on 矩阵诚实。 方向：这修的是一个真实存在、可以独立复现的 bug —— 使用不同 方案：范围很紧 —— 5 个文件，+1812/-29，全部在
测试数量增长到 233（26 个 permission 相关测试，上次 triage 时 222）。覆盖了每一个错误分支。 进入代码审查和测试 🔍 — Qwen Code · qwen3.7-max |
2a. Code reviewIndependent proposal (written before reading the diff):
Comparing to the diff: the PR's solution matches this proposal and exceeds it in several areas:
Reuse-before-new-code: Critical blockers: none. The authorization model is preserved end-to-end across all paths:
Clear AGENTS.md violations: none. All new methods are small, well-named, used in ≥2 call sites. Comments explain non-obvious why (multi-client attach semantics, sibling entry safety) — not narration. Test coverage: 26 permission-specific tests (up from 15 in the prior triage). New coverage includes: 2b. TestingDaemon-only ACP-over-HTTP bugfix with no user-visible TUI change — the test suite exercises the real HTTP transport, connection registry, and dispatcher through integration-style vitest tests. Ran the targeted suite in tmux and captured output. Full suite (no filter): Other verification:
中文说明2a. 代码审查独立方案（在看 diff 之前写的）：
和 diff 对比：PR 完全一致并在多处超越：
重用检查： Critical blockers：无。授权模型端到端保留。 测试覆盖：26 个 permission 相关测试（上次 triage 时 15 个）。 2b. 测试纯 daemon 端 ACP-over-HTTP bugfix，无 TUI 变化。在 tmux 里跑了定向测试套件并 capture 了输出（见上方英文部分代码块）。 26 个命中 filter 的测试全绿。全量跑： 其它验证：
— Qwen Code · qwen3.7-max |
|
Reflection: This PR has continued to mature since the last triage. The author addressed every round of review feedback across 15 commits, and the result is notably stronger than what was already approved:
Going back to my independent proposal from Stage 2a: the PR matches it exactly and then exceeds it — the O(1) fast path in After seeing the updated code and test results:
Author: @chiga0 — thanks for the careful, iterative work. The progressive strengthening across 15 commits — from the core fix through error mapping parity, field whitelisting, and the pending-entry cleanup semantics — is exactly the kind of PR evolution that makes review productive. 中文说明反思： 这个 PR 自上次 triage 以来继续成熟。作者在 15 个 commit 中回应了每一轮 review 反馈，结果比之前已经 approve 的版本更强：
回头对照 Stage 2a 的独立方案：PR 完全一致并超越 ——
作者：@chiga0 —— 感谢这份渐进式的细致工作。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Code Review
Reviewed the cross-connection permission resolution fix across connection-registry.ts, dispatch.ts, index.ts, and the accompanying tests.
The connection-qualified ID format (_qwen_perm_${connectionId}_N) correctly prevents collisions when the same numeric counter is used across connections. The ownership check in findPendingPermission (requiring the voting connection to own the same session) is the right guard for cross-connection votes — it prevents a different session's connection from resolving a permission it shouldn't see. The initialization order fix in index.ts (registry before dispatcher) correctly resolves the prior circular dependency.
No high-confidence issues found. The 180-test pass count and focused ESLint output reported in the PR give good coverage confidence.
Generated by Claude Code
Resolve the review feedback on PR QwenLM#5912: - dispatch.ts session/permission: add server-side stderr logging to every failure mode (missing requestId, no pending entry, ownership failure, bridge rejection) so a stuck permission prompt is debuggable, matching the legacy resolveClientResponse path. - On bridge rejection (accepted === false), stop deleting the pending entry and stop reusing the "no pending" error. Keep the entry until teardown (as the legacy path does) and return a distinct 409 "vote not accepted" error, so the two states aren't conflated and a retry on another connection can still land. - connection-registry.ts: extract findPendingPermissionEntry shared by findPendingPermission and deletePendingPermission so the matching predicate lives in one place; delete now stops at the first (globally unique) match. - index.ts: the abandonPending callback logs-and-returns-false before the dispatcher is initialized instead of throwing through the teardown path, matching the detachClient callback's defensive posture. - Tests: cover the previously-untested handler branches (missing requestId, invalid outcome shapes, cancelled outcome, bridge rejection + sessionId inference + entry retention) and assert the connection-qualified id format and the undefined-sessionId lookup branch. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Sync 66 commits from main to pick up the check:serve-fast-path-bundle script the CI workflow now requires; resolves the failing 'Check serve fast-path bundle closure' step (Missing script). Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Overall the PR is well-scoped and the cross-connection permission resolution design is sound — the bridge remains the single source of truth, the ownership gates are consistent across both voting paths, and the connection-qualified _qwen_perm_<connectionId>_<N> id format is pinned by the unit test. Focused integration tests cover the happy path, the bridge-rejection/409 retry, and the cross-connection ownership guard on the legacy path. The one remaining concern below is a silent-drop landmine in the newly-modified legacy resolveClientResponse path that the new session/permission handler already avoids.
…branches Address the second ci-bot review round on PR QwenLM#5912: - dispatch.ts resolveClientResponse: the cross-connection ownership guard dropped a vote silently. Add a writeStderrLine so a vote rejected on the legacy path leaves the same grep-friendly operator signal the session/permission handler already emits — otherwise the agent's prompt stays blocked until teardown with no log to correlate. - transport.test.ts: add end-to-end coverage for two previously-untested handler branches — the no-pending 404 response (requestId misses the registry with no sessionId) and the unowned-session rejection (a connection voting on a session it does not own). Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Address the third ci-bot review round on PR QwenLM#5912 (all non-blocking suggestions): - connection-registry.ts: collapse the redundant private findPendingPermissionEntry pass-through into the public findPendingPermission, and align deletePendingPermission to the same (requestId, sessionId) argument order so the two can never be called with swapped string args (a swap would silently match nothing and leak the entry until teardown, with no type error). - dispatch.ts session/permission: look the pending entry up by the globally-unique requestId alone and treat the entry's own session as authoritative; when the client supplies a sessionId that does not match, reject with an explicit 409 instead of routing requireOwned and the bridge vote at the wrong session (which left the real entry to leak until teardown). - Tests: add the sessionId-mismatch rejection case and update call sites for the new argument order. Out of scope and deferred: making the dispatcher's registry a required constructor parameter (and the dependent dropResolvedPermission cleanup) — that changes the constructor contract beyond this fix. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Address wenshao's review on PR QwenLM#5912: findPendingPermission matches on bridgeRequestId (a per-request randomUUID), not the connection-qualified conn.pending map key. Under multi-client attach a permission_request reaches every co-owning connection, each minting its own entry that shares the same bridgeRequestId — so more than one entry can match and the prior "globally unique, at most one match" comment was wrong. - connection-registry.ts: correct the findPendingPermission doc to attribute uniqueness to the map key (not matched here) and note co-owning connections can share a bridgeRequestId, so callers needing a specific entry must act on the conn/map-key they already hold. - dispatch.ts dropResolvedPermission: delete the resolved entry by its exact conn/map-key instead of re-matching by bridgeRequestId, which under multi-attach could delete a sibling connection's entry and orphan the one just resolved. Drops the now-unused req parameter. deletePendingPermission stays for the session/permission handler, where the lookup and delete consistently target the same first match. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Address wenshao's two [Critical] review findings on PR QwenLM#5912: - session/permission no longer falls through to the bridge when the registry misses. In the scoped route a sessionId is always supplied, so a stale/ unknown requestId previously routed to the caller's session, got a bridge `false`, and was reported as a thrown 409 — diverging from the established `404 -> false` contract of DaemonClient.respondToSessionPermission() and the REST route. Now a registry miss returns 404; 409 is reserved for a present entry the bridge still rejects. - Wrap the bridge vote and map permission-specific throws like REST's sendPermissionVoteError: InvalidPermissionOptionError -> INVALID_PARAMS with httpStatus 400 + invalid_option_id, PermissionForbiddenError -> httpStatus 403 + permission_forbidden (with requestId/sessionId/reason). Previously these fell through the outer catch into a generic httpStatus-less internal error, so SDK callers saw 500s for normal permission outcomes. Import the error classes from acp-session-bridge (as REST does) so instanceof matches the class the bridge throws. - Tests: cover the 404-on-miss-with-sessionId regression and the 400/403 mappings. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Address the fourth ci-bot review round on PR QwenLM#5912 (non-blocking suggestions): - session/permission success path now drops the resolved entry through the shared dropResolvedPermission helper using the conn/map-key pendingRef already carries, instead of re-matching by requestId. Unifies the two delete sites and keeps the deletion precise. - parsePermissionResponse forwards only the bridge-contract fields (outcome plus the ACP-reserved _meta passthrough) rather than copying every remaining client key, removing a needless client-controlled surface on the server-side bridge argument. - transport.test.ts: the cross-connection permission test now asserts a duplicate vote on the same id does not reach the bridge again, locking down the cleanup guarantee that is the core of this PR. Declined (replied on the threads): a blanket local try/catch around the bridge vote (would shadow the outer dispatcher's typed-error mapping for non-permission errors) and success-side audit logging in the generic findPendingClientRequest (log noise / out of scope). Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Address the fifth ci-bot review round on PR QwenLM#5912: - session/permission now maps every permission-specific bridge throw like REST's sendPermissionVoteError: InvalidPermissionOptionError -> 400, PermissionForbiddenError -> 403, PermissionPolicyNotImplementedError -> 501 (policy), CancelSentinelCollisionError -> 500 (requestId/sentinel). The last two previously fell through to the outer dispatcher catch and became a generic -32603 without structured metadata. - Truly unexpected bridge/sessionCtx failures now run the same cancelAbandonedPermission fallback as the legacy resolveClientResponse path (dropping the entry only if the cancel landed, else keeping it for teardown) before rethrowing — so an unexpected error no longer leaves the mediator blocking the agent's prompt until session teardown. - parsePermissionResponse rebuilds the outcome sub-object from its validated keys instead of forwarding it verbatim, so a client can no longer inject extra outcome sub-fields (e.g. force) into the bridge argument; _meta is forwarded only when it is an object. - Tests: cross-connection vote via the session/permission method (ack on the voter's stream + entry removed from the originator), plus the 501 and 500 error mappings. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
… entry Address wenshao's two [Critical] findings on PR QwenLM#5912: - parsePermissionResponse no longer drops the AskUserQuestion `answers` payload. The whitelist tightening forwarded only outcome/_meta, but the bridge treats `answers` (an object map of string values) as the one supported non-ACP permission-response field, so votes were resolving while the agent received no submitted answers. Forward it under the same shape the bridge validates. - The session/permission success path now deletes only the voting connection's OWN pending entry for the requestId, not the first registry-wide match. pendingRef can belong to a sibling connection; under the consensus policy respondToSessionPermission returns true for an intermediate "recorded" vote, so deleting a sibling's entry could drop a co-owner's still-needed request and stall the quorum. A cross-connection voter with no own entry deletes nothing and leaves the originator's entry for teardown. - Tests: forward-answers/strip-unknown-fields case, and the cross-connection method test now asserts a co-owner's vote does NOT delete the originator's sibling entry. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Automated Code Review (incremental — final diff audit)
Reviewed commit: d019bafba2b0a4f8979aa0573faf7e7e99b6f444
Scope: Independent audit of the final diff after prior review rounds (wenshao + qwen-code-ci-bot) and fixup commits 565aade, 8818d3a, 3b48cd5, 8dd368b, 78b2ed3, 498aa32, 8110174, d019baf.
Audit focus (per review request)
- Permission vote resolution correctness
- Race conditions across connections
- Security (vote forgery / duplication)
Findings
No new HIGH-CONFIDENCE findings. All prior inline threads (20+) have been resolved in the fixup commits. Independent re-read of the final diff confirms:
- Vote authorization is sound. Both the new
session/permissionhandler (requireOwned(conn, sessionId, id)) and the legacyresolveClientResponsepath (conn.ownsSession(pending.sessionId)) enforce that the voting connection must own the target session before the bridge is invoked. Same-connection behavior is preserved; cross-connection resolution is gated on the same authorization predicate. - Request IDs are globally unique and unguessable. The connection-qualified format
_qwen_perm_<connectionId>_<counter>makes the per-connectionpendingmap keys globally unique across the registry, sofindPendingPermission/findPendingClientRequestcannot collide. The bridgebridgeRequestId(arandomUUID()) is the wire-facing vote token — computationally unguessable by a malicious client. - No TOCTOU between ownership check and bridge vote. The
session/permissionhandler is synchronous fromrequireOwnedthroughbridge.respondToSessionPermission(noawaitin between), so session teardown cannot interleave and revoke ownership mid-vote in Node's single-threaded execution model. - Success-path deletion is correctly scoped. The manual iteration over
conn.pendingafter an accepted vote deletes only the voting connection's own entry for thebridgeRequestId. Under the consensus policy whererespondToSessionPermissionreturnstruefor intermediate recorded votes, this preserves sibling co-owner entries so a second quorum vote can still land. Siblings are reaped byabandonPendingForSessionat teardown. - No injection surface in
parsePermissionResponse. Outcome is rebuilt from validated keys,answersis re-validated asRecord<string, string>, and only_metais passed through. Client-supplied extra keys are dropped. - Error parity with REST is complete.
InvalidPermissionOptionError(400),PermissionForbiddenError(403),PermissionPolicyNotImplementedError(501),CancelSentinelCollisionError(500), registry-miss (404), session-mismatch (409), and bridge-rejection (409) all have stablecodestrings and distincthttpStatus, matchingsendPermissionVoteError. index.tsdispatcherRefbreak is defensive-correct. The registry's abandon-callback guards against the pre-initialization window, matching the existingdetachClientpattern.
Verdict
The security and correctness model holds: the bridge remains the single source of truth for permission state, the registry is the authorization map for cross-connection routing, and the voting connection's session ownership is the only gate that matters. The design note in the PR body (cross-connection vote drops silently on main after HTTP 202) is fixed as described.
Prior CI failures referenced in the bot's downgraded review are the remaining blocker — this audit does not re-evaluate CI state.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Automated Code Review
Review coverage: 9 parallel reviewers + 1 verification agent + 2 reverse-audit rounds (0 new findings in round 2)
Deterministic: tsc ✅ | eslint ✅
Tests: 229 passed (25 connection-registry + 204 transport)
Needs Human Review
Possibly: answers silently dropped without logging — dispatch.ts:331
When answers is present but values aren't all strings (e.g. { q1: 42 }), the entire field is silently omitted. The client receives success but the agent has no answers. A writeStderrLine warning when answers is present but fails validation would aid debugging.
Possibly: resolveClientResponse ignores bridge return value — dispatch.ts:3398
The legacy path unconditionally deletes the pending entry after respondToSessionPermission, regardless of the boolean return. The session/permission handler retains the entry on bridge rejection (returning 409). This inconsistency means a retry after a bridge rejection on the legacy path gets a misleading 404 instead of 409.
— qwen3.7-max via Qwen Code /review
…o voter Address the sixth ci-bot review round on PR QwenLM#5912 (2 Critical + 4 suggestions): - resolveClientResponse now validates/whitelists the client result through the same parsePermissionResponse the session/permission handler uses. This PR had widened that legacy path to any co-owning connection (via findPendingClientRequest), so the raw `result as unknown` cast was a cross-connection injection surface for arbitrary top-level args and extra outcome sub-fields; a malformed result still throws and hits the cancel fallback as before. - The unexpected-error cancel fallback in the session/permission handler now drops only the VOTING connection's own entry (via the new shared dropOwnPendingPermission helper), not pendingRef — which is the first registry-wide match and may be the originator's entry, whose deletion would stall a consensus quorum still awaiting other co-owners. - parsePermissionResponse logs a stderr line when a present-but-malformed `answers` is dropped, instead of silently discarding it. - Removed ConnectionRegistry.deletePendingPermission: it had no production callers and its first-match semantics were unsafe under co-owned sessions (deletion is done connection-scoped in the dispatcher). - Tests: _meta object-preserved / non-object-dropped, and the generic unexpected-error fallthrough (cancel fallback runs + error propagates). Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Address wenshao's remaining [Critical] on PR QwenLM#5912: the session/permission ownership rejection went through the shared requireOwned, which sends an INVALID_PARAMS error with no `data` envelope. Every other error path in this handler carries `{ httpStatus }` (404/409/400/403/500/501), so SDK callers that classify permission-vote failures by error.data.httpStatus got undefined for the likeliest cross-connection failure (right session header, no session/new on this connection). Inline the ownership check so the rejection carries httpStatus 403 + sessionId + requestId, leaving the shared requireOwned untouched for other handlers. Test asserts the 403. (wenshao's other two criticals — legacy raw-result forwarding and the catch-all deleting the originator's entry — were already fixed in 4bb06e5.) Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds cross-connection permission vote routing for ACP-over-HTTP, allowing a permission response arriving on one HTTP connection to resolve a pending request originated on a different connection. The design is sound — connection-qualified IDs, registry-wide scan, ownership checks — and the test coverage is extensive (1115+ new lines). Deterministic checks pass (eslint clean, typecheck clean, 231/231 tests green).
However, I found one correctness issue in the legacy resolveClientResponse path and a few test gaps worth addressing.
[Critical] resolveClientResponse passes voter's conn to sessionCtx instead of originator's pendingConn
dispatch.ts:3442 — When a cross-connection permission vote arrives via resolveClientResponse, the bridge is called with this.sessionCtx(conn, pending.sessionId, fromLoopback). Here conn is the voter's connection (the one that sent the result), but pendingConn is the originator's connection (the one that created the pending permission request).
This is inconsistent with the line immediately below it — this.dropResolvedPermission(pendingConn, id) — which correctly uses pendingConn. It's also inconsistent with cancelAbandonedPermission, which correctly resolves pendingConn.sessions.get(...)?.clientId.
The practical impact: sessionCtx derives clientId from the connection's session map. If the voter's connection doesn't own pending.sessionId (which is the entire point of cross-connection routing), the bridge receives a wrong or missing clientId, which could cause the permission vote to be misattributed or rejected downstream.
Suggested fix: Change conn to pendingConn at line 3442:
this.sessionCtx(pendingConn, pending.sessionId, fromLoopback),[Suggestion] Legacy resolveClientResponse path's parsePermissionResponse validation is untested for cross-connection scenarios
The new parsePermissionResponse call in resolveClientResponse (line ~3431) applies the same whitelist validation as the session/permission handler, but the test suite only covers cross-connection permission resolution through the session/permission JSON-RPC method. There are no tests exercising resolveClientResponse with malformed permission results (e.g., invalid outcome values, extra fields) arriving on a different connection than the originator.
Consider adding a transport test that sends a permission result with an invalid outcome via the legacy response path to verify it returns the correct error response.
[Suggestion] Cancel fallback cancelled=false branch in session/permission catch block is untested
When the bridge vote throws and the cancel fallback also fails (cancelled=false), the handler retains the pending entry for teardown cleanup. This is a reachable but untested branch. A test that mocks both respondToSessionPermission and cancelAbandonedPermission to throw would verify the retention behavior and the stderr message.
Address the latest ci-bot suggestion on PR QwenLM#5912: parsePermissionResponse throws AcpParamError, a plain Error with no httpStatus, which the outer dispatcher catch maps to a bare INVALID_PARAMS — inconsistent with every other error path in this handler (404/409/400/403/500/501 all carry httpStatus). Catch AcpParamError locally and return a structured 400 with requestId, so SDK callers that classify by error.data.httpStatus see a consistent shape. The parametrized invalid-outcome test now asserts the 400. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…answers Address wenshao's three suggestions on PR QwenLM#5912: - findPendingClientRequest parses the originating connectionId from the server-minted id format (_qwen_perm_<connectionId>_<counter>) for an O(1) byId lookup, falling back to the full scan for client-chosen ids. - The session/permission success path now writes a stderr line ("vote accepted") so an operator debugging a stuck prompt can tell it apart from "vote never arrived" or "landed on another connection" — every failure branch already logs. - Added a test for the malformed-answers branch (non-string values) asserting the vote still lands but answers are not forwarded to the bridge. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Dismissing this stale automated CHANGES_REQUESTED: its thread was addressed and resolved in 9841535 (which post-dates it), no new findings on the current head, and the automated reviewer does not re-approve on its own. Re-review welcome on the latest commits.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Reviewed the latest head after the ACP permission-vote fixes. The prior critical paths are addressed: both vote entry points now share validation, ownership rejections carry structured status metadata, unexpected bridge failures keep the right pending entries, and the latest commit covers the malformed answers / lookup / success-log follow-ups. I found no new critical issue in this pass. CI is still pending, so I am leaving this as a comment rather than an approval.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: Comment
This PR adds cross-connection ACP-over-HTTP permission vote routing with connection-qualified IDs, a new session/permission JSON-RPC handler, and cross-connection resolution in resolveClientResponse. The design is solid — connection-qualified IDs prevent collisions, the parsePermissionResponse validator is reused consistently, and the multi-client co-ownership model is well-documented in comments.
CI: 30/30 checks passing ✅
Tests: 232 tests passing (25 registry + 207 transport) ✅
Deterministic checks: 0 eslint findings, 25 tsc findings (all environment-related @qwen-code/acp-bridge module resolution failures in the worktree, not PR-introduced)
Findings
Two inline comments below:
-
[Suggestion]
session/permissionhandler — missingrequestIderror path lacks the{ httpStatus }envelope that every other error branch in the same handler carries. SDK callers classify permission-vote failures byerror.data.httpStatus, so this omission makes the missing-requestId case indistinguishable from a generic INVALID_PARAMS. -
[Critical — Test Coverage]
resolveClientResponsecatch block — the cross-connection malformed-result path (whereparsePermissionResponsethrows on a non-owning connection's response) has no integration test. Thesession/permissionhandler has thorough error-mapping tests, but this legacy path's cross-connection variant is untested.
Overall: well-structured PR with good test coverage of the new session/permission handler. The two findings above are minor consistency/coverage gaps.
…ote test Address the latest ci-bot review on PR QwenLM#5912: - The missing-`requestId` rejection in session/permission was the only error branch without an { httpStatus } envelope. Add httpStatus 400 (+ requestId) so SDK callers can classify it like every other validation error here. Test asserts the 400. - Add an integration test for the legacy resolveClientResponse cross-connection variant: connection B (a co-owner) answers connection A's permission request with a malformed result, parsePermissionResponse (added for this path) throws, and the cancel fallback still releases the mediator. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Incremental review (3e68e97..43738c6): 1 commit, 2 files, +54/-2.
Commit 43738c6 adds httpStatus: 400 to the missing-requestId error envelope in session/permission — previously the only error path in this handler without it. Now consistent with all other error paths (409/404/403/400/501/500). The cross-connection malformed vote test properly covers the multi-connection scenario where a co-owner sends a malformed response, verifying the cancel fallback still releases the mediator.
No issues found. LGTM.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
Review Summary
Review coverage: 9 parallel reviewers (correctness, security, code quality, performance, test coverage, 3× undirected audit personas) + build/test verification
Deterministic: tsc ✅ | eslint ✅ (0 findings)
Build: ✅ all packages compiled
Tests: 233 passed (25 connection-registry + 208 transport), 0 failures in 18.84s
Assessment
The cross-connection ACP permission vote resolution is well-engineered. The connection-qualified ID format (_qwen_perm_<conn>_N), the session/permission JSON-RPC handler, and the cross-connection guard in resolveClientResponse are all correctly implemented with consistent authorization checks, structured error responses, and comprehensive stderr logging.
Key strengths:
- Authorization is correctly enforced: cross-connection votes require session ownership (
ownsSessioncheck, 403 on rejection) parsePermissionResponseproperly whitelists fields (outcome,answers,_meta) and is shared between both vote entry points, closing the injection surface- All error paths carry
{ httpStatus }envelopes for SDK classification, with typed bridge errors mapped to match REST'ssendPermissionVoteError - Entry deletion is correctly scoped to the voting connection's own pending map key (
dropOwnPendingPermission), preserving sibling co-owner entries under consensus policies - The legacy
resolveClientResponsepath now validates through the sameparsePermissionResponse, preventing co-owner injection - Defensive
dispatcherRefinitialization inindex.tsprevents teardown races
— qwen3.7-max via Qwen Code /review
✅ Local verification report — cross-connection ACP permission votesMaintainer local verification of head TL;DR — Verified. Both fixes behave as described, the new tests are load-bearing (proven by surgical mutation), the cross-connection authorization gate is sound on both vote paths, and there are no regressions in the 1. Baseline (PR head, real code)
2. The two claims, cross-checked against
|
| # | Mechanism reverted | Reproduces (main behavior) |
Targeted test | Clean → Mutated |
|---|---|---|---|---|
| M1 | registry-wide lookup → conn.pending.get(id) only |
cross-conn vote silently dropped after 202 | cross-connection resolve | 4 pass → 2 fail * |
| M2 | nextId() drops the _<conn>_ qualifier |
_qwen_perm_N collides across connections |
registry locator | 1 pass → 1 fail |
| M3 | remove case 'session/permission' |
Unknown method: session/permission |
method resolves | 1 pass → 1 fail |
| M4 | drop ownership gate (session/permission path) |
unowned connection can vote | unowned vote rejected | 1 pass → 1 fail |
| M5 | drop ownership gate (legacy response path) | unowned connection can vote | unowned response ignored | 1 pass → 1 fail |
* The cross-connection name filter matches 4 tests; the 2 that route through the reverted lookup fail, the other 2 (which don't) stay green — exactly as expected.
4. Independent over-the-wire tests (authored fresh, own spy + assertions)
To avoid simply re-running the author's assertions, I wrote 3 new tests on the same real-server harness. All pass on head, and each goes red under the relevant mutation:
- Fix — connection B (co-owner) resolves a permission streamed by connection A → reaches the bridge exactly once. → red under M1.
- Security — a connection that never claimed the session is refused on both paths: the legacy response (silently dropped, bridge never called) and the
session/permissionmethod (403envelope on its connection stream);respondToSessionPermissionis never invoked. → red under M4 and under M5. - Control — an owned connection does resolve via
session/permission(proves the denial above is the ownership gate, not a dead path).
5. Notes / scope
- Faked boundary = the bridge only; HTTP, SSE, dispatcher, registry and the connection lifecycle are all real. A full model-round-trip e2e wasn't necessary because the observable contract point (
respondToSessionPermission) is exercised faithfully over real HTTP/SSE. - "Before" was not run as a separate
maindaemon; it is reproduced surgically per-mechanism via M1/M3/M4/M5. - This is an independent behavioral verification provided as a merge reference; required-check status and any open review threads are tracked separately.
Environment: local macOS; packages built via npm ci at head 43738c6c.
中文版（完整对应）
✅ 本地验证报告 —— 跨连接 ACP 权限投票
维护者对 head 43738c6c 的本地验证。在独立 git worktree 中 npm ci 构建，并跑了真实的 over-the-wire ACP HTTP 传输：真实 Express server 监听真实端口，用真实 fetch + SSE 解析驱动。只有 HttpAcpBridge 被 fake —— 这是正确的边界，因为本 PR 的逻辑完全是 dispatcher 路由 + 授权，而 respondToSessionPermission(...) 正是"投票是否到达 mediator"的可观测点。
结论 —— 已验证。 两个修复都与描述一致；新增测试是承重的（通过定向变异证明）；跨连接授权门在两条投票路径上都成立；acp-http 测试套件无回归。tsc / eslint / git diff --check 均干净。
1. 基线（PR head，真实代码）
transport.test.ts+connection-registry.test.ts→ 233 通过- 整个
src/serve/acp-http/目录 → 284 通过（8 个文件），无回归 tsc --noEmit→ exit 0 ·eslint（5 个改动文件）→ exit 0 ·git diff --check→ 干净
2. 两个论断，对 main 逐一核验
session/permission确实是真实缺口。 在origin/main上，packages/sdk-typescript/src/daemon/acpRouteTable.ts:164-182已经把POST /session/:id/permission/:reqId映射到session/permission，而设计文档sse-resumable-stream.md明确写了 daemon "has no handler for" 它。所以新 handler 填的是真实缺口（修复前：Unknown method: session/permission）。- harness 忠实。 fake bridge 的
respondToSessionPermission(sessionId, requestId, response, context): boolean与真实HttpAcpBridge契约（bridgeTypes.ts:441）逐字一致，所以 over-the-wire 测试跑的是真实投票契约，而不是一个方便的替身。
3. 定向变异测试（测试是否承重？）
在 PR head 上逐个回退每一处修复机制；对应测试由绿翻红 —— 同时这也按机制复现了 main 的"修复前"行为：
| # | 回退的机制 | 复现（main 行为） |
命中的测试 | 干净 → 变异 |
|---|---|---|---|---|
| M1 | registry 全局查找 → 仅 conn.pending.get(id) |
跨连接投票在 202 之后被静默丢弃 | 跨连接 resolve | 4 通过 → 2 失败 * |
| M2 | nextId() 去掉 _<conn>_ 限定 |
_qwen_perm_N 跨连接撞号 |
registry 定位器 | 1 通过 → 1 失败 |
| M3 | 移除 case 'session/permission' |
Unknown method: session/permission |
method 解析 | 1 通过 → 1 失败 |
| M4 | 去掉所有权门（session/permission 路径） |
未拥有 session 的连接也能投票 | 拒绝越权投票 | 1 通过 → 1 失败 |
| M5 | 去掉所有权门（legacy 响应路径） | 未拥有 session 的连接也能投票 | 忽略越权响应 | 1 通过 → 1 失败 |
* cross-connection 名称过滤匹配到 4 个测试；走被回退查找路径的 2 个失败，另外 2 个（不走该路径）保持绿 —— 完全符合预期。
4. 我自己独立编写的 over-the-wire 测试（全新 spy + 断言）
为了不只是复跑作者的断言，我在同一个真实 server harness 上新写了 3 个测试。head 上全部通过，且在对应变异下各自翻红：
- 修复 —— 连接 B（co-owner）解析了由连接 A stream 出来的权限 → 恰好到达 bridge 一次。→ M1 下翻红。
- 安全 —— 从未 claim 过该 session 的连接在两条路径上都被拒：legacy 响应（静默丢弃，bridge 从不被调用）以及
session/permissionmethod（在其连接流上回403信封）；respondToSessionPermission始终未被调用。→ M4 和 M5 下都翻红。 - 对照 —— 拥有 session 的连接确实能通过
session/permission解析（证明上面的拒绝是所有权门，而非一条死路径）。
5. 说明 / 范围
- 被 fake 的边界只有 bridge；HTTP、SSE、dispatcher、registry、连接生命周期全是真实的。不需要完整的模型往返 e2e，因为可观测契约点（
respondToSessionPermission）已在真实 HTTP/SSE 上被忠实地驱动。 - "修复前"没有单独再起一个
maindaemon，而是通过 M1/M3/M4/M5 按机制定向复现。 - 这是一份独立的行为验证，作为合并参考；required check 状态与未关闭的 review thread 另行跟踪。
环境：本地 macOS；在 head 43738c6c 用 npm ci 构建。
🔎 Independent local verification by the maintainer — real over-the-wire harness + surgical mutation testing. Verification only; not a re-review of every line.
5 participants
What this PR does
This PR fixes the ACP-over-HTTP permission vote path so a permission response is no longer tied only to the connection that streamed the permission request. It gives daemon-issued permission request ids a connection-qualified namespace, lets the dispatcher find pending permission requests across live ACP connections, and only accepts a cross-connection vote when the voting connection also owns the session. It also implements the daemon-side
session/permissionJSON-RPC method that the ACP route table already emits for permission vote URLs, returning connection-stream acknowledgements and HTTP-status metadata for SDK callers.Why it's needed
I audited this against latest
main(51ec7c36f78c) before opening the PR. The core issue from the §1.7 note is still present onmain: a vote posted on a differentAcp-Connection-Idmisses the per-connection pending map and is silently dropped after the HTTP POST has already returned202, leaving the bridge mediator unresolved. Latestmainalso already maps permission vote URLs tosession/permissionin the SDK route table, but the daemon dispatcher had no matching method and returnedUnknown method: session/permission.Relationship to #5852: #5852 is still open and not merged into
main, so this PR is intentionally based directly on latestmainand does not include #5852's resumable/acpstream, grace-window, or Last-Event-ID work. The grace-deferral part of the design note remains #5852-dependent; the SDK vote URL mapping is already present onmain. This PR fixes only the independently reproducible daemon permission resolve gap that exists today onmain.Reviewer Test Plan
How to verify
Confirm that a permission request streamed on one ACP connection can be resolved by another connection that has also claimed the same session, and that a connection with no claim on that session cannot resolve it. Confirm that
session/permissionresolves by the bridge request id and returns its acknowledgement on the connection stream.Local commands run:
cd packages/cli && npx vitest run src/serve/acp-http/connection-registry.test.ts src/serve/acp-http/transport.test.ts;NODE_OPTIONS=--max-old-space-size=8192 npx eslint packages/cli/src/serve/acp-http/connection-registry.ts packages/cli/src/serve/acp-http/dispatch.ts packages/cli/src/serve/acp-http/index.ts packages/cli/src/serve/acp-http/connection-registry.test.ts packages/cli/src/serve/acp-http/transport.test.ts;npm run build;npm run typecheck;git diff --check.Evidence (Before & After)
Before: on latest
main, the targeted cross-connection test timed out waiting for the permission to resolve, and the targetedsession/permissiontest returned JSON-RPCUnknown method: session/permission. After: the full ACP HTTP transport test file and registry test pass locally (180 passed).Tested on
Environment (optional)
Local repository checkout using the package npm scripts. Root
npm run lintwas attempted with an 8GB heap but was manually interrupted after roughly three minutes with no output; the focused ESLint command above passed, and the pre-commit hook also ran prettier plus ESLint on the touched files.Risk & Scope
main.Linked Issues
Related to #5852.
中文说明
What this PR does
这个 PR 修复 ACP-over-HTTP 的权限投票路径，让权限响应不再只能由“收到 permission request 的同一个 connection”解析。它把 daemon 发出的 permission request id 改成带 connection 命名空间的全局唯一字符串，让 dispatcher 能在所有 live ACP connections 里查 pending permission，并且只有投票 connection 也拥有该 session 时才允许跨 connection 投票。同时补上 daemon 侧
session/permissionJSON-RPC 方法，因为 SDK route table 在 main 上已经会把权限投票 URL 映射到这个方法。Why it's needed
我先基于最新
main（51ec7c36f78c）做了审计再起 PR。§1.7 文档里的核心问题在 main 上仍然存在：如果 vote POST 使用了不同的Acp-Connection-Id，dispatcher 只查当前 connection 的 pending map，于是 miss 后静默丢弃；HTTP POST 已经返回202，但 bridge mediator 没有被 resolve，agent 就会卡住。main 上还已经有 SDK route table 到session/permission的映射，但 daemon dispatcher 没有对应 case，所以会返回Unknown method: session/permission。和 #5852 的关系：#5852 仍然 open 且还没有合入 main，所以这个 PR 刻意直接基于最新 main，不包含 #5852 的 resumable
/acpstream、grace window 或 Last-Event-ID 工作。设计文档里的 grace deferral 仍然依赖 #5852 的语境；SDK vote URL mapping 在 main 上已经存在。这个 PR 只修 main 当前可独立复现的 daemon permission resolve 缺口。Reviewer Test Plan
How to verify
确认一个 ACP connection stream 出来的 permission request，可以被另一个同样拥有该 session 的 connection resolve；同时确认没有 claim 该 session 的 connection 不能越权 resolve。再确认
session/permission能按 bridge request id resolve，并且 ack 走 connection stream。本地执行过的命令：
cd packages/cli && npx vitest run src/serve/acp-http/connection-registry.test.ts src/serve/acp-http/transport.test.ts；NODE_OPTIONS=--max-old-space-size=8192 npx eslint packages/cli/src/serve/acp-http/connection-registry.ts packages/cli/src/serve/acp-http/dispatch.ts packages/cli/src/serve/acp-http/index.ts packages/cli/src/serve/acp-http/connection-registry.test.ts packages/cli/src/serve/acp-http/transport.test.ts；npm run build；npm run typecheck；git diff --check。Evidence (Before & After)
Before：在最新 main 上，新增的 cross-connection 定向用例会超时等待 permission resolve，
session/permission定向用例会返回 JSON-RPCUnknown method: session/permission。After：完整 ACP HTTP transport 测试文件和 registry 测试在本地通过（180 passed）。Tested on
Environment (optional)
本地仓库 checkout，使用 package npm scripts。曾用 8GB heap 尝试 root
npm run lint，约三分钟无输出后手动中止；上面列出的 focused ESLint 已通过，pre-commit hook 也对本次 touched files 跑了 prettier 和 ESLint。Risk & Scope
Linked Issues
Related to #5852.