fix(server): PR watches wake when checks finish after an early failure

apps/server/src/orchestration-v2/pullRequestWatch.test.ts

@@ -205,6 +205,140 @@ describe("evaluatePullRequestWatch", () => {
     assert.deepEqual(evaluatePullRequestWatch(final.next, recovered, noRemarks).changes, []);
   });
 
+  it("wakes once when the other checks finish after one failed where none is required", () => {
+    const sonar = check("sonar", "cancelled");
+    const early = detail({ checks: [sonar, check("unit", "pending"), check("e2e", "pending")] });
+    const first = evaluatePullRequestWatch(watch(), early, noRemarks);
+    assert.deepEqual(first.changes, [{ kind: "checks-failed", failed: [sonar] }]);
+
+    const partway = detail({ checks: [sonar, check("unit", "success"), check("e2e", "pending")] });
+    const waiting = evaluatePullRequestWatch(first.next, partway, noRemarks);
+    assert.deepEqual(waiting.changes, []);
+
+    const done = detail({ checks: [sonar, check("unit", "success"), check("e2e", "success")] });
+    const final = evaluatePullRequestWatch(waiting.next, done, noRemarks);
+    assert.deepEqual(final.changes, [
+      { kind: "checks-finished", failing: [sonar], required: false },
+    ]);
+    assert.deepEqual(evaluatePullRequestWatch(final.next, done, noRemarks).changes, []);
+
+    // An owed check stays owed while it runs, even when the failed one turns required.
+    const gated = detail({
+      checks: [{ ...sonar, required: true }, check("unit", "success"), check("e2e", "pending")],
+    });
+    const still = evaluatePullRequestWatch(waiting.next, gated, noRemarks);
+    assert.deepEqual(still.changes, []);
+    assert.deepEqual(still.next.owedChecks, ["e2e"]);
+
+    // Another gate failure owes the gate's running checks on top of those still owed.
+    const lint = { ...check("lint", "failure"), required: true };
+    const another = detail({ checks: [...gated.checks, lint] });
+    const both = evaluatePullRequestWatch(still.next, another, noRemarks);
+    assert.deepEqual(both.changes, [{ kind: "checks-failed", failed: [lint] }]);
+    assert.deepEqual(both.next.owedChecks, ["e2e"]);
+
+    // The last owed check finishing still wakes when an unrelated check fails in the same read.
+    const bot = check("bot", "failure");
+    const finishedWithBot = detail({
+      checks: [sonar, check("unit", "success"), check("e2e", "success"), bot],
+    });
+    assert.deepEqual(evaluatePullRequestWatch(waiting.next, finishedWithBot, noRemarks).changes, [
+      { kind: "checks-failed", failed: [bot] },
+      { kind: "checks-finished", failing: [sonar, bot], required: false },
+    ]);
+
+    // A later failure is its own wake and leaves nothing more owed.
+    const broke = detail({ checks: [sonar, check("unit", "success"), check("e2e", "failure")] });
+    const failure = evaluatePullRequestWatch(waiting.next, broke, noRemarks);
+    assert.deepEqual(failure.changes, [
+      { kind: "checks-failed", failed: [check("e2e", "failure")] },
+    ]);
+    assert.deepEqual(evaluatePullRequestWatch(failure.next, broke, noRemarks).changes, []);
+
+    // A failure while another owed check still runs keeps that one owed.
+    const unitBroke = detail({
+      checks: [sonar, check("unit", "failure"), check("e2e", "pending")],
+    });
+    const second = evaluatePullRequestWatch(first.next, unitBroke, noRemarks);
+    assert.deepEqual(second.changes, [
+      { kind: "checks-failed", failed: [check("unit", "failure")] },
+    ]);
+    const settled = detail({ checks: [sonar, check("unit", "failure"), check("e2e", "success")] });
+    const last = evaluatePullRequestWatch(second.next, settled, noRemarks);
+    assert.deepEqual(last.changes, [
+      { kind: "checks-finished", failing: [sonar, check("unit", "failure")], required: false },
+    ]);
+  });
+
+  it("wakes once when the other required checks finish after a required one failed", () => {
+    const required = (name: string, status: PullRequestCheck["status"]) => ({
+      ...check(name, status),
+      required: true,
+    });
+    const early = detail({
+      checks: [
+        required("lint", "failure"),
+        required("unit", "pending"),
+        check("bot", "failure"),
+        check("e2e", "pending"),
+      ],
+    });
+    const first = evaluatePullRequestWatch(watch(), early, noRemarks);
+    assert.equal(first.changes.length, 1);
+    assert.deepEqual(first.next.owedChecks, ["unit"]);
+
+    const done = detail({
+      checks: [
+        required("lint", "failure"),
+        required("unit", "success"),
+        check("bot", "failure"),
+        check("e2e", "pending"),
+      ],
+    });
+    const final = evaluatePullRequestWatch(first.next, done, noRemarks);
+    assert.deepEqual(final.changes, [
+      { kind: "checks-finished", failing: [required("lint", "failure")], required: true },
+    ]);
+    assert.deepEqual(evaluatePullRequestWatch(final.next, done, noRemarks).changes, []);
+    const message = pullRequestWatchMessage({
+      number: 7,
+      url: "https://github.com/o/r/pull/7",
+      baseBranch: "main",
+      headSha: "aaaaaaaaaa",
+      report: final,
+    });
+    assert.deepEqual(message.notification.outcome, "failed");
+    assert.include(
+      message.text,
+      "- The required checks still running when one failed have finished on aaaaaaa. Still failing:\n  - lint https://ci.example/lint",
+    );
+
+    // A new advisory failure keeps what was owed rather than owing a required check started since.
+    const advisory = detail({
+      checks: [
+        required("lint", "failure"),
+        required("unit", "pending"),
+        required("smoke", "pending"),
+        check("bot", "failure"),
+        check("e2e", "failure"),
+      ],
+    });
+    const later = evaluatePullRequestWatch(first.next, advisory, noRemarks);
+    assert.deepEqual(later.changes, [{ kind: "checks-failed", failed: [check("e2e", "failure")] }]);
+    assert.deepEqual(later.next.owedChecks, ["unit"]);
+
+    // A rerun of the failed required check is not the gate finishing, even with an advisory failure.
+    const rerun = detail({
+      checks: [
+        required("lint", "pending"),
+        required("unit", "success"),
+        check("bot", "failure"),
+        check("e2e", "pending"),
+      ],
+    });
+    assert.deepEqual(evaluatePullRequestWatch(first.next, rerun, noRemarks).changes, []);
+  });
+
   it("clears old-head owed checks when a new head is pushed", () => {
     const required = { ...check("test", "success"), required: true };
     const early = detail({ checks: [required, check("e2e", "pending")] });

apps/server/src/orchestration-v2/pullRequestWatch.ts

@@ -17,6 +17,11 @@ const SNIPPET_LENGTH = 200;
 export type PullRequestWatchChange =
   | { readonly kind: "checks-failed"; readonly failed: ReadonlyArray<PullRequestCheck> }
   | { readonly kind: "checks-passed"; readonly count: number; readonly required: boolean }
+  | {
+      readonly kind: "checks-finished";
+      readonly failing: ReadonlyArray<PullRequestCheck>;
+      readonly required: boolean;
+    }
   | { readonly kind: "remarks"; readonly remarks: ReadonlyArray<PullRequestComment> }
   | { readonly kind: "no-checks"; readonly minutes: number }
   | { readonly kind: "conflicting" };
@@ -51,6 +56,7 @@ export function evaluatePullRequestWatch(
   let passedChecks = headMoved ? [] : watch.passedChecks;
   let owedChecks = headMoved ? [] : (watch.owedChecks ?? []);
   if (detail.checks.length > 0) {
+    const owedBefore = owedChecks;
     const failed = detail.checks.filter(isFailedCheck);
     const newlyFailed = failed.filter((check) => !failedChecks.includes(check.name));
     if (newlyFailed.length > 0) changes.push({ kind: "checks-failed", failed: newlyFailed });
@@ -85,6 +91,25 @@ export function evaluatePullRequestWatch(
     } else if (settled && owed.length > 0) {
       changes.push({ kind: "checks-passed", count: detail.checks.length, required: false });
       owedChecks = [];
+    } else if (gate.some(isFailedCheck)) {
+      // Gate checks still running when one failed are owed one wake when they finish, since
+      // the gate can no longer pass and so never sends the checks-passed wake.
+      const running = detail.checks
+        .filter((check) => check.status === "pending")
+        .map((check) => check.name);
+      const stillOwed = owedBefore.filter((name) => running.includes(name));
+      const gateNewlyFailed = newlyFailed.some((check) => gateNames.includes(check.name));
+      owedChecks = gateNewlyFailed
+        ? [...new Set([...stillOwed, ...running.filter((name) => gateNames.includes(name))])]
+        : stillOwed;
+      const owedFailed = newlyFailed.some((check) => owedBefore.includes(check.name));
+      if (!owedFailed && owedBefore.length > 0 && owedChecks.length === 0) {
+        changes.push({
+          kind: "checks-finished",
+          failing: gate.filter(isFailedCheck),
+          required: owedBefore.every((name) => required.some((check) => check.name === name)),
+        });
+      }
     }
     passed = passedNow;
     passedChecks = passedNow ? gateNames : [];
@@ -204,6 +229,14 @@ function changeLines(
           return `  - ${remark.author?.login ?? "someone"}${where}: ${said}${remark.url ? ` ${remark.url}` : ""}`;
         }),
       ];
+    case "checks-finished":
+      return [
+        `- The ${change.required ? "required " : ""}checks still running when one failed have finished${context.commit}. Still failing:`,
+        ...listed(
+          change.failing,
+          (check) => `  - ${check.name}${check.url ? ` ${check.url}` : ""}`,
+        ),
+      ];
     case "no-checks":
       return [
         `- No checks have reported${context.commit} after ${change.minutes} minutes. None may be configured to run on this change.`,
@@ -216,6 +249,7 @@ function changeLines(
 const SUMMARY: Record<PullRequestWatchChange["kind"], string> = {
   "checks-failed": "checks failed",
   "checks-passed": "checks passed",
+  "checks-finished": "checks finished",
   remarks: "new comments",
   "no-checks": "no checks reported",
   conflicting: "merge conflict",
@@ -242,7 +276,10 @@ export function pullRequestWatchMessage(input: {
       : "Look into each item and act on it as your task requires. This PR stays linked and watched, so do not link, watch, or list it again. End your turn when you are done, and T3 Code wakes you on the next change. When you hand the work back to the user, call unwatch_pull_request first so the thread returns to their inbox.",
   ].join("\n");
   const failed = changes.some(
-    (change) => change.kind === "checks-failed" || change.kind === "conflicting",
+    (change) =>
+      change.kind === "checks-failed" ||
+      change.kind === "checks-finished" ||
+      change.kind === "conflicting",
   );
   const summary = changes.map((change) => SUMMARY[change.kind]);
   if (exhausted) summary.push("stopped watching");