fix(ci): fail closed on truncated CI reads and partial GraphQL responses#9111
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
…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.
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.
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (92.85%) is below the target coverage (99.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #9111 +/- ##
==========================================
- Coverage 93.88% 92.66% -1.23%
==========================================
Files 813 813
Lines 80710 80735 +25
Branches 24489 24503 +14
==========================================
- Hits 75777 74811 -966
- Misses 3564 4843 +1279
+ Partials 1369 1081 -288
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-26 20:12:35 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 silent-failure classes on the wrong-merge path, both in the live CI/review readers that gate every merge decision. In each case the code could not distinguish "I read everything and it was clean" from "I couldn't read it all" — and resolved the ambiguity toward merge.
#9051 — check-run pagination cap exhaustion is silent. A failed page fetch already set
checkRunsIncomplete, but exhausting the 10-page cap withrel="next"still present did not: the loop simply exited andreduceLiveCiAggregatetreated a truncated set as complete. A red check on page 11+ was invisible →ciState: "passed"→ plannerreviewGood→ merge. The executor's act-boundary recheck calls the same function, so it reproduced the wrong verdict rather than catching it, and the falsepassedwas persisted into the durable cross-job CI cache. Fixed for both the check-runs and classic-status loops.Two related holes in the same read, also fixed:
Linkfollow, so a first-party suite still running on page 2 never setanyPending. Now paginated, returningnull(which the reducer already fails closed on) when its own cap is exhausted.checkSuitesselection had nohasNextPageguard, unlike itscontextssibling one line above. Added.#9052 — GraphQL readers ignore top-level
errors.fetchLiveReviewThreadBlockersreadconnection?.nodesand returned[]without checking theerrorsarray. GitHub's standard partial-failure shape under load is HTTP 200 withreviewThreads: nullpluserrors— which yielded[], indistinguishable from a genuinely thread-free PR. A maintainer's unresolved blocking thread was dropped fromadvisory.findingsand the gate could concludesuccessand 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:
fetchLivePullRequestReviewDecisionalso had noerrorscheck and returnedundefined, which let the caller'sliveReviewDecision ?? pr.reviewDecisionsubstitute a stale storedAPPROVEDfor a read that failed — so a PR later flipped toCHANGES_REQUESTEDstill merged. It now returns an explicitREVIEW_DECISION_UNREADABLEsentinel that survives the??fallback, matches no real GitHub enum (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
Also triaged
#9001 closed as already-fixed, not re-implemented — #8985 had already moved the anomaly-alert wire onto a dedicated
alert_dedup_claimstable (migration0181) and added the exact schema-conformance regression the issue asked for. Verified passing (30/30) before closing.Test plan
pending, neverpassed, when their cap is hit with pages remaining (and the caps still bound the walk at 10)checkSuitestruncation returnsnullso REST re-reads iterrorsreview-threads response yields a fail-closed blocker instead of[]?? storedand is neitherAPPROVEDnorCHANGES_REQUESTED; clean responses still return the real value /undefinedtsc --noEmitclean; 1368/1368 across the 12 affected suites, no regressions