From 646a1d6ae22cb0a3d9ff94a45d4235928664efc1 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Tue, 2 Jun 2026 07:58:40 +0000 Subject: [PATCH 1/5] codemod: add error-code-semantics transform for relocated error codes ErrorCode.RequestTimeout and ErrorCode.ConnectionClosed moved to SdkErrorCode (a string enum raised on SdkError) in v2. The symbol rename alone leaves guards like 'e instanceof ProtocolError && e.code === SdkErrorCode.RequestTimeout' compiling but never matching at runtime. The new transform rewrites such guards to SdkError where that is unambiguous, and emits diagnostics for switches and maps that mix the two enums. --- .changeset/codemod-error-code-semantics.md | 10 + .../v1-to-v2/transforms/errorCodeSemantics.ts | 243 ++++++++++++++++++ .../migrations/v1-to-v2/transforms/index.ts | 18 +- .../transforms/errorCodeSemantics.test.ts | 203 +++++++++++++++ 4 files changed, 469 insertions(+), 5 deletions(-) create mode 100644 .changeset/codemod-error-code-semantics.md create mode 100644 packages/codemod/src/migrations/v1-to-v2/transforms/errorCodeSemantics.ts create mode 100644 packages/codemod/test/v1-to-v2/transforms/errorCodeSemantics.test.ts diff --git a/.changeset/codemod-error-code-semantics.md b/.changeset/codemod-error-code-semantics.md new file mode 100644 index 0000000000..5180809bca --- /dev/null +++ b/.changeset/codemod-error-code-semantics.md @@ -0,0 +1,10 @@ +--- +'@modelcontextprotocol/codemod': minor +--- + +Add an `error-code-semantics` transform that fixes `instanceof` guards around the +`ErrorCode.RequestTimeout` / `ErrorCode.ConnectionClosed` members, which moved from the +protocol error enum to `SdkErrorCode` (raised on `SdkError`) in v2. Without this, migrated +checks like `e instanceof ProtocolError && e.code === SdkErrorCode.RequestTimeout` compile +but never match at runtime. The transform also flags switches and maps that mix the two +enums, since `SdkErrorCode` is a string enum. diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/errorCodeSemantics.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/errorCodeSemantics.ts new file mode 100644 index 0000000000..cb69b37f02 --- /dev/null +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/errorCodeSemantics.ts @@ -0,0 +1,243 @@ +import type { BinaryExpression, SourceFile } from 'ts-morph'; +import { Node, SyntaxKind } from 'ts-morph'; + +import type { Diagnostic, Transform, TransformContext, TransformResult } from '../../../types.js'; +import { error, warning } from '../../../utils/diagnostics.js'; +import { addOrMergeImport, isImportedFromMcp } from '../../../utils/importUtils.js'; +import { ERROR_CODE_SDK_MEMBERS } from '../mappings/symbolMap.js'; + +/** + * In v1, `ErrorCode.RequestTimeout` and `ErrorCode.ConnectionClosed` were numeric + * members of the protocol error enum and were raised on `McpError`. In v2 they moved + * to `SdkErrorCode` — a STRING enum — and are raised on `SdkError`, while protocol + * errors became `ProtocolError` with `ProtocolErrorCode`. + * + * The symbol rename transform updates the enum references, but a check like + * + * e instanceof ProtocolError && e.code === SdkErrorCode.RequestTimeout + * + * still compiles under loose typing and NEVER matches at runtime, because v2 raises + * timeouts/disconnects as `SdkError`, not `ProtocolError`. This transform rewrites the + * `instanceof` side of such checks to `SdkError` where that is safe, and emits + * diagnostics where it is not: + * + * - `subject instanceof ProtocolError/McpError` guarding a comparison against a moved + * member → guard rewritten to `subject instanceof SdkError` (import added). + * - comparison against a moved member with no detectable `instanceof` guard → warning + * to verify the subject is checked as `SdkError`. + * - `instanceof` guard against an unrecognized class → error with the exact manual fix. + * - `switch` over `x.code` mixing `SdkErrorCode` and `ProtocolErrorCode` cases → error + * (the enums live on different error classes in v2; the switch must be split). + * - object literals keyed by a moved member → warning (`SdkErrorCode` is a string enum; + * previously-numeric map keys are now strings). + * + * Both the post-rename (`SdkErrorCode.X`) and pre-rename (`ErrorCode.X`) spellings are + * recognized, so the transform composes with the symbol rename transform in either + * order; it never rewrites the enum reference itself. + */ + +const MOVED_MEMBER_ENUMS = new Set(['SdkErrorCode', 'ErrorCode']); +const V1_ERROR_CLASSES = new Set(['ProtocolError', 'McpError']); + +export const errorCodeSemanticsTransform: Transform = { + name: 'Error code semantics', + id: 'error-code-semantics', + apply(sourceFile: SourceFile, context: TransformContext): TransformResult { + const diagnostics: Diagnostic[] = []; + const filePath = sourceFile.getFilePath(); + let changesCount = 0; + let needsSdkErrorImport = false; + + // Collect first, mutate after — mutating while iterating invalidates descendants. + const movedMemberAccesses: Node[] = []; + sourceFile.forEachDescendant(node => { + if (!Node.isPropertyAccessExpression(node)) return; + const expr = node.getExpression(); + if (!Node.isIdentifier(expr)) return; + if (!MOVED_MEMBER_ENUMS.has(expr.getText())) return; + if (!ERROR_CODE_SDK_MEMBERS.has(node.getName())) return; + movedMemberAccesses.push(node); + }); + + const guardsToRewrite = new Set(); + const flaggedObjectLiterals = new Set(); + const flaggedSwitches = new Set(); + + for (const access of movedMemberAccesses) { + const memberName = access.getChildAtIndex(2)?.getText() ?? access.getText(); + const line = access.getStartLineNumber(); + + const comparison = getComparisonContext(access); + if (comparison) { + const guard = findInstanceofGuard(access, comparison.subjectText); + if (guard) { + const className = guard.getRight().getText(); + if (V1_ERROR_CLASSES.has(className)) { + guardsToRewrite.add(guard); + } else if (className !== 'SdkError') { + diagnostics.push( + error( + filePath, + line, + `\`${comparison.subjectText}.code === SdkErrorCode.${memberName}\` is guarded by ` + + `\`instanceof ${className}\`, but v2 raises ${memberName} on SdkError. ` + + `Manual fix: \`${comparison.subjectText} instanceof SdkError && ` + + `${comparison.subjectText}.code === SdkErrorCode.${memberName}\`.` + ) + ); + } + } else { + diagnostics.push( + warning( + filePath, + line, + `Comparison against SdkErrorCode.${memberName}: in v2 this code is raised on SdkError ` + + `(not McpError/ProtocolError as in v1). Verify \`${comparison.subjectText}\` is checked ` + + `with \`instanceof SdkError\` before this comparison.` + ) + ); + } + continue; + } + + const caseClause = access.getFirstAncestorByKind(SyntaxKind.CaseClause); + if (caseClause && caseClause.getExpression() === access) { + const switchStmt = caseClause.getFirstAncestorByKind(SyntaxKind.SwitchStatement); + if (switchStmt && !flaggedSwitches.has(switchStmt)) { + flaggedSwitches.add(switchStmt); + const mixesProtocolCodes = switchStmt + .getDescendantsOfKind(SyntaxKind.PropertyAccessExpression) + .some(p => Node.isIdentifier(p.getExpression()) && p.getExpression().getText() === 'ProtocolErrorCode'); + if (mixesProtocolCodes) { + diagnostics.push( + error( + filePath, + line, + `switch mixes SdkErrorCode and ProtocolErrorCode cases. In v2 these enums live on ` + + `different error classes (SdkError vs ProtocolError), so a single switch over ` + + `\`.code\` cannot handle both. Split into an \`instanceof SdkError\` branch and an ` + + `\`instanceof ProtocolError\` branch.` + ) + ); + } else { + diagnostics.push( + warning( + filePath, + line, + `switch case on SdkErrorCode.${memberName}: in v2 this code is raised on SdkError. ` + + `Verify the switch subject is checked with \`instanceof SdkError\`.` + ) + ); + } + } + continue; + } + + const computedName = access.getFirstAncestorByKind(SyntaxKind.ComputedPropertyName); + if (computedName && computedName.getExpression() === access) { + const objectLiteral = computedName.getFirstAncestorByKind(SyntaxKind.ObjectLiteralExpression); + if (objectLiteral && !flaggedObjectLiterals.has(objectLiteral)) { + flaggedObjectLiterals.add(objectLiteral); + const mixesProtocolCodes = objectLiteral + .getDescendantsOfKind(SyntaxKind.PropertyAccessExpression) + .some(p => Node.isIdentifier(p.getExpression()) && p.getExpression().getText() === 'ProtocolErrorCode'); + diagnostics.push( + warning( + filePath, + line, + `Map keyed by SdkErrorCode.${memberName}: SdkErrorCode is a string enum in v2, so this key ` + + `changed from a number to a string.` + + (mixesProtocolCodes + ? ` This literal also has ProtocolErrorCode keys (numeric) — split it into separate maps.` + : ` Update any \`Record\` typing and numeric lookups accordingly.`) + ) + ); + } + } + } + + for (const guard of guardsToRewrite) { + guard.getRight().replaceWithText('SdkError'); + needsSdkErrorImport = true; + changesCount++; + } + + if (needsSdkErrorImport && !isImportedFromMcp(sourceFile, 'SdkError')) { + const targetModule = resolveTargetModule(sourceFile, context); + addOrMergeImport(sourceFile, targetModule, ['SdkError'], false, sourceFile.getImportDeclarations().length); + changesCount++; + } + + return { changesCount, diagnostics }; + } +}; + +interface ComparisonContext { + subjectText: string; +} + +/** + * If `access` is one side of a `===`/`!==`/`==`/`!=` comparison whose other side is a + * `.code` property access, return the subject expression text. + */ +function getComparisonContext(access: Node): ComparisonContext | undefined { + const parent = access.getParent(); + if (!parent || !Node.isBinaryExpression(parent)) return undefined; + const op = parent.getOperatorToken().getKind(); + if ( + op !== SyntaxKind.EqualsEqualsEqualsToken && + op !== SyntaxKind.ExclamationEqualsEqualsToken && + op !== SyntaxKind.EqualsEqualsToken && + op !== SyntaxKind.ExclamationEqualsToken + ) { + return undefined; + } + const other = parent.getLeft() === access ? parent.getRight() : parent.getLeft(); + if (!Node.isPropertyAccessExpression(other)) return undefined; + if (other.getName() !== 'code') return undefined; + return { subjectText: other.getExpression().getText() }; +} + +/** + * Search the enclosing conditions for `subject instanceof `. Covers the same + * logical-AND chain (`e instanceof X && e.code === ...`) and enclosing if/ternary + * conditions (`if (e instanceof X) { if (e.code === ...) ... }`). + */ +function findInstanceofGuard(access: Node, subjectText: string): BinaryExpression | undefined { + let scope: Node | undefined = access; + while (scope) { + if (Node.isBinaryExpression(scope) || Node.isIfStatement(scope) || Node.isConditionalExpression(scope)) { + const searchRoot = Node.isIfStatement(scope) + ? scope.getExpression() + : Node.isConditionalExpression(scope) + ? scope.getCondition() + : scope; + for (const candidate of [searchRoot, ...searchRoot.getDescendantsOfKind(SyntaxKind.BinaryExpression)]) { + if (!Node.isBinaryExpression(candidate)) continue; + if (candidate.getOperatorToken().getKind() !== SyntaxKind.InstanceOfKeyword) continue; + if (candidate.getLeft().getText() !== subjectText) continue; + return candidate; + } + } + if ( + Node.isFunctionDeclaration(scope) || + Node.isFunctionExpression(scope) || + Node.isArrowFunction(scope) || + Node.isSourceFile(scope) + ) { + return undefined; + } + scope = scope.getParent(); + } + return undefined; +} + +function resolveTargetModule(sourceFile: SourceFile, context: TransformContext): string { + const existing = sourceFile.getImportDeclarations().find(i => { + const spec = i.getModuleSpecifierValue(); + return spec === '@modelcontextprotocol/client' || spec === '@modelcontextprotocol/server'; + }); + if (existing) return existing.getModuleSpecifierValue(); + if (context.projectType === 'client') return '@modelcontextprotocol/client'; + return '@modelcontextprotocol/server'; +} diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/index.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/index.ts index 7b6b54b28b..eb470ac797 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/index.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/index.ts @@ -1,5 +1,6 @@ import type { Transform } from '../../../types.js'; import { contextTypesTransform } from './contextTypes.js'; +import { errorCodeSemanticsTransform } from './errorCodeSemantics.js'; import { expressMiddlewareTransform } from './expressMiddleware.js'; import { handlerRegistrationTransform } from './handlerRegistration.js'; import { importPathsTransform } from './importPaths.js'; @@ -20,26 +21,33 @@ import { symbolRenamesTransform } from './symbolRenames.js'; // ProtocolError) and rewrites type references (e.g., SchemaInput → // StandardSchemaWithJSON.InferInput). // -// 3. removedApis runs after symbolRenames: handles removed Zod helpers, +// 3. errorCodeSemantics runs after symbolRenames: the rename turns +// ErrorCode.RequestTimeout/ConnectionClosed into SdkErrorCode members; this +// transform then fixes the surrounding semantics (instanceof guards that +// still reference ProtocolError, switches/maps mixing the two enums). It +// also recognizes the pre-rename spelling, so it is safe standalone. +// +// 4. removedApis runs after symbolRenames: handles removed Zod helpers, // IsomorphicHeaders, and StreamableHTTPError. Conceptually different // from renames — these are removals with diagnostic guidance. // -// 4. mcpServerApi SHOULD run before contextTypes: it rewrites .tool() etc. +// 5. mcpServerApi SHOULD run before contextTypes: it rewrites .tool() etc. // to .registerTool() etc. contextTypes handles both old and new names, // but running mcpServerApi first ensures consistent argument structure. // -// 5. handlerRegistration, schemaParamRemoval, and expressMiddleware are +// 6. handlerRegistration, schemaParamRemoval, and expressMiddleware are // independent of each other but all depend on importPaths having run. // -// 6. specSchemaAccess runs after handlerRegistration and schemaParamRemoval: +// 7. specSchemaAccess runs after handlerRegistration and schemaParamRemoval: // those transforms remove spec schema references they handle. specSchemaAccess // then processes remaining standalone usages (safeParse, parse, z.infer, etc.). // -// 7. mockPaths runs last: handles test mocks and dynamic imports, +// 8. mockPaths runs last: handles test mocks and dynamic imports, // independent of the other transforms. export const v1ToV2Transforms: Transform[] = [ importPathsTransform, symbolRenamesTransform, + errorCodeSemanticsTransform, removedApisTransform, mcpServerApiTransform, handlerRegistrationTransform, diff --git a/packages/codemod/test/v1-to-v2/transforms/errorCodeSemantics.test.ts b/packages/codemod/test/v1-to-v2/transforms/errorCodeSemantics.test.ts new file mode 100644 index 0000000000..132c6bf372 --- /dev/null +++ b/packages/codemod/test/v1-to-v2/transforms/errorCodeSemantics.test.ts @@ -0,0 +1,203 @@ +import { describe, it, expect } from 'vitest'; +import { Project } from 'ts-morph'; + +import { errorCodeSemanticsTransform } from '../../../src/migrations/v1-to-v2/transforms/errorCodeSemantics.js'; +import type { TransformContext } from '../../../src/types.js'; +import { DiagnosticLevel } from '../../../src/types.js'; + +const ctx: TransformContext = { projectType: 'client' }; + +function applyTransform(code: string, context: TransformContext = ctx) { + const project = new Project({ useInMemoryFileSystem: true }); + const sourceFile = project.createSourceFile('test.ts', code); + const result = errorCodeSemanticsTransform.apply(sourceFile, context); + return { text: sourceFile.getFullText(), result }; +} + +describe('error-code-semantics transform', () => { + it('rewrites instanceof ProtocolError guard in the same && chain', () => { + const input = [ + `import { ProtocolError, SdkErrorCode } from '@modelcontextprotocol/client';`, + `function handle(e: unknown) {`, + ` if (e instanceof ProtocolError && e.code === SdkErrorCode.RequestTimeout) {`, + ` retry();`, + ` }`, + `}`, + '' + ].join('\n'); + const { text, result } = applyTransform(input); + expect(text).toContain('e instanceof SdkError && e.code === SdkErrorCode.RequestTimeout'); + expect(text).not.toContain('instanceof ProtocolError'); + expect(result.changesCount).toBeGreaterThan(0); + }); + + it('rewrites instanceof guard in an enclosing if statement', () => { + const input = [ + `import { ProtocolError, SdkErrorCode } from '@modelcontextprotocol/client';`, + `function handle(e: unknown) {`, + ` if (e instanceof ProtocolError) {`, + ` if (e.code === SdkErrorCode.ConnectionClosed) {`, + ` reconnect();`, + ` }`, + ` }`, + `}`, + '' + ].join('\n'); + const { text } = applyTransform(input); + expect(text).toContain('e instanceof SdkError'); + expect(text).not.toContain('instanceof ProtocolError'); + }); + + it('rewrites pre-rename McpError guard without touching the enum reference', () => { + const input = [ + `import { McpError, ErrorCode } from '@modelcontextprotocol/sdk/types.js';`, + `function handle(e: unknown) {`, + ` if (e instanceof McpError && e.code === ErrorCode.RequestTimeout) {`, + ` retry();`, + ` }`, + `}`, + '' + ].join('\n'); + const { text } = applyTransform(input); + expect(text).toContain('e instanceof SdkError'); + // The enum reference itself belongs to the symbol-renames transform. + expect(text).toContain('ErrorCode.RequestTimeout'); + }); + + it('adds the SdkError import when rewriting a guard', () => { + const input = [ + `import { ProtocolError, SdkErrorCode } from '@modelcontextprotocol/client';`, + `declare const e: Error;`, + `if (e instanceof ProtocolError && e.code === SdkErrorCode.RequestTimeout) { /* retry */ }`, + '' + ].join('\n'); + const { text } = applyTransform(input); + expect(text).toMatch(/import \{[^}]*SdkError\b[^}]*\} from '@modelcontextprotocol\/client'/); + }); + + it('warns when a moved-member comparison has no instanceof guard', () => { + const input = [ + `import { SdkErrorCode } from '@modelcontextprotocol/client';`, + `declare const e: { code: unknown };`, + `const timedOut = e.code === SdkErrorCode.RequestTimeout;`, + '' + ].join('\n'); + const { text, result } = applyTransform(input); + expect(text).toContain('e.code === SdkErrorCode.RequestTimeout'); + const diag = result.diagnostics.find(d => d.level === DiagnosticLevel.Warning && d.message.includes('instanceof SdkError')); + expect(diag).toBeDefined(); + }); + + it('emits an error for an instanceof guard against an unrecognized class', () => { + const input = [ + `import { SdkErrorCode } from '@modelcontextprotocol/client';`, + `class CustomError extends Error { code = ''; }`, + `declare const e: unknown;`, + `if (e instanceof CustomError && e.code === SdkErrorCode.RequestTimeout) { /* retry */ }`, + '' + ].join('\n'); + const { text, result } = applyTransform(input); + expect(text).toContain('instanceof CustomError'); + const diag = result.diagnostics.find(d => d.level === DiagnosticLevel.Error && d.message.includes('Manual fix')); + expect(diag).toBeDefined(); + }); + + it('leaves an existing instanceof SdkError guard alone', () => { + const input = [ + `import { SdkError, SdkErrorCode } from '@modelcontextprotocol/client';`, + `declare const e: unknown;`, + `if (e instanceof SdkError && e.code === SdkErrorCode.RequestTimeout) { /* retry */ }`, + '' + ].join('\n'); + const { text, result } = applyTransform(input); + expect(result.changesCount).toBe(0); + expect(text).toContain('e instanceof SdkError && e.code === SdkErrorCode.RequestTimeout'); + }); + + it('is idempotent', () => { + const input = [ + `import { ProtocolError, SdkErrorCode } from '@modelcontextprotocol/client';`, + `declare const e: unknown;`, + `if (e instanceof ProtocolError && e.code === SdkErrorCode.ConnectionClosed) { /* reconnect */ }`, + '' + ].join('\n'); + const { text: first } = applyTransform(input); + const { text: second, result } = applyTransform(first); + expect(second).toBe(first); + expect(result.changesCount).toBe(0); + }); + + it('errors on a switch mixing SdkErrorCode and ProtocolErrorCode cases', () => { + const input = [ + `import { ProtocolErrorCode, SdkErrorCode } from '@modelcontextprotocol/client';`, + `declare const e: { code: unknown };`, + `switch (e.code) {`, + ` case SdkErrorCode.RequestTimeout:`, + ` retry();`, + ` break;`, + ` case ProtocolErrorCode.InvalidParams:`, + ` report();`, + ` break;`, + `}`, + '' + ].join('\n'); + const { result } = applyTransform(input); + const diag = result.diagnostics.find(d => d.level === DiagnosticLevel.Error && d.message.includes('Split into')); + expect(diag).toBeDefined(); + }); + + it('warns on a switch over moved members only', () => { + const input = [ + `import { SdkErrorCode } from '@modelcontextprotocol/client';`, + `declare const e: { code: unknown };`, + `switch (e.code) {`, + ` case SdkErrorCode.RequestTimeout:`, + ` retry();`, + ` break;`, + `}`, + '' + ].join('\n'); + const { result } = applyTransform(input); + const diag = result.diagnostics.find(d => d.level === DiagnosticLevel.Warning && d.message.includes('instanceof SdkError')); + expect(diag).toBeDefined(); + }); + + it('warns on maps keyed by a moved member', () => { + const input = [ + `import { SdkErrorCode } from '@modelcontextprotocol/client';`, + `const names = {`, + ` [SdkErrorCode.RequestTimeout]: 'timeout'`, + `};`, + '' + ].join('\n'); + const { result } = applyTransform(input); + const diag = result.diagnostics.find(d => d.level === DiagnosticLevel.Warning && d.message.includes('string enum')); + expect(diag).toBeDefined(); + }); + + it('flags maps mixing moved members with ProtocolErrorCode keys', () => { + const input = [ + `import { ProtocolErrorCode, SdkErrorCode } from '@modelcontextprotocol/client';`, + `const names = {`, + ` [SdkErrorCode.RequestTimeout]: 'timeout',`, + ` [ProtocolErrorCode.InvalidParams]: 'invalid params'`, + `};`, + '' + ].join('\n'); + const { result } = applyTransform(input); + const diag = result.diagnostics.find(d => d.message.includes('split it into separate maps')); + expect(diag).toBeDefined(); + }); + + it('ignores unrelated SdkErrorCode members', () => { + const input = [ + `import { SdkErrorCode } from '@modelcontextprotocol/client';`, + `declare const e: { code: unknown };`, + `const invalid = e.code === SdkErrorCode.InvalidResult;`, + '' + ].join('\n'); + const { result } = applyTransform(input); + expect(result.changesCount).toBe(0); + expect(result.diagnostics).toHaveLength(0); + }); +}); From 6250a78a61e14498362f960129c7eaf72c38dbbc Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Tue, 2 Jun 2026 08:00:06 +0000 Subject: [PATCH 2/5] codemod: preserve file-leading shebangs and banner comments Removing the first import declaration in a file (during import consolidation, the ErrorCode split, or removed-API handling) also removed the leading trivia attached to it, silently deleting shebangs and file-header comments. Capture the trivia before mutating and restore it afterwards. --- .changeset/codemod-preserve-file-headers.md | 6 ++ .../v1-to-v2/transforms/importPaths.ts | 14 ++++- .../v1-to-v2/transforms/removedApis.ts | 9 ++- .../v1-to-v2/transforms/symbolRenames.ts | 14 ++++- packages/codemod/src/utils/importUtils.ts | 24 ++++++++ .../v1-to-v2/transforms/importPaths.test.ts | 56 +++++++++++++++++++ .../v1-to-v2/transforms/symbolRenames.test.ts | 12 ++++ 7 files changed, 132 insertions(+), 3 deletions(-) create mode 100644 .changeset/codemod-preserve-file-headers.md diff --git a/.changeset/codemod-preserve-file-headers.md b/.changeset/codemod-preserve-file-headers.md new file mode 100644 index 0000000000..c9cf6623be --- /dev/null +++ b/.changeset/codemod-preserve-file-headers.md @@ -0,0 +1,6 @@ +--- +'@modelcontextprotocol/codemod': patch +--- + +Preserve file-leading shebangs and banner comments when import rewrites remove the first +import declaration in a file. Previously the attached leading trivia was silently deleted. 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 ade4451f44..f5473ae252 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts @@ -3,7 +3,14 @@ import type { SourceFile } from 'ts-morph'; import type { Diagnostic, Transform, TransformContext, TransformResult } from '../../../types.js'; import { renameAllReferences } from '../../../utils/astUtils.js'; import { actionRequired, info, v2Gap, warning } from '../../../utils/diagnostics.js'; -import { addOrMergeImport, getSdkExports, getSdkImports, isTypeOnlyImport } from '../../../utils/importUtils.js'; +import { + addOrMergeImport, + captureLeadingFileTrivia, + getSdkExports, + getSdkImports, + isTypeOnlyImport, + restoreLeadingFileTrivia +} from '../../../utils/importUtils.js'; import { resolveTypesPackage } from '../../../utils/projectAnalyzer.js'; import { IMPORT_MAP, isAuthImport } from '../mappings/importMap.js'; import { SIMPLE_RENAMES } from '../mappings/symbolMap.js'; @@ -49,6 +56,9 @@ export const importPathsTransform: Transform = { }); const insertIndex = sourceFile.getImportDeclarations().indexOf(sdkImports[0]!); + // Removing the first import would silently delete a file-leading shebang or + // banner comment attached to it — capture it up front and restore at the end. + const leadingTrivia = captureLeadingFileTrivia(sdkImports[0]!); interface PendingImport { names: string[]; @@ -204,6 +214,8 @@ export const importPathsTransform: Transform = { } } + restoreLeadingFileTrivia(sourceFile, leadingTrivia); + return { changesCount, diagnostics, usedPackages }; } }; diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/removedApis.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/removedApis.ts index 82f5c2b1a7..51d9231e64 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/removedApis.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/removedApis.ts @@ -4,7 +4,7 @@ import { Node, SyntaxKind } from 'ts-morph'; import type { Diagnostic, Transform, TransformContext, TransformResult } from '../../../types.js'; import { renameAllReferences } from '../../../utils/astUtils.js'; import { warning } from '../../../utils/diagnostics.js'; -import { addOrMergeImport, isAnyMcpSpecifier } from '../../../utils/importUtils.js'; +import { addOrMergeImport, captureLeadingFileTrivia, isAnyMcpSpecifier, restoreLeadingFileTrivia } from '../../../utils/importUtils.js'; const REMOVED_ZOD_HELPERS: Record = { schemaToJson: @@ -26,10 +26,17 @@ export const removedApisTransform: Transform = { const diagnostics: Diagnostic[] = []; let changesCount = 0; + // Import removals below may delete the file's first import, taking any attached + // shebang/banner comment with it — capture and restore around the mutations. + const firstImport = sourceFile.getImportDeclarations()[0]; + const leadingTrivia = firstImport ? captureLeadingFileTrivia(firstImport) : undefined; + changesCount += handleRemovedZodHelpers(sourceFile, diagnostics); changesCount += handleIsomorphicHeaders(sourceFile, diagnostics); changesCount += handleStreamableHTTPError(sourceFile, diagnostics); + restoreLeadingFileTrivia(sourceFile, leadingTrivia); + return { changesCount, diagnostics }; } }; diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/symbolRenames.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/symbolRenames.ts index 651faf5680..cd988591b6 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/symbolRenames.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/symbolRenames.ts @@ -4,7 +4,13 @@ import { Node } from 'ts-morph'; import type { Diagnostic, Transform, TransformContext, TransformResult } from '../../../types.js'; import { renameAllReferences } from '../../../utils/astUtils.js'; import { info, warning } from '../../../utils/diagnostics.js'; -import { addOrMergeImport, isAnyMcpSpecifier, removeUnusedImport } from '../../../utils/importUtils.js'; +import { + addOrMergeImport, + captureLeadingFileTrivia, + isAnyMcpSpecifier, + removeUnusedImport, + restoreLeadingFileTrivia +} from '../../../utils/importUtils.js'; import { resolveTypesPackage } from '../../../utils/projectAnalyzer.js'; import { ERROR_CODE_SDK_MEMBERS, SIMPLE_RENAMES } from '../mappings/symbolMap.js'; @@ -20,6 +26,10 @@ export const symbolRenamesTransform: Transform = { const imports = sourceFile.getImportDeclarations(); + // The ErrorCode split below may remove the file's first import, taking any + // attached shebang/banner comment with it — capture and restore around it. + const leadingTrivia = imports[0] ? captureLeadingFileTrivia(imports[0]) : undefined; + for (const imp of imports) { if (!isAnyMcpSpecifier(imp.getModuleSpecifierValue())) continue; for (const namedImport of imp.getNamedImports()) { @@ -40,6 +50,8 @@ export const symbolRenamesTransform: Transform = { changesCount += handleRequestHandlerExtra(sourceFile, context, diagnostics); changesCount += handleSchemaInput(sourceFile, context, diagnostics); + restoreLeadingFileTrivia(sourceFile, leadingTrivia); + return { changesCount, diagnostics }; } }; diff --git a/packages/codemod/src/utils/importUtils.ts b/packages/codemod/src/utils/importUtils.ts index 145c95328c..bcae6e5295 100644 --- a/packages/codemod/src/utils/importUtils.ts +++ b/packages/codemod/src/utils/importUtils.ts @@ -139,3 +139,27 @@ export function removeUnusedImport(sourceFile: SourceFile, symbolName: string, o } } } + +/** + * If `imp` is the first statement in the file and carries leading trivia (a shebang + * and/or a file-header banner comment), return that trivia text so it can be restored + * after the import is removed. ts-morph removes a node's leading trivia together with + * the node, which silently deletes file headers when the codemod consolidates imports. + */ +export function captureLeadingFileTrivia(imp: ImportDeclaration): string | undefined { + if (imp.getFullStart() !== 0) return undefined; + const fullText = imp.getFullText(); + const trivia = fullText.slice(0, fullText.length - imp.getText().length); + return /\S/.test(trivia) ? trivia : undefined; +} + +/** + * Re-attach file-leading trivia captured by {@link captureLeadingFileTrivia} when the + * import that carried it was removed. No-op when the trivia is still present (the + * import survived, or another call already restored it). + */ +export function restoreLeadingFileTrivia(sourceFile: SourceFile, trivia: string | undefined): void { + if (!trivia) return; + if (sourceFile.getFullText().startsWith(trivia)) return; + sourceFile.insertText(0, trivia.endsWith('\n') ? trivia : trivia + '\n'); +} 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 f0563f9f69..7bfefec942 100644 --- a/packages/codemod/test/v1-to-v2/transforms/importPaths.test.ts +++ b/packages/codemod/test/v1-to-v2/transforms/importPaths.test.ts @@ -12,6 +12,62 @@ function applyTransform(code: string, context: TransformContext = { projectType: } describe('import-paths transform', () => { + describe('file-leading trivia preservation', () => { + it('preserves a file-header banner comment when consolidating imports', () => { + const input = [ + `/**`, + ` * Acme MCP integration.`, + ` *`, + ` * Connects the Acme CLI to MCP servers over stdio.`, + ` */`, + `import { Client } from '@modelcontextprotocol/sdk/client/index.js';`, + `import { CallToolResult } from '@modelcontextprotocol/sdk/types.js';`, + `console.log(Client, CallToolResult);`, + '' + ].join('\n'); + const result = applyTransform(input); + expect(result).toContain('Acme MCP integration.'); + expect(result.indexOf('Acme MCP integration.')).toBeLessThan(result.indexOf('import')); + expect(result).not.toContain('@modelcontextprotocol/sdk'); + }); + + it('preserves a shebang when the first import is removed', () => { + const input = [ + `#!/usr/bin/env node`, + `import { Client } from '@modelcontextprotocol/sdk/client/index.js';`, + `console.log(Client);`, + '' + ].join('\n'); + const result = applyTransform(input); + expect(result.startsWith('#!/usr/bin/env node')).toBe(true); + }); + + it('preserves shebang and banner together', () => { + const input = [ + `#!/usr/bin/env node`, + `// Entry point for the example MCP client.`, + `import { Client } from '@modelcontextprotocol/sdk/client/index.js';`, + `console.log(Client);`, + '' + ].join('\n'); + const result = applyTransform(input); + expect(result.startsWith('#!/usr/bin/env node')).toBe(true); + expect(result).toContain('// Entry point for the example MCP client.'); + }); + + it('does not duplicate trivia when the first import is only rewritten in place', () => { + const input = [ + `// Aliased import keeps its declaration node.`, + `import { Client as McpClient } from '@modelcontextprotocol/sdk/client/index.js';`, + `console.log(McpClient);`, + '' + ].join('\n'); + const result = applyTransform(input); + const occurrences = result.split('Aliased import keeps its declaration node.').length - 1; + expect(occurrences).toBe(1); + }); + }); + it('rewrites client imports to @modelcontextprotocol/client', () => { const input = `import { Client } from '@modelcontextprotocol/sdk/client/index.js';\n`; const result = applyTransform(input); diff --git a/packages/codemod/test/v1-to-v2/transforms/symbolRenames.test.ts b/packages/codemod/test/v1-to-v2/transforms/symbolRenames.test.ts index d6ef103de6..a4bd175bb2 100644 --- a/packages/codemod/test/v1-to-v2/transforms/symbolRenames.test.ts +++ b/packages/codemod/test/v1-to-v2/transforms/symbolRenames.test.ts @@ -14,6 +14,18 @@ function applyTransform(code: string): string { } describe('symbol-renames transform', () => { + it('preserves a file-header banner when the ErrorCode split removes the first import', () => { + const input = [ + `// Error classification helpers.`, + `import { ErrorCode } from '@modelcontextprotocol/sdk/types.js';`, + `const a = ErrorCode.RequestTimeout;`, + '' + ].join('\n'); + const result = applyTransform(input); + expect(result).toContain('// Error classification helpers.'); + expect(result.indexOf('// Error classification helpers.')).toBeLessThan(result.indexOf('import')); + }); + it('renames McpError to ProtocolError', () => { const input = [`import { McpError } from '@modelcontextprotocol/sdk/types.js';`, `throw new McpError(1, 'error');`, ''].join('\n'); const result = applyTransform(input); From b926f9fd46cde29a05562dcba6a283aeb19919d2 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Tue, 2 Jun 2026 08:00:06 +0000 Subject: [PATCH 3/5] codemod: rewrite StreamableHTTPError to SdkHttpError The v2 replacement for StreamableHTTPError is SdkHttpError, which extends SdkError and exposes the HTTP status as typed .status/.statusText accessors. The codemod previously rewrote references to plain SdkError and told users the status lives in error.data?.status, leaving migrated code one subclass too general and the guidance pointing at an untyped field. --- .changeset/codemod-sdkhttperror-rewrite.md | 7 ++++++ .../v1-to-v2/transforms/importPaths.ts | 2 +- .../v1-to-v2/transforms/removedApis.ts | 14 +++++++----- packages/codemod/test/integration.test.ts | 4 ++-- .../v1-to-v2/transforms/removedApis.test.ts | 22 +++++++++---------- 5 files changed, 29 insertions(+), 20 deletions(-) create mode 100644 .changeset/codemod-sdkhttperror-rewrite.md diff --git a/.changeset/codemod-sdkhttperror-rewrite.md b/.changeset/codemod-sdkhttperror-rewrite.md new file mode 100644 index 0000000000..260937d30a --- /dev/null +++ b/.changeset/codemod-sdkhttperror-rewrite.md @@ -0,0 +1,7 @@ +--- +'@modelcontextprotocol/codemod': patch +--- + +Rewrite `StreamableHTTPError` to `SdkHttpError` (the documented v2 replacement with typed +`.status`/`.statusText`) instead of `SdkError`, and correct the migration guidance the +codemod emits for it. 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 f5473ae252..637a56098b 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts @@ -21,7 +21,7 @@ const REEXPORT_WARNINGS: Record = { 'Re-exported RequestHandlerExtra was renamed to ServerContext/ClientContext in v2. Update this re-export manually.', IsomorphicHeaders: 'Re-exported IsomorphicHeaders was removed in v2 (replaced by standard Headers API). Remove this re-export.', StreamableHTTPError: - 'Re-exported StreamableHTTPError was renamed to SdkError in v2 with different constructor. Update this re-export manually.' + 'Re-exported StreamableHTTPError was replaced by SdkHttpError in v2 with a different constructor. Update this re-export manually.' }; export const importPathsTransform: Transform = { diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/removedApis.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/removedApis.ts index 51d9231e64..59effe9962 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/removedApis.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/removedApis.ts @@ -155,13 +155,14 @@ function handleStreamableHTTPError(sourceFile: SourceFile, diagnostics: Diagnost warning( sourceFile.getFilePath(), node.getStartLineNumber(), - 'new StreamableHTTPError(statusCode, statusText, body?) → new SdkError(code, message, data?). ' + - 'Constructor arguments differ — manual review required. Map HTTP status to SdkErrorCode enum value.' + 'new StreamableHTTPError(statusCode, statusText, body?) → new SdkHttpError(code, message, { status, statusText }). ' + + 'Constructor arguments differ — manual review required. Pick the SdkErrorCode that matches the failure ' + + 'and pass the HTTP status in the data object.' ) ); } - renameAllReferences(sourceFile, localName, 'SdkError'); + renameAllReferences(sourceFile, localName, 'SdkHttpError'); changesCount++; foundImport.remove(); @@ -171,7 +172,7 @@ function handleStreamableHTTPError(sourceFile: SourceFile, diagnostics: Diagnost const targetModule = resolveTargetModule(sourceFile, moduleSpec); const insertIndex = sourceFile.getImportDeclarations().length; - const importsToAdd = hasConstructorCalls ? ['SdkError', 'SdkErrorCode'] : ['SdkError']; + const importsToAdd = hasConstructorCalls ? ['SdkHttpError', 'SdkErrorCode'] : ['SdkHttpError']; addOrMergeImport(sourceFile, targetModule, importsToAdd, false, insertIndex); changesCount++; @@ -179,8 +180,9 @@ function handleStreamableHTTPError(sourceFile: SourceFile, diagnostics: Diagnost warning( sourceFile.getFilePath(), line, - 'StreamableHTTPError replaced with SdkError. Constructor arguments differ — manual review required. ' + - 'HTTP status is now in error.data?.status.' + 'StreamableHTTPError replaced with SdkHttpError (extends SdkError). The HTTP status is exposed as ' + + 'error.status (with error.statusText alongside); error.code is now an SdkErrorCode string, ' + + 'not the HTTP status number.' ) ); diff --git a/packages/codemod/test/integration.test.ts b/packages/codemod/test/integration.test.ts index 7acd34c25e..0c150976da 100644 --- a/packages/codemod/test/integration.test.ts +++ b/packages/codemod/test/integration.test.ts @@ -324,8 +324,8 @@ describe('integration', () => { expect(output).toContain('const h: Headers'); expect(output).not.toContain('IsomorphicHeaders'); - // StreamableHTTPError renamed to SdkError - expect(output).toContain('instanceof SdkError'); + // StreamableHTTPError replaced with SdkHttpError + expect(output).toContain('instanceof SdkHttpError'); expect(output).not.toContain('StreamableHTTPError'); // schemaToJson removed (import gone) diff --git a/packages/codemod/test/v1-to-v2/transforms/removedApis.test.ts b/packages/codemod/test/v1-to-v2/transforms/removedApis.test.ts index 3e003f4182..30a033c8aa 100644 --- a/packages/codemod/test/v1-to-v2/transforms/removedApis.test.ts +++ b/packages/codemod/test/v1-to-v2/transforms/removedApis.test.ts @@ -163,37 +163,37 @@ describe('removed-apis transform', () => { }); }); - describe('StreamableHTTPError → SdkError', () => { - it('renames StreamableHTTPError to SdkError in references', () => { + describe('StreamableHTTPError → SdkHttpError', () => { + it('renames StreamableHTTPError to SdkHttpError in references', () => { const input = [ `import { StreamableHTTPError } from '@modelcontextprotocol/client';`, `if (error instanceof StreamableHTTPError) { throw error; }`, '' ].join('\n'); const { text } = applyTransform(input); - expect(text).toContain('instanceof SdkError'); + expect(text).toContain('instanceof SdkHttpError'); expect(text).not.toContain('StreamableHTTPError'); }); - it('adds SdkError import without SdkErrorCode when no constructor calls', () => { + it('adds SdkHttpError import without SdkErrorCode when no constructor calls', () => { const input = [ `import { StreamableHTTPError } from '@modelcontextprotocol/client';`, `if (error instanceof StreamableHTTPError) {}`, '' ].join('\n'); const { text } = applyTransform(input); - expect(text).toContain('SdkError'); + expect(text).toContain('SdkHttpError'); expect(text).not.toContain('SdkErrorCode'); }); - it('adds SdkError and SdkErrorCode imports when constructor calls exist', () => { + it('adds SdkHttpError and SdkErrorCode imports when constructor calls exist', () => { const input = [ `import { StreamableHTTPError } from '@modelcontextprotocol/client';`, `throw new StreamableHTTPError(404, 'Not Found');`, '' ].join('\n'); const { text } = applyTransform(input); - expect(text).toContain('SdkError'); + expect(text).toContain('SdkHttpError'); expect(text).toContain('SdkErrorCode'); }); @@ -215,7 +215,7 @@ describe('removed-apis transform', () => { '' ].join('\n'); const { result } = applyTransform(input); - const migrationWarning = result.diagnostics.find(d => d.message.includes('error.data?.status')); + const migrationWarning = result.diagnostics.find(d => d.message.includes('error.status')); expect(migrationWarning).toBeDefined(); }); @@ -227,7 +227,7 @@ describe('removed-apis transform', () => { ].join('\n'); const { text } = applyTransform(input); expect(text).not.toContain('import { StreamableHTTPError }'); - expect(text).toMatch(/import.*SdkError/); + expect(text).toMatch(/import.*SdkHttpError/); }); it('is idempotent', () => { @@ -249,9 +249,9 @@ describe('removed-apis transform', () => { '' ].join('\n'); const { text, result } = applyTransform(input); - expect(text).toContain('instanceof SdkError'); + expect(text).toContain('instanceof SdkHttpError'); expect(text).not.toMatch(/\bSHE\b/); - expect(text).toMatch(/import.*SdkError/); + expect(text).toMatch(/import.*SdkHttpError/); const constructorWarning = result.diagnostics.find(d => d.message.includes('Constructor arguments differ')); expect(constructorWarning).toBeDefined(); }); From 7bb7e23598965f218380a5e6dab81df2c49105bc Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Tue, 2 Jun 2026 08:20:06 +0000 Subject: [PATCH 4/5] codemod: add end-to-end test for error-code guard rewrites and header preservation --- packages/codemod/test/integration.test.ts | 57 +++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/packages/codemod/test/integration.test.ts b/packages/codemod/test/integration.test.ts index 0c150976da..84f6c69c95 100644 --- a/packages/codemod/test/integration.test.ts +++ b/packages/codemod/test/integration.test.ts @@ -271,6 +271,63 @@ describe('integration', () => { expect(result.packageJsonChanges!.added).not.toContain('@modelcontextprotocol/phantom-pkg'); }); + it('migrates an error-handling client file end to end, preserving the file header', () => { + const dir = createTempDir(); + const input = [ + `#\!/usr/bin/env node`, + `// Acme MCP client entry point.`, + `// Connects to local MCP servers over stdio and classifies failures.`, + `import { Client } from '@modelcontextprotocol/sdk/client/index.js';`, + `import { McpError, ErrorCode } from '@modelcontextprotocol/sdk/types.js';`, + ``, + `const client = new Client({ name: 'acme', version: '1.0' });`, + ``, + `export async function callWithRetry(fn: () => Promise): Promise {`, + ` try {`, + ` return await fn();`, + ` } catch (e) {`, + ` if (e instanceof McpError && e.code === ErrorCode.RequestTimeout) {`, + ` return await fn();`, + ` }`, + ` if (e instanceof McpError && e.code === ErrorCode.ConnectionClosed) {`, + ` reconnect();`, + ` }`, + ` throw e;`, + ` }`, + `}`, + `` + ].join('\n'); + + writeFileSync(path.join(dir, 'client.ts'), input); + + const result = run(migration, { targetDir: dir }); + + expect(result.filesChanged).toBe(1); + + const output = readFileSync(path.join(dir, 'client.ts'), 'utf8'); + + // File-leading trivia survives the full pipeline (multiple transforms + // capture/restore it in sequence). + expect(output.startsWith('#\!/usr/bin/env node')).toBe(true); + expect(output.split('Acme MCP client entry point.').length - 1).toBe(1); + + // The symbols transform renames McpError -> ProtocolError and splits the + // moved enum members to SdkErrorCode; error-code-semantics must then + // retarget the instanceof guards so the catch blocks still match at + // runtime. This asserts the composed, in-order pipeline result. + expect(output).toContain('e instanceof SdkError && e.code === SdkErrorCode.RequestTimeout'); + expect(output).toContain('e instanceof SdkError && e.code === SdkErrorCode.ConnectionClosed'); + expect(output).not.toContain('McpError'); + expect(output).not.toContain('instanceof ProtocolError'); + + // SdkError import added alongside the rewritten client import. (The + // renamed-but-now-unused ProtocolError specifier is tolerated here: + // the symbols transform renames the import before error-code-semantics + // retargets the usages. Linters flag it; nothing breaks at runtime.) + expect(output).toMatch(/import \{[^}]*\bSdkError\b[^}]*\} from ["']@modelcontextprotocol\/client["']/); + expect(output).not.toContain('@modelcontextprotocol/sdk'); + }); + it('respects transform filter option', () => { const dir = createTempDir(); const input = [ From 0298cfc38709666aecf883d2942d048814015633 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Tue, 2 Jun 2026 08:25:17 +0000 Subject: [PATCH 5/5] codemod: drop v1 error-class import when guard rewrite removes its last usage Retargeting an instanceof guard to SdkError can leave the renamed ProtocolError (or McpError) import specifier unused, which consumer linters flag in migrated output. Remove the specifier when no other references remain, keeping the import when the class is still used. removeUnusedImport now preserves file-leading shebangs and banner comments when it removes an emptied first-statement import, and reports whether it removed anything. --- .../v1-to-v2/transforms/errorCodeSemantics.ts | 13 ++++- packages/codemod/src/utils/importUtils.ts | 9 ++- packages/codemod/test/integration.test.ts | 9 +-- .../transforms/errorCodeSemantics.test.ts | 55 +++++++++++++++++++ 4 files changed, 79 insertions(+), 7 deletions(-) diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/errorCodeSemantics.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/errorCodeSemantics.ts index cb69b37f02..6c23636054 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/errorCodeSemantics.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/errorCodeSemantics.ts @@ -3,7 +3,7 @@ import { Node, SyntaxKind } from 'ts-morph'; import type { Diagnostic, Transform, TransformContext, TransformResult } from '../../../types.js'; import { error, warning } from '../../../utils/diagnostics.js'; -import { addOrMergeImport, isImportedFromMcp } from '../../../utils/importUtils.js'; +import { addOrMergeImport, isImportedFromMcp, removeUnusedImport } from '../../../utils/importUtils.js'; import { ERROR_CODE_SDK_MEMBERS } from '../mappings/symbolMap.js'; /** @@ -156,7 +156,9 @@ export const errorCodeSemanticsTransform: Transform = { } } + const rewrittenGuardClasses = new Set(); for (const guard of guardsToRewrite) { + rewrittenGuardClasses.add(guard.getRight().getText()); guard.getRight().replaceWithText('SdkError'); needsSdkErrorImport = true; changesCount++; @@ -168,6 +170,15 @@ export const errorCodeSemanticsTransform: Transform = { changesCount++; } + // Retargeting a guard may have removed the last usage of the v1 error + // class — drop the now-unused import specifier so migrated output does + // not carry unused-import lint debt. + for (const className of rewrittenGuardClasses) { + if (removeUnusedImport(sourceFile, className, true)) { + changesCount++; + } + } + return { changesCount, diagnostics }; } }; diff --git a/packages/codemod/src/utils/importUtils.ts b/packages/codemod/src/utils/importUtils.ts index bcae6e5295..e55efcb40d 100644 --- a/packages/codemod/src/utils/importUtils.ts +++ b/packages/codemod/src/utils/importUtils.ts @@ -113,7 +113,7 @@ export function resolveOriginalImportName(sourceFile: SourceFile, localName: str return undefined; } -export function removeUnusedImport(sourceFile: SourceFile, symbolName: string, onlyMcpImports?: boolean): void { +export function removeUnusedImport(sourceFile: SourceFile, symbolName: string, onlyMcpImports?: boolean): boolean { let referenceCount = 0; sourceFile.forEachDescendant(node => { if (Node.isIdentifier(node) && node.getText() === symbolName) { @@ -131,13 +131,18 @@ export function removeUnusedImport(sourceFile: SourceFile, symbolName: string, o if ((namedImport.getAliasNode()?.getText() ?? namedImport.getName()) === symbolName) { namedImport.remove(); if (imp.getNamedImports().length === 0 && !imp.getDefaultImport() && !imp.getNamespaceImport()) { + // Removing a first-statement import takes its leading trivia + // (shebang/file banner) with it — capture and restore. + const trivia = captureLeadingFileTrivia(imp); imp.remove(); + restoreLeadingFileTrivia(sourceFile, trivia); } - return; + return true; } } } } + return false; } /** diff --git a/packages/codemod/test/integration.test.ts b/packages/codemod/test/integration.test.ts index 84f6c69c95..e26b59fa52 100644 --- a/packages/codemod/test/integration.test.ts +++ b/packages/codemod/test/integration.test.ts @@ -320,11 +320,12 @@ describe('integration', () => { expect(output).not.toContain('McpError'); expect(output).not.toContain('instanceof ProtocolError'); - // SdkError import added alongside the rewritten client import. (The - // renamed-but-now-unused ProtocolError specifier is tolerated here: - // the symbols transform renames the import before error-code-semantics - // retargets the usages. Linters flag it; nothing breaks at runtime.) + // SdkError import added alongside the rewritten client import, and the + // renamed-but-now-unused ProtocolError specifier is cleaned up once its + // last usage has been retargeted — migrated output carries no + // unused-import lint debt. expect(output).toMatch(/import \{[^}]*\bSdkError\b[^}]*\} from ["']@modelcontextprotocol\/client["']/); + expect(output).not.toContain('ProtocolError'); expect(output).not.toContain('@modelcontextprotocol/sdk'); }); diff --git a/packages/codemod/test/v1-to-v2/transforms/errorCodeSemantics.test.ts b/packages/codemod/test/v1-to-v2/transforms/errorCodeSemantics.test.ts index 132c6bf372..f7c387e6d0 100644 --- a/packages/codemod/test/v1-to-v2/transforms/errorCodeSemantics.test.ts +++ b/packages/codemod/test/v1-to-v2/transforms/errorCodeSemantics.test.ts @@ -189,6 +189,61 @@ describe('error-code-semantics transform', () => { expect(diag).toBeDefined(); }); + it('removes the import specifier when the rewritten guard was its last usage', () => { + const input = [ + `import { ProtocolError, SdkErrorCode } from '@modelcontextprotocol/client';`, + `function handle(e: unknown) {`, + ` if (e instanceof ProtocolError && e.code === SdkErrorCode.RequestTimeout) {`, + ` retry();`, + ` }`, + `}`, + '' + ].join('\n'); + const { text } = applyTransform(input); + expect(text).not.toContain('ProtocolError'); + expect(text).toMatch(/import \{[^}]*\bSdkErrorCode\b[^}]*\} from '@modelcontextprotocol\/client'/); + expect(text).toMatch(/import \{[^}]*\bSdkError\b[^}]*\} from '@modelcontextprotocol\/client'/); + }); + + it('keeps the import when other usages of the error class remain', () => { + const input = [ + `import { ProtocolError, ProtocolErrorCode, SdkErrorCode } from '@modelcontextprotocol/client';`, + `function handle(e: unknown) {`, + ` if (e instanceof ProtocolError && e.code === SdkErrorCode.RequestTimeout) {`, + ` retry();`, + ` }`, + ` throw new ProtocolError(ProtocolErrorCode.InternalError, 'failed');`, + `}`, + '' + ].join('\n'); + const { text } = applyTransform(input); + expect(text).toContain('e instanceof SdkError'); + expect(text).toMatch(/import \{[^}]*\bProtocolError\b[^}]*\} from '@modelcontextprotocol\/client'/); + expect(text).toContain(`new ProtocolError(ProtocolErrorCode.InternalError, 'failed')`); + }); + + it('preserves the file header when removing an emptied first-statement import', () => { + const input = [ + `#!/usr/bin/env node`, + `// Acme retry helper.`, + `import { McpError } from '@modelcontextprotocol/sdk/types.js';`, + `import { ErrorCode } from '@modelcontextprotocol/sdk/types.js';`, + `function handle(e: unknown) {`, + ` if (e instanceof McpError && e.code === ErrorCode.RequestTimeout) {`, + ` retry();`, + ` }`, + `}`, + '' + ].join('\n'); + const { text } = applyTransform(input); + expect(text.startsWith('#!/usr/bin/env node')).toBe(true); + expect(text).toContain('// Acme retry helper.'); + expect(text).not.toContain('McpError'); + expect(text).toContain('e instanceof SdkError'); + // The enum reference itself is the symbols transform's job and stays. + expect(text).toContain('ErrorCode.RequestTimeout'); + }); + it('ignores unrelated SdkErrorCode members', () => { const input = [ `import { SdkErrorCode } from '@modelcontextprotocol/client';`,