[PR #19] [MERGED] fix(cursor-review): harden blocking gate + pin all action SHAs (BE-1981) #30

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/19
Author: @mattmillerai
Created: 6/30/2026
Status: ✅ Merged
Merged: 7/3/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-1981-blocking-gate-hardening

📝 Commits (1)

a15dbd5 fix(cursor-review): harden blocking gate + pin all action SHAs (BE-1981)

📊 Changes

1 file changed (+24 additions, -12 deletions)

View changed files

📝 .github/workflows/cursor-review.yml (+24 -12)

📄 Description

ELI-5

The opt-in "Blocking gate" check is the one a repo can require before merging. Two problems from PR #16 (BE-1891):

If the actual AI review crashed before posting anything, the gate saw "no unresolved threads" and turned green — letting a broken review slip through. Now the gate explicitly fails when a fresh, within-size-cap run's review (consolidate) didn't finish.
Several actions/* were pinned to mutable tags (@v6), which supply-chain scanners (zizmor/pinact) flag. They're now pinned to immutable commit SHAs.

What changed

.github/workflows/cursor-review.yml only.

Finding 1 — Blocking gate can pass on a failed fresh review

blocking-gate job: needs now includes diff-size → needs: [gate, diff-size, consolidate].
Added a guard step that fails the gate when a fresh, within-cap run did not complete consolidate:

- name: Fail if the fresh review did not complete
  if: ${{ needs.gate.outputs.already_reviewed != 'true' && needs.diff-size.outputs.within_cap == 'true' && needs.consolidate.result != 'success' }}
  run: |
    echo "Consolidate panel did not complete; refusing to pass the blocking gate."
    exit 1

Path analysis (all verified):

Fresh, within-cap, consolidate failed/cancelled → guard fires → gate fails (the fix).
Fresh, within-cap, consolidate succeeded → consolidate.result == 'success' → guard skipped → falls through to the live unresolved-thread query.
already_reviewed re-trigger → already_reviewed != 'true' is false (and diff-size is skipped, so within_cap == '') → guard skipped → falls through (unchanged behavior).
over-cap → within_cap != 'true' → guard skipped → falls through (unchanged behavior).

Pattern mirrors the existing notify-complete job, which already reads needs.consolidate.result.

Finding 2 — Pin actions to full SHAs

All actions/checkout@v6 (8 occurrences) pinned to df4cb1c… (# v6.0.3). For full file consistency and to satisfy zizmor's hash-pin policy (the repo README documents SHA-pinning as the convention), also pinned the other three actions in this file:

actions/upload-artifact@v7 → 043fb46… (# v7.0.1)
actions/download-artifact@v8 → 3e5f45b… (# v8.0.1)
actions/create-github-app-token@v3 → bcd2ba4… (# v3.2.0)

Verification

actionlint 1.7.12: clean (exit 0).
zizmor 1.16.3: regular persona — no findings; under the strict hash-pin policy this file now reports 0 unpinned-uses errors vs 11 on main. The pre-existing excessive-permissions pedantic findings are identical before/after (out of scope — not part of the two CodeRabbit Major threads).
YAML parse: OK.

Acceptance criteria

✅ Blocking gate fails on a fresh within-cap run whose consolidate didn't succeed; passes correctly on already-reviewed / over-cap skip paths.
✅ All actions/checkout references in cursor-review.yml pinned to full SHAs (plus the other three actions in the file).
✅ actionlint / zizmor clean (no new findings; the unpinned-uses concern is fully resolved).

Judgment calls / notes

Pinned 3 actions beyond checkout. Finding 2 enumerated only checkout, but AC3 ("zizmor clean") plus the README's documented SHA-pin convention meant the other three unpinned actions in this file would still fail the hash-pin policy. Pinning them is the same supply-chain fix class and makes the whole file clean. (Under hash-pin: 11 errors → 0.)
Out of scope: test-cursor-review-scripts.yml has its own unpinned actions/checkout@v6 and actions/setup-python@v6. Not touched (ticket scopes to cursor-review.yml); worth a follow-up if the repo wants every workflow hash-pin-clean.
Residual edge (beyond the specified guard): if the diff-size job itself fails (not skips) on a fresh run, within_cap is empty so the guard skips and the gate falls through to the thread query. The ticket specified the exact guard above (keyed on the consolidate path), which I implemented verbatim; this rarer diff-size-infra-failure case is the same kind of gap the existing notify-complete job has and is left as-is.

_{🔄 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/19 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 6/30/2026 **Status:** ✅ Merged **Merged:** 7/3/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-1981-blocking-gate-hardening` --- ### 📝 Commits (1) - [`a15dbd5`](https://github.com/Comfy-Org/github-workflows/commit/a15dbd5ae4d6922500798d5de4c7fd12d502ac63) fix(cursor-review): harden blocking gate + pin all action SHAs (BE-1981) ### 📊 Changes **1 file changed** (+24 additions, -12 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/cursor-review.yml` (+24 -12) </details> ### 📄 Description ## ELI-5 The opt-in "Blocking gate" check is the one a repo can require before merging. Two problems from PR #16 (BE-1891): 1. If the actual AI review crashed before posting anything, the gate saw "no unresolved threads" and turned **green** — letting a broken review slip through. Now the gate explicitly **fails** when a fresh, within-size-cap run's review (`consolidate`) didn't finish. 2. Several `actions/*` were pinned to mutable tags (`@v6`), which supply-chain scanners (zizmor/pinact) flag. They're now pinned to immutable commit SHAs. ## What changed `.github/workflows/cursor-review.yml` only. ### Finding 1 — Blocking gate can pass on a failed fresh review - `blocking-gate` job: `needs` now includes `diff-size` → `needs: [gate, diff-size, consolidate]`. - Added a guard step that fails the gate when a fresh, within-cap run did not complete `consolidate`: ```yaml - name: Fail if the fresh review did not complete if: ${{ needs.gate.outputs.already_reviewed != 'true' && needs.diff-size.outputs.within_cap == 'true' && needs.consolidate.result != 'success' }} run: | echo "Consolidate panel did not complete; refusing to pass the blocking gate." exit 1 ``` Path analysis (all verified): - **Fresh, within-cap, consolidate failed/cancelled** → guard fires → gate **fails** (the fix). - **Fresh, within-cap, consolidate succeeded** → `consolidate.result == 'success'` → guard skipped → falls through to the live unresolved-thread query. - **already_reviewed re-trigger** → `already_reviewed != 'true'` is false (and `diff-size` is skipped, so `within_cap == ''`) → guard skipped → falls through (unchanged behavior). - **over-cap** → `within_cap != 'true'` → guard skipped → falls through (unchanged behavior). Pattern mirrors the existing `notify-complete` job, which already reads `needs.consolidate.result`. ### Finding 2 — Pin actions to full SHAs All `actions/checkout@v6` (8 occurrences) pinned to `df4cb1c…` (`# v6.0.3`). For full file consistency and to satisfy zizmor's hash-pin policy (the repo README documents SHA-pinning as the convention), also pinned the other three actions in this file: - `actions/upload-artifact@v7` → `043fb46…` (`# v7.0.1`) - `actions/download-artifact@v8` → `3e5f45b…` (`# v8.0.1`) - `actions/create-github-app-token@v3` → `bcd2ba4…` (`# v3.2.0`) ## Verification - **actionlint** 1.7.12: clean (exit 0). - **zizmor** 1.16.3: regular persona — no findings; under the strict `hash-pin` policy this file now reports **0** `unpinned-uses` errors vs **11** on `main`. The pre-existing `excessive-permissions` pedantic findings are identical before/after (out of scope — not part of the two CodeRabbit Major threads). - YAML parse: OK. ## Acceptance criteria 1. ✅ Blocking gate fails on a fresh within-cap run whose `consolidate` didn't succeed; passes correctly on already-reviewed / over-cap skip paths. 2. ✅ All `actions/checkout` references in `cursor-review.yml` pinned to full SHAs (plus the other three actions in the file). 3. ✅ actionlint / zizmor clean (no new findings; the unpinned-uses concern is fully resolved). ## Judgment calls / notes - **Pinned 3 actions beyond `checkout`.** Finding 2 enumerated only `checkout`, but AC3 ("zizmor clean") plus the README's documented SHA-pin convention meant the other three unpinned actions in this file would still fail the hash-pin policy. Pinning them is the same supply-chain fix class and makes the whole file clean. (Under hash-pin: 11 errors → 0.) - **Out of scope:** `test-cursor-review-scripts.yml` has its own unpinned `actions/checkout@v6` and `actions/setup-python@v6`. Not touched (ticket scopes to `cursor-review.yml`); worth a follow-up if the repo wants every workflow hash-pin-clean. - **Residual edge (beyond the specified guard):** if the `diff-size` *job itself fails* (not skips) on a fresh run, `within_cap` is empty so the guard skips and the gate falls through to the thread query. The ticket specified the exact guard above (keyed on the consolidate path), which I implemented verbatim; this rarer diff-size-infra-failure case is the same kind of gap the existing `notify-complete` job has and is left as-is. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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