[PR #246] [MERGED] fix(cursor-review): restore the blocking gate dropped by #31 (BE-4691) #254

New issue

Closed

opened 2026-10-26 22:37:48 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:48 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/246
Author: @mattmillerai
Created: 8/28/2026
Status: ✅ Merged
Merged: 8/28/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-4691-restore-the-cursor-review-blocking-gate-dropped-by-31-or

📝 Commits (3)

2444d45 fix(cursor-review): restore the blocking gate dropped by #31 (BE-4691)
7d33b7f docs(cursor-review): correct the stale panel count — 3 labs, 6 cells (BE-4691)
140de31 fix(cursor-review): close the blocking gate's fail-open paths (BE-4691)

📊 Changes

12 files changed (+1006 additions, -82 deletions)

View changed files

📝 .github/cursor-review/README.md (+42 -33)
📝 .github/cursor-review/post-review.py (+95 -2)
➕ .github/cursor-review/tests/test_post_review_delivery.py (+286 -0)
➕ .github/cursor-review/tests/test_workflow_inputs_docs.py (+141 -0)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+5 -2)
📝 .github/workflows/cursor-review.yml (+292 -17)
📝 .github/workflows/test-cursor-review-scripts.yml (+11 -0)
📝 AGENTS.md (+1 -1)
📝 CONTRIBUTING.md (+1 -1)
📝 README.md (+1 -1)
📝 SECURITY.md (+1 -1)
📝 docs/callers/cursor-review.md (+130 -24)

📄 Description

ELI5

The review panel leaves findings on a PR, but nothing today makes anyone deal with them before merging — like a building inspector who mails you a defect list you're free to ignore. This restores the opt-in Blocking gate: a repo can now require that every finding thread is explicitly resolved (or fixed, which outdates it) before the merge button unlocks. It's off by default, so nothing changes for any caller until a repo flips it on.

Motivation

The gate shipped in #16 — feat(cursor-review): optional blocking gate for unresolved findings (BE-1891) — and was deleted by accident in #31 — fix(cursor-review): recover judge findings after prose, not first JSON region (BE-3160) — whose stated purpose was judge extraction; the regression went unnoticed for ~10 days. #80 — docs(callers): per-workflow setup guides, README restructure, public-repo hygiene (BE-4680) — could only fix the docs half, since restoring a merge-gating job is a workflow behavior change, and deferred the restore to its own ticket. That ticket held pending a product call on whether an unresolved-findings gate is wanted at all; that call has now been made in favor of restoring it. The restore has to be re-fitted rather than reverted: the job graph has since gained a credential split, so the observable "the review actually landed" signal moved from consolidate to post-review.

Provenance

Authored by: interactive session
Verified: python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py': 383 passed, 0 failed (the inputs-vs-docs drift suite plus the new test_post_review_delivery.py); python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py': 316 passed, 0 failed at the guard census of 18 sites; python3 .github/workflow-pins/check_workflow_pins.py: 11 workflows, none defaulted, every ref checkout guarded; python3 .github/agents-md-integrity/check_agents_md.py --root .: passed; actionlint .github/workflows/cursor-review.yml .github/workflows/test-cursor-review-scripts.yml: no new findings (one pre-existing job.workflow_sha schema false positive, present on main); the new user.type == "Bot" dedupe filter exercised against a hand-built reviews payload (bot review counts, spoofed-user and null-user reviews do not, no jq error on a null user). All PR checks green.
Deviations: the panel review raised ten findings against the first cut of the gate; nine are fixed here (see "Review round" below), one is deferred as a caller-contract change with the limit written into the setup guide instead. Two behaviour changes go beyond the original #16 job and are intentional: an over-cap PR no longer passes a blocking gate, and the dedupe waiver now requires a Bot author. Both only affect callers that set blocking: true.

Reviewer context

