diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index ccfc3db6d..9d2799b44 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -17,6 +17,7 @@ ### Fixed +- Fixed MCP startup status to live-update from "Connecting…" to connected, still-connecting, or failed server states so completed connections do not leave a stale banner. ([#3150](https://github.com/can1357/oh-my-pi/issues/3150)) - Fixed session history becoming desynchronized when using the `rewind` tool - Fixed `rewind` tool output and temporary assistant side-channel data polluting the prompt cache - Fixed `read` against a SQLite table with many columns (e.g. 33) rendering every cell as an ellipsis and chopping the right edge. The ASCII table shrinker bottomed out at `MIN_COLUMN_WIDTH=1` so every multi-char cell collapsed to `…`, and the final line truncation then cut off the right side. The renderer now bumps the per-column floor to 3 and falls back to a vertical `column: value` block layout per row when the column count exceeds the horizontal width budget. ([#3107](https://github.com/can1357/oh-my-pi/issues/3107)) diff --git a/packages/coding-agent/src/mcp/loader.ts b/packages/coding-agent/src/mcp/loader.ts index e43a7753a..07c970e45 100644 --- a/packages/coding-agent/src/mcp/loader.ts +++ b/packages/coding-agent/src/mcp/loader.ts @@ -8,6 +8,7 @@ import type { LoadedCustomTool } from "../extensibility/custom-tools/types"; import { AgentStorage } from "../session/agent-storage"; import type { AuthStorage } from "../session/auth-storage"; import { type MCPLoadResult, MCPManager } from "./manager"; +import type { McpConnectionStatusEvent } from "./startup-events"; import { MCPToolCache } from "./tool-cache"; /** Result from loading MCP tools */ @@ -26,8 +27,8 @@ export interface MCPToolsLoadResult { /** Options for loading MCP tools */ export interface MCPToolsLoadOptions { - /** Called when starting to connect to servers */ - onConnecting?: (serverNames: string[]) => void; + /** Called when MCP server connection state changes. */ + onStatus?: (event: McpConnectionStatusEvent) => void; /** Whether to load project-level config (default: true) */ enableProjectConfig?: boolean; /** Whether to filter out Exa MCP servers (default: true) */ @@ -68,7 +69,7 @@ export async function discoverAndLoadMCPTools(cwd: string, options?: MCPToolsLoa let result: MCPLoadResult; try { result = await manager.discoverAndConnect({ - onConnecting: options?.onConnecting, + onStatus: options?.onStatus, enableProjectConfig: options?.enableProjectConfig, filterExa: options?.filterExa, filterBrowser: options?.filterBrowser, diff --git a/packages/coding-agent/src/mcp/manager.ts b/packages/coding-agent/src/mcp/manager.ts index 74a7c0943..ea34bcdbd 100644 --- a/packages/coding-agent/src/mcp/manager.ts +++ b/packages/coding-agent/src/mcp/manager.ts @@ -26,13 +26,14 @@ import { subscribeToResources, unsubscribeFromResources, } from "./client"; -import { loadAllMCPConfigs, validateServerConfig } from "./config"; +import { type LoadMCPConfigsResult, loadAllMCPConfigs, validateServerConfig } from "./config"; import { lookupMcpOAuthCredential, type MCPOAuthCredentialLookup, selectMcpOAuthRefreshMaterial, } from "./oauth-credentials"; import { type MCPStoredOAuthCredential, refreshMCPOAuthToken } from "./oauth-flow"; +import type { McpConnectionStatusEvent } from "./startup-events"; import type { MCPToolDetails } from "./tool-bridge"; import { DeferredMCPTool, MCPTool } from "./tool-bridge"; import type { MCPToolCache } from "./tool-cache"; @@ -148,8 +149,8 @@ export interface MCPDiscoverOptions { filterExa?: boolean; /** Whether to filter out browser MCP servers when builtin browser tool is enabled (default: false) */ filterBrowser?: boolean; - /** Called when starting to connect to servers */ - onConnecting?: (serverNames: string[]) => void; + /** Called when MCP server connection state changes. */ + onStatus?: (event: McpConnectionStatusEvent) => void; } /** @@ -309,12 +310,20 @@ export class MCPManager { * Returns tools and any connection errors. */ async discoverAndConnect(options?: MCPDiscoverOptions): Promise { - const { configs, exaApiKeys, sources } = await loadAllMCPConfigs(this.cwd, { - enableProjectConfig: options?.enableProjectConfig, - filterExa: options?.filterExa, - filterBrowser: options?.filterBrowser, - }); - const result = await this.connectServers(configs, sources, options?.onConnecting); + let loadedConfigs: LoadMCPConfigsResult; + try { + loadedConfigs = await loadAllMCPConfigs(this.cwd, { + enableProjectConfig: options?.enableProjectConfig, + filterExa: options?.filterExa, + filterBrowser: options?.filterBrowser, + }); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + options?.onStatus?.({ type: "failed", serverName: ".mcp.json", error: message }); + throw error; + } + const { configs, exaApiKeys, sources } = loadedConfigs; + const result = await this.connectServers(configs, sources, options?.onStatus); result.exaApiKeys = exaApiKeys; return result; } @@ -326,7 +335,7 @@ export class MCPManager { async connectServers( configs: Record, sources: Record, - onConnecting?: (serverNames: string[]) => void, + onStatus?: (event: McpConnectionStatusEvent) => void, ): Promise { type ConnectionTask = { name: string; @@ -340,6 +349,8 @@ export class MCPManager { const allTools: CustomTool[] = []; const reportedErrors = new Set(); let allowBackgroundLogging = false; + const statusServerNames: string[] = []; + const validationFailures: Array<{ name: string; message: string }> = []; // Prepare connection tasks const connectionTasks: ConnectionTask[] = []; @@ -367,10 +378,14 @@ export class MCPManager { continue; } + statusServerNames.push(name); + // Validate config const validationErrors = validateServerConfig(name, config); if (validationErrors.length > 0) { - errors.set(name, validationErrors.join("; ")); + const message = validationErrors.join("; "); + errors.set(name, message); + validationFailures.push({ name, message }); reportedErrors.add(name); continue; } @@ -458,20 +473,25 @@ export class MCPManager { this.#onToolsChanged?.(this.#tools); void this.toolCache?.set(name, config, serverTools); + onStatus?.({ type: "connected", serverName: name }); await this.#loadServerResourcesAndPrompts(name, connection); }) .catch(error => { if (this.#pendingToolLoads.get(name) !== toolsPromise) return; this.#pendingToolLoads.delete(name); - if (!allowBackgroundLogging || reportedErrors.has(name)) return; const message = error instanceof Error ? error.message : String(error); + onStatus?.({ type: "failed", serverName: name, error: message }); + if (!allowBackgroundLogging || reportedErrors.has(name)) return; logger.error("MCP tool load failed", { path: `mcp:${name}`, error: message }); }); } - // Notify about servers we're connecting to - if (connectionTasks.length > 0 && onConnecting) { - onConnecting(connectionTasks.map(task => task.name)); + // Notify about servers we're connecting to, including configs that fail fast. + if (statusServerNames.length > 0 && onStatus) { + onStatus({ type: "connecting", serverNames: statusServerNames }); + for (const { name, message } of validationFailures) { + onStatus({ type: "failed", serverName: name, error: message }); + } } if (connectionTasks.length > 0) { diff --git a/packages/coding-agent/src/mcp/startup-events.ts b/packages/coding-agent/src/mcp/startup-events.ts index 16f18d371..9bebc07b7 100644 --- a/packages/coding-agent/src/mcp/startup-events.ts +++ b/packages/coding-agent/src/mcp/startup-events.ts @@ -1,9 +1,63 @@ -export const MCP_CONNECTING_EVENT_CHANNEL = "mcp:connecting"; +export const MCP_CONNECTION_STATUS_EVENT_CHANNEL = "mcp:connection-status"; -export type McpConnectingEvent = { serverNames: string[] }; +export type McpConnectionStatusEvent = + | { type: "connecting"; serverNames: string[] } + | { type: "connected"; serverName: string } + | { type: "failed"; serverName: string; error: string }; -export function formatMCPConnectingMessage(serverNames: string[]): string { - return `Connecting to MCP servers: ${serverNames.join(", ")}…`; +export type McpConnectionStatusSnapshot = { + pendingServers: readonly string[]; + connectedServers: readonly string[]; + failedServers: readonly { serverName: string; error: string }[]; +}; + +function formatServerList(serverNames: readonly string[]): string { + return serverNames.join(", "); +} + +function formatServerCount(count: number): string { + return count === 1 ? "server" : "servers"; +} + +export function formatMCPConnectingMessage(serverNames: readonly string[]): string { + return `Connecting to MCP servers: ${formatServerList(serverNames)}…`; +} + +export function formatMCPConnectionStatusMessage(snapshot: McpConnectionStatusSnapshot): string { + const { pendingServers, connectedServers, failedServers } = snapshot; + if (pendingServers.length > 0) { + if (connectedServers.length === 0 && failedServers.length === 0) { + return formatMCPConnectingMessage(pendingServers); + } + const parts: string[] = []; + if (connectedServers.length > 0) { + parts.push(`Connected: ${formatServerList(connectedServers)}.`); + } + if (failedServers.length > 0) { + parts.push(`Failed: ${failedServers.map(({ serverName, error }) => `${serverName}: ${error}`).join("; ")}.`); + } + parts.push(`Still connecting: ${formatServerList(pendingServers)}…`); + return parts.join(" "); + } + if (failedServers.length > 0) { + const failureText = failedServers.map(({ serverName, error }) => `${serverName}: ${error}`).join("; "); + if (connectedServers.length === 0) { + return `MCP ${formatServerCount(failedServers.length)} failed to connect: ${failureText}`; + } + return `MCP finished with failures. Connected: ${formatServerList(connectedServers)}. Failed: ${failureText}`; + } + if (connectedServers.length > 0) { + return `Connected to MCP ${formatServerCount(connectedServers.length)}: ${formatServerList(connectedServers)}.`; + } + return ""; +} + +function isRecord(data: unknown): data is Record { + return typeof data === "object" && data !== null; +} + +function isStringArray(data: unknown): data is string[] { + return Array.isArray(data) && data.every(item => typeof item === "string"); } /** @@ -11,11 +65,16 @@ export function formatMCPConnectingMessage(serverNames: string[]): string { * untyped at runtime, so the subscriber verifies the shape before formatting * rather than trusting a cast — a malformed emit is ignored instead of throwing. */ -export function isMcpConnectingEvent(data: unknown): data is McpConnectingEvent { - return ( - typeof data === "object" && - data !== null && - Array.isArray((data as { serverNames?: unknown }).serverNames) && - (data as { serverNames: unknown[] }).serverNames.every(name => typeof name === "string") - ); +export function isMcpConnectionStatusEvent(data: unknown): data is McpConnectionStatusEvent { + if (!isRecord(data) || typeof data.type !== "string") return false; + switch (data.type) { + case "connecting": + return isStringArray(data.serverNames); + case "connected": + return typeof data.serverName === "string"; + case "failed": + return typeof data.serverName === "string" && typeof data.error === "string"; + default: + return false; + } } diff --git a/packages/coding-agent/src/modes/interactive-mode.ts b/packages/coding-agent/src/modes/interactive-mode.ts index be947734b..07d05a566 100644 --- a/packages/coding-agent/src/modes/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive-mode.ts @@ -70,7 +70,12 @@ import type { Goal, GoalModeState } from "../goals/state"; import { resolveLocalUrlToPath } from "../internal-urls"; import { LSP_STARTUP_EVENT_CHANNEL, type LspStartupEvent } from "../lsp/startup-events"; import type { MCPManager } from "../mcp"; -import { formatMCPConnectingMessage, isMcpConnectingEvent, MCP_CONNECTING_EVENT_CHANNEL } from "../mcp/startup-events"; +import { + formatMCPConnectionStatusMessage, + isMcpConnectionStatusEvent, + MCP_CONNECTION_STATUS_EVENT_CHANNEL, + type McpConnectionStatusEvent, +} from "../mcp/startup-events"; import { humanizePlanTitle, type PlanApprovalDetails, @@ -527,6 +532,10 @@ export class InteractiveMode implements InteractiveModeContext { #observerRegistry: SessionObserverRegistry; #eventBus?: EventBus; #eventBusUnsubscribers: Array<() => void> = []; + #mcpStatusOrder: string[] = []; + #mcpPendingServers = new Set(); + #mcpConnectedServers = new Set(); + #mcpFailedServers = new Map(); #welcomeComponent?: WelcomeComponent; readonly #chatHost: ChatBlockHost = { requestRender: () => this.ui.requestRender() }; @@ -560,13 +569,12 @@ export class InteractiveMode implements InteractiveModeContext { }), ); this.#eventBusUnsubscribers.push( - eventBus.on(MCP_CONNECTING_EVENT_CHANNEL, data => { - if (!isMcpConnectingEvent(data)) { - logger.warn("Ignoring malformed mcp:connecting event", { data }); + eventBus.on(MCP_CONNECTION_STATUS_EVENT_CHANNEL, data => { + if (!isMcpConnectionStatusEvent(data)) { + logger.warn("Ignoring malformed mcp:connection-status event", { data }); return; } - if (this.settings.get("startup.quiet")) return; - this.showStatus(formatMCPConnectingMessage(data.serverNames)); + this.#handleMcpConnectionStatusEvent(data); }), ); } @@ -662,6 +670,54 @@ export class InteractiveMode implements InteractiveModeContext { this.#observerRegistry = new SessionObserverRegistry(); } + #handleMcpConnectionStatusEvent(event: McpConnectionStatusEvent): void { + if (this.settings.get("startup.quiet")) return; + if (event.type === "connecting") { + this.#mcpStatusOrder = []; + this.#mcpPendingServers.clear(); + this.#mcpConnectedServers.clear(); + this.#mcpFailedServers.clear(); + for (const serverName of event.serverNames) { + this.#trackMcpStatusServer(serverName); + this.#mcpPendingServers.add(serverName); + } + } else if (event.type === "connected") { + this.#trackMcpStatusServer(event.serverName); + this.#mcpPendingServers.delete(event.serverName); + this.#mcpFailedServers.delete(event.serverName); + this.#mcpConnectedServers.add(event.serverName); + } else { + this.#trackMcpStatusServer(event.serverName); + this.#mcpPendingServers.delete(event.serverName); + this.#mcpConnectedServers.delete(event.serverName); + this.#mcpFailedServers.set(event.serverName, event.error); + } + + const message = formatMCPConnectionStatusMessage({ + pendingServers: this.#orderedMcpStatusServers(this.#mcpPendingServers), + connectedServers: this.#orderedMcpStatusServers(this.#mcpConnectedServers), + failedServers: this.#orderedMcpStatusFailures(), + }); + if (message) this.showStatus(message); + } + + #trackMcpStatusServer(serverName: string): void { + if (!this.#mcpStatusOrder.includes(serverName)) { + this.#mcpStatusOrder.push(serverName); + } + } + + #orderedMcpStatusServers(servers: ReadonlySet): string[] { + return this.#mcpStatusOrder.filter(serverName => servers.has(serverName)); + } + + #orderedMcpStatusFailures(): Array<{ serverName: string; error: string }> { + return this.#mcpStatusOrder.flatMap(serverName => { + const error = this.#mcpFailedServers.get(serverName); + return error === undefined ? [] : [{ serverName, error }]; + }); + } + playWelcomeIntro(): void { const welcome = this.#welcomeComponent; // Component-scoped: the intro only mutates the welcome box's own rows, diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index 2bfad79ae..7c66c3657 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -87,7 +87,7 @@ import { type MCPToolsLoadResult, parseMCPToolName, } from "./mcp"; -import { MCP_CONNECTING_EVENT_CHANNEL, type McpConnectingEvent } from "./mcp/startup-events"; +import { MCP_CONNECTION_STATUS_EVENT_CHANNEL, type McpConnectionStatusEvent } from "./mcp/startup-events"; import { createSessionMemoryRuntimeContext, resolveMemoryBackend } from "./memory-backend"; import type { MnemopiSessionState } from "./mnemopi/state"; import asyncResultTemplate from "./prompts/tools/async-result.md" with { type: "text" }; @@ -1620,12 +1620,13 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} | ((liveSession: AgentSession, activation: DeferredMCPActivation) => void) | undefined; const startupQuiet = settings.get("startup.quiet"); - const onMCPConnecting = (serverNames: string[]) => { - if (!options.hasUI || startupQuiet || serverNames.length === 0) return; - eventBus.emit(MCP_CONNECTING_EVENT_CHANNEL, { serverNames } satisfies McpConnectingEvent); + const onMCPStatus = (event: McpConnectionStatusEvent) => { + if (!options.hasUI || startupQuiet) return; + if (event.type === "connecting" && event.serverNames.length === 0) return; + eventBus.emit(MCP_CONNECTION_STATUS_EVENT_CHANNEL, event); }; const mcpDiscoverOptions = { - onConnecting: onMCPConnecting, + onStatus: onMCPStatus, enableProjectConfig: settings.get("mcp.enableProjectConfig") ?? true, // Always filter Exa - we have native integration filterExa: true, diff --git a/packages/coding-agent/test/interactive-mode-mcp-connecting.test.ts b/packages/coding-agent/test/interactive-mode-mcp-connecting.test.ts index cb1f67cf1..85e00a133 100644 --- a/packages/coding-agent/test/interactive-mode-mcp-connecting.test.ts +++ b/packages/coding-agent/test/interactive-mode-mcp-connecting.test.ts @@ -4,9 +4,9 @@ import { Agent } from "@oh-my-pi/pi-agent-core"; import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { - formatMCPConnectingMessage, - MCP_CONNECTING_EVENT_CHANNEL, - type McpConnectingEvent, + formatMCPConnectionStatusMessage, + MCP_CONNECTION_STATUS_EVENT_CHANNEL, + type McpConnectionStatusEvent, } from "@oh-my-pi/pi-coding-agent/mcp/startup-events"; import { InteractiveMode } from "@oh-my-pi/pi-coding-agent/modes/interactive-mode"; import { initTheme } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; @@ -17,16 +17,13 @@ import { EventBus } from "@oh-my-pi/pi-coding-agent/utils/event-bus"; import { logger, TempDir } from "@oh-my-pi/pi-utils"; /** - * Behavioral wiring guard for the MCP connecting banner (mirrors - * interactive-mode-lsp-startup.test.ts). The fix routes the banner through the - * render tree instead of `process.stderr.write`: sdk emits on - * `MCP_CONNECTING_EVENT_CHANNEL` and InteractiveMode's constructor subscribes, - * rendering via `showStatus`. The shared-module contract test pins the channel - * string and formatter; this pins the live subscriber — dropping the - * `eventBus.on(...)` registration or diverging the channel would silently kill - * the banner with no type error, and this case would fail. + * Behavioral wiring guard for MCP startup status (mirrors + * interactive-mode-lsp-startup.test.ts). The SDK emits connection lifecycle + * events, and InteractiveMode aggregates them into one live status line. This + * pins the constructor-time subscription and the update path that replaces the + * stale "Connecting…" banner when servers connect or fail. */ -describe("InteractiveMode MCP connecting banner", () => { +describe("InteractiveMode MCP connection status", () => { let authStorage: AuthStorage; let eventBus: EventBus; let mode: InteractiveMode; @@ -87,46 +84,72 @@ describe("InteractiveMode MCP connecting banner", () => { resetSettingsForTest(); }); - it("routes a mcp:connecting event through the constructor-registered subscriber, before init()", () => { - // The subscription is registered in the InteractiveMode constructor, so the - // banner routes BEFORE init()/any async startup. Emitting here — with no - // init() — pins that race-sensitive invariant: the real sdk emit is gated - // behind async MCP config loading (loadAllMCPConfigs), so a constructor-time - // subscriber always wins. Stub showStatus so no initialized UI is needed. + it("routes a mcp:connection-status event through the constructor-registered subscriber, before init()", () => { const showStatusSpy = vi.spyOn(mode, "showStatus").mockImplementation(() => {}); const serverNames = ["sequential", "critic", "shannon"]; - eventBus.emit(MCP_CONNECTING_EVENT_CHANNEL, { serverNames } satisfies McpConnectingEvent); + const event = { type: "connecting", serverNames } satisfies McpConnectionStatusEvent; + eventBus.emit(MCP_CONNECTION_STATUS_EVENT_CHANNEL, event); - // A dropped subscription or a channel divergence would leave showStatus - // uncalled; a revert to raw stderr.write would never reach showStatus either. - expect(showStatusSpy).toHaveBeenCalledWith(formatMCPConnectingMessage(serverNames)); + expect(showStatusSpy).toHaveBeenCalledWith( + formatMCPConnectionStatusMessage({ + pendingServers: serverNames, + connectedServers: [], + failedServers: [], + }), + ); }); - it("does not render the mcp:connecting status when startup.quiet is enabled", () => { + it("does not render the mcp:connection-status status when startup.quiet is enabled", () => { session.settings.set("startup.quiet", true); const showStatusSpy = vi.spyOn(mode, "showStatus").mockImplementation(() => {}); - eventBus.emit(MCP_CONNECTING_EVENT_CHANNEL, { + eventBus.emit(MCP_CONNECTION_STATUS_EVENT_CHANNEL, { + type: "connecting", serverNames: ["sequential", "critic"], - } satisfies McpConnectingEvent); + } satisfies McpConnectionStatusEvent); expect(showStatusSpy).not.toHaveBeenCalled(); }); - it("rejects a malformed mcp:connecting payload via the guard instead of letting it throw", () => { + it("updates the live MCP status as servers connect and fail", () => { + const showStatusSpy = vi.spyOn(mode, "showStatus").mockImplementation(() => {}); + + eventBus.emit(MCP_CONNECTION_STATUS_EVENT_CHANNEL, { + type: "connecting", + serverNames: ["alpha", "broken", "slow"], + } satisfies McpConnectionStatusEvent); + eventBus.emit(MCP_CONNECTION_STATUS_EVENT_CHANNEL, { + type: "connected", + serverName: "alpha", + } satisfies McpConnectionStatusEvent); + eventBus.emit(MCP_CONNECTION_STATUS_EVENT_CHANNEL, { + type: "failed", + serverName: "broken", + error: "missing command", + } satisfies McpConnectionStatusEvent); + eventBus.emit(MCP_CONNECTION_STATUS_EVENT_CHANNEL, { + type: "connected", + serverName: "slow", + } satisfies McpConnectionStatusEvent); + + expect(showStatusSpy.mock.calls.map(call => call[0])).toEqual([ + "Connecting to MCP servers: alpha, broken, slow…", + "Connected: alpha. Still connecting: broken, slow…", + "Connected: alpha. Failed: broken: missing command. Still connecting: slow…", + "MCP finished with failures. Connected: alpha, slow. Failed: broken: missing command", + ]); + }); + + it("rejects a malformed mcp:connection-status payload via the guard instead of letting it throw", () => { const showStatusSpy = vi.spyOn(mode, "showStatus").mockImplementation(() => {}); const warnSpy = vi.spyOn(logger, "warn").mockImplementation(() => {}); - // The EventBus swallows handler throws into logger.error, so the discriminator - // is: with the guard the handler returns early (logger.warn, no error); without - // it the cast reaches formatMCPConnectingMessage(undefined) and throws a - // TypeError the bus catches as logger.error. const errorSpy = vi.spyOn(logger, "error").mockImplementation(() => {}); - eventBus.emit(MCP_CONNECTING_EVENT_CHANNEL, { wrong: "shape" }); + eventBus.emit(MCP_CONNECTION_STATUS_EVENT_CHANNEL, { wrong: "shape" }); expect(showStatusSpy).not.toHaveBeenCalled(); - expect(warnSpy).toHaveBeenCalled(); // guard took the reject branch - expect(errorSpy).not.toHaveBeenCalled(); // no swallowed TypeError from a bad cast + expect(warnSpy).toHaveBeenCalled(); + expect(errorSpy).not.toHaveBeenCalled(); }); }); diff --git a/packages/coding-agent/test/mcp-connection-status-events.test.ts b/packages/coding-agent/test/mcp-connection-status-events.test.ts new file mode 100644 index 000000000..bb32f7f15 --- /dev/null +++ b/packages/coding-agent/test/mcp-connection-status-events.test.ts @@ -0,0 +1,49 @@ +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { MCPManager } from "@oh-my-pi/pi-coding-agent/mcp/manager"; +import type { McpConnectionStatusEvent } from "@oh-my-pi/pi-coding-agent/mcp/startup-events"; +import type { MCPServerConfig } from "@oh-my-pi/pi-coding-agent/mcp/types"; + +const FIXTURE_PATH = path.join(import.meta.dir, "fixtures", "many-tools-mcp.ts"); +const BUN_EXEC = process.execPath; + +describe("MCPManager connection status events", () => { + let workDir: string; + + beforeEach(() => { + workDir = fs.mkdtempSync(path.join(os.tmpdir(), "omp-mcp-status-")); + }); + + afterEach(() => { + fs.rmSync(workDir, { recursive: true, force: true }); + }); + + it("emits connecting, connected, and failed updates for startup status", async () => { + const manager = new MCPManager(workDir); + const events: McpConnectionStatusEvent[] = []; + const success: MCPServerConfig = { + type: "stdio", + command: BUN_EXEC, + args: [FIXTURE_PATH], + }; + const invalid: MCPServerConfig = { type: "stdio", command: "" }; + + try { + const result = await manager.connectServers({ alpha: success, broken: invalid }, {}, event => + events.push(event), + ); + + expect(result.connectedServers).toContain("alpha"); + expect(result.errors.get("broken")).toBe('Server "broken": stdio server requires "command" field'); + expect(events).toEqual([ + { type: "connecting", serverNames: ["alpha", "broken"] }, + { type: "failed", serverName: "broken", error: 'Server "broken": stdio server requires "command" field' }, + { type: "connected", serverName: "alpha" }, + ]); + } finally { + await manager.disconnectAll(); + } + }); +}); diff --git a/packages/coding-agent/test/mcp-startup-events.test.ts b/packages/coding-agent/test/mcp-startup-events.test.ts index e50a9a6c2..f79a6f61e 100644 --- a/packages/coding-agent/test/mcp-startup-events.test.ts +++ b/packages/coding-agent/test/mcp-startup-events.test.ts @@ -1,62 +1,81 @@ import { describe, expect, it } from "bun:test"; import { formatMCPConnectingMessage, - isMcpConnectingEvent, - MCP_CONNECTING_EVENT_CHANNEL, + formatMCPConnectionStatusMessage, + isMcpConnectionStatusEvent, + MCP_CONNECTION_STATUS_EVENT_CHANNEL, } from "@oh-my-pi/pi-coding-agent/mcp/startup-events"; // Cross-module contract guard. // -// The MCP "connecting" banner spans two modules that never import each other: -// - sdk.ts (onMCPConnecting) EMITS on MCP_CONNECTING_EVENT_CHANNEL. -// - interactive-mode.ts SUBSCRIBES to that same channel and renders the -// banner via showStatus(formatMCPConnectingMessage(serverNames)). +// The MCP status lifecycle spans two modules that never import each other: +// - sdk.ts emits McpConnectionStatusEvent payloads on MCP_CONNECTION_STATUS_EVENT_CHANNEL. +// - interactive-mode.ts subscribes to that channel and renders the aggregate +// message via formatMCPConnectionStatusMessage. // -// They agree only by sharing this module's two exports. Two drifts silently -// kill the banner with no type error and no crash: -// 1. the channel string diverging between emitter and subscriber, and -// 2. the user-facing banner text (esp. the exact trailing ellipsis char). -// These assertions pin both halves of that contract. -describe("mcp/startup-events — connecting-banner cross-module contract", () => { +// They agree only through this shared module. Drift in the channel, payload +// guard, or user-facing status text silently leaves the startup banner stale. +describe("mcp/startup-events — connection-status cross-module contract", () => { it("pins the wire channel string sdk(emit) and interactive-mode(subscribe) share", () => { - // A drift here desyncs publisher and subscriber: the event fires on one - // string, nobody listens on the other, and the banner vanishes silently. - expect(MCP_CONNECTING_EVENT_CHANNEL).toBe("mcp:connecting"); + expect(MCP_CONNECTION_STATUS_EVENT_CHANNEL).toBe("mcp:connection-status"); }); - it("formats the exact banner for a multi-server list (comma-joined names)", () => { + it("formats the initial connecting banner for a multi-server list", () => { expect(formatMCPConnectingMessage(["alpha", "beta", "gamma"])).toBe( "Connecting to MCP servers: alpha, beta, gamma…", ); }); - it("formats the exact banner for a single server (no separators)", () => { - expect(formatMCPConnectingMessage(["solo"])).toBe("Connecting to MCP servers: solo…"); + it("formats a completion update when every server connects", () => { + expect( + formatMCPConnectionStatusMessage({ + pendingServers: [], + connectedServers: ["alpha", "beta"], + failedServers: [], + }), + ).toBe("Connected to MCP servers: alpha, beta."); }); - it("terminates the banner with a single U+2026 ellipsis, not an ASCII '...'", () => { - // The source uses one HORIZONTAL ELLIPSIS codepoint. A refactor to "..." - // would still "look right" in a terminal but break exact-match expectations - // and any downstream byte-sensitive consumer, so guard the codepoint itself. + it("formats failures with server names and errors", () => { + expect( + formatMCPConnectionStatusMessage({ + pendingServers: [], + connectedServers: ["alpha"], + failedServers: [{ serverName: "broken", error: "missing command" }], + }), + ).toBe("MCP finished with failures. Connected: alpha. Failed: broken: missing command"); + }); + + it("keeps pending servers visible while other servers settle", () => { + expect( + formatMCPConnectionStatusMessage({ + pendingServers: ["slow"], + connectedServers: ["alpha"], + failedServers: [{ serverName: "broken", error: "missing command" }], + }), + ).toBe("Connected: alpha. Failed: broken: missing command. Still connecting: slow…"); + }); + + it("terminates active connecting messages with a single U+2026 ellipsis", () => { const msg = formatMCPConnectingMessage(["x"]); expect(msg.endsWith("\u2026")).toBe(true); expect(msg.endsWith("...")).toBe(false); expect(msg.at(-1)).toBe("\u2026"); }); - // The event bus is untyped at runtime, so the subscriber validates the payload - // with isMcpConnectingEvent before formatting instead of trusting a cast — a - // malformed emit must be rejected (ignored) rather than throwing in the handler. - it("accepts a well-formed payload and rejects malformed ones", () => { - expect(isMcpConnectingEvent({ serverNames: ["a", "b"] })).toBe(true); - expect(isMcpConnectingEvent({ serverNames: [] })).toBe(true); + it("accepts well-formed payloads and rejects malformed ones", () => { + expect(isMcpConnectionStatusEvent({ type: "connecting", serverNames: ["a", "b"] })).toBe(true); + expect(isMcpConnectionStatusEvent({ type: "connecting", serverNames: [] })).toBe(true); + expect(isMcpConnectionStatusEvent({ type: "connected", serverName: "a" })).toBe(true); + expect(isMcpConnectionStatusEvent({ type: "failed", serverName: "a", error: "boom" })).toBe(true); - expect(isMcpConnectingEvent(null)).toBe(false); - expect(isMcpConnectingEvent(undefined)).toBe(false); - expect(isMcpConnectingEvent("mcp:connecting")).toBe(false); - expect(isMcpConnectingEvent({})).toBe(false); - expect(isMcpConnectingEvent({ serverNames: "alpha" })).toBe(false); - expect(isMcpConnectingEvent({ serverNames: [1, 2] })).toBe(false); - expect(isMcpConnectingEvent({ serverNames: ["ok", 3] })).toBe(false); + expect(isMcpConnectionStatusEvent(null)).toBe(false); + expect(isMcpConnectionStatusEvent(undefined)).toBe(false); + expect(isMcpConnectionStatusEvent("mcp:connection-status")).toBe(false); + expect(isMcpConnectionStatusEvent({})).toBe(false); + expect(isMcpConnectionStatusEvent({ type: "connecting", serverNames: "alpha" })).toBe(false); + expect(isMcpConnectionStatusEvent({ type: "connecting", serverNames: ["ok", 3] })).toBe(false); + expect(isMcpConnectionStatusEvent({ type: "connected", serverName: 1 })).toBe(false); + expect(isMcpConnectionStatusEvent({ type: "failed", serverName: "a" })).toBe(false); }); });