From 5ecc791d81a7221ebe15ae3ae3f36a8e978af820 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Wed, 9 Sep 2026 23:50:57 +0300 Subject: [PATCH] fix(codemod): only count real module specifiers in project-type inference (#2765) Co-authored-by: Claude --- .changeset/codemod-string-literal-imports.md | 5 ++ .../v1-to-v2/transforms/mockPaths.ts | 18 ++--- packages/codemod/src/utils/importUtils.ts | 19 ++++++ packages/codemod/src/utils/projectAnalyzer.ts | 30 ++++++-- packages/codemod/test/projectAnalyzer.test.ts | 68 +++++++++++++++++++ 5 files changed, 122 insertions(+), 18 deletions(-) create mode 100644 .changeset/codemod-string-literal-imports.md diff --git a/.changeset/codemod-string-literal-imports.md b/.changeset/codemod-string-literal-imports.md new file mode 100644 index 0000000000..9d96bcded8 --- /dev/null +++ b/.changeset/codemod-string-literal-imports.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/codemod': patch +--- + +Project-type inference no longer counts bare SDK paths that appear only in ordinary string data. The v1→v2 codemod's source scanner matched any quoted `@modelcontextprotocol/sdk/client|server` subpath anywhere in a file, so a server path stored as data (example text, a log message, a config value) misclassified a client-only project as `both` — rewriting shared type imports to `@modelcontextprotocol/server` and adding a server dependency the project never uses. The scanner now requires a module-specifier position: static imports and re-exports (`from '...'`), side-effect imports, dynamic `import('...')` (including webpack magic comments), `require('...')` / `require.resolve('...')`, and the `vi.`/`jest.` mock-method calls the mock-paths transform rewrites. Known limitation: the scan is lexical, so a string whose text embeds a complete import statement still counts. diff --git a/packages/codemod/src/migrations/v1-to-v2/transforms/mockPaths.ts b/packages/codemod/src/migrations/v1-to-v2/transforms/mockPaths.ts index bf217a936f..a7c02451ef 100644 --- a/packages/codemod/src/migrations/v1-to-v2/transforms/mockPaths.ts +++ b/packages/codemod/src/migrations/v1-to-v2/transforms/mockPaths.ts @@ -3,7 +3,7 @@ import { Node, SyntaxKind } from 'ts-morph'; import type { Diagnostic, Transform, TransformContext, TransformResult } from '../../../types'; import { actionRequired, v2Gap, warning } from '../../../utils/diagnostics'; -import { isSdkSpecifier } from '../../../utils/importUtils'; +import { isSdkSpecifier, MOCK_CALLERS, MOCK_METHODS } from '../../../utils/importUtils'; import { resolveTypesPackage } from '../../../utils/projectAnalyzer'; import type { ImportMapping } from '../mappings/importMap'; import { isAuthImport, lookupImportMapping } from '../mappings/importMap'; @@ -28,18 +28,10 @@ function routeSymbols(symbols: string[], mapping: ImportMapping): { target?: str return { mixed: false }; } -export const MOCK_METHODS: ReadonlySet = new Set([ - 'mock', - 'doMock', - 'unmock', - 'dontMock', - 'deepUnmock', - 'requireActual', - 'importActual', - 'requireMock', - 'createMockFromModule' -]); -export const MOCK_CALLERS: ReadonlySet = new Set(['vi', 'jest']); +// Defined in utils/importUtils (shared with the project analyzer, which cannot import from this +// module without a cycle — this module imports resolveTypesPackage from utils/projectAnalyzer); +// re-exported here for existing consumers (runner.ts). +export { MOCK_CALLERS, MOCK_METHODS } from '../../../utils/importUtils'; export const mockPathsTransform: Transform = { name: 'Mock and dynamic import path rewrites', diff --git a/packages/codemod/src/utils/importUtils.ts b/packages/codemod/src/utils/importUtils.ts index eba4572c7e..e2e6c70a49 100644 --- a/packages/codemod/src/utils/importUtils.ts +++ b/packages/codemod/src/utils/importUtils.ts @@ -16,6 +16,25 @@ export function isSdkSpecifier(specifier: string): boolean { return specifier === SDK_PREFIX || specifier.startsWith(SDK_PREFIX + '/'); } +/** + * Mock-framework methods whose first string argument is a module specifier. The single source of + * truth shared by the mock-paths transform (which rewrites these specifiers), the runner (which + * detects them), and the project analyzer (which counts them toward project-type inference) — + * keep the three consumers in sync by editing only this set. + */ +export const MOCK_METHODS: ReadonlySet = new Set([ + 'mock', + 'doMock', + 'unmock', + 'dontMock', + 'deepUnmock', + 'requireActual', + 'importActual', + 'requireMock', + 'createMockFromModule' +]); +export const MOCK_CALLERS: ReadonlySet = new Set(['vi', 'jest']); + export function getSdkImports(sourceFile: SourceFile): ImportDeclaration[] { return sourceFile.getImportDeclarations().filter(imp => { return isSdkSpecifier(imp.getModuleSpecifierValue()); diff --git a/packages/codemod/src/utils/projectAnalyzer.ts b/packages/codemod/src/utils/projectAnalyzer.ts index 1a836021c7..96cc86ad71 100644 --- a/packages/codemod/src/utils/projectAnalyzer.ts +++ b/packages/codemod/src/utils/projectAnalyzer.ts @@ -3,6 +3,7 @@ import path from 'node:path'; import type { Diagnostic, TransformContext } from '../types'; import { info, warning } from './diagnostics'; +import { MOCK_CALLERS, MOCK_METHODS } from './importUtils'; const PROJECT_ROOT_MARKERS = ['.git', 'node_modules']; @@ -10,14 +11,33 @@ const SCAN_EXTENSIONS = new Set(['.ts', '.tsx', '.mts', '.cts', '.js', '.jsx', ' const SCAN_SKIP_DIRS = new Set(['node_modules', 'dist', '.git', 'build', '.next', '.nuxt', 'coverage']); const SCAN_FILE_BUDGET = 5000; -// Matches a quoted v1 SDK client/server subpath import specifier — e.g. +// Matches a quoted v1 SDK client/server subpath — e.g. // '@modelcontextprotocol/sdk/client/index.js' "@modelcontextprotocol/sdk/server/mcp.js" // '@modelcontextprotocol/sdk/client' (extensionless / bare subpath; see the extensionless // import matching the codemod already supports) -// Anchored to the opening quote and a trailing `/` or closing quote so that comments or prose that -// merely mention the path do not count, and `…/client` is not confused with `…/clientfoo`. -const CLIENT_IMPORT_RE = /['"`]@modelcontextprotocol\/sdk\/client(?:\/|['"`])/; -const SERVER_IMPORT_RE = /['"`]@modelcontextprotocol\/sdk\/server(?:\/|['"`])/; +// — but only in a module-specifier position: after `from` (static imports and re-exports), `import` +// (side-effect and dynamic imports, tolerating webpack-style /* magic comments */ inside `import(`), +// `require(` / `require.resolve(`, or the vi./jest. mock-method calls the mock-paths transform +// rewrites (MOCK_CALLERS/MOCK_METHODS). A bare SDK path in ordinary string data (example text, log +// messages, config values) no longer counts toward project-type inference (#2760). +// +// Known limitation: the scan is lexical, not a parser, so a string whose TEXT embeds a full import +// statement (e.g. help text quoting `from '@modelcontextprotocol/sdk/server/mcp.js'`) still counts — +// the inner `from '` is indistinguishable from a real specifier position without parsing, which the +// budget-bounded scan deliberately avoids. +// +// The tail is anchored to a trailing `/` or closing quote so `…/client` is not confused with +// `…/clientfoo`. +const MOCK_CALL = String.raw`(?:${[...MOCK_CALLERS].join('|')})\s*\.\s*(?:${[...MOCK_METHODS].join('|')})`; +const SPECIFIER_POSITION = + String.raw`(?:\bfrom\s*` + // static import / re-export + String.raw`|\bimport\s*\(\s*(?:\/\*[\s\S]*?\*\/\s*)*` + // dynamic import(), optional magic comments + String.raw`|\bimport\s*` + // side-effect import + String.raw`|\brequire\s*(?:\.\s*resolve\s*)?\(\s*` + // require() / require.resolve() + String.raw`|\b${MOCK_CALL}\s*\(\s*` + // vi.mock(...), jest.requireActual(...), ... + `)`; +const CLIENT_IMPORT_RE = new RegExp(SPECIFIER_POSITION + /['"`]@modelcontextprotocol\/sdk\/client(?:\/|['"`])/.source); +const SERVER_IMPORT_RE = new RegExp(SPECIFIER_POSITION + /['"`]@modelcontextprotocol\/sdk\/server(?:\/|['"`])/.source); export function findPackageJson(startDir: string): string | undefined { let dir = path.resolve(startDir); diff --git a/packages/codemod/test/projectAnalyzer.test.ts b/packages/codemod/test/projectAnalyzer.test.ts index 222894aef7..8e02436733 100644 --- a/packages/codemod/test/projectAnalyzer.test.ts +++ b/packages/codemod/test/projectAnalyzer.test.ts @@ -180,6 +180,74 @@ describe('analyzeProject', () => { expect(analyzeProject(dir).projectType).toBe('client'); }); + it('ignores an SDK subpath that appears only in a string literal (not a module specifier)', () => { + // A real client import plus a server subpath stored as data in an ordinary string + // literal. Counting quoted paths anywhere would flip this to "both" (rewriting shared + // imports to the server package and adding a server dependency to a client-only + // project); only genuine module specifiers may contribute to inference (#2760). + const dir = v1Project({ + 'a.ts': [ + `import { Client } from '@modelcontextprotocol/sdk/client/index.js';`, + `const example = '@modelcontextprotocol/sdk/server/mcp.js';`, + '' + ].join('\n') + }); + expect(analyzeProject(dir).projectType).toBe('client'); + }); + + it('still infers from dynamic import() and require() specifiers', () => { + const dir = v1Project({ + 'a.ts': `const { Client } = await import('@modelcontextprotocol/sdk/client/index.js');`, + 'b.cjs': `const { McpServer } = require('@modelcontextprotocol/sdk/server/mcp.js');` + }); + expect(analyzeProject(dir).projectType).toBe('both'); + }); + + it('still infers from a side-effect import and an export-from re-export', () => { + const dir = v1Project({ + 'a.ts': `import '@modelcontextprotocol/sdk/client/index.js';`, + 'b.ts': `export { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';` + }); + expect(analyzeProject(dir).projectType).toBe('both'); + }); + + it('still infers from vi./jest. mock-method specifiers (the forms the mock-paths transform rewrites)', () => { + const dir = v1Project({ + 'a.test.ts': `vi.mock('@modelcontextprotocol/sdk/client/index.js');`, + 'b.test.ts': `const actual = jest.requireActual('@modelcontextprotocol/sdk/server/mcp.js');` + }); + expect(analyzeProject(dir).projectType).toBe('both'); + }); + + it('still infers from a dynamic import() carrying a webpack magic comment', () => { + const dir = v1Project({ + 'a.ts': `const mod = await import(/* webpackChunkName: "mcp-client" */ '@modelcontextprotocol/sdk/client/index.js');` + }); + expect(analyzeProject(dir).projectType).toBe('client'); + }); + + it('still infers from require.resolve()', () => { + const dir = v1Project({ + 'a.cjs': `const p = require.resolve('@modelcontextprotocol/sdk/server/mcp.js');` + }); + expect(analyzeProject(dir).projectType).toBe('server'); + }); + + it('documents a known limitation: a string whose text embeds a full import statement still counts', () => { + // The scan is lexical: the inner `from '` puts the quoted path in a specifier position + // even though it sits inside string data. Distinguishing that from a real import needs + // parsing, which the budget-bounded scan deliberately avoids. If the analyzer ever gets + // smart enough to make this fail, flip the expectation to 'client'. + const dir = v1Project({ + 'a.ts': [ + `import { Client } from '@modelcontextprotocol/sdk/client/index.js';`, + `const help = "import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'";`, + '' + ].join('\n') + }); + expect(analyzeProject(dir).projectType).toBe('both'); + }); + it('infers from source even without a package.json', () => { const dir = createTempDir(); mkdirSync(path.join(dir, 'src'), { recursive: true });