diff --git a/.github/groom/README.md b/.github/groom/README.md index f607b49b..80efd430 100644 --- a/.github/groom/README.md +++ b/.github/groom/README.md @@ -492,12 +492,15 @@ from `$GROOM_ASSETS`. It gets its own sparse checkout at the commit this That matters twice over, because this is executable supply chain rather than a prompt: -- `workflows_ref` is `required: false` and defaults to the mutable `main`, so a - caller that SHA-pins `uses:` but omits it would have *what executes* inside the - three agent jobs tracking a branch tip, while the sandbox flags those jobs pass - stay frozen at the pinned SHA. Reading from `job_workflow_sha` keeps the CLI - version and the flags that depend on it on the same commit, so SHA-pinning - `uses:` alone fully pins the CLI. +- `workflows_ref` is `required: false`, and a caller may legitimately point it at + a mutable BRANCH (the documented brief-testing override). A caller that SHA-pins + `uses:` and does that would have *what executes* inside the three agent jobs + tracking a branch tip, while the sandbox flags those jobs pass stay frozen at + the pinned SHA. Reading from `job.workflow_sha` keeps the CLI version and the + flags that depend on it on the same commit, so SHA-pinning `uses:` alone fully + pins the CLI. (Since BE-8077 the asset checkouts fall back to + `job.workflow_sha` too when the input is *omitted*, so on that path the two + agree by construction — this step simply never depended on them agreeing.) - The resolve step fails **closed** on a missing manifest. Read from `workflows_ref`, this repo's documented split-pin state (Dependabot moves `uses:` and leaves `workflows_ref:` behind — see @@ -506,12 +509,23 @@ prompt: `groom.yml` reads the manifest also ships it. `job.workflow_sha`, **not** `github.job_workflow_sha` — the latter is the -spelling everyone reaches for and it expands to an empty string inside a +spelling everyone reaches for (BE-4169's asset checkouts reached for it, and +BE-8077 moved all seven onto this one) and it expands to an empty string inside a reusable-workflow job, which Actions does not treat as an error. The populated accessor is the `job`-context one added in runner v2.334.0 (April 2026). All groom jobs are `ubuntu-latest`, so it is always available; the resolve step still re-checks it and emits a `::warning::` if it is ever empty, because the -failure mode is otherwise invisible. +failure mode is otherwise invisible. (`actionlint` ≤ 1.7.12 flags +`job.workflow_sha` as an undefined property — its `job`-context schema predates +that runner release. It is a false positive, and nothing in this repo's CI runs +actionlint, so nothing gates on it.) + +The seven asset checkouts differ from this step in one way: they pair the +fallback with a fail-**closed** `Require a resolvable workflows_ref` guard step +that `::error::`s and exits non-zero when both `inputs.workflows_ref` and +`job.workflow_sha` come back empty. This step warns instead, because a degraded +CLI pin is not worth a groom outage and the pin is still validated; a +default-branch *brief* checkout inside a job holding `ANTHROPIC_API_KEY` is. That resolve step is the **last** step in `gate` and runs only when `should_run == 'true'` (as does the checkout that feeds it). It is the one diff --git a/.github/workflow-pins/README.md b/.github/workflow-pins/README.md index 50a18275..da314258 100644 --- a/.github/workflow-pins/README.md +++ b/.github/workflow-pins/README.md @@ -9,8 +9,9 @@ repo's own workflow files. `workflows_ref` input, or (2) checks out at `ref: ${{ inputs.workflows_ref }}` in a job that does not run the empty-ref guard first — recognizing the canonical `-z` guard and the length/charset shape `pr-risk.yml` uses, and - exempting the `github.job_workflow_sha` self-pin `groom.yml` uses instead of - a guard (see below). Text-level parsing (this repo is stdlib-only — no + and treating the `job.workflow_sha` self-pin `groom.yml` uses as an exemption + from the *mutable-default* half only — it still has to carry a guard (see + below). Text-level parsing (this repo is stdlib-only — no PyYAML), the same constraint `bump-callers.sh` works under. - **`tests/`** — `unittest` suite, run by [`test-workflow-pins.yml`](../workflows/test-workflow-pins.yml) along with a @@ -43,15 +44,123 @@ check alone already rejects an empty ref (length 0), and `||` only ever widens what a condition rejects, so one qualifying branch is enough regardless of what it is OR'd with. -`groom.yml` is the one workflow that skips the guard entirely (BE-4169): its -`workflows_ref` defaults to `''`, and every checkout falls back to -`${{ inputs.workflows_ref || github.job_workflow_sha }}` — the exact commit -THIS reusable workflow was itself resolved from via the caller's `uses:` pin. -That value can never be empty or mutable, so an omitted input self-pins -instead of reaching a floating branch — the same guarantee the guard buys, -bought without needing one. The lint recognizes this LITERAL fallback -expression only; anything else OR'd in (a branch, a tag, another input) is the -same hole wearing a different hat and stays covered by both checks. +`groom.yml` is the one workflow whose `workflows_ref` is not required (BE-4169): +it defaults to `''`, and every checkout falls back to +`${{ inputs.workflows_ref || job.workflow_sha }}` — the exact commit THIS +reusable workflow was itself resolved from via the caller's `uses:` pin. That +value is never mutable, so an omitted input self-pins instead of reaching a +floating branch, which is what the lint's exemption is about. The lint +recognizes this LITERAL fallback expression only; anything else OR'd in (a +branch, a tag, another input) is the same hole wearing a different hat and stays +covered by both checks. + +**The old `github.job_workflow_sha` spelling is now FLAGGED, not exempt +(BE-8077).** That is an OIDC token claim, not a property of the `github` +context, so Actions expanded it to `''` and `actions/checkout` read `ref: ''` as +this repo's default branch — the exact hole this lint exists to close, blessed +by the lint itself. `job.workflow_sha` is the `job`-context accessor added in +runner v2.334.0 (April 2026), and because it *is* empty on an older runner, +`groom.yml` no longer skips the guard: each of those seven jobs runs a +fail-closed `Require a resolvable workflows_ref` step ahead of its checkout, and +`cursor-review.yml`'s never-fail ledger job resolves the ref in a step that +warns and skips the checkout instead. + +**The lint enforces that split, rather than trusting the prose.** The fallback +answers *mutability*; the guard answers *emptiness*; a checkout using the +fallback is exempt from the first check and still held to the second. Until +BE-8077 the fallback was exempt from **both**, so deleting all seven of +`groom.yml`'s guard steps left this lint — and its whole suite — green, while +the paragraph above already leaned on them. + +Three things make that enforceable, and each was a hole on its own: + +- **The guard-step detector knows both bindings, and keeps them apart.** It + accepts `WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }}` as + well as the bare `${{ inputs.workflows_ref }}`, because matching only the bare + form meant none of `groom.yml`'s seven guards was ever consulted. But the two + are **not** equivalent, and treating either as blanket job-wide coverage is a + live hole: a guard on the fallback proves only that the *OR expression* is + non-empty, so with the input omitted it passes on `job.workflow_sha` while a + sibling `ref: ${{ inputs.workflows_ref }}` in the same job still gets `''`. + So the lint records each guard's **strength** and requires the checkout's own + `ref:` to be no weaker: a bare guard covers everything, a fallback guard + covers fallback checkouts only. +- **The fallback pattern is anchored to the whole YAML value**, not merely to + the `${{` … `}}` interpolation, and matched against the comment-stripped line. + Anchoring the interpolation alone still accepted a mutable ref, and the + runtime guard cannot catch one (a guard proves non-emptiness, not + immutability). All of these are *not* self-pins: + `${{ inputs.workflows_ref || job.workflow_sha || 'main' }}` (resolves to the + default branch in exactly the pre-v2.334.0 case the fallback exists for), + `${{ inputs.override || inputs.workflows_ref || job.workflow_sha }}` + (resolves to whatever the leading operand names), + `refs/heads/${{ … }}` and `${{ inputs.override }}${{ … }}` (buried in a + concatenation), and a flow mapping whose *sibling* entry — not its `ref:` — + carries the fallback. A comment merely *naming* the expression buys nothing + either. Three matchers do this, mirroring the `_REF_USE_*` trio: block, flow + (bounded at the entry boundary, exactly as `_REF_USE_FLOW_RE` bounds its own + value), and a continuation line that *is* the expression. +- **The `default: ''` carve-out is scoped to actual ref checkouts**, and it asks + the *parser* rather than re-reading each line. Asking "does any line + self-pin?" of the whole file granted it to any file that merely mentions the + expression in code — most sharply the guard steps' own `env:` binding — so a + file whose checkouts all read the bare `ref: ${{ inputs.workflows_ref }}` + bought an empty default it does not self-pin against. Re-deriving it per line + went too far the other way: no single line satisfies both halves of the + block-scalar spelling (`ref: >-` with the expression below it), so a file that + genuinely self-pins that way lost the carve-out and got BE-5546's "delete the + default" while its checkouts got BE-8077's "the fallback IS recognized". It + was also quadratic — a full alias scan per line, on a 3,000-line groom.yml. + +**`env:` aliases are recognized as ref USES, never as self-pins.** The two +questions fail in opposite directions, so they get opposite answers: + +- *Does this `env:` binding reach the input?* — `env_aliases` matches **any** + value mentioning `inputs.workflows_ref` (comment-stripped, so prose in an + unrelated value cannot bind a name). Enumerating blessed spellings made an + unrecognized one fail *open*: `WORKFLOWS_REF: ${{ inputs.workflows_ref || + 'main' }}` registered no alias, so `ref: ${{ env.WORKFLOWS_REF }}` read as no + ref use at all and left the lint entirely, carrying the exact mutable fallback + the lint exists to catch. Over-approximating here can only ever *demand* a + guard. +- *Is this ref the immutable self-pin?* — judged from the **literal expression + on the checkout line**. An alias never qualifies, so `ref: ${{ env.NAME }}` + always needs a **bare** guard. + +Carrying an alias binding's strength to the checkout was tried and reverted. +`env:` is scoped per step and per job and it *shadows*, while these scans are +file-wide, so a file-wide "names bound to the fallback" set granted the +exemption at checkouts the binding never reaches — in both directions. A +binding in a *guard step's* `env:` is invisible to the sibling checkout step at +run time, so `ref: ${{ env.NAME }}` there expands to `''` and takes the default +branch while scoring as a guarded self-pin; and a step-local +`WORKFLOWS_REF: ${{ inputs.workflows_ref || 'main' }}` inherited fallback +strength from any *other* step binding that name strictly. `cursor-review.yml` +binds `WORKFLOWS_REF` both ways today (line 420 with the fallback, six more +without), so that cross-talk was not hypothetical. + +**The fallback cannot be hoisted to a shared `env:` at all**, so there is no +refactor left for strength propagation to serve. The `job` context is not +available in `jobs..env` — verified with `actionlint` 1.7.12 on +`${{ job.status }}`, a `job` property its schema *does* know, which isolates +context availability from the `job.workflow_sha` staleness noted below: +rejected at job level with *"context \"job\" is not allowed here"*, accepted in +a step's `env:`. And a step-level `env:` does not reach a sibling step. So +groom.yml's seven duplicated bindings are duplicated of necessity. A job-level +`env:` may still bind the **bare** input; the checkouts below it are then +required to carry a bare guard, which is the correct answer. +(`actionlint` ≤ 1.7.12 also false-positives on `job.workflow_sha` itself, whose +`job`-context schema predates runner v2.334.0; no CI here runs it.) + +**The leading operand has to reach the input.** A guard proves the *input* is +non-empty; it says nothing about an expression that never reaches the input. +GitHub's `||` returns the first **truthy** operand, so +`ref: ${{ 'main' || inputs.workflows_ref }}` mentions the input — making it a +ref use that clears the guard — while resolving to a mutable branch on every +runner, with no second input declaration involved. `check_dir` cannot see it +either; it reads only `workflows_ref`'s own `default:`. So the first `||` +operand of the ref expression must reach the input, or no guard in the job +covers that checkout. The guard is copied inline into each consuming job rather than factored into a composite action **on purpose**: a composite would have to be loaded with diff --git a/.github/workflow-pins/check_workflow_pins.py b/.github/workflow-pins/check_workflow_pins.py index 0aa0dc64..88468b98 100644 --- a/.github/workflow-pins/check_workflow_pins.py +++ b/.github/workflow-pins/check_workflow_pins.py @@ -94,9 +94,19 @@ # which fails the lint loudly instead of passing a checkout it never verified. # A trailing comment IS tolerated — unlike the flow form that is a real guard # doing its job, so rejecting it would fail a compliant workflow, not catch one. +# +# BOTH bindings are recognized, but they are NOT equivalent, and the `fallback` +# group is what keeps them apart. A guard on the bare input proves the INPUT is +# non-empty; a guard on `inputs.workflows_ref || job.workflow_sha` proves only +# that the OR EXPRESSION is. Treating the second as blanket job-wide coverage is +# a live hole: with the input omitted the guard passes on `job.workflow_sha` +# while a sibling `ref: ${{ inputs.workflows_ref }}` in the same job still gets +# '' and checkout takes the default branch. `find_unguarded_ref_checkouts` +# therefore records the STRENGTH of each guard and requires the checkout's own +# `ref:` to be no weaker. _GUARD_BINDING_RE = re.compile( - r"""^\s*(['"]?)WORKFLOWS_REF\1\s*:\s*(['"]?)\$\{\{\s*inputs\.workflows_ref\s*\}\}\2""" - r"""[^\S\n]*(?:#.*)?$""" + r"""^\s*(['"]?)WORKFLOWS_REF\1\s*:\s*(['"]?)\$\{\{\s*inputs\.workflows_ref\s*""" + r"""(?P\|\|\s*job\.workflow_sha\s*)?\}\}\2[^\S\n]*(?:#.*)?$""" ) # …but the binding alone is NOT the guard, it is only how the guard receives the # value. Keying on it by itself made ANY step that merely handles the ref — one @@ -262,9 +272,23 @@ def _ref_derived_names(body): # `ref: ${{ env.WORKFLOWS_REF }}` below reads as no ref use at all, dropping # the very checkouts this lint exists to cover. So the names bound to the input # are collected first, and a `ref:` reaching one of them counts as a use. +# ANY `env:` value that MENTIONS the input registers a binding — not only the +# bare input or the `|| job.workflow_sha` fallback. Enumerating blessed +# spellings made an unrecognized one fail OPEN, which is the wrong direction for +# a detector whose job is absence: `WORKFLOWS_REF: ${{ inputs.workflows_ref || +# 'main' }}` registered no alias, so `ref: ${{ env.WORKFLOWS_REF }}` read as no +# ref use at all and that checkout left the lint entirely — carrying the exact +# mutable fallback the lint exists to catch. Registering it instead hands the +# checkout to the guard and mutability checks, which is the posture the module +# states everywhere else (a flow-form guard "reads as ABSENT, which fails the +# lint loudly"). It answers ONLY "does this name reach the input" — never "is +# it strong": an alias never earns the self-pin exemption (see `_FALLBACK_RES`), +# because `env:` shadows and is scoped per step/job while this scan is +# file-wide. Matched against the comment-STRIPPED child, or this becomes the one +# place in the module reading a comment as code: `ASSETS: _dir # checked out at +# inputs.workflows_ref` would bind `ASSETS` and fail a compliant workflow. _ENV_ALIAS_RE = re.compile( - r"""^\s*(['"]?)([A-Za-z_]\w*)\1\s*:[^\S\n]*""" - r"""(['"]?)\$\{\{\s*inputs\.%s\s*\}\}\3[^\S\n]*(?:#.*)?$""" % INPUT_NAME + r"""^\s*(['"]?)([A-Za-z_]\w*)\1\s*:[^\S\n]*.*inputs\.%s\b""" % INPUT_NAME ) # Scoped to `env:` blocks, not every mapping key bound to the input: the # checkout's own `ref: ${{ inputs.workflows_ref }}` is such a binding too, and @@ -280,35 +304,159 @@ def _ref_derived_names(body): _COMMENT_RE = re.compile(r"(?:^|\s)#.*$") # The BE-4169 self-pinning fallback (see its use in check_dir): the LITERAL -# expression `inputs.workflows_ref || github.job_workflow_sha`, no other -# spelling. `github.job_workflow_sha` is the exact commit THIS reusable -# workflow was resolved from — never empty, never mutable — so an input -# defaulted to `''` and OR'd with it this way can't reach checkout empty or -# on a moving ref, the same guarantee `required: true` + the runtime guard -# buys, bought a different way. -_JOB_WORKFLOW_SHA_FALLBACK_RE = re.compile( - r"""inputs\.%s\s*\|\|\s*github\.job_workflow_sha\b""" % INPUT_NAME +# expression `inputs.workflows_ref || job.workflow_sha`, no other spelling. +# `job.workflow_sha` is the exact commit THIS reusable workflow was resolved +# from via the caller's `uses:` pin — never mutable — so an input defaulted to +# `''` and OR'd with it this way can't reach checkout on a moving ref, the same +# guarantee `required: true` + the runtime guard buys, bought a different way. +# +# It is `job.workflow_sha` and NOT `github.job_workflow_sha` (BE-8077). The +# latter is what this regex accepted until then, and it is a trap: it is an +# OIDC token CLAIM, not a property of the `github` context, so Actions expands +# it to '' and `actions/checkout` reads `ref: ''` as "the default branch" — the +# precise hole this lint exists to close, blessed by the lint itself. The +# populated accessor is the `job`-context one added in runner v2.334.0 (Apr +# 2026). Anything still spelling it the old way is now FLAGGED, not exempt. +# +# `job.workflow_sha` is only never-empty on a current runner, so unlike the +# BE-4169 story this expression is not self-sufficient: groom.yml pairs every +# one of these checkouts with a fail-closed empty-ref guard step, and +# cursor-review.yml's ledger job (which must never fail) resolves it in a step +# that warns and skips the checkout. This exemption is about the ref not being +# MUTABLE; those runtime guards cover the empty case. +# ANCHORED TO THE WHOLE YAML VALUE, not just to the interpolation, and matched +# against the comment-stripped line. Unanchored, this read "contains the +# fallback" rather than "IS the fallback", and every boundary matters because an +# extra operand — or an extra character — on either side reintroduces a mutable +# ref: `${{ inputs.workflows_ref || job.workflow_sha || 'main' }}` resolves to +# the default branch in exactly the pre-v2.334.0 case the fallback exists for, +# `${{ inputs.override || inputs.workflows_ref || job.workflow_sha }}` resolves +# to whatever the LEADING operand names, and `refs/heads/${{ … }}` / +# `${{ inputs.override }}${{ … }}` bury it in a concatenation. The runtime guard +# cannot catch any of them: a guard proves non-emptiness, not immutability. +# +# Three spellings, mirroring the `_REF_USE_*` trio, because a `${{ … }}` sitting +# somewhere on the line is not the same as being the REF's value: +# block `ref: ${{ … }}` — the value is the whole rest of the line +# flow `{…, ref: ${{ … }}, …}` — bounded at the entry boundary, exactly +# as `_REF_USE_FLOW_RE` bounds its own +# value, so a SIBLING entry carrying the +# fallback cannot be misread as the ref +# cont. a continuation line that IS the expression, for `ref: >-` and friends +_FALLBACK_BODY = r"""inputs\.%s\s*\|\|\s*job\.workflow_sha""" % INPUT_NAME + + +# The self-pin is judged from the LITERAL expression on the checkout line, and +# an `env:` alias is deliberately NOT accepted as a spelling of it. +# +# Carrying an alias binding's strength to `ref: ${{ env.NAME }}` was tried and +# reverted: `env:` is scoped per step and per job and it SHADOWS, while these +# scans are file-wide, so a file-wide set of "names bound to the fallback" +# grants the exemption at checkouts the binding never reaches. Both directions +# were live. A binding sitting in a GUARD step's `env:` is invisible to the +# sibling checkout step at run time, so `ref: ${{ env.NAME }}` there expands to +# '' and takes the default branch — while scoring as a guarded self-pin. And a +# step-local `WORKFLOWS_REF: ${{ inputs.workflows_ref || 'main' }}` inherited +# fallback strength from any OTHER step binding that name strictly; +# cursor-review.yml binds `WORKFLOWS_REF` both ways today (line 420 with the +# fallback, six more without it), so that cross-talk was not hypothetical. +# +# There is also no valid refactor left to serve. The fallback cannot be hoisted +# to a shared `env:` at all: the `job` context is not available in +# `jobs..env` (actionlint: `context "job" is not allowed here`), and a +# step-level `env:` does not reach a sibling step. So groom.yml's seven +# duplicated bindings are duplicated of necessity, and a `ref:` reaching the +# input through an alias is required to carry a BARE guard — the fail-closed +# answer, and the one the module's stated posture asks for. +_FALLBACK_RES = ( + re.compile( + r"""^\s*(?P['"]?)ref(?P=k)\s*:[^\S\n]*""" + r"""(?P['"]?)\$\{\{\s*%s\s*\}\}(?P=q)[^\S\n]*$""" % _FALLBACK_BODY + ), + re.compile( + r"""[{,]\s*(?P['"]?)ref(?P=k)\s*:[^\S\n]*""" + r"""(?P['"]?)\$\{\{\s*%s\s*\}\}(?P=q)[^\S\n]*(?=[,}])""" % _FALLBACK_BODY + ), + re.compile( + r"""^\s*(?P['"]?)\$\{\{\s*%s\s*\}\}(?P=q)[^\S\n]*$""" % _FALLBACK_BODY + ), ) +def _outside_quotes(line, pos): + """True when `pos` sits outside any quoted scalar on `line`.""" + quote = None + for ch in line[:pos]: + if quote: + if ch == quote: + quote = None + elif ch in "'\"": + quote = ch + return quote is None + + +# The `${{ … }}` body of the FIRST interpolation on a line, and its leading +# `||` operand. Splitting on `||` is enough here: Actions has no operator that +# can contain one, and a `||` inside a quoted literal would still leave a +# leading operand that does not reach the input, which is the answer we want. +_INTERPOLATION_RE = re.compile(r"""\$\{\{(.*?)\}\}""", re.S) + + +def _leading_operand_reaches_input(line, mention_re): + """True when the ref expression's first `||` operand reaches the input. + + Lines with no interpolation at all (a literal `ref: main` never reaches + here — it is not a ref use) answer True, so this only ever narrows. + """ + match = _INTERPOLATION_RE.search(_strip_comment(line)) + if not match: + return True + return bool(mention_re.search(match.group(1).split("||")[0])) + + +def _pins_to_job_workflow_sha(line): + """True when `line`'s CODE — not its comments — IS the BE-4169 fallback ref. + + Comments are stripped first because prose merely NAMING the expression + (this repo's workflows discuss it at length) must not buy the `default: ''` + carve-out for a file where no checkout actually uses it. + """ + block_re, flow_re, cont_re = _FALLBACK_RES + code = _strip_comment(line) + if block_re.match(code) or cont_re.match(code): + return True + # The flow form `search`es mid-line, so its `[{,]` boundary can be met by a + # comma INSIDE a quoted sibling scalar — planting a decoy `ref:` that scores + # the line a self-pin while the real `ref:` on it is bare. Require the entry + # boundary to be real YAML punctuation, not string content. + match = flow_re.search(code) + return bool(match and _outside_quotes(code, match.start())) + + def _default_value(line): """The comment-stripped RHS of a `default:` key line.""" return _strip_comment(line.split(":", 1)[1]) -def env_aliases(lines): - """Names bound to the input by an `env:` mapping, e.g. `WORKFLOWS_REF`.""" +def _env_bindings(lines, pattern): + """Names bound inside an `env:` block by `pattern`, e.g. `WORKFLOWS_REF`.""" names = set() for i, line in enumerate(lines): if not _ENV_KEY_RE.match(line): continue for _, child in _block_body(lines, i, _indent(line)): - match = _ENV_ALIAS_RE.match(child) + match = pattern.match(_strip_comment(child)) if match: names.add(match.group(2)) return frozenset(names) +def env_aliases(lines): + """Names whose `env:` binding REACHES the input, e.g. `WORKFLOWS_REF`.""" + return _env_bindings(lines, _ENV_ALIAS_RE) + + + def _mention_alt(aliases): """Regex alternation for "reaches the input" — directly or via an alias. @@ -607,14 +755,35 @@ def is_guard_step(lines, idx): return False -def find_unguarded_ref_checkouts(lines): - """1-based line numbers of `ref: ${{ inputs.workflows_ref }}` uses with no guard. +def ref_checkouts(lines): + """(1-based line, uses_fallback, guarded) for EVERY ref checkout in the file. A use is guarded when the empty-ref guard step appears earlier in the SAME - job — jobs run independently, so a guard in job A does nothing for job B. + job — jobs run independently, so a guard in job A does nothing for job B — + AND that guard validated an expression no weaker than the one the checkout + consumes. + + Strength matters because the two recognized bindings prove different + things. A guard on the bare input proves `inputs.workflows_ref` itself is + non-empty, which covers every checkout in the job. A guard on + `inputs.workflows_ref || job.workflow_sha` proves only that the OR + expression is non-empty: with the input omitted it passes on + `job.workflow_sha`, so it says nothing about a sibling + `ref: ${{ inputs.workflows_ref }}`, which still receives '' and sends + checkout to the default branch. So a fallback guard covers fallback + checkouts only, and a bare checkout needs a bare guard. + + A checkout reaching the ref through an `env:` alias is always judged BARE: + `ref: ${{ env.NAME }}` says nothing about which binding is in effect there, + so it needs a bare guard. Fail-closed by design — see `_FALLBACK_RES`. """ aliases = env_aliases(lines) ref_res = _ref_use_res(aliases) + # Computed ONCE per file, not per line: `env_aliases` is a full-file scan + # plus a `_block_body` walk per `env:` key, and `check_dir`'s carve-out used + # to rebuild both inside its own `any(...)` comprehension — quadratic on the + # files that matter most here (groom.yml is ~3,000 lines with dozens of + # `env:` blocks, and `check_dir` runs over the real tree in the CLI lint). mention_re = re.compile(_mention_alt(aliases)) jobs_line = None for i, line in enumerate(lines): @@ -635,9 +804,10 @@ def find_unguarded_ref_checkouts(lines): if _indent(line) == job_indent ] - unguarded = [] + found = [] for start in job_starts: - guarded = False + guarded_input = False # a guard proved the INPUT non-empty + guarded_fallback = False # …only the `|| job.workflow_sha` expression # An open `ref:` whose value continues below, as (line index, indent). # Continuation lines are the more-indented ones that follow; the first # line back at or above the key's indent closes the scalar. @@ -646,23 +816,63 @@ def find_unguarded_ref_checkouts(lines): if pending is not None: if _indent(line) > pending[1]: if mention_re.search(line): - if not guarded and not _JOB_WORKFLOW_SHA_FALLBACK_RE.search(line): - unguarded.append(pending[0] + 1) + # Report the `ref:` KEY line (that is the checkout the + # reader must find), but judge the CONTINUATION line — + # the key never holds the expression, so asking it + # whether this is a fallback always answered no. + fallback = _pins_to_job_workflow_sha(line) + guarded = guarded_input or (fallback and guarded_fallback) + found.append((pending[0] + 1, fallback, guarded)) pending = None continue # Scalar closed — fall through and judge this line normally. pending = None - if _GUARD_BINDING_RE.match(line): - guarded = guarded or is_guard_step(lines, i) + binding = _GUARD_BINDING_RE.match(line) + if binding: + # NO fallback exception on the guard requirement (BE-8077). The + # BE-4169 `inputs.workflows_ref || job.workflow_sha` form cannot + # resolve to a MUTABLE ref — that is what earns it the + # `default: ''` carve-out in `check_dir` — but it is NOT + # self-sufficient the way that story assumed: `job.workflow_sha` + # needs runner v2.334.0+ and expands to '' on anything older, + # and checkout reads `ref: ''` as the DEFAULT BRANCH. So the + # fallback answers MUTABILITY and the guard answers EMPTINESS, + # and this lint requires both. Exempting the fallback from the + # guard check meant deleting every one of groom.yml's seven + # guard steps kept this lint green. + if is_guard_step(lines, i): + if binding.group("fallback"): + guarded_fallback = True + else: + guarded_input = True elif is_ref_use(line, ref_res): - # BE-4169 exception: `inputs.workflows_ref || github.job_workflow_sha` - # can never resolve empty or mutable on its own — see check_dir's - # `self_pins_to_job_workflow_sha` docstring-equivalent comment. - if not guarded and not _JOB_WORKFLOW_SHA_FALLBACK_RE.search(line): - unguarded.append(i + 1) + fallback = _pins_to_job_workflow_sha(line) + guarded = guarded_input or (fallback and guarded_fallback) + # A guard proves the INPUT is non-empty. It says nothing about + # an expression that never reaches the input: GitHub's `||` + # returns the FIRST truthy operand, so `ref: ${{ 'main' || + # inputs.workflows_ref }}` mentions the input (and is therefore + # a ref use, and passes the guard) while resolving to a mutable + # branch on every runner — no second input declaration needed. + # `check_dir` cannot see it either; it reads only + # `workflows_ref`'s own `default:`. So the leading operand has + # to reach the input, or no guard in the job covers this ref. + if not _leading_operand_reaches_input(line, mention_re): + guarded = False + found.append((i + 1, fallback, guarded)) elif _REF_KEY_OPEN_RE.match(line): pending = (i, _indent(line)) - return unguarded + return found + + +def unguarded_ref_checkouts(lines): + """(1-based line, uses_fallback) for every ref checkout with no adequate guard.""" + return [(lineno, fb) for lineno, fb, guarded in ref_checkouts(lines) if not guarded] + + +def find_unguarded_ref_checkouts(lines): + """1-based line numbers of ref checkouts with no adequate guard.""" + return [lineno for lineno, _ in unguarded_ref_checkouts(lines)] def check_dir(workflows_dir, exempt=KNOWN_EXEMPT): @@ -715,21 +925,42 @@ def check_dir(workflows_dir, exempt=KNOWN_EXEMPT): # drops the default — which puts it back under the check below. continue - # A safe alternative to `required: true` + a runtime guard (BE-4169): - # default the input to '' and OR every checkout's `ref:` with - # `github.job_workflow_sha` — the exact commit THIS reusable workflow - # was itself resolved from via the caller's `uses:` pin. That value can - # never be empty and is never mutable, so an omitted `workflows_ref` - # self-pins instead of silently taking the default branch — the same - # guarantee `required: true` + the guard buys, bought a different way. - # `find_unguarded_ref_checkouts` already recognizes the checkout side - # of this (the LITERAL fallback `inputs.workflows_ref || - # github.job_workflow_sha`); this covers the matching `default: ''`, - # which is otherwise indistinguishable from the `default: main` hole. - # See groom.yml. - self_pins_to_job_workflow_sha = any( - _JOB_WORKFLOW_SHA_FALLBACK_RE.search(line) for line in lines - ) + # A safe alternative to `required: true` alone (BE-4169, corrected by + # BE-8077): default the input to '' and OR every checkout's `ref:` with + # `job.workflow_sha` — the exact commit THIS reusable workflow was itself + # resolved from via the caller's `uses:` pin. That value is never + # mutable, so an omitted `workflows_ref` self-pins instead of silently + # taking the default branch, which is what makes the `default: ''` + # tolerable here. `find_unguarded_ref_checkouts` already recognizes the + # checkout side of this (the LITERAL fallback `inputs.workflows_ref || + # job.workflow_sha`); this covers the matching `default: ''`, which is + # otherwise indistinguishable from the `default: main` hole. See + # groom.yml — which ALSO runs a fail-closed empty-ref guard in every one + # of those jobs, because `job.workflow_sha` needs runner v2.334.0+ and + # expands to '' on anything older. The old `github.job_workflow_sha` + # spelling is deliberately NOT accepted: it is an OIDC claim and expands + # to '' on EVERY runner, so a file "self-pinning" with it pins nothing. + # + # Scoped to actual ref CHECKOUTS, not to every line in the file. Asking + # it of all of them granted the carve-out to any file that merely + # MENTIONS the expression in code — most sharply, the guard steps' own + # `WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }}` + # `env:` binding — so a file whose checkouts all read the bare + # `ref: ${{ inputs.workflows_ref }}` bought an empty default it does not + # self-pin against. (Stripping comments closed the prose half of that; + # this closes the code half.) + # + # Asked of the PARSER rather than re-derived line by line here. A + # per-line scan cannot see the block-scalar spelling the parser already + # handles — for `ref: >-` with the expression on the line below, the key + # line carries no expression and the continuation line carries no `ref:` + # key, so no single line satisfies both tests. That file self-pins, but + # lost the carve-out and got BE-5546's "delete the default" error while + # its checkouts simultaneously got BE-8077's "the fallback IS + # recognized" one. Fail-closed, but a false failure with contradictory + # advice. `ref_checkouts` judges the continuation line the parser + # matched, and computes the alias scan once instead of once per line. + self_pins_to_job_workflow_sha = any(fb for _, fb, _ in ref_checkouts(lines)) for lineno in defaults: if self_pins_to_job_workflow_sha and _default_value(lines[lineno - 1]) in ("''", '""'): @@ -742,7 +973,25 @@ def check_dir(workflows_dir, exempt=KNOWN_EXEMPT): % (path, lineno, name, INPUT_NAME) ) - for lineno in find_unguarded_ref_checkouts(lines): + for lineno, uses_fallback in unguarded_ref_checkouts(lines): + # `uses_fallback` comes from the line the parser MATCHED, which for a + # block scalar or continuation value is not the reported `ref:` key + # line — re-reading that key line here always answered "not a + # fallback" and emitted the wrong (BE-5546) message for it. + if uses_fallback: + # The `|| job.workflow_sha` form: immutable, but empty on a + # pre-v2.334.0 runner, so it still needs the guard (BE-8077). + errors.append( + "::error file=%s,line=%d::%s checks out at `ref: ${{ inputs.%s " + "|| job.workflow_sha }}` with no empty-ref guard earlier in the " + "same job. The fallback stops the ref being MUTABLE, not being " + "EMPTY: `job.workflow_sha` needs an Actions runner >= v2.334.0 " + "and expands to '' on anything older, which checkout reads as " + "the default branch. Pair it with a `Require a resolvable " + "workflows_ref` step. See BE-8077." + % (path, lineno, name, INPUT_NAME) + ) + continue errors.append( "::error file=%s,line=%d::%s checks out at `ref: ${{ inputs.%s }}` " "with no empty-ref guard earlier in the same job. Copy the " diff --git a/.github/workflow-pins/tests/test_check_workflow_pins.py b/.github/workflow-pins/tests/test_check_workflow_pins.py index 05f654a6..097b3abc 100644 --- a/.github/workflow-pins/tests/test_check_workflow_pins.py +++ b/.github/workflow-pins/tests/test_check_workflow_pins.py @@ -189,6 +189,14 @@ class GuardCoverageTests(unittest.TestCase): " exit 1\n" " fi\n" ) + # groom.yml's variant (BE-8077): same guard, but the ref reaches it via the + # `|| job.workflow_sha` fallback. The binding detector must accept BOTH + # spellings or these seven real guards are never consulted. + GUARD_WITH_FALLBACK = GUARD.replace( + "${{ inputs.workflows_ref }}", + "${{ inputs.workflows_ref || job.workflow_sha }}", + ).replace("Require a pinned", "Require a resolvable") + CHECKOUT = ( " - name: Load assets\n" " uses: actions/checkout@abc\n" @@ -203,11 +211,16 @@ class GuardCoverageTests(unittest.TestCase): ' with: {repository: Comfy-Org/github-workflows, ref: "${{ inputs.workflows_ref }}"}\n' ) - def _jobs(self, *jobs): + @staticmethod + def _wrap(*jobs): + """The step blocks as a whole workflow, one job each.""" text = "name: F\non:\n workflow_call:\njobs:\n" for i, steps in enumerate(jobs): text += " job%d:\n runs-on: ubuntu-latest\n steps:\n%s" % (i, steps) - return cwp.find_unguarded_ref_checkouts(text.split("\n")) + return text + + def _jobs(self, *jobs): + return cwp.find_unguarded_ref_checkouts(self._wrap(*jobs).split("\n")) def test_guarded_checkout_passes(self): self.assertEqual(self._jobs(self.GUARD + self.CHECKOUT), []) @@ -569,22 +582,299 @@ def test_the_input_after_a_ref_scalar_closes_is_not_attributed_to_it(self): ) self.assertEqual(self._jobs(step), []) - def test_a_job_workflow_sha_fallback_needs_no_guard(self): - # BE-4169: `inputs.workflows_ref || github.job_workflow_sha` can never - # resolve empty or mutable on its own, so it needs no runtime guard — - # see groom.yml. + def test_a_job_workflow_sha_fallback_still_needs_a_guard(self): + # BE-8077, correcting BE-4169. The fallback can never resolve to a + # MUTABLE ref -- which is what still earns it the `default: ''` + # carve-out in `check_dir` -- but it is NOT self-sufficient: + # `job.workflow_sha` needs runner v2.334.0+ and expands to '' on + # anything older, which checkout reads as the default branch. So the + # fallback answers mutability, the guard answers emptiness, and the + # lint requires BOTH. Exempting the fallback from the guard check made + # groom.yml's seven guard steps deletable with this lint still green. + checkout = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ inputs.workflows_ref || job.workflow_sha }}\n" + ) + self.assertEqual(len(self._jobs(checkout)), 1) + + def test_a_guarded_job_workflow_sha_fallback_passes(self): + # The other direction: groom.yml's real shape -- the fallback checkout + # WITH its guard -- must stay clean. That guard binds the ref as + # `${{ inputs.workflows_ref || job.workflow_sha }}` too, and matching + # only the bare `${{ inputs.workflows_ref }}` binding meant this shape + # was never recognized as a guard at all. + checkout = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ inputs.workflows_ref || job.workflow_sha }}\n" + ) + self.assertEqual(self._jobs(self.GUARD_WITH_FALLBACK + checkout), []) + + def test_a_fallback_guard_does_not_cover_a_bare_input_checkout(self): + # The strength rule, and the reason `_GUARD_BINDING_RE` records WHICH + # expression each guard validated. This guard proves only that + # `inputs.workflows_ref || job.workflow_sha` is non-empty. With the + # input omitted it passes on `job.workflow_sha` while the bare + # `ref: ${{ inputs.workflows_ref }}` below still receives '' and + # checkout takes the default branch — so it must NOT mark the job + # guarded for that checkout. Treating any recognized binding as blanket + # job-wide coverage put a live hole behind a green lint. + self.assertEqual(len(self._jobs(self.GUARD_WITH_FALLBACK + self.CHECKOUT)), 1) + + def test_a_bare_guard_covers_a_fallback_checkout(self): + # The permitted direction: a guard on the bare input proves the INPUT + # itself is non-empty, which is strictly stronger than what a fallback + # checkout needs. + checkout = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ inputs.workflows_ref || job.workflow_sha }}\n" + ) + self.assertEqual(self._jobs(self.GUARD + checkout), []) + + def test_a_third_operand_defeats_the_fallback_exemption(self): + # The fallback regex is anchored to the CLOSE of the interpolation on + # purpose. Unanchored it read "contains the fallback" rather than "IS + # the fallback", so this expression -- which resolves to the MUTABLE + # default branch in precisely the pre-v2.334.0 case the fallback exists + # for -- was blessed by the very lint meant to catch it. It is reported + # here because it is unguarded; `test_a_third_operand_is_not_a_self_pin` + # covers the `default: ''` carve-out half. + checkout = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ inputs.workflows_ref || job.workflow_sha || 'main' }}\n" + ) + self.assertEqual(len(self._jobs(checkout)), 1) + + def test_a_leading_operand_defeats_the_fallback_exemption(self): + # The anchor has to hold at BOTH ends. Anchoring only the tail still let + # an operand in FRONT through, and that one resolves to whatever the + # leading operand names — a branch, a tag, another input — which the + # runtime guard cannot catch, since a guard proves non-emptiness, not + # immutability. + checkout = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ inputs.override || inputs.workflows_ref || job.workflow_sha }}\n" + ) + self.assertEqual(len(self._jobs(checkout)), 1) + + def test_an_env_alias_of_the_fallback_binding_is_still_a_ref_use(self): + # `_ENV_ALIAS_RE` has to know the same spellings `_GUARD_BINDING_RE` + # does. Hoisting the binding to a shared `env:` and checking out at + # `ref: ${{ env.WORKFLOWS_REF }}` is exactly the refactor the alias + # machinery exists to survive; if it registers no alias, `is_ref_use` + # stops seeing those checkouts and the file drops to zero coverage + # while reporting nothing at all. + self.assertEqual( + cwp.env_aliases( + "env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }}\n".split("\n") + ), + frozenset({"WORKFLOWS_REF"}), + ) + + def test_an_env_alias_NEVER_earns_the_self_pin_exemption(self): + # Carrying the binding's strength to `ref: ${{ env.NAME }}` was tried + # and reverted: `env:` is scoped per step and per job and it SHADOWS, + # while these scans are file-wide, so a file-wide "names bound to the + # fallback" set granted the exemption at checkouts the binding never + # reaches. This fixture is the counterexample that killed it — the + # binding lives in the GUARD step's `env:`, which is invisible to the + # sibling checkout step at run time, so `${{ env.WORKFLOWS_REF }}` + # expands to '' and takes the default branch. It scored a guarded + # self-pin. An alias is judged BARE now, so a fallback guard does not + # cover it and the checkout is reported. + aliased = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ env.WORKFLOWS_REF }}\n" + ) + lines = self._wrap(self.GUARD_WITH_FALLBACK + aliased).split("\n") + self.assertFalse( + any(fb for _, fb, _ in cwp.ref_checkouts(lines)), + "an env alias must never read as a self-pin", + ) + self.assertEqual(len(self._jobs(self.GUARD_WITH_FALLBACK + aliased)), 1) + # …and a BARE guard, which proves the input itself is non-empty, does + # cover it. That is the correct answer, and the one left standing. + self.assertEqual(self._jobs(self.GUARD + aliased), []) + + def test_a_mutable_step_local_binding_inherits_nothing(self): + # The other direction of the same file-wide bug: a step-local + # `WORKFLOWS_REF: ${{ inputs.workflows_ref || 'main' }}` inherited + # fallback strength from any OTHER step binding that name strictly. + # cursor-review.yml binds `WORKFLOWS_REF` both ways today, so this was + # not hypothetical. + mixed = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref || 'main' }}\n" + " with:\n" + " ref: ${{ env.WORKFLOWS_REF }}\n" + ) + lines = self._wrap(self.GUARD_WITH_FALLBACK + mixed).split("\n") + self.assertFalse(any(fb for _, fb, _ in cwp.ref_checkouts(lines))) + self.assertEqual(len(self._jobs(self.GUARD_WITH_FALLBACK + mixed)), 1) + + def test_the_leading_operand_has_to_reach_the_input(self): + # A guard proves the INPUT is non-empty; it says nothing about an + # expression that never reaches the input. GitHub's `||` returns the + # first TRUTHY operand, so both of these mention the input (so they are + # ref uses, and clear the guard) while resolving to a mutable ref on + # every runner. The literal form needs no second input declaration at + # all — which is why the earlier "unreachable without another input" + # deferral of this was wrong. + for ref in ("${{ 'main' || inputs.workflows_ref }}", + "${{ inputs.override || inputs.workflows_ref }}"): + checkout = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: %s\n" % ref + ) + self.assertEqual(len(self._jobs(self.GUARD + checkout)), 1, ref) + + def test_the_leading_operand_check_accepts_the_legitimate_shapes(self): + # The narrowing must not fire on anything this module works to accept. + for ref in ("${{ inputs.workflows_ref }}", + "${{ inputs.workflows_ref || job.workflow_sha }}", + "${{ env.WORKFLOWS_REF }}"): + checkout = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: %s\n" % ref + ) + self.assertEqual(self._jobs(self.GUARD + checkout), [], ref) + + def test_a_quoted_sibling_cannot_plant_a_decoy_ref(self): + # The flow matcher `search`es mid-line, so its `[{,]` boundary can be + # met by a comma INSIDE a quoted scalar — planting a `ref:` that scores + # the line a self-pin while the real `ref:` on it is bare, which buys + # the weaker fallback-guard requirement and the `default: ''` carve-out. + decoy = ( + " with: {ref: '${{ inputs.workflows_ref }}', " + "path: \"x, ref: ${{ inputs.workflows_ref || job.workflow_sha }}, y\"}" + ) + self.assertFalse(cwp._pins_to_job_workflow_sha(decoy)) + # …while a real flow-form fallback, quoted or not, still counts. + self.assertTrue(cwp._pins_to_job_workflow_sha( + " with: {repository: a/b, ref: ${{ inputs.workflows_ref || job.workflow_sha }}}" + )) + self.assertTrue(cwp._pins_to_job_workflow_sha( + " with: {ref: '${{ inputs.workflows_ref || job.workflow_sha }}', x: 1}" + )) + + def test_a_comment_does_not_bind_an_env_alias(self): + # The widened `_ENV_ALIAS_RE` matches any value mentioning the input, so + # unstripped it became the one place in the module reading a comment as + # code — failing a compliant workflow whose unrelated `env:` value + # merely mentions the input in prose. + lines = self._wrap( + " - name: x\n" + " env:\n" + " GROOM_ASSETS: _groom_assets # checked out at inputs.workflows_ref\n" + " run: echo hi\n" + ).split("\n") + self.assertEqual(cwp.env_aliases(lines), frozenset()) + + def test_an_unrecognized_env_binding_still_counts_as_reaching_the_input(self): + # Enumerating blessed spellings made an unrecognized one fail OPEN: + # `WORKFLOWS_REF: ${{ inputs.workflows_ref || 'main' }}` registered no + # alias, so `ref: ${{ env.WORKFLOWS_REF }}` read as no ref use at all + # and that checkout — carrying the exact mutable fallback this lint + # exists to catch — left the lint entirely. + lines = ( + "jobs:\n" + " check:\n" + " steps:\n" + " - name: Load assets\n" + " env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref || 'main' }}\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ env.WORKFLOWS_REF }}\n" + ).split("\n") + self.assertEqual(cwp.env_aliases(lines), frozenset({"WORKFLOWS_REF"})) + self.assertEqual(len(cwp.find_unguarded_ref_checkouts(lines)), 1) + + def test_the_fallback_must_be_the_WHOLE_ref_value(self): + # Anchoring `${{` … `}}` bounds the INTERPOLATION, not the YAML value. + # Each of these still scored as a self-pin — earning the weaker + # fallback-guard requirement and the `default: ''` carve-out while + # resolving to a mutable ref, or to a sibling entry's value. + for ref in ( + "refs/heads/${{ inputs.workflows_ref || job.workflow_sha }}", + "${{ inputs.override }}${{ inputs.workflows_ref || job.workflow_sha }}", + ): + checkout = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: %s\n" % ref + ) + lines = self._wrap(checkout).split("\n") + self.assertFalse(any(fb for _, fb, _ in cwp.ref_checkouts(lines)), ref) + self.assertEqual(len(self._jobs(checkout)), 1, ref) + + def test_a_flow_sibling_carrying_the_fallback_is_not_the_ref(self): + # `_REF_USE_FLOW_RE` bounds its value at the entry boundary so a sibling + # cannot be misread as the ref; the self-pin matcher needs the same + # boundary, or the sibling's fallback exempts a bare `ref:`. + checkout = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with: {ref: '${{ inputs.workflows_ref }}', " + "x: '${{ inputs.workflows_ref || job.workflow_sha }}'}\n" + ) + lines = self._wrap(checkout).split("\n") + self.assertFalse(any(fb for _, fb, _ in cwp.ref_checkouts(lines))) + self.assertEqual(len(self._jobs(checkout)), 1) + + def test_a_block_scalar_self_pin_is_seen_by_the_carve_out_scan(self): + # The per-line scan could not see this spelling: the key line carries no + # expression and the continuation line carries no `ref:` key, so no + # single line satisfied both tests. The file self-pins, but lost the + # carve-out and got BE-5546's "delete the default" while its checkouts + # got BE-8077's "the fallback IS recognized" — contradictory advice. + checkout = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: >-\n" + " ${{ inputs.workflows_ref || job.workflow_sha }}\n" + ) + lines = self._wrap(checkout).split("\n") + self.assertTrue(any(fb for _, fb, _ in cwp.ref_checkouts(lines))) + + def test_the_github_job_workflow_sha_spelling_is_no_longer_exempt(self): + # BE-8077: `github.job_workflow_sha` is an OIDC token claim, NOT a + # `github` context property, so Actions expands it to '' and checkout + # reads `ref: ''` as this repo's default branch. The old regex blessed + # exactly that. It must now be flagged like any other unguarded + # checkout, so the mistake cannot be reintroduced with the lint green. checkout = ( " - name: Load assets\n" " uses: actions/checkout@abc\n" " with:\n" " ref: ${{ inputs.workflows_ref || github.job_workflow_sha }}\n" ) - self.assertEqual(self._jobs(checkout), []) + self.assertEqual(len(self._jobs(checkout)), 1) def test_a_plain_fallback_to_a_branch_is_still_unguarded(self): - # Only the LITERAL `github.job_workflow_sha` fallback is exempt — a - # fallback to anything else (here, a floating branch) is the same - # `default: main` hole wearing a different hat and must still trip. + # Only the LITERAL `job.workflow_sha` fallback is exempt — a fallback to + # anything else (here, a floating branch) is the same `default: main` + # hole wearing a different hat and must still trip. checkout = ( " - name: Load assets\n" " uses: actions/checkout@abc\n" @@ -638,14 +928,130 @@ def test_this_repos_own_workflows_guard_every_ref_checkout(self): self.assertEqual(cwp.find_unguarded_ref_checkouts(lines), [], name) self.assertEqual( seen, - 16, + 15, "expected the 12 guarded sites BE-5546 fixed + pr-size.yml's (BE-5858) " - "+ cursor-review.yml's preflight (hard guard) and ledger (BE-4169 " - "job_workflow_sha self-pin) sites picked up merging main + " - "cursor-review.yml's diff-size job's check-pr-size-tool checkout, " - "also picked up merging main (BE-5546 added its guard here)", + "+ cursor-review.yml's preflight (hard guard) site picked up merging " + "main + cursor-review.yml's diff-size job's check-pr-size-tool " + "checkout, also picked up merging main (BE-5546 added its guard " + "here). cursor-review.yml's ledger checkout dropped OUT of this count " + "in BE-8077: it now reads `ref: ${{ steps.resolve_ref.outputs.ref }}` " + "— the input is resolved one step earlier, so the `ref:` line no " + "longer names it. groom.yml's 7 sites still do, via the " + "`|| job.workflow_sha` fallback — and are clean because each one " + "carries its guard, which BE-8077 made load-bearing rather than " + "exempt.", + ) + + WORKFLOWS_DIR = os.path.normpath( + os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..", "workflows") + ) + + def _workflow(self, name): + with open(os.path.join(self.WORKFLOWS_DIR, name), encoding="utf-8") as f: + return f.read().split("\n") + + @staticmethod + def _enclosing_step(lines, idx): + """The lines of the `- name:` step containing `idx`.""" + start = idx + while start >= 0 and not lines[start].lstrip().startswith("- name:"): + start -= 1 + assert start >= 0, "line %d is not inside a step" % idx + indent = len(lines[start]) - len(lines[start].lstrip()) + end = start + 1 + while end < len(lines): + stripped = lines[end].strip() + if stripped and len(lines[end]) - len(lines[end].lstrip()) <= indent: + break + end += 1 + return lines[start:end] + + def test_the_ledger_checkout_keeps_its_resolve_then_skip_guard(self): + # BE-8077 moved cursor-review.yml's ledger checkout to + # `ref: ${{ steps.resolve_ref.outputs.ref }}`, which no longer NAMES the + # input — so `is_ref_use` does not see it and the lint cannot cover it + # (that is the 16 -> 15 in the count assertion above). The only thing + # standing between an unresolvable ref and a silent default-branch + # checkout of the assets this job EXECUTES is the hand-written `if:`. + # Nothing lints it, so pin it here until the detector learns to follow a + # `ref:` through a step output. + lines = self._workflow("cursor-review.yml") + consumers = [ + i + for i, line in enumerate(lines) + if "ref: ${{ steps.resolve_ref.outputs.ref }}" in line + ] + self.assertTrue(consumers, "ledger checkout no longer reads resolve_ref") + + for idx in consumers: + # Scoped to the consuming step itself, and matched EXACTLY. A fixed + # line window let a neighbouring step's `if:` satisfy this while the + # consumer ran unconditionally, accepted a widened condition such as + # `... != '' || always()` (which runs the checkout precisely when the + # output is empty), and broke on a compliant step that simply grew a + # couple of comment lines. + step = [s.strip() for s in self._enclosing_step(lines, idx)] + self.assertIn( + "if: steps.resolve_ref.outputs.ref != ''", + step, + "checkout at line %d has no exact non-empty `if:` in its own step" + % (idx + 1), + ) + + # …and the resolving step must REJECT rather than sanitize: it emits the + # empty string for anything not ref-shaped, under a pinned byte locale. + # + # Scoped to that step, not to the whole file. Matched against the file + # body, any step in it satisfied these — and the file already carries + # six other hand-copied guards, so the natural consistency sweep that + # adds `export LC_ALL=C` to them would let someone delete it from + # `resolve_ref` with the only test covering that step still green. + resolvers = [i for i, line in enumerate(lines) if line.strip() == "id: resolve_ref"] + self.assertEqual(len(resolvers), 1, "expected exactly one resolve_ref step") + resolver = [s.strip() for s in self._enclosing_step(lines, resolvers[0])] + self.assertIn("*[!A-Za-z0-9._/@+-]*) REF='' ;;", resolver) + self.assertIn("export LC_ALL=C", resolver) + # …and it must resolve from the BE-8077 fallback. Every assertion above + # still passed with the binding swapped for `|| 'main'`, which is the + # mutable-ref hole this whole ticket exists to close. + self.assertIn( + "WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }}", + resolver, + "resolve_ref no longer resolves from the job.workflow_sha fallback", ) + def test_every_groom_guard_carries_the_shape_check(self): + # The `case` block is the only thing closing the Unicode-whitespace hole + # (the `-z` above it runs on an ASCII-only stripped copy), and + # `is_guard_step` only requires an emptiness test plus an exit — so + # deleting all seven `case` blocks left the pin lint AND this whole suite + # green. cursor-review.yml's twin is pinned by the test above; this is + # the matching cover for groom.yml. + lines = self._workflow("groom.yml") + starts = [ + i + for i, line in enumerate(lines) + if line.strip() == "- name: Require a resolvable workflows_ref" + ] + self.assertEqual(len(starts), 7, "expected 7 groom guard steps") + + bodies = [] + for idx in starts: + step = self._enclosing_step(lines, idx) + text = "\n".join(step) + self.assertIn("export LC_ALL=C", text) + self.assertIn('REF="$(printf \'%s\' "$WORKFLOWS_REF" | tr -d \'[:space:]\')"', text) + self.assertIn('if [ -z "$REF" ]; then', text) + self.assertIn('case "$WORKFLOWS_REF" in', text) + self.assertIn("*[!A-Za-z0-9._/@+-]*)", text) + self.assertEqual(text.count("exit 1"), 2, "both branches must exit") + bodies.append(text) + + # Seven hand-copied guards drift; the guarantee is only as strong as the + # weakest copy, so require them byte-identical. + self.assertEqual(len(set(bodies)), 1, "the 7 groom guard steps have drifted apart") + + class CheckDirTests(unittest.TestCase): def setUp(self): @@ -675,8 +1081,10 @@ def test_defaulted_dir_fails_with_an_annotation(self): def test_a_job_workflow_sha_default_is_tolerated(self): # BE-4169: `default: ''` paired with `inputs.workflows_ref || - # github.job_workflow_sha` at the checkout is not the `default: main` - # hole — the fallback can never be empty or mutable. See groom.yml. + # job.workflow_sha` at the checkout is not the `default: main` hole — + # the fallback can never be mutable. See groom.yml. The guard below is + # what BE-8077 additionally requires: the carve-out is about the ref + # not being MUTABLE, and the guard is what makes it non-EMPTY. text = ( "name: Fixture\n" "on:\n" @@ -690,16 +1098,191 @@ def test_a_job_workflow_sha_default_is_tolerated(self): " check:\n" " runs-on: ubuntu-latest\n" " steps:\n" + " - name: Require a resolvable workflows_ref\n" + " env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }}\n" + " run: |\n" + ' if [ -z "$WORKFLOWS_REF" ]; then\n' + ' echo "::error::empty"\n' + " exit 1\n" + " fi\n" " - name: Load assets\n" " uses: actions/checkout@abc\n" " with:\n" - " ref: ${{ inputs.workflows_ref || github.job_workflow_sha }}\n" + " ref: ${{ inputs.workflows_ref || job.workflow_sha }}\n" ) self._write("groom-like.yml", text) errors, checked, _ = cwp.check_dir(self.dir, exempt=frozenset()) self.assertEqual(errors, [], errors) self.assertEqual(checked, ["groom-like.yml"]) + def test_a_third_operand_is_not_a_self_pin(self): + # The `default: ''` carve-out half of the same anchoring bug. This + # fixture LOOKS like the groom shape and is not one: with both leading + # operands empty it checks out `main`, so the empty default is the + # BE-5546 mutable-default hole after all. The unanchored regex read the + # substring and waved it through. + text = ( + "name: Fixture\n" + "on:\n" + " workflow_call:\n" + " inputs:\n" + " workflows_ref:\n" + " type: string\n" + " required: false\n" + " default: ''\n" + "jobs:\n" + " check:\n" + " runs-on: ubuntu-latest\n" + " steps:\n" + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ inputs.workflows_ref || job.workflow_sha || 'main' }}\n" + ) + self._write("sneaky.yml", text) + errors, checked, _ = cwp.check_dir(self.dir, exempt=frozenset()) + self.assertTrue(any("BE-5546" in e and "default:" in e for e in errors), errors) + self.assertEqual(checked, ["sneaky.yml"]) + + def test_a_comment_naming_the_fallback_is_not_a_self_pin(self): + # `check_dir` asks "does ANY ref checkout self-pin?", so prose merely + # NAMING the expression -- which this repo's workflows do at length -- + # must not buy the `default: ''` carve-out for a file whose checkout + # never uses the fallback. Two mechanisms hold this now (the scan is + # scoped to checkouts, AND comments are stripped from what it reads); + # the trailing-comment test below is the one that pins the second. + text = ( + "name: Fixture\n" + "on:\n" + " workflow_call:\n" + " inputs:\n" + " workflows_ref:\n" + " type: string\n" + " required: false\n" + " default: ''\n" + "jobs:\n" + " check:\n" + " runs-on: ubuntu-latest\n" + " steps:\n" + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + # A COMPLETE `${{ … }}` — spelled without the opening `${{` the + # fixture cannot exercise the matcher at all — both on a whole-line + # comment and trailing a real `ref:` line. + " # we could have used ${{ inputs.workflows_ref || job.workflow_sha }}\n" + " ref: ${{ inputs.workflows_ref }} # not ${{ inputs.workflows_ref || job.workflow_sha }}\n" + ) + self._write("commented.yml", text) + errors, checked, _ = cwp.check_dir(self.dir, exempt=frozenset()) + self.assertTrue(any("BE-5546" in e and "default:" in e for e in errors), errors) + self.assertEqual(checked, ["commented.yml"]) + + def test_a_trailing_comment_does_not_hide_a_REAL_self_pin(self): + # The other direction, and the one that actually pins the stripping: + # the matcher anchors the fallback to the whole YAML value, so an + # unstripped `# pinned` sits between the expression and the end of the + # line and the genuine self-pin stops being recognized — costing a + # compliant file its `default: ''` carve-out. + text = ( + "name: Fixture\n" + "on:\n" + " workflow_call:\n" + " inputs:\n" + " workflows_ref:\n" + " type: string\n" + " required: false\n" + " default: ''\n" + "jobs:\n" + " check:\n" + " runs-on: ubuntu-latest\n" + " steps:\n" + " - name: Require a resolvable workflows_ref\n" + " env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }}\n" + " run: |\n" + " if [ -z \"$WORKFLOWS_REF\" ]; then\n" + " exit 1\n" + " fi\n" + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ inputs.workflows_ref || job.workflow_sha }} # pinned\n" + ) + self._write("pinned.yml", text) + errors, checked, _ = cwp.check_dir(self.dir, exempt=frozenset()) + self.assertEqual(errors, []) + self.assertEqual(checked, ["pinned.yml"]) + + def test_a_block_scalar_self_pin_keeps_the_default_carve_out(self): + # `check_dir`'s carve-out asks the PARSER, not each line in isolation. + # A per-line scan cannot see this spelling — the key line carries no + # expression, the continuation line carries no `ref:` key — so the file + # lost the carve-out and got BE-5546's "delete the default" while its + # checkouts got BE-8077's "the fallback IS recognized": a false failure + # carrying contradictory advice. + text = ( + "name: Fixture\n" + "on:\n" + " workflow_call:\n" + " inputs:\n" + " workflows_ref:\n" + " type: string\n" + " required: false\n" + " default: ''\n" + "jobs:\n" + " check:\n" + " runs-on: ubuntu-latest\n" + " steps:\n" + " - name: Require a resolvable workflows_ref\n" + " env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }}\n" + " run: |\n" + " if [ -z \"$WORKFLOWS_REF\" ]; then\n" + " exit 1\n" + " fi\n" + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: >-\n" + " ${{ inputs.workflows_ref || job.workflow_sha }}\n" + ) + self._write("folded.yml", text) + errors, checked, _ = cwp.check_dir(self.dir, exempt=frozenset()) + self.assertEqual(errors, []) + self.assertEqual(checked, ["folded.yml"]) + + + def test_the_github_job_workflow_sha_default_is_no_longer_tolerated(self): + # BE-8077: the same fixture spelled the OLD way is the `default: main` + # hole in disguise — `github.job_workflow_sha` expands to '' on every + # runner, so an omitted input self-pins to the default branch. Both + # halves must fire: the tolerated-default carve-out and the + # unguarded-checkout exemption. + text = ( + "name: Fixture\n" + "on:\n" + " workflow_call:\n" + " inputs:\n" + " workflows_ref:\n" + " type: string\n" + " required: false\n" + " default: ''\n" + "jobs:\n" + " check:\n" + " runs-on: ubuntu-latest\n" + " steps:\n" + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ inputs.workflows_ref || github.job_workflow_sha }}\n" + ) + self._write("stale-spelling.yml", text) + errors, checked, _ = cwp.check_dir(self.dir, exempt=frozenset()) + self.assertEqual(len(errors), 2, errors) + self.assertEqual(checked, ["stale-spelling.yml"]) + def test_a_default_without_the_fallback_is_still_reported(self): # The exemption is conditional on the fallback actually being present: # `default: ''` alone, with an unfallback'd checkout, is the ordinary diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 4feaddef..ee7daa42 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -387,19 +387,73 @@ jobs: rounds: ${{ steps.build.outputs.rounds || steps.fallback.outputs.rounds }} last_reviewed_sha: ${{ steps.build.outputs.last_reviewed_sha }} steps: + - name: Resolve the asset ref + # This job must never fail (see the job comment), so the fail-closed + # `Require a pinned workflows_ref` guard the other jobs run is not an + # option here. Resolve the ref in a step that can only WARN instead, and + # let the checkout below skip itself when there is nothing safe to check + # out. Skipping is safe by construction: the `Build prior-review ledger` + # step then fails under its `continue-on-error: true`, and the + # `Ensure ledger artifact exists` fallback publishes status=unknown — the + # already-designed degraded path, reported rather than silently `empty`. + # + # The fallback is `job.workflow_sha` — the commit THIS reusable workflow + # was resolved from via the caller's `uses:` pin — so an omitted + # `workflows_ref` still pins to an immutable SHA, never to this repo's + # moving default branch. It is NOT `github.job_workflow_sha`, which this + # step used to read: that is an OIDC token claim, not a `github` context + # property, so Actions expanded it to '' and `actions/checkout` read + # `ref: ''` as "the default branch" — the exact hole the comment here + # claimed to close (BE-8077). `job.workflow_sha` is the `job`-context + # accessor added in runner v2.334.0 (Apr 2026); `ubuntu-latest` is always + # current, so only a stale self-hosted runner reaches the warn branch. + # (actionlint <= 1.7.12 false-positives on it — its `job`-context schema + # predates that runner release; nothing here runs actionlint.) + id: resolve_ref + continue-on-error: true + # Every step in this job is time-boxed BELOW the job's own + # `timeout-minutes` on purpose (see the job comment): a hung step must + # trip its own timeout, which continue-on-error absorbs, rather than the + # job's, which would fail the job and skip the whole review matrix. + timeout-minutes: 2 + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }} + run: | + # Bracket RANGES (`A-Z`) match by COLLATION order, so under a glibc + # locale such as en_US.UTF-8 they also match accented letters and the + # negated class below quietly stops rejecting them. Pin the byte order. + export LC_ALL=C + # actions/checkout reads `ref` through core.getInput, which TRIMS, so a + # whitespace-only value is an empty ref to IT while sailing past a bare + # -z test here — and JS `trim()` strips U+00A0/U+FEFF that POSIX + # `[:space:]` leaves alone, so stripping cannot be how this decides. + # Nor can it emit a stripped value: `tr -d` removes INTERIOR whitespace + # too, so `a b` would be silently rewritten into the different-but-real + # ref `ab` instead of being rejected. Validate the SHAPE and emit the + # value UNTOUCHED. The class is deliberately narrower than git's own + # ref rules (which also permit `(`, `)`, `,`, `%` and UTF-8) — a policy + # restriction, not a validity claim — and it keeps out every Unicode + # space and every newline that could smuggle a ::workflow command:: + # into the log or into $GITHUB_OUTPUT. Anything outside it resolves to + # the empty output that skips the checkout below. + REF="$WORKFLOWS_REF" + case "$REF" in + *[!A-Za-z0-9._/@+-]*) REF='' ;; + esac + if [ -z "$REF" ]; then + echo "::warning::Could not resolve a usable ref for the cursor-review assets: either the workflows_ref input is empty AND job.workflow_sha resolved to '' (which needs an Actions runner older than v2.334.0), or the value contains characters outside the set this workflow accepts (A-Z a-z 0-9 . _ / - @ +) — a deliberate policy restriction, narrower than git's own ref rules. Skipping the asset checkout rather than silently loading them from this repo's default branch — the prior-review ledger degrades to status=unknown for this run. Set workflows_ref: to the same commit SHA the caller's uses: line pins." + fi + echo "ref=$REF" >> "$GITHUB_OUTPUT" + - name: Load cursor-review assets + # Skipped when the ref could not be resolved — see `resolve_ref` above. + if: steps.resolve_ref.outputs.ref != '' continue-on-error: true timeout-minutes: 5 - uses: actions/checkout@v7 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: repository: Comfy-Org/github-workflows - # This job must never fail (see the job comment), so an empty-ref - # guard that exits non-zero is not an option here. Fall back to the - # exact commit THIS reusable workflow was resolved from - # (github.job_workflow_sha, same pattern as groom.yml/BE-4169) - # instead: an omitted workflows_ref still pins to an immutable SHA, - # never to this repo's moving default branch. - ref: ${{ inputs.workflows_ref || github.job_workflow_sha }} + ref: ${{ steps.resolve_ref.outputs.ref }} path: _cursor_review_assets persist-credentials: false @@ -455,7 +509,7 @@ jobs: # of the artifact all download it continue-on-error, and consolidate # emits the unavailable banner when the file does not arrive). continue-on-error: true - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 with: name: cursor-review-ledger path: /tmp/ledger @@ -1259,7 +1313,7 @@ jobs: - name: Download prior-review ledger continue-on-error: true - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 with: name: cursor-review-ledger path: /tmp/ledger @@ -1505,7 +1559,7 @@ jobs: - name: Download prior-review ledger id: ledger_download continue-on-error: true - uses: actions/download-artifact@v8 + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 with: name: cursor-review-ledger path: /tmp/ledger diff --git a/.github/workflows/groom.yml b/.github/workflows/groom.yml index 97e82030..256fcb6c 100644 --- a/.github/workflows/groom.yml +++ b/.github/workflows/groom.yml @@ -112,7 +112,8 @@ name: Groom (reusable) # # Post/act issues as cloud-code-bot instead of github-actions[bot]. # bot_app_id: ${{ vars.APP_ID }} # # No workflows_ref: the briefs + ledger auto-load from the same commit -# # this `uses:` pin resolves to (github.job_workflow_sha). Set it only to +# # this `uses:` pin resolves to (job.workflow_sha — the runner v2.334.0+ +# # accessor, not the empty `github.job_workflow_sha`). Set it only to # # test briefs from a branch. # dry_run: ${{ github.event.inputs.dry_run == 'true' }} # # Forward the optional scoping input above. A schedule event carries no @@ -342,11 +343,19 @@ on: description: >- Ref of Comfy-Org/github-workflows to load the groom briefs + ledger from. LEAVE UNSET (the default): the assets are then checked out from - `github.job_workflow_sha` — the exact commit this reusable workflow was + `job.workflow_sha` — the exact commit this reusable workflow was resolved from via the caller's `uses:` pin — so the briefs + ledger always match the logic running them, with nothing for the caller to keep in sync. Set this ONLY to override that (e.g. test briefs from a - branch while pinning `uses:` elsewhere). + branch while pinning `uses:` elsewhere). It is `job.workflow_sha`, the + `job`-context accessor added in runner v2.334.0 — NOT the + `github.job_workflow_sha` this used to name, which is an OIDC token + claim rather than a `github` context property and expands to '' (the + silent default-branch checkout BE-8077 fixed). Every job here fails + closed if it resolves empty, so an unset input can no longer degrade to + this repo's default branch. (actionlint <= 1.7.12 false-positives on + `job.workflow_sha` — its `job`-context schema predates the runner + release; nothing in this repo's CI runs actionlint.) type: string required: false default: '' @@ -607,6 +616,60 @@ jobs: # steps. See the resolve step below for why the pin lives in a manifest. claude_code_version: ${{ steps.claude_version.outputs.version }} steps: + - name: Require a resolvable workflows_ref + # The asset checkout below falls back to `job.workflow_sha` when the + # caller omits `workflows_ref` — the commit the caller's `uses:` pin + # resolved to, so the assets always match the logic loading them. That + # accessor arrived with runner v2.334.0 (Apr 2026); on anything older + # Actions expands an unknown context property to '' rather than erroring, + # and `actions/checkout` reads `ref: ''` as "this repo's DEFAULT BRANCH" — + # loading mutable briefs and scripts into a job that holds the caller's + # ANTHROPIC_API_KEY and App token. Fail closed instead. Every job here is + # `ubuntu-latest`, which is always current, so only a stale self-hosted + # runner can trip this. (BE-8077, the same shape as cursor-review.yml's + # BE-5546 guard for the required-input case.) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }} + run: | + # Bracket RANGES (`A-Z`) match by COLLATION order, so under a glibc + # locale such as en_US.UTF-8 they also match accented letters and the + # negated class below quietly stops rejecting them. Hosted runners + # default to C.UTF-8, but the case this guard exists for is the stale + # SELF-HOSTED runner, whose locale is unknown — so pin the byte order + # rather than inherit it. + export LC_ALL=C + # actions/checkout reads `ref` through core.getInput, which TRIMS, so a + # whitespace-only value is an empty ref to IT while sailing past a bare + # -z test here. Compare the stripped form: dropping newlines also stops + # a multi-line value from smuggling a ::workflow command:: into the log. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::Cannot resolve a ref for the groom assets: the workflows_ref input is empty AND job.workflow_sha resolved to ''. job.workflow_sha needs an Actions runner >= v2.334.0, so a runner older than that is the only way this happens. Upgrade the runner, or set workflows_ref: to the same commit SHA the caller's uses: line pins." + exit 1 + fi + # …but POSIX `[:space:]` is ASCII-only, while the `trim()` behind + # core.getInput also strips U+00A0, U+FEFF and friends. A workflows_ref + # made purely of those survives the test above and still reaches + # checkout as '' — this repo's DEFAULT BRANCH — which is the precise + # case this guard exists to fail closed on. The checkout below consumes + # the RAW expression, not "$REF", so the value has to be REJECTED here + # rather than sanitized. Test the shape positively: a git ref is ASCII, + # so anything outside this class fails closed, which also covers the + # embedded space/newline case the comment above only half-addressed. + # The class is deliberately NARROWER than git's own ref rules, which + # also permit `(`, `)`, `,`, `%` and UTF-8 — so a legal branch like + # `feat(groom)/x` is rejected here. That is a policy call, not a + # validity claim, and the message says so: it excludes every character + # that means something to a shell or to a `::workflow command::`, and + # it still covers everything this input is meant to carry — a 40-hex + # SHA, or a branch/tag for the documented brief-testing override. + case "$WORKFLOWS_REF" in + *[!A-Za-z0-9._/@+-]*) + echo "::error::workflows_ref contains characters outside the set this workflow accepts (A-Z a-z 0-9 . _ / - @ +). That is a deliberate POLICY restriction, narrower than git's own ref rules — not a claim that your ref is invalid: actions/checkout trims Unicode whitespace a shell emptiness test cannot see, and a value it trims to empty would silently load the groom assets from this repo's default branch. Set workflows_ref: to the same commit SHA the caller's uses: line pins (the documented form), or rename the branch if you are using the brief-testing override." + exit 1 + ;; + esac + - name: Load groom assets (interval gate) # interval.py is loaded from THIS repo at the pinned ref — a single # source of truth like the briefs + ledger, never the target checkout. @@ -614,9 +677,10 @@ jobs: with: repository: Comfy-Org/github-workflows # Default to the exact commit THIS reusable workflow was resolved from - # (github.job_workflow_sha), so interval.py can't desync from the logic - # loading it; workflows_ref is an explicit override only. - ref: ${{ inputs.workflows_ref || github.job_workflow_sha }} + # (`job.workflow_sha`, NOT `github.job_workflow_sha` — see the guard + # above), so interval.py can't desync from the logic loading it; + # workflows_ref is an explicit override only. + ref: ${{ inputs.workflows_ref || job.workflow_sha }} path: _groom_assets persist-credentials: false @@ -859,16 +923,21 @@ jobs: - name: Load the agent CLI pin # Deliberately NOT from `$GROOM_ASSETS` (which tracks `inputs.workflows_ref`, - # a `required: false` input defaulting to the mutable `main`). Two reasons, - # both about this being executable supply chain rather than a brief: + # a `required: false` input a caller may legitimately point at a mutable + # BRANCH). Two reasons, both about this being executable supply chain + # rather than a brief: # - # 1. A caller that SHA-pins `uses:` but omits `workflows_ref` would have the - # CLI version — and therefore what actually executes inside the three - # agent jobs — governed by a branch tip, while the sandbox flags those - # jobs pass are frozen at the pinned `uses:` SHA. The two could diverge. + # 1. A caller that SHA-pins `uses:` but sets `workflows_ref` to a branch — + # the documented brief-testing override — would have the CLI version, + # and therefore what actually executes inside the three agent jobs, + # governed by a branch tip, while the sandbox flags those jobs pass are + # frozen at the pinned `uses:` SHA. The two could diverge. # `job.workflow_sha` IS the commit this very workflow file was read # from, so the pin and the flags that depend on it are always the same - # commit, and SHA-pinning `uses:` alone fully pins the CLI. + # commit, and SHA-pinning `uses:` alone fully pins the CLI. (Since + # BE-8077 the asset checkouts fall back to `job.workflow_sha` too when + # the input is OMITTED, so on that path the two now agree by + # construction — this step simply never depended on them agreeing.) # 2. The resolve step below fails CLOSED on a missing manifest. Read from # `workflows_ref`, that turns this repo's documented split-pin state # (Dependabot moves `uses:` and leaves `workflows_ref:` behind — see @@ -935,11 +1004,14 @@ jobs: run: | set -euo pipefail # WARN rather than fail: an empty `job.workflow_sha` means the checkout - # above silently fell back to the default branch, which is no worse than - # the pre-BE-5373 behaviour (`workflows_ref` also defaults to `main`) and - # is not worth turning into a groom outage. But it does mean the caller's - # `uses:` SHA no longer pins the CLI, so it must not pass unremarked — - # this is precisely the silent-empty trap `github.job_workflow_sha` sets. + # above silently fell back to the default branch. Since BE-8077 that can + # only happen when the caller DID set `workflows_ref` (an explicitly + # pinned run on a pre-v2.334.0 runner) — with the input omitted, this + # job's `Require a resolvable workflows_ref` guard has already failed the + # run closed. Degrading the CLI pin is not worth turning that narrower + # case into a groom outage, but the caller's `uses:` SHA no longer pins + # the CLI, so it must not pass unremarked — this is precisely the + # silent-empty trap `github.job_workflow_sha` sets on every runner. if [ -z "${WORKFLOW_SHA:-}" ]; then echo "::warning::job.workflow_sha is empty, so the agent CLI pin was read from the default branch rather than the ref this workflow was called at. The pin is still validated, but the caller's uses: SHA no longer pins it." fi @@ -1045,6 +1117,60 @@ jobs: persist-credentials: false path: repo + - name: Require a resolvable workflows_ref + # The asset checkout below falls back to `job.workflow_sha` when the + # caller omits `workflows_ref` — the commit the caller's `uses:` pin + # resolved to, so the assets always match the logic loading them. That + # accessor arrived with runner v2.334.0 (Apr 2026); on anything older + # Actions expands an unknown context property to '' rather than erroring, + # and `actions/checkout` reads `ref: ''` as "this repo's DEFAULT BRANCH" — + # loading mutable briefs and scripts into a job that holds the caller's + # ANTHROPIC_API_KEY and App token. Fail closed instead. Every job here is + # `ubuntu-latest`, which is always current, so only a stale self-hosted + # runner can trip this. (BE-8077, the same shape as cursor-review.yml's + # BE-5546 guard for the required-input case.) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }} + run: | + # Bracket RANGES (`A-Z`) match by COLLATION order, so under a glibc + # locale such as en_US.UTF-8 they also match accented letters and the + # negated class below quietly stops rejecting them. Hosted runners + # default to C.UTF-8, but the case this guard exists for is the stale + # SELF-HOSTED runner, whose locale is unknown — so pin the byte order + # rather than inherit it. + export LC_ALL=C + # actions/checkout reads `ref` through core.getInput, which TRIMS, so a + # whitespace-only value is an empty ref to IT while sailing past a bare + # -z test here. Compare the stripped form: dropping newlines also stops + # a multi-line value from smuggling a ::workflow command:: into the log. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::Cannot resolve a ref for the groom assets: the workflows_ref input is empty AND job.workflow_sha resolved to ''. job.workflow_sha needs an Actions runner >= v2.334.0, so a runner older than that is the only way this happens. Upgrade the runner, or set workflows_ref: to the same commit SHA the caller's uses: line pins." + exit 1 + fi + # …but POSIX `[:space:]` is ASCII-only, while the `trim()` behind + # core.getInput also strips U+00A0, U+FEFF and friends. A workflows_ref + # made purely of those survives the test above and still reaches + # checkout as '' — this repo's DEFAULT BRANCH — which is the precise + # case this guard exists to fail closed on. The checkout below consumes + # the RAW expression, not "$REF", so the value has to be REJECTED here + # rather than sanitized. Test the shape positively: a git ref is ASCII, + # so anything outside this class fails closed, which also covers the + # embedded space/newline case the comment above only half-addressed. + # The class is deliberately NARROWER than git's own ref rules, which + # also permit `(`, `)`, `,`, `%` and UTF-8 — so a legal branch like + # `feat(groom)/x` is rejected here. That is a policy call, not a + # validity claim, and the message says so: it excludes every character + # that means something to a shell or to a `::workflow command::`, and + # it still covers everything this input is meant to carry — a 40-hex + # SHA, or a branch/tag for the documented brief-testing override. + case "$WORKFLOWS_REF" in + *[!A-Za-z0-9._/@+-]*) + echo "::error::workflows_ref contains characters outside the set this workflow accepts (A-Z a-z 0-9 . _ / - @ +). That is a deliberate POLICY restriction, narrower than git's own ref rules — not a claim that your ref is invalid: actions/checkout trims Unicode whitespace a shell emptiness test cannot see, and a value it trims to empty would silently load the groom assets from this repo's default branch. Set workflows_ref: to the same commit SHA the caller's uses: line pins (the documented form), or rename the branch if you are using the brief-testing override." + exit 1 + ;; + esac + - name: Load groom assets (briefs) # Trusted briefs come from THIS workflow's repo (public, pinned to the # workflow's own resolved commit) — never from the target checkout, so a @@ -1053,9 +1179,10 @@ jobs: with: repository: Comfy-Org/github-workflows # Default to the exact commit THIS reusable workflow was resolved from - # (github.job_workflow_sha), so the briefs + ledger can't desync from - # the logic loading them; workflows_ref is an explicit override only. - ref: ${{ inputs.workflows_ref || github.job_workflow_sha }} + # (`job.workflow_sha`, NOT `github.job_workflow_sha` — see the guard + # above), so the briefs + ledger can't desync from the logic loading + # them; workflows_ref is an explicit override only. + ref: ${{ inputs.workflows_ref || job.workflow_sha }} path: _groom_assets persist-credentials: false @@ -1466,6 +1593,60 @@ jobs: persist-credentials: false path: repo + - name: Require a resolvable workflows_ref + # The asset checkout below falls back to `job.workflow_sha` when the + # caller omits `workflows_ref` — the commit the caller's `uses:` pin + # resolved to, so the assets always match the logic loading them. That + # accessor arrived with runner v2.334.0 (Apr 2026); on anything older + # Actions expands an unknown context property to '' rather than erroring, + # and `actions/checkout` reads `ref: ''` as "this repo's DEFAULT BRANCH" — + # loading mutable briefs and scripts into a job that holds the caller's + # ANTHROPIC_API_KEY and App token. Fail closed instead. Every job here is + # `ubuntu-latest`, which is always current, so only a stale self-hosted + # runner can trip this. (BE-8077, the same shape as cursor-review.yml's + # BE-5546 guard for the required-input case.) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }} + run: | + # Bracket RANGES (`A-Z`) match by COLLATION order, so under a glibc + # locale such as en_US.UTF-8 they also match accented letters and the + # negated class below quietly stops rejecting them. Hosted runners + # default to C.UTF-8, but the case this guard exists for is the stale + # SELF-HOSTED runner, whose locale is unknown — so pin the byte order + # rather than inherit it. + export LC_ALL=C + # actions/checkout reads `ref` through core.getInput, which TRIMS, so a + # whitespace-only value is an empty ref to IT while sailing past a bare + # -z test here. Compare the stripped form: dropping newlines also stops + # a multi-line value from smuggling a ::workflow command:: into the log. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::Cannot resolve a ref for the groom assets: the workflows_ref input is empty AND job.workflow_sha resolved to ''. job.workflow_sha needs an Actions runner >= v2.334.0, so a runner older than that is the only way this happens. Upgrade the runner, or set workflows_ref: to the same commit SHA the caller's uses: line pins." + exit 1 + fi + # …but POSIX `[:space:]` is ASCII-only, while the `trim()` behind + # core.getInput also strips U+00A0, U+FEFF and friends. A workflows_ref + # made purely of those survives the test above and still reaches + # checkout as '' — this repo's DEFAULT BRANCH — which is the precise + # case this guard exists to fail closed on. The checkout below consumes + # the RAW expression, not "$REF", so the value has to be REJECTED here + # rather than sanitized. Test the shape positively: a git ref is ASCII, + # so anything outside this class fails closed, which also covers the + # embedded space/newline case the comment above only half-addressed. + # The class is deliberately NARROWER than git's own ref rules, which + # also permit `(`, `)`, `,`, `%` and UTF-8 — so a legal branch like + # `feat(groom)/x` is rejected here. That is a policy call, not a + # validity claim, and the message says so: it excludes every character + # that means something to a shell or to a `::workflow command::`, and + # it still covers everything this input is meant to carry — a 40-hex + # SHA, or a branch/tag for the documented brief-testing override. + case "$WORKFLOWS_REF" in + *[!A-Za-z0-9._/@+-]*) + echo "::error::workflows_ref contains characters outside the set this workflow accepts (A-Z a-z 0-9 . _ / - @ +). That is a deliberate POLICY restriction, narrower than git's own ref rules — not a claim that your ref is invalid: actions/checkout trims Unicode whitespace a shell emptiness test cannot see, and a value it trims to empty would silently load the groom assets from this repo's default branch. Set workflows_ref: to the same commit SHA the caller's uses: line pins (the documented form), or rename the branch if you are using the brief-testing override." + exit 1 + ;; + esac + - name: Load groom assets (briefs) # Re-fetched fresh from THIS repo — never inherited from the finder job, # so a tampered brief cannot cross the job boundary. @@ -1473,9 +1654,10 @@ jobs: with: repository: Comfy-Org/github-workflows # Default to the exact commit THIS reusable workflow was resolved from - # (github.job_workflow_sha), so the briefs + ledger can't desync from - # the logic loading them; workflows_ref is an explicit override only. - ref: ${{ inputs.workflows_ref || github.job_workflow_sha }} + # (`job.workflow_sha`, NOT `github.job_workflow_sha` — see the guard + # above), so the briefs + ledger can't desync from the logic loading + # them; workflows_ref is an explicit override only. + ref: ${{ inputs.workflows_ref || job.workflow_sha }} path: _groom_assets persist-credentials: false @@ -1820,14 +2002,69 @@ jobs: permission-issues: read permission-pull-requests: read + - name: Require a resolvable workflows_ref + # The asset checkout below falls back to `job.workflow_sha` when the + # caller omits `workflows_ref` — the commit the caller's `uses:` pin + # resolved to, so the assets always match the logic loading them. That + # accessor arrived with runner v2.334.0 (Apr 2026); on anything older + # Actions expands an unknown context property to '' rather than erroring, + # and `actions/checkout` reads `ref: ''` as "this repo's DEFAULT BRANCH" — + # loading mutable briefs and scripts into a job that holds the caller's + # ANTHROPIC_API_KEY and App token. Fail closed instead. Every job here is + # `ubuntu-latest`, which is always current, so only a stale self-hosted + # runner can trip this. (BE-8077, the same shape as cursor-review.yml's + # BE-5546 guard for the required-input case.) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }} + run: | + # Bracket RANGES (`A-Z`) match by COLLATION order, so under a glibc + # locale such as en_US.UTF-8 they also match accented letters and the + # negated class below quietly stops rejecting them. Hosted runners + # default to C.UTF-8, but the case this guard exists for is the stale + # SELF-HOSTED runner, whose locale is unknown — so pin the byte order + # rather than inherit it. + export LC_ALL=C + # actions/checkout reads `ref` through core.getInput, which TRIMS, so a + # whitespace-only value is an empty ref to IT while sailing past a bare + # -z test here. Compare the stripped form: dropping newlines also stops + # a multi-line value from smuggling a ::workflow command:: into the log. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::Cannot resolve a ref for the groom assets: the workflows_ref input is empty AND job.workflow_sha resolved to ''. job.workflow_sha needs an Actions runner >= v2.334.0, so a runner older than that is the only way this happens. Upgrade the runner, or set workflows_ref: to the same commit SHA the caller's uses: line pins." + exit 1 + fi + # …but POSIX `[:space:]` is ASCII-only, while the `trim()` behind + # core.getInput also strips U+00A0, U+FEFF and friends. A workflows_ref + # made purely of those survives the test above and still reaches + # checkout as '' — this repo's DEFAULT BRANCH — which is the precise + # case this guard exists to fail closed on. The checkout below consumes + # the RAW expression, not "$REF", so the value has to be REJECTED here + # rather than sanitized. Test the shape positively: a git ref is ASCII, + # so anything outside this class fails closed, which also covers the + # embedded space/newline case the comment above only half-addressed. + # The class is deliberately NARROWER than git's own ref rules, which + # also permit `(`, `)`, `,`, `%` and UTF-8 — so a legal branch like + # `feat(groom)/x` is rejected here. That is a policy call, not a + # validity claim, and the message says so: it excludes every character + # that means something to a shell or to a `::workflow command::`, and + # it still covers everything this input is meant to carry — a 40-hex + # SHA, or a branch/tag for the documented brief-testing override. + case "$WORKFLOWS_REF" in + *[!A-Za-z0-9._/@+-]*) + echo "::error::workflows_ref contains characters outside the set this workflow accepts (A-Z a-z 0-9 . _ / - @ +). That is a deliberate POLICY restriction, narrower than git's own ref rules — not a claim that your ref is invalid: actions/checkout trims Unicode whitespace a shell emptiness test cannot see, and a value it trims to empty would silently load the groom assets from this repo's default branch. Set workflows_ref: to the same commit SHA the caller's uses: line pins (the documented form), or rename the branch if you are using the brief-testing override." + exit 1 + ;; + esac + - name: Load groom assets (ledger) uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: repository: Comfy-Org/github-workflows # Default to the exact commit THIS reusable workflow was resolved from - # (github.job_workflow_sha), so the briefs + ledger can't desync from - # the logic loading them; workflows_ref is an explicit override only. - ref: ${{ inputs.workflows_ref || github.job_workflow_sha }} + # (`job.workflow_sha`, NOT `github.job_workflow_sha` — see the guard + # above), so the briefs + ledger can't desync from the logic loading + # them; workflows_ref is an explicit override only. + ref: ${{ inputs.workflows_ref || job.workflow_sha }} path: _groom_assets persist-credentials: false @@ -1968,14 +2205,69 @@ jobs: # Least privilege: this job only opens `groom` issues (+ their labels). permission-issues: write + - name: Require a resolvable workflows_ref + # The asset checkout below falls back to `job.workflow_sha` when the + # caller omits `workflows_ref` — the commit the caller's `uses:` pin + # resolved to, so the assets always match the logic loading them. That + # accessor arrived with runner v2.334.0 (Apr 2026); on anything older + # Actions expands an unknown context property to '' rather than erroring, + # and `actions/checkout` reads `ref: ''` as "this repo's DEFAULT BRANCH" — + # loading mutable briefs and scripts into a job that holds the caller's + # ANTHROPIC_API_KEY and App token. Fail closed instead. Every job here is + # `ubuntu-latest`, which is always current, so only a stale self-hosted + # runner can trip this. (BE-8077, the same shape as cursor-review.yml's + # BE-5546 guard for the required-input case.) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }} + run: | + # Bracket RANGES (`A-Z`) match by COLLATION order, so under a glibc + # locale such as en_US.UTF-8 they also match accented letters and the + # negated class below quietly stops rejecting them. Hosted runners + # default to C.UTF-8, but the case this guard exists for is the stale + # SELF-HOSTED runner, whose locale is unknown — so pin the byte order + # rather than inherit it. + export LC_ALL=C + # actions/checkout reads `ref` through core.getInput, which TRIMS, so a + # whitespace-only value is an empty ref to IT while sailing past a bare + # -z test here. Compare the stripped form: dropping newlines also stops + # a multi-line value from smuggling a ::workflow command:: into the log. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::Cannot resolve a ref for the groom assets: the workflows_ref input is empty AND job.workflow_sha resolved to ''. job.workflow_sha needs an Actions runner >= v2.334.0, so a runner older than that is the only way this happens. Upgrade the runner, or set workflows_ref: to the same commit SHA the caller's uses: line pins." + exit 1 + fi + # …but POSIX `[:space:]` is ASCII-only, while the `trim()` behind + # core.getInput also strips U+00A0, U+FEFF and friends. A workflows_ref + # made purely of those survives the test above and still reaches + # checkout as '' — this repo's DEFAULT BRANCH — which is the precise + # case this guard exists to fail closed on. The checkout below consumes + # the RAW expression, not "$REF", so the value has to be REJECTED here + # rather than sanitized. Test the shape positively: a git ref is ASCII, + # so anything outside this class fails closed, which also covers the + # embedded space/newline case the comment above only half-addressed. + # The class is deliberately NARROWER than git's own ref rules, which + # also permit `(`, `)`, `,`, `%` and UTF-8 — so a legal branch like + # `feat(groom)/x` is rejected here. That is a policy call, not a + # validity claim, and the message says so: it excludes every character + # that means something to a shell or to a `::workflow command::`, and + # it still covers everything this input is meant to carry — a 40-hex + # SHA, or a branch/tag for the documented brief-testing override. + case "$WORKFLOWS_REF" in + *[!A-Za-z0-9._/@+-]*) + echo "::error::workflows_ref contains characters outside the set this workflow accepts (A-Z a-z 0-9 . _ / - @ +). That is a deliberate POLICY restriction, narrower than git's own ref rules — not a claim that your ref is invalid: actions/checkout trims Unicode whitespace a shell emptiness test cannot see, and a value it trims to empty would silently load the groom assets from this repo's default branch. Set workflows_ref: to the same commit SHA the caller's uses: line pins (the documented form), or rename the branch if you are using the brief-testing override." + exit 1 + ;; + esac + - name: Load groom assets (ledger) uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: repository: Comfy-Org/github-workflows # Default to the exact commit THIS reusable workflow was resolved from - # (github.job_workflow_sha), so the briefs + ledger can't desync from - # the logic loading them; workflows_ref is an explicit override only. - ref: ${{ inputs.workflows_ref || github.job_workflow_sha }} + # (`job.workflow_sha`, NOT `github.job_workflow_sha` — see the guard + # above), so the briefs + ledger can't desync from the logic loading + # them; workflows_ref is an explicit override only. + ref: ${{ inputs.workflows_ref || job.workflow_sha }} path: _groom_assets persist-credentials: false @@ -2157,14 +2449,69 @@ jobs: persist-credentials: false path: repo + - name: Require a resolvable workflows_ref + # The asset checkout below falls back to `job.workflow_sha` when the + # caller omits `workflows_ref` — the commit the caller's `uses:` pin + # resolved to, so the assets always match the logic loading them. That + # accessor arrived with runner v2.334.0 (Apr 2026); on anything older + # Actions expands an unknown context property to '' rather than erroring, + # and `actions/checkout` reads `ref: ''` as "this repo's DEFAULT BRANCH" — + # loading mutable briefs and scripts into a job that holds the caller's + # ANTHROPIC_API_KEY and App token. Fail closed instead. Every job here is + # `ubuntu-latest`, which is always current, so only a stale self-hosted + # runner can trip this. (BE-8077, the same shape as cursor-review.yml's + # BE-5546 guard for the required-input case.) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }} + run: | + # Bracket RANGES (`A-Z`) match by COLLATION order, so under a glibc + # locale such as en_US.UTF-8 they also match accented letters and the + # negated class below quietly stops rejecting them. Hosted runners + # default to C.UTF-8, but the case this guard exists for is the stale + # SELF-HOSTED runner, whose locale is unknown — so pin the byte order + # rather than inherit it. + export LC_ALL=C + # actions/checkout reads `ref` through core.getInput, which TRIMS, so a + # whitespace-only value is an empty ref to IT while sailing past a bare + # -z test here. Compare the stripped form: dropping newlines also stops + # a multi-line value from smuggling a ::workflow command:: into the log. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::Cannot resolve a ref for the groom assets: the workflows_ref input is empty AND job.workflow_sha resolved to ''. job.workflow_sha needs an Actions runner >= v2.334.0, so a runner older than that is the only way this happens. Upgrade the runner, or set workflows_ref: to the same commit SHA the caller's uses: line pins." + exit 1 + fi + # …but POSIX `[:space:]` is ASCII-only, while the `trim()` behind + # core.getInput also strips U+00A0, U+FEFF and friends. A workflows_ref + # made purely of those survives the test above and still reaches + # checkout as '' — this repo's DEFAULT BRANCH — which is the precise + # case this guard exists to fail closed on. The checkout below consumes + # the RAW expression, not "$REF", so the value has to be REJECTED here + # rather than sanitized. Test the shape positively: a git ref is ASCII, + # so anything outside this class fails closed, which also covers the + # embedded space/newline case the comment above only half-addressed. + # The class is deliberately NARROWER than git's own ref rules, which + # also permit `(`, `)`, `,`, `%` and UTF-8 — so a legal branch like + # `feat(groom)/x` is rejected here. That is a policy call, not a + # validity claim, and the message says so: it excludes every character + # that means something to a shell or to a `::workflow command::`, and + # it still covers everything this input is meant to carry — a 40-hex + # SHA, or a branch/tag for the documented brief-testing override. + case "$WORKFLOWS_REF" in + *[!A-Za-z0-9._/@+-]*) + echo "::error::workflows_ref contains characters outside the set this workflow accepts (A-Z a-z 0-9 . _ / - @ +). That is a deliberate POLICY restriction, narrower than git's own ref rules — not a claim that your ref is invalid: actions/checkout trims Unicode whitespace a shell emptiness test cannot see, and a value it trims to empty would silently load the groom assets from this repo's default branch. Set workflows_ref: to the same commit SHA the caller's uses: line pins (the documented form), or rename the branch if you are using the brief-testing override." + exit 1 + ;; + esac + - name: Load groom assets (builder brief) uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: repository: Comfy-Org/github-workflows # Default to the exact commit THIS reusable workflow was resolved from - # (github.job_workflow_sha), so the briefs + ledger can't desync from - # the logic loading them; workflows_ref is an explicit override only. - ref: ${{ inputs.workflows_ref || github.job_workflow_sha }} + # (`job.workflow_sha`, NOT `github.job_workflow_sha` — see the guard + # above), so the briefs + ledger can't desync from the logic loading + # them; workflows_ref is an explicit override only. + ref: ${{ inputs.workflows_ref || job.workflow_sha }} path: _groom_assets persist-credentials: false @@ -2597,14 +2944,69 @@ jobs: permission-pull-requests: write permission-issues: write + - name: Require a resolvable workflows_ref + # The asset checkout below falls back to `job.workflow_sha` when the + # caller omits `workflows_ref` — the commit the caller's `uses:` pin + # resolved to, so the assets always match the logic loading them. That + # accessor arrived with runner v2.334.0 (Apr 2026); on anything older + # Actions expands an unknown context property to '' rather than erroring, + # and `actions/checkout` reads `ref: ''` as "this repo's DEFAULT BRANCH" — + # loading mutable briefs and scripts into a job that holds the caller's + # ANTHROPIC_API_KEY and App token. Fail closed instead. Every job here is + # `ubuntu-latest`, which is always current, so only a stale self-hosted + # runner can trip this. (BE-8077, the same shape as cursor-review.yml's + # BE-5546 guard for the required-input case.) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }} + run: | + # Bracket RANGES (`A-Z`) match by COLLATION order, so under a glibc + # locale such as en_US.UTF-8 they also match accented letters and the + # negated class below quietly stops rejecting them. Hosted runners + # default to C.UTF-8, but the case this guard exists for is the stale + # SELF-HOSTED runner, whose locale is unknown — so pin the byte order + # rather than inherit it. + export LC_ALL=C + # actions/checkout reads `ref` through core.getInput, which TRIMS, so a + # whitespace-only value is an empty ref to IT while sailing past a bare + # -z test here. Compare the stripped form: dropping newlines also stops + # a multi-line value from smuggling a ::workflow command:: into the log. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::Cannot resolve a ref for the groom assets: the workflows_ref input is empty AND job.workflow_sha resolved to ''. job.workflow_sha needs an Actions runner >= v2.334.0, so a runner older than that is the only way this happens. Upgrade the runner, or set workflows_ref: to the same commit SHA the caller's uses: line pins." + exit 1 + fi + # …but POSIX `[:space:]` is ASCII-only, while the `trim()` behind + # core.getInput also strips U+00A0, U+FEFF and friends. A workflows_ref + # made purely of those survives the test above and still reaches + # checkout as '' — this repo's DEFAULT BRANCH — which is the precise + # case this guard exists to fail closed on. The checkout below consumes + # the RAW expression, not "$REF", so the value has to be REJECTED here + # rather than sanitized. Test the shape positively: a git ref is ASCII, + # so anything outside this class fails closed, which also covers the + # embedded space/newline case the comment above only half-addressed. + # The class is deliberately NARROWER than git's own ref rules, which + # also permit `(`, `)`, `,`, `%` and UTF-8 — so a legal branch like + # `feat(groom)/x` is rejected here. That is a policy call, not a + # validity claim, and the message says so: it excludes every character + # that means something to a shell or to a `::workflow command::`, and + # it still covers everything this input is meant to carry — a 40-hex + # SHA, or a branch/tag for the documented brief-testing override. + case "$WORKFLOWS_REF" in + *[!A-Za-z0-9._/@+-]*) + echo "::error::workflows_ref contains characters outside the set this workflow accepts (A-Z a-z 0-9 . _ / - @ +). That is a deliberate POLICY restriction, narrower than git's own ref rules — not a claim that your ref is invalid: actions/checkout trims Unicode whitespace a shell emptiness test cannot see, and a value it trims to empty would silently load the groom assets from this repo's default branch. Set workflows_ref: to the same commit SHA the caller's uses: line pins (the documented form), or rename the branch if you are using the brief-testing override." + exit 1 + ;; + esac + - name: Load groom assets (signature marker) uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: repository: Comfy-Org/github-workflows # Default to the exact commit THIS reusable workflow was resolved from - # (github.job_workflow_sha), so the briefs + ledger can't desync from - # the logic loading them; workflows_ref is an explicit override only. - ref: ${{ inputs.workflows_ref || github.job_workflow_sha }} + # (`job.workflow_sha`, NOT `github.job_workflow_sha` — see the guard + # above), so the briefs + ledger can't desync from the logic loading + # them; workflows_ref is an explicit override only. + ref: ${{ inputs.workflows_ref || job.workflow_sha }} path: _groom_assets persist-credentials: false diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index 53c07c43..d239740b 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -36,11 +36,15 @@ name: PR Risk Grade (reusable) # # WHAT ANCESTRY STILL DOES NOT PROVE is that the pin is the CURRENT one. A stale pin, left behind # when `uses:` moved, is a merged ancestor too and passes. The check that would close that is -# "`workflows_ref` equals the commit the caller's `uses:` resolved to", and the runner does NOT -# expose that commit to the workflow — `github.workflow_sha` is the CALLER's top-level workflow -# file, and `job_workflow_sha` (the value that would answer this) exists only as an OIDC token -# claim, which would mean an `id-token: write` grant from every caller and a token exchange in a job -# that today holds `permissions: {}`. The other residual is narrower and irreducible: a fork commit +# "`workflows_ref` equals the commit the caller's `uses:` resolved to". That commit IS reachable, +# just not the two ways this comment used to say it was not: `github.workflow_sha` is the CALLER's +# top-level workflow file, and the `job_workflow_sha` OIDC claim would need an `id-token: write` +# grant from every caller plus a token exchange in a job that today holds `permissions: {}` — but +# `job.workflow_sha`, the `job`-context accessor added in runner v2.334.0 (Apr 2026), needs neither +# and is what groom.yml reads (BE-8077). So this is a WIRING gap, not a runner limitation. It is +# deliberately still open: asserting equality here would fail red on every caller whose `uses:` and +# `workflows_ref` have drifted apart, which is a caller-contract change tracked separately. Do not +# assume this pin is proven CURRENT. The other residual is narrower and irreducible: a fork commit # that edits THIS FILE. Such a commit is not an ancestor of upstream `main`, so the axis rejects it # on the way in — but a caller pinned at one would be running the fork's own copy of this guard, and # no check written inside a file can bound a revision of that file chosen by an attacker. Both @@ -430,13 +434,15 @@ on: WHAT ANCESTRY DOES NOT PROVE is that the pin is the CURRENT one: a pin left behind when `uses:` moved is a merged ancestor too, and passes. The test that would close that is "equal to the commit `uses:` resolved - to for this job" — and the runner does not expose that commit to the - workflow. `github.workflow_sha` is the CALLER's top-level workflow - file, not this one; the value that would answer it, `job_workflow_sha`, - exists only as an OIDC token claim, which means an `id-token: write` - grant from every caller plus a token exchange in a job that today holds - `permissions: {}`. Adding that is a caller-contract change and is - tracked separately — do not assume this pin is proven CURRENT. The + to for this job". `github.workflow_sha` is the CALLER's top-level + workflow file, not this one, and the `job_workflow_sha` OIDC claim would + need an `id-token: write` grant from every caller plus a token exchange + in a job that today holds `permissions: {}` — but `job.workflow_sha`, + the `job`-context accessor added in runner v2.334.0, is neither of those + and answers it directly (BE-8077; groom.yml reads it). Wiring it in is + still a caller-contract change — every split pin would start failing red + — so it is tracked separately and NOT done: do not assume this pin is + proven CURRENT. The other residual is narrower and irreducible: a fork commit that edits `pr-risk.yml` itself is not an ancestor of upstream `main`, so it is rejected on the way in, but a caller pinned at one would be running the @@ -544,13 +550,15 @@ jobs: # a pin left behind when `uses:` moved is too. The ancestry check ~50 lines below closes the # FIRST case — a fork-authored commit is not, in general, an ancestor of this repo's own # `main` — leaving only the stale-but-merged-pin case, which needs "equal to the commit - # `uses:` resolved to for this job." That remains unavailable: the runner does not hand this - # job that commit. `github.workflow_sha` is the CALLER's top-level workflow file; - # `job_workflow_sha`, which is the value that would answer it, is an OIDC token CLAIM, not a - # `github` context property (`actionlint` rejects `github.job_workflow_sha`, and reading the - # claim needs `id-token: write` from every caller plus an exchange in a job that holds - # `permissions: {}` today). Reviewing the caller is what bounds that residual, and it is the - # only thing that can. + # `uses:` resolved to for this job." `github.workflow_sha` is the CALLER's top-level workflow + # file; `job_workflow_sha` is an OIDC token CLAIM, not a `github` context property (which is + # why `actionlint` rejects `github.job_workflow_sha`), and reading the claim needs + # `id-token: write` from every caller plus an exchange in a job that holds `permissions: {}` + # today. The value IS available as `job.workflow_sha` — the `job`-context accessor added in + # runner v2.334.0 (Apr 2026), which groom.yml reads and which needs no extra grant (BE-8077). + # Asserting equality against it is deliberately NOT done here: it would fail red on every + # caller whose `uses:` and `workflows_ref` have drifted, so it is a caller-contract change + # tracked separately. Until then, reviewing the caller is what bounds that residual. # # Enforced in the WORKFLOW, not in a script: the scripts are what the ref loads, so a # script-side check would sit inside the blast radius it is meant to bound. EVERY job that diff --git a/AGENTS.md b/AGENTS.md index 553a42fe..a5097d1c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -97,8 +97,9 @@ tests — run the matching command above for whatever you touched. and rewrites just the reviewer lists for a drift PR. Tests in `tests/`. - `.github/workflow-pins/` — `check_workflow_pins.py` + `tests/`: the lint forbidding a `default:` on `workflows_ref` and requiring the empty-ref guard - at every checkout, with an exception for the `github.job_workflow_sha` - self-pin (BE-4169, see below). + at every checkout. The `job.workflow_sha` self-pin is exempt from the + `default:` half only — it stops the ref being MUTABLE, not being EMPTY, so it + still has to carry a guard (BE-4169, corrected by BE-8077; see below). - `.github/bump-callers/` — `bump-callers.sh`, the ONE fleet-agnostic script that opens SHA-bump PRs in consumer repos when a reusable workflow changes, plus `preflight.sh` (BE-6475), the ONE staleness/decommission guard that runs @@ -204,9 +205,16 @@ tests — run the matching command above for whatever you touched. lets a caller SHA-pin `uses:` yet load mutable scripts, and `required:` is unenforced for `workflow_call` (omitted → `''` → `actions/checkout` takes the default branch) — hence the empty-ref guard, in the checkout's OWN job. - `groom.yml` is the sanctioned exception (BE-4169): it defaults to `''` and its - checkouts fall back to `github.job_workflow_sha` — the exact, immutable commit - the reusable resolved from — so an omitted input can't reach a mutable ref. + `groom.yml` is the sanctioned exception (BE-4169, fixed in BE-8077): it + defaults to `''` and its checkouts fall back to `job.workflow_sha` — the exact, + immutable commit the reusable resolved from — so an omitted input can't reach a + mutable ref. It is `job.workflow_sha`, NOT `github.job_workflow_sha`: that one + is an OIDC claim, not a `github` context property, so it expands to `''` and + checkout takes the default branch — which is why every one of those jobs also + runs a fail-closed empty-ref guard (`job.workflow_sha` needs runner v2.334.0+), + and why the pin lint now FLAGS the old spelling. `cursor-review.yml`'s + never-fail ledger job warns and skips its checkout instead. (actionlint + ≤ 1.7.12 false-positives on `job.workflow_sha`; nothing here runs actionlint.) - **Scripts are the single source of truth**, loaded at run time from a pinned ref of THIS repo — never from the caller's checkout. That's what makes the reviewer/checker tamper-proof: a PR can't rewrite the logic judging it. The diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b6ef13f8..5ce25458 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -47,9 +47,11 @@ the answer is yes, bump the major tag (`v1` → `v2`) and let callers opt in. **2. Does it change assets loaded at run time?** Several workflows fetch prompts, briefs, or checker scripts from `workflows_ref` while running. That input is -required with no default (`groom.yml` excepted — it defaults to `''`, but should -still be pinned; see the README), so a caller that bumps `uses:` and leaves -`workflows_ref:` behind mixes your new workflow with its old assets. When you +required with no default (`groom.yml` excepted — it defaults to `''` and every +asset checkout falls back to `job.workflow_sha`, the commit the caller's `uses:` +resolved to, so leaving it unset there is safe; see the README), so a caller that +bumps `uses:` and leaves an explicitly-set `workflows_ref:` behind mixes your new +workflow with its old assets. When you change assets and workflow together, say so in the PR body so callers bump both. ## Adding a new reusable workflow diff --git a/README.md b/README.md index 669395e3..bf2d0e7e 100644 --- a/README.md +++ b/README.md @@ -21,7 +21,7 @@ complete, copy-pasteable caller. | [`assign-reviewers.yml`](.github/workflows/assign-reviewers.yml) | Auto-requests expertise-aware, load-balanced PR reviewers with new-folk randomization. Matches changed paths against a caller-repo `.github/reviewers.yml` (path-glob → reviewers, plus a `default_pool`), drops the author + `vars.REVIEWER_EXCLUDE`, ranks candidates by open review load (steering off anyone at/over `vars.REVIEWER_LOAD_CAP`), and may swap a slot for a `vars.REVIEWER_GROWTH_POOL` member. Requests go through the CLOUD_CODE_BOT app token so they work on fork PRs. Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`. | [assign-reviewers.md](docs/callers/assign-reviewers.md) | | [`assign-prs-to-author.yml`](.github/workflows/assign-prs-to-author.yml) | Housekeeping — assigns every open PR with no assignees to its author (bot-authored PRs skipped by default). Run on a schedule from a thin caller; useful when a team tracks PR ownership via assignees. The calling job needs `pull-requests: write` and `issues: write`. | [assign-prs-to-author.md](docs/callers/assign-prs-to-author.md) | | [`pr-size.yml`](.github/workflows/pr-size.yml) | PR-size cap — fails (or, in `mode: warn`, only reports) when a PR's net diff exceeds `max_lines` non-generated changed lines, keeping diffs reviewable. Excludes dependency lockfiles, `linguist-generated` files (read from the base ref, so a PR can't exempt itself), Go generated-code markers, and per-repo `extra_lockfiles` / `extra_generated_globs`. Opt in to `exclude_tests` to cap production code rather than test coverage (excluded test lines are always reported, never silently dropped). A `bypass_label` (default `oversized-ok`) waves through a legitimately large change; a sticky bot comment explains overages when `bot_app_id` + `BOT_APP_PRIVATE_KEY` are supplied (degrades to status + step summary without them). Counting logic + tests live in [`scripts/check-pr-size/`](scripts/check-pr-size). | [pr-size.md](docs/callers/pr-size.md) | -| [`pr-risk.yml`](.github/workflows/pr-risk.yml) | **Advisory PR risk grading (shadow check)** — **automatic grading off by default** (`enabled: false`; a manual `workflow_dispatch` grades regardless, so a repo can trial it before switching on); switch it on with `enabled: true` or by setting the caller repo's `RISK_CONFIG` variable to `{"enabled": true}`, which outranks the input in both directions so `{"enabled": false}` is a no-PR kill switch. Grades every PR into a tier `R0` (safest) .. `R3` (riskiest) and syncs one label (`risk:R0`..`risk:R3`, or `risk:ungraded` when an input was unreadable). The label is the entire product: nothing is gated, routed, commented, or merged. Deterministic (`gh` + `jq`, no LLM): `grade = worst(path_floor, provenance, reversibility)` — path-glob map, what-process-produced-the-diff (registered runbooks with identity + diff-shape assertions; forks are R3 with no exceptions), and revertability (persistent-state mutation, deletions under sensitive classes, did green checks cover the lines). Grader + generic defaults live in [`scripts/pr-risk/`](scripts/pr-risk); a consumer sharpens them with `.github/risk.json` / `.github/risk-runbooks.json`, read from the PR's **base ref** so a PR can't edit the rules that judge it. The job excludes its own run from the check rollup and waits (`wait_for_checks_minutes`) for the rest to settle before labeling. Labels ride the plain `GITHUB_TOKEN` (cannot fire `labeled` triggers — no cascade risk); disagreement is recorded with a human-owned `risk-dispute` label. **Two further publish surfaces are available and are OFF by default**, so an enrolled caller behaves byte-identically until it opts in: `sticky_comment: true` posts ONE comment (created once, updated in place — N pushes leave one comment) carrying the per-file path-axis breakdown, the risk CONCENTRATION sentence ("94% of this diff is R0/R1; the 6% that puts the path floor at R3 is these two files, 40 lines") and a "this grade is wrong" checkbox whose state round-trips into a `risk-grade-disputed` label (distinct from the human-owned `risk-dispute`, which the grader still never touches); `check_run: true` publishes the tier and reason as a Check Run on the head commit — the immutable, timestamped, commit-attached record a mutable label cannot be — from a SEPARATE job, the only one holding `checks: write` — a grant every caller makes whether or not it switches the surface on, because GitHub validates every nested job's declared `permissions:` at startup and a job `if:` is a runtime condition (the input decides whether a check is PUBLISHED, not whether the grant is CHECKED). Both surfaces are advisory in the same sense as the label: the Check Run's conclusion is hardcoded `neutral`, and every publish failure is an annotation, never a red check. Label text is remappable via `label_map`. `workflows_ref` is **required**, and both its **shape and its ancestry are enforced** — every job that checks it out fails the run *before* the tool checkout unless the value is a full 40-hex lowercase commit SHA **and** that commit is an ancestor of `main` of this repo (fetched from a literal upstream URL; no `github` context can name this repo from inside a reusable workflow, so a variable there would be an alias a fork could point at itself). Shape rejects everything mutable — a branch, a tag, a `refs/pull/N/head`. Ancestry rejects everything unmerged: a fork of this public repo shares its object store, and GitHub serves a fork PR's head objects from this repo's own URL unauthenticated, so a fork-authored SHA is just as well-shaped and would otherwise be checked out into a job holding the caller's write token. There is deliberately **no opt-out input** — an opt-out would be set by the very pin-bump PR the check distrusts. Two consequences by design: **pinning a not-yet-merged SHA is rejected** (merge first, then bump), and the axis **fails closed** if this repo ever goes private or `main` is force-rewritten past a consumer's pin (re-pin to a commit on `main` to recover). **What is still not machine-checked is that the pin is the *current* one**: a stale pin left behind when `uses:` moved is a merged ancestor too. The test that would close that is "equal to the commit `uses:` resolved to", and the runner does not expose it to the workflow (`github.workflow_sha` is the *caller's* top-level file; `job_workflow_sha` is an OIDC claim, not a `github` context property, so reading it would need `id-token: write` from every caller). The other residual is irreducible: a fork commit that edits `pr-risk.yml` itself is rejected by ancestry, but a caller pinned at one runs the fork's own copy of the guard, and no in-file check can bound a revision of that file an attacker chose. **Reviewing the caller is what bounds both, and it is the only thing that can: require `uses:` at a full commit SHA of this repo and `with: workflows_ref:` set to that same SHA written out literally, character-for-character — never an expression, never a tag.** The guard also runs *before* enablement is resolved (the resolver is itself loaded from `workflows_ref`), so a floating pin fails red even with the `RISK_CONFIG` kill switch set — the switch stops the grading, not a broken enrollment. Call the workflow directly: a nested `workflow_call` chain through an org wrapper is unsupported. Enroll it as its own workflow rather than a job inside an existing CI workflow (the rollup exclusion is per-run). The calling job needs `contents: read` + `issues: write` + `pull-requests: write` + `checks: write` + `actions: read` + `statuses: read`; GitHub rejects a shorter grant at startup (a reusable workflow can only narrow the caller's token, never elevate it), so a caller enrolled from an older copy of this row fails before any step runs. The two label writes are the ONE label: repo-side label creation on first use maps to `issues`, and labeling a PR maps to `pull-requests` (the labels endpoint is dual-mapped by what the "issue" is, so `issues: write` alone 403s on a PR). `checks: write` is the `publish-check` job's declaration, required unconditionally per above — grading itself uses only `checks: read`. `actions: read` is for the rollup's `CheckRun -> checkSuite -> workflowRun` self-exclusion hop. No secrets. | [pr-risk.md](docs/callers/pr-risk.md) | +| [`pr-risk.yml`](.github/workflows/pr-risk.yml) | **Advisory PR risk grading (shadow check)** — **automatic grading off by default** (`enabled: false`; a manual `workflow_dispatch` grades regardless, so a repo can trial it before switching on); switch it on with `enabled: true` or by setting the caller repo's `RISK_CONFIG` variable to `{"enabled": true}`, which outranks the input in both directions so `{"enabled": false}` is a no-PR kill switch. Grades every PR into a tier `R0` (safest) .. `R3` (riskiest) and syncs one label (`risk:R0`..`risk:R3`, or `risk:ungraded` when an input was unreadable). The label is the entire product: nothing is gated, routed, commented, or merged. Deterministic (`gh` + `jq`, no LLM): `grade = worst(path_floor, provenance, reversibility)` — path-glob map, what-process-produced-the-diff (registered runbooks with identity + diff-shape assertions; forks are R3 with no exceptions), and revertability (persistent-state mutation, deletions under sensitive classes, did green checks cover the lines). Grader + generic defaults live in [`scripts/pr-risk/`](scripts/pr-risk); a consumer sharpens them with `.github/risk.json` / `.github/risk-runbooks.json`, read from the PR's **base ref** so a PR can't edit the rules that judge it. The job excludes its own run from the check rollup and waits (`wait_for_checks_minutes`) for the rest to settle before labeling. Labels ride the plain `GITHUB_TOKEN` (cannot fire `labeled` triggers — no cascade risk); disagreement is recorded with a human-owned `risk-dispute` label. **Two further publish surfaces are available and are OFF by default**, so an enrolled caller behaves byte-identically until it opts in: `sticky_comment: true` posts ONE comment (created once, updated in place — N pushes leave one comment) carrying the per-file path-axis breakdown, the risk CONCENTRATION sentence ("94% of this diff is R0/R1; the 6% that puts the path floor at R3 is these two files, 40 lines") and a "this grade is wrong" checkbox whose state round-trips into a `risk-grade-disputed` label (distinct from the human-owned `risk-dispute`, which the grader still never touches); `check_run: true` publishes the tier and reason as a Check Run on the head commit — the immutable, timestamped, commit-attached record a mutable label cannot be — from a SEPARATE job, the only one holding `checks: write` — a grant every caller makes whether or not it switches the surface on, because GitHub validates every nested job's declared `permissions:` at startup and a job `if:` is a runtime condition (the input decides whether a check is PUBLISHED, not whether the grant is CHECKED). Both surfaces are advisory in the same sense as the label: the Check Run's conclusion is hardcoded `neutral`, and every publish failure is an annotation, never a red check. Label text is remappable via `label_map`. `workflows_ref` is **required**, and both its **shape and its ancestry are enforced** — every job that checks it out fails the run *before* the tool checkout unless the value is a full 40-hex lowercase commit SHA **and** that commit is an ancestor of `main` of this repo (fetched from a literal upstream URL; no `github` context can name this repo from inside a reusable workflow, so a variable there would be an alias a fork could point at itself). Shape rejects everything mutable — a branch, a tag, a `refs/pull/N/head`. Ancestry rejects everything unmerged: a fork of this public repo shares its object store, and GitHub serves a fork PR's head objects from this repo's own URL unauthenticated, so a fork-authored SHA is just as well-shaped and would otherwise be checked out into a job holding the caller's write token. There is deliberately **no opt-out input** — an opt-out would be set by the very pin-bump PR the check distrusts. Two consequences by design: **pinning a not-yet-merged SHA is rejected** (merge first, then bump), and the axis **fails closed** if this repo ever goes private or `main` is force-rewritten past a consumer's pin (re-pin to a commit on `main` to recover). **What is still not machine-checked is that the pin is the *current* one**: a stale pin left behind when `uses:` moved is a merged ancestor too. The test that would close that is "equal to the commit `uses:` resolved to". That value IS readable — not as `github.workflow_sha` (the *caller's* top-level file) and not as the `job_workflow_sha` OIDC claim (not a `github` context property; reading it would need `id-token: write` from every caller), but as `job.workflow_sha`, the `job`-context accessor added in runner v2.334.0 that `groom.yml` reads (BE-8077). It is deliberately not wired in here: asserting equality would fail red on every caller whose `uses:` and `workflows_ref` have already drifted apart, which is a caller-contract change tracked separately. The other residual is irreducible: a fork commit that edits `pr-risk.yml` itself is rejected by ancestry, but a caller pinned at one runs the fork's own copy of the guard, and no in-file check can bound a revision of that file an attacker chose. **Reviewing the caller is what bounds both, and it is the only thing that can: require `uses:` at a full commit SHA of this repo and `with: workflows_ref:` set to that same SHA written out literally, character-for-character — never an expression, never a tag.** The guard also runs *before* enablement is resolved (the resolver is itself loaded from `workflows_ref`), so a floating pin fails red even with the `RISK_CONFIG` kill switch set — the switch stops the grading, not a broken enrollment. Call the workflow directly: a nested `workflow_call` chain through an org wrapper is unsupported. Enroll it as its own workflow rather than a job inside an existing CI workflow (the rollup exclusion is per-run). The calling job needs `contents: read` + `issues: write` + `pull-requests: write` + `checks: write` + `actions: read` + `statuses: read`; GitHub rejects a shorter grant at startup (a reusable workflow can only narrow the caller's token, never elevate it), so a caller enrolled from an older copy of this row fails before any step runs. The two label writes are the ONE label: repo-side label creation on first use maps to `issues`, and labeling a PR maps to `pull-requests` (the labels endpoint is dual-mapped by what the "issue" is, so `issues: write` alone 403s on a PR). `checks: write` is the `publish-check` job's declaration, required unconditionally per above — grading itself uses only `checks: read`. `actions: read` is for the rollup's `CheckRun -> checkSuite -> workflowRun` self-exclusion hop. No secrets. | [pr-risk.md](docs/callers/pr-risk.md) | | [`pr-derisk.yml`](.github/workflows/pr-derisk.yml) | **On-demand de-risk split plan (`/derisk`, beta)** — **off by default** (`enabled: false`; `vars.DERISK_CONFIG` on the caller repo outranks it in both directions, so `{"enabled": false}` is a no-PR kill switch). Someone with write access comments `/derisk` on a pull request; the workflow re-grades that PR with the pr-risk grader, makes **one** model call for a semantic partition of the diff into a chain of 2–5 **sequential** PRs, and posts **one sticky advisory comment**: a verdict line and a chain table above the fold, the plan, the chain-landing rules and the floor math inside collapsed `
`. **Every floor shown is computed by `grade-pr-risk.sh --stdin`** over a synthetic record built from each step's files — the model proposes *which files go together* and nothing else, and any tier it writes is discarded before rendering. The partition must cover the changed-file set **exactly**; a missing or duplicated path is rejected, re-prompted **once**, then falls back. **Nothing is gated, routed, merged, labelled or filed** — filing tickets from a plan is a later rung behind its own command. Honest by construction: when no step lands below the PR's **path floor** the verdict reads "N smaller single-concern R3s, same lane", never a fake lane win, because the verdict is arithmetic over grader output rather than the model's prose — and the comparison is against the path floor rather than the headline tier precisely because a split only moves the path axis, so a fork PR (provenance R3, path floor R0) is told the truth instead of a reduction no partition can deliver. An over-budget diff, an unvalidatable partition and an API failure each post an explaining comment — a `/derisk` that quietly does nothing is never an outcome. Safe as a comment command because an `issue_comment` workflow runs from the caller's **default branch** (a PR cannot edit the workflow serving it), the commenter is gated on `author_association` (`allowed_associations`, default `OWNER,MEMBER,COLLABORATOR` — narrow it, never widen), and **no PR code is ever checked out**: the diff is read over the API and handed to a model as text. `workflows_ref` is **required** and its shape + ancestry are enforced by the guard byte-identical to `pr-risk.yml`'s (see that row for the full rationale and its two residuals); a test fails the build if the copies drift. The calling job needs `contents: read` + `pull-requests: write` + `checks: read` + `actions: read` + `statuses: read`, and the `anthropic_api_key` secret. | [pr-derisk.md](docs/callers/pr-derisk.md) | | [`pr-area-label.yml`](.github/workflows/pr-area-label.yml) | **Agentic PR area labeling** — classifies each PR into exactly one `area:*` label, and (on push to the consumer's default branch) syncs the repo's `area:*` labels to the taxonomy. The taxonomy is the consumer's own `.github/area-labels.yml` (`repo_context` + `labels[]` with `name`/`color`/`description`/optional `guidance`); the shared workflow carries nothing repo-specific. An LLM makes the domain-vs-path judgement a static `paths:` map can't, but with **no tools and no token**: PR title/body/paths/labels go to the Anthropic Messages API as data inside `` tags (diff excluded), the reply is enum-constrained by a JSON schema to the taxonomy's own names, and a deterministic step applies it with targeted `area:*` add/remove ops (never a full-set PUT, so concurrent non-area edits survive). The taxonomy is read from the PR's **base ref** — a PR can't rewrite the rules that classify it — and validated (unique `area:[a-z0-9-]+` names, every label resolves to a non-blank routing guide) before it drives a write; everything fails soft rather than failing the check. Classifier logic lives in [`scripts/area-label/`](scripts/area-label), loaded from the pinned `workflows_ref` (validated to a full 40-hex SHA before checkout). Fork/Dependabot PRs are skipped by construction (no writable token, no secret). Labels ride the plain `GITHUB_TOKEN` (no `labeled`-trigger cascade). The calling job needs `contents: read` + `issues: write` + `pull-requests: write`; the `ANTHROPIC_API_KEY` secret is provided org-wide (unset ⇒ `label-pr` fails soft, sync still works). | [pr-area-label.md](docs/callers/pr-area-label.md) | | [`stale.yml`](.github/workflows/stale.yml) | Stale-PR sweeper (`actions/stale`) plus a Slack digest of what it touched. PRs inactive for N days are labeled `stale`; still-inactive PRs are closed. The digest header names the source repo so batches from different repos posted to the same channel are unambiguous. Thresholds, messages, exempt labels, and the Slack channel are inputs; the caller owns the schedule + dry-run toggle. The calling job needs `pull-requests: write` and `issues: write`. Optional `SLACK_BOT_TOKEN`. | [stale.md](docs/callers/stale.md) | @@ -104,13 +104,22 @@ it is empty or omitted (GitHub does not enforce `required: true` for `workflow_call` inputs, so those workflows re-check at run time; `pr-risk.yml`, `pr-derisk.yml`, `pr-area-label.yml` and `linear-ticket.yml` additionally reject anything that is not a merged full 40-hex SHA of this repo). The guards are -per-job, so a skipped job never evaluates one — and `cursor-review.yml`'s -never-fail `Prior-review ledger` job carries none by design. `groom.yml` is the -one workflow that does not require the input (BE-4169 meant an omitted value to -fall back to `github.job_workflow_sha`, which is not a `github` context property -— GitHub spells that value `job.workflow_sha` — so it currently resolves to `''` -and checkout takes the default branch). Pin it there too, until those checkouts -are moved onto `job.workflow_sha` behind an empty-ref guard. +per-job, so a skipped job never evaluates one. `groom.yml` is the one workflow +that does not require the input: an omitted value falls back to +`job.workflow_sha` — the commit the caller's `uses:` pin resolved to — so +leaving it unset is safe, and every job that checks out the briefs and scripts +fails closed on an empty ref (`job.workflow_sha` needs an Actions runner +≥ v2.334.0). The agent CLI pin is the deliberate exception: it reads +`ref: ${{ job.workflow_sha }}` alone and only warns, so on an older runner it +reads that one manifest from the default branch instead of failing the run. BE-4169 +originally spelled that fallback `github.job_workflow_sha`, which is an OIDC +token claim rather than a `github` context property: it expanded to `''` and +checkout took the default branch. BE-8077 moved all seven groom checkouts, and +`cursor-review.yml`'s never-fail `Prior-review ledger` job, onto +`job.workflow_sha`; the ledger job cannot fail the run, so it warns and SKIPS +its asset checkout instead of exiting non-zero, degrading the ledger to +`status=unknown`. (`actionlint` ≤ 1.7.12 false-positives on `job.workflow_sha` — +its `job`-context schema predates that runner release; no CI here runs it.) Per-workflow inputs, required secrets, and triggers are documented in each workflow file's header comment. diff --git a/docs/callers/README.md b/docs/callers/README.md index 37572866..65a3e6c4 100644 --- a/docs/callers/README.md +++ b/docs/callers/README.md @@ -153,22 +153,27 @@ Two caveats on "it fails fast", so you do not rely on it as a safety net: never evaluates it, so an omitted ref can pass a run silently. - One job is deliberately exempt: `cursor-review`'s `Prior-review ledger` job must never fail the run — the review matrix `needs:` it — so it carries no - guard and falls back rather than erroring. + fail-closed guard. Since BE-8077 it resolves the ref in a step that WARNS on + empty and skips the asset checkout, degrading the ledger to `status=unknown` + rather than checking assets out of this repo's default branch. `groom` is the one workflow that declares `workflows_ref` **without** making it -required (it defaults to `''`). Pin it anyway, exactly like the others. The -intent (BE-4169) was for an omitted input to fall back to the commit the -caller's `uses:` resolved to, but the fallback is spelled +required (it defaults to `''`), and there leaving it unset **is safe**: an +omitted input falls back to `job.workflow_sha`, the commit the caller's `uses:` +resolved to, and every groom job fails closed with an `::error::` if that +resolves empty (it needs an Actions runner ≥ v2.334.0). + +That is only true from BE-8077 onward. BE-4169 spelled the fallback `github.job_workflow_sha`, which is **not** a property of the `github` context — GitHub documents the commit-of-the-current-job's-workflow-file value as -`job.workflow_sha`, and this repo's own `groom.yml` says so where it reads the +`job.workflow_sha`, and this repo's own `groom.yml` said so where it reads the agent CLI pin. Actions expands an unknown context property to `''` rather than -erroring, so today an omitted `workflows_ref` resolves to the empty string and -`actions/checkout` takes this repo's **default branch** — the mutable-assets -hole the pin exists to close. Until groom's asset checkouts move to -`job.workflow_sha` with an empty-ref guard, set the input. +erroring, so on a groom pin older than BE-8077 an omitted `workflows_ref` +resolves to the empty string and `actions/checkout` takes this repo's **default +branch** — the mutable-assets hole the pin exists to close. On those pins, set +the input. -So, everywhere it is declared, set it explicitly to the same SHA: +So, everywhere the input is **required**, set it explicitly to the same SHA: ```yaml uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@07154fb… diff --git a/docs/callers/groom.md b/docs/callers/groom.md index c3cc3e5b..9c4d0fce 100644 --- a/docs/callers/groom.md +++ b/docs/callers/groom.md @@ -58,7 +58,11 @@ jobs: actions: read # the interval gate reads run history for the last real run uses: Comfy-Org/github-workflows/.github/workflows/groom.yml@ with: - workflows_ref: # must equal the uses: SHA — see its row + # Optional since BE-8077 — omit it and the briefs + ledger auto-load from + # the commit the uses: pin above resolved to. Kept explicit here because + # `bump-callers.sh` moves this line and the uses: pin in ONE pass, so a + # roster-enrolled caller costs nothing to double-pin. See its row. + workflows_ref: # keep byte-identical to the uses: SHA bot_app_id: ${{ vars.APP_ID }} # Cadence knob + the matching volume-gate window (BE-4004). Wire both to # one repo Actions variable so they can't drift: retune weekly -> @@ -96,8 +100,9 @@ rejects the run with "requesting 'actions: read', but is only allowed ## Inputs All are optional to GitHub's startup validator, and unlike the other workflows -here `workflows_ref` is optional to groom's own guards too — but set it anyway, -and set it to your `uses:` SHA. See its row for why. The ones that matter: +here `workflows_ref` is optional to groom's own guards too — leaving it unset is +safe, and it is the documented default. See its row for what it falls back to. +The ones that matter: | Input | Default | Why you'd change it | |---|---|---| @@ -109,7 +114,7 @@ and set it to your `uses:` SHA. See its row for why. The ones that matter: | `model` | `claude-opus-5` | The finder/verifier model. | | `themes` | `duplication, inconsistent patterns, missing abstractions, complexity hotspots, dead code` | Steer the finder at particular kinds of cleanup. The default mirrors the finder brief's own five dimensions, so it is a no-op; **narrow** it (e.g. `duplication, dead code`) to focus a repo. Security/auth-adjacent findings are filed regardless of theme. | | `scope_label` / `scope_desc` | `whole-repo` | Cosmetic labels for the scope in issue bodies. | -| `workflows_ref` | `''` | **Set it to your `uses:` SHA.** Alone among these workflows groom does not *require* it — it defaults to `''` and each asset checkout falls back to `${{ github.job_workflow_sha }}`. That fallback does not resolve (see the gotcha below), so an omitted input loads the briefs, `ledger.py` and `interval.py` from this repo's **default branch** into jobs holding your `ANTHROPIC_API_KEY` and App token. Pin it until groom's checkouts move to `job.workflow_sha`. | +| `workflows_ref` | `''` | **Leaving it unset is safe.** Alone among these workflows groom does not *require* it — it defaults to `''` and each asset checkout falls back to `${{ job.workflow_sha }}`, the commit your `uses:` pin resolved to, so the briefs, `ledger.py` and `interval.py` always match the logic running them with nothing to keep in sync. Set it only to test briefs from a branch. Before BE-8077 that fallback was spelled `github.job_workflow_sha` and silently loaded the assets from this repo's default branch — see the footgun below. | | `bot_app_id` | `''` | File as your App rather than `github-actions[bot]`. | | `builder` | `false` | Opt into PR-writing — see below. | | `max_prs` | `'5'` | Only with `builder: true`. Typed **string**, deliberately. | @@ -156,19 +161,31 @@ declares `concurrency: groom-${{ github.repository }}` with Duplicating that group **deadlocks the run**: your caller holds the group while its `uses:` job waits for the same group, and neither yields until timeout. -**Set `workflows_ref`, even though groom does not make you.** The finder and -verifier briefs, `ledger.py` and `interval.py` are checked out at run time from -`${{ inputs.workflows_ref || github.job_workflow_sha }}`. The intent (BE-4169) -was that omitting the input falls back to the commit your `uses:` pin resolved -to, leaving nothing to keep in sync. It does not: `job_workflow_sha` is **not** -a property of the `github` context — GitHub documents that value as -`job.workflow_sha`, which is why `groom.yml` uses that spelling where it reads -the agent CLI pin, and calls `github.job_workflow_sha` a trap in the same -comment. Actions expands an unknown context property to `''`, and -`actions/checkout` with an empty `ref` takes the **default branch**, so an -omitted input runs mutable assets in the jobs holding your `ANTHROPIC_API_KEY` -and App token. Until those checkouts move to `job.workflow_sha` behind an -empty-ref guard, the explicit pin is what keeps groom pinned. +**`workflows_ref` may be left unset — but check which SHA you pinned from.** +The finder and verifier briefs, `ledger.py` and `interval.py` are checked out at +run time from `${{ inputs.workflows_ref || job.workflow_sha }}`: omitting the +input falls back to the commit your `uses:` pin resolved to, leaving nothing to +keep in sync (BE-4169's intent, delivered in BE-8077). Two things to know. First, +if you pin `uses:` at a `github-workflows` commit **older than BE-8077**, that +fallback is still spelled `github.job_workflow_sha` there — which is **not** a +property of the `github` context (GitHub documents that value as +`job.workflow_sha`, the spelling `groom.yml` has always used where it reads the +agent CLI pin, calling the `github.` form a trap in the same comment). Actions +expands an unknown context property to `''`, and `actions/checkout` with an empty +`ref` takes the **default branch** — so on those older pins an omitted input runs +mutable assets in the jobs holding your `ANTHROPIC_API_KEY` and App token, and +the explicit pin is what keeps groom pinned. Bump past BE-8077 (or set +`workflows_ref`) to close it. Second, `job.workflow_sha` needs an Actions runner +≥ v2.334.0; every job that checks out the **briefs and scripts** now fails +closed with an `::error::` if the ref resolves empty, so that failure is loud +rather than a silent default-branch checkout. The one path deliberately left +warn-only is the **agent CLI pin**: it reads `ref: ${{ job.workflow_sha }}` +alone — no `workflows_ref` in front of it, and no guard — so on a pre-v2.334.0 +runner it reads the CLI manifest from the default branch behind a `::warning::` +rather than failing the run. The pin is still validated; it just stops being +pinned by the caller's `uses:` SHA. Every groom job runs on `ubuntu-latest`, +which is always current, so only a stale self-hosted runner reaches either +branch. **Bumping is not usually manual.** `bump-callers.sh` rewrites a caller's `workflows_ref:` in the same pass as its `uses:` pin, so a caller enrolled in the