fix(github): sweep rescues never-reviewed PRs (ws-22l)

lib/plugins/__tests__/github-approve-on-green.test.ts

@@ -358,6 +358,8 @@ interface SweepPr {
   base?: string;
   /** HTTP status for a PUT /pulls/{n}/merge (default 200). */
   mergeStatus?: number;
+  /** ISO created_at for the pulls listing (default: undefined — reads as "too young" for rescue). */
+  createdAt?: string;
 }
 
 /** Stub GitHub REST for a reconciliation sweep over one repo's open PRs. */
@@ -386,7 +388,7 @@ function stubSweepFetch(prs: SweepPr[]): Captured {
       return json(prs.map((p) => ({
         number: p.number, state: "open", draft: false,
         head: { sha: p.sha }, base: { ref: p.base ?? "feature/x" },
-        title: "t", html_url: "u", user: { login: "dev" },
+        title: "t", html_url: "u", user: { login: "dev" }, created_at: p.createdAt,
       })));
     }
     if (url.match(/\/pulls\/\d+\/reviews/) && method === "GET") {
@@ -437,24 +439,27 @@ function approvedGreenPr(number: number, sha: string, base: string): SweepPr {
 
 async function runSweepWithPlugin(
   prs: SweepPr[],
-  opts: { seedWatch?: { key: string; ageMs: number } } = {},
-): Promise<{ captured: Captured; plugin: GitHubPlugin }> {
+  opts: { seedWatch?: { key: string; ageMs: number }; plugin?: GitHubPlugin } = {},
+): Promise<{ captured: Captured; plugin: GitHubPlugin; dispatched: string[] }> {
   const captured = stubSweepFetch(prs);
   const registry = new ProjectRegistry();
   // Registry is populated from a remote fetch in prod; stub the one accessor the
   // sweep reads so it iterates our fixture repo.
   (registry as unknown as { getGithubCoords: () => string[] }).getGithubCoords = () => [`${OWNER}/${REPO}`];
-  const plugin = new GitHubPlugin("/tmp/nonexistent-ws", registry);
+  const plugin = opts.plugin ?? new GitHubPlugin("/tmp/nonexistent-ws", registry);
   if (opts.seedWatch) {
     (plugin as unknown as { mergeCompletionFirstSeen: Map<string, number> }).mergeCompletionFirstSeen.set(
       opts.seedWatch.key,
       Date.now() - opts.seedWatch.ageMs,
     );
   }
+  const bus = new InMemoryEventBus();
+  const dispatched: string[] = [];
+  bus.subscribe("message.inbound.github.#", "t", (m) => { dispatched.push(m.topic); });
   await (plugin as unknown as {
-    _reconcileApproveOnGreen: (g: () => Promise<string>) => Promise<void>;
-  })._reconcileApproveOnGreen(async () => "fake-token");
-  return { captured, plugin };
+    _reconcileApproveOnGreen: (b: InMemoryEventBus, g: () => Promise<string>) => Promise<void>;
+  })._reconcileApproveOnGreen(bus, async () => "fake-token");
+  return { captured, plugin, dispatched };
 }
 
 describe("_reconcileApproveOnGreen — level-triggered backstop (#879)", () => {
@@ -547,6 +552,38 @@ describe("_reconcileApproveOnGreen — level-triggered backstop (#879)", () => {
     expect(watch2.size).toBe(1); // kept — retry next sweep
   });
 
+  test("rescues an old unreviewed terminal PR with ONE dispatched review; second sweep is silent (ws-22l)", async () => {
+    const stranded: SweepPr = {
+      number: 906, sha: "los0906", reviews: [], createdAt: new Date(Date.now() - 30 * 60_000).toISOString(),
+      checkRuns: [{ status: "completed", conclusion: "success" }],
+    };
+    const r1 = await runSweepWithPlugin([stranded]);
+    expect(r1.dispatched.length).toBe(1); // rescue review dispatched
+    expect(r1.captured.approvePosts.length).toBe(0); // never a blind approve
+
+    // same plugin, next sweep — the rescue map holds, no duplicate
+    const r2 = await runSweepWithPlugin([stranded], { plugin: r1.plugin });
+    expect(r2.dispatched.length).toBe(0);
+  });
+
+  test("does NOT rescue a young unreviewed PR (opened-dispatch may still be in flight)", async () => {
+    const fresh: SweepPr = {
+      number: 909, sha: "yng0909", reviews: [], createdAt: new Date().toISOString(),
+      checkRuns: [{ status: "completed", conclusion: "success" }],
+    };
+    const { dispatched } = await runSweepWithPlugin([fresh]);
+    expect(dispatched.length).toBe(0);
+  });
+
+  test("does NOT rescue an unreviewed PR whose CI is still running", async () => {
+    const pending: SweepPr = {
+      number: 910, sha: "pnd0910", reviews: [], createdAt: new Date(Date.now() - 30 * 60_000).toISOString(),
+      checkRuns: [{ status: "in_progress" }],
+    };
+    const { dispatched } = await runSweepWithPlugin([pending]);
+    expect(dispatched.length).toBe(0);
+  });
+
   test("covers a reviewed-but-UNREGISTERED repo (the approve-on-green gap fix)", async () => {
     // Registry is empty (repo not tagged / not in EXPLICIT), but Quinn reviewed
     // it this process → the sweep must still cover it via reviewedRepoCoords.
@@ -560,8 +597,8 @@ describe("_reconcileApproveOnGreen — level-triggered backstop (#879)", () => {
     const plugin = new GitHubPlugin("/tmp/nonexistent-ws", registry);
     (plugin as unknown as { reviewedRepoCoords: Set<string> }).reviewedRepoCoords.add(`${OWNER}/${REPO}`);
     await (plugin as unknown as {
-      _reconcileApproveOnGreen: (g: () => Promise<string>) => Promise<void>;
-    })._reconcileApproveOnGreen(async () => "fake-token");
+      _reconcileApproveOnGreen: (b: InMemoryEventBus, g: () => Promise<string>) => Promise<void>;
+    })._reconcileApproveOnGreen(new InMemoryEventBus(), async () => "fake-token");
     expect(captured.approvePosts.length).toBe(1);
     expect(captured.approvePosts[0]!.commit_id).toBe("por0038");
   });

lib/plugins/github.ts

@@ -522,7 +522,7 @@ export class GitHubPlugin implements Plugin {
     this.reconcileTimer = setInterval(() => {
       if (this.reconcileInFlight) return;
       this.reconcileInFlight = true;
-      void this._reconcileApproveOnGreen(getToken)
+      void this._reconcileApproveOnGreen(bus, getToken)
         .catch((err) => log.error("Approve-on-green reconciliation sweep failed", { err }))
         .finally(() => { this.reconcileInFlight = false; });
     }, GitHubPlugin.RECONCILE_INTERVAL_MS);
@@ -879,6 +879,19 @@ export class GitHubPlugin implements Plugin {
   private static readonly MERGE_COMPLETION_GRACE_MS = 2 * 60_000;
   private static readonly MERGE_COMPLETION_TTL_MS = 60 * 60_000;
 
+  /**
+   * PR@sha7 keys the sweep has already rescue-dispatched a review for
+   * (ws-22l). One rescue per head SHA per process — if the rescue run also
+   * dies, a restart or a new push retries. TTL-pruned alongside use.
+   */
+  private readonly reviewRescueDispatched = new Map<string, number>();
+  /**
+   * A PR younger than this can still have its opened-webhook review in
+   * flight; rescuing it would double-review. Lost-dispatch PRs are minutes
+   * old by the time CI is terminal, so the grace costs nothing real.
+   */
+  private static readonly RESCUE_MIN_AGE_MS = 10 * 60_000;
+
   private _handleAutoReview(
     event: string,
     payload: Record<string, unknown>,
@@ -1092,33 +1105,48 @@ export class GitHubPlugin implements Plugin {
 
     for (const pr of pulls) {
       const decision = await this._evaluateApproveOnGreen(event, owner, repo, pr, headSha, getToken);
-      if (decision !== "needs-review") continue;
-
-      // Eligible + terminal but not auto-approvable — re-dispatch Quinn's
-      // pr_review LLM pass to upgrade the provisional COMMENT to a formal verdict.
-      const number = pr.number as number;
-      const ctx: GitHubEventContext = {
-        owner,
-        repo,
-        number,
-        title: (pr.title as string | undefined) ?? "",
-        url: (pr.html_url as string | undefined) ?? "",
-        body: (pr.body as string | undefined) ?? "",
-        author: ((pr.user as Record<string, unknown> | undefined)?.login as string | undefined) ?? "",
-      };
-      // Synthesize the shape _handleAutoReview reads (pull_request.draft +
-      // pull_request.head.sha). skipDedup: the all-terminal gate already ensures
-      // a single meaningful dispatch; the dispatcher's @sha7 cooldown collapses
-      // any workflow_run+check_suite co-arrival.
-      const synthPayload: Record<string, unknown> = { action: "ci_completed", pull_request: pr };
-      log.info(`CI-completion (${event}): re-dispatching pr_review for ${owner}/${repo}#${number} @${headSha.slice(0, 7)}`);
-      // event="pull_request" so the published topic is byte-identical to a
-      // normal auto-review (the routed path is proven); the "ci_completed"
-      // action just suppresses the opened-only leading comment.
-      this._handleAutoReview("pull_request", synthPayload, ctx, bus, getToken, { skipDedup: true });
+      // needs-review: eligible + terminal but not auto-approvable — upgrade the
+      // provisional COMMENT to a formal verdict. unreviewed: the opened-webhook
+      // dispatch was lost (ws-22l) — same rescue, first review instead.
+      if (decision !== "needs-review" && decision !== "unreviewed") continue;
+      this._dispatchReviewFor(event, owner, repo, pr, headSha, bus, getToken);
     }
   }
 
+  /**
+   * Re-dispatch Quinn's pr_review LLM pass for a PR (the CI-completion
+   * upgrade path and the ws-22l unreviewed rescue). Synthesizes the shape
+   * `_handleAutoReview` reads (pull_request.draft + pull_request.head.sha).
+   * skipDedup: callers gate on all-terminal CI / rescue maps; the
+   * dispatcher's @sha7 cooldown collapses webhook co-arrival.
+   */
+  private _dispatchReviewFor(
+    source: string,
+    owner: string,
+    repo: string,
+    pr: Record<string, unknown>,
+    headSha: string,
+    bus: EventBus,
+    getToken: (owner: string, repo: string) => Promise<string>,
+  ): void {
+    const number = pr.number as number;
+    const ctx: GitHubEventContext = {
+      owner,
+      repo,
+      number,
+      title: (pr.title as string | undefined) ?? "",
+      url: (pr.html_url as string | undefined) ?? "",
+      body: (pr.body as string | undefined) ?? "",
+      author: ((pr.user as Record<string, unknown> | undefined)?.login as string | undefined) ?? "",
+    };
+    const synthPayload: Record<string, unknown> = { action: "ci_completed", pull_request: pr };
+    log.info(`CI-completion (${source}): re-dispatching pr_review for ${owner}/${repo}#${number} @${headSha.slice(0, 7)}`);
+    // event="pull_request" so the published topic is byte-identical to a
+    // normal auto-review (the routed path is proven); the "ci_completed"
+    // action just suppresses the opened-only leading comment.
+    this._handleAutoReview("pull_request", synthPayload, ctx, bus, getToken, { skipDedup: true });
+  }
+
   /**
    * Deterministic approve-on-terminal-green decision for a single PR (#748,
    * #848). Shared by the edge-triggered CI-completion webhook and the
@@ -1153,14 +1181,27 @@ export class GitHubPlugin implements Plugin {
     pr: Record<string, unknown>,
     headSha: string,
     getToken: (owner: string, repo: string) => Promise<string>,
-  ): Promise<"approved" | "merged" | "needs-review" | "skip"> {
+  ): Promise<"approved" | "merged" | "needs-review" | "unreviewed" | "skip"> {
     const number = pr.number as number;
     if (!prEligibleForCiReview(pr, headSha)) return "skip";
 
     const reviews = (await this._ghGet(getToken, owner, repo, `/repos/${owner}/${repo}/pulls/${number}/reviews`)) as
       | Array<Record<string, unknown>>
       | null;
-    if (!quinnHasReviewed(reviews ?? undefined)) return "skip";
+    if (!quinnHasReviewed(reviews ?? undefined)) {
+      // Never reviewed at all. The PR-opened webhook normally dispatched the
+      // review, but that in-process run dies with a container restart (the
+      // #906 strand, ws-22l) and nothing else re-triggers: the promote/merge
+      // machinery needs a prior review to act on. Signal "unreviewed" so the
+      // caller can dispatch a rescue review — only once CI is terminal (the
+      // meaningful review moment) and the PR is old enough that the original
+      // opened-dispatch can't still be in flight.
+      const createdAt = Date.parse((pr.created_at as string | undefined) ?? "");
+      const oldEnough = Number.isFinite(createdAt) && Date.now() - createdAt > GitHubPlugin.RESCUE_MIN_AGE_MS;
+      if (!oldEnough) return "skip";
+      if (!(await this._ciTerminal(getToken, owner, repo, headSha))) return "skip";
+      return "unreviewed";
+    }
     if (!(await this._ciTerminal(getToken, owner, repo, headSha))) {
       log.info(`CI-completion (${source}): ${owner}/${repo}#${number} — checks not all terminal yet, deferring`);
       return "skip";
@@ -1325,6 +1366,7 @@ export class GitHubPlugin implements Plugin {
    * GitHub's native auto-merge stays wedged.
    */
   private async _reconcileApproveOnGreen(
+    bus: EventBus,
     getToken: (owner: string, repo: string) => Promise<string>,
   ): Promise<void> {
     // Union the registry with repos Quinn has reviewed this process. A repo
@@ -1335,6 +1377,7 @@ export class GitHubPlugin implements Plugin {
     for (const c of this.reviewedRepoCoords) coords.add(c);
     let approved = 0;
     let merged = 0;
+    let rescued = 0;
     for (const coord of coords) {
       const slash = coord.indexOf("/");
       if (slash <= 0) continue;
@@ -1356,6 +1399,21 @@ export class GitHubPlugin implements Plugin {
           const decision = await this._evaluateApproveOnGreen("reconcile", owner, repo, pr, headSha, getToken);
           if (decision === "approved") approved++;
           else if (decision === "merged") merged++;
+          else if (decision === "unreviewed") {
+            // Lost opened-dispatch rescue (ws-22l): one review per head SHA
+            // per process — the dispatcher cooldown alone would let every
+            // 3-min sweep re-dispatch while a slow review is in flight.
+            const now = Date.now();
+            for (const [k, t] of this.reviewRescueDispatched) {
+              if (now - t > GitHubPlugin.MERGE_COMPLETION_TTL_MS) this.reviewRescueDispatched.delete(k);
+            }
+            const key = `${owner}/${repo}#${pr.number}@${headSha.slice(0, 7)}`;
+            if (!this.reviewRescueDispatched.has(key)) {
+              this.reviewRescueDispatched.set(key, now);
+              this._dispatchReviewFor("reconcile-rescue", owner, repo, pr, headSha, bus, getToken);
+              rescued++;
+            }
+          }
         } catch (err) {
           log.error(
             `Reconcile approve-on-green failed for ${owner}/${repo}#${pr.number}`,
@@ -1364,10 +1422,10 @@ export class GitHubPlugin implements Plugin {
         }
       }
     }
-    if (approved > 0 || merged > 0) {
+    if (approved > 0 || merged > 0 || rescued > 0) {
       log.info(
         `Approve-on-green reconciliation sweep: ${approved} stranded PR(s) approved, ` +
-          `${merged} wedged merge(s) completed`,
+          `${merged} wedged merge(s) completed, ${rescued} unreviewed PR(s) rescue-dispatched`,
       );
     }
   }