From dc5055f52dfe2d517ffdd88089ffbf31f0c363c8 Mon Sep 17 00:00:00 2001 From: Goga Koreli Date: Mon, 30 Mar 2026 23:20:38 -0700 Subject: [PATCH 1/4] fix: inline local $ref in tool inputSchema for LLM consumption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tool schemas containing $ref cause LLM failures across multiple MCP clients. LLMs cannot resolve JSON Schema $ref pointers — they serialize referenced parameters as strings instead of objects. While $ref was always possible in tool schemas, #1460's switch from zod-to-json-schema to z.toJSONSchema() widened the blast radius: registered types (z.globalRegistry) and recursive types (z.lazy) now produce $ref on common patterns that previously rarely triggered it. Adds dereferenceLocalRefs() which inlines all local $ref pointers, wired into standardSchemaToJsonSchema() so all tool schemas are self-contained and LLM-consumable regardless of schema library. Recursive schemas throw at tools/list time — they cannot be represented without $ref and LLMs cannot handle them. Fixes: #1562 --- .changeset/inline-ref-in-tool-schema.md | 5 + packages/core/src/util/schema.ts | 98 +++++++++++ packages/core/src/util/standardSchema.ts | 4 +- packages/core/test/schema.test.ts | 131 ++++++++++++++ test/integration/test/server/mcp.test.ts | 213 +++++++++++++++++++++++ 5 files changed, 450 insertions(+), 1 deletion(-) create mode 100644 .changeset/inline-ref-in-tool-schema.md create mode 100644 packages/core/test/schema.test.ts diff --git a/.changeset/inline-ref-in-tool-schema.md b/.changeset/inline-ref-in-tool-schema.md new file mode 100644 index 000000000..56ed20b50 --- /dev/null +++ b/.changeset/inline-ref-in-tool-schema.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/core': patch +--- + +Inline local `$ref` pointers in tool `inputSchema` so schemas are self-contained and LLM-consumable. LLMs cannot resolve JSON Schema `$ref` — they serialize referenced parameters as strings instead of objects. Recursive schemas now throw at `tools/list` time instead of silently degrading. diff --git a/packages/core/src/util/schema.ts b/packages/core/src/util/schema.ts index 9676674b8..f330dd330 100644 --- a/packages/core/src/util/schema.ts +++ b/packages/core/src/util/schema.ts @@ -20,6 +20,104 @@ export type AnyObjectSchema = z.core.$ZodObject; */ export type SchemaOutput = z.output; +/** + * Resolves all local `$ref` pointers in a JSON Schema by inlining the + * referenced definitions. Removes `$defs`/`definitions` from the output. + * + * - Caches resolved defs to avoid redundant work with diamond references + * (A→B→D, A→C→D — D is resolved once and reused). + * - Throws on cycles — recursive schemas cannot be represented without `$ref` + * and LLMs cannot handle them. Fail loud so the developer knows to + * restructure their schema. + * - Preserves sibling keywords alongside `$ref` per JSON Schema 2020-12 + * (e.g. `{ "$ref": "...", "description": "override" }`). + * + * @internal Exported for testing only. + */ +export function dereferenceLocalRefs(schema: Record): Record { + const defs: Record = + (schema['$defs'] as Record) ?? (schema['definitions'] as Record) ?? {}; + + // Cache resolved defs to avoid redundant traversal on diamond references. + // Note: cached values are shared by reference. This is safe because schemas + // are treated as immutable after generation. If a consumer mutates a schema, + // they'd need to deep-clone it first regardless. + const cache = new Map(); + + function resolve(node: unknown, stack: Set): unknown { + if (node === null || typeof node !== 'object') return node; + if (Array.isArray(node)) return node.map(item => resolve(item, stack)); + + const obj = node as Record; + + if (typeof obj['$ref'] === 'string') { + const ref = obj['$ref'] as string; + + // Collect sibling keywords (JSON Schema 2020-12 allows keywords alongside $ref) + const { $ref: _ref, ...siblings } = obj; + void _ref; + const hasSiblings = Object.keys(siblings).length > 0; + + let resolved: unknown; + + if (ref === '#') { + // Self-referencing root + if (stack.has(ref)) { + throw new Error( + 'Recursive schema detected: the root schema references itself. ' + + 'MCP tool schemas cannot contain cycles because LLMs cannot resolve $ref pointers.' + ); + } + const { $defs: _defs, definitions: _definitions, ...rest } = schema; + void _defs; + void _definitions; + stack.add(ref); + resolved = resolve(rest, stack); + stack.delete(ref); + } else { + // Local definition: #/$defs/Name or #/definitions/Name + const match = ref.match(/^#\/(?:\$defs|definitions)\/(.+)$/); + if (!match) return obj; // Non-local $ref — leave as-is + + const defName = match[1]!; + const def = defs[defName]; + if (def === undefined) return obj; // Unknown def — leave as-is + + if (stack.has(defName)) { + throw new Error( + `Recursive schema detected: cycle through definition "${defName}". ` + + 'MCP tool schemas cannot contain cycles because LLMs cannot resolve $ref pointers.' + ); + } + + if (cache.has(defName)) { + resolved = cache.get(defName); + } else { + stack.add(defName); + resolved = resolve(def, stack); + stack.delete(defName); + cache.set(defName, resolved); + } + } + + // Merge sibling keywords onto the resolved schema + if (hasSiblings && resolved !== null && typeof resolved === 'object' && !Array.isArray(resolved)) { + return { ...(resolved as Record), ...siblings }; + } + return resolved; + } + + const result: Record = {}; + for (const [key, value] of Object.entries(obj)) { + if (key === '$defs' || key === 'definitions') continue; + result[key] = resolve(value, stack); + } + return result; + } + + return resolve(schema, new Set()) as Record; +} + /** * Parses data against a Zod schema (synchronous). * Returns a discriminated union with success/error. diff --git a/packages/core/src/util/standardSchema.ts b/packages/core/src/util/standardSchema.ts index 9817dc39a..b6998ca6c 100644 --- a/packages/core/src/util/standardSchema.ts +++ b/packages/core/src/util/standardSchema.ts @@ -6,6 +6,8 @@ /* eslint-disable @typescript-eslint/no-namespace */ +import { dereferenceLocalRefs } from './schema.js'; + // Standard Schema interfaces — vendored from https://standardschema.dev (spec v1, Jan 2025) export interface StandardTypedV1 { @@ -156,7 +158,7 @@ export function standardSchemaToJsonSchema(schema: StandardJSONSchemaV1, io: 'in `Wrap your schema in z.object({...}) or equivalent.` ); } - return { type: 'object', ...result }; + return dereferenceLocalRefs({ type: 'object', ...result }); } // Validation diff --git a/packages/core/test/schema.test.ts b/packages/core/test/schema.test.ts new file mode 100644 index 000000000..c6cd09bee --- /dev/null +++ b/packages/core/test/schema.test.ts @@ -0,0 +1,131 @@ +// Unit tests for dereferenceLocalRefs +// Tests raw JSON Schema edge cases independent of the server/client pipeline. +// See: https://github.com/anthropics/claude-code/issues/18260 + +import { describe, expect, test } from 'vitest'; + +import { dereferenceLocalRefs } from '../src/util/schema.js'; + +describe('dereferenceLocalRefs', () => { + test('schema with no $ref passes through unchanged', () => { + const schema = { + type: 'object', + properties: { name: { type: 'string' }, age: { type: 'number' } } + }; + const result = dereferenceLocalRefs(schema); + expect(result).toEqual(schema); + }); + + test('local $ref is inlined and $defs removed', () => { + const schema = { + type: 'object', + properties: { + primary: { $ref: '#/$defs/Tag' }, + secondary: { $ref: '#/$defs/Tag' } + }, + $defs: { + Tag: { type: 'object', properties: { label: { type: 'string' } } } + } + }; + const result = dereferenceLocalRefs(schema); + expect(JSON.stringify(result)).not.toContain('$ref'); + expect(JSON.stringify(result)).not.toContain('$defs'); + expect(result['properties']).toMatchObject({ + primary: { type: 'object', properties: { label: { type: 'string' } } }, + secondary: { type: 'object', properties: { label: { type: 'string' } } } + }); + }); + + test('diamond references resolve correctly', () => { + const schema = { + type: 'object', + properties: { + b: { type: 'object', properties: { inner: { $ref: '#/$defs/Shared' } } }, + c: { type: 'object', properties: { inner: { $ref: '#/$defs/Shared' } } } + }, + $defs: { + Shared: { type: 'object', properties: { x: { type: 'number' } } } + } + }; + const result = dereferenceLocalRefs(schema); + expect(JSON.stringify(result)).not.toContain('$ref'); + const props = result['properties'] as Record>; + const bInner = (props['b']!['properties'] as Record)['inner']; + const cInner = (props['c']!['properties'] as Record)['inner']; + expect(bInner).toMatchObject({ type: 'object', properties: { x: { type: 'number' } } }); + expect(cInner).toMatchObject({ type: 'object', properties: { x: { type: 'number' } } }); + }); + + test('non-existent $def reference is left as-is', () => { + const schema = { + type: 'object', + properties: { + broken: { $ref: '#/$defs/DoesNotExist' } + }, + $defs: {} + }; + const result = dereferenceLocalRefs(schema); + expect((result['properties'] as Record)['broken']).toEqual({ $ref: '#/$defs/DoesNotExist' }); + }); + + test('external $ref is left as-is', () => { + const schema = { + type: 'object', + properties: { + ext: { $ref: 'https://example.com/schemas/Foo.json' } + } + }; + const result = dereferenceLocalRefs(schema); + expect((result['properties'] as Record)['ext']).toEqual({ + $ref: 'https://example.com/schemas/Foo.json' + }); + }); + + test('sibling keywords alongside $ref are preserved', () => { + const schema = { + type: 'object', + properties: { + addr: { $ref: '#/$defs/Address', description: 'Home address' } + }, + $defs: { + Address: { type: 'object', properties: { street: { type: 'string' } } } + } + }; + const result = dereferenceLocalRefs(schema); + const addr = (result['properties'] as Record)['addr'] as Record; + expect(addr['type']).toBe('object'); + expect(addr['properties']).toEqual({ street: { type: 'string' } }); + expect(addr['description']).toBe('Home address'); + }); + + test('recursive $ref through $defs throws', () => { + const schema = { + type: 'object', + properties: { + value: { type: 'string' }, + children: { type: 'array', items: { $ref: '#/$defs/TreeNode' } } + }, + $defs: { + TreeNode: { + type: 'object', + properties: { + value: { type: 'string' }, + children: { type: 'array', items: { $ref: '#/$defs/TreeNode' } } + } + } + } + }; + expect(() => dereferenceLocalRefs(schema)).toThrow(/Recursive schema detected/); + }); + + test('$ref: "#" root self-reference throws', () => { + const schema = { + type: 'object', + properties: { + name: { type: 'string' }, + child: { $ref: '#' } + } + }; + expect(() => dereferenceLocalRefs(schema)).toThrow(/Recursive schema detected/); + }); +}); diff --git a/test/integration/test/server/mcp.test.ts b/test/integration/test/server/mcp.test.ts index 92af09744..dd1dc49f6 100644 --- a/test/integration/test/server/mcp.test.ts +++ b/test/integration/test/server/mcp.test.ts @@ -5169,6 +5169,219 @@ describe('Zod v4', () => { }); }); + // https://github.com/anthropics/claude-code/issues/18260 + // Tool inputSchema must not contain $ref — LLMs cannot resolve JSON Schema + // references and will stringify object parameters instead of passing objects. + describe('Tool inputSchema should not contain $ref', () => { + // Track schemas registered in globalRegistry so we can clean up + const registeredSchemas: z.core.$ZodType[] = []; + afterEach(() => { + for (const schema of registeredSchemas) { + z.globalRegistry.remove(schema); + } + registeredSchemas.length = 0; + }); + + function registerInGlobal(schema: T, meta: { id: string }): T { + z.globalRegistry.add(schema, meta); + registeredSchemas.push(schema); + return schema; + } + + test('registered types should be inlined in schema and callable', async () => { + const server = new McpServer({ name: 'test', version: '1.0.0' }); + const client = new Client({ name: 'test-client', version: '1.0.0' }); + + const Address = registerInGlobal(z.object({ street: z.string(), city: z.string() }), { id: 'Address' }); + + server.registerTool('update-address', { inputSchema: z.object({ home: Address, work: Address }) }, async args => ({ + content: [{ type: 'text' as const, text: `home: ${args.home.city}, work: ${args.work.city}` }] + })); + + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + await client.connect(clientTransport); + + // Schema invariant: no $ref in the schema sent to clients + const { tools } = await client.request({ method: 'tools/list' }); + const schema = tools[0]!.inputSchema; + expect(JSON.stringify(schema)).not.toContain('$ref'); + expect(JSON.stringify(schema)).not.toContain('$defs'); + expect(schema.properties!['home']).toMatchObject({ + type: 'object', + properties: { street: { type: 'string' }, city: { type: 'string' } } + }); + + // Runtime invariant: callTool with object args should succeed + const result = await client.callTool({ + name: 'update-address', + arguments: { + home: { street: '123 Main St', city: 'Springfield' }, + work: { street: '456 Oak Ave', city: 'Shelbyville' } + } + }); + expect(result.content).toEqual([{ type: 'text', text: 'home: Springfield, work: Shelbyville' }]); + }); + + test('discriminatedUnion with registered types should be inlined and callable', async () => { + const server = new McpServer({ name: 'test', version: '1.0.0' }); + const client = new Client({ name: 'test-client', version: '1.0.0' }); + + const CreateOp = z.object({ type: z.literal('create'), file_text: z.string() }); + const AppendOp = z.object({ type: z.literal('append'), new_str: z.string() }); + registerInGlobal(CreateOp, { id: 'CreateOp' }); + registerInGlobal(AppendOp, { id: 'AppendOp' }); + + server.registerTool( + 'write-file', + { + inputSchema: z.object({ + path: z.string(), + operation: z.discriminatedUnion('type', [CreateOp, AppendOp]) + }) + }, + async args => ({ + content: [{ type: 'text' as const, text: `${args.operation.type}: ${args.path}` }] + }) + ); + + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + await client.connect(clientTransport); + + // Schema invariant: oneOf variants should be inline objects, not $ref + const { tools } = await client.request({ method: 'tools/list' }); + const schema = tools[0]!.inputSchema; + expect(JSON.stringify(schema)).not.toContain('$ref'); + expect(JSON.stringify(schema)).not.toContain('$defs'); + const operation = schema.properties!['operation'] as Record; + const variants = (operation['oneOf'] ?? operation['anyOf']) as Array>; + expect(variants).toBeDefined(); + expect(variants.length).toBe(2); + expect(variants[0]).toHaveProperty('type', 'object'); + expect(variants[1]).toHaveProperty('type', 'object'); + + // Runtime invariant: callTool with object operation should succeed + // This is exactly the case that fails when LLMs stringify $ref params + const result = await client.callTool({ + name: 'write-file', + arguments: { + path: '/tmp/test.md', + operation: { type: 'create', file_text: 'hello world' } + } + }); + expect(result.content).toEqual([{ type: 'text', text: 'create: /tmp/test.md' }]); + }); + + test('mixed $ref and inline params in same tool should both work', async () => { + const server = new McpServer({ name: 'test', version: '1.0.0' }); + const client = new Client({ name: 'test-client', version: '1.0.0' }); + + // Reproduces the original Notion MCP bug: one param uses $ref, + // another uses inline type: object — only the $ref param gets stringified + const ParentRequest = z.object({ database_id: z.string() }); + registerInGlobal(ParentRequest, { id: 'ParentRequest' }); + + server.registerTool( + 'create-page', + { + inputSchema: z.object({ + parent: ParentRequest, + properties: z.object({ + title: z.string() + }) + }) + }, + async args => ({ + content: [{ type: 'text' as const, text: `db: ${args.parent.database_id}, title: ${args.properties.title}` }] + }) + ); + + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + await client.connect(clientTransport); + + const { tools } = await client.request({ method: 'tools/list' }); + const schema = tools[0]!.inputSchema; + + // Both params should be inline objects + expect(JSON.stringify(schema)).not.toContain('$ref'); + expect(schema.properties!['parent']).toMatchObject({ type: 'object' }); + expect(schema.properties!['properties']).toMatchObject({ type: 'object' }); + + const result = await client.callTool({ + name: 'create-page', + arguments: { + parent: { database_id: '2275ad9e-1234' }, + properties: { title: 'Test Page' } + } + }); + expect(result.content).toEqual([{ type: 'text', text: 'db: 2275ad9e-1234, title: Test Page' }]); + }); + + test('$ref pointing to oneOf union should be inlined', async () => { + const server = new McpServer({ name: 'test', version: '1.0.0' }); + const client = new Client({ name: 'test-client', version: '1.0.0' }); + + // Notion-style: $defs contains a oneOf union referenced via $ref + const ParentRequest = z.union([z.object({ database_id: z.string() }), z.object({ page_id: z.string() })]); + registerInGlobal(ParentRequest, { id: 'ParentRequest' }); + + server.registerTool( + 'create-item', + { + inputSchema: z.object({ parent: ParentRequest }) + }, + async _args => ({ + content: [{ type: 'text' as const, text: 'created' }] + }) + ); + + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + await client.connect(clientTransport); + + const { tools } = await client.request({ method: 'tools/list' }); + const schema = tools[0]!.inputSchema; + expect(JSON.stringify(schema)).not.toContain('$ref'); + expect(JSON.stringify(schema)).not.toContain('$defs'); + + // The inlined parent should have the union variants directly + const parent = schema.properties!['parent'] as Record; + const variants = (parent['oneOf'] ?? parent['anyOf']) as Array>; + expect(variants).toBeDefined(); + expect(variants.length).toBe(2); + + const result = await client.callTool({ + name: 'create-item', + arguments: { parent: { database_id: 'abc-123' } } + }); + expect(result.content).toEqual([{ type: 'text', text: 'created' }]); + }); + + test('recursive types should throw on tools/list', async () => { + const server = new McpServer({ name: 'test', version: '1.0.0' }); + const client = new Client({ name: 'test-client', version: '1.0.0' }); + + const TreeNode: z.ZodType = z.object({ + value: z.string(), + children: z.lazy(() => z.array(TreeNode)) + }); + + server.registerTool('process-tree', { inputSchema: z.object({ root: TreeNode }) }, async () => ({ + content: [{ type: 'text' as const, text: 'processed' }] + })); + + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + await client.connect(clientTransport); + + // Recursive schemas should fail at tools/list time so the developer + // knows immediately that the schema needs restructuring + await expect(client.request({ method: 'tools/list' })).rejects.toThrow(); + }); + }); + describe('Tools with transformation schemas', () => { test('should support z.preprocess() schemas', async () => { const server = new McpServer({ From 469805cd2b8cdf05392cd7dd638d9212357fc584 Mon Sep 17 00:00:00 2001 From: Goga Koreli Date: Wed, 1 Apr 2026 10:39:04 -0700 Subject: [PATCH 2/4] test: add unit tests for real Zod v4 $ref + sibling patterns Add 3 tests covering real-world Zod v4 output patterns: - $ref with multiple metadata siblings (.meta() pattern) - $ref with default value sibling (.default() pattern) - $def referencing another $def (nested registered types) These verify the sibling merge path handles all real schema generator output correctly. Exhaustive cross-library testing (Zod v4, ArkType, Valibot) confirmed no generator produces $ref with siblings containing nested $ref. --- packages/core/test/schema.test.ts | 68 +++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/packages/core/test/schema.test.ts b/packages/core/test/schema.test.ts index c6cd09bee..6dbaf85b4 100644 --- a/packages/core/test/schema.test.ts +++ b/packages/core/test/schema.test.ts @@ -128,4 +128,72 @@ describe('dereferenceLocalRefs', () => { }; expect(() => dereferenceLocalRefs(schema)).toThrow(/Recursive schema detected/); }); + + // The following tests cover real Zod v4 output patterns where $ref appears + // with sibling keywords. Zod only produces metadata siblings (description, + // title, default, etc.) — never schema nodes containing nested $ref. + // These prove the sibling merge path handles all real-world scenarios. + + test('$ref with multiple metadata siblings (Zod .meta() on registered type)', () => { + const schema = { + type: 'object', + properties: { + home: { $ref: '#/$defs/Address', title: 'Home', deprecated: true } + }, + $defs: { + Address: { type: 'object', properties: { street: { type: 'string' } } } + } + }; + const result = dereferenceLocalRefs(schema); + const home = (result['properties'] as Record>)['home']!; + expect(home['type']).toBe('object'); + expect(home['title']).toBe('Home'); + expect(home['deprecated']).toBe(true); + expect(JSON.stringify(result)).not.toContain('$ref'); + }); + + test('$ref with default value sibling (Zod .default() on registered type)', () => { + const schema = { + type: 'object', + properties: { + home: { $ref: '#/$defs/Address', default: { street: '123 Main' } } + }, + $defs: { + Address: { type: 'object', properties: { street: { type: 'string' } } } + } + }; + const result = dereferenceLocalRefs(schema); + const home = (result['properties'] as Record>)['home']!; + expect(home['type']).toBe('object'); + expect(home['default']).toEqual({ street: '123 Main' }); + expect(JSON.stringify(result)).not.toContain('$ref'); + }); + + test('$def referencing another $def (nested registered types)', () => { + const schema = { + type: 'object', + properties: { + employer: { $ref: '#/$defs/Company', description: 'The company' } + }, + $defs: { + Address: { type: 'object', properties: { street: { type: 'string' } } }, + Company: { + type: 'object', + properties: { + name: { type: 'string' }, + hq: { $ref: '#/$defs/Address' } + } + } + } + }; + const result = dereferenceLocalRefs(schema); + expect(JSON.stringify(result)).not.toContain('$ref'); + expect(JSON.stringify(result)).not.toContain('$defs'); + const employer = (result['properties'] as Record>)['employer']!; + expect(employer['description']).toBe('The company'); + expect(employer['type']).toBe('object'); + const hq = (employer['properties'] as Record>)['hq']!; + expect(hq['type']).toBe('object'); + expect(hq['properties']).toEqual({ street: { type: 'string' } }); + }); }); From ccf03967ac80a46ecffe8401584b12b5705ffe79 Mon Sep 17 00:00:00 2001 From: Goga Koreli Date: Wed, 1 Apr 2026 11:58:51 -0700 Subject: [PATCH 3/4] fix: resolve $ref siblings defensively, preserve property names `definitions`/`$defs` Address review feedback from PR #1563: 1. Defensive: resolve sibling values through resolve() before merging onto the resolved $ref schema. No known generator triggers this (Zod/ArkType/Valibot only produce metadata siblings), but it makes the sibling-merge and object-traversal paths consistent. 2. Bug fix: only strip $defs/definitions keys at the root schema level. Previously the filter fired at every depth, silently dropping any property named 'definitions' or '$defs' from nested objects. Tests added for both fixes. --- packages/core/src/util/schema.ts | 5 +-- packages/core/test/schema.test.ts | 51 +++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/packages/core/src/util/schema.ts b/packages/core/src/util/schema.ts index f330dd330..4b44a66ba 100644 --- a/packages/core/src/util/schema.ts +++ b/packages/core/src/util/schema.ts @@ -102,14 +102,15 @@ export function dereferenceLocalRefs(schema: Record): Record), ...siblings }; + const resolvedSiblings = Object.fromEntries(Object.entries(siblings).map(([k, v]) => [k, resolve(v, stack)])); + return { ...(resolved as Record), ...resolvedSiblings }; } return resolved; } const result: Record = {}; for (const [key, value] of Object.entries(obj)) { - if (key === '$defs' || key === 'definitions') continue; + if (obj === schema && (key === '$defs' || key === 'definitions')) continue; result[key] = resolve(value, stack); } return result; diff --git a/packages/core/test/schema.test.ts b/packages/core/test/schema.test.ts index 6dbaf85b4..d198cbc85 100644 --- a/packages/core/test/schema.test.ts +++ b/packages/core/test/schema.test.ts @@ -196,4 +196,55 @@ describe('dereferenceLocalRefs', () => { expect(hq['type']).toBe('object'); expect(hq['properties']).toEqual({ street: { type: 'string' } }); }); + + // Defensive: hand-crafted synthetic schema — no known schema generator (Zod v4, + // ArkType, Valibot) produces $ref with a sibling containing nested $ref. + // Generators put $ref inside allOf/anyOf/oneOf array elements (resolved by + // array recursion), not as siblings of $ref on the same node. + // See: https://github.com/modelcontextprotocol/typescript-sdk/pull/1563#discussion_r3022304127 + test('$ref siblings containing nested $ref are resolved (defensive)', () => { + const schema = { + type: 'object', + properties: { x: { $ref: '#/$defs/Outer' } }, + $defs: { + Outer: { $ref: '#/$defs/Inner', allOf: [{ $ref: '#/$defs/Mixin' }] }, + Inner: { type: 'object' }, + Mixin: { title: 'mixin' } + } + }; + const result = dereferenceLocalRefs(schema); + expect(JSON.stringify(result)).not.toContain('$ref'); + expect(JSON.stringify(result)).not.toContain('$defs'); + const x = (result['properties'] as Record>)['x']!; + expect(x['type']).toBe('object'); + expect(x['allOf']).toEqual([{ title: 'mixin' }]); + }); + + test('property named "definitions" is not stripped from nested objects', () => { + const schema = { + type: 'object', + properties: { + items: { type: 'array', items: { type: 'string' } }, + definitions: { type: 'array', items: { type: 'string' } } + }, + required: ['items', 'definitions'] + }; + const result = dereferenceLocalRefs(schema); + const props = result['properties'] as Record; + expect(props['definitions']).toEqual({ type: 'array', items: { type: 'string' } }); + expect(props['items']).toEqual({ type: 'array', items: { type: 'string' } }); + }); + + test('property named "$defs" is not stripped from nested objects', () => { + const schema = { + type: 'object', + properties: { + name: { type: 'string' }, + $defs: { type: 'object', properties: { x: { type: 'number' } } } + } + }; + const result = dereferenceLocalRefs(schema); + const props = result['properties'] as Record; + expect(props['$defs']).toEqual({ type: 'object', properties: { x: { type: 'number' } } }); + }); }); From b20f749d65c3796afba1ee58ef0eb3bdea7400e4 Mon Sep 17 00:00:00 2001 From: Goga Koreli Date: Thu, 2 Apr 2026 12:20:24 -0700 Subject: [PATCH 4/4] feat: graceful degradation for cyclic schemas, code quality tightening - Cyclic $ref left in place with $defs preserved (no throw) - Non-cyclic refs still fully inlined - Single-pass cycle tracking via cyclicDefs Set - resolve() -> inlineRefs() with JSDoc - Regex -> startsWith + slice - Preserved $defs use cached resolved versions (fixes dangling $ref bug) - Tests rewritten as declarative toEqual on full expected output - Added multi-hop cycle test with shuffled $defs order - ADR updated with review traceability and test philosophy --- packages/core/src/util/schema.ts | 138 ++++++------ packages/core/test/schema.test.ts | 267 +++++++++++++---------- test/integration/test/server/mcp.test.ts | 11 +- 3 files changed, 239 insertions(+), 177 deletions(-) diff --git a/packages/core/src/util/schema.ts b/packages/core/src/util/schema.ts index 4b44a66ba..32bc6889d 100644 --- a/packages/core/src/util/schema.ts +++ b/packages/core/src/util/schema.ts @@ -22,101 +22,115 @@ export type SchemaOutput = z.output; /** * Resolves all local `$ref` pointers in a JSON Schema by inlining the - * referenced definitions. Removes `$defs`/`definitions` from the output. + * referenced definitions. * * - Caches resolved defs to avoid redundant work with diamond references * (A→B→D, A→C→D — D is resolved once and reused). - * - Throws on cycles — recursive schemas cannot be represented without `$ref` - * and LLMs cannot handle them. Fail loud so the developer knows to - * restructure their schema. + * - Gracefully handles cycles — cyclic `$ref` are left in place with their + * `$defs` entries preserved. Non-cyclic refs in the same schema are still + * fully inlined. This avoids breaking existing servers that have recursive + * schemas which work (degraded) today. * - Preserves sibling keywords alongside `$ref` per JSON Schema 2020-12 * (e.g. `{ "$ref": "...", "description": "override" }`). * * @internal Exported for testing only. */ export function dereferenceLocalRefs(schema: Record): Record { - const defs: Record = - (schema['$defs'] as Record) ?? (schema['definitions'] as Record) ?? {}; - - // Cache resolved defs to avoid redundant traversal on diamond references. - // Note: cached values are shared by reference. This is safe because schemas - // are treated as immutable after generation. If a consumer mutates a schema, - // they'd need to deep-clone it first regardless. - const cache = new Map(); - - function resolve(node: unknown, stack: Set): unknown { + // "$defs" is the standard keyword since JSON Schema 2019-09. + // See: https://json-schema.org/draft/2020-12/json-schema-core#section-8.2.4 + // "definitions" is the legacy equivalent from drafts 04–07. + // See: https://json-schema.org/draft-07/json-schema-validation#section-9 + // If both exist (malformed schema), "$defs" takes precedence. + const defsKey = '$defs' in schema ? '$defs' : 'definitions' in schema ? 'definitions' : undefined; + const defs: Record = defsKey ? (schema[defsKey] as Record) : {}; + + // No definitions container — nothing to inline. + // Note: $ref: "#" (root self-reference) is intentionally not handled — no schema + // library produces it, no other MCP SDK handles it, and it's always cyclic. + if (!defsKey) return schema; + + // Cache resolved defs to avoid redundant traversal on diamond references + // (A→B→D, A→C→D — D is resolved once and reused). Cached values are shared + // by reference, which is safe because schemas are immutable after generation. + const resolvedDefs = new Map(); + // Def names where a cycle was detected — these $ref are left in place + // and their $defs entries must be preserved in the output. + const cyclicDefs = new Set(); + + /** + * Recursively inlines `$ref` pointers in a JSON Schema node by replacing + * them with the referenced definition content. + * + * @param node - The current schema node being traversed. + * @param stack - Def names currently being inlined (ancestor chain). If a + * def is encountered while already on the stack, it's a cycle — the + * `$ref` is left in place and the def name is added to `cyclicDefs`. + */ + function inlineRefs(node: unknown, stack: Set): unknown { if (node === null || typeof node !== 'object') return node; - if (Array.isArray(node)) return node.map(item => resolve(item, stack)); + if (Array.isArray(node)) return node.map(item => inlineRefs(item, stack)); const obj = node as Record; - if (typeof obj['$ref'] === 'string') { - const ref = obj['$ref'] as string; - - // Collect sibling keywords (JSON Schema 2020-12 allows keywords alongside $ref) - const { $ref: _ref, ...siblings } = obj; - void _ref; + // JSON Schema 2020-12 allows keywords alongside $ref (e.g. description, default). + // Destructure to get the ref target and any sibling keywords to merge later. + const { $ref: ref, ...siblings } = obj; + if (typeof ref === 'string') { const hasSiblings = Object.keys(siblings).length > 0; let resolved: unknown; - if (ref === '#') { - // Self-referencing root - if (stack.has(ref)) { - throw new Error( - 'Recursive schema detected: the root schema references itself. ' + - 'MCP tool schemas cannot contain cycles because LLMs cannot resolve $ref pointers.' - ); - } - const { $defs: _defs, definitions: _definitions, ...rest } = schema; - void _defs; - void _definitions; - stack.add(ref); - resolved = resolve(rest, stack); - stack.delete(ref); + // Local definition reference: #/$defs/Name or #/definitions/Name + const prefix = `#/${defsKey}/`; + if (!ref.startsWith(prefix)) return obj; // Non-local $ref (external URL, etc.) — leave as-is + + const defName = ref.slice(prefix.length); + const def = defs[defName]; + if (def === undefined) return obj; // Unknown def — leave as-is + if (stack.has(defName)) { + cyclicDefs.add(defName); + return obj; // Cycle — leave $ref in place + } + + if (resolvedDefs.has(defName)) { + resolved = resolvedDefs.get(defName); } else { - // Local definition: #/$defs/Name or #/definitions/Name - const match = ref.match(/^#\/(?:\$defs|definitions)\/(.+)$/); - if (!match) return obj; // Non-local $ref — leave as-is - - const defName = match[1]!; - const def = defs[defName]; - if (def === undefined) return obj; // Unknown def — leave as-is - - if (stack.has(defName)) { - throw new Error( - `Recursive schema detected: cycle through definition "${defName}". ` + - 'MCP tool schemas cannot contain cycles because LLMs cannot resolve $ref pointers.' - ); - } - - if (cache.has(defName)) { - resolved = cache.get(defName); - } else { - stack.add(defName); - resolved = resolve(def, stack); - stack.delete(defName); - cache.set(defName, resolved); - } + stack.add(defName); + resolved = inlineRefs(def, stack); + stack.delete(defName); + resolvedDefs.set(defName, resolved); } - // Merge sibling keywords onto the resolved schema + // Merge sibling keywords onto the resolved definition if (hasSiblings && resolved !== null && typeof resolved === 'object' && !Array.isArray(resolved)) { - const resolvedSiblings = Object.fromEntries(Object.entries(siblings).map(([k, v]) => [k, resolve(v, stack)])); + const resolvedSiblings = Object.fromEntries(Object.entries(siblings).map(([k, v]) => [k, inlineRefs(v, stack)])); return { ...(resolved as Record), ...resolvedSiblings }; } return resolved; } + // Regular object — recurse into values, skipping root-level $defs container const result: Record = {}; for (const [key, value] of Object.entries(obj)) { if (obj === schema && (key === '$defs' || key === 'definitions')) continue; - result[key] = resolve(value, stack); + result[key] = inlineRefs(value, stack); } return result; } - return resolve(schema, new Set()) as Record; + const resolved = inlineRefs(schema, new Set()) as Record; + + // Re-attach $defs only for cyclic definitions, using their resolved/cached + // versions so that any non-cyclic refs inside them are already inlined. + if (defsKey && cyclicDefs.size > 0) { + const prunedDefs: Record = {}; + for (const name of cyclicDefs) { + prunedDefs[name] = resolvedDefs.get(name) ?? defs[name]; + } + resolved[defsKey] = prunedDefs; + } + + return resolved; } /** diff --git a/packages/core/test/schema.test.ts b/packages/core/test/schema.test.ts index d198cbc85..c2c737578 100644 --- a/packages/core/test/schema.test.ts +++ b/packages/core/test/schema.test.ts @@ -27,12 +27,12 @@ describe('dereferenceLocalRefs', () => { Tag: { type: 'object', properties: { label: { type: 'string' } } } } }; - const result = dereferenceLocalRefs(schema); - expect(JSON.stringify(result)).not.toContain('$ref'); - expect(JSON.stringify(result)).not.toContain('$defs'); - expect(result['properties']).toMatchObject({ - primary: { type: 'object', properties: { label: { type: 'string' } } }, - secondary: { type: 'object', properties: { label: { type: 'string' } } } + expect(dereferenceLocalRefs(schema)).toEqual({ + type: 'object', + properties: { + primary: { type: 'object', properties: { label: { type: 'string' } } }, + secondary: { type: 'object', properties: { label: { type: 'string' } } } + } }); }); @@ -47,126 +47,109 @@ describe('dereferenceLocalRefs', () => { Shared: { type: 'object', properties: { x: { type: 'number' } } } } }; - const result = dereferenceLocalRefs(schema); - expect(JSON.stringify(result)).not.toContain('$ref'); - const props = result['properties'] as Record>; - const bInner = (props['b']!['properties'] as Record)['inner']; - const cInner = (props['c']!['properties'] as Record)['inner']; - expect(bInner).toMatchObject({ type: 'object', properties: { x: { type: 'number' } } }); - expect(cInner).toMatchObject({ type: 'object', properties: { x: { type: 'number' } } }); + expect(dereferenceLocalRefs(schema)).toEqual({ + type: 'object', + properties: { + b: { type: 'object', properties: { inner: { type: 'object', properties: { x: { type: 'number' } } } } }, + c: { type: 'object', properties: { inner: { type: 'object', properties: { x: { type: 'number' } } } } } + } + }); }); test('non-existent $def reference is left as-is', () => { const schema = { type: 'object', - properties: { - broken: { $ref: '#/$defs/DoesNotExist' } - }, + properties: { broken: { $ref: '#/$defs/DoesNotExist' } }, $defs: {} }; - const result = dereferenceLocalRefs(schema); - expect((result['properties'] as Record)['broken']).toEqual({ $ref: '#/$defs/DoesNotExist' }); + expect(dereferenceLocalRefs(schema)).toEqual({ + type: 'object', + properties: { broken: { $ref: '#/$defs/DoesNotExist' } } + }); }); - test('external $ref is left as-is', () => { + test('external $ref and root self-reference are left as-is', () => { const schema = { type: 'object', properties: { - ext: { $ref: 'https://example.com/schemas/Foo.json' } + ext: { $ref: 'https://example.com/schemas/Foo.json' }, + self: { $ref: '#' } } }; const result = dereferenceLocalRefs(schema); - expect((result['properties'] as Record)['ext']).toEqual({ - $ref: 'https://example.com/schemas/Foo.json' - }); + expect(result).toEqual(schema); }); test('sibling keywords alongside $ref are preserved', () => { const schema = { type: 'object', properties: { - addr: { $ref: '#/$defs/Address', description: 'Home address' } + addr: { $ref: '#/$defs/Address', description: 'Home address', title: 'Home', default: { street: '123 Main' } } }, $defs: { Address: { type: 'object', properties: { street: { type: 'string' } } } } }; - const result = dereferenceLocalRefs(schema); - const addr = (result['properties'] as Record)['addr'] as Record; - expect(addr['type']).toBe('object'); - expect(addr['properties']).toEqual({ street: { type: 'string' } }); - expect(addr['description']).toBe('Home address'); + expect(dereferenceLocalRefs(schema)).toEqual({ + type: 'object', + properties: { + addr: { + type: 'object', + properties: { street: { type: 'string' } }, + description: 'Home address', + title: 'Home', + default: { street: '123 Main' } + } + } + }); }); - test('recursive $ref through $defs throws', () => { + test('mixed cyclic and non-cyclic refs: non-cyclic inlined, cyclic preserved', () => { const schema = { type: 'object', properties: { - value: { type: 'string' }, - children: { type: 'array', items: { $ref: '#/$defs/TreeNode' } } + tag: { $ref: '#/$defs/Tag' }, + tree: { $ref: '#/$defs/TreeNode' } }, $defs: { + Tag: { type: 'object', properties: { label: { type: 'string' } } }, TreeNode: { type: 'object', properties: { value: { type: 'string' }, + tag: { $ref: '#/$defs/Tag' }, children: { type: 'array', items: { $ref: '#/$defs/TreeNode' } } } } } }; - expect(() => dereferenceLocalRefs(schema)).toThrow(/Recursive schema detected/); - }); - - test('$ref: "#" root self-reference throws', () => { - const schema = { - type: 'object', - properties: { - name: { type: 'string' }, - child: { $ref: '#' } - } - }; - expect(() => dereferenceLocalRefs(schema)).toThrow(/Recursive schema detected/); - }); - - // The following tests cover real Zod v4 output patterns where $ref appears - // with sibling keywords. Zod only produces metadata siblings (description, - // title, default, etc.) — never schema nodes containing nested $ref. - // These prove the sibling merge path handles all real-world scenarios. - - test('$ref with multiple metadata siblings (Zod .meta() on registered type)', () => { - const schema = { - type: 'object', - properties: { - home: { $ref: '#/$defs/Address', title: 'Home', deprecated: true } - }, - $defs: { - Address: { type: 'object', properties: { street: { type: 'string' } } } - } - }; - const result = dereferenceLocalRefs(schema); - const home = (result['properties'] as Record>)['home']!; - expect(home['type']).toBe('object'); - expect(home['title']).toBe('Home'); - expect(home['deprecated']).toBe(true); - expect(JSON.stringify(result)).not.toContain('$ref'); - }); - - test('$ref with default value sibling (Zod .default() on registered type)', () => { - const schema = { + expect(dereferenceLocalRefs(schema)).toEqual({ type: 'object', properties: { - home: { $ref: '#/$defs/Address', default: { street: '123 Main' } } + // Tag fully inlined + tag: { type: 'object', properties: { label: { type: 'string' } } }, + // TreeNode resolved one level: Tag inlined, self-ref stays + tree: { + type: 'object', + properties: { + value: { type: 'string' }, + tag: { type: 'object', properties: { label: { type: 'string' } } }, + children: { type: 'array', items: { $ref: '#/$defs/TreeNode' } } + } + } }, + // Only cyclic def preserved, with Tag inlined inside it $defs: { - Address: { type: 'object', properties: { street: { type: 'string' } } } + TreeNode: { + type: 'object', + properties: { + value: { type: 'string' }, + tag: { type: 'object', properties: { label: { type: 'string' } } }, + children: { type: 'array', items: { $ref: '#/$defs/TreeNode' } } + } + } } - }; - const result = dereferenceLocalRefs(schema); - const home = (result['properties'] as Record>)['home']!; - expect(home['type']).toBe('object'); - expect(home['default']).toEqual({ street: '123 Main' }); - expect(JSON.stringify(result)).not.toContain('$ref'); + }); }); test('$def referencing another $def (nested registered types)', () => { @@ -186,21 +169,23 @@ describe('dereferenceLocalRefs', () => { } } }; - const result = dereferenceLocalRefs(schema); - expect(JSON.stringify(result)).not.toContain('$ref'); - expect(JSON.stringify(result)).not.toContain('$defs'); - const employer = (result['properties'] as Record>)['employer']!; - expect(employer['description']).toBe('The company'); - expect(employer['type']).toBe('object'); - const hq = (employer['properties'] as Record>)['hq']!; - expect(hq['type']).toBe('object'); - expect(hq['properties']).toEqual({ street: { type: 'string' } }); + expect(dereferenceLocalRefs(schema)).toEqual({ + type: 'object', + properties: { + employer: { + type: 'object', + properties: { + name: { type: 'string' }, + hq: { type: 'object', properties: { street: { type: 'string' } } } + }, + description: 'The company' + } + } + }); }); // Defensive: hand-crafted synthetic schema — no known schema generator (Zod v4, // ArkType, Valibot) produces $ref with a sibling containing nested $ref. - // Generators put $ref inside allOf/anyOf/oneOf array elements (resolved by - // array recursion), not as siblings of $ref on the same node. // See: https://github.com/modelcontextprotocol/typescript-sdk/pull/1563#discussion_r3022304127 test('$ref siblings containing nested $ref are resolved (defensive)', () => { const schema = { @@ -212,39 +197,99 @@ describe('dereferenceLocalRefs', () => { Mixin: { title: 'mixin' } } }; - const result = dereferenceLocalRefs(schema); - expect(JSON.stringify(result)).not.toContain('$ref'); - expect(JSON.stringify(result)).not.toContain('$defs'); - const x = (result['properties'] as Record>)['x']!; - expect(x['type']).toBe('object'); - expect(x['allOf']).toEqual([{ title: 'mixin' }]); + expect(dereferenceLocalRefs(schema)).toEqual({ + type: 'object', + properties: { x: { type: 'object', allOf: [{ title: 'mixin' }] } } + }); }); - test('property named "definitions" is not stripped from nested objects', () => { + test('multi-hop cycle (A → B → C → A) with non-cyclic sibling: cycle detected, all non-cyclic parts inlined', () => { + // Company → Employee → Department → Company (cycle) + // Department also → Location (non-cyclic sibling) const schema = { + type: 'object', + properties: { company: { $ref: '#/$defs/Company' } }, + $defs: { + // Intentionally unordered — function follows $ref pointers, not declaration order + Location: { + type: 'object', + properties: { city: { type: 'string' } } + }, + Employee: { + type: 'object', + properties: { + name: { type: 'string' }, + department: { $ref: '#/$defs/Department' } + } + }, + Department: { + type: 'object', + properties: { + name: { type: 'string' }, + company: { $ref: '#/$defs/Company' }, + location: { $ref: '#/$defs/Location' } + } + }, + Company: { + type: 'object', + properties: { + name: { type: 'string' }, + employee: { $ref: '#/$defs/Employee' } + } + } + } + }; + const inlinedDepartment = { type: 'object', properties: { - items: { type: 'array', items: { type: 'string' } }, - definitions: { type: 'array', items: { type: 'string' } } - }, - required: ['items', 'definitions'] + name: { type: 'string' }, + company: { $ref: '#/$defs/Company' }, + location: { type: 'object', properties: { city: { type: 'string' } } } + } }; - const result = dereferenceLocalRefs(schema); - const props = result['properties'] as Record; - expect(props['definitions']).toEqual({ type: 'array', items: { type: 'string' } }); - expect(props['items']).toEqual({ type: 'array', items: { type: 'string' } }); + const inlinedEmployee = { + type: 'object', + properties: { + name: { type: 'string' }, + department: inlinedDepartment + } + }; + const inlinedCompany = { + type: 'object', + properties: { + name: { type: 'string' }, + employee: inlinedEmployee + } + }; + expect(dereferenceLocalRefs(schema)).toEqual({ + type: 'object', + properties: { company: inlinedCompany }, + // Only Company preserved — Location fully inlined everywhere including inside $defs + $defs: { Company: inlinedCompany } + }); }); - test('property named "$defs" is not stripped from nested objects', () => { + test('$ref inlined from real $defs while properties named "$defs" and "definitions" are preserved', () => { const schema = { type: 'object', properties: { - name: { type: 'string' }, - $defs: { type: 'object', properties: { x: { type: 'number' } } } + definitions: { type: 'array', items: { type: 'string' } }, + $defs: { type: 'object', properties: { x: { type: 'number' } } }, + tag: { $ref: '#/$defs/Tag' } + }, + required: ['definitions', '$defs'], + $defs: { + Tag: { type: 'object', properties: { label: { type: 'string' } } } } }; - const result = dereferenceLocalRefs(schema); - const props = result['properties'] as Record; - expect(props['$defs']).toEqual({ type: 'object', properties: { x: { type: 'number' } } }); + expect(dereferenceLocalRefs(schema)).toEqual({ + type: 'object', + properties: { + definitions: { type: 'array', items: { type: 'string' } }, + $defs: { type: 'object', properties: { x: { type: 'number' } } }, + tag: { type: 'object', properties: { label: { type: 'string' } } } + }, + required: ['definitions', '$defs'] + }); }); }); diff --git a/test/integration/test/server/mcp.test.ts b/test/integration/test/server/mcp.test.ts index dd1dc49f6..447ec72e7 100644 --- a/test/integration/test/server/mcp.test.ts +++ b/test/integration/test/server/mcp.test.ts @@ -5359,7 +5359,7 @@ describe('Zod v4', () => { expect(result.content).toEqual([{ type: 'text', text: 'created' }]); }); - test('recursive types should throw on tools/list', async () => { + test('recursive types should preserve $ref and $defs on tools/list', async () => { const server = new McpServer({ name: 'test', version: '1.0.0' }); const client = new Client({ name: 'test-client', version: '1.0.0' }); @@ -5376,9 +5376,12 @@ describe('Zod v4', () => { await server.connect(serverTransport); await client.connect(clientTransport); - // Recursive schemas should fail at tools/list time so the developer - // knows immediately that the schema needs restructuring - await expect(client.request({ method: 'tools/list' })).rejects.toThrow(); + // Recursive schemas should NOT throw — cyclic $ref stays in place + const { tools } = await client.request({ method: 'tools/list' }); + const schema = tools[0]!.inputSchema; + // Cyclic $ref is preserved (same as pre-PR behavior — no regression) + expect(JSON.stringify(schema)).toContain('$ref'); + expect(schema.$defs || schema.definitions).toBeDefined(); }); });