feat(web-shell): browse MCP server resources in the /mcp dialog - #5879
Conversation
Port the TUI's MCP resource browser (QwenLM#5544/QwenLM#5635) to the Web Shell. The /mcp dialog now shows per-server resource and prompt counts plus an expandable resource browser (URI, MIME type, size, description, and the @server:uri chat reference), reaching parity with the terminal UI. Resources and prompts were never serialized past the in-process core registries, so this wires the data end to end: per-server resourceCount and promptCount ride the existing /workspace/mcp status, and a new qwen/status/workspace/mcp/resources ext-method (mirroring the tools drill-down) carries the resource list through the daemon, SDK, and webui hook into the Web Shell McpDialog. All additions are protocol-additive; older daemons 404 the new route and the client degrades gracefully.
|
Thanks for the PR! Template looks good ✓ On direction: this is a straightforward parity port — the TUI On approach: the scope is tight for what it does. The plumbing mirrors the existing Re-run note: merge conflict with #5809 (serve server route split) has been resolved — the new resources route was correctly moved into the extracted Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：这是直接的对齐移植——TUI 的 方案：对于所做的事情来说范围紧凑。每一层的管道都镜像了既有的 重跑说明： 与 #5809（serve server 路由拆分）的合并冲突已解决——新的 resources 路由被正确移入抽取的 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewClean implementation — mirrors the existing A few observations (non-blocking):
Test ResultsAll unit/integration tests pass across every layer (re-run after merge conflict resolution): Typecheck clean across all packages. Real-Scenario Test (tmux)Started the dev daemon from PR code on port 19877 and hit all endpoints: Daemon logs confirm all five requests handled correctly: The new 中文说明代码审查实现干净——每一层都镜像既有的 测试结果所有单元/集成测试在合并冲突解决后重新运行均通过。全包 typecheck 通过。 真实场景测试（tmux）从 PR 代码启动 dev daemon，测试了 5 个端点场景：基础 MCP 状态、未配置 server 的 resources（返回错误 cell）、名称长度限制（300 字符返回 400）、URL 编码的 server 名称（正确解码）、空 server 名称（404）。所有端点行为正确。CI 环境未配置 MCP server，无法端到端测试资源浏览器 UI，但 React 组件以下的每一层均已验证。 — Qwen Code · qwen3.7-max |
|
This PR is a textbook parity port. The TUI's MCP resource browser has been shipping since #5544/#5635; the Web Shell was the obvious next surface, and this PR delivers exactly that — no more, no less. The implementation mirrors the established My independent proposal for this problem would have been: "add a Tests pass at every layer (re-verified after the merge conflict resolution with #5809), typecheck is clean, and the daemon endpoint responds correctly with proper error handling and input validation across five scenarios (base status, unconfigured server error, name-length limit, URL decoding, empty name). The only mild concern is the DRY repetition across the three The merge conflict resolution is clean — the resources route was correctly relocated into the extracted Ships the feature cleanly. ✅ 中文说明这个 PR 是教科书式的对齐移植。TUI 的 MCP 资源浏览器自 #5544/#5635 起已发布；Web Shell 是显而易见的下一个界面，本 PR 恰好交付了这一点——不多不少。 实现在每一层都镜像了已建立的 我对这个问题的独立方案是："添加一个镜像 tools 端点的 各层测试均在合并冲突解决后重新验证通过，typecheck 干净，daemon 端点在五个场景（基础状态、未配置 server 错误、名称长度限制、URL 解码、空名称）下均响应正确。 唯一的轻微顾虑是三个 合并冲突解决干净——resources 路由被正确迁入抽取的 功能干净交付。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
…size Address review on QwenLM#5879: - SerializedMcpStatusMessage.resourcesByServer is now optional, matching its JSDoc ("older clients omit it") so TypeScript enforces the `?? {}` defensive read at every consumer. - mcp.resource.bytes now pluralizes ("1 byte" vs "2 bytes"), consistent with the mcp.resourceCount / mcp.promptCount strings in this PR.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
…ref, harden fallbacks, add tests Address review on QwenLM#5879: - [Critical] Remove the "@server:uri" chat-reference UI from the resource browser: the Web Shell submits prompts as a plain text block and the daemon forwards it verbatim (the TUI's atCommandProcessor resolution is TUI-client-side only), so the reference never injected resource content. The browser stays as read-only metadata; @-reference injection is a follow-up that needs the resolver wired into the Web Shell prompt path. - [Critical] reloadServer now isolates the resource refetch in its own try/catch so a failed resource load can't report a successful reconnect/enable as failed. - [Critical] resolveServerMcpResources/Prompts skip a throwing session in the pool-mode fallback so one degraded session can't blank the base /workspace/mcp status (which now carries the counts). - [Suggestion] Resource section renders an "unavailable" state when a server advertises a count but the drill-down list is empty. - [Suggestion] Add SDK client URL-encoding test (workspaceMcpResources), route-table matching test, and an unconfigured-server error-branch test.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review: APPROVE (downgraded to comment — CI still running)
LGTM! ✅ Clean parity port of the TUI MCP resource browser to the Web Shell. The plumbing is consistent at every layer (daemon handler → ACP route → SDK client → webui action → React component), tests cover the new paths including graceful fallback for older daemons, and the follow-up commit (2abb167) addresses all previously raised inline comments.
Downgrade reason: 30 CI checks are still pending. Will re-approve once CI lands green.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed current head 2abb167. The earlier critical points are resolved, the resources path now stays metadata-only in Web Shell, the fallback/route/client tests are in place, and current checks show no failing jobs. LGTM.
|
@qwen-code /resolve |
Main refactored workspace status routes out of server.ts into registerWorkspaceStatusRoutes(). PR had added a new GET /workspace/mcp/:server/resources route inline. Resolution: take main's refactoring and port the resources route into the extracted workspace-status.ts module. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Qwen Code resolved the merge conflicts and pushed the branch update. Conflict Resolution Summary — PR #5879ConflictFile: What happened
Resolution
Files modified
Files NOT modifiedNo other files were touched. The merge brought in many new files from main |
ff62e0c
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed current head ff62e0c after the conflict-resolution update. The new MCP resources route is correctly ported into registerWorkspaceStatusRoutes after the server route split, server.ts keeps the route registration clean, and there are no unresolved review threads. Current checks show no failures at review time; only the review automation is still pending. LGTM.
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review: COMMENT — 4 suggestions, no blockers.
The plumbing is consistent at every layer (daemon → ACP route → SDK → webui → McpDialog) and the test suite covers the happy paths well. A few targeted suggestions: two untested code paths (session fallback loop and catch-all error handler), one silent catch that should log, and a negative invariant in the disabled-server test that should be asserted explicitly.
| if (sessionResources.length > 0) { | ||
| return sessionResources; | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
[Suggestion] The session fallback in resolveServerMcpResources swallows errors with a bare catch {} — no logging. The file uses debugLogger 58 times elsewhere for exactly this kind of silent-degradation scenario. The adjacent resolveServerMcpPrompts has the same pattern (line 3616).
Consider:
} catch {
debugLogger.debug('[acpAgent] session fallback: no resources from any session for %s', serverName);
}Without a log, a degraded session that can't return its config is invisible to operators debugging why a server shows resourceCount: 0 in the status but has resources in the registry.
| if (resources.length > 0) { | ||
| return resources; | ||
| } | ||
| for (const session of this.getActiveSessions()) { |
There was a problem hiding this comment.
[Suggestion] The pool-mode session fallback loop (~30 lines) is untested. The new test mocks the workspace registry to return resources directly for docs, so this branch (iterating over active sessions when the registry is empty) is never entered.
A test like: mock pool.activeSessions() to return one session whose getConfig() has the matching server, assert resources are returned from the session path — would cover the fallback. The analogous tools builder (resolveServerMcpTools) presumably has the same gap, so both could be addressed together.
| @@ -1611,6 +1642,8 @@ describe('QwenAgent MCP SSE/HTTP support', () => { | |||
| mcpStatus: 'connected', | |||
| transport: 'http', | |||
| disabled: false, | |||
There was a problem hiding this comment.
[Suggestion] resourceCount: 0 and promptCount: 0 are correctly added to the remote (non-disabled) server entry. However, the disabled server entry below intentionally omits these fields — the implementation only adds counts when !disabled.
The test currently only asserts presence via toMatchObject but doesn't verify the negative invariant. Consider an explicit assertion:
const disabledEntry = mcp!.servers.find(s => s.name === 'disabled-server');
expect(disabledEntry).toBeDefined();
expect(disabledEntry!.resourceCount).toBeUndefined();
expect(disabledEntry!.promptCount).toBeUndefined();This ensures a future refactor that accidentally adds counts to disabled servers gets caught.
| acpChannelLive: true, | ||
| resources, | ||
| }; | ||
| } catch (error) { |
There was a problem hiding this comment.
[Suggestion] The outer try { ... } catch at the bottom of buildWorkspaceMcpResourcesStatus handles unexpected errors (returning errors: [{ kind: 'mcp_resources', status: 'error' }]), but the test only exercises the happy path and the "not configured" early return.
A test that forces an error — e.g., stubbing resolveServerMcpResources to throw — and asserts the error envelope would cover this path. The earlier suggestion about the unconfigured-server error branch (line 5121) was already addressed in 2abb167; this is the remaining untested branch.
| // isolated in its own try/catch so a failed resource refresh never | ||
| // turns a successful reconnect/enable into a reported failure. | ||
| if (nextServer.resourceCount) { | ||
| try { |
There was a problem hiding this comment.
[Suggestion] The try/catch around mcp.loadResources(nextServer.name) is dead code. loadMcpResources in actions.ts catches all errors internally and returns a fallback ServeWorkspaceMcpResourcesStatus — it never rejects. The catch block ("Leave the prior resource list in place") can never execute; a failed fetch stores the fallback object (empty resources + error cell) into state instead, contrary to what the comment promises.
Either remove the try/catch since it's dead code, or change loadMcpResources in actions.ts to re-throw so the caller's catch can actually retain the prior list as the comment describes.
| return; | ||
| } | ||
|
|
||
| case `${QWEN_METHOD_NS}workspace/mcp/resources`: { |
There was a problem hiding this comment.
[Suggestion] The ACP dispatch for workspace/mcp/resources validates that serverName is non-empty but does not enforce MAX_NAME_LENGTH. The HTTP route in workspace-status.ts correctly checks serverName.length > MAX_SERVER_NAME_LENGTH, and the servers/add / servers/remove dispatch cases in this same file enforce name.length > MAX_NAME_LENGTH. The new resources dispatch is inconsistent with both.
An ACP/JSON-RPC client can send an arbitrarily long serverName that flows through to buildWorkspaceMcpResourcesStatus, which reflects it in the response body and error messages — enabling response amplification.
case `${QWEN_METHOD_NS}workspace/mcp/resources`: {
const serverName = String(params['serverName'] ?? '');
if (!serverName || serverName.length > MAX_NAME_LENGTH) {
// ...
}Note: the existing workspace/mcp/tools dispatch has the same gap — not a reason to skip it here, but a candidate for a follow-up cleanup.
| resource.title || resource.name || ''; | ||
| return ( | ||
| <div | ||
| key={resource.uri} |
There was a problem hiding this comment.
[Nice to have] React list key uses resource.uri alone, but the expand-state tracking above uses the NUL-separated resourceKey(server.name, resource.uri). If an MCP server ever returns duplicate URIs (protocol violation, but defensive coding is cheap), React would emit a key-collision warning.
<div key={resourceKey(server.name, resource.uri)} className={styles.tool}>
| // turns a successful reconnect/enable into a reported failure. | ||
| if (nextServer.resourceCount) { | ||
| try { | ||
| const nextResources = await mcp.loadResources(nextServer.name); |
There was a problem hiding this comment.
[Suggestion] mcp.loadTools() (line 381) and mcp.loadResources() (this line) execute sequentially — the resource fetch only begins after the tools fetch resolves. These are independent network round-trips with no data dependency. The initial-load path in App.tsx correctly parallelizes both fetches with Promise.all, so this is an inconsistency.
Reload latency is T_tools + T_resources instead of max(T_tools, T_resources). Consider fetching both in parallel with Promise.all and then updating state in the same synchronous block.
| // Disabled servers are not discovered, so leave their counts | ||
| // absent — mirrors the TUI ServerDetailStep gating. | ||
| if (!disabled) { | ||
| out.resourceCount = this.resolveServerMcpResources( |
There was a problem hiding this comment.
[Suggestion] resourceCount and promptCount assignments are not wrapped in try/catch. The resolveServerMcpResources method has defensive optional chaining for getResourceRegistry?.(), but getResourcesByServer(serverName) itself (a Map iteration + sort) could throw on corrupted registry data. Since this runs inside servers.map(...) building the base status, one throw rejects the entire /workspace/mcp response — the MCP dialog goes completely blank.
The intent is clearly defensive (the comment says "a missing registry must degrade to no resources rather than throwing and collapsing the whole /mcp status"), but the call site does not match. Consider wrapping each assignment in its own try/catch so a bad registry degrades gracefully.
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
LGTM — clean end-to-end plumbing that mirrors the existing tools pattern at every layer. The layered test coverage (daemon handler → HTTP route → SDK client → webui provider) is thorough. A few non-blocking suggestions posted as inline comments.
5 participants
What this PR does
Brings the Web Shell's
/mcpdialog to parity with the terminal UI's MCP resource browser (shipped for the TUI in #5544 and #5635). Each MCP server row now shows resource and prompt counts, and expanding a server reveals a browsable list of its resources — each resource expands to its URI, MIME type, byte size, description, and the@server:urireference you type in chat to inject the resource's contents.Because MCP resources and prompts only ever lived in the in-process core registries (
ResourceRegistry/PromptRegistry), none of this data reached the Web Shell, which talks to the daemon over the ACP/HTTP protocol. This PR plumbs it end to end: per-serverresourceCount/promptCountride the existingGET /workspace/mcpstatus payload, and a newGET /workspace/mcp/:server/resourcesdrill-down endpoint (qwen/status/workspace/mcp/resources, mirroring the existing/toolsendpoint) carries the resource metadata through the daemon handler, the ACP route table, the TypeScript SDK client, the webui workspace actions/hook, and finally theMcpDialogReact component.Why it's needed
The TUI already lets users discover and reference MCP resources from
/mcp; Web Shell users had no equivalent — they could see tools but not the resources or prompts a server advertises, and had no way to find the@server:urireference for injecting a resource into a conversation. This closes that gap so both front-ends expose the same MCP surface.Reviewer Test Plan
How to verify
Every layer is covered by unit/integration tests added in this PR:
npm --prefix packages/cli run test -- src/acp-integration/acpAgent.test.ts -t "status ext methods expose workspace snapshots" --runnpm --prefix packages/cli run test -- src/serve/server.test.ts src/serve/acp-http/transport.test.ts -t "resources" --runnpm --prefix packages/webui run test -- DaemonWorkspaceProvider --runFull gates run clean locally:
typecheckon acp-bridge / cli / webui / web-shell; builds of acp-bridge / sdk / webui / web-shell;prettier --checkandeslinton all changed files.Manual smoke (optional): start
qwen serve, open the Web Shell, configure an MCP server that advertises resources (e.g. a filesystem MCP server), run/mcp, expand the server, and confirm the resource/prompt count badges, the expandable resource list, and the@server:urireference render.Evidence (Before & After)
Before: the Web Shell
/mcpdialog showed only servers and their tools. After: it additionally shows per-server resource/prompt count badges and an expandable resource browser whose detail view matches the TUI'sResourceDetailStep(URI / Name / MIME Type / Size / Description + the@server:urichat reference). The behaviour is a direct port of the TUI browser shipped in #5635, and the data-flow at every layer is asserted by the new tests (resource payload shape,@server:urireference format, per-server counts, older-daemon fallback). Live browser screenshots were not captured in this environment; the change is covered by the layered test suite above.Tested on
Environment (optional)
Node 22 on macOS (darwin). Verification via
tsc --noEmit, package builds, andvitestunit/integration suites — no live daemon required for the test coverage.Risk & Scope
resourceCount/promptCountare computed on eachGET /workspace/mcpcall via in-memory registry scans — the same access pattern the TUI/mcpdialog already uses, with a pool-mode active-session fallback mirroring the existing tools builder. Negligible for realistic server/resource counts, and disabled servers are skipped. The SDK browser bundle cap was bumped 130KB → 131KB to accommodate the newworkspaceMcpResourcesclient method + route (legitimate growth, not a regression).McpStatusMessagechat-message component (superseded by theMcpDialogmodal, no live producer) is not extended./resourcesroute return 404, which the client degrades to an empty-with-notice result.Linked Issues
Follows up the TUI MCP resource browser (#5544, #5635) by porting it to the Web Shell. No issue to close.
中文说明
这个 PR 做了什么
让 Web Shell 的
/mcp对话框与终端 UI 的 MCP 资源浏览器对齐（该功能已在 #5544 和 #5635 为 TUI 发布）。现在每个 MCP server 行会显示资源数和 prompt 数，展开 server 后可浏览其资源列表——每个资源可展开显示 URI、MIME 类型、字节大小、描述，以及在对话中输入即可注入该资源内容的@server:uri引用。由于 MCP 资源和 prompts 一直只存在于进程内的 core 注册表（
ResourceRegistry/PromptRegistry），这些数据从未到达通过 ACP/HTTP 协议与 daemon 通信的 Web Shell。本 PR 把它纵向打通：每个 server 的resourceCount/promptCount搭在既有的GET /workspace/mcp状态负载上顺风车，新增的GET /workspace/mcp/:server/resources钻取端点（qwen/status/workspace/mcp/resources，镜像既有的/tools端点）则将资源元数据依次穿过 daemon handler、ACP 路由表、TypeScript SDK 客户端、webui 的 workspace actions/hook，最终到达McpDialogReact 组件。为什么需要
TUI 早已允许用户从
/mcp发现并引用 MCP 资源；Web Shell 用户却没有对应能力——只能看到工具，看不到 server 暴露的资源或 prompts，也无从得知用于把资源注入对话的@server:uri引用。本 PR 弥合了这个差距，让两个前端暴露相同的 MCP 能力面。Reviewer 验证计划
如何验证
每一层都有本 PR 新增的单元/集成测试覆盖：
npm --prefix packages/cli run test -- src/acp-integration/acpAgent.test.ts -t "status ext methods expose workspace snapshots" --runnpm --prefix packages/cli run test -- src/serve/server.test.ts src/serve/acp-http/transport.test.ts -t "resources" --runnpm --prefix packages/webui run test -- DaemonWorkspaceProvider --run本地全量关卡均通过：acp-bridge / cli / webui / web-shell 的
typecheck；acp-bridge / sdk / webui / web-shell 的构建；所有改动文件的prettier --check与eslint。手动冒烟（可选）：启动
qwen serve，打开 Web Shell，配置一个会暴露资源的 MCP server（如 filesystem MCP server），执行/mcp，展开 server，确认资源/prompt 计数徽章、可展开的资源列表，以及@server:uri引用正确渲染。证据（前后对比）
之前：Web Shell 的
/mcp对话框只显示 server 及其工具。之后：额外显示每个 server 的资源/prompt 计数徽章，以及可展开的资源浏览器，其详情视图与 TUI 的ResourceDetailStep一致（URI / 名称 / MIME 类型 / 大小 / 描述 +@server:uri对话引用）。该行为是 #5635 已发布的 TUI 浏览器的直接移植，且每一层的数据流都被新增测试断言（资源负载结构、@server:uri引用格式、每 server 计数、老 daemon fallback）。本环境未截取实时浏览器截图；改动由上述分层测试套件覆盖。测试平台
环境（可选）
macOS（darwin）+ Node 22。通过
tsc --noEmit、各包构建、vitest单元/集成套件验证——测试覆盖无需运行真实 daemon。风险与范围
resourceCount/promptCount在每次GET /workspace/mcp调用时通过内存注册表扫描计算——与 TUI/mcp对话框既有的访问方式相同，并带有镜像现有 tools builder 的 pool 模式 active-session fallback。对真实的 server/资源数量级可忽略，且禁用的 server 会跳过。SDK 浏览器 bundle 上限从 130KB 上调到 131KB 以容纳新增的workspaceMcpResources客户端方法 + 路由（合理增长，非回归）。McpStatusMessage聊天消息组件（已被McpDialog模态取代、无活跃生产方）未做扩展。/resources路由的老 daemon 返回 404，客户端据此降级为「空+提示」结果。关联 Issue
承接 TUI 的 MCP 资源浏览器（#5544、#5635），将其移植到 Web Shell。无需关闭的 issue。