Type: bug fix — restores a regressed opt-in feature, hardened against the fail-open paths the review round found
Slots into: the cursor-review reusable. Merging trips bump-cursor-review-callers.yml's path filter, so the fleet SHA-bump PRs carry it to consumers — all of them on unchanged blocking: false semantics until they opt in. No Skip-caller-bump trailer, deliberately: this is behavioral.
Not in scope: enrolling any caller with blocking: true (this repo's own ci-cursor-review.yml included) — that is per-repo policy with two switches, decided by each repo's owners.

Summary

Restores the blocking: input (default false) and the Blocking gate job #31 deleted. gate-unresolved.py never left the tree (build-ledger imports it) and is unchanged.
The job is re-fitted to today's graph rather than reverted verbatim, with three hardenings:
- Runs on every delivered event when blocking is on, instead of only should_run == 'true'. A skipped required check counts as passing, so the #16 condition let any "don't review" run (an unrelated label toggled, skip-cursor-review present, the trigger label removed) mint a green gate over open finding threads. Event-independence also makes pull_request_review_thread and plain PR events pure gate re-checks — panel, DMs, and comments all stay skipped — so a required check flips green the moment the last thread is resolved and reports on every push instead of sticking at "Expected".
- The fail-closed guard keys on post-review — the terminal write since the credential split; a consolidate that succeeded into a failed post still posted nothing — and refuses to pass when diff-size itself failed (the #16 guard only exempted the over-cap verdict).
- always(), not !cancelled(). post-review needs !cancelled() because it WRITES and must not race its replacement; this job only re-reads live thread state, so a superseded run and its replacement can only agree — while !cancelled() would skip it on cancellation, and GitHub counts a skipped required check as passing.

Review round

The four-lab panel found ten fail-open paths in the first cut of the gate. Nine are fixed:

A zero exit is not proof a review landed. post-review.py exits 0 after a read-only-token 403 (review written to the job summary, never the PR), after the body-only Review failed review, after the no-findings review posted when every panel cell errored, and after the 422 inline fallback — each leaving the thread query empty. It now states delivery positively on $GITHUB_OUTPUT (delivered, plus the split between findings that carry a thread and findings that reached the body only), and the gate requires the statement. Unset reads as false, so a run that died before deciding is closed by default.
The trigger job's own failure was never inspected, so a failed gate left should_run empty, skipped every guard phrased on it, and greened the check while the Gate job sat red. Now the first thing checked.
A review whose every finding was demoted to the body creates no thread. Caught via the new counts. A partially demoted round is deliberately not failed — a body-only finding has no thread to resolve — and that limit is now stated in the gotchas rather than implied away.
Over the cap no longer passes. Diff size is author-controlled, and a padding commit both skipped the panel and outdated the earlier round's threads: one commit that cleared the findings and greened the gate.
The dedupe waiver was spoofable. The consolidated marker is public text and anyone who can open a PR can review it, so a body-only review carrying it made the gate declare the SHA already reviewed. Now requires user.type == "Bot" — the type, not a login, since the poster is caller-configurable via bot_app_id.
Cancellation waived the gate (see the always() bullet above).
A non-PR event (merge_group / push / workflow_dispatch) left argparse exiting 2 behind a permanently red check with a traceback for an explanation. Now a named, actionable failure.
The drift alarm could not fire on the drift it was written for — docs/callers/cursor-review.md was not in test-cursor-review-scripts.yml's paths: filter, so a docs-only PR deleting the blocking row (the #31 scenario) ran no job.
The header's caller example set blocking: true over the label-only triggers and label-keyed concurrency group the new gotcha calls bricking; it now carries the blocking caller shape.

Deferred, with the limit documented instead: the gate does not prove the current head SHA was reviewed. A thread stops counting once its hunk changes (isOutdated), so under a label-gated caller a cosmetic edit to the flagged lines can green the check with no re-review. Requiring a review at the head SHA is a caller-contract change — every label-gated blocking caller would go red on every push until it widened types: or enrolled auto-label — so it belongs behind its own product call, not in the restore. The setup guide now says so plainly and points at cursor-review-auto-label.yml / run_without_label: true as the close.

Un-regresses the docs everywhere #31's deletion left them behind: the workflow header + input description, .github/cursor-review/README.md (file inventory, pipeline note, a live "make the review blocking" section, knobs table), docs/callers/cursor-review.md (two-switch contract, blocking input row, a blocking-gate gotchas section covering the trigger shape, waiver semantics, and who can resolve), the top-level README catalog row, and the AGENTS.md worked example.
Adds test_workflow_inputs_docs.py: two-way set equality between on.workflow_call.inputs and both docs tables, so the next accidental input deletion fails CI loudly instead of surviving in the docs.
Updates the workflow-pins guard census (17 → 18): the gate job loads gate-unresolved.py from workflows_ref in its own job and carries its own empty-ref guard.
Adds test_post_review_delivery.py: every path on which post-review.py exits 0, asserted against what it now claims about delivery — so the gate's fail-closed property is pinned by a test rather than by a comment.

Test plan

CI suites green (cursor-review scripts, workflow-pins, agents-md-integrity)
After merge + fleet bump: confirm an existing caller (all on the default blocking: false) runs unchanged — gate job skipped, review still posts
Opt one low-traffic repo into blocking: true WITHOUT marking the check required; confirm the check goes red on unresolved threads and green after resolving the last one via a pull_request_review_thread event
Only then mark <caller job id> / Blocking gate required in that repo's ruleset
New behavior is gated behind the blocking input, default off

Fixes BE-4691

_{🔄 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/246 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/28/2026 **Status:** ✅ Merged **Merged:** 8/28/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4691-restore-the-cursor-review-blocking-gate-dropped-by-31-or` --- ### 📝 Commits (3) - [`2444d45`](https://github.com/Comfy-Org/github-workflows/commit/2444d45b5525b160d81f6eb0db0c466b983697fc) fix(cursor-review): restore the blocking gate dropped by #31 (BE-4691) - [`7d33b7f`](https://github.com/Comfy-Org/github-workflows/commit/7d33b7f1365ef0cacc69df972d736a8f2a3cbb35) docs(cursor-review): correct the stale panel count — 3 labs, 6 cells (BE-4691) - [`140de31`](https://github.com/Comfy-Org/github-workflows/commit/140de313797f77f9511ab4df37d7dcb72d5542aa) fix(cursor-review): close the blocking gate's fail-open paths (BE-4691) ### 📊 Changes **12 files changed** (+1006 additions, -82 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+42 -33) 📝 `.github/cursor-review/post-review.py` (+95 -2) ➕ `.github/cursor-review/tests/test_post_review_delivery.py` (+286 -0) ➕ `.github/cursor-review/tests/test_workflow_inputs_docs.py` (+141 -0) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+5 -2) 📝 `.github/workflows/cursor-review.yml` (+292 -17) 📝 `.github/workflows/test-cursor-review-scripts.yml` (+11 -0) 📝 `AGENTS.md` (+1 -1) 📝 `CONTRIBUTING.md` (+1 -1) 📝 `README.md` (+1 -1) 📝 `SECURITY.md` (+1 -1) 📝 `docs/callers/cursor-review.md` (+130 -24) </details> ### 📄 Description ## ELI5 The review panel leaves findings on a PR, but nothing today makes anyone deal with them before merging — like a building inspector who mails you a defect list you're free to ignore. This restores the opt-in **Blocking gate**: a repo can now require that every finding thread is explicitly resolved (or fixed, which outdates it) before the merge button unlocks. It's off by default, so nothing changes for any caller until a repo flips it on. ## Motivation The gate shipped in #16 — feat(cursor-review): optional blocking gate for unresolved findings (BE-1891) — and was deleted by accident in #31 — fix(cursor-review): recover judge findings after prose, not first JSON region (BE-3160) — whose stated purpose was judge extraction; the regression went unnoticed for ~10 days. #80 — docs(callers): per-workflow setup guides, README restructure, public-repo hygiene (BE-4680) — could only fix the docs half, since restoring a merge-gating job is a workflow behavior change, and deferred the restore to its own ticket. That ticket held pending a product call on whether an unresolved-findings gate is wanted at all; that call has now been made in favor of restoring it. The restore has to be re-fitted rather than reverted: the job graph has since gained a credential split, so the observable "the review actually landed" signal moved from `consolidate` to `post-review`. ## Provenance - **Authored by:** interactive session - **Verified:** `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'`: 383 passed, 0 failed (the inputs-vs-docs drift suite plus the new `test_post_review_delivery.py`); `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'`: 316 passed, 0 failed at the guard census of 18 sites; `python3 .github/workflow-pins/check_workflow_pins.py`: 11 workflows, none defaulted, every ref checkout guarded; `python3 .github/agents-md-integrity/check_agents_md.py --root .`: passed; `actionlint .github/workflows/cursor-review.yml .github/workflows/test-cursor-review-scripts.yml`: no new findings (one pre-existing `job.workflow_sha` schema false positive, present on main); the new `user.type == "Bot"` dedupe filter exercised against a hand-built reviews payload (bot review counts, spoofed-user and null-user reviews do not, no jq error on a null `user`). All PR checks green. - **Deviations:** the panel review raised ten findings against the first cut of the gate; nine are fixed here (see "Review round" below), one is deferred as a caller-contract change with the limit written into the setup guide instead. Two behaviour changes go beyond the original #16 job and are intentional: an over-cap PR no longer passes a blocking gate, and the dedupe waiver now requires a Bot author. Both only affect callers that set `blocking: true`. ## Reviewer context - **Type:** bug fix — restores a regressed opt-in feature, hardened against the fail-open paths the review round found - **Slots into:** the cursor-review reusable. Merging trips `bump-cursor-review-callers.yml`'s path filter, so the fleet SHA-bump PRs carry it to consumers — all of them on unchanged `blocking: false` semantics until they opt in. No `Skip-caller-bump` trailer, deliberately: this is behavioral. - **Not in scope:** enrolling any caller with `blocking: true` (this repo's own `ci-cursor-review.yml` included) — that is per-repo policy with two switches, decided by each repo's owners. ## Summary - Restores the `blocking:` input (default `false`) and the **Blocking gate** job #31 deleted. `gate-unresolved.py` never left the tree (build-ledger imports it) and is unchanged. - The job is re-fitted to today's graph rather than reverted verbatim, with three hardenings: - **Runs on every delivered event when `blocking` is on**, instead of only `should_run == 'true'`. A skipped required check counts as passing, so the #16 condition let any "don't review" run (an unrelated label toggled, `skip-cursor-review` present, the trigger label removed) mint a green gate over open finding threads. Event-independence also makes `pull_request_review_thread` and plain PR events pure gate re-checks — panel, DMs, and comments all stay skipped — so a required check flips green the moment the last thread is resolved and reports on every push instead of sticking at "Expected". - **The fail-closed guard keys on `post-review`** — the terminal write since the credential split; a `consolidate` that succeeded into a failed post still posted nothing — and refuses to pass when `diff-size` itself failed (the #16 guard only exempted the over-cap verdict). - **`always()`, not `!cancelled()`.** `post-review` needs `!cancelled()` because it WRITES and must not race its replacement; this job only re-reads live thread state, so a superseded run and its replacement can only agree — while `!cancelled()` would skip it on cancellation, and GitHub counts a skipped required check as passing. ## Review round The four-lab panel found ten fail-open paths in the first cut of the gate. Nine are fixed: - **A zero exit is not proof a review landed.** `post-review.py` exits 0 after a read-only-token 403 (review written to the job summary, never the PR), after the body-only `Review failed` review, after the no-findings review posted when every panel cell errored, and after the 422 inline fallback — each leaving the thread query empty. It now states delivery positively on `$GITHUB_OUTPUT` (`delivered`, plus the split between findings that carry a thread and findings that reached the body only), and the gate requires the statement. Unset reads as false, so a run that died before deciding is closed by default. - **The trigger job's own failure was never inspected**, so a failed gate left `should_run` empty, skipped every guard phrased on it, and greened the check while the Gate job sat red. Now the first thing checked. - **A review whose every finding was demoted to the body** creates no thread. Caught via the new counts. A *partially* demoted round is deliberately not failed — a body-only finding has no thread to resolve — and that limit is now stated in the gotchas rather than implied away. - **Over the cap no longer passes.** Diff size is author-controlled, and a padding commit both skipped the panel and outdated the earlier round's threads: one commit that cleared the findings and greened the gate. - **The dedupe waiver was spoofable.** The consolidated marker is public text and anyone who can open a PR can review it, so a body-only review carrying it made the gate declare the SHA already reviewed. Now requires `user.type == "Bot"` — the type, not a login, since the poster is caller-configurable via `bot_app_id`. - **Cancellation waived the gate** (see the `always()` bullet above). - **A non-PR event** (`merge_group` / `push` / `workflow_dispatch`) left argparse exiting 2 behind a permanently red check with a traceback for an explanation. Now a named, actionable failure. - **The drift alarm could not fire on the drift it was written for** — `docs/callers/cursor-review.md` was not in `test-cursor-review-scripts.yml`'s `paths:` filter, so a docs-only PR deleting the `blocking` row (the #31 scenario) ran no job. - **The header's caller example** set `blocking: true` over the label-only triggers and label-keyed concurrency group the new gotcha calls bricking; it now carries the blocking caller shape. Deferred, with the limit documented instead: the gate does not prove the *current* head SHA was reviewed. A thread stops counting once its hunk changes (`isOutdated`), so under a label-gated caller a cosmetic edit to the flagged lines can green the check with no re-review. Requiring a review at the head SHA is a caller-contract change — every label-gated blocking caller would go red on every push until it widened `types:` or enrolled auto-label — so it belongs behind its own product call, not in the restore. The setup guide now says so plainly and points at `cursor-review-auto-label.yml` / `run_without_label: true` as the close. - Un-regresses the docs everywhere #31's deletion left them behind: the workflow header + input description, `.github/cursor-review/README.md` (file inventory, pipeline note, a live "make the review blocking" section, knobs table), `docs/callers/cursor-review.md` (two-switch contract, `blocking` input row, a blocking-gate gotchas section covering the trigger shape, waiver semantics, and who can resolve), the top-level README catalog row, and the AGENTS.md worked example. - Adds `test_workflow_inputs_docs.py`: two-way set equality between `on.workflow_call.inputs` and both docs tables, so the next accidental input deletion fails CI loudly instead of surviving in the docs. - Updates the workflow-pins guard census (17 → 18): the gate job loads `gate-unresolved.py` from `workflows_ref` in its own job and carries its own empty-ref guard. - Adds `test_post_review_delivery.py`: every path on which `post-review.py` exits 0, asserted against what it now claims about delivery — so the gate's fail-closed property is pinned by a test rather than by a comment. ## Test plan - [x] CI suites green (cursor-review scripts, workflow-pins, agents-md-integrity) - [ ] After merge + fleet bump: confirm an existing caller (all on the default `blocking: false`) runs unchanged — gate job skipped, review still posts - [ ] Opt one low-traffic repo into `blocking: true` WITHOUT marking the check required; confirm the check goes red on unresolved threads and green after resolving the last one via a `pull_request_review_thread` event - [ ] Only then mark `<caller job id> / Blocking gate` required in that repo's ruleset - [x] New behavior is gated behind the `blocking` input, default off Fixes BE-4691 --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:48 +00:00