fix(core): a never-delivered MCP call is a first delivery, not a replay - #8482
fix(core): a never-delivered MCP call is a first delivery, not a replay#8482wenshao wants to merge 12 commits into
Conversation
The replay-safety gate (QwenLM#8387) refuses to auto-replay a call that may have completed before its connection failed, unless the tool's annotations prove idempotency. Landed, it also refused the case its own premise excludes: a call issued while the server was already known DISCONNECTED never reached the server, so nothing 'may have completed' — retrying it is the first delivery of the call, and refusing that turns every dead transport into a permanent error for every unannotated tool. The existing reconnect test asserts exactly this behaviour and has been red on main since the gate merged, failing the required Test job on every full-profile PR behind it. The invocation now snapshots the server status before issuing its call; the gate is skipped only when that snapshot says DISCONNECTED. The fresh post-reconnect invocation snapshots its own status, so a call that dies mid-flight on the recovered connection faces the gate with no carve-out on the next hop. All QwenLM#8387 refusal cases are unchanged: 91/91 in mcp-tool.test.ts.
|
@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，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Re-run after the redesign — the PR has moved a long way since the first pass, so this updates all three staged comments against the current head.
Moving on to code review. 🔍 中文说明重设计后的重跑——自首轮 triage 以来 PR 变化很大，本次按当前 head 更新全部三条阶段评论。
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent proposal first, before the diff: given the Critical finding (a What I verified reading the diff against current
Two scenarios I considered and put down as non-issues: a malicious trusted server answering a delivered call with a crafted JSON-RPC error message of exactly Everything raised in earlier rounds is resolved in this head: the Critical (R1-2) by the redesign itself; R2-1 (unversioned SDK string) and R3-1 (unpinned exact-match boundary) by the real-SDK and boundary tests; and both of my first-pass nits — the status read is gone entirely, and the carve-out now has tests of its own, so the merged suite pins it regardless of #8478's annotated fixture. TestingUnattended CI run — no PR code was built or executed here; the evidence below is the PR's own CI read through the API at the reviewed commit, plus the sandboxed verification running in parallel.
Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 中文说明代码审查：先独立给出方案再读 diff。既然 Critical 发现已证明 已核实： 两个考虑后判定不构成问题的场景：恶意 trusted 服务器对已投递的调用回以措辞恰为 此前各轮的全部发现均已在当前 head 解决：Critical（R1-2）由重设计本身解决；R2-1（无版本锚定的 SDK 字符串）与 R3-1（未锚定的精确匹配边界）由真实 SDK 测试与边界测试解决；我首轮的两条 nit 也已解决——状态读取彻底移除，豁免现有自己的测试，合入后的测试套件无论如何都会锚定它（不依赖 #8478 的带注解夹具）。 测试：无人值守 CI 运行——未构建或执行任何 PR 代码；以上证据为通过 API 读取的该 PR 自身 CI，以及并行运行的沙箱验证。必需的 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean redesign that answers the Critical finding on its own terms, with genuinely good tests; the one real nit is that the PR description still documents the abandoned design, and CI hasn't landed on this head yet. Stepping back: the arc of this PR is the review process working as intended. The original snapshot carve-out was the obvious fix and was wrong in exactly the subtle way the Critical finding and @wenshao's TOCTOU analysis both nailed down — a status map is a belief about the world, not evidence about a specific call. The redesign takes the only honest exit: key the carve-out on the one client-side event that constitutes non-delivery (the SDK rejecting before it writes), treat everything else as ambiguous, and fail closed. My independent proposal for this round was precisely that mechanism, and I couldn't find a simpler one. The implementation is 41 production lines with no drive-by edits, and the test suite is better than the fix: mutation-killed exact-match boundaries, named reproductions of both scenarios that sank the old design, and both directions of the carve-out pinned against the real SDK rather than a mock's opinion of it. Six months from now, this reads as a careful, self-contained gate refinement — not as debt. The standing The 4 and not 5, plainly named:
Approval deferred until CI lands green on 中文说明置信度：4/5 —— 干净的重设计，以 Critical 发现自身认可的方式回应了它，测试质量真正出色；唯一的实质 nit 是 PR 描述仍在记载已被放弃的设计，且 CI 尚未在当前 head 上落定。 退一步看：这个 PR 的演进正是审查流程按预期工作的样本。最初的快照豁免是显而易见的修复，也恰好以 Critical 发现和 @wenshao 的 TOCTOU 分析共同指出的那种微妙方式错了——状态表是对世界的认知，不是关于某次具体调用的证据。重设计选择了唯一诚实的出口：豁免锚定在唯一构成未投递的客户端事件上（SDK 在写入前拒绝），其余一切视为歧义，失败关闭。我本轮的独立方案正是这一机制，且没有找到更简的方案。实现为 41 行生产代码、无任何顺手改动；测试比修复本身更好：突变杀灭的精确匹配边界、按名复现击沉旧设计的两个场景、豁免的两个方向都锚定在真实 SDK 而非 mock 的看法上。六个月后回看，这是一次谨慎、自洽的闸门精化——不是债务。 本 PR 上悬挂的 给 4 而非 5，直说两点：
审批推迟至 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
|
🔀 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: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action neededNo new review feedback was found for this round:
No code changes were made, and no commit was created. The PR head remains unchanged. 中文说明Autofix 审查轮次 —— 无需处理本轮未发现新的审查反馈：
未做任何代码修改，也未创建任何提交。PR 的 head 分支保持不变。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
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.
中文说明
已审查。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
— qwen3.7-max via Qwen Code /review (v0.21.4)
| const neverDelivered = | ||
| this.statusAtCallStart === MCPServerStatus.DISCONNECTED; |
There was a problem hiding this comment.
[Suggestion] Replay safety gate bypass is silent — no diagnostic logging when the neverDelivered carve-out fires.
Failure scenario: An oncall engineer investigating suspected double-execution of a non-idempotent MCP tool sees the same log output whether the replay gate was satisfied via canSafelyReplay() or bypassed via neverDelivered. The two code paths produce identical log output, making incident investigation require local reproduction with temporary logging.
| const neverDelivered = | |
| this.statusAtCallStart === MCPServerStatus.DISCONNECTED; | |
| const neverDelivered = | |
| this.statusAtCallStart === MCPServerStatus.DISCONNECTED; | |
| if (neverDelivered) { | |
| debugLogger.info(`Replay safety gate bypassed for '${this.serverName}': call was never delivered (status at call start: ${this.statusAtCallStart})`); | |
| } |
中文说明
[Suggestion] 重放安全闸门绕过时无诊断日志 — neverDelivered 豁免触发时没有记录。
失败场景：值班工程师调查疑似非幂等 MCP 工具双重执行时，无论重放闸门是通过 canSafelyReplay() 满足还是通过 neverDelivered 绕过，日志输出完全相同。两条代码路径产生相同的日志输出，使得事故调查需要在本地添加临时日志复现。
— qwen3.7-max via Qwen Code /review (v0.21.4)
ReviewVerdict: the behavior change is correct and worth keeping — but as of the current head it ships with zero test coverage, and the PR description's justification is stale. Details and evidence below. What this PR does
The reasoning is sound, the snapshot-before-the-call ordering is the right mechanism (a gate-time read is useless — the failure itself overwrites the status), and Verification I ranWorktree at head
That second row is the finding. 1. 🔴 The change is untested — mutation-verifiedDeleting the entire carve-out from this PR leaves the suite green. The reason: #8478 ("align MCP reconnect timeout test with safe replay policy", merged 2026-08-03T17:26Z) rewrote the fixtures of I wrote two probes that do cover it. Both pass on the head and both fail with the carve-out removed: Probe A + B (fail without the fix, pass with it)// A: an UNANNOTATED, untrusted tool on a known-disconnected server must
// reconnect and deliver (this is the user-visible bug being fixed).
it('unannotated tool on a disconnected server reconnects and delivers', async () => {
const deadClient = { callTool: vi.fn().mockRejectedValueOnce(new Error('Connection closed')) };
const liveClient = { callTool: vi.fn().mockResolvedValueOnce({ content: [{ type: 'text', text: 'OK' }] }) };
const newTool = new DiscoveredMCPTool(
mockCallableToolInstance, serverName, serverToolName, baseDescription, inputSchema,
undefined /* untrusted */, undefined, undefined, liveClient /* no annotations */,
);
const discoverToolsForServer = vi.fn().mockResolvedValue(undefined);
const mockConfig = {
isTrustedFolder: () => true,
getToolRegistry: () => ({ discoverToolsForServer, ensureTool: vi.fn().mockResolvedValue(newTool) }),
getTruncateToolOutputThreshold: () => 0,
getTruncateToolOutputLines: () => 0,
};
updateMCPServerStatus(serverName, MCPServerStatus.DISCONNECTED);
const tool = new DiscoveredMCPTool(
mockCallableToolInstance, serverName, serverToolName, baseDescription, inputSchema,
undefined, undefined, mockConfig as any, deadClient,
);
const result = await tool.build({ param: 'x' }).execute(new AbortController().signal);
expect(discoverToolsForServer).toHaveBeenCalled();
expect(liveClient.callTool).toHaveBeenCalledTimes(1);
expect(result.llmContent).toEqual([{ text: 'OK' }]);
});
// B: the no-chaining claim — once the reconnect restores CONNECTED, a call
// that dies mid-flight on the RECOVERED connection faces the full gate.
it('mid-flight death on the recovered connection is still gated', async () => {
const deadClient = { callTool: vi.fn().mockRejectedValueOnce(new Error('Connection closed')) };
const recoveredClient = { callTool: vi.fn().mockRejectedValue(new Error('Connection closed')) };
const thirdClient = { callTool: vi.fn().mockResolvedValue({ content: [{ type: 'text', text: 'REPLAYED' }] }) };
const mk = (c: any) => new DiscoveredMCPTool(
mockCallableToolInstance, serverName, serverToolName, baseDescription, inputSchema,
undefined, undefined, undefined, c,
);
const ensureTool = vi.fn().mockResolvedValueOnce(mk(recoveredClient)).mockResolvedValue(mk(thirdClient));
// a real reconnect flips the global status back to CONNECTED
const discoverToolsForServer = vi.fn().mockImplementation(async () => {
updateMCPServerStatus(serverName, MCPServerStatus.CONNECTED);
});
const mockConfig = {
isTrustedFolder: () => true,
getToolRegistry: () => ({ discoverToolsForServer, ensureTool }),
getTruncateToolOutputThreshold: () => 0,
getTruncateToolOutputLines: () => 0,
};
updateMCPServerStatus(serverName, MCPServerStatus.DISCONNECTED);
const tool = new DiscoveredMCPTool(
mockCallableToolInstance, serverName, serverToolName, baseDescription, inputSchema,
undefined, undefined, mockConfig as any, deadClient,
);
await expect(
tool.build({ param: 'x' }).execute(new AbortController().signal),
).rejects.toThrow(/may have completed before the connection failed/);
expect(recoveredClient.callTool).toHaveBeenCalledTimes(1);
expect(thirdClient.callTool).not.toHaveBeenCalled();
});Result with the fix: both pass (94/94 with the file). Result with Probe B is the good news: the no-chaining property the PR claims actually holds — the fresh invocation does snapshot its own pre-call status and the second hop is gated. That's the subtlest part of the change and it works; it just isn't asserted anywhere. 2. 🟡 The PR description no longer matches reality
Worth rewriting so the merge rationale is the product bug (unannotated MCP tools can never auto-recover from a dead transport) rather than a CI-unblock that already happened. It also changes the relationship to #8478: that PR adjusted the test to the gate, this one adjusts the gate — reviewers should see both framed together. 3. 🟡 Residual risk:
|
|
Status note: main has since resolved the red test differently — #8478 rewrote the test to accept the gate's refusal ( The queue is unblocked either way, so this is now a pure design decision rather than a firefight:
Leaving the call to the maintainers; happy to do the rebase if the first option is preferred. — via Claude Code (claude-fable-5) |
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #8482Commit: Feedback dispositions1. 🔴 The change is untested — mutation-verified (@wenshao) — ADDRESSED.
Mutation check reproduces the reviewer's finding and verifies the fix: with both 2. [Suggestion] rc:3707652692 — silent gate bypass (@doudouOUC / automated reviewer) — ADDRESSED. 3. Nit — three comment blocks restate the same rationale (@wenshao) — ADDRESSED. 4. Item 3 sub-suggestion — say believed-never-delivered (@wenshao) — ADDRESSED. 5. Nit — thread 6. Item 2 — PR description is stale (@wenshao) — DEFERRED (requires a GitHub write this mode does not perform). 7. Item 3 residual risk windows + Item 4 pooled servers (@wenshao) — DEFERRED to follow-up. 8. Status note — merge vs. close (@wenshao) — MAINTAINER DECISION, left open. No conflicts ( Verification
中文说明Autofix 审查轮次 — PR #8482提交： 反馈处理1. 🔴 改动无测试覆盖（突变测试验证）(@wenshao) — 已处理。
突变验证复现了审查者的发现并确认修复有效：删掉两处 2. [Suggestion] rc:3707652692 — 闸门绕过无日志 (@doudouOUC / 自动审查器) — 已处理。 3. 小问题 — 三处注释块重复同一理由 (@wenshao) — 已处理。 4. 第 2 条子建议 — 措辞改为"推定未投递" (@wenshao) — 已处理。 5. 小问题 — 把 6. 第 2 条 — PR 描述过期 (@wenshao) — 暂缓（需要本模式不执行的 GitHub 写操作）。 7. 第 3 条残余风险窗口 + 第 4 条池化服务器 (@wenshao) — 暂缓，转后续跟进。 8. 状态说明 — 合入还是关闭 (@wenshao) — 维护者决策，保持开放。 无冲突（ 验证
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/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] R1-2 packages/core/src/tools/mcp-tool.ts:389-391 (snapshot at :486) — the neverDelivered carve-out treats a DISCONNECTED status snapshot as proof the call never reached the server, but for streamable-HTTP/SSE servers the pinned MCP SDK (1.30.0) fires onerror on transient errors (SSE stream drop, POST failures such as a proxy 502/ECONNRESET) WITHOUT closing the transport — subsequent calls are still delivered, and nothing restores CONNECTED until a reconnect (~95s health-monitor window in non-pool mode, the standard interactive CLI path). Probe-verified double execution: driving the real shipped invocation with a delivered-then-failed call under a DISCONNECTED snapshot is automatically replayed (deliveries=2); with the carve-out removed the identical input throws UNSAFE_REPLAY (deliveries=1). The snapshot also keys on getMCPServerStatus(), which falls back to DISCONNECTED for servers with no registry entry — the exact comparison isExecutionTimeoutFailure() in this same file documents as unsafe and deliberately avoids. This re-opens the double-execution class #8387's gate closed, for unannotated (possibly non-idempotent) tools. Fix direction: require positive evidence of non-delivery — at minimum snapshot only a recorded status (this.statusAtCallStart = getAllMCPServerStatuses().get(this.serverName); fails closed for unregistered servers), and tie the bypass to delivery-level evidence (a local send-side rejection proving the call never left the client) rather than the global status map; network-level errors say nothing about whether the server received the request. (Relocated from inline: its anchor line overlaps an existing comment thread at mcp-tool.ts:391, which is a different — already addressed — finding.)
— qwen3.8-max via Qwen Code /review (v0.21.5)
| signal: AbortSignal, | ||
| updateOutput?: (output: ToolResultDisplay) => void, | ||
| ): Promise<ToolResult> { | ||
| this.statusAtCallStart = getMCPServerStatus(this.serverName); |
There was a problem hiding this comment.
[Suggestion] No test pins the call-start snapshot timing — a gate-time live status read survives the entire 93-test suite (probe-verified: the mutant const neverDelivered = getMCPServerStatus(this.serverName) === MCPServerStatus.DISCONNECTED evaluated in handleReconnectOnError was applied and the suite run — 93/93 still pass). No failing mock in this file flips the server status on failure the way the real client's onerror wiring does (mcp-client.ts marks the server DISCONNECTED when the transport dies), so a future refactor collapsing statusAtCallStart into a gate-time read would ship green with the safety gate neutered for its core scenario: a call issued CONNECTED that dies mid-flight — exactly the case #8387's gate was built for. — Concrete cost: the snapshot's "before the attempt" invariant (which the adjacent comment calls load-bearing) is protected only by code review; the suite cannot see it. Add a test that discriminates: status CONNECTED before execute(); the failing callTool mock flips status to DISCONNECTED before rejecting; unannotated tool; expect unsafeReplayErrorMessage. Verified: the proposed test passes on shipped code and fails on the mutant (which replays the may-have-completed call instead of gating it).
const deadClient: McpDirectClient = {
callTool: vi.fn().mockImplementation(async () => {
// The real client marks the server DISCONNECTED when the transport
// dies mid-flight; the snapshot at call start must decide, not this.
updateMCPServerStatus(serverName, MCPServerStatus.DISCONNECTED);
throw new Error('Connection closed');
}),
};
// untrusted/unannotated tool; updateMCPServerStatus(serverName, CONNECTED) before execute()
await expect(
reconnectTool.build(params).execute(new AbortController().signal),
).rejects.toThrow(unsafeReplayErrorMessage);中文说明
[建议] 没有测试钉住「调用发起前快照」的时机——在闸门处实时读取状态的变体实现可以通过全部 93 个测试（已用探针验证：把 mutant const neverDelivered = getMCPServerStatus(this.serverName) === MCPServerStatus.DISCONNECTED 挪到 handleReconnectOnError 里实时求值后跑整套测试，仍然 93/93 通过）。本文件中没有任何失败 mock 会在失败时翻转服务器状态，而真实客户端的 onerror 接线会（transport 死掉时 mcp-client.ts 会把状态写成 DISCONNECTED）。因此未来若有重构把 statusAtCallStart 坍缩为闸门处实时读取，测试会全绿合入，而安全闸门在其核心场景（调用发起时 CONNECTED、中途死掉——正是 #8387 闸门要防的情形）里已形同虚设。——具体代价：快照「先于尝试」的不变量（旁边注释称其为关键选择）只靠代码评审保护，测试套件无法察觉。建议补一个能区分的测试：execute() 前状态为 CONNECTED；失败的 callTool mock 在 reject 前把状态翻成 DISCONNECTED；无注解工具；断言抛出 unsafeReplayErrorMessage。已验证：该测试在现有代码上通过，在上述 mutant 上失败（mutant 会重放可能已执行的调用而不是拦截它）。
— qwen3.8-max via Qwen Code /review (v0.21.5)
Review —
|
| Run | Result |
|---|---|
| PR head, full file | 93/93 pass |
PR head with mcp-tool.ts reverted to main (mutation) |
2 failed — exactly the two new tests |
main (d6f55a1c), unmodified file |
91/91 pass |
eslint --max-warnings 0 on both changed files |
clean |
So the fix is necessary for its own tests, and both new tests are load-bearing (the second one fails on base because the gate short-circuits before recoveredClient is ever called).
1. The stated motivation is stale — main is already green
The description says the reconnect test "has been red on main since #8387" and is blocking #8468/#8471. That was true, but #8478 (e68f617b, "align MCP reconnect timeout test with safe replay policy") already landed on main and fixed it by giving the fixture server trust + idempotentAnnotations. I ran the file at d6f55a1c (current main): 91/91 green.
That changes what this PR is. It is no longer a CI unblock; it is a deliberate loosening of #8387's policy, and it should be reviewed and described as one. Please rewrite the summary accordingly — as written, a reviewer is being asked to approve a behavior change under the framing of an urgent red-CI fix. (It also means there's no rush, which is good, because of #2.)
2. Correctness: the status snapshot is a TOCTOU proxy for "never delivered"
McpClient.disconnect() writes the global DISCONNECTED before it closes the transport:
// packages/core/src/tools/mcp-client.ts:628-635
this.status = MCPServerStatus.DISCONNECTED;
updateMCPServerStatus(this.serverName, MCPServerStatus.DISCONNECTED);
this.isDisconnecting = true;
if (this.transport) {
await this.transport.close(); // <-- transport is still live during this await
}McpPoolEntry.forceShutdown() / doRestart() reach the same code via sweepAndDisconnect(), and the pid sweep + SIGTERM runs inside that window too. So there is a real interval in which the global map says DISCONNECTED while the SDK client still has a transport — a concurrent tool call issued in that window snapshots DISCONNECTED, is actually written to the server, may execute its side effect, then fails when the transport dies, and this PR replays it. That is precisely the double-execution #8387 exists to prevent. Idle-reaper sweeps, /mcp disable and pool restarts all run concurrently with tool calls, so the window is reachable, not theoretical.
Suggested fix — use a positive signal instead of an inferred one. The MCP SDK gives you exactly that: Protocol.request() (what client.callTool goes through) rejects before writing anything when the transport is gone:
// @modelcontextprotocol/sdk/dist/esm/shared/protocol.js:618-621
if (!this._transport) {
earlyReject(new Error('Not connected'));
return;
}Not connected is proof the call never left the process; Connection closed / ECONNRESET are not. Keying the carve-out on the error identity (or on error identity and the status snapshot) closes the race by construction and makes the invariant self-evident rather than dependent on teardown ordering in two other files. If you keep the status-based version, please at least add a comment in mcp-client.ts:628 warning that the write ordering there is now load-bearing for replay safety.
3. The tests assert the carve-out with the error that doesn't prove it
Both new tests give the dead client new Error('Connection closed') — the ambiguous case. A genuinely never-delivered call throws Not connected. As written, the tests document the implementation (status-only) rather than the property the PR title claims ("never delivered"), and they'd pass unchanged even if the call had been delivered. Switching deadClient to reject with Not connected would make the fixture faithful; if you adopt #2 it becomes the actual mechanism.
4. Minor
- Unknown server also reads as
DISCONNECTED.getMCPServerStatusisserverStatuses.get(name) || DISCONNECTED(mcp-status.ts:92), so the carve-out fires for "no status entry at all", not just "known disconnected" — e.g. afterremoveMCPServerStatus()on a/mcp-disabled server. Tool removal is supposed to be paired with those calls so it's hard to hit today, but the doc comment onstatusAtCallStartsays "already knownDISCONNECTED", which is stronger than what the code checks. Either tighten the check or soften the comment. statusAtCallStartis a mutable field written insideexecute(). Nothing else in this class carries per-execution state, and a re-entrant/reused invocation would clobber it. Threading the snapshot as a parameter (execute→executeWith*Client→handleReconnectOnError(error, statusAtCallStart, …)) keeps its lifetime obvious and makes the field unnecessary.- Test 1 can't distinguish the two carve-out sites. Its
discoverToolsForServerismockResolvedValue(undefined), so it never restoresCONNECTED— the post-reconnect invocation re-snapshotsDISCONNECTEDand would be exempt on its own merits. Have it flip toCONNECTED(like test 2 does) and the test then genuinely pins the outerneverDeliveredpropagation at line 426. - The
debugLogger.infoon bypass is good; consider also logging when the gate fires, so the two branches are symmetric in a support log.
5. Worth deciding explicitly
After an UNSAFE_REPLAY error the model is told "verify the outcome before trying again". With this carve-out, the model's next attempt at the same tool now silently succeeds via reconnect — correct per the "first delivery" argument, but it means the guard degrades to a one-turn speed bump for unannotated tools. That's a reasonable product call; it just isn't stated anywhere in the PR or the code comments, and it's the thing a future reader will need.
Verdict: the direction is right and the implementation is clean, but I'd hold on merging until (a) the description is re-framed now that #8478 made main green, and (b) #2 is addressed or explicitly accepted — the status snapshot narrows #8387's guarantee from "always" to "except during teardown races", and the SDK already hands you a race-free signal.
中文小结
结论:方向正确,实现干净,但建议先处理两点再合入。
本地验证(worktree 967f4741):PR head 93/93 通过;把 mcp-tool.ts 回退到 main 后恰好挂掉新增的 2 个测试(变异测试成立);eslint --max-warnings 0 无告警。
- 动机已过期。 fix(core): align MCP reconnect timeout test with safe replay policy #8478(
e68f617b)已经先落地并修好了那个红灯测试——我在 main(d6f55a1c)上实跑该文件是 91/91 全绿。所以本 PR 不再是"解 CI 阻塞",而是对 fix(core): Avoid replaying unsafe MCP tool calls #8387 策略的一次主动放宽,描述需要按这个定位重写。 - 状态快照只是"未投递"的近似,存在 TOCTOU。
McpClient.disconnect()在await this.transport.close()之前就写了全局DISCONNECTED(mcp-client.ts:628-635),forceShutdown/doRestart经sweepAndDisconnect走的是同一条路。在这个窗口里发出的调用会快照到DISCONNECTED、实际写到了服务端、副作用可能已执行,然后随 transport 一起失败并被本 PR 重放——正是 fix(core): Avoid replaying unsafe MCP tool calls #8387 要防的重复执行。空闲回收、/mcp禁用、连接池重启都会与工具调用并发,窗口是可达的。
建议改用正向信号:MCP SDK 的Protocol.request()在 transport 缺失时在写出任何字节之前就earlyReject(new Error('Not connected'))(protocol.js:618-621)。Not connected才是"从未投递"的证据,Connection closed不是。按错误身份判定可以从构造上消除竞态。 - 测试用的错误恰好是不能证明命题的那个。 两个新测试的 deadClient 抛的是
Connection closed(歧义态);真正未投递抛的是Not connected。改成后者 fixture 才忠实于 PR 标题的命题。 - 细节:
getMCPServerStatus对未知 server 也返回DISCONNECTED(mcp-status.ts:92),豁免范围比注释里写的"已知 DISCONNECTED"更宽;statusAtCallStart作为在execute()里写的可变字段不如按参数下传;测试 1 的discoverToolsForServer没有恢复CONNECTED,因此区分不出外层neverDelivered传递与内层重新快照。 - 需要明确表态:豁免之后,模型在收到
UNSAFE_REPLAY后的下一次同工具调用会静默走通——按"首次投递"论是自洽的,但这等于该防护对无注解工具降级为"只挡一轮"。这个取舍本身可以接受,只是 PR 与代码注释里都没写。
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #8482All findings addressed in commit Core redesign: positive delivery evidence instead of a status snapshotBoth the automated reviewer's Critical R1-2 and @wenshao's #2 identified the same defect: the Fix: the carve-out now keys on the MCP SDK's own delivery-level evidence. In the pinned SDK (1.30.0), Changes in
Tests (
|
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
|
🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action taken. The only new feedback since the last evaluation is an automated notification that the Qwen Code review run timed out after 10800 seconds of its 180-minute budget. This is a review-harness event, not a code-level finding:
No code changes were made this round and no commit was created. 中文说明Autofix 审查轮次 — 未采取任何操作。 自上次评估以来唯一的新反馈是一条自动通知：Qwen Code 审查运行在 180 分钟预算中超时（10800 秒后终止）。这是审查流程（review harness）层面的事件，而非针对代码的发现：
本轮未做任何代码更改，也未创建任何提交。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| function isNeverDeliveredError(error: unknown): boolean { | ||
| return getErrorMessage(error) === 'Not connected'; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The never-delivered carve-out keys on an unversioned MCP SDK implementation-detail string, and all four new tests mock callTool with hand-rolled errors instead of exercising the real SDK, so SDK drift cannot fail CI.
Failure scenario: The dependency range is ^1.30.0; a routine minor bump that rewords Protocol.request()'s pre-send rejection — or wraps it with a cause, which getErrorMessage would suffix as (cause: ...) — makes isNeverDeliveredError return false forever after. All four new tests reject with their own new Error('Not connected'), so they keep passing; in production, never-delivered calls on unannotated tools silently revert to UNSAFE_REPLAY refusals — the exact failure this PR removes — with no red signal anywhere. (The pinned 1.30.0 behavior itself was probe-verified correct: a real SDK Client connected-then-closed yields exactly the bare Error('Not connected') pre-send rejection, and a delivered call can never surface that string — the McpError prefix MCP error ${code}: defeats the exact match.)
Suggested fix: add one test that exercises the real @modelcontextprotocol/sdk Client (never connected, or connected-then-closed): assert its genuine pre-send rejection is classified as never-delivered, and symmetrically that an in-flight transport close (McpError 'Connection closed') is not — pinning the contract at the real dependency so a rewording bump surfaces as a red test instead of a silent production regression.
中文说明
[建议] 未投递豁免依赖 MCP SDK 未固定版本的实现细节字符串，且四个新测试全部用手工 mock 的 callTool 错误代替真实 SDK，SDK 漂移无法让 CI 变红。
失败场景：依赖范围为 ^1.30.0；一次常规 minor 升级若改写 Protocol.request() 的发送前拒绝措辞——或给它附加 cause（getErrorMessage 会追加 (cause: ...) 后缀）——isNeverDeliveredError 将永远返回 false。四个新测试都用自带的 new Error('Not connected') 拒绝，因此仍然全绿；生产环境中，无注解工具的未投递调用会静默退回 UNSAFE_REPLAY 拒绝——正是本 PR 要消除的故障——且没有任何红色信号。（已用探针验证 1.30.0 本身行为正确：真实 SDK Client 连接后关闭，产生的正是裸 Error('Not connected') 发送前拒绝；已投递的调用不可能浮出该字符串——McpError 前缀 MCP error ${code}: 会使精确匹配失败。）
建议修复：补一个使用真实 @modelcontextprotocol/sdk Client 的测试（从未连接或连接后关闭）：断言其真实的发送前拒绝被判定为未投递，对称地断言传输中途关闭（McpError 'Connection closed'）不被判定为未投递——把契约钉在真实依赖上，使措辞变更的升级以红灯测试显形，而非静默的生产回归。
— qwen3.8-max via Qwen Code /review (v0.21.5)
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Address review summary — PR #8482 (same-run verification repair)This round is a same-run verification repair: the previous commit Feedback points[Suggestion] rc:3715798544 — never-delivered carve-out keyed on an unversioned SDK string; all tests mock
|
|
🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| function isNeverDeliveredError(error: unknown): boolean { | ||
| return getErrorMessage(error) === 'Not connected'; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R3-1: No test pins the exact-match boundary of isNeverDeliveredError — Failure scenario: a future "tolerate SDK wording variations" edit changes === to .includes(); the whole 98-test suite stays green (mutant verified by run), and a delivered call whose server-side error message merely contains Not connected (e.g. a tool handler throwing new Error('Not connected to database')) is then classified as never-delivered and auto-replayed — the exact double-execution hazard the gate exists to prevent. The current exact-match code is safe (the McpError constructor's MCP error <code>: prefix keeps every delivered rejection from matching today); this test pins the boundary against a future loosening. Add a negative-case test in the auto-reconnect describe block, e.g.:
it('still gates a rejection whose message only contains the pre-send wording', async () => {
const deadClient: McpDirectClient = {
callTool: vi
.fn()
.mockRejectedValueOnce(new Error('Not connected to database')),
};
// ...unannotated tool, DISCONNECTED or CONNECTED status alike...
await expect(
reconnectTool.build(params).execute(new AbortController().signal),
).rejects.toThrow(unsafeReplayErrorMessage);
expect(discoverToolsForServer).not.toHaveBeenCalled();
expect(liveClient.callTool).not.toHaveBeenCalled();
});中文说明
[建议] 没有任何测试钉住 isNeverDeliveredError 的精确匹配边界 —— 失败场景：未来某次「兼容 SDK 措辞变化」的修改把 === 改成 .includes()，全部 98 个测试依然全绿（变异体已实跑验证），此后一个已投递、但服务端错误消息仅包含 Not connected 的调用（例如工具 handler 抛出 new Error('Not connected to database')）会被判定为「从未投递」并自动重放——这正是该闸门要防的重复执行风险。当前的精确匹配代码是安全的（McpError 构造器的 MCP error <code>: 前缀使任何已投递的错误响应在今天都不可能命中精确匹配）；此测试用于钉住该边界，防止未来放宽。可在 auto-reconnect describe 块中补一个反例测试（形状见上方英文代码示例）。
— qwen3.8-max via Qwen Code /review (v0.21.5)
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 314 passed · 1 failed · 315 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：314 通过 · 1 失败 · 315 总计 Verification reportPR 8482 Deep Verification — fix(core): a never-delivered MCP call is a first delivery, not a replayVerdict: The code change is correct, load-bearing, and regression-free — but the PR's stated premise (a deterministically red test blocking main) is stale at this merge base, and the PR body describes a different mechanism than the one that landed. Details below. 中文摘要
Central claim and A/B tableCentral claim (as verified, not as worded): the #8387 replay-safety gate refuses every failed call of an unannotated tool — including calls the SDK rejected before sending ( Secondary claims: (a) all #8387 refusal cases remain gated; (b) the gate re-applies to the retried call's own failure evidence.
The load-bearing proof is cell C: the two delivery tests flip broken→fixed with the carve-out, failing on base with the exact Corrections (to the PR description, not code-change requests)
FindingsF1 — premise/description: the blocking red test does not exist at this base (see Correction 1)Severity: reviewer-attention, not a code defect. The code fix remains justified on its own merits (cell C), but the merge rationale as written — "unblock the required Test job" — no longer matches the state of main at git worktree add /tmp/base 32e27415779226b23174a3b0aa6c04e094f1aca2 # wire node_modules
cd /tmp/base/packages/core && npx vitest run src/tools/mcp-tool.test.ts # 92/92 greenF2 — description: body mechanism ≠ landed mechanism (see Correction 2)Severity: reviewer-attention. Suggested action: update the PR body; no code change. F3 — coverage gap: the suite cannot tell the strict matcher from a substring matcher (M4 survivor)Severity: Suggestion. Mutation Mutation matrix (vacuity + load-bearing attribution)All runs against head's test file (98 tests); full logs in
No mutant regressed a passing test to green-while-wrong; M1's 14 kills show the #8387 era tests and the new tests pin the gate jointly. The new tests are not vacuous and are pinned by the carve-out itself (M2 disables only the predicate), not by an earlier branch. Gates
Not covered
MethodologyEnvironment: the CI verify container ( Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Round summaryPR #8482, branch Feedback points[Suggestion] R3-1 (rc:3717284179) — no test pins the exact-match boundary of
|
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
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. ✅
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： AutoFix round — no action neededNo new actionable review feedback has arrived since the last evaluation (2026-08-05T01:20:48Z), so no changes were made this round.
No base conflict was reported, so no merge was performed. The branch remains at 中文说明AutoFix 本轮 —— 无需处理自上次评估（2026-08-05T01:20:48Z）以来没有新的可处理评审反馈，因此本轮未做任何改动。
未报告基分支冲突，因此未执行合并。分支保持在 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: test-efficacy — harness baseline could not be green (nested ajv v8 not linked into the probe tree; environment artifact), so mutants/hunks did not run; the revert probe was inconclusive (no-tests).
[Critical] R4-2 (still-standing blocker — @wenshao's merge-hold condition (a), raised in two separate reviews on this PR): the PR description still documents the DISCARDED status-snapshot mechanism ('The invocation snapshots getMCPServerStatus(serverName) before issuing its call … The gate … is skipped only when that snapshot says DISCONNECTED') and a falsified regression-test claim ('The previously-red test is the regression test; it passes with the fix and fails without it'; '91/91'), while the shipped diff keys the carve-out solely on getErrorMessage(error) === 'Not connected' — no snapshot exists anywhere in the code, and the new JSDoc explicitly rejects the status-based approach. A/B-probed at this commit: the cited test passes with the carve-out reverted (#8478 already made it green on main via trust + idempotentAnnotations), and the file now has 100 tests. The merged-PR record would describe the very mechanism prior review rejected as double-execution-unsafe, inviting a future edit to restore snapshot semantics that do not exist. Fix: rewrite the Summary / The bug / The fix / Test plan sections (and the Chinese mirror) to describe the shipped error-identity mechanism (bypass only on the SDK's pre-send 'Not connected' rejection; ambiguous failures stay gated; the previously-red test was fixed by #8478), citing the new carve-out tests as the regression tests.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：test-efficacy — harness baseline could not be green (nested ajv v8 not linked into the probe tree; environment artifact), so mutants/hunks did not run; the revert probe was inconclusive (no-tests)。
[Critical] R4-2 (still-standing blocker — @wenshao's merge-hold condition (a), raised in two separate reviews on this PR): the PR description still documents the DISCARDED status-snapshot mechanism ('The invocation snapshots getMCPServerStatus(serverName) before issuing its call … The gate … is skipped only when that snapshot says DISCONNECTED') and a falsified regression-test claim ('The previously-red test is the regression test; it passes with the fix and fails without it'; '91/91'), while the shipped diff keys the carve-out solely on getErrorMessage(error) === 'Not connected' — no snapshot exists anywhere in the code, and the new JSDoc explicitly rejects the status-based approach. A/B-probed at this commit: the cited test passes with the carve-out reverted (#8478 already made it green on main via trust + idempotentAnnotations), and the file now has 100 tests. The merged-PR record would describe the very mechanism prior review rejected as double-execution-unsafe, inviting a future edit to restore snapshot semantics that do not exist. Fix: rewrite the Summary / The bug / The fix / Test plan sections (and the Chinese mirror) to describe the shipped error-identity mechanism (bypass only on the SDK's pre-send 'Not connected' rejection; ambiguous failures stay gated; the previously-red test was fixed by #8478), citing the new carve-out tests as the regression tests.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| function isNeverDeliveredError(error: unknown): boolean { | ||
| return getErrorMessage(error) === 'Not connected'; | ||
| } |
There was a problem hiding this comment.
[Suggestion] No test pins the McpError-prefix invariant this carve-out's safety rests on. The exact match is sound today only because a server-originated rejection can never arrive as a bare 'Not connected': the SDK wraps every JSON-RPC error response as McpError, whose constructor prefixes the message (MCP error ${code}: ${message}), so a delivered call whose handler echoes 'Not connected' arrives as 'MCP error -32603: Not connected' and fails the match. The real-SDK tests pin the pre-send wording and the in-flight close, but not this third leg. Distinct from R3-1 (the .includes() near-miss), which the new 'Not connected to database' test kills. Failure scenario: a future "tolerate SDK wording variations" edit loosens the matcher (e.g. endsWith('Not connected'), unanchored regex, case-fold) → the whole 100-test suite stays green (mutant executed: the near-miss test does not kill it) → a delivered server-echo now matches and the gate auto-replays it — probe-verified against the pinned SDK: handler executed 4× through MAX_RECONNECT_RETRIES instead of UNSAFE_REPLAY. Suggested fix: add the symmetric real-SDK pin next to the in-flight test:
it('still gates a delivered call whose server handler echoes the pre-send wording', async () => {
const params = { param: 'test' };
const [clientTransport, serverTransport] =
InMemoryTransport.createLinkedPair();
const server = new Server(
{ name: 'real-sdk-server', version: '0.0.0' },
{ capabilities: { tools: {} } },
);
// Delivered: the handler runs, then throws the SDK's own guard wording.
// The client must see the prefixed McpError and stay gated.
server.setRequestHandler(CallToolRequestSchema, () => {
throw new Error('Not connected');
});
await server.connect(serverTransport);
const realClient = new Client({
name: 'real-sdk-client',
version: '0.0.0',
});
await realClient.connect(clientTransport);
const discoverToolsForServer = vi.fn().mockResolvedValue(undefined);
const mockConfig = {
isTrustedFolder: () => true,
getToolRegistry: () => ({ discoverToolsForServer, ensureTool: vi.fn() }),
getTruncateToolOutputThreshold: () => 0,
getTruncateToolOutputLines: () => 0,
};
updateMCPServerStatus(serverName, MCPServerStatus.CONNECTED);
const reconnectTool = new DiscoveredMCPTool(
mockCallableToolInstance,
serverName,
serverToolName,
baseDescription,
inputSchema,
undefined,
undefined,
mockConfig as any,
realClient, // unannotated
);
await expect(
reconnectTool.build(params).execute(new AbortController().signal),
).rejects.toThrow(unsafeReplayErrorMessage);
expect(discoverToolsForServer).not.toHaveBeenCalled();
});(probe-verified: this exact test flips under the loosening mutant while the current suite stays green)
中文说明
没有测试钉住该豁免所依赖的 McpError 前缀不变量。当前精确匹配之所以可靠,仅仅是因为服务端产生的拒绝不可能以裸 'Not connected' 到达:SDK 把每个 JSON-RPC 错误响应包装为 McpError,其构造函数会给消息加前缀(MCP error ${code}: ${message}),因此已投递、handler 恰好回显 'Not connected' 的调用到达时是 'MCP error -32603: Not connected',无法命中精确匹配。真实 SDK 测试钉住了预发送措辞与中途断连,但没有钉住这第三条腿。这与 R3-1(.includes() 近似匹配)不同——那个变异体已被新增的 'Not connected to database' 测试杀死。失败场景:未来某次"容忍 SDK 措辞变化"的修改放宽匹配(如 endsWith('Not connected')、无锚正则、忽略大小写)→ 全部 100 个测试仍然全绿(已实测该变异体:近似匹配测试杀不死它)→ 已投递的服务端回显此时会命中豁免并被自动重放——针对锁定版本 SDK 的探针验证:handler 被执行 4 次(直到 MAX_RECONNECT_RETRIES),而不是抛出 UNSAFE_REPLAY。建议修复:在中途断连测试旁补一个对称的真实 SDK 钉桩(代码见上)。已验证:该测试在放宽变异体下会由绿转红,而现有套件保持全绿。
— qwen3.8-max via Qwen Code /review (v0.21.5)
|
Closing by maintainer decision: the #8478 resolution stands — the replay gate refuses auto-replay for unannotated tools even when the server was known-disconnected at call start, and manual retry is the recovery path. The never-delivered carve-out documented here remains available in history if that trade is ever revisited. |
4 participants
Summary
mcp-tool.test.ts > auto-reconnect on connection error > reconnects instead of reporting a timeout when the server is known disconnectedhas been red on main since the replay-safety gate merged (#8387,0cb109f513), and it fails deterministically — bisected: green atd1648b3af9, red from0cb109f513on. Every full-profile PR behind it fails the requiredTest (ubuntu-latest)job on this one test; #8468 and #8471 are both currently blocked by it.The bug
#8387's gate protects against re-executing a call that may have completed before its connection failed — sound policy, keyed on the tool's idempotency annotations. But it also fires in the one case its own premise excludes: a call issued while the server was already known
DISCONNECTEDnever reached the server. Nothing "may have completed"; retrying it is the first delivery of that call, not a replay. As landed, every dead transport became a permanentUNSAFE_REPLAYerror for every unannotated tool — which is exactly the recovery path the existing (and now-red) test asserts.The fix
getMCPServerStatus(serverName)before issuing its call — by the time the error surfaces, the status has been overwritten by the failure itself.DISCONNECTED.All of #8387's refusal cases are unchanged — they run against a live-at-call-start connection. 91/91 in
mcp-tool.test.ts.Test plan
mcp-tool.test.ts: 91/91.eslint --max-warnings 0clean.中文说明
概述
mcp-tool.test.ts的 "reconnects instead of reporting a timeout when the server is known disconnected" 自 #8387(0cb109f513)合入后在 main 上持续红灯,且为确定性失败——二分定位:d1648b3af9绿,0cb109f513起红。此后每个 full-profile PR 的必需Test (ubuntu-latest)任务都挂在这一个测试上;#8468 与 #8471 当前均被其阻塞。缺陷
#8387 的闸门防止重放"可能已经执行完成"的调用——策略本身正确,以工具的幂等性注解为准。但它同样拦截了其前提本身排除的场景:在服务器已知
DISCONNECTED时发出的调用从未到达服务器,不存在"可能已完成";重试它是该调用的首次投递,不是重放。按合入的行为,每个断连的 transport 对所有无注解工具都变成永久性UNSAFE_REPLAY错误——而这正是现有(现已红灯)测试断言的恢复路径。修复
getMCPServerStatus(serverName)——等错误浮出时,状态早已被失败本身覆盖。DISCONNECTED时跳过闸门(重连前与重连后两处检查一致)。#8387 的全部拒绝用例不受影响——它们的场景都是调用时连接存活。
mcp-tool.test.ts91/91。测试计划
mcp-tool.test.ts:91/91;eslint --max-warnings 0无告警。