[PR #118] [MERGED] fix(pr-risk): enforce workflows_ref as a full 40-hex commit SHA before the tool checkout (BE-6307) #127

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-6307-pr-risk-workflows-ref-pin

📝 Commits (8)

8024f69 fix(pr-risk): enforce workflows_ref as a full 40-hex commit SHA before the tool checkout (BE-6307)
48eb512 fix(pr-risk): tie workflows_ref to the SHA uses: resolved to, not just to SHA shape (BE-6307)
dabdfe1 fix(pr-risk): fail closed when job_workflow_sha is unreadable, and assert the guard by property (BE-6307)
b68a2cb fix(pr-risk): close the guard's neuter paths, and make the suite assert positions not sums (BE-6307)
f314614 fix(pr-risk): correct the pin contract's claims, sanitize the runner SHA, and close the suite's bypass holes (BE-6307)
c77b30b fix(pr-risk): scope the alias scan to the guard step, judge emits per statement, cover the runner SHA (BE-6307)
4efc5e7 test(pr-risk): use ERE for the emit-scan coverage grep (BE-6307)
936e275 fix(pr-risk): drop the lock-step axis — github.job_workflow_sha does not exist (BE-6307)

📊 Changes

4 files changed (+477 additions, -3 deletions)

View changed files

📝 .github/workflows/pr-risk.yml (+171 -1)
📝 .github/workflows/test-pr-risk.yml (+9 -1)
📝 README.md (+1 -1)
➕ scripts/pr-risk/tests/test_pin_contract.sh (+296 -0)

📄 Description

Correction after review (c2f849b) — read this first

A middle revision of this PR added a second "lock-step" axis comparing workflows_ref against ${{ github.job_workflow_sha }}. That is not a property of the github context — it exists only as an OIDC token claim. It expanded to the empty string, and the fail-closed -z branch added beside it would have aborted every invocation, correctly pinned callers included, before the tool checkout. Two of eight review models caught it; actionlint rejects the expression by name (property "job_workflow_sha" is not defined in object type {…}) and GitHub's context reference lists only workflow_ref / workflow_sha, which name the caller's top-level workflow file, not the reusable one.

The axis is removed, not repaired: reading the claim would need id-token: write from every caller plus a token exchange in a job that holds permissions: {} — a caller-contract change, not a fix. It is filed as a follow-up for a human to decide on, with the failed attempt written down so nobody re-adds the context expression.

So what this PR enforces is shape only — the ticket's original scope. The header, the input description and the README now say that explicitly and name review of the caller as the only thing that can bound which commit the ref points at. This repo has no actionlint job at all, which is why the bad expression survived four review rounds; that is a separate follow-up.

ELI-5

pr-risk.yml downloads its own grading code from this repo at whatever ref the caller names in workflows_ref, and then runs that code holding a token that can write to pull requests. The rule "point that at the exact commit you already pinned uses: to" was written down in a comment and nowhere else — so a caller could point it at main, at a tag, or at refs/pull/N/head (which on a public repo resolves to code from somebody's fork) and the run would happily execute it. This PR makes the workflow check: anything that is not a full 40-character commit SHA fails the run before the download happens.

What changed

.github/workflows/pr-risk.yml — a new Enforce workflows_ref pin contract step, first in both jobs that check out workflows_ref (gate and grade), rejecting anything that is not ^[0-9a-f]{40}$. The input's description, the header's pinned-ref paragraph, and the header's caller-pattern block now say the format is enforced rather than requested. required: true and the absence of a default are unchanged.
README.md — the pr-risk.yml row now says the format is ENFORCED, not just that the input is required.

The check lives in the workflow, not in grade-pr-risk.sh / apply-risk-label.sh, because those scripts are what the ref loads — a script-side check would sit inside the blast radius it is meant to bound.

Judgment calls (deviations from the ticket — please sanity-check these)

1. The guard is in gate as well as grade. The ticket specified the grade job only; since it was written, #115 added a gate job that checks out the same ref earlier in the run. Guarding only grade would have left the run's first execution of caller-named code unguarded. Both copies are byte-identical by design (asserted locally by parsing the YAML), and each job asserts the contract itself rather than inheriting it — grade skips when gate fails today, but a job should not depend on another job's step for its own trust boundary, and that invariant survives a later if: or graph re-ordering. gate holds permissions: {}, so the token stakes are entirely in grade; the earliness is the value there.

2. A length test + character-class glob, instead of the ticket's grep -Eq (and instead of [[ =~ ]]). grep anchors ^…$ per line, so a multi-line value carrying one SHA-shaped line passes it and is handed to checkout in full. [[ =~ ]] anchors the whole string, but whether ERE $ may also match just before a trailing newline is libc-dependent — not something a trust boundary should rest on. if [[ ${#WORKFLOWS_REF} -ne 40 || "$WORKFLOWS_REF" == *[!0-9a-f]* ]] has no anchors at all: a newline, a space or an uppercase letter is simply a character outside the class. Everything else about the step — the env: passthrough (no inline ${{ }} of the input being validated), the message, the exit code — is as specified.

3. Uppercase SHAs are rejected. The ticket's regex is lowercase-only and this keeps it. git rev-parse, gh, and the GitHub UI all emit lowercase, every pin in this repo is lowercase, and a false reject here is loud and fixed by lowercasing. It matters concretely: the value is handed to actions/checkout verbatim, so accepting a spelling the fetch may not resolve would trade this step's clear message for an obscure failure one step later. (A middle revision widened this to [0-9a-fA-F]; review caught that the un-normalized value still reached checkout. Narrowed back deliberately.)

Caller-breakage check — the ticket's premise was stale, so I re-ran it

BE-6304 concluded "zero callers pass workflows_ref today." That is no longer true — two callers now exist. I verified each one's actual pin rather than relying on the absence claim:

Caller	`workflows_ref`	Verdict
`Comfy-Org/cloud` `.github/workflows/ci-pr-risk.yml`	`e4a8f7cd4a073da082b03950136530d4df50738f`	full 40-hex — passes
`Comfy-Org/comfy-cloud-mcp-server` `.github/workflows/ci-pr-risk.yml`	`99288dfbf12c084f629746739e160cba882c80f3`	full 40-hex — passes

Both were run through the shipped guard body verbatim and pass. Also re-confirmed: this repo's test-pr-risk.yml never invokes the reusable (it is hermetic over scripts/pr-risk/), a global GitHub code search surfaces no callers outside the org, and the v1 tag (4d9cb6b) contains no pr-risk file at all, so no @v1 caller can exist. The conclusion holds — this breaks nobody — but it is now backed by evidence about the callers that exist, not by their absence.

One behavior change worth naming: gate runs regardless of enabled, so a repo that enrolls with enabled: false and a mutable pin now fails its check instead of silently no-opping. That is the intended shape — it surfaces a bad pin at enrollment, which is the cheapest moment to fix it — and neither live caller is in that state.

Verification

actionlint .github/workflows/pr-risk.yml — clean (shellcheck installed, so the run: bodies were linted too). Repo-wide actionlint is also clean. Re-run at every revision now — this is the check that would have caught the job_workflow_sha expression immediately.
Guard body extracted from the shipped YAML (not retyped) and exercised: main, v1, refs/pull/7/head, empty, main\n<sha>, <sha>\n::add-mask::x, <sha> (trailing space), a 39-char SHA, a 41-char SHA, and an uppercase SHA all fail with one bounded ::error::; both live callers' real SHAs pass.
test_pin_contract.sh now pins the guard's executable body verbatim rather than characterizing it — four of this round's findings were holes in the old emit-scan whitelist (prefix-clears-a-whole-statement, blacklist trigger, total-count fatality, presence-vs-exclusivity), and an equality closes all of them at once. Verified red against eleven mutations: permissive wrapper branch, &&-chained raw emit, printenv, set -euxo pipefail, ::warning:: instead of ::error::, dropped exit 1, loosened character class, dropped length test, raw value in the annotation, if: false on the step, and an unguarded checkout in a new job — and green against a pure rewording of the message.
Parsed the YAML to assert the invariants: the guard precedes the Load pr-risk tool checkout in every job that has one, the two copies are byte-identical, and workflows_ref is still required: true, type: string, with no default.
scripts/pr-risk/ suites (the path-filtered CI job this diff triggers): shellcheck -x clean; 24 + 57 + 91 + 21 = 193 tests, 0 failures.
check_agents_md.py --root . passes (1 pre-existing CODEOWNERS warning, untouched by this diff).

No test changes: this repo has no harness for workflow YAML, and test-pr-risk.yml covers the scripts, which this diff does not touch.

_{🔄 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/118 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/4/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6307-pr-risk-workflows-ref-pin` --- ### 📝 Commits (8) - [`8024f69`](https://github.com/Comfy-Org/github-workflows/commit/8024f692216f18f002121c83a43239cd3f8a377a) fix(pr-risk): enforce workflows_ref as a full 40-hex commit SHA before the tool checkout (BE-6307) - [`48eb512`](https://github.com/Comfy-Org/github-workflows/commit/48eb512200131f68e0dc8b1a9d5c6ed558edd13d) fix(pr-risk): tie workflows_ref to the SHA `uses:` resolved to, not just to SHA shape (BE-6307) - [`dabdfe1`](https://github.com/Comfy-Org/github-workflows/commit/dabdfe1792cd41b1611c57c49f3c164e4fccd333) fix(pr-risk): fail closed when job_workflow_sha is unreadable, and assert the guard by property (BE-6307) - [`b68a2cb`](https://github.com/Comfy-Org/github-workflows/commit/b68a2cb6be3511b589696afcc6600d91ab7cf689) fix(pr-risk): close the guard's neuter paths, and make the suite assert positions not sums (BE-6307) - [`f314614`](https://github.com/Comfy-Org/github-workflows/commit/f314614c9128901134b2baeef37cd35335b7e079) fix(pr-risk): correct the pin contract's claims, sanitize the runner SHA, and close the suite's bypass holes (BE-6307) - [`c77b30b`](https://github.com/Comfy-Org/github-workflows/commit/c77b30b684ccf2b6f1c3bee74520dfd2d700c7a6) fix(pr-risk): scope the alias scan to the guard step, judge emits per statement, cover the runner SHA (BE-6307) - [`4efc5e7`](https://github.com/Comfy-Org/github-workflows/commit/4efc5e7b4796324a283a287ea691e7fbe834cb47) test(pr-risk): use ERE for the emit-scan coverage grep (BE-6307) - [`936e275`](https://github.com/Comfy-Org/github-workflows/commit/936e2752d611d7f6d58a34a623c47dc3b10b072e) fix(pr-risk): drop the lock-step axis — github.job_workflow_sha does not exist (BE-6307) ### 📊 Changes **4 files changed** (+477 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/pr-risk.yml` (+171 -1) 📝 `.github/workflows/test-pr-risk.yml` (+9 -1) 📝 `README.md` (+1 -1) ➕ `scripts/pr-risk/tests/test_pin_contract.sh` (+296 -0) </details> ### 📄 Description ## Correction after review (c2f849b) — read this first A middle revision of this PR added a second "lock-step" axis comparing `workflows_ref` against `${{ github.job_workflow_sha }}`. **That is not a property of the `github` context** — it exists only as an OIDC token claim. It expanded to the empty string, and the fail-closed `-z` branch added beside it would have aborted *every* invocation, correctly pinned callers included, before the tool checkout. Two of eight review models caught it; `actionlint` rejects the expression by name (`property "job_workflow_sha" is not defined in object type {…}`) and GitHub's context reference lists only `workflow_ref` / `workflow_sha`, which name the **caller's** top-level workflow file, not the reusable one. The axis is **removed, not repaired**: reading the claim would need `id-token: write` from every caller plus a token exchange in a job that holds `permissions: {}` — a caller-contract change, not a fix. It is filed as a follow-up for a human to decide on, with the failed attempt written down so nobody re-adds the context expression. So what this PR enforces is **shape only** — the ticket's original scope. The header, the input description and the README now say that explicitly and name review of the caller as the only thing that can bound *which* commit the ref points at. This repo has no `actionlint` job at all, which is why the bad expression survived four review rounds; that is a separate follow-up. ## ELI-5 `pr-risk.yml` downloads its own grading code from this repo at whatever ref the caller names in `workflows_ref`, and then runs that code holding a token that can write to pull requests. The rule "point that at the exact commit you already pinned `uses:` to" was written down in a comment and nowhere else — so a caller could point it at `main`, at a tag, or at `refs/pull/N/head` (which on a public repo resolves to code from somebody's fork) and the run would happily execute it. This PR makes the workflow check: anything that is not a full 40-character commit SHA fails the run *before* the download happens. ## What changed - **`.github/workflows/pr-risk.yml`** — a new `Enforce workflows_ref pin contract` step, first in **both** jobs that check out `workflows_ref` (`gate` and `grade`), rejecting anything that is not `^[0-9a-f]{40}$`. The input's description, the header's pinned-ref paragraph, and the header's caller-pattern block now say the format is enforced rather than requested. `required: true` and the absence of a default are unchanged. - **`README.md`** — the `pr-risk.yml` row now says the format is **ENFORCED**, not just that the input is required. The check lives in the workflow, not in `grade-pr-risk.sh` / `apply-risk-label.sh`, because those scripts *are* what the ref loads — a script-side check would sit inside the blast radius it is meant to bound. ## Judgment calls (deviations from the ticket — please sanity-check these) **1. The guard is in `gate` as well as `grade`.** The ticket specified the `grade` job only; since it was written, #115 added a `gate` job that checks out the same ref *earlier in the run*. Guarding only `grade` would have left the run's first execution of caller-named code unguarded. Both copies are byte-identical by design (asserted locally by parsing the YAML), and each job asserts the contract itself rather than inheriting it — `grade` skips when `gate` fails today, but a job should not depend on another job's step for its own trust boundary, and that invariant survives a later `if:` or graph re-ordering. `gate` holds `permissions: {}`, so the token stakes are entirely in `grade`; the earliness is the value there. **2. A length test + character-class glob, instead of the ticket's `grep -Eq` (and instead of `[[ =~ ]]`).** `grep` anchors `^…$` per **line**, so a multi-line value carrying one SHA-shaped line passes it and is handed to `checkout` in full. `[[ =~ ]]` anchors the whole string, but whether ERE `$` may *also* match just before a trailing newline is libc-dependent — not something a trust boundary should rest on. `if [[ ${#WORKFLOWS_REF} -ne 40 || "$WORKFLOWS_REF" == *[!0-9a-f]* ]]` has no anchors at all: a newline, a space or an uppercase letter is simply a character outside the class. Everything else about the step — the `env:` passthrough (no inline `${{ }}` of the input being validated), the message, the exit code — is as specified. **3. Uppercase SHAs are rejected.** The ticket's regex is lowercase-only and this keeps it. `git rev-parse`, `gh`, and the GitHub UI all emit lowercase, every pin in this repo is lowercase, and a false reject here is loud and fixed by lowercasing. It matters concretely: the value is handed to `actions/checkout` **verbatim**, so accepting a spelling the fetch may not resolve would trade this step's clear message for an obscure failure one step later. (A middle revision widened this to `[0-9a-fA-F]`; review caught that the un-normalized value still reached `checkout`. Narrowed back deliberately.) ## Caller-breakage check — the ticket's premise was stale, so I re-ran it BE-6304 concluded "zero callers pass `workflows_ref` today." That is **no longer true** — two callers now exist. I verified each one's actual pin rather than relying on the absence claim: | Caller | `workflows_ref` | Verdict | |---|---|---| | `Comfy-Org/cloud` `.github/workflows/ci-pr-risk.yml` | `e4a8f7cd4a073da082b03950136530d4df50738f` | full 40-hex — passes | | `Comfy-Org/comfy-cloud-mcp-server` `.github/workflows/ci-pr-risk.yml` | `99288dfbf12c084f629746739e160cba882c80f3` | full 40-hex — passes | Both were run through the shipped guard body verbatim and pass. Also re-confirmed: this repo's `test-pr-risk.yml` never invokes the reusable (it is hermetic over `scripts/pr-risk/`), a global GitHub code search surfaces no callers outside the org, and the `v1` tag (`4d9cb6b`) contains no `pr-risk` file at all, so no `@v1` caller can exist. **The conclusion holds — this breaks nobody — but it is now backed by evidence about the callers that exist, not by their absence.** One behavior change worth naming: `gate` runs regardless of `enabled`, so a repo that enrolls with `enabled: false` **and** a mutable pin now fails its check instead of silently no-opping. That is the intended shape — it surfaces a bad pin at enrollment, which is the cheapest moment to fix it — and neither live caller is in that state. ## Verification - `actionlint .github/workflows/pr-risk.yml` — clean (shellcheck installed, so the `run:` bodies were linted too). Repo-wide `actionlint` is also clean. **Re-run at every revision now** — this is the check that would have caught the `job_workflow_sha` expression immediately. - Guard body **extracted from the shipped YAML** (not retyped) and exercised: `main`, `v1`, `refs/pull/7/head`, empty, `main\n<sha>`, `<sha>\n::add-mask::x`, `<sha> ` (trailing space), a 39-char SHA, a 41-char SHA, and an uppercase SHA all fail with one bounded `::error::`; both live callers' real SHAs pass. - `test_pin_contract.sh` now **pins the guard's executable body verbatim** rather than characterizing it — four of this round's findings were holes in the old emit-scan whitelist (prefix-clears-a-whole-statement, blacklist trigger, total-count fatality, presence-vs-exclusivity), and an equality closes all of them at once. Verified **red** against eleven mutations: permissive wrapper branch, `&&`-chained raw emit, `printenv`, `set -euxo pipefail`, `::warning::` instead of `::error::`, dropped `exit 1`, loosened character class, dropped length test, raw value in the annotation, `if: false` on the step, and an unguarded checkout in a new job — and **green** against a pure rewording of the message. - Parsed the YAML to assert the invariants: the guard precedes the `Load pr-risk tool` checkout in every job that has one, the two copies are byte-identical, and `workflows_ref` is still `required: true`, `type: string`, with no default. - `scripts/pr-risk/` suites (the path-filtered CI job this diff triggers): `shellcheck -x` clean; 24 + 57 + 91 + 21 = **193 tests, 0 failures**. - `check_agents_md.py --root .` passes (1 pre-existing CODEOWNERS warning, untouched by this diff). No test changes: this repo has no harness for workflow YAML, and `test-pr-risk.yml` covers the scripts, which this diff does not touch. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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