[PR #113] [MERGED] feat(pr-risk): grade a pull request by number on demand (workflow_dispatch) #122

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/pr-risk-dispatch-by-number

📝 Commits (4)

d7e7fce feat(pr-risk): grade a pull request by number, on demand
7b67ee0 test(pr-risk): cover the by-number path, base-ref resolution and batch isolation
2f24371 docs(pr-risk): document the on-demand grading surface
facd005 fix(pr-risk): encode the ref, retry the pre-grader reads, stop the summary claiming a label it did not apply

📊 Changes

7 files changed (+1324 additions, -157 deletions)

View changed files

📝 .github/workflows/pr-risk.yml (+233 -153)
📝 .github/workflows/test-pr-risk.yml (+11 -1)
📝 scripts/pr-risk/README.md (+85 -0)
📝 scripts/pr-risk/apply-risk-label.sh (+14 -3)
➕ scripts/pr-risk/grade-targets.sh (+524 -0)
📝 scripts/pr-risk/tests/test_apply_risk_label.sh (+40 -0)
➕ scripts/pr-risk/tests/test_grade_targets.sh (+417 -0)

📄 Description

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:

A repo that enrolls mid-stream cannot grade the queue it already has. The first consumer enrollment left exactly one PR ever graded — its own — and 30 others unlabeled until each independently receives a 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.
There is no manual re-grade. After a .github/risk.json change, or on a PR carrying the human-owned risk-dispute label, 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

Authored by: agent session, worktree off origin/main at 1c9443a
Verified: actionlint clean on both workflows; YAML parses; shellcheck -x clean on all six shell files; test_grade_pr_risk.sh 49 passed / 0 failed and untouched; test_apply_risk_label.sh 24 passed / 0 failed; new test_grade_targets.sh 91 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).
Deviations: the --stdin mode is not the batch vehicle — see Notes.
Review round (facd005): ten panel findings resolved — the ref/path are now percent-encoded into the contents
read (a raw # in a branch name truncated it into the silent default-branch read this PR exists to prevent), a
404 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

Type: feature — additive optional inputs on the reusable, plus a refactor that moves the per-target sequence out of the job body into a script
Slots into: the advisory pr-risk shadow pilot. The label remains the entire product; nothing gates, routes, or merges
Blast radius: consumers pin full SHAs, so nobody moves until they bump their own pin. The backward-compatibility claim is asserted against the stubbed gh call log, not stated — see Test plan

Summary

Two optional workflow_call inputs. pr_number grades one PR; pr_numbers grades a comma-separated list and wins when both are set. PR_NUMBERS resolves to pr_numbers || pr_number || github.event.pull_request.number, so the event path is the same single number it always was. Both are string, not number: workflow_dispatch inputs arrive as strings, and an empty string is what lets the fall-through stay one expression.
The base ref is re-read from the API per target, and an unresolvable one FAILS that target. This is the load-bearing half. The ref selects which branch's .github/risk.json judges 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.
The event path spends no extra API call. When a base ref arrives from the payload it is used as-is. A supplied number, though, ignores it — the target may be a different PR than the event's.
Batch never abandons the rest for one bad target. Every target is attempted and its outcome 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; a run cancelled mid-label is the one outcome with no signal at all, because the summary step never renders either.
The per-target sequence moves 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. One copy — and it is unit-testable for the first time, which is what lets the claims above be tests rather than assertions.
ungraded still keeps the run green, on both paths. An unreadable PR is a reported verdict, not a broken run.
Bot-authored and fork PRs grade on the dispatch path. The actor and fork clauses in a caller's if: are token guards — a bot's or a fork's pull_request run gets a read-only GITHUB_TOKEN, so the label write would 403 — and neither applies when a human presses the button. Fork risk is untouched: external still comes from the API's own isCrossRepository, never the actor, and still grades R3.
The header's copy-paste caller block is corrected for both event shapes, and this is not cosmetic — the live consumer caller trips both traps today:
- Its if: includes github.event.pull_request.head.repo.full_name == github.repository, which is false on workflow_dispatch, so the job would skip silently — no run, no error, no annotation. Guard clauses are now scoped behind github.event_name != 'pull_request'.
- Its concurrency group is ${{ github.workflow }}-${{ github.event.pull_request.number }}, which collapses to one constant group on dispatch, so with cancel-in-progress a batch would have each dispatch cancel the previous. The key now carries the resolved target.
- A caller-side skip is undetectable from inside the reusable (no run is created), so the header is the only lever. What the reusable can do, and now does, is refuse to be a silent no-op once reached: a run resolving no target fails loudly naming both inputs.
wait_for_checks_minutes on 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 that 0 produces on the event path. It is still not free: 0 breaks 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. 1 costs one 15s backoff plus a second read per PR. The header, the input description and the README all say prefer 1, not 0.

Notes

--stdin is 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.
Two retry constants gained env overrides (MAX_UNREADABLE_TRIES, READ_RETRY_BUDGET_SECONDS, plus POLL_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.
The target list is capped at 50 and rejects non-numeric, leading-zero and duplicate entries. Leading zeros are rejected rather than normalised: GitHub resolves 007 to 7, so tolerating it would let 7,007 present as two targets for one PR and label it twice.
test-pr-risk.yml's 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/.

Test plan

Verified by unit test (hermetic — gh stubbed on PATH, every call logged so the tests assert on which requests were made):

The event path issues no base-ref PR read and fetches the override from the payload's ref — the backward-compat claim, asserted against the call log
The by-number path resolves the base ref from the API and reads both override files from that ref, never from an empty one
An unresolvable base ref, and an empty one, fail their target having read no override and never invoked the grader
A non-404 override read fails the target with no tier invented and no label applied
A batch of three whose middle target is unreadable still grades the first and last
ungraded keeps the run green; a failed label write fails its target
Forks grade R3 from the API-derived fork flag; bot-authored PRs grade at all
A run id absent from the rollup (the dispatch shape) excludes nothing, so a settled rollup reads SUCCESS and reversibility is not floored at R2
Empty / non-numeric / leading-zero / over-long target lists are refused with exit 2, not silently partially graded
The grader suite passes untouched (49). The label suite grew by 3 (21 -> 24) in the review round, for the
one 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):

Batch of 1094,1086,999999: both real PRs graded R1; #1086's override was read from its feature-branch base matt/be-5532-cql-flat-pattern-costs, not main — the load-bearing behaviour, against the real API
The nonexistent 999999 failed at the base-ref hop and did not abandon the batch; the run reported 2 graded, 0 ungraded, 1 failed
Cost at wait_for_checks_minutes: 1 is 15s per PR, matching the predicted single-backoff-plus-confirmation
Grading a settled PR with a run id absent from its rollup yields checks_state=SUCCESS, pending=false and a real R1 — the empirical basis for the low-wait recommendation

Not verified until this is merged and a caller pins it — stated plainly rather than claimed:

A true workflow_dispatch end-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's uses:
The label write on the dispatch path. Locally it was dry-run, and the claim that a dispatch's GITHUB_TOKEN is writable where a bot's pull_request token is not is reasoned from GitHub's documented behaviour, not observed here
That a real dispatch's check run is genuinely absent from the graded PR's rollup. The unit test proves the grader handles a foreign run id correctly, which is the same code path, but the attachment behaviour itself is GitHub's

After 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_dispatch block from the header, and dispatch its open queue with wait_for_checks_minutes: 1. Running that backfill is deliberately a separate operational step, not part of this PR.

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

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/113 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/3/2026 **Status:** ✅ Merged **Merged:** 8/3/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/pr-risk-dispatch-by-number` --- ### 📝 Commits (4) - [`d7e7fce`](https://github.com/Comfy-Org/github-workflows/commit/d7e7fce7162e03aa87183b23b7b8f6bfdb6c5a3a) feat(pr-risk): grade a pull request by number, on demand - [`7b67ee0`](https://github.com/Comfy-Org/github-workflows/commit/7b67ee0963ee3ed3f6c351b82843157565ead214) test(pr-risk): cover the by-number path, base-ref resolution and batch isolation - [`2f24371`](https://github.com/Comfy-Org/github-workflows/commit/2f243710c5d92ab6bb6a20c9cc078aa481c99a82) docs(pr-risk): document the on-demand grading surface - [`facd005`](https://github.com/Comfy-Org/github-workflows/commit/facd005f3b2fed97f73b576e289bddb46540506a) fix(pr-risk): encode the ref, retry the pre-grader reads, stop the summary claiming a label it did not apply ### 📊 Changes **7 files changed** (+1324 additions, -157 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/pr-risk.yml` (+233 -153) 📝 `.github/workflows/test-pr-risk.yml` (+11 -1) 📝 `scripts/pr-risk/README.md` (+85 -0) 📝 `scripts/pr-risk/apply-risk-label.sh` (+14 -3) ➕ `scripts/pr-risk/grade-targets.sh` (+524 -0) 📝 `scripts/pr-risk/tests/test_apply_risk_label.sh` (+40 -0) ➕ `scripts/pr-risk/tests/test_grade_targets.sh` (+417 -0) </details> ### 📄 Description ## 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: 1. **A repo that enrolls mid-stream cannot grade the queue it already has.** The first consumer enrollment left exactly one PR ever graded — its own — and 30 others unlabeled until each independently receives a `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. 2. **There is no manual re-grade.** After a `.github/risk.json` change, or on a PR carrying the human-owned `risk-dispute` label, 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 - **Authored by:** agent session, worktree off `origin/main` at `1c9443a` - **Verified:** `actionlint` clean on both workflows; YAML parses; `shellcheck -x` clean on all six shell files; `test_grade_pr_risk.sh` 49 passed / 0 failed **and untouched**; `test_apply_risk_label.sh` 24 passed / 0 failed; new `test_grade_targets.sh` 91 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). - **Deviations:** the `--stdin` mode is *not* the batch vehicle — see Notes. - **Review round (facd005):** ten panel findings resolved — the ref/path are now percent-encoded into the contents read (a raw `#` in a branch name truncated it into the silent default-branch read this PR exists to prevent), a 404 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 - **Type:** feature — additive optional inputs on the reusable, plus a refactor that moves the per-target sequence out of the job body into a script - **Slots into:** the advisory pr-risk shadow pilot. The label remains the entire product; nothing gates, routes, or merges - **Blast radius:** consumers pin full SHAs, so nobody moves until they bump their own pin. The backward-compatibility claim is asserted against the stubbed `gh` call log, not stated — see Test plan ## Summary - **Two optional `workflow_call` inputs.** `pr_number` grades one PR; `pr_numbers` grades a comma-separated list and wins when both are set. `PR_NUMBERS` resolves to `pr_numbers || pr_number || github.event.pull_request.number`, so the event path is the same single number it always was. Both are `string`, not `number`: `workflow_dispatch` inputs arrive as strings, and an empty string is what lets the fall-through stay one expression. - **The base ref is re-read from the API per target, and an unresolvable one FAILS that target.** This is the load-bearing half. The ref selects which branch's `.github/risk.json` judges 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. - **The event path spends no extra API call.** When a base ref arrives from the payload it is used as-is. A supplied *number*, though, ignores it — the target may be a different PR than the event's. - **Batch never abandons the rest for one bad target.** Every target is attempted and its outcome 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; a run cancelled mid-label is the one outcome with no signal at all, because the summary step never renders either. - **The per-target sequence moves 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. One copy — and it is unit-testable for the first time, which is what lets the claims above be tests rather than assertions. - **`ungraded` still keeps the run green,** on both paths. An unreadable PR is a reported verdict, not a broken run. - **Bot-authored and fork PRs grade on the dispatch path.** The actor and fork clauses in a caller's `if:` are *token* guards — a bot's or a fork's `pull_request` run gets a read-only `GITHUB_TOKEN`, so the label write would 403 — and neither applies when a human presses the button. Fork *risk* is untouched: `external` still comes from the API's own `isCrossRepository`, never the actor, and still grades R3. - **The header's copy-paste caller block is corrected for both event shapes**, and this is not cosmetic — the live consumer caller trips both traps today: - Its `if:` includes `github.event.pull_request.head.repo.full_name == github.repository`, which is **false** on `workflow_dispatch`, so the job would skip **silently** — no run, no error, no annotation. Guard clauses are now scoped behind `github.event_name != 'pull_request'`. - Its concurrency group is `${{ github.workflow }}-${{ github.event.pull_request.number }}`, which collapses to one constant group on dispatch, so with `cancel-in-progress` a batch would have each dispatch cancel the previous. The key now carries the resolved target. - A caller-side skip is undetectable from inside the reusable (no run is created), so the header is the only lever. What the reusable *can* do, and now does, is refuse to be a silent no-op once reached: a run resolving no target fails loudly naming both inputs. - **`wait_for_checks_minutes` on 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 that `0` produces on the event path. It is still not free: `0` breaks 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. `1` costs one 15s backoff plus a second read per PR. The header, the input description and the README all say prefer `1`, not `0`. ## Notes - **`--stdin` is 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. - Two retry constants gained env overrides (`MAX_UNREADABLE_TRIES`, `READ_RETRY_BUDGET_SECONDS`, plus `POLL_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. - The target list is capped at 50 and rejects non-numeric, leading-zero and duplicate entries. Leading zeros are rejected rather than normalised: GitHub resolves `007` to 7, so tolerating it would let `7,007` present as two targets for one PR and label it twice. - `test-pr-risk.yml`'s 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/`. ## Test plan Verified by unit test (hermetic — `gh` stubbed on PATH, every call logged so the tests assert on *which* requests were made): - [x] The event path issues **no** base-ref PR read and fetches the override from the payload's ref — the backward-compat claim, asserted against the call log - [x] The by-number path resolves the base ref from the API and reads both override files from **that** ref, never from an empty one - [x] An unresolvable base ref, and an empty one, fail their target having read **no** override and **never** invoked the grader - [x] A non-404 override read fails the target with no tier invented and no label applied - [x] A batch of three whose middle target is unreadable still grades the first and last - [x] `ungraded` keeps the run green; a failed label write fails its target - [x] Forks grade R3 from the API-derived fork flag; bot-authored PRs grade at all - [x] A run id absent from the rollup (the dispatch shape) excludes nothing, so a settled rollup reads `SUCCESS` and reversibility is not floored at R2 - [x] Empty / non-numeric / leading-zero / over-long target lists are refused with exit 2, not silently partially graded - [x] The grader suite passes **untouched** (49). The label suite grew by 3 (21 -> 24) in the review round, for the one 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): - [x] Batch of `1094,1086,999999`: both real PRs graded `R1`; **#1086's override was read from its feature-branch base** `matt/be-5532-cql-flat-pattern-costs`, not `main` — the load-bearing behaviour, against the real API - [x] The nonexistent `999999` failed at the base-ref hop and did **not** abandon the batch; the run reported `2 graded, 0 ungraded, 1 failed` - [x] Cost at `wait_for_checks_minutes: 1` is 15s per PR, matching the predicted single-backoff-plus-confirmation - [x] Grading a settled PR with a run id absent from its rollup yields `checks_state=SUCCESS`, `pending=false` and a real `R1` — the empirical basis for the low-wait recommendation **Not verified until this is merged and a caller pins it** — stated plainly rather than claimed: - [ ] A true `workflow_dispatch` end-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's `uses:` - [ ] The **label write** on the dispatch path. Locally it was dry-run, and the claim that a dispatch's `GITHUB_TOKEN` is writable where a bot's `pull_request` token is not is reasoned from GitHub's documented behaviour, not observed here - [ ] That a real dispatch's check run is genuinely absent from the graded PR's rollup. The unit test proves the *grader* handles a foreign run id correctly, which is the same code path, but the attachment behaviour itself is GitHub's After 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_dispatch` block from the header, and dispatch its open queue with `wait_for_checks_minutes: 1`. Running that backfill is deliberately a separate operational step, not part of this PR. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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