feat(serve): add workspace-scoped MCP management - #10679
Conversation
…e-runtime-mcp # Conflicts: # docs/design/daemon-multi-workspace-hardening.md # docs/design/session-idle-reaper/README.md # docs/design/workspace-runtime-architecture.md # docs/developers/daemon/00-index.md # docs/developers/daemon/03-acp-bridge.md # docs/developers/daemon/05-mcp-transport-pool.md # docs/developers/daemon/08-session-lifecycle.md # docs/developers/daemon/17-configuration.md # docs/developers/daemon/20-quickstart-operations.md # docs/users/qwen-serve-deploy-local.md # packages/acp-bridge/src/bridge.test.ts # packages/acp-bridge/src/bridge.ts # packages/cli/src/commands/serve.ts # packages/cli/src/serve/routes/workspace-runtime.test.ts # packages/cli/src/serve/server/error-response.test.ts # packages/cli/src/serve/types.ts # packages/cli/src/serve/workspace-runtime-coordinator.test.ts # packages/cli/src/serve/workspace-runtime-coordinator.ts # packages/cli/src/serve/workspace-service/index.ts # packages/sdk-typescript/test/unit/DaemonClient.test.ts
|
Thanks for the PR!
Moving on to code review. 🔍 中文说明感谢贡献！
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Code reviewThis reads like a careful implementation of the target design, not an improvisation. The coordinator stays the single writable owner of MCP capability state, every write is guarded by Things worth a maintainer's attention:
Everything else checked out on read: the SDK keeps enable/disable out of runtime actions, qualified routes never fall back to primary, sequenceDiagram
participant P1 as Web Shell MCP page
participant P2 as WorkspaceRuntimeCoordinator
participant P3 as Bridge
participant P4 as ACP child
P1->>P2: POST runtime ensure
P2->>P3: preheat with 10 minute keepalive
P3->>P4: spawn or reuse channel
P2->>P2: queue MCP prepare at current revision
P2->>P4: MCP initialize when discovery not started
loop poll every 250 ms until completed or budget
P2->>P3: read MCP status tagged with epoch
P3-->>P2: live status for current epoch
end
P2-->>P1: lifecycle plus capabilities.mcp ready or starting
Files changed (30 of 38 shown)
TestingUnattended run — the PR's code is not executed here; the evidence below is the PR's own CI on the reviewed commit, fetched once via the API (no polling). Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The main unit suite, Serve A/B, daemon E2E, and web-shell visual capture were still running at review time — three PR-CI workflow runs ( Sandboxed verification would settle this: 中文说明代码审查这是一次对目标设计的忠实实现，而不是自由发挥：coordinator 仍是 MCP capability 状态的唯一可写所有者，每次写入都受 需要维护者关注的点：
其余部分读下来没有问题：SDK 未把 enable/disable 混入 runtime 动作；qualified 路由不回退 primary；活跃会话工具刷新所用的 （时序图见正文：ensure → preheat/keepalive → 串行 MCP prepare → 轮询带 epoch 的状态直至 ready/starting。） 测试无人值守运行——此处不执行 PR 代码；以下证据是 PR 自身 CI 在被审提交上的结果，通过 API 一次性获取（不轮询）。正文表格中：主单测套件、Serve A/B、daemon E2E、web-shell 视觉截图在审查时仍在运行——该提交上还有三个 PR CI 工作流（ 沙箱验证可以钉住这些： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — a disciplined, design-faithful implementation with tests that genuinely pin the state machine, but the Stage 0 core-size escalation caps this at policy, and one doc/behavior mismatch needs a maintainer to settle it. Stepping back: before reading the diff I sketched what this PR would have to look like from the target design alone — coordinator as the single writable owner of epoch-bound MCP status, durable config routes split from runtime routes, one process-wide OAuth lane, and an ensure-on-entry Web Shell migration. The PR matches that shape almost exactly, and goes further in the right directions (drain-rollback replay, management-operation counting toward removal admission, revision coalescing). I did not find a materially simpler path — the architecture doc mandates this structure, and the predecessor (#10593) already landed the foundation it builds on. If I had to maintain this in six months, the revision/epoch discipline and the test suite would read well, not feel imposed. Why not higher:
No approval submitted. Deferring to the maintainers — this comment is the explicit escalation; no owner could be resolved deterministically (no labels on the PR, no prior human review, no configured handle), so whoever picks it up: the two questions above are the ones I could not settle from the diff alone. — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
What this PR does / PR 主旨
This PR lands the workspace-scoped MCP management stage of the runtime architecture: the workspace runtime coordinator becomes the single writable owner of MCP capability state (every write guarded by revision + runtimeEpoch, all runtime mutations through one serial lane), durable MCP config routes are split from live runtime routes (/workspace(s)/.../mcp-config vs /runtime/mcp), a single process-wide OAuth admission lane is shared by all bridge instances, the SDK gains user-scope and workspace-scoped MCP config/runtime methods, and the Web Shell migrates to ensure-on-entry with epoch-checked catalog reads. It explicitly does not fall back to older daemons lacking the new routes (disclosed in the PR body), building on the foundation landed by PR 10593.
本 PR 落地 runtime 架构的 workspace 级 MCP 管理阶段：workspace runtime coordinator 成为 MCP capability 状态的唯一可写所有者（每次写入受 revision + runtimeEpoch 双守卫，所有 runtime mutation 走单一串行 lane）；持久化 MCP 配置路由与实时运行时路由分离（/workspace(s)/.../mcp-config vs /runtime/mcp）；进程级单一 OAuth 准入 lane 供所有 bridge 实例共享；SDK 增加用户级与 workspace 级的 MCP 配置/运行时方法；Web Shell 迁移为进入页面即 ensure、读取 catalog 前校验 epoch。明确不再回退缺少新路由的旧版 daemon（PR 正文已披露），建立在 PR 10593 打下的基础之上。
Review basis / 审查基线: head e11f8125ec913f6bf41d5d95aaf61c46ed5ecfe8, full static review (38 files, +3849/−563), all findings below cross-verified against a head tree at that SHA; no tests/builds were run. CI at publish time: all listed checks green (Test ubuntu, Serve A/B, web-shell visuals, Real daemon E2E, Desktop Shell, Integration Tests, Live Host, Java matrix, secret scan, CVE audit, etc.) except web-shell E2E Smoke = cancelled (the rollup renders it as fail, but it is a cancellation, not a test failure); the review-pr bot check was still pending. Not polled or waited on.
What reads well / 做得好的地方
- The revision/epoch discipline is consistent end to end: the coordinator rejects superseded operations and replaced ACP children, the serial physical lane keeps mutations ordered, and
activation: applied|deferred|reconcilingis reported honestly instead of optimistically. - The process-wide OAuth lane is one closure wired into every bridge creation site, with release paths covering synchronous rejection, non-pending responses, channel teardown, and the timeout-retire path, so the global lane cannot wedge.
- Web Shell pages verify epoch equality before trusting any catalog read — exactly what the architecture doc asks of pages.
- Test coverage genuinely pins the state machine: revision coalescing, cold deferral, drain-rollback replay, cross-workspace OAuth rejection, admission retention after a timed-out RPC, and untrusted-workspace read-vs-mutate asymmetry all have dedicated cases.
- The 13 endpoint contracts (path/method/fields/encoding) between routes, coordinator, bridge, and the SDK line up one by one — no dead fields found in the SDK sweep.
Findings / 发现
🔴 1. Critical — floating prepareMcp() promise in ensure() can take down the whole daemon
packages/cli/src/serve/workspace-runtime-coordinator.ts:159-168: ensure() calls const preparation = this.prepareMcp() and only attaches it to withTimeout(...) when remainingMs > 0. When the deadline is already exhausted, preparation floats with no handler at all (the > 0 path is safe because withTimeout's race always attaches handlers).
Two verified rejection paths feed it:
queueMcpWork(:278-294) swallows the queue-tail copy of the rejection but theoperationpromise returned to the caller is unprotected; the queued body'sassertAcceptingWork(:283) throwsWorkspaceDrainingErroroncebeginDrain()has run (called from the workspace-removal flow,workspace-management.ts:1586). BecauseprepareMcpRevisionpolls on a 2-minute budget (:296-351), a queued prepare can resolve minutes afterensure()gave up — so "remove this workspace within ~2 minutes after its ensure timed out" lands the rejection.- The out-of-try
await this.bridge.preheat(...)at :296-302 can reject on spawn failure.
The consequence chain is not hypothetical: the serve fast path installs no process-level unhandledRejection handler — a hazard you documented yourself at run-qwen-serve.ts:6527-6533 ("Node's default for one is to exit"), with a test asserting that behavior. One unhandled rejection here exits the daemon process, taking down every runtime, session, and connection it serves. Every sibling fire-and-forget in this same file attaches .catch (:206-213 scheduleMcpReconciliation, :251-259 runMcpRuntimeMutation); this one is the gap, and it is diff-new code.
Suggested fix — mirror :210-213 unconditionally:
const preparation = this.prepareMcp();
preparation.catch((error) =>
this.recordMcpError(
this.mcpRevision,
this.bridge.getWorkspaceRuntimeLifecycleSnapshot().runtimeEpoch,
error,
),
);
const remainingMs = deadline - Date.now();
if (remainingMs > 0) { ... }触发链：ensure() 超时后约 2 分钟内删除该 workspace（或预热阶段 spawn 失败）→ 漂浮的 prepareMcp() 拒绝 → serve 无进程级 unhandledRejection 兜底 → Node 默认退出进程 → 整个 daemon 连同所有 runtime/session/连接一起退出。同文件其他 fire-and-forget 均有 .catch，此处为 diff 新增代码中的遗漏。修复：无条件挂 preparation.catch(...) 记录错误。
🟡 2. Important — manageServer inferred return union breaks property access at 5 consumer sites (needs a local typecheck to settle)
packages/web-shell/client/daemon/workspace/hooks/useDaemonMcp.ts:75-111: manageServer has no return annotation. The async arrow returns three differently-shaped objects, so TypeScript infers a union — and the enable/disable literal ({serverName, action, ok: true as const, ...(changed ... ? {} : {changed})}) does not carry messages, authUrl, or pending. Consumers then access those properties on the unnarrowed union:
McpManagerPage.tsx:946/947/949—result.authUrl,result.messages,result.pendingMcpStatusMessage.tsx:524/525—result.messages,result.authUrl
Call-site narrowing of the action argument does not narrow the returned union, and nothing in the consumption chain constrains it: daemon-react-sdk.ts:98 re-exports the hook plainly, and both pages call it directly (the DaemonWorkspaceActions interface only types actions.manageMcpServer, not the hook's manageServer).
Why CI green does not settle it either way: this PR's CI gate never runs a package-level typecheck for web-shell — ci.yml only runs typecheck:integration; the root npm run typecheck lives in preflight/release flows, not the PR gate. A static read says these five accesses are TS2339 errors; that conflicts with the PR body's "full typecheck passed" claim, and I cannot run tsc in this review. Please run npm -w packages/web-shell run typecheck at e11f812 — if something saves it (a narrowing mechanism I'm missing), a reply noting it would close this out.
Suggested fix regardless — one annotation makes the contract explicit and removes the union:
manageServer: async (
serverName: string,
action: Parameters<typeof actions.manageMcpServer>[1],
): Promise<DaemonMcpManageResult> => { ... }（与本 PR 正文"全量 typecheck 通过"的声明冲突，静态阅读无法调和；请在该 SHA 上跑一次 npm -w packages/web-shell run typecheck 确认——若有隐式机制救场，回复说明即可关闭本条。）
🟡 3. Important — MCP status is stamped with the epoch at response arrival, not request send
packages/acp-bridge/src/bridge.ts:6100-6214 (requestWorkspaceStatus): the stamp at :6135-6143 reads the closure's current runtimeEpoch when the response arrives — there is no epochAtRequest capture before the await and no liveChannelInfo() recheck after it. Deferred channel retirement (:3559) keeps the replaced channel OPEN while sessions drain, so a late completed response from the old ACP child can win the race against channel.exited and get stamped with the new epoch + source:'live', then written into workspaceMcpStatusCache (:6209). The coordinator (:324-325) and McpManagerPage (:496-497) would then treat a stale child's completed state as the current runtime's live ready — violating exactly the epoch-trust invariant this PR states for pages.
The same file already contains the correct pattern: expireWorkspaceMcpAuthentication (:6217-6225) checks liveChannelInfo() !== info before trusting the captured channel. Suggested fix, mirroring it:
const info = liveChannelInfo();
const epochAtRequest = this.runtimeEpoch;
// ... await extMethod ...
if (liveChannelInfo() !== info || epochAtRequest !== this.runtimeEpoch) {
throw new BridgeChannelClosedError(...);
}（epoch 在响应到达时才读取；延迟退役通道保旧通道 OPEN，旧子进程的迟到 completed 可能被打上新 epoch + live 写入缓存，违反本 PR 自述的 epoch 信任不变量。同文件 :6217-6225 已有正确先例可参照。）
🟡 4. Important — tools/resources cache writes are unstamped, so cache hits can read as false discovery timeouts
packages/acp-bridge/src/bridge.ts: cacheWorkspaceMcpDetails (:6000-6053) writes the raw extMethod result into the tools/resources caches without runtimeEpoch, and the idle fallbacks in getWorkspaceMcpToolsStatus/getWorkspaceMcpResourcesStatus (:11810-11868) return {...cached, acpChannelLive: false} — again no runtimeEpoch. But the clients do hard epoch comparison (McpManagerPage.tsx:633/657, McpStatusMessage.tsx:469: result.runtimeEpoch !== runtimeStatus.runtimeEpoch), so a cache-served read fails the comparison (undefined !== N) and surfaces as a "discovery timeout" instead of the cache display the design intends. Reachability is narrowed in practice (loadReadyRuntime requires live+ready+epoch match, and ensure's keepalive is 10 minutes), but the stamp asymmetry is a latent trap with a one-line fix: stamp runtimeEpoch when writing the cache entries, same as the live writers at :11835/:11866 already do.
（tools/resources 缓存写入未盖 runtimeEpoch，而客户端是硬比较 → 缓存命中被误判为 discovery timeout；可达性虽被 ensure 的 10 分钟 keepalive 收窄，但修复只需缓存写入时盖章。）
🟡 5. Important — enable/disable from the chat MCP card drops activation, so a cold runtime burns ~121s before a misleading timeout
McpStatusMessage.tsx:380-425 includes enable/disable in serverActions and routes them through manageServer (:521-526). Neither the hook's enable/disable literal nor DaemonMcpManageResult (sdk types.ts:4001-4009) carries activation, so this path has no activation === 'deferred' short-circuit — unlike McpManagerPage.runAction (:909-940), which tracks activation and short-circuits deferred writes. Concrete scenario: cold runtime + a cached server list → the card's reloadSelectedServer loop polls 81 × 1.5s ≈ 121s and then reports a misleading mcp.discovery.timeout, where the manager page would have shown "applied on next start" immediately. Secondary: the changed: false double-scope write against a cold runtime lands the same page in the same trap. Suggested fix: thread activation through the manage-result type and short-circuit deferred in both consumers.
（聊天卡片里的 enable/disable 丢失 activation 字段，无 deferred 短路；冷 runtime + 缓存列表场景下空转约 121 秒后报误导性超时。建议结果类型穿透 activation 并在两个消费者都短路 deferred。）
🟢 Nits
manageRuntimeMcpServerlacks theopts?.timeoutMsoverride its sibling has.DaemonClient.ts:6036-6051hardcodes 330s (MCP_RESTART_DEFAULT_TIMEOUT_MS) while siblingmanageMcpServer(:4907-4933) accepts an override; the 10-minute authenticate budget inactions.ts:400is dead code because the inner SDK fetch aborts at 330s first. In practice the budget is adequate — the ACP-side authenticate handler returns as soon as the auth URL exists (30s start budget,acpAgent.ts:474) — so this is symmetry cleanup, not a timeout bug.parseReloadOptionsduplication with wording drift.workspace-runtime-mcp.ts:33-72re-implementsparseMcpReloadOptions(workspace-mcp-control.ts:46-88) with slightly different error messages; worth collapsing into one shared helper before they drift further.reloadSelectedServerpolling loop has no mounted check.McpStatusMessage.tsx:430-458fires up to 81 × 1.5s requests without consultingmountedRef/waitForPoll, leaving ~160 requests in flight after unmount.
💬 Low confidence / human check
useDaemonMcp.ts:34-39binds event-driven reloads to the primary workspace's signals (useDaemonWorkspaceEventSignals) even when browsing a secondary workspace viaworkspaceCwd. The fan-out semantics of those signals across workspaces were not verified in this pass — worth a maintainer's glance.
Notes on the bot triage (same SHA) / 与机器人审查的分歧
- The bot's claim that
manageServer's enable/disable branch is "unreachable from the current UI" does not hold:McpStatusMessage.tsx:405-411renders enable/disable actions and routes them throughmanageServer(see finding 5). 机器人"enable/disable 分支不可达"的论断已证伪。 - The bot's bare-preheat/documentation mismatch question (§8.3 / §14.4 / §16.1 of the architecture doc vs the
preserveForFirstStatusReadremoval) was not independently re-verified in this pass — carried forward as an open maintainer question, not counted among this review's findings. - The dropped older-daemon fallbacks are disclosed in the PR body; treated as a confirmed design decision.
Conclusion / 结论
COMMENT. One verified Critical (floating promise → daemon-exit path) and four Important items above. The state machine, OAuth lane, and test coverage are in good shape — the Critical is a narrow-window but high-blast-radius omission with a one-line fix, and the type-union item needs a local typecheck to settle. Self-authored PR, so no approval is possible here regardless; requesting the fixes or a reply addressing each point.
结论：COMMENT。1 条已验证 Critical（触发窗口窄但爆炸半径大，一行可修）+ 4 条 Important。状态机、OAuth lane 与测试覆盖整体扎实；类型联合那条需本地 typecheck 定论。本 PR 为作者本人提交，无法 approve，请逐条处理或回复。
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Independent review of head e11f8125ec913f6bf41d5d95aaf61c46ed5ecfe8. Findings frozen before reading the existing review; cross-check outcomes recorded below.
Scope
Static review: coordinator, bridge, config routes, runtime-MCP routes, SDK. Not read: McpStatusMessage full body, requestWorkspaceStatus pre-existing body. No local toolchain (rungs not run).
CI
ubuntu Test green, Desktop Shell green, Integration no-AK green. web-shell E2E cancelled. Test win/mac + Integration CLI: SKIPPED.
Own finding
F1 (Suggestion) - publishConfiguration (workspace-mcp-config.ts:~435) has 3 empty catch{} blocks for broadcast notifications. Intentional best-effort; observability concern only, not blocking.
Cross-check outcomes
Confirmed Critical — R1: floating prepareMcp() promise in ensure()
workspace-runtime-coordinator.ts:159-168: when deadline - Date.now() <= 0, const preparation = this.prepareMcp() has no .catch() handler. queueMcpWork returns the operation promise directly — mcpPhysicalTail absorbs the tail copy but does NOT suppress the caller-facing rejection. prepareMcpRevision can throw (preheat failure at :296, assertAcceptingWork drain-while-queued at :283, poll-loop errors at :296-351), arriving up to 120 s after ensure() returned. No process-level unhandledRejection exists (run-qwen-serve.ts:6527-6533) — Node's default exit fires. Every other fire-and-forget in the coordinator attaches .catch (lines 206-213, 251-259); this one does not.
Fix: ```ts
preparation.catch((error) => this.recordMcpError(
this.mcpRevision,
this.bridge.getWorkspaceRuntimeLifecycleSnapshot().runtimeEpoch, error));
### Cross-check misses (could not rule out)
- **R2** (TypeScript union in manageServer): McpManagerPage routes enable/disable via setConfigServerEnabled — that call site is clean. McpStatusMessage full body not read; typecheck not run. Cannot confirm or refute.
- **R3** (epoch at response arrival): Did not read pre-existing requestWorkspaceStatus body. Consistent with the diff evidence but unconfirmed.
- **R4** (tools/resources cache unstamped): cacheWorkspaceMcpDetails not read. Unverified.
- **R5** (enable/disable drops activation in McpStatusMessage): McpManagerPage confirmed clean. McpStatusMessage not fully read.
## Conclusion
COMMENT. R1 confirmed Critical — one-line fix. R2-R5 need maintainer verification. The state machine, OAuth lane, and MCP serial mutation queue are well-designed.
_Reviewed with AI assistance._
<!-- pr-review head=e11f8125ec913f6bf41d5d95aaf61c46ed5ecfe8 round=1 -->
|
Verification report — head Scope: full diff (38 files, +3849/−563) — coordinator, config/runtime route modules, acp-bridge, SDK client, web-shell MCP manager, design docs. Critical-focused review — no Critical foundRoute ownership (per the workspace-route rules). Coordinator state machine. Every MCP status write is guarded by the monotonic Cross-check of the standing R1 (floating OAuth admission. Secrets round-trip. Removals. Local verification at head (codeload tarball +
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 1: could not execute the modified test (review worktree has no node_modules; npx vitest failed to resolve vitest/config ) — verified it statically against the i…; "agent 1c": none — no check was cut short..
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 1：could not execute the modified test (review worktree has no node_modules; npx vitest failed to resolve vitest/config ) — verified it statically against the i…；"agent 1c"：none — no check was cut short.。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| await new Promise((resolve) => window.setTimeout(resolve, 1_500)); | ||
| } | ||
| if (!nextStatus || runtimeEpoch === undefined) { |
There was a problem hiding this comment.
[Suggestion] R1-13: McpStatusMessage poll loop has no unmount guard
The reloadSelectedServer poll loop runs up to 81 × 1.5s of daemon calls with no mounted/cancellation check — the component has no mountedRef at all, and the action is fire-and-forget from the keydown handler. Triggering a server action then switching conversation unmounts McpStatusMessage mid-poll: the detached loop keeps issuing runtimeStatus (and possibly reload/loadTools) daemon calls for up to ~2 minutes (11 calls measured in 15s of fake time after unmount), then calls setLocalStatus/setLocalToolsByServer on the unmounted component — wasted daemon traffic from a dead component for every abandoned action. Track mounted state in a ref and break the loop — and guard each setLocal* call — when unmounted, as McpManagerPage's waitForPoll does.
Witness:
Probe: render McpStatusMessage, trigger enable via keyboard, unmount mid-poll, advance 15s fake time — intact PR: calls before unmount 1, calls after +15s of polling post-unmount 11; with a mountedRef guard: 1.
Fix constraint: McpManagerPage's waitForPoll returns mountedRef.current and observeDiscovery bails with if (!mountedRef.current) return false; — the new guard should not change the 81-attempt budget the loops share.
Fix witness: New McpStatusMessage.test.tsx: with fake timers and a runtimeStatus mock that never becomes ready, start an action, unmount, advance timers past several 1.5s ticks, assert runtimeStatus is not called after unmount; removing the guard reds it.
中文说明
reloadSelectedServer 轮询循环最多会进行 81 × 1.5 秒的 daemon 调用，却没有任何 mounted/取消检查——组件根本没有 mountedRef，且操作是从 keydown 处理器以 fire-and-forget 方式发起的。触发服务器操作后切换会话会在轮询中途卸载 McpStatusMessage：脱离的循环会继续发出最多约 2 分钟的 runtimeStatus（以及可能的 reload/loadTools）daemon 调用（实测卸载后 15 秒虚拟时间内有 11 次调用），然后对已卸载组件调用 setLocalStatus/setLocalToolsByServer——每次被放弃的操作都会让一个已死组件浪费 daemon 流量。建议用 ref 跟踪 mounted 状态，在卸载时跳出循环并守卫每个 setLocal* 调用，与 McpManagerPage 的 waitForPoll 一致。
（Witness 证据、Fix constraint 修复约束与 Fix witness 修复见证见英文部分。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Thanks. Deferring this non-Critical suggestion to follow-up: this PR has already had more than five review rounds, and this pass is intentionally limited to correctness and regression fixes per project policy. No behavior change was made for this suggestion.
| const workspaces = useMemo(() => { | ||
| const listed = (workspace.capabilities?.workspaces ?? []).filter( | ||
| (entry) => entry.kind !== 'live', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-23: kind !== 'live' selector filter has no test witness
The kind !== 'live' filter — the only guard keeping daemon-owned live-conversation workspaces out of the MCP workspace selector — has no test witness; neither fixture in the new PluginManagerPage.test.tsx sets kind. /capabilities lists active internal entries (capabilities.ts:64-68) and stamps kind: 'live' for provenance 'live-conversation' (:130-133); resolveWorkspaceRuntimeFromParam does NOT filter internal runtimes (only resolveManagedWorkspaceRuntimeByPathSelector applies isInternalWorkspaceRuntime). Replacing the filter with () => true keeps the shipped suite green (2/2); a purpose-built probe test adding a kind:'live' entry flips pass→fail. Add a third workspace entry with kind: 'live' to the fixture and assert its label never appears among the selector's [role='option'] elements.
Witness:
Probe: mutant (filter → () => true) — shipped PluginManagerPage.test.tsx still passes 2/2; purpose-built probe test adding a kind:'live' fixture entry passes on intact code and fails on the mutant ('expected [ Primary, LiveConversation, …(1) ] to not include LiveConversation').
Fix constraint: The stamp value is exactly 'live' — routes/capabilities.ts:132 ? { kind: 'live' as const } (sdk-typescript types.ts:119 kind?: 'live';); the fixture must use that literal.
Fix witness: The new case asserting the live-kind workspace is absent from the option list; removing the entry.kind !== 'live' filter must make it fail.
中文说明
kind !== 'live' 过滤器——阻止 daemon 自有的会话级（live）workspace 进入 MCP workspace 选择器的唯一守卫——没有测试见证；新的 PluginManagerPage.test.tsx 两个 fixture 都没有设置 kind。/capabilities 会列出 active 的内部条目（capabilities.ts:64-68），并为 provenance 为 'live-conversation' 的条目盖上 kind: 'live'（:130-133）；resolveWorkspaceRuntimeFromParam 并不过滤内部 runtime（只有 resolveManagedWorkspaceRuntimeByPathSelector 应用 isInternalWorkspaceRuntime）。把过滤器替换为 () => true，现有套件仍然全绿（2/2）；专门构造的探针测试加入 kind:'live' 条目后由过转变失败。建议在 fixture 中加入第三个 kind: 'live' 的 workspace 条目，并断言其标签永远不出现在选择器的 [role='option'] 元素中。
（Witness 证据、Fix constraint 修复约束与 Fix witness 修复见证见英文部分。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Thanks. Deferring this non-Critical suggestion to follow-up: this PR has already had more than five review rounds, and this pass is intentionally limited to correctness and regression fixes per project policy. No behavior change was made for this suggestion.
| return withActionTimeout( | ||
| client.initializeWorkspaceMcp(), | ||
| workspace.ensureRuntime().then(() => ({ accepted: true })), | ||
| 'Initialize MCP timed out', |
There was a problem hiding this comment.
[Suggestion] R1-24: initializeMcp now always resolves accepted:true; consumer…
initializeMcp (and useDaemonMcp.initialize's workspaceClient branch, useDaemonMcp.ts:56-58) can now only ever resolve {accepted: true}, yet the sole consumer AgentCreatePage.tsx:227 still branches if (!initialization.accepted) await reloadMcpConfig() — permanently unreachable. The legacy contract genuinely returned accepted:false ('True only when this request started a new background discovery task'), so the type/comment now falsify the implementation; the dead branch also keeps reloadMcpConfig in the mount-effect dependency array (:274) — the very effect whose dependency instability the hook-identity Critical addresses. No runtime failure — the cost is a falsified contract and a stale dep on the effect under audit. Delete the dead branch and drop reloadMcpConfig from the effect deps (and the now-unused destructured binding), or change initializeMcp()'s result type so it no longer advertises an accepted flag nothing can make false.
Witness:
not run — dead-code fact settled by reading the construction site: the only value initializeMcp can resolve is the literal {accepted: true} (workspace.ensureRuntime().then(() => ({accepted: true}))), so no run could discriminate.
Fix witness: N/A (dead-code removal adds no guard a test can pin).
中文说明
initializeMcp（以及 useDaemonMcp.initialize 的 workspaceClient 分支，useDaemonMcp.ts:56-58）现在只可能 resolve {accepted: true}，但唯一消费者 AgentCreatePage.tsx:227 仍然保留 if (!initialization.accepted) await reloadMcpConfig() 分支——永远不可达。旧契约确实会返回 accepted:false（「仅当本次请求启动了新的后台 discovery 任务时为 true」），因此类型/注释现在与实现相悖；这个死分支还让 reloadMcpConfig 留在挂载 effect 的依赖数组中（:274）——正是 hook 身份 Critical 所处理的依赖不稳定的那个 effect。无运行时故障——代价是被伪造的契约和留在被审查 effect 上的陈旧依赖。建议删除死分支并从 effect 依赖中移除 reloadMcpConfig（连同不再使用的解构绑定），或修改 initializeMcp() 的结果类型，不再暴露一个不可能为 false 的 accepted 标志。
（Witness 证据、Fix constraint 修复约束与 Fix witness 修复见证见英文部分。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Thanks. Deferring this non-Critical suggestion to follow-up: this PR has already had more than five review rounds, and this pass is intentionally limited to correctness and regression fixes per project policy. No behavior change was made for this suggestion.
| const timeoutMs = action === 'authenticate' ? 10 * 60_000 : 5 * 60_000; | ||
| if (action === 'enable' || action === 'disable') { | ||
| const config = | ||
| action === 'disable' ? await workspace.mcpConfig() : undefined; |
There was a problem hiding this comment.
[Suggestion] R1-15: Disable scope diverges for .mcp.json-shadowed servers
For a server whose effective origin is project .mcp.json — which outranks user settings (config.ts:2071) — and whose name is ALSO defined in user settings, the disable-scope probe picks 'user' (serverName in config!.user is true from the raw user file; mcpConfig() returns the raw per-scope maps, .mcp.json in neither), while the legacy handler this replaces special-cased server.scope === 'project' → SettingScope.Workspace (acpAgent.ts:9484-9500), matching the design doc ('Disabling project or workspace servers writes the exclusion to workspace-local settings'). The chat card is the only UI that can disable project servers (isManagedServerVisible hides project_mcp_json from the manager page), so this is a regression on a live path: disabling such a server from the chat card writes mcp.excluded to USER settings — global across the user's workspaces — instead of workspace-local. Align the probe with origin semantics — route project-origin disables to workspace scope per the design doc — in both copies (actions.ts and useDaemonMcp.ts).
Witness:
not run — scope selection is a ternary over the quoted raw maps and the authority facts are quoted code/doc: precedence config.ts:2071 ('settings < project .mcp.json < workspace/system settings'), configOrigin 'project_mcp_json' derivation (acpAgent.ts:6510-6527), legacy special case (acpAgent.ts:9484-9500), design-doc rule.
Fix witness: actions.test.ts case: mcpConfig resolves a user-map entry for a server whose effective origin is project .mcp.json; manageMcpServer(name,'disable') must call the workspace-scope setMcpServerEnabled, not setUserMcpServerEnabled; red if the probe falls back to the raw user-map membership test.
中文说明
对于有效 origin 为项目 .mcp.json 的服务器（.mcp.json 优先于 user 配置，config.ts:2071），且其名称同时定义在 user 配置中时，disable 的 scope 探测会选择 'user'（serverName in config!.user 为 true，来自原始 user 文件；mcpConfig() 返回的是按 scope 的原始映射，其中不含 .mcp.json）；而它所替换的旧处理器对 server.scope === 'project' 有特殊分支 → SettingScope.Workspace（acpAgent.ts:9484-9500），与设计文档一致（「禁用项目或 workspace 服务器时排除项写入 workspace 本地配置」）。聊天卡片是唯一能禁用项目服务器的 UI（isManagedServerVisible 在管理页隐藏 project_mcp_json），因此这是一条实际可用路径上的回归：从聊天卡片禁用此类服务器会把 mcp.excluded 写入 USER 配置——对该用户的所有 workspace 全局生效——而不是 workspace 本地配置。建议在两个副本（actions.ts 与 useDaemonMcp.ts）中让探测与 origin 语义对齐——按设计文档把项目 origin 的 disable 路由到 workspace scope。
（Witness 证据、Fix constraint 修复约束与 Fix witness 修复见证见英文部分。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Thanks. Deferring this non-Critical suggestion to follow-up: this PR has already had more than five review rounds, and this pass is intentionally limited to correctness and regression fixes per project policy. No behavior change was made for this suggestion.
| () => | ||
| workspaceClient ? workspaceClient.runtimeMcp() : actions.loadMcpStatus(), | ||
| [actions, workspaceClient], |
There was a problem hiding this comment.
[Suggestion] R1-16: Cross-workspace auto-reload is keyed to the host session's…
With workspaceCwd set, load/mutations route to the requested workspace, but useWorkspaceEventReload is still keyed to the HOST session's signals?.mcpVersion. mcpVersion bumps only on workspace.mcp.* events arriving in the ACTIVE session's stream (DaemonSessionProvider.tsx:4961-4966; both bump call sites are gated on activeWorkspaceScoped), and daemon-side MCP events publish only on the affected workspace's bridge (server.ts:1283-1290). McpManagerPage — the only workspaceCwd consumer — has no continuous idle polling (only bounded 1.5s loops), so a foreign workspace's externally-driven MCP change (a CLI session on B restarting/approving/authenticating a server, another client editing B's workspace config) never refreshes the view until manual Refresh, while any MCP event on host workspace A spuriously re-fetches B's status. Key the reload signal to the requested workspace — a per-workspace MCP signal analogous to skillMutationsByCwd — or at minimum skip the host-signal-driven reload when workspaceClient is active.
Witness:
not run — the wiring gap is quoted through four layers: useDaemonMcp.ts:36-38 keys useWorkspaceEventReload to signals?.mcpVersion; bumps only on active-session workspace.mcp.* events (DaemonSessionProvider.tsx:4961-4966); events publish only to the affected workspace's bridge (server.ts:1283-1290 → workspace-service/index.ts:1297-1330); no per-cwd MCP signal exists to subscribe to.
Fix constraint: useWorkspaceEventReload.ts:16-23 skips the first observed version via hasMountedRef — the fix must preserve no-reload-on-mount semantics so the initial explicit load is not duplicated.
Fix witness: Render-hook test: with workspaceCwd set, a host-signal mcpVersion bump must NOT call workspaceClient.runtimeMcp() again (and a target-workspace signal source, once added, must); red on inversion.
中文说明
设置了 workspaceCwd 时，加载/变更会路由到所请求的 workspace，但 useWorkspaceEventReload 仍然以宿主会话的 signals?.mcpVersion 为键。mcpVersion 只在到达活跃会话事件流的 workspace.mcp.* 事件时递增（DaemonSessionProvider.tsx:4961-4966；两处递增调用点都以 activeWorkspaceScoped 为条件），而 daemon 侧 MCP 事件只发布到受影响 workspace 的 bridge（server.ts:1283-1290）。McpManagerPage——唯一传 workspaceCwd 的消费者——没有持续的空闲轮询（只有有界的 1.5 秒循环），因此其他 workspace 的外部驱动 MCP 变化（B 上的 CLI 会话重启/审批/认证服务器，或其他客户端编辑 B 的 workspace 配置）永远不会刷新视图，直到手动 Refresh；而宿主 workspace A 上的任何 MCP 事件都会无谓地重新拉取 B 的状态。建议把刷新信号改为以所请求的 workspace 为键——类似 skillMutationsByCwd 的按 workspace MCP 信号——至少应在 workspaceClient 激活时跳过宿主信号驱动的 reload。
（Witness 证据、Fix constraint 修复约束与 Fix witness 修复见证见英文部分。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Thanks. Deferring this non-Critical suggestion to follow-up: this PR has already had more than five review rounds, and this pass is intentionally limited to correctness and regression fixes per project policy. No behavior change was made for this suggestion.
…cp' into codex/daemon-workspace-runtime-mcp
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
6 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-16 self-satisfying labelFor assertions in the priority test (App.test.tsx:18826) — already reported (comment 3905016952)
- R1-21 dead-string negative assertions in 'does not show MCP discovery progress' (App.test.tsx:19278) — already reported (comment 3905016959)
- R1-30 McpStatusMessage poll loop has no unmount guard (McpStatusMessage.tsx:430-477) — already reported (comment 3905017012)
- R1-26 multi-scope enable commits user scope before workspace scope without rollback (actions.ts:414-417) — already reported (comment 3905016986)
- R1-32 initializeMcp always resolves accepted:true, leaving AgentCreatePage's reloadMcpConfig branch dead (actions.ts:337) — already reported (comment 3905017026)
- R1-19 server-side coverage gaps on the runtime-MCP route family (workspace-runtime-mcp.ts) — already reported (comment 3905016903)
Not reviewed: build-and-test — the test phase did not run (the harness budget expired before the test phase after 16/17 workspaces built cleanly; the coordinator suite 26/26 and useDaemonMcp 2/2 were run green by review agents).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 5: executing packages/cli/src/serve/workspace-runtime-coordinator.test.ts under vitest to confirm the manual traces — the review worktree has no node_modules , ….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/routes/workspace-runtime-mcp.ts:229 — [review] runtime mutation routes drop the originator client id (parity with legacy X-Qwen-Client-Id validation)packages/cli/src/serve/workspace-runtime-coordinator.ts:343 — [review] three prepareMcpRevision branches unpinned (not_started kickoff / discovery errors / prepare deadline)packages/cli/src/serve/workspace-runtime-coordinator.test.ts:121 — [review] live epoch-mismatch stale projection unpinnedpackages/cli/src/serve/workspace-runtime-coordinator.test.ts:191 — [review] 'observes an MCP reload already in progress' cannot discriminate observe-vs-retrypackages/web-shell/client/App.test.tsx:18858 — [review] priority test's message-side disabled-first / succeeded-passthrough edges unpinnedpackages/web-shell/client/App.test.tsx:18936 — [review] 'observes MCP config reconciliation and reports a timeout' cannot discriminate its two pathspackages/web-shell/client/App.test.tsx:19148 — [review] message-side reload live/epoch gate unpinnedpackages/web-shell/client/daemon/workspace/actions.ts:356 — [review] loadMcpTools rejection blanks AgentCreatePage's catalog (Promise.all)packages/cli/src/serve/workspace-runtime-coordinator.test.ts:392 — [review] 'abandons stale MCP preparation' test cannot discriminate the abandonmentpackages/web-shell/client/App.test.tsx:19352 — [review] manager-side freshness-gate conjuncts both violated by one fixturepackages/cli/src/serve/workspace-qualified-rest.test.ts:1264 — [probe] config/mcp toggle changed:true branch never exercised (persist / publish / exact-name exemption)packages/web-shell/client/App.test.tsx:18582 — [probe] refreshAll's observeDiscovery half unpinnedpackages/web-shell/client/App.test.tsx:19150 — [probe] rewritten poll loop bound pinned only at >=2packages/web-shell/client/App.test.tsx:19300 — [probe] init-effect epoch-mismatch fall-through untested
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 6 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — the test phase did not run (the harness budget expired before the test phase after 16/17 workspaces built cleanly; the coordinator suite 26/26 and useDaemonMcp 2/2 were run green by review agents)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 5：executing packages/cli/src/serve/workspace-runtime-coordinator.test.ts under vitest to confirm the manual traces — the review worktree has no node_modules , …。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 14 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| } | ||
| }); | ||
|
|
||
| it('rejects an MCP mutation completed by a closed generation', async () => { |
There was a problem hiding this comment.
[Suggestion] R2-10: The post-completion generation-close rejection added by the R1-5 fix (503 workspace_runtime_unavailable) is tested only for /runtime/mcp/:server/restart; the identical runtime.generationGuard?.assertOpen() guard in the sibling reload, approve, authenticate, and clear-auth handlers of routes/workspace-runtime-mcp.ts is exercised by no test. A refactor dropping assertOpen from (say) the reload handler ships green: a reload that finishes after the runtime generation was replaced — a policy change closing the guard mid-request, exactly as this test models for restart — returns 200 with the superseded generation's result instead of the retryable 503, and the client commits MCP state from a runtime that is no longer the active generation.
Witness:
Scratch-tree probe: mutant removing `runtime.generationGuard?.assertOpen();` from the reload handler
workspace-qualified-rest.test.ts -> Test Files 1 passed, Tests 45 passed (45) (baseline also 45/45)
Parametrize 'rejects an MCP mutation completed by a closed generation' over the remaining runtime mutation routes (POST /runtime/mcp/reload, /runtime/mcp/:server/approve, /runtime/mcp/:server/authenticate, /runtime/mcp/:server/clear-auth), closing the guard inside each mocked mutation function (bridge.reloadWorkspaceMcp / bridge.manageMcpServer). Each added case must go red (503 → 200) when the assertOpen line is removed from the corresponding handler; the existing restart case pins the expected 503/code shape they should share.
中文说明
R1-5 修复新增的「完成后 generation 关闭 → 拒绝」行为（503 workspace_runtime_unavailable）只对 /runtime/mcp/:server/restart 有测试；routes/workspace-runtime-mcp.ts 中同族的 reload、approve、authenticate、clear-auth 处理器里相同的 runtime.generationGuard?.assertOpen() 守卫没有任何测试覆盖。若删除（例如）reload 处理器中的 assertOpen，测试仍全绿：在 runtime generation 被替换后才完成的 reload（策略变更在请求中途关闭 guard，正如本测试为 restart 建模的场景）会带着已被取代 generation 的结果返回 200 而非可重试的 503，客户端会提交来自非活跃 generation 的 MCP 状态。（修复建议与见证见英文部分。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Verified the current production path and found no present behavior defect. This item requires broader coverage or a cross-path refactor rather than a small correctness fix, so I am intentionally leaving it unresolved to avoid expanding this already mature PR; it is suitable for focused follow-up work.
| h.workspaceRegistry.beginReplacement( | ||
| h.workspaceRegistry.primaryEntry, | ||
| 'policy-2', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R2-11: The user-scope reconcile fan-out's trust filter — registry.listManaged().filter((runtime) => runtime.trusted) in affectedRuntimes (workspace-mcp-config.ts:87) — is pinned by no test. 'reconciles user MCP configuration across managed workspaces' uses two trusted runtimes, and the untrusted-primary test closes that primary's generation guard via beginReplacement, which masks the mutant: the queued reload dies at assertAcceptingWork on the closed guard before ever reaching reloadWorkspaceMcp. Deleting the filter keeps the whole suite green, yet a user-scope MCP config write would then schedule reconcileMcpConfiguration() on live untrusted managed runtimes (workspace-trust-reconciler.ts:150 builds trusted:false runtimes that remain in listManaged()), calling reloadWorkspaceMcp so an ungranted workspace applies and starts configured MCP servers.
Witness:
Scratch-tree probe: mutant deleting `.filter((runtime) => runtime.trusted)`
workspace-qualified-rest.test.ts -> Test Files 1 passed, Tests 45 passed (45) <- same as baseline
Add a user-scope write case with a live untrusted runtime whose generation guard is still open (e.g. makeHarness({ secondaryTrusted: false }) without beginReplacement) and assert that runtime's bridge.reloadWorkspaceMcp is not called while trusted runtimes still get settings_changed/reconcile. The untrusted runtime used for the assertion must have an open guard: queued MCP work calls assertAcceptingWork before the body (workspace-runtime-coordinator.ts:410), and a closed guard — as beginReplacement produces (workspace-registry.ts:419, entry.current.guard.close()) — rejects the reload regardless of the filter, which is exactly what masks the mutant today. The new assertion must go red when the filter is removed from affectedRuntimes.
中文说明
user-scope reconcile 扇出的信任过滤——affectedRuntimes 中的 registry.listManaged().filter((runtime) => runtime.trusted)（workspace-mcp-config.ts:87）——没有任何测试钉住。'reconciles user MCP configuration across managed workspaces' 使用两个受信任 runtime，而 untrusted-primary 测试通过 beginReplacement 关闭了主 runtime 的 generation guard，恰好掩盖了突变体：排队的 reload 会在关闭的 guard 上经 assertAcceptingWork 被拒绝，根本到不了 reloadWorkspaceMcp。删除该过滤整个套件仍绿，但 user-scope MCP 配置写入将会对存活的、未受信任的受管 runtime 调度 reconcileMcpConfiguration()（workspace-trust-reconciler.ts:150 会构建仍留在 listManaged() 中的 trusted:false runtime），调用 reloadWorkspaceMcp，使未授权的 workspace 应用并启动已配置的 MCP 服务器。（修复建议、约束与见证见英文部分。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Verified the current production path and found no present behavior defect. This item requires broader coverage or a cross-path refactor rather than a small correctness fix, so I am intentionally leaving it unresolved to avoid expanding this already mature PR; it is suitable for focused follow-up work.
…e-runtime-mcp # Conflicts: # packages/sdk-typescript/test/unit/DaemonClient.test.ts
Local verification report — head
|
| finding | status at 3b936af |
|---|---|
R1-1 unstable hook identity loops AgentCreatePage |
not reproducible. Opening Agents → Create issued 6 daemon requests in 20 s; its MCP tab then issued 0 in the following 25 s. useDaemonMcp now returns a useMemo'd api object. |
| R1-5 mutation routes never re-assert the generation guard | fixed. runtime.generationGuard?.assertOpen() after every await (3 sites); removing all three fails a test. |
| R1-6 (fix-induced) reserved JS property names reach the handlers | fixed. __proto__ / constructor / prototype → 400 invalid_server_name; dotted names like foo-bar.io are admitted and reach the handler. |
R2-1 generic 500 instead of 501 workspace_runtime_not_supported |
fixed in code (requireRuntimeCoordinator → 501). Not reachable at runtime here — every bridge in a live daemon implements the lifecycle snapshot — so this one is code-read + unit pins only. |
| R2-2 preheat failures escape as an uncoded 500 | fixed, verified live. Cold ws-b with its cwd made unavailable → POST /workspaces/ws-b/runtime/mcp/reload returns 503 + Retry-After: 5 + {"code":"runtime_initialization_failed"}. |
Validation matrix on the new routes is clean: unknown workspace 400 workspace_mismatch, forceReconnectAll:"yes" 400, forceReconnectAll+forceReconnectWhich 400 conflicting_force_reconnect_options, entryIndex=-1 400. Legacy surfaces (GET /workspace/mcp, GET /workspace/tools, POST /workspace/mcp/initialize, POST /workspace/runtime/mcp/reload) all still answer.
Mutation matrix — 8/11 killed, and the 3 survivors are all the same guard
Baseline at head: cli 1087/1087, web-shell 688/688.
| # | mutation | result |
|---|---|---|
| M1 | drop the setTools() refresh after reinitializeMcpServers |
killed — acpAgent.test.ts › T2.8 › initializes discovery without reloading sessions… |
| M2 | drop all three post-await generationGuard.assertOpen() |
killed |
| M3 | drop the reserved-JS-name rejection | killed — workspace-qualified-rest.test.ts |
| M4 | drop current.runtimeEpoch !== epoch from the poll-loop stale guard (coordinator ~L349) |
survived |
| M5 | drop the re-queued prepare after a rejected mutation | killed — rechecks MCP readiness after a rejected runtime mutation |
| M6 | drop mcpReconcileDeferred = true on the drain-interrupted path |
killed — replays MCP reconciliation interrupted while draining |
| M7 | render the detail-page workspace control enabled | killed — shows the multi-workspace selector and locks it in MCP detail |
| M8 | drop the requiresAuth branch from statusLabel |
killed — prioritizes MCP approval and authentication labels over connection labels |
| M9 | drop current.runtimeEpoch !== epoch from the post-loop guard (~L392) |
survived |
| M10 | drop this.mcpStatus.runtimeEpoch !== snapshot.runtimeEpoch from status() (~L120) |
survived |
The one thing I'd like fixed before merge (Suggestion, not a blocker): the three survivors are exactly the runtime-epoch half of the guard the PR headlines — "stale responses from a replaced ACP child cannot overwrite the current state". Every existing epoch test either holds the epoch constant while flipping runtimeLive (marks MCP stale when its runtime stops **without changing epoch**) or varies the reported MCP status epoch, which is a different, well-covered guard.
M10 is load-bearing, not an equivalent mutant — I built it and drove it against the real daemon:
HEAD M10 mutant
1) ensure live=T e1 mcp=ready@e1 mcp=ready@e1
2) kill ACP child live=F e1 mcp=stale@e1 mcp=stale@e1
3) POST /session (new runtime, no MCP discovery)
4) status live=T e2 mcp=stale@e1 <-- ok mcp=ready@e1 <-- false ready
A runtime that never ran MCP discovery is reported ready, and nothing in 1087 tests notices. One test per guard — snapshot live at a bumped epoch, assert stale — would close it.
Pre-existing, out of scope, but adjacent
POST /workspace/mcp/__proto__/clear-auth on the legacy route returns 200 {"ok":true,"changed":true} for a server that does not exist — the prototype-chain bypass R1-6 described. workspace-mcp-control.ts is untouched by this PR and the new routes reject it correctly, so this is not a regression; it's a one-line follow-up now that validateRuntimeServerName exists.
Not covered here
- Real third-party OAuth callback, and therefore the "one process-wide OAuth flow at a time" admission (item 4, second half) — the release paths are only unit-pinned.
- R2-1's 501 branch (not reachable against a live daemon).
- Windows / Linux.
Verdict
Every claim in the Reviewer Test Plan reproduces on a real stack, all five standing Criticals are either fixed or not reproducible at this head, and the UI change is a clean improvement over the merge-base bundle. From my side this is good to merge; the epoch-guard test gap is worth a follow-up commit here or a follow-up PR, your call.
Evidence
E1 — same rig, same daemon state, two bundles. The merge-base bundle has no workspace selector and labels the requiresAuth server "Disconnected"; the PR-head bundle adds the selector and reads "Needs authentication". Bundle hashes recorded per arm (index-C6Hdj8xg.js vs index-VCf3Oqfj.js) so the comparison can't be a stale-cache artifact.
E2 — workspace-scoped configuration and live status.
E3 — selector, and the detail page where the workspace is visible but locked.
E4 — cold start: servers loaded, both workspaces still at zero sessions.
中文说明
本地验证报告 —— head 3b936af2d7
PR 描述里「证据（前后对比）」写的是 N/A，所以我把缺的那一半补上了：用本分支源码构建的真实 qwen serve daemon + 真实 Chrome，并对新接口做了链路级探针。以下结论全部在 macOS 上于 3b936af2d7 复现。
验证台：npm run build 后用 node packages/cli/dist/index.js serve --port 4171 --hostname 127.0.0.1 --workspace <ws-a> --workspace <ws-b> 启动（启动横幅确认 Web Shell 来自 packages/web-shell/dist）；隔离 HOME，trustedFolders.json 信任两个 workspace；MCP 服务器为自写 stdio echo server（user-echo 用户级、ws-a-only/ws-b-only workspace 级），oauth-demo（httpUrl + oauth.enabled）是从 UI 里加的；模型是本地假 OpenAI 服务，会把每次请求的 tools[] 原样落盘，所以「这个会话到底带了哪些工具」是抓出来的而不是推出来的；设 tools.toolSearch.enabled=false 让 MCP 工具前置声明。所有结论都与 daemon 自己的请求日志交叉核对。
Reviewer 测试计划 —— 五项全部复现
1. 冷启动不产生隐藏 session —— 成立。 两个 workspace 的 sessions 都是 [] 时打开「插件 → MCP」，链路上只有 POST /workspaces/ws-a/runtime/ensure（2218ms）+ runtime/status + runtime/mcp，整个窗口没有任何 POST /session，事后两个 workspace 仍是 {"sessions":[]}。2.2 秒就是「风险与范围」里说的冷启动开销，真实存在，且只付一次。
2. 双 workspace 隔离 + 详情页只读 —— 成立。 选择器列出两个受信任 workspace；切到 ws-b 会自己发 runtime/ensure（1201ms），完全不碰 /workspace/... 这类绑定 primary 的路由。两个 runtime 的 epoch 各自独立（ws-a=2、ws-b=1）；在 ws-b 批准 ws-b-only 后，ws-a 的 ws-a-only 仍是 approval=pending。详情页里 workspace 控件仍显示（值为 ws-b）但 disabled === true（读 DOM 确认，不只是看着灰）。
3. 配置 CRUD + 运行中 session 的工具热刷新 —— 成立，且这是最强的一条。 作用域路由精确：PUT /workspaces/ws-a/config/mcp/servers/oauth-demo 只改了 ws-a/.qwen/settings.json；DELETE /workspaces/ws-b/config/mcp/servers/cold-write 只改了 ws-b/.qwen/settings.json；用户级文件两次都没被重写。
「已运行 session 获得更新后的 MCP tools」这条，我盯的是模型真正收到的 tools[]，而且全程同一个 session（daemon 日志里只有一条 session spawned，两轮 prompt 的 sessionId 相同）：第 1 轮 35 个工具（user-echo ×2 + ws-a-only ×2）→ POST …/config/mcp/user-echo/disable（返回 activation: reconciling）→ 同一 session 第 2 轮 33 个工具，user-echo 的两个消失。反方向也成立：会话中途批准 ws-a-only，下一轮从 33 涨到 35。冷 runtime 下写配置是持久且延后的：PUT …/ws-b/config/mcp/servers/cold-write 返回 {"activation":"deferred"}，落盘成功，runtime 仍是 cold / runtimeLive=false，等 ws-b 起来后自动收敛。
4. 审批/认证状态优先于连接状态 —— 成立。 同一列表里两个 mcpStatus=disconnected 的服务器分别显示 Needs approval（approvalState=pending）和 Needs authentication（requiresAuth=true），而不是「已断开」。对比见下方 E1。
5. runtime 替换 / epoch 守卫 —— 用真实 kill -9 ACP 子进程验证成立。 200ms 间隔轮询 runtime/status：ready@e4 →（杀子进程）stale@e4 →（ensure）starting@e5 → ready@e5。旧 epoch 上永远不会报 ready，新 runtime 必须走完 live discovery（经过 starting）才 ready。
现存 CHANGES_REQUESTED 条目在本 head 的复核
- R1-1（hook 方法引用不稳定导致
AgentCreatePage循环）：复现不出来。打开「Agents → Create」20 秒内共 6 个请求，再进它的 MCP 页后续 25 秒 0 个请求；useDaemonMcp现在返回useMemo过的 api 对象。 - R1-5（mutation 路由 await 后不复检 generation guard）：已修，三处 await 后都有
runtime.generationGuard?.assertOpen()，三处一起删会挂测试。 - R1-6（修复引入）（保留 JS 属性名进到 handler）：已修，
__proto__/constructor/prototype一律400 invalid_server_name，而foo-bar.io这类带点的名字正常放行进 handler。 - R2-1（该 501 的地方给了泛化 500）：代码层面已修（
requireRuntimeCoordinator返回 501）。活体 daemon 里所有 bridge 都实现了生命周期快照，这条走不到，只能靠读码 + 单测钉住。 - R2-2（preheat 失败逃逸成无 code 的 500）：已修，且活体验证。把冷状态 ws-b 的 cwd 弄没，
POST /workspaces/ws-b/runtime/mcp/reload返回503+Retry-After: 5+{"code":"runtime_initialization_failed"}。
新路由的参数校验矩阵干净：未知 workspace 400 workspace_mismatch、forceReconnectAll:"yes" 400、两个 reconnect 选项同时给 400 conflicting_force_reconnect_options、entryIndex=-1 400。旧接口（GET /workspace/mcp、GET /workspace/tools、POST /workspace/mcp/initialize、POST /workspace/runtime/mcp/reload）全部仍然可用。
变异矩阵 —— 11 个变异体杀掉 8 个，存活的 3 个是同一个守卫
基线：cli 1087/1087，web-shell 688/688。杀掉的：M1（删掉 reinitializeMcpServers 后的 setTools() 刷新）、M2（删掉三处 await 后的 assertOpen()）、M3（删掉保留属性名拒绝）、M5（删掉 mutation 被拒后的重排 prepare）、M6（删掉 drain 中断路径的 mcpReconcileDeferred = true）、M7（详情页 workspace 控件改成可用）、M8（删掉 statusLabel 的 requiresAuth 分支）。
存活的 3 个（M4/M9/M10）全部是 runtime-epoch 守卫：poll 循环内（~L349）、循环后（~L392）、以及 status() 投影（~L120）各删掉 runtimeEpoch 比较，1087 个测试全绿。这正是 PR 主打的那句「被替换 ACP 子进程的迟到响应无法覆盖当前状态」。现有的 epoch 测试要么保持 epoch 不变只翻 runtimeLive（用例名就叫 marks MCP stale when its runtime stops **without changing epoch**），要么变的是 上报的 MCP status 的 epoch —— 那是另一个守卫，且已被覆盖。
M10 不是等价变异体，我把它打包成真 daemon 跑过：ensure 得到 ready@e1 → 杀 ACP 子进程得到 stale@e1 → POST /session 起一个没有做过任何 MCP discovery 的新 runtime → 此时 HEAD 报 live=T e2, mcp=stale@e1（正确），M10 报 live=T e2, mcp=ready@e1（假 ready）。每个守卫补一条「快照 live 但 epoch 已推进 → 断言 stale」的用例即可闭合。这是 Suggestion，不是阻塞项。
既有问题（不在本 PR 范围，但相邻）
旧路由 POST /workspace/mcp/__proto__/clear-auth 对一个根本不存在的服务器返回 200 {"ok":true,"changed":true} —— 就是 R1-6 描述的原型链绕过。workspace-mcp-control.ts 本 PR 没动，新路由也拒绝得很正确，所以这不是回归；但既然 validateRuntimeServerName 已经有了，这是个一行的后续。
未覆盖
真实第三方 OAuth 回调，以及随之而来的「进程级 OAuth 同一时间只允许一个」（测试计划第 4 项的后半，释放路径只有单测钉住）；R2-1 的 501 分支（活体 daemon 走不到）；Windows / Linux。
结论
Reviewer 测试计划的五项在真实栈上全部复现，五条现存 Critical 在本 head 上要么已修要么复现不出来，UI 相对 merge-base 是干净的改进。我这边认为可以合入；epoch 守卫的测试缺口值得补一个 commit 或另开 PR，由你定。
Verified locally by the maintainer against a real daemon + real browser at head 3b936af2d7. Evidence images: assets-pr10679.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
12 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- DELETE no-op config mutations churn — already reported as R1-8 (comment 3905016888)
- User-scope reconcile fan-out unpinned — already reported as R2-11 (comment 3908958546)
- Vacuous label-priority assertions — already reported as R1-16 (comment 3905016952)
- Enable activation aggregation masks 'deferred' — already reported as R1-10 (comment 3905016995)
- Multi-scope enable commits without rollback — already reported as R1-9 (comment 3905016986)
- Poll loop has no unmount guard — already reported as R1-13 (comment 3905017012)
- OAuth error-path release unwitnessed — already reported as R1-18 (comment 3905016869)
- prepareMcpRevision not_started/errors branches unwitnessed — already recorded in the round-2 deferred list (workspace-runtime-coordinator.ts:343, review 5083664753)
- Epoch-mismatch stale projection unwitnessed — already recorded in the round-2 deferred list (workspace-runtime-coordinator.test.ts:121, review 5083664753)
- Dead-string 'first startup' negative assertion — already reported as R1-21 (comment 3905016968)
- enable/disable branch lacks the observeDiscovery .catch — already reported as R1-14 (comment 3905017000)
- Epoch stamp applied at response arrival — already discussed in round-1 review 5074892799 finding 3 and @chiga0's cross-check R3
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped at the 3-round huge-diff cap without two consecutive dry rounds; round 3 re-derived mostly already-reported items.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/routes/workspace-mcp-config.ts:156 — [probe] config routes reject dotted server names the runtime routes acceptpackages/acp-bridge/src/bridge.ts:6062 — [probe] same-epoch merge branch of mergeManagedWorkspaceMcpStatus unpinnedpackages/web-shell/client/components/mcp/McpManagerPage.tsx:741 — [review] add/edit/remove dialog flows have zero test coveragepackages/web-shell/client/daemon/workspace/actions.ts:401 — [review] enable/disable no longer bumps mcpVersion (concurrent windows go stale)packages/cli/src/serve/workspace-runtime-coordinator.ts:267 — [probe] deferred-flag asymmetry on repair-prepare and ensure drain-rejection pathspackages/cli/src/serve/routes/workspace-mcp-config.ts:355 — [probe] disable appends exact name ignoring covering glob patternpackages/cli/src/serve/workspace-qualified-rest.test.ts:1362 — [probe] persistSetting scope asserted as expect.any(String)packages/web-shell/client/components/mcp/McpManagerPage.tsx:633 — [review] stale tools/resources cache across runtime epoch bumpspackages/web-shell/client/components/mcp/McpManagerPage.tsx:623 — [review] mcp.discovery.timeout reused as generic sentinelpackages/cli/src/serve/workspace-qualified-rest.test.ts:1385 — [probe] unchanged-enable test omits the persist-not-called pinpackages/cli/src/serve/workspace-runtime-coordinator.ts:104 — [probe] background MCP prep holds the removal busy gate up to 2 minutespackages/web-shell/client/daemon/workspace/hooks/useDaemonMcp.ts:70 — [probe] named action-timeout messages lost on the workspaceClient branch
Convergence: round 3 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 15 (15 new). Findings keep coming back to the same files: packages/cli/src/serve/workspace-runtime-coordinator.test.ts (findings in round 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 12 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — stopped at the 3-round huge-diff cap without two consecutive dry rounds; round 3 re-derived mostly already-reported items。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 12 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 15 条（其中 15 条首次提出）。发现反复回到同一批文件：packages/cli/src/serve/workspace-runtime-coordinator.test.ts（第 2 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.3)
| await Promise.resolve(); | ||
| await Promise.resolve(); | ||
| expect(mutationStarted).toHaveBeenCalledOnce(); |
There was a problem hiding this comment.
[Suggestion] R3-1: The drain-race repair test ('repairs a queued MCP mutation rejected by a drain race') synchronizes by counting raw microtask yields instead of polling for the intended state, coupling the test to the implementation's exact await schedule. The mutation body starts precisely two microtask hops after runMcpRuntimeMutation (the .catch reaction on the resolved physical tail, then the .then body), and the post-cancelDrain replay sets ready on the last microtask before the test's fourth bare yield — so any behavior-preserving change that adds a single await hop in queueMcpWork, runMcpRuntimeMutation, or prepareMcpRevision makes this test fail spuriously: a red CI run and debugging effort over an innocent refactor. The final commit 3b936af introduced this shape by replacing the prior vi.waitFor synchronization.
Witness:
Mutation probe (scratch tree): adding one behavior-preserving hop (await Promise.resolve(); as the first line of queueMcpWork's .then body) fails the test at line 459 — AssertionError: expected "spy" to be called once, but got 0 times. Pristine baseline for the same file: Tests 29 passed (29).
Replace the bare yields with the polling pattern the neighboring tests already use, at both synchronization sites:
await vi.waitFor(() => expect(mutationStarted).toHaveBeenCalledOnce());
// ... beginDrain / releaseMutation / cancelDrain ...
await vi.waitFor(() => {
expect(harness.reloadWorkspaceMcp).toHaveBeenCalledOnce();
expect(coordinator.status().capabilities?.mcp?.state).toBe('ready');
});The rewrite must keep discriminating the drain-race repair itself: queued mutation rejected with WorkspaceDrainingError, capabilities.mcp 'stale', then cancelDrain() replays — reloadWorkspaceMcp called once and state 'ready' (packages/cli/src/serve/workspace-runtime-coordinator.test.ts:443-481). The rewritten test must stay green when a behavior-preserving await Promise.resolve() hop is added inside queueMcpWork's chain, which the current bare-yield version fails — please confirm the new shape passes that mutation.
中文说明
drain 竞态修复测试（'repairs a queued MCP mutation rejected by a drain race'）通过计数裸微任务让出来同步，而不是轮询目标状态，使测试与实现的精确 await 调度耦合。mutation body 恰好在 runMcpRuntimeMutation 之后两个微任务跳开始（已解析物理尾上的 .catch 反应，然后是 .then body），cancelDrain 后的重放在测试第四次裸让出的前一个微任务上置位 ready——因此，任何在 queueMcpWork、runMcpRuntimeMutation 或 prepareMcpRevision 中增加单个 await 跳的行为保持型改动都会让该测试虚假失败：一次无辜的重构带来红色 CI 与排查成本。最终提交 3b936af 用这种形状替换了原先的 vi.waitFor 同步。（Witness 证据见英文部分。）建议将两处同步点的裸让出替换为相邻测试已在使用的 vi.waitFor 轮询。改写必须继续区分 drain 竞态修复本身：排队的 mutation 以 WorkspaceDrainingError 被拒绝、capabilities.mcp 为 'stale'，随后 cancelDrain() 重放——reloadWorkspaceMcp 被调用一次且状态回到 'ready'（packages/cli/src/serve/workspace-runtime-coordinator.test.ts:443-481）。改写后的测试在 queueMcpWork 链中加入行为保持的 await 跳时必须保持绿色（当前裸让出版本在同一变异下会失败）——请确认新形状能通过该变异。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| await actions.manageMcpServer('docs', 'disable'); | ||
|
|
||
| expect(setUserMcpServerEnabled).toHaveBeenCalledWith('docs', false); |
There was a problem hiding this comment.
[Suggestion] R2-5: (fix-induced) The disable-scope regression test added for R2-5 ('disables a user MCP server in user scope') pins only the user-only cell of the scope-selection matrix; the workspace-only and both-scopes cells remain unpinned in actions.test.ts, useDaemonMcp.test.tsx (which pins only the user-only 'constructor' prototype case), and App.test.tsx (manageServer mocked). Dropping the !Object.hasOwn(config!.workspace, serverName) conjunct from the disable condition in actions.ts and useDaemonMcp.ts keeps the whole suite green, and disabling a workspace-defined server — or one defined in both scopes — then routes the exclusion to USER scope via setUserMcpServerEnabled, whose publishConfiguration broadcasts to every managed runtime: the server is disabled in all workspaces instead of only the selected one, contradicting the design doc's 'Disabling project or workspace servers writes the exclusion to workspace-local settings'.
Witness:
Scratch-tree mutant: dropping the !Object.hasOwn(config!.workspace, ...) conjunct in both files keeps the existing suite green — Test Files 2 passed (2), Tests 24 passed (24). Both-scopes probe test (mcpConfig() resolving {user: {docs}, workspace: {docs}}, disable) FAILS under the mutant — setMcpServerEnabled (workspace) called 0 times — and passes on intact code: Tests 25 passed (25).
Add disable tests for the remaining matrix cells beside the current one: one with mcpConfig() resolving { user: {}, workspace: { docs: { command: 'docs' } } } asserting setMcpServerEnabled('docs', false) is called and setUserMcpServerEnabled is not, and one with both scopes defined asserting workspace scope wins.
The enable-side ordering is already pinned via invocationCallOrder by actions.test.ts 'enables an MCP server in every disabled scope' (packages/web-shell/client/daemon/workspace/actions.test.ts:400-435) — the new tests must not disturb that user-before-workspace expectation. The new workspace-only and both-scopes disable tests must go red when the !Object.hasOwn(config!.workspace, serverName) conjunct is dropped from the disable condition — please remove the conjunct and run the new tests to confirm.
中文说明
（fix-induced）为 R2-5 新增的 disable 作用域回归测试（'disables a user MCP server in user scope'）只钉住了作用域选择矩阵中「仅 user」一格；actions.test.ts、useDaemonMcp.test.tsx（只钉住 user-only 的 'constructor' 原型用例）与 App.test.tsx（manageServer 被 mock）中「仅 workspace」与「双作用域」两格均无测试。若删除 actions.ts 与 useDaemonMcp.ts 中 disable 条件里的 !Object.hasOwn(config!.workspace, serverName) 合取项，整个测试套件仍为绿色；此时禁用一个在 workspace 作用域定义（或在两个作用域都定义）的服务器会把排除项写入 USER 作用域（setUserMcpServerEnabled），其 publishConfiguration 会广播到所有受管 runtime——服务器将在所有 workspace 被禁用，而不是只在所选 workspace，与设计文档「禁用项目或 workspace 服务器时将排除项写入 workspace 本地设置」相悖。（Witness 证据见英文部分。）建议在当前用例旁补齐其余矩阵格的 disable 测试：mcpConfig() 解析为 { user: {}, workspace: { docs: { command: 'docs' } } } 时断言调用 setMcpServerEnabled('docs', false) 且未调用 setUserMcpServerEnabled；再补一个双作用域定义时断言选择 workspace 作用域的用例。enable 侧顺序已由 actions.test.ts 'enables an MCP server in every disabled scope' 通过 invocationCallOrder 钉住（packages/web-shell/client/daemon/workspace/actions.test.ts:400-435），新测试不得破坏该 user-before-workspace 预期。删除上述合取项时新增的两个用例必须变红——请移除合取项并运行新测试确认。
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
@qwen-code /triage |
|
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: 64 passed · 0 failed · 64 total Flakiness gate: 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：64 通过 · 0 失败 · 64 总计 抖动门： Verification reportPR 10679 Deep Verification — feat(serve): add workspace-scoped MCP managementVerdict: 中文摘要结论：
Scope selectionCentral claim (C1): MCP readiness is projected through runtime capability 1. A/B load-bearing proof of the central claimAn identical self-contained probe (
The base coordinator returns status with no Realpath hygiene for the base arm: the base worktree resolves 2. Wire-oracle harness (mock-free REST drive of the head build)
Key cells:
Harness environment notes (harness-side, not PR defects): two early red runs 3. Mutation matrix (vacuity + load-bearing proof of the guards)Driver:
Observation (not a defect — layered defense): the epoch-change scenario is Harness-capability control: every mutant above was caught by its own suite, 4. Targeted gates (exact counts)
5. Reviewer Test Plan walkthrough (per step)
6. FindingsNone blocking. No executed assertion failed; no defect survived the 7. Not covered
8. MethodologyEnvironment: CI Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Re-review of head 3b936af2d75be78762578f5ab0f83d8ef3b3a352 (new commits since prior reviews at e11f8125 / bot head).
Critical findings from prior reviews — status at current head
chiga0 R1 (floating prepareMcp() promise) — void preparation.catch(…) is present in the diff; the rejection is handled regardless of whether remainingMs <= 0. Closed.
qwen-code-ci-bot Critical R1-1 through R1-7 — all closed at current head:
- R1-1 (unstable hook identity):
useDaemonMcpwrapsapiinuseMemo. Closed. - R1-2 (cold runtime not started):
await mcp.ensureRuntime()is called before the poll loop. Closed. - R1-3 (drain-interrupted stale):
prepareMcpRevisionsetsmcpReconcileDeferred = trueinside its drain-exit branch; test added. Closed. - R1-4 (mutation silently drops queued reconciliation): The serial
mcpPhysicalTailqueue serializes all MCP work;configRevisionguard is independent ofmcpRevision, soreloadWorkspaceMcp()fires regardless of intervening mutations; the last queued item polls post-reload state. Closed. - R1-5 (generation guard not re-asserted):
runtime.generationGuard?.assertOpen()follows every mutation await; test added. Closed. - R1-6 (dotted server names rejected):
validateRuntimeServerNameuses no charset restriction;foo-bar.iotest passes at 200. Closed. - R1-7 (
inoperator on config maps):Object.hasOwnused throughout. Closed.
Remaining non-blocking suggestions (inherited from bot review, still open)
Minor (R1-9) — multi-scope enable commits user scope before workspace-scope check. See inline comment on actions.ts.
Minor (R1-12) — observation budget (81 x 1.5 s ≈ 121 s) can be exhausted before a queued restart completes (MCP_RESTART_TIMEOUT_MS = 300 s). See inline comment on McpStatusMessage.
Coverage
Read: coordinator, bridge, config routes, runtime-MCP routes, SDK types, actions, useDaemonMcp, McpStatusMessage, error-response, run-qwen-serve admission. Not read to full depth: McpManagerPage complete body, App.test.tsx fixture types, pre-existing requestWorkspaceStatus body.
No local execution.
Reviewed with AI assistance.
| action === 'disable' ? await workspace.mcpConfig() : undefined; | ||
| const scopes = | ||
| action === 'enable' | ||
| ? (['user', 'workspace'] as const) |
There was a problem hiding this comment.
Minor (R1-9 inherited) — enable iterates ["user", "workspace"] in that order. If the user-scope call succeeds but the workspace-scope call returns 409 mcp_excluded_by_pattern, the user-scope exclusion-removal is already committed with no rollback. The server remains visibly blocked (the wildcard still wins), so this does not cause an observable enable, but the user-scope exclusion list becomes inconsistent. Low-frequency; worth a comment noting partial-failure semantics or a pre-flight pattern check before writing user scope.
| let nextStatus: DaemonWorkspaceMcpStatus | undefined; | ||
| let runtimeEpoch: number | undefined; | ||
| await mcp.ensureRuntime(); | ||
| for (let attempt = 0; attempt < 81; attempt += 1) { |
There was a problem hiding this comment.
Minor (R1-12 inherited) — 81 x 1.5 s ≈ 121 s observation budget. The coordinator serial queue can be held by a restart/approve/authenticate mutation for up to MCP_RESTART_TIMEOUT_MS (300 s) + MCP_PREPARE_TIMEOUT_MS (120 s). Two concurrent operations can exhaust this budget and surface a spurious timeout. Not blocking — the retry pattern is in place — but if this budget is ever revisited it should account for the full queued tail.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Re-review of head 3b936af2d75be78762578f5ab0f83d8ef3b3a352 (round 2 — prior review was at e11f8125). Findings frozen before reading the updated cross-check below.
Prior round finding status
| Id | Summary | Status at current head |
|---|---|---|
| R1-1 | Floating prepareMcp() promise — daemon crash |
✅ Fixed — void preparation.catch(...) at coordinator.ts:163 |
| R2 | TypeScript union in manageServer enable/disable | Checked — route goes through mcp.manageServer with correct dispatch. Not a blocker. |
| R3–R5 | Epoch at response / cache unstamped / activation drop | Addressed — runtimeEpoch stamping in bridge + mcp.ensureRuntime() in reloadSelectedServer |
New finding
F2-1 (Major, supported) — useDaemonMcp auto-reload is keyed to the primary workspace's mcpVersion signal regardless of the workspaceCwd option.
useDaemonWorkspaceEventSignals() returns workspace-event signals for the active session's (primary) workspace. When McpManagerPage selects a secondary workspace and calls useDaemonMcp({ workspaceCwd: secondaryCwd }), the auto-reload hook (useWorkspaceEventReload) still fires on primary-workspace MCP events — not the requested workspace's events. Result: MCP status for a secondary workspace will not auto-refresh when that workspace's MCP changes; it may reload spuriously when the primary changes.
Named chain: useDaemonMcp.ts:36 → useWorkspaceEventReload(signals?.mcpVersion, …) where signals = useDaemonWorkspaceEventSignals() (session-scoped, not workspace-scoped by workspaceCwd). Confirmed via the test's mock: useDaemonWorkspaceEventSignals: () => undefined is workspace-agnostic.
This is a direct gap in the multi-workspace MCP management feature this PR introduces. The feature remains operable (initial load is correct; users can manually trigger actions), but live auto-refresh is broken for non-primary workspaces.
Cross-check against qwen-code-ci-bot CHANGES_REQUESTED (SHA e9ffb1cb)
| ci-bot Id | Title | Status at 3b936af2 |
|---|---|---|
| R1-5 | Mutation routes never re-assert generation guard | ✅ Fixed — runtime.generationGuard?.assertOpen() at workspace-runtime-mcp.ts:194, 223, 247 |
| R1-7 | in on config maps lets prototype keys flip scope |
✅ Fixed — Object.hasOwn() in actions.ts |
| R1-2 | Chat-card enable/disable never starts cold runtime | ✅ Fixed — mcp.ensureRuntime() in reloadSelectedServer |
| R1-3 | Drain-interrupted prepare latches stale | ✅ Addressed — mcpReconcileDeferred + cancelDrain() reschedules |
| R1-34 | Cross-workspace auto-reload keyed to host session | ❌ Still present — confirmed as F2-1 |
| R1-30 | McpStatusMessage poll loop no unmount guard | Still present (81 × 1.5 s loop); React 18 tolerates it, no data corruption. Minor. |
Could not rule on: R1-4 (mutation drops queued prepare — configRevision guard limits impact, not run-verified), R1-8/R1-9/R1-18/R1-22 (OAuth admission lane / tools-resources epoch — diff evidence looks correct, not fully verified).
Scope
Static review: coordinator, bridge patch, config routes, runtime-MCP routes, SDK, useDaemonMcp, McpStatusMessage, McpManagerPage. No local toolchain.
Verdict
No confirmed blocking finding. F2-1 is a functional gap in multi-workspace auto-refresh — bounded, non-corrupting, operable. Approving.
Reviewed with AI assistance.
| const result = useDaemonResource(load, resourceOptions); | ||
| const signals = useDaemonWorkspaceEventSignals(); | ||
| useWorkspaceEventReload( | ||
| signals?.mcpVersion, |
There was a problem hiding this comment.
Major — signals?.mcpVersion comes from useDaemonWorkspaceEventSignals(), which is keyed to the active session's (primary) workspace, not to the workspaceCwd option passed into this hook. When McpManagerPage calls useDaemonMcp({ workspaceCwd: secondary }), MCP status for that workspace will not auto-refresh when that workspace's MCP state changes; it reloads spuriously when the primary workspace's MCP changes instead.
Suggestion: drive the auto-reload signal from a per-workspace subscription (e.g. pass workspaceCwd to a workspace-aware signal hook), or fall back to polling when workspaceCwd targets a non-primary workspace.
|
Released in v0.23.0. |
5 participants
What this PR does
Adds workspace-scoped MCP configuration and runtime management on top of the shared workspace runtime. Configuration writes remain durable when a runtime is cold, while reload, reconnect, approval, authentication, tools, and resources are routed only to the selected live workspace runtime.
MCP readiness is projected through runtime capability status with monotonic revisions and runtime epochs, so stale responses from a replaced ACP child cannot overwrite the current state. Configuration changes also refresh MCP tools in already-running sessions without restarting those sessions.
Updates the Web Shell MCP manager to use the new API directly, select among trusted workspaces, keep the workspace visible but read-only on detail pages, and show approval or authentication state before connection state.
Why it's needed
The previous MCP management flow was tied to the primary workspace and mixed durable configuration with live runtime state. That made multi-workspace management ambiguous and allowed management refreshes to race ACP replacement or active sessions. This change makes workspace ownership explicit while preserving the existing MCP endpoints for existing consumers.
Reviewer Test Plan
How to verify
Local verification passed: full build, full typecheck, full lint, coordinator and workspace route tests, the complete ACP bridge test file, the complete daemon SDK client test file, and the Web Shell MCP/App regression set.
Evidence (Before & After)
N/A — no browser recording was captured; the UI and runtime flows are covered by component, route, coordinator, bridge, and SDK tests.
Tested on
Environment (optional)
Node.js 22 local workspace; daemon build and automated tests.
Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
在共享 Workspace Runtime 之上增加 workspace 级 MCP 配置与运行时管理。runtime 处于冷状态时配置写入仍会持久化；reload、重连、审批、认证、Tools 和 Resources 只路由到用户选择的实时 workspace runtime。
MCP 就绪状态通过带单调 revision 和 runtime epoch 的 capability status 对外投影，因此被替换 ACP 子进程的迟到响应无法覆盖当前状态。配置变化也会刷新已运行 session 中的 MCP tools，不需要重启这些 session。
Web Shell MCP 管理页改为直接使用新接口，可在多个受信任 workspace 之间选择；进入服务器详情后仍显示 workspace，但不可切换；列表中待审批或待认证状态优先于连接状态。
为什么需要
之前的 MCP 管理流程绑定 primary workspace，并混合了持久化配置与实时 runtime 状态，导致多 workspace 管理边界不清晰，管理刷新也可能与 ACP 替换或活跃 session 发生竞争。本改动明确了 workspace 所有权，同时保留现有 MCP 接口供原有消费者继续使用。
Reviewer 测试计划
如何验证
本地验证已通过：完整 build、完整 typecheck、完整 lint、Coordinator 与 workspace 路由测试、完整 ACP bridge 测试文件、完整 daemon SDK client 测试文件，以及 Web Shell MCP/App 回归测试集。
证据（前后对比）
N/A —— 未录制浏览器操作；UI 与 runtime 流程由组件、路由、Coordinator、Bridge 和 SDK 测试覆盖。
测试平台
环境（可选）
Node.js 22 本地 workspace；daemon build 与自动化测试。
风险与范围
关联 Issue
N/A