From 6a4b63692a156d6863547cacf510f1ad338d6488 Mon Sep 17 00:00:00 2001 From: L4XB Date: Sun, 13 Sep 2026 20:35:37 +0200 Subject: [PATCH 1/2] fix(banner): count the MCP servers the session loads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The startup banner read the global `~/.pi/agent/mcp.json` and reported `Object.keys(cfg.mcpServers).length`, which overstates the MCP surface in two directions: * A server carrying `"disabled": true` connects to nothing, authenticates nothing and registers no tools, but still counted. * A server configured only in the project layer never appeared at all, because only the global file was parsed. `countEnabledMcpServers` reads both layers lowest-precedence first and counts the surviving entries that are not disabled, so `/mcp disable` in a project turns a globally configured server off in the banner as well. A layer that is absent or unparseable contributes nothing and no longer discards the layers that did parse — the old `catch` reset the whole count to 0. The count is a pure function of the two file bodies, with the reader injected the way `readGitBranch` takes its `execFile`, so the tests state the contract without touching a filesystem. Fixes #979 --- extensions/startup-banner.ts | 58 +++++++++++++++++++++++++++++------- tests/startup-banner.test.ts | 58 +++++++++++++++++++++++++++++++++++- 2 files changed, 105 insertions(+), 11 deletions(-) diff --git a/extensions/startup-banner.ts b/extensions/startup-banner.ts index 4d96e266d..89b4df822 100644 --- a/extensions/startup-banner.ts +++ b/extensions/startup-banner.ts @@ -525,6 +525,53 @@ async function countPackageExtensions(packages: unknown[]): Promise { return count; } +interface McpServerEntry { + disabled?: boolean; +} + +interface McpConfigFile { + mcpServers?: Record; +} + +/** MCP config layers, lowest precedence first. A project layer replaces the + * global entry for a server of the same name, which is how `/mcp disable` + * turns a globally configured server off for one project. */ +export function mcpConfigPaths(cwd: string): string[] { + return [join(PI_AGENT_DIR, "mcp.json"), join(cwd, ".pi", "mcp.json")]; +} + +/** 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: McpConfigFile["mcpServers"]; + try { + entries = (JSON.parse(await read(path)) as McpConfigFile | null)?.mcpServers; + } catch { + continue; + } + if (!entries || typeof entries !== "object" || Array.isArray(entries)) continue; + for (const [name, entry] of Object.entries(entries)) { + 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 +701,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..7e0ea2ba3 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,59 @@ 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). +const mcpLayers = (cwd: string, layers: Record) => { + const [global, project] = mcpConfigPaths(cwd); + // Control: the two layers really are two distinct paths, or every case + // below would be measuring one file twice. + assert.notEqual(global, project); + return async (path: string) => { + const body = path === global ? layers.global : path === project ? layers.project : undefined; + 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 keeps the layers it can read when another is missing or malformed", async () => { + const twoEnabled = { mcpServers: { one: { command: "a" }, two: { command: "b" } } }; + // Missing project layer. + assert.equal(await countEnabledMcpServers("/repo", mcpLayers("/repo", { global: twoEnabled })), 2); + // Unparseable global layer, readable project layer. + assert.equal(await countEnabledMcpServers("/repo", mcpLayers("/repo", { + global: "{ not json", + project: { mcpServers: { only: { command: "c" } } }, + })), 1); + // Nothing readable at all is zero, not a crash. + assert.equal(await countEnabledMcpServers("/repo", mcpLayers("/repo", {})), 0); +}); + +test("MCP stat tolerates a config whose mcpServers is not an object of entries", async () => { + for (const shape of [{ mcpServers: [] }, { mcpServers: null }, {}, "null", { mcpServers: { one: null } }]) { + const expected = shape !== null && typeof shape === "object" + && "mcpServers" in shape && shape.mcpServers !== null && !Array.isArray(shape.mcpServers) ? 1 : 0; + assert.equal( + await countEnabledMcpServers("/repo", mcpLayers("/repo", { global: shape })), + expected, + `shape ${JSON.stringify(shape)}`, + ); + } +}); From 7d27fc988bb74d2281e672ec9e93252625b0582f Mon Sep 17 00:00:00 2001 From: L4XB Date: Tue, 15 Sep 2026 14:59:10 +0200 Subject: [PATCH 2/2] fix(banner): read every MCP layer the session merges, and skip non-entries Both findings from the review on #980, verified against the adapter's own source rather than taken from the summary. `pi-mcp-adapter@2.34.0`, `config.ts`: - `getConfigSources` orders six unconditional layers lowest to highest: `~/.config/mcp/mcp.json`, `~/.agents/mcp.json`, `~/.agents/mcp/mcp.json`, the Pi global file, `/.mcp.json`, `/.pi/mcp.json`. Reading only the two Pi-owned ones missed a server defined in a shared layer, and let an omitted higher-precedence `disabled` entry keep one in the count. - `toServerEntries` keeps an entry only when `isRecord` accepts it, so a null, a primitive or an array is not a server definition and never reaches the session. Storing `entry ?? {}` counted those as enabled. - `validateConfig` reads `raw.mcpServers ?? raw["mcp-servers"]`, so the alias spelling counts too. Four adapter sources are deliberately not mirrored, and the comment says so rather than leaving the gap silent: exclusive-config mode, opt-in host and ancestor discovery, and the package / agent-plugin / Claude-plugin configs. None can be resolved from a config path alone, and walking them would make the banner a second implementation of the loader rather than a reading of it. `isServerDisabled` in the adapter's `types.ts` is `definition?.disabled === true`, which is what this already used, so that half needed no change. Tests: the layer helper is keyed by layer name instead of destructuring two paths, four cells added (a shared-only layer is counted, a higher layer's `disabled` wins, the alias spelling, and the entry shapes), and the invalid-entry table now expects zero where it expected one. 12 pass. Five mutations, all killed. --- extensions/startup-banner.ts | 48 +++++++++++++++++---- tests/startup-banner.test.ts | 84 +++++++++++++++++++++++++++--------- 2 files changed, 102 insertions(+), 30 deletions(-) diff --git a/extensions/startup-banner.ts b/extensions/startup-banner.ts index 89b4df822..03d218e59 100644 --- a/extensions/startup-banner.ts +++ b/extensions/startup-banner.ts @@ -530,14 +530,41 @@ interface McpServerEntry { } interface McpConfigFile { - mcpServers?: Record; + mcpServers?: unknown; + "mcp-servers"?: unknown; } -/** MCP config layers, lowest precedence first. A project layer replaces the - * global entry for a server of the same name, which is how `/mcp disable` - * turns a globally configured server off for one project. */ +/** 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[] { - return [join(PI_AGENT_DIR, "mcp.json"), join(cwd, ".pi", "mcp.json")]; + 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. @@ -554,15 +581,18 @@ export async function countEnabledMcpServers( ): Promise { const servers = new Map(); for (const path of mcpConfigPaths(cwd)) { - let entries: McpConfigFile["mcpServers"]; + let entries: unknown; try { - entries = (JSON.parse(await read(path)) as McpConfigFile | null)?.mcpServers; + 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 (!entries || typeof entries !== "object" || Array.isArray(entries)) continue; + if (!isMcpServerEntry(entries)) continue; for (const [name, entry] of Object.entries(entries)) { - servers.set(name, entry ?? {}); + if (!isMcpServerEntry(entry)) continue; + servers.set(name, entry); } } let enabled = 0; diff --git a/tests/startup-banner.test.ts b/tests/startup-banner.test.ts index 7e0ea2ba3..9515bef92 100644 --- a/tests/startup-banner.test.ts +++ b/tests/startup-banner.test.ts @@ -127,14 +127,28 @@ 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). -const mcpLayers = (cwd: string, layers: Record) => { - const [global, project] = mcpConfigPaths(cwd); - // Control: the two layers really are two distinct paths, or every case - // below would be measuring one file twice. - assert.notEqual(global, project); +// 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 body = path === global ? layers.global : path === project ? layers.project : undefined; + 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); }; @@ -156,23 +170,51 @@ test("MCP stat sees a project layer, and lets it disable a globally enabled serv assert.equal(await countEnabledMcpServers("/repo", read), 1); }); -test("MCP stat keeps the layers it can read when another is missing or malformed", async () => { - const twoEnabled = { mcpServers: { one: { command: "a" }, two: { command: "b" } } }; - // Missing project layer. - assert.equal(await countEnabledMcpServers("/repo", mcpLayers("/repo", { global: twoEnabled })), 2); - // Unparseable global layer, readable project layer. - assert.equal(await countEnabledMcpServers("/repo", mcpLayers("/repo", { - global: "{ not json", - project: { mcpServers: { only: { command: "c" } } }, - })), 1); - // Nothing readable at all is zero, not a crash. - assert.equal(await countEnabledMcpServers("/repo", mcpLayers("/repo", {})), 0); +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 () => { - for (const shape of [{ mcpServers: [] }, { mcpServers: null }, {}, "null", { mcpServers: { one: null } }]) { - const expected = shape !== null && typeof shape === "object" - && "mcpServers" in shape && shape.mcpServers !== null && !Array.isArray(shape.mcpServers) ? 1 : 0; + // 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,