Skip to content

feat: add v2 codemod draft#1950

Draft
KKonstantinov wants to merge 28 commits intomainfrom
feature/v2-codemode-draft
Draft

feat: add v2 codemod draft#1950
KKonstantinov wants to merge 28 commits intomainfrom
feature/v2-codemode-draft

Conversation

@KKonstantinov
Copy link
Copy Markdown
Contributor

@KKonstantinov KKonstantinov commented Apr 23, 2026

feat: add @modelcontextprotocol/codemod package for automated v1 → v2 migration

Adds a new @modelcontextprotocol/codemod package that automatically migrates MCP TypeScript SDK code from v1 (@modelcontextprotocol/sdk) to the v2 multi-package architecture (@modelcontextprotocol/client, /server, /core, /node, /express). Powered by ts-morph for AST-level precision and shipped as both a CLI (mcp-codemod) and a programmatic API. Also automatically updates package.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

packages/codemod/
├── src/
│   ├── cli.ts                  # Commander CLI entry point
│   ├── runner.ts               # Core orchestrator
│   ├── types.ts                # Transform / Migration / Diagnostic types
│   ├── index.ts                # Public API exports
│   ├── migrations/
│   │   ├── index.ts            # Migration registry
│   │   └── v1-to-v2/
│   │       ├── index.ts        # Migration definition
│   │       ├── mappings/       # Declarative lookup tables
│   │       │   ├── importMap.ts
│   │       │   ├── symbolMap.ts
│   │       │   ├── schemaToMethodMap.ts
│   │       │   └── contextPropertyMap.ts
│   │       └── transforms/     # Ordered AST transforms
│   │           ├── index.ts
│   │           ├── importPaths.ts
│   │           ├── symbolRenames.ts
│   │           ├── removedApis.ts
│   │           ├── mcpServerApi.ts
│   │           ├── handlerRegistration.ts
│   │           ├── schemaParamRemoval.ts
│   │           ├── expressMiddleware.ts
│   │           ├── contextTypes.ts
│   │           └── mockPaths.ts
│   ├── generated/
│   │   └── versions.ts            # Build-time v2 package versions
│   └── utils/
│       ├── astUtils.ts            # AST rename helpers
│       ├── diagnostics.ts         # Diagnostic factories
│       ├── importUtils.ts         # Import manipulation
│       ├── packageJsonUpdater.ts  # Automatic package.json updates
│       └── projectAnalyzer.ts     # Detects client/server/both
├── scripts/
│   └── generate-versions.ts       # Reads sibling package versions at build time
└── test/                          # 14 test suites, 201 test cases

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 --> M
Loading

Transform Pipeline

Transforms run in a strict order per file. Each transform receives the SourceFile AST, 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 --> T9
Loading

Import 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)"]
Loading

Context Property Remapping

The v2 SDK restructures the handler context from a flat object into nested groups. The contextTypes transform 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 --> C6
Loading

What Each Transform Does

# Transform ID Description
1 imports Rewrites all @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 automatic package.json updates.
2 symbols Renames 9 symbols (e.g., McpErrorProtocolError). Splits ErrorCode into ProtocolErrorCode + SdkErrorCode based on member usage. Converts RequestHandlerExtraServerContext/ClientContext. Replaces SchemaInput<T> with StandardSchemaWithJSON.InferInput<T>.
3 removed-apis Removes references to dropped Zod helpers (schemaToJson, parseSchemaAsync, etc.), renames IsomorphicHeadersHeaders, converts StreamableHTTPErrorSdkError with constructor mapping warnings.
4 mcpserver-api Rewrites McpServer method calls: .tool().registerTool(), .prompt().registerPrompt(), .resource().registerResource(). Restructures 2-4 positional args into (name, config, callback) form. Wraps raw object schemas with z.object().
5 handlers Converts server.setRequestHandler(CallToolRequestSchema, ...) to server.setRequestHandler('tools/call', ...) using the schema-to-method mapping table. Covers 15 request schemas and 8 notification schemas.
6 schema-params Removes the schema parameter from .request(), .callTool(), and .send() calls where the second argument is a schema reference (ends with Schema).
7 express-middleware Updates hostHeaderValidation({ allowedHosts: [...] })hostHeaderValidation([...]).
8 context Renames the extra callback parameter to ctx. Rewrites 13 property access paths from the flat v1 context to the nested v2 context structure. Warns on destructuring patterns that need manual review.
9 mock-paths Rewrites vi.mock() / jest.mock() specifiers from v1 to v2 paths. Updates import() expressions in mock factories. Renames symbol references inside mock factory return objects.

