Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattmillerai merged 1 commit into from
Aug 4, 2026
+164 −4
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 72 additions & 4 deletions scripts/pr-risk/grade-pr-risk.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=""

Expand Down Expand Up @@ -427,7 +433,16 @@ fetch_pr_record() { # <repo> <num> -> 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){
Expand All @@ -436,9 +451,13 @@ fetch_pr_record() { # <repo> <num> -> 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
Expand All @@ -455,6 +474,55 @@ fetch_pr_record() { # <repo> <num> -> 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,
Expand Down
92 changes: 92 additions & 0 deletions scripts/pr-risk/tests/test_grade_pr_risk.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ]
Loading