fix(workflows): resolve asset checkouts from job.workflow_sha, not the empty github.job_workflow_sha (BE-8077)

[mattmillerai]

ELI-5

Groom and cursor-review load their prompts and scripts out of this repo at run time. When a caller didn't pin workflows_ref, they were supposed to fall back to "whatever commit the caller's uses: line pointed at". They asked for that commit by the wrong name — github.job_workflow_sha, which isn't a thing GitHub Actions knows about — and Actions quietly hands back an empty string instead of complaining. actions/checkout reads an empty ref as "give me the default branch", so those jobs were checking out whatever happened to be on main at that moment, inside jobs holding ANTHROPIC_API_KEY and a GitHub App token. This PR asks for the commit by its real name (job.workflow_sha) and adds a guard so that if it ever is empty, the run stops loudly instead of silently grabbing main.

What changed

groom.yml — 7 asset checkouts (gate, audit_find, audit_verify, build_select, file, build, build_pr) now read ref: ${{ inputs.workflows_ref || job.workflow_sha }}. Each is preceded by a fail-closed Require a resolvable workflows_ref step that binds the resolved value through env:, then rejects two ways: an emptiness test on the whitespace-stripped value (actions/checkout reads ref through core.getInput, which trims — so a whitespace-only value is empty to it while sailing past a bare -z), and a positive shape test on the raw value (case "$WORKFLOWS_REF" in *[!A-Za-z0-9._/@+-]*)). The second was added in review: POSIX [:space:] is ASCII-only while the JS trim() behind core.getInput also strips U+00A0/U+FEFF, so a ref made purely of those cleared the emptiness test and still reached checkout as ''. The checkout consumes the raw expression, so the value has to be rejected rather than sanitized. Either branch emits an ::error:: naming the input and job.workflow_sha, says a runner older than v2.334.0 is the only way it is empty, and exit 1. The comparison runs under export LC_ALL=C: bracket ranges match by collation order, so under a glibc locale [A-Za-z] also matches accented letters and the negated class quietly stops rejecting them — and the stale self-hosted runner this guard exists for has an unknown locale. The rejection is worded as a policy restriction, not a claim that the ref is invalid to git: the class is deliberately narrower than git's own rules, which permit (, ), ,, % and UTF-8. Shape copied from the BE-5546 guard in cursor-review.yml. The guard is inlined per job rather than factored into a composite action for the reason .github/workflow-pins/README.md already gives: a composite would have to be loaded from the very ref being validated.

cursor-review.yml — the Prior-review ledger job. This job must never fail, because the review matrix needs: it, so it gets no exit 1. A new resolve_ref step validates the ref's shape and emits it untouched into a step output — resolving anything non-ref-shaped to the empty string — and ::warning::s when the result is empty. (It emitted the stripped value until review: tr -d removes interior whitespace too, so a b was silently rewritten into the different-but-real ref ab instead of being rejected.) Load cursor-review assets is gated on if: steps.resolve_ref.outputs.ref != '' and reads ref: ${{ steps.resolve_ref.outputs.ref }}. With the checkout skipped, Build prior-review ledger fails under its existing continue-on-error: true and the existing Ensure ledger artifact exists fallback (id: fallback, if: always()) publishes status=unknown plus its own warning — no new fallback logic was needed. The new step carries timeout-minutes: 2 so it stays under the job timeout, per that job's stated design ("a hung step must trip its OWN timeout, not the job's").

check_workflow_pins.py — the BE-4169 exemption now keys on job.workflow_sha, and the old github.job_workflow_sha spelling is no longer exempt in either place it was honoured (the checkout carve-out and the default: '' carve-out), so the mistake cannot come back with the lint green.

Review hardened three things about that exemption:

