[PR #328] [MERGED] feat(cursor-review): fail the review when workflows_ref differs from the uses: pin #330

New issue

Closed

opened 2026-10-26 22:38:35 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:38:35 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/328
Author: @mattmillerai
Created: 9/19/2026
Status: ✅ Merged
Merged: 9/19/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-15934-cursor-review-workflows-ref-equality-guard

📝 Commits (1)

f0c7f73 feat(cursor-review): fail the review when workflows_ref differs from the uses: pin

📊 Changes

4 files changed (+516 additions, -1 deletions)

View changed files

📝 .github/cursor-review/README.md (+8 -0)
➕ .github/cursor-review/tests/test_workflow_ref_guard_parity.py (+229 -0)
📝 .github/workflows/cursor-review.yml (+278 -0)
📝 docs/callers/cursor-review.md (+1 -1)

📄 Description

ELI-5

A caller of cursor-review.yml pins it twice: uses: says which workflow to run, and workflows_ref: says which commit to load the review prompts and scripts from. They are supposed to name the same commit — the header has said so for a year — but nothing checked it. If they drift, the review runs one version's workflow against a different version's judging logic, and both pins look perfectly plausible in the caller file.

public-repo-hygiene.yml already solved this: compare workflows_ref against job.workflow_sha, which the runner computes from the uses: line, so a caller cannot forge it. This ports that guard into all eight of cursor-review's Require a pinned workflows_ref steps and fails the job on a mismatch.

What changed

The eight guard steps each gain WORKFLOW_SHA: ${{ job.workflow_sha }} in env: and an equality test after the existing empty/40-hex tests. They remain byte-identical to one another (there is no anchor, no step-level uses: ./, and no job-level run: in workflow_call, so eight literal copies is the only spelling available).
The exempt Prior-review ledger job gets the same cross-check as a warning with no exit. That job must never fail — the review matrix needs: it — but it runs for every caller, including one whose guarded jobs are all skipped by the label gate, so its log is sometimes the only place a split pin would be recorded.
Docs: the workflows_ref row in the setup guide, one line in the workflow's header example, and a new workflows_ref must equal the uses: pin section in .github/cursor-review/README.md covering the rule, the warn-on-empty branch, and how to fix a red run.
test_workflow_ref_guard_parity.py asserts the count is exactly 8 (so a ninth guarded job is an explicit opt-in), that all eight blocks are byte-identical, that the guard binds the bare input rather than the || job.workflow_sha fallback, that the mismatch branch exits, that the empty-job.workflow_sha branch does not, and — from the other side — that the ledger reads job.workflow_sha and never exits. Picked up automatically by the suite's unittest discover; no workflow wiring needed.

Blast radius — measured, not assumed

I swept every caller GitHub code search can see and compared the two pins in each:


external caller workflow files (one per repo, 27 repos)	27
pins already equal → unaffected	26
pins split → will fail once bumped past this commit	1 (`Comfy-Org/comfy-typescript-sdk`, `ebfd9e4` vs `ffcc3f5`)
`workflows_ref` values that are not 40-hex	0
this repo's own `ci-cursor-review.yml`	equal at `425c154`, and it runs the pre-change reusable, so it does not exercise this guard

The guard is a true positive in the wild today. The identical check in public-repo-hygiene.yml failed a live run this morning — comfy-cli run 35427331671, on branch dependabot/github_actions/…, with workflows_ref (3b2c8ca…) is not the commit this workflow was called at (a404fd4…). Dependabot had bumped uses: alone. That same run log is the empirical proof that job.workflow_sha is populated on real GitHub-hosted runners inside a workflow_call reusable (it printed a real github-workflows main SHA), which is the one premise that would otherwise make this change a false-fail machine. A separate hygiene run on comfy-cli (35428909227) is green, so the guard is not simply failing everything.

Dependabot is the mechanism that will actually trip this. bump-callers.sh rewrites both pins together, so a fleet bump keeps them equal; Dependabot rewrites only uses:, which splits them. Of the 27 external callers, 8 have a github-actions Dependabot ecosystem with no ignore entry for Comfy-Org/github-workflows and are therefore exposed — 5 public (ComfyUI_frontend, comfy-cli, comfy-mcp, comfy-python-sdk, comfy-typescript-sdk, each confirmed public via the API before naming it here) and 3 non-public, which I have deliberately not named. 3 callers already have the ignore, 15 have no dependabot.yml at all, and 1 has no github-actions ecosystem. Expect cursor-review to start going red on Dependabot PRs in those 8 the same way public-repo-hygiene already does. That is the intended signal, and the error message names the fix.

Judgment calls

The empty-job.workflow_sha branch warns; hygiene fails closed. Hygiene's rationale is that it is the checker whose own integrity rests on the equality, so a runner that cannot supply the value cannot run it. cursor-review is not in that position, and failing eight jobs over a property nothing could evaluate would take a review down on a caller that changed nothing. Every job here is runs-on: ubuntu-latest, so only a runner group shadowing that label reaches the branch. Flagging it explicitly because the two guards are meant to converge.
Both sides are lower-cased before the comparison — a deliberate deviation from the literal plan. The plan said to leave REF alone because "the 40-hex grep already fixes case". That is true in hygiene, where the shape test hard-fails an upper-case ref several lines earlier, and false here, where the same test only warns. So an upper-case-but-correct ref reaches the comparison, and comparing raw would fail all eight jobs of a caller whose two pins genuinely agree — with an error message stating something false. $REF itself is untouched, so the messages still quote what the caller wrote. Verified: an upper-case 40-hex ref equal to $SHA now exits 0 with only the pre-existing shape warning.
Dropped the cross-repo PR reference from the error message. The plan's text ended with "as comfy-python-sdk#161 did". That PR is still open (as is comfy-typescript-sdk#153), so the message would have pointed maintainers at an unmerged precedent; and the name is not on .github/lint/org-repo-allowlist.txt, so publishing it into a workflow file would sidestep the ratification step that allowlist exists to be. The message keeps the actionable half: add a dependabot.yml ignore for Comfy-Org/github-workflows.
A branch- or tag-shaped workflows_ref now hard-fails — not via the shape test, which I left as a ::warning:: exactly as instructed, but as a consequence of the equality test, since main is not a 40-hex SHA. This is unavoidable for any equality guard and matches hygiene. It affects nobody today: 0 of 27 callers use a non-SHA workflows_ref.
tr 'A-Z' 'a-z' draws shellcheck SC2018/SC2019 infos. Kept verbatim from hygiene — [:upper:]/[:lower:] would add accent handling that is wrong for byte-exact hex, and nothing in CI shellchecks workflow run: blocks.

Sequencing

The related port into the other six reusables (agents-md-integrity, linear-ticket, pr-area-label, pr-size, coderabbit-config-validate, refresh-reviewers) is tracked separately and deliberately untouched here — this PR is cursor-review.yml only. The two ports should converge on one guard text; judgment calls 1 and 2 above are the places where this one had to diverge from the hygiene original, and both reasons are file-specific rather than preferences.

comfy-typescript-sdk#153 re-syncs the one split caller and adds its Dependabot ignore; it is still open. Merging after it lands is tidier but not required: #153 re-syncs to 425c154, which predates this commit, so that repo's cursor-review does not go red the moment this merges — the guard only executes once its uses: is bumped to a commit containing it, and a bump-callers bump moves both pins together.

Open PRs #320/#321/#322 also edit cursor-review.yml; I confirmed none of them touches a guard step, a WORKFLOWS_REF binding, or adds a guarded job (which would need EXPECTED_GUARDS bumped).

Residual

The 8 callers exposed to the Dependabot pin-split are not fixed by this PR. Each needs an ignore entry for Comfy-Org/github-workflows in its own dependabot.yml; that is a cross-repo change and out of scope here. Until then, the first Dependabot bump after this merges will turn their cursor-review red. The 5 public ones are named above; the 3 non-public ones are identifiable by re-running the sweep against the CURSOR_REVIEW_CALLERS roster.
The CURSOR_REVIEW_CALLERS roster was not audited. The sweep above is GitHub code search, which sees only what the search index covers and only default branches. Roster secrets are write-only, so I could not check the sweep against the canonical caller list in either direction — a caller missing from code search would not appear in any number here.
The guard was never executed by a real Actions runner as part of this change. Verification was the extracted run: body under a local shell, plus the live evidence from the identical hygiene guard. This repo's own ci-cursor-review.yml pins the pre-change reusable, so it cannot exercise it; the first real execution is the next fleet bump.
The source spike's findings comment was not readable from this environment. The claims it carries that I could independently re-verify, I did: 8 guard steps, no pre-existing comparison, hygiene green on a live GitHub-hosted caller, check_workflow_pins.py accepting the hygiene shape, ci-cursor-review.yml pinned to 425c154, and none of #320/#321/#322 touching the guard. Anything else in that comment is unexercised.
Hard-failing the 40-hex shape test is still open, and this PR changes its practical effect without changing its code. The shape test remains a warning, but the new equality test rejects every non-SHA ref anyway, so the separate hard-fail change is now largely redundant in this file and should be re-scoped rather than applied as written.

Provenance

Authored by: agent-work loop
Verified: python3 .github/workflow-pins/check_workflow_pins.py → OK — 11 workflow(s) … every ref checkout guarded, exit 0; unittest discover .github/cursor-review/tests → 611 passed, 0 failed (11 of them the new parity suite); unittest discover .github/workflow-pins/tests → 499 passed, 32 skipped, 0 failed; check_agents_md.py --root . → passed (2 pre-existing warnings); check-org-repo-literals.sh → OK, plus shellcheck -x on it; PyYAML parse of cursor-review.yml confirming 8 guard steps with the expected env: and the ledger's two bindings; extracted-guard shell runs covering equal / differing / empty-WORKFLOW_SHA / empty-ref / whitespace-only-ref / branch-ref / upper-case-ref, and the extracted ledger body across the same matrix confirming it never exits non-zero; four mutation tests (drop one copy's binding, remove the exit, add an exit to the ledger, add a 9th guard step) each confirming the new suite fails.
Deviations: judgment calls 2 and 3 above — the comparison is case-insensitive, and the cross-repo PR reference was dropped from the error message. Everything else follows the plan as written.

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

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/328 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/19/2026 **Status:** ✅ Merged **Merged:** 9/19/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15934-cursor-review-workflows-ref-equality-guard` --- ### 📝 Commits (1) - [`f0c7f73`](https://github.com/Comfy-Org/github-workflows/commit/f0c7f73390b9648d59dd0875868aad042c93c5a1) feat(cursor-review): fail the review when workflows_ref differs from the uses: pin ### 📊 Changes **4 files changed** (+516 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+8 -0) ➕ `.github/cursor-review/tests/test_workflow_ref_guard_parity.py` (+229 -0) 📝 `.github/workflows/cursor-review.yml` (+278 -0) 📝 `docs/callers/cursor-review.md` (+1 -1) </details> ### 📄 Description ## ELI-5 A caller of `cursor-review.yml` pins it **twice**: `uses:` says which workflow to run, and `workflows_ref:` says which commit to load the review prompts and scripts from. They are supposed to name the same commit — the header has said so for a year — but nothing checked it. If they drift, the review runs one version's workflow against a *different* version's judging logic, and both pins look perfectly plausible in the caller file. `public-repo-hygiene.yml` already solved this: compare `workflows_ref` against `job.workflow_sha`, which the runner computes from the `uses:` line, so a caller cannot forge it. This ports that guard into all eight of cursor-review's `Require a pinned workflows_ref` steps and fails the job on a mismatch. ## What changed - **The eight guard steps** each gain `WORKFLOW_SHA: ${{ job.workflow_sha }}` in `env:` and an equality test after the existing empty/40-hex tests. They remain byte-identical to one another (there is no anchor, no step-level `uses: ./`, and no job-level `run:` in `workflow_call`, so eight literal copies is the only spelling available). - **The exempt `Prior-review ledger` job** gets the same cross-check as a **warning with no `exit`**. That job must never fail — the review matrix `needs:` it — but it runs for *every* caller, including one whose guarded jobs are all skipped by the label gate, so its log is sometimes the only place a split pin would be recorded. - **Docs:** the `workflows_ref` row in the setup guide, one line in the workflow's header example, and a new `workflows_ref` must equal the `uses:` pin section in `.github/cursor-review/README.md` covering the rule, the warn-on-empty branch, and how to fix a red run. - **`test_workflow_ref_guard_parity.py`** asserts the count is exactly 8 (so a ninth guarded job is an explicit opt-in), that all eight blocks are byte-identical, that the guard binds the bare input rather than the `|| job.workflow_sha` fallback, that the mismatch branch exits, that the empty-`job.workflow_sha` branch does *not*, and — from the other side — that the ledger reads `job.workflow_sha` and never exits. Picked up automatically by the suite's `unittest discover`; no workflow wiring needed. ## Blast radius — measured, not assumed I swept every caller GitHub code search can see and compared the two pins in each: | | | |---|---| | external caller workflow files (one per repo, 27 repos) | **27** | | pins already equal → unaffected | **26** | | pins split → will fail once bumped past this commit | **1** (`Comfy-Org/comfy-typescript-sdk`, `ebfd9e4` vs `ffcc3f5`) | | `workflows_ref` values that are not 40-hex | **0** | | this repo's own `ci-cursor-review.yml` | equal at `425c154`, and it runs the pre-change reusable, so it does not exercise this guard | **The guard is a true positive in the wild today.** The identical check in `public-repo-hygiene.yml` failed a live run this morning — `comfy-cli` run `35427331671`, on branch `dependabot/github_actions/…`, with `workflows_ref (3b2c8ca…) is not the commit this workflow was called at (a404fd4…)`. Dependabot had bumped `uses:` alone. That same run log is the empirical proof that `job.workflow_sha` is populated on real GitHub-hosted runners inside a `workflow_call` reusable (it printed a real `github-workflows` main SHA), which is the one premise that would otherwise make this change a false-fail machine. A separate hygiene run on `comfy-cli` (`35428909227`) is green, so the guard is not simply failing everything. **Dependabot is the mechanism that will actually trip this.** `bump-callers.sh` rewrites both pins together, so a fleet bump keeps them equal; Dependabot rewrites only `uses:`, which splits them. Of the 27 external callers, **8 have a `github-actions` Dependabot ecosystem with no `ignore` entry for `Comfy-Org/github-workflows`** and are therefore exposed — 5 public (`ComfyUI_frontend`, `comfy-cli`, `comfy-mcp`, `comfy-python-sdk`, `comfy-typescript-sdk`, each confirmed public via the API before naming it here) and 3 non-public, which I have deliberately not named. 3 callers already have the ignore, 15 have no `dependabot.yml` at all, and 1 has no `github-actions` ecosystem. Expect cursor-review to start going red on Dependabot PRs in those 8 the same way public-repo-hygiene already does. That is the intended signal, and the error message names the fix. ## Judgment calls 1. **The empty-`job.workflow_sha` branch warns; hygiene fails closed.** Hygiene's rationale is that it is the checker whose own integrity rests on the equality, so a runner that cannot supply the value cannot run it. cursor-review is not in that position, and failing eight jobs over a property nothing could evaluate would take a review down on a caller that changed nothing. Every job here is `runs-on: ubuntu-latest`, so only a runner group shadowing that label reaches the branch. Flagging it explicitly because the two guards are meant to converge. 2. **Both sides are lower-cased before the comparison — a deliberate deviation from the literal plan.** The plan said to leave `REF` alone because "the 40-hex grep already fixes case". That is true in hygiene, where the shape test *hard-fails* an upper-case ref several lines earlier, and false here, where the same test only **warns**. So an upper-case-but-correct ref reaches the comparison, and comparing raw would fail all eight jobs of a caller whose two pins genuinely agree — with an error message stating something false. `$REF` itself is untouched, so the messages still quote what the caller wrote. Verified: an upper-case 40-hex ref equal to `$SHA` now exits 0 with only the pre-existing shape warning. 3. **Dropped the cross-repo PR reference from the error message.** The plan's text ended with "as comfy-python-sdk#161 did". That PR is still **open** (as is `comfy-typescript-sdk#153`), so the message would have pointed maintainers at an unmerged precedent; and the name is not on `.github/lint/org-repo-allowlist.txt`, so publishing it into a workflow file would sidestep the ratification step that allowlist exists to be. The message keeps the actionable half: add a `dependabot.yml` ignore for `Comfy-Org/github-workflows`. 4. **A branch- or tag-shaped `workflows_ref` now hard-fails** — not via the shape test, which I left as a `::warning::` exactly as instructed, but as a consequence of the equality test, since `main` is not a 40-hex SHA. This is unavoidable for any equality guard and matches hygiene. It affects nobody today: 0 of 27 callers use a non-SHA `workflows_ref`. 5. `tr 'A-Z' 'a-z'` draws shellcheck `SC2018`/`SC2019` infos. Kept verbatim from hygiene — `[:upper:]`/`[:lower:]` would add accent handling that is wrong for byte-exact hex, and nothing in CI shellchecks workflow `run:` blocks. ## Sequencing The related port into the other six reusables (`agents-md-integrity`, `linear-ticket`, `pr-area-label`, `pr-size`, `coderabbit-config-validate`, `refresh-reviewers`) is tracked separately and deliberately untouched here — this PR is `cursor-review.yml` only. The two ports should converge on one guard text; judgment calls 1 and 2 above are the places where this one had to diverge from the hygiene original, and both reasons are file-specific rather than preferences. `comfy-typescript-sdk#153` re-syncs the one split caller and adds its Dependabot ignore; it is still open. Merging after it lands is tidier but not required: `#153` re-syncs to `425c154`, which predates this commit, so that repo's cursor-review does **not** go red the moment this merges — the guard only executes once its `uses:` is bumped to a commit containing it, and a `bump-callers` bump moves both pins together. Open PRs #320/#321/#322 also edit `cursor-review.yml`; I confirmed none of them touches a guard step, a `WORKFLOWS_REF` binding, or adds a guarded job (which would need `EXPECTED_GUARDS` bumped). ## Residual - **The 8 callers exposed to the Dependabot pin-split are not fixed by this PR.** Each needs an `ignore` entry for `Comfy-Org/github-workflows` in its own `dependabot.yml`; that is a cross-repo change and out of scope here. Until then, the first Dependabot bump after this merges will turn their cursor-review red. The 5 public ones are named above; the 3 non-public ones are identifiable by re-running the sweep against the `CURSOR_REVIEW_CALLERS` roster. - **The `CURSOR_REVIEW_CALLERS` roster was not audited.** The sweep above is GitHub code search, which sees only what the search index covers and only default branches. Roster secrets are write-only, so I could not check the sweep against the canonical caller list in either direction — a caller missing from code search would not appear in any number here. - **The guard was never executed by a real Actions runner as part of this change.** Verification was the extracted `run:` body under a local shell, plus the live evidence from the identical hygiene guard. This repo's own `ci-cursor-review.yml` pins the pre-change reusable, so it cannot exercise it; the first real execution is the next fleet bump. - **The source spike's findings comment was not readable from this environment.** The claims it carries that I could independently re-verify, I did: 8 guard steps, no pre-existing comparison, hygiene green on a live GitHub-hosted caller, `check_workflow_pins.py` accepting the hygiene shape, `ci-cursor-review.yml` pinned to `425c154`, and none of #320/#321/#322 touching the guard. Anything else in that comment is unexercised. - **Hard-failing the 40-hex shape test is still open, and this PR changes its practical effect without changing its code.** The shape test remains a warning, but the new equality test rejects every non-SHA ref anyway, so the separate hard-fail change is now largely redundant in this file and should be re-scoped rather than applied as written. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 .github/workflow-pins/check_workflow_pins.py` → `OK — 11 workflow(s) … every ref checkout guarded`, exit 0; `unittest discover .github/cursor-review/tests` → 611 passed, 0 failed (11 of them the new parity suite); `unittest discover .github/workflow-pins/tests` → 499 passed, 32 skipped, 0 failed; `check_agents_md.py --root .` → passed (2 pre-existing warnings); `check-org-repo-literals.sh` → OK, plus `shellcheck -x` on it; PyYAML parse of `cursor-review.yml` confirming 8 guard steps with the expected `env:` and the ledger's two bindings; extracted-guard shell runs covering equal / differing / empty-`WORKFLOW_SHA` / empty-ref / whitespace-only-ref / branch-ref / upper-case-ref, and the extracted ledger body across the same matrix confirming it never exits non-zero; four mutation tests (drop one copy's binding, remove the `exit`, add an `exit` to the ledger, add a 9th guard step) each confirming the new suite fails. - **Deviations:** judgment calls 2 and 3 above — the comparison is case-insensitive, and the cross-repo PR reference was dropped from the error message. Everything else follows the plan as written. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:38:35 +00:00