diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 39ece37bd2..0bbfd994b5 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -38,23 +38,16 @@ export const AI_CONSENSUS_FLOOR = 0.9; const REVIEW_SYSTEM_PROMPT = [ "You are a senior open-source maintainer giving a THOROUGH code review of a single pull request diff.", - "Review like a careful human: read each meaningful hunk and give SPECIFIC, concrete feedback — correctness", - "bugs, logic errors, risky patterns, missing/incorrect error handling, unhandled edge cases, security issues,", - "performance problems, race conditions, and API/contract or backward-compat breaks. Reference the file (and the", - "function/line where you can) for every point. Do NOT rubber-stamp: if the diff is genuinely clean, state", - "specifically WHY it is safe (what you checked); otherwise give real, actionable findings.", - "Judge only the diff and the context provided. `assessment` must be a substantive walkthrough of the change", - "(several sentences — what it does, whether it is correct, and the notable details), NOT one generic line.", - "`suggestions` = concrete improvements (file-referenced); `risks` = things that could break or need a human's", - "attention. Aim for real depth over brevity.", - "Report a criticalDefect ONLY when you are highly confident the change introduces a real bug, a security", - "hole, data loss, or a build break — NOT for style, nits, naming, or merely-missing tests (those belong in", - "suggestions/risks and must NOT block).", - "Never mention rewards, rankings, payouts, wallets, hotkeys, coldkeys, trust scores, scoreability,", - "reviewability, or farming.", - 'Respond with ONLY a JSON object of this exact shape (no prose, no code fence):', - '{"assessment": string, "suggestions": string[], "risks": string[],', - ' "criticalDefect": {"present": boolean, "confidence": number, "title": string, "detail": string}}', + "Read each meaningful hunk and review like a careful human; judge ONLY the diff and the context provided.", + "Respond with ONLY a JSON object of this exact shape (no prose, no code fence):", + '{"assessment": string, "blockers": string[], "nits": string[], "suggestions": string[]}', + "- assessment: a SUBSTANTIVE walkthrough (several sentences) — what the change does, whether it is correct, and the notable details. Specific to THIS diff; NEVER a generic one-liner and never hedging ('appears to', 'seems to').", + "- blockers: each ONE sentence naming a CONCRETE must-fix defect IN THIS DIFF — a correctness/logic bug, a security hole, data loss, a build/test breakage, a race condition, or an API/contract/backward-compat break. Reference the file (and function/line). Empty [] if there are genuinely none.", + "- nits: each ONE sentence — a NON-blocking point: style, naming, a 'consider…', a missing doc/comment, an unhandled edge case worth noting, or a minor improvement. File-reference where you can.", + "- suggestions: concrete, file-referenced improvements (may overlap nits).", + "Do NOT rubber-stamp. If the diff is genuinely clean, the assessment must state SPECIFICALLY why it is safe (what you checked) and blockers must be []. Otherwise give real, specific findings — aim for depth, list every concern you actually see.", + "SEVERITY DISCIPLINE: a BLOCKER is a real defect you can point to in the diff; a NIT is style / preference / hypothetical / optional / docs. CI or check status ITSELF (failing, pending, unverified) is NOT a code defect — NEVER list it as a blocker or nit (the gate evaluates CI separately). Nits and hypotheticals are never blockers.", + "Never mention rewards, rankings, payouts, wallets, hotkeys, coldkeys, trust scores, scoreability, reviewability, or farming.", ].join(" "); /** A maintainer's BYOK provider credential, decrypted at call time. Never logged, never returned. */ @@ -107,9 +100,11 @@ export type GittensoryAiReviewResult = type ModelReview = { assessment: string; + // blockers = concrete must-fix defects in the diff (drive the consensus defect / gate); nits = non-blocking + // points; suggestions = concrete improvements (rendered alongside nits). reviewbot-parity shape. (#extensive-reviews) + blockers: string[]; + nits: string[]; suggestions: string[]; - risks: string[]; - criticalDefect: { present: boolean; confidence: number; title: string; detail: string }; }; type AiGatewayOptions = { gateway?: { id: string } }; @@ -191,23 +186,13 @@ export function parseModelReview(text: string): ModelReview | null { try { const obj = JSON.parse(match[0]) as Record; const toList = (value: unknown): string[] => - Array.isArray(value) ? value.filter((x): x is string => typeof x === "string").map((x) => x.trim()).filter(Boolean).slice(0, 6) : []; + Array.isArray(value) ? value.filter((x): x is string => typeof x === "string").map((x) => x.trim()).filter(Boolean).slice(0, 12) : []; const assessment = typeof obj.assessment === "string" ? obj.assessment.trim() : ""; - const defectRaw = obj.criticalDefect && typeof obj.criticalDefect === "object" ? (obj.criticalDefect as Record) : {}; - const present = defectRaw.present === true; - const confidence = typeof defectRaw.confidence === "number" ? Math.max(0, Math.min(1, defectRaw.confidence)) : 0; - if (!assessment && !present && !Array.isArray(obj.suggestions)) return null; - return { - assessment, - suggestions: toList(obj.suggestions), - risks: toList(obj.risks), - criticalDefect: { - present, - confidence, - title: typeof defectRaw.title === "string" ? defectRaw.title.trim().slice(0, 140) : "", - detail: typeof defectRaw.detail === "string" ? defectRaw.detail.trim().slice(0, 400) : "", - }, - }; + const blockers = toList(obj.blockers); + const nits = toList(obj.nits); + const suggestions = toList(obj.suggestions); + if (!assessment && blockers.length === 0 && nits.length === 0 && suggestions.length === 0) return null; + return { assessment, blockers, nits, suggestions }; } catch { return null; } @@ -348,35 +333,38 @@ async function runProviderReview(providerKey: AiReviewProviderKey, system: strin /** Compose a public-safe markdown advisory blurb from one or two model reviews. Null if nothing safe. */ export function composeAdvisoryNotes(reviews: ModelReview[]): string | null { const assessments = reviews.map((r) => r.assessment).filter(Boolean); - const suggestions = [...new Set(reviews.flatMap((r) => r.suggestions))].slice(0, 5); - const risks = [...new Set(reviews.flatMap((r) => r.risks))].slice(0, 4); + const blockers = [...new Set(reviews.flatMap((r) => r.blockers))].slice(0, 8); + // nits + suggestions are both non-blocking — merge + dedupe for the write-up. + const nits = [...new Set(reviews.flatMap((r) => [...r.nits, ...r.suggestions]))].slice(0, 12); const assessment = toPublicSafe(assessments[0] ?? ""); - const safeSuggestions = suggestions.map((s) => toPublicSafe(s)).filter((s): s is string => Boolean(s)); - const safeRisks = risks.map((s) => toPublicSafe(s)).filter((s): s is string => Boolean(s)); - if (!assessment && safeSuggestions.length === 0 && safeRisks.length === 0) return null; + const safeBlockers = blockers.map((s) => toPublicSafe(s)).filter((s): s is string => Boolean(s)); + const safeNits = nits.map((s) => toPublicSafe(s)).filter((s): s is string => Boolean(s)); + if (!assessment && safeBlockers.length === 0 && safeNits.length === 0) return null; const lines: string[] = []; if (assessment) lines.push(assessment, ""); - if (safeSuggestions.length > 0) { - lines.push("**Suggestions**"); - lines.push(...safeSuggestions.map((s) => `- ${s}`)); + if (safeBlockers.length > 0) { + lines.push("**Blockers**"); + lines.push(...safeBlockers.map((s) => `- ${s}`)); lines.push(""); } - if (safeRisks.length > 0) { - lines.push("**Risks**"); - lines.push(...safeRisks.map((s) => `- ${s}`)); + if (safeNits.length > 0) { + lines.push("**Nits**"); + lines.push(...safeNits.map((s) => `- ${s}`)); } // Reaching here means at least one section was pushed (the all-empty case returned null above). return lines.join("\n").trim(); } -/** True iff BOTH reviews independently report a critical defect at/above the floor. */ +/** A CONSENSUS defect = BOTH reviews independently name at least one concrete blocker (the severity-disciplined + * reviewbot model: a lone blocker in a dual review is a split, not a hard block). `floor` retained for the + * caller signature; the consensus here is blocker PRESENCE in both reviews (the prompt's rubric keeps nits out). */ export function consensusDefectOf(a: ModelReview, b: ModelReview, floor: number): AiConsensusDefect | null { - const both = a.criticalDefect.present && b.criticalDefect.present && a.criticalDefect.confidence >= floor && b.criticalDefect.confidence >= floor; - if (!both) return null; - const title = toPublicSafe(a.criticalDefect.title || b.criticalDefect.title || "AI reviewers agree on a likely critical defect"); - const detail = toPublicSafe(a.criticalDefect.detail || b.criticalDefect.detail); + void floor; + if (a.blockers.length === 0 || b.blockers.length === 0) return null; + const title = toPublicSafe(a.blockers[0] || b.blockers[0] || "AI reviewers agree on a likely blocking defect"); + const detail = toPublicSafe([...new Set([...a.blockers, ...b.blockers])].slice(0, 4).join("; ")); if (!title) return null; // unsafe title → drop the block entirely (fail-safe) - return { title, detail: detail ?? "Both AI reviewers independently flagged a high-confidence critical defect in this change.", confidence: Math.min(a.criticalDefect.confidence, b.criticalDefect.confidence) }; + return { title, detail: detail ?? "Both AI reviewers independently flagged a concrete must-fix defect in this change.", confidence: 1 }; } /** diff --git a/test/unit/ai-review-advisory.test.ts b/test/unit/ai-review-advisory.test.ts index 8018646f1c..68eddfd70a 100644 --- a/test/unit/ai-review-advisory.test.ts +++ b/test/unit/ai-review-advisory.test.ts @@ -49,10 +49,10 @@ function advisory(over: Partial = {}): Advisory { const pr = { number: 3, title: "Add helper", body: "Adds a helper." }; function defectJson() { - return JSON.stringify({ assessment: "Likely crash.", suggestions: ["Guard null."], risks: ["Null deref."], criticalDefect: { present: true, confidence: 0.97, title: "Null deref", detail: "Dereferences null." } }); + return JSON.stringify({ assessment: "Likely crash.", blockers: ["Null dereference of a possibly-null value in src/a.ts."], nits: ["Guard null."], suggestions: ["Guard null."] }); } function notesOnlyJson() { - return JSON.stringify({ assessment: "Looks fine.", suggestions: ["Add a test."], risks: [], criticalDefect: { present: false, confidence: 0, title: "", detail: "" } }); + return JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: ["Add a test."], suggestions: ["Add a test."] }); } function aiEnv(run: () => Promise, flags = true) { diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 40b63c634f..77ce9da920 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -10,12 +10,13 @@ import { createTestEnv } from "../helpers/d1"; const { parseModelReview, coerceAiText, composeAdvisoryNotes, consensusDefectOf, toPublicSafe, runWorkersOpinion } = __aiReviewInternals; -function reviewJson(over: Partial<{ assessment: string; suggestions: string[]; risks: string[]; present: boolean; confidence: number; title: string; detail: string }> = {}): string { +function reviewJson(over: Partial<{ assessment: string; suggestions: string[]; nits: string[]; blockers: string[]; present: boolean; confidence: number; title: string; detail: string }> = {}): string { return JSON.stringify({ assessment: over.assessment ?? "The change looks reasonable and focused.", + // `present`/`title` retained for call-site compat: a "present" critical defect maps to one blocker. + blockers: over.blockers ?? (over.present ? [over.title || over.detail || "Unhandled null dereference in src/a.ts."] : []), + nits: over.nits ?? ["Edge case on empty input is untested."], suggestions: over.suggestions ?? ["Add a unit test for the new branch."], - risks: over.risks ?? ["Edge case on empty input is untested."], - criticalDefect: { present: over.present ?? false, confidence: over.confidence ?? 0, title: over.title ?? "", detail: over.detail ?? "" }, }); } @@ -115,7 +116,7 @@ describe("runGittensoryAiReview advisory mode", () => { expect(result.status).toBe("ok"); if (result.status !== "ok") return; expect(result.consensusDefect).toBeNull(); - expect(result.advisoryNotes).toContain("Suggestions"); + expect(result.advisoryNotes).toContain("Nits"); expect(result.advisoryNotes).toContain("Add a unit test"); // Advisory mode runs a single opinion (primary model). expect(run).toHaveBeenCalledTimes(1); @@ -128,7 +129,7 @@ describe("runGittensoryAiReview block mode (consensus)", () => { return createTestEnv({ AI: { run: vi.fn(run) } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI_DAILY_NEURON_BUDGET: "100000" }); } - it("reports a consensus defect only when BOTH models agree at/above the floor", async () => { + it("reports a consensus defect only when BOTH models name a concrete blocker", async () => { const env = envWith(async () => ({ response: reviewJson({ present: true, confidence: 0.95, title: "Unhandled null", detail: "Crashes on empty list." }) })); const result = await runGittensoryAiReview(env, { ...baseInput, mode: "block" }); expect(result.status).toBe("ok"); @@ -147,8 +148,9 @@ describe("runGittensoryAiReview block mode (consensus)", () => { expect(result.status === "ok" && result.consensusDefect).toBeNull(); }); - it("does NOT report a defect when both agree but below the confidence floor", async () => { - const env = envWith(async () => ({ response: reviewJson({ present: true, confidence: 0.6, title: "Maybe bug", detail: "Unsure." }) })); + it("does NOT report a defect when both models flag only nits (no blocker)", async () => { + // Severity discipline: nits never block. Both reviewers return nits but zero blockers → no consensus defect. + const env = envWith(async () => ({ response: reviewJson({ present: false, nits: ["Consider renaming the helper."] }) })); const result = await runGittensoryAiReview(env, { ...baseInput, mode: "block" }); expect(result.status === "ok" && result.consensusDefect).toBeNull(); }); @@ -267,40 +269,41 @@ describe("pure helpers", () => { expect(coerceAiText(42)).toBe(""); }); - it("parseModelReview returns null on junk, on brace-but-invalid JSON, on empty objects, and clamps confidence", () => { + it("parseModelReview returns null on junk / invalid JSON / empty objects; parses blockers + nits", () => { expect(parseModelReview("not json")).toBeNull(); expect(parseModelReview("{ not: valid json }")).toBeNull(); // matches the brace regex but JSON.parse throws - expect(parseModelReview('{"foo":1}')).toBeNull(); // no assessment, no defect, no suggestions - const parsed = parseModelReview(reviewJson({ present: true, confidence: 5, title: "X", detail: "Y" })); - expect(parsed?.criticalDefect.confidence).toBe(1); + expect(parseModelReview('{"foo":1}')).toBeNull(); // no assessment, no blockers/nits/suggestions + const parsed = parseModelReview(reviewJson({ present: true, title: "Null deref in src/a.ts" })); + expect(parsed?.blockers).toContain("Null deref in src/a.ts"); }); it("parseModelReview coerces non-string/non-array fields to safe defaults", () => { - const parsed = parseModelReview('{"assessment":"ok","suggestions":"not-an-array","risks":7,"criticalDefect":{"present":true,"confidence":0.9,"title":5,"detail":null}}'); + const parsed = parseModelReview('{"assessment":"ok","suggestions":"not-an-array","blockers":7,"nits":null}'); expect(parsed).not.toBeNull(); expect(parsed?.suggestions).toEqual([]); // non-array → [] - expect(parsed?.risks).toEqual([]); - expect(parsed?.criticalDefect.title).toBe(""); // non-string → "" - expect(parsed?.criticalDefect.detail).toBe(""); + expect(parsed?.blockers).toEqual([]); + expect(parsed?.nits).toEqual([]); }); - it("consensusDefectOf requires both present and at/above the floor and drops unsafe titles", () => { - const defect = (present: boolean, confidence: number, title = "Null deref", detail = "boom") => ({ assessment: "", suggestions: [], risks: [], criticalDefect: { present, confidence, title, detail } }); - expect(consensusDefectOf(defect(true, 0.95), defect(true, 0.95), AI_CONSENSUS_FLOOR)).not.toBeNull(); - expect(consensusDefectOf(defect(true, 0.8), defect(true, 0.95), AI_CONSENSUS_FLOOR)).toBeNull(); - expect(consensusDefectOf(defect(false, 0.95), defect(true, 0.95), AI_CONSENSUS_FLOOR)).toBeNull(); // one not present - expect(consensusDefectOf(defect(true, 0.95, "Boost your reward payout"), defect(true, 0.95, "Boost your reward payout"), AI_CONSENSUS_FLOOR)).toBeNull(); + it("consensusDefectOf requires a concrete blocker in BOTH reviews and drops unsafe titles", () => { + const r = (blockers: string[]) => ({ assessment: "", suggestions: [], nits: [], blockers }); + expect(consensusDefectOf(r(["Null deref in src/a.ts"]), r(["Null deref in src/a.ts"]), AI_CONSENSUS_FLOOR)).not.toBeNull(); + expect(consensusDefectOf(r([]), r(["Null deref"]), AI_CONSENSUS_FLOOR)).toBeNull(); // one has no blocker → split, not consensus + expect(consensusDefectOf(r(["Null deref"]), r([]), AI_CONSENSUS_FLOOR)).toBeNull(); + expect(consensusDefectOf(r(["Boost your reward payout"]), r(["Boost your reward payout"]), AI_CONSENSUS_FLOOR)).toBeNull(); // unsafe → dropped }); - it("consensusDefectOf falls back to b's title and a default detail when a is blank", () => { - const a = { assessment: "", suggestions: [], risks: [], criticalDefect: { present: true, confidence: 0.95, title: "", detail: "" } }; - const b = { assessment: "", suggestions: [], risks: [], criticalDefect: { present: true, confidence: 0.93, title: "Race condition", detail: "" } }; - const out = consensusDefectOf(a, b, AI_CONSENSUS_FLOOR); - expect(out?.title).toBe("Race condition"); - expect(out?.detail).toMatch(/independently flagged/); - // both titles blank → default title string is used - const blank = { ...a, criticalDefect: { ...a.criticalDefect } }; - expect(consensusDefectOf(blank, { ...blank, criticalDefect: { ...blank.criticalDefect } }, AI_CONSENSUS_FLOOR)?.title).toContain("AI reviewers agree"); + it("consensusDefectOf falls back to b's blocker when a's is blank", () => { + const a = { assessment: "", suggestions: [], nits: [], blockers: [""] }; + const b = { assessment: "", suggestions: [], nits: [], blockers: ["Race condition in src/x.ts"] }; + expect(consensusDefectOf(a, b, AI_CONSENSUS_FLOOR)?.title).toBe("Race condition in src/x.ts"); + }); + + it("consensusDefectOf uses the default title + detail when BOTH reviewers' blockers are blank", () => { + const blank = { assessment: "", suggestions: [], nits: [], blockers: [""] }; + const out = consensusDefectOf(blank, { ...blank, blockers: [""] }, AI_CONSENSUS_FLOOR); + expect(out?.title).toContain("AI reviewers agree"); // both blockers[0] falsy → default title + expect(out?.detail).toContain("independently flagged"); // joined detail empty → default detail }); it("runWorkersOpinion returns null without a binding and handles a single-model (no distinct fallback) list", async () => { @@ -321,19 +324,33 @@ describe("pure helpers", () => { }); it("composeAdvisoryNotes returns null when nothing is public-safe", () => { - expect(composeAdvisoryNotes([{ assessment: "reward payout farming", suggestions: ["payout"], risks: ["reward"], criticalDefect: { present: false, confidence: 0, title: "", detail: "" } }])).toBeNull(); + expect(composeAdvisoryNotes([{ assessment: "reward payout farming", suggestions: ["payout"], nits: ["reward"], blockers: [] }])).toBeNull(); }); it("composeAdvisoryNotes renders only the sections that have public-safe content", () => { - const review = (over: Partial<{ assessment: string; suggestions: string[]; risks: string[] }>) => ({ assessment: over.assessment ?? "", suggestions: over.suggestions ?? [], risks: over.risks ?? [], criticalDefect: { present: false, confidence: 0, title: "", detail: "" } }); + const review = (over: Partial<{ assessment: string; suggestions: string[]; nits: string[]; blockers: string[] }>) => ({ assessment: over.assessment ?? "", suggestions: over.suggestions ?? [], nits: over.nits ?? [], blockers: over.blockers ?? [] }); const assessmentOnly = composeAdvisoryNotes([review({ assessment: "Looks good." })]); expect(assessmentOnly).toBe("Looks good."); - const suggestionsOnly = composeAdvisoryNotes([review({ suggestions: ["Add a test."] })]); - expect(suggestionsOnly).toContain("**Suggestions**"); - expect(suggestionsOnly).not.toContain("**Risks**"); - const risksOnly = composeAdvisoryNotes([review({ risks: ["Edge case."] })]); - expect(risksOnly).toContain("**Risks**"); - expect(risksOnly).not.toContain("**Suggestions**"); + const nitsOnly = composeAdvisoryNotes([review({ nits: ["Add a test."] })]); + expect(nitsOnly).toContain("**Nits**"); + expect(nitsOnly).not.toContain("**Blockers**"); + const blockersOnly = composeAdvisoryNotes([review({ blockers: ["Null deref in src/a.ts."] })]); + expect(blockersOnly).toContain("**Blockers**"); + expect(blockersOnly).not.toContain("**Nits**"); + }); + + it("composeAdvisoryNotes merges + dedupes blockers/nits across two reviewers and renders both sections", () => { + const a = { assessment: "Solid change.", suggestions: ["Add a test."], nits: ["Rename x."], blockers: ["Null deref in src/a.ts."] }; + const b = { assessment: "Second look.", suggestions: ["Add a test."], nits: ["Rename x.", "Tighten the type."], blockers: ["Null deref in src/a.ts.", "Off-by-one in the loop bound."] }; + const out = composeAdvisoryNotes([a, b]) ?? ""; + expect(out).toContain("Solid change."); // first reviewer's assessment wins + expect(out).toContain("**Blockers**"); + expect(out).toContain("Off-by-one in the loop bound."); + expect(out).toContain("**Nits**"); + expect(out).toContain("Tighten the type."); // nits + suggestions merged + // the shared blocker + the shared nit/suggestion each appear exactly once (dedupe across reviewers) + expect(out.match(/Null deref in src\/a\.ts\./g)?.length).toBe(1); + expect(out.match(/Rename x\./g)?.length).toBe(1); }); it("runGittensoryAiReview is disabled when neither flag is set", async () => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 786942c80b..1c065e7e90 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1497,9 +1497,9 @@ describe("queue processors", () => { it("hard-blocks a confirmed contributor on a dual-model AI consensus defect when aiReview: block is opted in", async () => { const defectJson = JSON.stringify({ assessment: "Introduces a likely crash.", + blockers: ["Unhandled null dereference on empty input in src/a.ts — the new branch dereferences a possibly-null value."], + nits: ["Guard the null case."], suggestions: ["Guard the null case."], - risks: ["Unhandled null on empty input."], - criticalDefect: { present: true, confidence: 0.96, title: "Unhandled null dereference", detail: "The new branch dereferences a possibly-null value." }, }); const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(),