[PR #272] [MERGED] fix(cursor-review): confirm the first review is absent before tagging findings lost_to_fallback #274

New issue

Closed

opened 2026-10-26 22:37:58 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:58 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-12528-confirm-review-absent

📝 Commits (3)

dbf920b fix(cursor-review): confirm the first review is absent before tagging findings lost_to_fallback (BE-12528)
2f35f40 fix(cursor-review): identify the landed review by body, not by the panel marker (BE-12528)
fbcbb7f fix(cursor-review): close the round-2 panel findings on the landed-review check (BE-12528)

📊 Changes

3 files changed (+779 additions, -20 deletions)

View changed files

📝 .github/cursor-review/post-review.py (+303 -14)
📝 .github/cursor-review/tests/test_post_review.py (+467 -4)
📝 .github/workflows/cursor-review.yml (+9 -2)

📄 Description

ELI-5

When the reviewer bot posts its review, GitHub sometimes answers with an error even
though it already saved the review. Until now the script believed the error, posted a
second review, and labelled every finding "lost — the post failed". Next round then
saw each finding twice: once with its real comment thread, once with a note saying
nobody could possibly have replied to it.

Now the script checks before it decides. If GitHub rejected the request (a 4xx), the
review really is absent and nothing changes. If the error left the outcome genuinely
open (a 5xx, or the connection dropped), it reads the PR's reviews and looks: review
there → report it delivered and post nothing more; review absent → behave exactly as
before; couldn't read the list → still post the fallback, but label nothing, because a
read that failed is not the same as an answer of "no".

What changed

.github/cursor-review/post-review.py

New helpers beside gh_post_review / is_read_only_token_error:
- CONSOLIDATED_MARKER — the panel discriminator, mirroring gate-unresolved.py's
  constant (and the inline jq in the workflow's already_reviewed dup-check). A
  test pins the two equal, the same way test_build_ledger.py pins the ledger's copy.
- gh_http_status(result) — the status gh prints on stderr ((HTTP 422)), or
  None when it printed none.
- gh_list_reviews(repo, pr) — gh api --paginate --slurp /repos/…/pulls/N/reviews.
  Paginated because the review being asked about is the newest one, so on a busy PR it
  sits on the last page of an oldest-first list; the workflow's dup-check is the
  same discriminator without pagination, which it can afford because it only has to
  notice a review that already exists.
- review_already_posted(...) -> True | False | None — three-valued on purpose.
  None (list unreadable/unparseable) must never collapse into False; a guard that
  cannot read its input must not answer a zero.
main()'s wholesale-fallback branch now resolves landed before building anything:
a 4xx short-circuits to False with no API call, everything else asks the PR.
landed is True → finish_posted_review() and return. landed is None → log it and
continue untagged. lost_ids gains the landed is False conjunct.
The success tail (emit_delivery(...) + the clamp's summary write) is extracted into
a local finish_posted_review() closure, so the two paths that leave this body on
the PR report it through one implementation rather than two that can drift.
The in-code Residual (BE-10002) paragraph is replaced by a note describing the three
outcomes; the Every finding of the round is OFFERED to it comment now says the tag
is applied only when the first review is confirmed absent.

.github/workflows/cursor-review.yml — comment only. The post-review job header
now reads "one POST, plus at most one paginated GET of the PR's reviews on a non-4xx
POST failure". No permission change: pull-requests: write already implies the read,
and timeout-minutes: 10 is ample.

Tests — EndToEndPostTest.run_main gains existing_reviews / list_returncode and
always patches gh_list_reviews (default [[]], i.e. confirmed-absent), so no case
in this suite can shell out to a real gh. It also resets _DELIVERY_EMITTED and
captures $GITHUB_OUTPUT via a temp file, with list_calls / outputs / notes
out-parameters. Eight new cases in FirstReviewConfirmationTest cover: 4xx tags without
reading; 5xx-absent tags; 5xx-present posts once and reports delivered=true,
gated_findings=2, exit 0; a human author / another commit / a DISMISSED review each
fail the discriminator; an unreadable list posts untagged with
post_failed_count == 0; a status-less transport error goes through the read; the
marker matches the gate's; and gh_http_status parses gh stderr.

Acceptance criteria

Criterion	Status
A 422 first POST behaves byte-for-byte as before	✅ verified empirically, see below
A 5xx whose review is on the PR → one review, `delivered=true`, no fallback, exit 0	✅ `test_a_5xx_with_the_review_present_skips_the_fallback`
A 5xx with the review absent behaves as the 422 case	✅ `test_a_5xx_with_the_review_confirmed_absent_tags_post_failed`
A failed list read still posts the fallback, every finding `[unanchorable]`, `post_failed_count == 0`	✅ `test_an_unreadable_review_list_posts_the_fallback_untagged`

The byte-for-byte claim is measured, not asserted. I drove origin/main's
post-review.py and this branch's over the same fixture (one anchored + one demoted
finding, 422 stderr) with a stubbed POST, and diffed the JSON payloads: identical. The
same harness on a 502 shows old main posting 2 reviews where this branch posts 1.

The GET was exercised against the real API, not only against my own stubs. Running
gh_list_reviews / review_already_posted read-only against this repo's public PR #263
confirmed: --slurp returns a list-of-pages that flattens as written (12 reviews, 1
page); real payloads carry commit_id, state, user.type and body exactly as the
discriminator reads them; and the function answered True for the SHA that genuinely
carries a marked Bot panel review (a57f1e5) and False for that PR's actual head
SHA, which does not. That is the one part of this change a unit test could only assert
against its own premise.

Residual

A prior round's review is no longer mistaken for this one — the residual is now
narrower. The panel (6 of 6 reviewers) was right that
body.startswith(CONSOLIDATED_MARKER) identifies "some bot panel review exists at
this SHA", not this run's: a previous round's body-only fallback and any
post_error_review body both open with the marker, are Bot-authored and carry the
same commit_id. Matching one returned True, suppressing the fallback and reporting
delivered=true over another round's threads — trading a duplicate review for a
silent loss of this round's findings. The discriminator now requires the stored
body to BE posted_body, up to the CRLF/trailing-whitespace normalization GitHub
applies. What remains is "a prior round posted a byte-identical BODY at the same head
SHA". Round 2 correctly pushed back on my first wording of this: that is not the
same as "the same findings". The body carries the header, the counts, the severity
table and the body-only section; each inline finding's path, line and text lives in
payload["comments"], which this comparison never sees — so two rounds sharing a
count and severity mix could match while differing inline. Narrower than the marker
match by a wide margin, but not nil. Closing it properly means identifying the review
by something the POST would have to return (the review id it created), which it
cannot when it errors; a body digest is the workaround, and it changes the body every
reader and the ledger parser sees, so it wants its own round.
The check errs toward the pre-change behaviour, not past it. If GitHub ever
stored a body this normalization does not reconcile, the answer is False, which is
exactly what main does today (duplicate posted, findings tagged lost). A strictness
miss costs the fix's benefit; it cannot cost more than the status quo.
gh_list_reviews is now bounded (GH_LIST_REVIEWS_TIMEOUT_SECONDS = 60,
degrading to UNKNOWN). It was left unbounded to match gh_post_review; the panel
pointed out the asymmetry that matters — this read sits AHEAD of the fallback POST and
write_step_summary, so a hang bounded only by timeout-minutes: 10 takes the round
out of both channels, where the pre-BE-12528 code posted the fallback immediately.
gh_post_review keeps its own behaviour: nothing runs after it that a hang could cost.
The 4xx short-circuit is now restricted to pre-write rejections. 408/425/429 are
4xx only in the sense that an edge or a proxy said so, possibly about a request the
API went on to serve, so they take the read like a 5xx (RETRYABLE_4XX_STATUSES).
What still rests on a documented property of GitHub's API rather than on an
observation I could make from here is the remaining premise: that a review payload is
validated before anything is committed. It is the same premise BE-10002's
not comments branch already relies on, and it fails in the safe direction (a 4xx
that had written would produce today's duplicate, not something worse). 403 never
reaches it: is_read_only_token_error intercepts that earlier.
UNKNOWN still posts the fallback, and one snapshot can miss a late write. Raised
by the panel; answered rather than changed. Declining to repost on UNKNOWN would lose
the round from the PR altogether, which is worse than the duplicate it avoids, and a
retry loop cannot close a window that has no bound. The lost_to_fallback tag is
already withheld on that branch, so what remains possible is an extra review — never
a false claim about one.
Not exercised: a live cursor-review run. Every assertion here comes from the unit
suite, the payload diff against origin/main, and read-only calls to the reviews API.
A real 5xx from GitHub's review POST is not reproducible on demand, so the
PRESENT/UNKNOWN branches have never executed against a genuine failure — only against
faithful stubs and real API response shapes.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py': 440 passed, 0 failed; .github/agents-md-integrity/tests: 46 passed; .github/groom/tests: 374 passed, 1 skipped; shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh: clean; check_agents_md.py --root .: passed (2 pre-existing warnings); cursor-review.yml re-parsed as YAML. The 422 payload-diff harness was re-run against origin/main after the review fixes: 2 payloads each, byte-identical, zero list reads on that path. All ten fixes across both review rounds are mutation-checked — reverting any one of them turns a test red.
Deviations: four round-2 findings were answered on their threads rather than in code, each with the reasoning recorded there: the not comments branch not consulting the landed-check (a gate-behaviour change, and a fix-the-fix at low severity); --jq/page-cap byte bounding (the MemoryError it guards is not reachable at any PR size this runs on, and --jq would change the one payload shape verified against the real API); the body-vs-inline-comments gap in the residual (wording corrected above instead); and the throttled-403 path, which is a change to the read-only degradation guard rather than to this one and is filed as a follow-up.

_{🔄 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/272 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/8/2026 **Status:** ✅ Merged **Merged:** 9/8/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-12528-confirm-review-absent` --- ### 📝 Commits (3) - [`dbf920b`](https://github.com/Comfy-Org/github-workflows/commit/dbf920b27df9dbe742cd74764a3c9db53cf9e042) fix(cursor-review): confirm the first review is absent before tagging findings lost_to_fallback (BE-12528) - [`2f35f40`](https://github.com/Comfy-Org/github-workflows/commit/2f35f4009964622d9f327830311e741361a221a0) fix(cursor-review): identify the landed review by body, not by the panel marker (BE-12528) - [`fbcbb7f`](https://github.com/Comfy-Org/github-workflows/commit/fbcbb7f1546856d0e3d63ba9771e206edeee7018) fix(cursor-review): close the round-2 panel findings on the landed-review check (BE-12528) ### 📊 Changes **3 files changed** (+779 additions, -20 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/post-review.py` (+303 -14) 📝 `.github/cursor-review/tests/test_post_review.py` (+467 -4) 📝 `.github/workflows/cursor-review.yml` (+9 -2) </details> ### 📄 Description ## ELI-5 When the reviewer bot posts its review, GitHub sometimes answers with an error even though it already saved the review. Until now the script believed the error, posted a *second* review, and labelled every finding "lost — the post failed". Next round then saw each finding twice: once with its real comment thread, once with a note saying nobody could possibly have replied to it. Now the script checks before it decides. If GitHub *rejected* the request (a 4xx), the review really is absent and nothing changes. If the error left the outcome genuinely open (a 5xx, or the connection dropped), it reads the PR's reviews and looks: review there → report it delivered and post nothing more; review absent → behave exactly as before; couldn't read the list → still post the fallback, but label nothing, because a read that failed is not the same as an answer of "no". ## What changed **`.github/cursor-review/post-review.py`** - New helpers beside `gh_post_review` / `is_read_only_token_error`: - `CONSOLIDATED_MARKER` — the panel discriminator, mirroring `gate-unresolved.py`'s constant (and the inline `jq` in the workflow's `already_reviewed` dup-check). A test pins the two equal, the same way `test_build_ledger.py` pins the ledger's copy. - `gh_http_status(result)` — the status `gh` prints on stderr (`(HTTP 422)`), or `None` when it printed none. - `gh_list_reviews(repo, pr)` — `gh api --paginate --slurp /repos/…/pulls/N/reviews`. Paginated because the review being asked about is the newest one, so on a busy PR it sits on the **last** page of an oldest-first list; the workflow's dup-check is the same discriminator *without* pagination, which it can afford because it only has to notice a review that already exists. - `review_already_posted(...) -> True | False | None` — three-valued on purpose. `None` (list unreadable/unparseable) must never collapse into `False`; a guard that cannot read its input must not answer a zero. - `main()`'s wholesale-fallback branch now resolves `landed` before building anything: a 4xx short-circuits to `False` with no API call, everything else asks the PR. `landed is True` → `finish_posted_review()` and return. `landed is None` → log it and continue untagged. `lost_ids` gains the `landed is False` conjunct. - The success tail (`emit_delivery(...)` + the clamp's summary write) is extracted into a local `finish_posted_review()` closure, so the two paths that leave *this* body on the PR report it through one implementation rather than two that can drift. - The in-code `Residual (BE-10002)` paragraph is replaced by a note describing the three outcomes; the `Every finding of the round is OFFERED to it` comment now says the tag is applied only when the first review is confirmed **absent**. **`.github/workflows/cursor-review.yml`** — comment only. The `post-review` job header now reads "one POST, plus at most one paginated GET of the PR's reviews on a non-4xx POST failure". No permission change: `pull-requests: write` already implies the read, and `timeout-minutes: 10` is ample. **Tests** — `EndToEndPostTest.run_main` gains `existing_reviews` / `list_returncode` and **always** patches `gh_list_reviews` (default `[[]]`, i.e. confirmed-absent), so no case in this suite can shell out to a real `gh`. It also resets `_DELIVERY_EMITTED` and captures `$GITHUB_OUTPUT` via a temp file, with `list_calls` / `outputs` / `notes` out-parameters. Eight new cases in `FirstReviewConfirmationTest` cover: 4xx tags without reading; 5xx-absent tags; 5xx-present posts once and reports `delivered=true`, `gated_findings=2`, exit 0; a human author / another commit / a DISMISSED review each fail the discriminator; an unreadable list posts untagged with `post_failed_count == 0`; a status-less transport error goes through the read; the marker matches the gate's; and `gh_http_status` parses `gh` stderr. ## Acceptance criteria | Criterion | Status | |---|---| | A 422 first POST behaves **byte-for-byte** as before | ✅ verified empirically, see below | | A 5xx whose review is on the PR → one review, `delivered=true`, no fallback, exit 0 | ✅ `test_a_5xx_with_the_review_present_skips_the_fallback` | | A 5xx with the review absent behaves as the 422 case | ✅ `test_a_5xx_with_the_review_confirmed_absent_tags_post_failed` | | A failed list read still posts the fallback, every finding `[unanchorable]`, `post_failed_count == 0` | ✅ `test_an_unreadable_review_list_posts_the_fallback_untagged` | **The byte-for-byte claim is measured, not asserted.** I drove `origin/main`'s `post-review.py` and this branch's over the same fixture (one anchored + one demoted finding, 422 stderr) with a stubbed POST, and diffed the JSON payloads: identical. The same harness on a 502 shows old `main` posting 2 reviews where this branch posts 1. **The GET was exercised against the real API, not only against my own stubs.** Running `gh_list_reviews` / `review_already_posted` read-only against this repo's public PR #263 confirmed: `--slurp` returns a list-of-pages that flattens as written (12 reviews, 1 page); real payloads carry `commit_id`, `state`, `user.type` and `body` exactly as the discriminator reads them; and the function answered **True** for the SHA that genuinely carries a marked Bot panel review (`a57f1e5`) and **False** for that PR's actual head SHA, which does not. That is the one part of this change a unit test could only assert against its own premise. ## Residual - **A prior round's review is no longer mistaken for this one — the residual is now narrower.** The panel (6 of 6 reviewers) was right that `body.startswith(CONSOLIDATED_MARKER)` identifies "some bot panel review exists at this SHA", not this run's: a previous round's body-only fallback and any `post_error_review` body both open with the marker, are Bot-authored and carry the same `commit_id`. Matching one returned True, suppressing the fallback and reporting `delivered=true` over another round's threads — trading a duplicate review for a *silent loss* of this round's findings. The discriminator now requires the stored body to BE `posted_body`, up to the CRLF/trailing-whitespace normalization GitHub applies. What remains is "a prior round posted a byte-identical BODY at the same head SHA". Round 2 correctly pushed back on my first wording of this: that is *not* the same as "the same findings". The body carries the header, the counts, the severity table and the body-only section; each inline finding's path, line and text lives in `payload["comments"]`, which this comparison never sees — so two rounds sharing a count and severity mix could match while differing inline. Narrower than the marker match by a wide margin, but not nil. Closing it properly means identifying the review by something the POST would have to return (the review id it created), which it cannot when it errors; a body digest is the workaround, and it changes the body every reader and the ledger parser sees, so it wants its own round. - **The check errs toward the pre-change behaviour, not past it.** If GitHub ever stored a body this normalization does not reconcile, the answer is `False`, which is exactly what `main` does today (duplicate posted, findings tagged lost). A strictness miss costs the fix's benefit; it cannot cost more than the status quo. - **`gh_list_reviews` is now bounded** (`GH_LIST_REVIEWS_TIMEOUT_SECONDS = 60`, degrading to UNKNOWN). It was left unbounded to match `gh_post_review`; the panel pointed out the asymmetry that matters — this read sits AHEAD of the fallback POST and `write_step_summary`, so a hang bounded only by `timeout-minutes: 10` takes the round out of both channels, where the pre-BE-12528 code posted the fallback immediately. `gh_post_review` keeps its own behaviour: nothing runs after it that a hang could cost. - **The 4xx short-circuit is now restricted to pre-write rejections.** 408/425/429 are 4xx only in the sense that an edge or a proxy said so, possibly about a request the API went on to serve, so they take the read like a 5xx (`RETRYABLE_4XX_STATUSES`). What still rests on a documented property of GitHub's API rather than on an observation I could make from here is the remaining premise: that a review payload is validated before anything is committed. It is the same premise BE-10002's `not comments` branch already relies on, and it fails in the safe direction (a 4xx that *had* written would produce today's duplicate, not something worse). `403` never reaches it: `is_read_only_token_error` intercepts that earlier. - **UNKNOWN still posts the fallback, and one snapshot can miss a late write.** Raised by the panel; answered rather than changed. Declining to repost on UNKNOWN would lose the round from the PR altogether, which is worse than the duplicate it avoids, and a retry loop cannot close a window that has no bound. The `lost_to_fallback` tag is already withheld on that branch, so what remains possible is an extra review — never a false claim about one. - **Not exercised: a live cursor-review run.** Every assertion here comes from the unit suite, the payload diff against `origin/main`, and read-only calls to the reviews API. A real 5xx from GitHub's review POST is not reproducible on demand, so the PRESENT/UNKNOWN branches have never executed against a genuine failure — only against faithful stubs and real API *response shapes*. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'`: 440 passed, 0 failed; `.github/agents-md-integrity/tests`: 46 passed; `.github/groom/tests`: 374 passed, 1 skipped; `shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh`: clean; `check_agents_md.py --root .`: passed (2 pre-existing warnings); `cursor-review.yml` re-parsed as YAML. The 422 payload-diff harness was re-run against `origin/main` after the review fixes: 2 payloads each, byte-identical, zero list reads on that path. All ten fixes across both review rounds are mutation-checked — reverting any one of them turns a test red. - **Deviations:** four round-2 findings were answered on their threads rather than in code, each with the reasoning recorded there: the `not comments` branch not consulting the landed-check (a gate-behaviour change, and a fix-the-fix at low severity); `--jq`/page-cap byte bounding (the `MemoryError` it guards is not reachable at any PR size this runs on, and `--jq` would change the one payload shape verified against the real API); the body-vs-inline-comments gap in the residual (wording corrected above instead); and the throttled-403 path, which is a change to the read-only degradation guard rather than to this one and is filed as a follow-up. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:58 +00:00