fix(desktop): publish MCP tools through native capabilities - #4490
Conversation
37d94f6 to
0c2aae4
Compare
M4n5ter
left a comment
There was a problem hiding this comment.
English
I found two issues that should be resolved or explicitly scoped before this is treated as a complete Runtime Host fix.
-
prepareCapabilityGroups()validates MCP tools individually, but publishes every surviving MCP tool in onedesktop_mcpoffer. The protocol caps an offer at 64 tools, the full manifest at 256 tools and 56 KiB. With 65 otherwise valid small tools, the complete capability replacement is rejected withInvalid Client Capability offer tools, so MCP can also prevent Browser, Settings, Rive, and the other Desktop capabilities from registering.The existing CLI MCP provider already sorts and chunks tools before validating the complete replacement. Desktop needs equivalent aggregate handling, including a defined policy for the total-count/byte ceiling that preserves the fixed Desktop capabilities and reports why MCP publication was omitted.
-
The new E2E test calls
provider.call()directly, so it does not cross Runtime Host admission. In the default managedaskmode,managedClientCapabilityGrantTarget()only recognizes Desktop Settings and Browser; a publisheddesktop_mcpcall still fails withClient Capability has no managed admission policy.This does not require moving MCP ownership into Host. If this PR is intentionally limited to publication and Full Access invocation, say that explicitly and narrow the behavior claim/test accordingly. If it is meant to make Desktop MCP invocable on the default path, wire the existing
desktop_mcp/mcp_toolgrant into the coordinator and add a Host-level invocation test. #4012 tracks that work, and #4143 explicitly left Desktop MCP admission to a follow-up.
I did not find a useful simplification in the adapter itself: the prepared tool/descriptor pair, per-group omission policy, and shared descriptor decoder each prevent a real consistency problem.
中文
这次改动的方向没问题,但现在还不能算“Desktop MCP 已经完整走通 Runtime Host”,有两个坑需要处理清楚。
-
现在只检查了每个 MCP 工具自己是否合法,最后却把剩下的工具一股脑塞进同一个
desktop_mcpoffer。协议规定单个 offer 最多 64 个工具,整份清单最多 256 个工具、56 KiB。我用 65 个很小的合法工具复现后,整次 capability replacement 会直接报Invalid Client Capability offer tools。倒霉的不只是 MCP:Browser、Settings、Rive 等同一批注册的 Desktop 能力也会一起发布失败。CLI 那套 MCP provider 已经会先稳定排序、分组,再校验完整清单。Desktop 至少也得把总量限制管起来;超过总工具数或字节上限时,要保证固定的 Desktop 能力还能正常注册,同时明确告诉用户为什么这次没有发布 MCP,不能让一组动态工具把整包能力拖死。
-
新的 E2E 实际上是直接调用
provider.call(),没有经过 Runtime Host 的准入逻辑。默认 managedask模式里,Host 目前只给 Desktop Settings 和 Browser 配了策略;desktop_mcp就算成功出现在工具列表里,调用时仍然会报Client Capability has no managed admission policy。这里不需要把 MCP 的执行权搬到 Host。如果这个 PR 只打算修“能发布、Full Access 下能调用”,就把 PR 的行为说明和测试范围写准确;如果目标是默认模式也能用,那就把现成的
desktop_mcp/mcp_toolgrant 接到 coordinator,并补一条真正穿过 Host 的调用测试。#4012 正在跟踪这件事,#4143 也明确说 Desktop MCP admission 留到后续。
简化审计没有发现值得删的结构:prepared tool/descriptor、按 group 决定是否跳过不兼容工具,以及共享 decoder,都在避免真实的一致性问题。
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
0c2aae4 to
d5f1fb8
Compare
…ssion Grant managedClientCapabilityGrantTarget() only recognized Desktop Settings and Browser, so a managed ask-mode call to a desktop_mcp tool failed before admission with 'no managed admission policy'. Recognize the Desktop-owned desktop_mcp server identity and produce the existing desktop_mcp/mcp_tool grant target, reusing the generic approval, grant persistence, and prompt plumbing. The policy keys on the descriptor serverId so chunked offers stay admissible. Adds Host-level ask regressions covering accept-approval-admit-execute, per-tool Session Grant reuse, and the deny path (apache#4490).
A desktop_mcp group beyond the 64-tool single-offer limit failed the full manifest decode, taking Browser, Settings, and Rive registration down with it. Chunk dynamic groups into stable per-offer ids while keeping the group's server identity, shed trailing dynamic tools when the total tool, offer, or byte budgets are exceeded, keep fixed groups loud when they overflow, decode the manifest authoritatively before it is sent, and report omissions through a visible diagnostic (apache#4490).
Astro-Han
left a comment
There was a problem hiding this comment.
The direction is right and the wiring is correct: this attaches to the existing seams (the shared protocol decoder, the already-defined desktop_mcp capability and mcp_tool grant scope, which core, storage and the UI have supported since #4143 but nothing published) rather than adding a Desktop-specific MCP path. The two points from the earlier review are addressed at this head, so I did not re-litigate them. CI is green. Three P2s below, none of which block the design.
P2 (cross-cutting): the model-facing tool name gets double-prefixed. buildMcpTools already produces mcp__<server>__<tool>, the Desktop group pins every descriptor's serverId to the constant desktop_mcp, and the coordinator prefixes again at client-capability-coordinator.ts:1441/:619. Running the real mcpProxyToolName algorithm: github/create_pull_request becomes mcp__desktop_mcp__mcp__github__create_pull_request, and playwright/browser_generate_playwright_test overflows 64 chars and becomes mcp__desktop_mcp__mcp__playwright__browser_generate___9cccd640a3. The approval prompt has the same collapse: conversation-copy.ts:674 renders Allow ${toolName} from ${serverId}, and serverId is always desktop_mcp, so the real MCP server only survives inside the tool name. The mcp_tool scope was shaped as {serverId, toolName} for per-server granularity, and it now carries a constant. This was unreachable on main because no MCP tool was ever published, so this PR is what exposes it. Giving each MCP server its own offerId and serverId in the Desktop group (publishing the bare MCP tool name) would fix the naming and restore the scope's designed granularity. If the single desktop_mcp identity is deliberate because the chunking logic depends on it, please say so in the description and I will treat the naming as accepted. The same point applies to #4592.
Housekeeping: the description links no issue. This closes #4591 and #4652, and it should say so. It would also help to state that tools hitting #4614's allowlist gap are skipped here, not offered.
Overlap with #4592, for ordering: #4592 (head 42b7433) touches three files that are all a subset of this PR's Desktop files, rewrites the same capabilityOffer / toolInputSchema / requireZodSchema combination, and solves the same #4591. They conflict textually as well: both change the same assertion in runtime-host-desktop-candidate.test.ts, to incompatible values, because #4592 deliberately stops treating an unrepresentable schema as a construction failure while this PR keeps that for fixed groups. They are alternatives, not complements. This PR is the superset (it also lands #4652 and the #4143 admission follow-up), so taking it as the base and folding in #4592's two useful ideas (the per-tool warning and dropping the dead validate branch) is the cheaper ordering. Merging #4592 first would leave this PR needing a full rewrite of the same function, not a rebase.
I found no P0 or P1. managedClientCapabilityGrantTarget still gates on registration.trustedProvider, the desktop_mcp branch's lack of a tool allowlist is appropriate for a dynamic tool set, and every tool still needs its own user approval and Session Grant outside bypass.
Review apache#4490: report every tool the decoder rejects through the shared diagnostic channel instead of dropping it silently; drop the unreachable validate branch in parseToolArguments (JSON-Schema tools have no client-side validator); narrow the chunking comment to the scope and published names the grant key actually pins; rename omitUnsupportedTools to dynamic with a comment covering all three behaviors it gates.
Review apache#4490: the shared desktop_mcp server identity double-prefixed every model-facing tool name and collapsed the mcp_tool grant scope's per-server granularity to a constant. Publish one dynamic offer per MCP server with the descriptor carrying the normalized real serverId and bare tool name, so the model sees mcp__<server>__<tool> and approval prompts and Session Grants are scoped per server. buildMcpToolsWithIdentities exports the source identity from a single snapshot read; clientCapabilityEntityId moves to the protocol package and is shared with the CLI provider; Host admission matches the desktop_mcp offer prefix.
…ility-schema Resolve packages/runtime/src/mcp-tools.ts: keep buildMcpToolsWithIdentities and fold in main's requestInteraction plumbing (apache#4379).
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head 04fc935f7030604238b5e29efad12e942a780456 (OPEN, hosted test green). No P0–P3; nothing here blocks merging on technical grounds.
What was checked
MCP native-publish schema/descriptor boundaries, per-server offers against the 64/256/32/56 KiB budgets, dynamic-degradation diagnostics, host-managed admission with server/tool scopes, refuse/offline/unknown-result paths, and replace-vs-concurrent paths. Local verification: runtime-host coordinator 32/32, runtime MCP 10/10, desktop native-capabilities 21/21, MCP end-to-end 1/1, protocol tests 76/76, @maka/desktop build passes, git diff --check clean.
What I could not judge
The root-workspace full npm run build is still blocked by pre-existing UI type errors this PR does not touch (streaming/autoScroll/trailingAction and similar) — recorded here so it is not misattributed to this change either way. Remaining merge requirements are the normal independent-review/branch-protection gates.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
本条结论全部来自 @未开智选手 的审查。我自己没有读这份 diff;我核的是当前 head 有没有漂移、以及 exact-head 的 CI 状态。当前 head 是 04fc935,test 通过。技术上无阻断问题,根目录全量构建的既有 UI 类型错误与本 PR 无关。合并仍需走正常独立复核与分支保护门禁。
M4n5ter
left a comment
There was a problem hiding this comment.
Reviewed exact head 04fc935f7030604238b5e29efad12e942a780456.
The two previously reported failures are closed. The default managed ask path now admits trusted Desktop MCP tools through a tool-scoped Session Grant, and large MCP catalogs are decoded per tool, split within the protocol's 64-tool offer limit, and deterministically degraded without removing healthy fixed capabilities. The published descriptor and dispatch path retain the real MCP server/tool identity from one immutable snapshot.
I found no remaining P0–P3 issue in this change. The exact-head hosted check is green. Focused exact-head tests passed 102/102 across Desktop, Runtime, Runtime Host, and the CLI provider. I also built and tested the current-main merge result because mcp-tools.ts changed on both sides; its relevant builds passed and the combined focused set passed 98/98.
Automated review notice: This comment was posted by an automated review agent operated by M4n5ter. It is not an independent human review and does not replace one.
Summary
Desktop can connect to an MCP server while still failing to publish its tools to Runtime Host: MCP tools carry an AI SDK JSON Schema wrapper, but the native-capability adapter previously accepted Zod only. The connection then appears healthy while the model cannot see or call the tool.
This PR covers the full Desktop MCP capability path:
prefixItemsproduced by a pydantictuple[...]), whose description exceeds 8192 chars, or whose schema exceeds 32 KiB are skipped with a per-tool diagnostic — never offered.desktop_mcptools go through Host managed admission. Managed ask mode produces the existingdesktop_mcp/mcp_toolSession Grant (enhancement(runtime): allow Auto mode to use tools currently restricted to Full Access on macOS #4012); Host-level regressions cover accept → approval → admit → execute, per-tool grant reuse, and the deny path.serverIdand bare tool name, so model-facing names staymcp__<server>__<tool>and approval prompts ("Allowcreate_pull_requestfromgithub") and Session Grant scopes keep their designed per-server granularity. A server change re-prompts only that server's tools.Closes #4591, closes #4652.
Relationship to #4592: this PR is the superset (it also lands #4652 and the #4143 admission follow-up); #4592's two useful ideas — the per-tool omission warning and dropping the dead
validatebranch — are folded in here.Verification
npm run buildnode --teston the affected suites:packages/runtime-hostclient-capability coordinator / protocol / admission-integration / invocation-broker / channel (47 passed, including the new Host-level ask regressions),packages/runtimemcp-tools (10),packages/clicapability provider command + remote publication (19),apps/desktopnative-capabilities / mcp-runtime-e2e / desktop-candidate / browser-tools / session-execution-ipc (83)The end-to-end test connects the real stdio MCP fixture, builds its tools, publishes them through the Desktop capability provider under their real server identity, validates the protocol payload, and invokes the offered tool. No screenshot applies: this repairs capability publication behind the existing MCP settings UI.
AI use
Tool(s) and scope: AI coding assistants assisted investigation, implementation, tests, and review; the commits include the required trailer.
Checklist
Does this PR entail a change in behavior?