diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 2945090615..109ead0e81 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -153,6 +153,7 @@ import { unionScopedOverlapClusters, type ContributorProfile, } from "../signals/engine"; +import { buildUnifiedReviewDiff } from "../review/review-diff"; import { buildUnifiedCommentBody, isUnifiedReviewCommentEnabled } from "../review/unified-comment-bridge"; import { screenshotsAllowed } from "../review/visual-wire"; import { isVisualPath } from "../review/visual/paths"; @@ -1547,21 +1548,19 @@ async function resolvePullRequestFilesForReview( * huge PR cannot blow the model context or the neuron budget; each file's patch is taken from the raw * GitHub file payload when present. */ export function buildAiReviewDiff(files: Awaited>): string { - const MAX_DIFF_CHARS = 60000; - const parts: string[] = []; - let total = 0; - for (const file of files) { - const patch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; - const header = `### ${file.path}${file.status ? ` (${file.status})` : ""} +${file.additions}/-${file.deletions}`; - const block = patch ? `${header}\n${patch}` : header; - if (total + block.length > MAX_DIFF_CHARS) { - parts.push(`… diff truncated (${files.length} files total).`); - break; - } - parts.push(block); - total += block.length; - } - return parts.join("\n\n"); + // Source-first + hunk-aware + always-list-dropped-files (ported from reviewbot). The old blind 60k + // head-slice `break`-dropped whole files in stored order, so the file DEFINING a symbol could vanish + // while another referenced it → the model hallucinated "missing import / undefined symbol" (the #1528 + // class, which survived even with grounding on). (#accuracy-gap-1) + return buildUnifiedReviewDiff( + files.map((file) => ({ + path: file.path, + patch: typeof file.payload?.patch === "string" ? file.payload.patch : undefined, + status: file.status, + additions: file.additions, + deletions: file.deletions, + })), + ); } /** diff --git a/src/review/review-diff.ts b/src/review/review-diff.ts new file mode 100644 index 0000000000..e3feb5ac01 --- /dev/null +++ b/src/review/review-diff.ts @@ -0,0 +1,117 @@ +// Bounded, source-first, hunk-aware unified-diff builder for the AI reviewers. +// Ported from reviewbot (the source-of-truth engine, src/core/diff.ts). The previous gittensory builder +// was a blind head-slice that `break`-DROPPED whole files on overflow with no priority ordering — so on a +// multi-file PR the file that DEFINES a symbol could be dropped while another file references it, and the +// model then hallucinated "missing import / undefined symbol" (the metagraphed #1528 false-positive class, +// which survived even with full-file grounding on). This builder orders source-first, reduces oversized +// patches hunk-aware instead of dropping them, and always lists patch-less/over-budget files. (#accuracy-gap-1) + +/** Char budget of the diff fed to the review models. The 120B review models have ~128k-token context, so + * even a large PR fits in ONE coherent pass (accuracy over speed). Only a genuinely huge PR truncates — + * and then SOURCE survives via priority ordering. */ +export const DEFAULT_DIFF_BUDGET = 80_000; + +/** Review priority for diff ordering. When the budget is tight, SOURCE survives and + * lockfiles/generated/docs/tests are dropped first (least useful to a code reviewer). Lower = kept. */ +export function diffFilePriority(path: string): number { + if (/(^|\/)(package-lock\.json|pnpm-lock\.yaml|yarn\.lock|bun\.lockb|cargo\.lock|poetry\.lock|composer\.lock|go\.sum)$|\.(min\.(js|css)|map|snap)$/i.test(path)) return 4; + if (/(^|\/)(dist|build|out|coverage|vendor|node_modules)\//i.test(path)) return 4; + if (/\.(md|mdx|rst|txt|adoc)$/i.test(path)) return 2; + if (/\.(test|spec)\.[a-z0-9]+$|(^|\/)(__tests__|tests?)\//i.test(path)) return 1; + return 0; // source code +} + +/** Added (`+`) line count in a patch — the substantive-change signal (context/removed lines are noise). */ +export function addedLineCount(patch: string | undefined): number { + if (!patch) return 0; + let n = 0; + for (const line of patch.split("\n")) if (line.startsWith("+") && !line.startsWith("+++")) n += 1; + return n; +} + +/** Split a unified patch into hunks (each starting at an `@@` header); any preamble stays as hunk 0. */ +function splitHunks(patch: string): string[] { + const hunks: string[] = []; + let cur: string[] = []; + for (const line of patch.split("\n")) { + if (line.startsWith("@@") && cur.length > 0) { + hunks.push(cur.join("\n")); + cur = [line]; + } else { + cur.push(line); + } + } + if (cur.length > 0) hunks.push(cur.join("\n")); + return hunks; +} + +/** + * Fit a file's patch into `budget` chars by keeping the HIGHEST-SIGNAL hunks (most added lines) and + * dropping lower-signal ones — so when a big file must be cut, the reviewer keeps the added logic and + * loses boilerplate/context, instead of a blind head-slice that drops whatever is at the tail. Kept + * hunks are emitted in original order so the diff still reads top-to-bottom. + */ +export function keepHighSignalHunks(patch: string, budget: number): string { + if (budget <= 0) return "… (this file's diff truncated)"; + const hunks = splitHunks(patch); + if (hunks.length <= 1) { + return patch.length > budget ? `${patch.slice(0, budget)}\n… (this file's diff truncated)` : patch; + } + const ranked = hunks.map((h, i) => ({ i, len: h.length, sig: addedLineCount(h) })).sort((a, b) => b.sig - a.sig); + const keep = new Set(); + let used = 0; + for (const r of ranked) { + if (used + r.len + 1 > budget) continue; + keep.add(r.i); + used += r.len + 1; + } + const top = ranked[0]; + if (keep.size === 0 && top) keep.add(top.i); // always keep the single highest-signal hunk + const dropped = hunks.length - keep.size; + const kept = hunks.filter((_, i) => keep.has(i)).join("\n"); + return dropped > 0 ? `${kept}\n… (${dropped} lower-signal hunk(s) dropped)` : kept; +} + +/** A changed file, shape-agnostic so any caller's file record can map into it. The explicit `| undefined` + * unions let a caller pass through possibly-undefined fields under exactOptionalPropertyTypes. */ +export interface ReviewDiffFile { + path: string; + patch?: string | undefined; + status?: string | null | undefined; + additions?: number | undefined; + deletions?: number | undefined; +} + +/** + * Build a bounded unified-diff string from ALL changed files. Files are ordered by review priority + * (SOURCE first), then by added-line count, so if the budget is hit lockfiles/generated/docs/tests drop + * before source — the file defining a symbol is never silently dropped while another references it. + * Oversized files keep their highest-signal hunks (not a blind head-slice); patch-less files (binary / + * too large) are still listed with status + add/del counts so the change is never invisible. + */ +export function buildUnifiedReviewDiff(files: ReviewDiffFile[], budget: number = DEFAULT_DIFF_BUDGET): string { + const ordered = [...files].sort( + (a, b) => diffFilePriority(a.path) - diffFilePriority(b.path) || addedLineCount(b.patch) - addedLineCount(a.patch), + ); + let diff = ""; + for (const file of ordered) { + const status = file.status ?? "modified"; + const header = `### ${file.path} (${status}) +${file.additions ?? 0}/-${file.deletions ?? 0}\n`; + const remaining = budget - diff.length; + if (remaining < 240) { + diff += `### …diff truncated (${files.length} files total)\n`; + break; + } + if (!file.patch) { + diff += `${header}(no inline patch — binary or too large)\n\n`; + continue; + } + let body = file.patch; + if (header.length + body.length + 2 > remaining) { + // Hunk-aware: keep the highest-signal hunks that fit rather than a blind head-slice. + body = keepHighSignalHunks(file.patch, remaining - header.length - 4); + } + diff += `${header}${body}\n\n`; + } + return diff.trim(); +} diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index e50b640f23..2dce746407 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -182,12 +182,45 @@ export function coerceAiText(result: unknown): string { return ""; } +/** + * Extract the LAST complete top-level JSON object from text — brace-depth-aware + string-safe. + * The gpt-oss/nemotron reasoning models emit a `` scratchpad object BEFORE the real verdict; a + * greedy `/\{[\s\S]*\}/` spans first-`{` to last-`}` and swallows BOTH, corrupting the parse (silently + * dropping/garbling reviews). Ported from reviewbot (the source-of-truth engine). Returns null when there + * is no complete top-level object. (#accuracy-gap-3) + */ +export function extractLastJsonObject(text: string): string | null { + let depth = 0; + let start = -1; + let inStr = false; + let esc = false; + let last: string | null = null; + for (let i = 0; i < text.length; i += 1) { + const ch = text[i]; + if (inStr) { + if (esc) esc = false; + else if (ch === "\\") esc = true; + else if (ch === '"') inStr = false; + continue; + } + if (ch === '"') inStr = true; + else if (ch === "{") { + if (depth === 0) start = i; + depth += 1; + } else if (ch === "}" && depth > 0) { + depth -= 1; + if (depth === 0 && start >= 0) last = text.slice(start, i + 1); + } + } + return last; +} + /** Parse a model's JSON review into a normalized {@link ModelReview}, or null when unparseable. */ export function parseModelReview(text: string): ModelReview | null { - const match = text.replace(/^```(?:json)?\s*/i, "").replace(/```$/i, "").match(/\{[\s\S]*\}/); - if (!match) return null; + const jsonText = extractLastJsonObject(text); + if (!jsonText) return null; try { - const obj = JSON.parse(match[0]) as Record; + const obj = JSON.parse(jsonText) 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) : []; const assessment = typeof obj.assessment === "string" ? obj.assessment.trim() : ""; diff --git a/test/unit/ai-review-advisory.test.ts b/test/unit/ai-review-advisory.test.ts index 68eddfd70a..798bf7eac0 100644 --- a/test/unit/ai-review-advisory.test.ts +++ b/test/unit/ai-review-advisory.test.ts @@ -13,18 +13,20 @@ function fileRecord(over: Partial & { path: string }): Pu } describe("buildAiReviewDiff", () => { - it("includes patches and headers, omits the patch when absent, and truncates oversized diffs", () => { + it("includes patches and headers, lists a patch-less file, and truncates oversized diffs (source-first)", () => { const diff = buildAiReviewDiff([ fileRecord({ path: "src/a.ts", status: "modified", payload: { patch: "@@\n+const x = 1;" } }), fileRecord({ path: "src/b.ts", status: undefined, payload: {} }), ]); expect(diff).toContain("### src/a.ts (modified) +1/-0"); expect(diff).toContain("+const x = 1;"); - expect(diff).toContain("### src/b.ts +1/-0"); // no status, no patch + expect(diff).toContain("### src/b.ts (modified) +1/-0"); // status defaults to "modified" + expect(diff).toContain("no inline patch"); // patch-less file still listed, never invisible expect(buildAiReviewDiff([])).toBe(""); - const huge = buildAiReviewDiff([fileRecord({ path: "src/big.ts", payload: { patch: "x".repeat(70000) } }), fileRecord({ path: "src/next.ts" })]); - expect(huge).toContain("diff truncated"); + // Oversized patch beyond the 80k budget is truncated (per-file hunk-aware or top-level), never silently dropped. + const huge = buildAiReviewDiff([fileRecord({ path: "src/big.ts", payload: { patch: "x".repeat(90000) } }), fileRecord({ path: "src/next.ts" })]); + expect(huge).toContain("truncated"); }); }); diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 63d7b62164..86e1ddaa6b 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -285,6 +285,23 @@ describe("pure helpers", () => { expect(parsed?.nits).toEqual([]); }); + it("parseModelReview takes the LAST top-level object — a reasoning scratchpad object no longer corrupts the verdict (#accuracy-gap-3)", () => { + // gpt-oss/nemotron emit a scratchpad object BEFORE the verdict. The old greedy /\{[\s\S]*\}/ spanned + // first-{ to last-} and swallowed both → JSON.parse failed / garbled. The brace-aware extractor takes + // only the LAST complete top-level object (the real verdict). + const withScratchpad = `{"thought":"file a.ts looks fine, but b.ts has a leak","draft":{"x":1}}\n{"assessment":"leak in b.ts","blockers":["Unclosed handle in src/b.ts"],"nits":[],"suggestions":[]}`; + const parsed = parseModelReview(withScratchpad); + expect(parsed).not.toBeNull(); + expect(parsed?.assessment).toBe("leak in b.ts"); + expect(parsed?.blockers).toEqual(["Unclosed handle in src/b.ts"]); + }); + + it("parseModelReview parses a verdict wrapped in ```json fences without a regex strip (#accuracy-gap-3)", () => { + const fenced = '```json\n{"assessment":"ok","blockers":["X in src/a.ts"],"nits":[],"suggestions":[]}\n```'; + const parsed = parseModelReview(fenced); + expect(parsed?.blockers).toEqual(["X in src/a.ts"]); + }); + 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(); diff --git a/test/unit/review-diff.test.ts b/test/unit/review-diff.test.ts new file mode 100644 index 0000000000..86f79f91ff --- /dev/null +++ b/test/unit/review-diff.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from "vitest"; +import { addedLineCount, buildUnifiedReviewDiff, diffFilePriority, keepHighSignalHunks } from "../../src/review/review-diff"; + +describe("diffFilePriority — source survives, noise drops first", () => { + it("ranks source(0) < tests(1) < docs(2) < lockfiles/generated(4)", () => { + expect(diffFilePriority("src/a.ts")).toBe(0); + expect(diffFilePriority("src/a.test.ts")).toBe(1); + expect(diffFilePriority("README.md")).toBe(2); + expect(diffFilePriority("package-lock.json")).toBe(4); + expect(diffFilePriority("dist/bundle.js")).toBe(4); + expect(diffFilePriority("app.min.css")).toBe(4); + }); +}); + +describe("addedLineCount — counts +lines, ignores +++ header", () => { + it("counts only substantive added lines", () => { + expect(addedLineCount("@@\n+a\n+b\n-c\n d")).toBe(2); + expect(addedLineCount("+++ b/file.ts\n+real")).toBe(1); + expect(addedLineCount(undefined)).toBe(0); + }); +}); + +describe("buildUnifiedReviewDiff — the #1528 fix: never silently drop the file defining a symbol", () => { + it("orders SOURCE before a lockfile, so under a tight budget source survives and the lockfile drops", () => { + const bigLock = `@@\n${"+x\n".repeat(400)}`; // large, low-priority + const source = "@@\n+export function loadArtifactData() { return 1; }"; + const diff = buildUnifiedReviewDiff( + [ + { path: "package-lock.json", patch: bigLock, status: "modified", additions: 400, deletions: 0 }, + { path: "src/mcp-server.mjs", patch: source, status: "modified", additions: 1, deletions: 0 }, + ], + 300, // tight budget — only one file fits + ); + expect(diff).toContain("src/mcp-server.mjs"); // source kept + expect(diff).toContain("loadArtifactData"); // the symbol-defining hunk survives + expect(diff).toContain("…diff truncated"); // the lockfile was dropped, and that is announced + }); + + it("lists a patch-less (binary/too-large) file with its counts instead of making it invisible", () => { + const diff = buildUnifiedReviewDiff([{ path: "logo.png", patch: undefined, status: "added", additions: 0, deletions: 0 }]); + expect(diff).toContain("logo.png (added)"); + expect(diff).toContain("no inline patch"); + }); + + it("reduces an oversized single file hunk-aware (keeps the highest-signal hunk) rather than head-slicing", () => { + const lowSignal = `@@ -1,2 +1,2 @@\n context\n context`; + const highSignal = `@@ -10,1 +10,5 @@\n+critical1\n+critical2\n+critical3\n+critical4`; + const reduced = keepHighSignalHunks(`${lowSignal}\n${highSignal}`, 70); // room for the high-signal hunk only + expect(reduced).toContain("critical1"); // the high-signal hunk is kept + expect(reduced).not.toContain("context"); // the low-signal hunk is dropped + expect(reduced).toContain("dropped"); // and the drop is announced + }); +});