feat(serve): establish workspace runtime ownership - #8213
Conversation
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR — the write-up is thorough and the test coverage is substantial. Template looks good ✓.
I'm going to stop here and hand this to a maintainer rather than push it into a full code review, because the open question is direction, not implementation quality.
The problem this solves is architectural, not observed. There's no linked issue and no before/after reproduction — the motivation is that workspace management "depends too heavily on Session lifecycle," which is a legitimate design concern but not a bug anyone has hit. By the PR's own account there is no user-visible behavior change here; the user-facing payoff (converging Extensions, MCP, Skills, and Tools behind one parameterless ensure) is explicitly future work. So this lands a large piece of infrastructure whose value depends on follow-up PRs that aren't merged.
This is effectively proposing a new daemon architecture. It introduces a five-state runtime snapshot, workspace-scoped monotonic epochs, physical work leases, bounded startup/teardown, ten-minute keepalive windows, and admission control wired into workspace removal, trust replacement, and daemon shutdown — and it expands the public contract with new owner-aware REST routes (primary + workspace-qualified ensure/status) plus additive TypeScript SDK methods. It's also the foundation of a stack from the same author (#7308 is the v1 of this same change; #7309 MCP, #7310 extensions, #7311 skills all build on it).
For context on scale: ~1,900 production lines across acp-bridge, cli/serve, and sdk-typescript (plus ~2,000 test lines and ~1,200 lines of design docs), 46 files. That's not a blocking number on its own for a feat — but it's why the direction question should be settled before anyone invests in reviewing the code line by line.
The ask: whether qwen-code wants to commit to this workspace-runtime-ownership model (epochs, leases, admission control), and whether landing the foundation ahead of the capability convergence is the right sequencing, is an architecture call the gate can't make — and on a core-surface change this size we won't approve a direction we can't fully vouch for. Escalating for a maintainer decision, not rejecting the work: it's clearly substantial and well-structured, and if the direction is wanted, this is a reasonable shape for it.
@ytahdn — could you link the issue or design discussion this stack is working toward? Having the target (and ideally a concrete zero-Session management scenario that's broken today) would make the foundation much easier to evaluate.
cc @wenshao @tanzhenxin @yiliang114 @LaZzyMan for the direction call.
中文说明
感谢贡献——PR 描述非常详尽，测试覆盖也很充分。模板完整 ✓。
我会在这里停下来，转交 maintainer 处理，而不是直接进入完整代码审查——因为当前的核心问题是方向，而不是实现质量。
这个 PR 解决的是架构问题，而非已观测到的问题。 没有关联 issue，也没有 before/after 复现——动机是 workspace 管理"对 Session 生命周期依赖过重"，这是一个合理的设计顾虑，但并不是有人实际遇到的 bug。按 PR 自己的说法，这里没有任何用户可见的行为变化；真正面向用户的收益（把 Extensions、MCP、Skills、Tools 收敛到单个无参数 ensure）明确属于后续工作。也就是说，这个 PR 落地了一大块基础设施，而其价值依赖于尚未合并的后续 PR。
这实际上是在提议一套新的 daemon 架构。 它引入了五态 runtime 快照、workspace 级单调 epoch、物理 work lease、有界启动/回收、十分钟保活窗口，以及接入 workspace removal、trust replacement、daemon shutdown 的 admission control——并通过新的 owner-aware REST 路由（primary 和 workspace-qualified 的 ensure/status）以及增量 TypeScript SDK 方法扩展了公共契约。它也是同一作者一系列 PR 的基础（#7308 是本 PR 的 v1；#7309 MCP、#7310 extensions、#7311 skills 都建立在它之上）。
规模背景： 约 1,900 行生产代码，横跨 acp-bridge、cli/serve、sdk-typescript（另有约 2,000 行测试、约 1,200 行设计文档），共 46 个文件。对 feat 来说这个数字本身不构成阻断——但正因如此，方向问题应该在有人逐行审查代码之前先确定下来。
诉求： qwen-code 是否要采用这套 workspace-runtime-ownership 模型（epoch、lease、admission control），以及先落地 foundation、再做 capability 收敛的时序是否合适，这是一个 gate 无法替项目做出的架构决策——而对于这种规模的核心层变更，我们不会批准一个我们无法完全担保的方向。这里是转交 maintainer 决策，而不是否定这项工作：它显然很充实、结构也良好，如果方向被认可，这是一个合理的形态。
@ytahdn —— 能否关联这一系列 PR 所面向的 issue 或设计讨论？有一个明确的目标（最好是一个今天确实会出问题的零 Session 管理场景）会让这个 foundation 容易评估得多。
cc @wenshao @tanzhenxin @yiliang114 @LaZzyMan 做方向决策。
— Qwen Code · qwen3.8-max-preview
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
features[] |
— | "workspace_runtime" |
— Qwen Code · serve A/B
8373454 to
c7f52e7
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本 PR 来自 fork，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
/verify |
|
@qwen-code /verify |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 1261 passed · 0 failed · 1261 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：1261 通过 · 0 失败 · 1261 总计 Verification report (report.md)Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action taken — awaiting maintainer direction decisionThe only review feedback on this PR is a CHANGES_REQUESTED from the automated triage gate (stage 1c) that explicitly escalates for a maintainer decision on architecture direction rather than identifying any code defect. The gate states: "Escalating for a maintainer decision, not rejecting the work: it's clearly substantial and well-structured, and if the direction is wanted, this is a reasonable shape for it." The open questions are product/architecture calls, not implementation issues:
These decisions belong to the maintainers (cc'd: @wenshao @tanzhenxin @yiliang114 @LaZzyMan). No code change can resolve them — neither implementing nor declining the direction is appropriate for the autofix loop. Current PR health:
The PR is implementation-ready and awaits the maintainer direction call. 中文说明未采取任何操作 —— 等待 maintainer 方向决策本 PR 唯一的评审反馈来自自动 triage 门禁（stage 1c）的 CHANGES_REQUESTED，该评审明确将问题升级为 maintainer 方向决策，而非指出任何代码缺陷。门禁原文："Escalating for a maintainer decision, not rejecting the work: it's clearly substantial and well-structured, and if the direction is wanted, this is a reasonable shape for it."（转交 maintainer 决策，并非否定这项工作：它显然充实且结构良好，如果方向被认可，这是一个合理的形态。） 待决问题属于产品/架构决策，而非实现问题：
这些决策属于 maintainer（已 cc：@wenshao @tanzhenxin @yiliang114 @LaZzyMan）。没有任何代码变更可以解决它们 —— autofix 循环既不应擅自实现某个有争议的方向，也不应将其拒绝为"超出范围"。 PR 当前状态：
PR 在实现层面已就绪，等待 maintainer 的方向决策。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.
中文说明
已审查。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 未审查：反向审计——有审计 agent 运行并打开了自己的 brief，但没有 agent 是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的，agent 实际被要求做的并不是本 skill 所认证的内容。
— qwen3.8-max-preview via Qwen Code /review
| finishWorkspaceMcpDiscovery(info); | ||
| if (hasNoChannelWork(info)) { | ||
| await startIdleTimer(info, 'workspace MCP discovery complete'); |
There was a problem hiding this comment.
[Suggestion] Dead hasNoChannelWork() + startIdleTimer() branch — workspaceControlInFlight is still positive inside the withWorkspaceControl callback, so hasNoChannelWork(info) always returns false and this startIdleTimer call is unreachable. The idle timer is correctly armed by withWorkspaceControl's own finally block. — Concrete cost: the dead branch misleads readers into thinking the idle policy depends on a separate check here; a future edit removing the wrapper's finally (believing this check covers it) would break idle reaping. Same pattern at line 8714 (runtimeOperationReservations still positive).
中文说明
withWorkspaceControl 回调内部 workspaceControlInFlight 仍为正数，因此 hasNoChannelWork(info) 始终返回 false，此处的 startIdleTimer 调用不可达。idle timer 实际由 withWorkspaceControl 自身的 finally 块正确启动。具体代价：死分支会误导读者认为 idle 策略依赖此处的单独检查；若未来编辑移除了 wrapper 的 finally（误以为此检查已覆盖），将破坏 idle 回收。第 8714 行存在相同模式（runtimeOperationReservations 仍为正数）。
— qwen3.8-max-preview via Qwen Code /review
| if (info && hasNoChannelWork(info) && !info.isDying) { | ||
| await startIdleTimer(info, 'workspace generation'); | ||
| } |
There was a problem hiding this comment.
[Suggestion] Dead hasNoChannelWork() + startIdleTimer() branch — runtimeOperationReservations is still positive at this point (incremented at line 8658, decremented by releaseRuntimeOperationReservation on the next line), so hasNoChannelWork always returns false. The idle timer is correctly armed by settleReleasedRuntimeWork inside releaseRuntimeOperationReservation. — Concrete cost: same as the MCP discovery instance above.
中文说明
此时 runtimeOperationReservations 仍为正数（在第 8658 行递增，由下一行的 releaseRuntimeOperationReservation 递减），因此 hasNoChannelWork 始终返回 false。idle timer 实际由 releaseRuntimeOperationReservation 内部的 settleReleasedRuntimeWork 正确启动。具体代价同上。
— qwen3.8-max-preview via Qwen Code /review
| const preheat = Promise.resolve().then(() => | ||
| opts?.keepAliveMs === undefined | ||
| ? preheatAcpChildOnBridge() | ||
| : preheatAcpChildOnBridge({ keepAliveMs: opts.keepAliveMs }), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] Post-timeout preheat errors are silently swallowed — the old code's standalone void promise.then(_, errorHandler) logged bridge preheat failures unconditionally; the replacement has no post-timeout error handler. — Failure scenario: preheatAcpChildOnBridge() takes 7 s to fail with ECONNREFUSED while the caller uses the default 5 s timeout. At 5 s the withTimeout wrapper rejects with TimeoutError and the catch block logs "ACP preheat timed out". At 7 s the preheat promise rejects, but the outer promise is already settled so reject(err) is a no-op. The root-cause error is never written to stderr — an operator cannot distinguish a slow-but-working child from a permanently unreachable one.
| const preheat = Promise.resolve().then(() => | |
| opts?.keepAliveMs === undefined | |
| ? preheatAcpChildOnBridge() | |
| : preheatAcpChildOnBridge({ keepAliveMs: opts.keepAliveMs }), | |
| ); | |
| const preheat = Promise.resolve().then(() => | |
| opts?.keepAliveMs === undefined | |
| ? preheatAcpChildOnBridge() | |
| : preheatAcpChildOnBridge({ keepAliveMs: opts.keepAliveMs }), | |
| ); | |
| void preheat.then(undefined, (err) => { | |
| writeStderrLineSafe( | |
| `qwen serve: ACP preheat failed: ${err instanceof Error ? err.message : String(err)}`, | |
| ); | |
| }); |
中文说明
超时后的 preheat 错误被静默吞掉——旧代码通过独立的 void promise.then(_, errorHandler) 无条件记录 bridge preheat 失败；新代码没有超时后的错误处理器。失败场景：preheatAcpChildOnBridge() 在 7 秒后因 ECONNREFUSED 失败，而调用方使用默认 5 秒超时。5 秒时 withTimeout 以 TimeoutError 拒绝，catch 块记录 "ACP preheat timed out"。7 秒时 preheat promise 拒绝，但外层 promise 已结算，reject(err) 为空操作。根因错误永远不会写入 stderr——运维人员无法区分慢但正常的子进程与永久不可达的子进程。
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Deferred to a follow-up. The observability gap is real — a post-timeout preheat rejection is currently swallowed because the outer promise is already settled. This round was scoped to the base-conflict resolution plus two verified dead-code removals (and carried a time-budget warning), so this behavioral change — which needs its own focused implementation and a test exercising the slow-then-failing preheat path — is being tracked on its open thread rather than batched here.
中文说明
延后到后续处理。这个可观测性缺口确实存在——超时后的 preheat 拒绝目前会被吞掉，因为外层 promise 已经结算。本轮的范围限定为基础冲突解决加上两处经过验证的死代码移除（并且带有时间预算警告），因此这项行为变更——需要单独聚焦实现并配以一个覆盖"先慢后失败"preheat 路径的测试——在其开启的讨论串中跟踪，而不在此处合并处理。
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
[Suggestion] terminateChannel re-throws the original timeout error after killSync() succeeds — callers log "channel kill failed" when the channel was actually killed by the forced path. — Failure scenario: channel.kill() exceeds initTimeoutMs, killSync() sends SIGKILL and succeeds (the process is dead), but throw error propagates the timeout. killChannelWithLog catches it and logs qwen serve: channel kill failed (idle timeout): …. An operator seeing this message may waste time investigating a non-problem. Consider returning normally after a successful killSync() — the channel is dead, which is the contract.
中文说明
terminateChannel 在 killSync() 成功后仍重新抛出原始超时错误——调用方在通道实际已被强制终止时记录 "channel kill failed"。失败场景：channel.kill() 超过 initTimeoutMs，killSync() 发送 SIGKILL 并成功（进程已死），但 throw error 传播了超时错误。killChannelWithLog 捕获并记录 qwen serve: channel kill failed (idle timeout): …。运维人员看到此消息可能会浪费时间调查一个不存在的问题。建议在 killSync() 成功后正常返回——通道已死，这就是契约。
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Deferred to a follow-up. Returning normally after a successful killSync() is a reasonable fix for the misleading "channel kill failed" log, but it changes error propagation out of terminateChannel and needs every caller (killChannelWithLog and the idle/timeout retirement paths) verified against the new contract. That verification did not fit this conflict-resolution round's budget, so it stays open on its thread.
中文说明
延后到后续处理。在 killSync() 成功后正常返回，对于消除误导性的 "channel kill failed" 日志是一个合理的修复，但它改变了 terminateChannel 向外的错误传播，需要针对新契约逐一验证所有调用方（killChannelWithLog 以及 idle/timeout 回收路径）。该验证不在本轮冲突解决的预算之内，因此保持其讨论串开启。
| if ( | ||
| server !== undefined && | ||
| server.authenticationState !== 'pending' | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] Removed cleanup path for servers that disappear from discovery results while OAuth is pending — the expiration timer now kills the entire channel instead of silently clearing the stale tracking entry. — Failure scenario: user initiates OAuth for server X → X is removed from config → the next requestWorkspaceStatus poll sees discoveryState === 'completed' but X is absent from rawServers. Old code: server?.authenticationState !== 'pending' is true (undefined ≠ 'pending') and discoveryState === 'completed' satisfies the second conjunct, so X is removed from tracking and its timer cancelled. New code: server !== undefined is false, so X stays tracked. When the 10-minute timer fires, expireWorkspaceMcpAuthentication kills the shared channel, disconnecting every session on it. Consider restoring the disappeared-server cleanup inside the loop.
中文说明
移除了 OAuth 挂起期间从 discovery 结果中消失的服务器的清理路径——过期定时器现在会终止整个通道，而不是静默清除过期的跟踪条目。失败场景：用户为服务器 X 发起 OAuth → X 从配置中移除 → 下次 requestWorkspaceStatus 轮询看到 discoveryState === 'completed' 但 X 不在 rawServers 中。旧代码会清除 X 的跟踪并取消定时器。新代码：server !== undefined 为 false，X 保持跟踪。10 分钟定时器触发时，expireWorkspaceMcpAuthentication 终止共享通道，断开其上的所有会话。建议在循环内恢复消失服务器的清理。
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Deferred to a follow-up. This is the highest-risk of the batch: if a server disappears from discovery results while its OAuth is pending, the new server !== undefined guard keeps it tracked and the 10-minute expiry can kill the shared channel. Restoring the disappeared-server cleanup inside the loop is a behavioral regression fix that warrants a dedicated regression test (OAuth pending → server removed from config → next poll clears tracking without killing the channel), which did not fit this round's budget. Tracked on its open thread.
中文说明
延后到后续处理。这是本批中风险最高的一项：如果某个服务器在其 OAuth 挂起期间从 discovery 结果中消失，新的 server !== undefined 守卫会使其保持被跟踪，而 10 分钟过期可能会终止共享通道。在循环内恢复消失服务器的清理是一项行为回归修复，应当配以一个专门的回归测试（OAuth 挂起 → 服务器从配置移除 → 下次轮询清除跟踪而不终止通道），这不在本轮预算之内。在其开启的讨论串中跟踪。
| if (ci) { | ||
| await withWorkspaceControl(ci, cancelActivePrompt); | ||
| } else { |
There was a problem hiding this comment.
[Suggestion] Wrapping the best-effort closeSession cancel in withWorkspaceControl means a cancel timeout kills the shared channel via retireChannelOnTimeout, disrupting all other sessions. — Failure scenario: session A is being closed while sessions B and C share the same channel. The agent takes >10 s (initTimeoutMs) to acknowledge the cancel for session A. withTimeout throws BridgeTimeoutError → withWorkspaceControl's catch calls retireChannelOnTimeout → killChannelWithLog kills the channel. Sessions B and C lose their live channel. The cancel is fire-and-forget cleanup for an already-closed session (the outer catch {} swallows the error); its failure should not retire the channel that other sessions depend on. Consider calling cancelActivePrompt() directly (as the else branch already does) instead of routing through withWorkspaceControl.
中文说明
将尽力而为的 closeSession cancel 包装在 withWorkspaceControl 中，意味着 cancel 超时会通过 retireChannelOnTimeout 终止共享通道，影响所有其他会话。失败场景：会话 A 正在关闭，而会话 B 和 C 共享同一通道。agent 花费超过 10 秒（initTimeoutMs）才确认会话 A 的 cancel。withTimeout 抛出 BridgeTimeoutError → withWorkspaceControl 的 catch 调用 retireChannelOnTimeout → killChannelWithLog 终止通道。会话 B 和 C 失去活跃通道。cancel 是对已关闭会话的即发即弃清理（外层 catch {} 吞掉错误）；其失败不应终止其他会话依赖的通道。建议直接调用 cancelActivePrompt()（如 else 分支所做），而不是通过 withWorkspaceControl 路由。
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Deferred to a follow-up. Routing the best-effort closeSession cancel through withWorkspaceControl means a cancel timeout can retire the shared channel via retireChannelOnTimeout, disrupting other sessions on it; calling cancelActivePrompt() directly (as the else branch does) is the suggested fix. That changes channel-retirement semantics for an already-closed session's cleanup and needs careful verification across shared-channel sessions, which did not fit this round's budget. Tracked on its open thread.
中文说明
延后到后续处理。将尽力而为的 closeSession cancel 通过 withWorkspaceControl 路由，意味着 cancel 超时可能经由 retireChannelOnTimeout 回收共享通道，从而影响其上的其他会话；建议的修复是直接调用 cancelActivePrompt()（如 else 分支所做）。这会改变一个已关闭会话清理过程的通道回收语义，需要在共享通道的多个会话之间仔细验证，这不在本轮预算之内。在其开启的讨论串中跟踪。
|
🤖 AutoFix ran out of time before finishing (timeout (3000000ms)) (attempt 1/100) — it will retry on the next scan.
See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/30643811014 🧠 Handled by Qwen Code · model/模型 |
…me-foundation-v2 # Conflicts: # docs/developers/daemon/17-configuration.md
The idle timer is armed by withWorkspaceControl's finally block and by settleReleasedRuntimeWork inside releaseRuntimeOperationReservation, so the hasNoChannelWork guards inside those callbacks never fire (the workspace-control counter and runtime-operation reservation are still positive at those points). Drop the dead branches and the now write-only info local.
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix review round summary (PR #8213)Conflict resolution (
|
| rc id | Location | Decision |
|---|---|---|
| 3691942492 | bridge.ts ~3649 (workspace MCP discovery) |
Resolved — removed the dead branch |
| 3691942497 | bridge.ts ~8716 (workspace generation) |
Resolved — removed the dead branch + write-only local |
| 3691942500 | workspace-service/index.ts:453 |
Deferred (behavioral, needs its own verification) |
| 3691942517 | bridge.ts:1672 |
Deferred (behavioral, error-propagation change) |
| 3691942521 | bridge.ts:3667 |
Deferred (behavioral regression, needs dedicated test) |
| 3691942524 | bridge.ts:5273 |
Deferred (behavioral, channel-retirement semantics) |
Resolved — dead idle-timer branches (3691942492, 3691942497). Verified both branches are unreachable before removing them:
- At the MCP-discovery site, the code runs inside
withWorkspaceControl(info, …)(call at line 3591), soworkspaceControlInFlight ≥ 1whenhasNoChannelWork(info)is evaluated. That predicate requiresworkspaceControlInFlight === 0, so it always returns false. The idle timer is correctly armed bywithWorkspaceControl's ownfinallyblock once the counter decrements. - At the workspace-generation site,
runtimeOperationReservationsis incremented before the IIFE and only decremented byreleaseRuntimeOperationReservation(…)on the line after thehasNoChannelWork(info)check, so the predicate (which requiresruntimeOperationReservations === 0) always returns false. The idle timer is correctly armed bysettleReleasedRuntimeWorkinsidereleaseRuntimeOperationReservation.
Removed the two dead if (…) await startIdleTimer(…) branches, plus the info local in the generation IIFE that became write-only after the removal (kept noUnusedLocals clean). No behavior change — the full acp-bridge suite (1024 tests) still passes.
Deferred — 3691942500, 3691942517, 3691942521, 3691942524. These four are Suggestion-level behavioral changes (post-timeout preheat error logging, terminateChannel error propagation after a successful killSync, restoring disappeared-server OAuth cleanup, and unwrapping the best-effort closeSession cancel from withWorkspaceControl). Each alters runtime behavior and warrants its own focused implementation plus targeted regression test rather than being batched into a conflict-resolution round. This round also carried a budget warning (a prior round exhausted its time budget), so the scope was intentionally limited to the conflict plus the two verified, zero-behavior-change dead-code removals. Each deferred finding keeps its thread open with a per-finding reply recording the reason.
Verification
git merge origin/main— completed; one conflict (docs/developers/daemon/17-configuration.md) resolved by combining both sides, re-aligned with Prettiernpm run build— passednpm run typecheck— passednpm run lint— passednpx eslint packages/acp-bridge/src/bridge.ts— passedvitestpackages/acp-bridge (touched) — 1024 passed (20 files)vitestpackages/cli touched serve tests (workspace-runtime-coordinator, routes/workspace-runtime, routes/workspace-management, workspace-service facade, server/error-response) — 234 passed (5 files)vitestpackages/cli (run-qwen-serve, server, commands/serve) — 1097 passed (3 files)
中文说明
Autofix 审查轮次总结（PR #8213）
冲突解决（--conflict true）
将 origin/main 合并进 PR 分支。main 带入了 5 个提交，包括托管内存隔离工作（#8056）和一个 flaky e2e 超时修复（#8246）。只有一个文件冲突：docs/developers/daemon/17-configuration.md。双方都编辑了同样的两个表格——main 增加了 --memory-project-scope CLI flag 行和 memoryProjectScope ServeOptions 行，而本分支更新了 --channel-idle-timeout-ms / channelIdleTimeoutMs 的描述以反映新的"在会话与 workspace 操作都排空后才回收"语义。解决方式两者都保留：保留 main 新增的 memory 行，以及本分支更新后的 idle-timeout 措辞（在 main 的表格之上重新应用，再用 Prettier 重新对齐）。其余所有文件均自动合并；通过完整的 build + typecheck 确认自动合并在语义上是正确的（本分支对 AcpSessionBridge 接口的增量与 main 的 memory 增量可以无错误共存）。
反馈点
| rc id | 位置 | 决定 |
|---|---|---|
| 3691942492 | bridge.ts ~3649（workspace MCP discovery） |
已解决 — 移除死分支 |
| 3691942497 | bridge.ts ~8716（workspace generation） |
已解决 — 移除死分支 + 只写局部变量 |
| 3691942500 | workspace-service/index.ts:453 |
延后（行为变更，需单独验证） |
| 3691942517 | bridge.ts:1672 |
延后（行为变更，错误传播改动） |
| 3691942521 | bridge.ts:3667 |
延后（行为回归，需专门测试） |
| 3691942524 | bridge.ts:5273 |
延后（行为变更，通道回收语义） |
已解决 — 死的 idle-timer 分支（3691942492、3691942497）。 在移除前已验证两个分支都不可达：
- 在 MCP-discovery 处，代码运行于
withWorkspaceControl(info, …)内部（调用在 3591 行），因此当hasNoChannelWork(info)被求值时workspaceControlInFlight ≥ 1。该谓词要求workspaceControlInFlight === 0，所以始终返回 false。idle timer 由withWorkspaceControl自身的finally块在计数器递减后正确启动。 - 在 workspace-generation 处，
runtimeOperationReservations在 IIFE 之前递增，且仅在hasNoChannelWork(info)检查的下一行由releaseRuntimeOperationReservation(…)递减，因此该谓词（要求runtimeOperationReservations === 0）始终返回 false。idle timer 由releaseRuntimeOperationReservation内部的settleReleasedRuntimeWork正确启动。
移除了两个死的 if (…) await startIdleTimer(…) 分支，以及 generation IIFE 中在移除后变为只写的 info 局部变量（保持 noUnusedLocals 通过）。无行为变化——完整的 acp-bridge 测试套件（1024 个测试）仍然通过。
延后 — 3691942500、3691942517、3691942521、3691942524。 这四项都是建议级的行为变更（超时后的 preheat 错误日志、killSync 成功后 terminateChannel 的错误传播、恢复消失服务器的 OAuth 清理、以及将尽力而为的 closeSession cancel 从 withWorkspaceControl 中解包）。每一项都会改变运行时行为，应当各自单独聚焦实现并配以针对性的回归测试，而不是塞进一个冲突解决轮次。本轮还带有预算警告（上一轮耗尽了时间预算），因此有意将范围限定为冲突加上两处经过验证、零行为变化的死代码移除。每个被延后的发现都保持其讨论串开启，并附有一条记录原因的逐条回复。
验证
git merge origin/main— 完成；一个冲突（docs/developers/daemon/17-configuration.md）通过合并双方内容解决，并用 Prettier 重新对齐npm run build— 通过npm run typecheck— 通过npm run lint— 通过npx eslint packages/acp-bridge/src/bridge.ts— 通过vitestpackages/acp-bridge（触及）— 1024 通过（20 个文件）vitestpackages/cli 触及的 serve 测试（workspace-runtime-coordinator、routes/workspace-runtime、routes/workspace-management、workspace-service facade、server/error-response）— 234 通过（5 个文件）vitestpackages/cli（run-qwen-serve、server、commands/serve）— 1097 通过（3 个文件）
Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 main 有冲突——已在本次推送中解决。
Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。
🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: PR #8213 (QwenLM/qwen-code) establishes WorkspaceRuntime ... — pointed at diff lines it never opened: it made tool calls, but none of them read the diff.
— qwen3.8-max-preview via Qwen Code /review
| if (err instanceof WorkspaceRuntimeStillStartingError) { | ||
| res.set('Retry-After', '5'); | ||
| res.status(503).json({ |
There was a problem hiding this comment.
[Suggestion] This WorkspaceRuntimeStillStartingError → 503 mapping (Retry-After: 5, code: 'runtime_still_starting') has no test asserting the response shape, while the sibling WorkspaceRuntimeInitializationError → 503 runtime_initialization_failed mapping right below it is covered in error-response.test.ts. The coordinator test only asserts the error class is thrown; no route or error-response test exercises this branch. — Concrete cost: a regression (wrong status, missing Retry-After, wrong code, or the branch reordered behind a catch-all) ships undetected and breaks a polling client's retry loop during slow workspace startup.
Mirror the existing initialization-failure test:
const { response, set, status, json } = responseMock();
sendBridgeError(
response,
new WorkspaceRuntimeStillStartingError(),
{ route: 'POST /workspace/runtime/ensure' },
);
expect(set).toHaveBeenCalledWith('Retry-After', '5');
expect(status).toHaveBeenCalledWith(503);
expect(json).toHaveBeenCalledWith({
error: 'Workspace runtime is still starting',
code: 'runtime_still_starting',
});— qwen3.8-max-preview via Qwen Code /review
|
🤖 AutoFix ran out of time before finishing (timeout (3000000ms)) (attempt 3/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/30660674462 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed[rc:3693278413] Suggestion: Add test for
|
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Address review — PR #8213Feedback triageNo new review feedback was present this round: the workflow-supplied feedback ChangesMerged
All other touched files auto-merged cleanly. Edited markdown tables were run Verification
中文说明处理评审 — PR #8213反馈分诊本轮没有新的评审反馈：工作流提供的反馈中不包含任何新于上次评估的评审评论、 变更内容将
其余所有受影响文件均自动干净合并。编辑过的 Markdown 表格已用 Prettier 规范化 验证
Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 main 有冲突——已在本次推送中解决。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
|
🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round: no action requiredNo addressable feedback exists for PR #8213 in this round:
No code changes were made and no commit was created this round. The PR head remains 中文说明Autofix 审查轮次:无需处理本轮 PR #8213 没有任何可处理的反馈:
本轮未做任何代码修改,也未创建提交。PR 头部仍为 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
|
🤖 AutoFix ran out of time before finishing (timeout (1080000ms)) (attempt 4/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/30981471291 🧠 Handled by Qwen Code · model/模型 |
|
🤖 AutoFix stopped: this counting window now contains 3 time-budget exhaustions (pushed rounds in between included; this round itself may have failed differently). That is 3 full agent runs that pushed nothing. A human should split or reduce the PR (or raise the agent time budget AND its step backstop together), then comment What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/30984968850 🧠 Handled by Qwen Code · model/模型 |
|
⏸️ Takeover paused: this PR reached its round cap (100/100). Comment 中文说明⏸️ 托管已暂停：本 PR 达到轮次上限（100/100）。评论 |
|
@qwen-code /resolve |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge summary: PR #8213 ← mainRoot causeThis PR restructured Textual or semanticSemantic in const raw = await withWorkspaceControl(ci, () =>
withTimeout(
Promise.race([
ci.connection.extMethod(SERVE_CONTROL_EXT_METHODS.sessionCd, {
sessionId,
path: req.path,
...(req.allowedRoots ? { allowedRoots: req.allowedRoots } : {}),
...(req.managedRelocation
? { managedRelocation: req.managedRelocation }
: {}),
}),
getChannelClosedReject(ci),
]),
Math.max(initTimeoutMs, 30_000),
'changeSessionCwd',
),
);
Load-bearing
Not verifiedNo build/tests ran here. (1) The 186KB SDK budget is inferred; if the combined bundle exceeds it, 中文说明根因：本 PR 重构了 语义冲突：保留 PR 的 关键点： 未验证：未运行构建/测试。186KB 为推算上界，若 CI 实测超出需再上调；main 的 managed-relocation 测试现跑在 PR 新路径上，需实际运行确认。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Unresolved, please confirm: [Critical] Maintainer overview concern 1 (comment 5151473223): requestWorkspaceStatus wrapped in withWorkspaceControl (bridge.ts:3609) — with default channelIdleTimeoutMs (0) and no keepalive lease, the first status read reaps the preheated warmth, and a status-poll BridgeTimeoutError retires the shared channel. Mechanism still present at e34cce8 and locked by the PR's own test; whether this is a defect or an accepted tradeoff is a design call awaiting maintainer direction — cannot be settled from the code alone. [Critical] Maintainer overview concern 2 (comment 5151473223): beginWorkspaceMcpDiscovery (bridge.ts:1796-1812) and cacheWorkspaceMcpDetails kill the channel on MCP discovery/detail timeout with no session guard. Still present at e34cce8, deliberate and test-locked; whether timeout => retire-channel for read-only MCP queries is a defect or accepted policy is a design call awaiting maintainer direction — cannot be settled from the code alone.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the job that exercises the integration suite this PR modifies).
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Test Plan (not a blocker): 494 tests passed — this review observed 1101, 1458, 475, 2825, 444 passed; 25 tests passed — this review observed 1101, 1458, 475, 2825, 444 passed; 306 tests passed — this review observed 1101, 1458, 475, 2825, 444 passed.
中文说明
未决，请确认：[Critical] Maintainer overview concern 1 (comment 5151473223): requestWorkspaceStatus wrapped in withWorkspaceControl (bridge.ts:3609) — with default channelIdleTimeoutMs (0) and no keepalive lease, the first status read reaps the preheated warmth, and a status-poll BridgeTimeoutError retires the shared channel. Mechanism still present at e34cce8 and locked by the PR's own test; whether this is a defect or an accepted tradeoff is a design call awaiting maintainer direction — cannot be settled from the code alone. [Critical] Maintainer overview concern 2 (comment 5151473223): beginWorkspaceMcpDiscovery (bridge.ts:1796-1812) and cacheWorkspaceMcpDetails kill the channel on MCP discovery/detail timeout with no session guard. Still present at e34cce8, deliberate and test-locked; whether timeout => retire-channel for read-only MCP queries is a defect or accepted policy is a design call awaiting maintainer direction — cannot be settled from the code alone.
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the job that exercises the integration suite this PR modifies)。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
Test Plan（非阻断）：494 tests passed — this review observed 1101, 1458, 475, 2825, 444 passed; 25 tests passed — this review observed 1101, 1458, 475, 2825, 444 passed; 306 tests passed — this review observed 1101, 1458, 475, 2825, 444 passed。
— qwen3.8-max-preview via Qwen Code /review (v0.21.6)
| // Raw exit is successful teardown after the forced signal; kill() | ||
| // supplies the bounded failure path when exit is never observed. | ||
| await Promise.race([ | ||
| channel.exited.then(() => undefined), | ||
| channel.kill(), | ||
| ]); | ||
| await terminateChannel(channel, 'channel construction failure'); |
There was a problem hiding this comment.
[Critical] Construction-failure teardown now misclassifies a successful forced teardown as a failure: killSync() SIGKILLs the child, then terminateChannel() rejects on the very signal-exit it caused (ProcessRegistry.terminateOnce treats signal-exit as unclean), so callers receive AggregateError('ACP channel construction and teardown failed') instead of the original construction error. Pre-PR, Promise.race([channel.exited.then(() => undefined), channel.kill()]) resolved on the raw exit and propagated the plain construction error. The retained comment directly above ("Raw exit is successful teardown after the forced signal") contradicts the new behavior, and downstream instanceof-based classification (mapDomainErrorToErrorKind) now sees the wrapper, not the original error. Unit tests mask this: the fake channel's kill() resolves even after killSync(). Related to the deferred thread on terminateChannel re-throwing (comment on bridge.ts:1693) — but this call site is newly rewritten by this PR. — Failure scenario: BridgeClient/ClientSideConnection construction throws while the child is alive → catch runs killSync(), then terminateChannel rejects with "ACP child pid=… exited uncleanly during shutdown (code=none, signal=SIGKILL)" → spawn callers surface a false "teardown failed" aggregate that hides the real construction error.
| // Raw exit is successful teardown after the forced signal; kill() | |
| // supplies the bounded failure path when exit is never observed. | |
| await Promise.race([ | |
| channel.exited.then(() => undefined), | |
| channel.kill(), | |
| ]); | |
| await terminateChannel(channel, 'channel construction failure'); | |
| // Raw exit is successful teardown after the forced signal; kill() | |
| // supplies the bounded failure path when exit is never observed. | |
| await Promise.race([ | |
| channel.exited.then(() => undefined), | |
| channel.kill(), | |
| ]); |
中文说明
构造失败路径的 teardown 现在会把一次成功的强制 teardown 误判为失败：killSync() 先 SIGKILL 子进程，随后 terminateChannel() 又对它自己造成的 signal-exit 抛错（ProcessRegistry.terminateOnce 把 signal-exit 视为不干净退出），于是调用方收到 AggregateError('ACP channel construction and teardown failed') 而不是原始的构造错误。改动前 Promise.race([channel.exited.then(() => undefined), channel.kill()]) 以原始退出为成功信号，传播的是普通构造错误。上方保留的注释（"Raw exit is successful teardown after the forced signal"）与新行为矛盾，且下游基于 instanceof 的分类（mapDomainErrorToErrorKind）现在看到的是包装错误而非原始错误。单测无法暴露该问题：fake channel 的 kill() 在 killSync() 之后仍然 resolve。与已延后的 terminateChannel re-throw 线程（bridge.ts:1693 的评论）相关，但本调用点是本 PR 新改写的。— 失败场景：子进程存活时 BridgeClient/ClientSideConnection 构造抛错 → catch 执行 killSync()，随后 terminateChannel 以 "ACP child pid=… exited uncleanly during shutdown (code=none, signal=SIGKILL)" 拒绝 → spawn 调用方向外暴露一个虚假的 "teardown failed" 聚合错误，掩盖真正的构造错误。
— qwen3.8-max-preview via Qwen Code /review (v0.21.6)
| > SDK 的 primary/qualified runtime 方法。第 10～13 节描述的 capability | ||
| > generation/revision、Catalog 投影和 operation 状态机仍属于后续阶段；当前 |
There was a problem hiding this comment.
[Suggestion] The §1 foundation-progress blockquote places the Target contracts (capability generation/revision, Catalog projection, operation state machine) in 第10～13节, but the Catalog projection rules actually live in §9, and §1.1 four lines below says 第9～14节 for the same content — two ranges in the same section disagree. — Failure scenario: §9 opens by splitting itself (epoch = Foundation, but "capability status 和 Catalog 规则属于 Target"); a reader navigating by the §1 blockquote is told only sections 10–13 are later-stage, so §9's Catalog projection rules read as already-dependable Foundation behavior even though the shipped status projects only lifecycle/runtimeLive/runtimeEpoch — exactly the misreading the §1.1 Foundation/Target markers were written to prevent.
| > SDK 的 primary/qualified runtime 方法。第 10～13 节描述的 capability | |
| > generation/revision、Catalog 投影和 operation 状态机仍属于后续阶段；当前 | |
| > SDK 的 primary/qualified runtime 方法。第 9～14 节描述的 capability | |
| > generation/revision、Catalog 投影和 operation 状态机仍属于后续阶段；当前 |
中文说明
§1 的 foundation 进度引用块把 Target 契约（capability generation/revision、Catalog 投影、operation 状态机）放在第 10～13 节，但 Catalog 投影规则实际在 §9，且下方四行的 §1.1 对同一内容写的是第 9～14 节——同一节内两个范围不一致。— 失败场景：§9 开篇即自我拆分（epoch 属于 Foundation，但 "capability status 和 Catalog 规则属于 Target"）；按 §1 引用块导航的读者会被告知只有 10～13 节是后续阶段，于是 §9 的 Catalog 投影规则会被读成已经可依赖的 Foundation 行为，而实际发布的 status 只投影 lifecycle/runtimeLive/runtimeEpoch——这正是 §1.1 的 Foundation/Target 标记要防止的误读。
— qwen3.8-max-preview via Qwen Code /review (v0.21.6)
|
|
||
| - **ACP** - Agent Client Protocol. JSON-RPC over stdio spoken between the daemon bridge and the ACP child process. This is not the HTTP protocol that clients use against the daemon. | ||
| - **ACP child** - the `qwen --acp` child that hosts one workspace's agent runtime. Production attempts to preheat the primary bridge and retries on first use after failure; a trusted secondary starts its child on demand, while an untrusted secondary does not. The owning bridge multiplexes sessions and clients onto that child. | ||
| - **ACP child** - the `qwen --acp` child that hosts one workspace's agent runtime. Production attempts to preheat the trusted primary child for compatibility; trusted secondaries start on their first runtime command or Session, and untrusted workspaces do not start ACP. The owning bridge multiplexes sessions and clients onto that child. |
There was a problem hiding this comment.
[Suggestion] This rewrite broadens the previously accurate "an untrusted secondary does not" to "untrusted workspaces do not start ACP", but an untrusted primary can still start its ACP child through the legacy singular session surface: routes/session.ts gates session creation on !runtime.primary && !runtime.trusted, and requirePrimarySessionRuntime has no trust check — this PR's own test "does not preheat an untrusted primary workspace" proves the untrusted-primary state is supported. Note this glossary line also drops the still-true "retries on first use after failure" guarantee (startBridgePreheat still logs "ACP preheat failed, will retry on first session"). — Failure scenario: an operator relying on the glossary as an execution-gate guarantee is misled for the primary case; a client author testing against an untrusted-primary daemon observes the child start and distrusts the doc.
| - **ACP child** - the `qwen --acp` child that hosts one workspace's agent runtime. Production attempts to preheat the trusted primary child for compatibility; trusted secondaries start on their first runtime command or Session, and untrusted workspaces do not start ACP. The owning bridge multiplexes sessions and clients onto that child. | |
| - **ACP child** - the `qwen --acp` child that hosts one workspace's agent runtime. Production attempts to preheat the trusted primary child for compatibility and retries on first use after failure; trusted secondaries start on their first runtime command or Session, and untrusted secondaries never start ACP (an untrusted primary still starts its child through the legacy singular Session compatibility surface). The owning bridge multiplexes sessions and clients onto that child. |
中文说明
本次改写把原本准确的 "an untrusted secondary does not" 放宽为 "untrusted workspaces do not start ACP"，但不受信任的 primary 仍可以通过 legacy singular session 入口启动其 ACP 子进程：routes/session.ts 的 session 创建门控是 !runtime.primary && !runtime.trusted，requirePrimarySessionRuntime 没有信任检查——本 PR 自己的测试 "does not preheat an untrusted primary workspace" 证明不受信任的 primary 是被支持的状态。另外该词条行还丢掉了仍然成立的 "retries on first use after failure" 保证（startBridgePreheat 仍会打印 "ACP preheat failed, will retry on first session"）。— 失败场景：把词条当作执行门控保证的运维会在 primary 场景被误导；对着不受信任 primary daemon 测试的客户端作者会观察到子进程被启动从而不信任文档。
— qwen3.8-max-preview via Qwen Code /review (v0.21.6)
| `packages/acp-bridge/` owns the boundary between the daemon's HTTP layer and the ACP child process. It is consumed by `packages/cli/src/serve/` (the `qwen serve` daemon) and was extracted in #4175 F1 step 3 so future consumers (`channels/base/AcpBridge.ts`, the VS Code IDE companion) can use the same bridge core without reaching into the CLI package. | ||
|
|
||
| Each active `WorkspaceRuntime` owns one `HttpAcpBridge` instance. Production attempts to preheat the primary bridge and retries on first use after failure. A trusted secondary opens its `AcpChannel` and starts its child on demand; an untrusted secondary cannot start ACP. Within the runtime, the bridge provides multiplexed sessions over the channel, per-session `EventBus`es, a `MultiClientPermissionMediator`, a `BridgeFileSystem` adapter, and ACP-oriented helpers (`spawnOrAttach`, `loadSession`, `resumeSession`, `sendPrompt`, `cancelSession`, `respondToPermission`, plus extMethod RPCs for workspace status and MCP restart). Bridges and children are never shared across workspace runtimes. | ||
| Each active `WorkspaceRuntime` owns one `HttpAcpBridge` instance. Production attempts to preheat the trusted primary child for compatibility; trusted secondaries open their `AcpChannel` on the first runtime command or Session, and untrusted workspaces cannot start ACP. Within the runtime, the bridge provides multiplexed sessions over the channel, per-session `EventBus`es, a `MultiClientPermissionMediator`, a `BridgeFileSystem` adapter, and ACP-oriented helpers (`spawnOrAttach`, `loadSession`, `resumeSession`, `sendPrompt`, `cancelSession`, `respondToPermission`, plus extMethod RPCs for workspace status and MCP restart). Bridges and children are never shared across workspace runtimes. |
There was a problem hiding this comment.
[Suggestion] "untrusted workspaces cannot start ACP" overgeneralizes: an untrusted primary still starts its child through the legacy singular session routes (routes/session.ts gates only !runtime.primary && !runtime.trusted; requirePrimarySessionRuntime has no trust check). This intro also drops the still-true retry guarantee, and now contradicts this same file's unchanged §State & Lifecycle line ("A failed preheat leaves first use free to retry"). — Failure scenario: a developer reading this canonical bridge doc concludes session creation on an untrusted primary cannot spawn a child — the opposite of the degraded-but-functional mode the code implements (writes rejected with untrusted_workspace, sessions served). Suggested fix: "…and untrusted secondaries cannot start ACP (an untrusted primary still starts its child through the legacy singular Session compatibility surface, and failed preheat retries on first use)."
中文说明
"untrusted workspaces cannot start ACP" 过度泛化：不受信任的 primary 仍会通过 legacy singular session 路由启动子进程（routes/session.ts 只门控 !runtime.primary && !runtime.trusted；requirePrimarySessionRuntime 没有信任检查）。该引言段还丢掉了仍然成立的重试保证，并与本文件未改动的 §State & Lifecycle 一行（"A failed preheat leaves first use free to retry"）矛盾。— 失败场景：阅读这份权威 bridge 文档的开发者会得出"不受信任 primary 上创建 session 不会拉起子进程"的结论——与代码实际实现的降级但可用模式（写操作以 untrusted_workspace 拒绝、session 正常服务）相反。建议改为："…and untrusted secondaries cannot start ACP (an untrusted primary still starts its child through the legacy singular Session compatibility surface, and failed preheat retries on first use)."
— qwen3.8-max-preview via Qwen Code /review (v0.21.6)
| ## Overview | ||
|
|
||
| `McpTransportPool` (`packages/core/src/tools/mcp-transport-pool.ts`) is the F2 (#4175 commit 5) workspace-scoped pool: multiple ACP sessions inside one runtime share one transport per unique `(serverName + configFingerprint)` tuple, instead of each spawning its own MCP child process. When pool mode is enabled, every started ACP child owns an independent pool (`QwenAgent.mcpPool`). Production attempts to preheat the primary child and retries on first use after failure; a trusted secondary starts its child on demand, while an untrusted secondary starts neither. The pool is constructed once at agent startup with the runtime's bootstrap `Config` and survives session lifecycles. Entries reference-count session attaches and close after a configurable grace period when the reference count reaches zero. | ||
| `McpTransportPool` (`packages/core/src/tools/mcp-transport-pool.ts`) is the F2 (#4175 commit 5) workspace-scoped pool: multiple ACP sessions inside one runtime share one transport per unique `(serverName + configFingerprint)` tuple, instead of each spawning its own MCP child process. When pool mode is enabled, every started ACP child owns an independent pool (`QwenAgent.mcpPool`). Production attempts to preheat the trusted primary child for compatibility; trusted secondaries start on demand, and an untrusted workspace starts neither the child nor its pool. The pool is constructed once at agent startup with the runtime's bootstrap `Config` and survives session lifecycles. Entries reference-count session attaches and close after a configurable grace period when the reference count reaches zero. |
There was a problem hiding this comment.
[Suggestion] The rewrite broadens "an untrusted secondary starts neither" to "an untrusted workspace starts neither the child nor its pool", but an untrusted primary still starts its child on demand when a session is created (legacy singular routes gate only !runtime.primary && !runtime.trusted), and the child constructs its McpTransportPool unconditionally at agent startup. — Failure scenario: a developer reading this page concludes session creation on an untrusted primary cannot spawn a child/pool — the opposite of the degraded-but-functional mode the code implements. Suggested fix: "…trusted secondaries start on demand, and an untrusted secondary starts neither the child nor its pool (an untrusted primary skips preheat but still starts its child on first session)."
中文说明
本次改写把 "an untrusted secondary starts neither" 放宽为 "an untrusted workspace starts neither the child nor its pool"，但不受信任的 primary 在创建 session 时仍会按需启动子进程（legacy singular 路由只门控 !runtime.primary && !runtime.trusted），且子进程在 agent 启动时无条件构造 McpTransportPool。— 失败场景：阅读本页的开发者会得出"不受信任 primary 上创建 session 不会拉起子进程/池"的结论——与代码实际实现的降级但可用模式相反。建议改为："…trusted secondaries start on demand, and an untrusted secondary starts neither the child nor its pool (an untrusted primary skips preheat but still starts its child on first session)."
— qwen3.8-max-preview via Qwen Code /review (v0.21.6)
| if (err instanceof WorkspaceRuntimeStillStartingError) { | ||
| reportBridgeError(err, ctx, daemonLog); | ||
| res.set('Retry-After', '5'); |
There was a problem hiding this comment.
[Suggestion] The retryable runtime_still_starting 503 is routed through reportBridgeError (recordDaemonBridgeError + recordDaemonError + error-level daemon log / stderr stack), while the sibling retryable 503 workspace_draining a few lines below (~313) is sent silently with the same Retry-After treatment. WorkspaceRuntimeStillStartingError is the designed outcome when the 60s observer budget expires while the bounded physical startup continues — the protocol doc tells clients to poll status or call ensure again. Earlier thread on bridge.ts:236 asked for parity with WorkspaceRuntimeInitializationError and the autofix round added full error reporting; the noise tradeoff that thread itself mentioned was not implemented. — Failure scenario: in the cold-container deployments the docs explicitly anticipate (raising --initialize-timeout-ms for child startup headroom), every ensure call inside the startup window returns runtime_still_starting, and each hit records daemon-error metrics and an error-level log line, multiplied by every polling client — operators alerting on daemon error metrics get paged for a design-sanctioned state.
| if (err instanceof WorkspaceRuntimeStillStartingError) { | |
| reportBridgeError(err, ctx, daemonLog); | |
| res.set('Retry-After', '5'); | |
| if (err instanceof WorkspaceRuntimeStillStartingError) { | |
| res.set('Retry-After', '5'); |
中文说明
可重试的 runtime_still_starting 503 走了 reportBridgeError（recordDaemonBridgeError + recordDaemonError + error 级 daemon 日志 / stderr 堆栈），而下方几行（~313）同样可重试的 503 workspace_draining 却静默返回、带相同的 Retry-After 处理。WorkspaceRuntimeStillStartingError 是 60s 观察预算耗尽而有界物理启动仍在继续时的设计内结果——协议文档明确告知客户端轮询 status 或再次调用 ensure。早先 bridge.ts:236 的线程要求与 WorkspaceRuntimeInitializationError 对齐，autofix 轮次加上了完整的错误上报；但该线程自己也提到的噪音折中方案并未实现。— 失败场景：在文档明确预期的冷容器部署中（调大 --initialize-timeout-ms 给子进程启动余量），启动窗口内的每次 ensure 调用都返回 runtime_still_starting，每次命中都记录 daemon 错误指标和 error 级日志，再乘以每个轮询客户端——对 daemon 错误指标设了告警的操作者会被一个设计内状态反复呼叫。
— qwen3.8-max-preview via Qwen Code /review (v0.21.6)
| 页面不存在 capability-selecting prepare，也不调用 | ||
| `refreshWorkspaceConfigExtensions()` 作为新架构 runtime command。 |
There was a problem hiding this comment.
[Suggestion] §13.2's normative prohibition names refreshWorkspaceConfigExtensions(), a symbol that exists nowhere in the repository (exactly one occurrence repo-wide: this doc line). The real legacy refresh APIs are DaemonClient.refreshExtensions() (POST /workspace/extensions/refresh) and WorkspaceDaemonClient.refreshExtensionRuntime() (POST /workspaces/:workspace/extensions/refresh); §13.1 of this same document correctly names /workspace/extensions/refresh. — Failure scenario: this section is the Web Shell migration contract; an implementer or reviewer verifying the rule greps for refreshWorkspaceConfigExtensions, gets zero hits, and concludes the prohibition is satisfied vacuously even if the admin pages still invoke the real legacy refresh as a runtime command — the exact behavior this section exists to rule out. The migration gate cannot be checked against a phantom symbol.
| 页面不存在 capability-selecting prepare，也不调用 | |
| `refreshWorkspaceConfigExtensions()` 作为新架构 runtime command。 | |
| 页面不存在 capability-selecting prepare，也不调用 | |
| `refreshExtensions()` / `refreshExtensionRuntime()`（旧 `/workspace/extensions/refresh`、`/workspaces/:workspace/extensions/refresh`）作为新架构 runtime command。 |
中文说明
§13.2 的规范性禁令写了 refreshWorkspaceConfigExtensions()，但该符号在整个仓库中不存在（全仓库唯一一次出现就是本行文档）。真实的 legacy refresh API 是 DaemonClient.refreshExtensions()（POST /workspace/extensions/refresh）和 WorkspaceDaemonClient.refreshExtensionRuntime()（POST /workspaces/:workspace/extensions/refresh）；本文档 §13.1 也正确地写了 /workspace/extensions/refresh。— 失败场景：本节是 Web Shell 迁移契约；验证该规则的实现者或评审者 grep refreshWorkspaceConfigExtensions 得到零结果，即使管理页面仍在以 runtime command 形式调用真实的 legacy refresh，也会得出"禁令已被满足"的空洞结论——这正是本节要排除的行为。迁移门控无法对着一个幽灵符号检查。
— qwen3.8-max-preview via Qwen Code /review (v0.21.6)
| | `permissionConsensusQuorum` | from `settings.json` | N for consensus policy. | | ||
| | `permissionAudit` | `createNoOpPermissionAuditPublisher()` | Wire to `PermissionAuditRing` for the audit trail. | | ||
| | `channelIdleTimeoutMs` | `0` | Keep the ACP child alive for this many milliseconds after the last session closes. | | ||
| | `channelIdleTimeoutMs` | `0` | Auto-reap delay after all workspace runtime work drains; unset or `0` reaps immediately. | |
There was a problem hiding this comment.
[Suggestion] This PR broadens initializeTimeoutMs to a shared channel-factory + initialize startup deadline (plus default request timeout) and rewrites its blurb in four places — but leaves the stale blurb "ACP initialize handshake timeout." in the initializeTimeoutMs row of this very BridgeOptions table (line ~202, directly above the rewritten channelIdleTimeoutMs row anchored here) and in the DEFAULT_INIT_TIMEOUT_MS row of docs/developers/daemon/17-configuration.md:153 (a file where the same diff updates two other descriptions of this option). — Failure scenario: the new startup code (startupStartedAt / remainingStartupMs, added by this diff) means the factory spawn now consumes the same budget as the handshake; a developer following the canonical bridge reference sees "initialize handshake timeout", assumes the handshake gets the full 10s, and cannot diagnose why a slow spawn (e.g. 9s cold start) leaves ~1s for the handshake and fails initialize; the stale rows also never mention the same knob bounds newSession/workspace-status/MCP ext-method requests. Suggested fix: update both rows to the wording used elsewhere in this PR — "ACP child startup deadline (channel factory + initialize handshake) and default request timeout."
中文说明
本 PR 把 initializeTimeoutMs 扩展为 channel factory + initialize 共享的启动期限（兼作默认请求超时），并在四处改写了它的说明——但在本 BridgeOptions 表的 initializeTimeoutMs 行（~202 行，就在锚定的已改写 channelIdleTimeoutMs 行上方）仍留着过时的 "ACP initialize handshake timeout."，docs/developers/daemon/17-configuration.md:153 的 DEFAULT_INIT_TIMEOUT_MS 行也是如此（同一 diff 在该文件里更新了该选项的另外两处描述）。— 失败场景：新的启动代码（startupStartedAt / remainingStartupMs，本 diff 新增）意味着 factory spawn 现在与握手共享同一预算；按权威 bridge 参考表行事的开发者看到 "initialize handshake timeout"，会以为握手有完整 10s，无法诊断为何慢 spawn（如 9s 冷启动）只剩 ~1s 给握手并导致 initialize 失败；过时行也从未提到同一个参数还约束 newSession/workspace-status/MCP ext-method 请求。建议：把这两行更新为本 PR 其他位置使用的措辞——"ACP child startup deadline (channel factory + initialize handshake) and default request timeout."
— qwen3.8-max-preview via Qwen Code /review (v0.21.6)
| workspaceRuntimeAvailable: () => { | ||
| const runtimes = workspaceRegistry.list(); | ||
| return ( |
There was a problem hiding this comment.
[Suggestion] The dynamic workspace_runtime capability closure (registry non-empty AND every runtime's bridge supports lifecycle) has no test pinning either condition — no app-level /capabilities test asserts the feature's absence. Verified by mutation: flipping this closure to () => true keeps all 867 server.test.ts tests green (the 'returns the v1 envelope' test computes its expected set with workspaceRuntimeAvailable: true, matching the happy path; the integration baseline only checks the positive case and is merge_group-gated); a temporary probe asserting absence for an injected legacy bridge fails under the mutation and passes with the real closure. The sibling capability closures directly above each have dedicated support-conditional tests ('advertises session generation only when…', 'advertises workspace generation only when…'). — Failure scenario: the guard regresses to () => true (or .every → .some) green; the daemon then advertises workspace_runtime in exactly the scenario the guard exists for — an embedded/legacy injected bridge without getWorkspaceRuntimeLifecycleSnapshot — and capability-gated SDK clients hit 501 workspace_runtime_not_supported on every ensure/status call. Suggested fix: add a createServeApp test in the sibling style — build the app with a bridge/registry lacking getWorkspaceRuntimeLifecycleSnapshot and assert features does not contain 'workspace_runtime', plus the positive counterpart.
中文说明
动态的 workspace_runtime capability 闭包（registry 非空且每个 runtime 的 bridge 都支持 lifecycle）没有任何测试钉住其中任一条件——没有 app 级 /capabilities 测试断言该特性的缺失。变异验证：把该闭包改成 () => true 后，server.test.ts 全部 867 个测试仍通过（'returns the v1 envelope' 测试以 workspaceRuntimeAvailable: true 计算期望集合，与 happy path 相符；集成基线只检查正例且由 merge_group 门控）；临时补写的"注入 legacy bridge 时断言缺失"探针在该变异下失败、恢复真实闭包后通过。上方的兄弟 capability 闭包各有专门的条件测试（'advertises session generation only when…'、'advertises workspace generation only when…'）。— 失败场景：该守卫在全绿状态下退化为 () => true（或 .every → .some）；daemon 随后恰好在该守卫存在的理由场景——注入的、没有 getWorkspaceRuntimeLifecycleSnapshot 的 embedded/legacy bridge——宣告 workspace_runtime，capability 门控的 SDK 客户端每次 ensure/status 都会撞上 501 workspace_runtime_not_supported。建议：仿照兄弟测试新增一个 createServeApp 测试——用缺少 getWorkspaceRuntimeLifecycleSnapshot 的 bridge/registry 构建 app，断言 features 不含 'workspace_runtime'，并补上正向对照。
— qwen3.8-max-preview via Qwen Code /review (v0.21.6)
| '/runtime/ensure', | ||
| 'POST /workspaces/:workspace/runtime/ensure', | ||
| { | ||
| method: 'POST', | ||
| timeoutMs: WORKSPACE_RUNTIME_ENSURE_TIMEOUT_MS, |
There was a problem hiding this comment.
[Suggestion] The new fake-timer test pins the 62s "server deadline + client headroom" budget only for the primary ensureWorkspaceRuntime(); this qualified WorkspaceDaemonClient.ensureRuntime() has no test touching its timeoutMs. Verified by mutation: deleting the timeoutMs: option here keeps the entire SDK suite (1458 tests) green — it compiles (all jsonRequest opts are optional) and raises no lint (the constant is still read by the primary method); a probe mirroring the primary headroom test for this qualified client fails under the mutation (requestSignal.aborted is true at 1ms client default instead of holding the 62s budget) and passes with the original. — Failure scenario: when that regression ships, workspace.ensureRuntime() falls back to DEFAULT_FETCH_TIMEOUT_MS (30s) against a daemon whose own ensure budget is 60s: a cold runtime whose spawn + initialize takes 31–60s is aborted client-side at 30s with a TimeoutError while the daemon finishes the ensure successfully seconds later. Suggested fix: run the qualified path through the same oracle in the headroom test — after the primary assertion, const qualified = client.workspaceByCwd('/work/a').ensureRuntime(); with an equally slow fetch, advance 61_999 ms, and assert the request signal is not aborted (or parameterize the existing test over both entry points).
中文说明
新的 fake-timer 测试只为 primary 的 ensureWorkspaceRuntime() 钉住了 62s 的"服务端期限 + 客户端余量"预算；这个 qualified 的 WorkspaceDaemonClient.ensureRuntime() 没有任何测试触碰它的 timeoutMs。变异验证：删除此处的 timeoutMs: 选项后，整个 SDK 套件（1458 个测试）仍全绿——它能编译（jsonRequest 的所有选项都可选）、也不触发 lint（该常量仍被 primary 方法读取）；为这个 qualified 客户端镜像 primary 余量测试的探针在该变异下失败（requestSignal.aborted 在 1ms 客户端默认值处即为 true，而不是撑住 62s 预算），恢复原代码后通过。— 失败场景：该回归合入后，workspace.ensureRuntime() 会退回 DEFAULT_FETCH_TIMEOUT_MS（30s），而 daemon 自己的 ensure 预算是 60s：spawn + initialize 需要 31–60s 的冷 runtime 会在 30s 时被客户端以 TimeoutError 中止，而 daemon 几秒后就成功完成了 ensure。建议：在余量测试中让 qualified 路径走同一个预言——primary 断言之后，用同样慢的 fetch 执行 const qualified = client.workspaceByCwd('/work/a').ensureRuntime();，advance 61_999 ms，断言请求信号未被中止（或把现有测试参数化到两个入口）。
— qwen3.8-max-preview via Qwen Code /review (v0.21.6)
|
@qwen-code /resolve |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge resolution: PR #8213 ← mainRoot cause. Main's Semantic, not textual (bridge.ts; docs/tests were mostly unions):
void settlementPromise.finally(() => {
const current = inFlightRestores.get(req.sessionId);
if (current?.settlementPromise === settlementPromise) {
inFlightRestores.delete(req.sessionId);
void settleReleasedRuntimeWork('session restore', false);
}
});
Load-bearing.
Could not verify. No build/typecheck/tests run here; the 187KB SDK budget is an estimate (only the sdk build gate is affected if the real bundle differs). 中文说明根因：main 的 语义冲突（bridge.ts）：① restore 清理保留 #8691 的"结算后再删 inFlightRestores 条目"，删除后调用 PR 的 关键约束：必须先删 inFlightRestores 条目再 settle；不恢复 未能验证：不跑构建/测试；187KB 为估算；两测试文件中双方测试现已并存（run-qwen-serve.test.ts 中两个测试曾被 git 交错，已按原文重建），需 CI 确认。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Unresolved, please confirm:
- [Critical] Maintainer overview concern 1 (comment 5151473223; related thread 3699909386) — status reads reap the compat-preheat warmth and status-poll re-arms the idle timer: mechanism present at the reviewed commit and test-locked; whether it is a defect or an accepted tradeoff still awaits the maintainer direction the review has been waiting on since round 2 — cannot be settled from code alone.
- [Critical] Maintainer overview concern 2 / review #2 High 2 (comments 5151473223, 5157924428) — MCP discovery timeout kills the channel with no session guard: mechanism present at the reviewed commit and test-locked; awaiting maintainer direction.
- [Critical] Review #2 High 3 (comment 5157924428; thread 3691942524) — vanished-server auth drain kills the owning channel: mechanism present; deferred to a follow-up by the author.
Not reviewed: reverse audit — stopped at the 3-round cap for huge diffs; rounds 2 and 3 still surfaced findings, so two consecutive dry rounds were never reached (all chunks audited rounds 1-3; chunk 16 retired on its two-dry certificate).
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — completed the walkthrough. I did not separately verify the §16.1-4/16.1-5 test coverage claims (channel factory deadline, OAuth safe-drain test existence…; chunk 3: nothing material — I did not verify §11.1's "并发 ensure 复用 Bridge 启动 Promise；每个成功调用从自己的成功时刻续期 keepalive" (concurrent ensure reuses the Bridge startup Promise; ea…; chunk 16: Did not run workspace-runtime.test.ts under vitest (worktree has no node_modules; skipped npm install due to budget) — instead verified the assertions by read…; chunk 16: did not execute workspace-runtime.test.ts under vitest (worktree has no node_modules; npm install skipped at the budget ceiling) — assertions were instead v…; chunk 4: I did not individually verify that each automated test claimed in design doc §16.1 items 1–7 (OAuth pending-lease/safe-drain tests, SDK REST-route/timeout tests…, and 17 more.
Test Plan (not a blocker): 25 tests passed — this review observed 1274, 19142, 1513, 494, 3277, 523 passed; 306 tests passed — this review observed 1274, 19142, 1513, 494, 3277, 523 passed.
[Critical] R5-1 — packages/acp-bridge/src/bridge.ts:3019 — still stands at the reviewed commit (re-verified this round by code trace; first reported in round 5, existing inline thread; independently re-derived this round by two separate audit agents). This PR replaced the construction-failure path's Promise.race([channel.exited.then(() => undefined), channel.kill()]) with await terminateChannel(channel, 'channel construction failure'). The catch calls channel.killSync() (SIGKILL) first; terminateChannel then rejects on the very signal-exit that killSync caused (ProcessRegistry.terminateOnce treats a signal exit as unclean), so callers receive AggregateError('ACP channel construction and teardown failed') instead of the original construction error, and downstream instanceof-based classification sees the wrapper, not the original error. The retained comment directly above ("Raw exit is successful teardown after the forced signal") contradicts the new behavior. Unit tests mask this: the fake channel's kill() resolves even after killSync(). Suggested fix: race the forced exit as success here (await Promise.race([channel.exited.then(() => undefined), terminateChannel(channel, 'channel construction failure')])), or make terminateOnce treat a signal-exit that follows its own killSync as success; otherwise update the retained comment to state the new semantics.
中文说明
[Critical] R5-1 — packages/acp-bridge/src/bridge.ts:3019 — 在受审 commit 上仍然存在（本轮经代码追踪复核；首次报告于第 5 轮，已有行内线程；本轮另有两个独立审计 agent 重新推导出同一机制）。本 PR 把构造失败路径的 Promise.race([channel.exited.then(() => undefined), channel.kill()]) 替换为 await terminateChannel(channel, 'channel construction failure')。catch 先调用 channel.killSync()（SIGKILL）；随后 terminateChannel 恰好对它自己造成的 signal-exit 抛错（ProcessRegistry.terminateOnce 把 signal 退出视为不干净），于是调用方收到 AggregateError('ACP channel construction and teardown failed') 而不是原始构造错误，下游基于 instanceof 的分类看到的是包装错误而非原始错误。上方保留的注释（"Raw exit is successful teardown after the forced signal"）与新行为矛盾。单测无法暴露该问题：fake channel 的 kill() 在 killSync() 之后仍然 resolve。建议修复：在此处把强制退出竞速为成功（await Promise.race([channel.exited.then(() => undefined), terminateChannel(channel, 'channel construction failure')])），或让 terminateOnce 把紧随自身 killSync 之后的 signal-exit 视为成功；否则更新保留的注释以声明新语义。
[Critical] R6-1 — packages/acp-bridge/src/bridge.ts:2266-2269 (paired sites bridge.ts:5742, :5608, :2349/:2358) — the PR-added outerRestoreCount term in hasNoChannelWork counts the subject restore's own inFlightRestores entry at decision sites that never pass ignoreRestoreId (the abandon timer's kill_empty determination, the quarantine kill in settleAbandonedRestore, and reapPendingEmptyChannel armed from armRestoreSettlementGrace). An abandoned restore's entry stays in inFlightRestores until physical settlement — which happens only when the transport closes — which happens only when the channel is killed — which these gates now permanently block. Failure scenario: session/load on an empty channel whose child wedges -> at the deadline kill_empty is dead code, the grace reap is likewise blocked, the channel is never killed, the session id stays fenced, and every new spawn/restore/branch throws BridgeChannelQuarantinedError('restore_settlement_overdue') until daemon restart. Measured by this review's build & test run + base-tree test-delta: the acp-bridge suite is red on this PR (7 tests fail in bridge.test.ts, all green at the merge base; netNew), including the five main restore-recovery tests and the two PR-added 'retires a shared channel when a timed-out load/resume session RPC remains physically pending' variants. The spawn side is handled correctly (ignoreCurrentSessionSpawn), so the defect is restore-specific; the fix must also reconcile the two PR-added restore-retire tests with main's merged restore-timeout budget semantics (resolveSessionRestoreTimeoutMs floor). Suggested fix: pass { ignoreRestoreId: req.sessionId } at bridge.ts:5742 and :5608, and forward an ignore option from armRestoreSettlementGrace through reapPendingEmptyChannel (mirroring the three restore paths that already do this: bridge.ts:5808, :5813, :6012). (Not anchored inline: the gate lines collide with the round-1 blocker thread at bridge.ts:2269, which covered the now-fixed spawn side of this same gate.)
中文说明
[Critical] R6-1 — packages/acp-bridge/src/bridge.ts:2266-2269（配对位点 bridge.ts:5742、:5608、:2349/:2358）——本 PR 在 hasNoChannelWork 中新增的 outerRestoreCount 项，在未传 ignoreRestoreId 的判定位点把当事 restore 自己的 inFlightRestores 条目也计入了（abandon timer 的 kill_empty 判定、settleAbandonedRestore 中的隔离 kill、以及 armRestoreSettlementGrace 启动的 reapPendingEmptyChannel）。被放弃 restore 的条目会一直留在 inFlightRestores 中直到物理结算——而物理结算只在 transport 关闭时发生——transport 只在 channel 被杀时关闭——而这些门控现在永久阻止了这次 kill。失败场景：空 channel 上的 session/load 遇到卡死的 child -> 到达期限时 kill_empty 是死代码，grace reap 同样被阻塞，channel 永不被杀，session id 一直被 fence，之后每一次 spawn/restore/branch 都会抛 BridgeChannelQuarantinedError('restore_settlement_overdue')，直到 daemon 重启。本审查的 build & test + base-tree test-delta 实测：acp-bridge 套件在本 PR 上变红（bridge.test.ts 中 7 个测试失败，在 merge base 上全绿；netNew），包括 5 个主要的 restore 恢复测试和 2 个 PR 新增的 'retires a shared channel when a timed-out load/resume session RPC remains physically pending' 变体。spawn 侧处理正确（ignoreCurrentSessionSpawn），因此缺陷是 restore 特有的；修复还必须让 PR 新增的两个 restore-retire 测试与 main 合入的 restore-timeout 预算语义（resolveSessionRestoreTimeoutMs 下限）对齐。建议修复：在 bridge.ts:5742 与 :5608 传入 { ignoreRestoreId: req.sessionId }，并把 ignore 选项从 armRestoreSettlementGrace 透传到 reapPendingEmptyChannel（与已经这样做的三个 restore 路径一致：bridge.ts:5808、:5813、:6012）。（未行内锚定：gate 所在行与第 1 轮 blocker 线程在 bridge.ts:2269 冲突，该线程覆盖的是同一 gate 现已修复的 spawn 侧。）
中文说明
未决，请确认：共 3 条（原文未翻译，列表见上方英文部分）。
未审查：reverse audit — stopped at the 3-round cap for huge diffs; rounds 2 and 3 still surfaced findings, so two consecutive dry rounds were never reached (all chunks audited rounds 1-3; chunk 16 retired on its two-dry certificate)。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — completed the walkthrough. I did not separately verify the §16.1-4/16.1-5 test coverage claims (channel factory deadline, OAuth safe-drain test existence…；chunk 3：nothing material — I did not verify §11.1's "并发 ensure 复用 Bridge 启动 Promise；每个成功调用从自己的成功时刻续期 keepalive" (concurrent ensure reuses the Bridge startup Promise; ea…；chunk 16：Did not run workspace-runtime.test.ts under vitest (worktree has no node_modules; skipped npm install due to budget) — instead verified the assertions by read…；chunk 16：did not execute workspace-runtime.test.ts under vitest (worktree has no node_modules; npm install skipped at the budget ceiling) — assertions were instead v…；chunk 4：I did not individually verify that each automated test claimed in design doc §16.1 items 1–7 (OAuth pending-lease/safe-drain tests, SDK REST-route/timeout tests…，另有 17 条。
Test Plan（非阻断）：25 tests passed — this review observed 1274, 19142, 1513, 494, 3277, 523 passed; 306 tests passed — this review observed 1274, 19142, 1513, 494, 3277, 523 passed。
[Critical] R5-1 — packages/acp-bridge/src/bridge.ts:3019 — still stands at the reviewed commit (re-verified this round by code trace; first reported in round 5, existing inline thread; independently re-derived this round by two separate audit agents). This PR replaced the construction-failure path's Promise.race([channel.exited.then(() => undefined), channel.kill()]) with await terminateChannel(channel, 'channel construction failure'). The catch calls channel.killSync() (SIGKILL) first; terminateChannel then rejects on the very signal-exit that killSync caused (ProcessRegistry.terminateOnce treats a signal exit as unclean), so callers receive AggregateError('ACP channel construction and teardown failed') instead of the original construction error, and downstream instanceof-based classification sees the wrapper, not the original error. The retained comment directly above ("Raw exit is successful teardown after the forced signal") contradicts the new behavior. Unit tests mask this: the fake channel's kill() resolves even after killSync(). Suggested fix: race the forced exit as success here (await Promise.race([channel.exited.then(() => undefined), terminateChannel(channel, 'channel construction failure')])), or make terminateOnce treat a signal-exit that follows its own killSync as success; otherwise update the retained comment to state the new semantics.
中文说明
[Critical] R5-1 — packages/acp-bridge/src/bridge.ts:3019 — 在受审 commit 上仍然存在（本轮经代码追踪复核；首次报告于第 5 轮，已有行内线程；本轮另有两个独立审计 agent 重新推导出同一机制）。本 PR 把构造失败路径的 Promise.race([channel.exited.then(() => undefined), channel.kill()]) 替换为 await terminateChannel(channel, 'channel construction failure')。catch 先调用 channel.killSync()（SIGKILL）；随后 terminateChannel 恰好对它自己造成的 signal-exit 抛错（ProcessRegistry.terminateOnce 把 signal 退出视为不干净），于是调用方收到 AggregateError('ACP channel construction and teardown failed') 而不是原始构造错误，下游基于 instanceof 的分类看到的是包装错误而非原始错误。上方保留的注释（"Raw exit is successful teardown after the forced signal"）与新行为矛盾。单测无法暴露该问题：fake channel 的 kill() 在 killSync() 之后仍然 resolve。建议修复：在此处把强制退出竞速为成功（await Promise.race([channel.exited.then(() => undefined), terminateChannel(channel, 'channel construction failure')])），或让 terminateOnce 把紧随自身 killSync 之后的 signal-exit 视为成功；否则更新保留的注释以声明新语义。
[Critical] R6-1 — packages/acp-bridge/src/bridge.ts:2266-2269 (paired sites bridge.ts:5742, :5608, :2349/:2358) — the PR-added outerRestoreCount term in hasNoChannelWork counts the subject restore's own inFlightRestores entry at decision sites that never pass ignoreRestoreId (the abandon timer's kill_empty determination, the quarantine kill in settleAbandonedRestore, and reapPendingEmptyChannel armed from armRestoreSettlementGrace). An abandoned restore's entry stays in inFlightRestores until physical settlement — which happens only when the transport closes — which happens only when the channel is killed — which these gates now permanently block. Failure scenario: session/load on an empty channel whose child wedges -> at the deadline kill_empty is dead code, the grace reap is likewise blocked, the channel is never killed, the session id stays fenced, and every new spawn/restore/branch throws BridgeChannelQuarantinedError('restore_settlement_overdue') until daemon restart. Measured by this review's build & test run + base-tree test-delta: the acp-bridge suite is red on this PR (7 tests fail in bridge.test.ts, all green at the merge base; netNew), including the five main restore-recovery tests and the two PR-added 'retires a shared channel when a timed-out load/resume session RPC remains physically pending' variants. The spawn side is handled correctly (ignoreCurrentSessionSpawn), so the defect is restore-specific; the fix must also reconcile the two PR-added restore-retire tests with main's merged restore-timeout budget semantics (resolveSessionRestoreTimeoutMs floor). Suggested fix: pass { ignoreRestoreId: req.sessionId } at bridge.ts:5742 and :5608, and forward an ignore option from armRestoreSettlementGrace through reapPendingEmptyChannel (mirroring the three restore paths that already do this: bridge.ts:5808, :5813, :6012). (Not anchored inline: the gate lines collide with the round-1 blocker thread at bridge.ts:2269, which covered the now-fixed spawn side of this same gate.)
中文说明
[Critical] R6-1 — packages/acp-bridge/src/bridge.ts:2266-2269（配对位点 bridge.ts:5742、:5608、:2349/:2358）——本 PR 在 hasNoChannelWork 中新增的 outerRestoreCount 项，在未传 ignoreRestoreId 的判定位点把当事 restore 自己的 inFlightRestores 条目也计入了（abandon timer 的 kill_empty 判定、settleAbandonedRestore 中的隔离 kill、以及 armRestoreSettlementGrace 启动的 reapPendingEmptyChannel）。被放弃 restore 的条目会一直留在 inFlightRestores 中直到物理结算——而物理结算只在 transport 关闭时发生——transport 只在 channel 被杀时关闭——而这些门控现在永久阻止了这次 kill。失败场景：空 channel 上的 session/load 遇到卡死的 child -> 到达期限时 kill_empty 是死代码，grace reap 同样被阻塞，channel 永不被杀，session id 一直被 fence，之后每一次 spawn/restore/branch 都会抛 BridgeChannelQuarantinedError('restore_settlement_overdue')，直到 daemon 重启。本审查的 build & test + base-tree test-delta 实测：acp-bridge 套件在本 PR 上变红（bridge.test.ts 中 7 个测试失败，在 merge base 上全绿；netNew），包括 5 个主要的 restore 恢复测试和 2 个 PR 新增的 'retires a shared channel when a timed-out load/resume session RPC remains physically pending' 变体。spawn 侧处理正确（ignoreCurrentSessionSpawn），因此缺陷是 restore 特有的；修复还必须让 PR 新增的两个 restore-retire 测试与 main 合入的 restore-timeout 预算语义（resolveSessionRestoreTimeoutMs 下限）对齐。建议修复：在 bridge.ts:5742 与 :5608 传入 { ignoreRestoreId: req.sessionId }，并把 ignore 选项从 armRestoreSettlementGrace 透传到 reapPendingEmptyChannel（与已经这样做的三个 restore 路径一致：bridge.ts:5808、:5813、:6012）。（未行内锚定：gate 所在行与第 1 轮 blocker 线程在 bridge.ts:2269 冲突，该线程覆盖的是同一 gate 现已修复的 spawn 侧。）
— qwen3.8-max via Qwen Code /review (v0.21.10)
| } catch (error) { | ||
| await retireChannelOnTimeout(ci, error, 'workspace control timeout'); | ||
| throw error; |
There was a problem hiding this comment.
[Suggestion] withWorkspaceControl's catch awaits the full bounded channel retirement inline before rethrowing, so a caller whose RPC already timed out pays for a complete teardown before receiving its error. The same awaited pattern exists at every retireChannelOnTimeout catch site (doSpawn newSession catch bridge.ts:3474, restore catch bridge.ts:5812). — Failure scenario: a workspace-control RPC against a wedged child times out at initTimeoutMs (default 10s); the caller then blocks roughly another initTimeoutMs before the rethrow (kill + exit wait, itself capped by withTimeout(kill(), initTimeoutMs)). Verified by probe: ~2x the timeout with the current code vs ~1x with fire-and-forget, with the retirement still completing in the background — safe because killChannelWithLog sets ci.isDying = true synchronously before any await, and downstream reap/settle paths gate on isDying.
| } catch (error) { | |
| await retireChannelOnTimeout(ci, error, 'workspace control timeout'); | |
| throw error; | |
| } catch (error) { | |
| void retireChannelOnTimeout(ci, error, 'workspace control timeout'); | |
| throw error; |
中文说明
[建议] withWorkspaceControl 的 catch 在重新抛出前内联等待完整的有界 channel 回收，因此 RPC 已经超时的调用方还要先付出完整 teardown 的成本才能收到错误。同样的“等待式”模式存在于每一个 retireChannelOnTimeout catch 位点（doSpawn 的 newSession catch bridge.ts:3474、restore catch bridge.ts:5812）。— 失败场景：针对卡死 child 的 workspace-control RPC 在 initTimeoutMs（默认 10s）超时后，调用方还要再阻塞约一个 initTimeoutMs 才能收到重抛的错误（kill + 等待退出，其自身又以 withTimeout(kill(), initTimeoutMs) 为上限）。探针验证：当前代码约为 2 倍超时时长，改为 fire-and-forget 后约为 1 倍，且回收仍会在后台完成——killChannelWithLog 在任何 await 之前同步设置 ci.isDying = true，后续 reap/settle 路径都以 isDying 为门控，因此 fire-and-forget 是安全的。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| dispose(): void { | ||
| this.disposed = true; | ||
| this.draining = true; | ||
| } |
There was a problem hiding this comment.
[Suggestion] dispose() has zero test coverage: no test asserts that ensure() rejects after dispose(), nor that cancelDrain() must not clear draining on a disposed coordinator (the !this.disposed guard at line 76). dispose() is wired into the runtime-removal cleanup path (disposeRuntime in run-qwen-serve.ts); all existing drain tests exercise only beginDrain()/cancelDrain(). — Failure scenario: a mutation probe deleted both disposed guards and the full affected suite stayed green (130 passed plus the drains-before-close test) — a rollback/retry could re-open ensure() on a runtime being torn down post-removal, restarting an ACP child for a removed workspace, with nothing going red. Suggested fix: add two cases to workspace-runtime-coordinator.test.ts: dispose() then ensure() rejects with workspace_draining; and dispose() -> cancelDrain() -> ensure() still rejects.
中文说明
[建议] dispose() 零测试覆盖：没有测试断言 dispose() 之后 ensure() 会拒绝，也没有测试断言 cancelDrain() 在已 dispose 的 coordinator 上不得清除 draining（第 76 行的 !this.disposed 守卫）。dispose() 已接入 runtime 移除清理路径（run-qwen-serve.ts 的 disposeRuntime）；现有 drain 测试只覆盖 beginDrain()/cancelDrain()。— 失败场景：变异探针删除了两个 disposed 守卫后，整个受影响测试套件仍然全绿（130 通过 + drains-before-close 测试）——回滚/重试可以在移除后正在拆除的 runtime 上重新打开 ensure()，为已移除的 workspace 重启 ACP child，而没有任何测试变红。建议：在 workspace-runtime-coordinator.test.ts 中补两个用例：dispose() 后 ensure() 以 workspace_draining 拒绝；以及 dispose() -> cancelDrain() -> ensure() 仍然拒绝。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| ### N parallel sessions share one `qwen --acp` child per workspace runtime | ||
|
|
||
| Multiple sessions on the same trusted workspace **share that runtime's `qwen --acp` child process** via the agent's native multi-session support (`packages/cli/src/acp-integration/acpAgent.ts:194: private sessions: Map<string, Session>`). The bridge calls `connection.newSession({cwd, mcpServers})` for each session — the agent stores them in its sessions map and demultiplexes per-call sessionId. Production can own up to one primary child (preheat attempted by default) plus one on-demand child per trusted secondary; untrusted secondaries own none. | ||
| Multiple sessions on the same trusted workspace **share that runtime's `qwen --acp` child process** via the agent's native multi-session support (`packages/cli/src/acp-integration/acpAgent.ts:194: private sessions: Map<string, Session>`). The bridge calls `connection.newSession({cwd, mcpServers})` for each session — the agent stores them in its sessions map and demultiplexes per-call sessionId. Production can own up to one child per trusted workspace: it attempts to preheat primary for compatibility, while trusted secondaries start on demand; untrusted workspaces own none. |
There was a problem hiding this comment.
[Suggestion] The file:line citation rewritten by this diff is stale: acpAgent.ts:194 is mid-import-block at this commit; private sessions: Map<string, Session> actually lives at packages/cli/src/acp-integration/acpAgent.ts:3620. — Failure scenario: an operator following the pointer to verify the native-multi-session claim lands on import lines and finds no sessions map — the doc's evidence link for the one-child-per-workspace multiplexing claim resolves to the wrong location. Suggested fix: update the citation to acpAgent.ts:3620, or drop the line number and cite only the file and member:
(`packages/cli/src/acp-integration/acpAgent.ts:3620: private sessions: Map<string, Session>`)
中文说明
[建议] 本 diff 改写的这处 file:line 引用已过期：在本 commit 上 acpAgent.ts:194 位于 import 块中间；private sessions: Map<string, Session> 实际位于 packages/cli/src/acp-integration/acpAgent.ts:3620。— 失败场景：想验证“原生多 session”说法的运维人员顺着指针打开，只会看到 import 语句而找不到 sessions map——每个 workspace 一个 child 的多路复用说法的证据链接指向了错误位置。建议：把引用更新为 acpAgent.ts:3620，或去掉行号、只引用文件与成员。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
|
||
| - Bridge construction is synchronous. A caller may preheat the channel before the first session; otherwise the first `spawnOrAttach` cold-starts the ACP child. A failed preheat leaves first use free to retry. | ||
| - `defaultEntry` lives for the lifetime of the bridge under `sessionScope: 'single'`; the channel reaps when `sessionIds.size === 0` (after `killSession`) AND `isDying` flips true. | ||
| - `defaultEntry` is the reusable logical Session under `sessionScope: 'single'`. Session close removes only its Session lease. After all Session, restore, workspace-control, discovery, authentication, and runtime-operation work drains and no explicit-ensure keepalive window is pending, an omitted or zero `channelIdleTimeoutMs` reaps the child immediately; a positive value — or an active keepalive window — delays reaping. |
There was a problem hiding this comment.
[Suggestion] Pattern R6-5 (1/4): the "zero/omitted channelIdleTimeoutMs reaps immediately" statements this PR writes omit the deliberate compat-preheat carve-out — a plain preheat() (no keepAliveMs) under timeout 0 leaves the child alive indefinitely: preheat is itself runtime-operation work by this bullet's own enumeration, and its finally settles with armIdleTimer = resolvedChannelIdleTimeoutMs() > 0 = false (bridge.ts:10588-10591), so no timer is armed; reapPendingEmptyChannel declines a non-condemned fresh channel. Verified intentional (design doc §16.2-9; tests preserves the preheated channel with an omitted/explicit zero timeout); same gap as still-standing round-5 finding R5-14. The same omission also exists at docs/developers/daemon/20-quickstart-operations.md:99, packages/acp-bridge/src/bridgeOptions.ts:470-472, packages/cli/src/commands/serve.ts:511, packages/cli/src/serve/types.ts:314, docs/design/session-idle-reaper/README.md:95 and :436, the 08-session-lifecycle.md preheat section, the 17-configuration.md channelIdleTimeoutMs rows, and design doc §6.3/§8.3. — Failure scenario: an operator seeing a long-lived sessionless child under default config misdiagnoses a leak; or a maintainer "fixes" the code to match the docs and breaks the deliberately preserved compat-preheat semantics.
| - `defaultEntry` is the reusable logical Session under `sessionScope: 'single'`. Session close removes only its Session lease. After all Session, restore, workspace-control, discovery, authentication, and runtime-operation work drains and no explicit-ensure keepalive window is pending, an omitted or zero `channelIdleTimeoutMs` reaps the child immediately; a positive value — or an active keepalive window — delays reaping. | |
| - `defaultEntry` is the reusable logical Session under `sessionScope: 'single'`. Session close removes only its Session lease. After all Session, restore, workspace-control, discovery, authentication, and runtime-operation work drains and no explicit-ensure keepalive window is pending, an omitted or zero `channelIdleTimeoutMs` reaps the child immediately — except a compatibility preheat without a keepalive window, which keeps the child alive (legacy preheat semantics); a positive value — or an active keepalive window — delays reaping. |
中文说明
[建议] 模式 R6-5（1/4）：本 PR 写入的“zero/omitted channelIdleTimeoutMs 立即回收”表述遗漏了有意的兼容 preheat 例外——不带 keepAliveMs 的 preheat() 在 timeout 0 下会让 child 无限期存活：按本条自己的枚举，preheat 本身就是 runtime-operation work，而它的 finally 以 armIdleTimer = resolvedChannelIdleTimeoutMs() > 0 = false 结算（bridge.ts:10588-10591），因此不会启动任何 timer；reapPendingEmptyChannel 又拒绝回收未被 condemn 的新 channel。已验证为有意设计（设计文档 §16.2-9；测试 preserves the preheated channel with an omitted/explicit zero timeout）；与仍然成立的第 5 轮发现 R5-14 是同一缺口。同样的遗漏还存在于 docs/developers/daemon/20-quickstart-operations.md:99、packages/acp-bridge/src/bridgeOptions.ts:470-472、packages/cli/src/commands/serve.ts:511、packages/cli/src/serve/types.ts:314、docs/design/session-idle-reaper/README.md:95 与 :436、08-session-lifecycle.md 的 preheat 小节、17-configuration.md 的 channelIdleTimeoutMs 行，以及设计文档 §6.3/§8.3。— 失败场景：运维人员在默认配置下看到一个长期存活的无 session child，误判为泄漏；或 maintainer 按文档“修复”代码，破坏刻意保留的兼容 preheat 语义。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| | `--writer-idle-timeout-ms <n>` | number | none | - | Per-SSE-connection idle timeout in ms. | | ||
| | `--channel-idle-timeout-ms <n>` | number | `0` | - | Keeps the ACP child alive after the last session closes. `0` means reclaim immediately. | | ||
| | `--initialize-timeout-ms <n>` | number | `10000` | - | ACP child request timeout, including the initialize handshake (ms). | | ||
| | `--channel-idle-timeout-ms <n>` | non-negative integer | `0` | - | ACP child auto-reap delay after all Session and management work drains. Unset or `0` reaps immediately. | |
There was a problem hiding this comment.
[Suggestion] Pattern R6-5 (2/4): same carve-out omission as docs/developers/daemon/03-acp-bridge.md:180 (see that comment for the mechanism and the full location list) — under unset/0, a bare startup preheat is never reaped, contradicting "Unset or 0 reaps immediately". — Failure scenario: same as the sibling comment: misdiagnosed "leak" under default config, or a code "fix" matching this row breaks the preserved legacy-preheat semantics.
| | `--channel-idle-timeout-ms <n>` | non-negative integer | `0` | - | ACP child auto-reap delay after all Session and management work drains. Unset or `0` reaps immediately. | | |
| | `--channel-idle-timeout-ms <n>` | non-negative integer | `0` | - | ACP child auto-reap delay after all Session and management work drains. Unset or `0` reaps immediately, except a compatibility preheat that never served a session. | |
中文说明
[建议] 模式 R6-5（2/4）：与 docs/developers/daemon/03-acp-bridge.md:180 相同的例外遗漏（机制与完整位置清单见该评论）——在 unset/0 下，从未服务过 session 的启动 preheat 永远不会被回收，与“Unset or 0 reaps immediately”矛盾。— 失败场景同上：默认配置下误判“泄漏”，或按此行“修复”代码而破坏保留的 legacy preheat 语义。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const inflight = client.ensureWorkspaceRuntime(); | ||
| await vi.advanceTimersByTimeAsync(61_999); | ||
| expect(requestSignal?.aborted).toBe(false); |
There was a problem hiding this comment.
[Suggestion] The new test 'gives runtime ensure the server deadline plus client headroom' pins only the lower bound of the 62s client timeout — it never asserts the timer fires at 62,000ms, unlike this file's established boundary pattern (three sibling timeout tests advance to boundary-1ms, assert not-aborted, then advance the final 1ms and assert the TimeoutError; the restore test comments "Pin the exact boundary, not a range"). — Failure scenario: probe-verified — inflating the client headroom from 2s to 12s keeps the committed test green, so a regression inflating the ensure timeout ships undetected; SDK callers of ensureWorkspaceRuntime()/ensureRuntime() would block well past the designed 62s budget against a connection that accepted but never answers. The probe's boundary-asserting variant fails under the inflated constant and passes under the correct one. Suggested fix: mirror the sibling pattern — after the not-aborted assertion, drop the early response resolution, advance the final 1ms, and assert the abort fired:
| const inflight = client.ensureWorkspaceRuntime(); | |
| await vi.advanceTimersByTimeAsync(61_999); | |
| expect(requestSignal?.aborted).toBe(false); | |
| const inflight = client.ensureWorkspaceRuntime(); | |
| await vi.advanceTimersByTimeAsync(61_999); | |
| expect(requestSignal?.aborted).toBe(false); | |
| await vi.advanceTimersByTimeAsync(1); | |
| expect(requestSignal?.aborted).toBe(true); |
中文说明
[建议] 新测试 'gives runtime ensure the server deadline plus client headroom' 只钉住了 62s 客户端超时的下界——从未断言 timer 在 62,000ms 触发，不同于本文件既有的边界模式（三个 sibling 超时测试都推进到 boundary−1ms、断言未 abort、再推进最后 1ms 断言 TimeoutError；restore 测试注释写明“Pin the exact boundary, not a range”）。— 失败场景（探针验证）：把客户端 headroom 从 2s 加大到 12s 后，已提交的测试仍然通过——因此放大 ensure 超时的回归会悄无声息地合入；ensureWorkspaceRuntime()/ensureRuntime() 的 SDK 调用方会在“已接受但永不响应”的连接上阻塞远超设计的 62s 预算。探针的边界断言变体在放大常量下失败、在正确常量下通过。建议：仿照 sibling 模式——在 not-aborted 断言后去掉提前 resolve 响应，推进最后 1ms 并断言 abort 已触发。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| `active`，lease 释放后为 `idle`。Capability 自己仍可保持 `starting` 或 `error`， | ||
| 但不会改变顶层五态。只要存在新的可复用 live Channel，顶层就按该 Channel 的 | ||
| `active/idle` 投影；旧 epoch 的 Channel 可同时处于退出过程。 |
There was a problem hiding this comment.
[Suggestion] The Foundation-marked five-state projection sentence contradicts the snapshot code this PR commits: while any old-epoch channel is still exiting, getWorkspaceRuntimeLifecycleSnapshot() computes stopping = aliveChannels.some(c => c.isDying) and folds it into activeWork before the live-channel branch — so the top level is forced to active regardless of the live channel's own work, never idle, for the whole old-channel exit window. The section's own state-machine diagram agrees with the code; this prose sentence is the outlier. — Failure scenario: a §16 verification test or SDK consumer written against this Foundation prose asserts/awaits idle and observes active; or a maintainer "fixing" the code to match the prose drops stopping from activeWork, making hasActiveWork() (the workspaceRuntime term of removal admission) false during teardown and letting non-force removal race a channel still physically exiting. Suggested fix: rewrite the clause — as long as another epoch's channel is still asynchronously exiting (or a startup is in flight), the top level projects active even if the live channel itself holds no lease; only once the exiting channel's process has fully exited does the top level fall back to the live channel's own active/idle projection.
中文说明
[建议] 这句标注 Foundation 的五态投影描述与本 PR 提交的快照代码矛盾：只要旧 epoch 的 channel 还在退出，getWorkspaceRuntimeLifecycleSnapshot() 会计算 stopping = aliveChannels.some(c => c.isDying) 并在 live-channel 分支之前并入 activeWork——因此整个旧 channel 退出窗口内顶层都被强制为 active、绝不可能是 idle，与 live channel 自身是否有 work 无关。本节自己的状态机图与代码一致；这句文字是唯一的例外。— 失败场景：按此 Foundation 描述编写的 §16 验证测试或 SDK 消费方断言/等待 idle 却观察到 active；或 maintainer 按描述“修复”代码、把 stopping 从 activeWork 中拿掉，使 hasActiveWork()（removal admission 的 workspaceRuntime 项）在 teardown 期间为 false，让非 force 移除与尚在物理退出的 channel 竞速。建议：改写该句——只要另一 epoch 的 channel 仍在异步退出（或启动在进行中），即使 live channel 自身没有 lease，顶层也投影 active；只有退出 channel 的进程完全退出后，顶层才回落到 live channel 自身的 active/idle 投影。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| Math.max(initTimeoutMs, 30_000), | ||
| 'changeSessionCwd', | ||
| ), |
There was a problem hiding this comment.
[Suggestion] The new physical deadline added to changeSessionCwd (default Math.max(initTimeoutMs, 30_000) = 30s) is coupled via withWorkspaceControl to retire-on-timeout, but it is <= the child's own legitimate worst-case handling time for sessionCd: the agent handler budgets up to SESSION_DRAIN_TIMEOUT_MS (30s, acpAgent.ts:381) in beginSessionCloseAfterCurrentGate and up to a further 30s in waitForSessionDrain before it even starts validation and relocateWorkingDirectory — zero margin remains for the relocation work itself. — Failure scenario: a changeSessionCwd whose child-side turn-settlement drain consumes most of its designed 30s budget crosses the bridge deadline first; withWorkspaceControl's catch calls retireChannelOnTimeout, killing the shared channel and every sibling session even though the child was operating inside its own designed budget. Pre-diff behavior tolerated this case (caller-facing timeout without killing the channel — verified against the merge base). The retirement is deliberate and tested; what is undersized is the deadline. Suggested fix: size the cd deadline above the child's worst case (e.g. 60s+), or exempt changeSessionCwd from retire-on-timeout.
| Math.max(initTimeoutMs, 30_000), | |
| 'changeSessionCwd', | |
| ), | |
| Math.max(initTimeoutMs, 60_000), | |
| 'changeSessionCwd', | |
| ), |
中文说明
[建议] 本 PR 为 changeSessionCwd 新增的物理期限（默认 Math.max(initTimeoutMs, 30_000) = 30s）经由 withWorkspaceControl 与超时回收耦合，但它 <= child 自身处理 sessionCd 的合法最坏耗时：agent handler 在 beginSessionCloseAfterCurrentGate 中最多预算 SESSION_DRAIN_TIMEOUT_MS（30s，acpAgent.ts:381），随后 waitForSessionDrain 又可再耗 30s，之后才开始校验和 relocateWorkingDirectory——留给迁移工作本身的余量为零。— 失败场景：child 侧 turn 结算排空耗掉大部分 30s 预算的 changeSessionCwd 会先越过 bridge 期限；withWorkspaceControl 的 catch 调用 retireChannelOnTimeout，杀掉共享 channel 及其上所有 sibling session——尽管 child 仍在自身设计预算内运行。改动前的行为容忍该场景（只向调用方抛超时、不杀 channel——已对 merge base 验证）。回收本身是有意且经过测试的；不足的是期限本身。建议：把 cd 期限设为大于 child 最坏耗时（如 60s+），或让 changeSessionCwd 不走超时回收。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| const info = liveChannelInfo(); | ||
| if (!info) throw new SessionNotFoundError(`workspace-command:${method}`); | ||
| return await withWorkspaceControl(info, () => invoke(info)); |
There was a problem hiding this comment.
[Suggestion] The non-workspaceMcpRestart branch of invokeWorkspaceCommand changed semantics in this diff (raw invoke() at the merge base -> now wrapped in withWorkspaceControl, so any generic workspace command that misses its deadline retires/kills the shared channel), but no test exercises this branch against a live channel — the only existing coverage asserts the no-channel SessionNotFoundError case (invokeWorkspaceCommand appears exactly 3 times in bridge.test.ts — two restart-branch, one no-channel; workspaceSkillsRefresh/workspaceReload/permissions/setRules appear zero times). — Failure scenario: at the merge base a timed-out workspaceSkillsRefresh / qwen/permissions/setRules / workspaceReload failed with BridgeTimeoutError and left the channel and its sessions intact; this diff makes the same timeout kill the channel and every sibling session on it. That destructive semantic is pinned by explicit tests for every sibling retire site this PR introduces, but by none for this one; a follow-up that drops the wrapping or alters the retire condition leaves the entire affected suite green. Suggested fix: add a bridge.test.ts case mirroring 'bounds a hung session extension refresh': spawn a session, stall a non-restart invokeWorkspaceCommand extMethod, advance fake timers past initTimeoutMs, assert BridgeTimeoutError + handle.killed === true + a fresh channel spawned for subsequent work.
中文说明
[建议] invokeWorkspaceCommand 的非 workspaceMcpRestart 分支在本 diff 中语义发生了变化（merge base 上是裸 invoke() -> 现在包进 withWorkspaceControl，任何错过期限的通用 workspace 命令都会回收/杀掉共享 channel），但没有任何测试在存活 channel 上覆盖该分支——现有覆盖只断言无 channel 时的 SessionNotFoundError（invokeWorkspaceCommand 在 bridge.test.ts 中恰好出现 3 次——两次 restart 分支、一次无 channel；workspaceSkillsRefresh/workspaceReload/permissions/setRules 出现 0 次）。— 失败场景：在 merge base 上，超时的 workspaceSkillsRefresh / qwen/permissions/setRules / workspaceReload 只会以 BridgeTimeoutError 失败、channel 与其 session 完好；本 diff 让同样的超时杀掉 channel 及其上所有 sibling session。本 PR 引入的每一个 sibling 回收位点都有显式测试钉住这一破坏性语义，唯独这里没有；后续改动若去掉包装或改变回收条件，整个受影响套件仍会全绿。建议：仿照 'bounds a hung session extension refresh' 增加 bridge.test.ts 用例：spawn 一个 session，让非 restart 的 invokeWorkspaceCommand extMethod 卡住，推进 fake timer 超过 initTimeoutMs，断言 BridgeTimeoutError + handle.killed === true + 后续工作获得新 channel。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| { | ||
| label: 'MCP authentication', | ||
| method: SERVE_CONTROL_EXT_METHODS.workspaceMcpManage, | ||
| timeoutMs: 600_000, |
There was a problem hiding this comment.
[Suggestion] The 600s MCP OAuth budget (MCP_OAUTH_TIMEOUT_MS, bridge.ts:1411 — used both as the authenticate RPC race timeout at bridge.ts:9867 and as the pending-auth expiry timer at bridge.ts:9900) is never pinned from below: every test exercising it advances the fake clock in a single jump of exactly 600_000ms, so any shorter lease would also fire and pass. Only the upward direction is pinned. — Failure scenario: a refactor rebinds either use to a shorter clock (e.g. initTimeoutMs, or MCP_RESTART_TIMEOUT_MS); a user whose OAuth browser approval takes longer than the shrunken lease hits the expiry path, which kills the owning channel — the sibling test 'drains missing MCP auth through its owning channel with an active session' proves that kill proceeds even with active sessions. The suite stays green. Suggested fix: add the boundary step before the full advance — advance to timeoutMs - 1, assert still pending (and for the expiry tests, no recheck/kill yet), then advance the final 1ms and assert the timeout/kill — mirroring the keep-alive tests in this same file (advance 599_999 -> alive -> 1 -> killed).
中文说明
[建议] 600s 的 MCP OAuth 预算（MCP_OAUTH_TIMEOUT_MS，bridge.ts:1411——同时用作 bridge.ts:9867 的 authenticate RPC 竞速超时与 bridge.ts:9900 的 pending-auth 到期 timer）从未被从下界钉住：所有覆盖它的测试都一次性推进 fake clock 恰好 600_000ms，因此任何更短的租约同样会触发并通过。只有上方向被钉住。— 失败场景：某次重构把两处使用之一改绑到更短的时钟（如 initTimeoutMs 或 MCP_RESTART_TIMEOUT_MS）；OAuth 浏览器审批耗时超过被压缩租约的用户会走到到期路径——该路径杀掉 owning channel；sibling 测试 'drains missing MCP auth through its owning channel with an active session' 证明即使有活跃 session 该 kill 也会执行。套件仍全绿。建议：在完整推进之前加入边界步骤——先推进到 timeoutMs - 1、断言仍 pending（对到期测试则断言尚未 recheck/kill），再推进最后 1ms 断言超时/kill——仿照本文件中 keep-alive 测试（推进 599_999 -> 存活 -> 1 -> 被杀）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge resolution: PR #8213 ← mainRoot causePR #8213 restructured the daemon bridge around workspace runtime ownership SemanticInitialize: PR changed the timeout to a startup budget; main added a await withTimeout(
Promise.race([
connection.initialize({ ... }),
channelUnavailableReject(channel, 'during initialize'),
]),
remainingStartupMs,
'initialize',
);
if (!ci.isDying) {
await withWorkspaceControl(ci, () =>
withTimeout(
Promise.race([
ci.connection.extMethod(SERVE_CONTROL_EXT_METHODS.sessionClose, ...),
channelUnavailableReject(ci.channel, 'during branchSession cleanup'),
]),
initTimeoutMs,
'branchSession cleanup',
),
);
}For the What is load-bearing
What I could not verifyNo build, typecheck, or tests were run (per task contract). Auto-merged 中文说明根因：PR #8213 重构 daemon bridge 的工作区运行时所有权（ 三处语义冲突均合并双方意图：initialize 保留 PR 超时预算 + main 关闭竞速； 关键约束：两个缓存 reject 助手必须继续委托 未运行构建/测试； |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Unresolved, please confirm:
- [Critical] Maintainer overview concern 1 (comment 5151473223; related thread 3699909386) — status reads reap the compat-preheat warmth, status-poll re-arms the idle timer (~5s metrics sampler disables channelIdleTimeoutMs while any watcher is connected), and a status-poll BridgeTimeoutError retires the shared channel: mechanisms present at the reviewed commit and test-locked; whether this is a defect or an accepted tradeoff still awaits the maintainer direction the review has been waiting on since round 2 — cannot be settled from code alone.
- [Critical] Maintainer overview concern 2 / review #2 High 2 (comments 5151473223, 5157924428) — MCP discovery/detail timeout kills the channel with no session guard (bridge.ts:2677-2678, :4625): mechanism present at the reviewed commit and test-locked; awaiting maintainer direction. (The superseded-timer race within the OAuth expiry — R7-2 — was fixed-separately territory and is commented inline; the drain policy itself remains the design call.)
- [Critical] Review #2 High 3 (comment 5157924428; thread 3691942521) — vanished-server auth drain kills the owning channel instead of draining cleanly: mechanism present at the reviewed commit; deferred to a follow-up by the author; the deferral is acknowledged but the code behavior stands, and this review cannot rule it fixed-or-standing from code alone.
Not reviewed: reverse audit — stopped at the 3-round cap for huge diffs; rounds 1-3 all surfaced findings, so two consecutive dry rounds were never reached (all 21 chunks audited every round).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the job that exercises the integration suite this PR modifies, incl. the capability baseline and the new runtime routes); Test (macos/windows) also skipped — unit scope covered locally on Linux.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; PR #8213 establishes WorkspaceRuntime as the ownership bo...: none — all planned checks completed (config enumeration, early-return walk, reservation pairing, base comparisons, full package test run).; chunk 13: exact root-cause line for the 6 pre-existing failures within the restore machinery (outside chunk 13; surgical revert only ruled my chunk in/out); chunk 13: trailing lines of the reloadWorkspaceMcp hunk beyond diff line 4370 belong to chunk 14 (visible portion matched the verified initializeWorkspaceMcp pattern); chunk 15: none — all checks I intended completed within budget., and 9 more.
Test Plan (not a blocker): 25 tests passed — this review observed 1311, 19317, 1514, 494, 3393, 555 passed; 306 tests passed — this review observed 1311, 19317, 1514, 494, 3393, 555 passed.
[Critical] R5-1 — packages/acp-bridge/src/bridge.ts:3437 — still stands at the reviewed commit (re-derived this round by the chunk-10 reverse auditor; existing inline thread 3724278944, not re-posted to avoid duplication). The construction-failure path's await terminateChannel(channel, 'channel construction failure') runs after channel.killSync() (SIGKILL); terminateChannel then rejects on the very signal-exit killSync caused (ProcessRegistry.terminateOnce treats a signal exit as unclean), so callers receive AggregateError('ACP channel construction and teardown failed') instead of the original construction error, and downstream instanceof-based classification sees the wrapper, not the original error. The retained comment directly above ("Raw exit is successful teardown after the forced signal") contradicts the behavior; unit tests mask it (the fake channel's kill() resolves even after killSync()). Fix: race the forced exit as success (await Promise.race([channel.exited.then(() => undefined), terminateChannel(channel, 'channel construction failure')])), make terminateOnce treat a signal-exit following its own killSync as success, or update the retained comment to state the new semantics.
[Critical] R6-1 — packages/acp-bridge/src/bridge.ts:2661-2663 (decision sites ~6277 abandon kill_empty, ~6139 quarantine cleanup kill, grace reap via reapPendingEmptyChannel armed from armRestoreSettlementGrace) — still stands at the reviewed commit (re-derived this round by invariant agent C; the inline anchor overlapped the round-1 gate thread 3698048827, whose spawn-side defect is fixed but whose anchor still sits on the same line). The PR-added outerRestoreCount term in hasNoChannelWork counts the subject restore's own inFlightRestores entry at decision sites that never pass ignoreRestoreId. An abandoned restore's entry stays in inFlightRestores until physical settlement — which happens only when the transport closes — which happens only when the channel is killed — which these gates now permanently block. Failure scenario: session/load on an empty channel whose child wedges → at the deadline kill_empty is dead code, the grace reap is likewise blocked, the channel is never killed, the session id stays fenced, and every new spawn/restore/branch throws BridgeChannelQuarantinedError('restore_settlement_overdue') until daemon restart. Measured by this review's build & test run + base-tree test-delta: the acp-bridge suite is red on this PR (8 tests fail in bridge.test.ts, all green at the merge base; netNew), including the five restore-recovery tests and the two PR-added preheat retirement variants. The spawn side is handled correctly (ignoreCurrentSessionSpawn), so the defect is restore-specific. Fix: pass { ignoreRestoreId: req.sessionId } at the abandon-decision (~6277) and quarantine-cleanup (~6139) sites, and forward an ignore option from armRestoreSettlementGrace through reapPendingEmptyChannel (mirroring the three restore paths that already do this: bridge.ts:6341, :6349, :6556); reconcile the two PR-added restore-retire tests with main's merged restore-timeout budget semantics.
中文说明
未决，请确认：共 3 条（原文未翻译，列表见上方英文部分）。
未审查：reverse audit — stopped at the 3-round cap for huge diffs; rounds 1-3 all surfaced findings, so two consecutive dry rounds were never reached (all 21 chunks audited every round)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the job that exercises the integration suite this PR modifies, incl. the capability baseline and the new runtime routes); Test (macos/windows) also skipped — unit scope covered locally on Linux。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；PR #8213 establishes WorkspaceRuntime as the ownership bo...：none — all planned checks completed (config enumeration, early-return walk, reservation pairing, base comparisons, full package test run).；chunk 13：exact root-cause line for the 6 pre-existing failures within the restore machinery (outside chunk 13; surgical revert only ruled my chunk in/out)；chunk 13：trailing lines of the reloadWorkspaceMcp hunk beyond diff line 4370 belong to chunk 14 (visible portion matched the verified initializeWorkspaceMcp pattern)；chunk 15：none — all checks I intended completed within budget.，另有 9 条。
Test Plan（非阻断）：25 tests passed — this review observed 1311, 19317, 1514, 494, 3393, 555 passed; 306 tests passed — this review observed 1311, 19317, 1514, 494, 3393, 555 passed。
[Critical] R5-1 — packages/acp-bridge/src/bridge.ts:3437 — still stands at the reviewed commit (re-derived this round by the chunk-10 reverse auditor; existing inline thread 3724278944, not re-posted to avoid duplication). The construction-failure path's await terminateChannel(channel, 'channel construction failure') runs after channel.killSync() (SIGKILL); terminateChannel then rejects on the very signal-exit killSync caused (ProcessRegistry.terminateOnce treats a signal exit as unclean), so callers receive AggregateError('ACP channel construction and teardown failed') instead of the original construction error, and downstream instanceof-based classification sees the wrapper, not the original error. The retained comment directly above ("Raw exit is successful teardown after the forced signal") contradicts the behavior; unit tests mask it (the fake channel's kill() resolves even after killSync()). Fix: race the forced exit as success (await Promise.race([channel.exited.then(() => undefined), terminateChannel(channel, 'channel construction failure')])), make terminateOnce treat a signal-exit following its own killSync as success, or update the retained comment to state the new semantics.
[Critical] R6-1 — packages/acp-bridge/src/bridge.ts:2661-2663 (decision sites ~6277 abandon kill_empty, ~6139 quarantine cleanup kill, grace reap via reapPendingEmptyChannel armed from armRestoreSettlementGrace) — still stands at the reviewed commit (re-derived this round by invariant agent C; the inline anchor overlapped the round-1 gate thread 3698048827, whose spawn-side defect is fixed but whose anchor still sits on the same line). The PR-added outerRestoreCount term in hasNoChannelWork counts the subject restore's own inFlightRestores entry at decision sites that never pass ignoreRestoreId. An abandoned restore's entry stays in inFlightRestores until physical settlement — which happens only when the transport closes — which happens only when the channel is killed — which these gates now permanently block. Failure scenario: session/load on an empty channel whose child wedges → at the deadline kill_empty is dead code, the grace reap is likewise blocked, the channel is never killed, the session id stays fenced, and every new spawn/restore/branch throws BridgeChannelQuarantinedError('restore_settlement_overdue') until daemon restart. Measured by this review's build & test run + base-tree test-delta: the acp-bridge suite is red on this PR (8 tests fail in bridge.test.ts, all green at the merge base; netNew), including the five restore-recovery tests and the two PR-added preheat retirement variants. The spawn side is handled correctly (ignoreCurrentSessionSpawn), so the defect is restore-specific. Fix: pass { ignoreRestoreId: req.sessionId } at the abandon-decision (~6277) and quarantine-cleanup (~6139) sites, and forward an ignore option from armRestoreSettlementGrace through reapPendingEmptyChannel (mirroring the three restore paths that already do this: bridge.ts:6341, :6349, :6556); reconcile the two PR-added restore-retire tests with main's merged restore-timeout budget semantics.
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if ( | ||
| !ci.isDying && | ||
| hasNoChannelWork(ci, { ignoreCurrentSessionSpawn: true }) | ||
| ) { |
There was a problem hiding this comment.
[Critical] The added !ci.isDying guard skips the empty-channel kill whenever a transport failure already marked the channel dying — but markTransportFailed (bridge.ts:3481-3489) is the one isDying set-site with no paired kill, so a transport-failed empty channel is marked dying and killed by nobody. Pre-PR this catch killed unconditionally. Probe-verified this round: the pre-existing test does not register a session whose transport fails with newSession (bridge.test.ts:8663) fails on this commit (expect(handle.killed).toBe(true) receives false) and passes when the guard is removed from the kill branch — one of the 8 suite failures this review measured netNew against a green merge base. — Failure scenario: the child's transport fails during connection.newSession() on an empty channel → markTransportFailed's .then sets isDying before this catch runs → both terminateChannel and emptyReapPending are skipped → the qwen --acp child lingers with no teardown path until daemon exit.
| if ( | |
| !ci.isDying && | |
| hasNoChannelWork(ci, { ignoreCurrentSessionSpawn: true }) | |
| ) { | |
| if ( | |
| hasNoChannelWork(ci, { ignoreCurrentSessionSpawn: true }) | |
| ) { |
(Keep !ci.isDying on the emptyReapPending else-branch; terminateChannel is memoized/idempotent, so the already-dying case is harmless.)
中文说明
新增的 !ci.isDying 守卫会在 transport 失败已将 channel 标记为 dying 时跳过空 channel 的 kill——但 markTransportFailed（bridge.ts:3481-3489）是唯一一个设置了 isDying 却没有配对 kill 的位置，因此 transport 失败的空 channel 被标记为 dying 后无人回收。改动前该 catch 无条件 kill。本轮经探针验证：既有测试 does not register a session whose transport fails with newSession（bridge.test.ts:8663）在本 commit 上失败（expect(handle.killed).toBe(true) 得到 false），移除 kill 分支上的该守卫后通过——这是本审查相对绿色 merge base 实测出的 8 个 netNew 失败之一。— 失败场景：子进程 transport 在空 channel 上的 connection.newSession() 期间失败 → markTransportFailed 的 .then 先于本 catch 设置 isDying → terminateChannel 与 emptyReapPending 均被跳过 → qwen --acp 子进程在 daemon 退出前一直残留、无任何回收路径。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if ( | ||
| info.isDying || | ||
| liveChannelInfo() !== info || | ||
| !info.workspaceMcpAuthenticationServerNames.has(serverName) | ||
| ) { | ||
| return; | ||
| } | ||
| await killChannelWithLog( | ||
| info, | ||
| `workspace MCP authentication timeout for ${serverName}`, | ||
| ); |
There was a problem hiding this comment.
[Critical] Superseded OAuth expiry kills the renewed authentication's channel. expireWorkspaceMcpAuthentication decides staleness after its status poll using only Set membership, which is not tied to a specific authentication attempt: the timer arm (bridge.ts:10508-10512) passes no identity into the expiry, and clearTimeout(previousTimer) in manageMcpServer can only cancel pending timers, never an already-executing expiry callback. Pre-PR the timer body synchronously deleted the set/map entries, so a post-expiry re-auth cleanly re-owned the state and there was no kill at all. Probe-verified this round: with a re-authenticate landing inside the expiry's poll window the renewed attempt resolves successfully and the channel is still killed (handle.killed = true, snapshot cold); requiring workspaceMcpAuthenticationTimers.get(serverName) === timer after the poll stops the kill while the three existing lease-expiry tests stay green. — Failure scenario: T1 fires at the 10-minute OAuth deadline and starts its status poll (window up to initTimeoutMs) → the user retries authenticate in that window (exactly when a timed-out user retries) → the name is re-added and fresh timer T2 installed → the expiry's poll sees the NEW attempt's pending, so nothing clears → the post-poll check passes and the shared channel is killed, destroying the renewed auth lease and every unrelated live session on the channel. Distinct from the open design discussion about whether expiry should kill at all: under any policy, a superseded attempt must not drain its successor.
Suggested fix: pass the timer handle into the callback and abort the expiry when info.workspaceMcpAuthenticationTimers.get(serverName) !== timer after the poll (or track an attempt generation per server).
中文说明
被取代的 OAuth 过期回调会杀掉已续期认证的 channel。expireWorkspaceMcpAuthentication 在状态轮询之后仅凭 Set 成员关系判断是否过期，而该成员关系并不绑定到某一次具体的认证尝试：定时器启动处（bridge.ts:10508-10512）没有把任何身份信息传给过期回调，manageMcpServer 中的 clearTimeout(previousTimer) 只能取消未触发的定时器，无法阻止已经在执行的过期回调。改动前定时器回调体会同步删除 set/map 条目，因此过期后重新认证可以干净地重新持有状态，且根本没有 kill。本轮经探针验证：在过期回调的轮询窗口内重新认证时，续期的请求成功返回，但 channel 仍被杀掉（handle.killed = true，快照变为 cold）；在轮询之后要求 workspaceMcpAuthenticationTimers.get(serverName) === timer 即可阻止该 kill，且三个既有的 lease 过期测试保持绿色。— 失败场景：T1 在 10 分钟 OAuth 期限到达时触发并开始状态轮询（窗口最长 initTimeoutMs）→ 用户在该窗口内重试认证（恰好是超时用户重试的时机）→ 名称被重新加入、新定时器 T2 被安装 → 过期回调的轮询看到新尝试仍为 pending，于是什么都不清理 → 轮询后的检查通过，共享 channel 被杀，续期的认证 lease 与 channel 上所有无关的活跃 session 一起被摧毁。这与「过期是否应该 kill」的既有设计讨论相互独立：无论采用哪种策略，被取代的尝试都不应排空它的后继者。
建议修复：把定时器句柄传入回调，在轮询之后若 info.workspaceMcpAuthenticationTimers.get(serverName) !== timer 则中止过期流程（或为每个 server 记录认证尝试的代际）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| it.each(['new session', 'load session', 'resume session'] as const)( | ||
| 'retires a shared channel when a timed-out %s RPC remains physically pending', | ||
| async (operation) => { |
There was a problem hiding this comment.
[Critical] The acp-bridge suite is red on this PR: 8 tests fail, all green at the merge base (measured netNew via base-tree + test-delta, reproduced across two runs; build itself is green across all 18 workspaces). Failing tests: times out an empty restore channel without stopping the bridge (:6358), does not reap a timed-out restore channel during workspace control (:6524), closes a channel to fresh sessions when an abandoned restore outlives its grace (:7122), reaps a channel that drains while an abandoned restore is still unsettled (:7200), quarantines only fresh work when late restore cleanup fails (:7603), does not register a session whose transport fails with newSession (:8663), and the two PR-added retires a shared channel when a timed-out new/load/resume session RPC remains physically pending variants (here at :22661; the load/resume variants hang to the 5s timeout). Root causes identified this round: R6-1 (outerRestoreCount counting the subject restore's own inFlightRestores entry at the abandon/quarantine/grace decision sites) and the !ci.isDying guard commented on at bridge.ts:3939. The CI Test (ubuntu-latest, Node 22.x) failure is consistent with this. — Failure scenario: e.g. in the :6524 test a timed-out abandoned restore's channel is never reaped (handle.killed stays false) — it lingers warm for the whole idle window instead of being torn down; the design doc §16.1 claim '已通过相关单元测试' is false until this is green.
中文说明
acp-bridge 套件在本 PR 上是红的：8 个测试失败，在 merge base 上全部为绿（经 base-tree + test-delta 实测为 netNew，两次独立运行均复现；构建本身在全部 18 个 workspace 上是绿的）。失败的测试：times out an empty restore channel without stopping the bridge（:6358）、does not reap a timed-out restore channel during workspace control（:6524）、closes a channel to fresh sessions when an abandoned restore outlives its grace（:7122）、reaps a channel that drains while an abandoned restore is still unsettled（:7200）、quarantines only fresh work when late restore cleanup fails（:7603）、does not register a session whose transport fails with newSession（:8663），以及本 PR 新增的 retires a shared channel when a timed-out new/load/resume session RPC remains physically pending 的两个变体（此处 :22661；load/resume 变体会挂到 5s 超时）。本轮定位的根因：R6-1（outerRestoreCount 在 abandon/quarantine/grace 判定位点把当事 restore 自己的 inFlightRestores 条目也计入）以及 bridge.ts:3939 处评论的 !ci.isDying 守卫。CI 上 Test (ubuntu-latest, Node 22.x) 的失败与此一致。— 失败场景：以 :6524 为例，超时被放弃 restore 的 channel 永远不会被回收（handle.killed 保持 false）——本应被回收的 channel 在整个空闲窗口内一直残留；在套件变绿之前，设计文档 §16.1 中「已通过相关单元测试」的说法不成立。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| 9. GET 状态和 Catalog 请求不隐式启动 ACP。新调用路径只能通过显式 `ensure`、其他 | ||
| runtime command 或 Session 创建来启动；Foundation 暂时保留 production startup |
There was a problem hiding this comment.
[Suggestion] Invariant 9 lists only "Session 创建" as a session-side startup path, but §8.1 (line ~301) says runtimes start from cold by "显式 runtime command（包括 ensure）或 Session create/load/resume", and §8.2 grants load/resume physical work leases. The implementation matches §8.1: the restore path calls ensureChannel() (bridge.ts:6170), so load/resume of a session on a cold runtime starts it. Section 4 is explicitly normative ("是实现选择的边界，不是建议"). — Failure scenario: a reviewer enforcing invariant 9 literally flags the implemented load/resume-driven startup as a violation, or an implementer wiring session resume avoids starting a cold runtime and breaks resume on cold workspaces. — Concrete cost: the two normative statements in the same Foundation doc contradict each other; fix is to change "Session 创建" here to "Session create/load/resume" (or "Session 创建/加载/恢复").
中文说明
不变量 9 只把「Session 创建」列为 session 侧的启动路径，但 §8.1（约第 301 行）写的是 runtime 从 cold 经「显式 runtime command（包括 ensure）或 Session create/load/resume」启动，且 §8.2 为 load/resume 授予物理工作 lease。实现与 §8.1 一致：restore 路径会调用 ensureChannel()（bridge.ts:6170），因此在 cold runtime 上 load/resume session 会启动它。第 4 节明确是规范性的（「是实现选择的边界，不是建议」）。— 失败场景：严格按不变量 9 执行的审查者会把已实现的 load/resume 触发启动判为违规；或者实现 session resume 的人避免启动 cold runtime，导致冷工作区上的 resume 被破坏。— 具体代价：同一份 Foundation 文档中两条规范性表述相互矛盾；修复方式是把此处的「Session 创建」改为「Session create/load/resume」（或「Session 创建/加载/恢复」）。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| 2. 新 epoch 开始时，旧 epoch 的 `ready` 立即变为 `stale`。 | ||
| 3. 旧 epoch 的 `completed` 不得覆盖新 epoch 的 `not_started` 或空结果。 |
There was a problem hiding this comment.
[Suggestion] §9 rule 3 references a backticked state literal `completed` that exists in no state model defined by this document: CapabilityState (defined 13 lines above) is 'not_started' | 'starting' | 'ready' | 'stale' | 'error', and the §8.1 lifecycle machine is cold/starting/active/idle/stopping. Every other backticked token in this rule list is a real state; grep confirms exactly one occurrence of `completed` in the document. Rule 1's phrasing ("当前 epoch 完成的 ACP 响应") and §14.2's un-backticked "旧 epoch 的 completed cache" show the intent is "a completed response from the old epoch". — Failure scenario: an implementer writing the epoch-projection guard searches the type system for completed, finds nothing, and must guess: mapping it to ready narrows the guard (a stale-epoch error response could then overwrite the new epoch's not_started/empty projection); inventing a completed state diverges from the published union.
| 2. 新 epoch 开始时，旧 epoch 的 `ready` 立即变为 `stale`。 | |
| 3. 旧 epoch 的 `completed` 不得覆盖新 epoch 的 `not_started` 或空结果。 | |
| 2. 新 epoch 开始时，旧 epoch 的 `ready` 立即变为 `stale`。 | |
| 3. 旧 epoch 已完成的响应不得覆盖新 epoch 的 `not_started` 或空结果。 |
中文说明
§9 规则 3 引用了带反引号的状态字面量 `completed`，但本文档定义的任何状态模型中都不存在该状态：CapabilityState（上方 13 行处定义）为 'not_started' | 'starting' | 'ready' | 'stale' | 'error'，§8.1 的生命周期状态机为 cold/starting/active/idle/stopping。该规则列表中其他所有带反引号的词都是真实状态；grep 确认整份文档中 `completed` 只出现这一次。规则 1 的措辞（「当前 epoch 完成的 ACP 响应」）和 §14.2 中不带反引号的「旧 epoch 的 completed cache」表明其本意是「旧 epoch 已完成的响应」。— 失败场景：实现 epoch 投影守卫的开发者在类型系统中查找 completed，一无所获，只能猜测：把它映射为 ready 会收窄守卫（旧 epoch 的 error 响应就可能覆盖新 epoch 的 not_started/空投影）；凭空发明一个 completed 状态则偏离已发布的并集。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| if (Object.keys(safeBody(req)).length > 0) { | ||
| res.status(400).json({ | ||
| error: 'Workspace runtime ensure does not accept parameters', |
There was a problem hiding this comment.
[Suggestion] The "takes no parameters" guard is half-enforced — two probe-verified bypasses. (1) Query strings: the guard only inspects the request body, so POST /workspace/runtime/ensure?timeoutMs=5000 silently discards the parameter and runs the fixed 60s budget instead of the advertised 400 (probe: 200 + runtime started; with the fix below, 400). The legacy route this replaces (POST /workspace/acp/preheat, workspace-status.ts:153-186) accepts and validates ?timeoutMs=. (2) Pollution-key-only bodies: production safeBody (request-helpers.ts:118-130) strips __proto__/constructor/prototype before the guard counts keys, so a body consisting only of those keys bypasses the guard (probe: {"__proto__":{...}} and {"constructor":{},"prototype":{}} both returned 200 with preheat called, where a control body correctly got 400). The unit test cannot catch either: it injects a fake safeBody with no stripping and never sends query params. No pollution occurs — the defect is the contract hole; both routes (primary and qualified) are affected via the shared registerFor. — Concrete cost: also reject query params (below) and raw-body presence, e.g. || (req.body && typeof req.body === 'object' && !Array.isArray(req.body) && Object.keys(req.body).length > 0); mirror the production stripping safeBody in the test.
| if (Object.keys(safeBody(req)).length > 0) { | |
| res.status(400).json({ | |
| error: 'Workspace runtime ensure does not accept parameters', | |
| if ( | |
| Object.keys(safeBody(req)).length > 0 || | |
| Object.keys(req.query).length > 0 | |
| ) { | |
| res.status(400).json({ | |
| error: 'Workspace runtime ensure does not accept parameters', |
中文说明
「不接受任何参数」的守卫只做了一半——两个经探针验证的绕过。(1) 查询字符串：守卫只检查请求体，因此 POST /workspace/runtime/ensure?timeoutMs=5000 会静默丢弃该参数并按固定的 60s 预算运行，而不是返回声明的 400（探针：200 + runtime 被启动；应用下方修复后为 400）。本路由所取代的旧路由（POST /workspace/acp/preheat，workspace-status.ts:153-186）是接受并校验 ?timeoutMs= 的。(2) 仅含原型污染键的请求体：生产环境的 safeBody（request-helpers.ts:118-130）会在守卫统计键数之前剥离 __proto__/constructor/prototype，因此只包含这些键的请求体可以绕过守卫（探针：{"__proto__":{...}} 与 {"constructor":{},"prototype":{}} 均返回 200 且 preheat 被调用，而对照请求体正确地得到了 400）。单元测试两种绕过都抓不到：它注入的假 safeBody 不做剥离，也从不发送查询参数。并未发生污染——缺陷是契约上的漏洞；primary 与 qualified 两条路由经共享的 registerFor 均受影响。— 具体代价：同时拒绝查询参数（见下方建议）与原始请求体的存在，例如 || (req.body && typeof req.body === 'object' && !Array.isArray(req.body) && Object.keys(req.body).length > 0)；并在测试中使用与生产一致、会做剥离的 safeBody。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| | └--------┘ 新 lease | ||
| └-> cold + lastError 启动失败 |
There was a problem hiding this comment.
[Suggestion] §8.1's Foundation lifecycle diagram promises cold + lastError on startup failure, but no lastError exists anywhere in the Foundation surface: ServeWorkspaceRuntimeStatus is exactly { v, workspaceCwd, state, runtimeLive, runtimeEpoch } (status.ts:13-19), grep finds zero lastError matches in packages/acp-bridge/src, and §1.1's own status row lists error 投影 as Target while marking the five-state lifecycle Foundation — the doc contradicts itself. §8 is not marked Target. — Failure scenario: an SDK/frontend implementer following §8.1 polls GET /runtime/status after a failed ensure expecting the promised lastError diagnostic; the payload cannot distinguish "never started" from "startup crashed" because the failure reason only ever arrives in the triggering ensure call's 503 response (startup-preheat failures are only logged). — Concrete cost: add lastError to the snapshot (Target work), or rewrite the annotation to state that the failure reason is carried only by the triggering call's response, and mark that annotation Target.
中文说明
§8.1 的 Foundation 生命周期图承诺启动失败时进入 cold + lastError，但 Foundation 面上根本不存在 lastError：ServeWorkspaceRuntimeStatus 恰好是 { v, workspaceCwd, state, runtimeLive, runtimeEpoch }（status.ts:13-19），grep 在 packages/acp-bridge/src 中找不到任何 lastError，而 §1.1 自己的 status 行把 error 投影 列为 Target、同时把五态生命周期标为 Foundation——文档自相矛盾。§8 并未标记为 Target。— 失败场景：按 §8.1 实现的 SDK/前端开发者会在 ensure 失败后轮询 GET /runtime/status，期望得到承诺的 lastError 诊断信息；而响应体无法区分「从未启动」与「启动崩溃」，因为失败原因只会出现在触发启动的 ensure 调用的 503 响应里（startup preheat 失败只会被记日志）。— 具体代价：要么把 lastError 加入快照（属 Target 工作），要么把该注解改写为「失败原因仅由触发启动的调用响应携带」，并把该注解标记为 Target。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| getChannelClosedReject(ci), | ||
| ]), | ||
| Math.max(initTimeoutMs, 30_000), |
There was a problem hiding this comment.
[Suggestion] This diff converts branchSession/createSideTaskSession (bridge.ts:8356) and changeSessionCwd (here) from getTransportClosedReject(entry) — whose message names the session (mid-request (session <id>)) — to getChannelClosedReject(ci), which hard-codes the context string 'mid-request (workspace status)' and memoizes it per channel in ci.statusClosedReject. — Failure scenario: the transport closes while a session/branch or session/cd ext method is in flight; the rejected request and daemon logs carry agent channel closed mid-request (workspace status) though no workspace-status request was involved, pointing operators at the status-poll path (which has its own known, distinct channel-retirement behavior) instead of the operation that actually failed; because the promise is memoized per channel, whichever operation observes the close first freezes this wording for all concurrent observers. Error mapping is unchanged (instanceof BridgeChannelClosedError), so this is diagnostics, not function. — Concrete cost: give getChannelClosedReject a per-call-site context parameter (e.g. getChannelClosedReject(ci, 'during session/branch')), or drop the status-specific wording from the shared constant.
中文说明
本 diff 把 branchSession/createSideTaskSession（bridge.ts:8356）和 changeSessionCwd（此处）从 getTransportClosedReject(entry)——其报错信息会带上 session（mid-request (session <id>)）——改为 getChannelClosedReject(ci)，后者把上下文字符串硬编码为 'mid-request (workspace status)' 并通过 ci.statusClosedReject 按 channel 记忆化。— 失败场景：session/branch 或 session/cd ext 方法在途时 transport 关闭；被拒绝的请求和 daemon 日志会显示 agent channel closed mid-request (workspace status)，尽管并不涉及任何 workspace status 请求，从而把运维人员引向 status 轮询路径（该路径有其独立的、已知的 channel 回收行为），而不是真正失败的操作；由于该 promise 按 channel 记忆化，最先观察到关闭的操作会为所有并发观察者冻结这一措辞。错误映射未变（instanceof BridgeChannelClosedError），因此这是诊断信息问题，不是功能问题。— 具体代价：给 getChannelClosedReject 增加按调用点的上下文参数（如 getChannelClosedReject(ci, 'during session/branch')），或从共享常量中去掉 status 专属措辞。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| expect(channelFactory).toHaveBeenCalledTimes(1); | ||
| expect(secondHandle.agent.extMethodCalls).not.toEqual( | ||
| expect.arrayContaining([expect.objectContaining({ method })]), |
There was a problem hiding this comment.
[Suggestion] 'does not run a queued $label against a replacement runtime epoch' never constructs a replacement runtime epoch: it asserts channelFactory was called exactly once, so the primed secondHandle is never published to the bridge and the trailing assertion is vacuously true. The guard the title promises — assertLivePromptEntry's identity check byId.get(sessionId) !== entry (bridge.ts:4579) in the queued continuations of branchSession/changeSessionCwd — is not pinned: probe-relaxing that check to a fresh liveness lookup leaves both variants passing (2 passed, same as baseline). The identity check is load-bearing where the id is replaced while the old channel stays live (e.g. close queued → same id re-spawned). — Failure scenario: a refactor that relaxes the identity check to a fresh byId lookup at drain time ships green past this test, letting a queued branch/cd execute against a replacement channel/epoch. — Concrete cost: after the crash and queued rejection, resume/recreate the session under the same sessionId (the factory's second slot returns secondHandle.channel), then assert the queued promise still rejects and secondHandle.agent.extMethodCalls never receives the method.
中文说明
'does not run a queued $label against a replacement runtime epoch' 从未真正构造出 replacement runtime epoch：它断言 channelFactory 恰好被调用一次，因此预先准备好的 secondHandle 永远不会发布到 bridge，结尾的断言恒真。测试标题所承诺的守卫——branchSession/changeSessionCwd 排队续体中 assertLivePromptEntry 的身份检查 byId.get(sessionId) !== entry（bridge.ts:4579）——并未被钉住：探针把该检查放宽为「重新按存活查找」后，两个变体依旧通过（2 passed，与基线相同）。该身份检查在「旧 channel 仍存活而 id 被替换」的场景下是承重的（例如 close 已排队 → 同一 id 被重新 spawn）。— 失败场景：把身份检查放宽为排空时重新 byId 查找的重构，可以在该测试保持绿色的情况下合入，从而让排队的 branch/cd 在 replacement channel/epoch 上执行。— 具体代价：在 crash 与排队拒绝之后，用同一 sessionId resume/重建 session（factory 的第二个槽位返回 secondHandle.channel），然后断言排队的 promise 仍然 reject，且 secondHandle.agent.extMethodCalls 从未收到该方法。
— qwen3.8-max via Qwen Code /review (v0.21.10)
| await closing; | ||
| } finally { | ||
| await (closing ?? handle.close()); |
There was a problem hiding this comment.
[Suggestion] 'drains every lifecycle runtime before close yields' pins only the draining state (coordinators exist, ensure() rejects with workspace_draining, preheat untouched) and asserts nothing after await closing; — no primaryBridge.shutdown/secondaryBridge.shutdown or registry-removal assertion, despite the mocks carrying shutdown: vi.fn() recorders. Probe-verified: mutating the shutdown path to drop lifecycle-capable runtimes from disposal entirely (both the listManaged() → disposeRuntime('daemon_shutdown') loop at run-qwen-serve.ts:7431 and the fallback bridge.shutdown() loop at :7461) leaves the test green. The sibling multi-workspace test pins shutdown({ reason: 'daemon_shutdown' }) only for non-lifecycle bridges, so no test anywhere pins disposal for lifecycle-capable runtimes. — Failure scenario: a future change dropping lifecycle-capable runtimes from the shutdown disposal loop orphans the bridges' qwen --acp children and leaks coordinator keepalive timers while this test — whose title promises exactly this — stays green. — Concrete cost: after await closing;, assert expect(primaryBridge.shutdown).toHaveBeenCalledWith({ reason: 'daemon_shutdown' }) (and same for the secondary), mirroring the sibling test.
中文说明
'drains every lifecycle runtime before close yields' 只钉住了 draining 状态（coordinator 存在、ensure() 以 workspace_draining 拒绝、preheat 未被调用），在 await closing; 之后没有任何断言——尽管 mock 携带了 shutdown: vi.fn() 记录器，却没有对 primaryBridge.shutdown/secondaryBridge.shutdown 或注册表移除的断言。经探针验证：把 shutdown 路径改为完全跳过 lifecycle-capable runtime 的处置（同时跳过 run-qwen-serve.ts:7431 的 listManaged() → disposeRuntime('daemon_shutdown') 循环和 :7461 的兜底 bridge.shutdown() 循环），该测试仍为绿色。兄弟的多 workspace 测试只为非 lifecycle bridge 钉住了 shutdown({ reason: 'daemon_shutdown' })，因此没有任何测试为 lifecycle-capable runtime 的处置提供保证。— 失败场景：未来某次改动把 lifecycle-capable runtime 从 shutdown 处置循环中移除时，bridge 的 qwen --acp 子进程会成为孤儿、coordinator 的 keepalive 定时器会泄漏，而这个标题恰好承诺该行为的测试却保持绿色。— 具体代价：在 await closing; 之后断言 expect(primaryBridge.shutdown).toHaveBeenCalledWith({ reason: 'daemon_shutdown' })（secondary 同理），与兄弟测试保持一致。
— qwen3.8-max via Qwen Code /review (v0.21.10)
qwen-code-dev-bot
added
the
autofix/needs-human
|
🔓 Takeover auto-released: the autofix loop paused on this PR 10 day(s) ago (🤖 AutoFix stopped: this counting window now contains 3 time-budget exhaustions (pushed rounds in between included; this ) and no re-arm followed, so the 中文说明🔓 已自动释放接管：autofix 循环在 10 天前暂停于此 PR（🤖 AutoFix stopped: this counting window now contains 3 time-budget exhaustions (pushed rounds in between included; this ），此后无人重新武装，现移除 |
qwen-code-dev-bot
removed
the
autofix/takeover
4 participants
What this PR does
This PR establishes WorkspaceRuntime as the ownership boundary for each workspace's ACP child lifecycle. It adds an authoritative five-state runtime snapshot, workspace-scoped monotonic epochs, physical work leases, bounded startup and teardown behavior, and explicit primary and workspace-qualified runtime ensure/status APIs.
An explicit ensure starts or reuses the workspace ACP runtime without creating a user-visible Session. A successful ensure renews a ten-minute keepalive window, while repeated or concurrent calls reuse the same physical startup and preserve the longest requested window. Session work, workspace control requests, MCP discovery, and pending MCP authentication now participate in channel lifetime decisions so an idle timeout cannot reclaim the child while physical work remains.
The change also integrates runtime admission with workspace removal, trust replacement, and daemon shutdown; exposes the additive API through the TypeScript SDK; retains the existing trusted-primary startup preheat during migration; and documents the implemented foundation separately from the future capability/Catalog convergence design.
Why it's needed
Workspace management currently depends too heavily on Session lifecycle. Runtime-backed state such as MCP, Skills, Tools, and Extensions belongs to the workspace ACP runtime, but starting, retaining, and reclaiming that runtime has historically been inferred from Session activity or legacy preheat calls. That makes zero-Session management fragile, allows management work to race channel reclamation, and leaves ownership unclear across primary and secondary workspaces.
This foundation gives the daemon one workspace-scoped lifecycle authority without introducing a second runtime or hidden management Session. It creates the stable ownership, epoch, lease, routing, and SDK contracts needed for later work to converge Extensions, MCP, Skills, and Tools behind a single parameterless ensure flow.
Reviewer Test Plan
How to verify
Automated evidence on macOS: ACP Bridge 494 tests passed; runtime coordinator/routes/error mapping 25 tests passed; TypeScript SDK client 306 tests passed; repository build, lint, typecheck, and diff whitespace validation passed.
Evidence (Before & After)
N/A — daemon lifecycle, API, SDK, tests, and documentation only; no UI changes.
Tested on
Environment (optional)
Local Node.js workspace with mocked ACP channels for lifecycle/concurrency coverage. A real-child process-level E2E remains recommended in CI or reviewer validation.
Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
本 PR 将 WorkspaceRuntime 确立为每个工作区 ACP 子进程生命周期的所有权边界。它新增权威的五态 runtime 快照、workspace 级单调 epoch、物理 work lease、有界启动与回收行为，以及 primary 和 workspace-qualified 的 runtime ensure/status API。
显式 ensure 可以在不创建用户可见 Session 的情况下启动或复用 workspace ACP runtime。ensure 成功后会续期十分钟保活窗口；重复或并发调用复用同一次物理启动，并保留最长的保活窗口。Session 工作、workspace control 请求、MCP discovery 和 pending MCP authentication 都参与 Channel 生命周期判断，因此 idle timeout 不会在物理工作仍存在时回收子进程。
本变更还把 runtime admission 接入 workspace removal、trust replacement 和 daemon shutdown；通过 TypeScript SDK 暴露增量 API；迁移期间保留受信任 primary 的既有 startup preheat；并在文档中明确区分已实现的 foundation 与后续 capability/Catalog 收敛目标。
为什么需要
当前 workspace 管理对 Session 生命周期依赖过重。MCP、Skills、Tools 和 Extensions 等运行时状态属于 workspace ACP runtime，但其启动、保留与回收历史上通过 Session 活动或 legacy preheat 间接推断。这会让零 Session 管理不稳定，使管理操作与 Channel 回收产生竞争，也会模糊 primary 与 secondary workspace 之间的所有权。
该 foundation 在不引入第二套 runtime 或隐藏管理 Session 的前提下，为 daemon 提供唯一的 workspace-scoped lifecycle 权威。它建立了后续通过统一无参数 ensure 收敛 Extensions、MCP、Skills 和 Tools 所需的 ownership、epoch、lease、routing 与 SDK 契约。
Reviewer 测试计划
如何验证
macOS 自动化证据：ACP Bridge 494 项测试通过；runtime coordinator/routes/error mapping 25 项测试通过；TypeScript SDK client 306 项测试通过；仓库 build、lint、typecheck 和 diff 空白检查通过。
前后对比证据
N/A——仅涉及 daemon lifecycle、API、SDK、测试和文档，没有 UI 变更。
测试环境
本地 Node.js workspace，并使用 mock ACP Channel 覆盖生命周期和并发行为。仍建议由 CI 或 reviewer 补充真实 child 的进程级 E2E。
风险与范围
关联 Issue
N/A