From 3c7af351031381d6521581ff3fac4ad5c93526fd Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Wed, 4 Nov 2026 18:14:52 -0800 Subject: [PATCH] fix(errors): return tool gateway rejections with their own status (#19158) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents call connected tools through the tool gateway. Some tool calls need a human review before they run. > - A user approves a tool review from a task card. The approve route calls the gateway. > - The gateway can reject the approval with `ToolGatewayHttpError(409, "approved_tool_target_changed")`. This is an expected rejection: the tool or connection changed after the review. > - `ToolGatewayHttpError` does not extend `HttpError`. The shared error handler only keeps the status of an `HttpError`. Thus the route answers 500 "Internal server error" and sends the rejection to Sentry as a crash. > - The user does not see the reason. The user clicks Approve again, and each click makes one more crash report. One user made 69 reports on staging this way. > - This pull request makes the shared error handler keep the status and reason code of client (4xx) gateway errors. > - The benefit is a clear 409 with a reason for the user, and no false crash reports. ## Linked Issues or Issue Description Refs #12775 (related community PR. It changes `ToolGatewayHttpError` to extend `HttpError`, which affects every `instanceof HttpError` check. This PR is smaller and keeps gateway server failures on the crash path.) **What happened?** A user approves a tool review on a task card. The tool definition or connection changed after the review. The API answers 500 "Internal server error". Sentry records the expected rejection "Tool definition or connection changed; request a new review" as a server crash. Each retry records one more event. **Expected behavior** The API answers 409 with `code: "approved_tool_target_changed"` and the message "Tool definition or connection changed; request a new review". Sentry does not record a crash. **Steps to reproduce** 1. Make a connected tool that needs approval, and start a tool call so that a review card appears. 2. Change the tool connection (for example, its config) before you approve. 3. Click Approve on the task card (`POST /api/issues/:id/interactions/:interactionId/accept`). 4. See 500 "Internal server error" and a new Sentry event from `approveActionRequest`. **Paperclip version or commit** `master` at `482ec0aea9ab6f2684baaa1487f2c7a2eedf009e`. ## What Changed - `server/src/middleware/error-handler.ts`: the handler converts a `ToolGatewayHttpError` with a 4xx status to an `HttpError`. The new error keeps the status, the message, the gateway details, and the reason code as `code`. The original stack stays. - Gateway errors with a 5xx status do not change. They still go on the crash path, answer 500 "Internal server error", and go to Sentry. Thus provider text in those messages stays hidden. - `server/src/__tests__/error-handler-tool-gateway.test.ts`: new tests for the 409 mapping, for the unchanged 5xx path, and for an Express route that lets the gateway error escape. ## Verification - `cd server && npx vitest run src/__tests__/error-handler-tool-gateway.test.ts src/__tests__/error-handler.test.ts` passes: 21 tests. - Without the fix, the new tests fail with `expected 500 to be 409`. With the fix, they pass. - `src/__tests__/issue-thread-interaction-routes.test.ts` and `src/__tests__/json-parse-error-handler.test.ts` also pass: 175 tests in 4 suites with the two error handler suites. - I checked the runtime import graph of `services/tool-gateway.ts` (685 modules). It does not import `middleware/error-handler.ts`, so the new import makes no cycle. - I did not run the full server typecheck locally, because it ran out of memory in an 8 GB sandbox. CI must do this check. ## Risks - Low risk. Only gateway errors with a 4xx status change. Before, they became 500. Now they keep their own status. - Some routes outside the gateway router can now return gateway 4xx details in the `details` field. The gateway routes already return the same details through `sendGatewayError`. - `middleware/error-handler.ts` now imports `services/tool-gateway.ts`. Unit tests that import the error handler load more modules. The app already loads this module. - This PR does not fix the cause of some of these rejections. A routine OAuth token refresh changes values in the signed approval snapshot (token secret versions, the order of grant secret refs, and token expiry times). This can make a pending review fail. That fix changes `services/tool-gateway.ts` and comes in a separate PR. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used as a coding agent with tool use (shell, file edits, test runs, and GitHub API). ## 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 documentation change is 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 --- .../error-handler-tool-gateway.test.ts | 91 +++++++++++++++++++ server/src/middleware/error-handler.ts | 18 +++- 2 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 server/src/__tests__/error-handler-tool-gateway.test.ts diff --git a/server/src/__tests__/error-handler-tool-gateway.test.ts b/server/src/__tests__/error-handler-tool-gateway.test.ts new file mode 100644 index 00000000000..d5c27696883 --- /dev/null +++ b/server/src/__tests__/error-handler-tool-gateway.test.ts @@ -0,0 +1,91 @@ +import express from "express"; +import type { Request, Response } from "express"; +import request from "supertest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { errorHandler } from "../middleware/error-handler.js"; +import { ToolGatewayHttpError } from "../services/tool-gateway.js"; + +const captureExceptionMock = vi.hoisted(() => vi.fn()); +const trackErrorHandlerCrashMock = vi.hoisted(() => vi.fn()); + +vi.mock("../services/responsible-user-denial-run-outcomes.js", () => ({ + recordResponsibleUserDenialOnActiveRun: vi.fn().mockResolvedValue(null), +})); +vi.mock("../sentry.js", () => ({ captureException: captureExceptionMock })); +vi.mock("../telemetry.js", () => ({ getTelemetryClient: () => ({}) })); +vi.mock("@paperclipai/shared/telemetry", () => ({ + trackErrorHandlerCrash: trackErrorHandlerCrashMock, +})); + +function makeReq(): Request { + return { + method: "POST", + originalUrl: "/api/issues/issue-1/interactions/interaction-1/accept", + body: {}, + params: {}, + query: {}, + } as unknown as Request; +} + +function makeRes(): Response { + const res = { status: vi.fn(), json: vi.fn() } as unknown as Response; + (res.status as unknown as ReturnType).mockReturnValue(res); + return res; +} + +function changedTargetError() { + return new ToolGatewayHttpError( + 409, + "Tool definition or connection changed; request a new review", + "approved_tool_target_changed", + ); +} + +describe("errorHandler with tool gateway errors", () => { + beforeEach(() => { + captureExceptionMock.mockReset(); + trackErrorHandlerCrashMock.mockReset(); + }); + + it("answers an expected gateway rejection with its own status and reason, without a crash report", () => { + const res = makeRes(); + + errorHandler(changedTargetError(), makeReq(), res, vi.fn()); + + expect(res.status).toHaveBeenCalledWith(409); + expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ + error: "Tool definition or connection changed; request a new review", + code: "approved_tool_target_changed", + })); + expect(captureExceptionMock).not.toHaveBeenCalled(); + expect(trackErrorHandlerCrashMock).not.toHaveBeenCalled(); + }); + + it("keeps gateway server failures on the crash path without exposing provider text", () => { + const error = new ToolGatewayHttpError(502, "provider said: secret detail", "mcp_remote_invalid_json"); + const res = makeRes(); + + errorHandler(error, makeReq(), res, vi.fn()); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ error: "Internal server error" }); + expect(captureExceptionMock).toHaveBeenCalledWith(error); + }); + + it("returns the 409 to a client whose route lets the gateway error escape", async () => { + const app = express(); + app.post("/api/issues/:id/interactions/:interactionId/accept", async () => { + throw changedTargetError(); + }); + app.use(errorHandler); + + const res = await request(app).post("/api/issues/issue-1/interactions/interaction-1/accept").send({}); + + expect(res.status).toBe(409); + expect(res.body).toMatchObject({ + error: "Tool definition or connection changed; request a new review", + code: "approved_tool_target_changed", + }); + expect(captureExceptionMock).not.toHaveBeenCalled(); + }); +}); diff --git a/server/src/middleware/error-handler.ts b/server/src/middleware/error-handler.ts index b4022b67c25..71058769f4c 100644 --- a/server/src/middleware/error-handler.ts +++ b/server/src/middleware/error-handler.ts @@ -16,6 +16,7 @@ import { import { recordResponsibleUserDenialOnActiveRun } from "../services/responsible-user-denial-run-outcomes.js"; import { isExpectedMcpConnectionFailure } from "../services/mcp-connection-failure.js"; import { isEndedDbPoolError } from "../database-retry.js"; +import { ToolGatewayHttpError } from "../services/tool-gateway.js"; export interface ErrorContext { error: { @@ -135,12 +136,27 @@ function recordResponsibleUserDenialFromHttpError( }); } +/** + * The tool gateway rejects requests with its own error type. A route that lets + * one reach this handler (such as approving a tool review from a task card) + * would otherwise turn an expected rejection, for example a 409 asking for a + * new review, into a 500 crash report. Gateway server failures stay on the + * crash path so their provider text is not exposed. + */ +function clientToolGatewayErrorAsHttpError(err: unknown): unknown { + if (!(err instanceof ToolGatewayHttpError) || err.status < 400 || err.status >= 500) return err; + const mapped = new HttpError(err.status, err.message, { ...err.details, code: err.reasonCode }); + mapped.stack = err.stack; + return mapped; +} + export function errorHandler( - err: unknown, + thrown: unknown, req: Request, res: Response, _next: NextFunction, ) { + const err = clientToolGatewayErrorAsHttpError(thrown); if (err instanceof AgentLifecycleConflict) { res.status(409).json({ error: err.message }); return; } if (err instanceof HttpError) { const details =