fix(review): auto-clear stale manual-review lock-contention hold; sync engine gate-decision twin#9107
Merged
Merged
Conversation
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Contributor
Logic backtestReplayed 0 historical case(s) for Backtest comparison:
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | f2565a1 | Commit Preview URL Branch Preview URL |
Jul 26 2026, 07:14 PM |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9107 +/- ##
==========================================
- Coverage 93.89% 93.88% -0.01%
==========================================
Files 813 813
Lines 80677 80694 +17
Branches 24479 24486 +7
==========================================
+ Hits 75748 75762 +14
- Misses 3562 3564 +2
- Partials 1367 1368 +1
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 19:11:12 UTC
Review summary Nits — 6 non-blocking
CI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionPartially addressed 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.
|
…ntion resolves manual-review has no removal path today, so once applied it sticks forever even when its underlying cause was a purely transient infra artifact. Narrow scope: only auto-clears for the one case with live evidence of being transient -- aiReviewLockContendedResult's "AI review already in progress for this PR head" finding (a pass losing the AI-review lock race, #8999). Every other hold reason (guardrail, migration collision, breaker downgrades, a human's own hold, ...) still requires a maintainer to lift it. A (repo, PR)-keyed transient marker (24h TTL) records when contention fires; the next pass clears the marker once contention no longer shows up in that pass's gate warnings, and planAgentMaintenanceActions removes the label only when nothing else currently justifies the hold. Closes #9009
…et_scan_incomplete change #9082/#9103 (merged) added the secret_scan_incomplete neutral-hold branch to src/rules/advisory.ts's evaluateGateCheckCore, plus an updated secret_leak provenance comment, but never touched the hand-duplicated engine-package twin (packages/loopover-engine/src/advisory/gate-advisory.ts) -- the two are supposed to stay logically identical (see check-engine-parity.ts's GATE_DECISION_TWIN_PAIR). Confirmed the drift is live on main right now (the engine copy is missing the whole branch), which is exactly what stranded PR #9103's own CI: the version-bump-or-co-edit guard (checkGateDecisionVersionBump) correctly flagged the host-only edit at merge time. Mirrors both pieces into the engine twin and bumps @loopover/engine 3.15.0 -> 3.15.1 (the guard's own documented escape hatch for a one-sided fix landing after the fact), syncing package-lock.json and the miner's expected-engine.version pin alongside it -- same shape as the precedent manual bump in bc9e81a.
JSONbored
force-pushed
the
fix/9009-lock-contention-label-autoclear
branch
from
July 26, 2026 19:11
9d5f01e to
f2565a1
Compare
JSONbored
added a commit
that referenced
this pull request
Jul 26, 2026
…check The miner-UI typecheck reaches into packages/loopover-miner/lib/** (declared as an input), and those files import @loopover/engine -- whose types resolve to packages/loopover-engine/dist/index.d.ts. But unlike @loopover/ui#typecheck (which declares the explicit @loopover/engine#build dependsOn edge for exactly this reason), the miner-UI task only had ^build, and miner-ui has no package.json dependency on engine to create that edge implicitly. Turbo therefore ran engine build CONCURRENTLY with this typecheck; whenever the engine cache missed AND the scheduler interleaved the two the wrong way, the typecheck raced ahead of the dist emit and failed with dozens of phantom "Cannot find module '@loopover/engine'" errors -- an intermittent, whole-job validate-code failure with no real defect behind it (observed live on #9107's first run and on main run 30214733191, while sibling runs of the identical commit passed).
JSONbored
added a commit
that referenced
this pull request
Jul 26, 2026
…ibutor-cap-lock TTL (#9109) * fix(orb): flush orphaned locks at boot, restore sweep candidacy on resume, and widen the contributor-cap-lock TTL Three related lock/liveness fixes: #9021 -- every Redis-backed lock (pr-actuation-lock, ai-review-lock, contributor-cap-wake/-lock) survives a container restart with its TTL intact. On this single-instance deployment, any lock present at boot is provably orphaned -- the process that claimed it is gone. Left alone, each class strands real work for its own TTL (30 min for ai-review-lock, #8998). Adds flushOrphanedLocksAtBoot (selfhost/redis-cache.ts), a best-effort SCAN-delete over the four exclusivity-lock prefixes, wired in at server boot before the queue starts. Deliberately leaves delivery:*, pr-panel-retrigger-pending:*, ci-pending-first-seen:*, and fresh-rebase-forced:* untouched -- none are exclusivity locks, and each has its own restart-survival contract. #9018 -- a paused repo's PRs can go green DURING the pause window (CI- completion passes plan-and-suppress the whole time), and resuming performed no catch-up: if a PR was ALSO regated once before the pause, agent-sweep.ts's #never-endless-reregate rule permanently excludes it from future sweep candidacy, stranding it silently. Adds clearPullRequestsRegatedAtForOpenPrs and calls it on the paused->live transition, both from the single-repo MCP pause/resume tool and the installation-wide bulk-settings route -- restoring one-shot sweep candidacy for the repo's open PRs exactly once. #9024 -- claimContributorCapLock's 30s TTL was sized only for the executor's brief pre-merge recheck, but maybeCloseForContributorCapOnOpen holds the SAME lock across a much longer body (token mint, live GitHub calls, label writes, the nested pr-actuation-lock, a full executeAgentMaintenanceActions pass) that can exceed 30s under GitHub rate-limit backoff -- reopening the exact #7284 TOCTOU this lock exists to close. Widened to 600s, matching claimPrActuationLock's own TTL for a comparably long mutating body. Closes #9018, #9021, #9024 Tests: 3 new flushOrphanedLocksAtBoot tests (deletes matching keys, returns 0 with nothing to do, fails open per-pattern on a scan error), a paused->live MCP-tool test (clears open PRs' markers, never closed ones, never on pause or a repeat resume), two bulk-settings-route tests (clears across all repos in an installation; a non-agentPaused bulk change never touches the marker), and a TTL-parity regression pinning claimContributorCapLock's TTL to claimPrActuationLock's. 100% line+branch coverage on every changed line (src/server.ts is Codecov's own documented ignore-listed entrypoint file). * fix(ci): add the missing engine-build edge to @loopover/ui-miner#typecheck The miner-UI typecheck reaches into packages/loopover-miner/lib/** (declared as an input), and those files import @loopover/engine -- whose types resolve to packages/loopover-engine/dist/index.d.ts. But unlike @loopover/ui#typecheck (which declares the explicit @loopover/engine#build dependsOn edge for exactly this reason), the miner-UI task only had ^build, and miner-ui has no package.json dependency on engine to create that edge implicitly. Turbo therefore ran engine build CONCURRENTLY with this typecheck; whenever the engine cache missed AND the scheduler interleaved the two the wrong way, the typecheck raced ahead of the dist emit and failed with dozens of phantom "Cannot find module '@loopover/engine'" errors -- an intermittent, whole-job validate-code failure with no real defect behind it (observed live on #9107's first run and on main run 30214733191, while sibling runs of the identical commit passed).
JSONbored
added a commit
that referenced
this pull request
Jul 26, 2026
…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.
JSONbored
added a commit
that referenced
this pull request
Jul 26, 2026
…erdicts, fix holdout misattribution (#9110) * fix(orb): retry the disposition on lock contention, retry inconclusive 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 * fix(ci): repair two pre-existing main breakages the drift/format gates 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.
JSONbored
added a commit
that referenced
this pull request
Jul 26, 2026
…ses (#9111) * fix(orb): retry the disposition on lock contention, retry inconclusive 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 * fix(ci): repair two pre-existing main breakages the drift/format gates 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. * fix(ci): fail closed on truncated CI reads and partial GraphQL responses Two silent-failure classes on the wrong-merge path, both in the live CI/review readers that gate every merge decision. #9051 -- a FAILED check-runs page fetch already set checkRunsIncomplete, but EXHAUSTING the 10-page cap with `rel="next"` still present did not: the loop just exited and reduceLiveCiAggregate treated a truncated set as complete. A red check on page 11+ was therefore invisible -> ciState "passed" -> planner reviewGood -> MERGE. The executor's act-boundary recheck calls the same function so it reproduced the wrong verdict rather than catching it, and the false "passed" was persisted into the durable cross-job CI cache. Fixed for both the check-runs and classic-status loops. Separately, the check-suites backstop -- the LAST gate before a commit is certified settled -- read page 1 only with no Link follow, so a first-party suite still running on page 2 never set anyPending; it now paginates and returns null (which the reducer already fails closed on) when its own cap is exhausted. The GraphQL twin's `checkSuites` selection had no hasNextPage guard either, unlike its `contexts` sibling one line above; added. #9052 -- fetchLiveReviewThreadBlockers read `connection?.nodes` and returned [] without ever checking the GraphQL top-level `errors` array. GitHub's standard partial-failure shape under load is HTTP 200 with `reviewThreads: null` plus `errors`, which yielded [] -- indistinguishable from a genuinely thread-free PR -- so a maintainer's unresolved blocking thread was dropped from the findings and the gate could conclude success and merge over the open objection. Unlike a transport error (nothing read at all -> fail open, unchanged), a partial result means the answer is known-unreliable, so it now fails CLOSED with a synthetic blocker. The sibling readers in this file already guarded this; this one was the outlier. Same class, second instance: fetchLivePullRequestReviewDecision also had no errors check and returned undefined, which let the caller's `liveReviewDecision ?? pr.reviewDecision` substitute a STALE stored APPROVED for a read that failed -- so a PR later flipped to CHANGES_REQUESTED still merged. It now returns an explicit REVIEW_DECISION_UNREADABLE sentinel that survives the ?? fallback, matches no real enum value (so every === comparison is correctly false), and is checked by name at the one approval-queue site that would otherwise have read it as "confirmed no changes requested". Closes #9051 Closes #9052 Tests: 3 cap-exhaustion regressions (check-runs, statuses, check-suites), a GraphQL checkSuites-truncation guard, a 200-with-errors review-threads fail-closed test, two review-decision sentinel tests, and an approval-queue test proving an unreadable decision no longer clears a conflict-justified close. 100% coverage on all 88 added lines; 1368/1368 across the 12 affected suites.
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
Two fixes bundled together:
#9009 — manual-review has no removal path. Once applied, the label sticks forever even when its cause was purely transient. Narrow scope: auto-clears specifically for AI-review lock contention (
aiReviewLockContendedResult's "AI review already in progress for this PR head" finding, #8999's exact trigger) once a later pass shows the contention has resolved and nothing else currently justifies the hold. Every other hold reason (guardrail, migration collision, breaker downgrades, a human's own hold, ...) still requires a maintainer to lift it — see themanualHoldReason === nullsafety check inplanAgentMaintenanceActions.A (repo, PR)-keyed transient marker (24h TTL) records when contention fires; the next pass consumes it once contention no longer shows up in that pass's gate warnings.
Closes #9009
Engine gate-decision twin drift. #9082/#9103 (merged) added the
secret_scan_incompleteneutral-hold branch tosrc/rules/advisory.tsbut never mirrored it into the hand-duplicated engine-package twin (packages/loopover-engine/src/advisory/gate-advisory.ts). This drift is live onmainright now, and is exactly what stranded #9103's own CI (the version-bump-or-co-edit guard correctly flagged the host-only edit). Mirrors both the new branch and the updatedsecret_leakprovenance comment into the twin, and bumps@loopover/engine3.15.0 → 3.15.1 (the guard's own documented escape hatch for a one-sided fix landing after the fact) — package-lock.json and the miner'sexpected-engine.versionpin synced alongside it.Test plan
test/unit/agent-actions.test.ts: 5 new pure-function tests for the label-removal branch (313/313 passing)test/unit/queue-2.test.ts: 2 new full-pipeline integration tests — one proving the label is applied then auto-cleared across two passes, one proving a co-occurring guardrail hit keeps the label despite contention resolvingnpx tsc --noEmitcleancheck-engine-parity.ts,check-migrations.ts,check-schema-drift.tsall green