[PR #114] [MERGED] fix(pr-risk): drain the check rollup instead of grading only its first page #123

New issue

Closed

opened 2026-10-26 22:36:28 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:36:28 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/114
Author: @mattmillerai
Created: 8/4/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/pr-risk-paginate-rollup

📝 Commits (1)

34dd1e5 fix(pr-risk): drain the check rollup instead of grading only its first page

📊 Changes

2 files changed (+164 additions, -4 deletions)

View changed files

📝 scripts/pr-risk/grade-pr-risk.sh (+72 -4)
📝 scripts/pr-risk/tests/test_grade_pr_risk.sh (+92 -0)

📄 Description

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:

PR	Checks	`hasNextPage`	Grade
the enrollment PR itself	66	false	`risk:R3` ✅
an ordinary code PR	103	true	`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

contexts is drained with a cursor loop. GraphQL caps first: 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_self keys on checkSuite.workflowRun.databaseId, so a drained page fetched with a narrower selection would silently stop excluding our own in-progress run — the grader would read PENDING forever 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 hasNextPage set 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:

a partial rollup is never graded as if it were whole
a PR we did successfully read never lands in the harsher "PR unreadable" bucket

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): unknown before, 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.

shellcheck clean, bash -n clean, all three suites in scripts/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.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/114 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/4/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/pr-risk-paginate-rollup` --- ### 📝 Commits (1) - [`34dd1e5`](https://github.com/Comfy-Org/github-workflows/commit/34dd1e57e9463b0837e74aefc2e02499de24be37) fix(pr-risk): drain the check rollup instead of grading only its first page ### 📊 Changes **2 files changed** (+164 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `scripts/pr-risk/grade-pr-risk.sh` (+72 -4) 📝 `scripts/pr-risk/tests/test_grade_pr_risk.sh` (+92 -0) </details> ### 📄 Description ## 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: | PR | Checks | `hasNextPage` | Grade | |---|---|---|---| | the enrollment PR itself | 66 | false | `risk:R3` ✅ | | an ordinary code PR | 103 | **true** | `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 **`contexts` is drained with a cursor loop.** GraphQL caps `first:` 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_self` keys on `checkSuite.workflowRun.databaseId`, so a drained page fetched with a narrower selection would silently stop excluding our own in-progress run — the grader would read `PENDING` forever 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 `hasNextPage` set 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: - a partial rollup is never graded as if it were whole - a PR we *did* successfully read never lands in the harsher "PR unreadable" bucket **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): `unknown` before, after the fix: ```json {"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. `shellcheck` clean, `bash -n` clean, all three suites in `scripts/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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:28 +00:00