fix(queue): verify the latest reopen actor before re-closing a disallowed reopen

src/github/pr-actions.ts

@@ -191,11 +191,34 @@ export async function closePullRequest(env: Env, installationId: number, repoFul
  *  the inspected window". The reopen guard uses this to fail CLOSED rather than allow a window-evasion bypass. */
 export type LastCloserResult = { login: string | null; coveredAllPages: boolean };
 
+/** Event-agnostic alias for {@link LastCloserResult} — the shape is identical for any single timeline-event-type
+ *  lookup (e.g. "closed" or "reopened"); kept as an alias rather than a rename so existing importers of
+ *  `LastCloserResult` are unaffected. */
+export type LastTimelineActorResult = LastCloserResult;
+
 /** Reopen-prevention (#one-shot-reopen): the login of whoever LAST closed this PR (most recent `closed` event in
  *  the issue-events timeline), or null if none / on error. Lets the reopen handler distinguish a maintainer/bot
  *  close (one-shot — a contributor may not reopen) from a contributor self-close (which they MAY reopen).
  *  `coveredAllPages` reports whether the bounded scan inspected the entire timeline (#audit-2.4). */
 export async function getLastCloserLogin(env: Env, installationId: number, repoFullName: string, issueNumber: number): Promise<LastCloserResult> {
+  return getLastActorForEvent(env, installationId, repoFullName, issueNumber, "closed");
+}
+
+/** Reopen-race guard (#2369): the login of whoever most recently REOPENED this PR (most recent `reopened` event in
+ *  the issue-events timeline), or null if none / on error. Lets `maybeRecloseDisallowedReopen` re-verify — right
+ *  before it re-closes a disallowed reopen — that the reopen it is reacting to is still the CURRENT reason the PR
+ *  is open, rather than blindly undoing a DIFFERENT, later, legitimately-authorized reopen (e.g. a maintainer
+ *  reopening again after the original disallowed reopen). Same pagination/fail-conservative semantics as
+ *  {@link getLastCloserLogin}. */
+export async function getLastReopenerLogin(env: Env, installationId: number, repoFullName: string, issueNumber: number): Promise<LastTimelineActorResult> {
+  return getLastActorForEvent(env, installationId, repoFullName, issueNumber, "reopened");
+}
+
+/** Shared timeline-scan engine behind {@link getLastCloserLogin} and {@link getLastReopenerLogin}: finds the actor
+ *  of the most recent issue-event matching `eventType`, scanning the newest bounded page window rather than the
+ *  oldest prefix (see the pagination comments below — identical for either event type). Factored out so the two
+ *  callers do not duplicate the pagination/fail-conservative logic. */
+async function getLastActorForEvent(env: Env, installationId: number, repoFullName: string, issueNumber: number, eventType: string): Promise<LastTimelineActorResult> {
   try {
     const { owner, repo } = splitRepo(repoFullName);
     const token = await createInstallationToken(env, installationId);
@@ -208,47 +231,47 @@ export async function getLastCloserLogin(env: Env, installationId: number, repoF
     if (lastPage === null) {
       // No rel="last" in the Link header. A genuine single page has no rel="next" either — return page 1 directly.
       // But GitHub can paginate WITHOUT emitting rel="last" (only rel="next"); then trusting page 1 alone would let
-      // a later maintainer/bot close hide behind the un-enumerated tail and the reopen guard would fail OPEN. So
-      // follow rel="next" forward, tracking the latest close across pages (events are oldest-first → a later page's
-      // close supersedes), bounded by the same page budget. coveredAllPages holds ONLY if we reached the tail within
-      // budget; otherwise report not-covered so the caller fails closed. (#audit-rel-last)
+      // a later maintainer/bot event hide behind the un-enumerated tail and the reopen guard would fail OPEN. So
+      // follow rel="next" forward, tracking the latest matching event across pages (events are oldest-first → a
+      // later page's event supersedes), bounded by the same page budget. coveredAllPages holds ONLY if we reached
+      // the tail within budget; otherwise report not-covered so the caller fails closed. (#audit-rel-last)
       if (!issueEventsHasNextPage(firstResponse.headers.link)) {
-        return { login: latestCloserInPage(firstEvents) ?? null, coveredAllPages: true };
+        return { login: latestActorInPage(firstEvents, eventType) ?? null, coveredAllPages: true };
       }
-      let latestCloser = latestCloserInPage(firstEvents);
+      let latestActor = latestActorInPage(firstEvents, eventType);
       let hasNext = true;
       for (let page = 2; hasNext && page <= ISSUE_EVENTS_RECENT_PAGE_LIMIT + 1; page += 1) {
         const response = await requestPage(page);
-        const closer = latestCloserInPage(response.data as Array<{ event?: string; actor?: { login?: string | null } | null }>);
-        if (closer !== undefined) latestCloser = closer;
+        const actor = latestActorInPage(response.data as Array<{ event?: string; actor?: { login?: string | null } | null }>, eventType);
+        if (actor !== undefined) latestActor = actor;
         hasNext = issueEventsHasNextPage(response.headers.link);
       }
       const coveredAllPages = !hasNext;
-      return { login: coveredAllPages ? (latestCloser ?? null) : null, coveredAllPages };
+      return { login: coveredAllPages ? (latestActor ?? null) : null, coveredAllPages };
     }
-    if (lastPage <= 1) return { login: latestCloserInPage(firstEvents) ?? null, coveredAllPages: true };
+    if (lastPage <= 1) return { login: latestActorInPage(firstEvents, eventType) ?? null, coveredAllPages: true };
 
     // GitHub returns issue-events oldest-first. Use the Link header to inspect the newest bounded window instead
-    // of the oldest prefix, so a long self-generated timeline cannot hide a later maintainer/bot close.
+    // of the oldest prefix, so a long self-generated timeline cannot hide a later maintainer/bot event.
     const firstPageToRead = Math.max(2, lastPage - ISSUE_EVENTS_RECENT_PAGE_LIMIT + 1);
     // We inspected the entire timeline only when the window reached page 2 (page 1 is read separately above).
     const coveredAllPages = firstPageToRead === 2;
     for (let page = lastPage; page >= firstPageToRead; page -= 1) {
       const response = await requestPage(page);
-      const closer = latestCloserInPage(response.data as Array<{ event?: string; actor?: { login?: string | null } | null }>);
-      if (closer !== undefined) return { login: closer, coveredAllPages };
+      const actor = latestActorInPage(response.data as Array<{ event?: string; actor?: { login?: string | null } | null }>, eventType);
+      if (actor !== undefined) return { login: actor, coveredAllPages };
     }
-    return { login: coveredAllPages ? (latestCloserInPage(firstEvents) ?? null) : null, coveredAllPages };
+    return { login: coveredAllPages ? (latestActorInPage(firstEvents, eventType) ?? null) : null, coveredAllPages };
   } catch {
     // On error we cannot prove we read the whole timeline — report not-covered so the caller decides conservatively.
     return { login: null, coveredAllPages: false };
   }
 }
 
-function latestCloserInPage(events: Array<{ event?: string; actor?: { login?: string | null } | null }>): string | null | undefined {
+function latestActorInPage(events: Array<{ event?: string; actor?: { login?: string | null } | null }>, eventType: string): string | null | undefined {
   for (let i = events.length - 1; i >= 0; i -= 1) {
     const entry = events[i];
-    if (entry?.event === "closed") return entry.actor?.login ?? null;
+    if (entry?.event === eventType) return entry.actor?.login ?? null;
   }
   return undefined;
 }

src/queue/processors.ts

@@ -389,6 +389,7 @@ import {
   closePullRequest,
   createIssueComment,
   getLastCloserLogin,
+  getLastReopenerLogin,
 } from "../github/pr-actions";
 import {
   loadLinkedIssueHardRules,
@@ -7656,6 +7657,39 @@ async function maybeRecloseDisallowedReopen(
     }).catch(() => undefined);
     return true; // handled (decision made); a newly-authorized reopener still counts as handled
   }
+  // Live re-check #3 (#2369): head/state freshness and the reopener's OWN permission are not the only things that
+  // can move in the window before this fires — a DIFFERENT person (e.g. an actual maintainer) can reopen the SAME
+  // PR again after the original disallowed reopen, which is a legitimate, authorized reopen. Since the PR was
+  // already open, that second reopen doesn't change head/state, so checks #1/#2 above cannot see it. Ask directly:
+  // is `reopener` STILL the most recent "reopened" actor on this PR's timeline? If someone else's reopen is now the
+  // live reason the PR is open, re-closing it would wrongly undo that person's authorized action.
+  const latestReopener = await getLastReopenerLogin(
+    env,
+    installationId,
+    repoFullName,
+    pr.number,
+  );
+  const latestReopenerLogin = latestReopener.login?.toLowerCase() ?? null;
+  // Ambiguous ("can't prove no later reopen exists beyond the inspected window") must fail CLOSED here — the
+  // opposite of the closer-lookup's fail-open bias above — because wrongly re-closing a maintainer-authorized PR
+  // is the worse failure mode than leaving a disallowed reopen unclosed for one more tick.
+  const reopenerWindowAmbiguous =
+    latestReopenerLogin == null && !latestReopener.coveredAllPages;
+  const reopenerSuperseded =
+    reopenerWindowAmbiguous || latestReopenerLogin !== reopener;
+  if (reopenerSuperseded) {
+    await recordAuditEvent(env, {
+      eventType: "github_app.reopen_reclosed",
+      actor: "gittensory",
+      targetKey: `${repoFullName}#${pr.number}`,
+      outcome: "denied",
+      detail: reopenerWindowAmbiguous
+        ? `could not confirm ${reopener} is still the most recent reopener (event window not fully covered) — reopen re-close not executed`
+        : `the current reopener is now ${latestReopenerLogin ?? "unknown"}, not ${reopener} — reopen re-close not executed`,
+      metadata: { deliveryId, repoFullName },
+    }).catch(() => undefined);
+    return true; // handled (decision made); a superseded/ambiguous reopener still counts as handled
+  }
   await createIssueComment(
     env,
     installationId,

test/unit/github-pr-actions.test.ts

@@ -1,6 +1,6 @@
 import { afterEach, describe, expect, it, vi } from "vitest";
 import { generateKeyPairSync } from "node:crypto";
-import { closePullRequest, createIssueComment, createPullRequestReview, createPullRequestReviewComments, dismissLatestBotApproval, getLastCloserLogin, mergePullRequest, updatePullRequestBranch } from "../../src/github/pr-actions";
+import { closePullRequest, createIssueComment, createPullRequestReview, createPullRequestReviewComments, dismissLatestBotApproval, getLastCloserLogin, getLastReopenerLogin, mergePullRequest, updatePullRequestBranch } from "../../src/github/pr-actions";
 import { createTestEnv } from "../helpers/d1";
 
 function envWithKey() {
@@ -273,6 +273,103 @@ describe("GitHub PR action primitives (#778)", () => {
     await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 24)).resolves.toEqual({ login: null, coveredAllPages: true });
   });
 
+  it("getLastReopenerLogin: walks paginated issue events to find the true most recent reopener (#2369)", async () => {
+    const calls: string[] = [];
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = input.toString();
+      calls.push(url);
+      if (url.includes("/access_tokens")) return Response.json({ token: "t" });
+      if (url.includes("/issues/117/events")) {
+        const page = new URL(url).searchParams.get("page");
+        if (page === "1") {
+          return Response.json([
+            ...Array.from({ length: 99 }, (_, index) => ({ event: "labeled", actor: { login: `labeler-${index}` } })),
+            { event: "reopened", actor: { login: "contributor" } },
+          ], { headers: { link: '<https://api.github.test/issues/117/events?per_page=100&page=2>; rel="last"' } });
+        }
+        if (page === "2") return Response.json([{ event: "reopened", actor: { login: "maintainer" } }]);
+      }
+      return new Response("unexpected", { status: 500 });
+    });
+
+    await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 117)).resolves.toEqual({ login: "maintainer", coveredAllPages: true });
+    expect(calls.some((url) => url.includes("per_page=100") && url.includes("page=1"))).toBe(true);
+    expect(calls.some((url) => url.includes("per_page=100") && url.includes("page=2"))).toBe(true);
+  });
+
+  it("getLastReopenerLogin: a single page with an EXPLICIT rel=\"last\" pointing at page 1 is read directly, no forward scan (#2369)", async () => {
+    // Distinct from the "no Link header at all" case above: here GitHub DOES emit rel="last", it just already
+    // points at page 1 (a genuinely single-page timeline), exercising the lastPage<=1 branch rather than the
+    // lastPage===null branch.
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = input.toString();
+      if (url.includes("/access_tokens")) return Response.json({ token: "t" });
+      if (url.includes("/issues/121/events")) {
+        return Response.json([{ event: "reopened", actor: { login: "contributor" } }], {
+          headers: { link: '<https://api.github.test/issues/121/events?per_page=100&page=1>; rel="last"' },
+        });
+      }
+      return new Response("unexpected", { status: 500 });
+    });
+    await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 121)).resolves.toEqual({ login: "contributor", coveredAllPages: true });
+  });
+
+  it("getLastReopenerLogin: a single (lastPage<=1) page with no matching event falls back to null (#2369)", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = input.toString();
+      if (url.includes("/access_tokens")) return Response.json({ token: "t" });
+      if (url.includes("/issues/122/events")) {
+        return Response.json([{ event: "labeled", actor: { login: "someone" } }], {
+          headers: { link: '<https://api.github.test/issues/122/events?per_page=100&page=1>; rel="last"' },
+        });
+      }
+      return new Response("unexpected", { status: 500 });
+    });
+    await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 122)).resolves.toEqual({ login: null, coveredAllPages: true });
+  });
+
+  it("getLastReopenerLogin: returns null when the events API throws (catch path, #2369)", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      if (input.toString().includes("/access_tokens")) return Response.json({ token: "t" });
+      throw new Error("network failure");
+    });
+    await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 118)).resolves.toEqual({ login: null, coveredAllPages: false });
+  });
+
+  it("getLastReopenerLogin: reads the newest bounded event pages instead of the oldest prefix (#2369)", async () => {
+    const fetchedPages: number[] = [];
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = input.toString();
+      if (url.includes("/access_tokens")) return Response.json({ token: "t" });
+      if (url.includes("/issues/119/events")) {
+        const page = Number(new URL(url).searchParams.get("page") ?? "1");
+        fetchedPages.push(page);
+        if (page === 1) {
+          return Response.json([{ event: "reopened", actor: { login: "stale-contributor" } }], {
+            headers: { link: '<https://api.github.test/issues/119/events?per_page=100&page=12>; rel="last"' },
+          });
+        }
+        const events = Array.from({ length: 100 }, (_, i) =>
+          page === 11 && i === 40 ? { event: "reopened", actor: { login: "maintainer" } } : { event: "labeled" },
+        );
+        return Response.json(events);
+      }
+      return new Response("unexpected", { status: 500 });
+    });
+    await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 119)).resolves.toEqual({ login: "maintainer", coveredAllPages: false });
+    expect(fetchedPages).toEqual([1, 12, 11]);
+    expect(fetchedPages).not.toContain(2);
+  });
+
+  it("getLastReopenerLogin: records null when a reopened event has a null actor (#2369)", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      if (input.toString().includes("/access_tokens")) return Response.json({ token: "t" });
+      if (input.toString().includes("/issues/120/events")) return Response.json([{ event: "reopened", actor: null }]);
+      return new Response("not found", { status: 404 });
+    });
+    await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 120)).resolves.toEqual({ login: null, coveredAllPages: true });
+  });
+
   it("dismisses the bot's own LATEST approve review, ignoring other reviewers and earlier bot reviews (#2254)", async () => {
     const calls: Array<{ method: string; url: string; body: Record<string, unknown> }> = [];
     vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {