fix(codemod): keep a file's leading comment block above rewritten imports by axits-lab · Pull Request #2582 · modelcontextprotocol/typescript-sdk

axits-lab · 2026-07-30T13:22:31Z

Root cause

v1-to-v2 positions a re-emitted import against the full start of the declaration it replaces, which is ahead of that declaration's leading trivia. Both shapes in the issue follow from that one fact:

Shape A — header hoisted over, blank line consumed

// Copyright (c) 2026 Example Corp.
// SPDX-License-Identifier: Apache-2.0

import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js';

export const ok = (): CallToolResult => ({ content: [] });

became

import type { CallToolResult } from "@modelcontextprotocol/server";

// Copyright (c) 2026 Example Corp.
// SPDX-License-Identifier: Apache-2.0
export const ok = (): CallToolResult => ({ content: [] });

The header stops being the first thing in the file, so eslint-plugin-header / eslint-plugin-notice / SPDX scanners start failing on a file that previously passed. The blank line goes with it, so the header now reads as a doc comment on export const ok.

Shape B — import inserted inside the header run

When the header is a multi-line // run and the SDK import is not the first import, line 1 is stranded above an unrelated import:

// page_to_markdown tool: fetches a URL and returns clean Markdown.
import type { CallToolResult } from "@modelcontextprotocol/server";

// Uses @page2ai/core under the hood - inherits SSRF protection and a 10MB cap.
// Static tab discovery emits per-tab sections for docs sites.

import { fetchAndConvert } from '@page2ai/core';

As the reporter notes, Prettier does not move the import back, so the guide's "run your formatter" step does not cover either shape.

Fix

Detach the comment block anchored at byte 0 for the duration of the rewrite and restore the exact bytes afterwards — the trivia those insertion positions are derived from no longer exists while imports are emitted.

Scope is deliberately narrow: only a block starting at byte 0 is treated as a file header. A comment above a mid-file import documents that import and must travel with it, which the existing leading-comment capture still handles (covered by a third test).

The detached slice includes the whitespace following the block, so the blank line separating header from code survives the round trip.

On the existing tests

There were already three leading-header tests. All three assert only that the header's content survives (toContain) — none assert its position, which is why both shapes passed them. The new tests assert position and the surviving blank line. Verified they fail on main without the src change:

× keeps a license header above the rewritten import, with its blank line intact
× does not split a multi-line header run when the SDK import is not the first import

Verification

632/632 codemod tests pass (629 existing + 3 new)
pnpm lint and pnpm typecheck clean
Built the CLI and ran it against the reporter's exact repro — output now matches their "Expected" block byte-for-byte:

// Copyright (c) 2026 Example Corp.
// SPDX-License-Identifier: Apache-2.0

import type { CallToolResult } from "@modelcontextprotocol/server";

export const ok = (): CallToolResult => ({ content: [] });

Changeset included.

…orts `v1-to-v2` inserts a re-emitted import against the full start of the declaration it replaces, which is ahead of that declaration's leading trivia. Two things follow, both reported in modelcontextprotocol#2575: - A license/SPDX header stops being the first thing in the file, so eslint-plugin-header, eslint-plugin-notice and SPDX scanners start failing on a file that previously passed. The blank line under the header is consumed with it, leaving the header attached to the next declaration as a doc comment. - When the header is a multi-line `//` run and the SDK import is not the first import, the new import is inserted between two header lines, stranding line 1 above an unrelated import. Prettier does not move the import back, so the guide's "run your formatter" step does not cover either shape. Detach the block anchored at byte 0 for the duration of the rewrite and restore the exact bytes afterwards, so the positions the insertion is derived from no longer exist. Only a block starting at byte 0 is treated as a file header; a comment above a mid-file import documents that import and still travels with it. The three existing leading-header tests only assert the header's content survives, which is why both shapes passed them — the new tests assert its position and the surviving blank line. Fixes modelcontextprotocol#2575

changeset-bot · 2026-07-30T13:22:37Z

🦋 Changeset detected

Latest commit: 529895d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages

