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 |
| export function renameAllReferences(sourceFile: SourceFile, oldName: string, newName: string): void { | ||
| sourceFile.forEachDescendant(node => { | ||
| if (Node.isIdentifier(node) && node.getText() === oldName) { | ||
| const parent = node.getParent(); | ||
| if (!parent) return; | ||
| if (Node.isImportSpecifier(parent)) return; | ||
| if (Node.isPropertyAssignment(parent) && parent.getNameNode() === node) return; | ||
| if (Node.isPropertyAccessExpression(parent) && parent.getNameNode() === node) return; | ||
| if (Node.isPropertySignature(parent) && parent.getNameNode() === node) return; | ||
| if (Node.isMethodDeclaration(parent) && parent.getNameNode() === node) return; | ||
| if (Node.isPropertyDeclaration(parent) && parent.getNameNode() === node) return; | ||
| node.replaceWithText(newName); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 The skip list here covers ImportSpecifier, PropertyAssignment name, PropertyAccessExpression name, etc., but not ShorthandPropertyAssignment or ExportSpecifier — so const errors = { McpError } becomes { ProtocolError } (silently changing the object key) and a local export { McpError } becomes export { ProtocolError } (changing the module's public export name). For shorthand, expand to { McpError: ProtocolError } — exactly what renameSymbolsInFactory in mockPaths.ts:125-132 already does for the same case; for export specifiers, emit export { ProtocolError as McpError } (or skip with a diagnostic).
Extended reasoning...
What the bug is
renameAllReferences (astUtils.ts:4-18) walks every identifier matching oldName and replaces it with newName unless the parent is one of: ImportSpecifier, PropertyAssignment name node, PropertyAccessExpression name node, PropertySignature name, MethodDeclaration name, or PropertyDeclaration name. Two parent kinds that also use the identifier as an externally-visible key are missing from this list:
ShorthandPropertyAssignment— in{ McpError }, the single identifier is simultaneously the property key and the value reference.Node.isPropertyAssignmentdoes not match shorthand (they are distinctSyntaxKinds in TS / ts-morph), so it falls through tonode.replaceWithText(newName).ExportSpecifier— inexport { McpError }(a local re-export with nofromclause), the identifier is both the local binding and the exported name. This parent kind is also not in the skip list.
Code path that triggers it
renameAllReferences is called from symbolRenamesTransform (for every SIMPLE_RENAMES hit), importPathsTransform (for renamedSymbols mappings), and removedApisTransform (IsomorphicHeaders → Headers, StreamableHTTPError → SdkError). Any of these can hit a shorthand property or local re-export in the file body.
Why existing code doesn't prevent it
The guard at line 10 — Node.isPropertyAssignment(parent) && parent.getNameNode() === node — only matches the long-form { McpError: x }. For shorthand, ts-morph returns a ShorthandPropertyAssignment node and Node.isPropertyAssignment(parent) is false, so the guard is bypassed. Likewise, line 9's Node.isImportSpecifier(parent) does not cover ExportSpecifier. No later transform repairs the result. The existing tests at symbolRenames.test.ts ("does not rename property keys" / "property access names") cover PropertyAssignment and PropertyAccessExpression but not shorthand or export specifiers.
Notably, renameSymbolsInFactory in mockPaths.ts:125-132 already handles the identical ShorthandPropertyAssignment case correctly by expanding to ${newName}: ${name} — so the omission here is an internal inconsistency, and the fix pattern is already in this PR.
Step-by-step proof
Shorthand case — input:
import { McpError } from '@modelcontextprotocol/sdk/types.js';
export const errors = { McpError };symbolRenamesTransformfindsMcpErrorin an MCP import, callsnamedImport.setName('ProtocolError')and thenrenameAllReferences(sourceFile, 'McpError', 'ProtocolError').forEachDescendantvisits the identifier inside{ McpError }.node.getText() === 'McpError'✓.parentis aShorthandPropertyAssignment. None of the six guards match (isImportSpecifier✗,isPropertyAssignment✗ — distinct kind,isPropertyAccessExpression✗,isPropertySignature✗,isMethodDeclaration✗,isPropertyDeclaration✗).node.replaceWithText('ProtocolError')→ output:export const errors = { ProtocolError };.- The object's runtime key is now
'ProtocolError'; any consumer readingerrors.McpErrorgetsundefined.
ExportSpecifier case — input:
import { McpError } from '@modelcontextprotocol/sdk/types.js';
export { McpError };- Same trigger as above.
forEachDescendantvisits the identifier insideexport { McpError }.parentis anExportSpecifier— not in the skip list.node.replaceWithText('ProtocolError')→export { ProtocolError };.- The module's public export name has changed; downstream files importing
{ McpError }from this module break. (Re-exports with afromclause go throughgetSdkExports/importPathsTransforminstead, so this specifically affects local re-exports.)
Impact
The output still compiles (the import was renamed too, so ProtocolError is a valid local binding), which makes this a silent semantic change — the worst codemod failure mode, since neither TypeScript nor the user's tests-of-the-codemod will flag it. The trigger patterns — packing an SDK error class into a shorthand object literal (error registries, DI containers) or re-exporting it from a local barrel — are realistic but not common, hence nit severity.
Fix
Add two cases to the guard list, mirroring the mockPaths.ts pattern:
if (Node.isShorthandPropertyAssignment(parent)) {
// { McpError } → { McpError: ProtocolError } — preserve the key
parent.replaceWithText(`${oldName}: ${newName}`);
return;
}
if (Node.isExportSpecifier(parent)) {
// export { McpError } → export { ProtocolError as McpError } — preserve the exported name
if (!parent.getAliasNode()) {
parent.setAlias(oldName);
}
parent.setName(newName);
return;
}(Or, more conservatively, just return for both and push a diagnostic.) Add tests covering const e = { McpError } → { McpError: ProtocolError } and export { McpError } → export { ProtocolError as McpError }.
| if (mapping.renamedSymbols) { | ||
| for (const [oldName, newName] of Object.entries(mapping.renamedSymbols)) { | ||
| renameAllReferences(sourceFile, oldName, newName); | ||
| } | ||
| } | ||
|
|
||
| const hasAlias = namedImports.some(n => n.getAliasNode() !== undefined); | ||
| if (defaultImport || namespaceImport || hasAlias) { | ||
| imp.setModuleSpecifier(targetPackage); | ||
| if (mapping.renamedSymbols) { | ||
| for (const n of namedImports) { | ||
| const newName = mapping.renamedSymbols[n.getName()]; | ||
| if (newName) { | ||
| n.setName(newName); | ||
| } | ||
| } | ||
| } | ||
| changesCount++; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🟡 When a path with renamedSymbols is namespace-imported (e.g., import * as transport from '@modelcontextprotocol/sdk/server/streamableHttp.js'; new transport.StreamableHTTPServerTransport({})), renameAllReferences skips the qualified property name (astUtils.ts:11) and the namespaceImport branch only rewrites the module specifier — so the output references transport.StreamableHTTPServerTransport on @modelcontextprotocol/node, which doesn't export it, with no diagnostic. This is a leftover gap from the fix for the earlier namespace-import comment; consider pushing a warning(...) when namespaceImport && mapping.renamedSymbols (or rewriting <ns>.<oldName> accesses) to match the "conservative — emit diagnostics" design.
Extended reasoning...
What the bug is
The if (defaultImport || namespaceImport || hasAlias) branch added to fix the earlier namespace/alias comment (3132456083) correctly preserves namespace imports by calling imp.setModuleSpecifier(targetPackage) instead of remove-and-re-add. But when the mapping also has renamedSymbols, the branch only loops over namedImports (lines 100-106) — which is empty for a namespace import — and the renameAllReferences call just above (lines 90-94) explicitly skips identifiers that are the name node of a PropertyAccessExpression (astUtils.ts:11). So qualified accesses like transport.StreamableHTTPServerTransport are never rewritten, and no diagnostic is emitted.
Code path that triggers it
Input:
import * as transport from '@modelcontextprotocol/sdk/server/streamableHttp.js';
const t = new transport.StreamableHTTPServerTransport({});namedImports = [],namespaceImportis set,defaultImportundefined.- Lines 90-94:
renameAllReferences(sourceFile, 'StreamableHTTPServerTransport', 'NodeStreamableHTTPServerTransport')walks identifiers. It findsStreamableHTTPServerTransportinsidetransport.StreamableHTTPServerTransport, butparentis aPropertyAccessExpressionandparent.getNameNode() === node→ astUtils.ts:11 returns early. Not renamed. (This guard is correct in general — you don't want to rename arbitraryobj.StreamableHTTPServerTransportaccesses — but it means namespace-qualified accesses aren't covered.) - Line 97:
namespaceImporttruthy → enters branch.imp.setModuleSpecifier('@modelcontextprotocol/node'). - Lines 100-106:
for (const n of namedImports)iterates an empty array → no-op. continue. No diagnostic pushed.
Why nothing else catches it
symbolRenamesTransform (next in the pipeline) only triggers renameAllReferences when it finds the old name in a named import specifier (imp.getNamedImports()), and there are none here. mockPathsTransform only handles vi.mock/jest.mock and dynamic import(). So no later transform repairs the qualified property access.
The existing test 'preserves namespace imports by rewriting module specifier' (importPaths.test.ts:152) uses sdk/types.js, which has no renamedSymbols, so it doesn't exercise this combination.
Step-by-step proof
Input:
import * as transport from '@modelcontextprotocol/sdk/server/streamableHttp.js';
const t = new transport.StreamableHTTPServerTransport({});IMPORT_MAP['@modelcontextprotocol/sdk/server/streamableHttp.js']={ target: '@modelcontextprotocol/node', status: 'renamed', renamedSymbols: { StreamableHTTPServerTransport: 'NodeStreamableHTTPServerTransport' } }.- After
importPathsTransform:import * as transport from '@modelcontextprotocol/node'; const t = new transport.StreamableHTTPServerTransport({}); - After
symbolRenamesTransform: unchanged (the import declaration'sgetNamedImports()is[], so theSIMPLE_RENAMESloop never matchesStreamableHTTPServerTransport). - Final output references
transport.StreamableHTTPServerTransport, but@modelcontextprotocol/nodeexportsNodeStreamableHTTPServerTransport→TS2339: Property 'StreamableHTTPServerTransport' does not exist on type .... Zero diagnostics emitted.
Impact
Namespace-importing the single-export streamableHttp module is uncommon (most v1 code uses import { StreamableHTTPServerTransport }), and the resulting TypeScript error is immediate and obvious — hence nit. But it produces silently-broken output with no diagnostic, which contradicts the codemod's stated "Conservative by default: prefers emitting diagnostics for cases that need manual attention" design, and it's the one remaining gap in the namespace/alias handling that the earlier review round otherwise fixed.
Fix
Minimal: emit a warning so the user knows manual attention is needed.
if (defaultImport || namespaceImport || hasAlias) {
imp.setModuleSpecifier(targetPackage);
if (mapping.renamedSymbols) {
for (const n of namedImports) { ... }
if (namespaceImport) {
diagnostics.push(warning(filePath, line,
`Namespace import of ${specifier}: exported symbol(s) ${Object.keys(mapping.renamedSymbols).join(', ')} ` +
`were renamed in ${targetPackage}. Update qualified accesses (e.g., ${namespaceImport.getText()}.${Object.keys(mapping.renamedSymbols)[0]}) manually.`));
}
}
...
}Alternatively, actively rewrite <ns>.<oldName> property accesses by walking PropertyAccessExpression nodes whose object is the namespace identifier — but the warning is the cheaper fix and matches how other un-automatable cases are handled.
| for (const removal of removals) { | ||
| for (const imp of sourceFile.getImportDeclarations()) { | ||
| for (const namedImport of imp.getNamedImports()) { | ||
| if (namedImport.getName() === removal.importName) { | ||
| namedImport.remove(); | ||
| if (imp.getNamedImports().length === 0 && !imp.getDefaultImport() && !imp.getNamespaceImport()) { | ||
| imp.remove(); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| diagnostics.push(warning(sourceFile.getFilePath(), removal.line, `${removal.importName}: ${removal.message}`)); |
There was a problem hiding this comment.
🟡 The removal loop iterates all import declarations without re-checking isAnyMcpSpecifier(), and the break at line 66 only exits the inner named-import loop — so if a file imports e.g. schemaToJson from both an MCP package and another library (aliased), the codemod strips it from both. This is the same missing-filter class flagged and fixed in symbolRenames.ts; add if (!isAnyMcpSpecifier(imp.getModuleSpecifierValue())) continue; at line 60 to match handleIsomorphicHeaders/handleStreamableHTTPError.
Extended reasoning...
What the bug is
handleRemovedZodHelpers (removedApis.ts:37-74) uses two passes: a find loop (lines 46-56) that correctly gates on isAnyMcpSpecifier(imp.getModuleSpecifierValue()) to populate removals, and a removal loop (lines 58-69) that re-iterates sourceFile.getImportDeclarations() to actually delete the named imports. The second loop has no specifier filter — it removes any named import whose getName() matches removal.importName, regardless of which package it came from. The break on line 66 only exits the innermost for (const namedImport of ...) loop; the middle for (const imp of ...) loop continues to the next import declaration, so a matching name in a non-MCP import is also removed.
Code path that triggers it
The literal scenario "two unaliased import { schemaToJson } lines" is invalid TypeScript (duplicate identifier). The realistic trigger is an aliased import — ts-morph's ImportSpecifier.getName() returns the exported name, not the local alias, so import { schemaToJson as toJson } from 'some-other-lib' still matches namedImport.getName() === 'schemaToJson'.
Why existing code doesn't prevent it
This is internally inconsistent with the two sibling functions in the same file: handleIsomorphicHeaders (lines 81-91) and handleStreamableHTTPError (lines 122-132) both capture the specific foundImport/foundImportDecl from the MCP-filtered scan and operate only on that. handleRemovedZodHelpers instead re-scans without the filter. The existing test 'does not touch non-MCP imports with same names' (removedApis.test.ts:79) only tests a file with only the non-MCP import — removals is empty, the second loop never runs, and the gap isn't exercised. This is also the same class of missing-isAnyMcpSpecifier filter that was already flagged and fixed in symbolRenames.ts (resolved comment 3133268143).
Step-by-step proof
Input:
import { McpServer, schemaToJson } from '@modelcontextprotocol/server';
import { schemaToJson as otherToJson } from 'some-json-schema-lib';
const json = otherToJson(schema);- Find loop (line 46): first declaration passes
isAnyMcpSpecifier, findsschemaToJson→removals = [{ importName: 'schemaToJson', ... }]. Second declaration failsisAnyMcpSpecifier→ skipped. ✓ - Removal loop (line 58):
removal.importName = 'schemaToJson'. - Iterates first declaration (
@modelcontextprotocol/server): findsschemaToJson, removes it, declaration still hasMcpServerso it survives,breakexits the named-import loop. - Middle loop continues to the second declaration (
some-json-schema-lib):namedImport.getName()returns'schemaToJson'(the export name, not the aliasotherToJson) → match →namedImport.remove(). Declaration now has zero named imports →imp.remove(). - Output:
import { McpServer } from '@modelcontextprotocol/server'; const json = otherToJson(schema);—otherToJsonis now an undefined reference.
Impact
Nit severity: the trigger requires importing one of the specific REMOVED_ZOD_HELPERS names (schemaToJson, parseSchemaAsync, getSchemaShape, etc.) from both an MCP package and a non-MCP package in the same file, which is uncommon. But when it does fire, the codemod silently corrupts an unrelated import with no diagnostic, and the fix is a one-liner.
Fix
Add the same guard used by the find loop to the removal loop:
for (const removal of removals) {
for (const imp of sourceFile.getImportDeclarations()) {
if (!isAnyMcpSpecifier(imp.getModuleSpecifierValue())) continue;
for (const namedImport of imp.getNamedImports()) {
...(Or refactor to capture the specific ImportSpecifier in the first pass, matching handleIsomorphicHeaders/handleStreamableHTTPError.)
1 participant
Adds 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.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] I --> G G -->|done| J{Dry run?} J -->|yes| K[Report changes<br/>without saving] J -->|no| L[Save modified files<br/>to disk] K --> M[Print summary:<br/>files changed,<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.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?
.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 isolation: Transforms run in a declared order (imports first, mocks last) but each transform's failure is isolated — one failing transform emits a diagnostic and doesn't block the rest. This maximizes the amount of migration that succeeds per run.
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.