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..8c281b4650 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,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: '; 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: '; 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: '; 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: '; 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");