mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-15 18:56:05 +01:00
## 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 <noreply@paperclip.ing>
235 lines
8.6 KiB
TypeScript
235 lines
8.6 KiB
TypeScript
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
|
import type { RunFailureEvent } from "../sentry.js";
|
|
|
|
/**
|
|
* Tests for `captureRunFailure`, the Sentry report for a terminal run
|
|
* failure. `@sentry/node` is an optional runtime dependency and is not
|
|
* 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 `captureException` spy records the real call
|
|
* shape.
|
|
*/
|
|
|
|
const DSN_ENV = "SENTRY_DSN";
|
|
const FRONTEND_DSN_ENV = "SENTRY_DSN_FRONTEND";
|
|
const BACKEND_DSN_ENV = "SENTRY_DSN_BACKEND";
|
|
const originalDsn = process.env[DSN_ENV];
|
|
const originalFrontendDsn = process.env[FRONTEND_DSN_ENV];
|
|
const originalBackendDsn = process.env[BACKEND_DSN_ENV];
|
|
|
|
function baseEvent(overrides: Partial<RunFailureEvent> = {}): RunFailureEvent {
|
|
return {
|
|
taskId: "11111111-1111-1111-1111-111111111111",
|
|
runId: "22222222-2222-2222-2222-222222222222",
|
|
errorMessage: "the provider process exited with code 1",
|
|
errorCode: "process_lost",
|
|
agentAdapter: "claude-code",
|
|
runStatus: "failed",
|
|
...overrides,
|
|
};
|
|
}
|
|
|
|
interface CaptureContext {
|
|
tags: Record<string, string>;
|
|
contexts: Record<string, Record<string, unknown>>;
|
|
fingerprint: string[];
|
|
}
|
|
|
|
/** Mirrors `sentry.test.ts`'s `mockSentryPackage`. */
|
|
function mockSentryPackage() {
|
|
const init = vi.fn();
|
|
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) => ({
|
|
name: "OnUnhandledRejection",
|
|
...(options as object),
|
|
}));
|
|
|
|
vi.doMock("@sentry/node", () => ({
|
|
init,
|
|
captureException,
|
|
close,
|
|
httpIntegration,
|
|
onUnhandledRejectionIntegration,
|
|
}));
|
|
vi.doMock("../peer-version-check.js", () => ({
|
|
checkExactPeerVersions: () => ({ ok: true }),
|
|
}));
|
|
|
|
return { init, captureException, close };
|
|
}
|
|
|
|
async function importFreshSentryWithGateOpen() {
|
|
process.env[BACKEND_DSN_ENV] = "https://public@sentry.example.com/1";
|
|
const mocks = mockSentryPackage();
|
|
vi.resetModules();
|
|
const sentryModule = await import("../sentry.js");
|
|
await sentryModule.sentryReady;
|
|
return { sentryModule, ...mocks };
|
|
}
|
|
|
|
beforeEach(() => {
|
|
delete process.env[DSN_ENV];
|
|
delete process.env[FRONTEND_DSN_ENV];
|
|
delete process.env[BACKEND_DSN_ENV];
|
|
});
|
|
|
|
afterEach(() => {
|
|
if (originalDsn === undefined) delete process.env[DSN_ENV];
|
|
else process.env[DSN_ENV] = originalDsn;
|
|
if (originalFrontendDsn === undefined) delete process.env[FRONTEND_DSN_ENV];
|
|
else process.env[FRONTEND_DSN_ENV] = originalFrontendDsn;
|
|
if (originalBackendDsn === undefined) delete process.env[BACKEND_DSN_ENV];
|
|
else process.env[BACKEND_DSN_ENV] = originalBackendDsn;
|
|
vi.restoreAllMocks();
|
|
vi.doUnmock("@sentry/node");
|
|
vi.doUnmock("../peer-version-check.js");
|
|
});
|
|
|
|
describe("captureRunFailure", () => {
|
|
it("sets the fingerprint [errorCode, agentAdapter] in that order", async () => {
|
|
const { sentryModule, captureException } = await importFreshSentryWithGateOpen();
|
|
|
|
sentryModule.captureRunFailure(baseEvent({ errorCode: "timeout", agentAdapter: "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, captureException } = await importFreshSentryWithGateOpen();
|
|
|
|
sentryModule.captureRunFailure(
|
|
baseEvent({ errorMessage: "message one", errorCode: "timeout", agentAdapter: "codex" }),
|
|
);
|
|
sentryModule.captureRunFailure(
|
|
baseEvent({ errorMessage: "message two", errorCode: "timeout", agentAdapter: "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, captureException } = await importFreshSentryWithGateOpen();
|
|
|
|
sentryModule.captureRunFailure(baseEvent({ errorCode: null }));
|
|
|
|
expect(captureException.mock.calls[0]![1]?.fingerprint).toEqual(["unknown", "claude-code"]);
|
|
});
|
|
|
|
it("keeps missing process exit evidence null on the run_failure context", async () => {
|
|
const { sentryModule, captureException } = await importFreshSentryWithGateOpen();
|
|
const event = baseEvent();
|
|
|
|
sentryModule.captureRunFailure(event);
|
|
|
|
expect(captureException.mock.calls[0]![1]?.contexts.run_failure).toEqual({
|
|
taskId: event.taskId,
|
|
runId: event.runId,
|
|
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();
|
|
|
|
sentryModule.captureRunFailure(baseEvent());
|
|
|
|
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, captureException } = await importFreshSentryWithGateOpen();
|
|
const event = baseEvent();
|
|
|
|
sentryModule.captureRunFailure(event);
|
|
|
|
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 () => {
|
|
const { sentryModule, captureException } = await importFreshSentryWithGateOpen();
|
|
const event = baseEvent({ errorMessage: "boom" });
|
|
|
|
sentryModule.captureRunFailure(event);
|
|
|
|
expect(captureException).toHaveBeenCalledTimes(1);
|
|
const [received] = captureException.mock.calls[0]!;
|
|
expect(received).toBeInstanceOf(Error);
|
|
expect((received as Error).message).toBe("boom");
|
|
});
|
|
|
|
it("does not throw and captures nothing when the gate is closed", async () => {
|
|
vi.resetModules();
|
|
const sentryModule = await import("../sentry.js");
|
|
await sentryModule.sentryReady;
|
|
|
|
expect(() => sentryModule.captureRunFailure(baseEvent())).not.toThrow();
|
|
});
|
|
|
|
it("does not throw when the client throws", async () => {
|
|
const { sentryModule, captureException } = await importFreshSentryWithGateOpen();
|
|
captureException.mockImplementation(() => {
|
|
throw new Error("sentry client is down");
|
|
});
|
|
|
|
expect(() => sentryModule.captureRunFailure(baseEvent())).not.toThrow();
|
|
});
|
|
});
|