Skip to content

feat(mcp): add MCP 2026 core and WebShell Apps host - #8992

Merged
samuelhsin merged 86 commits into
QwenLM:mainfrom
samuelhsin:codex/feat-mcp-2026-core
Aug 23, 2026
Merged

feat(mcp): add MCP 2026 core and WebShell Apps host#8992
samuelhsin merged 86 commits into
QwenLM:mainfrom
samuelhsin:codex/feat-mcp-2026-core

Conversation

@samuelhsin

@samuelhsin samuelhsin commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

This PR delivers the first MCP 2026 client slice and an MCP Apps host for daemon-backed WebShell sessions. Configured stdio MCP clients negotiate the modern protocol automatically; remote HTTP/SSE/TCP clients stay on the legacy initialize handshake because SDK v2 has no HTTP probe-to-initialize fallback. Clients advertise the Apps extension, preserve ui:// tool metadata, fetch and validate the declared HTML resource after a successful tool call, and keep the model-visible tool result unchanged.

WebShell renders the App inline through the official AppBridge transport. A static proxy on a different loopback origin relays JSON-RPC into an opaque-origin inner iframe, applies server-declared CSP and permissions, limits HTML to 1 MiB, and falls back to the ordinary text result when validation or isolation fails. Completed turns containing an MCP App stay expanded so the App is visible without opening the hidden steps manually.

Why it's needed

Issue #8968 calls for MCP 2026 protocol support, including Apps. Negotiation alone exposed metadata but gave daemon/WebShell users no way to see the interactive result. This change completes the first end-to-end path while retaining compatibility with legacy servers and existing MCP permission, timeout, cancellation, and model-context behavior.

Reviewer Test Plan

How to verify

  1. Configure the mock stdio server below in a temporary daemon workspace and start WebShell on a loopback address.
  2. Ask the model to call mcp__mcp-app-demo__show_revenue_dashboard with region: "APAC" exactly once.
  3. Confirm the completed turn is expanded by default and displays the Revenue dashboard inline with Revenue $128,420, Orders 1842, Conversion 7.8%, and Jan–Jun bars.
  4. Confirm the inner App frame has an opaque origin (sandbox="allow-scripts allow-forms", without allow-same-origin) and that an invalid or unavailable App resource falls back to the normal tool text.
Mock stdio MCP App used for the daemon/WebShell verification (reference only; not committed)

Save as mcp-app-demo.mjs beside any test workspace:

import { Server } from '@modelcontextprotocol/sdk/server/index.js';
import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js';
import {
  CallToolRequestSchema,
  ListToolsRequestSchema,
  ReadResourceRequestSchema,
} from '@modelcontextprotocol/sdk/types.js';

const extensionId = 'io.modelcontextprotocol/ui';
const resourceUri = 'ui://qwen-code-demo/revenue-dashboard';
const mimeType = 'text/html;profile=mcp-app';

const html = `<!doctype html>
<html>
<head>
  <meta charset="utf-8">
  <style>
    body { margin: 0; padding: 16px; color: #f7f8fc; background: #17172a; font: 14px system-ui; }
    main { border: 1px solid #454568; border-radius: 16px; padding: 20px; }
    h1 { margin: 0 0 16px; }
    #metrics { display: grid; grid-template-columns: repeat(3, 1fr); gap: 12px; }
    article { border: 1px solid #454568; border-radius: 10px; padding: 12px; }
    strong { display: block; margin-top: 4px; font-size: 22px; }
  </style>
</head>
<body>
  <main><h1>Revenue dashboard</h1><section id="metrics">Waiting for result…</section></main>
  <script>
    const send = (message) => parent.postMessage(message, '*');
    addEventListener('message', ({ data: message }) => {
      if (message?.id === 1 && message.result) {
        send({ jsonrpc: '2.0', method: 'ui/notifications/initialized', params: {} });
      }
      if (message?.method === 'ui/notifications/tool-result') {
        const value = message.params.structuredContent;
        document.querySelector('#metrics').innerHTML =
          '<article>Revenue<strong>$' + value.revenue.toLocaleString() + '</strong></article>' +
          '<article>Orders<strong>' + value.orders + '</strong></article>' +
          '<article>Conversion<strong>' + value.conversion + '%</strong></article>';
        send({ jsonrpc: '2.0', method: 'ui/notifications/size-changed', params: { height: 220 } });
      }
      if (message?.method === 'ui/resource-teardown') {
        send({ jsonrpc: '2.0', id: message.id, result: {} });
      }
    });
    send({
      jsonrpc: '2.0',
      id: 1,
      method: 'ui/initialize',
      params: {
        protocolVersion: '2026-01-26',
        appInfo: { name: 'qwen-code-mcp-app-demo', version: '1.0.0' },
        appCapabilities: {},
      },
    });
  </script>
</body>
</html>`;

const server = new Server(
  { name: 'qwen-code-mcp-app-demo', version: '1.0.0' },
  {
    capabilities: {
      tools: {},
      resources: {},
      extensions: { [extensionId]: { mimeTypes: [mimeType] } },
    },
  },
);

server.setRequestHandler(ListToolsRequestSchema, async () => ({
  tools: [{
    name: 'show_revenue_dashboard',
    description: 'Show a demo revenue dashboard as an MCP App.',
    inputSchema: {
      type: 'object',
      properties: { region: { type: 'string' } },
    },
    annotations: { readOnlyHint: true },
    _meta: { ui: { resourceUri } },
  }],
}));

server.setRequestHandler(CallToolRequestSchema, async (request) => ({
  content: [{
    type: 'text',
    text: `Revenue dashboard ready for ${request.params.arguments?.region ?? 'Global'}.`,
  }],
  structuredContent: { revenue: 128420, orders: 1842, conversion: 7.8 },
}));

server.setRequestHandler(ReadResourceRequestSchema, async (request) => {
  if (request.params.uri !== resourceUri) throw new Error('Unknown resource');
  return {
    contents: [{
      uri: resourceUri,
      mimeType,
      text: html,
      _meta: { ui: { csp: {}, permissions: {} } },
    }],
  };
});

await server.connect(new StdioServerTransport());

Register it in the temporary workspace's .qwen/settings.json:

{
  "security": { "folderTrust": { "enabled": false } },
  "mcpServers": {
    "mcp-app-demo": {
      "command": "node",
      "args": ["/absolute/path/to/mcp-app-demo.mjs"],
      "trust": true
    }
  }
}

Evidence (Before & After)

Before: daemon-backed WebShell displayed only the ordinary MCP tool result and had no MCP Apps rendering path.

After: the model called the mock MCP tool once in a real daemon session and WebShell rendered its App inline. This screenshot is committed as review evidence:

MCP App rendered inline in a daemon-backed WebShell session

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Node.js 22, daemon-served WebShell on loopback, stdio mock MCP server, and a real model turn using the configured OPENAI_API_KEY without exposing the credential.

Validation: 333 focused core MCP tests passed; 185 focused WebShell host, tool-group, and turn-collapse tests passed; 2 daemon sandbox tests passed; repository build, typecheck, and lint passed. Real UI verification confirmed the canonical tool was called exactly once, the App was visible by default after a hard reload, the nested iframe used an opaque origin without allow-same-origin, and the browser console contained no relevant errors.

Risk & Scope

  • Main risk or tradeoff: MCP App HTML is untrusted and requires a carefully constrained bridge. The host therefore uses a double iframe, loopback-origin separation, an opaque inner origin, CSP allowlists, a 1 MiB resource limit, a 10-second resource timeout, and text fallback.
  • Not validated / out of scope: modern-only remote (HTTP/SSE/TCP) protocol negotiation; App-initiated tool calls, links, downloads, messages, model-context updates, fullscreen mode, interactive MRTR flows, and local Windows/Linux UI runs.
  • Breaking changes / migration notes: None. Legacy MCP servers continue through negotiated compatibility behavior, and servers that do not advertise the Apps extension retain ordinary tool rendering.

Linked Issues

Refs #8968

中文说明

本 PR 做了什么

本 PR 实作第一阶段 MCP 2026 client 支持,以及 daemon WebShell 的 MCP Apps host。配置型 stdio MCP client 会自动协商新协议;远程 HTTP/SSE/TCP 因 SDK v2 没有 HTTP probe 到 initialize 的回退而保持 legacy handshake。Client 声明 Apps extension、保留工具的 ui:// metadata,并在工具成功后读取及验证对应 HTML resource,同时保持模型可见的工具结果不变。

WebShell 通过官方 AppBridge transport 把 App 渲染在会话中。不同 loopback origin 的静态 proxy 将 JSON-RPC 转发到 opaque-origin 内层 iframe,并套用服务器声明的 CSP 与权限;HTML 上限为 1 MiB,验证或隔离失败时回退普通文本。包含 MCP App 的完成回合会默认展开,因此不需要手动展开步骤才能看到 App。

为什么需要

Issue #8968 要求支持包括 Apps 在内的 MCP 2026 协议。只有协商能力仍无法让 daemon/WebShell 用户看到互动结果;本改动补齐第一个端到端路径,同时保留 legacy server 相容性,以及既有 MCP 权限、timeout、取消和模型上下文行为。

Reviewer Test Plan

如何验证

  1. 在临时 daemon workspace 配置上方折叠区中的 mock stdio server,并以 loopback 地址启动 WebShell。
  2. 要求模型以 region: "APAC" 恰好调用一次 mcp__mcp-app-demo__show_revenue_dashboard
  3. 确认完成回合默认展开,并直接显示 Revenue $128,420、Orders 1842、Conversion 7.8% 和 Jan–Jun 图表。
  4. 确认内层 App iframe 使用 opaque origin(sandbox="allow-scripts allow-forms",没有 allow-same-origin),而无效或无法读取的 App resource 会回退普通工具文本。

Mock MCP App 完整参考代码与 .qwen/settings.json 配置位于上方折叠区,只用于说明和复验,不提交到产品代码库。

Before / After 证据

Before:daemon WebShell 只能显示普通 MCP 工具结果,没有 MCP Apps 渲染路径。

After:真实 daemon 会话中的模型仅调用 mock MCP 工具一次,WebShell 在会话内渲染 App;截图见英文段落并已作为 review 证据提交。

测试环境

macOS 已验证;Windows 与 Linux 尚未做本地 UI 验证。环境为 Node.js 22、loopback daemon WebShell、stdio mock MCP server,以及使用已配置 OPENAI_API_KEY 的真实模型回合,过程中未暴露凭证。

验证结果:333 个 core MCP 针对性测试、185 个 WebShell host/tool-group/turn-collapse 针对性测试、2 个 daemon sandbox 测试通过;仓库 build、typecheck、lint 通过。真实 UI 验证确认 canonical tool 只调用一次、硬刷新后 App 默认可见、内层 iframe 使用不含 allow-same-origin 的 opaque origin,浏览器控制台无相关错误。

风险与范围

  • 主要风险或权衡:MCP App HTML 不可信,因此 host 使用双 iframe、loopback origin 隔离、opaque inner origin、CSP allowlist、1 MiB resource 上限、10 秒读取 timeout 和文本回退。
  • 未验证或不在范围内:纯 2026 远程(HTTP/SSE/TCP)协议协商;App 主动发起的工具调用、链接、下载、消息、模型上下文更新、全屏模式、互动 MRTR 流程,以及 Windows/Linux 本地 UI。
  • 破坏性变更或迁移说明:无。Legacy MCP server 继续走协商相容路径;未声明 Apps extension 的服务器仍显示普通工具结果。

关联 Issue

Refs #8968

@samuelhsin

Copy link
Copy Markdown
Collaborator Author

E2E / protocol verification report

  • Baseline: inspected the globally installed qwen CLI (0.19.10-preview.0) without starting an authenticated prompt or changing user configuration.
  • Modern negotiation: a real v2 control transport confirmed server/discover is attempted first, initialize is skipped, and tools, prompts, and resources remain usable.
  • HTTP metadata: a real Streamable HTTP server confirmed MCP-Protocol-Version, MCP-Method, and tool-specific MCP-Name headers on modern requests.
  • Cache behavior: positive private ttlMs hints cache tool, prompt, and resource list results within their TTL.
  • Legacy compatibility: a server returning method-not-found for server/discover falls back to initialize; tools, prompts, and resources remain usable through the compatibility path.
  • Automated verification: 248 focused MCP tests across 4 files passed; the full build, workspace typecheck, and targeted ESLint passed on Node.js 22.14.0 / macOS.

Windows and Linux remain for CI. MRTR and MCP Apps hosting are intentionally deferred to separate follow-up changes.

@samuelhsin samuelhsin changed the title feat(mcp): add MCP 2026 core negotiation foundation feat(mcp): add MCP 2026 core and WebShell Apps host Aug 12, 2026
@samuelhsin

Copy link
Copy Markdown
Collaborator Author

MCP Apps daemon/WebShell E2E report

  • Result: 4/4 checks passed.
  • A fresh daemon session called mcp__mcp-app-demo__show_revenue_dashboard exactly once with { "region": "APAC" }.
  • After a clean authentication, direct navigation, and hard reload, the turn was expanded by default without clicking the steps control; the inline App showed Revenue $128,420, Orders 1842, Conversion 7.8%, and Jan–Jun bars.
  • DevTools confirmed the nested untrusted frame uses sandbox="allow-scripts allow-forms" with srcdoc and no allow-same-origin.
  • Console/runtime monitoring found no relevant errors or exceptions.

MCP App daemon/WebShell E2E

The complete mock stdio MCP server and .qwen/settings.json registration used for this run are included as a reference-only collapsible section in the PR description; the mock code is intentionally not part of the repository diff.

@samuelhsin

Copy link
Copy Markdown
Collaborator Author

Review notes (verified at 96d88eb)

Local verification: build / typecheck / targeted ESLint pass; MCP-focused suite 248 tests pass and the src/mcp OAuth suite (198 tests) passes. The red Test (ubuntu-latest, Node 22.x) check is an infrastructure failure — the runner could not reach github.com to download shellcheck (curl: (28)) — unrelated to this change. The direction and slicing look right; one blocking finding from E2E verification below, plus two observations.

Critical — legacy servers that under-declare capabilities.tools silently lose their tools

discoverTools goes through mcpToTool(...) → v2 Client.listTools(). Unlike the v1 SDK, whose listTools issued an ungated raw request, the v2 typed list helper short-circuits when the capability is not declared:

// @modelcontextprotocol/client 2.0.0, Client.listTools()
if (!this._serverCapabilities?.tools && !this._enforceStrictCapabilities) {
  console.debug("...returning empty list");
  return { tools: [] };   // never sent over the wire
}

Legacy-era sessions populate _serverCapabilities from the initialize result, so a legacy server that serves tools/list but omits tools from capabilities now yields zero tools — no error, no wire request — and discoverTools treats the empty result as a "prompt-only server". This is exactly the under-declaring server class this file deliberately accommodates for prompts/resources (the retained legacy raw-request paths in listMcpPrompts / listMcpResources / readResource, and the comments explaining why), but tools did not get the same lenient treatment. It also contradicts the PR description's "legacy servers retain … existing behavior" / "no breaking changes".

E2E evidence — bundled CLI + real model, hand-rolled stdio probe servers, merge-base 1570e6c vs this commit:

Server Baseline This PR
modern-only (2026-07-28, rejects initialize) unreachable ✅ tool discovered and called
legacy, declares tools
legacy, capabilities: {} but serves tools/list ✅ tool discovered and called ❌ tools missing; CLI additionally warns "MCP server(s) failed to start" for that server

The PR run's stderr shows the gate firing: Client.listTools() called but server does not advertise tools capability - returning empty list (twice — genai discovery path + annotations fetch). The existing legacy-fallback test declares capabilities: { tools: {} }, so it cannot catch this. Suggested fix: keep tools/list on the raw request path for legacy-era sessions as well (mirroring prompts/resources), and add a regression test with an under-declaring legacy server.

Observation — qwen mcp list probe still uses a v1 client

packages/cli/src/commands/mcp/list.ts constructs its own v1 Client (only createTransport is shared), so a modern-only server reports DISCONNECTED there while connecting fine in-session. Not a regression (such servers were unreachable everywhere before), but worth tracking as a follow-up under #8968.

Observation — stdio connection cost under auto mode

With the base StdioClientTransport, v2 auto negotiation spawns a disposable sibling probe process on every connection, and a legacy stdio server that never answers server/discover delays the fallback until the probe timeout — which inherits the timeout passed to connect() (i.e., the per-server timeout setting, else 60s). This also applies to pool respawns and health-monitor reconnects. Consider capping versionNegotiation.probe.timeoutMs or documenting this in the design doc.

Nit: the getProtocolEra?.() optional calls are unnecessary — the method always exists on the v2 Client type.

The probe-server harness used for the E2E runs (three stdio servers + runner script) is available on request if it's useful for the follow-up slices.

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

🖼️ web-shell visual preview

Rendered against a mock daemon (no real backend): the PR base vs this PR head 4b8860e. Only screenshots that changed are shown (flows below, if any, are head-only) — refreshes on every push.

Screenshots · before / after

ℹ️ No screenshot changed against the PR base — but this PR edits 6 render-shaping files:

  • packages/web-shell/client/App.tsx
  • packages/web-shell/client/components/MessageList.tsx
  • packages/web-shell/client/components/WebShellTranscript.tsx
  • packages/web-shell/client/components/messages/McpApp.module.css
  • packages/web-shell/client/components/messages/McpApp.tsx
  • packages/web-shell/client/components/messages/ToolGroup.tsx

Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to packages/web-shell/client/e2e/visuals/screenshots.spec.ts that seeds whatever state the UI is gated on; it then appears here as a head-only (NEW) capture.

Full-resolution recordings (.webm) are attached to the workflow run.

Qwen Code · web-shell visuals

# Conflicts:
#	packages/web-shell/client/components/MessageList.tsx
@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

🩺 serve daemon A/B

Built the PR base vs this PR head 4b8860e, drove a fixed endpoint set against each, and diffed the JSON responses. Only fields that changed are shown.

No response changes against the PR base across 4 scenario(s).

Qwen Code · serve A/B

@samuelhsin

Copy link
Copy Markdown
Collaborator Author

Review follow-up: legacy tool discovery

Addressed the blocking legacy compatibility finding in 609ffce483 and merged current upstream/main (f2de42dec4).

  • Legacy sessions now use a raw tools/list request, preserving discovery for servers that implement the method but omit capabilities.tools.
  • Modern sessions continue through the typed/cache-aware v2 listTools() path.
  • The raw tool result is reused for annotations and MCP Apps metadata, so one discovery does not add a duplicate wire request.
  • Added a regression where the legacy server initializes with capabilities: {}: the SDK typed helper returns zero tools, while Qwen discovery sends one tools/list and returns echo.

Independent real-stdio verification with an under-declaring server observed exactly one tools/list and discovered legacy_echo. A modern cache/Apps run observed one wire tools/list across two discoveries and preserved ui://demo/dashboard through the mcp_app display.

Verification: core MCP 333/333; WebShell 297/297; CLI MCP App sandbox 5/5; full build, workspace typecheck, and lint passed. The qwen mcp list v1-client path and stdio auto-probe cost remain follow-up observations rather than changes in this PR.

