diff --git a/.github/workflows/sentry-contract.yml b/.github/workflows/sentry-contract.yml new file mode 100644 index 00000000000..f4bb2bcebbb --- /dev/null +++ b/.github/workflows/sentry-contract.yml @@ -0,0 +1,62 @@ +name: Sentry SDK contract + +on: + pull_request: + paths: + - .github/workflows/sentry-contract.yml + - server/package.json + - server/src/sentry*.ts + - server/src/peer-version-check.ts + - server/src/__tests__/*sentry*.test.ts + push: + branches: [master] + paths: + - .github/workflows/sentry-contract.yml + - server/package.json + - server/src/sentry*.ts + - server/src/peer-version-check.ts + - server/src/__tests__/*sentry*.test.ts + +permissions: + contents: read + +concurrency: + group: sentry-contract-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + sentry-contract: + name: Real Sentry SDK isolation + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + + - name: Setup Node.js + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7 + with: + node-version: 24 + package-manager-cache: false + + - name: Setup pnpm + uses: pnpm/action-setup@0977fd99725f1db4007ccb2928dbb4e90d06cc86 # v6 + with: + version: 9.15.4 + + - name: Install workspace dependencies + run: pnpm install --frozen-lockfile + + - name: Install the audited optional SDK outside the workspace + shell: bash + run: | + sentry_version=$(node -p 'require("./server/package.json").peerDependencies["@sentry/node"]') + npm install --prefix "$RUNNER_TEMP/sentry-contract-sdk" --ignore-scripts --no-audit --no-fund --package-lock=false "@sentry/node@$sentry_version" + + - name: Verify run failure isolation with the real SDK + env: + NODE_PATH: ${{ runner.temp }}/sentry-contract-sdk/node_modules + PAPERCLIP_REQUIRE_SENTRY_TEST_SDK: "1" + run: pnpm --filter @paperclipai/server exec vitest run src/__tests__/run-failure-sentry-real-sdk.test.ts diff --git a/doc/observability.md b/doc/observability.md index 9664f99c24d..41baaa13b92 100644 --- a/doc/observability.md +++ b/doc/observability.md @@ -373,6 +373,15 @@ context the other kept default integrations add: the host name, the runtime version, and the dependency list. See "Default capture set" below. +### Run failure context + +Terminal run failures also carry the run and task IDs, adapter, error code, +run status, and redacted error message. Their fingerprint is the error code +and adapter. These fields are passed directly to that event's capture call. +They do not change the ambient Sentry scope, whose isolation is unavailable +without an OpenTelemetry context manager. Later, unrelated exceptions must +not inherit a previous run's identity or fingerprint. + ### Browser data The browser sends no page URL, no referrer, no user agent, and no diff --git a/server/src/__tests__/http-log-redaction.test.ts b/server/src/__tests__/http-log-redaction.test.ts index a16e56e69a5..62def738d8b 100644 --- a/server/src/__tests__/http-log-redaction.test.ts +++ b/server/src/__tests__/http-log-redaction.test.ts @@ -399,6 +399,35 @@ describe("HTTP logger redaction", () => { expect(log.res.headers["set-cookie"]).toBe("[Redacted]"); }); + it.each([200, 403, 500])("redacts runtime GitHub capabilities from HTTP %i logs", async (status) => { + const capability = "runtime-github-capability-canary"; + const chunks: string[] = []; + const stream = new Writable({ + write(chunk, _encoding, callback) { + chunks.push(chunk.toString()); + callback(); + }, + }); + const app = express(); + app.use(createHttpLogger(pino({ redact: [...HTTP_LOG_REDACT_PATHS] }, stream))); + app.post("/runtime-tools/github/credentials", (_req, res) => { + res.status(status).json({ status }); + }); + + await request(app) + .post("/runtime-tools/github/credentials") + .set("X-Paperclip-Github-Capability", capability) + .send({}) + .expect(status); + + const output = chunks.join(""); + expect(output).not.toContain(capability); + const log = JSON.parse(output.trim()); + expect(log.req.headers["x-paperclip-github-capability"]).toBe("[Redacted]"); + expect(log.req.url).toBe("/runtime-tools/github/credentials"); + expect(log.res.statusCode).toBe(status); + }); + it("drops OAuth callback query data from the message and structured request", async () => { const chunks: string[] = []; const stream = new Writable({ diff --git a/server/src/__tests__/run-failure-sentry-real-sdk.test.ts b/server/src/__tests__/run-failure-sentry-real-sdk.test.ts new file mode 100644 index 00000000000..524b52445c0 --- /dev/null +++ b/server/src/__tests__/run-failure-sentry-real-sdk.test.ts @@ -0,0 +1,105 @@ +import { createRequire } from "node:module"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +// Sentry is an optional peer. When installed, exercise the real SDK with an +// in-memory transport, including its context behavior without an OTel manager. +const sentryPackage = (() => { + try { + const require = createRequire(import.meta.url); + return require("@sentry/node") as { + init(options: Record): unknown; + captureException(error: unknown, context?: unknown): string; + withScope(callback: (scope: unknown) => void): void; + httpIntegration(options: { breadcrumbs: boolean }): { name: string }; + onUnhandledRejectionIntegration(options: { mode: string }): { name: string }; + flush(timeout?: number): Promise; + close(timeout?: number): Promise; + }; + } catch { + return null; + } +})(); + +if (process.env.PAPERCLIP_REQUIRE_SENTRY_TEST_SDK === "1" && !sentryPackage) { + throw new Error("The Sentry SDK contract job requires the audited optional peer"); +} + +afterEach(async () => { + await sentryPackage?.close(2000); + vi.unstubAllEnvs(); + vi.doUnmock("@sentry/node"); + vi.doUnmock("../peer-version-check.js"); + vi.resetModules(); +}); + +describe.skipIf(!sentryPackage)("run failure context with the real Sentry SDK", () => { + it("keeps each run's identity and fingerprint off unrelated errors", async () => { + const Sentry = sentryPackage!; + const events: Array> = []; + vi.stubEnv("SENTRY_DSN_BACKEND", "https://public@example.invalid/1"); + vi.doMock("../peer-version-check.js", () => ({ + checkExactPeerVersions: () => ({ ok: true }), + })); + vi.doMock("@sentry/node", () => ({ + ...Sentry, + init: (options: Record) => Sentry.init({ + ...options, + transport: () => ({ send: async () => ({}), flush: async () => true }), + beforeSend: (event: Record) => { + events.push(event); + return event; + }, + }), + })); + vi.resetModules(); + const { sentryReady, captureRunFailure, captureException } = await import("../sentry.js"); + await sentryReady; + + const first = { + taskId: "11111111-1111-4111-8111-111111111111", + runId: "22222222-2222-4222-8222-222222222222", + errorMessage: "first run failed", + errorCode: "adapter_failed", + agentAdapter: "fixture-adapter", + runStatus: "failed" as const, + }; + const second = { + ...first, + taskId: "33333333-3333-4333-8333-333333333333", + runId: "44444444-4444-4444-8444-444444444444", + errorMessage: "second run timed out", + errorCode: "timeout", + agentAdapter: "other-adapter", + runStatus: "timed_out" as const, + }; + captureRunFailure(first); + await Promise.resolve(); + captureException(new Error("unrelated database error")); + captureRunFailure(second); + captureException(new Error("unrelated filesystem error")); + await Sentry.flush(2000); + + expect(events).toHaveLength(4); + const captured = (message: string) => events.find((event) => + (event.exception as { values: Array<{ value: string }> }).values[0]?.value === message, + ); + for (const run of [first, second]) { + expect(captured(run.errorMessage)).toMatchObject({ + tags: { run_id: run.runId, task_id: run.taskId, error_code: run.errorCode, + agent_adapter: run.agentAdapter, run_status: run.runStatus }, + contexts: { run_failure: { runId: run.runId, taskId: run.taskId, + errorMessage: run.errorMessage, errorCode: run.errorCode, agentAdapter: run.agentAdapter } }, + fingerprint: [run.errorCode, run.agentAdapter], + }); + } + for (const message of ["unrelated database error", "unrelated filesystem error"]) { + const event = captured(message); + expect(event).toBeDefined(); + expect(event).not.toHaveProperty("contexts.run_failure"); + expect(event).not.toHaveProperty("fingerprint"); + for (const tag of ["run_id", "task_id", "error_code", "agent_adapter", "run_status"]) { + expect(event).not.toHaveProperty(`tags.${tag}`); + } + } + }); +}); diff --git a/server/src/__tests__/run-failure-sentry.test.ts b/server/src/__tests__/run-failure-sentry.test.ts index aa6f51ff431..855b6b66853 100644 --- a/server/src/__tests__/run-failure-sentry.test.ts +++ b/server/src/__tests__/run-failure-sentry.test.ts @@ -7,7 +7,7 @@ import type { RunFailureEvent } from "../sentry.js"; * installed in this test environment, so each test mocks a fake copy of the * package the same way `sentry.test.ts` does, then imports a fresh copy of * `../sentry.js` with a backend DSN set. That opens the gate and lets the - * fake package's `withScope`/`captureException` spies record the real call + * fake package's `captureException` spy records the real call * shape. */ @@ -30,16 +30,16 @@ function baseEvent(overrides: Partial = {}): RunFailureEvent { }; } -/** Mirrors `sentry.test.ts`'s `mockSentryPackage`, plus a `withScope` spy. */ +interface CaptureContext { + tags: Record; + contexts: Record>; + fingerprint: string[]; +} + +/** Mirrors `sentry.test.ts`'s `mockSentryPackage`. */ function mockSentryPackage() { - const scope = { - setTag: vi.fn(), - setContext: vi.fn(), - setFingerprint: vi.fn(), - }; const init = vi.fn(); - const captureException = vi.fn(() => "event-id"); - const withScope = vi.fn((callback: (s: typeof scope) => void) => callback(scope)); + const captureException = vi.fn((_error: unknown, _context?: CaptureContext) => "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) => ({ @@ -50,7 +50,6 @@ function mockSentryPackage() { vi.doMock("@sentry/node", () => ({ init, captureException, - withScope, close, httpIntegration, onUnhandledRejectionIntegration, @@ -59,7 +58,7 @@ function mockSentryPackage() { checkExactPeerVersions: () => ({ ok: true }), })); - return { scope, init, captureException, withScope, close }; + return { init, captureException, close }; } async function importFreshSentryWithGateOpen() { @@ -91,15 +90,15 @@ afterEach(() => { describe("captureRunFailure", () => { it("sets the fingerprint [errorCode, agentAdapter] in that order", async () => { - const { sentryModule, scope } = await importFreshSentryWithGateOpen(); + const { sentryModule, captureException } = await importFreshSentryWithGateOpen(); sentryModule.captureRunFailure(baseEvent({ errorCode: "timeout", agentAdapter: "codex" })); - expect(scope.setFingerprint).toHaveBeenCalledWith(["timeout", "codex"]); + expect(captureException.mock.calls[0]![1]?.fingerprint).toEqual(["timeout", "codex"]); }); it("keeps the error message out of the fingerprint for two runs with the same code and adapter but different messages", async () => { - const { sentryModule, scope } = await importFreshSentryWithGateOpen(); + const { sentryModule, captureException } = await importFreshSentryWithGateOpen(); sentryModule.captureRunFailure( baseEvent({ errorMessage: "message one", errorCode: "timeout", agentAdapter: "codex" }), @@ -108,25 +107,25 @@ describe("captureRunFailure", () => { baseEvent({ errorMessage: "message two", errorCode: "timeout", agentAdapter: "codex" }), ); - expect(scope.setFingerprint).toHaveBeenNthCalledWith(1, ["timeout", "codex"]); - expect(scope.setFingerprint).toHaveBeenNthCalledWith(2, ["timeout", "codex"]); + expect(captureException.mock.calls[0]![1]?.fingerprint).toEqual(["timeout", "codex"]); + expect(captureException.mock.calls[1]![1]?.fingerprint).toEqual(["timeout", "codex"]); }); it("sets the fingerprint element \"unknown\" when the error code is absent", async () => { - const { sentryModule, scope } = await importFreshSentryWithGateOpen(); + const { sentryModule, captureException } = await importFreshSentryWithGateOpen(); sentryModule.captureRunFailure(baseEvent({ errorCode: null })); - expect(scope.setFingerprint).toHaveBeenCalledWith(["unknown", "claude-code"]); + expect(captureException.mock.calls[0]![1]?.fingerprint).toEqual(["unknown", "claude-code"]); }); it("sets the five diagnostic values on the run_failure context", async () => { - const { sentryModule, scope } = await importFreshSentryWithGateOpen(); + const { sentryModule, captureException } = await importFreshSentryWithGateOpen(); const event = baseEvent(); sentryModule.captureRunFailure(event); - expect(scope.setContext).toHaveBeenCalledWith("run_failure", { + expect(captureException.mock.calls[0]![1]?.contexts.run_failure).toEqual({ taskId: event.taskId, runId: event.runId, errorMessage: event.errorMessage, @@ -136,25 +135,27 @@ describe("captureRunFailure", () => { }); it("does not set an instance key on the run_failure context", async () => { - const { sentryModule, scope } = await importFreshSentryWithGateOpen(); + const { sentryModule, captureException } = await importFreshSentryWithGateOpen(); sentryModule.captureRunFailure(baseEvent()); - const [, context] = scope.setContext.mock.calls[0]!; + const context = captureException.mock.calls[0]![1]?.contexts.run_failure; expect(context).not.toHaveProperty("instance"); }); it("sets run_id, task_id, error_code, agent_adapter, and run_status as tags", async () => { - const { sentryModule, scope } = await importFreshSentryWithGateOpen(); + const { sentryModule, captureException } = await importFreshSentryWithGateOpen(); const event = baseEvent(); sentryModule.captureRunFailure(event); - expect(scope.setTag).toHaveBeenCalledWith("run_id", event.runId); - expect(scope.setTag).toHaveBeenCalledWith("task_id", event.taskId); - expect(scope.setTag).toHaveBeenCalledWith("error_code", event.errorCode); - expect(scope.setTag).toHaveBeenCalledWith("agent_adapter", event.agentAdapter); - expect(scope.setTag).toHaveBeenCalledWith("run_status", event.runStatus); + expect(captureException.mock.calls[0]![1]?.tags).toEqual({ + run_id: event.runId, + task_id: event.taskId, + error_code: event.errorCode, + agent_adapter: event.agentAdapter, + run_status: event.runStatus, + }); }); it("captures the redacted error message as the exception message", async () => { @@ -178,8 +179,8 @@ describe("captureRunFailure", () => { }); it("does not throw when the client throws", async () => { - const { sentryModule, withScope } = await importFreshSentryWithGateOpen(); - withScope.mockImplementation(() => { + const { sentryModule, captureException } = await importFreshSentryWithGateOpen(); + captureException.mockImplementation(() => { throw new Error("sentry client is down"); }); diff --git a/server/src/middleware/http-log-redaction.ts b/server/src/middleware/http-log-redaction.ts index a7d7ed05f25..8b50c5cf371 100644 --- a/server/src/middleware/http-log-redaction.ts +++ b/server/src/middleware/http-log-redaction.ts @@ -10,6 +10,8 @@ export const HTTP_LOG_REDACT_PATHS = [ 'req.headers["x-csrf-token"]', 'req.headers["x-xsrf-token"]', 'req.headers["x-api-key"]', + // Runtime GitHub capabilities authorize credential acquisition for a live run. + 'req.headers["x-paperclip-github-capability"]', // Telegram's optional webhook verification header is a reusable bearer // secret sent on every provider callback. 'req.headers["x-telegram-bot-api-secret-token"]', diff --git a/server/src/sentry.ts b/server/src/sentry.ts index 2b8c59423e4..424ba16f2e3 100644 --- a/server/src/sentry.ts +++ b/server/src/sentry.ts @@ -72,17 +72,16 @@ if (legacyFallbackUsed) { ); } -/** The subset of the `@sentry/node` scope surface `captureRunFailure` calls. */ -interface SentryScopeLike { - setTag(key: string, value: string): void; - setContext(name: string, context: Record | null): void; - setFingerprint(fingerprint: string[]): void; +/** Event-local context accepted by the optional Sentry package. */ +interface SentryCaptureContext { + tags: Record; + contexts: Record>; + fingerprint: string[]; } /** The subset of the `@sentry/node` client surface this gate calls. */ interface SentryHandle { - captureException(error: unknown): string; - withScope(callback: (scope: SentryScopeLike) => void): void; + captureException(error: unknown, context?: SentryCaptureContext): string; close(timeout?: number): Promise; } @@ -149,22 +148,28 @@ export function captureRunFailure(event: RunFailureEvent): void { if (!sentryHandle) return; const handle = sentryHandle; try { - handle.withScope((scope) => { - const errorCode = event.errorCode ?? "unknown"; - scope.setTag("run_id", event.runId); - scope.setTag("task_id", event.taskId); - scope.setTag("error_code", errorCode); - scope.setTag("agent_adapter", event.agentAdapter); - scope.setTag("run_status", event.runStatus); - scope.setContext("run_failure", { - taskId: event.taskId, - runId: event.runId, - errorMessage: event.errorMessage, - errorCode, - agentAdapter: event.agentAdapter, - }); - scope.setFingerprint([errorCode, event.agentAdapter]); - handle.captureException(new Error(event.errorMessage)); + const errorCode = event.errorCode ?? "unknown"; + // Sentry's async scope isolation is absent when OTel setup is skipped. + // A withScope mutation can then persist into unrelated later captures. + // Pass these fields on this event only; do not mutate the ambient scope. + handle.captureException(new Error(event.errorMessage), { + tags: { + run_id: event.runId, + task_id: event.taskId, + error_code: errorCode, + agent_adapter: event.agentAdapter, + run_status: event.runStatus, + }, + contexts: { + run_failure: { + taskId: event.taskId, + runId: event.runId, + errorMessage: event.errorMessage, + errorCode, + agentAdapter: event.agentAdapter, + }, + }, + fingerprint: [errorCode, event.agentAdapter], }); } catch (err) { // eslint-disable-next-line no-console @@ -281,8 +286,7 @@ async function bootstrapSentry(dsn: string): Promise { Sentry.init(buildSentryInitOptions(dsn, Sentry)); sentryHandle = { - captureException: (error) => Sentry.captureException(error), - withScope: (callback) => Sentry.withScope(callback), + captureException: (...args) => Sentry.captureException(...args), close: (timeout) => Sentry.close(timeout), }; } catch (err) {