feat(pr-risk): gate grading behind an enabled switch, off by default - #115
Conversation
Enrolling the workflow and switching it on become two decisions: a repo
can land the caller, get it reviewed, and start grading later — or stop
grading without reverting anything.
`vars.RISK_CONFIG` on the CALLING repo outranks the reviewed input in
BOTH directions. `{"enabled": true}` switches a caller on with no `with:`
change; `{"enabled": false}` is a kill switch that needs no PR at all.
The `vars` context inside a reusable resolves against the caller's
repository, which is what makes this reachable — the same mechanism
groom.yml already uses for vars.GROOM_CONFIG.
A malformed variable degrades to the REVIEWED value, never to off, and
says so in an annotation: "the variable is broken" and "the operator
switched it off" must not look alike, or one typo silently un-enrols a
repo and looks deliberate. A well-formed object with no `enabled` key is
silent rather than warning — it is the only key today, and a per-run
annotation would train operators to ignore the one that flags a real
typo.
Resolution lives in scripts/pr-risk/resolve-enabled.sh rather than inline
YAML so it is testable, following the same reasoning that moved the
target loop into grade-targets.sh. Its suite caught the bug that matters
most: `jq -e` sets its exit code from the TRUTHINESS of its output, so a
valid `{"enabled": false}` exited 1, read as a parse failure, and
disarmed the kill switch — the half of the lever that has to work when
something is already going wrong.
Runs as its own `gate` job holding no token scopes, so a disabled repo
pays one bare runner instead of booting the grading job to no-op, and a
disabled run explains itself in the step summary rather than looking
like a workflow that quietly did nothing. A disabled run touches no
label: whatever the last enabled run left stands.
Wired into test-pr-risk.yml explicitly — that workflow enumerates its
suites rather than globbing, so a new file is otherwise silently un-run.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 4 |
| 🟢 Low | 4 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
| name: Resolve enablement | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 5 | ||
| permissions: {} |
There was a problem hiding this comment.
🟠 High — The gate job declares permissions: {} but immediately runs actions/checkout against Comfy-Org/github-workflows, which presents the zero-scope GITHUB_TOKEN rather than falling back to anonymous read. If that clone is refused the gate job fails, enabled is empty, and the needs.gate.outputs.enabled == 'true' condition skips grading in every consumer — the sibling gate in groom.yml and the grade job here both declare contents: read, so match that proven pattern. Raised by 3 of 8 reviewers (gpt-5.6-sol-max edge-case, claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).
| fi | ||
| fi | ||
|
|
||
| [ -z "${GITHUB_OUTPUT:-}" ] || echo "enabled=$enabled" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
🟡 Medium — The append to $GITHUB_OUTPUT is unchecked and the script runs under set -uo pipefail without -e, so a failed write leaves the step green with an empty enabled output; grade then skips as though the repo were disabled — the exact "degrade toward off" the file header forbids. Check the write and fail the step (or treat a missing output as an error in the workflow). Raised by 4 of 8 reviewers (gpt-5.6-sol-max edge-case, kimi-k2.7-code edge-case, claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).
| degrades to this value and says so in an annotation. | ||
| type: boolean | ||
| required: false | ||
| default: false |
There was a problem hiding this comment.
🟡 Medium — Defaulting enabled to false silently disables grading for every already-enrolled caller the next time it moves its SHA pin, and a disabled run deliberately leaves the previous risk:* label in place — so those PRs keep showing a stale grade indistinguishable from a current one. There is no bump-pr-risk-callers fleet, so pins move by hand with no coordinated migration; this is a behavior break riding the in-place v1 tag. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).
| echo | ||
| echo "Decided by ${decided_by}. To switch grading on, either set the repository variable" | ||
| echo '`RISK_CONFIG` to `{"enabled": true}` (takes effect on the next run, no PR needed), or' | ||
| echo 'pass `enabled: true` in the caller'"'"'s `with:` block.' |
There was a problem hiding this comment.
🟡 Medium — The disabled-run summary always offers enabled: true in the caller's with: block as a remedy, but when decided_by is vars.RISK_CONFIG the variable outranks the input and that advice provably will not work. Branch the remediation text on decided_by and, for the kill-switch case, say the variable must be changed or removed first. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).
|
|
||
| raw="${RISK_CONFIG:-}" | ||
| if [ -n "${raw//[[:space:]]/}" ]; then | ||
| if ! jq -e 'type == "object"' >/dev/null 2>&1 <<<"$raw"; then |
There was a problem hiding this comment.
🟡 Medium — ! jq -e 'type == "object"' with stderr discarded treats every non-zero exit as "not a JSON object" — including exit 127 when jq is missing from the runner, OOM, or any jq-internal error. A perfectly valid {"enabled": true} is then discarded, the kill switch stops working, and the annotation blames the operator's variable for a tool failure. Distinguish jq exit 1 (predicate false) from exit >1 (jq itself failed) and fail loudly on the latter. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).
| eq "an enabled run writes no summary" "" "$SUMMARY" | ||
|
|
||
| echo "— phase 7: it is runnable outside Actions (neither env file set) —" | ||
| out="$(INPUT_ENABLED=true RISK_CONFIG='' bash "$SCRIPT" 2>/dev/null)" |
There was a problem hiding this comment.
🟢 Low — Phase 7 claims to test the "runnable outside Actions" path but never unsets GITHUB_OUTPUT / GITHUB_STEP_SUMMARY, both of which ARE set when the suite runs in CI — so the script takes the write-to-file branch, the assertion passes vacuously, and the run appends a stray enabled=true into the live step's GITHUB_OUTPUT. Invoke via env -u GITHUB_OUTPUT -u GITHUB_STEP_SUMMARY. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).
| *DISABLED*"no \`risk:*\` label was added, changed, or removed"*) ok "summary states nothing was touched" ;; | ||
| *) bad "summary states nothing was touched" "$SUMMARY" ;; | ||
| esac | ||
| case "$SUMMARY" in *RISK_CONFIG*) ok "and names the switch" ;; *) bad "and names the switch" "$SUMMARY" ;; esac |
There was a problem hiding this comment.
🟢 Low — Phase 6 only exercises the summary for the input-decided disable (run false ""); it never runs run true '{"enabled": false}' and asserts on the summary, which is precisely the kill-switch state whose remediation text is wrong. The suite also never asserts the script's exit status on any path, so a future non-zero exit would fail the gate job in CI while every test still reports green. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).
| if [ -n "${raw//[[:space:]]/}" ]; then | ||
| if ! jq -e 'type == "object"' >/dev/null 2>&1 <<<"$raw"; then | ||
| warn "vars.RISK_CONFIG is set but is not a JSON object — ignoring it and using the reviewed input (enabled=${reviewed}). Expected {\"enabled\": true}." | ||
| elif jq -e 'has("enabled")' >/dev/null 2>&1 <<<"$raw"; then |
There was a problem hiding this comment.
🟢 Low — jq -e exit status reflects only the truthiness of the LAST output value, so a multi-document RISK_CONFIG like {"enabled": true} {} passes the type == "object" test but makes has("enabled") exit 1, dropping into the deliberately-silent no-key path — the operator's enable is discarded with no warning at all. Assert a single document, e.g. jq -se 'length == 1 and (.[0] | type == "object")'. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).
| INPUT_ENABLED: ${{ inputs.enabled }} | ||
| # Empty when the caller repo has no such variable — which is the common case and is | ||
| # NOT an error; it just means the reviewed input decides. | ||
| RISK_CONFIG: ${{ vars.RISK_CONFIG }} |
There was a problem hiding this comment.
🟢 Low — vars.RISK_CONFIG is an out-of-band disable for a compliance-adjacent check that leaves no trace in git history, and because vars also resolves organization-level variables, one org variable of {"enabled": false} turns grading off across every enrolled repo at once. The disabled path deliberately emits no ::warning::, so the only evidence is a step summary nobody opens on a green run — emit an annotation whenever the variable is what turned grading off. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).
| echo "## PR risk grading is DISABLED for this repository" | ||
| echo | ||
| echo "No grade was computed, and no \`risk:*\` label was added, changed, or removed —" | ||
| echo "any label already on this PR is left exactly as the last enabled run left it." |
There was a problem hiding this comment.
⚪ Nit — The summary says "any label already on this PR is left exactly as the last enabled run left it", but the gate also runs on the pr_numbers batch-dispatch path where there are N target PRs (and on a dispatch with no target, none at all). Reword so it does not assume a single event-scoped pull request. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).
2 participants
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:
This works because the
varscontext inside a reusable resolves against the caller's repository — the same mechanismgroom.ymlalready uses forvars.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 pinsenabled: 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
enabledkey 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.shrather than inline YAML so it's testable — the same reasoning that moved the target loop intograde-targets.shin #113. That paid for itself immediately:jq -esets 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
gatejob resolving enablement, holding no token scopes (permissions: {}), withgradegated on its output. Its own job rather than a first step ofgradeso: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"/ numeric1) 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.ymlexplicitly — that workflow enumerates its suites rather than globbing, so a new file is otherwise silently un-run. Added to itsshellchecklist for the same reason.All four suites pass;
shellcheck -xclean; both workflow files parse.Consumer impact — read this before merging
This is a breaking change for the two live consumers, by design.
Comfy-Org/cloudandComfy-Org/comfy-cloud-mcp-serverare 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:
enabled: trueto the caller'swith:block in the same PR as the pin bump, orRISK_CONFIGto{"enabled": true}before merging the bumpThe 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.