Name	Type
@modelcontextprotocol/codemod	Patch
@modelcontextprotocol/client	Patch
@modelcontextprotocol/core	Patch
@modelcontextprotocol/server-legacy	Patch
@modelcontextprotocol/server	Patch
@modelcontextprotocol/core-internal	Patch

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

pkg-pr-new · 2026-07-30T13:25:41Z

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@2582

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@2582

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@2582

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@2582

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@2582

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@2582

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@2582

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@2582

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@2582

commit: 529895d

felixweinberger · 2026-09-28T14:05:56Z

@claude review

felixweinberger · 2026-09-28T14:08:05Z

Pushed two commits onto this branch:

a merge of main
fix(codemod): write rewritten imports where the v1 import stood

The second one replaces detach-and-reattach with counting the insert position among all top-level statements. That also keeps // @ts-nocheck, /// <reference> and 'use client' above the import, and keeps the @mcp-codemod-error marker on files that start with a comment.

The three original tests are kept.

claude

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked the sdkImports[0].wasForgotten() switch that picks childIndex vs childIndex + 1: every in-place path in importPaths.ts mutates the first import only via setModuleSpecifier (lines 266/331), which keeps the ts-morph node alive, and the remove paths (155/333) forget it, so the removed-vs-rewritten distinction is sound for this transform. The two-target split of a single types.js import (value to core plus type to server under a header) also lands in the right order with the blank line only on the topmost inserted import.

Extended reasoning...

The change is confined to the codemod package (importPaths.ts insertion-index computation, addOrMergeImport return value and blank-line trivia, plus tests and a changeset) and touches no security-sensitive surface. Inline findings already cover the directive blank-line regression, the trailing-comment blank-line miss, and the pre-existing gaps left for removedApis/symbolRenames paths and merge-only removals, so approval is not appropriate; this note only records what else was examined and ruled out.

claude · 2026-09-28T14:22:58Z

            namedImports: deduped.map(s => (s.alias ? { name: s.name, alias: s.alias } : { name: s.name })),
-            isTypeOnly
+            isTypeOnly,
+            leadingTrivia: blankLineAbove ? writer => writer.blankLineIfLastNot() : undefined


🟡 (optional) Users with a directive directly above their SDK import, with no blank line, get an extra blank line inserted between the directive and the rewritten import, a diff the base branch never produced in that spot. importUtils.ts:100-106 inserts at a child index whose previous sibling is an ExpressionStatement, and ts-morph's _standardWrite emits blankLineIfLastNot for any previous member that is not an import or comment. Fix: only accept ts-morph's separator when blankLineAbove is true; otherwise re-emit the original gap (as leadingCommentGap does) so 'use strict';\nimport round-trips byte-for-byte, and add a no-blank-line directive test alongside the existing 'use client' test at importPaths.test.ts.

Why this was flagged

Trigger: a file beginning 'use strict'; or 'use client'; immediately followed (no blank line) by a v1 SDK import that the transform removes and re-emits (importPaths.ts:333 then :361). Entry point: every codemod run over a Node/Next.js codebase; directive-first files are common in CommonJS-style TS and React server/client components. importPaths.ts:59 computes blankLineAbove = false for this input, and importUtils.ts:105 therefore passes no leadingTrivia. But ts-morph's insertImportDeclarations uses _standardWrite with previousNewLine only true for imports and comments; for an ExpressionStatement previous member it calls writer.blankLineIfLastNot() itself, so the output becomes 'use strict';\n\nimport .... The new test at importPaths.test.ts (keeps a 'use client' directive above the rewritten import) only covers the input that already has a blank line, so the added line is never asserted against. The base branch inserted at import index 0, above the directive (a different wrong), so the byte change is new for this population and produces an unexpected formatting diff in…

Verification: nit — triggered when a file's first SDK import sits directly under a directive prologue ('use strict'; / 'use client';) with no blank line and goes down the remove-and-re-emit path. Mechanism verified in the diff: importPaths.ts:57-59 sets childIndex = index of the SDK import among getStatementsWithComments() (1 when the directive is child 0) and blankLineAbove = false for `'use… | nit…