CLI Usage

# Run all transforms
mcp-codemod v1-to-v2 ./src

# Dry run — preview without writing
mcp-codemod v1-to-v2 ./src --dry-run --verbose

# Run specific transforms only
mcp-codemod v1-to-v2 ./src --transforms imports,symbols,context

# List available transforms
mcp-codemod v1-to-v2 --list

# Ignore additional patterns
mcp-codemod v1-to-v2 ./src --ignore "legacy/**" "generated/**"

Programmatic API

import { getMigration, run } from '@modelcontextprotocol/codemod';

const migration = getMigration('v1-to-v2')!;
const result = run(migration, {
  targetDir: './src',
  dryRun: false,
  verbose: true,
  transforms: ['imports', 'symbols'],
  ignore: ['test/**']
});

console.log(`${result.filesChanged} files changed, ${result.totalChanges} total changes`);
for (const d of result.diagnostics) {
  console.log(`[${d.level}] ${d.file}:${d.line}${d.message}`);
}

How Has This Been Tested?

  • 201 test cases across 14 test suites covering every transform, the CLI, the runner, the project analyzer, and the package.json updater.
  • Each transform has its own dedicated test suite with isolated ts-morph in-memory filesystem tests.
  • Integration tests run the full pipeline against realistic v1 source files and verify complete output including package.json updates.
  • Package.json updater has dedicated unit tests covering: deps/devDeps/both sections, partial migration, dry-run, malformed JSON, private package filtering, indentation and trailing newline preservation.
  • Integration tests verify end-to-end package detection for client-only, server-only, client+server, express middleware, and split-import (symbolTargetOverrides) scenarios.
  • Edge cases tested: duplicate imports, type-only imports, removed APIs, ambiguous shared paths, ErrorCode member splitting, destructuring patterns, mock factories, .d.ts exclusion, unknown transform ID validation.

Breaking Changes

None — this is a new package with no existing users.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Design Decisions

  1. 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.RequestTimeout from ErrorCode.InvalidRequest for the ProtocolErrorCode/SdkErrorCode split).

  2. 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.

  3. 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.

  4. Context-aware import resolution: Shared v1 paths like @modelcontextprotocol/sdk/types.js are resolved to client or server packages based on package.json dependency analysis, not just hardcoded defaults.

  5. 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.

  6. Automatic package.json updates: As transforms rewrite imports, they track which v2 packages are targeted. After all source transforms complete, the runner removes @modelcontextprotocol/sdk from package.json and adds exactly the v2 packages the code actually uses. Version specifiers are injected at build time from sibling package versions via scripts/generate-versions.ts. Private packages (@modelcontextprotocol/core) are filtered out. The updater preserves the original indentation and trailing newline, and respects dry-run mode.

  7. Import splitting with symbolTargetOverrides: When symbols from a single v1 module map to different v2 packages (e.g., StreamableHTTPServerTransport@modelcontextprotocol/node but EventStore@modelcontextprotocol/server), the import map supports per-symbol target overrides. The transform splits the import into separate declarations for each target package.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 23, 2026

⚠️ No Changeset found

Latest commit: 13a6345

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@KKonstantinov
Copy link
Copy Markdown
Contributor Author

@claude review

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Apr 23, 2026

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/client@1950

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/codemod@1950

@modelcontextprotocol/server

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/server@1950

@modelcontextprotocol/express

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/express@1950

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/fastify@1950

@modelcontextprotocol/hono

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/hono@1950

@modelcontextprotocol/node

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/node@1950

commit: 13a6345

Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/mockPaths.ts Outdated
Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts Outdated
Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts Outdated
Comment thread packages/codemod/test/v1-to-v2/transforms/mockPaths.test.ts Outdated
@KKonstantinov
Copy link
Copy Markdown
Contributor Author

@claude review

Comment thread packages/codemod/src/cli.ts Outdated
Comment thread packages/codemod/src/cli.ts
Comment thread packages/codemod/src/utils/projectAnalyzer.ts
@KKonstantinov KKonstantinov changed the title feat: add v2 codemode draft feat: add v2 codemod draft Apr 23, 2026
@KKonstantinov
Copy link
Copy Markdown
Contributor Author

@claude review

Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/mcpServerApi.ts
Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/symbolRenames.ts Outdated
@KKonstantinov
Copy link
Copy Markdown
Contributor Author

