[PR #358] [MERGED] feat(cursor-review): per-author auto-approve opt-in (approve_authors) #359

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-19479-cursor-review-approve-authors

📝 Commits (3)

65a46cc feat(cursor-review): per-author auto-approve opt-in (approve_authors)
2b6117c fix(cursor-review): fail closed on approve_authors edge cases
130836d docs(cursor-review): document approve_authors withdrawal and fail-closed list (BE-19479)

📊 Changes

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

View changed files

📝 .github/cursor-review/README.md (+1 -0)
📝 .github/cursor-review/auto-approve.py (+45 -8)
📝 .github/cursor-review/tests/test_auto_approve.py (+172 -1)
📝 .github/workflows/cursor-review.yml (+71 -5)
📝 docs/callers/cursor-review.md (+12 -1)

📄 Description

ELI-5

Auto-approve in cursor-review.yml is currently all-or-nothing for a caller repo. This adds an approve_authors list, so a repo can turn auto-approve on for a few PR authors first. PRs by anyone else still get the full review and its threads, but the bot doesn't approve or request changes on them.

What changed

New input approve_authors (string, default ''): a comma- or space-separated list of GitHub logins. Matching ignores case and a leading @. An empty list means every author, which is today's behaviour.
Resolved once, in the gate job. A new first step writes approve_author_ok (true/false). gate runs on every event, so the workflow-level approve_gate output reports off for an unlisted author whether or not a round runs. It doesn't fall back to untrusted on no-round events.
decide (auto-approve.py) has new --author-enabled / --pr-author flags. When the author isn't listed it writes approve_gate=off, posts no APPROVE and no REQUEST_CHANGES, and emits the note "auto-approve not enabled for author <login>". An invalid approve_max_severity still fails red.
post-review.py gets an empty approve threshold for an unlisted author. Without that, it would label threads "not blocking auto-approve" for a decision that is never taken.
Dismiss stale auto-approval is unchanged, so it still withdraws earlier marked approvals for every author.
post-review now lists gate in needs: so it can read the output. gate always runs and succeeds, so this adds no ordering and no skip path.
Docs: input table row and an auto-approve paragraph in docs/callers/cursor-review.md, the off row of the approve_gate table, and the knob table in .github/cursor-review/README.md.

Judgment calls

No withdrawal in decide for an unlisted author. This matches what an empty approve_max_severity does: decide doesn't run at all, and stale approvals are left to dismiss-stale. If an author is removed from the list mid-PR, an approval already on the current head stays until the next push. Unsetting approve_max_severity behaves the same way today.
Only the shell step parses the list, and the parse is tested by running that step from the YAML against a case table. Python gets the resolved boolean, so the parsing logic exists in one place.

Residual

Part 2 is not in this PR: the automatic re-review when every panel thread is resolved after a REQUEST_CHANGES (the new pull_request_review_thread: resolved path). It needs these six conditions:
- auto-approve is enabled for the PR (approve_max_severity set and the author passes approve_authors);
- the approver identity's latest decision is REQUEST_CHANGES, read live;
- zero unresolved panel-poster threads remain, read live across all pages;
- no round has run yet for the current (head SHA, set of resolved thread ids) marker;
- the max_rounds cap isn't reached;
- the PR is open, not a draft, and carries no veto label.
- It also needs the status line "All review threads resolved — starting round N", tests per condition plus firing exactly once per marker plus human threads not blocking, and caller docs.
- I left it out because it is a design change, not a wiring change. Three existing invariants have to be reworked together:
  - Duplicate check. The gate job skips any head SHA that already has a consolidated review. "Explain, then resolve" pushes no commit, so a re-review on the same SHA must bypass that check. The resolved-set marker would replace it.
  - Concurrency. The reusable's concurrency: block deliberately keeps thread events out of the trigger slot so that resolving a thread never cancels a panel. Sharing the slot with label-triggered rounds, as the ticket asks, means making cancel-in-progress depend on the event, and pending runs of the two kinds can then replace each other.
  - Caller groups. The documented caller groups (PR-number-only, plus the per-run thread slot for blocking callers) decide which of these runs ever reach the reusable.
- Getting that wrong either destroys running panels or loops rounds. It is roughly another 300+ lines across the workflow, auto-approve.py and the tests, which is too much to add to this unattended PR.
- The ticket asks for the direct in-reusable path to be preferred over the App-token label toggle. Both paths hit the same-SHA duplicate check, so whoever picks this up should decide that first.
The Comfy-Org/cloud caller follow-up is not done. It needs approve_authors: ${{ vars.CURSOR_APPROVE_AUTHORS }} and the variable set to the DRI's login. It's a separate repo and is out of scope for this PR.
Not exercised: a live caller run. Verification is unit- and workflow-text-level only (see below).

Provenance

Authored by: agent-work loop
Verified:
- Tests: cursor-review suite green (1001), including new ApproveAuthorsTest: an unlisted author gets off and no review event, a listed or empty-list author gets the normal APPROVE / REQUEST_CHANGES, the gate step's parse is run from the YAML against 11 cases, and the wiring is pinned
- workflow-pins suite green, including the input-docs drift harness (new knob documented in both tables)
- Lints: check_workflow_pins.py, check_agents_md.py, check-org-repo-literals.sh all pass
- Not run: a live caller run of the reusable
Deviations:
- Part 2 (auto re-review on full thread resolution) is not implemented; see Residual
- The cloud caller follow-up is not done (separate repo)

🤖 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/358 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/7/2026 **Status:** ✅ Merged **Merged:** 10/7/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-19479-cursor-review-approve-authors` --- ### 📝 Commits (3) - [`65a46cc`](https://github.com/Comfy-Org/github-workflows/commit/65a46ccd6831d67acdb767044544440269f67dbe) feat(cursor-review): per-author auto-approve opt-in (approve_authors) - [`2b6117c`](https://github.com/Comfy-Org/github-workflows/commit/2b6117c4eac9e053eec806ac724413bd1dc7cf15) fix(cursor-review): fail closed on approve_authors edge cases - [`130836d`](https://github.com/Comfy-Org/github-workflows/commit/130836dc9e948fe2fec337155866cbd656827c60) docs(cursor-review): document approve_authors withdrawal and fail-closed list (BE-19479) ### 📊 Changes **5 files changed** (+301 additions, -15 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+1 -0) 📝 `.github/cursor-review/auto-approve.py` (+45 -8) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+172 -1) 📝 `.github/workflows/cursor-review.yml` (+71 -5) 📝 `docs/callers/cursor-review.md` (+12 -1) </details> ### 📄 Description ## ELI-5 Auto-approve in `cursor-review.yml` is currently all-or-nothing for a caller repo. This adds an `approve_authors` list, so a repo can turn auto-approve on for a few PR authors first. PRs by anyone else still get the full review and its threads, but the bot doesn't approve or request changes on them. ## What changed - **New input `approve_authors`** (string, default `''`): a comma- or space-separated list of GitHub logins. Matching ignores case and a leading `@`. An empty list means every author, which is today's behaviour. - **Resolved once, in the `gate` job.** A new first step writes `approve_author_ok` (`true`/`false`). `gate` runs on every event, so the workflow-level `approve_gate` output reports `off` for an unlisted author whether or not a round runs. It doesn't fall back to `untrusted` on no-round events. - **`decide` (`auto-approve.py`)** has new `--author-enabled` / `--pr-author` flags. When the author isn't listed it writes `approve_gate=off`, posts no APPROVE and no REQUEST_CHANGES, and emits the note *"auto-approve not enabled for author `<login>`"*. An invalid `approve_max_severity` still fails red. - **`post-review.py`** gets an empty approve threshold for an unlisted author. Without that, it would label threads "not blocking auto-approve" for a decision that is never taken. - **Dismiss stale auto-approval** is unchanged, so it still withdraws earlier marked approvals for every author. - `post-review` now lists `gate` in `needs:` so it can read the output. `gate` always runs and succeeds, so this adds no ordering and no skip path. - Docs: input table row and an auto-approve paragraph in `docs/callers/cursor-review.md`, the `off` row of the `approve_gate` table, and the knob table in `.github/cursor-review/README.md`. ## Judgment calls - **No withdrawal in `decide` for an unlisted author.** This matches what an empty `approve_max_severity` does: `decide` doesn't run at all, and stale approvals are left to dismiss-stale. If an author is removed from the list mid-PR, an approval already on the current head stays until the next push. Unsetting `approve_max_severity` behaves the same way today. - **Only the shell step parses the list**, and the parse is tested by running that step from the YAML against a case table. Python gets the resolved boolean, so the parsing logic exists in one place. ## Residual - **Part 2 is not in this PR: the automatic re-review when every panel thread is resolved after a REQUEST_CHANGES** (the new `pull_request_review_thread: resolved` path). It needs these six conditions: - auto-approve is enabled for the PR (`approve_max_severity` set and the author passes `approve_authors`); - the approver identity's latest decision is REQUEST_CHANGES, read live; - zero unresolved panel-poster threads remain, read live across all pages; - no round has run yet for the current `(head SHA, set of resolved thread ids)` marker; - the `max_rounds` cap isn't reached; - the PR is open, not a draft, and carries no veto label. - It also needs the status line "All review threads resolved — starting round N", tests per condition plus firing exactly once per marker plus human threads not blocking, and caller docs. - I left it out because it is a design change, not a wiring change. Three existing invariants have to be reworked together: - **Duplicate check.** The `gate` job skips any head SHA that already has a consolidated review. "Explain, then resolve" pushes no commit, so a re-review on the same SHA must bypass that check. The resolved-set marker would replace it. - **Concurrency.** The reusable's `concurrency:` block deliberately keeps thread events out of the `trigger` slot so that resolving a thread never cancels a panel. Sharing the slot with label-triggered rounds, as the ticket asks, means making `cancel-in-progress` depend on the event, and pending runs of the two kinds can then replace each other. - **Caller groups.** The documented caller groups (PR-number-only, plus the per-run thread slot for blocking callers) decide which of these runs ever reach the reusable. - Getting that wrong either destroys running panels or loops rounds. It is roughly another 300+ lines across the workflow, `auto-approve.py` and the tests, which is too much to add to this unattended PR. - The ticket asks for the direct in-reusable path to be preferred over the App-token label toggle. Both paths hit the same-SHA duplicate check, so whoever picks this up should decide that first. - **The Comfy-Org/cloud caller follow-up is not done.** It needs `approve_authors: ${{ vars.CURSOR_APPROVE_AUTHORS }}` and the variable set to the DRI's login. It's a separate repo and is out of scope for this PR. - Not exercised: a live caller run. Verification is unit- and workflow-text-level only (see below). ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: cursor-review suite green (1001), including new `ApproveAuthorsTest`: an unlisted author gets `off` and no review event, a listed or empty-list author gets the normal APPROVE / REQUEST_CHANGES, the gate step's parse is run from the YAML against 11 cases, and the wiring is pinned - workflow-pins suite green, including the input-docs drift harness (new knob documented in both tables) - Lints: `check_workflow_pins.py`, `check_agents_md.py`, `check-org-repo-literals.sh` all pass - Not run: a live caller run of the reusable - **Deviations:** - Part 2 (auto re-review on full thread resolution) is not implemented; see Residual - The cloud caller follow-up is not done (separate repo) 🤖 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:38:59 +00:00