fix(MAIN-008): complete Step 2 — stabilize MCP executor bootstrap
Agent: engineer Fusion-Task-Id: MAIN-008 Co-authored-by: Fusion <noreply@runfusion.ai>
This commit is contained in:
@@ -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<typeof vi.fn>): 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<ReturnType<typeof vi.fn>> = [];
|
||||
|
||||
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);
|
||||
});
|
||||
});
|
||||
@@ -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: {
|
||||
|
||||
@@ -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 = {
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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<void>;
|
||||
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<McpSessionClient>();
|
||||
const usedToolNames = new Set<string>();
|
||||
let disposed = false;
|
||||
|
||||
const closeOnce = async (client: McpSessionClient): Promise<void> => {
|
||||
if (closedClients.has(client)) return;
|
||||
closedClients.add(client);
|
||||
await closeClient(client, opts.closeTimeoutMs ?? DEFAULT_CLOSE_TIMEOUT_MS);
|
||||
};
|
||||
|
||||
const closeAll = async (): Promise<void> => {
|
||||
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 {
|
||||
|
||||
@@ -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<AgentResult>
|
||||
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`);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user