@claude review

@KKonstantinov
Copy link
Copy Markdown
Contributor Author

Todo:

  • Test on sample size MCP SDK (v1) dependents
  • Test on everything-server
  • Test on v1 MCP SDK examples

Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/mockPaths.ts
@KKonstantinov
Copy link
Copy Markdown
Contributor Author

@claude review

Comment thread packages/codemod/src/runner.ts Outdated
Comment thread packages/codemod/src/runner.ts Outdated
@KKonstantinov
Copy link
Copy Markdown
Contributor Author

@claude review

Comment thread packages/codemod/src/utils/astUtils.ts
Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts
Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/removedApis.ts
Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/mcpServerApi.ts
Comment thread packages/codemod/src/runner.ts
@KKonstantinov
Copy link
Copy Markdown
Contributor Author

@claude review

Comment on lines +32 to +34
"files": [
"dist"
],
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

  1. .github/workflows/publish.yml:43 adds './packages/codemod' to the pkg-pr-new publish command, so the package is published on every PR push (and presumably on release).
  2. packages/codemod/package.json:32-34 sets "files": ["dist"] and a bin entry — so the package is intended for end-user consumption via npx.
  3. ls packages/codemod/ shows: eslint.config.mjs, package.json, src/, test/, tsconfig.json, tsdown.config.ts, typedoc.json, vitest.config.js — no README.md.
  4. ls packages/*/README.md packages/middleware/*/README.md shows READMEs for client, server, express, fastify, hono, and node — codemod is the only published package without one.
  5. On npm, the package page renders the README as the primary documentation surface. With no README, the page shows only the package.json description 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.

Copy link
Copy Markdown
Contributor Author

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.

Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/contextTypes.ts
Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts
@KKonstantinov
Copy link
Copy Markdown
Contributor Author

@claude review

Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts
Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/mockPaths.ts Outdated
Comment thread packages/codemod/src/utils/importUtils.ts
@KKonstantinov
Copy link
Copy Markdown
Contributor Author

@claude review

Comment thread packages/codemod/src/utils/astUtils.ts
Comment thread packages/codemod/src/utils/importUtils.ts
@KKonstantinov
Copy link
Copy Markdown
Contributor Author

@claude review

Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts
Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/mockPaths.ts
@KKonstantinov
Copy link
Copy Markdown
Contributor Author

@claude review

Comment thread packages/codemod/src/cli.ts Outdated
Comment thread packages/codemod/src/index.ts
Comment thread packages/codemod/src/runner.ts Outdated
Comment thread packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts
@KKonstantinov
Copy link
Copy Markdown
Contributor Author

@claude review

Comment thread packages/codemod/test/integration.test.ts Outdated
Comment thread packages/codemod/src/utils/importUtils.ts Outdated
@KKonstantinov
Copy link
Copy Markdown
Contributor Author

@claude review

Comment on lines 9 to +10
export type { CfWorkerSchemaDraft } from '@modelcontextprotocol/core';
export { CfWorkerJsonSchemaValidator } from '@modelcontextprotocol/core';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 These two cfWorker.ts hunks (client + server) just swap the order of export type {...} and export {...} — almost certainly a repo-wide lint:fix drive-by unrelated to the codemod package. Zero runtime impact, but it's why changeset-bot's auto-suggested changeset on this PR includes @modelcontextprotocol/client: patch and pkg-pr-new is publishing client/server previews. Per REVIEW.md ("Decompose by default"), consider git checkout main -- packages/{client,server}/src/validators/cfWorker.ts to keep this PR scoped to the codemod.

Extended reasoning...

What this is

The diff includes two hunks unrelated to the @modelcontextprotocol/codemod feature:

  • packages/client/src/validators/cfWorker.ts:9-10
  • packages/server/src/validators/cfWorker.ts:9-10

Both swap the order of two adjacent re-export lines from:

export { CfWorkerJsonSchemaValidator } from '@modelcontextprotocol/core';
export type { CfWorkerSchemaDraft } from '@modelcontextprotocol/core';

to:

export type { CfWorkerSchemaDraft } from '@modelcontextprotocol/core';
export { CfWorkerJsonSchemaValidator } from '@modelcontextprotocol/core';

This is a purely cosmetic reordering — same exports, same module, zero runtime or type-level effect. It's the signature of a monorepo-wide pnpm lint:fix (import/export sorting) that touched files outside the package being worked on.

