From d1c828b356dc2f33aaa292bc433a9ac4482f5ef2 Mon Sep 17 00:00:00 2001 From: Mathews-Tom Date: Fri, 24 Jul 2026 06:24:17 +0530 Subject: [PATCH] fix(coding-agent): include disabled-discovered servers in /mcp autocomplete, harden config-read failures - collectMcpServerNames now unions userConfig.disabledServers so a third-party-discovered server that was /mcp disable'd (and thus dropped from mcpManager.getAllServerNames()) still tab-completes as an /mcp enable target. - collectMcpServerNames accepts an optional preloaded { userConfig, projectConfig }; #handleList() passes what it already read instead of re-reading both config files a second time. - /mcp's argument completer catches collectMcpServerNames failures (e.g. malformed config JSON) and returns null instead of letting the rejection escape un-awaited callers. --- .../controllers/mcp-command-controller.ts | 53 +++++++++++++------ .../src/slash-commands/builtin-registry.ts | 10 +++- .../test/mcp-name-autocomplete.test.ts | 42 +++++++++++++++ 3 files changed, 88 insertions(+), 17 deletions(-) diff --git a/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts b/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts index 99cf15cc3..bc71477fc 100644 --- a/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts +++ b/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts @@ -48,7 +48,13 @@ import { searchSmitheryRegistry, toConfigName, } from "../../mcp/smithery-registry"; -import type { MCPAuthChallenge, MCPAuthConfig, MCPServerConfig, MCPServerConnection } from "../../mcp/types"; +import type { + MCPAuthChallenge, + MCPAuthConfig, + MCPConfigFile, + MCPServerConfig, + MCPServerConnection, +} from "../../mcp/types"; import { shortenPath } from "../../tools/render-utils"; import { urlHyperlinkAlways } from "../../tui"; import { copyToClipboard } from "../../utils/clipboard"; @@ -248,28 +254,45 @@ type MCPSearchParsed = { /** * Collect the de-duplicated union of every MCP server name we know about: - * user config, project config, and any runtime-discovered servers not - * already present in either config (`ctx.mcpManager.getAllServerNames()` - * covers connections, pending connections, and discovered-but-not-yet- - * connected sources). Disabled servers stay in this list — disabling a - * server only flips its config `enabled` flag, it doesn't remove the config - * entry, so a disabled server is still a valid `/mcp enable ` target. + * user config, project config, disabled-server entries, and any + * runtime-discovered servers not already present in either config + * (`ctx.mcpManager.getAllServerNames()` covers connections, pending + * connections, and discovered-but-not-yet-connected sources). Disabled + * servers stay in this list — disabling a server only flips its config + * `enabled` flag, it doesn't remove the config entry, so a disabled server + * is still a valid `/mcp enable ` target. This also covers a + * discovered (non-config) server that was disabled: `loadAllMCPConfigs` + * filters it out of `getAllServerNames()`, but its name survives in + * `userConfig.disabledServers`. * * This is the single source of truth for "every known server name": both * `MCPCommandController#handleList()` and the `/mcp` slash-command argument * completer (server-name autocomplete for `enable`/`disable`/`test`/etc.) * call this instead of re-deriving the union themselves. + * + * `preloaded` lets a caller that already read both config files (e.g. + * `#handleList()`) pass them in and skip the redundant re-read. */ -export async function collectMcpServerNames(ctx: InteractiveModeContext): Promise { - const cwd = getProjectDir(); - const [userConfig, projectConfig] = await Promise.all([ - readMCPConfigFile(getMCPConfigPath("user", cwd)), - readMCPConfigFile(getMCPConfigPath("project", cwd)), - ]); +export async function collectMcpServerNames( + ctx: InteractiveModeContext, + preloaded?: { userConfig: MCPConfigFile; projectConfig: MCPConfigFile }, +): Promise { + let userConfig: MCPConfigFile; + let projectConfig: MCPConfigFile; + if (preloaded) { + ({ userConfig, projectConfig } = preloaded); + } else { + const cwd = getProjectDir(); + [userConfig, projectConfig] = await Promise.all([ + readMCPConfigFile(getMCPConfigPath("user", cwd)), + readMCPConfigFile(getMCPConfigPath("project", cwd)), + ]); + } const names = new Set([ ...Object.keys(userConfig.mcpServers ?? {}), ...Object.keys(projectConfig.mcpServers ?? {}), + ...(userConfig.disabledServers ?? []), ]); if (ctx.mcpManager) { for (const name of ctx.mcpManager.getAllServerNames()) { @@ -1304,10 +1327,10 @@ export class MCPCommandController { // Collect runtime-discovered servers not in config files const configServerNames = new Set([...userServers, ...projectServers]); - const disabledServerNames = new Set(await readDisabledServers(userPath)); + const disabledServerNames = new Set(userConfig.disabledServers ?? []); const discoveredServers: { name: string; source: SourceMeta }[] = []; if (this.ctx.mcpManager) { - const allServerNames = await collectMcpServerNames(this.ctx); + const allServerNames = await collectMcpServerNames(this.ctx, { userConfig, projectConfig }); for (const name of allServerNames) { if (configServerNames.has(name)) continue; if (disabledServerNames.has(name)) continue; diff --git a/packages/coding-agent/src/slash-commands/builtin-registry.ts b/packages/coding-agent/src/slash-commands/builtin-registry.ts index 8eac9d7fc..d2706408e 100644 --- a/packages/coding-agent/src/slash-commands/builtin-registry.ts +++ b/packages/coding-agent/src/slash-commands/builtin-registry.ts @@ -3,7 +3,7 @@ import * as os from "node:os"; import * as path from "node:path"; import { getOAuthProviders } from "@oh-my-pi/pi-ai/oauth"; import { type AutocompleteItem, Spacer } from "@oh-my-pi/pi-tui"; -import { APP_NAME, getProjectDir, setProjectDir } from "@oh-my-pi/pi-utils"; +import { APP_NAME, getProjectDir, logger, setProjectDir } from "@oh-my-pi/pi-utils"; import { reset as resetCapabilities } from "../capability"; import { COLLAB_GUEST_ALLOWED_COMMANDS, CollabGuestLink } from "../collab/guest"; import { CollabHost } from "../collab/host"; @@ -2478,7 +2478,13 @@ function buildMcpArgumentCompletions( if (!MCP_SERVER_NAME_SUBCOMMANDS.has(rawSubcommand.toLowerCase())) return null; const namePrefix = argumentPrefix.slice(spaceIndex + 1).toLowerCase(); - const serverNames = await collectMcpServerNames(runtime.ctx); + let serverNames: string[]; + try { + serverNames = await collectMcpServerNames(runtime.ctx); + } catch (error) { + logger.warn("MCP server-name autocomplete failed to read config", { error }); + return null; + } const matches: AutocompleteItem[] = serverNames .filter(name => name.toLowerCase().startsWith(namePrefix)) .map(name => ({ value: `${rawSubcommand} ${name} `, label: name })); diff --git a/packages/coding-agent/test/mcp-name-autocomplete.test.ts b/packages/coding-agent/test/mcp-name-autocomplete.test.ts index 51a21903a..27470db7f 100644 --- a/packages/coding-agent/test/mcp-name-autocomplete.test.ts +++ b/packages/coding-agent/test/mcp-name-autocomplete.test.ts @@ -87,6 +87,36 @@ describe("MCP server-name autocomplete", () => { expect(names).toEqual(["project-server", "runtime-discovered", "user-disabled", "user-enabled"]); }); + test("collectMcpServerNames includes a discovered server disabled via disabledServers, even once dropped from mcpManager", async () => { + // A third-party-discovered server that was `/mcp disable`d: recorded in the user + // config's top-level `disabledServers` list, absent from `mcpServers`, and no + // longer reported by the manager (loadAllMCPConfigs filters disabled sources out). + await Bun.write( + getMCPConfigPath("user", projectDir), + `${JSON.stringify({ mcpServers: {}, disabledServers: ["discovered-disabled"] }, null, 2)}\n`, + ); + await writeConfig("project", projectDir, {}); + const { ctx } = createFakeCtx([]); + + const names = await collectMcpServerNames(ctx); + + expect(names).toEqual(["discovered-disabled"]); + }); + + test("collectMcpServerNames accepts preloaded configs and skips re-reading them from disk", async () => { + await writeConfig("user", projectDir, { "user-server": { type: "stdio", command: "one" } }); + await writeConfig("project", projectDir, { "project-server": { type: "stdio", command: "two" } }); + const { ctx } = createFakeCtx(["runtime-discovered"]); + + const names = await collectMcpServerNames(ctx, { + userConfig: { mcpServers: { "override-server": { type: "stdio", command: "override" } } }, + projectConfig: { mcpServers: {} }, + }); + + // Reflects the preloaded configs, not what's actually on disk for "user"/"project". + expect(names).toEqual(["override-server", "runtime-discovered"]); + }); + test("/mcp getArgumentCompletions resolves known server names after a server-name subcommand, filtered by prefix", async () => { await writeConfig("user", projectDir, { "my-server": { type: "stdio", command: "one" }, @@ -125,4 +155,16 @@ describe("MCP server-name autocomplete", () => { const matches = await mcp.getArgumentCompletions("en"); expect(matches?.map(item => item.label)).toEqual(["enable"]); }); + + test("/mcp getArgumentCompletions returns null instead of throwing when a config file is malformed", async () => { + // Malformed JSON makes readMCPConfigFile's JSON.parse throw (ENOENT is the only + // error it swallows), which must not escape the autocomplete provider. + await Bun.write(getMCPConfigPath("user", projectDir), "{ not valid json"); + const { ctx } = createFakeCtx([]); + const runtime: TuiSlashCommandRuntime = { ctx }; + const mcp = buildTuiBuiltinSlashCommands(runtime).find(c => c.name === "mcp"); + if (!mcp?.getArgumentCompletions) throw new Error("expected /mcp command with getArgumentCompletions"); + + await expect(mcp.getArgumentCompletions("enable ")).resolves.toBeNull(); + }); });