[PR #116] [MERGED] fix(pr-risk): let a manual dispatch grade while `enabled` is false #126

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/pr-risk-dispatch-bypasses-gate

📝 Commits (1)

ccb6108 fix(pr-risk): let a manual dispatch grade while enabled is false

📊 Changes

4 files changed (+106 additions, -23 deletions)

View changed files

📝 .github/workflows/pr-risk.yml (+34 -8)
📝 README.md (+1 -1)
📝 scripts/pr-risk/resolve-enabled.sh (+35 -11)
📝 scripts/pr-risk/tests/test_resolve_enabled.sh (+36 -3)

📄 Description

ELI-5

enabled: false was blocking every path, including a manual run. That broke the staged rollout the switch was added for: a repo enrolled OFF couldn't grade even one PR to see what the grader would say, so "try it here before turning it on" required turning it on first — the very thing being evaluated.

enabled now governs automatic grading (the pull_request stream). A manual workflow_dispatch grades regardless.

	`pull_request`	`workflow_dispatch`
`enabled: false`	no grade	grades
`enabled: true`	grades	grades

Why a dispatch is safe to exempt

Running a workflow_dispatch requires write access on the repo and is an explicit, attributed act. There's no unattended behaviour left for the switch to govern.

The cost, stated rather than buried — it's in the input description and beside the job if:, not just here: false is now a mute on the PR stream, not a lockout. A repo that needs the harder guarantee should remove the caller; nothing in this workflow can bind a maintainer who can already edit the workflow file.

The part worth reviewing: the summary was about to lie

The gate writes a step summary when grading is off. It said:

No grade was computed, and no risk:* label was added, changed, or removed

On a dispatch that is about to do exactly that, this is the single most misleading line this workflow could emit — it's the sentence a reader trusts to decide whether the label on their PR is current. Left unchanged, the fix above would have shipped a workflow that grades a PR while telling the operator it didn't.

So the resolver now branches on GITHUB_EVENT_NAME:

disabled + dispatch → says the manual run grades anyway, and warns that the label it leaves will go stale, because pushes won't re-grade it while automatic grading is off. That second half matters: a label with no refresh behind it is the stale-grade failure the whole check was designed against.
disabled + pull_request → keeps its original claim (accurate there) and gains a pointer to the manual route.
enabled, either event → no summary at all; the grade job owns the output from there.

Verification

test_resolve_enabled.sh is now 27 assertions, up from 21. The six new ones pin the summary contract in both directions, including a negative assertion that the disabled-dispatch summary does not contain "No grade was computed" — the specific wrong sentence, rather than a loose match that would pass on any rewrite.

All four pr-risk suites pass; shellcheck -x clean; the workflow parses; README row updated.

Consumer impact

None until a repo bumps its pin. For the two live consumers this only adds a capability — both pin enabled: true, so their automatic path is unchanged either way.

Worth knowing for whoever enrols next: a repo can now land the caller with enabled: false, dispatch a grade against two or three real PRs to see the tiers it produces, and only then switch it on. That was the intended flow when the switch landed and it didn't actually 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/116 **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/pr-risk-dispatch-bypasses-gate` --- ### 📝 Commits (1) - [`ccb6108`](https://github.com/Comfy-Org/github-workflows/commit/ccb610897abf740cc8417cf74816bcc36ebda40c) fix(pr-risk): let a manual dispatch grade while `enabled` is false ### 📊 Changes **4 files changed** (+106 additions, -23 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/pr-risk.yml` (+34 -8) 📝 `README.md` (+1 -1) 📝 `scripts/pr-risk/resolve-enabled.sh` (+35 -11) 📝 `scripts/pr-risk/tests/test_resolve_enabled.sh` (+36 -3) </details> ### 📄 Description ## ELI-5 `enabled: false` was blocking **every** path, including a manual run. That broke the staged rollout the switch was added for: a repo enrolled OFF couldn't grade even one PR to see what the grader would say, so *"try it here before turning it on"* required turning it on first — the very thing being evaluated. `enabled` now governs **automatic** grading (the `pull_request` stream). A manual `workflow_dispatch` grades regardless. | | `pull_request` | `workflow_dispatch` | |---|---|---| | `enabled: false` | no grade | **grades** | | `enabled: true` | grades | grades | ## Why a dispatch is safe to exempt Running a `workflow_dispatch` requires write access on the repo and is an explicit, attributed act. There's no unattended behaviour left for the switch to govern. **The cost, stated rather than buried** — it's in the input description and beside the job `if:`, not just here: `false` is now a **mute on the PR stream, not a lockout**. A repo that needs the harder guarantee should remove the caller; nothing in this workflow can bind a maintainer who can already edit the workflow file. ## The part worth reviewing: the summary was about to lie The gate writes a step summary when grading is off. It said: > No grade was computed, and no `risk:*` label was added, changed, or removed On a dispatch that is about to do exactly that, this is the single most misleading line this workflow could emit — it's the sentence a reader trusts to decide whether the label on their PR is current. Left unchanged, the fix above would have shipped a workflow that grades a PR while telling the operator it didn't. So the resolver now branches on `GITHUB_EVENT_NAME`: - **disabled + dispatch** → says the manual run grades anyway, **and** warns that the label it leaves will go stale, because pushes won't re-grade it while automatic grading is off. That second half matters: a label with no refresh behind it is the stale-grade failure the whole check was designed against. - **disabled + `pull_request`** → keeps its original claim (accurate there) and gains a pointer to the manual route. - **enabled, either event** → no summary at all; the grade job owns the output from there. ## Verification `test_resolve_enabled.sh` is now 27 assertions, up from 21. The six new ones pin the summary contract in both directions, including a negative assertion that the disabled-dispatch summary does **not** contain "No grade was computed" — the specific wrong sentence, rather than a loose match that would pass on any rewrite. All four pr-risk suites pass; `shellcheck -x` clean; the workflow parses; README row updated. ## Consumer impact None until a repo bumps its pin. For the two live consumers this only **adds** a capability — both pin `enabled: true`, so their automatic path is unchanged either way. Worth knowing for whoever enrols next: a repo can now land the caller with `enabled: false`, dispatch a grade against two or three real PRs to see the tiers it produces, and only then switch it on. That was the intended flow when the switch landed and it didn't actually 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:31 +00:00