[PR #345] [MERGED] feat(cursor-review): approve_gate output and per-PR max_rounds cap #347

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-19321-cursor-review-approve-gate

📝 Commits (5)

c6accd4 feat(cursor-review): approve_gate output and per-PR max_rounds cap
799ade1 Merge branch 'main' into matt/be-19321-cursor-review-approve-gate
8df69ab fix(cursor-review): round cap fails open on a label it cannot apply, and counts only real rounds
8a1b67f fix(cursor-review): a failed review POST withdraws, and the blocking gate reads the cap label live
b155655 feat(cursor-review): expose round and max_rounds workflow outputs

📊 Changes

8 files changed (+1145 additions, -31 deletions)

View changed files

📝 .github/cursor-review/README.md (+2 -1)
📝 .github/cursor-review/auto-approve.py (+406 -19)
📝 .github/cursor-review/tests/test_auto_approve.py (+444 -0)
📝 .github/cursor-review/tests/test_workflow_job_isolation.py (+1 -1)
📝 .github/cursor-review/tests/test_workflow_ref_guard_parity.py (+1 -1)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+5 -2)
📝 .github/workflows/cursor-review.yml (+222 -6)
📝 docs/callers/cursor-review.md (+64 -1)

📄 Description

ELI-5

The cursor-review bot reviews a PR in rounds. This PR adds two hooks to it. First, a new output says whether the latest round passed the severity gate, so a later workflow can wait for a pass before it runs. Second, a cap on rounds: once a PR has had max_rounds rounds (default 5), the bot stops reviewing it, adds the needs-human-review label, and posts one comment listing what is still open. Removing the label lets the bot start counting again.

What changed

approve_gate output (on.workflow_call.outputs). It always takes exactly one of five values:

Value	When
`pass`	`decide()` returned APPROVE
`fail`	REQUEST_CHANGES, or an open thread above the threshold withheld approval
`untrusted`	judge degraded / a panel cell failed / not delivered / head moved (including a move during the POST) / PR unreadable / the review POST failed. Also used when this run delivered no round at all, e.g. an unrelated event, an already-reviewed head, or an over-cap diff
`capped`	the round cap was hit this run, or the PR carries `needs-human-review` (while `max_rounds` or `approve_max_severity` is set)
`off`	`approve_max_severity` is empty (and the cap was not hit)

auto-approve.py: the new decide_gate() returns (event, gate, reasons, blocking). decide() is now a thin wrapper with the same 3-tuple, so existing callers and tests are unchanged. cmd_decide writes approve_gate to $GITHUB_OUTPUT before any check can fail, then overwrites it with the decision. If the step dies early, the value therefore reads untrusted.
post-review gets job output approve_gate from the Auto-approve decision step, which now has id: approve. The workflow-level expression adds capped, off and the no-round untrusted.

