fix(desktop): adapt MCP schemas for native capabilities - #4235
Conversation
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact commit 29bfec66f1d4f0932085f1a2d9fb79c26375506f and a conflict-free synthetic merge onto current main d2346707d65144682d45e905a378ee57be469769.
The change replaces the AI SDK JSON Schema wrapper with z.fromJSONSchema so Desktop native capabilities can publish and validate MCP tools, and adds a composition regression for a basic object schema. I found one P1 inline.
Validation: exact-head full npm run build:test passed; the 22 focused MCP/native-capability tests passed on both the exact head and synthetic merge; the synthetic-merge full build passed; git diff --check passed. Hosted test, label, and windows_recovery checks are green.
Codex-assisted review performed under the maintainer-approved review workflow.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
jackwener
left a comment
There was a problem hiding this comment.
Reviewed exact head ad6f652fd3c8fe3f19ba121712cb247bffac251a.
The previous schema-conversion issue is resolved. MCP tools now retain their complete JSON Schema, Desktop publishes that schema directly, and the shared parameter boundary validates it before admission with the appropriate JSON Schema draft. The conditional-schema production regression and the asynchronous dependentRequired/unevaluatedProperties test both pass.
[P2] Please update the PR metadata before approval. The English and Chinese summaries still say the implementation converts MCP schemas into Zod, but the current code intentionally avoids that conversion and preserves the original schema. The verification section also still reports the old 22-test set; the current focused set passes 23 tests. In addition, CONTRIBUTING.md requires every PR to state whether generative tooling contributed substantively. Please add that declaration; if the answer is yes, name the tool and scope and add a Generated-by: <tool> trailer to each affected commit.
The exact-head full build and focused tests passed (23/23). A clean synthetic merge onto current main (8c491e64b35cd7af66437efc8020a88b171e2bb6) preserved the patch, passed the full build, and passed the same 23 tests. Biome, git diff --check, the ASF header audit, and all exact-head hosted checks passed.
Automated review notice: This comment was posted by an automated review agent operated by WAWQAQ. It is not an independent human review and does not replace one.
ad6f652 to
2a5f0d9
Compare
jackwener
left a comment
There was a problem hiding this comment.
Reviewed exact head 2a5f0d99d3cfa8bcafb12789fe853e6977471fcd.
All previously raised issues are resolved. The implementation preserves complete MCP JSON Schemas, publishes the original schema through Desktop native capabilities, and validates Zod, AI SDK, Standard Schema, and draft-aware JSON Schema inputs at one shared admission boundary. The production regression covers conditional schemas, while the shared boundary test covers asynchronous validation, dependentRequired, and unevaluatedProperties.
The PR description now accurately explains schema preservation in both languages, reports the current 23-test set, and declares the substantive Codex contribution. Both substantive commits carry Generated-by: Codex trailers.
The exact-head full build and focused tests passed (23/23). A clean synthetic merge onto current main (9ff267510d381aba89f255b0d77bf6d277601de2) also passed the full build and the same 23 tests after installing the merge result's lockfile. The rebased patch ID matches the previously reviewed repair. Biome, git diff --check, the ASF header audit, and every exact-head hosted check passed. I found no remaining issues that should delay this fix.
Automated review notice: This comment was posted by an automated review agent operated by WAWQAQ. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for taking the earlier feedback seriously—the new shared parameter-validation direction is much cleaner than converting arbitrary MCP schemas into Zod.
I found one remaining production-boundary issue inline. The advanced schema is preserved by Desktop and accepted by the Runtime validator, but the Client Capability protocol rejects it before the provider can be installed. This is a normal MCP registration path and prevents the PR’s central behavior from working end to end, so I don’t think the current head is merge-ready yet.
Exact head reviewed: 2a5f0d99d3cfa8bcafb12789fe853e6977471fcd. Hosted checks are green.
中文对照
感谢认真处理之前的反馈。新的共享参数校验方向是对的,但高级 Schema 仍会在 Client Capability 协议层被拒绝,尚未真正贯通生产路径。因此当前不建议合并。
AI-assisted review: Codex traced the production composition and drafted this feedback; the maintainer verified the cited path and severity.
a7096a8 to
5c0fcb8
Compare
Generated-by: Codex
Generated-by: Codex
5c0fcb8 to
dddebab
Compare
Generated-by: Codex
dddebab to
c38d1b4
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact commit c38d1b4c5438c25a7d2a8839c26d05967a31ac0a. The new protocol coverage fixes the previously reported conditional-schema rejection, but I found one remaining P1 in the repository's supported SEP-2243 MCP path and left it inline.
Validation: clean npm ci; full npm run build:test; focused MCP/native-capability tests 28/28; Runtime 3094/3094 with 13 skips; MCP SEP-2243/modern-manager tests 19/19; full Runtime Host 1427 passed, 12 skipped, and one unrelated sandbox-context test failed identically on exact base 4cbe22413b29abf811de1f3fb3e8516ced7952ba; full typecheck, lint, format check, ASF header audit, git diff --check, and a conflict-free merge-tree against current main b83234888016d5f8ccac2a4f114f24ac22ee5fbf passed. All hosted checks are green.
I did not run a packaged Desktop session against a real remote SEP-2243 server; the deterministic production-path reproduction covers buildMcpTools through native-capability offer decoding.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| 'https://json-schema.org/draft/2019-09/schema', | ||
| 'https://json-schema.org/draft/2020-12/schema', | ||
| ]); | ||
| const CLIENT_CAPABILITY_SCHEMA_KEYWORDS = new Set([ |
There was a problem hiding this comment.
[P1] Admit the MCP header annotation supported by this repository
This allowlist still rejects x-mcp-header, even though packages/mcp/src/sep-2243.ts:120-184 validates that annotation and packages/mcp/src/index.ts:2297-2346 deliberately preserves it in the discovered tool schema. Desktop then forwards that schema unchanged, so decodeClientCapabilityReplaceInput() rejects the entire capability replacement before the tool can be installed. I reproduced this on the exact head with a valid { type: 'integer', 'x-mcp-header': 'Shard' } property through buildMcpTools -> provider.offers -> decodeClientCapabilityReplaceInput; it throws Unsupported Client Capability tool schema keyword. This recreates the original provider-registration failure for the product's supported SEP-2243 tools. Please either admit and type-check this annotation at the protocol boundary, or strip it only after the MCP layer has retained the header metadata, and add a production-path regression using a header-enabled tool.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for replacing the lossy JSON-Schema-to-Zod conversion with one shared validator; that closes the earlier advanced-schema issue cleanly. I found two remaining validator/production-normalization seams in addition to the existing x-mcp-header thread. These are suggestions from an outside review, so please do push back if MCP guarantees stronger schema identity or dialect behavior.
AI-assisted review disclosure: Codex ran independent protocol/schema and production/test analysis lanes; Astro-Han is the contributor of record for this review.
| const schemaForCompile = dialect.startsWith('https://json-schema.org/draft-07/schema') | ||
| ? { ...schema, $schema: dialect.replace('https://', 'http://') } | ||
| : schema; | ||
| const compiled = validator.compile(schemaForCompile as AnySchema); |
There was a problem hiding this comment.
[P1] (category ② — supported reconnect/recovery path)
Thanks for caching compiled schemas by object identity. The shared Ajv instance still registers each compiled schema’s $id globally. After a normal MCP reconnect/config refresh, the manager publishes a newly cloned descriptor object with the same $id, so the WeakMap misses and the second compile throws schema with key or id ... already exists; the tool then remains uncallable until process restart. The same happens on first use when two tools legitimately reuse one $id. Could independent tool schemas avoid the shared global ID registry (for example addUsedSchema: false, isolated validators, or canonical dialect+schema caching) and add a first call → reconnect/new clone → second call regression? Please feel free to push back if descriptors are guaranteed to preserve object identity across every reconnect.
| if (typeof schema !== 'object' || schema === null || Array.isArray(schema)) return undefined; | ||
| const cached = compiledSchemas.get(schema); | ||
| if (cached) return cached; | ||
| const declaredDialect = (schema as { readonly $schema?: unknown }).$schema; |
There was a problem hiding this comment.
[P1] (category ① — normal supported MCP schema path)
Thanks for selecting the validator from the declared dialect. The production MCP manager currently strips $schema recursively in normalizeToolInputSchema(), so an explicit draft-07 schema reaches this branch with no dialect and is compiled as 2020-12. A valid draft-07 tuple (items: [...], additionalItems: false) is then rejected by Ajv2020 even though the MCP server declared it correctly. The current native-capability tests hand-construct descriptors and bypass that normalization. Could the root dialect survive manager discovery into this validator (or be carried separately), with a real manager → buildMcpTools → native invocation draft-07 tuple regression? Please push back if the product intentionally canonicalizes every MCP schema to 2020-12 elsewhere.
Summary
dependentRequired, andunevaluatedPropertiesTesting
npm run build:testnode --test packages/runtime-host/dist/__tests__/client-capability-protocol.test.js packages/runtime/dist/__tests__/mcp-tools.test.js packages/runtime/dist/__tests__/tool-parameters.test.js apps/desktop/dist/main/__tests__/runtime-host-native-capabilities.test.js apps/desktop/dist/main/__tests__/mcp-runtime-e2e.test.js(28 passed)npm run typechecknpm run check:asf-headersnpm run lintAI use
Tool(s) and scope: Codex — schema-boundary design, implementation, regression tests, and validation.
Fixes #4209.
中文
摘要
dependentRequired和unevaluatedProperties回归测试验证
npm run build:testnode --test packages/runtime-host/dist/__tests__/client-capability-protocol.test.js packages/runtime/dist/__tests__/mcp-tools.test.js packages/runtime/dist/__tests__/tool-parameters.test.js apps/desktop/dist/main/__tests__/runtime-host-native-capabilities.test.js apps/desktop/dist/main/__tests__/mcp-runtime-e2e.test.js(28 项通过)npm run typechecknpm run check:asf-headersnpm run lintAI 使用
工具及范围:Codex — schema 边界设计、实现、回归测试和验证。
修复 #4209。