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. |
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.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head a5f980851608c3d8413fc649240aef8c57cde3a8 against current main 486e9ba65536d45cd54e7f6b471c205a217ecc94.
The follow-up now isolates malformed schemas per tool, removes regex execution from the local Ajv path, and translates draft-07 tuple schemas for Ajv 2020. The prior main-process ReDoS and whole-registration failure are fixed. I independently reproduced the two remaining valid-input rejection cases already reported on this exact head (patternProperties with additionalProperties: false, and tuple arrays with trailing elements), so I did not duplicate those inline comments.
One additional merge-blocking current-main issue is reported inline: main has advanced the Runtime Host compatibility epoch to 109 for other incompatible wire changes, while this branch still uses 106 and conflicts in the epoch file.
Validation: clean npm run build:test, full workspace npm run typecheck, 49 focused Desktop/Runtime Host tests, changed-file Biome, and git diff --check passed. Both valid-input probes rejected before the MCP implementation was invoked. All hosted checks on this head are terminal green, but git merge-tree against current main reports a conflict in packages/runtime-host/src/protocol/index.ts, so no synthetic-merge test result exists.
Not independently exercised: native Windows/macOS runtime behavior or a third-party MCP server outside deterministic production-path 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.
| // Increment when the same protocol version no longer guarantees safe Client-Host | ||
| // interoperability. Mismatches are rejected before domain commands are admitted. | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 105 as const; | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 106 as const; |
There was a problem hiding this comment.
P1 - rebase and allocate a fresh compatibility epoch before this can merge. Current main (486e9ba65536d45cd54e7f6b471c205a217ecc94) already declares epoch 109 for other incompatible protocol changes, while this head declares 106 for the new Client Capability schema vocabulary. git merge-tree reports a content conflict in this file. After resolving that conflict, keeping main’s 109 would still fail scripts/protocol-epoch-check.mjs because client-capability.ts changes without a compatible-change declaration; keeping 106 would move the epoch backward. Rebase onto current main, preserve its 106-109 history, and assign the next unused epoch (currently 110) to this incompatible change.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The two-part shape is right and the publish half now lives in the correct owner, but the invocation half stands up a second JSON Schema validation authority inside the Electron main process, and that copy is where the remaining defects live. Reviewed at a5f98085; CI is green at this head, but the branch is DIRTY against main (ab7b7392) and cannot merge as is.
Good, and I do not want to re-litigate it: projectToolInputSchema now sits beside validateToolInputSchema in packages/runtime-host/src/protocol/client-capability.ts and both read the same CLIENT_CAPABILITY_SCHEMA_CONTAINER_SHAPES table, so protocol vocabulary knowledge is out of the Electron app. Per-tool isolation, the compile cache key, the ReDoS removal, and the default/enum/examples passthrough from the earlier rounds all check out at this head.
P1: the argument validator is a second implementation of code that already exists. packages/runtime/src/ai-sdk-backend.ts:576-660 already does exactly this: pull the raw schema off parameters.jsonSchema, dispatch on $schema across draft-07 / 2019-09 / 2020-12 Ajv instances, cache compiled validators in a WeakMap, and summarize errors through a function also named schemaErrorSummary. runtime-host-native-capabilities.ts:537-672 rewrites all of it with the dialect dispatch removed, which is precisely why it then needs adaptSchemaForLocalValidation to hand-translate draft-07 tuples, and why that same function is the third hand-written copy of the keyword-to-shape table in this repo. The two live bugs below are both unique to this copy. The smaller complete solution is upstream at the single place MCP schemas become tool parameters, packages/runtime/src/mcp-tools.ts:117: build the wrapper as jsonSchema(descriptor.inputSchema, { validate }). That activates the existing validate branch in validateDeclaredToolArgs (packages/runtime/src/tool-runtime.ts:3021-3066), which also closes the same gap for CLI MCP tools that this PR leaves open on main; the desktop path then only calls wrapper.validate, and :524-618 plus the new ajv dependency in apps/desktop/package.json can be deleted. If validation genuinely has to run in the Electron main process (a ReDoS blast-radius argument would be a fair reason), say so in the code and reuse the dialect dispatch and cache from ai-sdk-backend.ts instead of rewriting them.
P2: schema-invalid tools now vanish with no diagnostic, and first-party tools lost a startup invariant. buildPublishedCapabilities (runtime-host-native-capabilities.ts:444-456) applies the skip-and-console.warn path to every group, not just untrusted MCP. An MCP server that omits the top-level type (projectToolInputSchema requires type === 'object', client-capability.ts:775-780) or ships an external $ref gets its tool silently dropped, and the user sees a tool that is simply missing. For Browser, Computer Use, settings, and Rive this replaces a hard construction failure with a warning nobody reads; runtime-host-desktop-candidate.test.ts:547-570 shows the invariant being traded away. Suggest scoping the per-tool isolation to the MCP group, keeping first-party groups fatal, reporting skips through a real Desktop diagnostic channel, and defaulting a missing MCP root type to "object" rather than discarding the tool.
P3 (no reply needed): parseNativeToolArguments's falsy/primitive early return (:624-629) and trailing return args (:647) are unreachable now that toolInputSchema rejects both classes at construction; there is a stray double blank line at :673-675; and the ai devDependency added to apps/desktop/package.json exists only for a test fixture that a { jsonSchema: {...} } literal would satisfy. Also worth noting for the process cost of the local validator: the desktop WeakMap is keyed correctly now, but Ajv 8.20.0 keeps its own strong Map cache (ajv/dist/core.js:100,447-453), so compiled validators are retained for the life of the main process across MCP re-lists. I did not measure whether descriptor identity actually changes on refresh.
Next step: rebase first. packages/runtime-host/src/protocol/index.ts is the only content conflict against main (ab7b7392), which already declares epoch 109 while this head declares 106; keep main's 106-109 history and take 110 for this change, then re-run scripts/protocol-epoch-check.mjs. Then the P1 answer and the two P2 items, and I will look again.
| // Increment when the same protocol version no longer guarantees safe Client-Host | ||
| // interoperability. Mismatches are rejected before domain commands are admitted. | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 105 as const; | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 106 as const; |
There was a problem hiding this comment.
P1: this must be rebased before it can merge. Current main (ab7b7392) already declares epoch 109, this head declares 106, and git merge-tree reports this file as the only content conflict. Resolving it by keeping 109 still fails scripts/protocol-epoch-check.mjs, because client-capability.ts changes without a compatible-change declaration; keeping 106 moves the epoch backward. Rebase, preserve main's 106-109 comment history, and allocate 110 for this change.
| strict: false, | ||
| validateFormats: false, | ||
| } as const; | ||
| const schemaValidator = new Ajv2020(jsonSchemaValidatorOptions); |
There was a problem hiding this comment.
P1: this is a second copy of an existing owner. packages/runtime/src/ai-sdk-backend.ts:576-660 already compiles parameters.jsonSchema with Ajv, dispatches on $schema across draft-07 / 2019-09 / 2020-12, caches in a WeakMap, and has a schemaErrorSummary of the same name; this block reproduces all of it minus the dialect dispatch, which is exactly why adaptSchemaForLocalValidation below has to hand-translate draft-07 tuples and re-encode the keyword-to-shape table a third time. Smallest complete fix is upstream: packages/runtime/src/mcp-tools.ts:117 is the one place an MCP inputSchema becomes tool parameters, so build it as jsonSchema(descriptor.inputSchema, { validate }). That makes the existing validate branch in validateDeclaredToolArgs (packages/runtime/src/tool-runtime.ts:3021-3066) fire, covers CLI MCP tools too, and reduces this file to calling wrapper.validate, deleting lines 524-618 and the new ajv dependency. If validation has to stay in the Electron main process, reuse the dispatch and cache from ai-sdk-backend.ts rather than rewriting them, and record the reason here.
| 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: dropping patternProperties while additionalProperties is passed through unchanged at line 586 makes the local validator stricter than the schema you advertise. For {type:'object', patternProperties:{'^x-':{type:'string'}}, additionalProperties:false} the published schema accepts {"x-foo":"bar"}, but local Ajv sees an unmatched property and the call throws Invalid arguments before ever reaching the MCP server, so the tool registers and is never callable. patternProperties is the keyword this PR adds, so this pairing is the expected case, not an exotic one. liugddx raised this on this exact head (comment 3924949223). If the local validator goes away with the P1 fix this resolves itself; if it stays, relax additionalProperties to true whenever patternProperties is dropped.
| result.prefixItems = (val as unknown[]).map((entry) => | ||
| adaptSchemaForLocalValidation(entry), | ||
| ); | ||
| result.items = false; |
There was a problem hiding this comment.
P3: a draft-07 tuple items: [A, B] with no additionalItems permits extra trailing items, but items: false forbids them, so the advertised schema accepts [1,2,3] and the local validator rejects it. Same direction as the patternProperties issue above: local enforcement exceeding what was published. Omit items (or set it to true). Also raised on this head as comment 3924949230.
| // One malformed MCP descriptor must not take down the whole offer set: | ||
| // skip and name the offending tool so Browser, Computer Use, settings, | ||
| // and other MCP tools keep publishing. | ||
| console.warn( |
There was a problem hiding this comment.
P2: per-tool isolation is right for untrusted MCP descriptors, but this catch covers every group, including Browser, Computer Use, settings, and Rive. A first-party tool with a bad schema now disappears behind a console.warn nobody sees instead of failing at startup, which is the invariant runtime-host-desktop-candidate.test.ts:547-570 used to hold. On the MCP side the same silence is user-visible as a tool that simply is not there: an MCP schema omitting the top-level type is rejected by projectToolInputSchema (client-capability.ts:775-780) and dropped with no signal. Suggest scoping the try/catch to the MCP group, keeping first-party groups fatal, surfacing skips through a real Desktop diagnostic path, and defaulting a missing MCP root type to "object" before projecting.
github-actions
Bot
added
effort/XL
5bd4093 to
c07c0f9
Compare
github-actions
Bot
added
effort/L
|
Thanks everyone for the detailed reviews and follow-up feedback. I have updated the implementation to fully support MCP tools using the The main changes include:
I also rebased the branch onto the latest Local verification completed successfully:
The latest commit has been pushed to the PR branch. |
e6fd844 to
67c55b6
Compare
Thanks for the detailed follow-up review. Yes, I understand that the previous E2E positional failure and validation bypass issues are resolved, but the latest head still has the four remaining blocking issues you listed. I’ll address them together:
I’ll also replace the non-causal tests with regression tests that:
I’ll rebase onto the latest |
liugddx
left a comment
liugddx
left a comment
There was a problem hiding this comment.
Reviewed the cleaned, rebased MCP-only head. The prior blocking findings are addressed: MCP JSON Schema validation is delegated to the MCP server instead of executing untrusted regexes in the Runtime main thread; Desktop uses the shared Runtime Host projector; the dynamic MCP isolation and annotated-schema tests are causal; and the protocol epoch is preserved on top of main. Focused Desktop, Runtime MCP, and Runtime Host protocol suites pass locally, along with the relevant typechecks and protocol epoch gate. The repository-wide build still has unrelated existing UI type errors in @maka/ui.
a5df40c to
830a8c6
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.
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>
88060f0 to
a85e141
Compare
liugddx
left a comment
liugddx
left a comment
There was a problem hiding this comment.
PR 的根因判断是对的：ai.jsonSchema() MCP 工具不能继续走 Zod-only 路径，Runtime Host 侧统一 projection 也比早期 Desktop 内部复制规则更合理。当前 CI test 为绿色，diff 为 +864/-68，其中生产代码约 +284/-46，测试约 +580/-22。
P2 — 本地预检会错误拒绝合法的 Draft 2020-12 MCP 输入。 packages/runtime/src/mcp-schema-preflight.ts:22,44 始终使用默认 Ajv 编译器；而 MCP discovery 在 packages/mcp/src/index.ts:2311 已删除 $schema，所以 Draft 2020-12 schema 会被按 Draft-07 语义解释。已复现：
{
"$schema": "https://json-schema.org/draft/2020-12/schema",
"type": "object",
"properties": {
"values": {
"type": "array",
"prefixItems": [{ "type": "string" }],
"items": { "type": "number" }
}
},
"required": ["values"]
}输入 { "values": ["head", 42] } 按 Draft 2020-12 合法，但当前本地预检返回 "/values/0 must be number"，请求在到达 MCP server 前就被拒绝。最小修复是删除这套 Runtime 本地完整 JSON Schema 预检，让 MCP server 保持唯一验证权威；如果必须保留，则保留原始 dialect 并复用已有的 dialect-aware Ajv 编译路径。
P2 — Desktop Client Capability 调用绕过了 PR 新增的 JSON Schema validator。 packages/runtime/src/mcp-tools.ts:146-149 给 ai.jsonSchema() wrapper 安装了 validate，但 apps/desktop/src/main/runtime-host-native-capabilities.ts:680-688 对非 Zod 参数直接返回原始 args。因此类似下面的调用会被 Desktop native capability 路径直接转发：
{
"inputSchema": {
"type": "object",
"required": ["token"],
"properties": { "token": { "type": "string" } }
},
"arguments": {}
}MCP server 可能随后拒绝它，但 PR 声称的“native tool invocation now parses arguments through the declared schema shape”并没有在这条路径成立。要么让 parseNativeToolArguments() 调用 wrapper 的 validate，要么明确把 MCP server 定为唯一验证方，并删掉/收窄 PR body 中关于 native invocation 本地解析的表述，同时补上对应行为测试。
P3 — Runtime 又维护了一份 JSON Schema 递归规则。 packages/runtime/src/mcp-schema-preflight.ts:53-100 重新枚举了 properties、$defs、items、allOf、prefixItems、additionalItems 等嵌套容器；协议侧已有 CLIENT_CAPABILITY_SCHEMA_CONTAINER_SHAPES 并同时驱动 projection 与 protocol validation。当前没有额外确认的立即故障，但后续新增 schema keyword 时会出现第三份 authority。删除本地预检是最小解决方案；若保留，应抽取共享 walker，而不是继续复制容器语义。
Tests
新增的 projection、根类型拒绝、动态 MCP 工具隔离和空组合关键字测试大多是有因果性的，应保留。但有三处测试质量问题：
apps/desktop/src/main/__tests__/runtime-host-desktop-candidate.test.ts:550-598 把原先验证固定 Browser capability 失败即回滚/关闭 Host 的测试替换成了动态 MCP 隔离测试。应恢复旧测试，再单独增加当前 MCP isolation case。
apps/desktop/src/main/__tests__/mcp-runtime-e2e.test.ts:93-125 只断言 annotated 被发布，实际调用却改成了不存在的 missing 工具；因此 identity/binding 错误仍可能漏过。应保留负例，并真正调用一次 annotated。
runtime-host-native-capabilities.test.ts:290-325 的 __proto__ 测试不明显依赖本 PR；370-448 与 450-503 的非法 patternProperties provider 测试也有重复，可删除或将其中一个改成直接协议边界测试。
我本地成功执行了协议 epoch 检查，结果为 112 -> 113，git diff --check 通过，并直接运行了上述 schema differential 反例。完整 Runtime build 未能完成，因为隔离 worktree 中缺少可用的 tsc 二进制；GitHub 当前 test check 是绿色的。
四个核心问题
是否最优？ 还不是。发布侧修复方向正确，但本地预检引入了 dialect differential，并且 Native capability 路径没有使用它。
是否符合第一原则？ MCP server 已经拥有完整 JSON Schema authority；Runtime 再解释一遍 schema 会产生不一致，尤其是方言和组合关键字。
Occam 能删什么？ 优先删除 mcp-schema-preflight.ts、wrapper 上的本地 validate 以及相关测试；保留 jsonSchema wrapper 适配、Runtime Host 单一 projection 和动态工具隔离。
低质量测试如何处理？ 恢复固定 capability fail-fast 测试；真正调用 annotated；增加 Draft 2020-12 prefixItems/items 合法输入回归测试；删除不具备 PR 因果性的 __proto__ 和重复正则测试。
最小正确版本是：保留 MCP JSON Schema wrapper 支持、Runtime Host 的统一 projection 和逐工具隔离；不要在 Runtime 重新建立一个默认 Draft-07 的本地验证权威。
Dismissed after applying the requested MCP schema validation and test fixes in 105a79f.
liugddx
left a comment
liugddx
left a comment
There was a problem hiding this comment.
已落实本轮 review：
- 删除 Runtime 本地 Ajv JSON Schema preflight，MCP server 重新成为完整 schema、dialect 和组合关键字的唯一验证权威。
- Desktop native capability 对 JSON Schema 参数保持原样转发，并补了回归测试；Zod 参数仍走本地解析。
- 恢复固定 Browser capability fail-fast 测试，保留 MCP 动态工具隔离；补上
annotated的真实调用和missing负例。 - 删除与本 PR 无直接因果的
__proto__和重复正则测试。
目标提交：105a79fc79fe11b2971e90fb89260b82c80b5be0。
4 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 MCP server remains the sole authority for validating the complete JSON Schema, including its dialect and composition keywords. Runtime Host owns the protocol projection and keeps malformed dynamic MCP tools isolated from healthy capabilities.
Verification
Ran locally:
npm --workspace @maka/mcp run buildnpm --workspace @maka/computer-use run buildnpm --workspace @maka/desktop run build:mainnode --test packages/runtime/dist/__tests__/mcp-tools.test.js apps/desktop/dist/main/__tests__/runtime-host-native-capabilities.test.js apps/desktop/dist/main/__tests__/runtime-host-desktop-candidate.test.js apps/desktop/dist/main/__tests__/mcp-runtime-e2e.test.js(64 passed, 0 failed)npx biome checkon the changed filesgit diff --checkThe isolated local worktree could not complete the full Runtime/Runtime Host build because its existing dependency graph is incomplete. GitHub CI for
105a79fc79fe11b2971e90fb89260b82c80b5be0has already passed Build and Typecheck; the remaining CI stages are still running.AI use
Tool(s) and scope:
Checklist
Does this PR entail a change in behavior?