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 =