diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 61e4011ee..36f5d3ba4 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -2253,8 +2253,26 @@ export function composeAdvisoryNotes(reviews: ModelReview[], options?: { allowBa const safeNits = nits .map((s) => toPublicSafe(s, options)) .filter((s): s is string => Boolean(s)); - const publicAssessment = + let publicAssessment = assessment || fallbackPublicAssessment(safeBlockers, safeNits); + // The model DID review and found nothing blocking, but its entire narrative was withheld by the + // public-safety sanitizer (every sentence referenced non-public vocabulary -- routine for a PR that + // touches this project's own scoring/gate code, where "score"/"ranking" appear in any honest sentence + // about it). Returning null here misreported that as a PROVIDER failure: the caller published "AI review + // is unavailable for this PR head" and held the PR for manual review, permanently, on every re-run -- + // observed live on JSONbored/loopover#9794, which re-ran cleanly (diagnostics `claude-code#0:parsed`) + // and was re-held every time. Publish a fixed, honest sentence instead: it is public-safe by + // construction (no model text), tells the reader what actually happened, and lets the gate act on the + // review's real verdict. The genuinely-empty case (no parsed review content at all) still returns null + // below, so a true provider failure keeps its accurate "unavailable" report. + if (!publicAssessment && assessments.length > 0) { + // Wording must track the review's REAL verdict: with raw blockers present (all withheld above), claiming + // "no blocking issues" would be false and could green-light a PR the model actually flagged. + publicAssessment = + blockers.length > 0 + ? "The AI review completed and raised blocking findings, but they were withheld from this public surface because they referenced non-public project internals. A maintainer should read the private review record before deciding this PR." + : "The AI review completed and found no blocking issues. Its narrative summary was withheld from this public surface because it referenced non-public project internals."; + } if (!publicAssessment) return null; const lines: string[] = []; lines.push(publicAssessment, ""); diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index f9e31bff8..f9b834c7e 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -17,8 +17,7 @@ import { runLoopOverAiReview, type AiContentBlock, type AiReviewDiagnostic, - type LoopOverAiReviewInput, -} from "../../src/services/ai-review"; + type LoopOverAiReviewInput, hasPublicReviewAssessment } from "../../src/services/ai-review"; import { createTestEnv } from "../helpers/d1"; import { FILE_CONTENT_BUDGET } from "../../src/review/review-grounding"; import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; @@ -3993,19 +3992,21 @@ describe("pure helpers", () => { expect(result.status).toBe("ok"); }); - it("composeAdvisoryNotes returns null when no assessment or finding is public-safe", () => { - expect( - composeAdvisoryNotes([ - { - assessment: "reward payout farming", - suggestions: ["payout"], - nits: ["reward"], - blockers: [], - inlineFindings: [], - confidence: 1, - }, - ]), - ).toBeNull(); + it("composeAdvisoryNotes publishes the honest withheld-narrative note when nothing is public-safe (was: null)", () => { + // This test used to pin `null` here -- which is exactly the behavior the #9794 incident traced to: null + // was misreported upstream as a PROVIDER failure and held the PR forever. A review that parsed but had + // every field withheld now yields the fixed, public-safe explanation instead. + const notes = composeAdvisoryNotes([ + { + assessment: "reward payout farming", + suggestions: ["payout"], + nits: ["reward"], + blockers: [], + inlineFindings: [], + confidence: 1, + }, + ]); + expect(notes).toContain("withheld"); }); // REGRESSION: FORBIDDEN_PUBLIC_COMMENT_WORDS is matched with a plain case-insensitive `.includes()` over a @@ -5603,3 +5604,51 @@ describe("reviewer vote attribution (#9478)", () => { expect(parsed.producedBy).toBe("primary"); }); }); + +describe("withheld-narrative honesty (#9794 regression)", () => { + it("REGRESSION: a clean review whose ENTIRE narrative is withheld publishes an honest note, not null", () => { + // JSONbored/loopover#9794: the model reviewed scoring/proof code, found no blockers or nits, and every + // narrative sentence used non-public vocabulary -- so composeAdvisoryNotes returned null and the caller + // misreported a PROVIDER failure ("AI review is unavailable"), holding the PR on every re-run. + const review = { + assessment: "The reward payout ranking cohort logic looks correct.", // every sentence trips the sanitizer + blockers: [], nits: [], suggestions: [], inlineFindings: [], confidence: 0.9, + } as never; + const notes = composeAdvisoryNotes([review]); + expect(notes).not.toBeNull(); + expect(notes).toContain("found no blocking issues"); + expect(notes).toContain("withheld"); + // The fixed sentence must itself be public-safe -- no model text, no forbidden vocabulary. + expect(hasPublicReviewAssessment(notes)).toBe(true); + }); + + it("INVARIANT: a truly empty parse (no assessments at all) still returns null — a real provider failure keeps its accurate report", () => { + const review = { assessment: "", blockers: [], nits: [], suggestions: [], inlineFindings: [], confidence: 0 } as never; + expect(composeAdvisoryNotes([review])).toBeNull(); + }); + + it("INVARIANT: withheld BLOCKERS are never reported as a clean result", () => { + // If the model raised blockers and every one was withheld by the sanitizer, the note must say findings + // exist -- "no blocking issues" here would green-light a PR the model actually flagged. + const review = { + assessment: "reward payout farming", + blockers: ["hotkey leak in the payout ranking path"], // withheld: trips the sanitizer + nits: [], suggestions: [], inlineFindings: [], confidence: 0.9, + } as never; + const notes = composeAdvisoryNotes([review]); + expect(notes).toContain("raised blocking findings"); + expect(notes).not.toContain("no blocking issues"); + }); + + it("INVARIANT: surviving blockers/nits still take precedence over the withheld-narrative note", () => { + const review = { + assessment: "The reward payout ranking cohort logic looks correct.", + blockers: ["Missing bounds check on the new pagination cursor."], + nits: [], suggestions: [], inlineFindings: [], confidence: 0.9, + } as never; + const notes = composeAdvisoryNotes([review]); + expect(notes).toContain("blocking findings"); + expect(notes).toContain("Missing bounds check"); + expect(notes).not.toContain("withheld"); + }); +});