fix(pr-risk): let a manual dispatch grade while enabled is false - #116
fix(pr-risk): let a manual dispatch grade while enabled is false#116mattmillerai wants to merge 1 commit into
enabled is false#116Conversation
The switch was gating every path, including `workflow_dispatch`, which broke the staged rollout it exists for: a repo enrolled OFF could not 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 thing being evaluated. `enabled` now governs AUTOMATIC grading, i.e. the `pull_request` stream. A dispatch requires write access on the repo and is an explicit, attributed act, so there is no unattended behaviour left for the switch to govern. Stated rather than buried, in the input description and beside the job `if:`: false is now a MUTE on the PR stream, not a lockout. A repo needing the harder guarantee should remove the caller — nothing here can bind a maintainer who can already edit the workflow file. The gate's step summary had to learn the difference, and this is the part worth reviewing: it claimed "No grade was computed, and no risk:* label was added, changed, or removed", which on a dispatch that is about to do exactly that is the single most misleading line this workflow could emit — it is the sentence a reader trusts to decide whether the label on their PR is current. A disabled dispatch now says it grades anyway, and warns that the label it leaves will go stale because pushes will not re-grade it. The disabled pull_request summary keeps its claim and gains a pointer to the manual route.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 8 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 4 |
| 🟢 Low | 3 |
Panel: 8/8 reviewers contributed findings.
| # bind a maintainer who can edit the workflow file anyway. | ||
| if: >- | ||
| needs.gate.outputs.enabled == 'true' || | ||
| github.event_name == 'workflow_dispatch' |
There was a problem hiding this comment.
🟠 High — The exemption is keyed on the event alone, but workflow_dispatch is not necessarily a human act: any principal with actions: write can fire it via gh workflow run or the REST endpoint, and since this file is on: workflow_call only, github.event_name reports the CALLER's event — a dispatch aimed at something else still satisfies the clause. That turns {"enabled": false} into a mute rather than a kill switch for the privileged grade job (pull-requests: write + issues: write), which can then relabel up to MAX_TARGETS=50 PRs per run. Gate on an explicit signal instead (e.g. a dedicated manual_grade input the caller must forward, or requiring a non-empty pr_number/pr_numbers). Raised by 4 of 8 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max edge-case, kimi-k2.7-code adversarial).
| if: >- | ||
| needs.gate.outputs.enabled == 'true' || | ||
| github.event_name == 'workflow_dispatch' | ||
| name: Grade PR risk |
There was a problem hiding this comment.
🟡 Medium — Nothing in the clause requires a resolvable target. pr_number/pr_numbers are both required: false, so the default Run-workflow form supplies neither, and on a dispatch there is no github.event.pull_request to fall back to — PR_NUMBERS resolves empty and grade-targets.sh hits its die in parse_targets (exit 2). A disabled repo that previously skipped cleanly now gets a red run; conditioning on (inputs.pr_number != '' || inputs.pr_numbers != '') fixes both this and part of the exemption's blast radius. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial).
| { | ||
| echo "## Automatic PR risk grading is OFF for this repository" | ||
| echo | ||
| echo "This MANUAL run grades anyway and will sync a \`risk:*\` label — \`enabled\` governs" |
There was a problem hiding this comment.
🟡 Medium — This gate step runs before grade and asserts an outcome it cannot know: a dispatch with no target dies in parse_targets, an unreadable input lands risk:ungraded, and a 403 on the label write applies nothing — in each case the summary has already promised a risk:* sync. That is the same class of false claim this change exists to remove, pointed the other way; word it as intent ("this manual run will attempt a grade") rather than settled fact. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial).
| # MUST distinguish the two. Claiming "no grade was computed" on a run that is about to compute | ||
| # one is worse than saying nothing: it is the one line a reader would trust to tell them | ||
| # whether their label is current. | ||
| if [ "${GITHUB_EVENT_NAME:-}" = workflow_dispatch ]; then |
There was a problem hiding this comment.
🟡 Medium — GITHUB_EVENT_NAME is a new decision input for this script but is not pinned in the resolve step's env: block in pr-risk.yml (which declares only INPUT_ENABLED and RISK_CONFIG), so it relies on the runner default. The ${GITHUB_EVENT_NAME:-} fallback silently selects the "No grade was computed" branch when the value is absent — precisely the misleading text this change exists to suppress — so a missing value degrades to the wrong message rather than an error. Add it to the step's env:. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).
| | [`assign-prs-to-author.yml`](.github/workflows/assign-prs-to-author.yml) | Housekeeping — assigns every open PR with no assignees to its author (bot-authored PRs skipped by default). Run on a schedule from a thin caller; useful when a team tracks PR ownership via assignees. The calling job needs `pull-requests: write` and `issues: write`. | | ||
| | [`pr-size.yml`](.github/workflows/pr-size.yml) | PR-size cap — fails (or, in `mode: warn`, only reports) when a PR's net diff exceeds `max_lines` non-generated changed lines, keeping diffs reviewable. Excludes dependency lockfiles, `linguist-generated` files (read from the base ref, so a PR can't exempt itself), Go generated-code markers, and per-repo `extra_lockfiles` / `extra_generated_globs`. A `bypass_label` (default `oversized-ok`) waves through a legitimately large change; a sticky bot comment explains overages when `bot_app_id` + `BOT_APP_PRIVATE_KEY` are supplied (degrades to status + step summary without them). Counting logic + tests live in [`scripts/check-pr-size/`](scripts/check-pr-size). | | ||
| | [`pr-risk.yml`](.github/workflows/pr-risk.yml) | **Advisory PR risk grading (shadow check)** — **off by default** (`enabled: false`); switch it on with `enabled: true` or by setting the caller repo's `RISK_CONFIG` variable to `{"enabled": true}`, which outranks the input in both directions so `{"enabled": false}` is a no-PR kill switch. Grades every PR into a tier `R0` (safest) .. `R3` (riskiest) and syncs one label (`risk:R0`..`risk:R3`, or `risk:ungraded` when an input was unreadable). The label is the entire product: nothing is gated, routed, commented, or merged. Deterministic (`gh` + `jq`, no LLM): `grade = worst(path_floor, provenance, reversibility)` — path-glob map, what-process-produced-the-diff (registered runbooks with identity + diff-shape assertions; forks are R3 with no exceptions), and revertability (persistent-state mutation, deletions under sensitive classes, did green checks cover the lines). Grader + generic defaults live in [`scripts/pr-risk/`](scripts/pr-risk); a consumer sharpens them with `.github/risk.json` / `.github/risk-runbooks.json`, read from the PR's **base ref** so a PR can't edit the rules that judge it. The job excludes its own run from the check rollup and waits (`wait_for_checks_minutes`) for the rest to settle before labeling. Labels ride the plain `GITHUB_TOKEN` (cannot fire `labeled` triggers — no cascade risk); disagreement is recorded with a human-owned `risk-dispute` label. Label text is remappable via `label_map`. `workflows_ref` is **required** — pin it to the same full commit SHA as `uses:`, so the grader cannot be loaded from a floating ref after the caller was reviewed. Enroll it as its own workflow rather than a job inside an existing CI workflow (the rollup exclusion is per-run). The calling job needs `contents: read` + `issues: write` + `pull-requests: write` + `checks: read` + `actions: read` + `statuses: read`; GitHub rejects a shorter grant at startup (a reusable workflow can only narrow the caller's token, never elevate it), so a caller enrolled from an older copy of this row fails before any step runs. Both writes are the ONE label: repo-side label creation on first use maps to `issues`, and labeling a PR maps to `pull-requests` (the labels endpoint is dual-mapped by what the "issue" is, so `issues: write` alone 403s on a PR). `actions: read` is for the rollup's `CheckRun -> checkSuite -> workflowRun` self-exclusion hop. No secrets. | | ||
| | [`pr-risk.yml`](.github/workflows/pr-risk.yml) | **Advisory PR risk grading (shadow check)** — **automatic grading off by default** (`enabled: false`; a manual `workflow_dispatch` grades regardless, so a repo can trial it before switching on); switch it on with `enabled: true` or by setting the caller repo's `RISK_CONFIG` variable to `{"enabled": true}`, which outranks the input in both directions so `{"enabled": false}` is a no-PR kill switch. Grades every PR into a tier `R0` (safest) .. `R3` (riskiest) and syncs one label (`risk:R0`..`risk:R3`, or `risk:ungraded` when an input was unreadable). The label is the entire product: nothing is gated, routed, commented, or merged. Deterministic (`gh` + `jq`, no LLM): `grade = worst(path_floor, provenance, reversibility)` — path-glob map, what-process-produced-the-diff (registered runbooks with identity + diff-shape assertions; forks are R3 with no exceptions), and revertability (persistent-state mutation, deletions under sensitive classes, did green checks cover the lines). Grader + generic defaults live in [`scripts/pr-risk/`](scripts/pr-risk); a consumer sharpens them with `.github/risk.json` / `.github/risk-runbooks.json`, read from the PR's **base ref** so a PR can't edit the rules that judge it. The job excludes its own run from the check rollup and waits (`wait_for_checks_minutes`) for the rest to settle before labeling. Labels ride the plain `GITHUB_TOKEN` (cannot fire `labeled` triggers — no cascade risk); disagreement is recorded with a human-owned `risk-dispute` label. Label text is remappable via `label_map`. `workflows_ref` is **required** — pin it to the same full commit SHA as `uses:`, so the grader cannot be loaded from a floating ref after the caller was reviewed. Enroll it as its own workflow rather than a job inside an existing CI workflow (the rollup exclusion is per-run). The calling job needs `contents: read` + `issues: write` + `pull-requests: write` + `checks: read` + `actions: read` + `statuses: read`; GitHub rejects a shorter grant at startup (a reusable workflow can only narrow the caller's token, never elevate it), so a caller enrolled from an older copy of this row fails before any step runs. Both writes are the ONE label: repo-side label creation on first use maps to `issues`, and labeling a PR maps to `pull-requests` (the labels endpoint is dual-mapped by what the "issue" is, so `issues: write` alone 403s on a PR). `actions: read` is for the rollup's `CheckRun -> checkSuite -> workflowRun` self-exclusion hop. No secrets. | |
There was a problem hiding this comment.
🟡 Medium — The updated row contradicts itself in one sentence: a manual workflow_dispatch "grades regardless", yet {"enabled": false} is still described as "a no-PR kill switch". That is the sentence an enrolling repo reads to decide whether the switch is a hard stop, and an operator reaching for it during an incident gets a guarantee the workflow no longer provides — qualify it as governing the automatic PR stream only. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).
| *) ok "a disabled dispatch does NOT claim nothing was graded" ;; | ||
| esac | ||
| case "$SUMMARY" in | ||
| *"This MANUAL run grades anyway"*) ok "it says the manual run will grade" ;; |
There was a problem hiding this comment.
🟢 Low — Phase 7 asserts the summary claims a manual run grades, but nothing tests the if: clause in pr-risk.yml that makes the claim true — the two halves sit in different files with no coupling. Dropping || github.event_name == 'workflow_dispatch' from the job condition leaves this suite fully green while every disabled dispatch announces a grade that was never computed. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).
| echo | ||
| echo "Decided by ${decided_by}. To grade automatically from here on, set the repository" | ||
| echo 'variable `RISK_CONFIG` to `{"enabled": true}` (no PR needed), or pass' | ||
| echo '`enabled: true` in the caller'"'"'s `with:` block.' |
There was a problem hiding this comment.
🟢 Low — "or pass enabled: true in the caller's with: block" is wrong precisely when decided_by is vars.RISK_CONFIG: the variable outranks the input in both directions, so with {"enabled": false} set, that route changes nothing. Suppress the input alternative (or invert it to "clear/flip the variable") when the variable made the decision. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).
| 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.' | ||
| echo | ||
| echo 'To grade a single PR right now WITHOUT switching the repo on, run this workflow from' |
There was a problem hiding this comment.
🟢 Low — The new escape-hatch advice assumes the consumer's caller declares a workflow_dispatch trigger with a pr_number input and forwards it; the reusable is on: workflow_call only and cannot be run from the Actions tab, so for a pull_request-only caller (a valid enrollment) this points at a button that does not exist. It also omits pr_numbers, the other accepted target input. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).
1 participant
ELI-5
enabled: falsewas 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.enablednow governs automatic grading (thepull_requeststream). A manualworkflow_dispatchgrades regardless.pull_requestworkflow_dispatchenabled: falseenabled: trueWhy a dispatch is safe to exempt
Running a
workflow_dispatchrequires 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:falseis 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:
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:pull_request→ keeps its original claim (accurate there) and gains a pointer to the manual route.Verification
test_resolve_enabled.shis 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 -xclean; 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.