Skip to content

feat(mcp): unified MCP registry, source-aware mcp_query, and doctor adoption#22

Closed
codewithkenzo wants to merge 25 commits intodevfrom
feat/sprint4-1-mcp-registry-adoption
Closed

feat(mcp): unified MCP registry, source-aware mcp_query, and doctor adoption#22
codewithkenzo wants to merge 25 commits intodevfrom
feat/sprint4-1-mcp-registry-adoption

Conversation

@codewithkenzo
Copy link
Owner

@codewithkenzo codewithkenzo commented Feb 7, 2026

Summary

  • Introduces a unified MCP registry (src/features/mcp-registry/) that merges builtin, custom (.mcp.json), plugin, and skill MCP servers into a single precedence-resolved map
  • Adds mcp_query tool for discovering and querying custom MCP servers with source filtering
  • Migrates doctor MCP health checks to use the registry instead of ad-hoc config parsing
  • Adopts registry in runtime MCP server map construction (src/index.ts)

Key Changes

New: MCP Registry (src/features/mcp-registry/)

  • service.ts: createMcpRegistry() with source precedence (builtin < custom < plugin < skill)
  • Collision resolution, filterMcpRegistryServers(), toRuntimeMcpServerMap()

New: mcp_query Tool (src/tools/mcp-query/)

  • Source filtering: all | builtin | custom | plugin | skill
  • Integrates with shared MCP client manager for tool listing

Updated: Doctor MCP Checks

  • Uses registry for health validation, removes duplicated config-parsing

Bugfix: pluginConfigToClaudeConfig null guard

  • Guards config.command before .length access to prevent crash on malformed configs

Testing

  • bun run typecheck
  • bun test src/features/mcp-registry/
  • bun test src/tools/mcp-query/
  • bun test src/cli/doctor/checks/mcp.test.ts

Follow-up

Foundation for Sprint 4.2 (lazy tool loading + tool port matrix).

Greptile Overview

Greptile Summary

This PR introduces a unified MCP registry to centralize builtin, custom, plugin, and skill MCP server management with source precedence resolution. The mcp_query tool enables discovery and introspection of MCP servers with source filtering, and doctor now uses the registry for health checks instead of ad-hoc config parsing.

Key accomplishments:

  • Registry correctly implements precedence (builtin < custom < plugin < skill) and collision tracking
  • mcp_query tool provides structured server discovery with operation listing
  • Doctor health checks successfully migrated to use registry
  • Bugfix: pluginConfigToClaudeConfig now validates command.length before access

Critical issues found:

  • Skills omitted from runtime registry (config-handler.ts:405-409): createMcpRegistry call excludes skills, causing skill MCP servers to never load in runtime config.mcp and preventing collision detection between skills and other sources
  • Silent MCP transform failures (service.ts:206-212): toRuntimeMcpServerMap swallows all errors, making misconfigured MCPs "disappear" with no trace
  • Scope filter logic broken (tools.ts:135-137): When source: "all" + scope are combined, builtin/plugin/skill entries are silently excluded because filter only checks server.source === "custom"
  • Limit doesn't prevent connection overhead (tools.ts:139-141): Limit applied to server list but operations are fetched for all servers before slicing results

Previous thread issues confirmed:
All four issues from previous review threads remain valid and are verified in this review.

Confidence Score: 2/5

  • Critical bugs in config-handler (skill MCPs never load) and mcp_query (broken scope filter) will cause production failures
  • Score reflects two critical logic bugs that break core functionality: (1) skills excluded from runtime registry means skill MCP servers won't load at all, and (2) scope filter in mcp_query silently drops non-custom sources. Both will cause user-facing failures. Additionally, silent error swallowing makes debugging impossible when MCPs fail to load.
  • src/plugin-handlers/config-handler.ts (line 405-409: add skills to registry), src/tools/mcp-query/tools.ts (lines 135-141: fix scope filter + limit), src/features/mcp-registry/service.ts (line 206-212: log transform errors)

Important Files Changed

Filename Overview
src/features/mcp-registry/service.ts Core registry logic solid, but silent error drops (line 209) and missing skill integration in config-handler make servers "disappear" without traces
src/tools/mcp-query/tools.ts Tool implementation functional but scope filter (line 136) silently excludes non-custom sources, and limit doesn't prevent connection overhead
src/cli/doctor/checks/mcp.ts Successfully migrated to registry, validation logic preserved for custom servers (lines 28-45)
src/plugin-handlers/config-handler.ts Critical: skills excluded from registry (line 405-409), agent demotion incomplete (line 332-345) — demoted agents still callable
src/index.ts MCP query tool properly integrated with registry loader and skill manager

