fix(cursor-review): confirm the first review is absent before tagging findings lost_to_fallback - #272
Merged
Merged
Conversation
… findings lost_to_fallback (BE-12528) A nonzero `gh` exit on the review POST is not proof the review was never committed server-side. BE-10002 tagged every anchored finding `lost_to_fallback` on that exit alone and wrote the hole down as a residual: when the write HAD landed, the fallback posted a second review and the next round's ledger carried each anchored finding twice — once with its real thread, once as a cap-exempt [post-failed] entry claiming nobody could have answered it. The failure path now establishes the answer, cheapest sufficient evidence first. A 4xx is GitHub validating and rejecting before writing (every firing observed in the field is a 422 over an inline position), so the review is absent by construction and no read is spent. Anything else — a 5xx, or a transport error carrying no status at all — takes one paginated GET of the PR's reviews, matched on the same four-part discriminator the blocking gate and the workflow's dup-check use. Paginated because the newest review sits on the last page of an oldest-first list. Three outcomes: PRESENT reports the review delivered and posts nothing more (the success tail is now a shared `finish_posted_review` closure, so both paths that leave a body on the PR report it through one implementation); ABSENT is this path exactly as before; UNKNOWN posts the fallback but tags nothing, because an unreadable list is not a confirmed absence (BE-4785). The 422 path is byte-for-byte unchanged — verified by driving both the pre-change and post-change modules over the same fixture and diffing the POST payloads.
mattmillerai
added
cursor-review
|
Warning Review limit reachedNext included review available in 4 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 103 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (3)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 6 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 1 |
| 🟢 Low | 4 |
Panel: 6/6 reviewers contributed findings.
…nel marker (BE-12528) Review-panel findings on #272, all against `review_already_posted`. - Identity, not family (6/6 reviewers). `body.startswith(CONSOLIDATED_MARKER)` answers "some bot panel review exists at this SHA" — which a previous round's body-only fallback and any `post_error_review` body both satisfy, being Bot-authored with the same `commit_id`. Matching one returned True, so the caller skipped the fallback and reported `delivered=true` while THIS round's findings reached neither the PR nor the job summary: a loose match traded a duplicate review for a silent loss. Now the stored body must BE `posted_body`, up to CRLF/trailing-whitespace normalization; the marker survives as a cheap prefix reject ahead of that (pinned by a test, since every head variant — attribution, `--notice`, `--ledger-note` — appends rather than prepends). - PENDING is not landed. `GET /pulls/{n}/reviews` returns the authenticated identity's own unsubmitted reviews, and that identity is the bot whose POST just errored — so the half-committed write this path detects could surface as PENDING, invisible to everyone and publishing no resolvable thread. Matched against a submitted-state allowlist instead of excluding DISMISSED alone. - A read that inspected nothing may not answer False (BE-4785). Exit-0 empty stdout no longer defaults to `[]`, and a payload that is not a list of pages is UNKNOWN rather than silently flattened to no reviews. - 408/425/429 take the read. The no-read short-circuit assumes a 4xx means "validated and refused before writing", which does not hold for statuses an edge or proxy can return for a request GitHub went on to serve. - The list read is bounded (60s) and degrades to UNKNOWN. It sits ahead of the fallback POST and the summary write, so an unbounded hang lost the round from both channels when `timeout-minutes: 10` fired. The 422 path is unchanged and re-verified byte-for-byte against origin/main, still spending no extra API call. Each fix is mutation-checked: reverting any one of them turns a test red.
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Round 2 — ledger: 6 prior finding(s) across 1 round(s) (0 never answered).
Found 8 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🟢 Low | 5 |
| ⚪ Nit | 1 |
Panel: 5/6 reviewers contributed findings.
Reviewers that did not contribute: gpt-5.6-sol-max:adversarial (error)
…view check (BE-12528) - `[]` no longer answers "confirmed absent". `all()` is vacuously true over it, so a zero-page payload fell through to `reviews = []` and tagged every anchored finding lost on a read that inspected no page — the same laundering the empty-stdout guard rejects. `[[]]`, what `--slurp` really returns for a PR with no reviews, remains the genuine absence. - The cheap prefix reject now runs on the NORMALIZED body. It ran on the raw one while the equality it guards normalized both sides, making the guard stricter than the check: a stored body differing only by a leading newline passed the equality but never reached it, answering "absent" for the run's own landed review. - Field types are trusted no further than payload shapes. A `user` that is a string, or a non-string `body`, raised AttributeError out of `review_already_posted` and killed the process ahead of both the fallback POST and `write_step_summary` — the both-channel loss the timeout exists to prevent. - A failed list read logs its exit code and stderr. UNKNOWN reposts the fallback and withholds the tag without explanation, so a 60s timeout, an auth failure and an older `gh` without `--slurp` were indistinguishable to an operator. - Comment correction: 403 is absent from RETRYABLE_4XX_STATUSES because `is_read_only_token_error` returns from `main()` before this decision is reached, not because a throttled 403 cannot happen. Listing it would be dead code that reads as coverage. Each fix is mutation-checked; reverting any one turns a test red.
Contributor
Author
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
The following carry
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.pygh_post_review/is_read_only_token_error:CONSOLIDATED_MARKER— the panel discriminator, mirroringgate-unresolved.py'sconstant (and the inline
jqin the workflow'salready_revieweddup-check). Atest pins the two equal, the same way
test_build_ledger.pypins the ledger's copy.gh_http_status(result)— the statusghprints on stderr ((HTTP 422)), orNonewhen 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 intoFalse; a guard thatcannot read its input must not answer a zero.
main()'s wholesale-fallback branch now resolveslandedbefore building anything:a 4xx short-circuits to
Falsewith no API call, everything else asks the PR.landed is True→finish_posted_review()and return.landed is None→ log it andcontinue untagged.
lost_idsgains thelanded is Falseconjunct.emit_delivery(...)+ the clamp's summary write) is extracted intoa local
finish_posted_review()closure, so the two paths that leave this body onthe PR report it through one implementation rather than two that can drift.
Residual (BE-10002)paragraph is replaced by a note describing the threeoutcomes; the
Every finding of the round is OFFERED to itcomment now says the tagis applied only when the first review is confirmed absent.
.github/workflows/cursor-review.yml— comment only. Thepost-reviewjob headernow 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: writealready implies the read,and
timeout-minutes: 10is ample.Tests —
EndToEndPostTest.run_maingainsexisting_reviews/list_returncodeandalways patches
gh_list_reviews(default[[]], i.e. confirmed-absent), so no casein this suite can shell out to a real
gh. It also resets_DELIVERY_EMITTEDandcaptures
$GITHUB_OUTPUTvia a temp file, withlist_calls/outputs/notesout-parameters. Eight new cases in
FirstReviewConfirmationTestcover: 4xx tags withoutreading; 5xx-absent tags; 5xx-present posts once and reports
delivered=true,gated_findings=2, exit 0; a human author / another commit / a DISMISSED review eachfail the discriminator; an unreadable list posts untagged with
post_failed_count == 0; a status-less transport error goes through the read; themarker matches the gate's; and
gh_http_statusparsesghstderr.Acceptance criteria
delivered=true, no fallback, exit 0test_a_5xx_with_the_review_present_skips_the_fallbacktest_a_5xx_with_the_review_confirmed_absent_tags_post_failed[unanchorable],post_failed_count == 0test_an_unreadable_review_list_posts_the_fallback_untaggedThe byte-for-byte claim is measured, not asserted. I drove
origin/main'spost-review.pyand this branch's over the same fixture (one anchored + one demotedfinding, 422 stderr) with a stubbed POST, and diffed the JSON payloads: identical. The
same harness on a 502 shows old
mainposting 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_postedread-only against this repo's public PR #263confirmed:
--slurpreturns a list-of-pages that flattens as written (12 reviews, 1page); real payloads carry
commit_id,state,user.typeandbodyexactly as thediscriminator 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 headSHA, which does not. That is the one part of this change a unit test could only assert
against its own premise.
Residual
narrower. The panel (6 of 6 reviewers) was right that
body.startswith(CONSOLIDATED_MARKER)identifies "some bot panel review exists atthis SHA", not this run's: a previous round's body-only fallback and any
post_error_reviewbody both open with the marker, are Bot-authored and carry thesame
commit_id. Matching one returned True, suppressing the fallback and reportingdelivered=trueover another round's threads — trading a duplicate review for asilent loss of this round's findings. The discriminator now requires the stored
body to BE
posted_body, up to the CRLF/trailing-whitespace normalization GitHubapplies. 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 acount 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.
stored a body this normalization does not reconcile, the answer is
False, which isexactly what
maindoes today (duplicate posted, findings tagged lost). A strictnessmiss costs the fix's benefit; it cannot cost more than the status quo.
gh_list_reviewsis now bounded (GH_LIST_REVIEWS_TIMEOUT_SECONDS = 60,degrading to UNKNOWN). It was left unbounded to match
gh_post_review; the panelpointed out the asymmetry that matters — this read sits AHEAD of the fallback POST and
write_step_summary, so a hang bounded only bytimeout-minutes: 10takes the roundout of both channels, where the pre-BE-12528 code posted the fallback immediately.
gh_post_reviewkeeps its own behaviour: nothing runs after it that a hang could cost.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 commentsbranch already relies on, and it fails in the safe direction (a 4xxthat had written would produce today's duplicate, not something worse).
403neverreaches it:
is_read_only_token_errorintercepts that earlier.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_fallbacktag isalready withheld on that branch, so what remains possible is an extra review — never
a false claim about one.
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
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.ymlre-parsed as YAML. The 422 payload-diff harness was re-run againstorigin/mainafter 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.not commentsbranch not consulting the landed-check (a gate-behaviour change, and a fix-the-fix at low severity);--jq/page-cap byte bounding (theMemoryErrorit guards is not reachable at any PR size this runs on, and--jqwould 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.