From d7e7fce7162e03aa87183b23b7b8f6bfdb6c5a3a Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Mon, 3 Aug 2026 14:53:13 -0700 Subject: [PATCH 1/4] feat(pr-risk): grade a pull request by number, on demand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .github/workflows/pr-risk.yml | 327 +++++++++++++------------ scripts/pr-risk/grade-targets.sh | 400 +++++++++++++++++++++++++++++++ 2 files changed, 574 insertions(+), 153 deletions(-) create mode 100755 scripts/pr-risk/grade-targets.sh diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index e540c7d..68bbcc5 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -26,6 +26,41 @@ name: PR Risk Grade (reusable) # out, then labels what it has. Pair the caller with a per-PR cancel-in-progress concurrency # group so a new push supersedes a waiting run instead of stacking behind it. # +# ON-DEMAND GRADING (`pr_number` / `pr_numbers`): supply a PR number and that PR is graded with +# no `pull_request` event involved — which is how a repo enrolling mid-stream grades the open +# queue it already has, and how a PR is re-graded after a `.github/risk.json` change or a +# `risk-dispute`. Absent, the workflow reads the event exactly as before. Three things differ on +# the by-number path, all of them deliberate: +# * BOT-AUTHORED PRs ARE GRADED. The `github.actor != 'dependabot[bot]'` clause callers put in +# their `if:` is a TOKEN guard, not a policy one: a bot-triggered `pull_request` run gets a +# read-only GITHUB_TOKEN, so the label write — this workflow's entire product — would 403. +# On a dispatch the actor is the human who pressed the button and the token is writable, so +# the reason evaporates. Excluding them would also bias any backfill corpus badly; bots +# author a substantial minority of merged PRs. The same is true of the fork clause, and for +# the same reason — fork RISK is unaffected either way, because `external` is derived from +# the API's `isCrossRepository`, never from the actor, and forks grade R3 with no exceptions. +# * THE BASE REF IS RE-READ FROM THE API, per target, because there is no event payload to take +# it from. It is load-bearing rather than cosmetic: it selects which branch's +# `.github/risk.json` judges the PR, live PRs are commonly stacked on feature branches rather +# than the default branch, and an empty ref is NOT an error to the contents API — it silently +# resolves to the default branch. So an unresolvable base ref FAILS that target instead of +# grading it against rules nobody read. +# * A LOW `wait_for_checks_minutes` IS RIGHT HERE, and is not the same trade-off as on the event +# path. The wait exists to outlast the rest of the rollup while our own check sits in it; a +# dispatched run's check is attached to the dispatched ref, not to the PR's head commit, so +# it is not in that rollup at all and a settled PR reads its true state (`SUCCESS`, nothing +# pending) on the first poll. It is still not FREE: `0` breaks out after a single read, ahead +# of both the not-yet-registered grace window and the "require a settled reading to repeat" +# confirmation, so a target someone pushed to minutes ago lands the honest R2 floor. `1` costs +# one 15s backoff and a second read per PR and keeps the confirmation. Prefer `1`, not `0`. +# +# Batch (`pr_numbers`) grades one target at a time and ONE UNREADABLE PR NEVER ABANDONS THE REST: +# each target's outcome is recorded, the whole list is attempted, and the run then reports whether +# any target failed. There is deliberately no `all_open: true` — an explicit list is bounded, +# reviewable and re-runnable, and the list is capped rather than silently truncated. A dispatch +# that reaches this workflow with no target at all FAILS LOUDLY instead of exiting 0: a button +# that silently grades nothing is worse than no button. +# # SECRETS: none. This workflow declares no `secrets:` inputs and callers pass none — the only # credential in the job is the automatic `GITHUB_TOKEN` (`github.token`), used for the PR read # and the one label write. There is no `secrets: inherit` to add and nothing to rotate. @@ -48,13 +83,40 @@ name: PR Risk Grade (reusable) # on: # pull_request: # types: [opened, synchronize, reopened, ready_for_review] +# workflow_dispatch: +# inputs: +# pr_number: +# description: Grade ONE pr by number (enrollment backfill, or a manual re-grade). +# required: false +# pr_numbers: +# description: Grade several, comma-separated (12,15,20). Wins over pr_number. +# required: false # concurrency: -# group: pr-risk-${{ github.event.pull_request.number }} +# # THE KEY MUST CARRY THE RESOLVED TARGET, not just the event PR. On a workflow_dispatch +# # `github.event.pull_request.number` is empty, so an event-only key collapses to ONE +# # constant group for every dispatch — and with cancel-in-progress that means each +# # dispatched PR cancels the one before it, which is precisely the shape a backfill has. +# # `inputs` is empty on a `pull_request` run, so this one expression is correct on both. +# group: ${{ github.workflow }}-${{ inputs.pr_numbers || inputs.pr_number || github.event.pull_request.number }} # cancel-in-progress: true # permissions: # contents: read # jobs: # pr-risk: +# # SCOPE EVERY GUARD CLAUSE TO THE EVENT IT DESCRIBES. Both clauses below are TOKEN +# # guards: a fork's and a bot's `pull_request` run gets a read-only GITHUB_TOKEN, so the +# # label write would 403 and the check would go red on every dependency bump. Neither is +# # a risk judgement — forks are graded R3 from the API's own fork flag, never from the +# # actor. On a workflow_dispatch there is no `github.event.pull_request`, so an +# # unscoped `head.repo.full_name == github.repository` is FALSE and the job SKIPS +# # SILENTLY: the dispatch button appears to do nothing, no run, no error, no annotation. +# # Keeping the clauses behind the event test is what makes the button work at all — and +# # on a dispatch the token is writable and the actor is a human, so there is nothing left +# # for either clause to protect. +# if: >- +# github.event_name != 'pull_request' || +# (github.actor != 'dependabot[bot]' && +# github.event.pull_request.head.repo.full_name == github.repository) # permissions: # contents: read # issues: write # create the risk:* labels repo-side on first use @@ -68,6 +130,20 @@ name: PR Risk Grade (reusable) # uses: Comfy-Org/github-workflows/.github/workflows/pr-risk.yml@ # v1 # with: # workflows_ref: +# # Both are empty on a `pull_request` run (`inputs` is empty there), which is exactly +# # the no-input event path — so ONE caller shape serves both event and dispatch and +# # there is nothing to keep in sync between two jobs. +# pr_number: ${{ inputs.pr_number }} +# pr_numbers: ${{ inputs.pr_numbers }} +# # For a backfill, dispatch with this lowered — see ON-DEMAND GRADING above for why a +# # low wait is sound on the by-number path and why `0` still is not the right value. +# # wait_for_checks_minutes: 1 +# +# A SKIPPED CALLER JOB IS INVISIBLE FROM HERE. This workflow cannot detect, warn about or recover +# from a caller whose `if:` excluded it — no run is created, so nothing of ours executes. The +# block above is the only lever, which is why the `if:` is spelled out rather than left to the +# enroller. What this workflow CAN do, and does, is refuse to be a silent no-op once it is +# actually reached: a run with no resolvable target fails with a message naming both inputs. # # GRANT ALL SIX OR THE RUN NEVER STARTS. A reusable workflow can only NARROW the caller's # token, never elevate it, so a caller whose block is short of what the `grade` job declares is @@ -80,6 +156,32 @@ name: PR Risk Grade (reusable) on: workflow_call: inputs: + pr_number: + description: >- + Grade ONE pull request by number instead of the event's. Leave it EMPTY on a + `pull_request` run: with no number supplied the target, the base ref and every emitted + label are exactly what they were before this input existed. Supplying it is what makes + grading possible without a `pull_request` event — the enrollment backfill of an + already-open queue, and the manual re-grade after a risk-map change or a + `risk-dispute`. Bot-authored and fork PRs ARE graded on this path (see the header). + Typed `string` rather than `number` because `workflow_dispatch` inputs arrive as + strings, and because an empty string is what lets the fall-through to the event's own + number stay a single expression. + type: string + required: false + default: '' + pr_numbers: + description: >- + Grade SEVERAL pull requests by number (comma-separated, e.g. `12,15,20`). Takes + precedence over `pr_number` when both are set. Targets are graded one at a time, and + one unreadable PR is REPORTED without abandoning the rest — the whole list is + attempted before the run decides its own outcome. Pair a long list with a low + `wait_for_checks_minutes`: the per-target waits are additive and the run stops starting + new targets once the job's budget is spent, reporting the un-attempted ones by number + rather than being cancelled mid-label. There is deliberately no `all_open: true`. + type: string + required: false + default: '' fleet_logins: description: >- GitHub logins whose PRs are supervised-agent output (comma-separated). @@ -107,12 +209,14 @@ on: default: '' wait_for_checks_minutes: description: >- - How long to wait for the REST of the check rollup to settle before + How long to wait, PER TARGET, for the REST of the check rollup to settle before labeling (the grading run itself is excluded from the rollup it reads). 0 labels immediately — expect R2 floors from still-pending checks. CLAMPED to what a 30-minute job can actually spend waiting (25), so an over-large value degrades to a shorter wait instead of a job - cancelled mid-sleep with the label never applied. + cancelled mid-sleep with the label never applied. On the by-number path a dispatched + run's own check is not in the graded PR's rollup at all, so `1` is usually enough + there; see ON-DEMAND GRADING in the header for why `0` still is not. type: number required: false default: 10 @@ -174,8 +278,17 @@ jobs: statuses: read # the same rollup's legacy commit-status contexts env: REPO: ${{ github.repository }} - PR_NUMBER: ${{ github.event.pull_request.number }} - BASE_REF: ${{ github.event.pull_request.base.ref }} + # The target list. `pr_numbers` wins over `pr_number`, and with neither supplied this is + # the event's own PR — so the event path resolves to the same single number it always did. + PR_NUMBERS: ${{ inputs.pr_numbers || inputs.pr_number || github.event.pull_request.number }} + # The event PR's base ref, passed through ONLY when no number was supplied. When one WAS, + # the target may be a different PR than the event's (or there may be no event at all), and + # the event's base ref would then point the override read at the wrong branch — so the + # by-number path re-reads it from the API per target instead. The expression reads oddly + # because GitHub's `||` yields the first TRUTHY operand: the `&& ... || ''` tail is how a + # conditional passthrough is spelled, and it yields '' both when a number was supplied and + # when there is no pull_request payload to read. + BASE_REF: ${{ (inputs.pr_number || inputs.pr_numbers) == '' && github.event.pull_request.base.ref || '' }} GH_TOKEN: ${{ github.token }} steps: - name: Load pr-risk tool @@ -188,175 +301,83 @@ jobs: ref: ${{ inputs.workflows_ref }} path: _pr_risk_tool persist-credentials: false - - - name: Fetch per-repo overrides from the base ref - id: overrides + - name: Grade the target PR(s) and sync the label + id: grade + # 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 — lives in + # scripts/pr-risk/grade-targets.sh rather than inline here, so the event path and the + # by-number path cannot drift into two copies of it, and so it is unit-testable at all + # (tests/test_grade_targets.sh drives every branch with a stubbed `gh`). It grades ONE + # target or fifty through the same code; a batch's per-target failures are recorded and + # the remaining targets still graded. env: MAP_PATH: ${{ inputs.repo_map_path }} RB_PATH: ${{ inputs.repo_runbooks_path }} - run: | - set -uo pipefail - # Read from the BASE ref, so the PR being graded cannot edit the rules - # that judge it. Absent (a genuine 404) falls back to the shipped - # defaults; present-but-invalid is left for the grader's structural - # validation to reject loudly — a repo that commits a corrupt map must - # see red, not silent generic grading. - # - # ONLY A 404 MEANS ABSENT. Treating any non-zero exit as "no override" - # meant a 403 rate-limit, a 5xx or a network blip silently graded the - # PR against the generic default map instead of the repo's sharpened - # one — a LOWER tier computed from an input nobody read, which is the - # confident-answer-from-an-unread-source failure the unknown contract - # forbids everywhere else. So the status code is captured and anything - # that is not 200-or-404 fails the run. - errf="$(mktemp)" - fetch_override() { # -> prints the outfile, or nothing when absent - local p="$1" out="$2" - if gh api "repos/${REPO}/contents/${p}?ref=${BASE_REF}" \ - -H "Accept: application/vnd.github.raw" > "$out" 2>"$errf"; then - echo "using ${p} from ${BASE_REF}" >&2 - printf '%s' "$out" - elif grep -q '(HTTP 404)' "$errf"; then - rm -f "$out" # the redirect already created it empty; nothing must read it - echo "no ${p} on ${BASE_REF} — using the generic default" >&2 - else - echo "::error::could not read ${p} from ${BASE_REF}: $(tr '\n' ' ' < "$errf"). NOT falling back to the generic default: that would grade this PR against rules nobody read." >&2 - return 1 - fi - } - map="$(fetch_override "$MAP_PATH" repo-risk-map.json)" || exit 1 - rb="$(fetch_override "$RB_PATH" repo-risk-runbooks.json)" || exit 1 - rm -f "$errf" - echo "map=$map" >> "$GITHUB_OUTPUT" - echo "runbooks=$rb" >> "$GITHUB_OUTPUT" - - - name: Grade (waits for the rest of the rollup to settle) - id: grade - env: FLEET_LOGINS: ${{ inputs.fleet_logins }} BOT_LOGINS: ${{ inputs.bot_logins }} + LABEL_MAP: ${{ inputs.label_map }} SELF_CONTEXT: ${{ github.workflow }} SELF_RUN_ID: ${{ github.run_id }} WAIT_MINUTES: ${{ inputs.wait_for_checks_minutes }} - MAP_OVERRIDE: ${{ steps.overrides.outputs.map }} - RB_OVERRIDE: ${{ steps.overrides.outputs.runbooks }} JOB_TIMEOUT_MINUTES: 30 # keep in sync with timeout-minutes on the job + TOOL_DIR: _pr_risk_tool/scripts/pr-risk run: | set -uo pipefail - TOOL="_pr_risk_tool/scripts/pr-risk/grade-pr-risk.sh" - ARGS=( --repo "$REPO" --pr "$PR_NUMBER" - --self-context "$SELF_CONTEXT" --self-run-id "$SELF_RUN_ID" ) - [ -n "$FLEET_LOGINS" ] && ARGS+=( --fleet-logins "$FLEET_LOGINS" ) - [ -n "$BOT_LOGINS" ] && ARGS+=( --bot-logins "$BOT_LOGINS" ) - [ -n "$MAP_OVERRIDE" ] && ARGS+=( --map "$MAP_OVERRIDE" ) - [ -n "$RB_OVERRIDE" ] && ARGS+=( --runbooks "$RB_OVERRIDE" ) - - # CLAMP THE WAIT TO THE JOB'S OWN BUDGET rather than trusting the input - # description. A caller who passes 40 against a 30-minute timeout gets a - # job cancelled mid-sleep: the label step never runs, the PR keeps - # whatever label the previous push left, and the check goes red for a - # reason nobody would guess. 5 minutes of headroom is left for the - # label + summary steps. - max_wait=$(( JOB_TIMEOUT_MINUTES - 5 )) - wait_minutes="${WAIT_MINUTES:-10}" - [ "$wait_minutes" -ge 0 ] 2>/dev/null || wait_minutes=10 - if [ "$wait_minutes" -gt "$max_wait" ]; then - echo "::warning::wait_for_checks_minutes=${wait_minutes} exceeds what a ${JOB_TIMEOUT_MINUTES}-minute job can spend waiting — clamped to ${max_wait}" - wait_minutes="$max_wait" - fi - deadline=$(( $(date +%s) + 60 * wait_minutes )) - empty_deadline=$(( $(date +%s) + 120 )) # grace for checks to REGISTER at all - waited=0 - # Poll backoff: 15s doubling to 120s. The fixed 30s poll idled a runner - # up to the full wait on every `synchronize` event, per PR, across the - # fleet, for a grade that is one API call — increasing backoff cuts both - # runner minutes and API calls without changing the settle behaviour. - delay=15 - settled_once=0 - # rc=3 (the PR could not be read) covers rate limits, secondary rate - # limits and transient 5xx as well as a genuinely missing PR. Labeling - # `ungraded` on the first blip turns a momentary hiccup into a durable - # verdict, so it is retried with backoff. This gets its OWN budget, not - # the settle deadline: a transient API failure has nothing to do with - # how long the caller wants to wait for CI, and sharing the deadline - # would leave `wait_for_checks_minutes: 0` with no retry at all. - unreadable_tries=0 - max_unreadable_tries=4 - read_deadline=$(( $(date +%s) + 150 )) - read_delay=10 - while :; do - rc=0 - bash "$TOOL" "${ARGS[@]}" > record.json || rc=$? - case "$rc" in - 0|1) ;; # graded (1 = graded but unknown — still labeled) - 3) unreadable_tries=$(( unreadable_tries + 1 )) - if [ "$unreadable_tries" -lt "$max_unreadable_tries" ] && [ "$(date +%s)" -lt "$read_deadline" ]; then - echo "PR read failed (attempt ${unreadable_tries}/${max_unreadable_tries}) — retrying in ${read_delay}s" - sleep "$read_delay"; waited=$(( waited + read_delay )) - read_delay=$(( read_delay * 2 )); [ "$read_delay" -le 60 ] || read_delay=60 - continue - fi - echo "PR unreadable via the API after ${unreadable_tries} attempt(s) — labeling ungraded" - printf '{}' > record.json - break ;; - *) echo "grader failed (rc=$rc)"; exit 1 ;; # setup/usage bug: fail loud - esac - pending="$(jq -r '.checks_pending_excl_self // false' record.json)" - state="$(jq -r '.checks_state // "none"' record.json)" - now="$(date +%s)" - [ "$now" -lt "$deadline" ] || break - if [ "$pending" = "true" ]; then - settled_once=0 - elif [ "$state" = "none" ] && [ "$now" -lt "$empty_deadline" ]; then - # An EMPTY rollup is checks that have not registered yet, not CI - # that finished — exiting the poll here would apply a label that a - # check appearing a second later never revises. Bounded by a SHORT - # grace window rather than the whole budget, so a repo with - # genuinely no CI is not made to wait 25 minutes on every PR to - # arrive at the same honest R2. - settled_once=0 - elif [ "$settled_once" -eq 0 ]; then - # First settled reading. Require it to REPEAT before labeling: one - # transiently-quiet poll is not a settled rollup. - settled_once=1 - else - break - fi - sleep "$delay"; waited=$(( waited + delay )) - delay=$(( delay * 2 )); [ "$delay" -le 120 ] || delay=120 - done - tier="$(jq -r '.risk.tier // "unknown"' record.json)" - echo "tier=$tier" >> "$GITHUB_OUTPUT" - echo "waited=$waited" >> "$GITHUB_OUTPUT" - echo "graded tier: $tier (waited ${waited}s for the rollup)" - - - name: Apply risk label - env: - TIER: ${{ steps.grade.outputs.tier }} - LABEL_MAP: ${{ inputs.label_map }} - run: | - set -uo pipefail - bash _pr_risk_tool/scripts/pr-risk/apply-risk-label.sh + bash _pr_risk_tool/scripts/pr-risk/grade-targets.sh - name: Step summary if: always() env: TIER: ${{ steps.grade.outputs.tier }} WAITED: ${{ steps.grade.outputs.waited }} + TARGETS: ${{ steps.grade.outputs.targets }} + GRADED: ${{ steps.grade.outputs.graded }} + UNGRADED: ${{ steps.grade.outputs.ungraded }} + FAILED: ${{ steps.grade.outputs.failed }} + SKIPPED: ${{ steps.grade.outputs.skipped }} run: | set -uo pipefail + RESULTS=pr-risk-results.jsonl + [ -f "$RESULTS" ] || : > "$RESULTS" { - echo "## PR risk grade: ${TIER:-unknown} (advisory)" - echo - if [ -s record.json ] && jq -e '.risk' record.json >/dev/null 2>&1; then - jq -r ' - "Grade = worst(path_floor, provenance, reversibility) — \(.risk.reason)\n", - "| axis | tier | reason |", "|---|---|---|", - (.risk.axes | to_entries[] | "| \(.key) | \(.value.tier // "unknown") | \(.value.reason) |"), - "\nMap `\(.risk.map_version)` · registry `\(.risk.registry_version)` · waited \($waited)s for the rollup · checks `\(.checks_state // "none")`" - ' --arg waited "${WAITED:-0}" record.json + if [ "${TARGETS:-1}" = 1 ] || [ ! -s "$RESULTS" ]; then + # ONE target: the per-axis table, unchanged — this is what the event path renders. + one="$(jq -sc '.[0] // {}' "$RESULTS" 2>/dev/null || echo '{}')" + rec="$(jq -r '.record // ""' <<<"$one")" + st="$(jq -r '.status // ""' <<<"$one")" + note="$(jq -r '.note // ""' <<<"$one")" + echo "## PR risk grade: ${TIER:-unknown} (advisory)" + echo + if [ -n "$rec" ] && [ -s "$rec" ] && jq -e '.risk' "$rec" >/dev/null 2>&1; then + jq -r ' + "Grade = worst(path_floor, provenance, reversibility) — \(.risk.reason)\n", + "| axis | tier | reason |", "|---|---|---|", + (.risk.axes | to_entries[] | "| \(.key) | \(.value.tier // "unknown") | \(.value.reason) |"), + "\nMap `\(.risk.map_version)` · registry `\(.risk.registry_version)` · waited \($waited)s for the rollup · checks `\(.checks_state // "none")`" + ' --arg waited "${WAITED:-0}" "$rec" + elif [ "$st" = failed ] && [ -n "$note" ]; then + # A target that never reached the grader (an unresolvable base ref, an override + # file that would not read) says WHICH input failed. Reporting it as "could not + # be read via the API" would name the wrong cause and imply a label was applied. + echo "Nothing was graded: ${note}" + elif [ ! -s "$RESULTS" ]; then + # No target was even attempted — a usage error (an empty or malformed target + # list), which is a different failure from an unreadable PR and must not borrow + # its wording: no PR was named, so none "could not be read". + echo "No pull request was graded: the run resolved no target. On a \`workflow_dispatch\`, supply \`pr_number\` or \`pr_numbers\` — see the step log for the exact reason." + else + echo "The PR could not be read via the API; nothing was graded (labeled ungraded — this is NOT a low-risk verdict)." + fi else - echo "The PR could not be read via the API; nothing was graded (labeled ungraded — this is NOT a low-risk verdict)." + # A BATCH has no single tier and no room for N axis tables, so it reports one row + # per target. `not attempted` is listed as its own outcome rather than folded into + # the failures: those PRs are un-graded, not mis-graded, and re-dispatching exactly + # those numbers is the fix. + echo "## PR risk grades: ${GRADED:-0} graded, ${UNGRADED:-0} ungraded, ${FAILED:-0} failed, ${SKIPPED:-0} not attempted (advisory)" + echo + echo "| pr | tier | label | base ref | waited | note |" + echo "|---|---|---|---|---|---|" + jq -r '"| #\(.pr) | \(.tier // "—") | \(.label // "—") | `\(.base_ref // "—")` | \(.waited)s | \(.note // "") |"' "$RESULTS" fi echo echo "This label routes nothing and gates nothing. Disagree with the grade? Add the \`risk-dispute\` label and say why in a comment — the grader never touches that label." diff --git a/scripts/pr-risk/grade-targets.sh b/scripts/pr-risk/grade-targets.sh new file mode 100755 index 0000000..2f47073 --- /dev/null +++ b/scripts/pr-risk/grade-targets.sh @@ -0,0 +1,400 @@ +#!/usr/bin/env bash +# grade-targets.sh — grade and label N pull requests, one target at a time. The orchestration +# layer of the reusable pr-risk.yml workflow: it sequences the four per-PR steps (resolve the +# base ref, fetch that ref's override files, poll the grader until the rest of the check rollup +# settles, sync the one label) and computes nothing about risk itself. The tier comes from +# grade-pr-risk.sh; the label write is apply-risk-label.sh's. +# +# EXTRACTED from pr-risk.yml's inline job body, unchanged in behaviour, for two reasons: +# * ONE code path now serves all three ways a grade can be asked for — the `pull_request` +# event, a `workflow_dispatch` naming one PR, and a dispatched comma-separated batch. The +# alternative was a second copy of the settle-poll and the override fetch for the by-number +# path, and this repo already knows what two copies of one grader cost (grade-pr-risk.sh's +# own header records that lesson). +# * It is TESTABLE. As inline YAML the poll loop, the 404-vs-error override contract and the +# per-target failure isolation had no unit coverage at all; tests/test_grade_targets.sh now +# drives every branch of them with a stubbed `gh`. +# +# ONE UNREADABLE TARGET NEVER ABANDONS THE REST. Each target is attempted independently and its +# outcome recorded; the run's exit code reports whether ANY target failed, but only after every +# target has had its turn. A 30-PR backfill that hits one deleted PR still labels the other 29. +# +# Inputs (env): +# REPO owner/name of the repo holding the PRs (required) +# PR_NUMBERS target PR numbers, comma/space/newline separated (required) +# BASE_REF base ref for the SINGLE target, when the caller already has it from +# the event payload. Honoured ONLY when there is exactly one target; +# empty means "resolve it from the API per target". +# MAP_PATH / RB_PATH per-repo override paths, read from each target's BASE ref +# TOOL_DIR directory holding grade-pr-risk.sh + apply-risk-label.sh +# (default: beside this script) +# FLEET_LOGINS BOT_LOGINS SELF_CONTEXT SELF_RUN_ID passed through to the grader +# WAIT_MINUTES per-target settle wait (default 10) +# JOB_TIMEOUT_MINUTES the calling job's timeout-minutes (default 30) — the whole run is +# budgeted against it, not just each target +# LABEL_MAP passed through to apply-risk-label.sh +# MAX_TARGETS refuse a list longer than this (default 50) +# RESULTS per-target JSONL outcome file (default pr-risk-results.jsonl) +# DRY_RUN 1 = grade and report, write no label +# GH_TOKEN token for gh +# GITHUB_OUTPUT when set, tier / waited / targets / graded / ungraded / failed / +# skipped are appended +# +# Exit: 0 = every target ended with a label in sync (a `risk:ungraded` label IS in sync — an +# unreadable PR is a reported verdict, not a broken run, exactly as on the event path). +# 1 = at least one target could not be graded or labeled, after all of them were tried. +# 2 = usage/setup error: nothing was attempted. +# +# Deliberately bash (shebang), not zsh — CI runners and the test suite both exercise bash. + +set -uo pipefail + +SELF_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +TOOL_DIR="${TOOL_DIR:-$SELF_DIR}" +GRADER="$TOOL_DIR/grade-pr-risk.sh" +LABELER="$TOOL_DIR/apply-risk-label.sh" + +REPO="${REPO:-}" +PR_NUMBERS="${PR_NUMBERS:-}" +BASE_REF="${BASE_REF:-}" +MAP_PATH="${MAP_PATH:-.github/risk.json}" +RB_PATH="${RB_PATH:-.github/risk-runbooks.json}" +FLEET_LOGINS="${FLEET_LOGINS:-}" +BOT_LOGINS="${BOT_LOGINS:-}" +SELF_CONTEXT="${SELF_CONTEXT:-}" +SELF_RUN_ID="${SELF_RUN_ID:-}" +WAIT_MINUTES="${WAIT_MINUTES:-10}" +JOB_TIMEOUT_MINUTES="${JOB_TIMEOUT_MINUTES:-30}" +LABEL_MAP="${LABEL_MAP:-}" +MAX_TARGETS="${MAX_TARGETS:-50}" +RESULTS="${RESULTS:-pr-risk-results.jsonl}" +DRY_RUN="${DRY_RUN:-0}" +# The two retry/backoff constants are env-overridable ONLY so the suite can exercise the +# unreadable-PR and settle-repeat branches without sleeping through the production backoff. CI +# passes neither, so the values below are what runs in production. +MAX_UNREADABLE_TRIES="${MAX_UNREADABLE_TRIES:-4}" +READ_RETRY_BUDGET_SECONDS="${READ_RETRY_BUDGET_SECONDS:-150}" +POLL_DELAY_SECONDS="${POLL_DELAY_SECONDS:-15}" + +# Set by the per-target helpers, read by their callers. +G_TIER="" +G_WAITED=0 +TARGET_COUNT=1 +OVERALL_DEADLINE=0 + +log() { printf '[grade-targets] %s\n' "$*" >&2; } +die() { printf '[grade-targets] ERROR %s\n' "$*" >&2; exit 2; } + +ERRF="$(mktemp "${TMPDIR:-/tmp}/grade-targets-err.XXXXXX")" || die "mktemp failed" +LABELF="$(mktemp "${TMPDIR:-/tmp}/grade-targets-label.XXXXXX")" || die "mktemp failed" +trap 'rm -f "$ERRF" "$LABELF"' EXIT +gherr() { tr '\n' ' ' < "$ERRF" | sed 's/[[:space:]]*$//'; } + +# ---- targets --------------------------------------------------------------------------------- +# A PR number is a positive integer with NO leading zeros. `007` is rejected rather than +# normalised: GitHub would resolve it to 7, so tolerating it would let `7,007` present as two +# targets for one PR and label it twice, the second write racing the first. +parse_targets() { # -> one validated, de-duplicated number per line + local raw="$1" norm n seen=" " nums=() fields=() + norm="$(tr ',\n\t' ' ' <<<"$raw")" + read -ra fields <<<"$norm" + for n in "${fields[@]}"; do + [ -n "$n" ] || continue + [[ "$n" =~ ^[1-9][0-9]*$ ]] \ + || die "bad PR number '$n' in PR_NUMBERS — want positive integers, comma-separated" + case "$seen" in *" $n "*) log "target $n listed twice — grading it once"; continue ;; esac + seen="$seen$n " + nums+=("$n") + done + [ "${#nums[@]}" -gt 0 ] \ + || die "no PR numbers to grade. On the event path PR_NUMBERS comes from the pull_request payload; on a workflow_dispatch the caller must supply pr_number or pr_numbers. Refusing to exit 0 on an empty target list — a dispatch that silently grades nothing is worse than no dispatch." + [ "${#nums[@]}" -le "$MAX_TARGETS" ] \ + || die "${#nums[@]} targets exceeds MAX_TARGETS=$MAX_TARGETS — split the backfill into batches. A list this long cannot finish inside the job's timeout, and a run cancelled mid-batch leaves an arbitrary prefix labeled with no record of where it stopped." + printf '%s\n' "${nums[@]}" +} + +# ---- the base ref ---------------------------------------------------------------------------- +# WHY AN UNRESOLVED REF IS FATAL RATHER THAN DEFAULTED. The ref is interpolated into the override +# read below as `contents/${p}?ref=${base}`, and an EMPTY ref is not an error to that endpoint — +# GitHub resolves it to the repository's DEFAULT branch. So a base ref we failed to read would +# silently fetch some other branch's .github/risk.json (or fall back to the generic default when +# the PR's real base carries an override the default branch does not) and grade the PR against +# rules nobody read. That is the same failure the non-404 guard in fetch_override exists to +# prevent, arriving by a different door. Stacked PRs make it concrete: base refs on live PRs in +# the pilot repo include feature branches, not just `main`. +resolve_base_ref() { # -> ref on stdout, rc 1 (reason already annotated on stderr) + local num="$1" ref rc + ref="$(gh api "repos/${REPO}/pulls/${num}" --jq '.base.ref' 2>"$ERRF")"; rc=$? + if [ "$rc" -ne 0 ]; then + echo "::error::could not read the base ref of ${REPO}#${num}: $(gherr). NOT grading it against the default branch's rules." >&2 + return 1 + fi + ref="${ref%$'\n'}" + case "$ref" in + ""|null) + echo "::error::the base ref of ${REPO}#${num} read back empty — refusing to fall through to the repository default branch, which would grade this PR against another branch's rules." >&2 + return 1 ;; + esac + printf '%s' "$ref" +} + +# ---- the per-repo overrides ------------------------------------------------------------------- +# Read from the target's BASE ref, so the PR being graded cannot edit the rules that judge it. +# Absent (a genuine 404) falls back to the shipped defaults; present-but-invalid is left for the +# grader's structural validation to reject loudly — a repo that commits a corrupt map must see +# red, not silent generic grading. +# +# ONLY A 404 MEANS ABSENT. Treating any non-zero exit as "no override" meant a 403 rate-limit, a +# 5xx or a network blip silently graded the PR against the generic default map instead of the +# repo's sharpened one — a LOWER tier computed from an input nobody read, which is the +# confident-answer-from-an-unread-source failure the unknown contract forbids everywhere else. So +# the status code is captured and anything that is not 200-or-404 fails the target. +fetch_override() { # -> prints the outfile, or nothing when absent + local p="$1" out="$2" base="$3" + if gh api "repos/${REPO}/contents/${p}?ref=${base}" \ + -H "Accept: application/vnd.github.raw" > "$out" 2>"$ERRF"; then + echo "using ${p} from ${base}" >&2 + printf '%s' "$out" + elif grep -q '(HTTP 404)' "$ERRF"; then + rm -f "$out" # the redirect already created it empty; nothing must read it + echo "no ${p} on ${base} — using the generic default" >&2 + else + echo "::error::could not read ${p} from ${base}: $(gherr). NOT falling back to the generic default: that would grade this PR against rules nobody read." >&2 + return 1 + fi +} + +# ---- grade one target, waiting for the rest of the rollup to settle --------------------------- +# Sets G_TIER and G_WAITED. rc 0 = there is a record to label (including the deliberate `unknown` +# one), rc 1 = the grader itself failed and nothing may be labeled. +# +# CHECKS SETTLE BEFORE THE LABEL DOES: the reversibility axis asks "did tests covering these +# lines actually run", and at event time the rest of the rollup is usually still running. The +# grading job excludes its own RUN from the rollup it reads, so what it waits out is the OTHER +# checks. On a workflow_dispatch the grading run's own check is attached to the dispatched ref, +# not to the PR's head commit, so there is nothing of ours in that rollup to wait out and a +# settled PR reads its true state on the first poll — which is what makes a LOW wait the right +# setting for a backfill, and why a low wait there does not reproduce the R2 floors that +# `wait_for_checks_minutes: 0` produces on the event path. +settle_grade() { # + local num="$1" record="$2" map_override="$3" rb_override="$4" deadline="$5" + local args=( --repo "$REPO" --pr "$num" ) + [ -z "$SELF_CONTEXT" ] || args+=( --self-context "$SELF_CONTEXT" ) + [ -z "$SELF_RUN_ID" ] || args+=( --self-run-id "$SELF_RUN_ID" ) + [ -z "$FLEET_LOGINS" ] || args+=( --fleet-logins "$FLEET_LOGINS" ) + [ -z "$BOT_LOGINS" ] || args+=( --bot-logins "$BOT_LOGINS" ) + [ -z "$map_override" ] || args+=( --map "$map_override" ) + [ -z "$rb_override" ] || args+=( --runbooks "$rb_override" ) + + local empty_deadline pending state now delay settled_once rc + # Grace for checks to REGISTER at all. + empty_deadline=$(( $(date +%s) + 120 )) + G_WAITED=0 + # Poll backoff: 15s doubling to 120s. The fixed 30s poll idled a runner up to the full wait on + # every `synchronize` event, per PR, across the fleet, for a grade that is one API call — + # increasing backoff cuts both runner minutes and API calls without changing settle behaviour. + delay="$POLL_DELAY_SECONDS" + settled_once=0 + # rc=3 (the PR could not be read) covers rate limits, secondary rate limits and transient 5xx + # as well as a genuinely missing PR. Labeling `ungraded` on the first blip turns a momentary + # hiccup into a durable verdict, so it is retried with backoff. This gets its OWN budget, not + # the settle deadline: a transient API failure has nothing to do with how long the caller wants + # to wait for CI, and sharing the deadline would leave `wait_for_checks_minutes: 0` with no + # retry at all. It IS capped by the whole run's budget, so one dead PR in a batch cannot eat + # the time the remaining targets need. + local unreadable_tries=0 max_unreadable_tries="$MAX_UNREADABLE_TRIES" read_deadline read_delay=10 + read_deadline=$(( $(date +%s) + READ_RETRY_BUDGET_SECONDS )) + [ "$read_deadline" -le "$OVERALL_DEADLINE" ] || [ "$TARGET_COUNT" -eq 1 ] || read_deadline="$OVERALL_DEADLINE" + while :; do + rc=0 + bash "$GRADER" "${args[@]}" > "$record" || rc=$? + case "$rc" in + 0|1) ;; # graded (1 = graded but unknown — still labeled) + 3) unreadable_tries=$(( unreadable_tries + 1 )) + if [ "$unreadable_tries" -lt "$max_unreadable_tries" ] && [ "$(date +%s)" -lt "$read_deadline" ]; then + log "PR read failed (attempt ${unreadable_tries}/${max_unreadable_tries}) — retrying in ${read_delay}s" + sleep "$read_delay"; G_WAITED=$(( G_WAITED + read_delay )) + read_delay=$(( read_delay * 2 )); [ "$read_delay" -le 60 ] || read_delay=60 + continue + fi + log "PR unreadable via the API after ${unreadable_tries} attempt(s) — labeling ungraded" + printf '{}' > "$record" + break ;; + *) log "grader failed for ${REPO}#${num} (rc=$rc)"; G_TIER=""; return 1 ;; # setup/usage bug + esac + pending="$(jq -r '.checks_pending_excl_self // false' "$record")" + state="$(jq -r '.checks_state // "none"' "$record")" + now="$(date +%s)" + [ "$now" -lt "$deadline" ] || break + if [ "$pending" = "true" ]; then + settled_once=0 + elif [ "$state" = "none" ] && [ "$now" -lt "$empty_deadline" ]; then + # An EMPTY rollup is checks that have not registered yet, not CI that finished — exiting + # the poll here would apply a label that a check appearing a second later never revises. + # Bounded by a SHORT grace window rather than the whole budget, so a repo with genuinely no + # CI is not made to wait 25 minutes on every PR to arrive at the same honest R2. + settled_once=0 + elif [ "$settled_once" -eq 0 ]; then + # First settled reading. Require it to REPEAT before labeling: one transiently-quiet poll + # is not a settled rollup. + settled_once=1 + else + break + fi + sleep "$delay"; G_WAITED=$(( G_WAITED + delay )) + delay=$(( delay * 2 )); [ "$delay" -le 120 ] || delay=120 + done + G_TIER="$(jq -r '.risk.tier // "unknown"' "$record")" + return 0 +} + +# ---- one target, end to end ------------------------------------------------------------------- +record_result() { #