From 64b37b711d3cc69e53fb5908ebb30ae19b8e6cba Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 4 Aug 2026 10:43:01 -0700 Subject: [PATCH] fix(pr-size): require a pinned workflows_ref, guard empty refs, drain the exemption (BE-5858) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pr-size.yml was the last workflow carrying a `workflows_ref` default, so a consumer could SHA-pin `uses: .../pr-size.yml@` and still build the check-pr-size tool from a floating `main` — the pin proving nothing about the code that actually ran. Apply the BE-5546 playbook to it: drop the default, mark the input required, and add the `Require a pinned workflows_ref` step (copied verbatim from cursor-review.yml) ahead of the tool checkout, since GitHub does not enforce `required: true` for workflow_call inputs and an omitted input arrives as '' that checkout resolves to the default branch. Only the `pr-size` job consumes the ref; the `comment` job checks out nothing, so it gets no guard. With the default gone, the KNOWN_EXEMPT entry would itself fail the lint (it hard-fails on a stale exemption by design), so it is removed — leaving the frozenset empty and every reusable workflow here held to both checks. The caller fleet was audited under BE-5856 and re-verified here: the roster has one entry, its caller already passes `workflows_ref` SHA-matched to its `uses:` pin, the check is not a required status on that repo's default branch, and a global code search finds no unenrolled caller. So dropping the default breaks nobody, and the shared bumper moves `uses:` and `workflows_ref` in lockstep. --- .github/workflow-pins/README.md | 10 +++-- .github/workflow-pins/check_workflow_pins.py | 9 ++-- .../tests/test_check_workflow_pins.py | 8 ++-- .github/workflows/pr-size.yml | 41 +++++++++++++++---- README.md | 2 +- 5 files changed, 48 insertions(+), 22 deletions(-) diff --git a/.github/workflow-pins/README.md b/.github/workflow-pins/README.md index e6b2276..570f7d6 100644 --- a/.github/workflow-pins/README.md +++ b/.github/workflow-pins/README.md @@ -72,10 +72,12 @@ guard written in flow style reads as ABSENT and fails loudly, which is the right bias for a check whose whole job is noticing an absence. `KNOWN_EXEMPT` in the script carries workflows with the same debt that are -tracked under their own ticket (today: `pr-size.yml`, whose caller fleet has -not been enumerated yet). The lint fails on a **stale** entry so the list drains -itself rather than rotting — whether the workflow dropped its default (fixed) or -no longer exists under that name at all (renamed or deleted), the latter being +tracked under their own ticket. It is **empty today** — `pr-size.yml`, its last +entry, was fixed in BE-5858 once its caller fleet had been audited, so no +reusable workflow here is carved out of either check. The lint fails on a +**stale** entry so the list drains itself rather than rotting — whether the +workflow dropped its default (fixed) or no longer exists under that name at +all (renamed or deleted), the latter being the case that would otherwise silently pre-exempt whatever later reuses the filename. The list is only applied to this repo's own `.github/workflows`: run against an ad-hoc `--workflows-dir` every entry would look stale. diff --git a/.github/workflow-pins/check_workflow_pins.py b/.github/workflow-pins/check_workflow_pins.py index 5083c96..15fa97b 100644 --- a/.github/workflow-pins/check_workflow_pins.py +++ b/.github/workflow-pins/check_workflow_pins.py @@ -49,12 +49,9 @@ # instead of rotting, whether the workflow dropped its default (fixed) or no # longer exists under that name (renamed or deleted). The latter matters most: # left alone it would pre-exempt whatever future workflow reuses the filename. -# -# pr-size.yml — same shape as the three fixed in BE-5546, but its caller -# fleet (`vars.PR_SIZE_CALLERS`) was not enumerated by the BE-5543 spike, so -# dropping its default is an unverified break of consumer CI. Needs its own -# caller audit first. -KNOWN_EXEMPT = frozenset({"pr-size.yml"}) +# Empty as of BE-5858: the list drained itself exactly as designed, and every +# reusable workflow here is now held to both checks with no carve-out. +KNOWN_EXEMPT = frozenset() _ON_RE = re.compile(r"""^(['"]?)on\1\s*:(.*)$""") _JOBS_RE = re.compile(r"""^(['"]?)jobs\1\s*:""") diff --git a/.github/workflow-pins/tests/test_check_workflow_pins.py b/.github/workflow-pins/tests/test_check_workflow_pins.py index 43b1163..9e4bb23 100644 --- a/.github/workflow-pins/tests/test_check_workflow_pins.py +++ b/.github/workflow-pins/tests/test_check_workflow_pins.py @@ -555,14 +555,16 @@ def test_this_repos_own_workflows_guard_every_ref_checkout(self): os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..", "workflows") ) seen = 0 - for name in ("cursor-review.yml", "groom.yml", "agents-md-integrity.yml"): + for name in ("cursor-review.yml", "groom.yml", "agents-md-integrity.yml", "pr-size.yml"): with open(os.path.join(root, name), encoding="utf-8") as f: lines = f.read().split("\n") uses = [line for line in lines if cwp.is_ref_use(line)] self.assertTrue(uses, "%s: no ref checkout found — fixture drifted" % name) seen += len(uses) self.assertEqual(cwp.find_unguarded_ref_checkouts(lines), [], name) - self.assertEqual(seen, 12, "expected the 12 guarded sites BE-5546 fixed") + self.assertEqual( + seen, 13, "expected the 12 guarded sites BE-5546 fixed + pr-size.yml's (BE-5858)" + ) class CheckDirTests(unittest.TestCase): @@ -793,7 +795,7 @@ def test_this_repos_own_workflows_pass(self): root = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..", "workflows") errors, checked, _ = cwp.check_dir(os.path.normpath(root)) self.assertEqual(errors, [], errors) - for name in ("cursor-review.yml", "groom.yml", "agents-md-integrity.yml"): + for name in ("cursor-review.yml", "groom.yml", "agents-md-integrity.yml", "pr-size.yml"): self.assertIn(name, checked) diff --git a/.github/workflows/pr-size.yml b/.github/workflows/pr-size.yml index 98657e7..0fd285d 100644 --- a/.github/workflows/pr-size.yml +++ b/.github/workflows/pr-size.yml @@ -54,9 +54,9 @@ name: PR Size Cap (reusable) # uses: Comfy-Org/github-workflows/.github/workflows/pr-size.yml@ # v1 # with: # max_lines: 1000 -# # Pin the tool ref to the same ref you pin `uses:` to for -# # reproducibility (defaults to main). -# workflows_ref: +# # REQUIRED. Pin the tool ref to the SAME commit SHA you pin `uses:` +# # to — otherwise the workflow is pinned but its tool is not. +# workflows_ref: # # Optional: post the sticky comment under your GitHub App. # bot_app_id: ${{ vars.APP_ID }} # secrets: @@ -122,12 +122,13 @@ on: default: '' workflows_ref: description: >- - Ref of Comfy-Org/github-workflows to load the check-pr-size tool - from. Pin this to the same ref you pin `uses:` to for - reproducibility. + REQUIRED. Ref of Comfy-Org/github-workflows to load the check-pr-size + tool from. Must be the SAME commit SHA you pin `uses:` to — otherwise + the workflow is pinned but the code it runs is not. There is + deliberately no default: a floating `main` default would silently load + a mutable tool. type: string - required: false - default: main + required: true secrets: BOT_APP_PRIVATE_KEY: description: >- @@ -162,6 +163,30 @@ jobs: persist-credentials: false ref: ${{ github.event.pull_request.head.sha }} + - name: Require a pinned workflows_ref + # workflows_ref has no default on purpose. GitHub does NOT enforce + # `required: true` for workflow_call inputs, so an omitted input arrives + # as '' and actions/checkout would silently fall back to this repo's + # default branch — running MUTABLE scripts under a pin that claims + # otherwise. Fail fast instead. (BE-5546) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + # actions/checkout reads `ref` through core.getInput, which TRIMS, so a + # whitespace-only value is an empty ref to IT while sailing past a bare + # -z test here. Compare the stripped form, and echo only that: dropping + # newlines also stops a multi-line value from smuggling a ::workflow + # command:: into the log, and from satisfying the line-oriented grep + # below on one 40-hex line among many. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::workflows_ref is required; pin it to the same commit SHA as the uses: line (see header example)" + exit 1 + fi + if ! printf '%s' "$REF" | grep -Eq '^[0-9a-f]{40}$'; then + echo "::warning::workflows_ref '$REF' is not a full 40-hex commit SHA — branch and tag refs are mutable and can skew between jobs mid-run" + fi + - name: Load check-pr-size tool # The tool comes from THIS workflow's repo (public, pinned via # workflows_ref) — never from the PR checkout, so no PR-authored code diff --git a/README.md b/README.md index 4d130f9..af7199c 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ The SHA-pin format satisfies pin-validation tooling (`pinact`, `zizmor`, etc.) a A bare `@v1` tag is technically allowed but **will fail** in repos that run pin-validation in CI (e.g. `cloud`, `ComfyUI_frontend`). -Workflows that load their backing scripts at run time take a `workflows_ref` input — always set it to the *same* commit SHA you pin `uses:` to. Pinning only `uses:` runs a pinned workflow that loads **mutable** scripts from a floating branch, which defeats the pin. On `cursor-review.yml`, `groom.yml`, and `agents-md-integrity.yml` the input is **required with no default** and the run fails fast when it is empty or omitted (GitHub does not enforce `required: true` for `workflow_call` inputs, so those workflows check at run time). +Workflows that load their backing scripts at run time take a `workflows_ref` input — always set it to the *same* commit SHA you pin `uses:` to. Pinning only `uses:` runs a pinned workflow that loads **mutable** scripts from a floating branch, which defeats the pin. On every workflow here that takes it — `cursor-review.yml`, `groom.yml`, `agents-md-integrity.yml`, and `pr-size.yml` — the input is **required with no default** and the run fails fast when it is empty or omitted (GitHub does not enforce `required: true` for `workflow_call` inputs, so those workflows check at run time). Per-workflow inputs, required secrets, and triggers are documented in each workflow file's header comment.