fix(review): forced re-run steals the AI-review lock; re-run checkbox ignores closed/merged PRs#9108
Conversation
…of silently no-op'ing; re-run checkbox ignores closed/merged PRs Two related fixes to the panel re-run mechanism: #9008 -- a forced re-run (the panel checkbox / `@loopover review`) previously died silently behind the AI-review lock: claimAiReviewLock had no way to bypass a still-held (possibly orphaned, from a self-host process that died mid-review) lock, so the contended path returned the same inconclusive placeholder a genuine race would, with zero audit trail, and that placeholder's plain-prose notes masqueraded as a real review summary (hasPublicReviewAssessment had no way to tell it apart), suppressing the one alarm meant to catch this. shouldStartAiReviewForAdvisory also silently ignored forceAiReview entirely, so a low-reputation author's forced re-run could skip for a completely different, unrelated reason. Adds a `steal` option to claimTransientLock (cache path: unconditional `set` instead of `claim`; SubmissionLock DO: same overwrite semantics), threaded through claimAiReviewLock and used whenever `forceAiReview: true`. Names the contended path in the audit trail unconditionally (forced or not). Fixes the masquerade by checking `persistable === false` directly -- the one signal aiReviewLockContendedResult uniquely produces. Threads forceAiReview into shouldStartAiReviewForAdvisory to bypass ONLY the reputation skip, never a maintainer-configured hard gate. #9020 -- the panel comment is deliberately preserved after merge/close and its re-run checkbox stays interactive on GitHub forever, but maybeProcessPrPanelRetrigger never checked pr.state, so a post-merge click ran deep into the pipeline (real GitHub/DB spend) before dying at the freshness guard with contradictory telemetry and no user feedback. Adds the same `pr.state !== "open"` guard reReviewStoredPullRequest already has. Separately, createOrUpdateSkippedGateCheckRun had no updateExisting mode, so its no-checkRunId lookup path could PATCH an already-`completed` Gate run to `skipped` -- falsifying a fully-evaluated, merged PR's historical verdict. Scoped to updateExisting: "in_progress_only", matching this call's actual purpose (finalize an in-progress evaluation that never finished). Closes #9008, #9020 Tests: 4 new transient-lock steal tests (cache + DO paths, both success and fail-open), a forceAiReview-bypasses-reputation test, two full-pipeline integration tests proving the steal + audit event + masquerade fix end to end (including the audit write's own fail-open branch), a pr_not_open skip test, and a completed-check-run-never-demoted test. 100% line+branch coverage on every changed line across all 5 touched source files.
|
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 #9108 +/- ##
==========================================
- Coverage 93.88% 92.66% -1.23%
==========================================
Files 813 813
Lines 80666 80677 +11
Branches 24477 24479 +2
==========================================
- Hits 75737 74759 -978
- Misses 3562 4842 +1280
+ Partials 1367 1076 -291
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-26 19:09:36 UTC
Review summary Nits — 6 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed 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.
|
Summary
Two related fixes to the panel re-run mechanism:
#9008 — forced re-run silently died behind an orphaned AI-review lock.
claimAiReviewLockhad no bypass, so a maintainer's forced re-run (the panel checkbox /@loopover review) landed on the exact same contended-placeholder path a genuine race would — with zero audit trail, since everyai_review_*event lives inside the lock-acquired branch. Worse, the placeholder's plain-prose notes masqueraded as a real review summary, suppressing the one alarm (ai_review_public_summary_missing) meant to catch "no fresh review happened." Confirmed live: an orphaned lock (from a self-host process that died mid-review) made the re-run button inert for 15+ minutes.Fix:
stealoption onclaimTransientLock(cache path: unconditionalsetinstead ofclaim; SubmissionLock DO: same overwrite semantics), threaded throughclaimAiReviewLockand used wheneverforceAiReview: true— a duplicate LLM call is the explicitly accepted cost.github_app.ai_review_lock_contended, with aforcedflag), forced or not.persistable === falsedirectly in the alarm condition — the one signalaiReviewLockContendedResultuniquely produces.forceAiReviewintoshouldStartAiReviewForAdvisoryto bypass ONLY the reputation-based skip, never a maintainer-configured hard gate (aiReviewMode off, an ineligible author, etc.).#9020 — the re-run checkbox does real work (and worse) on an already-merged PR. The panel comment is deliberately preserved after merge (
#preserve-review-on-close) and its checkbox stays interactive on GitHub forever, butmaybeProcessPrPanelRetriggernever checkedpr.state, so a post-merge click ran deep into the pipeline — real GitHub/DB spend — before finally dying at the freshness guard with contradictory telemetry and no user-visible feedback. Second harm: it could reachcreateOrUpdateSkippedGateCheckRun, which had noupdateExistingmode and could PATCH an already-completed, fully-evaluated Gate run down toskipped— falsifying the historical verdict on a merged PR.Fix: added the same
pr.state !== "open"guardreReviewStoredPullRequestalready has (mirrors its sibling entry point exactly), and scopedcreateOrUpdateSkippedGateCheckRuntoupdateExisting: "in_progress_only"— matching its actual purpose (finalize an in-progress evaluation that never finished) without ever touching an already-terminal run.Closes #9008, #9020
Test plan
transient-locks.test.tstests: steal on both the cache and DO paths, a genuine (non-steal) claim still contends normally, steal fails open when the cache errorsai-review-advisory.test.ts:forceAiReviewbypasses reputation but not a hard gatequeue.test.ts: two full-pipeline integration tests — forced steal + audit event + masquerade fix end to end, plus the audit write's own fail-open branchqueue-4.test.ts: a re-run click on a merged PR is skipped immediately withpr_not_open, no live GitHub callsgithub-app.test.ts: an already-completedGate check run is never demoted, a separate run is posted insteadnpx tsc --noEmit,check-migrations.ts,check-schema-drift.ts,check-engine-parity.tsall green