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
92 changes: 66 additions & 26 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4369,6 +4369,27 @@ export async function reReviewStoredPullRequest(
))
)
return false;
// #10222: back off AFTER the readiness gate, but BEFORE onReachedReadiness and the retrigger consumption
// just below. #10204 placed it after the actuation-lock claim, past both -- so a backed-off pass had already
// consumed the user's one-shot "Re-run LoopOver review" marker and charged regatePullRequest's repair budget
// for work it never did. It must not move EARLIER than readiness either: readiness legitimately defers a
// pass (rebase fired, CI still running), and the screenshot-table gate's bounded recapture chain (#10061)
// depends on those deferrals continuing to happen, so a pre-readiness guard silently truncates that retry
// budget. Between the two is the only correct place.
//
// `force` is an operator's manual re-gate and `previewPollAttempt` is a visual poll's own next tick -- both
// are explicit requests for THIS pass, and backoff must never suppress a pass a human or a bounded retry
// chain asked for.
if (
await stableVerdictBackoffEngaged(env, {
repoFullName,
prNumber,
headSha: pr.headSha,
deliveryId,
explicitlyRequested: options.force === true || previewPollAttempt !== undefined,
})
)
return false;
// Fire BEFORE any further (throwable) work below -- this is the one instant readiness is confirmed, so a
// caller learns it even if this call goes on to THROW instead of returning (see the JSDoc above).
options.onReachedReadiness?.();
Expand Down Expand Up @@ -4448,32 +4469,6 @@ export async function reReviewStoredPullRequest(
}).catch(() => undefined);
throw new PrActuationLockContendedError(repoFullName, pr.number, "public-surface-publish");
}
// #10184: a PR whose answer has not changed does not need asking again yet. metagraphed#8886 produced 56
// identical `hold | missing_linked_issue` verdicts on ONE head SHA in 47 minutes; four such PRs made 66% of
// all decision records in a two-hour window, and that window exhausted the installation's REST quota.
//
// Placed AFTER the lock claim so the check itself is nearly free (one cache read on a pass that already
// owns the PR) and BEFORE the refresh, so a backed-off pass spends nothing. Returns rather than throws:
// this is not contention, there is no work to retry, and the state is already published and correct.
//
// Fails OPEN in every uncertain case -- no state, unreadable state, no cache -- see verdict-stability.ts.
// The delay is capped, so a stuck PR is still revisited; it just stops being asked 1.2x/minute.
if (pr.headSha) {
const stability = await readVerdictStability(env.SELFHOST_TRANSIENT_CACHE, verdictStabilityKey(repoFullName, pr.number, pr.headSha));
if (shouldSkipStableVerdict(stability, Date.now())) {
await recordAuditEvent(env, {
eventType: "github_app.review_skipped_stable_verdict",
actor: "loopover",
targetKey: `${repoFullName}#${pr.number}`,
outcome: "completed",
detail: `Verdict unchanged across ${stability?.repeats ?? 0} consecutive evaluations of this commit; backing off instead of re-deriving the same answer.`,
metadata: { deliveryId, repoFullName, repeats: stability?.repeats ?? 0 },
}).catch(() => undefined);
await releasePrActuationLock(env, repoFullName, pr.number, actuationLock.ownerToken).catch(() => undefined);
// false = "did not re-review", the same signal this function's other early bail uses.
return false;
}
}
// #2537 follow-up (gate-flagged): the durable review cache's only invalidation path is markPullRequestReviewsInvalidated
// on a webhook (processors.ts). A "quiet" PR (no new pushes, slop evidence + manifest gate both off, no
// pre-merge check paths) never hits any of the three reasons below, so a DROPPED invalidation write could sit
Expand Down Expand Up @@ -5142,6 +5137,51 @@ async function consumePendingPrPanelRetrigger(
* varies by call (#selfhost-ci-deferral-staleness). A missing cache / cache hiccup degrades to `false` (never
* force-finalize → keeps the safe old defer rather than acting early).
*/
/**
* #10222: should this publish-and-maintain pass back off, because this PR's verdict has not changed (#10184)?
*
* Shared by BOTH sites that run the unit -- `reReviewStoredPullRequest` (sweep / CI completion) and
* `handlePullRequestWebhookEvent` (the `pull_request` webhook). #10204 guarded only the first, which left the
* DOMINANT source unthrottled: over 24h on the Orb, 293 of 344 repeat evaluations carried
* `upstream_state_change` -- `deriveReevaluationReason`'s mapping for a RAW GitHub delivery, i.e. the webhook
* path. A label write the engine itself caused arrives there, not on the sweep.
*
* Call this BEFORE the readiness gate at either site. Readiness fires `onReachedReadiness` (which charges
* regatePullRequest's repair budget) and consumes the one-shot panel-retrigger marker, and a pass that backs
* off after those has silently eaten a user's "Re-run LoopOver review" click with nothing left to re-trigger
* it. Before the lock claim, too: there is no lock to take or release on a pass that is not going to run.
*
* `explicitlyRequested` is the escape hatch and the reason this takes a flag at all -- backoff exists to stop
* the machine asking itself the same question, and must NEVER suppress a pass a human asked for.
*
* Fails OPEN everywhere: no head SHA, no state, unreadable state, no cache, or a throwing read all return
* false and evaluate normally (see verdict-stability.ts). The delay is capped, so even a stuck PR is still
* revisited -- it just stops being asked 1.2x/minute.
*/
async function stableVerdictBackoffEngaged(
env: Env,
args: { repoFullName: string; prNumber: number; headSha: string | null | undefined; deliveryId: string; explicitlyRequested: boolean },
): Promise<boolean> {
const { repoFullName, prNumber, headSha, deliveryId, explicitlyRequested } = args;
// The `!headSha` half is enforced by TSC, not by a test: verdictStabilityKey takes a `string`, so removing
// it does not compile. Mutation testing confirms no RUNTIME test can distinguish its absence -- with no head
// SHA the lookup would miss and shouldSkipStableVerdict would return false anyway -- so it is an early-out
// that saves a pointless cache round-trip, not a safety guard. Recorded here so nobody later mistakes it for
// one (same reasoning as verdict-stability.ts's removed exponent clamp).
if (explicitlyRequested || !headSha) return false;
const stability = await readVerdictStability(env.SELFHOST_TRANSIENT_CACHE, verdictStabilityKey(repoFullName, prNumber, headSha)).catch(() => null);
if (!shouldSkipStableVerdict(stability, Date.now())) return false;
await recordAuditEvent(env, {
eventType: "github_app.review_skipped_stable_verdict",
actor: "loopover",
targetKey: `${repoFullName}#${prNumber}`,
outcome: "completed",
detail: `Verdict unchanged across ${stability?.repeats ?? 0} consecutive evaluations of this commit; backing off instead of re-deriving the same answer.`,
metadata: { deliveryId, repoFullName, repeats: stability?.repeats ?? 0 },
}).catch(() => undefined);
return true;
}

async function ciPendingDeferStuck(
env: Env,
repoFullName: string,
Expand Down
154 changes: 154 additions & 0 deletions test/unit/verdict-stability-wire.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
import { afterEach, describe, expect, it, vi } from "vitest";
import { listAuditEventsByType, upsertInstallation, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertRepositorySettings } from "../../src/db/repositories";
import { reReviewStoredPullRequest } from "../../src/queue/processors";
import { verdictStabilityKey, writeVerdictStability } from "../../src/review/verdict-stability";
import { normalizeRegistryPayload } from "../../src/registry/normalize";
import { persistRegistrySnapshot } from "../../src/registry/sync";
import { asCloudEnv, createTestEnv } from "../helpers/d1";
import { generatePrivateKeyPem } from "../helpers/github-app-key";

// #10222: the wiring #10204 shipped without a test. The backoff logic itself is covered by
// verdict-stability.test.ts; what was never covered is WHERE the guard sits, and #10204 put it after the
// readiness gate -- which fires onReachedReadiness and consumes the one-shot panel-retrigger marker. These
// tests pin the three properties that placement got wrong.

const REPO = "JSONbored/gittensory";
const HEAD = "sha-settled";

async function seed(env: ReturnType<typeof createTestEnv>) {
await persistRegistrySnapshot(
asCloudEnv(env),
normalizeRegistryPayload({ [REPO]: { emission_share: 0.01, issue_discovery_share: 0 } }, { kind: "raw-github", url: "https://example.test" }, "2026-05-23T00:00:00.000Z"),
);
await upsertInstallation(env, {
action: "created",
installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "selected", permissions: {}, events: [] },
});
await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: REPO, private: false, owner: { login: "JSONbored" } }, 123);
await upsertRepositorySettings(env, { repoFullName: REPO, autoLabelEnabled: false, autonomy: { label: "auto" } });
await upsertPullRequestFromGitHub(env, REPO, {
number: 77, title: "settled", state: "open", user: { login: "contributor" }, author_association: "CONTRIBUTOR",
head: { sha: HEAD }, base: { ref: "main" }, labels: [], body: "Closes #1", created_at: "2026-07-31T09:00:00Z",
} as never);
}

