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

Author: outof-place

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

@@ -78,14 +78,24 @@ describe("pre-PR checks step failure cause", () => {
     );
   });
 
-  it("prefers a system error code over a class name that says nothing", async () => {
-    const error = Object.assign(new Error("connect ECONNREFUSED 10.0.0.1:443"), {
+  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(describe_(error)).resolves.toBe(
-      `${MESSAGE_LEAD}ECONNREFUSED: connect ECONNREFUSED 10.0.0.1:443`,
+      `${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 () => {
@@ -212,4 +222,21 @@ describe("pre-PR checks step failure cause", () => {
       "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);
+    },
+  );
 });

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

@@ -2414,10 +2414,17 @@ export interface PrePrChecksFailureInput {
   name: string;
   message: string;
   /**
-   * What to prefix the cause with, or empty for no prefix. A system error code
-   * when there is one, because `ECONNREFUSED` names the cause where `Error`
-   * names nothing; the class name otherwise; nothing at all for a non-Error
+   * What to prefix the cause with: the class name, or empty for a non-Error
    * throw, whose `typeof` would only add noise to its own text.
+   *
+   * The class name is all there is to prefix with. This catch sits in workflow
+   * scope and every error it sees was thrown inside a step, and Workflow
+   * reduces a thrown error to its name, message and stack at the VM boundary
+   * and revives it as a plain Error on this side. A system error code
+   * (`ECONNREFUSED`) would name the cause far better than `Error` does, but
+   * `.code` is gone by the time anything here can read it. Recovering it would
+   * mean parsing the message, the way isReplayedRunControlStepError parses for
+   * run-control errors, and no incident has yet asked for it.
    */
   label: string;
   stack: string;
@@ -2434,6 +2441,15 @@ export interface PrePrChecksFailureInput {
  * protecting only the sentence an operator reads. Redaction runs before
  * truncation so a secret is blanked rather than cut in half.
  *
+ * The redactor runs in workflow scope here and again inside the step, and the
+ * two cannot disagree within an invocation: the workflow VM shims `process` as
+ * a frozen spread of `process.env` taken when the context is built, so both
+ * sides read the same snapshot. The narrow consequence is that a secret added
+ * or rotated AFTER the context was built is absent from that snapshot, so its
+ * literal value would cross the boundary into the journal unblanked while the
+ * operator's sentence stays clean. The pattern rules below (`sk-ant-`, `gh?_`,
+ * `glpat-`, `Bearer`) do not depend on the environment and still catch it.
+ *
  * Bounded here for the same reason: the step keeps exactly these bytes anyway,
  * so anything beyond them would be journaled and then dropped.
  *
@@ -2443,10 +2459,6 @@ export interface PrePrChecksFailureInput {
  */
 export function prePrChecksFailureInput(error: unknown): PrePrChecksFailureInput {
   const isError = error instanceof Error;
-  // Optional on purpose. `throw null` and a bare `Promise.reject()` both reach
-  // here, and a TypeError raised inside the reporting path would destroy the
-  // very cause this exists to carry.
-  const code = (error as { code?: unknown } | null | undefined)?.code;
   const name = isError ? error.name : typeof error;
   const stack = isError ? error.stack ?? "" : "";
   return {
@@ -2455,7 +2467,7 @@ export function prePrChecksFailureInput(error: unknown): PrePrChecksFailureInput
       0,
       PRE_PR_CHECKS_FAILURE_CAUSE_MAX_LENGTH,
     ),
-    label: typeof code === "string" && code ? code : isError ? name : "",
+    label: isError ? name : "",
     stack: stack
       ? redactDiagnosticText(stack).slice(-PRE_PR_CHECKS_FAILURE_STACK_TAIL_MAX_LENGTH)
       : "",
@@ -2494,11 +2506,11 @@ export function prePrChecksFailureReport(
  * Redact, bound and log a Pre-PR checks failure, and return the one sentence
  * the operator is allowed to see.
  *
- * A step, because both halves of this need the server: pino may only be used
- * inside a step, and the redactor reads process.env to find the secrets it
- * blanks. The checks themselves are no longer a step (they are launched
- * detached and polled across ticks), so without this the catch in workflow
- * scope could neither redact nor log.
+ * A step, because the logging needs the server: pino may only be used inside a
+ * step, never in workflow scope, and the checks themselves are no longer a step
+ * (they are launched detached and polled across ticks), so without this the
+ * catch in workflow scope could not log at all. The redaction is not what
+ * forces a step, it already ran in workflow scope before the boundary.
  */
 export async function describePrePrChecksFailureStep(
   error: PrePrChecksFailureInput,
@@ -2541,7 +2553,13 @@ export async function prePrChecksFailureMessage(
   const input = prePrChecksFailureInput(error);
   try {
     return await describePrePrChecksFailureStep(input, configurationVersion);
-  } catch {
+  } catch (reportingError) {
+    // A cancelled run surfaces at every step, this one included, and it is not
+    // a reporting failure: swallowing it would turn a cancellation into a
+    // Pre-PR checks failure and let the run carry on being cancelled anyway.
+    // Same rethrow this file makes at every other step boundary, including the
+    // catch that calls this one.
+    if (isRunControlError(reportingError)) throw reportingError;
     return `The Pre-PR checks step failed (${input.name.slice(0, 60)}), and the cause could not be recorded.`;
   }
 }

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

@@ -101,7 +101,7 @@ function collected(overrides: {
     exitCode: number;
     stdout: string;
     stderr: string;
-    phase?: "setup" | "workspace";
+    phase?: "setup" | "workspace" | "batch" | "omitted";
   }>;
   setupFailed?: boolean;
   progress?: { completed: number; total: number; stoppedAt: string | null };
@@ -319,6 +319,47 @@ describe("runPrePrChecksWithFixes", () => {
     expect(mocks.startPrePrRepairStep).not.toHaveBeenCalled();
   });
 
+  it("never reports a stall as a check whose fix cycles were suppressed", async () => {
+    // A stall is not a check result: nothing was verified. Without a phase it
+    // reads as an ordinary failing check, so with a setup failure earlier in
+    // the same pass it collects the sentence explaining that its fix cycles
+    // were suppressed, and it would be handed to the repair agent to fix.
+    mocks.pollPhaseUntilDone
+      .mockImplementationOnce(pollEnds("finished", 30_000))
+      .mockImplementation(pollEnds("duration_cap", 1_500_000));
+    mocks.checkPhaseDone.mockResolvedValue(false);
+    mocks.collectRepoCheckBatchStep
+      .mockResolvedValueOnce(
+        collected({
+          setupFailed: true,
+          failures: [{
+            provider: "github",
+            repoPath: "acme/web",
+            command: "make bootstrap",
+            exitCode: 127,
+            stdout: "",
+            stderr: "toolchain: command not found",
+            phase: "setup",
+          }],
+        }),
+      )
+      .mockResolvedValueOnce(
+        collected({ progress: { completed: 0, total: 1, stoppedAt: "pnpm test" } }),
+      );
+
+    const result = await runPrePrChecksWithFixes(options({ config }));
+
+    const entries = result.summary.split("\n\n");
+    const setupEntry = entries.find((entry) => entry.startsWith("SETUP FAILED"));
+    const stallEntry = entries.find((entry) => entry.includes("this is a timeout"));
+    expect(setupEntry).toBeDefined();
+    expect(stallEntry).toContain("CHECK BATCH ABANDONED for gitlab:acme/api");
+    // The proof the phase is doing the work: an ordinary failing check in this
+    // same summary would carry the suppression sentence.
+    expect(stallEntry).not.toContain("No agent fix cycles were run");
+    expect(mocks.startPrePrRepairStep).not.toHaveBeenCalled();
+  });
+
   it("keeps the stall diagnosis when the abandoned batch cannot be read either", async () => {
     // The sandbox-death path collects from a sandbox already observed as not
     // running twice, and the collect step's maxRetries is 0. Letting that

apps/worker/src/workflows/agent.ts

@@ -330,10 +330,6 @@ export async function runRepoCheckBatch(args: {
   fixCycle: number;
   repoIndex: number;
   requireChange: boolean;
-  /** Whether an exit-0 check that says its dependencies are missing counts as
-   *  a failure. Only the configured checks carry that history; see
-   *  collectRepoCheckBatchStep. */
-  scanBlockedDependencies: boolean;
   observeBudget: PrePrChecksOptions["observeBudget"];
   cancellation?: V2InvocationCancellation;
 }): Promise<RepoCheckBatchRun> {
@@ -363,7 +359,6 @@ export async function runRepoCheckBatch(args: {
       started.paths,
       started.localPath,
       batchFinished,
-      args.scanBlockedDependencies,
     );
 
   const outcome = newPhasePollOutcome();
@@ -465,7 +460,6 @@ async function runCheckBatches(
       fixCycle,
       repoIndex,
       requireChange: true,
-      scanBlockedDependencies: true,
       observeBudget: options.observeBudget,
       cancellation: options.cancellation,
     });
@@ -599,6 +593,12 @@ function stalledBatches(
     exitCode: -1,
     stdout: "",
     stderr: batchStallReason(stall, elapsedMs, collected.progress),
+    // The batch never reported, so this is not a check result at all. Without a
+    // phase it reads as an ordinary failing check: it would be handed to the
+    // repair agent, and under a setup failure elsewhere it would collect the
+    // sentence saying its fix cycles were suppressed, when the reason nothing
+    // was fixed is that nothing was verified.
+    phase: "batch",
   };
   const allResults = [...results, ...collected.results];
   const allFailures = [...failures, ...collected.failures, stallFailure];

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

@@ -267,14 +267,41 @@ describe("run_checks execute", () => {
     }
   });
 
-  it("asks for no missing-dependency scan, which this mode never had", async () => {
-    // The block runs whatever an author typed and its description promises
-    // nothing but the exit code. Six English phrases must not turn a green
-    // suite into a failed check; this repository's own test names contain the
-    // exact wording.
-    await execute(makeNode("run_checks", { commands: ["pnpm test"] }), {}, makeCtx());
-
-    expect(mocks.collectRepoCheckBatchStep.mock.calls[0]![8]).toBe(false);
+  it("keeps the sentence explaining a failure out of the truncated payload", async () => {
+    // A check that exits 0 because it never ran is reported as a failure, and
+    // the only thing saying why is the note. This block's failure shape has one
+    // `output` string, so the note is appended after the bound: folded in
+    // before it, it would sit at the join between the two streams, which is
+    // exactly the middle a head-and-tail bound deletes.
+    mocks.collectRepoCheckBatchStep.mockResolvedValue({
+      results: [{ provider: "github", repoPath: "acme/api", command: "yarn test", exitCode: 0 }],
+      failures: [
+        {
+          provider: "github",
+          repoPath: "acme/api",
+          command: "yarn test",
+          exitCode: 0,
+          stdout: `HEAD${"y".repeat(40_000)}TAIL`,
+          stderr: "",
+          note: "Pre-PR check exited 0 but its dependencies are not installed.",
+        },
+      ],
+      setupFailed: false,
+    });
+
+    const result = await execute(
+      makeNode("run_checks", { commands: ["yarn test"] }),
+      {},
+      makeCtx(),
+    );
+
+    const { failures } = result.output! as unknown as {
+      failures: Array<{ output: string }>;
+    };
+    const output = failures[0]!.output;
+    expect(output).toContain("dependencies are not installed");
+    expect(output).toContain("HEAD");
+    expect(output).toContain("TAIL");
   });
 
   it("fails the block when an explicit batch stalls, never reporting a partial pass", async () => {

apps/worker/src/workflows/blocks/pre-pr-checks.ts

@@ -4,6 +4,7 @@ import {
   listWorkspaceRepositoriesStep,
   type CheckOutcome,
   type CollectedRepoCheckBatch,
+  type PrePrCheckFailure,
 } from "../../pre-pr-checks/runner.js";
 import {
   RunBudgetError,
@@ -68,18 +69,39 @@ function toBlockResults(
 function toBlockFailures(
   collected: CollectedRepoCheckBatch,
 ): RunChecksStepResult["failures"] {
-  return collected.failures.map((failure) => ({
+  return collected.failures.map(toBlockFailure);
+}
+
+function toBlockFailure(
+  failure: PrePrCheckFailure,
+): RunChecksStepResult["failures"][number] {
+  return {
     repo: `${failure.provider}:${failure.repoPath}`,
     command: failure.command,
     exitCode: failure.exitCode,
-    output: boundFailureOutput(
-      [failure.stderr, failure.stdout]
-        .map((part) => part.trim())
-        .filter(Boolean)
-        .join("\n"),
-      OUTPUT_TRUNCATE,
-    ),
-  }));
+    output: failureOutput(failure),
+  };
+}
+
+/**
+ * The command's own output, bounded, then the note on its own line.
+ *
+ * The note is appended AFTER the bound on purpose. This block's failure shape
+ * has one `output` string and no field for a note, so folding the note into a
+ * stream before bounding puts it at the join between stderr and stdout, which
+ * is the middle a head-and-tail bound deletes: an operator would read
+ * `exitCode: 0` under a heading that says failures with nothing saying why.
+ */
+function failureOutput(failure: PrePrCheckFailure): string {
+  const output = boundFailureOutput(
+    [failure.stderr, failure.stdout]
+      .map((part) => part.trim())
+      .filter(Boolean)
+      .join("\n"),
+    OUTPUT_TRUNCATE,
+  );
+  if (!failure.note) return output;
+  return output ? `${output}\n${failure.note}` : failure.note;
 }
 
 /**
@@ -121,11 +143,6 @@ async function runExplicitCommands(
       // Every attached repository runs, changed or not. That is this mode's
       // contract, and it never inspected HEAD before.
       requireChange: false,
-      // And it never scanned output for missing-dependency phrases either. The
-      // block runs whatever an author typed and promises nothing but the exit
-      // code, so a green suite whose output mentions one of six English
-      // sentences must not come back failed.
-      scanBlockedDependencies: false,
       observeBudget,
       cancellation,
     });
@@ -202,18 +219,7 @@ async function runConfiguredChecks(
     observeBudget,
     cancellation,
   });
-  const failures = run.failures.map((failure) => ({
-    repo: `${failure.provider}:${failure.repoPath}`,
-    command: failure.command,
-    exitCode: failure.exitCode,
-    output: boundFailureOutput(
-      [failure.stderr, failure.stdout]
-        .map((part) => part.trim())
-        .filter(Boolean)
-        .join("\n"),
-      OUTPUT_TRUNCATE,
-    ),
-  }));
+  const failures = run.failures.map(toBlockFailure);
   const results = (run.results ?? run.failures).map((result) => ({
     repo: `${result.provider}:${result.repoPath}`,
     command: result.command,