From 8e21f4b41d599c2ff2ac91d6b5c2cf4c4f84615f Mon Sep 17 00:00:00 2001 From: metaphorics <152830360+metaphorics@users.noreply.github.com> Date: Sun, 14 Jun 2026 05:15:29 +0900 Subject: [PATCH] fix(coding-agent): route MCP connecting banner through the render tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deferred MCP discovery wrote 'Connecting to MCP servers: …' straight to process.stderr while the TUI owned the terminal, overdrawing the chat input box border. onMCPConnecting now emits McpConnectingEvent on the mcp:connecting channel; InteractiveMode subscribes and renders it via showStatus (status container), mirroring the LSP-startup pattern. New mcp/startup-events.ts holds the channel, type, and formatMCPConnectingMessage. --- packages/coding-agent/CHANGELOG.md | 3 + .../coding-agent/src/mcp/startup-events.ts | 21 +++ .../src/modes/interactive-mode.ts | 10 ++ packages/coding-agent/src/sdk.ts | 8 +- .../interactive-mode-mcp-connecting.test.ts | 121 ++++++++++++++++++ .../test/mcp-startup-events.test.ts | 62 +++++++++ 6 files changed, 219 insertions(+), 6 deletions(-) create mode 100644 packages/coding-agent/src/mcp/startup-events.ts create mode 100644 packages/coding-agent/test/interactive-mode-mcp-connecting.test.ts create mode 100644 packages/coding-agent/test/mcp-startup-events.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 55fe1bc60..bf5f625e3 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,6 +1,9 @@ # Changelog ## [Unreleased] +### Fixed + +- Fixed the deferred MCP discovery banner (`Connecting to MCP servers: …`) overdrawing the chat input bar. `onMCPConnecting` wrote the banner straight to `process.stderr` while the TUI owned the terminal; it now emits on an `mcp:connecting` event channel that `InteractiveMode` renders through `showStatus` (the status container), mirroring the existing LSP-startup pattern so the banner can never paint over the input box border ([#2483](https://github.com/can1357/oh-my-pi/issues/2483)) ### Breaking Changes diff --git a/packages/coding-agent/src/mcp/startup-events.ts b/packages/coding-agent/src/mcp/startup-events.ts new file mode 100644 index 000000000..16f18d371 --- /dev/null +++ b/packages/coding-agent/src/mcp/startup-events.ts @@ -0,0 +1,21 @@ +export const MCP_CONNECTING_EVENT_CHANNEL = "mcp:connecting"; + +export type McpConnectingEvent = { serverNames: string[] }; + +export function formatMCPConnectingMessage(serverNames: string[]): string { + return `Connecting to MCP servers: ${serverNames.join(", ")}…`; +} + +/** + * Runtime validator for the cross-module event payload. The event bus is + * 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") + ); +} diff --git a/packages/coding-agent/src/modes/interactive-mode.ts b/packages/coding-agent/src/modes/interactive-mode.ts index be7bf1645..2775bb9bf 100644 --- a/packages/coding-agent/src/modes/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive-mode.ts @@ -69,6 +69,7 @@ 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 { humanizePlanTitle, type PlanApprovalDetails, @@ -538,6 +539,15 @@ export class InteractiveMode implements InteractiveModeContext { this.#handleLspStartupEvent(data as LspStartupEvent); }), ); + this.#eventBusUnsubscribers.push( + eventBus.on(MCP_CONNECTING_EVENT_CHANNEL, data => { + if (!isMcpConnectingEvent(data)) { + logger.warn("Ignoring malformed mcp:connecting event", { data }); + return; + } + this.showStatus(formatMCPConnectingMessage(data.serverNames)); + }), + ); } this.ui = new TUI(new ProcessTerminal(), settings.get("showHardwareCursor")); diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index db0b86993..ca83dd942 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -34,7 +34,6 @@ import { prompt, Snowflake, } from "@oh-my-pi/pi-utils"; -import chalk from "chalk"; import { type AsyncJob, AsyncJobManager } from "./async"; import { AutoLearnController, buildAutoLearnInstructions } from "./autolearn/controller"; import { loadCapability } from "./capability"; @@ -98,6 +97,7 @@ import { type MCPToolsLoadResult, parseMCPToolName, } from "./mcp"; +import { MCP_CONNECTING_EVENT_CHANNEL, type McpConnectingEvent } 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" }; @@ -319,10 +319,6 @@ type DeferredMCPActivation = { activateAllMCPTools: boolean; }; -function formatMCPConnectingMessage(serverNames: string[]): string { - return `Connecting to MCP servers: ${serverNames.join(", ")}…`; -} - function createPendingMCPTool(name: string): Tool { const parsed = parseMCPToolName(name); const serverName = parsed?.serverName; @@ -1602,7 +1598,7 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} | undefined; const onMCPConnecting = (serverNames: string[]) => { if (!options.hasUI || serverNames.length === 0) return; - process.stderr.write(`${chalk.gray(formatMCPConnectingMessage(serverNames))}\n`); + eventBus.emit(MCP_CONNECTING_EVENT_CHANNEL, { serverNames } satisfies McpConnectingEvent); }; const mcpDiscoverOptions = { onConnecting: onMCPConnecting, diff --git a/packages/coding-agent/test/interactive-mode-mcp-connecting.test.ts b/packages/coding-agent/test/interactive-mode-mcp-connecting.test.ts new file mode 100644 index 000000000..166a2394b --- /dev/null +++ b/packages/coding-agent/test/interactive-mode-mcp-connecting.test.ts @@ -0,0 +1,121 @@ +import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "bun:test"; +import * as path from "node:path"; +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, +} 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"; +import { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session"; +import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; +import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; +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. + */ +describe("InteractiveMode MCP connecting banner", () => { + let authStorage: AuthStorage; + let eventBus: EventBus; + let mode: InteractiveMode; + let session: AgentSession; + let tempDir: TempDir; + + beforeAll(() => { + initTheme(); + }); + + beforeEach(async () => { + // Keep ProcessTerminal.start() from probing the real terminal; the test + // only drives the event bus and spies on showStatus. + vi.spyOn(process.stdout, "write").mockReturnValue(true); + vi.spyOn(process.stdin, "resume").mockReturnValue(process.stdin); + vi.spyOn(process.stdin, "pause").mockReturnValue(process.stdin); + vi.spyOn(process.stdin, "setEncoding").mockReturnValue(process.stdin); + if (typeof process.stdin.setRawMode === "function") { + vi.spyOn(process.stdin, "setRawMode").mockReturnValue(process.stdin); + } + + resetSettingsForTest(); + tempDir = TempDir.createSync("@pi-interactive-mode-mcp-connecting-"); + await Settings.init({ inMemory: true, cwd: tempDir.path() }); + authStorage = await AuthStorage.create(path.join(tempDir.path(), "testauth.db")); + const modelRegistry = new ModelRegistry(authStorage); + const model = modelRegistry.find("anthropic", "claude-sonnet-4-5"); + if (!model) { + throw new Error("Expected claude-sonnet-4-5 to exist in registry"); + } + + session = new AgentSession({ + agent: new Agent({ + initialState: { + model, + systemPrompt: ["Test"], + tools: [], + messages: [], + }, + }), + sessionManager: SessionManager.create(tempDir.path(), tempDir.path()), + settings: Settings.isolated(), + modelRegistry, + }); + eventBus = new EventBus(); + mode = new InteractiveMode(session, "test", undefined, () => {}, [], undefined, eventBus); + // This contract is the banner wiring, not git branch watching; a real + // fs.watch in a parallel Bun worker can trip an unrelated-worker SIGTRAP. + vi.spyOn(mode.statusLine, "watchBranch").mockImplementation(() => {}); + }); + + afterEach(async () => { + mode?.stop(); + vi.restoreAllMocks(); + await session?.dispose(); + authStorage?.close(); + tempDir?.removeSync(); + 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. + const showStatusSpy = vi.spyOn(mode, "showStatus").mockImplementation(() => {}); + + const serverNames = ["sequential", "critic", "shannon"]; + eventBus.emit(MCP_CONNECTING_EVENT_CHANNEL, { serverNames } satisfies McpConnectingEvent); + + // 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)); + }); + + it("rejects a malformed mcp:connecting 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" }); + + expect(showStatusSpy).not.toHaveBeenCalled(); + expect(warnSpy).toHaveBeenCalled(); // guard took the reject branch + expect(errorSpy).not.toHaveBeenCalled(); // no swallowed TypeError from a bad cast + }); +}); diff --git a/packages/coding-agent/test/mcp-startup-events.test.ts b/packages/coding-agent/test/mcp-startup-events.test.ts new file mode 100644 index 000000000..e50a9a6c2 --- /dev/null +++ b/packages/coding-agent/test/mcp-startup-events.test.ts @@ -0,0 +1,62 @@ +import { describe, expect, it } from "bun:test"; +import { + formatMCPConnectingMessage, + isMcpConnectingEvent, + MCP_CONNECTING_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)). +// +// 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", () => { + 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"); + }); + + it("formats the exact banner for a multi-server list (comma-joined names)", () => { + 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("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. + 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); + + 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); + }); +});