fix(core): refresh MCP session metadata without reconnecting - #8522
Conversation
Verification reportIssue: #8492 Deterministic before/after regressionBefore the fix, the focused reproduction completed with 4 expected failures and 133 passes. The failures independently demonstrated:
After the fix: $ npm test -w @qwen-code/qwen-code-core -- src/tools/session-mcp-view.test.ts src/tools/mcp-client-manager.test.ts src/tools/mcp-transport-pool.test.ts
Test Files 3 passed (3)
Tests 177 passed (177)The permanent coverage now verifies pooled and unpooled refresh, stable unpooled transport identity, transport-changing reconnects, same-fingerprint runtime replacement, legacy reconciliation, absent versus empty allowlists, order/duplicate/argument-suffix normalization, in-place config mutation, released-handle rejection, prompt/resource replay, per-session trust and eager loading, shared-snapshot isolation, and zero churn for equivalent metadata. Real MCP process checksInstalled CLI dry run. Global Qwen Code 0.21.3 ran from an isolated project against the repository's real Source-build lifecycle harness. A real local stdio child was acquired from the built core, refreshed in place, called, checked for equivalent-config churn, and shut down cleanly: {
"status": "pass",
"lifecycleId": "real-mcp::3e85f9c0a94c09ba",
"transportId": "real-mcp::3e85f9c0a94c09ba",
"sameClientAfterRefresh": true,
"sharedSnapshotAlwaysLoad": false,
"projectedTrust": true,
"projectedAlwaysLoad": true,
"toolCallReturnedOk": true,
"equivalentRefreshRegistryChurn": 0
}No fixture child process remained after the harness. Broader validation
Visual evidenceScreenshot: N/A — this is a non-visual core MCP lifecycle change. The observable evidence is the process identity, registry behavior, real tool result, and test output above. 中文验证报告验证报告Issue：#8492 确定性的修复前 / 修复后回归验证修复前，定向复现得到 4 项预期失败、133 项通过。四项失败分别证明：
修复后： $ npm test -w @qwen-code/qwen-code-core -- src/tools/session-mcp-view.test.ts src/tools/mcp-client-manager.test.ts src/tools/mcp-transport-pool.test.ts
Test Files 3 passed (3)
Tests 177 passed (177)永久回归覆盖现已验证：池化与非池化刷新、稳定的非池化传输标识、影响传输的配置变更触发重连、同指纹运行时替换、传统单会话对账、缺失与空允许列表的差异、顺序 / 重复项 / 参数后缀规范化、原地配置对象变更、已释放句柄拒绝刷新、提示词与资源重放、按会话投影信任与预加载、共享快照隔离，以及等价元数据产生零注册抖动。 真实 MCP 进程检查已安装 CLI 的干跑验证。 全局 Qwen Code 0.21.3 在隔离项目中使用仓库真实的 源码构建生命周期 harness。 从已构建 core 获取真实本地 stdio 子进程，原地刷新配置，完成真实调用，检查等价配置抖动，并干净关闭： {
"status": "pass",
"lifecycleId": "real-mcp::3e85f9c0a94c09ba",
"transportId": "real-mcp::3e85f9c0a94c09ba",
"sameClientAfterRefresh": true,
"sharedSnapshotAlwaysLoad": false,
"projectedTrust": true,
"projectedAlwaysLoad": true,
"toolCallReturnedOk": true,
"equivalentRefreshRegistryChurn": 0
}harness 完成后没有遗留 fixture 子进程。 更广泛的验证
可视化证据截图：不适用——这是非可视化的 core MCP 生命周期变更。可观察证据为上面的进程标识、注册行为、真实工具结果和测试输出。 |
|
Re-run requested by @wenshao after the autofix round and the merge of
Moving on to code review. 🔍 中文说明应 @wenshao 的要求，在 autofix 轮次与合并
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI formed an independent proposal before reading the diff (two identities per handle, one canonical per-session metadata key driving all reconciliation paths, snapshot replay through re-projected views, canonicalized filters so equivalent settings don't churn) — the PR matches it, and it also resolves everything the round-1 review raised, each with a pinning test:
No critical blockers, no convention violations. The Two non-blocking follow-ups from @wenshao's verification, both pre-existing (identical on base) and correctly out of scope here: the legacy discovery filter's substring matching on a malformed string The refresh flow, for navigation: sequenceDiagram
participant P1 as Settings reconcile
participant P2 as McpClientManager
participant P3 as PoolEntry
participant P4 as SessionMcpView
participant P5 as Session registries
P1->>P2: settings changed, reconcile
P2->>P2: desired fingerprint == captured transportId
P2->>P3: updateConfig on the surviving handle
P3->>P4: updateConfig(cfg)
P4-->>P3: metadata key changed
P3->>P4: replay tools, prompts, resources snapshots
P4->>P5: remove and re-register the session projection
Files changed (10 of 10)
Test evidence — the PR's own CI on
|
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) | ✅ success |
| Desktop Shell (ubuntu-22.04) | ✅ success |
| Desktop Shell (windows-2022) | ✅ success |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | ✅ success |
| Classify PR · label · route · Remind on force-push | ✅ success |
| Test (macos/windows) · build-cli · Integration Tests · autofix jobs | ⏭️ skipped (standard for fork PRs) |
One red item in the actions list is not a code signal: the qwen-code-pr-review.yml workflow run on this SHA failed with zero jobs — no job ever ran, so no PR code was executed. It's the review-bot orchestration workflow (its checkout is base-only by design), it failed identically on the previous autofix push 5cabd9d5, and the same workflow ran green on this PR's earlier commits. Classification: pre-existing infra noise, not PR-caused. The earlier red that was real — Test (ubuntu-latest, Node 22.x) on 5cabd9d5 — was resolved by the merge of main and is green on this head.
The behavioral claim is substantiated beyond mocked transports: @wenshao posted an independent A/B verification on this exact commit (base 4ec0371…head 24ff52f, real stdio MCP subprocesses, all three reconciliation paths, real TUI /context surface, malformed-settings totality, 300 targeted tests green) — base reproduces all four failure modes, this PR fixes them without regressing transport-affecting reconnects. That is maintainer-run evidence, attributed as such, not the author's self-report. The author's own matrix covers macOS only; the remaining untested surfaces (Windows/macOS locally, the qwen serve REST surface, HTTP/SSE transports) could be closed by a sponsored @qwen-code /verify run if a maintainer wants bot-run A/B proof in addition — the lane is available for fork PRs with a pre-execution risk screen, and its report should be read with the same skepticism as the fork's own CI logs.
中文说明
代码审查
读 diff 之前我先独立给出了自己的方案（每个句柄两个标识、三条对账路径共用一个规范化的按会话元数据键、经重新投影的视图重放快照、规范化等价过滤器避免抖动）——PR 与之一致，并且把第一轮审查提出的问题全部解决了，每一条都有钉死的测试：
- 畸形过滤器的全函数覆盖（第一轮 Critical）：新的
mcp-session-config.ts对非数组/非字符串形状做强制转换，且session-mcp-view.ts的运行时过滤器与元数据键使用同一套转换——键永远不会承诺一个过滤器随后会抛异常的形状。 - 刷新时的回滚缺口：运行时替换现在先刷新会话视图、再持久化 Config overlay，测试钉死了调用顺序——抛异常的刷新不再可能持久化视图从未收到的配置。
- 故障隔离：批量对账把每个
updateConfig包在按服务器的 try/catch 里；运行时 add/replace 保留严格传播并附有注释说明原因。两种行为均有测试。 - 会话可见的
toolCount：改从toolRegistry.getToolsByServer读取，与独立分支一致。已对照 base 核实toolRegistry是必需的构造依赖，新调用点不会取到 undefined。 - 标识拆分：
transportId在 spawn 时捕获而非事后重算，就地篡改调用方配置无法伪造"新鲜"。grep 了 base 中全部指纹比较点——恰好两处，均已改用transportId。isTerminated()覆盖closed与failed，fail-closed 守卫成立，测试钉死了failed转换。 - 传统路径：
singleSessionConnectedKeyOf复用同一规范化键，元数据编辑在该路径会重连并重新发现，等价过滤器保持无操作——均有测试钉死。
无阻断问题，无规范违规。mcp-pool-key.ts 一处为纯注释改动，且已确认 fingerprint() 的显式白名单从未包含 alwaysLoadTools，无需行为变更。
@wenshao 验证中留下的两条非阻断后续项，均为既有问题（base 上完全一致）、本 PR 范围之外，判断正确：传统发现过滤器对畸形字符串 excludeTools 的子串匹配与池化路径的强制转换不一致；include/exclude 的强制转换方向相反（fail-closed vs fail-open）。值得开一个小 follow-up：在 mcp-client.ts 与 mcp-session-config.ts 之间共享同一套强制转换，并在过滤器不是字符串数组时打一条警告日志。
测试证据
该提交自身的 CI 全绿（见上表）。actions 列表中唯一的红色不构成代码信号：qwen-code-pr-review.yml 在该提交上的运行以零个 job失败——没有任何 job 运行过，也就没有执行任何 PR 代码；它是 review 机器人的编排工作流，前一次 autofix 推送 5cabd9d5 上同样失败，而同一工作流在本 PR 更早的提交上是绿的。判定：既有基础设施噪音，非 PR 引入。之前真实的红色——5cabd9d5 上的 Test (ubuntu-latest, Node 22.x)——已由合并 main 解决，当前 head 为绿。
行为性主张的证据超出了 mock 传输：@wenshao 在本提交上发布了独立 A/B 验证（真实 stdio MCP 子进程、全部三条对账路径、真实 TUI /context 面、畸形设置全函数覆盖、300 项定向测试通过）——base 复现全部四种失败模式，本 PR 修复且不回归"影响传输的变更仍重连"的边界。这是维护者亲自运行的证据，按此归属注明，不是作者的自我报告。作者本人的矩阵仅覆盖 macOS；剩余未测面（Windows/macOS 本地、qwen serve REST 面、HTTP/SSE 传输）如维护者希望补一份机器人运行的 A/B 证明，可由 sponsored @qwen-code /verify 运行关闭——该通道对 fork PR 可用、带运行前风险筛查，其报告应像对待 fork 自身 CI 日志一样保持审慎。
— Qwen Code · qwen3.8-max
Reviewed at 24ff52f0ea53d23d927efdb1482811c03531f8ed · re-run with @qwen-code /triage
|
Confidence: 4/5 — clean, minimal, fully pinned fix for a reproduced bug, independently A/B-verified on this exact commit; the two open items are pre-existing follow-ups, not defects of this PR. Stepping back: this is what a good fix in a high-risk area looks like. The problem was observed by someone other than the author (#8492, deterministic repro), the diff does exactly what the issue asks and nothing else, and the committed design doc states its invariants explicitly. The implementation matches the approach I'd have taken independently — two identities per handle, one canonical metadata key shared by all three reconciliation paths, snapshot replay through re-projected views, fail-closed at every terminal state. What a second pass adds over my first review of this PR: every round-1 finding now has a regression test that would catch it if the code regressed, including the subtle ones (key/filter coercion agreement, refresh-before-overlay ordering, sibling-session isolation). I checked the load-bearing claims against base code rather than taking the diff's word: the two fingerprint comparison sites, What keeps this at 4 rather than 5: the coercion follow-ups wenshao surfaced (the legacy substring-match divergence and the fail-closed-vs-fail-open asymmetry) are real papercuts for users who typo a filter — pre-existing, out of scope here, but worth a tracking issue so they don't evaporate. And platform coverage is macOS-author + Linux-maintainer; the behavioral claim doesn't rest on that gap because of the A/B on this commit, but Windows/macOS runtime behavior is unverified all the same. The behavioral core claim is substantiated: green unit suite on this SHA plus @wenshao's independent real-runtime A/B (base reproduces all four failure modes, this PR fixes them, transport-affecting reconnects still work). My earlier reservation about fault isolation around 中文说明置信度：4/5 —— 对一个有复现的 bug 的干净、最小、处处钉死的修复，且已在本提交上被独立 A/B 验证；剩余两项是既有问题的后续事项，不是本 PR 的缺陷。 退一步看：这是高风险领域一个优秀修复的样子。问题由作者以外的用户观测到（#8492，确定性复现）；diff 精确地做了 issue 要求的事，没有任何顺手改动；附带的设计文档明确列出了不变量。实现与我独立给出的方案一致——每个句柄两个标识、三条对账路径共用一个规范化元数据键、经重新投影的视图重放快照、所有终态 fail closed。相比我第一次审这个 PR，第二轮新增的价值在于：第一轮的每一条发现现在都有能在代码退化时抓住它的回归测试，包括那些微妙的点（键与过滤器的强制转换一致性、先刷新后写 overlay 的顺序、兄弟会话隔离）。承重声明我是对照 base 代码核实过的，而不是只听 diff 的一面之词：两处指纹比较点、 停在 4 而不是 5 的原因：wenshao 指出的强制转换后续项（传统路径的子串匹配分歧、fail-closed 与 fail-open 的不对称）对于打错过滤器参数的用户是真实的痛点——虽是既有问题、本 PR 范围之外，但值得开一个跟踪 issue，免得不了了之。另外平台覆盖是"作者 macOS + 维护者 Linux"；行为性主张并不依赖这个缺口（有本提交上的 A/B），但 Windows/macOS 运行时行为确实未被验证。 行为性核心主张已有支撑：本提交上全绿的单测套件 + @wenshao 的真实运行时独立 A/B（base 复现全部四种失败模式，本 PR 修复之，影响传输的变更仍然重连）。我之前对 — Qwen Code · qwen3.8-max Reviewed at |
doudouOUC
left a comment
doudouOUC
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. Test Plan (not a blocker): src/tools/mcp-client-manager.test.ts — no such file or directory; src/tools/mcp-transport-pool.test.ts — no such file or directory.
中文说明
已审查。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。 Test Plan（非阻断）：src/tools/mcp-client-manager.test.ts — no such file or directory; src/tools/mcp-transport-pool.test.ts — no such file or directory。
— qwen3.7-max via Qwen Code /review (v0.21.4)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
Review follow-up verification reportVerified head: Failure-first evidenceThe new two-server regression failed on the reviewed head: server A's retained-connection Review changes
VerificationThis is non-visual core lifecycle behavior, so a screenshot would not add meaningful evidence. 中文验证报告审查跟进验证报告验证 head： 失败优先证据新增的双 server 回归测试在被审查 head 上失败：server A 的保留连接 审查改动
验证这是不可视的 core 生命周期行为，截图不会增加有效证据。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): src/tools/mcp-client-manager.test.ts — no such file or directory; src/tools/mcp-transport-pool.test.ts — no such file or directory.
中文说明
Test Plan（非阻断）：src/tools/mcp-client-manager.test.ts — no such file or directory; src/tools/mcp-transport-pool.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| includeTools: | ||
| config.includeTools === undefined | ||
| ? null | ||
| : normalizeFilter(config.includeTools, true), |
There was a problem hiding this comment.
[Critical] includeTools: null is keyed as an empty allowlist ("allow none") while every runtime filter treats null as absent ("allow all") — Failure scenario: the branch tests === undefined, so a JSON null falls into normalizeFilter(null, true) → [], producing a key byte-identical to includeTools: [] (probe-verified). A pooled server connected with includeTools: null registers all tools; an edit to includeTools: [] computes an identical key, SessionMcpView.updateConfig returns false, and every tool stays registered and callable despite the explicit allow-none config. The reverse direction ([] → null) leaves the session stuck at zero tools. The legacy single-session path misses the change the same way via singleSessionConnectedKeyOf; nothing upstream coerces shapes (SETTINGS_SCHEMA.mcpServers is a bare object), so null reaches this code from a hand-written settings.json. This violates the file's own invariant: "an absent allowlist accepts every name, while an explicit empty allowlist accepts none".
| includeTools: | |
| config.includeTools === undefined | |
| ? null | |
| : normalizeFilter(config.includeTools, true), | |
| includeTools: | |
| config.includeTools == null | |
| ? null | |
| : normalizeFilter(config.includeTools, true), |
中文说明
includeTools: null 被编码为"不允许任何工具"的空允许列表，而所有运行时过滤器都把 null 视为缺省（允许全部）——失败场景：此分支用 === undefined 判断，JSON 中的 null 会落入 normalizeFilter(null, true) → []，生成的 key 与 includeTools: [] 完全相同（已用探针验证）。以 includeTools: null 连接的池化服务器会注册全部工具；将其改为 includeTools: [] 时 key 不变，SessionMcpView.updateConfig 返回 false，尽管配置已明确"不允许任何工具"，所有工具仍然保持注册且可调用。反向（[] → null）则会让会话一直停留在零工具状态。传统单会话路径经由 singleSessionConnectedKeyOf 同样无法感知此变化；上游不做类型校正（SETTINGS_SCHEMA.mcpServers 只是一个裸 object），手写 settings.json 中的 null 可以直达此处。这违反了本文件自身的不变量："缺省允许列表接受所有名称，显式空允许列表不接受任何名称"。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| entries: readonly string[] | undefined, | ||
| stripParenthesizedSuffix: boolean, | ||
| ): string[] { | ||
| const normalized = (entries ?? []).map((entry) => { |
There was a problem hiding this comment.
[Critical] The new metadata key is non-total over malformed filter shapes that the pre-diff key tolerated — Failure scenario: a non-array filter (e.g. excludeTools: "x", a realistic settings typo — nothing upstream coerces shapes: SETTINGS_SCHEMA.mcpServers is a bare object, and the daemon runtime-add surfaces validate only that config is an object) throws (entries ?? []).map is not a function. At the single-session classification loop (mcp-client-manager.ts:2271) singleSessionConnectedKeyOf runs outside any per-server try/catch — probe-measured: the entire reconciliation pass dies, no server connects or reconnects (a healthy sibling with a legitimate change never reconnected), discoveryState stays IN_PROGRESS, the terminal mcp-client-update never emits, and every subsequent pass re-throws until the config is fixed. Pre-diff the spread-based key tolerated strings. Note: an Array.isArray guard alone is not enough — non-string entries still throw in normalizeMcpIncludeEntry (see the sibling comment); the coercion below covers both shapes in the key path.
| const normalized = (entries ?? []).map((entry) => { | |
| const normalized = (Array.isArray(entries) ? entries : []) | |
| .filter((entry): entry is string => typeof entry === 'string') | |
| .map((entry) => { |
中文说明
新的元数据 key 对修复前 key 能够容忍的畸形过滤器形状不是全函数——失败场景：非数组过滤器（例如 excludeTools: "x"，一个真实可能出现的配置笔误——上游不做类型校正：SETTINGS_SCHEMA.mcpServers 只是一个裸 object，daemon 运行时新增接口只校验 config 是对象）会抛出 (entries ?? []).map is not a function。在单会话分类循环（mcp-client-manager.ts:2271）中，singleSessionConnectedKeyOf 的调用不在任何逐服务器 try/catch 之内——探针实测：整个对账流程中断，没有任何服务器连接或重连（一个携带合法变更的健康兄弟服务器也未能重连），discoveryState 停留在 IN_PROGRESS，末尾的 mcp-client-update 永远不会发出，并且在配置修正之前每次对账都会再次抛出。修复前基于展开运算符的 key 可以容忍字符串。注意：仅加 Array.isArray 防护是不够的——非字符串条目仍会在 normalizeMcpIncludeEntry 中抛错（见相邻评论）；下面的类型收敛可以同时覆盖 key 路径上的两种形状。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| export function normalizeMcpIncludeEntry(entry: string): string { | ||
| const paren = entry.indexOf('('); |
There was a problem hiding this comment.
[Critical] Same non-totality, second trigger: a non-string entry inside a valid array — Failure scenario: includeTools: [123] throws entry.indexOf is not a function inside mcpSessionMetadataKey (probe-verified), producing the same whole-pass abort at the unguarded classification loop; pre-diff the spread-based key tolerated numeric entries. The sibling fix (array guard + string-entry coercion in normalizeFilter) covers the key path, but compileNameFilter (session-mcp-view.ts:55) also calls this function on the raw config at apply time and re-throws on the same input — harden both call paths together (see the PoolEntry.updateSessionConfig ordering comment) before landing a key-only fix.
中文说明
同样的非全函数问题，第二个触发点：合法数组中的非字符串条目——失败场景：includeTools: [123] 会在 mcpSessionMetadataKey 内部抛出 entry.indexOf is not a function（已用探针验证），在无防护的分类循环处造成同样的整轮对账中断；修复前基于展开运算符的 key 可以容忍数字条目。相邻修复（normalizeFilter 中的数组防护 + 字符串条目收敛）可以覆盖 key 路径，但 compileNameFilter（session-mcp-view.ts:55）在应用阶段也会对原始配置调用本函数，并对同样的输入抛错——在只做 key 修复之前，请把两条调用路径一起加固（见 PoolEntry.updateSessionConfig 顺序问题的评论）。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| this.cliConfig.addRuntimeMcpServer(name, config); | ||
| existingConn.updateConfig(config); |
There was a problem hiding this comment.
[Critical] Overlay mutation has no rollback when the refresh throws — Failure scenario: this branch writes the Config overlay before existingConn.updateConfig(config). The daemon runtime-add surfaces validate only that config is an object, so a malformed metadata shape reaches here; probe-measured: the overlay write lands (1 add, 0 compensating removes) and the refresh throws — the overlay keeps the new config while the session view keeps projecting old metadata. Unlike the spawn-failure path below, which rolls back via removeRuntimeMcpServer, nothing compensates: every subsequent pool-mode reconcile re-throws in the per-server updateConfig catch (logged, that server never refreshes again). Pre-diff this branch could not throw (overlay write only). Suggested fix refreshes before mutating (updateConfig takes the config directly and needs no overlay state); alternatively wrap and roll back like the spawn-failure path.
| this.cliConfig.addRuntimeMcpServer(name, config); | |
| existingConn.updateConfig(config); | |
| existingConn.updateConfig(config); | |
| this.cliConfig.addRuntimeMcpServer(name, config); |
中文说明
刷新抛错时，运行时覆盖层（overlay）的写入没有回滚——失败场景：此分支先写 Config 覆盖层，再调用 existingConn.updateConfig(config)。daemon 运行时新增接口只校验 config 是对象，畸形元数据形状可以到达这里；探针实测：覆盖层写入成功（1 次 add，0 次补偿性 remove），随后刷新抛错——覆盖层保留了新配置，而会话视图继续投影旧元数据。与下方生成失败路径（通过 removeRuntimeMcpServer 回滚）不同，这里没有任何补偿：此后每次池化模式对账都会在逐服务器的 updateConfig catch 中再次抛错（仅记录日志，该服务器永远无法再刷新）。修复前此分支不可能抛错（只有覆盖层写入）。建议的修复是先刷新再写覆盖层（updateConfig 直接接收 config，不依赖覆盖层状态）；或者像生成失败路径一样包裹并在抛错时回滚。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| existingConn.updateConfig(config); | ||
| const toolCount = existingConn.toolsSnapshot.length; |
There was a problem hiding this comment.
[Suggestion] toolCount is read from the unfiltered canonical snapshot after the diff-added updateConfig re-filters the session — Concrete cost: pre-diff this branch never re-filtered, so the reported count matched the session's registrations; now a same-fingerprint re-add with includeTools: ['a'] shrinks the registry to 1 while the mcp_server_added workspace event (acp-bridge) and the mcp_registered response (cli/serve/acp-http) still report the unfiltered N — clients are told "N tools" at the exact moment the session can invoke 1 (probe-verified: the implementation returns 3 while the session can invoke 1). The standalone branch below reports the session-accurate this.toolRegistry.getToolsByServer(name).length. Note: the rewritten case-4 test currently pins the stale count and must be updated together with this fix.
| existingConn.updateConfig(config); | |
| const toolCount = existingConn.toolsSnapshot.length; | |
| existingConn.updateConfig(config); | |
| const toolCount = this.toolRegistry.getToolsByServer(name).length; |
中文说明
在 diff 新增的 updateConfig 重新过滤会话之后，toolCount 却从未经过滤的规范快照读取——具体代价：修复前此分支从不重新过滤，因此上报数量与会话注册数一致；现在同指纹重复添加并带 includeTools: ['a'] 时，注册表收缩到 1 个工具，而 mcp_server_added 工作区事件（acp-bridge）和 mcp_registered 响应（cli/serve/acp-http）仍上报未过滤的 N——在会话只能调用 1 个工具的时刻，客户端却被告知"N 个工具"（探针验证：实现返回 3，而会话只能调用 1）。下方独立分支已经使用会话准确的 this.toolRegistry.getToolsByServer(name).length。注意：重写后的 case-4 测试目前固定了这个过期数值，修复时需要一并更新。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| updateSessionConfig(sessionId: string, cfg: MCPServerConfig): void { | ||
| if (this.isTerminated()) { | ||
| throw new Error( |
There was a problem hiding this comment.
[Suggestion] Both fail-closed guards in this new method (terminated entry, detached session) have no test — Concrete cost: only the released-handle guard in PooledConnectionImpl.updateConfig is asserted (toThrow(/released MCP connection/)); the manager-level tests mock updateConfig with vi.fn(), so these two guards are never exercised. A refactor turning either throw into a silent return ships green, and combined with the overlay-before-refresh ordering in addRuntimeMcpServer, a just-terminated entry could leave stale registrations while the API reports success. Add pool-level tests: call updateConfig after forcing the entry terminal (expect the in-state throw), and for a never-attached/detached session id (expect the detached-session throw).
中文说明
这个新方法中的两个失败即关闭（fail-closed）守卫（已终止条目、已分离会话）没有测试——具体代价：目前只有 PooledConnectionImpl.updateConfig 中已释放句柄的守卫被断言（toThrow(/released MCP connection/)）；manager 层测试用 vi.fn() 模拟了 updateConfig，因此这两个守卫从未被执行。若重构把任一抛错改成静默返回，整个测试套件仍然绿灯；结合 addRuntimeMcpServer 中先写覆盖层再刷新的顺序，一个刚终止的条目可能在 API 报告成功的同时留下过期注册。请补充池层测试：在强制条目进入终止状态后调用 updateConfig（期望对应的状态抛错），以及对从未挂载/已分离的会话 id 调用（期望分离会话抛错）。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| it('refreshes one session metadata in place without mutating the shared snapshot', async () => { | ||
| const mocked = mockMcpSuccess({ | ||
| toolNames: ['alpha', 'beta'], |
There was a problem hiding this comment.
[Suggestion] Sibling-session isolation in PoolEntry.updateSessionConfig is unpinned — Concrete cost: every connection.updateConfig test runs with exactly one session attached, and the two-session shared-transport test (:314) never calls updateConfig. A refactor broadcasting the refresh to all subscribers instead of only subscribers.get(sessionId) ships green: session B would then get removeMcpToolsByServer plus re-registration under session A's updated trust/includeTools — transient tool loss and cross-session trust bleed (B's tools re-projected with A's trust: true, bypassing the approval gate for a session that never opted in). Attach a second session with its own registries, refresh the first connection, and assert the second session saw zero registry calls and keeps its own trust/alwaysLoad.
中文说明
PoolEntry.updateSessionConfig 的兄弟会话隔离没有任何测试固定——具体代价：所有 connection.updateConfig 测试都只挂载了一个会话，而双会话共享传输的测试（:314）从不调用 updateConfig。如果把刷新重构为广播给所有订阅者（而不是只刷新 subscribers.get(sessionId)），测试套件仍然绿灯：会话 B 会被 removeMcpToolsByServer 并按会话 A 更新后的 trust/includeTools 重新注册——瞬时工具丢失加跨会话信任渗透（B 的工具被按 A 的 trust: true 重新投影，绕过了一个从未选择信任的会话的审批关卡）。请挂载第二个带独立注册表的会话，刷新第一个连接，并断言第二个会话的注册表零调用且保留自己的 trust/alwaysLoad。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| const equivalent = { | ||
| command: 'node', | ||
| includeTools: ['alpha', 'beta'], | ||
| excludeTools: ['zeta'], | ||
| } as MCPServerConfig; |
There was a problem hiding this comment.
[Suggestion] trust and excludeTools participation in mcpSessionMetadataKey is unpinned by any test — Concrete cost: mutation-probed inventory at this commit: dropping trust from the key, or substituting excludeTools with undefined, each leaves all 178 tests across the three suites green (includeTools and alwaysLoadTools ARE pinned). This test varies excludeTools only between normalization-equivalent values (['zeta','zeta'] vs ['zeta']) and never varies trust at all. A refactor dropping either field ships green → a trust-only or excludeTools-only settings edit yields an identical key → no re-apply → tools keep executing as trusted after a downgrade, or just-excluded tools stay registered and invocable, until an unrelated metadata change or reconnect. Add not.toBe cases: {command, trust: true} vs {command} (plus false vs missing, per the design doc's three-state rule), and {command, excludeTools: ['foo']} vs {command}; ideally also vary each field alone in a pool-level updateConfig refresh test.
中文说明
trust 和 excludeTools 参与 mcpSessionMetadataKey 这件事没有任何测试固定——具体代价：在本提交上做了变异探针实测：把 trust 从 key 中去掉、或把 excludeTools 替换为 undefined，三种套件的 178 个测试全部仍然通过（includeTools 和 alwaysLoadTools 是有固定的）。这个测试只在规范化后等价的取值之间变化 excludeTools（['zeta','zeta'] 对比 ['zeta']），且从不变化 trust。一旦重构丢掉了其中任一字段，测试仍然绿灯 → 只改 trust 或只改 excludeTools 的配置编辑会得到相同的 key → 不触发重新应用 → 降级后工具继续按受信任状态执行，或刚被排除的工具仍然注册且可调用，直到某个无关的元数据变化或重连。请补充 not.toBe 用例：{command, trust: true} 对比 {command}（按设计文档的三态规则再加 false 对比缺省），以及 {command, excludeTools: ['foo']} 对比 {command}；最好再在池层 updateConfig 刷新测试中单独变化每个字段。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| if (!view.updateConfig(cfg)) return; | ||
| if (this.state === 'active') { | ||
| view.applyTools(this.toolsSnapshot); |
There was a problem hiding this comment.
[Suggestion] Fix-guidance: the metadata key is committed before this fallible re-apply — do not harden the metadata key without also hardening compileNameFilter — Failure scenario: unreachable at the reviewed commit (mcpSessionMetadataKey throws first), but probe-reproduced under a key-only hardening of the malformed-shape findings elsewhere in this review: the key commits → applyTools removes all of the server's tools, then compileNameFilter throws on the same input → the equal-key gate blocks every retry → silent per-session tool loss. The restart fan-out re-throws too, so recovery needs a config change or session teardown. applyPrompts has the same remove-before-compile shape. Couple the fixes: harden compileNameFilter with the same array/string coercion in the same change as the key hardening, or compile the filter before removing registrations (both applyTools and applyPrompts if reordering).
中文说明
修复指引：元数据 key 在这次可能失败的重新应用之前就已提交——加固元数据 key 时不要漏掉 compileNameFilter——失败场景：在被审查提交上不可达（mcpSessionMetadataKey 会先抛错），但在对本审查其余畸形形状发现只做 key 加固的前提下，探针复现了该问题：key 提交 → applyTools 移除该服务器的全部工具，随后 compileNameFilter 对同样的输入抛错 → key 相等的大门挡住所有重试 → 会话级工具被静默丢失。重启扇出同样会再次抛错，因此恢复只能靠配置变更或会话销毁。applyPrompts 有同样的先移除后编译形状。请把修复耦合起来：在加固 key 的同一变更中用同样的数组/字符串收敛加固 compileNameFilter，或者先编译过滤器再移除注册（若选择重排，applyTools 和 applyPrompts 都要改）。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| expect(updateConfig).toHaveBeenCalledOnce(); | ||
| expect(updateConfig).toHaveBeenCalledWith(updatedConfig); |
There was a problem hiding this comment.
[Suggestion] This test never asserts the Config overlay write that makes the same-fingerprint refresh persist — Concrete cost: probe-verified: deleting this.cliConfig.addRuntimeMcpServer(name, config) (mcp-client-manager.ts:2977) leaves 178/178 green. Case 1 pins the overlay write for the fresh-add branch; nothing pins it for this refresh branch. A maintainer simplifying the branch could drop the overlay write as redundant (transport reused, live session already refreshed); runtime metadata replacement would then apply to the live session but persist nowhere — the next reconciliation pass resolves the server from settings plus the stale overlay entry, and the retained-connection refresh silently reverts filters/trust/alwaysLoadTools to pre-edit values.
| expect(updateConfig).toHaveBeenCalledOnce(); | |
| expect(updateConfig).toHaveBeenCalledWith(updatedConfig); | |
| expect(updateConfig).toHaveBeenCalledOnce(); | |
| expect(updateConfig).toHaveBeenCalledWith(updatedConfig); | |
| expect(config.addRuntimeMcpServer).toHaveBeenLastCalledWith('dup-srv', updatedConfig); |
中文说明
这个测试从未断言使同指纹刷新得以持久化的 Config 覆盖层写入——具体代价：探针验证：删除 this.cliConfig.addRuntimeMcpServer(name, config)（mcp-client-manager.ts:2977）后 178/178 仍然全部通过。case 1 为新增分支固定了覆盖层写入；这个刷新分支没有任何固定。维护者简化该分支时可能把覆盖层写入当作冗余删掉（传输被复用、活动会话已经刷新）；那样运行时元数据替换只会作用于活动会话而不持久化——下一次对账会从设置加过期覆盖层条目解析该服务器，保留连接的刷新会把 filters/trust/alwaysLoadTools 静默回退到编辑前的值。
— qwen3.8-max via Qwen Code /review (v0.21.5)
The daemon's ACP session executed tool batches differently from the core scheduler in two ways that broke long agent fan-outs such as /review: runBounded — the runner for concurrent batches — forced the first three calls of any batch larger than the invalid-params threshold to run one at a time, then clamped the rest to concurrency 3, although agent calls are concurrency-safe and core's runConcurrently runs them at QWEN_CODE_MAX_TOOL_CONCURRENCY (default 10). A /review fan-out of 10-15 agents therefore ran almost serially. Agent-only batches now skip the serial prefix and the clamp: an invalid agent call fails in build() before any side effect, so the concurrent loop's near-threshold check still catches invalid-params loops just as fast. The per-turn tool-call cap halted unconditionally at model.maxToolCallsPerTurn (default 100) while core's LoopDetectionService treats the default as adaptive — past the soft cap a productive turn (diverse calls, no repetition) continues until a stuck-repetition signal or the hard backstop (soft cap x 10). A /review orchestrator needs well over 100 calls, so every high-effort review under qwen serve died mid-review at call 101. The daemon now mirrors core's checkTurnToolCallCap semantics, reusing the same thresholds. Measured on two high-effort /review runs (PRs #8522 and #8529): the baseline died at call 101 after ~8.3h each; after this fix both reviews run to completion in 4.4h / 5.3h, first fan-out wave 85m -> 33m, reverse-audit rounds 63-86m -> 25-35m.
The daemon's ACP session executed tool batches differently from the core scheduler in two ways that broke long agent fan-outs such as /review: runBounded — the runner for concurrent batches — forced the first three calls of any batch larger than the invalid-params threshold to run one at a time, then clamped the rest to concurrency 3, although agent calls are concurrency-safe and core's runConcurrently runs them at QWEN_CODE_MAX_TOOL_CONCURRENCY (default 10). A /review fan-out of 10-15 agents therefore ran almost serially. Agent-only batches now skip the serial prefix and the clamp: an invalid agent call fails in build() before any side effect, so the concurrent loop's near-threshold check still catches invalid-params loops just as fast. The per-turn tool-call cap halted unconditionally at model.maxToolCallsPerTurn (default 100) while core's LoopDetectionService treats the default as adaptive — past the soft cap a productive turn (diverse calls, no repetition) continues until a stuck-repetition signal or the hard backstop (soft cap x 10). A /review orchestrator needs well over 100 calls, so every high-effort review under qwen serve died mid-review at call 101. The daemon now mirrors core's checkTurnToolCallCap semantics, reusing the same thresholds. Measured on two high-effort /review runs (PRs #8522 and #8529): the baseline died at call 101 after ~8.3h each; after this fix both reviews run to completion in 4.4h / 5.3h, first fan-out wave 85m -> 33m, reverse-audit rounds 63-86m -> 25-35m.
The daemon's ACP session executed tool batches differently from the core scheduler in two ways that broke long agent fan-outs such as /review: runBounded — the runner for concurrent batches — forced the first three calls of any batch larger than the invalid-params threshold to run one at a time, then clamped the rest to concurrency 3, although agent calls are concurrency-safe and core's runConcurrently runs them at QWEN_CODE_MAX_TOOL_CONCURRENCY (default 10). A /review fan-out of 10-15 agents therefore ran almost serially. Agent-only batches now skip the serial prefix and the clamp: an invalid agent call fails in build() before any side effect, so the concurrent loop's near-threshold check still catches invalid-params loops just as fast. The per-turn tool-call cap halted unconditionally at model.maxToolCallsPerTurn (default 100) while core's LoopDetectionService treats the default as adaptive — past the soft cap a productive turn (diverse calls, no repetition) continues until a stuck-repetition signal or the hard backstop (soft cap x 10). A /review orchestrator needs well over 100 calls, so every high-effort review under qwen serve died mid-review at call 101. The daemon now mirrors core's checkTurnToolCallCap semantics, reusing the same thresholds. Measured on two high-effort /review runs (PRs #8522 and #8529): the baseline died at call 101 after ~8.3h each; after this fix both reviews run to completion in 4.4h / 5.3h, first fan-out wave 85m -> 33m, reverse-audit rounds 63-86m -> 25-35m.
…cap (QwenLM#8631) * fix(cli): Run ACP agent fan-outs concurrently and past the tool-call cap The daemon's ACP session executed tool batches differently from the core scheduler in two ways that broke long agent fan-outs such as /review: runBounded — the runner for concurrent batches — forced the first three calls of any batch larger than the invalid-params threshold to run one at a time, then clamped the rest to concurrency 3, although agent calls are concurrency-safe and core's runConcurrently runs them at QWEN_CODE_MAX_TOOL_CONCURRENCY (default 10). A /review fan-out of 10-15 agents therefore ran almost serially. Agent-only batches now skip the serial prefix and the clamp: an invalid agent call fails in build() before any side effect, so the concurrent loop's near-threshold check still catches invalid-params loops just as fast. The per-turn tool-call cap halted unconditionally at model.maxToolCallsPerTurn (default 100) while core's LoopDetectionService treats the default as adaptive — past the soft cap a productive turn (diverse calls, no repetition) continues until a stuck-repetition signal or the hard backstop (soft cap x 10). A /review orchestrator needs well over 100 calls, so every high-effort review under qwen serve died mid-review at call 101. The daemon now mirrors core's checkTurnToolCallCap semantics, reusing the same thresholds. Measured on two high-effort /review runs (PRs QwenLM#8522 and QwenLM#8529): the baseline died at call 101 after ~8.3h each; after this fix both reviews run to completion in 4.4h / 5.3h, first fan-out wave 85m -> 33m, reverse-audit rounds 63-86m -> 25-35m. * fix(cli): regenerate settings schema after maxToolCallsPerTurn doc update (QwenLM#8631) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): Gate the daemon repeat halt on skipLoopDetection like core * fix(cli): Address ACP fan-out review: keep wide-batch results, shared cap predicate (QwenLM#8631) - runBounded no longer aborts in-flight calls when loop detection fires in the capped race branch: wide batches keep in-flight results and only skip the unstarted tail, matching narrow-batch behaviour (nothing executed is discarded either way). - Extract shouldHaltOnTurnToolCallCap from core's checkTurnToolCallCap and call it from the daemon guard so the two runtimes share one halt predicate and cannot drift. - Hoist canonicalToolName into tools/tool-names.ts beside ToolNamesMigration; scheduler, loop detection, plan redaction and memory refresh now share the single alias resolver. - Correct the wrong-direction cap wording (the daemon undershoots an explicit cap / hard backstop — the batch check runs before execution; the adaptive soft cap is exceeded by design up to the backstop) in the daemon comment, settingsSchema.ts (schema regenerated) and settings.md, and scope the always-on-guard sentence to core-client sessions. - Tests: adaptive hard backstop, wide-batch loop tail skip, wide-batch keep-results, provider-duplicate counter exclusion, `task`-alias fan-out, getToolCallRepeatKey alias/key-order coverage; raise the fan-out concurrency deadline off the 2s wall clock. * fix(cli): Address review: complete loop-guard docs, pin test envs, drop dead export (QwenLM#8631) * fix(cli): Address review: correct parity comment, pin halt semantics, test cross-response repeats (QwenLM#8631) --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
|
@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，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Address-review summary — PR #8522 (round 2)Commit: All 12 inline findings from the automated reviewer and the 1 earlier suggestion are resolved in code; no finding was declined, deferred, or escalated. Critical findings — fixed
Suggestions — implemented
Already resolved before this round (re-verified)
No code action
Failure-first evidence
Files changed
Verification
中文说明审查处理总结 — PR #8522（第 2 轮）提交： 自动审查者的 12 条行内发现与此前的 1 条建议已全部在代码中解决；没有拒绝、推迟或升级任何发现。 Critical 发现 — 已修复
建议 — 已实现
本轮之前已解决（本轮复核确认）
无需代码处理
失败优先证据
变更文件
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 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/模型 |
|
🔀 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 将重新运行。 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Address-review round — no code changes (PR #8522)This round's only feedback item was the failed Why the failed check is not a code defect
The remedy for this kind of failure is a job re-run — there is no code fix to apply. Surrogate verification on head
|
| Bundle | Test file | Result |
|---|---|---|
| PR head | PR version (workspaceCwd: REPO_ROOT) |
fails — same 30 s timeout signature |
origin/main |
PR version (workspaceCwd: REPO_ROOT) |
fails — identical signature |
origin/main |
main version (workspaceCwd: workspaceDir) |
passes (whole file in ~7 s) |
So the failure is a property of the older, heavier test variant (it runs the daemon session against the full repository root and is timing-tight on a loaded shared runner — machine load averaged 13 during the failing runs), not of this PR's code. Main already replaced that variant via 02f1692d40 (#8445), which landed after this PR forked; rebasing onto current main will pick the faster variant up automatically. The Todo Stop Guard code path (ACP session layer) is untouched by this PR, and the test session starts no MCP servers at all.
No conflicts to resolve (--conflict false). No commits pushed this round.
中文说明
评审处理轮次 — 无代码改动（PR #8522）
本轮唯一的反馈项是失败的 Test (ubuntu-latest, Node 22.x) 检查（无评审意见、行内评论或 issue 级评论）。经过基于证据的排查，本轮不做任何代码改动：失败的 CI 任务在到达任何代码质量步骤之前就已终止，因此本 PR 的改动既不是失败原因，也无法通过代码修复解决。
为什么该失败检查不是代码缺陷
- 失败的任务只运行了 22 秒：开始于
2026-08-07T19:34:39Z，结束于2026-08-07T19:35:01Z（run31212053783，job92976981345；数据来自本轮的检查状态快照）。 - 根据
.github/workflows/ci.yml，Test 任务要在工作区清理、checkout、Node 安装和npm ci之后才会到达代码质量关卡（ESLint、Prettier、i18n、settings-schema 新鲜度检查、单元测试、no-AK 集成门禁）——这些需要数十分钟。22 秒的运行不可能到达其中任何一步；它失败在代码之前的基础设施窗口（checkout / head 提交校验 / Node 供应），而ci.yml中本身就记录了这些步骤上可能出现的过期 ref 与出口代理抖动。 - 本 PR 的 CI 此前是绿色的（
e33110b，8 月 4 日，见 triage 机器人评论），此后新增的改动仅涉及packages/core的 MCP 代码和一份设计文档。
这类失败的解决办法是重新运行该任务——没有可应用的代码修复。
在 head 5cabd9d534 上的替代验证
npm run build— 通过npm run typecheck— 通过npm run bundle— 通过npm run lint— 通过（0 错误/警告）- 对所有改动文件执行
npx prettier --check— 通过 npm run check-i18n/npm run check:lockfile/npm run check:desktop-isolation— 全部通过- 针对改动文件的聚焦 vitest（
mcp-client-manager、mcp-transport-pool、session-mcp-view）— 184 通过 packages/core完整 vitest 套件 — 19,107 通过；12 个非 MCP 套件（logger、storage、ide-client、editor、token-storage 等）有 82 个失败，且在本环境中签出origin/main后完全相同地复现 —— 属于既有/环境问题，并非本 PR 引入npm run test:integration:no-ak:sandbox:none（Test 任务的集成步骤，使用与 CI 相同的空 API key 环境）— 128/129 通过
唯一的集成失败项 cli/qwen-serve-streaming.test.ts 的 "daemon Todo Stop Guard replay … bounded attempts"（30 秒计时轮询只观察到 4 次预期模型调用中的 2 次），已通过 bundle/测试文件矩阵隔离：
| Bundle | 测试文件 | 结果 |
|---|---|---|
| PR head | PR 版本（workspaceCwd: REPO_ROOT） |
失败 — 相同的 30 秒超时特征 |
origin/main |
PR 版本（workspaceCwd: REPO_ROOT） |
失败 — 特征完全一致 |
origin/main |
main 版本（workspaceCwd: workspaceDir） |
通过（整个文件约 7 秒） |
因此该失败是旧版、更重的测试变体的属性（它把守护进程会话跑在整个仓库根目录上，在高负载的共享 runner 上时间窗口很紧——失败运行时机器平均负载达 13），而不是本 PR 代码的问题。main 分支已通过 02f1692d40（#8445）替换了该变体，且该提交在本 PR 分叉之后才合入；rebase 到最新 main 时会自动获得更快的变体。Todo Stop Guard 代码路径（ACP 会话层）本 PR 完全未触碰，且该测试会话根本不会启动任何 MCP 服务器。
无冲突需要解决（--conflict false）。本轮未推送任何提交。
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。
🧠 Handled by Qwen Code · model/模型 qwen3.8-max
Independent verification — real local runtime, A/B against
|
| Base | 4ec0371 — the main side of this PR's merge commit, so base…head is exactly this PR's diff |
| Head | 24ff52f |
| Platform | Linux x86_64, Node v22.22.2, npm 10.9.7 |
| Builds | Two separate worktrees, each built independently. Each worktree gets its own node_modules/@qwen-code symlink set, so neither the harness nor the CLI under test can silently resolve @qwen-code/qwen-code-core to the other build's packages/core. |
| Fixture | A real stdio MCP server (@modelcontextprotocol/sdk 1.29.0) exposing 3 tools, 3 prompts, 1 resource. It appends its own PID to a log on every process start, so "did the transport reconnect?" is answered by counting OS processes, not by inspecting a mock. |
What is actually driven: the real McpTransportPool + McpClientManager + Config.reinitializeMcpServers loaded from each build's dist, wired in the same order production uses (setMcpTransportPool before initialize(), matching acpAgent.createWorkspaceMcpDiscoveryConfig). reinitializeMcpServers is the exact entry point both the settings watcher (packages/cli/src/config/hot-reload.ts) and the daemon reload path (acpAgent.reloadWorkspaceMcpDiscovery) call.
A/B invariant matrix
| BASE | PR | |
|---|---|---|
| Pooled path (daemon / shared transport) | 5/10 | 10/10 |
| Unpooled path (non-pooled transport) | 1/3 | 3/3 |
| Legacy single-session path (plain CLI) | 2/5 | 5/5 |
On BASE, P3/P5/P6 pass vacuously — BASE never refreshes a retained session at all, so there is nothing to churn. They are kept in the matrix because they are the regression guards for the PR side.
The four failure modes, measured
1. Pooled: a metadata-only edit left the session stale. Session holds includeTools: ["alpha"], then edits to includeTools: ["beta","gamma"], trust: true, alwaysLoadTools: true.
BASE tools=[alpha] <- stale; the edit is silently dropped
PR tools=[beta,gamma] trust=true eager=true, spawns=1, pid unchanged
On the PR build a real tool call against the newly allowed tool succeeds and reports the original PID, proving the refresh reused the transport rather than reconnecting:
{"tool":"beta","pid":3121310,"echo":"after-refresh"} // pid == the pid from the initial spawn2. Unpooled: healthy transports were respawned on every reconcile pass. Five reconcile passes with an unchanged config:
BASE spawns=6 pids=[3121700, 3121708, 3121716, 3121724, ...] <- 5 unnecessary reconnects
PR spawns=1 pids=[3121370]
This is the clearest measurable win in the PR — five settings touches previously cost five MCP process restarts per unpooled server. The new transportId is visible on the handle exactly as designed:
BASE id=fixture::unpooled-0 transportId=(absent on this build)
PR id=fixture::unpooled-0 transportId=fixture::c60dbdece15f2a72
3. Cross-session metadata leak (beyond what #8492 describes). Two sessions share one transport; session A sets alwaysLoadTools: false, session B sets alwaysLoadTools: true:
BASE A=[alpha/trust=false/eager=false] B=[beta/trust=true/eager=false] <- B lost its eager flag
PR A=[alpha/trust=false/eager=false] B=[beta/trust=true/eager=true]
BASE projected trust per session but read alwaysLoad off the shared canonical snapshot, which had baked in the first subscriber's value. withSessionConfig is what closes this; it is a genuine bug fix, not just a rename of withTrust. Worth calling out in the changelog.
4. Legacy single-session path. alwaysLoadTools-only edits took effect (BASE: ignored); canonical-equivalent filter edits (['beta(x)','alpha','beta','alpha']) stopped reconnecting (BASE: reconnects); and includeTools: [] → null is now a real change (BASE collapsed both to [] via ?? [], so "allow none" → "allow all" never applied).
No regression on the transport boundary. Changing a transport-affecting field still releases and reacquires on both builds — spawns=2 tags=[v1, v2-transport-changed].
Real TUI, end to end
.qwen/settings.json edited mid-session to change only alwaysLoadTools: false → true, same fixture server, --yolo so MCP approval gating is not a confounding variable. /context is the surface where eager loading is observable — a deferred MCP tool is excluded from the model's declaration list, an alwaysLoad one is not.
BASE no "MCP tools" row spawns=1 -> edit silently ignored
PR MCP tools 114 tokens (0.0%) spawns=2 -> reconnect + re-apply
A control step in the same run rewrote settings.json with identical content: both builds correctly did nothing (spawns unchanged), so the PR-side reconnect is attributable to the semantic change and not to the file write.
Malformed settings totality (pool path)
Settings files are not schema-coerced, so these shapes reach the code. Every one is total on the PR build, and the session recovers afterwards:
| setting | PR result |
|---|---|
includeTools: ["alpha"] |
alpha |
includeTools: null |
alpha,beta,gamma (allow all) |
includeTools: [] |
(none) (allow none) |
includeTools: "alpha" |
(none) |
includeTools: [123,"beta"] |
beta |
excludeTools: "gamma" |
alpha,beta,gamma |
recovery → ["alpha","beta"] |
alpha,beta |
On BASE every row returns alpha — the session never refreshes at all, so it also never recovers. Neither build throws.
Other checks
npm test -w @qwen-code/qwen-code-core -- src/tools/session-mcp-view.test.ts \
src/tools/mcp-client-manager.test.ts src/tools/mcp-transport-pool.test.ts \
src/tools/mcp-pool-entry.test.ts src/tools/mcp-tool.test.ts
-> Test Files 5 passed (5) Tests 300 passed (300)
npm run typecheck -w @qwen-code/qwen-code-core -> clean
eslint <the 6 changed source files> -> clean
I also re-read fingerprint() in mcp-pool-key.ts to confirm the PR's claim that it needed no behavior change: it builds an explicit allowlist canonical object, and alwaysLoadTools was never a member. The mcp-pool-key.ts hunk is correctly comment-only.
Non-blocking observations
(a) Malformed string filters diverge between the pooled and legacy paths. With excludeTools: "gamma" (a string, not an array), the legacy discovery filter at mcp-client.ts:2283 does excludeTools.includes(name) — on a string that is JS substring matching, so gamma is excluded. The pooled session view coerces a non-array to "no excludes", so all three tools register. I confirmed by isolating initial discovery on both builds that this divergence is identical on BASE — pre-existing, not introduced here. A follow-up that shares one coercion between mcp-client.ts and mcp-session-config.ts would close it.
(b) The two coercion directions are inconsistent. includeTools: "alpha" (a plausible user typo) coerces to [] = allow none, silently hiding every tool from that server; excludeTools: "gamma" coerces to no excludes = fail-open. Fail-closed on include is the safer default, so I am not asking to change the semantics — but a one-line user-visible warning when a filter is not a string array would turn a silent "my MCP server has no tools" into a diagnosable one.
Scope not covered
- The
qwen servedaemon was not driven end-to-end over its REST surface. The pooled evidence exercises the sameMcpTransportPool/ per-sessionConfigobjects the daemon constructs, but not the HTTP routes. - Linux only; Windows and macOS not run here.
- No SDK-MCP or HTTP/SSE transports exercised — stdio fixture only.
中文版
独立验证 —— 真实本地运行时，与 main 做 A/B 对比
结论：验证通过，建议合并。 PR 描述中的每一项声明都在真实 MCP 子进程上复现成功，覆盖全部三条对账路径。base 构建复现了全部四种失败模式；PR 构建修复了它们，且没有破坏"影响传输的变更仍然重连"这一边界。文末有 2 条非阻断观察，均非本 PR 引入。
环境
| Base | 4ec0371 —— 本 PR 合并提交的 main 一侧，因此 base…head 恰好等于本 PR 的 diff |
| Head | 24ff52f |
| 平台 | Linux x86_64、Node v22.22.2、npm 10.9.7 |
| 构建 | 两个独立 worktree，各自独立构建。每个 worktree 拥有自己的 node_modules/@qwen-code 符号链接集，因此被测的 harness 与 CLI 都不可能悄悄把 @qwen-code/qwen-code-core 解析到另一个构建的 packages/core。 |
| Fixture | 真实 stdio MCP 服务（@modelcontextprotocol/sdk 1.29.0），暴露 3 个工具、3 个 prompt、1 个 resource。它在每次进程启动时把自己的 PID 追加写入日志，因此"传输是否重连"是通过统计操作系统进程数得出的，而不是靠检查 mock。 |
实际被驱动的对象：从各构建 dist 加载的真实 McpTransportPool + McpClientManager + Config.reinitializeMcpServers，并按生产代码的顺序接线（setMcpTransportPool 在 initialize() 之前，与 acpAgent.createWorkspaceMcpDiscoveryConfig 一致）。reinitializeMcpServers 正是 settings 监听器（packages/cli/src/config/hot-reload.ts）与 daemon 重载路径（acpAgent.reloadWorkspaceMcpDiscovery）共同调用的入口。
A/B 不变量矩阵
| BASE | PR | |
|---|---|---|
| 池化路径（daemon / 共享传输） | 5/10 | 10/10 |
| 非池化路径（不参与池化的传输） | 1/3 | 3/3 |
| 传统单会话路径（普通 CLI） | 2/5 | 5/5 |
在 BASE 上，P3/P5/P6 属于空洞通过 —— BASE 根本不会刷新已保留的会话，所以也就无从抖动。保留它们是因为它们是 PR 一侧的回归护栏。
四种失败模式的实测
1. 池化：仅元数据的变更让会话保持过期状态。 会话初始为 includeTools: ["alpha"]，随后改为 includeTools: ["beta","gamma"], trust: true, alwaysLoadTools: true：
BASE tools=[alpha] <- 过期；变更被静默丢弃
PR tools=[beta,gamma] trust=true eager=true，spawns=1，pid 未变
在 PR 构建上，针对新放行工具的真实调用成功，且返回的是最初那个 PID，证明刷新复用了传输而非重连：
{"tool":"beta","pid":3121310,"echo":"after-refresh"} // pid 与首次 spawn 的 pid 相同2. 非池化：健康的传输在每次对账时都被重启。 用未发生变化的配置执行 5 次对账：
BASE spawns=6 pids=[3121700, 3121708, 3121716, 3121724, ...] <- 5 次不必要的重连
PR spawns=1 pids=[3121370]
这是本 PR 中最可量化的收益 —— 此前每动 5 次配置，每个非池化 server 就要付出 5 次 MCP 进程重启。新的 transportId 也完全按设计出现在句柄上：
BASE id=fixture::unpooled-0 transportId=(此构建上不存在)
PR id=fixture::unpooled-0 transportId=fixture::c60dbdece15f2a72
3. 跨会话元数据泄漏（超出 #8492 的描述范围）。 两个会话共享一个传输，会话 A 设 alwaysLoadTools: false，会话 B 设 alwaysLoadTools: true：
BASE A=[alpha/trust=false/eager=false] B=[beta/trust=true/eager=false] <- B 丢失了自己的预加载标志
PR A=[alpha/trust=false/eager=false] B=[beta/trust=true/eager=true]
BASE 按会话投影了 trust，却从共享的规范快照读取 alwaysLoad，而该快照已经固化了第一个订阅者的取值。withSessionConfig 正是修复此问题的关键；它是一处真实的缺陷修复，而不只是 withTrust 的改名。值得写进 changelog。
4. 传统单会话路径。 仅改 alwaysLoadTools 现在会生效（BASE：被忽略）；语义等价的过滤器改动（['beta(x)','alpha','beta','alpha']）不再触发重连（BASE：会重连）；includeTools: [] → null 现在被识别为真实变更（BASE 通过 ?? [] 把两者都塌缩成 []，因此"全部禁止"→"全部允许"永远不会生效）。
传输边界无回归。 修改影响传输的字段时，两个构建都仍然释放并重新获取连接 —— spawns=2 tags=[v1, v2-transport-changed]。
真实 TUI 端到端
会话运行中编辑 .qwen/settings.json，只修改 alwaysLoadTools: false → true，使用同一个 fixture server，并加 --yolo 以排除 MCP 审批门控这一干扰变量。/context 是可观测预加载行为的界面 —— 被延迟加载的 MCP 工具不会进入模型的声明列表，而 alwaysLoad 的工具会。
BASE 没有 "MCP tools" 行 spawns=1 -> 变更被静默忽略
PR MCP tools 114 tokens (0.0%) spawns=2 -> 重连并重新应用
同一次运行中的对照步骤：用完全相同的内容重写 settings.json，两个构建都正确地什么都没做（spawns 不变）。因此 PR 一侧的重连可归因于语义变更本身，而不是文件写入动作。
畸形配置的完全性（池化路径）
配置文件不经过 schema 强制转换，所以下列形态确实会到达这段代码。在 PR 构建上每一项都是完全的，且之后会话可以恢复：
| 配置 | PR 结果 |
|---|---|
includeTools: ["alpha"] |
alpha |
includeTools: null |
alpha,beta,gamma（全部允许） |
includeTools: [] |
(无)（全部禁止） |
includeTools: "alpha" |
(无) |
includeTools: [123,"beta"] |
beta |
excludeTools: "gamma" |
alpha,beta,gamma |
恢复 → ["alpha","beta"] |
alpha,beta |
在 BASE 上每一行都返回 alpha —— 会话根本不会刷新，因此也永远不会恢复。两个构建都不抛异常。
其他检查
npm test -w @qwen-code/qwen-code-core -- src/tools/session-mcp-view.test.ts \
src/tools/mcp-client-manager.test.ts src/tools/mcp-transport-pool.test.ts \
src/tools/mcp-pool-entry.test.ts src/tools/mcp-tool.test.ts
-> Test Files 5 passed (5) Tests 300 passed (300)
npm run typecheck -w @qwen-code/qwen-code-core -> 通过
eslint <6 个被改动的源文件> -> 通过
我还重读了 mcp-pool-key.ts 中的 fingerprint()，以核实 PR 关于"它无需行为变更"的说法：它构造的是一个显式白名单的 canonical 对象，alwaysLoadTools 从来就不在其中。mcp-pool-key.ts 这个 hunk 确实只是注释改动。
非阻断观察
(a) 畸形字符串过滤器在池化路径与传统路径之间行为不一致。 当 excludeTools: "gamma"（是字符串而非数组）时，mcp-client.ts:2283 的传统发现过滤器执行 excludeTools.includes(name) —— 对字符串来说这是 JS 的子串匹配，所以 gamma 确实被排除了。而池化的 session view 把非数组强制转换为"无排除项"，于是三个工具全部注册。我通过在两个构建上隔离初始发现流程确认：该差异在 BASE 上完全一致 —— 属于既有行为，并非本 PR 引入。后续可让 mcp-client.ts 与 mcp-session-config.ts 共用同一套强制转换来消除它。
(b) 两个方向的强制转换语义不一致。 includeTools: "alpha"（一个很可能出现的用户笔误）被转换为 [] = 全部禁止，会静默隐藏该 server 的所有工具；而 excludeTools: "gamma" 被转换为无排除项 = fail-open。对 include 采取 fail-closed 是更安全的默认，因此我不要求改变语义 —— 但如果在过滤器不是字符串数组时输出一行用户可见的警告，就能把"我的 MCP server 怎么没工具了"从静默故障变成可诊断问题。
未覆盖范围
- 未通过 REST 接口端到端驱动
qwen servedaemon。池化部分的证据驱动的是 daemon 所构造的同一批McpTransportPool/ 每会话Config对象，但没有覆盖 HTTP 路由。 - 仅 Linux；此处未运行 Windows 与 macOS。
- 未覆盖 SDK-MCP 及 HTTP/SSE 传输 —— 仅使用了 stdio fixture。
|
@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: 92 passed · 0 failed · 92 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：92 通过 · 0 失败 · 92 总计 Verification reportPR 8522 verification — fix(core): refresh MCP session metadata without reconnectingVerdict: 中文摘要
Central claim and A/B proofCentral claim: a metadata-only settings change ( Harness:
Head: 34/34 assertions; base: 29/29 (each base cell asserts the documented bug reproduces — expected-failure cells counted as passes). Witness captures: Reviewer Test Plan walkthrough (all steps performed by the harness)
Mutation matrix (vacuity of the PR's new tests)Baseline (unmutated scratch tree): 184/184 green. Each mutant reverts one production hunk; witness:
6/6 killed, no survivors, attribution exact (each mutant dies on the test written for its hunk). Positive control is inherent: the same suite is green unmutated, so the harness demonstrably can make it fail. Sibling sweep: metadata key vs runtime filter agreement
FindingsF1 (nit) — design doc inverts the runtime-replacement ordering. Not covered
MethodologyEnvironment: Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Released in v0.21.8. |
5 participants
What this PR does
Refreshes per-session MCP tool, prompt, and resource registrations when
trust,alwaysLoadTools,includeTools, orexcludeToolschanges while retaining a healthy transport. It separates a handle's lifecycle identity from its captured transport identity, projects trust and eager-loading metadata without mutating shared discovery snapshots, and canonicalizes equivalent filters to avoid registry churn.The same metadata identity now drives legacy single-session reconciliation and same-fingerprint runtime replacement, so all supported reconciliation paths apply consistent settings semantics.
Why it's needed
Metadata-only settings updates previously looked transport-identical and could leave tools visible, hidden, trusted, or eagerly loaded according to stale values until a restart. Unpooled handles had the inverse problem: their unique lifecycle ID was compared with a transport fingerprint, causing healthy connections to restart on every reconciliation pass.
This makes hot reload immediate without sacrificing pooled transport reuse or cross-session isolation. Fixes #8492.
Reviewer Test Plan
How to verify
npm test -w @qwen-code/qwen-code-core -- src/tools/session-mcp-view.test.ts src/tools/mcp-client-manager.test.ts src/tools/mcp-transport-pool.test.ts npm run typecheck npm run buildLocal result: 177 targeted tests passed; workspace typecheck and build passed. A real local stdio MCP process also retained its client and transport across metadata refresh, completed a real tool call returning
ok, and produced zero registry churn for canonical-equivalent settings.Evidence (Before & After)
N/A — this is a non-visual core lifecycle change. Deterministic pre-fix checks reproduced stale pooled metadata, unnecessary unpooled reconnects, stale same-fingerprint runtime replacement, and missing eager-load reconciliation; the added regressions pass after the change. Detailed command output is provided in the verification comment.
Tested on
Environment (optional)
macOS 26.3.2 arm64, Node.js v22.23.2, npm 10.9.8, local stdio MCP fixture.
Risk & Scope
Linked Issues
Fixes #8492
中文说明
此 PR 做了什么
当
trust、alwaysLoadTools、includeTools或excludeTools发生变化时，刷新单个会话中的 MCP 工具、提示词和资源注册，同时保留健康的传输连接。此变更将句柄的生命周期标识与创建时捕获的传输标识分离，在不修改共享发现快照的情况下按会话投影信任与预加载元数据，并规范化语义等价的过滤器以避免注册表抖动。传统单会话对账和同指纹运行时替换现在也使用相同的元数据标识，因此所有受支持的对账路径都采用一致的配置语义。
为什么需要此变更
此前，仅元数据发生变化的配置更新在传输层看起来完全相同，因此工具可能持续按照旧值保持可见、隐藏、受信任或预加载状态，直到连接重启。非池化句柄则存在相反的问题：其唯一生命周期 ID 被拿来与传输指纹比较，导致健康连接在每次对账时都被重启。
此变更让热更新立即生效，同时保留池化传输复用和跨会话隔离。修复 #8492。
审阅者测试计划
如何验证
npm test -w @qwen-code/qwen-code-core -- src/tools/session-mcp-view.test.ts src/tools/mcp-client-manager.test.ts src/tools/mcp-transport-pool.test.ts npm run typecheck npm run build本地结果：177 项定向测试全部通过；工作区类型检查和构建通过。真实本地 stdio MCP 进程在元数据刷新期间也保留了同一个 client 和传输，完成了一次返回
ok的真实工具调用，并且对规范化后语义等价的配置产生了 0 次注册表抖动。证据（变更前与变更后）
不适用——这是非可视化的核心生命周期变更。确定性的修复前检查复现了池化元数据过期、非池化连接不必要重连、同指纹运行时替换过期以及预加载配置未参与对账；新增回归测试在修复后全部通过。详细命令输出见验证评论。
测试平台
环境（可选）
macOS 26.3.2 arm64、Node.js v22.23.2、npm 10.9.8、本地 stdio MCP fixture。
风险与范围
关联 Issue
修复 #8492