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 #407] [MERGED] feat(cursor-review): let a maintainer ask for another round on the same commit #406

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/cursor-review-rereview-command

📝 Commits (3)

08cea43 feat(cursor-review): let a maintainer ask for another round on the same commit
091a5ff fix(cursor-review): rereview_command — open PRs only, retry the re-add, page every review, first line before trim
b32e82d fix(cursor-review): rereview_command — no issues grant, prefilter comments, wait out the remove

📊 Changes

6 files changed (+722 additions, -11 deletions)

View changed files

📝 .github/cursor-review/README.md (+8 -2)
➕ .github/cursor-review/tests/test_rereview_command.py (+407 -0)
📝 .github/workflows/cursor-review-auto-label.yml (+140 -0)
📝 .github/workflows/cursor-review.yml (+80 -4)
📝 docs/callers/cursor-review-auto-label.md (+1 -0)
📝 docs/callers/cursor-review.md (+86 -5)

📄 Description

The problem

A round can reach no decision for a reason that has nothing to do with the code — most often reviewer cells erroring, which approve_max_failed_reviewers counts. The answer is to run it again. There was no way to.

The dedupe in Check for prior review on this commit keys on a non-dismissed Bot review carrying the consolidated marker at the current HEAD SHA:

select(.commit_id == $sha and .state != "DISMISSED" and .user.type == "Bot"
       and (.body | startswith("## 🔍 Cursor Review — Consolidated panel")))

Both documented escape hatches fail for exactly the case that wants one:

hatch	why it fails here
push a commit	the code is fine — there is nothing to push
dismiss the existing review	impossible. That review is posted `COMMENTED`, and GitHub dismisses only `APPROVED` and `CHANGES_REQUESTED` reviews

Across 30 recent consumer PRs the panel review is COMMENTED 8 times out of 8 and has never once been DISMISSED — consistent with the dismissal simply not being available. This repo nevertheless prescribed it in three places. The dedupe step's own comment already admitted the symptom: "it also made the documented remedy for a genuine error review (re-trigger on the same SHA) a silent no-op".

What this adds

rereview_command — default '', so every existing caller is byte-identical. Set it to /review on both workflows and a maintainer comments /review:

error: no decision — 2 of 6 reviewers errored
/review                      ← maintainer
→ next round runs on the same commit

Two halves, because one cannot do it. cursor-review.yml is reachable only by a label: 49 of its expressions read github.event.pull_request, so a caller cannot invoke it from an issue_comment event at all. cursor-review-auto-label.yml already exists to turn an event into that label with an App token, so the command becomes a new moment there — and it removes before re-adding, because adding a label that is already present raises no event, and after a round it always is. The gate half then declines to dedupe. Setting one half alone is a silent no-op, which both input descriptions say and a test pins.

One request buys one round. The signal is a command comment newer than the review it overrides. The round it starts posts a newer review, so the next run dedupes again and a /review from three rounds ago can never re-fire. No new label, no state to clean up. max_rounds is untouched.

Safety

