[PR #193] [MERGED] fix(workflows): resolve asset checkouts from job.workflow_sha, not the empty github.job_workflow_sha (BE-8077) #202

New issue

Closed

opened 2026-10-26 22:37:19 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:19 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/193
Author: @mattmillerai
Created: 8/20/2026
Status: ✅ Merged
Merged: 8/20/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-8077-job-workflow-sha-fallback

📝 Commits (5)

3cf1621 fix(workflows): resolve asset checkouts from job.workflow_sha, not the empty github.job_workflow_sha (BE-8077)
8a506b5 fix(workflow-pins): make the job.workflow_sha guards load-bearing, and fail closed on Unicode whitespace (BE-8077)
32e8159 fix(workflow-pins): track guard STRENGTH, and anchor the self-pin at both ends (BE-8077)
302ff5e fix(workflow-pins): anchor the self-pin to the whole ref value, and carry alias strength (BE-8077)
8e78bbb fix(workflow-pins): drop alias strength as unsound, judge the leading operand (BE-8077)

📊 Changes

12 files changed (+1652 additions, -192 deletions)

View changed files

📝 .github/groom/README.md (+22 -8)
📝 .github/workflow-pins/README.md (+120 -11)
📝 .github/workflow-pins/check_workflow_pins.py (+295 -46)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+601 -18)
📝 .github/workflows/cursor-review.yml (+65 -11)
📝 .github/workflows/groom.yml (+438 -36)
📝 .github/workflows/pr-risk.yml (+27 -19)
📝 AGENTS.md (+13 -5)
📝 CONTRIBUTING.md (+5 -3)
📝 README.md (+17 -8)
📝 docs/callers/README.md (+15 -10)
📝 docs/callers/groom.md (+34 -17)

📄 Description

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

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/193 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/20/2026 **Status:** ✅ Merged **Merged:** 8/20/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8077-job-workflow-sha-fallback` --- ### 📝 Commits (5) - [`3cf1621`](https://github.com/Comfy-Org/github-workflows/commit/3cf1621f1470e976785169139d5d9263d89ff753) fix(workflows): resolve asset checkouts from job.workflow_sha, not the empty github.job_workflow_sha (BE-8077) - [`8a506b5`](https://github.com/Comfy-Org/github-workflows/commit/8a506b53ad6ebc415e5a08d57731910b3bb7258c) fix(workflow-pins): make the job.workflow_sha guards load-bearing, and fail closed on Unicode whitespace (BE-8077) - [`32e8159`](https://github.com/Comfy-Org/github-workflows/commit/32e8159125132e0dd27846c712ab0f7e6f276397) fix(workflow-pins): track guard STRENGTH, and anchor the self-pin at both ends (BE-8077) - [`302ff5e`](https://github.com/Comfy-Org/github-workflows/commit/302ff5edea6f6237734837b69c5f051cfee91c22) fix(workflow-pins): anchor the self-pin to the whole ref value, and carry alias strength (BE-8077) - [`8e78bbb`](https://github.com/Comfy-Org/github-workflows/commit/8e78bbb35367dd2add821e75b94c2f2e4f87e66b) fix(workflow-pins): drop alias strength as unsound, judge the leading operand (BE-8077) ### 📊 Changes **12 files changed** (+1652 additions, -192 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+22 -8) 📝 `.github/workflow-pins/README.md` (+120 -11) 📝 `.github/workflow-pins/check_workflow_pins.py` (+295 -46) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+601 -18) 📝 `.github/workflows/cursor-review.yml` (+65 -11) 📝 `.github/workflows/groom.yml` (+438 -36) 📝 `.github/workflows/pr-risk.yml` (+27 -19) 📝 `AGENTS.md` (+13 -5) 📝 `CONTRIBUTING.md` (+5 -3) 📝 `README.md` (+17 -8) 📝 `docs/callers/README.md` (+15 -10) 📝 `docs/callers/groom.md` (+34 -17) </details> ### 📄 Description ## 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 #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 #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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:19 +00:00