mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-23 17:33:51 +02:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Operators use Sentry to inspect run and server failures. > - Reports identify the run and task, but the existing links open API records. > - Operators need a direct path from an error to the normal task and run pages. > - This change adds navigation links through the shared server Sentry hook. > - Saved company and task records determine the links. A failed lookup preserves the error report. ## Linked Issues or Issue Description Related work: #16300 added saved run API references. **What existing behavior does this improve?** Sentry event context for run failures, Stop timeouts, and server errors from an authenticated agent run. **Current behavior** Reports show task and run UUIDs. The existing run URL opens an API record. The operator must find the normal task page separately. **Proposed behavior** Add `task_url`, `run_url`, and `task_identifier` tags when saved run metadata is available. Add `taskUrl` and `runPageUrl` to the event context. Use the company prefix and saved task identifier in normal board URLs. **Reason and benefit** An operator can open the task or run directly from the error report. ## What Changed - Add a shared Sentry hook for events with a known run identity. - Read only the saved company prefix, task identifier, and run identity. Keep the lookup within the recorded company and agent. - Include authenticated run identity on request errors. Use the existing run identity on Stop errors. - Bound optional lookups to one second and eight unfinished reads. Preserve the original event on failure. - Keep the existing API references and error grouping. - Document the fields and add SDK, timeout, and company-boundary coverage. ## Verification - Passed: `pnpm -r typecheck` and the final server TypeScript check. - Passed: `pnpm build`. - Passed: 332 tests across the affected server suites, plus 25 final link and real Sentry SDK tests after review fixes. The SDK test checks outgoing event envelopes for run failures, request errors, and Stop errors. It checks that unrelated errors receive no links. The database test verifies saved metadata and company boundaries. It passes with embedded PostgreSQL and with an explicitly configured disposable test database. - Passed in CI on `bec0cdac853efdef71041d59a66261941827653e`: the full test matrix, typecheck, build, Runner checks, browser suites, and the real Sentry SDK contract. All 56 active checks pass. Two Storybook jobs are skipped. - The full local `pnpm test:run` attempt first failed because PostgreSQL library links were missing. The package setup script repaired the install. The affected database suites then passed. The repaired full local run was stopped after all CI test jobs passed; it did not complete locally. - Greptile: 5/5 on the final commit. All three review threads are resolved. - Read-only browser inspection confirms that Sentry displays context URL values with an Open link control. - No live test event was posted. Validation uses the real SDK with an in-memory transport. ## Risks - A slow or unavailable database can prevent optional links. It does not discard the error. At most eight unfinished lookups can remain active. - Missing or unsafe public origins omit the links. URLs longer than the tag limit remain in context. - This change applies to newly captured events. It does not update existing Sentry events. - The change adds no schema, permission, or first-party Telemetry contract change. ## Model Used OpenAI Codex, `gpt-6.1-sol`, xhigh reasoning, with tool use and code execution. The exact context window is not exposed in the session metadata. ## 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 - [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 --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
396 lines
14 KiB
TypeScript
396 lines
14 KiB
TypeScript
import type { NextFunction, Request, Response } from "express";
|
|
import { beforeEach, describe, expect, it, vi } from "vitest";
|
|
import { HttpError } from "../errors.js";
|
|
import { errorHandler } from "../middleware/error-handler.js";
|
|
import { mcpDiscoveryHttpFailure, retainMcpConnectionFailure } from "../services/mcp-connection-failure.js";
|
|
|
|
const recordResponsibleUserDenialOnActiveRunMock = vi.hoisted(() => vi.fn());
|
|
const captureExceptionMock = vi.hoisted(() => vi.fn());
|
|
const telemetryMocks = vi.hoisted(() => ({
|
|
client: {},
|
|
trackErrorHandlerCrash: vi.fn(),
|
|
}));
|
|
|
|
vi.mock("../services/responsible-user-denial-run-outcomes.js", () => ({
|
|
recordResponsibleUserDenialOnActiveRun:
|
|
recordResponsibleUserDenialOnActiveRunMock,
|
|
}));
|
|
|
|
vi.mock("../sentry.js", () => ({ captureException: captureExceptionMock }));
|
|
vi.mock("../telemetry.js", () => ({
|
|
getTelemetryClient: () => telemetryMocks.client,
|
|
}));
|
|
vi.mock("@paperclipai/shared/telemetry", () => ({
|
|
trackErrorHandlerCrash: telemetryMocks.trackErrorHandlerCrash,
|
|
}));
|
|
|
|
function makeReq(): Request {
|
|
return {
|
|
method: "GET",
|
|
originalUrl: "/api/test",
|
|
body: { a: 1 },
|
|
params: { id: "123" },
|
|
query: { q: "x" },
|
|
} 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<typeof vi.fn>).mockReturnValue(res);
|
|
return res;
|
|
}
|
|
|
|
describe("errorHandler", () => {
|
|
beforeEach(() => {
|
|
recordResponsibleUserDenialOnActiveRunMock.mockReset();
|
|
recordResponsibleUserDenialOnActiveRunMock.mockResolvedValue(null);
|
|
captureExceptionMock.mockReset();
|
|
telemetryMocks.trackErrorHandlerCrash.mockReset();
|
|
});
|
|
|
|
it("passes only authenticated run identity to Sentry for a run-related server error", () => {
|
|
const req = makeReq();
|
|
req.actor = { type: "agent", runId: "run", companyId: "company", agentId: "agent" };
|
|
const error = new Error("run request failed");
|
|
errorHandler(error, req, makeRes(), vi.fn());
|
|
expect(captureExceptionMock).toHaveBeenCalledWith(error, { runId: "run", companyId: "company", agentId: "agent" });
|
|
expect(JSON.stringify(captureExceptionMock.mock.calls)).not.toContain('"a":1');
|
|
});
|
|
|
|
it("attaches the original Error to res.err for 500s", () => {
|
|
const req = makeReq();
|
|
const res = makeRes() as any;
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const err = new Error("boom");
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(500);
|
|
expect(res.json).toHaveBeenCalledWith({ error: "Internal server error" });
|
|
expect(res.err).toBe(err);
|
|
expect(res.__errorContext?.error?.message).toBe("boom");
|
|
});
|
|
|
|
it("ends aborted client requests without reporting a crash", () => {
|
|
// A closed tab or dropped network surfaces as `Error: aborted` with
|
|
// ECONNRESET; there is no server fault and nobody left to answer.
|
|
const req = makeReq();
|
|
const res = { ...makeRes(), end: vi.fn(), headersSent: false } as any;
|
|
(res.status as ReturnType<typeof vi.fn>).mockReturnValue(res);
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const err = Object.assign(new Error("aborted"), { code: "ECONNRESET" });
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(499);
|
|
expect(res.end).toHaveBeenCalled();
|
|
expect(res.json).not.toHaveBeenCalled();
|
|
expect(captureExceptionMock).not.toHaveBeenCalled();
|
|
expect(telemetryMocks.trackErrorHandlerCrash).not.toHaveBeenCalled();
|
|
});
|
|
|
|
function endedPoolQueryError() {
|
|
// Drizzle wraps the driver error; postgres.js uses CONNECTION_ENDED for a
|
|
// query issued after this process called `sql.end()`.
|
|
const driverError = Object.assign(new Error("write CONNECTION_ENDED db.example:5432"), {
|
|
code: "CONNECTION_ENDED",
|
|
});
|
|
return new Error('Failed query: select "id" from "issues"', { cause: driverError });
|
|
}
|
|
|
|
it("answers 503 without reporting a query that outlived the database shutdown", () => {
|
|
const req = { ...makeReq(), app: { locals: { paperclipDatabaseClosing: true } } } as unknown as Request;
|
|
const res = makeRes() as any;
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
|
|
errorHandler(endedPoolQueryError(), req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(503);
|
|
expect(res.json).toHaveBeenCalledWith({ error: "Server is shutting down" });
|
|
expect(res.err).toBeUndefined();
|
|
expect(captureExceptionMock).not.toHaveBeenCalled();
|
|
expect(telemetryMocks.trackErrorHandlerCrash).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("only ends the response when headers were already sent during database shutdown", () => {
|
|
const req = { ...makeReq(), app: { locals: { paperclipDatabaseClosing: true } } } as unknown as Request;
|
|
const res = { ...makeRes(), end: vi.fn(), headersSent: true } as any;
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
|
|
errorHandler(endedPoolQueryError(), req, res, next);
|
|
|
|
expect(res.status).not.toHaveBeenCalled();
|
|
expect(res.end).toHaveBeenCalled();
|
|
expect(captureExceptionMock).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("still reports an ended-pool query error outside database shutdown", () => {
|
|
const req = { ...makeReq(), app: { locals: {} } } as unknown as Request;
|
|
const res = makeRes() as any;
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const err = endedPoolQueryError();
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(500);
|
|
expect(captureExceptionMock).toHaveBeenCalledWith(err);
|
|
});
|
|
|
|
it("still reports other connection failures during database shutdown", () => {
|
|
const req = { ...makeReq(), app: { locals: { paperclipDatabaseClosing: true } } } as unknown as Request;
|
|
const res = makeRes() as any;
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const err = new Error("Failed query", {
|
|
cause: Object.assign(new Error("write CONNECTION_CLOSED"), { code: "CONNECTION_CLOSED" }),
|
|
});
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(500);
|
|
expect(captureExceptionMock).toHaveBeenCalledWith(err);
|
|
});
|
|
|
|
it("exposes raw 500 messages for trusted Cloud tenant imports", () => {
|
|
const req = {
|
|
...makeReq(),
|
|
method: "POST",
|
|
originalUrl: "/api/companies/import",
|
|
actor: {
|
|
type: "board",
|
|
userId: "cloud-user",
|
|
source: "cloud_tenant",
|
|
},
|
|
} as unknown as Request;
|
|
const res = makeRes() as any;
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const err = new Error("portable file references missing upload id");
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(500);
|
|
expect(res.json).toHaveBeenCalledWith({
|
|
error: "Internal server error",
|
|
message: "portable file references missing upload id",
|
|
});
|
|
expect(res.err).toBe(err);
|
|
});
|
|
|
|
it("attaches HttpError instances for 500 responses", () => {
|
|
const req = makeReq();
|
|
const res = makeRes() as any;
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const err = new HttpError(500, "db exploded");
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(500);
|
|
expect(res.json).toHaveBeenCalledWith({ error: "db exploded" });
|
|
expect(res.err).toBe(err);
|
|
expect(res.__errorContext?.error?.message).toBe("db exploded");
|
|
});
|
|
|
|
it("keeps HTTP failures and existing telemetry for proven external MCP outages while skipping Sentry", () => {
|
|
const req = makeReq();
|
|
const res = makeRes() as any;
|
|
const original = mcpDiscoveryHttpFailure(new Response(null, { status: 503 }), "Remote app returned HTTP 503");
|
|
const err = retainMcpConnectionFailure(original, new HttpError(502, original.message));
|
|
|
|
errorHandler(err, req, res, vi.fn());
|
|
|
|
expect(res.status).toHaveBeenCalledWith(502);
|
|
expect(res.json).toHaveBeenCalledWith({ error: original.message });
|
|
expect(res.err).toBe(err);
|
|
expect(res.__errorContext.error.message).toBe(original.message);
|
|
expect(telemetryMocks.trackErrorHandlerCrash).toHaveBeenCalledOnce();
|
|
expect(captureExceptionMock).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("reports copied connection markers and unrelated database failures", () => {
|
|
for (const err of [
|
|
Object.assign(new HttpError(502, "Remote app returned HTTP 503", { status: 503 }), {
|
|
connectionFailure: { schemaVersion: 1, provider: "mcp_http", operation: "discover_tools", reason: "remote_unavailable" },
|
|
}),
|
|
new HttpError(500, "database unavailable"),
|
|
]) {
|
|
errorHandler(err, makeReq(), makeRes(), vi.fn());
|
|
}
|
|
expect(captureExceptionMock).toHaveBeenCalledTimes(2);
|
|
expect(telemetryMocks.trackErrorHandlerCrash).toHaveBeenCalledTimes(2);
|
|
});
|
|
|
|
it("sanitizes chat setup errors before logs and crash reporting", () => {
|
|
const req = {
|
|
...makeReq(),
|
|
method: "POST",
|
|
originalUrl: "/api/chat-endpoints/endpoint-1/setup",
|
|
body: { credentials: { botToken: "setup-error-token-canary" } },
|
|
} as unknown as Request;
|
|
const res = makeRes() as any;
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const err = new Error("provider echoed setup-error-token-canary");
|
|
err.name = "SecretName-setup-error-token-canary";
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.err).not.toBe(err);
|
|
expect(res.err).toMatchObject({
|
|
name: "Error",
|
|
message: "Secret-sensitive request failed",
|
|
});
|
|
expect(res.__errorContext.error).toEqual({
|
|
name: "Error",
|
|
message: "Secret-sensitive request failed",
|
|
});
|
|
expect(captureExceptionMock).toHaveBeenCalledWith(res.err);
|
|
expect(JSON.stringify(captureExceptionMock.mock.calls)).not.toContain(
|
|
"setup-error-token-canary",
|
|
);
|
|
expect(telemetryMocks.trackErrorHandlerCrash).toHaveBeenCalledWith(
|
|
telemetryMocks.client,
|
|
{ errorCode: "Error" },
|
|
);
|
|
expect(
|
|
JSON.stringify(telemetryMocks.trackErrorHandlerCrash.mock.calls),
|
|
).not.toContain("setup-error-token-canary");
|
|
});
|
|
|
|
it("keeps actionable setup validation details while removing submitted credentials", () => {
|
|
const req = {
|
|
...makeReq(),
|
|
method: "POST",
|
|
originalUrl: "/api/chat-endpoints/endpoint-1/setup",
|
|
body: { credentials: { botToken: "invalid-token-canary" } },
|
|
} as unknown as Request;
|
|
const res = makeRes() as any;
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const err = new HttpError(
|
|
422,
|
|
"Missing required Slack scopes for invalid-token-canary",
|
|
{
|
|
code: "chat_provider_permissions_missing",
|
|
credentials: { botToken: "invalid-token-canary" },
|
|
explanation: "Provider rejected invalid-token-canary",
|
|
requiredScopes: ["chat:write", "reactions:write"],
|
|
},
|
|
);
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(422);
|
|
expect(res.json).toHaveBeenCalledWith({
|
|
error: "Missing required Slack scopes for [REDACTED]",
|
|
code: "chat_provider_permissions_missing",
|
|
details: {
|
|
code: "chat_provider_permissions_missing",
|
|
credentials: "[REDACTED]",
|
|
explanation: "Provider rejected [REDACTED]",
|
|
requiredScopes: ["chat:write", "reactions:write"],
|
|
},
|
|
});
|
|
});
|
|
|
|
it("returns 400 for Zod validation errors from another module instance", () => {
|
|
const req = makeReq();
|
|
const res = makeRes() as any;
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const issue = {
|
|
code: "invalid_type",
|
|
expected: "string",
|
|
received: "undefined",
|
|
path: ["provider"],
|
|
message: "Required",
|
|
};
|
|
const err = Object.assign(new Error("Validation failed"), {
|
|
name: "ZodError",
|
|
issues: [issue],
|
|
errors: [issue],
|
|
});
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(400);
|
|
expect(res.json).toHaveBeenCalledWith({
|
|
error: "Validation error",
|
|
details: [issue],
|
|
});
|
|
expect(res.err).toBeUndefined();
|
|
expect(res.__errorContext).toBeUndefined();
|
|
});
|
|
|
|
it("removes submitted credentials from setup Zod issue prose", () => {
|
|
const req = {
|
|
...makeReq(),
|
|
method: "POST",
|
|
originalUrl: "/api/chat-endpoints/endpoint-1/setup",
|
|
body: { credentials: { botToken: "zod-token-canary" } },
|
|
} as unknown as Request;
|
|
const res = makeRes() as any;
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const issue = {
|
|
code: "custom",
|
|
path: ["credentials", "botToken"],
|
|
message: "Rejected zod-token-canary",
|
|
};
|
|
const err = Object.assign(new Error("Validation failed"), {
|
|
name: "ZodError",
|
|
issues: [issue],
|
|
errors: [issue],
|
|
});
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(400);
|
|
expect(res.json).toHaveBeenCalledWith({
|
|
error: "Validation error",
|
|
details: [
|
|
{
|
|
code: "custom",
|
|
path: ["credentials", "botToken"],
|
|
message: "Rejected [REDACTED]",
|
|
},
|
|
],
|
|
});
|
|
});
|
|
|
|
it("records responsible-user denial codes on the active agent run", () => {
|
|
const db = { marker: "db" };
|
|
const req = {
|
|
...makeReq(),
|
|
app: { locals: { paperclipDb: db } },
|
|
actor: {
|
|
type: "agent",
|
|
agentId: "agent-1",
|
|
companyId: "company-1",
|
|
runId: "run-1",
|
|
source: "agent_jwt",
|
|
},
|
|
} as unknown as Request;
|
|
const res = makeRes();
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const err = new HttpError(403, "Responsible user is not authorized", {
|
|
code: "RESPONSIBLE_USER_UNAUTHORIZED",
|
|
});
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(403);
|
|
expect(res.json).toHaveBeenCalledWith({
|
|
error: "Responsible user is not authorized",
|
|
code: "RESPONSIBLE_USER_UNAUTHORIZED",
|
|
details: { code: "RESPONSIBLE_USER_UNAUTHORIZED" },
|
|
});
|
|
expect(recordResponsibleUserDenialOnActiveRunMock).toHaveBeenCalledWith(
|
|
db,
|
|
{
|
|
runId: "run-1",
|
|
agentId: "agent-1",
|
|
companyId: "company-1",
|
|
code: "RESPONSIBLE_USER_UNAUTHORIZED",
|
|
},
|
|
);
|
|
});
|
|
});
|