From 46475eaba3ffbcfb8f0248f7a46b9c9f7bc6862c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 14:45:29 -0700 Subject: [PATCH 1/3] fix(queue): verify the latest reopen actor before re-closing a disallowed reopen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Head/state freshness and a fresh re-check of the original reopener's own permission both miss a real race: a DIFFERENT person (e.g. an actual maintainer) can reopen the same PR again after the original disallowed reopen — a legitimate, authorized action. Since the PR was already open, that second reopen changes neither head SHA nor open/closed state, so neither existing live re-check can see it, and the handler would still wrongly undo the maintainer's reopen. Generalizes getLastCloserLogin's timeline-scanning pagination engine (src/github/pr-actions.ts) into a shared, event-type-parameterized helper, adds getLastReopenerLogin alongside it, and wires a third live re-check into maybeRecloseDisallowedReopen: deny the close unless the ORIGINAL disallowed reopener is still the most recent "reopened" actor on the PR's live timeline. Fails closed (denies) when the timeline scan can't prove that, mirroring the existing fail-closed precedent for an unresolvable freshness read. --- src/github/pr-actions.ts | 55 +++++++--- src/queue/processors.ts | 34 ++++++ test/unit/github-pr-actions.test.ts | 68 +++++++++++- test/unit/queue.test.ts | 164 +++++++++++++++++++++++++++- 4 files changed, 301 insertions(+), 20 deletions(-) diff --git a/src/github/pr-actions.ts b/src/github/pr-actions.ts index a08918c146..647c1327b6 100644 --- a/src/github/pr-actions.ts +++ b/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 { + 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 { + 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 { 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; } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index b6bd3abfe8..1e2cab6b1b 100644 --- a/src/queue/processors.ts +++ b/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, diff --git a/test/unit/github-pr-actions.test.ts b/test/unit/github-pr-actions.test.ts index 9d297adcb9..36d0830adf 100644 --- a/test/unit/github-pr-actions.test.ts +++ b/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,72 @@ 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: '; 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: 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: '; 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 }> = []; vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index e4282c1b20..ad1fc485d8 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -10717,7 +10717,10 @@ describe("one-shot reopen prevention", () => { if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); if (url.endsWith("/collaborators/contributor/permission")) return Response.json({ permission: "read" }); if (url.endsWith("/collaborators/maintainer/permission")) return Response.json({ permission: "write" }); - if (url.includes("/issues/42/events")) return Response.json([{ event: "closed", actor: { login: "maintainer" } }]); + // Both a "closed" event (by the write collaborator) AND a "reopened" event (by the contributor, still the + // most recent reopener) — the new live re-check (#2369) reads this same endpoint to confirm the contributor + // is still the current reopener before proceeding to close. + if (url.includes("/issues/42/events")) return Response.json([{ event: "closed", actor: { login: "maintainer" } }, { event: "reopened", actor: { login: "contributor" } }]); if (url.endsWith("/issues/42/comments")) return Response.json({ id: 99 }, { status: 201 }); if (url.endsWith("/pulls/42") && method === "PATCH") return Response.json({ state: "closed" }); return new Response("not found", { status: 404 }); @@ -10807,6 +10810,157 @@ describe("one-shot reopen prevention", () => { expect(audit?.detail).toContain("now holds maintainer permission"); }); + it("REGRESSION: does NOT re-close when a DIFFERENT (maintainer) reopener supersedes the original disallowed reopen (#2369)", async () => { + // The original contributor reopen is what triggered this handler, but by the time it runs, a real maintainer + // has ALSO reopened the same PR (a legitimate, authorized reopen is now the current reason it's open). Head/ + // state freshness and the reopener's OWN permission re-check both miss this — neither sees WHO most recently + // reopened. The timeline shows a "closed" event by "maintainer" (the original one-shot close) followed by a + // LATER "reopened" event by a different maintainer login ("second-maintainer"), after the contributor's own + // earlier reopen. + const calls: Array<{ url: string; method: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push({ url, method }); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/collaborators/contributor/permission")) return Response.json({ permission: "read" }); + if (url.endsWith("/collaborators/maintainer/permission")) return Response.json({ permission: "write" }); + if (url.includes("/issues/42/events")) { + return Response.json([ + { event: "closed", actor: { login: "maintainer" } }, + { event: "reopened", actor: { login: "contributor" } }, + { event: "closed", actor: { login: "maintainer" } }, + { event: "reopened", actor: { login: "second-maintainer" } }, + ]); + } + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await repositoriesModule.upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { merge: "auto", request_changes: "auto" } }); + + await processJob(env, { type: "github-webhook", deliveryId: "reopen-superseded", eventName: "pull_request", payload: reopenedPayload("contributor") }); + + expect(calls.some((c) => c.method === "POST" && c.url.endsWith("/issues/42/comments"))).toBe(false); + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("github_app.reopen_reclosed").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("denied"); + expect(audit?.detail).toContain("second-maintainer"); + expect(audit?.detail).toContain("not contributor"); + }); + + it("happy path unaffected: re-closes when the same reopener is still the latest reopener on the timeline (#2369)", async () => { + // Confirms the new live re-check does not spuriously block the ordinary case: the contributor is BOTH the + // original AND the still-current reopener (no one else reopened it again in the meantime). + const calls: Array<{ url: string; method: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push({ url, method }); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/collaborators/contributor/permission")) return Response.json({ permission: "read" }); + if (url.endsWith("/collaborators/maintainer/permission")) return Response.json({ permission: "write" }); + if (url.includes("/issues/42/events")) return Response.json([{ event: "closed", actor: { login: "maintainer" } }, { event: "reopened", actor: { login: "contributor" } }]); + if (url.endsWith("/issues/42/comments")) return Response.json({ id: 99 }, { status: 201 }); + if (url.endsWith("/pulls/42") && method === "PATCH") return Response.json({ state: "closed" }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await repositoriesModule.upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { merge: "auto", request_changes: "auto" } }); + + await processJob(env, { type: "github-webhook", deliveryId: "reopen-same-latest", eventName: "pull_request", payload: reopenedPayload("contributor") }); + + expect(calls.some((c) => c.method === "POST" && c.url.endsWith("/issues/42/comments"))).toBe(true); + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(true); + const audit = await env.DB.prepare("select outcome from audit_events where event_type = ?").bind("github_app.reopen_reclosed").first<{ outcome: string }>(); + expect(audit?.outcome).toBe("completed"); + }); + + it("REGRESSION: fails CLOSED (denies the re-close) when the reopener-timeline read errors (#2369)", async () => { + // The reopener-timeline lookup errors (network failure) → getLastReopenerLogin catches and returns + // { login: null, coveredAllPages: false } — the same ambiguous shape as an un-covered window. The design + // explicitly fails CLOSED here (deny the close) rather than proceeding, since wrongly re-closing a + // maintainer-authorized PR is worse than leaving a disallowed reopen open for one more tick. + const calls: Array<{ url: string; method: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push({ url, method }); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/collaborators/contributor/permission")) return Response.json({ permission: "read" }); + if (url.endsWith("/collaborators/maintainer/permission")) return Response.json({ permission: "write" }); + if (url.includes("/issues/42/events")) throw new Error("GitHub events API down"); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await repositoriesModule.upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { merge: "auto", request_changes: "auto" } }); + + await processJob(env, { type: "github-webhook", deliveryId: "reopen-timeline-error", eventName: "pull_request", payload: reopenedPayload("contributor") }); + + expect(calls.some((c) => c.method === "POST" && c.url.endsWith("/issues/42/comments"))).toBe(false); + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("github_app.reopen_reclosed").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("denied"); + expect(audit?.detail).toContain("could not confirm"); + }); + + it("REGRESSION: denies with an 'unknown' current-reopener detail when the timeline genuinely has no reopen event at all (#2369)", async () => { + // The window is FULLY covered (a single page, no Link header) but contains no "reopened" event whatsoever — + // getLastReopenerLogin returns { login: null, coveredAllPages: true }, which is NOT the ambiguous case (that + // requires coveredAllPages: false); it lands on the "superseded by a different actor" arm with a null login, + // exercising the `latestReopenerLogin ?? "unknown"` fallback in the audit detail. + const calls: Array<{ url: string; method: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push({ url, method }); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/collaborators/contributor/permission")) return Response.json({ permission: "read" }); + if (url.endsWith("/collaborators/maintainer/permission")) return Response.json({ permission: "write" }); + if (url.includes("/issues/42/events")) return Response.json([{ event: "closed", actor: { login: "maintainer" } }]); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await repositoriesModule.upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { merge: "auto", request_changes: "auto" } }); + + await processJob(env, { type: "github-webhook", deliveryId: "reopen-no-reopen-event", eventName: "pull_request", payload: reopenedPayload("contributor") }); + + expect(calls.some((c) => c.method === "POST" && c.url.endsWith("/issues/42/comments"))).toBe(false); + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("github_app.reopen_reclosed").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("denied"); + expect(audit?.detail).toContain("the current reopener is now unknown, not contributor"); + }); + + it("swallows a recordAuditEvent failure on the superseded-reopener denial path — handler still completes (#2369)", async () => { + const calls: Array<{ url: string; method: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push({ url, method }); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/collaborators/contributor/permission")) return Response.json({ permission: "read" }); + if (url.endsWith("/collaborators/maintainer/permission")) return Response.json({ permission: "write" }); + if (url.includes("/issues/42/events")) { + return Response.json([ + { event: "closed", actor: { login: "maintainer" } }, + { event: "reopened", actor: { login: "contributor" } }, + { event: "closed", actor: { login: "maintainer" } }, + { event: "reopened", actor: { login: "second-maintainer" } }, + ]); + } + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" }); + await repositoriesModule.upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { merge: "auto", request_changes: "auto" } }); + vi.spyOn(repositoriesModule, "recordAuditEvent").mockRejectedValueOnce(new Error("D1 write error")); + + await expect( + processJob(env, { type: "github-webhook", deliveryId: "reopen-superseded-audit-fail", eventName: "pull_request", payload: reopenedPayload("contributor") }), + ).resolves.toBeUndefined(); + expect(calls.some((c) => c.method === "POST" && c.url.endsWith("/issues/42/comments"))).toBe(false); + expect(calls.some((c) => c.method === "PATCH" && c.url.endsWith("/pulls/42"))).toBe(false); + }); + it("swallows a recordAuditEvent failure on the stale-reopen denial path — handler still completes (#2130)", async () => { vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { const url = input.toString(); @@ -10950,12 +11104,16 @@ describe("one-shot reopen prevention", () => { if (url.includes("/issues/42/events")) { // Long timeline (lastPage=12): the contributor padded the events so the real close sits before the // inspected newest window. No "closed" appears in the read pages → null closer + coveredAllPages=false. + // The tail DOES include the contributor's own "reopened" event (the one this whole handler is reacting + // to), so the new live re-check (#2369) still finds `contributor` as the current reopener and does not + // itself block the re-close — only the (deliberately fail-closed) window-evasion path above does. const page = Number(new URL(url).searchParams.get("page") ?? "1"); if (page === 1) { return Response.json([{ event: "labeled", actor: { login: "contributor" } }], { headers: { link: '; rel="last"' }, }); } + if (page === 12) return Response.json([{ event: "labeled", actor: { login: "contributor" } }, { event: "reopened", actor: { login: "contributor" } }]); return Response.json([{ event: "labeled", actor: { login: "contributor" } }]); } if (url.endsWith("/issues/42/comments")) return Response.json({ id: 99 }, { status: 201 }); @@ -10978,7 +11136,7 @@ describe("one-shot reopen prevention", () => { calls.push({ url, method }); if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); if (url.endsWith("/collaborators/contributor/permission")) return Response.json({ permission: "read" }); - if (url.includes("/issues/42/events")) return Response.json([{ event: "closed", actor: { login: "gittensory[bot]" } }]); + if (url.includes("/issues/42/events")) return Response.json([{ event: "closed", actor: { login: "gittensory[bot]" } }, { event: "reopened", actor: { login: "contributor" } }]); if (url.endsWith("/issues/42/comments")) return Response.json({ id: 99 }, { status: 201 }); if (url.endsWith("/pulls/42") && method === "PATCH") return Response.json({ state: "closed" }); return new Response("not found", { status: 404 }); @@ -11026,7 +11184,7 @@ describe("one-shot reopen prevention", () => { const url = input.toString(); if (url.includes("/access_tokens")) return Response.json({ token: "t" }); if (url.endsWith("/collaborators/contributor/permission")) return Response.json({ permission: "read" }); - if (url.includes("/issues/42/events")) return Response.json([{ event: "closed", actor: { login: "maintainer" } }]); + if (url.includes("/issues/42/events")) return Response.json([{ event: "closed", actor: { login: "maintainer" } }, { event: "reopened", actor: { login: "contributor" } }]); if (url.endsWith("/collaborators/maintainer/permission")) return Response.json({ permission: "write" }); // createIssueComment (POST) and closePullRequest (PATCH) both throw → their .catch(() => undefined) bodies run throw new Error("GitHub API unavailable"); From a73f69957a4e52345fe839bb9fa690cde30c5f1a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 14:49:56 -0700 Subject: [PATCH 2/3] test(queue): cover the remaining reopen-reclose and timeline-scan branches The audit-write catch on the new superseded-reopener denial path, the null-latest-reopener 'unknown' fallback in its detail message, and the lastPage<=1 single-page-with-Link-header branch in the generalized timeline scanner were all reachable but untested. --- test/unit/github-pr-actions.test.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/unit/github-pr-actions.test.ts b/test/unit/github-pr-actions.test.ts index 36d0830adf..039e269deb 100644 --- a/test/unit/github-pr-actions.test.ts +++ b/test/unit/github-pr-actions.test.ts @@ -297,6 +297,23 @@ describe("GitHub PR action primitives (#778)", () => { 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: '; rel="last"' }, + }); + } + return new Response("unexpected", { status: 500 }); + }); + await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 121)).resolves.toEqual({ login: "contributor", 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" }); From 909b42f04ab48996fa14127b7201410a2b664db7 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 14:52:41 -0700 Subject: [PATCH 3/3] test(github): cover the no-matching-event fallback for the lastPage<=1 branch --- test/unit/github-pr-actions.test.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/unit/github-pr-actions.test.ts b/test/unit/github-pr-actions.test.ts index 039e269deb..8c281b4650 100644 --- a/test/unit/github-pr-actions.test.ts +++ b/test/unit/github-pr-actions.test.ts @@ -314,6 +314,20 @@ describe("GitHub PR action primitives (#778)", () => { 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: '; 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" });