Skip to content

fix(pr-size): require a pinned workflows_ref, guard empty refs, drain the exemption (BE-5858) - #124

Merged
mattmillerai merged 1 commit into
matt/be-5546-require-workflows-reffrom
matt/be-5858-pr-size-workflows-ref
Aug 4, 2026
Merged

mattmillerai merged 1 commit into
matt/be-5546-require-workflows-reffrom
matt/be-5858-pr-size-workflows-ref

Conversation

@mattmillerai

Copy link
Copy Markdown
Contributor

STACKED — merging lands on matt/be-5546-require-workflows-ref (PR #103, same author), NOT main. This PR is based on #103's branch because the guard step and the whole .github/workflow-pins/ lint it edits exist only there. Merge #103 first; GitHub then retargets this PR to main. Do not treat this as "ready to merge" into main on its own.

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 to main. So you could pin the workflow to a frozen SHA and still get a tool built from whatever landed on main five 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.yml was the last of the four reusable workflows still carrying a workflows_ref default — this closes the BE-5546 pin-integrity sweep.

  • Input declaration — required: true, default: main deleted, description rewritten to mirror the wording fix(workflows): make workflows_ref required, guard empty refs, lint the default (BE-5546) #103 used in cursor-review.yml / groom.yml / agents-md-integrity.yml.
  • Header caller example — now says the ref is REQUIRED and must match the uses: SHA, instead of "defaults to main".
  • Runtime guard — the Require a pinned workflows_ref step, copied byte-for-byte from cursor-review.yml (verified programmatically: both extract to an identical 1425-character block), inserted immediately before the Load check-pr-size tool checkout. It is needed because GitHub does not enforce required: true for workflow_call inputs: an omitted input arrives as '', and actions/checkout with ref: '' silently takes the default branch. The step takes the value via env: (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_EXEMPT drained — "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.
  • Docs synced — the root README.md usage note and .github/workflow-pins/README.md both named the three fixed workflows / the pr-size exemption and would otherwise have gone stale.

The guard is added only to the pr-size job. The comment job never consumes workflows_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_CALLERS has exactly one entry, and it resolves to a live repo.
  • That caller already passes workflows_ref explicitly, SHA-matched to its own uses: pin (same 40-hex SHA on both lines) — so it is unaffected by the removal.
  • The pr-size check is not a required status check on that repo's default branch (the branch carries no protection rule at all).
  • A global GitHub code search for github-workflows/.github/workflows/pr-size.yml returns 3 hits: this repo's own header example, this repo's bump-callers test 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_CALLERS and the code-search API.

No caller change is needed. After this merges, bump-pr-size-callers.yml bumps the caller's uses: SHA and workflows_ref in lockstep — confirmed in bump-callers.sh, whose SHA_ADDR matches /github-workflows|workflows_ref/, so both pins move together.

Verification

  • python3 .github/workflow-pins/check_workflow_pins.py → green: "4 workflow(s) declare workflows_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.
  • Other suites (cursor-review 40, agents-md-integrity 18, groom 170, bump-callers 123 + shellcheck) → all green.
  • Grep sanity: no default: main in pr-size.yml; exactly one Require a pinned workflows_ref step, at line 166, ahead of Load check-pr-size tool at line 190.

Judgment calls

  1. Stacked instead of blocked. The ticket's precondition said to hand this back if fix(workflows): make workflows_ref required, guard empty refs, lint the default (BE-5546) #103 had not merged. fix(workflows): make workflows_ref required, guard empty refs, lint the default (BE-5546) #103 is open (CI green, but mergeable: CONFLICTING against main — 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.
  2. Two test tweaks beyond the ticket's list. pr-size.yml was 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.
  3. Two doc lines beyond the ticket's list. Removing the exemption made the root README.md sentence ("on cursor-review.yml, groom.yml, and agents-md-integrity.yml…") and the workflow-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.

… 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.
@mattmillerai mattmillerai added cursor-review Multi-model cursor review agent-coded Authored by the agent-work loop labels Aug 4, 2026
@mattmillerai
mattmillerai marked this pull request as ready for review August 4, 2026 17:43
@coderabbitai

coderabbitai Bot commented Aug 4, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: cb6ca929-138b-4cae-9860-88c34ed3bcbf

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 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.

Comment thread .github/workflows/pr-size.yml
Comment thread .github/workflows/pr-size.yml
Comment thread .github/workflows/pr-size.yml
Comment thread .github/workflows/pr-size.yml
@mattmillerai

Copy link
Copy Markdown
Contributor Author

🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:

  • BE-6505 — Sync docs/callers/ for the workflows_ref required-no-default sweep (BE-5546/BE-5858) — filed as agent-spike (premise unverified)
  • BE-6506 — Make the workflows_ref guard hard-fail on a non-40-hex ref across all four reusable workflows — filed as agent-spike (premise unverified)

The following carry agent-spike instead of agent-ok because their reachability claim was not backed by evidence (BE-5378) — the claim is investigated before any code is written, and "the premise does not hold" is a valid, successful outcome:

  • Sync docs/callers/ for the workflows_ref required-no-default sweep (BE-5546/BE-5858) — no reachability block in the proposal
  • Make the workflows_ref guard hard-fail on a non-40-hex ref across all four reusable workflows — no reachability block in the proposal

@mattmillerai

Copy link
Copy Markdown
Contributor Author

Merging unreviewed. Blast radius: check-pr-size reusable only — makes workflows_ref a required pinned input, guards empty refs, and drains the exemption path. Why safe without review: every downstream caller pins this repo by full 40-hex commit SHA, so no consumer changes behavior until a bump PR moves a pin. Full cursor-review panel plus Socket/CodeRabbit green on this head. Note for the follow-ups: #103 and #118 touch the same workflows_ref contract and will need a rebase on top of this — I'll reconcile them rather than land overlapping validation twice.

@mattmillerai
mattmillerai merged commit 8c0b682 into matt/be-5546-require-workflows-ref Aug 4, 2026
27 checks passed
@mattmillerai
mattmillerai deleted the matt/be-5858-pr-size-workflows-ref branch August 4, 2026 21:17
mattmillerai added a commit that referenced this pull request Aug 12, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-coded Authored by the agent-work loop cursor-review Multi-model cursor review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants