fix(review): report a withheld narrative honestly instead of as a provider failure - #9806
Merged
Merged
Conversation
…vider failure A review that PARSED cleanly but had every public field withheld by the sanitizer made composeAdvisoryNotes return null, which the orchestration misreports as "AI review is unavailable for this PR head" -- a PROVIDER failure -- and holds the PR for manual review. Because the head is unchanged, every re-run reproduces it: observed live on #9794, which re-ran with diagnostics `claude-code#0:parsed` and was re-held every time, including after explicit maintainer re-ticks. The shape is routine for this project's own scoring/gate code: an honest narrative about it says "score"/"ranking"/"reward" in every sentence, the per-sentence sanitizer drops them all, and a clean review (no blockers, no nits) has nothing left to publish. Publish a fixed, public-safe sentence instead, worded by the review's REAL verdict: "found no blocking issues" only when the model returned no blockers; "raised blocking findings ... read the private review record" when it did -- withheld blockers must never read as a clean result. A truly empty parse (no assessment at all) still returns null, so a genuine provider failure keeps its accurate report. One legacy test pinned the old null; updated with the incident reference.
Contributor
|
Important 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏳ LoopOver is waiting…LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting |
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 #9806 +/- ##
==========================================
- Coverage 90.33% 89.51% -0.82%
==========================================
Files 918 918
Lines 113935 113938 +3
Branches 26974 26976 +2
==========================================
- Hits 102925 101997 -928
- Misses 9681 10850 +1169
+ Partials 1329 1091 -238
Flags with carried forward coverage won't be shown. Click here to find out more.
|
JSONbored
added a commit
that referenced
this pull request
Jul 30, 2026
…withheld (#9956) * feat(review): regenerate a public-safe summary when the narrative is withheld Closes #9809. When every sentence of a review's narrative trips the public-safety sanitizer -- routine for a PR touching this project's own scoring/gate code, where an honest sentence says 'score' or 'ranking' -- the reader got a fixed placeholder rather than a real summary. #9806 made that placeholder honest; this makes it a summary. On the withheld branch only, the narrative is rewritten for a public audience in one small completion and then held to the IDENTICAL sanitizer. The prompt is not the boundary: a rewrite that still names forbidden vocabulary is discarded and the fixed sentence stays the floor, so this can only improve the text a reader sees and can never widen what is publishable. A prompt-injected or careless rewrite fails closed, as does a provider error or the model's own CANNOT_SUMMARIZE opt-out. Scoped to the NO-BLOCKERS branch. The issue asks for a real summary every time and that a withheld blocker never read as clean; for a withheld blocker those conflict, and this resolves toward the safety property -- prose reading clean over a blocker the model actually raised is the one outcome that could green-light a PR. Blockers keep their fixed sentence, structurally, not by trusting the prompt. The provider call lives at the single caller rather than inside composeAdvisoryNotes, which stays pure and synchronous: making the composer async would push await through its call site and cost the testability that makes the sanitizer's behaviour verifiable. narrativeWasWithheld() lets the caller decide whether to spend the call before composing, computed the same way the composer decides, so the two cannot drift. The counter records both outcomes -- an all-fallback result must not look identical to the feature never firing. * test(review): unstub fetch after the regeneration cases The provider-path cases stub global fetch and never restored it, so the stub leaked into whatever file the worker ran next -- it took out selfhost-metrics.test.ts in a full run while passing in isolation.
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.
The bug (the last of today's review-reliability incidents)
JSONbored/loopover#9794produced "AI review is unavailable for this PR head" on every run — including forced re-ticks, including after the beta.4 deploy — while its diagnostics saidclaude-code#0:parsed. The provider was fine every time.The real path: the model reviewed scoring/proof code, found no blockers and no nits, so its entire output was the narrative — and an honest narrative about this project's own gate/scoring code says "score"/"ranking"/"reward" in every sentence. The per-sentence sanitizer withheld all of them,
composeAdvisoryNotesreturnednull, and the orchestration's null-branch misattributed that to the provider, held the PR, and — the head being unchanged — reproduced identically on every re-run. A permanently wedged PR whose review succeeded every single time.The fix
When the parse produced a narrative but nothing survived sanitization, publish a fixed, public-safe sentence instead of
null— no model text, so the never-echo guarantee is untouched — worded by the review's real verdict:A truly empty parse (no assessment at all) still returns
null, so a genuine provider failure keeps its accurate "unavailable" report.One legacy test explicitly pinned the old
nullbehavior — the exact behavior this incident traces to — and is updated with the incident reference.Tests
286 pass in
test/unit/ai-review.test.ts, including the #9794 regression (all-withheld clean review → honest note,hasPublicReviewAssessmenttrue), the withheld-blockers invariant, and the true-provider-failure null case.tscclean.With this, every failure mode from today's investigation has a landed fix: cadence downgrade + false "skipped" bullet (#9800), restart-orphaned reviews (#9802), and this misattribution. Next image picks up all three.