diff --git a/.changeset/codemod-preserve-file-header.md b/.changeset/codemod-preserve-file-header.md new file mode 100644 index 0000000000..b686d36080 --- /dev/null +++ b/.changeset/codemod-preserve-file-header.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/codemod': patch +--- + +The `v1-to-v2` codemod now writes rewritten imports where the first v1 import stood, not at the top of the file, so a license header, `// @ts-nocheck`, `/// ` or a `'use client'` / `'use server'` / `'use strict'` directive above it stays in place. 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, and a `/** */` header can be removed. Files already migrated with codemod 2.1.0 or earlier are not repaired; check the top of those files. diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts index 1d05a307f1..f4d338e4b7 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts @@ -52,7 +52,11 @@ export const importPathsTransform: Transform = { return spec.includes('/server/'); }); - const insertIndex = sourceFile.getImportDeclarations().indexOf(sdkImports[0]!); + const importIndex = sourceFile.getImportDeclarations().indexOf(sdkImports[0]!); + // 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)); // A leading file-header / JSDoc comment attaches to the first SDK import as leading trivia. When // that import is removed and re-emitted (the per-symbol split/merge path calls imp.remove()), @@ -62,9 +66,12 @@ export const importPathsTransform: Transform = { // (a blank line, or CRLF in CRLF files), so the later survival check would never match a header // that actually survived (in-place setModuleSpecifier rewrite) and would re-insert it, duplicating // it. The slice reproduces the block verbatim, so the includes() guard below is byte-exact. - const leadingRanges = sdkImports[0]!.getLeadingCommentRanges(); + // Own-line comments above the import are siblings and survive its removal; only comments attached to it can be dropped. + const leadingRanges = sdkImports[0]!.getLeadingCommentRanges().filter(range => range.getPos() >= previousEnd); const leadingCommentText = leadingRanges.length > 0 ? sourceFile.getFullText().slice(leadingRanges[0]!.getPos(), leadingRanges.at(-1)!.getEnd()) : ''; + const leadingCommentGap = + leadingRanges.length > 0 ? sourceFile.getFullText().slice(leadingRanges.at(-1)!.getEnd(), sdkImports[0]!.getStart()) : ''; interface PendingImport { specs: NamedImportSpec[]; @@ -334,6 +341,10 @@ export const importPathsTransform: Transform = { } } + // New imports go where the first SDK import stood, or right below it when it was rewritten in place and still stands there. + const firstRemoved = sdkImports[0]!.wasForgotten(); + const insertIndex = firstRemoved ? childIndex : childIndex + 1; + const specLocal = (spec: NamedImportSpec): string => (typeof spec === 'string' ? spec : (spec.alias ?? spec.name)); for (const [target, groups] of pendingImports) { // Dedupe by local binding name (alias when present), keeping the spec so aliases survive. @@ -345,12 +356,22 @@ export const importPathsTransform: Transform = { } } + 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); } } @@ -358,8 +379,8 @@ export const importPathsTransform: Transform = { // the first import was rewritten in place and kept its comment). if (leadingCommentText && !sourceFile.getFullText().includes(leadingCommentText)) { const imports = sourceFile.getImportDeclarations(); - const anchor = imports[Math.min(insertIndex, imports.length - 1)]; - sourceFile.insertText(anchor ? anchor.getStart() : 0, `${leadingCommentText}\n`); + const anchor = imports[Math.min(importIndex, imports.length - 1)]; + sourceFile.insertText(anchor ? anchor.getStart() : 0, `${leadingCommentText}${leadingCommentGap}`); } return { changesCount, diagnostics, usedPackages }; diff --git a/packages/codemod/src/utils/importUtils.ts b/packages/codemod/src/utils/importUtils.ts index e2e6c70a49..24eaa0cf5f 100644 --- a/packages/codemod/src/utils/importUtils.ts +++ b/packages/codemod/src/utils/importUtils.ts @@ -64,14 +64,16 @@ function specLocalName(s: { name: string; alias?: string }): string { return s.alias ?? s.name; } +/** Adds the names to an existing import of the module, or inserts a new import; returns true when it inserted one. */ export function addOrMergeImport( sourceFile: SourceFile, moduleSpecifier: string, namedImports: NamedImportSpec[], isTypeOnly: boolean, - insertIndex: number -): void { - if (namedImports.length === 0) return; + insertIndex: number, + blankLineAbove = false +): boolean { + if (namedImports.length === 0) return false; const specs = namedImports.map(n => toSpec(n)); @@ -86,6 +88,7 @@ export function addOrMergeImport( if (newSpecs.length > 0) { existing.addNamedImports(newSpecs.map(s => (s.alias ? { name: s.name, alias: s.alias } : { name: s.name }))); } + return false; } else { const seen = new Set(); const deduped = specs.filter(s => { @@ -94,12 +97,14 @@ export function addOrMergeImport( seen.add(local); return true; }); - const clampedIndex = Math.min(insertIndex, sourceFile.getImportDeclarations().length); + const clampedIndex = Math.min(insertIndex, sourceFile.getStatementsWithComments().length); sourceFile.insertImportDeclaration(clampedIndex, { moduleSpecifier, namedImports: deduped.map(s => (s.alias ? { name: s.name, alias: s.alias } : { name: s.name })), - isTypeOnly + isTypeOnly, + leadingTrivia: blankLineAbove ? writer => writer.blankLineIfLastNot() : undefined }); + return true; } } diff --git a/packages/codemod/test/commentInsertion.test.ts b/packages/codemod/test/commentInsertion.test.ts index 255cb9e581..689c949363 100644 --- a/packages/codemod/test/commentInsertion.test.ts +++ b/packages/codemod/test/commentInsertion.test.ts @@ -330,6 +330,36 @@ describe('comment insertion', () => { }); describe('markers whose import-declaration anchor is removed by the same pass', () => { + it('keeps a license header first and still writes the marker below it (#2575)', () => { + const dir = createTempDir(); + writeFileSync( + path.join(dir, 'package.json'), + JSON.stringify({ name: 'app', dependencies: { '@modelcontextprotocol/sdk': '^1.29.0' } }) + ); + const file = path.join(dir, 'auth.ts'); + writeFileSync( + file, + [ + `// Copyright (c) 2026 Example Corp.`, + `// SPDX-License-Identifier: Apache-2.0`, + ``, + `import { requireBearerAuth } from '@modelcontextprotocol/sdk/server/auth/middleware/bearerAuth.js';`, + ``, + `export const guard = requireBearerAuth({ verifier });`, + '' + ].join('\n') + ); + + const result = run(migration, { targetDir: dir, dryRun: false }); + + const lines = readFileSync(file, 'utf8').split('\n'); + expect(lines.slice(0, 3)).toEqual(['// Copyright (c) 2026 Example Corp.', '// SPDX-License-Identifier: Apache-2.0', '']); + expect(lines[3]).toBe(`import { requireBearerAuth } from "@modelcontextprotocol/server-legacy/auth";`); + const markerIndex = lines.findIndex(line => line.includes(CODEMOD_ERROR_PREFIX)); + expect(lines[markerIndex + 1]).toContain('requireBearerAuth({ verifier })'); + expect(result.diagnostics.find(d => d.insertComment)?.line).toBe(markerIndex + 1); + }); + it('inserts the resource-server auth helper marker at the usage site', () => { const dir = createTempDir(); writeFileSync( diff --git a/packages/codemod/test/v1-to-v2/transforms/importPaths.test.ts b/packages/codemod/test/v1-to-v2/transforms/importPaths.test.ts index 933cf02e29..34fdaf4e4b 100644 --- a/packages/codemod/test/v1-to-v2/transforms/importPaths.test.ts +++ b/packages/codemod/test/v1-to-v2/transforms/importPaths.test.ts @@ -204,6 +204,135 @@ describe('import-paths transform', () => { expect(result).toContain('@modelcontextprotocol/server'); }); + it('keeps a license header above the rewritten import, with its blank line intact', () => { + // #2575: the header survived in the text but the re-emitted import was inserted above it, so + // the header stopped being the first thing in the file (breaking eslint-plugin-header / + // SPDX scanners) and the blank line separating it from the code was consumed, turning the + // header into a doc comment for the next declaration. Content-only assertions miss both. + const input = [ + `// Copyright (c) 2026 Example Corp.`, + `// SPDX-License-Identifier: Apache-2.0`, + ``, + `import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js';`, + ``, + `export const ok = (): CallToolResult => ({ content: [] });`, + '' + ].join('\n'); + const result = applyTransform(input, { projectType: 'server' }); + const lines = result.split('\n'); + + expect(lines[0]).toBe('// Copyright (c) 2026 Example Corp.'); + expect(lines[1]).toBe('// SPDX-License-Identifier: Apache-2.0'); + // the blank line between header and code must survive, or the header reads as a doc comment + expect(lines[2]).toBe(''); + expect(lines.findIndex(l => l.startsWith('import'))).toBeGreaterThan(1); + expect(result).toContain('@modelcontextprotocol/server'); + expect(result).toBe(input.replace(`'@modelcontextprotocol/sdk/types.js'`, `"@modelcontextprotocol/server"`)); + }); + + it('does not split a multi-line header run when the SDK import is not the first import', () => { + // #2575, second shape: the rewritten import was inserted *inside* the leading `//` run, + // stranding line 1 above an unrelated import. + const input = [ + `// page_to_markdown tool: fetches a URL and returns clean Markdown.`, + `// 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';`, + `import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js';`, + ``, + `export const x = (r: CallToolResult) => fetchAndConvert(r);`, + '' + ].join('\n'); + const result = applyTransform(input, { projectType: 'server' }); + const lines = result.split('\n'); + + // the three header lines stay contiguous at the top of the file + expect(lines[0]).toContain('page_to_markdown tool'); + expect(lines[1]).toContain('@page2ai/core under the hood'); + expect(lines[2]).toContain('Static tab discovery'); + expect(lines.findIndex(l => l.startsWith('import'))).toBeGreaterThan(2); + expect(result).toContain('@modelcontextprotocol/server'); + expect(result).toBe(input.replace(`'@modelcontextprotocol/sdk/types.js'`, `"@modelcontextprotocol/server"`)); + }); + + it('leaves a comment above a mid-file import attached to that import', () => { + // A comment above a later import documents that import: it stays directly above it. + const input = [ + `import { fetchAndConvert } from '@page2ai/core';`, + ``, + `// Result type returned to the caller.`, + `import type { CallToolResult } from '@modelcontextprotocol/sdk/types.js';`, + ``, + `export const x = (r: CallToolResult) => fetchAndConvert(r);`, + '' + ].join('\n'); + const result = applyTransform(input, { projectType: 'server' }); + + expect(result.split('// Result type returned to the caller.').length - 1).toBe(1); + expect(result.indexOf('// Result type returned to the caller.')).toBeGreaterThan(result.indexOf('@page2ai/core')); + expect(result).toBe(input.replace(`'@modelcontextprotocol/sdk/types.js'`, `"@modelcontextprotocol/server"`)); + }); + + it(`keeps a 'use client' directive above the rewritten import`, () => { + const input = [ + `'use client';`, + ``, + `import { Client } from '@modelcontextprotocol/sdk/client/index.js';`, + `import { useState } from 'react';`, + '' + ].join('\n'); + const expected = input.replace(`'@modelcontextprotocol/sdk/client/index.js'`, `"@modelcontextprotocol/client"`); + expect(applyTransform(input)).toBe(expected); + }); + + it('keeps the blank line below a JSDoc-style license header', () => { + const input = [ + `/**`, + ` * @license Apache-2.0`, + ` */`, + ``, + `import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';`, + ``, + `export const server = new McpServer({ name: 'x', version: '1.0.0' });`, + '' + ].join('\n'); + const expected = input.replace(`'@modelcontextprotocol/sdk/server/mcp.js'`, `"@modelcontextprotocol/server"`); + expect(applyTransform(input)).toBe(expected); + }); + + it('does not duplicate a license header that is followed by a JSDoc block', () => { + const input = [ + `// Copyright (c) 2026 Example Corp.`, + ``, + `/**`, + ` * What this file does.`, + ` */`, + ``, + `import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';`, + ``, + `export const server = new McpServer({ name: 'x', version: '1.0.0' });`, + '' + ].join('\n'); + const expected = input.replace(`'@modelcontextprotocol/sdk/server/mcp.js'`, `"@modelcontextprotocol/server"`); + expect(applyTransform(input)).toBe(expected); + }); + + it('puts new imports below a namespace import that is rewritten in place, not above the header', () => { + const input = [ + `/** @license Apache-2.0 */`, + ``, + `import * as types from '@modelcontextprotocol/sdk/types.js';`, + `import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';`, + '' + ].join('\n'); + const lines = applyTransform(input, { projectType: 'server' }).split('\n'); + expect(lines[0]).toBe(`/** @license Apache-2.0 */`); + expect(lines[1]).toBe(''); + expect(lines[2]).toContain('* as types'); + expect(lines[3]).toBe(`import { McpServer } from "@modelcontextprotocol/server";`); + }); + it('routes OAuth *Schema from sdk/shared/auth.js to core; the TYPE resolves by context', () => { // OAuthTokensSchema is a Zod schema re-exported by core (AUTH_SCHEMA_NAMES), so route it // there — `OAuthTokensSchema.parse(...)` keeps working. OAuthTokens (the type) has no schema-name @@ -766,6 +895,29 @@ describe('import-paths transform', () => { const output = sourceFile.getFullText(); expect(output).toContain('@modelcontextprotocol/client'); expect(output).not.toContain('@modelcontextprotocol/sdk'); + const lines = output.split('\n'); + expect(lines[0]).toBe(`import { Client, StreamableHTTPClientTransport } from '@modelcontextprotocol/client';`); + expect(lines[1]).toBe(`import type { Tool } from "@modelcontextprotocol/client";`); + expect(lines.findLastIndex(line => line.startsWith('import '))).toBeLessThan(lines.findIndex(line => line.startsWith('const c'))); + }); + + it('keeps the type import with the imports when the value import merges into an existing v2 import', () => { + const input = [ + `// Copyright (c) 2026 Example Corp.`, + `// SPDX-License-Identifier: Apache-2.0`, + ``, + `import { Client } from '@modelcontextprotocol/client';`, + `import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js';`, + `import type { Tool } from '@modelcontextprotocol/sdk/types.js';`, + ``, + `const c = new Client({});`, + '' + ].join('\n'); + const lines = applyTransform(input, { projectType: 'client' }).split('\n'); + expect(lines.slice(0, 3)).toEqual(['// Copyright (c) 2026 Example Corp.', '// SPDX-License-Identifier: Apache-2.0', '']); + expect(lines[3]).toBe(`import { Client, StreamableHTTPClientTransport } from '@modelcontextprotocol/client';`); + expect(lines[4]).toBe(`import type { Tool } from "@modelcontextprotocol/client";`); + expect(lines.findLastIndex(line => line.startsWith('import '))).toBeLessThan(lines.findIndex(line => line.startsWith('const c'))); }); it('applies SIMPLE_RENAMES to re-export specifiers', () => {