Comfy-Org/github-workflows

Fork 0

mirror of https://github.com/Comfy-Org/github-workflows.git synced 2026-11-05 00:17:37 +00:00

[PR #418] [MERGED] feat(cursor-review): don't count a round a push superseded toward max_rounds #419

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/418
Author: @mattmillerai
Created: 10/12/2026
Status: ✅ Merged
Merged: 10/12/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-20267-rereview-every-push

📝 Commits (2)

d1f4451 feat(cursor-review): don't count a round a push superseded toward max_rounds
e27a13b fix(cursor-review): bound and harden the superseded-round excusal

📊 Changes

5 files changed (+212 additions, -15 deletions)

View changed files

📝 .github/cursor-review/README.md (+1 -1)
📝 .github/cursor-review/auto-approve.py (+90 -9)
📝 .github/cursor-review/tests/test_auto_approve.py (+100 -0)
📝 .github/workflows/cursor-review.yml (+9 -2)
📝 docs/callers/cursor-review.md (+12 -3)

📄 Description

Summary

A round that a newer push made moot no longer counts toward max_rounds.

auto-approve.py: the new superseded_by_push() check excludes a consolidated panel review from round_reviews() when that round's own decision says the PR head moved. The round's commit is the head in its body's round sentinel, not the review's commit_id, which GitHub can move forward. "Own decision" means the first auto-approve review that:
- was posted by the same posting identity,
- is unedited, with a body that starts with the auto-approve marker,
- carries the round's commit as its reviewed SHA,
- comes after the panel review and before the next consolidated review on that commit, ordered by review id.
"Says the head moved" means its body has decide_gate's the PR head moved while the review ran reason line.
The round-cap job, the round output (next_round) and the #416 re-review checks (rounds_so_far) all go through round_reviews, so all three use the new count.
The cap stays bounded. At most one round is excused per reviewed commit, and at most max_rounds in total, so a PR runs at most twice the cap's panels. decide_gate also no longer gives the head-moved reason for an unreadable (empty) live head; that case gets its own the PR head could not be read reason, and the SHA comparison is case-insensitive.
These rounds still count:
- one that decided on its own head (approve, request changes, or no decision for any other reason);
- one with no decision on the PR;
- one whose decision is a forged marker from another author;
- one decided by a separate APPROVER_TOKEN identity, which round-cap does not read. Counting it is the conservative side;
- one whose head moved during the decision's own POST (that follow-up uses different wording);
- one whose body carries no round sentinel;
- any superseded round past either bound.
The round output is computed before the round runs, so a round that is excused afterwards still reports N, and the next counted round reports N again. The docs now say this.
Docs: the max_rounds input and round output descriptions, .github/cursor-review/README.md, and docs/callers/cursor-review.md (input table, the round-cap section and the outputs table).

Testing

python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py': 1586 tests, OK. The RoundCounterTest cases are built from the real render_standing_body / render_body / render_round_sentinel with CRLF-stored bodies. They cover:
- head-moved exclusion;
- sentinel-over-commit_id;
- forged, edited, quoted, earlier and other-SHA decisions;
- own-decision-governs;
- no-decision-round-does-not-borrow (same-second timestamps);
- the per-commit and max_rounds bounds;
- the unreadable live head.
The cursor-approve suite and the workflow-pins suite (including the input-docs drift test) are OK. check_workflow_pins.py, check_agents_md.py and check-org-repo-literals.sh all pass.
Doc-drift sweep: run. I updated the max_rounds / round descriptions in the workflow, the README and the setup guide.

Residual

This PR delivers only item 3 of the ticket (superseded rounds stop counting), and only one of its two kinds of evidence. Still open:

Re-run after every head bump (item 1) is not done. #416's auto-rereview still fires only when a push dismisses an approval. Its whole chain is built on that stale pass card: mark_card_stale gates on a card approval, and cmd_auto_rereview requires card.stale_head == head and reads its in-flight cutoff from the approval's submitted_at. Extending it to a not-approved lane PR on every synchronize needs three things:
- a card-independent trigger in dismiss-stale-approval;
- a different in-flight cutoff, such as the push time or the last consolidated review;
- the once-per-head record written somewhere other than a pass card.
Tests named by the ticket that are not written yet: a burst of pushes inside the cooldown starts one round on the last head (not-approved PR and dismissed approval), and a non-lane author's PR keeps label-only behaviour.
Cancelling the stale round on a push (item 2) is not done. Choose between reusing run_without_label's supersede-on-push concurrency, scoped per PR to lane authors, and an explicit run cancel, then document the choice. Test still to write: a push while a round runs cancels it.
Superseded-round evidence, second half. A round with no decision for its SHA, plus a newer head pushed before it finished, still counts. This is the cancelled-after-panel-posted case and a push during cursor-approve's axis phase. Two reasons it is not handled:
- the PR timeline gives no trustworthy push time for a non-force push;
- cursor-approve's superseded outcome is a card state, not a review this check can match on author.
Cloud wiring is not done. It is a separate repo and lands only after this PR plus items 1 and 2 are pinned. Set the re-run input and auto_rereview_delay_minutes: 10 in the caller only after the bump: an undeclared input breaks every round. Per the ticket's burst analysis, also confirm the fleet push gate is live first.
Unexercised artifacts: the cost scorecard PRD and the two linked review pages the ticket cites (not reachable from here). I did not re-measure the 19-of-80 head-moved figure against live PRs.

Provenance

Authored by: agent-work loop
Verified:
- cursor-review unit suite: 1586 tests, OK
- cursor-approve unit suite: OK
- workflow-pins suite: OK
- pins, AGENTS.md and org-literal lints: pass
- doc-drift sweep: run
Deviations:
- Partial delivery. Only the round-cap exclusion (item 3, decision-evidence half) shipped; items 1 and 2 and the cloud wiring are listed under Residual.

🤖 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/418 **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-20267-rereview-every-push` --- ### 📝 Commits (2) - [`d1f4451`](https://github.com/Comfy-Org/github-workflows/commit/d1f4451598df8507c08fb0f2904c6d7c63611f91) feat(cursor-review): don't count a round a push superseded toward max_rounds - [`e27a13b`](https://github.com/Comfy-Org/github-workflows/commit/e27a13bbcbdfd998b39facbe1bb3b6762c110634) fix(cursor-review): bound and harden the superseded-round excusal ### 📊 Changes **5 files changed** (+212 additions, -15 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+1 -1) 📝 `.github/cursor-review/auto-approve.py` (+90 -9) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+100 -0) 📝 `.github/workflows/cursor-review.yml` (+9 -2) 📝 `docs/callers/cursor-review.md` (+12 -3) </details> ### 📄 Description ## Summary A round that a newer push made moot no longer counts toward `max_rounds`. - `auto-approve.py`: the new `superseded_by_push()` check excludes a consolidated panel review from `round_reviews()` when that round's own decision says the PR head moved. The round's commit is the `head` in its body's round sentinel, not the review's `commit_id`, which GitHub can move forward. "Own decision" means the first auto-approve review that: - was posted by the same posting identity, - is unedited, with a body that starts with the auto-approve marker, - carries the round's commit as its reviewed SHA, - comes after the panel review and before the next consolidated review on that commit, ordered by review id. "Says the head moved" means its body has decide_gate's `the PR head moved while the review ran` reason line. - The `round-cap` job, the `round` output (`next_round`) and the #416 re-review checks (`rounds_so_far`) all go through `round_reviews`, so all three use the new count. - The cap stays bounded. At most one round is excused per reviewed commit, and at most `max_rounds` in total, so a PR runs at most twice the cap's panels. decide_gate also no longer gives the head-moved reason for an unreadable (empty) live head; that case gets its own `the PR head could not be read` reason, and the SHA comparison is case-insensitive. - These rounds still count: - one that decided on its own head (approve, request changes, or no decision for any other reason); - one with no decision on the PR; - one whose decision is a forged marker from another author; - one decided by a separate `APPROVER_TOKEN` identity, which round-cap does not read. Counting it is the conservative side; - one whose head moved during the decision's own POST (that follow-up uses different wording); - one whose body carries no round sentinel; - any superseded round past either bound. - The `round` output is computed before the round runs, so a round that is excused afterwards still reports N, and the next counted round reports N again. The docs now say this. - Docs: the `max_rounds` input and `round` output descriptions, `.github/cursor-review/README.md`, and `docs/callers/cursor-review.md` (input table, the round-cap section and the outputs table). ## Testing - `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'`: 1586 tests, OK. The `RoundCounterTest` cases are built from the real `render_standing_body` / `render_body` / `render_round_sentinel` with CRLF-stored bodies. They cover: - head-moved exclusion; - sentinel-over-`commit_id`; - forged, edited, quoted, earlier and other-SHA decisions; - own-decision-governs; - no-decision-round-does-not-borrow (same-second timestamps); - the per-commit and `max_rounds` bounds; - the unreadable live head. - The cursor-approve suite and the workflow-pins suite (including the input-docs drift test) are OK. `check_workflow_pins.py`, `check_agents_md.py` and `check-org-repo-literals.sh` all pass. - Doc-drift sweep: run. I updated the `max_rounds` / `round` descriptions in the workflow, the README and the setup guide. ## Residual This PR delivers only item 3 of the ticket (superseded rounds stop counting), and only one of its two kinds of evidence. Still open: 1. **Re-run after every head bump (item 1) is not done.** #416's `auto-rereview` still fires only when a push dismisses an approval. Its whole chain is built on that stale `pass` card: `mark_card_stale` gates on a card approval, and `cmd_auto_rereview` requires `card.stale_head == head` and reads its in-flight cutoff from the approval's `submitted_at`. Extending it to a not-approved lane PR on every `synchronize` needs three things: - a card-independent trigger in `dismiss-stale-approval`; - a different in-flight cutoff, such as the push time or the last consolidated review; - the once-per-head record written somewhere other than a pass card. Tests named by the ticket that are not written yet: a burst of pushes inside the cooldown starts one round on the last head (not-approved PR and dismissed approval), and a non-lane author's PR keeps label-only behaviour. 2. **Cancelling the stale round on a push (item 2) is not done.** Choose between reusing `run_without_label`'s supersede-on-push concurrency, scoped per PR to lane authors, and an explicit run cancel, then document the choice. Test still to write: a push while a round runs cancels it. 3. **Superseded-round evidence, second half.** A round with *no* decision for its SHA, plus a newer head pushed before it finished, still counts. This is the cancelled-after-panel-posted case and a push during cursor-approve's axis phase. Two reasons it is not handled: - the PR timeline gives no trustworthy push time for a non-force push; - cursor-approve's `superseded` outcome is a card state, not a review this check can match on author. 4. **Cloud wiring is not done.** It is a separate repo and lands only after this PR plus items 1 and 2 are pinned. Set the re-run input and `auto_rereview_delay_minutes: 10` in the caller only after the bump: an undeclared input breaks every round. Per the ticket's burst analysis, also confirm the fleet push gate is live first. 5. **Unexercised artifacts:** the cost scorecard PRD and the two linked review pages the ticket cites (not reachable from here). I did not re-measure the 19-of-80 head-moved figure against live PRs. ## Provenance - **Authored by:** agent-work loop - **Verified:** - cursor-review unit suite: 1586 tests, OK - cursor-approve unit suite: OK - workflow-pins suite: OK - pins, AGENTS.md and org-literal lints: pass - doc-drift sweep: run - **Deviations:** - Partial delivery. Only the round-cap exclusion (item 3, decision-evidence half) shipped; items 1 and 2 and the cloud wiring are listed under Residual. 🤖 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:35 +00:00

closed this issue
added the
pull-request
label

No labels

cursor-review-catalog-drift

groom

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/github-workflows#419

No description provided.

Rows
Columns