[PR #343] [MERGED] fix(cursor-review): close four ways a stale or unearned auto-approval keeps counting #344

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-19297-auto-approve-gaps

📝 Commits (3)

fed980f fix(cursor-review): close four ways a stale or unearned auto-approval keeps counting
74c7386 fix(cursor-review): read approval staleness from the PR's live head and base
53dfd3c Merge origin/main: reconcile #342's reviewed-SHA anchor with the base record

📊 Changes

9 files changed (+699 additions, -101 deletions)

View changed files

📝 .github/cursor-review/README.md (+1 -1)
📝 .github/cursor-review/auto-approve.py (+171 -56)
📝 .github/cursor-review/tests/test_auto_approve.py (+199 -12)
➕ .github/detect-unreviewed-merge/tests/approval.test.cjs (+102 -0)
📝 .github/workflows/cursor-review.yml (+76 -18)
📝 .github/workflows/detect-unreviewed-merge.yml (+48 -4)
➕ .github/workflows/test-detect-unreviewed-merge.yml (+27 -0)
📝 docs/callers/cursor-review.md (+62 -10)
📝 docs/callers/detect-unreviewed-merge.md (+13 -0)

📄 Description

Closes four gaps in the opt-in auto-approve added by #340. In each one, a bot approval that is stale or was never earned keeps counting on rulesets with dismiss_stale_reviews_on_push: false. A review panel found them on the first caller to enable approve_max_severity. Until this merges, callers should not set that input.

Changes

Unsetting the input no longer stops dismissals. dismiss-stale-approval used to require inputs.approve_max_severity != ''. It now runs for every same-repo pull_request event of an open PR. It only acts on reviews that have the auto-approve marker and come from the approver identity. On a repo that never auto-approved, it lists the reviews, finds none of its own, and does nothing.
- Because the job now runs for every caller, the App-token mint only runs when BOT_APP_PRIVATE_KEY is present (the same BOT_CONFIGURED pattern over-cap-comment uses). Without this, every bot_app_id caller would get a red job on Dependabot pushes, which don't receive Actions secrets. If bot_app_id is set but the key is missing, the job runs read-only with an empty approver login: it dismisses nothing, and it goes red if any stale marked approval exists. More generally, a stale marked approval by any login the run can't act as goes red instead of passing unchecked. That covers APPROVER_TOKEN being unavailable on a Dependabot PR and a rotated approver identity.
Dismissal reads the PR's live state, not the event.
- Each auto-approve review records the base branch it was reviewed against, alongside the reviewed SHA that #342 (now merged in) records. dismiss-stale dismisses any marked approval whose recorded SHA isn't the live head, or whose recorded base differs from the live base. A retarget therefore dismisses the on-head approval too: the head didn't move, but the diff did.
- Deviation from the spec: the job runs on every action of an open PR (state guard; closed and merged PRs are skipped), not only synchronize/reopened/edited. Any event in the same concurrency slot can cancel an in-flight dismissal. Because staleness comes from live state, whichever run comes next redoes it.
- edited with changes.base still passes --all-approvals, for approvals posted before the base was recorded.
- If the live PR head can't be read, dismiss-stale falls back to the event's head (--head-sha, kept from #342) and skips the base check for that run; the next event redoes it from live state. With no event head either, or if the reviews can't be listed, it prints a clean ::error:: instead of a traceback.
- decide takes --base-ref (the round's base). It refuses to approve if the base was retargeted mid-round, and the post-write re-read withdraws the review if the head or base moved.
An empty reviewed diff no longer earns an APPROVE. decide reads the reviewed diff (--reviewed-diff /tmp/pr-diff.patch, the same artifact post-review already downloads). If it has no @@ content hunk, the round counts as untrusted: no review is posted and earlier approvals are withdrawn. That covers diffs that are empty or fully excluded by diff_excludes or the generated-file classifier, plus pure renames, mode changes and binary-only changes. A missing patch is treated the same way (fail closed).
detect-unreviewed-merge can ignore automated approvers. New ignore-approvers input: logins separated by whitespace or commas, case-insensitive, [bot] suffix included. Reviews by those logins are removed before both any-approval and latest-per-reviewer check for an approval. When the list is set, a review with no usable login (a deleted account) doesn't count either. A warning fires when every counted approval came from a Bot account. The input defaults to '', so existing callers see no change.

Docs:

docs/callers/cursor-review.md:
- the caller example's types: now includes reopened and edited;
- new sections on the empty-diff rule, the dismissal events and why dismissal isn't tied to approve_max_severity;
- a concurrency snippet so edited doesn't cancel a panel on widened callers;
- the known residual where a PR can drop the trigger;
- a pointer to ignore-approvers.
docs/callers/detect-unreviewed-merge.md: inputs-table row and a section on automated approvers.
.github/cursor-review/README.md: row updated.

Tests

.github/cursor-review/tests/test_auto_approve.py:
- the empty-diff decision;
- reviewed_diff_is_empty on empty, rename, binary and missing patches;
- an excluded-only round posts nothing and withdraws a round-1 approval;
- a retarget dismisses the approval on the current head, a push keeps it;
- a later event without BASE_FROM redoes a cancelled retarget dismissal, using the recorded base;
- a stale approval the run can't act on goes red; with nothing stale, the job stays clean;
- a base retargeted before or during the post withholds or withdraws the approval;
- list failures go red cleanly;
- workflow-text assertions that the dismiss job's if: doesn't mention approve_max_severity or filter by action, requires an open PR, and that decide gets --base-ref.
New .github/detect-unreviewed-merge/tests/approval.test.cjs plus test-detect-unreviewed-merge.yml. The test runs the shipped github-script, extracted by sentinels the same way the assign-reviewers harness does it, against API fixtures in both modes. Also covered: a review with no usable login, and the bot-only warning. A mutation check (removing the filter) made 4 of the original 11 tests fail.

Verified locally, all green:

cursor-review suite: 781 tests;
workflow-pins suite (inputs-docs drift harness included);
check_workflow_pins.py;
check_agents_md.py;
the org-repo-literal lint;
node tests: 15/15;
YAML parses;
shellcheck on the extracted dismiss step.

Residual

Bumping the first caller's pin is not done here. It needs a separate PR in that repo (the cursor-review and detect-unreviewed-merge bumpers may do it). That caller also has to:
- add reopened and edited to its types:;
- pass its approver identity as ignore-approvers to its detect-unreviewed-merge caller.
A PR can still drop the trigger. pull_request runs the caller from the PR head, so a commit can remove synchronize (or the caller itself) and no dismissal runs. This is documented, not fixed, as the ticket asked. Closing it needs a dismiss-only pull_request_target job driven by base-controlled code.
Partial exclusion still approves. If a PR has reviewable content plus changes to excluded paths, it can still be approved even though nobody saw the excluded content. The ticket only covered the empty or fully-excluded case. Closing this would mean refusing approval whenever any changed path was excluded, and the generated-file classifier also drops paths, so which exclusions should block needs a decision first. Proposed as a follow-up.
Changing the approver identity orphans old approvals. If a caller changes APPROVER_TOKEN or bot_app_id while approvals are live, the new identity can't dismiss the old identity's approvals. The job goes red on the next stale one until someone dismisses it by hand. This is documented in the guide.
Approvals posted before the base was recorded are caught on a retarget only by that retarget's own edited run. None exist yet, since no caller is enabled.
Not exercised: the original review thread on the first caller's PR (I only had the summary) and any live run against a real ruleset. Verification here is unit- and fixture-level only.

Provenance

Authored by: agent-work loop
Verified:
- cursor-review unittest suite (781) and workflow-pins suite (502) pass at the current head, after merging main (#341, #342)
- node fixture tests for the shipped detect-unreviewed-merge script pass (15/15)
- check_workflow_pins.py, check_agents_md.py and the org-repo-literal lint pass
- shellcheck on the extracted dismiss step, and a YAML parse of both changed workflows
- no live Actions run against a real ruleset
Deviations:
- the dismiss job runs on every action of an open PR and reads the live head and base, not only on synchronize/reopened/edited
- the App-token mint in the dismiss job is conditional on the key; without it, the job checks read-only and goes red over a stale approval it can't dismiss
- an unreadable live head falls back to the event's head (from #342) and skips the base check for that run, instead of going red

🤖 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/343 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/6/2026 **Status:** ✅ Merged **Merged:** 10/6/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-19297-auto-approve-gaps` --- ### 📝 Commits (3) - [`fed980f`](https://github.com/Comfy-Org/github-workflows/commit/fed980fd49084d5cfcef34472f6bdb7ccaf76792) fix(cursor-review): close four ways a stale or unearned auto-approval keeps counting - [`74c7386`](https://github.com/Comfy-Org/github-workflows/commit/74c7386c9b992205e299769ee8abd5354d52d7e9) fix(cursor-review): read approval staleness from the PR's live head and base - [`53dfd3c`](https://github.com/Comfy-Org/github-workflows/commit/53dfd3c6736902bee181ee5091c577769311605e) Merge origin/main: reconcile #342's reviewed-SHA anchor with the base record ### 📊 Changes **9 files changed** (+699 additions, -101 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+1 -1) 📝 `.github/cursor-review/auto-approve.py` (+171 -56) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+199 -12) ➕ `.github/detect-unreviewed-merge/tests/approval.test.cjs` (+102 -0) 📝 `.github/workflows/cursor-review.yml` (+76 -18) 📝 `.github/workflows/detect-unreviewed-merge.yml` (+48 -4) ➕ `.github/workflows/test-detect-unreviewed-merge.yml` (+27 -0) 📝 `docs/callers/cursor-review.md` (+62 -10) 📝 `docs/callers/detect-unreviewed-merge.md` (+13 -0) </details> ### 📄 Description Closes four gaps in the opt-in auto-approve added by #340. In each one, a bot approval that is stale or was never earned keeps counting on rulesets with `dismiss_stale_reviews_on_push: false`. A review panel found them on the first caller to enable `approve_max_severity`. Until this merges, callers should not set that input. ## Changes 1. **Unsetting the input no longer stops dismissals.** `dismiss-stale-approval` used to require `inputs.approve_max_severity != ''`. It now runs for every same-repo `pull_request` event of an open PR. It only acts on reviews that have the auto-approve marker *and* come from the approver identity. On a repo that never auto-approved, it lists the reviews, finds none of its own, and does nothing. - Because the job now runs for every caller, the App-token mint only runs when `BOT_APP_PRIVATE_KEY` is present (the same `BOT_CONFIGURED` pattern `over-cap-comment` uses). Without this, every `bot_app_id` caller would get a red job on Dependabot pushes, which don't receive Actions secrets. If `bot_app_id` is set but the key is missing, the job runs read-only with an empty approver login: it dismisses nothing, and it goes red if any stale marked approval exists. More generally, a stale marked approval by any login the run can't act as goes red instead of passing unchecked. That covers `APPROVER_TOKEN` being unavailable on a Dependabot PR and a rotated approver identity. 2. **Dismissal reads the PR's live state, not the event.** - Each auto-approve review records the base branch it was reviewed against, alongside the reviewed SHA that #342 (now merged in) records. `dismiss-stale` dismisses any marked approval whose recorded SHA isn't the live head, or whose recorded base differs from the live base. A retarget therefore dismisses the on-head approval too: the head didn't move, but the diff did. - **Deviation from the spec:** the job runs on every action of an open PR (state guard; closed and merged PRs are skipped), not only `synchronize`/`reopened`/`edited`. Any event in the same concurrency slot can cancel an in-flight dismissal. Because staleness comes from live state, whichever run comes next redoes it. - `edited` with `changes.base` still passes `--all-approvals`, for approvals posted before the base was recorded. - If the live PR head can't be read, `dismiss-stale` falls back to the event's head (`--head-sha`, kept from #342) and skips the base check for that run; the next event redoes it from live state. With no event head either, or if the reviews can't be listed, it prints a clean `::error::` instead of a traceback. - `decide` takes `--base-ref` (the round's base). It refuses to approve if the base was retargeted mid-round, and the post-write re-read withdraws the review if the head or base moved. 3. **An empty reviewed diff no longer earns an APPROVE.** `decide` reads the reviewed diff (`--reviewed-diff /tmp/pr-diff.patch`, the same artifact `post-review` already downloads). If it has no `@@` content hunk, the round counts as untrusted: no review is posted and earlier approvals are withdrawn. That covers diffs that are empty or fully excluded by `diff_excludes` or the generated-file classifier, plus pure renames, mode changes and binary-only changes. A missing patch is treated the same way (fail closed). 4. **`detect-unreviewed-merge` can ignore automated approvers.** New `ignore-approvers` input: logins separated by whitespace or commas, case-insensitive, `[bot]` suffix included. Reviews by those logins are removed before *both* `any-approval` and `latest-per-reviewer` check for an approval. When the list is set, a review with no usable login (a deleted account) doesn't count either. A warning fires when every counted approval came from a `Bot` account. The input defaults to `''`, so existing callers see no change. Docs: - `docs/callers/cursor-review.md`: - the caller example's `types:` now includes `reopened` and `edited`; - new sections on the empty-diff rule, the dismissal events and why dismissal isn't tied to `approve_max_severity`; - a concurrency snippet so `edited` doesn't cancel a panel on widened callers; - the known residual where a PR can drop the trigger; - a pointer to `ignore-approvers`. - `docs/callers/detect-unreviewed-merge.md`: inputs-table row and a section on automated approvers. - `.github/cursor-review/README.md`: row updated. ## Tests - `.github/cursor-review/tests/test_auto_approve.py`: - the empty-diff decision; - `reviewed_diff_is_empty` on empty, rename, binary and missing patches; - an excluded-only round posts nothing and withdraws a round-1 approval; - a retarget dismisses the approval on the current head, a push keeps it; - a later event without `BASE_FROM` redoes a cancelled retarget dismissal, using the recorded base; - a stale approval the run can't act on goes red; with nothing stale, the job stays clean; - a base retargeted before or during the post withholds or withdraws the approval; - list failures go red cleanly; - workflow-text assertions that the dismiss job's `if:` doesn't mention `approve_max_severity` or filter by action, requires an open PR, and that `decide` gets `--base-ref`. - New `.github/detect-unreviewed-merge/tests/approval.test.cjs` plus `test-detect-unreviewed-merge.yml`. The test runs the **shipped** github-script, extracted by sentinels the same way the assign-reviewers harness does it, against API fixtures in both modes. Also covered: a review with no usable login, and the bot-only warning. A mutation check (removing the filter) made 4 of the original 11 tests fail. Verified locally, all green: - cursor-review suite: 781 tests; - workflow-pins suite (inputs-docs drift harness included); - `check_workflow_pins.py`; - `check_agents_md.py`; - the org-repo-literal lint; - node tests: 15/15; - YAML parses; - `shellcheck` on the extracted dismiss step. ## Residual - **Bumping the first caller's pin is not done here.** It needs a separate PR in that repo (the cursor-review and detect-unreviewed-merge bumpers may do it). That caller also has to: - add `reopened` and `edited` to its `types:`; - pass its approver identity as `ignore-approvers` to its `detect-unreviewed-merge` caller. - **A PR can still drop the trigger.** `pull_request` runs the caller from the PR head, so a commit can remove `synchronize` (or the caller itself) and no dismissal runs. This is documented, not fixed, as the ticket asked. Closing it needs a dismiss-only `pull_request_target` job driven by base-controlled code. - **Partial exclusion still approves.** If a PR has reviewable content *plus* changes to excluded paths, it can still be approved even though nobody saw the excluded content. The ticket only covered the empty or fully-excluded case. Closing this would mean refusing approval whenever any changed path was excluded, and the generated-file classifier also drops paths, so which exclusions should block needs a decision first. Proposed as a follow-up. - **Changing the approver identity orphans old approvals.** If a caller changes `APPROVER_TOKEN` or `bot_app_id` while approvals are live, the new identity can't dismiss the old identity's approvals. The job goes red on the next stale one until someone dismisses it by hand. This is documented in the guide. - **Approvals posted before the base was recorded** are caught on a retarget only by that retarget's own `edited` run. None exist yet, since no caller is enabled. - **Not exercised:** the original review thread on the first caller's PR (I only had the summary) and any live run against a real ruleset. Verification here is unit- and fixture-level only. ## Provenance - **Authored by:** agent-work loop - **Verified:** - cursor-review unittest suite (781) and workflow-pins suite (502) pass at the current head, after merging main (#341, #342) - node fixture tests for the shipped detect-unreviewed-merge script pass (15/15) - `check_workflow_pins.py`, `check_agents_md.py` and the org-repo-literal lint pass - `shellcheck` on the extracted dismiss step, and a YAML parse of both changed workflows - no live Actions run against a real ruleset - **Deviations:** - the dismiss job runs on every action of an open PR and reads the live head and base, not only on `synchronize`/`reopened`/`edited` - the App-token mint in the dismiss job is conditional on the key; without it, the job checks read-only and goes red over a stale approval it can't dismiss - an unreadable live head falls back to the event's head (from #342) and skips the base check for that run, instead of going red 🤖 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:45 +00:00