[PR #100] [CLOSED] feat(pr-risk): shadow-mode risk grade — risk:R0..R3 label, Check Run, sticky comment (BE-5508) #110

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/100
Author: @mattmillerai
Created: 7/31/2026
Status: ❌ Closed

Base: main ← Head: matt/be-5508-pr-risk-shadow

📝 Commits (6)

55708da feat(pr-risk): shadow-mode risk grade — risk:R0..R3 label, Check Run, sticky comment (BE-5508)
3eb48f7 fix(pr-risk): harden the grader and publisher against the review panel findings (BE-5508)
8d32d8d fix(pr-risk): second review round — sandbox the fallback renderer, validate the label, bound the comment (BE-5508)
ecee79b refactor(pr-risk): single-source the sticky-comment footer and make the size bound unconditional (BE-5508)
f8f342e fix(pr-risk): third review round — pin the sticky comment's author, isolate the three publications, catch a retarget (BE-5508)
32ec26a fix(pr-risk): fourth review round — wrap every API failure, refuse a labelless graded report, harden the dispute tick (BE-5508)

📊 Changes

10 files changed (+4134 additions, -0 deletions)

View changed files

➕ .github/pr-risk/README.md (+185 -0)
➕ .github/pr-risk/grade-risk.sh (+235 -0)
➕ .github/pr-risk/grade_risk.py (+686 -0)
➕ .github/pr-risk/publish_risk.py (+758 -0)
➕ .github/pr-risk/tests/test_grade_risk.py (+720 -0)
➕ .github/pr-risk/tests/test_publish_risk.py (+1002 -0)
➕ .github/workflows/pr-risk.yml (+483 -0)
➕ .github/workflows/test-pr-risk.yml (+52 -0)
📝 AGENTS.md (+12 -0)
📝 README.md (+1 -0)

📄 Description

ELI-5

Reviewers are staring at a 200-deep PR queue with no way to tell which changes are safe to move quickly on. This adds a check that reads every PR's diff, decides how much attention it needs (R0 trivial → R3 needs a careful read), and publishes that grade so you can filter the queue with is:pr is:open label:risk:R0 and knock out ten low-risk PRs in one pass.

It grades nothing else. It never fails a check, never blocks a merge, and no automation reads the label. A human still clicks every button.

What it does

Adds pr-risk.yml, a reusable workflow, plus its grading logic under .github/pr-risk/. On every PR event it publishes three things:

A label — exactly one risk:R0..risk:R3 at any time. Stale tiers are removed on each re-grade, so a PR that grows from R0 into R3 carries risk:R3 and only risk:R3.
A Check Run (PR risk (advisory)) — immutable, timestamped, attached to the head commit, carrying the tier and the reason it was assigned. This is the per-change audit artifact.
A sticky comment — updated in place, never duplicated per push — with the per-file breakdown, the risk concentration ("94% of this diff is R0/R1; the 6% that makes it R3 is these two files, 40 lines"), and a "this grade is wrong" checkbox whose toggling is recorded as a risk-grade-disputed label, giving a queryable stream of real reviewer disagreement.

The load-bearing bit: it re-grades on every push

The failure mode named in the ticket is a classifier that grades once, marks a PR extra-small, and never re-analyses it while it grows into several thousand lines. A stale grade is worse than no grade, because it spends trust that does not come back. So the caller pattern requires synchronize in its trigger types, and the label is recomputed and replaced on every run — not added once. There is an end-to-end test (test_a_grown_pr_relabels_from_r0_to_r3_across_two_publishes) that asserts exactly this across two publishes against a shared fake API.

Nothing is gated — four independent guarantees

Because "advisory" is easy to claim and easy to break later, it is enforced in four places rather than asserted once: the Check Run's conclusion is hardcoded neutral (a neutral check cannot fail a PR even if someone later marks it required); grade-risk.sh always exits 0; the grade step wraps the grader in if ! … fi + exit 0 so a broken grader still cannot redden a PR; and every publish step is continue-on-error with an explicit "degraded mode" note so nothing goes green silently.

Unknown is published as unknown

A PR the grader could not read (bad refs, git failure, malformed diff) gets no tier: it is published as unknown in the Check Run and the comment, and carries no risk:* label. It is never defaulted to risk:R0 and never silently skipped — a silent default to the safest tier is the same trust-spending failure as a stale grade. reconcile_labels(…, desired=None) also removes any pre-existing tier, so a PR that becomes ungradable does not keep a stale grade.

Shadow mode by default

mode defaults to shadow: Check Run only, no label and no comment. An adopting repo accumulates grades on its commits and checks them against reviewer intuition before any reviewer-facing surface changes. mode: publish opts in. This is the feature-flag equivalent for a CI surface, following pr-size.yml's mode: warn precedent.

Security topology

Copied from pr-size.yml, because it is the security model and not a convenience:

The grade job runs against PR code with contents: read and no write credentials. It hands its report to the next job as an artifact.
The publish job holds the bot write token and checks out no PR code — its only checkout is this repo's own pinned scripts.
Scripts load at run time from a pinned ref of this repo, never the caller's checkout, so a PR cannot rewrite the logic grading it.
pull_request only, deliberately not pull_request_target — the fork case is solved by the two-job split, not by handing a privileged context a PR checkout.
The dispute job only fires on a comment authored by a Bot, so a user pasting the marker into their own comment cannot drive the label.

Verification

All run locally and wired into test-pr-risk.yml (path-filtered, matching test-pr-size.yml / test-groom-scripts.yml):

python3 -m unittest discover -s .github/pr-risk/tests -p 'test_*.py' — 68 tests, all passing. Covers classification, --numstat -z parsing (renames, binaries, paths with spaces, malformed records), tier/escalation math, the unknown path, rendering, the shell entrypoint end-to-end over throwaway git repos, and all of the publisher invariants above against an injected fake API.
shellcheck -x .github/pr-risk/grade-risk.sh — clean.
python3 .github/agents-md-integrity/check_agents_md.py --root . — passes.
Manual end-to-end: built a real git repo, added db/migrations/1.sql, ran grade-risk.sh against it → risk:R3, status: graded, correct sticky marker in the rendered comment.

Judgment calls / things to know

1. The risk map is a documented first cut, not derived thresholds. The ticket says grading comes from grade-risk.sh (BE-5507) and that deriving thresholds is out of scope here. BE-5507 has not landed and has no branch, so there was nothing to consume. Rather than block a ticket whose other nine acceptance criteria are all CI wiring, RISK_RULES / FILE_ESCALATE_LINES / SIZE_ESCALATE_LINES in grade_risk.py ship as an explicitly-labelled placeholder with a RISK MAP PROVENANCE docstring. Nothing else in the diff depends on their values and nothing anywhere is gated on the result, so BE-5507 replaces those three in place and nothing else moves. Flagging this as the one place I widened scope past the ticket's letter.

2. No consumer caller lands in this PR. The ticket allows one or two. A caller must pin a SHA that contains pr-risk.yml, and that SHA only exists after this merges — shipping a caller here would pin a SHA lacking the file and redden every PR in this repo at startup. Enrollment is the standard two-step follow-up (merge the caller and add the repo to its callers roster). The caller pattern is fully documented in the workflow header and .github/pr-risk/README.md.

3. Diff size. ~2140 lines added, above the usual "small coherent PR" preference. It is one coherent feature that the ticket asks for as a unit — workflow + scripts + tests + harness — and splitting it would land a workflow calling scripts that do not exist. ~1100 of those lines are tests and READMEs.

4. AGENTS.md is now 154 lines, four over the aspirational 150 target (hard ceiling 200, warn-only, and the checker passes). The pre-existing no-CODEOWNERS warning is unchanged.

5. Caller permissions are a startup requirement. The calling job must grant contents: read + checks: write + pull-requests: write; GitHub rejects a caller granting less than a called job requests, even in shadow mode. Documented in the header, the README, and the catalog entry.

6. Negative-claim falsification (self-review clause e): not applicable, checked deliberately. The diff does add "unknown" strings, but the trigger is a capability denial — a throw/deny dead-end telling a user something is not supported. This is the opposite: a new advisory feature where unknown is a required status for an ungradable diff (an explicit acceptance criterion), not a claim that some capability does not exist. No existing path is replaced with a dead-end; the diff is purely additive.

7. Chesterton's Fence: nothing existing changed behaviour. The only edits to existing files are additive documentation rows in README.md and AGENTS.md, plus two open()-without-context-manager cleanups in the grader's own new test file.

Acceptance criteria

All ten met. The two worth naming precisely: "grading logic comes from BE-5507" is satisfied structurally (the entrypoint is grade-risk.sh and the map is a single replaceable block) but not literally (BE-5507's map does not exist yet) — see judgment call 1. "Land one or two consumer callers" is deferred to the enrollment follow-up for the SHA-pin reason in judgment call 2.

_{🔄 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/100 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/31/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `matt/be-5508-pr-risk-shadow` --- ### 📝 Commits (6) - [`55708da`](https://github.com/Comfy-Org/github-workflows/commit/55708da3d1b811dabbc29becec7faf7bd6a37044) feat(pr-risk): shadow-mode risk grade — risk:R0..R3 label, Check Run, sticky comment (BE-5508) - [`3eb48f7`](https://github.com/Comfy-Org/github-workflows/commit/3eb48f7c49cd718e313623ed9d9c66049169e30d) fix(pr-risk): harden the grader and publisher against the review panel findings (BE-5508) - [`8d32d8d`](https://github.com/Comfy-Org/github-workflows/commit/8d32d8d8948fe41d8473b0eadff6763219b6d2f2) fix(pr-risk): second review round — sandbox the fallback renderer, validate the label, bound the comment (BE-5508) - [`ecee79b`](https://github.com/Comfy-Org/github-workflows/commit/ecee79b15d803ac50759ba050569d2f75e34229f) refactor(pr-risk): single-source the sticky-comment footer and make the size bound unconditional (BE-5508) - [`f8f342e`](https://github.com/Comfy-Org/github-workflows/commit/f8f342ec388061beb9dfeb8540ffe81a4c8d6a6c) fix(pr-risk): third review round — pin the sticky comment's author, isolate the three publications, catch a retarget (BE-5508) - [`32ec26a`](https://github.com/Comfy-Org/github-workflows/commit/32ec26a53c23645e7ea803ee7a0a67b81cafd90d) fix(pr-risk): fourth review round — wrap every API failure, refuse a labelless graded report, harden the dispute tick (BE-5508) ### 📊 Changes **10 files changed** (+4134 additions, -0 deletions) <details> <summary>View changed files</summary> ➕ `.github/pr-risk/README.md` (+185 -0) ➕ `.github/pr-risk/grade-risk.sh` (+235 -0) ➕ `.github/pr-risk/grade_risk.py` (+686 -0) ➕ `.github/pr-risk/publish_risk.py` (+758 -0) ➕ `.github/pr-risk/tests/test_grade_risk.py` (+720 -0) ➕ `.github/pr-risk/tests/test_publish_risk.py` (+1002 -0) ➕ `.github/workflows/pr-risk.yml` (+483 -0) ➕ `.github/workflows/test-pr-risk.yml` (+52 -0) 📝 `AGENTS.md` (+12 -0) 📝 `README.md` (+1 -0) </details> ### 📄 Description ## ELI-5 Reviewers are staring at a 200-deep PR queue with no way to tell which changes are safe to move quickly on. This adds a check that reads every PR's diff, decides how much attention it needs (`R0` trivial → `R3` needs a careful read), and publishes that grade so you can filter the queue with `is:pr is:open label:risk:R0` and knock out ten low-risk PRs in one pass. It grades **nothing else**. It never fails a check, never blocks a merge, and no automation reads the label. A human still clicks every button. ## What it does Adds `pr-risk.yml`, a reusable workflow, plus its grading logic under `.github/pr-risk/`. On every PR event it publishes three things: - **A label** — exactly one `risk:R0`..`risk:R3` at any time. Stale tiers are removed on each re-grade, so a PR that grows from R0 into R3 carries `risk:R3` and only `risk:R3`. - **A Check Run** (`PR risk (advisory)`) — immutable, timestamped, attached to the head commit, carrying the tier *and* the reason it was assigned. This is the per-change audit artifact. - **A sticky comment** — updated in place, never duplicated per push — with the per-file breakdown, the **risk concentration** ("94% of this diff is R0/R1; the 6% that makes it R3 is these two files, 40 lines"), and a **"this grade is wrong" checkbox** whose toggling is recorded as a `risk-grade-disputed` label, giving a queryable stream of real reviewer disagreement. ## The load-bearing bit: it re-grades on every push The failure mode named in the ticket is a classifier that grades once, marks a PR extra-small, and never re-analyses it while it grows into several thousand lines. A stale grade is worse than no grade, because it spends trust that does not come back. So the caller pattern requires `synchronize` in its trigger types, and the label is *recomputed and replaced* on every run — not added once. There is an end-to-end test (`test_a_grown_pr_relabels_from_r0_to_r3_across_two_publishes`) that asserts exactly this across two publishes against a shared fake API. ## Nothing is gated — four independent guarantees Because "advisory" is easy to claim and easy to break later, it is enforced in four places rather than asserted once: the Check Run's conclusion is **hardcoded** `neutral` (a neutral check cannot fail a PR even if someone later marks it required); `grade-risk.sh` always exits 0; the grade step wraps the grader in `if ! … fi` + `exit 0` so a broken grader still cannot redden a PR; and every publish step is `continue-on-error` with an explicit "degraded mode" note so nothing goes green silently. ## Unknown is published as unknown A PR the grader could not read (bad refs, git failure, malformed diff) gets **no tier**: it is published as `unknown` in the Check Run and the comment, and carries no `risk:*` label. It is never defaulted to `risk:R0` and never silently skipped — a silent default to the safest tier is the same trust-spending failure as a stale grade. `reconcile_labels(…, desired=None)` also *removes* any pre-existing tier, so a PR that becomes ungradable does not keep a stale grade. ## Shadow mode by default `mode` defaults to `shadow`: Check Run only, no label and no comment. An adopting repo accumulates grades on its commits and checks them against reviewer intuition before any reviewer-facing surface changes. `mode: publish` opts in. This is the feature-flag equivalent for a CI surface, following `pr-size.yml`'s `mode: warn` precedent. ## Security topology Copied from `pr-size.yml`, because it is the security model and not a convenience: - The `grade` job runs against PR code with `contents: read` and **no write credentials**. It hands its report to the next job as an **artifact**. - The `publish` job holds the bot write token and **checks out no PR code** — its only checkout is this repo's own pinned scripts. - Scripts load at run time from a **pinned ref of this repo**, never the caller's checkout, so a PR cannot rewrite the logic grading it. - `pull_request` only, deliberately **not** `pull_request_target` — the fork case is solved by the two-job split, not by handing a privileged context a PR checkout. - The `dispute` job only fires on a comment authored by a **Bot**, so a user pasting the marker into their own comment cannot drive the label. ## Verification All run locally and wired into `test-pr-risk.yml` (path-filtered, matching `test-pr-size.yml` / `test-groom-scripts.yml`): - `python3 -m unittest discover -s .github/pr-risk/tests -p 'test_*.py'` — **68 tests, all passing**. Covers classification, `--numstat -z` parsing (renames, binaries, paths with spaces, malformed records), tier/escalation math, the unknown path, rendering, the shell entrypoint end-to-end over throwaway git repos, and all of the publisher invariants above against an injected fake API. - `shellcheck -x .github/pr-risk/grade-risk.sh` — clean. - `python3 .github/agents-md-integrity/check_agents_md.py --root .` — passes. - Manual end-to-end: built a real git repo, added `db/migrations/1.sql`, ran `grade-risk.sh` against it → `risk:R3`, `status: graded`, correct sticky marker in the rendered comment. ## Judgment calls / things to know **1. The risk map is a documented first cut, not derived thresholds.** The ticket says grading comes from `grade-risk.sh` (BE-5507) and that deriving thresholds is out of scope here. BE-5507 has not landed and has no branch, so there was nothing to consume. Rather than block a ticket whose other nine acceptance criteria are all CI wiring, `RISK_RULES` / `FILE_ESCALATE_LINES` / `SIZE_ESCALATE_LINES` in `grade_risk.py` ship as an explicitly-labelled placeholder with a `RISK MAP PROVENANCE` docstring. Nothing else in the diff depends on their values and nothing anywhere is gated on the result, so BE-5507 replaces those three in place and nothing else moves. Flagging this as the one place I widened scope past the ticket's letter. **2. No consumer caller lands in this PR.** The ticket allows one or two. A caller must pin a SHA that *contains* `pr-risk.yml`, and that SHA only exists after this merges — shipping a caller here would pin a SHA lacking the file and redden every PR in this repo at startup. Enrollment is the standard two-step follow-up (merge the caller **and** add the repo to its callers roster). The caller pattern is fully documented in the workflow header and `.github/pr-risk/README.md`. **3. Diff size.** ~2140 lines added, above the usual "small coherent PR" preference. It is one coherent feature that the ticket asks for as a unit — workflow + scripts + tests + harness — and splitting it would land a workflow calling scripts that do not exist. ~1100 of those lines are tests and READMEs. **4. `AGENTS.md` is now 154 lines**, four over the aspirational 150 target (hard ceiling 200, warn-only, and the checker passes). The pre-existing no-CODEOWNERS warning is unchanged. **5. Caller permissions are a startup requirement.** The calling job must grant `contents: read` + `checks: write` + `pull-requests: write`; GitHub rejects a caller granting less than a called job requests, even in shadow mode. Documented in the header, the README, and the catalog entry. **6. Negative-claim falsification (self-review clause e): not applicable, checked deliberately.** The diff does add "unknown" strings, but the trigger is a *capability denial* — a throw/deny dead-end telling a user something is not supported. This is the opposite: a new advisory feature where `unknown` is a required status for an ungradable diff (an explicit acceptance criterion), not a claim that some capability does not exist. No existing path is replaced with a dead-end; the diff is purely additive. **7. Chesterton's Fence: nothing existing changed behaviour.** The only edits to existing files are additive documentation rows in `README.md` and `AGENTS.md`, plus two `open()`-without-context-manager cleanups in the grader's own new test file. ## Acceptance criteria All ten met. The two worth naming precisely: "grading logic comes from BE-5507" is satisfied *structurally* (the entrypoint is `grade-risk.sh` and the map is a single replaceable block) but not *literally* (BE-5507's map does not exist yet) — see judgment call 1. "Land one or two consumer callers" is deferred to the enrollment follow-up for the SHA-pin reason in judgment call 2. --- <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:20 +00:00