[PR #409] [MERGED] feat(cursor-review): auto-retry a round the panel broke, with an announced delay #410

New issue

Closed

opened 2026-10-26 22:39:29 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:39:29 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/cursor-review-auto-retry-errored-rounds

📝 Commits (2)

fb6f081 feat(cursor-review): auto-retry a round the panel broke, with an announced delay
b9ade28 fix(cursor-review): address panel findings on the errored-round auto-retry

📊 Changes

8 files changed (+1077 additions, -47 deletions)

View changed files

📝 .github/cursor-approve/card.py (+33 -2)
📝 .github/cursor-review/README.md (+2 -0)
📝 .github/cursor-review/auto-approve.py (+123 -23)
📝 .github/cursor-review/tests/test_auto_approve.py (+121 -5)
➕ .github/cursor-review/tests/test_auto_retry_errored.py (+552 -0)
📝 .github/workflows/cursor-review.yml (+152 -12)
📝 docs/callers/cursor-approve.md (+12 -5)
📝 docs/callers/cursor-review.md (+82 -0)

📄 Description

Follow-up to #407. That PR made it possible to re-run a round on the same commit; this makes it automatic for the one cause that needs it most.

Why

A round can end with no decision for a reason that has nothing to do with the code — overwhelmingly reviewer cells erroring on provider capacity or a cell timeout. Re-running it is the entire fix, and until #407 there was no way to:

the gate dedupes on the consolidated review for the current HEAD SHA
that review is COMMENTED, which GitHub will not dismiss
the diff is fine, so there is nothing to push

So a whole panel was spent and a human had to notice the card and relabel by hand.

What

Two new inputs, both off by default:

input	default	what it does
`auto_retry_errored_rounds`	`false`	extends the BE-19526 moved-head retry to the errored-panel cause
`auto_retry_delay_minutes`	`0`	waits before relabelling — capacity failures need time to clear

The card and the standing block name the wait before it starts (in about 5 minutes). That is the point of the delay being visible: an author who reads "re-run automatically" and then sees nothing happen for five minutes re-runs it by hand, which is the work this removes.

What bounds it

All pre-existing except the last:

one retry per PR, counted off decide's own marked block, shared with the moved-head cause rather than a second budget
max_rounds — a round already at the cap is not retried into it
a token that can actually relabel (a GITHUB_TOKEN-applied label fires no run)
no standing block, no retry — nothing would count it
never a structural cause or findings above the threshold: the next round would hit the same wall. On an unchanged head that includes findings from the cells that did complete, and open threads above the threshold (blocking_without_trust). Mixed causes follow the strictest one.
after the wait, the job re-reads the PR and skips a closed PR or one labelled needs-human-review

The part worth reviewing closely

The gate now has two force signals and they are deliberately separate. The human one (rereview_command) scans PR comments and authorizes by association. The retry posts no comment. Its signal is the marked block decide writes, and the gate accepts it only when it:

was authored by a Bot (the App identity) or by the identity that applied the label (the APPROVER_TOKEN PAT, which the auto-retry job relabels as)
has decide's own shape: the auto-approve marker first, and the retry marker on a line of its own
is not dismissed
is pinned to the commit
is newer, by review id, than the newest consolidated review The comment query stays guarded on rereview_command alone, so enabling auto-retry cannot widen who may force a round by commenting; test_the_retry_cannot_use_the_human_comment_path pins that by reading the actual guard line.

The gate hardcodes the marker literals because it runs before any asset is loaded. A test pins them equal to auto_approve.AUTO_RETRY_MARKER / APPROVE_MARKER, and another pins card.py's clamp equal to the workflow's.

A real bug found while wiring this

The relabel job budgeted timeout-minutes: 5. Any wait past that would have killed the round the card had already promised — the one failure mode where announcing a retry is worse than not retrying. Now 35 (5 + the 30 the delay clamps to), pinned by a test that fails if the budget drops below the cap. The job is also no longer named Re-run the round (head moved), since it now serves two causes.

Tests

New cases in test_auto_retry_errored.py:

the eligibility truth table, including every False a retry cannot fix
the gate's real jq filter, extracted from the workflow and run against the real render_standing_body output and against these payload shapes: older id, same-second block, other commit, PAT identity that relabelled, other human, dismissed, quoted marker, two marked, null body/id, empty list
the delay clamp, run through bash with sleep stubbed: 0 → none, 5 → 300s, 31/1000/23-digit → 1800s, abc/-5/1.5 → none
the predicate matched against a reason generated by panel_gate, not a hand-written string, so producer and matcher cannot drift
constant and cap parity, and the job-timeout floor

Plus behavioural cases in AutoRetryTest that drive the real cmd_decide and cover the card marker, already-blocking findings and threads, and the moved-head exemption. There are also cmd_auto_retry cases for a PR that is closed, labelled for a human, or unreadable.

Enabling it

Nothing changes for any existing caller — both inputs default off, and the zero-delay card sentence is byte-identical to today's. Turning it on for cloud is a separate PR there.

Provenance

Verified:

cursor-review suite: 1525 tests OK (local, head b9ade28)
cursor-approve suite: 236 OK; workflow-pins suite: 551 OK (43 skipped)
check_workflow_pins.py, check-org-repo-literals.sh and check_agents_md.py pass; cursor-review.yml parses as YAML
CI on b9ade28 was still pending at the time of this update

Deviations:

Review round 1 (10 panel findings) addressed in b9ade28. The gate identity check now accepts a Bot author or the label sender, instead of keying on the approver login, because the gate holds no token to resolve that login.
Under APPROVER_TOKEN, a hand relabel on the same commit is still deduped. This is documented, and rereview_command is the hand path.

🤖 Generated with Claude Code

_{🔄 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/409 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/11/2026 **Status:** ✅ Merged **Merged:** 10/11/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/cursor-review-auto-retry-errored-rounds` --- ### 📝 Commits (2) - [`fb6f081`](https://github.com/Comfy-Org/github-workflows/commit/fb6f081256e8cea620c45b4ef6f2a800fc17c595) feat(cursor-review): auto-retry a round the panel broke, with an announced delay - [`b9ade28`](https://github.com/Comfy-Org/github-workflows/commit/b9ade2832d53c0fb142e93c1be853aabe9e4231b) fix(cursor-review): address panel findings on the errored-round auto-retry ### 📊 Changes **8 files changed** (+1077 additions, -47 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-approve/card.py` (+33 -2) 📝 `.github/cursor-review/README.md` (+2 -0) 📝 `.github/cursor-review/auto-approve.py` (+123 -23) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+121 -5) ➕ `.github/cursor-review/tests/test_auto_retry_errored.py` (+552 -0) 📝 `.github/workflows/cursor-review.yml` (+152 -12) 📝 `docs/callers/cursor-approve.md` (+12 -5) 📝 `docs/callers/cursor-review.md` (+82 -0) </details> ### 📄 Description Follow-up to #407. That PR made it *possible* to re-run a round on the same commit; this makes it automatic for the one cause that needs it most. ## Why A round can end with no decision for a reason that has nothing to do with the code — overwhelmingly reviewer cells erroring on provider capacity or a cell timeout. Re-running it is the entire fix, and until #407 there was no way to: - the gate dedupes on the consolidated review for the current HEAD SHA - that review is `COMMENTED`, which GitHub will not dismiss - the diff is fine, so there is nothing to push So a whole panel was spent and a human had to notice the card and relabel by hand. ## What Two new inputs, both off by default: | input | default | what it does | | --- | --- | --- | | `auto_retry_errored_rounds` | `false` | extends the BE-19526 moved-head retry to the errored-panel cause | | `auto_retry_delay_minutes` | `0` | waits before relabelling — capacity failures need time to clear | The card and the standing block **name the wait before it starts** (`in about 5 minutes`). That is the point of the delay being visible: an author who reads "re-run automatically" and then sees nothing happen for five minutes re-runs it by hand, which is the work this removes. ## What bounds it All pre-existing except the last: - **one retry per PR**, counted off `decide`'s own marked block, **shared** with the moved-head cause rather than a second budget - `max_rounds` — a round already at the cap is not retried into it - a token that can actually relabel (a `GITHUB_TOKEN`-applied label fires no run) - no standing block, no retry — nothing would count it - **never** a structural cause or findings above the threshold: the next round would hit the same wall. On an unchanged head that includes findings from the cells that *did* complete, and open threads above the threshold (`blocking_without_trust`). Mixed causes follow the strictest one. - after the wait, the job re-reads the PR and skips a closed PR or one labelled `needs-human-review` ## The part worth reviewing closely The gate now has **two** force signals and they are deliberately separate. The human one (`rereview_command`) scans PR comments and authorizes by association. The retry posts no comment. Its signal is the marked block `decide` writes, and the gate accepts it only when it: - was authored by a Bot (the App identity) or by the identity that applied the label (the `APPROVER_TOKEN` PAT, which the `auto-retry` job relabels as) - has `decide`'s own shape: the auto-approve marker first, and the retry marker on a line of its own - is not dismissed - is pinned to the commit - is newer, by review id, than the newest consolidated review The comment query stays guarded on `rereview_command` alone, so enabling auto-retry cannot widen who may force a round by commenting; `test_the_retry_cannot_use_the_human_comment_path` pins that by reading the actual guard line. The gate hardcodes the marker literals because it runs before any asset is loaded. A test pins them equal to `auto_approve.AUTO_RETRY_MARKER` / `APPROVE_MARKER`, and another pins `card.py`'s clamp equal to the workflow's. ## A real bug found while wiring this The relabel job budgeted `timeout-minutes: 5`. Any wait past that would have killed the round the card had **already promised** — the one failure mode where announcing a retry is worse than not retrying. Now 35 (5 + the 30 the delay clamps to), pinned by a test that fails if the budget drops below the cap. The job is also no longer named `Re-run the round (head moved)`, since it now serves two causes. ## Tests New cases in `test_auto_retry_errored.py`: - the eligibility truth table, including every `False` a retry cannot fix - the gate's **real jq filter**, extracted from the workflow and run against the real `render_standing_body` output and against these payload shapes: older id, same-second block, other commit, PAT identity that relabelled, other human, dismissed, quoted marker, two marked, null body/id, empty list - the delay clamp, **run through bash** with `sleep` stubbed: `0 → none`, `5 → 300s`, `31`/`1000`/23-digit → `1800s`, `abc`/`-5`/`1.5` → none - the predicate matched against a reason **generated by `panel_gate`**, not a hand-written string, so producer and matcher cannot drift - constant and cap parity, and the job-timeout floor Plus behavioural cases in `AutoRetryTest` that drive the real `cmd_decide` and cover the card marker, already-blocking findings and threads, and the moved-head exemption. There are also `cmd_auto_retry` cases for a PR that is closed, labelled for a human, or unreadable. ## Enabling it Nothing changes for any existing caller — both inputs default off, and the zero-delay card sentence is byte-identical to today's. Turning it on for cloud is a separate PR there. ## Provenance **Verified:** - cursor-review suite: 1525 tests OK (local, head `b9ade28`) - cursor-approve suite: 236 OK; workflow-pins suite: 551 OK (43 skipped) - `check_workflow_pins.py`, `check-org-repo-literals.sh` and `check_agents_md.py` pass; `cursor-review.yml` parses as YAML - CI on `b9ade28` was still pending at the time of this update **Deviations:** - Review round 1 (10 panel findings) addressed in `b9ade28`. The gate identity check now accepts a Bot author *or* the label sender, instead of keying on the approver login, because the gate holds no token to resolve that login. - Under `APPROVER_TOKEN`, a *hand* relabel on the same commit is still deduped. This is documented, and `rereview_command` is the hand path. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:39:29 +00:00