[PR #347] [MERGED] fix(cursor-review): treat an edited auto-approval as stale #346

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: matt/dismiss-stale-live-head ← Head: matt/distrust-edited-approvals

📝 Commits (5)

39baacc fix(cursor-review): treat an edited auto-approval as stale
72a06a8 Merge matt/dismiss-stale-live-head into matt/distrust-edited-approvals
88a49f0 fix(cursor-review): dismiss a marker-stripped edited approval; fail closed on a short reviews read
5b8860c Merge origin/matt/dismiss-stale-live-head into matt/distrust-edited-approvals
dc0a98c Merge origin/matt/dismiss-stale-live-head into matt/distrust-edited-approvals

📊 Changes

2 files changed (+351 additions, -60 deletions)

View changed files

📝 .github/cursor-review/auto-approve.py (+152 -36)
📝 .github/cursor-review/tests/test_auto_approve.py (+199 -24)

📄 Description

ELI5

A robot reviewer approves a pull request and writes down, inside its own comment, which commit it approved — and a separate job withdraws the approval once that commit is no longer the latest. The problem is that anyone with write access to the repo can edit that comment and change the number, which makes the approval look current forever. This reads an extra flag GitHub keeps — "has this comment been edited?" — and refuses to trust an approval that someone has touched.

Motivation

A cursor-review panel thread on #342 — fix(cursor-review): judge auto-approval staleness by the reviewed SHA — flagged this when that PR moved the staleness anchor out of the server-controlled commit_id and into the review body. The body is mutable: PATCH /repos/{owner}/{repo}/pulls/{pr}/reviews/{id} is available to the review's author, and GitHub lets write-access collaborators edit comments they did not write. The beneficiary is the PR author, and the prize is a standing approval that survives every later push — which is the entire thing dismiss-stale exists to deny. The thread also proposed cross-checking commit_id, which does not work here: for an approval GitHub advances commit_id to each new head, so a forged body marker pointing at the current head satisfies that check too. lastEditedAt is the one signal that separates a review this workflow wrote from one somebody has since altered.

Provenance

Authored by: interactive session
Verified:
- python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' → 873 tests OK at the current head (#346's base, including #345's round cap, merged in).
- workflow-pins unittest suite, check_workflow_pins.py, check_agents_md.py --root . and check-org-repo-literals.sh → clean.
- Live API: list_reviews over #342 read every review, with ids past 2^31-1 and the bot as github-actions[bot], same as REST.
- Negative checks: reverting the edited-distrust condition fails only its test, and so does dropping the [bot] restoration; without submittedAt/url the round-cap end-to-end test fails.
Deviations:
- The edited check sits inside #343's _stale_approvals (off-head test) rather than the old keep-condition, because the base reworked that filter.
- An edited approval counts even with its marker deleted, and a short, inconsistent or wrong-shaped GraphQL read raises (ValueError) where it used to return a partial list.
- Merging #346's base: its round cap (#345) also reads list_reviews, so the query now fetches submittedAt and url as well (mapped to submitted_at/html_url).

Reviewer context

Type: bug fix — closes a privileged bypass of dismiss-stale
Slots into: stacked on #346 — fix(cursor-review): announce a degraded live-head read in dismiss-stale, which is this PR's base; both touch cmd_dismiss_stale and its reviews list. Review #346 first; this PR's own changes are its two non-merge commits, plus the GraphQL field adaptation in its latest merge of #346. GitHub retargets this to main when #346 merges. Both are leftovers from #342's review.
Not in scope: the dismissal itself is still REST (PUT …/reviews/{id}/dismissals), which is why the list has to keep carrying a REST id rather than a node id.

Summary

list_reviews now reads GraphQL and returns the same REST shape its callers already expect (withdraw, dismiss-stale, and the base's round cap, which needs submitted_at and html_url), plus an edited flag. lastEditedAt has no REST equivalent on /pulls/{n}/reviews, which is the only reason for the switch.
An edited approval is stale, whatever its markers say, and that includes one whose marker was deleted. Only the approver login's reviews are dismissed. An unedited unmarked approval is still never touched, and an edited change request is still left alone, because only approvals are this filter's business. Another login's edited unmarked approval (a human editing their own) is not reported as unactionable.
Two translations the switch had to get right, each of which would otherwise have failed green: a Bot's [bot] suffix is restored from __typename (GraphQL drops it, and --approver-login carries it, so the identity match would have gone silently dead), and the id is read from fullDatabaseId because live review ids are past 2^31-1 while databaseId is typed Int.
A short read raises. That covers a missing pullRequest or reviews connection, a hasNextPage page with no new cursor, a response body of the wrong JSON shape, and a review with no id. Returning a partial list instead would read as "nothing to dismiss" and exit green, and since reviews come back oldest-first, a truncated read drops the newest approval.

Test plan

unittest + shellcheck job green on this PR
On a PR with a live auto-approval, edit the bot's review body (any edit) and confirm the next synchronize dismisses it and the job stays green
Confirm an untouched approval at the current head is still not dismissed, so normal rounds are unaffected
Confirm a PR with more than 100 reviews still lists them all (pagination)
Not feature-flagged: it tightens an existing guard rather than adding a surface, and the failure direction it removes is an approval that should have been withdrawn

🤖 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/347 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/6/2026 **Status:** ✅ Merged **Merged:** 10/7/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `matt/dismiss-stale-live-head` ← **Head:** `matt/distrust-edited-approvals` --- ### 📝 Commits (5) - [`39baacc`](https://github.com/Comfy-Org/github-workflows/commit/39baaccf9721ee46b58405994fceccc15ba4c43d) fix(cursor-review): treat an edited auto-approval as stale - [`72a06a8`](https://github.com/Comfy-Org/github-workflows/commit/72a06a8db6605e8c02b9b6e42a3baf1ff6d6fb0a) Merge matt/dismiss-stale-live-head into matt/distrust-edited-approvals - [`88a49f0`](https://github.com/Comfy-Org/github-workflows/commit/88a49f09c2ed7010a1a733620e8c68606e7b363a) fix(cursor-review): dismiss a marker-stripped edited approval; fail closed on a short reviews read - [`5b8860c`](https://github.com/Comfy-Org/github-workflows/commit/5b8860c1dc068cc9d334717f26c619073d2dc3e8) Merge origin/matt/dismiss-stale-live-head into matt/distrust-edited-approvals - [`dc0a98c`](https://github.com/Comfy-Org/github-workflows/commit/dc0a98c52088ca247909aefb16e3da9d53aca8ee) Merge origin/matt/dismiss-stale-live-head into matt/distrust-edited-approvals ### 📊 Changes **2 files changed** (+351 additions, -60 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/auto-approve.py` (+152 -36) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+199 -24) </details> ### 📄 Description ## ELI5 A robot reviewer approves a pull request and writes down, inside its own comment, which commit it approved — and a separate job withdraws the approval once that commit is no longer the latest. The problem is that anyone with write access to the repo can edit that comment and change the number, which makes the approval look current forever. This reads an extra flag GitHub keeps — "has this comment been edited?" — and refuses to trust an approval that someone has touched. ## Motivation A cursor-review panel thread on #342 — fix(cursor-review): judge auto-approval staleness by the reviewed SHA — flagged this when that PR moved the staleness anchor out of the server-controlled `commit_id` and into the review body. The body is mutable: `PATCH /repos/{owner}/{repo}/pulls/{pr}/reviews/{id}` is available to the review's author, and GitHub lets write-access collaborators edit comments they did not write. The beneficiary is the PR author, and the prize is a standing approval that survives every later push — which is the entire thing `dismiss-stale` exists to deny. The thread also proposed cross-checking `commit_id`, which does not work here: for an approval GitHub advances `commit_id` to each new head, so a forged body marker pointing at the current head satisfies that check too. `lastEditedAt` is the one signal that separates a review this workflow wrote from one somebody has since altered. ## Provenance - **Authored by:** interactive session - **Verified:** - `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'` → 873 tests OK at the current head (#346's base, including #345's round cap, merged in). - `workflow-pins` unittest suite, `check_workflow_pins.py`, `check_agents_md.py --root .` and `check-org-repo-literals.sh` → clean. - Live API: `list_reviews` over #342 read every review, with ids past 2^31-1 and the bot as `github-actions[bot]`, same as REST. - Negative checks: reverting the edited-distrust condition fails only its test, and so does dropping the `[bot]` restoration; without `submittedAt`/`url` the round-cap end-to-end test fails. - **Deviations:** - The edited check sits inside #343's `_stale_approvals` (off-head test) rather than the old keep-condition, because the base reworked that filter. - An edited approval counts even with its marker deleted, and a short, inconsistent or wrong-shaped GraphQL read raises (`ValueError`) where it used to return a partial list. - Merging #346's base: its round cap (#345) also reads `list_reviews`, so the query now fetches `submittedAt` and `url` as well (mapped to `submitted_at`/`html_url`). ## Reviewer context - **Type:** bug fix — closes a privileged bypass of `dismiss-stale` - **Slots into:** **stacked on #346 — fix(cursor-review): announce a degraded live-head read in dismiss-stale**, which is this PR's base; both touch `cmd_dismiss_stale` and its reviews list. Review #346 first; this PR's own changes are its two non-merge commits, plus the GraphQL field adaptation in its latest merge of #346. GitHub retargets this to `main` when #346 merges. Both are leftovers from #342's review. - **Not in scope:** the dismissal itself is still REST (`PUT …/reviews/{id}/dismissals`), which is why the list has to keep carrying a REST id rather than a node id. ## Summary - **`list_reviews` now reads GraphQL** and returns the same REST shape its callers already expect (`withdraw`, `dismiss-stale`, and the base's round cap, which needs `submitted_at` and `html_url`), plus an `edited` flag. `lastEditedAt` has no REST equivalent on `/pulls/{n}/reviews`, which is the only reason for the switch. - **An edited approval is stale**, whatever its markers say, and that includes one whose marker was deleted. Only the approver login's reviews are dismissed. An unedited unmarked approval is still never touched, and an edited *change request* is still left alone, because only approvals are this filter's business. Another login's edited unmarked approval (a human editing their own) is not reported as unactionable. - **Two translations the switch had to get right**, each of which would otherwise have failed green: a Bot's `[bot]` suffix is restored from `__typename` (GraphQL drops it, and `--approver-login` carries it, so the identity match would have gone silently dead), and the id is read from `fullDatabaseId` because live review ids are past 2^31-1 while `databaseId` is typed `Int`. - **A short read raises.** That covers a missing `pullRequest` or `reviews` connection, a `hasNextPage` page with no new cursor, a response body of the wrong JSON shape, and a review with no id. Returning a partial list instead would read as "nothing to dismiss" and exit green, and since reviews come back oldest-first, a truncated read drops the newest approval. ## Test plan - [ ] `unittest + shellcheck` job green on this PR - [ ] On a PR with a live auto-approval, edit the bot's review body (any edit) and confirm the next `synchronize` dismisses it and the job stays green - [ ] Confirm an untouched approval at the current head is still **not** dismissed, so normal rounds are unaffected - [ ] Confirm a PR with more than 100 reviews still lists them all (pagination) - [ ] Not feature-flagged: it tightens an existing guard rather than adding a surface, and the failure direction it removes is an approval that should have been withdrawn 🤖 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:38:52 +00:00

bsakaguchi referenced this issue

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

[PR #346] [MERGED] fix(cursor-review): announce a degraded live-head read in dismiss-stale #348

bsakaguchi closed this issue

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