diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index fd8ecd1..4fab474 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -115,7 +115,7 @@ jobs: diff_excludes: >- :!**/package-lock.json :!**/*.generated.* - # Pin the prompts/scripts to the same ref you pin `uses:` to. + # REQUIRED — the same SHA as the `uses:` pin above. workflows_ref: secrets: CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} @@ -185,8 +185,9 @@ Notes: ## Configuration knobs -All optional, with defaults — pass them under `with:` in the caller. Full -descriptions live in the [workflow header](../workflows/cursor-review.yml). +All optional except `workflows_ref` (required, no default) — pass them under +`with:` in the caller. Full descriptions live in the +[workflow header](../workflows/cursor-review.yml). | Input | Default | What it does | |---|---|---| @@ -194,7 +195,7 @@ descriptions live in the [workflow header](../workflows/cursor-review.yml). | `diff_size_cap` | `5000` | Max changed lines (after excludes); larger PRs are skipped. | | `review_label` | `cursor-review` | Label whose addition triggers the review. | | `diff_excludes` | lockfiles, `node_modules`, `dist`, `vendor`, minified/generated files | Pathspecs excluded from both the size count and the reviewed diff. | -| `workflows_ref` | `main` | Ref this directory's prompts/scripts are loaded from. Pin to your `uses:` SHA. | +| `workflows_ref` | **required** (no default) | Ref this directory's prompts/scripts are loaded from. Must be the same commit SHA as your `uses:` pin — omit it and the run fails fast, because pinning `uses:` while loading scripts from a mutable branch defeats the pin. | | `bot_app_id` | `''` | Optional GitHub App ID; when set (with `BOT_APP_PRIVATE_KEY`), the review posts under that App's identity instead of `github-actions[bot]`. | | `blocking` | `false` | Opt-in merge gate. `true` fails the **Blocking gate** check while any cursor-review finding thread is unresolved. See [Make the review blocking](#optional-make-the-review-blocking-merge-gate). | diff --git a/.github/workflow-pins/README.md b/.github/workflow-pins/README.md new file mode 100644 index 0000000..e6b2276 --- /dev/null +++ b/.github/workflow-pins/README.md @@ -0,0 +1,81 @@ +# workflow-pins + +An **internal repo lint** — unlike the other directories here, nothing in this +one is loaded by a reusable workflow at run time. It guards a property of this +repo's own workflow files. + +- **`check_workflow_pins.py`** — for every `on: workflow_call` workflow in + `.github/workflows/`, fails if it (1) declares a `default:` for its + `workflows_ref` input, or (2) checks out at `ref: ${{ inputs.workflows_ref }}` + in a job that does not run the empty-ref guard first. 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 + CLI smoke test that a reintroduced default really exits non-zero. + +```bash +python3 .github/workflow-pins/check_workflow_pins.py +``` + +## Why (BE-5546) + +Every reusable workflow that loads its backing scripts at run time takes a +`workflows_ref` input and checks this repo out at that ref. If that input +defaults to a floating branch, a caller can SHA-pin `uses:` and *still* load +**mutable** scripts — into jobs that hold write permissions. The pin then +proves nothing about the code that actually runs. + +So `cursor-review.yml`, `groom.yml`, and `agents-md-integrity.yml` declare +`workflows_ref` with `required: true` and **no default**, and each job that +consumes it runs a fail-fast guard before its assets checkout. The guard is not +belt-and-braces: **GitHub does not enforce `required: true` for `workflow_call` +inputs.** An omitted input arrives as `''`, and `actions/checkout` with +`ref: ''` silently checks out the default branch — recreating the hole exactly. +The guard also emits a (non-fatal) `::warning::` when the ref is not a full +40-hex SHA, since branch and tag refs can move between jobs mid-run. + +It is copied inline into each consuming job rather than factored into a +composite action **on purpose**: a composite would have to be loaded with +`uses: Comfy-Org/github-workflows/.github/actions/…@` — the very ref being +validated — and a job cannot `uses: ./…` before its checkout. Twelve copies of a +16-line guard is the cost of not making the check depend on the thing it checks. + +Deleting a `default:` is a one-line edit to undo, hence the lint. It covers +**every** `workflow_call` workflow, not an allow-list of today's three, so a +workflow added later is guarded the day it lands. + +The lint checks the guard as well as the default, because the default is only +half the hole. A **new job** — or a whole new reusable workflow — that checks +out at `ref: ${{ inputs.workflows_ref }}` without the guard reopens the `ref: ''` +default-branch fallback, and a default-only lint stays green throughout: there +was never a `default:` to find. So every such checkout must be preceded, *in its +own job*, by the guard step (a guard in job A does nothing for job B). An +exempt workflow is not held to this — it still has its default, so an omitted +input can never arrive as `''` — which puts it back under the check the moment +its own ticket drops the default. + +Two shapes the text parser is deliberately strict about: a `default` inside a +flow mapping (`workflows_ref: {type: string, default: main}`) is caught even +though it has no child lines to walk, and a file that *uses* `inputs.workflows_ref` +but whose declaration the parser cannot locate is a hard **error**, not a quiet +skip — "not applicable" and "I could not read this" must never look the same, +or a shape the parser trips on drops out of coverage with CI still green. + +The ref-checkout detector recognises all three YAML spellings of the same +checkout — block (`ref: ${{ … }}`), flow (`with: {…, ref: "${{ … }}"}`), and a +value carried on the following line (`ref: >-`, `ref: |`, or a bare `ref:`). +They are one checkout to Actions, so a detector that knows only one of them +reports an unguarded job as clean. Do not "simplify" the extra patterns away. +The guard's own signature is matched in block form **only**, deliberately: a +guard written in flow style reads as ABSENT and fails loudly, which is the right +bias for a check whose whole job is noticing an absence. + +`KNOWN_EXEMPT` in the script carries workflows with the same debt that are +tracked under their own ticket (today: `pr-size.yml`, whose caller fleet has +not been enumerated yet). The lint fails on a **stale** entry so the list drains +itself rather than rotting — whether the workflow dropped its default (fixed) or +no longer exists under that name at all (renamed or deleted), the latter being +the case that would otherwise silently pre-exempt whatever later reuses the +filename. The list is only applied to this repo's own `.github/workflows`: run +against an ad-hoc `--workflows-dir` every entry would look stale. diff --git a/.github/workflow-pins/check_workflow_pins.py b/.github/workflow-pins/check_workflow_pins.py new file mode 100644 index 0000000..5083c96 --- /dev/null +++ b/.github/workflow-pins/check_workflow_pins.py @@ -0,0 +1,717 @@ +#!/usr/bin/env python3 +"""Fail if a reusable workflow's `workflows_ref` is defaulted or unguarded. + +Why this exists (BE-5546): every reusable workflow here that loads its backing +scripts at run time takes a `workflows_ref` input and checks this repo out at +that ref. When that input defaults to a floating branch, a caller can SHA-pin +`uses:` and still load MUTABLE scripts — into jobs that hold write permissions. +The pin then proves nothing. So `workflows_ref` carries no default at all and +each consuming job fails fast on an empty value (GitHub does NOT enforce +`required: true` for `workflow_call` inputs — an omitted input arrives as `''` +and `actions/checkout` with `ref: ''` silently takes the default branch). + +Removing the default is a one-line edit to undo, hence this lint: it is the +regression guard that keeps the hole from coming back, and it deliberately +covers workflows added *later* rather than an allow-list of today's three. + +Two checks, because the default is only half the hole: + +1. no `default:` on the `workflows_ref` input, and +2. every job that checks out at `ref: ${{ inputs.workflows_ref }}` runs the + fail-fast empty-ref guard first. Without (2) a *new* job — or a whole new + reusable workflow — reintroduces the `ref: ''` default-branch fallback with + the lint still green, since it never declared a default to begin with. + +Parsing is text-level on purpose: this repo is stdlib-only (no PyYAML), same +constraint the `bump-callers.sh` awk rewrite works under. We only need to +locate one input block and look for one key inside it. Text parsing fails +*silently* when it meets a shape it can't follow, so a file that references +`inputs.workflows_ref` but whose declaration the parser cannot find is a hard +error rather than a quiet skip — "not applicable" and "I couldn't read this" +must never look the same. + +Run locally: + python3 .github/workflow-pins/check_workflow_pins.py + python3 .github/workflow-pins/check_workflow_pins.py --workflows-dir +""" + +import argparse +import os +import re +import sys + +INPUT_NAME = "workflows_ref" +DEFAULT_WORKFLOWS_DIR = ".github/workflows" + +# Reusable workflows that still carry a `workflows_ref` default and are tracked +# for the same fix under their own ticket. An entry here is a KNOWN debt, not a +# blessing — the checker fails on a STALE entry so the list drains itself +# instead of rotting, whether the workflow dropped its default (fixed) or no +# longer exists under that name (renamed or deleted). The latter matters most: +# left alone it would pre-exempt whatever future workflow reuses the filename. +# +# pr-size.yml — same shape as the three fixed in BE-5546, but its caller +# fleet (`vars.PR_SIZE_CALLERS`) was not enumerated by the BE-5543 spike, so +# dropping its default is an unverified break of consumer CI. Needs its own +# caller audit first. +KNOWN_EXEMPT = frozenset({"pr-size.yml"}) + +_ON_RE = re.compile(r"""^(['"]?)on\1\s*:(.*)$""") +_JOBS_RE = re.compile(r"""^(['"]?)jobs\1\s*:""") + +# `ref: ${{ inputs.workflows_ref }}` — the checkout the guard exists to protect. +# Any `ref:` mentioning the input counts, not just the bare expression: a +# `${{ inputs.workflows_ref || 'main' }}` fallback is the same hole wearing a +# different hat, and it should trip the lint rather than slip past it. +# +# Two spellings, because a key at line start is not the only way to write one. +# The flow-mapping form puts the whole `with:` on one line — +# with: {repository: Comfy-Org/github-workflows, ref: "${{ inputs.workflows_ref }}"} +# — which is the same unguarded checkout, and the same one-line bypass already +# barred for `default:`. The flow pattern stops the value at the entry boundary +# (`[^,}]`) so a sibling entry mentioning the input can't be misread as the ref. +_REF_USE_BLOCK_RE = re.compile(r"""^\s*(['"]?)ref\1\s*:.*inputs\.%s\b""" % INPUT_NAME) +_REF_USE_FLOW_RE = re.compile(r"""[{,]\s*(['"]?)ref\1\s*:[^,}]*inputs\.%s\b""" % INPUT_NAME) +# …and a third, because the value does not have to share the key's line at all: +# ref: >- ref: | ref: ref: # pinned +# ${{ … }} ${{ … }} ${{ … }} ${{ … }} +# A block scalar (`|`/`>`, with any chomping or explicit-indent modifier), or a +# plain multi-line scalar, or a quote opened at end of line — all leave the key +# line with no `inputs.` on it, so BOTH same-line patterns read the checkout as +# absent. Same bypass as the flow form, spelled vertically. The key line only +# OPENS a window; a hit needs the input to actually appear in the continuation. +# +# A trailing `#` comment does not close that window: `ref: # pinned` still +# takes its value from the line below, and YAML also allows a comment after a +# block header (`ref: | # pinned`). Requiring end-of-line right after the key +# read both as ordinary scalars and lost the continuation. Not after an opening +# QUOTE, though — there a `#` is string content, not a comment. +_REF_KEY_OPEN_RE = re.compile( + r"""^\s*(['"]?)ref\1\s*:[^\S\n]*(?:["'][^\S\n]*$|(?:[|>][+-]?\d*)?[^\S\n]*(?:#.*)?$)""" +) +_INPUT_MENTION_RE = re.compile(r"""inputs\.%s\b""" % INPUT_NAME) +# How the guard RECEIVES the ref: through `env:` (never interpolated into the +# script body) under this one name. Half the signature — `is_guard_step` below +# checks the other half, that the step actually rejects an empty value. +# Block form only, deliberately: a guard written in flow style reads as ABSENT, +# 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. +_GUARD_BINDING_RE = re.compile( + r"""^\s*(['"]?)WORKFLOWS_REF\1\s*:\s*(['"]?)\$\{\{\s*inputs\.workflows_ref\s*\}\}\2""" + r"""[^\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 +# that echoes it, or clones with it — mark its whole job guarded, so every later +# checkout in that job passed unexamined. That is the lint's own subject failing +# silently, so the step must also be seen to REJECT the empty value: an +# emptiness test and a non-zero exit, both inside that same step. +# And the two halves must be about the SAME thing. "an emptiness test somewhere, +# a non-zero exit somewhere" passes a step that tests an unrelated variable and +# exits on an unrelated condition — a near-match, and a likelier accident than +# the bare decoy. So the `-z` must name the ref (or a variable derived from it) +# and the exit must sit in THAT test's branch. +_GUARD_FAIL_RE = re.compile(r"""^\s*exit\s+[1-9]""") +# A command that IS `exit N`, for a branch written inline. The multiline path +# anchors `exit` at the start of its line, so a conditional `[ … ] && exit 1` +# does not count there; the inline paths hold to the same rule by requiring a +# `;`-separated command that is nothing but the exit. +_GUARD_FAIL_INLINE_RE = re.compile(r"""^\s*exit\s+[1-9]\d*\s*$""") + + +def _exits_unconditionally(text): + """True when `text` contains a bare `exit N` as one of its `;` commands.""" + return any(_GUARD_FAIL_INLINE_RE.match(part) for part in text.split(";")) +_SHELL_ASSIGN_RE = re.compile(r"""^\s*(?:export\s+)?([A-Za-z_]\w*)=(.*)$""") +_BRANCH_END_RE = re.compile(r"""^\s*(?:fi|else|elif)\b""") +# The same boundary mid-line, for a branch written inline after `then`. +_INLINE_BRANCH_END_RE = re.compile(r"""\b(?:fi|else|elif)\b""") +# Nesting, so a conditional `exit` one level in is not read as the branch's own. +_IF_OPEN_RE = re.compile(r"""\bif\b""") +_FI_RE = re.compile(r"""\bfi\b""") +# Step-level keys that can stop a correct guard from actually guarding. +_STEP_IF_RE = re.compile(r"""^\s*(['"]?)if\1\s*:""") +_STEP_CONTINUE_RE = re.compile(r"""^\s*(['"]?)continue-on-error\1\s*:\s*(.*)$""") + + +def _empty_test_re(names): + """`[ -z "$NAME" ]` / `[[ -z $NAME ]]` / `test -z …` for any of `names`.""" + alt = "|".join(sorted(re.escape(n) for n in names)) + return re.compile(r"""(?:\[\[?|\btest)\s+-z\s+"?\$\{?(?:%s)\b""" % alt) + + +def _whole_empty_test_re(names): + """The same test as the ENTIRE condition — nothing ANDed onto it. + + `if [ -z "$REF" ] && [ "$OTHER" = blocked ]; then exit 1; fi` contains the + emptiness test but does not fail for every empty ref: empty + `OTHER` + unset falls through to the checkout. A text lint cannot evaluate shell, so + it accepts only a condition that is exactly the emptiness test and rejects + every compound as ambiguous — including a widening `||`, which is safe in + fact but not worth a special case in a detector that fails closed. + """ + alt = "|".join(sorted(re.escape(n) for n in names)) + var = r""""?\$\{?(?:%s)\}?"?""" % alt + return re.compile( + r"""^(?:\[\[?\s+-z\s+%s\s+\]\]?|test\s+-z\s+%s)$""" % (var, var) + ) + + +# An `if`/`elif` split into its condition and whatever follows `then` (which is +# the branch body itself when the whole statement is written on one line). +_IF_COND_RE = re.compile(r"""^\s*(?:el)?if\s+(.*?)\s*;?\s*then\b(.*)$""") +# A single-line `run:` puts the shell on the key's own line — the `run:` is +# YAML, not part of the condition being judged. +_RUN_PREFIX_RE = re.compile(r"""^(['"]?)run\1\s*:\s*""") + + +def _ref_derived_names(body): + """Shell variables carrying the ref: `WORKFLOWS_REF` and anything set from it. + + The real guard tests `$REF`, assigned from `$WORKFLOWS_REF` through a + `printf | tr` strip, so following one assignment hop is what makes the test + recognizable at all — but only a hop that actually carries the value. + """ + names = {"WORKFLOWS_REF"} + for _ in range(3): # a short chain of derivations; converges immediately + grew = False + for line in body: + match = _SHELL_ASSIGN_RE.match(line) + if not match or match.group(1) in names: + continue + if re.search(r"""\$\{?(?:%s)\b""" % "|".join(sorted(names)), match.group(2)): + names.add(match.group(1)) + grew = True + if not grew: + break + return names +# A mapping value that IS the input (`ref:`/`WORKFLOWS_REF:` etc.) — used to +# tell "not applicable" apart from "the parser lost this file". Deliberately +# narrower than "the string appears somewhere": the test workflow's own shell +# fixtures name the input in prose and in a `sed` script, and neither is a use. +# Flow form included for the same reason as above — otherwise a file whose only +# use is one-line escapes the "NOT covering this file" error too. +# (`:[^\S\n]*(?:#…)?\s*` rather than a plain `\s*`, so a comment sitting between +# the key and a value on the next line does not hide the use — the same gap, in +# the backstop that is supposed to catch exactly this kind of miss.) +_CONSUMES_BLOCK_RE = re.compile( + r"""(?m)^\s*(['"]?)[\w.-]+\1\s*:[^\S\n]*(?:#[^\n]*)?\s*""" + r"""(['"]?)\$\{\{\s*inputs\.%s\s*\}\}\2\s*$""" % INPUT_NAME +) +_CONSUMES_FLOW_RE = re.compile( + r"""[{,]\s*(['"]?)[\w.-]+\1\s*:\s*(['"]?)\$\{\{\s*inputs\.%s\s*\}\}\2\s*[,}]""" % INPUT_NAME +) +# The block-scalar form, for the same reason again. (The plain multi-line form +# already lands in _CONSUMES_BLOCK_RE, whose `\s*` spans the newline; only the +# `|`/`>` indicator sits between the colon and the value and defeats it.) +_CONSUMES_SCALAR_RE = re.compile( + r"""(?m)^\s*(['"]?)[\w.-]+\1\s*:\s*[|>][+-]?\d*[^\S\n]*(?:#[^\n]*)?\n""" + r"""\s*\$\{\{\s*inputs\.%s\s*\}\}""" % INPUT_NAME +) + +# An `env:` binding of the input to a NAME (`WORKFLOWS_REF: ${{ inputs… }}`). +# A checkout does not have to name the input directly: hoist it to a job-level +# `env:` — the natural refactor once several steps want it — and every +# `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. +_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 +) +# 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 +# treating `ref` as an alias would make `env.ref`/`$ref` anywhere read as the +# input. (Block form only — a flow-style `env: {…}` binds no alias here, which +# loses nothing the `_CONSUMES_*` backstop does not already catch.) +_ENV_KEY_RE = re.compile(r"""^\s*(['"]?)env\1\s*:[^\S\n]*(?:#.*)?$""") + +# A `default` key inside a flow mapping: `{type: string, default: main}`. +_FLOW_DEFAULT_RE = re.compile(r"""[{,]\s*(['"]?)default\1\s*:""") + +# A `#` opens a comment at the start of a value or after whitespace. +_COMMENT_RE = re.compile(r"(?:^|\s)#.*$") + + +def env_aliases(lines): + """Names bound to the input by an `env:` mapping, 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) + if match: + names.add(match.group(2)) + return frozenset(names) + + +def _mention_alt(aliases): + """Regex alternation for "reaches the input" — directly or via an alias. + + File-wide rather than scope-aware on purpose: `env:` is scoped per job and + per step, but over-approximating can only ever DEMAND a guard, never excuse + a missing one — the safe direction for a detector whose job is absence. + """ + alt = r"""inputs\.%s\b""" % INPUT_NAME + if aliases: + names = "|".join(sorted(re.escape(a) for a in aliases)) + alt += r"""|env\.(?:%s)\b|\$\{?(?:%s)\b""" % (names, names) + return alt + + +def _ref_use_res(aliases): + """The (block, flow) `ref:` patterns, widened to the input's env aliases.""" + alt = _mention_alt(aliases) + return ( + re.compile(r"""^\s*(['"]?)ref\1\s*:.*(?:%s)""" % alt), + re.compile(r"""[{,]\s*(['"]?)ref\1\s*:[^,}]*(?:%s)""" % alt), + ) + + +def is_ref_use(line, res=None): + """True when `line` checks out at the input — block or flow-mapping form.""" + block_re, flow_re = res or (_REF_USE_BLOCK_RE, _REF_USE_FLOW_RE) + return bool(block_re.match(line) or flow_re.search(line)) + + +def _consumes_input(text): + """True when `text` uses the input as a mapping value in any YAML style.""" + return bool( + _CONSUMES_BLOCK_RE.search(text) + or _CONSUMES_FLOW_RE.search(text) + or _CONSUMES_SCALAR_RE.search(text) + ) + + +def _strip_comment(value): + """Drop a trailing `# …` comment from a scalar value.""" + return _COMMENT_RE.sub("", value).strip() + + +def _is_skippable(line): + """Blank lines and whole-line comments never open or close a YAML block.""" + stripped = line.strip() + return not stripped or stripped.startswith("#") + + +def _indent(line): + return len(line) - len(line.lstrip(" ")) + + +def _key_re(indent, key): + """`key:` at exactly `indent`, bare or quoted (both are valid Actions YAML).""" + return re.compile(r"""^ {%d}(['"]?)%s\1\s*:""" % (indent, re.escape(key))) + + +def _block_body(lines, start, indent): + """Yield (lineno, line) for the block nested under `lines[start]`. + + The block runs until the first non-skippable line indented at or above + (i.e. numerically at or below) `indent` — that line belongs to the parent. + """ + for i in range(start + 1, len(lines)): + line = lines[i] + if _is_skippable(line): + continue + if _indent(line) <= indent: + return + yield i, line + + +def _find_key(lines, key, start, indent, stop_indent): + """Line index of `key:` at exactly `indent` inside the block opened at `start`. + + Returns None if the block ends (a line at or shallower than `stop_indent`) + before the key appears. + """ + pattern = _key_re(indent, key) + for i in range(start + 1, len(lines)): + line = lines[i] + if _is_skippable(line): + continue + if _indent(line) <= stop_indent: + return None + if pattern.match(line): + return i + return None + + +def _first_child_indent(lines, start, indent): + """Indent of the first child line under `lines[start]`, or None if childless.""" + for _, line in _block_body(lines, start, indent): + return _indent(line) + return None + + +def find_workflows_ref_defaults(lines): + """Line numbers (1-based) of `default:` keys inside the input's block. + + Returns None when the file is not a `workflow_call` workflow declaring a + `workflows_ref` input — i.e. "nothing to check here", which is distinct + from the empty list ("checked, and clean"). + """ + on_line = None + for i, line in enumerate(lines): + if _is_skippable(line): + continue + match = _ON_RE.match(line) + if match and _indent(line) == 0: + # `on: [push]` / `on: push` inline forms declare no workflow_call + # inputs, so only the block form can hold what we look for. A + # trailing comment (`on: # triggers`) is NOT an inline value — + # treating it as one would silently drop the file from the lint. + if _strip_comment(match.group(2)): + return None + on_line = i + break + if on_line is None: + return None + + on_child = _first_child_indent(lines, on_line, 0) + if on_child is None: + return None + call_line = _find_key(lines, "workflow_call", on_line, on_child, 0) + if call_line is None: + return None + + call_child = _first_child_indent(lines, call_line, on_child) + if call_child is None: + return None + inputs_line = _find_key(lines, "inputs", call_line, call_child, on_child) + if inputs_line is None: + return None + + input_indent = _first_child_indent(lines, inputs_line, call_child) + if input_indent is None: + return None + ref_line = _find_key(lines, INPUT_NAME, inputs_line, input_indent, call_child) + if ref_line is None: + return None + + hits = [] + + # The flow-mapping form puts the whole input on one line: + # workflows_ref: {type: string, default: main} + # It has no child lines at all, so the block scan below would call it clean + # — and it is the shortest possible way to write the regression. + inline = _strip_comment(lines[ref_line].split(":", 1)[1]) + if inline.startswith("{") and _FLOW_DEFAULT_RE.search(inline): + hits.append(ref_line + 1) + + # The input's own block: from the `workflows_ref:` line down to the next key + # at the same indentation (the next input, or the end of the inputs map). + # Only lines at the input's OWN property indent count — a `default:` deeper + # than that belongs to something else, e.g. a wrapped line of a folded + # `description: >-` scalar, which must not fail a compliant workflow. + prop_indent = _first_child_indent(lines, ref_line, input_indent) + if prop_indent is not None: + default_re = _key_re(prop_indent, "default") + hits.extend( + i + 1 + for i, line in _block_body(lines, ref_line, input_indent) + if default_re.match(line) + ) + return hits + + +def _step_bounds(lines, idx): + """(start, end, key_indent) of the STEP whose `env:` holds the binding at `idx`. + + None when the binding is not inside a step at all — a job-level `env:` + hoists the value out of every step, which is a binding but not a guard. + """ + ind = _indent(lines[idx]) + key_indent = None # the step's own key column, i.e. where `env:`/`run:` sit + for j in range(idx - 1, -1, -1): + if _is_skippable(lines[j]): + continue + if _indent(lines[j]) < ind: + key_indent = _indent(lines[j]) + break + if key_indent is None: + return None + + start = None # the step's `- …` list-item line + for j in range(idx, -1, -1): + if _is_skippable(lines[j]) or _indent(lines[j]) >= key_indent: + continue + if lines[j].lstrip().startswith("- "): + start = j + break # first shallower line decides it: a step, or not one at all + if start is None: + return None + + end = len(lines) + for j in range(start + 1, len(lines)): + if _is_skippable(lines[j]): + continue + if _indent(lines[j]) < key_indent: + end = j + break + return start, end, key_indent + + +def is_guard_step(lines, idx): + """True when the binding at `idx` sits in a step that REJECTS an empty ref. + + Fail-closed: a step the parser cannot resolve, or one that takes the value + without testing it, is not a guard — which reports the checkout it precedes + rather than passing a checkout nothing verified. + """ + bounds = _step_bounds(lines, idx) + if bounds is None: + return False + start, end, key_indent = bounds + body = lines[start:end] + + # Two Actions-level ways a perfectly-written guard still guards nothing — + # and they never touch the shell, so every check below would pass them: + # `continue-on-error: true` means the `exit 1` does not fail the job and + # the checkout runs anyway, and a step-level `if:` can skip the guard + # outright for some events while the checkout still runs. Neither is + # evaluable here, so both disqualify the step rather than being assumed + # benign. + for line in body: + if _indent(line) != key_indent: + continue + if _STEP_IF_RE.match(line): + return False + cont = _STEP_CONTINUE_RE.match(line) + if cont and _strip_comment(cont.group(2)).lower() not in ("false", ""): + return False + names = _ref_derived_names(body) + empty_re = _empty_test_re(names) + whole_re = _whole_empty_test_re(names) + for i, line in enumerate(body): + if not empty_re.search(line): + continue + code = _RUN_PREFIX_RE.sub("", line.strip()) + cond_match = _IF_COND_RE.match(code) + if cond_match: + # An `if`: the emptiness test must BE the condition, not part of it. + if not whole_re.match(cond_match.group(1).strip()): + continue + # `if [ -z "$REF" ]; then exit 1; fi` all on one line. The branch + # still ends at its `fi` — `then echo "missing"; fi; exit 1` exits + # AFTER the branch, so the empty ref never triggers it. Same + # boundary the multiline path below applies, which is where this + # inline path had quietly stopped agreeing with it. + inline = _INLINE_BRANCH_END_RE.split(cond_match.group(2), 1)[0] + if _exits_unconditionally(inline): + return True + # Otherwise the exit must be inside this test's own branch, which + # ends at the matching `fi`/`else` — an exit after it answers to + # something else entirely. + # …at the branch's OWN depth. An `exit` nested inside an inner + # `if` is conditional on that inner test, so the empty ref can + # still fall through — the multiline twin of the inline rule above. + depth = 0 + for rest in body[i + 1:]: + if depth == 0: + if _BRANCH_END_RE.match(rest): + break + if _GUARD_FAIL_RE.match(rest): + return True + # `\bif\b` does not match inside `elif`, and a nested one-liner + # `if …; then …; fi` opens and closes on the same line. + depth = max(0, depth + len(_IF_OPEN_RE.findall(rest)) - len(_FI_RE.findall(rest))) + else: + # The one-liner `[ -z "$REF" ] && exit 1` — everything left of the + # first `&&` is the condition, and it is held to the same rule. + # The exit must be the command the `&&` actually reaches, so only + # the first `;`-segment counts: `… && echo warn; … && exit 1` + # leaves the empty ref walking on. + head, sep, tail = code.partition("&&") + if sep and whole_re.match(head.strip()): + if _GUARD_FAIL_INLINE_RE.match(tail.split(";")[0]): + return True + return False + + +def find_unguarded_ref_checkouts(lines): + """1-based line numbers of `ref: ${{ inputs.workflows_ref }}` uses with no guard. + + 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. + """ + aliases = env_aliases(lines) + ref_res = _ref_use_res(aliases) + mention_re = re.compile(_mention_alt(aliases)) + jobs_line = None + for i, line in enumerate(lines): + if _is_skippable(line): + continue + if _indent(line) == 0 and _JOBS_RE.match(line): + jobs_line = i + break + if jobs_line is None: + return [] + + job_indent = _first_child_indent(lines, jobs_line, 0) + if job_indent is None: + return [] + job_starts = [ + i + for i, line in _block_body(lines, jobs_line, 0) + if _indent(line) == job_indent + ] + + unguarded = [] + for start in job_starts: + guarded = False + # 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. + pending = None + for i, line in _block_body(lines, start, job_indent): + if pending is not None: + if _indent(line) > pending[1]: + if mention_re.search(line): + if not guarded: + unguarded.append(pending[0] + 1) + 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) + elif is_ref_use(line, ref_res): + if not guarded: + unguarded.append(i + 1) + elif _REF_KEY_OPEN_RE.match(line): + pending = (i, _indent(line)) + return unguarded + + +def check_dir(workflows_dir, exempt=KNOWN_EXEMPT): + """Returns (errors, checked, exempt_ok) — errors are annotation-ready strings.""" + errors = [] + checked = [] + exempt_ok = [] + seen_exempt = set() + + names = sorted( + n + for n in os.listdir(workflows_dir) + if n.endswith((".yml", ".yaml")) and os.path.isfile(os.path.join(workflows_dir, n)) + ) + for name in names: + path = os.path.join(workflows_dir, name) + with open(path, "r", encoding="utf-8", errors="replace") as f: + text = f.read() + lines = text.split("\n") + + defaults = find_workflows_ref_defaults(lines) + if defaults is None: + # "Nothing to check" — unless the file plainly USES the input, in + # which case the text parser lost a declaration that must exist and + # this file is silently uncovered. Fail loudly instead. + if _consumes_input(text): + errors.append( + "::error file=%s::%s references `inputs.%s` but the checker " + "could not find its input declaration — the lint is NOT " + "covering this file. Fix the workflow's shape or teach " + ".github/workflow-pins/check_workflow_pins.py the new one." + % (path, name, INPUT_NAME) + ) + continue + checked.append(name) + + if name in exempt: + seen_exempt.add(name) + if defaults: + exempt_ok.append(name) + else: + errors.append( + "::error file=%s::%s is in KNOWN_EXEMPT but its %s input no " + "longer has a default — delete it from KNOWN_EXEMPT in " + ".github/workflow-pins/check_workflow_pins.py" + % (path, name, INPUT_NAME) + ) + # An exempt workflow still has its default, so an omitted input can + # never reach checkout as ''. The guard is moot until its own ticket + # drops the default — which puts it back under the check below. + continue + + for lineno in defaults: + errors.append( + "::error file=%s,line=%d::%s declares a `default:` for the `%s` " + "input. Delete it (and keep `required: true` + the runtime " + "empty-ref guard): a default lets a caller SHA-pin `uses:` while " + "loading scripts from a mutable ref. See BE-5546." + % (path, lineno, name, INPUT_NAME) + ) + + for lineno in find_unguarded_ref_checkouts(lines): + 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 " + "`Require a pinned workflows_ref` step in ahead of it: `required: " + "true` is unenforced for workflow_call, so an omitted input " + "arrives as '' and checkout silently takes the default branch. " + "See BE-5546." % (path, lineno, name, INPUT_NAME) + ) + + # A KNOWN_EXEMPT entry naming a workflow that no longer declares the input + # at all — renamed, deleted, or fixed. Left alone it would silently + # pre-exempt whatever future workflow reuses the filename. + for name in sorted(set(exempt) - seen_exempt): + errors.append( + "::error::%s is in KNOWN_EXEMPT but no workflow in %s declares a `%s` " + "input under that name (renamed, deleted, or already fixed) — delete " + "it from KNOWN_EXEMPT in " + ".github/workflow-pins/check_workflow_pins.py" + % (name, workflows_dir, INPUT_NAME) + ) + + return errors, checked, exempt_ok + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument( + "--workflows-dir", + default=DEFAULT_WORKFLOWS_DIR, + help="Directory of workflow files to check (default: %s)." % DEFAULT_WORKFLOWS_DIR, + ) + args = parser.parse_args(argv) + + if not os.path.isdir(args.workflows_dir): + print("::error::no such directory: %s" % args.workflows_dir) + return 2 + + # KNOWN_EXEMPT names files in THIS repo's workflows dir, and the staleness + # check reads "an entry with no matching workflow is dead". Applied to an + # ad-hoc --workflows-dir (a fixture, another repo) every entry looks stale, + # so the list only applies where it means something. + exempt = KNOWN_EXEMPT if args.workflows_dir == DEFAULT_WORKFLOWS_DIR else frozenset() + errors, checked, exempt_ok = check_dir(args.workflows_dir, exempt=exempt) + + for name in checked: + note = " (KNOWN_EXEMPT — tracked separately)" if name in exempt_ok else "" + print("checked %s%s" % (name, note)) + if not checked: + print("no reusable workflow declares a `%s` input" % INPUT_NAME) + + for err in errors: + print(err) + if errors: + print( + "\n%d problem(s): a reusable workflow's `%s` input must have NO " + "default, and every job checking out at it must guard against an " + "empty value first." % (len(errors), INPUT_NAME) + ) + return 1 + + print( + "\nOK — %d workflow(s) declare `%s`, none with a default, every ref " + "checkout guarded (%d exempt)." + % (len(checked), INPUT_NAME, len(exempt_ok)) + ) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflow-pins/tests/test_check_workflow_pins.py b/.github/workflow-pins/tests/test_check_workflow_pins.py new file mode 100644 index 0000000..43b1163 --- /dev/null +++ b/.github/workflow-pins/tests/test_check_workflow_pins.py @@ -0,0 +1,801 @@ +#!/usr/bin/env python3 +"""Tests for check_workflow_pins.py — the `workflows_ref` default regression lint. + +The lint is the only thing standing between this repo and a one-line +reintroduction of the BE-5546 hole (a `default: main` on `workflows_ref` lets a +caller SHA-pin `uses:` and still load mutable scripts). Its parsing is +text-level (no PyYAML in this repo), so the fixtures below pin the block +boundaries that text parsing is easy to get wrong: a `workflows_ref:` mentioned +in the header comment, a caller's `with:` value of the same name, and a +`default:` belonging to the NEXT input. +""" + +import os +import shutil +import sys +import tempfile +import unittest + +sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), "..")) + +import check_workflow_pins as cwp # noqa: E402 + + +def _reusable(ref_block, extra_inputs="", header=""): + """A minimal `on: workflow_call` workflow whose workflows_ref block varies.""" + return ( + "name: Fixture\n" + + header + + "\non:\n" + " workflow_call:\n" + " inputs:\n" + " some_other:\n" + " type: string\n" + " required: false\n" + " default: hello\n" + " workflows_ref:\n" + ref_block + extra_inputs + "\n" + "jobs:\n" + " check:\n" + " runs-on: ubuntu-latest\n" + " steps:\n" + " - run: echo hi\n" + ) + + +PINNED = " description: Ref to load scripts from.\n type: string\n required: true\n" +DEFAULTED = PINNED.replace("required: true", "required: false") + " default: main\n" + + +class FindDefaultsTests(unittest.TestCase): + def _find(self, text): + return cwp.find_workflows_ref_defaults(text.split("\n")) + + def test_required_no_default_is_clean(self): + self.assertEqual(self._find(_reusable(PINNED)), []) + + def test_default_is_reported_with_its_line_number(self): + text = _reusable(DEFAULTED) + hits = self._find(text) + self.assertEqual(len(hits), 1, hits) + self.assertEqual(text.split("\n")[hits[0] - 1].strip(), "default: main") + + def test_a_later_inputs_default_is_not_attributed_to_workflows_ref(self): + # The block ends at the next key of the same indentation. A naive + # "search forward for default:" would blame this one on workflows_ref. + trailing = ( + " verbosity:\n" + " type: string\n" + " required: false\n" + " default: quiet\n" + ) + self.assertEqual(self._find(_reusable(PINNED, extra_inputs=trailing)), []) + self.assertEqual(len(self._find(_reusable(DEFAULTED, extra_inputs=trailing))), 1) + + def test_header_comment_mentioning_the_input_is_ignored(self): + header = ( + "# Caller pattern:\n" + "# with:\n" + "# workflows_ref: main # <- prose, not a declaration\n" + "# default: main\n" + ) + self.assertEqual(self._find(_reusable(PINNED, header=header)), []) + + def test_a_caller_workflow_is_not_a_reusable_workflow(self): + # `workflows_ref:` here is a `with:` VALUE in a caller — nothing to check. + caller = ( + "name: CI\n" + "on:\n" + " pull_request:\n" + "jobs:\n" + " review:\n" + " uses: Comfy-Org/github-workflows/.github/workflows/x.yml@abc # v1\n" + " with:\n" + " workflows_ref: abc\n" + ) + self.assertIsNone(self._find(caller)) + + def test_reusable_without_the_input_is_skipped(self): + text = ( + "name: Fixture\n" + "on:\n" + " workflow_call:\n" + " inputs:\n" + " max_lines:\n" + " type: number\n" + " required: false\n" + " default: 200\n" + ) + self.assertIsNone(self._find(text)) + + def test_inline_on_forms_are_skipped(self): + self.assertIsNone(self._find("name: F\non: [push]\njobs: {}\n")) + self.assertIsNone(self._find("name: F\non: push\njobs: {}\n")) + + def test_quoted_on_key_is_still_parsed(self): + # YAML 1.1 turns a bare `on` into True, so some repos quote the key. + self.assertEqual(len(self._find(_reusable(DEFAULTED).replace("\non:", '\n"on":'))), 1) + + def test_a_trailing_comment_on_the_on_key_is_not_an_inline_trigger_list(self): + # `on: # triggers` is the block form. Reading the comment as an inline + # value would drop the whole file from the lint while CI still says OK. + text = _reusable(DEFAULTED).replace("\non:\n", "\non: # when this runs\n") + self.assertEqual(len(self._find(text)), 1) + + def test_an_inline_trigger_list_with_a_comment_is_still_skipped(self): + self.assertIsNone(self._find("name: F\non: [push] # only pushes\njobs: {}\n")) + + def test_flow_mapping_default_on_the_input_line_is_caught(self): + # The one-line reintroduction: no child lines for a block scan to walk. + text = _reusable("").replace( + " workflows_ref:\n", + " workflows_ref: {type: string, required: false, default: main}\n", + ) + hits = self._find(text) + self.assertEqual(len(hits), 1, hits) + self.assertIn("default: main", text.split("\n")[hits[0] - 1]) + + def test_flow_mapping_without_a_default_is_clean(self): + text = _reusable("").replace( + " workflows_ref:\n", + " workflows_ref: {type: string, required: true}\n", + ) + self.assertEqual(self._find(text), []) + + def test_a_default_inside_a_folded_description_is_not_a_declaration(self): + # `description: >-` continuation lines are indented DEEPER than the + # input's own properties. This diff's own prose ("There is deliberately + # no default: …") is one reflow away from starting such a line. + folded = ( + " description: >-\n" + " Ref to load scripts from. There is deliberately no\n" + " default: a floating default would defeat the pin.\n" + " type: string\n" + " required: true\n" + ) + self.assertEqual(self._find(_reusable(folded)), []) + + def test_quoted_keys_do_not_hide_a_declaration_or_a_default(self): + # Quoting any of these is valid Actions YAML and must not be an escape + # hatch — the same one `on` already had. + text = ( + _reusable(DEFAULTED) + .replace(" workflow_call:", ' "workflow_call":') + .replace(" inputs:", ' "inputs":') + .replace(" workflows_ref:", ' "workflows_ref":') + .replace(" default: main", ' "default": main') + ) + self.assertEqual(len(self._find(text)), 1) + + +class GuardCoverageTests(unittest.TestCase): + """Every `ref: ${{ inputs.workflows_ref }}` needs the guard in its own job. + + Dropping the default is only half the fix: a NEW job (or a new reusable + workflow) that checks out at the ref without the guard reopens the `ref: ''` + default-branch fallback, and the default-only lint stays green because it + never declared a default to begin with. + """ + + # The real guard's shape: it RECEIVES the ref through `env:` and REJECTS an + # empty one. Both halves matter to the detector — see + # `test_a_step_that_only_handles_the_ref_is_not_a_guard`. + GUARD = ( + " - name: Require a pinned workflows_ref\n" + " env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref }}\n" + " run: |\n" + ' REF="$(printf \'%s\' "$WORKFLOWS_REF" | tr -d \'[:space:]\')"\n' + ' if [ -z "$REF" ]; then\n' + " exit 1\n" + " fi\n" + ) + CHECKOUT = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ inputs.workflows_ref }}\n" + ) + # Same checkout, written as a one-line flow mapping — the shape that walked + # past a `ref:`-at-line-start anchor while the hole stayed wide open. + FLOW_CHECKOUT = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + ' with: {repository: Comfy-Org/github-workflows, ref: "${{ inputs.workflows_ref }}"}\n' + ) + + def _jobs(self, *jobs): + 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")) + + def test_guarded_checkout_passes(self): + self.assertEqual(self._jobs(self.GUARD + self.CHECKOUT), []) + + def test_unguarded_checkout_is_reported(self): + self.assertEqual(len(self._jobs(self.CHECKOUT)), 1) + + def test_a_guard_after_the_checkout_does_not_count(self): + self.assertEqual(len(self._jobs(self.CHECKOUT + self.GUARD)), 1) + + def test_a_guard_in_another_job_does_not_count(self): + # Jobs run independently — job A's guard protects nothing in job B. + self.assertEqual(len(self._jobs(self.GUARD, self.CHECKOUT)), 1) + + def test_each_job_is_judged_on_its_own_guard(self): + self.assertEqual(self._jobs(self.GUARD + self.CHECKOUT, self.GUARD + self.CHECKOUT), []) + + # A step that RECEIVES the ref but never tests it. Keying the guard on its + # `env:` binding alone let this mark the whole job guarded, so every later + # checkout passed unexamined — the lint's own subject, failing silently. + DECOY = ( + " - name: Print the ref\n" + " env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref }}\n" + ' run: echo "$WORKFLOWS_REF"\n' + ) + + # The NEAR match: an emptiness test and a non-zero exit are both present, + # but they are about an unrelated variable and an unrelated condition. + # "a `-z` somewhere, an `exit` somewhere" passed this, and it is a likelier + # accident than the bare decoy above — arg validation next to a clone. + NEAR_MISS = ( + " - name: Clone at the ref\n" + " env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref }}\n" + " run: |\n" + ' if [ -z "$UNRELATED" ]; then\n' + ' echo "unrelated value is empty"\n' + " fi\n" + ' if [ "$UNRELATED" = "blocked" ]; then\n' + " exit 1\n" + " fi\n" + ) + # Tests the RIGHT variable, but only warns — the exit belongs to a later, + # separate branch, so an empty ref still reaches the checkout. + NO_EXIT_IN_BRANCH = ( + " - name: Warn only\n" + " env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref }}\n" + " run: |\n" + ' if [ -z "$WORKFLOWS_REF" ]; then\n' + ' echo "::warning::no ref"\n' + " fi\n" + ' if [ "$OTHER" = "x" ]; then\n' + " exit 1\n" + " fi\n" + ) + + def test_a_step_that_only_handles_the_ref_is_not_a_guard(self): + self.assertEqual(len(self._jobs(self.DECOY + self.CHECKOUT)), 1) + + def test_an_unrelated_test_and_an_unrelated_exit_are_not_a_guard(self): + self.assertEqual(len(self._jobs(self.NEAR_MISS + self.CHECKOUT)), 1) + + def test_the_exit_must_be_in_the_empty_branch(self): + self.assertEqual(len(self._jobs(self.NO_EXIT_IN_BRANCH + self.CHECKOUT)), 1) + + def test_a_one_line_empty_test_counts(self): + one_liner = ( + " - name: Require a pinned workflows_ref\n" + " env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref }}\n" + ' run: [ -z "$WORKFLOWS_REF" ] && exit 1\n' + ) + self.assertEqual(self._jobs(one_liner + self.CHECKOUT), []) + + def test_a_compound_condition_is_not_a_guard(self): + # `-z "$REF" && OTHER = blocked` contains the emptiness test but does + # not fail for EVERY empty ref: empty + `OTHER` unset falls straight + # through to the checkout. A text lint cannot evaluate shell, so an + # ANDed condition is rejected as ambiguous rather than trusted. + compound = self.GUARD.replace( + 'if [ -z "$REF" ]; then', + 'if [ -z "$REF" ] && [ "$OTHER" = "blocked" ]; then', + ) + self.assertEqual(len(self._jobs(compound + self.CHECKOUT)), 1) + + def test_a_single_line_if_then_exit_counts(self): + one_line_if = ( + " - name: Require a pinned workflows_ref\n" + " env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref }}\n" + ' run: if [ -z "$WORKFLOWS_REF" ]; then exit 1; fi\n' + ) + self.assertEqual(self._jobs(one_line_if + self.CHECKOUT), []) + + def test_an_inline_exit_after_the_branch_closes_is_not_a_guard(self): + # `then echo "missing"; fi; exit 1` — the branch does not exit, so an + # empty ref walks on to the checkout; the trailing exit answers to + # nothing. The multiline path already stopped at `fi`; the inline one + # had quietly stopped agreeing with it. + after_fi = ( + " - name: Decoy\n" + " env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref }}\n" + ' run: if [ -z "$WORKFLOWS_REF" ]; then echo "missing"; fi; exit 1\n' + ) + self.assertEqual(len(self._jobs(after_fi + self.CHECKOUT)), 1) + + def _run_step(self, script): + return ( + " - name: S\n" + " env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref }}\n" + " run: %s\n" % script + ) + + def _guard_with(self, extra_keys): + return self.GUARD.replace( + " - name: Require a pinned workflows_ref\n", + " - name: Require a pinned workflows_ref\n" + extra_keys, + ) + + def test_a_skippable_guard_step_is_not_a_guard(self): + # A step-level `if:` can skip the guard outright while the checkout + # still runs. The shell inside is impeccable and guards nothing. + step = self._guard_with(" if: github.event_name == 'push'\n") + self.assertEqual(len(self._jobs(step + self.CHECKOUT)), 1) + + def test_a_continue_on_error_guard_is_not_a_guard(self): + # `exit 1` that does not fail the job: the checkout runs regardless. + step = self._guard_with(" continue-on-error: true\n") + self.assertEqual(len(self._jobs(step + self.CHECKOUT)), 1) + # …and an expression is not evaluable here, so it disqualifies too. + expr = self._guard_with(" continue-on-error: ${{ inputs.soft }}\n") + self.assertEqual(len(self._jobs(expr + self.CHECKOUT)), 1) + + def test_continue_on_error_false_is_still_a_guard(self): + step = self._guard_with(" continue-on-error: false\n") + self.assertEqual(self._jobs(step + self.CHECKOUT), []) + + def test_a_conditional_exit_inside_the_branch_is_not_a_guard(self): + # The branch is entered on an empty ref but only exits if `$X` matches, + # so the empty ref still reaches the checkout. The multiline path + # already required a bare `exit`; the inline path had not. + step = self._run_step( + 'if [ -z "$WORKFLOWS_REF" ]; then [ "$X" = y ] && exit 1; fi' + ) + self.assertEqual(len(self._jobs(step + self.CHECKOUT)), 1) + + def test_a_nested_conditional_exit_is_not_a_guard_in_a_script_block(self): + # The multiline twin of the test above: the empty-ref branch is + # entered, but its only `exit` sits inside an inner `if`, so an empty + # ref still reaches the checkout. One rule, two paths. + step = ( + " - name: S\n" + " env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref }}\n" + " run: |\n" + ' if [ -z "$WORKFLOWS_REF" ]; then\n' + ' if [ "$X" = y ]; then\n' + " exit 1\n" + " fi\n" + " fi\n" + ) + self.assertEqual(len(self._jobs(step + self.CHECKOUT)), 1) + + def test_an_exit_after_a_nested_block_still_counts(self): + # …but nesting must not blind the scan to the branch's own exit. + step = ( + " - name: S\n" + " env:\n" + " WORKFLOWS_REF: ${{ inputs.workflows_ref }}\n" + " run: |\n" + ' if [ -z "$WORKFLOWS_REF" ]; then\n' + ' if [ "$X" = y ]; then\n' + ' echo "note"\n' + " fi\n" + " exit 1\n" + " fi\n" + ) + self.assertEqual(self._jobs(step + self.CHECKOUT), []) + + def test_the_one_liner_exit_must_be_what_the_and_reaches(self): + # `… && echo warn; … && exit 1` — the `&&` reaches only the echo. + step = self._run_step( + '[ -z "$WORKFLOWS_REF" ] && echo warn; [ "$X" = y ] && exit 1' + ) + self.assertEqual(len(self._jobs(step + self.CHECKOUT)), 1) + + def test_the_guard_may_test_a_variable_derived_from_the_ref(self): + # The real guard tests `$REF`, assigned from `$WORKFLOWS_REF` — but the + # hop has to actually carry the value. + self.assertEqual(self._jobs(self.GUARD + self.CHECKOUT), []) + unrelated_hop = self.GUARD.replace( + "REF=\"$(printf '%s' \"$WORKFLOWS_REF\" | tr -d '[:space:]')\"", + 'REF="$(cat /etc/hostname)"', + ) + self.assertEqual(len(self._jobs(unrelated_hop + self.CHECKOUT)), 1) + + def test_a_decoy_before_the_real_guard_still_passes(self): + # The decoy must not POISON a job that does guard — only fail to excuse + # one that does not. + self.assertEqual(self._jobs(self.DECOY + self.GUARD + self.CHECKOUT), []) + + def test_a_job_level_env_hoist_is_not_a_guard(self): + # Hoisting the ref to a job-level `env:` is the natural refactor once + # several steps want it. It binds the value but rejects nothing, and it + # is not a step at all — so the checkout below it is unguarded, and the + # `ref: ${{ env.WORKFLOWS_REF }}` spelling must still read as a ref use. + text = ( + "name: F\non:\n workflow_call:\njobs:\n" + " job0:\n runs-on: ubuntu-latest\n" + " env:\n WORKFLOWS_REF: ${{ inputs.workflows_ref }}\n" + " steps:\n" + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ env.WORKFLOWS_REF }}\n" + ) + self.assertEqual(len(cwp.find_unguarded_ref_checkouts(text.split("\n"))), 1) + + ALIASED_CHECKOUT = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ env.WORKFLOWS_REF }}\n" + ) + + def test_an_aliased_checkout_still_needs_a_guard_in_its_own_job(self): + # The guard in job A binds the name; job B checks out at it with no + # guard of its own. Reading only `inputs.` made job B's checkout vanish. + self.assertEqual(len(self._jobs(self.GUARD, self.ALIASED_CHECKOUT)), 1) + + def test_an_aliased_checkout_behind_the_guard_passes(self): + self.assertEqual(self._jobs(self.GUARD + self.ALIASED_CHECKOUT), []) + + def test_only_env_blocks_bind_an_alias(self): + # The checkout's own `ref:` is a mapping key bound to the input too. + # Collecting it as an alias would make `env.ref`/`$ref` anywhere read + # as the input. + lines = (self.GUARD + self.CHECKOUT).split("\n") + self.assertEqual(cwp.env_aliases(lines), frozenset({"WORKFLOWS_REF"})) + + def test_an_unbound_env_name_is_not_a_ref_use(self): + # Nothing binds this name to the input, so it is an unrelated variable + # — demanding a guard for it would fail a compliant workflow. + self.assertEqual(self._jobs(self.ALIASED_CHECKOUT), []) + + def test_a_flow_mapping_checkout_is_not_an_escape_hatch(self): + # `with: {…, ref: …}` on one line is the same unguarded checkout, and + # anchoring on `ref:` at line start reported nothing at all for it. + self.assertEqual(len(self._jobs(self.FLOW_CHECKOUT)), 1) + + def test_a_guarded_flow_mapping_checkout_passes(self): + self.assertEqual(self._jobs(self.GUARD + self.FLOW_CHECKOUT), []) + + def test_a_sibling_flow_entry_is_not_read_as_the_ref(self): + # `ref:` is pinned to a literal here; the input feeds a DIFFERENT key. + # Matching greedily across the whole line would call this a ref use and + # fail a workflow that never checks out at the input. + step = ( + " - name: Not a ref checkout\n" + ' with: {ref: v1, path: "${{ inputs.workflows_ref }}"}\n' + ) + self.assertEqual(self._jobs(step), []) + + # The same checkout again, with the value on the FOLLOWING line. Neither + # same-line pattern sees an `inputs.` on the `ref:` line, so before the + # continuation scan these reported nothing for a job with no guard at all. + FOLDED_CHECKOUT = ( + " - name: Load assets\n" + " uses: actions/checkout@abc\n" + " with:\n" + " ref: >-\n" + " ${{ inputs.workflows_ref }}\n" + ) + LITERAL_CHECKOUT = FOLDED_CHECKOUT.replace("ref: >-", "ref: |") + PLAIN_CHECKOUT = FOLDED_CHECKOUT.replace("ref: >-", "ref:") + # …and the same two with a comment where the value would sit. A comment does + # not end the mapping value: both still take the ref from the line below. + COMMENTED_CHECKOUT = FOLDED_CHECKOUT.replace("ref: >-", "ref: # the pinned ref") + COMMENTED_FOLDED_CHECKOUT = FOLDED_CHECKOUT.replace("ref: >-", "ref: >- # pinned") + + def test_a_folded_scalar_checkout_is_not_an_escape_hatch(self): + self.assertEqual(len(self._jobs(self.FOLDED_CHECKOUT)), 1) + + def test_a_literal_scalar_checkout_is_not_an_escape_hatch(self): + self.assertEqual(len(self._jobs(self.LITERAL_CHECKOUT)), 1) + + def test_a_plain_multiline_checkout_is_not_an_escape_hatch(self): + self.assertEqual(len(self._jobs(self.PLAIN_CHECKOUT)), 1) + + def test_a_commented_ref_key_checkout_is_not_an_escape_hatch(self): + # `ref: # pinned` with the value below is a working checkout, but the + # comment left the key line looking like an ordinary finished scalar, so + # the continuation scan never opened and the job read as having no + # checkout at all — silently, since `_consumes_input` missed it too. + self.assertEqual(len(self._jobs(self.COMMENTED_CHECKOUT)), 1) + + def test_a_comment_after_a_block_header_is_not_an_escape_hatch(self): + # YAML allows a comment after `|`/`>`; the value still follows below. + self.assertEqual(len(self._jobs(self.COMMENTED_FOLDED_CHECKOUT)), 1) + + def test_a_guarded_multiline_checkout_passes(self): + self.assertEqual(self._jobs(self.GUARD + self.FOLDED_CHECKOUT), []) + + def test_a_guarded_commented_checkout_passes(self): + self.assertEqual(self._jobs(self.GUARD + self.COMMENTED_CHECKOUT), []) + + def test_a_trailing_comment_on_the_guard_still_counts_as_the_guard(self): + # The guard is doing its job; refusing to see it would fail a compliant + # workflow, which is the opposite of the flow-form guard's trade-off. + guard = self.GUARD.replace( + "WORKFLOWS_REF: ${{ inputs.workflows_ref }}", + "WORKFLOWS_REF: ${{ inputs.workflows_ref }} # the pinned ref", + ) + self.assertEqual(self._jobs(guard + self.CHECKOUT), []) + + def test_a_multiline_ref_pinned_to_a_literal_is_not_a_use(self): + # The window a `ref:` key opens is not itself a finding: this checkout + # never names the input, and failing it would fail a compliant workflow. + step = ( + " - name: Literal ref\n" + " with:\n" + " ref: >-\n" + " main\n" + ) + self.assertEqual(self._jobs(step), []) + + def test_the_input_after_a_ref_scalar_closes_is_not_attributed_to_it(self): + # `ref:` is pinned; the input feeds a LATER, shallower key. Running the + # continuation scan past the scalar's end would blame it on the `ref:`. + step = ( + " - name: Literal ref\n" + " with:\n" + " ref: >-\n" + " main\n" + " path: ${{ inputs.workflows_ref }}\n" + ) + self.assertEqual(self._jobs(step), []) + + def test_this_repos_own_workflows_guard_every_ref_checkout(self): + root = os.path.normpath( + os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..", "workflows") + ) + seen = 0 + for name in ("cursor-review.yml", "groom.yml", "agents-md-integrity.yml"): + with open(os.path.join(root, name), encoding="utf-8") as f: + lines = f.read().split("\n") + uses = [line for line in lines if cwp.is_ref_use(line)] + self.assertTrue(uses, "%s: no ref checkout found — fixture drifted" % name) + seen += len(uses) + self.assertEqual(cwp.find_unguarded_ref_checkouts(lines), [], name) + self.assertEqual(seen, 12, "expected the 12 guarded sites BE-5546 fixed") + + +class CheckDirTests(unittest.TestCase): + def setUp(self): + self.dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.dir, True) + + def _write(self, name, text): + with open(os.path.join(self.dir, name), "w", encoding="utf-8") as f: + f.write(text) + + def test_clean_dir_passes(self): + self._write("good.yml", _reusable(PINNED)) + self._write("unrelated.yml", "name: F\non: [push]\njobs: {}\n") + errors, checked, exempt_ok = cwp.check_dir(self.dir, exempt=frozenset()) + self.assertEqual(errors, []) + self.assertEqual(checked, ["good.yml"]) + self.assertEqual(exempt_ok, []) + + def test_defaulted_dir_fails_with_an_annotation(self): + self._write("bad.yml", _reusable(DEFAULTED)) + errors, checked, _ = cwp.check_dir(self.dir, exempt=frozenset()) + self.assertEqual(len(errors), 1, errors) + self.assertTrue(errors[0].startswith("::error file="), errors[0]) + self.assertIn("bad.yml", errors[0]) + self.assertIn("BE-5546", errors[0]) + self.assertEqual(checked, ["bad.yml"]) + + def test_exempt_workflow_is_tolerated(self): + self._write("legacy.yml", _reusable(DEFAULTED)) + errors, checked, exempt_ok = cwp.check_dir(self.dir, exempt=frozenset({"legacy.yml"})) + self.assertEqual(errors, []) + self.assertEqual(exempt_ok, ["legacy.yml"]) + self.assertEqual(checked, ["legacy.yml"]) + + def test_stale_exemption_fails_so_the_list_drains(self): + self._write("legacy.yml", _reusable(PINNED)) + errors, _, exempt_ok = cwp.check_dir(self.dir, exempt=frozenset({"legacy.yml"})) + self.assertEqual(len(errors), 1, errors) + self.assertIn("KNOWN_EXEMPT", errors[0]) + self.assertEqual(exempt_ok, []) + + def test_a_lost_declaration_is_an_error_not_a_silent_skip(self): + # The file plainly USES the input, so a declaration must exist. If the + # text parser cannot find it, the file is uncovered — which must look + # different from "not applicable", not identical to it. + self._write( + "unparseable.yml", + "name: F\n" + "on:\n" + " workflow_call:\n" + "jobs:\n" + " j:\n" + " runs-on: ubuntu-latest\n" + " steps:\n" + " - uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ inputs.workflows_ref }}\n", + ) + errors, checked, _ = cwp.check_dir(self.dir, exempt=frozenset()) + self.assertEqual(len(errors), 1, errors) + self.assertIn("NOT covering this file", errors[0]) + self.assertEqual(checked, []) + + def test_a_lost_declaration_is_caught_through_a_flow_mapping_use(self): + # Same uncovered file, with its only use written in flow style. Before + # the flow patterns this returned zero errors — the loudest failure the + # checker has, silenced by a pair of braces. + self._write( + "unparseable.yml", + "name: F\n" + "on:\n" + " workflow_call:\n" + "jobs:\n" + " j:\n" + " runs-on: ubuntu-latest\n" + " steps:\n" + " - uses: actions/checkout@abc\n" + ' with: {repository: Comfy-Org/github-workflows, ref: "${{ inputs.workflows_ref }}"}\n', + ) + errors, checked, _ = cwp.check_dir(self.dir, exempt=frozenset()) + self.assertEqual(len(errors), 1, errors) + self.assertIn("NOT covering this file", errors[0]) + self.assertEqual(checked, []) + + def test_a_lost_declaration_is_caught_through_a_block_scalar_use(self): + # And once more with the value on the next line — the third spelling of + # the same use, which has to stay just as loud as the other two. + self._write( + "unparseable.yml", + "name: F\n" + "on:\n" + " workflow_call:\n" + "jobs:\n" + " j:\n" + " runs-on: ubuntu-latest\n" + " steps:\n" + " - uses: actions/checkout@abc\n" + " with:\n" + " ref: >-\n" + " ${{ inputs.workflows_ref }}\n", + ) + errors, checked, _ = cwp.check_dir(self.dir, exempt=frozenset()) + self.assertEqual(len(errors), 1, errors) + self.assertIn("NOT covering this file", errors[0]) + self.assertEqual(checked, []) + + def test_an_unguarded_block_scalar_checkout_fails_the_lint(self): + # End to end: a declared, default-free workflow whose only checkout is + # written vertically and unguarded still exits non-zero. + self._write( + "leaky.yml", + _reusable(PINNED).replace( + " - run: echo hi\n", + " - uses: actions/checkout@abc\n" + " with:\n" + " ref: >-\n" + " ${{ inputs.workflows_ref }}\n", + ), + ) + errors, checked, _ = cwp.check_dir(self.dir, exempt=frozenset()) + self.assertEqual(len(errors), 1, errors) + self.assertIn("no empty-ref guard", errors[0]) + self.assertEqual(checked, ["leaky.yml"]) + + def test_a_lost_declaration_is_caught_through_a_commented_ref_key(self): + # The fourth spelling: a comment where the value would go. This one was + # doubly silent — invisible to the guard scan AND to this backstop, so a + # whole uncovered file passed clean rather than failing loudly. + self._write( + "unparseable.yml", + "name: F\n" + "on:\n" + " workflow_call:\n" + "jobs:\n" + " j:\n" + " runs-on: ubuntu-latest\n" + " steps:\n" + " - uses: actions/checkout@abc\n" + " with:\n" + " ref: # the pinned ref\n" + " ${{ inputs.workflows_ref }}\n", + ) + errors, checked, _ = cwp.check_dir(self.dir, exempt=frozenset()) + self.assertEqual(len(errors), 1, errors) + self.assertIn("NOT covering this file", errors[0]) + self.assertEqual(checked, []) + + def test_an_unguarded_commented_ref_checkout_fails_the_lint(self): + self._write( + "leaky.yml", + _reusable(PINNED).replace( + " - run: echo hi\n", + " - uses: actions/checkout@abc\n" + " with:\n" + " ref: # the pinned ref\n" + " ${{ inputs.workflows_ref }}\n", + ), + ) + errors, checked, _ = cwp.check_dir(self.dir, exempt=frozenset()) + self.assertEqual(len(errors), 1, errors) + self.assertIn("no empty-ref guard", errors[0]) + self.assertEqual(checked, ["leaky.yml"]) + + def test_an_unrelated_workflow_is_still_a_silent_skip(self): + self._write("unrelated.yml", "name: F\non: [push]\njobs: {}\n") + errors, checked, _ = cwp.check_dir(self.dir, exempt=frozenset()) + self.assertEqual((errors, checked), ([], [])) + + def test_an_unguarded_ref_checkout_fails_the_lint(self): + self._write( + "leaky.yml", + _reusable(PINNED).replace( + " - run: echo hi\n", + " - uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ inputs.workflows_ref }}\n", + ), + ) + errors, checked, _ = cwp.check_dir(self.dir, exempt=frozenset()) + self.assertEqual(len(errors), 1, errors) + self.assertIn("no empty-ref guard", errors[0]) + self.assertEqual(checked, ["leaky.yml"]) + + def test_an_exempt_workflow_is_not_held_to_the_guard(self): + # It still has its default, so an omitted input never reaches checkout + # as '' — the guard only becomes required when the default goes. + self._write( + "legacy.yml", + _reusable(DEFAULTED).replace( + " - run: echo hi\n", + " - uses: actions/checkout@abc\n" + " with:\n" + " ref: ${{ inputs.workflows_ref }}\n", + ), + ) + errors, _, exempt_ok = cwp.check_dir(self.dir, exempt=frozenset({"legacy.yml"})) + self.assertEqual(errors, []) + self.assertEqual(exempt_ok, ["legacy.yml"]) + + def test_an_exemption_for_a_missing_file_fails(self): + # Rename or delete the workflow and the entry would otherwise survive + # forever, pre-exempting whatever later reuses the filename. + self._write("good.yml", _reusable(PINNED)) + errors, _, _ = cwp.check_dir(self.dir, exempt=frozenset({"renamed-away.yml"})) + self.assertEqual(len(errors), 1, errors) + self.assertIn("renamed-away.yml", errors[0]) + self.assertIn("KNOWN_EXEMPT", errors[0]) + + def test_an_exemption_for_a_workflow_that_dropped_the_input_fails(self): + self._write("legacy.yml", "name: F\non: [push]\njobs: {}\n") + errors, _, _ = cwp.check_dir(self.dir, exempt=frozenset({"legacy.yml"})) + self.assertEqual(len(errors), 1, errors) + self.assertIn("KNOWN_EXEMPT", errors[0]) + + def test_the_real_known_exempt_list_is_not_stale(self): + # KNOWN_EXEMPT is checked against the real tree by the default run too; + # this pins it so a rename cannot quietly widen the exemption. + root = os.path.normpath( + os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..", "workflows") + ) + errors, checked, exempt_ok = cwp.check_dir(root) + self.assertEqual(errors, [], errors) + self.assertEqual(sorted(exempt_ok), sorted(cwp.KNOWN_EXEMPT)) + + def test_this_repos_own_workflows_pass(self): + # The real forcing function: the checked-in tree must stay clean. + root = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..", "workflows") + errors, checked, _ = cwp.check_dir(os.path.normpath(root)) + self.assertEqual(errors, [], errors) + for name in ("cursor-review.yml", "groom.yml", "agents-md-integrity.yml"): + self.assertIn(name, checked) + + +if __name__ == "__main__": + unittest.main(verbosity=2) diff --git a/.github/workflows/agents-md-integrity.yml b/.github/workflows/agents-md-integrity.yml index 7d36318..b92226a 100644 --- a/.github/workflows/agents-md-integrity.yml +++ b/.github/workflows/agents-md-integrity.yml @@ -27,9 +27,9 @@ name: AGENTS.md Integrity (reusable) # contents: read # uses: Comfy-Org/github-workflows/.github/workflows/agents-md-integrity.yml@ # v1 # with: -# workflows_ref: # pin the checker to the same ref as `uses:` +# workflows_ref: # REQUIRED — same SHA as `uses:` above # -# INPUTS (all optional): +# INPUTS (`workflows_ref` is REQUIRED; all others optional): # max_lines Hard line ceiling for AGENTS.md; over it FAILS. # Default 200 (official Anthropic guidance). # warn_lines Aspirational target; over it emits a non-fatal WARNING @@ -48,9 +48,10 @@ name: AGENTS.md Integrity (reusable) # (default) an unowned AGENTS.md only WARNS; when true it # FAILS. Default false (warn only). # agents_file Path to the agents file to check. Default `AGENTS.md`. -# workflows_ref Ref of Comfy-Org/github-workflows to load the check -# script from. Pin this to the same ref you pin `uses:` -# to for reproducibility. Default `main`. +# workflows_ref REQUIRED. Ref of Comfy-Org/github-workflows to load the +# check script from. Must be the SAME commit SHA you pin +# `uses:` to — no default, so a caller can't pin the +# workflow while loading the checker from a mutable branch. # # No secrets required. @@ -103,11 +104,13 @@ on: default: AGENTS.md workflows_ref: description: >- - Ref of Comfy-Org/github-workflows to load the check script from. Pin - this to the same ref you pin `uses:` to for reproducibility. + REQUIRED. Ref of Comfy-Org/github-workflows to load the check script + from. Must be the SAME commit SHA you pin `uses:` to — otherwise the + workflow is pinned but the code it runs is not. There is deliberately + no default: a floating `main` default would silently load a mutable + checker. type: string - required: false - default: main + required: true permissions: contents: read @@ -124,6 +127,30 @@ jobs: with: persist-credentials: false + - name: Require a pinned workflows_ref + # workflows_ref has no default on purpose. GitHub does NOT enforce + # `required: true` for workflow_call inputs, so an omitted input arrives + # as '' and actions/checkout would silently fall back to this repo's + # default branch — running MUTABLE scripts under a pin that claims + # otherwise. Fail fast instead. (BE-5546) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + # 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, and echo only that: dropping + # newlines also stops a multi-line value from smuggling a ::workflow + # command:: into the log, and from satisfying the line-oriented grep + # below on one 40-hex line among many. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::workflows_ref is required; pin it to the same commit SHA as the uses: line (see header example)" + exit 1 + fi + if ! printf '%s' "$REF" | grep -Eq '^[0-9a-f]{40}$'; then + echo "::warning::workflows_ref '$REF' is not a full 40-hex commit SHA — branch and tag refs are mutable and can skew between jobs mid-run" + fi + - name: Load integrity check script # The checker comes from THIS repo (public, pinned via workflows_ref), # never from the caller's checkout, so a PR can't rewrite the check. diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index d7d83b2..01b9433 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -24,15 +24,15 @@ name: Cursor Review (reusable) # cancel-in-progress: true # jobs: # cursor-review: -# uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@main +# uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@ # v1 # with: # # Repo-specific pathspecs excluded from the size cap and the diff. # diff_excludes: >- # :!**/package-lock.json # :!data/object_info.json.gz -# # Pin the assets ref to the same ref you pin `uses:` to for -# # reproducibility (defaults to main). -# workflows_ref: main +# # REQUIRED. Pin the assets ref to the SAME commit SHA you pin `uses:` +# # to — otherwise the workflow is pinned but its scripts are not. +# workflows_ref: # # Optional: post the review under your own GitHub App so its threads are # # a distinct, queryable identity instead of github-actions[bot]. Supply # # your App's id + private key (App IDs aren't secret, so id is an input). @@ -91,11 +91,13 @@ on: :!**/*.min.css workflows_ref: description: >- - Ref of Comfy-Org/github-workflows to load the prompts and scripts - from. Pin this to the same ref you pin `uses:` to for reproducibility. + REQUIRED. Ref of Comfy-Org/github-workflows to load the prompts and + scripts from. Must be the SAME commit SHA you pin `uses:` to — + otherwise the workflow is pinned but the code it runs is not. There is + deliberately no default: a floating `main` default would silently load + mutable scripts into jobs that hold write permissions. type: string - required: false - default: main + required: true bot_app_id: description: >- GitHub App ID. When set (with the bot_app_private_key secret), the @@ -461,6 +463,30 @@ jobs: fetch-depth: 0 persist-credentials: false + - name: Require a pinned workflows_ref + # workflows_ref has no default on purpose. GitHub does NOT enforce + # `required: true` for workflow_call inputs, so an omitted input arrives + # as '' and actions/checkout would silently fall back to this repo's + # default branch — running MUTABLE scripts under a pin that claims + # otherwise. Fail fast instead. (BE-5546) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + # 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, and echo only that: dropping + # newlines also stops a multi-line value from smuggling a ::workflow + # command:: into the log, and from satisfying the line-oriented grep + # below on one 40-hex line among many. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::workflows_ref is required; pin it to the same commit SHA as the uses: line (see header example)" + exit 1 + fi + if ! printf '%s' "$REF" | grep -Eq '^[0-9a-f]{40}$'; then + echo "::warning::workflows_ref '$REF' is not a full 40-hex commit SHA — branch and tag refs are mutable and can skew between jobs mid-run" + fi + - name: Load cursor-review assets # Trusted prompts/scripts come from THIS workflow's repo (public, # pinned via workflows_ref) — never from the PR checkout, so a @@ -592,6 +618,30 @@ jobs: fetch-depth: 0 persist-credentials: false + - name: Require a pinned workflows_ref + # workflows_ref has no default on purpose. GitHub does NOT enforce + # `required: true` for workflow_call inputs, so an omitted input arrives + # as '' and actions/checkout would silently fall back to this repo's + # default branch — running MUTABLE scripts under a pin that claims + # otherwise. Fail fast instead. (BE-5546) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + # 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, and echo only that: dropping + # newlines also stops a multi-line value from smuggling a ::workflow + # command:: into the log, and from satisfying the line-oriented grep + # below on one 40-hex line among many. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::workflows_ref is required; pin it to the same commit SHA as the uses: line (see header example)" + exit 1 + fi + if ! printf '%s' "$REF" | grep -Eq '^[0-9a-f]{40}$'; then + echo "::warning::workflows_ref '$REF' is not a full 40-hex commit SHA — branch and tag refs are mutable and can skew between jobs mid-run" + fi + - name: Load cursor-review assets uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -876,6 +926,30 @@ jobs: permissions: contents: read steps: + - name: Require a pinned workflows_ref + # workflows_ref has no default on purpose. GitHub does NOT enforce + # `required: true` for workflow_call inputs, so an omitted input arrives + # as '' and actions/checkout would silently fall back to this repo's + # default branch — running MUTABLE scripts under a pin that claims + # otherwise. Fail fast instead. (BE-5546) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + # 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, and echo only that: dropping + # newlines also stops a multi-line value from smuggling a ::workflow + # command:: into the log, and from satisfying the line-oriented grep + # below on one 40-hex line among many. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::workflows_ref is required; pin it to the same commit SHA as the uses: line (see header example)" + exit 1 + fi + if ! printf '%s' "$REF" | grep -Eq '^[0-9a-f]{40}$'; then + echo "::warning::workflows_ref '$REF' is not a full 40-hex commit SHA — branch and tag refs are mutable and can skew between jobs mid-run" + fi + - name: Load cursor-review assets uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -930,6 +1004,30 @@ jobs: permissions: contents: read steps: + - name: Require a pinned workflows_ref + # workflows_ref has no default on purpose. GitHub does NOT enforce + # `required: true` for workflow_call inputs, so an omitted input arrives + # as '' and actions/checkout would silently fall back to this repo's + # default branch — running MUTABLE scripts under a pin that claims + # otherwise. Fail fast instead. (BE-5546) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + # 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, and echo only that: dropping + # newlines also stops a multi-line value from smuggling a ::workflow + # command:: into the log, and from satisfying the line-oriented grep + # below on one 40-hex line among many. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::workflows_ref is required; pin it to the same commit SHA as the uses: line (see header example)" + exit 1 + fi + if ! printf '%s' "$REF" | grep -Eq '^[0-9a-f]{40}$'; then + echo "::warning::workflows_ref '$REF' is not a full 40-hex commit SHA — branch and tag refs are mutable and can skew between jobs mid-run" + fi + - name: Load cursor-review assets uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: diff --git a/.github/workflows/groom.yml b/.github/workflows/groom.yml index a956994..82a9d45 100644 --- a/.github/workflows/groom.yml +++ b/.github/workflows/groom.yml @@ -93,7 +93,7 @@ name: Groom (reusable) # with: # # Post/act issues as cloud-code-bot instead of github-actions[bot]. # bot_app_id: ${{ vars.APP_ID }} -# workflows_ref: # pin assets to the same ref as `uses:` +# workflows_ref: # REQUIRED — same SHA as `uses:` above # dry_run: ${{ github.event.inputs.dry_run == 'true' }} # # Cadence knob + the matching volume-gate window (BE-4004). # interval_days: ${{ vars.GROOM_INTERVAL_DAYS || '7' }} @@ -244,11 +244,13 @@ on: default: claude-opus-5 workflows_ref: description: >- - Ref of Comfy-Org/github-workflows to load the groom briefs + ledger - from. Pin this to the same ref you pin `uses:` to for reproducibility. + REQUIRED. Ref of Comfy-Org/github-workflows to load the groom briefs + + ledger from. Must be the SAME commit SHA you pin `uses:` to — + otherwise the workflow is pinned but the code it runs is not. There is + deliberately no default: a floating `main` default would silently load + mutable scripts into jobs that hold write permissions. type: string - required: false - default: main + required: true config: description: >- OPTIONAL JSON object of operational knobs (BE-5227), as an ESCAPE HATCH @@ -406,6 +408,30 @@ jobs: # either gate runs, so it is populated even on a skipped tick. resolved: ${{ steps.resolve.outputs.resolved }} steps: + - name: Require a pinned workflows_ref + # workflows_ref has no default on purpose. GitHub does NOT enforce + # `required: true` for workflow_call inputs, so an omitted input arrives + # as '' and actions/checkout would silently fall back to this repo's + # default branch — running MUTABLE scripts under a pin that claims + # otherwise. Fail fast instead. (BE-5546) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + # 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, and echo only that: dropping + # newlines also stops a multi-line value from smuggling a ::workflow + # command:: into the log, and from satisfying the line-oriented grep + # below on one 40-hex line among many. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::workflows_ref is required; pin it to the same commit SHA as the uses: line (see header example)" + exit 1 + fi + if ! printf '%s' "$REF" | grep -Eq '^[0-9a-f]{40}$'; then + echo "::warning::workflows_ref '$REF' is not a full 40-hex commit SHA — branch and tag refs are mutable and can skew between jobs mid-run" + fi + - 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. @@ -628,6 +654,30 @@ jobs: persist-credentials: false path: repo + - name: Require a pinned workflows_ref + # workflows_ref has no default on purpose. GitHub does NOT enforce + # `required: true` for workflow_call inputs, so an omitted input arrives + # as '' and actions/checkout would silently fall back to this repo's + # default branch — running MUTABLE scripts under a pin that claims + # otherwise. Fail fast instead. (BE-5546) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + # 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, and echo only that: dropping + # newlines also stops a multi-line value from smuggling a ::workflow + # command:: into the log, and from satisfying the line-oriented grep + # below on one 40-hex line among many. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::workflows_ref is required; pin it to the same commit SHA as the uses: line (see header example)" + exit 1 + fi + if ! printf '%s' "$REF" | grep -Eq '^[0-9a-f]{40}$'; then + echo "::warning::workflows_ref '$REF' is not a full 40-hex commit SHA — branch and tag refs are mutable and can skew between jobs mid-run" + fi + - name: Load groom assets (briefs) # Trusted briefs come from THIS workflow's repo (public, pinned via # workflows_ref) — never from the target checkout, so a malicious repo @@ -961,6 +1011,30 @@ jobs: persist-credentials: false path: repo + - name: Require a pinned workflows_ref + # workflows_ref has no default on purpose. GitHub does NOT enforce + # `required: true` for workflow_call inputs, so an omitted input arrives + # as '' and actions/checkout would silently fall back to this repo's + # default branch — running MUTABLE scripts under a pin that claims + # otherwise. Fail fast instead. (BE-5546) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + # 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, and echo only that: dropping + # newlines also stops a multi-line value from smuggling a ::workflow + # command:: into the log, and from satisfying the line-oriented grep + # below on one 40-hex line among many. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::workflows_ref is required; pin it to the same commit SHA as the uses: line (see header example)" + exit 1 + fi + if ! printf '%s' "$REF" | grep -Eq '^[0-9a-f]{40}$'; then + echo "::warning::workflows_ref '$REF' is not a full 40-hex commit SHA — branch and tag refs are mutable and can skew between jobs mid-run" + fi + - 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. @@ -1258,6 +1332,30 @@ jobs: permission-issues: read permission-pull-requests: read + - name: Require a pinned workflows_ref + # workflows_ref has no default on purpose. GitHub does NOT enforce + # `required: true` for workflow_call inputs, so an omitted input arrives + # as '' and actions/checkout would silently fall back to this repo's + # default branch — running MUTABLE scripts under a pin that claims + # otherwise. Fail fast instead. (BE-5546) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + # 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, and echo only that: dropping + # newlines also stops a multi-line value from smuggling a ::workflow + # command:: into the log, and from satisfying the line-oriented grep + # below on one 40-hex line among many. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::workflows_ref is required; pin it to the same commit SHA as the uses: line (see header example)" + exit 1 + fi + if ! printf '%s' "$REF" | grep -Eq '^[0-9a-f]{40}$'; then + echo "::warning::workflows_ref '$REF' is not a full 40-hex commit SHA — branch and tag refs are mutable and can skew between jobs mid-run" + fi + - name: Load groom assets (ledger) uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -1403,6 +1501,30 @@ jobs: # Least privilege: this job only opens `groom` issues (+ their labels). permission-issues: write + - name: Require a pinned workflows_ref + # workflows_ref has no default on purpose. GitHub does NOT enforce + # `required: true` for workflow_call inputs, so an omitted input arrives + # as '' and actions/checkout would silently fall back to this repo's + # default branch — running MUTABLE scripts under a pin that claims + # otherwise. Fail fast instead. (BE-5546) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + # 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, and echo only that: dropping + # newlines also stops a multi-line value from smuggling a ::workflow + # command:: into the log, and from satisfying the line-oriented grep + # below on one 40-hex line among many. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::workflows_ref is required; pin it to the same commit SHA as the uses: line (see header example)" + exit 1 + fi + if ! printf '%s' "$REF" | grep -Eq '^[0-9a-f]{40}$'; then + echo "::warning::workflows_ref '$REF' is not a full 40-hex commit SHA — branch and tag refs are mutable and can skew between jobs mid-run" + fi + - name: Load groom assets (ledger) uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -1585,6 +1707,30 @@ jobs: persist-credentials: false path: repo + - name: Require a pinned workflows_ref + # workflows_ref has no default on purpose. GitHub does NOT enforce + # `required: true` for workflow_call inputs, so an omitted input arrives + # as '' and actions/checkout would silently fall back to this repo's + # default branch — running MUTABLE scripts under a pin that claims + # otherwise. Fail fast instead. (BE-5546) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + # 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, and echo only that: dropping + # newlines also stops a multi-line value from smuggling a ::workflow + # command:: into the log, and from satisfying the line-oriented grep + # below on one 40-hex line among many. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::workflows_ref is required; pin it to the same commit SHA as the uses: line (see header example)" + exit 1 + fi + if ! printf '%s' "$REF" | grep -Eq '^[0-9a-f]{40}$'; then + echo "::warning::workflows_ref '$REF' is not a full 40-hex commit SHA — branch and tag refs are mutable and can skew between jobs mid-run" + fi + - name: Load groom assets (builder brief) uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -1970,6 +2116,30 @@ jobs: permission-pull-requests: write permission-issues: write + - name: Require a pinned workflows_ref + # workflows_ref has no default on purpose. GitHub does NOT enforce + # `required: true` for workflow_call inputs, so an omitted input arrives + # as '' and actions/checkout would silently fall back to this repo's + # default branch — running MUTABLE scripts under a pin that claims + # otherwise. Fail fast instead. (BE-5546) + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + # 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, and echo only that: dropping + # newlines also stops a multi-line value from smuggling a ::workflow + # command:: into the log, and from satisfying the line-oriented grep + # below on one 40-hex line among many. + REF="$(printf '%s' "$WORKFLOWS_REF" | tr -d '[:space:]')" + if [ -z "$REF" ]; then + echo "::error::workflows_ref is required; pin it to the same commit SHA as the uses: line (see header example)" + exit 1 + fi + if ! printf '%s' "$REF" | grep -Eq '^[0-9a-f]{40}$'; then + echo "::warning::workflows_ref '$REF' is not a full 40-hex commit SHA — branch and tag refs are mutable and can skew between jobs mid-run" + fi + - name: Load groom assets (signature marker) uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: diff --git a/.github/workflows/test-workflow-pins.yml b/.github/workflows/test-workflow-pins.yml new file mode 100644 index 0000000..cd12d90 --- /dev/null +++ b/.github/workflows/test-workflow-pins.yml @@ -0,0 +1,108 @@ +name: Test workflow pins + +# Regression lint (BE-5546): no reusable workflow here may give its +# `workflows_ref` input a `default:`. A default lets a caller SHA-pin `uses:` +# and still load this repo's scripts from a MUTABLE branch, into jobs holding +# write permissions — the pin then proves nothing. Removing that default is a +# one-line edit to undo, so it is guarded by CI rather than by convention. +# +# Runs on every workflow-file change (not just the three fixed ones) so a +# workflow added later is covered the day it lands. + +on: + pull_request: + paths: + - '.github/workflows/**' + - '.github/workflow-pins/**' + push: + branches: [main] + paths: + - '.github/workflows/**' + - '.github/workflow-pins/**' + +permissions: + contents: read + +# One lint run per ref: a push that supersedes an in-flight run should cancel +# it, not race it for a runner. +concurrency: + group: test-workflow-pins-${{ github.ref }} + cancel-in-progress: true + +jobs: + test: + name: unittest + lint this repo + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Set up Python + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: '3.12' + + - name: Run workflow-pins unit tests + run: python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' -v + + - name: Lint this repo's workflows + run: python3 .github/workflow-pins/check_workflow_pins.py + + - name: Smoke test — a reintroduced default fails the lint + run: | + # Proves the lint is wired to FAIL, not just to run: the unit tests + # exercise the parser, this exercises the CLI's exit status. + dir="$(mktemp -d)" + cat > "$dir/regressed.yml" <<'YAML' + name: Regressed + on: + workflow_call: + inputs: + workflows_ref: + type: string + required: false + default: main + YAML + if python3 .github/workflow-pins/check_workflow_pins.py --workflows-dir "$dir"; then + echo "::error::lint passed a workflow with a workflows_ref default — expected a non-zero exit" + exit 1 + fi + echo "lint correctly rejected the reintroduced default" + + - name: Smoke test — an unguarded ref checkout fails the lint + run: | + # The other half of the hole: no default to find, but a job that + # checks out at the ref without the fail-fast guard still lets an + # omitted input reach `ref: ''` and take the default branch. + dir="$(mktemp -d)" + # The ref expression goes in via a placeholder, for two reasons: a + # literal Actions expression written here would be evaluated when this + # run block is rendered (and `inputs` is empty in this workflow), so + # the fixture would silently lose the very line under test; and the + # lint's own "this file uses the input, so it must declare it" check + # would then read this workflow as a consumer it failed to parse. + D='$' + sed "s|__REF__|${D}{{ inputs.workflows_ref }}|" > "$dir/unguarded.yml" <<'YAML' + name: Unguarded + on: + workflow_call: + inputs: + workflows_ref: + type: string + required: true + jobs: + run: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@abc + with: + ref: __REF__ + YAML + grep -q 'inputs.workflows_ref' "$dir/unguarded.yml" # fixture intact + if python3 .github/workflow-pins/check_workflow_pins.py --workflows-dir "$dir"; then + echo "::error::lint passed an unguarded workflows_ref checkout — expected a non-zero exit" + exit 1 + fi + echo "lint correctly rejected the unguarded checkout" diff --git a/AGENTS.md b/AGENTS.md index 3023441..2e69cf3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -27,7 +27,10 @@ python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh bash .github/bump-callers/tests/test_bump_callers.sh -# run the AGENTS.md integrity checker against any repo tree +# workflow-pins lint (no reusable workflow may default `workflows_ref`) + its tests +python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' && python3 .github/workflow-pins/check_workflow_pins.py + +# AGENTS.md integrity checker against any repo tree python3 .github/agents-md-integrity/check_agents_md.py --root . ``` @@ -56,6 +59,7 @@ tests — run the matching command above for whatever you touched. gate (`GROOM_INTERVAL_DAYS`) that early-exits a daily tick unless the interval has elapsed since the last real run (derived from Actions run history — no new secret). Tests in `tests/`. +- `.github/workflow-pins/` — `check_workflow_pins.py` + `tests/`: the lint forbidding a `default:` on a `workflow_call` workflow's `workflows_ref`, and requiring the empty-ref guard in every job that checks out at it. - `.github/bump-callers/` — `bump-callers.sh`, the ONE fleet-agnostic script that opens SHA-bump PRs in consumer repos when a reusable workflow changes. Tests in `tests/`. @@ -101,6 +105,10 @@ tests — run the matching command above for whatever you touched. - **Pin everything by full commit SHA**, with a trailing `# v1` comment — both the `uses:` in callers and every third-party action here. Bare `@v1` fails the pin-validation (`pinact`, `zizmor`) that consumer CI runs. See README "Usage". +- **`workflows_ref` is REQUIRED, never given a `default:`** (BE-5546) — a default + 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. - **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/README.md b/README.md index 17d83d0..4d130f9 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,8 @@ The SHA-pin format satisfies pin-validation tooling (`pinact`, `zizmor`, etc.) a A bare `@v1` tag is technically allowed but **will fail** in repos that run pin-validation in CI (e.g. `cloud`, `ComfyUI_frontend`). +Workflows that load their backing scripts at run time take a `workflows_ref` input — always set it to the *same* commit SHA you pin `uses:` to. Pinning only `uses:` runs a pinned workflow that loads **mutable** scripts from a floating branch, which defeats the pin. On `cursor-review.yml`, `groom.yml`, and `agents-md-integrity.yml` the input is **required with no default** and the run fails fast when it is empty or omitted (GitHub does not enforce `required: true` for `workflow_call` inputs, so those workflows check at run time). + Per-workflow inputs, required secrets, and triggers are documented in each workflow file's header comment. ## Versioning