fix(worker): carry the real cause when the pre-PR checks step throws (#316)

Author: outof-place

apps/worker/src/workflows/agent-pre-pr-checks-failure.test.ts

@@ -0,0 +1,213 @@
+import { beforeEach, describe, expect, it, vi } from "vitest";
+
+const mocks = vi.hoisted(() => ({
+  getCurrentPrePrCheckConfig: vi.fn(),
+  runPrePrChecksWithFixes: vi.fn(),
+  info: vi.fn(),
+  warn: vi.fn(),
+  error: vi.fn(),
+}));
+
+vi.mock("../db/client.js", () => ({ getDb: () => ({ kind: "db" }) }));
+vi.mock("../pre-pr-checks/store.js", () => ({
+  getCurrentPrePrCheckConfig: (...args: any[]) =>
+    mocks.getCurrentPrePrCheckConfig(...args),
+}));
+vi.mock("../pre-pr-checks/runner.js", () => ({
+  runPrePrChecksWithFixes: (...args: any[]) =>
+    mocks.runPrePrChecksWithFixes(...args),
+}));
+vi.mock("../lib/logger.js", () => ({
+  logger: { info: mocks.info, warn: mocks.warn, error: mocks.error },
+}));
+vi.mock("../../env.js", () => ({
+  env: { DASHBOARD_ORIGIN: "https://dashboard.example.com" },
+}));
+
+import {
+  PRE_PR_CHECKS_FAILURE_CAUSE_MAX_LENGTH,
+  PRE_PR_CHECKS_FAILURE_STACK_TAIL_MAX_LENGTH,
+  prePrChecksFailureMustPropagate,
+  prePrChecksFailureReport,
+  runPrePrChecksStep,
+} from "./agent.js";
+import { isDurationAbortError } from "./run-budget.js";
+import { isRunControlError } from "./run-control-error.js";
+import { runControlErrorCases } from "./blocks/test-support.js";
+
+const MESSAGE_LEAD = "The Pre-PR checks step failed: ";
+
+function namedError(name: string, message: string): Error {
+  const error = new Error(message);
+  error.name = name;
+  return error;
+}
+
+function runStep() {
+  return runPrePrChecksStep("sbx-test-123", "codex", "gpt-5");
+}
+
+describe("pre-PR checks step failure cause", () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    mocks.getCurrentPrePrCheckConfig.mockResolvedValue({
+      version: 7,
+      config: { repositories: [] },
+    });
+  });
+
+  it("returns the checks result and the loaded version when nothing throws", async () => {
+    mocks.runPrePrChecksWithFixes.mockResolvedValue({
+      outcome: "passed",
+      passed: true,
+      fixCycles: 0,
+      fixCycleUsages: [],
+      budgetFailure: null,
+      summary: "All checks passed.",
+    });
+
+    await expect(runStep()).resolves.toEqual({
+      outcome: "passed",
+      passed: true,
+      fixCycles: 0,
+      fixCycleUsages: [],
+      budgetFailure: null,
+      summary: "All checks passed.",
+      configurationVersion: 7,
+    });
+    expect(mocks.error).not.toHaveBeenCalled();
+  });
+
+  it("names the thrown cause instead of leaving Workflow's wrapper to speak alone", async () => {
+    // Production runs wrun_01M0CBQNAX24STRMN5SGCKKGB2 and
+    // wrun_01M0CAZKV3YMNFBCZJA8MT95GW died here reading only "exceeded max
+    // retries", with no sanitized output captured and nothing in the runtime
+    // logs. Whatever the step throws has to reach the operator-facing text.
+    mocks.runPrePrChecksWithFixes.mockRejectedValue(
+      new Error("sandbox connection reset"),
+    );
+
+    await expect(runStep()).rejects.toThrow(
+      `${MESSAGE_LEAD}sandbox connection reset`,
+    );
+    expect(mocks.error).toHaveBeenCalledTimes(1);
+    const [record, msg] = mocks.error.mock.calls[0] as [
+      { version: number | null; name: string; cause: string; stackTail: string },
+      string,
+    ];
+    expect(msg).toBe("pre_pr_checks_step_failed");
+    expect(record.version).toBe(7);
+    expect(record.name).toBe("Error");
+    expect(record.cause).toBe("sandbox connection reset");
+    // The tail, so a very deep stack keeps its innermost-to-outermost end
+    // rather than growing the log record without bound. The message itself is
+    // already in `cause`, so nothing is lost when the head is clipped.
+    expect(record.stackTail).toMatch(/\bat\b/);
+    expect(record.stackTail.length).toBeLessThanOrEqual(
+      PRE_PR_CHECKS_FAILURE_STACK_TAIL_MAX_LENGTH,
+    );
+  });
+
+  it("prefers a system error code over a class name that says nothing", async () => {
+    mocks.runPrePrChecksWithFixes.mockRejectedValue(
+      Object.assign(new Error("connect ECONNREFUSED 10.0.0.1:443"), {
+        code: "ECONNREFUSED",
+      }),
+    );
+
+    await expect(runStep()).rejects.toThrow(
+      `${MESSAGE_LEAD}ECONNREFUSED: connect ECONNREFUSED 10.0.0.1:443`,
+    );
+  });
+
+  it("bounds a runaway cause instead of embedding it whole", async () => {
+    const thrownMessage = "sandbox refused the launch. ".repeat(200);
+    mocks.runPrePrChecksWithFixes.mockRejectedValue(new Error(thrownMessage));
+
+    const thrown = await runStep().then(
+      () => null,
+      (err: unknown) => err as Error,
+    );
+
+    expect(thrown?.message).toContain(MESSAGE_LEAD);
+    expect(thrown?.message).not.toContain(thrownMessage);
+    // Sentence plus the bound the cause is clamped to, and nothing more, so a
+    // runaway error text cannot become the run status.
+    expect(thrown?.message.length).toBeLessThanOrEqual(
+      MESSAGE_LEAD.length + PRE_PR_CHECKS_FAILURE_CAUSE_MAX_LENGTH,
+    );
+    const logged = mocks.error.mock.calls[0]?.[0] as { cause: string };
+    expect(logged.cause.length).toBe(PRE_PR_CHECKS_FAILURE_CAUSE_MAX_LENGTH);
+  });
+
+  it.each(runControlErrorCases())(
+    "rethrows %s untouched so the call site still recognizes it",
+    async (_label, error) => {
+      mocks.runPrePrChecksWithFixes.mockRejectedValue(error);
+
+      await expect(runStep()).rejects.toBe(error);
+      const thrown = await runStep().then(
+        () => null,
+        (err: unknown) => err,
+      );
+      expect(isRunControlError(thrown)).toBe(true);
+      expect(mocks.error).not.toHaveBeenCalled();
+    },
+  );
+
+  it.each([["AbortError"], ["TimeoutError"]])(
+    "rethrows a %s untouched so the duration budget stop survives",
+    async (name) => {
+      const error = namedError(name, "The operation was aborted.");
+      mocks.runPrePrChecksWithFixes.mockRejectedValue(error);
+
+      await expect(runStep()).rejects.toBe(error);
+      const thrown = await runStep().then(
+        () => null,
+        (err: unknown) => err,
+      );
+      expect(isDurationAbortError(thrown)).toBe(true);
+      expect(mocks.error).not.toHaveBeenCalled();
+    },
+  );
+
+  it("wraps only what wrapping cannot break", () => {
+    // Why the two predicates gate the wrap at all: both match structurally on
+    // `name`, so a control error re-thrown as `new Error(message)` stops being
+    // one, and a duration budget stop would report as a generic failure.
+    const budgetStop = namedError("RunBudgetError", "budget exceeded");
+    const abort = namedError("AbortError", "The operation was aborted.");
+    const identity = (value: string) => value;
+
+    expect(prePrChecksFailureMustPropagate(budgetStop)).toBe(true);
+    expect(prePrChecksFailureMustPropagate(abort)).toBe(true);
+    expect(prePrChecksFailureMustPropagate(new Error("sandbox died"))).toBe(false);
+
+    const rewrappedBudget = new Error(
+      prePrChecksFailureReport(budgetStop, identity).message,
+    );
+    expect(isRunControlError(rewrappedBudget)).toBe(false);
+    const rewrappedAbort = new Error(
+      prePrChecksFailureReport(abort, identity).message,
+    );
+    expect(isDurationAbortError(rewrappedAbort)).toBe(false);
+  });
+
+  it("redacts the cause and the stack tail through the caller's redactor", () => {
+    const report = prePrChecksFailureReport(
+      new Error("auth failed for token=abcd1234"),
+      (value) => value.replace("abcd1234", "[REDACTED]"),
+    );
+
+    expect(report.cause).toBe("auth failed for token=[REDACTED]");
+    expect(report.stackTail).not.toContain("abcd1234");
+  });
+
+  it("names a non-Error throw rather than dropping it", () => {
+    const report = prePrChecksFailureReport("sandbox vanished", (v) => v);
+
+    expect(report.message).toBe(`${MESSAGE_LEAD}sandbox vanished`);
+    expect(report.name).toBe("string");
+    expect(report.stackTail).toBe("");
+  });
+});

