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
127 changes: 61 additions & 66 deletions src/queue/review-evasion.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,36 @@ import { applyModerationEscalationForRule } from "../services/agent-action-execu
import { resolveRepositorySettings } from "../settings/repository-settings";
import { claimPrActuationLock, PrActuationLockContendedError, releasePrActuationLock } from "./transient-locks";
import { errorMessage } from "../utils/json";

// #8802: a one-shot-closed PR's ONLY explanation is the guard's comment — a swallowed transient failure
// (secondary rate limit, 5xx) previously left the contributor with a permanently closed PR and no visible
// reason, forever: no sweep backfills a missing explanation on an already-closed PR. Bounded in-handler
// retry (three spaced attempts — a transient window rarely spans them), still fail-safe toward the close
// (an exhausted retry never blocks or reorders the guard), but the residue is LOUD (level:error reaches
// the structured-log Sentry forwarder) and every guard's audit row now records `explanationPosted`, so an
// operator can enumerate any unexplained close directly from the audit trail. Exported for its own tests.
export const CLOSE_EXPLANATION_RETRY_DELAYS_MS: readonly number[] = [0, 400, 1600];

export async function postCloseExplanation(
env: Env,
installationId: number,
repoFullName: string,
prNumber: number,
body: string,
): Promise<boolean> {
for (const delayMs of CLOSE_EXPLANATION_RETRY_DELAYS_MS) {
if (delayMs > 0) await new Promise((resolve) => setTimeout(resolve, delayMs));
const posted = await createIssueComment(env, installationId, repoFullName, prNumber, body)
.then(() => true)
.catch(() => false);
if (posted) return true;
}
console.log(
JSON.stringify({ level: "error", event: "close_explanation_post_failed", repoFullName, prNumber }),
);
return false;
}

import type { GitHubWebhookPayload, JsonValue, PullRequestRecord, RepositorySettings } from "../types";

