fix(worker): run pre-PR checks detached so they survive the invocation ceiling (#318)

* fix(worker): run pre-PR checks detached so they survive the invocation ceiling

* fix(worker): keep the check reporting honest under truncation and failure

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015kfeohXE66xx7RJPxZ2pvH

* fix(worker): keep a check failure's reason out of its truncated payload

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015kfeohXE66xx7RJPxZ2pvH

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

Author: outof-place

apps/worker/src/pre-pr-checks/runner.test.ts

@@ -11,12 +11,16 @@ import type {
   RunnableSandbox,
   SerializableAgentCliSpec,
 } from "./types.js";
+import { redactDiagnosticText } from "./redact.js";
 import { AgentRuntimeError } from "./runtime-error.js";
 
 export {
   AgentRuntimeError,
   isAgentRuntimeError,
 } from "./runtime-error.js";
+// Re-exported so every existing caller keeps its import. It lives in a module
+// of its own because workflow scope needs it and cannot have node:crypto.
+export { redactDiagnosticText } from "./redact.js";
 
 const DIAGNOSTIC_TAIL_BYTES = 2 * 1024;
 const MAX_SCHEMA_ISSUES = 20;
@@ -394,20 +398,6 @@ export function eventMetadata(value: unknown): AgentProtocolDiagnostic["event"]
   return Object.keys(metadata).length > 0 ? metadata : undefined;
 }
 
-export function redactDiagnosticText(value: string): string {
-  let redacted = value;
-  const sensitiveValues = Object.entries(process.env)
-    .filter(([key, secret]) =>
-      secret && secret.length >= 8 && /(KEY|TOKEN|SECRET|PASSWORD|CREDENTIAL)/i.test(key),
-    )
-    .map(([, secret]) => secret as string);
-  for (const secret of sensitiveValues) redacted = redacted.split(secret).join("[REDACTED]");
-  return redacted
-    .replace(/\b(?:sk-ant-[A-Za-z0-9_-]+|sk-[A-Za-z0-9_-]{16,}|gh[pousr]_[A-Za-z0-9]+|github_pat_[A-Za-z0-9_]+|glpat-[A-Za-z0-9_-]+)\b/g, "[REDACTED]")
-    .replace(/\b(Bearer\s+)[^\s,;]+/gi, "$1[REDACTED]")
-    .replace(/\b(api[_-]?key|token|secret|password)\s*[:=]\s*[^\s,;]+/gi, "$1=[REDACTED]");
-}
-
 export function hashText(value: string): string {
   return createHash("sha256").update(value).digest("hex");
 }

apps/worker/src/pre-pr-checks/runner.ts

@@ -0,0 +1,26 @@
+/**
+ * Diagnostic redaction, deliberately alone in a module of its own.
+ *
+ * It lives here rather than in protocol.ts so it can be imported from workflow
+ * scope: protocol.ts imports `node:crypto` for its schema hashing, and a Node
+ * builtin anywhere in the workflow module graph fails the Vercel build alone,
+ * never vitest or a local build, which is what makes it worth stating. This
+ * module imports nothing at all.
+ *
+ * Redacting before a value crosses into a step is not cosmetic. Workflow
+ * journals step arguments durably, so an unredacted secret handed to a step is
+ * written to the run's event log whatever the step then does with it.
+ */
+export function redactDiagnosticText(value: string): string {
+  let redacted = value;
+  const sensitiveValues = Object.entries(process.env)
+    .filter(([key, secret]) =>
+      secret && secret.length >= 8 && /(KEY|TOKEN|SECRET|PASSWORD|CREDENTIAL)/i.test(key),
+    )
+    .map(([, secret]) => secret as string);
+  for (const secret of sensitiveValues) redacted = redacted.split(secret).join("[REDACTED]");
+  return redacted
+    .replace(/\b(?:sk-ant-[A-Za-z0-9_-]+|sk-[A-Za-z0-9_-]{16,}|gh[pousr]_[A-Za-z0-9]+|github_pat_[A-Za-z0-9_]+|glpat-[A-Za-z0-9_-]+)\b/g, "[REDACTED]")
+    .replace(/\b(Bearer\s+)[^\s,;]+/gi, "$1[REDACTED]")
+    .replace(/\b(api[_-]?key|token|secret|password)\s*[:=]\s*[^\s,;]+/gi, "$1=[REDACTED]");
+}

apps/worker/src/sandbox/agents/protocol.ts

@@ -1,35 +1,29 @@
 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 },
 }));
+// The regex half of the real redactor is what these assert against; its
+// process.env half would make the output depend on the machine running the
+// suite, and there is no secret in these fixtures for it to find.
 vi.mock("../../env.js", () => ({
   env: { DASHBOARD_ORIGIN: "https://dashboard.example.com" },
 }));
 
 import {
   PRE_PR_CHECKS_FAILURE_CAUSE_MAX_LENGTH,
+  prePrChecksFailureMessage,
   PRE_PR_CHECKS_FAILURE_STACK_TAIL_MAX_LENGTH,
+  describePrePrChecksFailureStep,
+  prePrChecksFailureInput,
   prePrChecksFailureMustPropagate,
   prePrChecksFailureReport,
-  runPrePrChecksStep,
 } from "./agent.js";
 import { isDurationAbortError } from "./run-budget.js";
 import { isRunControlError } from "./run-control-error.js";
@@ -43,51 +37,27 @@ function namedError(name: string, message: string): Error {
   return error;
 }
 
-function runStep() {
-  return runPrePrChecksStep("sbx-test-123", "codex", "gpt-5");
+/**
+ * What the call site does with a throw it may not propagate: flatten it in
+ * workflow scope, then compose and log the sentence inside the step. The
+ * checks stopped being a step of their own (they are launched detached and
+ * polled), so this pair is the seam that carries what #316 landed.
+ */
+function describe_(error: unknown, version: number | null = 7) {
+  return describePrePrChecksFailureStep(prePrChecksFailureInput(error), version);
 }
 
 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(
+    await expect(describe_(new Error("sandbox connection reset"))).resolves.toBe(
       `${MESSAGE_LEAD}sandbox connection reset`,
     );
     expect(mocks.error).toHaveBeenCalledTimes(1);
@@ -108,66 +78,57 @@ describe("pre-PR checks step failure cause", () => {
     );
   });
 
-  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",
-      }),
-    );
+  it("labels the cause with the class name, the only thing left of the error", async () => {
+    // Everything caught here was thrown inside a step, and Workflow reduces a
+    // thrown error to name, message and stack at the VM boundary and revives it
+    // as a plain Error. A system error code would name the cause far better
+    // than `Error` does, but `.code` cannot reach this side, so nothing may be
+    // built on it: a label that can never fire reads as coverage that does not
+    // exist. Recovering it would mean parsing the message.
+    const error = Object.assign(namedError("SandboxError", "connect ECONNREFUSED 10.0.0.1:443"), {
+      code: "ECONNREFUSED",
+    });
 
-    await expect(runStep()).rejects.toThrow(
-      `${MESSAGE_LEAD}ECONNREFUSED: connect ECONNREFUSED 10.0.0.1:443`,
+    await expect(describe_(error)).resolves.toBe(
+      `${MESSAGE_LEAD}SandboxError: connect ECONNREFUSED 10.0.0.1:443`,
     );
+    // A plain Error adds nothing worth prefixing, and a non-Error throw's
+    // `typeof` is noise on top of its own text.
+    await expect(describe_(new Error("plain"))).resolves.toBe(`${MESSAGE_LEAD}plain`);
+    await expect(describe_("just a string")).resolves.toBe(`${MESSAGE_LEAD}just a string`);
   });
 
   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,
-    );
+    const message = await describe_(new Error(thrownMessage));
 
-    expect(thrown?.message).toContain(MESSAGE_LEAD);
-    expect(thrown?.message).not.toContain(thrownMessage);
+    expect(message).toContain(MESSAGE_LEAD);
+    expect(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(
+    expect(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();
+    "keeps %s out of the wrap so the call site still recognizes it",
+    (_label, error) => {
+      expect(prePrChecksFailureMustPropagate(error)).toBe(true);
+      expect(isRunControlError(error)).toBe(true);
     },
   );
 
   it.each([["AbortError"], ["TimeoutError"]])(
-    "rethrows a %s untouched so the duration budget stop survives",
-    async (name) => {
+    "keeps a %s out of the wrap so the duration budget stop survives",
+    (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();
+
+      expect(prePrChecksFailureMustPropagate(error)).toBe(true);
+      expect(isDurationAbortError(error)).toBe(true);
     },
   );
 
@@ -184,18 +145,18 @@ describe("pre-PR checks step failure cause", () => {
     expect(prePrChecksFailureMustPropagate(new Error("sandbox died"))).toBe(false);
 
     const rewrappedBudget = new Error(
-      prePrChecksFailureReport(budgetStop, identity).message,
+      prePrChecksFailureReport(prePrChecksFailureInput(budgetStop), identity).message,
     );
     expect(isRunControlError(rewrappedBudget)).toBe(false);
     const rewrappedAbort = new Error(
-      prePrChecksFailureReport(abort, identity).message,
+      prePrChecksFailureReport(prePrChecksFailureInput(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"),
+      prePrChecksFailureInput(new Error("auth failed for token=abcd1234")),
       (value) => value.replace("abcd1234", "[REDACTED]"),
     );
 
@@ -204,10 +165,78 @@ describe("pre-PR checks step failure cause", () => {
   });
 
   it("names a non-Error throw rather than dropping it", () => {
-    const report = prePrChecksFailureReport("sandbox vanished", (v) => v);
+    const report = prePrChecksFailureReport(
+      prePrChecksFailureInput("sandbox vanished"),
+      (v) => v,
+    );
 
     expect(report.message).toBe(`${MESSAGE_LEAD}sandbox vanished`);
     expect(report.name).toBe("string");
     expect(report.stackTail).toBe("");
   });
+
+  it("redacts and bounds before the value can be journaled", async () => {
+    // Workflow journals step arguments durably, so whatever the flattener
+    // returns is written into the run's event log verbatim. Redacting inside
+    // the step would protect only the sentence an operator reads.
+    const secret = "glpat-AAAAAAAAAAAAAAAAAAAA";
+    const error = new Error(
+      `clone failed for https://oauth2:${secret}@gitlab.com/acme/api.git ${"pad ".repeat(200)}`,
+    );
+
+    const input = prePrChecksFailureInput(error);
+
+    expect(input.message).not.toContain(secret);
+    expect(input.message).toContain("[REDACTED]");
+    expect(input.message.length).toBe(PRE_PR_CHECKS_FAILURE_CAUSE_MAX_LENGTH);
+    expect(input.stack.length).toBeLessThanOrEqual(
+      PRE_PR_CHECKS_FAILURE_STACK_TAIL_MAX_LENGTH,
+    );
+  });
+
+  it("survives a throw with no properties at all", async () => {
+    // `throw null` and a bare Promise.reject() both reach here. A TypeError
+    // raised inside the reporting path would lose the cause it exists to carry
+    // and hand the operator an unrelated error.
+    for (const thrown of [null, undefined]) {
+      expect(() => prePrChecksFailureInput(thrown)).not.toThrow();
+      await expect(describe_(thrown)).resolves.toContain(MESSAGE_LEAD);
+    }
+  });
+
+  it("degrades to a fixed sentence when the reporting step itself fails", async () => {
+    // The step is the only place the cause is logged and its maxRetries is 0.
+    // A failed dynamic import on a cold start, or an invocation killed
+    // mid-step, must not substitute the reporting path's own error for the
+    // report.
+    mocks.error.mockImplementation(() => {
+      throw new Error("logger transport is gone");
+    });
+
+    const message = await prePrChecksFailureMessage(
+      Object.assign(new Error("sandbox connection reset"), { name: "SandboxError" }),
+      7,
+    );
+
+    expect(message).toBe(
+      "The Pre-PR checks step failed (SandboxError), and the cause could not be recorded.",
+    );
+  });
+
+  it.each(runControlErrorCases())(
+    "rethrows %s from the reporting step instead of degrading",
+    async (_label, controlError) => {
+      // The degraded sentence is for a reporting path that broke. A cancelled
+      // run surfaces at every step, this one included, and swallowing it here
+      // would report a Pre-PR checks failure for a run the operator cancelled,
+      // and let it carry on being cancelled anyway.
+      mocks.error.mockImplementation(() => {
+        throw controlError;
+      });
+
+      await expect(
+        prePrChecksFailureMessage(new Error("sandbox connection reset"), 7),
+      ).rejects.toBe(controlError);
+    },
+  );
 });