diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 93765641f4..9829d34c61 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -8016,6 +8016,7 @@ 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, @@ -8023,16 +8024,23 @@ async function maybeRecloseDisallowedReopen( 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; } diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index c071f4c54a..61d59a5ef9 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -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 }>(); @@ -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) => {