claude · 2026-09-28T14:22:58Z

+        // ts-morph inserts by position among all top-level children (own-line comments and statements count), not among imports.
+        const childIndex = sdkImports[0]!.getChildIndex();
+        const previousEnd = childIndex > 0 ? sourceFile.getStatementsWithComments()[childIndex - 1]!.getEnd() : 0;
+        const blankLineAbove = childIndex > 0 && /^[ \t]*\r?\n[ \t]*\r?\n/.test(sourceFile.getFullText().slice(previousEnd));


🟡 nit (optional): a blank line between a preceding import that ends in a trailing comment and the re-emitted SDK import is dropped after migration. blankLineAbove at importPaths.ts:59 tests the text from getEnd(), which for import a from 'a'; // note starts at // note, so the regex never sees the blank line and the flag stays false. Fix: measure the gap from the previous sibling's trailing-trivia end (getTrailingTriviaEnd()) rather than getEnd() so a same-line trailing comment on any preceding statement or comment does not hide the blank line; previousEnd for the leadingRanges filter can use the same position since trailing comments are never leading ranges of the import.

Why this was flagged

Input: import a from 'a'; // eslint-disable-line\n\nimport { X } from '@ modelcontextprotocol/sdk/types.js';, run through importPathsTransform.apply via the runner. importPaths.ts:58 sets previousEnd to the previous import's getEnd(), which is the position right after ;, so importPaths.ts:59 slices // eslint-disable-line\n\nimport and the regex ^[ \t]*\r?\n[ \t]*\r?\n fails; blankLineAbove is false. The SDK import is removed at importPaths.ts:333 and re-inserted at importPaths.ts:361 with blankLineAbove false, so importUtils.ts:105 emits no leadingTrivia; ts-morph writes only a newline between two import declarations. Result: import a from 'a'; // eslint-disable-line\nimport { X } from "@ modelcontextprotocol/server"; with the separating blank line gone. On the base branch the import went to line 1 above everything, so this is an improvement, but the PR's stated blank-line preservation does not hold for this shape. When the previous sibling is a non-import statement ts-morph adds a blank line itself, so only the previous-import (and comment-node) cases are affected.

Verification: nit — triggered when the statement directly above the first SDK import carries a same-line trailing comment (e.g. import a from 'a'; // eslint-disable-line) and is separated from it by a blank line. Mechanism verified in /home/claude/typescript-sdk/packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts: line 58 `const previousEnd = childIndex > 0 ?… | nit — triggered when the…

claude · 2026-09-28T14:22:58Z

        });

-        const insertIndex = sourceFile.getImportDeclarations().indexOf(sdkImports[0]!);
+        const importIndex = sourceFile.getImportDeclarations().indexOf(sdkImports[0]!);


🟡 nit (optional): CLAUDE.md formatting asks for 2-space indentation: every added line in importPaths.ts, importUtils.ts, commentInsertion.test.ts and importPaths.test.ts is indented in 4-space steps (e.g. 8 spaces before const importIndex). Semicolons and single quotes are fine. Fix: the repo's committed .prettierrc sets tabWidth: 4 and pnpm lint:all enforces it, so re-indenting to 2 spaces would fail lint; the compliant resolution is to bring the CLAUDE.md line in line with the Prettier config (or vice versa) rather than reformat this diff. Same instruction across all 4 changed code files.

Why this was flagged

Nothing fails at runtime. The written rule (2-space) and the enforced tool config (.prettierrc tabWidth 4, checked by pnpm lint:all) contradict each other, and this diff follows the tool rather than the text, as does all existing code in packages/codemod. The only cost is that contributors reading CLAUDE.md are told a convention the linter rejects; a one-line edit to CLAUDE.md removes the contradiction. Filed only because the instruction as written is broken by the added lines; this is a wording/layout nit for the maintainer to weigh.

