Skip to content

fix(review): short-circuit the retry loop on a deliberate INCOHERENT_DIFF_ASSESSMENT bail - #7520

Merged
JSONbored merged 1 commit into
mainfrom
fix/incoherent-diff-short-circuit
Jul 20, 2026
Merged

fix(review): short-circuit the retry loop on a deliberate INCOHERENT_DIFF_ASSESSMENT bail#7520
JSONbored merged 1 commit into
mainfrom
fix/incoherent-diff-short-circuit

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

ai_review_inconclusive and ai_review_provider_unparseable_exhausted errors have been recurring (Sentry LOOPOVER-1P/2B/29). One observed case shows the model returning a clean, well-formed response with "assessment": "Cannot review — the diff appears out of sync with the PR head.", "blockers": [], "nits": [], "suggestions": [] — the model's own deliberate INCOHERENT_DIFF_ASSESSMENT bail (its prompt explicitly instructs it to emit this exact text rather than rubber-stamp a diff it can't map to the PR). parseModelReview correctly treats this the same as a genuine parse failure (null, since neither yields a usable review) — but the retry loop doesn't distinguish the two, so it burns the full 3-attempt-per-model retry budget (6 attempts across primary + fallback) re-asking the same model the same question before giving up.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (e.g. Closes #123) — a linked open issue is required for every contributor PR.

Closes #7518

Validation

  • git diff --check
  • npm run typecheck
  • npm run test:coverage locally — 3 new tests in test/unit/ai-review.test.ts: the retry loop stops after one bail (fallback still gets its own full budget), both models bailing exhausts in 2 attempts total (not 6), and a dedicated unit test for the new isIncoherentDiffBail helper covering the JSON-shape edge cases (no JSON object, malformed JSON hitting the parse catch, a non-string assessment, a look-alike-but-different assessment). New code is 100% covered (verified via targeted per-line coverage analysis); full ai-review.test.ts (204 tests) passes.
  • npm run branding-drift:check
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • Same pre-existing, local-environment-only failures noted in earlier PRs this session (Node version mismatch, 2 unrelated miner-CLI test files) — confirmed unrelated, not present in real CI.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — internal retry-loop logic only.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — no UI changes.)
  • Visible UI changes include a UI Evidence section. (N/A.)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. (N/A.)

Notes

  • New isIncoherentDiffBail helper is a small, standalone check that mirrors parseModelReview's own JSON extraction rather than changing that function's return contract — keeps the change localized to the one retry loop that actually wastes attempts on this (a second, single-shot call site to parseModelReview has no retry budget to save, so it's untouched).

…DIFF_ASSESSMENT bail (#7518)

parseModelReview collapses a model's deliberate INCOHERENT_DIFF_ASSESSMENT
bail (its prompt instructs it to emit this exact text rather than
rubber-stamp a diff it can't map to the PR) into the same null as a
genuine parse failure -- correct for its own contract, since neither
yields a usable review. But the retry loop didn't distinguish the two, so
it burned the full 3-attempt-per-model budget (6 attempts across primary +
fallback) re-asking the same model the same question before giving up
(Sentry: ai_review_inconclusive / ai_review_provider_unparseable_exhausted,
LOOPOVER-1P/2B/29).

Adds isIncoherentDiffBail, a small standalone check mirroring
parseModelReview's own JSON extraction (so it can never disagree with what
that function actually parsed), and short-circuits runWorkersOpinion's
retry loop on it -- same reasoning as the existing CLI-timeout/429/
structural-config breaks: the model's answer about THIS diff is
deliberate and won't change on a same-model retry, but the fallback model
still gets its own full budget since it may reach a different verdict.

Closes #7518
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored JSONbored self-assigned this Jul 20, 2026
@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.46%. Comparing base (784c9aa) to head (9f619eb).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7520      +/-   ##
==========================================
- Coverage   88.48%   88.46%   -0.02%     
==========================================
  Files         720      720              
  Lines       75718    75725       +7     
  Branches    22540    22543       +3     
==========================================
- Hits        66997    66993       -4     
- Misses       7677     7684       +7     
- Partials     1044     1048       +4     
Flag Coverage Δ
shard-1 32.93% <0.00%> (-0.12%) ⬇️
shard-2 36.86% <100.00%> (+<0.01%) ⬆️
shard-3 25.64% <0.00%> (-0.02%) ⬇️
shard-4 38.89% <0.00%> (-0.01%) ⬇️
shard-5 34.21% <85.71%> (+<0.01%) ⬆️
shard-6 36.23% <14.28%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/services/ai-review.ts 96.79% <100.00%> (+0.03%) ⬆️

... and 2 files with indirect coverage changes

@JSONbored
JSONbored merged commit b24cec8 into main Jul 20, 2026
17 checks passed
@JSONbored
JSONbored deleted the fix/incoherent-diff-short-circuit branch July 20, 2026 20:36
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(review): short-circuit the retry loop on a deliberate INCOHERENT_DIFF_ASSESSMENT bail

1 participant