round and max_rounds outputs (scope addition requested on the PR, for cursor-approve's "round R of M" status card). round is the 1-based number of the round this run delivered, counted the same way the cap counts. When this run hit the cap it is the last round instead. It is empty when no round landed, the count could not be read, or max_rounds is 0. max_rounds is the effective cap: 0 for no cap, including a value the cap step rejects. round-cap writes next_round and the applied cap, and round uses next_round only when post-review reports delivered. A delivered review is exactly what the counter counts, so a "Review failed" or all-cells-failed review gets no number.

max_rounds input (number, default 5, 0 = no cap):

New job round-cap (needs: gate, same if: as ledger). It checks out no PR code, holds pull-requests: write, runs on ubuntu-latest, and uses a byte-identical copy of the workflows_ref guard. It counts reviews whose body starts with post-review.py's CONSOLIDATED_MARKER and whose author is the posting identity (<app-slug>[bot] or github-actions[bot]). Rounds that reviewed nothing (the "Review failed" error review and the all-cells-failed review) do not count. Only reviews after the most recent unlabeled timeline event for needs-human-review count. At or over the cap it:
- adds needs-human-review, creating the label if it is missing (the create is best-effort: it needs issues: write, so a failed create still attempts the add);
- posts one comment listing the latest round's open findings above approve_max_severity (every open finding when that is empty). The comment is keyed on  from the poster after the last reset, so a re-trigger does not post it twice for the same cap;
- outputs capped=true (and labelled=true, which is also set when the PR already carries the label).
diff-size now needs: round-cap and skips when capped == 'true'. Every job that spends money hangs off diff-size, so a capped PR runs no panel.
If the count cannot be read, max_rounds is not a finite whole number, or the label cannot be applied, the cap fails open: the panel runs, as it did before this change. The label goes on before the PR is committed to capped, because it is the only reset. If the comment write fails after the label landed, the PR stays capped and the job goes red so someone sees it.
The round-cap job's App-token mint uses the same BOT_CONFIGURED guard as over-cap-comment and is continue-on-error, so an app id without a key degrades to github-actions[bot] instead of skipping the panel.
If the review POST fails (other than GitHub refusing a self-approval), approve_gate becomes untrusted and the bot's earlier approvals are withdrawn, so a REQUEST_CHANGES that was lost cannot leave the last round's approval standing.
dismiss-stale treats a PR read that has a head but no base as "no base", so it skips the base check instead of dismissing every approval.
decide() refuses to approve when the PR carries needs-human-review. It returns NONE, which also withdraws earlier bot approvals, and the gate value is capped. The post-write re-read also checks the label, so an APPROVE that races a label being applied is dismissed.
Blocking gate now needs: round-cap and holds red on a capped head, with its own message. This matches how the over-cap path is treated: no panel looked at that head. When max_rounds > 0 it also reads the PR's live labels on every event and fails while needs-human-review is present, so a thread-resolve event (which skips round-cap) cannot turn a capped head green. A failed label read fails closed.

Docs: new max_rounds row in docs/callers/cursor-review.md and in the README knob table, plus a "Round cap and the approve_gate output" section with a consumer example and a table for the round / max_rounds outputs. Test counts bumped: EXPECTED_GUARDS 9→10, workflow-pins guarded sites 19→20, and round-cap added to the isolation test's bot-key holders.

Judgment calls

Default 5 vs. "unchanged when unset" (please decide). The spec says max_rounds defaults to 5. The acceptance criteria say that with max_rounds unset, behaviour is unchanged apart from the new output. Those two conflict: with a default of 5, a PR's 6th round on any existing caller now gets capped. I followed the explicit default, kept the cap fail-open on any read error, and left everything else untouched. If you want existing callers unchanged, it is a one-line change to default: 0 plus the two doc rows.
capped wins over off. The cap applies whether or not auto-approve is on, so a capped run reports capped even when approve_max_severity is empty.
No-round runs report untrusted, so the output is never empty. "No round was delivered" is the closest of the five values.
Dismissed consolidated reviews still count as rounds. The round was spent either way. Only the poster's own reviews count, so a review by someone else that carries the marker cannot use up a PR's rounds.
"Still above the threshold" means open findings. The latest round's inline findings are filtered to threads that are still unresolved and not outdated. If the thread query fails, nothing is filtered: listing too much is better than hiding a finding.
Label permissions: creating and applying the label uses the poster token, so no new permission scope is requested. Requesting issues: write would have broken callers that grant only pull-requests: write. Creating a repo label does need issues: write, so the docs tell callers to pre-create needs-human-review unless the bot App has Issues write. Without it the cap fails open rather than capping a PR it cannot label.
Label name is fixed at needs-human-review, as specified. Making it an input was raised in review and left to the owner.

Residual

Not exercised against live GitHub. The label create/apply, the issue-timeline unlabeled read, and the issue-comment post are covered only by unit tests with gh stubbed by endpoint. Applying a label to a PR with pull-requests: write has not been confirmed against a real GITHUB_TOKEN or App token. If it turns out not to be enough, the cap fails open (the panel keeps running) and the Round cap job logs a warning.
max_rounds default conflict (see Judgment calls). The maintainer has to choose between 5 and 0.
The cursor-approve.yml consumer is out of scope and does not exist yet, so nothing reads approve_gate end to end.
The design doc is unexercised. The ticket links an external design document I could not open, so this was built from the ticket text alone.
actionlint: the local v1.7.12 reports the same classes of issue on origin/main (job.workflow_sha unknown to this version, and SC2018/SC2019 info in the guard). After normalising, the only difference is one extra instance of each, all from the byte-identical guard copy. There are no new findings. CI does not run actionlint.

Provenance

Authored by: agent-work loop
Verified:
- Tests: cursor-review suite (830) and workflow-pins suite are green at the current head. Coverage includes every approve_gate value, a failed POST giving untrusted and withdrawing an earlier approval, a refused self-approval keeping its gate, and a base-less PR read not dismissing anything.
- The round counter and cap path are covered: marker, author, unlabel reset, rounds that reviewed nothing, the label probe/create fallbacks, fail-open on a read, label or parse failure, and red on a failed comment. The round / max_rounds outputs are covered under the cap, at the cap, after a reset, with the cap off or invalid, and on an unreadable count, and their workflow expressions are pinned.
- Lints: check_workflow_pins.py, check_agents_md.py and the org-repo-literal lint are clean. cursor-review.yml parses as YAML.
- Not run: a live workflow run against a real PR, including the Blocking gate's new label-read step. actionlint is not installed on the verifying host.
Deviations: acceptance criterion 2 ("unchanged when unset") is only met if the max_rounds default becomes 0. The spec's default of 5 ships (see Judgment calls), and the maintainer still has to decide.

_{🔄 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/345 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/6/2026 **Status:** ✅ Merged **Merged:** 10/6/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-19321-cursor-review-approve-gate` --- ### 📝 Commits (5) - [`c6accd4`](https://github.com/Comfy-Org/github-workflows/commit/c6accd4047a24b0d86df9676d16d910c56b03bec) feat(cursor-review): approve_gate output and per-PR max_rounds cap - [`799ade1`](https://github.com/Comfy-Org/github-workflows/commit/799ade1538ab06c47755fd6d2c23ede123b0d524) Merge branch 'main' into matt/be-19321-cursor-review-approve-gate - [`8df69ab`](https://github.com/Comfy-Org/github-workflows/commit/8df69abc9362bca9fe03baa8c6c2c6ff7f91d613) fix(cursor-review): round cap fails open on a label it cannot apply, and counts only real rounds - [`8a1b67f`](https://github.com/Comfy-Org/github-workflows/commit/8a1b67f47a80a70b2d061a35776187d9d63399c6) fix(cursor-review): a failed review POST withdraws, and the blocking gate reads the cap label live - [`b155655`](https://github.com/Comfy-Org/github-workflows/commit/b155655441a8866bdf2e04923cee3177b797b5fd) feat(cursor-review): expose round and max_rounds workflow outputs ### 📊 Changes **8 files changed** (+1145 additions, -31 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+2 -1) 📝 `.github/cursor-review/auto-approve.py` (+406 -19) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+444 -0) 📝 `.github/cursor-review/tests/test_workflow_job_isolation.py` (+1 -1) 📝 `.github/cursor-review/tests/test_workflow_ref_guard_parity.py` (+1 -1) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+5 -2) 📝 `.github/workflows/cursor-review.yml` (+222 -6) 📝 `docs/callers/cursor-review.md` (+64 -1) </details> ### 📄 Description ## ELI-5 The cursor-review bot reviews a PR in rounds. This PR adds two hooks to it. First, a new output says whether the latest round passed the severity gate, so a later workflow can wait for a pass before it runs. Second, a cap on rounds: once a PR has had `max_rounds` rounds (default 5), the bot stops reviewing it, adds the `needs-human-review` label, and posts one comment listing what is still open. Removing the label lets the bot start counting again. ## What changed **`approve_gate` output** (`on.workflow_call.outputs`). It always takes exactly one of five values: | Value | When | |---|---| | `pass` | `decide()` returned APPROVE | | `fail` | REQUEST_CHANGES, or an open thread above the threshold withheld approval | | `untrusted` | judge degraded / a panel cell failed / not delivered / head moved (including a move during the POST) / PR unreadable / the review POST failed. Also used when this run delivered no round at all, e.g. an unrelated event, an already-reviewed head, or an over-cap diff | | `capped` | the round cap was hit this run, or the PR carries `needs-human-review` (while `max_rounds` or `approve_max_severity` is set) | | `off` | `approve_max_severity` is empty (and the cap was not hit) | - `auto-approve.py`: the new `decide_gate()` returns `(event, gate, reasons, blocking)`. `decide()` is now a thin wrapper with the same 3-tuple, so existing callers and tests are unchanged. `cmd_decide` writes `approve_gate` to `$GITHUB_OUTPUT` before any check can fail, then overwrites it with the decision. If the step dies early, the value therefore reads `untrusted`. - `post-review` gets job output `approve_gate` from the `Auto-approve decision` step, which now has `id: approve`. The workflow-level expression adds `capped`, `off` and the no-round `untrusted`. **`round` and `max_rounds` outputs** (scope addition requested on the PR, for cursor-approve's "round R of M" status card). `round` is the 1-based number of the round this run delivered, counted the same way the cap counts. When this run hit the cap it is the last round instead. It is empty when no round landed, the count could not be read, or `max_rounds` is 0. `max_rounds` is the effective cap: `0` for no cap, including a value the cap step rejects. `round-cap` writes `next_round` and the applied cap, and `round` uses `next_round` only when `post-review` reports `delivered`. A delivered review is exactly what the counter counts, so a "Review failed" or all-cells-failed review gets no number. **`max_rounds` input** (number, default `5`, `0` = no cap): - New job **`round-cap`** (`needs: gate`, same `if:` as `ledger`). It checks out no PR code, holds `pull-requests: write`, runs on `ubuntu-latest`, and uses a byte-identical copy of the `workflows_ref` guard. It counts reviews whose body starts with `post-review.py`'s `CONSOLIDATED_MARKER` and whose author is the posting identity (`<app-slug>[bot]` or `github-actions[bot]`). Rounds that reviewed nothing (the "Review failed" error review and the all-cells-failed review) do not count. Only reviews after the most recent `unlabeled` timeline event for `needs-human-review` count. At or over the cap it: - adds `needs-human-review`, creating the label if it is missing (the create is best-effort: it needs `issues: write`, so a failed create still attempts the add); - posts one comment listing the latest round's open findings above `approve_max_severity` (every open finding when that is empty). The comment is keyed on `` from the poster after the last reset, so a re-trigger does not post it twice for the same cap; - outputs `capped=true` (and `labelled=true`, which is also set when the PR already carries the label). - `diff-size` now `needs: round-cap` and skips when `capped == 'true'`. Every job that spends money hangs off `diff-size`, so a capped PR runs no panel. - If the count cannot be read, `max_rounds` is not a finite whole number, or the label cannot be applied, the cap fails **open**: the panel runs, as it did before this change. The label goes on *before* the PR is committed to `capped`, because it is the only reset. If the comment write fails after the label landed, the PR stays capped and the job goes red so someone sees it. - The round-cap job's App-token mint uses the same `BOT_CONFIGURED` guard as `over-cap-comment` and is `continue-on-error`, so an app id without a key degrades to `github-actions[bot]` instead of skipping the panel. - If the review POST fails (other than GitHub refusing a self-approval), `approve_gate` becomes `untrusted` and the bot's earlier approvals are withdrawn, so a REQUEST_CHANGES that was lost cannot leave the last round's approval standing. - `dismiss-stale` treats a PR read that has a head but no base as "no base", so it skips the base check instead of dismissing every approval. - `decide()` refuses to approve when the PR carries `needs-human-review`. It returns NONE, which also withdraws earlier bot approvals, and the gate value is `capped`. The post-write re-read also checks the label, so an APPROVE that races a label being applied is dismissed. - **Blocking gate** now `needs: round-cap` and holds red on a capped head, with its own message. This matches how the over-cap path is treated: no panel looked at that head. When `max_rounds > 0` it also reads the PR's live labels on every event and fails while `needs-human-review` is present, so a thread-resolve event (which skips `round-cap`) cannot turn a capped head green. A failed label read fails closed. Docs: new `max_rounds` row in `docs/callers/cursor-review.md` and in the README knob table, plus a "Round cap and the `approve_gate` output" section with a consumer example and a table for the `round` / `max_rounds` outputs. Test counts bumped: `EXPECTED_GUARDS` 9→10, workflow-pins guarded sites 19→20, and `round-cap` added to the isolation test's bot-key holders. ## Judgment calls - **Default `5` vs. "unchanged when unset" (please decide).** The spec says `max_rounds` defaults to 5. The acceptance criteria say that with `max_rounds` unset, behaviour is unchanged apart from the new output. Those two conflict: with a default of 5, a PR's 6th round on any existing caller now gets capped. I followed the explicit default, kept the cap fail-open on any read error, and left everything else untouched. If you want existing callers unchanged, it is a one-line change to `default: 0` plus the two doc rows. - **`capped` wins over `off`.** The cap applies whether or not auto-approve is on, so a capped run reports `capped` even when `approve_max_severity` is empty. - **No-round runs report `untrusted`**, so the output is never empty. "No round was delivered" is the closest of the five values. - **Dismissed consolidated reviews still count as rounds.** The round was spent either way. Only the poster's own reviews count, so a review by someone else that carries the marker cannot use up a PR's rounds. - **"Still above the threshold" means open findings.** The latest round's inline findings are filtered to threads that are still unresolved and not outdated. If the thread query fails, nothing is filtered: listing too much is better than hiding a finding. - **Label permissions:** creating and applying the label uses the poster token, so no new permission scope is requested. Requesting `issues: write` would have broken callers that grant only `pull-requests: write`. Creating a repo label does need `issues: write`, so the docs tell callers to pre-create `needs-human-review` unless the bot App has Issues write. Without it the cap fails open rather than capping a PR it cannot label. - **Label name is fixed** at `needs-human-review`, as specified. Making it an input was raised in review and left to the owner. ## Residual - **Not exercised against live GitHub.** The label create/apply, the issue-timeline `unlabeled` read, and the issue-comment post are covered only by unit tests with `gh` stubbed by endpoint. Applying a label to a PR with `pull-requests: write` has not been confirmed against a real `GITHUB_TOKEN` or App token. If it turns out not to be enough, the cap fails open (the panel keeps running) and the `Round cap` job logs a warning. - **`max_rounds` default conflict** (see Judgment calls). The maintainer has to choose between 5 and 0. - **The `cursor-approve.yml` consumer is out of scope** and does not exist yet, so nothing reads `approve_gate` end to end. - **The design doc is unexercised.** The ticket links an external design document I could not open, so this was built from the ticket text alone. - **actionlint:** the local v1.7.12 reports the same classes of issue on `origin/main` (`job.workflow_sha` unknown to this version, and SC2018/SC2019 info in the guard). After normalising, the only difference is one extra instance of each, all from the byte-identical guard copy. There are no new findings. CI does not run actionlint. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: cursor-review suite (830) and workflow-pins suite are green at the current head. Coverage includes every `approve_gate` value, a failed POST giving `untrusted` and withdrawing an earlier approval, a refused self-approval keeping its gate, and a base-less PR read not dismissing anything. - The round counter and cap path are covered: marker, author, unlabel reset, rounds that reviewed nothing, the label probe/create fallbacks, fail-open on a read, label or parse failure, and red on a failed comment. The `round` / `max_rounds` outputs are covered under the cap, at the cap, after a reset, with the cap off or invalid, and on an unreadable count, and their workflow expressions are pinned. - Lints: `check_workflow_pins.py`, `check_agents_md.py` and the org-repo-literal lint are clean. `cursor-review.yml` parses as YAML. - Not run: a live workflow run against a real PR, including the Blocking gate's new label-read step. actionlint is not installed on the verifying host. - **Deviations:** acceptance criterion 2 ("unchanged when unset") is only met if the `max_rounds` default becomes 0. The spec's default of 5 ships (see Judgment calls), and the maintainer still has to decide. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

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

bsakaguchi referenced this issue

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

[PR #346] [MERGED] fix(cursor-review): announce a degraded live-head read in dismiss-stale #348

bsakaguchi closed this issue

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