[PR #120] [MERGED] feat(pr-risk): opt-in Check Run + sticky comment with risk concentration and a dispute checkbox #129

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-6326-pr-risk-publish-surfaces

📝 Commits (3)

fead856 feat(pr-risk): opt-in Check Run + sticky comment with risk concentration and a dispute checkbox
4d54d54 fix(pr-risk): never lose a dispute or duplicate a sticky on a transient read (BE-6326)
5baabcd refactor(pr-risk): collapse the sticky comment to one line + a

📊 Changes

10 files changed (+1329 additions, -11 deletions)

View changed files

📝 .github/workflows/pr-risk.yml (+155 -5)
📝 .github/workflows/test-pr-risk.yml (+9 -1)
📝 README.md (+1 -1)
📝 scripts/pr-risk/README.md (+24 -0)
📝 scripts/pr-risk/grade-pr-risk.sh (+27 -2)
📝 scripts/pr-risk/grade-targets.sh (+95 -0)
➕ scripts/pr-risk/publish-risk-surfaces.sh (+584 -0)
📝 scripts/pr-risk/tests/test_grade_pr_risk.sh (+23 -1)
📝 scripts/pr-risk/tests/test_grade_targets.sh (+44 -1)
➕ scripts/pr-risk/tests/test_publish_risk_surfaces.sh (+367 -0)

📄 Description

ELI-5

The risk grader stamps a risk:R0..risk:R3 sticker on every PR, and that sticker never says why. This adds two things you can turn on if you want them: a comment that explains the grade ("94% of this diff is R0/R1; the 6% that puts the path floor at R3 is these two files, 40 lines") with a tickbox for "this grade is wrong", and a Check Run that pins the grade to the exact commit so there's a permanent record. Both are off unless you switch them on, and neither can ever fail your build.

What this adds

Two publish surfaces, both false by default:

sticky_comment: true — ONE comment, created once and updated in place (N pushes leave one comment), carrying a per-file breakdown, the concentration sentence, and a dispute checkbox whose state round-trips into a risk-grade-disputed label. It rides the pull-requests: write the label already needs, so switching it on requires no permission change.
check_run: true — the tier and reason as a Check Run on the head commit, with conclusion: neutral hardcoded. Published from a separate job that holds checks: write.

Judgment calls

Per-file breakdown = the PATH axis, and it says so. The ticket asked me to pick between deriving per-path tiers from the same rules or reporting by matched class. I derived per-path tiers: the grader now emits risk.axes.path_floor.files, matched with the same rules as the floor, reporting-only. worst over those per-file floors equals the floor by construction, and a test pins that equality so the breakdown cannot quietly become a second grading model. Because only the path axis is per-file at all, each row is labelled a contribution, and whenever the headline tier exceeds the path floor the concentration sentence names the axis that supplied it and quotes its reason — without that, a docs-only diff headlines R2 (no green checks) directly above "all 600 lines are R0" and the explanation contradicts the tier it explains.

The Check Run is a separate job, and that is the two-job split the ticket asked about. Not primarily for elegance: raising the grade job's checks: read to checks: write would be rejected before any step ran for every caller already enrolled with the reviewed permissions block in pr-risk.yml's header — a reusable workflow can only narrow the caller's token. Splitting it keeps the new grant opt-in and gives the posture BE-5508 wanted: all PR-controlled text is escaped in the grading job (narrow token), and the job holding checks: write reads nothing from a pull request — its whole input is a pre-rendered, pre-escaped string.

Dispute state: the checkbox is the source of truth, the label mirrors it. PR #100 OR-ed the label back into the state, which made the label one-way — once set, no un-tick could ever clear it. Here ticking applies risk-grade-disputed and un-ticking removes it. The cost is one narrow race (a tick landing between the read-back and the PATCH is overwritten), which is why the body is re-read by id immediately before the write rather than reused from the paginated scan. risk-grade-disputed stays distinct from the human-owned risk-dispute, which the grader still never touches.

Hardening (ported, and tested rather than estimated)

A crafted FILENAME cannot forge a dispute. git permits |, backticks and newlines in a path; rendered raw into a table it breaks out of its code span and its row, letting an author inject a pre-ticked - [x] **This grade is wrong** that the next re-grade reads back as genuine reviewer disagreement — corrupting the exact calibration signal the checkbox exists to collect. Newlines are flattened (nothing PR-controlled may start a line), pipes are backslash-escaped, and a backtick or backslash forces fully-escaped plain text. Same treatment for an unknown report's reason, which quotes PR-authored path names. Covered by a test using a filename containing all three plus a literal pre-ticked checkbox and an <img> tag.
The body is bounded. GitHub 422s a comment over 65536 chars, and the label write happens first, so an over-long body would leave the label fresh and the comment stale on every future re-grade. Per-path (160) / per-reason (500) caps, ≤50 rows, row-dropping by halving, and an unconditional backstop that re-appends the footer so a truncated body still carries the checkbox the publisher parses. Verified by a test rendering 400 files with 300-char deeply-nested paths (10,444 chars).
The sticky marker is one constant, and a test pins that the rendered body's first line is exactly what find_sticky matches — if they drifted, every push would POST a new comment instead of updating.
Comment ownership requires author type Bot and an allow-listed login and the marker at position 0. Marker-only matching would let a PR author pre-post a comment carrying the (public) marker and thereafter control the preserved checkbox state.

Verification

All five suites green locally, plus shellcheck on every touched script:

test_publish_risk_surfaces.sh — 48 passed, 0 failed (new)
test_grade_pr_risk.sh — 62 passed (was 57; +5 for the per-file-floor invariant)
test_grade_targets.sh — 91 passed · test_apply_risk_label.sh — 24 · test_resolve_enabled.sh — 21
shellcheck -x clean on all scripts and suites; both workflow files parse.

test-pr-risk.yml runs the new suite and shellchecks the new script.

What a reviewer should check hardest

The skipped-job permission assumption. This design relies on GitHub not rejecting a run because a skipped job (if: ... && inputs.check_run) declares checks: write the caller did not grant. I could not exercise that from here. If GitHub validates eagerly at graph construction instead of at job start, every existing caller breaks on the SHA bump — which would make the fix "gate the whole job behind a caller-side if: instead", not a code change here. This is the one line I would want confirmed against a real run before the tag moves.
surfaces as a job output. Job outputs share a ~1MB budget, so the rendered summary is capped at 12,000 chars per target before it travels (CHECK_SUMMARY_CAP). A 50-target batch therefore fits; without the cap it would silently truncate every target's payload, not just the offenders.

Notes

No capability is denied anywhere in this diff — it only adds surfaces, and the pre-existing unknown path (which reports unknown, never R0) is unchanged and covered by tests including the empty and non-JSON record cases. So the negative-claim falsification rule does not apply.
The label surface, the grading model, the risk map and the runbook registry are all untouched; size is still not a grading axis (out of scope, owned by the threshold work).

_{🔄 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/120 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/4/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6326-pr-risk-publish-surfaces` --- ### 📝 Commits (3) - [`fead856`](https://github.com/Comfy-Org/github-workflows/commit/fead856078e099793cce231b47841138f6d9bbfd) feat(pr-risk): opt-in Check Run + sticky comment with risk concentration and a dispute checkbox - [`4d54d54`](https://github.com/Comfy-Org/github-workflows/commit/4d54d54f771cbbefdababc125ba9b0bdf0a07374) fix(pr-risk): never lose a dispute or duplicate a sticky on a transient read (BE-6326) - [`5baabcd`](https://github.com/Comfy-Org/github-workflows/commit/5baabcdff33d42031d97f49450c14af5dd82a9c8) refactor(pr-risk): collapse the sticky comment to one line + a <details> ### 📊 Changes **10 files changed** (+1329 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/pr-risk.yml` (+155 -5) 📝 `.github/workflows/test-pr-risk.yml` (+9 -1) 📝 `README.md` (+1 -1) 📝 `scripts/pr-risk/README.md` (+24 -0) 📝 `scripts/pr-risk/grade-pr-risk.sh` (+27 -2) 📝 `scripts/pr-risk/grade-targets.sh` (+95 -0) ➕ `scripts/pr-risk/publish-risk-surfaces.sh` (+584 -0) 📝 `scripts/pr-risk/tests/test_grade_pr_risk.sh` (+23 -1) 📝 `scripts/pr-risk/tests/test_grade_targets.sh` (+44 -1) ➕ `scripts/pr-risk/tests/test_publish_risk_surfaces.sh` (+367 -0) </details> ### 📄 Description ## ELI-5 The risk grader stamps a `risk:R0`..`risk:R3` sticker on every PR, and that sticker never says *why*. This adds two things you can turn on if you want them: a comment that explains the grade ("94% of this diff is R0/R1; the 6% that puts the path floor at R3 is these two files, 40 lines") with a tickbox for "this grade is wrong", and a Check Run that pins the grade to the exact commit so there's a permanent record. Both are **off unless you switch them on**, and neither can ever fail your build. ## What this adds Two publish surfaces, both `false` by default: - **`sticky_comment: true`** — ONE comment, created once and **updated in place** (N pushes leave one comment), carrying a per-file breakdown, the concentration sentence, and a dispute checkbox whose state round-trips into a `risk-grade-disputed` label. It rides the `pull-requests: write` the label already needs, so switching it on requires no permission change. - **`check_run: true`** — the tier and reason as a Check Run on the head commit, with `conclusion: neutral` hardcoded. Published from a **separate job** that holds `checks: write`. ## Judgment calls **Per-file breakdown = the PATH axis, and it says so.** The ticket asked me to pick between deriving per-path tiers from the same rules or reporting by matched class. I derived per-path tiers: the grader now emits `risk.axes.path_floor.files`, matched with the *same* rules as the floor, reporting-only. `worst` over those per-file floors equals the floor by construction, and a test pins that equality so the breakdown cannot quietly become a second grading model. Because only the path axis is per-file at all, each row is labelled a *contribution*, and whenever the headline tier exceeds the path floor the concentration sentence **names the axis that supplied it and quotes its reason** — without that, a docs-only diff headlines `R2` (no green checks) directly above "all 600 lines are R0" and the explanation contradicts the tier it explains. **The Check Run is a separate job, and that is the two-job split the ticket asked about.** Not primarily for elegance: raising the grade job's `checks: read` to `checks: write` would be **rejected before any step ran** for every caller already enrolled with the reviewed permissions block in `pr-risk.yml`'s header — a reusable workflow can only narrow the caller's token. Splitting it keeps the new grant opt-in *and* gives the posture BE-5508 wanted: all PR-controlled text is escaped in the grading job (narrow token), and the job holding `checks: write` reads nothing from a pull request — its whole input is a pre-rendered, pre-escaped string. **Dispute state: the checkbox is the source of truth, the label mirrors it.** PR #100 OR-ed the label back into the state, which made the label one-way — once set, no un-tick could ever clear it. Here ticking applies `risk-grade-disputed` and un-ticking removes it. The cost is one narrow race (a tick landing between the read-back and the PATCH is overwritten), which is why the body is re-read by id immediately before the write rather than reused from the paginated scan. `risk-grade-disputed` stays **distinct** from the human-owned `risk-dispute`, which the grader still never touches. ## Hardening (ported, and tested rather than estimated) - **A crafted FILENAME cannot forge a dispute.** git permits `|`, backticks and newlines in a path; rendered raw into a table it breaks out of its code span *and* its row, letting an author inject a pre-ticked `- [x] **This grade is wrong**` that the next re-grade reads back as genuine reviewer disagreement — corrupting the exact calibration signal the checkbox exists to collect. Newlines are flattened (nothing PR-controlled may start a line), pipes are backslash-escaped, and a backtick or backslash forces fully-escaped plain text. Same treatment for an `unknown` report's reason, which quotes PR-authored path names. Covered by a test using a filename containing all three plus a literal pre-ticked checkbox and an `<img>` tag. - **The body is bounded.** GitHub 422s a comment over 65536 chars, and the label write happens *first*, so an over-long body would leave the label fresh and the comment stale on every future re-grade. Per-path (160) / per-reason (500) caps, ≤50 rows, row-dropping by halving, and an unconditional backstop that **re-appends the footer** so a truncated body still carries the checkbox the publisher parses. Verified by a test rendering 400 files with 300-char deeply-nested paths (10,444 chars). - **The sticky marker is one constant**, and a test pins that the rendered body's first line is exactly what `find_sticky` matches — if they drifted, every push would POST a new comment instead of updating. - **Comment ownership** requires author type `Bot` **and** an allow-listed login **and** the marker at position 0. Marker-only matching would let a PR author pre-post a comment carrying the (public) marker and thereafter control the preserved checkbox state. ## Verification All five suites green locally, plus shellcheck on every touched script: - `test_publish_risk_surfaces.sh` — **48 passed, 0 failed** (new) - `test_grade_pr_risk.sh` — 62 passed (was 57; +5 for the per-file-floor invariant) - `test_grade_targets.sh` — 91 passed · `test_apply_risk_label.sh` — 24 · `test_resolve_enabled.sh` — 21 - `shellcheck -x` clean on all scripts and suites; both workflow files parse. `test-pr-risk.yml` runs the new suite and shellchecks the new script. ## What a reviewer should check hardest 1. **The skipped-job permission assumption.** This design relies on GitHub *not* rejecting a run because a **skipped** job (`if: ... && inputs.check_run`) declares `checks: write` the caller did not grant. I could not exercise that from here. If GitHub validates eagerly at graph construction instead of at job start, every existing caller breaks on the SHA bump — which would make the fix "gate the whole job behind a caller-side `if:` instead", not a code change here. This is the one line I would want confirmed against a real run before the tag moves. 2. **`surfaces` as a job output.** Job outputs share a ~1MB budget, so the rendered summary is capped at 12,000 chars per target before it travels (`CHECK_SUMMARY_CAP`). A 50-target batch therefore fits; without the cap it would silently truncate *every* target's payload, not just the offenders. ## Notes - No capability is denied anywhere in this diff — it only adds surfaces, and the pre-existing `unknown` path (which reports `unknown`, never `R0`) is unchanged and covered by tests including the empty and non-JSON record cases. So the negative-claim falsification rule does not apply. - The label surface, the grading model, the risk map and the runbook registry are all untouched; size is still not a grading axis (out of scope, owned by the threshold work). --- <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:32 +00:00