fix(cursor-review): wait out a throttle before the landed-review read, and decline the fallback when the answer is still unknown

[mattmillerai]

STACKED — merging lands on matt/be-12612-narrow-read-only-403-guard (owned by @mattmillerai, PR #277), NOT main. Review it after #277; the base moves to main once #277 merges. Do not read anything below as "ready to merge" against main.

ELI-5

When GitHub tells the review bot "you're going too fast", the bot used to immediately ask GitHub "hey, did my review actually post?" — on the same connection GitHub had just told it to slow down on. That question usually got throttled too, so the bot got a shrug, assumed the worst, and posted the review again. If the first one had actually gone through, the PR now had two identical reviews and nobody could delete either.

Now the bot waits the amount of time GitHub asked for (capped at 90 seconds) before asking. And if it still can't get a straight answer, it stops writing: the findings go to the run's job summary instead, and the check goes red so nobody mistakes an unverified write for a delivered review.

What changed

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

• gh_post_review passes -i. gh api prints only the response body by default and offers no knob for a single header, so this is the only way Retry-After / X-RateLimit-* become reachable. Everything else about the call is unchanged (evidence below). The response goes to stdout, which nothing but the new parser reads.
• gh_response_headers(result) parses that stdout: requires the HTTP/x.y nnn status line, stops at the blank line (so the JSON body is never read as a header), splits on the first colon, lower-cases names. {} on anything else. It reads stdout rather than stderr deliberately — under GH_DEBUG=api stderr carries an echo of the POSTed review body, so scraping headers from there would let a finding that quotes a Retry-After: line dictate how long the process sleeps.
• is_throttle(result) — 429 by status, or a 403 carrying one of the throttle wordings is_throttled_403 already matches. 408/425 stay out: they are in RETRYABLE_4XX_STATUSES because an edge can raise them on a served request, but neither says a rate window is closed, so neither buys anything by waiting.
• throttle_delay_seconds(result, now=None) — Retry-After, else X-RateLimit-Remaining: 0 + a parseable X-RateLimit-Reset (rounded up, since time.time() is fractional and truncating would wake up before the window reopens), else 60s (GitHub's "wait for at least one minute"). Clamped to [1, 90]. Non-numeric and negative values fall to the next rule rather than being guessed at.
• confirm_landed(...) — the one seam all three failure paths now read the PR through, so the backoff cannot be added to two and forgotten on the third. It sleeps only under a throttle and is otherwise review_already_posted verbatim.
• The inline path's UNKNOWN-under-a-throttle now fails closed. No fallback POST; emit_delivery(False); findings to the job summary under POST_FAILED_SUMMARY_NOTE; SystemExit(1). Confirmed-ABSENT under a throttle waits once more and then posts the fallback exactly as before.

Round 2 — the nine panel findings (all fixed, threads resolved):

• One backoff window per throttled POST, not one per wait. throttle_backoff_deadline computes an absolute monotonic deadline once; wait_for_backoff sleeps only the remainder. The second wait used to recompute a fresh full delay from the first response's now-stale headers, so a Retry-After was sat out twice — and on the X-RateLimit-Reset rule the recomputed window had gone negative during the first sleep, fell through, and spent a flat 60s after the window it had just waited out demonstrably reopened. The remainder is now normally zero, which also collapses the ~150s staleness of the confirmed-ABSENT answer down to read latency.
• gh_post_review is bounded by GH_POST_REVIEW_TIMEOUT_SECONDS. Both POSTs precede write_step_summary, so a wedged one took the round out of both channels. A timeout becomes a status-less CompletedProcess — not a throttle, not a read-only token — so it routes into the undecided path and the PR gets asked.
• No API call inside a declared embargo. throttle_exceeds_budget compares the unclamped declared delay against the cap; longer than that and the run makes no further call at all — no wait, no read, no fallback — and goes straight to the job summary and red.
• A messageless 403 carrying a Retry-After is a throttle. is_read_only_token_error now excludes on is_throttle rather than is_throttled_403, so an edge/WAF/GHES refusal with a bare status stops degrading green over a write GitHub may have served.
• POST_UNCONFIRMED_SUMMARY_NOTE replaces the "the API rejected the request" claim on every path that cannot make it — including the fallback POST's own throttled-and-unreadable case, which needed post_or_degrade to report via a new optional outcome dict what its False could not say.
• Integer-only reset arithmetic (reset - math.floor(now)), so a ~309-digit X-RateLimit-Reset cannot raise OverflowError and kill the poster before the job summary; and remaining > 0 rather than >= 0, so a window that lapsed within the last second falls through to the default instead of becoming the 1s floor.

Docs: a delivery/fallback subsection in .github/cursor-review/README.md, scoped to distinguish the throttle path from every other undecided failure, and the post job's budget comment in cursor-review.yml restated for the bounded waits, POSTs and reads.

Judgment calls

1. posted=false on an unconfirmed write, not true. A throttle can be raised on a request GitHub served, so a true here would be a guess that greens the fresh-review gate over nothing. False is the honest and fail-closed direction: worst case a human re-triggers the label on a review that did land.
2. is_throttle's 403 arm conjoins the status (status == 403 and is_throttled_403(...)) rather than calling is_throttled_403 alone as the plan's formula reads. That function's own docstring states it is "only ever consulted once the status is already known to be 403" — its allowlist is a substring match over gh's error line, and a 422 validation message is free to quote "rate limit". Strictly narrower than the literal formula, identical on every path that actually reaches it, and it satisfies every case the plan enumerates.
3. The worst-case budget is two sleeps, and every term in it is now bounded. Round 1 counted three (the plan counted two); sharing one window between the inline read and the fallback removed one, and bounding both POSTs added the last unbounded terms. 2 × 90 (waits) + 2 × 60 (reads) + 2 × 60 (POSTs) = 420s, inside the post job's timeout-minutes: 10 alongside the artifact downloads and the token mint. The docstring and the workflow comment both state the real number.
4. Retry-After is capped at 90s even when GitHub asks for an hour. Reversed in round 2 — the reviewer was right. Clamping an hour-long embargo to 90s and then calling anyway is a request issued inside a window GitHub explicitly closed, which is what escalates a secondary limit for the whole App installation, not just this run. The cap is still 90s for windows the job can sit out; a longer declared window now skips the wait, the read and the fallback entirely and degrades to the job summary and a red check. Same end state as the old reasoning wanted, reached without the offending call — and in seconds rather than after 90s of sleep that bought nothing. The 60s default is unaffected: it is this repo's guess, not an embargo.
5. The header arm for a 403 is narrower than the finding asked for. The finding said to treat any 403 with a parseable Retry-After as a throttle. As written that would let a proxy-attached header override an affirmative refusal ("Resource not accessible by integration", an SSO/IP-allowlist block, an archived repo), trading its green degrade for a doomed wait, a doomed read and a permanently red check — the exact regression BE-12612 narrowed this family to avoid. The arm therefore fires only on a messageless 403 (gh's HTTP 403 (https://…) rendering), which is the scenario the finding actually describes. gh_error_is_messageless fails closed: anything it cannot fully strip reads as messageful and keeps today's classification.
6. Three of round 2's fixes rewrite round-1 tests rather than adding beside them. test_a_throttled_post_confirmed_absent_waits_again_then_falls_back asserted the double-wait finding 5 identifies as a bug; it is now ..._falls_back_without_a_second_wait. Two note assertions moved to POST_UNCONFIRMED_SUMMARY_NOTE. All three pinned behaviour this round deliberately inverts, and all three were added by this PR — no coverage from main or from fix(cursor-review): match the read-only-token 403 by message and let a throttled 403 reach the landed-review check #277 was weakened. test_a_standing_403_never_waits, also this PR's, is kept and is what bounds the new header arm.
7. One test from fix(cursor-review): match the read-only-token 403 by message and let a throttled 403 reach the landed-review check #277 is rewritten, not added to. test_a_throttled_403_with_an_unreadable_list_tags_nothing asserted the old "UNKNOWN posts the fallback" behaviour; that is the exact behaviour this change inverts, and fix(cursor-review): match the read-only-token 403 by message and let a throttled 403 reach the landed-review check #277's is_read_only_token_error docstring names it as a deferred residual to be closed here. It is now test_a_throttled_403_with_an_unreadable_list_declines_the_fallback, and a new sibling test pins that UNKNOWN from any other cause (5xx, dropped connection, 408/425) still posts the fallback untagged, so the narrowing is bounded.

Verification of the -i premise

The change rests on a claim about gh behaviour, so it was checked against the real binary rather than asserted — a non-mutating 404 probe (gh api -i /repos/Comfy-Org/github-workflows/pulls/99999999) on gh 2.92.0:

• exit code still 1;
• stderr still exactly gh: Not Found (HTTP 404) — byte-identical, so gh_http_status, gh_error_line, is_throttled_403 and is_read_only_token_error (all of which read stderr) are untouched;
• stdout carries HTTP/2.0 404 Not Found, CRLF-terminated headers in Go's canonical casing (X-Ratelimit-Reset, not the X-RateLimit-Reset GitHub documents), a blank line, then the JSON body.

That real capture is what GH_INCLUDE_STDOUT in the tests is transcribed from, and the parser was run against the captured bytes directly. The CRLF handling and the lower-casing both exist because of what that probe returned, not because of what the plan predicted.

Residual

• A real throttled POST /pulls/{n}/reviews was never exercised end to end. Inducing a live secondary rate limit means hammering GitHub's API, which is exactly the resource-exhausting action that is out of bounds — so the throttle responses are synthetic in the tests, while the gh -i output shape they are built on is a verbatim capture from the live API (above). What remains unproven by observation: that GitHub's throttled review-POST response actually carries Retry-After (the code falls through to the 60s default if it does not, which is a correct outcome either way) and that the 6.5-minute worst case fits the runner's real timing.
• The caller-fleet SHA bump is not done here. cursor-review.yml and .github/cursor-review/ are watched surfaces and this PR is behavioral, so consumers' uses: pins must move after merge via the bump-cursor-review-callers fleet. Deliberately no Skip-caller-bump: true trailer.
• This PR is stacked and its base is unmerged. If fix(cursor-review): match the read-only-token 403 by message and let a throttled 403 reach the landed-review check #277 changes in review — particularly is_throttled_403, gh_error_line or RETRYABLE_4XX_STATUSES, all of which this hangs off — this branch needs a rebase and is_throttle may need to follow. It cannot merge before fix(cursor-review): match the read-only-token 403 by message and let a throttled 403 reach the landed-review check #277.
• Unexercised artifacts. The spike this plan came from, its findings comment, and the worker transcript it cites are all on internal surfaces this run holds no access to; nothing in this change was verified against them, only against the plan text reproduced into the task. The main commit the plan names as its baseline (e29cf0c) was read and matches.
• The remaining 403-family gap is unchanged: nothing here detects a primary rate limit before the POST is attempted — the backoff is still reactive only. (The other gap round 1 named — a 403 that carries no recognizable message — is closed by round 2's header arm, but only when such a response carries a Retry-After; one carrying neither a message nor a usable header is still indistinguishable from a standing refusal and still degrades green.)
• The over-budget-embargo path is synthetic too. Round 2 makes an hour-long Retry-After skip every remaining API call, but a real primary rate limit on POST /pulls/{n}/reviews was no more reachable in this run than a secondary one was; the header values it keys on are constructed in the tests.
• gh_post_review's timeout was not observed firing against a real wedged connection — only against a TimeoutExpired raised by a stubbed subprocess.run. What is verified is the shape it produces and how every classifier reads it.

Provenance

• Authored by: agent-work loop
• Verified: python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py': 531 passed, 0 failed (23 new this round, 3 round-1 tests rewritten); python3 .github/workflow-pins/check_workflow_pins.py: OK, 11 workflows, 0 exempt; python3 .github/agents-md-integrity/check_agents_md.py --root .: passed, 2 pre-existing warnings (AGENTS.md 155 lines, no CODEOWNERS); actionlint .github/workflows/cursor-review.yml: one finding, job.workflow_sha, pre-existing and identical on the base branch; python3 -m py_compile on the changed script. Round 1's gh api -i live probe is unchanged and still the basis for the response-shape fixtures; no new live API call was made this round.
• Deviations: all nine panel findings fixed. Two are implemented differently from how they were phrased, both narrowings, both argued under "Judgment calls" (4) and (5): the over-budget embargo skips every remaining API call rather than merely degrading, and the 403 header arm requires a messageless body so it cannot override an affirmative refusal. The confirmed-ABSENT staleness finding is closed by removing the stale gap (one shared backoff window) rather than by adding the re-confirm read it proposed — reasoning on that thread. One fix was extended past the finding: the undecided-write summary note is applied to all three such paths, not only the one flagged.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: 86e2207a-00d1-4a35-95c5-c730f8b0629d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 9 finding(s).

Severity	Count
🟡 Medium	4
🟢 Low	5

Panel: 6/6 reviewers contributed findings.