fix(desktop): accept jsonSchema() tool params in native capability offers - #4592
fix(desktop): accept jsonSchema() tool params in native capability offers#4592liuxiaocs7 wants to merge 5 commits into
Conversation
…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
Astro-Han
left a comment
There was a problem hiding this comment.
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:117 把 parameters 设成 jsonSchema(...),它没有 Zod 原型,runtime-host-native-capabilities.ts:465 的 requireZodSchema 在 toolInputSchema 和 invokeNativeTool 两处都会抛。接收侧确实已经通过 buildMcpTools 接受 jsonSchema(),所以改提供侧是对的落点。新测试对 HEAD~1 确实会红,报的就是 #4591 里那句。本地 build、15 条 native-capability 测试、format、lint 都绿。
正文少说了两件事,其中一件我认为会改变这个 PR 该包含什么。
一个工具坏掉会把桌面端所有能力一起带走。 provider.offers() 是在 client-capability-channel.ts:97 的 replace() 里整帧调用的,所以抛出不只让 MCP 消失:Browser、Computer Use、Client settings、Rive 也一并注册不上。这和报告里的现象一致,什么都没有,只在 console 留一行。严重度大半在这里,建议在 issue 和正文里写明。
合并之后同样的失败还会发生。 client-capability.ts:719 是一张关键字白名单,:762 在遇到表外的第一个 key 时就拒掉整帧。prefixItems、not、patternProperties、contentEncoding、if/then/else、deprecated 都不在表里。我在这个分支上用真实的 buildMcpTools() 产物跑了 provider.offers() 和 decodeClientCapabilityReplaceInput:普通 schema 和 uniqueItems 能过,上面这六个报 Unsupported Client Capability tool schema keyword,连坐范围一样。这些都是很常见的产物,pydantic 的 tuple[...] 会出 prefixItems,bytes 会出 contentEncoding,而 packages/mcp/src/index.ts:2311 本身就特意保留了其中几个,说明仓库早就预期会遇到。
所以改动本身没错,但它宣称解决的事情没有成立,#4591 会被关掉,而相当一部分 server 仍以同样的方式坏着。我更希望在这里修隔离而不是白名单:在 capabilityOffer 里按工具 try/catch,遇到无法表达的工具就跳过并 warn,而不是让整帧失败。这个改动很小,能让本 PR 自己的结论对更多 server 成立,而且以后第三种无法表达的 schema 出现时,代价只是少一个工具,不是所有能力消失。放宽白名单可以单独做,那是另一件需要协议层判断的事。如果你更想把这个 PR 保持在 Zod 这一半,也可以,但请把 summary 收窄并开一个 follow-up,因为现在正文读起来就是「桌面端的 MCP 好了」。
行内还有三条小的,最大的一条是 parseToolArguments 的 validate 分支没有生产者,删掉能少三分之一体量。
… 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
|
Thanks for the careful review — reproducing the whole-frame blast radius and running real Isolation (main comment + inline on 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.
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
|
Follow-up review surfaced two more issues in the isolation I added; pushed 42b7433:
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 |
Astro-Han
left a comment
There was a problem hiding this comment.
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-server 和 my.server 两个 server 会撞名,并且带走整个注册。
收窄后的结论和代码一致。那八个关键字确实都不在 CLIENT_CAPABILITY_SCHEMA_KEYWORDS 里,:762 仍在遇到第一个时就拒整帧;#4614 的落点写对了,也点出了要动 epoch。
validate 分支删掉后确实没有生产者了。带 validate 的 jsonSchema() 调用还剩三处,但都是 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 留着。
可以更小
capabilityMetadataValid、OfferMetadata 和三个探针常量,约 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 已经有 error 和 stderrTail 这个用户可见的接缝。开个 issue 就够。
价值
这个改动该存在。问题是复现出来的而不是假想的,Desktop 上任何 MCP server 都触发,代价是全部原生能力消失。它修的是因,也就是协议两侧对 parameters 形状的分歧,落在唯一的分歧点上,而且接收侧本来就是更宽容的那一侧。
后半部分也不是范围蔓延。只有第一个 commit 的话,一个 pydantic 的 tuple[...] 参数就能原样复现 #4591:同样的整体停摆,只是换了个关键字。隔离是同一个 bug 的剩下一半。
形状比第一轮更好,理由就是上面那点:offer 侧现在借用 Host 的解码器,而不是长出第二份规则;ResolvedCapability 把两份状态并回一份。这两处都不是新增抽象。没有关掉它的理由。可以削减的是那 45 行,以及如果采纳整帧校验,两个探针合成一个。
|
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: P2 — P2 — a skipped tool is invisible to the user and contradicted by the UI. Normal path: P3 — P3 — 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 —— 聚合上限仍然会把桌面端所有能力一起带走。 正常支持路径,而且是常见配置而不是边角: P2 —— P2 —— 被跳过的工具对用户是隐形的,而且和界面自相矛盾。 正常路径: P3 —— P3 —— P3 —— 正文还写着 16/16,head 上是 18。 无发现:上一轮那四点确实都真修好了;bindings 与 offers 的一致性;逐工具探针是不是第二个校验权威(它借用的是 Host 自己的解码器,这个选择是对的);以及它的成本(256 次探针实测不到 7 ms)。 那条 P1 与其说是你引入的缺陷,不如说是一个取舍:这个 PR 要不要把聚合这一半也扛下来,还是让 issue 留着。其余都很小。 |
Astro-Han
left a comment
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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: [ |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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({ |
There was a problem hiding this comment.
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.
…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
|
Pushed e4ab066 — took the P1 as a boundary decision and the rest as fixes. P1 — P2 — the schema test proves something again. P3 — dropped P3 — P3 — body test count now reads 18/18. P2 — Settings visibility of a skipped tool is real: Local: 18/18 native-capability tests pass; 简体中文已推送 e4ab066 —— P1 当作边界取舍,其余当作修复处理。 P1 —— 改为 P2 —— 那条测试重新有了意义。 P3 —— 删掉了 P3 —— P3 —— 正文测试数已改为 18/18。 P2 —— 被跳过工具在设置里的可见性确实存在: |
|
CI ( |
|
No description provided. |
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 loggedMCP startup failed: ... Desktop native capability tool has an invalid schemaon 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'sparameterstojsonSchema(descriptor.inputSchema)— an AI SDKSchema, not a Zod type — but the Desktop native-capability provider offered and validated tools throughrequireZodSchema(tool.parameters instanceof z.ZodType), so it threw on the first MCP tool. The receiving side (client-capability-coordinator) already rebuilds the same tools withbuildMcpToolsand acceptsjsonSchema(); 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.tsreplace()), 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 pydantictuple[...]→prefixItems.capabilityOffernow builds each tool's descriptor independently and validates it through the samedecodeClientCapabilityReplaceInputthe 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:
prefixItemsand friends (needs its own protocol review). Until then those specific tools are skipped rather than offered.Details:
toolInputSchemareads the JSON Schema directly from an AI SDKSchemawhenparametersis not Zod, copying it before deleting$schema/ freezing so the (already upstream-frozen) MCPinputSchemais never mutated.parseToolArguments) mirrors the runtime'svalidateDeclaredToolArgs: ZodparseAsync, else pass-through for JSON-schema-only MCP tools (which carry no client-side validator), else throw with a call-path-specific message. The deadvalidatebranch (no producer) and itsNativeToolValidationtype 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.js— 18/18 pass, including:offers and dispatches MCP tools whose parameters are JSON Schema, not Zod— runs realbuildMcpTools()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 usesprefixItemsis skipped while a valid sibling tool and every other Desktop capability still register and the frame still encodes; fails without the isolation.biome linton the changed files — clean. (apps/desktop/**is formatter-excluded perbiome.jsonc; existing quote conventions matched.)Not run: the full Electron
npm run devend-to-end launch (headless environment). The tests exercise the same offer + call code paths, including protocol encoding viadecodeClientCapabilityReplaceInput.AI use
Select exactly one:
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 Codetrailer.Checklist
Does this PR entail a change in behavior?