• Anchored to the closing }}, and matched against the comment-stripped line. Unanchored it read "contains the fallback" rather than "IS the fallback", so ${{ inputs.workflows_ref || job.workflow_sha || 'main' }} was exempted by both callers of the regex — and that resolves to the mutable default branch in precisely the pre-v2.334.0 case the fallback exists for. check_dir's file-wide any(...) was looser still: a comment merely naming the expression bought the default: '' carve-out.
• _GUARD_BINDING_RE widened to accept the WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }} binding groom's seven guards use. Matching only the bare form meant none of the seven was ever consulted by is_guard_step, and falsely reported a checkout that dropped the fallback while keeping its guard.
• A guard is now required even when the fallback is present. The fallback answers mutability (and still earns the default: '' carve-out); the guard answers emptiness. Exempting the fallback from both meant deleting all seven of groom's guard steps left the lint and its whole suite green — measured, not theorised — while the file's own comments already leaned on them. Unguarded fallback checkouts get their own BE-8077 message rather than the BE-5546 one.

A second round tightened three more edges on that same exemption:

• Guard strength. Recognizing the fallback binding is not the same as treating it as blanket job-wide coverage — see the judgment call below. A guard on the fallback covers fallback checkouts only.
• Anchored at the head as well as the tail. ${{ inputs.override || inputs.workflows_ref || job.workflow_sha }} resolves to whatever the leading operand names, and no runtime guard catches it, because a guard proves non-emptiness rather than immutability.
• The default: '' carve-out is scoped to ref-checkout lines. Asking "does any line self-pin?" of the whole file granted it to any file merely mentioning the expression in code — most sharply the guard steps' own env: binding.

_ENV_ALIAS_RE also learned the fallback spelling, so hoisting that binding to a job-level env: keeps coverage instead of silently dropping to zero, and check_dir now reads the fallback flag the parser determined rather than re-reading the reported line (a block-scalar ref: key never holds the expression, so that path always emitted the wrong message).

Suite is 80 -> 90 tests: both directions of the interlock (delete the guards -> 7 errors; fallback guard + bare checkout -> reported; bare guard + fallback checkout -> clean), the leading- and trailing-operand holes, the alias refactor, a per-step check that all seven groom guards carry their shape test and are byte-identical (deleting all seven case blocks previously left everything green), and a backstop for the ledger checkout, which left the lint's coverage when it moved to a step output.

Docs — AGENTS.md, README.md, CONTRIBUTING.md, docs/callers/README.md, docs/callers/groom.md, .github/groom/README.md, .github/workflow-pins/README.md, plus the workflows_ref input description and header caller example in groom.yml. The "leave it unset" advice is now true rather than aspirational, and the "pin it until groom's checkouts move to job.workflow_sha" instructions are flipped. Wherever the accessor is documented there is now a note that actionlint ≤ 1.7.12 false-positives on job.workflow_sha (its job-context schema predates runner v2.334.0); nothing in this repo's CI runs actionlint, so nothing gates on it.

Sweep

job_workflow_sha appeared on 54 lines across 11 files on main. After this PR: 38 lines across 11 files, and zero of them are live ${{ … }} expressions outside the two lint test fixtures that deliberately assert the old spelling is rejected. Every remaining mention is prose that names the old spelling to contrast it with the correct one, or to explain what a caller pinned at a pre-BE-8077 commit still gets.

That sweep also turned up three stale prose claims in pr-risk.yml (and the matching README.md catalog row) asserting that "the runner does not expose [the commit uses: resolved to] to the workflow" and that the answering value "exists only as an OIDC token claim". That is no longer true — job.workflow_sha answers it and needs no id-token: write. The prose is corrected, but the pr-risk.yml check itself is deliberately NOT wired up: asserting workflows_ref == job.workflow_sha there would fail red on every caller whose two pins have already drifted apart, which is a caller-contract change well outside this ticket. It is now recorded as a wiring gap rather than a runner limitation.

Verification

workflows_ref set (every current fleet caller) is byte-identical: || short-circuits to the same string, the checkout inputs are unchanged, and the only new work is a guard step that passes.

