From 2feb37adcb8f1acb5e768f6925f442b2b1f32cd3 Mon Sep 17 00:00:00 2001 From: Konstantin Konstantinov Date: Tue, 30 Jun 2026 22:18:55 +0300 Subject: [PATCH 1/7] codemod iterations 5 --- packages/codemod/batch-test/repos.json | 10 +- packages/codemod/src/bin/batchTest.ts | 52 +- .../v1-to-v2/transforms/commonjsInterop.ts | 453 ++++++++++++++++++ .../migrations/v1-to-v2/transforms/index.ts | 15 +- packages/codemod/src/types.ts | 1 + packages/codemod/src/utils/projectAnalyzer.ts | 73 ++- packages/codemod/test/batchTest.test.ts | 29 +- packages/codemod/test/integration.test.ts | 33 +- packages/codemod/test/projectAnalyzer.test.ts | 51 ++ .../transforms/commonjsInterop.test.ts | 312 ++++++++++++ 10 files changed, 995 insertions(+), 34 deletions(-) create mode 100644 packages/codemod/src/migrations/v1-to-v2/transforms/commonjsInterop.ts create mode 100644 packages/codemod/test/v1-to-v2/transforms/commonjsInterop.test.ts diff --git a/packages/codemod/batch-test/repos.json b/packages/codemod/batch-test/repos.json index 28dbc4348d..960a6ec16d 100644 --- a/packages/codemod/batch-test/repos.json +++ b/packages/codemod/batch-test/repos.json @@ -1,16 +1,16 @@ [ { - "repo": "firebase/firebase-tools", + "repo": "KKonstantinov/firebase-tools-fork", "ref": "main", "packages": [ { "dir": ".", - "sourceDir": "src/mcp", + "sourceDir": "src", "checks": { - "typecheck": "npx tsc -p tsconfig.compile.json", + "typecheck": "npm run test:compile", "build": null, - "test": null, - "lint": null + "test": "npx mocha 'src/mcp/onemcp/onemcp_server.spec.ts' 'src/mcp/tool.spec.ts' 'src/mcp/prompt.spec.ts'", + "lint": "ESLINT_USE_FLAT_CONFIG=false npx eslint --config .eslintrc.js --ext .ts,.js src/mcp" } } ] diff --git a/packages/codemod/src/bin/batchTest.ts b/packages/codemod/src/bin/batchTest.ts index 8c71bf7164..93206f1d2f 100644 --- a/packages/codemod/src/bin/batchTest.ts +++ b/packages/codemod/src/bin/batchTest.ts @@ -157,19 +157,28 @@ function detectPm(repoRoot: string): string { return 'npm'; } -function installCommand(pm: string): string { +export function installCommand(pm: string, opts: { hasOwnPnpmWorkspace: boolean; packageDirs: string[] }): string { if (pm !== 'pnpm') return `${pm} install --ignore-scripts`; - // pnpm walks up to find a workspace; clones live inside this SDK's pnpm workspace, so a plain - // `pnpm install` targets the OUTER workspace and never populates the clone's node_modules — every - // downstream check (tsc base config, tsup, vitest) then fails identically at baseline and post, - // masking real codemod signal. - // --ignore-workspace: treat the clone as a standalone project (not part of the SDK workspace). - // --no-frozen-lockfile: the codemod rewrites package.json to swap v1 → v2 deps, so the lockfile - // must be allowed to change. CI=true (set in shell()) otherwise defaults - // pnpm to a frozen lockfile and the post-codemod reinstall silently skips - // the new v2 deps, leaving the clone on v1. - // npm/yarn/bun key off a `workspaces` field in package.json (absent at this repo root), so they - // need no equivalent flags. + // --no-frozen-lockfile: the codemod rewrites package.json to swap v1 → v2 deps, so the lockfile must + // be allowed to change. CI=true (set in shell()) otherwise defaults pnpm to a frozen lockfile and the + // post-codemod reinstall silently skips the new v2 deps, leaving the clone on v1. + if (opts.hasOwnPnpmWorkspace) { + // The clone is its OWN pnpm workspace — pnpm-workspace.yaml defines catalog:/workspace: deps + // (e.g. mastra). `--ignore-workspace` would discard that file and pnpm would fail to resolve them + // (ERR_PNPM_CATALOG_ENTRY_NOT_FOUND_FOR_SPEC; unresolved workspace: links → repo skipped). We don't + // need it: the SDK workspace excludes the clones (`!packages/codemod/batch-test/**`) and pnpm uses + // the clone's own pnpm-workspace.yaml as the nearest root. Scope the install to the target packages + // and their dependencies (`...`) so a monorepo target installs only what the checks need (e.g. + // 2 of mastra's 161 projects) instead of the whole tree. + const filters = opts.packageDirs + .filter(dir => dir !== '.') + .map(dir => `--filter ${JSON.stringify(`./${dir}...`)}`) + .join(' '); + return `pnpm install --ignore-scripts --no-frozen-lockfile${filters ? ` ${filters}` : ''}`; + } + // Single-package clone with no workspace of its own: pnpm would walk up to the (clone-excluding) SDK + // workspace and never populate the clone's node_modules. `--ignore-workspace` treats it as standalone. + // npm/yarn/bun key off a `workspaces` field in package.json (absent at this repo root). return 'pnpm install --ignore-scripts --ignore-workspace --no-frozen-lockfile'; } @@ -643,18 +652,23 @@ function main(): void { const pm = detectPm(clonePath); console.log(` Package manager: ${pm}`); - // Step 3: Install + // Process packages + const packages: PackageEntry[] = entry.packages ?? [{ dir: '.', sourceDir: 'src' }]; + const repoPkgResults: PackageReport[] = []; + + // Step 3: Install. A clone that is its own pnpm workspace (catalog:/workspace: deps) must keep its + // pnpm-workspace.yaml — see installCommand. Computed once and reused for the post-codemod reinstall. + const installCmd = installCommand(pm, { + hasOwnPnpmWorkspace: existsSync(path.join(clonePath, 'pnpm-workspace.yaml')), + packageDirs: packages.map(p => p.dir) + }); console.log(' Installing dependencies...'); - const installResult = shell(installCommand(pm), clonePath); + const installResult = shell(installCmd, clonePath); if (installResult.exitCode !== 0) { console.log(` ERROR: install failed, skipping\n ${installResult.stderr.split('\n')[0]}`); continue; } - // Process packages - const packages: PackageEntry[] = entry.packages ?? [{ dir: '.', sourceDir: 'src' }]; - const repoPkgResults: PackageReport[] = []; - for (const pkg of packages) { const sourceDir = pkg.sourceDir ?? 'src'; const fullPkgDir = path.join(clonePath, pkg.dir); @@ -697,7 +711,7 @@ function main(): void { if (rewrites > 0) console.log(` Pinned ${rewrites} deps to resolved published versions`); } console.log(' Re-installing dependencies...'); - shell(installCommand(pm), clonePath); + shell(installCmd, clonePath); // Step 7: Post-codemod checks console.log(' Running post-codemod checks...'); diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/commonjsInterop.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/commonjsInterop.ts new file mode 100644 index 0000000000..ace67f50bf --- /dev/null +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/commonjsInterop.ts @@ -0,0 +1,453 @@ +import type { Block, ExportDeclaration, Identifier, ImportDeclaration, ImportSpecifier, SourceFile } from 'ts-morph'; +import { Node } from 'ts-morph'; + +import type { Diagnostic, Transform, TransformContext, TransformResult } from '../../../types'; +import { isKeyPositionIdentifier } from '../../../utils/astUtils'; +import { actionRequired, warning } from '../../../utils/diagnostics'; + +const V2_PACKAGE_ROOTS = new Set([ + '@modelcontextprotocol/client', + '@modelcontextprotocol/server', + '@modelcontextprotocol/core', + '@modelcontextprotocol/server-legacy', + '@modelcontextprotocol/node', + '@modelcontextprotocol/express' +]); + +/** A v2 package root or any `/subpath` of one (e.g. `@modelcontextprotocol/server/stdio`). */ +export function isV2Specifier(specifier: string): boolean { + if (V2_PACKAGE_ROOTS.has(specifier)) return true; + const secondSlash = specifier.indexOf('/', specifier.indexOf('/') + 1); + return secondSlash !== -1 && V2_PACKAGE_ROOTS.has(specifier.slice(0, secondSlash)); +} + +export interface V2ValueImport { + decl: ImportDeclaration; + specifier: string; + named: ImportSpecifier[]; // value (non-type-only) named specifiers + namespace?: Identifier; // `import * as ns` + defaultImport?: Identifier; // `import d from` +} + +/** + * v2 imports with at least one VALUE-referenced binding (these break under CommonJS). + * + * The transform keys off value USAGE, not import syntax. A binding declared with a plain `import { X }` + * but used only in type position (`: X` / `as X`) has zero value references, so TypeScript erases it at + * emit time — the static import never produces a runtime `require`, so it is harmless and must be left + * intact. Including such a binding here would wrongly remove its static import and orphan the surviving + * type annotations (TS2304). So a binding is collected only when `findValueRefs` finds ≥1 value reference; + * a whole `import type { … }` (and any inline `type X` specifier) is skipped as before. (`findValueRefs` + * is a hoisted function declaration below, so it is safe to call here.) + */ +export function collectV2ValueImports(sourceFile: SourceFile): V2ValueImport[] { + const out: V2ValueImport[] = []; + for (const decl of sourceFile.getImportDeclarations()) { + const specifier = decl.getModuleSpecifierValue(); + if (!isV2Specifier(specifier)) continue; + if (decl.isTypeOnly()) continue; // whole `import type { … }` — erased, harmless + const named = decl + .getNamedImports() + .filter(n => !n.isTypeOnly() && findValueRefs(sourceFile, n.getAliasNode()?.getText() ?? n.getName()).length > 0); + const namespaceBinding = decl.getNamespaceImport(); + const namespace = + namespaceBinding && findValueRefs(sourceFile, namespaceBinding.getText()).length > 0 ? namespaceBinding : undefined; + const defaultBinding = decl.getDefaultImport(); + const defaultImport = defaultBinding && findValueRefs(sourceFile, defaultBinding.getText()).length > 0 ? defaultBinding : undefined; + if (named.length === 0 && !namespace && !defaultImport) continue; + out.push({ decl, specifier, named, namespace, defaultImport }); + } + return out; +} + +/** v2 re-exports that carry a runtime VALUE (named non-type exports, or a star re-export). These break + * under CommonJS and can't be made dynamic, so they are diagnosed (never converted). */ +export function collectV2ValueReExports(sourceFile: SourceFile): ExportDeclaration[] { + return sourceFile.getExportDeclarations().filter(exp => { + const specifier = exp.getModuleSpecifierValue(); + if (!specifier || !isV2Specifier(specifier)) return false; + if (exp.isTypeOnly()) return false; + const named = exp.getNamedExports(); + if (named.length === 0) return true; // `export * from ""` + return named.some(n => !n.isTypeOnly()); + }); +} + +/** Local binding names a v2 value import introduces (aliases preferred). */ +export function localBindings(imp: V2ValueImport): string[] { + const names: string[] = []; + if (imp.namespace) names.push(imp.namespace.getText()); + if (imp.defaultImport) names.push(imp.defaultImport.getText()); + for (const n of imp.named) names.push(n.getAliasNode()?.getText() ?? n.getName()); + return names; +} + +/** + * Conservative type-position test: returns true ONLY when the identifier is clearly part of a type + * (a type reference / type query target). Anything uncertain is treated as a VALUE usage, which biases + * toward diagnosing rather than wrongly converting a sync usage. + */ +export function isTypePositionReference(node: Node): boolean { + let current: Node | undefined = node; + // Walk up through dotted type names (`ns.Foo` in type position is a QualifiedName). + while (current) { + const parent: Node | undefined = current.getParent(); + if (!parent) return false; + if (Node.isTypeReference(parent) || Node.isTypeQuery(parent) || Node.isImportTypeNode(parent)) { + return true; + } + if (Node.isQualifiedName(parent)) { + current = parent; + continue; + } + return false; + } + return false; +} + +/** In-file value references to `localName` (excludes the import binding itself, property keys, and type positions). */ +export function findValueRefs(sourceFile: SourceFile, localName: string): Node[] { + const refs: Node[] = []; + sourceFile.forEachDescendant(node => { + if (!Node.isIdentifier(node) || node.getText() !== localName) return; + const parent = node.getParent(); + if (!parent) return; + if (Node.isImportSpecifier(parent) || Node.isImportClause(parent) || Node.isNamespaceImport(parent)) return; + if (isKeyPositionIdentifier(node)) return; + if (isTypePositionReference(node)) return; + refs.push(node); + }); + return refs; +} + +/** + * True iff `localName` appears in at least one TYPE position (a `: localName` / `as localName` / `` + * reference), excluding the import binding itself and property keys. A binding referenced as BOTH a value and + * a type cannot be converted: removing its static import (to load the value dynamically) would orphan the + * surviving type usage (TS2304). Such a binding is diagnosed rather than converted — see `analyzeConvertibility`. + */ +export function hasTypeReference(sourceFile: SourceFile, localName: string): boolean { + let found = false; + sourceFile.forEachDescendant(node => { + if (found) return; + if (!Node.isIdentifier(node) || node.getText() !== localName) return; + const parent = node.getParent(); + if (!parent) return; + if (Node.isImportSpecifier(parent) || Node.isImportClause(parent) || Node.isNamespaceImport(parent)) return; + if (isKeyPositionIdentifier(node)) return; + if (isTypePositionReference(node)) found = true; + }); + return found; +} + +/** + * True iff the nearest enclosing function-like is an async function/method/arrow WITH A BLOCK BODY + * (await-capable AND able to host a `const { … } = await …;` statement). An expression-bodied async + * arrow (`async () => expr`) returns false → the file is diagnosed rather than mis-converted into code + * that references an unbound symbol. + */ +export function isAwaitSafe(node: Node): boolean { + const fn = node.getFirstAncestor( + a => + Node.isFunctionDeclaration(a) || + Node.isFunctionExpression(a) || + Node.isArrowFunction(a) || + Node.isMethodDeclaration(a) || + Node.isConstructorDeclaration(a) || + Node.isGetAccessorDeclaration(a) || + Node.isSetAccessorDeclaration(a) + ); + if (!fn) return false; // module top-level + if (Node.isConstructorDeclaration(fn) || Node.isGetAccessorDeclaration(fn) || Node.isSetAccessorDeclaration(fn)) { + return false; // never async-capable + } + if (!fn.isAsync()) return false; + const body = fn.getBody(); + if (!body || !Node.isBlock(body)) return false; // expression-bodied async arrow -> not hostable -> diagnose + // The body is async/block-hostable, but the ref must also sit INSIDE it: parameter defaults and other + // signature positions evaluate in parameter scope (outside the body block) and can't see a body-level + // `const { … } = await …;`, so a ref there is not await-safe -> diagnose rather than emit unbound code. + return node.getStart() >= body.getStart() && node.getEnd() <= body.getEnd(); +} + +export interface Convertibility { + convertible: boolean; + // A blocker's root cause selects its diagnostic message: + // - `isDefaultImport`: a default import (no v2 default export, and no dynamic-destructure form). + // - `isValueAndType`: a named binding referenced as both a value and a type (converting would orphan the type usage). + // - neither: a value used in a synchronous context that cannot await a dynamic import. + blockers: Array<{ symbol: string; line: number; decl: ImportDeclaration; isDefaultImport?: boolean; isValueAndType?: boolean }>; +} + +export function analyzeConvertibility(sourceFile: SourceFile, v2ValueImports: V2ValueImport[]): Convertibility { + const blockers: Convertibility['blockers'] = []; + for (const imp of v2ValueImports) { + // Default imports have no dynamic-destructure form (and v2 ships no default exports) -> always diagnose. + if (imp.defaultImport) { + blockers.push({ + symbol: imp.defaultImport.getText(), + line: imp.decl.getStartLineNumber(), + decl: imp.decl, + isDefaultImport: true + }); + continue; + } + // Sync-context blockers first: when a binding is BOTH used in a sync context and used as a type, + // both reasons target the same import declaration and the diagnostic dedups per declaration. The + // sync usage is the more fundamental blocker (it cannot await even after migrating the type usage), + // so it is recorded first and wins. + for (const local of localBindings(imp)) { + for (const ref of findValueRefs(sourceFile, local)) { + if (!isAwaitSafe(ref)) { + blockers.push({ symbol: local, line: ref.getStartLineNumber(), decl: imp.decl }); + } + } + } + // A named binding referenced as both a value and a type cannot be converted: removing its static + // import to load the value dynamically would orphan the surviving `: name` / `as name` type usage + // (TS2304). Diagnose rather than convert (the conservative, type-safe choice — we do not auto-split + // the value and type usages into a retained `import type` in this pass). + for (const n of imp.named) { + const local = n.getAliasNode()?.getText() ?? n.getName(); + if (hasTypeReference(sourceFile, local)) { + blockers.push({ symbol: local, line: imp.decl.getStartLineNumber(), decl: imp.decl, isValueAndType: true }); + } + } + } + return { convertible: blockers.length === 0, blockers }; +} + +const HELPER_ID = '_mcpDynImport'; +const HELPER_DECL = + '// eslint-disable-next-line no-new-func, @typescript-eslint/no-implied-eval\n' + + `const ${HELPER_ID} = new Function("s", "return import(s)") as (s: string) => Promise;`; + +/** Camel-cased tail of a v2 specifier (e.g. `@modelcontextprotocol/server/stdio` -> `serverStdio`). */ +function packageCamel(specifier: string): string { + return specifier + .replace(/^@modelcontextprotocol\//, '') + .split(/[/-]/) + .filter(Boolean) + .map((part, i) => (i === 0 ? part : part.charAt(0).toUpperCase() + part.slice(1))) + .join(''); +} + +/** Suffix `base` with `_2`, `_3`, … until it is free in `taken`, then reserve and return it. */ +function reserveUniqueId(base: string, taken: Set): string { + let id = base; + let n = 2; + while (taken.has(id)) id = `${base}_${n++}`; + taken.add(id); + return id; +} + +/** Stable, collision-free identifier for a package's module promise (e.g. `/server/stdio` -> `_mcpServerStdio`). */ +export function packagePromiseId(specifier: string, taken: Set): string { + const camel = packageCamel(specifier); + return reserveUniqueId(`_mcp${camel.charAt(0).toUpperCase()}${camel.slice(1)}`, taken); +} + +/** + * Stable, collision-free identifier for a package's type-only namespace import + * (e.g. `/server/stdio` -> `_McpServerStdioTypes`), parallel to {@link packagePromiseId}. + * + * The `import type * as from ""` declaration this names keeps `` present as a real import + * DECLARATION — the runner derives each manifest's v2 deps from the post-transform import declarations, so the + * converted package stays in the migrated package.json — while being fully erased at runtime (a type-only import + * never emits a CommonJS `require`, so it does not re-break the ESM-only load). + */ +export function packageTypeId(specifier: string, taken: Set): string { + const camel = packageCamel(specifier); + return reserveUniqueId(`_Mcp${camel.charAt(0).toUpperCase()}${camel.slice(1)}Types`, taken); +} + +/** Named bindings a v2 value import contributes, as `{ local, imported }`. */ +function namedBindings(imp: V2ValueImport): Array<{ local: string; imported: string }> { + return imp.named.map(n => ({ local: n.getAliasNode()?.getText() ?? n.getName(), imported: n.getName() })); +} + +/** + * The nearest enclosing async-hostable function BLOCK body for a value ref, or undefined. Only + * function/method/arrow/function-expression bodies qualify (where a `const { … } = await …;` can live); + * constructors and accessors are intentionally excluded — files using a value there are diagnosed as + * non-convertible upstream, so they never reach this rewrite. + */ +function enclosingFnBlock(ref: Node): Block | undefined { + const fn = ref.getFirstAncestor( + a => Node.isFunctionDeclaration(a) || Node.isFunctionExpression(a) || Node.isArrowFunction(a) || Node.isMethodDeclaration(a) + ); + const body = fn?.getBody(); + return body && Node.isBlock(body) ? body : undefined; +} + +export function convertFile(sourceFile: SourceFile, v2ValueImports: V2ValueImport[]): number { + let changes = 0; + + // Reserve identifiers already present so generated ids never collide. + const taken = new Set(); + sourceFile.forEachDescendant(node => { + if (Node.isIdentifier(node)) taken.add(node.getText()); + }); + taken.add(HELPER_ID); + + // One promise id + one type-binding id per distinct specifier. Both are reserved against `taken` so + // they never collide with existing identifiers nor with each other. + const promiseIdBySpecifier = new Map(); + const typeIdBySpecifier = new Map(); + for (const imp of v2ValueImports) { + if (promiseIdBySpecifier.has(imp.specifier)) continue; + promiseIdBySpecifier.set(imp.specifier, packagePromiseId(imp.specifier, taken)); + typeIdBySpecifier.set(imp.specifier, packageTypeId(imp.specifier, taken)); + } + + // Insert destructures into each async function that uses a package's bindings. + for (const imp of v2ValueImports) { + const promiseId = promiseIdBySpecifier.get(imp.specifier)!; + // Map: async-function body Block -> set of destructure entries to inject. + const perFn = new Map>(); + + if (imp.namespace) { + const ns = imp.namespace.getText(); + // Whole-namespace binding: `const ns = await _pkg;` + for (const ref of findValueRefs(sourceFile, ns)) { + const body = enclosingFnBlock(ref); + if (!body) continue; + if (!perFn.has(body)) perFn.set(body, new Set()); + perFn.get(body)!.add(`__NS__${ns}`); + } + } + for (const { local, imported } of namedBindings(imp)) { + const entry = local === imported ? local : `${imported}: ${local}`; + for (const ref of findValueRefs(sourceFile, local)) { + const body = enclosingFnBlock(ref); + if (!body) continue; + if (!perFn.has(body)) perFn.set(body, new Set()); + perFn.get(body)!.add(entry); + } + } + + for (const [body, entries] of perFn) { + const nsEntry = [...entries].find(e => e.startsWith('__NS__')); + const namedEntries = [...entries].filter(e => !e.startsWith('__NS__')); + const statements: string[] = []; + if (namedEntries.length > 0) { + statements.push(`const { ${namedEntries.toSorted().join(', ')} } = await ${promiseId};`); + } + if (nsEntry) { + statements.push(`const ${nsEntry.slice('__NS__'.length)} = await ${promiseId};`); + } + body.insertStatements(0, statements); + changes += statements.length; + } + } + + // A type-only namespace import per package keeps the specifier present as a real import DECLARATION + // (the runner derives each manifest's v2 deps from getImportDeclarations(), so the converted package stays + // in the migrated package.json), while erasing at runtime — `import type` never emits a CommonJS `require`. + // The promise's `typeof` then references this binding instead of an inline `typeof import("…")`, which the + // runner's dependency detection does not recognize. + for (const [spec, typeId] of typeIdBySpecifier) { + sourceFile.addImportDeclaration({ isTypeOnly: true, namespaceImport: typeId, moduleSpecifier: spec }); + changes++; + } + + // Insert the helper + per-package promises after the last import declaration, then remove static value imports. + const imports = sourceFile.getImportDeclarations(); + const insertAt = imports.length > 0 ? imports.at(-1)!.getChildIndex() + 1 : 0; + const promiseDecls = [...promiseIdBySpecifier.entries()].map( + ([spec, id]) => `const ${id} = ${HELPER_ID}("${spec}");` + ); + sourceFile.insertStatements(insertAt, [HELPER_DECL, ...promiseDecls]); + changes += 1 + promiseDecls.length; + + // Remove the static value imports we converted; keep any type-only named siblings. + // (Default imports never reach convertFile — analyzeConvertibility diagnoses them as non-convertible.) + for (const imp of v2ValueImports) { + if (imp.namespace) { + imp.decl.remove(); // `import * as ns` is a standalone declaration + changes++; + continue; + } + for (const n of imp.named) n.remove(); + const decl = imp.decl; + if (decl.getNamedImports().length === 0 && !decl.getDefaultImport() && !decl.getNamespaceImport()) { + decl.remove(); // nothing (incl. type-only siblings) left + } + changes++; + } + + return changes; +} + +export const commonjsInteropTransform: Transform = { + name: 'CommonJS interop for ESM-only v2', + id: 'commonjs-interop', + apply(sourceFile: SourceFile, context: TransformContext): TransformResult { + const diagnostics: Diagnostic[] = []; + if (context.moduleSystem === 'esm') return { changesCount: 0, diagnostics }; + + const v2ValueImports = collectV2ValueImports(sourceFile); + const v2ValueReExports = collectV2ValueReExports(sourceFile); + if (v2ValueImports.length === 0 && v2ValueReExports.length === 0) { + return { changesCount: 0, diagnostics }; + } + + const firstLine = v2ValueImports[0]?.decl.getStartLineNumber() ?? v2ValueReExports[0]!.getStartLineNumber(); + + if (context.moduleSystem === 'unknown') { + // Don't auto-rewrite a project we can't classify; flag once per file. + diagnostics.push( + warning( + sourceFile.getFilePath(), + firstLine, + 'Could not determine this project’s module system. If it is CommonJS, these v2 value imports/exports ' + + 'are ESM-only and will fail at load. See docs/migration/upgrade-to-v2.md.' + ) + ); + return { changesCount: 0, diagnostics }; + } + + // Value re-exports can never be made dynamic -> always diagnose, and they force the whole file + // non-convertible (a surviving static re-export re-breaks the load). + for (const exp of v2ValueReExports) { + diagnostics.push( + actionRequired( + sourceFile.getFilePath(), + exp, + `Re-export of ${exp.getModuleSpecifierValue()} carries a runtime value, but that package is ESM-only and ` + + `this project is CommonJS, so this re-export fails at load. A re-export cannot be made dynamic — migrate ` + + `the project to ESM, or replace it with a wrapper that loads the value via dynamic import() inside an async ` + + `function. See docs/migration/upgrade-to-v2.md.` + ) + ); + } + + const verdict = analyzeConvertibility(sourceFile, v2ValueImports); + if (v2ValueReExports.length > 0 || !verdict.convertible) { + const seen = new Set(); + for (const b of verdict.blockers) { + if (seen.has(b.decl)) continue; + seen.add(b.decl); + const message = b.isDefaultImport + ? `"${b.symbol}" is a default import, but v2 packages have no default export and a default import cannot ` + + `be converted to a dynamic import. Migrate the project to ESM, or use the named/namespace import form. ` + + `See docs/migration/upgrade-to-v2.md.` + : b.isValueAndType + ? `"${b.symbol}" is used as both a value and a type; converting ${b.decl.getModuleSpecifierValue()} to a ` + + `dynamic import would remove the static import and orphan the type-position usage (TS2304). Migrate the ` + + `project to ESM, or split the usages — keep the type via a separate "import type" and load the value via ` + + `dynamic import() inside an async function. See docs/migration/upgrade-to-v2.md.` + : `${b.decl.getModuleSpecifierValue()} is ESM-only and this project is CommonJS, so this static import ` + + `fails at load (ERR_PACKAGE_PATH_NOT_EXPORTED). "${b.symbol}" is used in a synchronous context ` + + `(line ${b.line}) that cannot await a dynamic import. Restructure so the value loads inside an async ` + + `function (then re-run to auto-convert), or migrate the project to ESM. See docs/migration/upgrade-to-v2.md.`; + diagnostics.push(actionRequired(sourceFile.getFilePath(), b.decl, message)); + } + return { changesCount: 0, diagnostics }; + } + + // Convertible (value imports only, all await-safe). + const changesCount = convertFile(sourceFile, v2ValueImports); + return { changesCount, diagnostics }; + } +}; 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 ca10d9b5f3..95eaa2b996 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/index.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/index.ts @@ -1,4 +1,5 @@ import type { Transform } from '../../../types'; +import { commonjsInteropTransform } from './commonjsInterop'; import { completableNestingTransform } from './completableNesting'; import { contextTypesTransform } from './contextTypes'; import { handlerRegistrationTransform } from './handlerRegistration'; @@ -33,8 +34,15 @@ import { symbolRenamesTransform } from './symbolRenames'; // 6. completableNesting runs after importPaths (it matches the rewritten // completable import) and is independent of the rest. // -// 7. mockPaths runs last: handles test mocks and dynamic imports, -// independent of the other transforms. +// 7. mockPaths handles test mocks and dynamic imports, independent of the +// other transforms. It used to run last but no longer does (see item 8). +// +// 8. commonjsInterop MUST run last: it inspects the FINAL v2 package +// specifiers that importPaths (and every later specifier-rewriting +// transform) produce, then decides whether ESM-only value imports under +// CommonJS can be converted to dynamic import() or must be diagnosed. +// Running it after all other transforms ensures it sees the settled +// import shape rather than intermediate v1/partially-rewritten specifiers. export const v1ToV2Transforms: Transform[] = [ importPathsTransform, symbolRenamesTransform, @@ -44,5 +52,6 @@ export const v1ToV2Transforms: Transform[] = [ schemaParamRemovalTransform, contextTypesTransform, completableNestingTransform, - mockPathsTransform + mockPathsTransform, + commonjsInteropTransform ]; diff --git a/packages/codemod/src/types.ts b/packages/codemod/src/types.ts index d2f42c9802..a8c83da3a2 100644 --- a/packages/codemod/src/types.ts +++ b/packages/codemod/src/types.ts @@ -34,6 +34,7 @@ export interface Transform { export interface TransformContext { projectType: 'client' | 'server' | 'both' | 'unknown'; + moduleSystem?: 'esm' | 'commonjs' | 'unknown'; } export interface Migration { diff --git a/packages/codemod/src/utils/projectAnalyzer.ts b/packages/codemod/src/utils/projectAnalyzer.ts index 1a836021c7..91bef6b4ee 100644 --- a/packages/codemod/src/utils/projectAnalyzer.ts +++ b/packages/codemod/src/utils/projectAnalyzer.ts @@ -1,6 +1,8 @@ import { existsSync, readdirSync, readFileSync, statSync } from 'node:fs'; import path from 'node:path'; +import { ts } from 'ts-morph'; + import type { Diagnostic, TransformContext } from '../types'; import { info, warning } from './diagnostics'; @@ -31,7 +33,70 @@ export function findPackageJson(startDir: string): string | undefined { } } +const ESM_TSCONFIG_MODULES = new Set(['esnext', 'es6', 'es2015', 'es2017', 'es2020', 'es2022', 'preserve']); + +function findTsconfig(startDir: string): string | undefined { + let dir = path.resolve(startDir); + const root = path.parse(dir).root; + while (true) { + const candidate = path.join(dir, 'tsconfig.json'); + if (existsSync(candidate)) return candidate; + if (dir === root) return undefined; + if (PROJECT_ROOT_MARKERS.some(m => existsSync(path.join(dir, m)))) return undefined; + dir = path.dirname(dir); + } +} + +function readPackageType(targetDir: string): string | undefined { + const pkgJsonPath = findPackageJson(targetDir); + if (!pkgJsonPath) return undefined; + try { + const pkg = JSON.parse(readFileSync(pkgJsonPath, 'utf8')) as { type?: string }; + return pkg.type; + } catch { + return undefined; + } +} + +function readTsconfigModule(targetDir: string): string | undefined { + const tsconfigPath = findTsconfig(targetDir); + if (!tsconfigPath) return undefined; + try { + // ts.parseConfigFileTextToJson handles JSONC (comments + trailing commas). `extends` is not resolved. + const raw = readFileSync(tsconfigPath, 'utf8'); + const parsed = ts.parseConfigFileTextToJson(tsconfigPath, raw); + const mod = (parsed.config as { compilerOptions?: { module?: unknown } } | undefined)?.compilerOptions?.module; + return typeof mod === 'string' ? mod.toLowerCase() : undefined; + } catch { + return undefined; + } +} + +/** + * Best-effort detection of whether the target emits ESM or CommonJS — i.e. whether a static `import` of + * an ESM-only v2 package compiles to a native `import` (works) or a `require()` (breaks at load). Reads + * the nearest `package.json` `"type"` and the nearest `tsconfig.json` `compilerOptions.module`. Never + * throws: any read/parse failure falls through to the next rule, ending at `'unknown'`. + */ +export function detectModuleSystem(targetDir: string): 'esm' | 'commonjs' | 'unknown' { + const pkgType = readPackageType(targetDir); + if (pkgType === 'module') return 'esm'; + + const tsModule = readTsconfigModule(targetDir); + if (tsModule) { + if (ESM_TSCONFIG_MODULES.has(tsModule)) return 'esm'; + if (tsModule === 'commonjs') return 'commonjs'; + if (tsModule === 'node16' || tsModule === 'nodenext') { + // pkgType === 'module' already returned 'esm' above, so here it emits CJS. + return 'commonjs'; + } + } + if (pkgType === 'commonjs') return 'commonjs'; + return 'unknown'; +} + export function analyzeProject(targetDir: string): TransformContext { + const moduleSystem = detectModuleSystem(targetDir); const pkgJsonPath = findPackageJson(targetDir); if (pkgJsonPath) { try { @@ -44,9 +109,9 @@ export function analyzeProject(targetDir: string): TransformContext { const hasClient = '@modelcontextprotocol/client' in allDeps; const hasServer = '@modelcontextprotocol/server' in allDeps; - if (hasClient && hasServer) return { projectType: 'both' }; - if (hasClient) return { projectType: 'client' }; - if (hasServer) return { projectType: 'server' }; + if (hasClient && hasServer) return { projectType: 'both', moduleSystem }; + if (hasClient) return { projectType: 'client', moduleSystem }; + if (hasServer) return { projectType: 'server', moduleSystem }; // No v2 split deps — this is almost always a v1 project mid-migration (v1 ships as the single // `@modelcontextprotocol/sdk` package). Fall through to inferring the type from source usage. } catch { @@ -54,7 +119,7 @@ export function analyzeProject(targetDir: string): TransformContext { } } - return { projectType: inferProjectTypeFromSource(targetDir) }; + return { projectType: inferProjectTypeFromSource(targetDir), moduleSystem }; } /** diff --git a/packages/codemod/test/batchTest.test.ts b/packages/codemod/test/batchTest.test.ts index 41ea0e9c61..32d7941960 100644 --- a/packages/codemod/test/batchTest.test.ts +++ b/packages/codemod/test/batchTest.test.ts @@ -3,7 +3,7 @@ import { tmpdir } from 'node:os'; import path from 'node:path'; import { describe, it, expect, vi, afterEach } from 'vitest'; -import { parseArgs, parseCodemodCliOutput, runCodemod } from '../src/bin/batchTest'; +import { parseArgs, parseCodemodCliOutput, runCodemod, installCommand } from '../src/bin/batchTest'; import { computeResultsDirName, type ResolvedConfig } from '../src/bin/batchTest'; import { parseNpmViewVersion, rewriteToPublishedVersion, cleanSubprocessEnv } from '../src/bin/batchTest'; import { getMigration } from '../src/migrations/index'; @@ -12,6 +12,33 @@ afterEach(() => { vi.restoreAllMocks(); }); +describe('installCommand', () => { + it('uses plain --ignore-scripts for non-pnpm managers', () => { + expect(installCommand('npm', { hasOwnPnpmWorkspace: false, packageDirs: ['.'] })).toBe('npm install --ignore-scripts'); + expect(installCommand('yarn', { hasOwnPnpmWorkspace: true, packageDirs: ['packages/a'] })).toBe('yarn install --ignore-scripts'); + }); + + it('isolates a pnpm clone with no workspace of its own via --ignore-workspace', () => { + expect(installCommand('pnpm', { hasOwnPnpmWorkspace: false, packageDirs: ['.'] })).toBe( + 'pnpm install --ignore-scripts --ignore-workspace --no-frozen-lockfile' + ); + }); + + it('respects a pnpm clone that is its own workspace and scopes the install to the target packages', () => { + const cmd = installCommand('pnpm', { hasOwnPnpmWorkspace: true, packageDirs: ['packages/core', 'packages/mcp'] }); + expect(cmd).not.toContain('--ignore-workspace'); + expect(cmd).toBe( + 'pnpm install --ignore-scripts --no-frozen-lockfile --filter "./packages/core..." --filter "./packages/mcp..."' + ); + }); + + it('installs the whole workspace (no --filter) when the only target is the clone root', () => { + expect(installCommand('pnpm', { hasOwnPnpmWorkspace: true, packageDirs: ['.'] })).toBe( + 'pnpm install --ignore-scripts --no-frozen-lockfile' + ); + }); +}); + describe('parseArgs', () => { it('defaults to local/local with latest codemod version and unset sdk version', () => { const opts = parseArgs([]); diff --git a/packages/codemod/test/integration.test.ts b/packages/codemod/test/integration.test.ts index 6a149da7f0..4429287546 100644 --- a/packages/codemod/test/integration.test.ts +++ b/packages/codemod/test/integration.test.ts @@ -1,12 +1,14 @@ import { mkdtempSync, mkdirSync, writeFileSync, readFileSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import path from 'node:path'; +import { Project } from 'ts-morph'; import { describe, it, expect, afterEach } from 'vitest'; import { getMigration } from '../src/migrations/index'; +import { v1ToV2Transforms } from '../src/migrations/v1-to-v2/transforms/index'; import { run } from '../src/runner'; import { DiagnosticLevel } from '../src/types'; -import type { Migration, Transform } from '../src/types'; +import type { Migration, Transform, TransformContext } from '../src/types'; const migration = getMigration('v1-to-v2')!; @@ -712,7 +714,8 @@ describe('integration', () => { expect(ids.indexOf('imports')).toBeLessThan(ids.indexOf('symbols')); expect(ids.indexOf('symbols')).toBeLessThan(ids.indexOf('removed-apis')); expect(ids.indexOf('mcpserver-api')).toBeLessThan(ids.indexOf('context')); - expect(ids.at(-1)).toBe('mock-paths'); + expect(ids.indexOf('mock-paths')).toBeLessThan(ids.indexOf('commonjs-interop')); + expect(ids.at(-1)).toBe('commonjs-interop'); }); it('processes .mts files', () => { @@ -771,3 +774,29 @@ describe('integration', () => { expect(v2Gaps.length).toBe(0); }); }); + +describe('commonjs interop — full pipeline', () => { + it('migrates v1 server usage in async context to dynamic import() under CommonJS', () => { + const project = new Project({ useInMemoryFileSystem: true }); + const sf = project.createSourceFile( + 'svc.ts', + [ + `import { CallToolResultSchema } from "@modelcontextprotocol/sdk/types.js";`, + `export async function check(body: unknown) {`, + ` return CallToolResultSchema.parse(body);`, + `}`, + '' + ].join('\n') + ); + const ctx: TransformContext = { projectType: 'server', moduleSystem: 'commonjs' }; + for (const t of v1ToV2Transforms) t.apply(sf, ctx); + const out = sf.getFullText(); + expect(out).not.toContain('@modelcontextprotocol/sdk'); + expect(out).toContain('new Function("s", "return import(s)")'); + // Converted package referenced via a runtime-erased import DECLARATION the runner detects, and a + // `typeof ` promise (not the inline `typeof import("…")` that the runner missed). + expect(out).toMatch(/import type \* as \w+ from ["']@modelcontextprotocol\/core["']/); + expect(out).toMatch(/_mcpDynImport\(["']@modelcontextprotocol\/core["']\)/); + expect(out).toContain('const { CallToolResultSchema } = await'); + }); +}); diff --git a/packages/codemod/test/projectAnalyzer.test.ts b/packages/codemod/test/projectAnalyzer.test.ts index 222894aef7..8d4bda08f0 100644 --- a/packages/codemod/test/projectAnalyzer.test.ts +++ b/packages/codemod/test/projectAnalyzer.test.ts @@ -221,3 +221,54 @@ describe('resolveTypesPackage', () => { expect(resolveTypesPackage({ projectType: 'unknown' }, false, true)).toBe('@modelcontextprotocol/server'); }); }); + +describe('analyzeProject moduleSystem', () => { + function setup(files: Record): string { + const dir = mkdtempSync(path.join(tmpdir(), 'mcp-codemod-mod-')); + mkdirSync(path.join(dir, 'src'), { recursive: true }); + for (const [rel, content] of Object.entries(files)) { + writeFileSync(path.join(dir, rel), content); + } + return dir; + } + + it('package.json type:module => esm', () => { + const dir = setup({ 'package.json': JSON.stringify({ type: 'module' }) }); + expect(analyzeProject(path.join(dir, 'src')).moduleSystem).toBe('esm'); + rmSync(dir, { recursive: true, force: true }); + }); + + it('tsconfig module:commonjs, no type => commonjs', () => { + const dir = setup({ + 'package.json': JSON.stringify({}), + 'tsconfig.json': '{ "compilerOptions": { "module": "commonjs" } } // jsonc ok' + }); + expect(analyzeProject(path.join(dir, 'src')).moduleSystem).toBe('commonjs'); + rmSync(dir, { recursive: true, force: true }); + }); + + it('tsconfig module:nodenext, no type => commonjs', () => { + const dir = setup({ + 'package.json': JSON.stringify({}), + 'tsconfig.json': '{ "compilerOptions": { "module": "nodenext" } }' + }); + expect(analyzeProject(path.join(dir, 'src')).moduleSystem).toBe('commonjs'); + rmSync(dir, { recursive: true, force: true }); + }); + + it('tsconfig module:esnext => esm', () => { + const dir = setup({ + 'package.json': JSON.stringify({}), + 'tsconfig.json': '{ "compilerOptions": { "module": "esnext" } }' + }); + expect(analyzeProject(path.join(dir, 'src')).moduleSystem).toBe('esm'); + rmSync(dir, { recursive: true, force: true }); + }); + + it('no package.json and no tsconfig => unknown', () => { + const dir = setup({}); + mkdirSync(path.join(dir, '.git'), { recursive: true }); + expect(analyzeProject(path.join(dir, 'src')).moduleSystem).toBe('unknown'); + rmSync(dir, { recursive: true, force: true }); + }); +}); diff --git a/packages/codemod/test/v1-to-v2/transforms/commonjsInterop.test.ts b/packages/codemod/test/v1-to-v2/transforms/commonjsInterop.test.ts new file mode 100644 index 0000000000..e0bc764a71 --- /dev/null +++ b/packages/codemod/test/v1-to-v2/transforms/commonjsInterop.test.ts @@ -0,0 +1,312 @@ +import { Project } from 'ts-morph'; +import { describe, expect, it } from 'vitest'; + +import { commonjsInteropTransform } from '../../../src/migrations/v1-to-v2/transforms/commonjsInterop'; +import type { TransformContext } from '../../../src/types'; +import { DiagnosticLevel } from '../../../src/types'; + +function apply(code: string, moduleSystem: TransformContext['moduleSystem']): { text: string; changes: number } { + const project = new Project({ useInMemoryFileSystem: true }); + const sourceFile = project.createSourceFile('test.ts', code); + const result = commonjsInteropTransform.apply(sourceFile, { projectType: 'server', moduleSystem }); + return { text: sourceFile.getFullText(), changes: result.changesCount }; +} + +describe('commonjs-interop transform — no-op cases', () => { + it('no-op for ESM target', () => { + const code = `import { Server } from "@modelcontextprotocol/server";\nasync function f() { return new Server(); }\n`; + const { text, changes } = apply(code, 'esm'); + expect(changes).toBe(0); + expect(text).toContain('import { Server }'); + }); + + it('no-op when only type-only v2 imports', () => { + const code = `import type { CallToolResult } from "@modelcontextprotocol/server";\nconst x: CallToolResult = {} as CallToolResult;\n`; + const { text, changes } = apply(code, 'commonjs'); + expect(changes).toBe(0); + expect(text).toContain('import type { CallToolResult }'); + }); + + it('no-op when no v2 imports at all', () => { + const code = `import { readFileSync } from "node:fs";\nreadFileSync("x");\n`; + const { changes } = apply(code, 'commonjs'); + expect(changes).toBe(0); + }); +}); + +function applyFull(code: string, moduleSystem: TransformContext['moduleSystem']) { + const project = new Project({ useInMemoryFileSystem: true }); + const sourceFile = project.createSourceFile('test.ts', code); + const result = commonjsInteropTransform.apply(sourceFile, { projectType: 'server', moduleSystem }); + return { text: sourceFile.getFullText(), result }; +} + +describe('commonjs-interop transform — convertibility & diagnostics', () => { + it('diagnoses a value used in a sync constructor (not await-safe)', () => { + const code = [ + `import { Server } from "@modelcontextprotocol/server";`, + `class Host {`, + ` server: Server;`, + ` constructor() { this.server = new Server(); }`, + `}`, + '' + ].join('\n'); + const { text, result } = applyFull(code, 'commonjs'); + // unchanged file + expect(text).toContain('import { Server }'); + // one action-required diagnostic on the import line + expect(result.diagnostics.length).toBe(1); + expect(result.diagnostics[0]!.level).toBe(DiagnosticLevel.Warning); + expect(result.diagnostics[0]!.insertComment).toBe(true); + expect(result.diagnostics[0]!.message).toMatch(/ESM-only/); + }); + + it('reports convertible=true when all value usages are in async functions (no rewrite yet)', () => { + const code = [ + `import { CallToolResultSchema } from "@modelcontextprotocol/core";`, + `async function f(res: { body: { result: unknown } }) {`, + ` return CallToolResultSchema.parse(res.body.result);`, + `}`, + '' + ].join('\n'); + const { result } = applyFull(code, 'commonjs'); + // No diagnostics for a convertible file (rewrite lands in Task 4). + expect(result.diagnostics.length).toBe(0); + }); + + it('emits a single advisory for unknown module system', () => { + const code = `import { Server } from "@modelcontextprotocol/server";\nasync function f() { return new Server(); }\n`; + const { result } = applyFull(code, 'unknown'); + expect(result.diagnostics.length).toBe(1); + expect(result.diagnostics[0]!.message).toMatch(/could not determine|couldn't determine|unknown module/i); + }); + + it('treats a type-position usage as erased (convertible despite a type annotation)', () => { + // The type annotation references a *separate* type-only import, so the value symbol `Server` + // is used purely as a value (async-safe) and remains convertible. (A symbol used as BOTH a value + // and a type is instead diagnosed — see the value-vs-type edge cases below.) + const code = [ + `import { Server } from "@modelcontextprotocol/server";`, + `import type { CallToolResult } from "@modelcontextprotocol/server";`, + `let held: CallToolResult | undefined;`, // type usage of a type-only symbol — erased + `async function f() { held = undefined; return new Server(); }`, // value usage — async-safe + '' + ].join('\n'); + const { result } = applyFull(code, 'commonjs'); + expect(result.diagnostics.length).toBe(0); // convertible + }); +}); + +describe('commonjs-interop transform — rewrite', () => { + it('converts an async-only value import to a dynamic import()', () => { + const code = [ + `import { CallToolResultSchema, ListToolsResultSchema } from "@modelcontextprotocol/core";`, + `class C {`, + ` async listTools() { return ListToolsResultSchema.parse({}); }`, + ` async callTool() { return CallToolResultSchema.parse({}); }`, + `}`, + '' + ].join('\n'); + const { text } = applyFull(code, 'commonjs'); + // static value import removed + expect(text).not.toMatch(/^import \{ CallToolResultSchema/m); + // helper present + expect(text).toContain('new Function("s", "return import(s)")'); + // type-only namespace import keeps the package a runner-recognized import DECLARATION (erased at runtime) + expect(text).toMatch(/import type \* as \w+ from ["']@modelcontextprotocol\/core["']/); + // typed module promise references that type binding (a `typeof `, not `typeof import(...)`), + // with the dynamic call arg still the package string + expect(text).toMatch(/_mcpDynImport\(["']@modelcontextprotocol\/core["']\)/); + expect(text).not.toContain('typeof import('); + // per-async-function destructures (only the symbols used in each) + expect(text).toContain('const { ListToolsResultSchema } = await'); + expect(text).toContain('const { CallToolResultSchema } = await'); + // call sites unchanged + expect(text).toContain('ListToolsResultSchema.parse({})'); + expect(text).toContain('CallToolResultSchema.parse({})'); + }); + + it('keeps a type-only sibling import intact while converting the value import', () => { + const code = [ + `import { Server } from "@modelcontextprotocol/server";`, + `import type { CallToolResult } from "@modelcontextprotocol/server";`, + `async function start(): Promise { const s = new Server(); return undefined; }`, + '' + ].join('\n'); + const { text } = applyFull(code, 'commonjs'); + expect(text).toContain('import type { CallToolResult }'); + expect(text).not.toMatch(/^import \{ Server \}/m); + expect(text).toContain('const { Server } = await'); + }); + + it('keeps an inline type sibling importable while converting the value sibling', () => { + const code = [ + `import { Server, type CallToolResult } from "@modelcontextprotocol/server";`, + `async function start(): Promise { const s = new Server(); return undefined; }`, + '' + ].join('\n'); + const { text } = applyFull(code, 'commonjs'); + // The inline `type CallToolResult` sibling survives as a type-only import. + expect(text).toContain('import { type CallToolResult }'); + // The value `Server` is no longer statically imported; it loads dynamically. + expect(text).not.toMatch(/^import \{ Server/m); + expect(text).toContain('const { Server } = await'); + }); + + // Regression (post-merge): the runner derives each manifest's v2 deps from the POST-transform import + // DECLARATIONS (packages/codemod/src/runner.ts). The earlier inline `typeof import("pkg")` form is an + // ImportTypeNode — not a declaration — and `_mcpDynImport("pkg")` is a plain call, so the converted + // package (e.g. @modelcontextprotocol/core) was dropped from package.json → TS2307 at install even though + // it is still used at runtime. The type-only namespace import keeps it a recognizable, runtime-erased + // declaration that getImportDeclarations() detects. + it('emits the converted package as an import DECLARATION the runner detects as a v2 dependency', () => { + const code = [ + `import { CallToolResultSchema } from "@modelcontextprotocol/core";`, + `async function f() { return CallToolResultSchema.parse({}); }`, + '' + ].join('\n'); + const { text } = applyFull(code, 'commonjs'); + expect(text).toMatch(/import type \* as \w+ from ["']@modelcontextprotocol\/core["']/); + + // Mirror the runner's getImportDeclarations()-based detection over the converted output. + const probe = new Project({ useInMemoryFileSystem: true }); + const detected = probe + .createSourceFile('out.ts', text) + .getImportDeclarations() + .map(d => d.getModuleSpecifierValue()); + expect(detected).toContain('@modelcontextprotocol/core'); + }); +}); + +describe('commonjs-interop transform — edge cases', () => { + it('converts a namespace value import to a whole-namespace destructure', () => { + const code = [ + `import * as mcp from "@modelcontextprotocol/core";`, + `async function f() { return mcp.CallToolResultSchema.parse({}); }`, + '' + ].join('\n'); + const { text } = applyFull(code, 'commonjs'); + expect(text).not.toContain('import * as mcp'); + expect(text).toContain('const mcp = await _mcpCore;'); + expect(text).toContain('mcp.CallToolResultSchema.parse({})'); + }); + + it('preserves an alias in the destructure', () => { + const code = [ + `import { CallToolResultSchema as CTR } from "@modelcontextprotocol/core";`, + `async function f() { return CTR.parse({}); }`, + '' + ].join('\n'); + const { text } = applyFull(code, 'commonjs'); + expect(text).toContain('const { CallToolResultSchema: CTR } = await _mcpCore;'); + expect(text).toContain('CTR.parse({})'); + }); + + it('is idempotent — a second pass makes no changes', () => { + const code = [ + `import { CallToolResultSchema } from "@modelcontextprotocol/core";`, + `async function f() { return CallToolResultSchema.parse({}); }`, + '' + ].join('\n'); + const project = new Project({ useInMemoryFileSystem: true }); + const sf = project.createSourceFile('t.ts', code); + const ctx: TransformContext = { projectType: 'server', moduleSystem: 'commonjs' }; + commonjsInteropTransform.apply(sf, ctx); + const afterFirst = sf.getFullText(); + const second = commonjsInteropTransform.apply(sf, ctx); + expect(second.changesCount).toBe(0); + expect(sf.getFullText()).toBe(afterFirst); + }); + + it('diagnoses (does not convert) an expression-bodied async arrow', () => { + const code = [ + `import { CallToolResultSchema } from "@modelcontextprotocol/core";`, + `export const parse = async (b: unknown) => CallToolResultSchema.parse(b);`, + '' + ].join('\n'); + const { text, result } = applyFull(code, 'commonjs'); + expect(text).toContain('import { CallToolResultSchema }'); // unchanged + expect(result.diagnostics.length).toBe(1); + expect(result.diagnostics[0]!.insertComment).toBe(true); + }); + + it('diagnoses a default import (v2 has no default export form)', () => { + const code = [`import core from "@modelcontextprotocol/core";`, `async function f() { return core; }`, ''].join('\n'); + const { text, result } = applyFull(code, 'commonjs'); + expect(text).toContain('import core from'); // unchanged + expect(result.diagnostics.length).toBe(1); + // Distinct, accurate reason (not the generic "synchronous context" message). + expect(result.diagnostics[0]!.message).toMatch(/default import/i); + expect(result.diagnostics[0]!.message).not.toMatch(/synchronous context/i); + }); + + it('diagnoses a v2 value re-export (cannot be made dynamic)', () => { + const code = `export { Server } from "@modelcontextprotocol/server";\n`; + const { text, result } = applyFull(code, 'commonjs'); + expect(text).toContain('export { Server } from'); // unchanged + expect(result.diagnostics.length).toBe(1); + expect(result.diagnostics[0]!.message).toMatch(/re-export/i); + }); + + it('diagnoses (does not convert) a v2 value used as an async parameter default', () => { + // The default `x = CallToolResultSchema` evaluates in parameter scope, outside the body block, + // so a body-level `const { CallToolResultSchema } = await …;` cannot satisfy it -> must be diagnosed. + const code = [ + `import { CallToolResultSchema } from "@modelcontextprotocol/core";`, + `export async function f(x = CallToolResultSchema): Promise { return (x as { parse(v: unknown): unknown }).parse({}); }`, + '' + ].join('\n'); + const { text, result } = applyFull(code, 'commonjs'); + expect(text).toContain('import { CallToolResultSchema }'); // unchanged — diagnosed, not converted + expect(text).not.toContain('new Function'); // never converted to a dynamic import + expect(result.diagnostics.length).toBe(1); + }); + + // ── Value-vs-type import handling (regression: firebase-tools broke when a value-syntax import was + // used only as a type — the static import was removed and the surviving type annotations broke with + // TS2304, plus a dead dynamic-import promise was emitted). The transform keys off value USAGE now. + it('leaves a value-syntax import used only as a type as a static import (no-op)', () => { + const code = [ + `import { CallToolResult } from "@modelcontextprotocol/server";`, + `export function f(): CallToolResult { return {} as CallToolResult; }`, + '' + ].join('\n'); + const { text, result } = applyFull(code, 'commonjs'); + // No value reference -> TS erases the import -> nothing to convert and nothing to diagnose. + expect(result.changesCount).toBe(0); + expect(text).toContain('import { CallToolResult }'); // static import left intact + expect(text).not.toContain('new Function'); // never converted to a dynamic import + expect(result.diagnostics.length).toBe(0); + }); + + it('retains a type-only-used named sibling statically while converting the value sibling', () => { + const code = [ + `import { CallToolResultSchema, CallToolResult } from "@modelcontextprotocol/core";`, + `export async function f(): Promise { return CallToolResultSchema.parse({}) as CallToolResult; }`, + '' + ].join('\n'); + const { text } = applyFull(code, 'commonjs'); + // The value symbol is converted to a dynamic import (the only package promise, and it is awaited). + expect(text).toContain('new Function("s", "return import(s)")'); + expect(text).toMatch(/_mcpDynImport\(["']@modelcontextprotocol\/core["']\)/); + expect(text).toContain('const { CallToolResultSchema } = await _mcpCore'); + // The type-only-used sibling is RETAINED as a static @modelcontextprotocol/core import (TS elides it). + expect(text).toMatch(/import \{ CallToolResult \} from ["']@modelcontextprotocol\/core["']/); + expect(text).toContain('CallToolResult'); + // The value symbol is no longer statically imported; the type symbol is NOT in the dynamic destructure. + expect(text).not.toMatch(/import \{[^}]*CallToolResultSchema/); + }); + + it('diagnoses (does not convert) a symbol used as both a value and a type', () => { + const code = [ + `import { Server } from "@modelcontextprotocol/server";`, + `let s: Server | undefined;`, + `export async function f(): Promise { s = new Server(); return s; }`, + '' + ].join('\n'); + const { text, result } = applyFull(code, 'commonjs'); + expect(text).toContain('import { Server }'); // unchanged — diagnosed, not converted + expect(text).not.toContain('new Function'); // converting would orphan the `: Server` type usage + expect(result.diagnostics.length).toBeGreaterThanOrEqual(1); + }); +}); From f9460c8e8fa770695f276cdfea3361289b26aac0 Mon Sep 17 00:00:00 2001 From: Konstantin Konstantinov Date: Wed, 1 Jul 2026 21:22:15 +0300 Subject: [PATCH 2/7] fixes --- packages/codemod/src/generated/versions.ts | 12 +- .../v1-to-v2/transforms/commonjsInterop.ts | 453 ------------------ .../v1-to-v2/transforms/contextTypes.ts | 78 +++ .../transforms/handlerRegistration.ts | 40 ++ .../migrations/v1-to-v2/transforms/index.ts | 15 +- packages/codemod/src/types.ts | 1 - packages/codemod/src/utils/projectAnalyzer.ts | 73 +-- packages/codemod/test/integration.test.ts | 33 +- packages/codemod/test/projectAnalyzer.test.ts | 51 -- .../transforms/commonjsInterop.test.ts | 312 ------------ .../v1-to-v2/transforms/contextTypes.test.ts | 65 ++- .../transforms/handlerRegistration.test.ts | 55 +++ 12 files changed, 252 insertions(+), 936 deletions(-) delete mode 100644 packages/codemod/src/migrations/v1-to-v2/transforms/commonjsInterop.ts delete mode 100644 packages/codemod/test/v1-to-v2/transforms/commonjsInterop.test.ts diff --git a/packages/codemod/src/generated/versions.ts b/packages/codemod/src/generated/versions.ts index 196a367508..a82d986db8 100644 --- a/packages/codemod/src/generated/versions.ts +++ b/packages/codemod/src/generated/versions.ts @@ -1,9 +1,9 @@ // AUTO-GENERATED — do not edit. Run `pnpm run generate:versions` to regenerate. export const V2_PACKAGE_VERSIONS: Record = { - '@modelcontextprotocol/client': '^2.0.0-alpha.3', - '@modelcontextprotocol/server': '^2.0.0-alpha.3', - '@modelcontextprotocol/node': '^2.0.0-alpha.3', - '@modelcontextprotocol/express': '^2.0.0-alpha.3', - '@modelcontextprotocol/server-legacy': '^2.0.0-alpha.3', - '@modelcontextprotocol/core': '^2.0.0-alpha.1' + '@modelcontextprotocol/client': '^2.0.0-beta.1', + '@modelcontextprotocol/server': '^2.0.0-beta.1', + '@modelcontextprotocol/node': '^2.0.0-beta.1', + '@modelcontextprotocol/express': '^2.0.0-beta.1', + '@modelcontextprotocol/server-legacy': '^2.0.0-beta.1', + '@modelcontextprotocol/core': '^2.0.0-beta.1' }; diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/commonjsInterop.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/commonjsInterop.ts deleted file mode 100644 index ace67f50bf..0000000000 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/commonjsInterop.ts +++ /dev/null @@ -1,453 +0,0 @@ -import type { Block, ExportDeclaration, Identifier, ImportDeclaration, ImportSpecifier, SourceFile } from 'ts-morph'; -import { Node } from 'ts-morph'; - -import type { Diagnostic, Transform, TransformContext, TransformResult } from '../../../types'; -import { isKeyPositionIdentifier } from '../../../utils/astUtils'; -import { actionRequired, warning } from '../../../utils/diagnostics'; - -const V2_PACKAGE_ROOTS = new Set([ - '@modelcontextprotocol/client', - '@modelcontextprotocol/server', - '@modelcontextprotocol/core', - '@modelcontextprotocol/server-legacy', - '@modelcontextprotocol/node', - '@modelcontextprotocol/express' -]); - -/** A v2 package root or any `/subpath` of one (e.g. `@modelcontextprotocol/server/stdio`). */ -export function isV2Specifier(specifier: string): boolean { - if (V2_PACKAGE_ROOTS.has(specifier)) return true; - const secondSlash = specifier.indexOf('/', specifier.indexOf('/') + 1); - return secondSlash !== -1 && V2_PACKAGE_ROOTS.has(specifier.slice(0, secondSlash)); -} - -export interface V2ValueImport { - decl: ImportDeclaration; - specifier: string; - named: ImportSpecifier[]; // value (non-type-only) named specifiers - namespace?: Identifier; // `import * as ns` - defaultImport?: Identifier; // `import d from` -} - -/** - * v2 imports with at least one VALUE-referenced binding (these break under CommonJS). - * - * The transform keys off value USAGE, not import syntax. A binding declared with a plain `import { X }` - * but used only in type position (`: X` / `as X`) has zero value references, so TypeScript erases it at - * emit time — the static import never produces a runtime `require`, so it is harmless and must be left - * intact. Including such a binding here would wrongly remove its static import and orphan the surviving - * type annotations (TS2304). So a binding is collected only when `findValueRefs` finds ≥1 value reference; - * a whole `import type { … }` (and any inline `type X` specifier) is skipped as before. (`findValueRefs` - * is a hoisted function declaration below, so it is safe to call here.) - */ -export function collectV2ValueImports(sourceFile: SourceFile): V2ValueImport[] { - const out: V2ValueImport[] = []; - for (const decl of sourceFile.getImportDeclarations()) { - const specifier = decl.getModuleSpecifierValue(); - if (!isV2Specifier(specifier)) continue; - if (decl.isTypeOnly()) continue; // whole `import type { … }` — erased, harmless - const named = decl - .getNamedImports() - .filter(n => !n.isTypeOnly() && findValueRefs(sourceFile, n.getAliasNode()?.getText() ?? n.getName()).length > 0); - const namespaceBinding = decl.getNamespaceImport(); - const namespace = - namespaceBinding && findValueRefs(sourceFile, namespaceBinding.getText()).length > 0 ? namespaceBinding : undefined; - const defaultBinding = decl.getDefaultImport(); - const defaultImport = defaultBinding && findValueRefs(sourceFile, defaultBinding.getText()).length > 0 ? defaultBinding : undefined; - if (named.length === 0 && !namespace && !defaultImport) continue; - out.push({ decl, specifier, named, namespace, defaultImport }); - } - return out; -} - -/** v2 re-exports that carry a runtime VALUE (named non-type exports, or a star re-export). These break - * under CommonJS and can't be made dynamic, so they are diagnosed (never converted). */ -export function collectV2ValueReExports(sourceFile: SourceFile): ExportDeclaration[] { - return sourceFile.getExportDeclarations().filter(exp => { - const specifier = exp.getModuleSpecifierValue(); - if (!specifier || !isV2Specifier(specifier)) return false; - if (exp.isTypeOnly()) return false; - const named = exp.getNamedExports(); - if (named.length === 0) return true; // `export * from ""` - return named.some(n => !n.isTypeOnly()); - }); -} - -/** Local binding names a v2 value import introduces (aliases preferred). */ -export function localBindings(imp: V2ValueImport): string[] { - const names: string[] = []; - if (imp.namespace) names.push(imp.namespace.getText()); - if (imp.defaultImport) names.push(imp.defaultImport.getText()); - for (const n of imp.named) names.push(n.getAliasNode()?.getText() ?? n.getName()); - return names; -} - -/** - * Conservative type-position test: returns true ONLY when the identifier is clearly part of a type - * (a type reference / type query target). Anything uncertain is treated as a VALUE usage, which biases - * toward diagnosing rather than wrongly converting a sync usage. - */ -export function isTypePositionReference(node: Node): boolean { - let current: Node | undefined = node; - // Walk up through dotted type names (`ns.Foo` in type position is a QualifiedName). - while (current) { - const parent: Node | undefined = current.getParent(); - if (!parent) return false; - if (Node.isTypeReference(parent) || Node.isTypeQuery(parent) || Node.isImportTypeNode(parent)) { - return true; - } - if (Node.isQualifiedName(parent)) { - current = parent; - continue; - } - return false; - } - return false; -} - -/** In-file value references to `localName` (excludes the import binding itself, property keys, and type positions). */ -export function findValueRefs(sourceFile: SourceFile, localName: string): Node[] { - const refs: Node[] = []; - sourceFile.forEachDescendant(node => { - if (!Node.isIdentifier(node) || node.getText() !== localName) return; - const parent = node.getParent(); - if (!parent) return; - if (Node.isImportSpecifier(parent) || Node.isImportClause(parent) || Node.isNamespaceImport(parent)) return; - if (isKeyPositionIdentifier(node)) return; - if (isTypePositionReference(node)) return; - refs.push(node); - }); - return refs; -} - -/** - * True iff `localName` appears in at least one TYPE position (a `: localName` / `as localName` / `` - * reference), excluding the import binding itself and property keys. A binding referenced as BOTH a value and - * a type cannot be converted: removing its static import (to load the value dynamically) would orphan the - * surviving type usage (TS2304). Such a binding is diagnosed rather than converted — see `analyzeConvertibility`. - */ -export function hasTypeReference(sourceFile: SourceFile, localName: string): boolean { - let found = false; - sourceFile.forEachDescendant(node => { - if (found) return; - if (!Node.isIdentifier(node) || node.getText() !== localName) return; - const parent = node.getParent(); - if (!parent) return; - if (Node.isImportSpecifier(parent) || Node.isImportClause(parent) || Node.isNamespaceImport(parent)) return; - if (isKeyPositionIdentifier(node)) return; - if (isTypePositionReference(node)) found = true; - }); - return found; -} - -/** - * True iff the nearest enclosing function-like is an async function/method/arrow WITH A BLOCK BODY - * (await-capable AND able to host a `const { … } = await …;` statement). An expression-bodied async - * arrow (`async () => expr`) returns false → the file is diagnosed rather than mis-converted into code - * that references an unbound symbol. - */ -export function isAwaitSafe(node: Node): boolean { - const fn = node.getFirstAncestor( - a => - Node.isFunctionDeclaration(a) || - Node.isFunctionExpression(a) || - Node.isArrowFunction(a) || - Node.isMethodDeclaration(a) || - Node.isConstructorDeclaration(a) || - Node.isGetAccessorDeclaration(a) || - Node.isSetAccessorDeclaration(a) - ); - if (!fn) return false; // module top-level - if (Node.isConstructorDeclaration(fn) || Node.isGetAccessorDeclaration(fn) || Node.isSetAccessorDeclaration(fn)) { - return false; // never async-capable - } - if (!fn.isAsync()) return false; - const body = fn.getBody(); - if (!body || !Node.isBlock(body)) return false; // expression-bodied async arrow -> not hostable -> diagnose - // The body is async/block-hostable, but the ref must also sit INSIDE it: parameter defaults and other - // signature positions evaluate in parameter scope (outside the body block) and can't see a body-level - // `const { … } = await …;`, so a ref there is not await-safe -> diagnose rather than emit unbound code. - return node.getStart() >= body.getStart() && node.getEnd() <= body.getEnd(); -} - -export interface Convertibility { - convertible: boolean; - // A blocker's root cause selects its diagnostic message: - // - `isDefaultImport`: a default import (no v2 default export, and no dynamic-destructure form). - // - `isValueAndType`: a named binding referenced as both a value and a type (converting would orphan the type usage). - // - neither: a value used in a synchronous context that cannot await a dynamic import. - blockers: Array<{ symbol: string; line: number; decl: ImportDeclaration; isDefaultImport?: boolean; isValueAndType?: boolean }>; -} - -export function analyzeConvertibility(sourceFile: SourceFile, v2ValueImports: V2ValueImport[]): Convertibility { - const blockers: Convertibility['blockers'] = []; - for (const imp of v2ValueImports) { - // Default imports have no dynamic-destructure form (and v2 ships no default exports) -> always diagnose. - if (imp.defaultImport) { - blockers.push({ - symbol: imp.defaultImport.getText(), - line: imp.decl.getStartLineNumber(), - decl: imp.decl, - isDefaultImport: true - }); - continue; - } - // Sync-context blockers first: when a binding is BOTH used in a sync context and used as a type, - // both reasons target the same import declaration and the diagnostic dedups per declaration. The - // sync usage is the more fundamental blocker (it cannot await even after migrating the type usage), - // so it is recorded first and wins. - for (const local of localBindings(imp)) { - for (const ref of findValueRefs(sourceFile, local)) { - if (!isAwaitSafe(ref)) { - blockers.push({ symbol: local, line: ref.getStartLineNumber(), decl: imp.decl }); - } - } - } - // A named binding referenced as both a value and a type cannot be converted: removing its static - // import to load the value dynamically would orphan the surviving `: name` / `as name` type usage - // (TS2304). Diagnose rather than convert (the conservative, type-safe choice — we do not auto-split - // the value and type usages into a retained `import type` in this pass). - for (const n of imp.named) { - const local = n.getAliasNode()?.getText() ?? n.getName(); - if (hasTypeReference(sourceFile, local)) { - blockers.push({ symbol: local, line: imp.decl.getStartLineNumber(), decl: imp.decl, isValueAndType: true }); - } - } - } - return { convertible: blockers.length === 0, blockers }; -} - -const HELPER_ID = '_mcpDynImport'; -const HELPER_DECL = - '// eslint-disable-next-line no-new-func, @typescript-eslint/no-implied-eval\n' + - `const ${HELPER_ID} = new Function("s", "return import(s)") as (s: string) => Promise;`; - -/** Camel-cased tail of a v2 specifier (e.g. `@modelcontextprotocol/server/stdio` -> `serverStdio`). */ -function packageCamel(specifier: string): string { - return specifier - .replace(/^@modelcontextprotocol\//, '') - .split(/[/-]/) - .filter(Boolean) - .map((part, i) => (i === 0 ? part : part.charAt(0).toUpperCase() + part.slice(1))) - .join(''); -} - -/** Suffix `base` with `_2`, `_3`, … until it is free in `taken`, then reserve and return it. */ -function reserveUniqueId(base: string, taken: Set): string { - let id = base; - let n = 2; - while (taken.has(id)) id = `${base}_${n++}`; - taken.add(id); - return id; -} - -/** Stable, collision-free identifier for a package's module promise (e.g. `/server/stdio` -> `_mcpServerStdio`). */ -export function packagePromiseId(specifier: string, taken: Set): string { - const camel = packageCamel(specifier); - return reserveUniqueId(`_mcp${camel.charAt(0).toUpperCase()}${camel.slice(1)}`, taken); -} - -/** - * Stable, collision-free identifier for a package's type-only namespace import - * (e.g. `/server/stdio` -> `_McpServerStdioTypes`), parallel to {@link packagePromiseId}. - * - * The `import type * as from ""` declaration this names keeps `` present as a real import - * DECLARATION — the runner derives each manifest's v2 deps from the post-transform import declarations, so the - * converted package stays in the migrated package.json — while being fully erased at runtime (a type-only import - * never emits a CommonJS `require`, so it does not re-break the ESM-only load). - */ -export function packageTypeId(specifier: string, taken: Set): string { - const camel = packageCamel(specifier); - return reserveUniqueId(`_Mcp${camel.charAt(0).toUpperCase()}${camel.slice(1)}Types`, taken); -} - -/** Named bindings a v2 value import contributes, as `{ local, imported }`. */ -function namedBindings(imp: V2ValueImport): Array<{ local: string; imported: string }> { - return imp.named.map(n => ({ local: n.getAliasNode()?.getText() ?? n.getName(), imported: n.getName() })); -} - -/** - * The nearest enclosing async-hostable function BLOCK body for a value ref, or undefined. Only - * function/method/arrow/function-expression bodies qualify (where a `const { … } = await …;` can live); - * constructors and accessors are intentionally excluded — files using a value there are diagnosed as - * non-convertible upstream, so they never reach this rewrite. - */ -function enclosingFnBlock(ref: Node): Block | undefined { - const fn = ref.getFirstAncestor( - a => Node.isFunctionDeclaration(a) || Node.isFunctionExpression(a) || Node.isArrowFunction(a) || Node.isMethodDeclaration(a) - ); - const body = fn?.getBody(); - return body && Node.isBlock(body) ? body : undefined; -} - -export function convertFile(sourceFile: SourceFile, v2ValueImports: V2ValueImport[]): number { - let changes = 0; - - // Reserve identifiers already present so generated ids never collide. - const taken = new Set(); - sourceFile.forEachDescendant(node => { - if (Node.isIdentifier(node)) taken.add(node.getText()); - }); - taken.add(HELPER_ID); - - // One promise id + one type-binding id per distinct specifier. Both are reserved against `taken` so - // they never collide with existing identifiers nor with each other. - const promiseIdBySpecifier = new Map(); - const typeIdBySpecifier = new Map(); - for (const imp of v2ValueImports) { - if (promiseIdBySpecifier.has(imp.specifier)) continue; - promiseIdBySpecifier.set(imp.specifier, packagePromiseId(imp.specifier, taken)); - typeIdBySpecifier.set(imp.specifier, packageTypeId(imp.specifier, taken)); - } - - // Insert destructures into each async function that uses a package's bindings. - for (const imp of v2ValueImports) { - const promiseId = promiseIdBySpecifier.get(imp.specifier)!; - // Map: async-function body Block -> set of destructure entries to inject. - const perFn = new Map>(); - - if (imp.namespace) { - const ns = imp.namespace.getText(); - // Whole-namespace binding: `const ns = await _pkg;` - for (const ref of findValueRefs(sourceFile, ns)) { - const body = enclosingFnBlock(ref); - if (!body) continue; - if (!perFn.has(body)) perFn.set(body, new Set()); - perFn.get(body)!.add(`__NS__${ns}`); - } - } - for (const { local, imported } of namedBindings(imp)) { - const entry = local === imported ? local : `${imported}: ${local}`; - for (const ref of findValueRefs(sourceFile, local)) { - const body = enclosingFnBlock(ref); - if (!body) continue; - if (!perFn.has(body)) perFn.set(body, new Set()); - perFn.get(body)!.add(entry); - } - } - - for (const [body, entries] of perFn) { - const nsEntry = [...entries].find(e => e.startsWith('__NS__')); - const namedEntries = [...entries].filter(e => !e.startsWith('__NS__')); - const statements: string[] = []; - if (namedEntries.length > 0) { - statements.push(`const { ${namedEntries.toSorted().join(', ')} } = await ${promiseId};`); - } - if (nsEntry) { - statements.push(`const ${nsEntry.slice('__NS__'.length)} = await ${promiseId};`); - } - body.insertStatements(0, statements); - changes += statements.length; - } - } - - // A type-only namespace import per package keeps the specifier present as a real import DECLARATION - // (the runner derives each manifest's v2 deps from getImportDeclarations(), so the converted package stays - // in the migrated package.json), while erasing at runtime — `import type` never emits a CommonJS `require`. - // The promise's `typeof` then references this binding instead of an inline `typeof import("…")`, which the - // runner's dependency detection does not recognize. - for (const [spec, typeId] of typeIdBySpecifier) { - sourceFile.addImportDeclaration({ isTypeOnly: true, namespaceImport: typeId, moduleSpecifier: spec }); - changes++; - } - - // Insert the helper + per-package promises after the last import declaration, then remove static value imports. - const imports = sourceFile.getImportDeclarations(); - const insertAt = imports.length > 0 ? imports.at(-1)!.getChildIndex() + 1 : 0; - const promiseDecls = [...promiseIdBySpecifier.entries()].map( - ([spec, id]) => `const ${id} = ${HELPER_ID}("${spec}");` - ); - sourceFile.insertStatements(insertAt, [HELPER_DECL, ...promiseDecls]); - changes += 1 + promiseDecls.length; - - // Remove the static value imports we converted; keep any type-only named siblings. - // (Default imports never reach convertFile — analyzeConvertibility diagnoses them as non-convertible.) - for (const imp of v2ValueImports) { - if (imp.namespace) { - imp.decl.remove(); // `import * as ns` is a standalone declaration - changes++; - continue; - } - for (const n of imp.named) n.remove(); - const decl = imp.decl; - if (decl.getNamedImports().length === 0 && !decl.getDefaultImport() && !decl.getNamespaceImport()) { - decl.remove(); // nothing (incl. type-only siblings) left - } - changes++; - } - - return changes; -} - -export const commonjsInteropTransform: Transform = { - name: 'CommonJS interop for ESM-only v2', - id: 'commonjs-interop', - apply(sourceFile: SourceFile, context: TransformContext): TransformResult { - const diagnostics: Diagnostic[] = []; - if (context.moduleSystem === 'esm') return { changesCount: 0, diagnostics }; - - const v2ValueImports = collectV2ValueImports(sourceFile); - const v2ValueReExports = collectV2ValueReExports(sourceFile); - if (v2ValueImports.length === 0 && v2ValueReExports.length === 0) { - return { changesCount: 0, diagnostics }; - } - - const firstLine = v2ValueImports[0]?.decl.getStartLineNumber() ?? v2ValueReExports[0]!.getStartLineNumber(); - - if (context.moduleSystem === 'unknown') { - // Don't auto-rewrite a project we can't classify; flag once per file. - diagnostics.push( - warning( - sourceFile.getFilePath(), - firstLine, - 'Could not determine this project’s module system. If it is CommonJS, these v2 value imports/exports ' + - 'are ESM-only and will fail at load. See docs/migration/upgrade-to-v2.md.' - ) - ); - return { changesCount: 0, diagnostics }; - } - - // Value re-exports can never be made dynamic -> always diagnose, and they force the whole file - // non-convertible (a surviving static re-export re-breaks the load). - for (const exp of v2ValueReExports) { - diagnostics.push( - actionRequired( - sourceFile.getFilePath(), - exp, - `Re-export of ${exp.getModuleSpecifierValue()} carries a runtime value, but that package is ESM-only and ` + - `this project is CommonJS, so this re-export fails at load. A re-export cannot be made dynamic — migrate ` + - `the project to ESM, or replace it with a wrapper that loads the value via dynamic import() inside an async ` + - `function. See docs/migration/upgrade-to-v2.md.` - ) - ); - } - - const verdict = analyzeConvertibility(sourceFile, v2ValueImports); - if (v2ValueReExports.length > 0 || !verdict.convertible) { - const seen = new Set(); - for (const b of verdict.blockers) { - if (seen.has(b.decl)) continue; - seen.add(b.decl); - const message = b.isDefaultImport - ? `"${b.symbol}" is a default import, but v2 packages have no default export and a default import cannot ` + - `be converted to a dynamic import. Migrate the project to ESM, or use the named/namespace import form. ` + - `See docs/migration/upgrade-to-v2.md.` - : b.isValueAndType - ? `"${b.symbol}" is used as both a value and a type; converting ${b.decl.getModuleSpecifierValue()} to a ` + - `dynamic import would remove the static import and orphan the type-position usage (TS2304). Migrate the ` + - `project to ESM, or split the usages — keep the type via a separate "import type" and load the value via ` + - `dynamic import() inside an async function. See docs/migration/upgrade-to-v2.md.` - : `${b.decl.getModuleSpecifierValue()} is ESM-only and this project is CommonJS, so this static import ` + - `fails at load (ERR_PACKAGE_PATH_NOT_EXPORTED). "${b.symbol}" is used in a synchronous context ` + - `(line ${b.line}) that cannot await a dynamic import. Restructure so the value loads inside an async ` + - `function (then re-run to auto-convert), or migrate the project to ESM. See docs/migration/upgrade-to-v2.md.`; - diagnostics.push(actionRequired(sourceFile.getFilePath(), b.decl, message)); - } - return { changesCount: 0, diagnostics }; - } - - // Convertible (value imports only, all await-safe). - const changesCount = convertFile(sourceFile, v2ValueImports); - return { changesCount, diagnostics }; - } -}; diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/contextTypes.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/contextTypes.ts index dd8abbd137..143bfedfb0 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/contextTypes.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/contextTypes.ts @@ -9,6 +9,24 @@ import { CONTEXT_PROPERTY_MAP, CTX_PARAM_NAME, EXTRA_PARAM_NAME } from '../mappi const CONTEXT_LIKE_KEYS = new Set(CONTEXT_PROPERTY_MAP.map(mapping => mapping.from.slice(1))); +/** + * v1 context keys distinctive enough that a single one on an object literal is a strong + * signal it's a hand-built handler-context mock (vs. generic keys like `signal`/`sessionId` + * that appear on unrelated objects and only count in aggregate). + */ +const DISTINCTIVE_CONTEXT_KEYS = new Set([ + 'sendRequest', + 'sendNotification', + 'requestId', + 'requestInfo', + 'authInfo', + 'closeSSEStream', + 'closeStandaloneSSEStream' +]); + +/** A literal already carrying one of these is in the v2 nested shape — not a stale v1 mock. */ +const V2_SHAPE_KEYS = new Set(['mcpReq', 'http', 'task']); + const HANDLER_METHODS = new Set(['setRequestHandler', 'setNotificationHandler']); const REGISTER_METHODS = new Set(['registerTool', 'registerPrompt', 'registerResource', 'tool', 'prompt', 'resource']); @@ -303,6 +321,7 @@ export const contextTypesTransform: Transform = { changesCount += processFallbackHandlerAssignments(sourceFile, diagnostics); changesCount += remapAnnotatedContextParams(sourceFile, diagnostics); + flagV1MockContextLiterals(sourceFile, diagnostics); return { changesCount, diagnostics }; } @@ -329,6 +348,65 @@ function processFallbackHandlerAssignments(sourceFile: SourceFile, diagnostics: return changes; } +/** + * Render a v1 context key as a reshape hint, e.g. `sendRequest` → `mcpReq.send`. Returns + * undefined for `sessionId` (a no-op — it stays top-level in v2) and for non-context keys. + */ +function contextKeyReshapeHint(key: string): string | undefined { + const mapping = CONTEXT_PROPERTY_MAP.find(m => m.from === '.' + key); + if (mapping === undefined || mapping.from === mapping.to) return undefined; + // Render the target as a plain object path: '.http?.authInfo' → 'http.authInfo'. + return `${key} → ${mapping.to.replace(/^\./, '').replaceAll('?', '')}`; +} + +/** + * Flag hand-built mocks of the handler context (common in tests). The call-site scan above + * only reshapes `extra.X` inside handler definitions it can anchor on (registerTool, + * setRequestHandler, fallback handlers, annotated params). A test hands its mock to a bare + * `handler(args, mockCtx)` invocation, so the object literal is never reached and keeps the + * flat v1 shape — at runtime the migrated handler reads `ctx.mcpReq.send` / `.id` / … against + * it and throws "Cannot read properties of undefined (reading 'send')". Advisory only: an + * untyped literal that merely shares a key name might not be a context mock, so never rewrite. + */ +function flagV1MockContextLiterals(sourceFile: SourceFile, diagnostics: Diagnostic[]): void { + for (const obj of sourceFile.getDescendantsOfKind(SyntaxKind.ObjectLiteralExpression)) { + // Only literals in a mock-context position: a call argument or a variable initializer. + // Covers both `handler({}, { sendRequest: fn })` and `const extra = { … }; handler(a, extra)`. + const parent = obj.getParent(); + const isCallArg = parent !== undefined && Node.isCallExpression(parent) && parent.getArguments().includes(obj); + const isVarInit = parent !== undefined && Node.isVariableDeclaration(parent) && parent.getInitializer() === obj; + if (!isCallArg && !isVarInit) continue; + + // Collect named property keys (skip spreads and computed names). + const keys: string[] = []; + for (const prop of obj.getProperties()) { + if (!Node.isPropertyAssignment(prop) && !Node.isShorthandPropertyAssignment(prop) && !Node.isMethodDeclaration(prop)) continue; + const nameNode = prop.getNameNode(); + if (Node.isIdentifier(nameNode)) keys.push(nameNode.getText()); + else if (Node.isStringLiteral(nameNode)) keys.push(nameNode.getLiteralText()); + } + if (keys.some(key => V2_SHAPE_KEYS.has(key))) continue; // already v2-shaped + + const contextKeys = keys.filter(key => CONTEXT_LIKE_KEYS.has(key)); + const hasDistinctive = contextKeys.some(key => DISTINCTIVE_CONTEXT_KEYS.has(key)); + if (!hasDistinctive && contextKeys.length < 2) continue; + + const reshapes = contextKeys.map(key => contextKeyReshapeHint(key)).filter((hint): hint is string => hint !== undefined); + const sessionNote = contextKeys.includes('sessionId') ? '; sessionId stays top-level' : ''; + diagnostics.push({ + ...actionRequired( + sourceFile.getFilePath(), + obj, + `This object looks like a v1 handler-context mock (${contextKeys.join(', ')}). v2 nests the context — ` + + `reshape it (${reshapes.join('; ')}${sessionNote}), e.g. { sendRequest: fn } → { mcpReq: { send: fn } }. ` + + `Passed as-is to a migrated handler that reads ctx.mcpReq.*, the v1 shape throws ` + + `"Cannot read properties of undefined".` + ), + advisoryOnly: true + }); + } +} + const CONTEXT_TYPE_NAMES = new Set(['RequestHandlerExtra', 'ServerContext', 'ClientContext']); /** * Split a type's text on top-level `|` only (angle brackets tracked). Returns diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/handlerRegistration.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/handlerRegistration.ts index 1ecfa00ce8..cb140aada3 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/handlerRegistration.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/handlerRegistration.ts @@ -173,6 +173,46 @@ export const handlerRegistrationTransform: Transform = { removeUnusedImport(sourceFile, schemaName, true); } + // Flag surviving registration-schema references. A method-mapped *RequestSchema/*NotificationSchema + // whose import survived the conversion above, used as a VALUE in a handler-registration context + // (a call argument or an `===`/`!==` operand) in a file that performs handler registration, is almost + // always a stale v1 registration assertion/lookup — in v2 the registration key is the method string, + // not the schema. Advisory, not rewritten: the same constant is still valid for `.parse()` etc., so we + // flag rather than risk breaking a legitimate schema use. (A converted source drops the import, so only + // files that still reference the schema — typically registration tests — are flagged.) + const usesRegistration = sourceFile + .getDescendantsOfKind(SyntaxKind.PropertyAccessExpression) + .some(pa => pa.getName() === 'setRequestHandler' || pa.getName() === 'setNotificationHandler'); + if (usesRegistration) { + const flagged = new Set(); + for (const id of sourceFile.getDescendantsOfKind(SyntaxKind.Identifier)) { + const local = id.getText(); + if (flagged.has(local)) continue; + const original = resolveOriginalImportName(sourceFile, local) ?? local; + const method = ALL_SCHEMA_TO_METHOD[original]; + if (method === undefined || !isImportedFromMcp(sourceFile, local)) continue; + const parent = id.getParent(); + if (parent === undefined) continue; + const isCallArg = Node.isCallExpression(parent) && parent.getArguments().some(a => a === id); + const isComparand = + Node.isBinaryExpression(parent) && + ['===', '!==', '==', '!='].includes(parent.getOperatorToken().getText()) && + (parent.getLeft() === id || parent.getRight() === id); + if (!isCallArg && !isComparand) continue; + flagged.add(local); + diagnostics.push({ + ...actionRequired( + sourceFile.getFilePath(), + id, + `${local} is no longer the setRequestHandler/setNotificationHandler key in v2 — handlers ` + + `register by the method string '${method}'. Update registration assertions/lookups ` + + `(e.g. against a setRequestHandler mock) to compare against '${method}'.` + ), + advisoryOnly: true + }); + } + } + return { changesCount, diagnostics }; } }; 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 95eaa2b996..ca10d9b5f3 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,4 @@ import type { Transform } from '../../../types'; -import { commonjsInteropTransform } from './commonjsInterop'; import { completableNestingTransform } from './completableNesting'; import { contextTypesTransform } from './contextTypes'; import { handlerRegistrationTransform } from './handlerRegistration'; @@ -34,15 +33,8 @@ import { symbolRenamesTransform } from './symbolRenames'; // 6. completableNesting runs after importPaths (it matches the rewritten // completable import) and is independent of the rest. // -// 7. mockPaths handles test mocks and dynamic imports, independent of the -// other transforms. It used to run last but no longer does (see item 8). -// -// 8. commonjsInterop MUST run last: it inspects the FINAL v2 package -// specifiers that importPaths (and every later specifier-rewriting -// transform) produce, then decides whether ESM-only value imports under -// CommonJS can be converted to dynamic import() or must be diagnosed. -// Running it after all other transforms ensures it sees the settled -// import shape rather than intermediate v1/partially-rewritten specifiers. +// 7. mockPaths runs last: handles test mocks and dynamic imports, +// independent of the other transforms. export const v1ToV2Transforms: Transform[] = [ importPathsTransform, symbolRenamesTransform, @@ -52,6 +44,5 @@ export const v1ToV2Transforms: Transform[] = [ schemaParamRemovalTransform, contextTypesTransform, completableNestingTransform, - mockPathsTransform, - commonjsInteropTransform + mockPathsTransform ]; diff --git a/packages/codemod/src/types.ts b/packages/codemod/src/types.ts index a8c83da3a2..d2f42c9802 100644 --- a/packages/codemod/src/types.ts +++ b/packages/codemod/src/types.ts @@ -34,7 +34,6 @@ export interface Transform { export interface TransformContext { projectType: 'client' | 'server' | 'both' | 'unknown'; - moduleSystem?: 'esm' | 'commonjs' | 'unknown'; } export interface Migration { diff --git a/packages/codemod/src/utils/projectAnalyzer.ts b/packages/codemod/src/utils/projectAnalyzer.ts index 91bef6b4ee..1a836021c7 100644 --- a/packages/codemod/src/utils/projectAnalyzer.ts +++ b/packages/codemod/src/utils/projectAnalyzer.ts @@ -1,8 +1,6 @@ import { existsSync, readdirSync, readFileSync, statSync } from 'node:fs'; import path from 'node:path'; -import { ts } from 'ts-morph'; - import type { Diagnostic, TransformContext } from '../types'; import { info, warning } from './diagnostics'; @@ -33,70 +31,7 @@ export function findPackageJson(startDir: string): string | undefined { } } -const ESM_TSCONFIG_MODULES = new Set(['esnext', 'es6', 'es2015', 'es2017', 'es2020', 'es2022', 'preserve']); - -function findTsconfig(startDir: string): string | undefined { - let dir = path.resolve(startDir); - const root = path.parse(dir).root; - while (true) { - const candidate = path.join(dir, 'tsconfig.json'); - if (existsSync(candidate)) return candidate; - if (dir === root) return undefined; - if (PROJECT_ROOT_MARKERS.some(m => existsSync(path.join(dir, m)))) return undefined; - dir = path.dirname(dir); - } -} - -function readPackageType(targetDir: string): string | undefined { - const pkgJsonPath = findPackageJson(targetDir); - if (!pkgJsonPath) return undefined; - try { - const pkg = JSON.parse(readFileSync(pkgJsonPath, 'utf8')) as { type?: string }; - return pkg.type; - } catch { - return undefined; - } -} - -function readTsconfigModule(targetDir: string): string | undefined { - const tsconfigPath = findTsconfig(targetDir); - if (!tsconfigPath) return undefined; - try { - // ts.parseConfigFileTextToJson handles JSONC (comments + trailing commas). `extends` is not resolved. - const raw = readFileSync(tsconfigPath, 'utf8'); - const parsed = ts.parseConfigFileTextToJson(tsconfigPath, raw); - const mod = (parsed.config as { compilerOptions?: { module?: unknown } } | undefined)?.compilerOptions?.module; - return typeof mod === 'string' ? mod.toLowerCase() : undefined; - } catch { - return undefined; - } -} - -/** - * Best-effort detection of whether the target emits ESM or CommonJS — i.e. whether a static `import` of - * an ESM-only v2 package compiles to a native `import` (works) or a `require()` (breaks at load). Reads - * the nearest `package.json` `"type"` and the nearest `tsconfig.json` `compilerOptions.module`. Never - * throws: any read/parse failure falls through to the next rule, ending at `'unknown'`. - */ -export function detectModuleSystem(targetDir: string): 'esm' | 'commonjs' | 'unknown' { - const pkgType = readPackageType(targetDir); - if (pkgType === 'module') return 'esm'; - - const tsModule = readTsconfigModule(targetDir); - if (tsModule) { - if (ESM_TSCONFIG_MODULES.has(tsModule)) return 'esm'; - if (tsModule === 'commonjs') return 'commonjs'; - if (tsModule === 'node16' || tsModule === 'nodenext') { - // pkgType === 'module' already returned 'esm' above, so here it emits CJS. - return 'commonjs'; - } - } - if (pkgType === 'commonjs') return 'commonjs'; - return 'unknown'; -} - export function analyzeProject(targetDir: string): TransformContext { - const moduleSystem = detectModuleSystem(targetDir); const pkgJsonPath = findPackageJson(targetDir); if (pkgJsonPath) { try { @@ -109,9 +44,9 @@ export function analyzeProject(targetDir: string): TransformContext { const hasClient = '@modelcontextprotocol/client' in allDeps; const hasServer = '@modelcontextprotocol/server' in allDeps; - if (hasClient && hasServer) return { projectType: 'both', moduleSystem }; - if (hasClient) return { projectType: 'client', moduleSystem }; - if (hasServer) return { projectType: 'server', moduleSystem }; + if (hasClient && hasServer) return { projectType: 'both' }; + if (hasClient) return { projectType: 'client' }; + if (hasServer) return { projectType: 'server' }; // No v2 split deps — this is almost always a v1 project mid-migration (v1 ships as the single // `@modelcontextprotocol/sdk` package). Fall through to inferring the type from source usage. } catch { @@ -119,7 +54,7 @@ export function analyzeProject(targetDir: string): TransformContext { } } - return { projectType: inferProjectTypeFromSource(targetDir), moduleSystem }; + return { projectType: inferProjectTypeFromSource(targetDir) }; } /** diff --git a/packages/codemod/test/integration.test.ts b/packages/codemod/test/integration.test.ts index 4429287546..6a149da7f0 100644 --- a/packages/codemod/test/integration.test.ts +++ b/packages/codemod/test/integration.test.ts @@ -1,14 +1,12 @@ import { mkdtempSync, mkdirSync, writeFileSync, readFileSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import path from 'node:path'; -import { Project } from 'ts-morph'; import { describe, it, expect, afterEach } from 'vitest'; import { getMigration } from '../src/migrations/index'; -import { v1ToV2Transforms } from '../src/migrations/v1-to-v2/transforms/index'; import { run } from '../src/runner'; import { DiagnosticLevel } from '../src/types'; -import type { Migration, Transform, TransformContext } from '../src/types'; +import type { Migration, Transform } from '../src/types'; const migration = getMigration('v1-to-v2')!; @@ -714,8 +712,7 @@ describe('integration', () => { expect(ids.indexOf('imports')).toBeLessThan(ids.indexOf('symbols')); expect(ids.indexOf('symbols')).toBeLessThan(ids.indexOf('removed-apis')); expect(ids.indexOf('mcpserver-api')).toBeLessThan(ids.indexOf('context')); - expect(ids.indexOf('mock-paths')).toBeLessThan(ids.indexOf('commonjs-interop')); - expect(ids.at(-1)).toBe('commonjs-interop'); + expect(ids.at(-1)).toBe('mock-paths'); }); it('processes .mts files', () => { @@ -774,29 +771,3 @@ describe('integration', () => { expect(v2Gaps.length).toBe(0); }); }); - -describe('commonjs interop — full pipeline', () => { - it('migrates v1 server usage in async context to dynamic import() under CommonJS', () => { - const project = new Project({ useInMemoryFileSystem: true }); - const sf = project.createSourceFile( - 'svc.ts', - [ - `import { CallToolResultSchema } from "@modelcontextprotocol/sdk/types.js";`, - `export async function check(body: unknown) {`, - ` return CallToolResultSchema.parse(body);`, - `}`, - '' - ].join('\n') - ); - const ctx: TransformContext = { projectType: 'server', moduleSystem: 'commonjs' }; - for (const t of v1ToV2Transforms) t.apply(sf, ctx); - const out = sf.getFullText(); - expect(out).not.toContain('@modelcontextprotocol/sdk'); - expect(out).toContain('new Function("s", "return import(s)")'); - // Converted package referenced via a runtime-erased import DECLARATION the runner detects, and a - // `typeof ` promise (not the inline `typeof import("…")` that the runner missed). - expect(out).toMatch(/import type \* as \w+ from ["']@modelcontextprotocol\/core["']/); - expect(out).toMatch(/_mcpDynImport\(["']@modelcontextprotocol\/core["']\)/); - expect(out).toContain('const { CallToolResultSchema } = await'); - }); -}); diff --git a/packages/codemod/test/projectAnalyzer.test.ts b/packages/codemod/test/projectAnalyzer.test.ts index 8d4bda08f0..222894aef7 100644 --- a/packages/codemod/test/projectAnalyzer.test.ts +++ b/packages/codemod/test/projectAnalyzer.test.ts @@ -221,54 +221,3 @@ describe('resolveTypesPackage', () => { expect(resolveTypesPackage({ projectType: 'unknown' }, false, true)).toBe('@modelcontextprotocol/server'); }); }); - -describe('analyzeProject moduleSystem', () => { - function setup(files: Record): string { - const dir = mkdtempSync(path.join(tmpdir(), 'mcp-codemod-mod-')); - mkdirSync(path.join(dir, 'src'), { recursive: true }); - for (const [rel, content] of Object.entries(files)) { - writeFileSync(path.join(dir, rel), content); - } - return dir; - } - - it('package.json type:module => esm', () => { - const dir = setup({ 'package.json': JSON.stringify({ type: 'module' }) }); - expect(analyzeProject(path.join(dir, 'src')).moduleSystem).toBe('esm'); - rmSync(dir, { recursive: true, force: true }); - }); - - it('tsconfig module:commonjs, no type => commonjs', () => { - const dir = setup({ - 'package.json': JSON.stringify({}), - 'tsconfig.json': '{ "compilerOptions": { "module": "commonjs" } } // jsonc ok' - }); - expect(analyzeProject(path.join(dir, 'src')).moduleSystem).toBe('commonjs'); - rmSync(dir, { recursive: true, force: true }); - }); - - it('tsconfig module:nodenext, no type => commonjs', () => { - const dir = setup({ - 'package.json': JSON.stringify({}), - 'tsconfig.json': '{ "compilerOptions": { "module": "nodenext" } }' - }); - expect(analyzeProject(path.join(dir, 'src')).moduleSystem).toBe('commonjs'); - rmSync(dir, { recursive: true, force: true }); - }); - - it('tsconfig module:esnext => esm', () => { - const dir = setup({ - 'package.json': JSON.stringify({}), - 'tsconfig.json': '{ "compilerOptions": { "module": "esnext" } }' - }); - expect(analyzeProject(path.join(dir, 'src')).moduleSystem).toBe('esm'); - rmSync(dir, { recursive: true, force: true }); - }); - - it('no package.json and no tsconfig => unknown', () => { - const dir = setup({}); - mkdirSync(path.join(dir, '.git'), { recursive: true }); - expect(analyzeProject(path.join(dir, 'src')).moduleSystem).toBe('unknown'); - rmSync(dir, { recursive: true, force: true }); - }); -}); diff --git a/packages/codemod/test/v1-to-v2/transforms/commonjsInterop.test.ts b/packages/codemod/test/v1-to-v2/transforms/commonjsInterop.test.ts deleted file mode 100644 index e0bc764a71..0000000000 --- a/packages/codemod/test/v1-to-v2/transforms/commonjsInterop.test.ts +++ /dev/null @@ -1,312 +0,0 @@ -import { Project } from 'ts-morph'; -import { describe, expect, it } from 'vitest'; - -import { commonjsInteropTransform } from '../../../src/migrations/v1-to-v2/transforms/commonjsInterop'; -import type { TransformContext } from '../../../src/types'; -import { DiagnosticLevel } from '../../../src/types'; - -function apply(code: string, moduleSystem: TransformContext['moduleSystem']): { text: string; changes: number } { - const project = new Project({ useInMemoryFileSystem: true }); - const sourceFile = project.createSourceFile('test.ts', code); - const result = commonjsInteropTransform.apply(sourceFile, { projectType: 'server', moduleSystem }); - return { text: sourceFile.getFullText(), changes: result.changesCount }; -} - -describe('commonjs-interop transform — no-op cases', () => { - it('no-op for ESM target', () => { - const code = `import { Server } from "@modelcontextprotocol/server";\nasync function f() { return new Server(); }\n`; - const { text, changes } = apply(code, 'esm'); - expect(changes).toBe(0); - expect(text).toContain('import { Server }'); - }); - - it('no-op when only type-only v2 imports', () => { - const code = `import type { CallToolResult } from "@modelcontextprotocol/server";\nconst x: CallToolResult = {} as CallToolResult;\n`; - const { text, changes } = apply(code, 'commonjs'); - expect(changes).toBe(0); - expect(text).toContain('import type { CallToolResult }'); - }); - - it('no-op when no v2 imports at all', () => { - const code = `import { readFileSync } from "node:fs";\nreadFileSync("x");\n`; - const { changes } = apply(code, 'commonjs'); - expect(changes).toBe(0); - }); -}); - -function applyFull(code: string, moduleSystem: TransformContext['moduleSystem']) { - const project = new Project({ useInMemoryFileSystem: true }); - const sourceFile = project.createSourceFile('test.ts', code); - const result = commonjsInteropTransform.apply(sourceFile, { projectType: 'server', moduleSystem }); - return { text: sourceFile.getFullText(), result }; -} - -describe('commonjs-interop transform — convertibility & diagnostics', () => { - it('diagnoses a value used in a sync constructor (not await-safe)', () => { - const code = [ - `import { Server } from "@modelcontextprotocol/server";`, - `class Host {`, - ` server: Server;`, - ` constructor() { this.server = new Server(); }`, - `}`, - '' - ].join('\n'); - const { text, result } = applyFull(code, 'commonjs'); - // unchanged file - expect(text).toContain('import { Server }'); - // one action-required diagnostic on the import line - expect(result.diagnostics.length).toBe(1); - expect(result.diagnostics[0]!.level).toBe(DiagnosticLevel.Warning); - expect(result.diagnostics[0]!.insertComment).toBe(true); - expect(result.diagnostics[0]!.message).toMatch(/ESM-only/); - }); - - it('reports convertible=true when all value usages are in async functions (no rewrite yet)', () => { - const code = [ - `import { CallToolResultSchema } from "@modelcontextprotocol/core";`, - `async function f(res: { body: { result: unknown } }) {`, - ` return CallToolResultSchema.parse(res.body.result);`, - `}`, - '' - ].join('\n'); - const { result } = applyFull(code, 'commonjs'); - // No diagnostics for a convertible file (rewrite lands in Task 4). - expect(result.diagnostics.length).toBe(0); - }); - - it('emits a single advisory for unknown module system', () => { - const code = `import { Server } from "@modelcontextprotocol/server";\nasync function f() { return new Server(); }\n`; - const { result } = applyFull(code, 'unknown'); - expect(result.diagnostics.length).toBe(1); - expect(result.diagnostics[0]!.message).toMatch(/could not determine|couldn't determine|unknown module/i); - }); - - it('treats a type-position usage as erased (convertible despite a type annotation)', () => { - // The type annotation references a *separate* type-only import, so the value symbol `Server` - // is used purely as a value (async-safe) and remains convertible. (A symbol used as BOTH a value - // and a type is instead diagnosed — see the value-vs-type edge cases below.) - const code = [ - `import { Server } from "@modelcontextprotocol/server";`, - `import type { CallToolResult } from "@modelcontextprotocol/server";`, - `let held: CallToolResult | undefined;`, // type usage of a type-only symbol — erased - `async function f() { held = undefined; return new Server(); }`, // value usage — async-safe - '' - ].join('\n'); - const { result } = applyFull(code, 'commonjs'); - expect(result.diagnostics.length).toBe(0); // convertible - }); -}); - -describe('commonjs-interop transform — rewrite', () => { - it('converts an async-only value import to a dynamic import()', () => { - const code = [ - `import { CallToolResultSchema, ListToolsResultSchema } from "@modelcontextprotocol/core";`, - `class C {`, - ` async listTools() { return ListToolsResultSchema.parse({}); }`, - ` async callTool() { return CallToolResultSchema.parse({}); }`, - `}`, - '' - ].join('\n'); - const { text } = applyFull(code, 'commonjs'); - // static value import removed - expect(text).not.toMatch(/^import \{ CallToolResultSchema/m); - // helper present - expect(text).toContain('new Function("s", "return import(s)")'); - // type-only namespace import keeps the package a runner-recognized import DECLARATION (erased at runtime) - expect(text).toMatch(/import type \* as \w+ from ["']@modelcontextprotocol\/core["']/); - // typed module promise references that type binding (a `typeof `, not `typeof import(...)`), - // with the dynamic call arg still the package string - expect(text).toMatch(/_mcpDynImport\(["']@modelcontextprotocol\/core["']\)/); - expect(text).not.toContain('typeof import('); - // per-async-function destructures (only the symbols used in each) - expect(text).toContain('const { ListToolsResultSchema } = await'); - expect(text).toContain('const { CallToolResultSchema } = await'); - // call sites unchanged - expect(text).toContain('ListToolsResultSchema.parse({})'); - expect(text).toContain('CallToolResultSchema.parse({})'); - }); - - it('keeps a type-only sibling import intact while converting the value import', () => { - const code = [ - `import { Server } from "@modelcontextprotocol/server";`, - `import type { CallToolResult } from "@modelcontextprotocol/server";`, - `async function start(): Promise { const s = new Server(); return undefined; }`, - '' - ].join('\n'); - const { text } = applyFull(code, 'commonjs'); - expect(text).toContain('import type { CallToolResult }'); - expect(text).not.toMatch(/^import \{ Server \}/m); - expect(text).toContain('const { Server } = await'); - }); - - it('keeps an inline type sibling importable while converting the value sibling', () => { - const code = [ - `import { Server, type CallToolResult } from "@modelcontextprotocol/server";`, - `async function start(): Promise { const s = new Server(); return undefined; }`, - '' - ].join('\n'); - const { text } = applyFull(code, 'commonjs'); - // The inline `type CallToolResult` sibling survives as a type-only import. - expect(text).toContain('import { type CallToolResult }'); - // The value `Server` is no longer statically imported; it loads dynamically. - expect(text).not.toMatch(/^import \{ Server/m); - expect(text).toContain('const { Server } = await'); - }); - - // Regression (post-merge): the runner derives each manifest's v2 deps from the POST-transform import - // DECLARATIONS (packages/codemod/src/runner.ts). The earlier inline `typeof import("pkg")` form is an - // ImportTypeNode — not a declaration — and `_mcpDynImport("pkg")` is a plain call, so the converted - // package (e.g. @modelcontextprotocol/core) was dropped from package.json → TS2307 at install even though - // it is still used at runtime. The type-only namespace import keeps it a recognizable, runtime-erased - // declaration that getImportDeclarations() detects. - it('emits the converted package as an import DECLARATION the runner detects as a v2 dependency', () => { - const code = [ - `import { CallToolResultSchema } from "@modelcontextprotocol/core";`, - `async function f() { return CallToolResultSchema.parse({}); }`, - '' - ].join('\n'); - const { text } = applyFull(code, 'commonjs'); - expect(text).toMatch(/import type \* as \w+ from ["']@modelcontextprotocol\/core["']/); - - // Mirror the runner's getImportDeclarations()-based detection over the converted output. - const probe = new Project({ useInMemoryFileSystem: true }); - const detected = probe - .createSourceFile('out.ts', text) - .getImportDeclarations() - .map(d => d.getModuleSpecifierValue()); - expect(detected).toContain('@modelcontextprotocol/core'); - }); -}); - -describe('commonjs-interop transform — edge cases', () => { - it('converts a namespace value import to a whole-namespace destructure', () => { - const code = [ - `import * as mcp from "@modelcontextprotocol/core";`, - `async function f() { return mcp.CallToolResultSchema.parse({}); }`, - '' - ].join('\n'); - const { text } = applyFull(code, 'commonjs'); - expect(text).not.toContain('import * as mcp'); - expect(text).toContain('const mcp = await _mcpCore;'); - expect(text).toContain('mcp.CallToolResultSchema.parse({})'); - }); - - it('preserves an alias in the destructure', () => { - const code = [ - `import { CallToolResultSchema as CTR } from "@modelcontextprotocol/core";`, - `async function f() { return CTR.parse({}); }`, - '' - ].join('\n'); - const { text } = applyFull(code, 'commonjs'); - expect(text).toContain('const { CallToolResultSchema: CTR } = await _mcpCore;'); - expect(text).toContain('CTR.parse({})'); - }); - - it('is idempotent — a second pass makes no changes', () => { - const code = [ - `import { CallToolResultSchema } from "@modelcontextprotocol/core";`, - `async function f() { return CallToolResultSchema.parse({}); }`, - '' - ].join('\n'); - const project = new Project({ useInMemoryFileSystem: true }); - const sf = project.createSourceFile('t.ts', code); - const ctx: TransformContext = { projectType: 'server', moduleSystem: 'commonjs' }; - commonjsInteropTransform.apply(sf, ctx); - const afterFirst = sf.getFullText(); - const second = commonjsInteropTransform.apply(sf, ctx); - expect(second.changesCount).toBe(0); - expect(sf.getFullText()).toBe(afterFirst); - }); - - it('diagnoses (does not convert) an expression-bodied async arrow', () => { - const code = [ - `import { CallToolResultSchema } from "@modelcontextprotocol/core";`, - `export const parse = async (b: unknown) => CallToolResultSchema.parse(b);`, - '' - ].join('\n'); - const { text, result } = applyFull(code, 'commonjs'); - expect(text).toContain('import { CallToolResultSchema }'); // unchanged - expect(result.diagnostics.length).toBe(1); - expect(result.diagnostics[0]!.insertComment).toBe(true); - }); - - it('diagnoses a default import (v2 has no default export form)', () => { - const code = [`import core from "@modelcontextprotocol/core";`, `async function f() { return core; }`, ''].join('\n'); - const { text, result } = applyFull(code, 'commonjs'); - expect(text).toContain('import core from'); // unchanged - expect(result.diagnostics.length).toBe(1); - // Distinct, accurate reason (not the generic "synchronous context" message). - expect(result.diagnostics[0]!.message).toMatch(/default import/i); - expect(result.diagnostics[0]!.message).not.toMatch(/synchronous context/i); - }); - - it('diagnoses a v2 value re-export (cannot be made dynamic)', () => { - const code = `export { Server } from "@modelcontextprotocol/server";\n`; - const { text, result } = applyFull(code, 'commonjs'); - expect(text).toContain('export { Server } from'); // unchanged - expect(result.diagnostics.length).toBe(1); - expect(result.diagnostics[0]!.message).toMatch(/re-export/i); - }); - - it('diagnoses (does not convert) a v2 value used as an async parameter default', () => { - // The default `x = CallToolResultSchema` evaluates in parameter scope, outside the body block, - // so a body-level `const { CallToolResultSchema } = await …;` cannot satisfy it -> must be diagnosed. - const code = [ - `import { CallToolResultSchema } from "@modelcontextprotocol/core";`, - `export async function f(x = CallToolResultSchema): Promise { return (x as { parse(v: unknown): unknown }).parse({}); }`, - '' - ].join('\n'); - const { text, result } = applyFull(code, 'commonjs'); - expect(text).toContain('import { CallToolResultSchema }'); // unchanged — diagnosed, not converted - expect(text).not.toContain('new Function'); // never converted to a dynamic import - expect(result.diagnostics.length).toBe(1); - }); - - // ── Value-vs-type import handling (regression: firebase-tools broke when a value-syntax import was - // used only as a type — the static import was removed and the surviving type annotations broke with - // TS2304, plus a dead dynamic-import promise was emitted). The transform keys off value USAGE now. - it('leaves a value-syntax import used only as a type as a static import (no-op)', () => { - const code = [ - `import { CallToolResult } from "@modelcontextprotocol/server";`, - `export function f(): CallToolResult { return {} as CallToolResult; }`, - '' - ].join('\n'); - const { text, result } = applyFull(code, 'commonjs'); - // No value reference -> TS erases the import -> nothing to convert and nothing to diagnose. - expect(result.changesCount).toBe(0); - expect(text).toContain('import { CallToolResult }'); // static import left intact - expect(text).not.toContain('new Function'); // never converted to a dynamic import - expect(result.diagnostics.length).toBe(0); - }); - - it('retains a type-only-used named sibling statically while converting the value sibling', () => { - const code = [ - `import { CallToolResultSchema, CallToolResult } from "@modelcontextprotocol/core";`, - `export async function f(): Promise { return CallToolResultSchema.parse({}) as CallToolResult; }`, - '' - ].join('\n'); - const { text } = applyFull(code, 'commonjs'); - // The value symbol is converted to a dynamic import (the only package promise, and it is awaited). - expect(text).toContain('new Function("s", "return import(s)")'); - expect(text).toMatch(/_mcpDynImport\(["']@modelcontextprotocol\/core["']\)/); - expect(text).toContain('const { CallToolResultSchema } = await _mcpCore'); - // The type-only-used sibling is RETAINED as a static @modelcontextprotocol/core import (TS elides it). - expect(text).toMatch(/import \{ CallToolResult \} from ["']@modelcontextprotocol\/core["']/); - expect(text).toContain('CallToolResult'); - // The value symbol is no longer statically imported; the type symbol is NOT in the dynamic destructure. - expect(text).not.toMatch(/import \{[^}]*CallToolResultSchema/); - }); - - it('diagnoses (does not convert) a symbol used as both a value and a type', () => { - const code = [ - `import { Server } from "@modelcontextprotocol/server";`, - `let s: Server | undefined;`, - `export async function f(): Promise { s = new Server(); return s; }`, - '' - ].join('\n'); - const { text, result } = applyFull(code, 'commonjs'); - expect(text).toContain('import { Server }'); // unchanged — diagnosed, not converted - expect(text).not.toContain('new Function'); // converting would orphan the `: Server` type usage - expect(result.diagnostics.length).toBeGreaterThanOrEqual(1); - }); -}); diff --git a/packages/codemod/test/v1-to-v2/transforms/contextTypes.test.ts b/packages/codemod/test/v1-to-v2/transforms/contextTypes.test.ts index 4b41562c1e..050d8bff1c 100644 --- a/packages/codemod/test/v1-to-v2/transforms/contextTypes.test.ts +++ b/packages/codemod/test/v1-to-v2/transforms/contextTypes.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect } from 'vitest'; import { Project } from 'ts-morph'; import { contextTypesTransform } from '../../../src/migrations/v1-to-v2/transforms/contextTypes'; -import type { TransformContext } from '../../../src/types'; +import type { Diagnostic, TransformContext } from '../../../src/types'; const ctx: TransformContext = { projectType: 'server' }; @@ -637,3 +637,66 @@ describe('fallback handler and annotated context params (B5, #152)', () => { expect(diag?.advisoryOnly).toBe(true); }); }); + +describe('v1 mock context literals in tests (test doubles)', () => { + function apply(code: string) { + const project = new Project({ useInMemoryFileSystem: true }); + const sourceFile = project.createSourceFile('test.ts', code); + const result = contextTypesTransform.apply(sourceFile, { projectType: 'server' }); + return { text: sourceFile.getFullText(), result }; + } + const IMPORT = `import { McpServer } from '@modelcontextprotocol/server';\n`; + const findMockDiag = (diags: Diagnostic[]) => diags.find(d => d.message.includes('v1 handler-context mock')); + + it('advises (without rewriting) on a v1 context mock passed inline as a call argument', () => { + const code = IMPORT + [`const r = await handler({}, { sendRequest: mockSendRequest });`, ''].join('\n'); + const { text, result } = apply(code); + // Advisory only — the literal is left untouched. + expect(text).toContain('{ sendRequest: mockSendRequest }'); + const diag = findMockDiag(result.diagnostics); + expect(diag).toBeDefined(); + expect(diag?.advisoryOnly).toBe(true); + expect(diag?.message).toContain('sendRequest → mcpReq.send'); + }); + + it('advises on a v1 context mock bound to a variable, and notes sessionId stays top-level', () => { + const code = IMPORT + [`const extra = { sessionId: 'session-1', sendRequest: fn };`, `await handler(args, extra);`, ''].join('\n'); + const { result } = apply(code); + const diag = findMockDiag(result.diagnostics); + expect(diag).toBeDefined(); + expect(diag?.advisoryOnly).toBe(true); + expect(diag?.message).toContain('sessionId stays top-level'); + }); + + it('advises on the _meta/requestId progress-notification mock shape', () => { + const code = IMPORT + [`await handler({ duration: 1 }, { _meta: {}, requestId: 'test-123' });`, ''].join('\n'); + const { result } = apply(code); + const diag = findMockDiag(result.diagnostics); + expect(diag).toBeDefined(); + expect(diag?.message).toContain('requestId → mcpReq.id'); + }); + + it('advises when two generic context keys appear together (no distinctive key)', () => { + const code = IMPORT + [`await handler(args, { signal: ac.signal, sessionId: 's' });`, ''].join('\n'); + const { result } = apply(code); + expect(findMockDiag(result.diagnostics)).toBeDefined(); + }); + + it('does not advise on an already-migrated v2 context mock', () => { + const code = IMPORT + [`await handler({}, { mcpReq: { send: fn } });`, ''].join('\n'); + const { result } = apply(code); + expect(findMockDiag(result.diagnostics)).toBeUndefined(); + }); + + it('does not advise on a non-context object literal', () => { + const code = IMPORT + [`const config = { title: 'x', description: 'y' };`, ''].join('\n'); + const { result } = apply(code); + expect(findMockDiag(result.diagnostics)).toBeUndefined(); + }); + + it('does not advise on a lone generic key (needs a distinctive key or >=2 context keys)', () => { + const code = IMPORT + [`doThing({ signal: ac.signal });`, ''].join('\n'); + const { result } = apply(code); + expect(findMockDiag(result.diagnostics)).toBeUndefined(); + }); +}); diff --git a/packages/codemod/test/v1-to-v2/transforms/handlerRegistration.test.ts b/packages/codemod/test/v1-to-v2/transforms/handlerRegistration.test.ts index b29f1cefcb..8b71bb37f4 100644 --- a/packages/codemod/test/v1-to-v2/transforms/handlerRegistration.test.ts +++ b/packages/codemod/test/v1-to-v2/transforms/handlerRegistration.test.ts @@ -394,3 +394,58 @@ describe('variable-hop guard rails (B6 review)', () => { expect(result.diagnostics.some(d => d.message.includes('Custom method handler'))).toBe(true); }); }); + +describe('stale registration-schema references (analysis: mcp-servers/memory)', () => { + function applyWithDiagnostics(code: string) { + const project = new Project({ useInMemoryFileSystem: true }); + const sourceFile = project.createSourceFile('test.ts', code); + const result = handlerRegistrationTransform.apply(sourceFile, { projectType: 'server' }); + return { text: sourceFile.getFullText(), result }; + } + + it('flags a request schema used as a setRequestHandler-mock assertion arg with the method string', () => { + const code = [ + `import { SubscribeRequestSchema } from '@modelcontextprotocol/sdk/types.js';`, + `const schemas = inner.setRequestHandler.mock.calls.map((c) => c[0]);`, + `expect(schemas).toContain(SubscribeRequestSchema);`, + '' + ].join('\n'); + const { result } = applyWithDiagnostics(code); + const diag = result.diagnostics.find( + d => d.message.includes('SubscribeRequestSchema') && d.message.includes("'resources/subscribe'") + ); + expect(diag).toBeDefined(); + }); + + it('flags a schema passed to a registration-lookup helper', () => { + const code = [ + `import { UnsubscribeRequestSchema } from '@modelcontextprotocol/sdk/types.js';`, + `const c = inner.setRequestHandler.mock.calls.find((x) => x[0] === s);`, + `handlerFor(inner, UnsubscribeRequestSchema);`, + '' + ].join('\n'); + const { result } = applyWithDiagnostics(code); + expect(result.diagnostics.some(d => d.message.includes("'resources/unsubscribe'"))).toBe(true); + }); + + it('does NOT flag a request schema used as a schema (.parse) — property-access base', () => { + const code = [ + `import { CallToolRequestSchema } from '@modelcontextprotocol/sdk/types.js';`, + `server.setRequestHandler(CallToolRequestSchema, async () => ({ content: [] }));`, + `const parsed = CallToolRequestSchema.parse(x);`, + '' + ].join('\n'); + const { result } = applyWithDiagnostics(code); + expect(result.diagnostics.some(d => d.message.includes('no longer the setRequestHandler'))).toBe(false); + }); + + it('does NOT flag in a file that performs no handler registration', () => { + const code = [ + `import { SubscribeRequestSchema } from '@modelcontextprotocol/sdk/types.js';`, + `someValidator(SubscribeRequestSchema);`, + '' + ].join('\n'); + const { result } = applyWithDiagnostics(code); + expect(result.diagnostics.some(d => d.message.includes('no longer the setRequestHandler'))).toBe(false); + }); +}); From 643a5a7fc05154fd779485a2aa1c7af938579f96 Mon Sep 17 00:00:00 2001 From: Konstantin Konstantinov Date: Wed, 1 Jul 2026 21:24:41 +0300 Subject: [PATCH 3/7] clean up --- packages/codemod/batch-test/repos.json | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/codemod/batch-test/repos.json b/packages/codemod/batch-test/repos.json index 960a6ec16d..28dbc4348d 100644 --- a/packages/codemod/batch-test/repos.json +++ b/packages/codemod/batch-test/repos.json @@ -1,16 +1,16 @@ [ { - "repo": "KKonstantinov/firebase-tools-fork", + "repo": "firebase/firebase-tools", "ref": "main", "packages": [ { "dir": ".", - "sourceDir": "src", + "sourceDir": "src/mcp", "checks": { - "typecheck": "npm run test:compile", + "typecheck": "npx tsc -p tsconfig.compile.json", "build": null, - "test": "npx mocha 'src/mcp/onemcp/onemcp_server.spec.ts' 'src/mcp/tool.spec.ts' 'src/mcp/prompt.spec.ts'", - "lint": "ESLINT_USE_FLAT_CONFIG=false npx eslint --config .eslintrc.js --ext .ts,.js src/mcp" + "test": null, + "lint": null } } ] From e3e6c5bf8e74fd26535350c47c8921fcc2841f45 Mon Sep 17 00:00:00 2001 From: Konstantin Konstantinov Date: Wed, 1 Jul 2026 21:28:02 +0300 Subject: [PATCH 4/7] lint fix --- .../src/migrations/v1-to-v2/transforms/handlerRegistration.ts | 2 +- packages/codemod/test/batchTest.test.ts | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/handlerRegistration.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/handlerRegistration.ts index cb140aada3..57413f6f40 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/handlerRegistration.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/handlerRegistration.ts @@ -193,7 +193,7 @@ export const handlerRegistrationTransform: Transform = { if (method === undefined || !isImportedFromMcp(sourceFile, local)) continue; const parent = id.getParent(); if (parent === undefined) continue; - const isCallArg = Node.isCallExpression(parent) && parent.getArguments().some(a => a === id); + const isCallArg = Node.isCallExpression(parent) && parent.getArguments().includes(id); const isComparand = Node.isBinaryExpression(parent) && ['===', '!==', '==', '!='].includes(parent.getOperatorToken().getText()) && diff --git a/packages/codemod/test/batchTest.test.ts b/packages/codemod/test/batchTest.test.ts index 32d7941960..3dbc1c62ae 100644 --- a/packages/codemod/test/batchTest.test.ts +++ b/packages/codemod/test/batchTest.test.ts @@ -27,9 +27,7 @@ describe('installCommand', () => { it('respects a pnpm clone that is its own workspace and scopes the install to the target packages', () => { const cmd = installCommand('pnpm', { hasOwnPnpmWorkspace: true, packageDirs: ['packages/core', 'packages/mcp'] }); expect(cmd).not.toContain('--ignore-workspace'); - expect(cmd).toBe( - 'pnpm install --ignore-scripts --no-frozen-lockfile --filter "./packages/core..." --filter "./packages/mcp..."' - ); + expect(cmd).toBe('pnpm install --ignore-scripts --no-frozen-lockfile --filter "./packages/core..." --filter "./packages/mcp..."'); }); it('installs the whole workspace (no --filter) when the only target is the clone root', () => { From 49d1237af44a1cbb17ca7f7f40ad85ca527166fc Mon Sep 17 00:00:00 2001 From: Konstantin Konstantinov Date: Wed, 1 Jul 2026 21:44:10 +0300 Subject: [PATCH 5/7] eslint ignore dist --- common/eslint-config/eslint.config.mjs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/common/eslint-config/eslint.config.mjs b/common/eslint-config/eslint.config.mjs index fb0b5d3db6..808fe1729d 100644 --- a/common/eslint-config/eslint.config.mjs +++ b/common/eslint-config/eslint.config.mjs @@ -94,6 +94,11 @@ export default defineConfig( 'no-console': 'off' } }, + { + // Ignore build artifacts everywhere (mirrors .prettierignore). A flat-config + // object with only `ignores` is a global ignore; ESLint does not skip dist by default. + ignores: ['**/dist/**', '**/build/**', '**/coverage/**'] + }, { // Ignore generated protocol types everywhere ignores: ['**/spec.types.2025-11-25.ts', '**/spec.types.2026-07-28.ts'] From b486d41a013aae9a717abf9c3c812d548592b231 Mon Sep 17 00:00:00 2001 From: Konstantin Konstantinov Date: Wed, 1 Jul 2026 22:17:42 +0300 Subject: [PATCH 6/7] claude fixes --- packages/codemod/src/bin/batchTest.ts | 8 +++++--- packages/codemod/test/batchTest.test.ts | 6 +++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/codemod/src/bin/batchTest.ts b/packages/codemod/src/bin/batchTest.ts index 93206f1d2f..1a6802bc86 100644 --- a/packages/codemod/src/bin/batchTest.ts +++ b/packages/codemod/src/bin/batchTest.ts @@ -168,11 +168,13 @@ export function installCommand(pm: string, opts: { hasOwnPnpmWorkspace: boolean; // (ERR_PNPM_CATALOG_ENTRY_NOT_FOUND_FOR_SPEC; unresolved workspace: links → repo skipped). We don't // need it: the SDK workspace excludes the clones (`!packages/codemod/batch-test/**`) and pnpm uses // the clone's own pnpm-workspace.yaml as the nearest root. Scope the install to the target packages - // and their dependencies (`...`) so a monorepo target installs only what the checks need (e.g. - // 2 of mastra's 161 projects) instead of the whole tree. + // and their dependencies (`{./}...`) so a monorepo target installs only what the checks need + // (e.g. 2 of mastra's 161 projects) instead of the whole tree. The braces are load-bearing: pnpm + // silently ignores the trailing `...` on a bare `./...` path selector (installing the package + // without its workspace deps); the `{./}...` form honors it. const filters = opts.packageDirs .filter(dir => dir !== '.') - .map(dir => `--filter ${JSON.stringify(`./${dir}...`)}`) + .map(dir => `--filter ${JSON.stringify(`{./${dir}}...`)}`) .join(' '); return `pnpm install --ignore-scripts --no-frozen-lockfile${filters ? ` ${filters}` : ''}`; } diff --git a/packages/codemod/test/batchTest.test.ts b/packages/codemod/test/batchTest.test.ts index 3dbc1c62ae..2d470f027a 100644 --- a/packages/codemod/test/batchTest.test.ts +++ b/packages/codemod/test/batchTest.test.ts @@ -27,7 +27,11 @@ describe('installCommand', () => { it('respects a pnpm clone that is its own workspace and scopes the install to the target packages', () => { const cmd = installCommand('pnpm', { hasOwnPnpmWorkspace: true, packageDirs: ['packages/core', 'packages/mcp'] }); expect(cmd).not.toContain('--ignore-workspace'); - expect(cmd).toBe('pnpm install --ignore-scripts --no-frozen-lockfile --filter "./packages/core..." --filter "./packages/mcp..."'); + // braces required: pnpm ignores the `...` on a bare `./dir...` selector (drops workspace deps); + // `{./dir}...` includes them. + expect(cmd).toBe( + 'pnpm install --ignore-scripts --no-frozen-lockfile --filter "{./packages/core}..." --filter "{./packages/mcp}..."' + ); }); it('installs the whole workspace (no --filter) when the only target is the clone root', () => { From d896d58e236df64498d29814afe489eef149171d Mon Sep 17 00:00:00 2001 From: Konstantin Konstantinov Date: Wed, 1 Jul 2026 23:38:44 +0300 Subject: [PATCH 7/7] fixes --- .../v1-to-v2/transforms/contextTypes.ts | 64 +++++++++++++++++-- .../transforms/handlerRegistration.ts | 28 +++++--- .../v1-to-v2/transforms/contextTypes.test.ts | 55 ++++++++++++++++ .../transforms/handlerRegistration.test.ts | 14 ++++ 4 files changed, 144 insertions(+), 17 deletions(-) diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/contextTypes.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/contextTypes.ts index 143bfedfb0..f0f733ba32 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/contextTypes.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/contextTypes.ts @@ -1,4 +1,4 @@ -import type { SourceFile } from 'ts-morph'; +import type { ObjectLiteralExpression, SourceFile } from 'ts-morph'; import { Node, SyntaxKind } from 'ts-morph'; import type { Diagnostic, Transform, TransformContext, TransformResult } from '../../../types'; @@ -11,13 +11,13 @@ const CONTEXT_LIKE_KEYS = new Set(CONTEXT_PROPERTY_MAP.map(mapping => mapping.fr /** * v1 context keys distinctive enough that a single one on an object literal is a strong - * signal it's a hand-built handler-context mock (vs. generic keys like `signal`/`sessionId` - * that appear on unrelated objects and only count in aggregate). + * signal it's a hand-built handler-context mock (vs. generic keys like `signal`/`sessionId`/ + * `requestId` — a bare correlation-ID literal such as `logger.info(msg, { requestId })` is + * not a context mock — which appear on unrelated objects and only count in aggregate). */ const DISTINCTIVE_CONTEXT_KEYS = new Set([ 'sendRequest', 'sendNotification', - 'requestId', 'requestInfo', 'authInfo', 'closeSSEStream', @@ -29,6 +29,13 @@ const V2_SHAPE_KEYS = new Set(['mcpReq', 'http', 'task']); const HANDLER_METHODS = new Set(['setRequestHandler', 'setNotificationHandler']); +/** + * Transport ingestion methods whose second argument is a flat `MessageExtraInfo` + * (authInfo/request/closeSSEStream/… stay top-level in v2), NOT a handler context — + * so a literal handed to them must never get handler-context reshape guidance. + */ +const TRANSPORT_MESSAGE_METHODS = new Set(['onmessage', 'handleMessage']); + const REGISTER_METHODS = new Set(['registerTool', 'registerPrompt', 'registerResource', 'tool', 'prompt', 'resource']); /** @@ -372,11 +379,36 @@ function flagV1MockContextLiterals(sourceFile: SourceFile, diagnostics: Diagnost for (const obj of sourceFile.getDescendantsOfKind(SyntaxKind.ObjectLiteralExpression)) { // Only literals in a mock-context position: a call argument or a variable initializer. // Covers both `handler({}, { sendRequest: fn })` and `const extra = { … }; handler(a, extra)`. - const parent = obj.getParent(); - const isCallArg = parent !== undefined && Node.isCallExpression(parent) && parent.getArguments().includes(obj); - const isVarInit = parent !== undefined && Node.isVariableDeclaration(parent) && parent.getInitializer() === obj; + // Unwrap casts/parens first so typed mocks — `{ … } as unknown as RequestHandlerExtra`, + // `({ … })`, `{ … } satisfies X` — are anchored by what encloses the cast, not the + // AsExpression that directly wraps the literal. + let expr: Node = obj; + let parent = expr.getParent(); + while ( + parent !== undefined && + (Node.isAsExpression(parent) || Node.isSatisfiesExpression(parent) || Node.isParenthesizedExpression(parent)) + ) { + expr = parent; + parent = parent.getParent(); + } + const isCallArg = parent !== undefined && Node.isCallExpression(parent) && parent.getArguments().includes(expr); + const isVarInit = parent !== undefined && Node.isVariableDeclaration(parent) && parent.getInitializer() === expr; if (!isCallArg && !isVarInit) continue; + // A literal handed to a transport ingestion method (`transport.onmessage(msg, { … })`, + // `transport.handleMessage(msg, { … })`) is a flat MessageExtraInfo, not a handler-context + // mock — reshaping its authInfo/request/closeSSEStream/… under http/mcpReq would be wrong. + if (isCallArg && Node.isCallExpression(parent)) { + const callee = parent.getExpression(); + if (Node.isPropertyAccessExpression(callee) && TRANSPORT_MESSAGE_METHODS.has(callee.getName())) continue; + } + + // The codemod's OWN output: an options object inside a just-rewritten handler whose values + // now read `ctx.mcpReq.*` / `ctx.http.*`. The keys keep their v1 names — so V2_SHAPE_KEYS + // below, which only inspects key names, won't catch it — but the values are already v2, not + // a stale mock. Flagging it would insert a comment above code the codemod itself produced. + if (readsFromMigratedContext(obj)) continue; + // Collect named property keys (skip spreads and computed names). const keys: string[] = []; for (const prop of obj.getProperties()) { @@ -407,6 +439,24 @@ function flagV1MockContextLiterals(sourceFile: SourceFile, diagnostics: Diagnost } } +/** + * True when any property value already reads from the v2-nested context — a `.mcpReq` or `.http` + * property access (e.g. `ctx.mcpReq.signal`, `ctx.http?.authInfo`). Such a literal is migrated + * output, not a hand-built v1 mock: a real v1 mock supplies raw values (`fn`, `ac.signal`, a + * literal), never the nested v2 shape the codemod itself just wrote. + */ +function readsFromMigratedContext(obj: ObjectLiteralExpression): boolean { + for (const prop of obj.getProperties()) { + if (!Node.isPropertyAssignment(prop)) continue; + const init = prop.getInitializer(); + if (init === undefined) continue; + const accesses = init.getDescendantsOfKind(SyntaxKind.PropertyAccessExpression); + if (Node.isPropertyAccessExpression(init)) accesses.push(init); + if (accesses.some(access => access.getName() === 'mcpReq' || access.getName() === 'http')) return true; + } + return false; +} + const CONTEXT_TYPE_NAMES = new Set(['RequestHandlerExtra', 'ServerContext', 'ClientContext']); /** * Split a type's text on top-level `|` only (angle brackets tracked). Returns diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/handlerRegistration.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/handlerRegistration.ts index 57413f6f40..9c0ff40af7 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/handlerRegistration.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/handlerRegistration.ts @@ -174,16 +174,24 @@ export const handlerRegistrationTransform: Transform = { } // Flag surviving registration-schema references. A method-mapped *RequestSchema/*NotificationSchema - // whose import survived the conversion above, used as a VALUE in a handler-registration context - // (a call argument or an `===`/`!==` operand) in a file that performs handler registration, is almost - // always a stale v1 registration assertion/lookup — in v2 the registration key is the method string, - // not the schema. Advisory, not rewritten: the same constant is still valid for `.parse()` etc., so we - // flag rather than risk breaking a legitimate schema use. (A converted source drops the import, so only - // files that still reference the schema — typically registration tests — are flagged.) - const usesRegistration = sourceFile - .getDescendantsOfKind(SyntaxKind.PropertyAccessExpression) - .some(pa => pa.getName() === 'setRequestHandler' || pa.getName() === 'setNotificationHandler'); - if (usesRegistration) { + // whose import survived the conversion above, used as a VALUE (a call argument or an `===`/`!==` + // operand), is almost always a stale v1 registration assertion/lookup — in v2 the registration key is + // the method string, not the schema. Advisory, not rewritten: the same constant is still valid for + // `.parse()` etc., so we flag rather than risk breaking a legitimate schema use. + // + // Gate on a registration MOCK/lookup, not on any setRequestHandler access. A file that only *calls* + // setRequestHandler/setNotificationHandler (real registrations, which the pass above rewrites) tells us + // nothing about its other schema references — those may be plain schema uses (`validateSchema(S)`, + // `zodToJsonSchema(S)`, `ajv.compile(S)`). Only a NON-call reference — `inner.setRequestHandler.mock`, + // `expect(x.setRequestHandler)…`, a comparison operand — signals the file makes registration + // assertions, where a surviving schema key is likely stale. + const usesRegistrationMock = sourceFile.getDescendantsOfKind(SyntaxKind.PropertyAccessExpression).some(pa => { + if (pa.getName() !== 'setRequestHandler' && pa.getName() !== 'setNotificationHandler') return false; + const paParent = pa.getParent(); + // Exclude real registration calls `x.setRequestHandler(…)`, where pa is the callee. + return !(Node.isCallExpression(paParent) && paParent.getExpression() === pa); + }); + if (usesRegistrationMock) { const flagged = new Set(); for (const id of sourceFile.getDescendantsOfKind(SyntaxKind.Identifier)) { const local = id.getText(); diff --git a/packages/codemod/test/v1-to-v2/transforms/contextTypes.test.ts b/packages/codemod/test/v1-to-v2/transforms/contextTypes.test.ts index 050d8bff1c..b530dc4b34 100644 --- a/packages/codemod/test/v1-to-v2/transforms/contextTypes.test.ts +++ b/packages/codemod/test/v1-to-v2/transforms/contextTypes.test.ts @@ -699,4 +699,59 @@ describe('v1 mock context literals in tests (test doubles)', () => { const { result } = apply(code); expect(findMockDiag(result.diagnostics)).toBeUndefined(); }); + + it('does not advise on a lone requestId (a bare correlation-ID literal, not a context mock)', () => { + const code = IMPORT + [`logger.info('handling lookup', { requestId: args.id });`, ''].join('\n'); + const { result } = apply(code); + expect(findMockDiag(result.diagnostics)).toBeUndefined(); + }); + + it('advises on a cast-wrapped inline mock (`{ … } as unknown as RequestHandlerExtra`)', () => { + const code = IMPORT + [`await handler(args, { sendRequest: fn } as unknown as RequestHandlerExtra);`, ''].join('\n'); + const { result } = apply(code); + const diag = findMockDiag(result.diagnostics); + expect(diag).toBeDefined(); + expect(diag?.advisoryOnly).toBe(true); + }); + + it('advises on a cast-wrapped variable mock (`const ctx = { … } as any`)', () => { + const code = IMPORT + [`const mockCtx = { requestId: 1, sendRequest: fn } as any;`, `await handler(args, mockCtx);`, ''].join('\n'); + const { result } = apply(code); + expect(findMockDiag(result.diagnostics)).toBeDefined(); + }); + + it('does not advise on already-v2 values (an options object reading ctx.mcpReq / ctx.sessionId)', () => { + const code = IMPORT + [`doThing({ signal: ctx.mcpReq.signal, sessionId: ctx.sessionId });`, ''].join('\n'); + const { result } = apply(code); + expect(findMockDiag(result.diagnostics)).toBeUndefined(); + }); + + it("does not re-flag the codemod's own migrated handler output", () => { + const code = + IMPORT + + [ + `server.setRequestHandler('x', async (req, extra) => {`, + ` await loadData(req.uri, { signal: extra.signal, sessionId: extra.sessionId });`, + ` return {};`, + `});`, + '' + ].join('\n'); + const { text, result } = apply(code); + // sanity: the handler body WAS migrated to the v2 nested shape … + expect(text).toContain('ctx.mcpReq.signal'); + // … and the resulting options object is not mistaken for a stale v1 mock. + expect(findMockDiag(result.diagnostics)).toBeUndefined(); + }); + + it('does not advise on a flat MessageExtraInfo literal passed to onmessage', () => { + const code = IMPORT + [`transport.onmessage(msg, { requestInfo: req, authInfo });`, ''].join('\n'); + const { result } = apply(code); + expect(findMockDiag(result.diagnostics)).toBeUndefined(); + }); + + it('does not advise on a flat MessageExtraInfo literal passed to handleMessage', () => { + const code = IMPORT + [`await transport.handleMessage(msg, { authInfo, closeSSEStream: fn });`, ''].join('\n'); + const { result } = apply(code); + expect(findMockDiag(result.diagnostics)).toBeUndefined(); + }); }); diff --git a/packages/codemod/test/v1-to-v2/transforms/handlerRegistration.test.ts b/packages/codemod/test/v1-to-v2/transforms/handlerRegistration.test.ts index 8b71bb37f4..6cf7f136d1 100644 --- a/packages/codemod/test/v1-to-v2/transforms/handlerRegistration.test.ts +++ b/packages/codemod/test/v1-to-v2/transforms/handlerRegistration.test.ts @@ -448,4 +448,18 @@ describe('stale registration-schema references (analysis: mcp-servers/memory)', const { result } = applyWithDiagnostics(code); expect(result.diagnostics.some(d => d.message.includes('no longer the setRequestHandler'))).toBe(false); }); + + it('does NOT flag a schema consumed by a validator when setRequestHandler is only a real registration', () => { + // The file registers a handler for real (a call the pass rewrites); an unrelated schema-consuming + // call like validateSchema(S) / zodToJsonSchema(S) is not a stale registration key — only a + // setRequestHandler MOCK/lookup reference signals that surviving schema refs are stale. + const code = [ + `import { CallToolRequestSchema, SubscribeRequestSchema } from '@modelcontextprotocol/sdk/types.js';`, + `server.setRequestHandler(CallToolRequestSchema, async () => ({ content: [] }));`, + `validateSchema(SubscribeRequestSchema);`, + '' + ].join('\n'); + const { result } = applyWithDiagnostics(code); + expect(result.diagnostics.some(d => d.message.includes('no longer the setRequestHandler'))).toBe(false); + }); });