feat(serve): ACP WebSocket transport (RFD Streamable HTTP phase 2) - #4773

This is a placeholder PR (0 files changed, +0/-0). The implementation plan for ACP WebSocket transport looks well-structured, but there is no code to review yet.

Will review once code is pushed and dependency PRs (#4737 → #4736) are merged.

— qwen3.7-max via Qwen Code /review

Stacked PR Findings (not in diff — from dependency PRs #4736/#4737)

dispatch.ts:

• [Critical] sessions/delete (line 1662) accepts any sessionIds array without checking conn.ownsSession(). Compare to session/close (line 734) which calls requireOwned(). Any authenticated connection can delete other connections' sessions.
• [Critical] workspace/agents/create and agents/update (lines 1729, 1794) skip input validations enforced by the REST layer: tools element types, max entries (256), max per-entry length (256 chars), description/systemPrompt max size (256KB), non-empty checks. Also, agents/update and agents/delete don't call assertMutableLevel() — builtin/extension/session-level agents can be modified via ACP while REST blocks with 403.
• [Critical] 11 tsc errors: dispatcher calls non-existent HttpAcpBridge methods (generateSessionRecap, generateSessionBtw, executeShellCommand, etc.). PR does not compile.
• [Suggestion] CONN_ROUTED_METHODS (line 86) and buildInitializeResult methods (line 418) maintained separately with no compiler enforcement. Define once and derive both.
• [Suggestion] toRpcError (line 199) forwards SubagentError.message verbatim — leaks absolute filesystem paths for FILE_ERROR subcode. REST API gates behind isServeDebugMode().

server.ts: 25 tsc errors (property mismatches on BridgeSessionSummary, 'err' is of type 'unknown' x19, index signature access).

runQwenServe.ts: 4 tsc errors (missing module @qwen-code/acp-bridge/spawnChannel, implicit any parameters x3).

transport.test.ts: 10 of 56 tests fail — 2 expect status 400 but implementation returns 404 (per RFD); 8 session-extension tests read the wrong SSE frame (connection-level stream receives session/new reply before extension method reply).

Deterministic analysis: tsc = 40 errors total, eslint = 0. Coverage: WsStream (101 lines) and WS upgrade handler (~210 lines) have zero test coverage.

— qwen3.7-max via Qwen Code /review

[Critical] closeSessionStream (line 270) calls binding.stream?.close() unconditionally. After the type widening to TransportStream, WS transport shares the same WsStream instance between conn.connStream and binding.stream (set via conn.attachSessionStream(sid, conn.connStream!, ac) at index.ts:528). Closing any single session tears down the entire WS connection and all other sessions on it.

(Posting as body-level comment because closeSessionStream is on a line not touched by this PR's diff, but the bug is caused by the type widening in this PR.)

— qwen3.7-max via Qwen Code /review

tsc: 12 compilation errors in dispatch.ts (missing BTW_MAX_INPUT_LENGTH import, 9 missing methods on HttpAcpBridge, 2 type mismatches). The build does not compile.

Test coverage: zero tests for new code paths — WsStream adapter (83 lines, parallel SseStream has 8 test scenarios), toRpcError 7 new error-type branches, sessions/delete batch orchestration.

Needs human review: device_flow/get returns full state to non-initiators (REST redacts), session/shell passes no AbortSignal (commands run 120s after disconnect), tools array skips element type validation.

— qwen3.7-max via Qwen Code /review

Independent Review at HEAD 9a61356e

PR: feat(serve): ACP WebSocket transport (RFD Streamable HTTP phase 2) — +1345/-21, 9 files, 3 commits

This review validates wenshao's 2 rounds of CHANGES_REQUESTED (15 Critical + 16 Suggestions) and adds 1 additional Critical finding not previously raised.

Additional Finding (not in wenshao's reviews)

Verified: grep -n 'cmd\b' dispatch.ts shows cmd used at lines 1057/1061 but never declared. rawCmd declared at line 1043, used only in the validation check.

Validated wenshao Critical findings (independently confirmed)

Security (must-fix before merge)

Reliability (must-fix before merge)

Protocol/Correctness

Prioritized fix plan

P0 — Security (blocks merge):

1. W-C2: Add Origin header check on WS upgrade (CSRF)
2. W-C26: Redact device flow verification data for non-initiators
3. W-C27: Add BuiltinAgentRegistry.isBuiltinAgent() shadow check to agents/create

P1 — Reliability (blocks merge):
4. NEW-1: Fix cmd → rawCmd in session/shell handler
5. W-C1/C10: Add connStream identity guard in closeSessionStream
6. W-C7: Add pong listener to WsStream heartbeat
7. W-C19: send() should return this.writeChain not raw next
8. W-C4 + W-C14: Serialize WS message handlers (fixes both concurrent handler and lazy attach race)
9. W-C8: Add pending-write cap to WsStream.writeChain
10. W-C28: Clean up registry state in sessions/delete

P2 — Protocol (should-fix):
11. W-C5/W-C6: Initialize response error callback + uninitialized connection timeout
12. W-C12: socket.destroy() on non-matching upgrade path
13. W-C25: Use validateMcpRuntimeServerName for MCP server names
14. W-C22: Restore configOptions: true or document removal

P3 — Quality (follow-up OK):
15. Suggestions: WeakMap, dispose listener cleanup, loopback DRY, SubagentError codes, agent name trimming, memory constant sharing, etc.

Total: 16 Critical (15 wenshao + 1 new) + 16 Suggestions. Recommend addressing P0+P1 before next review round.

[Critical] sessions/delete (line ~1638) 接受最多 100 个 session ID 并通过 closeSession + removeSessions 删除，但从未调用 requireOwned() 过滤。对比 session/close（line 679）在操作前检查 this.requireOwned(conn, sessionId, id)。任何已认证客户端可批量删除其他客户端的 session——包括磁盘上的持久化数据。建议在 Promise.allSettled 前添加：const ids = [...new Set(sessionIds)].filter(sid => conn.ownsSession(sid));

— qwen3.7-max via Qwen Code /review

Build: 2 tsc errors (dispatch.ts:1747 TS2345 missing level in SubagentConfig — already reported at this line; server.ts:2889 TS4111 dot-access on index signature — already reported at this line).

Tests: 11 failures — 3 capability registry drift (SERVE_CAPABILITY_REGISTRY gained workspace_agent_generate, session_stats, workspace_mcp_manage but EXPECTED_STAGE1_FEATURES in server.test.ts was not updated); 8 runQwenServe afterEach hook timeouts (10s limit exceeded by accumulated cleanup steps).

dispatch.ts:1623 — sessions/delete allows empty-string sessionId: validation checks typeof s === 'string' but not s.length > 0. An array like ["", "abc"] passes. Add && s.length > 0 to the .every() predicate.

Test coverage: zero tests added for ~1,400 lines of new code. Critical untested paths: WS upgrade handler (auth + protocol handshake), WsStream adapter (writeChain serialization, close idempotency, heartbeat), session/shell (command execution security boundary), sessions/delete (concurrent aggregation), POST /acp content-type/batch guards (415/501). Existing transport.test.ts:359 may be broken by the 400→404 status code split.

— qwen3.7-max via Qwen Code /review

tsc: 2 compilation errors (same as R5 — dispatch.ts:1747 TS2345 missing level in SubagentConfig; server.ts:2889 TS4111 dot-access on index signature).

Tests: transport.test.ts — 2 tests fail because the PR changed unknown-connection responses from 400→404 (index.ts:174,231) but the test assertions at lines 366 and 1451 still expect 400. Update tests to expect 404.

Most findings from R5 (53 inline comments) remain unresolved at this same commit. Two new findings below.

— qwen3.7-max via Qwen Code /review

No issues found. LGTM! The previous review's critical findings (CSWSH protection, Host allowlist, maxPayload, message serialization, teardownBinding WS guard) have all been addressed across the commit chain. The latest commit adds appropriate defensive guards for attachSessionStream shared-stream handling and the cleanupSession race condition. Security posture for the WS upgrade path is solid: origin validation, host allowlist, timing-safe bearer token auth, rate limiting, and 10 MB payload cap. The TransportStream abstraction is clean and the WsStream implementation correctly serializes writes, handles heartbeat/dead-connection detection, and integrates with the existing connection registry lifecycle. Good test coverage for security scenarios. -- qwen3-coder via Qwen Code /review

Re-Review at HEAD 9139f286 (3 new commits since fd6d3027)

New Commits

Fixes Verified Since Last Review

wenshao Cross-Validation (Review 4474984014)

New Code Quality Assessment

SHA-256 token hashing (createHash('sha256').update(token).digest()) — eliminates both length oracle and timing side-channel. Correct improvement over raw buffer comparison.

Message serialization (messageQueue = messageQueue.then(...).catch(log)) — proper promise chaining prevents concurrent dispatcher.handle() calls. The .catch() prevents unhandled rejection from stalling the queue. Named function handleWsMessage improves stack traces.

Rate limiting — tryConsume exposed on RateLimiterInstance. WS tier classification mirrors REST. wsKey uses socket.remoteAddress. Consistent.

ACP handle disposal — app.locals['acpHandle'] bracket notation (TS4111 fix). dispose() called during shutdown with try/catch.

Previously Deferred to #4782 (unchanged at HEAD)

connRef: any typing, providerId type assertion, batch JSON-RPC, file/read+file/edit maxBytes, description defaults.

Final Verdict — COMMENT

3 commits add solid security hardening with comprehensive test coverage. One remaining Critical: attachSessionStream connStream guard (confirmed, one-line fix). After this fix, the PR should be ready for merge.

This review was generated by QoderWork AI

All previously reported Critical findings have been addressed in this revision:

• Shared stream close guard: teardownBinding now skips closing binding.stream when it equals conn.connStream, preventing WS connection teardown on session close.
• WS upgrade security: Host allowlist, CSRF origin validation, and bearer token auth (with timing-safe comparison) are all enforced before handleUpgrade.
• maxPayload: WebSocketServer now sets maxPayload: 10 * 1024 * 1024 (10 MB), matching the REST body limit.
• Message serialization: Inbound WS messages are serialized via a messageQueue promise chain, preventing race conditions between concurrent frames.
• Initialize timeout: A 30-second timer closes WS connections that never send initialize.
• dispose() cleanup: The upgrade listener is properly removed on dispose.
• Heartbeat/pong: Dead connection detection via ping/pong with alive flag in WsStream.

The TransportStream abstraction is clean, the rate limiter refactor correctly extracts tryConsume for reuse by both Express middleware and WS, and the test coverage is thorough (unit + integration). LGTM.

— qwen3-235b-a22b via Qwen Code /review

Incremental Review (commits b016ba2 → 2fb0b60)

Good progress on addressing the previous review findings. Here's the status:

✅ Fixed

• WS message serialization — messageQueue per-connection promise chain correctly serializes message processing. handleWsMessage is async and properly chained. ✅
• Rate limiter integration — tryConsume extracted and exposed via checkRate, threaded through MountAcpHttpOptions. ✅
• Token hashing — SHA-256 pre-hash eliminates byte-length side channel. ✅
• acpHandle.dispose() — Called in shutdown sequence; upgrade listener properly removed. ✅
• connStream guard — Both attachSessionStream and teardownBinding now guard against closing shared WS connStream. ✅
• cleanupSession race — Abort controller identity check (b?.abort === myAbort) prevents stale cleanup from closing new pumps. ✅
• WS security tests — 10 new tests covering origin check, token auth, maxPayload, protocol state, serialization, and rate limiting. ✅

Remaining from previous review

• initTimer not cleared on early disconnect (low confidence — minor timer leak)
• No test for connection cap (1013), initTimer timeout, or lazy session stream attachment

New finding

One minor regression in the rateLimit.ts refactor — see inline comment.

— qwen3.7-max via Qwen Code /review

All Critical findings from the initial review have been addressed. LGTM! ✅ — qwen3.7-max via Qwen Code /review

Re-reviewed at bb051e28 (11 commits since last review). No high-confidence issues found.

Summary of changes reviewed:

• WebSocket transport (WsStream) with transport-agnostic TransportStream interface
• WS upgrade handler with CSWSH/DNS-rebinding/bearer-auth security
• Message serialization via promise queue (prevents concurrent handler races)
• Prompt fire-and-forget to avoid deadlock on permission votes
• Shared connStream guard in attachSessionStream/teardownBinding (WS uses one socket for all sessions)
• Rate limiter refactored to expose checkRate() for WS (DRY via tryConsume)
• Token pre-hash (SHA-256) before timingSafeEqual for constant-time comparison
• Explicit read-method allowlist for rate-limit tier classification
• dispose() properly removes upgrade listener from httpServer
• 17 WsStream unit tests + 10 WS security integration tests

APPROVE.

Re-Review at HEAD c6ded92 (8 new commits since fd6d3027)

New Commits

Reviewer Status at HEAD

Cross-Validation Table (Critical findings)

Independent Verification at HEAD

Security layer (all 5 layers confirmed):

1. Host allowlist: loopback + host:port matching ✓
2. CSWSH origin: applies to loopback, IPv6 bracket strip ✓
3. Bearer auth: SHA-256 pre-hash + timingSafeEqual, no length leak ✓
4. Connection cap: registry.create() returns null → 1013 close ✓
5. Init timeout: 30s with .unref(), logged on fire ✓

Message processing:

• messageQueue serializes all messages via promise chain ✓
• Prompt fire-and-forget: const isPrompt = ... → if (!isPrompt) await dispatchP ✓
• Rate limit: WS_EXEMPT_METHODS (heartbeat/metadata), WS_READ_METHODS (explicit set), else mutation ✓

Connection lifecycle:

• connStream guard: conn.connStream && !conn.connStream.isClosed ✓
• Abort identity: myAbort captured, cleanup checks b?.abort === myAbort ✓
• Dispose: removes upgrade listener, closes WSS, disposes registry ✓
• Destroy: try-catch per teardownBinding ✓

Rate limiter:

• tryConsume shared by Express middleware + WS + HTTP ACP ✓
• onError callback preserved on bucket overflow ✓
• IP normalization: ::ffff: prefix strip ✓
• ACP HTTP POST: checkRate(httpKey, tier) with Retry-After: 5 ✓

Remaining Items (all Suggestions, non-blocking)

Final Verdict — LGTM, Recommend Merge

All 21 Critical findings from 5 reviewers addressed across 8 commits. Security layer is defense-in-depth (host → origin → auth → cap → timeout). Message serialization + prompt fire-and-forget resolves the deadlock. Rate limiting achieves full parity between WS and HTTP transports. Remaining 7 items are all Suggestions (pre-existing or acceptable trade-offs). 10 integration tests cover all security paths.

This re-review was generated by QoderWork AI

Incremental Review (commits 2fb0b60 → c6ded92)

Good set of fixes addressing the previous review and additional hardening. Summary of changes:

✅ Well done

• Prompt fire-and-forget — session/prompt no longer blocks the message queue, preventing deadlock when permission votes or cancel requests are queued behind a long-running prompt. Clear comment explaining the rationale.
• Type safety — connRef: AcpConnection | undefined replaces any, upgradeListener properly typed, connStream! non-null assertion removed with proper guard.
• Token hash pre-computed — expectedTokenHash computed once at setup scope, not per-upgrade.
• destroy() exception safety — teardownBinding wrapped in try-catch so one session failure doesn't prevent others from being torn down.
• Rate limit bucket overflow — onError callback restored in tryConsume, fixing the regression from last round.
• Rate limit method classification parity — Explicit WS_EXEMPT_METHODS / WS_READ_METHODS sets ensure REST and WS rate limiting use the same tier classification.
• REST POST rate limiting — checkRate now applied to non-initialize POST requests, closing the gap where REST POST had no rate limit.
• Init timeout logging — 30s init timeout now logs the source address for debugging.
• Lazy session stream guard — conn.connStream && !conn.connStream.isClosed prevents attaching to a closed stream.
• wsKey IPv6 normalization — Matches normalizeIp behavior used in REST middleware.

Findings

Two minor observations — see inline comments.

— qwen3.7-max via Qwen Code /review

All previous Critical findings resolved. Two minor Suggestions remain (naming + rate-limit key parity) but are non-blocking. LGTM! ✅ — qwen3.7-max via Qwen Code /review