mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-12 06:10:44 +01:00
Pavel-GuseynovandClaude Opus 5.5
4d297b998a
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The tool gateway calls remote MCP servers over Streamable HTTP, and a server can answer `tools/call` with a `text/event-stream` body > - Such a server can send progress notifications before the response, and a slow tool can take more than 10 seconds > - The gateway aborts a remote MCP `tools/call` that uses the default deadline after 10 seconds, the general tool default > - When the call fails, the call evidence does not show how far the stream got, or whether the cause was the deadline or a broken transport > - This pull request gives remote MCP calls a default deadline of 30 seconds, the default response timeout of the remote HTTP client, and records the failure cause and the stream progress > - The benefit is that slow remote tools complete, and an operator can tell a deadline from a peer reset in the call evidence ## Linked Issues or Issue Description Refs #10147. That issue reports that a named MCP gateway aborts remote tools after 10 seconds and ignores the connection timeoutMs. This pull request raises the default deadline of a remote MCP `tools/call` to 30 seconds. It does not make the named gateway read the connection timeoutMs. Related: #11910 (open) keeps a connection callable after one timed-out tools/call. On current master a timeout and a JSON-RPC response error already keep the connection healthy, and this pull request keeps that policy. I searched pull requests and issues for "event stream tools/call", "SSE MCP tool call hang", "remote MCP timeout stream", "mcp text/event-stream" and "tool_timeout remote MCP". The only related open pull request is #11910. ## What Changed - `server/src/services/remote-http-fetch.ts`: the default response timeout is exported as `DEFAULT_REMOTE_HTTP_RESPONSE_TIMEOUT_MS` (30,000 ms, unchanged value). - `server/src/services/tool-gateway.ts`: - A remote MCP `tools/call` that uses the default deadline now gets `DEFAULT_REMOTE_HTTP_RESPONSE_TIMEOUT_MS` instead of the general tool default of 10 seconds. The Railway `run-command` budget and an explicit `timeoutMs` keep their values. - The call evidence records the response status, content type and request id before the body is read. A body that fails partway records the bytes received so far as its size. - A failed call records `failureKind` (`invocation_timeout`, `protocol_failure` or `transport_failure`), the deadline and whether it was aborted, the stream progress, and the error with its cause chain. Each string in the chain is redacted with `redactSensitiveText`, and a cyclic cause chain is cut with a "[Circular cause]" marker. - A deadline abort is a 504 `tool_timeout` and starts a fresh MCP session on the next call, as before. The connection health policy of master does not change. - A deliberate stop of a call (`elicitation_required`, `provider_interaction_required`, `oauth_insufficient_scope`, `oauth_refreshed_retry_required`) keeps its own reason when the deadline passes while it is recorded, as on master. An error that is already a gateway error also gets a failure kind: `tool_timeout` is `invocation_timeout`; a malformed, invalid or oversized response is `protocol_failure`; and `remote_http_*` (for example a DNS failure) and `mcp_remote_fetch_failed` are `transport_failure`. - A non-success response body is read as a stream, so a body that fails partway records the bytes that arrived. The reader stops at the size limit and cancels the body, as the `tools/call` reader does. #19785 makes the same reader change for the context path. - `server/src/services/mcp-http.ts`: `readMcpHttpResponse` takes an `onProgress` callback. It reports bytes, chunks and events received, whether the matching response arrived, and whether the stream ended. As on master, the read returns as soon as the response with the matching id arrives. - Tests: - `server/src/__tests__/mcp-http.test.ts`: a partial event followed by a peer error keeps the progress that was consumed, and releases the reader. - `server/src/__tests__/tool-gateway.test.ts`: six scenarios run against a remote server that streams one progress event first: slow success after 12 seconds, the default deadline, an explicit 1,500 ms deadline, a peer reset, a cyclic error cause, and a primitive error value. - `server/src/__tests__/tool-gateway.test.ts`: three regression tests: a tool question sent after the deadline passed keeps `elicitation_required`; a DNS failure records `transport_failure`; and a 503 body that fails partway records the bytes that arrived. All three fail on717e6c77cand pass on this head. - `server/src/__tests__/tool-gateway-service.test.ts`: when the call deadline passes during a forced Vercel token refresh, the call keeps the 409 `oauth_refreshed_retry_required` stop. `tool-gateway.ts` adds this reason to the deliberate stops, so the deadline does not change it to a 504 `tool_timeout`. ## Verification Head SHA: `25c6e703d1ded895989e9d2fcd6d6352a4f851f5`. It is the previous head `7836447f2` plus commit `25c6e703d`, which keeps the `oauth_refreshed_retry_required` reason when the call deadline passes during a token refresh. At this head, run from `server` with pnpm 9.15.4: `pnpm typecheck`: no errors; `mcp-http.test.ts`, `tool-gateway.test.ts` and `tool-gateway-service.test.ts`: 307 passed. Without the one-line change in `tool-gateway.ts`, the new test "keeps the explicit retry reason when the call deadline passes during the Vercel token refresh" fails: it receives 504 `tool_timeout`, not 409 `oauth_refreshed_retry_required`. Previous head: `7836447f24e2e6e9e0c402d4644d1f7f69b75cb8`. It is an ordinary merge of master `76994c50275dbc005cc248e11189a38baa21719f`, which includes #19877, into the previous head `00150e59c`. At this head, run from `server` with pnpm 9.15.4: `pnpm typecheck` and the test commands below. Results: typecheck: no errors; the 2 test files above: 116 passed. Previous head:00150e59c1. It merges master18b6b71d8. Run from `server`: ```sh pnpm exec vitest run src/__tests__/mcp-http.test.ts src/__tests__/tool-gateway.test.ts ``` - With this branch: 116 passed. With tool-gateway-service.test.ts and remote-mcp-socks-proxy.test.ts added: 315 passed in 4 files. - With `services/mcp-http.ts`, `services/remote-http-fetch.ts` and `services/tool-gateway.ts` from master and the test files from this branch, measured at717e6c77c: 7 failed, 106 passed. The failing tests are the six "records remote SSE ... without confusing a call deadline with a broken transport" scenarios and "retains consumed SSE progress when a partial event fails and releases the reader". - The existing master tests "keeps a remote MCP connection healthy and starts a fresh MCP session after a tool call timeout" and "... after a response body timeout" pass with this branch. - Typecheck: `pnpm --filter @paperclipai/server typecheck` reports 29 errors, all "Cannot find module @paperclipai/plugin-sdk" and the implicit any that follows, in the sandbox provider plugin packages of my local install. None is in a file that this pull request touches. - I ran the tests with an embedded PostgreSQL from the repository dependencies. The full upstream suite runs in CI. I ran only the test files named above. ## Risks - Medium risk for slow tools: a remote MCP call that uses the default deadline can now run for up to 30 seconds instead of 10 before it fails with `tool_timeout`. An explicit `timeoutMs` still applies. - The call evidence is larger: it can hold a redacted error chain and its stack traces. All strings go through `redactSensitiveText`. - No automatic replay: a timed-out `tools/call` is not sent again, because it can have changed data in the remote app. ## Model Used - Original change: the model that wrote it was not recorded. Port to current master, the adaptation to the connection health policy of master, and this description: Claude Opus 5.5 (claude-opus-5-5), with tool use and terminal code execution. ## 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 change needed) - [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 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>