fix(review): reputation-based AI-review skip must fail closed - #9041
Merged
Conversation
LOOPOVER_REVIEW_REPUTATION is live in production. When the reputation signal downgraded a submitter (low signal, or the submissions>=8 / merged<1 burst), AI review was suppressed with NO compensating hold, so the PR proceeded on deterministic checks alone — none of which read code semantics — and could auto-merge with zero defect detection. Suspicion bought LESS scrutiny, inverting the feature's intent; three submitters were in the burst state at the time of the report. maybeAddReputationSkipHold mirrors the contributor-controlled skip's fail-closed hold exactly, adding ai_review_inconclusive wherever public AI review was expected to run. Also repoints the cadence query from review_targets — frozen since the 2026-06-22 self-host cutover, leaving the machine-paced leg silently inert — to the live pull_requests ledger.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
JSONbored
force-pushed
the
fix/reputation-skip-fail-closed
branch
from
July 26, 2026 16:20
e368608 to
609ef30
Compare
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
JSONbored
added a commit
that referenced
this pull request
Jul 26, 2026
… to pull_requests (#9097) #9041 repointed getSubmitterCadence's query from review_targets (frozen since the 2026-06-22 self-host cutover) to the live pull_requests ledger, but the local seedReviewTarget helper inside "submission-cadence signal (#4514)" still inserted into review_targets -- so its own test seeded a table the code no longer reads, making "FLAG-ON: true for a machine-paced submitter" fail on main for everyone (cadence always resolves to 0 samples against the real query). Renamed to seedCadencePullRequest and repointed at pull_requests (repo_full_name/author_login, matching the real query shape) so the test again exercises the code path it names. The module-level seedReviewTarget (used elsewhere in this file for the separate quality/burst signal, which #9041 did not change) is untouched.
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
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.
Closes #9015.
LOOPOVER_REVIEW_REPUTATION=trueis live on edge-nl-01, so this was an active production exposure, not a theoretical one.The defect: when
shouldSkipAiForReputationfired (low signal, or thesubmissions>=8 && merged<1burst),aiReviewWillRunwent false with no compensating hold. The contributor-controlled skip beside it fails CLOSED viamaybeAddRequiredAutoReviewSkipHold; the reputation skip set noautoReviewSkipReason, so no hold branch fired. The PR then proceeded on deterministic checks only — CI, coverage, guardrails, secrets, linked-issue — none of which read code semantics — and could auto-merge with zero AI defect detection. A suspected abuser therefore received LESS scrutiny than a trusted contributor, exactly inverting the feature's intent. The issue verified three submitters sitting in the burst state at report time.Fix 1 — fail closed (
maybeAddReputationSkipHold): the exact sibling of the contributor-controlled hold, sharing itsshouldRequirePublicAiReviewForAdvisorypredicate, adding anai_review_inconclusivefinding so the gate holds for a human instead of passing silently. No-ops when no skip fired (the overwhelmingly common path) and when AI review is off for the repo (nothing was suppressed).Fix 2 — restore the half-blind leg:
getSubmitterCadencequeriedreview_targets, which stopped receiving writes at the 2026-06-22 self-host cutover (newest row frozen there), so the machine-paced cadence signal never fired for anyone. It now reads the livepull_requestsledger (repo_full_name+ case-insensitiveauthor_login,created_atas the submission timestamp), making the second leg of the feature actually functional.Tests: the hold fires under a reputation skip and stays silent on both no-skip and review-off arms; a query-shape assertion pins the cadence source to
pull_requestsand away fromreview_targetsso this cannot silently regress at the next cutover. Changed-line coverage: all statements and branches.Acceptance (from the issue): a burst-flagged author's PR now either gets a review or a named human hold — never a silent deterministic-only auto-merge.