From e5bf9d49a511c371c3bccfde95961cb6c34ee10c Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Tue, 29 Sep 2026 10:29:17 -0700 Subject: [PATCH] fix(sentry): retain recorded process exit details (#14575) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip manages agents and records their task runs. > - Operators can enable Sentry reports for terminal run failures. > - A failed adapter can leave only the generic message “Adapter failed.” > - The run already records a process exit code and signal, but the report omits them. > - This pull request carries those two values through a strict capture boundary. > - Operators can distinguish a nonzero exit from signal termination when the message is generic. ## Linked Issues or Issue Description **What happened?** A failed run can store `exitCode: 1` or `signal: "SIGTERM"` while its Sentry event contains only `adapter_failed` and “Adapter failed.” The existing reporter drops both recorded fields. This occurs on the current master reporting path. **Expected behavior** The opt-in report preserves bounded process exit evidence without exporting adapter output or changing run behavior. **Steps to reproduce** Enable the backend Sentry DSN and report a failed run whose message is “Adapter failed” and whose stored signal is `SIGTERM`. Before this change, the event has no signal field. After this change, `run_failure.signal` is `SIGTERM` and the existing fingerprint stays the same. Related: #12105 and #8222 describe missing adapter/HTTP failure details. #13152 changes terminal-result cleanup classification, and #12886 adds process-failure classification and runtime URL checks. None forwards these stored fields through the Sentry reporter. This change does not resolve those broader issues. ## What Changed - Forward the stored exit code and signal from the terminal run reporter. - Accept only signed 32-bit integer exit codes; use `null` for missing or malformed values. - Accept only the reporting host's Node signal constants; use `null` for missing values and `unknown` for unrecognized values. - Keep the added fields in event-local context, outside tags and fingerprints. - Cover database-backed reporting, malformed input, privacy, and isolation through the real Sentry SDK. - Document the fields and their limits. - Give the dedicated Sentry job the normal PR dependency-resolution fallback, with lifecycle scripts disabled on every install and the required real-SDK test retained. ## Verification - Before the change: 16 report-shape/exit-field assertions failed in the focused capture suite. - After the change: 91 focused Sentry, DSN, and database-backed reporting tests passed. The real SDK uses an in-memory transport. - Final real-SDK test also passed with malformed metadata; it verifies that arbitrary signal text is absent from captured events and unrelated errors inherit no run context. - `pnpm -r typecheck`: passed. - `pnpm build`: passed. - `pnpm test:run`: exited nonzero after 718 general-server files: 14,085 tests passed, 14 failed, 86 skipped. Thirteen skill-service/cache failures reproduce on the unchanged base commit on this macOS host. One comment-wake test timed out; the complete 29-test suite passes on the unchanged base and in final-head Linux CI. Local isolated rechecks skipped because embedded PostgreSQL could not start; these are not counted as passes. The remaining local workspace/serialized lanes did not run after the failing first lane; all CI lanes passed. - Initial Sentry CI failed before tests with `ERR_PNPM_LOCKFILE_CONFIG_MISMATCH`. Its install step lacked the normal PR fallback. The repaired real-SDK job passed. Security review then requested disabling lifecycle scripts for resolved dependencies; every install now uses `--ignore-scripts`. A fresh isolated checkout passed the exact script-disabled fallback and real-SDK contract. Final-head real-SDK CI and the security scan passed. - GitHub CI: all 54 checks passed on `37e0a836e50660f7753d367bcf5a4959eaf89b90`, including required `ci / verify` and `ci / e2e`; two unrelated checks skipped. - Greptile: 5/5 on that commit. No unresolved review comments. - Merge status: conflict-free; required CODEOWNER approval for the workflow change is still pending. ## Risks Low risk: this only adds two validated fields to existing opt-in error reports. It changes no database schema, run status, retry, fingerprint, or suppression rule. Process output and adapter result payloads remain excluded. A recorded signal does not identify its sender or prove an out-of-memory kill. Missing exit evidence stays unknown; this change does not establish the cause of a historical generic adapter failure. ## Model Used OpenAI GPT-6 (Codex), with reasoning, terminal tools, and code execution. The context window size is not exposed in this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes:` / `Closes` / `Refs` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub references) - [x] My branch name describes the change and contains no internal ticket id or instance-derived details - [x] I have run focused tests locally and they pass; broader validation is recorded above - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- .github/workflows/sentry-contract.yml | 9 +++- doc/observability.md | 9 ++++ .../run-failure-sentry-real-sdk.test.ts | 20 +++++++- .../src/__tests__/run-failure-sentry.test.ts | 47 ++++++++++++++++++- server/src/sentry.ts | 24 ++++++++++ .../__tests__/run-failure-report.test.ts | 29 ++++++++++++ server/src/services/run-failure-report.ts | 2 + 7 files changed, 136 insertions(+), 4 deletions(-) diff --git a/.github/workflows/sentry-contract.yml b/.github/workflows/sentry-contract.yml index f4bb2bcebbb..87515150a0e 100644 --- a/.github/workflows/sentry-contract.yml +++ b/.github/workflows/sentry-contract.yml @@ -47,7 +47,14 @@ jobs: version: 9.15.4 - name: Install workspace dependencies - run: pnpm install --frozen-lockfile + # Master owns the lockfile; PR verification can start before its refresh. + # Resolve like the normal PR jobs, but this narrow contract needs no + # dependency or workspace lifecycle scripts, including on the fast path. + run: | + if ! pnpm install --frozen-lockfile --ignore-scripts; then + pnpm install --resolution-only --ignore-scripts --no-frozen-lockfile + pnpm install --frozen-lockfile --ignore-scripts + fi - name: Install the audited optional SDK outside the workspace shell: bash diff --git a/doc/observability.md b/doc/observability.md index 491e8e35c85..8a379342a1d 100644 --- a/doc/observability.md +++ b/doc/observability.md @@ -406,6 +406,15 @@ 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. +The `run_failure` context also includes the recorded process `exitCode` and +`signal`, so a generic adapter error can still distinguish a nonzero exit from +a signal termination. Exit codes must fit the database's signed 32-bit integer; +missing or malformed values become `null`. Signals must match the reporting +host's Node signal constants; missing values become `null` and unrecognized +values become `unknown`. A signal such as `SIGKILL` does not establish who sent +it or prove an out-of-memory kill. These fields do not change error grouping +or run outcomes, and do not include process output or adapter result payloads. + ### Browser data The browser sends no page URL, no referrer, no user agent, and no diff --git a/server/src/__tests__/run-failure-sentry-real-sdk.test.ts b/server/src/__tests__/run-failure-sentry-real-sdk.test.ts index 524b52445c0..5cfe758e2bb 100644 --- a/server/src/__tests__/run-failure-sentry-real-sdk.test.ts +++ b/server/src/__tests__/run-failure-sentry-real-sdk.test.ts @@ -62,6 +62,8 @@ describe.skipIf(!sentryPackage)("run failure context with the real Sentry SDK", errorCode: "adapter_failed", agentAdapter: "fixture-adapter", runStatus: "failed" as const, + exitCode: 1, + signal: null, }; const second = { ...first, @@ -71,15 +73,23 @@ describe.skipIf(!sentryPackage)("run failure context with the real Sentry SDK", errorCode: "timeout", agentAdapter: "other-adapter", runStatus: "timed_out" as const, + exitCode: null, + signal: "SIGKILL", }; captureRunFailure(first); await Promise.resolve(); captureException(new Error("unrelated database error")); captureRunFailure(second); + captureRunFailure({ + ...first, + errorMessage: "malformed process metadata", + exitCode: NaN, + signal: "private-signal-payload", + }); captureException(new Error("unrelated filesystem error")); await Sentry.flush(2000); - expect(events).toHaveLength(4); + expect(events).toHaveLength(5); const captured = (message: string) => events.find((event) => (event.exception as { values: Array<{ value: string }> }).values[0]?.value === message, ); @@ -88,10 +98,16 @@ describe.skipIf(!sentryPackage)("run failure context with the real Sentry SDK", 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 } }, + errorMessage: run.errorMessage, errorCode: run.errorCode, agentAdapter: run.agentAdapter, + exitCode: run.exitCode, signal: run.signal } }, fingerprint: [run.errorCode, run.agentAdapter], }); } + expect(captured("malformed process metadata")).toMatchObject({ + contexts: { run_failure: { exitCode: null, signal: "unknown" } }, + fingerprint: [first.errorCode, first.agentAdapter], + }); + expect(JSON.stringify(events)).not.toContain("private-signal-payload"); for (const message of ["unrelated database error", "unrelated filesystem error"]) { const event = captured(message); expect(event).toBeDefined(); diff --git a/server/src/__tests__/run-failure-sentry.test.ts b/server/src/__tests__/run-failure-sentry.test.ts index 855b6b66853..d5285698a43 100644 --- a/server/src/__tests__/run-failure-sentry.test.ts +++ b/server/src/__tests__/run-failure-sentry.test.ts @@ -119,7 +119,7 @@ describe("captureRunFailure", () => { expect(captureException.mock.calls[0]![1]?.fingerprint).toEqual(["unknown", "claude-code"]); }); - it("sets the five diagnostic values on the run_failure context", async () => { + it("keeps missing process exit evidence null on the run_failure context", async () => { const { sentryModule, captureException } = await importFreshSentryWithGateOpen(); const event = baseEvent(); @@ -131,9 +131,54 @@ describe("captureRunFailure", () => { errorMessage: event.errorMessage, errorCode: event.errorCode, agentAdapter: event.agentAdapter, + exitCode: null, + signal: null, }); }); + it.each([ + { exitCode: 1, signal: null }, + { exitCode: 0, signal: null }, + { exitCode: -1, signal: null }, + { exitCode: -2147483648, signal: null }, + { exitCode: 2147483647, signal: null }, + { exitCode: null, signal: "SIGTERM" }, + { exitCode: null, signal: "SIGKILL" }, + ])("retains process exit evidence without changing grouping: %j", async (processExit) => { + const { sentryModule, captureException } = await importFreshSentryWithGateOpen(); + const event = baseEvent({ ...processExit, errorMessage: "Adapter failed", errorCode: "adapter_failed" }); + + sentryModule.captureRunFailure(event); + + const [error, context] = captureException.mock.calls[0]!; + expect((error as Error).message).toBe("Adapter failed"); + expect(context?.contexts.run_failure).toMatchObject(processExit); + expect(context?.fingerprint).toEqual(["adapter_failed", event.agentAdapter]); + expect(context?.tags).not.toHaveProperty("exitCode"); + expect(context?.tags).not.toHaveProperty("signal"); + }); + + it.each([ + ["private-exit-payload", "SIGTERM private-signal-payload"], + ["1", "constructor"], + [true, "__proto__"], + [1.5, "SIGCUSTOM"], + [NaN, ""], + [Infinity, 9], + [2147483648, { private: "signal-payload" }], + [-2147483649, ["SIGTERM"]], + ])("rejects malformed process exit fields (%j, %j)", async (exitCode, signal) => { + const { sentryModule, captureException } = await importFreshSentryWithGateOpen(); + + sentryModule.captureRunFailure({ ...baseEvent(), exitCode, signal } as RunFailureEvent); + + expect(captureException.mock.calls[0]![1]?.contexts.run_failure).toMatchObject({ + exitCode: null, + signal: "unknown", + }); + expect(JSON.stringify(captureException.mock.calls)).not.toContain("private-"); + }); + it("does not set an instance key on the run_failure context", async () => { const { sentryModule, captureException } = await importFreshSentryWithGateOpen(); diff --git a/server/src/sentry.ts b/server/src/sentry.ts index 424ba16f2e3..1a9c30f4642 100644 --- a/server/src/sentry.ts +++ b/server/src/sentry.ts @@ -132,6 +132,28 @@ export interface RunFailureEvent { agentAdapter: string; /** The run status that triggered this report. */ runStatus: RunFailureStatus; + /** Recorded process exit evidence, when available. Validated before capture. */ + exitCode?: number | null; + signal?: string | null; +} + +function normalizeRunExitCode(value: unknown): number | null { + // Match the persisted PostgreSQL integer. Do not coerce adapter-supplied text. + return typeof value === "number" && + Number.isInteger(value) && + value >= -2147483648 && + value <= 2147483647 + ? value + : null; +} + +function normalizeRunSignal(value: unknown): string | null { + if (value == null) return null; + // Only host signal constants may leave the process; arbitrary adapter text + // can contain output or credentials even when stored in the signal column. + return typeof value === "string" && Object.hasOwn(os.constants.signals, value) + ? value + : "unknown"; } /** @@ -167,6 +189,8 @@ export function captureRunFailure(event: RunFailureEvent): void { errorMessage: event.errorMessage, errorCode, agentAdapter: event.agentAdapter, + exitCode: normalizeRunExitCode(event.exitCode), + signal: normalizeRunSignal(event.signal), }, }, fingerprint: [errorCode, event.agentAdapter], diff --git a/server/src/services/__tests__/run-failure-report.test.ts b/server/src/services/__tests__/run-failure-report.test.ts index 75ba13de1fe..9b32245e763 100644 --- a/server/src/services/__tests__/run-failure-report.test.ts +++ b/server/src/services/__tests__/run-failure-report.test.ts @@ -112,6 +112,35 @@ describeEmbeddedPostgres("reportRunFailure", () => { ); }); + it("forwards stored exit evidence even when the adapter error is generic", async () => { + await seedCompanyAndAgent(); + for (const processExit of [ + { exitCode: 1, signal: null }, + { exitCode: null, signal: "SIGTERM" }, + { exitCode: null, signal: null }, + ]) { + mockCaptureRunFailure.mockClear(); + await reportRunFailure(db, buildRun({ + error: "Adapter failed", + errorCode: "adapter_failed", + ...processExit, + stdoutExcerpt: "private-output", + stderrExcerpt: "private-error-output", + resultJson: { private: "adapter-result" }, + })); + + expect(mockCaptureRunFailure).toHaveBeenCalledWith(expect.objectContaining({ + errorMessage: "Adapter failed", + errorCode: "adapter_failed", + ...processExit, + })); + const captured = mockCaptureRunFailure.mock.calls[0][0]; + expect(captured).not.toHaveProperty("stdoutExcerpt"); + expect(captured).not.toHaveProperty("stderrExcerpt"); + expect(captured).not.toHaveProperty("resultJson"); + } + }); + it("captures nothing for succeeded, cancelled, and interrupted", async () => { await seedCompanyAndAgent(); for (const status of ["succeeded", "cancelled", "interrupted"] as const) { diff --git a/server/src/services/run-failure-report.ts b/server/src/services/run-failure-report.ts index 98aa9193111..7e403455138 100644 --- a/server/src/services/run-failure-report.ts +++ b/server/src/services/run-failure-report.ts @@ -90,6 +90,8 @@ async function captureTerminalRunFailure( : sanitizeAdapterText(run.errorCode, MAX_ERROR_CODE_LENGTH), agentAdapter: agent?.adapterType ?? UNKNOWN_ADAPTER, runStatus, + exitCode: run.exitCode, + signal: run.signal, }); } catch (err) { logger.warn({ err, runId: run.id }, "failed to report run failure to Sentry");