fix(pr-risk): drain the check rollup instead of grading only its first page - #114
Conversation
…t page GraphQL caps any connection page at 100, and the rollup was read as a single `contexts(first:100)` with no pagination. The grader correctly refuses to grade a truncated rollup — a subset cannot answer "did green checks covering these lines actually run?" — so every PR with more than 100 checks fell straight to `risk:ungraded`. The effect scales the wrong way: the more CI a repo has, the more of it goes ungraded. Measured on one consumer repo, an ordinary code PR carries 103 checks and graded `ungraded`, while the PR that enrolled the repo carried 66 (path filters kept its CI small) and graded fine — so the failure was invisible on the enrollment itself and only appeared once real traffic arrived. This is the same shape as the `files(first:100)` bug already fixed by moving the changed-file list to REST `--paginate`; the rollup is the connection that was left behind. Drain it with a cursor loop. The context selection is now one shared string used by both the first read and the drain query, because `is_self` keys on checkSuite.workflowRun.databaseId — a drained page fetched with a narrower selection would silently stop excluding our own in-progress run and read PENDING forever. Every failure path leaves the record's `hasNextPage` set rather than splicing: a failed page read, a cursor that does not advance, and a rollup deeper than the page guard all keep today's honest ungraded outcome. A partial rollup is never graded as if it were whole, and a PR we did successfully read never lands in the harsher "unreadable" bucket. The splice is also skipped entirely when no pagination ran, because assigning into a null statusCheckRollup would create the object and turn "this PR has no checks" into "this PR has an empty rollup". Verified live against the 103-check PR: `unknown` before, R3 after (checks SUCCESS). Two of the eight new assertions fail without the fix; the other six guard the failure paths it introduces.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Comment |
3 participants
ELI-5
The risk grader reads a PR's check rollup to answer "did green checks covering these lines actually run?". It read only the first 100 checks and, finding there were more, refused to grade at all — correctly, since a subset can't answer that question. The result: any PR with more than 100 checks got
risk:ungraded. This pages through the rest.The bug scales the wrong way
The more CI a repo has, the more of it goes ungraded — so the check is weakest exactly where a risk grade is worth most.
It was also invisible on enrollment. On the consumer repo where this surfaced:
hasNextPagerisk:R3✅risk:ungraded❌The enrollment PR touches one workflow file, so path filters kept its check count under the cap and it graded fine. The failure only appeared once real traffic arrived. Sampling 12 open PRs on that repo: 2 over the cap, the rest 61–93 — so the whole repo sits just under the cliff and any new workflow pushes more over it.
This is a known shape, one connection later
files(first:100)had the identical bug and was already fixed by moving the changed-file list to REST--paginate. Its comment says it put "exactly the PRs a risk grade helps most (the 150-file ones) in the ungraded lane." The rollup is the connection that was left behind.What changed
contextsis drained with a cursor loop. GraphQL capsfirst:at 100, so raising it isn't an option.The context field selection is now one shared string, used by both the first read and the drain query. This is load-bearing rather than tidiness:
is_selfkeys oncheckSuite.workflowRun.databaseId, so a drained page fetched with a narrower selection would silently stop excluding our own in-progress run — the grader would readPENDINGforever and land a confident R2 floor after burning the whole wait budget. A test puts our own check on page 2 specifically to catch that.Every failure path leaves
hasNextPageset instead of splicing. A failed page read, a non-advancing cursor, and a rollup deeper than the page guard all keep today's honest ungraded outcome. Two properties preserved:The splice is skipped when no pagination ran. A PR with no checks has a null
statusCheckRollup, and assigning into a null in jq creates the object — an unguarded splice would turn "no checks" into "an empty rollup" and route it away from the branch that treats a checkless PR as readable.Page guard:
PR_RISK_MAX_ROLLUP_PAGES(default 30 = 3000 checks). Same ceiling GitHub puts on the REST changed-files endpoint this script already accepts, and ~29× the largest rollup measured. It's a runaway bound, not a policy — past it the PR stays ungraded rather than being graded off the pages that fit.Verification
Live, against the PR that surfaced this (103 checks):
unknownbefore, after the fix:{"status":"ok","tier":"R3","checks_state":"SUCCESS", "reason":"worst of path_floor=R3, provenance=R1, reversibility=R0"}Tests: 57 pass, up from 49. Being precise about what the 8 new ones buy — 2 fail without the fix (the bug itself: a two-page rollup grading at all, and self-exclusion still working past the boundary). The other 6 pass both with and without it: they guard the failure paths this change introduces, so they'd stay green on unfixed code and only fire if a future edit made the drain grade a partial rollup.
shellcheckclean,bash -nclean, all three suites inscripts/pr-risk/tests/pass.Consumer impact
Additive and pinned — no consumer moves until it bumps its own SHA. No input, output, or workflow-interface change; a repo whose PRs all sit under 100 checks sees byte-identical behaviour.