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
…he default (BE-5546) (#103) * fix(workflows): make workflows_ref required, guard empty refs, lint the default (BE-5546) cursor-review.yml, groom.yml and agents-md-integrity.yml load their backing scripts at run time from `workflows_ref`, which defaulted to the floating `main` branch. A caller that SHA-pinned `uses:` but omitted the input pinned the workflow while loading MUTABLE scripts into jobs holding write permissions — the pin proved nothing about the code that ran. - Drop `default: main`, set `required: true`, and rewrite the descriptions + header examples to say the input is required and must equal the `uses:` SHA. - Add a fail-fast guard immediately before every assets checkout: `::error` + exit 1 on an empty ref, `::warning` (non-fatal) when it is not a full 40-hex SHA. The guard is mandatory, not belt-and-braces: GitHub does NOT enforce `required: true` for workflow_call inputs, so an omitted input arrives as '' and `actions/checkout` with `ref: ''` silently takes the default branch. - Add .github/workflow-pins/ — a stdlib-only lint (plus tests and test-workflow-pins.yml) that fails if any `workflow_call` workflow declares a `default:` for `workflows_ref`, so the hole cannot come back. Every caller GitHub code search can see (42 files across org and external repos) already passes an explicit 40-hex ref, so no caller breaks and no bump-callers rollout is needed. * fix(workflows): close the review panel's gaps in the workflows_ref lint + guard (BE-5546) Addresses the cursor-review panel's findings on #103. Guard shell (all 12 copies): trim 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 test. Echo only the stripped form: dropping newlines also stops a multi-line value from satisfying the line-oriented grep on one 40-hex line among many, and from smuggling a ::workflow command:: into the log. Lint: the default was only half the hole, so the checker now also requires the empty-ref guard in the same job as every `ref: ${{ inputs.workflows_ref }}` checkout — a NEW job reopened the `ref: ''` fallback with a default-only lint still green, because there was never a default to find. Exempt workflows are not held to it (they still have a default, so '' cannot arrive). Parser: a trailing comment on `on:` no longer reads as an inline trigger list (it silently dropped the file from the lint); the flow-mapping form `workflows_ref: {type: string, default: main}` is caught; the default scan is scoped to the input's own property indent, so a wrapped `description: >-` line starting with `default:` cannot fail a compliant workflow; quoted keys no longer hide a declaration or a default; and a file that USES the input but whose declaration the parser cannot locate is now a hard error rather than a silent skip — "not applicable" and "I could not read this" must not look alike. KNOWN_EXEMPT staleness now also catches an entry whose workflow was renamed or deleted, which would otherwise pre-exempt whatever reused the filename. The list only applies to this repo's own workflows dir, where it means something. Tests: 13 -> 32, plus a second CLI smoke test that an unguarded checkout really exits non-zero. * docs(workflow-pins): correct the KNOWN_EXEMPT staleness comment (BE-5546) The comment still described the narrow original behaviour ("one whose workflow no longer has the default") after the check was widened in review to also catch an entry whose workflow was renamed or deleted — which is the case that would otherwise pre-exempt a future workflow reusing the filename. * fix(workflow-pins): catch the flow-mapping form of the ref checkout (BE-5546) Addresses CodeRabbit's findings on #103. The guard lint anchored on a `ref:` key at line start, so the one-line flow-mapping spelling of the same checkout — with: {repository: Comfy-Org/github-workflows, ref: "${{ inputs.workflows_ref }}"} — was invisible to it: `find_unguarded_ref_checkouts` reported nothing for a job that had no guard at all. `_CONSUMES_RE` had the same anchor, so a file whose only use was written that way also escaped the "the lint is NOT covering this file" hard error, turning the checker's loudest failure into silence. This is the same one-line bypass already barred for `default:`, wearing braces. Both patterns now have a flow-form sibling behind a small helper. The flow pattern stops the value at the entry boundary (`[^,}]`) so `{ref: v1, path: ${{ inputs.workflows_ref }}}` is not misread as a ref checkout — a greedy match there would fail a workflow that never checks out at the input. The guard detector is deliberately left block-only: a guard written in flow style reads as ABSENT, which fails loudly rather than passing an unverified checkout. Verified against the real tree: still the same 4 workflows checked, the same 12 guarded sites, no new hits. 32 -> 36 tests. Also from the same review round: the cursor-review README's knob table still said "All optional, with defaults" after workflows_ref became required with none; the pins test workflow had no concurrency group, so every push to a PR left the superseded lint run burning a runner; and CheckDirTests leaked twelve temp directories per run. * fix(workflow-pins): catch the multi-line spelling of the ref checkout (BE-5546) Self-review of the previous commit, which closed the one-line flow-mapping bypass in the guard detector but left its vertical sibling open. Both ref patterns required `inputs.workflows_ref` to appear on the SAME line as the `ref:` key. YAML does not require that. All four of these are one working checkout to Actions, and every one of them reported an unguarded job as clean: ref: >- ref: | ref: ref: " ${{ … }} ${{ … }} ${{ … }} ${{ … }}" So the lint's whole subject — a job that checks out at the input without the fail-fast guard — was invisible whenever the value was carried on the next line. `_CONSUMES_*` had the same blind spot for the block-scalar form, so a file whose only use was written that way also escaped the "the lint is NOT covering this file" hard error: the loudest failure the checker has, silenced again, this time by a line break rather than a pair of braces. A `ref:` key whose value is empty, a block-scalar indicator (`|`/`>` with any chomping or explicit-indent modifier), or an opening quote now opens a window over the more-indented lines that follow. The window is not itself a finding — a hit needs the input to actually appear in the continuation — so `ref: >-` over a literal SHA stays clean, and the window closes at the first line back at or above the key's indent so a later sibling key is never blamed on the `ref:`. Verified by probe, not by inspection: all four shapes now flag, the flow and block forms still flag, and the four false-positive shapes stay clean. Real tree unchanged — same 4 workflows checked, same 12 guarded sites, no new hits — and the check is still non-vacuous: stripping the guards flags all 12. 44 tests (was 36). actionlint clean; cursor-review 40, agents-md-integrity 18, groom 170, bump-callers 123 green; AGENTS.md still 150 lines. * fix(workflow-pins): catch the commented spelling of the ref checkout (BE-5546) A `#` comment between a mapping key and a value on the next line was unhandled, which reopened the same bypass the previous two commits closed: ref: # the pinned ref ${{ inputs.workflows_ref }} Valid YAML, and the value is exactly the input — a working, unguarded checkout. But the comment left the key line looking like a finished scalar, so `_REF_KEY_OPEN_RE` never opened the continuation window and the job read as having no ref checkout at all. Worse, `_CONSUMES_BLOCK_RE` missed it too, so the "this file uses the input but the lint can't parse its declaration" backstop stayed silent as well: an uncovered workflow passed clean instead of failing loudly. Same for a comment after a block header (`ref: | # …`), which YAML also allows. Teach the four affected patterns that a comment may sit between a key and its value — but not after an opening QUOTE, where `#` is string content. Also let a trailing comment ride on the guard's `WORKFLOWS_REF:` env line. That one was a false FAILURE rather than a bypass: the guard is doing its job, so refusing to see it fails a compliant workflow. This is the opposite trade-off from the flow-form guard, which stays unrecognized on purpose. Six tests, covering both directions — the commented checkout is caught with and without a guard, and a commented LITERAL ref (plus a commented-out `ref:` line, and one whose input feeds a later key) still is not read as a use. * fix(workflow-pins): the env binding is not the guard — verify the rejection (BE-5546) The guard detector keyed on the step's `env:` binding alone (`WORKFLOWS_REF: ${{ inputs.workflows_ref }}`), on the reasoning that nothing else uses that name. But the binding is only how the guard RECEIVES the ref, not the guard: any step that merely handles the value — one that echoes it, or clones with it — carries the same binding, and it marked the whole job guarded, so every later checkout passed unexamined. Reproduced end to end before touching anything: a step whose entire body is `echo "$WORKFLOWS_REF"`, followed by a plainly unguarded `ref: ${{ inputs.workflows_ref }}`, returned zero errors from `check_dir`. The lint's own subject, failing silently. A step now counts as the guard only if it is also seen to REJECT the empty value — an emptiness test and a non-zero exit, both inside that same step. Same probe found the second half: a checkout does not have to name the input. Hoist it to a job-level `env:` — the natural refactor once several steps want it — and `ref: ${{ env.WORKFLOWS_REF }}` read as no ref use at all, dropping the checkouts from coverage AND (before this fix) marking the job guarded on the way past. Names bound to the input inside `env:` blocks are collected first, and a `ref:` reaching one counts as a use. Scoped to `env:` blocks on purpose: the checkout's own `ref:` is a binding too, and collecting it would make `env.ref`/`$ref` anywhere read as the input. Both stay fail-closed: a step the parser cannot resolve is not a guard, so the checkout it precedes is reported rather than passed unverified. Verified non-vacuously rather than by a green run: on the real tree, 0 unguarded across the 12 sites; stripping the env bindings flags all 12; and — the case this commit adds — leaving the bindings in place while neutering only the `-z` test also flags all 12, which the old detector passed. Suite 50 → 57. * fix(workflow-pins): tie the guard's empty test to the ref and its own branch (BE-5546) CodeRabbit caught the near-match left by the previous commit, and it was real — reproduced before touching anything. Requiring "an emptiness test somewhere in the step, a non-zero exit somewhere in the step" let the two halves be about entirely different things: if [ -z "$UNRELATED" ]; then echo "empty"; fi if [ "$UNRELATED" = "blocked" ]; then exit 1; fi ...marked the job guarded. That is a likelier accident than the bare decoy the previous commit fixed: arg validation sitting next to a step that clones at the ref is ordinary shell, not a contrivance. The `-z` must now name the ref — `WORKFLOWS_REF`, or a variable derived from it, since the real guard tests `$REF` assigned through a `printf | tr` strip — and the non-zero exit must sit in THAT test's own branch, ending at the matching `fi`/`else`. The one-liner `[ -z "$REF" ] && exit 1` counts on its own line. Verified by mutation rather than by a green run, since a green run is what the bug produced. Against the real tree, 0 unguarded across the 12 sites, and each of five separate mutations flags all 12: dropping the env binding, neutering the `-z` test, testing the WRONG variable, moving the exit out of the empty branch, and breaking the derivation hop so `REF` no longer carries the value. Suite 57 → 61. * fix(workflow-pins): the guard's condition must be the empty test, not contain it (BE-5546) Second CodeRabbit finding on the same function, and real again — reproduced before changing anything. Tying the `-z` to the ref and the exit to that test's branch still accepted a condition that merely CONTAINS the emptiness test: if [ -z "$REF" ] && [ "$OTHER" = "blocked" ]; then exit 1 fi That marks the job guarded, but it does not fail for every empty ref — empty `REF` with `OTHER` unset falls straight through to the checkout. The emptiness test must now BE the whole condition. A text-level lint cannot evaluate shell, so every compound is rejected as ambiguous — that includes a widening `||`, which is safe in fact but not worth a special case in a detector whose entire bias is fail-closed. Both canonical forms still pass, including the single-line `if …; then exit 1; fi` and `[ -z "$REF" ] && exit 1` (a `run:` on the key's own line is YAML, not part of the condition — that cost a fix of its own). Six mutations now, each flagging all 12 sites while the real tree stays at 0: no binding, no `-z`, wrong variable, exit outside the branch, dead derivation hop, and the ANDed condition this commit adds. Suite 61 → 63. * fix(workflow-pins): the inline branch ends at `fi`, and its exit must be bare (BE-5546) Third CodeRabbit finding on this function, and right again — reproduced first. The single-line path searched everything after `then` for an exit, including past the branch: run: if [ -z "$REF" ]; then echo "missing ref"; fi; exit 1 The branch does not exit, so an empty ref walks on to the checkout. The multiline path had stopped at `fi` since it was written; the inline path had quietly stopped agreeing with it. Reviewing the fix for its own siblings — the failure mode on this PR has been that each fix leaves a weaker copy of the same assumption one layer down — found two more of the same shape, both accepted before this commit: if [ -z "$REF" ]; then [ "$X" = y ] && exit 1; fi # conditional exit [ -z "$REF" ] && echo warn; [ "$X" = y ] && exit 1 # && reaches the echo The multiline path never accepted these: it anchors `exit` at the start of its line, so a conditional exit is not one. Both inline paths now hold to that same rule — the branch ends at `fi`, and the exit must be a bare `exit N` command, the one the `&&` actually reaches. Three decoys rejected, both canonical forms still accepted. Eight mutations of the real workflows now, each flagging all 12 sites with the real tree at 0. Suite 63 → 66. * fix(workflow-pins): the branch's exit must be at its own nesting depth (BE-5546) Fourth CodeRabbit finding, and the exact mirror of the previous commit — which is what makes it worth stating rather than fixing quietly. That commit hardened the INLINE paths to require a bare, directly reachable `exit`, and I checked its siblings for the same drift. I checked the wrong siblings: the multiline path stops at the first `fi`/`else`/`elif`, so an `exit` nested inside an inner `if` was still read as the branch's own. if [ -z "$REF" ]; then if [ "$X" = y ]; then exit 1 fi fi Branch entered on an empty ref, exit conditional on something else, job reads as guarded. Reproduced before changing anything. The scan now tracks nesting and accepts `exit` only at the branch's own depth (`\bif\b` does not match inside `elif`, and a nested one-liner opens and closes on the same line). A test pins the other direction too: an exit AFTER a nested block is still the branch's own and still counts. Nine mutations of the real workflows now, each flagging all 12 sites with the real tree at 0. Suite 66 → 68. * fix(workflow-pins): a guard that can be skipped or soft-failed is not a guard (BE-5546) Found by sweeping the detector systematically rather than waiting for the next review round — four consecutive findings had all been one more spelling of shell, so I went looking somewhere else. Both of these are Actions-level, and every shell check in `is_guard_step` passes them happily because the shell is impeccable: - name: Require a pinned workflows_ref continue-on-error: true # `exit 1` no longer fails the job ... - name: Require a pinned workflows_ref if: github.event_name == 'push' # skipped outright on other events In both cases the checkout runs with an empty ref while the job reads as guarded. `continue-on-error: true` is the more complete bypass of the two: the guard runs, fails exactly as designed, and is ignored. Neither is evaluable in a text lint, so both disqualify the step rather than being assumed benign. `continue-on-error: false` is still a guard; the keys are matched only at the step's own indent, so a shell `if` in the run body is untouched. Eleven mutations of the real workflows now, each flagging all 12 sites with the real tree at 0. Suite 68 → 71. * fix(pr-size): require a pinned workflows_ref, guard empty refs, drain the exemption (BE-5858) (#124) 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. * docs(workflow-pins): document the job_workflow_sha exemption and length-guard shape The README still described the pre-merge state (groom.yml required+guarded like the others, no mention of pr-risk.yml's guard shape). Update it to match what the lint now actually checks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: resolve review feedback Merging main pulled in an unguarded workflows_ref checkout: main independently added the diff-size job's "Load check-pr-size tool" step (checking out ref: ${{ inputs.workflows_ref }}) after this branch forked, with no empty-ref guard ahead of it, since that guard convention is this PR's own contribution. Add the guard step the same way every other ref checkout in this workflow gets it, and update the self-test's expected guarded-site count (15 -> 16) to match. * fix: resolve review feedback is_guard_step treated an empty inline continue-on-error value (comment- only or bare "continue-on-error:") the same as false, so a step whose real value continued on the next, more-indented line - "continue-on-error: # note\n true" - passed as a guard. Actions reads the continuation as the value, so the guard's exit 1 would run exactly as designed and be ignored, letting the job carry on to the checkout anyway. Read the continuation line when the inline value is empty, and add a same-line-true / continuation-false regression test. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.