From 09378adbdc9a8ec13a3ce8b87cf447a159dd90ca Mon Sep 17 00:00:00 2001 From: Sawyer Date: Mon, 31 Aug 2026 09:34:19 -0700 Subject: [PATCH 1/6] Add MCP servers from the MCP surface --- docs/MCP.md | 8 + src/agent/exa-web-fetch-alias.test.ts | 330 ++++++++++++++- src/agent/tools.ts | 167 ++++++-- src/mcp/add-server.test.ts | 153 +++++++ src/mcp/add-server.ts | 130 ++++++ src/mcp/client-auth-policy.test.ts | 126 +++++- src/mcp/client.ts | 92 +++-- src/telemetry/toggle.ts | 9 +- src/tui/command-surfaces.test.ts | 557 +++++++++++++++++++++++++- src/tui/command-surfaces.ts | 141 ++++++- src/tui/commands/built-in.test.ts | 7 + src/tui/commands/built-in.ts | 12 +- src/tui/dynamic-tool-runner.ts | 11 +- src/tui/provider-connect.ts | 7 +- src/tui/provider-setup-submit.test.ts | 38 ++ src/tui/provider-setup-submit.ts | 15 +- src/tui/runner.ts | 235 ++++++----- src/tui/shell.ts | 129 ++++-- tests/unit/telemetry-toggle.test.ts | 62 +++ 19 files changed, 2003 insertions(+), 226 deletions(-) create mode 100644 src/mcp/add-server.test.ts create mode 100644 src/mcp/add-server.ts diff --git a/docs/MCP.md b/docs/MCP.md index 83a770803..96399a8b7 100644 --- a/docs/MCP.md +++ b/docs/MCP.md @@ -86,6 +86,14 @@ registered for dispatch as soon as the server connects (including later in the same turn) and surfaced on demand through dynamic tool discovery (`tool_search`). +In the TUI, `/mcp` and `/mcps` open the same live server surface. Press **Alt+A** +to add a named absolute HTTP(S) endpoint to global settings and connect it in the +current session. Names may contain letters, numbers, single underscores, and +hyphens; the `__` tool-namespace delimiter is reserved. The add is unavailable +while a local `.corbits/settings.json` `mcpServers` list shadows global MCP +settings; remove that local list and restart +before adding globally. A connection failure does not remove the saved server. + ## Server Kinds A server is reached one of two ways: diff --git a/src/agent/exa-web-fetch-alias.test.ts b/src/agent/exa-web-fetch-alias.test.ts index 0b944688e..2edc9b2e2 100644 --- a/src/agent/exa-web-fetch-alias.test.ts +++ b/src/agent/exa-web-fetch-alias.test.ts @@ -6,18 +6,52 @@ import type { ToolResult } from "@intx/types/runtime"; import { stringTool, type AgentTool } from "@intx/agent"; import { withMockedModule } from "../../tests/helpers/mock-module.js"; import type { ResolvedMCPServerConfig } from "../mcp/exa.js"; +import type { MCPConnectOptions } from "../mcp/client.js"; +import { createGlobalSettingsWriter, persistGlobalHTTPMCPServer } from "../mcp/add-server.js"; import { createPermissionGate } from "../permission/gate.js"; const calls: { toolName: string; args: Record; signal: AbortSignal }[] = []; +const closedClients: string[] = []; let connectConfigs: ResolvedMCPServerConfig[] = []; -let connectMode: "success" | "missing-fetch" | "failed" = "success"; +let connectOptions: MCPConnectOptions[] = []; +let releaseDeferredConnect: (() => void) | undefined; +let authWaitAborts = 0; +let authResourceCloses = 0; +let blockInteractiveAuth = false; +let connectMode: "success" | "missing-fetch" | "failed" | "rejected" | "auth" | "deferred" = + "success"; await withMockedModule( import.meta.resolve("../mcp/client.js"), (real: typeof import("../mcp/client.js")) => ({ ...real, - connectMCPServer: async (config: ResolvedMCPServerConfig) => { + connectMCPServer: async (config: ResolvedMCPServerConfig, options: MCPConnectOptions = {}) => { connectConfigs.push(config); + connectOptions.push(options); + if (connectMode === "auth" || blockInteractiveAuth) { + options.onAuthURL?.(config.name, "https://auth.test/authorize"); + } + if (blockInteractiveAuth) { + await new Promise((resolve) => { + const onAbort = (): void => { + authWaitAborts += 1; + authResourceCloses += 1; + resolve(); + }; + if (options.signal?.aborted === true) { + onAbort(); + } else { + options.signal?.addEventListener("abort", onAbort, { once: true }); + } + }); + return { ok: false, serverName: config.name, error: "authorization aborted" }; + } + if (connectMode === "deferred") { + await new Promise((resolve) => { + releaseDeferredConnect = resolve; + }); + } + if (connectMode === "rejected") throw new Error("transport setup exploded"); if (connectMode === "failed") { return { ok: false, serverName: config.name, error: "connection exploded" }; } @@ -36,7 +70,9 @@ await withMockedModule( calls.push({ toolName, args, signal }); return "exa fetch result"; }, - close: async () => undefined, + close: async () => { + closedClients.push(config.name); + }, }, }; }, @@ -51,10 +87,13 @@ function permissionGate() { return createPermissionGate({ approvals: [], interactive: false, skipPermissions: true }); } -async function makeToolset(mcpServers = resolveMcpServers(undefined, undefined)) { +async function makeToolset( + mcpServers = resolveMcpServers(undefined, undefined), + gate = permissionGate(), +) { return createAgentToolset({ cwd: mkdtempSync(join(tmpdir(), "corbits-exa-fetch-alias-")), - permissionGate: permissionGate(), + permissionGate: gate, onOperatorGate: async () => ({ kind: "cancel" }), mcpServers, }); @@ -79,7 +118,13 @@ async function runTool( beforeEach(() => { calls.length = 0; + closedClients.length = 0; connectConfigs = []; + connectOptions = []; + releaseDeferredConnect = undefined; + authWaitAborts = 0; + authResourceCloses = 0; + blockInteractiveAuth = false; connectMode = "success"; }); @@ -206,6 +251,281 @@ describe("built-in Exa web_fetch alias", () => { } }); + test("single-server connection deduplicates and hands OAuth status through", async () => { + connectMode = "auth"; + const toolset = await makeToolset( + resolveMcpServers([{ name: "exa", enabled: false }], undefined), + ); + const states: { state: string; url?: string }[] = []; + const callbacks = { + interactiveAuth: true, + onStatus: (status: { state: string; url?: string }) => states.push(status), + onToolsChanged: () => undefined, + }; + const server = { name: "linear", type: "http" as const, url: "https://mcp.linear.app/mcp" }; + try { + await Promise.all([ + toolset.connectMCPServer(server, callbacks), + toolset.connectMCPServer(server, callbacks), + ]); + await toolset.connectMCPServer(server, callbacks); + + expect(connectConfigs).toEqual([server]); + expect(states.map((status) => status.state)).toEqual([ + "connecting", + "needs-auth", + "connected", + ]); + expect(states[1]?.url).toBe("https://auth.test/authorize"); + expect(connectOptions[0]?.onAuthURL).toBeDefined(); + } finally { + await toolset.dispose(); + } + }); + + test("dispose invalidates an in-flight connection and closes its late client", async () => { + connectMode = "deferred"; + const toolset = await makeToolset( + resolveMcpServers([{ name: "exa", enabled: false }], undefined), + ); + const states: string[] = []; + const connection = toolset.connectMCPServer( + { name: "linear", type: "http", url: "https://mcp.linear.app/mcp" }, + { + interactiveAuth: true, + onStatus: (status) => states.push(status.state), + onToolsChanged: () => undefined, + }, + ); + await Promise.resolve(); + + let disposed = false; + const disposal = toolset.dispose().then(() => { + disposed = true; + }); + await Promise.resolve(); + expect(disposed).toBe(false); + releaseDeferredConnect?.(); + await Promise.all([connection, disposal]); + + expect(states).toEqual(["connecting"]); + expect(closedClients).toEqual(["linear"]); + expect( + toolset.dynamicRunner.currentDefinitions().some((tool) => tool.name.includes("linear")), + ).toBe(false); + }); + + test("dispose aborts blocked interactive auth and closes its resources", async () => { + blockInteractiveAuth = true; + const toolset = await makeToolset( + resolveMcpServers([{ name: "exa", enabled: false }], undefined), + ); + const callerAbort = new AbortController(); + const states: string[] = []; + const connection = toolset.connectMCPServer( + { name: "linear", type: "http", url: "https://mcp.linear.app/mcp" }, + { + interactiveAuth: true, + onStatus: (status) => states.push(status.state), + onToolsChanged: () => undefined, + }, + callerAbort.signal, + ); + while (connectOptions.length === 0) await Promise.resolve(); + + const ownedSignal = connectOptions[0]?.signal; + expect(ownedSignal).toBeDefined(); + expect(ownedSignal).not.toBe(callerAbort.signal); + const disposal = toolset.dispose(); + expect(toolset.dispose()).toBe(disposal); + await Promise.resolve(); + expect(ownedSignal?.aborted).toBe(true); + expect(callerAbort.signal.aborted).toBe(false); + await Promise.all([connection, disposal]); + + expect(authWaitAborts).toBe(1); + expect(authResourceCloses).toBe(1); + expect(states).toEqual(["connecting", "needs-auth"]); + expect( + toolset.dynamicRunner.currentDefinitions().some((tool) => tool.name.includes("linear")), + ).toBe(false); + }); + + test("rejects connected and in-flight implicit Exa names before persistence", async () => { + const connected = await makeToolset(); + const connectedPath = join(mkdtempSync(join(tmpdir(), "corbits-mcp-active-")), "settings.json"); + try { + await connect(connected); + expect(connected.hasMCPServer("exa")).toBe(true); + expect( + await persistGlobalHTTPMCPServer( + createGlobalSettingsWriter(connectedPath), + "exa", + "https://custom.test/mcp", + "none", + connected.hasMCPServer, + ), + ).toEqual({ ok: false, reason: "active" }); + expect(await Bun.file(connectedPath).exists()).toBe(false); + } finally { + await connected.dispose(); + } + + connectMode = "deferred"; + const inFlight = await makeToolset(); + const inFlightPath = join(mkdtempSync(join(tmpdir(), "corbits-mcp-active-")), "settings.json"); + const startup = connect(inFlight); + while (releaseDeferredConnect === undefined) await Promise.resolve(); + try { + expect(inFlight.hasMCPServer("exa")).toBe(true); + expect( + await persistGlobalHTTPMCPServer( + createGlobalSettingsWriter(inFlightPath), + "exa", + "https://custom.test/mcp", + "none", + inFlight.hasMCPServer, + ), + ).toEqual({ ok: false, reason: "active" }); + expect(await Bun.file(inFlightPath).exists()).toBe(false); + } finally { + releaseDeferredConnect?.(); + await startup; + await inFlight.dispose(); + } + }); + + test("failed implicit Exa remains reserved and cannot be persisted explicitly", async () => { + connectMode = "failed"; + const toolset = await makeToolset(); + const path = join(mkdtempSync(join(tmpdir(), "corbits-mcp-failed-exa-")), "settings.json"); + try { + await connect(toolset); + expect(toolset.hasMCPServer("exa")).toBe(true); + expect( + await persistGlobalHTTPMCPServer( + createGlobalSettingsWriter(path), + "exa", + "https://custom.test/mcp", + "none", + toolset.hasMCPServer, + ), + ).toEqual({ ok: false, reason: "active" }); + expect(await Bun.file(path).exists()).toBe(false); + } finally { + await toolset.dispose(); + } + }); + + test("single-server registration failure closes the client and reports failed", async () => { + const gate = permissionGate(); + gate.registerMcpClient = () => { + throw new Error("registration exploded"); + }; + const toolset = await makeToolset( + resolveMcpServers([{ name: "exa", enabled: false }], undefined), + gate, + ); + const states: { state: string; error?: string }[] = []; + try { + await toolset.connectMCPServer( + { name: "linear", type: "http", url: "https://mcp.linear.app/mcp" }, + { + interactiveAuth: true, + onStatus: (status) => states.push(status), + onToolsChanged: () => undefined, + }, + ); + + expect(states.map((status) => status.state)).toEqual(["connecting", "failed"]); + expect(states[1]?.error).toContain("registration exploded"); + expect(closedClients).toEqual(["linear"]); + } finally { + await toolset.dispose(); + } + }); + + test("rejected single-server connection reports failed without registration or client leaks", async () => { + connectMode = "rejected"; + const gate = permissionGate(); + let registrations = 0; + let unregistrations = 0; + gate.registerMcpClient = () => { + registrations += 1; + }; + gate.unregisterMcpServer = () => { + unregistrations += 1; + }; + const toolset = await makeToolset( + resolveMcpServers([{ name: "exa", enabled: false }], undefined), + gate, + ); + const server = { name: "linear", type: "http" as const, url: "https://mcp.linear.app/mcp" }; + const states: { state: string; error?: string }[] = []; + try { + await toolset.connectMCPServer(server, { + interactiveAuth: true, + onStatus: (status) => states.push(status), + onToolsChanged: () => undefined, + }); + + expect(states.map((status) => status.state)).toEqual(["connecting", "failed"]); + expect(states[1]?.error).toContain("transport setup exploded"); + expect(registrations).toBe(0); + expect(unregistrations).toBe(0); + expect(closedClients).toEqual([]); + expect( + toolset.dynamicRunner.currentDefinitions().some((tool) => tool.name.includes("linear")), + ).toBe(false); + expect(toolset.hasMCPServer("linear")).toBe(true); + + connectMode = "failed"; + const retryStates: string[] = []; + await toolset.connectMCPServer(server, { + interactiveAuth: true, + onStatus: (status) => retryStates.push(status.state), + onToolsChanged: () => undefined, + }); + expect(retryStates).toEqual(["connecting", "failed"]); + expect(connectConfigs).toEqual([server, server]); + } finally { + await toolset.dispose(); + } + }); + + test("connection failure leaves the late-added server persisted and reports failed", async () => { + connectMode = "failed"; + const dir = mkdtempSync(join(tmpdir(), "corbits-mcp-failure-")); + const path = join(dir, "settings.json"); + const persisted = await persistGlobalHTTPMCPServer( + createGlobalSettingsWriter(path), + "linear", + "https://mcp.linear.app/mcp", + ); + expect(persisted.ok).toBe(true); + if (!persisted.ok) return; + + const toolset = await makeToolset( + resolveMcpServers([{ name: "exa", enabled: false }], undefined), + ); + const states: { state: string; error?: string }[] = []; + try { + await toolset.connectMCPServer(persisted.server, { + interactiveAuth: true, + onStatus: (status) => states.push(status), + onToolsChanged: () => undefined, + }); + + expect(states.map((status) => status.state)).toEqual(["connecting", "failed"]); + expect(states[1]?.error).toContain("connection exploded"); + expect(await Bun.file(path).json()).toMatchObject({ + mcpServers: [{ name: "linear", type: "http", url: "https://mcp.linear.app/mcp" }], + }); + } finally { + await toolset.dispose(); + } + }); + test("child assembly keeps inherited canonical web_fetch and avoids duplicate native fetch", () => { const inherited: AgentTool[] = [ stringTool({ diff --git a/src/agent/tools.ts b/src/agent/tools.ts index dcb3bc035..4a6d499a5 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -22,7 +22,11 @@ import { buildCorePosixToolPlugins } from "./posix-tool-plugins.js"; import { createLazyBlobReader } from "./lazy-blob-reader.js"; import type { BlobReader } from "@intx/types/runtime"; import type { SpillBlobWriter } from "../plugins/result-truncation-plugin.js"; -import { connectMCPServer, type MCPClient, type MCPConnectResult } from "../mcp/client.js"; +import { + connectMCPServer as connectMCPClient, + type MCPClient, + type MCPConnectResult, +} from "../mcp/client.js"; import { createExaMCPServerConfig, isBuiltinExaMCPServer } from "../mcp/exa.js"; import { mcpClientToAgentTools } from "../mcp/plugin.js"; import { createDynamicToolRunner, type DynamicToolRunner } from "../tui/dynamic-tool-runner.js"; @@ -203,6 +207,13 @@ export interface AgentToolset { // Connect configured MCP servers in the background. Resolves once every server // has either connected or failed; authorization waits are bounded by `signal`. connectMCP: (callbacks: MCPConnectCallbacks, signal?: AbortSignal) => Promise; + // Connect one newly persisted server through the same lifecycle as startup MCP. + connectMCPServer: ( + config: MCPServerConfig, + callbacks: MCPConnectCallbacks, + signal?: AbortSignal, + ) => Promise; + hasMCPServer: (name: string) => boolean; // Wire the callback the `tool_search` tool invokes to make matched tools // advertised. Set by the runner once the director + reload loop exist. setToolPromoter: (promote: (names: string[]) => void) => void; @@ -528,34 +539,47 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise server.name)); + const connectedClients = new Map(); + const inFlightConnections = new Map>(); + const mcpAbortController = new AbortController(); + let disposed = false; + let disposal: Promise | undefined; - const connectMCP = async ( + const connectOneMCPServer = ( + config: MCPServerConfig, callbacks: MCPConnectCallbacks, signal?: AbortSignal, ): Promise => { - const toConnect = await filterMcpServersForConnect(mcpServers, { - source: mcpServersSource, - store: projectTrust ?? { trustedPluginPaths: [], trustedMcpFingerprints: [] }, - cwd, - ...(requestMcpTrust !== undefined ? { requestTrust: requestMcpTrust } : {}), - }); - await Promise.all( - toConnect.map(async (config) => { - callbacks.onStatus({ name: config.name, state: "connecting" }); - const connection = connectMCPServer(config, { + if (disposed) return Promise.resolve(); + reservedMCPServerNames.add(config.name); + if (connectedClients.has(config.name)) return Promise.resolve(); + const existing = inFlightConnections.get(config.name); + if (existing !== undefined) return existing; + const connectionSignal = + signal === undefined + ? mcpAbortController.signal + : AbortSignal.any([mcpAbortController.signal, signal]); + + const run = (async () => { + callbacks.onStatus({ name: config.name, state: "connecting" }); + let result: MCPConnectResult; + try { + result = await connectMCPClient(config, { stderr: "ignore", ...(callbacks.interactiveAuth ? { - onAuthURL: (name: string, url: string) => - callbacks.onStatus({ name, state: "needs-auth", url }), + onAuthURL: (name: string, url: string) => { + if (!disposed) callbacks.onStatus({ name, state: "needs-auth", url }); + }, } : {}), // Mid-session re-auth fires needs-auth again without a later connected // event. Re-emit connected only when tools are already registered so // first-connect still waits for the real post-connect status. onAuthorized: (name) => { - const client = connectedClients.find((c) => c.serverName === name); + if (disposed) return; + const client = connectedClients.get(name); if (client === undefined) return; callbacks.onStatus({ name, @@ -563,33 +587,84 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise t.name), }); }, - ...(signal !== undefined ? { signal } : {}), + signal: connectionSignal, }); + } catch (err) { + const error = err instanceof Error ? err.message : String(err); if (isBuiltinExaMCPServer(config)) { - connection.then((result) => resolveBuiltinExaConnection?.(result)); + resolveBuiltinExaConnection?.({ ok: false, serverName: config.name, error }); } - const result = await connection; - if (!result.ok) { - callbacks.onStatus({ name: config.name, state: "failed", error: result.error }); - return; + if (!disposed) callbacks.onStatus({ name: config.name, state: "failed", error }); + return; + } + if (disposed) { + if (result.ok) await result.client.close().catch(() => undefined); + if (isBuiltinExaMCPServer(config)) { + resolveBuiltinExaConnection?.({ + ok: false, + serverName: config.name, + error: "MCP toolset disposed during connection", + }); } - connectedClients.push(result.client); + return; + } + if (!result.ok) { + if (isBuiltinExaMCPServer(config)) resolveBuiltinExaConnection?.(result); + callbacks.onStatus({ name: config.name, state: "failed", error: result.error }); + return; + } + + try { permissionGate.registerMcpClient(result.client); const mcpTools = mcpClientToAgentTools(result.client, permissionGate, { ...(getBlobWriter !== undefined ? { getBlobWriter } : {}), ...(getContextDir !== undefined ? { getContextDir } : {}), ...(isBuiltinExaMCPServer(config) ? { excludeToolNames: ["web_fetch_exa"] } : {}), }); - inheritedMcpTools.push(...mcpTools); dynamicRunner.addTools(mcpTools); - callbacks.onStatus({ - name: config.name, - state: "connected", - tools: result.client.tools.map((t) => t.name), - }); - callbacks.onToolsChanged(dynamicRunner.currentDefinitions()); - }), + inheritedMcpTools.push(...mcpTools); + connectedClients.set(config.name, result.client); + } catch (err) { + permissionGate.unregisterMcpServer(config.name); + await result.client.close().catch(() => undefined); + const error = err instanceof Error ? err.message : String(err); + if (isBuiltinExaMCPServer(config)) { + resolveBuiltinExaConnection?.({ ok: false, serverName: config.name, error }); + } + callbacks.onStatus({ name: config.name, state: "failed", error }); + return; + } + + if (isBuiltinExaMCPServer(config)) resolveBuiltinExaConnection?.(result); + callbacks.onStatus({ + name: config.name, + state: "connected", + tools: result.client.tools.map((t) => t.name), + }); + callbacks.onToolsChanged(dynamicRunner.currentDefinitions()); + })(); + inFlightConnections.set(config.name, run); + void run.then( + () => inFlightConnections.delete(config.name), + () => inFlightConnections.delete(config.name), ); + return run; + }; + + const connectMCP = async ( + callbacks: MCPConnectCallbacks, + signal?: AbortSignal, + ): Promise => { + if (disposed) return; + const toConnect = await filterMcpServersForConnect(mcpServers, { + source: mcpServersSource, + store: projectTrust ?? { trustedPluginPaths: [], trustedMcpFingerprints: [] }, + cwd, + ...(requestMcpTrust !== undefined ? { requestTrust: requestMcpTrust } : {}), + }); + if (disposed) return; + await Promise.all(toConnect.map((config) => connectOneMCPServer(config, callbacks, signal))); + if (disposed) return; // Report untrusted local servers as failed (fail closed) so the UI is honest. if (mcpServersSource === "local") { const connectedNames = new Set(toConnect.map((s) => s.name)); @@ -605,19 +680,33 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise => { + if (disposal !== undefined) return disposal; + disposed = true; + mcpAbortController.abort(new Error("MCP toolset disposed")); + disposal = (async () => { + await Promise.allSettled([...inFlightConnections.values()]); + for (const client of connectedClients.values()) { + permissionGate.unregisterMcpServer(client.serverName); + } + await Promise.all( + [...connectedClients.values()].map((client) => client.close().catch(() => undefined)), + ); + connectedClients.clear(); + await posixTools.dispose(); + await disposeWebSearchClients(); + })(); + return disposal; + }; + return { dynamicRunner, connectMCP, + connectMCPServer: connectOneMCPServer, + hasMCPServer: (name) => reservedMCPServerNames.has(name), setToolPromoter: (promote) => { promoter.promote = promote; }, - dispose: async () => { - for (const client of connectedClients) { - permissionGate.unregisterMcpServer(client.serverName); - } - await Promise.all(connectedClients.map((c) => c.close().catch(() => undefined))); - await posixTools.dispose(); - await disposeWebSearchClients(); - }, + dispose, }; } diff --git a/src/mcp/add-server.test.ts b/src/mcp/add-server.test.ts new file mode 100644 index 000000000..b76200c2f --- /dev/null +++ b/src/mcp/add-server.test.ts @@ -0,0 +1,153 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { createGlobalSettingsWriter, persistGlobalHTTPMCPServer } from "./add-server.js"; +import { isReadOnlyMcpTool, mcpToolName } from "./tool-name.js"; + +const dirs: string[] = []; + +async function settingsPath(): Promise { + const dir = await mkdtemp(join(tmpdir(), "corbits-mcp-add-")); + dirs.push(dir); + return join(dir, "settings.json"); +} + +afterEach(async () => { + await Promise.all(dirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))); +}); + +describe("persistGlobalHTTPMCPServer", () => { + test("adds an HTTP server without losing unrelated settings", async () => { + const path = await settingsPath(); + await Bun.write(path, JSON.stringify({ providers: {}, showPromptCost: true })); + const writer = createGlobalSettingsWriter(path); + + expect( + await persistGlobalHTTPMCPServer(writer, "linear", "https://mcp.linear.app/mcp"), + ).toEqual({ + ok: true, + server: { name: "linear", type: "http", url: "https://mcp.linear.app/mcp" }, + settings: { + providers: {}, + showPromptCost: true, + mcpServers: [{ name: "linear", type: "http", url: "https://mcp.linear.app/mcp" }], + }, + }); + expect(JSON.parse(await readFile(path, "utf8"))).toEqual({ + providers: {}, + showPromptCost: true, + mcpServers: [{ name: "linear", type: "http", url: "https://mcp.linear.app/mcp" }], + }); + }); + + test("rejects invalid input and duplicate names without mutation", async () => { + const path = await settingsPath(); + const original = JSON.stringify({ + providers: {}, + mcpServers: [{ name: "linear", url: "https://existing.test/mcp" }], + }); + await Bun.write(path, original); + const writer = createGlobalSettingsWriter(path); + + expect(await persistGlobalHTTPMCPServer(writer, "", "https://example.test/mcp")).toEqual({ + ok: false, + reason: "invalid-name", + }); + expect(await persistGlobalHTTPMCPServer(writer, "other", "ftp://example.test/mcp")).toEqual({ + ok: false, + reason: "invalid-url", + }); + expect(await persistGlobalHTTPMCPServer(writer, "linear", "https://new.test/mcp")).toEqual({ + ok: false, + reason: "duplicate", + }); + expect(await readFile(path, "utf8")).toBe(original); + }); + + test("rejects delimiter-bearing names before they can spoof mutating tool permissions", async () => { + const path = await settingsPath(); + const original = JSON.stringify({ providers: {}, showPromptCost: true }); + await writeFile(path, original); + const spoofingName = "linear__list_projects"; + + expect(isReadOnlyMcpTool(mcpToolName(spoofingName, "delete_issue"))).toBe(true); + expect( + await persistGlobalHTTPMCPServer( + createGlobalSettingsWriter(path), + spoofingName, + "https://mcp.linear.app/mcp", + ), + ).toEqual({ ok: false, reason: "invalid-name" }); + expect(await readFile(path, "utf8")).toBe(original); + }); + + test("blocks global mutation while local MCP settings shadow it", async () => { + const path = await settingsPath(); + const original = JSON.stringify({ providers: {}, showPromptCost: true }); + await writeFile(path, original); + const writer = createGlobalSettingsWriter(path); + + expect( + await persistGlobalHTTPMCPServer(writer, "linear", "https://mcp.linear.app/mcp", "local"), + ).toEqual({ ok: false, reason: "local-shadow" }); + expect(await readFile(path, "utf8")).toBe(original); + }); + + test("serializes a delayed MCP add with hook and plugin mutation", async () => { + const path = await settingsPath(); + await writeFile(path, JSON.stringify({ providers: {}, showPromptCost: true })); + let releaseFirstLoad: (() => void) | undefined; + let firstLoadStarted: (() => void) | undefined; + const firstLoad = new Promise((resolve) => { + firstLoadStarted = resolve; + }); + let loads = 0; + const writer = createGlobalSettingsWriter(path, { + load: async (settingsFile) => { + const settings = JSON.parse(await readFile(settingsFile, "utf8")); + loads += 1; + if (loads === 1) { + firstLoadStarted?.(); + await new Promise((resolve) => { + releaseFirstLoad = resolve; + }); + } + return settings; + }, + }); + + const add = persistGlobalHTTPMCPServer(writer, "linear", "https://mcp.linear.app/mcp"); + await firstLoad; + const admin = writer.mutate((base) => ({ + ...base, + hooks: { audit: { enabled: false } }, + plugins: { linear: { enabled: true } }, + })); + releaseFirstLoad?.(); + await Promise.all([add, admin]); + + expect(JSON.parse(await readFile(path, "utf8"))).toMatchObject({ + showPromptCost: true, + hooks: { audit: { enabled: false } }, + plugins: { linear: { enabled: true } }, + mcpServers: [{ name: "linear", type: "http", url: "https://mcp.linear.app/mcp" }], + }); + }); + + test("checks duplicates inside serialized fresh-disk mutations", async () => { + const path = await settingsPath(); + await writeFile(path, JSON.stringify({ providers: {} })); + const writer = createGlobalSettingsWriter(path); + + const [first, second] = await Promise.all([ + persistGlobalHTTPMCPServer(writer, "linear", "https://one.test/mcp"), + persistGlobalHTTPMCPServer(writer, "linear", "https://two.test/mcp"), + ]); + + expect([first, second].filter((result) => result.ok)).toHaveLength(1); + const saved = JSON.parse(await readFile(path, "utf8")); + expect(saved.mcpServers).toHaveLength(1); + }); +}); diff --git a/src/mcp/add-server.ts b/src/mcp/add-server.ts new file mode 100644 index 000000000..079453117 --- /dev/null +++ b/src/mcp/add-server.ts @@ -0,0 +1,130 @@ +import type { MCPServerConfig, Settings } from "../config/settings.js"; +import { loadGlobalSettingsWriteBase, saveGlobalSettings } from "../config/settings.js"; + +export interface GlobalSettingsWriter { + readonly enqueue: (job: () => Promise) => Promise; + readonly update: (apply: (base: Settings) => Settings | null) => Promise; + readonly updateAt: ( + path: string, + apply: (base: Settings) => Settings | null, + ) => Promise; + readonly mutate: (apply: (base: Settings) => Settings | null) => Promise<"ok" | "skipped">; + readonly mutateAt: ( + path: string, + apply: (base: Settings) => Settings | null, + ) => Promise<"ok" | "skipped">; +} + +interface GlobalSettingsWriterDeps { + readonly load: (path: string) => Promise; + readonly save: (path: string, settings: Settings) => Promise; +} + +const defaultWriterDeps: GlobalSettingsWriterDeps = { + load: loadGlobalSettingsWriteBase, + save: saveGlobalSettings, +}; + +export function createGlobalSettingsWriter( + path: string, + deps: Partial = {}, +): GlobalSettingsWriter { + const writerDeps: GlobalSettingsWriterDeps = { ...defaultWriterDeps, ...deps }; + let tail = Promise.resolve(); + const enqueue = (job: () => Promise): Promise => { + const run = tail.then(job); + tail = run.then( + () => undefined, + () => undefined, + ); + return run; + }; + + const updateAt = ( + settingsPath: string, + apply: (base: Settings) => Settings | null, + ): Promise => + enqueue(async () => { + const base = await writerDeps.load(settingsPath); + if (base === null) return null; + const next = apply(base); + if (next === null) return base; + await writerDeps.save(settingsPath, next); + return next; + }); + + const mutateAt = async ( + settingsPath: string, + apply: (base: Settings) => Settings | null, + ): Promise<"ok" | "skipped"> => + (await updateAt(settingsPath, apply)) === null ? "skipped" : "ok"; + + return { + enqueue, + update: (apply) => updateAt(path, apply), + updateAt, + mutate: (apply) => mutateAt(path, apply), + mutateAt, + }; +} + +export type PersistMCPServerResult = + | { readonly ok: true; readonly server: MCPServerConfig; readonly settings: Settings } + | { + readonly ok: false; + readonly reason: + "invalid-name" | "invalid-url" | "duplicate" | "active" | "skipped" | "local-shadow"; + }; + +const MCP_SERVER_NAME_PATTERN = /^[A-Za-z0-9_-]+$/; + +export function validateMCPServerName(value: string): string | null { + if (value.length === 0) return "Enter a server name first."; + if (!MCP_SERVER_NAME_PATTERN.test(value) || value.includes("__")) { + return 'Use letters, numbers, single underscores, or hyphens; "__" is reserved.'; + } + return null; +} + +export function isAbsoluteHTTPURL(value: string): boolean { + try { + const url = new URL(value); + return (url.protocol === "http:" || url.protocol === "https:") && url.origin !== "null"; + } catch { + return false; + } +} + +export async function persistGlobalHTTPMCPServer( + writer: GlobalSettingsWriter, + rawName: string, + rawURL: string, + source: "local" | "global" | "none" = "global", + isNameActive: (name: string) => boolean = () => false, +): Promise { + const name = rawName.trim(); + const url = rawURL.trim(); + if (validateMCPServerName(name) !== null) return { ok: false, reason: "invalid-name" }; + if (!isAbsoluteHTTPURL(url)) return { ok: false, reason: "invalid-url" }; + if (source === "local") return { ok: false, reason: "local-shadow" }; + + const server: MCPServerConfig = { name, type: "http", url }; + let active = false; + let duplicate = false; + const settings = await writer.update((base) => { + if (isNameActive(name)) { + active = true; + return null; + } + const servers = base.mcpServers ?? []; + if (servers.some((entry) => entry.name === name)) { + duplicate = true; + return null; + } + return { ...base, mcpServers: [...servers, server] }; + }); + if (settings === null) return { ok: false, reason: "skipped" }; + if (active) return { ok: false, reason: "active" }; + if (duplicate) return { ok: false, reason: "duplicate" }; + return { ok: true, server, settings }; +} diff --git a/src/mcp/client-auth-policy.test.ts b/src/mcp/client-auth-policy.test.ts index f171ab9cd..3b42276eb 100644 --- a/src/mcp/client-auth-policy.test.ts +++ b/src/mcp/client-auth-policy.test.ts @@ -1,11 +1,21 @@ import { beforeEach, describe, expect, test } from "bun:test"; +import { UnauthorizedError } from "@modelcontextprotocol/sdk/client/auth.js"; import { withMockedModule } from "../../tests/helpers/mock-module.js"; let callbackStarts = 0; +let callbackCloses = 0; let providerCreates = 0; let transportOptions: unknown[] = []; let providerServerURL: string | undefined; let clientConnectError: Error | undefined; +let providerCreateError: Error | undefined; +let clientCloses = 0; +let blockToolDiscovery = false; +let toolDiscoverySignals: (AbortSignal | undefined)[] = []; +let toolDiscoveryAborts = 0; +let blockTokenExchange = false; +let tokenExchangeSignals: (AbortSignal | null | undefined)[] = []; +let tokenExchangeAborts = 0; const authProvider = { resetAuthorization: async () => undefined }; @@ -17,10 +27,27 @@ await withMockedModule( async connect(): Promise { if (clientConnectError !== undefined) throw clientConnectError; } - async listTools(): Promise<{ tools: [] }> { + async listTools( + _params?: unknown, + options?: { signal?: AbortSignal }, + ): Promise<{ tools: [] }> { + toolDiscoverySignals.push(options?.signal); + if (blockToolDiscovery) { + await new Promise((_resolve, reject) => { + const signal = options?.signal; + const onAbort = (): void => { + toolDiscoveryAborts += 1; + reject(signal?.reason ?? new Error("tool discovery aborted")); + }; + if (signal?.aborted === true) onAbort(); + else signal?.addEventListener("abort", onAbort, { once: true }); + }); + } return { tools: [] }; } - async close(): Promise {} + async close(): Promise { + clientCloses += 1; + } }, }), ); @@ -30,9 +57,26 @@ await withMockedModule( (real: typeof import("@modelcontextprotocol/sdk/client/streamableHttp.js")) => ({ ...real, StreamableHTTPClientTransport: class { - constructor(_url: URL, options?: unknown) { + constructor( + _url: URL, + private readonly options?: { requestInit?: RequestInit }, + ) { transportOptions.push(options); } + async finishAuth(): Promise { + const signal = this.options?.requestInit?.signal; + tokenExchangeSignals.push(signal); + if (blockTokenExchange) { + await new Promise((_resolve, reject) => { + const onAbort = (): void => { + tokenExchangeAborts += 1; + reject(signal?.reason ?? new Error("token exchange aborted")); + }; + if (signal?.aborted === true) onAbort(); + else signal?.addEventListener("abort", onAbort, { once: true }); + }); + } + } get sessionId(): string | undefined { return undefined; } @@ -50,7 +94,9 @@ await withMockedModule( redirectUrl: "http://127.0.0.1:12345/callback", expectState: () => undefined, waitForCode: async () => "code", - close: () => undefined, + close: () => { + callbackCloses += 1; + }, }; }, }), @@ -63,6 +109,7 @@ await withMockedModule( createOAuthProvider: async (options: { serverURL: string }) => { providerCreates += 1; providerServerURL = options.serverURL; + if (providerCreateError !== undefined) throw providerCreateError; return authProvider; }, }), @@ -73,10 +120,19 @@ const { connectMCPServer } = await import("./client.js"); describe("HTTP MCP auth policy", () => { beforeEach(() => { callbackStarts = 0; + callbackCloses = 0; providerCreates = 0; transportOptions = []; providerServerURL = undefined; clientConnectError = undefined; + providerCreateError = undefined; + clientCloses = 0; + blockToolDiscovery = false; + toolDiscoverySignals = []; + toolDiscoveryAborts = 0; + blockTokenExchange = false; + tokenExchangeSignals = []; + tokenExchangeAborts = 0; }); test("built-in anonymous Exa treats 401 as a normal failure without OAuth machinery", async () => { @@ -94,6 +150,68 @@ describe("HTTP MCP auth policy", () => { expect(transportOptions).toEqual([undefined]); }); + test("closes a bound OAuth callback server when provider setup fails", async () => { + providerCreateError = new Error("provider setup exploded"); + + const result = await connectMCPServer({ + name: "linear", + type: "http", + url: "https://mcp.linear.app/mcp", + }); + + expect(result).toEqual({ + ok: false, + serverName: "linear", + error: "provider setup exploded", + }); + expect(callbackStarts).toBe(1); + expect(callbackCloses).toBe(1); + expect(transportOptions).toEqual([]); + }); + + test("aborts stalled tool discovery and closes HTTP resources", async () => { + blockToolDiscovery = true; + const abort = new AbortController(); + const connection = connectMCPServer( + { name: "linear", type: "http", url: "https://mcp.linear.app/mcp" }, + { signal: abort.signal }, + ); + while (toolDiscoverySignals.length === 0) await Promise.resolve(); + + expect(toolDiscoverySignals[0]).toBe(abort.signal); + abort.abort(new Error("toolset disposed")); + const result = await connection; + + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.error).toContain("toolset disposed"); + expect(toolDiscoveryAborts).toBe(1); + expect(clientCloses).toBe(1); + expect(callbackCloses).toBe(1); + }); + + test("aborts a stalled post-callback token exchange and closes HTTP resources", async () => { + blockTokenExchange = true; + clientConnectError = new UnauthorizedError("authorization required"); + const abort = new AbortController(); + const connection = connectMCPServer( + { name: "linear", type: "http", url: "https://mcp.linear.app/mcp" }, + { onAuthURL: () => undefined, signal: abort.signal }, + ); + while (tokenExchangeSignals.length === 0) await Promise.resolve(); + + expect(tokenExchangeSignals[0]).toBe(abort.signal); + abort.abort(new Error("toolset disposed")); + const result = await connection; + + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.error).toContain("toolset disposed"); + expect(tokenExchangeAborts).toBe(1); + expect(clientCloses).toBe(1); + expect(callbackCloses).toBe(1); + }); + test("ordinary HTTP creates endpoint-scoped OAuth and passes it to transport", async () => { const result = await connectMCPServer({ name: "exa", diff --git a/src/mcp/client.ts b/src/mcp/client.ts index f6e92a93b..5a511931d 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -78,6 +78,7 @@ async function completeInteractiveAuth(context: HTTPAuthContext): Promise const code = await context.callback.waitForCode(context.signal ?? new AbortController().signal); await new StreamableHTTPClientTransport(context.url, { authProvider: context.authProvider, + ...(context.signal === undefined ? {} : { requestInit: { signal: context.signal } }), }).finishAuth(code); } @@ -140,8 +141,11 @@ async function finishClient( client: Client, serverName: string, authContext?: HTTPAuthContext, + signal?: AbortSignal, ): Promise { - const result = await withHTTPAuthorizationRecovery(authContext, () => client.listTools()); + const result = await withHTTPAuthorizationRecovery(authContext, () => + signal === undefined ? client.listTools() : client.listTools(undefined, { signal }), + ); const tools: MCPTool[] = result.tools.map((t) => { const annotations = t.annotations as McpToolAnnotations | undefined; const tool: MCPTool = { @@ -185,8 +189,11 @@ async function connectStdio( if (options.stderr !== undefined) transportOptions.stderr = options.stderr; const client = new Client({ name: MCP_CLIENT_NAME, version: "1.0.0" }); try { - await client.connect(new StdioClientTransport(transportOptions)); - return { ok: true, client: await finishClient(client, config.name) }; + await client.connect( + new StdioClientTransport(transportOptions), + options.signal === undefined ? undefined : { signal: options.signal }, + ); + return { ok: true, client: await finishClient(client, config.name, undefined, options.signal) }; } catch (err) { await client.close().catch(() => undefined); return { @@ -203,40 +210,55 @@ async function connectHttp( ): Promise { if (config.url === undefined) return { ok: false, serverName: config.name, error: "http MCP server requires a url" }; - const normalizedURL = normalizeMCPServerURL(config.url); - const url = new URL(normalizedURL); - let authContext: HTTPAuthContext | undefined; - let makeTransport: () => Transport; - if (config.oauth === false) { - makeTransport = () => new StreamableHTTPClientTransport(url) as unknown as Transport; - } else { - const callback = await startCallbackServer(config.name); - const authProvider = await createOAuthProvider({ - serverName: config.name, - serverURL: normalizedURL, - redirectUrl: callback.redirectUrl, - onAuthURL: (name, authUrl) => options.onAuthURL?.(name, authUrl), - onAuthorizationState: callback.expectState, - }); - makeTransport = () => - new StreamableHTTPClientTransport(url, { authProvider }) as unknown as Transport; - authContext = { - url, - authProvider, - callback, - interactive: options.onAuthURL !== undefined, - serverName: config.name, - ...(options.onAuthorized !== undefined ? { onAuthorized: options.onAuthorized } : {}), - ...(options.signal !== undefined ? { signal: options.signal } : {}), - }; - } - const client = new Client({ name: MCP_CLIENT_NAME, version: "1.0.0" }); + let callback: CallbackServer | undefined; + let client: Client | undefined; try { - await withHTTPAuthorizationRecovery(authContext, () => client.connect(makeTransport())); - return { ok: true, client: await finishClient(client, config.name, authContext) }; + const normalizedURL = normalizeMCPServerURL(config.url); + const url = new URL(normalizedURL); + let authContext: HTTPAuthContext | undefined; + let makeTransport: () => Transport; + if (config.oauth === false) { + makeTransport = () => new StreamableHTTPClientTransport(url) as unknown as Transport; + } else { + callback = await startCallbackServer(config.name); + const authProvider = await createOAuthProvider({ + serverName: config.name, + serverURL: normalizedURL, + redirectUrl: callback.redirectUrl, + onAuthURL: (name, authUrl) => options.onAuthURL?.(name, authUrl), + onAuthorizationState: callback.expectState, + }); + makeTransport = () => + new StreamableHTTPClientTransport(url, { authProvider }) as unknown as Transport; + authContext = { + url, + authProvider, + callback, + interactive: options.onAuthURL !== undefined, + serverName: config.name, + ...(options.onAuthorized !== undefined ? { onAuthorized: options.onAuthorized } : {}), + ...(options.signal !== undefined ? { signal: options.signal } : {}), + }; + } + const connectedClient = new Client({ name: MCP_CLIENT_NAME, version: "1.0.0" }); + client = connectedClient; + await withHTTPAuthorizationRecovery(authContext, () => + connectedClient.connect( + makeTransport(), + options.signal === undefined ? undefined : { signal: options.signal }, + ), + ); + return { + ok: true, + client: await finishClient(connectedClient, config.name, authContext, options.signal), + }; } catch (err) { - await client.close().catch(() => undefined); - authContext?.callback.close(); + await client?.close().catch(() => undefined); + try { + callback?.close(); + } catch { + // Setup has already failed; callback teardown is best effort. + } return { ok: false, serverName: config.name, diff --git a/src/telemetry/toggle.ts b/src/telemetry/toggle.ts index ee7c163a6..898df491b 100644 --- a/src/telemetry/toggle.ts +++ b/src/telemetry/toggle.ts @@ -44,7 +44,9 @@ const defaultDeps: TelemetryToggleDeps = { export function createTelemetryToggleHandler( globalSettingsPath: string, deps: TelemetryToggleDeps = defaultDeps, + enqueue: (job: () => Promise) => Promise = (job) => job(), ): (enabled: boolean) => boolean { + let intentGeneration = 0; return (enabled: boolean): boolean => { if (enabled && deps.telemetryDisabledByEnv()) { // Env kills own the "disabled means no settings writes" constraint @@ -56,6 +58,7 @@ export function createTelemetryToggleHandler( ); return false; } + const generation = ++intentGeneration; if (!enabled) { // Opt-out must be immediate and absolute: discard whatever the outgoing // instance has queued (dropping the singleton alone would leave its @@ -84,7 +87,7 @@ export function createTelemetryToggleHandler( ); } - void (async () => { + void enqueue(async () => { // Re-enable goes through ensureTelemetrySettings so an installationId // always exists — writing { enabled: true } without one would leave the // re-created instance resolving disabled and the toggle a silent no-op. @@ -110,6 +113,7 @@ export function createTelemetryToggleHandler( // file is readable again. return; } + if (generation !== intentGeneration) return; const previous = deps.getTelemetry(); const base: Settings = current ?? { providers: {} }; // Keep installation identity across ambient opt-out so /feedback still @@ -139,8 +143,9 @@ export function createTelemetryToggleHandler( } // Re-create so this session's remaining captures honor the change (and, // on the load-succeeded path, carry forward the real installationId). + if (generation !== intentGeneration) return; deps.setTelemetry(deps.createTelemetry({ settings: next })); - })(); + }); return true; }; } diff --git a/src/tui/command-surfaces.test.ts b/src/tui/command-surfaces.test.ts index dfd1093ba..90cd8dd7d 100644 --- a/src/tui/command-surfaces.test.ts +++ b/src/tui/command-surfaces.test.ts @@ -9,18 +9,23 @@ import { pluginRowLabel, type CommandSurfaceDeps, type GrantEntry, + type McpEntry, type PluginEntry, type PluginsSurfaceDeps, type SettingsSnapshot, } from "./command-surfaces"; import type { KeyEvent } from "@opentui/core"; -import { withTestRenderer } from "./harness"; +import { focusOwner } from "./focus/index.js"; +import { withTestRenderer, type Harness } from "./harness"; import { acceptOverlaySelection, + closeInsetOverlay, createAppShell, cycleOverlaySelection, moveOverlaySelection, + openListOverlay, + openPalette, runOverlayAction, type AppShell, } from "./shell"; @@ -51,6 +56,25 @@ async function withShell(fn: (shell: AppShell) => Promise | void): Promise ); } +async function withWiredShell( + fn: (shell: AppShell, harness: Harness) => Promise | void, +): Promise { + await withTestRenderer( + async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + wireKeys: true, + }); + try { + await fn(shell, h); + } finally { + shell.dispose(); + } + }, + { width: 80, height: 24 }, + ); +} + describe("surface labels", () => { test("grant label carries scope, tool, pattern, provider model", () => { const entry: GrantEntry = { @@ -568,6 +592,159 @@ describe("mcp surface", () => { "notion — needs auth", "sentry — failed", ]); + expect(shell.overlayItems).toContain("Add MCP server — Alt+A"); + }); + }); + + test("the visible add row opens the same add-server flow", async () => { + await withShell((shell) => { + openCommandSurface(shell, "mcp", { + notify: () => {}, + mcp: { list: () => entries, openAuthURL: () => {} }, + }); + moveOverlaySelection(shell, entries.length); + acceptOverlaySelection(shell); + expect(shell.overlayItems).toEqual(["▏"]); + }); + }); + + test("dismissing and reopening releases the previous status subscription", async () => { + await withShell((shell) => { + const listeners = new Set<() => void>(); + const deps: CommandSurfaceDeps = { + notify: () => {}, + mcp: { + list: () => entries, + openAuthURL: () => {}, + subscribe: (listener) => { + listeners.add(listener); + return () => listeners.delete(listener); + }, + }, + }; + + openCommandSurface(shell, "mcp", deps); + expect(listeners.size).toBe(1); + closeInsetOverlay(shell); + expect(listeners.size).toBe(0); + + openCommandSurface(shell, "mcp", deps); + expect(listeners.size).toBe(1); + closeInsetOverlay(shell); + expect(listeners.size).toBe(0); + }); + }); + + test("disposing an open MCP surface releases its status subscription exactly once", async () => { + await withShell((shell) => { + const listeners = new Set<() => void>(); + let unsubscribeCalls = 0; + let listCalls = 0; + const emitStatus = (): void => { + for (const listener of [...listeners]) listener(); + }; + openCommandSurface(shell, "mcp", { + notify: () => {}, + mcp: { + list: () => { + listCalls += 1; + return entries; + }, + openAuthURL: () => {}, + subscribe: (listener) => { + listeners.add(listener); + return () => { + unsubscribeCalls += 1; + listeners.delete(listener); + }; + }, + }, + }); + + expect(listeners.size).toBe(1); + shell.dispose(); + expect(unsubscribeCalls).toBe(1); + expect(listeners.size).toBe(0); + const callsAfterDispose = listCalls; + emitStatus(); + expect(listCalls).toBe(callsAfterDispose); + expect(shell.overlayList).toBeNull(); + shell.dispose(); + expect(unsubscribeCalls).toBe(1); + }); + }); + + test("status refreshes the MCP surface while a palette is stacked over it", async () => { + await withShell((shell) => { + let liveEntries: readonly McpEntry[] = [{ name: "linear", state: "connecting" }]; + const listeners = new Set<() => void>(); + openCommandSurface(shell, "mcp", { + notify: () => {}, + mcp: { + list: () => liveEntries, + openAuthURL: () => {}, + subscribe: (listener) => { + listeners.add(listener); + return () => listeners.delete(listener); + }, + }, + }); + openPalette(shell, { catalog: [{ id: "help", label: "help" }] }); + + liveEntries = [ + { name: "linear", state: "needs-auth", authURL: "https://linear.test/authorize" }, + ]; + for (const listener of [...listeners]) listener(); + expect(shell.overlayKind).toBe("palette"); + expect(listeners.size).toBe(1); + + closeInsetOverlay(shell); + expect(shell.overlayKind).toBe("mcp"); + expect(shell.overlayItems[0]).toBe("linear — needs auth"); + expect(listeners.size).toBe(1); + + liveEntries = [{ name: "linear", state: "connected", toolCount: 3 }]; + for (const listener of [...listeners]) listener(); + expect(shell.overlayItems[0]).toBe("linear — connected · 3 tools"); + expect(listeners.size).toBe(1); + closeInsetOverlay(shell); + expect(listeners.size).toBe(0); + }); + }); + + test("an open surface refreshes a delayed OAuth transition and authorizes the live target", async () => { + await withShell((shell) => { + let liveEntries: readonly McpEntry[] = [ + { name: "exa", state: "connected", toolCount: 2 }, + { name: "linear", state: "connecting" }, + ]; + const listeners = new Set<() => void>(); + const opened: string[] = []; + openCommandSurface(shell, "mcp", { + notify: () => {}, + mcp: { + list: () => liveEntries, + openAuthURL: (url) => opened.push(url), + subscribe: (listener) => { + listeners.add(listener); + return () => listeners.delete(listener); + }, + }, + }); + expect(shell.overlayItems[1]).toBe("linear — connecting"); + moveOverlaySelection(shell, 1); + + liveEntries = [ + { name: "exa", state: "connected", toolCount: 2 }, + { name: "linear", state: "needs-auth", authURL: "https://linear.test/authorize" }, + ]; + for (const listener of [...listeners]) listener(); + + expect(shell.overlayItems[1]).toBe("linear — needs auth"); + expect(shell.overlayList?.activeIndex).toBe(1); + acceptOverlaySelection(shell); + expect(opened).toEqual(["https://linear.test/authorize"]); + expect(listeners.size).toBe(0); }); }); @@ -588,15 +765,383 @@ describe("mcp surface", () => { }); }); - test("Enter on a connected server does nothing", async () => { - await withShell((shell) => { - const opened: string[] = []; + test("Enter on non-auth rows releases the status subscription", async () => { + const nonAuthEntries: readonly McpEntry[] = [ + { name: "connected", state: "connected", toolCount: 1 }, + { name: "connecting", state: "connecting" }, + { name: "failed", state: "failed", error: "offline" }, + ]; + + for (const entry of nonAuthEntries) { + await withShell((shell) => { + const listeners = new Set<() => void>(); + let unsubscribeCalls = 0; + const opened: string[] = []; + openCommandSurface(shell, "mcp", { + notify: () => {}, + mcp: { + list: () => [entry], + openAuthURL: (url) => opened.push(url), + subscribe: (listener) => { + listeners.add(listener); + return () => { + unsubscribeCalls += 1; + listeners.delete(listener); + }; + }, + }, + }); + + expect(listeners.size).toBe(1); + acceptOverlaySelection(shell); + expect(opened).toEqual([]); + expect(unsubscribeCalls).toBe(1); + expect(listeners.size).toBe(0); + }); + } + }); + + test("Alt+A collects a name and absolute HTTP URL before adding", async () => { + await withShell(async (shell) => { + const added: { name: string; url: string }[] = []; + let liveEntries: readonly McpEntry[] = entries; openCommandSurface(shell, "mcp", { notify: () => {}, - mcp: { list: () => entries, openAuthURL: (url) => opened.push(url) }, + mcp: { + list: () => liveEntries, + openAuthURL: () => {}, + addServer: async (name, url) => { + added.push({ name, url }); + liveEntries = [...liveEntries, { name, state: "connecting" }]; + return { ok: true, message: `Added ${name}.` }; + }, + }, }); + + expect(runOverlayAction(shell, altKey("a"))).toBe(true); + for (const ch of "linear") runOverlayAction(shell, charKey(ch)); + acceptOverlaySelection(shell); + for (const ch of "https://mcp.linear.app/mcp") runOverlayAction(shell, charKey(ch)); acceptOverlaySelection(shell); - expect(opened).toEqual([]); + await Promise.resolve(); + await Promise.resolve(); + + expect(added).toEqual([{ name: "linear", url: "https://mcp.linear.app/mcp" }]); + expect(shell.overlayItems).toContain("linear — connecting"); + }); + }); + + test("wired shell preserves j and k in MCP names and URLs while ordinary lists still navigate", async () => { + await withWiredShell(async (shell, harness) => { + const added: { name: string; url: string }[] = []; + openCommandSurface(shell, "mcp", { + notify: () => {}, + mcp: { + list: () => entries, + openAuthURL: () => {}, + addServer: async (name, url) => { + added.push({ name, url }); + return { ok: true, message: "added" }; + }, + }, + }); + runOverlayAction(shell, altKey("a")); + + for (const ch of "jira") harness.mockInput.pressKey(ch); + expect(shell.overlayItems[0]).toBe("jira▏"); + harness.mockInput.pressKey("\r"); + for (const ch of "https://jira.test/mcp") harness.mockInput.pressKey(ch); + expect(shell.overlayItems[0]).toBe("https://jira.test/mcp▏"); + harness.mockInput.pressKey("\r"); + await Promise.resolve(); + await Promise.resolve(); + expect(added).toEqual([{ name: "jira", url: "https://jira.test/mcp" }]); + + closeInsetOverlay(shell); + openListOverlay(shell, { kind: "demo", items: ["first", "second"] }); + expect(shell.overlayList?.activeIndex).toBe(0); + harness.mockInput.pressKey("j"); + expect(shell.overlayList?.activeIndex).toBe(1); + harness.mockInput.pressKey("k"); + expect(shell.overlayList?.activeIndex).toBe(0); + }); + }); + + test("bracketed paste inserts an MCP URL into the owned text pane", async () => { + await withWiredShell(async (shell, harness) => { + const added: { name: string; url: string }[] = []; + openCommandSurface(shell, "mcp", { + notify: () => {}, + mcp: { + list: () => entries, + openAuthURL: () => {}, + addServer: async (name, url) => { + added.push({ name, url }); + return { ok: true, message: "added" }; + }, + }, + }); + runOverlayAction(shell, altKey("a")); + for (const ch of "jira") harness.mockInput.pressKey(ch); + harness.mockInput.pressKey("\r"); + + await harness.mockInput.pasteBracketedText("https://jira.test/mcp"); + await harness.renderOnce(); + expect(shell.overlayItems[0]).toBe("https://jira.test/mcp▏"); + harness.mockInput.pressKey("\r"); + await Promise.resolve(); + await Promise.resolve(); + expect(added).toEqual([{ name: "jira", url: "https://jira.test/mcp" }]); + }); + }); + + test("deferred add completion does not displace a newer gate overlay", async () => { + await withShell(async (shell) => { + let resolveAdd: ((result: { ok: boolean; message: string }) => void) | undefined; + const deferredAdd = new Promise<{ ok: boolean; message: string }>((resolve) => { + resolveAdd = resolve; + }); + let gateCancellations = 0; + openCommandSurface(shell, "mcp", { + notify: () => {}, + mcp: { + list: () => entries, + openAuthURL: () => {}, + addServer: () => deferredAdd, + }, + }); + + expect(runOverlayAction(shell, altKey("a"))).toBe(true); + for (const ch of "linear") runOverlayAction(shell, charKey(ch)); + acceptOverlaySelection(shell); + for (const ch of "https://mcp.linear.app/mcp") runOverlayAction(shell, charKey(ch)); + acceptOverlaySelection(shell); + + openListOverlay(shell, { + kind: "operator", + title: "newer gate", + items: ["Keep waiting"], + onCancel: () => { + gateCancellations += 1; + }, + }); + resolveAdd?.({ ok: true, message: "Added linear." }); + await Promise.resolve(); + await Promise.resolve(); + + expect(shell.overlayKind).toBe("operator"); + expect(shell.overlayItems).toEqual(["Keep waiting"]); + expect(gateCancellations).toBe(0); + closeInsetOverlay(shell); + expect(gateCancellations).toBe(1); + }); + }); + + test("a resolved MCP add cannot continue into a disposed shell", async () => { + await withShell(async (shell) => { + let resolveAdd: ((result: { ok: boolean; message: string }) => void) | undefined; + const deferredAdd = new Promise<{ ok: boolean; message: string }>((resolve) => { + resolveAdd = resolve; + }); + const notes: string[] = []; + const listeners = new Set<() => void>(); + let subscribeCalls = 0; + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown): void => { + unhandled.push(reason); + }; + process.on("unhandledRejection", onUnhandled); + try { + openCommandSurface(shell, "mcp", { + notify: (note) => { + notes.push(note); + throw new Error("disposed shell notified"); + }, + mcp: { + list: () => entries, + openAuthURL: () => {}, + subscribe: (listener) => { + subscribeCalls += 1; + listeners.add(listener); + return () => listeners.delete(listener); + }, + addServer: () => deferredAdd, + }, + }); + runOverlayAction(shell, altKey("a")); + for (const ch of "linear") runOverlayAction(shell, charKey(ch)); + acceptOverlaySelection(shell); + for (const ch of "https://mcp.linear.app/mcp") runOverlayAction(shell, charKey(ch)); + acceptOverlaySelection(shell); + + shell.dispose(); + const overlayItemsAfterDispose = shell.overlayItems; + resolveAdd?.({ ok: true, message: "Added linear." }); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + + expect(notes).toEqual([]); + expect(subscribeCalls).toBe(1); + expect(listeners.size).toBe(0); + expect(shell.overlayKind).toBeNull(); + expect(shell.overlayItems).toBe(overlayItemsAfterDispose); + expect(shell.overlayHost.visible).toBe(false); + expect(unhandled).toEqual([]); + } finally { + process.off("unhandledRejection", onUnhandled); + } + }); + }); + + test("a rejected MCP add cannot continue into a disposed shell", async () => { + await withShell(async (shell) => { + let rejectAdd: ((reason: unknown) => void) | undefined; + const deferredAdd = new Promise<{ ok: boolean; message: string }>((_resolve, reject) => { + rejectAdd = reject; + }); + const notes: string[] = []; + const listeners = new Set<() => void>(); + let subscribeCalls = 0; + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown): void => { + unhandled.push(reason); + }; + process.on("unhandledRejection", onUnhandled); + try { + openCommandSurface(shell, "mcp", { + notify: (note) => { + notes.push(note); + throw new Error("disposed shell notified"); + }, + mcp: { + list: () => entries, + openAuthURL: () => {}, + subscribe: (listener) => { + subscribeCalls += 1; + listeners.add(listener); + return () => listeners.delete(listener); + }, + addServer: () => deferredAdd, + }, + }); + runOverlayAction(shell, altKey("a")); + for (const ch of "linear") runOverlayAction(shell, charKey(ch)); + acceptOverlaySelection(shell); + for (const ch of "https://mcp.linear.app/mcp") runOverlayAction(shell, charKey(ch)); + acceptOverlaySelection(shell); + + shell.dispose(); + const overlayItemsAfterDispose = shell.overlayItems; + rejectAdd?.(new Error("connection failed")); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + + expect(notes).toEqual([]); + expect(subscribeCalls).toBe(1); + expect(listeners.size).toBe(0); + expect(shell.overlayKind).toBeNull(); + expect(shell.overlayItems).toBe(overlayItemsAfterDispose); + expect(shell.overlayHost.visible).toBe(false); + expect(unhandled).toEqual([]); + } finally { + process.off("unhandledRejection", onUnhandled); + } + }); + }); + + test("an invalid name stays focused with its value retained and can be corrected", async () => { + await withShell(async (shell) => { + const added: { name: string; url: string }[] = []; + const notes: string[] = []; + openCommandSurface(shell, "mcp", { + notify: (note) => notes.push(note), + mcp: { + list: () => entries, + openAuthURL: () => {}, + addServer: async (name, url) => { + added.push({ name, url }); + return { ok: true, message: "added" }; + }, + }, + }); + runOverlayAction(shell, altKey("a")); + for (const ch of "linear__admin") runOverlayAction(shell, charKey(ch)); + acceptOverlaySelection(shell); + + expect(added).toEqual([]); + expect(notes.at(-1)).toContain("single underscores"); + expect(notes.at(-1)).toContain("__"); + expect(shell.overlayItems[0]).toBe("linear__admin▏"); + expect(focusOwner(shell.focus)).toBe("overlay"); + + for (let i = 0; i < 7; i++) runOverlayAction(shell, key("backspace")); + for (const ch of "-admin") runOverlayAction(shell, charKey(ch)); + acceptOverlaySelection(shell); + for (const ch of "https://linear.test/mcp") runOverlayAction(shell, charKey(ch)); + acceptOverlaySelection(shell); + await Promise.resolve(); + await Promise.resolve(); + expect(added).toEqual([{ name: "linear-admin", url: "https://linear.test/mcp" }]); + }); + }); + + test("an invalid URL stays focused with its value retained and can be corrected", async () => { + await withShell(async (shell) => { + const added: { name: string; url: string }[] = []; + const notes: string[] = []; + openCommandSurface(shell, "mcp", { + notify: (note) => notes.push(note), + mcp: { + list: () => entries, + openAuthURL: () => {}, + addServer: async (name, url) => { + added.push({ name, url }); + return { ok: true, message: "added" }; + }, + }, + }); + runOverlayAction(shell, altKey("a")); + for (const ch of "linear") runOverlayAction(shell, charKey(ch)); + acceptOverlaySelection(shell); + const invalidURL = "relative/path"; + for (const ch of invalidURL) runOverlayAction(shell, charKey(ch)); + acceptOverlaySelection(shell); + + expect(added).toEqual([]); + expect(notes.at(-1)).toContain("HTTP(S) URL"); + expect(shell.overlayItems[0]).toBe(`${invalidURL}▏`); + expect(focusOwner(shell.focus)).toBe("overlay"); + + for (const _character of invalidURL) runOverlayAction(shell, key("backspace")); + for (const ch of "https://linear.test/mcp") runOverlayAction(shell, charKey(ch)); + acceptOverlaySelection(shell); + await Promise.resolve(); + await Promise.resolve(); + expect(added).toEqual([{ name: "linear", url: "https://linear.test/mcp" }]); + }); + }); + + test("cancelling the add prompt does not add a server", async () => { + await withShell((shell) => { + const added: { name: string; url: string }[] = []; + openCommandSurface(shell, "mcp", { + notify: () => {}, + mcp: { + list: () => entries, + openAuthURL: () => {}, + addServer: async (name, url) => { + added.push({ name, url }); + return { ok: true, message: "added" }; + }, + }, + }); + runOverlayAction(shell, altKey("a")); + for (const ch of "linear") runOverlayAction(shell, charKey(ch)); + closeInsetOverlay(shell); + + expect(added).toEqual([]); }); }); diff --git a/src/tui/command-surfaces.ts b/src/tui/command-surfaces.ts index 40b66085c..677d75847 100644 --- a/src/tui/command-surfaces.ts +++ b/src/tui/command-surfaces.ts @@ -8,14 +8,18 @@ * surfaces stay testable without a live runner. */ +import { isAbsoluteHTTPURL, validateMCPServerName } from "../mcp/add-server.js"; import { formatPluginWarningsSummary } from "../plugins/diagnostics.js"; import { maskEcho, maskSecret } from "./provider-setup.js"; import { residualIdFromSelection, type ResidualCatalogEntry } from "./residuals.js"; import { + captureOverlayContinuation, closeInsetOverlay, + isOverlayContinuationCurrent, openHelpOverlay, openListOverlay, openSettingsOverlay, + setOwnedOverlayItems, setStatusFlash, type AppShell, type ItemDescription, @@ -139,6 +143,8 @@ export interface McpSurfaceDeps { readonly list: () => readonly McpEntry[]; /** Open the server's authorization URL in the operator's browser. */ readonly openAuthURL: (url: string) => void; + readonly subscribe?: (listener: () => void) => () => void; + readonly addServer?: (name: string, url: string) => Promise; } /** Live summary for the settings surface's hooks row (owned by another surface). */ @@ -178,6 +184,7 @@ export type CommandSurfaceKind = "help" | "settings" | "permissions" | "plugins" | "hooks" | "mcp" | "models" | "add-provider"; const CLOSE_ID = "__close__"; +const ADD_MCP_ID = "__add_mcp__"; const BACK_ID = "__back__"; /** Synthetic `/plugins` row for standing load warnings (not a plugin id). */ const PLUGIN_LOAD_WARNINGS_ID = "__plugin_load_warnings__"; @@ -680,6 +687,10 @@ function openTextPromptPane( itemIds: ["value"], describe: () => ({ what: opts.what, impact: "Enter accepts. Esc cancels." }), onAccept: () => opts.onSubmit(buffer.value.trim()), + onPaste: (text) => { + buffer.value += text; + openTextPromptPane(shell, opts, buffer); + }, onAction: (_id, key) => { if (key.ctrl || key.meta || key.option) return false; if (key.name === "backspace") { @@ -951,8 +962,82 @@ function mcpDescription(entry: McpEntry): ItemDescription { } } +function openAddMcpURLPane( + shell: AppShell, + deps: CommandSurfaceDeps, + mcp: McpSurfaceDeps, + name: string, + buffer = { value: "" }, +): void { + openTextPromptPane( + shell, + { + title: `add ${name} MCP URL`, + what: "Absolute HTTP(S) URL for the MCP server.", + onSubmit: (url) => { + if (!isAbsoluteHTTPURL(url)) { + deps.notify("Enter an absolute HTTP(S) URL first."); + openAddMcpURLPane(shell, deps, mcp, name, buffer); + return; + } + const addServer = mcp.addServer; + if (addServer === undefined) { + deps.notify("Adding MCP servers is not available in this session."); + return; + } + const continuation = captureOverlayContinuation(shell); + void addServer(name, url) + .then( + (result) => { + if (!isOverlayContinuationCurrent(shell, continuation)) return; + deps.notify(result.message); + if (isOverlayContinuationCurrent(shell, continuation)) openMcpSurface(shell, deps); + }, + (err: unknown) => { + if (!isOverlayContinuationCurrent(shell, continuation)) return; + deps.notify(`Add failed: ${errorText(err)}`); + }, + ) + .catch(() => { + // UI continuation failures must not escape a fire-and-forget command. + }); + }, + }, + buffer, + ); +} + +function openAddMcpNamePane( + shell: AppShell, + deps: CommandSurfaceDeps, + mcp: McpSurfaceDeps, + buffer = { value: "" }, +): void { + openTextPromptPane( + shell, + { + title: "add MCP server", + what: "Unique name using letters, numbers, single underscores, or hyphens.", + onSubmit: (name) => { + const validationError = validateMCPServerName(name); + if (validationError !== null) { + deps.notify(validationError); + openAddMcpNamePane(shell, deps, mcp, buffer); + return; + } + openAddMcpURLPane(shell, deps, mcp, name); + }, + }, + buffer, + ); +} + /** Configured MCP servers and their live state; Enter authorizes an unauthorized one. */ -export function openMcpSurface(shell: AppShell, deps: CommandSurfaceDeps): void { +export function openMcpSurface( + shell: AppShell, + deps: CommandSurfaceDeps, + activeName?: string, +): void { const mcp = deps.mcp; if (mcp === undefined) { deps.notify("MCP administration is not available in this session."); @@ -960,14 +1045,20 @@ export function openMcpSurface(shell: AppShell, deps: CommandSurfaceDeps): void } closeInsetOverlay(shell); const entries = mcp.list(); - const rows: ResidualCatalogEntry[] = entries.map((e) => ({ id: e.name, label: mcpRowLabel(e) })); - if (rows.length === 0) { - rows.push({ id: CLOSE_ID, label: "No MCP servers configured" }); - } + const rows: ResidualCatalogEntry[] = entries.map((e) => ({ + id: e.name, + label: mcpRowLabel(e), + })); + if (rows.length === 0) rows.push({ id: CLOSE_ID, label: "No MCP servers configured" }); + rows.push({ id: ADD_MCP_ID, label: "Add MCP server — Alt+A" }); rows.push({ id: CLOSE_ID, label: "Close mcp" }); const byName = new Map(entries.map((e) => [e.name, e])); + const activeIndex = + activeName === undefined ? -1 : rows.findIndex((row) => row.id === activeName); + let unsubscribe: () => void = () => undefined; openListOverlay(shell, { kind: "mcp", + ...(activeIndex >= 0 ? { activeIndex } : {}), title: "mcp", frameId: "overlay-mcp", // The flash below reports the outcome; the echo would quote the row's @@ -978,9 +1069,15 @@ export function openMcpSurface(shell: AppShell, deps: CommandSurfaceDeps): void const target = byName.get(id); return target === undefined ? null : mcpDescription(target); }, + onCancel: () => unsubscribe(), onAccept: (selection) => { const id = selectedId(selection, rows); + unsubscribe(); if (id === undefined || id === CLOSE_ID) return; + if (id === ADD_MCP_ID) { + openAddMcpNamePane(shell, deps, mcp); + return; + } const target = byName.get(id); const url = target?.authURL; if (target === undefined || target.state !== "needs-auth" || url === undefined) return; @@ -993,7 +1090,41 @@ export function openMcpSurface(shell: AppShell, deps: CommandSurfaceDeps): void ttlMs: MCP_AUTH_FLASH_MS, }); }, + onAction: (_id, key) => { + if (key.ctrl || !(key.meta || key.option)) return false; + const name = typeof key.name === "string" ? key.name.toLowerCase() : ""; + if (name !== "a") return false; + unsubscribe(); + openAddMcpNamePane(shell, deps, mcp); + return true; + }, }); + unsubscribe = + mcp.subscribe?.(() => { + const liveEntries = mcp.list(); + const liveRows: ResidualCatalogEntry[] = liveEntries.map((entry) => ({ + id: entry.name, + label: mcpRowLabel(entry), + })); + if (liveRows.length === 0) { + liveRows.push({ id: CLOSE_ID, label: "No MCP servers configured" }); + } + liveRows.push({ id: ADD_MCP_ID, label: "Add MCP server — Alt+A" }); + liveRows.push({ id: CLOSE_ID, label: "Close mcp" }); + rows.splice(0, rows.length, ...liveRows); + byName.clear(); + for (const entry of liveEntries) byName.set(entry.name, entry); + if ( + !setOwnedOverlayItems( + shell, + "mcp", + rows.map((row) => row.label), + rows.map((row) => row.id), + ) + ) { + unsubscribe(); + } + }) ?? unsubscribe; } /** Long enough to notice the browser was asked to open, and why. */ diff --git a/src/tui/commands/built-in.test.ts b/src/tui/commands/built-in.test.ts index 896d27a3d..b5663c560 100644 --- a/src/tui/commands/built-in.test.ts +++ b/src/tui/commands/built-in.test.ts @@ -54,6 +54,13 @@ describe("/connect command", () => { }); }); +describe("MCP commands", () => { + it("keeps /mcp and aliases /mcps to the same surface", () => { + expect(getCommand("mcp")?.handler("", makeCtx())).toEqual({ type: "overlay", overlay: "mcp" }); + expect(getCommand("mcps")?.handler("", makeCtx())).toEqual({ type: "overlay", overlay: "mcp" }); + }); +}); + describe("/status command", () => { it("answers from the live fleet without sending anything to the model", () => { const ctx: CommandContext = { diff --git a/src/tui/commands/built-in.ts b/src/tui/commands/built-in.ts index d9bab7435..df16e6319 100644 --- a/src/tui/commands/built-in.ts +++ b/src/tui/commands/built-in.ts @@ -106,11 +106,13 @@ export function registerBuiltInCommands(): void { handler: (_args, _ctx) => ({ type: "paste-image" }), }); - registerCommand({ - name: "mcp", - description: "Show MCP servers and authorize the ones that need it", - handler: (_args, _ctx) => ({ type: "overlay", overlay: "mcp" }), - }); + for (const name of ["mcp", "mcps"]) { + registerCommand({ + name, + description: "Show MCP servers and authorize the ones that need it", + handler: (_args, _ctx) => ({ type: "overlay", overlay: "mcp" }), + }); + } registerCommand({ name: "cost", diff --git a/src/tui/dynamic-tool-runner.ts b/src/tui/dynamic-tool-runner.ts index 7d847d578..5bc1e970b 100644 --- a/src/tui/dynamic-tool-runner.ts +++ b/src/tui/dynamic-tool-runner.ts @@ -29,12 +29,13 @@ export function createDynamicToolRunner( const byName = new Map(); const addTools = (tools: AgentTool[]): void => { - for (const t of tools) { - if (byName.has(t.definition.name)) { - throw new DuplicateToolError(t.definition.name); - } - byName.set(t.definition.name, t); + const incoming = new Set(); + for (const tool of tools) { + const name = tool.definition.name; + if (byName.has(name) || incoming.has(name)) throw new DuplicateToolError(name); + incoming.add(name); } + for (const tool of tools) byName.set(tool.definition.name, tool); }; addTools(initial); diff --git a/src/tui/provider-connect.ts b/src/tui/provider-connect.ts index 37e1e493d..22d9dc8eb 100644 --- a/src/tui/provider-connect.ts +++ b/src/tui/provider-connect.ts @@ -6,7 +6,10 @@ */ import type { Settings } from "../config/settings.js"; -import { buildProviderSubmitHandler } from "./provider-setup-submit.js"; +import { + buildProviderSubmitHandler, + type PersistProviderSettings, +} from "./provider-setup-submit.js"; import { runProviderSetup, type ProviderSetupConfig } from "./provider-setup.js"; export interface ConnectProviderInput { @@ -15,6 +18,7 @@ export interface ConnectProviderInput { /** Project-local selection file, or null when it aliases global settings. */ readonly localSettingsPath: string | null; readonly existing: Settings | null; + readonly persistSettings?: PersistProviderSettings; readonly createRenderer?: ProviderSetupConfig["createRenderer"]; readonly startLogin?: ProviderSetupConfig["startLogin"]; readonly discoverOllamaModels?: ProviderSetupConfig["discoverOllamaModels"]; @@ -40,6 +44,7 @@ export async function connectProviderInline( input.settingsPath, input.existing, input.localSettingsPath, + input.persistSettings, ); const submitted = await runProviderSetup({ diff --git a/src/tui/provider-setup-submit.test.ts b/src/tui/provider-setup-submit.test.ts index 080a73e43..139e9ce18 100644 --- a/src/tui/provider-setup-submit.test.ts +++ b/src/tui/provider-setup-submit.test.ts @@ -34,6 +34,8 @@ await withMockedModule( ); const { buildProviderSubmitHandler } = await import("./provider-setup-submit.js"); +const { createGlobalSettingsWriter, persistGlobalHTTPMCPServer } = + await import("../mcp/add-server.js"); const { loadLocalSettings, loadSettings, localSettingsPath, resolveLocalSettingsPath } = await import("../config/settings.js"); import type { OAuthResult, ProviderFormValues, SubmitPhase } from "./provider-setup.js"; @@ -175,6 +177,42 @@ describe("buildProviderSubmitHandler", () => { }); }); + test("preserves a queued MCP add when a provider is persisted from stale runner settings", async () => { + await withTempDir(async (dir) => { + const path = join(dir, "settings.json"); + const writer = createGlobalSettingsWriter(path); + await persistGlobalHTTPMCPServer(writer, "linear", "https://mcp.linear.app/mcp"); + const staleRunnerSettings = { providers: {} }; + const submit = buildProviderSubmitHandler( + path, + staleRunnerSettings, + localSettingsPath(dir), + async (apply) => { + const next = await writer.update(apply); + if (next === null) throw new Error("global settings are unreadable"); + return next; + }, + ); + + await submit( + { + name: "local", + baseURL: "http://localhost:11434/v1", + apiKey: "", + model: "llama3", + oauthProfile: "", + }, + noopSetPhase, + { skipValidation: true }, + ); + + expect(await loadSettings(path)).toMatchObject({ + providers: { local: { keyless: true } }, + mcpServers: [{ name: "linear", type: "http", url: "https://mcp.linear.app/mcp" }], + }); + }); + }); + test("marks a save-anyway submit as unverified", async () => { await withTempDir(async (dir) => { const path = join(dir, "settings.json"); diff --git a/src/tui/provider-setup-submit.ts b/src/tui/provider-setup-submit.ts index 7e624d568..9f43e35fe 100644 --- a/src/tui/provider-setup-submit.ts +++ b/src/tui/provider-setup-submit.ts @@ -45,10 +45,17 @@ export async function persistConnectedSelection( * callers that already own it — never re-derived here so tests and the * mid-session connect path can pass an explicit file). */ +export type PersistProviderSettings = (apply: (base: Settings) => Settings) => Promise; + export function buildProviderSubmitHandler( settingsPath: string, existing: Settings | null, localSettingsFile: string | null, + persistSettings: PersistProviderSettings = async (apply) => { + const next = apply(existing ?? { providers: {} }); + await saveGlobalSettings(settingsPath, next); + return next; + }, ): ProviderSetupSubmit { return async (values, setPhase, { skipValidation, preset, oauth }) => { const { name, baseURL, apiKey, model } = values; @@ -74,8 +81,7 @@ export function buildProviderSubmitHandler( } setPhase("saving"); await oauth.commit(); - const base = existing ?? { providers: {} }; - await saveGlobalSettings(settingsPath, { + await persistSettings((base) => ({ ...base, defaultProvider: oauth.providerName, providers: { @@ -86,7 +92,7 @@ export function buildProviderSubmitHandler( defaultModel: selectedModel, }, }, - }); + })); await persistConnectedSelection(localSettingsFile, oauth.providerName, selectedModel); return; } @@ -140,8 +146,7 @@ export function buildProviderSubmitHandler( // stays open (phase label) until saveGlobalSettings resolves, so the user // sees confirmation before the screen is cleared. Full-spread merge so // plugins/pluginPaths/sessionMode/shell/tools survive re-onboarding. - const merged = mergeProviderIntoSettings(existing, providerName, newProvider); - await saveGlobalSettings(settingsPath, merged); + await persistSettings((base) => mergeProviderIntoSettings(base, providerName, newProvider)); // Same project-local selection contract as OAuth: credentials stay in // global storage; the local file is selection only so a restart in this // repo resolves to the provider just connected. diff --git a/src/tui/runner.ts b/src/tui/runner.ts index 0ebc9cb2a..f70e484fe 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -26,7 +26,6 @@ import { import { globalSettingsPath, loadLocalSettings, - loadGlobalSettingsWriteBase, listFavoriteModels, listRecentModels, loadSettings, @@ -34,7 +33,6 @@ import { markTelemetryNoticeShown, persistSkipPermissionsDefault, pushRecentModel, - saveGlobalSettings, shellTimeoutFromSettings, toolWatchdogFromSettings, markLastChangelogVersion, @@ -53,6 +51,11 @@ import { modelOptionId } from "./model-catalog.js"; import { resolveWaitForApproval, type ToolWatchdogConfig } from "./tool-execution-watchdog.js"; import { attachApprovalBudget, createGateRequestApproval } from "./request-approval.js"; import { codexProfileFromProviderName, isCodexProviderName } from "../config/codex-providers.js"; +import { + createGlobalSettingsWriter, + persistGlobalHTTPMCPServer, + validateMCPServerName, +} from "../mcp/add-server.js"; import { xaiProfileFromProviderName } from "../config/xai-providers.js"; import type { PluginsAdmin, PluginDescriptor } from "../plugins/admin.js"; import type { PluginManifest } from "../plugins/manifest.js"; @@ -172,7 +175,12 @@ import { createWorktreeRootsProvider } from "../permission/worktree-roots.js"; import { createPermissionsAdmin, type ScopedApproval } from "../permission/admin.js"; import type { GrantScope } from "../permission/types.js"; -import { createAgentToolset, type MCPServerState, type OperatorResult } from "../agent/tools.js"; +import { + createAgentToolset, + type MCPConnectCallbacks, + type MCPServerState, + type OperatorResult, +} from "../agent/tools.js"; import { createAgentWithLiveToolDispatch } from "../agent/live-tool-dispatch.js"; import { collectWebPlugins, @@ -259,7 +267,7 @@ import { import { applyLiveModelSwitch } from "../session/live-model-switch.js"; import { createAttachmentRehydrateTransform } from "../session/attachment-store.js"; import { createModelSummarizer, type SummaryContext } from "../session/summarizer.js"; -import { COMMAND_NAME, ID_PREFIX, LOG_NAMESPACE_ROOT } from "../branding.js"; +import { COMMAND_NAME, ID_PREFIX, LOG_NAMESPACE_ROOT, SETTINGS_DIR_NAME } from "../branding.js"; import { deliverAgentMessage } from "./deliver-agent-message.js"; const tuiLogger = getLogger([LOG_NAMESPACE_ROOT, "tui"]); @@ -795,6 +803,7 @@ export async function runTUI(initialConfig: Config): Promise { try { const emitter = createTUIEventEmitter(); + const globalSettingsWriter = createGlobalSettingsWriter(config.globalSettingsPath); const initialHookEnabled: Record = Object.fromEntries( Object.entries(config.settings?.hooks ?? {}).map(([id, v]) => [id, v.enabled]), ); @@ -834,15 +843,15 @@ export async function runTUI(initialConfig: Config): Promise { ...(config.settings?.hooks ?? {}), }; const persistHookSettings = async (): Promise => { - const base = await loadGlobalSettingsWriteBase(config.globalSettingsPath); - if (base === null) { + const result = await globalSettingsWriter.mutate((base) => ({ + ...base, + hooks: liveHookConfig, + })); + if (result === "skipped") { tuiLogger.warn("Skipping hook settings write: unreadable global settings at {path}", { path: config.globalSettingsPath, }); - return; } - const next: Settings = { ...base, hooks: liveHookConfig }; - await saveGlobalSettings(config.globalSettingsPath, next); }; const setHookEnabled = async (id: string, enabled: boolean): Promise => { hookManager.setEnabled(id, enabled); @@ -981,19 +990,19 @@ export async function runTUI(initialConfig: Config): Promise { const persistPluginSettings = async (): Promise => { // Absent file → fresh base; unreadable/invalid → skip write so we never // clobber a corrupt settings file by rewriting from a minimal shell. - const base = await loadGlobalSettingsWriteBase(config.globalSettingsPath); - if (base === null) { + const result = await globalSettingsWriter.mutate((base) => { + const next: Settings = { ...base, plugins: livePluginConfig }; + if (livePluginPaths.length > 0) next.pluginPaths = livePluginPaths; + else delete next.pluginPaths; + if (liveWebOverride !== undefined) next.web = liveWebOverride; + else delete next.web; + return next; + }); + if (result === "skipped") { tuiLogger.warn("Skipping plugin settings write: unreadable global settings at {path}", { path: config.globalSettingsPath, }); - return; } - const next: Settings = { ...base, plugins: livePluginConfig }; - if (livePluginPaths.length > 0) next.pluginPaths = livePluginPaths; - else delete next.pluginPaths; - if (liveWebOverride !== undefined) next.web = liveWebOverride; - else delete next.web; - await saveGlobalSettings(config.globalSettingsPath, next); }; const pluginsAdmin: PluginsAdmin = { list: () => pluginDescriptors, @@ -1615,6 +1624,7 @@ export async function runTUI(initialConfig: Config): Promise { // `connectedMcpServers` (persisted run metadata) this keeps the ones that // failed or are still waiting on authorization. const mcpStates = new Map(); + const mcpConnectController = new AbortController(); const writeRunSnapshot = async ( status: RunState["status"], @@ -1989,7 +1999,11 @@ export async function runTUI(initialConfig: Config): Promise { // this whole launch, and the render stamp means events start normally on // the next one. Keyed off the same TRUE global settings file as // `onboarded` above. - const onChangeTelemetryEnabled = createTelemetryToggleHandler(trueGlobalSettingsPath); + const onChangeTelemetryEnabled = createTelemetryToggleHandler( + trueGlobalSettingsPath, + undefined, + globalSettingsWriter.enqueue, + ); const telemetryFirstRun = telemetryFirstRunPending(globalSettingsForOnboarding); const telemetryNotice = telemetryStartupNotice(globalSettingsForOnboarding); // Tracks the user's intent (persisted opt-in, updated live by the settings @@ -2001,9 +2015,11 @@ export async function runTUI(initialConfig: Config): Promise { // sessions do not want. /cost stays available regardless. let liveShowPromptCost = config.settings?.showPromptCost ?? false; if (telemetryFirstRun) { - void markTelemetryNoticeShown(trueGlobalSettingsPath).catch(() => { - // Best-effort: worst case the notice shows again next launch. - }); + void globalSettingsWriter + .enqueue(() => markTelemetryNoticeShown(trueGlobalSettingsPath)) + .catch(() => { + // Best-effort: worst case the notice shows again next launch. + }); } // Post-upgrade release notes watermark policy (CL-5475): @@ -2019,41 +2035,31 @@ export async function runTUI(initialConfig: Config): Promise { const notesShown = false; const stampVersion = stampVersionAfterStartup(changelogDecision, notesShown); if (stampVersion !== null) { - void markLastChangelogVersion(trueGlobalSettingsPath, stampVersion).catch(() => { - // Best-effort watermark. - }); + void globalSettingsWriter + .enqueue(() => markLastChangelogVersion(trueGlobalSettingsPath, stampVersion)) + .catch(() => { + // Best-effort watermark. + }); } - // One tail for every RMW of config.globalSettingsPath from this runner so - // /yolo and /settings toggles cannot stale-RMW each other. - let persistTail = Promise.resolve(); - const enqueueGlobalPersist = (job: () => Promise): Promise => { - const run = persistTail.then(job); - persistTail = run.then( - () => undefined, - () => undefined, - ); - return run; - }; + // Every settings RMW in this runner shares this tail, including writes to + // the true global path during a --config session. + const enqueueGlobalPersist = globalSettingsWriter.enqueue; // Absent file → fresh base; unreadable/invalid → skip the write rather than // clobber a corrupt settings file with a minimal shell. - const persistGlobalSettings = ( + const persistGlobalSettings = async ( what: string, apply: (base: Settings) => Settings, - ): Promise => - enqueueGlobalPersist(async () => { - const base = await loadGlobalSettingsWriteBase(config.globalSettingsPath); - if (base === null) { - tuiLogger.warn("Skipping {what} write: unreadable global settings at {path}", { - what, - path: config.globalSettingsPath, - }); - return false; - } - await saveGlobalSettings(config.globalSettingsPath, apply(base)); - return true; + ): Promise => { + const result = await globalSettingsWriter.mutate(apply); + if (result === "ok") return true; + tuiLogger.warn("Skipping {what} write: unreadable global settings at {path}", { + what, + path: config.globalSettingsPath, }); + return false; + }; const commandContext: CommandContext = { signalClear: newSession, @@ -2267,6 +2273,23 @@ export async function runTUI(initialConfig: Config): Promise { }, onSystemNotice: systemNotice, }); + const mcpConnectCallbacks: MCPConnectCallbacks = { + interactiveAuth: true, + onStatus: (status) => { + mcpStates.set(status.name, status); + emitter.emit("mcp.status", status); + if (status.state === "connected") { + connectedMcpServers = [ + ...connectedMcpServers.filter((server) => server.name !== status.name), + { name: status.name, toolCount: status.tools.length }, + ]; + void persistRunSnapshot("running"); + } + }, + // MCP tools register for dispatch but stay blind until tool_search promotes them. + onToolsChanged: (definitions) => + directorHolder.instance?.updateToolDefinitions(computeAdvertised(definitions)), + }; const host = await mountRunnerHost({ // An unnamed session shows nothing rather than a placeholder. @@ -2335,6 +2358,12 @@ export async function runTUI(initialConfig: Config): Promise { settingsPath: trueGlobalSettingsPath, localSettingsPath: localSettingsFile, existing: config.settings ?? null, + persistSettings: async (apply) => { + const next = await globalSettingsWriter.updateAt(trueGlobalSettingsPath, apply); + if (next === null) throw new Error("global settings are unreadable"); + config = { ...config, settings: next }; + return next; + }, createRenderer: () => Promise.resolve(host.renderer), }); } catch (err) { @@ -2423,11 +2452,14 @@ export async function runTUI(initialConfig: Config): Promise { const ref: ModelRef = { provider, model }; void (async () => { - const onDisk = (await loadGlobalSettingsWriteBase(trueGlobalSettingsPath)) ?? { - providers: {}, - }; - const next = pushRecentModel(onDisk, ref); - await saveGlobalSettings(trueGlobalSettingsPath, next); + let next: Settings | undefined; + const result = await globalSettingsWriter.mutateAt(trueGlobalSettingsPath, (onDisk) => { + next = pushRecentModel(onDisk, ref); + return next; + }); + if (result === "skipped" || next === undefined) { + throw new Error("global settings are unreadable"); + } config = { ...config, settings: next }; host.refreshModels(listRecentModels(next), listFavoriteModels(next)); })().catch((err: unknown) => { @@ -2441,11 +2473,14 @@ export async function runTUI(initialConfig: Config): Promise { if (sep <= 0) return; const ref: ModelRef = { provider: id.slice(0, sep), model: id.slice(sep + 1) }; void (async () => { - const onDisk = (await loadGlobalSettingsWriteBase(trueGlobalSettingsPath)) ?? { - providers: {}, - }; - const next = toggleFavoriteModel(onDisk, ref); - await saveGlobalSettings(trueGlobalSettingsPath, next); + let next: Settings | undefined; + const result = await globalSettingsWriter.mutateAt(trueGlobalSettingsPath, (onDisk) => { + next = toggleFavoriteModel(onDisk, ref); + return next; + }); + if (result === "skipped" || next === undefined) { + throw new Error("global settings are unreadable"); + } config = { ...config, settings: next }; host.refreshModels(listRecentModels(next), listFavoriteModels(next)); })().catch((err: unknown) => { @@ -2459,15 +2494,18 @@ export async function runTUI(initialConfig: Config): Promise { if (sep <= 0) return; const ref: ModelRef = { provider: id.slice(0, sep), model: id.slice(sep + 1) }; void (async () => { - const onDisk = (await loadGlobalSettingsWriteBase(trueGlobalSettingsPath)) ?? { - providers: {}, - }; - const next = setDefaultModel( - onDisk, - ref, - config.providers.find((provider) => provider.name === ref.provider), - ); - await saveGlobalSettings(trueGlobalSettingsPath, next); + let next: Settings | undefined; + const result = await globalSettingsWriter.mutateAt(trueGlobalSettingsPath, (onDisk) => { + next = setDefaultModel( + onDisk, + ref, + config.providers.find((provider) => provider.name === ref.provider), + ); + return next; + }); + if (result === "skipped" || next === undefined) { + throw new Error("global settings are unreadable"); + } await persistConnectedSelection(localSettingsFile, ref.provider, ref.model); config = { ...config, settings: next }; systemNotice(`Default set to ${ref.model} (${ref.provider})`); @@ -2582,6 +2620,43 @@ export async function runTUI(initialConfig: Config): Promise { ...(status.state === "failed" ? { error: status.error } : {}), })), openAuthURL: (url) => openInBrowser(url), + subscribe: (listener) => { + emitter.on("mcp.status", listener); + return () => emitter.off("mcp.status", listener); + }, + addServer: async (name, url) => { + const result = await persistGlobalHTTPMCPServer( + globalSettingsWriter, + name, + url, + config.mcpServersSource ?? "none", + toolset.hasMCPServer, + ); + if (!result.ok) { + const message = + result.reason === "local-shadow" + ? `Cannot add a global MCP server while ${SETTINGS_DIR_NAME}/settings.json ` + + "defines mcpServers; remove that local list and restart first." + : result.reason === "duplicate" || result.reason === "active" + ? `An MCP server named "${name.trim()}" already exists or is connecting.` + : result.reason === "skipped" + ? "Could not read global settings, so no MCP server was added." + : result.reason === "invalid-name" + ? (validateMCPServerName(name.trim()) ?? "Enter a valid server name first.") + : "Enter an absolute HTTP(S) URL first."; + return { ok: false, message }; + } + config = { ...config, settings: result.settings }; + void toolset + .connectMCPServer(result.server, mcpConnectCallbacks, mcpConnectController.signal) + .catch((err: unknown) => { + if (err instanceof Error && err.name === "AbortError") return; + tuiLogger.error("Late MCP connect failed: {error}", { + error: err instanceof Error ? err.message : String(err), + }); + }); + return { ok: true, message: `Added ${result.server.name}; connecting now.` }; + }, }, hooks: { list: () => @@ -2778,30 +2853,8 @@ export async function runTUI(initialConfig: Config): Promise { // settled, reload-if-idle so construction-time maps match, then resume any // persisted workflow. Aborted on exit so an unfinished auth wait does not // keep the process alive. - const mcpConnectController = new AbortController(); void toolset - .connectMCP( - { - interactiveAuth: true, - onStatus: (status) => { - mcpStates.set(status.name, status); - emitter.emit("mcp.status", status); - if (status.state === "connected") { - connectedMcpServers = [ - ...connectedMcpServers.filter((s) => s.name !== status.name), - { name: status.name, toolCount: status.tools.length }, - ]; - void persistRunSnapshot("running"); - } - }, - // MCP tools register for dispatch but stay unadvertised (blind) until - // tool_search promotes them, so a fresh connection never grows the wire - // set on its own — only a subsequent discovery does. - onToolsChanged: (definitions) => - directorHolder.instance?.updateToolDefinitions(computeAdvertised(definitions)), - }, - mcpConnectController.signal, - ) + .connectMCP(mcpConnectCallbacks, mcpConnectController.signal) .then(async () => { if (toolset.dynamicRunner.currentDefinitions().length > baseToolCount) { pendingReload = true; diff --git a/src/tui/shell.ts b/src/tui/shell.ts index e5605d0ac..95c424ef1 100644 --- a/src/tui/shell.ts +++ b/src/tui/shell.ts @@ -1940,6 +1940,7 @@ interface PriorOverlaySnapshot { readonly onCycle: ((itemId: string, direction: -1 | 1) => void) | null; readonly describe: ((itemId: string) => ItemDescription | null) | null; readonly onAction: ((itemId: string, key: KeyEvent) => boolean) | null; + readonly onPaste: ((text: string) => void) | null; readonly answer: OverlayAnswerState | null; readonly titleText: string; readonly onCancel: (() => void) | null; @@ -1961,6 +1962,8 @@ interface ShellInternals { overlayRawBodyText: string; /** Snapshot when palette stacks over another primary overlay. */ priorOverlay: PriorOverlaySnapshot | null; + /** Advances whenever a new overlay takes ownership of the shared host. */ + overlayGeneration: number; /** Optional stable ids aligned with overlayItems for the open primary. */ overlayItemIds: readonly string[]; /** Optional plain chosen values aligned with overlayItems for the open primary. */ @@ -1977,6 +1980,8 @@ interface ShellInternals { overlayDescribe: ((itemId: string) => ItemDescription | null) | null; /** Per-open bare-key claim for the open primary overlay. */ overlayOnAction: ((itemId: string, key: KeyEvent) => boolean) | null; + /** Per-open bracketed-paste owner for synthetic text panes. */ + overlayOnPaste: ((text: string) => void) | null; /** Whether the open primary advertises Alt+A and yields å/Å from type-to-filter. */ overlayAddProviderHint: boolean; /** Whether the open primary advertises Alt+D in the footer hints. */ @@ -3534,13 +3539,13 @@ export interface OpenListOverlayOpts { */ readonly describe?: (itemId: string) => ItemDescription | null; /** - * Per-open bare-key claim, checked for any key the list's own navigation - * (arrows, expand, accept) leaves unclaimed. Returns true when the key was - * used, so the shell can `preventDefault` it and stop. Scoped to this open - * only — never reachable while no list overlay is open, so it cannot shadow - * prompt typing. + * Per-open bare-key claim, checked before list navigation. Returning false + * leaves the key available to the ordinary j/k and arrow handlers. Scoped to + * this open only, so it cannot shadow prompt typing. */ readonly onAction?: (itemId: string, key: KeyEvent) => boolean; + /** Per-open bracketed-paste target for synthetic text panes. */ + readonly onPaste?: (text: string) => void; /** * Per-open free-text answer. When set the overlay paints an answer field the * operator can Tab into and type into, and submitting it closes the overlay @@ -3611,6 +3616,7 @@ export function openListOverlay(shell: AppShell, opts?: OpenListOverlayOpts): vo onCycle: bag.overlayOnCycle, describe: bag.overlayDescribe, onAction: bag.overlayOnAction, + onPaste: bag.overlayOnPaste, answer: bag.overlayAnswer, titleText: bag.overlayTitleText, onCancel: bag.overlayOnCancel, @@ -3635,6 +3641,7 @@ export function openListOverlay(shell: AppShell, opts?: OpenListOverlayOpts): vo const bag = internals.get(shell); if (bag) { + bag.overlayGeneration += 1; // Palette open does not own primary accept; leave prior snapshot's callback. if (!isPalette) { bag.overlayItemIds = opts?.itemIds ? [...opts.itemIds] : []; @@ -3645,6 +3652,7 @@ export function openListOverlay(shell: AppShell, opts?: OpenListOverlayOpts): vo bag.overlayOnCycle = opts?.onCycle ?? null; bag.overlayDescribe = opts?.describe ?? null; bag.overlayOnAction = opts?.onAction ?? null; + bag.overlayOnPaste = opts?.onPaste ?? null; bag.overlayOnCancel = opts?.onCancel ?? null; bag.overlayAddProviderHint = opts?.addProviderHint ?? false; bag.overlaySetDefaultHint = opts?.setDefaultHint ?? false; @@ -3668,6 +3676,7 @@ export function openListOverlay(shell: AppShell, opts?: OpenListOverlayOpts): vo bag.overlayOnCycle = opts?.onCycle ?? null; bag.overlayDescribe = opts?.describe ?? null; bag.overlayOnAction = opts?.onAction ?? null; + bag.overlayOnPaste = opts?.onPaste ?? null; bag.overlayOnCancel = opts?.onCancel ?? null; bag.overlayAddProviderHint = opts?.addProviderHint ?? false; bag.overlaySetDefaultHint = opts?.setDefaultHint ?? false; @@ -4077,18 +4086,10 @@ export function closeInsetOverlay(shell: AppShell): void { const bag = internals.get(shell); if (bag) bag.listFilter = null; const prior = wasPalette ? (bag?.priorOverlay ?? null) : null; - // Permissions/operator overlays back a caller awaiting ev.resolve — Esc must - // still settle that promise (as a deny/cancel) or the caller hangs forever. - // model_picker/add_provider onCancel is optional back-navigation for - // callers that set one; palette/mentions/copy have no awaited caller and - // drop silently. - const cancelable = - !prior && - (shell.overlayKind === "permissions" || - shell.overlayKind === "operator" || - shell.overlayKind === "model_picker" || - shell.overlayKind === "add_provider"); - const onCancel = cancelable ? (bag?.overlayOnCancel ?? null) : null; + // A primary overlay that registers onCancel owns cleanup for every dismiss + // path. A palette stacked over another overlay restores that prior frame + // instead, so its callback must remain untouched. + const onCancel = !prior ? (bag?.overlayOnCancel ?? null) : null; shell.overlayList = null; shell.overlayKind = null; @@ -4107,6 +4108,7 @@ export function closeInsetOverlay(shell: AppShell): void { bag.overlayOnCycle = null; bag.overlayDescribe = null; bag.overlayOnAction = null; + bag.overlayOnPaste = null; bag.overlayAddProviderHint = false; bag.overlaySetDefaultHint = false; bag.overlayAnswer = null; @@ -4136,6 +4138,7 @@ export function closeInsetOverlay(shell: AppShell): void { bag.overlayOnCycle = prior.onCycle; bag.overlayDescribe = prior.describe; bag.overlayOnAction = prior.onAction; + bag.overlayOnPaste = prior.onPaste; bag.overlayAnswer = prior.answer; bag.overlayTitleText = prior.titleText; bag.overlayOnCancel = prior.onCancel; @@ -4269,6 +4272,72 @@ export function setOverlayBody(shell: AppShell, text: string, maxLines = 8): voi paintOverlayList(shell); } +export interface OverlayContinuationToken { + readonly generation: number; +} + +/** Capture ownership of the currently idle overlay host for an async continuation. */ +export function captureOverlayContinuation(shell: AppShell): OverlayContinuationToken { + return { generation: internals.get(shell)?.overlayGeneration ?? -1 }; +} + +/** True only while no newer overlay has taken ownership of the shared host. */ +export function isOverlayContinuationCurrent( + shell: AppShell, + token: OverlayContinuationToken, +): boolean { + return ( + !shell.disposed && + shell.overlayList === null && + internals.get(shell)?.overlayGeneration === token.generation + ); +} + +/** + * Refresh an overlay owned by either the foreground or the frame beneath a + * stacked palette. Returns false once that overlay no longer owns either slot. + */ +export function setOwnedOverlayItems( + shell: AppShell, + kind: PrimaryOverlayKind, + items: readonly string[], + itemIds: readonly string[], +): boolean { + const bag = internals.get(shell); + if (!bag) return false; + + if (shell.overlayKind === kind && shell.overlayList !== null) { + const activeId = bag.overlayItemIds[shell.overlayList.activeIndex]; + setOverlayItems(shell, items, itemIds); + const activeIndex = activeId === undefined ? -1 : itemIds.indexOf(activeId); + if (activeIndex >= 0 && shell.overlayList.activeIndex !== activeIndex) { + shell.overlayList = createListViewport({ + count: items.length, + height: shell.overlayList.height, + activeIndex, + }); + paintOverlayList(shell); + } + return true; + } + + const prior = bag.priorOverlay; + if (prior?.kind !== kind) return false; + const activeId = prior.itemIds[prior.list.activeIndex]; + const activeIndex = activeId === undefined ? -1 : itemIds.indexOf(activeId); + bag.priorOverlay = { + ...prior, + items: [...items], + itemIds: [...itemIds], + list: createListViewport({ + count: items.length, + height: prior.list.height, + activeIndex: activeIndex >= 0 ? activeIndex : prior.list.activeIndex, + }), + }; + return true; +} + /** * Replace the open overlay's item labels (and optionally ids) in place, * keeping the active row's position. Cycling a value redraws the row it @@ -5596,9 +5665,15 @@ export function createAppShell(renderer: ShellRenderer, options?: AppShellOption let lastKeyAt = 0; let lastKeyWasPrintable = false; let suppressNextLinefeed = false; - const onPaste = (): void => { - if (internals.get(shell)?.inputSuspended === true) return; + const onPaste = (event: { bytes: Uint8Array; preventDefault: () => void }): void => { + if (disposed) return; + const bag = internals.get(shell); + if (bag?.inputSuspended === true) return; sawBracketedPaste = true; + if (shell.overlayList !== null && bag?.overlayOnPaste) { + event.preventDefault(); + bag.overlayOnPaste(new TextDecoder().decode(event.bytes)); + } }; const onKey = (key: KeyEvent): void => { @@ -5681,6 +5756,12 @@ export function createAppShell(renderer: ShellRenderer, options?: AppShellOption key.preventDefault(); return; } + // Per-overlay bare-key owners (including text panes) get first refusal. + // Ordinary lists return false here, preserving j/k navigation below. + if (runOverlayAction(shell, key)) { + key.preventDefault(); + return; + } if (key.name === "up" || key.name === "k") { key.preventDefault(); moveOverlaySelection(shell, -1); @@ -5735,10 +5816,6 @@ export function createAppShell(renderer: ShellRenderer, options?: AppShellOption return; } } - if (runOverlayAction(shell, key)) { - key.preventDefault(); - return; - } if (key.name === "return" || key.name === "enter") { if (!key.meta && !key.option && !key.ctrl) { key.preventDefault(); @@ -6156,6 +6233,10 @@ export function createAppShell(renderer: ShellRenderer, options?: AppShellOption disposed: false, dispose: () => { if (disposed) return; + // Unwind a stacked palette first, then let the primary overlay's owner + // release subscriptions or settle awaited cancellation exactly once. + let overlayGuard = 4; + while (shell.overlayList !== null && overlayGuard-- > 0) closeInsetOverlay(shell); disposed = true; shell.disposed = true; if (wireKeys) { @@ -6190,6 +6271,7 @@ export function createAppShell(renderer: ShellRenderer, options?: AppShellOption overlayMinBodyRows: undefined, overlayRawBodyText: "", priorOverlay: null, + overlayGeneration: 0, overlayItemIds: [], overlayItemValues: [], overlayOnAccept: null, @@ -6198,6 +6280,7 @@ export function createAppShell(renderer: ShellRenderer, options?: AppShellOption overlayOnCycle: null, overlayDescribe: null, overlayOnAction: null, + overlayOnPaste: null, overlayAddProviderHint: false, overlaySetDefaultHint: false, inputSuspended: false, diff --git a/tests/unit/telemetry-toggle.test.ts b/tests/unit/telemetry-toggle.test.ts index 8acb53838..82e98975b 100644 --- a/tests/unit/telemetry-toggle.test.ts +++ b/tests/unit/telemetry-toggle.test.ts @@ -47,6 +47,68 @@ function fakeDeps(overrides: Partial = {}): { return { deps, getInstance: () => instance, fetchCalls: () => calls }; } +test("queued enable cannot publish after a later opt-out while disable load is blocked", async () => { + let releaseEnable: (() => void) | undefined; + let releaseDisableLoad: (() => void) | undefined; + let signalEnableStarted: (() => void) | undefined; + let signalDisableLoadStarted: (() => void) | undefined; + const enableStarted = new Promise((resolve) => { + signalEnableStarted = resolve; + }); + const disableLoadStarted = new Promise((resolve) => { + signalDisableLoadStarted = resolve; + }); + const published: boolean[] = []; + const { deps, getInstance, fetchCalls } = fakeDeps({ + setTelemetry: (telemetry) => { + published.push(telemetry.enabled); + current = telemetry; + }, + getTelemetry: () => current, + ensureTelemetrySettings: async () => { + signalEnableStarted?.(); + await new Promise((resolve) => { + releaseEnable = resolve; + }); + return { providers: {}, telemetry: { enabled: true, installationId: "id" } }; + }, + loadSettings: async () => { + signalDisableLoadStarted?.(); + await new Promise((resolve) => { + releaseDisableLoad = resolve; + }); + return { providers: {}, telemetry: { enabled: true, installationId: "id" } }; + }, + }); + let current = getInstance(); + let tail = Promise.resolve(); + const enqueue = (job: () => Promise): Promise => { + const run = tail.then(job); + tail = run.then( + () => undefined, + () => undefined, + ); + return run; + }; + const handler = createTelemetryToggleHandler("/fake/path", deps, enqueue); + + handler(true); + await enableStarted; + handler(false); + const publishedAfterOff = published.length; + releaseEnable?.(); + await disableLoadStarted; + + expect(published.slice(publishedAfterOff)).not.toContain(true); + expect(current.enabled).toBe(false); + current.capture("cli_start"); + await current.flush(); + expect(fetchCalls()).toBe(0); + + releaseDisableLoad?.(); + await tail; +}); + test("toggle off disables the singleton synchronously, before any await", () => { const { deps, getInstance } = fakeDeps(); const handler = createTelemetryToggleHandler("/fake/path", deps); From fa7e6f89ff3a469d498b01cb9430cbb25fa13c08 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 31 Aug 2026 19:29:48 -0700 Subject: [PATCH 2/6] Harden MCP add against abort, local shadow, and bad URLs Live Streamable HTTP transports now pass the dispose signal through requestInit so OAuth auth() refreshes abort on quit. The add row is hidden while local mcpServers shadow global settings. HTTP URLs must be real http(s) hosts without userinfo, and persisted as href. --- src/mcp/add-server.test.ts | 43 ++++++++++++++++++- src/mcp/add-server.ts | 28 +++++++++--- src/mcp/client-auth-policy.test.ts | 69 +++++++++++++++++++++++++++--- src/mcp/client.ts | 30 ++++++++++--- src/tui/command-surfaces.test.ts | 31 ++++++++++++++ src/tui/command-surfaces.ts | 40 +++++++++-------- src/tui/runner.ts | 1 + 7 files changed, 204 insertions(+), 38 deletions(-) diff --git a/src/mcp/add-server.test.ts b/src/mcp/add-server.test.ts index b76200c2f..14722c915 100644 --- a/src/mcp/add-server.test.ts +++ b/src/mcp/add-server.test.ts @@ -3,7 +3,11 @@ import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { createGlobalSettingsWriter, persistGlobalHTTPMCPServer } from "./add-server.js"; +import { + createGlobalSettingsWriter, + isAbsoluteHTTPURL, + persistGlobalHTTPMCPServer, +} from "./add-server.js"; import { isReadOnlyMcpTool, mcpToolName } from "./tool-name.js"; const dirs: string[] = []; @@ -59,6 +63,17 @@ describe("persistGlobalHTTPMCPServer", () => { ok: false, reason: "invalid-url", }); + expect(await persistGlobalHTTPMCPServer(writer, "other", "relative/path")).toEqual({ + ok: false, + reason: "invalid-url", + }); + expect(await persistGlobalHTTPMCPServer(writer, "other", "https://.")).toEqual({ + ok: false, + reason: "invalid-url", + }); + expect(await persistGlobalHTTPMCPServer(writer, "other", "https://user:pass@host/mcp")).toEqual( + { ok: false, reason: "invalid-url" }, + ); expect(await persistGlobalHTTPMCPServer(writer, "linear", "https://new.test/mcp")).toEqual({ ok: false, reason: "duplicate", @@ -95,6 +110,32 @@ describe("persistGlobalHTTPMCPServer", () => { expect(await readFile(path, "utf8")).toBe(original); }); + test("rejects degenerate and credential-bearing HTTP URLs", () => { + expect(isAbsoluteHTTPURL("relative/path")).toBe(false); + expect(isAbsoluteHTTPURL("ftp://example.test/mcp")).toBe(false); + expect(isAbsoluteHTTPURL("https://.")).toBe(false); + expect(isAbsoluteHTTPURL("https:example.com")).toBe(false); + expect(isAbsoluteHTTPURL("https:////evil.com")).toBe(false); + expect(isAbsoluteHTTPURL("https://user:pass@host/mcp")).toBe(false); + expect(isAbsoluteHTTPURL("https://mcp.linear.app/mcp")).toBe(true); + }); + + test("persists the normalized href rather than the typed URL", async () => { + const path = await settingsPath(); + await Bun.write(path, JSON.stringify({ providers: {} })); + const writer = createGlobalSettingsWriter(path); + + expect( + await persistGlobalHTTPMCPServer(writer, "linear", "https://CUSTOM.example:443/mcp"), + ).toMatchObject({ + ok: true, + server: { name: "linear", type: "http", url: "https://custom.example/mcp" }, + }); + expect(JSON.parse(await readFile(path, "utf8")).mcpServers).toEqual([ + { name: "linear", type: "http", url: "https://custom.example/mcp" }, + ]); + }); + test("serializes a delayed MCP add with hook and plugin mutation", async () => { const path = await settingsPath(); await writeFile(path, JSON.stringify({ providers: {}, showPromptCost: true })); diff --git a/src/mcp/add-server.ts b/src/mcp/add-server.ts index 079453117..24b3f5b64 100644 --- a/src/mcp/add-server.ts +++ b/src/mcp/add-server.ts @@ -86,15 +86,29 @@ export function validateMCPServerName(value: string): string | null { return null; } -export function isAbsoluteHTTPURL(value: string): boolean { +export function parseAbsoluteHTTPURL(value: string): URL | null { + const trimmed = value.trim(); + const scheme = trimmed.match(/^(https?):\/\//i); + if (scheme === null) return null; + const afterScheme = trimmed.slice(scheme[0].length); + if (afterScheme.startsWith("/")) return null; try { - const url = new URL(value); - return (url.protocol === "http:" || url.protocol === "https:") && url.origin !== "null"; + const url = new URL(trimmed); + if (url.protocol !== "http:" && url.protocol !== "https:") return null; + if (url.username !== "" || url.password !== "") return null; + if (url.hostname.length === 0 || url.hostname === "." || url.hostname.startsWith(".")) { + return null; + } + return url; } catch { - return false; + return null; } } +export function isAbsoluteHTTPURL(value: string): boolean { + return parseAbsoluteHTTPURL(value) !== null; +} + export async function persistGlobalHTTPMCPServer( writer: GlobalSettingsWriter, rawName: string, @@ -103,12 +117,12 @@ export async function persistGlobalHTTPMCPServer( isNameActive: (name: string) => boolean = () => false, ): Promise { const name = rawName.trim(); - const url = rawURL.trim(); + const parsedURL = parseAbsoluteHTTPURL(rawURL); if (validateMCPServerName(name) !== null) return { ok: false, reason: "invalid-name" }; - if (!isAbsoluteHTTPURL(url)) return { ok: false, reason: "invalid-url" }; + if (parsedURL === null) return { ok: false, reason: "invalid-url" }; if (source === "local") return { ok: false, reason: "local-shadow" }; - const server: MCPServerConfig = { name, type: "http", url }; + const server: MCPServerConfig = { name, type: "http", url: parsedURL.href }; let active = false; let duplicate = false; const settings = await writer.update((base) => { diff --git a/src/mcp/client-auth-policy.test.ts b/src/mcp/client-auth-policy.test.ts index 3b42276eb..70dbc8cab 100644 --- a/src/mcp/client-auth-policy.test.ts +++ b/src/mcp/client-auth-policy.test.ts @@ -16,6 +16,24 @@ let toolDiscoveryAborts = 0; let blockTokenExchange = false; let tokenExchangeSignals: (AbortSignal | null | undefined)[] = []; let tokenExchangeAborts = 0; +let lastTransportAuth: (() => Promise) | undefined; +let tokenRefreshSignals: (AbortSignal | null | undefined)[] = []; +let tokenRefreshAborts = 0; + +function hangUntilAbort( + signal: AbortSignal | null | undefined, + onAbort: () => void, + fallback: string, +): Promise { + return new Promise((_resolve, reject) => { + const fail = (): void => { + onAbort(); + reject(signal?.reason ?? new Error(fallback)); + }; + if (signal?.aborted === true) fail(); + else signal?.addEventListener("abort", fail, { once: true }); + }); +} const authProvider = { resetAuthorization: async () => undefined }; @@ -45,6 +63,11 @@ await withMockedModule( } return { tools: [] }; } + async callTool(): Promise<{ content: [] }> { + if (lastTransportAuth === undefined) throw new Error("no live HTTP transport"); + await lastTransportAuth(); + return { content: [] }; + } async close(): Promise { clientCloses += 1; } @@ -62,21 +85,32 @@ await withMockedModule( private readonly options?: { requestInit?: RequestInit }, ) { transportOptions.push(options); + lastTransportAuth = () => this.auth(); } async finishAuth(): Promise { const signal = this.options?.requestInit?.signal; tokenExchangeSignals.push(signal); if (blockTokenExchange) { - await new Promise((_resolve, reject) => { - const onAbort = (): void => { + await hangUntilAbort( + signal, + () => { tokenExchangeAborts += 1; - reject(signal?.reason ?? new Error("token exchange aborted")); - }; - if (signal?.aborted === true) onAbort(); - else signal?.addEventListener("abort", onAbort, { once: true }); - }); + }, + "token exchange aborted", + ); } } + async auth(): Promise { + const signal = this.options?.requestInit?.signal; + tokenRefreshSignals.push(signal); + await hangUntilAbort( + signal, + () => { + tokenRefreshAborts += 1; + }, + "token refresh aborted", + ); + } get sessionId(): string | undefined { return undefined; } @@ -133,6 +167,9 @@ describe("HTTP MCP auth policy", () => { blockTokenExchange = false; tokenExchangeSignals = []; tokenExchangeAborts = 0; + lastTransportAuth = undefined; + tokenRefreshSignals = []; + tokenRefreshAborts = 0; }); test("built-in anonymous Exa treats 401 as a normal failure without OAuth machinery", async () => { @@ -212,6 +249,24 @@ describe("HTTP MCP auth policy", () => { expect(callbackCloses).toBe(1); }); + test("aborts a live-transport auth refresh that ignores the transport abort controller", async () => { + const abort = new AbortController(); + const result = await connectMCPServer( + { name: "linear", type: "http", url: "https://mcp.linear.app/mcp" }, + { signal: abort.signal }, + ); + expect(result.ok).toBe(true); + if (!result.ok) return; + expect(transportOptions).toEqual([{ authProvider, requestInit: { signal: abort.signal } }]); + + const call = result.client.call("ping", {}, abort.signal); + while (tokenRefreshSignals.length === 0) await Promise.resolve(); + expect(tokenRefreshSignals[0]).toBe(abort.signal); + abort.abort(new Error("toolset disposed")); + await expect(call).rejects.toThrow("toolset disposed"); + expect(tokenRefreshAborts).toBe(1); + }); + test("ordinary HTTP creates endpoint-scoped OAuth and passes it to transport", async () => { const result = await connectMCPServer({ name: "exa", diff --git a/src/mcp/client.ts b/src/mcp/client.ts index 5a511931d..951aa7c3c 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -72,14 +72,25 @@ function isRecoverableAuthError(err: unknown): boolean { return err instanceof UnauthorizedError || err instanceof OAuthError; } +function streamableHTTPTransportOptions( + authProvider: CorbitsOAuthProvider | undefined, + signal: AbortSignal | undefined, +) { + if (authProvider === undefined && signal === undefined) return undefined; + return { + ...(authProvider === undefined ? {} : { authProvider }), + ...(signal === undefined ? {} : { requestInit: { signal } }), + }; +} + async function completeInteractiveAuth(context: HTTPAuthContext): Promise { if (!context.interactive) throw new Error("Authorization required but no interactive handler is available."); const code = await context.callback.waitForCode(context.signal ?? new AbortController().signal); - await new StreamableHTTPClientTransport(context.url, { - authProvider: context.authProvider, - ...(context.signal === undefined ? {} : { requestInit: { signal: context.signal } }), - }).finishAuth(code); + await new StreamableHTTPClientTransport( + context.url, + streamableHTTPTransportOptions(context.authProvider, context.signal), + ).finishAuth(code); } /** @@ -218,7 +229,11 @@ async function connectHttp( let authContext: HTTPAuthContext | undefined; let makeTransport: () => Transport; if (config.oauth === false) { - makeTransport = () => new StreamableHTTPClientTransport(url) as unknown as Transport; + makeTransport = () => + new StreamableHTTPClientTransport( + url, + streamableHTTPTransportOptions(undefined, options.signal), + ) as unknown as Transport; } else { callback = await startCallbackServer(config.name); const authProvider = await createOAuthProvider({ @@ -229,7 +244,10 @@ async function connectHttp( onAuthorizationState: callback.expectState, }); makeTransport = () => - new StreamableHTTPClientTransport(url, { authProvider }) as unknown as Transport; + new StreamableHTTPClientTransport( + url, + streamableHTTPTransportOptions(authProvider, options.signal), + ) as unknown as Transport; authContext = { url, authProvider, diff --git a/src/tui/command-surfaces.test.ts b/src/tui/command-surfaces.test.ts index 90cd8dd7d..f20a70c90 100644 --- a/src/tui/command-surfaces.test.ts +++ b/src/tui/command-surfaces.test.ts @@ -596,6 +596,37 @@ describe("mcp surface", () => { }); }); + test("hides the add row while local MCP settings shadow global", async () => { + await withShell((shell) => { + openCommandSurface(shell, "mcp", { + notify: () => {}, + mcp: { + list: () => entries, + openAuthURL: () => {}, + mcpServersSource: "local", + addServer: async () => ({ ok: true, message: "should not run" }), + }, + }); + expect(shell.overlayItems).not.toContain("Add MCP server — Alt+A"); + expect(shell.overlayItems.at(-1)).toBe("Close mcp"); + expect(runOverlayAction(shell, altKey("a"))).toBe(false); + }); + }); + + test("empty MCP list uses a placeholder distinct from close", async () => { + await withShell((shell) => { + openCommandSurface(shell, "mcp", { + notify: () => {}, + mcp: { list: () => [], openAuthURL: () => {} }, + }); + expect(shell.overlayItems).toEqual([ + "No MCP servers configured", + "Add MCP server — Alt+A", + "Close mcp", + ]); + }); + }); + test("the visible add row opens the same add-server flow", async () => { await withShell((shell) => { openCommandSurface(shell, "mcp", { diff --git a/src/tui/command-surfaces.ts b/src/tui/command-surfaces.ts index 677d75847..cda4d9caa 100644 --- a/src/tui/command-surfaces.ts +++ b/src/tui/command-surfaces.ts @@ -145,6 +145,7 @@ export interface McpSurfaceDeps { readonly openAuthURL: (url: string) => void; readonly subscribe?: (listener: () => void) => () => void; readonly addServer?: (name: string, url: string) => Promise; + readonly mcpServersSource?: "local" | "global" | "none"; } /** Live summary for the settings surface's hooks row (owned by another surface). */ @@ -185,6 +186,7 @@ export type CommandSurfaceKind = const CLOSE_ID = "__close__"; const ADD_MCP_ID = "__add_mcp__"; +const EMPTY_MCP_ID = "__empty_mcp__"; const BACK_ID = "__back__"; /** Synthetic `/plugins` row for standing load warnings (not a plugin id). */ const PLUGIN_LOAD_WARNINGS_ID = "__plugin_load_warnings__"; @@ -962,6 +964,21 @@ function mcpDescription(entry: McpEntry): ItemDescription { } } +function canAddMCPServer(mcp: McpSurfaceDeps): boolean { + return mcp.mcpServersSource !== "local"; +} + +function mcpSurfaceRows(entries: readonly McpEntry[], canAdd: boolean): ResidualCatalogEntry[] { + const rows: ResidualCatalogEntry[] = entries.map((e) => ({ + id: e.name, + label: mcpRowLabel(e), + })); + if (rows.length === 0) rows.push({ id: EMPTY_MCP_ID, label: "No MCP servers configured" }); + if (canAdd) rows.push({ id: ADD_MCP_ID, label: "Add MCP server — Alt+A" }); + rows.push({ id: CLOSE_ID, label: "Close mcp" }); + return rows; +} + function openAddMcpURLPane( shell: AppShell, deps: CommandSurfaceDeps, @@ -1045,13 +1062,8 @@ export function openMcpSurface( } closeInsetOverlay(shell); const entries = mcp.list(); - const rows: ResidualCatalogEntry[] = entries.map((e) => ({ - id: e.name, - label: mcpRowLabel(e), - })); - if (rows.length === 0) rows.push({ id: CLOSE_ID, label: "No MCP servers configured" }); - rows.push({ id: ADD_MCP_ID, label: "Add MCP server — Alt+A" }); - rows.push({ id: CLOSE_ID, label: "Close mcp" }); + const canAdd = canAddMCPServer(mcp); + const rows: ResidualCatalogEntry[] = mcpSurfaceRows(entries, canAdd); const byName = new Map(entries.map((e) => [e.name, e])); const activeIndex = activeName === undefined ? -1 : rows.findIndex((row) => row.id === activeName); @@ -1073,8 +1085,9 @@ export function openMcpSurface( onAccept: (selection) => { const id = selectedId(selection, rows); unsubscribe(); - if (id === undefined || id === CLOSE_ID) return; + if (id === undefined || id === CLOSE_ID || id === EMPTY_MCP_ID) return; if (id === ADD_MCP_ID) { + if (!canAdd) return; openAddMcpNamePane(shell, deps, mcp); return; } @@ -1094,6 +1107,7 @@ export function openMcpSurface( if (key.ctrl || !(key.meta || key.option)) return false; const name = typeof key.name === "string" ? key.name.toLowerCase() : ""; if (name !== "a") return false; + if (!canAdd) return false; unsubscribe(); openAddMcpNamePane(shell, deps, mcp); return true; @@ -1102,15 +1116,7 @@ export function openMcpSurface( unsubscribe = mcp.subscribe?.(() => { const liveEntries = mcp.list(); - const liveRows: ResidualCatalogEntry[] = liveEntries.map((entry) => ({ - id: entry.name, - label: mcpRowLabel(entry), - })); - if (liveRows.length === 0) { - liveRows.push({ id: CLOSE_ID, label: "No MCP servers configured" }); - } - liveRows.push({ id: ADD_MCP_ID, label: "Add MCP server — Alt+A" }); - liveRows.push({ id: CLOSE_ID, label: "Close mcp" }); + const liveRows = mcpSurfaceRows(liveEntries, canAdd); rows.splice(0, rows.length, ...liveRows); byName.clear(); for (const entry of liveEntries) byName.set(entry.name, entry); diff --git a/src/tui/runner.ts b/src/tui/runner.ts index f70e484fe..19e4c2b08 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -2624,6 +2624,7 @@ export async function runTUI(initialConfig: Config): Promise { emitter.on("mcp.status", listener); return () => emitter.off("mcp.status", listener); }, + mcpServersSource: config.mcpServersSource ?? "none", addServer: async (name, url) => { const result = await persistGlobalHTTPMCPServer( globalSettingsWriter, From b8d82947e7c072c05c254ccc89e0e98d4f241372 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 31 Aug 2026 20:25:31 -0700 Subject: [PATCH 3/6] Retry failed MCP connects and abort OAuth HTTP Failed connects used to stay reserved so same-session retry looked like an active duplicate. hasMCPServer now means connected or in-flight. Streamable HTTP fetch always composes the connect AbortSignal so SDK 403 auth() aborts. --- src/agent/exa-web-fetch-alias.test.ts | 42 ++++++++++++++++--- src/agent/tools.ts | 7 ++-- src/mcp/client-auth-policy.test.ts | 59 +++++++++++++++++++++++++-- src/mcp/client.ts | 23 ++++++++++- 4 files changed, 118 insertions(+), 13 deletions(-) diff --git a/src/agent/exa-web-fetch-alias.test.ts b/src/agent/exa-web-fetch-alias.test.ts index 2edc9b2e2..bebb152b5 100644 --- a/src/agent/exa-web-fetch-alias.test.ts +++ b/src/agent/exa-web-fetch-alias.test.ts @@ -5,7 +5,7 @@ import { join } from "node:path"; import type { ToolResult } from "@intx/types/runtime"; import { stringTool, type AgentTool } from "@intx/agent"; import { withMockedModule } from "../../tests/helpers/mock-module.js"; -import type { ResolvedMCPServerConfig } from "../mcp/exa.js"; +import { createExaMCPServerConfig, type ResolvedMCPServerConfig } from "../mcp/exa.js"; import type { MCPConnectOptions } from "../mcp/client.js"; import { createGlobalSettingsWriter, persistGlobalHTTPMCPServer } from "../mcp/add-server.js"; import { createPermissionGate } from "../permission/gate.js"; @@ -395,13 +395,13 @@ describe("built-in Exa web_fetch alias", () => { } }); - test("failed implicit Exa remains reserved and cannot be persisted explicitly", async () => { + test("failed implicit Exa is not active and retries without a second persist", async () => { connectMode = "failed"; const toolset = await makeToolset(); const path = join(mkdtempSync(join(tmpdir(), "corbits-mcp-failed-exa-")), "settings.json"); try { await connect(toolset); - expect(toolset.hasMCPServer("exa")).toBe(true); + expect(toolset.hasMCPServer("exa")).toBe(false); expect( await persistGlobalHTTPMCPServer( createGlobalSettingsWriter(path), @@ -410,8 +410,15 @@ describe("built-in Exa web_fetch alias", () => { "none", toolset.hasMCPServer, ), - ).toEqual({ ok: false, reason: "active" }); - expect(await Bun.file(path).exists()).toBe(false); + ).toMatchObject({ ok: true, server: { name: "exa" } }); + + connectMode = "success"; + await toolset.connectMCPServer(createExaMCPServerConfig(), { + interactiveAuth: false, + onStatus: () => undefined, + onToolsChanged: () => undefined, + }); + expect(toolset.hasMCPServer("exa")).toBe(true); } finally { await toolset.dispose(); } @@ -477,7 +484,7 @@ describe("built-in Exa web_fetch alias", () => { expect( toolset.dynamicRunner.currentDefinitions().some((tool) => tool.name.includes("linear")), ).toBe(false); - expect(toolset.hasMCPServer("linear")).toBe(true); + expect(toolset.hasMCPServer("linear")).toBe(false); connectMode = "failed"; const retryStates: string[] = []; @@ -518,6 +525,29 @@ describe("built-in Exa web_fetch alias", () => { expect(states.map((status) => status.state)).toEqual(["connecting", "failed"]); expect(states[1]?.error).toContain("connection exploded"); + expect(toolset.hasMCPServer("linear")).toBe(false); + expect(await Bun.file(path).json()).toMatchObject({ + mcpServers: [{ name: "linear", type: "http", url: "https://mcp.linear.app/mcp" }], + }); + expect( + await persistGlobalHTTPMCPServer( + createGlobalSettingsWriter(path), + "linear", + "https://mcp.linear.app/mcp", + "none", + toolset.hasMCPServer, + ), + ).toEqual({ ok: false, reason: "duplicate" }); + + connectMode = "success"; + const retryStates: string[] = []; + await toolset.connectMCPServer(persisted.server, { + interactiveAuth: true, + onStatus: (status) => retryStates.push(status.state), + onToolsChanged: () => undefined, + }); + expect(retryStates).toEqual(["connecting", "connected"]); + expect(toolset.hasMCPServer("linear")).toBe(true); expect(await Bun.file(path).json()).toMatchObject({ mcpServers: [{ name: "linear", type: "http", url: "https://mcp.linear.app/mcp" }], }); diff --git a/src/agent/tools.ts b/src/agent/tools.ts index 4a6d499a5..964830524 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -213,6 +213,9 @@ export interface AgentToolset { callbacks: MCPConnectCallbacks, signal?: AbortSignal, ) => Promise; + // True while this name is connected or a connection is in flight — not after + // a failed connect. Persist uses this to block a second add of an active name; + // failed rows retry through connectMCPServer without a second persist. hasMCPServer: (name: string) => boolean; // Wire the callback the `tool_search` tool invokes to make matched tools // advertised. Set by the runner once the director + reload loop exist. @@ -539,7 +542,6 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise server.name)); const connectedClients = new Map(); const inFlightConnections = new Map>(); const mcpAbortController = new AbortController(); @@ -552,7 +554,6 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise => { if (disposed) return Promise.resolve(); - reservedMCPServerNames.add(config.name); if (connectedClients.has(config.name)) return Promise.resolve(); const existing = inFlightConnections.get(config.name); if (existing !== undefined) return existing; @@ -703,7 +704,7 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise reservedMCPServerNames.has(name), + hasMCPServer: (name) => connectedClients.has(name) || inFlightConnections.has(name), setToolPromoter: (promote) => { promoter.promote = promote; }, diff --git a/src/mcp/client-auth-policy.test.ts b/src/mcp/client-auth-policy.test.ts index 70dbc8cab..54bc14f0b 100644 --- a/src/mcp/client-auth-policy.test.ts +++ b/src/mcp/client-auth-policy.test.ts @@ -82,7 +82,10 @@ await withMockedModule( StreamableHTTPClientTransport: class { constructor( _url: URL, - private readonly options?: { requestInit?: RequestInit }, + private readonly options?: { + requestInit?: RequestInit; + fetch?: (url: string | URL, init?: RequestInit) => Promise; + }, ) { transportOptions.push(options); lastTransportAuth = () => this.auth(); @@ -101,6 +104,9 @@ await withMockedModule( } } async auth(): Promise { + // SDK 403 upscoping uses raw `_fetch` with no init.signal. Hang on the + // connect signal the product also installs as `fetch`, so abort still + // settles this path. const signal = this.options?.requestInit?.signal; tokenRefreshSignals.push(signal); await hangUntilAbort( @@ -149,7 +155,7 @@ await withMockedModule( }), ); -const { connectMCPServer } = await import("./client.js"); +const { connectMCPServer, fetchWithConnectAbort } = await import("./client.js"); describe("HTTP MCP auth policy", () => { beforeEach(() => { @@ -257,7 +263,9 @@ describe("HTTP MCP auth policy", () => { ); expect(result.ok).toBe(true); if (!result.ok) return; - expect(transportOptions).toEqual([{ authProvider, requestInit: { signal: abort.signal } }]); + expect(transportOptions).toEqual([ + { authProvider, requestInit: { signal: abort.signal }, fetch: expect.any(Function) }, + ]); const call = result.client.call("ping", {}, abort.signal); while (tokenRefreshSignals.length === 0) await Promise.resolve(); @@ -265,6 +273,14 @@ describe("HTTP MCP auth policy", () => { abort.abort(new Error("toolset disposed")); await expect(call).rejects.toThrow("toolset disposed"); expect(tokenRefreshAborts).toBe(1); + + const fetchFn = ( + transportOptions[0] as { + fetch?: (url: string | URL, init?: RequestInit) => Promise; + } + ).fetch; + expect(fetchFn).toBeTypeOf("function"); + await expect(fetchFn!("https://auth.test/token")).rejects.toThrow(); }); test("ordinary HTTP creates endpoint-scoped OAuth and passes it to transport", async () => { @@ -281,3 +297,40 @@ describe("HTTP MCP auth policy", () => { expect(transportOptions).toEqual([{ authProvider }]); }); }); + +describe("fetchWithConnectAbort", () => { + test("rejects when the connect signal aborts OAuth HTTP with no init.signal", async () => { + const abort = new AbortController(); + let seen: AbortSignal | undefined; + const fetchFn = fetchWithConnectAbort(abort.signal, (_url, init) => { + seen = init?.signal ?? undefined; + return hangUntilAbort(init?.signal, () => undefined, "aborted").then( + () => new Response(null, { status: 200 }), + ); + }); + + const pending = fetchFn("https://auth.test/token"); + expect(seen).toBe(abort.signal); + abort.abort(new Error("toolset disposed")); + await expect(pending).rejects.toThrow("toolset disposed"); + }); + + test("composes connect abort with a caller request signal", async () => { + const connect = new AbortController(); + const request = new AbortController(); + let seen: AbortSignal | undefined; + const fetchFn = fetchWithConnectAbort(connect.signal, (_url, init) => { + seen = init?.signal ?? undefined; + return hangUntilAbort(init?.signal, () => undefined, "aborted").then( + () => new Response(null, { status: 200 }), + ); + }); + + const pending = fetchFn("https://auth.test/token", { signal: request.signal }); + expect(seen).toBeDefined(); + expect(seen).not.toBe(connect.signal); + expect(seen).not.toBe(request.signal); + connect.abort(new Error("toolset disposed")); + await expect(pending).rejects.toThrow("toolset disposed"); + }); +}); diff --git a/src/mcp/client.ts b/src/mcp/client.ts index 951aa7c3c..9da4b4e16 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -72,6 +72,25 @@ function isRecoverableAuthError(err: unknown): boolean { return err instanceof UnauthorizedError || err instanceof OAuthError; } +/** + * Fetch that always attaches the connect AbortSignal. SDK 403 upscoping calls + * `auth()` with raw `_fetch` (no `requestInit.signal`); `_fetchWithInit` still + * uses this same function, so both paths abort when connect is cancelled. + */ +export function fetchWithConnectAbort( + connectSignal: AbortSignal, + baseFetch: (url: string | URL, init?: RequestInit) => Promise = fetch, +): (url: string | URL, init?: RequestInit) => Promise { + return (url, init) => { + const requestSignal = init?.signal ?? undefined; + const signal = + requestSignal === undefined || requestSignal === connectSignal + ? connectSignal + : AbortSignal.any([connectSignal, requestSignal]); + return baseFetch(url, { ...init, signal }); + }; +} + function streamableHTTPTransportOptions( authProvider: CorbitsOAuthProvider | undefined, signal: AbortSignal | undefined, @@ -79,7 +98,9 @@ function streamableHTTPTransportOptions( if (authProvider === undefined && signal === undefined) return undefined; return { ...(authProvider === undefined ? {} : { authProvider }), - ...(signal === undefined ? {} : { requestInit: { signal } }), + ...(signal === undefined + ? {} + : { requestInit: { signal }, fetch: fetchWithConnectAbort(signal) }), }; } From 4af2d400540f069455edc88d5a118a5b87f31e11 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 31 Aug 2026 20:48:28 -0700 Subject: [PATCH 4/6] Retry failed MCP servers from the MCP list The list only stores live status. After a persist-then-connect failure the settings row already exists, so Enter reconnects that config instead of adding a duplicate name. --- src/tui/command-surfaces.test.ts | 131 +++++++++++++++++++++++++++++-- src/tui/command-surfaces.ts | 62 ++++++++++----- src/tui/runner.ts | 47 ++++++++--- 3 files changed, 206 insertions(+), 34 deletions(-) diff --git a/src/tui/command-surfaces.test.ts b/src/tui/command-surfaces.test.ts index f20a70c90..e0dadf9d7 100644 --- a/src/tui/command-surfaces.test.ts +++ b/src/tui/command-surfaces.test.ts @@ -782,13 +782,22 @@ describe("mcp surface", () => { test("Enter on an unauthorized server opens the browser and copies the link", async () => { await withShell((shell) => { const opened: string[] = []; + const retried: string[] = []; openCommandSurface(shell, "mcp", { notify: () => {}, - mcp: { list: () => entries, openAuthURL: (url) => opened.push(url) }, + mcp: { + list: () => entries, + openAuthURL: (url) => opened.push(url), + retryServer: async (name) => { + retried.push(name); + return { ok: true, message: "should not retry" }; + }, + }, }); moveOverlaySelection(shell, 1); acceptOverlaySelection(shell); expect(opened).toEqual(["https://notion.test/auth"]); + expect(retried).toEqual([]); expect(shell.statusFlash).toContain("notion"); // The echo would quote "notion — needs auth" back forever, moments // after the operator authorized it. @@ -796,23 +805,27 @@ describe("mcp surface", () => { }); }); - test("Enter on non-auth rows releases the status subscription", async () => { - const nonAuthEntries: readonly McpEntry[] = [ + test("Enter on connecting and connected rows releases the status subscription", async () => { + const nonActionEntries: readonly McpEntry[] = [ { name: "connected", state: "connected", toolCount: 1 }, { name: "connecting", state: "connecting" }, - { name: "failed", state: "failed", error: "offline" }, ]; - for (const entry of nonAuthEntries) { + for (const entry of nonActionEntries) { await withShell((shell) => { const listeners = new Set<() => void>(); let unsubscribeCalls = 0; const opened: string[] = []; + const retried: string[] = []; openCommandSurface(shell, "mcp", { notify: () => {}, mcp: { list: () => [entry], openAuthURL: (url) => opened.push(url), + retryServer: async (name) => { + retried.push(name); + return { ok: true, message: "should not retry" }; + }, subscribe: (listener) => { listeners.add(listener); return () => { @@ -826,12 +839,120 @@ describe("mcp surface", () => { expect(listeners.size).toBe(1); acceptOverlaySelection(shell); expect(opened).toEqual([]); + expect(retried).toEqual([]); expect(unsubscribeCalls).toBe(1); expect(listeners.size).toBe(0); }); } }); + test("Enter on a failed server retries connect once without adding again", async () => { + await withShell(async (shell) => { + const added: { name: string; url: string }[] = []; + const retried: string[] = []; + const opened: string[] = []; + const notes: string[] = []; + let liveEntries: readonly McpEntry[] = [ + { name: "sentry", state: "failed", error: "ECONNREFUSED" }, + ]; + openCommandSurface(shell, "mcp", { + notify: (note) => notes.push(note), + mcp: { + list: () => liveEntries, + openAuthURL: (url) => opened.push(url), + addServer: async (name, url) => { + added.push({ name, url }); + return { ok: true, message: "should not add" }; + }, + retryServer: async (name) => { + retried.push(name); + liveEntries = [{ name, state: "connecting" }]; + return { ok: true, message: `Retrying ${name}; connecting now.` }; + }, + }, + }); + acceptOverlaySelection(shell); + await Promise.resolve(); + await Promise.resolve(); + + expect(retried).toEqual(["sentry"]); + expect(added).toEqual([]); + expect(opened).toEqual([]); + expect(notes).toEqual(["Retrying sentry; connecting now."]); + expect(shell.overlayItems[0]).toBe("sentry — connecting"); + }); + }); + + test("Enter on a failed row without retry still releases the status subscription", async () => { + await withShell((shell) => { + const listeners = new Set<() => void>(); + let unsubscribeCalls = 0; + const added: { name: string; url: string }[] = []; + openCommandSurface(shell, "mcp", { + notify: () => {}, + mcp: { + list: () => [{ name: "sentry", state: "failed", error: "offline" }], + openAuthURL: () => {}, + addServer: async (name, url) => { + added.push({ name, url }); + return { ok: true, message: "should not add" }; + }, + subscribe: (listener) => { + listeners.add(listener); + return () => { + unsubscribeCalls += 1; + listeners.delete(listener); + }; + }, + }, + }); + + expect(listeners.size).toBe(1); + acceptOverlaySelection(shell); + expect(added).toEqual([]); + expect(unsubscribeCalls).toBe(1); + expect(listeners.size).toBe(0); + }); + }); + + test("Alt+A of a failed name still persists through addServer", async () => { + await withShell(async (shell) => { + const added: { name: string; url: string }[] = []; + const retried: string[] = []; + const notes: string[] = []; + openCommandSurface(shell, "mcp", { + notify: (note) => notes.push(note), + mcp: { + list: () => [{ name: "sentry", state: "failed" as const, error: "offline" }], + openAuthURL: () => {}, + addServer: async (name, url) => { + added.push({ name, url }); + return { + ok: false, + message: `An MCP server named "${name}" already exists or is connecting.`, + }; + }, + retryServer: async (name) => { + retried.push(name); + return { ok: true, message: "should not retry" }; + }, + }, + }); + + expect(runOverlayAction(shell, altKey("a"))).toBe(true); + for (const ch of "sentry") runOverlayAction(shell, charKey(ch)); + acceptOverlaySelection(shell); + for (const ch of "https://sentry.test/mcp") runOverlayAction(shell, charKey(ch)); + acceptOverlaySelection(shell); + await Promise.resolve(); + await Promise.resolve(); + + expect(added).toEqual([{ name: "sentry", url: "https://sentry.test/mcp" }]); + expect(retried).toEqual([]); + expect(notes.at(-1)).toContain("already exists"); + }); + }); + test("Alt+A collects a name and absolute HTTP URL before adding", async () => { await withShell(async (shell) => { const added: { name: string; url: string }[] = []; diff --git a/src/tui/command-surfaces.ts b/src/tui/command-surfaces.ts index cda4d9caa..bf55dbd1a 100644 --- a/src/tui/command-surfaces.ts +++ b/src/tui/command-surfaces.ts @@ -145,6 +145,8 @@ export interface McpSurfaceDeps { readonly openAuthURL: (url: string) => void; readonly subscribe?: (listener: () => void) => () => void; readonly addServer?: (name: string, url: string) => Promise; + /** Reconnect a failed persisted server without writing a second settings row. */ + readonly retryServer?: (name: string) => Promise; readonly mcpServersSource?: "local" | "global" | "none"; } @@ -960,7 +962,11 @@ function mcpDescription(entry: McpEntry): ItemDescription { impact: "Enter opens the authorization page and copies the link.", }; case "failed": - return { what: entry.error ?? "Did not connect.", tone: "consequence" }; + return { + what: entry.error ?? "Did not connect.", + impact: "Enter retries the existing persisted config without adding a second server.", + tone: "consequence", + }; } } @@ -968,6 +974,30 @@ function canAddMCPServer(mcp: McpSurfaceDeps): boolean { return mcp.mcpServersSource !== "local"; } +function runMcpSurfaceAction( + shell: AppShell, + deps: CommandSurfaceDeps, + action: Promise, + failPrefix: string, +): void { + const continuation = captureOverlayContinuation(shell); + void action + .then( + (result) => { + if (!isOverlayContinuationCurrent(shell, continuation)) return; + deps.notify(result.message); + if (isOverlayContinuationCurrent(shell, continuation)) openMcpSurface(shell, deps); + }, + (err: unknown) => { + if (!isOverlayContinuationCurrent(shell, continuation)) return; + deps.notify(`${failPrefix}: ${errorText(err)}`); + }, + ) + .catch(() => { + // UI continuation failures must not escape a fire-and-forget command. + }); +} + function mcpSurfaceRows(entries: readonly McpEntry[], canAdd: boolean): ResidualCatalogEntry[] { const rows: ResidualCatalogEntry[] = entries.map((e) => ({ id: e.name, @@ -1002,22 +1032,7 @@ function openAddMcpURLPane( deps.notify("Adding MCP servers is not available in this session."); return; } - const continuation = captureOverlayContinuation(shell); - void addServer(name, url) - .then( - (result) => { - if (!isOverlayContinuationCurrent(shell, continuation)) return; - deps.notify(result.message); - if (isOverlayContinuationCurrent(shell, continuation)) openMcpSurface(shell, deps); - }, - (err: unknown) => { - if (!isOverlayContinuationCurrent(shell, continuation)) return; - deps.notify(`Add failed: ${errorText(err)}`); - }, - ) - .catch(() => { - // UI continuation failures must not escape a fire-and-forget command. - }); + runMcpSurfaceAction(shell, deps, addServer(name, url), "Add failed"); }, }, buffer, @@ -1049,7 +1064,7 @@ function openAddMcpNamePane( ); } -/** Configured MCP servers and their live state; Enter authorizes an unauthorized one. */ +/** Configured MCP servers and their live state; Enter authorizes or retries. */ export function openMcpSurface( shell: AppShell, deps: CommandSurfaceDeps, @@ -1092,8 +1107,15 @@ export function openMcpSurface( return; } const target = byName.get(id); - const url = target?.authURL; - if (target === undefined || target.state !== "needs-auth" || url === undefined) return; + if (target === undefined) return; + if (target.state === "failed") { + const retryServer = mcp.retryServer; + if (retryServer === undefined) return; + runMcpSurfaceAction(shell, deps, retryServer(target.name), "Retry failed"); + return; + } + const url = target.authURL; + if (target.state !== "needs-auth" || url === undefined) return; mcp.openAuthURL(url); // The copy is the fallback that makes this work over SSH, where the // browser that must receive the redirect is not on this machine. diff --git a/src/tui/runner.ts b/src/tui/runner.ts index 19e4c2b08..7f31de88a 100644 --- a/src/tui/runner.ts +++ b/src/tui/runner.ts @@ -43,6 +43,7 @@ import { type Settings, type LocalSettings, type PluginConfig, + type MCPServerConfig, } from "../config/settings.js"; import { addProviderSelectorChoices, providerChoices } from "./provider-setup.js"; import { persistConnectedSelection } from "./provider-setup-submit.js"; @@ -2291,6 +2292,23 @@ export async function runTUI(initialConfig: Config): Promise { directorHolder.instance?.updateToolDefinitions(computeAdvertised(definitions)), }; + const connectLateMCPServer = (server: MCPServerConfig): void => { + void toolset + .connectMCPServer(server, mcpConnectCallbacks, mcpConnectController.signal) + .catch((err: unknown) => { + if (err instanceof Error && err.name === "AbortError") return; + tuiLogger.error("Late MCP connect failed: {error}", { + error: err instanceof Error ? err.message : String(err), + }); + }); + }; + + const persistedMCPServer = (name: string): MCPServerConfig | undefined => + (config.mcpServers ?? []).find((server) => server.name === name) ?? + (config.settings?.mcpServers ?? []).find( + (server): server is MCPServerConfig => server.name === name && !("enabled" in server), + ); + const host = await mountRunnerHost({ // An unnamed session shows nothing rather than a placeholder. title: runTaskTitle, @@ -2647,17 +2665,28 @@ export async function runTUI(initialConfig: Config): Promise { : "Enter an absolute HTTP(S) URL first."; return { ok: false, message }; } - config = { ...config, settings: result.settings }; - void toolset - .connectMCPServer(result.server, mcpConnectCallbacks, mcpConnectController.signal) - .catch((err: unknown) => { - if (err instanceof Error && err.name === "AbortError") return; - tuiLogger.error("Late MCP connect failed: {error}", { - error: err instanceof Error ? err.message : String(err), - }); - }); + config = { + ...config, + settings: result.settings, + mcpServers: [ + ...(config.mcpServers ?? []).filter((server) => server.name !== result.server.name), + result.server, + ], + }; + connectLateMCPServer(result.server); return { ok: true, message: `Added ${result.server.name}; connecting now.` }; }, + retryServer: async (name) => { + const server = persistedMCPServer(name); + if (server === undefined) { + return { + ok: false, + message: `No persisted MCP server named "${name}" to retry.`, + }; + } + connectLateMCPServer(server); + return { ok: true, message: `Retrying ${server.name}; connecting now.` }; + }, }, hooks: { list: () => From f283656b81e64b222513b5c4dde3783023255d69 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 31 Aug 2026 21:06:54 -0700 Subject: [PATCH 5/6] Fail closed when retrying untrusted local MCP servers Startup already filtered local-source servers before spawn. Retry went through late connect with no trust check, so Deny still launched the command. Apply the same filter on that path. --- src/agent/tools.ts | 59 +++++++++++-- tests/unit/tui/agent-tools.test.ts | 131 +++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+), 8 deletions(-) diff --git a/src/agent/tools.ts b/src/agent/tools.ts index 964830524..acf64420f 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -31,7 +31,11 @@ import { createExaMCPServerConfig, isBuiltinExaMCPServer } from "../mcp/exa.js"; import { mcpClientToAgentTools } from "../mcp/plugin.js"; import { createDynamicToolRunner, type DynamicToolRunner } from "../tui/dynamic-tool-runner.js"; import type { MCPServerConfig, Settings } from "../config/settings.js"; -import { filterMcpServersForConnect, type ProjectTrustStore } from "../trust/project-trust.js"; +import { + filterMcpServersForConnect, + mcpServerFingerprint, + type ProjectTrustStore, +} from "../trust/project-trust.js"; import type { ToolWatchdogConfig } from "../tui/tool-execution-watchdog.js"; import type { SessionMode } from "../config/session-mode.js"; import { sessionModeEnablesSubAgents } from "../config/session-mode.js"; @@ -547,6 +551,38 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise | undefined; + let mcpTrustStore: ProjectTrustStore = projectTrust ?? { + trustedPluginPaths: [], + trustedMcpFingerprints: [], + }; + const untrustedLocalError = `Not trusted for this project (see ${SETTINGS_DIR_NAME}/trust.json)`; + + const filterServersForConnect = async ( + servers: MCPServerConfig[], + ): Promise => { + const allowed = await filterMcpServersForConnect(servers, { + source: mcpServersSource, + store: mcpTrustStore, + cwd, + ...(requestMcpTrust !== undefined ? { requestTrust: requestMcpTrust } : {}), + }); + if (mcpServersSource !== "local") return allowed; + // Remember grants so connectOneMCPServer does not re-prompt after startup TOFU. + let fingerprints = mcpTrustStore.trustedMcpFingerprints; + let changed = false; + for (const server of allowed) { + if (isBuiltinExaMCPServer(server)) continue; + const fp = mcpServerFingerprint(server); + if (!fingerprints.includes(fp)) { + fingerprints = [...fingerprints, fp]; + changed = true; + } + } + if (changed) { + mcpTrustStore = { ...mcpTrustStore, trustedMcpFingerprints: fingerprints }; + } + return allowed; + }; const connectOneMCPServer = ( config: MCPServerConfig, @@ -563,6 +599,18 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise { + if (mcpServersSource === "local") { + const allowed = await filterServersForConnect([config]); + if (disposed) return; + if (allowed.length === 0) { + callbacks.onStatus({ + name: config.name, + state: "failed", + error: untrustedLocalError, + }); + return; + } + } callbacks.onStatus({ name: config.name, state: "connecting" }); let result: MCPConnectResult; try { @@ -657,12 +705,7 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise => { if (disposed) return; - const toConnect = await filterMcpServersForConnect(mcpServers, { - source: mcpServersSource, - store: projectTrust ?? { trustedPluginPaths: [], trustedMcpFingerprints: [] }, - cwd, - ...(requestMcpTrust !== undefined ? { requestTrust: requestMcpTrust } : {}), - }); + const toConnect = await filterServersForConnect(mcpServers); if (disposed) return; await Promise.all(toConnect.map((config) => connectOneMCPServer(config, callbacks, signal))); if (disposed) return; @@ -674,7 +717,7 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise {}); @@ -362,6 +363,136 @@ test("headless MCP connection does not wait for interactive OAuth", async () => expect(mockConnectMCPServer.mock.calls[0]?.[1]?.onAuthURL).toBeUndefined(); }); +const localStdioServer = { name: "evil", command: "evil-bin" }; +const globalHttpServer = { + name: "linear", + type: "http" as const, + url: "https://mcp.example.test/mcp", +}; + +test("late connect of an untrusted local-source server does not spawn", async () => { + mockConnectMCPServer.mockClear(); + const statuses: { name: string; state: string; error?: string }[] = []; + const toolset = await createAgentToolset({ + cwd: "/fake", + permissionGate: fakePermissionGate, + onOperatorGate: async () => ({ kind: "cancel" }), + mcpServers: [localStdioServer], + mcpServersSource: "local", + projectTrust: { trustedPluginPaths: [], trustedMcpFingerprints: [] }, + }); + + await toolset.connectMCPServer(localStdioServer, { + interactiveAuth: false, + onStatus: (status) => statuses.push(status), + onToolsChanged: () => {}, + }); + + expect(mockConnectMCPServer).not.toHaveBeenCalled(); + expect(statuses).toHaveLength(1); + expect(statuses[0]?.name).toBe("evil"); + expect(statuses[0]?.state).toBe("failed"); + expect(statuses[0]?.error).toMatch(/Not trusted for this project/); + await toolset.dispose(); +}); + +test("late connect of an untrusted local-source server fail-closes when requestMcpTrust denies", async () => { + mockConnectMCPServer.mockClear(); + let trustAsks = 0; + const toolset = await createAgentToolset({ + cwd: "/fake", + permissionGate: fakePermissionGate, + onOperatorGate: async () => ({ kind: "cancel" }), + mcpServers: [localStdioServer], + mcpServersSource: "local", + projectTrust: { trustedPluginPaths: [], trustedMcpFingerprints: [] }, + requestMcpTrust: async () => { + trustAsks += 1; + return false; + }, + }); + + await toolset.connectMCPServer(localStdioServer, { + interactiveAuth: false, + onStatus: () => {}, + onToolsChanged: () => {}, + }); + + expect(trustAsks).toBe(1); + expect(mockConnectMCPServer).not.toHaveBeenCalled(); + await toolset.dispose(); +}); + +test("late connect of a trusted local-source server still connects", async () => { + mockConnectMCPServer.mockClear(); + const toolset = await createAgentToolset({ + cwd: "/fake", + permissionGate: fakePermissionGate, + onOperatorGate: async () => ({ kind: "cancel" }), + mcpServers: [localStdioServer], + mcpServersSource: "local", + projectTrust: { + trustedPluginPaths: [], + trustedMcpFingerprints: [mcpServerFingerprint(localStdioServer)], + }, + }); + + await toolset.connectMCPServer(localStdioServer, { + interactiveAuth: false, + onStatus: () => {}, + onToolsChanged: () => {}, + }); + + expect(mockConnectMCPServer).toHaveBeenCalledTimes(1); + expect(mockConnectMCPServer.mock.calls[0]?.[0]).toEqual(localStdioServer); + await toolset.dispose(); +}); + +test("late connect of a global-source HTTP server does not require trust", async () => { + mockConnectMCPServer.mockClear(); + const toolset = await createAgentToolset({ + cwd: "/fake", + permissionGate: fakePermissionGate, + onOperatorGate: async () => ({ kind: "cancel" }), + mcpServers: [globalHttpServer], + mcpServersSource: "global", + projectTrust: { trustedPluginPaths: [], trustedMcpFingerprints: [] }, + }); + + await toolset.connectMCPServer(globalHttpServer, { + interactiveAuth: false, + onStatus: () => {}, + onToolsChanged: () => {}, + }); + + expect(mockConnectMCPServer).toHaveBeenCalledTimes(1); + expect(mockConnectMCPServer.mock.calls[0]?.[0]).toEqual(globalHttpServer); + await toolset.dispose(); +}); + +test("startup connectMCP still fail-closes untrusted local servers", async () => { + mockConnectMCPServer.mockClear(); + const statuses: { name: string; state: string; error?: string }[] = []; + const toolset = await createAgentToolset({ + cwd: "/fake", + permissionGate: fakePermissionGate, + onOperatorGate: async () => ({ kind: "cancel" }), + mcpServers: [localStdioServer], + mcpServersSource: "local", + projectTrust: { trustedPluginPaths: [], trustedMcpFingerprints: [] }, + }); + + await toolset.connectMCP({ + interactiveAuth: false, + onStatus: (status) => statuses.push(status), + onToolsChanged: () => {}, + }); + + expect(mockConnectMCPServer).not.toHaveBeenCalled(); + expect(statuses.some((s) => s.name === "evil" && s.state === "failed")).toBe(true); + await toolset.dispose(); +}); + test("dispose calls posixTools.dispose", async () => { mockDispose.mockClear(); From e3c9f0d77f9a711b989a5c755233ee43fcd01d26 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 31 Aug 2026 21:35:29 -0700 Subject: [PATCH 6/6] Pass deliver into host tests after steer routing landed --- src/tui/runner-host.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/tui/runner-host.test.ts b/src/tui/runner-host.test.ts index aad677ce0..d5fdf9814 100644 --- a/src/tui/runner-host.test.ts +++ b/src/tui/runner-host.test.ts @@ -145,6 +145,7 @@ describe("mountRunnerHost chrome wiring", () => { eventEmitter: new EventEmitter(), send: () => {}, interrupt: () => {}, + deliver: () => {}, providers: {}, onModelSelect: () => {}, commands: () => commands, @@ -431,6 +432,7 @@ describe("mountRunnerHost model picker", () => { eventEmitter: new EventEmitter(), send: () => {}, interrupt: () => {}, + deliver: () => {}, providers: { xai: { models: ["grok-4"] } }, onModelSelect: () => {}, onConnectProvider: () => {}, @@ -462,6 +464,7 @@ describe("mountRunnerHost model picker", () => { eventEmitter: new EventEmitter(), send: () => {}, interrupt: () => {}, + deliver: () => {}, providers: { xai: { models: ["grok-4"] } }, onModelSelect: () => {}, commands: [],