Skip to content

fix(desktop): adapt MCP schemas for native capabilities - #4235

Open
sunheyi6 wants to merge 3 commits into
apache:mainfrom
sunheyi6:codex/fix-mcp-native-schema
Open

fix(desktop): adapt MCP schemas for native capabilities#4235
sunheyi6 wants to merge 3 commits into
apache:mainfrom
sunheyi6:codex/fix-mcp-native-schema

Conversation

@sunheyi6

@sunheyi6 sunheyi6 commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • preserve complete MCP JSON Schema descriptors instead of converting them into partial Zod schemas
  • validate Zod, AI SDK, Standard Schema, and draft-07/2019-09/2020-12 JSON Schema through one shared tool-parameter boundary
  • publish original MCP schemas through Desktop native capabilities and validate arguments before admission
  • align the bounded Client Capability protocol contract with supported modern JSON Schema applicator and validation keywords
  • add regressions for protocol canonicalization, conditional schemas, asynchronous schemas, dependentRequired, and unevaluatedProperties

Testing

  • npm run build:test
  • node --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 typecheck
  • npm run check:asf-headers
  • npm run lint

AI use

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

Tool(s) and scope: Codex — schema-boundary design, implementation, regression tests, and validation.

Fixes #4209.

中文

摘要

  • 保留完整的 MCP JSON Schema 描述,不再将其转换为能力不完整的 Zod schema
  • 通过统一的工具参数边界校验 Zod、AI SDK、Standard Schema,以及 draft-07/2019-09/2020-12 JSON Schema
  • 通过 Desktop native capability 发布原始 MCP schema,并在 admission 前校验参数
  • 让受限的 Client Capability 协议契约支持相应的现代 JSON Schema applicator 与 validation 关键字
  • 增加协议 canonicalization、条件 schema、异步 schema、dependentRequiredunevaluatedProperties 回归测试

验证

  • npm run build:test
  • node --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 typecheck
  • npm run check:asf-headers
  • npm run lint

AI 使用

  • 没有生成式工具作出实质性贡献
  • 生成式工具作出了实质性贡献

工具及范围:Codex — schema 边界设计、实现、回归测试和验证。

修复 #4209

@github-actions github-actions Bot added the effort/S Under 100 readable lines label Aug 30, 2026

@hqhq1025 hqhq1025 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.

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.

Comment thread packages/runtime/src/mcp-tools.ts Outdated

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@sunheyi6
sunheyi6 force-pushed the codex/fix-mcp-native-schema branch from ad6f652 to 2a5f0d9 Compare August 30, 2026 09:26

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 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.

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.

@sunheyi6
sunheyi6 force-pushed the codex/fix-mcp-native-schema branch from a7096a8 to 5c0fcb8 Compare August 30, 2026 13:20
@sunheyi6
sunheyi6 force-pushed the codex/fix-mcp-native-schema branch from 5c0fcb8 to dddebab Compare August 30, 2026 13:47
@sunheyi6
sunheyi6 force-pushed the codex/fix-mcp-native-schema branch from dddebab to c38d1b4 Compare August 30, 2026 14:01

@hqhq1025 hqhq1025 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.

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([

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] 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 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.

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);

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] (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;

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] (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.

@github-actions github-actions Bot added effort/L Under 1000 readable lines and removed effort/S Under 100 readable lines labels Aug 31, 2026
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

4 participants