fix(server): stop PR watch flapping while workflows await approval

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

@@ -3908,6 +3908,9 @@ const makeOrchestrator = Effect.fn("orchestrationV2.Orchestrator.layer")(functio
                     ...(existing.endedWatch?.owedChecks !== undefined && {
                       owedChecks: existing.endedWatch.owedChecks,
                     }),
+                    ...(existing.endedWatch?.awaitingApproval !== undefined && {
+                      awaitingApproval: existing.endedWatch.awaitingApproval,
+                    }),
                     ...(existing.endedWatch?.checksAwaitedSince !== undefined && {
                       checksAwaitedSince: existing.endedWatch.checksAwaitedSince,
                     }),

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

@@ -175,6 +175,8 @@ function watchesEqual(left: ThreadPullRequestWatch, right: ThreadPullRequestWatc
     left.passed === right.passed &&
     left.passedChecks.join("\n") === right.passedChecks.join("\n") &&
     (left.owedChecks ?? []).join("\n") === (right.owedChecks ?? []).join("\n") &&
+    (left.awaitingApproval === undefined) === (right.awaitingApproval === undefined) &&
+    (left.awaitingApproval ?? []).join("\n") === (right.awaitingApproval ?? []).join("\n") &&
     left.remarksThrough === right.remarksThrough &&
     left.remarkIds.join("\n") === right.remarkIds.join("\n") &&
     left.conflicting === right.conflicting &&

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

@@ -31,6 +31,8 @@ const watch = (overrides: Partial<ThreadPullRequestWatch> = {}): ThreadPullReque
   ...overrides,
 });
 
