feat: complete Sprint 4.1 MCP registry adoption#21
feat: complete Sprint 4.1 MCP registry adoption#21codewithkenzo wants to merge 5 commits intomainfrom
Conversation
|
@greptileai review\n\nSprint 4.1 registry adoption ready for review. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/tools/mcp-query/tools.ts
Outdated
| const pluginServers = options.loadPluginMcpServers | ||
| ? await options.loadPluginMcpServers() | ||
| : {} |
There was a problem hiding this comment.
Unhandled plugin load failure
defaultLoadRegistry awaits loadPluginMcpServers() without a try/catch. If plugin discovery throws (e.g., bad plugin config / missing plugin file), mcp_query fails entirely even in include_operations: false mode. Consider catching and surfacing a non-fatal error (e.g., omit plugin servers + include an error string in the response) so querying other sources still works.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/mcp-query/tools.ts
Line: 84:86
Comment:
**Unhandled plugin load failure**
`defaultLoadRegistry` awaits `loadPluginMcpServers()` without a try/catch. If plugin discovery throws (e.g., bad plugin config / missing plugin file), `mcp_query` fails entirely even in `include_operations: false` mode. Consider catching and surfacing a non-fatal error (e.g., omit plugin servers + include an error string in the response) so querying other sources still works.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/features/mcp-registry/service.ts
Line: 39:41
Comment:
**Plugin local config crash**
`pluginConfigToClaudeConfig` assumes `config.command` exists (`config.command.length`), but `McpServerConfig` for non-remote plugin servers can be missing/undefined (and you already treat missing url as invalid). In that case this will throw `Cannot read properties of undefined (reading 'length')` while building the registry (startup + `mcp_query`), rather than producing a controlled error. Guard `config.command` before reading `.length` and emit a consistent validation error.
How can I resolve this? If you propose a fix, please make it concise. |
src/cli/doctor/checks/mcp.ts
Outdated
| return filterMcpRegistryServers(registry, "custom").map((server) => ({ | ||
| id: server.name, | ||
| type: "user" as const, | ||
| enabled: true, | ||
| valid: true, | ||
| })) |
There was a problem hiding this comment.
Always returns valid: true - invalid MCP configs won't be detected
The old implementation validated each server config and set valid: false with error messages for broken configs. Now all servers are marked valid, so checkUserMcpServers can never warn about configuration issues. Test at mcp.test.ts:89-101 expects this validation but will fail.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/doctor/checks/mcp.ts
Line: 27:32
Comment:
Always returns `valid: true` - invalid MCP configs won't be detected
The old implementation validated each server config and set `valid: false` with error messages for broken configs. Now all servers are marked valid, so `checkUserMcpServers` can never warn about configuration issues. Test at `mcp.test.ts:89-101` expects this validation but will fail.
How can I resolve this? If you propose a fix, please make it concise.| config.mcp = { | ||
| ...(config.mcp as Record<string, unknown>), | ||
| ...createBuiltinMcps(pluginConfig.disabled_mcps), | ||
| ...mcpResult.servers, | ||
| ...pluginComponents.mcpServers, | ||
| ...toRuntimeMcpServerMap(filterMcpRegistryServers(mcpRegistry, "all")), | ||
| }; |
There was a problem hiding this comment.
No error handling for toRuntimeMcpServerMap
transformMcpServer throws on invalid configs (missing url for http, missing command for stdio). If any server in the registry has bad config, this will crash plugin initialization. Wrap in try-catch and log + skip invalid servers instead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin-handlers/config-handler.ts
Line: 418:421
Comment:
No error handling for `toRuntimeMcpServerMap`
`transformMcpServer` throws on invalid configs (missing url for http, missing command for stdio). If any server in the registry has bad config, this will crash plugin initialization. Wrap in try-catch and log + skip invalid servers instead.
How can I resolve this? If you propose a fix, please make it concise.| export function toRuntimeMcpServerMap( | ||
| servers: McpRegistryServerDescriptor[] | ||
| ): Record<string, McpServerConfig> { | ||
| const map: Record<string, McpServerConfig> = {} | ||
|
|
||
| for (const server of servers) { | ||
| map[server.name] = transformMcpServer(server.name, server.config) | ||
| } | ||
|
|
||
| return map | ||
| } |
There was a problem hiding this comment.
transformMcpServer can throw - consider error handling
Called from config-handler critical path. If any server descriptor has invalid config, this throws and breaks initialization. Consider catching per-server and returning a partial map with errors logged.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/mcp-registry/service.ts
Line: 197:207
Comment:
`transformMcpServer` can throw - consider error handling
Called from config-handler critical path. If any server descriptor has invalid config, this throws and breaks initialization. Consider catching per-server and returning a partial map with errors logged.
How can I resolve this? If you propose a fix, please make it concise.
|
Closing: duplicate of #22. This PR incorrectly targets main instead of dev. |
src/cli/doctor/checks/mcp.ts
Outdated
| return filterMcpRegistryServers(registry, "custom").map((server) => ({ | ||
| id: server.name, | ||
| type: "user" as const, | ||
| enabled: true, | ||
| valid: true, | ||
| })) |
There was a problem hiding this comment.
always hardcoded to valid: true - validation was removed
old implementation checked typeof config === "object" && config !== null, now registry-based approach doesn't validate config structure so test at mcp.test.ts:89-101 will fail
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/doctor/checks/mcp.ts
Line: 27:32
Comment:
always hardcoded to `valid: true` - validation was removed
old implementation checked `typeof config === "object" && config !== null`, now registry-based approach doesn't validate config structure so test at mcp.test.ts:89-101 will fail
How can I resolve this? If you propose a fix, please make it concise.| if (!config.command) { | ||
| throw new Error("Invalid plugin MCP local config: missing command") | ||
| } |
There was a problem hiding this comment.
guard added but empty command array still throws on line 43
should also check array length here before accessing command[0] on line 49
| if (!config.command) { | |
| throw new Error("Invalid plugin MCP local config: missing command") | |
| } | |
| if (!config.command || config.command.length === 0) { | |
| throw new Error("Invalid plugin MCP local config: missing or empty command") | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/mcp-registry/service.ts
Line: 39:41
Comment:
guard added but empty `command` array still throws on line 43
should also check array length here before accessing `command[0]` on line 49
```suggestion
if (!config.command || config.command.length === 0) {
throw new Error("Invalid plugin MCP local config: missing or empty command")
}
```
How can I resolve this? If you propose a fix, please make it concise.| const mcpRegistry = createMcpRegistry({ | ||
| builtinServers: createBuiltinMcps(pluginConfig.disabled_mcps), | ||
| customServers: customRawServers, | ||
| pluginServers: pluginComponents.mcpServers, | ||
| }); |
There was a problem hiding this comment.
registry creation can throw if pluginServers contains invalid configs
pluginConfigToClaudeConfig (called during registry creation) throws on missing command/url. wrap in try-catch and log errors instead of crashing initialization
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin-handlers/config-handler.ts
Line: 402:406
Comment:
registry creation can throw if `pluginServers` contains invalid configs
`pluginConfigToClaudeConfig` (called during registry creation) throws on missing `command`/`url`. wrap in try-catch and log errors instead of crashing initialization
How can I resolve this? If you propose a fix, please make it concise.| config.mcp = { | ||
| ...(config.mcp as Record<string, unknown>), | ||
| ...createBuiltinMcps(pluginConfig.disabled_mcps), | ||
| ...mcpResult.servers, | ||
| ...pluginComponents.mcpServers, | ||
| ...toRuntimeMcpServerMap(filterMcpRegistryServers(mcpRegistry, "all")), | ||
| }; |
There was a problem hiding this comment.
toRuntimeMcpServerMap calls transformMcpServer which throws on invalid configs (missing url for http, missing command for stdio)
if any server descriptor has bad config, this crashes initialization. wrap in try-catch and skip invalid servers
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin-handlers/config-handler.ts
Line: 418:421
Comment:
`toRuntimeMcpServerMap` calls `transformMcpServer` which throws on invalid configs (missing `url` for http, missing `command` for stdio)
if any server descriptor has bad config, this crashes initialization. wrap in try-catch and skip invalid servers
How can I resolve this? If you propose a fix, please make it concise.
src/tools/mcp-query/tools.ts
Outdated
| const pluginServers = options.loadPluginMcpServers | ||
| ? await options.loadPluginMcpServers() | ||
| : {} |
There was a problem hiding this comment.
no try-catch around loadPluginMcpServers()
if plugin discovery throws (bad plugin config / missing file), mcp_query fails entirely even with include_operations: false. catch and surface non-fatal error so querying other sources still works
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/mcp-query/tools.ts
Line: 84:86
Comment:
no try-catch around `loadPluginMcpServers()`
if plugin discovery throws (bad plugin config / missing file), `mcp_query` fails entirely even with `include_operations: false`. catch and surface non-fatal error so querying other sources still works
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
if any plugin server has invalid config, registry creation fails and crashes the caller. consider catching per-server and logging + skipping invalid entries Prompt To Fix With AIThis is a comment left during a code review.
Path: src/features/mcp-registry/service.ts
Line: 118:130
Comment:
`pluginConfigToClaudeConfig` can throw on lines 29, 40, 44
if any plugin server has invalid config, registry creation fails and crashes the caller. consider catching per-server and logging + skipping invalid entries
How can I resolve this? If you propose a fix, please make it concise. |
src/cli/doctor/checks/mcp.ts
Outdated
| if (!config.command) { | ||
| valid = false | ||
| error = "Missing required field: command" | ||
| } |
There was a problem hiding this comment.
check if config.command is empty string or empty array - !config.command passes for empty array [] which would cause transformMcpServer to fail later
| if (!config.command) { | |
| valid = false | |
| error = "Missing required field: command" | |
| } | |
| if (!config.command || (Array.isArray(config.command) && config.command.length === 0) || config.command === "") { | |
| valid = false | |
| error = "Missing required field: command" | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/doctor/checks/mcp.ts
Line: 36:39
Comment:
check if `config.command` is empty string or empty array - `!config.command` passes for empty array `[]` which would cause `transformMcpServer` to fail later
```suggestion
if (!config.command || (Array.isArray(config.command) && config.command.length === 0) || config.command === "") {
valid = false
error = "Missing required field: command"
}
```
How can I resolve this? If you propose a fix, please make it concise.
| export function toRuntimeMcpServerMap( | ||
| servers: McpRegistryServerDescriptor[] | ||
| ): Record<string, McpServerConfig> { | ||
| const map: Record<string, McpServerConfig> = {} | ||
|
|
||
| for (const server of servers) { | ||
| try { | ||
| map[server.name] = transformMcpServer(server.name, server.config) | ||
| } catch { | ||
| // Skip servers with invalid configs during runtime map construction | ||
| } | ||
| } | ||
|
|
||
| return map |
There was a problem hiding this comment.
Type mismatch in map
toRuntimeMcpServerMap is declared to return Record<string, McpServerConfig>, but it assigns transformMcpServer(...) (which returns a Claude Code runtime server config, not the raw McpServerConfig type used for plugin configs). This makes the return type incorrect and can hide real incompatibilities at call sites (e.g. config-handler.ts spreads this into config.mcp). Update the return type to match transformMcpServer’s actual return type (or stop using transformMcpServer here) so type checking reflects what’s actually produced.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/mcp-registry/service.ts
Line: 201:214
Comment:
**Type mismatch in map**
`toRuntimeMcpServerMap` is declared to return `Record<string, McpServerConfig>`, but it assigns `transformMcpServer(...)` (which returns a Claude Code runtime server config, not the raw `McpServerConfig` type used for plugin configs). This makes the return type incorrect and can hide real incompatibilities at call sites (e.g. `config-handler.ts` spreads this into `config.mcp`). Update the return type to match `transformMcpServer`’s actual return type (or stop using `transformMcpServer` here) so type checking reflects what’s actually produced.
How can I resolve this? If you propose a fix, please make it concise.| if (args.scope) { | ||
| servers = servers.filter((server) => server.scope === args.scope) | ||
| servers = servers.filter((server) => server.source === "custom" ? server.scope === args.scope : true) | ||
| } | ||
|
|
||
| const candidateServers = servers.length | ||
| servers = servers.slice(0, limit) | ||
|
|
There was a problem hiding this comment.
Scope filter breaks totals
The new scope filter only applies to custom servers (servers.filter(server => server.source === "custom" ? server.scope === args.scope : true)), but args.scope is documented as a general “Filter by MCP config scope”. When querying source: all (or plugin/skill), this will still return non-custom servers even though a scope filter was requested, and candidate_servers/returned_servers will include those unfiltered entries. Either apply the scope filter consistently across all sources (using the appropriate scope values for each) or reject scope for non-custom sources with a clear error.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/mcp-query/tools.ts
Line: 135:141
Comment:
**Scope filter breaks totals**
The new scope filter only applies to `custom` servers (`servers.filter(server => server.source === "custom" ? server.scope === args.scope : true)`), but `args.scope` is documented as a general “Filter by MCP config scope”. When querying `source: all` (or `plugin`/`skill`), this will still return non-custom servers even though a scope filter was requested, and `candidate_servers`/`returned_servers` will include those unfiltered entries. Either apply the scope filter consistently across all sources (using the appropriate scope values for each) or reject `scope` for non-custom sources with a clear error.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
After slicing Prompt To Fix With AIThis is a comment left during a code review.
Path: src/tools/mcp-query/tools.ts
Line: 139:272
Comment:
**Incorrect total_servers count**
After slicing `servers` to `limit` (line ~140), `total_servers` is returned as `results.length` (line ~269), which is now “results after limiting and query filtering”, not the total available matches. This makes `total_servers` misleading (and can be smaller than `returned_servers` semantics suggest), especially when `include_operations: true` and query filtering continues inside the loop. Consider defining totals explicitly (e.g., total_matching_servers pre-limit vs returned_servers post-limit) and wiring `total_servers` to the pre-limit count to match the field name.
How can I resolve this? If you propose a fix, please make it concise. |
…luginConfigToClaudeConfig
…, scope filter, early limit - Restore per-server config validation in getUserMcpInfo() (was hardcoded valid: true) - Make toRuntimeMcpServerMap() resilient with try-catch per server - Wrap createMcpRegistry() in try-catch with fallback to builtin+custom only - Wrap loadPluginMcpServers() in try-catch for graceful degradation - Scope filter now only applies to custom sources (builtin/plugin/skill not silently dropped) - Apply limit before connecting to servers (not after)
…y 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
codewithkenzo
force-pushed
the
feat/sprint4-1-mcp-registry-adoption
branch
from
01de010 to
12a6b3f
Compare
|
Superseded by #23 which includes all Sprint 4.1 + 4.2 commits. |
| export function toRuntimeMcpServerMap( | ||
| servers: McpRegistryServerDescriptor[] | ||
| ): Record<string, McpServerConfig> { | ||
| const map: Record<string, McpServerConfig> = {} | ||
|
|
||
| for (const server of servers) { | ||
| try { | ||
| map[server.name] = transformMcpServer(server.name, server.config) | ||
| } catch { | ||
| // Skip servers with invalid configs during runtime map construction | ||
| } | ||
| } | ||
|
|
||
| return map |
There was a problem hiding this comment.
Runtime MCP type mismatch
toRuntimeMcpServerMap is typed as returning Record<string, McpServerConfig> but it populates values with transformMcpServer(server.name, server.config). Here server.config is a ClaudeCode runtime server shape (ClaudeCodeMcpServer), while McpServerConfig is the raw loader shape ({type: local|remote, command/url, ...}). This map is spread into config.mcp in src/plugin-handlers/config-handler.ts:428-433, so config.mcp will end up in the wrong shape for downstream MCP wiring.
Fix: either (a) change the function to return Record<string, ClaudeCodeMcpServer> and skip transformMcpServer entirely, or (b) keep transformMcpServer but don’t use the result as config.mcp (use it only where raw configs are expected).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/mcp-registry/service.ts
Line: 201:214
Comment:
**Runtime MCP type mismatch**
`toRuntimeMcpServerMap` is typed as returning `Record<string, McpServerConfig>` but it populates values with `transformMcpServer(server.name, server.config)`. Here `server.config` is a *ClaudeCode runtime* server shape (`ClaudeCodeMcpServer`), while `McpServerConfig` is the *raw loader* shape (`{type: local|remote, command/url, ...}`). This map is spread into `config.mcp` in `src/plugin-handlers/config-handler.ts:428-433`, so `config.mcp` will end up in the wrong shape for downstream MCP wiring.
Fix: either (a) change the function to return `Record<string, ClaudeCodeMcpServer>` and skip `transformMcpServer` entirely, or (b) keep `transformMcpServer` but don’t use the result as `config.mcp` (use it only where raw configs are expected).
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Fix: apply the limit in one place (either slice Prompt To Fix With AIThis is a comment left during a code review.
Path: src/tools/mcp-query/tools.ts
Line: 121:140
Comment:
**Result limiting is inconsistent**
In `execute`, `candidate_servers` is captured before applying the limit, then `servers = servers.slice(0, limit)` is applied, but the later `limitedResults` variable was changed to `const limitedResults = results` (no-op). This leaves the implementation in an inconsistent state where the limiting logic is split and easy to regress (e.g., if `results` ever diverges from `servers` again).
Fix: apply the limit in one place (either slice `servers` and remove `limitedResults`, or keep `results.slice(0, limit)` and don’t slice `servers`), and ensure `candidate_servers`/`returned_servers`/`total_servers` reflect the chosen behavior.
How can I resolve this? If you propose a fix, please make it concise. |
1 participant
Summary
mcp_queryto load plugin MCP servers through registry-backed discovery while preserving custom-first behavior and clearer totals metadataVerification
bun run typecheckbun test src/features/mcp-registry/service.test.ts src/tools/mcp-query/tools.test.ts src/cli/doctor/checks/mcp.test.tsbun test src/features/claude-code-mcp-loader/loader.test.ts src/features/skill-mcp-manager/manager.test.ts src/features/mcp-registry/service.test.ts src/tools/mcp-query/tools.test.ts src/cli/doctor/checks/mcp.test.tsNotes
bun test src/plugin-handlers/config-handler.test.tsstill contains pre-existing category-default expectation failures unrelated to this change set.Greptile Overview
Greptile Summary
config.mcp.mcp_queryto discover plugin MCP servers via an injectedloadPluginMcpServerscallback while preserving registry-based filtering and totals metadata.Confidence Score: 3/5
toRuntimeMcpServerMapappears to write the wrong config shape intoconfig.mcp, which can break MCP startup at runtime. There is also an inconsistent limit/refactor state inmcp_querythat should be cleaned up to avoid metadata/behavior drift.Important Files Changed
Sequence Diagram
sequenceDiagram autonumber participant Doctor as doctor:mcp check participant Loader as claude-code-mcp-loader participant Registry as mcp-registry participant Query as tool:mcp_query participant PluginLoader as claude-code-plugin-loader Doctor->>Registry: createMcpRegistry(builtinServers) Doctor->>Registry: filterMcpRegistryServers("builtin") Doctor-->>Doctor: format builtin info Doctor->>Loader: loadRawMcpConfigs() Loader-->>Doctor: loadedServers Doctor->>Registry: createMcpRegistry(customServers) Doctor->>Registry: filterMcpRegistryServers("custom") Doctor-->>Doctor: validate command/url basics Query->>Loader: loadRawMcpConfigs() (if includeCustomMcp) Query->>PluginLoader: loadPluginMcpServers() (optional) PluginLoader-->>Query: pluginServers map Query->>Registry: createMcpRegistry(builtin+custom+plugin+skills) Query->>Registry: filterMcpRegistryServers(source) Query-->>Query: apply scope filter + limit Query-->>Query: optionally fetch operations via manager PluginLoader->>Registry: createMcpRegistry(builtin+custom+pluginServers) PluginLoader->>Registry: filterMcpRegistryServers("all") PluginLoader->>Registry: toRuntimeMcpServerMap(effectiveServers) Registry-->>PluginLoader: runtime mcp map