Repository navigation
Conversation
🦋 Changeset detectedLatest commit: d6c26e8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
ANcpLua
left a comment
ANcpLua
left a comment
There was a problem hiding this comment.
The approach looks right to me. It is an INFO notice marked advisoryOnly that makes no code change, so upgrading the packages stays separate from opting into 2026-07-28. The three detected names match what importMap.ts rewrites v1 imports into. I ran the branch (f4b6fc5) against main (b022522): it merges cleanly, all 649 codemod tests pass, and typecheck passes. serveStdio (@modelcontextprotocol/server/stdio) and createMcpHandler (@modelcontextprotocol/server) both exist on main, and the doc anchor resolves on the live site.
I probed it with extra fixtures and found four gaps. All four are in the reference lookup at modernServerEntryNotice.ts:38-42.
1. A file that only uses the type can get a notice at another file's line number. findReferencesAsNodes() searches the whole project. Once @modelcontextprotocol/server resolves (I installed the published 2.3.1 next to the fixture), it returns references from other files too. .find() then accepts the first new it sees, and the notice is filed under the current file. It depends on processing order: it shows when the type-only file is processed after the constructing file has been rewritten.
// main.ts
import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js';
import { server } from './server.js';
// …5 comment lines…
await server.connect(new StdioServerTransport()); // line 8
// types.ts (3 lines)
import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js';
export type Entry = StdioServerTransport;main.ts:8 (text there: "await server.connect(new StdioServerTransport());")
types.ts:8 (file has 3 lines; text there: <past EOF>)
Without node_modules the bug doesn't show, so the current tests can't catch it. Filtering with reference.getSourceFile() === sourceFile fixes it.
2. Only one notice per binding, not one per call site. The lookup uses .find(), so it stops at the first match:
if (process.argv[2] === 'a') await a.connect(new StdioServerTransport()); // notice
else await b.connect(new StdioServerTransport()); // no notice3. The line number goes stale when a later transform rewrites the imports above the call. This transform runs second, so the line it records is from before the later rewrites. The runner only re-resolves lines for diagnostics that set resolveCurrentLine. With a v1 import of StreamableHTTPError, or an import type of IsomorphicHeaders, in the same file, a later transform removes import lines and the reported line no longer points at the call:
s.ts:7 -> "" (StreamableHTTPError fixture; the construct is on line 6)
s.ts:7 -> undefined (IsomorphicHeaders fixture; past EOF, construct on line 5)
Adding resolveCurrentLine: () => construction.getStartLineNumber() fixes this.
4. Namespace imports are not covered. Only named imports are scanned, so this gets no notice:
import * as stdio from '@modelcontextprotocol/sdk/server/stdio.js';
await server.connect(new stdio.StdioServerTransport());The PR's other claims hold. The notice fires for aliases, for a transport assigned to a variable, for sessionful Node HTTP, for web-standard HTTP and for .mjs. It stays silent for client transports, unused imports, type-only use, files already on serveStdio/createMcpHandler, and re-runs. Barrel re-exports, subclasses and CommonJS require get no notice either. That is consistent with the codemod, which doesn't rewrite require, so I'd leave those out of scope.
The smallest fix I tested keeps findReferencesAsNodes: filter with reference.getSourceFile() === sourceFile, change .find to .filter, and add resolveCurrentLine: () => construction.getStartLineNumber(). That fixes 1–3, and a locally declared class StdioServerTransport {} stays silent as it does today. Namespace imports (4) still need their own handling. A regression test for 3 should assert the text at the reported line, since a line count alone doesn't catch the drift. Happy to share the diff and tests if useful.
Two smaller wording points:
createServer()reads like an SDK API, but it is the user's own factory, and Node'shttpmodule has acreateServertoo.serveStdio(() => buildServer())matchessupport-2026-07-28.md:201.- For sessionful Node HTTP,
createMcpHandler(...)on its own drops session state. The linked section covers keeping it withisLegacyRequestin front oflegacy: 'reject'. "To also serve 2026-07-28, see …" would describe the decision more accurately than presenting a drop-in replacement.
Probes, patch and this review were drafted with AI assistance (Claude). The outputs quoted are from real runs.
claude
Bot
added
the
v2
|
Thanks for picking this up! A ready-made test case: plugins/mcp-builder-v2/evals/ts-migrate-v1/fixture in https://github.com/ANcpLua/mcp-builder-v2 is a small v1 stdio server. Today's codemod (2.3.1) migrates it to |
|
Thanks for the detailed probes. I pushed a focused follow-up that addresses gaps 1–3:
I left namespace imports out of this follow-up for now because they need separate member-access handling. CI will provide the full validation for the new commits. |
This branch has not been deployed
2 participants
Summary
StdioServerTransportcreateMcpHandler@modelcontextprotocol/codemodThe codemod does not rewrite these serving entries automatically because adopting the 2026-07-28 protocol revision is a separate choice. The new info diagnostic makes that choice visible at the end of the v1-to-v2 migration instead of leaving a clean-looking, legacy-only server.
Fixes #2977
Verification
pnpm --filter @modelcontextprotocol/codemod typecheckpnpm --filter @modelcontextprotocol/codemod buildpnpm changeset status --since=origin/mainpassedThe one skipped test creates a directory symlink cycle and cannot run without Windows symlink privileges. The full pre-push hook also encountered checkout-level limits unrelated to this diff: the workspace was installed with the codemod-only filter, and the Windows CRLF checkout makes
sync:snippets --checkreport existing tracked files as out of sync. Package-scoped build, typecheck, lint, formatting, and tests are covered above; CI remains the full-workspace check.