Verification: Base CLAUDE.md § Code Style Guidelines reads verbatim "- Formatting: 2-space indentation, semicolons required, single quotes preferred", and it binds the whole repo (no nested CLAUDE.md under packages/codemod). Added line packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts:55 const importIndex = sourceFile.getImportDeclarations().indexOf(sdkImports[0]!); is…

claude · 2026-09-28T14:22:58Z

            return true;
        });
-        const clampedIndex = Math.min(insertIndex, sourceFile.getImportDeclarations().length);
+        const clampedIndex = Math.min(insertIndex, sourceFile.getStatementsWithComments().length);


🟣 pre-existing, not blocking: pre-existing: a user whose only SDK import is StreamableHTTPError (or a schema-input import) still gets the replacement import placed above a license header or 'use client' directive, the #2575 shape this PR fixes. importUtils.ts:100 now clamps and inserts by statement index, but removedApis.ts:201 and symbolRenames.ts:737 still pass getImportDeclarations().length, an import count. With a comment node or directive at index 0 that lands before the last import, or at index 0 above the header when no import remains. Fix: pass every caller a statement-based position (child index after the last import via getStatementsWithComments, or let addOrMergeImport translate an import-relative index), which covers the 2 sites listed. Same pattern at 2 sites (removedApis.ts:201, symbolRenames.ts:737). [also at: packages/codemod/src/utils/importUtils.ts:100 - nit: REVIEW.md Completeness asks that a replaced pattern be swept from the package and every leftover site flagged: this PR changes addOrMergeImport to treat insertIndex as a position among getStatementsWithComments() and reworks importPaths.ts to compute it from getChildIndex(), but the…]
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

Input: a file starting with // Copyright ... or 'use client';, a blank line, and import { StreamableHTTPError } from '@ modelcontextprotocol/sdk/client/streamableHttp.js'; as its only import, run through the v1-to-v2 migration. importPaths keeps the header in place (the new code at importPaths.ts:345-346), but handleStreamableHTTPError in removedApis.ts:195-197 then removes the emptied declaration and at removedApis.ts:201 computes insertIndex = sourceFile.getImportDeclarations().length, which is 0. addOrMergeImport at importUtils.ts:100-101 clamps against getStatementsWithComments().length and calls insertImportDeclaration(0), so the SdkHttpError import is written before the header comment or directive at statement index 0. The header is…

Verification: pre-existing; acknowledged in diff: .changeset/codemod-preserve-file-header.md explicitly says "Known gap: when a later step of the codemod replaces or removes the import (for example a file whose only SDK import is ErrorCode or StreamableHTTPError), the new import can still land above or inside the header" — the note is accurate, and the base branch already produces the same output…

claude · 2026-09-28T14:22:58Z

+            let valueInserted = false;
            if (valueSpecs.size > 0) {
-                addOrMergeImport(sourceFile, target, [...valueSpecs.values()], false, insertIndex);
+                valueInserted = addOrMergeImport(
+                    sourceFile,
+                    target,
+                    [...valueSpecs.values()],
+                    false,
+                    insertIndex,
+                    firstRemoved && blankLineAbove
+                );
            }
            if (typeOnlySpecs.size > 0) {
-                const typeInsertIndex = valueSpecs.size > 0 ? insertIndex + 1 : insertIndex;
-                addOrMergeImport(sourceFile, target, [...typeOnlySpecs.values()], true, typeInsertIndex);
+                // The type import goes one lower only when the value import was inserted, not when it merged into an existing import.
+                const typeInsertIndex = valueInserted ? insertIndex + 1 : insertIndex;
+                const blankLine = firstRemoved && blankLineAbove && !valueInserted;
+                addOrMergeImport(sourceFile, target, [...typeOnlySpecs.values()], true, typeInsertIndex, blankLine);
            }


🟣 pre-existing, not blocking: Pre-existing gap the fix leaves open: a header's blank line is still consumed when the removed first SDK import produces no new declaration. The blank line is only re-emitted through the leadingTrivia of an inserted import (importPaths.ts:367 and :373), so when every pending value import merges into an existing v2 import and there are no type-only specs, or when the removed import yields no pending imports at all, nothing carries blankLineAbove and the header ends up touching the next statement. Fix: after the pending loop, when firstRemoved && blankLineAbove and no declaration was inserted at childIndex, re-insert the blank line above the statement now at childIndex (e.g. insertText of '\n' at its start), so the header keeps its separation on every path, not only the insert path.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

