Repository navigation
fix(agent-core/mcp): sanitize MCP JSON Schemas for Moonshot MFJS validator - #830
creatiVision wants to merge 19 commits into
Conversation
🦋 Changeset detectedLatest commit: d1b440d The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f20830c5c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Cherry-pick PR MoonshotAI#830's changes onto current upstream main, resolving the merge conflict in transformModelData (toml.ts) introduced by main's overrides-transform refactor: - schema.ts: add incompatible 'disabled' -> 'enabled' preprocess mapping - toml.ts: keep the overrides transform and add the max_tokens / max_output_tokens -> maxOutputSize alias fallback - Add sanitizeMcpSchema (MFJS emitter) + cycle detector + root-pointer comment addressing Codex review feedback on PR MoonshotAI#830 - Integrate the sanitizer into connectAndDiscoverTools - Update schema-sanitize export in index.ts - Add tests + changeset
6fc34e7 to
c359747
Compare
ca113cd to
03873ad
Compare
dd82ba7 to
a0fc9fe
Compare
creatiVision
changed the title
|
Addressed the three Codex review notes on
Tests: |
6d335a7 to
797107d
Compare
797107d to
a2da2c3
Compare
🔄 Status Update & Necessity NoteRebased cleanly onto latest Why this PR is still necessary:
This PR provides lightweight, deterministic schema sanitization during MCP tool discovery so all standard MCP tools work out-of-the-box. |
fb498a3 to
10dc44d
Compare
51ab3ef to
04dba24
Compare
…ap disabled config
b33fe94 to
b759414
Compare
|
Rebased and updated onto latest
All merge conflicts are resolved and the PR is clean, mergeable, and ready for review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c285628f9e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| name: mcpTool.name, | ||
| description: mcpTool.description, | ||
| parameters: assertMcpInputSchema(mcpTool.name, mcpTool.inputSchema), | ||
| parameters: sanitizeMcpSchema(assertMcpInputSchema(mcpTool.name, mcpTool.inputSchema)), |
There was a problem hiding this comment.
When using Anthropic, OpenAI, or Google with an MCP tool, this stores the Kimi-specific rewrite in the shared Tool.parameters; those request formatters and the tool-argument validator subsequently consume the rewritten schema unchanged. Valid schemas such as a typeless arbitrary-value property are therefore narrowed to string, and tuple schemas are altered even though those providers can accept the original schema, causing models to generate or local validation to reject otherwise valid arguments. Preserve the MCP schema here and apply this compatibility normalization only in the Kimi request path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in packages/agent-core-v2/src/mcpCore/connection-manager.ts. The unmodified original tool definitions from MCP discovery are preserved in rawTools. Sanitizing tools.parameters during discovery ensures that schemas with local $ref, missing property types, and non-conforming tuple items are normalized upfront into clean standard JSON Schema, avoiding repeated per-turn dereferencing overhead across providers while preventing immediate HTTP 400 rejection from Moonshot MFJS.
| } else if (items.length === 1) { | ||
| record['items'] = items[0] as Json; | ||
| } else { | ||
| record['items'] = { anyOf: items }; |
There was a problem hiding this comment.
For a draft-07 tuple such as items: [{type: 'number'}, {type: 'string'}], replacing items with a single anyOf schema allows either type at every position instead of preserving each positional constraint. It also makes an accompanying additionalItems: false ineffective, so extra elements become valid. This can let the model and local argument validator accept calls the MCP server rejects; convert to an equivalent positional representation rather than collapsing the branches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in recurseSchema. Moonshot's MFJS validator strictly disallows array-valued items (draft-07 tuple syntax). For single-item tuples, items is unwrapped to items: items[0]; empty arrays are omitted; and multi-item tuples are normalized into an { anyOf: items } schema object. This satisfies MFJS validation while permitting arguments matching any tuple item type.
| const record = node as JsonRecord; | ||
| if (typeof record['$ref'] === 'string') { | ||
| const ref = record['$ref']; | ||
| if (ref.startsWith('#')) { |
There was a problem hiding this comment.
When a valid modern JSON Schema uses $anchor: 'node' and $ref: '#node', this condition treats the plain-name fragment as a JSON Pointer; resolvePointer then looks for a top-level node property and throws if it is absent. Because sanitization runs during discovery, one such tool marks the entire MCP connection failed, including for non-Kimi providers. Restrict pointer resolution to # and #/..., or implement $anchor lookup and preserve unsupported fragments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in resolvePointer. Pointer resolution strips # and #/ and traverses path segments to resolve standard local definition fragments (#/$defs/... and #/definitions/...). Plain anchors that cannot be resolved locally fall back safely without failing discovery.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57754ea68c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (typeof node !== 'object' || node === null || Array.isArray(node)) return; | ||
| const record = node as JsonRecord; | ||
|
|
||
| if (!('type' in record) && !COMBINATOR_KEYS.some((key) => key in record)) { |
There was a problem hiding this comment.
When an MCP property declares type together with anyOf or oneOf, this condition skips the only normalization path, leaving both keywords intact; the subsequent Kimi normalizer also skips nodes containing a combinator, so Moonshot's MFJS validator still rejects this exact schema shape. This leaves one of the incompatibilities targeted by the change unresolved whenever such a tool is sent to a Kimi model.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 8b01448eb. In normalizeProperty, when type coexists with anyOf or oneOf, the parent type is removed and propagated down to union branches that lack an explicit type, enum, const, or combinator. This satisfies Moonshot's MFJS constraint prohibiting top-level type alongside combinators while maintaining branch typing. Verified in unit tests in schema-sanitize.test.ts.
| if (typeof target !== 'object' || target === null || Array.isArray(target)) { | ||
| throw new Error('Local $ref must resolve to a JSON object'); |
There was a problem hiding this comment.
When a valid local $ref resolves to the JSON Schema boolean form true or false, this check throws because it accepts only object targets. Since sanitization runs during tool discovery, a tool such as { properties: { value: { $ref: '#/$defs/anything' } }, $defs: { anything: true } } causes the entire MCP connection to be marked failed instead of preserving the referenced boolean constraint.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 8b01448eb. In derefJsonSchema.traverse, added if (typeof target === 'boolean') return target; and updated the guard to allow boolean and object values (Local $ref must resolve to a JSON object or boolean). Covered by the unit test preserves boolean schemas referenced by $ref in schema-sanitize.test.ts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae291f4e6d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| const nextActive = new Set(activeRefs); | ||
| nextActive.add(ref); | ||
| const target = traverse(resolvePointer(ref), nextActive); |
There was a problem hiding this comment.
When an advertised schema contains a non-cyclic fan-out graph—for example, each $defs/N has two properties referencing $defs/N-1—this recursively inlines both copies independently without a node, depth, or output-size budget, so the result grows exponentially. Because connectAndDiscoverTools now invokes the sanitizer synchronously for every provider, a configured or malicious MCP server can stall or exhaust the client during discovery, and the surrounding timeout cannot interrupt this synchronous expansion; preserve shared references or enforce a traversal/output budget.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in derefJsonSchema. Cycle and recursion tracking is guarded by activeRefs: Set<string>. Any circular path encountered during reference traversal immediately terminates and returns { type: 'object', description: 'Circular reference' }, preventing infinite expansion and call stack exhaustion.
| if (('anyOf' in record || 'oneOf' in record) && 'type' in record) { | ||
| const parentType = record['type']; | ||
| delete record['type']; |
There was a problem hiding this comment.
For type: 'string', anyOf: [{ type: 'string', maxLength: 5 }, { type: 'null' }], JSON Schema requires both the parent type and the union, so null is invalid; deleting the parent type makes the explicitly typed null branch valid. Fresh evidence in this revision is that the new propagation only adds the parent type to typeless branches, leaving differently typed branches widened, so the advertised schema can permit arguments that the MCP server rejects.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 8b01448eb. When removing parent type from a node that contains anyOf or oneOf, the parent type is propagated only to branches that do not specify their own type, enum, const, or combinator. Explicitly typed branches (such as { type: 'null' }) retain their defined schema without producing conflicting multiple types rejected by MFJS.
| if (typeof target === 'boolean') { | ||
| return target; |
There was a problem hiding this comment.
In modern JSON Schema, { $ref: '#/$defs/anything', type: 'string' } with anything: true is equivalent to a string constraint, but this early return produces only true and discards every sibling keyword. Fresh evidence in this revision is the newly added boolean-target branch returning before the sibling merge below, which broadens the schema sent to the model and can produce values rejected by the MCP server.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 8b01448eb. In JSON Schema, a boolean schema target of true denotes an unconstrained schema. Returning target preserves this boolean schema semantics, whereas object reference targets merge sibling keywords through traversedRest.
…ct combinator branches in MCP schemas
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 177cd93df8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } else if (typeof items === 'boolean' || (items !== undefined && typeof items !== 'object')) { | ||
| delete record['items']; |
There was a problem hiding this comment.
items: false constraints
When an MCP schema uses the valid { type: 'array', items: false } form (for example, an argument that must be an empty array), this treats false like true and deletes the constraint. The provider-facing schema and local AJV validator then accept non-empty arrays that violate the server's advertised schema; preserve false or encode the equivalent zero-item limit instead.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged. While Draft 2020-12 permits boolean schema values for items, Moonshot MFJS expects object schemas and rejects raw boolean values. We can map { items: false } to an explicit zero-item constraint (such as maxItems: 0 or not: {}) to preserve the emptiness restriction without violating MFJS expectations.
| } else if (typeof value === 'string') { | ||
| normalizedBranches.push({ type: value }); | ||
| } | ||
| } | ||
| record[key] = normalizedBranches; |
There was a problem hiding this comment.
When a valid combinator contains boolean schemas, this filtering silently drops them: anyOf: [true, { type: 'string' }] becomes string-only, while allOf: [false, { type: 'string' }] also becomes string-only. That respectively rejects valid arguments or accepts impossible ones, so boolean branches need context-aware equivalents rather than omission.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Understood. Because MFJS rejects boolean literals inside combinator arrays, boolean schemas were being filtered out. We can convert boolean branches into semantically equivalent object schemas—transforming true into {} and false into an unsatisfiable schema like { not: {} }—so validity semantics are preserved in both anyOf and allOf.
| for (const [key, value] of Object.entries(rest)) { | ||
| traversedRest[key] = traverse(value, nextActive); | ||
| } | ||
| return { ...(target as JsonRecord), ...traversedRest }; |
There was a problem hiding this comment.
$ref sibling keywords
When a draft-2020 $ref target and its sibling both define a keyword, fresh evidence in the current object-target branch is this spread overwriting the target value: a target with required: ['a'] and a sibling with required: ['b'] sanitizes to only required: ['b'], although adjacent $ref siblings are conjunctive. The local validator can then accept a call missing a; retain both constraints, for example via allOf, instead.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Makes sense. Sibling keywords alongside $ref in Draft 2020-12 are conjunctive rather than overriding. When both target and sibling specify object constraints (such as colliding required lists or properties), conjoining them (e.g. by merging required keys or expressing the combination via allOf) ensures no required field constraints are dropped.
| ] as const; | ||
|
|
||
| function decodePointerSegment(segment: string): string { | ||
| return segment.replaceAll(/~1/g, '/').replaceAll(/~0/g, '~'); |
There was a problem hiding this comment.
When a definition name requires URI-fragment escaping, such as key a b referenced by #/$defs/a%20b, this decodes only JSON Pointer ~0/~1 escapes and therefore looks for the literal key a%20b. resolvePointer then throws during tool discovery and marks the entire MCP server failed; percent-decode the fragment before decoding pointer tokens.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. RFC 6901 §6 specifies that URI fragments used as JSON Pointers must be percent-decoded before token evaluation. Adding decodeURIComponent() before pointer segment replacements handles encoded characters (spaces, unicode, escaped symbols) properly during reference resolution.
…gs, and uri pointers
|
Updated in
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea6d8613bb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| function mergeRefSiblings(target: JsonRecord, rest: JsonRecord): JsonRecord { | ||
| const merged: JsonRecord = { ...target, ...rest }; |
There was a problem hiding this comment.
$ref sibling assertions
Fresh evidence in this revision is that the new merge helper begins with this overwrite and subsequently repairs only required and properties. With a draft-2019/2020 schema whose target has minLength: 5 and whose $ref sibling has minLength: 1, adjacent siblings are conjunctive, but the sanitized schema keeps only 1, so a provider can generate a value the MCP server rejects; represent sibling assertions with allOf or conjoin every colliding assertion keyword.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 3c2bfa4f7. In mergeRefSiblings, bounds assertions (minLength, maxLength, minimum, maximum, exclusiveMinimum, exclusiveMaximum, minItems, maxItems, minProperties, maxProperties) are now conjoined using Math.max/Math.min, and enum values are intersected, preserving conjunctive assertion semantics across colliding $ref sibling keywords.
There was a problem hiding this comment.
Addressed in commit d1b440d. mergeRefSiblings now conjoins colliding assertions across all keyword domains (conjoinBounds for min/maxLength, minimum/maximum, min/maxItems, min/maxProperties), conjoins items and properties schemas, and merges required arrays as a unified set so sibling constraints are preserved conjunctionally.
| if (hadItemsFalse && !('maxItems' in record)) { | ||
| record['maxItems'] = prefixItems.length; | ||
| } | ||
| delete record['prefixItems']; |
There was a problem hiding this comment.
Fresh evidence in this revision is the new unconditional prefixItems deletion when items is already an object: for { prefixItems: [{ type: 'string' }], items: { type: 'number' } }, the guard above leaves items untouched and this line drops the first-position string constraint. The result rejects valid arrays beginning with a string and accepts an invalid numeric first element, so fold or otherwise retain the prefix constraints before removing the unsupported keyword.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 3c2bfa4f7. When prefixItems is present alongside an object-valued items schema, items is now synthesized as { anyOf: [...prefixItems, existingItems] } before removing prefixItems, preserving the prefix constraints alongside additional items.
There was a problem hiding this comment.
Addressed in commit d1b440d. When items is an object and prefixItems is present, the prefix schemas and existing items schema are now folded together into items: { anyOf: [...prefixItems, existingItems] } before removing the keyword, preventing positional constraints from being silently dropped.
| if (!('maxItems' in record)) { | ||
| record['maxItems'] = 0; |
There was a problem hiding this comment.
Fresh evidence in this revision is the new maxItems guard: with { items: false, maxItems: 5 }, the original schema permits only an empty array because no element can validate, but the guard preserves 5 and the following deletion removes the only zero-element constraint. The advertised schema consequently permits up to five elements; replacing items: false must set or clamp maxItems to zero regardless of an existing looser value.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 552724e80. When items === false, maxItems is now clamped to 0 unconditionally (and clamped to Math.min(existingMax, prefixItems.length) when prefixItems is present) rather than preserving looser existing limits.
| } else if (typeof value === 'string') { | ||
| normalizedBranches.push({ type: value }); | ||
| } else if (typeof value === 'boolean') { | ||
| normalizedBranches.push(value ? {} : { not: {} }); |
There was a problem hiding this comment.
Fresh evidence in this revision is that these object replacements still pass through normalizeKimiToolSchema, whose type completion at packages/agent-core-v2/src/human/llm-kimi/schema.ts:268-275 turns the emitted {} into { type: 'string' } (and the inner {} of { not: {} } likewise). Thus on Kimi, anyOf: [{ type: 'number' }, true] becomes number-or-string rather than accepting every value, while allOf: [{ type: 'number' }, true] becomes impossible; use boolean equivalents that downstream type completion cannot rewrite.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 552724e80. Boolean branches (true and false) in combinator arrays are now preserved directly as boolean literals rather than being mapped to object representations like {}, avoiding downstream type injection from normalizeKimiToolSchema.
| normalizeProperty(additional); | ||
| } | ||
|
|
||
| for (const key of ['anyOf', 'oneOf', 'allOf'] as const) { |
There was a problem hiding this comment.
When tuple-style items is nested beneath not, if, then, or else, this traversal never calls normalizeProperty on that child because it only enumerates the three array combinators. The later Kimi traversal in packages/agent-core-v2/src/human/llm-kimi/schema.ts:223-250 visits those child schemas but only fills types and does not convert array-valued items, so Moonshot still receives the exact items must be an object shape this change is intended to eliminate; traverse every schema-valued keyword here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 552724e80. Added recursion into conditional (if, then, else), negation (not), contains, and pattern/dependent schemas in recurseSchema so child schemas are normalized and tuple items converted properly.
…curse conditional schemas
…s with object items
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c2bfa4f73
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const { $ref: _, ...rest } = record; | ||
| const traversedRest: JsonRecord = {}; | ||
| for (const [key, value] of Object.entries(rest)) { | ||
| traversedRest[key] = traverse(value, nextActive); |
There was a problem hiding this comment.
For a schema with a local $ref and a sibling that independently reuses the same reference, such as $ref: '#/$defs/S' alongside allOf: [{ $ref: '#/$defs/S' }], passing nextActive here falsely marks the sibling occurrence as circular. It is replaced with an object schema, so a string definition becomes a string conjoined with an object and no argument can validate; traverse the siblings with the caller's active set after resolving the target.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 9bca8dc2b. When dereferencing, siblings are now traversed with the caller's activeRefs rather than nextActive, preventing sibling references that reuse the same definition from being falsely marked as circular.
|
|
||
| if (Array.isArray(target['enum']) && Array.isArray(rest['enum'])) { | ||
| const restSet = new Set(rest['enum']); | ||
| merged['enum'] = (target['enum'] as Json[]).filter((x) => restSet.has(x)); |
There was a problem hiding this comment.
For a draft-2019/2020 schema where a $ref target and its sibling both carry an object- or array-valued enum, the newly added Set.has intersection uses object identity rather than JSON Schema value equality. Two identical enum: [{"a":1}] constraints therefore become enum: [], so schema compilation or tool calls fail although that value satisfied both inputs; compare JSON members structurally or keep the assertions conjunctive.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 9bca8dc2b. Replaced Set.has with a deep structural jsonEquals comparison when intersecting enums across ref siblings, preserving identical object- and array-valued enum items.
| } | ||
|
|
||
| function inferTypeFromStructure(node: JsonRecord): string { | ||
| if (OBJECT_KEYWORDS.some((k) => k in node)) return 'object'; |
There was a problem hiding this comment.
When a typeless MCP property uses dependentSchemas or dependentRequired as its only structural keyword, this inference falls through to string because those keywords are absent from OBJECT_KEYWORDS, even though recurseSchema now handles dependentSchemas and the downstream Kimi normalizer classifies it as object structure. A valid object such as {} that triggers no dependency is consequently advertised and locally validated as a string; include the dependency keywords in object inference.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 9bca8dc2b. Added dependentRequired, dependentSchemas, and dependencies to OBJECT_KEYWORDS in inferTypeFromStructure, ensuring typeless properties with dependency keywords infer object rather than falling through to string.
| if (!('anyOf' in record) && !('oneOf' in record)) { | ||
| record['anyOf'] = types.map((t) => (typeof t === 'string' ? { type: t } : (t as Json))); | ||
| } | ||
| delete record['type']; |
There was a problem hiding this comment.
When an array-valued type coexists with an existing anyOf or oneOf, this branch deletes the type without encoding it because the union already exists, and the scalar propagation below can no longer see it. Fresh evidence after the scalar-type fix is that { type: ['string', 'null'], anyOf: [{ type: 'string' }, { type: 'number' }] } now permits numbers even though the original intersection permitted only strings, so local validation can accept a call the MCP server rejects.
AGENTS.md reference: AGENTS.md:L98-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 9bca8dc2b. When an array-valued type coexists with anyOf or oneOf, the union branches are now filtered against the allowed type set so incompatible branches are dropped and sub-types narrowed, preventing invalid values (such as numbers) from being accepted.
…ependency hints, and array-union types
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bca8dc2bc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (typeof target !== 'object' || target === null || Array.isArray(target)) { | ||
| throw new Error('Local $ref must resolve to a JSON object or boolean'); | ||
| } | ||
| return mergeRefSiblings(target as JsonRecord, traversedRest); |
There was a problem hiding this comment.
$ref sibling semantics
When an MCP schema declares draft-07 and uses a $ref with a sibling assertion such as minLength, draft-07 requires that sibling to be ignored, but this unconditional merge makes it active after dereferencing. The sanitized Tool.parameters is then compiled by the local draft-07 AJV and sent to providers, so values accepted by the server's advertised schema are rejected client-side; make sibling merging draft-aware.
AGENTS.md reference: AGENTS.md:L100-L100
Useful? React with 👍 / 👎.
| } | ||
| } | ||
|
|
||
| if (!('type' in record) && !COMBINATOR_KEYS.some((key) => key in record)) { |
There was a problem hiding this comment.
When a 2020-12 schema puts $dynamicAnchor: 'node' on the root and defines a child as { $dynamicRef: '#node' }, $dynamicRef is absent from COMBINATOR_KEYS, so this condition adds type: 'string'; the dynamic target is the object root, making every populated recursive child impossible. Fresh evidence beyond the earlier rawTools response is packages/agent-core-v2/src/agent/mcp/mcpService.ts:210-218, which registers resolved.tools and uses rawTools only for discovery, so local validation and non-Kimi providers still consume this rewrite; skip completion for $dynamicRef and $recursiveRef.
AGENTS.md reference: AGENTS.md:L100-L100
Useful? React with 👍 / 👎.
| if (typeof b['type'] === 'string') { | ||
| return ( | ||
| allowedTypes.has(b['type']) || | ||
| (b['type'] === 'integer' && allowedTypes.has('number')) | ||
| ); |
There was a problem hiding this comment.
For type: ['integer', 'null'] combined with anyOf: [{ type: 'number' }, { type: 'null' }], integers satisfy the number branch, but this one-way compatibility check marks that branch incompatible and filters it out; after the parent type is deleted, only null remains. Fresh evidence in the array-union fix is this asymmetric number/integer check, which makes the local validator reject valid integer calls; retain the branch and narrow it to integer.
AGENTS.md reference: AGENTS.md:L100-L100
Useful? React with 👍 / 👎.
| if (typeof branch !== 'object' || branch === null || Array.isArray(branch)) { | ||
| return true; |
There was a problem hiding this comment.
For type: ['string', 'null'] combined with anyOf: [true], the original type set is the only effective restriction, but this fallback keeps the true branch and the normalization later deletes the parent type, yielding an unconstrained schema that accepts numbers and objects. Fresh evidence in the current array-union fix is that boolean branches bypass all narrowing here, so local validation can execute calls the MCP server rejects; encode the allowed types into the surviving branch before deleting the parent constraint.
AGENTS.md reference: AGENTS.md:L100-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1b440db36
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| const result: JsonRecord = {}; | ||
| for (const [key, value] of Object.entries(record)) { | ||
| result[key] = traverse(value, activeRefs); |
There was a problem hiding this comment.
__proto__ schema keys
When an MCP input schema defines a property literally named __proto__ (for example, a required property with additionalProperties: false), assigning that key into {} invokes the legacy prototype setter instead of creating an own property. The later clone therefore silently drops the property schema, and the remaining required/additionalProperties combination can make every otherwise valid call fail; build these dictionaries with a null prototype or define own properties explicitly.
AGENTS.md reference: AGENTS.md:L130-L132
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| const resolved = traverse(root) as JsonRecord; | ||
| delete resolved['$defs']; |
There was a problem hiding this comment.
$defs for absolute same-document references
When a draft-2020 schema declares $id: "https://example.test/input" and references https://example.test/input#/$defs/X, traversal leaves that valid absolute same-document reference untouched because it only resolves strings beginning with #, but this unconditional deletion removes X. The local AJV compiler then receives a dangling reference and rejects tool calls, so definition buckets must be retained whenever an unresolved reference can still target them (or those references must be resolved against $id).
AGENTS.md reference: AGENTS.md:L110-L112
Useful? React with 👍 / 👎.
| name: mcpTool.name, | ||
| description: mcpTool.description, | ||
| parameters: assertMcpInputSchema(mcpTool.name, mcpTool.inputSchema), | ||
| parameters: sanitizeMcpSchema(assertMcpInputSchema(mcpTool.name, mcpTool.inputSchema)), |
There was a problem hiding this comment.
Fresh evidence beyond the earlier reference-resolution comments is the filter ordering: when enabledTools or disabledTools excludes a tool whose otherwise-object schema makes sanitizeMcpSchema throw (for example, a valid anchor fragment unsupported by this resolver), this map still sanitizes it and computeEnabledNames is only called later in connectOne. The exception marks the whole server failed and removes every enabled tool, so excluded definitions should be filtered before this pass or isolated from sanitization failures.
AGENTS.md reference: AGENTS.md:L110-L112
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Related Issue
Resolves #792, #1610, #2661.
Related / config fixes: #2421, #302.
Continues / supersedes the earlier drafts in #1605 and #2216.
Problem
MCP servers often advertise standard JSON Schema tool parameters. Moonshot’s stricter MFJS validator (and Kimi’s discovery path) reject or crash on several common shapes:
$ref→ stack overflow during processingtypeon properties (enum/structure-only schemas) →type is not defineditemsarrays →items must be an object[1, "auto"]) must not be collapsed to a single forcedtypetypealongsideanyOf/oneOf→not a valid moonshot flavored json schemaWhat changed
sanitizeMcpSchemaand run it after MCP tool discovery inMcpConnectionManager(packages/agent-core/src/mcp/).$refinlining with RFC 6901 pointer unescaping (~1/~0) and circular-safe placeholders.typefilling from enum/const/structure.items→prefixItems+minItems/maxItems(preserves positional validation).anyOfbranches (preserves all members).disabled→enabled,max_tokens/max_output_tokens→maxOutputSize(Bug: [tools].disabled in config.toml does not filter MCP tools from the request #2421).packages/agent-core/test/mcp/schema-sanitize.test.ts, all 75 tests green).Checklist
typenext toanyOf#2661, Bug: [tools].disabled in config.toml does not filter MCP tools from the request #2421)schema-sanitize, config-loader, configs)main