From 605e10a249d45a9e965f60e2fec51e7895ac2a64 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 23 Jun 2026 11:41:15 -0700 Subject: [PATCH] fix(review): guard against hallucinated missing-symbol blockers + collapse nits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Soak finding: the bot FALSE-CLOSED a good contributor PR (metagraphed #1528, green CI, mergeable) on a hallucinated blocker — it claimed loadArtifactData was 'not imported -> ReferenceError', but that function is defined locally and used by ~24 call sites. Root cause: the model sees only the DIFF, not the whole file, so new code referencing an existing (out-of-diff) symbol looks undefined; both free models made the same mistake -> consensus -> close. (Full-file grounding is enabled but did not prevent it, so a prompt-level guard is the reliable fix.) - DIFF SCOPE guard in the review prompt: never report a missing import / undefined symbol / 'X not defined -> ReferenceError' as a blocker unless the diff ITSELF removes the definition or introduces the symbol without defining it; otherwise it is at most a nit. Directly kills the false-close-on-hallucination class. - Nits now render inside a collapsed
toggle (assessment + blockers stay visible), per request — keeps the comment focused. --- src/services/ai-review.ts | 6 +++++- test/unit/ai-review.test.ts | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 64a354ff48..e50b640f23 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -48,6 +48,7 @@ const REVIEW_SYSTEM_PROMPT = [ "BE SELECTIVE — report only the findings that genuinely matter. List at MOST ~3 blockers and ~5 nits, keeping only the most important; prefer signal over volume and do NOT pad the lists.", "DEDUPLICATE — if the same kind of issue recurs across several functions or lines, report it ONCE and note it applies broadly; never repeat a near-identical finding per occurrence.", "SEVERITY DISCIPLINE — defensive or speculative hardening ('should handle X', 'consider validating', 'add error handling') is a NIT, not a blocker, UNLESS a real input WILL actually trigger the failure. CI or check status itself (failing, pending, unverified) is NOT a code defect — never list it (the gate evaluates CI separately).", + "DIFF SCOPE — the diff shows only CHANGED lines, NOT whole files. A function, variable, import, type, or symbol you do not SEE may already be defined or imported elsewhere in the same file/module. NEVER report a 'missing import', 'undefined/not-imported symbol', or 'X is not defined -> ReferenceError' as a blocker unless the diff ITSELF removes the definition or introduces the symbol without defining it anywhere shown. When you cannot confirm a symbol is missing from the visible diff, it is NOT a blocker — at most a nit ('verify X is imported/defined').", "Do NOT rubber-stamp: if the diff is genuinely clean, the assessment states specifically why and blockers is [].", "Never mention rewards, rankings, payouts, wallets, hotkeys, coldkeys, trust scores, scoreability, reviewability, or farming.", ].join(" "); @@ -352,8 +353,11 @@ export function composeAdvisoryNotes(reviews: ModelReview[]): string | null { lines.push(""); } if (safeNits.length > 0) { - lines.push("**Nits**"); + // Nits go inside a collapsed
toggle so the body stays focused on the assessment + blockers; + // the blank line after lets GitHub render the markdown list inside the dropdown. (#focused-reviews) + lines.push("
", `Nits (${safeNits.length})`, ""); lines.push(...safeNits.map((s) => `- ${s}`)); + lines.push("
"); } // Reaching here means at least one section was pushed (the all-empty case returned null above). return lines.join("\n").trim(); diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 77ce9da920..63d7b62164 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -332,11 +332,11 @@ describe("pure helpers", () => { const assessmentOnly = composeAdvisoryNotes([review({ assessment: "Looks good." })]); expect(assessmentOnly).toBe("Looks good."); const nitsOnly = composeAdvisoryNotes([review({ nits: ["Add a test."] })]); - expect(nitsOnly).toContain("**Nits**"); + 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**"); + expect(blockersOnly).not.toContain("Nits"); }); it("composeAdvisoryNotes merges + dedupes blockers/nits across two reviewers and renders both sections", () => { @@ -346,7 +346,7 @@ describe("pure helpers", () => { 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("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);