[PR #115] [MERGED] feat(pr-risk): gate grading behind an `enabled` switch, off by default #125

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/pr-risk-enabled-switch

📝 Commits (1)

111d202 feat(pr-risk): gate grading behind an enabled switch, off by default

📊 Changes

5 files changed (+244 additions, -2 deletions)

View changed files

📝 .github/workflows/pr-risk.yml (+62 -0)
📝 .github/workflows/test-pr-risk.yml (+7 -1)
📝 README.md (+1 -1)
➕ scripts/pr-risk/resolve-enabled.sh (+75 -0)
➕ scripts/pr-risk/tests/test_resolve_enabled.sh (+99 -0)

📄 Description

ELI-5

Grading is now off unless switched on. Enrolling the workflow and turning it on become two separate decisions: a repo can land the caller, get it reviewed, and start grading when it's ready — or stop grading without reverting anything.

The switch has two layers, and the variable outranks the input in both directions:

# reviewed, pinned in the caller's workflow file
with:
  enabled: true

# operational, no PR — and it can also turn it OFF
gh variable set RISK_CONFIG --repo <owner>/<repo> --body '{"enabled": true}'

This works because the vars context inside a reusable resolves against the caller's repository — the same mechanism groom.yml already uses for vars.GROOM_CONFIG.

Why the variable can also disable

A kill switch that needs a PR isn't a kill switch. {"enabled": false} stops grading on the next run even when the caller pins enabled: true, so the escape hatch works at the moment you need it — which is by definition a moment when something is already going wrong.

A malformed variable degrades to the reviewed value, never to off

"The variable is broken" and "the operator switched it off" must not look alike. If a typo read as false, one bad edit would silently un-enrol a repo and look entirely deliberate. So a value this can't read is announced in an annotation and discarded, and the reviewed input stands.

A well-formed object with no enabled key is silent rather than warning — it's the only key today, and a per-run annotation would train operators to ignore the one that flags a real typo (and would start lying the moment a second key lands there).

The bug the tests caught

Resolution lives in scripts/pr-risk/resolve-enabled.sh rather than inline YAML so it's testable — the same reasoning that moved the target loop into grade-targets.sh in #113. That paid for itself immediately:

jq -e sets its exit code from the TRUTHINESS of its output. So a perfectly valid {"enabled": false} made jq exit 1, was mistaken for a parse failure, fell through to the fallback — and silently disarmed the kill switch. The one half of this lever that has to work when things are already going wrong. It now reads the value as text and tests the text.

I would not have found that by inspection; it took the test that asserts the disable direction specifically.

Shape

A gate job resolving enablement, holding no token scopes (permissions: {}), with grade gated on its output. Its own job rather than a first step of grade so:

a disabled repo pays one bare runner instead of booting the grading job to immediately no-op
the decision, and the reason for it, is visible in the run graph rather than buried in a step log
a disabled run explains itself in the step summary instead of looking like a workflow that quietly did nothing

A disabled run touches no label. Whatever the last enabled run left stands — it does not grade, and it does not clean up.

Verification

test_resolve_enabled.sh, 21 assertions: both override directions, all four malformed shapes (unparseable / non-object / string "true" / numeric 1) degrading to the reviewed value in both the enabled and disabled starting states, the silent no-key case, whitespace-only handling, the disabled summary, and that an enabled run writes no summary at all.

Wired into test-pr-risk.yml explicitly — that workflow enumerates its suites rather than globbing, so a new file is otherwise silently un-run. Added to its shellcheck list for the same reason.

All four suites pass; shellcheck -x clean; both workflow files parse.

Consumer impact — read this before merging

This is a breaking change for the two live consumers, by design. Comfy-Org/cloud and Comfy-Org/comfy-cloud-mcp-server are enrolled and grading today; when they bump past this SHA they stop, unless enabled first.

Neither is affected until it bumps its own pin, so there is no rush — but the bump and the enable belong in the same change. Two options per repo, either works:

add enabled: true to the caller's with: block in the same PR as the pin bump, or
set RISK_CONFIG to {"enabled": true} before merging the bump

The disabled step summary names both, so a repo that bumps and goes dark finds out on its next PR rather than wondering where its labels went.

_{🔄 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/115 **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-enabled-switch` --- ### 📝 Commits (1) - [`111d202`](https://github.com/Comfy-Org/github-workflows/commit/111d202b8fb5c70bb875a874a298d47acead3639) feat(pr-risk): gate grading behind an `enabled` switch, off by default ### 📊 Changes **5 files changed** (+244 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/pr-risk.yml` (+62 -0) 📝 `.github/workflows/test-pr-risk.yml` (+7 -1) 📝 `README.md` (+1 -1) ➕ `scripts/pr-risk/resolve-enabled.sh` (+75 -0) ➕ `scripts/pr-risk/tests/test_resolve_enabled.sh` (+99 -0) </details> ### 📄 Description ## ELI-5 Grading is now **off unless switched on**. Enrolling the workflow and turning it on become two separate decisions: a repo can land the caller, get it reviewed, and start grading when it's ready — or stop grading without reverting anything. The switch has two layers, and the **variable outranks the input in both directions**: ```yaml # reviewed, pinned in the caller's workflow file with: enabled: true ``` ```bash # operational, no PR — and it can also turn it OFF gh variable set RISK_CONFIG --repo <owner>/<repo> --body '{"enabled": true}' ``` This works because the `vars` context inside a reusable resolves against the **caller's** repository — the same mechanism `groom.yml` already uses for `vars.GROOM_CONFIG`. ## Why the variable can also disable A kill switch that needs a PR isn't a kill switch. `{"enabled": false}` stops grading on the next run even when the caller pins `enabled: true`, so the escape hatch works at the moment you need it — which is by definition a moment when something is already going wrong. ## A malformed variable degrades to the reviewed value, never to off **"The variable is broken" and "the operator switched it off" must not look alike.** If a typo read as `false`, one bad edit would silently un-enrol a repo and look entirely deliberate. So a value this can't read is announced in an annotation and discarded, and the reviewed input stands. A well-formed object with **no** `enabled` key is silent rather than warning — it's the only key today, and a per-run annotation would train operators to ignore the one that flags a real typo (and would start lying the moment a second key lands there). ## The bug the tests caught Resolution lives in `scripts/pr-risk/resolve-enabled.sh` rather than inline YAML so it's testable — the same reasoning that moved the target loop into `grade-targets.sh` in #113. That paid for itself immediately: **`jq -e` sets its exit code from the TRUTHINESS of its output.** So a perfectly valid `{"enabled": false}` made jq exit 1, was mistaken for a parse failure, fell through to the fallback — and silently disarmed the kill switch. The one half of this lever that has to work when things are already going wrong. It now reads the value as text and tests the text. I would not have found that by inspection; it took the test that asserts the disable direction specifically. ## Shape A `gate` job resolving enablement, holding **no token scopes** (`permissions: {}`), with `grade` gated on its output. Its own job rather than a first step of `grade` so: - a disabled repo pays one bare runner instead of booting the grading job to immediately no-op - the decision, and the reason for it, is visible in the run graph rather than buried in a step log - a disabled run **explains itself in the step summary** instead of looking like a workflow that quietly did nothing **A disabled run touches no label.** Whatever the last enabled run left stands — it does not grade, and it does not clean up. ## Verification `test_resolve_enabled.sh`, 21 assertions: both override directions, all four malformed shapes (unparseable / non-object / string `"true"` / numeric `1`) degrading to the reviewed value in **both** the enabled and disabled starting states, the silent no-key case, whitespace-only handling, the disabled summary, and that an enabled run writes no summary at all. Wired into `test-pr-risk.yml` **explicitly** — that workflow enumerates its suites rather than globbing, so a new file is otherwise silently un-run. Added to its `shellcheck` list for the same reason. All four suites pass; `shellcheck -x` clean; both workflow files parse. ## Consumer impact — read this before merging **This is a breaking change for the two live consumers**, by design. `Comfy-Org/cloud` and `Comfy-Org/comfy-cloud-mcp-server` are enrolled and grading today; when they bump past this SHA they stop, unless enabled first. Neither is affected until it bumps its own pin, so there is no rush — but the bump and the enable belong in the same change. Two options per repo, either works: 1. add `enabled: true` to the caller's `with:` block in the same PR as the pin bump, or 2. set `RISK_CONFIG` to `{"enabled": true}` **before** merging the bump The disabled step summary names both, so a repo that bumps and goes dark finds out on its next PR rather than wondering where its labels went. --- <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:30 +00:00