diff --git a/README.md b/README.md index 5f19d11df6..80ce5ce44f 100644 --- a/README.md +++ b/README.md @@ -469,6 +469,8 @@ Find Plugins and more at [awesome-paperclip](https://github.com/gsxdsm/awesome-p Paperclip ships with opt-in OpenTelemetry auto-instrumentation for the server (traces only). It activates when `OTEL_EXPORTER_OTLP_ENDPOINT` is set and supports `grpc`, `http/protobuf`, and `http/json` via the standard `OTEL_EXPORTER_OTLP_PROTOCOL` env var. The `@opentelemetry/*` packages are optional peer dependencies — install them only if you want tracing. See [doc/observability.md](doc/observability.md) for install commands and the full env-var reference. +Paperclip also ships with opt-in Sentry error monitoring for the server and the browser. Set `SENTRY_DSN` to activate it — the server and the browser then report to the same Sentry project. `@sentry/node` is an optional peer dependency for the server; install it only if you want error monitoring. See [doc/observability.md](doc/observability.md#sentry-error-monitoring) for the install command, the privacy settings, and the full default capture set. + ## Telemetry Paperclip collects anonymous usage telemetry to help us understand how the product is used and improve it. No personal information, issue content, prompts, file paths, or secrets are ever collected. Private repository references are hashed with a per-install salt before being sent. diff --git a/doc/observability.md b/doc/observability.md index 02ddef5f4b..db3d8bea26 100644 --- a/doc/observability.md +++ b/doc/observability.md @@ -1,7 +1,8 @@ # Observability This document is the Observability contract. It covers the OpenTelemetry -trace path and two local instrumentation contracts; see the +trace path, the opt-in Sentry error-monitoring path, and two local +instrumentation contracts; see the [Telemetry Data Contract](../packages/shared/src/telemetry/README.md) for the separate first-party event system. @@ -109,6 +110,206 @@ This document also holds two local instrumentation contracts: the sandbox startup trace spans, and the sandbox duplex transport instrumentation. Both sections follow below. +## Sentry Error Monitoring + +Paperclip ships with **opt-in** Sentry error monitoring for the server +process and the browser app. The operator activates it with one +environment variable, `SENTRY_DSN`. The server and the browser both read +this same value, so both report to **one** Sentry project. The feature +uses built-in Sentry options only. It adds no `beforeSend` hook and no +custom filter code. + +When `SENTRY_DSN` is unset, the feature is fully inactive. The server +imports no Sentry package. The browser fetches no Sentry chunk. + +### Enabling Sentry + +#### 1. Install the Sentry peer dependency + +Install `@sentry/node` in the server, the same way you install the +OpenTelemetry packages above. `@sentry/node` is an *optional peer +dependency*: it is not in the default lockfile, and the server loads it +dynamically only when `SENTRY_DSN` is set. + +```bash +pnpm add @sentry/node +``` + +The browser package, `@sentry/browser`, needs no install step. It is +already a development dependency of the `ui` package, so the browser code +ships inside every build. A signed-out browser, or a browser with no DSN, +never fetches the Sentry chunk — see "DSN delivery to the browser" below. + +#### 2. Set the environment + +```bash +export SENTRY_DSN="https://@/" +``` + +No other variable is needed. + +### One Sentry project + +The server and the browser report to **one** Sentry project, because both +read the same `SENTRY_DSN` value. The server reads it from the process +environment. The browser reads it from the authenticated +`GET /api/auth/get-session` response. + +### DSN delivery to the browser + +The browser never reads the DSN from a `` tag or from any other part +of `index.html`. The served `index.html` holds no DSN — it is a static +file, built once and served unchanged to every request. + +Instead, the browser receives the DSN inside the authenticated +`GET /api/auth/get-session` response body, next to the signed-in session +and the user profile. A signed-out browser calls this route with no board +actor, so the route answers 401 and sends no DSN. A signed-out browser +therefore loads no Sentry chunk and sends no event. These pages run +signed out: + +- `/auth` +- `/cli-auth/:id` +- `/board-claim/:token` +- `/invite/:token` + +**A gap the operator must know:** a browser error that happens before the +session response arrives is not captured. The gate opens only after the +session query resolves. + +### Privacy settings + +The feature uses built-in Sentry options only. + +- `sendDefaultPii` is `false`, on both runtimes. +- `tracesSampleRate` is `0`, on both runtimes. Paperclip sends no + performance trace and no profile. +- There is no `beforeSend` hook and no custom filter, on either runtime. + +### Server request data + +**A server event carries no request data at all.** It holds no URL, no +method, no header, no cookie, no query string, and no body. This is a +verified result, not the Sentry SDK's documented default. A live test +against the real `@sentry/node@10.71.0` package proves it: it captures an +event from inside a real HTTP request handler and confirms the event +holds no request field (see `server/src/__tests__/sentry.test.ts`, "a +server event captured inside a real HTTP request handler carries no +request field"). + +The reason is `skipOpenTelemetrySetup: true`. This feature sets that +option so it never fights Paperclip's separate, independently opt-in +OpenTelemetry feature for control of the global tracer. The same option +turns off Sentry's per-request context tracking. Sentry's built-in +`RequestData` integration needs that tracking to find a URL, a method, a +header set, a cookie set, or a query string to attach. `RequestData` +stays in the integration list — the initializer does not remove it — but +it attaches nothing under this configuration. + +This holds even when the operator turns on the separate OpenTelemetry +feature too (`OTEL_EXPORTER_OTLP_ENDPOINT` set). A live test with a real +OpenTelemetry SDK, a real HTTP instrumentation package, and a real +async-context manager registered still shows no request field on the +captured event. + +A server event carries only the exception, its stack trace, and the +context the other kept default integrations add: the host name, the +runtime version, and the dependency list. See "Default capture set" +below. + +### Browser data + +The browser sends no page URL, no referrer, no user agent, and no +breadcrumb. + +### Fail-open behavior + +A failed Sentry import or a failed init never stops the server and never +breaks the browser app. Both runtimes fall through to a single diagnostic +log line and keep running with no error monitoring. + +### Default capture set + +The lists below name every event and every context field this feature +sends, so an operator can read what the feature does before turning it on. +Each Sentry integration name below is verified against the default +integration list of `@sentry/node@10.71.0` and `@sentry/browser@10.71.0`. + +**Server events this feature adds** + +- An Express `HttpError` with `status >= 500`. +- Any unknown throw that is not a `ZodError`. It always answers 500. +- A server startup failure. + +**Server events the default integrations add** + +- `OnUncaughtException` — each uncaught exception on the main thread, at + level `fatal`. The process still exits. +- `OnUnhandledRejection` — each unhandled promise rejection. The mode is + `strict`, so the process exits after the capture. +- `ChildProcess` — one event for each worker-thread `error`. +- `LinkedErrors` — the `error.cause` chain of each captured error. + +**Server context the kept integrations attach** + +- `RequestData` — attaches nothing under this feature's configuration. See + "Server request data" above for the verified reason. +- `ChildProcess` — a non-zero child-process exit becomes a breadcrumb. +- `Modules` and `Context` — the dependency list, the host name, the + operating system, and the runtime version. +- `ProcessSession` — one release-health session for each process. +- `LocalVariablesAsync` — off. It needs `includeLocalVariables: true`, + which this feature omits. +- `NodeSystemError` — a Node system error (for example, `ENOENT`) gets a + `node_system_error` context field with its error code. The `path` and + `dest` fields are removed by default. + +**Server sources this feature removes** + +- `Console` — raw `console.*` arguments. +- `ContextLines` — 7 local source lines around each stack frame. +- The outbound breadcrumb of `Http` — outbound request URLs and query + strings. + +**Browser events this feature adds** + +- A crash in the application error boundary and a crash in the route + error boundary. + +**Browser events and context the kept integrations add** + +- `GlobalHandlers` — `window.onerror` and `window.onunhandledrejection`. +- `BrowserApiErrors` — a throw inside `setTimeout`, `setInterval`, + `requestAnimationFrame`, and an event listener. +- `CultureContext` — the locale and the timezone. +- `Dedupe`, `LinkedErrors`, and `BrowserSession`. + +**Browser sources this feature removes** + +- `HttpContext` — the page URL, the referrer, and the user agent. +- `Breadcrumbs` — console output, a click and a keypress target, a + `fetch` and an `XHR` request URL, and history navigation. + +**Not captured on either runtime** + +- A Zod validation error, which answers 400. +- Each `HttpError` below status 500, such as 401, 403, 404, 409, and 422. +- A performance trace and a profile, because `tracesSampleRate` is 0. + +### Operator responsibilities + +Two controls belong to the operator. This feature ships neither one. + +1. **Set a rate limit and a quota alert.** Set a per-client-key ingestion + rate limit and a quota alert in the Sentry project. The feature sends + no built-in rate limit of its own. +2. **Give a self-hosted sink a reachable host name.** If `SENTRY_DSN` + points at a self-hosted Sentry instance, give it an externally + reachable ingest host name, not an internal-only host name. The + browser sends its events from the operator's network, not from the + server's network, so an internal-only host name fails silently for + the browser even when it works for the server. + ## Sandbox Startup Trace Spans Paperclip opens OpenTelemetry spans on the sandbox start path. These spans are diff --git a/packages/shared/src/validators/access.test.ts b/packages/shared/src/validators/access.test.ts index b8d2ffcba4..32ae79444f 100644 --- a/packages/shared/src/validators/access.test.ts +++ b/packages/shared/src/validators/access.test.ts @@ -106,6 +106,7 @@ describe("authSessionSchema", () => { const result = authSessionSchema.safeParse({ session: { id: "s1", userId: "u1" }, user: { id: "u1", email: "a@b.com", name: "", image: null }, + sentryDsn: null, }); expect(result.success).toBe(true); expect(result.success && result.data.user.name).toBe(null); @@ -115,6 +116,7 @@ describe("authSessionSchema", () => { const result = authSessionSchema.safeParse({ session: { id: "s1", userId: "u1" }, user: { id: "u1", email: "a@b.com", name: "Jane", image: null }, + sentryDsn: null, }); expect(result.success).toBe(true); expect(result.success && result.data.user.name).toBe("Jane"); @@ -124,6 +126,7 @@ describe("authSessionSchema", () => { const result = authSessionSchema.safeParse({ session: { id: "s1", userId: "u1" }, user: { id: "u1", email: "a@b.com", name: null, image: null }, + sentryDsn: null, }); expect(result.success).toBe(true); expect(result.success && result.data.user.name).toBe(null); @@ -133,8 +136,37 @@ describe("authSessionSchema", () => { const result = authSessionSchema.safeParse({ session: { id: "s1", userId: "u1" }, user: { id: "u1", email: "", name: "Jane", image: null }, + sentryDsn: null, }); expect(result.success).toBe(true); expect(result.success && result.data.user.email).toBe(null); }); + + it("rejects a payload with no sentryDsn field", () => { + const result = authSessionSchema.safeParse({ + session: { id: "s1", userId: "u1" }, + user: { id: "u1", email: "a@b.com", name: "Jane", image: null }, + }); + expect(result.success).toBe(false); + }); + + it("accepts a null sentryDsn", () => { + const result = authSessionSchema.safeParse({ + session: { id: "s1", userId: "u1" }, + user: { id: "u1", email: "a@b.com", name: "Jane", image: null }, + sentryDsn: null, + }); + expect(result.success).toBe(true); + expect(result.success && result.data.sentryDsn).toBe(null); + }); + + it("accepts a real sentryDsn value", () => { + const result = authSessionSchema.safeParse({ + session: { id: "s1", userId: "u1" }, + user: { id: "u1", email: "a@b.com", name: "Jane", image: null }, + sentryDsn: "https://public@o0.ingest.sentry.io/1", + }); + expect(result.success).toBe(true); + expect(result.success && result.data.sentryDsn).toBe("https://public@o0.ingest.sentry.io/1"); + }); }); diff --git a/packages/shared/src/validators/access.ts b/packages/shared/src/validators/access.ts index cee3e5567c..d9f7f39d48 100644 --- a/packages/shared/src/validators/access.ts +++ b/packages/shared/src/validators/access.ts @@ -198,6 +198,12 @@ export const authSessionSchema = z.object({ userId: z.string().min(1), }), user: currentUserProfileSchema, + // The Sentry DSN for the current instance, or `null` when the operator has + // not set `SENTRY_DSN`. Required, not optional: a missing value must fail + // the response schema instead of silently disabling browser error + // monitoring. The browser reads this value to open its own Sentry gate — + // see `ui/src/lib/sentry.ts`. + sentryDsn: z.string().min(1).nullable(), }); export type AuthSession = z.infer; diff --git a/server/src/__tests__/auth-routes.test.ts b/server/src/__tests__/auth-routes.test.ts index 5b08b978b3..549f67618e 100644 --- a/server/src/__tests__/auth-routes.test.ts +++ b/server/src/__tests__/auth-routes.test.ts @@ -1,6 +1,6 @@ import express from "express"; import request from "supertest"; -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it } from "vitest"; import { errorHandler } from "../middleware/index.js"; import { authRoutes } from "../routes/auth.js"; @@ -58,8 +58,15 @@ describe.sequential("auth routes", () => { email: "jane@example.com", image: "https://example.com/jane.png", }; + const originalSentryDsn = process.env.SENTRY_DSN; + + afterEach(() => { + if (originalSentryDsn === undefined) delete process.env.SENTRY_DSN; + else process.env.SENTRY_DSN = originalSentryDsn; + }); it("returns the persisted user profile in the session payload", async () => { + delete process.env.SENTRY_DSN; const app = await createApp( { type: "board", @@ -78,9 +85,60 @@ describe.sequential("auth routes", () => { userId: "user-1", }, user: baseUser, + sentryDsn: null, }); }); + it("sends sentryDsn for a board actor when SENTRY_DSN is set", async () => { + process.env.SENTRY_DSN = "https://public@o0.ingest.sentry.io/1"; + const app = await createApp( + { + type: "board", + userId: "user-1", + source: "session", + }, + baseUser, + ); + + const res = await request(app).get("/api/auth/get-session"); + + expect(res.status).toBe(200); + expect(res.body.sentryDsn).toBe("https://public@o0.ingest.sentry.io/1"); + }); + + it("sends a null sentryDsn when SENTRY_DSN is unset", async () => { + delete process.env.SENTRY_DSN; + const app = await createApp( + { + type: "board", + userId: "user-1", + source: "session", + }, + baseUser, + ); + + const res = await request(app).get("/api/auth/get-session"); + + expect(res.status).toBe(200); + expect(res.body.sentryDsn).toBe(null); + }); + + it("answers 401 and sends no DSN when the actor type is none", async () => { + process.env.SENTRY_DSN = "https://public@o0.ingest.sentry.io/1"; + const app = await createApp( + { + type: "none", + source: "none", + }, + baseUser, + ); + + const res = await request(app).get("/api/auth/get-session"); + + expect(res.status).toBe(401); + expect(res.body.sentryDsn).toBeUndefined(); + }); + it("updates the signed-in profile", async () => { const app = await createApp( { diff --git a/server/src/__tests__/heartbeat-process-recovery.test.ts b/server/src/__tests__/heartbeat-process-recovery.test.ts index bd1136990d..174f086b56 100644 --- a/server/src/__tests__/heartbeat-process-recovery.test.ts +++ b/server/src/__tests__/heartbeat-process-recovery.test.ts @@ -5640,18 +5640,27 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { recoveryCause: "execution_review_participant_recovery", }); - const comments = await db.select().from(issueComments).where(eq(issueComments.issueId, issueId)); - const recoveryComment = comments.find((comment) => - comment.body.includes("pending execution-review participant once") && - noticeMetadataReferencesRecoveryAction(comment.metadata, recoveryAction.id), - ); + // The source issue flips to "blocked" before the recovery service posts + // its escalation comment and writes the activity-log event, so a read + // right after the status check can race an in-flight write. Poll for + // each row instead of reading once. + const recoveryComment = await waitForValue(async () => { + const comments = await db.select().from(issueComments).where(eq(issueComments.issueId, issueId)); + return comments.find((comment) => + comment.body.includes("pending execution-review participant once") && + noticeMetadataReferencesRecoveryAction(comment.metadata, recoveryAction.id), + ) ?? null; + }); expect(recoveryComment).toBeTruthy(); - const activity = await db.select().from(activityLog).where(eq(activityLog.entityId, issueId)); - expect(activity.some((event) => - (event.details as Record | null)?.source === - "recovery.reconcile_execution_review_participant", - )).toBe(true); + const recoveryActivityEvent = await waitForValue(async () => { + const activity = await db.select().from(activityLog).where(eq(activityLog.entityId, issueId)); + return activity.find((event) => + (event.details as Record | null)?.source === + "recovery.reconcile_execution_review_participant", + ) ?? null; + }); + expect(recoveryActivityEvent).toBeTruthy(); }); it("blocks failed execution-review recovery under the reviewer when the source assignee differs", async () => { diff --git a/server/src/__tests__/sentry.test.ts b/server/src/__tests__/sentry.test.ts new file mode 100644 index 0000000000..2e7ab88390 --- /dev/null +++ b/server/src/__tests__/sentry.test.ts @@ -0,0 +1,561 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createRequire } from "node:module"; +import http from "node:http"; +import express from "express"; +import request from "supertest"; +import type { NextFunction, Request, Response } from "express"; +import { HttpError } from "../errors.js"; +import { errorHandler } from "../middleware/error-handler.js"; +import { finalizeServerShutdown } from "../shutdown.js"; +import { authRoutes } from "../routes/auth.js"; +import * as sentryModule from "../sentry.js"; + +/** + * Tests for the opt-in Sentry error-monitoring gate. `@sentry/node` is an + * optional runtime dependency and is NOT installed in CI, which is itself + * part of the contract under test: with `SENTRY_DSN` set and the package + * absent, the module must warn and settle instead of crashing the server. + * + * The module reads `SENTRY_DSN` at import time, so each test resets the + * module registry and imports a fresh copy. + */ + +const DSN_ENV = "SENTRY_DSN"; +const originalDsn = process.env[DSN_ENV]; + +async function importFreshSentry() { + vi.resetModules(); + return await import("../sentry.js"); +} + +/** + * Register a fake `@sentry/node` module for the next dynamic import. Each + * mock function is returned so a test can assert on the call it received. + * The mock stays in place until `vi.doUnmock` runs, so `afterEach` clears it. + */ +function mockSentryPackage() { + const init = vi.fn(); + const captureException = vi.fn(() => "event-id"); + const close = vi.fn(async () => true); + const httpIntegration = vi.fn((options: unknown) => ({ name: "Http", ...(options as object) })); + const onUnhandledRejectionIntegration = vi.fn((options: unknown) => ({ + name: "OnUnhandledRejection", + ...(options as object), + })); + + vi.doMock("@sentry/node", () => ({ + init, + captureException, + close, + httpIntegration, + onUnhandledRejectionIntegration, + })); + + return { init, captureException, close, httpIntegration, onUnhandledRejectionIntegration }; +} + +// A representative default-integration list, shaped like the array +// `@sentry/node@10.71.0`'s `getDefaultIntegrations()` returns for a Node +// server. Recorded against the published package with: +// node -e "const S=require('@sentry/node'); \ +// console.log(S.getDefaultIntegrations({}).map(i=>i.name))" +const DEFAULT_INTEGRATION_NAMES = [ + "InboundFilters", + "FunctionToString", + "LinkedErrors", + "RequestData", + "NodeSystemError", + "ConversationId", + "Console", + "OnUncaughtException", + "OnUnhandledRejection", + "ContextLines", + "LocalVariablesAsync", + "Context", + "ChildProcess", + "ProcessSession", + "Modules", + "Http", + "NodeFetch", +]; + +beforeEach(() => { + delete process.env[DSN_ENV]; +}); + +afterEach(() => { + if (originalDsn === undefined) delete process.env[DSN_ENV]; + else process.env[DSN_ENV] = originalDsn; + vi.restoreAllMocks(); + vi.doUnmock("@sentry/node"); +}); + +describe("sentryReady", () => { + it("resolves and imports no SDK when SENTRY_DSN is unset", async () => { + // A spy that fails the test if the module attempts a dynamic import of + // an SDK it should never touch on the closed-gate path. `@sentry/node` + // is not installed, so an attempted import would settle this promise + // with a warning instead of resolving clean. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + const { sentryReady } = await importFreshSentry(); + + await expect(sentryReady).resolves.toBeUndefined(); + expect(warn).not.toHaveBeenCalled(); + }); +}); + +describe("captureException", () => { + it("is a no-op and does not throw when the gate is closed", async () => { + const { captureException, sentryReady } = await importFreshSentry(); + await sentryReady; + + expect(() => captureException(new Error("boom"))).not.toThrow(); + }); +}); + +describe("shutdownSentry", () => { + it("resolves once for concurrent callers", async () => { + const { shutdownSentry } = await importFreshSentry(); + + const first = shutdownSentry(); + const second = shutdownSentry(); + + // Memoized: concurrent callers share one shutdown promise. + expect(first).toBe(second); + await expect(first).resolves.toBeUndefined(); + }); +}); + +/** + * Seam tests for the two `errorHandler` call sites that report to Sentry. + * These tests spy on the exported `captureException` binding rather than + * import a real client, so they assert the call-site shape (one call, the + * Error object only) without a live Sentry SDK. + */ +describe("errorHandler Sentry capture", () => { + 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).mockReturnValue(res); + return res; + } + + it("captures one event for a 500-level HttpError", () => { + const capture = vi.spyOn(sentryModule, "captureException").mockImplementation(() => {}); + const req = makeReq(); + const res = makeRes(); + const next = vi.fn() as unknown as NextFunction; + const err = new HttpError(500, "db exploded"); + + errorHandler(err, req, res, next); + + expect(capture).toHaveBeenCalledTimes(1); + }); + + it("captures one event for an unknown error", () => { + const capture = vi.spyOn(sentryModule, "captureException").mockImplementation(() => {}); + const req = makeReq(); + const res = makeRes(); + const next = vi.fn() as unknown as NextFunction; + const err = new Error("boom"); + + errorHandler(err, req, res, next); + + expect(capture).toHaveBeenCalledTimes(1); + }); + + it("captures no event for a Zod validation 400 response", () => { + const capture = vi.spyOn(sentryModule, "captureException").mockImplementation(() => {}); + const req = makeReq(); + const res = makeRes(); + 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], + }); + + errorHandler(err, req, res, next); + + expect(res.status).toHaveBeenCalledWith(400); + expect(capture).not.toHaveBeenCalled(); + }); + + it("passes the Error object only to captureException, never the request-bearing ErrorContext", () => { + const capture = vi.spyOn(sentryModule, "captureException").mockImplementation(() => {}); + const req = makeReq(); + const res = makeRes(); + const next = vi.fn() as unknown as NextFunction; + const err = new Error("boom"); + + errorHandler(err, req, res, next); + + expect(capture).toHaveBeenCalledWith(err); + const [received] = capture.mock.calls[0]!; + expect(received).toBeInstanceOf(Error); + // The `ErrorContext` shape carries the request body, params, and query. + // The received value must not carry any of them. + expect(received).not.toHaveProperty("reqBody"); + expect(received).not.toHaveProperty("reqParams"); + expect(received).not.toHaveProperty("reqQuery"); + }); +}); + +describe("finalizeServerShutdown Sentry teardown", () => { + it("calls shutdownSentry after shutdownInstrumentation", async () => { + const order: string[] = []; + const shutdownInstrumentation = vi.fn(async () => { + order.push("instrumentation"); + }); + const shutdownSentry = vi.fn(async () => { + order.push("sentry"); + }); + + await finalizeServerShutdown({ + signal: "SIGTERM", + shutdownAppServices: undefined, + stopEmbeddedPostgres: null, + shutdownInstrumentation, + shutdownSentry, + log: { info: vi.fn(), error: vi.fn() }, + }); + + expect(order).toEqual(["instrumentation", "sentry"]); + }); +}); + +describe("missing @sentry/node package", () => { + it("logs one warning and resolves", async () => { + process.env[DSN_ENV] = "https://public@o0.ingest.sentry.io/1"; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + const { sentryReady } = await importFreshSentry(); + + // Bootstrap must absorb the failed dynamic import — the server keeps + // booting without error monitoring rather than crashing on an opt-in + // feature. + await expect(sentryReady).resolves.toBeUndefined(); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("@sentry/node package is not installed"), + expect.anything(), + ); + }); +}); + +describe("buildSentryInitOptions", () => { + it("sets sendDefaultPii false, tracesSampleRate 0, and skipOpenTelemetrySetup true", async () => { + const { buildSentryInitOptions } = await importFreshSentry(); + + const options = buildSentryInitOptions("https://public@o0.ingest.sentry.io/1", { + httpIntegration: () => ({ name: "Http" }), + onUnhandledRejectionIntegration: () => ({ name: "OnUnhandledRejection" }), + }); + + expect(options.sendDefaultPii).toBe(false); + expect(options.tracesSampleRate).toBe(0); + expect(options.skipOpenTelemetrySetup).toBe(true); + }); + + it("passes onUnhandledRejection with mode strict", async () => { + const { buildSentryInitOptions } = await importFreshSentry(); + const onUnhandledRejectionIntegration = vi.fn((options: unknown) => ({ + name: "OnUnhandledRejection", + ...(options as object), + })); + + const options = buildSentryInitOptions("https://public@o0.ingest.sentry.io/1", { + httpIntegration: () => ({ name: "Http" }), + onUnhandledRejectionIntegration, + }); + const resolved = options.integrations(DEFAULT_INTEGRATION_NAMES.map((name) => ({ name }))); + + expect(onUnhandledRejectionIntegration).toHaveBeenCalledWith({ mode: "strict" }); + const rejectionIntegration = resolved.find((i) => i.name === "OnUnhandledRejection"); + expect(rejectionIntegration).toMatchObject({ mode: "strict" }); + // The default OnUnhandledRejection entry must not survive alongside it. + expect(resolved.filter((i) => i.name === "OnUnhandledRejection")).toHaveLength(1); + }); + + it("the resolved server integration list holds no Console integration and no ContextLines integration", async () => { + const { buildSentryInitOptions } = await importFreshSentry(); + + const options = buildSentryInitOptions("https://public@o0.ingest.sentry.io/1", { + httpIntegration: () => ({ name: "Http" }), + onUnhandledRejectionIntegration: () => ({ name: "OnUnhandledRejection" }), + }); + const resolved = options.integrations(DEFAULT_INTEGRATION_NAMES.map((name) => ({ name }))); + const names = resolved.map((i) => i.name); + + expect(names).not.toContain("Console"); + expect(names).not.toContain("ContextLines"); + }); + + it("the resolved server integration list keeps OnUncaughtException, OnUnhandledRejection, LinkedErrors, and RequestData", async () => { + const { buildSentryInitOptions } = await importFreshSentry(); + + const options = buildSentryInitOptions("https://public@o0.ingest.sentry.io/1", { + httpIntegration: () => ({ name: "Http" }), + onUnhandledRejectionIntegration: () => ({ name: "OnUnhandledRejection" }), + }); + const resolved = options.integrations(DEFAULT_INTEGRATION_NAMES.map((name) => ({ name }))); + const names = resolved.map((i) => i.name); + + // The full error-capture set the plan requires, not just the four named + // in this test's title. + expect(names).toEqual( + expect.arrayContaining([ + "OnUncaughtException", + "OnUnhandledRejection", + "ChildProcess", + "LinkedErrors", + "RequestData", + "Modules", + "Context", + "ProcessSession", + ]), + ); + }); + + it("turns the outbound HTTP breadcrumb off and keeps the rest of the Http integration", async () => { + const { buildSentryInitOptions } = await importFreshSentry(); + const httpIntegration = vi.fn((options: unknown) => ({ name: "Http", ...(options as object) })); + + const options = buildSentryInitOptions("https://public@o0.ingest.sentry.io/1", { + httpIntegration, + onUnhandledRejectionIntegration: () => ({ name: "OnUnhandledRejection" }), + }); + const resolved = options.integrations(DEFAULT_INTEGRATION_NAMES.map((name) => ({ name }))); + + expect(httpIntegration).toHaveBeenCalledWith({ breadcrumbs: false }); + expect(resolved.filter((i) => i.name === "Http")).toHaveLength(1); + }); +}); + +describe("with @sentry/node mocked", () => { + it("initializes the client and shares captureException / shutdownSentry with it", async () => { + process.env[DSN_ENV] = "https://public@o0.ingest.sentry.io/1"; + const mocks = mockSentryPackage(); + + const { sentryReady, captureException, shutdownSentry } = await importFreshSentry(); + await sentryReady; + + expect(mocks.init).toHaveBeenCalledTimes(1); + const initOptions = mocks.init.mock.calls[0][0] as { dsn: string }; + expect(initOptions.dsn).toBe("https://public@o0.ingest.sentry.io/1"); + + captureException(new Error("boom")); + expect(mocks.captureException).toHaveBeenCalledWith(expect.any(Error)); + + await shutdownSentry(); + expect(mocks.close).toHaveBeenCalledWith(5_000); + }); +}); + +/** + * Proves the one-project result: the server and the browser both resolve + * their Sentry client from the same `SENTRY_DSN` value, so both send events + * to the same Sentry project. The browser reads its DSN from the + * `GET /api/auth/get-session` response body (see `ui/src/lib/sentry.ts` and + * `ui/src/components/SentryGate.tsx`), never from a `` tag. + */ +describe("one-project resolution", () => { + function makeSessionApp() { + const app = express(); + app.use((req, _res, next) => { + req.actor = { type: "board", userId: "user-1", source: "session" }; + next(); + }); + const db = { + select: () => ({ + from: () => ({ + where: () => + Promise.resolve([ + { id: "user-1", name: "Jane Example", email: "jane@example.com", image: null }, + ]), + }), + }), + }; + app.use("/api/auth", authRoutes(db as unknown as Parameters[0])); + app.use(errorHandler); + return app; + } + + it("the server initializer and GET /api/auth/get-session resolve the same Sentry project", async () => { + process.env[DSN_ENV] = "https://public@o0.ingest.sentry.io/1"; + const mocks = mockSentryPackage(); + + const { sentryReady } = await importFreshSentry(); + await sentryReady; + const initOptions = mocks.init.mock.calls[0]![0] as { dsn: string }; + + const app = makeSessionApp(); + const res = await request(app).get("/api/auth/get-session"); + + expect(res.status).toBe(200); + expect(res.body.sentryDsn).toBe(initOptions.dsn); + }); +}); + +// `@sentry/node` is an optional runtime dependency (see the module comment +// in sentry.ts). When it is absent, the three tests below cannot run against +// the true SDK, so they are skipped — the same pattern instrumentation.test.ts +// uses for its OpenTelemetry-SDK-dependent test. +const sentryPackage = (() => { + try { + const require = createRequire(import.meta.url); + return require("@sentry/node") as { + init(options: Record): unknown; + captureException(error: unknown): string; + httpIntegration(options: { breadcrumbs: boolean }): { name: string }; + onUnhandledRejectionIntegration(options: { mode: string }): { name: string }; + flush(timeout?: number): Promise; + close(timeout?: number): Promise; + }; + } catch { + return null; + } +})(); + +describe.skipIf(!sentryPackage)("captured event shape against the real @sentry/node SDK", () => { + /** + * Initialize the real SDK with this module's exact options, plus a + * transport stub so no event leaves the test process, plus `beforeSend` + * so the test can inspect the resolved event before it would have been + * sent. `beforeSend` is test-only introspection — the module under test + * adds no `beforeSend` of its own (constraint: built-in options only). + */ + async function initRealSentryForTest(onEvent: (event: Record) => void) { + const Sentry = sentryPackage!; + const { buildSentryInitOptions } = await importFreshSentry(); + const options = { + ...buildSentryInitOptions("https://public@o0.ingest.sentry.io/1", Sentry), + transport: () => ({ send: async () => ({}), flush: async () => true }), + beforeSend: (event: Record) => { + onEvent(event); + return event; + }, + }; + Sentry.init(options); + } + + it("a server event captured after a console.error call carries no console breadcrumb", async () => { + const Sentry = sentryPackage!; + let captured: Record | null = null; + await initRealSentryForTest((event) => { + captured = event; + }); + + // eslint-disable-next-line no-console + console.error("child process stderr: simulated failure"); + Sentry.captureException(new Error("after console.error")); + await Sentry.flush(2000); + + expect(captured).not.toBeNull(); + expect((captured as Record).breadcrumbs).toBeUndefined(); + }); + + it("a server event carries no stack-frame source context", async () => { + const Sentry = sentryPackage!; + let captured: Record | null = null; + await initRealSentryForTest((event) => { + captured = event; + }); + + Sentry.captureException(new Error("stack frame check")); + await Sentry.flush(2000); + + const event = captured as unknown as { + exception: { values: Array<{ stacktrace: { frames: Array> } }> }; + }; + const frames = event.exception.values[0].stacktrace.frames; + expect(frames.length).toBeGreaterThan(0); + for (const frame of frames) { + expect(frame.context_line).toBeUndefined(); + expect(frame.pre_context).toBeUndefined(); + expect(frame.post_context).toBeUndefined(); + } + }); + + it("a server event carries no outbound HTTP breadcrumb", async () => { + const Sentry = sentryPackage!; + let captured: Record | null = null; + await initRealSentryForTest((event) => { + captured = event; + }); + + const server = http.createServer((_req, res) => res.end("ok")); + await new Promise((resolve) => server.listen(0, resolve)); + const port = (server.address() as { port: number }).port; + await new Promise((resolve) => { + http.get(`http://127.0.0.1:${port}/probe?token=secret`, (res) => { + res.resume(); + res.on("end", resolve); + }); + }); + server.close(); + + Sentry.captureException(new Error("after outbound http call")); + await Sentry.flush(2000); + + expect((captured as unknown as Record).breadcrumbs).toBeUndefined(); + }); + + /** + * `skipOpenTelemetrySetup: true` (set above) keeps this module out of + * Paperclip's separate, independently opt-in OpenTelemetry feature. It + * also turns off Sentry's own per-request async-context tracking. The + * `RequestData` integration reads the inbound URL, method, headers, + * cookies, and query string from that per-request context. With the + * context off, a captured event carries no request field — not the + * SDK's documented default. This test proves the gap, so the operator + * documentation states the true capture set. + */ + it("a server event captured inside a real HTTP request handler carries no request field", async () => { + const Sentry = sentryPackage!; + let captured: Record | null = null; + await initRealSentryForTest((event) => { + captured = event; + }); + + const server = http.createServer((req, res) => { + req.on("data", () => {}); + req.on("end", () => { + Sentry.captureException(new Error("boom from a real request handler")); + res.end("ok"); + }); + }); + await new Promise((resolve) => server.listen(0, resolve)); + const port = (server.address() as { port: number }).port; + await new Promise((resolve) => { + http.get(`http://127.0.0.1:${port}/api/test?foo=bar`, (res) => { + res.resume(); + res.on("end", resolve); + }); + }); + server.close(); + await Sentry.flush(2000); + + expect(captured).not.toBeNull(); + expect((captured as unknown as Record).request).toBeUndefined(); + }); +}); diff --git a/server/src/index.ts b/server/src/index.ts index e455de481b..3890076b04 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -4,6 +4,7 @@ // instrumentationReady before opening DB connections or constructing the // HTTP server, so trace coverage does not depend on incidental timing. import { instrumentationReady, shutdownInstrumentation } from "./instrumentation.js"; +import { sentryReady, shutdownSentry, captureException } from "./sentry.js"; import { existsSync, readFileSync, rmSync } from "node:fs"; import { createServer } from "node:http"; import { resolve } from "node:path"; @@ -157,6 +158,9 @@ export async function startServer(): Promise { // Tracing must be active (or have failed and logged) before the first DB // connection or the HTTP server exists — see instrumentation.ts. await instrumentationReady; + // Error monitoring must be ready before the first request can fail — see + // sentry.ts. + await sentryReady; ensureDecisionSigningSecret(); let config = loadConfig(); initTelemetry({ enabled: config.telemetryEnabled }); @@ -1780,6 +1784,7 @@ export async function startServer(): Promise { shutdownAppServices: appShutdown, stopEmbeddedPostgres, shutdownInstrumentation, + shutdownSentry, log: logger, }); @@ -1814,8 +1819,10 @@ function isMainModule(metaUrl: string): boolean { } if (isMainModule(import.meta.url)) { - void startServer().catch((err) => { + void startServer().catch(async (err) => { logger.error({ err }, "Paperclip server failed to start"); + captureException(err); + await shutdownSentry(); process.exit(1); }); } diff --git a/server/src/middleware/error-handler.ts b/server/src/middleware/error-handler.ts index 0a4aa1a170..0c0a218a1d 100644 --- a/server/src/middleware/error-handler.ts +++ b/server/src/middleware/error-handler.ts @@ -4,6 +4,7 @@ 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 { @@ -49,6 +50,13 @@ function attachErrorContext( } } +/** Report a server-side crash to every error sink. */ +function reportCrash(error: Error): void { + const tc = getTelemetryClient(); + if (tc) trackErrorHandlerCrash(tc, { errorCode: error.name }); + captureException(error); +} + function getPaperclipDb(req: Request): Db | null { const locals = req.app?.locals as { paperclipDb?: Db; db?: Db } | undefined; return locals?.paperclipDb ?? locals?.db ?? null; @@ -107,8 +115,7 @@ export function errorHandler( { message: err.message, stack: err.stack, name: err.name, details: err.details }, err, ); - const tc = getTelemetryClient(); - if (tc) trackErrorHandlerCrash(tc, { errorCode: err.name }); + reportCrash(err); } res.status(err.status).json({ error: err.message, @@ -147,8 +154,7 @@ export function errorHandler( rootError, ); - const tc = getTelemetryClient(); - if (tc) trackErrorHandlerCrash(tc, { errorCode: rootError.name }); + reportCrash(rootError); res.status(500).json({ error: "Internal server error", diff --git a/server/src/routes/auth.ts b/server/src/routes/auth.ts index ee94341c8f..4cb37e0508 100644 --- a/server/src/routes/auth.ts +++ b/server/src/routes/auth.ts @@ -49,6 +49,10 @@ export function authRoutes(db: Db) { userId: req.actor.userId, }, user, + // The browser reads this value to open its own Sentry gate — see + // `ui/src/lib/sentry.ts`. `req.actor.type` already gates this whole + // handler, so no second authorization check runs here. + sentryDsn: process.env.SENTRY_DSN || null, })); }); diff --git a/server/src/sentry.ts b/server/src/sentry.ts new file mode 100644 index 0000000000..76f75d2a1d --- /dev/null +++ b/server/src/sentry.ts @@ -0,0 +1,173 @@ +// Optional Sentry error monitoring for the server process. +// +// Activated only when `SENTRY_DSN` is set. When unset, no Sentry package is +// loaded at all. +// +// The import is dynamic and the package is an optional runtime dependency — +// operators who want server-side error monitoring install `@sentry/node` +// themselves. That keeps Sentry off the default dependency graph and avoids +// forcing a lockfile bump for an opt-in feature. This gate mirrors the +// OpenTelemetry gate in `instrumentation.ts`. +// +// OpenTelemetry keeps ownership of trace setup: the initializer passes +// `skipOpenTelemetrySetup: true` and `tracesSampleRate: 0`, so this module +// adds error monitoring only and starts no span or trace behavior of its +// own. +// +// Default-integration privacy note: `sendDefaultPii: false` filters values +// by name, inside the `RequestData` integration only. Three other default +// integrations copy raw values past that filter, so the initializer removes +// or narrows them with built-in Sentry options — no custom filter code: +// - `Console` turns a `console.*` call into a breadcrumb with the raw +// arguments. The initializer drops it. +// - `ContextLines` reads local source lines around each stack frame off +// the host disk. The initializer drops it. +// - `Http` records a breadcrumb for each outbound request, with its URL +// and query string. The initializer keeps the integration (`RequestData` +// and request isolation need it) and turns the breadcrumb off with the +// integration's own `breadcrumbs` option. +// +// `onUnhandledRejectionIntegration` defaults to `mode: "warn"`, which +// registers a `process.on("unhandledRejection")` listener. Node cancels its +// own crash-on-unhandled-rejection behavior when any listener is registered. +// The server relies on that crash today, so the initializer passes +// `mode: "strict"`: Sentry still captures the event, then exits the process, +// so the existing crash-and-restart behavior stays. + +const dsn = process.env.SENTRY_DSN; + +/** The subset of the `@sentry/node` client surface this gate calls. */ +interface SentryHandle { + captureException(error: unknown): string; + close(timeout?: number): Promise; +} + +let sentryHandle: SentryHandle | null = null; +let shutdownPromise: Promise | null = null; + +/** + * Resolves once the Sentry SDK has started, or once bootstrap has failed and + * logged, or at once when `SENTRY_DSN` is unset. No caller needs to await + * this before calling `captureException` — it is a no-op until ready — but + * `index.ts` awaits it at startup so the first real error has a live client. + */ +export const sentryReady: Promise = dsn ? bootstrapSentry(dsn) : Promise.resolve(); + +/** + * Report an error to Sentry. A no-op before the gate opens, when the gate + * never opens (`SENTRY_DSN` unset), or when bootstrap failed. Never throws — + * observability must not change control flow. + */ +export function captureException(error: unknown): void { + if (!sentryHandle) return; + try { + sentryHandle.captureException(error); + } catch (err) { + // eslint-disable-next-line no-console + console.error("[paperclip] Sentry captureException failed", err); + } +} + +/** + * Flush buffered events and close the Sentry client. Idempotent — concurrent + * callers share one shutdown. A no-op when monitoring is off or bootstrap + * failed. + */ +export function shutdownSentry(): Promise { + shutdownPromise ??= (async () => { + await sentryReady; + if (!sentryHandle) return; + try { + // Awaiting matters: the client flushes buffered events to Sentry + // during close; exiting before it settles silently drops them. + await sentryHandle.close(5_000); + } catch (err) { + // eslint-disable-next-line no-console + console.error("[paperclip] Sentry shutdown failed", err); + } + })(); + return shutdownPromise; +} + +/** + * The subset of the `@sentry/node` module surface the initializer needs to + * build its options object. A structural type, not the real Sentry type — + * the real type is unavailable at compile time because the package is an + * optional runtime dependency (see the module comment above). + */ +interface SentryModuleLike { + httpIntegration(options: { breadcrumbs: boolean }): { name: string }; + onUnhandledRejectionIntegration(options: { mode: string }): { name: string }; +} + +/** The `Sentry.init` options this gate builds. */ +export interface SentryInitOptions { + dsn: string; + skipOpenTelemetrySetup: boolean; + tracesSampleRate: number; + sendDefaultPii: boolean; + integrations: (defaults: Array<{ name: string }>) => Array<{ name: string }>; +} + +/** + * Build the `Sentry.init` options object. A pure function, split out from + * `bootstrapSentry` so a test can call it with a real `@sentry/node` module + * and assert the resolved integration list and the captured-event shape + * against the true SDK, not a stand-in. + */ +export function buildSentryInitOptions( + dsn: string, + Sentry: SentryModuleLike, +): SentryInitOptions { + return { + dsn, + skipOpenTelemetrySetup: true, + tracesSampleRate: 0, + sendDefaultPii: false, + integrations: (defaults: Array<{ name: string }>) => { + const kept = defaults.filter( + (integration) => + integration.name !== "Console" && + integration.name !== "ContextLines" && + integration.name !== "Http" && + integration.name !== "OnUnhandledRejection", + ); + return [ + ...kept, + // Keep the rest of the Http integration — RequestData and request + // isolation need it — but turn the outbound breadcrumb off. + Sentry.httpIntegration({ breadcrumbs: false }), + // Keep today's crash-on-unhandled-rejection behavior. See the + // module comment above for why the default mode cannot stay. + Sentry.onUnhandledRejectionIntegration({ mode: "strict" }), + ]; + }, + }; +} + +async function bootstrapSentry(dsn: string): Promise { + try { + // Dynamic import so type-resolution doesn't require the package to be + // installed unless the operator actually opts in. + // @ts-ignore optional peer dep + const Sentry = await import("@sentry/node"); + + Sentry.init(buildSentryInitOptions(dsn, Sentry)); + + sentryHandle = { + captureException: (error) => Sentry.captureException(error), + close: (timeout) => Sentry.close(timeout), + }; + } catch (err) { + // The package is not installed, or the dynamic import or init call + // failed. Fall through with a single diagnostic so the opt-in path is + // self-documenting. The gate fails open — the server keeps booting + // without error monitoring rather than crashing on an opt-in feature. + // eslint-disable-next-line no-console + console.warn( + "[paperclip] SENTRY_DSN is set but the @sentry/node package is not " + + "installed. Install @sentry/node to enable server error monitoring.", + err, + ); + } +} diff --git a/server/src/shutdown.test.ts b/server/src/shutdown.test.ts index 5b8a3c7a95..9b994248f8 100644 --- a/server/src/shutdown.test.ts +++ b/server/src/shutdown.test.ts @@ -37,6 +37,9 @@ describe("finalizeServerShutdown", () => { const shutdownInstrumentation = vi.fn(async () => { order.push("instrumentation:flush"); }); + const shutdownSentry = vi.fn(async () => { + order.push("sentry:flush"); + }); let exited = false; const finalize = finalizeServerShutdown({ @@ -44,6 +47,7 @@ describe("finalizeServerShutdown", () => { shutdownAppServices, stopEmbeddedPostgres, shutdownInstrumentation, + shutdownSentry, log: stubLogger(), }).then(() => { // This models the caller's `process.exit(0)` continuation. @@ -67,6 +71,7 @@ describe("finalizeServerShutdown", () => { "appServices:settled", "postgres:stop", "instrumentation:flush", + "sentry:flush", "exit", ]); }); @@ -87,6 +92,7 @@ describe("finalizeServerShutdown", () => { const shutdownInstrumentation = vi.fn(async () => { order.push("instrumentation:flush"); }); + const shutdownSentry = vi.fn(async () => undefined); const log = stubLogger(); let exited = false; @@ -95,6 +101,7 @@ describe("finalizeServerShutdown", () => { shutdownAppServices, stopEmbeddedPostgres, shutdownInstrumentation, + shutdownSentry, log, }).then(() => { exited = true; @@ -120,6 +127,7 @@ describe("finalizeServerShutdown", () => { it("skips the database stop when no embedded PostgreSQL runs in this process", async () => { const shutdownAppServices = vi.fn(async () => undefined); const shutdownInstrumentation = vi.fn(async () => undefined); + const shutdownSentry = vi.fn(async () => undefined); const log = stubLogger(); await finalizeServerShutdown({ @@ -127,6 +135,7 @@ describe("finalizeServerShutdown", () => { shutdownAppServices, stopEmbeddedPostgres: null, shutdownInstrumentation, + shutdownSentry, log, }); diff --git a/server/src/shutdown.ts b/server/src/shutdown.ts index 9882e256ec..bc15aca8f2 100644 --- a/server/src/shutdown.ts +++ b/server/src/shutdown.ts @@ -25,6 +25,7 @@ export async function finalizeServerShutdown(input: { shutdownAppServices: (() => Promise) | undefined; stopEmbeddedPostgres: (() => Promise) | null; shutdownInstrumentation: () => Promise; + shutdownSentry: () => Promise; log: ShutdownLogger; }): Promise { const { signal } = input; @@ -50,6 +51,10 @@ export async function finalizeServerShutdown(input: { // Flush buffered OTel spans before the process goes away; without this await // the exporter's final batch is dropped on exit. await input.shutdownInstrumentation(); + + // Flush buffered Sentry events before the process goes away; without this + // await the last events are dropped on exit. + await input.shutdownSentry(); } const COORDINATED_SHUTDOWN_SIGNALS = ["SIGINT", "SIGTERM"] as const; diff --git a/ui/package.json b/ui/package.json index bf0fb32c38..ec0e14d9f0 100644 --- a/ui/package.json +++ b/ui/package.json @@ -72,6 +72,7 @@ "tailwind-merge": "^3.6.0" }, "devDependencies": { + "@sentry/browser": "^10.71.0", "@storybook/addon-a11y": "10.5.10", "@storybook/addon-docs": "10.5.10", "@storybook/react-vite": "10.5.10", diff --git a/ui/src/components/AppErrorBoundary.test.tsx b/ui/src/components/AppErrorBoundary.test.tsx index 8de874f7e4..285b9c595f 100644 --- a/ui/src/components/AppErrorBoundary.test.tsx +++ b/ui/src/components/AppErrorBoundary.test.tsx @@ -5,6 +5,12 @@ import { createRoot } from "react-dom/client"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { AppErrorBoundary } from "./AppErrorBoundary"; +const captureBrowserExceptionMock = vi.hoisted(() => vi.fn()); + +vi.mock("@/lib/sentry", () => ({ + captureBrowserException: (error: unknown) => captureBrowserExceptionMock(error), +})); + function BoomRender(): never { throw new Error("Maximum update depth exceeded"); } @@ -30,6 +36,30 @@ describe("AppErrorBoundary", () => { afterEach(() => { consoleErrorSpy.mockRestore(); container.remove(); + captureBrowserExceptionMock.mockClear(); + }); + + it("reports one captured error and keeps the reload prompt", () => { + const root = createRoot(container); + act(() => { + root.render( + + + , + ); + }); + + expect(captureBrowserExceptionMock).toHaveBeenCalledTimes(1); + expect(captureBrowserExceptionMock).toHaveBeenCalledWith(expect.any(Error)); + expect( + Array.from(container.querySelectorAll("button")).some( + (button) => button.textContent === "Reload page", + ), + ).toBe(true); + + act(() => { + root.unmount(); + }); }); it("renders a reload prompt instead of a blank page when the shell throws in render", () => { diff --git a/ui/src/components/AppErrorBoundary.tsx b/ui/src/components/AppErrorBoundary.tsx index 12717b7747..68884f0614 100644 --- a/ui/src/components/AppErrorBoundary.tsx +++ b/ui/src/components/AppErrorBoundary.tsx @@ -1,4 +1,5 @@ import { Component, type ErrorInfo, type ReactNode } from "react"; +import { captureBrowserException } from "@/lib/sentry"; type AppErrorBoundaryState = { error: Error | null; @@ -24,6 +25,7 @@ export class AppErrorBoundary extends Component<{ children: ReactNode }, AppErro override componentDidCatch(error: unknown, info: ErrorInfo): void { console.error("App shell crashed", { error, componentStack: info.componentStack }); + captureBrowserException(error); } override render() { diff --git a/ui/src/components/RouteErrorBoundary.test.tsx b/ui/src/components/RouteErrorBoundary.test.tsx index d6195f1ecf..ba8f7d34cd 100644 --- a/ui/src/components/RouteErrorBoundary.test.tsx +++ b/ui/src/components/RouteErrorBoundary.test.tsx @@ -7,12 +7,17 @@ import { RouteErrorBoundary } from "./RouteErrorBoundary"; const navigateMock = vi.hoisted(() => vi.fn()); const routerLocation = vi.hoisted(() => ({ current: { pathname: "/co/agents/new", search: "?adapterType=claude_local" } })); +const captureBrowserExceptionMock = vi.hoisted(() => vi.fn()); vi.mock("@/lib/router", () => ({ useLocation: () => routerLocation.current, useNavigate: () => navigateMock, })); +vi.mock("@/lib/sentry", () => ({ + captureBrowserException: (error: unknown) => captureBrowserExceptionMock(error), +})); + function Boom(): never { throw new Error("Maximum update depth exceeded"); } @@ -33,6 +38,30 @@ describe("RouteErrorBoundary", () => { afterEach(() => { consoleErrorSpy.mockRestore(); container.remove(); + captureBrowserExceptionMock.mockClear(); + }); + + it("reports one captured error and keeps the recovery button", () => { + const root = createRoot(container); + act(() => { + root.render( + + + , + ); + }); + + expect(captureBrowserExceptionMock).toHaveBeenCalledTimes(1); + expect(captureBrowserExceptionMock).toHaveBeenCalledWith(expect.any(Error)); + expect( + Array.from(container.querySelectorAll("button")).some( + (button) => button.textContent === "Go back", + ), + ).toBe(true); + + act(() => { + root.unmount(); + }); }); it("renders a recoverable error card instead of a blank page when a child throws", () => { diff --git a/ui/src/components/RouteErrorBoundary.tsx b/ui/src/components/RouteErrorBoundary.tsx index 69ab488445..2c2bd9403a 100644 --- a/ui/src/components/RouteErrorBoundary.tsx +++ b/ui/src/components/RouteErrorBoundary.tsx @@ -1,6 +1,7 @@ import { Component, type ErrorInfo, type ReactNode } from "react"; import { useLocation, useNavigate } from "@/lib/router"; import { Button } from "@/components/ui/button"; +import { captureBrowserException } from "@/lib/sentry"; type RouteErrorBoundaryInnerProps = { resetKey: string; @@ -21,6 +22,7 @@ class RouteErrorBoundaryInner extends Component vi.fn()); +const initBrowserErrorMonitoringMock = vi.hoisted(() => vi.fn(async (_dsn: string) => {})); +const teardownBrowserErrorMonitoringMock = vi.hoisted(() => vi.fn(async () => {})); + +vi.mock("@/api/auth", () => ({ + authApi: { getSession: () => getSessionMock() }, +})); + +vi.mock("@/lib/sentry", () => ({ + initBrowserErrorMonitoring: (dsn: string) => initBrowserErrorMonitoringMock(dsn), + teardownBrowserErrorMonitoring: () => teardownBrowserErrorMonitoringMock(), +})); + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +(globalThis as any).IS_REACT_ACT_ENVIRONMENT = true; + +async function flushReact() { + await act(async () => { + await Promise.resolve(); + await new Promise((resolve) => window.setTimeout(resolve, 0)); + }); +} + +describe("SentryGate", () => { + let container: HTMLDivElement; + let queryClient: QueryClient; + + beforeEach(() => { + container = document.createElement("div"); + document.body.appendChild(container); + queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + }); + + afterEach(() => { + container.remove(); + document.body.innerHTML = ""; + vi.clearAllMocks(); + }); + + async function renderGate() { + const root = createRoot(container); + await act(async () => { + root.render( + + + , + ); + }); + await flushReact(); + await flushReact(); + return root; + } + + it("loads no SDK when the session query answers null", async () => { + getSessionMock.mockResolvedValue(null); + + const root = await renderGate(); + + expect(initBrowserErrorMonitoringMock).not.toHaveBeenCalled(); + expect(container.textContent).toBe(""); + root.unmount(); + }); + + it("loads no SDK when the session sentryDsn is null", async () => { + getSessionMock.mockResolvedValue({ + session: { id: "s1", userId: "u1" }, + user: { id: "u1", email: "a@b.com", name: "Jane", image: null }, + sentryDsn: null, + }); + + const root = await renderGate(); + + expect(initBrowserErrorMonitoringMock).not.toHaveBeenCalled(); + root.unmount(); + }); + + it("opens the gate once when the session carries a DSN", async () => { + getSessionMock.mockResolvedValue({ + session: { id: "s1", userId: "u1" }, + user: { id: "u1", email: "a@b.com", name: "Jane", image: null }, + sentryDsn: "https://public@o0.ingest.sentry.io/1", + }); + + const root = await renderGate(); + + expect(initBrowserErrorMonitoringMock).toHaveBeenCalledTimes(1); + expect(initBrowserErrorMonitoringMock).toHaveBeenCalledWith("https://public@o0.ingest.sentry.io/1"); + root.unmount(); + }); + + it("opens the gate after a signed-out session query refetches with a DSN", async () => { + getSessionMock.mockResolvedValue(null); + const root = await renderGate(); + expect(initBrowserErrorMonitoringMock).not.toHaveBeenCalled(); + + getSessionMock.mockResolvedValue({ + session: { id: "s1", userId: "u1" }, + user: { id: "u1", email: "a@b.com", name: "Jane", image: null }, + sentryDsn: "https://public@o0.ingest.sentry.io/1", + }); + await act(async () => { + await queryClient.refetchQueries({ queryKey: queryKeys.auth.session }); + }); + await flushReact(); + + expect(initBrowserErrorMonitoringMock).toHaveBeenCalledTimes(1); + expect(initBrowserErrorMonitoringMock).toHaveBeenCalledWith("https://public@o0.ingest.sentry.io/1"); + root.unmount(); + }); + + it("closes browser monitoring when sign-out clears the session's DSN", async () => { + getSessionMock.mockResolvedValue({ + session: { id: "s1", userId: "u1" }, + user: { id: "u1", email: "a@b.com", name: "Jane", image: null }, + sentryDsn: "https://public@o0.ingest.sentry.io/1", + }); + const root = await renderGate(); + expect(initBrowserErrorMonitoringMock).toHaveBeenCalledTimes(1); + expect(teardownBrowserErrorMonitoringMock).not.toHaveBeenCalled(); + + // `useSignOut` resets the session query on sign-out; a mounted observer + // (this component) sees its data drop to `undefined` immediately. + getSessionMock.mockResolvedValue(null); + await act(async () => { + queryClient.resetQueries({ queryKey: queryKeys.auth.session }); + }); + await flushReact(); + + expect(teardownBrowserErrorMonitoringMock).toHaveBeenCalledTimes(1); + // Signing back in must start a fresh client, not skip re-init. + expect(initBrowserErrorMonitoringMock).toHaveBeenCalledTimes(1); + root.unmount(); + }); + + it("closes browser monitoring when the gate unmounts while a session DSN is set", async () => { + getSessionMock.mockResolvedValue({ + session: { id: "s1", userId: "u1" }, + user: { id: "u1", email: "a@b.com", name: "Jane", image: null }, + sentryDsn: "https://public@o0.ingest.sentry.io/1", + }); + const root = await renderGate(); + expect(initBrowserErrorMonitoringMock).toHaveBeenCalledTimes(1); + + root.unmount(); + + expect(teardownBrowserErrorMonitoringMock).toHaveBeenCalledTimes(1); + }); +}); diff --git a/ui/src/components/SentryGate.tsx b/ui/src/components/SentryGate.tsx new file mode 100644 index 0000000000..0fa0d80e1f --- /dev/null +++ b/ui/src/components/SentryGate.tsx @@ -0,0 +1,39 @@ +import { useEffect } from "react"; +import { useQuery } from "@tanstack/react-query"; +import { authApi } from "@/api/auth"; +import { queryKeys } from "@/lib/queryKeys"; +import { initBrowserErrorMonitoring, teardownBrowserErrorMonitoring } from "@/lib/sentry"; + +/** + * Opens the browser Sentry gate for an authorized board actor. Reads the + * signed-in session and starts browser error monitoring only when the + * session carries a Sentry DSN. Renders nothing. + * + * Sets no `enabled` option on the session query, so this also runs in + * `local_trusted` mode — the actor middleware fabricates a board actor + * there, so `/api/auth/get-session` still answers 200 with a session. + * + * Sign-out clears the session query (`useSignOut`), so `dsn` goes back to + * falsy on the same render pass that drops the session. That change tears + * down monitoring through the effect cleanup below, closing the running + * client, so a signed-out browser sends Sentry no more events. + */ +export function SentryGate() { + const { data: session } = useQuery({ + queryKey: queryKeys.auth.session, + queryFn: () => authApi.getSession(), + retry: false, + }); + + const dsn = session?.sentryDsn; + + useEffect(() => { + if (!dsn) return; + void initBrowserErrorMonitoring(dsn); + return () => { + void teardownBrowserErrorMonitoring(); + }; + }, [dsn]); + + return null; +} diff --git a/ui/src/lib/sentry.test.ts b/ui/src/lib/sentry.test.ts new file mode 100644 index 0000000000..b2838ebb53 --- /dev/null +++ b/ui/src/lib/sentry.test.ts @@ -0,0 +1,408 @@ +// @vitest-environment jsdom + +import { afterEach, describe, expect, it, vi } from "vitest"; + +/** + * Tests for the browser Sentry gate. Unlike the server gate, + * `@sentry/browser` is a real development dependency of this package (see + * the constraint in the plan), so most tests here run against the true SDK + * instead of a stand-in. + * + * The module holds module-scoped state (the readiness promise, the client + * handle), so each test resets the module registry and imports a fresh copy. + */ + +const DSN = "https://public@o0.ingest.sentry.io/1"; + +async function importFreshSentry() { + vi.resetModules(); + return await import("./sentry"); +} + +/** + * Register a fake `@sentry/browser` module for the next dynamic import. Used + * by the tests that only care about the call the module makes into the SDK, + * not the shape of a captured event. + * + * `init` and `setClient` both write one shared `attachedClient` variable — + * the real SDK's `Sentry.init` and `Sentry.getCurrentScope().setClient` both + * mutate the same module-global scope, not a value scoped to one caller. A + * race that lets a stale `setClient(undefined)` land after a newer + * `Sentry.init` needs a mock that tracks this shared state to catch it — + * two mocks that record calls independently cannot see the clobber. + */ +function mockSentryPackage() { + let nextClientId = 0; + let attachedClient: { id: string; close: (timeout?: number) => Promise } | null = null; + + // One shared `close` mock, attached to every client `init` creates. The + // fix reads the client with `getClient()` and calls `client.close()` + // directly, so the mocked client — not the mocked module — needs the + // `close` method a test can hold open or reject. + const close = vi.fn(async () => true); + const init = vi.fn((_options: Record) => { + attachedClient = { id: `client-${nextClientId++}`, close }; + }); + const captureException = vi.fn(() => (attachedClient ? "event-id" : undefined)); + const getClient = vi.fn(() => attachedClient ?? undefined); + const setClient = vi.fn((client: typeof attachedClient | undefined) => { + attachedClient = client ?? null; + }); + const getCurrentScope = vi.fn(() => ({ setClient })); + + vi.doMock("@sentry/browser", () => ({ init, captureException, close, getClient, getCurrentScope })); + + return { + init, + captureException, + close, + getClient, + getCurrentScope, + setClient, + /** The `id` of whichever client `init`/`setClient` last attached, or `null` if none is. */ + attachedClientId: () => attachedClient?.id ?? null, + }; +} + +/** + * Hold a mocked `close()` call open until the test releases it. Returns the + * release function. Used to put a teardown mid-flight without a second, + * truly concurrent dynamic `import()` of the mocked `@sentry/browser` module + * — Vitest does not guarantee two in-flight `import()` calls for one mocked + * specifier both resolve against the same mock instance, so a test must + * never rely on that to race two sign-ins. + */ +function holdCloseOpen(mocks: ReturnType): () => void { + let release = () => {}; + const gate = new Promise((resolve) => { + release = resolve; + }); + mocks.close.mockImplementationOnce(async () => { + await gate; + return true; + }); + return release; +} + +afterEach(() => { + vi.restoreAllMocks(); + vi.doUnmock("@sentry/browser"); +}); + +// A representative default-integration list, shaped like the array +// `@sentry/browser@10.71.0`'s `getDefaultIntegrations()` returns. Recorded +// 2026-08-25 with `node -e` against the published package. +const DEFAULT_INTEGRATION_NAMES = [ + "InboundFilters", + "FunctionToString", + "ConversationId", + "BrowserApiErrors", + "Breadcrumbs", + "GlobalHandlers", + "LinkedErrors", + "Dedupe", + "HttpContext", + "CultureContext", + "BrowserSession", +]; + +describe("initBrowserErrorMonitoring", () => { + it("initializes with the DSN it receives", async () => { + const mocks = mockSentryPackage(); + const { initBrowserErrorMonitoring } = await importFreshSentry(); + + await initBrowserErrorMonitoring(DSN); + + expect(mocks.init).toHaveBeenCalledTimes(1); + const initOptions = mocks.init.mock.calls[0][0] as { dsn: string }; + expect(initOptions.dsn).toBe(DSN); + }); + + it("a second call starts no second client", async () => { + const mocks = mockSentryPackage(); + const { initBrowserErrorMonitoring } = await importFreshSentry(); + + await initBrowserErrorMonitoring(DSN); + await initBrowserErrorMonitoring(DSN); + + expect(mocks.init).toHaveBeenCalledTimes(1); + }); +}); + +describe("teardownBrowserErrorMonitoring", () => { + it("is a no-op when monitoring never started", async () => { + const { teardownBrowserErrorMonitoring } = await importFreshSentry(); + + await expect(teardownBrowserErrorMonitoring()).resolves.toBeUndefined(); + }); + + it("closes the running client and detaches it from the current scope", async () => { + const mocks = mockSentryPackage(); + const { initBrowserErrorMonitoring, teardownBrowserErrorMonitoring } = await importFreshSentry(); + await initBrowserErrorMonitoring(DSN); + + await teardownBrowserErrorMonitoring(); + + expect(mocks.close).toHaveBeenCalledTimes(1); + expect(mocks.setClient).toHaveBeenCalledWith(undefined); + }); + + it("detaches the client from the current scope before close() settles", async () => { + // The client stays attached, and stays enabled, for the whole `close()` + // call — a signed-out page can still reach it until detach runs. Hold + // `close()` open and assert the detach already ran while it is still + // pending, so a regression back to close-then-detach fails this test. + const mocks = mockSentryPackage(); + const { initBrowserErrorMonitoring, teardownBrowserErrorMonitoring } = await importFreshSentry(); + await initBrowserErrorMonitoring(DSN); + const releaseClose = holdCloseOpen(mocks); + + const teardownDone = teardownBrowserErrorMonitoring(); + // Give the queued teardown operation a few turns of the microtask queue + // to run up to its `await client.close()` call, without waiting for + // `close()` itself to settle — the gate above holds that call open. + await Promise.resolve(); + await Promise.resolve(); + + expect(mocks.setClient).toHaveBeenCalledWith(undefined); + expect(mocks.close).toHaveBeenCalledTimes(1); + + releaseClose(); + await expect(teardownDone).resolves.toBeUndefined(); + }); + + it("detaches the client from the current scope even when close() rejects, and still resolves", async () => { + const mocks = mockSentryPackage(); + mocks.close.mockRejectedValueOnce(new Error("close timed out")); + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const { initBrowserErrorMonitoring, teardownBrowserErrorMonitoring } = await importFreshSentry(); + await initBrowserErrorMonitoring(DSN); + + // The returned promise must resolve, not reject: SentryGate.tsx discards + // it with a bare `void` call, so a rejection would surface as an + // unhandled promise rejection in the browser. + await expect(teardownBrowserErrorMonitoring()).resolves.toBeUndefined(); + + expect(mocks.setClient).toHaveBeenCalledWith(undefined); + expect(consoleErrorSpy).toHaveBeenCalled(); + }); + + it("stops captureBrowserException from reaching the client", async () => { + const mocks = mockSentryPackage(); + const { initBrowserErrorMonitoring, teardownBrowserErrorMonitoring, captureBrowserException } = + await importFreshSentry(); + await initBrowserErrorMonitoring(DSN); + await teardownBrowserErrorMonitoring(); + + captureBrowserException(new Error("boom after sign-out")); + await Promise.resolve(); + await Promise.resolve(); + + expect(mocks.captureException).not.toHaveBeenCalled(); + }); + + it("a call to initBrowserErrorMonitoring after teardown starts a fresh client", async () => { + const mocks = mockSentryPackage(); + const { initBrowserErrorMonitoring, teardownBrowserErrorMonitoring } = await importFreshSentry(); + await initBrowserErrorMonitoring(DSN); + await teardownBrowserErrorMonitoring(); + + await initBrowserErrorMonitoring(DSN); + + expect(mocks.init).toHaveBeenCalledTimes(2); + }); + + it("a sign-back-in that overlaps a still-in-flight teardown ends up monitored, not silently disabled", async () => { + // A sign-out whose `Sentry.close()` call is still in flight when the + // browser signs back in. `Sentry.init` and `Sentry.getCurrentScope(). + // setClient` both mutate ONE shared scope, so the old teardown's + // `setClient(undefined)` call, if it lands after the new sign-in's + // `Sentry.init`, would detach the NEW client rather than the old one — + // silently disabling monitoring for the session that just signed in. + // The fix serializes the two: the old teardown's close-and-detach must + // finish in full before the new sign-in's `Sentry.init` runs. + const mocks = mockSentryPackage(); + const { initBrowserErrorMonitoring, teardownBrowserErrorMonitoring, captureBrowserException } = + await importFreshSentry(); + await initBrowserErrorMonitoring(DSN); + expect(mocks.init).toHaveBeenCalledTimes(1); + const clientAId = mocks.attachedClientId(); + expect(clientAId).not.toBeNull(); + + const releaseClose = holdCloseOpen(mocks); + const teardownDone = teardownBrowserErrorMonitoring(); + // The sign-back-in's own `Sentry.init` is now queued behind the + // in-flight teardown, so its returned promise settles only once the + // gate below releases — await it after releasing, not before. + const signInBDone = initBrowserErrorMonitoring(DSN); + + releaseClose(); + await teardownDone; + await signInBDone; + + expect(mocks.close).toHaveBeenCalledTimes(1); + expect(mocks.init).toHaveBeenCalledTimes(2); + // The old teardown's close-and-detach ran to completion BEFORE the new + // sign-in's `Sentry.init`, so the client left attached is the new one — + // not `undefined`, and not the old client A ever reused. + const clientBId = mocks.attachedClientId(); + expect(clientBId).not.toBeNull(); + expect(clientBId).not.toBe(clientAId); + + captureBrowserException(new Error("boom after the race")); + await Promise.resolve(); + await Promise.resolve(); + + expect(mocks.captureException).toHaveBeenCalledWith(expect.any(Error)); + }); +}); + +describe("captureBrowserException", () => { + it("does not throw when the gate is closed", async () => { + const { captureBrowserException } = await importFreshSentry(); + + expect(() => captureBrowserException(new Error("boom"))).not.toThrow(); + }); + + it("reaches the client once the gate opens", async () => { + const mocks = mockSentryPackage(); + const { initBrowserErrorMonitoring, captureBrowserException } = await importFreshSentry(); + + await initBrowserErrorMonitoring(DSN); + captureBrowserException(new Error("boom")); + // captureBrowserException resolves asynchronously; give its internal + // promise a turn to settle before asserting. + await Promise.resolve(); + await Promise.resolve(); + + expect(mocks.captureException).toHaveBeenCalledWith(expect.any(Error)); + }); +}); + +/** + * Call the `integrations` option with the given default list. The real + * `@sentry/browser` option type allows `integrations` to be an array instead + * of a function, so this guard narrows it before the call — this gate always + * builds a function, never an array, but the type does not know that. + */ +function resolveIntegrations( + options: ReturnType, + defaults: Array<{ name: string }>, +) { + if (typeof options.integrations !== "function") { + throw new Error("expected buildBrowserSentryInitOptions to set a function, not an array"); + } + return options.integrations(defaults); +} + +describe("buildBrowserSentryInitOptions", () => { + it("sets the recorded built-in privacy options", async () => { + const { buildBrowserSentryInitOptions } = await importFreshSentry(); + + const options = buildBrowserSentryInitOptions(DSN); + + expect(options.sendDefaultPii).toBe(false); + expect(options.tracesSampleRate).toBe(0); + }); + + it("holds no beforeSend hook and no custom filter function", async () => { + const { buildBrowserSentryInitOptions } = await importFreshSentry(); + + const options = buildBrowserSentryInitOptions(DSN); + + expect(options.beforeSend).toBeUndefined(); + expect(options.beforeSendTransaction).toBeUndefined(); + }); + + it("the resolved integration list holds no HttpContext integration and no Breadcrumbs integration", async () => { + const { buildBrowserSentryInitOptions } = await importFreshSentry(); + + const options = buildBrowserSentryInitOptions(DSN); + const resolved = resolveIntegrations(options, DEFAULT_INTEGRATION_NAMES.map((name) => ({ name }))); + const names = resolved.map((i) => i.name); + + expect(names).not.toContain("HttpContext"); + expect(names).not.toContain("Breadcrumbs"); + }); + + it("the resolved integration list keeps GlobalHandlers, BrowserApiErrors, Dedupe, and LinkedErrors", async () => { + const { buildBrowserSentryInitOptions } = await importFreshSentry(); + + const options = buildBrowserSentryInitOptions(DSN); + const resolved = resolveIntegrations(options, DEFAULT_INTEGRATION_NAMES.map((name) => ({ name }))); + const names = resolved.map((i) => i.name); + + expect(names).toEqual( + expect.arrayContaining(["GlobalHandlers", "BrowserApiErrors", "Dedupe", "LinkedErrors"]), + ); + }); +}); + +/** + * Tests against the real `@sentry/browser` SDK. `@sentry/browser` is not an + * optional dependency here — it is a real, always-installed development + * dependency of this package (see the module comment in `sentry.ts`) — so + * these tests run unconditionally. + */ +describe("captured event shape against the real @sentry/browser SDK", () => { + /** + * Initialize the real SDK with this module's exact options, plus a + * transport stub so no event leaves the test process, plus `beforeSend` + * so the test can inspect the resolved event before it would have been + * sent. `beforeSend` here is test-only introspection — the shipped module + * adds no `beforeSend` of its own (see the "holds no beforeSend hook" + * test above). + */ + async function initRealSentryForTest(onEvent: (event: Record) => void) { + const { buildBrowserSentryInitOptions } = await importFreshSentry(); + const Sentry = await import("@sentry/browser"); + Sentry.init({ + ...buildBrowserSentryInitOptions(DSN), + transport: () => ({ send: async () => ({}), flush: async () => true }), + beforeSend: (event) => { + onEvent(event as unknown as Record); + return event; + }, + }); + return Sentry; + } + + it("an event from a page URL that holds a test capability value carries no request URL, no query string, and no referrer", async () => { + window.history.pushState({}, "", "/dashboard?token=test-capability-value"); + Object.defineProperty(document, "referrer", { + value: "https://from.example/previous-page", + configurable: true, + }); + let captured: Record | null = null; + const Sentry = await initRealSentryForTest((event) => { + captured = event; + }); + + Sentry.captureException(new Error("boom")); + await Sentry.flush(2000); + + expect(captured).not.toBeNull(); + // `HttpContext` is the only default integration that writes + // `event.request`. With it removed, the field never appears. + expect((captured as unknown as Record).request).toBeUndefined(); + }); + + it("an event captured after a console call and a fetch call carries no breadcrumb", async () => { + const originalFetch = globalThis.fetch; + globalThis.fetch = vi.fn(async () => new Response("ok")) as unknown as typeof fetch; + let captured: Record | null = null; + const Sentry = await initRealSentryForTest((event) => { + captured = event; + }); + + // eslint-disable-next-line no-console + console.warn("a console call the Breadcrumbs integration would otherwise record"); + await fetch("/api/probe?token=test-capability-value"); + Sentry.captureException(new Error("boom")); + await Sentry.flush(2000); + + globalThis.fetch = originalFetch; + expect(captured).not.toBeNull(); + expect((captured as unknown as Record).breadcrumbs).toBeUndefined(); + }); +}); diff --git a/ui/src/lib/sentry.ts b/ui/src/lib/sentry.ts new file mode 100644 index 0000000000..404f64191e --- /dev/null +++ b/ui/src/lib/sentry.ts @@ -0,0 +1,165 @@ +// Optional Sentry error monitoring for the browser. +// +// Activated only when the signed-in session carries a Sentry DSN. `SentryGate` +// reads the DSN off `GET /api/auth/get-session` and calls +// `initBrowserErrorMonitoring` once. A signed-out browser, or a browser with +// no DSN, calls this module never — see `SentryGate.tsx`. +// +// Sign-out must stop monitoring, not just stop starting it: `SentryGate` +// calls `teardownBrowserErrorMonitoring` when the session's DSN goes away. +// The function detaches the client from the current scope first +// (`Sentry.getCurrentScope().setClient(undefined)`), then closes that same +// client (`client.close()`), so a signed-out page's global handlers find no +// attached client at any point. Detach runs first because a client stays +// attached, and stays enabled, for the whole close call — closing first +// would leave the signed-out page able to send one more event to Sentry for +// as long as the close call takes. Both calls are built-in Sentry client +// calls — no custom filter code. +// +// `initBrowserErrorMonitoring`, `teardownBrowserErrorMonitoring`, and +// `captureBrowserException` each queue their work on one shared promise +// chain (`enqueue`) instead of running at once. One operation runs at a +// time, in call order, so no operation can ever observe another one +// part-finished — a sign-out always closes the client a sign-in already +// finished starting, never one still starting. +// +// `@sentry/browser` loads through a dynamic import, so Vite puts it in a +// separate chunk that a browser with no DSN never fetches. +// +// Default-integration privacy note: two default integrations copy values +// this app does not want inside a Sentry event, so the initializer removes +// them with a built-in Sentry option — no custom filter code: +// - `Breadcrumbs` turns a console call, a click, and a fetch call into a +// breadcrumb with the raw arguments and the raw request URL. +// - `HttpContext` copies the page URL, the query string, and the referrer +// onto every event. +// The initializer keeps every other default integration, so the browser +// still captures `window.onerror` and `window.onunhandledrejection` +// (`GlobalHandlers`), the two React error boundaries, deduplicates a repeat +// event (`Dedupe`), and links a caused-by chain (`LinkedErrors`). + +let queue: Promise = Promise.resolve(); + +/** Run gate operations one at a time, in call order. */ +function enqueue(op: () => Promise): Promise { + const next = queue.then(op); + queue = next.catch(() => {}); + return next; +} + +/** The `@sentry/browser` module shape, resolved once. */ +type SentryBrowserModule = typeof import("@sentry/browser"); + +/** + * The `Sentry.init` options this gate builds. `Sentry.init`'s parameter is + * optional, so `Parameters<...>[0]` alone carries an `| undefined` arm this + * gate never returns. `NonNullable` removes only that arm — the object shape + * underneath stays the true `@sentry/browser` option type. + */ +type BrowserSentryInitOptions = NonNullable[0]>; + +let sentry: SentryBrowserModule | null = null; + +/** + * Load `@sentry/browser` and start the client with the given DSN. Idempotent + * — the session query can refetch and call this again, and a second call is + * a no-op because a client is already started. + */ +export function initBrowserErrorMonitoring(dsn: string): Promise { + return enqueue(async () => { + if (sentry) return; + try { + const Sentry = await import("@sentry/browser"); + Sentry.init(buildBrowserSentryInitOptions(dsn)); + sentry = Sentry; + } catch (err) { + // The dynamic import or the init call failed. Fall through with a + // single diagnostic. The gate fails open — the app keeps running + // without error monitoring rather than crashing on an opt-in feature. + // eslint-disable-next-line no-console + console.error("[paperclip] Sentry browser bootstrap failed", err); + } + }); +} + +/** + * Stop browser error monitoring and forget the started client. Call this on + * sign-out, so the browser sends Sentry no more events and no more + * breadcrumbs after the session ends. A no-op when monitoring never started. + * + * Detaches the client from the current scope, then closes that same client. + * A client stays attached, and stays enabled, for the whole close call — a + * signed-out page's `window.onerror` or `window.onunhandledrejection` + * handler would still reach it for as long as the close call takes. + * Detaching first removes that window instead of leaving it open. + */ +export function teardownBrowserErrorMonitoring(): Promise { + return enqueue(async () => { + const Sentry = sentry; + sentry = null; + if (!Sentry) return; + // Read the client before detaching it. The scope holds no client after + // detach, so the close step below needs this reference to flush and + // close the right client. + const client = Sentry.getClient(); + // Detach first, close second — the opposite order from a plain + // `Sentry.close()` call, which reads the client off the current scope + // and would find nothing to close if detach ran first. A separate + // guarded try keeps this step unable to reject, unlike a bare finally + // block: a thrown error here would still reach the caller, and + // `SentryGate.tsx` discards this function's returned promise with a + // bare `void` call, so a rejection would surface as an unhandled + // promise rejection in the browser. + try { + Sentry.getCurrentScope().setClient(undefined); + } catch (err) { + // eslint-disable-next-line no-console + console.error("[paperclip] Sentry client detach failed", err); + } + // A second, separate guarded try, for the same reason as the one + // above: this step must never reject the returned promise. + try { + // Awaiting matters: the client flushes buffered events to Sentry + // during close; a caller that does not wait may navigate away, or + // the page may unload, before the flush finishes. + await client?.close(2_000); + } catch (err) { + // eslint-disable-next-line no-console + console.error("[paperclip] Sentry teardownBrowserErrorMonitoring failed", err); + } + }); +} + +/** + * Report an error to Sentry. Never throws — observability must not change + * control flow. A no-op before the gate opens, when the gate never opens (no + * DSN on the session), or when bootstrap failed. + */ +export function captureBrowserException(error: unknown): void { + void enqueue(async () => { + try { + sentry?.captureException(error); + } catch (err) { + // eslint-disable-next-line no-console + console.error("[paperclip] Sentry captureBrowserException failed", err); + } + }); +} + +/** + * Build the `Sentry.init` options object. A pure function, split out from + * `initBrowserErrorMonitoring` so a test can call it with the real + * `@sentry/browser` module and assert the resolved integration list and the + * captured-event shape against the true SDK, not a stand-in. + */ +export function buildBrowserSentryInitOptions(dsn: string): BrowserSentryInitOptions { + return { + dsn, + tracesSampleRate: 0, + sendDefaultPii: false, + integrations: (defaults) => + defaults.filter( + (integration) => integration.name !== "HttpContext" && integration.name !== "Breadcrumbs", + ), + }; +} diff --git a/ui/src/main.tsx b/ui/src/main.tsx index c6aa4eb559..6496fe3558 100644 --- a/ui/src/main.tsx +++ b/ui/src/main.tsx @@ -6,6 +6,7 @@ import { BrowserRouter } from "@/lib/router"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { App } from "./App"; import { AppErrorBoundary } from "./components/AppErrorBoundary"; +import { SentryGate } from "./components/SentryGate"; import { CompanyProvider, useCompany } from "./context/CompanyContext"; import { LiveUpdatesProvider } from "./context/LiveUpdatesProvider"; import { BreadcrumbProvider } from "./context/BreadcrumbContext"; @@ -60,6 +61,7 @@ createRoot(document.getElementById("root")!).render( + diff --git a/ui/storybook/fixtures/paperclipData.ts b/ui/storybook/fixtures/paperclipData.ts index ee9d4a3534..0bafed389b 100644 --- a/ui/storybook/fixtures/paperclipData.ts +++ b/ui/storybook/fixtures/paperclipData.ts @@ -119,6 +119,7 @@ export const storybookAuthSession: AuthSession = { email: "riley@paperclip.local", image: null, }, + sentryDsn: null, }; export const storybookAgents: Agent[] = [