[PR #226] [MERGED] docs(pr-risk): drop the dependabot skip from the documented caller pattern #233

New issue

Closed

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

bsakaguchi commented

2026-10-26 22:37:37 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-8035-pr-risk-docs-dependabot-skip

📝 Commits (2)

f22154a docs(pr-risk): drop the dependabot skip from the documented caller pattern
c46e0b9 docs(pr-risk): sharpen the dependabot-skip rationale after review

📊 Changes

3 files changed (+121 additions, -28 deletions)

View changed files

📝 .github/workflows/pr-risk.yml (+56 -20)
📝 docs/callers/pr-risk.md (+43 -0)
📝 scripts/pr-risk/README.md (+22 -8)

📄 Description

ELI-5

The pr-risk docs told every consumer to copy an if: line that skips Dependabot PRs. That skip was there because Dependabot's runs get a read-only token and the label write would fail — except GitHub's permissions: key elevates that token, and pr-risk uses no secrets, so the write actually succeeds. The skip bought nothing and left each repo's highest-volume automated PR producer ungraded. This drops it from the documented pattern and explains why the fork half of the same line is still real.

What changed

Docs only. No behaviour change to the reusable — its own if: logic, its inputs, and its fork guidance are untouched (the diff contains no non-comment YAML line).

.github/workflows/pr-risk.yml — the caller pattern's example if: drops the github.actor != 'dependabot[bot]' clause and keeps the fork clause; the comment above it is rewritten into two paragraphs: the fork clause is still a token guard (the permissions: elevation does not extend to fork-PR tokens, so a fork PR is ungraded-by-absence on plain pull_request, and is gradable by dispatching on pr_number or by enrolling with pull_request_target, which this header already endorses), and there is deliberately no dependabot clause (read-only by default, elevated by the caller's permissions: block, no secrets declared). The second paragraph states explicitly that this reasoning does not transfer to a caller that needs an Actions secret — cursor-review-auto-label.yml's app private key is the named local example — because no permissions: block hands a Dependabot run an Actions secret.
.github/workflows/pr-risk.yml, ON-DEMAND GRADING bullet — the "BOT-AUTHORED PRs ARE GRADED" bullet described the by-number path as the place bots get graded, keyed on the caller clause that is now gone. It is now "FORK PRs ARE GRADED" (the one thing that really is by-number-only) and says bots are graded on both paths, so the header and the pattern agree.
docs/callers/pr-risk.md — a gotcha stating the rule positively, plus the consumer-coordination note: existing callers drop the clause in their own repos, and bump-pr-risk-callers.yml moves SHA pins only and will never edit an if:, so a caller that mirrors its if:/concurrency shape in a repo-side unit test has to update that test in the same PR.
scripts/pr-risk/README.md — the same correction where the skip was explained.

Both docs cite GitHub's Troubleshooting Dependabot on GitHub Actions → "Changing GITHUB_TOKEN permissions".

Review round (`c46e0b9`)

Four of the panel's five findings turned into edits; all three files are still comment/prose only.

