fix(review): filter the AI narrative assessment per sentence, not all-or-nothing

src/services/ai-review.ts

@@ -674,6 +674,50 @@ export function toPublicSafe(text: string | null | undefined, options?: { allowB
   }
 }
 
+/** Sentence boundary: a `.`/`!`/`?` followed by whitespace. Deliberately simple — the narrative assessment the
+ *  prompt asks for is 2-4 sentences of plain prose ("a substantive but CONCISE summary"), not markdown with
+ *  embedded code blocks, so an abbreviation like "e.g." at worst keeps two sentences joined (a slightly larger
+ *  unit is dropped) and never splits mid-word. It can never merge separate sentences, which is the direction
+ *  that would matter for safety. */
+const SENTENCE_BOUNDARY = /(?<=[.!?])\s+/;
+
+/**
+ * Public-safe filter for the narrative assessment, applied PER SENTENCE instead of all-or-nothing.
+ *
+ * {@link toPublicSafe} drops its whole input when `sanitizePublicComment` throws, and
+ * FORBIDDEN_PUBLIC_COMMENT_WORDS is matched with a plain case-insensitive `.includes()` over a list that
+ * contains ordinary English review vocabulary -- "reward", "rewards", "ranking", "rankings", "cohort",
+ * "farming", "reviewability". A perfectly safe review of this codebase's own gate/scoring code ("updates the
+ * ranking comparator so ties resolve deterministically") therefore had its ENTIRE narrative discarded and
+ * replaced by the generic "did not include a separate narrative summary" placeholder -- observed live across
+ * ~40% of reviews, with the model's assessment confirmed present (no `ai_review_missing_assessment` diagnostic
+ * was ever emitted for those PRs, so the text was produced and then thrown away downstream).
+ *
+ * Dropping only the offending SENTENCE is both more useful and no less safe. A leaked private VALUE ("trust
+ * score 0.82", "reward estimate 12 TAO") necessarily sits in the same sentence as the term that names it, so
+ * removing that sentence removes the risky content -- whereas discarding the whole assessment protected
+ * nothing additional and cost the reader every other sentence. It also matches how this file ALREADY treats
+ * findings: `safeNits`/`safeBlockers` filter per item and keep the survivors, so granular filtering is the
+ * existing convention and the assessment was the lone all-or-nothing holdout.
+ *
+ * Returns null when no sentence survives, so the caller's existing fallback path is unchanged for the genuinely
+ * unsafe case.
+ */
+export function toPublicSafeBySentence(text: string | null | undefined, options?: { allowBareScoreTerm?: boolean }): string | null {
+  const trimmed = (text ?? "").trim();
+  if (!trimmed) return null;
+  // Whole-text pass first: the common case is entirely safe, and this preserves the exact original spacing
+  // (a split/rejoin would normalise interior whitespace) plus any multi-sentence markdown the splitter would
+  // otherwise chop.
+  const whole = toPublicSafe(trimmed, options);
+  if (whole) return whole;
+  const kept = trimmed
+    .split(SENTENCE_BOUNDARY)
+    .map((sentence) => toPublicSafe(sentence, options))
+    .filter((sentence): sentence is string => Boolean(sentence));
+  return kept.length > 0 ? kept.join(" ") : null;
+}
+
 /** Coerce the varied Workers-AI / provider response envelopes into a scannable string. */
 export function coerceAiText(result: unknown): string {
   if (typeof result === "string") return result;
@@ -1979,7 +2023,10 @@ export function composeAdvisoryNotes(reviews: ModelReview[], options?: { allowBa
   const nits = [
     ...new Set(reviews.flatMap((r) => [...r.nits, ...r.suggestions])),
   ].slice(0, 5);
-  const assessment = toPublicSafe(assessments[0] ?? "", options);
+  // Per-sentence, not all-or-nothing: see toPublicSafeBySentence. The findings below already filter per item
+  // (`safeBlockers`/`safeNits`); this makes the narrative consistent with them instead of being discarded whole
+  // because one sentence used ordinary review vocabulary like "ranking" or "reward".
+  const assessment = toPublicSafeBySentence(assessments[0] ?? "", options);
   const safeBlockers = blockers
     .map((s) => toPublicSafe(s, options))
     .filter((s): s is string => Boolean(s));
@@ -3147,6 +3194,7 @@ export const __aiReviewInternals = {
   resolveDualAiTieBreakWithOrderStability,
   synthesizeDefect,
   toPublicSafe,
+  toPublicSafeBySentence,
   estimateNeurons,
   runWorkersOpinion,
   coerceAiUsage,

test/unit/ai-review.test.ts

@@ -46,6 +46,7 @@ const {
   resolveDualAiTieBreakWithOrderStability,
   synthesizeDefect,
   toPublicSafe,
+  toPublicSafeBySentence,
   runWorkersOpinion,
   coerceAiUsage,
   aggregateActualUsage,
@@ -2150,6 +2151,17 @@ describe("resolveEffectiveAiReviewPlan (#2567 gate-review follow-up: combine/rev
 });
 
 describe("pure helpers", () => {
+  it("toPublicSafeBySentence returns null for absent or blank input, without splitting", () => {
+    // Both arms of the `text ?? ""` nullish coalesce: a model that returns no assessment field at all reaches
+    // this as undefined, and composeAdvisoryNotes' own `assessments[0] ?? ""` reaches it as "". Neither may be
+    // split into sentences -- an empty assessment must fall through to the existing placeholder path, not
+    // become an empty joined string that would read as a real (blank) narrative.
+    expect(toPublicSafeBySentence(undefined)).toBeNull();
+    expect(toPublicSafeBySentence(null)).toBeNull();
+    expect(toPublicSafeBySentence("")).toBeNull();
+    expect(toPublicSafeBySentence("   \n  ")).toBeNull();
+  });
+
   it("toPublicSafe drops forbidden public text and neutralizes markdown, mentions, links, and control characters", () => {
     expect(toPublicSafe("This change is solid.")).toBe("This change is solid.");
     expect(toPublicSafe("Boost your reward payout")).toBeNull();
@@ -3769,6 +3781,65 @@ describe("pure helpers", () => {
     ).toBeNull();
   });
 
+  // REGRESSION: FORBIDDEN_PUBLIC_COMMENT_WORDS is matched with a plain case-insensitive `.includes()` over a
+  // list containing ordinary review vocabulary ("reward", "ranking", "cohort", "farming", "reviewability"), and
+  // toPublicSafe drops its WHOLE input when sanitizePublicComment throws. A safe review of this codebase's own
+  // gate/scoring code therefore lost its entire narrative to the generic no-summary placeholder -- observed
+  // live across ~40% of reviews, with the model's assessment confirmed present (no ai_review_missing_assessment
+  // diagnostic was emitted for those PRs, so the text was produced and then discarded downstream).
+  it("REGRESSION: keeps the safe sentences of an assessment that mentions ordinary review vocabulary", () => {
+    const notes = composeAdvisoryNotes([
+      {
+        assessment:
+          "Updates the ranking comparator so ties resolve deterministically. The change is correct and adds matching coverage.",
+        suggestions: [],
+        nits: ["Rename the helper."],
+        blockers: [],
+        inlineFindings: [],
+        confidence: 1,
+      },
+    ]);
+    // The offending sentence is dropped ...
+    expect(notes).not.toContain("ranking comparator");
+    // ... the safe one survives, instead of the whole narrative being replaced by the placeholder.
+    expect(notes).toContain("The change is correct and adds matching coverage.");
+    expect(notes).not.toContain("did not include a separate narrative summary");
+    expect(notes).toContain("**Nits (1)**");
+  });
+
+  // SAFETY: the point of the filter is a leaked private VALUE, which necessarily sits in the same sentence as
+  // the term naming it -- so sentence-level dropping removes it just as completely as the old whole-text drop.
+  it("SAFETY: a sentence carrying a private value is removed, and an all-unsafe assessment still falls back", () => {
+    const leak = composeAdvisoryNotes([
+      {
+        assessment: "This looks correct overall. The contributor trust score is 0.82 and the reward estimate is 12 TAO.",
+        suggestions: [],
+        nits: ["Rename the helper."],
+        blockers: [],
+        inlineFindings: [],
+        confidence: 1,
+      },
+    ]);
+    expect(leak).not.toContain("0.82");
+    expect(leak).not.toContain("12 TAO");
+    expect(leak).not.toContain("trust score");
+    expect(leak).toContain("This looks correct overall.");
+
+    // Every sentence unsafe ⇒ null assessment ⇒ existing placeholder path, unchanged.
+    const allUnsafe = composeAdvisoryNotes([
+      {
+        assessment: "The trust score is 0.9. The reward estimate is 12 TAO.",
+        suggestions: [],
+        nits: ["Rename the helper."],
+        blockers: [],
+        inlineFindings: [],
+        confidence: 1,
+      },
+    ]);
+    expect(allUnsafe).toContain("did not include a separate narrative summary");
+    expect(allUnsafe).toContain("Rename the helper.");
+  });
+
   it("composeAdvisoryNotes preserves blockers and nits when the model omits a narrative assessment", () => {
     const withBlocker = composeAdvisoryNotes([
       {