Skip to content

Commit ce8ad3c

Browse files
DGouronDGouron
authored
Merge pull request #89 from DGouron/feat/cli-auto-mcp-config
feat(cli): automatic MCP server configuration validation
2 parents ed34d39 + cf6f54a commit ce8ad3c

File tree

7 files changed

+189
-18
lines changed

7 files changed

+189
-18
lines changed

‎src/cli/formatters/initSummary.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ export interface InitSummaryInput {
33
envPath: string;
44
port: number;
55
repositoryCount: number;
6-
mcpStatus: 'configured' | 'already-configured' | 'claude-not-found' | 'skipped' | 'failed';
6+
mcpStatus: 'configured' | 'already-configured' | 'claude-not-found' | 'validation-failed' | 'skipped' | 'failed';
77
gitlabUsername: string;
88
githubUsername: string;
99
}
@@ -18,6 +18,8 @@ function mcpLine(status: InitSummaryInput['mcpStatus']): string {
1818
return ' MCP server: skipped (Claude CLI not found)';
1919
case 'skipped':
2020
return ' MCP server: skipped by user';
21+
case 'validation-failed':
22+
return ' MCP server: configuration written but validation failed';
2123
case 'failed':
2224
return ' MCP server: configuration failed';
2325
}

‎src/entities/mcpSettings/mcpSettings.guard.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
import { createGuard } from '@/shared/foundation/guard.base.js';
2+
import { mcpSettingsSchema, type McpSettings } from '@/entities/mcpSettings/mcpSettings.schema.js';
3+
4+
const mcpSettingsGuard = createGuard(mcpSettingsSchema);
5+
6+
export type { McpSettings };
7+
8+
export const isValidMcpSettings: (data: unknown) => data is McpSettings = mcpSettingsGuard.isValid;
9+
export const safeParseMcpSettings = mcpSettingsGuard.safeParse;
10+
11+
export function parseMcpSettings(data: unknown): McpSettings {
12+
return mcpSettingsSchema.passthrough().parse(data);
13+
}
14+
15+
export function hasServerEntry(data: unknown, serverName: string): boolean {
16+
const result = mcpSettingsGuard.safeParse(data);
17+
if (!result.success) return false;
18+
return serverName in result.data.mcpServers;
19+
}

‎src/entities/mcpSettings/mcpSettings.schema.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import { z } from 'zod';
2+
3+
export const mcpServerEntrySchema = z.object({
4+
command: z.string(),
5+
args: z.array(z.string()),
6+
});
7+
8+
export const mcpSettingsSchema = z.object({
9+
mcpServers: z.record(z.string(), mcpServerEntrySchema),
10+
});
11+
12+
export type McpSettings = z.infer<typeof mcpSettingsSchema>;

‎src/main/cli.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,7 @@ export async function executeInit(
404404
}
405405
}
406406

