smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(coding-agent): include disabled-discovered servers in /mcp autocomplete, harden config-read failures

Browse Source 
- 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.
This commit is contained in:
Mathews-Tom
2026-07-24 06:24:17 +05:30
parent 21721834b2
commit d1c828b356
3 changed files with 88 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/src/modes/controllers/mcp-command-controller.ts
+38 -15
View File
@@ -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 <name>` 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 <name>` 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<string[]> {
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<string[]> {
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<string>([
...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;
packages/coding-agent/src/slash-commands/builtin-registry.ts
+8 -2
View File
@@ -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 }));
packages/coding-agent/test/mcp-name-autocomplete.test.ts
+42
View File
@@ -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();
});
});