Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions .github/workflow-pins/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,12 @@ 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
tracked under their own ticket. It is **empty today** — `pr-size.yml`, its last
entry, was fixed in BE-5858 once its caller fleet had been audited, so no
reusable workflow here is carved out of either check. 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.
9 changes: 3 additions & 6 deletions .github/workflow-pins/check_workflow_pins.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,9 @@
# 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"})
# Empty as of BE-5858: the list drained itself exactly as designed, and every
# reusable workflow here is now held to both checks with no carve-out.
KNOWN_EXEMPT = frozenset()

_ON_RE = re.compile(r"""^(['"]?)on\1\s*:(.*)$""")
_JOBS_RE = re.compile(r"""^(['"]?)jobs\1\s*:""")
Expand Down
8 changes: 5 additions & 3 deletions .github/workflow-pins/tests/test_check_workflow_pins.py
Original file line number Diff line number Diff line change
Expand Up @@ -555,14 +555,16 @@ def test_this_repos_own_workflows_guard_every_ref_checkout(self):
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"):
for name in ("cursor-review.yml", "groom.yml", "agents-md-integrity.yml", "pr-size.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")
self.assertEqual(
seen, 13, "expected the 12 guarded sites BE-5546 fixed + pr-size.yml's (BE-5858)"
)


class CheckDirTests(unittest.TestCase):
Expand Down Expand Up @@ -793,7 +795,7 @@ def test_this_repos_own_workflows_pass(self):
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"):
for name in ("cursor-review.yml", "groom.yml", "agents-md-integrity.yml", "pr-size.yml"):
self.assertIn(name, checked)


Expand Down
41 changes: 33 additions & 8 deletions .github/workflows/pr-size.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ name: PR Size Cap (reusable)
# uses: Comfy-Org/github-workflows/.github/workflows/pr-size.yml@<sha> # v1
# with:
# max_lines: 1000
# # Pin the tool ref to the same ref you pin `uses:` to for
# # reproducibility (defaults to main).
# workflows_ref: <sha>
# # REQUIRED. Pin the tool ref to the SAME commit SHA you pin `uses:`
# # to — otherwise the workflow is pinned but its tool is not.
# workflows_ref: <same-sha-as-uses>
Comment thread
mattmillerai marked this conversation as resolved.
# # Optional: post the sticky comment under your GitHub App.
# bot_app_id: ${{ vars.APP_ID }}
# secrets:
Expand Down Expand Up @@ -122,12 +122,13 @@ on:
default: ''
workflows_ref:
description: >-
Ref of Comfy-Org/github-workflows to load the check-pr-size tool
from. Pin this to the same ref you pin `uses:` to for
reproducibility.
REQUIRED. Ref of Comfy-Org/github-workflows to load the check-pr-size
tool 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 tool.
type: string
required: false
default: main
required: true
Comment thread
mattmillerai marked this conversation as resolved.
secrets:
BOT_APP_PRIVATE_KEY:
description: >-
Expand Down Expand Up @@ -162,6 +163,30 @@ jobs:
persist-credentials: false
ref: ${{ github.event.pull_request.head.sha }}

- 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:]')"
Comment thread
mattmillerai marked this conversation as resolved.
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
Comment thread
mattmillerai marked this conversation as resolved.
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 check-pr-size tool
# The tool comes from THIS workflow's repo (public, pinned via
# workflows_ref) — never from the PR checkout, so no PR-authored code
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ 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).
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 every workflow here that takes it — `cursor-review.yml`, `groom.yml`, `agents-md-integrity.yml`, and `pr-size.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.

Expand Down
Loading