Skip to content

fix(queue): reclose ambiguous padded reopens #2470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 2 commits into from
Jul 2, 2026
Merged

fix(queue): reclose ambiguous padded reopens #2470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
daae727
fix(queue): reclose ambiguous padded reopens
JSONbored Jul 2, 2026
5277b93
fix(github): distinguish a timeline read error from a clean bounded scan
JSONbored Jul 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions src/github/pr-actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<LastTimelineActorResult> {
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 });
Expand Down Expand Up @@ -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 };
}
}

Expand Down
22 changes: 13 additions & 9 deletions src/queue/processors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
36 changes: 18 additions & 18 deletions test/unit/github-pr-actions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand All @@ -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 () => {
Expand All @@ -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 () => {
Expand All @@ -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);
});
Expand All @@ -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]);
});

Expand All @@ -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 () => {
Expand All @@ -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]);
});

Expand All @@ -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
});

Expand All @@ -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 () => {
Expand All @@ -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 () => {
Expand All @@ -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);
});
Expand All @@ -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 () => {
Expand All @@ -392,15 +392,15 @@ 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 () => {
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 });
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 () => {
Expand All @@ -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);
});
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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 });
});
});

Expand Down
40 changes: 37 additions & 3 deletions test/unit/queue.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: '<https://api.github.com/repos/owner/repo/issues/42/events?per_page=100&page=12>; 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();
Expand Down
Loading