diff --git a/extensions/startup-banner.ts b/extensions/startup-banner.ts index 4d96e266d..03d218e59 100644 --- a/extensions/startup-banner.ts +++ b/extensions/startup-banner.ts @@ -525,6 +525,83 @@ async function countPackageExtensions(packages: unknown[]): Promise { return count; } +interface McpServerEntry { + disabled?: boolean; +} + +interface McpConfigFile { + mcpServers?: unknown; + "mcp-servers"?: unknown; +} + +/** MCP config layers, lowest precedence first, as `getConfigSources` in + * pi-mcp-adapter orders them (verified against 2.34.0). A later layer replaces + * the earlier entry for a server of the same name, which is how `/mcp disable` + * turns a globally configured server off for one project. + * + * Reading only the two Pi-owned files missed a server defined in a shared + * layer entirely, and let an omitted higher-precedence `disabled` entry keep a + * server in the count that the session does not load. + * + * Four adapter sources are deliberately NOT mirrored, because none of them can + * be resolved from a config path alone: exclusive-config mode, opt-in host and + * ancestor discovery, and the package / agent-plugin / Claude-plugin configs. + * A banner that walks those would be a second implementation of the loader + * rather than a reading of it. */ +export function mcpConfigPaths(cwd: string): string[] { + const home = os.homedir(); + return [ + join(home, ".config", "mcp", "mcp.json"), + join(home, ".agents", "mcp.json"), + join(home, ".agents", "mcp", "mcp.json"), + join(PI_AGENT_DIR, "mcp.json"), + join(cwd, ".mcp.json"), + join(cwd, ".pi", "mcp.json"), + ]; +} + +/** The adapter's own entry test (`isRecord` in its `config.ts`): a null, a + * primitive or an array is not a server definition and never reaches the + * session, so it must not reach the count either. */ +function isMcpServerEntry(value: unknown): value is McpServerEntry { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +/** How many MCP servers this session actually loads. + * + * Counting the keys of the global config file overstated it twice over: a + * server carrying `"disabled": true` connects to nothing and registers no + * tools, and a server configured only in the project layer was invisible to a + * global-file parse. A layer that is absent or unparseable contributes + * nothing and does not discard the others. + */ +export async function countEnabledMcpServers( + cwd: string, + read: (path: string) => Promise = (path) => readFile(path, "utf8"), +): Promise { + const servers = new Map(); + for (const path of mcpConfigPaths(cwd)) { + let entries: unknown; + try { + const file = JSON.parse(await read(path)) as McpConfigFile | null; + // `mcp-servers` is the alias the adapter reads alongside `mcpServers`. + entries = file?.mcpServers ?? file?.["mcp-servers"]; + } catch { + continue; + } + if (!isMcpServerEntry(entries)) continue; + for (const [name, entry] of Object.entries(entries)) { + if (!isMcpServerEntry(entry)) continue; + servers.set(name, entry); + } + } + let enabled = 0; + for (const entry of servers.values()) { + if (entry?.disabled !== true) enabled += 1; + } + return enabled; +} + export function readGitBranch(cwd: string, run: typeof execFile = execFile): Promise { return new Promise((resolve) => { run("git", ["-C", cwd, "branch", "--show-current"], { @@ -654,16 +731,7 @@ export default function (pi: ExtensionAPI) { setTimeout(() => { (async () => { - try { - const raw = await readFile( - join(os.homedir(), ".pi", "agent", "mcp.json"), - "utf8", - ); - const cfg = JSON.parse(raw); - mcpServersCount = Object.keys(cfg.mcpServers || {}).length; - } catch { - mcpServersCount = 0; - } + mcpServersCount = await countEnabledMcpServers(ctx.cwd); refreshStats(); })(); }, 150); diff --git a/tests/startup-banner.test.ts b/tests/startup-banner.test.ts index 052f3a918..9515bef92 100644 --- a/tests/startup-banner.test.ts +++ b/tests/startup-banner.test.ts @@ -2,7 +2,7 @@ import assert from "node:assert/strict"; import test from "node:test"; import { syncBuiltinESMExports } from "node:module"; import fs from "node:fs/promises"; -import startup, { readGitBranch } from "../extensions/startup-banner.ts"; +import startup, { countEnabledMcpServers, mcpConfigPaths, readGitBranch } from "../extensions/startup-banner.ts"; import type { ExtensionAPI } from "@earendil-works/pi-coding-agent"; import { visibleWidth } from "@earendil-works/pi-tui"; import { stripAnsi } from "../lib/terminal-theme.ts"; @@ -124,3 +124,101 @@ for (const showRose of [false, true]) for (const showTextLogo of [false, true]) } }); } + +// The banner's MCP stat counted every key in the GLOBAL config file, so a +// server that connects to nothing still inflated it and a project-only server +// never appeared (#979). It also read only the two Pi-owned files, while the +// session merges six layers, so a server defined in a shared layer was missing +// and an omitted higher-precedence `disabled` entry kept one in the count. +const MCP_LAYER_NAMES = [ + "sharedGlobal", + "agentsGlobal", + "agentsNested", + "global", + "projectShared", + "project", +] as const; + +const mcpLayers = (cwd: string, layers: Partial>) => { + const paths = mcpConfigPaths(cwd); + // Control: the layers really are distinct paths, or every case below would + // be measuring one file several times over. + assert.equal(paths.length, MCP_LAYER_NAMES.length); + assert.equal(new Set(paths).size, paths.length); + const byPath = new Map(paths.map((path, index) => [path, MCP_LAYER_NAMES[index]!])); + return async (path: string) => { + const name = byPath.get(path); + const body = name === undefined ? undefined : layers[name]; + if (body === undefined) throw new Error(`ENOENT: ${path}`); + return typeof body === "string" ? body : JSON.stringify(body); + }; +}; + +test("MCP stat counts servers the session loads, not keys in the config file", async () => { + const read = mcpLayers("/repo", { + global: { mcpServers: { one: { command: "a" }, two: { command: "b", disabled: true } } }, + }); + assert.equal(await countEnabledMcpServers("/repo", read), 1); +}); + +test("MCP stat sees a project layer, and lets it disable a globally enabled server", async () => { + const read = mcpLayers("/repo", { + global: { mcpServers: { shared: { command: "a" } } }, + project: { mcpServers: { shared: { command: "a", disabled: true }, local: { command: "c" } } }, + }); + // `shared` is off for this project and `local` exists only here: one server. + assert.equal(await countEnabledMcpServers("/repo", read), 1); +}); + +test("MCP stat reads the shared layers the session merges, not only the Pi-owned two", async () => { + // One server per shared layer, none of them in a Pi-owned file. Reading only + // `~/.pi/agent/mcp.json` and `/.pi/mcp.json` reported none of them. + const read = mcpLayers("/repo", { + sharedGlobal: { mcpServers: { a: { command: "a" } } }, + agentsGlobal: { mcpServers: { b: { command: "b" } } }, + agentsNested: { mcpServers: { c: { command: "c" } } }, + projectShared: { mcpServers: { d: { command: "d" } } }, + }); + assert.equal(await countEnabledMcpServers("/repo", read), 4); +}); + +test("MCP stat lets a higher layer disable a server a lower one enabled", async () => { + // The direction the old two-path read could not see: `.mcp.json` sits ABOVE + // the Pi global file, so its `disabled` wins and the banner must not keep + // counting the server the global layer enabled. + const read = mcpLayers("/repo", { + sharedGlobal: { mcpServers: { shared: { command: "a" } } }, + projectShared: { mcpServers: { shared: { command: "a", disabled: true } } }, + }); + assert.equal(await countEnabledMcpServers("/repo", read), 0); +}); + +test("MCP stat reads the mcp-servers spelling the adapter also accepts", async () => { + const read = mcpLayers("/repo", { + global: { "mcp-servers": { one: { command: "a" } } }, + }); + assert.equal(await countEnabledMcpServers("/repo", read), 1); +}); + +test("MCP stat tolerates a config whose mcpServers is not an object of entries", async () => { + // An entry that is not a non-null object is not a server definition: the + // adapter's `isRecord` drops it before the session sees it, so counting it + // as enabled reported a server that never loads. + const shapes: [unknown, number][] = [ + [{ mcpServers: [] }, 0], + [{ mcpServers: null }, 0], + [{}, 0], + ["null", 0], + [{ mcpServers: { one: null } }, 0], + [{ mcpServers: { one: "a" } }, 0], + [{ mcpServers: { one: [] } }, 0], + [{ mcpServers: { one: { command: "a" }, two: null } }, 1], + ]; + for (const [shape, expected] of shapes) { + assert.equal( + await countEnabledMcpServers("/repo", mcpLayers("/repo", { global: shape })), + expected, + `shape ${JSON.stringify(shape)}`, + ); + } +});