+const run = (id: number) => `https://github.com/o/r/actions/runs/${id}`;
+
 const check = (name: string, status: PullRequestCheck["status"]): PullRequestCheck => ({
   name,
   status,
@@ -79,6 +81,78 @@ const remark = (
 const noRemarks: ReadonlyArray<PullRequestComment> = [];
 
 describe("evaluatePullRequestWatch", () => {
+  // regression: #24575 fork workflows awaiting approval flapped between failed and passed
+  it("waits for workflow approval once, through reads that miss it, until the run finishes", () => {
+    const bot = check("bot", "success");
+    const approval = { ...check("CI", "action-required"), url: run(1) };
+    const gated = detail({ checks: [bot, approval] });
+    const first = evaluatePullRequestWatch(watch(), gated, noRemarks);
+    assert.deepEqual(first.changes, [{ kind: "checks-awaiting-approval", checks: [approval] }]);
+    const message = pullRequestWatchMessage({
+      number: 1,
+      url: "https://github.com/o/r/pull/1",
+      baseBranch: "main",
+      headSha: "aaaaaaaaaa",
+      report: first,
+    });
+    assert.equal(message.notification.outcome, "updated");
+
+    // GitHub's approval read can answer empty while the rest of the checks read fine.
+    const missing = evaluatePullRequestWatch(first.next, detail({ checks: [bot] }), noRemarks);
+    assert.deepEqual(missing.changes, []);
+    assert.isFalse(missing.next.passed);
+    assert.deepEqual(evaluatePullRequestWatch(missing.next, gated, noRemarks).changes, []);
+    const pushed = detail({ headSha: "bbbbbbbbbb", checks: [bot] });
+    assert.deepEqual(evaluatePullRequestWatch(missing.next, pushed, noRemarks).changes, [
+      { kind: "checks-passed", count: 1, required: false },
+    ]);
+
+    // Approved, the run's jobs carry their own names under the run's URL.
+    const job = { ...check("lint", "success"), url: `${run(1)}/job/7` };
+    const done = evaluatePullRequestWatch(missing.next, detail({ checks: [bot, job] }), noRemarks);
+    assert.deepEqual(done.changes, [{ kind: "checks-passed", count: 2, required: false }]);
+  });
+
+  it("does not pass or fail while the host cannot tell whether workflows await approval", () => {
+    const bot = check("bot", "success");
+    const unknown = {
+      ...check("Workflow approval status", "action-required"),
+      url: null,
+      placeholder: true,
+    };
+    const unread = evaluatePullRequestWatch(watch(), detail({ checks: [bot, unknown] }), noRemarks);
+    assert.deepEqual(unread.changes, []);
+    assert.isFalse(unread.next.passed);
+    const read = evaluatePullRequestWatch(unread.next, detail({ checks: [bot] }), noRemarks);
+    assert.deepEqual(read.changes, [{ kind: "checks-passed", count: 1, required: false }]);
+    for (const status of ["failure", "pending"] as const) {
+      const blocked = evaluatePullRequestWatch(
+        read.next,
+        detail({ checks: [{ ...bot, status }, unknown] }),
+        noRemarks,
+      );
+      assert.isFalse(blocked.next.passed);
+      assert.deepEqual(
+        evaluatePullRequestWatch(blocked.next, detail({ checks: [bot] }), noRemarks).changes,
+        [{ kind: "checks-passed", count: 1, required: false }],
+      );
+    }
+  });
+
+  it("still reports failures beside workflows awaiting approval", () => {
+    const approval = { ...check("CI", "action-required"), url: run(1) };
+    const broken = check("lint", "failure");
+    const report = evaluatePullRequestWatch(
+      watch(),
+      detail({ checks: [approval, broken] }),
+      noRemarks,
+    );
+    assert.deepEqual(report.changes, [
+      { kind: "checks-failed", failed: [broken] },
+      { kind: "checks-awaiting-approval", checks: [approval] },
+    ]);
+  });
+
   it("reports each failure at once, even while another check never finishes", () => {
     const bot = check("CodeRabbit", "pending");
     const first = detail({ checks: [check("lint", "failure"), check("test", "pending"), bot] });
@@ -429,6 +503,16 @@ describe("evaluatePullRequestWatch", () => {
     assert.isTrue(anHourLater(legacy, detail({ checks: [] })).every((kinds) => kinds.length === 0));
     const pushed = anHourLater(legacy, detail({ headSha: "bbbbbbbbbb", checks: [] }));
     assert.deepEqual(pushed.flat(), ["no-checks"]);
+    const unknown = detail({
+      checks: [{ ...check("Workflow approval status", "action-required"), placeholder: true }],
+    });
+    assert.deepEqual(anHourLater(watch(), unknown).flat(), []);
+    assert.deepEqual(anHourLater(legacy, { ...unknown, headSha: "bbbbbbbbbb" }).flat(), []);
+    const unread = evaluate(watch(), unknown, noRemarks, 0);
+    assert.deepEqual(
+      evaluate(unread.next, detail({ checks: [] }), noRemarks, 60 * 60_000).changes,
+      [{ kind: "no-checks", minutes: 60 }],
+    );
   });
 
   it("never reports missing checks on a host where an empty list says nothing", () => {

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

@@ -17,6 +17,7 @@ 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-awaiting-approval"; readonly checks: ReadonlyArray<PullRequestCheck> }
   | { readonly kind: "remarks"; readonly remarks: ReadonlyArray<PullRequestComment> }
   | { readonly kind: "no-checks"; readonly minutes: number }
   | { readonly kind: "conflicting" };
@@ -27,9 +28,12 @@ export interface PullRequestWatchReport {
   readonly exhausted: boolean;
 }
 
-// action-required is a finished check needing human input.
 const isFailedCheck = (check: PullRequestCheck) =>
-  check.status === "failure" || check.status === "cancelled" || check.status === "action-required";
+  check.status === "failure" || check.status === "cancelled";
+
+// A GitHub Actions job's URL extends its workflow run's URL; their names need not match.
+const workflowRunOf = (url: string | null) => url?.match(/^.*\/actions\/runs\/\d+/)?.[0] ?? null;
+const approvalKey = (check: PullRequestCheck) => workflowRunOf(check.url) ?? check.name;
 
 export function evaluatePullRequestWatch(
   watch: ThreadPullRequestWatch,
@@ -50,15 +54,54 @@ export function evaluatePullRequestWatch(
   let passed = headMoved ? false : watch.passed;
   let passedChecks = headMoved ? [] : watch.passedChecks;
   let owedChecks = headMoved ? [] : (watch.owedChecks ?? []);
-  if (detail.checks.length > 0) {
-    const failed = detail.checks.filter(isFailedCheck);
+  let awaitingApproval = headMoved ? [] : (watch.awaitingApproval ?? []);
+  // "action-required" waits on someone, such as a maintainer approving a fork's workflows, so the
+  // agent is told once and nothing passes meanwhile. A placeholder is a host that could not tell,
+  // so it counts as no check and never confirms a pass, though a failed, pending, or awaited check
+  // beside it ends one.
+  const unread = detail.checks.some((check) => check.placeholder === true);
+  const checks = detail.checks.filter((check) => check.placeholder !== true);
+  const failed = checks.filter(isFailedCheck);
+  const awaiting = checks.filter((check) => check.status === "action-required");
+  if (checks.length > 0) {
+    // A read beside a placeholder may have missed checks, so it lets go only of those it lists.
+    const missed = (name: string) => unread && !checks.some((check) => check.name === name);
     const newlyFailed = failed.filter((check) => !failedChecks.includes(check.name));
     if (newlyFailed.length > 0) changes.push({ kind: "checks-failed", failed: newlyFailed });
-    failedChecks = failed.map((check) => check.name);
+    // A watch saved before awaitingApproval reported these as failed checks.
+    const toldBefore =
+      awaitingApproval.length > 0 ||
+      (watch.awaitingApproval === undefined &&
+        awaiting.every((check) => failedChecks.includes(check.name)));
+    if (awaiting.length > 0 && !toldBefore) {
+      changes.push({ kind: "checks-awaiting-approval", checks: awaiting });
+    }
+    failedChecks = [...failedChecks.filter(missed), ...failed.map((check) => check.name)];
 
-    const required = detail.checks.filter((check) => check.required === true);
-    const gate = required.length > 0 ? required : detail.checks;
-    const passedNow = gate.every((check) => check.status !== "pending" && !isFailedCheck(check));
+    // GitHub lists runs awaiting approval in a separate read that can come back empty, so a run
+    // stays awaited through reads that miss it until one of its jobs finishes. One known only by
+    // name stays only while listed, or through a read beside a placeholder that does not list it.
+    const awaitingNow = [...new Set(awaiting.map(approvalKey))];
+    const finished = new Set(
+      checks
+        .filter((check) => check.status !== "action-required" && check.status !== "pending")
+        .map((check) => workflowRunOf(check.url)),
+    );
+    awaitingApproval = [
+      ...awaitingNow,
+      ...awaitingApproval.filter(
+        (key) =>
+          !awaitingNow.includes(key) &&
+          (workflowRunOf(key) === key ? !finished.has(key) : missed(key)),
+      ),
+    ];
+  }
+  const required = checks.filter((check) => check.required === true);
+  const gate = required.length > 0 ? required : checks;
+  const passedNow =
+    awaitingApproval.length === 0 &&
+    gate.every((check) => check.status !== "pending" && !isFailedCheck(check));
+  if (checks.length > 0 && !(unread && passedNow)) {
     const gateNames = gate.map((check) => check.name);
     const told = passed && passedChecks.length === 0 ? gateNames : passedChecks;
     // Required checks can finish between sweeps; detect new names. With no required checks, ignore advisory gate growth.
@@ -67,23 +110,25 @@ export function evaluatePullRequestWatch(
     // Only those are owed, so later advisory churn never wakes. One turned required is gate news.
     const owed =
       passedNow && passed
-        ? detail.checks
+        ? checks
             .filter((check) => check.required !== true && owedChecks.includes(check.name))
             .map((check) => check.name)
         : [];
     const settled =
-      failed.length === 0 && detail.checks.every((check) => check.status !== "pending");
+      failed.length === 0 &&
+      awaitingApproval.length === 0 &&
+      checks.every((check) => check.status !== "pending");
     owedChecks = owed;
     if (passedNow && (!passed || gateGrew)) {
       changes.push({ kind: "checks-passed", count: gate.length, required: required.length > 0 });
       owedChecks =
         required.length > 0
-          ? detail.checks
+          ? checks
               .filter((check) => check.required !== true && check.status === "pending")
               .map((check) => check.name)
           : [];
     } else if (settled && owed.length > 0) {
-      changes.push({ kind: "checks-passed", count: detail.checks.length, required: false });
+      changes.push({ kind: "checks-passed", count: checks.length, required: false });
       owedChecks = [];
     }
     passed = passedNow;
@@ -99,9 +144,9 @@ export function evaluatePullRequestWatch(
     const awaitedBefore = headMoved
       ? DateTime.formatIso(DateTime.makeUnsafe(now))
       : (watch.checksAwaitedSince ?? null);
-    checksAwaitedSince = detail.checks.length > 0 ? null : awaitedBefore;
+    checksAwaitedSince = checks.length > 0 ? null : awaitedBefore;
     const awaitedMs = checksAwaitedSince === null ? 0 : now - Date.parse(checksAwaitedSince);
-    if (awaitedMs >= PULL_REQUEST_WATCH_NO_CHECKS_MS) {
+    if (!unread && awaitedMs >= PULL_REQUEST_WATCH_NO_CHECKS_MS) {
       changes.push({ kind: "no-checks", minutes: Math.round(awaitedMs / 60_000) });
       checksAwaitedSince = null;
     }
@@ -148,6 +193,7 @@ export function evaluatePullRequestWatch(
       passed,
       passedChecks,
       owedChecks,
+      awaitingApproval,
       remarksThrough,
       remarkIds,
       conflicting,
@@ -190,6 +236,11 @@ function changeLines(
             `  - ${check.name}${check.status === "failure" ? "" : ` (${check.status})`}${check.url ? ` ${check.url}` : ""}`,
         ),
       ];
+    case "checks-awaiting-approval":
+      return [
+        `- Checks are waiting for approval or action from a maintainer${context.commit}:`,
+        ...listed(change.checks, (check) => `  - ${check.name}${check.url ? ` ${check.url}` : ""}`),
+      ];
     case "checks-passed":
       return [
         `- All ${change.count} ${change.required ? "required " : ""}${change.count === 1 ? "check" : "checks"} passed${context.commit}.`,
@@ -216,6 +267,7 @@ function changeLines(
 const SUMMARY: Record<PullRequestWatchChange["kind"], string> = {
   "checks-failed": "checks failed",
   "checks-passed": "checks passed",
+  "checks-awaiting-approval": "checks await approval",
   remarks: "new comments",
   "no-checks": "no checks reported",
   conflicting: "merge conflict",