[PR #412] [MERGED] feat(cursor-review): link every auto-approve review to the cursor-approve card (BE-20246) #415

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-20246-card-link-in-auto-approve-reviews

📝 Commits (3)

4b5a056 feat(cursor-review): link every auto-approve review to the cursor-approve card (BE-20246)
d8c6a33 fix(cursor-approve): give the pending card the start phase's pass/none pair (BE-20246)
7e285f2 fix(cursor-review): never strand the pending card a first round creates (BE-20246)

📊 Changes

8 files changed (+542 additions, -23 deletions)

View changed files

📝 .github/cursor-approve/card.py (+68 -6)
📝 .github/cursor-approve/tests/test_card.py (+126 -0)
📝 .github/cursor-review/README.md (+1 -1)
📝 .github/cursor-review/auto-approve.py (+114 -6)
📝 .github/cursor-review/tests/test_approve_external.py (+25 -2)
📝 .github/cursor-review/tests/test_auto_approve.py (+179 -6)
📝 docs/callers/cursor-approve.md (+18 -2)
📝 docs/callers/cursor-review.md (+11 -0)

📄 Description

ELI-5

Each round, the bot posts a review that approves a PR or blocks it. It also keeps one status card comment on the PR, updated every round, showing what it decided and why. Until now those reviews didn't link to the card, so people had to scroll the PR to find it. Every one of those reviews now links straight to the card.

What changed

Every auto-approve review links the card. That covers decide's APPROVE, its REQUEST_CHANGES and its standing block, plus the standing block approve-external leaves when it withholds. approve-external's approval already linked the card. The link line reads: Live status: the [cursor-approve card](…), updated in place every round. It sits below the decision and above the scope and threshold notes. The markers at the top of each review are unchanged, so staleness, the recorded base and the auto-retry budget all parse the same way. The URL check (https only, nothing that can close the link) is now one helper, safe_card_url, and render_external_body uses it too.
First round: the card is created just before the review. On a PR's first round there is no card yet. Decide now calls a new card.ensure just before posting its first review of the round. ensure returns the existing card, or creates a pending one (render_pending: "Deciding this round…" with pass / none, the pair the start phase already writes for a round in progress, so agents read it as nothing to do) when there is none. The round's card replaces the pending one in the same step, after the review, as before. This is the same pattern cursor-approve.yml's Find the card step already uses for its approval link.
The round's card edits the pending card by its comment id. upsert(..., card=) skips the second comment listing. A listing taken seconds after the card was created could miss it and post a second card.
Never fatal. If the card cannot be found or created, the review is posted without the link and the run logs a warning.
No new pending card where it would get stuck:
- rounds that post no review (a deferred pass, or capped);
- a PR the approver authored. GitHub refuses that review, so decide skips the card. If an empty or stale --pr-author lets the round through to the refused POST, decide deletes the pending card it created.
- a review body that fails to render. The body is rendered before the card is looked up or created.
- runs without --card true.
A pending card is never left pointing at the wrong review. If decide cannot withdraw the review it just posted after the head or base moved (or after a hand-off), it writes a no_decision / human card. card.ensure also lists the comments again after a create that failed ambiguously, so the caller edits a card that did land rather than posting a second one.
Docs: docs/callers/cursor-review.md, the status-card contract in docs/callers/cursor-approve.md (the pending card and what none means there), both module docstrings and the cursor-review README row.

Why not edit the review afterwards

The ticket's first plan was to post the review, write the card, then edit the review to add the link. That would break the dismissal model. _review_node treats any review with lastEditedAt as edited, and:

an edited approval is always treated as stale, so dismiss-stale withdraws it on the next event;
an edited request for changes is never withdrawn by the bot, so the standing block would have to be dismissed by hand.

So the link has to be in the review when it is posted, and on the first round that means the card must exist first. A test pins that no review is ever edited.

Why

On a recent four-round PR in a private caller repo, the bot posted four auto-approve reviews and none of them linked the card. One standing block said "Address the axis verdicts above, push, then start a new round". The verdicts it meant are on the card, which nothing in the review linked to.

Testing

python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py': 1543 tests, OK.
python3 -m unittest discover -s .github/cursor-approve/tests -p 'test_*.py': 246 tests, OK. The 1 skip ("needs /proc") is a pre-existing macOS-only skip.
check_workflow_pins.py OK. check-org-repo-literals.sh (and its shellcheck) OK. check_agents_md.py passed with 2 warnings that were already there.
New tests:
- each review type links the card, and its markers still parse;
- unsafe or missing URLs are never linked;
- first round: pending card, then the review, then a PATCH of that same comment, and never a second card;
- a later round links the card it finds without writing it first;
- an APPROVE withdrawn after a head move, and the block that replaces it, link one card that was looked up once;
- a card lookup that fails only costs the link;
- the approver's own PR, deferred-pass and capped rounds create no pending card;
- an own-PR refusal that gets past the author check deletes the pending card; a body that cannot render creates none; a review that cannot be withdrawn gets a no_decision / human card;
- card.ensure returns a card that landed after an ambiguous create, and raises when none did;
- no review is ever edited;
- the auto-retry block still passes the gate's own-line marker check;
- card.ensure found and created paths, the ensure phase's card_url output and its never-fatal failure, and upsert(card=).
Mutation-checked: dropping the by-id edit, the pending card, the own-PR guard or the standing-block link each fails the suite.
Existing test fixtures changed:
- StatusCard tests now check that every card write lands on one comment, rather than that there was exactly one write;
- AutoRetry tests run on round 2 with that round's existing card.
Not run end to end on a live PR. The first-round path should be confirmed on the first caller round after the pin bumps.

Notes for callers

Only assets loaded at run time change (auto-approve.py, card.py). There are no new inputs, secrets or permissions, because decide already writes the card. A caller gets this when uses: and workflows_ref: are bumped together.

Provenance

Authored by: agent-work loop
Verified:

Both script suites pass at the current head: cursor-review (1543 tests) and cursor-approve (246 tests).
check_workflow_pins.py, check-org-repo-literals.sh and check_agents_md.py all pass (2 warnings that were already there).
The review round's stranded-pending-card, own-PR and ambiguous-create fixes each have a test.

Deviations:

Not run end to end on a live PR. The first-round path needs confirming on the first caller round after the pin bump.
Not changed: the overlapping-runs card race. The per-PR concurrency group prevents it, and upsert has always listed and then POSTed.

🤖 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/412 **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-20246-card-link-in-auto-approve-reviews` --- ### 📝 Commits (3) - [`4b5a056`](https://github.com/Comfy-Org/github-workflows/commit/4b5a0562c92c5ffe762786f8e0d923cfc5bd223a) feat(cursor-review): link every auto-approve review to the cursor-approve card (BE-20246) - [`d8c6a33`](https://github.com/Comfy-Org/github-workflows/commit/d8c6a339d0cb1329a5da6330d6b9f6b68450a63f) fix(cursor-approve): give the pending card the start phase's pass/none pair (BE-20246) - [`7e285f2`](https://github.com/Comfy-Org/github-workflows/commit/7e285f21870be475b7693115c457033c9caff087) fix(cursor-review): never strand the pending card a first round creates (BE-20246) ### 📊 Changes **8 files changed** (+542 additions, -23 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-approve/card.py` (+68 -6) 📝 `.github/cursor-approve/tests/test_card.py` (+126 -0) 📝 `.github/cursor-review/README.md` (+1 -1) 📝 `.github/cursor-review/auto-approve.py` (+114 -6) 📝 `.github/cursor-review/tests/test_approve_external.py` (+25 -2) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+179 -6) 📝 `docs/callers/cursor-approve.md` (+18 -2) 📝 `docs/callers/cursor-review.md` (+11 -0) </details> ### 📄 Description ## ELI-5 Each round, the bot posts a review that approves a PR or blocks it. It also keeps one status card comment on the PR, updated every round, showing what it decided and why. Until now those reviews didn't link to the card, so people had to scroll the PR to find it. Every one of those reviews now links straight to the card. ## What changed - **Every auto-approve review links the card.** That covers decide's APPROVE, its REQUEST_CHANGES and its standing block, plus the standing block approve-external leaves when it withholds. approve-external's approval already linked the card. The link line reads: `Live status: the [cursor-approve card](…), updated in place every round.` It sits below the decision and above the scope and threshold notes. The markers at the top of each review are unchanged, so staleness, the recorded base and the auto-retry budget all parse the same way. The URL check (https only, nothing that can close the link) is now one helper, `safe_card_url`, and `render_external_body` uses it too. - **First round: the card is created just before the review.** On a PR's first round there is no card yet. Decide now calls a new `card.ensure` just before posting its first review of the round. `ensure` returns the existing card, or creates a pending one (`render_pending`: "Deciding this round…" with `pass` / `none`, the pair the start phase already writes for a round in progress, so agents read it as nothing to do) when there is none. The round's card replaces the pending one in the same step, after the review, as before. This is the same pattern cursor-approve.yml's `Find the card` step already uses for its approval link. - **The round's card edits the pending card by its comment id.** `upsert(..., card=)` skips the second comment listing. A listing taken seconds after the card was created could miss it and post a second card. - **Never fatal.** If the card cannot be found or created, the review is posted without the link and the run logs a warning. - **No new pending card where it would get stuck:** - rounds that post no review (a deferred pass, or `capped`); - a PR the approver authored. GitHub refuses that review, so decide skips the card. If an empty or stale `--pr-author` lets the round through to the refused POST, decide deletes the pending card it created. - a review body that fails to render. The body is rendered before the card is looked up or created. - runs without `--card true`. - **A pending card is never left pointing at the wrong review.** If decide cannot withdraw the review it just posted after the head or base moved (or after a hand-off), it writes a `no_decision` / `human` card. `card.ensure` also lists the comments again after a create that failed ambiguously, so the caller edits a card that did land rather than posting a second one. - Docs: `docs/callers/cursor-review.md`, the status-card contract in `docs/callers/cursor-approve.md` (the pending card and what `none` means there), both module docstrings and the cursor-review README row. ## Why not edit the review afterwards The ticket's first plan was to post the review, write the card, then edit the review to add the link. That would break the dismissal model. `_review_node` treats any review with `lastEditedAt` as edited, and: - an **edited approval is always treated as stale**, so dismiss-stale withdraws it on the next event; - an **edited request for changes is never withdrawn by the bot**, so the standing block would have to be dismissed by hand. So the link has to be in the review when it is posted, and on the first round that means the card must exist first. A test pins that no review is ever edited. ## Why On a recent four-round PR in a private caller repo, the bot posted four auto-approve reviews and none of them linked the card. One standing block said "Address the axis verdicts above, push, then start a new round". The verdicts it meant are on the card, which nothing in the review linked to. ## Testing - `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'`: 1543 tests, OK. - `python3 -m unittest discover -s .github/cursor-approve/tests -p 'test_*.py'`: 246 tests, OK. The 1 skip ("needs /proc") is a pre-existing macOS-only skip. - `check_workflow_pins.py` OK. `check-org-repo-literals.sh` (and its shellcheck) OK. `check_agents_md.py` passed with 2 warnings that were already there. - New tests: - each review type links the card, and its markers still parse; - unsafe or missing URLs are never linked; - first round: pending card, then the review, then a PATCH of that same comment, and never a second card; - a later round links the card it finds without writing it first; - an APPROVE withdrawn after a head move, and the block that replaces it, link one card that was looked up once; - a card lookup that fails only costs the link; - the approver's own PR, deferred-pass and capped rounds create no pending card; - an own-PR refusal that gets past the author check deletes the pending card; a body that cannot render creates none; a review that cannot be withdrawn gets a `no_decision` / `human` card; - `card.ensure` returns a card that landed after an ambiguous create, and raises when none did; - no review is ever edited; - the auto-retry block still passes the gate's own-line marker check; - `card.ensure` found and created paths, the `ensure` phase's `card_url` output and its never-fatal failure, and `upsert(card=)`. - Mutation-checked: dropping the by-id edit, the pending card, the own-PR guard or the standing-block link each fails the suite. - **Existing test fixtures changed:** - StatusCard tests now check that every card write lands on one comment, rather than that there was exactly one write; - AutoRetry tests run on round 2 with that round's existing card. - Not run end to end on a live PR. The first-round path should be confirmed on the first caller round after the pin bumps. ## Notes for callers Only assets loaded at run time change (`auto-approve.py`, `card.py`). There are no new inputs, secrets or permissions, because decide already writes the card. A caller gets this when `uses:` and `workflows_ref:` are bumped together. ## Provenance **Authored by:** agent-work loop **Verified:** - Both script suites pass at the current head: cursor-review (1543 tests) and cursor-approve (246 tests). - `check_workflow_pins.py`, `check-org-repo-literals.sh` and `check_agents_md.py` all pass (2 warnings that were already there). - The review round's stranded-pending-card, own-PR and ambiguous-create fixes each have a test. **Deviations:** - Not run end to end on a live PR. The first-round path needs confirming on the first caller round after the pin bump. - Not changed: the overlapping-runs card race. The per-PR concurrency group prevents it, and `upsert` has always listed and then POSTed. 🤖 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:32 +00:00