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

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

@@ -4057,6 +4057,12 @@ 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?.approvalAnnounced !== undefined && {
+                      approvalAnnounced: existing.endedWatch.approvalAnnounced,
+                    }),
                     ...(existing.endedWatch?.checksAwaitedSince !== undefined && {
                       checksAwaitedSince: existing.endedWatch.checksAwaitedSince,
                     }),

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

@@ -178,6 +178,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 ?? []).join("\n") === (right.awaitingApproval ?? []).join("\n") &&
+    (left.approvalAnnounced ?? false) === (right.approvalAnnounced ?? false) &&
     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,122 @@ 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 later = { ...check("Preview", "action-required"), url: run(2) };
+    assert.deepEqual(
+      evaluatePullRequestWatch(missing.next, detail({ checks: [bot, approval, later] }), noRemarks)
+        .changes,
+      [],
+    );
+    const unnamed = { ...check("Docs", "action-required"), url: null };
+    const named = detail({ checks: [bot, unnamed] });
+    const alone = evaluatePullRequestWatch(watch(), named, noRemarks);
+    assert.deepEqual(alone.changes, [{ kind: "checks-awaiting-approval", checks: [unnamed] }]);
+    const placeholder = { ...unnamed, name: "Workflow approval status", placeholder: true };
+    const unlisted = evaluatePullRequestWatch(
+      alone.next,
+      detail({ checks: [bot, placeholder] }),
+      noRemarks,
+    );
+    assert.deepEqual(unlisted.changes, []);
+    assert.isFalse(unlisted.next.passed);
+    assert.deepEqual(evaluatePullRequestWatch(unlisted.next, named, noRemarks).changes, []);
+    const dropped = evaluatePullRequestWatch(alone.next, detail({ checks: [bot] }), noRemarks);
+    assert.deepEqual(dropped.changes, []);
+    assert.isFalse(dropped.next.passed);
+    assert.deepEqual(dropped.next.awaitingApproval, [unnamed.name]);
+    assert.deepEqual(evaluatePullRequestWatch(dropped.next, named, noRemarks).changes, []);
+    const namedPending = evaluatePullRequestWatch(
+      dropped.next,
+      detail({
+        checks: [
+          { ...bot, required: true },
+          { ...unnamed, status: "pending" },
+        ],
+      }),
+      noRemarks,
+    );
+    assert.deepEqual(namedPending.changes, []);
+    assert.isFalse(namedPending.next.passed);
+    const namedDone = detail({ checks: [bot, { ...unnamed, status: "success" }] });
+    const namedFinished = evaluatePullRequestWatch(namedPending.next, namedDone, noRemarks);
+    assert.deepEqual(namedFinished.changes, [{ kind: "checks-passed", count: 2, required: false }]);
+    assert.deepEqual(namedFinished.next.awaitingApproval, []);
+    assert.deepEqual(
+      evaluatePullRequestWatch(namedFinished.next, namedDone, 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 pending = { ...check("test", "pending"), url: `${run(1)}/job/8` };
+    const required = { ...bot, required: true };
+    const running = evaluatePullRequestWatch(
+      missing.next,
+      detail({ checks: [required, job, pending] }),
+      noRemarks,
+    );
+    assert.deepEqual(running.changes, []);
+    assert.isFalse(running.next.passed);
+    assert.deepEqual(running.next.awaitingApproval, [run(1)]);
+    const completed = detail({ checks: [required, job, { ...pending, status: "success" }] });
+    const finished = evaluatePullRequestWatch(running.next, completed, noRemarks);
+    assert.deepEqual(finished.changes, [{ kind: "checks-passed", count: 1, required: true }]);
+    assert.deepEqual(finished.next.awaitingApproval, []);
+    assert.deepEqual(evaluatePullRequestWatch(finished.next, completed, noRemarks).changes, []);
+
+    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 }]);
+    const blocked = evaluatePullRequestWatch(
+      read.next,
+      detail({ checks: [{ ...bot, status: "pending" }, 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("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] });
@@ -95,6 +213,13 @@ describe("evaluatePullRequestWatch", () => {
 
     const pushed = detail({ ...second, headSha: "bbbbbbbbbb" });
     assert.equal(evaluatePullRequestWatch(test.next, pushed, noRemarks).changes.length, 1);
+
+    const approval = { ...check("CI", "action-required"), url: run(1) };
+    const gated = detail({ checks: [approval, check("lint", "failure")] });
+    assert.deepEqual(evaluatePullRequestWatch(watch(), gated, noRemarks).changes, [
+      { kind: "checks-failed", failed: [check("lint", "failure")] },
+      { kind: "checks-awaiting-approval", checks: [approval] },
+    ]);
   });
 
   it("reports passed once the required checks pass, whatever the others do", () => {
@@ -429,6 +554,15 @@ 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(), []);
+    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,60 @@ 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 ?? []);
+  let approvalAnnounced = headMoved ? false : (watch.approvalAnnounced ?? false);
+  // "action-required" waits on someone, such as a maintainer approving a fork's workflows, so the
+  // agent is told once per head 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) {
     const newlyFailed = failed.filter((check) => !failedChecks.includes(check.name));
     if (newlyFailed.length > 0) changes.push({ kind: "checks-failed", failed: newlyFailed });
+    if (!approvalAnnounced && awaiting.length > 0) {
+      changes.push({ kind: "checks-awaiting-approval", checks: awaiting });
+      approvalAnnounced = true;
+    }
     failedChecks = 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 all reported jobs finish. One known only by
+    // name needs a completed check with that name before its hold can clear.
+    const awaitingNow = [...new Set(awaiting.map(approvalKey))];
+    const nameUnfinished = (name: string) => {
+      const matching = checks.filter((check) => check.name === name);
+      return (
+        matching.length === 0 ||
+        matching.some((check) => check.status === "pending" || check.status === "action-required")
+      );
+    };
+    const pending = new Set(
+      checks.filter((check) => check.status === "pending").map((check) => workflowRunOf(check.url)),
+    );
+    const finished = new Set(
+      checks
+        .filter((check) => check.status !== "action-required" && check.status !== "pending")
+        .map((check) => workflowRunOf(check.url))
+        .filter((run) => !pending.has(run)),
+    );
+    awaitingApproval = [
+      ...awaitingNow,
+      ...awaitingApproval.filter(
+        (key) =>
+          !awaitingNow.includes(key) &&
+          (workflowRunOf(key) === key ? !finished.has(key) : nameUnfinished(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 +116,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 +150,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 +199,8 @@ export function evaluatePullRequestWatch(
       passed,
       passedChecks,
       owedChecks,
+      awaitingApproval,
+      approvalAnnounced,
       remarksThrough,
       remarkIds,
       conflicting,
@@ -190,6 +243,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 +274,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",