From be3d8855dedfcd42e2dc838c558b3fa33014848d Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 11:54:16 -0700 Subject: [PATCH 1/2] fix(review): filter the AI narrative assessment per sentence, not all-or-nothing FORBIDDEN_PUBLIC_COMMENT_WORDS is matched with a plain case-insensitive `.includes()` over a list containing ordinary review vocabulary -- "reward", "rewards", "ranking", "rankings", "cohort", "farming", "reviewability" -- and toPublicSafe drops its WHOLE input when sanitizePublicComment throws. A 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. The model's assessment was confirmed PRESENT in those cases: no `ai_review_missing_assessment` diagnostic was emitted for the affected PRs, so the text was produced and then thrown away downstream, and the cached rows carry `inconclusive: false` (the review succeeded). Drop only the offending SENTENCE instead. A leaked private VALUE ("trust score 0.82", "reward estimate 12 TAO") necessarily sits in the same sentence as the term naming it, so removing that sentence removes the risky content -- discarding the whole assessment protected nothing additional and cost the reader every other sentence. It also matches how this file already treats findings: safeBlockers/safeNits filter per item and keep the survivors, so the assessment was the lone all-or-nothing holdout. An assessment whose every sentence is unsafe still returns null and takes the existing placeholder path unchanged. --- src/services/ai-review.ts | 49 +++++++++++++++++++++++++++++- test/unit/ai-review.test.ts | 59 +++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 1 deletion(-) diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index a9e0514837..d8cc27fbe7 100644 --- a/src/services/ai-review.ts +++ b/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)); diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 2d354c4198..04e4804329 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -3769,6 +3769,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([ { From 68af7df8e5de9d39c7df28178cce54828f470a91 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 12:14:41 -0700 Subject: [PATCH 2/2] test(review): cover the nullish arm of toPublicSafeBySentence's input coalesce codecov/patch flagged line 707 branch 1: `text ?? ""` was only ever exercised with a present string. 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, since an empty assessment must fall through to the existing placeholder path rather than becoming an empty joined string that reads as a real (blank) narrative. Exports the helper through __aiReviewInternals so the test can reach it directly. --- src/services/ai-review.ts | 1 + test/unit/ai-review.test.ts | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index d8cc27fbe7..eb7b393418 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -3194,6 +3194,7 @@ export const __aiReviewInternals = { resolveDualAiTieBreakWithOrderStability, synthesizeDefect, toPublicSafe, + toPublicSafeBySentence, estimateNeurons, runWorkersOpinion, coerceAiUsage, diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 04e4804329..1f65e1a121 100644 --- a/test/unit/ai-review.test.ts +++ b/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();