mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 01:57:31 +01:00
## 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 <noreply@anthropic.com>
345 lines
12 KiB
TypeScript
345 lines
12 KiB
TypeScript
import { AgentLifecycleConflict } from "../modules/agent-lifecycle/index.js";
|
|
import type { Request, Response, NextFunction } from "express";
|
|
import type { Db } from "@paperclipai/db";
|
|
import { ZodError } from "zod";
|
|
import { HttpError } from "../errors.js";
|
|
import { trackErrorHandlerCrash } from "@paperclipai/shared/telemetry";
|
|
import { getTelemetryClient } from "../telemetry.js";
|
|
import { captureException } from "../sentry.js";
|
|
import { COMPANY_IMPORT_API_PATH } from "../routes/company-import-paths.js";
|
|
import { logger } from "./logger.js";
|
|
import { isSecretSensitiveHttpRequest } from "./http-log-policy.js";
|
|
import {
|
|
collectSensitiveStringValues,
|
|
redactSensitiveValueOccurrences,
|
|
} from "./redact-sensitive.js";
|
|
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: {
|
|
message: string;
|
|
stack?: string;
|
|
name?: string;
|
|
details?: unknown;
|
|
raw?: unknown;
|
|
};
|
|
method: string;
|
|
url: string;
|
|
reqBody?: unknown;
|
|
reqParams?: unknown;
|
|
reqQuery?: unknown;
|
|
}
|
|
|
|
function isRedactedSkillPolicyDenial(details: Record<string, unknown> | null) {
|
|
return details?.code === "skill_policy_denied";
|
|
}
|
|
|
|
function readZodIssues(err: unknown): unknown[] | null {
|
|
if (err instanceof ZodError) return err.issues;
|
|
if (
|
|
!err ||
|
|
typeof err !== "object" ||
|
|
(err as { name?: unknown }).name !== "ZodError"
|
|
)
|
|
return null;
|
|
const issues = (err as { issues?: unknown }).issues;
|
|
return Array.isArray(issues) ? issues : null;
|
|
}
|
|
|
|
function attachErrorContext(
|
|
req: Request,
|
|
res: Response,
|
|
payload: ErrorContext["error"],
|
|
rawError?: Error,
|
|
) {
|
|
(res as any).__errorContext = {
|
|
error: payload,
|
|
method: req.method,
|
|
url: req.originalUrl,
|
|
reqBody: req.body,
|
|
reqParams: req.params,
|
|
reqQuery: req.query,
|
|
} satisfies ErrorContext;
|
|
if (rawError) {
|
|
(res as any).err = rawError;
|
|
}
|
|
}
|
|
|
|
function sanitizeSecretSensitiveError(req: Request, error: Error): Error {
|
|
if (!isSecretSensitiveHttpRequest(req.method, req.originalUrl)) return error;
|
|
const sanitized = new Error("Secret-sensitive request failed");
|
|
// Both `name` and `message` are attacker/provider-controlled properties on
|
|
// JavaScript errors. Do not preserve either on a credential-bearing route.
|
|
sanitized.name = "Error";
|
|
return sanitized;
|
|
}
|
|
|
|
function sanitizeSecretSensitiveResponse(
|
|
req: Request,
|
|
value: unknown,
|
|
): unknown {
|
|
if (!isSecretSensitiveHttpRequest(req.method, req.originalUrl)) return value;
|
|
return redactSensitiveValueOccurrences(
|
|
value,
|
|
collectSensitiveStringValues(req.body),
|
|
);
|
|
}
|
|
|
|
/** Report a server-side crash to every error sink. */
|
|
function reportCrash(error: Error, req: Request, reportToSentry = true): void {
|
|
const tc = getTelemetryClient();
|
|
if (tc) trackErrorHandlerCrash(tc, { errorCode: error.name });
|
|
if (reportToSentry) {
|
|
if (req.actor?.type === "agent" && req.actor.runId && req.actor.companyId && req.actor.agentId) {
|
|
captureException(error, { runId: req.actor.runId, companyId: req.actor.companyId, agentId: req.actor.agentId });
|
|
} else captureException(error);
|
|
}
|
|
}
|
|
|
|
/** Set by the orderly shutdown just before it ends the database pools. */
|
|
function isDatabaseClosing(req: Request): boolean {
|
|
const locals = req.app?.locals as { paperclipDatabaseClosing?: boolean } | undefined;
|
|
return locals?.paperclipDatabaseClosing === true;
|
|
}
|
|
|
|
function getPaperclipDb(req: Request): Db | null {
|
|
const locals = req.app?.locals as { paperclipDb?: Db; db?: Db } | undefined;
|
|
return locals?.paperclipDb ?? locals?.db ?? null;
|
|
}
|
|
|
|
function recordResponsibleUserDenialFromHttpError(
|
|
req: Request,
|
|
details: Record<string, unknown> | null,
|
|
) {
|
|
if (req.actor?.type !== "agent") return;
|
|
const db = getPaperclipDb(req);
|
|
if (!db) return;
|
|
|
|
void recordResponsibleUserDenialOnActiveRun(db, {
|
|
runId: req.actor.runId ?? null,
|
|
agentId: req.actor.agentId ?? null,
|
|
companyId: req.actor.companyId ?? null,
|
|
code: details?.code,
|
|
}).catch((recordErr) => {
|
|
logger.warn(
|
|
{
|
|
err: recordErr,
|
|
runId: req.actor?.runId ?? null,
|
|
agentId:
|
|
req.actor?.type === "agent" ? (req.actor.agentId ?? null) : null,
|
|
},
|
|
"failed to record responsible-user denial on heartbeat run",
|
|
);
|
|
});
|
|
}
|
|
|
|
/**
|
|
* 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(
|
|
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 =
|
|
err.details &&
|
|
typeof err.details === "object" &&
|
|
!Array.isArray(err.details)
|
|
? (err.details as Record<string, unknown>)
|
|
: null;
|
|
const redactedSkillPolicyDenial = isRedactedSkillPolicyDenial(details);
|
|
const workspaceRepairPreconditionFailure =
|
|
details?.code === "workspace_repair_precondition_failed";
|
|
const structuredConnectionError = new Set([
|
|
"user_authorization_required",
|
|
"organization_authorization_required",
|
|
"grant_audience_denied",
|
|
"grant_revoked",
|
|
"needs_reauthorization",
|
|
"installation_required",
|
|
"connection_not_installed",
|
|
"subject_not_permitted",
|
|
"standing_delegation_required",
|
|
"grant_owner_membership_inactive",
|
|
]).has(typeof details?.code === "string" ? details.code : "");
|
|
const responseDetailsValue = sanitizeSecretSensitiveResponse(
|
|
req,
|
|
err.details,
|
|
);
|
|
const responseDetails =
|
|
responseDetailsValue &&
|
|
typeof responseDetailsValue === "object" &&
|
|
!Array.isArray(responseDetailsValue)
|
|
? (responseDetailsValue as Record<string, unknown>)
|
|
: null;
|
|
recordResponsibleUserDenialFromHttpError(req, details);
|
|
if (err.status >= 500) {
|
|
const reportableError = sanitizeSecretSensitiveError(req, err);
|
|
attachErrorContext(
|
|
req,
|
|
res,
|
|
isSecretSensitiveHttpRequest(req.method, req.originalUrl)
|
|
? { message: reportableError.message, name: reportableError.name }
|
|
: {
|
|
message: err.message,
|
|
stack: err.stack,
|
|
name: err.name,
|
|
details: err.details,
|
|
},
|
|
reportableError,
|
|
);
|
|
reportCrash(reportableError, req, !isExpectedMcpConnectionFailure(err));
|
|
}
|
|
const secretSensitiveServerError =
|
|
err.status >= 500 &&
|
|
isSecretSensitiveHttpRequest(req.method, req.originalUrl);
|
|
res.status(err.status).json(
|
|
secretSensitiveServerError
|
|
? { error: "Internal server error" }
|
|
: {
|
|
error: sanitizeSecretSensitiveResponse(req, err.message),
|
|
...(typeof responseDetails?.code === "string"
|
|
? { code: responseDetails.code }
|
|
: {}),
|
|
...(redactedSkillPolicyDenial &&
|
|
typeof responseDetails?.reason === "string"
|
|
? { reason: responseDetails.reason }
|
|
: {}),
|
|
...(workspaceRepairPreconditionFailure &&
|
|
typeof responseDetails?.reason === "string"
|
|
? { reason: responseDetails.reason }
|
|
: {}),
|
|
...(workspaceRepairPreconditionFailure &&
|
|
typeof responseDetails?.repairPhase === "string"
|
|
? { repairPhase: responseDetails.repairPhase }
|
|
: {}),
|
|
...(typeof responseDetails?.remediation === "string" ||
|
|
(structuredConnectionError &&
|
|
responseDetails?.remediation &&
|
|
typeof responseDetails.remediation === "object")
|
|
? { remediation: responseDetails.remediation }
|
|
: {}),
|
|
...(structuredConnectionError && responseDetails?.connection
|
|
? { connection: responseDetails.connection }
|
|
: {}),
|
|
...(structuredConnectionError && responseDetails?.subject
|
|
? { subject: responseDetails.subject }
|
|
: {}),
|
|
...(structuredConnectionError &&
|
|
typeof responseDetails?.grantId === "string"
|
|
? { grantId: responseDetails.grantId }
|
|
: {}),
|
|
...(!redactedSkillPolicyDenial &&
|
|
!workspaceRepairPreconditionFailure &&
|
|
responseDetailsValue
|
|
? { details: responseDetailsValue }
|
|
: {}),
|
|
},
|
|
);
|
|
return;
|
|
}
|
|
|
|
const zodIssues = readZodIssues(err);
|
|
if (zodIssues) {
|
|
res.status(400).json({
|
|
error: "Validation error",
|
|
details: sanitizeSecretSensitiveResponse(req, zodIssues),
|
|
});
|
|
return;
|
|
}
|
|
|
|
// Only body-parser's malformed-JSON errors are client input failures.
|
|
// Parser messages can quote request bytes; return a constant response and
|
|
// keep the raw error out of crash reporting and HTTP error context.
|
|
if (
|
|
err instanceof SyntaxError &&
|
|
"status" in err && err.status === 400 &&
|
|
"type" in err && err.type === "entity.parse.failed"
|
|
) {
|
|
res.status(400).json({ error: "Invalid JSON body" });
|
|
return;
|
|
}
|
|
|
|
const rootError = err instanceof Error ? err : new Error(String(err));
|
|
|
|
// The client tore down the connection mid-request (closed tab, dropped
|
|
// mobile network, cancelled upload): Node surfaces it as `Error: aborted`
|
|
// with ECONNRESET. There is no server fault to report and nobody left to
|
|
// answer, so skip the error sinks and just close out the response.
|
|
if (
|
|
rootError.message === "aborted" &&
|
|
(rootError as NodeJS.ErrnoException).code === "ECONNRESET"
|
|
) {
|
|
if (!res.headersSent) res.status(499);
|
|
res.end();
|
|
return;
|
|
}
|
|
|
|
// Shutdown drains the HTTP listener before it ends the pool, but a handler
|
|
// whose client already disconnected (or whose socket the drain timeout
|
|
// closed) keeps running. Its next query then fails with CONNECTION_ENDED
|
|
// against the pool this process deliberately ended. That is shutdown, not a
|
|
// server fault, so skip the error sinks. Any other error during shutdown is
|
|
// still reported.
|
|
if (isDatabaseClosing(req) && isEndedDbPoolError(rootError)) {
|
|
if (!res.headersSent) {
|
|
res.status(503).json({ error: "Server is shutting down" });
|
|
} else {
|
|
res.end();
|
|
}
|
|
return;
|
|
}
|
|
|
|
const reportableError = sanitizeSecretSensitiveError(req, rootError);
|
|
attachErrorContext(
|
|
req,
|
|
res,
|
|
isSecretSensitiveHttpRequest(req.method, req.originalUrl)
|
|
? { message: reportableError.message, name: reportableError.name }
|
|
: err instanceof Error
|
|
? { message: err.message, stack: err.stack, name: err.name }
|
|
: {
|
|
message: String(err),
|
|
raw: err,
|
|
stack: rootError.stack,
|
|
name: rootError.name,
|
|
},
|
|
reportableError,
|
|
);
|
|
|
|
reportCrash(reportableError, req);
|
|
|
|
res.status(500).json({
|
|
error: "Internal server error",
|
|
...(shouldExposeTrustedCloudTenantImportError(req)
|
|
? { message: rootError.message }
|
|
: {}),
|
|
});
|
|
}
|
|
|
|
function shouldExposeTrustedCloudTenantImportError(req: Request) {
|
|
return (
|
|
req.actor?.source === "cloud_tenant" &&
|
|
req.method === "POST" &&
|
|
req.originalUrl.split("?")[0] === COMPANY_IMPORT_API_PATH
|
|
);
|
|
}
|