fix(orb): retry dispositions on lock contention, retry inconclusive verdicts, fix holdout misattribution - #9110
Merged
Conversation
…e verdicts, and stop misattributing holdout holds Three disposition-integrity fixes: #9025 -- maybeRunAgentMaintenance returned silently when it lost the per-PR actuation lock: the job completed "successfully", nothing re-queued the disposition, and no audit row recorded that a planned action was abandoned. That silently amplified every restart incident -- the recovered job re-ran, hit its own dead predecessor's orphaned lock, and lost the disposition a SECOND time with no trace at all. Now throws PrActuationLockContendedError (the same contract review-evasion.ts's withPrActuationLock already used for this exact condition; the queue honors its 5s retryAfterMs via consumingRetryDelayMs) and records a named audit event. Both maintenance call sites' catch handlers now re-throw retryable/rate-limit errors instead of logging-and-dropping them, matching the review pipeline's own propagation contract; a plain non-retryable failure is still swallowed and logged. #9019 -- `cacheable=0` conflates two unrelated things: a DYNAMIC review context (grounding/RAG), where the verdict is conclusive but not durable across time, and a genuinely INCONCLUSIVE verdict (a provider outage, a consensus-disputed roll). Because published rows were exempt from the non-cacheable cooldown, a transient outage verdict became FINAL for that head the moment it surfaced -- the bot never retried, directly contradicting the finding's own "re-evaluates on the next update" text, while a green PR gave the contributor no reason to push the commit that would force one. Worse, the head-AGNOSTIC one-shot lookup pinned that same outage verdict across ALL future heads, so a contributor pushing new code could not escape it either. Records the review's own verdict as metadata.inconclusive and keys both behaviors on it: the publish exemption still applies to dynamic-context rows (#2119 unchanged) but not to inconclusive ones, and the one-shot cadence skips inconclusive rows entirely -- that PR never got its one real shot. The cooldown still bounds retries to at most one attempt per window. #9040 -- every "auto-action held by precision circuit breaker" audit row was wrong. agentHoldAuditDetail inferred the breaker purely from "a terminal action was planned but is not in the final plan", but the call site passes the POST-HOLDOUT plan, so every ε-holdout adjudication hold (#8831) was attributed to a breaker that had never engaged -- 6 of 6 live rows paired 1:1 (within 20ms) with decision_audit_holdout events while system_flags contained no engaged breaker at all. Each transform now REPORTS its own engagement, derived from its own before/after pair, and the holdout gets its own reason string; the residual set-difference case returns an honest generic reason instead of a false specific attribution. Closes #9019 Closes #9025 Closes #9040
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
…s enforce Both of these are broken on main right now and fail every PR branched from it, including this one: 1. .release-please-manifest.json still pinned packages/loopover-engine at 3.15.0 while its package.json says 3.15.1 -- the manual bump in #9107 (which the engine twin-parity guard required) never synced the release manifest, so release-manifest:sync:check has failed on every commit since. Regenerated via `npm run release-manifest:sync`. 2. apps/loopover-ui's prettier gate failed on two files last touched by #8848 (proof-of-power-stats-model.ts and proof-of-power-stats.test.tsx) -- two over-long object literals prettier wants wrapped. Applied `prettier --write`; pure formatting, no behavior change.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | f9188bd | Commit Preview URL Branch Preview URL |
Jul 26 2026, 07:45 PM |
Bundle ReportChanges will increase total bundle size by 6.93kB (0.09%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: loopover-uiAssets Changed:
|
7 tasks
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9110 +/- ##
==========================================
- Coverage 93.88% 92.66% -1.23%
==========================================
Files 813 813
Lines 80710 80723 +13
Branches 24489 24495 +6
==========================================
- Hits 75777 74799 -978
- Misses 3564 4843 +1279
+ Partials 1369 1081 -288
Flags with carried forward coverage won't be shown. Click here to find out more.
|
loopover-orb
Bot
added
the
gittensor:bug
Contributor
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-26 20:00:44 UTC
Review summary Nits — 6 non-blocking
CI checks failing
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
Visual preview
Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy. Scroll preview
A short scroll-through clip (desktop) — click either thumbnail to open the full animation. Evidence for scroll-linked behavior a single screenshot can't show. 🟩 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.
|
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
Three disposition-integrity fixes from the audit backlog.
#9025 —
maybeRunAgentMaintenancesilently drops the disposition on lock contention. It returned early when it lost the per-PR actuation lock: the job completed "successfully", nothing re-queued the disposition, and no audit row recorded that a planned action was abandoned. This silently amplified every restart incident — the recovered job re-ran, hit its own dead predecessor's orphaned lock, and lost the disposition a second time with no trace. Now throwsPrActuationLockContendedError(the exact contractreview-evasion.ts'swithPrActuationLockalready used for this condition; the queue honors its 5sretryAfterMsviaconsumingRetryDelayMs) and records a named audit event. Both maintenance call sites' catch handlers re-throw retryable/rate-limit errors rather than logging-and-dropping them; a plain non-retryable failure is still swallowed and logged exactly as before.#9019 — one-shot cadence pins a transient/inconclusive verdict across ALL future heads.
cacheable=0conflates two unrelated things: a dynamic review context (grounding/RAG), where the verdict is conclusive but not durable across time, and a genuinely inconclusive verdict (provider outage, consensus-disputed roll). Since published rows were exempt from the non-cacheable cooldown, a transient outage verdict became final for that head the moment it surfaced — the bot never retried, contradicting the finding's own "re-evaluates on the next update" text, while a green PR gave the contributor no reason to push. Worse, the head-agnostic one-shot lookup pinned that same verdict across every future head, so pushing new code couldn't escape it either.Fix: record the review's own verdict as
metadata.inconclusiveand key both behaviors on it. The publish exemption still applies to dynamic-context rows (#2119 unchanged, covered by its own test) but not inconclusive ones; the one-shot cadence skips inconclusive rows entirely — that PR never got its one real shot. The cooldown still bounds retries to at most one attempt per window.#9040 — hold reason is misattributed; 100% of "precision circuit breaker" rows are wrong.
agentHoldAuditDetailinferred the breaker purely from "a terminal action was planned but is not in the final plan" — but the call site passes the post-holdout plan, so every ε-holdout adjudication hold (#8831) was blamed on a breaker that had never engaged. Live evidence: 6 of 6 such rows paired 1:1 (within 20ms) withdecision_audit_holdoutevents, whilesystem_flagscontained no engaged breaker at all. Each transform now reports its own engagement, derived from its own before/after pair; the holdout gets a distinct reason string; and the residual set-difference case returns an honest generic reason instead of a false specific attribution.Closes #9019
Closes #9025
Closes #9040
Test plan
precision-breakers-chain.test.ts: holdout attributed to the holdout, holdout wins when both engaged, unreported-transform case gets the honest generic reasonai-review-cache.test.ts: a published inconclusive row expires with the cooldown (and is still reused inside it);conclusiveOnlyskips inconclusive rows and falls through to the newest conclusive one, or returns null when all are inconclusivequeue.test.ts: the low-activity PR test rewritten to the orb(review): one-shot cadence pins a transient/inconclusive verdict across ALL future heads #9019 contract — one retry per cooldown window, zero within onequeue-2.test.ts: contended pass throws the retryable error + records the audit row; a failing audit write never swallows the retry; a non-retryable failure is still swallowed and loggedtsc --noEmit,check-migrations,check-schema-drift,check-engine-parityall green