407-
let mcpStatus: 'configured' | 'already-configured' | 'claude-not-found' | 'skipped' | 'failed' = 'skipped';
407+
let mcpStatus: 'configured' | 'already-configured' | 'claude-not-found' | 'validation-failed' | 'skipped' | 'failed' = 'skipped';
408408
if (!skipMcp) {
409409
console.log(dim('\nConfiguring MCP server...'));
410410
try {

‎src/tests/units/entities/mcpSettings/mcpSettings.guard.test.ts‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
import { describe, it, expect } from 'vitest';
2+
import { isValidMcpSettings, parseMcpSettings, hasServerEntry } from '@/entities/mcpSettings/mcpSettings.guard.js';
3+
4+
describe('McpSettings guard', () => {
5+
it('should validate a settings object with one mcp server', () => {
6+
const settings = {
7+
mcpServers: {
8+
'review-progress': {
9+
command: 'node',
10+
args: ['/path/to/mcpServer.js'],
11+
},
12+
},
13+
};
14+
15+
expect(isValidMcpSettings(settings)).toBe(true);
16+
});
17+
18+
it('should reject settings without mcpServers', () => {
19+
expect(isValidMcpSettings({})).toBe(false);
20+
});
21+
22+
it('should confirm server entry exists by name', () => {
23+
const settings = {
24+
mcpServers: {
25+
'review-progress': { command: 'node', args: ['/path/to/mcpServer.js'] },
26+
},
27+
};
28+
29+
expect(hasServerEntry(settings, 'review-progress')).toBe(true);
30+
});
31+
32+
it('should return false when server entry does not exist', () => {
33+
const settings = {
34+
mcpServers: {
35+
'other-server': { command: 'python', args: ['server.py'] },
36+
},
37+
};
38+
39+
expect(hasServerEntry(settings, 'review-progress')).toBe(false);
40+
});
41+
42+
it('should return false for invalid settings', () => {
43+
expect(hasServerEntry({}, 'review-progress')).toBe(false);
44+
});
45+
46+
it('should parse settings and return typed mcpServers', () => {
47+
const raw = {
48+
mcpServers: {
49+
'review-progress': {
50+
command: 'node',
51+
args: ['/path/to/mcpServer.js'],
52+
},
53+
},
54+
someOtherKey: 'preserved',
55+
};
56+
57+
const result = parseMcpSettings(raw);
58+
59+
expect(result.mcpServers['review-progress'].command).toBe('node');
60+
expect(result.mcpServers['review-progress'].args).toEqual(['/path/to/mcpServer.js']);
61+
});
62+
});

‎src/tests/units/usecases/cli/configureMcp.usecase.test.ts‎

Lines changed: 74 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { describe, it, expect, vi } from 'vitest';
22
import {
33
ConfigureMcpUseCase,
44
type ConfigureMcpDependencies,
5-
} from '../../../../usecases/cli/configureMcp.usecase.js';
5+
} from '@/usecases/cli/configureMcp.usecase.js';
66

77
function createFakeDeps(
88
overrides?: Partial<ConfigureMcpDependencies>,
@@ -31,9 +31,16 @@ describe('ConfigureMcpUseCase', () => {
3131
});
3232

3333
it('should configure mcp when settings file does not exist', () => {
34+
const validWrittenSettings = JSON.stringify({
35+
mcpServers: {
36+
'review-progress': { command: 'node', args: ['/path/to/dist/mcpServer.js'] },
37+
},
38+
});
3439
const deps = createFakeDeps({
3540
existsSync: vi.fn(() => false),
36-
readFileSync: vi.fn(() => { throw new Error('ENOENT'); }),
41+
readFileSync: vi.fn()
42+
.mockImplementationOnce(() => { throw new Error('ENOENT'); })
43+
.mockReturnValueOnce(validWrittenSettings),
3744
});
3845
const usecase = new ConfigureMcpUseCase(deps);
3946

@@ -69,16 +76,26 @@ describe('ConfigureMcpUseCase', () => {
6976
});
7077

7178
it('should update mcp config when path has changed', () => {
72-
const existingSettings = {
79+
const existingSettings = JSON.stringify({
7380
mcpServers: {
7481
'review-progress': {
7582
command: 'node',
7683
args: ['/old/path/mcpServer.js'],
7784
},
7885
},
79-
};
86+
});
87+
const updatedSettings = JSON.stringify({
88+
mcpServers: {
89+
'review-progress': {
90+
command: 'node',
91+
args: ['/path/to/dist/mcpServer.js'],
92+
},
93+
},
94+
});
8095
const deps = createFakeDeps({
81-
readFileSync: vi.fn(() => JSON.stringify(existingSettings)),
96+
readFileSync: vi.fn()
97+
.mockReturnValueOnce(existingSettings)
98+
.mockReturnValueOnce(updatedSettings),
8299
});
83100
const usecase = new ConfigureMcpUseCase(deps);
84101

@@ -89,18 +106,27 @@ describe('ConfigureMcpUseCase', () => {
89106
});
90107

91108
it('should preserve existing mcp servers when adding review-progress', () => {
92-
const existingSettings = {
109+
const existingSettings = JSON.stringify({
93110
mcpServers: {
94111
'other-server': { command: 'python', args: ['server.py'] },
95112
},
96-
};
113+
});
114+
const updatedSettings = JSON.stringify({
115+
mcpServers: {
116+
'other-server': { command: 'python', args: ['server.py'] },
117+
'review-progress': { command: 'node', args: ['/path/to/dist/mcpServer.js'] },
118+
},
119+
});
97120
const deps = createFakeDeps({
98-
readFileSync: vi.fn(() => JSON.stringify(existingSettings)),
121+
readFileSync: vi.fn()
122+
.mockReturnValueOnce(existingSettings)
123+
.mockReturnValueOnce(updatedSettings),
99124
});
100125
const usecase = new ConfigureMcpUseCase(deps);
101126

102-
usecase.execute();
127+
const result = usecase.execute();
103128

129+
expect(result).toBe('configured');
104130
const writtenContent = JSON.parse(
105131
(deps.writeFileSync as ReturnType<typeof vi.fn>).mock.calls[0][1],
106132
);
@@ -109,8 +135,15 @@ describe('ConfigureMcpUseCase', () => {
109135
});
110136

111137
it('should backup settings before modifying', () => {
138+
const validWrittenSettings = JSON.stringify({
139+
mcpServers: {
140+
'review-progress': { command: 'node', args: ['/path/to/dist/mcpServer.js'] },
141+
},
142+
});
112143
const deps = createFakeDeps({
113-
readFileSync: vi.fn(() => '{}'),
144+
readFileSync: vi.fn()
145+
.mockReturnValueOnce('{}')
146+
.mockReturnValueOnce(validWrittenSettings),
114147
});
115148
const usecase = new ConfigureMcpUseCase(deps);
116149

@@ -121,4 +154,35 @@ describe('ConfigureMcpUseCase', () => {
121154
'/home/user/.claude/settings.json.bak',
122155
);
123156
});
157+
158+
it('should return validation-failed when review-progress entry is missing after write', () => {
159+
const settingsWithoutReviewProgress = JSON.stringify({
160+
mcpServers: {
161+
'some-other-server': { command: 'python', args: ['server.py'] },
162+
},
163+
});
164+
const deps = createFakeDeps({
165+
readFileSync: vi.fn()
166+
.mockReturnValueOnce('{}')
167+
.mockReturnValueOnce(settingsWithoutReviewProgress),
168+
});
169+
const usecase = new ConfigureMcpUseCase(deps);
170+
171+
const result = usecase.execute();
172+
173+
expect(result).toBe('validation-failed');
174+
});
175+
176+
it('should return validation-failed when written file is not valid', () => {
177+
const deps = createFakeDeps({
178+
readFileSync: vi.fn()
179+
.mockReturnValueOnce('{}')
180+
.mockReturnValueOnce('not json'),
181+
});
182+
const usecase = new ConfigureMcpUseCase(deps);
183+
184+
const result = usecase.execute();
185+
186+
expect(result).toBe('validation-failed');
187+
});
124188
});

‎src/usecases/cli/configureMcp.usecase.ts‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1-
export type ConfigureMcpResult = 'configured' | 'already-configured' | 'claude-not-found' | 'failed';
1+
import { hasServerEntry, safeParseMcpSettings } from '@/entities/mcpSettings/mcpSettings.guard.js';
2+
3+
export type ConfigureMcpResult = 'configured' | 'already-configured' | 'claude-not-found' | 'validation-failed' | 'failed';
24

35
export interface ConfigureMcpDependencies {
46
isClaudeInstalled: () => boolean;
@@ -28,12 +30,13 @@ export class ConfigureMcpUseCase {
2830
settings = {};
2931
}
3032

31-
const mcpServers = (settings.mcpServers ?? {}) as Record<string, unknown>;
32-
const existing = mcpServers['review-progress'] as
33-
| { command: string; args: string[] }
34-
| undefined;
33+
const parseResult = safeParseMcpSettings(settings);
34+
const mcpServers = parseResult.success
35+
? parseResult.data.mcpServers
36+
: {};
37+
const existingArgs = mcpServers['review-progress']?.args;
3538

36-
if (existing?.args?.[0] === mcpServerPath) {
39+
if (existingArgs?.[0] === mcpServerPath) {
3740
return 'already-configured';
3841
}
3942

@@ -55,6 +58,15 @@ export class ConfigureMcpUseCase {
5558
JSON.stringify(settings, null, 2),
5659
);
5760

61+
try {
62+
const written = this.deps.readFileSync(this.deps.settingsPath, 'utf-8');
63+
if (!hasServerEntry(JSON.parse(written), 'review-progress')) {
64+
return 'validation-failed';
65+
}
66+
} catch {
67+
return 'validation-failed';
68+
}
69+
5870
return 'configured';
5971
}
6072
}

0 commit comments

Comments
 (0)