fix(core): Avoid replaying unsafe MCP tool calls - #8387
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Independent E2E verificationAn independent test pass reproduced the pre-fix failure at The same harness against the implementation diff confirmed that missing annotations and Additional checks confirmed safe read-only replay and unsafe rejection through the callable fallback, plus unchanged Abort, non-connection error, and MCP 中文验证说明独立测试在 同一 harness 验证修复后，annotations 缺失和 额外验证还覆盖了 callable fallback 上的安全只读重放和不安全调用拒绝，以及 Abort、非连接错误和 MCP |
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. |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Post-implementation audit updateThe broad follow-up audit found and fixed three issues:
Regression coverage confirms zero replacement calls when annotations disappear, server trust is removed, or workspace trust changes during reconnect. An independent post-fix pass also verified argument/upstream-error redaction and unchanged safe direct/callable replay, Abort, non-connection, and Final verification: Core MCP tests 82/82, CLI MCP add tests 27/27, repository build, typecheck, lint, built CLI help inspection, and 中文说明实现后审计更新后续全面审计发现并修复了三项问题：
回归测试确认：reconnect 后 annotations 消失、server trust 被移除或 workspace trust 改变时，替换 client 的调用次数均为零。独立 post-fix 复验还确认参数和上游错误不会泄露，并验证安全 direct/callable 重放、Abort、非连接错误和 最终验证结果：Core MCP 测试 82/82、CLI MCP add 测试 27/27、仓库 build、typecheck、lint、构建后 CLI help 检查和 |
|
Thanks for the PR! Template: complete — all required sections present, bilingual body, evidence and risk sections filled in. ✓ Problem: The replay path this PR gates is real and easy to confirm on Direction: Aligned. MCP annotations exist in the spec precisely so clients can make retry decisions, and their documented defaults (destructive, non-idempotent) mean "no annotations" must mean "do not replay". Choosing duplicate-side-effect prevention over transparent-retry availability is the right tradeoff for an agent that executes writes on the user's behalf. Claude Code's changelog has no direct reference to MCP replay policy, but the area is clearly relevant. Size: Core production change is small — Approach: The scope feels right. The gate sits exactly where the retry decision is made, which both execution paths already funnel through; re-validating the re-discovered tool after reconnect closes the stale-inheritance gap I confirmed exists on Risk: No elevated risk signals — none of the changed files match the revert-correlated path list. The behavioral claim (a real server committing a side effect before dropping the connection) is covered by unit tests with mocked clients plus the author's local harness; I'll look at the CI evidence and whether a sandboxed lane should pin the live behavior in the review stage. Moving on to code review. 🔍 中文说明感谢贡献！ 模板： 完整 —— 所有必填章节齐全，中英双语，证据与风险部分已填写。✓ 问题： 本 PR 限制的重放路径在 方向： 对齐。MCP annotations 在规范中存在的意义就是让 client 做重试决策，其文档默认值（destructive、非幂等）意味着"没有标注"必须等于"不可重放"。在代理代替用户执行写入的场景下，选择防止重复副作用而不是透明重试的可用性，是正确的取舍。Claude Code 的 changelog 没有直接涉及 MCP 重放策略的条目，但该领域明显相关。 规模： 核心生产代码改动很小 —— 方案： 范围合理。门控正好落在重试决策发生的位置，两条执行路径本就汇聚于此；重连后对重新发现的工具再次校验，堵上了我确认在 风险： 无升级风险信号 —— 改动文件均未命中与 revert 相关的高风险路径列表。行为层面的主张（真实 server 在提交副作用后断开连接）由 mock 单测加作者本地验证脚本覆盖；我会在审查阶段核对 CI 证据，并判断是否需要沙箱验证通道来钉住真实行为。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewReviewed What I verified against the codebase, not just the diff:
One non-blocking nit: the unsafe-replay error is thrown as a plain Verdict so far: no correctness or convention issues found. The change is the minimal fix for the stated hazard. sequenceDiagram
participant P1 as Scheduler
participant P2 as Tool Invocation
participant P3 as MCP Server - old connection
participant P4 as Tool Registry
participant P5 as Replacement Tool - new connection
P1->>P2: execute the call
P2->>P3: tools call
P3--xP2: connection lost after side effect
P2->>P2: canSafelyReplay - trust, workspace, annotations
alt unsafe to replay
P2-->>P1: stable unknown-result error, no reconnect
else safe to replay
P2->>P4: re-discover the server
P4->>P5: new client and tool state
P2->>P2: revalidate replacement tool and workspace
P2->>P5: replay the same tools call
P5-->>P1: result or stable error
end
Test evidence (unattended CI run — PR's own CI via API, no PR code executed here)The PR's
Not verified: the live side-effect scenario (a real server committing a write before dropping the response connection) — the independent harness cited in the PR body is the author's claim, not independently re-run here; mocked unit tests cannot exercise a real transport dropping mid-response. Windows/macOS behavior rides on the merge-queue matrix, which is the repo's normal posture for PRs. Sandboxed verification would settle the remaining behavioral claim: 中文说明代码审查在干净的 worktree 中审查了 基于代码库（而非仅凭 diff）核实的内容：
一个非阻塞的小建议：不安全重放错误以普通 目前结论： 未发现正确性或规范问题。该改动是所述隐患的最小修复。时序图展示了重连-重放生命周期中的两处门控位置（英文原文见上图）。 测试证据（无人值守 CI —— 通过 API 引用 PR 自身 CI，未在此执行任何 PR 代码）该 commit 的 未验证：真实副作用场景（server 提交写入后断开响应连接）—— PR 正文提到的独立验证脚本是作者的声明，未在此独立复跑；mock 单测无法模拟真实传输中途断开。Windows/macOS 行为依赖合并队列矩阵，这是本仓库对 PR 的常规做法。 沙箱验证可以钉住剩余的行为主张： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — small, focused fix for a hazard I confirmed exists on Stepping back: my independent proposal for this problem and the PR's implementation landed in the same place — gate the retry where the retry decision lives, require explicit safety evidence rather than assuming it, and say "result unknown, verify before retrying" when there is none. Where the PR beat my proposal is the post-reconnect revalidation: I'd have gated the replay once, up front; the PR gates it again against the replacement tool, which closes the stale-trust/annotation inheritance I confirmed is real on The honest reservations, such as they are: there is no field report of a duplicated write (the linked ref is the author's umbrella proposal), and the end-to-end side-effect harness is the author's own. I'm comfortable anyway, because the vulnerable code path is unconditional and mechanical — it doesn't require a rare race, just a server that finishes work before its connection drops — and because every branch of the new decision table is pinned by a test. The availability tradeoff (unannotated tools lose transparent retry) is stated plainly in the PR, documented for users, and matches what the MCP spec's annotation defaults already imply. On the pattern question: this is one of a dozen open PRs from this author this week, and I looked at it on its own merits — it's among the smallest and tightest of the batch, with no scope creep, a design doc, and tests that test the policy rather than the diff. The one follow-up worth tracking is the telemetry nit from Stage 2 (the unsafe-replay error lands as generic CI is settled for this commit — the Linux unit gate is green with the new tests included, and no PR-CI runs are pending — so this approval is immediate rather than deferred. 中文说明置信度：4/5 —— 针对我在 退一步看整体：我对这个问题的独立方案和 PR 的实现落在同一处 —— 在重试决策发生的地方设门控，要求明确的安全证据而不是默认安全，没有证据时返回"结果未知，请先核实再重试"。PR 比我的方案多想了一步的是重连后的再次校验：我原本只会在前面做一次门控，而 PR 对替换工具再查一遍，堵上了我确认在 如实保留的疑虑：没有线上重复写入的事故报告（关联引用是作者自己的伞型提案），端到端副作用验证脚本也出自作者本人。我依然认可，因为有漏洞的代码路径是无条件且机械的 —— 它不需要罕见的竞态，只需要一个在完成工作后断开连接的 server —— 而且新判定表的每个分支都有测试钉住。可用性取舍（无标注工具失去透明重试）在 PR 中写明、在用户文档中说明，也符合 MCP 规范标注默认值的含义。 关于"模式"问题：这是该作者本周十来个 open PR 之一，我按单个 PR 的价值评估 —— 它是其中最小、最紧凑的几个之一，没有范围蔓延，附设计文档，测试测的是策略而不是 diff。唯一值得跟进的是 Stage 2 提到的遥测小问题（不安全重放错误以通用的 该 commit 的 CI 已稳定 —— Linux 单测门禁带新增测试通过，且没有仍在进行的 PR CI 运行 —— 因此本次批准为即时批准而非延迟批准。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Review — gate verified end-to-end; one refinement worth consideringReviewed at
Suggestion: the gate conflates "response lost after send" with "request never sent"
The practical fallout lands on subsequent calls. After one dropped connection, the SDK client detaches its transport and rejects further requests with Two options, either of which keeps the PR's core guarantee intact:
Minor
Not a blocker either way — the duplicate-side-effect hazard this PR closes is real and confirmed on 中文摘要门禁本身验证通过: 主要建议:门禁把"请求已发出、响应丢失"(真不确定)与"请求根本没发出去"(可证明安全)混为一谈。 |
|
@wenshao Thanks — partially agree on the availability concern, but I’m deferring both proposed exemptions from this PR. The current classifier has only error text and connection status; neither is reliable send-phase provenance. The same text can be wrapped by another transport/tool layer, and a DISCONNECTED status can be observed after an ambiguous mid-flight loss. Treating those strings as proof that the server never received the call would either replay an unannotated write or make the stronger “server did not receive this call” claim without evidence, weakening the duplicate-side-effect guarantee this PR is intended to establish. The current “may have completed” wording is intentionally conservative when the client cannot prove otherwise. Recovery for later calls is worth improving separately, preferably by reconnecting without replaying the current invocation or by adding a transport-level sent/not-sent signal. Retaining the original error as a cause is also reasonable follow-up work, but it is not needed for the safety fix. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| if ( | ||
| this.trust !== true || | ||
| this.cliConfig?.isTrustedFolder() !== true || |
There was a problem hiding this comment.
[Suggestion] The trust gate trust === true && isTrustedFolder() is hand-copied here from getDefaultPermission() (line 177 of this file); a third copy lives in read-mcp-resource.ts. The compound gate has already changed once — this PR's own docs hunks catch up to the earlier trust → trust && isTrustedFolder change. Concrete cost: if the gate next gains or loses a condition and one copy is missed, the confirmation-bypass rule and the automatic-replay rule silently diverge — a call auto-confirmed under one trust rule and replayed under another. Suggested fix:
private isTrustedServerInTrustedWorkspace(): boolean {
return this.trust === true && this.cliConfig?.isTrustedFolder() === true;
}Extract one private predicate and call it from both getDefaultPermission() and canSafelyReplay() (the annotation checks stay in canSafelyReplay()).
中文说明
trust gate trust === true && isTrustedFolder() 在这里是从 getDefaultPermission()（本文件第 177 行）手工复制而来；第三份副本在 read-mcp-resource.ts。这个复合 gate 已经变过一次——本 PR 的 docs 改动正是在追赶早前 trust → trust && isTrustedFolder 的变更。具体代价：如果下次 gate 增删条件时漏改某一份副本，免确认规则和自动重放规则会悄悄分叉——一次调用按一种 trust 规则被自动确认，却按另一种规则被重放。建议修复：抽取一个私有谓词（如上），在 getDefaultPermission() 和 canSafelyReplay() 中共同调用（annotation 检查仍留在 canSafelyReplay()）。
— qwen3.8-max via Qwen Code /review (v0.21.4)
There was a problem hiding this comment.
Thanks — deferring. The two checks in this class are currently identical and correct; extracting a helper for two call sites would add indirection without fixing a present bug. The separate resource path has a different owner and is outside this PR.
| if ( | ||
| this.annotations.readOnlyHint === true && | ||
| (this.annotations.destructiveHint === true || |
There was a problem hiding this comment.
[Suggestion] Three documented cells of the canSafelyReplay() annotation matrix are implemented correctly but pinned by no committed test — each verified by mutation: inverting the rule keeps all 82 tests green, while a probe test flips. This location: the tie-break rule "an explicit idempotentHint: true does NOT override contradictory read-only annotations" has no test case combining {readOnlyHint: true, destructiveHint: true, idempotentHint: true}. Failure scenario: hoisting if (this.annotations.idempotentHint === true) return true; above this conflict check makes a contradictory-annotated tool replayable after connection loss — a plausible one-line refactor that no committed test would catch. See the sibling comment on the final return for the two unpinned positive cells. Suggested fix: add a should not replay case with annotations {readOnlyHint: true, destructiveHint: true, idempotentHint: true}.
中文说明
canSafelyReplay() annotation 矩阵中有三个已文档化的分支虽然实现正确，但没有任何已提交测试固定——均经变异验证：反转规则后全部 82 个测试仍然通过，而探针测试可以区分。本位置：tie-break 规则"显式 idempotentHint: true 不能覆盖相互矛盾的只读标注"缺少 {readOnlyHint: true, destructiveHint: true, idempotentHint: true} 的组合用例。失败场景：把 if (this.annotations.idempotentHint === true) return true; 提升到这个冲突检查之上，会使带矛盾标注的工具在断线后变为可重放——这样一个合理的单行重构不会有任何已提交测试能捕获。另外两个未固定的正向分支见 final return 处的姊妹评论。建议修复：新增一个 annotations 为 {readOnlyHint: true, destructiveHint: true, idempotentHint: true} 的 should not replay 用例。
— qwen3.8-max via Qwen Code /review (v0.21.4)
There was a problem hiding this comment.
Thanks — deferring. The implementation correctly rejects this contradictory shape, and the committed suite already pins both conflict rules. This is additional mutation coverage rather than a correctness defect, so I’m avoiding test-only churn after the current review rounds.
| return ( | ||
| this.annotations.idempotentHint === true || | ||
| this.annotations.readOnlyHint === true | ||
| ); |
There was a problem hiding this comment.
[Suggestion] Two more unpinned cells of the canSafelyReplay() matrix at this final return (same pattern as the sibling comment on the conflict check): (1) "an explicit idempotency declaration can cover a mutating operation" — no replay fixture uses {readOnlyHint: false, idempotentHint: true}; the mutant if (this.annotations.readOnlyHint === false) return false; survives all 82 tests. (2) "idempotentHint: true alone qualifies destructive-but-idempotent tools" — no fixture combines {destructiveHint: true, idempotentHint: true}; the mutant if (this.annotations.destructiveHint === true) return false; survives all 82 tests. Failure scenario: MCP spec defaults are readOnlyHint: false / idempotentHint: false, so a careful server annotating an idempotent write publishes exactly {readOnlyHint: false, idempotentHint: true}, and a delete-style tool is {destructiveHint: true, idempotentHint: true} — either mutant would silently strip connection-loss recovery from exactly these carefully-annotated tools. Both verified: mutant applied → suite green and probe fails; original code → probe passes. Suggested fix: add one positive replay case per shape.
中文说明
在这个 final return 处还有 canSafelyReplay() 矩阵的两个未固定分支（与冲突检查处的姊妹评论属于同一模式）：(1)"显式幂等声明可以覆盖变更性操作"——没有任何 replay fixture 使用 {readOnlyHint: false, idempotentHint: true}；变异 if (this.annotations.readOnlyHint === false) return false; 在全部 82 个测试下存活。(2)"仅 idempotentHint: true 即可让破坏性但幂等的工具获得重放资格"——没有任何 fixture 组合 {destructiveHint: true, idempotentHint: true}；变异 if (this.annotations.destructiveHint === true) return false; 在全部 82 个测试下存活。失败场景：MCP 规范默认值为 readOnlyHint: false / idempotentHint: false，因此认真标注幂等写操作的工具发布的正是 {readOnlyHint: false, idempotentHint: true}，删除类工具正是 {destructiveHint: true, idempotentHint: true}——上述任一变异都会悄悄剥夺这些认真标注工具的断线恢复能力。两者均已验证：变异下测试套件全绿且探针失败；原始代码下探针通过。建议修复：为每种形状各加一个正向 replay 用例。
— qwen3.8-max via Qwen Code /review (v0.21.4)
There was a problem hiding this comment.
Thanks — deferring. Both shapes already follow the documented explicit-idempotency rule; the missing cases are additional matrix coverage, not an implementation defect. I’m keeping this safety PR narrow after the current review rounds.
| if (!this.canSafelyReplay()) { | ||
| throw new Error(DiscoveredMCPToolInvocation.UNSAFE_REPLAY_ERROR_MESSAGE); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The no-replay decision is silent: nothing logs which of the three safety gates (server trust, workspace trust, annotations) failed — neither here nor at the post-reconnect revalidation site below. Even with debug logging enabled, only the original transport error is recorded at the top of handleReconnectOnError; the post-reconnect site logs "Successfully reconnected..." immediately followed by "Automatic replay was skipped", with no record that the re-discovered tool lost its trust or annotations — a scenario this PR's own tests declare real. Failure scenario: an admin configures trust: true and the server publishes idempotentHint: true, but the workspace is not trusted (fresh clone never approved, or a headless/CI deployment) — every transient connection loss then fails with the identical generic message, and telling "workspace untrusted" from "server dropped annotations after restart" requires reading the source. Suggested fix: before each throw, debugLogger.warn naming trust, the isTrustedFolder() result, and annotations (and the newInvocation equivalents at the second site).
中文说明
不重放的决定是静默的：没有任何日志记录三个安全 gate（server trust、workspace trust、annotations）中哪一个未通过——此处和下方 reconnect 后的再校验位置都是如此。即使开启 debug 日志，也只在 handleReconnectOnError 开头记录原始 transport 错误；post-reconnect 位置会先记录 "Successfully reconnected..."，紧接着 "Automatic replay was skipped"，却没有记录重新发现的工具丢失的是 trust 还是 annotations——而这正是本 PR 自己的测试声明会发生的场景。失败场景：管理员配置了 trust: true、server 发布了 idempotentHint: true，但 workspace 不可信（新克隆目录未批准，或 headless/CI 部署）——每次临时断线都返回完全相同的笼统错误，要区分"workspace 不可信"还是"server 重启后丢失了 annotations"只能去读源码。建议修复：在每个 throw 前用 debugLogger.warn 输出 trust、isTrustedFolder() 结果和 annotations（第二个位置输出 newInvocation 的对应值）。
— qwen3.8-max via Qwen Code /review (v0.21.4)
There was a problem hiding this comment.
Thanks — won’t take this one. The model-visible error is intentionally stable and sanitized, and logging raw annotations or trust state at both sites would expand the diagnostic surface. A structured, content-free reason code would be a better follow-up if operational telemetry is needed.
| this.mcpToolIdleTimeoutMs, | ||
| this.annotations, | ||
| newTool.annotations, | ||
| newTool['allowInvocationContext'] === true, |
There was a problem hiding this comment.
[Suggestion] Replay revalidation now hinges on newTool.annotations — but annotations are fetched best-effort during (re-)discovery: discoverTools (mcp-client.ts, around lines 1345-1361) wraps a second listTools() round-trip in catch { /* If listTools fails, proceed without annotations — non-critical */ } with only a debug-level log. Reconnection happens exactly when the transport is flapping, so that fetch can fail even though tool registration succeeds; the re-discovered tool then has annotations: undefined, revalidation fails, and the user gets "may have completed... Do not retry automatically" even though reconnect succeeded and the safe annotations were merely lost in transit. Probe-verified: with the annotations fetch rejecting, replay is DENIED after a successful reconnect; with the same setup resolving the annotations, replay proceeds. A second trigger needs no flap: this raw listTools() passes no pagination cursor while the primary genai listing follows nextCursor — on a paginating server, every tool past page one can never be replayed, deterministically. The "non-critical" comment became stale precisely because this diff made the fetch load-bearing. Suggested fix: treat the fetch as replay-critical (retry once before falling back, warn-level log when annotations are dropped, drop the stale comment), and/or pass the pagination cursor through.
中文说明
replay 再校验现在依赖 newTool.annotations——但 annotations 是在（重新）发现期间尽力获取的：discoverTools（mcp-client.ts，约 1345-1361 行）把第二次 listTools() 往返包在 catch { /* If listTools fails, proceed without annotations — non-critical */ } 里，只有 debug 级日志。reconnect 恰恰发生在 transport 抖动的时候，因此即使工具注册成功，这次获取也可能失败；重新发现的工具会带上 annotations: undefined，再校验失败，用户会收到 "may have completed... Do not retry automatically"——即使 reconnect 已经成功、安全标注只是在传输中丢失。已用探针验证：annotations 获取被拒绝时，reconnect 成功后 replay 仍被拒绝；同样配置下若能返回 annotations，replay 正常进行。第二个触发条件无需抖动：这里的原始 listTools() 不传分页 cursor，而 genai 主列表会跟随 nextCursor——在分页的 server 上，第一页之后的所有工具永远无法重放，且是确定性的。"non-critical" 注释正是因为本 diff 让这次获取变得关键而过时。建议修复：把该获取视为 replay 关键路径（降级前重试一次、annotations 被丢弃时输出 warn 级日志、删除过时注释），和/或把分页 cursor 传进去。
— qwen3.8-max via Qwen Code /review (v0.21.4)
There was a problem hiding this comment.
Thanks — deferring. The availability concern and pagination gap are valid, but fail-closed behavior is required when the re-discovered tool’s annotations cannot be verified. Fixing paginated, bounded annotation retrieval belongs in the shared discovery path as a follow-up; this PR must not carry forward or assume stale annotations.
Maintainer verification — real local stack (macOS)I verified this PR end-to-end against a real MCP stdio server driven by the real HarnessA test MCP server exposes Result matrix (side effects actually executed on the server)
In every blocked case the model-facing tool result is exactly the intended stable text, with no arguments and no upstream transport error leaked: Case I raw evidence (after): Screenshots (interactive TUI, real terminal via tmux)Before — unannotated tool, connection lost: the write ran twice and the user sees a green ✓ with
After — same tool, same fixture: replay is skipped, the write ran once, and the user is told to verify manually.
After — trusted server +
After — eligible call whose replacement tool loses its annotations: reconnect succeeds, replay is still refused.
Repository checks (on PR head)
Notes for reviewers (non-blocking)
Verdict: LGTM. The reported duplicate-side-effect bug reproduces on the baseline and is fixed here, eligible replay is preserved, and no regression appeared in the normal MCP call path. 中文版本维护者验证 —— 本地真实环境（macOS）我用真实 MCP stdio server + 真实 验证方法测试 MCP server 提供 结果矩阵（服务端实际执行的副作用次数）
所有被拦截的场景中，返回给模型的工具结果就是预期的稳定文案，不泄露参数、也不泄露上游传输错误： 案例 I 原始证据（修改后）： 截图（tmux 真实终端下的交互式 TUI）修改前 —— 无 annotations 工具断线：写操作执行了两次，用户看到的是绿色 ✓ 和 
修改后 —— 同样的工具、同样的 fixture：跳过重放，写操作只执行一次，并提示用户手动核实。 
修改后 —— 可信 server + 
修改后 —— 初始符合条件、但替换后的工具丢失 annotations：reconnect 成功，重放依旧被拒绝。 
仓库检查（基于 PR head）
给评审者的备注（不阻塞合并）
结论：LGTM。 基线上能复现重复副作用问题，本 PR 修复了它，符合条件的重放行为得以保留，正常 MCP 调用路径未见回归。 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — security-conservative change, no bypass found. Verified: post-reconnect re-gate uses newTool.trust/annotations (no stale-trust inheritance), replay requires trust+workspace+explicit-safe (same condition as initial auto-approve, so replay never skips a confirmation the first call required), no alternate replay paths exist (retryWithBackoff only wraps control-plane ops), annotation semantics correct vs MCP spec defaults (missing={}⇒deny, conflicts⇒deny, readOnly⇒safe).
Non-blocking:
- (P2) shouldAttemptReconnect short-circuits true on DISCONNECTED status regardless of error type — a non-connection error in that race window shows the side-effect warning even though the call may never have been sent. Consider keeping the original error text when the message doesn't match connection patterns.
- (P3) isTrustedFolder() fails open (default true) — design doc's 'two trust gates' framing overstates the workspace factor. The server trust opt-in is the controlling gate.
- (P3) Unsafe-replay denial is a bare Error (UNHANDLED_EXCEPTION) with no debug log of which condition failed. Consider debugLogger.info the denial reason.
- (P3) Test matrix gaps: empty {} annotations, destructiveHint alone, trust: undefined, post-reconnect conflicting annotations. All handled correctly by the gate, just untested.
…felyReplay gate The test 'reconnects instead of reporting a timeout when the server is known disconnected' was written before #8387 added the canSafelyReplay() safety gate. The gate requires trust === true, isTrustedFolder() === true, and annotations with idempotentHint === true or readOnlyHint === true. The test fixture now passes trust: true and { idempotentHint: true } to both the initial tool and the reconnected tool, so the reconnect path is exercised as intended instead of hitting UNSAFE_REPLAY_ERROR. Fixes the 1 test failure blocking the v0.21.5 release (19158 pass / 1 fail).
Brings in QwenLM#8478, which aligns the MCP reconnect-timeout test with the safe-replay policy already on this branch (QwenLM#8387) — the stale fixture is what fails the Test check on this PR's head.
|
Released in v0.21.5. |
4 participants
What this PR does
This PR makes MCP connection-loss replay conservative. Qwen Code now reconnects and replays the current tool invocation only when the server has
trust: true, the workspace is trusted, and the tool explicitly declaresidempotentHint: trueor a consistent read-only annotation. Missing annotations, explicit non-idempotency, conflicting read-only annotations, and either failed trust gate return a stable unknown-result error without reconnecting or constructing a second invocation.After reconnecting, Qwen Code applies the same checks to the newly discovered tool and the current workspace before sending the replay. A restarted or upgraded server cannot inherit stale trust or annotations from the previous process. The policy is shared by the direct MCP client and callable fallback. Existing bounded replay remains available for trusted, explicitly safe tools, while Abort errors, non-connection errors, and MCP
isError: trueprotocol results retain their prior behavior. The MCP documentation and CLI help now consistently describe the server-plus-workspace trust boundary.Why it's needed
An MCP server can finish a side effect and then lose the response connection. The previous unconditional reconnect path sent the same
tools/callagain even when annotations were absent or explicitly non-idempotent, so one user-approved invocation could perform the write multiple times while only the final response was visible. MCP annotations default conservatively and are only trustworthy for a trusted server, so automatic replay must require both trust gates and an explicit safe declaration. Those conditions must be checked again after discovery because the replacement server process or workspace trust state may have changed during reconnect.Reviewer Test Plan
How to verify
Use an MCP test server whose tool increments a counter before dropping the first response connection. With missing annotations or
idempotentHint: false, confirm the server receives one invocation, Qwen Code does not rediscover or reconnect for that invocation, and the returned error says the result may be unknown without exposing arguments or the upstream error. Then configuretrust: true, use a trusted workspace, and declareidempotentHint: true; confirm the existing reconnect path invokes the tool again and returns the successful result. A consistentreadOnlyHint: truetool should also replay, while conflicting annotations and either failed trust gate should not.For reconnect revalidation, start with an eligible call and make the newly discovered tool omit annotations or become untrusted, or change the workspace to untrusted before replay. Discovery should complete, but the replacement client must receive zero tool calls and Qwen Code must return the same stable unknown-result error.
The focused MCP tool suite covers both execution paths, safe annotations, missing and conflicting annotations, both trust gates before and after reconnect, connection error patterns, Abort, non-connection errors, protocol errors, reconnect failure, and the maximum retry path. The MCP CLI suite covers the adjacent help-text change. Repository build, typecheck, lint,
git diff --check, and an independent side-effect-after-disconnect harness were also completed.Evidence (Before & After)
Before: an unannotated or explicitly non-idempotent test tool committed its side effect, lost the response, was rediscovered, and received the same invocation again; the server counter reached
2and the user saw only the second successful result. A replacement tool could also inherit the previous process's safe annotations and trust for the replay decision.After: unsafe scenarios reach the server exactly once with zero replay and return
MCP tool execution may have completed before the connection failed. Automatic replay was skipped because the call could not be verified as safe to replay. Do not retry automatically; verify the outcome before trying again.Re-discovery may restore the connection for an initially eligible call, but replay is still blocked if the replacement tool or current workspace no longer passes policy. Trusted tools with explicit safe annotations retain bounded replay.Tested on
Environment (optional)
macOS, Node.js v24.12.0, npm 10.9.8, local sandbox-none workspace.
Risk & Scope
idempotentHintor consistent read-only annotations, and administrators must enable server trust in a trusted workspace. Manual verification is required before retrying an unsafe call whose response connection was lost.Linked Issues
Refs #7585
中文说明
本 PR 做了什么
本 PR 将 MCP 断线重放改为保守策略。只有当 server 配置了
trust: true、当前 workspace 可信，并且工具明确声明idempotentHint: true或一致的只读标注时，Qwen Code 才会重新连接并重放当前工具调用。annotations 缺失、明确非幂等、只读标注冲突，或任一 trust gate 未通过时，当前调用会返回稳定的结果未知错误，不会 reconnect，也不会创建第二个 invocation。reconnect 完成后，Qwen Code 会在发送 replay 前再次检查重新发现的工具和当前 workspace。重启或升级后的 server 不能继承旧进程的 trust 或 annotations。该策略由 direct MCP client 和 callable fallback 共用。可信且明确安全的工具继续保留现有有界重放；Abort、非连接错误和 MCP
isError: true协议结果维持原有行为。MCP 文档和 CLI 帮助也统一为 server 与 workspace 双重 trust gate 的真实语义。为什么需要
MCP server 可能已经完成副作用，但在返回响应时连接丢失。此前无条件 reconnect 的路径会在 annotations 缺失或明确非幂等时再次发送相同的
tools/call，因此一次经用户批准的调用可能执行多次写入，而用户只会看到最后一次响应。MCP annotations 的默认语义是保守的，并且只有可信 server 提供的标注才可依赖，因此自动重放必须同时要求双重 trust gate 和明确的安全声明。由于替换后的 server 进程或 workspace trust 状态可能在 reconnect 期间发生变化，这些条件还必须在 discovery 后再次校验。Reviewer 测试计划
如何验证
使用一个在第一次响应断开前先递增计数器的 MCP 测试工具。annotations 缺失或
idempotentHint: false时，确认 server 只收到一次 invocation，Qwen Code 不会为当前 invocation 重新发现或重连，并且返回的错误说明结果可能未知，同时不泄露参数或上游错误。然后配置trust: true、使用可信 workspace，并声明idempotentHint: true；确认现有 reconnect 路径会再次调用工具并返回成功结果。一致的readOnlyHint: true工具也应允许重放，而冲突标注或任一 trust gate 失败都不应重放。对于 reconnect 后的再次校验，从一个符合重放条件的调用开始，让重新发现的工具不再提供 annotations、变为不可信，或在 replay 前将 workspace 改为不可信。discovery 可以完成，但替换后的 client 必须收到零次工具调用，Qwen Code 必须返回相同的稳定结果未知错误。
定向 MCP 工具测试覆盖两条执行路径、安全标注、缺失与冲突标注、reconnect 前后的双重 trust gate、连接错误模式、Abort、非连接错误、协议错误、重连失败和最大重试路径。MCP CLI 定向测试覆盖相邻的帮助文案修正。此外还完成了仓库 build、typecheck、lint、
git diff --check，以及独立的“副作用完成后断线”验证脚本。证据（修改前后）
修改前：无 annotations 或明确非幂等的测试工具先提交副作用，再丢失响应，随后被重新发现并收到完全相同的 invocation；server 计数器变为
2，用户只看到第二次调用的成功结果。替换后的工具还可能在 replay 判断中继承旧进程的安全标注与 trust。修改后：不安全场景只到达 server 一次，replay 为零，并返回
MCP tool execution may have completed before the connection failed. Automatic replay was skipped because the call could not be verified as safe to replay. Do not retry automatically; verify the outcome before trying again.。对于最初符合条件的调用，re-discovery 可以恢复连接；但如果替换后的工具或当前 workspace 不再满足策略，replay 仍会被阻止。可信且明确声明安全的工具继续保留有界重放。测试平台
环境（可选）
macOS、Node.js v24.12.0、npm 10.9.8、本地 sandbox-none workspace。
风险与范围
idempotentHint或一致只读标注，管理员还必须在可信 workspace 中启用 server trust。对于响应连接丢失的不安全调用，手动重试前必须先核实实际结果。关联 Issue
Refs #7585