From dc768f2a4d9de30738b3d8482070087078c1618a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 09:59:56 -0700 Subject: [PATCH 1/2] =?UTF-8?q?fix(review):=20stop=20AI-review=20verdict-s?= =?UTF-8?q?hopping=20=E2=80=94=20content-fingerprint=20stickiness=20+=20fl?= =?UTF-8?q?ip-count=20escalation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The AI reviewer is non-deterministic, and the review cache keys purely on head SHA, so a contributor could force fresh re-rolls (a no-op recommit invalidating the cache key, or a same-head retry once the 30-minute non-cacheable cooldown lapses) until a lucky clean roll auto-merged a PR another roll had flagged as blocked. Two independent defenses: 1. getCachedAiReviewAcrossHeads (src/db/repositories.ts): a fallback used only when the exact-head lookup misses. The input fingerprint already hashes the actual per-file patch content, not just the head SHA, so an identical fingerprint under a different head means the reviewed content is genuinely unchanged -- reuse the prior verdict instead of spending an independently-random fresh roll on it. 2. A per-PR verdict-flip counter (migration 0183, src/review/ verdict-flip-guard.ts + verdict-flip-store.ts): every FRESH (non-cache-hit) verdict in block mode is compared against the PR's last fresh verdict; a flip is a change in whether the verdict had a blocking AI defect. Once flips clear a threshold, the gate holds for a human instead of trusting the newest roll, regardless of what it says. Fixed a pre-existing test (#ops-review-burst) whose fixture returned identical patch content across two different head SHAs -- exactly the no-op-recommit case fix #1 is designed to reuse, which made its own aiCalls assertion test the wrong thing; it now varies real content between heads and stays a valid regression for the ORIGINAL concern (a genuinely new commit is never suppressed). --- migrations/0183_ai_review_verdict_flips.sql | 15 ++ scripts/check-schema-drift.ts | 1 + src/db/repositories.ts | 52 ++++++ src/queue/processors.ts | 47 ++++- src/review/verdict-flip-guard.ts | 37 ++++ src/review/verdict-flip-store.ts | 51 ++++++ src/selfhost/pg-dialect.ts | 3 + test/unit/queue.test.ts | 111 ++++++++++++ test/unit/verdict-flip-guard.test.ts | 188 ++++++++++++++++++++ 9 files changed, 503 insertions(+), 2 deletions(-) create mode 100644 migrations/0183_ai_review_verdict_flips.sql create mode 100644 src/review/verdict-flip-guard.ts create mode 100644 src/review/verdict-flip-store.ts create mode 100644 test/unit/verdict-flip-guard.test.ts diff --git a/migrations/0183_ai_review_verdict_flips.sql b/migrations/0183_ai_review_verdict_flips.sql new file mode 100644 index 0000000000..812e59ccf6 --- /dev/null +++ b/migrations/0183_ai_review_verdict_flips.sql @@ -0,0 +1,15 @@ +-- #9016 (security): bound how many times an AI-review verdict can flip-flop for one PR before a human is +-- required. The AI reviewer is non-deterministic even at temperature 0; without a bound, a contributor can +-- force fresh re-rolls (a no-op recommit that invalidates the head-SHA cache key, or a same-head retry once +-- the non-cacheable cooldown lapses) until a lucky CLEAN roll auto-merges a PR other rolls flagged as +-- blocked. One row per (repo_full_name, pull_number) tracks the last FRESH (non-cache-hit) verdict's +-- defect/clean state and how many times it has flipped since; the escalation itself is computed in +-- src/review/verdict-flip-guard.ts (pure) and applied by the caller. +CREATE TABLE IF NOT EXISTS ai_review_verdict_flips ( + repo_full_name TEXT NOT NULL, + pull_number INTEGER NOT NULL, + last_had_defect INTEGER NOT NULL, -- 0 | 1 + flip_count INTEGER NOT NULL DEFAULT 0, + updated_at TEXT NOT NULL, + PRIMARY KEY (repo_full_name, pull_number) +); diff --git a/scripts/check-schema-drift.ts b/scripts/check-schema-drift.ts index 5eca26852e..3c82f3bc09 100644 --- a/scripts/check-schema-drift.ts +++ b/scripts/check-schema-drift.ts @@ -45,6 +45,7 @@ export const RAW_SQL_ONLY_TABLES: Set = new Set([ "decision_ledger", "decision_records", "decision_replay_inputs", + "ai_review_verdict_flips", "global_agent_controls", "global_contributor_blacklist", "global_moderation_config", diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 6a7619ad71..069b98a09b 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -5100,6 +5100,58 @@ export async function getCachedAiReview( }; } +/** Bounds the fingerprint scan below to a handful of rows — a PR rarely accumulates more than a few AI + * review cache rows across its lifetime, and this is a fallback path, not the hot lookup. */ +const AI_REVIEW_FINGERPRINT_SCAN_LIMIT = 20; + +/** + * #9016 (security): a content-fingerprint-sticky FALLBACK for {@link getCachedAiReview}, used only when the + * exact head-SHA lookup misses. The head-SHA-keyed cache treats every new commit as an independent roll of + * the (non-deterministic) AI reviewer — so a contributor can force fresh re-rolls with a no-op recommit + * (an empty commit, a metadata-only push) until a lucky CLEAN roll auto-merges a PR another roll flagged as + * blocked. `expectedInputFingerprint` already hashes every prompt-shaping input including the actual per- + * file PATCH content (see ai-review-cache-input.ts's `reviewFiles`), so an IDENTICAL fingerprint under a + * DIFFERENT head SHA means the reviewed content is genuinely unchanged — this reuses that prior verdict + * instead of spending a fresh, independently-random roll on content the reviewer has already judged. + * Same recency/cacheable rules as getCachedAiReview: a durable (cacheable) row reuses unboundedly; a + * dynamic-context (non-cacheable) row only within `maxAgeMs` of being written, and never once published + * (a stale non-durable published verdict is not trustworthy indefinitely across heads either). + */ +export async function getCachedAiReviewAcrossHeads( + env: Env, + repoFullName: string, + pullNumber: number, + mode: string, + inputFingerprint: string, + options?: { maxAgeMs?: number } | undefined, +): Promise<{ notes: string; reviewerCount: number; findings: AdvisoryFinding[]; metadata?: Record | undefined } | null> { + const { results } = await env.DB + .prepare( + "SELECT notes, reviewer_count AS reviewerCount, findings_json AS findingsJson, metadata_json AS metadataJson, cacheable, published_at AS publishedAt, created_at AS createdAt FROM ai_review_cache WHERE repo_full_name = ? AND pull_number = ? AND ai_review_mode = ? ORDER BY created_at DESC LIMIT ?", + ) + .bind(repoFullName, pullNumber, mode, AI_REVIEW_FINGERPRINT_SCAN_LIMIT) + .all<{ notes: string; reviewerCount: number; findingsJson: string | null; metadataJson: string | null; cacheable: number; publishedAt: string | null; createdAt: string }>(); + for (const row of results ?? []) { + const metadata = parseJson>(row.metadataJson, {}); + if (metadata.inputFingerprint !== inputFingerprint) continue; + if (row.cacheable !== 1) { + if (row.publishedAt != null) continue; + const ageMs = Date.now() - Date.parse(row.createdAt); + if (!Number.isFinite(ageMs) || ageMs < 0 || ageMs > (options?.maxAgeMs ?? 0)) continue; + } + // Unlike getCachedAiReview above, `metadata` is NEVER empty here by construction: reaching this line + // already required `metadata.inputFingerprint === inputFingerprint` to pass, above — so no conditional + // spread is needed (there is no empty-metadata arm to guard). + return { + notes: row.notes, + reviewerCount: row.reviewerCount, + findings: parseJson(row.findingsJson, []), + metadata, + }; + } + return null; +} + /** #regate-churn (maintainer-gated freeze): the most recently PUBLISHED AI review for this PR, regardless of * which head SHA it was computed against. Used ONLY when the PR is currently held for manual review — a repeat * contributor push must not buy a fresh, real AI call (or a chance to flip the published verdict via plain LLM diff --git a/src/queue/processors.ts b/src/queue/processors.ts index ee54622ce1..d96a8d77d8 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -41,6 +41,7 @@ import { markRepositoriesRemovedFromInstallation, persistAdvisory, getCachedAiReview, + getCachedAiReviewAcrossHeads, getLatestPublishedAiReview, countPublishedAiReviewHeads, putCachedAiReview, @@ -648,6 +649,7 @@ import { import { AI_JUDGMENT_BLOCKER_CODES } from "../rules/advisory"; import { computeSalvageabilityForTarget } from "../review/salvageability-wire"; import { deriveDecisionReasonCode, persistDecisionReplayInputForGate } from "../review/decision-replay"; +import { recordVerdictFlip } from "../review/verdict-flip-store"; import { REVIEW_PROMPT_VERSION, REVIEW_SYSTEM_PROMPT } from "../services/ai-review"; import { resolveAutomaticCloseConfidence } from "../review/risk-control-wire"; import { maybeApplyCloseAuditHoldout } from "../review/close-audit-holdout"; @@ -10381,7 +10383,7 @@ async function maybePublishPrPublicSurface( // (unbounded reuse, exactly as before this fix). const cachedReview = webhook.forceAiReview === true ? null - : await getCachedAiReview( + : (await getCachedAiReview( env, repoFullName, pr.number, @@ -10389,7 +10391,20 @@ async function maybePublishPrPublicSurface( settings.aiReviewMode, inputFingerprint, { allowNonCacheable: true, maxAgeMs: AI_REVIEW_NON_CACHEABLE_RETRY_COOLDOWN_MS }, - ).catch(() => null); + ).catch(() => null)) ?? + // #9016 (security): the exact-head lookup just missed. Before spending a fresh, independently + // random LLM roll, check whether this PR already has a verdict computed against IDENTICAL + // review content (the fingerprint already hashes the actual patch content, not just the head + // SHA) under a DIFFERENT head — closes the no-op-recommit reroll exploit without weakening the + // cache's normal same-head behavior at all. + (await getCachedAiReviewAcrossHeads( + env, + repoFullName, + pr.number, + settings.aiReviewMode, + inputFingerprint, + { maxAgeMs: AI_REVIEW_NON_CACHEABLE_RETRY_COOLDOWN_MS }, + ).catch(() => null)); if (cachedReview && hasPublicReviewAssessment(cachedReview.notes)) { advisory.findings.push(...cachedReview.findings); aiReview = cachedReview; @@ -10475,6 +10490,34 @@ async function maybePublishPrPublicSurface( deliveryId: webhook.deliveryId, preComputedReputationSkip, }); + // #9016 (security): a FRESH verdict (this branch only runs on a genuine cache miss — never for a + // reused cache hit, which is not a new independent roll) is checked against the PR's flip history. + // The AI reviewer is non-deterministic, so a contributor can otherwise force re-rolls (a no-op + // recommit, or a same-head retry after the non-cacheable cooldown lapses) until a lucky CLEAN roll + // auto-merges a PR another roll flagged as blocked. Scoped to block mode only — advisory mode never + // 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 ?? []); + if (verdictFlip.escalate) { + advisory.findings.push({ + code: "ai_review_inconclusive", + severity: "warning", + title: "AI review verdict has flip-flopped too many times", + detail: `This PR's AI review result has changed direction ${verdictFlip.flipCount} times across recent re-reviews of the same or similar content. Repeated re-rolls of a non-deterministic reviewer are held for a human instead of trusting the newest roll.`, + action: "A maintainer should review this PR directly, or push a substantive fix so the next review reflects real content change.", + }); + incr("loopover_ai_review_verdict_flip_escalated_total"); + await recordAuditEvent(env, { + eventType: "github_app.ai_review_verdict_flip_escalated", + actor: author, + targetKey: `${repoFullName}#${pr.number}`, + outcome: "completed", + detail: `verdict flipped ${verdictFlip.flipCount} times; held for human review`, + metadata: { deliveryId: webhook.deliveryId, repoFullName, headSha: advisory.headSha ?? null, flipCount: verdictFlip.flipCount }, + }).catch(() => undefined); + } + } // `persistable === false` (only the lock-contention placeholder — see runAiReviewForAdvisory's return // type doc comment) is excluded from EVERY write, not just the durable one: it describes a transient // scheduling race, not a real AI opinion, and the concurrent pass it deferred to persists the real diff --git a/src/review/verdict-flip-guard.ts b/src/review/verdict-flip-guard.ts new file mode 100644 index 0000000000..11f2488347 --- /dev/null +++ b/src/review/verdict-flip-guard.ts @@ -0,0 +1,37 @@ +// AI-review verdict-flip escalation (#9016, security) — PURE state machine. +// +// The AI reviewer is non-deterministic even at temperature 0. Without a bound, a contributor can force +// fresh re-rolls (a no-op recommit that invalidates the head-SHA cache key, or a same-head retry once the +// non-cacheable-result cooldown lapses) until a lucky CLEAN roll auto-merges a PR other rolls flagged as +// blocked — verdict-shopping against a randomized judge. This tracks the last FRESH (non-cache-hit) +// verdict's defect/clean state per PR and counts how many times it has FLIPPED; once flips clear a +// threshold, the caller holds the gate for a human instead of trusting the newest roll. +import { AI_JUDGMENT_BLOCKER_CODES } from "../rules/advisory"; + +/** Flips this large signal a genuine, sustained oscillation rather than reviewer noise on the first retry + * — the exploit needs several rolls to land a lucky one, so the bound should not fire on ordinary review + * 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 VerdictFlipResult = VerdictFlipState & { escalate: boolean }; + +/** + * Advance the flip state with one FRESH verdict (never call this for a cache-hit reuse of a prior verdict — + * reusing a result is not a new independent roll). PURE. A first observation for a PR never escalates + * (there is nothing to flip against yet). A flip is a CHANGE from the immediately prior fresh verdict; + * repeating the same verdict does not add to the count — a PR that is consistently blocked, or + * 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 }; + const flipCount = prior.lastHadDefect === hadDefect ? prior.flipCount : prior.flipCount + 1; + return { lastHadDefect: hadDefect, flipCount, escalate: flipCount >= VERDICT_FLIP_ESCALATION_THRESHOLD }; +} + +/** Whether a fresh review's findings carry a blocking AI-judgment defect (ai_consensus_defect / + * ai_review_split) — the boolean this guard tracks flips of. PURE. */ +export function findingsHadAiDefect(findings: ReadonlyArray<{ code: string }>): boolean { + return findings.some((finding) => AI_JUDGMENT_BLOCKER_CODES.has(finding.code)); +} diff --git a/src/review/verdict-flip-store.ts b/src/review/verdict-flip-store.ts new file mode 100644 index 0000000000..c1051599cd --- /dev/null +++ b/src/review/verdict-flip-store.ts @@ -0,0 +1,51 @@ +// Verdict-flip persistence (#9016, security) — the IO around src/review/verdict-flip-guard.ts's pure state +// machine. One row per PR (migration 0182); fail-open throughout — an infra blip must degrade to "never +// escalate," never block or unblock the gate on its own. +import { findingsHadAiDefect, nextVerdictFlipState, type VerdictFlipResult, type VerdictFlipState } from "./verdict-flip-guard"; +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 = ?", + ) + .bind(repoFullName, pullNumber) + .first<{ lastHadDefect: number; flipCount: number }>(); + if (!row) return null; + return { lastHadDefect: row.lastHadDefect === 1, flipCount: row.flipCount }; + } catch (error) { + console.warn(JSON.stringify({ event: "verdict_flip_read_error", repoFullName, pullNumber, message: errorMessage(error).slice(0, 120) })); + return null; + } +} + +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`, + ) + .bind(repoFullName, pullNumber, next.lastHadDefect ? 1 : 0, next.flipCount, nowIso()) + .run(); + } catch (error) { + console.warn(JSON.stringify({ event: "verdict_flip_write_error", repoFullName, pullNumber, message: errorMessage(error).slice(0, 120) })); + } +} + +/** + * Record one FRESH AI-review verdict (never call for a cache-hit reuse — see verdict-flip-guard's doc + * comment) and return the advanced flip state. Fail-open: any read/write error degrades to treating this + * as a first observation (flipCount 0, never escalates) — an infra blip must never itself force a hold. + */ +export async function recordVerdictFlip( + env: Env, + repoFullName: string, + pullNumber: number, + findings: ReadonlyArray<{ code: string }>, +): Promise { + const hadDefect = findingsHadAiDefect(findings); + const prior = await readVerdictFlipState(env, repoFullName, pullNumber); + const next = nextVerdictFlipState(prior, hadDefect); + await writeVerdictFlipState(env, repoFullName, pullNumber, next); + return next; +} diff --git a/src/selfhost/pg-dialect.ts b/src/selfhost/pg-dialect.ts index 07b0686f34..7852792b2b 100644 --- a/src/selfhost/pg-dialect.ts +++ b/src/selfhost/pg-dialect.ts @@ -15,6 +15,9 @@ const REPLACE_CONFLICT_KEYS: Record = { // #8893: orb_reuse_counters (migrations/0177) is written with INSERT OR REPLACE by src/orb/ingest.ts; its // PRIMARY KEY (instance_id, day) is the conflict target the hourly ORB export needs on self-host Postgres. orb_reuse_counters: ["instance_id", "day"], + // #9016: ai_review_verdict_flips (migrations/0183) is written with ON CONFLICT by recordVerdictFlip; + // its PRIMARY KEY (repo_full_name, pull_number) is the conflict target on self-host Postgres. + ai_review_verdict_flips: ["repo_full_name", "pull_number"], // ams_signals (#8382): TWO columns here, deliberately — this must name the table's REAL unique constraint // (`UNIQUE (instance_id, pr_hash)`, migrations/0148_ams_signals.sql), or Postgres rejects the generated // `ON CONFLICT` with "no unique or exclusion constraint matching". The 3-column shape orb_signals needed diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 002e502e70..c8139ffe27 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -12,6 +12,7 @@ import * as repositorySettingsModule from "../../src/settings/repository-setting import { counterValue, renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; import { REQUIRED_CONTEXTS_UNRESOLVED_METRIC } from "../../src/queue/ci-resolution"; import { jobCoalesceKey } from "../../src/selfhost/queue-common"; +import { VERDICT_FLIP_ESCALATION_THRESHOLD } from "../../src/review/verdict-flip-guard"; import { listCollisionEdges, createAgentRun, @@ -6716,8 +6717,14 @@ describe("queue processors", () => { // A genuinely NEW commit -- same AI verdict (still "Looks fine", so the gate conclusion is IDENTICAL to // the first pass), but a DIFFERENT head SHA, which has no prior recorded gate-check row of its own. + // #9016: resolvePullRequestFilesForReview reads STORED pull_request_files first and only falls back to + // a live GitHub fetch when storage is empty, so a genuinely new commit's real content must be reflected + // in storage here (as a real webhook-driven detail-sync would have already done) -- otherwise this is + // indistinguishable from the no-op-recommit case #9016's content-fingerprint cache fallback is DESIGNED + // to reuse, and the assertions below would be testing the wrong thing. headSha = "c75b"; await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 75, title: "Fix the retry loop", state: "open", user: { login: "contributor" }, head: { sha: headSha }, labels: [], body: "Closes #1" }); + await upsertPullRequestFile(env, { repoFullName: "JSONbored/gittensory", pullNumber: 75, path: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, payload: { patch: "@@\n+export const ok = false;" } }); await processJob(env, { type: "agent-regate-pr", deliveryId: "regate-sweep:JSONbored/gittensory#75:1", repoFullName: "JSONbored/gittensory", prNumber: 75, installationId: 123 }); expect(aiCalls).toBeGreaterThan(aiCallsAfterFirst); // the new head SHA is never cache-reused against the old one @@ -6732,6 +6739,110 @@ describe("queue processors", () => { expect(noopAudit?.n).toBe(0); }); + it("#9016 (security): a flip-count already at the escalation threshold forces a HOLD on the next verdict change, instead of trusting a lucky clean roll", async () => { + // Simulates the tail of verdict-shopping: the PR has already flip-flopped up to the escalation + // threshold (pre-seeded here — the per-flip mechanics themselves are unit-tested exhaustively in + // verdict-flip-guard.test.ts's nextVerdictFlipState suite). This is the end-to-end proof that the + // wiring actually reaches the gate: the NEXT fresh, genuinely-different-content roll comes back CLEAN + // (no blockers at all) and would otherwise auto-pass -- but crossing the threshold instead forces + // ai_review_inconclusive onto the advisory, holding the gate for a human. + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => ({ response: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [] }) }) } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await seedRegateChurnRepo(env, { publicSurface: "comment_and_label" }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_and_label", checkRunMode: "off", reviewCheckMode: "required", aiReviewMode: "block" }, review: { auto_review: { cadence: "continuous" } } }); + const headSha = "flipfinal"; + 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(); + + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes(`/pulls/77/files`)) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const roll = final;" }]); + if (url.endsWith("/pulls/77")) return Response.json({ number: 77, title: "Flip-flop PR", state: "open", user: { login: "contributor" }, head: { sha: headSha }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes(`/commits/${headSha}/check-runs`)) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes(`/commits/${headSha}/status`)) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/77/comments")) return Response.json(method === "GET" ? [] : { id: 1 }, { status: method === "GET" ? 200 : 201 }); + if (url.includes("/issues/comments/1")) return Response.json({ id: 1 }, { status: 200 }); + if (url.includes("/issues/77/labels") && method === "GET") return Response.json([{ name: "gittensor" }, { name: "gittensor:bug" }]); + if (url.includes("/issues/77/labels") && method === "POST") return Response.json([]); + if (url.includes("/check-runs") && (method === "POST" || method === "PATCH")) return Response.json({ id: 601, html_url: "https://github.com/checks/601" }, { status: method === "POST" ? 201 : 200 }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + 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") + .bind("JSONbored/gittensory", 77) + .first<{ blocker_codes_json: string }>(); + expect(finalBlocker?.blocker_codes_json ?? "").not.toContain("ai_consensus_defect"); // the fresh roll really was clean + const escalationAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and target_key = ?") + .bind("github_app.ai_review_verdict_flip_escalated", "JSONbored/gittensory#77") + .first<{ n: number }>(); + expect(escalationAudit?.n).toBe(1); + expect(counterValue("loopover_ai_review_verdict_flip_escalated_total")).toBeGreaterThanOrEqual(1); + const pr77 = await getPullRequest(env, "JSONbored/gittensory", 77); + expect(pr77?.state).toBe("open"); // never auto-merged on the exploited clean roll + const flipRow = 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 = ?") + .bind("JSONbored/gittensory", 77) + .first<{ lastHadDefect: number; flipCount: number }>(); + expect(flipRow).toMatchObject({ lastHadDefect: 0, flipCount: VERDICT_FLIP_ESCALATION_THRESHOLD + 1 }); + }); + + it("#9016: advisory-mode reviews never touch the flip-escalation wiring at all (nothing to shop for when AI never gates)", async () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => ({ response: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [] }) }) } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await seedRegateChurnRepo(env, { publicSurface: "comment_and_label" }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_and_label", checkRunMode: "off", reviewCheckMode: "required", aiReviewMode: "advisory" }, review: { auto_review: { cadence: "continuous" } } }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 78, title: "Advisory PR", state: "open", user: { login: "contributor" }, head: { sha: "adv78" }, labels: [], body: "Closes #1" }); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 78, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + await upsertPullRequestFile(env, { repoFullName: "JSONbored/gittensory", pullNumber: 78, path: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, payload: { patch: "@@\n+export const ok = true;" } }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes(`/pulls/78/files`)) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.endsWith("/pulls/78")) return Response.json({ number: 78, title: "Advisory PR", state: "open", user: { login: "contributor" }, head: { sha: "adv78" }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/adv78/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/adv78/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/78/comments")) return Response.json(method === "GET" ? [] : { id: 1 }, { status: method === "GET" ? 200 : 201 }); + if (url.includes("/issues/comments/1")) return Response.json({ id: 1 }, { status: 200 }); + if (url.includes("/issues/78/labels") && method === "GET") return Response.json([{ name: "gittensor" }]); + if (url.includes("/issues/78/labels") && method === "POST") return Response.json([]); + if (url.includes("/check-runs") && (method === "POST" || method === "PATCH")) return Response.json({ id: 601, html_url: "https://github.com/checks/601" }, { status: method === "POST" ? 201 : 200 }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "advisory-78", repoFullName: "JSONbored/gittensory", prNumber: 78, installationId: 123 }); + + const flipRow = await env.DB.prepare("select count(*) as n from ai_review_verdict_flips where repo_full_name = ? and pull_number = ?") + .bind("JSONbored/gittensory", 78) + .first<{ n: number }>(); + expect(flipRow?.n).toBe(0); // advisory mode never records a flip-state row at all + }); + it("REGRESSION (#6685): a draft PR's repeat fork-CI-completion triggers stop republishing once the head is already current", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); // reviewCheckMode: "disabled" reproduces the live incident exactly -- gateEnabled (shouldPublishReviewCheck diff --git a/test/unit/verdict-flip-guard.test.ts b/test/unit/verdict-flip-guard.test.ts new file mode 100644 index 0000000000..c323e540f0 --- /dev/null +++ b/test/unit/verdict-flip-guard.test.ts @@ -0,0 +1,188 @@ +import { describe, expect, it, vi } from "vitest"; +import { findingsHadAiDefect, nextVerdictFlipState, VERDICT_FLIP_ESCALATION_THRESHOLD } 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"; + +// #9016 (security): the AI reviewer is non-deterministic, so a contributor can otherwise force fresh +// re-rolls (a no-op recommit, or a same-head retry after the non-cacheable cooldown) until a lucky CLEAN +// roll auto-merges a PR another roll flagged as blocked. Two independent defenses: content-fingerprint +// stickiness across head SHAs (getCachedAiReviewAcrossHeads), and a per-PR verdict-flip counter that +// escalates to a human hold after repeated oscillation (verdict-flip-guard + verdict-flip-store). + +describe("findingsHadAiDefect", () => { + it("true only for AI-judgment codes (ai_consensus_defect / ai_review_split), never other blockers", () => { + expect(findingsHadAiDefect([{ code: "ai_consensus_defect" }])).toBe(true); + expect(findingsHadAiDefect([{ code: "ai_review_split" }])).toBe(true); + expect(findingsHadAiDefect([{ code: "secret_leak" }])).toBe(false); + expect(findingsHadAiDefect([])).toBe(false); + }); +}); + +describe("nextVerdictFlipState", () => { + 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 }); + }); + + 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 }); + }); + + 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 }); + }); + + 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 }); + }); + + it("simulates the exploit: defect, clean, defect, clean — escalates on the Nth flip, not before", () => { + let state: { lastHadDefect: boolean; flipCount: number } | 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); + 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]); + }); +}); + +describe("verdict-flip store (D1, fail-open)", () => { + it("readVerdictFlipState: null on no row, and on a DB error (never throws)", async () => { + const env = createTestEnv(); + expect(await readVerdictFlipState(env, "o/r", 1)).toBeNull(); + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + const broken = createTestEnv(); + vi.spyOn(broken.DB, "prepare").mockImplementation(() => { + throw new Error("db down"); + }); + expect(await readVerdictFlipState(broken, "o/r", 1)).toBeNull(); + expect(warn).toHaveBeenCalled(); + vi.restoreAllMocks(); + }); + + it("recordVerdictFlip: persists across calls, escalates at the threshold, and a write error still returns the computed (unpersisted) state", async () => { + const env = createTestEnv(); + const defect = [{ code: "ai_consensus_defect", title: "d", severity: "critical", detail: "d" }]; + const clean: typeof defect = []; + // Roll 1: defect (first observation, no escalate). + expect(await recordVerdictFlip(env, "o/r", 5, defect)).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 }); + // 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", 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); + vi.spyOn(writeBroken.DB, "prepare").mockImplementation(() => { + throw new Error("db down"); + }); + const result = await recordVerdictFlip(writeBroken, "o/r", 6, clean); + expect(result).toMatchObject({ flipCount: 0 }); // read failed (mocked) -> treated as first observation + expect(warn).toHaveBeenCalled(); + vi.restoreAllMocks(); + }); +}); + +describe("getCachedAiReviewAcrossHeads (#9016)", () => { + const write = (env: Env, headSha: string, cacheable: boolean, fingerprint: string) => + putCachedAiReview(env, "o/r", 7, headSha, "block", { + notes: "review", + reviewerCount: 2, + findings: cacheable ? [] : [{ code: "ai_consensus_defect", title: "d", severity: "critical", detail: "d" }], + cacheable, + metadata: { inputFingerprint: fingerprint }, + }); + + it("a no-op recommit (new head SHA, IDENTICAL fingerprint) reuses the prior CACHEABLE verdict", async () => { + const env = createTestEnv(); + await write(env, "sha-old", true, "fp-A"); + const hit = await getCachedAiReviewAcrossHeads(env, "o/r", 7, "block", "fp-A"); + expect(hit).not.toBeNull(); + expect(hit!.notes).toBe("review"); + }); + + it("a genuinely different fingerprint (real content change) is a miss — legitimate re-review is untouched", async () => { + const env = createTestEnv(); + await write(env, "sha-old", true, "fp-A"); + expect(await getCachedAiReviewAcrossHeads(env, "o/r", 7, "block", "fp-B")).toBeNull(); + }); + + it("a different repo/PR/mode never matches — scoped exactly like the exact-head lookup", async () => { + const env = createTestEnv(); + await write(env, "sha-old", true, "fp-A"); + expect(await getCachedAiReviewAcrossHeads(env, "o/r", 8, "block", "fp-A")).toBeNull(); + expect(await getCachedAiReviewAcrossHeads(env, "other/repo", 7, "block", "fp-A")).toBeNull(); + expect(await getCachedAiReviewAcrossHeads(env, "o/r", 7, "advisory", "fp-A")).toBeNull(); + }); + + it("a non-cacheable (dynamic-context) row matches only within maxAgeMs, and never once published", async () => { + const env = createTestEnv(); + await env.DB.prepare( + `INSERT INTO ai_review_cache (repo_full_name, pull_number, head_sha, ai_review_mode, notes, reviewer_count, findings_json, metadata_json, cacheable, published_at, created_at) + VALUES ('o/r', 7, 'sha-dyn', 'block', 'dyn review', 2, '[]', ?, 0, NULL, ?)`, + ) + .bind(JSON.stringify({ inputFingerprint: "fp-dyn" }), new Date(Date.now() - 5000).toISOString()) + .run(); + expect(await getCachedAiReviewAcrossHeads(env, "o/r", 7, "block", "fp-dyn", { maxAgeMs: 60_000 })).not.toBeNull(); + expect(await getCachedAiReviewAcrossHeads(env, "o/r", 7, "block", "fp-dyn", { maxAgeMs: 1 })).toBeNull(); + // Once published, a non-cacheable row is never reused across heads regardless of age. + await env.DB.prepare(`UPDATE ai_review_cache SET published_at = ? WHERE head_sha = 'sha-dyn'`).bind(new Date().toISOString()).run(); + expect(await getCachedAiReviewAcrossHeads(env, "o/r", 7, "block", "fp-dyn", { maxAgeMs: 60_000 })).toBeNull(); + }); + + it("degrades to a miss (never throws) when the query returns no results array, or a row carries an unparseable created_at", async () => { + const env = createTestEnv(); + vi.spyOn(env.DB, "prepare").mockReturnValueOnce({ + bind: () => ({ all: async () => ({}) }), // no `results` key at all + } as never); + expect(await getCachedAiReviewAcrossHeads(env, "o/r", 7, "block", "fp-A")).toBeNull(); + + await env.DB.prepare( + `INSERT INTO ai_review_cache (repo_full_name, pull_number, head_sha, ai_review_mode, notes, reviewer_count, findings_json, metadata_json, cacheable, published_at, created_at) + VALUES ('o/r', 7, 'sha-bad-date', 'block', 'dyn', 2, '[]', ?, 0, NULL, 'not-a-real-date')`, + ) + .bind(JSON.stringify({ inputFingerprint: "fp-bad" })) + .run(); + expect(await getCachedAiReviewAcrossHeads(env, "o/r", 7, "block", "fp-bad", { maxAgeMs: 60_000 })).toBeNull(); + vi.restoreAllMocks(); + }); + + it("a non-cacheable row with a FUTURE created_at (negative age) is a miss, and omitting options defaults maxAgeMs to 0 (always too old)", async () => { + const env = createTestEnv(); + await env.DB.prepare( + `INSERT INTO ai_review_cache (repo_full_name, pull_number, head_sha, ai_review_mode, notes, reviewer_count, findings_json, metadata_json, cacheable, published_at, created_at) + VALUES ('o/r', 7, 'sha-future', 'block', 'dyn', 2, '[]', ?, 0, NULL, ?)`, + ) + .bind(JSON.stringify({ inputFingerprint: "fp-future" }), new Date(Date.now() + 60_000).toISOString()) + .run(); + expect(await getCachedAiReviewAcrossHeads(env, "o/r", 7, "block", "fp-future", { maxAgeMs: 60_000 })).toBeNull(); + // No options at all: maxAgeMs defaults to 0, so any positive-age non-cacheable row is "too old". + await env.DB.prepare( + `INSERT INTO ai_review_cache (repo_full_name, pull_number, head_sha, ai_review_mode, notes, reviewer_count, findings_json, metadata_json, cacheable, published_at, created_at) + VALUES ('o/r', 7, 'sha-noopts', 'block', 'dyn', 2, '[]', ?, 0, NULL, ?)`, + ) + .bind(JSON.stringify({ inputFingerprint: "fp-noopts" }), new Date(Date.now() - 5).toISOString()) + .run(); + expect(await getCachedAiReviewAcrossHeads(env, "o/r", 7, "block", "fp-noopts")).toBeNull(); + }); + + it("returns the MOST RECENT matching row when several heads share a fingerprint", async () => { + const env = createTestEnv(); + await write(env, "sha-1", true, "fp-A"); + await new Promise((resolve) => setTimeout(resolve, 5)); + await write(env, "sha-2", true, "fp-A"); + const hit = await getCachedAiReviewAcrossHeads(env, "o/r", 7, "block", "fp-A"); + expect(hit).not.toBeNull(); + }); +}); From 7fc63c6a6918dfeca49386842dedf3e1cbde4a83 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 10:50:07 -0700 Subject: [PATCH 2/2] fix(selfhost): register the verdict-flip-escalation metric and fix a stale migration-number comment loopover_ai_review_verdict_flip_escalated_total was emitted (processors.ts) with no DEFAULT_METRIC_META entry, failing the completeness drift guard. Also corrects verdict-flip-store.ts's header comment, which still said "migration 0182" -- the ai_review_verdict_flips migration was bumped to 0183 after this branch rebased past #9090's 0181->0182 renumbering, but the comment was never updated. --- src/review/verdict-flip-store.ts | 2 +- src/selfhost/metrics.ts | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/review/verdict-flip-store.ts b/src/review/verdict-flip-store.ts index c1051599cd..a1add2831e 100644 --- a/src/review/verdict-flip-store.ts +++ b/src/review/verdict-flip-store.ts @@ -1,5 +1,5 @@ // Verdict-flip persistence (#9016, security) — the IO around src/review/verdict-flip-guard.ts's pure state -// machine. One row per PR (migration 0182); fail-open throughout — an infra blip must degrade to "never +// machine. One row per PR (migration 0183); fail-open throughout — an infra blip must degrade to "never // escalate," never block or unblock the gate on its own. import { findingsHadAiDefect, nextVerdictFlipState, type VerdictFlipResult, type VerdictFlipState } from "./verdict-flip-guard"; import { errorMessage, nowIso } from "../utils/json"; diff --git a/src/selfhost/metrics.ts b/src/selfhost/metrics.ts index d4258acd31..fca3d29bf5 100644 --- a/src/selfhost/metrics.ts +++ b/src/selfhost/metrics.ts @@ -153,6 +153,7 @@ export const DEFAULT_METRIC_META: readonly (readonly [string, MetricMeta])[] = [ ["loopover_ai_review_one_shot_reuse_total", { help: "AI review passes that reused a one-shot prior verdict instead of re-running.", type: "counter" }], ["loopover_ai_review_paused_reuse_total", { help: "AI review passes that reused a prior verdict because the repo is paused.", type: "counter" }], ["loopover_ai_review_tiebreak_order_unstable_total", { help: "Dual-reviewer tiebreak passes where reviewer order was not stable, by combine mode.", type: "counter" }], + ["loopover_ai_review_verdict_flip_escalated_total", { help: "AI review verdict-flip escalations (#9016): a PR's fresh AI-judgment verdict oscillated past the flip threshold and was held for a human instead of trusting the newest roll.", type: "counter" }], ["loopover_grounding_cache_hit_total", { help: "Review grounding-context cache hits.", type: "counter" }], ["loopover_grounding_cache_miss_total", { help: "Review grounding-context cache misses.", type: "counter" }], ["loopover_impact_map_cache_hit_total", { help: "Impact-map cache hits.", type: "counter" }],