Authorization is the comment's own association (OWNER / MEMBER / COLLABORATOR), the bar /derisk applies. The opted-in roster is deliberately not consulted — it governs who gets labelled automatically, not who may ask for a re-read.
The comment body reaches the shell only through env:, never interpolated into a run:. A test asserts ${{ appears nowhere in that script, because ${{ github.event.comment.body }} inside a run: is a shell-injection hole.
Nothing from the body is echoed — it is attacker-authored text and these logs may be public.
Matched as the first token of the first line: /review please look again counts, /reviewer does not, and quoting someone else's /review further down a comment does nothing.
Comments on plain issues are ignored; skip-cursor-review still wins, and so does a closed or merged PR: no label is touched unless the PR is OPEN.
After the remove, the re-add is retried 3 times. If all of them fail, the step fails loudly instead of quietly leaving the PR unlabeled.
The gate reads every page of reviews (--paginate) before it picks the newest round. It takes the first line before trimming, so a body that opens with a blank line cannot move /review up from line two.

Two bugs the tests caught before this shipped, both mine

["OWNER",…] | index(.author_association) — the pipe rebinds . to the literal array, so the association was read off that array, and every authorized comment errored rather than counting. The failure pattern gave it away: only cases that got past the first select failed.
"" | split("\n") is [], so [0] was null and the next sub died on any empty, whitespace-only or missing comment body — i.e. on most real PRs.

Neither was reachable by reading the filter; both fell out of running the real jq program against the payload shapes it will actually meet.

Tests

36 new, stdlib-only (no yaml — CI installs no requirements). The jq program and the first-token pipeline are extracted from the workflows and executed, not copied, so the tests cannot drift from what ships: 23 payload shapes through the real filter, and the awk token match run as the step runs it.

All 10 mutations of the new guards are caught. One assertion initially passed for the wrong reason — it matched the words issues: read inside the step's own explanatory comment, so deleting the actual permission still passed — and is now line-anchored.

Suites: cursor-review 1452, cursor-approve 236, workflow-pins 551, pins lint and AGENTS.md integrity clean. The repo's own inputs-docs drift test required the new knob in the README table, which is how I learned I had missed it.

Docs

The three places prescribing the impossible dismissal are corrected rather than left to mislead, and both caller pages plus the README knob table document the input and the both-halves requirement.

Not in scope

No consumer caller is changed here — enabling this on a repo is a separate PR there, and needs issue_comment added to the auto-label caller's triggers.

Provenance

Verified:

cursor-review suite (1452 tests) passes at the current head; it includes the review-round tests for the open-PR guard, retry, pagination and blank-first-line cases
check_workflow_pins.py, check_agents_md.py and the org-repo-literal lint pass
the reviews-slurp fallback was exercised by hand against empty, multi-page and garbage input

Deviations:

none

🤖 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/407 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/10/2026 **Status:** ✅ Merged **Merged:** 10/10/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/cursor-review-rereview-command` --- ### 📝 Commits (3) - [`08cea43`](https://github.com/Comfy-Org/github-workflows/commit/08cea43a9eeb986ac03b486699f87fbb4b5d9232) feat(cursor-review): let a maintainer ask for another round on the same commit - [`091a5ff`](https://github.com/Comfy-Org/github-workflows/commit/091a5ff3eedfd5ac1664935376bb4854c8b73102) fix(cursor-review): rereview_command — open PRs only, retry the re-add, page every review, first line before trim - [`b32e82d`](https://github.com/Comfy-Org/github-workflows/commit/b32e82d09b9f73b48a8c9ed1f55de684ccc2f8d3) fix(cursor-review): rereview_command — no issues grant, prefilter comments, wait out the remove ### 📊 Changes **6 files changed** (+722 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+8 -2) ➕ `.github/cursor-review/tests/test_rereview_command.py` (+407 -0) 📝 `.github/workflows/cursor-review-auto-label.yml` (+140 -0) 📝 `.github/workflows/cursor-review.yml` (+80 -4) 📝 `docs/callers/cursor-review-auto-label.md` (+1 -0) 📝 `docs/callers/cursor-review.md` (+86 -5) </details> ### 📄 Description ## The problem A round can reach no decision for a reason that has nothing to do with the code — most often reviewer cells erroring, which `approve_max_failed_reviewers` counts. The answer is to run it again. **There was no way to.** The dedupe in `Check for prior review on this commit` keys on a non-dismissed Bot review carrying the consolidated marker at the current HEAD SHA: ```jq select(.commit_id == $sha and .state != "DISMISSED" and .user.type == "Bot" and (.body | startswith("## 🔍 Cursor Review — Consolidated panel"))) ``` Both documented escape hatches fail for exactly the case that wants one: | hatch | why it fails here | |---|---| | push a commit | the code is fine — there is nothing to push | | dismiss the existing review | **impossible.** That review is posted `COMMENTED`, and GitHub dismisses only `APPROVED` and `CHANGES_REQUESTED` reviews | Across 30 recent consumer PRs the panel review is `COMMENTED` 8 times out of 8 and has never once been `DISMISSED` — consistent with the dismissal simply not being available. This repo nevertheless prescribed it in **three** places. The dedupe step's own comment already admitted the symptom: *"it also made the documented remedy for a genuine error review (re-trigger on the same SHA) a silent no-op"*. ## What this adds `rereview_command` — default `''`, so every existing caller is byte-identical. Set it to `/review` on both workflows and a maintainer comments `/review`: ``` error: no decision — 2 of 6 reviewers errored /review ← maintainer → next round runs on the same commit ``` **Two halves, because one cannot do it.** `cursor-review.yml` is reachable only by a label: 49 of its expressions read `github.event.pull_request`, so a caller cannot invoke it from an `issue_comment` event at all. `cursor-review-auto-label.yml` already exists to turn an event into that label with an App token, so the command becomes a new *moment* there — and it **removes before re-adding**, because adding a label that is already present raises no event, and after a round it always is. The gate half then declines to dedupe. Setting one half alone is a silent no-op, which both input descriptions say and a test pins. **One request buys one round.** The signal is a command comment *newer than the review it overrides*. The round it starts posts a newer review, so the next run dedupes again and a `/review` from three rounds ago can never re-fire. No new label, no state to clean up. `max_rounds` is untouched. ## Safety - Authorization is the comment's own association (`OWNER` / `MEMBER` / `COLLABORATOR`), the bar `/derisk` applies. The opted-in roster is deliberately *not* consulted — it governs who gets labelled automatically, not who may ask for a re-read. - The comment body reaches the shell only through `env:`, **never** interpolated into a `run:`. A test asserts `${{` appears nowhere in that script, because `${{ github.event.comment.body }}` inside a `run:` is a shell-injection hole. - Nothing from the body is echoed — it is attacker-authored text and these logs may be public. - Matched as the **first token of the first line**: `/review please look again` counts, `/reviewer` does not, and quoting someone else's `/review` further down a comment does nothing. - Comments on plain issues are ignored; `skip-cursor-review` still wins, and so does a closed or merged PR: no label is touched unless the PR is `OPEN`. - After the remove, the re-add is retried 3 times. If all of them fail, the step fails loudly instead of quietly leaving the PR unlabeled. - The gate reads **every page** of reviews (`--paginate`) before it picks the newest round. It takes the first line *before* trimming, so a body that opens with a blank line cannot move `/review` up from line two. ## Two bugs the tests caught before this shipped, both mine 1. **`["OWNER",…] | index(.author_association)`** — the pipe rebinds `.` to the literal array, so the association was read off *that array*, and every authorized comment errored rather than counting. The failure pattern gave it away: only cases that got past the first `select` failed. 2. **`"" | split("\n")` is `[]`**, so `[0]` was `null` and the next `sub` died on any empty, whitespace-only or missing comment body — i.e. on most real PRs. Neither was reachable by reading the filter; both fell out of running the real jq program against the payload shapes it will actually meet. ## Tests 36 new, stdlib-only (no `yaml` — CI installs no requirements). The jq program and the first-token pipeline are **extracted from the workflows and executed**, not copied, so the tests cannot drift from what ships: 23 payload shapes through the real filter, and the `awk` token match run as the step runs it. All **10 mutations** of the new guards are caught. One assertion initially passed for the wrong reason — it matched the words `issues: read` inside the step's own explanatory comment, so deleting the actual permission still passed — and is now line-anchored. Suites: cursor-review **1452**, cursor-approve **236**, workflow-pins **551**, pins lint and AGENTS.md integrity clean. The repo's own inputs-docs drift test required the new knob in the README table, which is how I learned I had missed it. ## Docs The three places prescribing the impossible dismissal are corrected rather than left to mislead, and both caller pages plus the README knob table document the input and the both-halves requirement. ## Not in scope No consumer caller is changed here — enabling this on a repo is a separate PR there, and needs `issue_comment` added to the auto-label caller's triggers. ## Provenance **Verified:** - cursor-review suite (1452 tests) passes at the current head; it includes the review-round tests for the open-PR guard, retry, pagination and blank-first-line cases - `check_workflow_pins.py`, `check_agents_md.py` and the org-repo-literal lint pass - the reviews-slurp fallback was exercised by hand against empty, multi-page and garbage input **Deviations:** - none 🤖 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 added the

pull-request

label

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

bsakaguchi referenced this issue

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

[PR #406] [MERGED] ci: bump cursor-review to github-workflows@fe43805 #408

bsakaguchi closed this issue

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