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