[PR #357] [MERGED] feat(cursor-review): approve_scope — gate auto-approve rounds 2+ on the delta since the last reviewed commit #357

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/357
Author: @mattmillerai
Created: 10/7/2026
Status: ✅ Merged
Merged: 10/7/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-19455-approve-scope-delta

📝 Commits (1)

d492b6a feat(cursor-review): approve_scope — gate auto-approve rounds 2+ on the delta since the last reviewed commit

📊 Changes

7 files changed (+494 additions, -17 deletions)

View changed files

📝 .github/cursor-review/README.md (+1 -0)
📝 .github/cursor-review/auto-approve.py (+223 -13)
📝 .github/cursor-review/post-review.py (+57 -3)
➕ .github/cursor-review/tests/fixtures/incremental-delta.patch (+12 -0)
📝 .github/cursor-review/tests/test_auto_approve.py (+154 -0)
📝 .github/workflows/cursor-review.yml (+46 -1)
📝 docs/callers/cursor-review.md (+1 -0)

📄 Description

ELI-5

Auto-approve used to block a fix round on any Medium the panel found, even in code the author never touched that round, so fix rounds never converged. Now, from round 2 on, only findings in this round's new changes, live re-raises of still-open threads, and High/Critical findings anywhere can block. Everything else is still posted, but labelled as not blocking.

What changed

New input approve_scope (delta default, or full). It only takes effect when approve_max_severity is set. It is passed to both post-review.py and auto-approve.py decide, so approve_gate (and with it defer_approval / cursor-approve) sees the same gate.
diff-size emits incremental_state: none / built / unavailable / discarded. "No block because it's round 1" used to be indistinguishable from "no block because the build failed" and from "empty block from a pure rebase".
auto-approve.py: resolve_scope() falls back to full when the state is not built, the block can't be read, or the ledger status isn't ok/empty. Round 1 (none) also runs as full. Under delta, gating_reason() counts an above-threshold finding only if it is:
- inside a new-side hunk of the verified block;
- a repeat_of pointing at an unresolved ledger thread;
- High or Critical, or of unrecognised severity, anywhere.
The decision note (a log line plus a _Scope:_ line in the review body) states the scope, the gating and non-gating counts, and why each finding gated.
post-review.py runs the same classifier and appends _Outside this round's changes: not blocking auto-approve._ plus a marker to non-gating threads. The marker is stripped from model text first, so a finding can't exempt itself.
Open-thread check / auto-resolve: under delta, a marked thread doesn't hold back the approval, and it doesn't block the other threads from resolving either. It is never resolved itself, so it stays open for a human. A full round counts marked threads as before.

Judgment calls

Under delta, an earlier round's thread that was marked non-gating doesn't block a later round's approval, unless it is re-raised. Without this, round 3 would block on round 2's non-blocking Medium and rounds still wouldn't converge. I read the ticket's empty-delta rule ("no earlier unresolved finding above the threshold") as applying to gating threads.
An unrecognised severity counts as severe, so it gates anywhere. This is the fail-closed choice.
A re-run at the same head (LAST_REVIEWED_SHA == HEAD_SHA) reads as none, so it runs as full.
Ledger disabled (the kill switch) also runs as full, because without the ledger there is no live-repeat data.

Residual

The opt-in blocking gate (gate-unresolved.py) still counts a non-gating Medium thread as unresolved. That is consistent with "left open for a human", but a blocking + approve_max_severity caller will see an approval and a red blocking check together until a human resolves the thread. Whether blocking should also honour approve_scope is a separate decision.
I didn't re-measure the convergence numbers the ticket quotes (round-by-round clean rate on a consumer repo). That needs live rounds after rollout.
The panel-completeness rule from the related "allow N errored reviewers" work is untouched. The empty-delta path still requires every panel cell ok.
Live repeats are matched against anchored ledger entries' discussion_url only. A re-raise whose ancestor was body-only (unanchored) has no thread, so it isn't a live repeat. Under delta it gates only if it falls in the delta or is severe.

Provenance

Authored by: agent-work loop
Verified:
- Tests: cursor-review suite green (976, including 15 new ApproveScopeTest cases covering every case the ticket lists, plus the marker-injection and constant-parity checks)
- workflow-pins suite green (551; input-docs drift test included)
- check_workflow_pins.py, check_agents_md.py, check-org-repo-literals.sh: pass
- cursor-review.yml parses as YAML
- Not run: a live cursor-review round on a real PR
Deviations: none from the acceptance list. See Judgment calls for interpretation.

_{🔄 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/357 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/7/2026 **Status:** ✅ Merged **Merged:** 10/7/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-19455-approve-scope-delta` --- ### 📝 Commits (1) - [`d492b6a`](https://github.com/Comfy-Org/github-workflows/commit/d492b6ab2d8fb673b58ba500598d1dcd64abbcfe) feat(cursor-review): approve_scope — gate auto-approve rounds 2+ on the delta since the last reviewed commit ### 📊 Changes **7 files changed** (+494 additions, -17 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+1 -0) 📝 `.github/cursor-review/auto-approve.py` (+223 -13) 📝 `.github/cursor-review/post-review.py` (+57 -3) ➕ `.github/cursor-review/tests/fixtures/incremental-delta.patch` (+12 -0) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+154 -0) 📝 `.github/workflows/cursor-review.yml` (+46 -1) 📝 `docs/callers/cursor-review.md` (+1 -0) </details> ### 📄 Description ## ELI-5 Auto-approve used to block a fix round on any Medium the panel found, even in code the author never touched that round, so fix rounds never converged. Now, from round 2 on, only findings in this round's new changes, live re-raises of still-open threads, and High/Critical findings anywhere can block. Everything else is still posted, but labelled as not blocking. ## What changed - **New input `approve_scope`** (`delta` default, or `full`). It only takes effect when `approve_max_severity` is set. It is passed to both `post-review.py` and `auto-approve.py decide`, so `approve_gate` (and with it `defer_approval` / cursor-approve) sees the same gate. - **`diff-size` emits `incremental_state`**: `none` / `built` / `unavailable` / `discarded`. "No block because it's round 1" used to be indistinguishable from "no block because the build failed" and from "empty block from a pure rebase". - **`auto-approve.py`**: `resolve_scope()` falls back to `full` when the state is not `built`, the block can't be read, or the ledger status isn't `ok`/`empty`. Round 1 (`none`) also runs as `full`. Under `delta`, `gating_reason()` counts an above-threshold finding only if it is: - inside a new-side hunk of the verified block; - a `repeat_of` pointing at an unresolved ledger thread; - High or Critical, or of unrecognised severity, anywhere. The decision note (a log line plus a `_Scope:_` line in the review body) states the scope, the gating and non-gating counts, and why each finding gated. - **`post-review.py`** runs the same classifier and appends `_Outside this round's changes: not blocking auto-approve._` plus a marker to non-gating threads. The marker is stripped from model text first, so a finding can't exempt itself. - **Open-thread check / auto-resolve**: under `delta`, a marked thread doesn't hold back the approval, and it doesn't block the other threads from resolving either. It is never resolved itself, so it stays open for a human. A `full` round counts marked threads as before. ## Judgment calls - Under `delta`, an earlier round's thread that was **marked non-gating** doesn't block a later round's approval, unless it is re-raised. Without this, round 3 would block on round 2's non-blocking Medium and rounds still wouldn't converge. I read the ticket's empty-delta rule ("no earlier unresolved finding above the threshold") as applying to gating threads. - An unrecognised severity counts as severe, so it gates anywhere. This is the fail-closed choice. - A re-run at the same head (`LAST_REVIEWED_SHA == HEAD_SHA`) reads as `none`, so it runs as `full`. - Ledger `disabled` (the kill switch) also runs as `full`, because without the ledger there is no live-repeat data. ## Residual - The opt-in `blocking` gate (`gate-unresolved.py`) still counts a non-gating Medium thread as unresolved. That is consistent with "left open for a human", but a `blocking` + `approve_max_severity` caller will see an approval and a red blocking check together until a human resolves the thread. Whether `blocking` should also honour `approve_scope` is a separate decision. - I didn't re-measure the convergence numbers the ticket quotes (round-by-round clean rate on a consumer repo). That needs live rounds after rollout. - The panel-completeness rule from the related "allow N errored reviewers" work is untouched. The empty-delta path still requires every panel cell `ok`. - Live repeats are matched against anchored ledger entries' `discussion_url` only. A re-raise whose ancestor was body-only (unanchored) has no thread, so it isn't a live repeat. Under `delta` it gates only if it falls in the delta or is severe. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: cursor-review suite green (976, including 15 new `ApproveScopeTest` cases covering every case the ticket lists, plus the marker-injection and constant-parity checks) - workflow-pins suite green (551; input-docs drift test included) - `check_workflow_pins.py`, `check_agents_md.py`, `check-org-repo-literals.sh`: pass - `cursor-review.yml` parses as YAML - Not run: a live cursor-review round on a real PR - **Deviations:** none from the acceptance list. See Judgment calls for interpretation. --- <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:58 +00:00