[PR #103] [MERGED] fix(workflows): make workflows_ref required, guard empty refs, lint the default (BE-5546) #113

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-5546-require-workflows-ref

📝 Commits (10+)

b668437 fix(workflows): make workflows_ref required, guard empty refs, lint the default (BE-5546)
a924144 fix(workflows): close the review panel's gaps in the workflows_ref lint + guard (BE-5546)
697e07e docs(workflow-pins): correct the KNOWN_EXEMPT staleness comment (BE-5546)
a8f101e fix(workflow-pins): catch the flow-mapping form of the ref checkout (BE-5546)
c9c4cf7 fix(workflow-pins): catch the multi-line spelling of the ref checkout (BE-5546)
c59ceae fix(workflow-pins): catch the commented spelling of the ref checkout (BE-5546)
66984ae fix(workflow-pins): the env binding is not the guard — verify the rejection (BE-5546)
2866ac1 fix(workflow-pins): tie the guard's empty test to the ref and its own branch (BE-5546)
ad1036e fix(workflow-pins): the guard's condition must be the empty test, not contain it (BE-5546)
69e8a4e fix(workflow-pins): the inline branch ends at fi, and its exit must be bare (BE-5546)

📊 Changes

11 files changed (+2262 additions, -38 deletions)

View changed files

📝 .github/cursor-review/README.md (+5 -4)
➕ .github/workflow-pins/README.md (+101 -0)
➕ .github/workflow-pins/check_workflow_pins.py (+815 -0)
➕ .github/workflow-pins/tests/test_check_workflow_pins.py (+937 -0)
📝 .github/workflows/agents-md-integrity.yml (+36 -9)
📝 .github/workflows/cursor-review.yml (+173 -9)
📝 .github/workflows/pr-size.yml (+33 -8)
📝 .github/workflows/refresh-reviewers.yml (+24 -0)
➕ .github/workflows/test-workflow-pins.yml (+108 -0)
📝 AGENTS.md (+16 -3)
📝 README.md (+14 -5)

📄 Description

ELI-5

Our shared workflows keep their real logic (prompts, checkers, the groom briefs) here, and each caller repo says "run workflow X, and load its scripts from ref Y". Ref Y is the workflows_ref input, and it used to default to main. So a repo could carefully pin the workflow to an exact commit and still be loading the scripts from whatever landed on main five minutes ago — into jobs that hold write permissions. The careful pin bought nothing.

This PR removes that default on the three affected workflows, makes the input required, fails the run fast when it is missing, and adds a lint so the default can't quietly come back.

What changed

workflows_ref is required, with no default — in cursor-review.yml, groom.yml, agents-md-integrity.yml, pr-size.yml (added mid-review, judgment call below), and refresh-reviewers.yml (main's own design already required it; this PR only closed a guard gap main introduced there — see judgment calls). Descriptions and header examples now say it is required and must equal the uses: SHA.
A run-time guard immediately before every assets checkout (17 sites across the 5 workflows above — grew from the original 12 as review rounds added pr-size.yml, and merging main picked up cursor-review.yml's new preflight/ledger/diff-size jobs and refresh-reviewers.yml). ::error + exit 1 on an empty value; a non-fatal ::warning when the value isn't a full 40-hex SHA (branch/tag refs are mutable and can skew between jobs mid-run). The value is passed via env:, never interpolated into the script body.
Docs that taught the hole are fixed — the cursor-review.yml header example (workflows_ref: main → <same-sha-as-uses>, and its uses: …@main → @<sha> # v1), the agents-md-integrity.yml header ("Default main" → REQUIRED), .github/cursor-review/README.md's input table, plus new notes in README.md and AGENTS.md.
A regression lint: .github/workflow-pins/ — a stdlib-only, text-level checker (no PyYAML in this repo), wired up as test-workflow-pins.yml. It fails if any on: workflow_call workflow (a) declares a default: for its workflows_ref input, in block or flow-mapping form, or (b) checks out at ref: ${{ inputs.workflows_ref }} in a job that does not run the empty-ref guard first. (b) was added in review: the default is only half the hole, since a new job reopens the ref: '' fallback with a default-only lint still green — there was never a default: to find. A file that uses the input but whose declaration the parser cannot locate is a hard error, not a silent skip. 32-case unit suite plus two CLI smoke tests (a reintroduced default, and an unguarded checkout) proving each really exits non-zero.

Why the guard is mandatory, not belt-and-braces

GitHub does not enforce required: true for workflow_call inputs. An omitted input arrives as '', and actions/checkout with ref: '' silently checks out the default branch — recreating the exact hole. required: true is documentation; the guard is the enforcement.

Falsification of the "this breaks nobody" claim

This change denies a previously-working configuration (omitting workflows_ref), so I went and looked instead of trusting the premise. GitHub code search surfaces 42 caller files across org repos, private repos my token can read, and six external forks/mirrors (Windecay/SimpAI_Studio, ussoewwin/*, BossGao0808/AstroTexAI, iho6/photoreal_ai_app, kylevirtuous1211/*, Sifr-r/Documents). All pass an explicit 40-hex workflows_ref. Zero break. Re-audited during review with indent-aware job-block parsing: 38 real caller jobs across 23 repos, all passing the input explicitly (the earlier count included this repo's own header examples and test fixtures). Zero break, confirmed twice. This corroborates the BE-5543 spike independently — no bump-callers rollout needed, no tag action.

Judgment calls

pr-size.yml had the same hole; UPDATE — it's now fixed, not exempted. It shipped as a fourth workflow_call workflow with workflows_ref / default: main. The original round of this PR left it in the lint's KNOWN_EXEMPT (caller fleet unaudited, so dropping the default was an unverified break of a required status check downstream) and filed BE-5856 to audit it. That audit happened: vars.PR_SIZE_CALLERS has exactly one entry, its caller already passes workflows_ref SHA-matched to its uses: pin, and the check is not a required status on that repo's default branch — so dropping the default breaks nobody. pr-size.yml now carries required: true, no default, and the standard guard (same as the other four), and KNOWN_EXEMPT is empty — the frozenset stays empty rather than growing new entries.
refresh-reviewers.yml picked up the guard too, with no compatibility question. It's a workflow main added after this branch forked, already shipping workflows_ref as required: true with no default from day one — reconciling this branch with main (merge commit) surfaced that its assets checkout had no empty-ref guard ahead of it (main doesn't carry this PR's guard convention). Added the same guard step; no required/default change, so no caller-breakage question to audit — this is a straight security-gap close, not an interface change.
The guard is copied inline 12 times rather than factored into a composite action. A composite would have to be loaded via uses: Comfy-Org/github-workflows/.github/actions/…@<ref> — the very ref being validated — and a job can't uses: ./… before its checkout. Making the check depend on the thing it checks is worse than the duplication; noted in .github/workflow-pins/README.md so the next reviewer doesn't "simplify" it.
Guard placement follows the ticket's "simplest correct form": immediately before each assets checkout, rather than once in a gate job. It stays correct if the needs: graph changes later.
No tag work, per the ticket: v1 predates all three workflows (it carries only detect-unreviewed-merge.yml) and no caller consumes tags. bump-callers.sh is untouched — the pin shape is unchanged.
ci-groom.yml's REPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHA placeholder is a pre-existing breakage owned by its own ticket; left alone. It is non-empty, so it takes the new SHA-shape warning rather than the hard failure.
AGENTS.md grew by 6 lines and is now exactly 150 — at, not over, its aspirational target (hard ceiling 200).

Testing

actionlint clean on all 24 workflow files (it runs shellcheck over every run: block, so the new guard shell is shellcheck-clean).
New suite: 32 tests, covering the positive/negative fixtures plus the block-boundary cases text parsing gets wrong — a workflows_ref mentioned in a header comment, a caller's with: value of the same name, a default: belonging to the next input, inline on: [push], a quoted "on": key, and stale-exemption detection. One test lints this repo's own checked-in tree.
Existing suites all green: cursor-review 40, agents-md-integrity 18, groom 170, bump-callers 123 (+ shellcheck -x), and check_agents_md.py --root . passes.
Self-CI is unaffected: this repo's own callers (ci-cursor-review.yml, ci-groom.yml) both pass workflows_ref explicitly and pin an already-merged SHA of the reusable, not this PR's head.
Merged main in (BE-5546 vs. main drift). main moved substantially since this branch forked: cursor-review.yml gained preflight/ledger jobs and a diff-size job that checks out a shared check-pr-size tool at ref: ${{ inputs.workflows_ref }} with no guard (main doesn't know this PR's guard convention — added one, matching the other 16 sites); extra_generated_globs/extra_lockfiles inputs were added alongside diff_excludes (kept both in the merged docs); and pr-size.yml/refresh-reviewers.yml/bump-callers/groom picked up unrelated fixes. Reproduced the gap locally by merging and re-running the workflow-pins suite before touching anything (it failed 3/77 pointing at the exact new unguarded site), fixed it, and updated the self-test's hardcoded guarded-site count (15 → 16 across the four files it enumerates) to match. Every suite in the repo is green post-merge: cursor-review (159), workflow-pins (78, incl. the new test below), agents-md-integrity (46), groom (365), refresh-reviewers (48), bump-callers + preflight shell suites (185 + 107) and shellcheck -x, pr-risk's shell suites (377), and check-pr-size's Go tests.
Closed a second, independently-reported guard gap: a comment-only continue-on-error: line. CodeRabbit flagged that continue-on-error: # note with the real true/false on the next, more-indented line was read by is_guard_step as an empty (= not-disqualifying) value, when Actions actually takes the continuation as the value — a guard that fails the step exactly as designed while continue-on-error: true on the next line lets the job carry on to the checkout anyway. Fixed to read the continuation line when the inline value is empty/comment-only, with a same-line-true and continuation-false regression test.
Review round (a924144): the guard shell was hardened in all 12 copies — it now trims before the empty test (actions/checkout reads ref via core.getInput, which trims, so a whitespace-only value was an empty ref to it while passing a bare -z) and echoes only the stripped value, which also stops a multi-line ref from satisfying the line-oriented grep on one 40-hex line and from smuggling a ::workflow command:: into the log. Verified behaviourally against each adversarial input, not by inspection. The guard-coverage check was verified non-vacuous: it finds all 12 sites, and flags all 12 when the guard is stripped.

_{🔄 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/103 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/1/2026 **Status:** ✅ Merged **Merged:** 8/12/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-5546-require-workflows-ref` --- ### 📝 Commits (10+) - [`b668437`](https://github.com/Comfy-Org/github-workflows/commit/b668437e8c4e5f14b50d9153d49310cf0486c4e7) fix(workflows): make workflows_ref required, guard empty refs, lint the default (BE-5546) - [`a924144`](https://github.com/Comfy-Org/github-workflows/commit/a924144fab8f2e2392fc10afb04fcf8a94d7d289) fix(workflows): close the review panel's gaps in the workflows_ref lint + guard (BE-5546) - [`697e07e`](https://github.com/Comfy-Org/github-workflows/commit/697e07ee25639d3d45b74a49df7a22195b9ff6b3) docs(workflow-pins): correct the KNOWN_EXEMPT staleness comment (BE-5546) - [`a8f101e`](https://github.com/Comfy-Org/github-workflows/commit/a8f101e4e3c1e70cd4e65ea064f36ca58bd99ab8) fix(workflow-pins): catch the flow-mapping form of the ref checkout (BE-5546) - [`c9c4cf7`](https://github.com/Comfy-Org/github-workflows/commit/c9c4cf77a7556a7444fd69998dc1eab29dbafe80) fix(workflow-pins): catch the multi-line spelling of the ref checkout (BE-5546) - [`c59ceae`](https://github.com/Comfy-Org/github-workflows/commit/c59ceae511e56d66d4bb86368cba6feebca5c423) fix(workflow-pins): catch the commented spelling of the ref checkout (BE-5546) - [`66984ae`](https://github.com/Comfy-Org/github-workflows/commit/66984aeae5c54bff96bfad895cd8ed6454f01169) fix(workflow-pins): the env binding is not the guard — verify the rejection (BE-5546) - [`2866ac1`](https://github.com/Comfy-Org/github-workflows/commit/2866ac1caffa0b5cb43f1d4a6d379fe8919b4183) fix(workflow-pins): tie the guard's empty test to the ref and its own branch (BE-5546) - [`ad1036e`](https://github.com/Comfy-Org/github-workflows/commit/ad1036ebf74f59e2ea26bb2a34f655a312899daf) fix(workflow-pins): the guard's condition must be the empty test, not contain it (BE-5546) - [`69e8a4e`](https://github.com/Comfy-Org/github-workflows/commit/69e8a4e295e394b6380897a231ff13076356c98b) fix(workflow-pins): the inline branch ends at `fi`, and its exit must be bare (BE-5546) ### 📊 Changes **11 files changed** (+2262 additions, -38 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+5 -4) ➕ `.github/workflow-pins/README.md` (+101 -0) ➕ `.github/workflow-pins/check_workflow_pins.py` (+815 -0) ➕ `.github/workflow-pins/tests/test_check_workflow_pins.py` (+937 -0) 📝 `.github/workflows/agents-md-integrity.yml` (+36 -9) 📝 `.github/workflows/cursor-review.yml` (+173 -9) 📝 `.github/workflows/pr-size.yml` (+33 -8) 📝 `.github/workflows/refresh-reviewers.yml` (+24 -0) ➕ `.github/workflows/test-workflow-pins.yml` (+108 -0) 📝 `AGENTS.md` (+16 -3) 📝 `README.md` (+14 -5) </details> ### 📄 Description ## ELI-5 Our shared workflows keep their real logic (prompts, checkers, the groom briefs) here, and each caller repo says "run workflow X, and load its scripts from ref Y". Ref Y is the `workflows_ref` input, and it used to default to `main`. So a repo could carefully pin the *workflow* to an exact commit and still be loading the *scripts* from whatever landed on `main` five minutes ago — into jobs that hold write permissions. The careful pin bought nothing. This PR removes that default on the three affected workflows, makes the input required, fails the run fast when it is missing, and adds a lint so the default can't quietly come back. ## What changed 1. **`workflows_ref` is required, with no default** — in `cursor-review.yml`, `groom.yml`, `agents-md-integrity.yml`, `pr-size.yml` (added mid-review, judgment call below), and `refresh-reviewers.yml` (main's own design already required it; this PR only closed a guard gap main introduced there — see judgment calls). Descriptions and header examples now say it is required and must equal the `uses:` SHA. 2. **A run-time guard immediately before every assets checkout** (17 sites across the 5 workflows above — grew from the original 12 as review rounds added `pr-size.yml`, and merging `main` picked up `cursor-review.yml`'s new `preflight`/`ledger`/`diff-size` jobs and `refresh-reviewers.yml`). `::error` + `exit 1` on an empty value; a non-fatal `::warning` when the value isn't a full 40-hex SHA (branch/tag refs are mutable and can skew between jobs mid-run). The value is passed via `env:`, never interpolated into the script body. 3. **Docs that taught the hole are fixed** — the `cursor-review.yml` header example (`workflows_ref: main` → `<same-sha-as-uses>`, and its `uses: …@main` → `@<sha> # v1`), the `agents-md-integrity.yml` header ("Default `main`" → REQUIRED), `.github/cursor-review/README.md`'s input table, plus new notes in `README.md` and `AGENTS.md`. 4. **A regression lint: `.github/workflow-pins/`** — a stdlib-only, text-level checker (no PyYAML in this repo), wired up as `test-workflow-pins.yml`. It fails if any `on: workflow_call` workflow (a) declares a `default:` for its `workflows_ref` input, in block or flow-mapping form, or (b) checks out at `ref: ${{ inputs.workflows_ref }}` in a job that does not run the empty-ref guard first. (b) was added in review: the default is only half the hole, since a *new* job reopens the `ref: ''` fallback with a default-only lint still green — there was never a `default:` to find. A file that uses the input but whose declaration the parser cannot locate is a hard error, not a silent skip. 32-case unit suite plus two CLI smoke tests (a reintroduced default, and an unguarded checkout) proving each really exits non-zero. ## Why the guard is mandatory, not belt-and-braces **GitHub does not enforce `required: true` for `workflow_call` inputs.** An omitted input arrives as `''`, and `actions/checkout` with `ref: ''` silently checks out the default branch — recreating the exact hole. `required: true` is documentation; the guard is the enforcement. ## Falsification of the "this breaks nobody" claim This change *denies* a previously-working configuration (omitting `workflows_ref`), so I went and looked instead of trusting the premise. GitHub code search surfaces **42 caller files** across org repos, private repos my token can read, and six external forks/mirrors (`Windecay/SimpAI_Studio`, `ussoewwin/*`, `BossGao0808/AstroTexAI`, `iho6/photoreal_ai_app`, `kylevirtuous1211/*`, `Sifr-r/Documents`). **All pass an explicit 40-hex `workflows_ref`.** Zero break. Re-audited during review with indent-aware job-block parsing: **38 real caller jobs across 23 repos, all passing the input explicitly** (the earlier count included this repo's own header examples and test fixtures). Zero break, confirmed twice. This corroborates the BE-5543 spike independently — no bump-callers rollout needed, no tag action. ## Judgment calls - **`pr-size.yml` had the same hole; UPDATE — it's now fixed, not exempted.** It shipped as a fourth `workflow_call` workflow with `workflows_ref` / `default: main`. The original round of this PR left it in the lint's `KNOWN_EXEMPT` (caller fleet unaudited, so dropping the default was an unverified break of a required status check downstream) and filed BE-5856 to audit it. That audit happened: `vars.PR_SIZE_CALLERS` has exactly one entry, its caller already passes `workflows_ref` SHA-matched to its `uses:` pin, and the check is not a required status on that repo's default branch — so dropping the default breaks nobody. `pr-size.yml` now carries `required: true`, no default, and the standard guard (same as the other four), and `KNOWN_EXEMPT` is empty — the frozenset stays empty rather than growing new entries. - **`refresh-reviewers.yml` picked up the guard too, with no compatibility question.** It's a workflow `main` added after this branch forked, already shipping `workflows_ref` as `required: true` with no default from day one — reconciling this branch with `main` (merge commit) surfaced that its assets checkout had no empty-ref guard ahead of it (main doesn't carry this PR's guard convention). Added the same guard step; no `required`/`default` change, so no caller-breakage question to audit — this is a straight security-gap close, not an interface change. - **The guard is copied inline 12 times rather than factored into a composite action.** A composite would have to be loaded via `uses: Comfy-Org/github-workflows/.github/actions/…@<ref>` — the very ref being validated — and a job can't `uses: ./…` before its checkout. Making the check depend on the thing it checks is worse than the duplication; noted in `.github/workflow-pins/README.md` so the next reviewer doesn't "simplify" it. - **Guard placement** follows the ticket's "simplest correct form": immediately before each assets checkout, rather than once in a gate job. It stays correct if the `needs:` graph changes later. - **No tag work**, per the ticket: `v1` predates all three workflows (it carries only `detect-unreviewed-merge.yml`) and no caller consumes tags. `bump-callers.sh` is untouched — the pin shape is unchanged. - `ci-groom.yml`'s `REPLACE_AT_MERGE_WITH_THIS_PRS_SQUASH_SHA` placeholder is a pre-existing breakage owned by its own ticket; left alone. It is non-empty, so it takes the new SHA-shape warning rather than the hard failure. - `AGENTS.md` grew by 6 lines and is now exactly 150 — at, not over, its aspirational target (hard ceiling 200). ## Testing - `actionlint` clean on all 24 workflow files (it runs `shellcheck` over every `run:` block, so the new guard shell is shellcheck-clean). - New suite: 32 tests, covering the positive/negative fixtures plus the block-boundary cases text parsing gets wrong — a `workflows_ref` mentioned in a header comment, a caller's `with:` value of the same name, a `default:` belonging to the *next* input, inline `on: [push]`, a quoted `"on":` key, and stale-exemption detection. One test lints this repo's own checked-in tree. - Existing suites all green: cursor-review 40, agents-md-integrity 18, groom 170, bump-callers 123 (+ `shellcheck -x`), and `check_agents_md.py --root .` passes. - Self-CI is unaffected: this repo's own callers (`ci-cursor-review.yml`, `ci-groom.yml`) both pass `workflows_ref` explicitly and pin an already-merged SHA of the reusable, not this PR's head. - **Merged `main` in (BE-5546 vs. main drift).** `main` moved substantially since this branch forked: `cursor-review.yml` gained `preflight`/`ledger` jobs and a `diff-size` job that checks out a shared `check-pr-size` tool at `ref: ${{ inputs.workflows_ref }}` with no guard (main doesn't know this PR's guard convention — added one, matching the other 16 sites); `extra_generated_globs`/`extra_lockfiles` inputs were added alongside `diff_excludes` (kept both in the merged docs); and `pr-size.yml`/`refresh-reviewers.yml`/`bump-callers`/`groom` picked up unrelated fixes. Reproduced the gap locally by merging and re-running the workflow-pins suite before touching anything (it failed 3/77 pointing at the exact new unguarded site), fixed it, and updated the self-test's hardcoded guarded-site count (15 → 16 across the four files it enumerates) to match. Every suite in the repo is green post-merge: cursor-review (159), workflow-pins (78, incl. the new test below), agents-md-integrity (46), groom (365), refresh-reviewers (48), bump-callers + preflight shell suites (185 + 107) and `shellcheck -x`, pr-risk's shell suites (377), and `check-pr-size`'s Go tests. - **Closed a second, independently-reported guard gap: a comment-only `continue-on-error:` line.** CodeRabbit flagged that `continue-on-error: # note` with the real `true`/`false` on the next, more-indented line was read by `is_guard_step` as an empty (= not-disqualifying) value, when Actions actually takes the continuation as the value — a guard that fails the step exactly as designed while `continue-on-error: true` on the next line lets the job carry on to the checkout anyway. Fixed to read the continuation line when the inline value is empty/comment-only, with a same-line-`true` and continuation-`false` regression test. - Review round (a924144): the guard shell was hardened in all 12 copies — it now trims before the empty test (`actions/checkout` reads `ref` via `core.getInput`, which trims, so a whitespace-only value was an empty ref to *it* while passing a bare `-z`) and echoes only the stripped value, which also stops a multi-line ref from satisfying the line-oriented `grep` on one 40-hex line and from smuggling a `::workflow command::` into the log. Verified behaviourally against each adversarial input, not by inspection. The guard-coverage check was verified non-vacuous: it finds all 12 sites, and flags all 12 when the guard is stripped. --- <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:23 +00:00