From 34dd1e57e9463b0837e74aefc2e02499de24be37 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Mon, 3 Aug 2026 18:02:24 -0700 Subject: [PATCH] fix(pr-risk): drain the check rollup instead of grading only its first page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- scripts/pr-risk/grade-pr-risk.sh | 76 ++++++++++++++++- scripts/pr-risk/tests/test_grade_pr_risk.sh | 92 +++++++++++++++++++++ 2 files changed, 164 insertions(+), 4 deletions(-) diff --git a/scripts/pr-risk/grade-pr-risk.sh b/scripts/pr-risk/grade-pr-risk.sh index 4cef24c..1eb4141 100755 --- a/scripts/pr-risk/grade-pr-risk.sh +++ b/scripts/pr-risk/grade-pr-risk.sh @@ -62,6 +62,12 @@ FLEET_LOGINS="${PR_RISK_FLEET_LOGINS:-mattmillerai}" BOT_LOGINS="${PR_RISK_BOT_LOGINS:-github-actions,dependabot,renovate,coderabbitai,cursor,comfy-pr-bot,web-flow}" SELF_CONTEXT="${PR_RISK_SELF_CONTEXT:-}" SELF_RUN_ID="${PR_RISK_SELF_RUN_ID:-}" +# How many 100-context pages of the check rollup to drain before giving up and leaving the PR +# UNKNOWN. 30 pages = 3000 checks, the same ceiling GitHub puts on the REST changed-files +# endpoint this script already accepts, and ~29x the largest rollup measured on Comfy-Org/cloud +# (103). It is a runaway bound, not a policy: a rollup past it stays ungraded rather than being +# graded off the pages that fit. +MAX_ROLLUP_PAGES="${PR_RISK_MAX_ROLLUP_PAGES:-30}" PR_NUM="" MODE="" @@ -427,7 +433,16 @@ fetch_pr_record() { # -> record JSON on stdout, rc 1 on an unreadab # forever, and the PR would land a confident-looking R2 floor after burning the whole wait # budget. Rejecting the read outright is what keeps that from being reachable — so do NOT # "recover" partial data here by dropping the rc check. - local repo="$1" num="$2" q resp files fstatus errf + local repo="$1" num="$2" q qctx ctxsel resp files fstatus errf + # The rollup's context selection, shared by the first read and the drain query below so the + # two cannot drift. It is ONE string on purpose: `is_self` keys on + # `checkSuite.workflowRun.databaseId` and `is_failing`/`is_pending` on `__typename` + + # status/conclusion, so a drained page missing any of those fields would read as neither + # ours nor failing — self-exclusion would quietly stop working past context 100. + # shellcheck disable=SC2016 # GraphQL: $vars are query variables + ctxsel='pageInfo{ hasNextPage endCursor } nodes{ __typename + ... on CheckRun{ name status conclusion checkSuite{ workflowRun{ databaseId workflow{ name } } } } + ... on StatusContext{ context state } }' # shellcheck disable=SC2016 # GraphQL: $vars are query variables q='query($owner:String!,$name:String!,$num:Int!){ repository(owner:$owner,name:$name){ pullRequest(number:$num){ @@ -436,9 +451,13 @@ fetch_pr_record() { # -> record JSON on stdout, rc 1 on an unreadab additions deletions changedFiles labels(first:100){ pageInfo{ hasNextPage } nodes{ name } } commits(last:1){ nodes{ commit{ statusCheckRollup{ state - contexts(first:100){ pageInfo{ hasNextPage } nodes{ __typename - ... on CheckRun{ name status conclusion checkSuite{ workflowRun{ databaseId workflow{ name } } } } - ... on StatusContext{ context state } } } } } } } + contexts(first:100){ '"$ctxsel"' } } } } } + } } }' + # shellcheck disable=SC2016 # GraphQL: $vars are query variables + qctx='query($owner:String!,$name:String!,$num:Int!,$cursor:String!){ + repository(owner:$owner,name:$name){ pullRequest(number:$num){ + commits(last:1){ nodes{ commit{ statusCheckRollup{ + contexts(first:100, after:$cursor){ '"$ctxsel"' } } } } } } } }' # SURFACE WHY THE READ FAILED. Discarding gh's stderr made a PERMANENTLY misconfigured token # look exactly like a transient blip: the caller's rc=3 handler retries four times with @@ -455,6 +474,55 @@ fetch_pr_record() { # -> record JSON on stdout, rc 1 on an unreadab || { warn "PR read for $repo#$num returned no pullRequest — treating as unreadable" rm -f "$errf"; return 1; } + # ---- drain the rollup's `contexts` connection ---------------------------------------------- + # GraphQL caps ANY connection page at 100, so the read above returns at most the first 100 + # checks plus `hasNextPage`. The grader refuses to grade a TRUNCATED rollup — a subset cannot + # answer "did green checks covering these lines actually run?" — so without this loop every PR + # with more than 100 checks landed `risk:ungraded`, and the busier a repo's CI the more of its + # PRs went ungraded. That is the same shape as the `files(first:100)` bug fixed by moving the + # changed-file list to REST `--paginate` (see the note below it); the rollup is the connection + # that was left behind. + # + # ON FAILURE, LEAVE THE FLAG SET. A page read that errors, a cursor that does not advance, or a + # rollup deeper than the page guard all `break` WITHOUT splicing, so the record keeps the + # first page and its `hasNextPage: true` — which the unknown contract below turns into exactly + # the ungraded outcome this path already produced. Never a partial rollup graded as whole, and + # never the harsher "PR unreadable" bucket for a PR we did successfully read. + local rollup='.data.repository.pullRequest.commits.nodes[0].commit.statusCheckRollup' + local ctxnodes ctxmore ctxcur page pages=0 + ctxnodes="$(jq -c "[$rollup.contexts.nodes[]?]" <<<"$resp")" + ctxmore="$(jq -r "$rollup.contexts.pageInfo.hasNextPage // false" <<<"$resp")" + ctxcur="$(jq -r "$rollup.contexts.pageInfo.endCursor // \"\"" <<<"$resp")" + while [ "$ctxmore" = true ] && [ -n "$ctxcur" ]; do + pages=$((pages + 1)) + if [ "$pages" -gt "$MAX_ROLLUP_PAGES" ]; then + warn "rollup for $repo#$num exceeds $((MAX_ROLLUP_PAGES * 100)) checks — grading it would mean grading a partial rollup, so it stays UNKNOWN" + break + fi + page="$(gh api graphql -f query="$qctx" -F owner="${repo%%/*}" -F name="${repo##*/}" \ + -F num="$num" -F cursor="$ctxcur" 2>"$errf")" || { + warn "rollup page $pages failed for $repo#$num: $(tr '\n' ' ' < "$errf")" + break + } + ctxnodes="$(jq -c --argjson acc "$ctxnodes" "\$acc + [$rollup.contexts.nodes[]?]" <<<"$page")" || break + ctxmore="$(jq -r "$rollup.contexts.pageInfo.hasNextPage // false" <<<"$page")" + # A cursor that repeats would spin until the page guard; treat it as a drained-but-unsure + # read and let the flag above decide. + local prev="$ctxcur" + ctxcur="$(jq -r "$rollup.contexts.pageInfo.endCursor // \"\"" <<<"$page")" + [ "$ctxcur" != "$prev" ] || { warn "rollup cursor for $repo#$num did not advance — stopping"; break; } + done + # Splice ONLY when pagination actually ran AND drained. `pages -gt 0` is load-bearing beyond + # saving work: on a PR with no checks at all `statusCheckRollup` is null, and assigning into a + # null in jq CREATES the object — which would turn "no rollup" into "an empty rollup" and route + # it away from the `$ro == null` branch that treats a checkless PR as readable. + if [ "$pages" -gt 0 ] && [ "$ctxmore" = false ]; then + resp="$(jq -c --argjson ctx "$ctxnodes" \ + "$rollup.contexts = {pageInfo: {hasNextPage: false, endCursor: null}, nodes: \$ctx}" \ + <<<"$resp")" || { warn "could not reassemble the drained rollup for $repo#$num" + rm -f "$errf"; return 1; } + fi + # ---- the changed-file list comes from REST, not GraphQL ------------------------------------ # GraphQL's `files` connection cannot answer this axis. Two reasons, both structural: # * PullRequestChangedFile has NO previous-path field (its whole field set is additions, diff --git a/scripts/pr-risk/tests/test_grade_pr_risk.sh b/scripts/pr-risk/tests/test_grade_pr_risk.sh index 09f1beb..cda2981 100755 --- a/scripts/pr-risk/tests/test_grade_pr_risk.sh +++ b/scripts/pr-risk/tests/test_grade_pr_risk.sh @@ -286,6 +286,98 @@ case "$err" in *) bad "and the warning still refuses to read as low risk" "$err" ;; esac +echo "— phase 20: a check rollup deeper than one page is DRAINED, not abandoned —" +# GraphQL caps ANY connection page at 100, so a rollup with more checks than that came back +# `hasNextPage: true` and the whole PR graded UNKNOWN. Measured on Comfy-Org/cloud: an ordinary +# code PR carries 103 checks and graded `risk:ungraded`, while the enrollment PR (66 checks) +# graded fine — so the bug was invisible until real traffic hit it, and the busier a repo's CI +# the more of it went ungraded. +# +# The paging stub answers page 1 when no cursor is passed and page 2 when `cursor=C1` is. OUR +# OWN check deliberately lives on PAGE 2: a drain that dropped page-2 nodes, or fetched them +# with a narrower field selection than page 1, would silently stop excluding self and read +# PENDING forever. +mkdir -p "$SANDBOX/binpage" +cat > "$SANDBOX/page1.json" <<'FIX' +{"data":{"repository":{"pullRequest":{ + "number":42,"title":"docs: tweak readme","state":"OPEN","isDraft":false, + "createdAt":"2026-08-01T00:00:00Z","updatedAt":"2026-08-01T00:10:00Z","closedAt":null,"mergedAt":null, + "author":{"login":"dev"},"authorAssociation":"MEMBER","baseRefName":"main","headRefName":"docs-tweak", + "isCrossRepository":false,"additions":3,"deletions":1,"changedFiles":1, + "labels":{"pageInfo":{"hasNextPage":false},"nodes":[]}, + "commits":{"nodes":[{"commit":{"statusCheckRollup":{"state":"PENDING","contexts":{ + "pageInfo":{"hasNextPage":true,"endCursor":"C1"}, + "nodes":[ + {"__typename":"CheckRun","name":"unit tests","status":"COMPLETED","conclusion":"SUCCESS", + "checkSuite":{"workflowRun":{"databaseId":1000,"workflow":{"name":"CI"}}}} + ]}}}}]} +}}}} +FIX +cat > "$SANDBOX/page2.json" <<'FIX' +{"data":{"repository":{"pullRequest":{ + "commits":{"nodes":[{"commit":{"statusCheckRollup":{"contexts":{ + "pageInfo":{"hasNextPage":false,"endCursor":null}, + "nodes":[ + {"__typename":"CheckRun","name":"Grade PR risk","status":"IN_PROGRESS","conclusion":null, + "checkSuite":{"workflowRun":{"databaseId":999,"workflow":{"name":"CI - PR Risk Grade"}}}} + ]}}}}]} +}}}} +FIX +cat > "$SANDBOX/binpage/gh" <<'STUB' +#!/usr/bin/env bash +fixture=""; filter=""; paged=0 +for ((i=1; i<=$#; i++)); do + a="${!i}" + case "$a" in + cursor=*) paged=1 ;; + graphql) fixture="$FIXTURE_DIR/page1.json" ;; + *pulls/*/files*) fixture="$FIXTURE_DIR/files.json" ;; + --jq) n=$((i+1)); filter="${!n}" ;; + esac +done +if [ "$paged" -eq 1 ]; then + [ -z "${FAILPAGE:-}" ] || { echo "HTTP 502" >&2; exit 1; } + fixture="$FIXTURE_DIR/${PAGE2:-page2.json}" +fi +[ -n "$fixture" ] || { echo "gh stub: unhandled args: $*" >&2; exit 1; } +if [ -n "$filter" ]; then jq -c "$filter" "$fixture"; else cat "$fixture"; fi +STUB +chmod +x "$SANDBOX/binpage/gh" +paged_pr() { PATH="$SANDBOX/binpage:$PATH" bash "$GRADER" --repo test/repo --pr 42 "$@" 2>/dev/null; } + +out="$(paged_pr --self-run-id 999)" +eq "a two-page rollup grades instead of going unknown" ok "$(jq -r '.risk.status' <<<"$out")" +# Self lives on page 2, so this only reads SUCCESS if the drained page was both fetched AND +# carried the checkSuite.workflowRun.databaseId that is_self keys on. +eq "self-exclusion still works past the page boundary" SUCCESS "$(jq -r '.checks_state' <<<"$out")" +eq "drained rollup reports nothing else pending" false "$(jq -r '.checks_pending_excl_self' <<<"$out")" + +# A page-2 read that FAILS must leave the record's hasNextPage set — the honest ungraded +# outcome — never a grade computed from page 1 alone. +out="$(FAILPAGE=1 paged_pr --self-run-id 999)" +eq "a failed rollup page stays UNKNOWN, never a partial grade" unknown "$(jq -r '.risk.status' <<<"$out")" + +# A cursor that repeats would spin until the page guard; it must stop and stay unknown. +jq '.data.repository.pullRequest.commits.nodes[0].commit.statusCheckRollup.contexts.pageInfo + = {"hasNextPage":true,"endCursor":"C1"}' "$SANDBOX/page2.json" > "$SANDBOX/page2stuck.json" +out="$(PAGE2=page2stuck.json paged_pr --self-run-id 999)" +eq "a non-advancing cursor stops and stays UNKNOWN" unknown "$(jq -r '.risk.status' <<<"$out")" + +# The page guard is a runaway bound, not a policy: past it the PR stays ungraded rather than +# being graded off the pages that fit. +out="$(PR_RISK_MAX_ROLLUP_PAGES=0 paged_pr --self-run-id 999)" +eq "a rollup past the page guard stays UNKNOWN" unknown "$(jq -r '.risk.status' <<<"$out")" + +# A PR with NO checks at all has a null statusCheckRollup. Assigning into a null CREATES the +# object in jq, so an unguarded splice would turn "no rollup" into "an empty rollup" and route +# it away from the branch that treats a checkless PR as readable. Pagination must not run here. +jq '.data.repository.pullRequest.commits.nodes[0].commit.statusCheckRollup = null' \ + "$SANDBOX/page1.json" > "$SANDBOX/page1norollup.json" +cp "$SANDBOX/page1norollup.json" "$SANDBOX/page1.json" +out="$(paged_pr --self-run-id 999)" +eq "a PR with no checks is untouched by the drain" ok "$(jq -r '.checks_status // "ok"' <<<"$out")" +eq "and still grades" ok "$(jq -r '.risk.status' <<<"$out")" + echo echo "passed $PASS, failed $FAIL" [ "$FAIL" -eq 0 ]