Skip to content

Commit 9794a5a

Browse files
committed
fix(queue): record the real outcome when a reopen-reclose fails
Both the warning comment and the actual close call in maybeRecloseDisallowedReopen were wrapped in .catch(() => undefined), but the function unconditionally wrote a github_app.reopen_reclosed audit event with outcome:"completed" regardless of whether the close API call actually succeeded. A 403 from reduced permissions, a 404, or a transient 5xx was silently swallowed while the audit ledger kept recording a successful re-close — an operator trusting the audit trail would believe the one-shot close was enforced when the PR may still be open. This mirrors the same audit-fidelity gap already fixed on the draft-dodge path. Capture the close call's settled result and branch the audit outcome on it: "completed" only when closePullRequest actually resolves, "error" otherwise, with the underlying error captured in metadata. The courtesy comment's own failure still never affects this — it's independent of whether the close succeeded.
1 parent c31de60 commit 9794a5a

2 files changed

Lines changed: 51 additions & 7 deletions

File tree

src/queue/processors.ts

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8017,23 +8017,31 @@ async function maybeRecloseDisallowedReopen(
80178017
}).catch(() => undefined);
80188018
return true; // handled (decision made); a superseded/ambiguous reopener still counts as handled
80198019
}
8020+
// The comment is a courtesy notice; its failure must not mask whether the close itself succeeded (below).
80208021
await createIssueComment(
80218022
env,
80228023
installationId,
80238024
repoFullName,
80248025
pr.number,
80258026
"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.",
80268027
).catch(() => undefined);
8027-
await closePullRequest(env, installationId, repoFullName, pr.number).catch(
8028-
() => undefined,
8029-
);
8028+
// #2260: the audit outcome must reflect whether the close actually happened on GitHub, not just whether this
8029+
// handler ran. A swallowed 403/404/5xx here previously still recorded outcome:"completed", so an operator
8030+
// trusting the audit trail believed a one-shot close was enforced when it may not have been.
8031+
const closeError = await closePullRequest(env, installationId, repoFullName, pr.number)
8032+
.then(() => null)
8033+
.catch((error: unknown) => error);
8034+
const originallyClosedBy = closer ?? "Gittensory (close beyond the inspected event window)";
80308035
await recordAuditEvent(env, {
80318036
eventType: "github_app.reopen_reclosed",
80328037
actor: "gittensory",
80338038
targetKey: `${repoFullName}#${pr.number}`,
8034-
outcome: "completed",
8035-
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`,
8036-
metadata: { deliveryId, repoFullName },
8039+
outcome: closeError === null ? "completed" : "error",
8040+
detail:
8041+
closeError === null
8042+
? `re-closed a disallowed reopen by ${reopener} (originally closed by ${originallyClosedBy}) — one-shot; resubmit a new PR`
8043+
: `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`,
8044+
metadata: closeError === null ? { deliveryId, repoFullName } : { deliveryId, repoFullName, error: errorMessage(closeError) },
80378045
}).catch(() => undefined);
80388046
return true;
80398047
}

test/unit/queue.test.ts

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11415,7 +11415,8 @@ describe("one-shot reopen prevention", () => {
1141511415
expect(calls.some((call) => call.url.endsWith("/collaborators/maintainer/permission"))).toBe(true);
1141611416
expect(calls.some((call) => call.method === "POST" && call.url.endsWith("/issues/42/comments"))).toBe(true);
1141711417
expect(calls.some((call) => call.method === "PATCH" && call.url.endsWith("/pulls/42"))).toBe(true);
11418-
const audit = await env.DB.prepare("select detail from audit_events where event_type = ?").bind("github_app.reopen_reclosed").first<{ detail: string }>();
11418+
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 }>();
11419+
expect(audit?.outcome).toBe("completed"); // #2260: a successful close is unaffected
1141911420
expect(audit?.detail).toContain("originally closed by maintainer");
1142011421
// #review-audit: the early return after a re-close stamps the delivery processed (was left "queued").
1142111422
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,41 @@ describe("one-shot reopen prevention", () => {
1167811679
expect(contributorPermissionCalls).toBe(2);
1167911680
});
1168011681

11682+
it("records outcome:error (not completed) when the reclose PATCH call itself fails (#2260)", async () => {
11683+
const calls: Array<{ url: string; method: string }> = [];
11684+
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
11685+
const url = input.toString();
11686+
const method = init?.method ?? "GET";
11687+
calls.push({ url, method });
11688+
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
11689+
if (url.endsWith("/collaborators/contributor/permission")) return Response.json({ permission: "read" });
11690+
if (url.endsWith("/collaborators/maintainer/permission")) return Response.json({ permission: "write" });
11691+
if (url.includes("/issues/42/events")) return Response.json([{ event: "closed", actor: { login: "maintainer" } }]);
11692+
if (url.endsWith("/issues/42/comments")) return Response.json({ id: 99 }, { status: 201 }); // the courtesy comment succeeds
11693+
if (url.endsWith("/pulls/42") && method === "PATCH") return new Response("forbidden", { status: 403 }); // the close itself fails
11694+
return new Response("not found", { status: 404 });
11695+
});
11696+
11697+
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem(), GITHUB_APP_SLUG: "gittensory" });
11698+
await repositoriesModule.upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { merge: "auto", request_changes: "auto" } });
11699+
11700+
await processJob(env, {
11701+
type: "github-webhook",
11702+
deliveryId: "reopen-close-fails",
11703+
eventName: "pull_request",
11704+
payload: reopenedPayload("contributor"),
11705+
});
11706+
11707+
expect(calls.some((call) => call.method === "PATCH" && call.url.endsWith("/pulls/42"))).toBe(true); // the close WAS attempted
11708+
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 }>();
11709+
expect(audit?.outcome).toBe("error"); // NOT "completed" — the close did not actually succeed
11710+
expect(audit?.detail).toContain("FAILED to re-close");
11711+
expect(JSON.parse(audit?.metadata_json ?? "{}").error).toBeTruthy();
11712+
// The handler still owns the decision (never falls through to normal re-review) even though the API call failed.
11713+
const webhookRow = await env.DB.prepare("select status from webhook_events where delivery_id = ?").bind("reopen-close-fails").first<{ status: string }>();
11714+
expect(webhookRow?.status).toBe("processed");
11715+
});
11716+
1168111717
it("does NOT re-close a disallowed reopen on an OBSERVE-only / un-opted-in repo (autonomy floor, #review-audit)", async () => {
1168211718
const calls: Array<{ url: string; method: string }> = [];
1168311719
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {

0 commit comments

Comments
 (0)