"Dependabot-triggered runs", not "bot-authored PRs". github.actor is who triggered the run, not who authored the PR, so the removed clause never keyed on bot authorship — a human pushing to a Dependabot branch made the actor human and the PR graded even with the clause in place. What it did select is exactly the read-only-token set the permissions: block covers. Corrected in pr-risk.yml and scripts/pr-risk/README.md, with the mechanism spelled out rather than just the label swapped.
"Buys nothing" was overclaiming, so both docs now name what it did buy. A Dependabot github-actions PR bumping this caller's own uses: pin runs the newly pinned reusable inside that PR's own run under the elevated token, where the clause used to skip it. Bounded honestly (Dependabot only proposes SHAs behind this repo's published tags; the tool checkout is separately gated on workflows_ref being an ancestor of main; the fleet's own bump PRs always had this property). The adjacent gotcha that falls out of it is now stated too: Dependabot rewrites uses: only and leaves workflows_ref stale, and the uses:/workflows_ref equality check that would catch that is a deliberate open gap — fix the input in the same PR.
docs/callers/pr-risk.md and the header now agree on caller shape. The page's copy-paste block carries no if: while the header pattern does, and the new prose pointed at a clause the page never showed. Fixed by scoping rather than by editing the block: a new paragraph under the fork gotcha says why it has no if: (under pull_request_target the token is writable for fork PRs too; a same-repo-only repo has no fork PRs to guard), names the third shape that does need the clause — public repo staying on plain pull_request, forks ungraded rather than red — and points at the header for the event-scoped variant.
Dropped the "and the actor is a human" residue from the fork-clause bullets in both pr-risk.yml and scripts/pr-risk/README.md. Only the writable-token half explains why the fork clause does not apply on a dispatch, and the actor half contradicted the next sentence.
Deferred, not fixed: the observation that this comment-only edit to pr-risk.yml still fans a SHA-bump PR across the fleet. Real — the fleet already excludes scripts/pr-risk/tests/** and scripts/pr-risk/README.md for exactly this reason, and a comment-only workflow-file edit is the same class but is not expressible as a path filter. The fix belongs in the shared bump-callers.sh (safely: a # line inside a run: heredoc is not a comment) or in a commit trailer, which is a design call; tracked as a follow-up. The related half — a maintainer reading a bumped caller as current while it still carries the skip — is handled here, by the docs/callers/pr-risk.md note that the fleet moves pins only and will never edit your if:.

Judgment calls

docs/callers/pr-risk.md showed neither the caller if: nor the dependabot skip, so the "same correction" asked for there was vacuous as written. Rather than skip it, I added the rule as a gotcha, so a consumer who reads only that page learns not to add the clause. I deliberately did not add an if: to that page's caller example — it has never carried one, and adding one would be a scope expansion beyond a docs correction.
scripts/pr-risk/README.md was not named in the scope, but it did explain the skip in the wording being corrected. Leaving it would have left the repo contradicting itself, so it is fixed here. It is excluded from the bump fleet's path filter, so it fans no PRs.
This PR will fan a SHA-bump PR across the pr-risk fleet. bump-pr-risk-callers.yml watches .github/workflows/pr-risk.yml with no docs carve-out (only scripts/pr-risk/tests/** and scripts/pr-risk/README.md are excluded), so a header-comment-only change still bumps. That is the existing design and I did not widen the exclusion; it is arguably useful here, since the bump PR is the moment a consumer's reviewer sees the corrected header and can drop their own clause.
No consumer repos are named anywhere in the diff or this body, per this repo's public-repo rule; the coordination note describes the coupling generically ("a unit test asserting on the caller's YAML").

Verification of the claim itself

The premise is load-bearing, so I checked it against the vendor docs rather than reasoning from the old comment. Troubleshooting Dependabot on GitHub Actions states, under "Changing GITHUB_TOKEN permissions": "By default, GitHub Actions workflows triggered by Dependabot get a GITHUB_TOKEN with read-only permissions. You can use the permissions key in your workflow to increase the access for the token". The same page, under "Accessing secrets", states that a Dependabot-triggered workflow sees Dependabot secrets only and "GitHub Actions secrets are not available" — which is precisely the boundary of the non-transfer note above. pr-risk.yml declares no secrets: and callers pass none, so it sits on the safe side of that boundary.

A useful cross-check inside this repo: pr-area-label.yml keeps its own dependabot skip, and its comment gives the reason this change does not apply to it — that job needs the ANTHROPIC secret. So the two workflows now disagree for the right reason rather than by copy-paste.

Residual

The four enrolled consumer repos still carry the clause, and nothing here removes it. This PR corrects the source of the copy; each consumer has to edit its own caller, together with whatever repo-side test asserts on that caller's if:/concurrency shape. The bump fleet moves pins only. Until those edits land, those repos' Dependabot PRs stay ungraded — i.e. the outcome this change exists to fix is not yet realized anywhere.
I did not exercise a live Dependabot run against a pr-risk caller. This is a docs change in a repo with no pr-risk caller of its own, so there is nothing here to run the claim against; the evidence is the vendor documentation quoted above plus the live verification recorded on the originating investigation, which I could not read from this environment. If a consumer drops the clause and the label write still 403s on a Dependabot PR, this PR's premise is what to re-check first.
Unexercised artifacts. The originating investigation's findings comment (which carries the live verification), and the consumer repo where that verification was performed, were named to me but not reachable from this environment; I could neither read the comment nor observe the verified run. Nothing in this diff depends on detail from either beyond the vendor-documented behaviour I confirmed independently.
The by-number path's third consequence is now slightly over-claimed elsewhere. The reusable's pr_number input description still says "Bot-authored and fork PRs ARE graded on this path (see the header)". That is still true, but bot PRs are no longer distinctive of that path. I left it alone as a one-word-accurate sentence rather than widen the diff; a follow-up could tighten it to "Fork PRs".

Provenance

Authored by: agent-work loop
Verified: bash scripts/pr-risk/tests/test_pin_contract.sh 28 passed / 0 failed (the suite that pins the workflows_ref guard body verbatim — the named landmine); test_grade_pr_risk.sh 138/0, test_grade_targets.sh 105/0, test_apply_risk_label.sh 48/0, test_publish_risk_surfaces.sh 94/0, test_resolve_enabled.sh 27/0; shellcheck -x scripts/pr-risk/*.sh clean; python3 .github/workflow-pins/check_workflow_pins.py --workflows-dir .github/workflows OK (11 workflows declare workflows_ref, none with a default, 0 exempt); yaml.safe_load parses the edited workflow after the review round. All re-run on c46e0b9. python3 .github/agents-md-integrity/check_agents_md.py --root . fails on AGENTS.md being 305 lines against a 200-line ceiling — pre-existing on main at the merge base, untouched by this diff. Not exercised: a live Dependabot run against a pr-risk caller (see Residual).
Deviations: none against the requested scope; the two extensions to it (scripts/pr-risk/README.md, and a positive gotcha instead of a vacuous correction in docs/callers/pr-risk.md) are argued under Judgment calls above. The review round in c46e0b9 stayed inside that same scope — prose and comments in the same three files — and deferred the one finding whose fix lives in the bump fleet.

_{🔄 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/226 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/25/2026 **Status:** ✅ Merged **Merged:** 8/25/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8035-pr-risk-docs-dependabot-skip` --- ### 📝 Commits (2) - [`f22154a`](https://github.com/Comfy-Org/github-workflows/commit/f22154a7f09606d73720905457b429577b96996b) docs(pr-risk): drop the dependabot skip from the documented caller pattern - [`c46e0b9`](https://github.com/Comfy-Org/github-workflows/commit/c46e0b9e2f895907af3ba1d9cf24b3498f1dd67d) docs(pr-risk): sharpen the dependabot-skip rationale after review ### 📊 Changes **3 files changed** (+121 additions, -28 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/pr-risk.yml` (+56 -20) 📝 `docs/callers/pr-risk.md` (+43 -0) 📝 `scripts/pr-risk/README.md` (+22 -8) </details> ### 📄 Description ## ELI-5 The pr-risk docs told every consumer to copy an `if:` line that skips Dependabot PRs. That skip was there because Dependabot's runs get a read-only token and the label write would fail — except GitHub's `permissions:` key elevates that token, and pr-risk uses no secrets, so the write actually succeeds. The skip bought nothing and left each repo's highest-volume automated PR producer ungraded. This drops it from the documented pattern and explains why the *fork* half of the same line is still real. ## What changed Docs only. No behaviour change to the reusable — its own `if:` logic, its inputs, and its fork guidance are untouched (the diff contains no non-comment YAML line). - **`.github/workflows/pr-risk.yml`** — the caller pattern's example `if:` drops the `github.actor != 'dependabot[bot]'` clause and keeps the fork clause; the comment above it is rewritten into two paragraphs: the fork clause is still a token guard (the `permissions:` elevation does **not** extend to fork-PR tokens, so a fork PR is ungraded-by-absence on plain `pull_request`, and is gradable by dispatching on `pr_number` or by enrolling with `pull_request_target`, which this header already endorses), and there is deliberately no dependabot clause (read-only by default, elevated by the caller's `permissions:` block, no secrets declared). The second paragraph states explicitly that this reasoning does **not** transfer to a caller that needs an Actions secret — `cursor-review-auto-label.yml`'s app private key is the named local example — because no `permissions:` block hands a Dependabot run an Actions secret. - **`.github/workflows/pr-risk.yml`, ON-DEMAND GRADING bullet** — the "BOT-AUTHORED PRs ARE GRADED" bullet described the by-number path as the place bots get graded, keyed on the caller clause that is now gone. It is now "FORK PRs ARE GRADED" (the one thing that really is by-number-only) and says bots are graded on **both** paths, so the header and the pattern agree. - **`docs/callers/pr-risk.md`** — a gotcha stating the rule positively, plus the consumer-coordination note: existing callers drop the clause in their own repos, and `bump-pr-risk-callers.yml` moves SHA pins only and will never edit an `if:`, so a caller that mirrors its `if:`/`concurrency` shape in a repo-side unit test has to update that test in the same PR. - **`scripts/pr-risk/README.md`** — the same correction where the skip was explained. Both docs cite GitHub's *Troubleshooting Dependabot on GitHub Actions* → "Changing `GITHUB_TOKEN` permissions". ### Review round (`c46e0b9`) Four of the panel's five findings turned into edits; all three files are still comment/prose only. - **"Dependabot-triggered runs", not "bot-authored PRs".** `github.actor` is who *triggered* the run, not who authored the PR, so the removed clause never keyed on bot authorship — a human pushing to a Dependabot branch made the actor human and the PR graded even with the clause in place. What it did select is exactly the read-only-token set the `permissions:` block covers. Corrected in `pr-risk.yml` and `scripts/pr-risk/README.md`, with the mechanism spelled out rather than just the label swapped. - **"Buys nothing" was overclaiming, so both docs now name what it did buy.** A Dependabot github-actions PR bumping *this* caller's own `uses:` pin runs the newly pinned reusable inside that PR's own run under the elevated token, where the clause used to skip it. Bounded honestly (Dependabot only proposes SHAs behind this repo's published tags; the tool checkout is separately gated on `workflows_ref` being an ancestor of `main`; the fleet's own bump PRs always had this property). The adjacent gotcha that falls out of it is now stated too: Dependabot rewrites `uses:` only and leaves `workflows_ref` stale, and the `uses:`/`workflows_ref` equality check that would catch that is a deliberate open gap — fix the input in the same PR. - **`docs/callers/pr-risk.md` and the header now agree on caller shape.** The page's copy-paste block carries no `if:` while the header pattern does, and the new prose pointed at a clause the page never showed. Fixed by scoping rather than by editing the block: a new paragraph under the fork gotcha says *why* it has no `if:` (under `pull_request_target` the token is writable for fork PRs too; a same-repo-only repo has no fork PRs to guard), names the third shape that does need the clause — public repo staying on plain `pull_request`, forks ungraded rather than red — and points at the header for the event-scoped variant. - **Dropped the "and the actor is a human" residue** from the fork-clause bullets in both `pr-risk.yml` and `scripts/pr-risk/README.md`. Only the writable-token half explains why the fork clause does not apply on a dispatch, and the actor half contradicted the next sentence. - **Deferred, not fixed:** the observation that this comment-only edit to `pr-risk.yml` still fans a SHA-bump PR across the fleet. Real — the fleet already excludes `scripts/pr-risk/tests/**` and `scripts/pr-risk/README.md` for exactly this reason, and a comment-only workflow-file edit is the same class but is not expressible as a path filter. The fix belongs in the shared `bump-callers.sh` (safely: a `#` line inside a `run:` heredoc is not a comment) or in a commit trailer, which is a design call; tracked as a follow-up. The related half — a maintainer reading a bumped caller as current while it still carries the skip — *is* handled here, by the `docs/callers/pr-risk.md` note that the fleet moves pins only and will never edit your `if:`. ## Judgment calls - **`docs/callers/pr-risk.md` showed neither the caller `if:` nor the dependabot skip**, so the "same correction" asked for there was vacuous as written. Rather than skip it, I added the rule as a gotcha, so a consumer who reads only that page learns not to add the clause. I deliberately did **not** add an `if:` to that page's caller example — it has never carried one, and adding one would be a scope expansion beyond a docs correction. - **`scripts/pr-risk/README.md` was not named in the scope, but it *did* explain the skip** in the wording being corrected. Leaving it would have left the repo contradicting itself, so it is fixed here. It is excluded from the bump fleet's path filter, so it fans no PRs. - **This PR will fan a SHA-bump PR across the pr-risk fleet.** `bump-pr-risk-callers.yml` watches `.github/workflows/pr-risk.yml` with no docs carve-out (only `scripts/pr-risk/tests/**` and `scripts/pr-risk/README.md` are excluded), so a header-comment-only change still bumps. That is the existing design and I did not widen the exclusion; it is arguably useful here, since the bump PR is the moment a consumer's reviewer sees the corrected header and can drop their own clause. - **No consumer repos are named** anywhere in the diff or this body, per this repo's public-repo rule; the coordination note describes the coupling generically ("a unit test asserting on the caller's YAML"). ## Verification of the claim itself The premise is load-bearing, so I checked it against the vendor docs rather than reasoning from the old comment. *Troubleshooting Dependabot on GitHub Actions* states, under "Changing `GITHUB_TOKEN` permissions": "By default, GitHub Actions workflows triggered by Dependabot get a `GITHUB_TOKEN` with read-only permissions. You can use the `permissions` key in your workflow to increase the access for the token". The same page, under "Accessing secrets", states that a Dependabot-triggered workflow sees Dependabot secrets only and "GitHub Actions secrets are not available" — which is precisely the boundary of the non-transfer note above. `pr-risk.yml` declares no `secrets:` and callers pass none, so it sits on the safe side of that boundary. A useful cross-check inside this repo: `pr-area-label.yml` keeps its own dependabot skip, and its comment gives the reason this change does not apply to it — that job needs the `ANTHROPIC` secret. So the two workflows now disagree for the right reason rather than by copy-paste. ## Residual - **The four enrolled consumer repos still carry the clause, and nothing here removes it.** This PR corrects the source of the copy; each consumer has to edit its own caller, together with whatever repo-side test asserts on that caller's `if:`/`concurrency` shape. The bump fleet moves pins only. Until those edits land, those repos' Dependabot PRs stay ungraded — i.e. the outcome this change exists to fix is not yet realized anywhere. - **I did not exercise a live Dependabot run against a pr-risk caller.** This is a docs change in a repo with no pr-risk caller of its own, so there is nothing here to run the claim against; the evidence is the vendor documentation quoted above plus the live verification recorded on the originating investigation, which I could not read from this environment. If a consumer drops the clause and the label write still 403s on a Dependabot PR, this PR's premise is what to re-check first. - **Unexercised artifacts.** The originating investigation's findings comment (which carries the live verification), and the consumer repo where that verification was performed, were named to me but not reachable from this environment; I could neither read the comment nor observe the verified run. Nothing in this diff depends on detail from either beyond the vendor-documented behaviour I confirmed independently. - **The by-number path's third consequence is now slightly over-claimed elsewhere.** The reusable's `pr_number` input description still says "Bot-authored and fork PRs ARE graded on this path (see the header)". That is still true, but bot PRs are no longer *distinctive* of that path. I left it alone as a one-word-accurate sentence rather than widen the diff; a follow-up could tighten it to "Fork PRs". ## Provenance - **Authored by:** agent-work loop - **Verified:** `bash scripts/pr-risk/tests/test_pin_contract.sh` 28 passed / 0 failed (the suite that pins the `workflows_ref` guard body verbatim — the named landmine); `test_grade_pr_risk.sh` 138/0, `test_grade_targets.sh` 105/0, `test_apply_risk_label.sh` 48/0, `test_publish_risk_surfaces.sh` 94/0, `test_resolve_enabled.sh` 27/0; `shellcheck -x scripts/pr-risk/*.sh` clean; `python3 .github/workflow-pins/check_workflow_pins.py --workflows-dir .github/workflows` OK (11 workflows declare `workflows_ref`, none with a default, 0 exempt); `yaml.safe_load` parses the edited workflow after the review round. All re-run on `c46e0b9`. `python3 .github/agents-md-integrity/check_agents_md.py --root .` fails on `AGENTS.md` being 305 lines against a 200-line ceiling — pre-existing on `main` at the merge base, untouched by this diff. Not exercised: a live Dependabot run against a pr-risk caller (see **Residual**). - **Deviations:** none against the requested scope; the two extensions to it (`scripts/pr-risk/README.md`, and a positive gotcha instead of a vacuous correction in `docs/callers/pr-risk.md`) are argued under **Judgment calls** above. The review round in `c46e0b9` stayed inside that same scope — prose and comments in the same three files — and deferred the one finding whose fix lives in the bump fleet. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:37 +00:00