Sequence Diagram

sequenceDiagram
    participant Plugin as Plugin/Skill Loader
    participant ConfigHandler as Config Handler
    participant Registry as MCP Registry
    participant McpQuery as mcp_query Tool
    participant Doctor as Doctor Check
    participant Runtime as Runtime MCP Map

    Note over Plugin,ConfigHandler: Plugin Initialization
    Plugin->>ConfigHandler: loadAllPluginComponents()
    ConfigHandler->>ConfigHandler: Load builtin/custom/plugin MCP configs
    
    Note over ConfigHandler,Registry: Registry Creation
    ConfigHandler->>Registry: createMcpRegistry({builtin, custom, plugin})
    Note right of ConfigHandler: ❌ Skills NOT passed here
    Registry->>Registry: Apply precedence (builtin<custom<plugin<skill)
    Registry->>Registry: Detect collisions
    Registry-->>ConfigHandler: McpRegistryResult

    ConfigHandler->>Registry: filterMcpRegistryServers(registry, "all")
    Registry-->>ConfigHandler: Effective servers
    ConfigHandler->>Registry: toRuntimeMcpServerMap(servers)
    Registry->>Registry: Transform configs
    Note right of Registry: ❌ Errors silently swallowed
    Registry-->>ConfigHandler: Runtime map
    ConfigHandler->>Runtime: config.mcp = runtimeMap

    Note over McpQuery,Registry: mcp_query Tool Flow
    McpQuery->>Registry: createMcpRegistry({builtin, custom, plugin, skills})
    Registry-->>McpQuery: Registry with all sources
    McpQuery->>Registry: filterMcpRegistryServers(registry, source)
    Registry-->>McpQuery: Filtered servers
    Note right of McpQuery: ❌ scope filter excludes non-custom
    McpQuery->>McpQuery: Apply limit to server list
    Note right of McpQuery: ❌ Still connects to all before limit
    McpQuery->>McpQuery: Connect & list operations
    McpQuery-->>McpQuery: Return results

    Note over Doctor,Registry: Doctor Health Check
    Doctor->>Registry: createMcpRegistry({builtin})
    Registry-->>Doctor: Builtin servers
    Doctor->>Registry: createMcpRegistry({custom})
    Registry-->>Doctor: Custom servers
    Doctor->>Doctor: Validate custom configs
    Doctor-->>Doctor: Report health status
Loading

Ubuntu and others added 23 commits January 31, 2026 17:51
- state.test.ts: enable getMainSessionID undefined test (was skipped due to state pollution)
- ralph-loop/index.test.ts: enable timeout protection test (reduced delay from 10s to 200ms)
- state.ts: add clearMainSession export for test isolation
- Add console.error logging to stderr read error handler
- Add console.error logging to JSON parse error handler
- Add debug logging for /proc/version read failure

Closes: audit-2026-02-01
- Define proper TuiClient interface instead of 'as any' type assertion
- Replace empty catch blocks with error logging
- Improve type safety and debugging visibility
- Add MessageInfo and ThinkingPart interfaces
- Replace as any casts with proper type assertions
- Improves TypeScript strictness and code maintainability
Rebased Sprint 1 (agent compression 23→8) and Sprint 2 (skill system
modernization) onto current main branch, integrating memory-persistence
hooks, research tools, type safety fixes, and other main-branch additions.

Key changes:
- Agent compression: 23 agents → 8 with LEGACY_TO_MUSASHI_NAME compat map
- Skill system: split 1204-line skills.ts into per-skill files
- BrowserAutomationProvider config + agent-browser/dev-browser skills
- Atlas hook (renamed from sisyphus-orchestrator) with BackgroundManager
- Per-provider skill cache in skill-content.ts
- Added case-insensitive utilities and model availability modules
@github-actions
Copy link

github-actions bot commented Feb 7, 2026

Thank you for your contribution! Before we can merge this PR, we need you to sign our Contributor License Agreement (CLA).

To sign the CLA, please comment on this PR with:

I have read the CLA Document and I hereby sign the CLA

