feat(acp): add session-scoped runtime MCP - #7847
Conversation
Rebase verification
Live channel-loop execution is intentionally deferred to the serialized follow-up integration PR. This PR provides only the session-scoped routing primitive. |
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Supersedes #7645 — good to see it carried forward. Template looks good ✓ Problem: this is a real architectural gap, not theoretical hardening. Daemon-managed channels need session-private tools, and the existing workspace-scoped route fans out to every live session in the runtime — so a channel-loop server registered for one session would leak into unrelated terminal and web sessions. The linked work (#7628, #7641, both merged) confirms this is the next building block in the daemon channel infrastructure. Direction: aligned. Session isolation for runtime MCP is a natural extension of the workspace route, and the PR stays generic — no channel-loop business logic, just the isolation boundary. CHANGELOG has no direct reference to session-scoped runtime MCP, but session isolation and background-session MCP are active areas upstream; the direction is sound. Size: cross-package (acp-bridge + cli) → core infrastructure gate applies. 471 production lines (23 bridge.ts + 21 bridgeClient.ts + 8 bridgeOptions.ts + 69 bridgeTypes.ts + 2 status.ts + 348 acpAgent.ts) vs. 267 test lines (104 + 84 + 79). Under the 500-line awareness threshold. The acpAgent.ts delta is mostly the refactoring extraction — net new logic is modest. Approach: the scope feels right. Extracting the shared validation/stripping helpers ( Moving on to code review. 🔍 中文说明感谢贡献！本 PR 替代 #7645，很高兴看到它被延续。 模板完整 ✓ 问题：这是一个真实的架构缺口，不是理论性加固。daemon-managed channel 需要 session 私有工具，而现有的 workspace 路由会扩散到 runtime 中所有 live session——因此为一个 session 注册的 channel-loop server 会泄漏到无关的终端和 Web session。关联的 #7628、#7641（均已合并）确认这是 daemon channel 基础设施的下一个构建块。 方向：对齐。session 级运行时 MCP 隔离是 workspace 路由的自然扩展，PR 保持通用——不含 channel-loop 业务逻辑，只提供隔离边界。CHANGELOG 没有直接引用，但 session 隔离和后台 session MCP 是上游活跃领域；方向合理。 规模：跨包（acp-bridge + cli）→ 触发核心基础设施门禁。471 行生产代码 vs. 267 行测试代码。低于 500 行关注阈值。acpAgent.ts 的变化主要是重构提取——净新逻辑不多。 方案：范围合理。从内联 workspace handler 中提取共享验证/剥离辅助函数并在 session 级 handler 中复用，是自然的去重。bridgeClient.ts 中的 session 身份校验（ownsSession）封堵了反向路径缺口。没有看到更简路径——workspace 和 session 路由确实需要不同的扩散行为，共享辅助函数避免了安全剥离逻辑的重复。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewIndependent proposal: I would have done essentially what this PR does — extract the inline validation/stripping/error-mapping from the workspace handlers into shared helpers, add two new ext-method constants, wire session-scoped handlers that call Findings: No critical blockers. One suggestion (non-blocking): the refactoring drops the detailed comments that explained why Everything else is clean:
TestingThis is protocol and routing infrastructure with no user-visible behavior — real-scenario testing is N/A. CI evidence from the PR's own checks on Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 No failures so far. The Node unit suite and daemon E2E are still running — approval is deferred until they land. 中文说明代码审查独立方案： 我基本上会做和本 PR 相同的事情——将内联的验证/剥离/错误映射从 workspace handler 提取为共享辅助函数，添加两个新的 ext-method 常量，接入 session 级 handler（调用 sessionOrThrow + session 自身的 manager，不扩散），通过 requestSessionStatus 添加 bridge 方法，并在反向 MCP 路径上通过 ownsSession 校验传播 session 身份。PR 与此高度吻合。 发现： 无关键阻塞项。 一个建议（非阻塞）：重构删除了解释 clientMcpOverWs === true 为何强制 type: 'sdk' 的详细注释——原文说明 manager 必须绑定 SdkControlClientTransport，其 sendMcpMessage 通过 daemon WS 回传（#5626 Phase 2）。新的 readRuntimeMcpAddRequest 保留了较短的注释，但传输绑定的理由丢失了。建议在那里保留一行说明，因为 type 覆盖在没有上下文的情况下看起来是任意的。 其余部分干净：安全剥离忠实于原始代码；错误映射一致；session 级 handler 正确跳过了兄弟扩散；bridgeClient.ts 验证 sessionId 并检查 ownsSession；ClientMcpMessageSender 签名变更向后兼容；类型提取干净；测试覆盖了关键路径。 测试本 PR 是协议和路由基础设施，没有用户可见行为——真实场景测试不适用。 CI 证据来自 PR 自身在 265bd72 上的检查：目前无失败。Node 单元测试套件和 daemon E2E 仍在运行——批准推迟到它们完成。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — clean, focused infrastructure PR; only nit is a lost comment worth restoring. This does one thing and does it well: it gives the daemon a session-scoped MCP route that doesn't leak into sibling sessions. The refactoring of the workspace handlers into shared helpers is the right call — it avoids duplicating the security-stripping logic across two code paths, and the extracted functions ( The session identity propagation on the reverse path ( Tests cover the important paths: routing isolation, unknown-session rejection, reverse-path identity forwarding, foreign-session rejection, and security-field stripping. The test in The one thing I'd restore: the comment explaining why Approval deferred until CI lands green on 中文说明置信度：4/5 —— 干净、聚焦的基础设施 PR；唯一的小问题是值得恢复一条被删除的注释。 本 PR 只做一件事并做好：为 daemon 提供 session 级 MCP 路由，不会泄漏到兄弟 session。将 workspace handler 重构为共享辅助函数是正确的选择——避免了在两个代码路径中重复安全剥离逻辑，提取的函数足够直白，读者不需要追踪抽象就能理解发生了什么。 反向路径上的 session 身份传播（ownsSession 校验 + ClientMcpMessageSender 上的可选 context）是良好的安全边界——防止子进程冒充其他 channel 的 session。向后兼容的签名变更意味着现有 sender 无需修改即可继续工作。 测试覆盖了重要路径：路由隔离、未知 session 拒绝、反向路径身份转发、外部 session 拒绝、安全字段剥离。acpAgent.test.ts 中验证第一个 session 的 manager 未被调用的断言，正是此功能需要的正确断言。 唯一建议恢复的：解释 clientMcpOverWs === true 为何强制 type: 'sdk' 的注释（#5626 的 SdkControlClientTransport 绑定）。没有它，readRuntimeMcpAddRequest 中的 type 覆盖看起来是任意的。非阻塞。 批准推迟到 CI 在 265bd72 上全部通过。 — Qwen Code · qwen3.8-max-preview Reviewed at |
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 — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
| const response = await send( | ||
| payload, | ||
| typeof sessionId === 'string' ? { sessionId } : undefined, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The sessionId context is validated and passed to send(), but the sole production sender — ClientMcpSenderRegistry.lookup — returns a one-argument lambda (payload: unknown) => entry.sender(...) that silently drops the second argument. The test passes because it uses a mock that directly captures context, bypassing the registry entirely.
Failure scenario: any downstream consumer relying on session-aware routing of reverse MCP messages will find the sessionId absent at the WS client layer, because JavaScript silently ignores the extra argument on the production sender.
| const response = await send( | |
| payload, | |
| typeof sessionId === 'string' ? { sessionId } : undefined, | |
| ); | |
| const response = await send( | |
| payload, | |
| typeof sessionId === 'string' ? { sessionId } : undefined, | |
| ); // NOTE: production sender (ClientMcpSenderRegistry.lookup) currently ignores context |
Consider updating ClientMcpSenderRegistry.lookup to forward the context parameter and updating WsClientMcpSender to accept it.
中文说明
sessionId 经过校验后传给 send()，但唯一的生产环境 sender（ClientMcpSenderRegistry.lookup）返回的是单参数 lambda (payload: unknown) => entry.sender(...)，会静默丢弃第二个参数。测试通过是因为 mock 直接捕获了 context，绕过了 registry。
失败场景：任何依赖反向 MCP 消息中 session 路由的下游消费者，在 WS 客户端层会发现 sessionId 不存在。
建议更新 ClientMcpSenderRegistry.lookup 以转发 context 参数，并更新 WsClientMcpSender 以接收它。
— qwen3.7-max via Qwen Code /review
| async addSessionRuntimeMcpServer( | ||
| sessionId, | ||
| name, | ||
| config, | ||
| originatorClientId, | ||
| ) { | ||
| return requestSessionStatus<RuntimeMcpServerAddResult>( | ||
| sessionId, | ||
| SERVE_CONTROL_EXT_METHODS.sessionMcpRuntimeAdd, | ||
| { name, config, originatorClientId }, | ||
| ); | ||
| }, |
There was a problem hiding this comment.
[Suggestion] Both new session-scoped methods route through requestSessionStatus, which uses initTimeoutMs (default 10s). The workspace-scoped addRuntimeMcpServer / removeRuntimeMcpServer above use MCP_RESTART_SERVER_DEADLINE_MS (300s / 5 min) because stdio transports that spawn a child process can take that long.
Failure scenario: a session-scoped add with a stdio transport taking 15–30s to spawn would hit the 10s timeout and fail, while the identical operation through the workspace path succeeds. The tests only exercise type: 'sdk' (in-process, sub-second), so this gap is invisible in CI.
Consider adding an optional timeoutMs parameter to requestSessionStatus and passing MCP_RESTART_SERVER_DEADLINE_MS from both new methods.
中文说明
两个新的 session-scoped 方法都通过 requestSessionStatus 路由，该方法使用 initTimeoutMs（默认 10 秒）。上方 workspace-scoped 的 addRuntimeMcpServer/removeRuntimeMcpServer 使用 MCP_RESTART_SERVER_DEADLINE_MS（300 秒），因为 stdio 传输需要启动子进程，可能需要那么长时间。
失败场景：session-scoped add 使用 stdio 传输启动耗时 15-30 秒时，会在 10 秒超时后失败，而相同的操作通过 workspace 路径可以成功。测试只覆盖了 type: 'sdk'（进程内，亚秒级），所以这个差距在 CI 中不可见。
建议给 requestSessionStatus 添加可选的 timeoutMs 参数，并在两个新方法中传入 MCP_RESTART_SERVER_DEADLINE_MS。
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
已修复：session-scoped MCP add/remove 现在与 workspace 路径一致使用 300s MCP restart deadline，并新增回归测试覆盖操作超过 initialize timeout 后仍可完成。验证证据：cd packages/acp-bridge && npx vitest run src/bridge.test.ts（450/450 passed）；cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts -t "mutates runtime MCP state on only the selected live session"（1/1 passed）；npm run build --workspace=@qwen-code/acp-bridge、npm run typecheck --workspace=@qwen-code/acp-bridge、Prettier 与 git diff --check 均通过。Commit: f57b02307c。
| [CLIENT_MCP_OVER_WS_CONFIG_FLAG]: clientMcpOverWs, | ||
| ...safeConfig | ||
| } = config as ClientMcpOverWsRuntimeConfig; |
There was a problem hiding this comment.
[Suggestion] The refactoring into readRuntimeMcpAddRequest lost two architectural comments from the original inline code:
- Why
typeis stripped and restored as'sdk'only forclientMcpOverWs: withouttype: 'sdk', the manager won't bindSdkControlClientTransportwhosesendMcpMessageroutes back over the daemon WS — the reverse tool channel silently fails. - Why the workspace add handler fans out to active sessions: per-session config isolation means an already-active session would not see a server added only to the bootstrap config, and
tools/callwould fail with "not found in registry."
Failure scenario: a future maintainer "simplifies" by removing the type: 'sdk' override or the fan-out, both changes compile and pass tests, but silently break the reverse tool channel for client-hosted MCP servers.
Consider restoring concise versions of both comments in their new locations. The adjacent workspaceMcpRuntimeRemove still references "the add fan-out (#5626)" but the add side no longer explains its own rationale.
中文说明
重构为 readRuntimeMcpAddRequest 时丢失了原始内联代码中的两条架构注释：
- 为什么
type被无条件剥离，仅在clientMcpOverWs时恢复为'sdk'：没有type: 'sdk'，manager 不会绑定SdkControlClientTransport，其sendMcpMessage通过 daemon WS 路由——反向工具通道会静默失败。 - 为什么 workspace add 需要扇出到活跃 session：每个 session 有独立的 config，已在运行的 session 不会看到只添加到 bootstrap config 的 server，
tools/call会报 "not found in registry"。
建议在新位置恢复这两条注释的精简版本。相邻的 workspaceMcpRuntimeRemove 仍然引用 "the add fan-out (#5626)"，但 add 侧已不再解释自己的理由。
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
|
已修复：将 session MCP 超时回归测试中的异步断言改为直接 await，修复 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
中文说明
— qwen3.7-max via Qwen Code /review
Maintainer verification — real local build & tests @
|
| Gate | Result |
|---|---|
node scripts/lint.js --eslint (full repo) |
✅ exit 0 |
packages/acp-bridge — bridgeClient.test.ts + bridge.test.ts |
✅ 528 passed (2 files) |
packages/cli — acp-integration/acpAgent.test.ts |
✅ 318 passed |
npm run typecheck --workspace=packages/acp-bridge |
✅ exit 0 |
npm run typecheck --workspace=packages/cli |
✅ exit 0 |
The CI blocker is real and is fixed. I reproduced the red Test (ubuntu-latest, Node 22.x) failure at f57b023 byte-for-byte locally (12006:27 / 12022:30 vitest/valid-expect), and confirmed 51eba8ea4 clears it. Note 51eba8ea4 touches only bridge.test.ts — production source is identical to f57b023, so the earlier green build/typecheck signal still holds.
2. Mutation matrix — 8/8 killed
I neutered each new behaviour one at a time and confirmed a named test dies. No survivors, no vacuity:
| # | Neutered behaviour | Verdict |
|---|---|---|
| M1 | session add/remove reverts to initializeTimeout |
🔴 KILLED |
| M2 | drop originatorClientId from the wire params |
🔴 KILLED |
| M3 | drop the ownsSession ownership check |
🔴 KILLED |
| M4 | stop forwarding session context to the sender | 🔴 KILLED |
| M5 | accept an empty-string sessionId |
🔴 KILLED |
| M6 | session add targets the workspace Config | 🔴 KILLED |
| M7 | stop stripping trust/cwd/creds from the runtime config |
🔴 KILLED |
| M8 | session remove targets the workspace Config | 🔴 KILLED |
Worth calling out: M1 stays killed after the ESLint fix. Restructuring expect(...).resolves into a bare promise + trailing await expect(...) is the kind of edit that silently unpins a timing assertion; it didn't here.
3. Finding — the ownership check fires during the session pre-registration window
I wrote a differential probe that drives the real createAcpSessionBridge + BridgeClient and swapped only the five acp-bridge source files between base and head (probe file byte-identical in both runs):
| Probe | base 7f8adc659 |
head 51eba8ea4 |
|---|---|---|
| P2 frame sent after the session is registered | delivered, context = undefined |
delivered, context = { sessionId } ✅ |
| P3 frame claiming a foreign session id | leaked — sender invoked ×1 | rejected — sender invoked ×0 ✅ |
P1 frame sent from inside the child's newSession |
delivered — sender invoked ×1 | -32602 — sender invoked ×0 |
P2 and P3 confirm the PR does exactly what it says. P1 is the concern.
Why the window exists. The parent registers a session id only after connection.newSession() resolves (bridge.ts:2601 → createSessionEntry → bridge.ts:3865 ci.sessionIds.add(...)), and ownsSession is exactly (id) => sessionIds.has(id) (bridge.ts:2238). Meanwhile — all of this pre-exists on main — the child already stamps sessionId on every per-session reverse frame (acpAgent.ts:10208 → 2640), and a new session inherits the workspace's runtime MCP servers before config.initialize(), with the comment stating this exists so "its discovery pass picks them up and binds THIS session's sendSdkMcpMessage" (acpAgent.ts:10476, bind at :10533). startMcpDiscoveryInBackground() fires inside initialize() (config.ts:2811) and SdkControlClientTransport.start() is a no-op (sdk-control-client-transport.ts:67) — so for a client-hosted server there is no spawn latency before the first frame goes out.
The repo already documents this exact ordering, in bridgeClient.ts:2097:
"The frames were captured before the entry existed (e.g. MCP discovery during the child's
newSessionhandler)"
And the sibling guards handle it: sessionUpdate (:789) and handleArtifactEvent (:1685) both carry an inFlightRestoreIds escape hatch, and unowned notifications fall through to bufferEarlyEvent/drainEarlyEvents. The new check at bridgeClient.ts:1313 has no equivalent — and because client_mcp/message is a request, buffering isn't available; it just hard-fails, and the client-hosted server's tools silently never register for that session (surfaceMcpFailuresWhenReady only logs).
Scope, stated honestly: the A/B above is a deterministic proof of the guard's behaviour on the real bridge. The production reachability is a code-path argument (plus the repo's own comment), not a live-daemon capture — I did not stand up qwen serve with a real extension-hosted MCP server. So this is a "please confirm", not a "this is broken in prod".
Possible resolutions (your call which fits the follow-up):
- Reject the identity, not the frame — when
ownsSession(sessionId)is false, dropsessionIdand forward withcontext: undefined+ a[demux]stderr line. That is exactly pre-PR behaviour for unverifiable ids, keeps a forged id from ever reaching the sender as authenticated context, and can't break discovery. Costs a change to the "rejects a session id owned by another ACP channel" assertion. - Narrow the window — track an in-flight
newSessioncount per channel (the analogue ofmarkRestoreInFlight) and accept an unknown id only while one is outstanding. Tighter, slightly more machinery. - Confirm it's unreachable — if the channel-loop follow-up never creates a session while a client-hosted server is registered at the workspace level, this is moot; say so and I'll drop it.
4. Minor
The qwen-code-ci-bot nit is still open: readRuntimeMcpAddRequest lost the rationale for why clientMcpOverWs === true forces type: 'sdk' (the SdkControlClientTransport binding, #5626 Phase 2). One line would restore it. Non-blocking.
Verdict
Implementation, tests and CI hygiene are all in good shape — this is a clean, well-tested isolation primitive and the mutation results are better than most PRs I check. I'd like §3 answered before merge, but if the answer is "unreachable" or you take option 1, this is ready.
中文版本
Maintainer 验证 —— 本地真实构建与测试 @ 51eba8ea4
在基于 merge-base（7f8adc659）的隔离 worktree 中构建并运行了本 PR，macOS 24.6.0 / Node 22.23.1。Reviewer Test Plan 中列出的所有 gate 全部通过，ESLint CI 阻塞已真正修复，8 个变异探针全部被杀死 —— 新增测试是真正起作用的，不是摆设。
我发现一处建议在合并前确认的问题：反向 client-MCP 路径上新增的 ownsSession 校验，会在一个"session 尚未注册"的时间窗内触发，而代码库其他地方对这个窗口是显式容忍的。详见 §3。
1. Gate 结果
| Gate | 结果 |
|---|---|
node scripts/lint.js --eslint（全仓库） |
✅ exit 0 |
packages/acp-bridge —— bridgeClient.test.ts + bridge.test.ts |
✅ 528 passed（2 个文件） |
packages/cli —— acp-integration/acpAgent.test.ts |
✅ 318 passed |
npm run typecheck --workspace=packages/acp-bridge |
✅ exit 0 |
npm run typecheck --workspace=packages/cli |
✅ exit 0 |
CI 阻塞是真实的，也确实修好了。 我在本地逐字复现了 f57b023 上 Test (ubuntu-latest, Node 22.x) 的红色失败（12006:27 / 12022:30 vitest/valid-expect），并确认 51eba8ea4 已清除。注意 51eba8ea4 只改动了 bridge.test.ts，生产代码与 f57b023 完全一致，因此此前的 build/typecheck 绿色信号依然有效。
2. 变异矩阵 —— 8/8 被杀死
我逐个"废掉"每项新行为，确认都有具名测试因此失败。无幸存者，无空测试：
| # | 被废掉的行为 | 判定 |
|---|---|---|
| M1 | session add/remove 回退到 initializeTimeout |
🔴 KILLED |
| M2 | 从 wire params 中去掉 originatorClientId |
🔴 KILLED |
| M3 | 去掉 ownsSession 归属校验 |
🔴 KILLED |
| M4 | 不再向 sender 转发 session context | 🔴 KILLED |
| M5 | 接受空字符串 sessionId |
🔴 KILLED |
| M6 | session add 打到 workspace Config 上 | 🔴 KILLED |
| M7 | 不再剥离 trust/cwd/凭据等敏感运行时配置 |
🔴 KILLED |
| M8 | session remove 打到 workspace Config 上 | 🔴 KILLED |
特别值得一提：M1 在 ESLint 修复之后仍然被杀死。 把 expect(...).resolves 改写成裸 promise + 末尾 await expect(...)，这类改动很容易悄悄让时序断言失去约束力 —— 这次没有。
3. 发现 —— 归属校验会在 session 注册前的时间窗内触发
我写了一个差分探针，驱动真实的 createAcpSessionBridge + BridgeClient，两次运行之间只替换 acp-bridge 的 5 个源文件（探针文件逐字节相同）：
| 探针 | base 7f8adc659 |
head 51eba8ea4 |
|---|---|---|
| P2 session 注册之后发送的 frame | 送达，context = undefined |
送达，context = { sessionId } ✅ |
| P3 声称属于其他 channel 的 session id | 泄漏 —— sender 被调用 ×1 | 拒绝 —— sender 被调用 ×0 ✅ |
P1 从子进程 newSession 内部发出的 frame |
送达 —— sender 被调用 ×1 | -32602 —— sender 被调用 ×0 |
P2 与 P3 证实本 PR 确实做到了它声称的事。P1 是需要关注的部分。
这个窗口为什么存在。 父进程只在 connection.newSession() 返回之后才注册 session id（bridge.ts:2601 → createSessionEntry → bridge.ts:3865 ci.sessionIds.add(...)），而 ownsSession 正是 (id) => sessionIds.has(id)（bridge.ts:2238）。与此同时 —— 以下全部是 main 上既有的代码 —— 子进程已经会在每个 per-session 反向 frame 上打上 sessionId（acpAgent.ts:10208 → 2640），并且新 session 会在 config.initialize() 之前继承 workspace 的运行时 MCP server，注释明确写着这样做是为了让"其 discovery pass 能拿到它们并绑定本 session 的 sendSdkMcpMessage"（acpAgent.ts:10476，绑定在 :10533）。startMcpDiscoveryInBackground() 是在 initialize() 内部触发的（config.ts:2811），而 SdkControlClientTransport.start() 是空操作（sdk-control-client-transport.ts:67）—— 所以对 client-hosted server 来说，第一帧发出之前没有任何进程启动延迟。
仓库自己就记录了这个顺序，见 bridgeClient.ts:2097：
"The frames were captured before the entry existed (e.g. MCP discovery during the child's
newSessionhandler)"
而且同类守卫都处理了它：sessionUpdate（:789）和 handleArtifactEvent（:1685）都带有 inFlightRestoreIds 逃生口，未归属的通知还会走 bufferEarlyEvent/drainEarlyEvents。新增的 bridgeClient.ts:1313 校验没有对应机制 —— 而且因为 client_mcp/message 是 request 而非通知，缓冲这条路走不通，它只会直接失败，导致该 session 的 client-hosted server 工具静默注册不上（surfaceMcpFailuresWhenReady 只会打日志）。
关于结论范围，如实说明： 上面的 A/B 是在真实 bridge 上对校验行为本身的确定性证明。生产可达性是代码路径推导（外加仓库自身的注释），不是 live daemon 抓取 —— 我没有真的起一个带真实 extension-hosted MCP server 的 qwen serve。所以这是"请确认"，不是"线上已经坏了"。
可选处理方式（取决于后续 PR 的设计）：
- 拒绝身份，而不是拒绝整个 frame —— 当
ownsSession(sessionId)为 false 时，丢弃sessionId，以context: undefined转发并打一条[demux]stderr 日志。这与 PR 之前对不可验证 id 的行为完全一致，既能保证伪造的 id 永远不会作为可信 context 到达 sender，也不可能破坏 discovery。代价是需要改动 "rejects a session id owned by another ACP channel" 这条断言。 - 收窄窗口 —— 按 channel 记录在途
newSession计数（markRestoreInFlight的对应物），只在有在途创建时才接受未知 id。更严格，机制稍多。 - 确认不可达 —— 如果 channel-loop 后续 PR 永远不会在 workspace 级别已注册 client-hosted server 的情况下创建 session，那这条就不成立，告诉我即可，我撤回。
4. 次要
qwen-code-ci-bot 提的小问题仍未处理：readRuntimeMcpAddRequest 丢失了"为什么 clientMcpOverWs === true 要强制 type: 'sdk'"的说明（即 SdkControlClientTransport 绑定，#5626 Phase 2）。补一行注释即可。非阻塞。
结论
实现、测试和 CI 卫生状况都很好 —— 这是一个干净、测试充分的隔离原语，变异测试结果比我检查过的大多数 PR 都好。我希望 §3 在合并前有个答复；如果答复是"不可达"或采用方案 1，就可以合并了。
|
已修复：仅在同一 ACP channel 的 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
Maintainer re-verification — real local build & tests @
|
| R1 finding | Severity | Status @ e5365cc203 |
|---|---|---|
§3 ownsSession rejects reverse client-MCP frames emitted during the session pre-registration window |
✅ Resolved by e5365cc203. Re-tested with the identical probe — see §2. |
|
§4 readRuntimeMcpAddRequest lost the rationale for why clientMcpOverWs === true forces type: 'sdk' |
🔹 nit | ⚪ Still open. Re-checked at this head (acpAgent.ts:2692-2709): the base's 6-line SdkControlClientTransport / #5626 explanation is still gone. Non-blocking, but two reviewers have now flagged it. |
1. Gates
| Gate | Result |
|---|---|
packages/acp-bridge — bridgeClient.test.ts + bridge.test.ts |
✅ 530 passed (was 528 at R1; the fix adds 2) |
packages/cli — acp-integration/acpAgent.test.ts |
✅ 318 passed |
node scripts/lint.js --eslint (full repo) |
✅ exit 0 |
npm run typecheck --workspace=packages/acp-bridge |
✅ exit 0 |
npm run typecheck --workspace=packages/cli |
✅ exit 0 |
npm run build --workspace=packages/{core,acp-bridge,cli} |
✅ exit 0 — forwarded_without_session verified present in the emitted dist |
Two honesty notes on that table:
- The lint pass is a real pass. I planted a
console.loginbridgeClient.tsas a positive control: eslint returned exit 1 /no-console, and exit 0 again once restored. An exit-0 from a silently-broken linter is not evidence. - The full
npm run builddoes not complete in my tree, stopping inpackages/webuiwithCannot find package 'vite-plugin-dts'. This is environmental, and I verified rather than assumed it: the base commit7f8adc659fails identically in the same tree, and the PR touches 9 ACP source files and zeropackage.json/lockfile entries. The three packages the PR actually changes all build clean.
2. The §3 finding is fixed — 3-arm differential
Same probe as R1 (byte-identical file, sha256 f196bec4…), driving the real createAcpSessionBridge + BridgeClient; only the five acp-bridge production sources are swapped between arms. P4 and P5 are new this round and target the fix itself.
| Probe | base 7f8adc659 |
R1 51eba8ea4 |
R2 e5365cc20 |
|---|---|---|---|
P1 discovery frame from inside the child's newSession |
delivered, no context | ✗ -32602, sender ×0 |
✅ delivered, no context, sender ×1 |
| P2 frame after registration (positive control) | delivered, no context | ✅ { sessionId } |
✅ { sessionId } |
| P3 foreign id outside the window (security control) | ✗ leaked, sender ×1 | ✅ rejected -32602 |
✅ rejected -32602 |
| P4 forged id inside the window (new) | forwarded, not trusted | rejected -32602 |
✅ forwarded, not trusted |
P5 foreign id after a failed newSession (new) |
✗ leaked, sender ×1 | ✅ rejected -32602 |
✅ rejected -32602 |
Reading. The fix is the hybrid of options 1 and 2 from my last comment, and it lands well:
- It narrows the window rather than removing the check —
hasSessionSpawnInFlight()(bridgeClient.ts:1320) readschannelInfo.sessionSpawnsInFlight(bridge.ts:2285), a counter that already exists onmain, incremented atbridge.ts:2581beforeconnection.newSession()and decremented atbridge.ts:2849inside afinally. - Inside the window it rejects the identity, not the frame —
context: undefinedplus a[demux] … action=forwarded_without_session reason=session_registration_pendingaudit line (bridgeClient.ts:1329).
The two rows that matter most for merge confidence:
- P4 is the only cell where R2 is more permissive than R1, and the forged id reaches the sender with
context: undefined— never as authenticated context (forged-id-trusted = false). So the window is a routing relaxation, not a trust relaxation. That is exactly pre-PR behaviour for an unverifiable id. - P5 confirms the
finallydecrement empirically. I drove anewSessionthat throws on a channel that survives (first session still live,sessionScope: 'thread'): afterwards a foreign id is rejected-32602again and the owned session still gets{ sessionId }. A leaked counter would have left the ownership check permanently disabled for that channel; it does not.
Net: R2 restores base behaviour at exactly the one place base was permissive-and-safe (P1), and keeps every hardening R1 introduced (P3, P5).
3. Mutation matrix — 13 probes, 12 killed
The 8 R1 mutants re-run at this head (anchors refreshed for M3/M4), plus 5 new ones aimed at the fix commit. Each kill is attributed to a named test, not just a count.
| # | Behaviour neutered | Verdict | Killing test |
|---|---|---|---|
| M1 | session add/remove reverts to initializeTimeout |
🔴 KILLED | allows session MCP mutations to outlive the initialize timeout |
| M2 | drop originatorClientId from the wire params |
🔴 KILLED | routes add and remove to the selected live session only |
| M3 | drop the ownsSession check entirely |
🔴 KILLED | 3 tests, incl. rejects a session id owned by another ACP channel |
| M4 | stop forwarding session context to the sender | 🔴 KILLED | forwards the originating session id to the client MCP sender |
| M5 | accept an empty-string sessionId |
🔴 KILLED | rejects a malformed optional session id |
| M6 | session add targets the workspace Config | 🔴 KILLED | mutates runtime MCP state on only the selected live session |
| M7 | stop stripping trust/cwd/creds |
🔴 KILLED | mutates runtime MCP state on only the selected live session |
| M8 | session remove targets the workspace Config | 🔴 KILLED | mutates runtime MCP state on only the selected live session |
| M9 | remove the spawn-window escape hatch (revert to R1) | 🔴 KILLED | allows client MCP discovery during session pre-registration only |
| M10 | trust the unowned id inside the window | 🔴 KILLED | forwards a pre-registration session frame without trusted context |
| M11 | drop the per-channel identity guard on the counter | 🟡 SURVIVED | — see §4 |
| M12 | drop the [demux] audit line |
🔴 KILLED | forwards a pre-registration session frame without trusted context |
| M13 | make the hasSessionSpawnInFlight default fail-open |
🔴 KILLED | rejects a session id owned by another ACP channel |
M9/M10 are the important pair: they prove the two halves of the fix are independently pinned — you cannot revert to the strict reject, and you cannot start trusting the unowned id, without a named test going red. M13 confirms the default is fail-closed, which matters for the callers that don't pass the closure.
4. The one survivor — M11, adjudicated
bridge.ts:2284 opens the window with channelInfo?.sessionIds === sessionIds && channelInfo.sessionSpawnsInFlight > 0. Dropping the sessionIds identity comparison survives the whole suite.
I checked this against an independent oracle rather than by reading the code, and I do not think it should block:
- The two only diverge during the kill-then-respawn channel overlap (A dying, B freshly spawned as
channelInfo). I tried to drive it and could not: a bridge is bound to one workspace and refuses a second outright ("Workspace mismatch: runtime is bound to/work/abut request asked for/work/b… this bridge will not fall back"), so a second channel never coexists that way. - The repo already documents this as untestable —
bridge.ts:4722-4732carries aHAZARDnote on the same overlap: "The regression test is single-channel smoke only and WILL NOT fail if this reverts to module-scopedchannelInfo. KeepchannelInfoForEntry(entry)until a deterministic overlap test lands." M11 surviving is that same pre-existing gap, not something this PR introduced. - Impact if it were dropped is bounded anyway: even in the overlap the frame is forwarded with
contextstripped, so there is still no trust escalation.
So: a correctly conservative defensive guard, uncovered for a documented reason. Worth a comment pointing at the HAZARD note if you want it findable later.
Scope, stated honestly
As in R1, this is a deterministic proof at the bridge level — real createAcpSessionBridge, real BridgeClient, real reverse ext-method. I did not stand up qwen serve with a real extension-hosted MCP server, so production reachability of the original window remains a code-path argument (backed by the repo's own comment at bridgeClient.ts:2107-2112). That caveat applied to the finding; it applies equally to the fix.
Verdict
Merge-ready from my side. The §3 concern is properly closed — narrowed window, identity rejected instead of the frame, an audit line for the forwarded case, and two new tests that are load-bearing under mutation. Gates are green, 12/13 mutants die to named assertions, and the one survivor is a pre-existing documented testing gap over correct code.
Only leftover is the §4 documentation nit, which is non-blocking and can ride along here or in the channel-loop follow-up.
中文版本
Maintainer 复验 —— 本地真实构建与测试 @ e5365cc203
第 2 轮，承接我在 51eba8ea4 的报告。此后的增量是一个 commit —— e5365cc203 fix(acp): allow MCP discovery during session spawn —— 正是针对我提出的 §3 问题。在基于 merge-base（7f8adc659）的隔离 worktree 中重跑，macOS 24.6.0 / Node 22.23.1。
上一轮的每一条发现都重新实测过，没有凭信任直接沿用。
上一轮 → 本 head 的状态
| R1 发现 | 级别 | e5365cc203 上的状态 |
|---|---|---|
§3 ownsSession 会拒绝在 session 注册前时间窗内发出的反向 client-MCP frame |
✅ 已解决，由 e5365cc203 修复。用同一探针重新实测 —— 见 §2。 |
|
§4 readRuntimeMcpAddRequest 丢失了「为何 clientMcpOverWs === true 要强制 type: 'sdk'」的说明注释 |
🔹 小问题 | ⚪ 仍未处理。 在本 head 重新核对（acpAgent.ts:2692-2709）：base 中那 6 行关于 SdkControlClientTransport / #5626 的解释依然缺失。非阻塞，但现在已有两位 reviewer 提到。 |
1. Gate 结果
| Gate | 结果 |
|---|---|
packages/acp-bridge —— bridgeClient.test.ts + bridge.test.ts |
✅ 530 passed（R1 时为 528；本次修复新增 2 个） |
packages/cli —— acp-integration/acpAgent.test.ts |
✅ 318 passed |
node scripts/lint.js --eslint（全仓库） |
✅ exit 0 |
npm run typecheck --workspace=packages/acp-bridge |
✅ exit 0 |
npm run typecheck --workspace=packages/cli |
✅ exit 0 |
npm run build --workspace=packages/{core,acp-bridge,cli} |
✅ exit 0 —— 已确认 forwarded_without_session 出现在产出的 dist 中 |
关于上表的两点如实说明：
- lint 的绿色是真绿。 我在
bridgeClient.ts中植入了一个console.log作为阳性对照：eslint 返回 exit 1 /no-console，恢复后重新 exit 0。一个静默失效的 linter 返回 exit 0 不能算证据。 - 完整的
npm run build在我的环境里跑不完，卡在packages/webui的Cannot find package 'vite-plugin-dts'。这是环境问题，而且我是验证而非假定：base commit7f8adc659在同一棵树上以完全相同的方式失败，且本 PR 只改动 9 个 ACP 源文件、零个package.json/lockfile。PR 真正改动的三个 package 全部构建通过。
2. §3 已修复 —— 三臂差分
与 R1 完全相同的探针（文件逐字节一致，sha256 f196bec4…），驱动真实的 createAcpSessionBridge + BridgeClient；各臂之间只替换 acp-bridge 的 5 个生产源文件。P4、P5 是本轮新增，专门针对这次修复本身。
| 探针 | base 7f8adc659 |
R1 51eba8ea4 |
R2 e5365cc20 |
|---|---|---|---|
P1 从 child 的 newSession 内部发出的 discovery frame |
送达，无 context | ✗ -32602，sender ×0 |
✅ 送达，无 context，sender ×1 |
| P2 注册之后的 frame（阳性对照） | 送达，无 context | ✅ { sessionId } |
✅ { sessionId } |
| P3 窗口之外的外部 session id（安全对照） | ✗ 泄漏，sender ×1 | ✅ 拒绝 -32602 |
✅ 拒绝 -32602 |
| P4 窗口之内的伪造 id（新增） | 转发，未被信任 | 拒绝 -32602 |
✅ 转发，未被信任 |
P5 newSession 失败后的外部 id（新增） |
✗ 泄漏，sender ×1 | ✅ 拒绝 -32602 |
✅ 拒绝 -32602 |
解读。 这次修复是我上一条评论中方案 1 与方案 2 的结合，落地得很好：
- 它收窄了窗口而不是移除校验 ——
hasSessionSpawnInFlight()（bridgeClient.ts:1320）读取channelInfo.sessionSpawnsInFlight（bridge.ts:2285），这个计数器在main上已经存在，在bridge.ts:2581调用connection.newSession()之前自增，并在bridge.ts:2849的finally内自减。 - 在窗口内它拒绝的是身份而不是 frame ——
context: undefined，外加一行[demux] … action=forwarded_without_session reason=session_registration_pending审计日志（bridgeClient.ts:1329）。
对合并信心最关键的两行：
- P4 是 R2 唯一比 R1 更宽松的格子，而伪造的 id 到达 sender 时
context: undefined—— 绝不会作为已认证身份传入（forged-id-trusted = false）。因此这个窗口是路由上的放宽，不是信任上的放宽。这恰恰是 PR 之前对不可验证 id 的行为。 - P5 用实测确认了
finally中的自减。 我在一个存活的 channel 上驱动了一次抛异常的newSession（第一个 session 仍然活着，sessionScope: 'thread'）：之后外部 id 重新被-32602拒绝，而属于自己的 session 仍然拿到{ sessionId }。如果计数器泄漏，该 channel 的归属校验会被永久关闭；实际并没有。
总体：R2 在 base 原本「宽松但安全」的那一处（P1）恢复了 base 行为，同时保留了 R1 引入的全部加固（P3、P5）。
3. 变异矩阵 —— 13 个探针，杀死 12 个
R1 的 8 个变异体在本 head 重跑（M3/M4 的锚点已更新），外加 5 个针对修复 commit 的新变异体。每次「杀死」都归因到具名测试，而不只是一个计数。
| # | 被废掉的行为 | 判定 | 杀死它的测试 |
|---|---|---|---|
| M1 | session add/remove 回退到 initializeTimeout |
🔴 KILLED | allows session MCP mutations to outlive the initialize timeout |
| M2 | 从 wire params 去掉 originatorClientId |
🔴 KILLED | routes add and remove to the selected live session only |
| M3 | 完全去掉 ownsSession 校验 |
🔴 KILLED | 3 个测试，含 rejects a session id owned by another ACP channel |
| M4 | 不再向 sender 转发 session context | 🔴 KILLED | forwards the originating session id to the client MCP sender |
| M5 | 接受空字符串 sessionId |
🔴 KILLED | rejects a malformed optional session id |
| M6 | session add 打到 workspace Config | 🔴 KILLED | mutates runtime MCP state on only the selected live session |
| M7 | 不再剥离 trust/cwd/凭据 |
🔴 KILLED | mutates runtime MCP state on only the selected live session |
| M8 | session remove 打到 workspace Config | 🔴 KILLED | mutates runtime MCP state on only the selected live session |
| M9 | 移除 spawn 窗口的放行通道（回退到 R1） | 🔴 KILLED | allows client MCP discovery during session pre-registration only |
| M10 | 在窗口内信任未归属的 id | 🔴 KILLED | forwards a pre-registration session frame without trusted context |
| M11 | 去掉计数器上的按 channel 身份校验 | 🟡 存活 | —— 见 §4 |
| M12 | 去掉 [demux] 审计日志 |
🔴 KILLED | forwards a pre-registration session frame without trusted context |
| M13 | 把 hasSessionSpawnInFlight 默认值改成 fail-open |
🔴 KILLED | rejects a session id owned by another ACP channel |
M9/M10 是关键的一对：它们证明修复的两个部分被独立地钉住了 —— 你既不能退回严格拒绝，也不能开始信任未归属的 id，否则都会有具名测试变红。M13 确认默认值是 fail-closed，这对那些不传该闭包的调用方很重要。
4. 唯一的存活者 —— M11 的裁定
bridge.ts:2284 用 channelInfo?.sessionIds === sessionIds && channelInfo.sessionSpawnsInFlight > 0 打开窗口。去掉 sessionIds 身份比较后，整个测试套件依然全绿。
我用独立的判据而非「读代码」来裁定这一点，我认为它不应阻塞合并：
- 两者只有在 kill-then-respawn 的 channel 重叠窗口（A 正在消亡、B 刚被 spawn 成为
channelInfo）中才会分歧。我尝试驱动但没能做到：一个 bridge 绑定单一 workspace，并会直接拒绝第二个（"Workspace mismatch: runtime is bound to/work/abut request asked for/work/b… this bridge will not fall back"），所以第二个 channel 根本不会以那种方式并存。 - 仓库自己已经把这一点记为不可测 ——
bridge.ts:4722-4732针对同一个重叠窗口写着HAZARD注释："The regression test is single-channel smoke only and WILL NOT fail if this reverts to module-scopedchannelInfo. KeepchannelInfoForEntry(entry)until a deterministic overlap test lands." M11 的存活正是这个既有缺口，不是本 PR 引入的。 - 即便真的去掉，影响也有限：在重叠窗口中 frame 仍然是在剥离
context后转发的，因此依然不存在信任提升。
结论：一个正确且保守的防御性校验，因既有且已被记录的原因而未被覆盖。如果希望以后能被找到，值得加一行注释指向那段 HAZARD 说明。
范围（如实说明）
与 R1 一样，这是 bridge 层的确定性证明 —— 真实的 createAcpSessionBridge、真实的 BridgeClient、真实的反向 ext-method。我没有起一个带真实 extension-hosted MCP server 的 qwen serve，因此原始窗口在生产中的可达性仍然是一个代码路径论证（由仓库自身在 bridgeClient.ts:2107-2112 的注释支持）。这个限定适用于当初的发现，同样也适用于这次的修复。
结论
从我这边看可以合并。 §3 的问题被妥善关闭了 —— 收窄窗口、拒绝身份而非 frame、为转发情形留下审计日志，以及两个在变异测试下真正起作用的新测试。各项 gate 全绿，13 个变异体中 12 个被具名断言杀死，唯一的存活者是既有且已被记录的测试缺口，其覆盖的代码本身是正确的。
剩下的只有 §4 的文档小问题，非阻塞，可以在本 PR 顺带处理，也可以放到 channel-loop 的后续 PR。
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.8-max-preview via Qwen Code /review
Maintainer re-verification — real local build & tests @
|
| Gate | Result |
|---|---|
packages/acp-bridge — bridge.test.ts + bridgeClient.test.ts |
✅ 530 passed (2 files) |
packages/cli — acp-integration/acpAgent.test.ts |
✅ 318 passed |
npm run typecheck --workspace=packages/acp-bridge |
✅ exit 0 |
npm run typecheck --workspace=packages/cli |
✅ 0 errors in src/acp-integration (see note) |
npx eslint — all 9 changed files |
✅ exit 0 |
npx prettier --check — all 9 changed files |
✅ exit 0 |
git diff --check |
✅ exit 0 |
build — packages/core + the cli's full workspace dep closure |
✅ 13 / 13 packages OK |
Suite delta against the merge-base: 522 → 530 tests, all 8 new ones pass and no existing test changed verdict. The ESLint blocker from the previous round stays fixed.
Two environment notes, so the numbers above are reproducible and the ones I could not get are stated rather than hidden. This box's shared install is 45 deps behind
main, so (a) a whole-monoreponpm run buildcannot run here — I builtpackages/coreplus the cli's entire workspace dependency closure instead, 13/13 OK; (b)packages/clitypecheck still reports 49 errors, all of them insrc/ui/**fromink/difftype skew, and zero insrc/acp-integration/**— this PR touches nosrc/uifile. Separately, before I builtpackages/corein the worktree,bridge.test.tsshowed 2 failures (forwards childEnvOverrides…,rebuilds authoritative invocation metadata…) that fail identically on the merge-base:PRIVATE_ACP_CAPABILITY_ENVresolved toundefined, so the env var landed under a literal"undefined"key. Building core clears both.
2. The fix behaves as advertised
I re-ran the same differential probe as last time — a byte-identical file (md5 305ea215…) driving the real createAcpSessionBridge + BridgeClient, with only packages/acp-bridge/src differing between columns — and extended it to the paths the fix did not touch.
| Probe | base 7f8adc659 |
head e5365cc203 |
|---|---|---|
P1 frame from inside the child's newSession |
delivered, context = undefined |
delivered, context stripped ✅ |
| P2 frame after the session is registered | delivered, context = undefined |
delivered, context = { sessionId } ✅ |
| P3 frame claiming a foreign id, no window open | leaked to the sender | -32602, sender not called ✅ |
| P6 frame with no session id (legacy sender) | delivered, context = undefined |
unchanged ✅ |
P1 is the finding from my last review, now closed exactly the way I hoped — the identity is rejected, the frame is not. forwarded_without_session on stderr is a nice touch; it makes the window observable instead of silent.
3. The same window is still closed on loadSession and unstable_resumeSession
| Probe | base 7f8adc659 |
head e5365cc203 |
head + patch below |
|---|---|---|---|
P4 frame from inside the child's loadSession |
delivered | -32602 |
delivered, context stripped |
P5 frame from inside the child's unstable_resumeSession |
delivered | -32602 |
delivered, context stripped |
Why. The parent opens the window in exactly one place — ci.sessionSpawnsInFlight++ in doSpawn (bridge.ts:2581, released in the finally at :2849, so it does span the whole spawn). But the parent has three session-creating ACP calls, and the other two never touch that counter:
bridge.ts |
ACP call | opens the window? |
|---|---|---|
:2604 |
ci.connection.newSession(…) |
✅ yes |
:4427 |
ci.connection.loadSession(…) |
❌ no |
:4458 |
ci.connection.unstable_resumeSession(…) |
❌ no |
On the child side all three converge on the same code. newSession (:4049), loadSession (:4159) and unstable_resumeSession (:4321) each call newSessionConfig(cwd, …, params.sessionId, true), which inherits the workspace's runtime MCP servers before config.initialize() and binds sendSdkMcpMessage: this.buildClientMcpSender(wiredSessionId) (:10532). deliverClientMcpMessage then stamps sessionId on the frame (:2640). So a cold restore emits exactly the same identity-carrying discovery frame as a spawn — and the parent has not registered the id yet, because createSessionEntry runs after the ACP call resolves.
For completeness, the fourth caller of newSessionConfig (:10257, transcript replay) passes skipMcpDiscovery: true and no session id, so those three are the whole surface.
The patch. BridgeClient already maintains the right allow-list for this — inFlightRestoreIds, populated by markRestoreInFlight at bridge.ts:4397, before the loadSession / resumeSession call, and used by the sibling guards in sessionUpdate (:792) and handleArtifactEvent (:1699). One clause reuses it:
!ownsSession &&
- !this.hasSessionSpawnInFlight()
+ !this.hasSessionSpawnInFlight() &&
+ !this.inFlightRestoreIds.has(sessionId)
) {With that applied: P4/P5 go back to base behaviour with the context still stripped, P3 stays rejected, P2 still gets trusted context, and the same two suites still report 530/530 — so it breaks nothing and needs no test edits. Worth noting it is stricter than the spawn-path bypass: it matches the specific id, where hasSessionSpawnInFlight() accepts any id while any spawn is running. That looseness is unavoidable on the spawn path (the child picks the id), but it is avoidable here, so there is no reason not to.
Scope, stated honestly — same as last round. The A/B is a deterministic proof of the guard's behaviour on the real bridge, and the child-side path is read from source, not captured from a live daemon: I did not stand up qwen serve with a real extension-hosted MCP server and restore a persisted session into it. What I am confident about is the narrower claim, which I think is enough to act on: main allowed these two frames and this PR rejects them, on paths whose child-side code is identical to the one you just fixed.
4. Mutation matrix for the new fix — 5/6 killed
| # | Neutered behaviour | Verdict |
|---|---|---|
| N1 | revert the pre-registration bypass (hard reject again) | 🔴 KILLED |
| N2 | forward the claimed id as trusted context | 🔴 KILLED |
| N3 | drop the channelInfo?.sessionIds === sessionIds clause |
|
| N4 | never close the window (> 0 → >= 0) |
🔴 KILLED |
| N5 | drop the [demux] … forwarded_without_session line |
🔴 KILLED |
| N6 | accept an empty-string sessionId |
🔴 KILLED |
N1/N2/N4/N5 each kill a named new test, and N6 re-confirms the malformed-id assertion still bites after the block was restructured. The 8 mutations from 51eba8ea4 are untouched — production source outside bridgeClient.ts / bridge.ts is byte-identical.
N3 is the only survivor, and it is a test gap, not a defect. The clause is right: without it a replaced or dying channel's BridgeClient would read the current channelInfo's spawn counter and open a window it has no business opening. Nothing pins that today, so a future refactor can delete it and stay green. A one-line assertion (spawn on a respawned channel, frame from the stale client) would fix it. Non-blocking.
5. Minor — still open
The qwen-code-ci-bot nit from stage 2 is still unaddressed at this head: readRuntimeMcpAddRequest (acpAgent.ts:2684) forces type: 'sdk' when clientMcpOverWs === true with no explanation, and the pre-refactor code explained why (the manager must bind an SdkControlClientTransport whose sendMcpMessage routes back over the daemon WS — #5626 Phase 2). One line. Non-blocking.
Verdict
The fix is correct and well-tested for the path it targets, and my previous §3 is genuinely closed. I'd like the two-line restore-window patch in §3 folded in before merge — it is the same bug class on sibling paths, the patch reuses machinery you already have, and it costs zero test churn. §4's N3 and §5 are both nits I'm happy to see land in a follow-up.
中文版本
Maintainer 复验 —— 本地真实构建与测试 @ e5365cc203
在基于 merge-base（7f8adc659）的全新隔离 worktree 中重跑了全部验证，Linux 6.12.63 / Node 22.22.2。§3 的修复确实有效 —— 我在真实 bridge 上复现了预注册窗口的关闭与重新打开。 所有 gate 全绿，变异矩阵依然起作用。
有一点建议在合并前处理：本次修复关闭了 newSession 的窗口，但两条 restore 路径上的同一窗口仍然是关闭的，而这相对 main 是行为变更。详情与一行补丁见 §3。
1. Gate 结果
| Gate | 结果 |
|---|---|
packages/acp-bridge —— bridge.test.ts + bridgeClient.test.ts |
✅ 530 passed（2 个文件） |
packages/cli —— acp-integration/acpAgent.test.ts |
✅ 318 passed |
npm run typecheck --workspace=packages/acp-bridge |
✅ exit 0 |
npm run typecheck --workspace=packages/cli |
✅ 0 errors 在 src/acp-integration（见下方说明） |
npx eslint —— 9 个改动文件 |
✅ exit 0 |
npx prettier --check —— 9 个改动文件 |
✅ exit 0 |
git diff --check |
✅ exit 0 |
构建 —— packages/core + cli 的完整 workspace 依赖闭包 |
✅ 13 / 13 packages OK |
相对 merge-base 的测试数变化：522 → 530，8 个新增测试全部通过，且没有任何既有测试改变结论。上一轮的 ESLint 阻塞保持修复状态。
两点环境说明，既为了让上面的数字可复现，也为了把我没能拿到的信号明说而不是藏起来。这台机器的共享安装比
main落后 45 个依赖，因此：(a) 整仓库的npm run build在这里跑不起来 —— 我改为构建packages/core加上 cli 的完整 workspace 依赖闭包，13/13 通过；(b)packages/cli的 typecheck 仍有 49 个错误，全部位于src/ui/**，来自ink/diff的类型版本偏移，src/acp-integration/**中为 0 —— 而本 PR 没有改动任何src/ui文件。另外，在我于 worktree 中构建packages/core之前，bridge.test.ts会出现 2 个失败（forwards childEnvOverrides…、rebuilds authoritative invocation metadata…），它们在 merge-base 上同样失败：PRIVATE_ACP_CAPABILITY_ENV解析为undefined，于是环境变量被写到了字面量"undefined"这个 key 下。构建 core 后两个都消失。
2. 修复的行为与描述一致
我重跑了与上次相同的差分探针 —— 一个逐字节相同的文件（md5 305ea215…）驱动真实的 createAcpSessionBridge + BridgeClient，两列之间只有 packages/acp-bridge/src 不同 —— 并把它扩展到本次修复没有覆盖的路径。
| 探针 | base 7f8adc659 |
head e5365cc203 |
|---|---|---|
P1 从子进程 newSession 内部发出的 frame |
送达，context = undefined |
送达，context 被剥离 ✅ |
| P2 session 注册之后发送的 frame | 送达，context = undefined |
送达，context = { sessionId } ✅ |
| P3 声称属于其他 channel 的 id，窗口未打开 | 泄漏给 sender | -32602，sender 未被调用 ✅ |
| P6 完全不带 session id 的 frame（旧 sender） | 送达，context = undefined |
无变化 ✅ |
P1 正是我上次提的问题，现在以我期望的方式被关闭了 —— 拒绝的是身份，而不是整个 frame。stderr 上的 forwarded_without_session 是个好设计，让这个窗口从静默变成可观测。
3. loadSession 与 unstable_resumeSession 上的同一窗口仍然关闭
| 探针 | base 7f8adc659 |
head e5365cc203 |
head + 下方补丁 |
|---|---|---|---|
P4 从子进程 loadSession 内部发出的 frame |
送达 | -32602 |
送达，context 被剥离 |
P5 从子进程 unstable_resumeSession 内部发出的 frame |
送达 | -32602 |
送达，context 被剥离 |
原因。 父进程只在一个地方打开窗口 —— doSpawn 中的 ci.sessionSpawnsInFlight++（bridge.ts:2581，在 :2849 的 finally 中释放，所以确实覆盖整个 spawn）。但父进程有三处创建 session 的 ACP 调用，另外两处从不碰这个计数器：
bridge.ts |
ACP 调用 | 是否打开窗口 |
|---|---|---|
:2604 |
ci.connection.newSession(…) |
✅ 是 |
:4427 |
ci.connection.loadSession(…) |
❌ 否 |
:4458 |
ci.connection.unstable_resumeSession(…) |
❌ 否 |
而在子进程侧，三者收敛到同一段代码。newSession（:4049）、loadSession（:4159）和 unstable_resumeSession（:4321）都调用 newSessionConfig(cwd, …, params.sessionId, true)，它会在 config.initialize() 之前继承 workspace 的运行时 MCP server，并绑定 sendSdkMcpMessage: this.buildClientMcpSender(wiredSessionId)（:10532）。随后 deliverClientMcpMessage 会把 sessionId 打到 frame 上（:2640）。所以一次冷 restore 发出的 discovery frame 与一次 spawn 完全同形 —— 而父进程此时还没注册该 id，因为 createSessionEntry 是在 ACP 调用返回之后才执行的。
补充完整性：newSessionConfig 的第四个调用方（:10257，transcript replay）传入 skipMcpDiscovery: true 且没有 session id，所以上面三处就是全部受影响面。
补丁。 BridgeClient 已经维护了正好合用的允许列表 —— inFlightRestoreIds，由 bridge.ts:4397 的 markRestoreInFlight 在 loadSession / resumeSession 调用之前填充，并且已被 sessionUpdate（:792）与 handleArtifactEvent（:1699）中的同类守卫使用。加一个子句即可复用：
!ownsSession &&
- !this.hasSessionSpawnInFlight()
+ !this.hasSessionSpawnInFlight() &&
+ !this.inFlightRestoreIds.has(sessionId)
) {应用后：P4/P5 回到 base 行为且 context 仍被剥离，P3 仍然被拒绝，P2 仍然拿到可信 context，同样两个 suite 仍然 530/530 —— 不破坏任何东西，也不需要改动测试。值得一提的是它比 spawn 路径的旁路更严格：它匹配具体的 id，而 hasSessionSpawnInFlight() 在任意 spawn 进行中会接受任意 id。那种宽松在 spawn 路径上无法避免（id 由子进程选择），但在这里是可以避免的，所以没有理由不做。
关于结论范围，如实说明 —— 与上一轮相同。这个 A/B 是在真实 bridge 上对校验行为本身的确定性证明，子进程侧的路径是读源码得出的，不是从 live daemon 抓取的：我没有真的起一个带真实 extension-hosted MCP server 的 qwen serve 并把持久化 session restore 进去。我有信心的是那个更窄的结论，我认为它已经足以支撑处理：main 允许这两个 frame，而本 PR 拒绝它们，而且这些路径的子进程侧代码与你刚修好的那条完全相同。
4. 针对新修复的变异矩阵 —— 6 个中杀死 5 个
| # | 被废掉的行为 | 判定 |
|---|---|---|
| N1 | 回退预注册旁路（重新变成硬拒绝） | 🔴 KILLED |
| N2 | 把声称的 id 当作可信 context 转发 | 🔴 KILLED |
| N3 | 去掉 channelInfo?.sessionIds === sessionIds 子句 |
|
| N4 | 窗口永不关闭（> 0 → >= 0） |
🔴 KILLED |
| N5 | 去掉 [demux] … forwarded_without_session 日志 |
🔴 KILLED |
| N6 | 接受空字符串 sessionId |
🔴 KILLED |
N1/N2/N4/N5 各自都能杀死一个具名的新测试，N6 则确认在该代码块被重构之后，畸形 id 的断言仍然有效。来自 51eba8ea4 的那 8 个变异不受影响 —— bridgeClient.ts / bridge.ts 之外的生产代码逐字节相同。
N3 是唯一的幸存者，而且它是测试缺口，不是缺陷。 这个子句是对的：没有它，一个被替换或正在消亡的 channel 的 BridgeClient 会去读当前 channelInfo 的 spawn 计数器，从而打开一个本不该打开的窗口。目前没有任何测试固定这个行为，所以未来的重构可以把它删掉而 CI 依然全绿。补一行断言即可（在重建后的 channel 上 spawn，并从旧 client 发一个 frame）。非阻塞。
5. 次要 —— 仍未处理
stage 2 中 qwen-code-ci-bot 提的小问题在当前 head 上仍未处理：readRuntimeMcpAddRequest（acpAgent.ts:2684）在 clientMcpOverWs === true 时强制 type: 'sdk'，却没有任何说明，而重构前的代码是有解释的（manager 必须绑定 SdkControlClientTransport，其 sendMcpMessage 会经由 daemon WS 回传 —— #5626 Phase 2）。一行注释即可。非阻塞。
结论
对于它所针对的路径，这个修复是正确且测试充分的，我上一轮的 §3 已经真正关闭。希望 §3 里那两行 restore 窗口补丁能在合并前合入 —— 它是同类 bug 出现在兄弟路径上，补丁复用了你已有的机制，且零测试改动成本。§4 的 N3 与 §5 都是小问题，放到后续 PR 处理我没有意见。
|
Released in v0.21.1. |
3 participants
What this PR does
Supersedes #7645, which GitHub could not reopen after its branch was rebased onto the current
main.Adds a session-owned runtime MCP route alongside the existing workspace-owned route. A daemon host can now add or remove an SDK MCP server for one selected live session without mutating bootstrap state, fanning out to siblings, or publishing workspace-wide events.
Carries the originating session identity through reverse MCP messages and verifies that the claimed session belongs to the ACP channel before forwarding it to the host-side sender.
Why it's needed
Daemon-managed channels need private agent tools that belong to the channel's live session. Reusing the workspace route would expose those tools to unrelated terminal and web sessions in the same runtime. This provides the generic isolation boundary required by the later channel-loop integration while remaining independent of channel behavior.
Reviewer Test Plan
How to verify
cd packages/acp-bridge && npx vitest run src/bridgeClient.test.ts src/bridge.test.ts.cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts.npm run build,npm run typecheck, andnpm run lint.Evidence (Before & After)
N/A — protocol and routing infrastructure only; no UI changes.
Tested on
Environment (optional)
Node.js 25.9.0 workspace install; package-level Vitest suites and repository build/type/lint checks.
Risk & Scope
Linked Issues
Related to #7628 and #7641. Supersedes #7645.
中文说明
本 PR 做了什么
本 PR 替代 #7645；该 PR 的分支变基到当前
main后，GitHub 不再允许重新打开。在现有 workspace 归属的运行时 MCP 路由之外，新增 session 归属的运行时 MCP 路由。daemon host 现在可以只为一个指定的 live session 添加或移除 SDK MCP server，不会修改 bootstrap 状态、扩散到兄弟 session，也不会发布 workspace 级事件。
反向 MCP 消息会携带来源 session 身份；在转交给 host 侧 sender 之前，还会校验该 session 确实归当前 ACP channel 所有。
为什么需要
daemon-managed channel 需要只属于其 live session 的私有 agent tools。如果复用 workspace 路由，这些工具会暴露给同一 runtime 中无关的终端和 Web session。这个 PR 提供后续 channel-loop 集成所需的通用隔离边界，同时不包含任何 channel 业务。
Reviewer 测试计划
如何验证
cd packages/acp-bridge && npx vitest run src/bridgeClient.test.ts src/bridge.test.ts。cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts。npm run build、npm run typecheck和npm run lint。证据（前后对比）
N/A——仅协议和路由基础设施，没有 UI 变化。
测试平台
环境（可选）
Node.js 25.9.0 workspace 安装；执行 package 级 Vitest suite 与仓库 build/type/lint 检查。
风险与范围
关联 Issue
关联 #7628 和 #7641。本 PR 替代 #7645。