feat(pr-risk): advisory PR risk grader + label-only shadow check (reusable) - #111
feat(pr-risk): advisory PR risk grader + label-only shadow check (reusable)#111mattmillerai wants to merge 1 commit into
Conversation
…sable) Grades every PR event into R0..R3 = worst(path_floor, provenance, reversibility) and syncs one label; nothing gates, routes, or merges. Deterministic gh+jq, no LLM. Generic default map/registry here; consumers sharpen via .github/risk.json read from the PR base ref. The grading job excludes its own run from the check rollup it reads and waits for the rest to settle, so live grades are not floored at R2 by the measurement itself.
📝 WalkthroughWalkthroughAdds deterministic PR risk grading across path, provenance, and reversibility axes. Adds configurable risk labels, base-ref overrides, pending-check handling, advisory summaries, tests, and documentation. ChangesPR risk grading
Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant pr-risk.yml
participant grade-pr-risk.sh
participant apply-risk-label.sh
PullRequest->>pr-risk.yml: Trigger reusable workflow
pr-risk.yml->>grade-pr-risk.sh: Load configuration and grade PR
grade-pr-risk.sh-->>pr-risk.yml: Return tier and axis reasons
pr-risk.yml->>apply-risk-label.sh: Synchronize risk label
apply-risk-label.sh-->>PullRequest: Update PR labels
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-risk.yml:
- Around line 105-111: Update the workflows_ref input in the pr-risk workflow to
be required and remove its floating main default, ensuring callers explicitly
provide the pinned repository reference used to load grader and workflow
scripts.
- Around line 121-126: Add inline documentation to the permissions block in the
workflow job, placing the existing header’s rationale beside each granted scope,
especially issues: write. Document contents, pull-requests, checks, and statuses
as well, without changing their permission levels.
- Around line 190-191: Clamp the wait duration used to compute deadline in the
workflow’s wait-for-checks step to the job’s available timeout, ensuring
wait_for_checks_minutes cannot make the job sleep past timeout-minutes and
preventing the subsequent label step from being skipped.
- Around line 192-207: Update the retry loop around the bash "$TOOL" invocation
so rc=3 is retried with bounded backoff until a defined retry limit or the
existing deadline, labeling the PR ungraded only after retries are exhausted;
retain immediate failure for other unexpected codes. Replace the fixed 30-second
pending poll in this loop with increasing bounded backoff while preserving the
deadline and checks_pending_excl_self behavior.
- Around line 1-57: Add a header comment to the reusable workflow documenting
that callers provide no secrets and that it uses only GITHUB_TOKEN. Create or
move the semver-major v1 Git tag to the commit containing this workflow, without
changing the existing README catalog entry.
In @.github/workflows/test-pr-risk.yml:
- Around line 1-8: Add the required workflow header before name in the Test
pr-risk scripts workflow, documenting its event triggers, no inputs, no secrets,
and no reusable-workflow caller pattern. Preserve the existing workflow name and
content below the new header.
In `@scripts/pr-risk/apply-risk-label.sh`:
- Around line 92-110: URL-encode label names before interpolating them into
GitHub API path segments in the stale-label DELETE and target-label lookup flows
around has and the label-management loop. Apply the encoding to both l and
TARGET while preserving the existing raw names for logging, form fields, and
label comparisons so names containing spaces or slashes produce valid requests.
- Around line 86-87: Update the labels API request assigned to current in the
label-reading flow to include gh’s --paginate option, ensuring all labels are
loaded before has() and the removal loop evaluate them. Preserve the existing
error handling and JSON output format.
In `@scripts/pr-risk/grade-pr-risk.sh`:
- Around line 356-361: Update the PR risk GraphQL query and the parsing logic
around changed_paths_status to paginate the files connection through endCursor
until hasNextPage is false, retaining unknown only when a hard page-count cap is
reached. Aggregate all returned file nodes before grading so large PRs receive
normal path and reversibility scores. Also paginate labels beyond the first 20
and merge all label pages before detecting agent-coded or related provenance
labels.
- Around line 291-292: Move the test-file matching patterns used by the
$touched_test calculation into the versioned risk map, while retaining the
current regex as the fallback when the map key is absent or empty. Add the
matching key to risk-map.v0.json and update the map validation to document and
validate it, including patterns covering Python, Java, and Ruby test naming
conventions.
In `@scripts/pr-risk/risk-map.v0.json`:
- Around line 24-25: Update scripts/pr-risk/risk-map.v0.json at lines 24-25 to
use **/risk-map.*.json and include **/runbook-registry.*.json, **/pr-risk/**,
and **/.github/risk-runbooks.json; update lines 53-54 from *-test.sh to
**/*-test.sh. Add a regression case in
scripts/pr-risk/tests/test_grade_pr_risk.sh that grades a nested
scripts/pr-risk/risk-map.v0.json path and asserts path_floor is R3.
- Around line 53-54: Update the "tests" class path patterns in the risk-map
configuration so the shell-test glob matches files at any directory depth, not
only root-level files. Change the existing "*-test.sh" pattern to its recursive
equivalent while preserving the other test path patterns and classification.
In `@scripts/pr-risk/runbook-registry.v0.json`:
- Around line 31-33: Update the permitted_paths configuration associated with
the deps rule to include every dependency-manifest pattern listed by deps in
risk-map.v0.json, including Gemfile, Gemfile.lock, composer.json, composer.lock,
*.gemspec, poetry.lock, Pipfile, Pipfile.lock, npm-shrinkwrap.json, setup.py,
and go.work*. Prefer deriving this set from the deps rule if the registry
supports it; otherwise extend the existing list while preserving all current
entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0a3cd30f-3f43-4461-b395-8b2f25aa8a0d
📒 Files selected for processing (10)
.github/workflows/pr-risk.yml.github/workflows/test-pr-risk.ymlREADME.mdscripts/pr-risk/README.mdscripts/pr-risk/apply-risk-label.shscripts/pr-risk/grade-pr-risk.shscripts/pr-risk/risk-map.v0.jsonscripts/pr-risk/runbook-registry.v0.jsonscripts/pr-risk/tests/test_apply_risk_label.shscripts/pr-risk/tests/test_grade_pr_risk.sh
| name: PR Risk Grade (reusable) | ||
|
|
||
| # Reusable ADVISORY PR risk grader — the shadow-check rung of the PR risk-grading ladder. | ||
| # Grades every PR event into a tier R0 (safest) .. R3 (riskiest) and syncs ONE label | ||
| # (`risk:R0` .. `risk:R3`, or `risk:ungraded` when an input was unreadable). That label is | ||
| # the entire product: nothing is gated, nothing is blocked, nothing merges, no comment is | ||
| # posted. Humans look at the label and agree or disagree; disagreement is recorded by adding | ||
| # the `risk-dispute` label (which this workflow never touches) plus a comment saying why. | ||
| # | ||
| # grade = worst(path_floor, provenance, reversibility) — three deterministic axes; the worst | ||
| # tier wins, so no axis can move a PR into a safer lane than another axis put it. No LLM, no | ||
| # model call anywhere: `gh` + `jq` over the PR's own API record. See | ||
| # scripts/pr-risk/grade-pr-risk.sh for the axes and the unknown contract. | ||
| # | ||
| # The grader and its default risk map load from THIS repo at the pinned `workflows_ref`, | ||
| # never from the graded PR's checkout (no PR code is checked out at all) — a PR cannot edit | ||
| # the rules that judge it. A consumer repo sharpens the generic defaults by committing | ||
| # `.github/risk.json` (map) / `.github/risk-runbooks.json` (runbook registry), which are read | ||
| # from the PR's BASE ref: present-but-invalid fails the run loudly; absent falls back to the | ||
| # defaults in scripts/pr-risk/. | ||
| # | ||
| # CHECKS SETTLE BEFORE THE LABEL DOES: the reversibility axis asks "did tests covering these | ||
| # lines actually run", and at event time the rest of the rollup is usually still running (the | ||
| # grading job itself is excluded from the rollup it reads — see --self-context in the | ||
| # grader). The job re-polls until the other checks settle or `wait_for_checks_minutes` runs | ||
| # out, then labels what it has. Pair the caller with a per-PR cancel-in-progress concurrency | ||
| # group so a new push supersedes a waiting run instead of stacking behind it. | ||
| # | ||
| # The label is applied with the plain GITHUB_TOKEN on purpose: GITHUB_TOKEN-applied labels | ||
| # cannot fire `labeled` triggers, so the shadow check is structurally unable to start a | ||
| # workflow cascade. Fork PRs under a plain `pull_request` trigger get a read-only token and | ||
| # the label write will fail — enroll public repos with `pull_request_target` instead (safe | ||
| # here by construction: this workflow never checks out or executes PR code). | ||
| # | ||
| # Caller pattern (consumer repo, .github/workflows/ci-pr-risk.yml): | ||
| # | ||
| # name: CI - PR Risk Grade | ||
| # on: | ||
| # pull_request: | ||
| # types: [opened, synchronize, reopened, ready_for_review] | ||
| # concurrency: | ||
| # group: pr-risk-${{ github.event.pull_request.number }} | ||
| # cancel-in-progress: true | ||
| # permissions: | ||
| # contents: read | ||
| # jobs: | ||
| # pr-risk: | ||
| # permissions: | ||
| # contents: read | ||
| # issues: write # the risk label rides the issues API | ||
| # pull-requests: read | ||
| # checks: read # the check rollup the reversibility axis reads | ||
| # statuses: read | ||
| # uses: Comfy-Org/github-workflows/.github/workflows/pr-risk.yml@<sha> # v1 | ||
| # with: | ||
| # workflows_ref: <same sha> | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the README catalog for a pr-risk.yml entry and confirm existing major-tag practice.
set -uo pipefail
rg -n -C 3 'pr-risk' README.md || echo "NO pr-risk row in README.md"
echo "--- catalog rows for comparison ---"
rg -n -C 2 '\.github/workflows/[a-z0-9-]+\.yml@' README.md | head -60
echo "--- existing tags ---"
git tag --list | head -40Repository: Comfy-Org/github-workflows
Length of output: 7339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- workflow contract ---'
sed -n '1,140p' .github/workflows/pr-risk.yml
printf '%s\n' '--- README catalog header and pr-risk row ---'
sed -n '1,28p' README.md
printf '%s\n' '--- versioning and reusable-workflow references ---'
rg -n -i 'major tag|semver|version|@<sha>|workflow.*@|reusable' README.md .github . || true
printf '%s\n' '--- repository refs and workflow declarations ---'
git show-ref --tags || true
rg -n '^\s*(on:|workflow_call:|secrets:|inputs:|workflows_ref:)' .github/workflows/pr-risk.ymlRepository: Comfy-Org/github-workflows
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- README versioning and new-workflow requirements ---'
sed -n '44,72p' README.md
printf '%s\n' '--- pr-risk secret references ---'
rg -n -i 'secret|token|github\.token|secrets\.' .github/workflows/pr-risk.yml
printf '%s\n' '--- workflow header contract markers ---'
sed -n '1,70p' .github/workflows/pr-risk.yml | rg -n -i 'trigger|input|secret|caller|workflow_call|GITHUB_TOKEN'
printf '%s\n' '--- tag refs ---'
git show-ref --tags || trueRepository: Comfy-Org/github-workflows
Length of output: 1931
Document the secret contract and publish the v1 tag.
Add a header line stating that no caller secrets are required and that the workflow uses only GITHUB_TOKEN. Create or move the semver major v1 tag; no tag currently exists. The README catalog entry is already present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pr-risk.yml around lines 1 - 57, Add a header comment to
the reusable workflow documenting that callers provide no secrets and that it
uses only GITHUB_TOKEN. Create or move the semver-major v1 Git tag to the commit
containing this workflow, without changing the existing README catalog entry.
Source: Coding guidelines
| workflows_ref: | ||
| description: >- | ||
| Ref of Comfy-Org/github-workflows to load the grader + default map | ||
| from. Pin to the same ref you pin `uses:` to. | ||
| type: string | ||
| required: false | ||
| default: main |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
workflows_ref must not default to main.
The header at Lines 15-16 states the grader loads from "THIS repo at the pinned workflows_ref". The default is main, which is a floating ref. A caller who pins uses: to a SHA and omits workflows_ref gets a SHA-pinned workflow that then downloads grader code from HEAD of main. The two halves of the same tool drift apart, and the grading logic becomes mutable after review.
Make the input required, so the caller states the ref, and the run fails loudly when they forget. A floating default is a supply chain with no chain.
As per coding guidelines: "Load reviewer, checker, and other workflow scripts from a pinned reference of this repository at runtime, never from the caller's checkout or a local path."
🔒️ Proposed fix
workflows_ref:
description: >-
Ref of Comfy-Org/github-workflows to load the grader + default map
- from. Pin to the same ref you pin `uses:` to.
+ from. Pin to the SAME commit SHA you pin `uses:` to. Required: a
+ floating default would let the grader code drift from the pinned
+ workflow that loads it.
type: string
- required: false
- default: main
+ required: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| workflows_ref: | |
| description: >- | |
| Ref of Comfy-Org/github-workflows to load the grader + default map | |
| from. Pin to the same ref you pin `uses:` to. | |
| type: string | |
| required: false | |
| default: main | |
| workflows_ref: | |
| description: >- | |
| Ref of Comfy-Org/github-workflows to load the grader + default map | |
| from. Pin to the SAME commit SHA you pin `uses:` to. Required: a | |
| floating default would let the grader code drift from the pinned | |
| workflow that loads it. | |
| type: string | |
| required: true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pr-risk.yml around lines 105 - 111, Update the
workflows_ref input in the pr-risk workflow to be required and remove its
floating main default, ensuring callers explicitly provide the pinned repository
reference used to load grader and workflow scripts.
Source: Coding guidelines
| permissions: | ||
| contents: read | ||
| issues: write | ||
| pull-requests: read | ||
| checks: read | ||
| statuses: read |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Document the job permissions.
zizmor reports undocumented-permissions at Line 123. Each scope here has a specific reason, and the header already explains them. Move that reasoning next to the grant so a reader auditing issues: write does not have to hunt for it.
♻️ Proposed change
permissions:
- contents: read
- issues: write
- pull-requests: read
- checks: read
- statuses: read
+ contents: read # read the base-ref risk config via the contents API
+ issues: write # the one write: sync the risk label (labels ride the issues API)
+ pull-requests: read # read the PR record the grader grades
+ checks: read # the check rollup the reversibility axis reads
+ statuses: read # commit statuses in the same rollup📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| permissions: | |
| contents: read | |
| issues: write | |
| pull-requests: read | |
| checks: read | |
| statuses: read | |
| permissions: | |
| contents: read # read the base-ref risk config via the contents API | |
| issues: write # the one write: sync the risk label (labels ride the issues API) | |
| pull-requests: read # read the PR record the grader grades | |
| checks: read # the check rollup the reversibility axis reads | |
| statuses: read # commit statuses in the same rollup |
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 123-123: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pr-risk.yml around lines 121 - 126, Add inline
documentation to the permissions block in the workflow job, placing the existing
header’s rationale beside each granted scope, especially issues: write. Document
contents, pull-requests, checks, and statuses as well, without changing their
permission levels.
Source: Linters/SAST tools
| deadline=$(( $(date +%s) + 60 * WAIT_MINUTES )) | ||
| waited=0 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clamp the wait against the job timeout.
timeout-minutes is 30. The input description asks the caller to keep wait_for_checks_minutes at or below 25, but nothing enforces it. A caller who passes 40 gets a job that is cancelled while still sleeping: the label step never runs, and the PR keeps whatever label the previous push left. The check goes red for a reason no one will guess. Clamp in the workflow instead of trusting the reader.
🐛 Proposed fix
+ # The job times out at 30 minutes; never wait past the point where
+ # the label step can still run.
+ [ "$WAIT_MINUTES" -le 25 ] 2>/dev/null || WAIT_MINUTES=25
deadline=$(( $(date +%s) + 60 * WAIT_MINUTES ))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deadline=$(( $(date +%s) + 60 * WAIT_MINUTES )) | |
| waited=0 | |
| # The job times out at 30 minutes; never wait past the point where | |
| # the label step can still run. | |
| [ "$WAIT_MINUTES" -le 25 ] 2>/dev/null || WAIT_MINUTES=25 | |
| deadline=$(( $(date +%s) + 60 * WAIT_MINUTES )) | |
| waited=0 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pr-risk.yml around lines 190 - 191, Clamp the wait
duration used to compute deadline in the workflow’s wait-for-checks step to the
job’s available timeout, ensuring wait_for_checks_minutes cannot make the job
sleep past timeout-minutes and preventing the subsequent label step from being
skipped.
| while :; do | ||
| rc=0 | ||
| bash "$TOOL" "${ARGS[@]}" > record.json || rc=$? | ||
| case "$rc" in | ||
| 0|1) ;; # graded (1 = graded but unknown — still labeled) | ||
| 3) echo "PR unreadable via the API — labeling ungraded" | ||
| printf '{}' > record.json | ||
| break ;; | ||
| *) echo "grader failed (rc=$rc)"; exit 1 ;; # setup/usage bug: fail loud | ||
| esac | ||
| pending="$(jq -r '.checks_pending_excl_self // false' record.json)" | ||
| if [ "$pending" = "true" ] && [ "$(date +%s)" -lt "$deadline" ]; then | ||
| sleep 30; waited=$(( waited + 30 )); continue | ||
| fi | ||
| break | ||
| done |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Retry an unreadable PR before labeling it ungraded, and reconsider the fixed 30s poll.
Two points on this loop.
First, rc=3 breaks immediately. Exit 3 fires whenever the GraphQL call fails, which includes rate limits, secondary rate limits, and transient 5xx. One blip therefore produces risk:ungraded, and the summary tells the reader the PR could not be read — a durable verdict from a momentary hiccup. The loop already owns a deadline; use it. Retry rc=3 a few times with backoff, and label ungraded only when the retries are exhausted.
Second, the poll is a fixed 30s sleep for up to wait_for_checks_minutes. At the default that is a runner idling up to 10 minutes on every synchronize event, per PR, across the fleet. The grade itself takes one API call. Increasing backoff would cut both runner minutes and API calls with no change to the settle behaviour. Waiting is fine; waiting at full price is not.
♻️ Proposed change: bounded retry on rc=3 plus backoff
deadline=$(( $(date +%s) + 60 * WAIT_MINUTES ))
waited=0
+ unreadable=0
+ nap=15
while :; do
rc=0
bash "$TOOL" "${ARGS[@]}" > record.json || rc=$?
case "$rc" in
0|1) ;; # graded (1 = graded but unknown — still labeled)
- 3) echo "PR unreadable via the API — labeling ungraded"
+ 3) unreadable=$(( unreadable + 1 ))
+ if [ "$unreadable" -lt 3 ] && [ "$(date +%s)" -lt "$deadline" ]; then
+ echo "PR read failed (attempt $unreadable) — retrying in ${nap}s"
+ sleep "$nap"; waited=$(( waited + nap )); nap=$(( nap * 2 )); continue
+ fi
+ echo "PR unreadable via the API after $unreadable attempts — labeling ungraded"
printf '{}' > record.json
break ;;
*) echo "grader failed (rc=$rc)"; exit 1 ;; # setup/usage bug: fail loud
esac
pending="$(jq -r '.checks_pending_excl_self // false' record.json)"
if [ "$pending" = "true" ] && [ "$(date +%s)" -lt "$deadline" ]; then
- sleep 30; waited=$(( waited + 30 )); continue
+ sleep "$nap"; waited=$(( waited + nap ))
+ [ "$nap" -lt 120 ] && nap=$(( nap * 2 ))
+ continue
fi
break
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while :; do | |
| rc=0 | |
| bash "$TOOL" "${ARGS[@]}" > record.json || rc=$? | |
| case "$rc" in | |
| 0|1) ;; # graded (1 = graded but unknown — still labeled) | |
| 3) echo "PR unreadable via the API — labeling ungraded" | |
| printf '{}' > record.json | |
| break ;; | |
| *) echo "grader failed (rc=$rc)"; exit 1 ;; # setup/usage bug: fail loud | |
| esac | |
| pending="$(jq -r '.checks_pending_excl_self // false' record.json)" | |
| if [ "$pending" = "true" ] && [ "$(date +%s)" -lt "$deadline" ]; then | |
| sleep 30; waited=$(( waited + 30 )); continue | |
| fi | |
| break | |
| done | |
| deadline=$(( $(date +%s) + 60 * WAIT_MINUTES )) | |
| waited=0 | |
| unreadable=0 | |
| nap=15 | |
| while :; do | |
| rc=0 | |
| bash "$TOOL" "${ARGS[@]}" > record.json || rc=$? | |
| case "$rc" in | |
| 0|1) ;; # graded (1 = graded but unknown — still labeled) | |
| 3) unreadable=$(( unreadable + 1 )) | |
| if [ "$unreadable" -lt 3 ] && [ "$(date +%s)" -lt "$deadline" ]; then | |
| echo "PR read failed (attempt $unreadable) — retrying in ${nap}s" | |
| sleep "$nap"; waited=$(( waited + nap )); nap=$(( nap * 2 )); continue | |
| fi | |
| echo "PR unreadable via the API after $unreadable attempts — labeling ungraded" | |
| printf '{}' > record.json | |
| break ;; | |
| *) echo "grader failed (rc=$rc)"; exit 1 ;; # setup/usage bug: fail loud | |
| esac | |
| pending="$(jq -r '.checks_pending_excl_self // false' record.json)" | |
| if [ "$pending" = "true" ] && [ "$(date +%s)" -lt "$deadline" ]; then | |
| sleep "$nap"; waited=$(( waited + nap )) | |
| [ "$nap" -lt 120 ] && nap=$(( nap * 2 )) | |
| continue | |
| fi | |
| break | |
| done |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pr-risk.yml around lines 192 - 207, Update the retry loop
around the bash "$TOOL" invocation so rc=3 is retried with bounded backoff until
a defined retry limit or the existing deadline, labeling the PR ungraded only
after retries are exhausted; retain immediate failure for other unexpected
codes. Replace the fixed 30-second pending poll in this loop with increasing
bounded backoff while preserving the deadline and checks_pending_excl_self
behavior.
| | ($plist | any(matches_any($M.flippable_flag_paths // []))) as $flag | ||
| | ($plist | any(test("(_test\\.|\\.test\\.|\\.spec\\.|(^|/)tests?/|-test\\.sh$)"))) as $touched_test |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Move the test-file patterns into the versioned map.
Line 292 hardcodes the "did a test file change" heuristic. Every other input on this axis comes from the map. The regex misses test_*.py, *_test.py, *Test.java, and *_spec.rb, so a Python or Java consumer can never reach clean_tier and sits at R1 forever. A consumer cannot fix that with .github/risk.json, which is the one lever the workflow gives them. Put the patterns in the map, and keep the current regex as the fallback — a map-shaped test beats a test that is map-shaped in name only.
♻️ Proposed change: read glob patterns from the map
- | ($plist | any(test("(_test\\.|\\.test\\.|\\.spec\\.|(^|/)tests?/|-test\\.sh$)"))) as $touched_test
+ | (($M.test_path_globs // []) as $tg
+ | if ($tg | length) > 0 then ($plist | any(matches_any($tg)))
+ else ($plist | any(test("(_test\\.|\\.test\\.|\\.spec\\.|(^|/)tests?/|-test\\.sh$)"))) end) as $touched_testAdd the matching key to scripts/pr-risk/risk-map.v0.json and document it in the map validation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | ($plist | any(matches_any($M.flippable_flag_paths // []))) as $flag | |
| | ($plist | any(test("(_test\\.|\\.test\\.|\\.spec\\.|(^|/)tests?/|-test\\.sh$)"))) as $touched_test | |
| | ($plist | any(matches_any($M.flippable_flag_paths // []))) as $flag | |
| | (($M.test_path_globs // []) as $tg | |
| | if ($tg | length) > 0 then ($plist | any(matches_any($tg))) | |
| else ($plist | any(test("(_test\\.|\\.test\\.|\\.spec\\.|(^|/)tests?/|-test\\.sh$)"))) end) as $touched_test |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/pr-risk/grade-pr-risk.sh` around lines 291 - 292, Move the test-file
matching patterns used by the $touched_test calculation into the versioned risk
map, while retaining the current regex as the fallback when the map key is
absent or empty. Add the matching key to risk-map.v0.json and update the map
validation to document and validate it, including patterns covering Python,
Java, and Ruby test naming conventions.
| labels(first:20){ nodes{ name } } | ||
| commits(last:1){ nodes{ commit{ statusCheckRollup{ state | ||
| contexts(first:100){ pageInfo{ hasNextPage } nodes{ __typename | ||
| ... on CheckRun{ name status conclusion checkSuite{ workflowRun{ workflow{ name } } } } | ||
| ... on StatusContext{ context state } } } } } } } | ||
| files(first:100){ pageInfo{ hasNextPage } nodes{ path additions deletions changeType } } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Paginate the file list instead of grading every large PR ungraded.
files(first:100) caps the changed-path list. Above 100 files the record sets changed_paths_status: "unknown", which nulls the path floor AND the reversibility axis, so the whole grade is unknown. The PRs that touch 150 files are the PRs a risk grade helps most, and right now they all land in the ungraded lane. Page the files connection with endCursor until hasNextPage is false, and keep the unknown result only for a hard page-count cap.
Related: labels(first:20) at Line 356 has no truncation twin, unlike files and contexts. A PR with more than 20 labels can silently lose agent-coded. Both default provenance tiers are R1 today, so no tier moves — but a consumer map that splits agent-supervised from human would grade off a label list nobody confirmed was complete.
Also applies to: 400-403
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/pr-risk/grade-pr-risk.sh` around lines 356 - 361, Update the PR risk
GraphQL query and the parsing logic around changed_paths_status to paginate the
files connection through endCursor until hasNextPage is false, retaining unknown
only when a hard page-count cap is reached. Aggregate all returned file nodes
before grading so large PRs receive normal path and reversibility scores. Also
paginate labels beyond the first 20 and merge all label pages before detecting
agent-coded or related provenance labels.
| { "class": "risk-map", "tier": "R3", "why": "touching the map itself is automatically R3 — a PR must not be able to lower its own grade", | ||
| "paths": ["risk-map.*.json", ".github/risk.json", ".github/risk-runbooks.json", "**/.github/risk.json"] }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Root-anchored globs in the default map. glob2re in scripts/pr-risk/grade-pr-risk.sh (Lines 166-170) anchors the whole path with ^/$, and * does not cross /. Any glob without a leading **/ therefore matches root-level files only. Two rules in this map assume otherwise, so they silently match nothing in a real tree — a floor that never touches the ground.
scripts/pr-risk/risk-map.v0.json#L24-L25: prefixrisk-map.*.jsonwith**/, and add**/runbook-registry.*.json,**/pr-risk/**, and**/.github/risk-runbooks.json. Without this,scripts/pr-risk/risk-map.v0.jsonand the grader scripts grade atdefault_tierR0, and a PR that edits the judge is graded safest by that judge.scripts/pr-risk/risk-map.v0.json#L53-L54: change*-test.shto**/*-test.sh, so shell tests outside atests/directory match the R0 tests class.
Then add a regression case to scripts/pr-risk/tests/test_grade_pr_risk.sh that feeds a nested scripts/pr-risk/risk-map.v0.json path and asserts path_floor is R3.
📍 Affects 1 file
scripts/pr-risk/risk-map.v0.json#L24-L25(this comment)scripts/pr-risk/risk-map.v0.json#L53-L54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/pr-risk/risk-map.v0.json` around lines 24 - 25, Update
scripts/pr-risk/risk-map.v0.json at lines 24-25 to use **/risk-map.*.json and
include **/runbook-registry.*.json, **/pr-risk/**, and
**/.github/risk-runbooks.json; update lines 53-54 from *-test.sh to
**/*-test.sh. Add a regression case in
scripts/pr-risk/tests/test_grade_pr_risk.sh that grades a nested
scripts/pr-risk/risk-map.v0.json path and asserts path_floor is R3.
| { "class": "tests", "tier": "R0", "why": "test-only changes cannot break production behaviour", | ||
| "paths": ["**/*_test.go", "**/*.test.ts", "**/*.test.tsx", "**/*.spec.ts", "**/test/**", "**/tests/**", "*-test.sh"] } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
*-test.sh matches root-level files only.
* does not cross /, so this glob compiles to ^[^/]*-test\.sh$. scripts/pr-risk/tests/test_apply_risk_label.sh is rescued by **/tests/**, but a shell test outside a tests/ directory is not. Prefix the glob with **/.
🐛 Proposed fix
- "paths": ["**/*_test.go", "**/*.test.ts", "**/*.test.tsx", "**/*.spec.ts", "**/test/**", "**/tests/**", "*-test.sh"] }
+ "paths": ["**/*_test.go", "**/*.test.ts", "**/*.test.tsx", "**/*.spec.ts", "**/test/**", "**/tests/**", "**/*-test.sh"] }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { "class": "tests", "tier": "R0", "why": "test-only changes cannot break production behaviour", | |
| "paths": ["**/*_test.go", "**/*.test.ts", "**/*.test.tsx", "**/*.spec.ts", "**/test/**", "**/tests/**", "*-test.sh"] } | |
| { "class": "tests", "tier": "R0", "why": "test-only changes cannot break production behaviour", | |
| "paths": ["**/*_test.go", "**/*.test.ts", "**/*.test.tsx", "**/*.spec.ts", "**/test/**", "**/tests/**", "**/*-test.sh"] } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/pr-risk/risk-map.v0.json` around lines 53 - 54, Update the "tests"
class path patterns in the risk-map configuration so the shell-test glob matches
files at any directory depth, not only root-level files. Change the existing
"*-test.sh" pattern to its recursive equivalent while preserving the other test
path patterns and classification.
| "permitted_paths": ["go.mod", "go.sum", "**/go.mod", "**/go.sum", "**/package.json", "**/package-lock.json", | ||
| "**/pnpm-lock.yaml", "**/yarn.lock", "**/requirements*.txt", "**/pyproject.toml", | ||
| "**/uv.lock", "**/Cargo.toml", "**/Cargo.lock", ".github/workflows/**"], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Align permitted_paths with the deps class in the risk map.
The deps rule in scripts/pr-risk/risk-map.v0.json (lines 39-43) lists Gemfile, Gemfile.lock, composer.json, composer.lock, *.gemspec, poetry.lock, Pipfile, Pipfile.lock, npm-shrinkwrap.json, setup.py, and go.work*. This list omits all of them. A Ruby or PHP dependabot PR therefore fails paths_ok, falls back to human, and records a shape_failure that says dependabot failed to look like dependabot. The overall tier does not move, because the path floor pins those manifests at R3 anyway — but the noise trains readers to skip the one field designed to catch real impersonation.
Two manifest lists that must agree are one list too many. Consider deriving the permitted set from the deps rule, or at minimum extend it here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/pr-risk/runbook-registry.v0.json` around lines 31 - 33, Update the
permitted_paths configuration associated with the deps rule to include every
dependency-manifest pattern listed by deps in risk-map.v0.json, including
Gemfile, Gemfile.lock, composer.json, composer.lock, *.gemspec, poetry.lock,
Pipfile, Pipfile.lock, npm-shrinkwrap.json, setup.py, and go.work*. Prefer
deriving this set from the deps rule if the registry supports it; otherwise
extend the existing list while preserving all current entries.
![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 | 5 |
| 🟡 Medium | 5 |
Panel: 7/8 reviewers contributed findings.
Reviewers that did not contribute: claude-opus-5-thinking-max:edge-case (empty)
| checks_pending_excl_self:$checks.pending, | ||
| outcome:(if .mergedAt != null then "merged" elif .state == "CLOSED" then "closed_unmerged" else "open" end), | ||
| changed_paths:(if (.files.pageInfo.hasNextPage // false) then null | ||
| else [.files.nodes[] | {path:.path, additions:.additions, deletions:.deletions, change_type:.changeType}] end), |
There was a problem hiding this comment.
🟠 High — Changed-file records keep only path and change_type, so a RENAMED file is recorded solely under its destination path and the previous path is discarded. Renaming .github/workflows/deploy.yml, an auth/ file, or a migration to an innocuous location therefore escapes both the sensitive path floor and the DELETED-under-sensitive-class rule. Add previousFilePath to the GraphQL files selection and feed it into the path-floor and deletion checks.
Raised by 2 of 8 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case).
| then "fork / author-association were not collected (\($pvst)) — the `external` provenance class is un-decidable, and defaulting it to 'not a fork' would silently retire the external => R3 rule" | ||
| else "PR author did not resolve to a GitHub account — provenance is unattributable" end), | ||
| provenance:null} | ||
| else {tier: (($M.provenance_tiers // {})[$prov] // "R1"), status:"ok", |
There was a problem hiding this comment.
🟠 High — read_map only checks that the tier VALUES in provenance_tiers are known tiers; it never requires the external key. A repo map that omits it passes validation and then ($M.provenance_tiers[$prov] // "R1") grades fork and first-time-contributor PRs R1, silently retiring the "external is R3, no exceptions" invariant the map comment and README both promise. Require external (and ideally all four classes) in the structural validation, or fall back to R3 rather than R1 for a missing key.
Raised by 3 of 8 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, kimi-k2.7-code adversarial).
| then {state: null, pending: false, status: "unknown"} | ||
| else | ||
| ([$ro.contexts.nodes[] | ||
| | select(((.__typename == "CheckRun") and ((.checkSuite.workflowRun.workflow.name // "") == $self)) | not)]) as $ctx |
There was a problem hiding this comment.
🟠 High — Self-exclusion drops every CheckRun whose checkSuite.workflowRun.workflow.name equals $self, not just this job's own check run. A consumer that puts the pr-risk job in an existing CI workflow loses all its sibling jobs from the rollup, so a FAILED test job is excluded and the remaining green contexts aggregate to SUCCESS — reversibility then grades R0/R1 on a red PR. Match on the run/job ID (e.g. GITHUB_RUN_ID via checkSuite.workflowRun.databaseId) instead of the display name.
Raised by 3 of 8 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case).
| -H "Accept: application/vnd.github.raw" > repo-risk-map.json 2>/dev/null; then | ||
| echo "map=repo-risk-map.json" >> "$GITHUB_OUTPUT" | ||
| echo "using ${MAP_PATH} from ${BASE_REF}" | ||
| else |
There was a problem hiding this comment.
🟠 High — The override fetch treats ANY non-zero gh api exit as "file absent" (stderr is discarded by 2>/dev/null), so a 403 rate-limit, 5xx, or network blip silently grades the PR against the generic default map instead of the repo's sharpened one — a lower tier computed from an input nobody read, which is what the unknown contract forbids everywhere else. Distinguish a genuine 404 (e.g. capture the status via gh api --include/-i or check the error text) and fail loudly on anything else.
Raised by 4 of 8 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case, kimi-k2.7-code adversarial).
| else | ||
| ([$A1.classes[]? | select(. as $c | ($RV.irreversible_classes // []) | index($c))]) as $irrev | ||
| | ([$paths[] | select(.change_type == "DELETED") | .path]) as $deleted | ||
| | (($A1.classes // []) | any(. as $c | ($RV.delete_sensitive_classes // []) | index($c))) as $del_sensitive |
There was a problem hiding this comment.
🟠 High — del_sensitive is computed from $A1.classes — the classes matched by ANY changed file — and then combined with "the PR deletes at least one file". A PR that modifies an auth file while deleting an unrelated README therefore reports "deletes N file(s) under a sensitive class" and pins reversibility at R3. Compute the sensitive-class match over the DELETED paths only.
Raised by 3 of 8 reviewers (gemini-3.1-pro adversarial, gemini-3.1-pro edge-case, gpt-5.6-sol-max edge-case).
| elif any($ctx[]; (.__typename == "CheckRun" and ((.status != "COMPLETED") or ((.conclusion // "") == "STALE"))) | ||
| or (.__typename == "StatusContext" and (.state | IN("PENDING","EXPECTED")))) | ||
| then "PENDING" | ||
| elif ($ctx | length) > 0 then "SUCCESS" |
There was a problem hiding this comment.
🟡 Medium — Any nonempty set of completed, non-failing contexts aggregates to SUCCESS, including a rollup where every CheckRun concluded SKIPPED, NEUTRAL, or null. Those establish nothing about whether tests ran, yet they let reversibility drop to R0/R1 — the axis's own stated question is "did tests covering these lines actually run". Require at least one SUCCESS conclusion before aggregating to SUCCESS.
Raised by 1 of 8 reviewers (gpt-5.6-sol-max edge-case).
| *) echo "grader failed (rc=$rc)"; exit 1 ;; # setup/usage bug: fail loud | ||
| esac | ||
| pending="$(jq -r '.checks_pending_excl_self // false' record.json)" | ||
| if [ "$pending" = "true" ] && [ "$(date +%s)" -lt "$deadline" ]; then |
There was a problem hiding this comment.
🟡 Medium — The poll exits as soon as no CURRENTLY VISIBLE context is pending, so a rollup that is empty or transiently all-green (checks that have not registered yet) ends the wait and applies a low-risk label that a later failure never revises. Consider requiring the rollup to be non-empty and stable across two consecutive polls, or documenting that late-registering checks are not reflected.
Raised by 2 of 8 reviewers (gpt-5.6-sol-max adversarial, gpt-5.6-sol-max edge-case).
| for l in "${OWNED[@]}"; do | ||
| [ "$l" = "$TARGET" ] && continue | ||
| if has "$l"; then | ||
| gh api -X DELETE "repos/$REPO/issues/$PR_NUMBER/labels/$l" >/dev/null 2>&1 \ |
There was a problem hiding this comment.
🟡 Medium — Label names are interpolated raw into REST paths (.../labels/$l here and the GET at line 105). A remapped label containing a space, /, #, ?, or % — all legal in GitHub labels — produces a malformed or misrouted request; the DELETE failure is then treated as a write failure and hard-fails the run with exit 4. Percent-encode the segment (jq -Rr @uri) before building the path.
Raised by 5 of 8 reviewers (claude-opus-5-thinking-max adversarial, gemini-3.1-pro adversarial, gemini-3.1-pro edge-case, gpt-5.6-sol-max adversarial+edge-case, kimi-k2.7-code adversarial+edge-case).
| fi | ||
|
|
||
| # Current labels on the PR (a PR is an issue to the labels API). | ||
| current="$(gh api "repos/$REPO/issues/$PR_NUMBER/labels" --jq '[.[].name]' 2>/dev/null)" \ |
There was a problem hiding this comment.
🟡 Medium — The current-label read has no --paginate, so only the first 30 labels are seen. On a PR carrying more than 30 labels a stale grader-owned label falls off the page, is never removed, and the PR displays two contradictory risk tiers — breaking the "exactly one grader label" ownership contract this script is built around.
Raised by 5 of 8 reviewers (claude-opus-5-thinking-max adversarial, gemini-3.1-pro adversarial, gemini-3.1-pro edge-case, gpt-5.6-sol-max adversarial+edge-case).
| number title state isDraft createdAt updatedAt closedAt mergedAt | ||
| author{ login } authorAssociation baseRefName headRefName isCrossRepository | ||
| additions deletions changedFiles | ||
| labels(first:20){ nodes{ name } } |
There was a problem hiding this comment.
🟡 Medium — labels(first:20) has no pageInfo { hasNextPage } guard, unlike files(first:100) and contexts(first:100) which both degrade to unknown on truncation. A PR with more than 20 labels yields an incomplete labels array and a wrong agent_coded flag, presenting a truncated read as a confident provenance grade.
Raised by 5 of 8 reviewers (claude-opus-5-thinking-max adversarial, gemini-3.1-pro adversarial+edge-case, gpt-5.6-sol-max adversarial+edge-case, kimi-k2.7-code adversarial+edge-case).
3 participants
Summary
New reusable workflow
pr-risk.yml: grades every PR event into an advisory risk tier R0 (safest) .. R3 (riskiest) and syncs exactly one label —risk:R0..risk:R3, orrisk:ungradedwhen an input could not be read. The label is the entire product: nothing is gated, routed, commented on, or merged. Humans glance at the label and agree or disagree; disagreement is recorded with a human-ownedrisk-disputelabel the grader never touches.grade = worst(path_floor, provenance, reversibility)— three deterministic axes, worst wins, so no axis can move a PR into a safer lane than another axis put it. The whole path isgh+jq; no model call anywhere.Details
scripts/pr-risk/grade-pr-risk.sh— the grader. Three axes: a versioned path-glob map (worst tier over every matched rule, so a docs file can never cancel a migration), provenance (registered runbook producers must assert identity AND diff shape; fork / first-time contributors are R3 with no exceptions), and reversibility (persistent-state mutation, deletions under sensitive classes, whether green checks covered the lines). Unreadable input gradesunknown, never a confident tier.scripts/pr-risk/apply-risk-label.sh— the one write. Owns exactly the five mapped labels: removes stale ones, applies the computed one, touches nothing else. Missing labels are created color-coded on first use. Label text is remappable vialabel_map..github/risk.json/.github/risk-runbooks.jsonin the consumer repo are read from the PR's base ref, so a PR cannot edit the rules that judge it (and touching them at all grades R3 by the map's first rule). Absent falls back to the shipped defaults; present-but-invalid fails loudly.wait_for_checks_minutes(default 10) for the rest of CI to settle before labeling.GITHUB_TOKEN, which cannot firelabeledtriggers — the shadow check is structurally unable to start a workflow cascade. No secrets required.workflows_ref.test-pr-risk.yml: shellcheck + two hermetic suites (35 checks — synthetic records plus a stubbedghfor the live-PR path, including the self-excluding-rollup cases). README catalog row + feature README (scripts/pr-risk/README.md) included.ELI5
Some pull requests are scary (they touch billing, or database migrations, or the CI itself) and some are boring (a typo fix in a README). Today a human has to open each one to tell the difference. This workflow reads the list of changed files, who made the PR, and whether the tests passed, and puts a colored sticker on it: green for boring, red for scary, gray for "couldn't tell".
That sticker doesn't DO anything — it can't merge, block, or approve. It exists so the team can spend a couple of weeks checking whether the stickers match their own judgement. If someone thinks a sticker is wrong, they slap on a
risk-disputesticker and say why. Only after the stickers prove themselves does anyone talk about acting on them.Two design points that matter: the rules that decide the grade can't be changed by the PR being graded (they're read from the target branch, not the PR), and the checker leaves out its own still-running check when it asks "did the tests pass?" — otherwise it would always answer "not yet" about itself.