fix: support MCP json schema tools - #4553
Conversation
105bcd7 to
b5c5252
Compare
liugddx
left a comment
liugddx
left a comment
There was a problem hiding this comment.
Thanks for tackling #4334 — the two-sided framing (publish projection + invoke parsing) is right, and the Zod path is left untouched, which keeps the blast radius small. I had three independent passes read this against the source at b5c5252, and the same few things kept surfacing. Ranked:
1. jsonSchema tool arguments reach impl unvalidated. ai.jsonSchema(schema) with no validate option — which is exactly how the AI SDK wraps MCP proxy tool input — returns { jsonSchema, validate: undefined } with no parseAsync/safeParse/~standard. So every branch in parseNativeToolArguments misses and control reaches the trailing return args. An MCP tool declaring required: ['count'], count: {type:'number'} will now hand { count: "not-a-number", evil: {} } straight to its impl. On main, requireZodSchema enforced the schema at this boundary for every tool; that invariant is now silently dropped for the whole MCP class. The new test even pins the bypass — the fixture declares prefix: {pattern: '^[a-z]+$'} but asserts {prefix:'abc','x-test':...} passes through verbatim, so a value violating the pattern would pass identically. If delegating validation to the MCP server is intended, the code should say so and the test should assert it; otherwise this should validate.
2. @ai-sdk/provider-utils already owns the parse dispatch. The five-branch duck-typing in parseNativeToolArguments is dead code for the two inputs that actually ship: Zod only ever hits parseAsync; jsonSchema hits none. The package is already a dependency and exports safeValidateTypes({ value, schema }), whose FlexibleSchema covers Zod, Standard Schema, and jsonSchema wrappers uniformly. Routing through it collapses ~45 lines to a few and — because it compiles the JSON Schema — also closes (1) on the maintained path.
3. Two keyword allowlists, one truth. CAPABILITY_SCHEMA_KEYWORDS in the desktop layer is a byte-for-byte clone of CLIENT_CAPABILITY_SCHEMA_KEYWORDS in client-capability.ts — this PR had to add patternProperties to both in lockstep, which is the tell. When they drift, either the producer emits a keyword the protocol rejects (the #4334 crash class, reintroduced) or strips one the protocol would accept. The protocol set is the security boundary and the natural single source of truth; export it and import it here, deleting the copy.
4. Sanitizing in the producer is the wrong layer, and it's lossy. cleanJsonSchemaForCapability only enforces the keyword allowlist, but the protocol validator also enforces local-only $ref, dedup'd required, numeric-bound types, valid pattern, non-empty items/allOf. A real MCP schema with a non-local $ref still throws at decode after being sanitized, so the pass buys false confidence. Worse, cleanSchemaValue treats non-schema JSON values as schemas: default: { retries: 3, verbose: true } is key-pruned to default: {}, and enum: [{...}] to [{}]. Object-valued default/const/enum/examples are common and this rewrites them silently. Either have the protocol tolerate-and-ignore unknown keywords (one validator owns the policy, no producer sanitizer), or export a single shared sanitizer; and in any case pass const/default/enum/examples through untouched.
5. A type-less MCP schema takes down the whole provider. toolInputSchema throws unless the wrapper's top-level type === 'object', and offers is built eagerly in the constructor with no per-tool guard, so one MCP tool that omits top-level type (common, and valid) throws out of createDesktopNativeCapabilityProvider and drops browser, computer-use, settings, and every other group with it. Defaulting a missing top-level type to "object" and/or isolating per-tool failures would contain it.
On tests: the protocol patternProperties case is a clean regression. Two are weaker. The $id → throws assertion doesn't guard this change — $id was already rejected and the PR never touches it, and its fixture also carries patternProperties, so it throws in every world, before and after. And no test exercises any branch of parseNativeToolArguments other than the fall-through; a fixture with a rejecting validate asserting the call is refused and impl never runs (mirroring the existing Zod Invalid URL test) would pin the contract that's currently most at risk.
Net: the smallest correct version looks like — export the protocol allowlist (drop the copy), decide the $id-class policy in the one validator, and parse through safeValidateTypes — which replaces most of the added lines and surfaces, rather than hides, the validation question. Happy to be wrong on the delegation intent in (1); if so it just wants a line of documentation.
| signal.throwIfAborted(); | ||
| const parameters = requireZodSchema(binding.tool); | ||
| const args = await parameters.parseAsync(frame.arguments); | ||
| const args = await parseNativeToolArguments(binding.tool.parameters, frame.arguments); |
There was a problem hiding this comment.
This is the standing invariant that changes: on main this was requireZodSchema(...).parseAsync(...), which validated every tool's arguments at the trust boundary. For an ai.jsonSchema() wrapper with no validate option (the MCP proxy case), parseNativeToolArguments matches none of its branches and returns args untouched — so arguments reach binding.tool.impl unvalidated. Either compile the JSON Schema and validate (e.g. via safeValidateTypes from @ai-sdk/provider-utils, already a dependency), or make the delegation-to-MCP-server intent explicit in code + test.
| return result; | ||
| } | ||
|
|
||
| function cleanSchemaValue(value: unknown): unknown { |
There was a problem hiding this comment.
cleanSchemaValue treats non-schema JSON values as schemas. cleanSchemaKeywordValue routes const/default/enum members/examples here, and for any object this calls cleanJsonSchemaForCapability, which prunes every key not in the keyword allowlist. So default: { retries: 3, verbose: true } publishes as default: {}, and enum: [{status:'a'}] as [{}] — silently, since the protocol validator doesn't inspect those contents. These four keywords carry arbitrary JSON and should be deep-cloned through unchanged, not key-pruned.
There was a problem hiding this comment.
Thanks, that makes sense. I’ll align the PR with this direction and consolidate the schema handling layer.
liugddx
left a comment
liugddx
left a comment
There was a problem hiding this comment.
Review — PR #4553 "fix: support MCP json schema tools"
Reviewed at e62878323d540c5b01066f8b7c89d0ffc3bc2c26 · Verdict: REQUEST CHANGES (one red gate that is yours, two P2 correctness holes, one P1 test that certifies a bug). Fixes #4334. +244/−31 (prod ~93 / test ~151).
Nice, focused work — you found the real root cause. MCP proxy tools carry ai.jsonSchema() wrappers, not Zod, and the old requireZodSchema path threw on them at provider construction, so no MCP native tool could ever be offered. The two-part shape is right: teach the protocol vocabulary one new container keyword (patternProperties) and give the desktop layer a projection so raw MCP schemas survive the client-capability boundary. The whitelist projection is genuinely necessary — ai.jsonSchema() stores the schema verbatim (it does not normalize), and real MCP schemas routinely carry $schema/$id, which the protocol rejects; passing them through as-is would fail nearly every MCP tool. That instinct is correct. The problems are in where the logic lives, and in one claim the diff doesn't actually keep.
P1 — blocking
The red test check is yours: the protocol epoch guard, not a flake. packages/runtime-host/src/protocol/client-capability.ts is a protocol file, but RUNTIME_HOST_COMPATIBILITY_EPOCH (packages/runtime-host/src/protocol/index.ts:103) is unchanged and the base parent already advanced to 98. CI fails at scripts/protocol-epoch-check.mjs: "Protocol files changed but …EPOCH is still 98… every protocol change must land with an epoch the current base has not seen." This is the #3313 same-number-merge guard, and it will stay red until addressed.
Fix (one of two): rebase onto current main, then either (a) bump the epoch past 98 in index.ts:103, or — since adding patternProperties is an additive extension — (b) add a compatible-change declaration file under packages/runtime-host/protocol-compatible-changes/ that names packages/runtime-host/src/protocol/client-capability.ts. Option (b) is the intended path for a compatible keyword addition; pick it only if you're confident an older peer tolerates the new advertised keyword, else bump.
A test enshrines a validation bypass as if it were the feature. runtime-host-native-capabilities.test.ts (the accepts jsonSchema-wrapped MCP proxy tool descriptors case, ~L199–210) declares prefix with enum: ['ready','done'] and pattern: '^[a-z]+$', then invokes with prefix: 'abc' and asserts via deepEqual that the args arrive at impl unchanged — i.e. it asserts that an enum-violating value is accepted. That "pass-through" is an identity no-op, not proof of validation, and it locks in the P2 below. Split it: keep the reachability half, and replace the acceptance half with a test that sends invalid input and asserts rejection (that test fails against current prod — which is the point).
P2 — should fix before merge
The invocation path no longer validates MCP arguments — and the PR body says it does. parseNativeToolArguments (runtime-host-native-capabilities.ts:530-535) replaces requireZodSchema(...).parseAsync(args) with validateTypes({ value, schema: binding.tool.parameters }). Verified against the pinned @ai-sdk/provider-utils@5.0.28: validateTypes → safeValidateTypes → asSchema returns the jsonSchema() wrapper as-is, then if (actualSchema.validate == null) return { success: true, value }. An ai.jsonSchema(raw) wrapper built with no validate option has validate === undefined, so for exactly the MCP tools this PR adds, arguments flow to impl with no enum / pattern / type / required enforcement. Repro: schema {type:'object',properties:{mode:{enum:['read']}},required:['mode']}, frame args {mode:'DELETE_EVERYTHING',extra:1} → passed through verbatim. The body's "parses the arguments through the tool's declared schema shape" is an overclaim for the jsonSchema case. Severity is P2 not P1 because desktop's high-risk tools (browser, computer-use) use Zod and are still validated, the MCP server re-validates downstream, and the codec already imposes structural caps — but the advertised contract and the enforced one diverge silently.
Fix: attach a real validator — jsonSchema(wrapper.jsonSchema, { validate: <compiled ajv/std-schema validator> }) — or validate args against the projected schema before calling impl; then un-skip the P1 rejection test.
One malformed MCP schema poisons the whole registration. Offers are built eagerly at provider creation, but the jsonSchema branch (toolInputSchema, ~L473) returns the projected schema without validating its structural soundness; validateToolInputSchema only runs later when client.capability.replace is decoded (client-capability.ts:680). Projection can emit boundary-invalid schemas: items: [] maps to [] (protocol throws "Invalid … items" on empty array, client-capability.ts:~839), and non-array allOf/anyOf/oneOf map to [] (protocol throws on empty composite, ~849). Repro input: {type:'object',properties:{x:{type:'array',items:[]}}}. One such tool → decodeClientCapabilityReplaceInput throws → every desktop native offer fails to publish, with a generic error and no offending-tool name. The Zod path fails per-tool with the tool name; the new path regresses that.
Fix: run validateToolInputSchema (or a normalizing pass that drops empty items/composites) per tool inside toolInputSchema, so a bad MCP tool is skipped and named locally.
Second authority: the projection hand-mirrors the protocol's own traversal, across a package boundary. client-capability.ts:validateToolInputSchema.visit() already owns, authoritatively, which keywords carry nested schemas and in what shape — record (properties/patternProperties/$defs/definitions), array (allOf/anyOf/oneOf), single-or-array (items), single (additionalProperties/propertyNames). The desktop projectClientCapabilitySchemaKeyword switch (runtime-host-native-capabilities.ts:495-522) re-encodes that same table. The keyword set is now shared by export (good) — but the recursion structure is a second copy in another package. This PR is itself the evidence: supporting one keyword required edits in three coupled sites (index/set :746, protocol visit :814, desktop switch :~497). The drift failure mode is concrete and bad: add a future nested keyword to the shared set + protocol visit but forget the desktop switch → projectClientCapabilitySchema keeps the keyword (it's in the set) and default: return value passes its subtree verbatim, including disallowed nested keywords → the protocol boundary then rejects it → whole-batch publish failure (same blast radius as above).
Fix: export a single projectToolInputSchema(schema) from @maka/runtime-host beside CLIENT_CAPABILITY_SCHEMA_KEYWORDS, ideally driven by the same keyword→shape table as visit(); desktop imports it and only decides which schema object to hand in. This also moves protocol-vocabulary knowledge out of the Electron app, where it doesn't belong.
P3 — non-blocking
- Root-type asymmetry. The Zod branch enforces
schema.type === 'object'with a clear per-tool error (:461); the jsonSchema branch has no such check, so a non-object MCP root ({type:'string'}) is offered then rejected generically at the boundary. Add the same guard before projecting. - Non-causal tests. The protocol
annotated_values(default/enum/examples) andannotated_schema($idthrows) additions pass onmainwithout this PR —$idwas never a whitelisted keyword, unchanged here. They document boundaries but guard nothing this PR changes. Deleteannotated_values; either deleteannotated_schemaor reframe it as a real two-layer test (feed a raw schema through desktop projection and assert the projected output still trips the boundary via a non-stripped invalidity). - Dead-code guard (not a bug).
parseNativeToolArguments's falsy/primitive early-return is unreachable for offered tools (toolInputSchemathrows first at construction). Harmless; a one-line comment would explain it. - Refuted, so nobody re-runs them: no ReDoS surface (protocol compiles
patternonce in try/catch, and since the jsonSchema path never executes validation, patterns are never matched at runtime); projection is sound on valid input (it doesn't corrupt valid schemas — it only fails to sanitize already-invalid ones);ai.jsonSchema()does not pre-normalize, so the whitelist is not redundant.
The four questions
- Optimal? No. It makes MCP tools offerable (real fix, right root cause), but the invocation half silently stops validating arguments (P2 #1) and the eager-offer half can take down the whole registration on one odd schema (P2 #2). Functionally "works" in the happy path; not correct at the edges the body claims to cover.
- First principles / right layer? The cause is correctly identified (jsonSchema wrappers ≠ Zod). But the projection — pure protocol-vocabulary knowledge — lives in the desktop app and duplicates
validateToolInputSchema's traversal. It belongs in@maka/runtime-host. - Occam. Collapse the three
projectClientCapabilitySchema*functions + the protocolvisit()shape table into one exportedprojectToolInputSchemadriven by a single keyword→shape table; delete the desktop copy. One authority, one place to add the next keyword. - Low-quality tests. Fix the invocation test that certifies the bypass (P1); delete the two non-causal protocol assertions; add (a) a real reject-invalid-input test — fails today, pins P2 #1; (b) a projection→boundary rejection test — pins P2 #2; (c) an unsupported-schema-type throw test; (d) explicit early-return coverage.
Smallest correct version: export projectToolInputSchema() (with the root-object check and empty-items/composite normalization) from @maka/runtime-host, driven by the same shape table as validateToolInputSchema; call it from desktop's jsonSchema branch. Make parseNativeToolArguments actually validate (compile the JSON schema into a validate fn). Bump the protocol epoch (or add the compatible-change declaration). Replace the pass-through invocation assertion with a reject-invalid-input test and drop the two non-causal ones.
| 'multipleOf', | ||
| 'oneOf', | ||
| 'pattern', | ||
| 'patternProperties', |
There was a problem hiding this comment.
P1 (CI gate): this file is a protocol file, so the epoch guard (scripts/protocol-epoch-check.mjs, #3313) fails the test check while RUNTIME_HOST_COMPATIBILITY_EPOCH (protocol/index.ts:103) stays at the base's 98. Rebase onto current main and either bump the epoch past 98, or add a compatible-change declaration under packages/runtime-host/protocol-compatible-changes/ naming this file. Also: this is the authoritative keyword table + traversal — the desktop projection re-encodes the same structure, so keep them from drifting (see the note on the desktop switch).
| signal.throwIfAborted(); | ||
| const parameters = requireZodSchema(binding.tool); | ||
| const args = await parameters.parseAsync(frame.arguments); | ||
| const args = await parseNativeToolArguments(binding.tool.parameters, frame.arguments); |
There was a problem hiding this comment.
P2 (validation bypass): parseNativeToolArguments -> validateTypes({value, schema: tool.parameters}). Verified against @ai-sdk/provider-utils@5.0.28: for an ai.jsonSchema(raw) wrapper built with no validate option, validate is undefined and safeValidateTypes returns the value unchanged — so MCP tool args reach impl() with NO enum/pattern/type/required enforcement. Repro: schema {type:object,properties:{mode:{enum:[read]}},required:[mode]} + args {mode:"DELETE_EVERYTHING",extra:1} pass through verbatim. The PR body's "parses through the declared schema shape" overclaims for exactly the jsonSchema tools this PR adds. Fix: compile a real validator (jsonSchema(raw,{validate})) or validate args against the projected schema before impl.
| return result; | ||
| } | ||
|
|
||
| function projectClientCapabilitySchemaKeyword(key: string, value: unknown): unknown { |
There was a problem hiding this comment.
P2 (second authority / duplication): this switch re-encodes the keyword->shape traversal that client-capability.ts:validateToolInputSchema.visit() already owns (record: properties/patternProperties/$defs/definitions; array: allOf/anyOf/oneOf; single-or-array: items; single: additionalProperties/propertyNames). This PR needed edits in three coupled sites for one keyword. Drift risk: add a future nested keyword to the shared set + protocol visit but forget this switch -> default: return value passes the subtree verbatim (incl. disallowed keywords) -> the protocol boundary rejects it -> the WHOLE registration fails to publish with an opaque error. Fix: export a single projectToolInputSchema() from @maka/runtime-host driven by the same shape table; desktop just picks which schema to hand in. (Also emits protocol-invalid schemas today: items:[] / empty composites -> boundary rejects the whole batch.)
e1f4bd8 to
d32632c
Compare
Move schema projection to the protocol layer as `projectToolInputSchema`, driven by a shared per-keyword shape table that both projection and `validateToolInputSchema` use for recursion. Desktop imports the single authority instead of maintaining a duplicate. Add Ajv-based argument validation for jsonSchema-wrapped MCP tools so that enum/pattern/required constraints are enforced at call time. Also: - Drop empty `items` / `allOf` / `anyOf` / `oneOf` during projection so one malformed MCP schema cannot poison the entire registration. - Reject non-object root schemas with a per-tool error (addresses the root-type asymmetry with Zod path). - Remove non-causal protocol tests; add projection and validation coverage to desktop tests.
5d902ab to
1a7ab81
Compare
2 participants
Summary
Fixes #4334
Maka Desktop now accepts MCP proxy tools that expose
ai.jsonSchema()wrappers instead of requiring every tool schema to be a Zod instance.This change covers both parts of the failure:
The result is that MCP proxy tools can publish successfully and still be invoked normally at runtime.
Verification
Ran locally:
npm --workspace @maka/desktop run build:testnpm --workspace @maka/runtime-host run buildnode --test apps/desktop/dist/main/__tests__/runtime-host-native-capabilities.test.jsnode --test packages/runtime-host/dist/__tests__/client-capability-protocol.test.jsAI use
Tool(s) and scope:
Checklist
Does this PR entail a change in behavior?