Input: '// Copyright (c) 2026 Example Corp.', a blank line, 'import { StreamableHTTPClientTransport } from '@ modelcontextprotocol/sdk/client/streamableHttp.js';', then 'import { Client } from '@ modelcontextprotocol/client';' (a partially migrated file, the same mixed state the PR's own test at importPaths.test.ts:905 models). importPaths.ts:57-59 computes childIndex=1 and blankLineAbove=true. importPaths.ts:333 calls imp.remove(); ts-morph joins the comment node and the Client import with a single newline, consuming the blank line. importPaths.ts:361 then calls addOrMergeImport, which finds the existing '@ modelcontextprotocol/client' import (importUtils.ts:80-91) and merges, returning false and never emitting the blankLineIfLastNot trivia; typeOnlySpecs is empty so importPaths.ts:374 never runs. Output is '// Copyright (c) 2026 Example Corp.' directly followed by 'import { Client, StreamableHTTPClientTransport } ...', the header now reads as a doc comment on the import. The same happens when the removed import's only specifiers are removedSymbols (no addPending) or the module…

Verification: pre-existing — the base already drops the blank line on this path by the same route, and the PR's fix does not reach it. Trigger: the first SDK import sits under a header + blank line and is removed (line 333 imp.remove(), or line 155 for a status: 'removed' module such as @ modelcontextprotocol/sdk/client/websocket.js, importMap.ts:75-77) but produces no inserted declaration: either…

axits-lab requested a review from a team as a code owner July 30, 2026 13:22

claude Bot added the v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes label Aug 18, 2026

felixweinberger added 2 commits September 28, 2026 14:00

Merge branch 'main' into fix/codemod-preserve-file-header

f905836

fix(codemod): write rewritten imports where the v1 import stood

529895d

felixweinberger approved these changes Sep 28, 2026

View reviewed changes

felixweinberger merged commit f091897 into modelcontextprotocol:main Sep 28, 2026
16 checks passed

github-actions Bot mentioned this pull request Sep 28, 2026

Version Packages #2849

Merged

claude Bot reviewed Sep 28, 2026

View reviewed changes

This was referenced Sep 28, 2026

Version Packages wsnchristopher/typescript-sdk#2

Open

Version Packages ryoppippi/typescript-sdk#175

Open

Version Packages p5hzehxa/typescript-sdk#41

Open

Version Packages TheRakeshPurohit/typescript-sdk#218

Open

Navigation Menu

fix(codemod): keep a file's leading comment block above rewritten imports - #2582

fix(codemod): keep a file's leading comment block above rewritten imports#2582
felixweinberger merged 3 commits into
modelcontextprotocol:mainfrom
axits-lab:fix/codemod-preserve-file-header

axits-lab commented Jul 30, 2026

Uh oh!

changeset-bot Bot commented Jul 30, 2026 •

edited

Loading

Uh oh!

pkg-pr-new Bot commented Jul 30, 2026 •

edited

Loading

Uh oh!

felixweinberger commented Sep 28, 2026

Uh oh!

felixweinberger commented Sep 28, 2026

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

claude Bot Sep 28, 2026

Uh oh!

claude Bot Sep 28, 2026

Uh oh!

claude Bot Sep 28, 2026

Uh oh!

claude Bot Sep 28, 2026

Uh oh!

claude Bot Sep 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

axits-lab commented Jul 30, 2026

Root cause

Fix

On the existing tests

Verification

Uh oh!

changeset-bot Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

pkg-pr-new Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixweinberger commented Sep 28, 2026

Uh oh!

felixweinberger commented Sep 28, 2026

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot Bot commented Jul 30, 2026 •

edited

Loading

pkg-pr-new Bot commented Jul 30, 2026 •

edited

Loading