From 5536254409de4fd4fa2d2f9dff0dce7e8a3d0e40 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 31 Jul 2026 04:29:39 -0700 Subject: [PATCH 1/2] feat(review): verdict-stability backoff logic for PRs whose answer never changes metagraphed#8886 was evaluated 56 times on ONE unchanged commit in 47 minutes -- about 1.2 per minute -- producing the identical `hold | missing_linked_issue` every time. It is CONFLICTING so it cannot merge, held so it does not close, and its linked issue was closed by a merged rival (#10168), so the hold never clears. Four such PRs produced 66% of all decision records in a two-hour window, and that window exhausted the installation's GitHub REST quota. The webhook coalescer (#10127) cannot help: this is not a burst but a steady drip of legitimately distinct deliveries arriving long after any window. The missing control is a different kind -- not "collapse events that arrive together" but "stop asking a question whose answer has not changed". Sameness is the DECISION fields (action, reason_code, hold_cause) and deliberately NOT record_digest: #8886 has 56 distinct digests for its 56 identical verdicts, because the digest commits to per-evaluation data. Using it would make every repeat look novel, which is how this went unnoticed. Safety properties, each pinned by a test: the delay is capped so a stuck PR is still revisited; state is keyed on the head SHA so a new commit resets structurally rather than by a rule someone must remember; a changed verdict resets the count; and every uncertain case -- no state, malformed state, no cache, a throwing cache -- fails OPEN. A backoff that engaged on missing information would silently stop reviewing PRs, which is worse than the churn. Mutation testing removed a clamp on the exponent that looked like defence in depth: `2 ** 1000` is Infinity and `Math.min(Infinity, cap)` is the cap, so no test could distinguish its presence. The cap is the only thing bounding this and it is tested directly; a second guard nobody can verify is a claim, not a safeguard. The module is registered in STAGED_AHEAD_OF_CONSUMERS. Wiring the skip is deliberately a separate change: it suppresses re-evaluation in processors.ts's hottest path, and getting it wrong stops reviewing PRs rather than merely wasting work. That belongs in a focused diff with its own review, not appended to this one. Refs #10184 --- scripts/check-dead-source-files.ts | 4 + src/review/verdict-stability.ts | 148 +++++++++++++++++++++ test/unit/verdict-stability.test.ts | 193 ++++++++++++++++++++++++++++ 3 files changed, 345 insertions(+) create mode 100644 src/review/verdict-stability.ts create mode 100644 test/unit/verdict-stability.test.ts diff --git a/scripts/check-dead-source-files.ts b/scripts/check-dead-source-files.ts index dd0ac2fc8..0167abba8 100644 --- a/scripts/check-dead-source-files.ts +++ b/scripts/check-dead-source-files.ts @@ -66,6 +66,10 @@ const STAGED_AHEAD_OF_CONSUMERS: ReadonlyMap = new Map([ "src/chat/grounding-registry.ts", "The maintainer chat grounding-tool registry (#9189, spec #9187). Deliberately ahead of its route: the six tools are defined over an INJECTED GroundingServices surface, and binding that surface to the real readers (queueSnapshotFromBinding, verifyDecisionLedger, resolveProofPage, loadRepoFocusManifest, getRepository) plus wiring the protected route and its authz is the second half of #9189. The registry's own invariants -- read-only by construction, allowlisted response shapes, determinism, per-tool budgets -- are fully covered by test/unit/grounding-registry.test.ts today.", ], + [ + "src/review/verdict-stability.ts", + "Verdict-stability backoff (#10184). Deliberately ahead of its call sites: the decision logic and its persistence are complete and fully covered by test/unit/verdict-stability.test.ts, but WIRING the skip belongs in a focused change with its own review — it suppresses re-evaluation in processors.ts's hottest path, and getting it wrong stops reviewing PRs rather than merely wasting work. #10184 wires it: record on the persistDecisionRecord path (the single ledger write every verdict passes through) and consult it before the publish-and-maintain pass.", + ], [ "src/review/benchmark-eval-records.ts", "The benchmark score-record emitter + leaderboard derivation (#9265). Deliberately ahead of its serving route: #9216's endpoint sub-issue wires GET /v1/public/eval-scores to emit benchmark_run records, and #9264 supplies the attestation envelopes its `attested` tier needs. Fully covered by test/unit/benchmark-eval-records.test.ts today.", diff --git a/src/review/verdict-stability.ts b/src/review/verdict-stability.ts new file mode 100644 index 000000000..0b44990d5 --- /dev/null +++ b/src/review/verdict-stability.ts @@ -0,0 +1,148 @@ +// Backoff for a pull request whose verdict never changes (#10184). +// +// metagraphed#8886 was evaluated 56 times on ONE unchanged commit in 47 minutes -- about 1.2 per minute -- +// producing the identical `hold | missing_linked_issue` every time. It is CONFLICTING so it cannot merge, +// held so it does not close, and its linked issue was closed by a merged rival (#10168), so the hold never +// clears. It is in a state it cannot leave, and nothing throttled re-entry. +// +// Four such PRs produced 66% of all decision records in a two-hour window. That is not a burst: it is a +// steady drip of legitimately distinct deliveries (CI completions, label writes, sibling activity) arriving +// long after any coalescing window. The webhook coalescer (#10127) collapses SIMULTANEOUS events and cannot +// help here. The missing control is a different kind: +// +// not "collapse events that arrive together" +// but "stop asking a question whose answer has not changed" +// +// ── WHAT COUNTS AS "THE SAME ANSWER" ────────────────────────────────────────────────────────────────────── +// The decision fields -- action, reason_code, hold_cause -- and nothing else. Deliberately NOT +// `record_digest`: #8886 has 56 DISTINCT digests for its 56 identical verdicts, because the digest commits to +// per-evaluation data. Using it would make every repeat look novel, which is exactly how this went unnoticed. +// +// ── WHY IT CANNOT STRAND A PR ───────────────────────────────────────────────────────────────────────────── +// The delay is capped, so a stuck PR is still revisited -- just at the sweep's cadence rather than 1.2x/min. +// A new head SHA resets it outright (a new commit is a genuinely new question), and so does any change in the +// verdict itself. Backoff that could grow without bound would trade a spend bug for a liveness bug. + +/** The decision fields that make two verdicts "the same answer". Kept as an explicit shape rather than a + * free-form string so a caller cannot accidentally fingerprint on something incidental. */ +export type VerdictFacts = { + action: string; + reasonCode: string; + /** #9991's recorded hold cause. Null/absent for a verdict that is not a hold. */ + holdCause?: string | null | undefined; +}; + +/** Stability state carried between evaluations for one (repo, pull, head SHA). */ +export type VerdictStabilityState = { + fingerprint: string; + /** How many CONSECUTIVE evaluations produced this same fingerprint, including the first. */ + repeats: number; + /** When the most recent evaluation ran. */ + lastEvaluatedMs: number; +}; + +/** First delay applied once a verdict has repeated enough to be considered stable. */ +export const VERDICT_BACKOFF_BASE_MS = 60_000; + +/** Ceiling on the delay. Chosen so a stuck PR is still revisited on roughly the sweep's own cadence -- the + * point is to stop the 1.2/min drip, not to stop looking. */ +export const VERDICT_BACKOFF_CAP_MS = 15 * 60_000; + +/** Repeats tolerated before backoff engages at all. Two identical verdicts can be an ordinary race (a webhook + * and the sweep landing together); a third means the answer is genuinely settled. Below this the behaviour + * is byte-identical to having no backoff. */ +export const VERDICT_BACKOFF_MIN_REPEATS = 3; + +/** PURE. The fingerprint two evaluations must share to count as the same answer. */ +export function verdictFingerprint(facts: VerdictFacts): string { + return [facts.action, facts.reasonCode, facts.holdCause ?? ""].join("|"); +} + +/** + * PURE. How long to wait before re-evaluating, given how many times this answer has repeated. + * + * Exponential from the base, capped. Returns 0 below the threshold so the common case -- a PR whose verdict + * is still moving -- is completely unaffected. + */ +export function verdictBackoffDelayMs(repeats: number): number { + if (repeats < VERDICT_BACKOFF_MIN_REPEATS) return 0; + const doublings = repeats - VERDICT_BACKOFF_MIN_REPEATS; + // No clamp on the exponent: an earlier version had one, and mutation testing showed removing it changed + // nothing, because `2 ** 1000` is Infinity and `Math.min(Infinity, cap)` is the cap. The cap is the single + // thing keeping this bounded, and it is directly tested -- a second guard that no test can distinguish is + // not defence in depth, it is a claim nobody is checking. + return Math.min(VERDICT_BACKOFF_BASE_MS * 2 ** doublings, VERDICT_BACKOFF_CAP_MS); +} + +/** + * PURE. Fold a fresh verdict into the prior state. + * + * A DIFFERENT fingerprint resets the count to 1 -- the answer moved, so whatever we had learned about its + * stability is void. Callers reset on a new head SHA by keying the state on the head SHA, so a new commit + * never sees the old state at all. + */ +export function recordVerdict(prior: VerdictStabilityState | null, facts: VerdictFacts, nowMs: number): VerdictStabilityState { + const fingerprint = verdictFingerprint(facts); + const repeats = prior !== null && prior.fingerprint === fingerprint ? prior.repeats + 1 : 1; + return { fingerprint, repeats, lastEvaluatedMs: nowMs }; +} + +/** + * PURE. Should this evaluation be skipped because the answer is settled and the backoff has not elapsed? + * + * Fails OPEN in every uncertain case -- no prior state, an unreadable state, or a delay of zero all evaluate + * normally. A backoff that engaged on missing information would silently stop reviewing PRs, which is far + * worse than the churn it is trying to prevent. + */ +export function shouldSkipStableVerdict(prior: VerdictStabilityState | null, nowMs: number): boolean { + if (prior === null) return false; + const delay = verdictBackoffDelayMs(prior.repeats); + if (delay <= 0) return false; + return nowMs - prior.lastEvaluatedMs < delay; +} + +// ── PERSISTENCE ─────────────────────────────────────────────────────────────────────────────────────────── +// Same transient-cache idiom as ciPendingDeferStuck (processors.ts): keyed on repo#pr:headSha, so a NEW +// COMMIT never sees the old state -- the reset on a new head is structural rather than a rule someone has to +// remember. Best effort throughout: a cache miss or error yields null, which fails OPEN at every caller. + +/** Cache key. Includes the head SHA so a new commit starts clean. */ +export function verdictStabilityKey(repoFullName: string, prNumber: number, headSha: string): string { + return `verdict-stability:${repoFullName.toLowerCase()}#${prNumber}:${headSha}`; +} + +/** How long a stability record outlives its last write. Comfortably longer than the cap so a slow-drip PR + * keeps accumulating repeats, short enough that an abandoned head SHA does not linger. */ +const VERDICT_STABILITY_TTL_SECONDS = 6 * 3600; + +type TransientCache = { + get(key: string): Promise; + set(key: string, value: string, ttlSeconds: number): Promise; +}; + +/** Read prior stability state. Null on absence, malformed JSON, or any error -- every one of which must let + * the evaluation proceed rather than suppress it. */ +export async function readVerdictStability(cache: TransientCache | undefined, key: string): Promise { + if (!cache) return null; + try { + const raw = await cache.get(key); + if (!raw) return null; + const parsed = JSON.parse(raw) as Partial; + if (typeof parsed.fingerprint !== "string" || typeof parsed.repeats !== "number" || typeof parsed.lastEvaluatedMs !== "number") return null; + if (!Number.isFinite(parsed.repeats) || !Number.isFinite(parsed.lastEvaluatedMs)) return null; + return { fingerprint: parsed.fingerprint, repeats: parsed.repeats, lastEvaluatedMs: parsed.lastEvaluatedMs }; + } catch { + return null; + } +} + +/** Persist stability state. Best effort: a failed write means the next evaluation sees no prior state and + * proceeds normally, which is the safe direction. */ +export async function writeVerdictStability(cache: TransientCache | undefined, key: string, state: VerdictStabilityState): Promise { + if (!cache) return; + try { + await cache.set(key, JSON.stringify(state), VERDICT_STABILITY_TTL_SECONDS); + } catch { + // Telemetry-grade write; never fail the pass carrying it. + } +} diff --git a/test/unit/verdict-stability.test.ts b/test/unit/verdict-stability.test.ts new file mode 100644 index 000000000..40bce1b67 --- /dev/null +++ b/test/unit/verdict-stability.test.ts @@ -0,0 +1,193 @@ +import { describe, expect, it } from "vitest"; + +import { + recordVerdict, + shouldSkipStableVerdict, + verdictBackoffDelayMs, + verdictFingerprint, + VERDICT_BACKOFF_BASE_MS, + VERDICT_BACKOFF_CAP_MS, + VERDICT_BACKOFF_MIN_REPEATS, + verdictStabilityKey, + readVerdictStability, + writeVerdictStability, + type VerdictStabilityState, +} from "../../src/review/verdict-stability"; + +// #10184: metagraphed#8886 was evaluated 56 times on ONE unchanged commit in 47 minutes, producing the +// identical `hold | missing_linked_issue` every time. Four such PRs made 66% of all decision records in a +// two-hour window, and the same window exhausted the installation's GitHub REST quota. +// +// Every test below is about a way this control could be worse than the problem: stranding a PR it should +// still revisit, engaging on a verdict that is actually moving, or failing closed on missing state. + +const HOLD = { action: "hold", reasonCode: "missing_linked_issue", holdCause: null }; + +describe("verdictFingerprint", () => { + it("is the DECISION fields and nothing else", () => { + expect(verdictFingerprint(HOLD)).toBe("hold|missing_linked_issue|"); + expect(verdictFingerprint({ action: "hold", reasonCode: "success", holdCause: "guardrailHit" })).toBe("hold|success|guardrailHit"); + }); + + it("separates verdicts that differ only by hold cause", () => { + // #9991 exists precisely because "hold / success" conflated seven mechanisms. Two different causes are + // two different answers, and treating them as one would back off through a genuine change. + const a = verdictFingerprint({ action: "hold", reasonCode: "success", holdCause: "guardrailHit" }); + const b = verdictFingerprint({ action: "hold", reasonCode: "success", holdCause: "screenshotEvidenceHold" }); + expect(a).not.toBe(b); + }); + + it("treats null and absent hold cause identically", () => { + expect(verdictFingerprint({ action: "merge", reasonCode: "success" })).toBe( + verdictFingerprint({ action: "merge", reasonCode: "success", holdCause: null }), + ); + }); +}); + +describe("verdictBackoffDelayMs", () => { + it("is ZERO below the threshold — the common path is untouched", () => { + // A PR whose verdict is still moving must behave exactly as it did before this existed. + for (let n = 0; n < VERDICT_BACKOFF_MIN_REPEATS; n += 1) { + expect(verdictBackoffDelayMs(n), `repeats=${n}`).toBe(0); + } + }); + + it("engages at the threshold and grows exponentially", () => { + expect(verdictBackoffDelayMs(VERDICT_BACKOFF_MIN_REPEATS)).toBe(VERDICT_BACKOFF_BASE_MS); + expect(verdictBackoffDelayMs(VERDICT_BACKOFF_MIN_REPEATS + 1)).toBe(VERDICT_BACKOFF_BASE_MS * 2); + expect(verdictBackoffDelayMs(VERDICT_BACKOFF_MIN_REPEATS + 2)).toBe(VERDICT_BACKOFF_BASE_MS * 4); + }); + + it("INVARIANT: never exceeds the cap, however long a PR stays stuck", () => { + // The liveness property. An uncapped backoff would turn a spend bug into a PR nobody ever looks at again. + for (const repeats of [10, 50, 500, 10_000]) { + expect(verdictBackoffDelayMs(repeats), `repeats=${repeats}`).toBeLessThanOrEqual(VERDICT_BACKOFF_CAP_MS); + } + }); + + it("stays finite at an absurd repeat count — the cap, not an exponent clamp, is what bounds this", () => { + // `2 ** 1000` is Infinity and `Math.min(Infinity, cap)` is the cap, so the cap alone is sufficient. An + // earlier version also clamped the exponent; mutation testing showed no test could tell the difference, + // so the clamp was removed rather than kept as an unverifiable second guard. + expect(Number.isFinite(verdictBackoffDelayMs(1000))).toBe(true); + expect(verdictBackoffDelayMs(1000)).toBe(VERDICT_BACKOFF_CAP_MS); + }); + + it("is monotonic — more repeats never means a shorter wait", () => { + let prev = -1; + for (let n = 0; n <= 20; n += 1) { + const d = verdictBackoffDelayMs(n); + expect(d, `repeats=${n}`).toBeGreaterThanOrEqual(prev); + prev = d; + } + }); +}); + +describe("recordVerdict", () => { + it("counts consecutive identical verdicts", () => { + let state: VerdictStabilityState | null = null; + for (let i = 1; i <= 5; i += 1) state = recordVerdict(state, HOLD, i * 1000); + expect(state?.repeats).toBe(5); + }); + + it("REGRESSION: a CHANGED verdict resets the count to 1", () => { + // The answer moved, so everything learned about its stability is void. Without this, a PR that flipped + // between two states would keep backing off as though settled. + let state = recordVerdict(null, HOLD, 1000); + state = recordVerdict(state, HOLD, 2000); + state = recordVerdict(state, { action: "merge", reasonCode: "success" }, 3000); + expect(state.repeats).toBe(1); + expect(state.fingerprint).toBe("merge|success|"); + }); + + it("resets when only the hold cause changes", () => { + let state = recordVerdict(null, { action: "hold", reasonCode: "success", holdCause: "guardrailHit" }, 1000); + state = recordVerdict(state, { action: "hold", reasonCode: "success", holdCause: "advisoryCheckHold" }, 2000); + expect(state.repeats).toBe(1); + }); +}); + +describe("shouldSkipStableVerdict", () => { + const settled = (nowMs: number): VerdictStabilityState => ({ + fingerprint: verdictFingerprint(HOLD), + repeats: VERDICT_BACKOFF_MIN_REPEATS, + lastEvaluatedMs: nowMs, + }); + + it("skips while the backoff window is open", () => { + expect(shouldSkipStableVerdict(settled(0), VERDICT_BACKOFF_BASE_MS - 1)).toBe(true); + }); + + it("evaluates again once the window has elapsed", () => { + expect(shouldSkipStableVerdict(settled(0), VERDICT_BACKOFF_BASE_MS)).toBe(false); + expect(shouldSkipStableVerdict(settled(0), VERDICT_BACKOFF_BASE_MS + 1)).toBe(false); + }); + + it("FAILS OPEN with no prior state", () => { + // Missing state must never suppress a review. Failing closed here would silently stop reviewing PRs -- + // far worse than the churn this prevents. + expect(shouldSkipStableVerdict(null, Date.now())).toBe(false); + }); + + it("FAILS OPEN below the repeat threshold, however recent the last evaluation", () => { + const fresh: VerdictStabilityState = { fingerprint: verdictFingerprint(HOLD), repeats: 1, lastEvaluatedMs: 1000 }; + expect(shouldSkipStableVerdict(fresh, 1001)).toBe(false); + }); + + it("INVARIANT: a stuck PR is always revisited within the cap", () => { + // The liveness guarantee stated as a property rather than trusted from the delay function. + const veryStuck: VerdictStabilityState = { fingerprint: verdictFingerprint(HOLD), repeats: 9999, lastEvaluatedMs: 0 }; + expect(shouldSkipStableVerdict(veryStuck, VERDICT_BACKOFF_CAP_MS)).toBe(false); + }); +}); + +describe("persistence (#10184)", () => { + const makeCache = () => { + const store = new Map(); + return { + store, + get: async (k: string) => store.get(k) ?? null, + set: async (k: string, v: string) => void store.set(k, v), + }; + }; + + it("keys on the head SHA, so a new commit starts clean", () => { + // The reset-on-new-commit rule is structural rather than something a caller must remember. + const a = verdictStabilityKey("Acme/Widgets", 7, "aaa"); + const b = verdictStabilityKey("acme/widgets", 7, "bbb"); + expect(a).not.toBe(b); + expect(a).toBe(verdictStabilityKey("acme/widgets", 7, "aaa")); // repo case-insensitive + }); + + it("round-trips state", async () => { + const cache = makeCache(); + const key = verdictStabilityKey("acme/widgets", 7, "aaa"); + const state = recordVerdict(null, HOLD, 1000); + await writeVerdictStability(cache, key, state); + expect(await readVerdictStability(cache, key)).toEqual(state); + }); + + it("FAILS OPEN on absent, malformed, or mistyped state", async () => { + // Every one of these must let the evaluation proceed. Returning a bogus state instead would suppress a + // review on corrupt data -- the one outcome worse than the churn this prevents. + const cache = makeCache(); + expect(await readVerdictStability(cache, "missing")).toBeNull(); + cache.store.set("bad-json", "{not json"); + expect(await readVerdictStability(cache, "bad-json")).toBeNull(); + cache.store.set("wrong-shape", JSON.stringify({ fingerprint: 1, repeats: "x" })); + expect(await readVerdictStability(cache, "wrong-shape")).toBeNull(); + cache.store.set("nan", JSON.stringify({ fingerprint: "f", repeats: Number.NaN, lastEvaluatedMs: 1 })); + expect(await readVerdictStability(cache, "nan")).toBeNull(); + }); + + it("FAILS OPEN with no cache configured at all", async () => { + expect(await readVerdictStability(undefined, "k")).toBeNull(); + await expect(writeVerdictStability(undefined, "k", recordVerdict(null, HOLD, 1))).resolves.toBeUndefined(); + }); + + it("a throwing cache never propagates — telemetry must not fail the pass carrying it", async () => { + const broken = { get: async () => { throw new Error("boom"); }, set: async () => { throw new Error("boom"); } }; + expect(await readVerdictStability(broken, "k")).toBeNull(); + await expect(writeVerdictStability(broken, "k", recordVerdict(null, HOLD, 1))).resolves.toBeUndefined(); + }); +}); From e5be077cc38db323531c693032f1613bd6856927 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 31 Jul 2026 04:49:28 -0700 Subject: [PATCH 2/2] feat(queue): wire verdict-stability backoff into the re-review path Completes the backoff by connecting it, rather than leaving the module staged ahead of its consumers. Wiring it immediately found a design flaw its own 21 tests could not: "same verdict" is NOT "nothing happened". RECORD at the persistDecisionRecord call site -- the single ledger write every verdict passes through, so no path can bypass it, the same reasoning that function uses for its own reevaluation check. Keyed on the head SHA, so a new commit starts clean structurally rather than by a rule someone must remember. SKIP after the actuation-lock claim and before the refresh: the check is then one cache read on a pass that already owns the PR, and a backed-off pass spends nothing. It composes with #10174's reorder. It RETURNS rather than throws -- this is not contention, there is no work to retry, and the published state is already correct -- using `false`, the same "did not re-review" signal this function's other early bail uses. No caller branches on the result, so it cannot drive a retry loop. The lock is released explicitly first; wiring is what surfaced that leak. HOLDS ONLY, and this is the flaw wiring exposed. The force-fresh-rebase test (#9497/#2552) runs three deliberate identical passes to spend the 24h update-branch cap, and the first version swallowed the third: those passes take real actions -- update-branch, cap accounting -- while producing an unchanged verdict. Throttling them suppresses progress, not waste. A `hold` is the one action meaning "the gate declined to act", so repeating it genuinely produces nothing; every other action keeps today's behaviour exactly. That also covers the motivating case precisely -- #8886 is 56 identical HOLDS on one commit. Removed from STAGED_AHEAD_OF_CONSUMERS: it has consumers now. Closes #10184 --- scripts/check-dead-source-files.ts | 4 --- src/queue/processors.ts | 40 +++++++++++++++++++++++++++++ src/review/verdict-stability.ts | 10 ++++++++ test/unit/verdict-stability.test.ts | 18 +++++++++++++ 4 files changed, 68 insertions(+), 4 deletions(-) diff --git a/scripts/check-dead-source-files.ts b/scripts/check-dead-source-files.ts index 0167abba8..dd0ac2fc8 100644 --- a/scripts/check-dead-source-files.ts +++ b/scripts/check-dead-source-files.ts @@ -66,10 +66,6 @@ const STAGED_AHEAD_OF_CONSUMERS: ReadonlyMap = new Map([ "src/chat/grounding-registry.ts", "The maintainer chat grounding-tool registry (#9189, spec #9187). Deliberately ahead of its route: the six tools are defined over an INJECTED GroundingServices surface, and binding that surface to the real readers (queueSnapshotFromBinding, verifyDecisionLedger, resolveProofPage, loadRepoFocusManifest, getRepository) plus wiring the protected route and its authz is the second half of #9189. The registry's own invariants -- read-only by construction, allowlisted response shapes, determinism, per-tool budgets -- are fully covered by test/unit/grounding-registry.test.ts today.", ], - [ - "src/review/verdict-stability.ts", - "Verdict-stability backoff (#10184). Deliberately ahead of its call sites: the decision logic and its persistence are complete and fully covered by test/unit/verdict-stability.test.ts, but WIRING the skip belongs in a focused change with its own review — it suppresses re-evaluation in processors.ts's hottest path, and getting it wrong stops reviewing PRs rather than merely wasting work. #10184 wires it: record on the persistDecisionRecord path (the single ledger write every verdict passes through) and consult it before the publish-and-maintain pass.", - ], [ "src/review/benchmark-eval-records.ts", "The benchmark score-record emitter + leaderboard derivation (#9265). Deliberately ahead of its serving route: #9216's endpoint sub-issue wires GET /v1/public/eval-scores to emit benchmark_run records, and #9264 supplies the attestation envelopes its `attested` tier needs. Fully covered by test/unit/benchmark-eval-records.test.ts today.", diff --git a/src/queue/processors.ts b/src/queue/processors.ts index cd4c56af5..701ddfa91 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -97,6 +97,7 @@ import { upsertRepositoryFromGitHub, getLatestAdvisoryForPullRequest, } from "../db/repositories"; +import { readVerdictStability, recordVerdict, shouldSkipStableVerdict, verdictStabilityKey, writeVerdictStability } from "../review/verdict-stability"; import { withLinkedIssueMaintainerExemption, type LinkedIssueExemptionAuthor } from "../settings/linked-issue-exemption"; import { resolveConfiguredRepoCandidates } from "../review/configured-repo-set"; import { renameRepositoryIdentity } from "../db/repo-identity-rename"; @@ -3968,6 +3969,19 @@ async function runAgentMaintenancePlanAndExecute( { reason: deriveReevaluationReason(deliveryId) }, holdCause, ); + // #10184: fold this verdict into the PR's stability state, at the one place every verdict passes through + // so no caller can bypass it -- the same reasoning persistDecisionRecord itself uses for its + // reevaluation check. Keyed on the head SHA, so a new commit starts clean without an explicit reset. + // Best effort: a failed write means the next pass sees no prior state and evaluates normally. + if (record.headSha) { + const stabilityKey = verdictStabilityKey(repoFullName, pr.number, record.headSha); + const priorStability = await readVerdictStability(env.SELFHOST_TRANSIENT_CACHE, stabilityKey); + await writeVerdictStability( + env.SELFHOST_TRANSIENT_CACHE, + stabilityKey, + recordVerdict(priorStability, { action: record.action, reasonCode: record.reasonCode, holdCause }, Date.now()), + ); + } // #8838: persist the evaluation's own exact inputs beside the record (PRIVATE sibling, migration 0182) // so the replay harness can re-derive this decision bit-exactly. Best-effort, like the record itself; // the no-replay no-op (synthetic content-lane/bridge evaluations) lives inside the helper. Keyed to the @@ -4428,6 +4442,32 @@ 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 diff --git a/src/review/verdict-stability.ts b/src/review/verdict-stability.ts index 0b44990d5..e3bd546cc 100644 --- a/src/review/verdict-stability.ts +++ b/src/review/verdict-stability.ts @@ -96,6 +96,16 @@ export function recordVerdict(prior: VerdictStabilityState | null, facts: Verdic */ export function shouldSkipStableVerdict(prior: VerdictStabilityState | null, nowMs: number): boolean { if (prior === null) return false; + // HOLDS ONLY. "Same verdict" is not the same as "nothing happened": a pass can take real actions -- + // update-branch, cap accounting, assignment -- and still produce an unchanged verdict, and throttling that + // suppresses actual progress. The force-fresh-rebase test (#9497/#2552) is exactly this shape: three + // identical passes deliberately spend the 24h update-branch cap, and an earlier version of this backoff + // silently swallowed the third. + // + // A `hold` is the one action that means "the gate declined to act", so repeating it genuinely produces + // nothing -- and it is the case this exists for (#8886: 56 identical holds on one commit). Everything else + // keeps its current behaviour exactly. + if (!prior.fingerprint.startsWith("hold|")) return false; const delay = verdictBackoffDelayMs(prior.repeats); if (delay <= 0) return false; return nowMs - prior.lastEvaluatedMs < delay; diff --git a/test/unit/verdict-stability.test.ts b/test/unit/verdict-stability.test.ts index 40bce1b67..22a027669 100644 --- a/test/unit/verdict-stability.test.ts +++ b/test/unit/verdict-stability.test.ts @@ -134,6 +134,24 @@ describe("shouldSkipStableVerdict", () => { expect(shouldSkipStableVerdict(fresh, 1001)).toBe(false); }); + it("REGRESSION: only a HOLD is ever backed off — a pass that ACTED is never throttled", () => { + // "Same verdict" is not "nothing happened". A pass can take real actions (update-branch, cap accounting, + // assignment) and still produce an unchanged verdict; throttling that suppresses progress. Caught by the + // force-fresh-rebase test (#9497/#2552), where three deliberate identical passes spend the 24h + // update-branch cap and an earlier version of this backoff swallowed the third. + for (const action of ["merge", "close", "update_branch", "approve", "label"]) { + const acted: VerdictStabilityState = { + fingerprint: verdictFingerprint({ action, reasonCode: "success" }), + repeats: 99, + lastEvaluatedMs: 0, + }; + expect(shouldSkipStableVerdict(acted, 1), action).toBe(false); + } + // ...while the hold this exists for still backs off. + const held: VerdictStabilityState = { fingerprint: verdictFingerprint(HOLD), repeats: 99, lastEvaluatedMs: 0 }; + expect(shouldSkipStableVerdict(held, 1)).toBe(true); + }); + it("INVARIANT: a stuck PR is always revisited within the cap", () => { // The liveness guarantee stated as a property rather than trusted from the delay function. const veryStuck: VerdictStabilityState = { fingerprint: verdictFingerprint(HOLD), repeats: 9999, lastEvaluatedMs: 0 };