mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-30 21:49:18 +01:00
fix(sentry): preserve run failure stacks and diagnostic context (#14585)
Builds on merged #14575 and targets `master`. ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Operators use optional Sentry reports to diagnose failed agent runs. > - The shared reporter currently converts each saved error message into a new exception. > - This loses the original stack and cause chain, and omits available adapter and provider diagnostics. > - This pull request selects, redacts, and bounds useful diagnostics before it sends them to Sentry. > - Operators can locate the failing operation and correlate upstream failures without copying arbitrary run data. ## Linked Issues or Issue Description Refs #14573. That change preserves ACP provider failure details in the run record. Refs #14575. That merged PR adds recorded exit codes and signals. This PR covers stacks, causes, and structured diagnostics without duplicating those fields. A thrown setup or adapter error currently appears in Sentry with the reporter's stack. A saved provider failure can contain useful details that never reach the Sentry event. Both cases use the same shared reporting path. ## What Changed - Pass caught setup and execution exceptions, and structured adapter error metadata, through successful terminal status transitions. - Snapshot a fixed set of execution, adapter, provider, and exception fields. Preserve up to four exceptions in the cause chain. - Remove registered secret values, declared runtime environment credentials, unknown inherited environment values and encoded credential forms, and credential patterns before truncation. Mark truncated fields and bound provider details and stacks. - Rebuild sanitized Sentry exceptions with the original stacks and causes. Use an adapter stack preview when available. Omit a fabricated reporting stack when the source has no stack. - Keep contexts local to each event. Preserve the optional DSN gate and existing error-code/adapter fingerprint. - Document the fields, limits, and omitted data. - Settle leftover chat fixture outbox rows only after assertions and worker shutdown, so subsequent tests cannot claim earlier cases’ pending actions or provider I/O. This fixes the CI shard contamination exposed during verification; production chat behavior and test timeouts are unchanged. ## Verification - `pnpm -r typecheck` passed. - `pnpm build` passed. - 86 focused diagnostic, Sentry, and startup tests passed after the rebase, including the real `@sentry/node@10.71.0` SDK with an in-memory transport. - Tests cover cause chains, HTTP status and request IDs, long provider details, secret redaction, failed secret resolution, cyclic causes, size limits, and context isolation. - Five targeted heartbeat integration cases passed, including thrown and returned errors containing an opaque environment-bound credential. The earlier full heartbeat integration run also passed its integration cases. - The focused suite also passed with an unknown inherited environment value set to `1`; reporting tests use a controlled environment and separately verify short-secret redaction. - Before the fixture cleanup, the full chat shard reproduced the CI Telegram timeout at `pending recovery before restart` (354 passed, 1 failed). After the cleanup, the same shard passed all 355 tests. No timeout or production behavior changed. - Latest-head CI (`0ae9e70df321d66dda025c3a7ba4787e169e0a4f`) passed typecheck, build, all 12 server shards, all 3 chat shards, workspace and serialized suites, browser shards, Runner checks, canary validation, the real Sentry SDK contract, and security checks. Required `ci / verify` and `ci / e2e` passed. - The branch has been rebased onto `master` after #14575 merged. All 55 applicable checks passed on this head (2 unrelated Storybook checks skipped). Greptile re-reviewed the current 11-file diff at 5/5 with no findings or unresolved review threads. - Local monolithic full-suite attempts were interrupted to apply fixes; full-suite success is not claimed from those runs. ## Risks - Error messages and stacks can contain credentials. The reporter uses existing redactors and the run's encrypted secret registry, reads only known fields, and skips capture if registered-secret resolution fails. - Unknown environment values remain private by default. Unrecognized short values can mask benign matches; known public settings are explicitly allowed. - Diagnostic text is bounded and can be truncated. Truncation is explicit. Data discarded upstream cannot be recovered. - These additional fields go to the operator's configured Sentry endpoint. Arbitrary request/response objects, headers, configuration, prompts, and stdout/stderr are not copied. ## Model Used OpenAI GPT-6 through Codex, with tool use and code execution. The exact serving model ID, context window, and configured reasoning level are 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 `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
2de43fc909
commit
b3eb03fcba
11 files changed
+667
-36
No files matched your search
@@ -1048,7 +1048,19 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => {
|
||||
try {
|
||||
await Promise.all([...fixtureServices].map((service) => service.shutdown()));
|
||||
} finally {
|
||||
await retireFixtureState([...fixtureCompanies]);
|
||||
const companyIds = [...fixtureCompanies];
|
||||
await retireFixtureState(companyIds);
|
||||
if (companyIds.length > 0) {
|
||||
// Pausing an endpoint does not remove its rows from global recovery
|
||||
// selectors. After every assertion and worker shutdown, settle leftover
|
||||
// fixture work so later cases cannot claim its leases or retry its I/O.
|
||||
await db.update(chatActions).set({ status: "cancelled" })
|
||||
.where(and(inArray(chatActions.companyId, companyIds), notInArray(chatActions.status, ["processed", "cancelled"])));
|
||||
await db.update(chatDeliveries).set({ state: "failed", nextAttemptAt: null })
|
||||
.where(and(inArray(chatDeliveries.companyId, companyIds), inArray(chatDeliveries.state, ["received", "processing", "retry"])));
|
||||
await db.update(chatPublications).set({ state: "cancelled", nextAttemptAt: null })
|
||||
.where(and(inArray(chatPublications.companyId, companyIds), inArray(chatPublications.state, ["pending", "awaiting_consent", "streaming", "retry"])));
|
||||
}
|
||||
fixtureServices.clear();
|
||||
fixtureCompanies.clear();
|
||||
}
|
||||
|
||||
@@ -1565,8 +1565,9 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => {
|
||||
"continuation_task_ownership_changed",
|
||||
])("retains untyped continuation setup failures: %s", async (message) => {
|
||||
const { runId, wakeupRequestId } = await seedQueuedIssueRunFixture();
|
||||
const error = new Error(message, { cause: Object.assign(new Error("upstream setup failed"), { code: "ECONNRESET" }) });
|
||||
const build = vi.spyOn(executionContinuation, "buildExecutionContinuation")
|
||||
.mockRejectedValueOnce(new Error(message));
|
||||
.mockRejectedValueOnce(error);
|
||||
try {
|
||||
const heartbeat = heartbeatService(db);
|
||||
await heartbeat.resumeQueuedRuns();
|
||||
@@ -1576,6 +1577,11 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => {
|
||||
expect(await heartbeat.getRun(runId)).toMatchObject({
|
||||
status: "failed", errorCode: "setup_failed", error: message,
|
||||
});
|
||||
const report = await waitForValue(async () => mockCaptureRunFailure.mock.calls.find(([event]) => event.runId === runId)?.[0]);
|
||||
expect(report?.diagnostics).toMatchObject({
|
||||
execution: { failurePhase: "setup" },
|
||||
exceptions: [{ message, stack: expect.stringContaining("heartbeat-process-recovery.test.ts") }, { code: "ECONNRESET" }],
|
||||
});
|
||||
const [wakeup] = await db.select().from(agentWakeupRequests)
|
||||
.where(eq(agentWakeupRequests.id, wakeupRequestId));
|
||||
expect(wakeup.status).toBe("failed");
|
||||
@@ -4801,6 +4807,11 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => {
|
||||
expect(failedRun?.error).toContain(
|
||||
"is not installed or its plugin worker is not running",
|
||||
);
|
||||
const report = await waitForValue(async () => mockCaptureRunFailure.mock.calls.find(([event]) => event.runId === runId)?.[0]);
|
||||
expect(report?.diagnostics).toMatchObject({
|
||||
execution: { failurePhase: "execute" },
|
||||
exceptions: [{ stack: expect.stringContaining("heartbeat-process-recovery.test.ts") }],
|
||||
});
|
||||
|
||||
const interaction = await waitForValue(async () => {
|
||||
const row = await db
|
||||
@@ -5282,6 +5293,38 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => {
|
||||
expect(validationComment).toBeTruthy();
|
||||
});
|
||||
|
||||
it.each(["throw", "result"])("redacts opaque environment-bound credentials from Sentry diagnostics: %s", async (mode) => {
|
||||
const { companyId, agentId, runId } = await seedQueuedIssueRunFixture();
|
||||
const svc = secretService(db);
|
||||
const value = "opaque-runtime-credential-fixture";
|
||||
const secret = await svc.create(companyId, {
|
||||
name: `sentry-redaction-${randomUUID()}`, provider: "local_encrypted", value,
|
||||
});
|
||||
const env = { CUSTOM_BINDING: { type: "secret_ref", secretId: secret.id, version: "latest" } };
|
||||
await svc.syncEnvBindingsForTarget(companyId, { targetType: "agent", targetId: agentId }, env);
|
||||
await db.update(agents).set({ adapterConfig: { env } }).where(eq(agents.id, agentId));
|
||||
mockAdapterExecute.mockImplementationOnce(async (input) => {
|
||||
expect(input.config.env.CUSTOM_BINDING).toBe(value);
|
||||
const message = `upstream rejected ${value}`;
|
||||
if (mode === "throw") throw new Error(message, { cause: new Error(message) });
|
||||
return {
|
||||
exitCode: 1, signal: null, timedOut: false, errorMessage: message, errorCode: "adapter_failed",
|
||||
errorMeta: { causeMessage: message },
|
||||
resultJson: { terminalSessionFailure: { category: "service", details: message } },
|
||||
};
|
||||
});
|
||||
const heartbeat = heartbeatService(db);
|
||||
await heartbeat.resumeQueuedRuns();
|
||||
await waitForRunToSettle(heartbeat, runId, 5_000);
|
||||
await heartbeat.waitForRunExecutionDrain(runId);
|
||||
const report = await waitForValue(async () => mockCaptureRunFailure.mock.calls.find(([event]) => event.runId === runId)?.[0]);
|
||||
expect(report).toBeDefined();
|
||||
expect(report.errorMessage).toContain("upstream rejected");
|
||||
expect(JSON.stringify(report)).not.toContain(value);
|
||||
if (mode === "throw") expect(report.diagnostics.exceptions).toHaveLength(2);
|
||||
else expect(report.diagnostics.provider.category).toBe("service");
|
||||
});
|
||||
|
||||
it("blocks before dispatch when a declared secret ref has no binding instead of emitting an opaque setup failure", async () => {
|
||||
const { companyId, agentId, runId, issueId } =
|
||||
await seedQueuedIssueRunFixture();
|
||||
|
||||
@@ -1,5 +1,7 @@
|
||||
import { createRequire } from "node:module";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { collectRunFailureDiagnostics, sanitizeRunFailureDiagnostics } from "../services/run-failure-diagnostics.js";
|
||||
import type { heartbeatRuns } from "@paperclipai/db";
|
||||
|
||||
// Sentry is an optional peer. When installed, exercise the real SDK with an
|
||||
// in-memory transport, including its context behavior without an OTel manager.
|
||||
@@ -76,7 +78,17 @@ describe.skipIf(!sentryPackage)("run failure context with the real Sentry SDK",
|
||||
exitCode: null,
|
||||
signal: "SIGKILL",
|
||||
};
|
||||
captureRunFailure(first);
|
||||
const original = Object.assign(new Error(first.errorMessage, { cause: Object.assign(new Error("upstream connection reset"), {
|
||||
code: "ECONNRESET", requestId: "request-123", status: 503,
|
||||
stack: "Error: upstream connection reset\n at socketRead (/app/provider.js:19:7)",
|
||||
response: { body: "private-response" },
|
||||
}) }), { stack: "Error: first run failed\n at originalAdapter (/app/adapter.js:42:7)", request: { headers: "private-headers" } });
|
||||
const diagnostics = sanitizeRunFailureDiagnostics(collectRunFailureDiagnostics({
|
||||
runtimeMode: "legacy", resultJson: { terminalSessionFailure: {
|
||||
category: "service", title: "Provider failed", details: "d".repeat(9000),
|
||||
} },
|
||||
} as unknown as typeof heartbeatRuns.$inferSelect, { error: original, phase: "execute" }));
|
||||
captureRunFailure({ ...first, diagnostics });
|
||||
await Promise.resolve();
|
||||
captureException(new Error("unrelated database error"));
|
||||
captureRunFailure(second);
|
||||
@@ -91,7 +103,7 @@ describe.skipIf(!sentryPackage)("run failure context with the real Sentry SDK",
|
||||
|
||||
expect(events).toHaveLength(5);
|
||||
const captured = (message: string) => events.find((event) =>
|
||||
(event.exception as { values: Array<{ value: string }> }).values[0]?.value === message,
|
||||
(event.exception as { values: Array<{ value: string }> }).values.some((value) => value.value === message),
|
||||
);
|
||||
for (const run of [first, second]) {
|
||||
expect(captured(run.errorMessage)).toMatchObject({
|
||||
@@ -108,10 +120,25 @@ describe.skipIf(!sentryPackage)("run failure context with the real Sentry SDK",
|
||||
fingerprint: [first.errorCode, first.agentAdapter],
|
||||
});
|
||||
expect(JSON.stringify(events)).not.toContain("private-signal-payload");
|
||||
const firstCapture = captured(first.errorMessage)!;
|
||||
const exceptions = (firstCapture.exception as { values: Array<Record<string, unknown>> }).values;
|
||||
expect(exceptions).toHaveLength(2);
|
||||
expect(JSON.stringify(exceptions)).toContain("originalAdapter");
|
||||
expect(JSON.stringify(exceptions)).toContain("socketRead");
|
||||
expect(JSON.stringify(exceptions)).not.toContain("captureRunFailure");
|
||||
expect(JSON.stringify(captured(second.errorMessage)?.exception)).not.toContain("captureRunFailure");
|
||||
expect(firstCapture).toMatchObject({ contexts: {
|
||||
provider_failure: { category: "service", details: "d".repeat(9000) },
|
||||
run_exception_1: { code: "ECONNRESET", requestId: "request-123", status: 503 },
|
||||
} });
|
||||
expect(JSON.stringify(events)).not.toContain("private-");
|
||||
for (const message of ["unrelated database error", "unrelated filesystem error"]) {
|
||||
const event = captured(message);
|
||||
expect(event).toBeDefined();
|
||||
expect(event).not.toHaveProperty("contexts.run_failure");
|
||||
for (const context of ["run_execution", "adapter_failure", "provider_failure", "run_exception_0", "run_exception_1"]) {
|
||||
expect(event).not.toHaveProperty(`contexts.${context}`);
|
||||
}
|
||||
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}`);
|
||||
|
||||
@@ -215,6 +215,31 @@ describe("captureRunFailure", () => {
|
||||
expect((received as Error).message).toBe("boom");
|
||||
});
|
||||
|
||||
it("uses sanitized original stacks and causes instead of the reporting stack", async () => {
|
||||
const { sentryModule, captureException } = await importFreshSentryWithGateOpen();
|
||||
sentryModule.captureRunFailure(baseEvent({ diagnostics: {
|
||||
execution: { runtimeMode: "native", failurePhase: "setup" },
|
||||
adapter: {}, provider: {}, truncatedFields: [],
|
||||
exceptions: [
|
||||
{ name: "TypeError", message: "setup failed", stack: "TypeError: setup failed\n at originalSetup (/app/setup.js:42:7)" },
|
||||
{ name: "Error", message: "connection reset", stack: "Error: connection reset\n at socketRead (/app/network.js:9:4)", code: "ECONNRESET", requestId: "req-123" },
|
||||
],
|
||||
} }));
|
||||
const [error, context] = captureException.mock.calls[0]!;
|
||||
expect(error).toBeInstanceOf(Error);
|
||||
expect(error).toMatchObject({ name: "TypeError", message: "setup failed", cause: { message: "connection reset" } });
|
||||
expect((error as Error).stack).toContain("originalSetup");
|
||||
expect((error as Error).stack).not.toContain("captureRunFailure");
|
||||
expect(context?.contexts.run_exception_1).toMatchObject({ code: "ECONNRESET", requestId: "req-123" });
|
||||
expect(context?.contexts.run_execution).toMatchObject({ failurePhase: "setup" });
|
||||
});
|
||||
|
||||
it("does not invent a reporter stack for a saved result with no original exception", async () => {
|
||||
const { sentryModule, captureException } = await importFreshSentryWithGateOpen();
|
||||
sentryModule.captureRunFailure(baseEvent());
|
||||
expect((captureException.mock.calls[0]![0] as Error).stack).toBeUndefined();
|
||||
});
|
||||
|
||||
it("does not throw and captures nothing when the gate is closed", async () => {
|
||||
vi.resetModules();
|
||||
const sentryModule = await import("../sentry.js");
|
||||
|
||||
Reference in new issue
Block a user