[PR #124] [MERGED] fix(pr-size): require a pinned workflows_ref, guard empty refs, drain the exemption (BE-5858) #135

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: matt/be-5546-require-workflows-ref ← Head: matt/be-5858-pr-size-workflows-ref

📝 Commits (1)

64b37b7 fix(pr-size): require a pinned workflows_ref, guard empty refs, drain the exemption (BE-5858)

📊 Changes

5 files changed (+48 additions, -22 deletions)

View changed files

📝 .github/workflow-pins/README.md (+6 -4)
📝 .github/workflow-pins/check_workflow_pins.py (+3 -6)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+5 -3)
📝 .github/workflows/pr-size.yml (+33 -8)
📝 README.md (+1 -1)

📄 Description

STACKED — merging lands on matt/be-5546-require-workflows-ref (PR #103, same author), NOT main. This PR is based on #103's branch because the guard step and the whole .github/workflow-pins/ lint it edits exist only there. Merge #103 first; GitHub then retargets this PR to main. Do not treat this as "ready to merge" into main on its own.

ELI-5

A consumer repo pins our PR-size check by exact commit SHA, which is supposed to mean "run exactly this code, forever." But the check downloads its actual counting tool separately, using a second setting called workflows_ref — and that setting used to quietly default to main. So you could pin the workflow to a frozen SHA and still get a tool built from whatever landed on main five minutes ago. The pin looked airtight and wasn't. This PR removes the default and makes the workflow refuse to run if the setting is missing, so the pin now means what it says.

What changed

pr-size.yml was the last of the four reusable workflows still carrying a workflows_ref default — this closes the BE-5546 pin-integrity sweep.

Input declaration — required: true, default: main deleted, description rewritten to mirror the wording #103 used in cursor-review.yml / groom.yml / agents-md-integrity.yml.
Header caller example — now says the ref is REQUIRED and must match the uses: SHA, instead of "defaults to main".
Runtime guard — the Require a pinned workflows_ref step, copied byte-for-byte from cursor-review.yml (verified programmatically: both extract to an identical 1425-character block), inserted immediately before the Load check-pr-size tool checkout. It is needed because GitHub does not enforce required: true for workflow_call inputs: an omitted input arrives as '', and actions/checkout with ref: '' silently takes the default branch. The step takes the value via env: (never interpolated into the script body), strips whitespace, hard-fails on empty, and warns when the ref is not a full 40-hex SHA.
KNOWN_EXEMPT drained — "pr-size.yml" removed along with its rationale block. This is not optional bookkeeping: the lint hard-fails on a stale exemption by design, so leaving it would turn CI red. The frozenset is now empty, with the explanatory comment kept.
Docs synced — the root README.md usage note and .github/workflow-pins/README.md both named the three fixed workflows / the pr-size exemption and would otherwise have gone stale.

The guard is added only to the pr-size job. The comment job never consumes workflows_ref (it checks out nothing) — verified in this diff, not just inherited from the ticket.

Why this is safe to drop the default

The change denies something that previously worked (omitting the input), so I re-ran the BE-5856 caller audit empirically rather than trusting it:

vars.PR_SIZE_CALLERS has exactly one entry, and it resolves to a live repo.
That caller already passes workflows_ref explicitly, SHA-matched to its own uses: pin (same 40-hex SHA on both lines) — so it is unaffected by the removal.
The pr-size check is not a required status check on that repo's default branch (the branch carries no protection rule at all).
A global GitHub code search for github-workflows/.github/workflows/pr-size.yml returns 3 hits: this repo's own header example, this repo's bump-callers test fixture, and that one caller. No unenrolled consumer exists — the roster audit holds in both directions.

Caller repo names are withheld here per the public-repo hygiene rule; the evidence above was gathered from vars.PR_SIZE_CALLERS and the code-search API.

No caller change is needed. After this merges, bump-pr-size-callers.yml bumps the caller's uses: SHA and workflows_ref in lockstep — confirmed in bump-callers.sh, whose SHA_ADDR matches /github-workflows|workflows_ref/, so both pins move together.

Verification

python3 .github/workflow-pins/check_workflow_pins.py → green: "4 workflow(s) declare workflows_ref, none with a default, every ref checkout guarded (0 exempt)."
python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' → 71 passed.
actionlint .github/workflows/*.yml → clean.
Other suites (cursor-review 40, agents-md-integrity 18, groom 170, bump-callers 123 + shellcheck) → all green.
Grep sanity: no default: main in pr-size.yml; exactly one Require a pinned workflows_ref step, at line 166, ahead of Load check-pr-size tool at line 190.

Judgment calls

Stacked instead of blocked. The ticket's precondition said to hand this back if #103 had not merged. #103 is open (CI green, but mergeable: CONFLICTING against main — its author needs to reconcile it). Both reasons the ticket gave for waiting are resolved by stacking rather than waiting: the files to edit exist on #103's branch, and #103's own lint CI is untouched because this change lands on a separate branch that can only merge after #103 does. The standing team directive is to stack on an unmerged blocker that carries buildable branch code rather than gate on it, so I stacked. If you would rather this had waited, closing this PR costs nothing — the branch keeps the work.
Two test tweaks beyond the ticket's list. pr-size.yml was added to the two "this repo's own workflows" test loops, and the guarded-checkout count went 12 → 13. Without this the new guard has no direct regression pin.
Two doc lines beyond the ticket's list. Removing the exemption made the root README.md sentence ("on cursor-review.yml, groom.yml, and agents-md-integrity.yml…") and the workflow-pins/README.md "today: pr-size.yml" note factually wrong. I rewrote both rather than leave stale docs in the same commit that invalidated them.

No unmet acceptance criteria.

_{🔄 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/124 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/4/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `matt/be-5546-require-workflows-ref` ← **Head:** `matt/be-5858-pr-size-workflows-ref` --- ### 📝 Commits (1) - [`64b37b7`](https://github.com/Comfy-Org/github-workflows/commit/64b37b711d3cc69e53fb5908ebb30ae19b8e6cba) fix(pr-size): require a pinned workflows_ref, guard empty refs, drain the exemption (BE-5858) ### 📊 Changes **5 files changed** (+48 additions, -22 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflow-pins/README.md` (+6 -4) 📝 `.github/workflow-pins/check_workflow_pins.py` (+3 -6) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+5 -3) 📝 `.github/workflows/pr-size.yml` (+33 -8) 📝 `README.md` (+1 -1) </details> ### 📄 Description > **STACKED — merging lands on `matt/be-5546-require-workflows-ref` (PR #103, same author), NOT `main`.** This PR is based on #103's branch because the guard step and the whole `.github/workflow-pins/` lint it edits exist only there. Merge #103 first; GitHub then retargets this PR to `main`. Do not treat this as "ready to merge" into `main` on its own. ## ELI-5 A consumer repo pins our PR-size check by exact commit SHA, which is supposed to mean "run exactly this code, forever." But the check downloads its actual counting tool separately, using a second setting called `workflows_ref` — and that setting used to quietly default to `main`. So you could pin the workflow to a frozen SHA and still get a tool built from whatever landed on `main` five minutes ago. The pin looked airtight and wasn't. This PR removes the default and makes the workflow refuse to run if the setting is missing, so the pin now means what it says. ## What changed `pr-size.yml` was the last of the four reusable workflows still carrying a `workflows_ref` default — this closes the BE-5546 pin-integrity sweep. - **Input declaration** — `required: true`, `default: main` deleted, description rewritten to mirror the wording #103 used in `cursor-review.yml` / `groom.yml` / `agents-md-integrity.yml`. - **Header caller example** — now says the ref is REQUIRED and must match the `uses:` SHA, instead of "defaults to main". - **Runtime guard** — the `Require a pinned workflows_ref` step, copied **byte-for-byte** from `cursor-review.yml` (verified programmatically: both extract to an identical 1425-character block), inserted immediately before the `Load check-pr-size tool` checkout. It is needed because GitHub does **not** enforce `required: true` for `workflow_call` inputs: an omitted input arrives as `''`, and `actions/checkout` with `ref: ''` silently takes the default branch. The step takes the value via `env:` (never interpolated into the script body), strips whitespace, hard-fails on empty, and warns when the ref is not a full 40-hex SHA. - **`KNOWN_EXEMPT` drained** — `"pr-size.yml"` removed along with its rationale block. This is not optional bookkeeping: the lint hard-fails on a *stale* exemption by design, so leaving it would turn CI red. The frozenset is now empty, with the explanatory comment kept. - **Docs synced** — the root `README.md` usage note and `.github/workflow-pins/README.md` both named the three fixed workflows / the pr-size exemption and would otherwise have gone stale. The guard is added **only** to the `pr-size` job. The `comment` job never consumes `workflows_ref` (it checks out nothing) — verified in this diff, not just inherited from the ticket. ## Why this is safe to drop the default The change *denies* something that previously worked (omitting the input), so I re-ran the BE-5856 caller audit empirically rather than trusting it: - `vars.PR_SIZE_CALLERS` has **exactly one** entry, and it resolves to a live repo. - That caller already passes `workflows_ref` explicitly, **SHA-matched to its own `uses:` pin** (same 40-hex SHA on both lines) — so it is unaffected by the removal. - The pr-size check is **not** a required status check on that repo's default branch (the branch carries no protection rule at all). - A **global GitHub code search** for `github-workflows/.github/workflows/pr-size.yml` returns 3 hits: this repo's own header example, this repo's `bump-callers` test fixture, and that one caller. No unenrolled consumer exists — the roster audit holds in both directions. Caller repo names are withheld here per the public-repo hygiene rule; the evidence above was gathered from `vars.PR_SIZE_CALLERS` and the code-search API. No caller change is needed. After this merges, `bump-pr-size-callers.yml` bumps the caller's `uses:` SHA and `workflows_ref` in lockstep — confirmed in `bump-callers.sh`, whose `SHA_ADDR` matches `/github-workflows|workflows_ref/`, so both pins move together. ## Verification - `python3 .github/workflow-pins/check_workflow_pins.py` → green: *"4 workflow(s) declare `workflows_ref`, none with a default, every ref checkout guarded (0 exempt)."* - `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'` → 71 passed. - `actionlint .github/workflows/*.yml` → clean. - Other suites (cursor-review 40, agents-md-integrity 18, groom 170, bump-callers 123 + shellcheck) → all green. - Grep sanity: no `default: main` in `pr-size.yml`; exactly one `Require a pinned workflows_ref` step, at line 166, ahead of `Load check-pr-size tool` at line 190. ## Judgment calls 1. **Stacked instead of blocked.** The ticket's precondition said to hand this back if #103 had not merged. #103 is open (CI green, but `mergeable: CONFLICTING` against `main` — its author needs to reconcile it). Both reasons the ticket gave for waiting are resolved by stacking rather than waiting: the files to edit exist on #103's branch, and #103's own lint CI is untouched because this change lands on a separate branch that can only merge after #103 does. The standing team directive is to stack on an unmerged blocker that carries buildable branch code rather than gate on it, so I stacked. If you would rather this had waited, closing this PR costs nothing — the branch keeps the work. 2. **Two test tweaks beyond the ticket's list.** `pr-size.yml` was added to the two "this repo's own workflows" test loops, and the guarded-checkout count went 12 → 13. Without this the new guard has no direct regression pin. 3. **Two doc lines beyond the ticket's list.** Removing the exemption made the root `README.md` sentence ("on `cursor-review.yml`, `groom.yml`, and `agents-md-integrity.yml`…") and the `workflow-pins/README.md` "today: `pr-size.yml`" note factually wrong. I rewrote both rather than leave stale docs in the same commit that invalidated them. No unmet acceptance criteria. --- <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:35 +00:00