fix(mcp): show config source in startup failures
This commit is contained in:
@@ -2,6 +2,10 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed MCP startup failures omitting the originating configuration path, so stale imported server entries can be traced to their source file.
|
||||
|
||||
## [17.2.15] - 2026-08-12
|
||||
|
||||
### Added
|
||||
|
||||
@@ -74,6 +74,12 @@ type TrackedPromise<T> = {
|
||||
|
||||
const STARTUP_TIMEOUT_MS = 250;
|
||||
|
||||
function createMcpStartupFailure(serverName: string, error: string, source?: SourceMeta): McpConnectionStatusEvent {
|
||||
return source
|
||||
? { type: "failed", serverName, error, sourcePath: source.path }
|
||||
: { type: "failed", serverName, error };
|
||||
}
|
||||
|
||||
/**
|
||||
* Per-server reconnect-storm circuit breaker.
|
||||
*
|
||||
@@ -563,7 +569,7 @@ export class MCPManager {
|
||||
if (this.#pendingToolLoads.get(name) !== toolsPromise) return;
|
||||
this.#pendingToolLoads.delete(name);
|
||||
const message = error instanceof Error ? error.message : String(error);
|
||||
onStatus?.({ type: "failed", serverName: name, error: message });
|
||||
onStatus?.(createMcpStartupFailure(name, message, sources[name]));
|
||||
if (!allowBackgroundLogging || reportedErrors.has(name)) return;
|
||||
logger.error("MCP tool load failed", { path: `mcp:${name}`, error: message });
|
||||
});
|
||||
@@ -573,7 +579,7 @@ export class MCPManager {
|
||||
if (statusServerNames.length > 0 && onStatus) {
|
||||
onStatus({ type: "connecting", serverNames: statusServerNames });
|
||||
for (const { name, message } of validationFailures) {
|
||||
onStatus({ type: "failed", serverName: name, error: message });
|
||||
onStatus(createMcpStartupFailure(name, message, sources[name]));
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -3,15 +3,21 @@ import { replaceTabs, shortenPath, TRUNCATE_LENGTHS, truncateToWidth } from "../
|
||||
|
||||
export const MCP_CONNECTION_STATUS_EVENT_CHANNEL = "mcp:connection-status";
|
||||
|
||||
export type McpConnectionFailure = {
|
||||
serverName: string;
|
||||
error: string;
|
||||
sourcePath?: string;
|
||||
};
|
||||
|
||||
export type McpConnectionStatusEvent =
|
||||
| { type: "connecting"; serverNames: string[] }
|
||||
| { type: "connected"; serverName: string }
|
||||
| { type: "failed"; serverName: string; error: string };
|
||||
| ({ type: "failed" } & McpConnectionFailure);
|
||||
|
||||
export type McpConnectionStatusSnapshot = {
|
||||
pendingServers: readonly string[];
|
||||
connectedServers: readonly string[];
|
||||
failedServers: readonly { serverName: string; error: string }[];
|
||||
failedServers: readonly McpConnectionFailure[];
|
||||
};
|
||||
|
||||
function sanitizeMcpStatusText(value: string, maxWidth: number): string {
|
||||
@@ -55,8 +61,9 @@ export function formatMCPConnectingMessage(serverNames: readonly string[]): stri
|
||||
return `Connecting to MCP servers: ${formatServerList(serverNames)}…`;
|
||||
}
|
||||
|
||||
function formatFailedServer({ serverName, error }: { serverName: string; error: string }): string {
|
||||
return `${sanitizeMcpServerName(serverName)}: ${sanitizeMcpStatusError(error)}`;
|
||||
function formatFailedServer({ serverName, error, sourcePath }: McpConnectionFailure): string {
|
||||
const source = sourcePath ? ` [config: ${sanitizeMcpStatusText(sourcePath, TRUNCATE_LENGTHS.CONTENT)}]` : "";
|
||||
return `${sanitizeMcpServerName(serverName)}${source}: ${sanitizeMcpStatusError(error)}`;
|
||||
}
|
||||
|
||||
export function formatMCPConnectionStatusMessage(snapshot: McpConnectionStatusSnapshot): string {
|
||||
@@ -109,7 +116,11 @@ export function isMcpConnectionStatusEvent(data: unknown): data is McpConnection
|
||||
case "connected":
|
||||
return typeof data.serverName === "string";
|
||||
case "failed":
|
||||
return typeof data.serverName === "string" && typeof data.error === "string";
|
||||
return (
|
||||
typeof data.serverName === "string" &&
|
||||
typeof data.error === "string" &&
|
||||
(data.sourcePath === undefined || typeof data.sourcePath === "string")
|
||||
);
|
||||
default:
|
||||
return false;
|
||||
}
|
||||
|
||||
@@ -88,6 +88,7 @@ import {
|
||||
formatMCPConnectionStatusMessage,
|
||||
isMcpConnectionStatusEvent,
|
||||
MCP_CONNECTION_STATUS_EVENT_CHANNEL,
|
||||
type McpConnectionFailure,
|
||||
type McpConnectionStatusEvent,
|
||||
} from "../mcp/startup-events";
|
||||
import { humanizePlanTitle, type PlanApprovalDetails, resolvePlanTitle } from "../plan-mode/approved-plan";
|
||||
@@ -723,7 +724,7 @@ export class InteractiveMode implements InteractiveModeContext {
|
||||
#mcpStatusOrder: string[] = [];
|
||||
#mcpPendingServers = new Set<string>();
|
||||
#mcpConnectedServers = new Set<string>();
|
||||
#mcpFailedServers = new Map<string, string>();
|
||||
#mcpFailedServers = new Map<string, { error: string; sourcePath?: string }>();
|
||||
#welcomeComponent?: WelcomeComponent;
|
||||
readonly #chatHost: ChatBlockHost = { requestRender: () => this.ui.requestRender() };
|
||||
|
||||
@@ -897,7 +898,10 @@ export class InteractiveMode implements InteractiveModeContext {
|
||||
this.#trackMcpStatusServer(event.serverName);
|
||||
this.#mcpPendingServers.delete(event.serverName);
|
||||
this.#mcpConnectedServers.delete(event.serverName);
|
||||
this.#mcpFailedServers.set(event.serverName, event.error);
|
||||
this.#mcpFailedServers.set(event.serverName, {
|
||||
error: event.error,
|
||||
sourcePath: event.sourcePath,
|
||||
});
|
||||
}
|
||||
|
||||
const message = formatMCPConnectionStatusMessage({
|
||||
@@ -918,10 +922,10 @@ export class InteractiveMode implements InteractiveModeContext {
|
||||
return this.#mcpStatusOrder.filter(serverName => servers.has(serverName));
|
||||
}
|
||||
|
||||
#orderedMcpStatusFailures(): Array<{ serverName: string; error: string }> {
|
||||
#orderedMcpStatusFailures(): McpConnectionFailure[] {
|
||||
return this.#mcpStatusOrder.flatMap(serverName => {
|
||||
const error = this.#mcpFailedServers.get(serverName);
|
||||
return error === undefined ? [] : [{ serverName, error }];
|
||||
const failure = this.#mcpFailedServers.get(serverName);
|
||||
return failure === undefined ? [] : [{ serverName, ...failure }];
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
@@ -127,6 +127,7 @@ describe("InteractiveMode MCP connection status", () => {
|
||||
type: "failed",
|
||||
serverName: "broken",
|
||||
error: "missing command",
|
||||
sourcePath: "/tmp/codex/config.toml",
|
||||
} satisfies McpConnectionStatusEvent);
|
||||
eventBus.emit(MCP_CONNECTION_STATUS_EVENT_CHANNEL, {
|
||||
type: "connected",
|
||||
@@ -136,8 +137,8 @@ describe("InteractiveMode MCP connection status", () => {
|
||||
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",
|
||||
"Connected: alpha. Failed: broken [config: /tmp/codex/config.toml]: missing command. Still connecting: slow…",
|
||||
"MCP finished with failures. Connected: alpha, slow. Failed: broken [config: /tmp/codex/config.toml]: missing command",
|
||||
]);
|
||||
});
|
||||
|
||||
|
||||
@@ -47,4 +47,45 @@ describe("MCPManager connection status events", () => {
|
||||
await manager.disconnectAll();
|
||||
}
|
||||
});
|
||||
|
||||
it("includes the originating config path when a discovered server fails to start", async () => {
|
||||
const manager = new MCPManager(workDir);
|
||||
const events: McpConnectionStatusEvent[] = [];
|
||||
const missingCommand = path.join(workDir, "missing-mcp-server");
|
||||
const configPath = path.join(os.homedir(), ".codex", "config.toml");
|
||||
|
||||
try {
|
||||
const result = await manager.connectServers(
|
||||
{
|
||||
broken: {
|
||||
type: "stdio",
|
||||
command: missingCommand,
|
||||
},
|
||||
},
|
||||
{
|
||||
broken: {
|
||||
provider: "codex",
|
||||
providerName: "Codex",
|
||||
path: configPath,
|
||||
level: "user",
|
||||
},
|
||||
},
|
||||
event => events.push(event),
|
||||
);
|
||||
|
||||
const message = result.errors.get("broken") ?? "";
|
||||
expect(message).toMatch(/ENOENT|No such file|not found/i);
|
||||
expect(events).toEqual([
|
||||
{ type: "connecting", serverNames: ["broken"] },
|
||||
{
|
||||
type: "failed",
|
||||
serverName: "broken",
|
||||
error: message,
|
||||
sourcePath: configPath,
|
||||
},
|
||||
]);
|
||||
} finally {
|
||||
await manager.disconnectAll();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -61,6 +61,24 @@ describe("mcp/startup-events — connection-status cross-module contract", () =>
|
||||
expect(message).toContain("broken: failed at ~/.omp/mcp.log");
|
||||
});
|
||||
|
||||
it("keeps the config source and transport error visible under independent truncation", () => {
|
||||
const message = formatMCPConnectionStatusMessage({
|
||||
pendingServers: [],
|
||||
connectedServers: [],
|
||||
failedServers: [
|
||||
{
|
||||
serverName: "broken",
|
||||
error: `ENOENT ${"missing executable ".repeat(10)}`,
|
||||
sourcePath: `${os.homedir()}/.codex/config.toml`,
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
expect(message).not.toContain(os.homedir());
|
||||
expect(message).toContain("broken [config: ~/.codex/config.toml]: ENOENT");
|
||||
expect(message).toContain("…");
|
||||
});
|
||||
|
||||
it("sanitizes server names before rendering them in status text", () => {
|
||||
const homePath = `${os.homedir()}/.omp`;
|
||||
const message = formatMCPConnectionStatusMessage({
|
||||
@@ -99,6 +117,14 @@ describe("mcp/startup-events — connection-status cross-module contract", () =>
|
||||
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(
|
||||
isMcpConnectionStatusEvent({
|
||||
type: "failed",
|
||||
serverName: "a",
|
||||
error: "boom",
|
||||
sourcePath: "/tmp/config.toml",
|
||||
}),
|
||||
).toBe(true);
|
||||
|
||||
expect(isMcpConnectionStatusEvent(null)).toBe(false);
|
||||
expect(isMcpConnectionStatusEvent(undefined)).toBe(false);
|
||||
@@ -108,5 +134,13 @@ describe("mcp/startup-events — connection-status cross-module contract", () =>
|
||||
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);
|
||||
expect(
|
||||
isMcpConnectionStatusEvent({
|
||||
type: "failed",
|
||||
serverName: "a",
|
||||
error: "boom",
|
||||
sourcePath: 42,
|
||||
}),
|
||||
).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user