fix(pr-size): require a pinned workflows_ref, guard empty refs, drain the exemption (BE-5858) - #124
Conversation
… the exemption (BE-5858) pr-size.yml was the last workflow carrying a `workflows_ref` default, so a consumer could SHA-pin `uses: .../pr-size.yml@<sha>` 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.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 4 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 2 |
| 🟢 Low | 1 |
Panel: 8/8 reviewers contributed findings.
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
The following carry
|
|
Merging unreviewed. Blast radius: |
8c0b682
into
matt/be-5546-require-workflows-ref
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 tomain. So you could pin the workflow to a frozen SHA and still get a tool built from whatever landed onmainfive 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.ymlwas the last of the four reusable workflows still carrying aworkflows_refdefault — this closes the BE-5546 pin-integrity sweep.required: true,default: maindeleted, description rewritten to mirror the wording fix(workflows): make workflows_ref required, guard empty refs, lint the default (BE-5546) #103 used incursor-review.yml/groom.yml/agents-md-integrity.yml.uses:SHA, instead of "defaults to main".Require a pinned workflows_refstep, copied byte-for-byte fromcursor-review.yml(verified programmatically: both extract to an identical 1425-character block), inserted immediately before theLoad check-pr-size toolcheckout. It is needed because GitHub does not enforcerequired: trueforworkflow_callinputs: an omitted input arrives as'', andactions/checkoutwithref: ''silently takes the default branch. The step takes the value viaenv:(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_EXEMPTdrained —"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.README.mdusage note and.github/workflow-pins/README.mdboth named the three fixed workflows / the pr-size exemption and would otherwise have gone stale.The guard is added only to the
pr-sizejob. Thecommentjob never consumesworkflows_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_CALLERShas exactly one entry, and it resolves to a live repo.workflows_refexplicitly, SHA-matched to its ownuses:pin (same 40-hex SHA on both lines) — so it is unaffected by the removal.github-workflows/.github/workflows/pr-size.ymlreturns 3 hits: this repo's own header example, this repo'sbump-callerstest 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_CALLERSand the code-search API.No caller change is needed. After this merges,
bump-pr-size-callers.ymlbumps the caller'suses:SHA andworkflows_refin lockstep — confirmed inbump-callers.sh, whoseSHA_ADDRmatches/github-workflows|workflows_ref/, so both pins move together.Verification
python3 .github/workflow-pins/check_workflow_pins.py→ green: "4 workflow(s) declareworkflows_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.default: maininpr-size.yml; exactly oneRequire a pinned workflows_refstep, at line 166, ahead ofLoad check-pr-size toolat line 190.Judgment calls
mergeable: CONFLICTINGagainstmain— 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 fix(workflows): make workflows_ref required, guard empty refs, lint the default (BE-5546) #103's branch, and fix(workflows): make workflows_ref required, guard empty refs, lint the default (BE-5546) #103's own lint CI is untouched because this change lands on a separate branch that can only merge after fix(workflows): make workflows_ref required, guard empty refs, lint the default (BE-5546) #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.pr-size.ymlwas 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.README.mdsentence ("oncursor-review.yml,groom.yml, andagents-md-integrity.yml…") and theworkflow-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.