Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 14 additions & 6 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8016,23 +8016,31 @@ async function maybeRecloseDisallowedReopen(
}).catch(() => undefined);
return true; // handled (decision made); a superseded/ambiguous reopener still counts as handled
}
// The comment is a courtesy notice; its failure must not mask whether the close itself succeeded (below).
await createIssueComment(
env,
installationId,
repoFullName,
pr.number,
"This pull request was closed by Gittensory and can't be reopened — reviews are one-shot. Please open a new pull request with the issues resolved.",
).catch(() => undefined);
await closePullRequest(env, installationId, repoFullName, pr.number).catch(
() => undefined,
);
// #2260: the audit outcome must reflect whether the close actually happened on GitHub, not just whether this
// handler ran. A swallowed 403/404/5xx here previously still recorded outcome:"completed", so an operator
// trusting the audit trail believed a one-shot close was enforced when it may not have been.
const closeError = await closePullRequest(env, installationId, repoFullName, pr.number)
.then(() => null)
.catch((error: unknown) => error);
const originallyClosedBy = closer ?? "Gittensory (close beyond the inspected event window)";
await recordAuditEvent(env, {
eventType: "github_app.reopen_reclosed",
actor: "gittensory",
targetKey: `${repoFullName}#${pr.number}`,
outcome: "completed",
detail: `re-closed a disallowed reopen by ${reopener} (originally closed by ${closer ?? "Gittensory (close beyond the inspected event window)"}) — one-shot; resubmit a new PR`,
metadata: { deliveryId, repoFullName },
outcome: closeError === null ? "completed" : "error",
detail:
closeError === null
? `re-closed a disallowed reopen by ${reopener} (originally closed by ${originallyClosedBy}) — one-shot; resubmit a new PR`
: `FAILED to re-close a disallowed reopen by ${reopener} (originally closed by ${originallyClosedBy}) — the close API call did not succeed; the PR may still be open`,
metadata: closeError === null ? { deliveryId, repoFullName } : { deliveryId, repoFullName, error: errorMessage(closeError) },
}).catch(() => undefined);
return true;
}
Expand Down
40 changes: 39 additions & 1 deletion test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11415,7 +11415,8 @@ describe("one-shot reopen prevention", () => {
expect(calls.some((call) => call.url.endsWith("/collaborators/maintainer/permission"))).toBe(true);
expect(calls.some((call) => call.method === "POST" && call.url.endsWith("/issues/42/comments"))).toBe(true);
expect(calls.some((call) => call.method === "PATCH" && call.url.endsWith("/pulls/42"))).toBe(true);
const audit = await env.DB.prepare("select detail from audit_events where event_type = ?").bind("github_app.reopen_reclosed").first<{ detail: string }>();
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"); // #2260: a successful close is unaffected
expect(audit?.detail).toContain("originally closed by maintainer");
// #review-audit: the early return after a re-close stamps the delivery processed (was left "queued").
const webhookRow = await env.DB.prepare("select status from webhook_events where delivery_id = ?").bind("reopen-write-collab-close").first<{ status: string }>();
Expand Down Expand Up @@ -11678,6 +11679,43 @@ describe("one-shot reopen prevention", () => {
expect(contributorPermissionCalls).toBe(2);
});

it("records outcome:error (not completed) when the reclose PATCH call itself fails (#2260)", 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" });
// "contributor" (the payload's reopener) must be the MOST RECENT "reopened" actor in the timeline, or the
// #2369 live-recheck #3 (reopenerSuperseded) denies before ever reaching the close attempt this test targets.
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 }); // the courtesy comment succeeds
if (url.endsWith("/pulls/42") && method === "PATCH") return new Response("forbidden", { status: 403 }); // the close itself fails
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-close-fails",
eventName: "pull_request",
payload: reopenedPayload("contributor"),
});

expect(calls.some((call) => call.method === "PATCH" && call.url.endsWith("/pulls/42"))).toBe(true); // the close WAS attempted
const audit = await env.DB.prepare("select outcome, detail, metadata_json from audit_events where event_type = ?").bind("github_app.reopen_reclosed").first<{ outcome: string; detail: string; metadata_json: string }>();
expect(audit?.outcome).toBe("error"); // NOT "completed" — the close did not actually succeed
expect(audit?.detail).toContain("FAILED to re-close");
expect(JSON.parse(audit?.metadata_json ?? "{}").error).toBeTruthy();
// The handler still owns the decision (never falls through to normal re-review) even though the API call failed.
const webhookRow = await env.DB.prepare("select status from webhook_events where delivery_id = ?").bind("reopen-close-fails").first<{ status: string }>();
expect(webhookRow?.status).toBe("processed");
});

it("does NOT re-close a disallowed reopen on an OBSERVE-only / un-opted-in repo (autonomy floor, #review-audit)", async () => {
const calls: Array<{ url: string; method: string }> = [];
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
Expand Down