The core premise was confirmed against a real public run rather than asserted — Comfy-Org/comfy-cli groom run 32240568032, groom / Gate job 96029967905 (public repo, read-only log fetch):

• WORKFLOW_SHA: eb4b26c9bc13feea5ae76c4334370158713fb23a — job.workflow_sha resolves to a real 40-hex SHA on ubuntu-latest.
• That SHA equals the workflows_ref the caller passed, which equals the commit its uses: line pins — i.e. job.workflow_sha really is "the commit the caller's uses: resolved to", which is the whole basis of the fallback.
• The existing job.workflow_sha is empty warning did not fire (no ##[warning] annotations in that job at all).

Judgment calls

• The pin lint recognizes the new guard steps, but only for checkouts of matching strength. This took two review rounds and one wrong turn, recorded because the wrong turn is the instructive part. The original reasoning here — that a guard proving inputs.workflows_ref || job.workflow_sha non-empty says nothing about a later bare ref: ${{ inputs.workflows_ref }} checkout in the same job — is correct. Round one widened the binding regex anyway, on the mistaken argument that the fallback made the later checkout safe too; it does not, because that checkout has no fallback of its own and receives '' directly. Round two restored the original insight without giving up what the widening bought: _GUARD_BINDING_RE records which expression each guard validated, and coverage is granted by strength — a bare guard covers every checkout in its job, a fallback guard covers fallback checkouts only. So groom's seven guards are consulted (they were not before, which made them deletable with CI green) and a de-fallbacked checkout under one of them is still correctly reported.
• docs/callers/groom.md's caller snippet keeps its explicit workflows_ref: (now annotated as optional) even though the row and prose say leaving it unset is safe. bump-callers.sh moves that line and the uses: pin in one pass for roster-enrolled callers, and AGENTS.md records double-pinning as the groom convention; changing the recommended snippet's shape is not something this ticket's evidence covers. groom.yml's own header example keeps its # No workflows_ref form, per the ticket.
• No live groom or cursor-review run was triggered. Both spend model credits and mutate GitHub state (issues, PRs, review comments), so they are out of bounds for non-mutating acceptance. The runner-side premise was verified from the existing run log above; the two new failure branches are unreachable on ubuntu-latest by construction.

Not fixed here (why this says Refs, not Closes)

• Bare action pins — two of six fixed in review, four left. actions/checkout@v7 and actions/upload-artifact@v7 in the ledger job are now pinned by full SHA: they sit in the very job this PR rewrites, and a mutable action tag inside the job whose point is loading assets from an immutable commit undercuts the guarantee. Still bare and left alone: two actions/download-artifact@v8 elsewhere in cursor-review.yml (lines 1310, 1556) and actions/checkout@v7 / actions/setup-python@v7 in test-refresh-reviewers.yml. All six are a regression from the Dependabot major-group bump in chore(deps): bump the github-actions-major group with 3 updates #182, which replaced full-SHA pins with floating tags — against AGENTS.md's "pin everything by full commit SHA". The remaining four are untouched by this PR and unrelated to the asset-checkout path; they want a repo-wide sweep that also fixes the Dependabot config, not a rider on this diff.
• AGENTS.md is 273 lines against its own 200-line hard ceiling (python3 .github/agents-md-integrity/check_agents_md.py --root . fails on it today). Pre-existing; this repo does not self-enrol agents-md-integrity.yml, so nothing gates on it. My edits there are net +9 lines and I did not trim the file, because deciding what to cut from a repo's own conventions doc is not this ticket's call.

Both of those sit on artifacts the ticket names (cursor-review.yml's ledger checkout, AGENTS.md), so the closure claim is downgraded: the ticket stays open to carry them.

Unexercised artifacts

The upstream investigation ticket this was written from (BE-8069) and its findings comment are not reachable from this environment, so the evidence in it was taken as given and re-derived independently from the run log cited above rather than read.

Post-merge

No manual dispatch needed: bump-groom-callers.yml and bump-cursor-review-callers.yml both carry push: branches: [main] with paths: covering their own reusable workflow file, so both fleets re-bump themselves on merge. Confirmed by reading both trigger blocks.

Refs BE-8077

Provenance

• Authored by: agent-work loop
• Verified: check_workflow_pins.py --workflows-dir .github/workflows: OK, 10 workflows declare workflows_ref, none with a default, every ref checkout guarded, 0 problems. Unittest: workflow-pins 102 passed (80 on the first push; 90, 98, 102 across three review rounds), groom 365 passed (1 skipped), cursor-review 194 passed, agents-md-integrity 46 passed. yaml.safe_load parses both edited workflows; py_compile clean on both edited Python files.
  Round 4 reverted a round-3 change of mine as unsound, on evidence rather than assertion. Carrying an env: binding's strength to ref: ${{ env.NAME }} was reproduced failing in both directions before the revert: a fallback binding in a guard step's env: scored the sibling checkout (14, True, True) — a guarded self-pin — although step-level env: never reaches a sibling step, so it expands to '' and takes the default branch; and a step-local WORKFLOWS_REF: ${{ inputs.workflows_ref || 'main' }} scored (16, True, True) by inheriting strength from another step's strict binding. cursor-review.yml binds WORKFLOWS_REF both ways today (line 420 with the fallback, six more without), so the cross-talk was live. An alias is judged bare now — fail-closed.
  The refactor that change existed to serve turned out to be impossible, which is why the revert is total rather than a scope fix: job is not available in jobs.<job_id>.env (actionlint 1.7.12 on ${{ job.status }}, chosen to isolate context availability from the job.workflow_sha schema staleness — rejected at job level, accepted at step level), and a step-level env: does not reach a sibling step.
  Also corrected: a round-3 deferral whose stated reason was wrong. ref: ${{ 'main' || inputs.workflows_ref }} linted clean behind a bare guard with no second input declared — || returns the first truthy operand — so the hole was reachable, and the leading-operand check closes it rather than re-wording the excuse.
  Every fix across rounds 3 and 4 is mutation-tested: reverting any one of them fails a named test — value anchoring, quote-aware flow matching, comment-stripped alias binding, the leading-operand check, the block-scalar carve-out, comment stripping, alias strength (now the reverse assertion), and the two cursor-review.yml guards (deleting export LC_ALL=C from resolve_ref alone, and swapping its binding for || 'main'). The quadratic carve-out was measured, not inferred: restoring the per-line comprehension takes the suite from 0.08s to 4.04s.
  Not run: .github/coderabbit-config/tests — fails to import on a missing jsonschema module locally, pre-existing and unrelated (path-filtered to a directory this PR does not touch; its CI job is green). The glibc collation behaviour behind LC_ALL=C cannot be reproduced on macOS libc, which rejects the accented case either way.
• Deviations: The charset accepted by the shape checks is [A-Za-z0-9._/@+-], deliberately narrower than git's own ref rules — a policy restriction that keeps every shell and ::workflow command:: metacharacter out. It covers a 40-hex SHA and any branch/tag this input is documented to carry; a legal-but-excluded ref such as feat(groom)/x fails closed loudly, and the message says it is policy rather than validity. pr-risk.yml's current-pin equality check is documented as now-possible but deliberately left unwired (caller-contract change, out of scope). Dropping workflows_ref from ci-groom.yml to give the fallback CI coverage was considered and declined: that caller documents its uses: and workflows_ref pins as byte-identical and bump-callers.sh rewrites both in one pass. No live groom/cursor-review run was triggered — both are credit-spending and state-mutating.
  Three reviewer findings are deferred to follow-up tickets, each with a full plan rather than a note: teaching the lint to follow a ref: through a step output (a new detection capability, not a correction — cursor-review.yml:456 is covered only by a per-site grep today); three spellings that leave a checkout invisible (flow-form env:, bracket access, env-to-env chains — none used in this tree, and the _CONSUMES_* backstop cannot catch them because it only fires when the input declaration is unparseable); and block-scalar blindness, where a run: script can forge a guard binding and mark its whole job guarded. All three are pre-existing and unreached by anything in the tree; closing the last one means teaching the parser block-scalar extents, which touches every scan in the module.
  Two bare actions/download-artifact@v8 refs in cursor-review.yml were pinned to the SHA already written out elsewhere in that file, finishing the sweep this PR started there. The two in test-refresh-reviewers.yml are left: a different file, untouched here, wanting the repo-wide sweep that also fixes the Dependabot config behind the chore(deps): bump the github-actions-major group with 3 updates #182 regression.
  AGENTS.md is over its own 200-line ceiling. Pre-existing (273 on main, and this repo does not self-enrol agents-md-integrity.yml, so nothing gates on it); not trimmed here, because deciding what to cut from a repo's conventions doc is not this ticket's call.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your current included review allowance is based on your included PR review attempts over the past 7 days.

Next review available in: 24 minutes

Limit details: You’ve used the included review currently available. Your 95 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 54e81624-4edf-4c3d-aa38-18caa1038102

📥 Commits

Reviewing files that changed from the base of the PR and between b09780e and 8e78bbb.

📒 Files selected for processing (12)

• .github/groom/README.md
• .github/workflow-pins/README.md
• .github/workflow-pins/check_workflow_pins.py
• .github/workflow-pins/tests/test_check_workflow_pins.py
• .github/workflows/cursor-review.yml
• .github/workflows/groom.yml
• .github/workflows/pr-risk.yml
• AGENTS.md
• CONTRIBUTING.md
• README.md
• docs/callers/README.md
• docs/callers/groom.md

---

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

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 8 finding(s).

Severity	Count
🟠 High	1
🟡 Medium	4
🟢 Low	3

Panel: 8/8 reviewers contributed findings.

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Round 2 — ledger: 8 prior finding(s) across 1 round(s) (0 never answered).

Found 8 finding(s).

Severity	Count
🟠 High	1
🟡 Medium	3
🟢 Low	4

Panel: 8/8 reviewers contributed findings.

[mattmillerai]

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

• BE-8127 — workflow-pins: follow a checkout ref: through a step output so resolve-then-consume jobs stay linted — 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:

• workflow-pins: follow a checkout ref: through a step output so resolve-then-consume jobs stay linted — no reachability block in the proposal

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Round 3 — ledger: 16 prior finding(s) across 2 round(s) (0 never answered).

Found 10 finding(s).

Severity	Count
🟡 Medium	3
🟢 Low	6
⚪ Nit	1

Panel: 8/8 reviewers contributed findings.

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Round 4 — ledger: 18 prior finding(s) across 2 round(s) (0 never answered).

Found 10 finding(s).

Severity	Count
🟠 High	1
🟡 Medium	4
🟢 Low	5

Panel: 8/8 reviewers contributed findings.

[mattmillerai]

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

• BE-8140 — workflow-pins: follow a ref: through a step output so resolve-then-consume jobs are linted — filed as agent-spike (premise unverified)
• BE-8141 — workflow-pins: close three spellings that make a ref checkout invisible to the lint — filed as agent-spike (premise unverified)
• BE-8142 — workflow-pins: skip block-scalar bodies so a run: script cannot forge a guard or an alias — 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:

• workflow-pins: follow a ref: through a step output so resolve-then-consume jobs are linted — no reachability block in the proposal
• workflow-pins: close three spellings that make a ref checkout invisible to the lint — no reachability block in the proposal
• workflow-pins: skip block-scalar bodies so a run: script cannot forge a guard or an alias — no reachability block in the proposal