From 655fed1e488efa32159d7553e22423cc5576723b Mon Sep 17 00:00:00 2001 From: roboomp Date: Sat, 20 Jun 2026 21:14:41 +0000 Subject: [PATCH] fix(mcp): updated startup connection status Emitted MCP connection lifecycle events through the startup event bus so the TUI can replace the initial connecting banner with connected, pending, or failed server state. Added manager and interactive-mode coverage for mixed success/failure MCP startup updates. Fixes #3150 --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/mcp/loader.ts | 7 +- packages/coding-agent/src/mcp/manager.ts | 50 +++++++---- .../coding-agent/src/mcp/startup-events.ts | 81 ++++++++++++++--- .../src/modes/interactive-mode.ts | 68 ++++++++++++-- packages/coding-agent/src/sdk.ts | 11 +-- .../interactive-mode-mcp-connecting.test.ts | 89 ++++++++++++------- .../test/mcp-connection-status-events.test.ts | 49 ++++++++++ .../test/mcp-startup-events.test.ts | 89 +++++++++++-------- 9 files changed, 337 insertions(+), 108 deletions(-) create mode 100644 packages/coding-agent/test/mcp-connection-status-events.test.ts 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); }); });