feat(pr-risk): grade a pull request by number on demand (workflow_dispatch) - #113
Conversation
The grader could only ever grade the PR whose event triggered it, so a repo enrolling mid-stream had no way to grade the open queue it already had, and a re-grade after a risk-map change or a `risk-dispute` meant pushing a commit. The grading logic needed no change: it was already an API read keyed on a PR number rather than a read of the event payload. Two optional `workflow_call` inputs expose that — `pr_number` for one PR and `pr_numbers` for a comma-separated list — and with neither supplied the target, the base ref and every emitted label are what they were before. The per-target sequence (resolve the base ref, fetch that ref's override files, poll the grader until the rest of the rollup settles, sync the one label) moves out of the job body into scripts/pr-risk/grade-targets.sh. It is per-target either way, so leaving it inline meant a second copy of the settle poll and the override contract for the by-number path; as a script there is one copy, and it is unit-testable for the first time. Four things the by-number path has to get right: * The base ref is re-read from the API per target, because there is no event payload to take it from, and an unresolvable one FAILS that target. An empty ref is not an error to the contents API — it silently resolves to the default branch — so defaulting it would grade a PR against another branch's rules. Live PRs are commonly stacked on feature branches, not the default one. * A batch never abandons the rest for one bad target: every target is attempted and recorded, then the run reports whether any failed. A target the job's time budget cannot reach is reported by number as not attempted, rather than started and cut off. * Bot-authored and fork PRs are graded here. The actor and fork clauses in a caller's `if:` are token guards — a bot's or fork's `pull_request` run gets a read-only token, so the label write would 403 — and neither applies on a dispatch. Fork risk is untouched: `external` still comes from the API's own fork flag and still grades R3. * A run that resolves no target fails loudly instead of exiting 0. A dispatch button that silently grades nothing is worse than no button. The header's copy-paste caller block is updated for both event shapes: guard clauses scoped to the event they describe (an unscoped fork clause is false on a dispatch, so the job would skip silently) and a concurrency key carrying the resolved target (an event-only key collapses to one constant group, so a batch would have each dispatch cancel the previous one).
…h isolation Hermetic, in the shape of the existing suites: `gh` is stubbed on PATH and every call it receives is logged, so the tests assert on WHICH requests were made and not only on the outcome. 68 assertions across 14 phases. The two that matter most are the ones that fail silently in production: * The event path spends no extra API call. With a base ref supplied from the payload, no PR read is issued and the override is fetched from that ref — the backward-compatibility claim, asserted against the call log rather than stated. * The by-number path reads the override from the target's OWN base ref, and an unresolvable or empty one fails that target having read no override at all. Both hazards are invisible when they regress: the wrong `.github/risk.json` still produces a confident-looking tier. Also pinned: a batch where the middle target is unreadable still grades the first and last; `ungraded` keeps the run green on both paths; a failed label write fails its target; forks grade R3 and bots grade at all; a foreign run id excludes nothing from the rollup, which is the dispatch shape, so a settled PR reads its true SUCCESS instead of the R2 floor; and the target list rejects non-numeric, leading-zero, over-long and empty inputs rather than quietly grading a subset. The two retry constants gain env overrides so the suite can reach the unreadable-PR branch without sleeping through the production backoff. CI passes neither, so the defaults are what runs. test-pr-risk.yml gains the new suite and shellchecks the new files. Its path filter now includes pr-risk.yml, which passes the scripts their whole input contract through env and could otherwise break them without touching a file under scripts/pr-risk/.
The README is the other half of the header for anyone enrolling a repo, so it carries the same three consequences of the by-number path: bots and forks are graded there and why that is a token question rather than a risk one, both guard clauses and the concurrency key must be scoped to the event or the dispatch is a silent no-op, and a low check-settle wait is right for a backfill while `0` still is not.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 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 (7)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 6 |
| 🟢 Low | 2 |
Panel: 8/8 reviewers contributed findings.
…mmary claiming a label it did not apply
Review findings on the by-number grading surface, worst first.
* PERCENT-ENCODE THE REF AND THE OVERRIDE PATH. `?ref=${base}` interpolated raw
truncates at a `#`, and git permits `#`, `&`, `+` and `%` in branch names — a PR
based on `fix/#123-thing` reached the contents endpoint with an EMPTY `?ref=`,
which is not an error there: it silently resolves to the repository default
branch. That is the "graded against rules nobody read" failure resolve_base_ref
exists to prevent, arriving through a different door. Paths are encoded per
segment so `/` stays structural.
* A 404 FOR THE REF IS NOT A 404 FOR THE FILE. The contents endpoint 404s both for
an absent path (benign: use the shipped default) and for a base branch that was
deleted or renamed, reachable on a by-number re-grade of an old PR. The second
now fails its target instead of grading against the generic map.
* THE PRE-GRADER READS RETRY. The base-ref and override reads are the first hop
for every target and had no retry, while the grader one step later retries the
same failure class four times. Rate limits are global, not per-PR, so one
secondary-rate-limit burst mid-backfill failed every remaining target wholesale
— the inverse of "one unreadable PR never abandons the rest". A definitive
answer (404/401/422) is still never retried; an ambiguous 403 is retried only
when the body reads like a rate limit.
* THE SUMMARY NO LONGER RENDERS A GRADE TABLE FOR A FAILED TARGET. A label write
that 403s left a record carrying `.risk`, so the summary headed it "PR risk
grade: R1" with the full axis table — the one outcome where the label on the PR
is not what the summary shows.
* THE SUMMARY BRANCHES ON RECORDED ROWS, NOT ON A STEP OUTPUT. `targets` is
written by the grade step's last lines, so a step cancelled mid-batch emitted
none of it, `${TARGETS:-1}` chose the single-target rendering, and every target
after the first was silently discarded — on exactly the path those records
exist for. Counters are derived from the rows too, and a row count short of the
target count now says so.
* THE START GATE IS THE JOB'S DEADLINE, NOT THE WAIT BUDGET. They are different
questions: a spent wait budget only means no target may sleep, and grading with
zero wait is still a real grade. At `JOB_TIMEOUT_MINUTES <= 5` the wait budget
was 0 from the first instant and a batch recorded every target `skipped`,
grading nothing. The first target is now always attempted.
* CLAMP THE SETTLE SLEEP TO THE DEADLINE. The loop tested `now < deadline` then
slept a full 15/30/60/120s, overrunning the caller's wait by up to 105s per
target and charging it to the later targets' budget.
* AN EMPTY TIER IS THE UNKNOWN LANE. A record that is empty or not JSON makes jq
print nothing at exit 0; apply-risk-label.sh maps that to `risk:ungraded`, so
counting it `graded` credited a labeled-ungraded PR as graded.
* SOURCING HAS NO SIDE EFFECTS. The mktemp calls and `trap ... EXIT` ran at file
scope, so sourcing replaced the sourcing shell's EXIT trap and cost it its own
cleanup. Both are deferred to first use, and the trap is installed only by a
direct invocation.
* A CONCURRENT DELETE OF AN ALREADY-REMOVED LABEL IS NOT A FAILURE (404 means the
loop's goal state). A batch cannot serialize per-PR — one run, N PRs, one
concurrency group — so the residual overlap window is documented in the header
and the README rather than left implied.
Tests: targets 68 -> 91, label 21 -> 24, grader 49 untouched; actionlint and
shellcheck clean.
mattmillerai
added
cursor-review
|
Review round resolved in `facd005` — all ten panel findings addressed, threads replied to individually and resolved. Summary: High
Medium
Low
Tests: targets suite 68 → 91, label suite 21 → 24, grader suite 49 untouched; Still not verified pre-merge, unchanged from the original body: a true |
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
The following carry
|
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 7 |
| 🟢 Low | 2 |
Panel: 8/8 reviewers contributed findings.
| else | ||
| echo "No pull request was graded: targets WERE resolved, but the grade step recorded no outcome for any of them — it was cancelled or died before the first target finished. The dispatched numbers are in the step log; none of them was labeled." | ||
| fi | ||
| elif [ "$ROWS" -eq 1 ] && [ "${TARGETS:-1}" = 1 ]; then |
There was a problem hiding this comment.
🟠 High — [ "${TARGETS:-1}" = 1 ] defaults an unset targets output back to 1, which reintroduces the failure the comment above claims to fix: a batch of N cancelled or timed out after recording exactly one row never writes targets, so ROWS is 1 and the summary renders the full single-PR axis table with no hint that the other targets were never graded. Default to 0 (or let the empty case fall through to the batch branch) so the "never reported its own summary" warning fires. Raised by 3 of 8 reviewers (gemini-3.1-pro edge-case, claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max adversarial).
| ROWS="$(jq -s 'length' "$RESULTS" 2>/dev/null || echo 0)" | ||
| COUNTS="$(jq -sr '[(map(select(.status == "graded")) | length), | ||
| (map(select(.status == "ungraded")) | length), | ||
| (map(select(.status == "failed")) | length), |
There was a problem hiding this comment.
🟡 Medium — jq -s fails on the whole file if any single line is malformed — exactly what a step killed mid-record_result leaves behind — so both ROWS and COUNTS fall back to zero and the summary prints "No pull request was graded ... recorded no outcome for any of them" even though earlier targets were graded and labeled. Use a per-line-tolerant read such as jq -R 'fromjson? // empty' so recorded rows survive a truncated tail. Raised by 2 of 8 reviewers (gemini-3.1-pro adversarial, claude-opus-5-thinking-max edge-case).
| sleep "$nap"; G_WAITED=$(( G_WAITED + nap )) | ||
| delay=$(( delay * 2 )); [ "$delay" -le 120 ] || delay=120 | ||
| done | ||
| G_TIER="$(jq -r '.risk.tier // "unknown"' "$record" 2>/dev/null)" |
There was a problem hiding this comment.
🟡 Medium — jq -r '.risk.tier // "unknown"' has its exit status discarded, so a truncated or malformed grader record silently becomes tier unknown, gets risk:ungraded applied, and the run exits green — masking a real grader/protocol failure as a reported verdict. Distinguish a jq parse failure from a genuinely absent .risk.tier and fail the target in the former case. Raised by 2 of 8 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case; related observation from kimi-k2.7-code adversarial).
| # is the failure the single-target clamp in main() was added for, one level up. | ||
| now="$(date +%s)" | ||
| deadline=$(( now + 60 * WAIT_MINUTES )) | ||
| [ "$deadline" -le "$OVERALL_DEADLINE" ] || [ "$TARGET_COUNT" -eq 1 ] || deadline="$OVERALL_DEADLINE" |
There was a problem hiding this comment.
🟡 Medium — The [ "$TARGET_COUNT" -eq 1 ] exemption from the OVERALL_DEADLINE clamp computes the settle deadline as now + 60 * WAIT_MINUTES where now is taken after the base-ref and two override reads, which now retry with backoff (up to ~90s). With wait_for_checks_minutes near the clamp maximum this pushes the settle window past the reserved headroom and the run can be cancelled mid-sleep before the label is applied — the exact outcome the clamp exists to prevent. Raised by 3 of 8 reviewers (gemini-3.1-pro adversarial, gemini-3.1-pro edge-case, claude-opus-5-thinking-max adversarial).
| # JOB_TIMEOUT_MINUTES <= 5 the wait budget is 0 from the first instant, which made a batch record | ||
| # every target `skipped` and grade NOTHING. | ||
| OVERALL_DEADLINE=$(( started + 60 * max_wait )) | ||
| JOB_DEADLINE=$(( started + 60 * JOB_TIMEOUT_MINUTES - 90 )) |
There was a problem hiding this comment.
🟡 Medium — JOB_DEADLINE = started + 60*JOB_TIMEOUT_MINUTES - 90 is anchored to this script's start, not the job's, so the checkout and runner setup that ran first have already eaten into the reserve; worse, JOB_TIMEOUT_MINUTES=1 (legal per the ^[1-9][0-9]*$ check) puts the deadline in the past, skipping every target after the first and disabling all retry_read retries. Guard the degenerate case the way max_wait does with its > 5 ? ... : 0 clamp, and derive the anchor from a job-start timestamp passed in by the workflow. Raised by 4 of 8 reviewers (claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max edge-case, kimi-k2.7-code adversarial).
| fi | ||
| ref="$(tr -d '\n' < "$OUTF")" | ||
| case "$ref" in | ||
| ""|null) |
There was a problem hiding this comment.
🟡 Medium — gh --jq '.base.ref' renders both a JSON null and the literal branch name null as the bare string null, so the ""|null) case rejects a PR whose base branch is genuinely named null. Distinguish them by reading the field with a jq expression that marks absence explicitly (e.g. --jq '.base.ref // "\u0000"') rather than pattern-matching the rendered text. Raised by 4 of 8 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, gemini-3.1-pro adversarial, kimi-k2.7-code edge-case).
| # check and skipped the target label for a PR that was in the desired state. | ||
| if ! ghq api -X DELETE "repos/$REPO/issues/$PR_NUMBER/labels/$(enc "$l")" >/dev/null; then | ||
| case "$(gherr)" in | ||
| *"(HTTP 404)"*) log "stale '$l' was already gone (404) — treating it as removed" ;; |
There was a problem hiding this comment.
🟡 Medium — Accepting any (HTTP 404) on the stale-label DELETE as "already gone" also swallows the 404 GitHub returns for a nonexistent issue, an inaccessible repo, or a label-name encoding mismatch, so a persistent removal failure leaves the PR carrying two contradictory risk:* labels behind a green check. Re-read the label list after the 404 and only accept it if the label is genuinely absent from the PR. Raised by 3 of 8 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, claude-opus-5-thinking-max adversarial/edge-case).
| if [ -n "${GITHUB_OUTPUT:-}" ]; then | ||
| { | ||
| # `tier` is the single-target tier, which is what the step summary heading reads; a batch | ||
| # has no one tier and says so rather than picking one. |
There was a problem hiding this comment.
🟡 Medium — G_TIER is appended to $GITHUB_OUTPUT as a bare tier=<value> with no validation; it comes from jq -r '.risk.tier' on a record influenced by the --map override read from the PR's base branch, so a tier string containing a newline injects extra key=value lines and lets a caller set arbitrary step outputs consumed by the summary step. Constrain it to ^[A-Za-z0-9_:.-]+$ or use the random-delimiter heredoc form. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).
| local max_wait started | ||
| started="$(date +%s)" | ||
| max_wait=$(( JOB_TIMEOUT_MINUTES > 5 ? JOB_TIMEOUT_MINUTES - 5 : 0 )) | ||
| [ "$WAIT_MINUTES" -ge 0 ] 2>/dev/null || WAIT_MINUTES=10 |
There was a problem hiding this comment.
🟢 Low — [ "$WAIT_MINUTES" -ge 0 ] 2>/dev/null is not a type check: bash's test runs integer operands through arithmetic evaluation, so a[$(cmd)] executes rather than being rejected, and the value is re-evaluated later in $(( now + 60 * WAIT_MINUTES )). It also silently replaces a valid fractional number input like 0.5 with the 10-minute default. Apply the same ^[0-9]+$ regex used for JOB_TIMEOUT_MINUTES and MAX_TARGETS, and to POLL_DELAY_SECONDS, READ_RETRY_* and MAX_UNREADABLE_TRIES. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max edge-case).
| bash "$LABELER" > "$LABELF" || label_rc=$? | ||
| label="$(tail -n 1 "$LABELF")" | ||
| if [ "$label_rc" -ne 0 ]; then | ||
| record_result "$num" failed "$G_TIER" "" "$record" "$G_WAITED" "$base" "graded ${G_TIER} but the label write FAILED (rc=${label_rc}) — the PR still carries whatever label it had" |
There was a problem hiding this comment.
🟢 Low — The recorded note claims "the PR still carries whatever label it had", but apply-risk-label.sh deletes stale risk:* labels before POSTing the target one, so a failure on the add (403, or label creation failing) leaves the PR with no risk label at all and the summary describes a state the PR is not in. Word the note to cover both, or have the labeler report which phase failed. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).
2 participants
ELI5
The risk grader could only ever grade the PR that just poked it. So a repo that turns the grader on today has thirty already-open PRs it can never label — each one has to be poked individually, and poking one means running that repo's entire CI. This adds a button: name a PR number (or a list) and it grades those, no poke required. With no number supplied, nothing about the old path changes.
Motivation
Two live gaps, both of them in the label-the-open-queue product rather than the offline calibration corpus:
synchronize/reopened/ready_for_review. Generating those events costs ~750 jobs on 2-vCPU runners for 30 advisory labels, because six workflows in that repo share the same event types. An empty-commit push costs the same and pollutes 30 branches' history..github/risk.jsonchange, or on a PR carrying the human-ownedrisk-disputelabel, the only way to re-grade was to push a commit to the PR.The grading logic needed no change to support this: it was already an API read keyed on a PR number, never a read of the event payload. The event coupling was two lines of workflow YAML.
Provenance
origin/mainat1c9443aactionlintclean on both workflows; YAML parses;shellcheck -xclean on all six shell files;test_grade_pr_risk.sh49 passed / 0 failed and untouched;test_apply_risk_label.sh24 passed / 0 failed; newtest_grade_targets.sh91 passed / 0 failed. (The review round moved the last two: the label script gained 404-tolerance on a concurrent stale-label DELETE, and the targets suite grew from 68 to 91 phases — see the review-resolution commit.) Plus a live read-only run of the new orchestration against real open PRs (details under Test plan).--stdinmode is not the batch vehicle — see Notes.read (a raw
#in a branch name truncated it into the silent default-branch read this PR exists to prevent), a404 for the ref is distinguished from a 404 for the file, the pre-grader reads retry a transient failure like
the grader already does, the step summary no longer renders a grade table for a target whose label write failed
and no longer discards rows when the grade step is cancelled mid-batch, the start gate is the job's deadline
rather than the wait budget, and sourcing the script no longer replaces the sourcing shell's EXIT trap.
Reviewer context
ghcall log, not stated — see Test planSummary
workflow_callinputs.pr_numbergrades one PR;pr_numbersgrades a comma-separated list and wins when both are set.PR_NUMBERSresolves topr_numbers || pr_number || github.event.pull_request.number, so the event path is the same single number it always was. Both arestring, notnumber:workflow_dispatchinputs arrive as strings, and an empty string is what lets the fall-through stay one expression..github/risk.jsonjudges the PR, and an empty?ref=is not an error to the contents API — it silently resolves to the default branch. Defaulting it would grade a PR against another branch's rules, which is the same failure the existing non-404 guard exists to prevent, arriving by a different door. It is not hypothetical: of five open PRs sampled on the pilot repo, two are stacked on feature branches.scripts/pr-risk/grade-targets.sh. It is per-target either way, so leaving it inline meant a second copy of the settle poll and the override contract for the by-number path. One copy — and it is unit-testable for the first time, which is what lets the claims above be tests rather than assertions.ungradedstill keeps the run green, on both paths. An unreadable PR is a reported verdict, not a broken run.if:are token guards — a bot's or a fork'spull_requestrun gets a read-onlyGITHUB_TOKEN, so the label write would 403 — and neither applies when a human presses the button. Fork risk is untouched:externalstill comes from the API's ownisCrossRepository, never the actor, and still grades R3.if:includesgithub.event.pull_request.head.repo.full_name == github.repository, which is false onworkflow_dispatch, so the job would skip silently — no run, no error, no annotation. Guard clauses are now scoped behindgithub.event_name != 'pull_request'.${{ github.workflow }}-${{ github.event.pull_request.number }}, which collapses to one constant group on dispatch, so withcancel-in-progressa batch would have each dispatch cancel the previous. The key now carries the resolved target.wait_for_checks_minuteson a backfill: measured rather than assumed. A dispatched run's own check attaches to the dispatched ref, not the PR's head commit, so it is absent from the graded PR's rollup and a settled PR reads its true state on the first poll — grading real tiers, not the R2 floor that0produces on the event path. It is still not free:0breaks out after a single read, ahead of the "require a settled reading to repeat" confirmation, so a target pushed to minutes ago lands the honest R2.1costs one 15s backoff plus a second read per PR. The header, the input description and the README all say prefer1, not0.Notes
--stdinis not the batch vehicle, despite being the obvious candidate. It grades pre-collected scorecard records fed to the grading jq — it performs no API read and cannot turn a PR number into a record. Batch is therefore a loop in the orchestration layer, where the per-target base ref and per-target override fetch have to live anyway.MAX_UNREADABLE_TRIES,READ_RETRY_BUDGET_SECONDS, plusPOLL_DELAY_SECONDS) purely so the suite can reach the unreadable-PR branch without sleeping through the production backoff. CI passes none of them, so the defaults are what runs.007to 7, so tolerating it would let7,007present as two targets for one PR and label it twice.test-pr-risk.yml's path filter now includespr-risk.yml, which passes the scripts their whole input contract through env and could otherwise break them without touching a file underscripts/pr-risk/.Test plan
Verified by unit test (hermetic —
ghstubbed on PATH, every call logged so the tests assert on which requests were made):ungradedkeeps the run green; a failed label write fails its targetSUCCESSand reversibility is not floored at R2one behaviour change there: a stale-label DELETE that 404s is the removal loop's goal state, not a failure
Verified live, read-only, against real open PRs on the pilot repo (
DRY_RUN=1, so no label was written):1094,1086,999999: both real PRs gradedR1; #1086's override was read from its feature-branch basematt/be-5532-cql-flat-pattern-costs, notmain— the load-bearing behaviour, against the real API999999failed at the base-ref hop and did not abandon the batch; the run reported2 graded, 0 ungraded, 1 failedwait_for_checks_minutes: 1is 15s per PR, matching the predicted single-backoff-plus-confirmationchecks_state=SUCCESS,pending=falseand a realR1— the empirical basis for the low-wait recommendationNot verified until this is merged and a caller pins it — stated plainly rather than claimed:
workflow_dispatchend-to-end run. It needs a consumer caller pinned to a SHA of this branch, which is awkward pre-merge; the reusable is only reachable through a caller'suses:GITHUB_TOKENis writable where a bot'spull_requesttoken is not is reasoned from GitHub's documented behaviour, not observed hereAfter merge, the natural first exercise is the backfill this was built for: bump the pilot consumer's caller to the new SHA, add the
workflow_dispatchblock from the header, and dispatch its open queue withwait_for_checks_minutes: 1. Running that backfill is deliberately a separate operational step, not part of this PR.