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

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/346
Author: @mattmillerai
Created: 10/6/2026
Status: ✅ Merged
Merged: 10/7/2026
Merged by: @mattmillerai

Base: main ← Head: matt/dismiss-stale-live-head

📝 Commits (9)

e28d1ba fix(cursor-review): announce a degraded live-head read in dismiss-stale
648c540 Merge origin/main into matt/dismiss-stale-live-head
41d497d fix(cursor-review): guard and announce an empty live base too
7d4b467 fix(cursor-review): make every dismiss-stale annotation mode-accurate
9767d4d fix(cursor-review): no payload shape reaches a caller as a traceback
7b9e77d Merge origin/main (#345) into matt/dismiss-stale-live-head
b2706b7 fix(cursor-review): single-line cmd_decide errors; keep a headless read's base
edb1fde fix(cursor-review): treat an edited auto-approval as stale (#347)
dc73a0c fix(cursor-review): close the remaining payload-shape escapes in auto-approve

📊 Changes

2 files changed (+848 additions, -65 deletions)

View changed files

📝 .github/cursor-review/auto-approve.py (+325 -43)
📝 .github/cursor-review/tests/test_auto_approve.py (+523 -22)

📄 Description

ELI5

A robot reviewer can approve a pull request, and a separate job withdraws that approval whenever new code is pushed. To decide, that job asks GitHub what the newest commit is — and if the question fails, it quietly falls back to a possibly older answer it was handed, which can withdraw an approval that was perfectly valid. The job still finishes green either way, so nobody could tell. Now it says so out loud.

Motivation

cmd_dismiss_stale was changed in #342 — fix(cursor-review): judge auto-approval staleness by the reviewed SHA — to compare against the live head rather than github.event.pull_request.head.sha, because a queued or redelivered synchronize replays an older head and would withdraw an approval valid for the current one. That change kept a deliberate fallback to the event head when the live read fails, which is the right call: degrading the comparison beats skipping the scan. What it did not do is say anything when that happens. A dismissal on the degraded path succeeds, so the job exits green, and the only two signals a reader gets — a green check and a "dismissed N stale review(s)" line — are identical whether the comparison used the real head or a replayed one. A review thread on #342 raised the underlying asymmetry; this closes the remaining half of it.

Provenance

Authored by: interactive session
Verified:
- python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' (what test-cursor-review-scripts.yml runs): 884 tests OK at the current head
- The regression tests added for the review rounds fail without their fixes and pass with them. That includes the round-5 set: CRLF banners, a POST without an id, non-list labels and pages, malformed review nodes, a non-SHA event head, and the annotation clamp
- check_workflow_pins.py, check-org-repo-literals.sh and check_agents_md.py are clean
- The branch merges cleanly into current main
Deviations:
- The PR grew past "only add a warning". Review rounds added the empty-live-base guard, payload-shape coercion across every read, single-line clamped annotations, keeping a headless read's base, and (via #347, merged into this branch) treating an edited approval as stale. See Summary
- Review findings that belong to other surfaces are deferred to follow-ups rather than fixed here (see Not in scope)

Reviewer context

Type: bug fix — observability on a security-relevant fallback, plus narrow behaviour fixes on degraded payloads (an empty live base no longer withdraws every approval; a malformed payload no longer crashes with a traceback)
Slots into: follows #342 — fix(cursor-review): judge auto-approval staleness by the reviewed SHA, which is merged; this is the leftover diagnostic from its review. No stacked PRs.
Not in scope (deferred to follow-ups): when the live read fails, the base axis still fails open: an off-base approval survives that run, and the fix needs a --base-ref fallback or a fail-closed decision. withdraw_own_approvals has no unactionable-approval gate after an identity rotation. And a round cap that labels needs-human-review does not withdraw an approval already standing on the same head (the cap path is from #345).
Folded in from #347: the other open finding from that review was that the recorded SHA lives in the review body, which a user with write access can edit, so a rewritten marker kept a stale approval alive. #347 was merged into this branch: the reviews list now comes from GraphQL for lastEditedAt, and an edited approval is always treated as stale.

Summary

cmd_dismiss_stale names both degraded live-head cases: a raised RuntimeError/ValueError, and a read that returns no head. On either it prints a ::warning:: with the cause, the repo and PR, the head actually compared against, and whether the base check ran. Both still fall back to args.head_sha. A read that succeeded but had no head keeps the base it did return.
A retarget (--all-approvals) with no readable head and no event head still withdraws everything rather than going red, and its warning does not claim a head comparison.
An empty live base is normalised to None and announced. "" matches no recorded base, so before this it silently withdrew every approval that had recorded one.
read_pr, pr_head_base, has_label, list_reviews and _paginate either coerce a malformed payload to the empty value the announced fallbacks already handle, or raise ValueError into the caller's error path. A malformed review node now raises instead of being silently dropped. Before this, those shapes escaped every except as a traceback.
A review POST whose response has no parseable id takes the ambiguous-POST path: approve_gate untrusted, own approvals withdrawn, red.
Dismiss-stale falls back only to an event head that is a full 40-hex SHA. round_reviews recognises the "reviewed nothing" banners in a CRLF-stored body.
annotation_cause turns every interpolated exception in auto-approve.py's dismiss and withdraw paths into a single-line, %-escaped cause, clamped to 500 characters. That includes the two cmd_decide sites, so DISMISS_PERMISSION_HINT stays inside the annotation.
A clean read warns about nothing, so the happy path stays quiet.

Test plan

unittest + shellcheck job green on this PR
On a PR where the approver's token cannot read the head (or by temporarily pointing the read at a nonexistent PR), confirm the job logs the warning, still runs the scan, and exits green
Confirm a normal synchronize with a readable head produces no warning and dismisses as before
Not feature-flagged: this adds a log line on an existing fallback path and changes no decision the job makes

🤖 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/346 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/6/2026 **Status:** ✅ Merged **Merged:** 10/7/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/dismiss-stale-live-head` --- ### 📝 Commits (9) - [`e28d1ba`](https://github.com/Comfy-Org/github-workflows/commit/e28d1ba5b987e15003ce8cbb880a41972469e071) fix(cursor-review): announce a degraded live-head read in dismiss-stale - [`648c540`](https://github.com/Comfy-Org/github-workflows/commit/648c5403118a83429501003dcc5924aeca73aa87) Merge origin/main into matt/dismiss-stale-live-head - [`41d497d`](https://github.com/Comfy-Org/github-workflows/commit/41d497ddfc1efe9bfe7e7153082ed5f662599ba6) fix(cursor-review): guard and announce an empty live base too - [`7d4b467`](https://github.com/Comfy-Org/github-workflows/commit/7d4b467760d25ec678c04aed3161adb648ab907f) fix(cursor-review): make every dismiss-stale annotation mode-accurate - [`9767d4d`](https://github.com/Comfy-Org/github-workflows/commit/9767d4d30b6f7173e70a886a5c9cbc7bca2e1470) fix(cursor-review): no payload shape reaches a caller as a traceback - [`7b9e77d`](https://github.com/Comfy-Org/github-workflows/commit/7b9e77d7d0d0d106e2e9e9ad249dac6b90afa8d6) Merge origin/main (#345) into matt/dismiss-stale-live-head - [`b2706b7`](https://github.com/Comfy-Org/github-workflows/commit/b2706b7b4c38b17492b037cada10066b05b47696) fix(cursor-review): single-line cmd_decide errors; keep a headless read's base - [`edb1fde`](https://github.com/Comfy-Org/github-workflows/commit/edb1fdeba0ab7263cbea8aa9d0c8cc14e6ba5894) fix(cursor-review): treat an edited auto-approval as stale (#347) - [`dc73a0c`](https://github.com/Comfy-Org/github-workflows/commit/dc73a0c3e7faf1715bc840c503f1752de4626815) fix(cursor-review): close the remaining payload-shape escapes in auto-approve ### 📊 Changes **2 files changed** (+848 additions, -65 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/auto-approve.py` (+325 -43) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+523 -22) </details> ### 📄 Description ## ELI5 A robot reviewer can approve a pull request, and a separate job withdraws that approval whenever new code is pushed. To decide, that job asks GitHub what the newest commit is — and if the question fails, it quietly falls back to a possibly older answer it was handed, which can withdraw an approval that was perfectly valid. The job still finishes green either way, so nobody could tell. Now it says so out loud. ## Motivation `cmd_dismiss_stale` was changed in #342 — fix(cursor-review): judge auto-approval staleness by the reviewed SHA — to compare against the live head rather than `github.event.pull_request.head.sha`, because a queued or redelivered `synchronize` replays an older head and would withdraw an approval valid for the current one. That change kept a deliberate fallback to the event head when the live read fails, which is the right call: degrading the comparison beats skipping the scan. What it did not do is say anything when that happens. A dismissal on the degraded path succeeds, so the job exits green, and the only two signals a reader gets — a green check and a "dismissed N stale review(s)" line — are identical whether the comparison used the real head or a replayed one. A review thread on #342 raised the underlying asymmetry; this closes the remaining half of it. ## Provenance - **Authored by:** interactive session - **Verified:** - `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'` (what `test-cursor-review-scripts.yml` runs): 884 tests OK at the current head - The regression tests added for the review rounds fail without their fixes and pass with them. That includes the round-5 set: CRLF banners, a POST without an id, non-list labels and pages, malformed review nodes, a non-SHA event head, and the annotation clamp - `check_workflow_pins.py`, `check-org-repo-literals.sh` and `check_agents_md.py` are clean - The branch merges cleanly into current `main` - **Deviations:** - The PR grew past "only add a warning". Review rounds added the empty-live-base guard, payload-shape coercion across every read, single-line clamped annotations, keeping a headless read's base, and (via #347, merged into this branch) treating an edited approval as stale. See Summary - Review findings that belong to other surfaces are deferred to follow-ups rather than fixed here (see Not in scope) ## Reviewer context - **Type:** bug fix — observability on a security-relevant fallback, plus narrow behaviour fixes on degraded payloads (an empty live base no longer withdraws every approval; a malformed payload no longer crashes with a traceback) - **Slots into:** follows #342 — fix(cursor-review): judge auto-approval staleness by the reviewed SHA, which is merged; this is the leftover diagnostic from its review. No stacked PRs. - **Not in scope (deferred to follow-ups):** when the live read fails, the base axis still fails open: an off-base approval survives that run, and the fix needs a `--base-ref` fallback or a fail-closed decision. `withdraw_own_approvals` has no unactionable-approval gate after an identity rotation. And a round cap that labels `needs-human-review` does not withdraw an approval already standing on the same head (the cap path is from #345). - **Folded in from #347:** the other open finding from that review was that the recorded SHA lives in the review body, which a user with write access can edit, so a rewritten marker kept a stale approval alive. #347 was merged into this branch: the reviews list now comes from GraphQL for `lastEditedAt`, and an edited approval is always treated as stale. ## Summary - `cmd_dismiss_stale` names both degraded live-head cases: a raised `RuntimeError`/`ValueError`, and a read that returns no head. On either it prints a `::warning::` with the cause, the repo and PR, the head actually compared against, and whether the base check ran. Both still fall back to `args.head_sha`. A read that succeeded but had no head keeps the base it did return. - A retarget (`--all-approvals`) with no readable head and no event head still withdraws everything rather than going red, and its warning does not claim a head comparison. - An empty live base is normalised to `None` and announced. `""` matches no recorded base, so before this it silently withdrew every approval that had recorded one. - `read_pr`, `pr_head_base`, `has_label`, `list_reviews` and `_paginate` either coerce a malformed payload to the empty value the announced fallbacks already handle, or raise `ValueError` into the caller's error path. A malformed review node now raises instead of being silently dropped. Before this, those shapes escaped every `except` as a traceback. - A review POST whose response has no parseable id takes the ambiguous-POST path: `approve_gate` untrusted, own approvals withdrawn, red. - Dismiss-stale falls back only to an event head that is a full 40-hex SHA. `round_reviews` recognises the "reviewed nothing" banners in a CRLF-stored body. - `annotation_cause` turns every interpolated exception in `auto-approve.py`'s dismiss and withdraw paths into a single-line, `%`-escaped cause, clamped to 500 characters. That includes the two `cmd_decide` sites, so `DISMISS_PERMISSION_HINT` stays inside the annotation. - A clean read warns about nothing, so the happy path stays quiet. ## Test plan - [ ] `unittest + shellcheck` job green on this PR - [ ] On a PR where the approver's token cannot read the head (or by temporarily pointing the read at a nonexistent PR), confirm the job logs the warning, still runs the scan, and exits green - [ ] Confirm a normal `synchronize` with a readable head produces no warning and dismisses as before - [ ] Not feature-flagged: this adds a log line on an existing fallback path and changes no decision the job makes 🤖 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:52 +00:00