Concrete consequence

The changeset-bot comment on this PR auto-suggests a changeset that includes "@modelcontextprotocol/client": patch (visible in the URL-encoded "add a changeset" link in the bot's comment: %22%40modelcontextprotocol%2Fclient%22%3A%20patch). The bot only flags client — not server — but that's enough to show the PR's footprint is wider than intended. Similarly, pkg-pr-new is building and publishing preview packages for @modelcontextprotocol/client and @modelcontextprotocol/server on every push to this branch, solely because of these two no-op reorderings.

If the maintainer clicks the bot's suggestion, the client package gets a version bump for a change that has nothing to do with it.

Why this is worth a nit (addressing the counter-arguments)

One reviewer argued this is below the bar: (a) zero runtime impact, (b) REVIEW.md's "Decompose by default" targets bundled functional changes rather than incidental lint output, (c) the changeset-bot suggestion is just a heuristic that maintainers override, and (d) ~10 review rounds have passed without anyone flagging it. Those are fair points and are why this is nit, not normal:

  • (a) Agreed — no one disputes zero runtime impact. The comment is about PR scope, not correctness.
  • (b) REVIEW.md:22 says "A PR doing multiple things should be multiple PRs unless there's a strong reason to bundle" — it doesn't carve out lint auto-fixes, and there's no reason to bundle these here. That said, two swapped lines is admittedly the smallest possible instance of the principle.
  • (c) True that maintainers write the actual changeset — but the bot suggestion and the pkg-pr-new client/server preview builds are real, verifiable signals in this PR's own timeline that the change footprint is wider than "new codemod package". A maintainer skimming the bot's link could easily accept the suggested client: patch without noticing it's for a no-op.
  • (d) The prior ~10 review rounds were all automated reviews focused on functional bugs in the codemod transforms (import handling, symbol renames, etc.). None of them examined PR scope, so silence there isn't a strong acceptance signal.

The fix is also trivially low-churn — one command, no rebase needed — which tips the cost/benefit toward mentioning it.

Step-by-step proof

  1. The diff for packages/client/src/validators/cfWorker.ts shows lines 9-10 swapped: export { CfWorkerJsonSchemaValidator } moved below export type { CfWorkerSchemaDraft }. Identical swap in packages/server/src/validators/cfWorker.ts.
  2. Neither file is referenced anywhere in packages/codemod/ — the codemod's IMPORT_MAP doesn't mention cfWorker or CfWorkerJsonSchemaValidator, and no transform reads these files.
  3. The changeset-bot comment (first comment in the PR timeline, 2026-04-23T16:59:17Z) includes an "add a changeset" link whose URL-encoded body is ---\n"@modelcontextprotocol/client": patch\n"@modelcontextprotocol/codemod": patch\n--- — i.e., the bot detected client as changed because of this hunk.
  4. The pkg-pr-new comment (2026-04-23T17:00:53Z) lists @modelcontextprotocol/client and @modelcontextprotocol/server as published preview packages — again because these two files changed.
  5. Reverting the two hunks would reduce the bot's suggested changeset to codemod only and stop pkg-pr-new from rebuilding client/server on every push.

Fix

git checkout main -- packages/client/src/validators/cfWorker.ts packages/server/src/validators/cfWorker.ts

(Or leave them — this is non-blocking. Just flagging per the repo's own REVIEW.md convention so the stray hunks don't accidentally drive a client/server version bump.)

Comment on lines +299 to +308
const newName = allRenames[lookupKey];
if (newName) {
if (propertyName) {
element.getPropertyNameNode()!.replaceWithText(newName);
} else {
element.replaceWithText(`${newName}: ${bindingName}`);
}
changes++;
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The Object.keys(allRenames).length > 0 guard is always true (since allRenames at line 259 unconditionally spreads in all 7 SIMPLE_RENAMES entries), so this warning fires for every non-destructured dynamic SDK import and lists McpError, JSONRPCError, JSONRPCErrorSchema, isJSONRPCError, isJSONRPCResponse, ResourceReference, ResourceReferenceSchema even for modules like server/mcp.js that export none of them. Gate on resolved.renamedSymbols being non-empty (and list only those keys) so the warning is module-relevant rather than noise.

Extended reasoning...

What the bug is

In rewriteDynamicImports (mockPaths.ts:310-318), when a dynamic import is assigned to a non-destructured variable — e.g. const mod = await import('@modelcontextprotocol/sdk/server/mcp.js') — the code emits:

if (!Node.isObjectBindingPattern(nameNode) && Object.keys(allRenames).length > 0) {
    diagnostics.push(warning(..., `Dynamic import assigned to variable (not destructured). Symbol renames (${Object.keys(allRenames).join(', ')}) were not applied. Manual update may be needed.`));
}

But allRenames is built at line 259 as { ...SIMPLE_RENAMES, ...resolved.renamedSymbols }. SIMPLE_RENAMES (symbolMap.ts) is a fixed 7-entry table: McpError, JSONRPCError, JSONRPCErrorSchema, isJSONRPCError, isJSONRPCResponse, ResourceReference, ResourceReferenceSchema. So Object.keys(allRenames).length is always ≥ 7, regardless of which v1 module was imported. The > 0 guard is therefore tautologically true (dead code), and the warning text lists all 7 SIMPLE_RENAMES keys even for modules that export none of them.

Code path that triggers it

SIMPLE_RENAMES symbols are all exported only from @modelcontextprotocol/sdk/types.js. For a dynamic import of any other SDK module — server/mcp.js, client/index.js, server/stdio.js, etc. — resolved.renamedSymbols is undefined (per IMPORT_MAP), so allRenames is exactly SIMPLE_RENAMES. The warning then tells the user that 7 symbols "were not applied" when the imported module never exported any of them in the first place.

Why existing code doesn't prevent it

The guard at line 310 was clearly intended to suppress the warning when there are no relevant renames for the imported module — otherwise there's no reason to check the length at all. But spreading SIMPLE_RENAMES into allRenames defeats that intent: the length check can never be false. The only per-module rename data is resolved.renamedSymbols (currently only populated for server/streamableHttp.js), and that's what the gate should consult. No test exercises a non-destructured dynamic import of a non-types.js module, so the noise isn't caught.

Step-by-step proof

Input:

const mod = await import('@modelcontextprotocol/sdk/server/mcp.js');
const server = new mod.McpServer({ name: 'test', version: '1.0' });
  1. mockPaths.ts:217 — expr.getKind() === ImportKeyword ✓; specifier = '@modelcontextprotocol/sdk/server/mcp.js'; isSdkSpecifier ✓.
  2. resolveTargetIMPORT_MAP['.../server/mcp.js'] = { target: '@modelcontextprotocol/server', status: 'moved' } → returns { target: '@modelcontextprotocol/server', renamedSymbols: undefined, symbolTargetOverrides: undefined }.
  3. Line 259: allRenames = { ...SIMPLE_RENAMES, ...undefined } = the 7 SIMPLE_RENAMES entries.
  4. Line 286: specifier rewritten to '@modelcontextprotocol/server' ✓ (correct output).
  5. Line 290-293: parent is AwaitExpression, grandparent is VariableDeclaration, nameNode is the identifier mod.
  6. Line 294: Node.isObjectBindingPattern(mod)false.
  7. Line 310: !isObjectBindingPattern && Object.keys(allRenames).length > 0true && 7 > 0true.
  8. Line 315: pushes warning: "Dynamic import assigned to variable (not destructured). Symbol renames (McpError, JSONRPCError, JSONRPCErrorSchema, isJSONRPCError, isJSONRPCResponse, ResourceReference, ResourceReferenceSchema) were not applied. Manual update may be needed."

None of those 7 symbols is exported from server/mcp.js in v1 (it exports McpServer, ResourceTemplate, etc.), so the user is sent looking for renames that don't apply. The actually-relevant case — mod.McpServer is unchanged in v2 — needs no manual update at all.

Impact

Diagnostic-quality only — nit severity. The transformed source is correct; only the warning is misleading. The trigger pattern (const mod = await import(<sdk-path>) without destructuring) is uncommon, and the message is hedged with "may be needed". But it's a clear logic gap: the gate is dead code, and the warning lists symbols irrelevant to the module being imported, which undermines the codemod's "diagnostic-first" design goal.

Fix

Gate on per-module renames and list only those:

const moduleRenames = resolved.renamedSymbols ?? {};
// ...
if (!Node.isObjectBindingPattern(nameNode) && Object.keys(moduleRenames).length > 0) {
    diagnostics.push(warning(..., `... Symbol renames (${Object.keys(moduleRenames).join(', ')}) were not applied. ...`));
}

(If you also want to flag SIMPLE_RENAMES for namespace-style types.js imports, gate that on specifier ending in /types.js rather than unconditionally.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant