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

Author: outof-place

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

@@ -1,35 +1,31 @@
 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 real redactor reads process.env to find the secrets it blanks, which
+// makes its output depend on the machine running the suite. Redaction itself
+// is pinned below against an injected redactor.
+vi.mock("../sandbox/agents/protocol.js", () => ({
+  redactDiagnosticText: (value: string) => value,
+}));
 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,
+  describePrePrChecksFailureStep,
+  prePrChecksFailureInput,
   prePrChecksFailureMustPropagate,
   prePrChecksFailureReport,
-  runPrePrChecksStep,
 } from "./agent.js";
 import { isDurationAbortError } from "./run-budget.js";
 import { isRunControlError } from "./run-control-error.js";
@@ -43,51 +39,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);
@@ -109,65 +81,46 @@ 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",
-      }),
-    );
+    const error = Object.assign(new Error("connect ECONNREFUSED 10.0.0.1:443"), {
+      code: "ECONNREFUSED",
+    });
 
-    await expect(runStep()).rejects.toThrow(
+    await expect(describe_(error)).resolves.toBe(
       `${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,
-    );
+    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 +137,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,7 +157,10 @@ 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");