/** Claims the per-PR actuation lock, runs `action`, and always releases it -- the byte-identical
Expand Down Expand Up @@ -304,13 +334,13 @@ async function closeDraftDodgeAttemptIfBlocked(
if (!gate.proceed) return;

const codes = block.blockerCodes.join(", ");
await createIssueComment(
const explanationPosted = await postCloseExplanation(
env,
installationId,
repoFullName,
pr.number,
`Gate verdict stands for this commit — converting to draft does not reset the review. Re-submit a new PR with the issues addressed${codes ? ` (${codes})` : ""}.`,
).catch(() => undefined);
);
// #2260/#8801: the audit outcome must reflect whether the close actually happened on GitHub, not just
// whether this handler ran. This guard was the ONE sibling in this file still swallowing the close
// error and unconditionally recording "completed" — a transient 403/5xx left the PR open on GitHub
Expand All @@ -327,7 +357,8 @@ async function closeDraftDodgeAttemptIfBlocked(
closeError === null
? `closed draft-dodge attempt by ${pr.authorLogin ?? "unknown"} — prior gate failure on headSha ${pr.headSha} stands`
: `FAILED to close draft-dodge attempt by ${pr.authorLogin ?? "unknown"} — the close API call did not succeed; the PR may still be open`,
metadata: closeError === null ? gateMetadata : { ...gateMetadata, error: errorMessage(closeError) },
// #8801 + #8802 compose: the close's real outcome AND whether the explanation landed, both in one row.
metadata: closeError === null ? { explanationPosted, ...gateMetadata } : { explanationPosted, ...gateMetadata, error: errorMessage(closeError) },
}).catch(() => undefined);
}
}
Expand Down Expand Up @@ -505,13 +536,7 @@ async function recloseDisallowedReopenIfNeeded(
}
// The comment is a courtesy notice; its failure must not mask whether the close itself succeeded (below).
/* v8 ignore next -- fail-safe: a courtesy-comment failure never blocks the handler. */
await createIssueComment(
env,
installationId,
repoFullName,
pr.number,
"This pull request was closed by LoopOver and can't be reopened — reviews are one-shot. Please open a new pull request with the issues resolved.",
).catch(() => undefined);
const explanationPosted = await postCloseExplanation(env, installationId, repoFullName, pr.number, "This pull request was closed by LoopOver and can't be reopened — reviews are one-shot. Please open a new pull request with the issues resolved.");
// #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.
Expand All @@ -529,7 +554,7 @@ async function recloseDisallowedReopenIfNeeded(
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) },
metadata: { explanationPosted, ...(closeError === null ? { deliveryId, repoFullName } : { deliveryId, repoFullName, error: errorMessage(closeError) }) },
}).catch(() => undefined);
return true;
}
Expand Down Expand Up @@ -733,17 +758,11 @@ async function closeReviewEvasionSelfCloseIfReviewed(
// The close succeeded: post the public explanation, apply the configured label, record the strike -- in
// that order, after the enforcement close is confirmed (never before).
const shouldPostSelfCloseComment = settings.reviewEvasionComment ?? true;
// #8802: null = the repo opted out of the explanation comment (never attempted) — distinct in the
// audit row from a delivery failure (false) and a successful post (true).
let explanationPosted: boolean | null = null;
if (shouldPostSelfCloseComment) {
await createIssueComment(
env,
installationId,
repoFullName,
pr.number,
"LoopOver had already started reviewing this pull request — closing it to dodge the one-shot review process is not allowed. Please open a new pull request with the issues addressed.",
).catch(
/* v8 ignore next -- fail-safe: a courtesy-comment failure never blocks the handler. */
() => undefined,
);
explanationPosted = await postCloseExplanation(env, installationId, repoFullName, pr.number, "LoopOver had already started reviewing this pull request — closing it to dodge the one-shot review process is not allowed. Please open a new pull request with the issues addressed.");
}
const label = resolveNullableLabel(settings.reviewEvasionLabel, DEFAULT_REVIEW_EVASION_LABEL);
if (label !== null) {
Expand All @@ -756,7 +775,7 @@ async function closeReviewEvasionSelfCloseIfReviewed(
targetKey,
outcome: "completed",
detail: `re-closed a review-evasion self-close by ${pr.authorLogin} -- active review on headSha ${pr.headSha} was in progress`,
metadata: { deliveryId, repoFullName, headSha: pr.headSha },
metadata: { explanationPosted, deliveryId, repoFullName, headSha: pr.headSha },
}).catch(
/* v8 ignore next -- fail-safe: an audit write failure never blocks the handler. */
() => undefined,
Expand Down Expand Up @@ -893,17 +912,11 @@ async function closeReviewEvasionDraftConversionIfReviewed(
}

const shouldPostDraftConversionComment = settings.reviewEvasionComment ?? true;
// #8802: null = the repo opted out of the explanation comment (never attempted) — distinct in the
// audit row from a delivery failure (false) and a successful post (true).
let explanationPosted: boolean | null = null;
if (shouldPostDraftConversionComment) {
await createIssueComment(
env,
installationId,
repoFullName,
pr.number,
"LoopOver had already started reviewing this pull request — converting it to draft to dodge the one-shot review process is not allowed. Please open a new pull request with the issues addressed.",
).catch(
/* v8 ignore next -- fail-safe: a courtesy-comment failure never blocks the handler. */
() => undefined,
);
explanationPosted = await postCloseExplanation(env, installationId, repoFullName, pr.number, "LoopOver had already started reviewing this pull request — converting it to draft to dodge the one-shot review process is not allowed. Please open a new pull request with the issues addressed.");
}
const label = resolveNullableLabel(settings.reviewEvasionLabel, DEFAULT_REVIEW_EVASION_LABEL);
if (label !== null) {
Expand All @@ -916,7 +929,7 @@ async function closeReviewEvasionDraftConversionIfReviewed(
targetKey,
outcome: "completed",
detail: `closed a review-evasion draft-conversion by ${pr.authorLogin} -- headSha ${pr.headSha} had already been reviewed`,
metadata: { deliveryId, repoFullName, headSha: pr.headSha },
metadata: { explanationPosted, deliveryId, repoFullName, headSha: pr.headSha },
}).catch(
/* v8 ignore next -- fail-safe: an audit write failure never blocks the handler. */
() => undefined,
Expand Down Expand Up @@ -1062,17 +1075,11 @@ async function closeRepeatedDraftCyclingIfDetected(
}

const shouldPostComment = settings.reviewEvasionComment ?? true;
// #8802: null = the repo opted out of the explanation comment (never attempted) — distinct in the
// audit row from a delivery failure (false) and a successful post (true).
let explanationPosted: boolean | null = null;
if (shouldPostComment) {
await createIssueComment(
env,
installationId,
repoFullName,
pr.number,
`LoopOver detected this pull request has been converted to draft ${draftConversionCount} times — repeatedly cycling between ready and draft to solicit review feedback without a real one-shot attempt is not allowed. Please open a new pull request with the issues addressed.`,
).catch(
/* v8 ignore next -- fail-safe: a courtesy-comment failure never blocks the handler. */
() => undefined,
);
explanationPosted = await postCloseExplanation(env, installationId, repoFullName, pr.number, `LoopOver detected this pull request has been converted to draft ${draftConversionCount} times — repeatedly cycling between ready and draft to solicit review feedback without a real one-shot attempt is not allowed. Please open a new pull request with the issues addressed.`);
}
const label = resolveNullableLabel(settings.reviewEvasionLabel, DEFAULT_REVIEW_EVASION_LABEL);
if (label !== null) {
Expand All @@ -1085,7 +1092,7 @@ async function closeRepeatedDraftCyclingIfDetected(
targetKey,
outcome: "completed",
detail: `closed repeated draft-cycling by ${pr.authorLogin} -- conversion #${draftConversionCount} on this PR`,
metadata: { deliveryId, repoFullName, headSha: pr.headSha, draftConversionCount },
metadata: { explanationPosted, deliveryId, repoFullName, headSha: pr.headSha, draftConversionCount },
}).catch(
/* v8 ignore next -- fail-safe: an audit write failure never blocks the handler. */
() => undefined,
Expand Down Expand Up @@ -1219,17 +1226,11 @@ async function closeDraftPrIfPolicyEnabled(
}

const shouldPostComment = settings.reviewEvasionComment ?? true;
// #8802: null = the repo opted out of the explanation comment (never attempted) — distinct in the
// audit row from a delivery failure (false) and a successful post (true).
let explanationPosted: boolean | null = null;
if (shouldPostComment) {
await createIssueComment(
env,
installationId,
repoFullName,
pr.number,
"This repository closes pull requests automatically while they're in draft, to keep CI capacity and review bandwidth available for work that's ready to review. Reopen (or open a fresh pull request) once your changes are ready — LoopOver will pick it up from there.",
).catch(
/* v8 ignore next -- fail-safe: a courtesy-comment failure never blocks the handler. */
() => undefined,
);
explanationPosted = await postCloseExplanation(env, installationId, repoFullName, pr.number, "This repository closes pull requests automatically while they're in draft, to keep CI capacity and review bandwidth available for work that's ready to review. Reopen (or open a fresh pull request) once your changes are ready — LoopOver will pick it up from there.");
}
const label = resolveNullableLabel(settings.reviewEvasionLabel, DEFAULT_REVIEW_EVASION_LABEL);
if (label !== null) {
Expand All @@ -1242,7 +1243,7 @@ async function closeDraftPrIfPolicyEnabled(
targetKey,
outcome: "completed",
detail: `closed draft PR by ${pr.authorLogin} -- draftPrClosePolicy is "close"`,
metadata: gateMetadata,
metadata: { explanationPosted, ...(gateMetadata) },
}).catch(
/* v8 ignore next -- fail-safe: an audit write failure never blocks the handler. */
() => undefined,
Expand Down Expand Up @@ -1350,17 +1351,11 @@ async function closeSynchronizeAmendmentIfPolicyEnabled(
}

const shouldPostComment = settings.reviewEvasionComment ?? true;
// #8802: null = the repo opted out of the explanation comment (never attempted) — distinct in the
// audit row from a delivery failure (false) and a successful post (true).
let explanationPosted: boolean | null = null;
if (shouldPostComment) {
await createIssueComment(
env,
installationId,
repoFullName,
pr.number,
"This repository reviews pull requests one-shot: the PR must be correct as originally opened. Pushing an additional commit closes it automatically instead of restarting review — open a fresh pull request with every fix included.",
).catch(
/* v8 ignore next -- fail-safe: a courtesy-comment failure never blocks the handler. */
() => undefined,
);
explanationPosted = await postCloseExplanation(env, installationId, repoFullName, pr.number, "This repository reviews pull requests one-shot: the PR must be correct as originally opened. Pushing an additional commit closes it automatically instead of restarting review — open a fresh pull request with every fix included.");
}
const label = resolveNullableLabel(settings.reviewEvasionLabel, DEFAULT_REVIEW_EVASION_LABEL);
if (label !== null) {
Expand All @@ -1373,7 +1368,7 @@ async function closeSynchronizeAmendmentIfPolicyEnabled(
targetKey,
outcome: "completed",
detail: `closed PR by ${pr.authorLogin} for pushing an additional commit after opening -- synchronizeClosePolicy is "close"`,
metadata: gateMetadata,
metadata: { explanationPosted, ...(gateMetadata) },
}).catch(
/* v8 ignore next -- fail-safe: an audit write failure never blocks the handler. */
() => undefined,
Expand Down
4 changes: 3 additions & 1 deletion test/unit/queue-lifecycle-guards.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1133,9 +1133,11 @@ describe("converted_to_draft gate-close (draft-dodge prevention)", () => {

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 detail from audit_events where event_type = ?").bind("github_app.draft_dodge_closed").first<{ detail: string }>();
const audit = await env.DB.prepare("select detail, metadata_json from audit_events where event_type = ?").bind("github_app.draft_dodge_closed").first<{ detail: string; metadata_json: string }>();
expect(audit?.detail).toContain("abc123");
expect(audit?.detail).toContain("contributor");
// #8802: every guard's audit row records whether the contributor-facing explanation actually landed.
expect(JSON.parse(audit?.metadata_json ?? "{}").explanationPosted).toBe(true);
});

it("#8801: a FAILED close records outcome 'error' with the failure named — never a false 'completed' (the #2260 contract)", async () => {
Expand Down
56 changes: 56 additions & 0 deletions test/unit/review-evasion-close-explanation.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { CLOSE_EXPLANATION_RETRY_DELAYS_MS, postCloseExplanation } from "../../src/queue/review-evasion";
import { createIssueComment } from "../../src/github/pr-actions";
import { createTestEnv } from "../helpers/d1";

vi.mock("../../src/github/pr-actions", async (importOriginal) => ({
...(await importOriginal<typeof import("../../src/github/pr-actions")>()),
createIssueComment: vi.fn(async () => ({ id: 1 })),
}));

// #8802: the bounded retry around a one-shot close's ONLY explanation comment. Fake timers make the spaced
// retries instant; the delays constant itself is pinned so a future edit can't silently unbound the loop.
describe("postCloseExplanation (#8802)", () => {
beforeEach(() => {
vi.useFakeTimers();
});
afterEach(() => {
vi.useRealTimers();
vi.clearAllMocks();
});

it("posts once and returns true on first success — no retries, no delay", async () => {
const env = createTestEnv();
const result = await postCloseExplanation(env, 123, "owner/repo", 7, "Closed: reason.");
expect(result).toBe(true);
expect(createIssueComment).toHaveBeenCalledTimes(1);
});

it("retries through transient failures and succeeds — a rate-limit window rarely spans three spaced attempts", async () => {
const env = createTestEnv();
vi.mocked(createIssueComment).mockRejectedValueOnce(new Error("secondary rate limit")).mockRejectedValueOnce(new Error("502"));
const pending = postCloseExplanation(env, 123, "owner/repo", 7, "Closed: reason.");
await vi.advanceTimersByTimeAsync(CLOSE_EXPLANATION_RETRY_DELAYS_MS.reduce((a, b) => a + b, 0));
expect(await pending).toBe(true);
expect(createIssueComment).toHaveBeenCalledTimes(3);
});

it("returns false with a LOUD level:error residue log when every attempt fails — the audit row records the unexplained close", async () => {
const env = createTestEnv();
const log = vi.spyOn(console, "log").mockImplementation(() => undefined);
vi.mocked(createIssueComment).mockRejectedValue(new Error("down"));
const pending = postCloseExplanation(env, 123, "owner/repo", 7, "Closed: reason.");
await vi.advanceTimersByTimeAsync(CLOSE_EXPLANATION_RETRY_DELAYS_MS.reduce((a, b) => a + b, 0));
expect(await pending).toBe(false);
expect(createIssueComment).toHaveBeenCalledTimes(CLOSE_EXPLANATION_RETRY_DELAYS_MS.length);
const residue = log.mock.calls.map((c) => c[0]).find((l) => typeof l === "string" && l.includes("close_explanation_post_failed"));
expect(residue).toBeDefined();
expect(JSON.parse(residue as string)).toMatchObject({ level: "error", event: "close_explanation_post_failed", repoFullName: "owner/repo", prNumber: 7 });
log.mockRestore();
});

it("the delay ladder stays bounded and starts immediately (pinned)", () => {
expect(CLOSE_EXPLANATION_RETRY_DELAYS_MS[0]).toBe(0);
expect(CLOSE_EXPLANATION_RETRY_DELAYS_MS.length).toBe(3);
});
});
Loading