diff --git a/src/github/pr-actions.ts b/src/github/pr-actions.ts index 97f51f0de4..aed2a0eaa9 100644 --- a/src/github/pr-actions.ts +++ b/src/github/pr-actions.ts @@ -200,8 +200,13 @@ export async function closePullRequest(env: Env, installationId: number, repoFul /** The last-closer lookup result. `coveredAllPages` is false when the bounded newest-events window did NOT reach * back to page 1 (a very long timeline), so a `login: null` may mean "no close found" OR "a close exists beyond - * 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 }; + * the inspected window". The reopen guard uses this to fail CLOSED rather than allow a window-evasion bypass. + * `errored` distinguishes a genuine read failure (network/auth/rate-limit — we learned NOTHING) from a bounded + * scan that ran to completion and simply found no match in its window (we learned something, just not enough + * to prove full coverage). Both leave `coveredAllPages: false`, but callers that treat "no match in a bounded + * window" as evidence of timeline-padding (rather than proof of nothing) must NOT extend that trust to a scan + * that never actually ran. */ +export type LastCloserResult = { login: string | null; coveredAllPages: boolean; errored: 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 @@ -233,7 +238,7 @@ export async function getLastReopenerLogin(env: Env, installationId: number, rep async function getLastActorForEvent(env: Env, installationId: number, repoFullName: string, issueNumber: number, eventType: string): Promise { try { const { owner, repo } = splitRepo(repoFullName); - return await withInstallationTokenRetry(env, installationId, async (token) => { + const result = await withInstallationTokenRetry(env, installationId, async (token) => { const octokit = makeInstallationOctokit(env, token, "live", githubRateLimitAdmissionKeyForInstallation(installationId)); const requestPage = (page: number) => octokit.request("GET /repos/{owner}/{repo}/issues/{issue_number}/events", { owner, repo, issue_number: issueNumber, per_page: ISSUE_EVENTS_PAGE_SIZE, page }); @@ -275,9 +280,11 @@ async function getLastActorForEvent(env: Env, installationId: number, repoFullNa } return { login: coveredAllPages ? (latestActorInPage(firstEvents, eventType) ?? null) : null, coveredAllPages }; }); + return { ...result, errored: false }; } catch { - // On error we cannot prove we read the whole timeline — report not-covered so the caller decides conservatively. - return { login: null, coveredAllPages: false }; + // On error we learned NOTHING — unlike a bounded scan that ran to completion and found no match, this must + // not be treated as evidence of anything; report it distinctly so the caller can fail closed. (#2369) + return { login: null, coveredAllPages: false, errored: true }; } } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 343c32cd9c..bc04d52d2c 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -8101,25 +8101,29 @@ async function recloseDisallowedReopenIfNeeded( 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; + // A bounded scan that RAN TO COMPLETION and cannot see any reopened event must NOT deny the re-close: otherwise + // a contributor can pad the event timeline until their disallowed reopen falls outside the inspected window. + // Only a fully covered timeline with no reopen, or a visible different latest reopener, proves this webhook was + // superseded. A timeline read that ERRORED is different — it proves nothing — so it must still fail CLOSED, 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. (#2369) const reopenerSuperseded = - reopenerWindowAmbiguous || latestReopenerLogin !== reopener; + latestReopener.errored || + (latestReopener.coveredAllPages + ? latestReopenerLogin !== reopener + : latestReopenerLogin != null && 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` + detail: latestReopener.errored + ? `could not confirm ${reopener} is still the most recent reopener (timeline read failed) — 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 + return true; // handled (decision made); a confirmed superseding reopener still counts as handled } // The comment is a courtesy notice; its failure must not mask whether the close itself succeeded (below). await createIssueComment( diff --git a/test/unit/github-pr-actions.test.ts b/test/unit/github-pr-actions.test.ts index 4b8213ea52..b9cef5a4de 100644 --- a/test/unit/github-pr-actions.test.ts +++ b/test/unit/github-pr-actions.test.ts @@ -188,7 +188,7 @@ describe("GitHub PR action primitives (#778)", () => { return new Response("unexpected", { status: 500 }); }); - await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 17)).resolves.toEqual({ login: "maintainer", coveredAllPages: true }); + await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 17)).resolves.toEqual({ login: "maintainer", coveredAllPages: true, errored: false }); 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); }); @@ -198,7 +198,7 @@ describe("GitHub PR action primitives (#778)", () => { if (input.toString().includes("/access_tokens")) return Response.json({ token: "t" }); throw new Error("network failure"); }); - await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 18)).resolves.toEqual({ login: null, coveredAllPages: false }); + await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 18)).resolves.toEqual({ login: null, coveredAllPages: false, errored: true }); }); it("records null lastCloser when the closed event has a null actor", async () => { @@ -207,7 +207,7 @@ describe("GitHub PR action primitives (#778)", () => { if (input.toString().includes("/issues/19/events")) return Response.json([{ event: "closed", actor: null }]); return new Response("not found", { status: 404 }); }); - await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 19)).resolves.toEqual({ login: null, coveredAllPages: true }); + await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 19)).resolves.toEqual({ login: null, coveredAllPages: true, errored: false }); }); it("reads the newest bounded event pages instead of the oldest prefix", async () => { @@ -230,7 +230,7 @@ describe("GitHub PR action primitives (#778)", () => { } return new Response("unexpected", { status: 500 }); }); - await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 20)).resolves.toEqual({ login: "maintainer", coveredAllPages: false }); + await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 20)).resolves.toEqual({ login: "maintainer", coveredAllPages: false, errored: false }); expect(fetchedPages).toEqual([1, 12, 11]); expect(fetchedPages).not.toContain(2); }); @@ -248,7 +248,7 @@ describe("GitHub PR action primitives (#778)", () => { } return new Response("unexpected", { status: 500 }); }); - await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 21)).resolves.toEqual({ login: null, coveredAllPages: false }); + await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 21)).resolves.toEqual({ login: null, coveredAllPages: false, errored: false }); expect(fetchedPages).toEqual([1, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3]); }); @@ -266,7 +266,7 @@ describe("GitHub PR action primitives (#778)", () => { } return new Response("unexpected", { status: 500 }); }); - await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 22)).resolves.toEqual({ login: "page1-closer", coveredAllPages: true }); + await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 22)).resolves.toEqual({ login: "page1-closer", coveredAllPages: true, errored: false }); }); it("follows rel=next forward when GitHub omits rel=last, finding the later maintainer close (#audit-rel-last)", async () => { @@ -286,7 +286,7 @@ describe("GitHub PR action primitives (#778)", () => { } return new Response("unexpected", { status: 500 }); }); - await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 23)).resolves.toEqual({ login: "maintainer", coveredAllPages: true }); + await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 23)).resolves.toEqual({ login: "maintainer", coveredAllPages: true, errored: false }); expect(fetchedPages).toEqual([1, 2, 3]); }); @@ -304,7 +304,7 @@ describe("GitHub PR action primitives (#778)", () => { } return new Response("unexpected", { status: 500 }); }); - await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 25)).resolves.toEqual({ login: null, coveredAllPages: false }); + await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 25)).resolves.toEqual({ login: null, coveredAllPages: false, errored: false }); expect(fetchedPages).toEqual([1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]); // page 1 + the 10-page budget }); @@ -321,7 +321,7 @@ describe("GitHub PR action primitives (#778)", () => { } return new Response("unexpected", { status: 500 }); }); - await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 26)).resolves.toEqual({ login: null, coveredAllPages: true }); + await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 26)).resolves.toEqual({ login: null, coveredAllPages: true, errored: false }); }); it("returns null when bounded window (firstPageToRead=2) AND page 1 also have no close event (?? null right branch)", async () => { @@ -337,7 +337,7 @@ describe("GitHub PR action primitives (#778)", () => { } return new Response("unexpected", { status: 500 }); }); - await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 24)).resolves.toEqual({ login: null, coveredAllPages: true }); + await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 24)).resolves.toEqual({ login: null, coveredAllPages: true, errored: false }); }); it("getLastReopenerLogin: walks paginated issue events to find the true most recent reopener (#2369)", async () => { @@ -359,7 +359,7 @@ describe("GitHub PR action primitives (#778)", () => { return new Response("unexpected", { status: 500 }); }); - await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 117)).resolves.toEqual({ login: "maintainer", coveredAllPages: true }); + await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 117)).resolves.toEqual({ login: "maintainer", coveredAllPages: true, errored: false }); 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); }); @@ -378,7 +378,7 @@ describe("GitHub PR action primitives (#778)", () => { } return new Response("unexpected", { status: 500 }); }); - await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 121)).resolves.toEqual({ login: "contributor", coveredAllPages: true }); + await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 121)).resolves.toEqual({ login: "contributor", coveredAllPages: true, errored: false }); }); it("getLastReopenerLogin: a single (lastPage<=1) page with no matching event falls back to null (#2369)", async () => { @@ -392,7 +392,7 @@ describe("GitHub PR action primitives (#778)", () => { } return new Response("unexpected", { status: 500 }); }); - await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 122)).resolves.toEqual({ login: null, coveredAllPages: true }); + await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 122)).resolves.toEqual({ login: null, coveredAllPages: true, errored: false }); }); it("getLastReopenerLogin: returns null when the events API throws (catch path, #2369)", async () => { @@ -400,7 +400,7 @@ describe("GitHub PR action primitives (#778)", () => { 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 }); + await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 118)).resolves.toEqual({ login: null, coveredAllPages: false, errored: true }); }); it("getLastReopenerLogin: reads the newest bounded event pages instead of the oldest prefix (#2369)", async () => { @@ -423,7 +423,7 @@ describe("GitHub PR action primitives (#778)", () => { } return new Response("unexpected", { status: 500 }); }); - await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 119)).resolves.toEqual({ login: "maintainer", coveredAllPages: false }); + await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 119)).resolves.toEqual({ login: "maintainer", coveredAllPages: false, errored: false }); expect(fetchedPages).toEqual([1, 12, 11]); expect(fetchedPages).not.toContain(2); }); @@ -434,7 +434,7 @@ describe("GitHub PR action primitives (#778)", () => { 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 }); + await expect(getLastReopenerLogin(envWithKey(), 123, "owner/repo", 120)).resolves.toEqual({ login: null, coveredAllPages: true, errored: false }); }); it("dismisses the bot's own LATEST approve review, ignoring other reviewers and earlier bot reviews (#2254)", async () => { @@ -579,7 +579,7 @@ describe("GitHub PR action primitives (#778)", () => { } return new Response("unexpected", { status: 500 }); }); - await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 27)).resolves.toEqual({ login: "solo-page-closer", coveredAllPages: true }); + await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 27)).resolves.toEqual({ login: "solo-page-closer", coveredAllPages: true, errored: false }); }); it("returns null when rel=last explicitly reports a single page with no close event (?? null right branch)", async () => { @@ -593,7 +593,7 @@ describe("GitHub PR action primitives (#778)", () => { } return new Response("unexpected", { status: 500 }); }); - await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 28)).resolves.toEqual({ login: null, coveredAllPages: true }); + await expect(getLastCloserLogin(envWithKey(), 123, "owner/repo", 28)).resolves.toEqual({ login: null, coveredAllPages: true, errored: false }); }); }); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index a1de5c16f0..3fd6b89ab9 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -11538,11 +11538,45 @@ describe("one-shot reopen prevention", () => { expect(audit?.outcome).toBe("completed"); }); + it("REGRESSION: re-closes when the reopener is hidden beyond the inspected event window", 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.includes("/issues/42/events")) { + const page = Number(new URL(url).searchParams.get("page") ?? "1"); + if (page === 1) { + return Response.json([{ event: "closed", actor: { login: "maintainer" } }, { event: "reopened", actor: { login: "contributor" } }], { + headers: { link: '; rel="last"' }, + }); + } + return Response.json([{ event: "renamed", 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-window-stuffed", 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, detail from audit_events where event_type = ?").bind("github_app.reopen_reclosed").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("completed"); + expect(audit?.detail).toContain("beyond the inspected event window"); + }); + 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. + // { login: null, coveredAllPages: false, errored: true } — DISTINCT from the padded-window case above + // (which has errored: false). 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();