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.)
5d902ab to
1a7ab81
Compare
liugddx
left a comment
liugddx
left a comment
There was a problem hiding this comment.
Re-review — PR #4553 "fix: support MCP json schema tools" (round 2)
Reviewed at 1a7ab81739ca645027b58a76c1dbf9097f178565 (was e628783). Verdict: the blocking issues are resolved; moving toward APPROVE pending green CI + one perf fix. Author addressed the round-1 review directly ("unify MCP schema projection and add argument validation").
Round-1 findings — all resolved
- P1 protocol epoch (red CI): rebased onto main,
RUNTIME_HOST_COMPATIBILITY_EPOCHbumped 94 → 102 (past the base's 98). Thetestgate is re-running; the epoch guard is satisfied. - P1 test enshrined a bypass: fixed cleanly. The "accepts …" test now declares
prefix: { type:'string', pattern:'^[a-z]+$' }and invokesprefix:'abc', which legitimately matches — so it exercises accept-valid. A new test "validates jsonSchema-wrapped tool arguments and rejects invalid input" invokesprefix:'abc'againstenum:['ready','done']and assertsassert.rejects(…, /Invalid arguments/)withcalls.length === 0. That's the reject-invalid test I asked for, and it fails against the old no-op path. - P2 validation bypass:
parseNativeToolArgumentsnow compiles the projected schema with Ajv and throwsInvalid arguments: …on failure. Real enforcement, not a cast. - P2 whole-batch poisoning:
projectSchemaKeywordreturnsundefinedfor emptyitems/allOf/anyOf/oneOf, andprojectSchemaNodeskips undefined — the projected schema can no longer emit a shape the protocol boundary rejects. The doc comment says exactly this. - P2 second authority / duplication: resolved better than proposed. A single
CLIENT_CAPABILITY_SCHEMA_CONTAINER_SHAPEStable (client-capability.ts:755) now drives bothprojectToolInputSchema(viaprojectSchemaKeyword) andvalidateToolInputSchema.visit(~L901,for (const [key, shape] of Object.entries(CONTAINER_SHAPES))). The traversal exists once, in@maka/runtime-host; desktop importsprojectToolInputSchema. This is the single-authority shape I hoped for. - P3 root-type check:
projectToolInputSchemathrowsroot must be an object, now shared by both the offer and validation paths.
New findings on the reworked Ajv layer
P2 (efficiency) — the compile cache never hits, so Ajv recompiles on every invocation. compiledSchemas (a WeakMap) is keyed on the object returned by projectToolInputSchema(wrapper.jsonSchema) — but that call produces a fresh object every invocation (parseNativeToolArguments → const projected = projectToolInputSchema(...) → compileJsonSchema(projected)). The WeakMap lookup compiledSchemas.get(projected) is therefore always a miss, and validator.compile(schema) (the expensive step) runs on every native tool call. Fix: key the cache on the stable wrapper.jsonSchema (or the binding), or project + compile once when the offer is built and reuse the ValidateFunction.
P3 (Occam + edge) — the draft dialect dispatch is dead code. $schema is not in CLIENT_CAPABILITY_SCHEMA_KEYWORDS, so projectToolInputSchema strips it. compileJsonSchema then reads projected.$schema → undefined → dialect '' → always draft2020Validator. The draft7Validator and draft2019Validator instances and the .includes('draft-07')/'2019-09' ladder are unreachable for the projected-schema path. Fix: either delete the two unused validators and the dispatch, or compile the raw wrapper.jsonSchema (which still carries $schema) so the dispatch is meaningful.
Sub-edge (PLAUSIBLE, worth a test): a draft-07 schema with a tuple items: [A, B] survives projection (single_or_array keeps the array), but under Ajv 2020-12 items must be a single schema — tuple validation moved to prefixItems. Compiling such a projected schema under draft2020Validator will mis-apply items (or throw at compile, which surfaces as an invocation-time failure). Add a tuple-items case to pin the behavior.
P3 (note, not a defect) — validation is against the lossy projected schema. Because parseNativeToolArguments validates projectToolInputSchema(raw) rather than raw, any constraint expressed via non-whitelisted keywords (not, if/then/else, contains, dependentRequired, dependentSchemas, unevaluatedProperties, prefixItems) is dropped before Ajv sees it and thus not enforced locally. This is defensible — you validate against the contract you actually advertised to the model, and the downstream MCP server re-validates — but a one-line comment would prevent a future reader from assuming full-schema enforcement.
The four questions (unchanged rubric)
- Optimal now? Much closer. Root cause fixed, validation is real, projection is single-authority. The remaining gap is the dead compile cache (a perf regression) and dead dialect code.
- First principles / layer. Correct now — projection lives in the protocol package beside the keyword set and the validator, driven by one shared shape table.
- Occam. Two things left to collapse: the never-hit
compiledSchemascache (fix the key) and the unreachable draft7/2019 validators + dispatch (delete, or feed them the raw schema). - Low-quality tests. The bypass-enshrining assertion is gone and a genuine reject-invalid test replaced it. Add: a tuple-
itemsprojection/validation test; optionally a test asserting the same wrapper reuses one compiled validator (guards the cache fix).
Smallest remaining delta: key the Ajv cache on the stable wrapper (or compile at offer time), and either delete the draft7/2019 validators or validate the raw schema so the dispatch is real. With CI green, that's an APPROVE.
| const projected = projectToolInputSchema( | ||
| wrapper.jsonSchema as Record<string, unknown>, | ||
| ); | ||
| const validator = compileJsonSchema(projected); |
There was a problem hiding this comment.
P2 (efficiency): the Ajv compile cache never hits. projectToolInputSchema(wrapper.jsonSchema) returns a FRESH object every invocation, and compiledSchemas (WeakMap) is keyed on that ephemeral projected object — so compiledSchemas.get(projected) always misses and validator.compile() (the expensive step) runs on every native tool call. Fix: key the cache on the stable wrapper.jsonSchema (or the binding), or project+compile once when the offer is built and reuse the ValidateFunction.
| ).$schema; | ||
| const dialect = | ||
| typeof declaredDialect === 'string' ? declaredDialect : ''; | ||
| const validator = dialect.includes('draft-07') |
There was a problem hiding this comment.
P3 (Occam + edge): this dialect dispatch is dead code. $schema is not in CLIENT_CAPABILITY_SCHEMA_KEYWORDS, so projectToolInputSchema strips it; compileJsonSchema then always sees no $schema -> dialect '' -> always draft2020Validator. draft7Validator/draft2019Validator and the draft-07/2019-09 branches are unreachable for the projected path. Either delete the two unused validators + dispatch, or compile the RAW schema (which still carries $schema) so the dispatch is meaningful. Sub-edge worth a test: a draft-07 tuple items:[A,B] survives projection but Ajv2020 treats items as a single schema (tuple moved to prefixItems) -> mis-validated or a compile throw.
36bb081 to
daf37d1
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head d46ca239e33e5cbd3db65ca72044b0d80a65b0e9 against current main 148f8eb297c86aa3045c75e87e19cacd4967c2dc.
The diff replaces the Zod-only Desktop capability path with shared JSON-Schema projection, adds patternProperties, validates MCP arguments with cached Ajv 2020 validators, and advances the Runtime Host compatibility epoch to 105. The happy path and the earlier validation/cache issues are fixed, but this head still has one P1 and two P2 correctness/availability findings in the inline comments below.
Validation performed:
- Clean
npm ci,npm run build:test, and full workspacenpm run typecheckpassed on the exact head. - Changed-file Biome,
git diff --check, and protocol epoch guard (104 -> 105) passed. - Focused Desktop/Runtime Host capability suites passed 28/28 on the exact head and on a conflict-free synthetic merge onto current main; synthetic tree:
db3942437fb458eff17333dcb26a84d3c52cac90. - Full Runtime Host: 1652 passed, 1 failed, 12 skipped. The sole failure is in unchanged managed-sandbox coverage and reproduced on the synthetic merge; this runner rejects both
unshareandbwrapwith permission denied. - Full Desktop: 2027 passed, 0 failed, 8 cancelled in the unchanged MCP OAuth deadline group.
- All hosted checks are terminal green at publication.
Not independently exercised: native Windows/macOS packaging/runtime behavior or a third-party MCP server outside deterministic local provider probes.
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.
| strict: false, | ||
| validateFormats: false, | ||
| } as const; | ||
| const schemaValidator = new Ajv2020(jsonSchemaValidatorOptions); |
There was a problem hiding this comment.
P1 - untrusted MCP regexes can synchronously block the Electron main process. This Ajv instance compiles and executes the projected pattern/patternProperties expressions on the call path before admission, with no timeout or isolation. The schema comes from the connected MCP server, while Client Capability arguments may be up to 40 KiB. On this exact head, the valid pattern ^(a+)+$ took about 1.8 seconds to reject only 29 input characters; the cost grows exponentially, and the synchronous validator cannot observe the abort signal while it is running. A remote/untrusted MCP descriptor can therefore freeze all Desktop main-process work when its tool is invoked. Use a linear-time regex engine or worker/deadline isolation, or omit regex constraints from local validation and let the MCP endpoint enforce them.
| case 'single_or_array': { | ||
| if (Array.isArray(value)) { | ||
| if (value.length === 0) return undefined; | ||
| return value.map((entry) => projectSchemaNode(entry)); |
There was a problem hiding this comment.
P2 - the protocol advertises schemas that the invocation validator cannot compile. items arrays are deliberately retained here and the new protocol test accepts items: [{type:"integer"}, {type:"integer"}], but Desktop always compiles the projected schema with Ajv 2020. The MCP layer and this projection both remove $schema, so an otherwise valid draft-07 tuple loses its dialect. Exact-head probe: publication passed decodeClientCapabilityReplaceInput, then a valid {coordinate:[1,2]} call failed before impl with items must be object,boolean. The same mismatch exists for regexes accepted by new RegExp(pattern) but rejected by Ajv unicode mode, e.g. \\8. Make the protocol vocabulary match the selected validator, or preserve/translate the source dialect before advertising the schema.
| const schema = wrapper.jsonSchema; | ||
| if (typeof schema === "object" && schema !== null) { | ||
| return Object.freeze( | ||
| projectToolInputSchema(schema as Record<string, unknown>), |
There was a problem hiding this comment.
P2 - one malformed MCP schema still unregisters every Desktop capability. This path projects each schema but does not contain a per-tool failure. The new invalid-patternProperties test proves the provider can be constructed and only fails when the complete offer set is canonicalized. Production combines Browser, Computer Use, settings, Rive, and every MCP tool in one provider; snapshotProvider() decodes that whole set, and refresh() catches any schema error by calling #clearRegistration(). Exact-head probe: after publishing desktop_browser, desktop_settings, and desktop_mcp, refreshing with one MCP key "(" produced CapabilityProviderPublicationError and one unregisterClientCapabilities call. Validate and omit/report only the bad MCP descriptor, or isolate MCP publication so unrelated local capabilities remain registered.
a831f0b to
b90aeb5
Compare
The three findings above are addressed in b90aeb5: regex constraints are stripped from the local validator (ReDoS), tuple items are translated to prefixItems for Ajv 2020, and malformed MCP tools are skipped per-tool so the rest of the registration survives. Focused suites pass 23/23 (desktop) and 5/5 (runtime-host). Re-reviewed head: b90aeb5. |
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.
Address review follow-ups on the MCP jsonSchema tool support: - Reject invalid `patternProperties` regex keys at the protocol boundary (`validateToolInputSchema`), mirroring the existing `pattern` check, so a malformed key from an untrusted MCP server is refused at decode instead of crashing `Ajv.compile` with a raw SyntaxError on every tool invocation. - Guard `schemaValidator.compile` with try/catch and surface a clean error. - Drop the undeclared `@ai-sdk/provider-utils` production import; validate Zod schemas with their native `parseAsync` (simpler, no hoisting dependency). - Fold projection into `compileJsonSchema` so it runs only on a cache miss (was recomputed on every call); remove the now-unreachable guard and the dead `!validator` branch. - Remove the dead Zod `.issues` branch in `schemaErrorSummary` (only Ajv error arrays reach it now). - Rename the misnamed "one bad MCP schema is named…" test to describe what it actually checks, and add negative coverage for the patternProperties regex rejection and empty allOf/anyOf/oneOf projection drop. Verified: `@maka/runtime-host` build + protocol suite (5/5) and `@maka/desktop` build:test + native-capabilities suite (21/21) pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
b90aeb5 to
c4639ce
Compare
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.
| @@ -743,6 +743,7 @@ const CLIENT_CAPABILITY_SCHEMA_KEYWORDS = new Set([ | |||
| '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).
| @@ -337,8 +339,7 @@ async function invokeNativeTool( | |||
| } | |||
| const signal = AbortSignal.any([options.signal, invocation.signal]); | |||
| 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.)
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.
| @@ -743,6 +743,7 @@ const CLIENT_CAPABILITY_SCHEMA_KEYWORDS = new Set([ | |||
| '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).
| @@ -337,8 +339,7 @@ async function invokeNativeTool( | |||
| } | |||
| const signal = AbortSignal.any([options.signal, invocation.signal]); | |||
| 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.)
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.
| @@ -743,6 +743,7 @@ const CLIENT_CAPABILITY_SCHEMA_KEYWORDS = new Set([ | |||
| '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).
| @@ -337,8 +339,7 @@ async function invokeNativeTool( | |||
| } | |||
| const signal = AbortSignal.any([options.signal, invocation.signal]); | |||
| 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.)
liugddx
left a comment
liugddx
left a comment
There was a problem hiding this comment.
Reviewed at a5f98085.
Nice progress since the last round — the epoch bump (105→106) correctly gates the new patternProperties vocabulary, per-tool isolation means one malformed MCP descriptor no longer fails the whole client.capability.replace, and folding projection + validation onto the single CLIENT_CAPABILITY_SCHEMA_CONTAINER_SHAPES table removes the previous "two authorities" smell in the protocol layer.
There are two blocking correctness issues, both from the same root cause: the local validator is built from a lossily adapted copy of the projected schema, so it ends up stricter than the schema you advertise to the model and rejects legitimate calls before they ever reach the MCP server.
P2 (blocking) — patternProperties + additionalProperties: false rejects valid input
adaptSchemaForLocalValidation strips patternProperties (runtime-host-native-capabilities.ts:551) but passes additionalProperties through unchanged (:586). For a raw schema { type:'object', patternProperties:{ '^x-':{type:'string'} }, additionalProperties:false }, the advertised schema accepts {"x-foo":"bar"}, but the local Ajv validator — now missing patternProperties while keeping additionalProperties:false — rejects it with must NOT have additional properties, so parseNativeToolArguments throws before the call reaches MCP. Fail-closed, but a real functional break for any MCP tool that uses patternProperties.
P3 (blocking) — tuple items → items:false rejects extra elements
The draft-07 → 2020 rewrite emits prefixItems:[…], items:false (:557-560). A draft-07 array with tuple items and no additionalItems permits extra trailing items of any type; items:false forbids them. For items:[{integer},{integer}], the advertised schema accepts [1,2,3] while the local validator rejects it. Use items: true (or omit items) so the local check matches what you advertise.
The bigger question (first principles / Occam)
Is local Ajv validation on the jsonSchema path worth its weight at all? The arguments are forwarded to the MCP server, which re-validates against the full schema — it is the authority. The local validator only ever sees the projected schema (no pattern, no if/then/else, no contains…), so it is strictly weaker, does no coercion (returns args untouched), and — as P2/P3 show — currently rejects inputs the authority would accept. Its only unique value is failing fast before the consent prompt / round-trip.
If the team lets MCP be the sole authority for jsonSchema tools, deleting the local validator (the ajv dependency, the Ajv2020 instance, compileJsonSchema, adaptSchemaForLocalValidation, schemaErrorSummary, and the WeakMap cache — ~165 lines) makes both P2 and P3 disappear by construction, and removes adaptSchemaForLocalValidation, which is a third hand-maintained copy of the keyword-shape knowledge that CONTAINER_SHAPES already owns. The Zod native tools (browser / computer-use / settings) keep parseAsync validation + coercion untouched. That is the smallest correct version of this change.
If you'd rather keep fail-fast: fix P2/P3 as above and drive adaptSchemaForLocalValidation off the exported CONTAINER_SHAPES table instead of re-enumerating properties/$defs/definitions/allOf/anyOf/oneOf/additionalProperties/propertyNames/items by hand.
Smaller cleanups (independent)
- The
aidev-dependency (apps/desktop/package.json) is imported only by the test, forjsonSchema(). Production keys solely onwrapper.jsonSchemaandMakaTool.parametersisunknown, so the test can build the wrapper as a plain literal{ jsonSchema: {…} }and the whole dependency can go. - In
adaptSchemaForLocalValidation, the empty-itemsbranch and the non-arrayallOf/anyOf/oneOffallback are unreachable:projectToolInputSchemaalready drops emptyitems/allOf/anyOf/oneOfbefore adapt runs.
Test quality
an invalid patternProperties regex key is isolated at the provider boundaryduplicates thebad_toolcase already covered byskips a malformed MCP tool…plus the protocol-layer reject test — safe to delete.skips non-object root…andskips unsupported schema type…assert the same "bad skipped / good survives" outcome asskips a malformed MCP tool…; fold the three into one parameterized test.- The rewritten candidate test (
does not drop the Host connection when a native tool schema is invalid) is weaker than it reads: the fixture has a single tool, so nothing proves isolation — it only checkscloseCallstransitions and would stay green even if the invalid tool were silently published. Add a healthy sibling tool and assert it survived (andipc.size > 0). - Missing coverage today: the P2 and P3 divergences. If you delete local validation they become moot; otherwise add one test each.
The four questions, briefly
- Optimal? Direction is right, but not yet — two over-reject bugs plus a third keyword-shape authority.
- First principles / layering? Projection + protocol validation belong in the protocol layer and are correct; the thing to reconsider is the necessity of a second, weaker local validator.
- Occam? Delete the local validator (biggest cut, kills both bugs), or at minimum table-drive
adapt; drop theaidep and the dead branches. - Low-quality tests? Delete the redundant patternProperties-isolation test, merge the three skip tests, strengthen the candidate test.
Smallest correct version: keep projection + per-tool isolation; drop local Ajv on the jsonSchema path and forward to MCP as the sole authority; protocol layer unchanged; trim the duplicated tests.
| const schema = value as Record<string, unknown>; | ||
| const result: Record<string, unknown> = {}; | ||
| for (const [key, val] of Object.entries(schema)) { | ||
| if (key === 'pattern' || key === 'patternProperties') continue; |
There was a problem hiding this comment.
P2 (blocking). Stripping patternProperties here while additionalProperties is passed through unchanged at line 586 makes the local validator reject valid input. For {type:'object', patternProperties:{'^x-':{type:'string'}}, additionalProperties:false}, the advertised schema accepts {"x-foo":"bar"} but local Ajv rejects it (must NOT have additional properties), so the call throws before reaching MCP. If you keep local validation, also relax additionalProperties to true whenever you drop patternProperties. If you delete local validation (recommended), this whole path goes away.
| result.prefixItems = (val as unknown[]).map((entry) => | ||
| adaptSchemaForLocalValidation(entry), | ||
| ); | ||
| result.items = false; |
There was a problem hiding this comment.
P3 (blocking). draft-07 tuple items:[…] with no additionalItems permits extra trailing items; items:false forbids them. For items:[{integer},{integer}] the advertised schema accepts [1,2,3] but the local validator rejects it. Use items: true (or omit items) so the local check matches what you advertise.
Submitted in error due to a local tooling path bug (stale draft re-submitted). This is a duplicate of the existing review; please see the current review at commit a5f9808 (#pullrequestreview-5102397076). Dismissing to reduce noise.
3 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?