diff --git a/migrations/0078_ai_review_cache_findings.sql b/migrations/0078_ai_review_cache_findings.sql new file mode 100644 index 0000000000..46bfd6a28f --- /dev/null +++ b/migrations/0078_ai_review_cache_findings.sql @@ -0,0 +1,3 @@ +-- Preserve the gate-relevant AI verdict when reusing a cached review: the public notes alone are not enough +-- because block-mode consensus/split/inconclusive findings are advisory side effects that must be replayed. +ALTER TABLE ai_review_cache ADD COLUMN findings_json TEXT NOT NULL DEFAULT '[]'; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 72ba12e582..4bcc77b060 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -59,6 +59,7 @@ import { } from "./schema"; import type { Advisory, + AdvisoryFinding, AgentActionRecord, AgentActionStatus, AgentActionType, @@ -3217,14 +3218,14 @@ export async function getCachedAiReview( pullNumber: number, headSha: string | null | undefined, mode: string, -): Promise<{ notes: string; reviewerCount: number } | null> { +): Promise<{ notes: string; reviewerCount: number; findings: AdvisoryFinding[] } | null> { if (!headSha) return null; const row = await env.DB - .prepare("SELECT notes, reviewer_count AS reviewerCount, ai_review_mode AS mode FROM ai_review_cache WHERE repo_full_name = ? AND pull_number = ? AND head_sha = ?") + .prepare("SELECT notes, reviewer_count AS reviewerCount, ai_review_mode AS mode, findings_json AS findingsJson FROM ai_review_cache WHERE repo_full_name = ? AND pull_number = ? AND head_sha = ?") .bind(repoFullName, pullNumber, headSha) - .first<{ notes: string; reviewerCount: number; mode: string }>(); + .first<{ notes: string; reviewerCount: number; mode: string; findingsJson: string | null }>(); if (!row || row.mode !== mode) return null; - return { notes: row.notes, reviewerCount: row.reviewerCount }; + return { notes: row.notes, reviewerCount: row.reviewerCount, findings: parseJson(row.findingsJson, []) }; } /** Upsert the AI review for (repo, pull, head SHA). A nullish head SHA is a no-op. */ @@ -3234,17 +3235,17 @@ export async function putCachedAiReview( pullNumber: number, headSha: string | null | undefined, mode: string, - review: { notes: string; reviewerCount: number }, + review: { notes: string; reviewerCount: number; findings?: AdvisoryFinding[] }, ): Promise { if (!headSha) return; await env.DB .prepare( - `INSERT INTO ai_review_cache (repo_full_name, pull_number, head_sha, ai_review_mode, notes, reviewer_count) - VALUES (?, ?, ?, ?, ?, ?) + `INSERT INTO ai_review_cache (repo_full_name, pull_number, head_sha, ai_review_mode, notes, reviewer_count, findings_json) + VALUES (?, ?, ?, ?, ?, ?, ?) ON CONFLICT(repo_full_name, pull_number, head_sha) DO UPDATE SET - ai_review_mode = excluded.ai_review_mode, notes = excluded.notes, reviewer_count = excluded.reviewer_count, created_at = CURRENT_TIMESTAMP`, + ai_review_mode = excluded.ai_review_mode, notes = excluded.notes, reviewer_count = excluded.reviewer_count, findings_json = excluded.findings_json, created_at = CURRENT_TIMESTAMP`, ) - .bind(repoFullName, pullNumber, headSha, mode, review.notes, review.reviewerCount) + .bind(repoFullName, pullNumber, headSha, mode, review.notes, review.reviewerCount, jsonString(review.findings ?? [])) .run(); } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 943c9822de..fd81efb412 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -3505,7 +3505,12 @@ export async function runAiReviewForAdvisory( reviewInlineComments?: boolean | undefined; }, ): Promise< - | { notes: string; reviewerCount: number; inlineFindings: InlineFinding[] } + | { + notes: string; + reviewerCount: number; + inlineFindings: InlineFinding[]; + findings: AdvisoryFinding[]; + } | undefined > { const packAllowsAnyAuthorBlockingReview = @@ -3674,21 +3679,21 @@ export async function runAiReviewForAdvisory( ), }); if (result.status !== "ok") return undefined; + const findings: AdvisoryFinding[] = []; if (result.consensusDefect) { - const defect: AdvisoryFinding = { + findings.push({ code: "ai_consensus_defect", severity: "critical", title: `AI reviewers agree on a likely critical defect: ${result.consensusDefect.title}`, detail: result.consensusDefect.detail, action: "Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.", - }; - args.advisory.findings.push(defect); + }); } else if (result.split) { // The reviewers DISAGREED — exactly one flagged a blocking defect. reviewbot's quorum: ANY reviewer // rejection closes the PR, so a split is a HARD BLOCKER (advisory.ts gates `ai_review_split` like a // consensus defect → gate failure → close); the contributor resubmits a fresh PR. (#ai-review-split) - args.advisory.findings.push({ + findings.push({ code: "ai_review_split", severity: "critical", title: "An AI reviewer flagged a likely blocking defect", @@ -3700,7 +3705,7 @@ export async function runAiReviewForAdvisory( } else if (result.inconclusive) { // Fail-CLOSED (#ai-fail-closed): block-mode AI could not return a usable verdict. Hold the PR for a human // (an evaluation-blocker code → neutral gate) rather than letting it pass to auto-merge uncertified. - args.advisory.findings.push({ + findings.push({ code: "ai_review_inconclusive", severity: "warning", title: "AI review could not be completed", @@ -3710,11 +3715,13 @@ export async function runAiReviewForAdvisory( "The gate is held for a human reviewer rather than passed automatically; it re-evaluates on the next update.", }); } + args.advisory.findings.push(...findings); return result.advisoryNotes ? { notes: result.advisoryNotes, reviewerCount: result.reviewerCount, inlineFindings: result.inlineFindings, + findings, } : undefined; } catch (error) { @@ -4179,9 +4186,15 @@ async function maybePublishPrPublicSurface( let preflight!: ReturnType; let gateEvaluation: ReturnType | undefined; // inlineFindings is present ONLY on a FRESH review (cache miss) with inline comments enabled; the AI cache - // round-trips just notes + reviewerCount, so a cache hit carries no findings and never re-posts (#inline-comments). + // round-trips notes + reviewerCount + the gate findings (so a cache hit replays consensus/split/inconclusive + // blockers — see below), but NOT inlineFindings, so a cache hit never re-posts inline comments (#inline-comments). let aiReview: - | { notes: string; reviewerCount: number; inlineFindings?: InlineFinding[] } + | { + notes: string; + reviewerCount: number; + inlineFindings?: InlineFinding[]; + findings?: AdvisoryFinding[]; + } | undefined; let inlineCommentsEnabledForReview = false; let gateFinalized = false; @@ -4441,6 +4454,7 @@ async function maybePublishPrPublicSurface( settings.aiReviewMode, ).catch(() => null); if (cachedReview) { + advisory.findings.push(...cachedReview.findings); aiReview = cachedReview; } else { // `.gittensory.yml` review.profile + review.path_instructions + review.exclude_paths (#review-profile / diff --git a/test/unit/ai-review-advisory.test.ts b/test/unit/ai-review-advisory.test.ts index 65e7cd7b75..a3e09db93e 100644 --- a/test/unit/ai-review-advisory.test.ts +++ b/test/unit/ai-review-advisory.test.ts @@ -208,6 +208,25 @@ describe("runAiReviewForAdvisory", () => { expect(result?.notes).toBeDefined(); // the single parseable opinion still produces advisory notes }); + it("appends an ai_review_split finding (lone-blocker HOLD) when the two block-mode reviewers disagree", async () => { + const adv = advisory(); + // Both opinions parse, but only the FIRST reviewer names a blocker → consensus needs BOTH → no defect → split + // (reviewbot's quorum: a lone rejection holds the PR). The split finding must be both applied to the advisory + // AND round-tripped on the returned cache payload so a cache hit can replay this blocker (#ai-review-split). + const run = (async (model: string) => ({ response: model === BEST_REVIEW_MODELS[0] ? defectJson() : notesOnlyJson() })) as unknown as () => Promise; + const result = await runAiReviewForAdvisory(aiEnv(run), { + settings: { aiReviewMode: "block" } as RepositorySettings, + advisory: adv, + repoFullName: "acme/widgets", + pr, + author: "alice", + confirmedContributor: true, + }); + expect(adv.findings.map((f) => f.code)).toEqual(["ai_review_split"]); // applied to the advisory (gate blocker) + expect(result?.findings.map((f) => f.code)).toEqual(["ai_review_split"]); // returned for the AI cache to persist + expect(result?.notes).toBeDefined(); + }); + it("uses the caller's pre-resolved files (FIX B) instead of the stored read, so the model sees the real diff", async () => { // FIX B: the processor passes `files` (its resolvePullRequestFilesForReview output). With no rows ever // written to the test DB, a stored read would yield an EMPTY diff; passing files proves the model gets the diff --git a/test/unit/ai-review-cache.test.ts b/test/unit/ai-review-cache.test.ts index c8b7246731..b38264d8d8 100644 --- a/test/unit/ai-review-cache.test.ts +++ b/test/unit/ai-review-cache.test.ts @@ -14,7 +14,7 @@ describe("AI review cache (#1)", () => { it("reuses a stored review ONLY on the same (repo, pull, head SHA, mode)", async () => { const env = createTestEnv(); await putCachedAiReview(env, "o/r", 7, "sha1", "block", { notes: "the review", reviewerCount: 2 }); - expect(await getCachedAiReview(env, "o/r", 7, "sha1", "block")).toEqual({ notes: "the review", reviewerCount: 2 }); + expect(await getCachedAiReview(env, "o/r", 7, "sha1", "block")).toEqual({ notes: "the review", reviewerCount: 2, findings: [] }); expect(await getCachedAiReview(env, "o/r", 7, "sha1", "advisory")).toBeNull(); // mode changed → miss expect(await getCachedAiReview(env, "o/r", 7, "sha2", "block")).toBeNull(); // new head SHA → miss expect(await getCachedAiReview(env, "o/r", 8, "sha1", "block")).toBeNull(); // different PR → miss @@ -23,7 +23,15 @@ describe("AI review cache (#1)", () => { it("upserts — a re-run at the same key replaces the stored review (+ mode)", async () => { const env = createTestEnv(); await putCachedAiReview(env, "o/r", 7, "sha1", "advisory", { notes: "first", reviewerCount: 1 }); - await putCachedAiReview(env, "o/r", 7, "sha1", "block", { notes: "second", reviewerCount: 2 }); - expect(await getCachedAiReview(env, "o/r", 7, "sha1", "block")).toEqual({ notes: "second", reviewerCount: 2 }); + await putCachedAiReview(env, "o/r", 7, "sha1", "block", { + notes: "second", + reviewerCount: 2, + findings: [{ code: "ai_review_split", severity: "critical", title: "Split", detail: "One reviewer blocked." }], + }); + expect(await getCachedAiReview(env, "o/r", 7, "sha1", "block")).toEqual({ + notes: "second", + reviewerCount: 2, + findings: [{ code: "ai_review_split", severity: "critical", title: "Split", detail: "One reviewer blocked." }], + }); }); }); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 9006a1a418..2febaf7e17 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -735,11 +735,13 @@ describe("queue processors", () => { expect(mergeAudit?.n).toBe(0); }); - it("#1: the block-mode re-gate sweep reuses a cached AI review for the same head SHA — no AI call re-spent", async () => { + it("#1: the block-mode re-gate sweep replays cached AI findings before gate evaluation", async () => { let aiCalls = 0; const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), AI: { run: async () => { aiCalls += 1; return { response: JSON.stringify({ assessment: "Critical defect found.", blockers: ["x"], nits: [], suggestions: [] }) }; } } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", AI_DAILY_NEURON_BUDGET: "100000", }); await upsertInstallation(env, { action: "created", installation: { id: 9001, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] } }); @@ -748,7 +750,11 @@ describe("queue processors", () => { await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 7, title: "Stale PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, labels: [], body: "Closes #1" }); await upsertPullRequestFile(env, { repoFullName: "owner/agent-repo", pullNumber: 7, path: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, payload: { patch: "@@\n+export const ok = value.length;" } }); // Pre-seed the AI review for this exact head SHA + mode → the sweep's block-mode review must reuse it, not re-run. - await putCachedAiReview(env, "owner/agent-repo", 7, "a7", "block", { notes: "cached review", reviewerCount: 2 }); + await putCachedAiReview(env, "owner/agent-repo", 7, "a7", "block", { + notes: "cached review", + reviewerCount: 2, + findings: [{ code: "ai_consensus_defect", severity: "critical", title: "Cached defect", detail: "Cached critical defect." }], + }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { const url = input.toString(); if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); @@ -768,6 +774,10 @@ describe("queue processors", () => { await sweepAndDrainPerPr(env, "owner/agent-repo"); expect(aiCalls).toBe(0); // the cached AI review was reused — the LLM was never called for this head SHA + const blocker = 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("owner/agent-repo", 7).first<{ blocker_codes_json: string }>(); + expect(blocker?.blocker_codes_json).toContain("ai_consensus_defect"); + const mergeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = ? and detail like ?").bind("agent.action.merge", "%merged%").first<{ n: number }>(); + expect(mergeAudit?.n).toBe(0); }); it("posts the 🟪 reviewing placeholder before the AI review runs, then overwrites it with the verdict (#reviewing-placeholder)", async () => {