/** A verdict that has repeated enough to be settled, evaluated a moment ago -- so the backoff is engaged. */
async function seedSettledVerdict(env: ReturnType<typeof createTestEnv>) {
await writeVerdictStability(env.SELFHOST_TRANSIENT_CACHE, verdictStabilityKey(REPO, 77, HEAD), {
fingerprint: "hold|missing_linked_issue|",
repeats: 8,
lastEvaluatedMs: Date.now(),
});
}

function stubGitHub(onReadiness: () => void) {
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
const url = input.toString();
if (url.includes("/access_tokens")) return Response.json({ token: "t" });
// A files read means the pass got PAST the guard into the publish unit. Readiness itself deliberately
// runs before the guard (#10061), so it is not the probe.
if (url.includes("/files")) onReadiness();
if (url.endsWith("/pulls/77")) return Response.json({ number: 77, title: "settled", state: "open", user: { login: "contributor" }, head: { sha: HEAD }, labels: [], body: "Closes #1", mergeable_state: "clean" });
if (url.includes("/check-runs")) return Response.json({ total_count: 1, check_runs: [{ name: "t", status: "completed", conclusion: "success", app: { slug: "github-actions" } }] });
if (url.includes("/status")) return Response.json({ state: "success", statuses: [] });
if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } });
return Response.json({});
});
}

