Skip to content

Commit 12a6b3f

Browse files
committed
fix(review): catch invalid plugin configs per-server, strengthen empty command validation
- Wrap pluginConfigToClaudeConfig in try-catch per server in createMcpRegistry so one bad plugin config doesn't crash the entire registry - Strengthen doctor command validation to catch empty string and empty array - Update tests: malformed plugin configs are now silently skipped, not thrown
1 parent 5837b7d commit 12a6b3f

File tree

3 files changed

+40
-34
lines changed

3 files changed

+40
-34
lines changed

src/cli/doctor/checks/mcp.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ export async function getUserMcpInfo(): Promise<McpServerInfo[]> {
3333
valid = false
3434
error = "Invalid config: not an object"
3535
} else if (config.type === "stdio" || (!config.type && !config.url)) {
36-
if (!config.command) {
36+
if (!config.command || (Array.isArray(config.command) && config.command.length === 0) || config.command === "") {
3737
valid = false
3838
error = "Missing required field: command"
3939
}

src/features/mcp-registry/service.test.ts

Lines changed: 25 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -109,33 +109,35 @@ describe("createMcpRegistry", () => {
109109
expect(registry.effectiveServersByName.hybrid?.transport).toBe("stdio")
110110
})
111111

112-
test("throws for malformed plugin remote config missing url", () => {
113-
// #given / #when / #then
114-
expect(() =>
115-
createMcpRegistry({
116-
pluginServers: {
117-
broken: {
118-
type: "remote",
119-
// Intentionally malformed to verify runtime guard.
120-
url: "",
121-
},
112+
test("skips malformed plugin remote config missing url", () => {
113+
// #given / #when
114+
const registry = createMcpRegistry({
115+
pluginServers: {
116+
broken: {
117+
type: "remote",
118+
// Intentionally malformed — should be silently skipped.
119+
url: "",
122120
},
123-
})
124-
).toThrow(/missing url/)
121+
},
122+
})
123+
124+
// #then
125+
expect(registry.effectiveServers).toHaveLength(0)
125126
})
126127

127-
test("throws for malformed plugin local config with empty command", () => {
128-
// #given / #when / #then
129-
expect(() =>
130-
createMcpRegistry({
131-
pluginServers: {
132-
broken: {
133-
type: "local",
134-
command: [],
135-
},
128+
test("skips malformed plugin local config with empty command", () => {
129+
// #given / #when
130+
const registry = createMcpRegistry({
131+
pluginServers: {
132+
broken: {
133+
type: "local",
134+
command: [],
136135
},
137-
})
138-
).toThrow(/empty command array/)
136+
},
137+
})
138+
139+
// #then
140+
expect(registry.effectiveServers).toHaveLength(0)
139141
})
140142
})
141143

src/features/mcp-registry/service.ts

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -116,17 +116,21 @@ export function createMcpRegistry(input: CreateMcpRegistryInput): McpRegistryRes
116116
}
117117

118118
for (const [name, config] of Object.entries(input.pluginServers ?? {})) {
119-
const claudeConfig = pluginConfigToClaudeConfig(config)
119+
try {
120+
const claudeConfig = pluginConfigToClaudeConfig(config)
120121

121-
allServers.push({
122-
name,
123-
source: "plugin",
124-
scope: "plugin",
125-
precedence: SOURCE_PRECEDENCE.plugin,
126-
transport: inferTransport(claudeConfig),
127-
config: claudeConfig,
128-
contextName: "plugin",
129-
})
122+
allServers.push({
123+
name,
124+
source: "plugin",
125+
scope: "plugin",
126+
precedence: SOURCE_PRECEDENCE.plugin,
127+
transport: inferTransport(claudeConfig),
128+
config: claudeConfig,
129+
contextName: "plugin",
130+
})
131+
} catch {
132+
// Skip plugin servers with invalid config (missing command/url)
133+
}
130134
}
131135

132136
allServers.push(...fromSkills(input.skills ?? []))

0 commit comments

Comments
 (0)