feat: add v2 codemod draft #1950
Code review found 3 potential issues
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 3 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts:206-216 |
Re-exports from SDK don't apply SIMPLE_RENAMES — only IMPORT_MAP.renamedSymbols |
| 🟡 Nit | packages/codemod/src/migrations/v1-to-v2/transforms/mockPaths.ts:188-197 |
rewriteDynamicImports: aliased binding looked up by wrong key — getPropertyNameNode() branch is dead code |
| 🟡 Nit | packages/codemod/src/utils/importUtils.ts:76-81 |
isImportedFromMcp checks export name, not local binding |
Annotations
Check warning on line 216 in packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts
claude / Claude Code Review
Re-exports from SDK don't apply SIMPLE_RENAMES — only IMPORT_MAP.renamedSymbols
The fix for comment 3136362554 handles `mapping.renamedSymbols` for re-exports, but `SIMPLE_RENAMES` (McpError→ProtocolError, JSONRPCError→JSONRPCErrorResponse, ResourceReference→ResourceTemplateReference, etc.) are still never applied to `export { ... } from` declarations — `IMPORT_MAP['.../types.js']` has no `renamedSymbols`, and `symbolRenamesTransform` only iterates `getImportDeclarations()`. So `export { McpError } from '@modelcontextprotocol/sdk/types.js'` becomes `export { McpError } from
Check warning on line 197 in packages/codemod/src/migrations/v1-to-v2/transforms/mockPaths.ts
claude / Claude Code Review
rewriteDynamicImports: aliased binding looked up by wrong key — getPropertyNameNode() branch is dead code
For an already-aliased dynamic-import destructuring like `const { StreamableHTTPServerTransport: MyTransport } = await import(...)`, `element.getName()` returns the *local* binding (`'MyTransport'`), so `renamedSymbols['MyTransport']` is undefined and the element is skipped — leaving `{ StreamableHTTPServerTransport: MyTransport } = await import('@modelcontextprotocol/node')` (TS2339). This makes the `if (element.getPropertyNameNode())` branch added for the earlier shorthand fix effectively unre
Check warning on line 81 in packages/codemod/src/utils/importUtils.ts
claude / Claude Code Review
isImportedFromMcp checks export name, not local binding
`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` the