async function skippedEvents(env: ReturnType<typeof createTestEnv>) {
return listAuditEventsByType(env, "github_app.review_skipped_stable_verdict", "2000-01-01T00:00:00Z");
}

/** The same key processors.ts's pendingPrPanelRetriggerKey builds -- written directly because the marker
* writer is module-private to processors.ts. */
const RETRIGGER_KEY = `pr-panel-retrigger-pending:${REPO.toLowerCase()}#77:${HEAD}`;

describe("verdict-stability backoff wiring (#10222)", () => {
afterEach(() => {
vi.unstubAllGlobals();
});

it("backs off a settled verdict, and records why", async () => {
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
await seed(env);
await seedSettledVerdict(env);
let reviewRan = false;
stubGitHub(() => { reviewRan = true; });

expect(await reReviewStoredPullRequest(env, "d1", 123, REPO, 77)).toBe(false);
expect(reviewRan, "a backed-off pass must not reach the publish unit").toBe(false);
expect(await skippedEvents(env)).toHaveLength(1);
});

it("REGRESSION: an explicit force is NEVER backed off", async () => {
// #10204's guard ignored options.force, so an operator's manual re-gate could be silently suppressed.
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
await seed(env);
await seedSettledVerdict(env);
let reviewRan = false;
stubGitHub(() => { reviewRan = true; });

await reReviewStoredPullRequest(env, "d2", 123, REPO, 77, undefined, { force: true });
expect(reviewRan, "a forced pass must proceed into the publish unit").toBe(true);
expect(await skippedEvents(env)).toHaveLength(0);
});

it("REGRESSION: a visual-preview poll tick is NEVER backed off", async () => {
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
await seed(env);
await seedSettledVerdict(env);
let reviewRan = false;
stubGitHub(() => { reviewRan = true; });

await reReviewStoredPullRequest(env, "d3", 123, REPO, 77, 2);
expect(reviewRan).toBe(true);
expect(await skippedEvents(env)).toHaveLength(0);
});

it("REGRESSION: a backed-off pass does not eat the one-shot panel-retrigger marker (#7626)", async () => {
// The failure #10204's placement caused: readiness consumed the marker, THEN the guard returned, so the
// user's "Re-run LoopOver review" click vanished with nothing left to re-trigger it.
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
await seed(env);
await seedSettledVerdict(env);
await env.SELFHOST_TRANSIENT_CACHE?.set(RETRIGGER_KEY, "1", 3600);
stubGitHub(() => undefined);

expect(await reReviewStoredPullRequest(env, "d4", 123, REPO, 77)).toBe(false);

// The marker must still be there for a later pass to consume.
const stillPending = await env.SELFHOST_TRANSIENT_CACHE?.get(RETRIGGER_KEY);
expect(stillPending, "the retrigger marker must survive a backed-off pass").toBeTruthy();
});

it("never backs off a PR with no head SHA -- there is no key to have settled under", async () => {
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
await seed(env);
await seedSettledVerdict(env);
await upsertPullRequestFromGitHub(env, REPO, {
number: 78, title: "no head", state: "open", user: { login: "contributor" }, author_association: "CONTRIBUTOR",
base: { ref: "main" }, labels: [], body: "Closes #1", created_at: "2026-07-31T09:00:00Z",
} as never);
stubGitHub(() => undefined);

await reReviewStoredPullRequest(env, "d6", 123, REPO, 78);
expect(await skippedEvents(env)).toHaveLength(0);
});

it("does not back off a verdict that has not settled yet", async () => {
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
await seed(env);
await writeVerdictStability(env.SELFHOST_TRANSIENT_CACHE, verdictStabilityKey(REPO, 77, HEAD), {
fingerprint: "hold|missing_linked_issue|",
repeats: 1,
lastEvaluatedMs: Date.now(),
});
let reviewRan = false;
stubGitHub(() => { reviewRan = true; });

await reReviewStoredPullRequest(env, "d5", 123, REPO, 77);
expect(reviewRan).toBe(true);
expect(await skippedEvents(env)).toHaveLength(0);
});
});