@samuelhsin
samuelhsin marked this pull request as ready for review August 12, 2026 18:06
@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Re-running the gate at the current head — this replaces my 2026-08-12 pass.

Template: complete ✓ — all required headings, bilingual section, and a concrete reviewer test plan.

Problem: feature PR — the first MCP 2026 client slice of #8968 plus the first MCP Apps host for daemon/WebShell sessions. The motivation is real and documented in the linked issue and the committed design doc: a server implementing only the 2026-07-28 stateless protocol cannot complete the legacy initialize handshake, and once negotiation exists the advertised Apps metadata has nowhere to render for daemon/WebShell users. No before/after reproduction is expected for a feature request.

Direction: aligned. MCP 2026 / Apps is a real upstream protocol direction, the slice order is explicit in the design doc (negotiation + first end-to-end Apps surface now; MRTR, App-initiated tool calls, and internal-server migration as follow-ups), and maintainers have been steering this PR through review since it opened.

Size: touches core paths (packages/core/src/tools/**) and spans core / cli / web-shell / SDK packages. Current breakdown: ~1,387 production-logic lines, ~2,207 test lines, ~196 doc lines, ~1,003 regenerated lines (NOTICES + lockfile). As a feat this is not size-blocked, and the 500+-line core escalation does not apply: the author has write access on this repo, so per the prior run's precedent this is treated as maintainer-authored and the size is recorded rather than escalated. The 1000+-line advisory stands as a note: this is one coherent slice, and anything further belongs in the documented follow-ups.

Approach: the slice is honest, and the defaults moved in the compatibility-conservative direction review asked for — stdio servers stay on the single-process legacy flow unless versionNegotiation: "auto" is explicitly configured (the docs in this diff describe that correctly). Two hygiene notes for the record, neither gate-blocking: the PR description still says "Configured stdio MCP clients negotiate the modern protocol automatically", which no longer matches the opt-in behavior, and it implies the App stays visible after a hard refresh, which transcript replay does not restore (wenshao raised the latter with the author on 2026-08-22 — the designed degraded form is the text fallback).

Risk: Stage 1e matches high-risk paths packages/core/src/tools/mcp-client.ts, packages/core/src/tools/mcp-pool-key.ts, and packages/cli/src/acp-integration/acpAgent.ts — MCP client and ACP config surfaces are over-represented in this repo's revert history. Not a blocker; it means full review depth and CI evidence before approval. Both are in the Stage 2 comment.

Moving on to code review. 🔍

中文说明

在当前 head 上重跑门禁——本次替代 2026-08-12 的那一轮。

模板:完整 ✓——所有必需标题、双语部分、具体的 reviewer test plan。

问题:功能 PR——#8968 的第一个 MCP 2026 客户端切片,外加 daemon/WebShell 会话的第一个 MCP Apps 宿主。动机真实,有关联 issue 和已提交的设计文档支撑:只实现 2026-07-28 stateless 协议的服务器无法完成 legacy initialize 握手;协商存在之后,对外声明的 Apps metadata 在 daemon/WebShell 用户侧也没有渲染出口。功能请求不要求 before/after 复现。

方向:对齐。MCP 2026 / Apps 是真实的上游协议方向,切片顺序在设计文档中写得很明确(先做协商 + 第一个端到端 Apps 面;MRTR、App 主动发起的工具调用、内部 server 迁移留作后续),且维护者从 PR 提出起就一直在通过 review 引导它。

规模:触及核心路径(packages/core/src/tools/**),横跨 core / cli / web-shell / SDK。当前拆分:约 1,387 行生产逻辑、约 2,207 行测试、约 196 行文档、约 1,003 行重新生成内容(NOTICES + lockfile)。feat 类型不因规模被拦截;500+ 核心行升级也不适用——作者在本仓库有 write 权限,沿用上一轮的先例按维护者自提处理,规模仅作记录。1000+ 行的大 PR 提示仍然记一笔:这是一个内聚的切片,后续内容应放在已文档化的 follow-up 里。

方案:切片诚实,默认值也走向了评审所要求的兼容保守方向——stdio server 默认保持单进程 legacy 流程,除非显式配置 versionNegotiation: "auto"(diff 内的文档对此描述正确)。两条卫生层面的记录,均不阻门禁:PR 描述仍写着"Configured stdio MCP clients negotiate the modern protocol automatically",与当前的 opt-in 行为不符;且描述暗示硬刷新后 App 仍可见,而 transcript 回放并不会恢复它(wenshao 已于 2026-08-22 向作者提出——设计上的降级形态就是文本回退)。

风险:Stage 1e 命中高风险路径 packages/core/src/tools/mcp-client.tspackages/core/src/tools/mcp-pool-key.tspackages/cli/src/acp-integration/acpAgent.ts——MCP client 与 ACP 配置面在本仓库 revert 历史中占比偏高。不是拦截项;它意味着完整审查深度和批准前必须有 CI 证据,两者都在 Stage 2 评论里。

进入代码审查 🔍

Qwen Code · qwen3.8-max

Reviewed at 54c20920d8caa7a9a5403e188bef08b0ff85bc59 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Code review at 54c2092

My independent baseline for this problem (written before reading the diff): opt-in server/discover probe with a conservative legacy default for stdio, post-call resources/read of the ui:// resource with MIME and size validation, a double-iframe host with an opaque inner origin for WebShell, text fallback on every failure path, and — the part that matters most — the HTML payload never entering model context, session history, or non-rendering UI surfaces. The PR's approach matches that baseline, and beats it in two places: delegating wire-level negotiation to SDK v2 instead of re-implementing probe/fallback, and gating the v2 typed list/read helpers behind declared capabilities so under-declared legacy servers stay visible. Findings below.

Every prior blocker on this PR is now fixed and tested at this head. Walking the three open items my last pass and the human reviews left behind:

  • The mcp_app display object is handled on every surface now (my 2026-08-12 blocker). The TUI renders fallbackText instead of stringifying the object (ToolMessage.tsx, and daemon-tui-adapter.ts for daemon sessions), and history compaction drops html and toolResult while keeping fallbackText. Each fix carries a probe-style test asserting the HTML marker does not leak into the rendered or persisted output.
  • Both of jifeng's 2026-08-19 Criticals are closed. qwen mcp list now shares the session client factory, gets a 10 s budget that covers probe plus fallback handshake, and drops ping() (absent from the 2026 request registry, so it marked working modern servers Disconnected). wenshao's A/B at exactly this head shows the modern-only server Connected on the PR build vs Disconnected on base. The headless-JSON half (mcp_app rendering as empty string) is fixed via toolResultContent mapping to fallbackText, with a test.
  • doudouOUC's last held Critical (R2-15) is fixed here: isMcpToolVisibleToModel now treats visibility: null as visible, matching the undefined line above it, so pydantic-style servers that serialize unset fields as null no longer lose tools silently.

The security surface holds up, and is harder than my last pass. Loopback frame-src is now pinned to the daemon's Host port (the wildcard ports I noted in 2026-08-12 are gone, with a test rejecting them); CSP domains pass a strict ASCII origin pattern that drops ;-injection and Unicode homoglyph attempts (tested, including the ſ case); an oversized CSP query falls back to the locked-down policy; both iframes run allow-scripts allow-forms without allow-same-origin, so the App HTML executes at an opaque origin that cannot read WebShell storage or hit the daemon API as a same-origin client (the API stays bearer-gated — the pre-auth sandbox route serves only static, no-store proxy HTML). Resource reads stay bounded (exact MIME, 1 MiB, 10 s, abort-aware) and every failure path falls back to the text result. wenshao's browser-level probes on this head confirmed the isolation end to end, and the persisted-transcript check shows the HTML never reaches model context.

Non-blocking items for the record:

  • The PR body's "negotiate the modern protocol automatically" sentence and its hard-refresh visibility claim are stale relative to the shipped behavior (opt-in negotiation; replay shows the text fallback). The docs in the diff are correct; only the PR description needs a touch-up.
  • wenshao's O2: a modern server that omits ttlMs/cacheScope from a resources/read result fails the strict v2 schema and its App degrades silently to text. Fail-safe, but a one-liner in docs/users/features/mcp.md would save server authors the same stumble.
  • Two of doudouOUC's higher-ranked Suggestions remain open at this head, correctly deferred as non-blocking per the round-count guidance: compaction still spreads toolArguments and csp into retained displays (R2-12), and the proxy's hostOrigin anchor is validated by hostname only, port unpinned. On the latter I looked at the exploit shape before classifying it: a spoofed host origin gets its own isolated sandbox instance per daemon origin and never touches a real session's bearer-gated API or storage, so this is hardening for the follow-up, not a merge blocker.
  • Minor interop strictness: visibility must be an array containing "model" — a server sending a bare string loses the tool (tracked via hadVisibilityFilteredTools, so discovery doesn't fail). Matches the spec shape; just noting it.
sequenceDiagram
    participant P1 as Model
    participant P2 as core mcp-tool
    participant P3 as MCP server
    participant P4 as WebShell McpApp
    participant P5 as sandbox proxy
    participant P6 as inner app iframe
    P1->>P2: call MCP tool
    P2->>P3: tools call
    P3-->>P2: result with ui resourceUri
    P2->>P3: resources read the ui URI
    P3-->>P2: HTML with CSP and permissions
    P2-->>P4: mcp_app display, model result unchanged
    P4->>P5: iframe on swapped loopback origin
    P5->>P6: srcdoc at opaque origin
    P6-->>P4: AppBridge initialize, size updates
Loading
Files changed (30 of 61 shown)
File What changed
docs/design/mcp-2026-core-client-foundation.md Design doc for the slice - scope, safety model, deferred follow-ups, verification matrix
docs/users/features/mcp.md Documents the versionNegotiation opt-in, the legacy default, and the probe budget caveat
docs/developers/tools/mcp-server.md Adds versionNegotiation to the config reference
docs/developers/daemon/05-mcp-transport-pool.md Pool key now covers versionNegotiation
packages/core/package.json Adds the MCP client and core packages pinned at 2.0.0
packages/core/src/config/config.ts MCPServerConfig gains versionNegotiation
packages/core/src/tools/mcp-client.ts SDK v2 client factory, opt-in server-discover probe, lenient list and read for under-declared servers, Apps metadata
packages/core/src/tools/mcp-client-v2.test.ts New negotiation matrix - modern-only, legacy fallback, probe budgets, cache reuse, Apps metadata
packages/core/src/tools/mcp-client.test.ts Extended legacy behavior and v2 migration coverage
packages/core/src/tools/mcp-tool.ts Loads and validates the ui HTML resource after a call, builds the mcp_app display with fallbackText
packages/core/src/tools/mcp-tool.test.ts App display load, validation-failure fallback, and visibility filtering tests
packages/core/src/tools/mcp-pool-key.ts Fingerprint separates auto negotiation from the legacy default
packages/core/src/tools/tools.ts McpAppResultDisplay type added to the ToolResultDisplay union
packages/core/src/utils/toolResultDisplayCompaction.ts Compacts mcp_app history entries to fallbackText, dropping HTML and raw result
packages/cli/src/serve/mcp-app-sandbox.ts New pre-auth static proxy - validated CSP header, opaque-origin inner iframe, origin-checked relay
packages/cli/src/serve/mcp-app-sandbox.test.ts CSP injection and homoglyph rejection, proxy header and markup assertions
packages/cli/src/serve/server.ts Mounts the sandbox route beside the Web Shell
packages/cli/src/serve/web-shell-static.ts Host-port-pinned loopback frame-src, sandbox route in the pre-auth allowlist, Permissions-Policy builder
packages/cli/src/serve/web-shell-static.test.ts Pins frame-src to the daemon port and rejects wildcard ports
packages/cli/src/commands/mcp/list.ts Shares the session client factory, 10s budget, drops ping so modern servers count as Connected
packages/cli/src/nonInteractive/io/BaseJsonOutputAdapter.ts Headless JSON output maps mcp_app displays to fallbackText
packages/cli/src/nonInteractive/control/controllers/systemController.ts Passes versionNegotiation through daemon initialize
packages/cli/src/acp-integration/acpAgent.ts Validates and persists versionNegotiation across the ACP config paths
packages/cli/src/ui/components/messages/ToolMessage.tsx TUI renders fallbackText for mcp_app instead of stringifying the object
packages/cli/src/ui/daemon/daemon-tui-adapter.ts Daemon TUI path projects mcp_app to fallbackText
packages/web-shell/client/components/messages/McpApp.tsx New App host - loopback origin swap, AppBridge lifecycle, size clamps, text fallback
packages/web-shell/client/components/messages/McpApp.dom.test.tsx DOM tests for sandbox URL resolution, CSP query cap, and fallback rendering
packages/web-shell/client/components/messages/ToolGroup.tsx App rows stay expanded; compacted replays render text without mounting the iframe
packages/web-shell/client/components/MessageList.tsx Completed turns containing an App stay expanded
…and 31 more files App/MessageList wiring tests, MCP App unit tests, SDK and desktop schema plumbing for versionNegotiation, NOTICES and lockfile regen, demo screenshots

Testing evidence — the PR's own CI, fetched via API

This is an unattended CI run, so per the triage rules I did not build or execute any PR code; the evidence below is the PR's own CI at the reviewed commit, read through the API. All six pull_request-event workflow runs on 54c2092 completed success (Qwen Code CI, SDK Java, Web-shell Visuals, Serve A/B, Security Checks, Qwen Live Host CI). The ubuntu unit suite — the leg that actually compiles and runs the changed packages — is green on this head, which closes my 2026-08-12 CI blocker. The macOS/Windows Test legs and the no-sandbox CLI integration suite are skipped by this repo's matrix (not failures); the only non-green check is a cancelled route job, which is bot orchestration, not PR CI.

Check Conclusion
Test (ubuntu-latest, Node 22.x) ✅ success
Test (macos-latest, Node 22.x) ⏭️ skipped (matrix)
Test (windows-latest, Node 22.x) ⏭️ skipped (matrix)
Integration Tests (CLI, No Sandbox) ⏭️ skipped
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
Capture web-shell visuals (ubuntu-latest, Node 22.x) ✅ success
Serve A/B (ubuntu-latest, Node 22.x) ✅ success
Live Host (macos-latest) ✅ success
SDK Java matrix (ubuntu/windows/macos × Java 11/17/21) ✅ success
Dependency CVE audit ✅ success
Secret scan (TruffleHog) ✅ success
precheck-pr / Classify PR / label ✅ success

Not verified here: Windows behavior (no CI leg and no human report), and the live negotiation wire path — CI unit tests pin the protocol matrix with mock transports, not a real server process. On the latter, two stronger signals exist at exactly this head: wenshao's independent A/B verification on Linux (2026-08-22 — maintainer-run, real stdio servers, wire taps, and real Chromium; concluded no blockers), and the maintainer-triggered @qwen-code /verify run that is in flight right now (run 32617423836). Sandboxed verification would settle the remaining behavioral claim — that auto-negotiation's probe/fallback and the App fallback paths are load-bearing A/B against the base build — and its report will post separately when that run completes.

中文说明

代码审查(54c2092

我在读 diff 前先写了独立基线:stdio 采用 opt-in 的 server/discover 探测且默认保守走 legacy;工具调用成功后对 ui:// resource 做 resources/read 并校验 MIME 与大小;WebShell 用双层 iframe、内层 opaque origin;所有失败路径回退文本;最关键的是 HTML 永不进入模型上下文、会话历史或不渲染的 UI 面。PR 的方案与基线一致,且有两处更好:把 wire 层协商交给 SDK v2 而不是自己实现探测/回退;v2 的类型化 list/read 助手仅在 server 声明了对应 capability 时使用,低声明的 legacy server 依然可见。

本 PR 此前的所有阻塞项在当前 head 上均已修复并有测试: 我 2026-08-12 提出的 mcp_app 跨界面问题(TUI 改为渲染 fallbackText、历史压缩丢弃 html/toolResult,均有防泄漏探针测试);jifeng 2026-08-19 的两个 Critical(qwen mcp list 改用会话同款工厂 + 10 秒预算 + 去掉 ping,headless JSON 输出映射到 fallbackText);doudouOUC 唯一保留的 Critical R2-15(visibility: null 现在与 undefined 一致视为可见)。

安全面站得住,且比上一轮更硬frame-src 从通配环回端口收紧为钉死 daemon Host 端口(有测试);CSP 域名走严格 ASCII 源校验,; 注入与 Unicode 同形字(含 ſ 用例)均被丢弃(有测试);超长 CSP query 回退到全锁死策略;双层 iframe 均为 allow-scripts allow-forms 且不带 allow-same-origin,App HTML 在 opaque origin 下运行,读不到 WebShell 存储、也无法以同源身份访问 daemon API(API 仍走 bearer token,预授权的 sandbox 路由只提供静态、no-store 的代理 HTML);resource 读取有界(精确 MIME、1 MiB、10 秒、可中止),处处文本回退。wenshao 在本 head 上的浏览器级探针证实了端到端隔离,持久化 transcript 检查证实 HTML 不进入模型上下文。

非阻塞记录项:PR 描述里"自动协商"与硬刷新后 App 可见两处表述已过时(diff 内文档正确,只需更新描述);wenshao 的 O2 建议在 docs/users/features/mcp.md 补一句——modern server 若在 resources/read 结果中省略 ttlMs/cacheScope 会静默降级为文本;doudouOUC 排序靠前的两条 Suggestion 在当前 head 仍未处理(压缩保留 toolArguments/csp 的 R2-12;hostOrigin 仅校验主机名不钉端口)——后者我看过利用形态:伪造的 host origin 只能得到按 daemon 源隔离的独立 sandbox 实例,碰不到真实会话的 bearer 网关与存储,属于 follow-up 加固,不构成合并阻塞;另有一条互操作严格性记录——visibility 必须是包含 "model" 的数组,发裸字符串的 server 会被过滤(有 hadVisibilityFilteredTools 兜底,发现流程不会失败)。

测试证据

这是无人值守的 CI 运行,按门禁规则不构建、不执行任何 PR 代码;以上证据是通过 API 读取的该 PR 在受审提交上的自有 CI。54c2092 上全部 6 个 pull_request 事件工作流均为 success(Qwen Code CI、SDK Java、Web-shell Visuals、Serve A/B、Security Checks、Qwen Live Host CI)。ubuntu 单测——真正编译并运行改动包的那一档——在本 head 为绿,关闭了我 2026-08-12 的 CI 阻塞。macOS/Windows 测试档与无沙箱 CLI 集成套件是本仓库矩阵中的 skipped(非失败);唯一的非绿是 cancelled 的 route,属于 bot 编排任务,不是 PR CI。

未在此验证:Windows 行为(无 CI 档、也无人工报告);真实 server 进程下的协商 wire 路径(CI 单测用 mock transport 钉住协议矩阵)。后一点在本 head 上有两个更强的信号:wenshao 2026-08-22 在 Linux 上的独立 A/B 验证(维护者亲自执行——真实 stdio server、wire tap、真实 Chromium,结论为无阻塞),以及维护者触发、此刻正在运行@qwen-code /verify(run 32617423836)。沙箱验证将解决剩余的行为性声明——自动协商的探测/回退与 App 回退路径相对 base 构建是否真正承重——运行完成后报告会单独发布。

Qwen Code · qwen3.8-max

Reviewed at 54c20920d8caa7a9a5403e188bef08b0ff85bc59 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — every prior blocker is fixed and tested at this head, CI is fully green on it, and what remains is hygiene and hardening follow-ups, not merge blockers.

Stepping back: this PR grew through eight review rounds into something genuinely solid. The approach matches my independent proposal and exceeds it where it counts — SDK v2 owns the wire compatibility instead of a hand-rolled probe, and the lenient discovery behavior this codebase deliberately maintains for under-declared servers survived the migration intact. The two things that made me defer in 2026-08-12 are closed with probe-style tests (the mcp_app object no longer leaks into the TUI or session history), jifeng's two Criticals are closed, and doudouOUC's last held Critical (the visibility: null trap) is fixed at this head. The security-critical layer — the thing this PR most needed to get right — reads well: opaque inner origin, port-pinned framing, strict CSP validation, bounded reads, text fallback everywhere a failure can happen. The compatibility posture is the right one for a first slice: nothing is forced onto the modern protocol, and the default stdio path is byte-for-byte the old flow. If I had to maintain this in six months, the modern/legacy era split is the standing cost, and it's commented well enough to live with. The deferred scope (MRTR, App-initiated calls, remote negotiation, internal-server migration) is documented in the design doc, not lost.

Why not 5/5: the PR description is stale in two places (it still describes automatic stdio negotiation and post-refresh App visibility that don't match the shipped opt-in/text-replay behavior), and two cheap-but-real follow-ups ride along — the compaction spread retaining toolArguments/csp, and the unpinned hostOrigin port — plus a doc note for the strict resources/read schema. None of that blocks merge; per the round-count guidance they belong in a tracked follow-up issue so nothing is silently dropped. Windows remains untested by anyone (CI leg skipped); the unit suite is pinned on ubuntu only. The live-behavior gap is covered as well as it can be pre-merge: wenshao's independent A/B verification at exactly this head found no blockers, and the maintainer-triggered /verify run is in flight now and will post its own report.

Verdict: approving, pinned to the reviewed commit. The approval guardrail check passes (cross-repo but feat, not refactor), CI is settled green on the reviewed head, and no maintainer escalation is open.

中文说明

置信度:4/5 —— 此前的所有阻塞项在当前 head 上均已修复并有测试,CI 在该提交上全绿;剩下的只是卫生项与加固类 follow-up,不构成合并阻塞。

退一步看:这个 PR 经过八轮评审,已经打磨得相当扎实。方案与我独立的设想一致,且在关键处更好——wire 兼容性交给 SDK v2 而不是手写探测;本仓库刻意为低声明 server 保留的宽松发现行为在迁移后完整保留。让我在 2026-08-12 暂缓的两件事已用探针式测试关闭(mcp_app 对象不再泄漏进 TUI 或会话历史);jifeng 的两个 Critical 已关闭;doudouOUC 唯一保留的 Critical(visibility: null 陷阱)在当前 head 已修复。安全关键层——本 PR 最需要做对的部分——经得起读:内层 opaque origin、钉端口的 framing、严格的 CSP 校验、有界读取、处处文本回退。兼容姿态对第一个切片而言是正确的:没有任何东西被强制切到 modern 协议,stdio 默认路径与旧流程逐字节一致。六个月后接手维护的话,modern/legacy 双时代是长期成本,但注释写得足够清楚。被推迟的范围(MRTR、App 主动调用、远程协商、内部 server 迁移)都记录在设计文档里,没有丢。

为什么不是 5/5:PR 描述有两处过时(仍描述 stdio 自动协商、以及硬刷新后 App 可见,与实际 shipped 的 opt-in / 文本回放行为不符);另有两条便宜但真实的 follow-up——压缩时的 spread 仍保留 toolArguments/csphostOrigin 未钉端口——外加一条关于严格 resources/read schema 的文档提示。这些都不阻塞合并;按评审轮次的指引,应放入一个跟踪 issue,避免被悄悄丢弃。Windows 仍无人验证(CI 档被跳过),单测只在 ubuntu 一档被钉住。行为性缺口在合并前已被尽力覆盖:wenshao 在本 head 上的独立 A/B 验证未发现阻塞项,维护者触发的 /verify 正在运行,完成后会单独发布报告。

结论:批准,并钉在受审提交上。批准护栏检查通过(跨仓库但类型为 feat 而非 refactor),CI 在受审 head 上已稳定为绿,没有未关闭的维护者升级事项。

Qwen Code · qwen3.8-max

Reviewed at 54c20920d8caa7a9a5403e188bef08b0ff85bc59 · re-run with @qwen-code /triage

@samuelhsin samuelhsin left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Request changes to Comment: self-PR; CI failing: Test (ubuntu-latest, Node 22.x). ⚠️ This run could not certify that any of this diff was reviewed. Suggestions are inline.

Not reviewed: reverse audit — Cursor agent fan-out timed out; not re-run at 20-auditor scale.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not reviewed: coverage — could not read the agents' transcripts (the CLI did not export QWEN_CODE_PROJECT_DIR / QWEN_CODE_SESSION_ID, so this run cannot find the harness's record of what its agents did), so this run cannot show that any of the diff was read.

Not reviewed: verification — could not check that Step 4 and Step 5 ran (the CLI did not export QWEN_CODE_PROJECT_DIR / QWEN_CODE_SESSION_ID, so this run cannot find the harness's record of what its agents did).

中文说明

⚠️ 已从请求修改降级为评论:self-PR; CI failing: Test (ubuntu-latest, Node 22.x)。 ⚠️ 本次运行无法证明这个 diff 的任何部分经过了审查。 建议见行内评论。

未审查:reverse audit — Cursor agent fan-out timed out; not re-run at 20-auditor scale。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未审查:覆盖情况——无法读取 agent 的运行记录(the CLI did not export QWEN_CODE_PROJECT_DIR / QWEN_CODE_SESSION_ID, so this run cannot find the harness's record of what its agents did),本次运行无法证明 diff 的任何部分被读过。

未审查:验证——无法检查步骤 4 与步骤 5 是否运行(the CLI did not export QWEN_CODE_PROJECT_DIR / QWEN_CODE_SESSION_ID, so this run cannot find the harness's record of what its agents did)。

— Cursor Grok 4.6 via Qwen Code /review (v0.21.10)

Comment thread packages/core/src/tools/tools.ts
Comment thread packages/core/src/tools/mcp-client.ts Outdated
Comment thread packages/core/src/tools/mcp-tool.ts Outdated
Comment thread packages/web-shell/client/components/messages/McpApp.tsx Outdated
Comment thread packages/web-shell/client/components/messages/McpApp.tsx Outdated

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: end-to-end trace of how frequently the daemon transcript store performs a full block-object reseed mid-session (reconnect/resync/repair) in packages/sdk-typesc…; You are review agent reverse-audit — Reverse audit agen...: none — finished within budget.; chunk 6: full-repo npm run typecheck /build across cli and web-shell not run (their consuming code is outside my chunk anyway).; Change under review: This PR adds the first MCP 2026 clie...: none — all planned checks completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks completed within budget., and 12 more.

中文说明

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未探索到全部深度(达到工具调用预算):You are review agent reverse-audit — Reverse audit agen...:end-to-end trace of how frequently the daemon transcript store performs a full block-object reseed mid-session (reconnect/resync/repair) in packages/sdk-typesc…;You are review agent reverse-audit — Reverse audit agen...:none — finished within budget.;chunk 6:full-repo npm run typecheck /build across cli and web-shell not run (their consuming code is outside my chunk anyway).;Change under review: This PR adds the first MCP 2026 clie...:none — all planned checks completed within budget.;You are review agent reverse-audit — Reverse audit agen...:none — all checks completed within budget.,另有 12 条。

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment thread packages/core/src/tools/mcp-client.ts Outdated
Comment thread packages/web-shell/client/components/messages/McpApp.tsx Outdated
typeof content.text === 'string'
? content.text
: typeof content.blob === 'string'
? Buffer.from(content.blob, 'base64').toString('utf8')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-3: The 1 MiB host limit is enforced only after the resource body has been fully decoded into memory: Buffer.from(content.blob, 'base64').toString('utf8') allocates the full decoded payload before the size check throws. — Failure scenario: a malicious or buggy MCP server returns the app resource as a ~300 MB base64 blob → ~225 MB Buffer plus a second large string are allocated before the guard fires, a transient ~3x-payload memory spike despite the explicit MCP_APP_RESOURCE_MAX_BYTES guard (defense-in-depth, not a bypass). Suggested fix: check size before decoding — for the blob branch, reject when content.blob.length > Math.ceil(MCP_APP_RESOURCE_MAX_BYTES / 3) * 4 (base64 length upper-bounds decoded size); for the text branch, reject when content.text.length > MCP_APP_RESOURCE_MAX_BYTES.

中文说明

1 MiB 宿主上限只在资源体被完整解码进内存之后才执行:Buffer.from(content.blob, 'base64').toString('utf8') 会在大小检查抛错之前分配完整的解码负载。— 失败场景:恶意或有缺陷的 MCP 服务器以约 300 MB 的 base64 blob 返回 app 资源 → 在守卫触发前会分配约 225 MB 的 Buffer 和第二个大字符串,尽管有显式的 MCP_APP_RESOURCE_MAX_BYTES 守卫,仍会产生约 3 倍负载的瞬时内存峰值(这是纵深防御问题,不是绕过)。建议修复:在解码前检查大小——blob 分支当 content.blob.length > Math.ceil(MCP_APP_RESOURCE_MAX_BYTES / 3) * 4 时拒绝(base64 长度是解码大小的上界);text 分支当 content.text.length > MCP_APP_RESOURCE_MAX_BYTES 时拒绝。

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment thread packages/web-shell/client/components/messages/McpApp.tsx Outdated
Comment thread packages/web-shell/client/components/messages/ToolGroup.tsx Outdated
Comment thread packages/cli/src/serve/web-shell-static.ts
the model called that tool with the APAC region. Its reference implementation
and daemon configuration are included in the PR description.

![MCP App rendered in a WebShell transcript](../images/mcp-app-webshell.png)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-38: This PR commits a 2,323,039-byte (~2.2 MB) PNG screenshot into git — measured ~6x larger than the repo's current largest docs image (391 KB) and ~69x its two siblings in docs/images/ (~33 KB each), with no Git LFS in use (.gitattributes has no filter=lfs entries). — Failure scenario: PNGs barely delta-compress in packfiles, so every clone and fetch of the repository permanently carries ~2.2 MB of additional history from this single commit; every future replacement of the screenshot doubles the cost since the old blob stays in history. All ~20 other images under docs/ are under 400 KB, so this also breaks the repo's de-facto size convention for doc assets. Suggested fix: downscale to the rendered size needed by the doc and recompress (e.g., pngquant/oxipng, or convert to JPEG/WebP — the folder already mixes both); a screenshot of this content typically lands at 100–400 KB.

中文说明

本 PR 把一张 2,323,039 字节(约 2.2 MB)的 PNG 截图提交进了 git——实测约为仓库当前最大文档图片(391 KB)的 6 倍,是 docs/images/ 中两个同级图片(各约 33 KB)的约 69 倍,且未使用 Git LFS(.gitattributes 中没有 filter=lfs 条目)。— 失败场景:PNG 在 packfile 中几乎无法增量压缩,因此仓库的每次 clone 和 fetch 都会永久携带这一个提交带来的约 2.2 MB 额外历史;未来每次替换该截图都会使成本翻倍,因为旧 blob 仍留在历史中。docs/ 下其余约 20 张图片都小于 400 KB,这也打破了仓库对文档资源大小的事实约定。建议修复:按文档所需的渲染尺寸缩小并重新压缩(如 pngquant/oxipng,或转换为 JPEG/WebP——该文件夹本就两者混用);这类内容的截图通常在 100–400 KB。

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment on lines +700 to +702
html,
toolResult,
toolArguments: this.params,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-40: loadMcpAppDisplay caps html at 1 MiB but embeds the raw callToolResult as toolResult with no size budget at all, and no downstream compaction exists for the mcp_app shape — compactToolResultDisplay handles the other display types and then falls through to return resultDisplay, and the recording path returns the object verbatim. — Failure scenario: an app-backed MCP tool returns a multi-MB payload (e.g., a base64 screenshot in an image block); the html resource is small and passes the 1 MiB check, but toolResult carries the full payload into returnDisplay — written untouched into chat history (interactive path) and recordings, and streamed whole to the web shell. The 1 MiB host limit introduced in this same function protects only one of the two large fields it constructs. Suggested fix: apply a size budget to the embedded toolResult as well (or to the serialized display object as a whole) — e.g., drop/summarize binary data blocks beyond the budget the way transformMcpContentToParts already summarizes them for the model path.

中文说明

loadMcpAppDisplayhtml 限制在 1 MiB,但把原始 callToolResult 作为 toolResult 嵌入时没有任何大小预算,而且下游不存在针对 mcp_app 形状的压缩——compactToolResultDisplay 处理其他 display 类型后直接落到 return resultDisplay,录制路径也原样返回该对象。— 失败场景:app 类 MCP 工具返回多 MB 的负载(例如 image block 中的 base64 截图);html 资源很小并通过 1 MiB 检查,但 toolResult 会把完整负载带进 returnDisplay——未经处理地写入聊天历史(交互路径)和录制,并完整流式传输到 web shell。本函数中新引入的 1 MiB 宿主上限只保护了它所构造的两个大字段中的一个。建议修复:对嵌入的 toolResult 也应用大小预算(或对序列化后的整个 display 对象设限)——例如按预算丢弃/摘要超出的二进制 data block,就像 transformMcpContentToParts 为模型路径所做的那样。

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment thread packages/web-shell/client/components/messages/ToolGroup.tsx
Comment on lines 43 to +44
"@datafe-open/markdown-chart-react": "^0.1.12",
"@modelcontextprotocol/ext-apps": "^1.7.5",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-43: @modelcontextprotocol/ext-apps is bundled into the published web-shell lib — it is absent from rollupOptions.external in vite.lib.config.ts, where every other heavyweight dependency (mermaid, shiki, echarts, codemirror, react-markdown, katex, …) is explicitly externalized — pulling in its transitive zod/v4 and @modelcontextprotocol/sdk/shared/protocol.js imports, while also being declared here in dependencies, so npm/yarn consumers install it (plus its non-optional sdk ^1.29 and zod ^3.25||^4 peers) even though the bundled dist/index.js never imports it at runtime. — Failure scenario (verified in the built dist: app-bridge code inlined, 0 ext-apps import specifiers; app-bridge.js = 43 KB pre-min): ~100 KB+ minified added to every load of the embeddable widget — the exact cost the maintained external list exists to prevent — and embedders whose own app already ships zod download a second copy; consumers pay for the code twice. Nothing in the repo argues bundling is deliberate (no README mention); the uniform externalize-and-declare convention points the other way. Suggested fix: add @modelcontextprotocol/ext-apps, @modelcontextprotocol/sdk (+ subpath regex), and zod/zod/v4 to rollupOptions.external and keep them declared here — or, if bundling is a deliberate self-containment choice, drop this dependencies entry and note the reason.

中文说明

@modelcontextprotocol/ext-apps 被打包进了发布的 web-shell 库——它不在 vite.lib.config.tsrollupOptions.external 中,而其他每个重量级依赖(mermaid、shiki、echarts、codemirror、react-markdown、katex……)都被显式外部化——连带其传递依赖 zod/v4@modelcontextprotocol/sdk/shared/protocol.js 一起被打包,同时这里又在 dependencies 中声明了它,于是 npm/yarn 使用者会安装它(加上其非可选的 sdk ^1.29zod ^3.25||^4 peer),尽管打包后的 dist/index.js 在运行时从不导入它。— 失败场景(已在构建产物中验证:app-bridge 代码被内联,ext-apps 导入说明符为 0;app-bridge.js 压缩前 43 KB):可嵌入组件每次加载都多出约 100 KB+ 的压缩代码——正是那份维护着的 external 列表存在的意义所要避免的成本——自身应用已携带 zod 的嵌入者还会下载第二份;使用者为同一份代码付两次费。仓库中没有任何内容表明打包是刻意为之(README 未提及);统一的外部化+声明约定指向相反方向。建议修复:把 @modelcontextprotocol/ext-apps@modelcontextprotocol/sdk(+ 子路径正则)和 zod/zod/v4 加入 rollupOptions.external 并保留这里的声明——或者,如果打包是刻意的自包含选择,删除这条 dependencies 条目并注明原因。

— qwen3.8-max via Qwen Code /review (v0.21.10)

TUI and history compaction dumped mcp_app HTML as JSON, and discoverTools registered app-only tools for the model.

Co-authored-by: Cursor <cursoragent@cursor.com>

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head 609ffce against merge base f2de42d.

The overall direction is sound: SDK v2 automatic negotiation with a legacy raw-request fallback is the right compatibility shape, and the double-iframe design with an opaque inner origin is a reasonable isolation boundary. I do not think the current combined slice is ready to merge, though.

Blocking correctness issues:

  1. App-only tools are still registered for the model. I independently ran discoverTools with _meta.ui.visibility: ["app"]; the built code returned [{"name":"app_private","uri":"ui://demo/private"}]. This violates the Apps visibility contract and exposes UI-internal tools to the agent. Existing thread: #8992 (comment)
  2. mcp_app is added to the shared ToolResultDisplay union without defining its non-WebShell behavior. TUI paths fall through to JSON.stringify, while history and recording compaction return the object unchanged. A probe containing 1 MiB of HTML plus a 1 MiB tool result retained the same object in both paths at 2,097,336 serialized bytes. TUI/daemon-TUI should render fallbackText, and history/recording need an explicit bounded persistence shape. Existing thread: #8992 (comment)
  3. The sandbox permission/CSP contract is not yet enforced end to end. The WebShell Permissions-Policy blocks camera, microphone, and geolocation delegation; camera/microphone cannot work from the opaque inner origin; allow-forms has no matching form-action restriction; and the host CSP permits framing every loopback port instead of only the daemon port. These are especially important because the PR claims server-declared CSP and permissions are applied. Relevant threads: #8992 (comment), #8992 (comment), #8992 (comment), #8992 (comment)
  4. The AppBridge lifetime is keyed to the display object identity and theme, so a theme change or transcript reseed tears down the bridge, reloads the iframe, and loses application state. Keep the bridge keyed to stable resource fields and use setHostContext for theme changes. Existing thread: #8992 (comment)

At the solution level, #8968 triage explicitly recommended separating Core 2026 compatibility from the Apps host. Those are independent compatibility axes, and this PR currently combines a core client migration with a new untrusted-HTML host across core, CLI, daemon, and WebShell. Splitting remains the safer review and rollout plan; if it stays combined, the blockers above and committed tests for the real AppBridge/pre-auth route must be resolved first.

Validation at this head: the full repository build completed during clean install, workspace typecheck passed, targeted ESLint passed, and 251 core MCP + 2 CLI sandbox + 197 WebShell tests passed. Those tests do not exercise the failure paths above. The red Ubuntu Node job is an infrastructure download failure while installing actionlint, not a code failure. Also, git diff --check currently reports trailing whitespace in packages/vscode-ide-companion/NOTICES.txt:4164.

Theme toggles and transcript reseeds were tearing down MCP Apps; the host CSP also allowed any loopback port and form posts bypassed connect-src.

Co-authored-by: Cursor <cursoragent@cursor.com>
@samuelhsin
samuelhsin enabled auto-merge August 13, 2026 06:17
samuelhsin and others added 2 commits August 13, 2026 14:17
v2 typed helpers return [] without a request when a capability is omitted.
Use them only when the server declared the capability, and keep Apps
unmounted in collapsed tool rows.

Co-authored-by: Cursor <cursoragent@cursor.com>

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of PR #8992 — feat(mcp): add MCP 2026 core and WebShell Apps host

I've reviewed this PR in detail. This is a large, well-structured addition spanning 41 files (+4094/-415) across the core MCP client, CLI sandbox proxy, WebShell App host, and WebShell transcript UI.

Summary

No new Critical, security, or correctness issues found. The code is well-structured, thoroughly tested, and includes proper security considerations (double-iframe sandbox, CSP, opaque origin, loopback origin swapping, pre-auth route isolation).

Key observations

  1. R14-1 (silent legacy stdio timeout) appears resolved: MCP_CONNECT_TIMEOUT_MS increased from 5000 to 10000, with the code comment explaining the probe+fallback budget. The list.test.ts timeout test was updated to match.

  2. R15-3 (modern stdio ping failure) is resolved: The unconditional client.ping() call was removed from testMCPConnection in list.ts. Connect + version negotiation is now the liveness proof, with a code comment explaining why ping is absent from the 2026 request registry.

  3. R4-2 (CSP Unicode case-folding) is resolved: The sandbox test includes regression cases for https://K.example.com (U+212A KELVIN SIGN) and https://ſ.example.com (U+017F LONG S), verifying they are dropped.

  4. R2-24 (hardcoded http:// CSP origins): loopbackSandboxOrigins now generates both http:// and https:// variants — confirmed resolved.

  5. R2-6 ([::1] hostname swap): resolveMcpAppSandboxUrl rewrites [::1] to localhost before checking same-origin, and loopbackCrossOriginHostname maps both 127.0.0.1 and [::1] to localhost — confirmed resolved.

  6. R2-14 (v2 typed list helper schemas): ListToolsResultSchema, ListPromptsResultSchema, ListResourcesResultSchema are imported from @modelcontextprotocol/core and used only for the raw request fallback path — confirmed resolved.

Test coverage

Comprehensive: 589 lines of integration tests in mcp-client-v2.test.ts, 315 lines of DOM tests in McpApp.dom.test.tsx, 108 lines of unit tests in McpApp.test.ts, plus extensions to mcp-client.test.ts, mcp-tool.test.ts, list.test.ts, ToolGroup.test.tsx, ToolMessage.test.tsx, and more. The 65-page tool list pagination test (testing listMaxPages: 0) is particularly thorough.

One minor suggestion

Suggestion (low): The mcp-client-v2.test.ts "short discovery budgets" test (line 82-105) uses a real subprocess (process.execPath with --eval) inside a unit test file, making it an integration-style test. This is acceptable given the ~1250ms headroom in the timeout, but it could be made more hermetic with a mock send function similar to the other tests in this file.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

5 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • D17-3 blob decode / 1 MiB cap ordering (packages/core/src/tools/mcp-tool.ts:693) — already reported (comment 3770706441)
  • D17-9 unreachable mcp-app branch in hasExpandableContent (packages/web-shell/client/components/messages/ToolGroup.tsx:118) — already reported (comment 3789274493)
  • D17-10 trailing-slash pre-auth gate parity (packages/cli/src/serve/web-shell-static.ts:161) — already reported (comment 3774878199)
  • D17-11 sandbox load-failure path (packages/web-shell/client/components/messages/McpApp.tsx:248) — already reported (comment 3791892208)
  • D17-14 SSE/TCP legacy-guard test coverage (packages/core/src/tools/mcp-client.ts:379) — already reported (comment 3825920025)

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not reviewed: build-and-test — Test (macos-latest / windows-latest, Node 22.x) platform matrix was skipped in CI; unit suites ran green on ubuntu CI and locally.

Not reviewed: reverse audit — stopped before round 5 by the review time budget.

Deferred under the convergence posture (round 17, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/nonInteractive/io/BaseJsonOutputAdapter.ts:1423 (+1 locations) — [review] new helpers inserted between existing JSDoc blocks and their functions mis-attach the docs
  • packages/core/src/tools/mcp-client.ts:1803 — [probe] listMaxPages: 0 removes the pagination ceiling; unbounded cursors hang discovery
  • packages/core/src/tools/mcp-tool.ts:625 — [probe] raw tool result embedded in the app display bypasses the truncation budget end to end
  • packages/web-shell/client/components/messages/ToolGroup.tsx:1665 — [probe] auto-expand effect re-fires on single→multi tool growth and undoes the user's collapse
  • packages/cli/src/commands/mcp/list.test.ts:197 — [probe] 10s list budget certifies remotes that sessions reject at the 5s cap
  • packages/core/src/tools/mcp-client-v2.test.ts:84 — [probe] probe-timeout→legacy fallback behavior is untested; only the arithmetic is asserted
  • packages/core/src/utils/toolResultDisplayCompaction.ts:489 — [probe] compaction retains unbounded toolArguments while stripping the sibling fields
  • packages/cli/src/commands/mcp/list.ts:101 — [review] legacy-era servers that hang after initialize now report Connected (ping removed)
  • packages/core/src/tools/mcp-tool.ts:686 — [review] blob decode, size cap, and uri-miss branches of loadMcpAppDisplay are untested
  • packages/cli/src/serve/web-shell-static.ts:160 — [probe] no test guards the cold-gate pre-auth exemption for /mcp-app-sandbox
中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 5 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未审查:build-and-test — Test (macos-latest / windows-latest, Node 22.x) platform matrix was skipped in CI; unit suites ran green on ubuntu CI and locally。

未审查:反向审计——评审时间预算不足,未能开始第 5 轮。

收敛姿态下延后(第 17 轮,非阻断)——已记录,本轮不要求修改:共 10 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.21.15)

chiga0
chiga0 previously approved these changes Aug 22, 2026

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review against head 91cb725 — round 2 of my review.

Round-1 critical findings — all resolved

C1 — localhost same-origin: resolveMcpAppSandboxUrl now returns undefined when sandbox.origin === host.origin with no available alias; McpApp.tsx shows fallbackText when !sandboxUrl. ✅

C2 — IPv6 bracketed CSP origins: [::1] is rewritten to localhost before the origin-equality check; loopbackCrossOriginHostname maps both 127.0.0.1 and [::1] to localhost. ✅

C3 — client→server extension gate: App resource URI extraction reads tool._meta?.['ui']?.['resourceUri'] directly via getMcpAppResourceUri; no longer gated on getServerCapabilities().extensions. ✅

C4 — remote-legacy scope: Intentional design limit — remote HTTP/SSE/TCP stays on mode: 'legacy' because SDK v2 rejects HTTP probe timeouts with no initialize fallback. Design doc and inline comments state this. Acknowledged limitation.

C5 — deferred teardown race: mountGenerationRef generation counter guards iframe.removeAttribute('src')unload() checks mountGenerationRef.current === generation before removing the src, so a superseded cleanup cannot blank the next mount's live iframe. ✅

C6 — Unicode homoglyph CSP bypass: sanitizeCspDomains regex [a-z0-9.-] blocks non-ASCII; regression tests for U+212A (KELVIN SIGN) and U+017F (LONG S) added in mcp-app-sandbox.test.ts. ✅

One open suggestion (pre-existing, not a blocker)

applySandboxCspQuery (McpApp.tsx:96) silently drops the csp query parameter when the JSON exceeds MAX_SANDBOX_QUERY_LENGTH (8192 bytes), returning the sandbox URL without the server-declared CSP restrictions. Impact is limited because inner App isolation depends on the sandbox attribute, not the proxy-page CSP — but the silent fallback makes the divergence invisible to the caller. Pre-existing as CI-bot R5-6; deferred under the round convergence posture.

CI scope

Test (ubuntu-latest, Node 22.x) ✅ · Dependency CVE audit ✅ · Serve A/B ✅ · web-shell E2E Smoke ✅.
Not covered: Integration Tests (CLI, No Sandbox) SKIPPED — CLI integration behavior unverified. Test (macos-latest/windows-latest) SKIPPED — platform-specific path/locale behaviour unverified.

No blocking findings at this head.

Reviewed with AI assistance.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

Not reviewed: reverse audit — did not converge within the round cap of 5 (rounds 3-5 still reported findings).

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not reviewed: build-and-test — Test (macos-latest / windows-latest, Node 22.x) platform matrix was skipped in CI; unit suites ran green on ubuntu CI and locally.

Not explored to full depth (tool budget reached): chunk 10: could not execute toolResultDisplayCompaction.test.ts and mcp-transport-pool.test.ts — the shared review worktree has no node_modules installed, and I did….

Deferred under the convergence posture (round 18, not a blocker) — recorded, not requested in this round:

  • packages/core/src/tools/mcp-client.ts:1581 — [review] O(N^2) listedTools.find visibility scan x unbounded catalog aggregation — ~4.2s synchronous event-loop stall at 50k tools (measured); fix: one-time name index (14ms)
  • packages/core/src/tools/mcp-tool.ts:666 — [review] awaited app-resource readResource (up to 10s) blocks every tool result even on TUI/headless surfaces that discard the HTML
中文说明

仅完成部分审查,审查缺口已披露。

未审查:reverse audit — did not converge within the round cap of 5 (rounds 3-5 still reported findings)。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未审查:build-and-test — Test (macos-latest / windows-latest, Node 22.x) platform matrix was skipped in CI; unit suites ran green on ubuntu CI and locally。

未探索到全部深度(达到工具调用预算):chunk 10:could not execute toolResultDisplayCompaction.test.ts and mcp-transport-pool.test.ts — the shared review worktree has no node_modules installed, and I did…

收敛姿态下延后(第 18 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.21.15)

Comment on lines +382 to +385
const probeTimeoutMs = Math.min(
MCP_VERSION_NEGOTIATION_PROBE_TIMEOUT_MS,
discoveryTimeoutFor(cfg) - MCP_VERSION_NEGOTIATION_FALLBACK_HEADROOM_MS,
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] The fixed 5s silent-probe fallback headroom means silent legacy stdio servers whose startup+initialize handshake exceeds the residual window now time out as Disconnected, although they connected pre-PR. mcpVersionNegotiationFor sets the probe to min(5s, discoveryTimeout − 5s headroom); a silent server (never answers server/discover) burns the probe serially before the legacy initialize fallback starts, so the residual handshake budget is exactly 5000ms for any budget ≤ 10s and discoveryTimeout − 5000 above.

Failure scenario: a silent legacy stdio server configured with discoveryTimeoutMs: 8000 whose cold start + initialize takes 5.1s connected pre-PR (5.1s < 8s); post-PR the probe consumes 3s and the handshake gets 5s, so the connect rejects Timed out after 8000ms … The MCP server may be hung and the server is marked DISCONNECTED — misattributed as hung. At the default 30s budget the window is 25–30s handshakes. The same arithmetic applies in McpClientManager.runWithDiscoveryTimeout and pool spawn/doRestart.

Witness (A/B probe, same silent stdio fixture, both trees):

BASE (v1 SDK, no probe) delay=5100 d=8000 → CONNECTED in 5188ms
PR arm delay=5100 d=8000 → rejects /Timed out after 8000ms/ at 8005ms
PR arm delay=4000 d=8000 → CONNECTED in 7108ms (brackets the 5000ms residual)
PR arm delay=5100 d=30000 → CONNECTED in 10202ms

Suggested fix: shrink the probe proportionally (e.g. Math.floor(discoveryTimeoutFor(cfg) / 2)) or gate mode: 'auto' on a budget large enough for probe + a meaningful handshake, so silent legacy servers keep the pre-PR guarantee that their whole discovery budget is available for the handshake.

中文说明

固定的 5 秒静默 probe 回退余量意味着:启动 + initialize 握手超过剩余窗口的静默 legacy stdio 服务器,现在会超时并被报告为 Disconnected,而 PR 之前它们可以正常连接。mcpVersionNegotiationFor 把 probe 设为 min(5s, discoveryTimeout − 5s 余量);静默服务器(从不响应 server/discover)会在 legacy initialize 回退开始之前串行耗掉整个 probe,因此任何 ≤10 秒的预算下剩余握手预算恰好是 5000ms,更大预算下为 discoveryTimeout − 5000

失败场景:配置 discoveryTimeoutMs: 8000、冷启动 + initialize 共 5.1 秒的静默 legacy stdio 服务器,PR 之前可以连接(5.1s < 8s);PR 之后 probe 消耗 3 秒、握手只剩 5 秒,连接以 Timed out after 8000ms … The MCP server may be hung 失败,服务器被标记为 DISCONNECTED——被误判为挂起。默认 30 秒预算下的回归窗口是 25–30 秒的握手。同样的算术适用于 McpClientManager.runWithDiscoveryTimeout 以及连接池的 spawn/doRestart。

证据(A/B probe,同一静默 stdio fixture,两棵树):

BASE(v1 SDK,无 probe)delay=5100 d=8000 → CONNECTED,5188ms
PR 侧 delay=5100 d=8000 → 拒绝 /Timed out after 8000ms/,8005ms
PR 侧 delay=4000 d=8000 → CONNECTED,7108ms(框定 5000ms 剩余边界)
PR 侧 delay=5100 d=30000 → CONNECTED,10202ms

建议修复:按比例缩小 probe(例如 Math.floor(discoveryTimeoutFor(cfg) / 2)),或者仅在预算足以覆盖 probe + 有意义的握手时才启用 mode: 'auto',让静默 legacy 服务器保留 PR 之前的保证——整个发现预算都可用于握手。

— qwen3.8-max via Qwen Code /review (v0.21.15)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recorded as deferred for this late review round, following the maintainer recommendation to keep the default negotiation policy unchanged. The 8 s / 5.1 s failure remains reproducible in default auto mode, so this thread is not being marked fixed. The new per-server versionNegotiation: "legacy" opt-out gives affected legacy servers their full discovery budget; the same fixture connected successfully with the opt-out while default auto still timed out.

Comment thread packages/core/src/tools/mcp-client.ts Outdated
Comment on lines +379 to +381
if (cfg.httpUrl || cfg.url || cfg.tcp) {
return { mode: 'legacy' };
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] type: 'sdk' (in-process control-plane) server configs fall through to mode: 'auto', but SdkControlClientTransport has no stderr/pid, so the SDK's structural probe classifier treats it as non-stdio — where a probe timeout is a hard error with no legacy initialize fallback. That is the same SDK gap this diff's own disclosure names as the reason remotes stay on legacy.

Failure scenario: an SDK-side server that answers initialize but never answers server/discover (a busy control loop dropping or queueing unknown pre-initialize methods, or taking >5s) fails connect post-PR with Version negotiation probe timed out after 5000ms; pre-PR it connected via the plain v1 initialize handshake. Production-reachable: createSdkMcpServer in packages/sdk-typescript (both src/mcp/ and src/daemon-mcp/), AcpBridge, client-mcp-sender-registry and channel-worker-group all connect through McpClientcreateMcpClient with no bypass. Servers answering -32601 are unaffected; only silent/slow responders regress.

Witness (live probe through the real connectToMcpServer + SdkControlClientTransport):

silent SDK server → connect failed in 5002ms, methods seen: server/discover only
  rejects 'Version negotiation probe timed out after 5000ms'
control answering -32601 → connected in 3ms, era=legacy
Suggested change
if (cfg.httpUrl || cfg.url || cfg.tcp) {
return { mode: 'legacy' };
}
if (cfg.httpUrl || cfg.url || cfg.tcp || !cfg.command || isSdkMcpServerConfig(cfg)) {
return { mode: 'legacy' };
}
中文说明

type: 'sdk'(进程内 control-plane)服务器配置会落入 mode: 'auto',但 SdkControlClientTransport 没有 stderr/pid,SDK 的结构化 probe 分类器将其视为非 stdio——在该类别下,probe 超时是硬性错误,没有 legacy initialize 回退。这正是本 diff 自己的披露中用来说明远端保持 legacy 原因的那个 SDK 缺口。

失败场景:一个能响应 initialize 但从不响应 server/discover 的 SDK 侧服务器(繁忙的控制循环丢弃或排队未知的 pre-initialize 方法,或响应超过 5 秒),PR 之后连接失败:Version negotiation probe timed out after 5000ms;PR 之前它通过普通的 v1 initialize 握手连接。生产可达:packages/sdk-typescript 中的 createSdkMcpServersrc/mcp/src/daemon-mcp/)、AcpBridge、client-mcp-sender-registry、channel-worker-group 都经由 McpClientcreateMcpClient 连接,没有旁路。响应 -32601 的服务器不受影响;只有静默/慢响应者回归。

证据(通过真实 connectToMcpServer + SdkControlClientTransport 的现场 probe):

静默 SDK 服务器 → 连接失败,5002ms,所见方法仅 server/discover
  拒绝 'Version negotiation probe timed out after 5000ms'
响应 -32601 的对照 → 3ms 连接成功,era=legacy

建议补丁:把 type: 'sdk' / 无 command 的配置也钉在 legacy(见上方 suggestion 块)。

— qwen3.8-max via Qwen Code /review (v0.21.15)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 61d9e6d923. mcpVersionNegotiationFor now pins SDK and other commandless internal transports to legacy. The regression uses the real connectToMcpServer + SdkControlClientTransport path and asserts the exact method sequence initialize, notifications/initialized, with no server/discover. Independent post-fix verification connected in 13 ms with the same sequence.

Comment on lines +389 to +392
return {
mode: 'auto',
probe: { timeoutMs: probeTimeoutMs },
};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] Stdio auto-negotiation spawns a disposable sibling copy of the server process for the server/discover probe (SDK negotiateStdioViaSibling), so every negotiated stdio connection launches the server twice and runs its startup side effects twice — undisclosed in the design doc.

Failure scenario: a stdio server that takes a single-owner lockfile/PID file at startup can fail its session spawn entirely: the probe sibling creates/holds the file and is reaped (SIGTERM → 1s → SIGKILL) with no cleanup chance before the session child starts. Witness (E2E probe, single-owner PID-file server, default 30s budget):

BASE (v1 SDK, single spawn): CONNECTED in 89ms
PR arm: FAILED — 'Connection failed … Connection closed after 5113ms'
PID log for ONE connect: start pid=3374611 → server/discover pid=3374611 (sibling probes)
  → start pid=3374693 (session child) — two distinct spawns

Universal cost: every silent legacy stdio server pays two process startups plus double startup side effects per connect/pool-restart, and qwen mcp list double-spawns every configured stdio server too.

Suggested fix: disclose the double-spawn in the design doc and either document the unsupported server profile (startup side effects / single-owner resources) or provide an opt-out, e.g. a per-server versionNegotiation: 'legacy' override.

中文说明

stdio 自动协商会为 server/discover probe 派生一个一次性兄弟服务器进程副本(SDK negotiateStdioViaSibling),因此每一次协商的 stdio 连接都会启动服务器两次、执行两次启动副作用——设计文档未披露。

失败场景:启动时获取单所有者 lockfile/PID 文件的 stdio 服务器,其会话派生可能彻底失败:probe 兄弟进程创建/持有该文件,并在会话子进程启动前被回收(SIGTERM → 1 秒 → SIGKILL),没有清理机会。证据(E2E probe,单所有者 PID 文件服务器,默认 30 秒预算):

BASE(v1 SDK,单次派生):CONNECTED,89ms
PR 侧:失败 —— 'Connection failed … Connection closed after 5113ms'
单次连接的 PID 日志:start pid=3374611 → server/discover pid=3374611(兄弟 probe)
  → start pid=3374693(会话子进程)—— 两次不同的派生

普遍成本:每个静默 legacy stdio 服务器在每次连接/连接池重启时都要付出两次进程启动 + 双倍启动副作用;qwen mcp list 也会对每个配置的 stdio 服务器双派生。

建议修复:在设计文档中披露双派生行为,并明确不支持的服务器形态(启动副作用/单所有者资源),或提供退出开关(例如按服务器配置 versionNegotiation: 'legacy')。

— qwen3.8-max via Qwen Code /review (v0.21.15)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independently reproduced at 0c6ffaa against the pinned @modelcontextprotocol/client@2.0.0 — and I think this is worse than "undisclosed cost".

The double spawn is real (one connect(), silent legacy stdio server):

spawn count: 2 | distinct pids: 2
start pid=4264 → recv pid=4264 method=server/discover    (probe sibling)
start pid=4880 → recv pid=4880 method=initialize         (session child)

The failure mode splits on whether the resource is OS-reclaimed. A port-bound server survives, because the kernel frees the port when the sibling is killed:

BIND-OK pid=1497 → SIGTERM pid=1497 → BIND-OK pid=1690   CONNECTED in 6123ms (vs 114ms pre-PR)

A lockfile is not reclaimed, so the sibling's SIGKILL leaves it behind and the session child cannot start:

mode:'legacy' (pre-PR):  CONNECTED in 148ms    ["LOCK-OK pid=4763"]
mode:'auto'   (this PR): FAILED in 5106ms — "Connection closed"
                         ["LOCK-OK pid=4911", "LOCK-FAIL pid=6277 code=EEXIST"]

So the affected profile is startup side effects that aren't OS-reclaimed — lockfiles, PID files, registry check-ins, usage counters — and for those a working configuration becomes a permanent connect failure, not just a doubled cost. That reads to me as beyond what a design-doc note can cover; the per-server versionNegotiation: 'legacy' opt-out you suggest would at least give affected users a way out.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 61d9e6d923. Configured stdio servers can now set versionNegotiation: "legacy"; the value is preserved through settings, ACP, SDK/control/daemon and Desktop round-trips, and is included in both the transport-pool fingerprint and approval hash. The design and user docs disclose the sibling process. A real child-process verification observed two distinct PIDs in default auto mode and exactly one PID with the legacy opt-out.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with @qwen-code /review. See workflow logs.

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review at 0c6ffaa — cannot approve; 3 open Criticals verified as still standing

I re-checked every unresolved Critical on this PR against the code as it stands. packages/core/src/tools/mcp-client.ts and packages/cli/src/commands/mcp/list.ts are byte-identical between 91cb725 (the last automated review) and this head — git diff 91cb725 0c6ffaa over those two files is empty — so the round-18 findings anchor cleanly here.

Witnesses below were run against the real pinned @modelcontextprotocol/client@2.0.0 installed standalone (not through the repo build), reproducing this PR's exact construction: the base StdioClientTransport from @modelcontextprotocol/client/stdio plus versionNegotiation: mcpVersionNegotiationFor(cfg).

Still stands

C1 — stdio auto-negotiation double-spawns every server, and single-owner-resource servers now fail outright (mcp-client.ts:392, thread …348)

Confirmed, and more severe than reported. The SDK documents the sibling probe as "one extra spawn per connect". One connect() against a silent legacy stdio server:

OUTCOME: CONNECTED era=2025-06-18   elapsedMs: 5138
spawn count: 2 | distinct pids: 2
start pid=4264 → recv pid=4264 method=server/discover    (probe sibling)
start pid=4880 → recv pid=4880 method=initialize         (session child)

Beyond the cost, a server holding a single-owner lockfile goes from working to permanently broken: the sibling is SIGKILL-reaped without running its cleanup path, so the lock outlives it.

mode:'legacy' (pre-PR):  CONNECTED in 148ms    ["LOCK-OK pid=4763"]
mode:'auto'   (this PR): FAILED in 5106ms — "Connection closed"
                         ["LOCK-OK pid=4911", "LOCK-FAIL pid=6277 code=EEXIST"]

A port-bound server survives, since the OS frees the port on kill (BIND-OKSIGTERMBIND-OK, connected in 6123ms vs 114ms pre-PR). So the blast radius is startup side effects that are not OS-reclaimed: lockfiles, PID files, registry check-ins, usage/telemetry counters — anything non-idempotent. Separately, every stdio server pays two spawns per connect and pool-restart, and qwen mcp list double-spawns every configured server.

Because a working configuration can now fail permanently, I don't think documentation alone closes this. A per-server versionNegotiation: 'legacy' opt-out would at least give affected users a way out.

C2 — type: 'sdk' configs get mode: 'auto' but have no fallback (mcp-client.ts:381, thread …340)

Verified end to end. mcpVersionNegotiationFor returns legacy only for httpUrl || url || tcp, so a type: 'sdk' config falls through to auto with a 5s probe (discoveryTimeoutFor yields the 30s stdio default, then min(5000, 25000)). SdkControlClientTransport declares neither stderr nor pid, and the SDK classifies structurally:

// @modelcontextprotocol/client@2.0.0
function detectProbeTransportKind(transport) {
  return "stderr" in transport && "pid" in transport ? "stdio" : "http";
}
// classifyProbeOutcome, case "timeout":
if (context.transportKind === "stdio") return { kind: "legacy" };
return { kind: "error", error: new SdkError(RequestTimeout, ...) };  // no initialize

The transport is therefore treated as HTTP, where a probe timeout is terminal. A transport of exactly that shape:

### SILENT on server/discover  → REJECTED in 5004ms
    SdkError: Version negotiation probe timed out after 5000ms
    methods seen by server: ["server/discover"]    (initialize never sent)
### answers -32601             → CONNECTED in 14ms

Production-reachable: McpClient's constructor calls createMcpClient(name, this.serverConfig) unconditionally, and mcp-client-manager routes SDK servers through that same path with sendSdkMcpMessage. The trigger is narrow — servers answering -32601 are unaffected, only silent or >5s responders regress — but this is precisely the SDK gap the diff cites as its reason for keeping remotes on legacy, so the guard should cover type: 'sdk' as well. The one-line fix suggested in that thread looks correct to me.

C3 — residual handshake budget (mcp-client.ts:385, thread …338)

Mechanism confirmed: the probe is serial and precedes the handshake (the 5138ms and 6123ms elapsed times above are probe-then-connect), so a silent stdio server's handshake window is discoveryTimeout − probe, not the full budget. I'd rate this below the other two — the band that actually regresses is a cold start landing in (budget − 5s, budget), and the headroom constant is a deliberate, documented tradeoff. Reasonable to defer.

Cleared — unresolved threads that no longer stand

Ten Criticals still read isResolved: false but are fixed at this head; flagging so they don't keep re-surfacing as blockers:

Thread Verdict at 0c6ffaa
R14-1 mcp list 5s budget (…521, …523) fixed — MCP_CONNECT_TIMEOUT_MS = 10_000
R15-3 unconditional ping (…532) fixed — ping removed, liveness is connect + negotiation
listMaxPages regression (…010) fixed — listMaxPages: 0 present in createMcpClient
visibility filter failing the whole server (…021) fixed — hadVisibilityFilteredTools returned and consumed at :706 / :1399
4 outdated threads (web-shell-static :50/:53, mcp-client :369/:372) superseded by later commits
R15-1 remote AC1 (…025) stands as a disclosed scope limitation, not a code defect — agree with the earlier "intentional design limit" read; better handled by keeping #8968 open than by blocking this slice

Recommendation

Not approving. Under the convergence posture for a PR this far along, I'd land only C2 (one-line guard, and an inconsistency with the diff's own stated rationale) and C1 (a working configuration now fails permanently). C3 and the 84 open Suggestion-level threads are better handled as a follow-up.

Everything else I reviewed independently — the double-iframe isolation and postMessage origin checks, the sandbox CSP construction and pre-auth route gating, mcp-tool.ts's app-resource read path and 1 MiB cap, and the mcp_app compaction / TUI fallbacks — turned up no new blocking defects. The slicing and the security posture of the Apps host look sound to me.

Not verified by me: the CI legs skipped on this run (CLI integration, macOS/Windows matrix), and I did not run the repository's unit suites — my evidence is protocol-level against the pinned SDK plus code reading at this head.

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — COMMENT (not approving; C=1 carried, S=63 open)

Reviewed at 0c6ffaaf against merge-base 6bbb273a, as a /packages/core/ owner. Holding the approve on one live correctness item, with a note on process.

1. Where this sits under the two-tier rule

Per AGENTS.mdCore Infrastructure Is Maintainer-Only (two-tier rule):

  • author is not in .github/CODEOWNERS → external PR;
  • it touches packages/core/src/tools/** → core module;
  • ~1177 production (non-test, non-generated) added lines → past the 1000-line advisory;
  • but it is feat, not refactornot hard-blocked.

So this is a maintainer-review PR rather than an auto-gate one, and the above is my maintainer review. Recording the classification explicitly because the advisory threshold matters for how the remaining Suggestions should be handled (§3) — a new server-HTML-in-an-iframe trust boundary inside core earns the slower path.

To be clear about what this is not: it is not a rejection, and not a size complaint. The work is substantial and most of the earlier Criticals are genuinely fixed at this commit — I re-checked and confirmed fixed: the loopbackSandboxOrigins scheme coverage (R2-24), the CSP query-overflow guard now measuring the serialized wire length (R2-30, R4-3), the unconditional appResourceUriMap population (R4-1), CSP_SOURCE_PATTERN's flag set (R4-2), the probe-timeout headroom (R5-1), the explicit legacy mode for remote transports, and the removal of the permissions echo from the AppBridge host capabilities (R2-7).

2. The one item holding the approve, and it is a one-liner

packages/core/src/tools/mcp-client.ts:437-441 — already filed as R2-15 and still unresolved; I have replied on that thread with the verification rather than opening a duplicate. Summary: visibility: null is classified as hidden, so tools from any server that serializes unset optionals as null (Python pydantic model_dump() without exclude_none is the common one) silently vanish from the model's toolset. The function's own preceding line — if (visibility === undefined) return true; — establishes that unset means visible, so null contradicts the author's own stated intent, not merely an external spec. One line fixes it.

Given the round count, I would land that fix only and nothing else.

3. On the remaining 63 open threads

AGENTS.md is directive here: "Once a PR has been through roughly 5 review rounds, land only Critical fixes — correctness, security, data loss, regressions — and defer remaining Suggestions to a follow-up issue or PR. Record each deferral in the PR thread so nothing is silently dropped."

This PR is at 84 commits and 7+ rounds. Continuing to absorb Suggestions is what has been widening the diff and triggering the next round. My recommendation: open one tracked follow-up issue, move the open Suggestions into it, and link it here so nothing is dropped silently. By theme, the open threads are roughly:

Theme Open
Test-efficacy gaps (hunk-survived probes, missing negative cases) 18
Web Shell UX / lifecycle (auto-expand, unmount ordering, compacted rendering) 12
Sandbox hardening (form-action, IPv6 CSP literals, permissions surface, proxy forwarding) 10
Compaction / memory budget (toolArguments, csp arrays retained via spread) 3
Design doc & image assets 2
Other (CSS motion, a11y, dev workflow) 5

Two of those I would personally rank above the rest when triaging the follow-up, because they are cheap and sit on the security/memory boundary: compactMcpAppResultDisplay's ...display spread retaining unbounded toolArguments (R2-12), and the self-declared hostOrigin trust anchor being validated by hostname only with the port unpinned — any loopback origin on any port can claim to be the host.

4. CI note

The Desktop Shell check reads as failing, but the job was cancelled (conclusion: cancelled, zero steps executed, started_at == completed_at) — it is infrastructure, not a test failure, and gh pr checks renders cancelled as fail. Worth a re-run so the signal is clean before a maintainer looks. review-pr is still pending.

中文说明

0c6ffaaf 上以 /packages/core/ owner 身份复核,暂不 approve

  1. 两级规则下的定位:作者不在 CODEOWNERS(外部 PR)+ 触及 packages/core/src/tools/**(核心模块)+ 约 1177 行生产代码(超过 1000 行提示线),但类型是 feat 而非 refactor,因此不构成硬阻断,属于需要 maintainer 人工评审的 PR——以上即为我作为 /packages/core/ owner 的评审。明确记录该分类,是因为该提示线决定了剩余 Suggestion 的处理方式(见第 3 点)。这不是拒绝,也不是嫌改动大——多数早期 Critical 在本 commit 上确实已修复(R2-24 / R2-30 / R4-1 / R4-2 / R4-3 / R5-1 / R2-7 等我都已逐条核实)。

  2. 仍有一个确定的正确性问题mcp-client.ts:437-441visibility: null 判为隐藏,导致把未设置字段序列化为 null 的服务端(Python pydantic model_dump() 不带 exclude_none 是典型情况)其工具从模型工具集中静默消失。该函数前一行 if (visibility === undefined) return true; 已确立"未设置即可见",所以 null 与作者自己的意图矛盾,一行即可修复。这一项已作为 R2-15 存在,我在原线程回复而非新开重复评论。

鉴于轮次已多(84 commits、7+ 轮),建议只落这一个修复,其余 63 个未解决 Suggestion 按 AGENTS.md 的"5 轮后只落 Critical"规则转入一个可追踪的 follow-up issue 并在此链接,避免静默丢弃。

另:Desktop Shell 显示为 fail 实际是被 cancelled(零步骤执行),属基础设施问题而非测试失败,建议重跑以获得干净信号。

@samuelhsin
samuelhsin requested a review from doudouOUC August 22, 2026 10:21

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not explored to full depth (tool budget reached): chunk 1: executing packages/cli list.test.ts — the review worktree has no node_modules and a full npm ci exceeds the tool budget; verified by source reading instea….

Deferred under the convergence posture (round 19, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/commands/mcp/list.ts:33 (+1 locations) — [review] 10s connect budget applied to remote transports too
  • packages/cli/src/nonInteractive/io/BaseJsonOutputAdapter.ts:1420 (+2 locations) — [review] Inserted helpers misattach existing JSDoc blocks (2…
  • packages/cli/src/serve/mcp-app-sandbox.test.ts:83 (+1 locations) — [probe] Sandbox proxy inline-script trust model never executed by…
  • packages/cli/src/serve/mcp-app-sandbox.test.ts:88 (+1 locations) — [probe] Unmarked Unicode look-alike literals in security tests
  • packages/cli/src/serve/mcp-app-sandbox.ts:16 (+1 locations) — [probe] CSP query length cap basis mismatch between client and…
  • packages/cli/src/serve/web-shell-static.ts:161 (+1 locations) — [probe] Deferred-runtime gate path for /mcp-app-sandbox is untested
  • packages/cli/src/serve/web-shell-static.ts:180 (+1 locations) — [probe] Per-request frame-src wiring has no end-to-end test
  • packages/core/package.json:62 (+1 locations) — [review] Second zod major bundled alongside zod 3
  • packages/core/src/tools/mcp-client-v2.test.ts:29 (+1 locations) — [review] Test file name has no corresponding source file
  • packages/core/src/tools/mcp-client.ts:93 (+1 locations) — [review] Silent probe-timeout fallback path untested
  • packages/core/src/tools/mcp-client.ts:379 (+1 locations) — [probe] SSE/TCP branches of the remote-legacy guard untested
  • packages/core/src/tools/mcp-client.ts:382 (+1 locations) — [probe] Degenerate probe window for mid-size stdio budgets
  • packages/core/src/tools/mcp-tool.ts:208 (+1 locations) — [review] MCP App MIME type constant defined twice
  • packages/core/src/tools/mcp-tool.ts:681 (+1 locations) — [probe] Byte-exact MIME comparison rejects equivalent spellings
  • packages/core/src/tools/mcp-tool.ts:686 (+1 locations) — [review] 1 MiB guard runs after full base64 decode
  • packages/core/src/tools/mcp-tool.ts:693 (+1 locations) — [probe] 1 MiB guard and base64 blob decode branch untested
  • packages/core/src/tools/mcp-tool.ts:706 (+1 locations) — [review] Display embeds uncapped raw toolResult on the live path
  • packages/core/src/tools/mcp-tool.ts:1086 (+1 locations) — [probe] Unbounded server-controlled csp/permissions arrays survive…
  • packages/core/src/utils/toolResultDisplayCompaction.ts:493 (+1 locations) — [review] Compaction leaves toolArguments unbounded
  • packages/web-shell/client/components/WebShellTranscript.tsx:71 (+1 locations) — [probe] mcpAppBaseUrl prop fully unexercised
  • …and 11 more (see the run report)
中文说明

未探索到全部深度(达到工具调用预算):chunk 1:executing packages/cli list.test.ts — the review worktree has no node_modules and a full npm ci exceeds the tool budget; verified by source reading instea…

收敛姿态下延后(第 19 轮,非阻断)——已记录,本轮不要求修改:共 31 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.21.15)

Comment thread packages/core/src/tools/mcp-client.ts Outdated
Comment on lines +379 to +381
if (cfg.httpUrl || cfg.url || cfg.tcp) {
return { mode: 'legacy' };
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R18-2: Round-18 blocker still standing — type: 'sdk' (in-process control-plane) server configs fall through to mode: 'auto', but SdkControlClientTransport has no stderr/pid, so the pinned SDK's structural classifier treats it as non-stdio — where a probe timeout is a hard error with no legacy initialize fallback: the exact SDK gap this diff's own comment names as the reason remotes stay on legacy.

Failure scenario: a type: 'sdk' server whose control plane does not answer server/discover within 5s (the SDK process busy at session startup so the control_request queues behind, or a non-TypeScript control plane that doesn't answer unknown pre-initialize methods with a JSON-RPC error) connected pre-PR via straight initialize, but now connect() throws "Version negotiation probe timed out after 5000ms" and the server is permanently Disconnected. Reachable via createSdkMcpServer, AcpBridge, channel-worker-group, and the desktop mcp-pool.

Witness (probe with a type:'sdk' control plane that drops server/discover but answers initialize):

auto mode: rejected after 5004 ms; methods seen: [ 'server/discover' ] (initialize never sent)
forced legacy: connect resolves
Suggested change
if (cfg.httpUrl || cfg.url || cfg.tcp) {
return { mode: 'legacy' };
}
if (cfg.httpUrl || cfg.url || cfg.tcp || isSdkMcpServerConfig(cfg)) {
return { mode: 'legacy' };
}
中文说明

第 18 轮阻断项仍然存在:type: 'sdk'(进程内控制面)服务器配置会落入 mode: 'auto',但 SdkControlClientTransport 没有 stderr/pid,因此锁定的 SDK 的结构化分类器将其视为非 stdio——而探测超时对非 stdio 是硬错误,没有 legacy initialize 回退:这正是本 diff 自身注释中说明"远程服务器保持 legacy"的那个 SDK 缺口。

失败场景:控制面在 5 秒内不响应 server/discovertype: 'sdk' 服务器(会话启动时 SDK 进程繁忙、control_request 排队在后;或不对未知 pre-initialize 方法返回 JSON-RPC 错误的非 TypeScript 控制面)在 PR 前通过直接 initialize 连接,现在 connect() 抛出 "Version negotiation probe timed out after 5000ms",服务器永久 Disconnected。可经 createSdkMcpServerAcpBridgechannel-worker-group 和桌面端 mcp-pool 触达。

见证(使用丢弃 server/discover 但响应 initializetype:'sdk' 控制面的探针):

auto 模式:5004 毫秒后被拒绝;所见方法:[ 'server/discover' ](initialize 从未发送)
强制 legacy:连接成功

建议的代码修复见上方英文部分的 suggestion 代码块(将 isSdkMcpServerConfig(cfg) 纳入 legacy 守卫,该符号在本文件中已导入)。

— qwen3.8-max via Qwen Code /review (v0.21.15)

Comment on lines +382 to +385
const probeTimeoutMs = Math.min(
MCP_VERSION_NEGOTIATION_PROBE_TIMEOUT_MS,
discoveryTimeoutFor(cfg) - MCP_VERSION_NEGOTIATION_FALLBACK_HEADROOM_MS,
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R18-1: Round-18 blocker still standing — the silent server/discover probe runs serially inside the fixed discovery window before the legacy initialize fallback, permanently subtracting up to 5s from a silent legacy stdio server's startup+handshake budget. Issue #8968 AC7 ("Legacy MCP tool-only servers continue to work") regresses for servers whose cold start fit the window pre-PR but not window−probe.

A configured stdio server that never answers server/discover and whose startup+initialize takes ~27s under the default 30s window (or ~6.5s under an 8s discoveryTimeoutMs override, where the probe shrinks to 3s) connected pre-PR with the full window available; post-PR the probe burns its budget first, the discovery-window timeout fires first, and the server is reported Disconnected.

Witness (probe against a silent legacy stdio server at this commit, 6s window):

auto mode: {"ok":false,"elapsedMs":6005,"error":"Timed out after 6000ms: probe-b connect..."} spawns: 2
forced legacy (pre-PR behavior), same input: {"ok":true,"era":"legacy","elapsedMs":5607} spawns: 1

Suggested fix: make the probe budget a fraction of the discovery window instead of a fixed window−5s headroom (or skip the probe below a window threshold that cannot cover probe + realistic cold start), and disclose the trade-off in the design doc's Compatibility section.

中文说明

第 18 轮阻断项仍然存在:静默的 server/discover 探测在固定的发现窗口内、在 legacy initialize 回退之前串行执行,从静默的 legacy stdio 服务器的启动+握手预算中永久扣除最多 5 秒。对于冷启动时间在 PR 前的窗口内、但不再适应"窗口−探测"时间的服务器,这回归了 issue #8968 AC7("Legacy MCP 纯工具服务器继续可用")。

失败场景:一个已配置的 stdio 服务器从不响应 server/discover,其启动+initialize 在默认 30 秒窗口下约需 27 秒(或在 8 秒 discoveryTimeoutMs 覆盖下约需 6.5 秒,此时探测缩短为 3 秒)。PR 前整个窗口都可用、能够连接;PR 后探测先耗尽预算,发现窗口超时先触发,服务器被报告为 Disconnected。

见证(在本提交下对静默 legacy stdio 服务器、6 秒窗口的探针):

auto 模式:{"ok":false,"elapsedMs":6005,"error":"Timed out after 6000ms: probe-b connect..."} spawns: 2
强制 legacy(PR 前行为),相同输入:{"ok":true,"era":"legacy","elapsedMs":5607} spawns: 1

建议修复:将探测预算改为发现窗口的一个比例,而不是固定的"窗口−5 秒"余量(或在窗口低于无法覆盖"探测+合理冷启动"的阈值时跳过探测),并在设计文档的兼容性部分披露该权衡。

— qwen3.8-max via Qwen Code /review (v0.21.15)

Comment on lines +389 to +392
return {
mode: 'auto',
probe: { timeoutMs: probeTimeoutMs },
};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R18-3: Round-18 blocker still standing — every stdio config resolves to mode: 'auto', and the pinned SDK's stdio negotiation probes server/discover on a disposable sibling copy of the server process spawned from the same command/args/env, reaps it, and only then starts the session child — so every negotiated stdio connection spawns the server twice and runs its startup side effects twice, undisclosed in the design doc.

Failure scenario: a configured stdio server that takes a single-owner lockfile/PID file, binds a fixed port, or enforces single-instance semantics at startup: the sibling spawn acquires the lock/port first, and the session spawn races the released lock or fails outright — a server that connected reliably pre-PR now fails intermittently (AC7). Even for tolerant servers, every connect/reconnect doubles startup cost (model loads, cache warm-up, network calls), and the sibling runs with stderr ignored, discarding the first process's diagnostics.

Witness (probe at this commit):

one successful auto-mode connect: spawns: 2 (disposable sibling probe + session child)
same input under forced legacy: spawns: 1

Suggested fix: disclose the double-spawn in the design doc's Compatibility section and gate stdio auto-negotiation behind an explicit config opt-in (default legacy) — or at minimum document it in mcpVersionNegotiationFor's comment so operators of single-instance servers can predict the behavior.

中文说明

第 18 轮阻断项仍然存在:每个 stdio 配置都会解析为 mode: 'auto',而锁定的 SDK 的 stdio 协商会用相同的命令/参数/环境变量生成一个一次性的服务器进程副本(sibling)来探测 server/discover,回收后再启动会话子进程——因此每次协商的 stdio 连接都会把服务器启动两次、执行两次启动副作用,且设计文档未披露。

失败场景:已配置的 stdio 服务器在启动时持有单所有者锁文件/PID 文件、绑定固定端口或强制单实例语义:sibling 进程先获得锁/端口,会话进程再去竞争已释放的锁或直接失败——PR 前能可靠连接的服务器现在会间歇性失败(AC7)。即使对宽容的服务器,每次连接/重连也会使启动成本翻倍(模型加载、缓存预热、网络调用),且 sibling 以忽略 stderr 的方式运行,丢弃第一个进程的诊断输出。

见证(在本提交下的探针):

一次成功的 auto 模式连接:spawns: 2(sibling 探测 + 会话子进程)
相同输入强制 legacy:spawns: 1

建议修复:在设计文档的兼容性部分披露双进程启动,并将 stdio 自动协商置于显式配置开关之后(默认 legacy)——至少应在 mcpVersionNegotiationFor 的注释中说明,让单实例服务器的运维者能够预期该行为。

— qwen3.8-max via Qwen Code /review (v0.21.15)

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with @qwen-code /review. See workflow logs.

@samuelhsin

Copy link
Copy Markdown
Collaborator Author

Post-fix verification (61d9e6d)

  • SDK control-plane MCP: real connect path sent initialize and notifications/initialized only; no server/discover.
  • Stdio legacy opt-out: real child-process fixture started exactly one PID; default auto mode still started the expected probe sibling plus session process (two PIDs).
  • Slow legacy 8 s fixture: default auto behavior remains unchanged and still reproduces the deferred timeout; explicit legacy connected successfully.
  • Focused tests: core 51/51, CLI SystemController 25/25, ACP settings 5/5, TypeScript SDK schema 81/81.
  • Repository build and workspace typecheck passed. Prettier, ESLint, diff checks, two clean self-audit passes, and an independent final review passed.

Desktop shared typecheck passed. Electron typecheck still reports only pre-existing errors in auto-update, settings-default-thinking tests, and MemorySettingsPage; it reports no error in the changed MCP settings file.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

Not explored to full depth (tool budget reached): chunk 6: could not execute the test file to confirm runtime behavior (no node_modules in this review worktree; npm ci + monorepo build exceeds the tool budget) — stati….

Not reviewed: reverse audit — stopped before round 5 by the review time budget.

Deferred under the convergence posture (round 20, not a blocker) — recorded, not requested in this round:

  • docs/developers/daemon/05-mcp-transport-pool.md:318 — [review] D20-1 rewritten fingerprint field list omits authProviderType/targetAudience/targetServiceAccount that fingerprint() hashes
  • packages/cli/src/nonInteractive/io/BaseJsonOutputAdapter.ts:1423 — [review] D20-2 mcpAppFallbackText inserted between toolResultContent's JSDoc and its declaration
  • packages/core/src/tools/mcp-client.ts:1446 — [review] D20-3 applyListingAppResourceUi inserted between discoverTools' JSDoc and its declaration
  • packages/web-shell/client/components/messages/ToolGroup.tsx:119 — [review] D20-4 hasExpandableContent's new mcp_app branch is unreachable dead code
  • packages/core/src/tools/mcp-tool.ts:693 — [review] D20-5 loadMcpAppDisplay's blob/1MiB/URI/readResource guards have no tests
  • packages/cli/src/serve/web-shell-static.ts:161 — [review] D20-6 cold deferred-runtime gate omits trailing-slash /mcp-app-sandbox/ variant
  • packages/core/src/utils/toolResultDisplayCompaction.ts:493 — [review] D20-7 compaction retains unbounded server-controlled toolArguments and csp arrays
  • packages/web-shell/client/components/WebShellTranscript.tsx:239 — [review] D20-8 public mcpAppBaseUrl prop and McpAppHostContext wiring untested
  • packages/core/src/tools/mcp-tool.ts:690 — [review] D20-9 1 MiB app-resource limit enforced only after fully decoding the blob
  • packages/core/src/tools/mcp-client.ts:380 — [review] D20-10 settings-file versionNegotiation values silently coerce to auto
  • packages/web-shell/client/components/MessageList.tsx:1537 — [review] D20-11 compacted mcp_app displays still pin replayed turns open
  • packages/web-shell/client/components/messages/ToolGroup.tsx:1471 — [review] D20-12 forced-expanded MCP App rows render the description nowhere
中文说明

仅完成部分审查,审查缺口已披露。

未探索到全部深度(达到工具调用预算):chunk 6:could not execute the test file to confirm runtime behavior (no node_modules in this review worktree; npm ci + monorepo build exceeds the tool budget) — stati…

未审查:反向审计——评审时间预算不足,未能开始第 5 轮。

收敛姿态下延后(第 20 轮,非阻断)——已记录,本轮不要求修改:共 12 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.21.15)

Comment on lines +390 to +393
const probeTimeoutMs = Math.min(
MCP_VERSION_NEGOTIATION_PROBE_TIMEOUT_MS,
discoveryTimeoutMs - MCP_VERSION_NEGOTIATION_FALLBACK_HEADROOM_MS,
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R18-1: Round-18 blocker still standing — the silent server/discover probe runs serially before the legacy initialize fallback and subtracts up to 5s from the discovery budget.

Re-checked at this commit (61d9e6d9) and probe-verified against the unmodified PR code: under default auto negotiation the probe takes its full 5s cap before the legacy handshake starts, so a stdio server whose legacy handshake lands in the top ~5s of its budget fails to connect while the identical versionNegotiation: "legacy" config connects. The per-server opt-out works and the trade-off is documented, but the default policy still regresses the (budget−5s, budget) band for slow-handshake servers that never opted in.

Witness (real stdio echo server, unmodified PR code):

ARM auto-T1000-defaultBudget:   CONNECTED elapsedMs=6047  <- probe costs ~5s
ARM legacy-T1000-defaultBudget: CONNECTED elapsedMs=1030  <- same server
ARM auto-T8000-B12000:   FAILED "Timed out after 12000ms" <- regressing band
ARM legacy-T8000-B12000: CONNECTED elapsedMs=8037          <- identical config connects

Consider keeping the stdio default legacy (opt-in auto), or racing the probe against the legacy initialize so a silent server does not serially consume the probe window.

中文说明

[Critical] R18-1:第 18 轮遗留的阻塞问题仍然存在——静默的 server/discover 探测在 legacy initialize 回退之前串行执行,最多会从发现预算中扣掉 5 秒。

已在当前提交(61d9e6d9)复查,并用未修改的 PR 代码做了探测验证:在默认 auto 协商下,探测会先占满 5 秒上限才开始 legacy 握手,因此 legacy 握手耗时落在预算最后约 5 秒区间的 stdio 服务器会连接失败,而配置相同但设置 versionNegotiation: "legacy" 的服务器可以连接。按服务器粒度的退出选项可用,且该取舍已在文档中说明,但默认策略仍会让未主动配置的慢握手服务器在 (预算−5秒, 预算) 区间内连接失败。

见证(真实 stdio echo 服务器,未修改的 PR 代码):

ARM auto-T1000-defaultBudget:   CONNECTED elapsedMs=6047  <- 探测消耗约 5 秒
ARM legacy-T1000-defaultBudget: CONNECTED elapsedMs=1030  <- 同一服务器
ARM auto-T8000-B12000:   FAILED "Timed out after 12000ms" <- 回退区间
ARM legacy-T8000-B12000: CONNECTED elapsedMs=8037          <- 相同配置可连接

建议将 stdio 默认保持为 legacy(auto 改为显式启用),或让探测与 legacy initialize 并行竞争,避免静默服务器串行消耗探测窗口。

— qwen3.8-max via Qwen Code /review (v0.21.15)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 54c2092. Configured stdio now defaults to legacy, so an omitted setting cannot consume probe time from discoveryTimeoutMs. Only explicit versionNegotiation:auto opts into probing. The regression uses an 8s discovery budget and a 5.1s legacy initialize: the new default connects in about 5.4s with one process, while the old default-auto path first spent 3s probing and timed out. The focused core suite passes 51/51.

Comment on lines +397 to +400
return {
mode: 'auto',
probe: { timeoutMs: probeTimeoutMs },
};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R18-3: Round-18 blocker still standing — every default stdio config resolves to mode: 'auto', which spawns a disposable sibling copy of the configured command for the probe.

Re-checked at this commit (61d9e6d9) and probe-verified: mcpVersionNegotiationFor resolves every stdio config without an explicit versionNegotiation: "legacy" to auto, and the SDK runs server/discover in a short-lived sibling process spawned from the same command before the session transport starts. Measured with a counting server that logs one line per process start:

MODE auto:   era=legacy processStarts=2  (pids 3438881, 3438948)
MODE legacy: era=legacy processStarts=1  (pid 3439329)

Every default stdio connection therefore starts the configured command twice. Stdio servers that hold a lockfile/PID file or have non-idempotent startup side effects — the exact case the new user docs name as the reason to opt out — break or contend on every connection unless individually configured with versionNegotiation: "legacy".

Consider defaulting stdio to legacy (opt-in auto) or reusing the session transport for the probe instead of spawning a sibling, so single-owner/non-idempotent servers are safe by default.

中文说明

[Critical] R18-3:第 18 轮遗留的阻塞问题仍然存在——所有默认 stdio 配置都会解析为 mode: 'auto',从而为探测生成一个由相同命令启动的一次性兄弟进程副本。

已在当前提交(61d9e6d9)复查并探测验证:mcpVersionNegotiationFor 将所有未显式设置 versionNegotiation: "legacy" 的 stdio 配置解析为 auto,而 SDK 会在会话传输启动之前,用相同命令生成一个短生命周期的兄弟进程来执行 server/discover。用一个每次进程启动记录一行日志的计数服务器实测:

MODE auto:   era=legacy processStarts=2  (pid 3438881、3438948)
MODE legacy: era=legacy processStarts=1  (pid 3439329)

因此,每个默认 stdio 连接都会把配置的命令启动两次。持有锁文件/PID 文件或启动副作用非幂等的 stdio 服务器——正是新用户文档中指出需要退出的场景——除非单独配置 versionNegotiation: "legacy",否则每次连接都会失败或产生资源争用。

建议将 stdio 默认改为 legacy(auto 显式启用),或复用会话传输来执行探测而不是另起兄弟进程,使单所有者/非幂等服务器默认安全。

— qwen3.8-max via Qwen Code /review (v0.21.15)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 54c2092. Default stdio and explicit legacy now both take the single-process initialize path. A real StdioClientTransport A/B recorded one unique child PID for default, one for explicit legacy, and two only for explicit auto. Pool identity, CLI/ACP/SDK/Java/Desktop config surfaces, validation, tests, and docs now model auto as the explicit opt-in.

@wenshao

wenshao commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

Local real-environment verification on Linux (head 54c2092)

Since the PR's test matrix lists Linux as ⚠️ unverified, I built a full real environment on Linux and verified the end-to-end path independently. Summary first: every core claim of this PR reproduces on Linux; I found no blocker. Two non-blocking observations at the end.

Environmentnpm ci at the PR head, full monorepo build + bundle; a real qwen serve daemon on loopback serving the built Web Shell; three stdio MCP servers from settings, each wrapped in a stdio wire tap that records every JSON-RPC frame: the PR-body mock server registered twice (once with defaults, once with versionNegotiation: "auto"), plus a hand-rolled modern-only server that answers server/discover and rejects legacy initialize with -32601. A scripted OpenAI-compatible fake model calls each tool exactly once, and real Chromium (Playwright) drives the Web Shell. A second arm was built identically at the merge base (6bbb273a8) for A/B.

1. Protocol negotiation (wire-level ground truth)

Config Observed on the wire Verdict
stdio, defaults single legacy initialize (2025-11-25), no server/discover ✅ legacy by default, as this head intends
stdio, versionNegotiation:"auto", legacy (SDK v1) server server/discover probe → -32601 → transport respawn → legacy initialize (probe answered in ~140 ms; total fallback well under a second); discovery unaffected
stdio, auto, modern-only server server/discover2026-07-28 session; no initialize ever; every later request carries the _meta protocol/clientInfo/clientCapabilities markers ✅ true 2026 session

The client advertises the Apps extension in both handshakes (legacy initialize.capabilities.extensions and modern _meta):

C->S {"method":"initialize","params":{"protocolVersion":"2025-11-25","capabilities":{"extensions":{"io.modelcontextprotocol/ui":{"mimeTypes":["text/html;profile=mcp-app"]}},...}}}
C->S {"id":"server-discover-probe-1","method":"server/discover","params":{"_meta":{"io.modelcontextprotocol/protocolVersion":"2026-07-28",...}}}
S->C {"id":"server-discover-probe-1","result":{"supportedVersions":["2026-07-28"],...}}   ← modern arm; no initialize follows

qwen mcp list (shared factory, R13-1/R15-3 path): on the PR build all three servers report Connected, including the modern-only one; on the base build the modern-only server is Disconnected — a clean discriminator that the negotiation code is what makes it work.

2. WebShell MCP App rendering (real daemon + real Chromium)

After — the model called show_revenue_dashboard once; the completed turn is expanded by default and the App renders inline:

After: MCP App rendered inline

Before (merge-base build, same servers/model/prompt) — plain text only, no Apps path:

Before: text only on the merge-base build

The same rendering works over the pure-2026 session (modern-only server):

Modern-only server App

Isolation assertions, all measured in the live browser:

  • Page origin http://127.0.0.1:4610, outer iframe http://localhost:4610/mcp-app-sandbox?...cross-loopback-origin proxy confirmed.
  • Outer and inner iframe: sandbox="allow-scripts allow-forms" — no allow-same-origin anywhere.
  • Probes executed inside the App frame: self.origin === "null"; parent.document, parent.sessionStorage, top.document all throw SecurityError. The App cannot reach WebShell DOM or the token in storage.
  • App content correct: Revenue $128,420 / Orders 1842 / Conversion 7.8% + Jan–Jun bars via the AppBridge tool-result notification; size-changed honored.
  • Model context unchanged: the persisted transcript and headless -o stream-json both show the tool result as plain text only ("Revenue dashboard ready for APAC.") — no HTML ever reaches the model.

3. Fallback paths (same live setup)

Case Core log line UI result
resource declares text/html (wrong profile) Failed to load MCP App ... resource must return text/html;profile=mcp-app plain text, no App card ✅
resource HTML > 1 MiB ... resource HTML exceeds the 1 MiB host limit plain text, no App card ✅
tool without ui metadata (negative control) ordinary text rendering ✅

Fallback: wrong-MIME tool renders text only

4. Sandbox proxy route (HTTP probes)

  • GET /mcp-app-sandbox pre-auth: 200 on the PR build, 401 on base (route absent).
  • CSP query reflection: valid https:// origins land in the right directives; javascript:alert(1), a U+017F homoglyph host (https://ſ.example.com), and bracketed IPv6 (http://[::1]:9999) are all dropped; a >8192-byte csp query falls back to the fully locked-down CSP (connect-src 'self', frame-src 'none').
  • Shell page CSP pins frame-src to the four loopback origins with the exact host port; Permissions-Policy: camera=(), microphone=(self), geolocation=(), payment=(), clipboard-write=(self) as coded.

5. Build & tests (this head, Linux, Node 22)

  • Full monorepo build ✅, bundle ✅, packages/core tsc --noEmit ✅.
  • Touched suites: core 337/337 (incl. the 10 v2-negotiation tests), cli 1764/1764, web-shell 234/234, sdk-typescript 81/81.

Two non-blocking observations

O1 — the App does not survive a hard reload here. After F5 the replayed turn stays expanded but shows only the fallback text; [data-testid="mcp-app"] count is 0. Mechanism: the transcript persists only the model-visible functionResponse text, so replay has no mcp_app display to render — the App is live-session-only on this build. The PR body claims "the App was visible by default after a hard reload" (verified on macOS); on Linux I could not reproduce that. Worth a quick author confirmation whether replay rendering is expected in this slice or deferred (adjacent to D20-11). Degradation is the designed text fallback, so not a blocker either way.

After hard reload: fallback text, no App

O2 — strict v2 read-result schema silently downgrades Apps. A modern server that omits ttlMs/cacheScope on the resources/read result fails the typed-helper validation and the App silently degrades to text (debug-level warn only: Invalid result for resources/read: ... ttlMs ... cacheScope). That is spec enforcement and fails safe, but server authors will hit it — my own mock did until fixed. Maybe worth one line in docs/users/features/mcp.md.

(Harness note, not a PR issue: for a scripted model the MCP tools only surface with alwaysLoadTools: true or via tool_search, and workspace-scoped mcpServers sit behind the approval gate — user-scoped settings avoid both when reproducing.)

Not covered here

Remote HTTP/SSE/TCP stays legacy by design (the no-probe-on-remotes behavior is covered by the PR's unit tests, which I ran, not by a live remote arm here); App-initiated tool calls / links / fullscreen are out of scope per the PR; Windows still unverified.

Overall: the Linux column of the test matrix can be considered covered by this pass — negotiation, Apps rendering, isolation, fallbacks, and the sandbox route all behave as described.

中文说明

Linux 本地真实环境验证(head 54c2092)

PR 的测试矩阵中 Linux 为 ⚠️ 未验证,因此我在 Linux 上搭建了完整真实环境做独立验证。结论先行:本 PR 的全部核心声明在 Linux 上均可复现,未发现阻塞问题;文末有两条非阻塞观察。

环境——在 PR head 上 npm ci、全仓构建 + bundle;真实 qwen serve daemon 在 loopback 上服务构建出的 Web Shell;settings 注册三个 stdio MCP server,每个都套了记录全部 JSON-RPC 帧的 wire tap:PR 正文的 mock server 注册两次(默认配置、versionNegotiation:"auto"),外加一个手写的纯 2026 server(响应 server/discover、对 legacy initialize 返回 -32601)。脚本化的 OpenAI 兼容假模型对每个工具恰好调用一次,真实 Chromium(Playwright)驱动 Web Shell。另按相同方式在 merge base(6bbb273a8)构建对照臂做 A/B。

1. 协议协商(wire 层实证)

配置 wire 上观察到的行为 结论
stdio 默认 仅一次 legacy initialize(2025-11-25), server/discover ✅ 默认 legacy,与本 head 意图一致
stdio + auto + legacy(SDK v1)server server/discover 探测 → -32601 → 重启 transport 走 legacy initialize(探测约 140ms 得到应答,整体回退远小于 1 秒);发现不受影响
stdio + auto + 纯 modern server server/discover2026-07-28 会话;始终无 initialize;后续请求均携带 _meta 协议标记 ✅ 真正的 2026 会话

client 在两种握手中都声明了 Apps extension(见英文段 wire 摘录)。qwen mcp list:PR 构建三个 server 全部 Connected(含纯 modern);base 构建上纯 modern server Disconnected——干净地证明是协商代码让它工作。

2. WebShell MCP App 渲染(真实 daemon + 真实 Chromium)

截图见英文段:After(App 内联渲染、回合默认展开)、Before(merge-base 构建同配置只有纯文本)、纯 2026 server 的 App 渲染。

隔离断言(均在真实浏览器中测得):页面 origin http://127.0.0.1:4610 vs 外层 iframe http://localhost:4610/mcp-app-sandbox 跨环回源;内外两层 iframe 均为 sandbox="allow-scripts allow-forms"(无 allow-same-origin);在 App frame 内部执行探针:self.origin === "null",访问 parent.documentparent.sessionStoragetop.document 全部抛 SecurityError;App 指标与柱状图渲染正确;持久化 transcript 与 headless -o stream-json 均显示模型只见纯文本工具结果,HTML 从未进入模型上下文。

3. 回退路径

错误 MIME → 核心日志 resource must return text/html;profile=mcp-app,UI 纯文本 ✅;HTML 超 1 MiB → resource HTML exceeds the 1 MiB host limit,UI 纯文本 ✅;无 ui metadata 的工具(阴性对照)→ 普通文本渲染 ✅。

4. sandbox 代理路由(HTTP 探针)

GET /mcp-app-sandbox 预授权:PR 构建 200,base 401(路由不存在)。CSP query:合法 https:// 源进入正确指令;javascript:、U+017F 同形字域名、带括号 IPv6 全部被丢弃;超 8192 字节的 csp query 回退到全锁死 CSP。Shell 页面 frame-src 钉在带宿主端口的四个 loopback 源;Permissions-Policy 与代码一致。

5. 构建与测试(本 head,Linux,Node 22)

全仓构建 ✅、bundle ✅、core tsc --noEmit ✅;触及套件:core 337/337、cli 1764/1764、web-shell 234/234、sdk-typescript 81/81

两条非阻塞观察

O1——硬刷新后 App 不会恢复。 F5 后回放的回合保持展开但只显示回退文本,mcp-app 卡片为 0。机制:transcript 只持久化模型可见的 functionResponse 文本,回放时没有 mcp_app display 可渲染——本构建上 App 仅存在于活跃会话。PR 正文声称"硬刷新后 App 默认可见"(在 macOS 验证);Linux 上我无法复现,请作者确认回放渲染是否属于本切片(与 D20-11 相邻)。降级形态就是设计中的文本回退,无论如何不算阻塞。

O2——严格的 v2 读取结果 schema 会静默降级 App。 modern server 若在 resources/read 结果中省略 ttlMs/cacheScope,类型化校验失败,App 静默降级为文本(仅 debug 级警告)。这是 spec 强制且 fail-safe,但 server 作者会踩到——我自己的 mock 起初就踩了。或可在 docs/users/features/mcp.md 加一句。

(harness 备注,非 PR 问题:脚本化模型要看到 MCP 工具需 alwaysLoadTools: true 或走 tool_search;workspace 级 mcpServers 受审批门限,复现时用 user 级 settings 可同时绕开两者。)

未覆盖

远程 HTTP/SSE/TCP 按设计保持 legacy(远程不探测的行为由 PR 自带单测覆盖 —— 我跑过这些单测,但没有搭真实远程臂);App 主动发起的工具调用/链接/全屏不在本 PR 范围;Windows 仍未验证。

总体:测试矩阵的 Linux 列可视为已被本轮覆盖——协商、App 渲染、隔离、回退与 sandbox 路由行为均与描述一致。

🤖 Generated with Claude Code — Claude Fable 5

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): chunk 8: executing packages/core/src/tools/mcp-client.test.ts under vitest — no node_modules in the review worktree or main checkout, and a full npm ci + build exc….

Deferred under the convergence posture (round 21, not a blocker) — recorded, not requested in this round:

  • docs/developers/daemon/05-mcp-transport-pool.md:318 — [review] Fingerprint enumeration edited by this diff omits three…
  • docs/developers/tools/mcp-server.md:119 — [review] User-facing docs omit the silent auto→legacy downgrade…
  • docs/users/features/mcp.md:266 — [review] PR description still claims stdio clients auto-negotiate;…
  • packages/cli/src/acp-integration/acpAgent.ts:2659 — [review] ACP setMcpServer accepts versionNegotiation for http/sse…
  • packages/cli/src/commands/mcp/list.ts:33 — [review] qwen mcp list 10s cap ignores auto-mode probe consumption…
  • packages/cli/src/nonInteractive/io/BaseJsonOutputAdapter.ts:1423 — [review] Second detached-JSDoc instance: mcpAppFallbackText…
  • packages/cli/src/serve/web-shell-static.ts:161 — [review] Deferred-runtime gate exemption for /mcp-app-sandbox has…
  • packages/cli/src/serve/web-shell-static.ts:161 — [review] isPreAuthWebShellRequest misses trailing-slash…
  • packages/core/src/tools/mcp-client-v2.test.ts:80 — [review] Negotiation-policy test pins no transport-exclusion branch…
  • packages/core/src/tools/mcp-client-v2.test.ts:459 — [review] Auto-mode probe-timeout fallback (silent stdio server) has…
  • packages/core/src/tools/mcp-client.ts:410 — [review] listMaxPages: 0 removes the pagination bound for…
  • packages/core/src/tools/mcp-client.ts:1445 — [review] JSDoc detached from discoverTools by inserted helper
  • packages/core/src/tools/mcp-pool-key.ts:141 — [review] Pool fingerprint hashes raw auto flag, not effective…
  • packages/core/src/tools/mcp-tool.ts:420 — [review] Reconnect-retry propagation of appResourceUri/appResourceUi…
  • packages/core/src/tools/mcp-tool.ts:692 — [review] loadMcpAppDisplay rejection branches (1 MiB cap, blob…
  • packages/core/src/utils/toolResultDisplayCompaction.ts:489 — [review] compactMcpAppResultDisplay passes toolArguments through…
  • packages/web-shell/client/components/WebShellTranscript.tsx:71 — [review] Public mcpAppBaseUrl prop → provider wiring has zero test…
  • packages/web-shell/client/components/messages/McpApp.dom.test.tsx:11 — [review] McpApp onsizechange handler (height clamp) has zero test…
  • packages/web-shell/client/components/messages/McpApp.dom.test.tsx:17 — [review] McpApp's four setError error paths are never exercised by…
  • packages/web-shell/client/components/messages/McpApp.dom.test.tsx:26 — [review] PostMessageTransport constructor arguments (send target +…
  • …and 5 more (see the run report)
中文说明

仅完成部分审查,审查缺口已披露。

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。

未探索到全部深度(达到工具调用预算):chunk 8:executing packages/core/src/tools/mcp-client.test.ts under vitest — no node_modules in the review worktree or main checkout, and a full npm ci + build exc…

收敛姿态下延后(第 21 轮,非阻断)——已记录,本轮不要求修改:共 25 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.22.0)

@wenshao

wenshao commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

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: 87 passed · 0 failed · 87 total

Flakiness gate: ⚠️ timeout — only 3 of 5 rounds fit the 15-minute budget; the completed rounds agreed

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:87 通过 · 0 失败 · 87 总计

抖动门:⚠️ timeout — only 3 of 5 rounds fit the 15-minute budget; the completed rounds agreed

Verification report

PR 8992 deep verification — feat(mcp): add MCP 2026 core and WebShell Apps host

Verdict: merge-ready — 87/87 scripted assertions passed (A/B wire harness 59, sandbox-route probe 28); targeted gates all green (core 337, CLI 1293+455+74, web-shell 234); 5/6 mutants killed, the 6th adjudicated as a layered-guard coverage gap, not a defect. No behavioral blocker found.
Verified head: 54c20920d8caa7a9a5403e188bef08b0ff85bc59 (= HEAD^2; the metadata snapshot's headRefOid matches). Base control: HEAD^1 = b6901ee0fd. Note: the snapshot's baseRefOid (6bbb273a…) is older than the merge-ref base tip; this round verified HEAD^1..HEAD per the CI contract.

中文摘要
  • 结论: merge-ready。87/87 脚本化断言通过,各受影响工作区测试全绿,未发现行为阻断问题。
  • A/B 结论(见 §3 表格,证据图 01-ab-cells-head-vs-base.png):
    • versionNegotiation: 'auto' 的 stdio 客户端对 2026 服务器协商成功(2026-07-28,会话进程无 initialize,探针走一次性兄弟进程);对沉默的 legacy 服务器在预算内回退 legacy handshake。
    • 默认/远程配置探针、保持 legacy —— 协商是显式 opt-in(与 PR 描述措辞不符,见 §4 更正)。
    • ui:// App metadata 保留、工具成功后 resources/read 抓取 HTML(≤1 MiB、mime 校验),模型可见结果(llmContent/functionResponse)head 与 base 完全一致、不含 HTML。
    • base 侧对照:无探针、无扩展声明、无 visibility 过滤、无资源抓取、展示为纯文本 —— 全部差异归因于本 PR。
  • 真实 daemon E2E:真实 qwen serve + 真实模型回合,wire 上完整出现 server/discover → modern 会话 → tools/callresources/read,SSE 事件携带 mcp_app display(html + fallbackText)(04-daemon-e2e-wire.png)。
  • Findings:1 个 Suggestion —— resolveMcpAppSandboxUrl 的双 loopback 守卫仅被组合用例钉住,缺少混合 origin 单一守卫用例(§5-1);其余为描述更正与注释小疵。
  • 未覆盖:浏览器内真实渲染、远程 modern 协商(PR 自述范围外)、86 个 commit 的逐 commit 归因(浅克隆仅可达 1 个)、NOTICES.txt 重新生成、仓库级全量测试。

1. Scope

Central claim: an opt-in (versionNegotiation: 'auto') stdio MCP client negotiates the modern 2026 protocol via a bounded server/discover probe with legacy fallback, advertises the Apps extension, preserves ui:// tool metadata, fetches and validates the declared HTML resource after a successful tool call — while the model-visible tool result stays byte-identical and everything not opted in (default stdio, remote HTTP/SSE/TCP, SDK-control) stays on the legacy handshake.

Secondary claims: (a) the daemon serves an MCP App sandbox proxy with sanitized server-declared CSP, loopback-port-pinned framing, and opaque-origin double iframes; (b) non-app MCP behavior is unchanged (lenient discovery, plain displays).

2. PR's own Reviewer Test Plan — walked per step

Step Result
1. Configure the mock stdio server in a daemon workspace, start WebShell on loopback Done live. Real qwen serve booted twice (ports 4191/4192); WebShell served from packages/web-shell/dist; /mcp-app-sandbox mounted pre-auth. Note: the workspace also needed the pre-existing trust store (trustedFolders.json) and the hash-bound mcpApprovals.json entry — both existing gates (#4615), not new in this PR. Without them the server is silently skipped (observed, then fixed, then re-run).
2. Model calls show_revenue_dashboard with region: "APAC" exactly once Done live. A real model turn called the canonical tool once (transcript CALL mcp__mcp-app-demo__show_revenue_dashboard {"region":"APAC"}; wire shows exactly one tools/call).
3. Completed turn expanded by default, dashboard values rendered inline Partial. The SSE wire to WebShell carries the full mcp_app display (html, fallbackText, resourceUri) that getMcpAppDisplay consumes — but no browser exists in this container, so actual inline rendering/expansion was not visually confirmed (DOM-level tests pin the iframe attributes instead).
4. Inner iframe opaque origin; invalid resource falls back to text Pinned by tests, not live browser. McpApp.dom.test.tsx asserts sandbox="allow-scripts allow-forms" and absence of allow-same-origin; fallback paths proven by A/B cell H5 and mutation M2.

3. Central claim — A/B load-bearing proof

Harnesses: ab-harness.mjs drives the compiled dist (packages/core/dist at head, tmp/base-tree/packages/core/dist for base) through a real stdio child process (mock-mcp-server.mjs) that logs every inbound JSON-RPC message per pid; run-ab.mjs asserts on observations + raw wire. Witness: 01-ab-cells-head-vs-base.png, raw logs in logs/.

Cell Build Server Config Key wire observations Result
H1 head legacy app server default initialize 2025-11-25 advertising io.modelcontextprotocol/ui; no server/discover; tools/callresources/read ui://… after; display mcp_app (246 B html, csp propagated); llmContent = plain text part only 19/19
H2 head legacy app server auto exactly one server/discover on a disposable sibling pid, -32601 → fallback initialize on the session pid; discovery lenient; app display still produced; total < 15 s 9/9
H3 head modern server auto one probe → modern session, zero initialize; negotiated 2026-07-28; session tools/list carries the modern _meta envelope; probe carries Apps extension capabilities; call → mcp_app display; llmContent unchanged 10/10
H4 head modern server default legacy initialize only, no probe — opt-in proven against a modern-capable server 4/4
H5 head plain legacy server default no appResourceUri, display stays a string, no resources/read — app path doesn't over-fire 4/4
B1 base legacy app server default SDK v1: no probe, no Apps extension in initialize, no visibility filtering (2 tools), no resources/read after the call, display plain string; llmContent byte-identical to H1 10/10
B2 base modern server default legacy initialize succeeds; no probe 3/3

Total: 59/59 (ASSERTIONS_JSON {"pass":59,"fail":0,"total":59}). The pair-of-counts shape: every behavioral delta (probe, modern era, extension advertisement, visibility filtering, post-call resource fetch, mcp_app display) is present at head and absent at base under identical wire conditions, while the model-visible payload is identical across both arms.

Daemon-level E2E (same claim, production path): on the trusted + approved workspace the real daemon spawned the probe sibling (server/discover on pid 136678) and a modern session (pid 136691: prompts/list, resources/list, tools/list, then tools/call + resources/read ui://… per turn, both turns), the model-visible functionResponse was {"output": "Revenue dashboard ready for APAC."} (text only), and the SSE stream carried "type":"mcp_app","serverName":"mcp-app-demo","resourceUri":"ui://mock/revenue-dashboard" with fallbackText. Witness: 04-daemon-e2e-wire.png, logs/wire-daemon-modern.jsonl, logs/sse-events-live.txt.

4. Corrections (to the PR description, not code-change requests)

  1. "Configured stdio MCP clients negotiate the modern protocol automatically" — as shipped, negotiation is opt-in: mcpVersionNegotiationFor returns {mode:'legacy'} unless versionNegotiation: 'auto' is set, and the final commit ("default stdio negotiation to legacy") deliberately made that the default. Measured: H4 (modern-capable server + default config → legacy initialize, zero probes). The body reads as if written before that final commit; a future reader would misread the default.
  2. Related: the mcp list timeout comment ("stdio createMcpClient spends up to 5s on server/discover…") describes only the auto case; default-config clients never probe, so the old 5 s budget was unaffected for them. The 10 s bump is harmless and unit-tested, but the comment overgeneralizes. Behavior is correct.

5. Findings

5-1. Suggestion — resolveMcpAppSandboxUrl loopback guards are pinned only as a set, not individually

The guard is !isLoopbackHostname(host.hostname) || !isLoopbackHostname(sandbox.hostname). Mutation matrix rows:

Mutant Suite Result
remove host-side check only McpApp.test.ts rejects non-loopback hosts survived
remove sandbox-side check only same survived
remove both (combination row) same killedexpected 'https://daemon.example.com/mcp-app-sa…' to be undefined

Both single rows survive because the only rejection fixture uses two non-loopback hosts, so either remaining guard rejects. The behavior in the shipped code is correct (both guards are present and each is independently sound); what is missing is a mixed-origin fixture (host loopback × daemon non-loopback, and vice versa) so a future edit breaking one guard cannot hide behind the other. Positive controls for the round: mutants M1–M5 below all turned their suites red, so the harness is live.

Reproduce (from the repo root):

# single-guard row (survives): disable only the host-side check in
# packages/web-shell/client/components/messages/McpApp.tsx — change
#   !isLoopbackHostname(host.hostname) ||
#   !isLoopbackHostname(sandbox.hostname)
# to
#   false ||
#   !isLoopbackHostname(sandbox.hostname)
cd packages/web-shell && npx vitest run client/components/messages/McpApp.test.ts   # 8/8 still pass
# combination row (killed): disable BOTH clauses (false || false) and rerun —
# 'rejects non-loopback hosts' fails: expected 'https://daemon.example.com/mcp-app-sa…' to be undefined

5-2. Nit — description/comment wording (see §4). No action on code.

6. Mutation matrix (vacuity of the new tests)

One row per central guard the PR introduces; unmutated control green for every suite before mutating (gate runs, §7).

# Guard (hunk) Mutation Suite that pins it Verdict
M1 negotiation opt-in (mcpVersionNegotiationFor) condition → always-auto for stdio mcp-client-v2.test.ts killed (2 red: expected {mode:'auto'} to deeply equal {mode:'legacy'} + discovery-budget timeout against a silent legacy server)
M2 app display wiring (returnDisplay: appDisplay ?? fallbackText) drop appDisplay mcp-tool.test.ts "MCP Apps display" killed (3 red: expected 'Dashboard ready' to match object {type:'mcp_app'…}); witness 02-mutation-m2-app-display-killed.png. The 2 fallback-path tests correctly stay green under this mutant — they assert the fallback, which is exactly what the mutant produces
M3 visibility filter in discovery loop filter disabled mcp-client.test.ts skips MCP App tools whose visibility does not include model killed (expected [Array(2)] to deeply equal ['show_dashboard']). Note: the same-named test in mcp-client-v2.test.ts pins only the pure predicate isMcpToolVisibleToModel, not the discovery integration — the integration pin lives in the other file
M4 CSP source sanitization regex loosen to ^https?:\/\/.+$ mcp-app-sandbox.test.ts killed (3 red: injection string reached the header not to contain 'bad.test'; two Unicode case-folding cases flipped 200→500)
M5 loopbackSandboxOrigins Host-port pinning port → wildcard :* web-shell-static.test.ts killed (2 red: expected ['http://localhost:*',…] to deeply equal ['http://localhost:4170',…])
M6 loopback pair in resolveMcpAppSandboxUrl see §5-1 McpApp.test.ts single rows survive, combination killed → coverage gap, not dead code, not a defect

No mutant regressed a previously-killed behavior; every revert was restored (git status clean after each).

7. Targeted gates (all at verified head)

Workspace Command scope Result
core mcp-client.test.ts, mcp-client-v2.test.ts, mcp-tool.test.ts, mcp-pool-key.test.ts, mcp-transport-pool.test.ts, configHash.test.ts, toolResultDisplayCompaction.test.ts 337 passed / 0 failed (7 files)
cli mcp-app-sandbox, web-shell-static, server, commands/mcp/list, BaseJsonOutputAdapter, ToolMessage, daemon-tui-adapter, systemController suites 1293 passed, 1 skipped / 0 failed (7 files)
cli acpAgent.test.ts 455 passed / 0 failed
cli ToolMessage.test.tsx (isolated rerun) 74 passed / 0 failed
web-shell McpApp.test.ts, McpApp.dom.test.tsx, ToolGroup.test.tsx, MessageList.test.ts, vite-config.test.ts 234 passed / 0 failed (5 files)

Sandbox-route wire probe (sandbox-probe.mjs, real HTTP against the compiled route): 28/28 — default CSP (frame-src 'none', form-action 'none', base-uri 'none'), no-store/nosniff/referrer headers, declared domains applied per directive, six injection shapes dropped (;-injection, percent-encoded ;, javascript:, data:, quote, space-traversal), wss: + wildcard subdomains allowed by pattern, oversized (>8 KiB) and malformed csp queries fall back to default, POST rejected. Witness 03-sandbox-route-probe.png. Gate liveness: proven by the mutation runs above (each red suite is the same command that went green unmutated).

8. Not covered

  • Browser rendering of the App (step 3 visual: expansion, bars/metrics) — no browser in-container; covered only by DOM-level attribute tests and the live SSE payload shape.
  • Modern-only remote (HTTP/SSE/TCP) negotiation — explicitly out of PR scope (SDK v2 probe gap); remote configs verified to stay legacy only at the unit level (PR's own tests), not over a real remote wire.
  • Per-commit attribution — checkout is depth 2; git rev-list HEAD^1..HEAD^2 reaches 1 commit vs 86 in the metadata commits array. Verified the aggregate HEAD^1..HEAD diff only.
  • NOTICES.txt regeneration — the 917-line vscode-ide-companion/NOTICES.txt delta was sanity-checked (new entries are exactly @modelcontextprotocol/client@2.0.0 / core@2.0.0), but the generator was not re-run (would require a full install of that companion package).
  • Repo-wide test suite and repo-wide lint/typecheck (targeted gates only, per budget); the base-side tsc build emitted JS despite pre-existing type noise in unrelated modules (ignore, ajv) — runtime dist was used, noted in methodology.
  • App-initiated tool calls, links, downloads, messages, model-context updates, fullscreen, MRTR flows (declared out of scope by the PR).
  • Windows/Linux desktop UI runs (PR self-declares unverified).

9. Methodology

Environment: CI verify container at merge commit 68556e5063 (npm ci + npm run build pre-done). Base control: git worktree add tmp/base-tree HEAD^1, rebuilt with the repo's own tsc against the shared root node_modules. Control purity checks, all quoted as executed: the lockfile delta is additive-only (root adds @modelcontextprotocol/{client,core}@2.0.0, ext-apps, @standard-schema/spec + nested zods; zero changed/removed entries), and the 94 nested packages/{core,cli,web-shell}/node_modules entries are byte-identical between the two lockfiles, so the base tree reuses them via a directory symlink; realpath of @modelcontextprotocol/sdk resolves to the same file from both arms' dist, base dist contains zero @qwen-code/* imports (grep over dist/src), so no workspace symlink can leak head code into the base cell. Wire oracles: the mock server (mock-mcp-server.mjs) is a raw JSON-RPC stdio process logging every inbound message per pid — assertions read the wire, not the client's self-report (client-side getProtocolEra() used only as a secondary signal). Daemon E2E drove the real compiled CLI (packages/cli/dist/index.js serve) with a trusted + hash-approved workspace and the lane's real model endpoint; MCP gating required seeding trustedFolders.json and mcpApprovals.json (hash computed with the head build's hashMcpServerConfig). Mutations were applied in-place, run, and restored (git status clean at exit); the base worktree was removed after the A/B. Raw per-cell wire logs: logs/wire-*.jsonl; harness scripts at the artifact root so every number above is rerunnable.

Flakiness gate log

rounds=5 files=22 skipped=0
file packages/cli/src/acp-integration/acpAgent.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/acpAgent.test.ts
file packages/cli/src/commands/mcp/list.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/mcp/list.test.ts
file packages/cli/src/nonInteractive/control/controllers/systemController.test.ts: (cd packages/cli) npx --no-install vitest run ./src/nonInteractive/control/controllers/systemController.test.ts
file packages/cli/src/nonInteractive/io/BaseJsonOutputAdapter.test.ts: (cd packages/cli) npx --no-install vitest run ./src/nonInteractive/io/BaseJsonOutputAdapter.test.ts
file packages/cli/src/serve/mcp-app-sandbox.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/mcp-app-sandbox.test.ts
file packages/cli/src/serve/server.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/server.test.ts
file packages/cli/src/serve/web-shell-static.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/web-shell-static.test.ts
file packages/cli/src/ui/components/messages/ToolMessage.test.tsx: (cd packages/cli) npx --no-install vitest run ./src/ui/components/messages/ToolMessage.test.tsx
file packages/cli/src/ui/daemon/daemon-tui-adapter.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/daemon/daemon-tui-adapter.test.ts
file packages/core/src/mcp/configHash.test.ts: (cd packages/core) npx --no-install vitest run ./src/mcp/configHash.test.ts
file packages/core/src/tools/mcp-client-v2.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/mcp-client-v2.test.ts
file packages/core/src/tools/mcp-client.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/mcp-client.test.ts
file packages/core/src/tools/mcp-pool-key.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/mcp-pool-key.test.ts
file packages/core/src/tools/mcp-tool.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/mcp-tool.test.ts
file packages/core/src/tools/mcp-transport-pool.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/mcp-transport-pool.test.ts
file packages/core/src/utils/toolResultDisplayCompaction.test.ts: (cd packages/core) npx --no-install vitest run ./src/utils/toolResultDisplayCompaction.test.ts
file packages/sdk-typescript/test/unit/queryOptionsSchema.test.ts: (cd packages/sdk-typescript) npx --no-install vitest run ./test/unit/queryOptionsSchema.test.ts
file packages/web-shell/client/components/MessageList.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/components/MessageList.test.ts
file packages/web-shell/client/components/messages/McpApp.dom.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/components/messages/McpApp.dom.test.tsx
file packages/web-shell/client/components/messages/McpApp.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/components/messages/McpApp.test.ts
file packages/web-shell/client/components/messages/ToolGroup.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/components/messages/ToolGroup.test.tsx
file packages/web-shell/client/vite-config.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/vite-config.test.ts


per-file results (P=pass F=fail I=infra-exit, one letter per run):
  packages/cli/src/acp-integration/acpAgent.test.ts: PPPP
  packages/cli/src/commands/mcp/list.test.ts: PPPP
  packages/cli/src/nonInteractive/control/controllers/systemController.test.ts: PPPP
  packages/cli/src/nonInteractive/io/BaseJsonOutputAdapter.test.ts: PPPP
  packages/cli/src/serve/mcp-app-sandbox.test.ts: PPPP
  packages/cli/src/serve/server.test.ts: PPPP
  packages/cli/src/serve/web-shell-static.test.ts: PPPP
  packages/cli/src/ui/components/messages/ToolMessage.test.tsx: PPPP
  packages/cli/src/ui/daemon/daemon-tui-adapter.test.ts: PPPP
  packages/core/src/mcp/configHash.test.ts: PPP
  packages/core/src/tools/mcp-client-v2.test.ts: PPP
  packages/core/src/tools/mcp-client.test.ts: PPP
  packages/core/src/tools/mcp-pool-key.test.ts: PPP
  packages/core/src/tools/mcp-tool.test.ts: PPP
  packages/core/src/tools/mcp-transport-pool.test.ts: PPP
  packages/core/src/utils/toolResultDisplayCompaction.test.ts: PPP
  packages/sdk-typescript/test/unit/queryOptionsSchema.test.ts: PPP
  packages/web-shell/client/components/MessageList.test.ts: PPP
  packages/web-shell/client/components/messages/McpApp.dom.test.tsx: PPP
  packages/web-shell/client/components/messages/McpApp.test.ts: PPP
  packages/web-shell/client/components/messages/ToolGroup.test.tsx: PPP
  packages/web-shell/client/vite-config.test.ts: PPP

verdict: timeout
summary: only 3 of 5 rounds fit the 15-minute budget; the completed rounds agreed

--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/mcp/list.test.ts: P (exit 0)
round 1 · packages/cli/src/nonInteractive/control/controllers/systemController.test.ts: P (exit 0)
round 1 · packages/cli/src/nonInteractive/io/BaseJsonOutputAdapter.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/mcp-app-sandbox.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/server.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/web-shell-static.test.ts: P (exit 0)
round 1 · packages/cli/src/ui/components/messages/ToolMessage.test.tsx: P (exit 0)
round 1 · packages/cli/src/ui/daemon/daemon-tui-adapter.test.ts: P (exit 0)
round 1 · packages/core/src/mcp/configHash.test.ts: P (exit 0)
round 1 · packages/core/src/tools/mcp-client-v2.test.ts: P (exit 0)
round 1 · packages/core/src/tools/mcp-client.test.ts: P (exit 0)
round 1 · packages/core/src/tools/mcp-pool-key.test.ts: P (exit 0)
round 1 · packages/core/src/tools/mcp-tool.test.ts: P (exit 0)
round 1 · packages/core/src/tools/mcp-transport-pool.test.ts: P (exit 0)
round 1 · packages/core/src/utils/toolResultDisplayCompaction.test.ts: P (exit 0)
round 1 · packages/sdk-typescript/test/unit/queryOptionsSchema.test.ts: P (exit 0)
round 1 · packages/web-shell/client/components/MessageList.test.ts: P (exit 0)
round 1 · packages/web-shell/client/components/messages/McpApp.dom.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/components/messages/McpApp.test.ts: P (exit 0)
round 1 · packages/web-shell/client/components/messages/ToolGroup.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/vite-config.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/mcp/list.test.ts: P (exit 0)
round 2 · packages/cli/src/nonInteractive/control/controllers/systemController.test.ts: P (exit 0)
round 2 · packages/cli/src/nonInteractive/io/BaseJsonOutputAdapter.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/mcp-app-sandbox.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/server.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/web-shell-static.test.ts: P (exit 0)
round 2 · packages/cli/src/ui/components/messages/ToolMessage.test.tsx: P (exit 0)
round 2 · packages/cli/src/ui/daemon/daemon-tui-adapter.test.ts: P (exit 0)
round 2 · packages/core/src/mcp/configHash.test.ts: P (exit 0)
round 2 · packages/core/src/tools/mcp-client-v2.test.ts: P (exit 0)
round 2 · packages/core/src/tools/mcp-client.test.ts: P (exit 0)
round 2 · packages/core/src/tools/mcp-pool-key.test.ts: P (exit 0)
round 2 · packages/core/src/tools/mcp-tool.test.ts: P (exit 0)
round 2 · packages/core/src/tools/mcp-transport-pool.test.ts: P (exit 0)
round 2 · packages/core/src/utils/toolResultDisplayCompaction.test.ts: P (exit 0)
round 2 · packages/sdk-typescript/test/unit/queryOptionsSchema.test.ts: P (exit 0)
round 2 · packages/web-shell/client/components/MessageList.test.ts: P (exit 0)
round 2 · packages/web-shell/client/components/messages/McpApp.dom.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/components/messages/McpApp.test.ts: P (exit 0)
round 2 · packages/web-shell/client/components/messages/ToolGroup.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/vite-config.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/mcp/list.test.ts: P (exit 0)
round 3 · packages/cli/src/nonInteractive/control/controllers/systemController.test.ts: P (exit 0)
round 3 · packages/cli/src/nonInteractive/io/BaseJsonOutputAdapter.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/mcp-app-sandbox.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/server.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/web-shell-static.test.ts: P (exit 0)
round 3 · packages/cli/src/ui/components/messages/ToolMessage.test.tsx: P (exit 0)
round 3 · packages/cli/src/ui/daemon/daemon-tui-adapter.test.ts: P (exit 0)
round 3 · packages/core/src/mcp/configHash.test.ts: P (exit 0)
round 3 · packages/core/src/tools/mcp-client-v2.test.ts: P (exit 0)
round 3 · packages/core/src/tools/mcp-client.test.ts: P (exit 0)
round 3 · packages/core/src/tools/mcp-pool-key.test.ts: P (exit 0)
round 3 · packages/core/src/tools/mcp-tool.test.ts: P (exit 0)
round 3 · packages/core/src/tools/mcp-transport-pool.test.ts: P (exit 0)
round 3 · packages/core/src/utils/toolResultDisplayCompaction.test.ts: P (exit 0)
round 3 · packages/sdk-typescript/test/unit/queryOptionsSchema.test.ts: P (exit 0)
round 3 · packages/web-shell/client/components/MessageList.test.ts: P (exit 0)
round 3 · packages/web-shell/client/components/messages/McpApp.dom.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/components/messages/McpApp.test.ts: P (exit 0)
round 3 · packages/web-shell/client/components/messages/ToolGroup.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/vite-config.test.ts: P (exit 0)
round 4 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/mcp/list.test.ts: P (exit 0)
round 4 · packages/cli/sr

...truncated -- full content in the run artifacts.

Evidence images

01-ab-cells-head-vs-base

02-mutation-m2-app-display-killed

03-sandbox-route-probe

04-daemon-e2e-wire

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@samuelhsin
samuelhsin added this pull request to the merge queue Aug 23, 2026
Merged via the queue into QwenLM:main with commit 4ddbf22 Aug 23, 2026
139 of 140 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.22.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants