mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-21 00:05:49 +02:00
fix(mcp): keep sibling tools available after transient calls (#16890)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents use remote MCP tools through a governed gateway. > - The gateway lists tools only while their shared connection is healthy. > - One failed call currently marks the whole connection as unhealthy. > - Later calls to healthy sibling tools then fail with `tool_not_found`. > - This pull request keeps non-authentication call failures on their invocation. > - Agents can continue their work while credential and access gates remain in force. ## Linked Issues or Issue Description Refs #10147. Related work: #15462 fixed oversized and malformed response handling. #15448 covers timeout and JSON-RPC health handling together with separate SSE and catalog discovery changes. Its patch still marks fetch failures and HTTP failures as connection errors. #15325 proposed a transport failure counter; it has merge conflicts and has not changed since October 6. #11910 is an older timeout fix with merge conflicts. This PR covers the remaining fetch and HTTP failure paths on current master, with the same narrow call-health rule for timeouts and JSON-RPC failures. It does not include the SSE or discovery changes from #15448. The timeout session-reset check follows the recovery case discussed in #15448. **What happened?** A shared remote MCP connection works until one call has a network failure, timeout, HTTP 503, or JSON-RPC error. Every later call on that connection returns `tool_not_found`, including healthy sibling tools. Connection setup then asks for user attention even though the failure did not prove that credentials need repair. **Expected behavior** The original invocation retains its failure. Authorized sibling tools remain callable. The gateway does not replay an ambiguous action. HTTP 401/403, missing credentials, and explicit insufficient scope keep their access gates. **Steps to reproduce** 1. Give an agent access to two read tools on one remote MCP connection. 2. Make one tool fail with a network error or HTTP 503. 3. Call the other tool in the same gateway session. 4. Before this fix, the second call fails with `tool_not_found` before provider dispatch. **Paperclip version or commit** `master` at `e6dcd1e8e9`. The regression runs against source with synthetic data. **Deployment mode** Any deployment that uses a shared remote MCP connection. This is a core gateway issue. ## What Changed - Preserve connection health after a network failure, timeout, non-authentication HTTP error, or JSON-RPC error from one tool call. - Retain HTTP 401/403 and insufficient-scope health gates. - Retain HTTP 401/403 gates during MCP initialization and its initialized notification, with HTTP 503 controls for both stages. - Discard only the upstream session used by a network failure or timeout. Let the next explicit call initialize again. - Add sibling-tool regression cases, authentication controls, and real HTTP header/body timeout checks. - Update runtime recovery guidance and the feature and access maps. ## Verification - Red: the four new sibling-tool cases failed on the original implementation. Each second call returned HTTP 404 with `tool_not_found`. HTTP 401/403 control cases passed. - Green: the same six-case selection passed after the production change: `pnpm --filter @paperclipai/server exec vitest run src/__tests__/tool-gateway-service.test.ts -t 'keeps sibling MCP tools usable|keeps the connection blocked' --maxWorkers=1`. - The regression checks the actual gateway and embedded PostgreSQL. The injected provider fault preserves the call boundary. It checks failure recording, no automatic replay, catalog retention, and successful sibling dispatch. - The real local HTTP fixture checks cancellation before headers and during a body, then a successful explicit call with a fresh MCP session. - `node scripts/check-maps.mjs` and `git diff --check` pass. - Complete nearby suites pass: 166 tests across `tool-gateway-service.test.ts`, `tool-gateway.test.ts`, `remote-mcp-protocol.test.ts`, and `mcp-http.test.ts`, with one test worker. - `pnpm --filter @paperclipai/server exec tsc --noEmit` passes after refreshing Runner TypeScript declarations. The first package typecheck used declarations from before the rebase and failed two unchanged native-session call sites; rebuilding that dependency resolved both errors. - The rebased head also passes all 11 selected sibling/authentication/session regressions. - Review regression red: setup-stage HTTP 401/403 controls failed because connection health stayed `ok`; the two setup-stage HTTP 503 controls passed. Green: all six setup controls and the complete 166-test nearby run pass. The fix separately handles typed initialization authentication failures and preserves the existing caller error. The server compiler also passes after this change. - Full repository CI passes on `256a65ccc302c1c31a80e87f1f91411d955028de`: typecheck, build, test shards, Runner checks, browser checks, Canary dry run, and aggregate gates. All checks are terminal: 55 successful, 2 expected skips, and the separate Paperclip Review check is neutral. Greptile is 5/5 on this exact head, with no open findings. - Full repository checks were run in CI. Local verification in this worktree used the focused suites and server compiler to keep parallel work bounded. - This change has no live provider or staging deployment proof. ## Risks - A provider that remains unavailable can stay listed until a health check detects the outage. Each call still fails with its original controlled reason code. - No automatic retry occurs because a failed remote action may already have run. - Already unhealthy connections still need a successful health check after an upgrade. - No schema, credential policy, or permission contract changes. ## Model Used - OpenAI Codex, GPT-6 family, with tool use and code execution. The runtime does not expose the exact serving model ID or context window. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
6f045207bb
commit
370fb50ad4
6 files changed
+171
-15
No files matched your search
@@ -145,6 +145,18 @@ curl -fsS -X PATCH \
|
||||
|
||||
## Verify Recovery
|
||||
|
||||
An individual remote MCP tool-call failure stays on its invocation. A network
|
||||
failure, timeout, non-authentication HTTP error, or JSON-RPC error does not change
|
||||
the connection's shared health or hide its other tools. HTTP 401/403 responses,
|
||||
missing credentials, and explicit insufficient-scope errors retain their access
|
||||
gates. Health checks and catalog refreshes still assess connection health.
|
||||
|
||||
After a network failure or timeout, the next explicit call can start a fresh MCP
|
||||
session. The gateway never automatically replays the failed action: the provider
|
||||
may already have performed it. Check the outcome before retrying a write. If a
|
||||
connection was already marked unhealthy before upgrading, use the health check
|
||||
below to restore access after verifying the endpoint.
|
||||
|
||||
1. Run the connection health check:
|
||||
|
||||
```sh
|
||||
|
||||
@@ -97,6 +97,13 @@ approvals and revocation checks remain independent. Regression coverage lives in
|
||||
[assigned-tool tests](../../server/src/services/native-runtime/assigned-mcp-tools.test.ts);
|
||||
these fixtures do not certify a live external-client journey.
|
||||
|
||||
Remote invocation regressions preserve access to authorized sibling tools after
|
||||
a fetch failure, timeout, non-authentication HTTP error, or JSON-RPC rejection.
|
||||
HTTP 401/403 and insufficient-scope errors keep their connection gates. The
|
||||
gateway records each failed invocation and never automatically replays it.
|
||||
This covers the gateway service with synthetic providers and a local HTTP
|
||||
server, not a live external provider or all client surfaces.
|
||||
|
||||
## `tokens`
|
||||
|
||||
Review: `baseline`.
|
||||
|
||||
@@ -41,6 +41,12 @@ endpoint authorization independently of whether the admin UI is visible.
|
||||
|
||||
Automated: [gateway service](../../server/src/__tests__/tool-gateway-service.test.ts), [connect client dialog](../../ui/src/pages/apps/gateways/ConnectClientDialog.test.tsx), and [token panel](../../ui/src/pages/apps/gateways/panels/TokensPanel.test.tsx) cover service/UI contracts.
|
||||
|
||||
Gateway regressions also cover a failed remote call followed by a successful
|
||||
sibling tool call. Transient failures preserve the catalog. Authentication
|
||||
failures retain their access gates. Real HTTP timeout fixtures verify an explicit
|
||||
retry starts a fresh MCP session without replaying the failed action. These
|
||||
checks do not establish a live provider journey.
|
||||
|
||||
Manual: Create a disposable gateway with one harmless tool, connect a client using its provided configuration, discover/call that tool, and inspect activity. Revoke the client token and confirm the same call fails. Test a tool outside the allowed set.
|
||||
|
||||
### `access-profile`
|
||||
|
||||
@@ -1137,6 +1137,114 @@ describeEmbeddedPostgres("tool gateway service", () => {
|
||||
expect((result.result as { result?: { data?: { target?: string } } }).result?.data?.target).toBe("repo");
|
||||
});
|
||||
|
||||
it.each([
|
||||
["fetch failure", "mcp_remote_fetch_failed"],
|
||||
["timeout", "tool_timeout"],
|
||||
["HTTP 503", "mcp_remote_status"],
|
||||
["JSON-RPC error", "remote_mcp_error"],
|
||||
] as const)("keeps sibling MCP tools usable after a per-call %s", async (failure, reasonCode) => {
|
||||
const { company, agent, run } = await createRunFixture(db);
|
||||
const { application, connection, catalogEntry } = await createRemoteMcpToolFixture(db, company.id);
|
||||
await db.insert(toolCatalogEntries).values({
|
||||
companyId: company.id,
|
||||
applicationId: application.id,
|
||||
connectionId: connection.id,
|
||||
entryKind: "tool",
|
||||
name: "read_status",
|
||||
toolName: "read_status",
|
||||
riskLevel: "read",
|
||||
isReadOnly: true,
|
||||
status: "active",
|
||||
versionHash: randomUUID(),
|
||||
schemaHash: randomUUID(),
|
||||
});
|
||||
await db.insert(toolPolicies).values(readPolicyRow(company, "Allow read tools"));
|
||||
const calls: string[] = [];
|
||||
const gateway = createTestToolGatewayService(db, {
|
||||
remoteHttpRequest: async (_url, init) => {
|
||||
const request = JSON.parse(String(init.body));
|
||||
calls.push(request.params.name);
|
||||
if (request.params.name === catalogEntry.toolName) {
|
||||
if (failure === "fetch failure") throw new TypeError("fetch failed");
|
||||
if (failure === "timeout") throw new DOMException("The operation was aborted", "AbortError");
|
||||
if (failure === "HTTP 503") return new Response("unavailable", { status: 503 });
|
||||
return Response.json({ jsonrpc: "2.0", id: request.id, error: { code: -32602, message: "Invalid tool arguments" } });
|
||||
}
|
||||
return Response.json({ jsonrpc: "2.0", id: request.id, result: { content: [{ type: "text", text: "available" }] } });
|
||||
},
|
||||
});
|
||||
const session = await gateway.createSession(runSessionInput(company, agent, run));
|
||||
const tools = (await gateway.listToolsForSession(session.token)).filter((tool) => tool.connectionId === connection.id);
|
||||
const failingTool = tools.find((tool) => tool.catalogEntryId === catalogEntry.id)!;
|
||||
const siblingTool = tools.find((tool) => tool.upstreamToolName === "read_status")!;
|
||||
|
||||
await expect(gateway.executeTool({ sessionToken: session.token, tool: failingTool.name, parameters: {} }))
|
||||
.rejects.toMatchObject({ reasonCode });
|
||||
expect(calls).toEqual([catalogEntry.toolName]);
|
||||
await expect(gateway.executeTool({ sessionToken: session.token, tool: siblingTool.name, parameters: {} }))
|
||||
.resolves.toMatchObject({ status: "completed" });
|
||||
expect(calls).toEqual([catalogEntry.toolName, "read_status"]);
|
||||
expect((await gateway.listToolsForSession(session.token)).filter((tool) => tool.connectionId === connection.id))
|
||||
.toEqual(expect.arrayContaining(tools));
|
||||
const [updated] = await db.select().from(toolConnections).where(eq(toolConnections.id, connection.id));
|
||||
expect(updated.healthStatus).toBe("ok");
|
||||
const [failed] = await db.select().from(toolInvocations).where(eq(toolInvocations.errorCode, reasonCode));
|
||||
expect(failed).toMatchObject({ status: reasonCode === "tool_timeout" ? "timed_out" : "failed" });
|
||||
});
|
||||
|
||||
it.each([401, 403])("keeps the connection blocked after an HTTP %s authentication failure", async (status) => {
|
||||
const { company, agent, run } = await createRunFixture(db);
|
||||
const { connection } = await createRemoteMcpToolFixture(db, company.id);
|
||||
await db.insert(toolPolicies).values(readPolicyRow(company, "Allow read tools"));
|
||||
const provider = vi.fn(async () => new Response("Access denied", { status }));
|
||||
const gateway = createTestToolGatewayService(db, { remoteHttpRequest: provider });
|
||||
const session = await gateway.createSession(runSessionInput(company, agent, run));
|
||||
const tool = (await gateway.listToolsForSession(session.token)).find((entry) => entry.connectionId === connection.id)!;
|
||||
|
||||
await expect(gateway.executeTool({ sessionToken: session.token, tool: tool.name, parameters: {} }))
|
||||
.rejects.toMatchObject({ reasonCode: "mcp_remote_status", details: { status } });
|
||||
const [updated] = await db.select().from(toolConnections).where(eq(toolConnections.id, connection.id));
|
||||
expect(updated.healthStatus).toBe("error");
|
||||
expect((await gateway.listToolsForSession(session.token)).filter((entry) => entry.connectionId === connection.id)).toEqual([]);
|
||||
expect(provider).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["initialize", 401],
|
||||
["initialize", 403],
|
||||
["initialize", 503],
|
||||
["notifications/initialized", 401],
|
||||
["notifications/initialized", 403],
|
||||
["notifications/initialized", 503],
|
||||
] as const)("gates only authentication failures during %s with HTTP %s", async (method, status) => {
|
||||
const { company, agent, run } = await createRunFixture(db);
|
||||
const { connection } = await createRemoteMcpToolFixture(db, company.id);
|
||||
await db.update(toolConnections).set({
|
||||
config: { ...connection.config, mcpSessionRequired: true },
|
||||
}).where(eq(toolConnections.id, connection.id));
|
||||
await db.insert(toolPolicies).values(readPolicyRow(company, "Allow read tools"));
|
||||
const methods: string[] = [];
|
||||
const gateway = createTestToolGatewayService(db, { remoteHttpRequest: async (_url, init) => {
|
||||
const request = JSON.parse(String(init.body));
|
||||
methods.push(request.method);
|
||||
if (request.method === method) return new Response("Rejected", { status });
|
||||
return Response.json({ jsonrpc: "2.0", id: request.id, result: {
|
||||
protocolVersion: "2025-06-18", capabilities: { tools: {} }, serverInfo: { name: "fixture", version: "1" },
|
||||
} }, { headers: { "mcp-session-id": "setup-session" } });
|
||||
} });
|
||||
const session = await gateway.createSession(runSessionInput(company, agent, run));
|
||||
const tool = (await gateway.listToolsForSession(session.token)).find((entry) => entry.connectionId === connection.id)!;
|
||||
|
||||
await expect(gateway.executeTool({ sessionToken: session.token, tool: tool.name, parameters: {} }))
|
||||
.rejects.toMatchObject({ reasonCode: "mcp_remote_fetch_failed" });
|
||||
const [updated] = await db.select().from(toolConnections).where(eq(toolConnections.id, connection.id));
|
||||
const authenticationFailure = status === 401 || status === 403;
|
||||
expect(updated.healthStatus).toBe(authenticationFailure ? "error" : "ok");
|
||||
const listed = (await gateway.listToolsForSession(session.token)).filter((entry) => entry.connectionId === connection.id);
|
||||
expect(listed).toHaveLength(authenticationFailure ? 0 : 1);
|
||||
expect(methods).toEqual(method === "initialize" ? ["initialize"] : ["initialize", "notifications/initialized"]);
|
||||
});
|
||||
|
||||
it("maps remote MCP elicitation to a durable issue interaction", async () => {
|
||||
const { company, agent, run } = await createRunFixture(db);
|
||||
await createRemoteMcpToolFixture(db, company.id);
|
||||
|
||||
@@ -487,12 +487,14 @@ async function startFakeRemoteMcpServer(handler: (request: FakeMcpRequest) => Pr
|
||||
body?: unknown;
|
||||
rawBody?: string;
|
||||
delayMs?: number;
|
||||
bodyDelayMs?: number;
|
||||
}> | {
|
||||
status?: number;
|
||||
headers?: Record<string, string>;
|
||||
body?: unknown;
|
||||
rawBody?: string;
|
||||
delayMs?: number;
|
||||
bodyDelayMs?: number;
|
||||
}) {
|
||||
const requests: FakeMcpRequest[] = [];
|
||||
const server = createServer(async (req, res) => {
|
||||
@@ -516,10 +518,15 @@ async function startFakeRemoteMcpServer(handler: (request: FakeMcpRequest) => Pr
|
||||
for (const [key, value] of Object.entries(response.headers ?? {})) {
|
||||
res.setHeader(key, value);
|
||||
}
|
||||
if (response.bodyDelayMs) {
|
||||
res.setHeader("content-type", "application/json");
|
||||
res.flushHeaders();
|
||||
await new Promise((resolve) => setTimeout(resolve, response.bodyDelayMs));
|
||||
}
|
||||
if (response.rawBody !== undefined) {
|
||||
res.end(response.rawBody);
|
||||
} else {
|
||||
res.setHeader("content-type", "application/json");
|
||||
if (!res.headersSent) res.setHeader("content-type", "application/json");
|
||||
res.end(JSON.stringify(response.body ?? {
|
||||
jsonrpc: "2.0",
|
||||
id: body?.id ?? "test",
|
||||
@@ -4438,6 +4445,20 @@ rl.on("line", (line) => {
|
||||
}
|
||||
|
||||
const perCallResponseFailures = [
|
||||
{
|
||||
name: "a tool call timeout",
|
||||
reasonCode: "tool_timeout",
|
||||
status: 504,
|
||||
timeoutMs: 300,
|
||||
response: () => ({ delayMs: 1_000, body: {} }),
|
||||
},
|
||||
{
|
||||
name: "a response body timeout",
|
||||
reasonCode: "tool_timeout",
|
||||
status: 504,
|
||||
timeoutMs: 300,
|
||||
response: () => ({ bodyDelayMs: 1_000, body: {} }),
|
||||
},
|
||||
{
|
||||
name: "an oversized response",
|
||||
reasonCode: "mcp_remote_response_too_large",
|
||||
@@ -4494,12 +4515,13 @@ rl.on("line", (line) => {
|
||||
sessionToken: session.token,
|
||||
tool: connectedTool!.name,
|
||||
parameters: { key: "everything" },
|
||||
timeoutMs: scenario.timeoutMs,
|
||||
}).then(
|
||||
() => {
|
||||
throw new Error("Expected the remote MCP call to fail");
|
||||
},
|
||||
(error) => {
|
||||
expectGatewayError(error, 502, scenario.reasonCode);
|
||||
expectGatewayError(error, scenario.status ?? 502, scenario.reasonCode);
|
||||
scenario.check?.(error as ToolGatewayHttpError);
|
||||
},
|
||||
);
|
||||
@@ -4508,6 +4530,13 @@ rl.on("line", (line) => {
|
||||
eq(toolConnections.id, connection.id),
|
||||
)).resolves.toEqual([{ healthStatus: "ok" }]);
|
||||
|
||||
expect(toolCallCount).toBe(1);
|
||||
const [failedInvocation] = await db.select().from(toolInvocations);
|
||||
expect(failedInvocation).toMatchObject({
|
||||
status: scenario.status === 504 ? "timed_out" : "failed",
|
||||
errorCode: scenario.reasonCode,
|
||||
});
|
||||
|
||||
const nextSession = await gateway.createSession({ companyId: company.id, agentId: agent.id, runId: run.id });
|
||||
await expect(gateway.listToolsForSession(nextSession.token)).resolves.toEqual(
|
||||
expect.arrayContaining([expect.objectContaining({ name: connectedTool!.name })]),
|
||||
|
||||
@@ -119,6 +119,7 @@ import {
|
||||
forgetMcpHttpSession,
|
||||
readMcpHttpResponse,
|
||||
McpHttpResponseError,
|
||||
McpHttpInitializationError,
|
||||
mcpHttpRequestHeaders,
|
||||
parseMcpHttpResponseBody,
|
||||
} from "./mcp-http.js";
|
||||
@@ -6312,7 +6313,7 @@ export function createToolGatewayService(
|
||||
if (!response.ok) {
|
||||
// Session expiration is recoverable on an explicit retry. Marking the
|
||||
// connection unhealthy here would hide every tool and prevent it.
|
||||
if (!sessionExpired) {
|
||||
if (response.status === 401 || response.status === 403) {
|
||||
await markCallHealth("error", "Remote MCP server returned an HTTP error.");
|
||||
}
|
||||
throw new ToolGatewayHttpError(
|
||||
@@ -6361,10 +6362,6 @@ export function createToolGatewayService(
|
||||
}
|
||||
if (payloadRecord.error !== undefined) {
|
||||
const errorRecord = asRecord(payloadRecord.error);
|
||||
await markCallHealth(
|
||||
"error",
|
||||
"Remote MCP server returned a JSON-RPC error.",
|
||||
);
|
||||
throw new ToolGatewayHttpError(
|
||||
502,
|
||||
"Remote MCP server returned an error",
|
||||
@@ -6437,10 +6434,7 @@ export function createToolGatewayService(
|
||||
);
|
||||
}
|
||||
if (error instanceof Error && error.name === "AbortError") {
|
||||
await markCallHealth(
|
||||
"error",
|
||||
"Remote MCP tool call timed out.",
|
||||
);
|
||||
if (mcpSession) forgetMcpHttpSession(mcpSession);
|
||||
throw new ToolGatewayHttpError(
|
||||
504,
|
||||
"Remote MCP tool call timed out",
|
||||
@@ -6452,10 +6446,10 @@ export function createToolGatewayService(
|
||||
},
|
||||
);
|
||||
}
|
||||
await markCallHealth(
|
||||
"error",
|
||||
"Remote MCP tool call failed.",
|
||||
);
|
||||
if (error instanceof McpHttpInitializationError && (error.status === 401 || error.status === 403)) {
|
||||
await markCallHealth("error", "Remote MCP session authorization failed.");
|
||||
}
|
||||
if (mcpSession) forgetMcpHttpSession(mcpSession);
|
||||
throw new ToolGatewayHttpError(
|
||||
502,
|
||||
"Remote MCP tool call failed",
|
||||
|
||||
Reference in new issue
Block a user