Repository navigation
fix(agent-core/mcp): sanitize MCP JSON Schemas for Moonshot MFJS validator - #830
creatiVision wants to merge 23 commits into
Conversation
🦋 Changeset detectedLatest commit: 5a4d9f3 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.
There was a problem hiding this comment.
Addressed in commits 3c2bfa4f7 and dada01971. Colliding assertion bounds (min/max lengths and values) are conjoined, and closed-object properties with additionalProperties: false are separated into conjunctive allOf branches.
| 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.
There was a problem hiding this comment.
Addressed in commit 3c2bfa4f7. prefixItems constraints are preserved when items is an object by wrapping them into { anyOf: prefixItems } or conjoining with the element schema.
| 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 👍 / 👎.
There was a problem hiding this comment.
Addressed in commits 9bca8dc2b and dada01971. Respects draft-07 $ref sibling handling by isolating closed-object assertions in allOf.
| } | ||
| } | ||
|
|
||
| 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 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit d1b440db3. Dynamic references skip inappropriate type completion.
| 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 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit d1b440db3. Preserves number branches when integer is allowed.
| 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.
Addressed in commit d1b440db3. Preserves parent types across boolean union branches.
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 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit c61021c9e. Preserves own __proto__ schema keys using Object.hasOwn instead of prototype lookups.
| } | ||
|
|
||
| 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 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit c61021c9e. Retains $defs when absolute same-document references remain.
| 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 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit c61021c9e. Excluded tools are skipped before schema sanitization during discovery.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c61021c9e4
ℹ️ 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 = record['$ref']; | ||
| if (ref.startsWith('#')) { | ||
| if (activeRefs.has(ref)) { | ||
| return { type: 'object', description: 'Circular reference' }; |
There was a problem hiding this comment.
When an MCP definition recursively refers to itself, such as a linked node that requires value at every level, this replacement turns the recursive occurrence into an otherwise unconstrained object. connectAndDiscoverTools registers that schema for local validation for every provider, so {value: 'x', next: {}} is accepted even though the advertised recursive definition rejects it (and array or scalar recursion is changed to the wrong type); preserve the recursive constraint rather than hard-coding an object cutoff.
AGENTS.md reference: AGENTS.md:L110-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit dada01971. When a recursive local $ref cycle is encountered, traverse constructs a cutoff schema preserving the target structural constraints (type, required keys, non-circular properties, bounds) rather than forcing an unconstrained object, preserving validation invariants for recursive objects and non-object types.
| const root = structuredClone(schema); | ||
|
|
||
| function resolvePointer(pointer: string): Json { | ||
| const pathStr = pointer.replace(/^#\/?/, ''); |
There was a problem hiding this comment.
#/ distinct from #
When a valid local reference is #/, this regex strips the slash and makes pathStr empty, so it resolves the document root. In JSON Pointer, # is the root whereas #/ addresses the root's empty-string property; an MCP schema using that legal property for a definition therefore gets a root expansion or circular placeholder instead of its target and is validated differently. Check exactly # before splitting so the empty segment is retained.
AGENTS.md reference: AGENTS.md:L110-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit dada01971. In resolvePointer, pointer resolution checks exact # for root document before segment splitting, preserving empty string path segment (root['']) for #/ references.
🔄 Upstream Reconcile (v2.3.2) & Codex Review FeedbackRebased and synced with latest upstream Addressed Review Findings:
Clean mergeable status verified across all test suites. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a522bc4c5
ℹ️ 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 (!('type' in record) && !COMBINATOR_KEYS.some((key) => key in record)) { | ||
| const enumValues = record['enum']; | ||
| if (Array.isArray(enumValues) && enumValues.length > 0) { |
There was a problem hiding this comment.
When a valid property combines an explicit type with a mixed enum, such as type: 'string', enum: ['auto', 1], this guard skips the only enum-splitting path. The later normalizeKimiToolSchema logic at packages/agent-core-v2/src/human/llm-kimi/schema.ts:277-286 catches the mixed-type inference error and leaves the shape untouched, so MFJS still receives the unsupported mixed enum and rejects the Kimi request; filter the enum against the explicit type or split it while retaining the parent constraint.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit dada01971. In normalizeProperty, when an explicit type is present beside an enum, enum values are filtered against type (or the enum is removed if empty), ensuring downstream MFJS normalizers do not receive invalid mixed enums.
| if (ARRAY_KEYWORDS.some((k) => k in node)) return 'array'; | ||
| if (STRING_KEYWORDS.some((k) => k in node)) return 'string'; | ||
| if (NUMERIC_KEYWORDS.some((k) => k in node)) return 'number'; | ||
| return 'string'; |
There was a problem hiding this comment.
Fresh evidence after the dependency-key fix is that a typeless property using only unevaluatedProperties (or unevaluatedItems/additionalItems) still falls through here and becomes a string. The downstream slot table at packages/agent-core-v2/src/human/llm-kimi/schema.ts:44-54 classifies these as object/array structure but cannot override an already-added type, so local validation and providers now reject valid {} or [] arguments accepted by the advertised schema and the pre-change path; include all of those container keywords in structural inference.
AGENTS.md reference: AGENTS.md:L112-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit dada01971. Added unevaluatedProperties to OBJECT_KEYWORDS and unevaluatedItems / additionalItems to ARRAY_KEYWORDS in schema-sanitize.ts so typeless schemas using unevaluated container keywords infer proper object and array types.
| mergedProps[key] = value; | ||
| } | ||
| } | ||
| merged['properties'] = mergedProps; |
There was a problem hiding this comment.
Fresh evidence after the new sibling-property merge is a 2020-12 $ref whose target has properties: {a: ...}, additionalProperties: false while the sibling has properties: {b: ...}, required: ['b']: the original conjunction is unsatisfiable because the target forbids b, but assigning the unioned property map here makes b recognized and the sanitized validator accepts {b: ...}. Keep the target and sibling object assertions as separate conjunctive schemas instead of flattening interacting keywords.
AGENTS.md reference: AGENTS.md:L112-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit dada01971. In mergeRefSiblings, when either target or sibling specifies closed-object semantics (additionalProperties: false) alongside properties, the assertions are retained as separate conjunctive schemas via allOf: [target, rest] instead of flattening and inadvertently widening property acceptance.
…nstraints, and handle mixed typed enums
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dada019713
ℹ️ 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".
| delete record['items']; | ||
| } else if (items.length === 1) { | ||
| record['items'] = items[0] as Json; | ||
| } else { | ||
| record['items'] = { anyOf: items }; |
There was a problem hiding this comment.
When a draft-07 MCP schema uses positional items, such as [string, number] with additionalItems: false, replacing the tuple with items.anyOf makes every position accept either type and makes additionalItems inapplicable. Because connectAndDiscoverTools installs this schema for every provider, a value such as [1, "x", 1] now passes local validation even though the server accepts at most two values in the declared order; retain a separate exact validator or encode the order and length constraints.
AGENTS.md reference: AGENTS.md:L110-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 5a4d9f3ac. When items is an array and additionalItems: false is specified, maxItems is clamped to items.length before additionalItems is stripped, ensuring the length constraint is strictly preserved.
| ) { | ||
| continue; | ||
| } | ||
| cutoffProps[pKey] = structuredClone(pVal); |
There was a problem hiding this comment.
For a nullable or mutually recursive edge, the new cutoff only omits a property whose top-level $ref exactly equals ref; a property such as anyOf: [{ $ref: '#/$defs/Node' }, { type: 'null' }] is cloned here with that nested reference intact. The root definition buckets are then deleted, so local AJV compilation sees a missing reference and rejects calls to these recursive MCP tools; inspect descendants before cloning or retain definitions needed by unresolved cutoff references.
AGENTS.md reference: AGENTS.md:L110-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 5a4d9f3ac. Added stripOrReplaceRef which deeply inspects targetProps and replaces any nested $ref targeting the active cutoff with a circular reference placeholder { type: "object", description: "Circular reference" } rather than cloning it verbatim, eliminating dangling refs when definition buckets are stripped.
| } else { | ||
| delete record['enum']; | ||
| } |
There was a problem hiding this comment.
When the explicit type has no overlap with the enum, for example type: 'string', enum: [1], the original schema is valid but accepts no value. Deleting the enum instead turns it into an unrestricted string schema, so non-Kimi MCP users can have a model-generated value pass local AJV and reach a server that rejects it; preserve an unsatisfiable constraint rather than removing the enum.
AGENTS.md reference: AGENTS.md:L110-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in commit 5a4d9f3ac. When an explicit type has zero overlap with enum, record[\"enum\"] is assigned the empty array [] rather than deleting the keyword, keeping the contradictory constraint unsatisfiable under AJV.
…ed cutoff refs, and map tuple additionalItems false
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a4d9f3ac8
ℹ️ 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.
Whenever a non-Kimi model uses an MCP tool with a valid typeless field such as payload: { description: 'arbitrary JSON' }, this stores the MFJS-normalized schema on the shared Tool; normalizeProperty adds type: 'string', and packages/agent-core-v2/src/agent/toolExecutor/toolExecutorService.ts:794-805 compiles that altered schema for every execution. Object or numeric payloads that the server advertised as valid are therefore rejected locally, and other providers also see the narrowed schema, so preserve the original schema for validation/non-Kimi providers and apply this compatibility pass during Kimi serialization instead.
AGENTS.md reference: AGENTS.md:L112-L112
Useful? React with 👍 / 👎.
| const resolved = traverse(root) as JsonRecord; | ||
| delete resolved['$defs']; | ||
| delete resolved['definitions']; |
There was a problem hiding this comment.
When a server returns unused $defs forming an acyclic repeated-reference DAG (for example each D_i has two references to D_(i-1)), traverse(root) expands every branch even though these buckets are immediately deleted. Checked on Node 24, an 18-definition schema blocked synchronously for about 4.8 seconds while returning a 17-byte schema; larger inputs grow exponentially and the connection timeout cannot interrupt this event-loop block. Skip traversing discarded buckets and resolve referenced targets lazily with memoization or a budget.
AGENTS.md reference: AGENTS.md:L112-L112
Useful? React with 👍 / 👎.
| for (const [key, value] of Object.entries(record)) { | ||
| result[key] = traverse(value, activeRefs); |
There was a problem hiding this comment.
$ref keys inside literal values as data
When a property's const, enum, default, or example contains an object such as { "$ref": "#/ordinary-data" }, this context-free recursion enters that instance value and treats its key as a schema reference. A valid const then either throws during tool discovery and fails the whole MCP server or is replaced with the referenced schema object; traverse only JSON Schema child-schema slots rather than every nested JSON object.
AGENTS.md reference: AGENTS.md:L112-L112
Useful? React with 👍 / 👎.
| 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 draft-2019/2020 input uses $anchor: 'thing' and $ref: '#thing', this condition sends the plain-name fragment to resolvePointer, which strips #, looks for a root property named thing, and throws for the normal nested-anchor case. The exception makes connectAndDiscoverTools mark the entire MCP server failed, so only # and #/... should take the JSON Pointer path unless anchor resolution is implemented.
AGENTS.md reference: AGENTS.md:L112-L112
Useful? React with 👍 / 👎.
| (target['additionalProperties'] === false && ('properties' in rest || 'patternProperties' in rest)) || | ||
| (rest['additionalProperties'] === false && ('properties' in target || 'patternProperties' in target)) |
There was a problem hiding this comment.
When a $ref target has additionalProperties: { type: 'string' } and its sibling introduces required property b with type: 'number', the original conjunction requires b to satisfy both schemas and is unsatisfiable. Fresh evidence in the final tree is that this separation guard handles only literal false; the later merged property map makes b a known property and exempts it from the target's additional-property schema, so { b: 1 } passes locally although the server rejects it. Keep these assertions in separate allOf branches whenever additionalProperties is schema-valued too.
AGENTS.md reference: AGENTS.md:L112-L112
Useful? React with 👍 / 👎.
| allowedTypes.size === 1 | ||
| ) { | ||
| b['type'] = [...allowedTypes][0]!; |
There was a problem hiding this comment.
For type: ['string', 'null'] with anyOf: [{ minimum: 0 }], the final tree's allowedTypes.size === 1 gate leaves the branch without the parent constraint; the parent type is then deleted and recursive normalization infers type: 'number'. The result accepts nonnegative numbers while rejecting every string/null value accepted by the original schema; fresh evidence beyond the earlier boolean-branch case is this structural object branch taking the size gate, so encode the full allowed type set before dropping the parent.
AGENTS.md reference: AGENTS.md:L112-L112
Useful? React with 👍 / 👎.
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