-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: add v2 codemod draft #1950
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 12 commits
05eadea
44ac609
a202bba
6386627
f7d13d3
834bfab
1f2b15a
b77cc9b
9c292f8
50e24f1
44b2c67
cfac197
321e918
dc25694
36a1774
174a8b6
cf990e3
b8e397a
1a46c4c
9ce1fcd
2e349dd
c9ef51e
96b1481
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| // @ts-check | ||
|
|
||
| import baseConfig from '@modelcontextprotocol/eslint-config'; | ||
|
|
||
| export default [...baseConfig]; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| { | ||
| "name": "@modelcontextprotocol/codemod", | ||
| "version": "2.0.0-alpha.0", | ||
| "description": "Codemod to migrate MCP TypeScript SDK code from v1 to v2", | ||
| "license": "MIT", | ||
| "author": "Anthropic, PBC (https://anthropic.com)", | ||
| "homepage": "https://modelcontextprotocol.io", | ||
| "bugs": "https://github.com/modelcontextprotocol/typescript-sdk/issues", | ||
| "type": "module", | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "git+https://github.com/modelcontextprotocol/typescript-sdk.git" | ||
| }, | ||
| "engines": { | ||
| "node": ">=20" | ||
| }, | ||
| "keywords": [ | ||
| "modelcontextprotocol", | ||
| "mcp", | ||
| "codemod", | ||
| "migration" | ||
| ], | ||
| "bin": { | ||
| "mcp-codemod": "./dist/cli.mjs" | ||
| }, | ||
| "exports": { | ||
| ".": { | ||
| "types": "./dist/index.d.mts", | ||
| "import": "./dist/index.mjs" | ||
| } | ||
| }, | ||
| "files": [ | ||
| "dist" | ||
| ], | ||
| "scripts": { | ||
| "typecheck": "tsgo -p tsconfig.json --noEmit", | ||
| "build": "tsdown", | ||
| "build:watch": "tsdown --watch", | ||
| "prepack": "pnpm run build", | ||
| "lint": "eslint src/ && prettier --ignore-path ../../.prettierignore --check .", | ||
| "lint:fix": "eslint src/ --fix && prettier --ignore-path ../../.prettierignore --write .", | ||
| "check": "pnpm run typecheck && pnpm run lint", | ||
| "test": "vitest run", | ||
| "test:watch": "vitest" | ||
| }, | ||
| "dependencies": { | ||
| "commander": "^13.0.0", | ||
| "ts-morph": "^28.0.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@modelcontextprotocol/tsconfig": "workspace:^", | ||
| "@modelcontextprotocol/vitest-config": "workspace:^", | ||
| "@modelcontextprotocol/eslint-config": "workspace:^", | ||
| "@eslint/js": "catalog:devTools", | ||
| "@typescript/native-preview": "catalog:devTools", | ||
| "eslint": "catalog:devTools", | ||
| "eslint-config-prettier": "catalog:devTools", | ||
| "eslint-plugin-n": "catalog:devTools", | ||
| "prettier": "catalog:devTools", | ||
| "tsdown": "catalog:devTools", | ||
| "tsx": "catalog:devTools", | ||
| "typescript": "catalog:devTools", | ||
| "typescript-eslint": "catalog:devTools", | ||
| "vitest": "catalog:devTools" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| #!/usr/bin/env node | ||
|
|
||
| import { existsSync, statSync } from 'node:fs'; | ||
| import { createRequire } from 'node:module'; | ||
| import path from 'node:path'; | ||
|
|
||
| import { Command } from 'commander'; | ||
|
|
||
| import { listMigrations } from './migrations/index.js'; | ||
| import { run } from './runner.js'; | ||
| import { DiagnosticLevel } from './types.js'; | ||
| import { formatDiagnostic } from './utils/diagnostics.js'; | ||
|
|
||
| const require = createRequire(import.meta.url); | ||
| const { version } = require('../package.json') as { version: string }; | ||
|
|
||
| const program = new Command(); | ||
|
|
||
| program.name('mcp-codemod').description('Codemod to migrate MCP TypeScript SDK code between versions').version(version); | ||
|
|
||
| for (const [name, migration] of listMigrations()) { | ||
| program | ||
| .command(`${name} [target-dir]`) | ||
| .description(migration.description) | ||
| .option('-d, --dry-run', 'Preview changes without writing files') | ||
| .option('-t, --transforms <ids>', 'Comma-separated transform IDs to run (default: all)') | ||
| .option('-v, --verbose', 'Show detailed per-change output') | ||
| .option('--ignore <patterns...>', 'Additional glob patterns to ignore') | ||
| .option('--list', 'List available transforms for this migration') | ||
| .action((targetDir: string | undefined, opts: Record<string, unknown>) => { | ||
| try { | ||
| if (opts['list']) { | ||
| console.log(`\nAvailable transforms for ${name}:\n`); | ||
| for (const t of migration.transforms) { | ||
| console.log(` ${t.id.padEnd(20)} ${t.name}`); | ||
| } | ||
| console.log(''); | ||
| return; | ||
| } | ||
|
|
||
| if (!targetDir) { | ||
| console.error(`\nError: missing required argument <target-dir>.\n`); | ||
| process.exitCode = 1; | ||
| return; | ||
| } | ||
|
|
||
| const resolvedDir = path.resolve(targetDir); | ||
|
|
||
| if (!existsSync(resolvedDir) || !statSync(resolvedDir).isDirectory()) { | ||
| console.error(`\nError: "${resolvedDir}" is not a valid directory.\n`); | ||
| process.exitCode = 1; | ||
| return; | ||
| } | ||
|
|
||
| console.log(`\n@modelcontextprotocol/codemod — ${migration.name}\n`); | ||
| console.log(`Scanning ${resolvedDir}...`); | ||
| if (opts['dryRun']) { | ||
| console.log('(dry run — no files will be modified)\n'); | ||
| } else { | ||
| console.log(''); | ||
| } | ||
|
|
||
| const transforms = opts['transforms'] ? (opts['transforms'] as string).split(',').map(s => s.trim()) : undefined; | ||
|
|
||
| const result = run(migration, { | ||
| targetDir: resolvedDir, | ||
| dryRun: opts['dryRun'] as boolean | undefined, | ||
| verbose: opts['verbose'] as boolean | undefined, | ||
| transforms, | ||
| ignore: opts['ignore'] as string[] | undefined | ||
| }); | ||
|
|
||
| if (result.filesChanged === 0 && result.diagnostics.length === 0) { | ||
| console.log('No changes needed — code already migrated or no SDK imports found.\n'); | ||
| return; | ||
| } | ||
|
Comment on lines
+73
to
+76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The early-return condition checks Extended reasoning...What the bug isThe 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 Code path that triggers it
This is called at runner.ts:111 with no gating other than Why nothing else prevents itThe triggers are realistic:
The dedicated unit test Step-by-step proofProject layout: User runs
Net effect: package.json was modified on disk; the CLI explicitly told the user nothing was modified. ImpactNit 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. FixOne-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.filesChanged > 0) { | ||
| console.log(`Changes: ${result.totalChanges} across ${result.filesChanged} file(s)\n`); | ||
| } | ||
|
|
||
| if (opts['verbose']) { | ||
| console.log('Files modified:'); | ||
| for (const fr of result.fileResults) { | ||
| console.log(` ${fr.filePath} (${fr.changes} change(s))`); | ||
| } | ||
| console.log(''); | ||
| } | ||
|
|
||
| const errors = result.diagnostics.filter(d => d.level === DiagnosticLevel.Error); | ||
| if (errors.length > 0) { | ||
| console.log(`Errors (${errors.length}):`); | ||
| for (const d of errors) { | ||
| console.log(formatDiagnostic(d)); | ||
| } | ||
| console.log(''); | ||
| process.exitCode = 1; | ||
| } | ||
|
|
||
| const warnings = result.diagnostics.filter(d => d.level === DiagnosticLevel.Warning); | ||
| if (warnings.length > 0) { | ||
| console.log(`Warnings (${warnings.length}):`); | ||
| for (const d of warnings) { | ||
| console.log(formatDiagnostic(d)); | ||
| } | ||
| console.log(''); | ||
| } | ||
|
|
||
| const infos = result.diagnostics.filter(d => d.level === DiagnosticLevel.Info); | ||
| if (infos.length > 0) { | ||
| console.log(`Info (${infos.length}):`); | ||
| for (const d of infos) { | ||
| console.log(formatDiagnostic(d)); | ||
| } | ||
| console.log(''); | ||
| } | ||
|
claude[bot] marked this conversation as resolved.
|
||
|
|
||
| if (opts['dryRun']) { | ||
| console.log('Run without --dry-run to apply changes.\n'); | ||
| } else { | ||
| console.log('Migration complete. Review the changes and run your build/tests.\n'); | ||
| } | ||
| } catch (error) { | ||
| console.error(`\nError: ${error instanceof Error ? error.message : String(error)}\n`); | ||
| process.exitCode = 1; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| program.parse(); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| export { getMigration, listMigrations } from './migrations/index.js'; | ||
| export { run } from './runner.js'; | ||
| export type { | ||
| Diagnostic, | ||
| FileResult, | ||
| Migration, | ||
| RunnerOptions, | ||
| RunnerResult, | ||
| Transform, | ||
| TransformContext, | ||
| TransformResult | ||
| } from './types.js'; | ||
|
Comment on lines
+3
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Extended reasoning...What the gap is
Why it's an oversight, not a design choiceThe Why nothing else exposes it
Step-by-step proofA 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);
ImpactNothing 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. FixOne word: export type {
Diagnostic,
FileResult,
Migration,
PackageJsonChange,
RunnerOptions,
RunnerResult,
Transform,
TransformContext,
TransformResult
} from './types.js'; |
||
| export { DiagnosticLevel } from './types.js'; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import type { Migration } from '../types.js'; | ||
| import { v1ToV2Migration } from './v1-to-v2/index.js'; | ||
|
|
||
| const migrations = new Map<string, Migration>([['v1-to-v2', v1ToV2Migration]]); | ||
|
|
||
| export function getMigration(name: string): Migration | undefined { | ||
| return migrations.get(name); | ||
| } | ||
|
|
||
| export function listMigrations(): Map<string, Migration> { | ||
| return migrations; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| import type { Migration } from '../../types.js'; | ||
| import { v1ToV2Transforms } from './transforms/index.js'; | ||
|
|
||
| export const v1ToV2Migration: Migration = { | ||
| name: 'v1-to-v2', | ||
| description: 'Migrate from @modelcontextprotocol/sdk (v1) to v2 packages (@modelcontextprotocol/client, /server, etc.)', | ||
| transforms: v1ToV2Transforms | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| export interface ContextMapping { | ||
| from: string; | ||
| to: string; | ||
| } | ||
|
|
||
| export const CONTEXT_PROPERTY_MAP: ContextMapping[] = [ | ||
| { from: '.signal', to: '.mcpReq.signal' }, | ||
| { from: '.requestId', to: '.mcpReq.id' }, | ||
| { from: '._meta', to: '.mcpReq._meta' }, | ||
| { from: '.sendRequest', to: '.mcpReq.send' }, | ||
| { from: '.sendNotification', to: '.mcpReq.notify' }, | ||
| { from: '.authInfo', to: '.http?.authInfo' }, | ||
| { from: '.sessionId', to: '.sessionId' }, | ||
| { from: '.requestInfo', to: '.http?.req' }, | ||
| { from: '.closeSSEStream', to: '.http?.closeSSE' }, | ||
| { from: '.closeStandaloneSSEStream', to: '.http?.closeStandaloneSSE' }, | ||
| { from: '.taskStore', to: '.task?.store' }, | ||
| { from: '.taskId', to: '.task?.id' }, | ||
| { from: '.taskRequestedTtl', to: '.task?.requestedTtl' } | ||
| ]; | ||
|
|
||
| export const EXTRA_PARAM_NAME = 'extra'; | ||
| export const CTX_PARAM_NAME = 'ctx'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| export interface ImportMapping { | ||
| target: string; | ||
| status: 'moved' | 'removed' | 'renamed'; | ||
| renamedSymbols?: Record<string, string>; | ||
| removalMessage?: string; | ||
| } | ||
|
|
||
| export const IMPORT_MAP: Record<string, ImportMapping> = { | ||
| '@modelcontextprotocol/sdk/client/index.js': { | ||
| target: '@modelcontextprotocol/client', | ||
| status: 'moved' | ||
| }, | ||
| '@modelcontextprotocol/sdk/client/auth.js': { | ||
| target: '@modelcontextprotocol/client', | ||
| status: 'moved' | ||
| }, | ||
| '@modelcontextprotocol/sdk/client/streamableHttp.js': { | ||
| target: '@modelcontextprotocol/client', | ||
| status: 'moved' | ||
| }, | ||
| '@modelcontextprotocol/sdk/client/sse.js': { | ||
| target: '@modelcontextprotocol/client', | ||
| status: 'moved' | ||
| }, | ||
| '@modelcontextprotocol/sdk/client/stdio.js': { | ||
| target: '@modelcontextprotocol/client', | ||
| status: 'moved' | ||
| }, | ||
| '@modelcontextprotocol/sdk/client/websocket.js': { | ||
| target: '', | ||
| status: 'removed', | ||
| removalMessage: 'WebSocketClientTransport removed in v2. Use StreamableHTTPClientTransport or StdioClientTransport.' | ||
| }, | ||
|
|
||
| '@modelcontextprotocol/sdk/server/mcp.js': { | ||
| target: '@modelcontextprotocol/server', | ||
| status: 'moved' | ||
| }, | ||
| '@modelcontextprotocol/sdk/server/index.js': { | ||
| target: '@modelcontextprotocol/server', | ||
| status: 'moved' | ||
| }, | ||
| '@modelcontextprotocol/sdk/server/stdio.js': { | ||
| target: '@modelcontextprotocol/server', | ||
| status: 'moved' | ||
| }, | ||
| '@modelcontextprotocol/sdk/server/streamableHttp.js': { | ||
| target: '@modelcontextprotocol/node', | ||
| status: 'renamed', | ||
| renamedSymbols: { | ||
| StreamableHTTPServerTransport: 'NodeStreamableHTTPServerTransport' | ||
| } | ||
| }, | ||
| '@modelcontextprotocol/sdk/server/webStandardStreamableHttp.js': { | ||
| target: '@modelcontextprotocol/server', | ||
| status: 'moved' | ||
| }, | ||
| '@modelcontextprotocol/sdk/server/sse.js': { | ||
| target: '', | ||
| status: 'removed', | ||
| removalMessage: 'SSE server transport removed in v2. Migrate to NodeStreamableHTTPServerTransport from @modelcontextprotocol/node.' | ||
| }, | ||
| '@modelcontextprotocol/sdk/server/middleware.js': { | ||
| target: '@modelcontextprotocol/express', | ||
| status: 'moved' | ||
| }, | ||
|
|
||
| '@modelcontextprotocol/sdk/server/auth/types.js': { | ||
| target: '', | ||
| status: 'removed', | ||
| removalMessage: | ||
| 'Server auth removed in v2. AuthInfo is now re-exported by @modelcontextprotocol/client and @modelcontextprotocol/server.' | ||
| }, | ||
| '@modelcontextprotocol/sdk/server/auth/provider.js': { | ||
| target: '', | ||
| status: 'removed', | ||
| removalMessage: 'Server auth removed in v2. Use an external auth library (e.g., better-auth).' | ||
| }, | ||
| '@modelcontextprotocol/sdk/server/auth/router.js': { | ||
| target: '', | ||
| status: 'removed', | ||
| removalMessage: 'Server auth removed in v2. Use an external auth library (e.g., better-auth).' | ||
| }, | ||
| '@modelcontextprotocol/sdk/server/auth/middleware.js': { | ||
| target: '', | ||
| status: 'removed', | ||
| removalMessage: 'Server auth removed in v2. Use an external auth library (e.g., better-auth).' | ||
| }, | ||
| '@modelcontextprotocol/sdk/server/auth/errors.js': { | ||
| target: '', | ||
| status: 'removed', | ||
| removalMessage: 'Server auth removed in v2. Use an external auth library (e.g., better-auth).' | ||
| }, | ||
|
|
||
| '@modelcontextprotocol/sdk/types.js': { | ||
| target: 'RESOLVE_BY_CONTEXT', | ||
| status: 'moved' | ||
| }, | ||
| '@modelcontextprotocol/sdk/shared/protocol.js': { | ||
| target: 'RESOLVE_BY_CONTEXT', | ||
| status: 'moved' | ||
| }, | ||
| '@modelcontextprotocol/sdk/shared/transport.js': { | ||
| target: 'RESOLVE_BY_CONTEXT', | ||
| status: 'moved' | ||
| }, | ||
| '@modelcontextprotocol/sdk/shared/uriTemplate.js': { | ||
| target: 'RESOLVE_BY_CONTEXT', | ||
| status: 'moved' | ||
| }, | ||
| '@modelcontextprotocol/sdk/shared/auth.js': { | ||
| target: 'RESOLVE_BY_CONTEXT', | ||
| status: 'moved' | ||
| }, | ||
| '@modelcontextprotocol/sdk/shared/stdio.js': { | ||
| target: 'RESOLVE_BY_CONTEXT', | ||
| status: 'moved' | ||
| }, | ||
|
|
||
| '@modelcontextprotocol/sdk/inMemory.js': { | ||
| target: '@modelcontextprotocol/core', | ||
| status: 'moved' | ||
| } | ||
| }; | ||
|
|
||
| export function isAuthImport(specifier: string): boolean { | ||
| return specifier.includes('/server/auth/') || specifier.includes('/server/auth.'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The new
@modelcontextprotocol/codemodpackage is added to the publish workflow but ships with noREADME.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 apackages/codemod/README.mdbefore this lands (npm auto-includes README regardless of thefilesarray, so nopackage.jsonchange is needed).Extended reasoning...
What the gap is
This PR adds
@modelcontextprotocol/codemodas a new published package — it has abinentry (mcp-codemod), a programmatic API export, and is added to.github/workflows/publish.yml:43so pkg-pr-new is already publishing preview builds. Butpackages/codemod/contains noREADME.md. Every other published package in the repo has one:packages/client/README.md,packages/server/README.md, and all fourpackages/middleware/*/README.mdfiles 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 runsnpx @modelcontextprotocol/codemodand 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.package.jsondescription string: "Codemod to migrate MCP TypeScript SDK code from v1 to v2" — no usage, no flags, no transform list.Note on
filesarraynpm always includes
README*(along withpackage.json,LICENSE*, and themainentry) regardless of thefilesallowlist, so addingpackages/codemod/README.mdis sufficient — no need to touch"files": ["dist"].Fix
Add
packages/codemod/README.mdcontaining (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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBD. Add README.md once the feasibility of the codemod has been confirmed.