From da45649bed33a6fdbaa567ebd19508710914a5f0 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sat, 27 Jun 2026 03:19:25 -0700 Subject: [PATCH] feat(cursor-review): optional blocking gate for unresolved findings (BE-1891) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add an opt-in `blocking` input (boolean, default false) to the reusable cursor-review workflow. When false (default) the review stays advisory and nothing changes for any caller. When true, a final "Blocking gate" job queries the PR's review threads and fails the check while any cursor-review finding thread is unresolved, so a required-status-check config can block merge until findings are addressed. The gate (gate-unresolved.py) recognizes a cursor-review thread by its originating review's body marker — identity-independent, so it works whether the review posts as github-actions[bot] or a dedicated bot App. Outdated threads are ignored (a re-review re-posts anything still wrong as a fresh thread). It queries live thread state on every run, so it is idempotent: resolve threads, re-run, green. READMEs document the input and the required-status-check setup step. --- .github/cursor-review/README.md | 43 +++++++ .github/cursor-review/gate-unresolved.py | 156 +++++++++++++++++++++++ .github/workflows/cursor-review.yml | 58 +++++++++ README.md | 2 +- 4 files changed, 258 insertions(+), 1 deletion(-) create mode 100644 .github/cursor-review/gate-unresolved.py diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 7e44269..293ba93 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -84,6 +84,7 @@ silently vanishing — the review tells you what didn't run. | [`prompt-judge.md`](prompt-judge.md) | Prompt the judge model uses to consolidate panel findings into one review. | | [`extract-findings.py`](extract-findings.py) | Parses a cell's raw `cursor-agent` output into a normalized findings record. Always emits structured JSON — even on empty output or parse failure — so the consolidate step has uniform input. | | [`post-review.py`](post-review.py) | Reads the judge's consolidated findings and posts **one** PR review with line-anchored inline comments and severity badges. | +| [`gate-unresolved.py`](gate-unresolved.py) | The opt-in blocking gate (`blocking: true`). Queries the PR's review threads and exits non-zero while any cursor-review finding thread is unresolved. | | [`slack-notify.sh`](slack-notify.sh) | Sends the start/complete Slack DMs to the triggerer (no-ops without a token). | ## Adopt it in your repo @@ -142,6 +143,46 @@ an app token is required). The opt-in roster lives in the caller's `vars.CURSOR_REVIEW_OPTED_IN_LOGINS`. See that workflow's header for the full example and the `vars.APP_ID` / `CLOUD_CODE_BOT_PRIVATE_KEY` requirements. +### Optional: make the review blocking (merge gate) + +By default the review is **advisory** — it posts findings as PR review threads, +but an unresolved (red) review never blocks merge. Opt into a blocking gate by +passing `blocking: true`: + +```yaml +jobs: + cursor-review: + uses: Comfy-Org/github-workflows/.github/workflows/cursor-review.yml@ # v1 + with: + blocking: true + secrets: + CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} +``` + +With `blocking: true`, a final **Blocking gate** job queries the PR's review +threads and **fails the check while any cursor-review finding thread is +unresolved**. Resolve the thread(s) — or push a fix and re-trigger the review — +and the gate goes green. It is idempotent: resolving threads and re-running the +check turns it green without a fresh panel. + +> **This workflow cannot set branch protection.** A red check is visible but +> does not block merge on its own. To actually gate merges you must **also mark +> the `Blocking gate` check as a required status check** in the calling repo: +> *Settings → Branches → Branch protection rule* (or *Rulesets*) → **Require +> status checks to pass** → add **`Blocking gate`** (it appears once the +> workflow has run at least once with `blocking: true`). The check name is +> prefixed with your caller job id, e.g. `cursor-review / Blocking gate`. + +Notes: + +- `blocking: false` (the default, or simply not passing the input) is **exactly + today's behavior** — no caller changes until it opts in. +- A thread is recognized as a cursor-review thread by its originating review's + body marker, so the gate works whether the review posts as + `github-actions[bot]` or under a dedicated `bot_app_id`. +- Outdated threads (their code changed since the finding was posted) don't + block — a re-review re-posts anything still wrong as a fresh thread. + ## Configuration knobs All optional, with defaults — pass them under `with:` in the caller. Full @@ -154,6 +195,8 @@ descriptions live in the [workflow header](../workflows/cursor-review.yml). | `review_label` | `cursor-review` | Label whose addition triggers the review. | | `diff_excludes` | lockfiles, `node_modules`, `dist`, `vendor`, minified/generated files | Pathspecs excluded from both the size count and the reviewed diff. | | `workflows_ref` | `main` | Ref this directory's prompts/scripts are loaded from. Pin to your `uses:` SHA. | +| `bot_app_id` | `''` | Optional GitHub App ID; when set (with `BOT_APP_PRIVATE_KEY`), the review posts under that App's identity instead of `github-actions[bot]`. | +| `blocking` | `false` | Opt-in merge gate. `true` fails the **Blocking gate** check while any cursor-review finding thread is unresolved. See [Make the review blocking](#optional-make-the-review-blocking-merge-gate). | ### Escape hatches diff --git a/.github/cursor-review/gate-unresolved.py b/.github/cursor-review/gate-unresolved.py new file mode 100644 index 0000000..85f42ef --- /dev/null +++ b/.github/cursor-review/gate-unresolved.py @@ -0,0 +1,156 @@ +#!/usr/bin/env python3 +"""Blocking gate: fail when a PR has unresolved cursor-review finding threads. + +Used by cursor-review.yml when the caller opts in with `blocking: true`. Queries +the PR's review threads (GraphQL) and exits non-zero when any cursor-review +finding thread is still unresolved — turning the check red so a required-status- +check configuration blocks the merge until the threads are addressed. With +`blocking: false` (the default) this script is never run and the review stays +purely advisory. + +Identifying a cursor-review thread +---------------------------------- +A review thread counts as a cursor-review finding thread when its originating +review's body starts with ``CONSOLIDATED_MARKER`` — the exact discriminator the +workflow's own dup-check uses to recognize its consolidated reviews. This was +chosen over matching the posting identity (e.g. ``cloud-code-bot`` / +``github-actions[bot]``) on purpose: + +* It is identity-independent. The consolidated review posts as + ``github-actions[bot]`` by default, or under a dedicated bot App when the + caller sets ``bot_app_id`` — the gate must not have to resolve a bot login + that varies per caller. +* It is the canonical cursor-review signal already used elsewhere in the + workflow, so there is one source of truth for "is this our review?". + +Only the consolidated review that carries inline findings creates review +threads; the "no findings", error, and inline-anchoring-fallback reviews are +body-only and create none. So a thread existing here already means a real +finding was anchored to a line. + +Outdated threads +---------------- +Threads whose hunk has changed since the finding was posted (``isOutdated``) +are NOT counted. A re-review on a new commit re-posts anything still wrong as a +fresh, non-outdated thread, so blocking on outdated ones would force resolution +of superseded findings. Resolving the live threads — or pushing a fix and +re-triggering the review — is what turns the gate green, which makes it +idempotent on re-run. +""" + +import argparse +import json +import os +import subprocess +import sys + +CONSOLIDATED_MARKER = "## 🔍 Cursor Review — Consolidated panel" + +QUERY = """ +query($owner: String!, $name: String!, $pr: Int!, $cursor: String) { + repository(owner: $owner, name: $name) { + pullRequest(number: $pr) { + reviewThreads(first: 100, after: $cursor) { + pageInfo { hasNextPage endCursor } + nodes { + isResolved + isOutdated + comments(first: 1) { + nodes { + author { login } + pullRequestReview { body } + } + } + } + } + } + } +} +""" + + +def run_graphql(owner: str, name: str, pr: int, cursor): + """Run one page of the reviewThreads query via the gh CLI.""" + args = [ + "gh", "api", "graphql", + "-f", f"query={QUERY}", + "-f", f"owner={owner}", + "-f", f"name={name}", + "-F", f"pr={pr}", + ] + # gh's -F coerces the literal `null` to a JSON null, which GraphQL reads as + # "from the start"; subsequent pages pass the opaque string cursor via -f. + args += ["-F", "cursor=null"] if cursor is None else ["-f", f"cursor={cursor}"] + + result = subprocess.run(args, capture_output=True, text=True) + if result.returncode != 0: + # A query failure must not silently pass the gate — exit 2 (distinct + # from the "found unresolved" exit 1) so the check fails loudly. + print(f"GraphQL query failed: {result.stderr.strip()}", file=sys.stderr) + raise SystemExit(2) + return json.loads(result.stdout) + + +def is_cursor_thread(thread: dict) -> bool: + """True when the thread's originating review is a cursor-review consolidation.""" + comments = (thread.get("comments") or {}).get("nodes") or [] + if not comments: + return False + review = comments[0].get("pullRequestReview") or {} + body = review.get("body") or "" + return body.startswith(CONSOLIDATED_MARKER) + + +def collect_unresolved(owner: str, name: str, pr: int) -> int: + """Count live (non-outdated, unresolved) cursor-review finding threads.""" + count = 0 + cursor = None + while True: + data = run_graphql(owner, name, pr, cursor) + threads = data["data"]["repository"]["pullRequest"]["reviewThreads"] + for thread in threads["nodes"]: + if not is_cursor_thread(thread): + continue + if thread.get("isResolved") or thread.get("isOutdated"): + continue + count += 1 + page = threads["pageInfo"] + if not page["hasNextPage"]: + return count + cursor = page["endCursor"] + + +def emit(text: str) -> None: + print(text) + summary_path = os.environ.get("GITHUB_STEP_SUMMARY") + if summary_path: + with open(summary_path, "a", encoding="utf-8") as f: + f.write(text + "\n") + + +def main() -> None: + parser = argparse.ArgumentParser() + parser.add_argument("--repo", required=True, help="owner/name of the PR repo") + parser.add_argument("--pr-number", required=True, type=int) + args = parser.parse_args() + + owner, _, name = args.repo.partition("/") + if not owner or not name: + print(f"--repo must be owner/name, got {args.repo!r}", file=sys.stderr) + raise SystemExit(2) + + count = collect_unresolved(owner, name, args.pr_number) + + if count: + emit( + f"❌ **Blocking gate: {count} unresolved cursor-review finding " + f"thread(s).**\n\nResolve each open cursor-review thread on this PR " + "(or push a fix and re-trigger the review), then re-run this check." + ) + raise SystemExit(1) + + emit("✅ **Blocking gate: no unresolved cursor-review finding threads.**") + + +if __name__ == "__main__": + main() diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 5531a15..9ae9e8a 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -37,6 +37,11 @@ name: Cursor Review (reusable) # # a distinct, queryable identity instead of github-actions[bot]. Supply # # your App's id + private key (App IDs aren't secret, so id is an input). # bot_app_id: ${{ vars.REVIEW_BOT_APP_ID }} +# # Optional: make the review blocking. true → the "Blocking gate" check +# # goes red while any cursor-review finding thread is unresolved. To +# # actually block merge you must ALSO mark "Blocking gate" as a required +# # status check in this repo's branch-protection settings (see README). +# blocking: true # secrets: # CURSOR_API_KEY: ${{ secrets.CURSOR_API_KEY }} # SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }} @@ -107,6 +112,20 @@ on: type: string required: false default: '' + blocking: + description: >- + Opt-in merge gate. false (default) → the review is advisory, exactly as + before: it posts findings but never fails the check, so no caller's + behavior changes until it opts in. true → a final "Blocking gate" job + fails (red check) when the PR has unresolved cursor-review finding + threads, and passes once they are all resolved (idempotent on re-run). + NOTE: this workflow cannot set branch protection. To actually block the + merge you must ALSO mark the "Blocking gate" check as a required status + check in the caller repo's branch-protection / ruleset settings — until + you do, the red check is visible but non-blocking. See the README. + type: boolean + required: false + default: false secrets: CURSOR_API_KEY: description: Cursor API key for cursor-agent (the panel + judge models bill through it). @@ -633,6 +652,45 @@ jobs: --triggered-by "$TRIGGERED_BY" fi + blocking-gate: + # Opt-in merge gate (inputs.blocking). Fails the check when the PR has + # unresolved cursor-review finding threads, so a required-status-check + # config can block the merge until they're addressed. With blocking:false + # (default) this job is skipped entirely — the review stays advisory and + # the existing path is unchanged. + # + # Runs whenever the review is genuinely triggered (should_run), regardless + # of already_reviewed / within_cap: on a resolve→re-trigger of an unchanged + # commit the panel is skipped (already_reviewed) but this gate still + # re-queries the live thread state and goes green once the threads are + # resolved. `needs: consolidate` (with always()) orders it after the review + # is posted on a fresh run, and lets it run even when consolidate is skipped. + name: Blocking gate + needs: [gate, consolidate] + if: ${{ always() && inputs.blocking && needs.gate.outputs.should_run == 'true' }} + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read + steps: + - name: Load cursor-review assets + uses: actions/checkout@v6 + with: + repository: Comfy-Org/github-workflows + ref: ${{ inputs.workflows_ref }} + path: _cursor_review_assets + persist-credentials: false + + - name: Fail on unresolved cursor-review threads + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_NUMBER: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} + run: | + python3 "$CURSOR_REVIEW_ASSETS/gate-unresolved.py" \ + --repo "$REPO" \ + --pr-number "$PR_NUMBER" + notify-start: name: Notify start needs: [gate, diff-size] diff --git a/README.md b/README.md index 9f1f02d..b672600 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ This repo is **public** so any repo — public or private, inside or outside the | Workflow | Purpose | |---|---| | [`detect-unreviewed-merge.yml`](.github/workflows/detect-unreviewed-merge.yml) | SOC 2 compliance — detects PRs merged without prior approval and opens a tracking issue in [`Comfy-Org/unreviewed-merges`](https://github.com/Comfy-Org/unreviewed-merges). | -| [`cursor-review.yml`](.github/workflows/cursor-review.yml) | Label-triggered multi-model code review. A 4-lab × 2-review-type cursor-agent panel runs adversarial + edge-case passes, a judge model consolidates them into one PR review with per-finding severity badges, and the triggerer gets Slack start/complete DMs. Prompts and scripts live in [`.github/cursor-review/`](.github/cursor-review) — the single source of truth, so consumer repos carry only a thin caller. Requires `CURSOR_API_KEY` (+ optional `SLACK_BOT_TOKEN`). | +| [`cursor-review.yml`](.github/workflows/cursor-review.yml) | Label-triggered multi-model code review. A 4-lab × 2-review-type cursor-agent panel runs adversarial + edge-case passes, a judge model consolidates them into one PR review with per-finding severity badges, and the triggerer gets Slack start/complete DMs. Advisory by default; opt in with `blocking: true` to fail a (required-status-check) gate while findings stay unresolved. Prompts and scripts live in [`.github/cursor-review/`](.github/cursor-review) — the single source of truth, so consumer repos carry only a thin caller. Requires `CURSOR_API_KEY` (+ optional `SLACK_BOT_TOKEN`). | | [`cursor-review-auto-label.yml`](.github/workflows/cursor-review-auto-label.yml) | Companion to `cursor-review.yml`. On PR assignment, applies the review label for an opted-in reviewer (via the CLOUD_CODE_BOT app token, so the label actually triggers the review). The opt-in roster lives in the caller's `vars.CURSOR_REVIEW_OPTED_IN_LOGINS` — no roster is baked into the workflow. Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`. | ## Usage