[PR #139] [MERGED] feat(pr-risk): require workflows_ref to be an ancestor of upstream main (BE-6492) #149

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-6492-ancestry-axis

📝 Commits (6)

a49e087 feat(pr-risk): require workflows_ref to be an ancestor of upstream main (BE-6492)
c85f124 fix(pr-risk): make the ancestry axis survive a bad network minute (BE-6492)
16a3252 docs(pr-risk): the caller grant is all six, and checks: write is not optional (BE-6492)
dcc17a3 Merge remote-tracking branch 'origin/main' into matt/be-6492-ancestry-axis
3a9e744 fix(pr-risk): bound the ancestry guard's fetches and lazy-fetch, fix stale docs (BE-6492)
e890a1d Merge remote-tracking branch 'origin/main' into matt/be-6492-ancestry-axis

📊 Changes

4 files changed (+444 additions, -54 deletions)

View changed files

📝 .github/workflows/pr-risk.yml (+311 -44)
📝 README.md (+1 -1)
📝 docs/callers/pr-risk.md (+23 -6)
📝 scripts/pr-risk/tests/test_pin_contract.sh (+109 -3)

📄 Description

ELI-5

The pin guard already checked that workflows_ref looks like a commit SHA. It never checked whose commit it was. Because this repo is public, GitHub will hand a fork's commits to anyone who asks for them from this repo's own URL — so a commit written by a fork was just as well-shaped as a real one, and would have been checked out and run inside a job holding the caller's pull-requests: write token. This PR adds a second question: is this commit actually part of our main? If not, the run stops before anything is checked out.

What changed

Both byte-identical copies of the Enforce workflows_ref pin contract step (the gate job and the grade job) gain an ancestry check after the shape check and still before the Load pr-risk tool checkout: fetch the pinned SHA depth-1 from a literal https://github.com/Comfy-Org/github-workflows, fetch main with an explicit +refs/heads/main:refs/heads/upstream-main refspec, then require git merge-base --is-ancestor. The URL is a literal on purpose — inside a reusable workflow github.repository names the caller and github.job_workflow_ref just repeats the caller's uses: string, so no context can name this repo, and a variable there would be an alias a fork could point back at itself. There is no opt-out input, deliberately: an opt-out would be set by the very pin-bump PR the check exists to distrust. All three rejection paths are annotated and fail closed.

Docs updated at the three honesty locations the previous PR established (workflow header, workflows_ref input description, README pr-risk row) plus the caller setup guide, stating both the new claim and the two residuals honestly.

Verification

I ran the real thing against the real upstream, unauthenticated, before shipping it — this is a network shape, and the suites are hermetic so they cannot prove it works:

Input	Result
merged `main` tip	ACCEPT
historical merged pin `97a280d`	ACCEPT
open PR #137 head — in this repo's object network, not on `main` (same serving mechanism as a fork PR head)	REJECT
well-shaped SHA that does not exist	REJECT (unfetchable)

bash scripts/pr-risk/tests/test_pin_contract.sh → 26 passed, 0 failed. All six pr-risk suites green (26 / 63 / 48 / 105 / 27 / 67). shellcheck -x clean across every pr-risk script. AGENTS.md integrity checker passes.

All five required mutations individually go red, each on the property that names it: ancestry deleted from one copy → drift + anchor coverage; from both → anchor coverage + literal-URL; exit 1→exit 0 → fatality + no-exit 0; literal URL → ${{ github.server_url }}/${{ github.repository }} → literal-URL check; ::error:: with no following exit → fatality.

Judgment calls — please read these

1. The ticket's premise about "axis 2" is not true on main, so the code deviates. BE-6492 says the guard "currently enforces two axes: shape and lock-step (== github.job_workflow_sha)" and instructs me to test $JOB_WORKFLOW_SHA because "axis 2 already proved them equal". There is no lock-step axis. PR #118 shipped shape only, and its own comments explain why lock-step is not implementable here: job_workflow_sha is an OIDC token claim, not a github context property, so reading it needs id-token: write from every caller. No $JOB_WORKFLOW_SHA or ${safe_job_sha} variable exists to read. I used $WORKFLOWS_REF and the existing ${safe_ref}. This is not merely a forced substitution — it is the value actions/checkout actually consumes, so validating it directly is at least as sound as validating a proxy proven equal to it. For the same reason the in-file comment calls this the second axis, not the third.

2. The residual is different from what the ticket says, and I documented mine, not its. The ticket says the only residual left is "a fork commit that EDITS pr-risk.yml itself". That would be true if lock-step existed. Since it does not, a stale-but-merged pin is also still unproven — it is an ancestor of main too. The docs state both residuals.

3. Test items 1–3 are implemented; item 4 does not apply. The ticket describes a suite with an emit-scan whitelist and an "every ::error:: is followed by an exit" property. That suite no longer exists: it was deliberately replaced with a verbatim equality on the guard's executable body, and the file's header explicitly records that the pattern-scan approach was abandoned because review kept finding ways through it. So I extended the pinned body (mandatory — otherwise the suite fails), and added the ancestry anchor, literal-URL and fatality checks as named restatements with coverage self-checks. I added a header paragraph making clear these are for better failure messages and are not a second line of defence, so nobody later mistakes them for the guarantee.

4. I added a fourth rejection path the ticket did not specify. The ticket's snippet leaves the main fetch bare, so a failure there exits 128 with no annotation — against this step's own doctrine that a rejection is never an obscure failure one step later. I wrapped it in the same if ! … ::error:: … exit 1 shape. The two-fetch shape and the explicit refspec are unchanged.

5. Blast radius — the one claim I could not independently re-verify. I confirmed directly that nothing in this repo produces a non-ancestor pin: pr-risk.yml has no caller here (so this repo's CI never self-invokes it), and bump-pr-risk-callers.yml triggers only on: push: branches: [main] and writes github.sha after asserting main_tip == GITHUB_SHA — so the bumper can only ever write a main-tip SHA. What I could not re-verify is the ticket's claim that all 4 historical pins across the live consumers are ancestors, because the roster is a private repo variable and this repo is public. That is BE-6491's evidence, taken on trust. Worth a reviewer's glance, especially as #137 (seeding the pr-risk roster) is still open.

6. Capability-denial falsification. This diff denies a capability (pinning an unmerged SHA), so I went looking for a legitimate user rather than assuming there is none — items in 5 above, plus the pre-merge testing path: a change to pr-risk can still be validated before merge by the six hermetic suites in scripts/pr-risk/tests/, which do not need a live pin. The redirect for anyone who was relying on an unmerged pin is "merge here first, then bump", which is what every consumer has done in practice.

7. No retry on transient network failure. A GitHub blip fails the run closed rather than retrying; recovery is a re-run, and the error text says so. I judged a retry loop not worth the added non-determinism on a trust boundary, but it is a deliberate choice, not an oversight.

Cost

Two small fetches of a few-hundred-commit repo per guarded job, ~1–2s each, on gate and grade.

_{🔄 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/139 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/5/2026 **Status:** ✅ Merged **Merged:** 8/10/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6492-ancestry-axis` --- ### 📝 Commits (6) - [`a49e087`](https://github.com/Comfy-Org/github-workflows/commit/a49e087a00dc2202dd9b54477a87cbc569a3fbd1) feat(pr-risk): require workflows_ref to be an ancestor of upstream main (BE-6492) - [`c85f124`](https://github.com/Comfy-Org/github-workflows/commit/c85f12404ce226fa1987f49c5414e9bbe65a877b) fix(pr-risk): make the ancestry axis survive a bad network minute (BE-6492) - [`16a3252`](https://github.com/Comfy-Org/github-workflows/commit/16a32527c63750432b952a308950fd11dc332a9e) docs(pr-risk): the caller grant is all six, and checks: write is not optional (BE-6492) - [`dcc17a3`](https://github.com/Comfy-Org/github-workflows/commit/dcc17a321c2f7677b81f45e946ec04ccc3bfb50b) Merge remote-tracking branch 'origin/main' into matt/be-6492-ancestry-axis - [`3a9e744`](https://github.com/Comfy-Org/github-workflows/commit/3a9e744b38c69060be666558f8e4d61bbf597fd4) fix(pr-risk): bound the ancestry guard's fetches and lazy-fetch, fix stale docs (BE-6492) - [`e890a1d`](https://github.com/Comfy-Org/github-workflows/commit/e890a1dd4e5aca088ade2aceead72720463b67c6) Merge remote-tracking branch 'origin/main' into matt/be-6492-ancestry-axis ### 📊 Changes **4 files changed** (+444 additions, -54 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/pr-risk.yml` (+311 -44) 📝 `README.md` (+1 -1) 📝 `docs/callers/pr-risk.md` (+23 -6) 📝 `scripts/pr-risk/tests/test_pin_contract.sh` (+109 -3) </details> ### 📄 Description ## ELI-5 The pin guard already checked that `workflows_ref` *looks* like a commit SHA. It never checked *whose* commit it was. Because this repo is public, GitHub will hand a fork's commits to anyone who asks for them **from this repo's own URL** — so a commit written by a fork was just as well-shaped as a real one, and would have been checked out and run inside a job holding the caller's `pull-requests: write` token. This PR adds a second question: *is this commit actually part of our `main`?* If not, the run stops before anything is checked out. ## What changed Both byte-identical copies of the `Enforce workflows_ref pin contract` step (the `gate` job and the `grade` job) gain an ancestry check after the shape check and still before the `Load pr-risk tool` checkout: fetch the pinned SHA depth-1 from a **literal** `https://github.com/Comfy-Org/github-workflows`, fetch `main` with an explicit `+refs/heads/main:refs/heads/upstream-main` refspec, then require `git merge-base --is-ancestor`. The URL is a literal on purpose — inside a reusable workflow `github.repository` names the *caller* and `github.job_workflow_ref` just repeats the caller's `uses:` string, so no context can name this repo, and a variable there would be an alias a fork could point back at itself. There is **no opt-out input**, deliberately: an opt-out would be set by the very pin-bump PR the check exists to distrust. All three rejection paths are annotated and fail closed. Docs updated at the three honesty locations the previous PR established (workflow header, `workflows_ref` input description, README `pr-risk` row) plus the caller setup guide, stating both the new claim and the two residuals honestly. ## Verification I ran the real thing against the real upstream, unauthenticated, before shipping it — this is a network shape, and the suites are hermetic so they cannot prove it works: | Input | Result | |---|---| | merged `main` tip | ACCEPT | | historical merged pin `97a280d` | ACCEPT | | **open PR #137 head** — in this repo's object network, not on `main` (same serving mechanism as a fork PR head) | **REJECT** | | well-shaped SHA that does not exist | REJECT (unfetchable) | `bash scripts/pr-risk/tests/test_pin_contract.sh` → 26 passed, 0 failed. All six pr-risk suites green (26 / 63 / 48 / 105 / 27 / 67). `shellcheck -x` clean across every pr-risk script. AGENTS.md integrity checker passes. All five required mutations individually go red, each on the property that names it: ancestry deleted from one copy → drift + anchor coverage; from both → anchor coverage + literal-URL; `exit 1`→`exit 0` → fatality + no-`exit 0`; literal URL → `${{ github.server_url }}/${{ github.repository }}` → literal-URL check; `::error::` with no following exit → fatality. ## Judgment calls — please read these **1. The ticket's premise about "axis 2" is not true on `main`, so the code deviates.** BE-6492 says the guard "currently enforces two axes: shape and lock-step (`== github.job_workflow_sha`)" and instructs me to test `$JOB_WORKFLOW_SHA` because "axis 2 already proved them equal". There is no lock-step axis. PR #118 shipped shape only, and its own comments explain why lock-step is *not implementable here*: `job_workflow_sha` is an OIDC token claim, not a `github` context property, so reading it needs `id-token: write` from every caller. No `$JOB_WORKFLOW_SHA` or `${safe_job_sha}` variable exists to read. I used `$WORKFLOWS_REF` and the existing `${safe_ref}`. This is not merely a forced substitution — it is the value `actions/checkout` actually consumes, so validating it directly is at least as sound as validating a proxy proven equal to it. For the same reason the in-file comment calls this the **second** axis, not the third. **2. The residual is different from what the ticket says, and I documented mine, not its.** The ticket says the only residual left is "a fork commit that EDITS `pr-risk.yml` itself". That would be true if lock-step existed. Since it does not, **a stale-but-merged pin is also still unproven** — it is an ancestor of `main` too. The docs state both residuals. **3. Test items 1–3 are implemented; item 4 does not apply.** The ticket describes a suite with an emit-scan whitelist and an "every `::error::` is followed by an exit" property. That suite no longer exists: it was deliberately replaced with a **verbatim equality on the guard's executable body**, and the file's header explicitly records that the pattern-scan approach was abandoned because review kept finding ways through it. So I extended the pinned body (mandatory — otherwise the suite fails), and added the ancestry anchor, literal-URL and fatality checks as *named restatements* with coverage self-checks. I added a header paragraph making clear these are for better failure messages and are **not** a second line of defence, so nobody later mistakes them for the guarantee. **4. I added a fourth rejection path the ticket did not specify.** The ticket's snippet leaves the `main` fetch bare, so a failure there exits 128 with no annotation — against this step's own doctrine that a rejection is never an obscure failure one step later. I wrapped it in the same `if ! … ::error:: … exit 1` shape. The two-fetch shape and the explicit refspec are unchanged. **5. Blast radius — the one claim I could not independently re-verify.** I confirmed directly that nothing in this repo produces a non-ancestor pin: `pr-risk.yml` has **no caller here** (so this repo's CI never self-invokes it), and `bump-pr-risk-callers.yml` triggers only `on: push: branches: [main]` and writes `github.sha` after asserting `main_tip == GITHUB_SHA` — so the bumper can only ever write a main-tip SHA. What I could **not** re-verify is the ticket's claim that all 4 historical pins across the live consumers are ancestors, because the roster is a private repo variable and this repo is public. That is BE-6491's evidence, taken on trust. Worth a reviewer's glance, especially as #137 (seeding the pr-risk roster) is still open. **6. Capability-denial falsification.** This diff denies a capability (pinning an unmerged SHA), so I went looking for a legitimate user rather than assuming there is none — items in 5 above, plus the pre-merge testing path: a change to pr-risk can still be validated before merge by the six hermetic suites in `scripts/pr-risk/tests/`, which do not need a live pin. The redirect for anyone who *was* relying on an unmerged pin is "merge here first, then bump", which is what every consumer has done in practice. **7. No retry on transient network failure.** A GitHub blip fails the run closed rather than retrying; recovery is a re-run, and the error text says so. I judged a retry loop not worth the added non-determinism on a trust boundary, but it is a deliberate choice, not an oversight. ## Cost Two small fetches of a few-hundred-commit repo per guarded job, ~1–2s each, on `gate` and `grade`. --- <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:47 +00:00