From 865ca6c9f3ef6bad3a7477fae472b7d4a5dbde03 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Mon, 29 Jun 2026 12:19:15 +0000 Subject: [PATCH 1/2] fix(codemod): keep request() result schemas the call still needs The schema-param-removal rule dropped the result-schema argument from every MCP-schema-bearing request()/callTool() call. For request() that is only safe when the method is provably a literal string: schema-less v2 request() enforces the spec result schema for known methods and resolves undefined for unknown ones, so stripping the schema from a dynamic-method call site (the proxy/forwarder shape) silently changed forwarding semantics and broke verbatim relays. request() now keeps its schema unless the request object literal carries a literal method (string or no-substitution template, unwrapping as-const/ satisfies/parens, and not followed by a spread that could override it), and the generic passthrough ResultSchema is always kept. callTool() is unchanged: its v2 signature has no schema parameter, so the argument always goes. --- .changeset/lucky-poets-refuse.md | 5 + .../v1-to-v2/transforms/schemaParamRemoval.ts | 34 +++++++ .../transforms/schemaParamRemoval.test.ts | 98 +++++++++++++++++++ 3 files changed, 137 insertions(+) create mode 100644 .changeset/lucky-poets-refuse.md diff --git a/.changeset/lucky-poets-refuse.md b/.changeset/lucky-poets-refuse.md new file mode 100644 index 0000000000..23f7d6702a --- /dev/null +++ b/.changeset/lucky-poets-refuse.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/codemod': patch +--- + +Keep the result-schema argument on `request()` calls whose method is not a literal string, and keep the generic passthrough `ResultSchema` even for literal methods. Schema-less v2 `request()` enforces the spec result schema for known methods and resolves `undefined` for unknown ones, so dropping the schema from a proxy/forwarder call site (`request({ method, params }, ResultSchema)`) silently changed forwarding semantics. `callTool()` is unaffected — v2 `callTool()` has no schema parameter. diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/schemaParamRemoval.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/schemaParamRemoval.ts index 66143b2534..6d391c9f59 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/schemaParamRemoval.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/schemaParamRemoval.ts @@ -6,6 +6,30 @@ import { hasMcpImports, isImportedFromMcp, removeUnusedImport, resolveOriginalIm const TARGET_METHODS = new Set(['request', 'callTool']); +// `request()` keeps its result-schema parameter in v2 (custom methods, passthrough +// forwarding); only calls whose method the codemod can PROVE is a literal string may +// safely lose it. A dynamic method — `request({ method, params }, schema)` in a +// proxy/forwarder — must keep the schema: schema-less v2 `request()` enforces the +// spec result schema for known methods and resolves `undefined` for unknown ones. +function hasLiteralMethod(arg: Node): boolean { + if (!Node.isObjectLiteralExpression(arg)) return false; + const prop = arg.getProperty('method'); + if (!prop || !Node.isPropertyAssignment(prop)) return false; + // A spread after the `method` property can override it at runtime — not provably literal. + const props = arg.getProperties(); + const spreadAfterMethod = props.slice(props.indexOf(prop) + 1).some(p => Node.isSpreadAssignment(p)); + if (spreadAfterMethod) return false; + let initializer = prop.getInitializer(); + // Unwrap `'tools/call' as const` / `satisfies` / parenthesized forms. + while ( + initializer !== undefined && + (Node.isAsExpression(initializer) || Node.isSatisfiesExpression(initializer) || Node.isParenthesizedExpression(initializer)) + ) { + initializer = initializer.getExpression(); + } + return initializer !== undefined && (Node.isStringLiteral(initializer) || Node.isNoSubstitutionTemplateLiteral(initializer)); +} + export const schemaParamRemovalTransform: Transform = { name: 'Schema parameter removal', id: 'schema-params', @@ -50,6 +74,16 @@ export const schemaParamRemovalTransform: Transform = { if (!originalName.endsWith('Schema')) continue; if (!isImportedFromMcp(sourceFile, schemaName)) continue; + // v2 `callTool()` has no schema parameter at all, so the argument always goes. + // v2 `request()` still accepts one — only drop it when the method is a literal + // (see hasLiteralMethod) and the schema is method-specific: the generic + // `ResultSchema` is the v1 passthrough idiom, and dropping it would silently + // switch the call from passthrough to spec-schema enforcement. + if (methodName === 'request') { + if (!hasLiteralMethod(args[0]!)) continue; + if (originalName === 'ResultSchema') continue; + } + call.removeArgument(1); changesCount++; diff --git a/packages/codemod/test/v1-to-v2/transforms/schemaParamRemoval.test.ts b/packages/codemod/test/v1-to-v2/transforms/schemaParamRemoval.test.ts index 5f65411b5e..70bb079ea8 100644 --- a/packages/codemod/test/v1-to-v2/transforms/schemaParamRemoval.test.ts +++ b/packages/codemod/test/v1-to-v2/transforms/schemaParamRemoval.test.ts @@ -166,4 +166,102 @@ describe('schema-param-removal transform', () => { const result = applyTransform(input); expect(result).toContain('undefined'); }); + + it('keeps the schema (and its import) when request() has a shorthand dynamic method', () => { + // The proxy/forwarder shape: schema-less v2 request() would enforce spec result + // schemas and resolve `undefined` for unknown methods — the schema must survive. + const input = [ + `import { ResultSchema } from '@modelcontextprotocol/sdk/types.js';`, + `const result = await upstream.request({ method, params }, ResultSchema);`, + '' + ].join('\n'); + const result = applyTransform(input); + expect(result).toContain('request({ method, params }, ResultSchema)'); + expect(result).toMatch(/import.*ResultSchema/); + }); + + it('keeps the schema when request() has a variable method property', () => { + const input = [ + `import { CallToolResultSchema } from '@modelcontextprotocol/sdk/types.js';`, + `const result = await client.request({ method: m, params: {} }, CallToolResultSchema);`, + '' + ].join('\n'); + const result = applyTransform(input); + expect(result).toContain('CallToolResultSchema)'); + }); + + it('keeps the schema when the request object is not an object literal', () => { + const input = [ + `import { CallToolResultSchema } from '@modelcontextprotocol/sdk/types.js';`, + `const result = await relay.request(inbound, CallToolResultSchema);`, + '' + ].join('\n'); + const result = applyTransform(input); + expect(result).toContain('relay.request(inbound, CallToolResultSchema)'); + }); + + it('keeps the generic passthrough ResultSchema even for a literal spec method', () => { + // Dropping it would switch the call from passthrough validation to spec-schema + // enforcement — a silent semantic change. + const input = [ + `import { ResultSchema } from '@modelcontextprotocol/sdk/types.js';`, + `const result = await client.request({ method: 'ping' }, ResultSchema);`, + '' + ].join('\n'); + const result = applyTransform(input); + expect(result).toContain("request({ method: 'ping' }, ResultSchema)"); + }); + + it('keeps an aliased passthrough ResultSchema', () => { + const input = [ + `import { ResultSchema as RS } from '@modelcontextprotocol/sdk/types.js';`, + `const result = await client.request({ method: 'tools/call' }, RS);`, + '' + ].join('\n'); + const result = applyTransform(input); + expect(result).toContain("request({ method: 'tools/call' }, RS)"); + }); + + it('still removes the schema for a no-substitution template-literal method', () => { + const input = [ + `import { CallToolResultSchema } from '@modelcontextprotocol/sdk/types.js';`, + 'const result = await client.request({ method: `tools/call`, params: {} }, CallToolResultSchema);', + '' + ].join('\n'); + const result = applyTransform(input); + expect(result).not.toContain('CallToolResultSchema)'); + }); + + it('still removes the schema when the literal method is wrapped in `as const`', () => { + const input = [ + `import { CallToolResultSchema } from '@modelcontextprotocol/sdk/types.js';`, + `const result = await client.request({ method: 'tools/call' as const, params: {} }, CallToolResultSchema);`, + '' + ].join('\n'); + const result = applyTransform(input); + expect(result).not.toContain('CallToolResultSchema)'); + }); + + it('keeps the schema when a spread follows the method property', () => { + // `{ method: 'tools/call', ...rest }` — rest can override method at runtime. + const input = [ + `import { CallToolResultSchema } from '@modelcontextprotocol/sdk/types.js';`, + `const result = await client.request({ method: 'tools/call', ...rest }, CallToolResultSchema);`, + '' + ].join('\n'); + const result = applyTransform(input); + expect(result).toContain('CallToolResultSchema)'); + }); + + it('still removes the schema from callTool() with a variable params object', () => { + // v2 callTool() has no schema parameter — the argument goes regardless of shape. + const input = [ + `import { CallToolResultSchema } from '@modelcontextprotocol/sdk/types.js';`, + `const result = await client.callTool(callParams, CallToolResultSchema);`, + '' + ].join('\n'); + const result = applyTransform(input); + expect(result).toContain('client.callTool(callParams)'); + expect(result).not.toContain('CallToolResultSchema'); + }); }); From 07906555b7c585ab2f2e8a606154f1944b67e064 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Mon, 29 Jun 2026 13:11:54 +0000 Subject: [PATCH 2/2] fix(codemod): only drop request() schemas for literal spec methods Review follow-up: a literal method proves nothing about spec-ness, and schema-less v2 request() throws a TypeError for non-spec methods, so a custom-method call site like request({ method: 'acme/search' }, Schema) lost a schema the call still needs. The drop is now gated on the literal appearing in the spec request-method set (SCHEMA_TO_METHOD values). Also corrects the changeset and comments: unknown methods fail loudly with a TypeError, they do not resolve undefined. --- .changeset/lucky-poets-refuse.md | 2 +- .../v1-to-v2/transforms/schemaParamRemoval.ts | 35 ++++++++++++------- .../transforms/schemaParamRemoval.test.ts | 11 ++++++ 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/.changeset/lucky-poets-refuse.md b/.changeset/lucky-poets-refuse.md index 23f7d6702a..92f0052858 100644 --- a/.changeset/lucky-poets-refuse.md +++ b/.changeset/lucky-poets-refuse.md @@ -2,4 +2,4 @@ '@modelcontextprotocol/codemod': patch --- -Keep the result-schema argument on `request()` calls whose method is not a literal string, and keep the generic passthrough `ResultSchema` even for literal methods. Schema-less v2 `request()` enforces the spec result schema for known methods and resolves `undefined` for unknown ones, so dropping the schema from a proxy/forwarder call site (`request({ method, params }, ResultSchema)`) silently changed forwarding semantics. `callTool()` is unaffected — v2 `callTool()` has no schema parameter. +Keep the result-schema argument on `request()` calls unless the method is a literal spec method, and keep the generic passthrough `ResultSchema` even then. Schema-less v2 `request()` enforces the spec result schema for spec methods and throws a `TypeError` for non-spec methods, so dropping the schema from a dynamic-method call site (the proxy/forwarder shape, `request({ method, params }, ResultSchema)`) or from a custom-method call broke the call. `callTool()` is unaffected — v2 `callTool()` has no schema parameter. diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/schemaParamRemoval.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/schemaParamRemoval.ts index 6d391c9f59..c8fd439eb3 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/schemaParamRemoval.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/schemaParamRemoval.ts @@ -3,22 +3,27 @@ import { Node, SyntaxKind } from 'ts-morph'; import type { Transform, TransformContext, TransformResult } from '../../../types'; import { hasMcpImports, isImportedFromMcp, removeUnusedImport, resolveOriginalImportName } from '../../../utils/importUtils'; +import { SCHEMA_TO_METHOD } from '../mappings/schemaToMethodMap'; const TARGET_METHODS = new Set(['request', 'callTool']); +/** Spec request methods — the only methods whose result schema v2 resolves by name. */ +const SPEC_REQUEST_METHODS: ReadonlySet = new Set(Object.values(SCHEMA_TO_METHOD)); + // `request()` keeps its result-schema parameter in v2 (custom methods, passthrough -// forwarding); only calls whose method the codemod can PROVE is a literal string may -// safely lose it. A dynamic method — `request({ method, params }, schema)` in a -// proxy/forwarder — must keep the schema: schema-less v2 `request()` enforces the -// spec result schema for known methods and resolves `undefined` for unknown ones. -function hasLiteralMethod(arg: Node): boolean { - if (!Node.isObjectLiteralExpression(arg)) return false; +// forwarding); only calls whose method the codemod can PROVE is a literal spec method +// may safely lose it. Schema-less v2 `request()` enforces the spec result schema for +// spec methods and throws a TypeError for non-spec methods, so dropping the schema +// from a dynamic-method call (`request({ method, params }, schema)` in a +// proxy/forwarder) or from a custom-method call breaks the call site. +function literalMethodOf(arg: Node): string | undefined { + if (!Node.isObjectLiteralExpression(arg)) return undefined; const prop = arg.getProperty('method'); - if (!prop || !Node.isPropertyAssignment(prop)) return false; + if (!prop || !Node.isPropertyAssignment(prop)) return undefined; // A spread after the `method` property can override it at runtime — not provably literal. const props = arg.getProperties(); const spreadAfterMethod = props.slice(props.indexOf(prop) + 1).some(p => Node.isSpreadAssignment(p)); - if (spreadAfterMethod) return false; + if (spreadAfterMethod) return undefined; let initializer = prop.getInitializer(); // Unwrap `'tools/call' as const` / `satisfies` / parenthesized forms. while ( @@ -27,7 +32,9 @@ function hasLiteralMethod(arg: Node): boolean { ) { initializer = initializer.getExpression(); } - return initializer !== undefined && (Node.isStringLiteral(initializer) || Node.isNoSubstitutionTemplateLiteral(initializer)); + if (initializer === undefined) return undefined; + if (!Node.isStringLiteral(initializer) && !Node.isNoSubstitutionTemplateLiteral(initializer)) return undefined; + return initializer.getLiteralValue(); } export const schemaParamRemovalTransform: Transform = { @@ -76,11 +83,13 @@ export const schemaParamRemovalTransform: Transform = { // v2 `callTool()` has no schema parameter at all, so the argument always goes. // v2 `request()` still accepts one — only drop it when the method is a literal - // (see hasLiteralMethod) and the schema is method-specific: the generic - // `ResultSchema` is the v1 passthrough idiom, and dropping it would silently - // switch the call from passthrough to spec-schema enforcement. + // SPEC method (schema-less request() throws a TypeError for anything else) and + // the schema is method-specific: the generic `ResultSchema` is the v1 + // passthrough idiom, and dropping it would silently switch the call from + // passthrough to spec-schema enforcement. if (methodName === 'request') { - if (!hasLiteralMethod(args[0]!)) continue; + const literal = literalMethodOf(args[0]!); + if (literal === undefined || !SPEC_REQUEST_METHODS.has(literal)) continue; if (originalName === 'ResultSchema') continue; } diff --git a/packages/codemod/test/v1-to-v2/transforms/schemaParamRemoval.test.ts b/packages/codemod/test/v1-to-v2/transforms/schemaParamRemoval.test.ts index 70bb079ea8..93a52ecaea 100644 --- a/packages/codemod/test/v1-to-v2/transforms/schemaParamRemoval.test.ts +++ b/packages/codemod/test/v1-to-v2/transforms/schemaParamRemoval.test.ts @@ -222,6 +222,17 @@ describe('schema-param-removal transform', () => { expect(result).toContain("request({ method: 'tools/call' }, RS)"); }); + it('keeps the schema for a literal NON-spec method (custom methods need their schema)', () => { + // Schema-less v2 request() throws a TypeError for non-spec methods. + const input = [ + `import { CallToolResultSchema } from '@modelcontextprotocol/sdk/types.js';`, + `const result = await client.request({ method: 'acme/search', params: {} }, CallToolResultSchema);`, + '' + ].join('\n'); + const result = applyTransform(input); + expect(result).toContain("request({ method: 'acme/search', params: {} }, CallToolResultSchema)"); + }); + it('still removes the schema for a no-substitution template-literal method', () => { const input = [ `import { CallToolResultSchema } from '@modelcontextprotocol/sdk/types.js';`,