apps/worker/src/workflows/agent.ts

@@ -2369,7 +2369,64 @@ async function resolveHarnessRuntimesStep(
 }
 resolveHarnessRuntimesStep.maxRetries = 0;
 
-async function runPrePrChecksStep(
+/** Longest failure cause carried into the Pre-PR checks step error message.
+ *  Same bound the Pre-PR repair launch failure puts on its carried cause
+ *  (#309): long enough for a sandbox, kill or stream verdict, short enough that
+ *  the composed block failure stays a detail rather than a payload. */
+export const PRE_PR_CHECKS_FAILURE_CAUSE_MAX_LENGTH = 200;
+
+/** Stack tail kept for the log record only, never for the operator message:
+ *  frames leak internal paths and are what turns a detail into a firehose. */
+export const PRE_PR_CHECKS_FAILURE_STACK_TAIL_MAX_LENGTH = 600;
+
+/**
+ * Errors `runPrePrChecksStep` must rethrow untouched.
+ *
+ * Both predicates identify an error structurally, by `name`
+ * (run-budget.ts:52, run-budget.ts:280) or by a sentinel in its message
+ * (run-control-errors.ts:16), because Workflow serializes step errors across
+ * VMs. Wrapping one in a new Error therefore destroys the identity the call
+ * site depends on, and a budget stop would start reporting as a generic
+ * failure.
+ */
+export function prePrChecksFailureMustPropagate(error: unknown): boolean {
+  return isRunControlError(error) || isDurationAbortError(error);
+}
+
+/**
+ * What a Pre-PR checks step failure is allowed to say, composed in one pure
+ * place so the bound and the wording can be pinned directly.
+ *
+ * `redact` is a parameter rather than an import: the redactor lives in
+ * `sandbox/agents/protocol.js`, which must stay out of the workflow module
+ * scope.
+ */
+export function prePrChecksFailureReport(
+  error: unknown,
+  redact: (value: string) => string,
+): { name: string; cause: string; stackTail: string; message: string } {
+  const name = error instanceof Error ? error.name : typeof error;
+  const message = error instanceof Error ? error.message : String(error);
+  // Prefer a system error code over the class name: `ECONNREFUSED` names the
+  // cause where `Error` names nothing.
+  const code = (error as { code?: unknown }).code;
+  const label =
+    typeof code === "string" && code ? code : error instanceof Error ? name : "";
+  const cause = redact(
+    label && label !== "Error" ? `${label}: ${message}` : message,
+  ).slice(0, PRE_PR_CHECKS_FAILURE_CAUSE_MAX_LENGTH);
+  const stack = error instanceof Error ? error.stack ?? "" : "";
+  return {
+    name,
+    cause,
+    stackTail: stack
+      ? redact(stack).slice(-PRE_PR_CHECKS_FAILURE_STACK_TAIL_MAX_LENGTH)
+      : "",
+    message: `The Pre-PR checks step failed: ${cause}`,
+  };
+}
+
+export async function runPrePrChecksStep(
   sandboxId: string,
   agentKind: AgentKind,
   model: string,
@@ -2403,17 +2460,40 @@ async function runPrePrChecksStep(
     { version: current?.version ?? null },
     "pre_pr_checks_config_version",
   );
-  const result = await runPrePrChecksWithFixes(
-    sandboxId,
-    current?.config ?? emptyPrePrCheckConfig,
-    agentKind,
-    model,
-    maxFixCycles,
-    timeoutMs,
-    budget,
-    runtime,
-    arthurTaskId,
-  );
+  let result: Awaited<ReturnType<typeof runPrePrChecksWithFixes>>;
+  try {
+    result = await runPrePrChecksWithFixes(
+      sandboxId,
+      current?.config ?? emptyPrePrCheckConfig,
+      agentKind,
+      model,
+      maxFixCycles,
+      timeoutMs,
+      budget,
+      runtime,
+      arthurTaskId,
+    );
+  } catch (err) {
+    // Nothing used to be caught here, so a throw reached the operator as
+    // Workflow's own wrapper alone ("exceeded max retries"): no error name, no
+    // message, no captured output, and nothing in the runtime logs either.
+    // Carry the reason, redacted and bounded exactly like a diagnostic tail.
+    // Run-control and duration-abort errors rethrow untouched, because the call
+    // site turns them into a budget stop by matching their identity.
+    if (prePrChecksFailureMustPropagate(err)) throw err;
+    const { redactDiagnosticText } = await import("../sandbox/agents/protocol.js");
+    const report = prePrChecksFailureReport(err, redactDiagnosticText);
+    logger.error(
+      {
+        version: current?.version ?? null,
+        name: report.name,
+        cause: report.cause,
+        stackTail: report.stackTail,
+      },
+      "pre_pr_checks_step_failed",
+    );
+    throw new Error(report.message);
+  }
   return {
     ...result,
     configurationVersion: current?.version ?? null,