diff --git a/migrations/0197_verdict_flip_fingerprint.sql b/migrations/0197_verdict_flip_fingerprint.sql new file mode 100644 index 0000000000..01427697e6 --- /dev/null +++ b/migrations/0197_verdict_flip_fingerprint.sql @@ -0,0 +1,14 @@ +-- #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 +-- 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..4160d2d446 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -6554,6 +6554,42 @@ 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(); + // 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 }; + } 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..962eaef79d 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,31 @@ 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. + // 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, escalate: flipCount >= VERDICT_FLIP_ESCALATION_THRESHOLD }; + return { + lastHadDefect: hadDefect, + flipCount, + lastFingerprint: fingerprint, + 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..9754a604d0 100644 --- a/test/unit/approval-queue-staleness.test.ts +++ b/test/unit/approval-queue-staleness.test.ts @@ -186,3 +186,101 @@ 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("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"); + 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/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") diff --git a/test/unit/verdict-flip-guard.test.ts b/test/unit/verdict-flip-guard.test.ts index c323e540f0..69b2ada528 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,105 @@ 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 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. + 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 +139,29 @@ 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 }); + // 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); 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();