Repository navigation
Commit c6b3eff
fix(workflows): make workflows_ref required, guard empty refs, lint the 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>1 parent 255187f commit c6b3eff
11 files changed
Lines changed: 2262 additions & 38 deletions
File tree
- .github
- cursor-review
- workflow-pins
- tests
- workflows
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
118 | 118 | | |
119 | 119 | | |
120 | 120 | | |
121 | | - | |
| 121 | + | |
122 | 122 | | |
123 | 123 | | |
124 | 124 | | |
| |||
173 | 173 | | |
174 | 174 | | |
175 | 175 | | |
176 | | - | |
177 | | - | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
178 | 179 | | |
179 | 180 | | |
180 | 181 | | |
| |||
185 | 186 | | |
186 | 187 | | |
187 | 188 | | |
188 | | - | |
| 189 | + | |
189 | 190 | | |
190 | 191 | | |
191 | 192 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
0 commit comments