fix(review): kill diff-only false-positives (source-first diff + brace-aware parse)

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<ReturnType<typeof listPullRequestFiles>>): 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,
+    })),
+  );
 }
 
 /**

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<number>();
+  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();
+}

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 `<think>` 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<string, unknown>;
+    const obj = JSON.parse(jsonText) as Record<string, unknown>;
     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() : "";

test/unit/ai-review-advisory.test.ts

@@ -13,18 +13,20 @@ function fileRecord(over: Partial<PullRequestFileRecord> & { 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");
   });
 });
 

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 <think> 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 = `<think>{"thought":"file a.ts looks fine, but b.ts has a leak","draft":{"x":1}}</think>\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();

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
+  });
+});