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/.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/.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/errorCodeSemantics.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/errorCodeSemantics.ts new file mode 100644 index 0000000000..6c23636054 --- /dev/null +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/errorCodeSemantics.ts @@ -0,0 +1,254 @@ +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, removeUnusedImport } 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.`) + ) + ); + } + } + } + + const rewrittenGuardClasses = new Set(); + for (const guard of guardsToRewrite) { + rewrittenGuardClasses.add(guard.getRight().getText()); + 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++; + } + + // 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 }; + } +}; + +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/importPaths.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/importPaths.ts index ade4451f44..637a56098b 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'; @@ -14,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 = { @@ -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/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/src/migrations/v1-to-v2/transforms/removedApis.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/removedApis.ts index 82f5c2b1a7..59effe9962 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 }; } }; @@ -148,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(); @@ -164,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++; @@ -172,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/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..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,11 +131,40 @@ 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; +} + +/** + * 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/integration.test.ts b/packages/codemod/test/integration.test.ts index 7acd34c25e..e26b59fa52 100644 --- a/packages/codemod/test/integration.test.ts +++ b/packages/codemod/test/integration.test.ts @@ -271,6 +271,64 @@ 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, 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'); + }); + it('respects transform filter option', () => { const dir = createTempDir(); const input = [ @@ -324,8 +382,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/errorCodeSemantics.test.ts b/packages/codemod/test/v1-to-v2/transforms/errorCodeSemantics.test.ts new file mode 100644 index 0000000000..f7c387e6d0 --- /dev/null +++ b/packages/codemod/test/v1-to-v2/transforms/errorCodeSemantics.test.ts @@ -0,0 +1,258 @@ +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('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';`, + `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); + }); +}); 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/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(); }); 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);