Skip to content

Commit 6386627

Browse files
committed
claude review fixes
1 parent a202bba commit 6386627

4 files changed

Lines changed: 233 additions & 5 deletions

File tree

packages/codemod/src/cli.ts

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,14 +20,14 @@ program.name('mcp-codemod').description('Codemod to migrate MCP TypeScript SDK c
2020

2121
for (const [name, migration] of listMigrations()) {
2222
program
23-
.command(`${name} <target-dir>`)
23+
.command(`${name} [target-dir]`)
2424
.description(migration.description)
2525
.option('-d, --dry-run', 'Preview changes without writing files')
2626
.option('-t, --transforms <ids>', 'Comma-separated transform IDs to run (default: all)')
2727
.option('-v, --verbose', 'Show detailed per-change output')
2828
.option('--ignore <patterns...>', 'Additional glob patterns to ignore')
2929
.option('--list', 'List available transforms for this migration')
30-
.action((targetDir: string, opts: Record<string, unknown>) => {
30+
.action((targetDir: string | undefined, opts: Record<string, unknown>) => {
3131
try {
3232
if (opts['list']) {
3333
console.log(`\nAvailable transforms for ${name}:\n`);
@@ -38,6 +38,12 @@ for (const [name, migration] of listMigrations()) {
3838
return;
3939
}
4040

41+
if (!targetDir) {
42+
console.error(`\nError: missing required argument <target-dir>.\n`);
43+
process.exitCode = 1;
44+
return;
45+
}
46+
4147
const resolvedDir = path.resolve(targetDir);
4248

4349
if (!existsSync(resolvedDir) || !statSync(resolvedDir).isDirectory()) {
@@ -98,7 +104,15 @@ for (const [name, migration] of listMigrations()) {
98104
console.log(formatDiagnostic(d));
99105
}
100106
console.log('');
101-
process.exitCode = 1;
107+
}
108+
109+
const infos = result.diagnostics.filter(d => d.level === DiagnosticLevel.Info);
110+
if (infos.length > 0) {
111+
console.log(`Info (${infos.length}):`);
112+
for (const d of infos) {
113+
console.log(formatDiagnostic(d));
114+
}
115+
console.log('');
102116
}
103117

104118
if (opts['dryRun']) {

packages/codemod/src/utils/projectAnalyzer.ts

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,23 @@ import path from 'node:path';
33

44
import type { TransformContext } from '../types.js';
55

6+
const PROJECT_ROOT_MARKERS = ['.git', 'node_modules'];
7+
8+
function findPackageJson(startDir: string): string | undefined {
9+
let dir = path.resolve(startDir);
10+
const root = path.parse(dir).root;
11+
while (true) {
12+
const candidate = path.join(dir, 'package.json');
13+
if (existsSync(candidate)) return candidate;
14+
if (dir === root) return undefined;
15+
if (PROJECT_ROOT_MARKERS.some(m => existsSync(path.join(dir, m)))) return undefined;
16+
dir = path.dirname(dir);
17+
}
18+
}
19+
620
export function analyzeProject(targetDir: string): TransformContext {
7-
const pkgJsonPath = path.join(targetDir, 'package.json');
8-
if (!existsSync(pkgJsonPath)) {
21+
const pkgJsonPath = findPackageJson(targetDir);
22+
if (!pkgJsonPath) {
923
return { projectType: 'unknown' };
1024
}
1125

packages/codemod/test/cli.test.ts

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
import { execFileSync } from 'node:child_process';
2+
import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs';
3+
import { tmpdir } from 'node:os';
4+
import path from 'node:path';
5+
import { describe, it, expect, afterEach } from 'vitest';
6+
7+
const CLI_PATH = path.resolve(__dirname, '../dist/cli.mjs');
8+
9+
let tempDir: string;
10+
11+
function createTempDir(): string {
12+
tempDir = mkdtempSync(path.join(tmpdir(), 'mcp-codemod-cli-'));
13+
return tempDir;
14+
}
15+
16+
function runCli(args: string[]): { stdout: string; stderr: string; exitCode: number } {
17+
try {
18+
const stdout = execFileSync('node', [CLI_PATH, ...args], {
19+
encoding: 'utf8',
20+
env: { ...process.env, NODE_NO_WARNINGS: '1' }
21+
});
22+
return { stdout, stderr: '', exitCode: 0 };
23+
} catch (error: unknown) {
24+
const e = error as { stdout: string; stderr: string; status: number };
25+
return { stdout: e.stdout ?? '', stderr: e.stderr ?? '', exitCode: e.status ?? 1 };
26+
}
27+
}
28+
29+
afterEach(() => {
30+
if (tempDir) {
31+
rmSync(tempDir, { recursive: true, force: true });
32+
}
33+
});
34+
35+
describe('CLI', () => {
36+
it('--list works without a target-dir argument', () => {
37+
const { stdout, exitCode } = runCli(['v1-to-v2', '--list']);
38+
expect(exitCode).toBe(0);
39+
expect(stdout).toContain('Available transforms');
40+
expect(stdout).toContain('imports');
41+
});
42+
43+
it('errors when target-dir is missing and --list is not set', () => {
44+
const { stderr, exitCode } = runCli(['v1-to-v2']);
45+
expect(exitCode).toBe(1);
46+
expect(stderr).toContain('missing required argument');
47+
});
48+
49+
it('exits 0 when only warnings are present (no errors)', () => {
50+
const dir = createTempDir();
51+
writeFileSync(
52+
path.join(dir, 'server.ts'),
53+
[
54+
`import { SSEServerTransport } from '@modelcontextprotocol/sdk/server/sse.js';`,
55+
`const transport = new SSEServerTransport();`,
56+
``
57+
].join('\n')
58+
);
59+
60+
const { stdout, exitCode } = runCli(['v1-to-v2', dir]);
61+
expect(stdout).toContain('Warning');
62+
expect(exitCode).toBe(0);
63+
});
64+
65+
it('prints info diagnostics', () => {
66+
const dir = createTempDir();
67+
writeFileSync(
68+
path.join(dir, 'server.ts'),
69+
[
70+
`import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';`,
71+
`const server = new McpServer({ name: 'test', version: '1.0' });`,
72+
`server.tool('greet', 'Say hello', { name: z.string() }, async ({ name }) => {`,
73+
` return { content: [{ type: 'text', text: name }] };`,
74+
`});`,
75+
``
76+
].join('\n')
77+
);
78+
79+
const { stdout, exitCode } = runCli(['v1-to-v2', dir]);
80+
expect(stdout).toContain('Info');
81+
expect(exitCode).toBe(0);
82+
});
83+
});
Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs';
2+
import { tmpdir } from 'node:os';
3+
import path from 'node:path';
4+
import { describe, it, expect, afterEach } from 'vitest';
5+
6+
import { analyzeProject } from '../src/utils/projectAnalyzer.js';
7+
8+
let tempDir: string;
9+
10+
function createTempDir(): string {
11+
tempDir = mkdtempSync(path.join(tmpdir(), 'mcp-codemod-analyzer-'));
12+
return tempDir;
13+
}
14+
15+
afterEach(() => {
16+
if (tempDir) {
17+
rmSync(tempDir, { recursive: true, force: true });
18+
}
19+
});
20+
21+
describe('analyzeProject', () => {
22+
it('returns unknown when no package.json exists', () => {
23+
const dir = createTempDir();
24+
mkdirSync(path.join(dir, '.git'), { recursive: true });
25+
mkdirSync(path.join(dir, 'src'), { recursive: true });
26+
27+
const result = analyzeProject(path.join(dir, 'src'));
28+
expect(result.projectType).toBe('unknown');
29+
});
30+
31+
it('finds package.json in parent directory', () => {
32+
const dir = createTempDir();
33+
mkdirSync(path.join(dir, 'src'), { recursive: true });
34+
writeFileSync(
35+
path.join(dir, 'package.json'),
36+
JSON.stringify({
37+
dependencies: { '@modelcontextprotocol/client': '^2.0.0' }
38+
})
39+
);
40+
41+
const result = analyzeProject(path.join(dir, 'src'));
42+
expect(result.projectType).toBe('client');
43+
});
44+
45+
it('finds package.json multiple levels up', () => {
46+
const dir = createTempDir();
47+
mkdirSync(path.join(dir, 'src', 'lib', 'utils'), { recursive: true });
48+
writeFileSync(
49+
path.join(dir, 'package.json'),
50+
JSON.stringify({
51+
dependencies: { '@modelcontextprotocol/server': '^2.0.0' }
52+
})
53+
);
54+
55+
const result = analyzeProject(path.join(dir, 'src', 'lib', 'utils'));
56+
expect(result.projectType).toBe('server');
57+
});
58+
59+
it('stops walking at .git boundary', () => {
60+
const dir = createTempDir();
61+
mkdirSync(path.join(dir, 'project', 'src'), { recursive: true });
62+
mkdirSync(path.join(dir, 'project', '.git'), { recursive: true });
63+
writeFileSync(
64+
path.join(dir, 'package.json'),
65+
JSON.stringify({
66+
dependencies: { '@modelcontextprotocol/client': '^2.0.0' }
67+
})
68+
);
69+
70+
const result = analyzeProject(path.join(dir, 'project', 'src'));
71+
expect(result.projectType).toBe('unknown');
72+
});
73+
74+
it('stops walking at node_modules boundary', () => {
75+
const dir = createTempDir();
76+
mkdirSync(path.join(dir, 'project', 'src'), { recursive: true });
77+
mkdirSync(path.join(dir, 'project', 'node_modules'), { recursive: true });
78+
writeFileSync(
79+
path.join(dir, 'package.json'),
80+
JSON.stringify({
81+
dependencies: { '@modelcontextprotocol/client': '^2.0.0' }
82+
})
83+
);
84+
85+
const result = analyzeProject(path.join(dir, 'project', 'src'));
86+
expect(result.projectType).toBe('unknown');
87+
});
88+
89+
it('detects both client and server dependencies', () => {
90+
const dir = createTempDir();
91+
writeFileSync(
92+
path.join(dir, 'package.json'),
93+
JSON.stringify({
94+
dependencies: {
95+
'@modelcontextprotocol/client': '^2.0.0',
96+
'@modelcontextprotocol/server': '^2.0.0'
97+
}
98+
})
99+
);
100+
101+
const result = analyzeProject(dir);
102+
expect(result.projectType).toBe('both');
103+
});
104+
105+
it('finds package.json at targetDir itself', () => {
106+
const dir = createTempDir();
107+
writeFileSync(
108+
path.join(dir, 'package.json'),
109+
JSON.stringify({
110+
dependencies: { '@modelcontextprotocol/server': '^2.0.0' }
111+
})
112+
);
113+
114+
const result = analyzeProject(dir);
115+
expect(result.projectType).toBe('server');
116+
});
117+
});

0 commit comments

Comments
 (0)