Skip to content

fix(ci): fail closed on truncated CI reads and partial GraphQL responses#9111

Merged
JSONbored merged 3 commits into
mainfrom
fix/9001-9051-9052-silent-failures
Jul 26, 2026
Merged

fix(ci): fail closed on truncated CI reads and partial GraphQL responses#9111
JSONbored merged 3 commits into
mainfrom
fix/9001-9051-9052-silent-failures

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Stacked on #9110 — retarget to main once that merges. The diff below is only this branch's own commit.

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 with rel="next" still present did not: the loop simply exited and reduceLiveCiAggregate treated a truncated set as complete. A red check on page 11+ was invisible → ciState: "passed" → planner reviewGoodmerge. 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.

Two related holes in the same read, also fixed:

  • 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. Now paginated, returning null (which the reducer already fails closed on) when its own cap is exhausted.
  • The GraphQL twin's checkSuites selection had no hasNextPage guard, unlike its contexts sibling one line above. Added.

#9052 — GraphQL readers ignore top-level errors. fetchLiveReviewThreadBlockers read connection?.nodes and returned [] without checking the 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. A maintainer's unresolved blocking thread was dropped from advisory.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 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_claims table (migration 0181) and added the exact schema-conformance regression the issue asked for. Verified passing (30/30) before closing.

Test plan

  • 3 cap-exhaustion regressions: check-runs, classic statuses, and check-suites each resolve to pending, never passed, when their cap is hit with pages remaining (and the caps still bound the walk at 10)
  • GraphQL checkSuites truncation returns null so REST re-reads it
  • A 200-with-errors review-threads response yields a fail-closed blocker instead of []
  • Two review-decision sentinel tests: unreadable survives ?? stored and is neither APPROVED nor CHANGES_REQUESTED; clean responses still return the real value / undefined
  • Approval-queue test proving an unreadable decision no longer clears a conflict-justified close
  • 100% coverage on all 88 added lines (verified by intersecting the diff's added line numbers against lcov)
  • tsc --noEmit clean; 1368/1368 across the 12 affected suites, no regressions

…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-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.85714% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.66%. Comparing base (8cea287) to head (240b5a9).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/queue/processors.ts 84.61% 0 Missing and 2 partials ⚠️
src/db/repositories.ts 90.90% 0 Missing and 1 partial ⚠️

❌ 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     
Flag Coverage Δ
backend 93.47% <92.85%> (-1.70%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/github/backfill.ts 95.89% <100.00%> (+0.03%) ⬆️
src/review/review-thread-findings.ts 94.44% <100.00%> (+0.15%) ⬆️
src/services/agent-approval-queue.ts 99.17% <100.00%> (ø)
src/db/repositories.ts 96.72% <90.90%> (-0.05%) ⬇️
src/queue/processors.ts 95.61% <84.61%> (-0.02%) ⬇️

... and 3 files with indirect coverage changes

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 26, 2026
@loopover-orb

loopover-orb Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-26 20:12:35 UTC

6 files · 1 AI reviewer · no blockers · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
This fixes two real silent-failure classes on the CI/review gate's live-read path: (1) in fetchLiveCiAggregate/fetchLiveCiAggregateViaGraphQl, exhausting the pagination cap while rel="next"/hasNextPage is still true previously exited the loop silently instead of signaling incompleteness, so a red check or still-running suite past page 10 (or >100 GraphQL nodes) was invisible and the aggregate read as "passed"; and (2) fetchLivePullRequestReviewDecision and fetchLiveReviewThreadBlockers ignored a top-level GraphQL `errors` array on an HTTP 200, treating a partial/unreliable read as a clean empty result. Both are now traced to their real cause and fixed at the source (the read layer itself, not a downstream symptom), and the new REVIEW_DECISION_UNREADABLE sentinel is deliberately a real value so it survives the `?? pr.reviewDecision` fallback in agent-approval-queue.ts. Tests exercise the actual GitHub response shapes (Link headers with rel="next", 200-with-errors bodies) rather than fabricated impossible states, and the check-suites backstop pagination correctly returns null on cap exhaustion, which the reducer already fails closed on.

Nits — 6 non-blocking
  • src/github/backfill.ts:3963 — REVIEW_DECISION_UNREADABLE is explicitly checked in src/services/agent-approval-queue.ts, but confirm every other caller of fetchLivePullRequestReviewDecision (e.g. the live planner in queue/processors.ts, not shown in this diff) also checks for the sentinel by name rather than only comparing against "APPROVED"/"CHANGES_REQUESTED", since an unwired call site could let an unreadable decision silently fall through as either.
  • src/github/backfill.ts:3163 (fetchSuites) — the `/* v8 ignore next -- unreachable */` after the for-loop is correct (every loop iteration returns), but consider restructuring as a `while (true)` to make the always-returns invariant more obvious without needing the ignore comment.
  • This PR bundles two related but independently-numbered issues (orb(ci): check-run pagination cap exhaustion is silent — a failing check beyond 1000 runs reads as CI passed (wrong-merge path) #9051 and orb(ci): GraphQL reviewThreads ignores top-level errors — an unresolved blocking review thread reads as zero threads (wrong-merge path) #9052) into one commit; each is well-scoped and cheap to review together here, but note the bundling for anyone trying to cherry-pick or revert just one.
  • src/github/backfill.ts — since checkRunsIncomplete/statusIncomplete now signal on cap exhaustion, double check reduceLiveCiAggregate's consumption of these flags is exercised by a test asserting the specific 'pending' vs 'passed' transition end-to-end (the added tests already do this well).
  • Consider adding a short changelog/README note on the new fail-closed behavior for operators who might see more 'pending' verdicts than before on repos with >1000 check runs or >100 review threads under GraphQL partial failures.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #9051, #9052
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ❌ 8/20 High review scope from cached public metadata (2 linked issues).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 13 registered-repo PR(s), 13 merged, 315 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 13 PR(s), 315 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Addressed
The diff sets checkRunsIncomplete/statusIncomplete when the page cap is hit with rel="next" still present, paginates the check-suites backstop (returning null on cap exhaustion, which the reducer fails closed on), and adds the missing hasNextPage guard to the GraphQL checkSuites selection — covering all three items in the issue's Fix list.

Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, Ruby, Go, MDX, Shell, Solidity, JavaScript
  • Official Gittensor activity: 13 PR(s), 315 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Then work through the remaining 2 steps in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask 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.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Decision record
  • action: merge · clause: success
  • config: 03a7f8b529a9 · pack: oss-anti-slop
  • record: 868ccb298ae3 (schema v3, head 240b5a9)

🟩 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.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 26, 2026
Base automatically changed from fix/9019-9025-9040-disposition-bundle to main July 26, 2026 20:12
@JSONbored
JSONbored merged commit 90ecd88 into main Jul 26, 2026
6 of 7 checks passed
@JSONbored
JSONbored deleted the fix/9001-9051-9052-silent-failures branch July 26, 2026 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

1 participant