fix(coding-agent): route MCP connecting banner through the render tree
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.
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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")
|
||||
);
|
||||
}
|
||||
@@ -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"));
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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
|
||||
});
|
||||
});
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user