diff --git a/packages/engine/src/__tests__/executor-project-mcp-resume.test.ts b/packages/engine/src/__tests__/executor-project-mcp-resume.test.ts new file mode 100644 index 0000000000..8b06f60f2a --- /dev/null +++ b/packages/engine/src/__tests__/executor-project-mcp-resume.test.ts @@ -0,0 +1,92 @@ +import "./executor-test-helpers.js"; +import { describe, expect, it, vi } from "vitest"; +import type { Transport } from "@modelcontextprotocol/sdk/shared/transport.js"; +import { TaskExecutor } from "../executor.js"; +import { connectMcpSessionTools, type McpSessionClient } from "../mcp-session-tools.js"; + +function createStore(options: { secretFailure?: boolean } = {}) { + const revealSecret = vi.fn(async () => { + if (options.secretFailure) throw new Error("credential-bearing detail must not escape"); + return { key: "postiz-token", plaintextValue: "in-memory-only" }; + }); + return { + on: vi.fn(), + off: vi.fn(), + getSettings: vi.fn(async () => ({ globalPause: false, enginePaused: false })), + listTasks: vi.fn(async () => []), + getSettingsByScope: vi.fn(async () => ({ + global: { mcpServers: { enabled: true, servers: [] } }, + project: { + mcpServers: { + enabled: true, + servers: [{ + name: "postiz", + transport: "streamable-http", + url: "https://redacted.invalid/mcp", + headers: { Authorization: { secretRef: "postiz-token", scope: "project" } }, + }], + }, + }, + })), + getSecretsStore: vi.fn(async () => ({ revealSecret })), + } as any; +} + +function fakeClient(close: ReturnType): McpSessionClient { + return { + connect: vi.fn(async () => undefined), + listTools: vi.fn(async () => ({ tools: [{ name: "integrationlist", inputSchema: { type: "object" } }] })), + callTool: vi.fn(async () => ({ content: [{ type: "text", text: "[]" }] })), + close, + }; +} + +const transportFactory = () => ({}) as Transport; + +describe("executor project MCP bootstrap and approval resume invariant", () => { + it("re-resolves one effective project server for every independent executor identity and fresh toolset", async () => { + const store = createStore(); + const executor = new TaskExecutor(store, "/tmp/project"); + const identities = [undefined, "agent-main-004", "agent-main-005"]; + const closes: Array> = []; + + for (const agentId of identities) { + const servers = await (executor as any).resolveMcpServers(agentId); + const close = vi.fn(async () => undefined); + closes.push(close); + const toolset = await connectMcpSessionTools(servers, { + clientFactory: () => fakeClient(close), + transportFactory, + }); + expect(toolset.tools.map((tool) => tool.name)).toEqual(["mcp__postiz__integrationlist"]); + await toolset.dispose(); + } + + expect(store.getSettingsByScope).toHaveBeenCalledTimes(3); + expect(closes.every((close) => close.mock.calls.length === 1)).toBe(true); + }); + + it("fails executor bootstrap with a sanitized outcome when secret materialization fails", async () => { + const executor = new TaskExecutor(createStore({ secretFailure: true }), "/tmp/project"); + + await expect((executor as any).resolveMcpServers("agent-main-007")).rejects.toThrow( + /^MCP resolution failed: server=postiz reason=secret-materialization$/, + ); + }); + + it("closes a client whose connection fails before registration", async () => { + const close = vi.fn(async () => undefined); + const client = fakeClient(close); + client.connect = vi.fn(async () => { + throw new TypeError("credential-bearing detail must not escape"); + }); + + const toolset = await connectMcpSessionTools( + [{ name: "postiz", transport: "stdio", command: "fake" }], + { clientFactory: () => client, transportFactory }, + ); + + expect(toolset.skipped).toEqual([{ name: "postiz", reason: "TypeError" }]); + expect(close).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/engine/src/__tests__/mcp-resolution.test.ts b/packages/engine/src/__tests__/mcp-resolution.test.ts index 0b72177261..6165e0d513 100644 --- a/packages/engine/src/__tests__/mcp-resolution.test.ts +++ b/packages/engine/src/__tests__/mcp-resolution.test.ts @@ -83,6 +83,27 @@ describe("resolveMcpServersForRuntime", () => { }); }); + it("treats a missing settings seam as a genuine empty configuration", async () => { + const { resolveMcpServersForStore } = await import("../mcp-resolution.js"); + await expect(resolveMcpServersForStore({})).resolves.toEqual({ servers: [], errors: [] }); + }); + + it("honors an explicitly disabled project scope and disabled project shadow", async () => { + const disabledScope = await resolveMcpServersForRuntime({ + globalSettings: { mcpServers: { enabled: true, servers: [{ name: "global", transport: "stdio", command: "node" }] } }, + projectSettings: { mcpServers: { enabled: false, servers: [] } }, + secrets: secrets({}), + }); + const disabledShadow = await resolveMcpServersForRuntime({ + globalSettings: { mcpServers: { enabled: true, servers: [{ name: "global", transport: "stdio", command: "node" }] } }, + projectSettings: { mcpServers: { enabled: true, servers: [{ name: "global", enabled: false, transport: "stdio", command: "noop" }] } }, + secrets: secrets({}), + }); + + expect(disabledScope).toEqual({ servers: [], errors: [] }); + expect(disabledShadow).toEqual({ servers: [], errors: [] }); + }); + it("returns materialization errors without leaking through logs", async () => { const result = await resolveMcpServersForRuntime({ globalSettings: { diff --git a/packages/engine/src/__tests__/pi-create-fn-agent.test.ts b/packages/engine/src/__tests__/pi-create-fn-agent.test.ts index 3b8dd8b12f..964a92d617 100644 --- a/packages/engine/src/__tests__/pi-create-fn-agent.test.ts +++ b/packages/engine/src/__tests__/pi-create-fn-agent.test.ts @@ -2289,6 +2289,38 @@ describe("createFnAgent", () => { expect(close).toHaveBeenCalledTimes(1); }); + it.each([ + ["connect", "TypeError"], + ["list", "RangeError"], + ] as const)("fails configured MCP session bootstrap explicitly on %s failure and closes once", async (phase, reason) => { + const close = vi.fn(async () => undefined); + const mcpClient = { + connect: vi.fn(async () => { + if (phase === "connect") throw new TypeError("sensitive connection detail"); + }), + listTools: vi.fn(async () => { + if (phase === "list") throw new RangeError("sensitive listing detail"); + return { tools: [] }; + }), + callTool: vi.fn(), + close, + }; + const { createFnAgent } = await import("../pi.js"); + + await expect(createFnAgent({ + cwd: "/test/project", + systemPrompt: "test", + tools: "coding", + defaultProvider: "anthropic", + defaultModelId: "claude-sonnet-4-5", + mcpServers: [{ name: "postiz", transport: "stdio", command: "redacted", enabled: true }], + mcpClientFactory: () => mcpClient as any, + })).rejects.toThrow(`MCP session bootstrap failed: server=postiz reason=${reason}`); + + expect(close).toHaveBeenCalledTimes(1); + expect(createAgentSessionMock).not.toHaveBeenCalled(); + }); + it("keeps MCP tools out of readonly sessions without the explicit opt-in", async () => { const { createFnAgent } = await import("../pi.js"); const mcpClient = { diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 34662a75f2..83b12bef32 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -94,7 +94,7 @@ import { } from "./agent-session-helpers.js"; import { buildSessionSkillContext } from "./session-skill-context.js"; import type { SkillSelectionContext } from "./skill-resolver.js"; -import { resolveMcpServersForStore } from "./mcp-resolution.js"; +import { assertMcpResolutionSucceeded, resolveMcpServersForStore } from "./mcp-resolution.js"; import { reviewStep, proseSignalsClearApproval, extractJsonObjectCandidates, type ReviewVerdict, type ReviewResult } from "./reviewer.js"; import { buildUserCommentsPromptSection, selectUserCommentsForAgentContext } from "./agent-user-comments.js"; import { resolveSandboxBackend } from "./sandbox/index.js"; @@ -2813,9 +2813,23 @@ export class TaskExecutor { * Paused tasks are moved back to `todo` rather than marked as `failed`. */ private async resolveMcpServers(agentId?: string | null) { - // FNXC:McpConfig 2026-06-25-22:20: - // Executor-owned lanes (main execution, retry, workflow model nodes, self-fix, and spawned child sessions) resolve the same trusted MCP server set from the task store immediately before session creation so secret material is never persisted in task state. - return (await resolveMcpServersForStore(this.store, { agentId: agentId ?? undefined })).servers; + /* + * FNXC:McpConfig 2026-06-25-22:20: + * Executor-owned lanes (main execution, retry, workflow model nodes, self-fix, and spawned child sessions) resolve the same trusted MCP server set from the task store immediately before session creation so secret material is never persisted in task state. + * + * FNXC:McpConfig 2026-07-12-17:02: + * MAIN-008 forbids executor paths from silently consuming a partially + * materialized server set. Convert secret-resolution errors into a + * content-free bootstrap failure before any runtime can connect with + * missing credentials; only server names/counts and a coarse category may + * cross this seam. + */ + const resolved = await resolveMcpServersForStore(this.store, { agentId: agentId ?? undefined }); + if (resolved.errors.length > 0) { + const serverNames = [...new Set(resolved.errors.map((error) => error.serverName))].sort(); + executorLog.warn(`MCP executor resolution failed: servers=${serverNames.join(",")} count=${serverNames.length} reason=secret-materialization`); + } + return assertMcpResolutionSucceeded(resolved); } constructor( diff --git a/packages/engine/src/mcp-resolution.ts b/packages/engine/src/mcp-resolution.ts index 0edb127dfd..14f105b06a 100644 --- a/packages/engine/src/mcp-resolution.ts +++ b/packages/engine/src/mcp-resolution.ts @@ -21,6 +21,32 @@ export interface ResolvedMcpServersForRuntime { errors: McpSecretResolutionError[]; } +/** + * A content-free executor bootstrap failure. The message deliberately contains + * only server names and a coarse category; raw secret-store errors can contain + * credential-bearing configuration details. + */ +export class McpResolutionBootstrapError extends Error { + readonly serverNames: string[]; + readonly reason = "secret-materialization" as const; + + constructor(errors: McpSecretResolutionError[]) { + const serverNames = [...new Set(errors.map((error) => error.serverName))].sort(); + super(`MCP resolution failed: server=${serverNames.join(",")} reason=secret-materialization`); + this.name = "McpResolutionBootstrapError"; + this.serverNames = serverNames; + } +} + +export function assertMcpResolutionSucceeded( + result: ResolvedMcpServersForRuntime, +): ResolvedMcpServerDefinition[] { + if (result.errors.length > 0) { + throw new McpResolutionBootstrapError(result.errors); + } + return result.servers; +} + /** * FNXC:McpConfig 2026-06-25-21:43: * Runtime MCP forwarding uses Fusion's trusted-once-enabled model: enabled effective servers are materialized once at session/probe creation and then forwarded without per-call prompts. Plaintext env/header values exist only in this in-memory return value and callers must log only counts/errors, never server contents. diff --git a/packages/engine/src/mcp-session-tools.ts b/packages/engine/src/mcp-session-tools.ts index 8a0c6d761c..85591e69ea 100644 --- a/packages/engine/src/mcp-session-tools.ts +++ b/packages/engine/src/mcp-session-tools.ts @@ -15,6 +15,19 @@ export interface McpSessionToolset { skipped: Array<{ name: string; reason: string }>; } +export class McpSessionBootstrapError extends Error { + readonly failures: Array<{ name: string; reason: string }>; + + constructor(failures: Array<{ name: string; reason: string }>) { + const sanitized = failures + .map(({ name, reason }) => ({ name, reason })) + .sort((a, b) => a.name.localeCompare(b.name) || a.reason.localeCompare(b.reason)); + super(`MCP session bootstrap failed: ${sanitized.map(({ name, reason }) => `server=${name} reason=${reason}`).join("; ")}`); + this.name = "McpSessionBootstrapError"; + this.failures = sanitized; + } +} + export interface McpSessionClient { connect(transport: Transport): Promise; listTools(): Promise<{ tools?: McpToolMetadata[] }>; @@ -68,13 +81,20 @@ export async function connectMcpSessionTools( const connected: string[] = []; const skipped: Array<{ name: string; reason: string }> = []; const clients: McpSessionClient[] = []; + const closedClients = new WeakSet(); const usedToolNames = new Set(); let disposed = false; + const closeOnce = async (client: McpSessionClient): Promise => { + if (closedClients.has(client)) return; + closedClients.add(client); + await closeClient(client, opts.closeTimeoutMs ?? DEFAULT_CLOSE_TIMEOUT_MS); + }; + const closeAll = async (): Promise => { if (disposed) return; disposed = true; - await Promise.allSettled(clients.map((client) => closeClient(client, opts.closeTimeoutMs ?? DEFAULT_CLOSE_TIMEOUT_MS))); + await Promise.allSettled(clients.map(closeOnce)); }; const onAbort = (): void => { @@ -93,13 +113,19 @@ export async function connectMcpSessionTools( break; } const client = (opts.clientFactory ?? defaultClientFactory)(server); - let didConnect = false; + // Track the client before transport creation/connect so abort and every + // partial-bootstrap failure can close it exactly once. + clients.push(client); try { const transport = (opts.transportFactory ?? defaultTransportFactory)(server, { cwd: opts.cwd }); await client.connect(transport); - didConnect = true; - clients.push(client); + if (opts.signal?.aborted || disposed) { + throw new DOMException("MCP bootstrap aborted", "AbortError"); + } const listed = await client.listTools(); + if (opts.signal?.aborted || disposed) { + throw new DOMException("MCP bootstrap aborted", "AbortError"); + } connected.push(server.name); const listedTools = listed.tools ?? []; opts.logger?.log?.(`MCP server connected for pi session: name=${server.name} transport=${server.transport} tools=${listedTools.length}`); @@ -107,12 +133,10 @@ export async function connectMcpSessionTools( tools.push(wrapMcpTool(server.name, tool, client, usedToolNames)); } } catch (error) { - const reason = safeErrorReason(error); + const reason = opts.signal?.aborted ? "aborted" : safeErrorReason(error); skipped.push({ name: server.name, reason }); opts.logger?.warn?.(`Skipping MCP server for pi session: name=${server.name} transport=${server.transport} reason=${reason}`); - if (didConnect) { - await closeClient(client, opts.closeTimeoutMs ?? DEFAULT_CLOSE_TIMEOUT_MS); - } + await closeOnce(client); } } } finally { diff --git a/packages/engine/src/pi.ts b/packages/engine/src/pi.ts index 629fd81eac..7ae4dabe2e 100644 --- a/packages/engine/src/pi.ts +++ b/packages/engine/src/pi.ts @@ -79,7 +79,7 @@ import { READONLY_ALLOWLIST, filterCustomToolsForReadonly, isReadonlyAllowed } f import { createStreamingDeltaNormalizer } from "./streaming-delta.js"; import { isModelAuthTierIncompatibilityError, isProviderModelNotFoundError, isUnsupportedMessageRoleError } from "./transient-error-detector.js"; import { logMcpForwardingSkipped, runtimeSupportsMcp } from "./mcp-runtime-support.js"; -import { connectMcpSessionTools, type McpClientFactory, type McpSessionToolset } from "./mcp-session-tools.js"; +import { connectMcpSessionTools, McpSessionBootstrapError, type McpClientFactory, type McpSessionToolset } from "./mcp-session-tools.js"; export { isModelAuthTierIncompatibilityError } from "./transient-error-detector.js"; const RTK_ACCEPTED_REWRITE_EXIT_CODES = new Set([0, 3]); @@ -2327,6 +2327,18 @@ export async function createFnAgent(options: AgentOptions): Promise clientFactory: options.mcpClientFactory, logger: piLog, }); + /* + * FNXC:McpConfig 2026-07-12-17:02: + * MAIN-008 requires a configured MCP bootstrap failure to be observably + * different from a genuine zero-server/tool catalog. Fail session creation + * using names plus coarse categories only, and dispose every partially + * connected client before the error crosses the runtime boundary. + */ + const bootstrapFailures = mcpToolset.skipped.filter(({ reason }) => reason !== "disabled"); + if (bootstrapFailures.length > 0) { + await mcpToolset.dispose(); + throw new McpSessionBootstrapError(bootstrapFailures); + } } else if (forwardedMcpServers.length > 0 && isReadonly) { piLog.log(`readonly session — MCP servers (${forwardedMcpServers.length}) skipped`); }