diff --git a/docs/MCP.md b/docs/MCP.md index 83a77080..96399a8b 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 0b944688..bebb152b 100644 --- a/src/agent/exa-web-fetch-alias.test.ts +++ b/src/agent/exa-web-fetch-alias.test.ts @@ -5,19 +5,53 @@ 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"; 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,311 @@ 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 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(false); + expect( + await persistGlobalHTTPMCPServer( + createGlobalSettingsWriter(path), + "exa", + "https://custom.test/mcp", + "none", + toolset.hasMCPServer, + ), + ).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(); + } + }); + + 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(false); + + 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(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" }], + }); + } 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 dcb3bc03..acf64420 100644 --- a/src/agent/tools.ts +++ b/src/agent/tools.ts @@ -22,12 +22,20 @@ 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"; 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"; @@ -203,6 +211,16 @@ 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; + // 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. setToolPromoter: (promote: (names: string[]) => void) => void; @@ -528,34 +546,89 @@ export async function createAgentToolset(args: AgentToolsetArgs): Promise(); + const inFlightConnections = new Map>(); + const mcpAbortController = new AbortController(); + let disposed = false; + let disposal: Promise | undefined; + let mcpTrustStore: ProjectTrustStore = projectTrust ?? { + trustedPluginPaths: [], + trustedMcpFingerprints: [], + }; + const untrustedLocalError = `Not trusted for this project (see ${SETTINGS_DIR_NAME}/trust.json)`; - const connectMCP = async ( - callbacks: MCPConnectCallbacks, - signal?: AbortSignal, - ): Promise => { - const toConnect = await filterMcpServersForConnect(mcpServers, { + const filterServersForConnect = async ( + servers: MCPServerConfig[], + ): Promise => { + const allowed = await filterMcpServersForConnect(servers, { source: mcpServersSource, - store: projectTrust ?? { trustedPluginPaths: [], trustedMcpFingerprints: [] }, + store: mcpTrustStore, cwd, ...(requestMcpTrust !== undefined ? { requestTrust: requestMcpTrust } : {}), }); - await Promise.all( - toConnect.map(async (config) => { - callbacks.onStatus({ name: config.name, state: "connecting" }); - const connection = connectMCPServer(config, { + 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, + callbacks: MCPConnectCallbacks, + signal?: AbortSignal, + ): Promise => { + if (disposed) return Promise.resolve(); + 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 () => { + 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 { + 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 +636,79 @@ 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 filterServersForConnect(mcpServers); + 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)); @@ -598,26 +717,40 @@ 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) => connectedClients.has(name) || inFlightConnections.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 00000000..14722c91 --- /dev/null +++ b/src/mcp/add-server.test.ts @@ -0,0 +1,194 @@ +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, + isAbsoluteHTTPURL, + 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, "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", + }); + 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("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 })); + 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 00000000..24b3f5b6 --- /dev/null +++ b/src/mcp/add-server.ts @@ -0,0 +1,144 @@ +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 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(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 null; + } +} + +export function isAbsoluteHTTPURL(value: string): boolean { + return parseAbsoluteHTTPURL(value) !== null; +} + +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 parsedURL = parseAbsoluteHTTPURL(rawURL); + if (validateMCPServerName(name) !== null) return { ok: false, reason: "invalid-name" }; + 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: parsedURL.href }; + 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 f171ab9c..54bc14f0 100644 --- a/src/mcp/client-auth-policy.test.ts +++ b/src/mcp/client-auth-policy.test.ts @@ -1,11 +1,39 @@ 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; +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 }; @@ -17,10 +45,32 @@ 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 callTool(): Promise<{ content: [] }> { + if (lastTransportAuth === undefined) throw new Error("no live HTTP transport"); + await lastTransportAuth(); + return { content: [] }; + } + async close(): Promise { + clientCloses += 1; + } }, }), ); @@ -30,8 +80,42 @@ 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; + fetch?: (url: string | URL, init?: RequestInit) => Promise; + }, + ) { transportOptions.push(options); + lastTransportAuth = () => this.auth(); + } + async finishAuth(): Promise { + const signal = this.options?.requestInit?.signal; + tokenExchangeSignals.push(signal); + if (blockTokenExchange) { + await hangUntilAbort( + signal, + () => { + tokenExchangeAborts += 1; + }, + "token exchange aborted", + ); + } + } + 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( + signal, + () => { + tokenRefreshAborts += 1; + }, + "token refresh aborted", + ); } get sessionId(): string | undefined { return undefined; @@ -50,7 +134,9 @@ await withMockedModule( redirectUrl: "http://127.0.0.1:12345/callback", expectState: () => undefined, waitForCode: async () => "code", - close: () => undefined, + close: () => { + callbackCloses += 1; + }, }; }, }), @@ -63,20 +149,33 @@ await withMockedModule( createOAuthProvider: async (options: { serverURL: string }) => { providerCreates += 1; providerServerURL = options.serverURL; + if (providerCreateError !== undefined) throw providerCreateError; return authProvider; }, }), ); -const { connectMCPServer } = await import("./client.js"); +const { connectMCPServer, fetchWithConnectAbort } = 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; + lastTransportAuth = undefined; + tokenRefreshSignals = []; + tokenRefreshAborts = 0; }); test("built-in anonymous Exa treats 401 as a normal failure without OAuth machinery", async () => { @@ -94,6 +193,96 @@ 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("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 }, fetch: expect.any(Function) }, + ]); + + 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); + + 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 () => { const result = await connectMCPServer({ name: "exa", @@ -108,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 f6e92a93..9da4b4e1 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -72,13 +72,46 @@ 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, +) { + if (authProvider === undefined && signal === undefined) return undefined; + return { + ...(authProvider === undefined ? {} : { authProvider }), + ...(signal === undefined + ? {} + : { requestInit: { signal }, fetch: fetchWithConnectAbort(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, - }).finishAuth(code); + await new StreamableHTTPClientTransport( + context.url, + streamableHTTPTransportOptions(context.authProvider, context.signal), + ).finishAuth(code); } /** @@ -140,8 +173,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 +221,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 +242,62 @@ 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, + streamableHTTPTransportOptions(undefined, options.signal), + ) 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, + streamableHTTPTransportOptions(authProvider, options.signal), + ) 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 ee7c163a..898df491 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 dfd1093b..e0dadf9d 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,19 +592,212 @@ describe("mcp surface", () => { "notion — needs auth", "sentry — failed", ]); + expect(shell.overlayItems).toContain("Add MCP server — Alt+A"); + }); + }); + + 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", { + 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); }); }); 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. @@ -588,15 +805,495 @@ describe("mcp surface", () => { }); }); - test("Enter on a connected server does nothing", async () => { - await withShell((shell) => { + 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" }, + ]; + + 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 () => { + unsubscribeCalls += 1; + listeners.delete(listener); + }; + }, + }, + }); + + 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: () => {}, - mcp: { list: () => entries, openAuthURL: (url) => opened.push(url) }, + 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 }[] = []; + let liveEntries: readonly McpEntry[] = entries; + openCommandSurface(shell, "mcp", { + notify: () => {}, + 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); + 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 40b66085..bf55dbd1 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,11 @@ 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; + /** Reconnect a failed persisted server without writing a second settings row. */ + readonly retryServer?: (name: string) => Promise; + readonly mcpServersSource?: "local" | "global" | "none"; } /** Live summary for the settings surface's hooks row (owned by another surface). */ @@ -178,6 +187,8 @@ export type CommandSurfaceKind = "help" | "settings" | "permissions" | "plugins" | "hooks" | "mcp" | "models" | "add-provider"; 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__"; @@ -680,6 +691,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") { @@ -947,12 +962,114 @@ 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", + }; } } -/** Configured MCP servers and their live state; Enter authorizes an unauthorized one. */ -export function openMcpSurface(shell: AppShell, deps: CommandSurfaceDeps): void { +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, + 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, + 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; + } + runMcpSurfaceAction(shell, deps, addServer(name, url), "Add failed"); + }, + }, + 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 or retries. */ +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 +1077,15 @@ 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" }); - } - 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); + 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,12 +1096,26 @@ 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); - if (id === undefined || id === CLOSE_ID) return; + unsubscribe(); + if (id === undefined || id === CLOSE_ID || id === EMPTY_MCP_ID) return; + if (id === ADD_MCP_ID) { + if (!canAdd) return; + openAddMcpNamePane(shell, deps, mcp); + 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. @@ -993,7 +1125,34 @@ 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; + if (!canAdd) return false; + unsubscribe(); + openAddMcpNamePane(shell, deps, mcp); + return true; + }, }); + unsubscribe = + mcp.subscribe?.(() => { + const liveEntries = mcp.list(); + const liveRows = mcpSurfaceRows(liveEntries, canAdd); + 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 896d27a3..b5663c56 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 d9bab743..df16e631 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 7d847d57..5bc1e970 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 37e1e493..22d9dc8e 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 080a73e4..139e9ce1 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 7e624d56..9f43e35f 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-host.test.ts b/src/tui/runner-host.test.ts index aad677ce..d5fdf981 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: [], diff --git a/src/tui/runner.ts b/src/tui/runner.ts index 0ebc9cb2..7f31de88 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, @@ -45,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"; @@ -53,6 +52,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 +176,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 +268,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 +804,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 +844,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 +991,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 +1625,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 +2000,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 +2016,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 +2036,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 +2274,40 @@ 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 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. @@ -2335,6 +2376,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 +2470,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 +2491,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 +2512,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 +2638,55 @@ 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); + }, + mcpServersSource: config.mcpServersSource ?? "none", + 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, + 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: () => @@ -2778,30 +2883,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 e5605d0a..95c424ef 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 8acb5383..82e98975 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); diff --git a/tests/unit/tui/agent-tools.test.ts b/tests/unit/tui/agent-tools.test.ts index 781624ac..520eba53 100644 --- a/tests/unit/tui/agent-tools.test.ts +++ b/tests/unit/tui/agent-tools.test.ts @@ -3,6 +3,7 @@ import type { ToolDefinition, ToolCall } from "@intx/types/runtime"; import { TOOL_NAMES } from "@intx/tools-posix"; import { createPermissionGate } from "../../../src/permission/gate.js"; import type { PermissionGate } from "../../../src/permission/gate.js"; +import { mcpServerFingerprint } from "../../../src/trust/project-trust.js"; import { withMockedModule } from "../../helpers/mock-module.js"; const mockDispose = mock(async () => {}); @@ -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();