fix(review): prevent stale review cache and sweep starvation - #2119
Merged
Conversation
Contributor
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-01 20:26:26 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 6 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
loopover-orb
Bot
added
gittensor
gittensor:bug
JSONbored
added this to the
M3.5 — Agent Layer Phase 2 (maintainer auto-maintain) milestone
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2119 +/- ##
=======================================
Coverage 95.82% 95.83%
=======================================
Files 224 224
Lines 24975 24996 +21
Branches 9076 9091 +15
=======================================
+ Hits 23933 23954 +21
Misses 428 428
Partials 614 614
🚀 New features to boost your workflow:
|
JSONbored
force-pushed
the
codex/fix-backup-exporter-selfhost
branch
from
July 1, 2026 09:55
376562f to
80b1fa9
Compare
JSONbored
changed the title
Key AI review cache reuse by the prompt, feature, and reviewer inputs that shape the result, so same-head stale cache entries from older review configuration are treated as misses. Let scheduled sweeps and backfills rely on GitHub REST admission instead of global queue backlog snapshots, so unrelated pending work cannot starve required review checks. Validation: npm run test:coverage; npx tsc --noEmit --pretty false; git diff --check.
JSONbored
changed the title
JSONbored
modified the milestones:
M3.5 — Agent Layer Phase 2 (maintainer auto-maintain),
Rate-limit & scheduler resilience
…nfig
The AI-review cache fingerprint recorded each reviewer plan entry as
only reviewer.model (the self-host PROVIDER name, e.g. "claude-code"),
never that provider's own underlying model/effort/timeout/base-url
(CLAUDE_AI_MODEL, CLAUDE_AI_EFFORT, CODEX_AI_MODEL, OLLAMA_AI_BASE_URL,
etc. -- resolved separately at review-call time, not carried by
AI_REVIEW_PLAN). Changing those while the provider name/plan stayed the
same reused a same-head cached AI review produced against a different
configuration.
Add selfHostProviderConfig to the fingerprint input, populated from the
same env vars each provider's own client construction reads (excludes
API keys -- secrets, and irrelevant to review output). Also closes two
codecov/patch branch gaps in the same diff: the new
env.AI_REVIEW_PLAN ? {...} : null path in processors.ts, and the
pre-existing (this PR's own diff) isGroundingEnabled/isEnrichmentEnabled
&& convergedRepoAllowed checks, neither of which had a test reaching
their truthy arm.
changedPaths only carried the touched file PATHS. A retarget (new base branch, same head commit) or certain rebases can change the diff GitHub reports for an otherwise-unchanged head SHA -- the same files can stay touched against the new base while their actual patch content differs, so aiReviewCacheInputMatches kept reusing the stale review. Add baseSha and a per-file content digest (path/status/patch/additions/ deletions -- exactly the fields buildAiReviewDiff and the AI review path read) to the fingerprint, sorted by path so a re-fetched diff in a different row order still hits the cache.
…ind the first The prior removal of ALL regate queue-depth backpressure let every cron tick enqueue another agent-regate-sweep fan-out even while one was still pending/processing, risking duplicate per-repo re-gates once both trigger messages are consumed. Reinstate the check scoped to the trigger job type only (agent-regate-sweep) — NOT per-PR agent-regate-pr backlog, whose normal staggered/rate-deferred drain is what caused the original sweep-starvation bug this PR set out to fix.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
gittensory-ui | f2baa48 | Commit Preview URL Branch Preview URL |
Jul 01 2026, 08:29 PM |
![superagent-security[bot]](https://avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
superagent-security
Bot
added
the
pr:flagged
…view cache for dynamic context Security scanner (P2): the backup-exporter's runtime apk add suppressed all output, hiding an install failure that would silently leave the metrics endpoint non-functional. Drop the >/dev/null 2>&1 redirection. Gate review: grounding/RAG/enrichment/reputation each pull time-varying external context (live CI checks, the vector index, REES/CVE data, the submitter's evolving reputation) that can change for an unchanged head SHA without any of the feature-activation booleans flipping, so a fingerprint built from just those booleans can't detect the drift. A repo with any of these active now bypasses the AI review cache entirely (never reads, never writes) instead of fingerprinting a signal that can't prove freshness.
superagent-security
Bot
removed
the
pr:flagged
# Conflicts: # src/queue/processors.ts # src/review/ai-review-cache-input.ts # test/unit/queue.test.ts
The AI review prompt threads the PR title in (runAiReviewForAdvisory's pr.title, used to build the reviewer's context), but the new rich fingerprint in AiReviewCacheInput never included it. A pull_request edited event that changes only the title (same head SHA) could therefore reuse a cached review generated under the old title, replaying findings for prompt metadata that no longer matches the PR. Add a required title field to AiReviewCacheInput and pass pr.title from the queue processor's fingerprint call site. Update the three test fixtures that construct AiReviewCacheInput literals, and add a direct regression test asserting the fingerprint changes when only the title changes.
8 tasks
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
Fixes two review-stack edge cases that could leave required review checks stale or replay an obsolete AI review result.
What changed
Why
The previous scheduler guard could suppress repo sweeps because old or unrelated regate jobs existed, which meant PRs could sit with required review checks expected instead of being re-evaluated. Separately, same-head AI cache reuse did not distinguish prompt/config changes, so stale review output could be replayed after review instructions or feature activation changed.
Validation
npx tsc --noEmit --pretty falsenpx vitest run test/unit/ai-review-cache-input.test.ts test/unit/index.test.ts test/unit/queue.test.tsnpm run test:coveragegit diff --check