fix(review): filter the AI narrative assessment per sentence, not all-or-nothing - #9445
Merged
Merged
Conversation
…-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.
Contributor
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-27 19:32:42 UTC
Review summary Nits — 3 non-blocking
Concerns raised — review before merging
📋 Copy for AI agents — paste into your coding agentDecision drivers
Context & advisory signals — never blocks the verdict
Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Contributor
Logic backtestReplayed 0 historical case(s) for Backtest comparison:
|
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9445 +/- ##
==========================================
- Coverage 89.54% 88.64% -0.90%
==========================================
Files 843 843
Lines 110011 110020 +9
Branches 26184 26188 +4
==========================================
- Hits 98511 97529 -982
- Misses 10238 11520 +1282
+ Partials 1262 971 -291
Flags with carried forward coverage won't be shown. Click here to find out more.
|
… 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.
This was referenced Jul 27, 2026
AI review intermittently publishes no narrative summary (~40% of reviews) — root cause unknown
#9432
Closed
Closed
JSONbored
added a commit
that referenced
this pull request
Jul 28, 2026
…d attribute votes to the producing model (#9477, #9478) (#9513) * fix(ai): make prompt drift a cache miss, bound the Ollama context, and attribute votes to the producing model (#9477, #9478) getCachedAiReview reuses a cacheable row with NO maxAgeMs and replays its findings verbatim, including a severity:critical ai_consensus_defect. AI_REVIEW_CACHE_INPUT_VERSION was hand-bumped and last moved for #8364, but #8789, #8791, #8833, #8845, #8961, #9035, #9074, #9087, #9114, #9145 and #9445 all changed prompt text or verdict logic without one -- so a PR closed or held under the pre-#9074 false-consensus rule re-gated at the same head to the SAME stale finding and the corrected logic never ran. The fingerprint now folds in REVIEW_PROMPT_VERSION and a digest of the canonical judge prompt, both pure and available before the call, so drift is an automatic miss rather than something a human must remember. Bumped to v7 to kill rows written under the incomplete fingerprint. Ollama silently left-truncates past num_ctx (typically 4k-8k by default) and then answers confidently over whatever survived -- the TAIL of the prompt, i.e. the context sections rather than the diff. The review path sends up to 120k chars of diff plus a 240k-char context budget, so on the fallback provider a review could come from a fraction of the change while carrying the same blocker authority and confidence semantics as the primary. An explicit num_ctx is now sent for the ollama provider only, since other OpenAI-compatible servers may reject an unknown options key, and an explicit caller providerOptions still wins. runWorkersOpinion iterates [primary, fallback] internally and its outcome carried no model identity, so a fallback-produced review was recorded as a PRIMARY vote -- poisoning the reviewer_vote audit events, #8229's routing track records, and scoreJudgmentAgreement's contribution to decision-record confidence. The doc's claim that slot-to-model is unambiguous by construction holds for the tie-break swap, not for in-slot fallback. * chore(selfhost): regenerate the env reference for the new env reads
This was referenced Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the root cause behind #9432.
FORBIDDEN_PUBLIC_COMMENT_WORDSis matched with a plain case-insensitive.includes()over a list that contains ordinary review vocabulary —reward,rewards,ranking,rankings,cohort,farming,reviewability— andtoPublicSafedrops its whole input whensanitizePublicCommentthrows. So a perfectly safe review of this codebase's own gate/scoring code had its entire narrative discarded and replaced by the generic "did not include a separate narrative summary" placeholder.Verified against the real filter — 3 of 4 realistic review sentences are rejected wholesale:
Evidence this is the right cause
ai_review_missing_assessmentdiagnostic was emitted for any affected PR (docker logsover the incident window: zero matches), soparseModelReviewreturned a non-empty assessment and it was discarded downstream.ai_review_cacherows carryinconclusive: falseandreviewer_count: 1— this is not a provider failure.Fix
Filter the assessment per sentence and keep the survivors.
Safety is not weakened. A leaked private VALUE ("trust score 0.82", "reward estimate 12 TAO") necessarily sits in the same sentence as the term naming it, so dropping that sentence removes the risky content — whereas discarding the whole assessment protected nothing additional and cost the reader every other sentence.
It also makes the narrative consistent with findings, which already filter per item (
safeBlockers/safeNitskeep survivors). The assessment was the lone all-or-nothing holdout.Behaviour is unchanged in both edge cases: a fully-safe assessment takes a whole-text fast path (preserving exact original spacing), and an assessment whose every sentence is unsafe still returns
nulland falls through to the existing placeholder.Test plan
0.82,12 TAO,trust scoreall absent) while the safe lead sentence survivescomposeAdvisoryNotes returns null when no assessment or finding is public-safe,preserves blockers and nits when the model omits a narrative assessment) still pass unchangednpm run typecheckcleanNot covered by this PR
The separate
inconclusive: truefailure mode (e.g. awesome-claude#5599, which returns "AI review is unavailable for this PR head") is a different branch and remains open under #9432.