Skip to content

fix(desktop): accept jsonSchema() tool params in native capability offers - #4592

Open
liuxiaocs7 wants to merge 5 commits into
apache:mainfrom
liuxiaocs7:fix/desktop-mcp-native-capability-jsonschema
Open

fix(desktop): accept jsonSchema() tool params in native capability offers#4592
liuxiaocs7 wants to merge 5 commits into
apache:mainfrom
liuxiaocs7:fix/desktop-mcp-native-capability-jsonschema

Conversation

@liuxiaocs7

@liuxiaocs7 liuxiaocs7 commented Sep 2, 2026

Copy link
Copy Markdown
Member

Summary

MCP tools configured on the Desktop app (including servers added in the TUI, which share the workspace's mcp.json) never loaded: the Runtime Host logged MCP startup failed: ... Desktop native capability tool has an invalid schema on every change, and no Desktop capability reached sessions — not just MCP, but Browser, Computer Use, Client settings and Rive too.

Two causes, both fixed here.

1. The offering side rejected non-Zod (jsonSchema()) parameters. buildMcpTools() sets each MCP tool's parameters to jsonSchema(descriptor.inputSchema) — an AI SDK Schema, not a Zod type — but the Desktop native-capability provider offered and validated tools through requireZodSchema (tool.parameters instanceof z.ZodType), so it threw on the first MCP tool. The receiving side (client-capability-coordinator) already rebuilds the same tools with buildMcpTools and accepts jsonSchema(); this aligns the offering side.

2. One unrepresentable tool took every Desktop capability down with it. provider.offers() is built and sent as a single registration frame (client-capability-channel.ts replace()), so any throw while building the frame dropped every capability at once — which is why the report saw nothing work and only a console line. This also still bit MCP tools whose JSON Schema uses a keyword outside the Client Capability allowlist (prefixItems, not, patternProperties, contentEncoding, if/then/else, deprecated) — ordinary output, e.g. a pydantic tuple[...]prefixItems.

capabilityOffer now builds each tool's descriptor independently and validates it through the same decodeClientCapabilityReplaceInput the Runtime Host runs. An unrepresentable tool is logged and skipped; the rest of that capability — and every other capability — still registers. A tool with a not-yet-supported schema keyword now costs one tool, not the whole app. Each tool is probed inside its group's real offer metadata, and bindings are built from the resolved offers, so the provider only ever dispatches a tool it actually advertised.

Out of scope / follow-ups:

Details:

  • toolInputSchema reads the JSON Schema directly from an AI SDK Schema when parameters is not Zod, copying it before deleting $schema / freezing so the (already upstream-frozen) MCP inputSchema is never mutated.
  • Call-time validation (parseToolArguments) mirrors the runtime's validateDeclaredToolArgs: Zod parseAsync, else pass-through for JSON-schema-only MCP tools (which carry no client-side validator), else throw with a call-path-specific message. The dead validate branch (no producer) and its NativeToolValidation type are gone.

Refs #4591 — the per-tool isolation makes any one unrepresentable tool survivable, but the protocol's aggregate limits (>64 tools/offer, 256 total, 56 KiB manifest) can still fail a large MCP config's whole registration. That half is tracked in #4652, so this does not close the issue.

Verification

  • npm --workspace @maka/desktop run build:main (tsc) — passes, no type errors.
  • node --test apps/desktop/dist/main/__tests__/runtime-host-native-capabilities.test.js18/18 pass, including:
    • offers and dispatches MCP tools whose parameters are JSON Schema, not Zod — runs real buildMcpTools() output through the offer + call paths; fails without the parameters fix.
    • drops one unrepresentable tool instead of failing every Desktop capability — an MCP tool whose schema uses prefixItems is skipped while a valid sibling tool and every other Desktop capability still register and the frame still encodes; fails without the isolation.
  • biome lint on the changed files — clean. (apps/desktop/** is formatter-excluded per biome.jsonc; existing quote conventions matched.)

Not run: the full Electron npm run dev end-to-end launch (headless environment). The tests exercise the same offer + call code paths, including protocol encoding via decodeClientCapabilityReplaceInput.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code diagnosed the root cause, implemented the fix and tests, and drafted this PR. The authoring commits carry a Generated-by: Claude Code trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, typecheck and the affected suite pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

…fers

MCP tools built by buildMcpTools() carry AI SDK jsonSchema() parameters,
not Zod schemas. The Desktop native-capability provider offered and
validated tools through requireZodSchema (instanceof z.ZodType), so it
threw "Desktop native capability tool has an invalid schema" on the first
MCP tool — no MCP tool from any configured server was ever offered to
sessions on Desktop (MCP added in the TUI was unusable in the Desktop app).

toolInputSchema now reads the JSON Schema directly from an AI SDK Schema
(cloning it before deleting $schema / freezing, so the MCP descriptor's
shared inputSchema is never mutated), and call-time validation goes through
a new parseToolArguments helper that mirrors the runtime's
validateDeclaredToolArgs precedence: Zod parse, AI SDK validate, or
pass-through for JSON-schema-only MCP tools. Zod-based tools are unchanged.

Adds a test that runs real buildMcpTools() output through the provider,
covering both the offer and the call paths; it fails without the fix.

Fixes apache#4591

Generated-by: Claude Code
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 2, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The diagnosis is right and I reproduced it. mcp-tools.ts:117 sets parameters: jsonSchema(descriptor.inputSchema), which has no Zod prototype, and requireZodSchema at runtime-host-native-capabilities.ts:465 throws on it from both toolInputSchema and invokeNativeTool. The receiving side really does already accept jsonSchema() through buildMcpTools, so the offering side is the right place to fix. The new test fails against HEAD~1 with exactly the error from #4591, so it is pinning something real. Build, the 15 native-capability tests, format and lint are green locally.

Two things the body understates, and I think one of them changes what this PR should contain.

One bad tool takes every Desktop capability with it. provider.offers() is called inside replace() at client-capability-channel.ts:97 for the whole frame, so the throw does not just drop MCP: Browser, Computer Use, Client settings and Rive never register either. That matches the report, where nothing worked and only a console line appeared. Worth stating in the issue and the body, because it is most of the severity.

The same failure still happens after this merge. client-capability.ts:719 holds a keyword allowlist and :762 rejects the frame on the first key outside it. prefixItems, not, patternProperties, contentEncoding, if/then/else and deprecated are all absent from it. I ran real buildMcpTools() output through provider.offers() and decodeClientCapabilityReplaceInput on this branch: plain schemas and uniqueItems pass, those six fail with Unsupported Client Capability tool schema keyword, same collateral. These are ordinary output, a pydantic tuple[...] gives prefixItems and bytes gives contentEncoding, and packages/mcp/src/index.ts:2311 deliberately preserves several of them, so the repo already expects to see them.

So the change is correct but the claim it settles is not, and #4591 would close while a real share of servers stays broken the same way. My preference is to fix the isolation here rather than the allowlist: a try/catch per tool in capabilityOffer, skipping and warning on a tool that cannot be expressed instead of failing the frame. That is small, it makes this PR's own claim true for more servers, and it means the third cause of an unrepresentable schema costs one tool rather than every capability. Widening the allowlist can then be its own change with its own protocol thinking. If you would rather keep this PR to the Zod half, that is fine too, but please narrow the summary and open the follow-up, because as written the PR reads as "MCP works on Desktop now".

Three smaller notes inline. The largest is that parseToolArguments's validate branch has no producer, so about a third of the diff can go.

Evidence boundary: static read of 93f65f5d against main 8ea3c4f0; build, the native-capability test file, format and lint run locally; the keyword failures reproduced on this branch's own dist with real buildMcpTools output; no Electron launch.

AI-assisted review: drafted with Maka; I verified the allowlist contents, the whole-frame call site and the HEAD~1 test failure myself.

简体中文

诊断是对的,我也复现了。mcp-tools.ts:117parameters 设成 jsonSchema(...),它没有 Zod 原型,runtime-host-native-capabilities.ts:465requireZodSchematoolInputSchemainvokeNativeTool 两处都会抛。接收侧确实已经通过 buildMcpTools 接受 jsonSchema(),所以改提供侧是对的落点。新测试对 HEAD~1 确实会红,报的就是 #4591 里那句。本地 build、15 条 native-capability 测试、format、lint 都绿。

正文少说了两件事,其中一件我认为会改变这个 PR 该包含什么。

一个工具坏掉会把桌面端所有能力一起带走。 provider.offers() 是在 client-capability-channel.ts:97replace() 里整帧调用的,所以抛出不只让 MCP 消失:Browser、Computer Use、Client settings、Rive 也一并注册不上。这和报告里的现象一致,什么都没有,只在 console 留一行。严重度大半在这里,建议在 issue 和正文里写明。

合并之后同样的失败还会发生。 client-capability.ts:719 是一张关键字白名单,:762 在遇到表外的第一个 key 时就拒掉整帧。prefixItemsnotpatternPropertiescontentEncodingif/then/elsedeprecated 都不在表里。我在这个分支上用真实的 buildMcpTools() 产物跑了 provider.offers()decodeClientCapabilityReplaceInput:普通 schema 和 uniqueItems 能过,上面这六个报 Unsupported Client Capability tool schema keyword,连坐范围一样。这些都是很常见的产物,pydantic 的 tuple[...] 会出 prefixItemsbytes 会出 contentEncoding,而 packages/mcp/src/index.ts:2311 本身就特意保留了其中几个,说明仓库早就预期会遇到。

所以改动本身没错,但它宣称解决的事情没有成立,#4591 会被关掉,而相当一部分 server 仍以同样的方式坏着。我更希望在这里修隔离而不是白名单:在 capabilityOffer 里按工具 try/catch,遇到无法表达的工具就跳过并 warn,而不是让整帧失败。这个改动很小,能让本 PR 自己的结论对更多 server 成立,而且以后第三种无法表达的 schema 出现时,代价只是少一个工具,不是所有能力消失。放宽白名单可以单独做,那是另一件需要协议层判断的事。如果你更想把这个 PR 保持在 Zod 这一半,也可以,但请把 summary 收窄并开一个 follow-up,因为现在正文读起来就是「桌面端的 MCP 好了」。

行内还有三条小的,最大的一条是 parseToolArgumentsvalidate 分支没有生产者,删掉能少三分之一体量。

Comment thread apps/desktop/src/main/runtime-host-native-capabilities.ts
Comment thread apps/desktop/src/main/runtime-host-native-capabilities.ts
Comment thread apps/desktop/src/main/runtime-host-native-capabilities.ts
… survivable

Review on apache#4592 surfaced two gaps the jsonSchema() fix left open.

The offer frame is built and sent as a unit (client-capability-channel
replace()), so a single tool whose schema cannot be expressed threw for the
whole registration -- dropping Browser, Computer Use, Client settings, Rive
and MCP together, not just the offending tool. That is most of the apache#4591
severity, and it still bit any MCP tool whose schema uses a keyword outside
the Client Capability allowlist (prefixItems, not, patternProperties,
contentEncoding, if/then/else, deprecated) -- e.g. a pydantic tuple[...].

capabilityOffer now builds each tool's descriptor independently and probes it
through the same decodeClientCapabilityReplaceInput the Runtime Host runs. An
unrepresentable tool is logged and skipped; the rest of that capability -- and
every other capability -- still registers. Widening the protocol allowlist is
left as a separate change that needs its own protocol review.

parseToolArguments had a validate branch with no producer: the only non-Zod
source, buildMcpTools, calls jsonSchema() with one argument, so validate is
always undefined. Dropped it, the thenable guard and the NativeToolValidation
type; call-time parsing is now Zod parseAsync, else JSON-schema pass-through,
else throw with a distinct message so its logs stay distinguishable from the
offer path.

Corrected the cloneNativeToolJsonSchema comment: MCP inputSchema is already
$schema-stripped and deep-frozen upstream (packages/mcp), so the copy is
defensive, not load-bearing.

Adds a test: an MCP tool whose schema uses prefixItems is dropped while a valid
sibling tool and every other Desktop capability still register and the frame
still encodes. Fails without the isolation.

Generated-by: Claude Code
@liuxiaocs7

Copy link
Copy Markdown
Member Author

Thanks for the careful review — reproducing the whole-frame blast radius and running real buildMcpTools() output through the allowlist was exactly the right thing to check. Pushed 8cb1f53 addressing all of it.

Isolation (main comment + inline on toolInputSchema). capabilityOffer now builds each tool's descriptor on its own and validates it through the same decodeClientCapabilityReplaceInput the Runtime Host runs, inside a per-tool try/catch. An unrepresentable tool is logged and skipped; the rest of that capability and every other capability still register. This deliberately covers the schema-keyword rejections too, not just the Zod throw: an MCP tool using prefixItems (or not, patternProperties, contentEncoding, if/then/else, deprecated) now costs one tool instead of the whole app. New test drops one unrepresentable tool instead of failing every Desktop capability pins this with a real buildMcpTools() prefixItems schema — it fails without the isolation (the bad tool survives and the frame throws).

Widening the allowlist is left as a separate change, as you suggested — it touches the protocol package and wants its own review. Happy to open the follow-up.

parseToolArguments validate branch. Removed — you're right there's no producer (buildMcpTools calls jsonSchema() with one argument, so validate is always undefined). Dropped the branch, the thenable guard in aiSchemaJson, and the NativeToolValidation type; call-time parsing is now Zod parseAsync, else JSON-schema pass-through, else throw. The thenable case falls through to toolInputSchema's object-shape check as you noted.

cloneNativeToolJsonSchema comment + shared error string. Corrected the comment: MCP inputSchema is already $schema-stripped and deep-frozen upstream (packages/mcp), so the copy is defensive, not load-bearing. The call path now throws a distinct message (... cannot parse call arguments) so its logs stay distinguishable from the offer path.

PR body narrowed to say what's actually true now: both causes are fixed, unrepresentable-schema tools are skipped rather than fatal, and allowlist-widening is called out as follow-up.

…tion

"closes the claimed Host connection when native capability construction fails"
triggered the failure with an unrepresentable tool schema. With per-tool offer
isolation such a tool is now skipped and warned rather than fatal, so
construction succeeds and the expected rejection never fired. Switch the
trigger to a genuine construction error -- two tools colliding on one name --
so the connection-cleanup contract is still exercised.

Generated-by: Claude Code
…a separately

Addresses two gaps in the per-tool offer isolation (review of apache#4592).

P2: bindings were built from the raw groups, so a tool dropped from its offer
(unrepresentable schema) stayed dispatchable via provider.call() even though it
was never advertised. Build bindings from the resolved offers instead, so the
provider only dispatches a tool it actually advertised.

P3: the per-tool probe validated group metadata (offerId, label) together with
the tool, so a misconfigured group reported every tool as having an
unrepresentable schema. Validate offer-level metadata once, separately, and
probe each tool against constant known-valid metadata, so group-level and
tool-level failures are diagnosed distinctly.

Adds tests: a dropped tool is not dispatchable; a capability with invalid
metadata is skipped with a metadata-level diagnostic, not a tool-schema one.

Aggregate protocol limits (>64 tools/offer, 256 total, 56 KiB manifest) can
still fail the whole registration for a large MCP config; tracked in apache#4652.

Generated-by: Claude Code
@liuxiaocs7

Copy link
Copy Markdown
Member Author

Follow-up review surfaced two more issues in the isolation I added; pushed 42b7433:

  • Bindings tracked the raw groups, not the advertised offers. A tool dropped from its offer (unrepresentable schema) was still dispatchable via provider.call() even though it was never advertised. Bindings are now built from the resolved offers, so the provider only dispatches a tool it advertised. Test added: a dropped tool now rejects with not offered.
  • The per-tool probe also validated group metadata, so a misconfigured group (bad offerId/label) was reported as every tool having an unrepresentable schema. Offer-level metadata is now validated once, separately, and each tool is probed against constant known-valid metadata, so group-level and tool-level failures are diagnosed distinctly. Test added.

One thing this PR still does not cover: the protocol's aggregate limits (>64 tools/offer, 256 total, 56 KiB manifest). Per-tool validation can't see them, so a large MCP config (e.g. a server exposing 65 valid tools — MCP discovery allows up to 1000) still fails the whole registration in replace() and drops every capability. Splitting/trimming the desktop_mcp offer needs a bit of design (offer-splitting + deterministic budget trimming + a final full-manifest validation), so I've filed it as a dedicated follow-up: #4652. Happy to fold it in here instead if you'd prefer it in one PR.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Both rounds land. Taking the four earlier points in order, at 42b7433.

Per-tool isolation is real and I checked it does not leak. offerableToolDescriptor wraps descriptor construction, toolInputSchema and the probe decode in one try, and capabilityOffer only pushes and freezes afterwards, so the three shapes that used to kill the frame (a non-Zod parameters, a keyword outside the allowlist, an over-long tool name) now each cost one tool. What can still throw during construction is buildMcpTools's name-collision check at mcp-tools.ts:100, the duplicate-service check, and indexBindings' duplicate-name check, all of which predate this change and are deliberately covered. Worth knowing that the collision one is reachable: sanitizeNamePart collapses punctuation, so servers named my-server and my.server collide and take the whole registration with them.

The narrowed claim matches the code. The eight keywords are still absent from CLIENT_CAPABILITY_SCHEMA_KEYWORDS and :762 still rejects on the first one, and #4614 names the right landing spot including the epoch cost.

The validate branch is gone with no producer left. There are still three jsonSchema(..., { validate }) callers, but they are all Runtime-side builtins; every Desktop capability group comes from runtime-host-boot.ts:977-1005, where the four non-MCP groups are Zod and MCP calls jsonSchema() with one argument. The thenable case does fall through to the object-shape check as you said.

Bindings and metadata: offers and bindings now come from the same resolved, and since a descriptor's serverId is its group.offerId, a tool cannot be advertised without being bound or the reverse.

The thing I want to single out: probing each tool through decodeClientCapabilityReplaceInput rather than reimplementing the keyword table in Desktop means "what can be expressed" still has exactly one authority. That is the right call and it is what makes the isolation cheap to trust. 256 single-tool probes measure under 7 ms, so the cost is not a concern.

The claim is still larger than the fix

CLIENT_CAPABILITY_MAX_TOOLS_PER_OFFER is 64, and runtime-host-boot.ts:995-1003 puts every MCP tool from every server into the single desktop_mcp offer with no cap. So 65 tools whose schemas are all perfectly representable still fail the whole frame in replace(), and Browser, Computer Use, Client settings and Rive go down with them, leaving one console line. Same at 256 tools total and at a 56 KiB manifest, which a handful of ordinary schemas reach. GitHub's own MCP server exposes around 70 tools by itself and discovery allows up to 1000, so this is not a large configuration, it is a common one.

Per-tool probing cannot see an aggregate by construction, so this is not a gap in your isolation, it is where you drew the boundary. The defect predates the PR. What does not hold is Fixes #4591: merging as written closes an issue whose exact symptom is still one MCP server away, with no new diagnostic, because every tool passes its own probe.

This does not need #4652's design, because the repo already contains the end state. packages/cli/src/mcp-capability-provider.ts:38-99 sorts tools by (serverId, name), chunks into offers of at most CLIENT_CAPABILITY_MAX_TOOLS_PER_OFFER, and validates the whole assembled frame once with decodeClientCapabilityReplaceInput before serving it. Desktop cannot reuse it verbatim, since it dispatches through tool.impl and the CLI through manager.callTool, but the validation shape transfers directly: assemble, decode the frame once, shed or chunk on failure, serve the canonical result. That is one owner instead of three, it covers the aggregates, and it deletes capabilityMetadataValid on the way through.

If you would rather keep the boundary where it is, that is defensible, but then this should say Refs #4591 and the issue should stay open.

Smaller

capabilityMetadataValid, OfferMetadata and the three probe constants, about 45 lines, buy log wording only. All four production offerIds are literals in runtime-host-boot.ts, so a bad offerId is a Desktop coding error that cannot reach production, and the test that covers it asserts on console.warn text. Under the whole-frame check above, both the mechanism and its test go.

One test changed meaning without changing text, and this is the one I would fix even if nothing else changes. publishes every production Desktop-owned tool schema through the protocol asserts only doesNotThrow on provider.offers(). Before this PR that proved every Client-settings and Rive tool was representable, because an unrepresentable one threw. Now a dropped tool simply vanishes and the remainder still decodes, so the test passes; it fails only if every offer is dropped. A future Desktop-owned tool with an unrepresentable schema will disappear silently with green CI. Asserting the expected tool names per offer is four lines.

parseToolArguments's comment says MCP arguments "are validated by the receiving Runtime Host against the same schema". They are not. The Host rebuilds the tools with buildMcpTools, and validateDeclaredToolArgs returns early when there is no safeParse, validate or ~standard. No observable consequence, the MCP server validates its own input and this matches the TUI, but the comment promises something that does not exist.

Body still says 16/16; head has 18.

One follow-up rather than a change here

A skipped tool is invisible where the user looks. mcp-page.tsx:917-924 renders status.tools from the MCP manager, which knows nothing about the probe, so Settings lists a tool as live while the model does not have it, and the only diagnostic is a main-process console line. Still a much better trade than the current total outage, so not a blocker, but skip-and-warn does swap a loud failure for an invisible one. McpServerStatus already carries error and stderrTail as a user-visible seam. An issue is a fine resolution.

Value

This should exist. The problem is demonstrated rather than hypothesized, it fires on any MCP server on Desktop, and the cost is every native capability disappearing. It fixes the cause, a disagreement between the two sides about the shape of parameters, at the only place they disagree, and the receiving side was already the more permissive one.

The second half is not scope creep either. With only the first commit, one pydantic tuple[...] parameter reproduces #4591 verbatim: same total outage, different keyword. Isolation is the rest of the same bug.

The shape is better than the first round, for the reason above: the offer path borrows the Host's decoder instead of growing a second copy of the rules, and ResolvedCapability folds two pieces of state back into one. Neither is an added abstraction. There is no case for closing this. The reductions are the 45 lines and, if you take the whole-frame check, the two probes collapsing into one.

Evidence boundary: read at 42b7433 against main b9748a77, with the two response commits diffed individually; the protocol limits, the single desktop_mcp offer, the eight missing keywords, and every producer of parameters in Desktop capability groups verified from source; the probe timing and the frame rejections reproduced against the packages/runtime-host dist built from main with hand-constructed frames matching the head's probe shape, not against this branch's own output; no build, no test run, no Electron launch, no real MCP server.

AI-assisted review: drafted with Maka. I verified the aggregate limits, the single desktop_mcp offer and the four prior points against source myself.

简体中文

两轮都落地了。先按顺序说之前那四点,基于 42b7433

逐工具隔离是真的,我核了它没有漏。offerableToolDescriptor 把构描述符、toolInputSchema、探针 decode 整体包在一个 try 里,capabilityOffer 之后只做 push 和 freeze,所以以前能杀掉整帧的三种形状(非 Zod 的 parameters、白名单外的关键字、超长工具名)现在各只损失一个工具。构造期仍在 try 之外的抛点是 mcp-tools.ts:100 的重名检查、重复服务检查和 indexBindings 的重名检查,三者都先于这次改动,也都是有意保留的覆盖。值得知道的是重名那条其实可达:sanitizeNamePart 会把标点收掉,所以 my-servermy.server 两个 server 会撞名,并且带走整个注册。

收窄后的结论和代码一致。那八个关键字确实都不在 CLIENT_CAPABILITY_SCHEMA_KEYWORDS 里,:762 仍在遇到第一个时就拒整帧;#4614 的落点写对了,也点出了要动 epoch。

validate 分支删掉后确实没有生产者了。带 validatejsonSchema() 调用还剩三处,但都是 Runtime 侧内建工具;Desktop 的 capability group 全部来自 runtime-host-boot.ts:977-1005,四个非 MCP 组是 Zod,MCP 走单参 jsonSchema()。thenable 那条确实会落到对象形状检查上。

bindings 与 metadata:offers 和 bindings 现在同源 resolved,而描述符的 serverId 就是它的 group.offerId,所以不会出现 advertised 但未绑定、或绑定但未 advertised。

有一点我想单独说:用 decodeClientCapabilityReplaceInput 逐工具探针,而不是在 Desktop 重写一份关键字表,意味着「什么可表达」仍然只有一个权威。这个选择是对的,也正是隔离能被放心信任的原因。256 次单工具探针实测不到 7 ms,成本不是问题。

结论仍然大于改动

CLIENT_CAPABILITY_MAX_TOOLS_PER_OFFER 是 64,而 runtime-host-boot.ts:995-1003 把所有 server 的所有 MCP 工具塞进单个 desktop_mcp offer,没有任何上限。所以 65 个 schema 完全合法的工具照样在 replace() 里整帧被拒,Browser、Computer Use、Client settings、Rive 一起消失,只留一行 console。总数 256 和 56 KiB manifest 同理。GitHub 官方 MCP server 一家就约 70 个工具,discovery 允许到 1000,所以这不是「很大的配置」,是常见配置。

逐工具探针在结构上看不见聚合量,所以这不是你隔离做得不够,而是你画的边界。缺陷本身也先于这个 PR。站不住的是 Fixes #4591:照现在合并,会关掉一个再接一个 MCP server 就能原样复现的 issue,而且因为每个工具都能通过自己的探针,新加的这套机制什么都不会报。

这不需要 #4652 的设计,因为仓库里已经有了那个终态。packages/cli/src/mcp-capability-provider.ts:38-99(serverId, name) 排序、按 CLIENT_CAPABILITY_MAX_TOOLS_PER_OFFER 分块成多个 offer,然后用 decodeClientCapabilityReplaceInput整帧校验一次再对外提供。Desktop 不能原样复用(它经 tool.impl 分发,CLI 经 manager.callTool),但校验的形状可以直接搬:组装、整帧解码一次、失败就分块或丢弃、提供规范化结果。这是一个权威而不是三个,它覆盖得到聚合量,顺手把 capabilityMetadataValid 一起删掉。

如果你更想把边界留在现在的位置,也讲得通,但那就该写 Refs #4591,issue 留着。

可以更小

capabilityMetadataValidOfferMetadata 和三个探针常量,约 45 行,只买到日志措辞。四个生产 offerId 都是 runtime-host-boot.ts 里的字面量,坏 offerId 是 Desktop 自己的编码错误,生产路径不可达,而覆盖它的那条测试断言的是 console.warn 的字符串。上面那个整帧校验一落地,机制和测试一起走。

有一条测试文本没改但含义变了,这条我认为即使别的都不动也该修。publishes every production Desktop-owned tool schema through the protocol 只断言 provider.offers() 不抛。改动前这证明了每个 Client settings 和 Rive 工具都可表达,因为不可表达的会抛;改动后被丢的工具直接消失,剩下的照样解码,测试照过,它现在只在所有 offer 都被丢时才红。将来某个 Desktop 自有工具的 schema 不可表达,它会在 CI 全绿的情况下悄悄消失。逐 offer 断言期望的工具名,四行的事。

parseToolArguments 的注释说 MCP 的参数「are validated by the receiving Runtime Host against the same schema」。并非如此。Host 用 buildMcpTools 重建,validateDeclaredToolArgs 在没有 safeParse/validate/~standard 时直接返回。没有可观察后果,MCP server 自己会校验,也和 TUI 现状一致,但这条注释承诺了一个不存在的保证。

正文还写着 16/16,head 上是 18 条。

这条建议单开而不是在这里改

被跳过的工具在用户能看到的地方是隐形的。mcp-page.tsx:917-924 直接渲染 MCP manager 的 status.tools,而它不知道探针的存在,所以设置里把一个工具列为在线,模型手上却没有,唯一的诊断是一行用户永远看不到的主进程日志。这仍然远好于现在的整体停摆,所以不阻塞,但 skip-and-warn 确实把一次响亮的失败换成了一次隐形的失败。McpServerStatus 已经有 errorstderrTail 这个用户可见的接缝。开个 issue 就够。

价值

这个改动该存在。问题是复现出来的而不是假想的,Desktop 上任何 MCP server 都触发,代价是全部原生能力消失。它修的是因,也就是协议两侧对 parameters 形状的分歧,落在唯一的分歧点上,而且接收侧本来就是更宽容的那一侧。

后半部分也不是范围蔓延。只有第一个 commit 的话,一个 pydantic 的 tuple[...] 参数就能原样复现 #4591:同样的整体停摆,只是换了个关键字。隔离是同一个 bug 的剩下一半。

形状比第一轮更好,理由就是上面那点:offer 侧现在借用 Host 的解码器,而不是长出第二份规则;ResolvedCapability 把两份状态并回一份。这两处都不是新增抽象。没有关掉它的理由。可以削减的是那 45 行,以及如果采纳整帧校验,两个探针合成一个。

@Astro-Han

Copy link
Copy Markdown
Contributor

Severities for the review above, which I should have included with it.

P1 — the aggregate limits still take every Desktop capability down. Normal supported operation, and a common configuration rather than an edge: CLIENT_CAPABILITY_MAX_TOOLS_PER_OFFER is 64 while runtime-host-boot.ts:995-1003 puts every MCP tool from every server into one desktop_mcp offer with no cap, so 65 perfectly representable tools fail the whole frame in replace() and Browser, Computer Use, Client settings and Rive go with them, leaving one console line. Same at 256 total and at a 56 KiB manifest. The defect is pre-existing, and per-tool probing cannot see an aggregate by construction, so this is not a hole in your isolation. What it does mean is that Fixes #4591 closes an issue still one MCP server away from reproducing. Either decode the assembled frame once and shed or chunk on failure, as packages/cli/src/mcp-capability-provider.ts:38-99 already does, or say Refs #4591 and leave the issue open.

P2 — publishes every production Desktop-owned tool schema through the protocol no longer proves anything. Normal path, developer-facing: it asserts only doesNotThrow, and now that an unrepresentable tool is dropped rather than thrown, it fails only if every offer disappears. A future Desktop-owned tool with a bad schema vanishes with green CI. Asserting the expected tool names per offer is four lines.

P2 — a skipped tool is invisible to the user and contradicted by the UI. Normal path: mcp-page.tsx:917-924 renders status.tools from the MCP manager, which knows nothing about the probe, so Settings lists a tool as live while the model does not have it, and the only diagnostic is a main-process console line. Still a much better trade than today's total outage, so a follow-up issue is a fine resolution; McpServerStatus already carries error and stderrTail.

P3 — parseToolArguments' comment claims a validator that does not exist. No observable consequence, but it makes an unvalidated path read as validated.

P3 — capabilityMetadataValid and its constants, about 45 lines, buy log wording only, and its test asserts on console.warn text. Both go under the whole-frame check.

P3 — the body still says 16/16; head has 18.

No finding on the four points from the earlier round, all of which are genuinely fixed, nor on binding and offer consistency, nor on the per-tool probe as a second authority (it borrows the Host's own decoder, which is the right call), nor on its cost (256 probes measure under 7 ms).

The P1 is a judgement call rather than a defect you introduced: decide whether this PR carries the aggregate case or whether the issue stays open. Everything else is small.

简体中文

上面那份 review 的分级,应该跟着一起给的。

P1 —— 聚合上限仍然会把桌面端所有能力一起带走。 正常支持路径,而且是常见配置而不是边角:CLIENT_CAPABILITY_MAX_TOOLS_PER_OFFER 是 64,而 runtime-host-boot.ts:995-1003 把所有 server 的所有 MCP 工具塞进单个 desktop_mcp offer 且没有上限,所以 65 个完全可表达的工具照样在 replace() 里整帧被拒,Browser、Computer Use、Client settings、Rive 一起消失,只留一行 console。总数 256 和 56 KiB manifest 同理。缺陷本身先于这个 PR,而逐工具探针在结构上就看不见聚合量,所以这不是你隔离做得不够。它的实际含义是 Fixes #4591 会关掉一个再接一个 MCP server 就能复现的 issue。要么对组装好的帧解码一次、失败就分块或丢弃(packages/cli/src/mcp-capability-provider.ts:38-99 已经是这个写法),要么改成 Refs #4591 把 issue 留着。

P2 —— publishes every production Desktop-owned tool schema through the protocol 已经证明不了任何事。 正常路径,面向开发者:它只断言不抛,而现在不可表达的工具是被丢掉而不是抛出,所以它只在所有 offer 都消失时才红。将来某个 Desktop 自有工具 schema 坏掉,会在 CI 全绿的情况下消失。逐 offer 断言期望的工具名,四行。

P2 —— 被跳过的工具对用户是隐形的,而且和界面自相矛盾。 正常路径:mcp-page.tsx:917-924 直接渲染 MCP manager 的 status.tools,它不知道探针的存在,所以设置里把一个工具列为在线,模型手上却没有,唯一的诊断是一行主进程日志。这仍然远好于现在的整体停摆,所以开个 follow-up issue 就够;McpServerStatus 已经带了 errorstderrTail

P3 —— parseToolArguments 的注释声称了一个不存在的校验器。 没有可观察后果,但它让一条未校验的路径读起来像已校验。

P3 —— capabilityMetadataValid 和那几个常量,约 45 行,只买到日志措辞,对应测试断言的是 console.warn 的字符串。整帧校验一落地两者一起走。

P3 —— 正文还写着 16/16,head 上是 18。

无发现:上一轮那四点确实都真修好了;bindings 与 offers 的一致性;逐工具探针是不是第二个校验权威(它借用的是 Host 自己的解码器,这个选择是对的);以及它的成本(256 次探针实测不到 7 ms)。

那条 P1 与其说是你引入的缺陷,不如说是一个取舍:这个 PR 要不要把聚合这一半也扛下来,还是让 issue 留着。其余都很小。

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Putting the local ones inline with their grades. Reasoning is in my review above.

const resolved = groups
.map((group) => capabilityOffer(group, hostPathAccess))
.filter((entry): entry is ResolvedCapability => entry !== undefined);
const offers = Object.freeze(resolved.map((entry) => entry.offer));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1, and the one thing that decides whether Fixes #4591 is honest. Per-tool probing cannot see the decoder's aggregate checks, and this is where they would be visible. CLIENT_CAPABILITY_MAX_TOOLS_PER_OFFER is 64 (client-capability.ts:79), CLIENT_CAPABILITY_MAX_TOOLS is 256, and the manifest cap is 56 KiB, all enforced only when replace() decodes the whole frame. runtime-host-boot.ts:995-1003 puts every MCP tool from every server into the single desktop_mcp offer with no cap, so 65 tools whose schemas are all perfectly representable still fail the whole frame and take Browser, Computer Use, Client settings and Rive with them, leaving one console line. GitHub's own MCP server exposes around 70 tools by itself and discovery allows up to 1000, so this is a common configuration rather than an edge, and every tool passes its own probe so the new machinery reports nothing.

The defect predates this PR, so this is not a hole in your isolation. It does mean merging as written closes an issue that is still one MCP server away from reproducing.

The repo already has the end state: packages/cli/src/mcp-capability-provider.ts:38-99 sorts tools by (serverId, name), chunks into offers of at most CLIENT_CAPABILITY_MAX_TOOLS_PER_OFFER, and validates the whole assembled frame once before serving it. Desktop cannot reuse it verbatim, since it dispatches through tool.impl and the CLI through manager.callTool, but the shape transfers: assemble, decode the frame once here, chunk or shed on failure, serve the canonical result. That covers the aggregates and deletes capabilityMetadataValid on the way through.

If you would rather keep the boundary where it is, that is defensible, but then this should say Refs #4591 and the issue should stay open.

try {
decodeClientCapabilityReplaceInput({
registrationId: CAPABILITY_PROBE_REGISTRATION_ID,
offers: [

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P3 This, OfferMetadata and the three probe constants are about 45 lines that buy log wording only. All four production offerIds are literals in runtime-host-boot.ts, so a bad offerId is a Desktop coding error that cannot reach production, and the test covering it asserts on console.warn text. Drop it and let the per-tool probe carry the real group metadata, as it did in the first round: a bad group is still dropped whole, you just get N warnings instead of one. Under the whole-frame check above, both this and its test go anyway.

/**
* Coerce/validate incoming call arguments against a tool's declared parameters,
* mirroring the runtime's `validateDeclaredToolArgs` precedence. Zod schemas
* parse (applying defaults/transforms); JSON-schema-only tools (MCP) carry no

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P3 This comment says JSON-schema-only arguments "are validated by the receiving Runtime Host against the same schema". They are not: the Host rebuilds these with buildMcpTools (client-capability-coordinator.ts:600), which calls jsonSchema(descriptor.inputSchema) with no validate, and validateDeclaredToolArgs (tool-runtime.ts:3021-3065) returns silently when there is no safeParse, validate or ~standard.

No observable consequence, the MCP server validates its own input and this matches the TUI, but the comment makes an unvalidated path read as a validated one, which is the kind of claim a later reader builds on. Saying the MCP server is the validator would be true.

});

test('publishes every production Desktop-owned tool schema through the protocol', () => {
const settingsTools = buildClientSettingsTools({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2, and worth doing even if nothing else changes. This test's text did not change but its meaning did. It asserts only doesNotThrow on provider.offers(). Before this PR that proved every Client-settings and Rive tool was representable, because an unrepresentable one threw at construction. Now a dropped tool simply vanishes from offers() and the remainder still decodes, so it passes; it fails only if every offer is dropped. A future Desktop-owned tool with an unrepresentable Zod schema will disappear silently with green CI, which is the exact failure mode this PR exists to make visible.

Asserting the expected tool names per offer is about four lines, for example deepEqual(offer.tools.map(t => t.name), settingsTools.map(t => t.name)) for desktop_settings and ['rive_workflow'] for desktop_rive.

@github-actions github-actions Bot added effort/L Under 1000 readable lines and removed effort/M Under 500 readable lines labels Sep 3, 2026
…in test

Addresses the remaining review notes on apache#4592 while keeping the per-tool
isolation boundary (aggregate protocol limits stay tracked in apache#4652, so this
now reads Refs apache#4591 rather than Fixes).

P3: the separate offer-metadata probe (`capabilityMetadataValid`, the
`OfferMetadata` type and two probe constants, ~45 lines) only bought a
distinct log line for a Desktop coding error that cannot reach production —
every production offerId is a literal. Drop it and probe each tool inside its
group's real offer metadata, as the first round did: a group whose metadata is
invalid still drops all of its tools and is shed whole, just with N warnings
instead of one.

P2: `publishes every production Desktop-owned tool schema through the protocol`
asserted only `doesNotThrow`, which a silently dropped tool now passes. Assert
the offered tool names per offer so a future Desktop-owned tool with an
unrepresentable schema fails CI instead of vanishing green.

P3: `parseToolArguments` claimed MCP arguments are "validated by the receiving
Runtime Host against the same schema". They are not — the Host rebuilds these
with `buildMcpTools` (no client-side validator); the owning MCP server is the
validator, as for the TUI. Corrected the comment.

Generated-by: Claude Code
@liuxiaocs7

Copy link
Copy Markdown
Member Author

Pushed e4ab066 — took the P1 as a boundary decision and the rest as fixes.

P1 — Refs #4591, issue stays open. Kept the per-tool boundary rather than pulling the aggregate case into this PR. The body now reads Refs #4591 and states plainly that >64 tools/offer, 256 total and the 56 KiB manifest can still fail a large config's whole registration, with that half tracked in #4652. So it no longer claims to close the issue.

P2 — the schema test proves something again. publishes every production Desktop-owned tool schema through the protocol now asserts the offered tool names per offer (desktop_settings → the two client-settings tools, desktop_riveRiveWorkflow), so a future Desktop-owned tool with an unrepresentable schema fails the assertion instead of vanishing green. Kept the doesNotThrow alongside it.

P3 — dropped capabilityMetadataValid. Removed it, the OfferMetadata type and the two probe constants (~45 lines). Each tool is now probed inside its group's real offer metadata, as the first round did, so a group whose metadata is invalid still drops all its tools and is shed whole — N warnings instead of one, on a path that cannot reach production (every production offerId is a literal). Its test now asserts the bad-metadata group is shed while the rest register, instead of asserting console.warn text.

P3 — parseToolArguments comment corrected. It no longer claims the Host validates MCP arguments; it says the owning MCP server is the validator (the Host rebuilds these with buildMcpTools, no client-side validator), as for the TUI.

P3 — body test count now reads 18/18.

P2 — Settings visibility of a skipped tool is real: mcp-page.tsx renders the manager's status.tools, which doesn't know about the probe, so a skipped tool reads as live. That's a separate UI seam (McpServerStatus already carries error/stderrTail); it deserves a dedicated follow-up rather than widening this PR — happy to open one.

Local: 18/18 native-capability tests pass; build:main (tsc) and Biome lint on the two changed files are clean.

简体中文

已推送 e4ab066 —— P1 当作边界取舍,其余当作修复处理。

P1 —— 改为 Refs #4591,issue 保留。 保持逐工具的边界,没有把聚合那半塞进本 PR。正文已改为 Refs #4591,并明确写出 >64 工具/offer、总数 256、56 KiB manifest 仍会让大配置整帧失败,这半由 #4652 跟踪,所以不再声称关闭该 issue。

P2 —— 那条测试重新有了意义。 publishes every production Desktop-owned tool schema through the protocol 现在逐 offer 断言工具名(desktop_settings → 两个 client-settings 工具,desktop_riveRiveWorkflow),将来某个 Desktop 自有工具 schema 不可表达会让断言失败,而不是 CI 全绿地消失;doesNotThrow 也保留。

P3 —— 删掉了 capabilityMetadataValid 连同 OfferMetadata 类型和两个探针常量一起删(约 45 行)。每个工具现在在其分组的真实 offer 元数据里探针,跟第一轮一样;元数据非法的分组仍会把所有工具丢掉、整组不 advertise —— N 条 warning 而不是一条,而这条路径生产不可达(生产 offerId 都是字面量)。对应测试改为断言坏元数据分组被丢、其余照常注册,不再断言 console.warn 文本。

P3 —— parseToolArguments 注释已更正。 不再声称 Host 校验 MCP 参数;改为说由拥有该工具的 MCP server 校验(Host 用 buildMcpTools 重建,没有客户端校验器),和 TUI 一致。

P3 —— 正文测试数已改为 18/18。

P2 —— 被跳过工具在设置里的可见性确实存在:mcp-page.tsx 渲染 manager 的 status.tools,它不知道探针,所以被跳过的工具读起来是在线的。这是另一处 UI 接缝(McpServerStatus 已带 error/stderrTail),更适合单开 follow-up 而不是扩大本 PR —— 我可以开一个。

@liuxiaocs7

Copy link
Copy Markdown
Member Author

CI (test) failed on a single unrelated e2e flake — e2e/partial-history-notice.spec.tsquiet reading-column control with neutral rail ticks, a toBeVisible() timeout waiting for [data-turn-id="turn-partial-history-8"] (104/109 passed, 4 skipped). That spec is renderer-only; this commit (e4ab066a) changes only apps/desktop/src/main/runtime-host-native-capabilities.ts and its test — main-process native-capability offering, nothing on the partial-history path — and the parent commit 42b7433 was green. Re-triggering CI via reopen (no admin rights to re-run the job directly).

@liuxiaocs7 liuxiaocs7 closed this Sep 3, 2026
@liuxiaocs7 liuxiaocs7 reopened this Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

No description provided.

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

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants