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 |
|
@claude review |
|
@claude review |
| "files": [ | ||
| "dist" | ||
| ], |
There was a problem hiding this comment.
🟡 The new @modelcontextprotocol/codemod package is added to the publish workflow but ships with no README.md, while every other published package in the repo (client, server, middleware/*) has one. The PR description's CLI usage, programmatic API, and transform table won't be visible on the npm package page — worth adding a packages/codemod/README.md before this lands (npm auto-includes README regardless of the files array, so no package.json change is needed).
Extended reasoning...
What the gap is
This PR adds @modelcontextprotocol/codemod as a new published package — it has a bin entry (mcp-codemod), a programmatic API export, and is added to .github/workflows/publish.yml:43 so pkg-pr-new is already publishing preview builds. But packages/codemod/ contains no README.md. Every other published package in the repo has one: packages/client/README.md, packages/server/README.md, and all four packages/middleware/*/README.md files exist.
The repo's own review checklist (REVIEW.md) explicitly calls this out: "New feature: verify prose documentation is added (not just JSDoc), and assess whether examples/ needs a new or updated example." A new package with both a CLI and a programmatic API is squarely in scope for that item.
Why it matters
The PR description already contains exactly the documentation users need — the CLI Usage block (mcp-codemod v1-to-v2 ./src, --dry-run, --transforms, --list, --ignore), the Programmatic API snippet (getMigration / run), and the 9-row transform table. None of that will be visible on npm. A user who runs npx @modelcontextprotocol/codemod and gets the commander help text will have nowhere to look for the transform IDs, the design decisions, or the migration scope.
Step-by-step proof
.github/workflows/publish.yml:43adds'./packages/codemod'to thepkg-pr-new publishcommand, so the package is published on every PR push (and presumably on release).packages/codemod/package.json:32-34sets"files": ["dist"]and abinentry — so the package is intended for end-user consumption vianpx.ls packages/codemod/shows:eslint.config.mjs,package.json,src/,test/,tsconfig.json,tsdown.config.ts,typedoc.json,vitest.config.js— noREADME.md.ls packages/*/README.md packages/middleware/*/README.mdshows READMEs for client, server, express, fastify, hono, and node — codemod is the only published package without one.- On npm, the package page renders the README as the primary documentation surface. With no README, the page shows only the
package.jsondescription string: "Codemod to migrate MCP TypeScript SDK code from v1 to v2" — no usage, no flags, no transform list.
Note on files array
npm always includes README* (along with package.json, LICENSE*, and the main entry) regardless of the files allowlist, so adding packages/codemod/README.md is sufficient — no need to touch "files": ["dist"].
Fix
Add packages/codemod/README.md containing (at minimum) the CLI Usage block, the Programmatic API snippet, and the transform table from this PR's description. Given the PR is explicitly titled "draft" and the author has an open TODO checklist, this is likely already planned — flagging it here per the REVIEW.md checklist item so it doesn't slip.
There was a problem hiding this comment.
TBD. Add README.md once the feasibility of the codemod has been confirmed.
|
@claude review |
| export function isImportedFromMcp(sourceFile: SourceFile, symbolName: string): boolean { | ||
| return sourceFile.getImportDeclarations().some(imp => { | ||
| if (!isAnyMcpSpecifier(imp.getModuleSpecifierValue())) return false; | ||
| return imp.getNamedImports().some(n => n.getName() === symbolName); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 isImportedFromMcp() compares against n.getName() (the export name), but callers at handlerRegistration.ts:40 and schemaParamRemoval.ts:31 pass firstArg.getText()/secondArg.getText() — the local binding name — so an aliased MCP import plus a same-named local import confuses the provenance check. The new test 'does not remove non-MCP import when MCP import of same name is consumed' (handlerRegistration.test.ts:108-119) actually locks in the wrong behavior: CallToolRequestSchema there is bound to ./local-schemas.js, yet the test asserts it's rewritten to 'tools/call'. Note that mcpServerApi.ts:15 and expressMiddleware.ts:12 pass hardcoded export names and rely on the current semantics, so the fix is to check the local binding (n.getAliasNode()?.getText() ?? n.getName()) only for the body-identifier callers — e.g., a separate helper or an optional flag — rather than changing the function unconditionally.
Extended reasoning...
What the bug is
isImportedFromMcp(sourceFile, symbolName) (importUtils.ts:76-81) tests imp.getNamedImports().some(n => n.getName() === symbolName). ts-morph's ImportSpecifier.getName() returns the export name (the identifier before as), not the local alias — this is the same fact already established and acted on elsewhere in this PR (e.g., resolved comment 3132456083, and removedApis.ts using foundImport.getAliasNode()?.getText() ?? 'IsomorphicHeaders' to derive the local name).
Two of the four callers pass the local identifier text as it appears in the body:
handlerRegistration.ts:40—schemaName = firstArg.getText()schemaParamRemoval.ts:31—schemaName = secondArg.getText()
For these callers, comparing the body identifier against the export name is a category error whenever an alias is involved.
The concrete manifestation: a test that asserts wrong behavior
The most actionable evidence is the new test at handlerRegistration.test.ts:108-119:
import { CallToolRequestSchema } from './local-schemas.js';
import { CallToolRequestSchema as McpSchema } from '@modelcontextprotocol/sdk/types.js';
server.setRequestHandler(CallToolRequestSchema, async () => ({ content: [] }));
validateSchema(McpSchema);In this fixture, the CallToolRequestSchema passed to setRequestHandler is the local binding from ./local-schemas.js — the MCP one is aliased to McpSchema. The test asserts expect(result).toContain("'tools/call'") and not.toMatch(/setRequestHandler\(CallToolRequestSchema/), i.e., it expects the local schema usage to be rewritten as if it were the MCP schema. That's the false positive, encoded as expected behavior. (The test was presumably written to verify that removeUnusedImport(..., onlyMcpImports=true) doesn't strip the local import — which it does verify — but the rewrite assertion is incorrect.)
Step-by-step proof (false positive)
firstArg.getText()='CallToolRequestSchema'.ALL_SCHEMA_TO_METHOD['CallToolRequestSchema']='tools/call'→ passes line 38.isImportedFromMcp(sf, 'CallToolRequestSchema'): iterates imports; the second declaration is an MCP specifier; its named import hasgetName() === 'CallToolRequestSchema'(the export name on the aliased specifier) → returns true.firstArg.replaceWithText("'tools/call'")— butfirstArgreferred to the local./local-schemas.jsbinding, not the MCP one.removeUnusedImport(sf, 'CallToolRequestSchema', true)then scans for body references toCallToolRequestSchema, finds none (it was just replaced), and — correctly — only removes the MCP-aliased specifier? Actually no:removeUnusedImportat importUtils.ts:96 also matches onnamedImport.getName()(export name), so it would try to remove the aliased MCP specifier, butMcpSchemais still referenced invalidateSchema(McpSchema), soreferenceCount > 0and nothing is removed. Hence the local import survives (which is what the test's first assertion checks), but the body call was still wrongly rewritten.
Addressing the refutation
A reviewer objected that (a) the proposed fix breaks the other two callers, (b) the false-negative paths are gated elsewhere, and (c) the trigger is contrived. All three points are fair and worth incorporating:
- (a) is correct and important.
mcpServerApi.ts:15andexpressMiddleware.ts:12pass literal export names ('McpServer','hostHeaderValidation') as a file-level gate. Forimport { McpServer as Server },n.getName() === 'McpServer'is exactly what they want. Changing the function to only check the alias would regress them. So the fix is not the one-liner originally proposed; it's either a second helper (e.g.,isLocalIdentifierFromMcp) for the body-identifier callers, or having the function match either form and letting callers 1/2 pass the local text while callers 3/4 keep passing the export name — but "match either" doesn't fix the false positive, so a separate helper is cleaner. - (b) is largely correct. The false-negative in
handlerRegistrationis moot becauseALL_SCHEMA_TO_METHOD[aliasName]is undefined and short-circuits first. InschemaParamRemoval,isSchemaReferencerequires the text to end in'Schema', so an alias likeRSis filtered out before the provenance check; only an alias that itself ends inSchema(e.g.,as ResultSchema) reaches the bug and is then silently skipped — safe (no corruption), just an incomplete migration. - (c) is correct on frequency, which is why this is a nit. But the fact that the test suite itself encodes the false positive as expected output is worth a one-line correction regardless of how rare the pattern is in the wild — tests that assert wrong behavior tend to bite later.
Impact
Nit severity. The output-corrupting path requires importing the same export name from both an MCP package (aliased) and a non-MCP source in one file — uncommon. The false-negative paths are mostly gated by earlier checks and, where reachable, leave code unchanged. The main value of the fix is (1) correcting the test so it doesn't lock in a wrong rewrite, and (2) eliminating an internal inconsistency where one helper serves two semantically different caller intents.
Fix
Introduce a local-binding variant for the body-identifier callers and leave the existing function for the file-level gates:
export function isLocalIdentifierFromMcp(sourceFile: SourceFile, localName: string): boolean {
return sourceFile.getImportDeclarations().some(imp => {
if (!isAnyMcpSpecifier(imp.getModuleSpecifierValue())) return false;
return imp.getNamedImports().some(n => (n.getAliasNode()?.getText() ?? n.getName()) === localName);
});
}Use it at handlerRegistration.ts:40 and schemaParamRemoval.ts:31. Then update the test at handlerRegistration.test.ts:108-119 to assert the correct behavior: the setRequestHandler(CallToolRequestSchema, ...) call (local binding) is not rewritten, and 'tools/call' does not appear.
There was a problem hiding this comment.
Thanks — the false-positive is fixed (handlerRegistration/schemaParamRemoval now resolve via local binding + resolveOriginalImportName), and the test at handlerRegistration.test.ts now asserts correct behavior. 👍
One leftover: isImportedFromMcp was changed unconditionally to match on the local binding, which regresses the file-level-gate caller the comment flagged. mcpServerApi.ts:15 still passes the hardcoded export name 'McpServer', so for import { McpServer as Server } from '@modelcontextprotocol/server' the gate now returns false (the local name is 'Server') and every .tool()/.prompt()/.resource() call in that file is silently skipped — previously n.getName() === 'McpServer' matched the export name on the aliased specifier and the transform ran. (expressMiddleware.ts:12 is moot in practice since its body check at line 32 also matches on the literal identifier text.)
"Match either form" won't work here — it would re-introduce the false positive (the comment's refutation point (a) covers this). Simplest fix is a second helper for the export-name semantics and switch mcpServerApi.ts:15 to it:
export function isExportImportedFromMcp(sourceFile: SourceFile, exportName: string): boolean {
return sourceFile.getImportDeclarations().some(imp =>
isAnyMcpSpecifier(imp.getModuleSpecifierValue()) &&
imp.getNamedImports().some(n => n.getName() === exportName)
);
}(Low-frequency edge case, so still nit — but it's a behavioral regression from the previous commit.)
|
@claude review |
| if (Node.isExportSpecifier(parent)) { | ||
| if (!parent.getAliasNode()) parent.setAlias(oldName); | ||
| parent.getNameNode().replaceWithText(newName); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 The ExportSpecifier branch added for comment 3136237226 checks only Node.isExportSpecifier(parent) without verifying whether the matched identifier is the name node or the alias node — unlike every sibling guard (lines 15-24) which checks parent.getNameNode() === node. For export { Foo as McpError }, forEachDescendant visits the alias identifier McpError, parent.getAliasNode() is truthy so setAlias is skipped, then parent.getNameNode().replaceWithText('ProtocolError') overwrites Foo → export { ProtocolError as McpError }, silently dropping the user's local binding. Add if (parent.getAliasNode() === node) return; at the top of the branch.
Extended reasoning...
What the bug is
The ExportSpecifier branch at astUtils.ts:10-14 was added in response to comment 3136237226 to rewrite export { McpError } → export { ProtocolError as McpError }. But it only checks Node.isExportSpecifier(parent) and then unconditionally operates on parent.getNameNode() — it never verifies whether the matched identifier node is the name node (the local binding, before as) or the alias node (the public export name, after as). Every other guard in this function (lines 15-24: PropertyAssignment, PropertyAccessExpression, PropertySignature, MethodDeclaration, BindingElement, etc.) explicitly checks parent.getNameNode() === node before acting; this branch is the inconsistent one.
Code path that triggers it
In ts-morph, for export { Foo as McpError }, ExportSpecifier.getNameNode() returns the Foo identifier and getAliasNode() returns the McpError identifier. forEachDescendant visits both identifiers. When it reaches the alias McpError:
Node.isIdentifier(node) && node.getText() === 'McpError'→ true (matchesoldName).Node.isExportSpecifier(parent)→ true.parent.getAliasNode()is truthy (it's theMcpErrornode) →setAliasis skipped.parent.getNameNode().replaceWithText('ProtocolError')→ replacesFoowithProtocolError.
Result: export { ProtocolError as McpError }. The user's local Foo binding is silently dropped, and ProtocolError may not even exist as a local binding (if the file imported the unaliased MCP McpError, the import was renamed to ProtocolError, so it happens to resolve — but the semantics changed: the module now exports the MCP error class instead of the user's Foo).
Why existing code doesn't prevent it
The branch correctly handles two of three cases: (a) export { McpError } — no alias, setAlias(oldName) then rename → export { ProtocolError as McpError } ✓; (b) export { McpError as Something } — forEachDescendant matches the name node McpError, alias is truthy so setAlias skipped, name replaced → export { ProtocolError as Something } ✓. But case (c) export { Foo as McpError } — where only the alias matches oldName — falls through the same code path and corrupts the name slot. The new test 'preserves export specifier public name with alias' (symbolRenames.test.ts) only covers case (a). No test covers an export specifier whose alias matches a SIMPLE_RENAMES key.
Step-by-step proof
Input (file imports MCP McpError to trigger renameAllReferences, and separately re-exports a local class under the same public name):
import { McpError } from '@modelcontextprotocol/sdk/types.js';
class AppError extends Error {}
export { AppError as McpError };
if (e instanceof McpError) { ... }symbolRenamesTransformfindsMcpErrorin the MCP import, callsnamedImport.setName('ProtocolError'), thenrenameAllReferences(sf, 'McpError', 'ProtocolError').forEachDescendantvisits the import specifier →Node.isImportSpecifier(parent)returns early. ✓- Visits
McpErrorin theinstanceofcheck → falls through tonode.replaceWithText('ProtocolError'). ✓ - Visits the alias
McpErrorinexport { AppError as McpError }→parentisExportSpecifier,getAliasNode()truthy →getNameNode().replaceWithText('ProtocolError')rewritesAppError.
Output:
import { ProtocolError } from '@modelcontextprotocol/sdk/types.js';
class AppError extends Error {}
export { ProtocolError as McpError }; // ← was AppError; now exports the MCP class instead
if (e instanceof ProtocolError) { ... }The module's public McpError export silently changed from the user's AppError to the SDK's ProtocolError — still compiles, no diagnostic, semantically wrong. (One verifier reproduced this empirically against ts-morph.)
Impact
Nit severity: the trigger requires a file that both (a) imports a SIMPLE_RENAMES key unaliased from MCP (to fire renameAllReferences) and (b) re-exports a different local under that same public name — uncommon but valid TypeScript. When it fires, though, it's the worst codemod failure mode: silent semantic corruption that type-checks. And it's a one-line internal-consistency fix in a branch that was just added in response to review.
Fix
Add a guard at the top of the branch matching the pattern used by every sibling:
if (Node.isExportSpecifier(parent)) {
if (parent.getAliasNode() === node) return; // alias is the public export name — leave it
if (!parent.getAliasNode()) parent.setAlias(oldName);
parent.getNameNode().replaceWithText(newName);
return;
}(Equivalently, if (parent.getNameNode() !== node) return;.) A test like import { McpError } from '@modelcontextprotocol/server'; class Foo {} export { Foo as McpError }; → expect output to contain export { Foo as McpError } would lock this in.
| export function isImportedFromMcp(sourceFile: SourceFile, symbolName: string): boolean { | ||
| return sourceFile.getImportDeclarations().some(imp => { | ||
| if (!isAnyMcpSpecifier(imp.getModuleSpecifierValue())) return false; | ||
| return imp.getNamedImports().some(n => { | ||
| const localName = n.getAliasNode()?.getText() ?? n.getName(); | ||
| return localName === symbolName; | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 The latest commit changed isImportedFromMcp() to compare only against the local binding (n.getAliasNode()?.getText() ?? n.getName()), but mcpServerApi.ts:15 and expressMiddleware.ts:12 still pass hardcoded export names — exactly what unresolved comment 3137901202 warned against. So import { McpServer as Server } from '...' now makes isImportedFromMcp(sf, 'McpServer') return false and the entire mcpserver-api transform silently no-ops, regressing previously-working aliased-import handling. Add a separate export-name helper for the two file-level gates (note: simply matching either name would re-break the handlerRegistration false-positive the latest commit just fixed).
Extended reasoning...
What the bug is
isImportedFromMcp (importUtils.ts:76-84) was changed in the latest commit from n.getName() === symbolName (matching the export name) to:
const localName = n.getAliasNode()?.getText() ?? n.getName();
return localName === symbolName;i.e., it now matches only the local binding name. This was done to fix the false-positive flagged in comment 3137901202 for handlerRegistration.ts:40 and schemaParamRemoval.ts:31, which pass body-identifier text (a local name). But that same comment explicitly warned: "mcpServerApi.ts:15 and expressMiddleware.ts:12 pass hardcoded export names and rely on the current semantics, so the fix is to check the local binding only for the body-identifier callers — e.g., a separate helper or an optional flag — rather than changing the function unconditionally." The function was changed unconditionally anyway, and those two callers were not updated.
Code path that triggers it
mcpServerApi.ts:15 calls isImportedFromMcp(sourceFile, 'McpServer') as a file-level gate; expressMiddleware.ts:12 calls isImportedFromMcp(sourceFile, 'hostHeaderValidation'). Both pass the export name as a literal string. With an aliased import, ts-morph's getAliasNode()?.getText() returns the alias, so localName is the alias — which never equals the hardcoded export name.
Why this is a regression
Before the latest commit, for import { McpServer as Server } from '@modelcontextprotocol/sdk/server/mcp.js', n.getName() === 'McpServer' was true, the gate passed, and the collection loop (which has no receiver check) correctly migrated server.tool(...) → server.registerTool(...). After the change, localName = 'Server', 'Server' !== 'McpServer', the gate returns false, and the transform returns { changesCount: 0 } without touching any .tool()/.prompt()/.resource() calls. No diagnostic is emitted. No test in mcpServerApi.test.ts covers an aliased McpServer import, so the regression is uncaught. (For expressMiddleware, the gate has the same issue, though the body check at line 32 was already literal-name-based — so the net effect there is unchanged; the observable regression is in mcpServerApi.)
Step-by-step proof
Input:
import { McpServer as Server } from '@modelcontextprotocol/sdk/server/mcp.js';
const s = new Server({ name: 'test', version: '1.0' });
s.tool('greet', async () => ({ content: [] }));mcpServerApiTransform.apply()runs; line 15 callsisImportedFromMcp(sf, 'McpServer').- importUtils.ts:78 — the import's specifier passes
isAnyMcpSpecifier. - importUtils.ts:80 — the single named import has
getAliasNode()?.getText() = 'Server', solocalName = 'Server'. - importUtils.ts:81 —
'Server' === 'McpServer'→ false..some()returns false. - mcpServerApi.ts:15-17 — gate fails →
return { changesCount: 0, diagnostics: [] }. s.tool('greet', ...)is never migrated. AfterimportPathsrewrites the import to@modelcontextprotocol/server, the user is left with a v2 import and an un-migrated v1.tool()call that produces a TypeScript error — with zero codemod diagnostics pointing at it.
Impact
Aliasing McpServer (e.g., as Server to avoid the Mcp prefix, or to disambiguate from another Server class) is uncommon but realistic. The failure mode is safe — the source is left unchanged rather than corrupted, and TypeScript will flag the unmigrated .tool() calls — hence nit severity. But it's a regression from the immediately-preceding commit, it was explicitly predicted in the still-unresolved review comment, and the fix is small.
Fix
Do not change isImportedFromMcp to match either export-or-local name — that would re-introduce the handlerRegistration false positive (and break the new test 'does not rewrite local import when aliased MCP import has same export name'). Instead, give the file-level gates an export-name check:
// importUtils.ts
export function isMcpExportImported(sourceFile: SourceFile, exportName: string): boolean {
return sourceFile.getImportDeclarations().some(imp =>
isAnyMcpSpecifier(imp.getModuleSpecifierValue()) &&
imp.getNamedImports().some(n => n.getName() === exportName)
);
}Use it at mcpServerApi.ts:15 and expressMiddleware.ts:12. (Or add a { matchExportName?: boolean } flag to the existing helper.) A test with import { McpServer as Server } + s.tool(...) → expect registerTool would lock this in.
|
@claude review |
| const hasAlias = namedImports.some(n => n.getAliasNode() !== undefined); | ||
| if (defaultImport || namespaceImport || hasAlias) { | ||
| let effectiveTarget = targetPackage; | ||
| if (mapping.symbolTargetOverrides && !namespaceImport && !defaultImport) { | ||
| const allOverridden = namedImports.length > 0 && namedImports.every(n => n.getName() in mapping.symbolTargetOverrides!); | ||
| if (allOverridden) { | ||
| effectiveTarget = mapping.symbolTargetOverrides[namedImports[0]!.getName()]!; | ||
| } | ||
| } | ||
| imp.setModuleSpecifier(effectiveTarget); | ||
| if (mapping.renamedSymbols) { | ||
| for (const n of namedImports) { | ||
| const newName = mapping.renamedSymbols[n.getName()]; | ||
| if (newName) { | ||
| n.setName(newName); | ||
| } | ||
| } | ||
| if (namespaceImport) { | ||
| diagnostics.push( | ||
| warning( | ||
| filePath, | ||
| line, | ||
| `Namespace import of ${specifier}: exported symbol(s) ${Object.keys(mapping.renamedSymbols).join(', ')} ` + | ||
| `were renamed in ${effectiveTarget}. Update qualified accesses manually.` | ||
| ) | ||
| ); | ||
| } | ||
| } | ||
| changesCount++; | ||
| continue; |
There was a problem hiding this comment.
🟡 When any specifier in a named import is aliased, the in-place setModuleSpecifier branch (line 107) fires, and the allOverridden check at line 110 only redirects to the override package when every specifier is in symbolTargetOverrides — so a mixed aliased import like import { StreamableHTTPServerTransport as T, EventStore } from '.../server/streamableHttp.js' is rewritten to import { NodeStreamableHTTPServerTransport as T, EventStore } from '@modelcontextprotocol/server', which doesn't export NodeStreamableHTTPServerTransport (TS2305), with no diagnostic. The non-aliased path at lines 138-144 already routes per-symbol via mapping.symbolTargetOverrides?.[name] ?? targetPackage; consider falling through to it (it can carry aliases via addPending) or at minimum push a warning(...) when hasAlias && symbolTargetOverrides && !allOverridden. (rewriteExportDeclarations at line 222 has the same all-or-nothing gap for mixed re-exports.)
Extended reasoning...
What the bug is
The newly-added symbolTargetOverrides mechanism lets a single v1 module map different symbols to different v2 packages — currently used only for @modelcontextprotocol/sdk/server/streamableHttp.js, where StreamableHTTPServerTransport goes to @modelcontextprotocol/node while everything else (EventStore, StreamId, etc.) goes to @modelcontextprotocol/server. The non-aliased path at importPaths.ts:138-144 handles this correctly by routing each specifier individually:
const symbolTarget = mapping.symbolTargetOverrides?.[name] ?? targetPackage;
addPending(symbolTarget, [resolvedName], specifierTypeOnly);But line 106 computes hasAlias = namedImports.some(n => n.getAliasNode() !== undefined), and if any specifier is aliased the whole declaration is sent into the in-place setModuleSpecifier branch (107-136). That branch can only set one module specifier, and the allOverridden guard at line 110 only redirects to the override package when every named import is a key in symbolTargetOverrides. For a mixed import, allOverridden is false, effectiveTarget stays at the base targetPackage (@modelcontextprotocol/server), and the renamed transport ends up imported from the wrong package.
Code path that triggers it
Input:
import { StreamableHTTPServerTransport as T, EventStore } from '@modelcontextprotocol/sdk/server/streamableHttp.js';(Note that aliasing just one of the two specifiers is enough — .some() at line 106 fires on a single match.)
mapping = { target: '@modelcontextprotocol/server', renamedSymbols: { StreamableHTTPServerTransport: 'NodeStreamableHTTPServerTransport' }, symbolTargetOverrides: { StreamableHTTPServerTransport: '@modelcontextprotocol/node' } }.- Lines 100-104:
renameAllReferences(sourceFile, 'StreamableHTTPServerTransport', 'NodeStreamableHTTPServerTransport')runs (no-op here since the body uses the aliasT). - Line 106:
hasAlias = true→ enters the in-place branch. - Line 110:
namedImports.every(n => n.getName() in {StreamableHTTPServerTransport: ...})—EventStoreis not a key →allOverridden = false. effectiveTargetstays'@modelcontextprotocol/server'; line 115imp.setModuleSpecifier('@modelcontextprotocol/server').- Lines 117-121: the specifier
StreamableHTTPServerTransportis renamed toNodeStreamableHTTPServerTransport(aliasTpreserved).
Output:
import { NodeStreamableHTTPServerTransport as T, EventStore } from '@modelcontextprotocol/server';→ TS2305: Module '"@modelcontextprotocol/server"' has no exported member 'NodeStreamableHTTPServerTransport'. No diagnostic is emitted (the only warning(...) in this branch is gated on namespaceImport).
Why existing code/tests don't catch it
The two existing tests bracket this case without covering it: 'handles aliased renamedSymbols correctly' (importPaths.test.ts:165) tests a single aliased specifier (so allOverridden is true → routes to /node ✓), and 'splits streamableHttp import: transport to /node, types to /server' (importPaths.test.ts:177) tests a mixed but unaliased import (so hasAlias is false → per-symbol addPending path ✓). The aliased-and-mixed combination falls between them.
The same all-or-nothing logic appears in rewriteExportDeclarations (line 222-227) — export { StreamableHTTPServerTransport, EventStore } from '.../streamableHttp.js' becomes export { NodeStreamableHTTPServerTransport as StreamableHTTPServerTransport, EventStore } from '@modelcontextprotocol/server' with the same TS2305 — and in rewriteMockCall / rewriteDynamicImports (mockPaths.ts), though those cases are even rarer.
Impact
Nit severity: only one IMPORT_MAP entry has symbolTargetOverrides, importing the transport and EventStore/StreamId from streamableHttp.js in a single declaration with at least one alias is uncommon, and the resulting TS2305 is immediate and points directly at the offending specifier. The codemod is also documented as 80-90% coverage. But it's an internal inconsistency in newly-added logic — the per-symbol routing was clearly intended (it's implemented for the common case three lines below), and this branch silently produces broken output with no diagnostic.
Step-by-step proof
Input: import { StreamableHTTPServerTransport as Transport, EventStore as Store } from '@modelcontextprotocol/sdk/server/streamableHttp.js';
- importPaths.ts:106 —
namedImports = [⟨StreamableHTTPServerTransport as Transport⟩, ⟨EventStore as Store⟩];.some(n => n.getAliasNode())→ true. - :107 —
defaultImport=undefined,namespaceImport=undefined,hasAlias=true→ enter branch. - :109 —
mapping.symbolTargetOverridestruthy,!namespaceImport && !defaultImport→ enter. - :110 —
namedImports.every(...):'StreamableHTTPServerTransport' in overrides✓,'EventStore' in overrides✗ → false. - :111-113 — skipped;
effectiveTarget = '@modelcontextprotocol/server'. - :115 —
imp.setModuleSpecifier('@modelcontextprotocol/server'). - :117-121 —
n.getName()='StreamableHTTPServerTransport'→n.setName('NodeStreamableHTTPServerTransport');n.getName()='EventStore'→ no rename. - :123 —
namespaceImportfalsy → no warning. - :134-135 —
changesCount++;continue.
Final: import { NodeStreamableHTTPServerTransport as Transport, EventStore as Store } from '@modelcontextprotocol/server'; — NodeStreamableHTTPServerTransport is exported from @modelcontextprotocol/node, not /server. diagnostics = [].
Fix
Either drop the hasAlias short-circuit and let aliased named-only imports fall through to the per-symbol addPending loop (extending PendingImport to carry an alias?: string so addOrMergeImport can pass { name, alias } specifiers — ts-morph's insertImportDeclaration already accepts that shape), or keep the branch but split the declaration in place when symbolTargetOverrides && !allOverridden: move the overridden specifiers to a new import for the override target and leave the rest on targetPackage. At minimum, push a warning(filePath, line, ...) for this case so it's not silent.
| function collectFactorySymbols(factoryArg: import('ts-morph').Node): string[] { | ||
| const symbols: string[] = []; | ||
| factoryArg.forEachDescendant(node => { | ||
| if (Node.isPropertyAssignment(node) || Node.isShorthandPropertyAssignment(node)) { | ||
| symbols.push(node.getName()); | ||
| } | ||
| }); | ||
| return symbols; | ||
| } |
There was a problem hiding this comment.
🟡 collectFactorySymbols uses forEachDescendant to collect property names at any depth, so for the canonical class-mocking pattern vi.mock('.../server/streamableHttp.js', () => ({ StreamableHTTPServerTransport: vi.fn().mockImplementation(() => ({ handleRequest: vi.fn() })) })) the nested handleRequest key is collected, allOverridden (line 105) is false, and the mock specifier is rewritten to @modelcontextprotocol/server instead of @modelcontextprotocol/node — so the mock no longer intercepts the module the migrated code actually imports the transport from. Only inspect the top-level properties of the factory's returned object literal (find the return value and iterate .getProperties()); the same depth issue in renameSymbolsInFactory (lines 131-153) would also rename a nested { McpError: ... } instance-property key inside a .mockImplementation body.
Extended reasoning...
What the bug is
collectFactorySymbols (mockPaths.ts:121-129) does:
factoryArg.forEachDescendant(node => {
if (Node.isPropertyAssignment(node) || Node.isShorthandPropertyAssignment(node)) {
symbols.push(node.getName());
}
});forEachDescendant visits every PropertyAssignment/ShorthandPropertyAssignment at any nesting depth inside the factory argument — not just the top-level keys of the object the factory returns. The result feeds the symbolTargetOverrides heuristic at line 105: factorySymbols.every(s => s in resolved.symbolTargetOverrides!). That check is meant to ask "is every export this factory mocks one that moved to the override package?" — but because nested object-literal keys (instance methods, config objects, etc.) leak into the list, the .every() is false whenever the factory contains any nested object literal.
Code path that triggers it
The canonical vitest/jest pattern for mocking a class is:
vi.mock('@modelcontextprotocol/sdk/server/streamableHttp.js', () => ({
StreamableHTTPServerTransport: vi.fn().mockImplementation(() => ({
handleRequest: vi.fn(),
sessionId: 'test-session'
}))
}));Walking through rewriteMockCall:
resolveTarget(...)→{ target: '@modelcontextprotocol/server', renamedSymbols: { StreamableHTTPServerTransport: 'NodeStreamableHTTPServerTransport' }, symbolTargetOverrides: { StreamableHTTPServerTransport: '@modelcontextprotocol/node' } }.- Line 103-104:
collectFactorySymbols(args[1])walks the arrow function.forEachDescendantvisits the outerStreamableHTTPServerTransport: ...assignment and the innerhandleRequest: vi.fn()andsessionId: 'test-session'assignments → returns['StreamableHTTPServerTransport', 'handleRequest', 'sessionId']. - Line 105:
['StreamableHTTPServerTransport', 'handleRequest', 'sessionId'].every(s => s in { StreamableHTTPServerTransport: '@modelcontextprotocol/node' })→'handleRequest' in {...}is false →allOverridden = false. effectiveTargetstays atresolved.target='@modelcontextprotocol/server'.- Line 110:
firstArg.setLiteralValue('@modelcontextprotocol/server'). renameSymbolsInFactorythen renames the top-level key toNodeStreamableHTTPServerTransport.
Output:
vi.mock('@modelcontextprotocol/server', () => ({
NodeStreamableHTTPServerTransport: vi.fn().mockImplementation(() => ({ handleRequest: vi.fn(), sessionId: 'test-session' }))
}));But the migrated production code now does import { NodeStreamableHTTPServerTransport } from '@modelcontextprotocol/node' (per importPathsTransform's symbolTargetOverrides handling). The mock intercepts @modelcontextprotocol/server, not @modelcontextprotocol/node, so vitest/jest loads the real transport — the test silently exercises real I/O (or throws on construction) with no codemod diagnostic explaining why.
Why existing code doesn't prevent it
The existing test 'renames symbols in vi.doMock factory for streamableHttp' (mockPaths.test.ts:31-41) uses a flat factory () => ({ StreamableHTTPServerTransport: mockTransport }) with no nested properties, so collectFactorySymbols returns exactly ['StreamableHTTPServerTransport'], allOverridden is true, and /node is correctly chosen. Nothing exercises a nested .mockImplementation(() => ({...})) body, even though that's the dominant real-world shape for mocking a class constructor.
The same forEachDescendant depth issue exists in renameSymbolsInFactory (lines 131-153): a nested { McpError: someValue } inside a .mockImplementation body — i.e., an instance property on the mock, not a module export — would be renamed to ProtocolError, silently changing the mock's instance shape. That collision is less likely than the handleRequest case but is the same root cause.
Impact
When triggered, the user's migrated test file mocks the wrong module: the mock factory provides NodeStreamableHTTPServerTransport on @modelcontextprotocol/server (which doesn't export it), while the code under test imports it from @modelcontextprotocol/node and gets the real class. The failure surfaces as a test failure the user has to debug manually, with no codemod diagnostic — exactly the silent-test-behavior-change a migration tool should avoid. Nit severity because: only one symbolTargetOverrides entry exists today, it only affects test files, and the user's own test suite will catch the broken mock (loudly, if not informatively). But vi.fn().mockImplementation(() => ({...methods})) is the most common shape for class mocks, so the heuristic is broken for the majority of real inputs it was added to handle.
Step-by-step proof
Input:
vi.mock('@modelcontextprotocol/sdk/server/streamableHttp.js', () => ({
StreamableHTTPServerTransport: vi.fn().mockImplementation(() => ({ handleRequest: vi.fn() }))
}));- mockPaths.ts:80 —
specifier = '@modelcontextprotocol/sdk/server/streamableHttp.js';isSdkSpecifier→ true. - mockPaths.ts:82 —
resolveTargetreturns{ target: '@modelcontextprotocol/server', renamedSymbols: {...}, symbolTargetOverrides: { StreamableHTTPServerTransport: '@modelcontextprotocol/node' } }. - mockPaths.ts:102-103 —
resolved.symbolTargetOverridestruthy,args.length === 2→ callcollectFactorySymbols(args[1]). - mockPaths.ts:123 —
forEachDescendanton the arrow function visits, in order: the outer ObjectLiteralExpression'sPropertyAssignment(StreamableHTTPServerTransport: ...) → push'StreamableHTTPServerTransport'; then descends intovi.fn().mockImplementation(() => ({...})), reaches the inner ObjectLiteralExpression'sPropertyAssignment(handleRequest: vi.fn()) → push'handleRequest'. Returns['StreamableHTTPServerTransport', 'handleRequest']. - mockPaths.ts:105 —
.every(s => s in { StreamableHTTPServerTransport: '@modelcontextprotocol/node' }):'StreamableHTTPServerTransport'✓,'handleRequest'✗ → false. - mockPaths.ts:106 — skipped;
effectiveTargetremains'@modelcontextprotocol/server'. - mockPaths.ts:110 — specifier rewritten to
'@modelcontextprotocol/server'. - mockPaths.ts:115 —
renameSymbolsInFactoryrenames the outer key toNodeStreamableHTTPServerTransport(and leaveshandleRequestalone since it's not inallRenames).
Final: vi.mock('@modelcontextprotocol/server', () => ({ NodeStreamableHTTPServerTransport: ... })) — wrong target package.
Fix
Restrict collection to the top-level properties of the factory's returned object literal. Locate the returned expression (arrow body if a parenthesized object, or the ReturnStatement argument if a block body), guard with Node.isObjectLiteralExpression, and iterate .getProperties() filtered to PropertyAssignment | ShorthandPropertyAssignment:
function collectFactorySymbols(factoryArg: Node): string[] {
let returned: Node | undefined;
if (Node.isArrowFunction(factoryArg) || Node.isFunctionExpression(factoryArg)) {
const body = factoryArg.getBody();
returned = Node.isBlock(body)
? body.getStatements().find(Node.isReturnStatement)?.getExpression()
: body;
if (returned && Node.isParenthesizedExpression(returned)) returned = returned.getExpression();
}
if (!returned || !Node.isObjectLiteralExpression(returned)) return [];
return returned
.getProperties()
.filter(p => Node.isPropertyAssignment(p) || Node.isShorthandPropertyAssignment(p))
.map(p => (p as any).getName());
}Apply the same top-level-only restriction to renameSymbolsInFactory (or have it operate on the same returned.getProperties() list) so nested instance-property keys aren't renamed. Add a test with vi.fn().mockImplementation(() => ({ handleRequest: vi.fn() })) asserting the specifier becomes '@modelcontextprotocol/node'.
|
@claude review |
| if (result.filesChanged === 0 && result.diagnostics.length === 0) { | ||
| console.log('No changes needed — code already migrated or no SDK imports found.\n'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 The early-return condition checks filesChanged === 0 && diagnostics.length === 0 but not result.packageJsonChanges — yet updatePackageJson() runs unconditionally inside run() (runner.ts:111) and writes to disk whenever @modelcontextprotocol/sdk is in package.json, regardless of whether any source files changed. So if a user points the codemod at already-migrated source (or a subdir with no SDK imports) but findPackageJson() walks up to a package.json that still lists the v1 SDK, the dependency is silently removed on disk while the CLI prints "No changes needed" and never reaches the package.json updated: block at line 118. Add && !result.packageJsonChanges to the condition.
Extended reasoning...
What the bug is
The CLI's "nothing to do" early-return at cli.ts:73-76 checks only two conditions:
if (result.filesChanged === 0 && result.diagnostics.length === 0) {
console.log('No changes needed — code already migrated or no SDK imports found.\n');
return;
}It does not check result.packageJsonChanges. But updatePackageJson() is called unconditionally inside run() (runner.ts:111) before the runner returns, and it performs writeFileSync() on disk (packageJsonUpdater.ts:62) whenever (a) findPackageJson() locates a package.json and (b) that package.json contains @modelcontextprotocol/sdk in dependencies or devDependencies — independent of whether any source transforms fired. So the CLI can write to disk and then tell the user nothing happened.
Code path that triggers it
updatePackageJson() (packageJsonUpdater.ts:15-69) early-returns undefined only when no package.json is found or the v1 SDK isn't in it. Otherwise it always:
- Removes
@modelcontextprotocol/sdkfrom deps/devDeps (line 53-54). - Adds whatever is in
usedPackages(which is empty if no source imports were rewritten —allUsedPackagesis only populated by transform results). - Calls
writeFileSync(pkgJsonPath, output)if!dryRun(line 62). - Returns
{ removed: ['@modelcontextprotocol/sdk'], added: [], packageJsonPath }.
This is called at runner.ts:111 with no gating other than dryRun. The result is threaded through to result.packageJsonChanges, but the CLI early-return at line 73 doesn't look at it, so control never reaches the if (result.packageJsonChanges) reporting block at lines 118-132.
Why nothing else prevents it
The triggers are realistic:
- Second run / idempotency check: user runs the codemod twice. On the second run, all source files are already v2 (so
filesChanged = 0,diagnostics = []), but if package.json was reverted or partially staged, the v1 SDK is still listed → silently removed, CLI says "No changes needed". - Subdirectory target: user runs
mcp-codemod v1-to-v2 ./scriptswhere./scriptscontains no SDK imports.findPackageJson()walks up from the target dir (projectAnalyzer.ts:8-17) and finds the project-root package.json with@modelcontextprotocol/sdk→ removes it (adds nothing, sinceallUsedPackagesis empty), CLI says "No changes needed". --transformsfilter: user runs with e.g.--transforms context(which doesn't populateusedPackages) on already-migrated context code → same outcome.
The dedicated unit test 'removes v1 SDK even when no v2 packages are detected' (packageJsonUpdater.test.ts:202-217) confirms that updatePackageJson(dir, new Set(), false) writes to disk and returns a non-undefined result with added: [] — exactly the state that the CLI early-return then hides.
Step-by-step proof
Project layout:
my-project/
package.json { "dependencies": { "@modelcontextprotocol/sdk": "^1.0.0" } }
scripts/
build.ts // no SDK imports
User runs mcp-codemod v1-to-v2 ./scripts:
- cli.ts:65 —
run(migration, { targetDir: '/my-project/scripts', ... }). - runner.ts:52 — glob finds
scripts/build.ts; it has no SDK imports, so every transform returns{ changesCount: 0, diagnostics: [] }.filesChanged = 0,allDiagnostics = [],allUsedPackages = ∅. - runner.ts:111 —
updatePackageJson('/my-project/scripts', ∅, false):findPackageJson('/my-project/scripts')walks up to/my-project/package.json.inDeps = true(v1 SDK present), so it does not early-return.packagesToAdd = [](empty set, nothing inV2_PACKAGE_VERSIONS).delete deps['@modelcontextprotocol/sdk'];removed = ['@modelcontextprotocol/sdk'].!dryRun→writeFileSync('/my-project/package.json', ...)— disk mutated.- Returns
{ added: [], removed: ['@modelcontextprotocol/sdk'], packageJsonPath: '...' }.
run()returns{ filesChanged: 0, totalChanges: 0, diagnostics: [], packageJsonChanges: {...} }.- cli.ts:73 —
0 === 0 && 0 === 0→ true. PrintsNo changes needed — code already migrated or no SDK imports found.and returns. - The
package.json updated: Removed: @modelcontextprotocol/sdkblock at lines 118-132 is never reached.
Net effect: package.json was modified on disk; the CLI explicitly told the user nothing was modified.
Impact
Nit severity. The mutation itself is benign (removing a dependency the user is migrating away from), git-visible, and easily reverted. The trigger is narrow (no source-file changes + stale v1 dep in package.json). But it's a clear correctness bug in user-facing output — a CLI that performs a disk write while reporting "No changes needed" is exactly the kind of contradiction that erodes trust in a migration tool, especially given the PR's emphasis on dry-run safety and explicit package.json reporting.
Fix
One-clause addition to the early-return condition:
if (result.filesChanged === 0 && result.diagnostics.length === 0 && !result.packageJsonChanges) {
console.log('No changes needed — code already migrated or no SDK imports found.\n');
return;
}With this, control falls through to the existing if (result.packageJsonChanges) block at line 118, which already handles the added.length > 0 / removed.length > 0 cases correctly (and the "Run your package manager" hint at line 138).
| export type { | ||
| Diagnostic, | ||
| FileResult, | ||
| Migration, | ||
| RunnerOptions, | ||
| RunnerResult, | ||
| Transform, | ||
| TransformContext, | ||
| TransformResult | ||
| } from './types.js'; |
There was a problem hiding this comment.
🟡 RunnerResult is exported and has a packageJsonChanges?: PackageJsonChange field, but PackageJsonChange itself is missing from this type re-export list — every other type reachable from RunnerResult (Diagnostic, FileResult) is exported. Consumers can work around it via NonNullable<RunnerResult['packageJsonChanges']>, but for consistency add PackageJsonChange here.
Extended reasoning...
What the gap is
packages/codemod/src/index.ts is the package's sole public entrypoint (per package.json exports: { '.': ... }). It re-exports RunnerResult from ./types.js, and RunnerResult (types.ts:58-64) has a packageJsonChanges?: PackageJsonChange field. But PackageJsonChange is not in the export type { ... } list at index.ts:3-12 — every other type referenced by RunnerResult's fields (Diagnostic, FileResult) is.
Why it's an oversight, not a design choice
The packageJsonChanges feature was added in a later commit on this branch (2e349dd add package.json transform), after the initial index.ts was written. The type list at index.ts:3-12 was clearly intended to expose every type a programmatic-API consumer might need to annotate — it includes Diagnostic, FileResult, TransformResult, TransformContext, etc. — and PackageJsonChange is the one constituent of RunnerResult that was missed when the new field was added.
Why nothing else exposes it
package.json only exports '.' → dist/index.mjs, so consumers cannot deep-import @modelcontextprotocol/codemod/types.js to get the type directly. The .d.mts bundler will include the interface structurally (so result.packageJsonChanges.added etc. type-check fine), but there's no named export, so import type { PackageJsonChange } from '@modelcontextprotocol/codemod' fails with TS2305: Module has no exported member 'PackageJsonChange'.
Step-by-step proof
A user following the PR description's Programmatic API example wants to write a helper:
import { run, getMigration, type RunnerResult, type PackageJsonChange } from '@modelcontextprotocol/codemod';
function reportPkgChanges(changes: PackageJsonChange): void {
console.log(`Removed: ${changes.removed.join(', ')}`);
console.log(`Added: ${changes.added.join(', ')}`);
}
const result = run(getMigration('v1-to-v2')!, { targetDir: './src' });
if (result.packageJsonChanges) reportPkgChanges(result.packageJsonChanges);- TypeScript resolves
@modelcontextprotocol/codemod→dist/index.d.mts. index.d.mtsre-exportsRunnerResult,Diagnostic,FileResult, etc., but notPackageJsonChange.import type { PackageJsonChange }→TS2305.- Workaround:
type PackageJsonChange = NonNullable<RunnerResult['packageJsonChanges']>— works, but is exactly the boilerplate the explicit export list is meant to avoid.
Impact
Nothing is broken: the type is structurally reachable via indexed access, no runtime behavior is affected, and there are no existing consumers (new package). This is purely an API-surface ergonomics/consistency gap — hence nit.
Fix
One word:
export type {
Diagnostic,
FileResult,
Migration,
PackageJsonChange,
RunnerOptions,
RunnerResult,
Transform,
TransformContext,
TransformResult
} from './types.js';| ); | ||
| } | ||
|
|
||
| const packageJsonChanges = updatePackageJson(options.targetDir, allUsedPackages, options.dryRun ?? false); |
There was a problem hiding this comment.
🟡 updatePackageJson() is called unconditionally after the transform loop, but only the imports/mock-paths transforms populate allUsedPackages — so mcp-codemod v1-to-v2 ./src --transforms symbols (a documented selective-execution path) leaves all v1 @modelcontextprotocol/sdk/... import paths in source while still deleting @modelcontextprotocol/sdk from package.json, breaking the build. Consider gating the package.json update on 'imports' being in enabledTransforms (or skipping when allUsedPackages.size === 0).
Extended reasoning...
What the bug is
runner.ts:111 calls updatePackageJson(options.targetDir, allUsedPackages, options.dryRun ?? false) unconditionally after the per-file transform loop, with no check on which transforms were actually enabled. Of the nine transforms, only importPathsTransform and mockPathsTransform return a usedPackages set in their TransformResult (types.ts: usedPackages?: Set<string>); symbolRenames, removedApis, mcpServerApi, handlerRegistration, schemaParamRemoval, expressMiddleware, and contextTypes return only { changesCount, diagnostics }. So if --transforms excludes imports and mock-paths, allUsedPackages stays empty.
updatePackageJson() (packageJsonUpdater.ts:30-55) doesn't treat an empty set as a no-op. Its only short-circuit is "if v1 SDK is not in deps/devDeps, return undefined". When @modelcontextprotocol/sdk is present, it proceeds to if (inDeps) delete deps![V1_PACKAGE] regardless of whether any v2 packages were detected, then writes the file. The unit test 'removes v1 SDK even when no v2 packages are detected' (packageJsonUpdater.test.ts) explicitly locks in this behavior with updatePackageJson(dir, new Set(), false).
Code path that triggers it
The --transforms <ids> flag is a documented selective-execution feature (PR description CLI Usage: mcp-codemod v1-to-v2 ./src --transforms imports,symbols,context), and cli.test.ts exercises run(migration, { targetDir: dir, transforms: ['symbols'] }). With that invocation:
runner.ts:21—enabledTransforms = [symbolRenamesTransform].- Per-file loop runs only
symbolRenamesTransform, which returns{ changesCount, diagnostics }with nousedPackagesfield. Theif (result.usedPackages)block at runner.ts:75-79 never executes. - After the loop,
allUsedPackagesis an emptySet. runner.ts:111—updatePackageJson(targetDir, new Set(), false).findPackageJsonwalks up from./srcand findspackage.jsoncontaining"@modelcontextprotocol/sdk": "^1.0.0".inDepsis true →packagesToAdd = [](empty set filtered) →added = []→delete deps['@modelcontextprotocol/sdk']→ file written.
Why existing code doesn't prevent it
There is no coupling between the package.json mutation and which transforms ran. The PR description's Design Decision #6 says "As transforms rewrite imports, they track which v2 packages are targeted" — i.e., the package.json update is conceptually downstream of import rewriting — but the runner doesn't gate it on imports being enabled. There's also no --no-package-json opt-out flag. The only existing test that passes transforms: ['symbols'] (cli.test.ts: "accepts valid transform IDs") doesn't write a package.json, so findPackageJson returns undefined (the temp dir is fresh and the .git boundary stops the upward walk in the other tests, but in this test there's simply no package.json) and the bug isn't observed.
Step-by-step proof
Project layout:
my-app/
├── package.json { "dependencies": { "@modelcontextprotocol/sdk": "^1.0.0" } }
└── src/server.ts import { McpError } from '@modelcontextprotocol/sdk/types.js'; throw new McpError(1, 'e');
User runs: mcp-codemod v1-to-v2 ./src --transforms symbols
analyzeProject('./src')→findPackageJsonwalks up from./src, findsmy-app/package.json.enabledTransforms = [symbolRenamesTransform](validation passes,'symbols'is a valid ID).symbolRenamesTransformfindsMcpErrorin the SDK import, renames it toProtocolError, returns{ changesCount: 1, diagnostics: [] }— nousedPackages.allUsedPackages = Set {}.updatePackageJson('./src', Set {}, false):findPackageJson→my-app/package.json;inDeps = true;packagesToAdd = [].filter(...) = [];delete deps['@modelcontextprotocol/sdk']; write file.- CLI prints
package.json updated: Removed: @modelcontextprotocol/sdk(with noAdded:line).
Result on disk:
// src/server.ts — import path NOT rewritten (imports transform didn't run)
import { ProtocolError } from '@modelcontextprotocol/sdk/types.js';
throw new ProtocolError(1, 'e');// package.json — dependency removed
{ "dependencies": {} }→ tsc / npm install now fails: Cannot find module '@modelcontextprotocol/sdk/types.js'.
Impact
This is a destructive side-effect orthogonal to what the user asked for: they requested only symbol renames, but the tool also deleted a dependency the source code still requires. It contradicts the PR's "conservative by default" claim. Mitigating factors: --transforms without imports is an advanced/niche path; the CLI does print Removed: @modelcontextprotocol/sdk so it's not silent; the breakage surfaces immediately on the next typecheck/install; and git checkout package.json is a trivial recovery. Hence nit rather than blocking — but it's a real correctness gap in newly-added orchestration logic.
Fix
Gate the call on the imports transform being enabled (since that's the transform that semantically "owns" the v1→v2 dependency swap):
const importsEnabled = enabledTransforms.some(t => t.id === 'imports');
const packageJsonChanges = importsEnabled
? updatePackageJson(options.targetDir, allUsedPackages, options.dryRun ?? false)
: undefined;Alternatively, have updatePackageJson() early-return when usedPackages.size === 0 — though note that would also skip removal in the (legitimate) case where every v1 import maps to a removed/private path and the user did run imports, so gating on the transform ID is cleaner.
| if (sdkImports.length === 0) { | ||
| return { changesCount, diagnostics }; | ||
| } |
There was a problem hiding this comment.
🟡 The early return when sdkImports.length === 0 omits usedPackages from the returned TransformResult, even though rewriteExportDeclarations() on line 36 just populated it via usedPackages.add(targetPackage). For a pure barrel file (e.g., export { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js' with no SDK imports), the v2 packages it re-exports from are tracked locally but never returned to the runner, so updatePackageJson() won't add them. The final return at line 175 includes usedPackages, so this is just a missed field on the early-return path — add , usedPackages here.
Extended reasoning...
What the bug is
importPathsTransform.apply() creates usedPackages = new Set<string>() at line 25, then calls rewriteExportDeclarations(sdkExports, ..., usedPackages) at line 36, which mutates the set via usedPackages.add(targetPackage) (line ~232) for each rewritten re-export. Immediately afterward, line 38-40 does:
if (sdkImports.length === 0) {
return { changesCount, diagnostics };
}— omitting usedPackages. TransformResult.usedPackages is optional (Set<string> | undefined), so this typechecks. The final return at line 175, by contrast, correctly returns { changesCount, diagnostics, usedPackages }, confirming this is an oversight in the early-return path rather than intentional.
Code path that triggers it
runner.ts:75-79 reads each transform result with:
if (result.usedPackages) {
for (const pkg of result.usedPackages) {
allUsedPackages.add(pkg);
}
}When result.usedPackages is undefined, the packages are silently skipped. allUsedPackages is then passed to updatePackageJson(options.targetDir, allUsedPackages, ...), which determines which v2 packages get added to package.json.
Why nothing else prevents it
The only other place usedPackages is populated in this transform is the imports loop (lines 116, 144), which is unreachable when sdkImports.length === 0. mockPathsTransform also tracks usedPackages but only for vi.mock/jest.mock/dynamic-import() calls — not static export ... from declarations. So for a file containing only export { X } from '@modelcontextprotocol/sdk/...', no transform reports the target package back to the runner.
Step-by-step proof
Consider a project that centralizes all SDK access through a barrel file src/mcp.ts:
export { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
export { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js';…and all other files in the project do import { McpServer } from './mcp.js' (no direct SDK imports).
importPathsTransform.apply()onsrc/mcp.ts:sdkImports = [],sdkExports = [<2 decls>].- Line 36:
rewriteExportDeclarationsrewrites both specifiers to'@modelcontextprotocol/server'and callsusedPackages.add('@modelcontextprotocol/server')twice.usedPackages = {'@modelcontextprotocol/server'},changesCount = 2. - Line 38:
sdkImports.length === 0→ returns{ changesCount: 2, diagnostics: [] }.usedPackagesis dropped. runner.ts:75:result.usedPackagesisundefined→ loop skipped.allUsedPackagesstays empty for this file.- No other file has SDK imports (they all go through the barrel), so after the project loop
allUsedPackagesis still empty. updatePackageJson(dir, new Set(), false)removes@modelcontextprotocol/sdkfromdependenciesand adds nothing (the test'removes v1 SDK even when no v2 packages are detected'confirms this behavior).- Result: the rewritten barrel now does
export { McpServer } from '@modelcontextprotocol/server', butpackage.jsonno longer has any MCP dependency. The user's nextnpm install/ build fails with a missing-module error.
Impact
Bounded — hence nit. allUsedPackages is accumulated project-wide, so the gap only manifests when no other file in the project directly imports from the same v2 package (otherwise that file's importPathsTransform run would track it). That's uncommon but realistic for projects that deliberately funnel all SDK access through a single barrel/wrapper module. The resulting missing-dependency error is loud and easy to diagnose, and the fix is a single-word addition.
Fix
if (sdkImports.length === 0) {
return { changesCount, diagnostics, usedPackages };
}1 participant
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.