diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 628173fe6..a5051037e 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -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?.(); @@ -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 @@ -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 { + 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, diff --git a/test/unit/verdict-stability-wire.test.ts b/test/unit/verdict-stability-wire.test.ts new file mode 100644 index 000000000..f89630270 --- /dev/null +++ b/test/unit/verdict-stability-wire.test.ts @@ -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) { + 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) { + 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) { + 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); + }); +});