mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-11 05:31:46 +02:00
<!-- Write all pull request text in Simplified Technical English (ASD-STE100): short sentences, one instruction per sentence, simple approved vocabulary, and the active voice. --> ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Agents call remote MCP tools through the Paperclip tool gateway > - The gateway keeps a health status for each remote MCP connection, and it hides the tools of a connection that has `healthStatus = "error"` > - One oversized, malformed, or invalid-JSON `tools/call` reply set the whole connection to `error` > - Health goes back to `ok` only after a successful call, but the hidden tools prevent that call, so the connection stayed locked until a person reconnected it > - This pull request makes these reply errors fail only the one call, and it starts a new MCP session for the caller on the next call > - The benefit is that one large or bad reply no longer disconnects a working connection for every agent ## Linked Issues or Issue Description No public issue exists. Related open PRs fix other health downgrades in the same function. They do not overlap with this change: - Refs #11910 (timeouts and JSON-RPC errors) - Refs #15325 (transport errors) **What happened?** An agent called a remote MCP tool that returned more than `MAX_REMOTE_MCP_RESPONSE_BYTES` (1 MB). The gateway returned 502 `mcp_remote_response_too_large`. It also set the connection to `healthStatus = "error"`. Then `connectedMcpConnectionFilter` hid all tools of the connection (except `per_user` connections), and `connections_search` showed the connection as `needs_user_action`. The `malformed_response` and `invalid_json` errors from `readMcpHttpResponse` did the same thing. `readMcpHttpResponse` can also cancel the reply stream before the end. The remote server can then close its MCP session. The gateway kept the cached `mcp-session-id` for up to 30 minutes and cleared it only after a 404. The next calls then failed with "Remote MCP session expired". **Expected behavior** A reply that is too large or not correct fails only that call. The connection stays healthy, and its tools stay visible. The next call uses a new MCP session. **Steps to reproduce** 1. Add a remote MCP connection with `mcpSessionRequired: true`. 2. Call a tool that returns a reply larger than 1 MB. 3. Look at the connection: `healthStatus` is `error`. 4. Start a new gateway session: the tools of the connection are not in the list. **Paperclip version or commit** `master` at `99a9de9940bf5974352d9dbfbb2f21e62e89689f` **Deployment mode** All modes. The fault is in the server tool gateway. ## What Changed - `server/src/services/tool-gateway.ts`: `too_large`, `malformed_response`, and `invalid_json` from `readMcpHttpResponse` now fail only the call. The caller gets the same 502 reason code as before. The gateway does not call `markRemoteConnectionHealth(…, "error")` for these errors. The `invalid_json` branch after `JSON.parse(body)` also does not change health now, so all `invalid_json` paths are the same. - The `mcp_remote_response_too_large` message now tells the caller to request a smaller result, for example a narrower query or a smaller page size. The error details now include `maxBytes`. - `server/src/services/mcp-http.ts`: new `forgetMcpHttpSession()`. It removes only the cached session for one scope and one credential set, and only while that entry still holds the session ID that failed. It does not touch other agents, sessions that are initializing, or a newer session that replaced the failed one. - The gateway calls `forgetMcpHttpSession()` after these reply errors, so the next call from that caller initializes a new session. The existing 404 "session expired" path now uses the same function. Before, both paths cleared all sessions and all pending initializations on the connection. That made a concurrent initialization by another agent fail with "MCP connection changed while initializing", which the gateway reported as a fetch failure and marked as a connection `error`. - `forgetMcpHttpSessions(connectionId)` is not changed. Disconnect and revocation still clear the full connection. Why `malformed_response` and `invalid_json` are also per-call errors: - Each error is about one reply body. The server was reachable, accepted the credentials, and sent HTTP 2xx. A reply can be too large or bad because of the tool and its arguments. That is not a fault of the connection. - The other malformed-reply checks in the same function (payload is not an object, or has no `result`) already throw `remote_mcp_malformed_response` and do not change health. Only the reader-level errors changed health. - Health recovers only after a successful call. An `error` status for one bad reply therefore locks the connection until a person reconnects it. - Other failures (HTTP errors, fetch failures, timeouts, JSON-RPC errors) still change health. This PR does not change them. #11910 and #15325 address some of them. ## Verification - `server/src/__tests__/tool-gateway.test.ts`: the recovery test now runs for three replies: oversized, invalid JSON, and a reply without the requested message ID. Each run uses a fake HTTP MCP server with `mcpSessionRequired: true` that gives `session-N` for each `initialize`. Each run checks that: - the call fails with the correct 502 reason code (and, for the oversized reply, the smaller-page hint and `maxBytes: 1000000`) - `healthStatus` stays `ok` - a new gateway session still lists the tool and can call it - the second `tools/call` uses `session-2`, not `session-1` - New test: agent A gets an oversized reply while agent B initializes a session on the same connection. Agent B's call completes, health stays `ok`, and the two calls use `session-1` and `session-2`. With the old connection-wide reset, this test fails: agent B gets 502 `mcp_remote_fetch_failed`. - `server/src/__tests__/remote-mcp-protocol.test.ts`: new unit test. `forgetMcpHttpSession()` keeps the session of a different identity, and a late failure from an old session does not remove the newer session. - Each regression check fails without its fix. Without the health change, the test fails on `healthStatus: 'error'`. Without the session reset, it fails with `['session-1', 'session-1']`. ``` cd server npx vitest run src/__tests__/tool-gateway.test.ts src/__tests__/tool-gateway-service.test.ts src/__tests__/remote-mcp-protocol.test.ts Test Files 3 passed (3) Tests 132 passed (132) ``` - I did not run the full server `tsc --noEmit` locally because the sandbox does not have sufficient memory. The CI typecheck covers it. ## Risks - Low risk. The change affects only the error path of remote MCP `tools/call`. - A remote server that always sends bad replies now keeps `healthStatus = "ok"`. Each call still fails with a clear 502 reason code and an audit record, so the failure stays visible. Only the connection-wide hiding of tools stops. - After one of these errors, the next call from the same caller sends one more `initialize` request. This adds one round trip. - A 404 "session expired" now clears only the session of the caller that got the 404. Before, it cleared the cached sessions of all agents on the connection. If the remote server restarts, each agent now gets its own "session expired" error one time and then initializes again. A 404 is about one session, and the old connection-wide clear also cancelled other agents' initializations. - #11910 and #15325 change the same catch block. The PR that merges last can have a small merge conflict. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - Claude Opus 5.5 (Anthropic), model ID `claude-opus-5-5`, 1M-token context window. - Run as an agent in Claude Code with tool use (shell, file edit, and local test runs). ## 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 (no user-facing documentation changes are necessary) - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge This PR replaces #15418. It keeps the same commits on a branch name without an internal ticket ID, and adds a fix for the Greptile review on #15418. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: devinfoley <139239+devinfoley@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
283 lines
12 KiB
TypeScript
283 lines
12 KiB
TypeScript
import { createHash } from "node:crypto";
|
|
// Helpers for talking to remote MCP servers over the Streamable HTTP transport.
|
|
//
|
|
// The MCP Streamable HTTP spec requires the client to advertise that it accepts
|
|
// BOTH a single JSON response and an SSE stream on every POST:
|
|
//
|
|
// Accept: application/json, text/event-stream
|
|
//
|
|
// Spec-compliant servers reject requests missing this header with 406 Not
|
|
// Acceptable, and when the header is present they are free to answer with an
|
|
// SSE stream (`event: message\ndata: {…}`) instead of a bare JSON body. So any
|
|
// code path that POSTs JSON-RPC to a remote `/mcp` endpoint must (a) send the
|
|
// Accept header and (b) be able to read an SSE-framed response.
|
|
|
|
/** The Accept header value required by the MCP Streamable HTTP transport. */
|
|
export const MCP_HTTP_ACCEPT = "application/json, text/event-stream";
|
|
export const MCP_PROTOCOL_VERSION = "2025-06-18";
|
|
|
|
export class McpHttpResponseError extends Error {
|
|
constructor(readonly reason: "invalid_json" | "malformed_response" | "too_large", message: string) {
|
|
super(message);
|
|
this.name = "McpHttpResponseError";
|
|
}
|
|
}
|
|
|
|
/**
|
|
* Default headers for an MCP Streamable HTTP JSON-RPC POST. Caller-supplied
|
|
* headers (e.g. resolved credentials) are preserved, while the required
|
|
* Streamable HTTP Accept value is kept authoritative.
|
|
*/
|
|
export function mcpHttpRequestHeaders(extra?: Record<string, string>): Record<string, string> {
|
|
return {
|
|
"content-type": "application/json",
|
|
...extra,
|
|
accept: MCP_HTTP_ACCEPT,
|
|
};
|
|
}
|
|
|
|
export class McpHttpInitializationError extends Error {
|
|
constructor(
|
|
message: string,
|
|
readonly stage: "initialize" | "initialized_notification",
|
|
readonly status: number | null,
|
|
readonly response?: Response,
|
|
) {
|
|
super(message);
|
|
this.name = "McpHttpInitializationError";
|
|
}
|
|
}
|
|
|
|
/**
|
|
* Establish a Streamable HTTP session. Callers may retain protocol headers in
|
|
* the in-memory cache scoped to the connection and effective credential identity.
|
|
*/
|
|
export async function initializeMcpHttpSession(input: {
|
|
send: (init: RequestInit) => Promise<Response>;
|
|
headers?: Record<string, string>;
|
|
requestId: string;
|
|
}): Promise<Record<string, string>> {
|
|
const initializeResponse = await input.send({
|
|
method: "POST",
|
|
headers: mcpHttpRequestHeaders(input.headers),
|
|
body: JSON.stringify({
|
|
jsonrpc: "2.0",
|
|
id: `${input.requestId}-initialize`,
|
|
method: "initialize",
|
|
params: {
|
|
protocolVersion: MCP_PROTOCOL_VERSION,
|
|
capabilities: {},
|
|
clientInfo: { name: "paperclip", version: "1" },
|
|
},
|
|
}),
|
|
});
|
|
if (!initializeResponse.ok) {
|
|
throw new McpHttpInitializationError(
|
|
`Remote MCP initialization returned HTTP ${initializeResponse.status}`,
|
|
"initialize",
|
|
initializeResponse.status,
|
|
initializeResponse,
|
|
);
|
|
}
|
|
let payload: unknown;
|
|
try {
|
|
payload = await readMcpHttpResponse(initializeResponse, `${input.requestId}-initialize`);
|
|
} catch {
|
|
throw new McpHttpInitializationError("Remote MCP initialization returned an invalid response", "initialize", null);
|
|
}
|
|
const result = payload && typeof payload === "object" && "result" in payload
|
|
? (payload as { result?: unknown }).result
|
|
: null;
|
|
const resultRecord = result && typeof result === "object" ? result as Record<string, unknown> : null;
|
|
if (!resultRecord) throw new McpHttpInitializationError("Remote MCP initialization failed", "initialize", null);
|
|
const protocolVersion = typeof resultRecord?.protocolVersion === "string" && resultRecord.protocolVersion
|
|
? resultRecord.protocolVersion
|
|
: MCP_PROTOCOL_VERSION;
|
|
const sessionId = initializeResponse.headers.get("mcp-session-id");
|
|
const sessionHeaders: Record<string, string> = {
|
|
...(input.headers ?? {}),
|
|
"MCP-Protocol-Version": protocolVersion,
|
|
...(sessionId ? { "Mcp-Session-Id": sessionId } : {}),
|
|
};
|
|
const initializedResponse = await input.send({
|
|
method: "POST",
|
|
headers: mcpHttpRequestHeaders(sessionHeaders),
|
|
body: JSON.stringify({
|
|
jsonrpc: "2.0",
|
|
method: "notifications/initialized",
|
|
params: {},
|
|
}),
|
|
});
|
|
if (!initializedResponse.ok) {
|
|
throw new McpHttpInitializationError(
|
|
`Remote MCP initialized notification returned HTTP ${initializedResponse.status}`,
|
|
"initialized_notification",
|
|
initializedResponse.status,
|
|
);
|
|
}
|
|
return sessionHeaders;
|
|
}
|
|
|
|
function looksLikeJsonRpcMessage(value: unknown): boolean {
|
|
if (typeof value !== "object" || value === null) return false;
|
|
const record = value as Record<string, unknown>;
|
|
return "result" in record || "error" in record || "method" in record || "id" in record;
|
|
}
|
|
|
|
/**
|
|
* Parse the body of an MCP Streamable HTTP response into its JSON-RPC payload.
|
|
*
|
|
* Handles both response shapes the transport allows:
|
|
* - `application/json`: the body is the JSON-RPC message directly.
|
|
* - `text/event-stream`: one or more SSE events; we return the JSON payload of
|
|
* the first `data:` event that parses as a JSON-RPC message.
|
|
*
|
|
* Falls back to a plain JSON parse when the content type is unknown so we stay
|
|
* compatible with non-compliant servers that ignore the Accept header.
|
|
*/
|
|
export function parseMcpHttpResponseBody(bodyText: string, contentType: string | null): unknown {
|
|
const isEventStream = (contentType ?? "").toLowerCase().includes("text/event-stream");
|
|
if (!isEventStream) {
|
|
return JSON.parse(bodyText) as unknown;
|
|
}
|
|
|
|
// Split the SSE stream into events on blank lines, then collect each event's
|
|
// `data:` lines (which may span multiple lines per the SSE spec).
|
|
const events = bodyText.replace(/\r\n/g, "\n").split(/\n\n+/);
|
|
let lastError: unknown = null;
|
|
let firstParsed: unknown;
|
|
let sawData = false;
|
|
for (const event of events) {
|
|
const dataLines = event
|
|
.split("\n")
|
|
.filter((line) => line.startsWith("data:"))
|
|
.map((line) => line.slice("data:".length).replace(/^ /, ""));
|
|
if (dataLines.length === 0) continue;
|
|
const data = dataLines.join("\n");
|
|
let parsed: unknown;
|
|
try {
|
|
parsed = JSON.parse(data) as unknown;
|
|
} catch (error) {
|
|
lastError = error;
|
|
continue;
|
|
}
|
|
if (!sawData) {
|
|
firstParsed = parsed;
|
|
sawData = true;
|
|
}
|
|
if (looksLikeJsonRpcMessage(parsed)) {
|
|
return parsed;
|
|
}
|
|
}
|
|
if (sawData) return firstParsed;
|
|
if (lastError) throw lastError;
|
|
throw new SyntaxError("MCP SSE response contained no data events");
|
|
}
|
|
|
|
/** Read until the response for this request arrives, without waiting for an SSE
|
|
* connection to close. Notifications and responses for other IDs are ignored. */
|
|
export async function readMcpHttpResponse(
|
|
response: Response,
|
|
requestId: string | number,
|
|
options: { maxBytes?: number; onRequest?: (message: Record<string, unknown>) => Promise<void> } = {},
|
|
): Promise<unknown> {
|
|
const maxBytes = options.maxBytes ?? 8 * 1024 * 1024;
|
|
const isStream = response.headers.get("content-type")?.toLowerCase().includes("text/event-stream");
|
|
const reader = response.body?.getReader();
|
|
// Injected HTTP transports can expose a buffered text response rather than a
|
|
// Web ReadableStream. Keep the same size and message-ID checks for both forms.
|
|
if (!reader) {
|
|
const body = await response.text();
|
|
if (Buffer.byteLength(body, "utf8") > maxBytes) throw new McpHttpResponseError("too_large", "MCP response exceeded the size limit");
|
|
return readMcpHttpResponse(new Response(body, {
|
|
headers: { "content-type": response.headers.get("content-type") ?? "application/json" },
|
|
}), requestId, options);
|
|
}
|
|
const decoder = new TextDecoder();
|
|
let buffer = "";
|
|
let bytes = 0;
|
|
const parse = (text: string): unknown => {
|
|
try { return JSON.parse(text); }
|
|
catch { throw new McpHttpResponseError("invalid_json", "MCP response contained invalid JSON"); }
|
|
};
|
|
const inspect = async (message: unknown): Promise<unknown | undefined> => {
|
|
if (!message || typeof message !== "object") return undefined;
|
|
const record = message as Record<string, unknown>;
|
|
if (record.id === requestId && ("result" in record || "error" in record)) return record;
|
|
if ("method" in record && "id" in record) await options.onRequest?.(record);
|
|
return undefined;
|
|
};
|
|
const event = async (value: string) => {
|
|
const data = value.split("\n").filter((line) => line.startsWith("data:")).map((line) => line.slice(5).replace(/^ /, "")).join("\n");
|
|
if (!data) return undefined;
|
|
return inspect(parse(data));
|
|
};
|
|
try {
|
|
while (true) {
|
|
const { value, done } = await reader.read();
|
|
bytes += value?.byteLength ?? 0;
|
|
if (bytes > maxBytes) throw new McpHttpResponseError("too_large", "MCP response exceeded the size limit");
|
|
buffer += decoder.decode(value, { stream: !done });
|
|
if (isStream) {
|
|
// Normalize CRLF after concatenating chunks, including split CR/LF pairs.
|
|
buffer = buffer.replace(/\r\n/g, "\n");
|
|
let boundary: number;
|
|
while ((boundary = buffer.indexOf("\n\n")) >= 0) {
|
|
const result = await event(buffer.slice(0, boundary));
|
|
buffer = buffer.slice(boundary + 2);
|
|
if (result !== undefined) return result;
|
|
}
|
|
}
|
|
if (done) break;
|
|
}
|
|
const result = isStream ? await event(buffer) : await inspect(parse(buffer));
|
|
if (result !== undefined) return result;
|
|
throw new McpHttpResponseError("malformed_response", "MCP response did not contain the requested message ID");
|
|
} finally {
|
|
await reader.cancel().catch(() => undefined);
|
|
reader.releaseLock();
|
|
}
|
|
}
|
|
|
|
const sessions = new Map<string, { headers: Record<string, string>; expiresAt: number }>();
|
|
const initializing = new Map<string, Promise<Record<string, string>>>();
|
|
const SESSION_TTL_MS = 30 * 60_000;
|
|
|
|
function mcpHttpSessionKey(scope: string, headers: Record<string, string> | undefined) {
|
|
return `${scope}:${createHash("sha256").update(JSON.stringify(Object.entries(headers ?? {}).sort())).digest("hex")}`;
|
|
}
|
|
|
|
/** Cache only protocol headers; credential hashes and scope separate every
|
|
* connection and effective identity. Never cache a tool call or replay a write. */
|
|
export async function getMcpHttpSession(input: Parameters<typeof initializeMcpHttpSession>[0] & { scope: string }) {
|
|
const key = mcpHttpSessionKey(input.scope, input.headers);
|
|
const cached = sessions.get(key);
|
|
if (cached && cached.expiresAt > Date.now()) return { ...input.headers, ...cached.headers };
|
|
const pending = initializing.get(key);
|
|
if (pending) return pending;
|
|
const promise = initializeMcpHttpSession(input).then((headers) => {
|
|
if (initializing.get(key) !== promise) throw new Error("MCP connection changed while initializing; reconnect before calling tools");
|
|
for (const [id, value] of sessions) if (value.expiresAt <= Date.now()) sessions.delete(id);
|
|
if (sessions.size >= 1000) sessions.delete(sessions.keys().next().value!);
|
|
const protocolHeaders = Object.fromEntries(Object.entries(headers).filter(([name]) => ["mcp-session-id", "mcp-protocol-version"].includes(name.toLowerCase())));
|
|
sessions.set(key, { headers: protocolHeaders, expiresAt: Date.now() + SESSION_TTL_MS });
|
|
return headers;
|
|
}).finally(() => { if (initializing.get(key) === promise) initializing.delete(key); });
|
|
initializing.set(key, promise);
|
|
return promise;
|
|
}
|
|
|
|
/** Drop one identity's cached session after the server may have ended it. Other
|
|
* identities, in-flight initializations, and a session that already replaced
|
|
* the failed one are kept. */
|
|
export function forgetMcpHttpSession(input: { scope: string; headers?: Record<string, string>; sessionId: string }) {
|
|
const key = mcpHttpSessionKey(input.scope, input.headers);
|
|
const cached = sessions.get(key);
|
|
if (cached && new Headers(cached.headers).get("mcp-session-id") === input.sessionId) sessions.delete(key);
|
|
}
|
|
|
|
export function forgetMcpHttpSessions(connectionId: string) {
|
|
for (const key of sessions.keys()) if (key.startsWith(`${connectionId}:`)) sessions.delete(key);
|
|
for (const key of initializing.keys()) if (key.startsWith(`${connectionId}:`)) initializing.delete(key);
|
|
}
|