[PR #416] [MERGED] feat(cursor-review): mark the card stale when a push dismisses the approval, and opt-in start the next round #418

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-20262-stale-card-auto-rereview

📝 Commits (6)

50de881 feat(cursor-review): mark the card stale when a push dismisses the approval, and opt-in start the next round
16a735e fix(cursor-review): promise an automatic round only where auto-rereview accepts the token
b7b8f70 feat(cursor-approve): name the automatic round's UTC start time, not "in about N minutes"
6ca27c2 fix(cursor-review): keep the stale card honest when the approval is withdrawn on its own head or the relabel fails
8597801 fix(cursor-review): serialise auto-rereview waiters, cut off at the approval, skip reviewed heads
5a30af9 docs(cursor-approve): a review-thread run can cancel the auto-rereview wait without a replacement

📊 Changes

12 files changed (+1314 additions, -49 deletions)

View changed files

📝 .github/cursor-approve/card.py (+184 -7)
📝 .github/cursor-review/README.md (+3 -1)
📝 .github/cursor-review/auto-approve.py (+339 -19)
📝 .github/cursor-review/tests/test_auto_approve.py (+2 -2)
📝 .github/cursor-review/tests/test_auto_retry_errored.py (+22 -8)
➕ .github/cursor-review/tests/test_dismissal_rereview.py (+477 -0)
📝 .github/cursor-review/tests/test_workflow_job_isolation.py (+4 -3)
📝 .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 (+221 -3)
📝 docs/callers/cursor-approve.md (+50 -0)
📝 docs/callers/cursor-review.md (+6 -3)

📄 Description

ELI-5

The bot approves commit X, and its status card says "✅ Approved". Then someone pushes Y. GitHub throws the approval away, but the card keeps saying "Approved" and nothing starts a new review. This PR makes the card say what actually happened ("approved X, the head moved to Y, which needs its own round"). Callers can also opt in to have the next round start on its own after a short wait.

What changes

1. The dismiss path marks the card stale (auto-approve.py mark_card_stale, called at the end of dismiss-stale; card.py render_dismissed). This applies only to a PR in the approval lane (approve_max_severity set and the author passes approve_authors), and only to a pass card whose commit the approver really approved, whether that approval is still live or was dismissed by the ruleset or by this job:

Case	state / next
head moved, below the cap	`no_decision` / `relabel`: "Approved `<old7>`; the head moved to `<new7>`, which needs its own round."
`needs-human-review`	`capped` / `human`, the same as the cap path
at the cap (delivered rounds ≥ `max_rounds`) without that label	`capped` / `human`, saying no round starts by itself (nothing applied the label on a push)
base retargeted only, or the head moved to a commit that already has a consolidated review	`no_decision` / `push_then_relabel` (a relabel alone would be deduped)
`skip-cursor-review`	`no_decision` / `human`
approval withdrawn on its own head and base (veto, `needs-human-review`, `--all-approvals`, or already dismissed)	as above, naming that reason instead of "the base changed"

The rewrite keeps the round heading and the cursor-approve:axes v1 history. It adds a cursor-approve-stale-head marker, so later events on the same head write nothing and a newer push rewrites the card again. A pass card with no matching approval is a round in progress (the pending or start card) and is left alone. A failed write only produces a warning. A run that could not withdraw every stale approval leaves the card alone and schedules nothing.

2. Opt-in auto re-run. There are two new inputs: auto_rereview_after_dismissal (default false) and auto_rereview_delay_minutes (default 10, clamped to 30). The dismiss job sets an auto_rereview output, and a new auto-rereview job waits for the delay, then runs auto-approve.py auto-rereview. That command re-reads everything live and relabels with the App token or APPROVER_TOKEN, the same way auto_retry_errored_rounds does, but only if all of these hold:

the head is still Y (debounce: a newer push schedules its own wait instead)
the PR is open, not a draft, and carries neither skip-cursor-review nor needs-human-review
the PR is under max_rounds
the card is still the stale card for Y, so no round has decided since
Y has no consolidated review yet (the gate's dup step would no-op the relabel)
the review label has not been applied since the approval the card reports (no round in flight)
no automatic round is already recorded for Y

The once-per-head record is written to the card before the relabel. It is a visible "started automatically" line plus a cursor-approve:auto-rereview v1 sentinel, which upsert carries through every later card write (bounded to 20 heads). So a head that flips back never loops. If the relabel itself fails, that line is rewritten to say so and to relabel by hand; the record stays. A veto or needs-human-review still moves a card whose automatic round already started to the human state. Under run_without_label there is no auto re-run, because the push already starts the round.

3. The card names when an automatic round starts. card.retry_delay_phrase() now renders at 05:42 UTC (5 min after the latest push) instead of "in about N minutes": the moment the wait is scheduled plus the clamped delay, in UTC HH:MM, with the date added only when the start falls on another UTC day. A newer push's dismissal rewrites the card, so the time shown is the live schedule. The existing auto_retry_errored_rounds notice shares the helper (… (5 min after this round ended): remove and re-add …). With delay 0 the wording is unchanged.

4. Docs: both input tables (docs/callers/cursor-review.md, .github/cursor-review/README.md), a new "A push that dismisses the approval" section in docs/callers/cursor-approve.md, and the trust-model job count (now four APPROVER_TOKEN jobs).

Judgment calls

"In flight" means the review label was applied at or after the approval the card reports, or the card is no longer the stale card for this head. The cutoff is the approval rather than the card's write, because the dismiss job writes the card after the push, so a relabel right after the push would predate it. A plain push has no server timestamp in the timeline, so the head-change time is not usable. The cost: a label applied for an intermediate head also counts, and the round is skipped rather than risking cancelling one for this head. The card then says to re-run by hand.
Scheduling on every event. Any PR event that finds the card stale for the live head (with no record yet) schedules a waiter, not only the event that rewrote the card. Otherwise, a caller whose concurrency group cancels on every PR event would lose the pending round to an unrelated event. The auto-rereview job has a concurrency group on PR + head (cancel-in-progress: false), so those waiters run one at a time and each one after the first reads the record and skips. Only one relabel happens per head. The cost is one runner per such event, sleeping during the delay.
The approve_authors rule is mirrored in Python (author_opted_in), because the dismiss job deliberately does not depend on gate. A test pins its edge cases to the gate's shell semantics.
The round count for "at the cap" reuses round-cap's own counting (round_reviews since needs-human-review last came off). If the count can't be read, the card is still rewritten as below the cap (round-cap enforces the cap at relabel time), but no auto re-run is scheduled.
cmd_auto_retry's relabel was extracted into relabel_review_label so both jobs share it. Its behavior and existing tests are unchanged.

Residual

The wiring change in the consumer repo is not done. It is a separate repo and must wait until the pin bump containing this change lands there: a caller that passes an undeclared input fails at startup on every round. After the bump, add auto_rereview_after_dismissal: true to that caller.
The two open consumer PRs the report cites were not exercised. One has a pass card two commits behind its head at round 1 of 5; the other is the same at round 5 of 5. They are in a private repo I cannot read from here. After the pin bump, the first event on each should rewrite its card: the first to no_decision/relabel, the second to capped/human.
No live run of the new job. The auto-rereview job and the dismiss-step wiring are verified only by unit tests with a faked GitHub and by structural workflow tests. A real pull_request_target run with the App token has not happened yet.
A label applied for an intermediate head counts as in flight, so that head's successor gets no automatic round, as described under judgment calls.
The wait can be cancelled without a replacement. It sleeps inside the push's run, so a pull_request_review_thread run in the same label- concurrency slot (e.g. resolving a thread during the wait) cancels it and does not schedule another; a later pull_request event does. Documented in the job comment and caller docs; the card's "If none starts, re-run it by hand" covers it. A cancellation-proof waiter is proposed as a follow-up.

Provenance

Authored by: agent-work loop
Verified:
- Tests: all green locally on the current head. cursor-review 1579, cursor-approve 277, workflow-pins 551 (43 skipped, as on main), agents-md-integrity 62.
- test_dismissal_rereview.py covers the ticket's list: below cap → no_decision/relabel naming both SHAs; at cap → capped/human; fires once after the delay on an unchanged head; a newer push, in flight, capped, draft, veto or human label → no relabel; flag off → card rewrite only. It also pins APPROVER_TOKEN scheduling a round only on pull_request_target, an approval withdrawn on its own head, a failed dismissal leaving the card, a failed relabel correcting it, the approval-time in-flight cutoff (a relabel before the card was written, a label in the same second), a head that already had a round, and the job's concurrency group.
- Mutation-tested: each of the 8 auto-rereview guards was disabled one at a time, and each was caught.
- Start-time phrase: tested with a fixed clock, including a delay that crosses UTC midnight, a non-UTC clock, the 30-minute clamp, delay 0 and unusable values.
- Lints: check_workflow_pins.py OK, check_agents_md.py passes (2 warnings that predate this PR), org-repo-literal lint OK.
- Not run: a live workflow run (see Residual).
Deviations:
- The consumer-repo wiring change is not included (separate repo, gated on the pin bump).
- The auto-rereview wait is not made cancellation-proof against pull_request_review_thread runs; the gap is documented and deferred.

_{🔄 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/416 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/12/2026 **Status:** ✅ Merged **Merged:** 10/12/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-20262-stale-card-auto-rereview` --- ### 📝 Commits (6) - [`50de881`](https://github.com/Comfy-Org/github-workflows/commit/50de8815cc9588151403cae5a243692fb1a1c654) feat(cursor-review): mark the card stale when a push dismisses the approval, and opt-in start the next round - [`16a735e`](https://github.com/Comfy-Org/github-workflows/commit/16a735ee64cedf8e095ff908aedf2a0a4a545288) fix(cursor-review): promise an automatic round only where auto-rereview accepts the token - [`b7b8f70`](https://github.com/Comfy-Org/github-workflows/commit/b7b8f704f2f9a87750dee7cf9ea90064d4d48b82) feat(cursor-approve): name the automatic round's UTC start time, not "in about N minutes" - [`6ca27c2`](https://github.com/Comfy-Org/github-workflows/commit/6ca27c2c5ac7f5d5a61a7ce7a6237d50fae5ec3b) fix(cursor-review): keep the stale card honest when the approval is withdrawn on its own head or the relabel fails - [`8597801`](https://github.com/Comfy-Org/github-workflows/commit/8597801c529cd61b1e03889cd80e29597c7316a5) fix(cursor-review): serialise auto-rereview waiters, cut off at the approval, skip reviewed heads - [`5a30af9`](https://github.com/Comfy-Org/github-workflows/commit/5a30af9c21695103122eccaf5558d84f52969a96) docs(cursor-approve): a review-thread run can cancel the auto-rereview wait without a replacement ### 📊 Changes **12 files changed** (+1314 additions, -49 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-approve/card.py` (+184 -7) 📝 `.github/cursor-review/README.md` (+3 -1) 📝 `.github/cursor-review/auto-approve.py` (+339 -19) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+2 -2) 📝 `.github/cursor-review/tests/test_auto_retry_errored.py` (+22 -8) ➕ `.github/cursor-review/tests/test_dismissal_rereview.py` (+477 -0) 📝 `.github/cursor-review/tests/test_workflow_job_isolation.py` (+4 -3) 📝 `.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` (+221 -3) 📝 `docs/callers/cursor-approve.md` (+50 -0) 📝 `docs/callers/cursor-review.md` (+6 -3) </details> ### 📄 Description ## ELI-5 The bot approves commit X, and its status card says "✅ Approved". Then someone pushes Y. GitHub throws the approval away, but the card keeps saying "Approved" and nothing starts a new review. This PR makes the card say what actually happened ("approved X, the head moved to Y, which needs its own round"). Callers can also opt in to have the next round start on its own after a short wait. ## What changes **1. The dismiss path marks the card stale** (`auto-approve.py` `mark_card_stale`, called at the end of `dismiss-stale`; `card.py` `render_dismissed`). This applies only to a PR in the approval lane (`approve_max_severity` set and the author passes `approve_authors`), and only to a `pass` card whose commit the approver really approved, whether that approval is still live or was dismissed by the ruleset or by this job: | Case | state / next | |---|---| | head moved, below the cap | `no_decision` / `relabel`: "Approved `<old7>`; the head moved to `<new7>`, which needs its own round." | | `needs-human-review` | `capped` / `human`, the same as the cap path | | at the cap (delivered rounds ≥ `max_rounds`) without that label | `capped` / `human`, saying no round starts by itself (nothing applied the label on a push) | | base retargeted only, or the head moved to a commit that already has a consolidated review | `no_decision` / `push_then_relabel` (a relabel alone would be deduped) | | `skip-cursor-review` | `no_decision` / `human` | | approval withdrawn on its own head and base (veto, `needs-human-review`, `--all-approvals`, or already dismissed) | as above, naming that reason instead of "the base changed" | The rewrite keeps the round heading and the `cursor-approve:axes v1` history. It adds a `cursor-approve-stale-head` marker, so later events on the same head write nothing and a newer push rewrites the card again. A `pass` card with no matching approval is a round in progress (the pending or start card) and is left alone. A failed write only produces a warning. A run that could not withdraw every stale approval leaves the card alone and schedules nothing. **2. Opt-in auto re-run.** There are two new inputs: `auto_rereview_after_dismissal` (default `false`) and `auto_rereview_delay_minutes` (default `10`, clamped to 30). The dismiss job sets an `auto_rereview` output, and a new `auto-rereview` job waits for the delay, then runs `auto-approve.py auto-rereview`. That command re-reads everything live and relabels with the App token or `APPROVER_TOKEN`, the same way `auto_retry_errored_rounds` does, but only if all of these hold: - the head is still Y (debounce: a newer push schedules its own wait instead) - the PR is open, not a draft, and carries neither `skip-cursor-review` nor `needs-human-review` - the PR is under `max_rounds` - the card is still the stale card for Y, so no round has decided since - Y has no consolidated review yet (the gate's `dup` step would no-op the relabel) - the review label has not been applied since the approval the card reports (no round in flight) - no automatic round is already recorded for Y The once-per-head record is written to the card **before** the relabel. It is a visible "started automatically" line plus a `cursor-approve:auto-rereview v1` sentinel, which `upsert` carries through every later card write (bounded to 20 heads). So a head that flips back never loops. If the relabel itself fails, that line is rewritten to say so and to relabel by hand; the record stays. A veto or `needs-human-review` still moves a card whose automatic round already started to the human state. Under `run_without_label` there is no auto re-run, because the push already starts the round. **3. The card names when an automatic round starts.** `card.retry_delay_phrase()` now renders `at 05:42 UTC (5 min after the latest push)` instead of "in about N minutes": the moment the wait is scheduled plus the clamped delay, in UTC `HH:MM`, with the date added only when the start falls on another UTC day. A newer push's dismissal rewrites the card, so the time shown is the live schedule. The existing `auto_retry_errored_rounds` notice shares the helper (`… (5 min after this round ended): remove and re-add …`). With delay `0` the wording is unchanged. **4. Docs:** both input tables (`docs/callers/cursor-review.md`, `.github/cursor-review/README.md`), a new "A push that dismisses the approval" section in `docs/callers/cursor-approve.md`, and the trust-model job count (now four `APPROVER_TOKEN` jobs). ## Judgment calls - **"In flight"** means the review label was applied at or after the approval the card reports, or the card is no longer the stale card for this head. The cutoff is the approval rather than the card's write, because the dismiss job writes the card after the push, so a relabel right after the push would predate it. A plain push has no server timestamp in the timeline, so the head-change time is not usable. The cost: a label applied for an intermediate head also counts, and the round is skipped rather than risking cancelling one for this head. The card then says to re-run by hand. - **Scheduling on every event.** Any PR event that finds the card stale for the live head (with no record yet) schedules a waiter, not only the event that rewrote the card. Otherwise, a caller whose concurrency group cancels on every PR event would lose the pending round to an unrelated event. The `auto-rereview` job has a concurrency group on PR + head (`cancel-in-progress: false`), so those waiters run one at a time and each one after the first reads the record and skips. Only one relabel happens per head. The cost is one runner per such event, sleeping during the delay. - **The `approve_authors` rule** is mirrored in Python (`author_opted_in`), because the dismiss job deliberately does not depend on `gate`. A test pins its edge cases to the gate's shell semantics. - **The round count** for "at the cap" reuses round-cap's own counting (`round_reviews` since `needs-human-review` last came off). If the count can't be read, the card is still rewritten as below the cap (round-cap enforces the cap at relabel time), but no auto re-run is scheduled. - `cmd_auto_retry`'s relabel was extracted into `relabel_review_label` so both jobs share it. Its behavior and existing tests are unchanged. ## Residual - **The wiring change in the consumer repo is not done.** It is a separate repo and must wait until the pin bump containing this change lands there: a caller that passes an undeclared input fails at startup on every round. After the bump, add `auto_rereview_after_dismissal: true` to that caller. - **The two open consumer PRs the report cites were not exercised.** One has a `pass` card two commits behind its head at round 1 of 5; the other is the same at round 5 of 5. They are in a private repo I cannot read from here. After the pin bump, the first event on each should rewrite its card: the first to `no_decision`/`relabel`, the second to `capped`/`human`. - **No live run of the new job.** The `auto-rereview` job and the dismiss-step wiring are verified only by unit tests with a faked GitHub and by structural workflow tests. A real `pull_request_target` run with the App token has not happened yet. - **A label applied for an intermediate head** counts as in flight, so that head's successor gets no automatic round, as described under judgment calls. - **The wait can be cancelled without a replacement.** It sleeps inside the push's run, so a `pull_request_review_thread` run in the same `label-` concurrency slot (e.g. resolving a thread during the wait) cancels it and does not schedule another; a later `pull_request` event does. Documented in the job comment and caller docs; the card's "If none starts, re-run it by hand" covers it. A cancellation-proof waiter is proposed as a follow-up. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: all green locally on the current head. cursor-review 1579, cursor-approve 277, workflow-pins 551 (43 skipped, as on main), agents-md-integrity 62. - `test_dismissal_rereview.py` covers the ticket's list: below cap → `no_decision`/`relabel` naming both SHAs; at cap → `capped`/`human`; fires once after the delay on an unchanged head; a newer push, in flight, capped, draft, veto or human label → no relabel; flag off → card rewrite only. It also pins `APPROVER_TOKEN` scheduling a round only on `pull_request_target`, an approval withdrawn on its own head, a failed dismissal leaving the card, a failed relabel correcting it, the approval-time in-flight cutoff (a relabel before the card was written, a label in the same second), a head that already had a round, and the job's concurrency group. - Mutation-tested: each of the 8 auto-rereview guards was disabled one at a time, and each was caught. - Start-time phrase: tested with a fixed clock, including a delay that crosses UTC midnight, a non-UTC clock, the 30-minute clamp, delay `0` and unusable values. - Lints: `check_workflow_pins.py` OK, `check_agents_md.py` passes (2 warnings that predate this PR), org-repo-literal lint OK. - Not run: a live workflow run (see Residual). - **Deviations:** - The consumer-repo wiring change is not included (separate repo, gated on the pin bump). - The `auto-rereview` wait is not made cancellation-proof against `pull_request_review_thread` runs; the gap is documented and deferred. --- <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:35 +00:00