feat: add v2 codemod draft#1950
Conversation
|
|
@claude review |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@claude review |
|
@claude review |
|
@claude review |
|
Todo:
|
|
@claude review |
|
@claude review |
|
Codemod follow-up found while migrating Cloudflare MCP: dynamic Zod-field-map input schemas are not wrapped.\n\nRepro shape:\n\n |
…otocol/typescript-sdk into feature/v2-codemode-draft
|
@claude review |
| // Collect identifiers that are actual references to the `extra` parameter | ||
| const identifiers: import('ts-morph').Node[] = []; | ||
| body.forEachDescendant(node => { | ||
| if (!Node.isIdentifier(node) || node.getText() !== EXTRA_PARAM_NAME) return; | ||
| const parent = node.getParent(); | ||
| // Skip property-name positions (e.g., meta.extra, { extra: value }) | ||
| if (parent && Node.isPropertyAccessExpression(parent) && parent.getNameNode() === node) return; | ||
| if (parent && Node.isPropertyAssignment(parent) && parent.getNameNode() === node) return; | ||
| identifiers.push(node); |
There was a problem hiding this comment.
🟡 The identifier-collection skip list in processCallback covers PropertyAccessExpression name-nodes and PropertyAssignment name-nodes, but misses ShorthandPropertyAssignment and BindingElement propertyName — both of which renameAllReferences in astUtils.ts already handles. A handler body like helper({ request, extra }) is rewritten to { request, ctx } (silently changing the object key) instead of { request, extra: ctx }, and const { extra: x } = unrelatedObj becomes const { ctx: x } = unrelatedObj (renaming a property-lookup key on an unrelated object). Add the same two guards already in astUtils.ts.
Extended reasoning...
What the bug is
processCallback in contextTypes.ts collects identifiers that reference the extra parameter via an AST walk (introduced in the latest commit, replacing the earlier regex-based replaceAll per review comment 3278820013). The skip list guarding which identifiers are not references to the parameter is:
if (parent && Node.isPropertyAccessExpression(parent) && parent.getNameNode() === node) return;
if (parent && Node.isPropertyAssignment(parent) && parent.getNameNode() === node) return;
identifiers.push(node);That misses two parent kinds where the identifier is a key, not a value reference:
-
ShorthandPropertyAssignment— in{ extra }, the single identifier is simultaneously the object key and the value reference.Node.isPropertyAssignmentdoes not match a shorthand (they are distinctSyntaxKinds), so the identifier is collected. -
BindingElementpropertyName — inconst { extra: x } = obj, theextraidentifier is a property-lookup key onobj, not a reference to the callback parameter.
Both fall through to the unconditional fallback replacements.push({ node: id, newText: CTX_PARAM_NAME }) and get a plain text rename.
Code path that triggers it
For a handler body like await helper({ request, extra });:
-
body.forEachDescendantvisits theextraidentifier inside{ request, extra }. Its parent is aShorthandPropertyAssignment. - Neither skip guard matches —
Node.isPropertyAccessExpression(parent)andNode.isPropertyAssignment(parent)are both false for that kind. - The identifier is pushed into
identifiers. - In the replacements loop, the parent is not a
PropertyAccessExpression(value position) or aQualifiedName, so it falls through to the plain rename. -
node.replaceWithText('ctx')producesawait helper({ request, ctx });.
The object key is now ctx instead of extra — any consumer reading .extra gets undefined. The correct rewrite is { request, extra: ctx }, exactly what renameAllReferences (astUtils.ts:27-29) does for the identical case.
For const { extra: x } = unrelatedObj, extra is the property-name node of a BindingElement. It is not skipped, so it's renamed to ctx, and the destructuring now reads unrelatedObj.ctx instead of unrelatedObj.extra — a silent semantic change to code that has nothing to do with the context parameter.
Why nothing prevents it
renameAllReferences in astUtils.ts was hardened across multiple review rounds (comments 3133560060, 3136237226, 3138328884) and now has explicit guards for ShorthandPropertyAssignment (line 27: expand to oldName: newName) and BindingElement propertyName (line 23: skip). The new identifier-collection loop in processCallback re-implements its own parent-kind guard list rather than reusing that one, and only carries over two of the established guards. contextTypes.test.ts does cover meta.extra (the PropertyAccessExpression name case) and the { extra: value } property-assignment case implicitly, but has no fixture exercising shorthand { extra } in a call argument or const { extra: ... } = unrelated.
Impact
Both produce silently-wrong output that may still compile (especially against a loosely-typed helper or destructure target) — the worst codemod failure mode. The trigger pattern (passing extra into a shared helper via shorthand object literal, or destructuring an extra property off an unrelated object inside a handler body) is realistic but not the dominant pattern, hence nit severity.
Step-by-step proof
Input:
import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
server.setRequestHandler('tools/call', async (request, extra) => {
await logToService({ request, extra });
return { content: [] };
});-
processCallbackfinds the second param namedextra, no collisions withctx→ proceeds. -
paramDecl.replaceWithText('ctx')renames the parameter declaration. -
body.forEachDescendantvisits theextraidentifier inside{ request, extra }. Parent isShorthandPropertyAssignment. -
Node.isPropertyAccessExpression(parent)→ false.Node.isPropertyAssignment(parent)→ false (distinct kind). →identifiers.push(node). - Replacements loop: parent is not
PropertyAccessExpressionvalue position, notQualifiedName→replacements.push({ node: id, newText: 'ctx' }). - Output:
await logToService({ request, ctx });— object key changed fromextratoctx.
Fix
Add the same two guards already present in renameAllReferences (astUtils.ts):
if (parent && Node.isPropertyAccessExpression(parent) && parent.getNameNode() === node) return;
if (parent && Node.isPropertyAssignment(parent) && parent.getNameNode() === node) return;
if (parent && Node.isBindingElement(parent) && parent.getPropertyNameNode() === node) return;
if (parent && Node.isShorthandPropertyAssignment(parent)) {
// collect, but mark for shorthand expansion
}The simplest implementation: skip BindingElement propertyName entirely, and handle ShorthandPropertyAssignment in the replacements loop by emitting replacements.push({ node: parent, newText: \${EXTRA_PARAM_NAME}: ${CTX_PARAM_NAME}` })— the same expansionastUtils.tsandrenameSymbolsInFactory(mockPaths.ts) already use. A test withhelper({ request, extra })asserting{ request, extra: ctx }and one withconst { extra: x } = obj` asserting it is left untouched would lock both in.
There was a problem hiding this comment.
Thanks — the BindingElement propertyName guard at line 105 is correct (const { extra: x } = unrelatedObj is a property lookup on an unrelated object, so skipping is right). But the ShorthandPropertyAssignment guard at line 104 just returns, dropping the identifier — while line 91 has already renamed the parameter to ctx. For:
async (request, extra) => {
helper({ request, extra });
}the output is now:
async (request, ctx) => {
helper({ request, extra }); // TS2304: Cannot find name 'extra'
}The new test 'does not rename "extra" in shorthand property assignment' asserts toContain('{ request, extra }') — which locks in this broken output, since extra is no longer in scope.
In a shorthand the identifier is both the object key (which must stay extra) and a value reference to the parameter (which is now ctx). The original comment's recommended fix was to expand it — same as renameAllReferences (astUtils.ts:27-29) and renameSymbolsInFactory (mockPaths.ts) already do:
if (parent && Node.isShorthandPropertyAssignment(parent)) {
replacements.push({ node: parent, newText: `${EXTRA_PARAM_NAME}: ${CTX_PARAM_NAME}` });
return; // collected as a replacement, skip the default identifier handling
}(or push the parent into a separate list and emit { extra: ctx } in the replacements loop). Then update the test to assert toContain('{ request, extra: ctx }').
|
@claude review |
| changesCount++; | ||
| }); | ||
|
|
||
| if (changesCount > 0) { | ||
| const errorCodeImportDecl = errorCodeImport.getImportDeclaration(); | ||
| errorCodeImport.remove(); | ||
| if ( | ||
| errorCodeImportDecl.getNamedImports().length === 0 && | ||
| !errorCodeImportDecl.getDefaultImport() && | ||
| !errorCodeImportDecl.getNamespaceImport() | ||
| ) { | ||
| errorCodeImportDecl.remove(); | ||
| } | ||
|
|
||
| const imp = | ||
| sourceFile.getImportDeclarations().find(i => { | ||
| const spec = i.getModuleSpecifierValue(); | ||
| return (spec === '@modelcontextprotocol/client' || spec === '@modelcontextprotocol/server') && !i.isTypeOnly(); | ||
| }) ?? | ||
| sourceFile.getImportDeclarations().find(i => { |
There was a problem hiding this comment.
🟡 Two related gaps in handleErrorCodeSplit: (1) the forEachDescendant walk only rewrites PropertyAccessExpression sites (ErrorCode.X), so type annotations (const code: ErrorCode = …), bare value references (Object.values(ErrorCode)), keyof typeof ErrorCode, and QualifiedName type members survive un-rewritten while the import is removed → guaranteed TS2304; and (2) targetModule is looked up after errorCodeImportDecl.remove(), so when ErrorCode was the only MCP named import in a client-only file the fallback hard-codes @modelcontextprotocol/server (a package the package.json updater never adds for that project). Capture errorCodeImportDecl.getModuleSpecifierValue() before .remove() (matching handleStreamableHTTPError's resolveTargetModule pattern in removedApis.ts), and either also rewrite type-position/bare references or skip removing the import when non-PropertyAccess references remain.
Extended reasoning...
What the bugs are
handleErrorCodeSplit (symbolRenames.ts:46–123) splits ErrorCode.X accesses into ProtocolErrorCode.X / SdkErrorCode.X, then removes the ErrorCode import and adds an import for whichever new symbol(s) were used. Two gaps:
(1) Only PropertyAccessExpression sites are rewritten
The forEachDescendant walk at lines 71–85 has a single guard:
sourceFile.forEachDescendant(node => {
if (!Node.isPropertyAccessExpression(node)) return;
const expr = node.getExpression();
if (!Node.isIdentifier(expr) || expr.getText() !== errorCodeLocalName) return;
// … rewrite expr to ProtocolErrorCode / SdkErrorCode
});Any reference to the local ErrorCode binding that is not the object of a property access is never visited:
- type annotation
const code: ErrorCode = ErrorCode.InvalidParams;— the type-position identifier's parent is aTypeReference, not a PAE - type-position member
let x: ErrorCode.RequestTimeout— parent is aQualifiedName - bare value position
Object.values(ErrorCode)/const All = ErrorCode— the identifier is aCallExpressionargument or initializer, not a PAE object type Codes = keyof typeof ErrorCode— parent is aTypeQuery
Because changesCount > 0 is satisfied by any rewritten PAE in the file, line 88 then unconditionally removes the ErrorCode specifier — leaving these un-rewritten references dangling. In v1, ErrorCode is a TypeScript enum used as both a value and a type, so function handle(code: ErrorCode) { return code === ErrorCode.X; } is canonical, common v1 code.
(2) targetModule is resolved after the import is removed
const errorCodeImportDecl = errorCodeImport.getImportDeclaration();
errorCodeImport.remove();
if (errorCodeImportDecl.getNamedImports().length === 0 && …) {
errorCodeImportDecl.remove();
}
const imp = sourceFile.getImportDeclarations().find(i => {
const spec = i.getModuleSpecifierValue();
return spec === '@modelcontextprotocol/client' || spec === '@modelcontextprotocol/server' …
}) ?? …;
const targetModule = imp?.getModuleSpecifierValue() ?? '@modelcontextprotocol/server';When ErrorCode was the only named import in its declaration and no other exact-match @modelcontextprotocol/client or /server import exists, the declaration is gone before the find() runs, and the ?? '@modelcontextprotocol/server' fallback fires — even when the (already-rewritten by importPaths) original specifier was @modelcontextprotocol/client. The new import { ProtocolErrorCode } from '@modelcontextprotocol/server' is added, but symbolRenames does not report usedPackages, so packageJsonUpdater only installs @modelcontextprotocol/client (tracked by importPaths) → missing-module error.
Why existing code doesn't prevent it
- For (1), nothing later in the pipeline rewrites bare/type-position
ErrorCodereferences —removedApis,mcpServerApi, andcontextTypesdon't touch it. The only diagnostic emitted is the generic "ErrorCode split into ProtocolErrorCode and SdkErrorCode. Verify the migration is correct.", which doesn't mention surviving references. - For (2),
handleStreamableHTTPErrorinremovedApis.tsfaces the same problem and solves it correctly: it capturesconst moduleSpec = foundImportDecl.getModuleSpecifierValue()before.remove()and passes it toresolveTargetModule(sourceFile, originalModule), which falls back tooriginalModule.includes('/client') ? '@modelcontextprotocol/client' : '@modelcontextprotocol/server'.handleErrorCodeSplitwas clearly meant to mirror this and missed the capture.
Step-by-step proof
(1) — type annotation survives, import removed
Input:
import { ErrorCode } from '@modelcontextprotocol/sdk/types.js';
const code: ErrorCode = ErrorCode.InvalidParams;- After
importPaths, the import isfrom '@modelcontextprotocol/server'(or/client). handleErrorCodeSplit:forEachDescendantvisits theErrorCodeidentifier inErrorCode.InvalidParams(PAE object) → rewrites toProtocolErrorCode. The type-positionErrorCodein: ErrorCodehas aTypeReferenceparent → never visited.changesCount === 1→errorCodeImport.remove().
Output:
import { ProtocolErrorCode } from '@modelcontextprotocol/server';
const code: ErrorCode = ProtocolErrorCode.InvalidParams; // TS2304: Cannot find name 'ErrorCode'(2) — wrong fallback module
A client-only file whose only MCP import is import { ErrorCode } from '@modelcontextprotocol/sdk/types.js':
importPathsrewrites it tofrom '@modelcontextprotocol/client'.handleErrorCodeSplitrewrites the body, thenerrorCodeImport.remove()empties the declaration →errorCodeImportDecl.remove().- The
find()overgetImportDeclarations()now returnsundefined(no exact-match/clientor/serverimport remains). targetModule = '@modelcontextprotocol/server'. New import added.packageJsonUpdateradds only@modelcontextprotocol/client(the packageimportPathsreported) →Cannot find module '@modelcontextprotocol/server'.
A second realistic trigger: a file with import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js' plus import { ErrorCode } from '@modelcontextprotocol/sdk/types.js'. After importPaths, the stdio import becomes @modelcontextprotocol/client/stdio (subpath, no exact match), and ErrorCode becomes its own @modelcontextprotocol/client declaration. handleErrorCodeSplit removes the now-empty /client declaration; only the /stdio subpath remains; the exact-match find() returns undefined; fallback to /server.
Impact
Both produce non-compiling output, but the failure is loud — a TS2304 at the dangling reference for (1), and a missing-module error at install/typecheck for (2). The codemod is documented as 80–90% coverage and emits a generic Verify the migration warning. Triggers are realistic but not the dominant pattern (the most common form, ErrorCode.X-only usage in a server file, works correctly). Filing as nit.
Fix
Capture the specifier before removal and use it as the fallback, mirroring handleStreamableHTTPError/resolveTargetModule:
const errorCodeImportDecl = errorCodeImport.getImportDeclaration();
const originalSpec = errorCodeImportDecl.getModuleSpecifierValue();
errorCodeImport.remove();
// … cleanup empty decl …
const imp = sourceFile.getImportDeclarations().find(/* … */);
const targetModule =
imp?.getModuleSpecifierValue() ??
(originalSpec.includes('/client') ? '@modelcontextprotocol/client' : '@modelcontextprotocol/server');For the parent-kind gap, either also rewrite TypeReference / QualifiedName / TypeQuery / bare Identifier references (mapping the type to ProtocolErrorCode | SdkErrorCode where ambiguous), or — simpler — skip removing the ErrorCode import when any non-PropertyAccessExpression reference to the local binding remains, and emit a more specific warning naming the surviving sites. Tests covering const code: ErrorCode = …, Object.values(ErrorCode), and a client-only-with-stdio fixture would lock both in (every existing fixture in symbolRenames.test.ts uses only ErrorCode.X value-position accesses).
There was a problem hiding this comment.
Thanks — gap (2) is fixed: origModule is now captured before .remove() and used as the fallback (imp?.getModuleSpecifierValue() ?? origModule ?? '@modelcontextprotocol/server'), and the find() runs before removal so it can still match the (already-rewritten) declaration. 👍
Gap (1) is still open: the forEachDescendant walk at lines 71-85 still has only the Node.isPropertyAccessExpression(node) guard, so type annotations (const code: ErrorCode = …), bare value references (Object.values(ErrorCode)), keyof typeof ErrorCode, and QualifiedName members survive un-rewritten while line 103 unconditionally removes the ErrorCode specifier when changesCount > 0 — leaving them as TS2304 dangling references. ErrorCode is a v1 enum used as both a value and a type, so function handle(code: ErrorCode) { return code === ErrorCode.X; } is canonical v1 code that still produces broken output.
The simpler half of the original suggestion would close it: before line 103, check whether any non-PropertyAccessExpression reference to errorCodeLocalName remains, and if so skip removing the import (and tighten the diagnostic to name the surviving sites). That's safer than trying to rewrite type-position references, since the type would have to become ProtocolErrorCode | SdkErrorCode in the ambiguous case.
|
@claude review |
| if (parent && Node.isShorthandPropertyAssignment(parent)) return; | ||
| if (parent && Node.isBindingElement(parent) && parent.getPropertyNameNode() === node) return; |
There was a problem hiding this comment.
🔴 The skip guard added for ShorthandPropertyAssignment parents (line ~104) returns at collection time, so extra inside { request, extra } is never collected — but the parameter declaration is still unconditionally renamed to ctx a few lines earlier, leaving extra as an undeclared identifier (TS2304). The new test 'does not rename "extra" in shorthand property assignment' asserts both (request, ctx) and { request, extra } together, locking in the non-compiling output. Per the prior review comment (3278917293), the fix is to expand the shorthand in the replacements loop — replacements.push({ node: parent, newText: 'extra: ctx' }) — the same expansion already used in renameAllReferences (astUtils.ts) and renameSymbolsInFactory (mockPaths.ts).
Extended reasoning...
What the bug is
processCallback in contextTypes.ts first renames the parameter declaration unconditionally:
const paramDecl = extraParam.getNameNode();
paramDecl.replaceWithText(CTX_PARAM_NAME); // (request, extra) → (request, ctx)…and then collects identifiers in the body to rewrite. The collection loop now contains:
if (parent && Node.isShorthandPropertyAssignment(parent)) return; // ← skips at collection timeThat guard treats a shorthand { extra } as if it were a pure key (like { extra: value } or obj.extra), but a ShorthandPropertyAssignment is both the key and a value reference to the local binding. Skipping it means the value-reference half is never updated to follow the renamed parameter.
Code path that triggers it
Input:
server.setRequestHandler('tools/call', async (request, extra) => {
helper({ request, extra });
const s = extra.signal;
return { content: [] };
});processCallbackrenames the parameter declaration:(request, extra)→(request, ctx).body.forEachDescendantvisits theextraidentifier inside{ request, extra }.parentis aShorthandPropertyAssignment→return. Not collected.extra.signalis collected and rewritten toctx.mcpReq.signal. ✓- Replacements are applied; the shorthand is untouched.
Output:
server.setRequestHandler('tools/call', async (request, ctx) => {
helper({ request, extra }); // ← TS2304: Cannot find name 'extra'
const s = ctx.mcpReq.signal;
return { content: [] };
});Why nothing else catches it
renameAllReferences in astUtils.ts correctly handles this exact case by expanding shorthand to oldName: newName, and renameSymbolsInFactory in mockPaths.ts does the same for mock factories — but processCallback re-implements its own parent-kind guard list and uses a bare skip instead of the established expansion. The prior review comment (3278917293) explicitly recommended the expansion form, not a skip; the skip is a misapplication of that guidance.
The new test 'does not rename "extra" in shorthand property assignment' makes the gap worse: it asserts toContain('{ request, extra }') and toContain('(request, ctx)') in the same fixture — i.e., it locks in the non-compiling output as expected behavior, so a future fix would have to first delete the test that defends the bug.
Impact
The output fails typecheck loudly (TS2304 points directly at the dangling extra), so the user will discover it. But the codemod claims success (changesCount is incremented, no diagnostic emitted), and the test suite asserts the broken output is correct, which makes this self-perpetuating. Passing extra to a shared helper via shorthand object literal (helper({ request, extra }), logCall({ request, extra })) is a realistic but secondary pattern in v1 handlers.
Step-by-step proof
- Input body text:
{ helper({ request, extra }); const s = extra.signal; return { content: [] }; }. - Line ~91:
paramDecl.replaceWithText('ctx')— parameter declaration becomesctx. forEachDescendantvisitsextrainhelper({ request, extra }).parentkind isShorthandPropertyAssignment→ guard fires → not collected.forEachDescendantvisitsextrainextra.signal.parentis aPropertyAccessExpressionwithgetExpression() === node(object position, not name node) → collected.- Replacement loop produces
ctx.mcpReq.signalfor the access; nothing for the shorthand. - Final source contains
helper({ request, extra })with noextrain scope.
Fix
In the collection loop, do not skip ShorthandPropertyAssignment parents — instead, in the replacements-building loop, emit the expansion that the rest of the codemod already uses:
// In the replacements loop:
if (parent && Node.isShorthandPropertyAssignment(parent)) {
replacements.push({ node: parent, newText: `${EXTRA_PARAM_NAME}: ${CTX_PARAM_NAME}` });
continue;
}This produces helper({ request, extra: ctx }) — preserving the public object key extra (the helper presumably reads .extra) while pointing it at the renamed local binding. Then update the test to assert toContain('{ request, extra: ctx }') and not.toContain('{ request, extra }'). (Alternatively, factor the shorthand-expansion logic out of renameAllReferences into a shared helper and reuse it here.)
| .find(i => i.getModuleSpecifierValue() === targetModule && !i.isTypeOnly()); | ||
| if (existingImp) { | ||
| const existingNames = new Set(existingImp.getNamedImports().map(n => n.getName())); | ||
| const toAdd = newImports.filter(n => !existingNames.has(n)); | ||
| if (toAdd.length > 0) { | ||
| existingImp.addNamedImports(toAdd); | ||
| } | ||
| } else { | ||
| sourceFile.addImportDeclaration({ | ||
| moduleSpecifier: targetModule, | ||
| namedImports: newImports | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 handleErrorCodeSplit and handleRequestHandlerExtra each locate a merge-target import via .find(i => i.getModuleSpecifierValue() === target && (!)i.isTypeOnly()) with no !i.getNamespaceImport() guard, and then call existingImp.addNamedImports(...) — ts-morph 28 throws InvalidOperationError ('Cannot add a named import to an import declaration that has a namespace import') if the matched declaration is a namespace import, and the runner per-file catch then rolls back the whole file with an opaque 'Transform failed' diagnostic. These inline .find() sites do NOT route through addOrMergeImport (importUtils.ts), so the namespace guard suggested for addOrMergeImport's predicate (comment 3279075847) won't cure them — add the same guard to all three lookups (handleErrorCodeSplit's existingImp, and handleRequestHandlerExtra's existingImp and valueImp).
Extended reasoning...
What the bug is
handleErrorCodeSplit (symbolRenames.ts ~lines 113–123) and handleRequestHandlerExtra (~lines 230–250) each re-implement "find an existing import to merge new named imports into" inline rather than going through addOrMergeImport in importUtils.ts. The lookup predicates are:
handleErrorCodeSplit→.find(i => i.getModuleSpecifierValue() === targetModule && !i.isTypeOnly())followed byexistingImp.addNamedImports(toAdd)(ProtocolErrorCode/SdkErrorCode)handleRequestHandlerExtra→.find(i => i.getModuleSpecifierValue() === target && i.isTypeOnly())then a fallback.find(... && !i.isTypeOnly()), each followed byaddNamedImports([name])(ServerContext/ClientContext)
None of the three predicates exclude namespace imports. In TypeScript's grammar ImportClause.namedBindings is a discriminated union — a declaration can have a NamespaceImport or NamedImports, never both — so ts-morph 28 throws InvalidOperationError: Cannot add a named import to an import declaration that has a namespace import. when addNamedImports() is called on a matched namespace import.
The code path that triggers it
A file that ends up with both a namespace import and a named ErrorCode/RequestHandlerExtra import to the same v2 package. One realistic shape that survives the full pipeline:
import * as types from '@modelcontextprotocol/sdk/types.js';
import type { RequestHandlerExtra } from '@modelcontextprotocol/sdk/shared/protocol.js';
type H = RequestHandlerExtra<ServerRequest, ServerNotification>;importPathsTransform: the namespace import is preserved in place viaimp.setModuleSpecifier('@modelcontextprotocol/server')(thedefaultImport || namespaceImport || hasAliasbranch). The type-onlyRequestHandlerExtraimport is collected and re-added viaaddOrMergeImportwithisTypeOnly===true, so it does not match the namespace import (isTypeOnly()===false) and lands in its own type-only declaration. No throw yet.symbolRenamesTransform→handleRequestHandlerExtra: the type ref is rewritten toServerContext, theRequestHandlerExtraspecifier is removed, and the now-empty type-only declaration is removed.- The
existingImplookup (isTypeOnly()true) finds nothing. The fallbackvalueImplookup (!isTypeOnly()) matches the namespace import —getModuleSpecifierValue() === '@modelcontextprotocol/server'andisTypeOnly() === falseboth hold. valueImp.addNamedImports(['ServerContext'])→ throws.
The same shape applies to handleErrorCodeSplit with ErrorCode in place of RequestHandlerExtra (e.g. an already-v2 file with import * as server from '@modelcontextprotocol/server' plus import { ErrorCode } from '@modelcontextprotocol/server' re-run through --transforms symbols for idempotency).
Why nothing else prevents it, and why the addOrMergeImport fix won't cure these
runner.ts wraps the per-file transform loop in a try/catch that reverts the entire file to its original source text and emits a single error(filePath, 1, 'Transform failed: …') diagnostic. So a hit here doesn't corrupt output — it just discards every successful transform that ran before the throw and gives the user a non-actionable message.
This is the same hazard class flagged in comment 3279075847 for addOrMergeImport() in importUtils.ts, but a distinct code location: handleErrorCodeSplit and handleRequestHandlerExtra re-implement the merge inline and never call addOrMergeImport. (Of the symbolRenames helpers, only handleSchemaInput routes through addOrMergeImport.) Fixing addOrMergeImport's .find() predicate alone leaves these three inline lookups unfixed.
Step-by-step proof
Input:
import * as types from '@modelcontextprotocol/sdk/types.js';
import type { RequestHandlerExtra } from '@modelcontextprotocol/sdk/shared/protocol.js';
type H = RequestHandlerExtra<ServerRequest, ServerNotification>;- After
importPaths:import * as types from '@modelcontextprotocol/server'; import type { RequestHandlerExtra } from '@modelcontextprotocol/server'; … handleRequestHandlerExtrareplaces the type ref withServerContext, removes theRequestHandlerExtraspecifier, removes the now-empty type-only declaration.needsServerContext = true.existingImp = .find(i => spec === '@modelcontextprotocol/server' && i.isTypeOnly())→undefined(only the namespace import remains,isTypeOnly()===false).valueImp = .find(i => spec === '@modelcontextprotocol/server' && !i.isTypeOnly())→ matches the namespace import.valueImp.addNamedImports(['ServerContext'])→ ts-morph throwsInvalidOperationError: Cannot add a named import to an import declaration that has a namespace import.runner.tscatch fires: file reverted to original v1 source,error(filePath, 1, 'Transform failed: Cannot add a named import…')emitted,fileChanges = 0. TheimportPathsrewrite that did succeed is discarded.
Impact
The trigger is uncommon — it requires a namespace import alongside a named ErrorCode/RequestHandlerExtra import that resolve to the same v2 package — and the failure is loud and safe (whole file rolled back, error diagnostic emitted, no corrupted output written). Hence nit severity. But it's a real completeness gap: the same hazard class is already on this PR for addOrMergeImport, and these inline sites need their own fix or the bug reappears the moment that one is fixed.
Fix
Add a !i.getNamespaceImport() guard to each of the three inline .find() calls:
// handleErrorCodeSplit
const existingImp = sourceFile
.getImportDeclarations()
.find(i => i.getModuleSpecifierValue() === targetModule && !i.isTypeOnly() && !i.getNamespaceImport());
// handleRequestHandlerExtra
const existingImp = sourceFile.getImportDeclarations()
.find(i => i.getModuleSpecifierValue() === target && i.isTypeOnly() && !i.getNamespaceImport());
// …
const valueImp = sourceFile.getImportDeclarations()
.find(i => i.getModuleSpecifierValue() === target && !i.isTypeOnly() && !i.getNamespaceImport());Or, cleaner: refactor both helpers to call addOrMergeImport (as handleSchemaInput already does) and put the namespace guard once in addOrMergeImport's predicate. A test like import * as types from '@modelcontextprotocol/server'; import { ErrorCode } from '@modelcontextprotocol/server'; const c = ErrorCode.InvalidParams; run through --transforms symbols would lock both in.
| `Replaced ${localName} with specTypeSchemas.${typeName}. Note: typed as StandardSchemaV1, not ZodType — Zod methods like .safeParse()/.parse() are not available.` | ||
| ) | ||
| ); | ||
| return true; | ||
| } | ||
|
|
||
| if (parent && Node.isPropertyAssignment(parent) && parent.getNameNode() === ref) { | ||
| return false; | ||
| } | ||
|
|
||
| if (parent && Node.isBindingElement(parent) && parent.getPropertyNameNode() === ref) { | ||
| return false; | ||
| } | ||
|
|
||
| if (parent && Node.isPropertyAccessExpression(parent) && parent.getNameNode() === ref) { | ||
| return false; | ||
| } | ||
|
|
||
| // Value position: replace identifier with specTypeSchemas.X | ||
| const line = ref.getStartLineNumber(); | ||
| ref.replaceWithText(`specTypeSchemas.${typeName}`); | ||
| ensureImport(sourceFile, 'specTypeSchemas'); | ||
| diagnostics.push( |
There was a problem hiding this comment.
🟡 handleReference() in specSchemaAccess.ts guards ExportSpecifier, ShorthandPropertyAssignment, PropertyAssignment name, BindingElement propertyName, and PropertyAccessExpression name — but not PropertySignature, MethodDeclaration, MethodSignature, PropertyDeclaration, EnumMember, or get/set accessor name nodes, all of which renameAllReferences in astUtils.ts already guards. For import { ToolSchema } from '@modelcontextprotocol/server'; interface Config { ToolSchema: string; }, the interface property identifier falls through to the value-position rewrite, producing interface Config { specTypeSchemas.Tool: string; } (a syntax error) — ts-morph throws and the runner rolls back the entire file with an opaque diagnostic. Add the same guard list astUtils.ts uses, or factor it into a shared isKeyPositionIdentifier() helper.
Extended reasoning...
What the bug is
handleReference() in specSchemaAccess.ts re-implements its own parent-kind guard list rather than reusing the one in astUtils.ts renameAllReferences(). Through several review rounds it has accumulated guards for ExportSpecifier, ShorthandPropertyAssignment, PropertyAssignment name node, BindingElement property name, and PropertyAccessExpression name node — but it is still missing the rest of the key-position parent kinds that astUtils.ts already handles: PropertySignature, MethodDeclaration, MethodSignature, PropertyDeclaration, EnumMember, GetAccessorDeclaration, and SetAccessorDeclaration.
findNonImportReferences() only filters out ImportSpecifier parents, so an identifier in any of those positions is collected and passed to handleReference(). None of the existing guards match, so it falls through to the value-position default at the end of the function: ref.replaceWithText(\specTypeSchemas.${typeName}`)`.
Why the existing code doesn't prevent it
A PropertySignature (interface member) is a distinct SyntaxKind from PropertyAssignment (object literal entry), so Node.isPropertyAssignment(parent) returns false. Same for class MethodDeclaration/PropertyDeclaration, MethodSignature, EnumMember, and accessor name nodes. The function explicitly checks each kind, so a kind not in the list is silently treated as a value reference.
Step-by-step proof
Input:
import { ToolSchema } from '@modelcontextprotocol/server';
interface Config { ToolSchema: string; }collectSpecSchemaImports()findsToolSchema(it's inSPEC_SCHEMA_NAMES), soschemaImports = { ToolSchema → ToolSchema },typeName = 'Tool'.findNonImportReferences()visits theToolSchemaidentifier insideinterface Config { ToolSchema: string; }. Its parent is aPropertySignature, which is not anImportSpecifier, so it's pushed intorefs.handleReference()walks the guard chain:isTypeofInTypePosition→ false (parent isn'tTypeQuery);isSafeParse*/isParse*→ false (parent isn'tPropertyAccessExpression); the value-position PAE check → false;ExportSpecifier→ false;ShorthandPropertyAssignment→ false;PropertyAssignmentname → false (PropertySignatureis a distinct kind);BindingElementpropertyName → false;PropertyAccessExpressionname → false.- Falls through to the value-position rewrite:
ref.replaceWithText('specTypeSchemas.Tool'). - ts-morph attempts to write
interface Config { specTypeSchemas.Tool: string; }, which is invalid syntax.replaceWithTextthrows a manipulation error. - The runner's per-file catch (
runner.ts) rolls back the entire file to its pre-pipeline state — undoing the successfulimportPaths/symbolRenamesrewrites too — and emits a single opaqueTransform failed: Manipulation error…diagnostic for line 1.
The same failure occurs for class members (class C { ToolSchema = ''; method ToolSchema() {} get ToolSchema() {} }), enum members (enum E { ToolSchema }), and interface method signatures.
Impact
The trigger is contrived — it requires importing a spec-schema name from MCP and declaring an unrelated interface/class/enum member with that exact name in the same file — and the failure is loud and safe (file reverted, error diagnostic emitted, no corrupted output written), hence nit severity. But it's a clear internal inconsistency in this transform: this is the same parent-kind-guard family that has been hardened repeatedly across this PR's review rounds (PropertyAssignment, BindingElement, and PropertyAccessExpression name guards were each added in successive commits), and astUtils.ts already has the complete list.
How to fix
Either add the missing guards alongside the existing ones in handleReference():
if (parent && Node.isPropertySignature(parent) && parent.getNameNode() === ref) return false;
if (parent && Node.isMethodDeclaration(parent) && parent.getNameNode() === ref) return false;
if (parent && Node.isMethodSignature(parent) && parent.getNameNode() === ref) return false;
if (parent && Node.isPropertyDeclaration(parent) && parent.getNameNode() === ref) return false;
if (parent && Node.isEnumMember(parent) && parent.getNameNode() === ref) return false;
if (parent && Node.isGetAccessorDeclaration(parent) && parent.getNameNode() === ref) return false;
if (parent && Node.isSetAccessorDeclaration(parent) && parent.getNameNode() === ref) return false;Or — cleaner, since this is now the third time this skip list has needed re-syncing — factor the key-position checks out of renameAllReferences() in astUtils.ts into a shared isKeyPositionIdentifier(node) helper and reuse it in both places (and in findNonImportReferences()).
| function wrapWithZObject(schemaText: string): string { | ||
| return `z.object(${schemaText})`; | ||
| } | ||
|
|
||
| function maybeWrapSchema(node: Node): string { | ||
| const text = node.getText(); | ||
| if (Node.isObjectLiteralExpression(node)) { | ||
| return wrapWithZObject(text); | ||
| } | ||
| return text; | ||
| } | ||
|
|
||
| function emitWrapDiagnostic(node: Node, sourceFile: SourceFile, call: CallExpression, diagnostics: Diagnostic[]): void { | ||
| if (Node.isObjectLiteralExpression(node)) { | ||
| diagnostics.push( | ||
| info( | ||
| sourceFile.getFilePath(), | ||
| call.getStartLineNumber(), | ||
| 'Raw object literal wrapped with z.object(). Verify that zod (z) is imported in this file.' | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🟡 Two leftover gaps from comment 3278854656: (1) the new fall-through warning in wrapSchemaInConfig() fires for any non-object-literal initializer, including the canonical correct v2 form inputSchema: z.object({ ... }) — already-migrated configs get a spurious 'value is not an object literal' warning on every run. (2) The parallel positional path (emitWrapDiagnostic()/maybeWrapSchema() used by migrateToolCall/migratePromptCall) still emits zero diagnostics for non-object-literal schema args, so server.tool('x', buildInputSchema(), cb) becomes registerTool('x', { inputSchema: buildInputSchema() }, cb) silently — the v1-positional analogue of the Cloudflare MCP repro mattzcarey reported. Add the recommended !initializer.getText().startsWith('z.object(') guard before the warning, and emit the same warning from emitWrapDiagnostic for non-ObjectLiteralExpression nodes.
Extended reasoning...
Gap 1 — spurious warning for already-correct z.object() configs
wrapSchemaInConfig() (mcpServerApi.ts:~209-216) handles the schema property like this:
if (Node.isObjectLiteralExpression(initializer)) {
// wrap with z.object(), emit info, return true
}
diagnostics.push(
warning(..., `${schemaPropertyName} value is not an object literal — verify it is a z.object() schema. ...`)
);
return false;For registerTool('x', { inputSchema: z.object({ name: z.string() }) }, cb), the schemaProp is a regular PropertyAssignment, the initializer is a CallExpression (z.object(...)), not an ObjectLiteralExpression — so it falls straight through to the warning. The warning text — "verify it is a z.object() schema" — is asking the user to verify something that is literally already a z.object() schema. The original comment 3278854656 explicitly recommended guarding with if (!initializer.getText().startsWith('z.object(')); that guard was not added.
This affects every registerTool/registerPrompt call that already uses the v2-correct form (which is also valid v1 code), and it makes the codemod non-diagnostic-idempotent: re-running it on output it just produced ({ name: z.string() } → z.object({ name: z.string() })) flags the codemod's own output as suspect.
The test 'does not double-wrap z.object() in .registerTool() config' only asserts on the rewritten text (not.toContain('z.object(z.object(')) — it never inspects result.diagnostics — so the spurious warning passes CI undetected.
Gap 2 — positional form still silent
emitWrapDiagnostic() / maybeWrapSchema() are the parallel helpers for the positional v1 overloads (server.tool('x', schemaVar, cb), server.prompt('x', schemaVar, cb)). They have the opposite gap:
function maybeWrapSchema(node: Node): string {
const text = node.getText();
if (Node.isObjectLiteralExpression(node)) return wrapWithZObject(text);
return text; // ← raw text, unwrapped, no diagnostic
}
function emitWrapDiagnostic(node: Node, ...): void {
if (Node.isObjectLiteralExpression(node)) {
diagnostics.push(info(...)); // only fires for object literals
}
// ← no else branch; complete no-op for Identifier / CallExpression
}So server.tool('execute', buildInputSchema(), cb) (where buildInputSchema() returns a Zod field map per v1's positional overload contract) becomes server.registerTool('execute', { inputSchema: buildInputSchema() }, cb) with zero diagnostics, and then fails typecheck on v2 with TS2741: Property '~standard' is missing in type 'Record<string, ZodType<...>>' — exactly the failure mattzcarey reported on this PR (2026-05-14) for the config-object form, but on the v1 positional overload that migrateToolCall converts. Comment 3278854656 explicitly asked: "Apply the same treatment in emitWrapDiagnostic/maybeWrapSchema for the positional server.tool(name, schemaVar, cb) form." That follow-up was not done. The new tests added in that round ('emits diagnostic for variable-valued schema in config', 'emits diagnostic for shorthand schema property in config') only cover the config-object form, not the positional form.
Step-by-step proof
Gap 1 — input:
import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
const server = new McpServer({ name: 't', version: '1.0' });
server.registerTool('echo', { inputSchema: z.object({ msg: z.string() }) }, async ({ msg }) => ({ content: [] }));registerToolCallscollects the call →wrapSchemaInConfig(call, 'inputSchema', ...).schemaPropis a regularPropertyAssignment→ passesisPropertyAssignment.initializeris thez.object({ msg: z.string() })CallExpression.Node.isObjectLiteralExpression(initializer)→ false (it's aCallExpression).- Falls through to the warning push → user is told to verify the very thing they already wrote.
Gap 2 — input:
import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
const server = new McpServer({ name: 't', version: '1.0' });
const inputSchema = buildInputSchema(); // Record<string, z.ZodType>
server.tool('execute', inputSchema, async (params) => ({ content: [] }));migrateToolCallenterscase 3,isStringArg(args[1])→ false →(name, schema, callback)branch.emitWrapDiagnostic(arg1, ...)—arg1is anIdentifier, not anObjectLiteralExpression→ no-op.maybeWrapSchema(arg1)→ returns the raw text'inputSchema'unwrapped.migrateToolCallreturnstrue→changesCount++, the "Could not automatically migrate" fallback never fires.- Output:
server.registerTool('execute', { inputSchema: inputSchema }, async ...)— fails TS2741 on v2, zero diagnostics, codemod claims clean migration.
Impact
Both are nit severity. Gap 1 is purely diagnostic noise (output is correct, schema is correctly left unwrapped) but trains users to ignore the codemod's diagnostics. Gap 2 leaves a TS2741 the user discovers loudly at build time, but the codemod gives no hint to track it back to server.tool() migration — contradicting the diagnostic-first design and the explicit reviewer ask in 3278854656. Both are clear leftover gaps from a fix applied directly in response to review.
Fix
For gap 1, in wrapSchemaInConfig() skip the warning when the initializer is already a z.object(...) call:
if (!initializer.getText().startsWith('z.object(')) {
diagnostics.push(warning(...));
}
return false;For gap 2, mirror the config-object treatment in emitWrapDiagnostic:
function emitWrapDiagnostic(node: Node, sourceFile: SourceFile, call: CallExpression, diagnostics: Diagnostic[]): void {
if (Node.isObjectLiteralExpression(node)) {
diagnostics.push(info(..., 'Raw object literal wrapped with z.object(). ...'));
} else if (!node.getText().startsWith('z.object(')) {
diagnostics.push(warning(..., 'inputSchema/argsSchema is not an object literal — verify it is a Standard Schema; if it is a raw Zod field map, wrap it with z.object(...).'));
}
}Tighten the test 'does not double-wrap z.object() in .registerTool() config' to also assert result.diagnostics is empty, and add a positional-form test (server.tool('x', schemaVar, cb)) asserting a warning is emitted.
🔬 also observed by mattzcarey
| } | ||
| } | ||
|
|
||
| changesCount += migrateConstructorTaskOptions(sourceFile, diagnostics); |
There was a problem hiding this comment.
🟡 The wrapSchemaInConfig(call, 'uriSchema', sourceFile, diagnostics) loop for registerResourceCalls is dead code: v2's registerResource config is ResourceMetadata (Omit<Resource, 'uri' | 'name'>), which has no uriSchema property, and v1's .resource() never had one either — configArg.getProperty('uriSchema') always returns undefined and the function early-returns false. Either delete the loop or use the actual config key if one was intended (compare registerTool→inputSchema and registerPrompt→argsSchema, which are real keys).
Extended reasoning...
What this is
mcpServerApiTransform post-processes already-v2 registerTool/registerPrompt/registerResource calls to wrap raw object-literal schemas with z.object(). The three loops pass property names to wrapSchemaInConfig:
for (const call of registerToolCalls) {
if (wrapSchemaInConfig(call, 'inputSchema', sourceFile, diagnostics)) { changesCount++; }
}
for (const call of registerPromptCalls) {
if (wrapSchemaInConfig(call, 'argsSchema', sourceFile, diagnostics)) { changesCount++; }
}
for (const call of registerResourceCalls) {
if (wrapSchemaInConfig(call, 'uriSchema', sourceFile, diagnostics)) { changesCount++; }
}inputSchema and argsSchema are real config keys on v2's ToolConfig/PromptConfig. uriSchema is not. v2's registerResource(name, uriOrTemplate, config, readCallback) takes a ResourceMetadata config, defined as Omit<Resource, 'uri' | 'name'> (packages/server/src/server/mcp.ts), which has no uriSchema field. v1's deprecated .resource() overloads also never had a uriSchema config key. Grep confirms uriSchema appears nowhere in packages/server, packages/core, or packages/client.
Why the loop never executes
wrapSchemaInConfig looks up the property and bails if it isn't there:
const schemaProp = configArg.getProperty(schemaPropertyName);
if (!schemaProp) return false;Because uriSchema is never present on any real registerResource config, getProperty('uriSchema') always returns undefined, the function returns false on the second guard, and the rest of wrapSchemaInConfig (the wrap, the shorthand/non-literal diagnostics added in later review rounds) is unreachable for this caller. The registerResourceCalls loop body never increments changesCount.
Step-by-step proof
- Input:
server.registerResource('config', 'config://app', { description: 'App config' }, async (uri) => ({ contents: [] })); mcpServerApiTransformcollects the call intoregisterResourceCalls.wrapSchemaInConfig(call, 'uriSchema', ...):configArgis the third argument{ description: 'App config' }.configArg.getProperty('uriSchema')→undefined→return false.changesCountnot incremented. Same for any other valid v2ResourceMetadatashape (title,description,mimeType,annotations, etc.).
There is no input that makes the loop body do anything, because there is no v1 or v2 config shape that includes a uriSchema property.
Why nothing else catches it
The transform reports changesCount: 0 for files that only use registerResource — same as a no-op — so the test suite passes regardless of whether the loop exists. mcpServerApi.test.ts has registerTool/registerPrompt config-wrap tests but no registerResource config-wrap test, because there's nothing to wrap.
Impact
Zero runtime impact — this is pure code hygiene. No corruption, no incorrect output, no missing diagnostics. The cost is purely confusion: a future reader will see the three parallel loops, assume uriSchema is a real v2 registerResource config key, and waste time looking for it. This is the same dead-code class as the previously-cleaned-up getNamedImportNames() and isV2Gap scaffolding flagged earlier on this PR.
Fix
Delete the loop:
for (const call of registerResourceCalls) {
if (wrapSchemaInConfig(call, 'uriSchema', sourceFile, diagnostics)) {
changesCount++;
}
}(registerResourceCalls is then unused too — drop the array and the case 'registerResource' collection arm. Or, if the intent was to validate a different config key on registerResource configs, swap 'uriSchema' for the real key.)
|
|
||
| // Collect identifiers that are actual references to the `extra` parameter | ||
| const identifiers: import('ts-morph').Node[] = []; | ||
| body.forEachDescendant(node => { | ||
| if (!Node.isIdentifier(node) || node.getText() !== EXTRA_PARAM_NAME) return; | ||
| const parent = node.getParent(); | ||
| // Skip property-name positions (e.g., meta.extra, { extra: value }, { extra }, { extra: x } = obj) | ||
| if (parent && Node.isPropertyAccessExpression(parent) && parent.getNameNode() === node) return; | ||
| if (parent && Node.isPropertyAssignment(parent) && parent.getNameNode() === node) return; | ||
| if (parent && Node.isShorthandPropertyAssignment(parent)) return; | ||
| if (parent && Node.isBindingElement(parent) && parent.getPropertyNameNode() === node) return; | ||
| identifiers.push(node); |
There was a problem hiding this comment.
🟡 The identifier-collection skip list in processCallback (lines ~99-105) covers PropertyAccessExpression name, PropertyAssignment name, ShorthandPropertyAssignment, and BindingElement propertyName — but is still missing the seven additional key-position parent kinds that renameAllReferences (astUtils.ts) already guards: PropertySignature, MethodDeclaration, MethodSignature, PropertyDeclaration, EnumMember, and get/set accessor name nodes. A handler body declaring a local type Result = { extra: string } or get extra() { ... } gets that key silently renamed to ctx (valid syntax → no throw, no rollback, no diagnostic). This is now the third site re-implementing this skip list — factoring it into a shared isKeyPositionIdentifier(node) helper would stop the recurring drift.
Extended reasoning...
What the bug is
processCallback in contextTypes.ts collects identifiers in the callback body that reference the extra parameter, then rewrites them to ctx. The skip list guarding which identifiers are not references to the parameter has been hardened across review rounds and now covers four key-position parent kinds:
if (parent && Node.isPropertyAccessExpression(parent) && parent.getNameNode() === node) return;
if (parent && Node.isPropertyAssignment(parent) && parent.getNameNode() === node) return;
if (parent && Node.isShorthandPropertyAssignment(parent)) return;
if (parent && Node.isBindingElement(parent) && parent.getPropertyNameNode() === node) return;
identifiers.push(node);But renameAllReferences in astUtils.ts — the canonical key-position guard list in this PR, hardened across three earlier review rounds — additionally guards PropertySignature, MethodDeclaration, MethodSignature, PropertyDeclaration, EnumMember, GetAccessorDeclaration, and SetAccessorDeclaration. None of those seven are in processCallback's list.
The code path that triggers it
An identifier named extra whose parent is one of those seven kinds — a local type member, a class field/method, an enum member, or an object accessor — is collected as if it were a value reference to the parameter. In the replacements loop, it doesn't match the PropertyAccessExpression value-position branch or the QualifiedName branch, so it falls through to the plain rename:
replacements.push({ node: id, newText: CTX_PARAM_NAME });Step-by-step proof
Input:
import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
server.setRequestHandler('tools/call', async (request, extra) => {
type Result = { extra: string; data: unknown }; // PropertySignature key
const obj = {
get extra() { return computeExtra(); }, // GetAccessorDeclaration name
};
return { content: [], _meta: { extraResult: extra.signal } };
});processCallbackrenames the parameter declaration toctx, then collects body identifiers matchingextra.- The
extrainside{ extra: string }has aPropertySignatureparent. None of the four guards match (PropertySignatureis a distinctSyntaxKindfromPropertyAssignment). Collected. - The
extrainsideget extra()has aGetAccessorDeclarationparent. None of the four guards match. Collected. extra.signalis a value-positionPropertyAccessExpressionand is correctly rewritten toctx.mcpReq.signal.- The replacement loop falls through to plain rename for the two key-position identifiers.
Output:
type Result = { ctx: string; data: unknown };
const obj = { get ctx() { return computeExtra(); } };Both rewrites are syntactically valid TypeScript, so there is no ts-morph manipulation error, no runner rollback, and no diagnostic. The local type's property key and the object accessor name silently change — any downstream .extra access now resolves to undefined (or fails typecheck if the receiver is typed). This is the worst codemod failure mode: silent semantic corruption that compiles.
Why nothing else prevents it
The skip list in processCallback is a re-implementation of the one in astUtils.ts (which has its own copy hardened across review comments 3133560060, 3136237226, 3138328884) and the one in specSchemaAccess.ts (hardened across 3189940697, 3193275781, and a current open comment for the same gap). Each re-implementation has drifted independently and is missing different members of the canonical list.
Impact
Trigger frequency is very low — it requires the literal identifier extra as an interface/type member, class field/method, enum member, or accessor name inside a v1 handler callback body. That's a contrived collision in real code. But when it fires, the failure mode is silent corruption that compiles, which is the failure class this PR has been most aggressively hardened against. Hence nit severity.
Fix
Add the missing seven guards alongside the existing ones in processCallback's collection loop:
if (parent && Node.isPropertySignature(parent) && parent.getNameNode() === node) return;
if (parent && Node.isMethodDeclaration(parent) && parent.getNameNode() === node) return;
if (parent && Node.isMethodSignature(parent) && parent.getNameNode() === node) return;
if (parent && Node.isPropertyDeclaration(parent) && parent.getNameNode() === node) return;
if (parent && Node.isEnumMember(parent) && parent.getNameNode() === node) return;
if (parent && Node.isGetAccessorDeclaration(parent) && parent.getNameNode() === node) return;
if (parent && Node.isSetAccessorDeclaration(parent) && parent.getNameNode() === node) return;Or — cleaner, since this is now the third file in the PR with its own copy of this guard list — factor the key-position checks out of renameAllReferences into a shared isKeyPositionIdentifier(node) helper in astUtils.ts and reuse it in processCallback, specSchemaAccess.handleReference, and specSchemaAccess.findNonImportReferences. That eliminates the recurring drift permanently. A test like async (request, extra) => { type Local = { extra: string }; return { content: [] }; } asserting extra is preserved as the type key would lock it in.
| const MOCK_METHODS = new Set(['mock', 'doMock']); | ||
| const MOCK_CALLERS = new Set(['vi', 'jest']); | ||
|
|
||
| export const mockPathsTransform: Transform = { | ||
| name: 'Mock and dynamic import path rewrites', | ||
| id: 'mock-paths', | ||
| apply(sourceFile: SourceFile, context: TransformContext): TransformResult { | ||
| const diagnostics: Diagnostic[] = []; | ||
| const usedPackages = new Set<string>(); | ||
| let changesCount = 0; | ||
|
|
||
| const calls = sourceFile.getDescendantsOfKind(SyntaxKind.CallExpression); | ||
|
|
||
| for (const call of calls) { | ||
| const expr = call.getExpression(); | ||
|
|
||
| if (Node.isPropertyAccessExpression(expr)) { | ||
| const objName = expr.getExpression().getText(); | ||
| const methodName = expr.getName(); | ||
| if (MOCK_CALLERS.has(objName) && MOCK_METHODS.has(methodName)) { | ||
| changesCount += rewriteMockCall(call, sourceFile, context, diagnostics, usedPackages); | ||
| } |
There was a problem hiding this comment.
🟡 MOCK_METHODS at mockPaths.ts:11 only covers mock/doMock, so the partial-mock idioms jest.requireActual() / vi.importActual() and the unmock family (vi.unmock, jest.unmock, jest.dontMock, jest.deepUnmock, jest.requireMock, jest.createMockFromModule) keep pointing at the v1 SDK that updatePackageJson() just removed — the migrated test file then throws Cannot find module '@modelcontextprotocol/sdk/types.js' at runtime with no codemod diagnostic. Extend MOCK_METHODS (or a sibling set) so these path-only calls route through the same firstArg.setLiteralValue(target) rewrite that rewriteMockCall already performs.
Extended reasoning...
What the bug is
mockPaths.ts:11 defines MOCK_METHODS = new Set(['mock', 'doMock']), and the dispatch loop at lines 22-32 only calls rewriteMockCall when both MOCK_CALLERS.has(objName) and MOCK_METHODS.has(methodName) hold. The other Jest/Vitest test-doubles APIs that take an SDK module specifier as their first string argument are never intercepted:
vi.importActual(spec)/jest.requireActual(spec)— the canonical partial-mock helpervi.unmock(spec)/vi.doUnmock(spec)/jest.unmock(spec)/jest.dontMock(spec)/jest.deepUnmock(spec)jest.requireMock(spec)jest.createMockFromModule(spec)/jest.genMockFromModule(spec)
Grep over the file confirms none of these names appear anywhere in mockPaths.ts.
The code path that triggers it
The canonical Jest partial-mock idiom (and the Vitest explicit-string equivalent) writes the same specifier twice:
jest.mock('@modelcontextprotocol/sdk/types.js', () => ({
...jest.requireActual('@modelcontextprotocol/sdk/types.js'),
isInitializeRequest: jest.fn()
}));After the codemod runs:
mockPathsTransformfinds the outerjest.mock(...)call (MOCK_METHODS.has('mock')is true) andrewriteMockCallrewrites the first argument to'@modelcontextprotocol/server'.- The inner
jest.requireActual(...)is also aCallExpressionwith aPropertyAccessExpressioncallee, butMOCK_METHODS.has('requireActual')isfalse→ it is silently skipped. renameSymbolsInFactoryonly touches the top-level object-literal keys of the factory return value, not nested call arguments.rewriteDynamicImportsonly matchesCallExpressions whoseexpr.getKind() === SyntaxKind.ImportKeyword—jest.requireActualis aPropertyAccessExpression, so it is never visited.importPathsTransformonly iterates staticImportDeclaration/ExportDeclarationnodes; it never looks at call arguments.updatePackageJson()then removes@modelcontextprotocol/sdkfrompackage.jsonbecause the static imports were rewritten.
Why nothing else prevents it
No transform visits string-literal call arguments other than rewriteMockCall (gated on MOCK_METHODS) and rewriteDynamicImports (gated on ImportKeyword). TypeScript will not flag the stale path either — requireActual/importActual return unknown/any and the module specifier is just a string. The only diagnostics in this file fire on unknown SDK paths inside recognized mock methods (lines 89-97), so a stale requireActual argument produces zero diagnostics.
The codemod's own test suite (mockPaths.test.ts — 'rewrites sdk/types.js path') only exercises the importOriginal callback form (vi.doMock('...', async importOriginal => { ... })), which works incidentally because the parameter inherits the rewritten outer specifier. The explicit-string form is never tested.
Step-by-step proof
Input test file:
jest.mock('@modelcontextprotocol/sdk/types.js', () => ({
...jest.requireActual('@modelcontextprotocol/sdk/types.js'),
isInitializeRequest: jest.fn(),
}));
import { isInitializeRequest } from '@modelcontextprotocol/sdk/types.js';After the codemod:
jest.mock('@modelcontextprotocol/server', () => ({
...jest.requireActual('@modelcontextprotocol/sdk/types.js'), // ← stale
isInitializeRequest: jest.fn(),
}));
import { isInitializeRequest } from '@modelcontextprotocol/server';…and package.json no longer contains @modelcontextprotocol/sdk. Running the test suite produces Cannot find module '@modelcontextprotocol/sdk/types.js' thrown from inside the mock factory. Even before the package is removed, the mock specifier and the requireActual specifier no longer match, so Jest's resolver intercepts a different module than the one being spread.
Impact
Test-file-only, and the failure is loud and easy to trace once it surfaces — hence nit. But requireActual inside a mock factory is the documented partial-mock pattern in both Jest and Vitest, the codemod actively removes the dependency that would otherwise make the stale path resolvable, and no diagnostic is emitted — contradicting the codemod's diagnostic-first design.
Fix
Add the path-only methods to a sibling set and route them through the same first-arg rewrite that rewriteMockCall already performs at line ~117 (firstArg.setLiteralValue(effectiveTarget)). These methods take only a specifier (no factory to walk), so the handler is a strict subset of rewriteMockCall:
const PATH_ONLY_MOCK_METHODS = new Set([
'importActual', 'requireActual', 'requireMock',
'unmock', 'doUnmock', 'dontMock', 'deepUnmock',
'createMockFromModule', 'genMockFromModule'
]);
// in apply():
if (MOCK_CALLERS.has(objName) && PATH_ONLY_MOCK_METHODS.has(methodName)) {
changesCount += rewriteSpecifierOnlyCall(call, sourceFile, context, diagnostics, usedPackages);
}A test like jest.mock('@modelcontextprotocol/sdk/types.js', () => ({ ...jest.requireActual('@modelcontextprotocol/sdk/types.js'), isInitializeRequest: jest.fn() })) asserting both specifiers become '@modelcontextprotocol/server' would lock this in.
| }, | ||
| '@modelcontextprotocol/sdk/client/stdio.js': { | ||
| target: '@modelcontextprotocol/client', | ||
| status: 'moved', | ||
| symbolTargetOverrides: { | ||
| StdioClientTransport: '@modelcontextprotocol/client/stdio', | ||
| DEFAULT_INHERITED_ENV_VARS: '@modelcontextprotocol/client/stdio', | ||
| getDefaultEnvironment: '@modelcontextprotocol/client/stdio', | ||
| StdioServerParameters: '@modelcontextprotocol/client/stdio' | ||
| } | ||
| }, |
There was a problem hiding this comment.
🟡 The base target: '@modelcontextprotocol/client' for client/stdio.js is unreachable for named imports (every symbol is overridden to /client/stdio via symbolTargetOverrides), but it IS the fallback for namespace imports (import * as stdio from ...), no-factory vi.mock(...) automocks, non-destructured dynamic imports, and export * from — and @modelcontextprotocol/client doesn't export the stdio symbols (per the comment at packages/client/src/index.ts:64), so those four paths produce TS2339/dead mocks with no diagnostic. Since every symbol routes to the same subpath, just use target: '@modelcontextprotocol/client/stdio' and drop the symbolTargetOverrides block, matching the sibling server/stdio.js entry above.
Extended reasoning...
What the bug is
The IMPORT_MAP entry for @modelcontextprotocol/sdk/client/stdio.js is:
'@modelcontextprotocol/sdk/client/stdio.js': {
target: '@modelcontextprotocol/client',
status: 'moved',
symbolTargetOverrides: {
StdioClientTransport: '@modelcontextprotocol/client/stdio',
DEFAULT_INHERITED_ENV_VARS: '@modelcontextprotocol/client/stdio',
getDefaultEnvironment: '@modelcontextprotocol/client/stdio',
StdioServerParameters: '@modelcontextprotocol/client/stdio'
}
},Every symbol that v1's client/stdio.js exports is overridden to @modelcontextprotocol/client/stdio — so the base target is unreachable for any named import. But the base target is exactly what's used in the cases where per-symbol routing can't apply, and @modelcontextprotocol/client (the root barrel) does not export StdioClientTransport / getDefaultEnvironment / DEFAULT_INHERITED_ENV_VARS / StdioServerParameters — packages/client/src/index.ts:64 explicitly notes those live on the ./stdio subpath only.
The four code paths that fall back to the wrong base target
-
Namespace import (
importPaths.ts): the override-routing block is gated on!namespaceImport && !defaultImport, so forimport * as stdio from '@modelcontextprotocol/sdk/client/stdio.js'the effective target stays@modelcontextprotocol/client. Output:import * as stdio from '@modelcontextprotocol/client'—stdio.StdioClientTransportis nowTS2339. -
1-arg
vi.mock(...)automock (mockPaths.tsrewriteMockCall): override routing is gated onargs.length >= 2.vi.mock('@modelcontextprotocol/sdk/client/stdio.js')becomesvi.mock('@modelcontextprotocol/client')— but the production code's static import is rewritten to@modelcontextprotocol/client/stdio, so the mock targets a different module and silently has no effect. -
Non-destructured dynamic import (
mockPaths.tsrewriteDynamicImports): the override check requiresNode.isObjectBindingPattern.const stdio = await import('@modelcontextprotocol/sdk/client/stdio.js')is rewritten to import from@modelcontextprotocol/client—stdio.StdioClientTransportis undefined at runtime. -
Star re-export (
importPaths.tsrewriteExportDeclarations):export * from '@modelcontextprotocol/sdk/client/stdio.js'—getNamedExports().length === 0, soallOverriddenis false and the base target is used.
Why this is internally inconsistent
The sibling entry directly above does it right:
'@modelcontextprotocol/sdk/server/stdio.js': {
target: '@modelcontextprotocol/server/stdio',
status: 'moved'
},No symbolTargetOverrides, the subpath is the target, and all four cases above route correctly for the server side. The client side should be the same shape — symbolTargetOverrides is meant for splitting an import across different v2 packages (like streamableHttp.js → /server + /node); when every symbol routes to the same place, it's just an indirect (and partial) way of saying target.
Step-by-step proof (namespace import)
Input:
import * as stdio from '@modelcontextprotocol/sdk/client/stdio.js';
const t = new stdio.StdioClientTransport({ command: 'node' });importPathsTransformfinds the namespace import;mapping.symbolTargetOverridesis set.- The override branch is gated on
!namespaceImport && !defaultImport→ skipped becausenamespaceImportis truthy. effectiveTargetstays'@modelcontextprotocol/client'(the base target).imp.setModuleSpecifier('@modelcontextprotocol/client'). No diagnostic.- Output:
import * as stdio from '@modelcontextprotocol/client'; const t = new stdio.StdioClientTransport(...)→TS2339: Property 'StdioClientTransport' does not exist on type 'typeof import(\"@modelcontextprotocol/client\")'.
Impact
The four trigger patterns are uncommon — most v1 code uses named imports of StdioClientTransport, which route correctly via the per-symbol addPending path (and the existing tests cover exactly that). But when one of these patterns is hit, the codemod produces non-compiling output (or a silently dead mock) with zero diagnostics, which is the worst codemod failure mode. The automock case in particular is a silent test-behavior change: the test still passes the mock setup but the real StdioClientTransport ends up loaded.
Fix
One-line change matching the sibling server/stdio.js entry:
'@modelcontextprotocol/sdk/client/stdio.js': {
target: '@modelcontextprotocol/client/stdio',
status: 'moved'
},The symbolTargetOverrides block becomes a no-op once the base target equals the override target, so it can be deleted. The existing tests for named/aliased imports ('rewrites client stdio to @modelcontextprotocol/client/stdio subpath', 'preserves alias for client stdio import and routes to subpath', 'rewrites client stdio mock to /stdio subpath') will continue to pass unchanged. Adding a test for import * as stdio from '@modelcontextprotocol/sdk/client/stdio.js' would lock the fix in.
…kage The packages/codemod/ package (added in #1950) was written before the bundler resolution flip and used the old .js-extension import convention. This brings it in line with the rest of the repo. Backtick string fixtures that simulate user code (which intentionally still uses .js) are left alone.
4 participants
feat: add
@modelcontextprotocol/codemodpackage for automated v1 → v2 migrationAdds a new
@modelcontextprotocol/codemodpackage that automatically migrates MCP TypeScript SDK code from v1 (@modelcontextprotocol/sdk) to the v2 multi-package architecture (@modelcontextprotocol/client,/server,/core,/node,/express). Powered byts-morphfor AST-level precision and shipped as both a CLI (mcp-codemod) and a programmatic API. Also automatically updatespackage.json— removes the v1 SDK dependency and adds the correct v2 packages based on what the code actually imports.Motivation and Context
The v2 SDK introduces a multi-package structure, renamed APIs, restructured context objects, and removed modules (WebSocket transport, server auth, Zod helpers). Manually migrating a codebase is tedious, error-prone, and blocks adoption. This codemod handles the mechanical 80-90% of migration — rewriting imports, renaming symbols, updating method signatures, and mapping context properties — while emitting actionable diagnostics for the remaining cases that require human judgment.
Architecture
Package Structure
High-Level Flow
flowchart TD A["mcp-codemod v1-to-v2 ./src"] --> B[CLI parses args] B --> C[Runner loads migration] C --> D[Analyze project type<br/>from package.json] D --> E[Create ts-morph Project<br/>glob .ts/.tsx/.mts files] E --> F[Filter out node_modules,<br/>dist, .d.ts, .d.mts] F --> G{For each<br/>source file} G --> H[Apply transforms<br/>in order] H --> I[Collect diagnostics,<br/>change counts,<br/>& used v2 packages] I --> G G -->|done| P[Update package.json:<br/>remove v1 SDK,<br/>add detected v2 packages] P --> J{Dry run?} J -->|yes| K[Report changes<br/>without saving] J -->|no| L[Save modified files<br/>& package.json to disk] K --> M[Print summary:<br/>files changed,<br/>package.json changes,<br/>diagnostics] L --> MTransform Pipeline
Transforms run in a strict order per file. Each transform receives the
SourceFileAST, mutates it in place, and returns a change count plus diagnostics. One failing transform does not block the others.flowchart LR subgraph "Phase 1: Foundation" T1["1. importPaths<br/>Rewrite import specifiers<br/>from v1 → v2 packages"] end subgraph "Phase 2: Symbols" T2["2. symbolRenames<br/>McpError → ProtocolError<br/>ErrorCode split, etc."] T3["3. removedApis<br/>Drop Zod helpers,<br/>IsomorphicHeaders"] end subgraph "Phase 3: API Surface" T4["4. mcpServerApi<br/>.tool() → .registerTool()<br/>restructure args"] T5["5. handlerRegistration<br/>Schema → method string"] T6["6. schemaParamRemoval<br/>Remove schema args"] T7["7. expressMiddleware<br/>hostHeaderValidation<br/>signature update"] end subgraph "Phase 4: Context & Tests" T8["8. contextTypes<br/>extra → ctx,<br/>property path remapping"] T9["9. mockPaths<br/>vi.mock / jest.mock<br/>specifier updates"] end T1 --> T2 --> T3 --> T4 --> T5 & T6 & T7 --> T8 --> T9Import Resolution Strategy
Some v1 paths (e.g.,
@modelcontextprotocol/sdk/types.js) are shared code that could belong to either the client or server package. The codemod resolves these contextually:flowchart TD A["v1 import path"] --> B{Path in<br/>IMPORT_MAP?} B -->|no| Z[Leave unchanged] B -->|yes| C{Status?} C -->|removed| D["Remove import +<br/>emit warning diagnostic"] C -->|moved| E{Target is<br/>RESOLVE_BY_CONTEXT?} C -->|renamed| F["Rewrite path +<br/>rename symbols"] E -->|no| G["Rewrite to<br/>fixed target package"] E -->|yes| H{Project type?} H -->|client only| I["→ @modelcontextprotocol/client"] H -->|server only| J["→ @modelcontextprotocol/server"] H -->|both or unknown| K["→ @modelcontextprotocol/server<br/>(safe default)"]Context Property Remapping
The v2 SDK restructures the handler context from a flat object into nested groups. The
contextTypestransform handles this remapping:flowchart LR subgraph "v1 — flat RequestHandlerExtra" E1["extra.signal"] E2["extra.requestId"] E3["extra.authInfo"] E4["extra.sendRequest(...)"] E5["extra.sendNotification(...)"] E6["extra.taskStore"] end subgraph "v2 — nested BaseContext" C1["ctx.mcpReq.signal"] C2["ctx.mcpReq.id"] C3["ctx.http?.authInfo"] C4["ctx.mcpReq.send(...)"] C5["ctx.mcpReq.notify(...)"] C6["ctx.task?.store"] end E1 --> C1 E2 --> C2 E3 --> C3 E4 --> C4 E5 --> C5 E6 --> C6What Each Transform Does
imports@modelcontextprotocol/sdk/...import paths to their v2 package destinations. Merges duplicate imports, separates type-only imports, resolves ambiguous shared paths by project type. Splits imports when symbols from a single v1 module map to different v2 packages (e.g.,StreamableHTTPServerTransport→/node,EventStore→/server). Tracks which v2 packages are used for automaticpackage.jsonupdates.symbolsMcpError→ProtocolError). SplitsErrorCodeintoProtocolErrorCode+SdkErrorCodebased on member usage. ConvertsRequestHandlerExtra→ServerContext/ClientContext. ReplacesSchemaInput<T>withStandardSchemaWithJSON.InferInput<T>.removed-apisschemaToJson,parseSchemaAsync, etc.), renamesIsomorphicHeaders→Headers, convertsStreamableHTTPError→SdkErrorwith constructor mapping warnings.mcpserver-apiMcpServermethod calls:.tool()→.registerTool(),.prompt()→.registerPrompt(),.resource()→.registerResource(). Restructures 2-4 positional args into(name, config, callback)form. Wraps raw object schemas withz.object().handlersserver.setRequestHandler(CallToolRequestSchema, ...)toserver.setRequestHandler('tools/call', ...)using the schema-to-method mapping table. Covers 15 request schemas and 8 notification schemas.schema-params.request(),.callTool(), and.send()calls where the second argument is a schema reference (ends withSchema).express-middlewarehostHeaderValidation({ allowedHosts: [...] })→hostHeaderValidation([...]).contextextracallback parameter toctx. Rewrites 13 property access paths from the flat v1 context to the nested v2 context structure. Warns on destructuring patterns that need manual review.mock-pathsvi.mock()/jest.mock()specifiers from v1 to v2 paths. Updatesimport()expressions in mock factories. Renames symbol references inside mock factory return objects.CLI Usage
Programmatic API
How Has This Been Tested?
package.jsonupdates..d.tsexclusion, unknown transform ID validation.Breaking Changes
None — this is a new package with no existing users.
Types of changes
Checklist
Additional context
Design Decisions
ts-morph over jscodeshift: ts-morph provides full TypeScript type information and a more ergonomic API for the precise symbol-level transforms this codemod requires (e.g., distinguishing
ErrorCode.RequestTimeoutfromErrorCode.InvalidRequestfor theProtocolErrorCode/SdkErrorCodesplit).Ordered transforms with per-file isolation: Transforms run in a declared order (imports first, mocks last). If a transform throws for a given file, the remaining transforms are skipped for that file (since the AST may be in a partially-mutated state), but processing continues for other files. An error diagnostic is emitted for the affected file.
Declarative mapping tables: All rename/remap rules live in dedicated mapping files (
importMap.ts,symbolMap.ts, etc.) rather than being scattered across transform logic. This makes the migration rules auditable and easy to extend.Context-aware import resolution: Shared v1 paths like
@modelcontextprotocol/sdk/types.jsare resolved to client or server packages based onpackage.jsondependency analysis, not just hardcoded defaults.Diagnostic-first approach for removals: When the codemod encounters removed APIs (WebSocket transport, server auth, Zod helpers), it doesn't silently drop them — it emits clear warning diagnostics with migration guidance so users know what needs manual attention.
Automatic
package.jsonupdates: As transforms rewrite imports, they track which v2 packages are targeted. After all source transforms complete, the runner removes@modelcontextprotocol/sdkfrompackage.jsonand adds exactly the v2 packages the code actually uses. Version specifiers are injected at build time from sibling package versions viascripts/generate-versions.ts. Private packages (@modelcontextprotocol/core) are filtered out. The updater preserves the original indentation and trailing newline, and respects dry-run mode.Import splitting with
symbolTargetOverrides: When symbols from a single v1 module map to different v2 packages (e.g.,StreamableHTTPServerTransport→@modelcontextprotocol/nodebutEventStore→@modelcontextprotocol/server), the import map supports per-symbol target overrides. The transform splits the import into separate declarations for each target package.