This is a one-time requirement. Once signed, all your future contributions will be automatically accepted.


I have read the CLA Document and I hereby sign the CLA


1 out of 3 committers have signed the CLA.
✅ (codewithkenzo)[https://github.com/codewithkenzo]
@ubuntu
@Kenzo
Ubuntu, Kenzo seem not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10 files reviewed, 5 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +5 to 15
import { createMcpRegistry, filterMcpRegistryServers } from "../../../features/mcp-registry"

const BUILTIN_MCP_SERVERS = ["context7", "grep_app"]
export async function getBuiltinMcpInfo(): Promise<McpServerInfo[]> {
const registry = createMcpRegistry({
builtinServers: createBuiltinMcps(),
})

const MCP_CONFIG_PATHS = [
join(homedir(), ".claude", ".mcp.json"),
join(process.cwd(), ".mcp.json"),
join(process.cwd(), ".claude", ".mcp.json"),
]

interface McpConfig {
mcpServers?: Record<string, unknown>
}

function loadUserMcpConfig(): Record<string, unknown> {
const servers: Record<string, unknown> = {}

for (const configPath of MCP_CONFIG_PATHS) {
if (!existsSync(configPath)) continue

try {
const content = readFileSync(configPath, "utf-8")
const config = parseJsonc<McpConfig>(content)
if (config.mcpServers) {
Object.assign(servers, config.mcpServers)
}
} catch {
// intentionally empty - skip invalid configs
}
}

return servers
}

export function getBuiltinMcpInfo(): McpServerInfo[] {
return BUILTIN_MCP_SERVERS.map((id) => ({
id,
return filterMcpRegistryServers(registry, "builtin").map((server) => ({
id: server.name,
type: "builtin" as const,
enabled: true,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User MCP validity lost

getUserMcpInfo() now returns every custom MCP as { valid: true } and no longer attempts to validate/flag malformed entries. Previously, invalid mcpServers entries were surfaced as valid: false with an error. With this change, doctor can report a “healthy” config even when .mcp.json contains invalid shapes, which is a regression in the check’s purpose.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/doctor/checks/mcp.ts
Line: 5:15

Comment:
**User MCP validity lost**

`getUserMcpInfo()` now returns every custom MCP as `{ valid: true }` and no longer attempts to validate/flag malformed entries. Previously, invalid `mcpServers` entries were surfaced as `valid: false` with an error. With this change, `doctor` can report a “healthy” config even when `.mcp.json` contains invalid shapes, which is a regression in the check’s purpose.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines 329 to 347
@@ -282,10 +341,8 @@ export function createConfigHandler(deps: ConfigHandlerDeps) {
...projectAgents,
...pluginAgents,
...filteredConfigAgents,
build: { ...migratedBuild, mode: "subagent", hidden: true, disable: true },
build: { ...migratedBuild, mode: "subagent", hidden: true },
...(planDemoteConfig ? { plan: planDemoteConfig } : {}),
explore: { disable: true },
general: { disable: true },
};
} else {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Demoted agents still enabled

When replacePlan is true, the code used to demote/disable OpenCode’s plan agent via { hidden: true, disable: true }, but now sets only { mode: "subagent" } (and similarly removed disable: true for build). This makes the “demoted” agents still selectable/usable by OpenCode, which changes behavior and can lead to the defaults competing with your intended builtins (and the PR comment suggests you’re trying to prevent overwrites).

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin-handlers/config-handler.ts
Line: 329:347

Comment:
**Demoted agents still enabled**

When `replacePlan` is true, the code used to demote/disable OpenCode’s `plan` agent via `{ hidden: true, disable: true }`, but now sets only `{ mode: "subagent" }` (and similarly removed `disable: true` for `build`). This makes the “demoted” agents still selectable/usable by OpenCode, which changes behavior and can lead to the defaults competing with your intended builtins (and the PR comment suggests you’re trying to prevent overwrites).

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines 394 to 413
delegate_task: "deny",
};

const mcpResult = (pluginConfig.claude_code?.mcp ?? true)
? await loadMcpConfigs()
: { servers: {} };
const includeCustomMcp = pluginConfig.claude_code?.mcp ?? true;
const customRawServers = includeCustomMcp
? (await loadRawMcpConfigs()).loadedServers
: [];

const mcpRegistry = createMcpRegistry({
builtinServers: createBuiltinMcps(pluginConfig.disabled_mcps),
customServers: customRawServers,
pluginServers: pluginComponents.mcpServers,
});

if (mcpRegistry.collisions.length > 0) {
log("MCP registry collisions detected", {
collisions: mcpRegistry.collisions.map((entry) => ({
name: entry.name,
winner: `${entry.winner.source}:${entry.winner.scope}`,
overridden: entry.overridden.map((server) => `${server.source}:${server.scope}`),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills omitted from registry

The MCP registry in createConfigHandler is built with builtin/custom/plugin servers, but skills are never passed to createMcpRegistry(...). If a skill defines mcpConfig with a name that collides with a builtin/custom/plugin server, collisions won’t be detected/logged here, and config.mcp will not include the skill servers at all (despite the registry’s stated goal to unify builtin/custom/plugin/skill).

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin-handlers/config-handler.ts
Line: 394:413

Comment:
**Skills omitted from registry**

The MCP registry in `createConfigHandler` is built with builtin/custom/plugin servers, but `skills` are never passed to `createMcpRegistry(...)`. If a skill defines `mcpConfig` with a name that collides with a builtin/custom/plugin server, collisions won’t be detected/logged here, and `config.mcp` will not include the skill servers at all (despite the registry’s stated goal to unify builtin/custom/plugin/skill).

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +92 to +117
skills: options.getLoadedSkills?.() ?? [],
})
}

export function createMcpQueryTool(options: McpQueryToolOptions): ToolDefinition {
const loadRegistry = options.loadRegistry ?? (() => defaultLoadRegistry(options))
const includeCustomMcp = options.includeCustomMcp ?? true

return tool({
description: MCP_QUERY_DESCRIPTION,
args: {
query: tool.schema.string().optional().describe("Case-insensitive filter over server metadata and operation names/descriptions"),
server_name: tool.schema.string().optional().describe("Exact server name filter"),
source: tool.schema.enum(["custom", "skill", "builtin", "plugin", "all"]).optional().describe("MCP source to query (default: custom)"),
scope: tool.schema.enum(["user", "project", "local"]).optional().describe("Filter by MCP config scope"),
include_operations: tool.schema.boolean().optional().describe("When true (default), connect and list tools/resources/prompts"),
limit: tool.schema.number().int().positive().optional().describe("Maximum servers returned (default 20, max 200)"),
},
async execute(args: McpQueryArgs) {
const source = args.source ?? "custom"

if (!includeCustomMcp && source === "custom") {
throw new Error(
"Custom MCP loading is disabled by `claude_code.mcp=false`. Enable it to use `mcp_query`."
)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scope filter excludes builtin/plugin

scope is defined as user|project|local, but registry descriptors can have scope: "builtin" | "plugin" | SkillScope. When a caller combines source: "all" with scope, the filter server.scope === args.scope will silently drop all builtin/plugin/skill entries, which is surprising for an "all" query and makes it easy to get empty results even when servers exist.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/mcp-query/tools.ts
Line: 92:117

Comment:
**Scope filter excludes builtin/plugin**

`scope` is defined as `user|project|local`, but registry descriptors can have `scope: "builtin" | "plugin" | SkillScope`. When a caller combines `source: "all"` with `scope`, the filter `server.scope === args.scope` will silently drop all builtin/plugin/skill entries, which is surprising for an "all" query and makes it easy to get empty results even when servers exist.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +165 to +203
contextName: server.contextName ?? MCP_QUERY_CONTEXT_NAME,
sessionID: options.getSessionID(),
}

const context: McpServerContext = {
config: server.config,
contextName: server.contextName ?? MCP_QUERY_CONTEXT_NAME,
}

const [toolsResult, resourcesResult, promptsResult] = await Promise.allSettled([
options.manager.listTools(info, context),
options.manager.listResources(info, context),
options.manager.listPrompts(info, context),
])

const errors: string[] = []
const operations: QueryOperation[] = []

if (toolsResult.status === "fulfilled") {
operations.push(
...toolsResult.value.map((entry) => ({
type: "tool" as const,
name: entry.name,
description: entry.description,
}))
)
} else {
errors.push(`tools: ${toolsResult.reason instanceof Error ? toolsResult.reason.message : String(toolsResult.reason)}`)
}

if (resourcesResult.status === "fulfilled") {
operations.push(
...resourcesResult.value.map((entry) => ({
type: "resource" as const,
name: getResourceName(entry),
description: entry.description,
}))
)
} else {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Connections ignore limit

The tool applies limit only after iterating all candidate servers. With include_operations=true (default), it will still connect/list tools/resources/prompts for every matching server before slicing the results. This defeats the purpose of limit and can cause unnecessary connections/process spawns when many servers match.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/mcp-query/tools.ts
Line: 165:203

Comment:
**Connections ignore limit**

The tool applies `limit` only after iterating all candidate servers. With `include_operations=true` (default), it will still connect/list tools/resources/prompts for every matching server before slicing the results. This defeats the purpose of `limit` and can cause unnecessary connections/process spawns when many servers match.

How can I resolve this? If you propose a fix, please make it concise.

…, 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)
@codewithkenzo
Copy link
Owner Author

Greptile Findings — Addressed & Intentional

Fixed (this push)

All 6 actionable findings have been addressed in commit 4253a75:

  1. ✅ Restored per-server config validation in getUserMcpInfo()
  2. toRuntimeMcpServerMap() resilient with try-catch per server
  3. createMcpRegistry() wrapped in try-catch with fallback
  4. loadPluginMcpServers() wrapped in try-catch
  5. ✅ Scope filter only applies to custom sources
  6. ✅ Limit applied before connecting to servers

Intentional (not bugs)

  • "Demoted agents still enabled" — By design. The plan agent is demoted to subagent mode (invoked via delegate_task only), not fully disabled. It still needs to be registered.
  • "Skills omitted from registry" — By design. Skills are loaded separately in config-handler.ts and passed to mcp_query at runtime via the getLoadedSkills callback. They don't go through the MCP registry because they have their own lifecycle.

@codewithkenzo
Copy link
Owner Author

@greptileai review

@codewithkenzo
Copy link
Owner Author

All 5 findings from this review cycle were already addressed in commit 4253a750 (pushed before the review completed — timing race):

  1. User MCP validity lost — Fixed: getUserMcpInfo() now validates per-server config (checks command/url per server type)
  2. Demoted agents still enabled — Intentional by design (subagent mode, not disabled)
  3. Skills omitted from registry — Intentional by design (skills have separate lifecycle via getLoadedSkills callback)
  4. Scope filter excludes builtin/plugin — Fixed: scope filter now only applies to custom source servers
  5. Connections ignore limit — Fixed: servers.slice(0, limit) applied before connecting

@greptileai review

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +403 to +412
let mcpRegistry: McpRegistryResult;
try {
mcpRegistry = createMcpRegistry({
builtinServers: createBuiltinMcps(pluginConfig.disabled_mcps),
customServers: customRawServers,
pluginServers: pluginComponents.mcpServers,
});
} catch (err) {
log("MCP registry creation failed, falling back to builtin+custom only", {
error: err instanceof Error ? err.message : String(err),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skill MCPs dropped

createMcpRegistry({ builtinServers, customServers, pluginServers }) omits skills, but config.mcp is then built from filterMcpRegistryServers(mcpRegistry, "all"). That means any MCP servers defined via LoadedSkill.mcpConfig are never included in runtime config.mcp, and collisions involving skill servers also won’t be detected/logged here. Pass the loaded skills into the registry call so skill MCPs participate in the unified precedence map.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin-handlers/config-handler.ts
Line: 403:412

Comment:
**Skill MCPs dropped**

`createMcpRegistry({ builtinServers, customServers, pluginServers })` omits `skills`, but `config.mcp` is then built from `filterMcpRegistryServers(mcpRegistry, "all")`. That means any MCP servers defined via `LoadedSkill.mcpConfig` are never included in runtime `config.mcp`, and collisions involving skill servers also won’t be detected/logged here. Pass the loaded skills into the registry call so skill MCPs participate in the unified precedence map.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +202 to +207
for (const server of servers) {
try {
map[server.name] = transformMcpServer(server.name, server.config)
} catch {
// Skip servers with invalid configs during runtime map construction
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silent invalid MCP drop

toRuntimeMcpServerMap() swallows all errors from transformMcpServer() and silently omits that server from the returned map. This makes a misconfigured MCP effectively “disappear” at runtime with no surfaced reason, which is hard to debug and can make registry collision output misleading (winner exists in registry but not in runtime map). At minimum, capture/report which server names failed transform (even if you keep skipping them).

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/mcp-registry/service.ts
Line: 202:207

Comment:
**Silent invalid MCP drop**

`toRuntimeMcpServerMap()` swallows all errors from `transformMcpServer()` and silently omits that server from the returned map. This makes a misconfigured MCP effectively “disappear” at runtime with no surfaced reason, which is hard to debug and can make registry collision output misleading (winner exists in registry but not in runtime map). At minimum, capture/report which server names failed transform (even if you keep skipping them).

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines 332 to 347
@@ -282,10 +342,8 @@ export function createConfigHandler(deps: ConfigHandlerDeps) {
...projectAgents,
...pluginAgents,
...filteredConfigAgents,
build: { ...migratedBuild, mode: "subagent", hidden: true, disable: true },
build: { ...migratedBuild, mode: "subagent", hidden: true },
...(planDemoteConfig ? { plan: planDemoteConfig } : {}),
explore: { disable: true },
general: { disable: true },
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Demoted agents still usable

When replacePlan is true, the default plan agent is now only set to { mode: "subagent" } (and build is { ... , mode: "subagent", hidden: true }), without disable: true. If OpenCode still allows invoking subagents directly, this changes behavior from “disabled” to “still callable/selectable”, which can cause the demoted defaults to compete with your intended builtins. If the goal is to fully prevent use, restore disable: true (or the equivalent supported flag) for these demotions.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin-handlers/config-handler.ts
Line: 332:347

Comment:
**Demoted agents still usable**

When `replacePlan` is true, the default `plan` agent is now only set to `{ mode: "subagent" }` (and `build` is `{ ... , mode: "subagent", hidden: true }`), without `disable: true`. If OpenCode still allows invoking subagents directly, this changes behavior from “disabled” to “still callable/selectable”, which can cause the demoted defaults to compete with your intended builtins. If the goal is to fully prevent use, restore `disable: true` (or the equivalent supported flag) for these demotions.

How can I resolve this? If you propose a fix, please make it concise.

…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
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +405 to +409
mcpRegistry = createMcpRegistry({
builtinServers: createBuiltinMcps(pluginConfig.disabled_mcps),
customServers: customRawServers,
pluginServers: pluginComponents.mcpServers,
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills missing from registry call — skill MCP servers won't participate in collision detection or runtime config.mcp. Pass mergedSkills (built on line 286) to createMcpRegistry.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugin-handlers/config-handler.ts
Line: 405:409

Comment:
Skills missing from registry call — skill MCP servers won't participate in collision detection or runtime `config.mcp`. Pass `mergedSkills` (built on line 286) to `createMcpRegistry`.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +135 to +137
if (args.scope) {
servers = servers.filter((server) => server.source === "custom" ? server.scope === args.scope : true)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scope filter only applies to source === "custom", silently dropping builtin/plugin/skill entries when source: "all" + scope are combined. Either remove scope from schema or apply it to all sources.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/mcp-query/tools.ts
Line: 135:137

Comment:
Scope filter only applies to `source === "custom"`, silently dropping builtin/plugin/skill entries when `source: "all"` + `scope` are combined. Either remove `scope` from schema or apply it to all sources.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +139 to +141
const candidateServers = servers.length
servers = servers.slice(0, limit)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

limit applied before MCP connections but operations fetched for all servers before the limit slice (line 144-263). With include_operations=true (default), this still spawns processes for every matching server. Move limit application earlier or apply to results after filtering.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/mcp-query/tools.ts
Line: 139:141

Comment:
`limit` applied before MCP connections but operations fetched for all servers before the limit slice (line 144-263). With `include_operations=true` (default), this still spawns processes for every matching server. Move limit application earlier or apply to results after filtering.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +206 to +212
for (const server of servers) {
try {
map[server.name] = transformMcpServer(server.name, server.config)
} catch {
// Skip servers with invalid configs during runtime map construction
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Transform errors swallowed — server disappears from runtime map with no error surfaced. Capture failed server names and log or return them so users know why their MCP didn't load.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/mcp-registry/service.ts
Line: 206:212

Comment:
Transform errors swallowed — server disappears from runtime map with no error surfaced. Capture failed server names and log or return them so users know why their MCP didn't load.

How can I resolve this? If you propose a fix, please make it concise.

@codewithkenzo
Copy link
Owner Author

Closing — duplicate of #21. Main is the canonical target branch, not dev.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant