From f0c7f73390b9648d59dd0875868aad042c93c5a1 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sat, 19 Sep 2026 08:03:03 +0000 Subject: [PATCH] feat(cursor-review): fail the review when workflows_ref differs from the uses: pin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each of the eight `Require a pinned workflows_ref` steps proved the ref was non-empty and warned when it was not 40-hex, but nothing ever compared it to the commit the caller's `uses:` line actually resolved to. A caller could pin `uses:` to one commit and load this workflow's prompts, scripts and model list from another — the review then runs one version's workflow against a different version's judging logic, with both pins looking plausible in the caller. Port the equality guard public-repo-hygiene.yml already runs: bind `job.workflow_sha` (computed by the runner from the `uses:` pin, so a caller cannot set it) and fail on a mismatch. All eight copies stay byte-identical; a new parity suite pins that, asserts the count, and pins the ledger's exemption from the other side. Two deliberate differences from the hygiene guard, both documented inline: * an empty `job.workflow_sha` (Actions runner older than v2.334.0) WARNS rather than failing. Hygiene fails closed there because it is the checker whose own integrity rests on the equality; here a property nothing could evaluate should not take the whole review down. * both sides are lower-cased before the comparison. Hygiene normalises only `$SHA` because its 40-hex test hard-fails an upper-case ref several lines earlier; the same test here only warns, so an upper-case ref naming the RIGHT commit reaches the comparison and must not fail. The exempt `Prior-review ledger` job — which must never fail, since the review matrix `needs:` it — gets the same cross-check as a `::warning::` with no exit, so a caller whose guarded jobs are all skipped still has the split recorded somewhere. --- .github/cursor-review/README.md | 8 + .../tests/test_workflow_ref_guard_parity.py | 229 +++++++++++++++ .github/workflows/cursor-review.yml | 278 ++++++++++++++++++ docs/callers/cursor-review.md | 2 +- 4 files changed, 516 insertions(+), 1 deletion(-) create mode 100644 .github/cursor-review/tests/test_workflow_ref_guard_parity.py diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index d30461d..6570f8b 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -279,6 +279,14 @@ All optional except `workflows_ref` (required, no default) — pass them under | `run_without_label` | `false` | Run on plain PR events instead of requiring the trigger label. Also requires widening the caller's `types:` — see [the setup guide](../../docs/callers/cursor-review.md). | | `blocking` | `false` | Adds the fail-closed **Blocking gate** check: red while any cursor-review finding thread is unresolved and non-outdated, and red when the round that should have produced those threads did not land (including an over-cap skip). Blocking the merge additionally requires marking that check required in the caller's ruleset — see [the blocking section above](#optional-make-the-review-blocking). | +### `workflows_ref` must equal the `uses:` pin + +The two pins are checked against each other at run time, not just documented. Every job that checks this directory out runs a `Require a pinned workflows_ref` step that rejects an empty value and then compares the ref to `job.workflow_sha` — the commit the caller's `uses:` line resolved to. That value is computed by the runner from the `uses:` line, so a caller cannot set it, which is what makes "the prompts come from the commit you pinned" a checked fact rather than a convention. A mismatch fails the job: otherwise a caller pinned to one commit would have its PR judged by another commit's prompts, scripts and model list. + +Two deliberate holes. A runner older than Actions v2.334.0 supplies no `job.workflow_sha`, so the comparison cannot be evaluated at all — that case **warns and continues** rather than failing, since a property that could not be measured is not evidence of a violation. (Every job here is `runs-on: ubuntu-latest`, which is always current, so only a runner group shadowing that label reaches it.) And the `Prior-review ledger` job is exempt from the guard entirely — it must never fail the run, because the review matrix `needs:` it — so it downgrades the same mismatch to a `::warning::` in its own log. That warning is the one signal a caller gets when the label gate skipped every guarded job. + +**Fixing a red run:** set `uses:` and `workflows_ref` to one and the same 40-hex SHA. They drift when only one of them gets bumped — the `bump-callers` fleet rewrites both together, but Dependabot rewrites only `uses:`, so a caller that lets Dependabot bump this reusable needs an `ignore` entry for `Comfy-Org/github-workflows` in its `dependabot.yml`. + ### Over the diff-size cap A PR whose counted diff exceeds `diff_size_cap` gets **no review panel**, and diff --git a/.github/cursor-review/tests/test_workflow_ref_guard_parity.py b/.github/cursor-review/tests/test_workflow_ref_guard_parity.py new file mode 100644 index 0000000..4bff4d2 --- /dev/null +++ b/.github/cursor-review/tests/test_workflow_ref_guard_parity.py @@ -0,0 +1,229 @@ +#!/usr/bin/env python3 +"""Structural regression tests for cursor-review.yml's `workflows_ref` guard. + +Every job that checks this directory out runs a `Require a pinned workflows_ref` +step, and those steps are COPY-PASTED on purpose: `workflow_call` has no +anchors, no `uses: ./…` for a step, and no job-level `run:`, so eight literal +copies is the only spelling available. Copies drift — one job gets a hardening +the other seven miss, and the hole is invisible in a diff because the file still +contains the guard. So the copies are pinned byte-for-byte here instead. + +What this suite asserts: + + * exactly EIGHT guard steps. The count is asserted, not derived, so adding a + ninth guarded job is an explicit opt-in that makes an author read this file; + * all eight `env:` + `run:` blocks are byte-identical to each other; + * the block carries the BE-15927 equality guard — the `job.workflow_sha` + binding, the mismatch comparison, and a hard `exit` in that branch. Empty + and 40-hex are covered by `check_workflow_pins.py`; equality is not, because + that lint only judges whether a ref is non-empty and immutable; and + * the exempt `Prior-review ledger` resolve step ALSO reads `job.workflow_sha` + and does NOT exit on a mismatch. That job must never fail (the review matrix + `needs:` it), so an `exit` creeping in there would take every review down. + +Deliberately parsed WITHOUT PyYAML, like test_workflow_job_isolation.py next +door: this repo is stdlib-only and CI installs no requirements for this suite, +so a yaml import would simply not run. A round-trip would also destroy exactly +what is under test — this is a test about the literal bytes of eight blocks. + +Run: python3 .github/cursor-review/tests/test_workflow_ref_guard_parity.py +""" + +import os +import re +import unittest + +WORKFLOW = os.path.normpath( + os.path.join( + os.path.dirname(__file__), "..", "..", "workflows", "cursor-review.yml" + ) +) + +# The guard steps. `.strip()`-compared so the assertion is about the step name, +# not about the indentation of whichever job happens to hold it. +GUARD_STEP_NAME = "- name: Require a pinned workflows_ref" +EXPECTED_GUARDS = 8 + +# The ledger's exempt resolver, addressed by its step id rather than its name so +# a comment rewrite above it cannot move the anchor. +LEDGER_STEP_ID = "id: resolve_ref" + +WORKFLOW_SHA_BINDING = "WORKFLOW_SHA: ${{ job.workflow_sha }}" +EXIT_RE = re.compile(r"^\s*exit\s+[1-9]", re.MULTILINE) + + +def read_lines(): + with open(WORKFLOW, encoding="utf-8") as fh: + return fh.read().split("\n") + + +def step_block(lines, start): + """The lines of the step opening at `lines[start]`, up to the next sibling. + + A step ends at the first non-blank line indented no deeper than its own `-`. + Blank lines inside a step (this workflow separates comment paragraphs with + them) are only a terminator when what follows them dedents, so they are + looked past rather than treated as the end. + """ + indent = len(lines[start]) - len(lines[start].lstrip()) + i = start + 1 + while i < len(lines): + if not lines[i].strip(): + j = i + while j < len(lines) and not lines[j].strip(): + j += 1 + if j >= len(lines) or (len(lines[j]) - len(lines[j].lstrip())) <= indent: + break + i = j + continue + if (len(lines[i]) - len(lines[i].lstrip())) <= indent: + break + i += 1 + return lines[start:i] + + +def guard_blocks(): + """The `env:` + `run:` body of every guard step, one string each. + + The step's leading comment block is dropped on purpose: parity is about the + BEHAVIOUR being identical. Keeping the prose in would make a per-job comment + (a legitimate thing to want) fail a test that exists to catch a per-job + SCRIPT. + """ + lines = read_lines() + starts = [i for i, ln in enumerate(lines) if ln.strip() == GUARD_STEP_NAME] + blocks = [] + for start in starts: + block = step_block(lines, start) + env_at = next( + (i for i, ln in enumerate(block) if ln.strip() == "env:"), + None, + ) + assert env_at is not None, "guard step at line %d has no env: block" % ( + start + 1, + ) + blocks.append("\n".join(ln.strip() for ln in block[env_at:])) + return starts, blocks + + +def ledger_resolve_block(): + lines = read_lines() + hit = [i for i, ln in enumerate(lines) if ln.strip() == LEDGER_STEP_ID] + assert len(hit) == 1, "expected exactly one `%s` step" % LEDGER_STEP_ID + # Walk back to the `- name:` line that opens the step holding that id. + start = hit[0] + while start >= 0 and not lines[start].lstrip().startswith("- name:"): + start -= 1 + assert start >= 0, "`%s` is not inside a named step" % LEDGER_STEP_ID + return "\n".join(step_block(lines, start)) + + +class GuardParityTest(unittest.TestCase): + def test_exactly_eight_guard_steps(self): + starts, _ = guard_blocks() + self.assertEqual( + len(starts), + EXPECTED_GUARDS, + "cursor-review.yml has %d `%s` steps, expected %d. A new guarded job " + "is welcome — bump EXPECTED_GUARDS here and confirm its guard is a " + "byte-for-byte copy of the others. A REMOVED one is the case this " + "count exists to catch." % (len(starts), GUARD_STEP_NAME, EXPECTED_GUARDS), + ) + + def test_all_guard_blocks_are_byte_identical(self): + starts, blocks = guard_blocks() + for line_no, block in zip(starts[1:], blocks[1:]): + self.assertEqual( + block, + blocks[0], + "the guard step at line %d differs from the one at line %d. The " + "eight copies are load-bearing: a hardening applied to one job " + "and not the rest leaves the others silently weaker." + % (line_no + 1, starts[0] + 1), + ) + + def test_guard_binds_job_workflow_sha_through_env(self): + _, blocks = guard_blocks() + self.assertIn(WORKFLOW_SHA_BINDING, blocks[0]) + # Through `env:`, never interpolated into the script body — the same + # rule `check_workflow_pins.py` enforces for WORKFLOWS_REF, and for the + # same reason: an interpolated value is substituted into the shell + # source before it runs. + self.assertNotIn("${{ job.workflow_sha }}", blocks[0].split("run: |")[1]) + + def test_guard_binds_the_bare_input_not_the_fallback(self): + # `check_workflow_pins.py` reads the STRENGTH of a guard off this + # binding: `inputs.workflows_ref || job.workflow_sha` proves only that + # the OR expression is non-empty, so a guard written that way would stop + # covering the sibling `ref: ${{ inputs.workflows_ref }}` checkouts in + # the same job. These jobs are not exempt; keep the binding bare. + _, blocks = guard_blocks() + self.assertIn("WORKFLOWS_REF: ${{ inputs.workflows_ref }}", blocks[0]) + self.assertNotIn("inputs.workflows_ref || job.workflow_sha", blocks[0]) + + def test_guard_fails_on_a_mismatch(self): + _, blocks = guard_blocks() + body = blocks[0] + self.assertIn('elif [ "$REF_LC" != "$SHA" ]; then', body) + tail = body.split('elif [ "$REF_LC" != "$SHA" ]; then', 1)[1] + # The exit must sit in THAT branch — before the `fi` that closes it. + branch = tail.split("\nfi", 1)[0] + self.assertRegex( + branch, + EXIT_RE, + "the mismatch branch does not exit non-zero, so a split pin would " + "only annotate the run and the review would proceed anyway", + ) + + def test_guard_only_warns_when_job_workflow_sha_is_unavailable(self): + # A runner older than v2.334.0 supplies no `job.workflow_sha`. That is + # "could not evaluate", not "mismatch" — failing there would take the + # review down over a property nothing measured. (public-repo-hygiene.yml + # fails closed in the same spot on purpose; see the comment there.) + _, blocks = guard_blocks() + empty_branch = blocks[0].split('if [ -z "$SHA" ]; then', 1)[1].split( + "\nelif", 1 + )[0] + self.assertIn("::warning::", empty_branch) + self.assertNotRegex(empty_branch, EXIT_RE) + + def test_guard_compares_case_insensitively(self): + # Hex is case-insensitive and the 40-hex shape test here only WARNS, so + # an upper-case-but-CORRECT ref reaches the comparison. Normalising only + # $SHA (which is what public-repo-hygiene.yml does, because its shape + # test hard-fails first) would fail all eight jobs of a caller whose two + # pins agree. + _, blocks = guard_blocks() + self.assertIn("REF_LC=\"$(printf '%s' \"$REF\" | tr 'A-Z' 'a-z')\"", blocks[0]) + + +class LedgerExemptionTest(unittest.TestCase): + def test_ledger_resolve_reads_job_workflow_sha(self): + self.assertIn(WORKFLOW_SHA_BINDING, ledger_resolve_block()) + + def test_ledger_warns_on_a_mismatch(self): + block = ledger_resolve_block() + self.assertIn('[ "$REF_LC" != "$SHA" ]', block) + mismatch = block.split('[ "$REF_LC" != "$SHA" ]', 1)[1] + self.assertIn("::warning::", mismatch.split("\n fi", 1)[0]) + + def test_ledger_never_exits(self): + # The review matrix `needs:` this job, so ANY non-zero exit here skips + # every review job — the exact failure the exemption exists to prevent. + # `continue-on-error: true` on the step covers a crash, not a deliberate + # exit that a later refactor might move out from under it. + self.assertNotRegex(ledger_resolve_block(), EXIT_RE) + + def test_ledger_keeps_its_fallback_binding(self): + # The ledger's WORKFLOWS_REF is the one place the `|| job.workflow_sha` + # fallback is sanctioned (it is why this job is exempt from the guard). + # Losing it would make an omitted input resolve to '' and skip the asset + # checkout on every run. + self.assertIn( + "WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }}", + ledger_resolve_block(), + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 56af0f1..1f3ba80 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -85,6 +85,8 @@ name: Cursor Review (reusable) # **/*.snap # # REQUIRED. Pin the assets ref to the SAME commit SHA you pin `uses:` # # to — otherwise the workflow is pinned but its scripts are not. +# # The workflow checks this against the uses: pin at run time and fails +# # on a mismatch. # 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 @@ -624,6 +626,11 @@ jobs: timeout-minutes: 2 env: WORKFLOWS_REF: ${{ inputs.workflows_ref || job.workflow_sha }} + # The `uses:` commit on its own, WITHOUT the fallback above, so the + # cross-check at the end of the script has something to compare + # against. Bound here rather than interpolated into the script body, + # like every other value this workflow reads. + WORKFLOW_SHA: ${{ job.workflow_sha }} run: | # Bracket RANGES (`A-Z`) match by COLLATION order, so under a glibc # locale such as en_US.UTF-8 they also match accented letters and the @@ -649,6 +656,29 @@ jobs: if [ -z "$REF" ]; then echo "::warning::Could not resolve a usable ref for the cursor-review assets: either the workflows_ref input is empty AND job.workflow_sha resolved to '' (which needs an Actions runner older than v2.334.0), or the value contains characters outside the set this workflow accepts (A-Z a-z 0-9 . _ / - @ +) — a deliberate policy restriction, narrower than git's own ref rules. Skipping the asset checkout rather than silently loading them from this repo's default branch — the prior-review ledger degrades to status=unknown for this run. Set workflows_ref: to the same commit SHA the caller's uses: line pins." fi + # Warn-only cross-check — the counterpart of the fail-closed equality + # test the guarded jobs run. This job must never fail (see the job + # comment), so a mismatch cannot `exit` here. It is reported anyway + # because the ledger is where the signal would otherwise be invisible: + # this job runs for every caller, including one whose guarded jobs all + # skip, so its run log can be the only place a split pin is recorded. + # + # $REF is the right left-hand side even though it carries the + # `|| job.workflow_sha` fallback: when the input was omitted it IS + # WORKFLOW_SHA, so it cannot differ, and a difference therefore means + # the caller passed a workflows_ref naming a different commit than its + # `uses:` line. An unusable input has already been rewritten to '' and + # warned about above, so skip it here rather than warn twice; an empty + # $SHA means a runner older than v2.334.0, which cannot be checked at + # all. Lower-cased on both sides, like the guarded jobs' copy of this + # test: hex is case-insensitive, so an upper-case pin naming the same + # commit is not a difference. $REF itself stays untouched — it is the + # value the checkout below consumes. (BE-15927) + REF_LC="$(printf '%s' "$REF" | tr 'A-Z' 'a-z')" + SHA="$(printf '%s' "$WORKFLOW_SHA" | tr -d '[:space:]' | tr 'A-Z' 'a-z')" + if [ -n "$REF_LC" ] && [ -n "$SHA" ] && [ "$REF_LC" != "$SHA" ]; then + echo "::warning::workflows_ref ($REF) differs from job.workflow_sha ($SHA); the prior-review ledger is being read with assets from a different commit than the workflow. Set the uses: pin and workflows_ref to the same commit." + fi echo "ref=$REF" >> "$GITHUB_OUTPUT" - name: Load cursor-review assets @@ -773,6 +803,7 @@ jobs: # otherwise. Fail fast instead. (BE-5546) env: WORKFLOWS_REF: ${{ inputs.workflows_ref }} + WORKFLOW_SHA: ${{ job.workflow_sha }} 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 @@ -788,6 +819,36 @@ jobs: 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 + # Equality with the commit the caller's `uses:` line resolved to — the + # same guard public-repo-hygiene.yml runs, and the check that turns + # "workflows_ref must be the same SHA as uses:" from a header comment + # into a fact. `job.workflow_sha` is computed by the RUNNER from the + # `uses:` pin, so a caller cannot set it. Without this, a caller can + # pin `uses:` to one commit and load this workflow's prompts, scripts + # and model list from another. (`job.workflow_sha`, NOT + # `github.job_workflow_sha` — that one is an OIDC token claim, absent + # from the `github` context, and expands to ''.) Both sides are + # lower-cased: hex is case-insensitive, and unlike hygiene's the + # 40-hex test above only WARNS, so an upper-case ref naming the RIGHT + # commit reaches here and must not fail. $REF itself is left alone so + # the messages quote what the caller wrote. (BE-15927) + REF_LC="$(printf '%s' "$REF" | tr 'A-Z' 'a-z')" + SHA="$(printf '%s' "$WORKFLOW_SHA" | tr -d '[:space:]' | tr 'A-Z' 'a-z')" + if [ -z "$SHA" ]; then + # Cannot check, NOT a mismatch. `job.workflow_sha` is the `job`-context + # accessor added in Actions runner v2.334.0, so only an older runner + # gets here — every job here is `runs-on: ubuntu-latest`, so that + # means a runner group shadowing the label. Warn and continue: a + # property nothing could EVALUATE is not evidence of a violation, and + # failing would take the whole review down on a caller that changed + # nothing. (public-repo-hygiene.yml fails closed in this branch on + # purpose — it is the checker whose own integrity rests on the + # equality. Here the ledger's warn-and-degrade posture is the fit.) + echo "::warning::job.workflow_sha is empty (Actions runner older than v2.334.0?), so workflows_ref could not be cross-checked against the uses: pin. The empty and 40-hex tests above still apply." + elif [ "$REF_LC" != "$SHA" ]; then + echo "::error::workflows_ref ($REF) is not the commit this workflow was called at ($SHA). The uses: pin and workflows_ref MUST name the same commit — otherwise the workflow is pinned but its prompts and scripts are loaded from a different version. Set both to $SHA. (The bump-callers fleet rewrites both together; Dependabot rewrites only uses:, so a caller that lets Dependabot bump this reusable needs a dependabot.yml ignore entry for Comfy-Org/github-workflows.)" + exit 1 + fi - name: Load check-pr-size tool # The SAME classifier the PR-size cap uses — the single source of truth @@ -1341,6 +1402,7 @@ jobs: # otherwise. Fail fast instead. (BE-5546) env: WORKFLOWS_REF: ${{ inputs.workflows_ref }} + WORKFLOW_SHA: ${{ job.workflow_sha }} 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 @@ -1356,6 +1418,36 @@ jobs: 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 + # Equality with the commit the caller's `uses:` line resolved to — the + # same guard public-repo-hygiene.yml runs, and the check that turns + # "workflows_ref must be the same SHA as uses:" from a header comment + # into a fact. `job.workflow_sha` is computed by the RUNNER from the + # `uses:` pin, so a caller cannot set it. Without this, a caller can + # pin `uses:` to one commit and load this workflow's prompts, scripts + # and model list from another. (`job.workflow_sha`, NOT + # `github.job_workflow_sha` — that one is an OIDC token claim, absent + # from the `github` context, and expands to ''.) Both sides are + # lower-cased: hex is case-insensitive, and unlike hygiene's the + # 40-hex test above only WARNS, so an upper-case ref naming the RIGHT + # commit reaches here and must not fail. $REF itself is left alone so + # the messages quote what the caller wrote. (BE-15927) + REF_LC="$(printf '%s' "$REF" | tr 'A-Z' 'a-z')" + SHA="$(printf '%s' "$WORKFLOW_SHA" | tr -d '[:space:]' | tr 'A-Z' 'a-z')" + if [ -z "$SHA" ]; then + # Cannot check, NOT a mismatch. `job.workflow_sha` is the `job`-context + # accessor added in Actions runner v2.334.0, so only an older runner + # gets here — every job here is `runs-on: ubuntu-latest`, so that + # means a runner group shadowing the label. Warn and continue: a + # property nothing could EVALUATE is not evidence of a violation, and + # failing would take the whole review down on a caller that changed + # nothing. (public-repo-hygiene.yml fails closed in this branch on + # purpose — it is the checker whose own integrity rests on the + # equality. Here the ledger's warn-and-degrade posture is the fit.) + echo "::warning::job.workflow_sha is empty (Actions runner older than v2.334.0?), so workflows_ref could not be cross-checked against the uses: pin. The empty and 40-hex tests above still apply." + elif [ "$REF_LC" != "$SHA" ]; then + echo "::error::workflows_ref ($REF) is not the commit this workflow was called at ($SHA). The uses: pin and workflows_ref MUST name the same commit — otherwise the workflow is pinned but its prompts and scripts are loaded from a different version. Set both to $SHA. (The bump-callers fleet rewrites both together; Dependabot rewrites only uses:, so a caller that lets Dependabot bump this reusable needs a dependabot.yml ignore entry for Comfy-Org/github-workflows.)" + exit 1 + fi - name: Load cursor-review assets # Same trusted-assets checkout the review/consolidate jobs do — this @@ -1503,6 +1595,7 @@ jobs: # otherwise. Fail fast instead. (BE-5546) env: WORKFLOWS_REF: ${{ inputs.workflows_ref }} + WORKFLOW_SHA: ${{ job.workflow_sha }} 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 @@ -1518,6 +1611,36 @@ jobs: 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 + # Equality with the commit the caller's `uses:` line resolved to — the + # same guard public-repo-hygiene.yml runs, and the check that turns + # "workflows_ref must be the same SHA as uses:" from a header comment + # into a fact. `job.workflow_sha` is computed by the RUNNER from the + # `uses:` pin, so a caller cannot set it. Without this, a caller can + # pin `uses:` to one commit and load this workflow's prompts, scripts + # and model list from another. (`job.workflow_sha`, NOT + # `github.job_workflow_sha` — that one is an OIDC token claim, absent + # from the `github` context, and expands to ''.) Both sides are + # lower-cased: hex is case-insensitive, and unlike hygiene's the + # 40-hex test above only WARNS, so an upper-case ref naming the RIGHT + # commit reaches here and must not fail. $REF itself is left alone so + # the messages quote what the caller wrote. (BE-15927) + REF_LC="$(printf '%s' "$REF" | tr 'A-Z' 'a-z')" + SHA="$(printf '%s' "$WORKFLOW_SHA" | tr -d '[:space:]' | tr 'A-Z' 'a-z')" + if [ -z "$SHA" ]; then + # Cannot check, NOT a mismatch. `job.workflow_sha` is the `job`-context + # accessor added in Actions runner v2.334.0, so only an older runner + # gets here — every job here is `runs-on: ubuntu-latest`, so that + # means a runner group shadowing the label. Warn and continue: a + # property nothing could EVALUATE is not evidence of a violation, and + # failing would take the whole review down on a caller that changed + # nothing. (public-repo-hygiene.yml fails closed in this branch on + # purpose — it is the checker whose own integrity rests on the + # equality. Here the ledger's warn-and-degrade posture is the fit.) + echo "::warning::job.workflow_sha is empty (Actions runner older than v2.334.0?), so workflows_ref could not be cross-checked against the uses: pin. The empty and 40-hex tests above still apply." + elif [ "$REF_LC" != "$SHA" ]; then + echo "::error::workflows_ref ($REF) is not the commit this workflow was called at ($SHA). The uses: pin and workflows_ref MUST name the same commit — otherwise the workflow is pinned but its prompts and scripts are loaded from a different version. Set both to $SHA. (The bump-callers fleet rewrites both together; Dependabot rewrites only uses:, so a caller that lets Dependabot bump this reusable needs a dependabot.yml ignore entry for Comfy-Org/github-workflows.)" + exit 1 + fi - name: Load cursor-review assets # Trusted prompts/scripts come from THIS workflow's repo (public, @@ -1797,6 +1920,7 @@ jobs: # otherwise. Fail fast instead. (BE-5546) env: WORKFLOWS_REF: ${{ inputs.workflows_ref }} + WORKFLOW_SHA: ${{ job.workflow_sha }} 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 @@ -1812,6 +1936,36 @@ jobs: 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 + # Equality with the commit the caller's `uses:` line resolved to — the + # same guard public-repo-hygiene.yml runs, and the check that turns + # "workflows_ref must be the same SHA as uses:" from a header comment + # into a fact. `job.workflow_sha` is computed by the RUNNER from the + # `uses:` pin, so a caller cannot set it. Without this, a caller can + # pin `uses:` to one commit and load this workflow's prompts, scripts + # and model list from another. (`job.workflow_sha`, NOT + # `github.job_workflow_sha` — that one is an OIDC token claim, absent + # from the `github` context, and expands to ''.) Both sides are + # lower-cased: hex is case-insensitive, and unlike hygiene's the + # 40-hex test above only WARNS, so an upper-case ref naming the RIGHT + # commit reaches here and must not fail. $REF itself is left alone so + # the messages quote what the caller wrote. (BE-15927) + REF_LC="$(printf '%s' "$REF" | tr 'A-Z' 'a-z')" + SHA="$(printf '%s' "$WORKFLOW_SHA" | tr -d '[:space:]' | tr 'A-Z' 'a-z')" + if [ -z "$SHA" ]; then + # Cannot check, NOT a mismatch. `job.workflow_sha` is the `job`-context + # accessor added in Actions runner v2.334.0, so only an older runner + # gets here — every job here is `runs-on: ubuntu-latest`, so that + # means a runner group shadowing the label. Warn and continue: a + # property nothing could EVALUATE is not evidence of a violation, and + # failing would take the whole review down on a caller that changed + # nothing. (public-repo-hygiene.yml fails closed in this branch on + # purpose — it is the checker whose own integrity rests on the + # equality. Here the ledger's warn-and-degrade posture is the fit.) + echo "::warning::job.workflow_sha is empty (Actions runner older than v2.334.0?), so workflows_ref could not be cross-checked against the uses: pin. The empty and 40-hex tests above still apply." + elif [ "$REF_LC" != "$SHA" ]; then + echo "::error::workflows_ref ($REF) is not the commit this workflow was called at ($SHA). The uses: pin and workflows_ref MUST name the same commit — otherwise the workflow is pinned but its prompts and scripts are loaded from a different version. Set both to $SHA. (The bump-callers fleet rewrites both together; Dependabot rewrites only uses:, so a caller that lets Dependabot bump this reusable needs a dependabot.yml ignore entry for Comfy-Org/github-workflows.)" + exit 1 + fi - name: Load cursor-review assets uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -2293,6 +2447,7 @@ jobs: # otherwise. Fail fast instead. (BE-5546) env: WORKFLOWS_REF: ${{ inputs.workflows_ref }} + WORKFLOW_SHA: ${{ job.workflow_sha }} 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 @@ -2308,6 +2463,36 @@ jobs: 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 + # Equality with the commit the caller's `uses:` line resolved to — the + # same guard public-repo-hygiene.yml runs, and the check that turns + # "workflows_ref must be the same SHA as uses:" from a header comment + # into a fact. `job.workflow_sha` is computed by the RUNNER from the + # `uses:` pin, so a caller cannot set it. Without this, a caller can + # pin `uses:` to one commit and load this workflow's prompts, scripts + # and model list from another. (`job.workflow_sha`, NOT + # `github.job_workflow_sha` — that one is an OIDC token claim, absent + # from the `github` context, and expands to ''.) Both sides are + # lower-cased: hex is case-insensitive, and unlike hygiene's the + # 40-hex test above only WARNS, so an upper-case ref naming the RIGHT + # commit reaches here and must not fail. $REF itself is left alone so + # the messages quote what the caller wrote. (BE-15927) + REF_LC="$(printf '%s' "$REF" | tr 'A-Z' 'a-z')" + SHA="$(printf '%s' "$WORKFLOW_SHA" | tr -d '[:space:]' | tr 'A-Z' 'a-z')" + if [ -z "$SHA" ]; then + # Cannot check, NOT a mismatch. `job.workflow_sha` is the `job`-context + # accessor added in Actions runner v2.334.0, so only an older runner + # gets here — every job here is `runs-on: ubuntu-latest`, so that + # means a runner group shadowing the label. Warn and continue: a + # property nothing could EVALUATE is not evidence of a violation, and + # failing would take the whole review down on a caller that changed + # nothing. (public-repo-hygiene.yml fails closed in this branch on + # purpose — it is the checker whose own integrity rests on the + # equality. Here the ledger's warn-and-degrade posture is the fit.) + echo "::warning::job.workflow_sha is empty (Actions runner older than v2.334.0?), so workflows_ref could not be cross-checked against the uses: pin. The empty and 40-hex tests above still apply." + elif [ "$REF_LC" != "$SHA" ]; then + echo "::error::workflows_ref ($REF) is not the commit this workflow was called at ($SHA). The uses: pin and workflows_ref MUST name the same commit — otherwise the workflow is pinned but its prompts and scripts are loaded from a different version. Set both to $SHA. (The bump-callers fleet rewrites both together; Dependabot rewrites only uses:, so a caller that lets Dependabot bump this reusable needs a dependabot.yml ignore entry for Comfy-Org/github-workflows.)" + exit 1 + fi - name: Load cursor-review assets uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -2734,6 +2919,7 @@ jobs: # otherwise. Fail fast instead. (BE-5546) env: WORKFLOWS_REF: ${{ inputs.workflows_ref }} + WORKFLOW_SHA: ${{ job.workflow_sha }} 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 @@ -2749,6 +2935,36 @@ jobs: 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 + # Equality with the commit the caller's `uses:` line resolved to — the + # same guard public-repo-hygiene.yml runs, and the check that turns + # "workflows_ref must be the same SHA as uses:" from a header comment + # into a fact. `job.workflow_sha` is computed by the RUNNER from the + # `uses:` pin, so a caller cannot set it. Without this, a caller can + # pin `uses:` to one commit and load this workflow's prompts, scripts + # and model list from another. (`job.workflow_sha`, NOT + # `github.job_workflow_sha` — that one is an OIDC token claim, absent + # from the `github` context, and expands to ''.) Both sides are + # lower-cased: hex is case-insensitive, and unlike hygiene's the + # 40-hex test above only WARNS, so an upper-case ref naming the RIGHT + # commit reaches here and must not fail. $REF itself is left alone so + # the messages quote what the caller wrote. (BE-15927) + REF_LC="$(printf '%s' "$REF" | tr 'A-Z' 'a-z')" + SHA="$(printf '%s' "$WORKFLOW_SHA" | tr -d '[:space:]' | tr 'A-Z' 'a-z')" + if [ -z "$SHA" ]; then + # Cannot check, NOT a mismatch. `job.workflow_sha` is the `job`-context + # accessor added in Actions runner v2.334.0, so only an older runner + # gets here — every job here is `runs-on: ubuntu-latest`, so that + # means a runner group shadowing the label. Warn and continue: a + # property nothing could EVALUATE is not evidence of a violation, and + # failing would take the whole review down on a caller that changed + # nothing. (public-repo-hygiene.yml fails closed in this branch on + # purpose — it is the checker whose own integrity rests on the + # equality. Here the ledger's warn-and-degrade posture is the fit.) + echo "::warning::job.workflow_sha is empty (Actions runner older than v2.334.0?), so workflows_ref could not be cross-checked against the uses: pin. The empty and 40-hex tests above still apply." + elif [ "$REF_LC" != "$SHA" ]; then + echo "::error::workflows_ref ($REF) is not the commit this workflow was called at ($SHA). The uses: pin and workflows_ref MUST name the same commit — otherwise the workflow is pinned but its prompts and scripts are loaded from a different version. Set both to $SHA. (The bump-callers fleet rewrites both together; Dependabot rewrites only uses:, so a caller that lets Dependabot bump this reusable needs a dependabot.yml ignore entry for Comfy-Org/github-workflows.)" + exit 1 + fi - name: Load cursor-review assets uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -2784,6 +3000,7 @@ jobs: # otherwise. Fail fast instead. (BE-5546) env: WORKFLOWS_REF: ${{ inputs.workflows_ref }} + WORKFLOW_SHA: ${{ job.workflow_sha }} 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 @@ -2799,6 +3016,36 @@ jobs: 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 + # Equality with the commit the caller's `uses:` line resolved to — the + # same guard public-repo-hygiene.yml runs, and the check that turns + # "workflows_ref must be the same SHA as uses:" from a header comment + # into a fact. `job.workflow_sha` is computed by the RUNNER from the + # `uses:` pin, so a caller cannot set it. Without this, a caller can + # pin `uses:` to one commit and load this workflow's prompts, scripts + # and model list from another. (`job.workflow_sha`, NOT + # `github.job_workflow_sha` — that one is an OIDC token claim, absent + # from the `github` context, and expands to ''.) Both sides are + # lower-cased: hex is case-insensitive, and unlike hygiene's the + # 40-hex test above only WARNS, so an upper-case ref naming the RIGHT + # commit reaches here and must not fail. $REF itself is left alone so + # the messages quote what the caller wrote. (BE-15927) + REF_LC="$(printf '%s' "$REF" | tr 'A-Z' 'a-z')" + SHA="$(printf '%s' "$WORKFLOW_SHA" | tr -d '[:space:]' | tr 'A-Z' 'a-z')" + if [ -z "$SHA" ]; then + # Cannot check, NOT a mismatch. `job.workflow_sha` is the `job`-context + # accessor added in Actions runner v2.334.0, so only an older runner + # gets here — every job here is `runs-on: ubuntu-latest`, so that + # means a runner group shadowing the label. Warn and continue: a + # property nothing could EVALUATE is not evidence of a violation, and + # failing would take the whole review down on a caller that changed + # nothing. (public-repo-hygiene.yml fails closed in this branch on + # purpose — it is the checker whose own integrity rests on the + # equality. Here the ledger's warn-and-degrade posture is the fit.) + echo "::warning::job.workflow_sha is empty (Actions runner older than v2.334.0?), so workflows_ref could not be cross-checked against the uses: pin. The empty and 40-hex tests above still apply." + elif [ "$REF_LC" != "$SHA" ]; then + echo "::error::workflows_ref ($REF) is not the commit this workflow was called at ($SHA). The uses: pin and workflows_ref MUST name the same commit — otherwise the workflow is pinned but its prompts and scripts are loaded from a different version. Set both to $SHA. (The bump-callers fleet rewrites both together; Dependabot rewrites only uses:, so a caller that lets Dependabot bump this reusable needs a dependabot.yml ignore entry for Comfy-Org/github-workflows.)" + exit 1 + fi - name: Load cursor-review assets uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -2871,6 +3118,7 @@ jobs: # otherwise. Fail fast instead. (BE-5546) env: WORKFLOWS_REF: ${{ inputs.workflows_ref }} + WORKFLOW_SHA: ${{ job.workflow_sha }} 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 @@ -2886,6 +3134,36 @@ jobs: 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 + # Equality with the commit the caller's `uses:` line resolved to — the + # same guard public-repo-hygiene.yml runs, and the check that turns + # "workflows_ref must be the same SHA as uses:" from a header comment + # into a fact. `job.workflow_sha` is computed by the RUNNER from the + # `uses:` pin, so a caller cannot set it. Without this, a caller can + # pin `uses:` to one commit and load this workflow's prompts, scripts + # and model list from another. (`job.workflow_sha`, NOT + # `github.job_workflow_sha` — that one is an OIDC token claim, absent + # from the `github` context, and expands to ''.) Both sides are + # lower-cased: hex is case-insensitive, and unlike hygiene's the + # 40-hex test above only WARNS, so an upper-case ref naming the RIGHT + # commit reaches here and must not fail. $REF itself is left alone so + # the messages quote what the caller wrote. (BE-15927) + REF_LC="$(printf '%s' "$REF" | tr 'A-Z' 'a-z')" + SHA="$(printf '%s' "$WORKFLOW_SHA" | tr -d '[:space:]' | tr 'A-Z' 'a-z')" + if [ -z "$SHA" ]; then + # Cannot check, NOT a mismatch. `job.workflow_sha` is the `job`-context + # accessor added in Actions runner v2.334.0, so only an older runner + # gets here — every job here is `runs-on: ubuntu-latest`, so that + # means a runner group shadowing the label. Warn and continue: a + # property nothing could EVALUATE is not evidence of a violation, and + # failing would take the whole review down on a caller that changed + # nothing. (public-repo-hygiene.yml fails closed in this branch on + # purpose — it is the checker whose own integrity rests on the + # equality. Here the ledger's warn-and-degrade posture is the fit.) + echo "::warning::job.workflow_sha is empty (Actions runner older than v2.334.0?), so workflows_ref could not be cross-checked against the uses: pin. The empty and 40-hex tests above still apply." + elif [ "$REF_LC" != "$SHA" ]; then + echo "::error::workflows_ref ($REF) is not the commit this workflow was called at ($SHA). The uses: pin and workflows_ref MUST name the same commit — otherwise the workflow is pinned but its prompts and scripts are loaded from a different version. Set both to $SHA. (The bump-callers fleet rewrites both together; Dependabot rewrites only uses:, so a caller that lets Dependabot bump this reusable needs a dependabot.yml ignore entry for Comfy-Org/github-workflows.)" + exit 1 + fi - name: Load cursor-review assets uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 7033bf5..78ef45d 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -113,7 +113,7 @@ pull-requests: write # posting the consolidated review | `extra_generated_globs` | `**/node_modules/**`
`**/dist/**`
`**/vendor/**`
`**/*.generated.*`
`**/*.min.js`
`**/*.min.css` | Extra globs the shared `check-pr-size` classifier treats as generated — kept out of **both** the size-budget count and the reviewed diff. Passing your own value **replaces** the default list, so re-state the entries you still want — **copy them verbatim**, `**/…/**` and all: a pattern with no `/` matches only the *base name*, so a bare `node_modules` matches a file literally named `node_modules` and excludes nothing under the directory; and conversely a pattern that *does* contain a `/` is anchored to the whole repo-relative path unless it opens with `**/`, so `data/gen.json` matches only the root-level file and misses `packages/x/data/gen.json`. These are plain globs, **not** git pathspecs — never carry a `:!` prefix over from `diff_excludes` (see that row). `.claude` is deliberately **not** in the default: hand-authored agent instructions are prose worth reviewing. A repo whose `.claude/` tree is vendored/tool-installed output (a BMAD-method install, say) should pass the defaults above plus `**/.claude/**` — otherwise that tree now counts toward `diff_size_cap`, and a PR over the cap is skipped silently (no review comment, no Slack notice). | | `extra_lockfiles` | `''` | Extra dependency-lockfile base names, on top of the classifier's built-ins. | | `diff_excludes` | `''` | Pathspecs excluded from the reviewed diff **only** (not the size count) — back-compat escape hatch; prefer `extra_generated_globs`. Each entry must carry git pathspec-magic (`:!**/foo/**` or `:(exclude)**/foo/**`); the value is word-split into `git diff … -- . `, so a plain path is OR'd with `.` and excludes nothing. **Migrating:** this input used to exclude from *both* the count and the diff. If your caller lists generated paths here, move them to `extra_generated_globs` — left here they still leave the reviewed diff but are now counted, which can push the PR over `diff_size_cap`. **Strip the `:!` / `:(exclude)` prefix on the way over:** `extra_generated_globs` takes plain globs, not pathspecs, and the classifier compiles each token literally — a verbatim `:!**/vendor/**` becomes the anchored regexp `^:!(?:.*/)?vendor/.*$`, which matches no repo-relative path, so the exclusion silently vanishes from both the count and the diff (only `extra_lockfiles` validates its entries). Write `**/vendor/**`. | -| `workflows_ref` | — (**required**) | Pin to the SAME full commit SHA as `uses:`. No default on purpose. The review prompts and scripts load from this ref at run time. Each job that checks them out carries its own `Require a pinned workflows_ref` step and fails fast on an empty or omitted value — but treat that as a backstop, not a guarantee: a job the label gate skips never evaluates it, and the `Prior-review ledger` job is deliberately exempt (it must never fail the run, since the review matrix `needs:` it) and falls back instead of erroring. | +| `workflows_ref` | — (**required**) | Pin to the SAME full commit SHA as `uses:`. No default on purpose. The review prompts and scripts load from this ref at run time. Each job that checks them out carries its own `Require a pinned workflows_ref` step and fails fast on an empty value **and on a value that differs from the commit `uses:` resolved to** (`job.workflow_sha`, which the runner computes from the `uses:` pin, so a caller cannot set it); a runner too old to supply `job.workflow_sha` warns and skips that comparison rather than failing. But treat the whole step as a backstop, not a guarantee: a job the label gate skips never evaluates it, and the `Prior-review ledger` job is deliberately exempt (it must never fail the run, since the review matrix `needs:` it) — it falls back instead of erroring, and downgrades the same mismatch to a `::warning::`. | | `bot_app_id` | `''` | Post as your App. | | `ledger_prior_review` | `true` | Give each round the prior rounds' findings + author replies, so a refuted or deferred finding is not re-litigated. | | `run_without_label` | `false` | Run on every PR rather than waiting for the label. **Also requires widening your caller's `types:`** — see the gotcha. |