[PR #280] [MERGED] fix(cursor-review): wait out a throttle before the landed-review read, and decline the fallback when the answer is still unknown #284

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-12691-throttle-backoff

📝 Commits (7)

f770127 fix(cursor-review): match the read-only-token 403 by message, not by status (BE-12612)
486af72 fix(cursor-review): key the read-only 403 guard on status + a throttle allowlist (BE-12612)
87c274f fix(cursor-review): read every gh status rendering, and ask the PR on all three failure paths (BE-12612)
5180568 docs(cursor-review): refresh two test docstrings the round-2 refactor left stale (BE-12612)
07235a1 fix(cursor-review): back off per Retry-After before the landed-review read (BE-12691)
befd160 fix(cursor-review): one backoff window per throttled POST, and bound every call on the recovery path (BE-12691)
e60a123 Merge remote-tracking branch 'origin/main' into matt/be-12691-throttle-backoff

📊 Changes

5 files changed (+1784 additions, -47 deletions)

View changed files

📝 .github/cursor-review/README.md (+8 -0)
📝 .github/cursor-review/post-review.py (+517 -30)
📝 .github/cursor-review/tests/test_post_review.py (+1052 -9)
📝 .github/cursor-review/tests/test_post_review_delivery.py (+180 -3)
📝 .github/workflows/cursor-review.yml (+27 -5)

📄 Description

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

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.
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.
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.
~~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.
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.
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 #277 was weakened. test_a_standing_403_never_waits, also this PR's, is kept and is what bounds the new header arm.
One test from #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 #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.
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': 593 passed, 0 failed (post-merge count; includes main's unrelated repeat_of membership suite alongside this PR's 23 new/rewritten cases); 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 files. 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. Post-open: #277 (this branch's former stacking base) merged to main, so GitHub retargeted this PR's base to main directly — it is no longer stacked. main also gained an unrelated repeat_of membership feature (#278/#279) in the same file, producing a real merge conflict: one duplicate insertion of THROTTLE_403_MESSAGES/is_throttled_403 (dropped, since this branch already carried that content) and several genuine overlaps where this PR's confirm_landed/backoff logic and main's older review_already_posted call sites touched the same lines (resolved in this branch's favor — the backoff behavior is a strict superset). No coverage or behavior from either side was weakened; the merged suite (593 tests) passes in full.

_{🔄 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/280 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/9/2026 **Status:** ✅ Merged **Merged:** 9/12/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-12691-throttle-backoff` --- ### 📝 Commits (7) - [`f770127`](https://github.com/Comfy-Org/github-workflows/commit/f7701272bb13c1015dbadc094ed3fb7dc6ee221e) fix(cursor-review): match the read-only-token 403 by message, not by status (BE-12612) - [`486af72`](https://github.com/Comfy-Org/github-workflows/commit/486af720064596721aedf72b239ddf0ad996f08d) fix(cursor-review): key the read-only 403 guard on status + a throttle allowlist (BE-12612) - [`87c274f`](https://github.com/Comfy-Org/github-workflows/commit/87c274fcb21fcd2cec7adb1c94bb68cc7cbfb033) fix(cursor-review): read every gh status rendering, and ask the PR on all three failure paths (BE-12612) - [`5180568`](https://github.com/Comfy-Org/github-workflows/commit/5180568ba417065c1c982f7cf5d0481dd33e150d) docs(cursor-review): refresh two test docstrings the round-2 refactor left stale (BE-12612) - [`07235a1`](https://github.com/Comfy-Org/github-workflows/commit/07235a1392b7de47650f4f955f47bacc44a28d4c) fix(cursor-review): back off per Retry-After before the landed-review read (BE-12691) - [`befd160`](https://github.com/Comfy-Org/github-workflows/commit/befd16006cbac13b0113635daedca5dd17414e16) fix(cursor-review): one backoff window per throttled POST, and bound every call on the recovery path (BE-12691) - [`e60a123`](https://github.com/Comfy-Org/github-workflows/commit/e60a123f27ef0fb2428fe061d5900ea4875a2475) Merge remote-tracking branch 'origin/main' into matt/be-12691-throttle-backoff ### 📊 Changes **5 files changed** (+1784 additions, -47 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+8 -0) 📝 `.github/cursor-review/post-review.py` (+517 -30) 📝 `.github/cursor-review/tests/test_post_review.py` (+1052 -9) 📝 `.github/cursor-review/tests/test_post_review_delivery.py` (+180 -3) 📝 `.github/workflows/cursor-review.yml` (+27 -5) </details> ### 📄 Description ## 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 #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 #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 #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. - **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'`: 593 passed, 0 failed (post-merge count; includes main's unrelated `repeat_of` membership suite alongside this PR's 23 new/rewritten cases); `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 files. 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. **Post-open:** #277 (this branch's former stacking base) merged to `main`, so GitHub retargeted this PR's base to `main` directly — it is no longer stacked. `main` also gained an unrelated `repeat_of` membership feature (#278/#279) in the same file, producing a real merge conflict: one duplicate insertion of `THROTTLE_403_MESSAGES`/`is_throttled_403` (dropped, since this branch already carried that content) and several genuine overlaps where this PR's `confirm_landed`/backoff logic and `main`'s older `review_already_posted` call sites touched the same lines (resolved in this branch's favor — the backoff behavior is a strict superset). No coverage or behavior from either side was weakened; the merged suite (593 tests) passes in full. --- <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:04 +00:00