From 6fae363f157c2001c90093869d86c6fae08dea4c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 18:20:47 -0700 Subject: [PATCH 1/4] fix(approval-queue,lifecycle): reopen expired staged actions, restore accept-path protections, and stop honest iteration escalating (#9481, #9482, #9483) The unique index on agent_pending_actions has no status predicate and the insert used onConflictDoNothing, so once #9032's sweep expired a row every later re-plan conflicted with it permanently: created:false, so stageForApproval returned before notifying and decidePendingAgentAction reported already_decided. Nothing deleted or reopened an expired row, making an auto_with_approval merge/close whose maintainer was away for the expiry window permanently unexecutable via the queue -- strictly worse than the pre-#9032 behaviour, and the opposite of what the sweep's own doc promises. An expired row is now reopened in place, guarded on status='expired' so a concurrent decision cannot be clobbered. The step-4 audit is also conditional on staging having actually happened; it used to assert "awaiting maintainer approval" on every later sweep even when staging no-op'd. The accept-replay path built a thinner executor context than the live path, silently voiding three protections. Without authorLogin, step 8c claimed the EMPTY-author cap-lock key, so #9159's mutex shared no namespace with the live path's author-keyed one and provided no mutual exclusion at all, while maybeEscalateModeration early-returned and staged-then-accepted enforcement closes recorded no moderation violation toward the warning/ban ladder. Without expectedBaseRef, #9055's base-retarget guard could never fire on a replay. The verdict-flip guard counted a flip on any direction change, keyed per PR with no content dimension -- so honest iteration produced the same signal as the exploit (two fix cycles hit the threshold), and because flipCount never decreased the PR then escalated forever, an absorbing state whose own advice to "push a substantive fix" could not clear it. Flips now count only at an unchanged content fingerprint; genuinely changed content resets. Re-rolling the same content still escalates exactly as before. --- migrations/0196_verdict_flip_fingerprint.sql | 14 +++ src/db/repositories.ts | 33 +++++++ src/queue/processors.ts | 2 +- src/review/verdict-flip-guard.ts | 33 ++++++- src/review/verdict-flip-store.ts | 17 ++-- src/services/agent-action-executor.ts | 14 ++- src/services/agent-approval-queue.ts | 25 +++++ test/unit/agent-approval-queue.test.ts | 63 +++++++++++++ test/unit/approval-queue-staleness.test.ts | 82 +++++++++++++++++ test/unit/verdict-flip-guard.test.ts | 97 ++++++++++++++++---- 10 files changed, 346 insertions(+), 34 deletions(-) create mode 100644 migrations/0196_verdict_flip_fingerprint.sql diff --git a/migrations/0196_verdict_flip_fingerprint.sql b/migrations/0196_verdict_flip_fingerprint.sql new file mode 100644 index 0000000000..730c1a9262 --- /dev/null +++ b/migrations/0196_verdict_flip_fingerprint.sql @@ -0,0 +1,14 @@ +-- #9483: the verdict-flip guard (#9016) counted a flip whenever the fresh verdict's direction changed, keyed +-- per (repo, pull) with no content dimension. The exploit it defends against is re-rolling a non-deterministic +-- reviewer against the SAME content until a lucky clean roll lands -- but an honest contributor iterating on +-- real feedback produces exactly the same signal: defect(head A) -> fix -> clean(head B) is flip 1, a new real +-- issue on head C is flip 2, fixed on head D is flip 3 -> escalate. Two honest fix cycles reached the +-- threshold, and because flipCount never decreased the PR then escalated on EVERY later fresh verdict -- +-- an absorbing state whose own user-facing advice ("push a substantive fix so the next review reflects real +-- content change") was structurally unable to clear it. +-- +-- Recording the content fingerprint each verdict was produced against lets the guard distinguish the two: a +-- re-roll at an UNCHANGED fingerprint is the abuse pattern and still counts; a verdict against genuinely +-- different content resets the count. Nullable so pre-existing rows keep working (a null prior fingerprint +-- simply cannot match, so the first verdict after this migration resets rather than escalating). +ALTER TABLE ai_review_verdict_flips ADD COLUMN last_fingerprint TEXT; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 1dec5a7af4..0a4ef00c1d 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -6554,6 +6554,39 @@ export async function createPendingAgentActionIfAbsent( .limit(1); /* v8 ignore next -- onConflictDoNothing only no-ops when a conflicting row exists, so the lookup always finds it. */ if (!existing) throw new Error(`pending action conflict had no row: ${repoFullName}#${input.pullNumber} ${input.actionClass}`); + // #9481: an EXPIRED row must not block re-staging forever. The unique index is on + // (repo_full_name, pull_number, action_class) with no status predicate, so once #9032's sweep expired a row + // this insert conflicted against it permanently -- `created: false`, so stageForApproval returned before + // notifying and decidePendingAgentAction reported `already_decided`. Nothing anywhere deletes or reopens an + // expired row, so an auto_with_approval merge/close whose maintainer was away for the expiry window became + // PERMANENTLY unexecutable via the queue for that PR + action class -- strictly worse than the pre-#9032 + // behaviour, where rows waited indefinitely but stayed acceptable. The sweep's own doc promises the + // opposite: "a later pass that re-plans the same action stages a fresh row with a fresh notification". + // + // Reopen it in place instead, guarded on status='expired' so a concurrent accept/reject can never be + // clobbered, and report `created: true` so the caller notifies exactly as it would for a brand-new row. + // Only `expired` reopens: `pending` is legitimately sticky (a decision is outstanding) and accepted/rejected + // are terminal by design. + if (existing.status === "expired") { + const [reopened] = await getDb(env.DB) + .update(agentPendingActions) + .set({ + installationId: input.installationId, + autonomyLevel: input.autonomyLevel, + paramsJson: jsonString(input.params), + reason: input.reason ?? null, + status: "pending", + decidedBy: null, + decidedAt: null, + createdAt: nowIso(), + }) + .where(and(eq(agentPendingActions.id, existing.id), eq(agentPendingActions.status, "expired"))) + .returning(); + if (reopened) return { action: toAgentPendingActionRecord(reopened), created: true }; + // Lost the CAS to a concurrent decision — fall through and report the row as it now stands. + const [current] = await getDb(env.DB).select().from(agentPendingActions).where(eq(agentPendingActions.id, existing.id)).limit(1); + if (current) return { action: toAgentPendingActionRecord(current), created: false }; + } return { action: toAgentPendingActionRecord(existing), created: false }; } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 9344c12276..5b13c8d4b7 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -11491,7 +11491,7 @@ async function maybePublishPrPublicSurface( // gates on the AI verdict, so there is nothing to shop for there. Best-effort/fail-open by // construction (recordVerdictFlip never throws); a persistable placeholder never counts as a roll. if (aiReview && aiReview.persistable !== false && settings.aiReviewMode === "block") { - const verdictFlip = await recordVerdictFlip(env, repoFullName, pr.number, aiReview.findings ?? []); + const verdictFlip = await recordVerdictFlip(env, repoFullName, pr.number, aiReview.findings ?? [], inputFingerprint); if (verdictFlip.escalate) { advisory.findings.push({ code: "ai_review_inconclusive", diff --git a/src/review/verdict-flip-guard.ts b/src/review/verdict-flip-guard.ts index 11f2488347..10aebf8603 100644 --- a/src/review/verdict-flip-guard.ts +++ b/src/review/verdict-flip-guard.ts @@ -13,7 +13,14 @@ import { AI_JUDGMENT_BLOCKER_CODES } from "../rules/advisory"; * churn (one legitimate re-review after a real fix is not abuse). */ export const VERDICT_FLIP_ESCALATION_THRESHOLD = 3; -export type VerdictFlipState = { lastHadDefect: boolean; flipCount: number }; +export type VerdictFlipState = { + lastHadDefect: boolean; + flipCount: number; + /** #9483: the content fingerprint the prior fresh verdict was produced against. Null for rows written + * before this was tracked (migration 0196) — a null can never match, so the first verdict after the + * migration resets rather than escalating, which is the fail-open direction. */ + lastFingerprint?: string | null | undefined; +}; export type VerdictFlipResult = VerdictFlipState & { escalate: boolean }; /** @@ -24,10 +31,28 @@ export type VerdictFlipResult = VerdictFlipState & { escalate: boolean }; * consistently clean, across many honest re-reviews is not the abuse pattern this guards against, only * genuine oscillation is. */ -export function nextVerdictFlipState(prior: VerdictFlipState | null, hadDefect: boolean): VerdictFlipResult { - if (prior === null) return { lastHadDefect: hadDefect, flipCount: 0, escalate: false }; +export function nextVerdictFlipState( + prior: VerdictFlipState | null, + hadDefect: boolean, + fingerprint?: string | null | undefined, +): VerdictFlipResult { + if (prior === null) return { lastHadDefect: hadDefect, flipCount: 0, lastFingerprint: fingerprint ?? null, escalate: false }; + // #9483: only a re-roll against UNCHANGED content is the abuse pattern this guards against. The exploit is + // re-rolling a non-deterministic reviewer over the same diff until a lucky clean roll lands; an honest + // contributor iterating on real feedback produced an identical signal, so defect -> fix -> clean -> new + // issue -> fixed (two honest cycles) hit the threshold. Worse, flipCount never decreased, so the PR then + // escalated on every later fresh verdict forever -- an absorbing state whose own advice to the contributor + // ("push a substantive fix so the next review reflects real content change") could not clear it, because a + // substantive fix produced a fresh verdict that re-escalated. A changed fingerprint resets the count. + const sameContent = fingerprint != null && prior.lastFingerprint != null && prior.lastFingerprint === fingerprint; + if (!sameContent) return { lastHadDefect: hadDefect, flipCount: 0, lastFingerprint: fingerprint ?? null, escalate: false }; const flipCount = prior.lastHadDefect === hadDefect ? prior.flipCount : prior.flipCount + 1; - return { lastHadDefect: hadDefect, flipCount, escalate: flipCount >= VERDICT_FLIP_ESCALATION_THRESHOLD }; + return { + lastHadDefect: hadDefect, + flipCount, + lastFingerprint: fingerprint ?? null, + escalate: flipCount >= VERDICT_FLIP_ESCALATION_THRESHOLD, + }; } /** Whether a fresh review's findings carry a blocking AI-judgment defect (ai_consensus_defect / diff --git a/src/review/verdict-flip-store.ts b/src/review/verdict-flip-store.ts index a1add2831e..1f4d38b016 100644 --- a/src/review/verdict-flip-store.ts +++ b/src/review/verdict-flip-store.ts @@ -7,12 +7,12 @@ import { errorMessage, nowIso } from "../utils/json"; export async function readVerdictFlipState(env: Env, repoFullName: string, pullNumber: number): Promise { try { const row = await env.DB.prepare( - "SELECT last_had_defect AS lastHadDefect, flip_count AS flipCount FROM ai_review_verdict_flips WHERE repo_full_name = ? AND pull_number = ?", + "SELECT last_had_defect AS lastHadDefect, flip_count AS flipCount, last_fingerprint AS lastFingerprint FROM ai_review_verdict_flips WHERE repo_full_name = ? AND pull_number = ?", ) .bind(repoFullName, pullNumber) - .first<{ lastHadDefect: number; flipCount: number }>(); + .first<{ lastHadDefect: number; flipCount: number; lastFingerprint: string | null }>(); if (!row) return null; - return { lastHadDefect: row.lastHadDefect === 1, flipCount: row.flipCount }; + return { lastHadDefect: row.lastHadDefect === 1, flipCount: row.flipCount, lastFingerprint: row.lastFingerprint }; } catch (error) { console.warn(JSON.stringify({ event: "verdict_flip_read_error", repoFullName, pullNumber, message: errorMessage(error).slice(0, 120) })); return null; @@ -22,10 +22,10 @@ export async function readVerdictFlipState(env: Env, repoFullName: string, pullN async function writeVerdictFlipState(env: Env, repoFullName: string, pullNumber: number, next: VerdictFlipState): Promise { try { await env.DB.prepare( - `INSERT INTO ai_review_verdict_flips (repo_full_name, pull_number, last_had_defect, flip_count, updated_at) VALUES (?, ?, ?, ?, ?) - ON CONFLICT(repo_full_name, pull_number) DO UPDATE SET last_had_defect = excluded.last_had_defect, flip_count = excluded.flip_count, updated_at = excluded.updated_at`, + `INSERT INTO ai_review_verdict_flips (repo_full_name, pull_number, last_had_defect, flip_count, last_fingerprint, updated_at) VALUES (?, ?, ?, ?, ?, ?) + ON CONFLICT(repo_full_name, pull_number) DO UPDATE SET last_had_defect = excluded.last_had_defect, flip_count = excluded.flip_count, last_fingerprint = excluded.last_fingerprint, updated_at = excluded.updated_at`, ) - .bind(repoFullName, pullNumber, next.lastHadDefect ? 1 : 0, next.flipCount, nowIso()) + .bind(repoFullName, pullNumber, next.lastHadDefect ? 1 : 0, next.flipCount, next.lastFingerprint ?? null, nowIso()) .run(); } catch (error) { console.warn(JSON.stringify({ event: "verdict_flip_write_error", repoFullName, pullNumber, message: errorMessage(error).slice(0, 120) })); @@ -42,10 +42,13 @@ export async function recordVerdictFlip( repoFullName: string, pullNumber: number, findings: ReadonlyArray<{ code: string }>, + /** #9483: the review's content fingerprint. A flip only counts when it MATCHES the prior verdict's, so + * honest iteration on genuinely changed content resets rather than accumulating toward a permanent hold. */ + fingerprint?: string | null | undefined, ): Promise { const hadDefect = findingsHadAiDefect(findings); const prior = await readVerdictFlipState(env, repoFullName, pullNumber); - const next = nextVerdictFlipState(prior, hadDefect); + const next = nextVerdictFlipState(prior, hadDefect, fingerprint); await writeVerdictFlipState(env, repoFullName, pullNumber, next); return next; } diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 1b237826e9..5b8e7f429e 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -437,8 +437,11 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE // 4) auto_with_approval stages the action in the approval queue (#779) for a one-tap maintainer decision // instead of executing it now. Staging is not a GitHub mutation; execution/replay runs this guard later. if (action.requiresApproval) { - await stageForApproval(env, ctx, action, autonomyLevel); - await audit("queued", `awaiting maintainer approval — ${action.reason}`); + // #9481: the audit is conditional on staging having actually happened. It used to fire unconditionally, + // so when staging no-op'd against an existing row every later sweep still recorded "awaiting maintainer + // approval" -- an audit trail asserting a notification that was never sent. + const staged = await stageForApproval(env, ctx, action, autonomyLevel); + if (staged) await audit("queued", `awaiting maintainer approval — ${action.reason}`); continue; } // 5) Write-permission readiness: a PR-visible action needs its exact GitHub App write permission granted. @@ -1350,7 +1353,9 @@ export function pendingActionToPlanned(input: { actionClass: AgentActionClass; p } // Persist the staged action + notify the maintainer ONCE (on first staging, not on every re-evaluation). -async function stageForApproval(env: Env, ctx: AgentActionExecutionContext, action: PlannedAgentAction, autonomyLevel: AutonomyLevel): Promise { +/** Returns whether a row was actually staged (a fresh one, or an expired one reopened per #9481) -- the + * caller only audits "awaiting maintainer approval" when a notification genuinely went out. */ +async function stageForApproval(env: Env, ctx: AgentActionExecutionContext, action: PlannedAgentAction, autonomyLevel: AutonomyLevel): Promise { const { created } = await createPendingAgentActionIfAbsent(env, { repoFullName: ctx.repoFullName, pullNumber: ctx.pullNumber, @@ -1360,7 +1365,7 @@ async function stageForApproval(env: Env, ctx: AgentActionExecutionContext, acti params: actionParams(action), reason: action.reason, }); - if (!created) return; + if (!created) return false; /* v8 ignore next -- a repo full name always has an owner segment; the empty fallback is purely defensive. */ const recipientLogin = ctx.repoFullName.split("/")[0] ?? ""; await insertNotificationDeliveryIfAbsent(env, { @@ -1375,4 +1380,5 @@ async function stageForApproval(env: Env, ctx: AgentActionExecutionContext, acti deeplink: `https://github.com/${ctx.repoFullName}/pull/${ctx.pullNumber}`, actorLogin: AGENT_ACTOR, }); + return true; } diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 6208f085bb..c05c753f54 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -499,6 +499,31 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de agentDryRun: settings.agentDryRun, installationPermissions: installation ? installation.permissions : null, mergeTrainMode: settings.mergeTrainMode, + // #9482: three protections were silently VOID on this replay path because the ctx omitted the fields + // they key on, while the live path (src/queue/processors.ts) supplies all three. + // + // authorLogin: step 8c claims `claimContributorCapLock(env, repo, ctx.authorLogin ?? "")`, so an absent + // author produced the key `contributor-cap-lock::` -- the EMPTY-author key. #9159's whole point is + // that an accept-merge and a sibling's cap-close for the same author serialize on one mutex; keyed on + // "" it provided zero mutual exclusion against the live path's `...:` key, leaving the #7284 + // TOCTOU wide open, while also making every accept-path merge in a repo contend on one shared key. + // maybeEscalateModeration also early-returns on a falsy authorLogin, so a staged-then-accepted + // blacklist / contributor-cap / review-nag close recorded NO moderation violation at all -- a + // contributor whose enforcement closes always route through approval accumulated zero standing + // violations toward the warning/ban ladder. + authorLogin: pr?.authorLogin, + moderationSettings: { + moderationGateMode: settings.moderationGateMode, + moderationRules: settings.moderationRules, + moderationWarningLabel: settings.moderationWarningLabel, + moderationBannedLabel: settings.moderationBannedLabel, + }, + // expectedBaseRef: fetchPullRequestFreshness only checks the base when this is set (pr-freshness.ts), so + // #9055's base-retarget guard never fired on a replay. NOTE this is the base as it stands NOW, which + // closes the common case (a retarget between staging and accept is visible here) but not the full one: + // catching a retarget that the webhook already wrote back requires the STAGING-TIME base persisted into + // AgentPendingActionParams, which has no field for it. Tracked as the remaining half of #9482. + expectedBaseRef: pr?.baseRef, pullRequestCreatedAt: pr?.createdAt, pullRequestLinkedIssues: pr?.linkedIssues, pullRequestChangedFiles: pr?.changedFiles, diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index c92d51dee5..05c823e061 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -68,6 +68,8 @@ import { resolveLinkedIssueHardRule } from "../../src/review/linked-issue-hard-r import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import { actionParams, executeAgentMaintenanceActions, pendingActionToPlanned, type AgentActionExecutionContext } from "../../src/services/agent-action-executor"; import { decidePendingAgentAction } from "../../src/services/agent-approval-queue"; +import { claimContributorCapLock, releaseContributorCapLock } from "../../src/queue/transient-locks"; +import { fetchPullRequestFreshness } from "../../src/github/pr-freshness"; import { countPendingAgentActions, createPendingAgentActionIfAbsent, @@ -1881,3 +1883,64 @@ describe("#9159: the accept-replay path threads a contributor-cap merge re-check expect(mergePullRequest).toHaveBeenCalled(); }); }); + +// #9482: the accept-replay path built a THINNER executor context than the live path, silently voiding three +// #-numbered protections. The ctx omitted authorLogin, expectedBaseRef and moderationSettings entirely, and +// every one of those fields is optional, so nothing forced parity with runAgentMaintenancePlanAndExecute's ctx. +describe("#9482: the accept-replay path threads the same protective context the live path does", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("REGRESSION: passes expectedBaseRef, so #9055's base-retarget guard is not inert on a replay", async () => { + // fetchPullRequestFreshness only evaluates the base when expectedBaseRef is set (pr-freshness.ts), so with + // it absent a contributor could retarget the PR's base between staging and accept -- head unchanged, so + // the head-SHA pin still matched -- and the merge landed against a base the reviewed diff and CI never + // targeted. Wrong-merge class. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: 8, + title: "PR", + state: "open", + user: { login: "contributor1" }, + head: { sha: "h8" }, + base: { ref: "main" }, + labels: [], + body: "x", + }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 8, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h8" }, reason: "clean" }); + + await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + const freshnessCalls = vi.mocked(fetchPullRequestFreshness).mock.calls; + expect(freshnessCalls.length).toBeGreaterThan(0); + // The guard can only fire if the executor is told what base to expect. + expect(freshnessCalls.some(([, args]) => (args as { expectedBaseRef?: string | null }).expectedBaseRef === "main")).toBe(true); + }); + + it("REGRESSION: passes authorLogin, so the #9159 cap mutex keys on the author rather than the empty string", async () => { + // Step 8c claims claimContributorCapLock(env, repo, ctx.authorLogin ?? ""). With authorLogin absent the key + // was `contributor-cap-lock::` -- an EMPTY-author key that shares no namespace with the live path's + // `...:`, so #9159's mutex provided zero mutual exclusion against a sibling cap-close and every + // accept-path merge in a repo contended on one shared key. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await upsertRepoFocusManifest(env, "owner/repo", { settings: { contributorOpenPrCap: 5 } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 9, title: "PR", state: "open", user: { login: "farmer99" }, head: { sha: "h9" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 9, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h9" }, reason: "clean" }); + + // Pre-claim the AUTHOR-keyed lock. Before this fix the accept path claimed the empty-author key instead, + // so it did not contend at all and the merge proceeded; now it must be denied. + const held = await claimContributorCapLock(env, "owner/repo", "farmer99"); + expect(held.acquired).toBe(true); + try { + await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(mergePullRequest).not.toHaveBeenCalled(); + } finally { + await releaseContributorCapLock(env, "owner/repo", "farmer99", held.ownerToken); + } + }); +}); diff --git a/test/unit/approval-queue-staleness.test.ts b/test/unit/approval-queue-staleness.test.ts index de46b9b79b..8d274297cb 100644 --- a/test/unit/approval-queue-staleness.test.ts +++ b/test/unit/approval-queue-staleness.test.ts @@ -186,3 +186,85 @@ describe("sweepStaleApprovalQueue (#9032)", () => { expect((await getPendingAgentAction(env, id))?.status).toBe("pending"); }); }); + +// #9481 regression: the unique index on (repo_full_name, pull_number, action_class) has NO status predicate, +// and createPendingAgentActionIfAbsent used onConflictDoNothing against it. So once #9032's sweep expired a +// row, every later re-plan conflicted with it permanently -- `created: false`, so stageForApproval returned +// before notifying and decidePendingAgentAction reported already_decided. Nothing anywhere deleted or reopened +// an expired row, which made an auto_with_approval merge/close whose maintainer was away for the expiry window +// PERMANENTLY unexecutable via the queue for that PR + action class. That is strictly worse than the +// pre-#9032 behaviour (rows waited forever but stayed acceptable), and the sweep's own doc promises the +// opposite: "a later pass that re-plans the same action stages a fresh row with a fresh notification". +describe("re-staging after expiry (#9481)", () => { + const stage = (env: Env, reason: string) => + createPendingAgentActionIfAbsent(env, { + repoFullName: "acme/widgets", + pullNumber: 42, + installationId: 5, + actionClass: "merge", + autonomyLevel: "auto_with_approval", + params: {}, + reason, + }); + + it("reopens an EXPIRED row so the action can be staged again", async () => { + const env = createTestEnv(); + const first = await stage(env, "original reason"); + expect(first.created).toBe(true); + + await setPendingAgentActionStatus(env, first.action.id, { status: "expired", decidedBy: "system" }); + expect((await getPendingAgentAction(env, first.action.id))?.status).toBe("expired"); + + const restaged = await stage(env, "re-planned reason"); + // `created: true` is what makes the caller notify -- without it the maintainer is never told. + expect(restaged.created).toBe(true); + expect(restaged.action.status).toBe("pending"); + expect(restaged.action.reason).toBe("re-planned reason"); + // Reopened in place, so the unique index is still satisfied and there is exactly one row for the target. + expect(restaged.action.id).toBe(first.action.id); + expect((await listPendingAgentActions(env, { repoFullName: "acme/widgets" })).length).toBe(1); + }); + + it("clears the prior decision metadata when reopening, so the row does not look already-decided", async () => { + const env = createTestEnv(); + const first = await stage(env, "original"); + await setPendingAgentActionStatus(env, first.action.id, { status: "expired", decidedBy: "sweeper" }); + + const restaged = await stage(env, "re-planned"); + expect(restaged.action.decidedBy).toBeNull(); + expect(restaged.action.decidedAt).toBeNull(); + }); + + it("INVARIANT: a PENDING row stays sticky — a re-plan must not reset a decision that is still outstanding", async () => { + const env = createTestEnv(); + const first = await stage(env, "original"); + const second = await stage(env, "should not overwrite"); + + expect(second.created).toBe(false); + expect(second.action.id).toBe(first.action.id); + expect(second.action.reason).toBe("original"); + }); + + it.each([["accepted"], ["rejected"]] as const)( + "INVARIANT: a %s row stays terminal — only expiry reopens", + async (status) => { + const env = createTestEnv(); + const first = await stage(env, "original"); + await setPendingAgentActionStatus(env, first.action.id, { status, decidedBy: "maintainer" }); + + const restaged = await stage(env, "re-planned"); + expect(restaged.created).toBe(false); + expect(restaged.action.status).toBe(status); + expect(restaged.action.reason).toBe("original"); + }, + ); + + it("a reopened row can be expired and reopened again (no one-shot recovery)", async () => { + const env = createTestEnv(); + const first = await stage(env, "one"); + await setPendingAgentActionStatus(env, first.action.id, { status: "expired", decidedBy: "system" }); + expect((await stage(env, "two")).created).toBe(true); + await setPendingAgentActionStatus(env, first.action.id, { status: "expired", decidedBy: "system" }); + expect((await stage(env, "three")).created).toBe(true); + }); +}); diff --git a/test/unit/verdict-flip-guard.test.ts b/test/unit/verdict-flip-guard.test.ts index c323e540f0..a214cc2f4e 100644 --- a/test/unit/verdict-flip-guard.test.ts +++ b/test/unit/verdict-flip-guard.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi } from "vitest"; -import { findingsHadAiDefect, nextVerdictFlipState, VERDICT_FLIP_ESCALATION_THRESHOLD } from "../../src/review/verdict-flip-guard"; +import { findingsHadAiDefect, nextVerdictFlipState, VERDICT_FLIP_ESCALATION_THRESHOLD, type VerdictFlipResult } from "../../src/review/verdict-flip-guard"; import { readVerdictFlipState, recordVerdictFlip } from "../../src/review/verdict-flip-store"; import { getCachedAiReviewAcrossHeads, putCachedAiReview } from "../../src/db/repositories"; import { createTestEnv } from "../helpers/d1"; @@ -20,37 +20,95 @@ describe("findingsHadAiDefect", () => { }); describe("nextVerdictFlipState", () => { + // #9483: every case below re-rolls against the SAME content fingerprint, because that is the exploit this + // guard exists to stop — a contributor re-rolling a non-deterministic reviewer over an unchanged diff. A + // verdict against genuinely CHANGED content is honest iteration and is covered separately below. + const FP = "fingerprint-unchanged"; + it("a first observation never escalates — nothing to flip against yet", () => { - expect(nextVerdictFlipState(null, true)).toEqual({ lastHadDefect: true, flipCount: 0, escalate: false }); - expect(nextVerdictFlipState(null, false)).toEqual({ lastHadDefect: false, flipCount: 0, escalate: false }); + expect(nextVerdictFlipState(null, true, FP)).toEqual({ lastHadDefect: true, flipCount: 0, lastFingerprint: FP, escalate: false }); + expect(nextVerdictFlipState(null, false, FP)).toEqual({ lastHadDefect: false, flipCount: 0, lastFingerprint: FP, escalate: false }); }); it("repeating the same verdict does NOT add to the flip count — consistency is not the abuse pattern", () => { - const state = { lastHadDefect: true, flipCount: 2 }; - expect(nextVerdictFlipState(state, true)).toEqual({ lastHadDefect: true, flipCount: 2, escalate: false }); + const state = { lastHadDefect: true, flipCount: 2, lastFingerprint: FP }; + expect(nextVerdictFlipState(state, true, FP)).toEqual({ lastHadDefect: true, flipCount: 2, lastFingerprint: FP, escalate: false }); }); it("a change from the prior verdict increments the flip count", () => { - expect(nextVerdictFlipState({ lastHadDefect: true, flipCount: 0 }, false)).toEqual({ lastHadDefect: false, flipCount: 1, escalate: false }); + expect(nextVerdictFlipState({ lastHadDefect: true, flipCount: 0, lastFingerprint: FP }, false, FP)).toEqual({ + lastHadDefect: false, + flipCount: 1, + lastFingerprint: FP, + escalate: false, + }); }); it(`escalates once flipCount clears the threshold (${VERDICT_FLIP_ESCALATION_THRESHOLD})`, () => { - const belowThreshold = { lastHadDefect: false, flipCount: VERDICT_FLIP_ESCALATION_THRESHOLD - 1 }; - expect(nextVerdictFlipState(belowThreshold, true)).toMatchObject({ flipCount: VERDICT_FLIP_ESCALATION_THRESHOLD, escalate: true }); + const belowThreshold = { lastHadDefect: false, flipCount: VERDICT_FLIP_ESCALATION_THRESHOLD - 1, lastFingerprint: FP }; + expect(nextVerdictFlipState(belowThreshold, true, FP)).toMatchObject({ flipCount: VERDICT_FLIP_ESCALATION_THRESHOLD, escalate: true }); }); - it("simulates the exploit: defect, clean, defect, clean — escalates on the Nth flip, not before", () => { - let state: { lastHadDefect: boolean; flipCount: number } | null = null; + it("simulates the exploit: defect, clean, defect, clean at an UNCHANGED fingerprint — escalates on the Nth flip", () => { + let state: { lastHadDefect: boolean; flipCount: number; lastFingerprint?: string | null | undefined } | null = null; const rolls = [true, false, true, false, true]; // 4 flips across 5 rolls const escalations: boolean[] = []; for (const hadDefect of rolls) { - const next = nextVerdictFlipState(state, hadDefect); + const next = nextVerdictFlipState(state, hadDefect, FP); escalations.push(next.escalate); state = next; } // Flips happen at rolls 2,3,4,5 (indices 1-4); threshold 3 is cleared at the 3rd flip (index 3). expect(escalations).toEqual([false, false, false, true, true]); }); + + // #9483 regressions: the guard used to be content-blind, so honest iteration produced the same signal as + // the exploit and, once escalated, could never recover. + it("#9483 REGRESSION: honest iteration on CHANGED content never accumulates flips", () => { + // defect(A) -> fix -> clean(B) -> new issue(C) -> fixed(D): two honest cycles reached the old threshold. + let state: VerdictFlipResult | null = null; + const rolls: Array<[boolean, string]> = [ + [true, "fp-A"], + [false, "fp-B"], + [true, "fp-C"], + [false, "fp-D"], + [true, "fp-E"], + ]; + for (const [hadDefect, fingerprint] of rolls) { + state = nextVerdictFlipState(state, hadDefect, fingerprint); + expect(state.escalate).toBe(false); + expect(state.flipCount).toBe(0); + } + }); + + it("#9483 REGRESSION: a substantive fix CLEARS an already-escalated state — the advice the finding gives is now effective", () => { + // The escalation finding tells the contributor to "push a substantive fix so the next review reflects real + // content change". Before this, a substantive fix produced a fresh verdict that simply re-escalated, + // because flipCount never decreased — an absorbing state its own remedy could not exit. + const escalated = { lastHadDefect: true, flipCount: VERDICT_FLIP_ESCALATION_THRESHOLD, lastFingerprint: FP }; + const afterRealChange = nextVerdictFlipState(escalated, false, "fp-after-substantive-fix"); + expect(afterRealChange.escalate).toBe(false); + expect(afterRealChange.flipCount).toBe(0); + }); + + it("#9483 INVARIANT: the exploit still escalates immediately after a content change resets the count", () => { + // Resetting must not hand the attacker a cheap way to launder flips: once they go back to re-rolling the + // SAME content, the count climbs again from zero exactly as before. + let state = nextVerdictFlipState(null, true, "fp-new"); + for (const hadDefect of [false, true, false]) { + state = nextVerdictFlipState(state, hadDefect, "fp-new"); + } + expect(state.flipCount).toBe(VERDICT_FLIP_ESCALATION_THRESHOLD); + expect(state.escalate).toBe(true); + }); + + it("#9483 a missing fingerprint on either side never counts a flip (fail-open for pre-migration rows)", () => { + // Rows written before migration 0196 have a null last_fingerprint. A null can never match, so the first + // verdict after the migration resets rather than escalating — the fail-open direction. + expect(nextVerdictFlipState({ lastHadDefect: true, flipCount: 2, lastFingerprint: null }, false, FP).flipCount).toBe(0); + expect(nextVerdictFlipState({ lastHadDefect: true, flipCount: 2, lastFingerprint: FP }, false, undefined).flipCount).toBe(0); + expect(nextVerdictFlipState({ lastHadDefect: true, flipCount: 2, lastFingerprint: null }, false, null).flipCount).toBe(0); + }); }); describe("verdict-flip store (D1, fail-open)", () => { @@ -71,23 +129,26 @@ describe("verdict-flip store (D1, fail-open)", () => { const env = createTestEnv(); const defect = [{ code: "ai_consensus_defect", title: "d", severity: "critical", detail: "d" }]; const clean: typeof defect = []; + // #9483: all four rolls share ONE fingerprint -- this test simulates the exploit (re-rolling unchanged + // content), which is exactly the case that must still escalate. + const FP = "fp-same-content"; // Roll 1: defect (first observation, no escalate). - expect(await recordVerdictFlip(env, "o/r", 5, defect)).toMatchObject({ lastHadDefect: true, flipCount: 0, escalate: false }); + expect(await recordVerdictFlip(env, "o/r", 5, defect, FP)).toMatchObject({ lastHadDefect: true, flipCount: 0, escalate: false }); // Rolls 2-4: clean, defect, clean — 3 flips, escalates on the 3rd. - expect(await recordVerdictFlip(env, "o/r", 5, clean)).toMatchObject({ flipCount: 1, escalate: false }); - expect(await recordVerdictFlip(env, "o/r", 5, defect)).toMatchObject({ flipCount: 2, escalate: false }); - expect(await recordVerdictFlip(env, "o/r", 5, clean)).toMatchObject({ flipCount: 3, escalate: true }); + expect(await recordVerdictFlip(env, "o/r", 5, clean, FP)).toMatchObject({ flipCount: 1, escalate: false }); + expect(await recordVerdictFlip(env, "o/r", 5, defect, FP)).toMatchObject({ flipCount: 2, escalate: false }); + expect(await recordVerdictFlip(env, "o/r", 5, clean, FP)).toMatchObject({ flipCount: 3, escalate: true }); // State persisted between calls (a different PR is fully independent). - expect(await readVerdictFlipState(env, "o/r", 5)).toEqual({ lastHadDefect: false, flipCount: 3 }); + expect(await readVerdictFlipState(env, "o/r", 5)).toEqual({ lastHadDefect: false, flipCount: 3, lastFingerprint: FP }); expect(await readVerdictFlipState(env, "o/r", 999)).toBeNull(); // Write failure: the RETURNED state is still correct (computed before the write attempt); it just won't persist. const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); const writeBroken = createTestEnv(); - await recordVerdictFlip(writeBroken, "o/r", 6, defect); + await recordVerdictFlip(writeBroken, "o/r", 6, defect, "fp-x"); vi.spyOn(writeBroken.DB, "prepare").mockImplementation(() => { throw new Error("db down"); }); - const result = await recordVerdictFlip(writeBroken, "o/r", 6, clean); + const result = await recordVerdictFlip(writeBroken, "o/r", 6, clean, "fp-x"); expect(result).toMatchObject({ flipCount: 0 }); // read failed (mocked) -> treated as first observation expect(warn).toHaveBeenCalled(); vi.restoreAllMocks(); From d01d718a52d3fd5d1a5e150d1cfb7b40c949bba0 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 18:35:15 -0700 Subject: [PATCH 2/4] test(approval-queue,lifecycle): close the remaining patch-coverage arms (#9481, #9483) Restructures the flip guard's content check as an early return so TypeScript narrows the fingerprint, removing a nullish fallback whose null arm was unreachable on the counting path. Drops the redundant re-read after a lost CAS -- reporting the row already in hand with created:false is the same answer -- and annotates the remaining race arm with its reason. Adds coverage for the nullish arms: reopening with no reason, a first observation and a counted flip both persisting null rather than undefined. --- src/db/repositories.ts | 9 ++++++--- src/review/verdict-flip-guard.ts | 9 ++++++--- test/unit/approval-queue-staleness.test.ts | 16 ++++++++++++++++ test/unit/verdict-flip-guard.test.ts | 13 +++++++++++++ 4 files changed, 41 insertions(+), 6 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 0a4ef00c1d..4160d2d446 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -6582,10 +6582,13 @@ export async function createPendingAgentActionIfAbsent( }) .where(and(eq(agentPendingActions.id, existing.id), eq(agentPendingActions.status, "expired"))) .returning(); + // Losing the CAS means a concurrent decision already moved the row off `expired`. Reporting the row we + // read with created:false is the correct answer either way -- the caller must not notify -- so there is no + // need to re-read it just to say the same thing. + /* v8 ignore next -- the falsy arm is a genuine CAS loss (a concurrent accept/reject moved the row off + `expired` between the SELECT and this UPDATE), which no deterministic test can stage; it falls through + to the same created:false answer below. */ if (reopened) return { action: toAgentPendingActionRecord(reopened), created: true }; - // Lost the CAS to a concurrent decision — fall through and report the row as it now stands. - const [current] = await getDb(env.DB).select().from(agentPendingActions).where(eq(agentPendingActions.id, existing.id)).limit(1); - if (current) return { action: toAgentPendingActionRecord(current), created: false }; } return { action: toAgentPendingActionRecord(existing), created: false }; } diff --git a/src/review/verdict-flip-guard.ts b/src/review/verdict-flip-guard.ts index 10aebf8603..962eaef79d 100644 --- a/src/review/verdict-flip-guard.ts +++ b/src/review/verdict-flip-guard.ts @@ -44,13 +44,16 @@ export function nextVerdictFlipState( // escalated on every later fresh verdict forever -- an absorbing state whose own advice to the contributor // ("push a substantive fix so the next review reflects real content change") could not clear it, because a // substantive fix produced a fresh verdict that re-escalated. A changed fingerprint resets the count. - const sameContent = fingerprint != null && prior.lastFingerprint != null && prior.lastFingerprint === fingerprint; - if (!sameContent) return { lastHadDefect: hadDefect, flipCount: 0, lastFingerprint: fingerprint ?? null, escalate: false }; + // Written as an early return rather than a `sameContent` boolean so TypeScript narrows `fingerprint` to a + // string below -- otherwise the counting path needs a `?? null` fallback whose null arm is unreachable. + if (fingerprint == null || prior.lastFingerprint == null || prior.lastFingerprint !== fingerprint) { + return { lastHadDefect: hadDefect, flipCount: 0, lastFingerprint: fingerprint ?? null, escalate: false }; + } const flipCount = prior.lastHadDefect === hadDefect ? prior.flipCount : prior.flipCount + 1; return { lastHadDefect: hadDefect, flipCount, - lastFingerprint: fingerprint ?? null, + lastFingerprint: fingerprint, escalate: flipCount >= VERDICT_FLIP_ESCALATION_THRESHOLD, }; } diff --git a/test/unit/approval-queue-staleness.test.ts b/test/unit/approval-queue-staleness.test.ts index 8d274297cb..9754a604d0 100644 --- a/test/unit/approval-queue-staleness.test.ts +++ b/test/unit/approval-queue-staleness.test.ts @@ -259,6 +259,22 @@ describe("re-staging after expiry (#9481)", () => { }, ); + it("reopens with a null reason when the re-plan supplies none", async () => { + const env = createTestEnv(); + const first = await stage(env, "original"); + await setPendingAgentActionStatus(env, first.action.id, { status: "expired", decidedBy: "system" }); + const restaged = await createPendingAgentActionIfAbsent(env, { + repoFullName: "acme/widgets", + pullNumber: 42, + installationId: 5, + actionClass: "merge", + autonomyLevel: "auto_with_approval", + params: {}, + }); + expect(restaged.created).toBe(true); + expect(restaged.action.reason).toBeNull(); + }); + it("a reopened row can be expired and reopened again (no one-shot recovery)", async () => { const env = createTestEnv(); const first = await stage(env, "one"); diff --git a/test/unit/verdict-flip-guard.test.ts b/test/unit/verdict-flip-guard.test.ts index a214cc2f4e..69b2ada528 100644 --- a/test/unit/verdict-flip-guard.test.ts +++ b/test/unit/verdict-flip-guard.test.ts @@ -102,6 +102,16 @@ describe("nextVerdictFlipState", () => { expect(state.escalate).toBe(true); }); + it("#9483 a first observation records a null fingerprint when none is supplied", () => { + expect(nextVerdictFlipState(null, true)).toEqual({ lastHadDefect: true, flipCount: 0, lastFingerprint: null, escalate: false }); + }); + + it("#9483 a counted flip records a null fingerprint when none is supplied", () => { + // The nullish fallback must hold on the counting path too, so a later comparison cannot match on undefined. + const state = { lastHadDefect: true, flipCount: 0, lastFingerprint: null }; + expect(nextVerdictFlipState(state, false, null).lastFingerprint).toBeNull(); + }); + it("#9483 a missing fingerprint on either side never counts a flip (fail-open for pre-migration rows)", () => { // Rows written before migration 0196 have a null last_fingerprint. A null can never match, so the first // verdict after the migration resets rather than escalating — the fail-open direction. @@ -140,6 +150,9 @@ describe("verdict-flip store (D1, fail-open)", () => { expect(await recordVerdictFlip(env, "o/r", 5, clean, FP)).toMatchObject({ flipCount: 3, escalate: true }); // State persisted between calls (a different PR is fully independent). expect(await readVerdictFlipState(env, "o/r", 5)).toEqual({ lastHadDefect: false, flipCount: 3, lastFingerprint: FP }); + // A verdict recorded with NO fingerprint persists null rather than undefined, so a later read compares cleanly. + await recordVerdictFlip(env, "o/r", 7, defect); + expect(await readVerdictFlipState(env, "o/r", 7)).toEqual({ lastHadDefect: true, flipCount: 0, lastFingerprint: null }); expect(await readVerdictFlipState(env, "o/r", 999)).toBeNull(); // Write failure: the RETURNED state is still correct (computed before the write attempt); it just won't persist. const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); From 2135db9fd523d62789adcd3f30a028c86c669d3f Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:17:55 -0700 Subject: [PATCH 3/4] test(lifecycle): make the flip-escalation integration test simulate the real exploit (#9483) The test inserted a prior flip row with a null fingerprint, which under #9483 correctly resets rather than escalating -- honest iteration on changed content is not the abuse pattern. It now runs one real pass to establish the fingerprint for this exact content, forces the counter to the threshold with the opposite prior verdict, clears the review cache so the second pass is a genuinely FRESH verdict rather than a reuse, and re-rolls the SAME content. That is precisely the exploit the guard exists to stop, and it still escalates. --- test/unit/queue.test.ts | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index ed5cbbd1b2..36daf3dd5b 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -6926,13 +6926,11 @@ describe("queue processors", () => { await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 77, title: "Flip-flop PR", state: "open", user: { login: "contributor" }, head: { sha: headSha }, labels: [], body: "Closes #1" }); await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 77, status: "complete", reviewsSyncedAt: new Date().toISOString() }); await upsertPullRequestFile(env, { repoFullName: "JSONbored/gittensory", pullNumber: 77, path: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, payload: { patch: "@@\n+export const roll = final;" } }); - // Pre-seed: last fresh verdict HAD a defect, and it has already flipped VERDICT_FLIP_ESCALATION_THRESHOLD - // times. The next verdict CHANGE (clean, i.e. no defect) crosses the threshold. - await env.DB.prepare( - "INSERT INTO ai_review_verdict_flips (repo_full_name, pull_number, last_had_defect, flip_count, updated_at) VALUES (?, ?, 1, ?, ?)", - ) - .bind("JSONbored/gittensory", 77, VERDICT_FLIP_ESCALATION_THRESHOLD, new Date().toISOString()) - .run(); + // #9483: a flip now only counts when the review's content fingerprint MATCHES the prior verdict's -- + // re-rolling UNCHANGED content is the exploit; honest iteration on changed content is not. So the prior + // state cannot simply be inserted with a null fingerprint (that would reset, by design). Instead run one + // real pass to establish the fingerprint for this exact content, then force the counter to the threshold + // and re-roll the SAME content, which is precisely the abuse pattern this guard exists to stop. vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { const url = input.toString(); @@ -6952,6 +6950,17 @@ describe("queue processors", () => { return Response.json({}); }); + // Pass 1: establishes last_fingerprint for this content. + await processJob(env, { type: "agent-regate-pr", deliveryId: "flip-seed", repoFullName: "JSONbored/gittensory", prNumber: 77, installationId: 123 }); + // Force the counter to the threshold with the OPPOSITE prior verdict, keeping the fingerprint pass 1 + // recorded, so the next roll of the same content is a flip that crosses the threshold. + await env.DB.prepare("UPDATE ai_review_verdict_flips SET last_had_defect = 1, flip_count = ? WHERE repo_full_name = ? AND pull_number = ?") + .bind(VERDICT_FLIP_ESCALATION_THRESHOLD, "JSONbored/gittensory", 77) + .run(); + // Drop the cached review so pass 2 is a genuinely FRESH verdict -- a cache hit is a reuse, not a new roll, + // and deliberately does not count toward the flip state. + await env.DB.prepare("DELETE FROM ai_review_cache").run(); + await processJob(env, { type: "agent-regate-pr", deliveryId: "flip-final", repoFullName: "JSONbored/gittensory", prNumber: 77, installationId: 123 }); const finalBlocker = await env.DB.prepare("select blocker_codes_json from gate_outcomes where repo_full_name = ? and pull_number = ? order by rowid desc limit 1") From 2d87621ea65153191ed98651680833f9384b7239 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 22:08:06 -0700 Subject: [PATCH 4/4] fix(migrations): renumber the verdict-flip migration to 0197 after a 0196 collision Both branches took 0196 from the same base, so the contiguity check passed on each in isolation. The retention index migration merged first, so this one moves. --- ...t_flip_fingerprint.sql => 0197_verdict_flip_fingerprint.sql} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename migrations/{0196_verdict_flip_fingerprint.sql => 0197_verdict_flip_fingerprint.sql} (86%) diff --git a/migrations/0196_verdict_flip_fingerprint.sql b/migrations/0197_verdict_flip_fingerprint.sql similarity index 86% rename from migrations/0196_verdict_flip_fingerprint.sql rename to migrations/0197_verdict_flip_fingerprint.sql index 730c1a9262..01427697e6 100644 --- a/migrations/0196_verdict_flip_fingerprint.sql +++ b/migrations/0197_verdict_flip_fingerprint.sql @@ -1,4 +1,4 @@ --- #9483: the verdict-flip guard (#9016) counted a flip whenever the fresh verdict's direction changed, keyed +-- #9483 (renumbered 0196 -> 0197: the retention index migration took 0196 first): the verdict-flip guard (#9016) counted a flip whenever the fresh verdict's direction changed, keyed -- per (repo, pull) with no content dimension. The exploit it defends against is re-rolling a non-deterministic -- reviewer against the SAME content until a lucky clean roll lands -- but an honest contributor iterating on -- real feedback produces exactly the same signal: defect(head A) -> fix -> clean(head B) is flip 1, a new real