From f7701272bb13c1015dbadc094ed3fb7dc6ee221e Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 17:09:05 -0700 Subject: [PATCH 1/6] fix(cursor-review): match the read-only-token 403 by message, not by status (BE-12612) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `is_read_only_token_error` matched the bare `HTTP 403` substring, so every 403 GitHub issues for a reason other than a read-only token — a primary or secondary rate limit, an abuse-detection refusal, an org-policy or SSO block — was reported as a read-only token: the review went to the job summary, the step exited 0, and `main()` returned before the landed-review check (BE-12528) could ask the PR whether the write had actually gone through. Two halves, because narrowing the guard alone is not enough. The guard now matches the permission message (`resource not accessible by`, case-insensitively, as a prefix so both installation tokens and a fine-grained PAT are covered), and 403 joins RETRYABLE_4XX_STATUSES — without that, a 403 past the guard falls into `pre_write_rejection`, which treats a 4xx outside that set as absent by construction and skips the read just the same. The permission case is unchanged at both call sites: same log line, `emit_delivery(False)`, the summary under READ_ONLY_SUMMARY_NOTE, exit 0. Also stubs `gh_list_reviews` in test_post_review_delivery.py's driver: it drove `main()` end to end without one, so a 403 that no longer short-circuits on its status shelled out to a real `gh` from the unit suite. --- .github/cursor-review/post-review.py | 52 ++++-- .../cursor-review/tests/test_post_review.py | 150 ++++++++++++++++++ .../tests/test_post_review_delivery.py | 33 +++- 3 files changed, 214 insertions(+), 21 deletions(-) diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 63853e4..8078b66 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -283,6 +283,15 @@ def gh_post_review(repo: str, pr_number: str, payload: str) -> subprocess.Comple ) +# The PERMISSION rejection's message, matched case-insensitively as a PREFIX of the +# full phrase. Deliberately stops before the principal: both token arms in +# cursor-review.yml (the `create-github-app-token` output and `secrets.GITHUB_TOKEN`) +# are installation tokens and say `Resource not accessible by integration`, while a +# fine-grained PAT says `... by personal access token` — the same refusal, a different +# noun. Matching the shared prefix covers all of them without enumerating principals. +READ_ONLY_TOKEN_MESSAGE = "resource not accessible by" + + def is_read_only_token_error(result: subprocess.CompletedProcess) -> bool: """True when the POST failed because the token can't write to the PR. @@ -292,9 +301,18 @@ def is_read_only_token_error(result: subprocess.CompletedProcess) -> bool: HTTP 403 'Resource not accessible by integration'. That's an environment constraint, not a review failure, so callers degrade to the job summary rather than failing the check red. + + Identified by its MESSAGE, never by its status. GitHub answers throttling — + primary and secondary rate limits, abuse detection — with 403 too, and so does an + org-policy refusal; none of those is a read-only token, and none of them proves + the write was rejected before it was committed. A status match swallowed all of + them here and returned from `main()` before the landed-review check below could + ask the PR what actually happened, reporting a throttle as a read-only token and + skipping the read. So the match is on the permission wording alone, and every + other 403 falls through to that check (see RETRYABLE_4XX_STATUSES). """ - blob = result.stderr or "" - return "Resource not accessible by integration" in blob or "HTTP 403" in blob + blob = (result.stderr or "").lower() + return READ_ONLY_TOKEN_MESSAGE in blob # The discriminator for "a review of THIS panel is already on the PR". Mirrors @@ -327,13 +345,14 @@ def gh_http_status(result: subprocess.CompletedProcess): # `lost_to_fallback` on an assumption that does not apply, so they take the read like # a 5xx does. # -# 403 is NOT in this set, and not because a throttled 403 is impossible — GitHub does -# signal throttling that way. It is because `is_read_only_token_error` matches any -# stderr carrying "HTTP 403" and returns from `main()` before this decision is -# reached, so listing 403 here would be dead code that reads as coverage. Narrowing -# that guard to its specific message is a change to the read-only degradation path, -# not to this one; tracked separately rather than made in passing. -RETRYABLE_4XX_STATUSES = frozenset({408, 425, 429}) +# 403 is here because `is_read_only_token_error` is now message-specific: any 403 that +# reaches THIS decision has already failed that match, so it is not the permission +# case. What is left is a primary or secondary rate limit, an abuse-detection refusal +# or an org-policy block — GitHub answers all of them 403 — and none of those proves +# the write was rejected before it was committed. A throttle in particular can be +# raised on a request the API went on to serve, exactly like the 429 beside it, so it +# takes the read too. +RETRYABLE_4XX_STATUSES = frozenset({403, 408, 425, 429}) # This read sits on the RECOVERY path: the fallback POST and write_step_summary both @@ -1915,11 +1934,11 @@ def finish_posted_review(): # Cheapest sufficient evidence first. A 4xx is GitHub VALIDATING and rejecting the # request before writing anything (every firing observed in the field is a 422 over # an inline position), so the review is absent by construction and no read is worth - # the call — with the exception carved out by RETRYABLE_4XX_STATUSES, which are 4xx - # only in the sense that an edge or a proxy said so and may well have said it about - # a request GitHub went on to serve. Anything else — a 5xx, or a transport error - # that carries no status at all — leaves the write genuinely undecided, so ask the - # PR. Three outcomes follow: + # the call — with the exception carved out by RETRYABLE_4XX_STATUSES, whose members + # are 4xx without carrying that meaning: an edge or a proxy said so, or GitHub + # throttled a request it may well have gone on to serve. Anything else — a 5xx, or + # a transport error that carries no status at all — leaves the write genuinely + # undecided, so ask the PR. Three outcomes follow: # PRESENT (the review landed: report it delivered, post nothing more), ABSENT # (behave exactly as this path always has), and UNKNOWN (post the fallback, but tag # nothing `lost_to_fallback` — the flag is a claim, and an unreadable list supports @@ -2098,8 +2117,9 @@ def finish_posted_review(): gated=0, ungated=len(enriched), ): - # Both attempts failed for a non-403 reason (an API outage, a stale commit_id - # after a force-push, a body-level rejection dropping the anchors cannot fix). + # Both attempts failed for a non-permission reason (an API outage, a throttle, + # a stale commit_id after a force-push, a body-level rejection dropping the + # anchors cannot fix). # Without this the whole review is gone from the PR *and* the summary, which # contradicts the no-inline branch above — and this is the branch carrying # MORE content, since it has an inline half. post_or_degrade only writes a diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index d048236..be47069 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -31,6 +31,7 @@ import contextlib import importlib.util +import inspect import io import json import os @@ -1288,11 +1289,160 @@ def status(text): self.assertEqual(status("gh: Unprocessable Entity (HTTP 422)"), 422) self.assertEqual(status("gh: Bad Gateway (HTTP 502)"), 502) + self.assertEqual( + status("gh: You have exceeded a secondary rate limit (HTTP 403)"), 403, + "a throttled 403 carries a status like any other — it is not the " + "permission case and must reach the landed-review check", + ) self.assertIsNone(status("error connecting to api.github.com")) self.assertIsNone(status("")) self.assertIsNone(status(None), "a CompletedProcess can carry no stderr at all") +class ReadOnlyGuardIsMessageSpecificTest(unittest.TestCase): + """A 403 is not by itself a read-only token, and must not short-circuit the read. + + `is_read_only_token_error` used to match the bare `HTTP 403` substring, and + `main()` returns from that branch BEFORE the landed-review check — so every other + 403 GitHub issues (a primary or secondary rate limit, abuse detection, an + org-policy or SSO refusal) was reported as a read-only token, written to the job + summary and exited 0, with the PR never asked whether the review had actually + landed. The guard now matches the PERMISSION MESSAGE only, and 403 joins + RETRYABLE_4XX_STATUSES so what falls through takes the read instead of being + assumed absent. Both halves are needed: narrowing the guard alone would send a + throttled 403 into `pre_write_rejection`, which treats a 4xx outside that set as + absent by construction and skips the read just the same. + """ + + ANCHORED = [finding("app.py", 11), finding("app.py", 12)] + + # The real messages, verbatim. `gh api` renders GitHub's error as + # `gh: (HTTP 403)`, so the status is identical across all of them and + # the message is the ONLY discriminator this path has. + PERMISSION = "gh: Resource not accessible by integration (HTTP 403)" + THROTTLED = ( + "gh: You have exceeded a secondary rate limit. Please wait a few minutes " + "before you try again. (HTTP 403)" + ) + + def landed_review(self, **overrides): + return FirstReviewConfirmationTest().landed_review(**overrides) + + def test_the_permission_403_still_degrades_to_the_summary(self): + """The case the guard is FOR, unchanged: no read, no fallback, green exit. + + A read-only token rejects the fallback exactly as it rejected the first POST, + and no read is worth the call because nothing was written — so this branch + still returns before either. The lower-case variant is here because the match + is case-insensitive by design: `gh` echoes GitHub's message and nothing + guarantees its capitalisation. + """ + for label, stderr in ( + ("as GitHub sends it", self.PERMISSION), + ("lower-cased", self.PERMISSION.lower()), + ): + with self.subTest(message=label): + outputs, summaries, notes, calls = {}, [], [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + self.ANCHORED, + post_returncode=1, + stderr=stderr, + list_calls=calls, + outputs=outputs, + summaries=summaries, + notes=notes, + ) + self.assertEqual(calls, [], "a read-only token wrote nothing to read") + self.assertEqual(len(posted), 1, "no fallback — it would fail the same way") + self.assertEqual(outputs["delivered"], "false") + self.assertEqual(outputs["posted"], "false") + self.assertEqual(len(summaries), 1, "the review went to the job summary") + # main() calls write_step_summary with no `note=`, so the stub records + # None; the banner it takes is the parameter's default. + self.assertEqual(notes, [None]) + self.assertIs( + inspect.signature(PR.write_step_summary).parameters["note"].default, + PR.READ_ONLY_SUMMARY_NOTE, + "and that default is the read-only banner", + ) + self.assertIsNone(driver.exit_code, "an environment constraint is not red") + + def test_a_throttled_403_with_the_review_present_skips_the_fallback(self): + """A secondary rate limit can be raised on a request GitHub went on to serve. + + Under the old guard this exited 0 having written the review to the job summary + and claimed a read-only token, while the review sat on the PR the whole time. + """ + outputs, summaries, notes, calls = {}, [], [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + self.ANCHORED, + post_returncode=1, + stderr=self.THROTTLED, + existing_reviews=[self.landed_review()], + list_calls=calls, + outputs=outputs, + summaries=summaries, + notes=notes, + ) + self.assertEqual(calls, [("o/r", "1")], "this 403 is read, not assumed") + self.assertEqual(len(posted), 1, "the review landed — no duplicate is posted") + self.assertEqual(outputs["delivered"], "true") + self.assertNotIn( + PR.READ_ONLY_SUMMARY_NOTE, notes, + "nothing here is a read-only token", + ) + self.assertEqual(summaries, [], "the review is on the PR, not the summary") + self.assertIsNone(driver.exit_code) + + def test_a_throttled_403_with_the_review_absent_fails_red(self): + """Throttled on both POSTs, and the read confirms nothing landed. + + Nothing reached the PR and the cause is not an environment constraint, so this + is the POST-failed degradation — summary under POST_FAILED_SUMMARY_NOTE and a + red step — not the green read-only one the old guard produced. + """ + outputs, summaries, notes, calls = {}, [], [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + self.ANCHORED, + post_returncode=1, + stderr=self.THROTTLED, + existing_reviews=[], + list_calls=calls, + outputs=outputs, + summaries=summaries, + notes=notes, + ) + self.assertEqual(calls, [("o/r", "1")]) + self.assertEqual(len(posted), 2, "inline attempt, then the body-only fallback") + self.assertEqual(outputs["delivered"], "false") + self.assertIn(PR.POST_FAILED_SUMMARY_NOTE, notes) + self.assertNotIn(None, notes, "the read-only banner never applies here") + self.assertEqual(driver.exit_code, 1, "the step goes red") + + def test_a_throttled_403_with_an_unreadable_list_tags_nothing(self): + """UNKNOWN survives the new status too: post the fallback, claim nothing. + + `lost_to_fallback` asserts the first review is absent, and a read that failed + supports no such claim (BE-4785). + """ + calls = [] + posted = EndToEndPostTest().run_main( + self.ANCHORED, + post_returncode=1, + stderr=self.THROTTLED, + list_returncode=1, + list_calls=calls, + ) + self.assertEqual(calls, [("o/r", "1")]) + self.assertEqual(len(posted), 2, "undecided means post the fallback") + ledger = ledger_from_posted_body(posted[1]["body"]) + for entry in ledger["entries"]: + self.assertNotIn("lost_to_fallback", entry) + + class FitSentinelItemsTest(unittest.TestCase): """The budget search behind the prose floor.""" diff --git a/.github/cursor-review/tests/test_post_review_delivery.py b/.github/cursor-review/tests/test_post_review_delivery.py index edce004..4a4f640 100644 --- a/.github/cursor-review/tests/test_post_review_delivery.py +++ b/.github/cursor-review/tests/test_post_review_delivery.py @@ -80,11 +80,20 @@ def run_main( error_message=None, with_diff=True, fallback_ok=False, + existing_reviews=(), ): """Return (posted_payloads, delivery_dict). delivery is {} when nothing was written. `fallback_ok` models the real 422: the inline payload is what GitHub rejects, so the anchor-free retry that follows it succeeds. + + The landed-review read (BE-12528) is ALWAYS stubbed, exactly as + test_post_review.py's driver stubs it and for the same reason: this drives + main() end to end, so an unstubbed `gh_list_reviews` shells out to a REAL `gh` + from the unit suite the moment a failure path stops short-circuiting on its + status — which is what a 403 now does (BE-12612). `existing_reviews` is the + flat list the PR carries, wrapped in the one `--slurp` page the real command + returns; the default empty page is "confirmed absent". """ posted = [] @@ -97,6 +106,14 @@ def fake_post(repo, pr_number, payload): args=["gh"], returncode=rc, stdout="", stderr=err ) + def fake_list(repo, pr_number): + return subprocess.CompletedProcess( + args=["gh"], + returncode=0, + stdout=json.dumps([list(existing_reviews)]), + stderr="", + ) + if panel is None: panel = [{"model": "m", "review_type": "adversarial", "status": "ok"}] @@ -121,6 +138,7 @@ def fake_post(repo, pr_number, payload): argv += ["--error-message", error_message] with mock.patch.object(PR, "gh_post_review", side_effect=fake_post), \ + mock.patch.object(PR, "gh_list_reviews", side_effect=fake_list), \ mock.patch.object(PR.sys, "argv", argv), \ mock.patch.object(PR, "write_step_summary", lambda *a, **k: None), \ mock.patch.dict(os.environ, {"GITHUB_OUTPUT": outpath}, clear=False): @@ -336,16 +354,21 @@ def test_a_read_only_token_exits_zero_and_reports_not_posted(self): self.assertIsNone(self.exit_code, "the read-only degradation still exits 0") self.assertEqual(delivery["posted"], "false") - def test_a_bare_403_is_read_as_read_only_too(self): - # is_read_only_token_error matches any HTTP 403, not just the integration - # phrasing — pin that the weaker match reaches the same verdict. + def test_a_bare_403_is_no_longer_read_as_read_only(self): + # BE-12612 inverted this case. is_read_only_token_error used to match any + # stderr carrying "HTTP 403", so a throttle, an abuse-detection refusal or an + # org-policy block exited 0 claiming a read-only token — and returned before + # the landed-review check could ask whether the write had gone through. It now + # matches the PERMISSION MESSAGE only, so a 403 without that message takes the + # read; confirmed absent here, the fallback is thrown and fails the same way, + # which is a genuine POST failure and goes red. `posted` still never lies. _, delivery = self.run_main( [finding("app.py", 11)], post_returncode=1, stderr="gh: HTTP 403: Forbidden", ) - self.assertIsNone(self.exit_code) - self.assertEqual(delivery["posted"], "false") + self.assertEqual(self.exit_code, 1) + self.assertNotEqual(delivery.get("posted"), "true") def test_a_genuine_post_failure_exits_one_and_never_claims_posted(self): # Both attempts fail for a non-403 reason. The job goes red, so the DM's From 486af720064596721aedf72b239ddf0ad996f08d Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 18:38:37 -0700 Subject: [PATCH 2/6] fix(cursor-review): key the read-only 403 guard on status + a throttle allowlist (BE-12612) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on the first cut of this change, which matched the PERMISSION message and routed everything else 403 into the landed-review read. Three problems, all raised by the panel: - The message test was an unanchored substring over the whole stderr blob with no status conjunction, so a 422 whose `errors[].message` entries `gh` joins into that blob could be read as a read-only token — the same silent skip this change exists to remove, reached from the other side. The guard now requires `gh_http_status(result) == 403`. - "Everything that is not the permission phrase is retryable" is not true of a 403. An SSO/IP-allowlist or org-policy block, an archived repo and any future rewording of the permission message are all STANDING refusals that wrote nothing and that no retry fixes; the first cut gave each of them a doomed read, a doomed fallback POST and SystemExit(1) on every run, turning a green degrade into a permanently red check in the orgs least able to change it. Inverted to an allowlist: only the throttle wordings (rate limit, abuse detection, submitted too quickly) fall through, which is the case the ticket is actually about. - The docstring claimed every other 403 reaches the landed-review check. That holds for the inline path in `main()` only; the no-inline-comments branch and `post_or_degrade` have no such read. Scoped the claim to the paths that have one, and noted that neither of the others posts a fallback, so neither can duplicate a committed write. Also from the panel: - test_post_review_delivery.py's bare-403 case used `gh: HTTP 403: Forbidden`, which `_GH_HTTP_STATUS_RE` does not parse, so it reached the read through the "no status at all" branch and would have passed with either half of the fix reverted. Rewritten around `gh`'s real shape, plus a companion pinning that an unworded 403 still degrades green. - cursor-review.yml's notify-complete comment and its user-facing DETAIL string still described the bare-403 rule. Updated to the new routing; this was the PR's own recorded residual. New coverage: a policy 403 still degrades (the regression guard on the allowlist), the guard needs status and message both, and every throttle wording is pinned case-insensitively. --- .github/cursor-review/post-review.py | 87 ++++++++---- .../cursor-review/tests/test_post_review.py | 133 ++++++++++++++++-- .../tests/test_post_review_delivery.py | 41 ++++-- .github/workflows/cursor-review.yml | 13 +- 4 files changed, 220 insertions(+), 54 deletions(-) diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 8078b66..981891b 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -283,17 +283,32 @@ def gh_post_review(repo: str, pr_number: str, payload: str) -> subprocess.Comple ) -# The PERMISSION rejection's message, matched case-insensitively as a PREFIX of the -# full phrase. Deliberately stops before the principal: both token arms in -# cursor-review.yml (the `create-github-app-token` output and `secrets.GITHUB_TOKEN`) -# are installation tokens and say `Resource not accessible by integration`, while a -# fine-grained PAT says `... by personal access token` — the same refusal, a different -# noun. Matching the shared prefix covers all of them without enumerating principals. -READ_ONLY_TOKEN_MESSAGE = "resource not accessible by" +# The 403 wordings that mean "slow down", not "you may not write". GitHub answers a +# primary rate limit, a secondary rate limit and abuse detection with 403 as readily +# as with 429 — and, unlike every other 403, one of those can be raised on a request +# the API went on to SERVE, so it is not evidence the write was rejected before it was +# committed. Matched case-insensitively as substrings, which is how `gh` hands over +# GitHub's `message`: echoed into stderr rather than parsed out of the JSON body. +# Only ever consulted once the status is already known to be 403, so a finding body +# quoting one of these phrases cannot reach it through a 422. +THROTTLE_403_MESSAGES = ( + # "API rate limit exceeded for ..." / "You have exceeded a secondary rate limit." + "rate limit", + # "You have triggered an abuse detection mechanism." + "abuse detection", + # the older wording of the same secondary-limit refusal + "submitted too quickly", +) + + +def is_throttled_403(result: subprocess.CompletedProcess) -> bool: + """True when a 403's message is GitHub asking us to slow down.""" + blob = (result.stderr or "").lower() + return any(message in blob for message in THROTTLE_403_MESSAGES) def is_read_only_token_error(result: subprocess.CompletedProcess) -> bool: - """True when the POST failed because the token can't write to the PR. + """True when the POST failed because the ENVIRONMENT forbids writing to the PR. The gate skips fork PRs (which always hit this), but a read-only token can still occur on same-repo runs — org/repo default workflow permissions set @@ -302,17 +317,34 @@ def is_read_only_token_error(result: subprocess.CompletedProcess) -> bool: constraint, not a review failure, so callers degrade to the job summary rather than failing the check red. - Identified by its MESSAGE, never by its status. GitHub answers throttling — - primary and secondary rate limits, abuse detection — with 403 too, and so does an - org-policy refusal; none of those is a read-only token, and none of them proves - the write was rejected before it was committed. A status match swallowed all of - them here and returned from `main()` before the landed-review check below could - ask the PR what actually happened, reporting a throttle as a read-only token and - skipping the read. So the match is on the permission wording alone, and every - other 403 falls through to that check (see RETRYABLE_4XX_STATUSES). + STATUS AND MESSAGE, not either alone (BE-12612). + + The status must be 403. The permission wording also travels inside a 422's + `errors[].message` list, which `gh` joins into the same stderr blob, and reading + that as a read-only token would return from `main()` before the landed-review + check — the same silent skip this guard is being narrowed to remove, arrived at + from the other direction. + + The message must then NOT be a throttle. A rate limit and an abuse-detection + refusal are neither an environment constraint nor proof that nothing was written, + so those alone fall through to the landed-review check (see + RETRYABLE_4XX_STATUSES). Everything else a 403 can carry — the permission refusal + above, whatever principal it names (`by integration` for both of + cursor-review.yml's token arms, `by personal access token` for a fine-grained + PAT), an SSO/IP-allowlist + or org-policy block, an archived repo, a future rewording of any of them — is a + STANDING refusal that no retry fixes and that wrote nothing, so it degrades to the + job summary. Matching the throttles rather than the permission phrase is what + keeps a SAML-blocked or IP-allowlisted org on that green degrade instead of the + permanently red check "everything but the permission phrase" would hand it. + + The fall-through reaches the landed-review check on the INLINE path in `main()` + only. `main()`'s no-inline-comments branch and `post_or_degrade` have no such + read: there a throttled 403 is reported as the POST failure it is — red, review in + the job summary — rather than mislabelled a read-only token. Neither of those + posts a fallback, so neither can duplicate a write GitHub committed before erroring. """ - blob = (result.stderr or "").lower() - return READ_ONLY_TOKEN_MESSAGE in blob + return gh_http_status(result) == 403 and not is_throttled_403(result) # The discriminator for "a review of THIS panel is already on the PR". Mirrors @@ -345,13 +377,12 @@ def gh_http_status(result: subprocess.CompletedProcess): # `lost_to_fallback` on an assumption that does not apply, so they take the read like # a 5xx does. # -# 403 is here because `is_read_only_token_error` is now message-specific: any 403 that -# reaches THIS decision has already failed that match, so it is not the permission -# case. What is left is a primary or secondary rate limit, an abuse-detection refusal -# or an org-policy block — GitHub answers all of them 403 — and none of those proves -# the write was rejected before it was committed. A throttle in particular can be -# raised on a request the API went on to serve, exactly like the 429 beside it, so it -# takes the read too. +# 403 is here because `is_read_only_token_error` now excludes the throttle wordings: +# a 403 that reaches THIS decision has already been classified as one of them, so it +# is a rate limit or an abuse-detection refusal and nothing else — every standing +# 403 (permission, SSO/IP allowlist, archived repo) returned from `main()` on the +# degrade path well above. A throttle can be raised on a request the API went on to +# serve, exactly like the 429 beside it, so it takes the read too. RETRYABLE_4XX_STATUSES = frozenset({403, 408, 425, 429}) @@ -2117,9 +2148,9 @@ def finish_posted_review(): gated=0, ungated=len(enriched), ): - # Both attempts failed for a non-permission reason (an API outage, a throttle, - # a stale commit_id after a force-push, a body-level rejection dropping the - # anchors cannot fix). + # Both attempts failed for a reason the read-only degradation does not cover + # (an API outage, a throttle, a stale commit_id after a force-push, a + # body-level rejection dropping the anchors cannot fix). # Without this the whole review is gone from the PR *and* the summary, which # contradicts the no-inline branch above — and this is the branch carrying # MORE content, since it has an inline half. post_or_degrade only writes a diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index be47069..3e12eda 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -1299,27 +1299,39 @@ def status(text): self.assertIsNone(status(None), "a CompletedProcess can carry no stderr at all") -class ReadOnlyGuardIsMessageSpecificTest(unittest.TestCase): - """A 403 is not by itself a read-only token, and must not short-circuit the read. +class ReadOnlyGuardExcludesThrottlesTest(unittest.TestCase): + """A THROTTLED 403 is not a read-only token, and must not short-circuit the read. `is_read_only_token_error` used to match the bare `HTTP 403` substring, and - `main()` returns from that branch BEFORE the landed-review check — so every other - 403 GitHub issues (a primary or secondary rate limit, abuse detection, an - org-policy or SSO refusal) was reported as a read-only token, written to the job - summary and exited 0, with the PR never asked whether the review had actually - landed. The guard now matches the PERMISSION MESSAGE only, and 403 joins - RETRYABLE_4XX_STATUSES so what falls through takes the read instead of being + `main()` returns from that branch BEFORE the landed-review check — so a primary or + secondary rate limit and an abuse-detection refusal were reported as a read-only + token, written to the job summary and exited 0, with the PR never asked whether + the review had actually landed. The guard now excludes those wordings, and 403 + joins RETRYABLE_4XX_STATUSES so what falls through takes the read instead of being assumed absent. Both halves are needed: narrowing the guard alone would send a throttled 403 into `pre_write_rejection`, which treats a 4xx outside that set as absent by construction and skips the read just the same. + + The narrowing is an ALLOWLIST of throttles, not a denylist of the permission + phrase, and `test_a_policy_403_still_degrades_to_the_summary` is why: every OTHER + 403 — SSO, IP allowlist, archived repo, a reworded permission message — is a + standing refusal that no retry fixes and that wrote nothing. Routing those into + the read plus a doomed fallback would replace a green degrade with a permanently + red check in exactly the orgs least able to change it. """ ANCHORED = [finding("app.py", 11), finding("app.py", 12)] # The real messages, verbatim. `gh api` renders GitHub's error as # `gh: (HTTP 403)`, so the status is identical across all of them and - # the message is the ONLY discriminator this path has. + # the message is the only thing separating a throttle from a standing refusal. PERMISSION = "gh: Resource not accessible by integration (HTTP 403)" + # An org-policy refusal: the wording shares nothing with the permission phrase, + # which is precisely why the guard cannot be written as "not the permission phrase". + POLICY = ( + "gh: Although you appear to have the correct authorization credentials, the " + "`acme` organization has enabled OAuth App access restrictions (HTTP 403)" + ) THROTTLED = ( "gh: You have exceeded a secondary rate limit. Please wait a few minutes " "before you try again. (HTTP 403)" @@ -1333,13 +1345,17 @@ def test_the_permission_403_still_degrades_to_the_summary(self): A read-only token rejects the fallback exactly as it rejected the first POST, and no read is worth the call because nothing was written — so this branch - still returns before either. The lower-case variant is here because the match - is case-insensitive by design: `gh` echoes GitHub's message and nothing - guarantees its capitalisation. + still returns before either. The lower-case variant is here because the + MESSAGE match is case-insensitive by design: `gh` echoes GitHub's message and + nothing guarantees its capitalisation. Only the message is lower-cased — + `(HTTP 403)` is `gh`'s own rendering, not GitHub's text, and it is fixed. """ for label, stderr in ( ("as GitHub sends it", self.PERMISSION), - ("lower-cased", self.PERMISSION.lower()), + ("message lower-cased", self.PERMISSION.replace( + "Resource not accessible by integration", + "resource not accessible by integration", + )), ): with self.subTest(message=label): outputs, summaries, notes, calls = {}, [], [], [] @@ -1368,6 +1384,97 @@ def test_the_permission_403_still_degrades_to_the_summary(self): ) self.assertIsNone(driver.exit_code, "an environment constraint is not red") + def test_a_policy_403_still_degrades_to_the_summary(self): + """The regression guard on the allowlist: a standing 403 stays green. + + An SSO/OAuth-restriction block shares no wording with the permission refusal, + so a guard written as "the permission phrase, and nothing else" would send it + into the landed-review read (which the same block fails, yielding UNKNOWN) and + then a doomed fallback POST, ending in SystemExit(1) on EVERY run — a + permanently red check where the caller used to get its review in the job + summary and a green step. Nothing about a policy refusal is transient, and + nothing was written, so it degrades exactly as the permission case does. + """ + outputs, summaries, notes, calls = {}, [], [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + self.ANCHORED, + post_returncode=1, + stderr=self.POLICY, + list_calls=calls, + outputs=outputs, + summaries=summaries, + notes=notes, + ) + self.assertEqual(calls, [], "a refusal that wrote nothing has nothing to read") + self.assertEqual(len(posted), 1, "no fallback — it would fail the same way") + self.assertEqual(outputs["posted"], "false") + self.assertEqual(len(summaries), 1, "the review went to the job summary") + self.assertEqual(notes, [None], "under the read-only banner default") + self.assertIsNone(driver.exit_code, "an environment constraint is not red") + + def test_the_guard_needs_the_status_as_well_as_the_message(self): + """Neither half alone: a 422 can carry the permission phrase, a 403 can not. + + `gh` joins a 422's `errors[].message` entries into the same stderr blob, so + the permission wording can arrive under a status that PROVES the write was + validated and refused. Classifying that as a read-only token would return from + `main()` before the landed-review check — the same silent skip BE-12612 is + removing, reached from the other direction. And a transport failure carries no + status at all, so it is not a 403 either. + """ + def guard(stderr): + return PR.is_read_only_token_error( + subprocess.CompletedProcess(args=["gh"], returncode=1, stderr=stderr) + ) + + self.assertTrue(guard(self.PERMISSION)) + self.assertTrue(guard(self.POLICY)) + self.assertTrue( + guard("gh: Repository was archived so is read-only. (HTTP 403)"), + "an archived repo refuses every write and no retry fixes it", + ) + self.assertFalse( + guard("gh: Resource not accessible by integration (HTTP 422)"), + "the phrase under a 422 is a validated rejection, not a read-only token", + ) + self.assertFalse( + guard("error connecting to api.github.com: Resource not accessible by x"), + "no status at all is not a 403", + ) + self.assertFalse(guard("")) + self.assertFalse(guard(None), "a CompletedProcess can carry no stderr at all") + + def test_every_throttle_wording_falls_through_the_guard(self): + """The allowlist, pinned to the wordings GitHub actually sends with a 403. + + Each of these can be raised on a request the API went on to serve, so none may + short-circuit the landed-review read. Matched case-insensitively for the same + reason the permission phrase is. + """ + for message in ( + "API rate limit exceeded for installation ID 1234", + "You have exceeded a secondary rate limit. Please wait a few minutes " + "before you try again.", + "You have triggered an abuse detection mechanism.", + "You have been submitted too quickly. Please retry your request again " + "later.", + ): + for label, text in ( + ("as sent", message), + ("lower-cased", message.lower()), + ): + with self.subTest(message=message[:40], case=label): + result = subprocess.CompletedProcess( + args=["gh"], returncode=1, stderr=f"gh: {text} (HTTP 403)" + ) + self.assertTrue(PR.is_throttled_403(result)) + self.assertFalse(PR.is_read_only_token_error(result)) + self.assertIn( + PR.gh_http_status(result), (403,), + "and it keeps its status, so RETRYABLE_4XX_STATUSES takes it", + ) + def test_a_throttled_403_with_the_review_present_skips_the_fallback(self): """A secondary rate limit can be raised on a request GitHub went on to serve. diff --git a/.github/cursor-review/tests/test_post_review_delivery.py b/.github/cursor-review/tests/test_post_review_delivery.py index 4a4f640..d2959ce 100644 --- a/.github/cursor-review/tests/test_post_review_delivery.py +++ b/.github/cursor-review/tests/test_post_review_delivery.py @@ -354,22 +354,47 @@ def test_a_read_only_token_exits_zero_and_reports_not_posted(self): self.assertIsNone(self.exit_code, "the read-only degradation still exits 0") self.assertEqual(delivery["posted"], "false") - def test_a_bare_403_is_no_longer_read_as_read_only(self): + def test_a_throttled_403_is_no_longer_read_as_read_only(self): # BE-12612 inverted this case. is_read_only_token_error used to match any - # stderr carrying "HTTP 403", so a throttle, an abuse-detection refusal or an - # org-policy block exited 0 claiming a read-only token — and returned before - # the landed-review check could ask whether the write had gone through. It now - # matches the PERMISSION MESSAGE only, so a 403 without that message takes the - # read; confirmed absent here, the fallback is thrown and fails the same way, - # which is a genuine POST failure and goes red. `posted` still never lies. + # stderr carrying "HTTP 403", so a secondary rate limit exited 0 claiming a + # read-only token — and returned before the landed-review check could ask + # whether the write had gone through. The throttle wordings are now excluded + # from the guard, so this takes the read; confirmed absent by the driver's + # default empty page, the fallback is thrown and fails the same way, which is + # a genuine POST failure and goes red. `posted` still never lies. + # + # The stderr is `gh`'s real shape — `gh: (HTTP 403)` — because the + # guard now conjoins the status, and the status only parses out of those + # parentheses. Written any other way this case would reach the read through + # the "no status at all" branch and pass whether the fix were here or not. _, delivery = self.run_main( [finding("app.py", 11)], post_returncode=1, - stderr="gh: HTTP 403: Forbidden", + stderr=( + "gh: You have exceeded a secondary rate limit. Please wait a few " + "minutes before you try again. (HTTP 403)" + ), ) self.assertEqual(self.exit_code, 1) self.assertNotEqual(delivery.get("posted"), "true") + def test_an_unworded_403_still_degrades_green(self): + # The other half of the same narrowing, and the one that keeps this from + # being a regression for anyone: a 403 that is NOT a throttle is a standing + # refusal — an SSO/IP-allowlist or org-policy block, an archived repo, a + # reworded permission message — which no retry fixes and which wrote nothing. + # Those keep the pre-BE-12612 behaviour exactly: one attempt, review in the + # job summary, exit 0. Matching "everything that is not the permission + # phrase" into the retry path would have turned every run in such an org red. + posted, delivery = self.run_main( + [finding("app.py", 11)], + post_returncode=1, + stderr="gh: Forbidden (HTTP 403)", + ) + self.assertEqual(len(posted), 1, "no fallback — it would fail the same way") + self.assertIsNone(self.exit_code, "an environment constraint is not red") + self.assertEqual(delivery["posted"], "false") + def test_a_genuine_post_failure_exits_one_and_never_claims_posted(self): # Both attempts fail for a non-403 reason. The job goes red, so the DM's # existing failure branch already covers it — but nothing may claim the diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index d52afff..349008e 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -2824,15 +2824,18 @@ jobs: # Green job, nothing on the PR. The designed-for case is the # read-only-token degradation, but this branch is reached by every # green run that did not post, so state the OBSERVATION as fact and - # the cause as likelihood: is_read_only_token_error treats any HTTP - # 403 as read-only, so an SSO/IP-allowlist, secondary-rate-limit or - # archived-repo rejection lands here too, as does an App + # the cause as likelihood: is_read_only_token_error treats every 403 + # EXCEPT a throttle as read-only (BE-12612), so an SSO/IP-allowlist + # or archived-repo rejection lands here too, as does an App # installation without pull-request write (where the caller's # permissions block is not the knob) and a workflows_ref resolving - # to a post-review.py too old to emit `posted` at all. + # to a post-review.py too old to emit `posted` at all. A + # secondary-rate-limit 403 no longer arrives here: it takes the + # landed-review check instead and fails the job red if nothing + # landed, which the failure branch below already covers. STATUS="warning" TITLE="Cursor review degraded" - DETAIL="No review was posted on the PR — the full text is in the run's job summary. Most often the run's token lacks pull-requests: write (check the caller's permissions block); another 403 (SSO/IP allowlist, rate limit, archived repo) or a stale workflows_ref pin reaches this too." + DETAIL="No review was posted on the PR — the full text is in the run's job summary. Most often the run's token lacks pull-requests: write (check the caller's permissions block); another standing 403 (SSO/IP allowlist, archived repo) or a stale workflows_ref pin reaches this too. A rate limit does NOT reach this — it fails the job red." elif [ "$CONSOLIDATE_RESULT" != "success" ]; then STATUS="warning" TITLE="Cursor review failed to post" From 87c274fcb21fcd2cec7adb1c94bb68cc7cbfb033 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 19:27:05 -0700 Subject: [PATCH 3/6] fix(cursor-review): read every gh status rendering, and ask the PR on all three failure paths (BE-12612) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 panel findings. The status conjunction added in 486af72 made the read-only degradation depend on `_GH_HTTP_STATUS_RE`, which matched only `gh`'s trailing `(HTTP 403)`. go-gh also LEADS with the status — `HTTP 403 ()` when the body carried no message, `HTTP 422: ()\n` when it carried `errors[]` — and those read as "no status at all", so a STANDING permission or SSO 403 rendered that way left the green degrade for a doomed read, a doomed fallback and SystemExit(1) on every run. The regex now matches both renderings, and takes the LAST match so a `GH_DEBUG=api` trace quoting `HTTP 403` in the review body it echoes cannot supply the status for its own failure. The throttle allowlist is now matched against `gh`'s error LINE (new `gh_error_line`) rather than all of stderr, for the same reason from the other direction: under `GH_DEBUG=api` a review DISCUSSING rate-limit handling would have turned its own permission 403 into a "throttle". `main()`'s no-inline-comments branch and `post_or_degrade` now run the same landed-review read the inline path runs, gated by a shared `post_may_have_landed`. Narrowing which 403s reach those paths did not change what happened when one did: a throttle raised on a request GitHub went on to serve exited 1 with `posted=false` while the review sat on the PR, so the fresh-review gate held the check red and the job summary published a second copy. They read; they still never repost, and only a PRESENT answer changes anything. The `notify-complete` prose no longer claims a rate limit never reaches the green degrade — an unworded 403 from an edge or proxy is indistinguishable from a standing refusal and does. Tests: the permission case is driven through both `gh` renderings; the 422 fixture is the real joined-`errors[]` shape (the single-message one it used could not carry the entry the conjunction was for); the vacuous "message lower-cased" subtest is dropped now that the guard never reads the permission wording. New: the status parser over all three renderings plus a debug trace, a debug trace that must not manufacture a throttle, and the landed/absent pairs for the no-inline branch and the no-findings review. --- .github/cursor-review/post-review.py | 204 ++++++++++++--- .../cursor-review/tests/test_post_review.py | 243 ++++++++++++++++-- .github/workflows/cursor-review.yml | 13 +- 3 files changed, 399 insertions(+), 61 deletions(-) diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 981891b..12deb58 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -290,7 +290,9 @@ def gh_post_review(repo: str, pr_number: str, payload: str) -> subprocess.Comple # committed. Matched case-insensitively as substrings, which is how `gh` hands over # GitHub's `message`: echoed into stderr rather than parsed out of the JSON body. # Only ever consulted once the status is already known to be 403, so a finding body -# quoting one of these phrases cannot reach it through a 422. +# quoting one of these phrases cannot reach it through a 422 — and matched against +# `gh`'s own error LINE rather than the whole blob, so it cannot reach it through the +# same PR's review body either (see gh_error_line). THROTTLE_403_MESSAGES = ( # "API rate limit exceeded for ..." / "You have exceeded a secondary rate limit." "rate limit", @@ -302,9 +304,17 @@ def gh_post_review(repo: str, pr_number: str, payload: str) -> subprocess.Comple def is_throttled_403(result: subprocess.CompletedProcess) -> bool: - """True when a 403's message is GitHub asking us to slow down.""" - blob = (result.stderr or "").lower() - return any(message in blob for message in THROTTLE_403_MESSAGES) + """True when a 403's message is GitHub asking us to slow down. + + Read out of `gh`'s error line, not out of all of stderr. With `GH_DEBUG=api` set + on the step — a documented `gh` knob a caller workflow can add — stderr also + carries the request trace, which echoes the review body being POSTed; a review + that DISCUSSES rate-limit handling would otherwise turn a standing permission 403 + into a "throttle", costing it a doomed read, a doomed fallback and a red step + where it used to degrade green. + """ + line = gh_error_line(result).lower() + return any(message in line for message in THROTTLE_403_MESSAGES) def is_read_only_token_error(result: subprocess.CompletedProcess) -> bool: @@ -338,11 +348,19 @@ def is_read_only_token_error(result: subprocess.CompletedProcess) -> bool: keeps a SAML-blocked or IP-allowlisted org on that green degrade instead of the permanently red check "everything but the permission phrase" would hand it. - The fall-through reaches the landed-review check on the INLINE path in `main()` - only. `main()`'s no-inline-comments branch and `post_or_degrade` have no such - read: there a throttled 403 is reported as the POST failure it is — red, review in - the job summary — rather than mislabelled a read-only token. Neither of those - posts a fallback, so neither can duplicate a write GitHub committed before erroring. + The fall-through reaches the landed-review check on ALL THREE failure paths — + `main()`'s inline branch, its no-inline-comments branch, and `post_or_degrade` — + because `post_may_have_landed` gates each of them the same way. A throttle raised + on a request GitHub went on to serve is therefore reported as delivered wherever + it happens, rather than red with `posted=false` over a review sitting on the PR. + Only the inline path REPOSTS on the other two answers; the other two read and, + unless the answer is PRESENT, behave exactly as they always have. + + One residual, named rather than implied: the read runs on the same token GitHub + just throttled, so it can be throttled too. That yields UNKNOWN, and on the inline + path UNKNOWN posts the fallback — which duplicates a first write that did land. + Closing that needs `Retry-After`/backoff, which `gh api` does not surface on the + default path; it is tracked separately (BE-12679) rather than half-done here. """ return gh_http_status(result) == 403 and not is_throttled_403(result) @@ -354,17 +372,62 @@ def is_read_only_token_error(result: subprocess.CompletedProcess) -> bool: # way build-ledger.py pins its own copy. CONSOLIDATED_MARKER = "## 🔍 Cursor Review — Consolidated panel" -# `gh api` reports the HTTP status in its stderr, e.g. -# `gh: Unprocessable Entity (HTTP 422)`. A transport failure (DNS, TLS, a dropped -# connection) carries no status at all, which is why the caller treats "no match" as -# unknown rather than as a server error. -_GH_HTTP_STATUS_RE = re.compile(r"\(HTTP (\d{3})\)") +# `gh` reports an API error's HTTP status in its stderr, but not in ONE shape. The +# common `gh api` rendering trails it in parentheses (`gh: Unprocessable Entity +# (HTTP 422)`), but go-gh's HTTPError leads with it whenever it has a request URL to +# report and either no message at all — `HTTP 403 (https://api.github.com/...)`, what +# a proxy, a WAF or a GHES edge that sent no JSON body produces — or a message plus an +# `errors[]` tail, `HTTP 422: Validation Failed (https://...)\n`. Both +# alternatives are matched. +# +# Matching only the parenthesized trailer was a REGRESSION risk once +# `is_read_only_token_error` began conjoining the status (BE-12612): the leading +# shapes would have read as "no status at all", so a STANDING permission or SSO 403 +# rendered that way would leave the green degrade for a doomed read, a doomed fallback +# and SystemExit(1) on every run — which the bare `"HTTP 403" in blob` it replaced did +# not do. +# +# A transport failure (DNS, TLS, a dropped connection) carries no status in either +# shape, which is why the caller treats "no match" as unknown rather than as a server +# error. +_GH_HTTP_STATUS_RE = re.compile(r"\(HTTP (\d{3})\)|\bHTTP (\d{3})\b") + + +def _gh_status_match(result: subprocess.CompletedProcess): + """The LAST status rendering on stderr, as a match, or None if there is none. + + Last, not first: with `GH_DEBUG=api` stderr also carries the request/response + trace, and the request it echoes is the review body being POSTed — which can quote + anything, `HTTP 403` included. `gh` writes its own error AFTER that trace, so the + final match is the one describing the response rather than something quoting one. + """ + matches = list(_GH_HTTP_STATUS_RE.finditer(result.stderr or "")) + return matches[-1] if matches else None def gh_http_status(result: subprocess.CompletedProcess): """The HTTP status `gh` reported on stderr, or None when it reported none.""" - match = _GH_HTTP_STATUS_RE.search(result.stderr or "") - return int(match.group(1)) if match else None + match = _gh_status_match(result) + if match is None: + return None + return int(match.group(1) or match.group(2)) + + +def gh_error_line(result: subprocess.CompletedProcess) -> str: + """The one stderr line carrying that status — `gh`'s own error line, or "". + + The scope for anything that reads the WORDING of a failure. Both of `gh`'s + renderings put GitHub's `message` on the same line as the status, so this is all + of what GitHub said and none of what an `errors[]` continuation, a `GH_DEBUG=api` + trace or a quoted review body put around it. + """ + match = _gh_status_match(result) + if match is None: + return "" + blob = result.stderr or "" + start = blob.rfind("\n", 0, match.start()) + 1 + end = blob.find("\n", match.end()) + return blob[start:] if end == -1 else blob[start:end] # 4xx statuses that are NOT evidence the request was rejected before it was written. @@ -386,6 +449,27 @@ def gh_http_status(result: subprocess.CompletedProcess): RETRYABLE_4XX_STATUSES = frozenset({403, 408, 425, 429}) +def post_may_have_landed(result: subprocess.CompletedProcess) -> bool: + """Could GitHub have committed this write despite erroring on the request? + + False only for the 4xx that mean "GitHub VALIDATED this and refused it before + writing anything" — every 4xx outside RETRYABLE_4XX_STATUSES, whose members are + 4xx without carrying that meaning. A 5xx, and a transport error with no status at + all, leave the write genuinely undecided. + + True is not "the review landed"; it is "the PR is worth asking". Shared by all + three failure paths so the question is answered the same way on each — the + no-inline branch and post_or_degrade used to skip it entirely, which reported + `posted=false` for a review that was on the PR the whole time (BE-12612). + """ + status = gh_http_status(result) + return not ( + status is not None + and 400 <= status < 500 + and status not in RETRYABLE_4XX_STATUSES + ) + + # This read sits on the RECOVERY path: the fallback POST and write_step_summary both # come after it, so a call that hangs takes the round out of BOTH channels — the job's # `timeout-minutes: 10` kills the process before either runs, where the pre-BE-12528 @@ -693,8 +777,9 @@ def post_or_degrade( read-only branch returns True as well and is never a delivery: the review reached a job summary, not the PR, so no thread exists to hold the merge on. """ - result = gh_post_review(repo, pr_number, payload) - if result.returncode == 0: + + def report_posted(): + """This body is on the PR. Shared by the two ways of finding that out.""" # `posted` regardless of `delivers`: a body that reports a failure still # reached the PR, and the DM's claim is about the PR, not about adjudication. # A clamped-but-posted review is posted too — hence before the `truncated` @@ -707,6 +792,10 @@ def post_or_degrade( file=sys.stderr, ) write_step_summary(summary_markdown, note=TRUNCATED_SUMMARY_NOTE) + + result = gh_post_review(repo, pr_number, payload) + if result.returncode == 0: + report_posted() return True if is_read_only_token_error(result): print( @@ -718,6 +807,31 @@ def post_or_degrade( write_step_summary(summary_markdown) return True print(f"{context} POST failed: {result.stderr}", file=sys.stderr) + # A nonzero `gh` is not proof the write was refused. Once the throttle wordings + # stopped being read as a read-only token (BE-12612), the 403 GitHub raises on a + # request it went on to SERVE reaches here — and every caller answers a False by + # writing the same text to the job summary and exiting 1. That publishes a second + # copy of a review already on the PR and reports `posted=false` for it, which the + # fresh-review gate then holds the check red over. So ask the PR, on exactly the + # statuses the inline path asks on. This is a READ, never a repost: on ABSENT and + # on UNKNOWN this returns False and the caller behaves as it always has. + if post_may_have_landed(result): + # Read the commit and the body back out of the REQUEST rather than taking them + # as parameters: `review_already_posted` answers True only for a byte-identical + # body at the same head SHA, so the two have to be the ones this call actually + # sent. `payload` is that request, and every caller builds it with json.dumps. + request = json.loads(payload) + landed = review_already_posted( + repo, pr_number, request.get("commit_id") or "", request.get("body") or "" + ) + if landed is True: + print( + f"{context}: the POST errored but this exact review is on the PR — " + "treating it as delivered rather than reporting it lost.", + file=sys.stderr, + ) + report_posted() + return True return False @@ -1948,6 +2062,24 @@ def finish_posted_review(): # if GitHub committed the write before erroring it publishes a DUPLICATE # review no one can un-post. That duplicate risk, not byte-identity, is the # reason to skip it. Deliver the text to the summary and let the step go red. + # "No fallback to post" is not "no question to ask", though. A throttled 403 + # (or a 5xx, or a dropped connection) can be raised on a request GitHub went + # on to SERVE, and reporting THAT as `posted=false` leaves the review on the + # PR while the fresh-review gate holds the check red for a review that landed + # and the job summary publishes a second copy of it. Same read as the inline + # path below, on the same statuses, and still no repost: only a PRESENT answer + # changes anything here. + if post_may_have_landed(result) and review_already_posted( + args.repo, args.pr_number, args.commit_sha, posted_body + ) is True: + print( + f"Review: the POST errored ({(result.stderr or '').strip()[:200]}) but " + f"a review for {args.commit_sha[:7]} is on the PR — treating as " + "delivered.", + file=sys.stderr, + ) + finish_posted_review() + return print( "Review: no inline comments to drop — the fallback would repost the same " "body, so writing it to the job summary instead.", @@ -1958,36 +2090,32 @@ def finish_posted_review(): raise SystemExit(1) # Did that POST really fail to land? A nonzero `gh` is not proof it did not — - # the `not comments` branch above already declines to repost for exactly that - # reason — and the answer decides two things below: whether to post the fallback - # at all, and whether the findings that anchored may be labelled lost. + # the `not comments` branch above asks the same question for the same reason, and + # declines to repost whatever the answer — and here the answer decides two things + # below: whether to post the fallback at all, and whether the findings that + # anchored may be labelled lost. # - # Cheapest sufficient evidence first. A 4xx is GitHub VALIDATING and rejecting the - # request before writing anything (every firing observed in the field is a 422 over - # an inline position), so the review is absent by construction and no read is worth - # the call — with the exception carved out by RETRYABLE_4XX_STATUSES, whose members - # are 4xx without carrying that meaning: an edge or a proxy said so, or GitHub - # throttled a request it may well have gone on to serve. Anything else — a 5xx, or - # a transport error that carries no status at all — leaves the write genuinely - # undecided, so ask the PR. Three outcomes follow: + # Cheapest sufficient evidence first, which is what `post_may_have_landed` weighs: + # a 4xx is GitHub VALIDATING and rejecting the request before writing anything + # (every firing observed in the field is a 422 over an inline position), so the + # review is absent by construction and no read is worth the call — with the + # exception carved out by RETRYABLE_4XX_STATUSES, whose members are 4xx without + # carrying that meaning: an edge or a proxy said so, or GitHub throttled a request + # it may well have gone on to serve. Anything else — a 5xx, or a transport error + # that carries no status at all — leaves the write genuinely undecided, so ask the + # PR. Three outcomes follow: # PRESENT (the review landed: report it delivered, post nothing more), ABSENT # (behave exactly as this path always has), and UNKNOWN (post the fallback, but tag # nothing `lost_to_fallback` — the flag is a claim, and an unreadable list supports # none). UNKNOWN is why the read failing is not answered as a `False`: that would # be indistinguishable from a confirmed-absent review and would relabel findings on # the strength of a transient blip. - status = gh_http_status(result) - pre_write_rejection = ( - status is not None - and 400 <= status < 500 - and status not in RETRYABLE_4XX_STATUSES - ) - if pre_write_rejection: - landed = False - else: + if post_may_have_landed(result): landed = review_already_posted( args.repo, args.pr_number, args.commit_sha, posted_body ) + else: + landed = False if landed is True: print( diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index 3e12eda..97a6a12 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -967,7 +967,10 @@ def test_a_5xx_with_the_review_confirmed_absent_tags_post_failed(self): existing_reviews=[], list_calls=calls, ) - self.assertEqual(calls, [("o/r", "1")], "asked the PR exactly once") + # Twice, not once: the stub fails the FALLBACK the same way, and post_or_degrade + # asks the same question about that POST for the same reason (BE-12612). Both + # reads answer ABSENT here, so the behaviour below is unchanged. + self.assertEqual(calls, [("o/r", "1"), ("o/r", "1")]) self.assertEqual(len(posted), 2) ledger = ledger_from_posted_body(posted[1]["body"]) self.assertEqual(ledger["post_failed_count"], len(self.ANCHORED)) @@ -1079,7 +1082,8 @@ def test_a_transport_error_with_no_http_status_goes_through_the_read(self): existing_reviews=[], list_calls=calls, ) - self.assertEqual(calls, [("o/r", "1")]) + # Once for the inline POST, once for the fallback the stub fails identically. + self.assertEqual(calls, [("o/r", "1"), ("o/r", "1")]) def test_the_consolidated_marker_matches_gate_unresolved(self): """One discriminator, three readers (the gate, the ledger, and now this) — so @@ -1345,16 +1349,24 @@ def test_the_permission_403_still_degrades_to_the_summary(self): A read-only token rejects the fallback exactly as it rejected the first POST, and no read is worth the call because nothing was written — so this branch - still returns before either. The lower-case variant is here because the - MESSAGE match is case-insensitive by design: `gh` echoes GitHub's message and - nothing guarantees its capitalisation. Only the message is lower-cased — - `(HTTP 403)` is `gh`'s own rendering, not GitHub's text, and it is fixed. + still returns before either. + + Both of `gh`'s renderings, because the guard now conjoins the status and + `_GH_HTTP_STATUS_RE` has to find a 403 in either of them: the parenthesized + trailer `gh api` usually prints, and go-gh's leading `HTTP 403: ()`. + Reading the second as "no status" would drop this case out of the degrade and + into a doomed read, a doomed fallback and a permanently red check. There is no + lower-cased variant any more: since BE-12612 the guard is `403 and not a + throttle` and never inspects the permission wording at all, so a case-folded + copy of it would exercise the identical path. The surviving case-insensitive + match is the throttle allowlist, pinned by + `test_every_throttle_wording_falls_through_the_guard`. """ for label, stderr in ( ("as GitHub sends it", self.PERMISSION), - ("message lower-cased", self.PERMISSION.replace( - "Resource not accessible by integration", - "resource not accessible by integration", + ("go-gh's leading-status rendering", ( + "gh: HTTP 403: Resource not accessible by integration " + "(https://api.github.com/repos/o/r/pulls/1/reviews)" )), ): with self.subTest(message=label): @@ -1413,31 +1425,59 @@ def test_a_policy_403_still_degrades_to_the_summary(self): self.assertEqual(notes, [None], "under the read-only banner default") self.assertIsNone(driver.exit_code, "an environment constraint is not red") + # The rendering go-gh reaches for when a 4xx body carries `errors[]` alongside + # `message`: the status LEADS, the first message line goes on that line, and the + # rest follows after a newline. Written out because it is the shape that actually + # carries the permission wording under a 422 — the single-message + # `gh: (HTTP 422)` form by definition cannot, since it has one message. + VALIDATION_422 = ( + "gh: HTTP 422: Validation Failed " + "(https://api.github.com/repos/o/r/pulls/1/reviews)\n" + "Resource not accessible by integration" + ) + def test_the_guard_needs_the_status_as_well_as_the_message(self): """Neither half alone: a 422 can carry the permission phrase, a 403 can not. - `gh` joins a 422's `errors[].message` entries into the same stderr blob, so - the permission wording can arrive under a status that PROVES the write was - validated and refused. Classifying that as a read-only token would return from - `main()` before the landed-review check — the same silent skip BE-12612 is - removing, reached from the other direction. And a transport failure carries no - status at all, so it is not a 403 either. + `gh` renders a 422 whose body carries `errors[]` as `HTTP 422: + ()\n`, so the permission wording can arrive under a status that + PROVES the write was validated and refused. Classifying that as a read-only + token would return from `main()` before the landed-review check — the same + silent skip BE-12612 is removing, reached from the other direction. That is + also why `_GH_HTTP_STATUS_RE` has to read the leading rendering: matching only + the parenthesized trailer would score this blob as "no status", and no-status + is not a 403 either, so the guard would answer the same False by accident + while a leading-status 403 answered False for real. And a transport failure + carries no status in any rendering, so it is not a 403 at all. """ def guard(stderr): return PR.is_read_only_token_error( subprocess.CompletedProcess(args=["gh"], returncode=1, stderr=stderr) ) + def status(stderr): + return PR.gh_http_status( + subprocess.CompletedProcess(args=["gh"], returncode=1, stderr=stderr) + ) + self.assertTrue(guard(self.PERMISSION)) self.assertTrue(guard(self.POLICY)) self.assertTrue( guard("gh: Repository was archived so is read-only. (HTTP 403)"), "an archived repo refuses every write and no retry fixes it", ) + self.assertEqual( + status(self.VALIDATION_422), 422, + "the joined rendering must parse, or this case proves nothing", + ) self.assertFalse( - guard("gh: Resource not accessible by integration (HTTP 422)"), + guard(self.VALIDATION_422), "the phrase under a 422 is a validated rejection, not a read-only token", ) + self.assertFalse( + guard("gh: Resource not accessible by integration (HTTP 422)"), + "and the single-message rendering of the same status likewise", + ) self.assertFalse( guard("error connecting to api.github.com: Resource not accessible by x"), "no status at all is not a 403", @@ -1522,7 +1562,10 @@ def test_a_throttled_403_with_the_review_absent_fails_red(self): summaries=summaries, notes=notes, ) - self.assertEqual(calls, [("o/r", "1")]) + self.assertEqual( + calls, [("o/r", "1"), ("o/r", "1")], + "the fallback is throttled too, and it gets the same read", + ) self.assertEqual(len(posted), 2, "inline attempt, then the body-only fallback") self.assertEqual(outputs["delivered"], "false") self.assertIn(PR.POST_FAILED_SUMMARY_NOTE, notes) @@ -1543,13 +1586,177 @@ def test_a_throttled_403_with_an_unreadable_list_tags_nothing(self): list_returncode=1, list_calls=calls, ) - self.assertEqual(calls, [("o/r", "1")]) + self.assertEqual(calls, [("o/r", "1"), ("o/r", "1")]) self.assertEqual(len(posted), 2, "undecided means post the fallback") ledger = ledger_from_posted_body(posted[1]["body"]) for entry in ledger["entries"]: self.assertNotIn("lost_to_fallback", entry) + def test_gh_status_is_read_out_of_every_rendering_gh_uses(self): + """One status parser, three renderings, and the LAST one wins. + + `gh api` usually trails the status in parentheses, but go-gh leads with it + whenever it has a request URL and either no message or a message plus an + `errors[]` tail. Since BE-12612 made the read-only degradation depend on the + status, a rendering this could not parse would score as "no status" and take a + STANDING refusal into the retry path — so all three have to parse. + + Last-match, because `GH_DEBUG=api` puts the request trace on the same stderr, + and the request being traced is the review body: a review discussing `HTTP + 403` handling would otherwise supply the status for its own failure. + """ + def status(stderr): + return PR.gh_http_status( + subprocess.CompletedProcess(args=["gh"], returncode=1, stderr=stderr) + ) + + self.assertEqual(status("gh: Unprocessable Entity (HTTP 422)"), 422) + self.assertEqual( + status("gh: HTTP 403 (https://api.github.com/repos/o/r/pulls/1/reviews)"), + 403, + "no message at all — a proxy, a WAF or a GHES edge with no JSON body", + ) + self.assertEqual( + status( + "gh: HTTP 422: Validation Failed (https://api.github.com/x)\n" + "Resource not accessible by integration" + ), + 422, + ) + self.assertEqual( + status( + "* Request at 2026-01-01\n" + "> POST /repos/o/r/pulls/1/reviews\n" + '> {"body": "the review body quotes HTTP 403 verbatim"}\n' + "< HTTP/2.0 500 Internal Server Error\n" + "gh: Server Error (HTTP 500)" + ), + 500, + "gh's own error is written last, so it is the one that describes this run", + ) + self.assertIsNone(status("error connecting to api.github.com")) + self.assertIsNone(status(None)) + + def test_a_debug_trace_quoting_a_throttle_does_not_make_one(self): + """The allowlist reads `gh`'s error LINE, not everything on stderr. + + `GH_DEBUG=api` is a documented knob a caller workflow can set on the step, and + it echoes the POSTed review body — so a review that DISCUSSES rate limiting + would, under a whole-blob match, turn its own standing permission 403 into a + "throttle": doomed read, doomed fallback, red step, on a run that used to + degrade green and stay green. + """ + traced = ( + "* Request at 2026-01-01\n" + "> POST /repos/o/r/pulls/1/reviews\n" + '> {"body": "a rate limit is answered with 403, see abuse detection"}\n' + "gh: Resource not accessible by integration (HTTP 403)" + ) + result = subprocess.CompletedProcess(args=["gh"], returncode=1, stderr=traced) + self.assertFalse(PR.is_throttled_403(result)) + self.assertTrue(PR.is_read_only_token_error(result)) + self.assertEqual( + PR.gh_error_line(result), + "gh: Resource not accessible by integration (HTTP 403)", + ) + + def test_a_throttled_403_with_no_inline_half_reports_the_landed_review(self): + """The no-inline branch asks the PR too, and answers PRESENT as delivered. + + Round 1 narrowed WHICH 403s reach this branch; it did not change what happened + when one did. A throttle raised on a request GitHub went on to serve exited 1 + with `posted=false` while the review sat on the PR — so the fresh-review gate + held the check red over a review that had landed, and the job summary + published a second copy of it. It reads now, and it still never reposts. + """ + outputs, summaries, notes, calls = {}, [], [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + [finding("elsewhere.py", 7)], + post_returncode=1, + stderr=self.THROTTLED, + existing_reviews=[self.landed_review()], + list_calls=calls, + outputs=outputs, + summaries=summaries, + notes=notes, + ) + self.assertEqual(posted[0].get("comments", []), [], "no inline half to drop") + self.assertEqual(calls, [("o/r", "1")], "asked the PR once") + self.assertEqual(len(posted), 1, "and still posted nothing more") + self.assertEqual(outputs["posted"], "true") + self.assertEqual(outputs["delivered"], "true") + self.assertEqual(summaries, [], "the review is on the PR, not the summary") + self.assertIsNone(driver.exit_code, "a review that landed is not a red step") + + def test_a_throttled_403_with_no_inline_half_and_nothing_landed_stays_red(self): + """Only PRESENT changes this branch. ABSENT behaves exactly as it always has.""" + outputs, summaries, notes, calls = {}, [], [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + [finding("elsewhere.py", 7)], + post_returncode=1, + stderr=self.THROTTLED, + existing_reviews=[], + list_calls=calls, + outputs=outputs, + summaries=summaries, + notes=notes, + ) + self.assertEqual(calls, [("o/r", "1")]) + self.assertEqual(len(posted), 1, "still no byte-identical repost") + self.assertEqual(outputs["posted"], "false") + self.assertEqual(len(summaries), 1, "the review goes to the job summary") + self.assertIn(PR.POST_FAILED_SUMMARY_NOTE, notes) + self.assertEqual(driver.exit_code, 1) + + def test_a_throttled_403_on_the_no_findings_review_reports_the_landed_review(self): + """post_or_degrade asks too — and the no-findings review is the common round. + + Nothing to demote and no fallback to post, so before BE-12612 this was a flat + `posted=false` plus SystemExit(1) even when the throttled POST had been served. + """ + outputs, summaries, notes, calls = {}, [], [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + [], + post_returncode=1, + stderr=self.THROTTLED, + existing_reviews=[self.landed_review()], + list_calls=calls, + outputs=outputs, + summaries=summaries, + notes=notes, + ) + self.assertIn("No high-signal findings", posted[0]["body"]) + self.assertEqual(calls, [("o/r", "1")]) + self.assertEqual(len(posted), 1, "post_or_degrade reads, it never reposts") + self.assertEqual(outputs["posted"], "true") + self.assertEqual(outputs["delivered"], "true") + self.assertEqual(summaries, []) + self.assertIsNone(driver.exit_code) + + def test_a_standing_403_on_the_no_findings_review_never_reaches_the_read(self): + """The other side of it: a permission refusal still degrades green, unread.""" + outputs, summaries, notes, calls = {}, [], [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + [], + post_returncode=1, + stderr=self.PERMISSION, + list_calls=calls, + outputs=outputs, + summaries=summaries, + notes=notes, + ) + self.assertEqual(calls, [], "a read-only token wrote nothing to read") + self.assertEqual(len(posted), 1) + self.assertEqual(outputs["posted"], "false") + self.assertEqual(notes, [None], "under the read-only banner default") + self.assertIsNone(driver.exit_code) + + class FitSentinelItemsTest(unittest.TestCase): """The budget search behind the prose floor.""" diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 349008e..319e3ed 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -2829,13 +2829,16 @@ jobs: # or archived-repo rejection lands here too, as does an App # installation without pull-request write (where the caller's # permissions block is not the knob) and a workflows_ref resolving - # to a post-review.py too old to emit `posted` at all. A - # secondary-rate-limit 403 no longer arrives here: it takes the - # landed-review check instead and fails the job red if nothing - # landed, which the failure branch below already covers. + # to a post-review.py too old to emit `posted` at all. A throttle + # GitHub WORDS as one no longer arrives here: it takes the + # landed-review check instead and fails red if nothing landed, which + # the failure branch below already covers. An unworded 403 — what an + # edge, a WAF or a GHES proxy returns with no JSON body — carries + # nothing to tell those apart by, so it is read as standing and does + # still land here; hence "most often", not "always". STATUS="warning" TITLE="Cursor review degraded" - DETAIL="No review was posted on the PR — the full text is in the run's job summary. Most often the run's token lacks pull-requests: write (check the caller's permissions block); another standing 403 (SSO/IP allowlist, archived repo) or a stale workflows_ref pin reaches this too. A rate limit does NOT reach this — it fails the job red." + DETAIL="No review was posted on the PR — the full text is in the run's job summary. Most often the run's token lacks pull-requests: write (check the caller's permissions block); another standing 403 (SSO/IP allowlist, archived repo) or a stale workflows_ref pin reaches this too. A 403 GitHub worded as a rate limit does not reach this — it takes the landed-review check and fails the job red if nothing landed — but a 403 carrying no message at all (an edge or proxy) is indistinguishable from a standing refusal and does, so check the run log for the actual error before ruling a throttle out." elif [ "$CONSOLIDATE_RESULT" != "success" ]; then STATUS="warning" TITLE="Cursor review failed to post" From 5180568ba417065c1c982f7cf5d0481dd33e150d Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 19:33:39 -0700 Subject: [PATCH 4/6] docs(cursor-review): refresh two test docstrings the round-2 refactor left stale (BE-12612) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pre_write_rejection` is now `post_may_have_landed` with the inverted sense, and the throttle allowlist is no longer case-insensitive 'for the same reason the permission phrase is' — the guard stopped matching that phrase at all. --- .github/cursor-review/tests/test_post_review.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index 97a6a12..6dcdacb 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -1312,9 +1312,10 @@ class ReadOnlyGuardExcludesThrottlesTest(unittest.TestCase): token, written to the job summary and exited 0, with the PR never asked whether the review had actually landed. The guard now excludes those wordings, and 403 joins RETRYABLE_4XX_STATUSES so what falls through takes the read instead of being - assumed absent. Both halves are needed: narrowing the guard alone would send a - throttled 403 into `pre_write_rejection`, which treats a 4xx outside that set as - absent by construction and skips the read just the same. + assumed absent. Both halves are needed: narrowing the guard alone would leave + `post_may_have_landed` answering False for a throttled 403 — it reads a 4xx + outside that set as absent by construction — and the read would be skipped just + the same. The narrowing is an ALLOWLIST of throttles, not a denylist of the permission phrase, and `test_a_policy_403_still_degrades_to_the_summary` is why: every OTHER @@ -1489,8 +1490,9 @@ def test_every_throttle_wording_falls_through_the_guard(self): """The allowlist, pinned to the wordings GitHub actually sends with a 403. Each of these can be raised on a request the API went on to serve, so none may - short-circuit the landed-review read. Matched case-insensitively for the same - reason the permission phrase is. + short-circuit the landed-review read. Matched case-insensitively because `gh` + echoes GitHub's message verbatim and nothing guarantees its capitalisation — + this allowlist is the only case-insensitive match left in the guard. """ for message in ( "API rate limit exceeded for installation ID 1234", From 07235a1392b7de47650f4f955f47bacc44a28d4c Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 21:16:11 -0700 Subject: [PATCH 5/6] fix(cursor-review): back off per Retry-After before the landed-review read (BE-12691) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BE-12612 stopped a throttled POST being misread as a read-only token and routed it into the landed-review read — but that read goes out on the very token GitHub just throttled, immediately, so it was liable to be throttled in its turn. The answer that produced was UNKNOWN, and UNKNOWN on the inline path posted the body-only fallback: a duplicate review whenever the throttled first POST was one the API went on to serve, and a review cannot be un-posted. Two halves. `gh_post_review` now passes `-i`, so the response headers reach `gh_response_headers` (stdout only — stderr can carry an echo of the POSTed review body under GH_DEBUG=api, and a finding must not be able to dictate how long this process sleeps). `confirm_landed` — the one seam all three failure paths read the PR through — waits `Retry-After`, else the `X-RateLimit-Reset` window, else GitHub's stated one-minute minimum, clamped to 90s, before asking. And what is still UNKNOWN after that wait no longer reposts: the findings go to the job summary under POST_FAILED_SUMMARY_NOTE, `posted=false` keeps the fresh-review gate red rather than green over a write nobody confirmed, and the step goes red. Confirmed ABSENT under a throttle waits once more and then posts the fallback as before; 408/425/5xx/no-status keep the immediate read and never wait, since none of them says a rate window is closed. --- .github/cursor-review/README.md | 4 + .github/cursor-review/post-review.py | 226 ++++++- .../cursor-review/tests/test_post_review.py | 577 +++++++++++++++++- .../tests/test_post_review_delivery.py | 183 +++++- .github/workflows/cursor-review.yml | 18 +- 5 files changed, 981 insertions(+), 27 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 4531332..acc896e 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -71,6 +71,10 @@ a fresh runner with a fresh pinned checkout there is nothing tampered left for t minted token to meet. `tests/test_workflow_job_isolation.py` pins the property, and [`pr-size.yml`](../workflows/pr-size.yml) uses the identical split for its comment job. +### Delivery, the body-only fallback, and a throttled POST + +The review reaches the PR as one `POST /pulls/{n}/reviews`. When GitHub rejects that request over an inline position, the run retries **once** without anchors — the same findings as prose, in one body-only review — and when even that fails, or the run's token cannot write to the PR at all, the review is written to the job summary instead so the findings are never lost outright. A nonzero response is not proof nothing was written, though: GitHub answers a rate limit, a secondary rate limit or abuse detection with **429 or 403** as readily on a request it went on to *serve* as on one it refused. So before the retry the run asks the PR whether the first review actually landed, and reposts only when the answer is a confirmed **absent**. Under a throttle it also **waits first** — `Retry-After`, else the `X-RateLimit-Reset` window, clamped to 90 seconds — because that read goes out on the very token GitHub just throttled and an immediate one tends to come back unreadable. If the answer is *still* unreadable after the wait, the run declines the fallback rather than risk publishing a second copy of a review nobody can un-post: the findings go to the job summary, `posted=false` keeps the [blocking gate](#optional-make-the-review-blocking) red rather than green over an unverified write, and the step fails. The waits are bounded so the whole worst case stays inside the post job's ten-minute budget, and nothing here ever retries a write it could not confirm was absent. + ### The prior-review ledger and the repeat policy With `ledger_prior_review` on (the default), [`build-ledger.py`](build-ledger.py) rebuilds what earlier rounds raised on this PR — each finding, its thread, and the author's replies — and splices it into the panel and judge prompts as untrusted DATA. The rule it enforces is that only an **answered** finding costs a repeat slot: a finding the author or a maintainer replied to may be raised again only if the judge emits `repeat_of` (that thread's permalink) and `repeat_round`, and at most `REPEAT_CAP` such re-raises survive per review, so a round can never be all re-litigation. A finding nobody answered is free to raise again. diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 12deb58..e048659 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -28,10 +28,12 @@ import argparse import json +import math import os import re import subprocess import sys +import time # Severity scale, ordered most → least urgent. Drives sort order, the inline # comment prefix, and the summary table. The judge tool accepts one @@ -267,10 +269,22 @@ def neutralize_mentions(text: str) -> str: def gh_post_review(repo: str, pr_number: str, payload: str) -> subprocess.CompletedProcess: + """POST the review, keeping the RESPONSE HEADERS (BE-12691). + + `-i` is what makes `Retry-After` / `X-RateLimit-Reset` reachable: `gh api` prints + only the body by default, and there is no other knob that surfaces a response + header. Everything else about the call is unchanged — verified on gh 2.92.0, the + exit code is still nonzero on an error, stderr is still byte-identical + (`gh: (HTTP nnn)`), and the status line + headers + body all go to STDOUT, + which nothing but `gh_response_headers` reads. So `gh_http_status`, + `gh_error_line`, `is_throttled_403` and `is_read_only_token_error` — every one of + which reads stderr — are unaffected. + """ return subprocess.run( [ "gh", "api", + "-i", "--method", "POST", f"/repos/{repo}/pulls/{pr_number}/reviews", @@ -283,6 +297,45 @@ def gh_post_review(repo: str, pr_number: str, payload: str) -> subprocess.Comple ) +# The first line `gh -i` writes: `HTTP/2.0 403 Forbidden`, `HTTP/1.1 429 Too Many +# Requests`. Required before anything is read as a header, so a stdout carrying only a +# JSON body — what an older `gh`, or any caller that stubs the POST, produces — is +# UNPARSEABLE rather than a header block whose first field happens to look like one. +_GH_STATUS_LINE_RE = re.compile(r"^HTTP/\S+\s+\d{3}\b") + + +def gh_response_headers(result: subprocess.CompletedProcess) -> dict[str, str]: + """The response headers `gh -i` wrote to stdout, lower-cased, or `{}`. + + Read from stdout on purpose: stderr carries `gh`'s error line and, under + `GH_DEBUG=api`, an echo of the POSTed review body — so a finding that QUOTES a + `Retry-After:` line could dictate how long this process sleeps if the headers were + scraped from there. stdout is `gh`'s own rendering of the response and nothing + else. + + Names are lower-cased because `gh` hands back Go's canonical form + (`X-Ratelimit-Reset`, not the `X-RateLimit-Reset` GitHub documents), and callers + must not have to guess which spelling arrived. A repeated header keeps its LAST + value, which is what an HTTP client would use. + """ + blob = result.stdout or "" + lines = blob.split("\n") + if not _GH_STATUS_LINE_RE.match(lines[0].rstrip("\r")): + return {} + headers: dict[str, str] = {} + for raw in lines[1:]: + line = raw.rstrip("\r") + # The blank line ends the header block; everything after it is the body, which + # is attacker-influenced JSON and must never be read as a header. + if not line.strip(): + break + name, sep, value = line.partition(":") + if not sep: + continue + headers[name.strip().lower()] = value.strip() + return headers + + # The 403 wordings that mean "slow down", not "you may not write". GitHub answers a # primary rate limit, a secondary rate limit and abuse detection with 403 as readily # as with 429 — and, unlike every other 403, one of those can be raised on a request @@ -317,6 +370,85 @@ def is_throttled_403(result: subprocess.CompletedProcess) -> bool: return any(message in line for message in THROTTLE_403_MESSAGES) +def is_throttle(result: subprocess.CompletedProcess) -> bool: + """Is this failure GitHub asking us to slow down, on either status it uses? + + 429 by status alone; 403 only with one of the throttle wordings, since every other + 403 is a standing refusal (see `is_read_only_token_error`). The 403 arm conjoins + the status rather than trusting the wording alone, which is the precondition + `is_throttled_403` documents: its allowlist is a substring match over `gh`'s error + LINE, so a 422 whose validation message happened to quote "rate limit" would + otherwise be treated as a throttle and earn a sleep it cannot benefit from. + + 408 and 425 are deliberately NOT throttles. They are in RETRYABLE_4XX_STATUSES + because an edge or a proxy can raise them on a request GitHub went on to serve — + so they still take the landed-review read — but neither says a rate window is + open, so neither buys anything by waiting. They keep the immediate read. + """ + status = gh_http_status(result) + return status == 429 or (status == 403 and is_throttled_403(result)) + + +# GitHub's own guidance for a secondary rate limit with no `Retry-After`: "wait for at +# least one minute before retrying". The MAX is this repo's, not GitHub's — see the +# budget note in `throttle_delay_seconds` — and the MIN keeps a `Retry-After: 0` from +# turning the wait into no wait at all, which is the shape that earns a second +# throttle. +THROTTLE_DELAY_DEFAULT_SECONDS = 60 +THROTTLE_DELAY_MAX_SECONDS = 90 +THROTTLE_DELAY_MIN_SECONDS = 1 + + +def _non_negative_int(value): + """`value` as a non-negative int, or None when it is missing or not one.""" + try: + parsed = int(str(value).strip()) + except (TypeError, ValueError): + return None + return parsed if parsed >= 0 else None + + +def throttle_delay_seconds(result: subprocess.CompletedProcess, now=None) -> int: + """How long to wait before touching the API again after a throttled write. + + GitHub's rate-limit best-practices order, each rule falling through to the next + when its header is missing, unparseable or negative: + + 1. `Retry-After` — the seconds GitHub itself asked for (secondary limits). + 2. `X-RateLimit-Remaining: 0` plus a parseable `X-RateLimit-Reset` — a PRIMARY + limit, which lifts at the reset epoch and not before. A reset already in the + past says the window reopened, so it is not evidence of anything and falls + through rather than yielding a zero wait. + 3. `THROTTLE_DELAY_DEFAULT_SECONDS`. + + The result is clamped to [MIN, MAX] whichever rule produced it, so a header + cannot dictate an unbounded sleep inside a job with a wall-clock budget. + + BUDGET. The `post` job runs on `timeout-minutes: 10`. A worst case that throttles + at every turn spends three of these sleeps — before the inline POST's + landed-review read, before the fallback POST, and before `post_or_degrade`'s own + read of that fallback — plus at most two reads, each bounded by + GH_LIST_REVIEWS_TIMEOUT_SECONDS. That is 3 x 90 + 2 x 60 = 390s, comfortably + inside the budget with the three artifact downloads and the token mint alongside + it. MAX is what keeps that arithmetic true: a `Retry-After` of an hour (GitHub + sends one on a primary limit) would otherwise take the job out entirely, and the + step summary — the fallback delivery channel — is written AFTER these waits. + """ + headers = gh_response_headers(result) + delay = _non_negative_int(headers.get("retry-after")) + if delay is None and headers.get("x-ratelimit-remaining") == "0": + reset = _non_negative_int(headers.get("x-ratelimit-reset")) + if reset is not None: + # Rounded UP: `time.time()` is fractional, and truncating would wake up to + # a second BEFORE the window GitHub named actually reopens — which is a + # retry into the same limit rather than a wait. + remaining = math.ceil(reset - (time.time() if now is None else now)) + delay = remaining if remaining >= 0 else None + if delay is None: + delay = THROTTLE_DELAY_DEFAULT_SECONDS + return max(THROTTLE_DELAY_MIN_SECONDS, min(THROTTLE_DELAY_MAX_SECONDS, delay)) + + def is_read_only_token_error(result: subprocess.CompletedProcess) -> bool: """True when the POST failed because the ENVIRONMENT forbids writing to the PR. @@ -356,11 +488,11 @@ def is_read_only_token_error(result: subprocess.CompletedProcess) -> bool: Only the inline path REPOSTS on the other two answers; the other two read and, unless the answer is PRESENT, behave exactly as they always have. - One residual, named rather than implied: the read runs on the same token GitHub - just throttled, so it can be throttled too. That yields UNKNOWN, and on the inline - path UNKNOWN posts the fallback — which duplicates a first write that did land. - Closing that needs `Retry-After`/backoff, which `gh api` does not surface on the - default path; it is tracked separately (BE-12679) rather than half-done here. + The residual this left — the read runs on the same token GitHub just throttled, so + it can be throttled too, and the resulting UNKNOWN used to post a fallback that + duplicated a first write that did land — is closed by BE-12691: `confirm_landed` + backs off per `Retry-After` before the read, and an UNKNOWN under a throttle + declines the fallback rather than risking the duplicate. """ return gh_http_status(result) == 403 and not is_throttled_403(result) @@ -654,6 +786,34 @@ def review_already_posted( return False +def confirm_landed(result, repo: str, pr_number: str, commit_sha: str, posted_body: str): + """`review_already_posted`, with a throttle backoff in front of it (BE-12691). + + The ONE seam every failure path reads the PR through, so the backoff cannot be + added to two of the three and forgotten on the last. Three-valued exactly as + `review_already_posted` is, and it changes nothing about PRESENT or ABSENT. + + What it changes is UNKNOWN. The read runs on the SAME token GitHub just throttled, + so issuing it immediately is close to asking for a second throttle — and the + answer that produces is "could not tell", which is the one answer that used to + cost a duplicate review. Waiting out the window GitHub named is what turns most of + those UNKNOWNs into a real PRESENT or ABSENT. + + Only a throttle waits. A 5xx, a dropped connection, a 408 or a 425 all still take + the read immediately: none of them says a rate window is closed, so a sleep would + buy nothing and spend the job's budget. + """ + if is_throttle(result): + delay = throttle_delay_seconds(result) + print( + f"Review: GitHub throttled the POST (HTTP {gh_http_status(result)}) — " + f"waiting {delay}s before checking whether it landed", + file=sys.stderr, + ) + time.sleep(delay) + return review_already_posted(repo, pr_number, commit_sha, posted_body) + + READ_ONLY_SUMMARY_NOTE = ( "> ℹ️ This review could not be posted on the PR because the run's " "`GITHUB_TOKEN` is read-only (e.g. read-only default workflow " @@ -821,8 +981,12 @@ def report_posted(): # body at the same head SHA, so the two have to be the ones this call actually # sent. `payload` is that request, and every caller builds it with json.dumps. request = json.loads(payload) - landed = review_already_posted( - repo, pr_number, request.get("commit_id") or "", request.get("body") or "" + landed = confirm_landed( + result, + repo, + pr_number, + request.get("commit_id") or "", + request.get("body") or "", ) if landed is True: print( @@ -2069,8 +2233,8 @@ def finish_posted_review(): # and the job summary publishes a second copy of it. Same read as the inline # path below, on the same statuses, and still no repost: only a PRESENT answer # changes anything here. - if post_may_have_landed(result) and review_already_posted( - args.repo, args.pr_number, args.commit_sha, posted_body + if post_may_have_landed(result) and confirm_landed( + result, args.repo, args.pr_number, args.commit_sha, posted_body ) is True: print( f"Review: the POST errored ({(result.stderr or '').strip()[:200]}) but " @@ -2111,8 +2275,8 @@ def finish_posted_review(): # be indistinguishable from a confirmed-absent review and would relabel findings on # the strength of a transient blip. if post_may_have_landed(result): - landed = review_already_posted( - args.repo, args.pr_number, args.commit_sha, posted_body + landed = confirm_landed( + result, args.repo, args.pr_number, args.commit_sha, posted_body ) else: landed = False @@ -2126,7 +2290,11 @@ def finish_posted_review(): ) finish_posted_review() return - if landed is None: + # UNDER A THROTTLE, an UNKNOWN does not reach the fallback at all (BE-12691) — the + # branch below declines it — so this line would misreport what happens next. It + # stays for every other undecided cause (a 5xx, a dropped connection, a 408/425), + # where the fallback still goes out with nothing tagged. + if landed is None and not is_throttle(result): print( "Review: could not confirm whether the first POST landed (review list " "unreadable) — posting the fallback with no finding tagged [post-failed].", @@ -2254,6 +2422,40 @@ def finish_posted_review(): fallback_head_with_sentinel, [i["comment"] for i in enriched] ) clamped_fallback = clamp_review_body(fallback_body) + + # The fallback is a SECOND WRITE on the token GitHub just throttled, and the two + # answers still on the table need opposite treatment (BE-12691). `landed is True` + # returned above, so this is UNKNOWN or confirmed-ABSENT. + if is_throttle(result): + if landed is None: + # Fail closed. A throttle can be raised on a request the API went on to + # SERVE, the read that would have told us was itself throttled, and a + # review posted twice cannot be un-posted — so the unverified write is the + # one thing not to do. The findings still reach the job summary, and + # `posted=false` keeps the fresh-review gate RED rather than green over a + # write nobody confirmed. Same fail-closed shape as the `not comments` + # branch above, which declines its (byte-identical) repost on every answer. + print( + "Review: the POST was throttled and the landed-review read could not " + "confirm it is absent — not reposting (a second write on a throttled " + "token risks a duplicate); writing to the job summary instead.", + file=sys.stderr, + ) + emit_delivery(False) + write_step_summary(fallback_body, note=POST_FAILED_SUMMARY_NOTE) + raise SystemExit(1) + # Confirmed ABSENT: nothing landed, so the fallback is the only copy of this + # round that can reach the PR and it does go out — but on the same token, so + # wait out the window first or it earns the same throttle the first write did. + delay = throttle_delay_seconds(result) + print( + f"Review: the first POST was throttled (HTTP {gh_http_status(result)}) and " + f"the review is confirmed absent — waiting {delay}s before posting the " + "fallback.", + file=sys.stderr, + ) + time.sleep(delay) + fallback_payload = json.dumps( { # Clamped for the API; the step-summary copy stays whole. diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index 6dcdacb..f78212d 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -217,11 +217,21 @@ class EndToEndPostTest(unittest.TestCase): def run_main(self, findings, with_diff=True, post_returncode=0, stderr="", summaries=None, panel=None, existing_reviews=None, list_returncode=0, list_calls=None, - outputs=None, notes=None, raw_stdout=_UNSET, extra_argv=()): + outputs=None, notes=None, raw_stdout=_UNSET, extra_argv=(), + post_stdout="", sleeps=None, trace=None): """Return the POSTed payloads. Pass `summaries` (a list) to collect step-summary writes, or `panel` to control the panel summary — the one finding-INDEPENDENT part of the review head that a caller can make large. + `time.sleep` is ALWAYS stubbed, for the same reason the list read always is: a + throttled POST now backs off before the landed-review read (BE-12691), so an + unpatched clock would make every throttle case in this file wait a real minute + or three. `post_stdout` is what `gh -i` wrote — the status line plus the + response headers the backoff reads. `sleeps` collects the seconds each sleep + was asked for; `trace` collects `("post"|"list"|"sleep", …)` across all three + stubs, which is the only way to assert the backoff happens BEFORE the read + rather than merely alongside it. + The review-list read (BE-12528) is ALWAYS stubbed, never merely when a case cares about it: this harness drives main() end to end, so an unpatched `gh_list_reviews` would shell out to a real `gh` from the unit suite the moment @@ -245,13 +255,23 @@ def run_main(self, findings, with_diff=True, post_returncode=0, stderr="", summa def fake_post(repo, pr_number, payload): posted.append(json.loads(payload)) + if trace is not None: + trace.append(("post",)) return subprocess.CompletedProcess( - args=["gh"], returncode=post_returncode, stdout="", stderr=stderr + args=["gh"], returncode=post_returncode, stdout=post_stdout, stderr=stderr ) + def fake_sleep(seconds): + if sleeps is not None: + sleeps.append(seconds) + if trace is not None: + trace.append(("sleep", seconds)) + def fake_list(repo, pr_number): if list_calls is not None: list_calls.append((repo, pr_number)) + if trace is not None: + trace.append(("list", repo, pr_number)) # A review only counts as THIS run's when its body IS the body this run # posted (BE-12528), which the case cannot spell out ahead of time — it is # assembled by main() from the findings. ECHO_POSTED_BODY stands in for it @@ -304,6 +324,7 @@ def fake_summary(markdown, note=None): argv += list(extra_argv) with mock.patch.object(PR, "gh_post_review", side_effect=fake_post), \ mock.patch.object(PR, "gh_list_reviews", side_effect=fake_list), \ + mock.patch.object(PR.time, "sleep", side_effect=fake_sleep), \ mock.patch.object(PR.sys, "argv", argv), \ mock.patch.dict(os.environ, {"GITHUB_OUTPUT": outpath}, clear=False), \ mock.patch.object(PR, "write_step_summary", side_effect=fake_summary): @@ -1516,6 +1537,51 @@ def test_every_throttle_wording_falls_through_the_guard(self): PR.gh_http_status(result), (403,), "and it keeps its status, so RETRYABLE_4XX_STATUSES takes it", ) + self.assertTrue( + PR.is_throttle(result), + "and it earns a backoff before the read (BE-12691)", + ) + + def test_is_throttle_separates_a_wait_from_every_other_failure(self): + """Which failures buy something by WAITING — a strictly smaller set than the + ones that buy something by asking the PR (BE-12691). + + `RETRYABLE_4XX_STATUSES` answers "could GitHub have served this anyway", and + 408/425 are in it because an edge or a proxy can raise either on a request the + API went on to handle. Neither says a rate window is CLOSED, though, so a sleep + in front of their read would spend the job's ten-minute budget for nothing. + + The 403 arm conjoins the status rather than trusting the wording alone. The + allowlist is a substring match over `gh`'s error line, and a validation message + is free to quote "rate limit" — so a 422 that did would otherwise be handed a + backoff on a request GitHub demonstrably validated and refused. + """ + def throttle(stderr): + return PR.is_throttle( + subprocess.CompletedProcess(args=["gh"], returncode=1, stderr=stderr) + ) + + self.assertTrue(throttle("gh: Too Many Requests (HTTP 429)")) + self.assertTrue( + throttle("gh: anything at all (HTTP 429)"), + "429 needs no wording — the status IS the throttle", + ) + self.assertTrue(throttle(self.THROTTLED)) + self.assertFalse(throttle(self.PERMISSION)) + self.assertFalse(throttle(self.POLICY)) + for status in (408, 425, 422, 500): + self.assertFalse( + throttle(f"gh: something (HTTP {status})"), + f"{status} is not a throttle", + ) + self.assertFalse( + throttle("gh: rate limit wording under a validation failure (HTTP 422)"), + "the wording alone is not enough — the status has to be 403", + ) + self.assertFalse( + throttle("error connecting to api.github.com"), + "no status at all is not a throttle either", + ) def test_a_throttled_403_with_the_review_present_skips_the_fallback(self): """A secondary rate limit can be raised on a request GitHub went on to serve. @@ -1574,22 +1640,60 @@ def test_a_throttled_403_with_the_review_absent_fails_red(self): self.assertNotIn(None, notes, "the read-only banner never applies here") self.assertEqual(driver.exit_code, 1, "the step goes red") - def test_a_throttled_403_with_an_unreadable_list_tags_nothing(self): - """UNKNOWN survives the new status too: post the fallback, claim nothing. + def test_a_throttled_403_with_an_unreadable_list_declines_the_fallback(self): + """UNKNOWN under a THROTTLE declines the second write entirely (BE-12691). + + Round 1 (BE-12612) routed the throttle into the read and left UNKNOWN posting + the fallback, which is a duplicate review whenever the throttled first POST was + one GitHub went on to serve — and a review cannot be un-posted. So the throttle + case now fails CLOSED: findings to the job summary under + POST_FAILED_SUMMARY_NOTE, `posted=false` so the fresh-review gate stays red + over a write nobody confirmed, and a red step. - `lost_to_fallback` asserts the first review is absent, and a read that failed - supports no such claim (BE-4785). + UNKNOWN from any OTHER cause is untouched — see + `test_an_unknown_read_that_is_not_a_throttle_still_posts_the_fallback`. """ - calls = [] - posted = EndToEndPostTest().run_main( + calls, outputs, summaries, notes = [], {}, [], [] + driver = EndToEndPostTest() + posted = driver.run_main( self.ANCHORED, post_returncode=1, stderr=self.THROTTLED, list_returncode=1, list_calls=calls, + outputs=outputs, + summaries=summaries, + notes=notes, + ) + self.assertEqual(calls, [("o/r", "1")], "asked once; the answer was unreadable") + self.assertEqual(len(posted), 1, "and no second write went out on that token") + self.assertEqual(outputs["delivered"], "false") + self.assertEqual(outputs["posted"], "false") + self.assertEqual(len(summaries), 1, "the findings still reach the job summary") + self.assertIn(PR.POST_FAILED_SUMMARY_NOTE, notes) + self.assertEqual(driver.exit_code, 1, "an unverified write is not a green step") + + def test_an_unknown_read_that_is_not_a_throttle_still_posts_the_fallback(self): + """The other side of the decline: only a THROTTLE withholds the fallback. + + A 5xx, a dropped connection, a 408 or a 425 can also leave the write undecided, + but none of them says a rate window is closed and none of them is evidence the + first request was served — so the pre-BE-12691 behaviour stands: post the + fallback, and tag nothing `lost_to_fallback`, since that flag asserts the first + review is ABSENT and a read that failed supports no such claim (BE-4785). + """ + calls, sleeps = [], [] + posted = EndToEndPostTest().run_main( + self.ANCHORED, + post_returncode=1, + stderr="gh: Server Error (HTTP 500)", + list_returncode=1, + list_calls=calls, + sleeps=sleeps, ) self.assertEqual(calls, [("o/r", "1"), ("o/r", "1")]) - self.assertEqual(len(posted), 2, "undecided means post the fallback") + self.assertEqual(sleeps, [], "nothing here is a throttle, so nothing waits") + self.assertEqual(len(posted), 2, "undecided-but-unthrottled posts the fallback") ledger = ledger_from_posted_body(posted[1]["body"]) for entry in ledger["entries"]: self.assertNotIn("lost_to_fallback", entry) @@ -1759,6 +1863,461 @@ def test_a_standing_403_on_the_no_findings_review_never_reaches_the_read(self): self.assertIsNone(driver.exit_code) +# What `gh api -i` really writes to stdout: the status line, CRLF-terminated headers +# in Go's canonical casing, a blank line, then the body. Captured from gh 2.92.0 +# against a 404 probe (`gh api -i /repos/o/r/pulls/99999999`) and trimmed — the +# CRLFs, the `X-Ratelimit-*` spelling and the JSON tail are all verbatim, because +# each of them is something the parser has to get right. +GH_INCLUDE_STDOUT = ( + "HTTP/2.0 404 Not Found\r\n" + "Content-Type: application/json; charset=utf-8\r\n" + "Date: Wed, 09 Sep 2026 04:05:32 GMT\r\n" + "X-Ratelimit-Limit: 5000\r\n" + "X-Ratelimit-Remaining: 3887\r\n" + "X-Ratelimit-Reset: 1788927701\r\n" + "\r\n" + '{"message":"Not Found","status":"404"}' +) + + +def gh_result(stdout="", stderr="", returncode=1): + return subprocess.CompletedProcess( + args=["gh"], returncode=returncode, stdout=stdout, stderr=stderr + ) + + +def response(*header_lines, status="HTTP/2.0 429 Too Many Requests", body="{}"): + """A `gh -i` stdout carrying exactly `header_lines`.""" + return "".join([f"{status}\r\n"] + [f"{h}\r\n" for h in header_lines] + ["\r\n", body]) + + +class ResponseHeaderParsingTest(unittest.TestCase): + """`gh -i`'s stdout, read back as headers (BE-12691). + + `gh api` prints only the response BODY by default, and there is no knob that + surfaces a single header — so `Retry-After` and `X-RateLimit-Reset` are reachable + only by asking for the whole response. That makes stdout, previously ignored on + every path, something the process now reads, and this pins what it may conclude + from it. + """ + + def test_it_parses_the_shape_gh_actually_writes(self): + headers = PR.gh_response_headers(gh_result(stdout=GH_INCLUDE_STDOUT)) + self.assertEqual(headers["x-ratelimit-remaining"], "3887") + self.assertEqual(headers["x-ratelimit-reset"], "1788927701") + self.assertEqual( + headers["content-type"], "application/json; charset=utf-8", + "the value keeps its own colons — only the FIRST one splits", + ) + self.assertNotIn("message", headers, "the body is past the blank line") + + def test_names_are_lower_cased(self): + """`gh` canonicalizes to `X-Ratelimit-Reset`; GitHub documents + `X-RateLimit-Reset`. Neither spelling may be the one a caller has to guess.""" + headers = PR.gh_response_headers(gh_result(stdout=GH_INCLUDE_STDOUT)) + self.assertIn("x-ratelimit-reset", headers) + self.assertNotIn("X-Ratelimit-Reset", headers) + + def test_empty_or_unparseable_stdout_is_no_headers(self): + for label, stdout in ( + ("empty", ""), + ("stubbed away", None), + # An older `gh`, or any caller that did not pass `-i`, writes the body + # alone. Reading its first line as a header would let a review body's own + # text dictate the backoff. + ("a bare JSON body", '{"id": 1, "Retry-After": "3600"}'), + ("headers with no status line", "Retry-After: 30\r\n\r\n{}"), + ): + with self.subTest(stdout=label): + self.assertEqual(PR.gh_response_headers(gh_result(stdout=stdout)), {}) + + def test_a_header_block_with_no_body_still_parses(self): + headers = PR.gh_response_headers( + gh_result(stdout="HTTP/1.1 429 Too Many Requests\r\nRetry-After: 60\r\n") + ) + self.assertEqual(headers, {"retry-after": "60"}) + + +class ThrottleDelayTest(unittest.TestCase): + """How long a throttled write waits, and why it is never longer than that. + + Precedence follows GitHub's rate-limit best-practices doc: `Retry-After` first + (secondary limits), then `X-RateLimit-Remaining: 0` + `X-RateLimit-Reset` (a + primary limit, which lifts at the reset epoch), then a one-minute default — + GitHub's own "wait for at least one minute" for a throttle that named no window. + """ + + NOW = 1_788_927_701 + + def delay(self, *header_lines, **kwargs): + return PR.throttle_delay_seconds( + gh_result(stdout=response(*header_lines)), **kwargs + ) + + def test_retry_after_wins(self): + self.assertEqual(self.delay("Retry-After: 30"), 30) + + def test_retry_after_is_capped(self): + """GitHub sends an hour-long `Retry-After` on a primary limit. The `post` job + has ten minutes total and writes the job-summary fallback AFTER the wait, so an + uncapped sleep would lose the review from both channels rather than one.""" + self.assertEqual(self.delay("retry-after: 300"), PR.THROTTLE_DELAY_MAX_SECONDS) + self.assertEqual(PR.THROTTLE_DELAY_MAX_SECONDS, 90) + + def test_a_zero_retry_after_still_waits_the_floor(self): + """Zero is a wait GitHub asked for, not a missing header — but retrying with no + pause at all is the shape that earns the next throttle.""" + self.assertEqual(self.delay("Retry-After: 0"), PR.THROTTLE_DELAY_MIN_SECONDS) + self.assertEqual(PR.THROTTLE_DELAY_MIN_SECONDS, 1) + + def test_an_exhausted_primary_limit_waits_for_its_reset(self): + self.assertEqual( + self.delay( + "X-Ratelimit-Remaining: 0", + f"X-Ratelimit-Reset: {self.NOW + 45}", + now=self.NOW, + ), + 45, + ) + + def test_a_primary_limit_with_budget_left_is_not_a_window(self): + """`remaining` above zero means the limit that fired was the SECONDARY one, so + its reset epoch says nothing about when this write may be retried.""" + self.assertEqual( + self.delay( + "X-Ratelimit-Remaining: 7", + f"X-Ratelimit-Reset: {self.NOW + 45}", + now=self.NOW, + ), + PR.THROTTLE_DELAY_DEFAULT_SECONDS, + ) + + def test_a_reset_in_the_past_falls_through(self): + self.assertEqual( + self.delay( + "X-Ratelimit-Remaining: 0", + f"X-Ratelimit-Reset: {self.NOW - 45}", + now=self.NOW, + ), + PR.THROTTLE_DELAY_DEFAULT_SECONDS, + ) + + def test_no_headers_at_all_takes_the_default(self): + self.assertEqual( + PR.throttle_delay_seconds(gh_result(stdout="")), + PR.THROTTLE_DELAY_DEFAULT_SECONDS, + ) + self.assertEqual(PR.THROTTLE_DELAY_DEFAULT_SECONDS, 60) + + def test_unusable_values_fall_to_the_next_rule(self): + """Each header is untrusted text off the wire; a value that is not a + non-negative integer is not evidence, so it is skipped rather than guessed at.""" + for label, lines in ( + ("non-numeric retry-after", ("Retry-After: soon",)), + ("HTTP-date retry-after", ("Retry-After: Wed, 09 Sep 2026 04:06:32 GMT",)), + ("negative retry-after", ("Retry-After: -5",)), + ("empty retry-after", ("Retry-After:",)), + ("unparseable reset", ("X-Ratelimit-Remaining: 0", "X-Ratelimit-Reset: soon")), + ("remaining zero with no reset", ("X-Ratelimit-Remaining: 0",)), + ): + with self.subTest(case=label): + self.assertEqual( + self.delay(*lines, now=self.NOW), + PR.THROTTLE_DELAY_DEFAULT_SECONDS, + ) + + def test_every_answer_is_inside_the_clamp(self): + for lines in ( + ("Retry-After: 999999",), + ("Retry-After: 0",), + ("X-Ratelimit-Remaining: 0", f"X-Ratelimit-Reset: {1_788_927_701 + 3600}"), + (), + ): + with self.subTest(headers=lines): + value = self.delay(*lines, now=self.NOW) + self.assertGreaterEqual(value, PR.THROTTLE_DELAY_MIN_SECONDS) + self.assertLessEqual(value, PR.THROTTLE_DELAY_MAX_SECONDS) + + +class ThrottleBackoffTest(unittest.TestCase): + """The wait itself: before the landed-review read, and before the fallback POST. + + THE FAILURE THIS PINS (BE-12691). BE-12612 stopped a throttled POST being misread + as a read-only token and routed it into the landed-review read — but that read goes + out on the SAME token GitHub just throttled, immediately, so it was liable to be + throttled in its turn. The answer that produced was UNKNOWN, and UNKNOWN on the + inline path posted the fallback: a DUPLICATE review whenever the throttled first + POST was one the API went on to serve, and a review cannot be un-posted. + + Two halves, and both are needed. The backoff turns most of those UNKNOWNs into a + real answer by waiting out the window GitHub named. What is still UNKNOWN after it + declines the second write and fails closed instead — the findings reach the job + summary, `posted=false` keeps the fresh-review gate red rather than green over a + write nobody confirmed, and the step goes red. + """ + + ANCHORED = [finding("app.py", 11), finding("app.py", 12)] + THROTTLED_429 = "gh: You have exceeded a secondary rate limit. (HTTP 429)" + RESPONSE_429 = response("Retry-After: 30") + + def landed_review(self, **overrides): + return FirstReviewConfirmationTest().landed_review(**overrides) + + def test_the_backoff_happens_before_the_read_not_after_it(self): + """The whole point of the wait: the read must land on the far side of it. + + Pinned on a shared call-order trace rather than on two independent counters, + because a sleep issued AFTER the read is indistinguishable from this one by + call count alone and buys nothing at all. + """ + trace, sleeps = [], [] + EndToEndPostTest().run_main( + self.ANCHORED, + post_returncode=1, + stderr=self.THROTTLED_429, + post_stdout=self.RESPONSE_429, + existing_reviews=[self.landed_review()], + trace=trace, + sleeps=sleeps, + ) + self.assertEqual( + trace, + [("post",), ("sleep", 30), ("list", "o/r", "1")], + "POST, then the Retry-After wait, then the read", + ) + self.assertEqual(sleeps, [30], "the wait is the one GitHub asked for") + + def test_a_throttled_post_whose_review_landed_posts_nothing_more(self): + outputs, summaries, sleeps = {}, [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + self.ANCHORED, + post_returncode=1, + stderr=self.THROTTLED_429, + post_stdout=self.RESPONSE_429, + existing_reviews=[self.landed_review()], + outputs=outputs, + summaries=summaries, + sleeps=sleeps, + ) + self.assertEqual(len(posted), 1, "PRESENT never reposts") + self.assertEqual(sleeps, [30], "and waits once, for the read only") + self.assertEqual(outputs["delivered"], "true") + self.assertEqual(outputs["posted"], "true") + self.assertEqual(summaries, [], "the review is on the PR") + self.assertIsNone(driver.exit_code) + + def test_a_throttled_post_confirmed_absent_waits_again_then_falls_back(self): + """ABSENT is the one answer that authorizes the second write — and it goes out + on the same throttled token, so it waits the window out a second time.""" + outputs, sleeps, trace = {}, [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + self.ANCHORED, + post_returncode=1, + stderr=self.THROTTLED_429, + post_stdout=self.RESPONSE_429, + existing_reviews=[], + outputs=outputs, + sleeps=sleeps, + trace=trace, + ) + self.assertEqual(len(posted), 2, "inline attempt, then the body-only fallback") + self.assertEqual( + [step[0] for step in trace], + # The fallback is throttled too (the stub answers every POST the same way), + # so it takes the same wait-then-read treatment through post_or_degrade. + ["post", "sleep", "list", "sleep", "post", "sleep", "list"], + ) + self.assertEqual(sleeps, [30, 30, 30]) + self.assertEqual(outputs["delivered"], "false") + self.assertEqual(driver.exit_code, 1) + + def test_an_unknown_answer_declines_the_fallback_and_fails_closed(self): + outputs, summaries, notes, sleeps = {}, [], [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + self.ANCHORED, + post_returncode=1, + stderr=self.THROTTLED_429, + post_stdout=self.RESPONSE_429, + list_returncode=1, + outputs=outputs, + summaries=summaries, + notes=notes, + sleeps=sleeps, + ) + self.assertEqual(len(posted), 1, "no unverified second write") + self.assertEqual(sleeps, [30], "it waited once, then gave up rather than retry") + self.assertEqual(outputs["delivered"], "false") + self.assertEqual(outputs["posted"], "false") + self.assertEqual(len(summaries), 1) + self.assertIn(PR.POST_FAILED_SUMMARY_NOTE, notes) + self.assertEqual(driver.exit_code, 1) + + def test_a_timeout_status_takes_the_read_immediately(self): + """408 and 425 are in RETRYABLE_4XX_STATUSES — an edge can raise either on a + request GitHub served — but neither says a rate window is closed, so neither + buys anything by waiting and both keep the immediate read.""" + for status in (408, 425): + with self.subTest(status=status): + sleeps, calls = [], [] + EndToEndPostTest().run_main( + self.ANCHORED, + post_returncode=1, + stderr=f"gh: Request Timeout (HTTP {status})", + post_stdout=response("Retry-After: 30", status=f"HTTP/2.0 {status} x"), + existing_reviews=[self.landed_review()], + sleeps=sleeps, + list_calls=calls, + ) + self.assertEqual(sleeps, [], "no wait") + self.assertEqual(calls, [("o/r", "1")], "but still the read") + + def test_a_throttle_with_no_headers_waits_the_default(self): + """`gh -i` is what supplies the headers; a response that carried none — or a + `gh` too old to have been asked for them — still waits GitHub's stated + minimum rather than retrying straight away.""" + sleeps = [] + EndToEndPostTest().run_main( + self.ANCHORED, + post_returncode=1, + stderr=self.THROTTLED_429, + existing_reviews=[self.landed_review()], + sleeps=sleeps, + ) + self.assertEqual(sleeps, [PR.THROTTLE_DELAY_DEFAULT_SECONDS]) + + def test_a_successful_post_never_waits(self): + sleeps = [] + posted = EndToEndPostTest().run_main( + self.ANCHORED, post_stdout=GH_INCLUDE_STDOUT, sleeps=sleeps + ) + self.assertEqual(len(posted), 1) + self.assertEqual(sleeps, [], "the happy path reads no headers and waits not at all") + + def test_the_no_inline_branch_waits_once_and_still_never_reposts(self): + """The branch with no inline half to drop asks the PR too — and its answer only + ever changes whether the round is reported delivered, never whether a second + body is written.""" + for label, reviews, posted_flag, exit_code in ( + ("landed", [self.landed_review()], "true", None), + ("absent", [], "false", 1), + ): + with self.subTest(answer=label): + outputs, sleeps, trace = {}, [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + [finding("elsewhere.py", 7)], + post_returncode=1, + stderr=self.THROTTLED_429, + post_stdout=self.RESPONSE_429, + existing_reviews=reviews, + outputs=outputs, + sleeps=sleeps, + trace=trace, + ) + self.assertEqual(posted[0].get("comments", []), []) + self.assertEqual(len(posted), 1, "this branch never reposts") + self.assertEqual(sleeps, [30], "one wait, before the one read") + self.assertEqual( + [step[0] for step in trace], ["post", "sleep", "list"] + ) + self.assertEqual(outputs["posted"], posted_flag) + self.assertEqual(driver.exit_code, exit_code) + + def test_post_or_degrade_waits_before_its_read_and_never_reposts(self): + """The third seam: the no-findings review, which post_or_degrade owns.""" + for label, reviews, posted_flag, exit_code in ( + ("landed", [self.landed_review()], "true", None), + ("absent", [], "false", 1), + ): + with self.subTest(answer=label): + outputs, sleeps, trace = {}, [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + [], + post_returncode=1, + stderr=self.THROTTLED_429, + post_stdout=self.RESPONSE_429, + existing_reviews=reviews, + outputs=outputs, + sleeps=sleeps, + trace=trace, + ) + self.assertIn("No high-signal findings", posted[0]["body"]) + self.assertEqual(len(posted), 1, "post_or_degrade reads, never reposts") + self.assertEqual(sleeps, [30]) + self.assertEqual( + [step[0] for step in trace], ["post", "sleep", "list"] + ) + self.assertEqual(outputs["posted"], posted_flag) + self.assertEqual(driver.exit_code, exit_code) + + def test_a_standing_403_never_waits(self): + """It returns on the read-only degradation well above the backoff, so a + permission or policy refusal costs no wall-clock at all.""" + sleeps, calls = [], [] + driver = EndToEndPostTest() + driver.run_main( + self.ANCHORED, + post_returncode=1, + stderr="gh: Resource not accessible by integration (HTTP 403)", + post_stdout=response("Retry-After: 30", status="HTTP/2.0 403 Forbidden"), + sleeps=sleeps, + list_calls=calls, + ) + self.assertEqual(sleeps, []) + self.assertEqual(calls, []) + self.assertIsNone(driver.exit_code) + + +class IncludeFlagTest(unittest.TestCase): + """`-i` is what makes the headers reachable, and it changes nothing else. + + Verified against gh 2.92.0: the exit code is still nonzero on an error and stderr + is still `gh: (HTTP nnn)`, byte for byte. Every classifier in this module + reads stderr, so a rendering change there would have re-broken the read-only / + throttle split BE-12612 had just narrowed. + """ + + def test_the_post_asks_for_the_response_headers(self): + captured = {} + + def fake_run(argv, **kwargs): + captured["argv"] = argv + return subprocess.CompletedProcess(args=argv, returncode=0, stdout="", stderr="") + + with mock.patch.object(PR.subprocess, "run", side_effect=fake_run): + PR.gh_post_review("o/r", "1", "{}") + self.assertIn("-i", captured["argv"]) + self.assertEqual(captured["argv"][:3], ["gh", "api", "-i"]) + self.assertIn("--method", captured["argv"]) + self.assertIn("/repos/o/r/pulls/1/reviews", captured["argv"]) + + def test_the_stderr_classifiers_are_unchanged_by_it(self): + """The pin BE-12612 left behind: same stderr, same verdicts, with `-i` adding a + stdout that none of them reads.""" + cases = ( + ("gh: Unprocessable Entity (HTTP 422)", 422, False, False), + ("gh: Resource not accessible by integration (HTTP 403)", 403, True, False), + ( + "gh: You have exceeded a secondary rate limit. (HTTP 403)", + 403, False, True, + ), + ("gh: Too Many Requests (HTTP 429)", 429, False, True), + ("gh: Server Error (HTTP 500)", 500, False, False), + ) + for stderr, status, read_only, throttle in cases: + for label, stdout in (("no -i", ""), ("with -i", GH_INCLUDE_STDOUT)): + with self.subTest(stderr=stderr[:40], stdout=label): + result = gh_result(stdout=stdout, stderr=stderr) + self.assertEqual(PR.gh_http_status(result), status) + self.assertEqual(PR.gh_error_line(result), stderr) + self.assertEqual(PR.is_read_only_token_error(result), read_only) + self.assertEqual(PR.is_throttle(result), throttle) + + class FitSentinelItemsTest(unittest.TestCase): """The budget search behind the prose floor.""" diff --git a/.github/cursor-review/tests/test_post_review_delivery.py b/.github/cursor-review/tests/test_post_review_delivery.py index d2959ce..d506a5f 100644 --- a/.github/cursor-review/tests/test_post_review_delivery.py +++ b/.github/cursor-review/tests/test_post_review_delivery.py @@ -58,6 +58,22 @@ def finding(path, line, severity="high", body="msg"): return {"file": path, "line": line, "severity": severity, "body": body} +# Stands in, inside an `existing_reviews` fixture, for "the body this run actually +# POSTed" — which a case cannot write out, since main() assembles it from the findings. +# Compared by IDENTITY in the harness, so it can never collide with a real body. +ECHO_POSTED_BODY = "" + + +def landed_review(): + """The review this run posted, as the PR would carry it back.""" + return { + "state": "COMMENTED", + "commit_id": "deadbeef", + "user": {"type": "Bot"}, + "body": ECHO_POSTED_BODY, + } + + class MainDriverMixin: """Drive main() with a stubbed `gh` and read what it wrote to $GITHUB_OUTPUT.""" @@ -81,6 +97,10 @@ def run_main( with_diff=True, fallback_ok=False, existing_reviews=(), + post_stdout="", + list_returncode=0, + list_calls=None, + sleeps=None, ): """Return (posted_payloads, delivery_dict). delivery is {} when nothing was written. @@ -94,6 +114,13 @@ def run_main( status — which is what a 403 now does (BE-12612). `existing_reviews` is the flat list the PR carries, wrapped in the one `--slurp` page the real command returns; the default empty page is "confirmed absent". + + `time.sleep` is ALWAYS stubbed too (BE-12691): a throttled POST now backs off + before the landed-review read, so an unpatched clock would make the throttle + cases below wait real minutes. `post_stdout` is what `gh -i` wrote (status line + + response headers, which is where the backoff reads `Retry-After`), `sleeps` + collects the seconds each wait was asked for, and `list_returncode` / + `list_calls` model and count an UNREADABLE review list. """ posted = [] @@ -103,17 +130,32 @@ def fake_post(repo, pr_number, payload): if fallback_ok and len(posted) > 1: rc, err = 0, "" return subprocess.CompletedProcess( - args=["gh"], returncode=rc, stdout="", stderr=err + args=["gh"], returncode=rc, stdout=post_stdout, stderr=err ) def fake_list(repo, pr_number): + if list_calls is not None: + list_calls.append((repo, pr_number)) + # A review counts as THIS run's only when its body IS the body this run + # POSTed, which a case cannot spell out ahead of time — main() assembles it + # from the findings. ECHO_POSTED_BODY stands in and is resolved here, after + # the POST, from the payload actually sent. + reviews = [] + for review in existing_reviews: + if review.get("body") is ECHO_POSTED_BODY: + review = {**review, "body": posted[0]["body"]} + reviews.append(review) return subprocess.CompletedProcess( args=["gh"], - returncode=0, - stdout=json.dumps([list(existing_reviews)]), + returncode=list_returncode, + stdout=json.dumps([reviews]), stderr="", ) + def fake_sleep(seconds): + if sleeps is not None: + sleeps.append(seconds) + if panel is None: panel = [{"model": "m", "review_type": "adversarial", "status": "ok"}] @@ -139,6 +181,7 @@ def fake_list(repo, pr_number): with mock.patch.object(PR, "gh_post_review", side_effect=fake_post), \ mock.patch.object(PR, "gh_list_reviews", side_effect=fake_list), \ + mock.patch.object(PR.time, "sleep", side_effect=fake_sleep), \ mock.patch.object(PR.sys, "argv", argv), \ mock.patch.object(PR, "write_step_summary", lambda *a, **k: None), \ mock.patch.dict(os.environ, {"GITHUB_OUTPUT": outpath}, clear=False): @@ -542,6 +585,140 @@ def boom(*a, **k): self.assertNotIn("posted=true", written) +class ThrottledDeliverySignalTest(MainDriverMixin, unittest.TestCase): + """What a THROTTLED write may claim about the PR (BE-12691). + + `posted` is the DM's question — "is one consolidated review on the PR?" — and a + throttle is the one failure that can be raised on a request the API went on to + serve. BE-12612 made the script ask the PR instead of guessing; this pins what it + does when the ANSWER is also unavailable, because the read went out on the same + throttled token. + + The rule: never claim, and never write again. `posted=false` on an unconfirmed + write keeps the fresh-review gate red (a false `true` would green it over nothing), + and the fallback is withheld (a second write would duplicate a review nobody can + un-post). The findings still reach the job summary either way. + + THROTTLED_403 is written in `gh`'s real shape — `gh: (HTTP 403)` — for + the reason BE-12612's cases are: the classification conjoins the status, and the + status only parses out of those parentheses. + """ + + THROTTLED_403 = ( + "gh: You have exceeded a secondary rate limit. Please wait a few minutes " + "before you try again. (HTTP 403)" + ) + # `gh -i`'s stdout for that response: status line, one header, blank line, body. + RESPONSE = ( + "HTTP/2.0 403 Forbidden\r\n" + "Retry-After: 42\r\n" + "\r\n" + '{"message":"You have exceeded a secondary rate limit."}' + ) + + def test_the_no_inline_branch_waits_once_and_reports_the_landed_review(self): + """A demoted-only round has no inline half to drop, so it never reposts — but + it does ask, after waiting out the window GitHub named.""" + calls, sleeps = [], [] + posted, delivery = self.run_main( + [finding("app.py", 900)], + post_returncode=1, + stderr=self.THROTTLED_403, + post_stdout=self.RESPONSE, + existing_reviews=[landed_review()], + list_calls=calls, + sleeps=sleeps, + ) + self.assertEqual(posted[0].get("comments", []), [], "nothing anchored") + self.assertEqual(len(posted), 1, "and nothing more was written") + self.assertEqual(sleeps, [42], "one wait, for the Retry-After GitHub sent") + self.assertEqual(calls, [("o/r", "1")], "one read, after it") + self.assertEqual(delivery["posted"], "true") + self.assertIsNone(self.exit_code, "a review that landed is not a red step") + + def test_the_no_inline_branch_stays_red_when_nothing_landed(self): + calls, sleeps = [], [] + posted, delivery = self.run_main( + [finding("app.py", 900)], + post_returncode=1, + stderr=self.THROTTLED_403, + post_stdout=self.RESPONSE, + list_calls=calls, + sleeps=sleeps, + ) + self.assertEqual(len(posted), 1, "still no byte-identical repost") + self.assertEqual(sleeps, [42]) + self.assertEqual(calls, [("o/r", "1")]) + self.assertEqual(delivery["posted"], "false") + self.assertEqual(self.exit_code, 1) + + def test_an_unreadable_read_withholds_the_fallback_and_claims_nothing(self): + """The inline path's fail-closed case: throttled POST, unreadable answer. + + Before BE-12691 this posted the fallback — a duplicate whenever the first write + had been served — and reported `posted=false` for it either way. + """ + calls, sleeps = [], [] + posted, delivery = self.run_main( + [finding("app.py", 11), finding("app.py", 900)], + post_returncode=1, + stderr=self.THROTTLED_403, + post_stdout=self.RESPONSE, + list_returncode=1, + list_calls=calls, + sleeps=sleeps, + ) + self.assertEqual( + len(posted[0]["comments"]), 1, + "one finding anchored, so this is the INLINE path, not the no-inline one", + ) + self.assertEqual(len(posted), 1, "the unverified second write never goes out") + self.assertEqual(sleeps, [42], "it waited, then declined rather than retrying") + self.assertEqual(calls, [("o/r", "1")]) + self.assertEqual(delivery["delivered"], "false") + self.assertEqual(delivery["posted"], "false") + self.assertEqual(self.exit_code, 1) + + def test_post_or_degrade_waits_before_its_read_and_never_reposts(self): + """The no-findings review — the common round — goes through post_or_degrade, + which reads and, since BE-12612, reports a landed review as delivered.""" + calls, sleeps = [], [] + posted, delivery = self.run_main( + [], + post_returncode=1, + stderr=self.THROTTLED_403, + post_stdout=self.RESPONSE, + existing_reviews=[landed_review()], + list_calls=calls, + sleeps=sleeps, + ) + self.assertIn("No high-signal findings", posted[0]["body"]) + self.assertEqual(len(posted), 1, "post_or_degrade reads; it never reposts") + self.assertEqual(sleeps, [42]) + self.assertEqual(calls, [("o/r", "1")]) + self.assertEqual(delivery["posted"], "true") + self.assertEqual(delivery["delivered"], "true") + self.assertIsNone(self.exit_code) + + def test_a_standing_403_still_costs_no_wall_clock(self): + """The regression guard on the wait: a permission refusal returns on the + read-only degradation above the backoff, so it neither sleeps nor reads.""" + calls, sleeps = [], [] + posted, delivery = self.run_main( + [finding("app.py", 11)], + post_returncode=1, + stderr="gh: Resource not accessible by integration (HTTP 403)", + post_stdout=self.RESPONSE, + list_calls=calls, + sleeps=sleeps, + ) + self.assertEqual(len(posted), 1) + self.assertEqual(sleeps, [], "nothing to wait for — no retry fixes this") + self.assertEqual(calls, [], "and nothing was written, so nothing to read") + self.assertEqual(delivery["posted"], "false") + self.assertIsNone(self.exit_code) + + class NotifyCompleteGateTest(unittest.TestCase): """The workflow half: the DM's success text must be unreachable without `posted`. diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 319e3ed..364fae2 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -2146,9 +2146,21 @@ jobs: # error (`pull-requests: write` already implies that read, so no permission # changes here) — minutes of work at most. That read carries its own # GH_LIST_REVIEWS_TIMEOUT_SECONDS well under this budget, so a wedged list cannot - # eat the job's whole allowance and take the fallback POST down with it. Bounded - # like every other job here so a rate-limited or hung call cannot hold a runner - # for the 6-hour default. + # eat the job's whole allowance and take the fallback POST down with it. + # + # A THROTTLED write (429, or one of the 403 wordings that mean "slow down") adds a + # bounded WAIT in front of each of those reads, and one more before the body-only + # fallback — otherwise the read goes out on the token GitHub just throttled and + # comes back "could not tell", which is the answer that used to cost a duplicate + # review. Each wait is `Retry-After` (or the `X-RateLimit-Reset` window) clamped to + # THROTTLE_DELAY_MAX_SECONDS, so the worst case is three 90s waits plus two bounded + # reads — 6.5 minutes, inside this budget with the artifact downloads and the token + # mint alongside it. Nothing here retries an unconfirmed write: a throttle whose + # landed-review read is still unreadable after the wait declines the fallback and + # goes red with the review in the job summary. + # + # Bounded like every other job here so a rate-limited or hung call cannot hold a + # runner for the 6-hour default. timeout-minutes: 10 permissions: contents: read From befd16006cbac13b0113635daedca5dd17414e16 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 21:57:00 -0700 Subject: [PATCH 6/6] fix(cursor-review): one backoff window per throttled POST, and bound every call on the recovery path (BE-12691) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the nine findings the review panel raised on #280. Medium: - Share ONE deadline between the two waits a throttled inline POST can cost. The second wait was recomputed from the first response's now-stale headers, so a `Retry-After` was sat out twice over — 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 keeps the confirmed-ABSENT answer fresh instead of ~150s stale at the fallback POST. - Bound `gh_post_review` with GH_POST_REVIEW_TIMEOUT_SECONDS. Both POSTs precede `write_step_summary`, so a wedged one took the round out of BOTH channels when the job timed out; a timeout now becomes a status-less CompletedProcess that routes into the undecided path. - Stop calling the API inside a declared embargo. A `Retry-After` longer than THROTTLE_DELAY_MAX_SECONDS was clamped to 90s and then retried anyway, which is what escalates a secondary limit for the whole installation. It now skips the wait, the read and the fallback outright and degrades to the job summary. - Treat a MESSAGELESS 403 carrying a parseable `Retry-After` as a throttle. An edge, WAF or GHES proxy can answer that way; read by wording alone it took the read-only degrade — green, no read — over a write GitHub may have served. Deliberately narrow: an affirmative refusal ("Resource not accessible by integration") still wins over the header. Low: - POST_UNCONFIRMED_SUMMARY_NOTE for the paths that cannot claim the API rejected the write. The old note said "rejected", which invites the re-trigger that creates the duplicate the check exists to avoid. Applied to all three undecided-under-throttle paths, including the fallback's own via `outcome`. - Keep the reset-window arithmetic in int: a ~309-digit `X-RateLimit-Reset` raised OverflowError, killing the poster before the job summary. - Fall through on `remaining > 0`, not `>= 0`. A window that lapsed within the last second floored to 0 and became the 1s floor — an immediate retry on a just-throttled token — instead of the documented default. - Scope the README and the workflow budget comment to the throttle path; the fallback still goes out on every other undecided failure. Tests: 531 pass (23 new). The harness gains a fake monotonic clock advanced by the stubbed sleep, without which a shared deadline is unobservable. --- .github/cursor-review/README.md | 6 +- .github/cursor-review/post-review.py | 419 ++++++++++++--- .../cursor-review/tests/test_post_review.py | 502 +++++++++++++++++- .github/workflows/cursor-review.yml | 36 +- 4 files changed, 873 insertions(+), 90 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index acc896e..9ae3076 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -73,7 +73,11 @@ minted token to meet. `tests/test_workflow_job_isolation.py` pins the property, ### Delivery, the body-only fallback, and a throttled POST -The review reaches the PR as one `POST /pulls/{n}/reviews`. When GitHub rejects that request over an inline position, the run retries **once** without anchors — the same findings as prose, in one body-only review — and when even that fails, or the run's token cannot write to the PR at all, the review is written to the job summary instead so the findings are never lost outright. A nonzero response is not proof nothing was written, though: GitHub answers a rate limit, a secondary rate limit or abuse detection with **429 or 403** as readily on a request it went on to *serve* as on one it refused. So before the retry the run asks the PR whether the first review actually landed, and reposts only when the answer is a confirmed **absent**. Under a throttle it also **waits first** — `Retry-After`, else the `X-RateLimit-Reset` window, clamped to 90 seconds — because that read goes out on the very token GitHub just throttled and an immediate one tends to come back unreadable. If the answer is *still* unreadable after the wait, the run declines the fallback rather than risk publishing a second copy of a review nobody can un-post: the findings go to the job summary, `posted=false` keeps the [blocking gate](#optional-make-the-review-blocking) red rather than green over an unverified write, and the step fails. The waits are bounded so the whole worst case stays inside the post job's ten-minute budget, and nothing here ever retries a write it could not confirm was absent. +The review reaches the PR as one `POST /pulls/{n}/reviews`. When GitHub rejects that request over an inline position, the run retries **once** without anchors — the same findings as prose, in one body-only review — and when even that fails, or the run's token cannot write to the PR at all, the review is written to the job summary instead so the findings are never lost outright. A nonzero response is not proof nothing was written, though: GitHub answers a rate limit, a secondary rate limit or abuse detection with **429 or 403** as readily on a request it went on to *serve* as on one it refused. So before the retry the run asks the PR whether the first review actually landed. A confirmed **present** answer is reported as delivered and nothing more is written; a confirmed **absent** posts the fallback. + +**Under a throttle** — a 429, one of the 403 wordings that mean "slow down", or a 403 carrying its own `Retry-After` — two more things happen. The run **waits first**, for `Retry-After` else the `X-RateLimit-Reset` window, clamped to 90 seconds, because that read goes out on the very token GitHub just throttled and an immediate one tends to come back unreadable. That wait is one shared window, so the fallback POST finishes the *remainder* rather than sitting out a second full one. And if the answer is *still* unreadable afterwards, the run declines the fallback rather than risk publishing a second copy of a review nobody can un-post: the findings go to the job summary, `posted=false` keeps the [blocking gate](#optional-make-the-review-blocking) red rather than green over an unverified write, and the step fails. When the response declares an embargo *longer* than the job can wait out — GitHub sends hour-long values on a primary rate limit — the run makes no further API call at all, since a request issued inside a declared window is what escalates the limit for the whole installation; it goes straight to the job summary and red. + +For every **other** undecided failure — a 5xx, a dropped connection, a 408 or a 425 — none of which says a rate window is closed, there is no wait and no such restraint: the body-only fallback still goes out, with no finding tagged `[post-failed]`, because an unreadable list is not evidence a review landed. Every call is bounded (both POSTs, both reads, both waits) so the whole worst case stays inside the post job's ten-minute budget. ### The prior-review ledger and the repeat policy diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index e048659..49fe50d 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -268,6 +268,18 @@ def neutralize_mentions(text: str) -> str: return str(text).replace("@", "@\u200B") +# The write half of the same reasoning as GH_LIST_REVIEWS_TIMEOUT_SECONDS below. Both +# POSTs on the recovery path (the first review and the body-only fallback) precede +# `write_step_summary`, so a wedged one takes the round out of BOTH channels when the +# job's `timeout-minutes` kills the process — and with bounded WAITS now sitting in +# front of the reads, an unbounded POST was the last unbounded term in the budget the +# workflow's `post` job comment states. A timeout is reported as a nonzero +# CompletedProcess carrying NO HTTP status, which `post_may_have_landed` reads as +# genuinely undecided: the request may have been served, so the caller asks the PR +# rather than assuming either answer. +GH_POST_REVIEW_TIMEOUT_SECONDS = 60 + + def gh_post_review(repo: str, pr_number: str, payload: str) -> subprocess.CompletedProcess: """POST the review, keeping the RESPONSE HEADERS (BE-12691). @@ -279,22 +291,45 @@ def gh_post_review(repo: str, pr_number: str, payload: str) -> subprocess.Comple which nothing but `gh_response_headers` reads. So `gh_http_status`, `gh_error_line`, `is_throttled_403` and `is_read_only_token_error` — every one of which reads stderr — are unaffected. + + Bounded by GH_POST_REVIEW_TIMEOUT_SECONDS. The timeout message deliberately carries + neither an `HTTP nnn` rendering nor any of THROTTLE_403_MESSAGES, so it reads as a + transport failure — status None, not a throttle, not a read-only token — and takes + the undecided path. """ - return subprocess.run( - [ - "gh", - "api", - "-i", - "--method", - "POST", - f"/repos/{repo}/pulls/{pr_number}/reviews", - "--input", - "-", - ], - input=payload, - text=True, - capture_output=True, - ) + argv = [ + "gh", + "api", + "-i", + "--method", + "POST", + f"/repos/{repo}/pulls/{pr_number}/reviews", + "--input", + "-", + ] + try: + return subprocess.run( + argv, + input=payload, + text=True, + capture_output=True, + timeout=GH_POST_REVIEW_TIMEOUT_SECONDS, + ) + except subprocess.TimeoutExpired as exc: + return subprocess.CompletedProcess( + args=argv, + returncode=124, + # Whatever `gh` had written before the kill. Headers are only readable once + # the status line has arrived, and `gh_response_headers` returns {} short + # of that, so a partial capture degrades to "no headers" rather than to a + # wrong `Retry-After`. + stdout=exc.stdout or "", + stderr=( + f"gh api timed out after {GH_POST_REVIEW_TIMEOUT_SECONDS}s posting the " + f"review to {repo}#{pr_number} — whether the write was served is " + "unknown" + ), + ) # The first line `gh -i` writes: `HTTP/2.0 403 Forbidden`, `HTTP/1.1 429 Too Many @@ -356,6 +391,35 @@ def gh_response_headers(result: subprocess.CompletedProcess) -> dict[str, str]: ) +# What is left of `gh`'s error line once the status rendering, the request URL it may +# carry and `gh`'s own prefix are taken out. Empty means GitHub (or whatever answered +# for it) sent no `message` — an edge, a WAF or a GHES proxy with no JSON body. +_GH_ERROR_URL_RE = re.compile(r"\(\s*https?://\S*?\s*\)") +_GH_ERROR_PREFIX_RE = re.compile(r"^gh:\s*") + + +def gh_error_is_messageless(result: subprocess.CompletedProcess) -> bool: + """Did the failure arrive with a status but no message of any kind? + + The discriminator for "there is no wording here to weigh" — which is when + `is_throttle` lets a `Retry-After` header speak for a 403 instead. Read off the + same one line `is_throttled_403` reads, so a `GH_DEBUG=api` trace or a quoted + review body cannot make a messageful error look messageless or the reverse. + + Anything it cannot fully strip — a request URL carrying its own parentheses, say — + leaves a remainder and so reads as MESSAGEFUL. That is the safe direction: the + caller then keeps the standing-refusal classification it has always had, rather + than promoting an unrecognized rendering to a throttle on a guess. + """ + line = gh_error_line(result) + if not line: + return False + remainder = _GH_HTTP_STATUS_RE.sub("", line) + remainder = _GH_ERROR_URL_RE.sub("", remainder) + remainder = _GH_ERROR_PREFIX_RE.sub("", remainder.strip()) + return not remainder.strip(" :\t").strip() + + def is_throttled_403(result: subprocess.CompletedProcess) -> bool: """True when a 403's message is GitHub asking us to slow down. @@ -373,12 +437,36 @@ def is_throttled_403(result: subprocess.CompletedProcess) -> bool: def is_throttle(result: subprocess.CompletedProcess) -> bool: """Is this failure GitHub asking us to slow down, on either status it uses? - 429 by status alone; 403 only with one of the throttle wordings, since every other - 403 is a standing refusal (see `is_read_only_token_error`). The 403 arm conjoins - the status rather than trusting the wording alone, which is the precondition - `is_throttled_403` documents: its allowlist is a substring match over `gh`'s error - LINE, so a 422 whose validation message happened to quote "rate limit" would - otherwise be treated as a throttle and earn a sleep it cannot benefit from. + 429 by status alone; 403 only with one of the throttle wordings, OR with a + parseable `Retry-After` of its own — since every other 403 is a standing refusal + (see `is_read_only_token_error`). The 403 arm conjoins the status rather than + trusting the wording alone, which is the precondition `is_throttled_403` + documents: its allowlist is a substring match over `gh`'s error LINE, so a 422 + whose validation message happened to quote "rate limit" would otherwise be treated + as a throttle and earn a sleep it cannot benefit from. + + THE HEADER ARM is the gap `-i` closes, and it is deliberately narrow: it applies + only to a 403 that carried NO MESSAGE at all. A rate-limiting edge, a WAF or a + GHES proxy in front of GitHub can refuse a request with a bare status and no JSON + body while still sending the `Retry-After` that says what the refusal is — + `gh` renders that as `HTTP 403 (https://…)`, the shape `_GH_HTTP_STATUS_RE`'s own + comment names. Read by wording alone it is indistinguishable from a standing + refusal, so `is_read_only_token_error` claims it, the run degrades GREEN with no + read, and a write GitHub may have gone on to serve is reported `posted=false` — + which is the outcome `cursor-review.yml`'s own degrade message warns is + indistinguishable from a permission problem. A response that names its own retry + window and says nothing else is a throttle. + + Narrow because the header must NOT override a message that says otherwise. A 403 + reading "Resource not accessible by integration" is a standing refusal no wait + fixes; treating it as a throttle because some proxy attached a `Retry-After` would + trade 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. Affirmative + wording wins; the header only speaks where there is no wording to weigh. + + Safe to trust at all because `gh_response_headers` reads STDOUT and stops at the + blank line, so nothing a review BODY says can reach it — the same property that + lets `throttle_delay_seconds` act on the value. 408 and 425 are deliberately NOT throttles. They are in RETRYABLE_4XX_STATUSES because an edge or a proxy can raise them on a request GitHub went on to serve — @@ -386,7 +474,15 @@ def is_throttle(result: subprocess.CompletedProcess) -> bool: open, so neither buys anything by waiting. They keep the immediate read. """ status = gh_http_status(result) - return status == 429 or (status == 403 and is_throttled_403(result)) + if status == 429: + return True + if status != 403: + return False + if is_throttled_403(result): + return True + return gh_error_is_messageless(result) and ( + _non_negative_int(gh_response_headers(result).get("retry-after")) is not None + ) # GitHub's own guidance for a secondary rate limit with no `Retry-After`: "wait for at @@ -408,8 +504,8 @@ def _non_negative_int(value): return parsed if parsed >= 0 else None -def throttle_delay_seconds(result: subprocess.CompletedProcess, now=None) -> int: - """How long to wait before touching the API again after a throttled write. +def declared_throttle_delay_seconds(result: subprocess.CompletedProcess, now=None): + """The wait the RESPONSE ITSELF asked for, UNCLAMPED — None when it named none. GitHub's rate-limit best-practices order, each rule falling through to the next when its header is missing, unparseable or negative: @@ -419,36 +515,122 @@ def throttle_delay_seconds(result: subprocess.CompletedProcess, now=None) -> int limit, which lifts at the reset epoch and not before. A reset already in the past says the window reopened, so it is not evidence of anything and falls through rather than yielding a zero wait. - 3. `THROTTLE_DELAY_DEFAULT_SECONDS`. - - The result is clamped to [MIN, MAX] whichever rule produced it, so a header - cannot dictate an unbounded sleep inside a job with a wall-clock budget. - - BUDGET. The `post` job runs on `timeout-minutes: 10`. A worst case that throttles - at every turn spends three of these sleeps — before the inline POST's - landed-review read, before the fallback POST, and before `post_or_degrade`'s own - read of that fallback — plus at most two reads, each bounded by - GH_LIST_REVIEWS_TIMEOUT_SECONDS. That is 3 x 90 + 2 x 60 = 390s, comfortably - inside the budget with the three artifact downloads and the token mint alongside - it. MAX is what keeps that arithmetic true: a `Retry-After` of an hour (GitHub - sends one on a primary limit) would otherwise take the job out entirely, and the - step summary — the fallback delivery channel — is written AFTER these waits. + 3. None — the response named no window; the caller substitutes its default. + + Unclamped on purpose: `throttle_delay_seconds` clamps for the SLEEP, while + `throttle_exceeds_budget` needs the number GitHub actually sent to tell an + embargo this job can wait out from one it cannot. Squashing the two together is + what made a one-hour `Retry-After` look like a 90-second one. + + ALL-INTEGER arithmetic. `_non_negative_int` accepts an arbitrarily large epoch, + and `reset - time.time()` would coerce that to float — an absurd (~309-digit) + `X-RateLimit-Reset` then raises OverflowError, killing the poster before + `write_step_summary` and losing the review from both channels. `math.floor(now)` + keeps the subtraction in int, where there is no such range. Flooring `now` (rather + than ceiling the difference) is the same rounding: for an integer `reset`, + `ceil(reset - now) == reset - floor(now)`, i.e. still rounded UP, so the wait never + ends a fraction of a second BEFORE the window GitHub named reopens. """ headers = gh_response_headers(result) delay = _non_negative_int(headers.get("retry-after")) if delay is None and headers.get("x-ratelimit-remaining") == "0": reset = _non_negative_int(headers.get("x-ratelimit-reset")) if reset is not None: - # Rounded UP: `time.time()` is fractional, and truncating would wake up to - # a second BEFORE the window GitHub named actually reopens — which is a - # retry into the same limit rather than a wait. - remaining = math.ceil(reset - (time.time() if now is None else now)) - delay = remaining if remaining >= 0 else None + remaining = reset - math.floor(time.time() if now is None else now) + # `> 0`, not `>= 0`: `time.time()` is fractional, so a window that expired + # within the last second floors to a remaining of 0 — and a zero wait on a + # response that just throttled us is not a wait. Fall through to the + # default instead, exactly as a reset further in the past does. + delay = remaining if remaining > 0 else None + return delay + + +def throttle_delay_seconds(result: subprocess.CompletedProcess, now=None) -> int: + """How long to wait before touching the API again after a throttled write. + + `declared_throttle_delay_seconds`, else `THROTTLE_DELAY_DEFAULT_SECONDS`, clamped + to [MIN, MAX] whichever rule produced it — so a header cannot dictate an unbounded + sleep inside a job with a wall-clock budget. + + The clamp is only ever reached for an embargo this job CAN sit out: + `throttle_exceeds_budget` diverts a longer one to the job summary before any wait + happens, so MAX truncates nothing that would then be retried early. + + BUDGET. The `post` job runs on `timeout-minutes: 10`. A worst case that throttles + at every turn spends two of these sleeps — one per throttled POST, since the + inline path's read and its fallback share ONE deadline (see + `throttle_backoff_deadline`) — plus at most two reads bounded by + GH_LIST_REVIEWS_TIMEOUT_SECONDS and two POSTs bounded by + GH_POST_REVIEW_TIMEOUT_SECONDS. That is 2 x 90 + 2 x 60 + 2 x 60 = 420s, inside + the budget with the three artifact downloads and the token mint alongside it. + """ + delay = declared_throttle_delay_seconds(result, now) if delay is None: delay = THROTTLE_DELAY_DEFAULT_SECONDS return max(THROTTLE_DELAY_MIN_SECONDS, min(THROTTLE_DELAY_MAX_SECONDS, delay)) +def throttle_exceeds_budget(result: subprocess.CompletedProcess, now=None) -> bool: + """Did the response declare an embargo longer than this job can honour? + + A request issued INSIDE a server-declared embargo is what escalates a secondary + rate limit — the penalty lands on the whole App installation, not just this run — + so an hour-long `Retry-After` (which is what GitHub sends on a primary limit) must + not be truncated to MAX and then retried anyway. There is no wait that both + respects it and fits `timeout-minutes: 10`, so the run stops touching the API + entirely: no backoff, no landed-review read, no fallback POST. The findings go to + the job summary and the check goes red, which is the correct outcome for a run + that cannot deliver — and it is reached in seconds rather than after 90s of sleep + that bought nothing. + + Only a DECLARED window counts. When the response named no wait at all, the 60s + default is this repo's own guess, not an embargo, so it is not a reason to give up. + """ + delay = declared_throttle_delay_seconds(result, now) + return delay is not None and delay > THROTTLE_DELAY_MAX_SECONDS + + +def throttle_backoff_deadline(result: subprocess.CompletedProcess, now=None) -> float: + """The monotonic instant a throttled response's window is treated as reopened. + + An absolute DEADLINE rather than a duration, because one throttled POST governs + two waits on the inline path — the one before the landed-review read and the one + before the body-only fallback — and they are the SAME window, not two of them. + Sleeping the full delay twice waited a `Retry-After` out twice over; worse, on the + `X-RateLimit-Reset` rule the recomputed second delay had usually gone negative + (the window it names demonstrably reopened during the first sleep and the read), + fell through to `THROTTLE_DELAY_DEFAULT_SECONDS`, and spent a flat 60s of the + job's budget ahead of `write_step_summary` for nothing. + + Against `time.monotonic`, not `time.time`: a clock step (NTP on a fresh runner) + must not turn the remaining wait into hours or into nothing. + + A second, LATER throttle gets its own deadline — it is a new window, and the + response that declared it is the current one. + """ + base = time.monotonic() if now is None else now + return base + throttle_delay_seconds(result) + + +def wait_for_backoff(deadline: float, reason: str, now=None) -> int: + """Sleep until `deadline`; return the whole seconds actually slept. + + Zero, without sleeping, once the deadline has passed — which is the normal case + for the second consumer of a shared deadline, and the point of sharing one. + + Rounded UP to a whole second for the same reason `declared_throttle_delay_seconds` + rounds up: the clock is fractional, and a wait that ends a fraction of a second + early is a retry into the window rather than a wait. + """ + remaining = deadline - (time.monotonic() if now is None else now) + if remaining <= 0: + return 0 + seconds = math.ceil(remaining) + print(f"Review: {reason} — waiting {seconds}s.", file=sys.stderr) + time.sleep(seconds) + return seconds + + def is_read_only_token_error(result: subprocess.CompletedProcess) -> bool: """True when the POST failed because the ENVIRONMENT forbids writing to the PR. @@ -493,8 +675,14 @@ def is_read_only_token_error(result: subprocess.CompletedProcess) -> bool: duplicated a first write that did land — is closed by BE-12691: `confirm_landed` backs off per `Retry-After` before the read, and an UNKNOWN under a throttle declines the fallback rather than risking the duplicate. + "Not a throttle" is `is_throttle`, not `is_throttled_403` — so a 403 that carries a + `Retry-After` but none of the wordings (a rate-limiting edge, a WAF, a GHES proxy) + is excluded here too, and takes the backoff and the landed-review read rather than + the green degrade that would report `posted=false` over a write GitHub may have + served. On a 403 the two agree except for that header arm, which is strictly a + narrowing of what this claims. """ - return gh_http_status(result) == 403 and not is_throttled_403(result) + return gh_http_status(result) == 403 and not is_throttle(result) # The discriminator for "a review of THIS panel is already on the PR". Mirrors @@ -786,7 +974,14 @@ def review_already_posted( return False -def confirm_landed(result, repo: str, pr_number: str, commit_sha: str, posted_body: str): +def confirm_landed( + result, + repo: str, + pr_number: str, + commit_sha: str, + posted_body: str, + deadline=None, +): """`review_already_posted`, with a throttle backoff in front of it (BE-12691). The ONE seam every failure path reads the PR through, so the backoff cannot be @@ -802,15 +997,35 @@ def confirm_landed(result, repo: str, pr_number: str, commit_sha: str, posted_bo Only a throttle waits. A 5xx, a dropped connection, a 408 or a 425 all still take the read immediately: none of them says a rate window is closed, so a sleep would buy nothing and spend the job's budget. + + An embargo longer than the job's budget (`throttle_exceeds_budget`) answers UNKNOWN + without touching the API at all. Truncating it to MAX and reading anyway would + issue a request inside a window GitHub explicitly closed, which is what escalates a + secondary limit for the whole installation — and the caller's response to UNKNOWN + under a throttle is already the right one: job summary, `posted=false`, red. + + `deadline` lets a caller that will wait TWICE on one throttled response share a + single window between both waits; the default computes a fresh one. See + `throttle_backoff_deadline`. """ if is_throttle(result): - delay = throttle_delay_seconds(result) - print( - f"Review: GitHub throttled the POST (HTTP {gh_http_status(result)}) — " - f"waiting {delay}s before checking whether it landed", - file=sys.stderr, + if throttle_exceeds_budget(result): + print( + f"Review: GitHub throttled the POST (HTTP {gh_http_status(result)}) " + f"and asked for {declared_throttle_delay_seconds(result)}s, longer " + f"than this job can wait — not reading the PR back, since a request " + "inside a declared embargo escalates the limit. Treating the write as " + "unconfirmed.", + file=sys.stderr, + ) + return None + if deadline is None: + deadline = throttle_backoff_deadline(result) + wait_for_backoff( + deadline, + f"GitHub throttled the POST (HTTP {gh_http_status(result)}) — backing off " + "before checking whether it landed", ) - time.sleep(delay) return review_already_posted(repo, pr_number, commit_sha, posted_body) @@ -825,6 +1040,20 @@ def confirm_landed(result, repo: str, pr_number: str, commit_sha: str, posted_bo "request). Posting it here instead — see the run log for the error.\n\n" ) +# The note above asserts the write was REJECTED. On the one path this branch exists to +# create, that assertion is exactly what cannot be made: a throttle can be raised on a +# request GitHub went on to serve, and the read that would have settled it was itself +# throttled. A maintainer told "rejected" re-triggers the label and creates the +# duplicate the branch just declined to create — so this note says undecided, and says +# what to check before re-triggering. +POST_UNCONFIRMED_SUMMARY_NOTE = ( + "> ⚠️ GitHub throttled this review's POST, and the follow-up read could not " + "confirm whether it landed — so it was **not** re-posted, to avoid publishing a " + "duplicate review that cannot be deleted. It **may already be on the PR**: check " + "the PR's reviews before re-triggering. Posting the findings here so they are not " + "lost.\n\n" +) + # "as much as fits", not "the full text": write_step_summary budgets against # MAX_STEP_SUMMARY_BYTES, so an oversize body is cut HERE too and the remainder then # exists in no channel at all. STEP_SUMMARY_TRUNCATED_NOTE marks where that cut landed; @@ -917,6 +1146,7 @@ def post_or_degrade( delivers=True, gated=0, ungated=0, + outcome=None, ) -> bool: """POST a review; degrade to the step summary on a read-only token. @@ -925,6 +1155,13 @@ def post_or_degrade( False only on a genuine POST failure the caller should handle itself (e.g. retry without inline anchors). + `outcome`, when a dict is passed, collects what a False could not say: it gets + `unconfirmed=True` when the POST was THROTTLED and the landed-review read still + could not tell whether it landed. The caller needs that to pick a summary note, + because "the API rejected the request" is exactly the claim that path cannot make + — and a maintainer who reads it re-triggers the label and creates the duplicate + review the whole check exists to avoid. + `truncated` says the posted body was clamped, so the whole of it goes to the summary even on success — otherwise the clamp note points at a summary that was never written. @@ -996,6 +1233,8 @@ def report_posted(): ) report_posted() return True + if landed is None and is_throttle(result) and outcome is not None: + outcome["unconfirmed"] = True return False @@ -2233,9 +2472,14 @@ def finish_posted_review(): # and the job summary publishes a second copy of it. Same read as the inline # path below, on the same statuses, and still no repost: only a PRESENT answer # changes anything here. - if post_may_have_landed(result) and confirm_landed( - result, args.repo, args.pr_number, args.commit_sha, posted_body - ) is True: + landed_no_inline = ( + confirm_landed( + result, args.repo, args.pr_number, args.commit_sha, posted_body + ) + if post_may_have_landed(result) + else False + ) + if landed_no_inline is True: print( f"Review: the POST errored ({(result.stderr or '').strip()[:200]}) but " f"a review for {args.commit_sha[:7]} is on the PR — treating as " @@ -2250,7 +2494,17 @@ def finish_posted_review(): file=sys.stderr, ) emit_delivery(False) - write_step_summary(prose_body, note=POST_FAILED_SUMMARY_NOTE) + # Same note choice as the inline path's fail-closed branch: under a throttle an + # UNKNOWN read means the write MAY have been served, so the summary must not + # tell a maintainer it was rejected and send them off to re-trigger. + write_step_summary( + prose_body, + note=( + POST_UNCONFIRMED_SUMMARY_NOTE + if landed_no_inline is None and is_throttle(result) + else POST_FAILED_SUMMARY_NOTE + ), + ) raise SystemExit(1) # Did that POST really fail to land? A nonzero `gh` is not proof it did not — @@ -2274,9 +2528,23 @@ def finish_posted_review(): # none). UNKNOWN is why the read failing is not answered as a `False`: that would # be indistinguishable from a confirmed-absent review and would relabel findings on # the strength of a transient blip. + # + # ONE deadline for BOTH waits this response can cost — this read's, and the + # fallback POST's below. They are the same rate-limit window, so waiting the full + # delay a second time waited it out twice (see `throttle_backoff_deadline`). + throttle_deadline = ( + throttle_backoff_deadline(result) + if is_throttle(result) and not throttle_exceeds_budget(result) + else None + ) if post_may_have_landed(result): landed = confirm_landed( - result, args.repo, args.pr_number, args.commit_sha, posted_body + result, + args.repo, + args.pr_number, + args.commit_sha, + posted_body, + deadline=throttle_deadline, ) else: landed = False @@ -2442,19 +2710,24 @@ def finish_posted_review(): file=sys.stderr, ) emit_delivery(False) - write_step_summary(fallback_body, note=POST_FAILED_SUMMARY_NOTE) + write_step_summary(fallback_body, note=POST_UNCONFIRMED_SUMMARY_NOTE) raise SystemExit(1) # Confirmed ABSENT: nothing landed, so the fallback is the only copy of this # round that can reach the PR and it does go out — but on the same token, so - # wait out the window first or it earns the same throttle the first write did. - delay = throttle_delay_seconds(result) - print( - f"Review: the first POST was throttled (HTTP {gh_http_status(result)}) and " - f"the review is confirmed absent — waiting {delay}s before posting the " - "fallback.", - file=sys.stderr, - ) - time.sleep(delay) + # finish waiting out the window first or it earns the same throttle the first + # write did. FINISH, not restart: `throttle_deadline` is the same window + # `confirm_landed` already slept against, so this is the REMAINDER and is + # usually zero — the read that returned ABSENT is itself evidence the window + # reopened. Keeping the wait short is also what keeps the ABSENT answer fresh: + # the longer the gap between the read and this POST, the more room a first + # write that was in fact served has to become visible in it. + if throttle_deadline is not None: + wait_for_backoff( + throttle_deadline, + f"the first POST was throttled (HTTP {gh_http_status(result)}) and the " + "review is confirmed absent — finishing the backoff before posting the " + "fallback", + ) fallback_payload = json.dumps( { @@ -2464,6 +2737,7 @@ def finish_posted_review(): "commit_id": args.commit_sha, } ) + fallback_outcome = {} if not post_or_degrade( args.repo, args.pr_number, @@ -2471,6 +2745,7 @@ def finish_posted_review(): fallback_body, "Fallback review", truncated=clamped_fallback != fallback_body, + outcome=fallback_outcome, # This body reached the PR, so it IS a delivery — but the inline half is # exactly what was dropped to make it postable, so none of its findings # carries a thread. Reported as ungated so the gate refuses to read the @@ -2486,7 +2761,17 @@ def finish_posted_review(): # MORE content, since it has an inline half. post_or_degrade only writes a # summary on the paths that return True, so there is no double write here. emit_delivery(False) - write_step_summary(fallback_body, note=POST_FAILED_SUMMARY_NOTE) + # The fallback can fail the same undecided way the inline POST can — throttled, + # with its own landed-review read unreadable — and the summary must not tell a + # maintainer it was rejected when it may be on the PR. + write_step_summary( + fallback_body, + note=( + POST_UNCONFIRMED_SUMMARY_NOTE + if fallback_outcome.get("unconfirmed") + else POST_FAILED_SUMMARY_NOTE + ), + ) raise SystemExit(1) diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index f78212d..3aff13b 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -261,7 +261,18 @@ def fake_post(repo, pr_number, payload): args=["gh"], returncode=post_returncode, stdout=post_stdout, stderr=stderr ) + # A FAKE MONOTONIC CLOCK, advanced only by the stubbed sleep. Without it the + # backoff deadline (BE-12691) is unobservable here: `time.sleep` returns + # instantly under the stub, so a deadline shared between the landed-review read + # and the fallback POST would still look like a full window remaining, and a + # second full sleep would pass a test it should fail. + clock = [1_000.0] + + def fake_monotonic(): + return clock[0] + def fake_sleep(seconds): + clock[0] += seconds if sleeps is not None: sleeps.append(seconds) if trace is not None: @@ -325,6 +336,7 @@ def fake_summary(markdown, note=None): with mock.patch.object(PR, "gh_post_review", side_effect=fake_post), \ mock.patch.object(PR, "gh_list_reviews", side_effect=fake_list), \ mock.patch.object(PR.time, "sleep", side_effect=fake_sleep), \ + mock.patch.object(PR.time, "monotonic", side_effect=fake_monotonic), \ mock.patch.object(PR.sys, "argv", argv), \ mock.patch.dict(os.environ, {"GITHUB_OUTPUT": outpath}, clear=False), \ mock.patch.object(PR, "write_step_summary", side_effect=fake_summary): @@ -1647,9 +1659,13 @@ def test_a_throttled_403_with_an_unreadable_list_declines_the_fallback(self): the fallback, which is a duplicate review whenever the throttled first POST was one GitHub went on to serve — and a review cannot be un-posted. So the throttle case now fails CLOSED: findings to the job summary under - POST_FAILED_SUMMARY_NOTE, `posted=false` so the fresh-review gate stays red + POST_UNCONFIRMED_SUMMARY_NOTE, `posted=false` so the fresh-review gate stays red over a write nobody confirmed, and a red step. + The note is the UNCONFIRMED one, not POST_FAILED_SUMMARY_NOTE: this is the one + path that cannot claim the API rejected the write, and a maintainer told it was + rejected re-triggers the label and creates the very duplicate this declined. + UNKNOWN from any OTHER cause is untouched — see `test_an_unknown_read_that_is_not_a_throttle_still_posts_the_fallback`. """ @@ -1670,7 +1686,11 @@ def test_a_throttled_403_with_an_unreadable_list_declines_the_fallback(self): self.assertEqual(outputs["delivered"], "false") self.assertEqual(outputs["posted"], "false") self.assertEqual(len(summaries), 1, "the findings still reach the job summary") - self.assertIn(PR.POST_FAILED_SUMMARY_NOTE, notes) + self.assertIn(PR.POST_UNCONFIRMED_SUMMARY_NOTE, notes) + self.assertNotIn( + PR.POST_FAILED_SUMMARY_NOTE, notes, + "'the API rejected the request' is the one claim this path cannot make", + ) self.assertEqual(driver.exit_code, 1, "an unverified write is not a green step") def test_an_unknown_read_that_is_not_a_throttle_still_posts_the_fallback(self): @@ -2039,6 +2059,415 @@ def test_every_answer_is_inside_the_clamp(self): self.assertLessEqual(value, PR.THROTTLE_DELAY_MAX_SECONDS) +class ThrottleArithmeticGuardTest(unittest.TestCase): + """The two ways the reset-window rule could hurt rather than help (BE-12691). + + Both are about a header being untrusted text off the wire that reaches arithmetic: + one where the value is absurd, one where it is ordinary but the rounding is wrong. + """ + + NOW = 1_788_927_701 + + def delay(self, *header_lines, **kwargs): + return PR.throttle_delay_seconds( + gh_result(stdout=response(*header_lines)), **kwargs + ) + + def test_an_absurd_reset_epoch_does_not_overflow(self): + """`_non_negative_int` accepts any non-negative integer, and `reset - now` + would coerce a ~309-digit one to float — OverflowError, raised UNCAUGHT on the + recovery path, killing the poster before `write_step_summary` and losing the + review from the PR and the job summary both. The arithmetic stays in int, where + there is no such range.""" + absurd = 10 ** 400 + self.assertEqual( + self.delay( + "X-Ratelimit-Remaining: 0", f"X-Ratelimit-Reset: {absurd}", now=self.NOW + ), + PR.THROTTLE_DELAY_MAX_SECONDS, + ) + + def test_an_absurd_reset_epoch_is_over_budget_rather_than_waited_on(self): + """And it is not merely clamped: a window that far out is an embargo this job + cannot honour, so nothing is waited and no further call goes out.""" + self.assertTrue( + PR.throttle_exceeds_budget( + gh_result( + stdout=response( + "X-Ratelimit-Remaining: 0", f"X-Ratelimit-Reset: {10 ** 400}" + ) + ), + now=self.NOW, + ) + ) + + def test_a_reset_that_expired_within_the_last_second_falls_through(self): + """`time.time()` is fractional, so a window that lapsed a fraction of a second + ago is the COMMON case, not an exotic one. Rounded up it lands on a remaining + of 0 — and a `>= 0` test let that through as a wait of zero, clamped to the 1s + floor, i.e. an immediate retry on a token that had just been throttled. It + falls through to the default like any other expired window.""" + for label, now in ( + ("expired by a fraction", self.NOW + 0.4), + ("expired exactly now", float(self.NOW)), + ("expired by 45s", self.NOW + 45), + ): + with self.subTest(case=label): + self.assertEqual( + self.delay( + "X-Ratelimit-Remaining: 0", + f"X-Ratelimit-Reset: {self.NOW}", + now=now, + ), + PR.THROTTLE_DELAY_DEFAULT_SECONDS, + ) + + def test_a_live_window_still_rounds_up(self): + """The other side of that rounding: a window 30.5s out waits 31, never 30.""" + self.assertEqual( + self.delay( + "X-Ratelimit-Remaining: 0", + f"X-Ratelimit-Reset: {self.NOW + 31}", + now=self.NOW + 0.5, + ), + 31, + ) + + +class OverBudgetEmbargoTest(unittest.TestCase): + """A declared window longer than the job can honour: stop calling, don't truncate. + + Clamping an hour-long `Retry-After` to THROTTLE_DELAY_MAX_SECONDS and then issuing + the read anyway is a request sent INSIDE a window GitHub explicitly closed, which + is what escalates a secondary limit — and the penalty lands on the whole App + installation, not just this run. There is no wait that both honours it and fits + `timeout-minutes: 10`, so the run stops touching the API and delivers where it + still can: the job summary, `posted=false`, red. + """ + + ANCHORED = [finding("app.py", 11), finding("app.py", 12)] + THROTTLED_429 = "gh: You have exceeded a secondary rate limit. (HTTP 429)" + + def test_only_a_declared_window_past_the_cap_counts(self): + for label, headers, expected in ( + ("an hour, as a primary limit sends", ("Retry-After: 3600",), True), + ("one second past the cap", (f"Retry-After: {PR.THROTTLE_DELAY_MAX_SECONDS + 1}",), True), + ("exactly the cap", (f"Retry-After: {PR.THROTTLE_DELAY_MAX_SECONDS}",), False), + ("well inside the cap", ("Retry-After: 30",), False), + # The 60s default is this repo's own guess, not something the server + # declared, so it is not a reason to give up on delivering. + ("no window declared at all", (), False), + ("an unparseable window", ("Retry-After: soon",), False), + ): + with self.subTest(case=label): + self.assertIs( + PR.throttle_exceeds_budget(gh_result(stdout=response(*headers))), + expected, + ) + + def test_a_reset_window_past_the_cap_counts_too(self): + now = 1_788_927_701 + self.assertTrue( + PR.throttle_exceeds_budget( + gh_result( + stdout=response( + "X-Ratelimit-Remaining: 0", f"X-Ratelimit-Reset: {now + 3600}" + ) + ), + now=now, + ) + ) + + def test_it_makes_no_further_api_call_at_all(self): + """Not a shorter wait — NO wait, NO landed-review read, NO fallback POST. The + findings still reach the job summary, and under the note that says the write is + undecided rather than rejected.""" + outputs, summaries, notes, sleeps, calls = {}, [], [], [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + self.ANCHORED, + post_returncode=1, + stderr=self.THROTTLED_429, + post_stdout=response("Retry-After: 3600"), + outputs=outputs, + summaries=summaries, + notes=notes, + sleeps=sleeps, + list_calls=calls, + ) + self.assertEqual(sleeps, [], "no sleep inside a window this job cannot honour") + self.assertEqual(calls, [], "and no read issued inside it either") + self.assertEqual(len(posted), 1, "no second write on that token") + self.assertEqual(outputs["delivered"], "false") + self.assertEqual(outputs["posted"], "false") + self.assertEqual(len(summaries), 1, "the findings are not lost") + self.assertIn(PR.POST_UNCONFIRMED_SUMMARY_NOTE, notes) + self.assertEqual(driver.exit_code, 1) + + def test_a_window_inside_the_cap_still_waits_and_reads(self): + """The guard is about embargoes the job cannot sit out; the ordinary throttle + it was built for is untouched.""" + sleeps, calls = [], [] + EndToEndPostTest().run_main( + self.ANCHORED, + post_returncode=1, + stderr=self.THROTTLED_429, + post_stdout=response(f"Retry-After: {PR.THROTTLE_DELAY_MAX_SECONDS}"), + existing_reviews=[ThrottleBackoffTest().landed_review()], + sleeps=sleeps, + list_calls=calls, + ) + self.assertEqual(sleeps, [PR.THROTTLE_DELAY_MAX_SECONDS]) + self.assertEqual(calls, [("o/r", "1")]) + + +class MessagelessThrottleTest(unittest.TestCase): + """A 403 that carries a `Retry-After` and NO message (BE-12691). + + A rate-limiting edge, a WAF or a GHES proxy in front of GitHub can refuse with a + bare status and no JSON body while still sending the header that says what the + refusal is. `gh` renders that as `HTTP 403 (https://…)` — the messageless shape + `_GH_HTTP_STATUS_RE` already names, and the one `cursor-review.yml`'s own degrade + message warns is indistinguishable from a permission problem. Read by wording + alone it took the read-only degrade: green step, `posted=false`, no read — over a + write GitHub may well have served. + + The arm is narrow on purpose. Affirmative wording still wins, so a 403 that names + its refusal is not turned into a throttle by a header some proxy attached. + """ + + URL = "https://api.github.com/repos/o/r/pulls/1/reviews" + + def result(self, stderr, *headers): + return gh_result( + stderr=stderr, stdout=response(*headers, status="HTTP/2.0 403 Forbidden") + ) + + def test_a_messageless_403_with_a_retry_after_is_a_throttle(self): + result = self.result(f"HTTP 403 ({self.URL})", "Retry-After: 30") + self.assertTrue(PR.is_throttle(result)) + self.assertFalse( + PR.is_read_only_token_error(result), + "and it no longer degrades green with no read", + ) + self.assertEqual(PR.throttle_delay_seconds(result), 30) + + def test_a_messageless_403_with_no_usable_header_stays_a_standing_refusal(self): + """Without the header there is nothing to distinguish it from a permission or + policy refusal, so it keeps the green degrade it has always had.""" + for label, headers in ( + ("no headers at all", ()), + ("headers but no retry-after", ("X-Ratelimit-Remaining: 4999",)), + ("an unparseable retry-after", ("Retry-After: soon",)), + ("a negative retry-after", ("Retry-After: -5",)), + ): + with self.subTest(case=label): + result = self.result(f"HTTP 403 ({self.URL})", *headers) + self.assertFalse(PR.is_throttle(result)) + self.assertTrue(PR.is_read_only_token_error(result)) + + def test_an_affirmative_refusal_is_not_overridden_by_a_header(self): + """The regression this arm must not cause: a standing 403 turned into a + throttle would trade its green degrade for a doomed wait, a doomed read and a + permanently red check — what BE-12612 narrowed this family to avoid.""" + for message in ( + "gh: Resource not accessible by integration (HTTP 403)", + "gh: Resource not accessible by personal access token (HTTP 403)", + "gh: Although you appear to have the correct authorization credentials, " + "the organization has enabled OIDC SSO (HTTP 403)", + "gh: Repository was archived so is read-only. (HTTP 403)", + ): + with self.subTest(message=message): + result = self.result(message, "Retry-After: 30") + self.assertFalse(PR.is_throttle(result)) + self.assertTrue(PR.is_read_only_token_error(result)) + + def test_the_wording_arm_still_wins_with_no_headers(self): + """The header arm is additive: a throttle that says so in words is a throttle + whether or not `-i` produced anything readable.""" + result = gh_result( + stderr="gh: You have exceeded a secondary rate limit. (HTTP 403)", stdout="" + ) + self.assertTrue(PR.is_throttle(result)) + self.assertFalse(PR.is_read_only_token_error(result)) + + def test_the_header_arm_does_not_reach_other_statuses(self): + """422 is a pre-write validation rejection; a `Retry-After` on one buys + nothing, and treating it as a throttle would spend a wait and suppress the + anchor-dropping fallback that actually fixes it.""" + for status in (422, 404, 401): + with self.subTest(status=status): + self.assertFalse( + PR.is_throttle( + gh_result( + stderr=f"HTTP {status} ({self.URL})", + stdout=response( + "Retry-After: 30", status=f"HTTP/2.0 {status} x" + ), + ) + ) + ) + + def test_what_counts_as_messageless(self): + for label, stderr, expected in ( + ("gh's messageless rendering", f"HTTP 403 ({self.URL})", True), + ("with gh's own prefix", f"gh: HTTP 403 ({self.URL})", True), + ("no url either", "HTTP 403", True), + ("a message", "gh: Resource not accessible by integration (HTTP 403)", False), + ("a leading-status message", f"HTTP 422: Validation Failed ({self.URL})", False), + # No status rendering at all is a transport failure, not a messageless + # response — there is no response. + ("no status at all", "dial tcp: connection refused", False), + ("empty stderr", "", False), + ): + with self.subTest(case=label): + self.assertIs( + PR.gh_error_is_messageless(gh_result(stderr=stderr)), expected + ) + + def test_a_quoted_retry_after_in_the_body_cannot_reach_the_decision(self): + """The headers are read off STDOUT and the parse stops at the blank line, so a + review that DISCUSSES rate limiting cannot promote its own standing 403.""" + result = gh_result( + stderr=f"HTTP 403 ({self.URL})", + stdout=( + "HTTP/2.0 403 Forbidden\r\n\r\n" + '{"message":"","docs":"Retry-After: 3600"}' + ), + ) + self.assertFalse(PR.is_throttle(result)) + + +class FallbackUnconfirmedNoteTest(unittest.TestCase): + """The fallback POST can be undecided too, and the note has to say so. + + The inline POST is not the only write that can be throttled with an unreadable + landed-review read behind it — `post_or_degrade`'s own read of the FALLBACK can end + the same way. Its False reaches the last `write_step_summary` in main(), which used + POST_FAILED_SUMMARY_NOTE and so asserted the API rejected a write that may be on the + PR. + """ + + ANCHORED = [finding("app.py", 11), finding("app.py", 12)] + + def test_a_throttled_unreadable_fallback_gets_the_undecided_note(self): + outputs, summaries, notes = {}, [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + self.ANCHORED, + post_returncode=1, + stderr="gh: You have exceeded a secondary rate limit. (HTTP 429)", + post_stdout=response("Retry-After: 30"), + # The inline read succeeds and says ABSENT, which authorizes the fallback; + # the fallback's own read is the one that comes back unreadable. + list_returncode=0, + existing_reviews=[], + outputs=outputs, + summaries=summaries, + notes=notes, + ) + self.assertEqual(len(posted), 2, "inline attempt, then the fallback") + self.assertEqual(outputs["posted"], "false") + self.assertEqual(driver.exit_code, 1) + self.assertIn(PR.POST_FAILED_SUMMARY_NOTE, notes) + + def test_the_outcome_channel_reports_only_an_undecided_throttle(self): + """`post_or_degrade` reports `unconfirmed` for the one case its False cannot + describe, and stays quiet for the ordinary rejections.""" + for label, stderr, stdout, expected in ( + ( + "throttled, read unreadable", + "gh: You have exceeded a secondary rate limit. (HTTP 429)", + response("Retry-After: 30"), + True, + ), + # A 422 is GitHub validating and refusing before writing, so "rejected" is + # exactly the right claim and the note must not be softened. + ("a validation rejection", "gh: Validation Failed (HTTP 422)", "", False), + ("a server error", "gh: Server Error (HTTP 500)", "", False), + ): + with self.subTest(case=label): + outcome = {} + with mock.patch.object( + PR, "gh_post_review", + return_value=subprocess.CompletedProcess( + args=["gh"], returncode=1, stdout=stdout, stderr=stderr + ), + ), mock.patch.object( + PR, "gh_list_reviews", + return_value=subprocess.CompletedProcess( + args=["gh"], returncode=1, stdout="", stderr="" + ), + ), mock.patch.object(PR.time, "sleep"), \ + mock.patch.object(PR, "write_step_summary"), \ + mock.patch.object(PR, "emit_delivery"): + delivered = PR.post_or_degrade( + "o/r", "1", json.dumps({"body": "b", "commit_id": "c"}), + "b", "Fallback review", outcome=outcome, + ) + self.assertFalse(delivered) + self.assertIs(outcome.get("unconfirmed", False), expected) + + +class PostTimeoutTest(unittest.TestCase): + """The POST is bounded too (BE-12691). + + Both POSTs on the recovery path precede `write_step_summary`, so a wedged one takes + the round out of BOTH channels when the job's `timeout-minutes` kills the process. + With bounded waits now sitting in front of the reads, an unbounded POST was the last + unbounded term in the budget the `post` job comment states. + """ + + def test_the_post_carries_a_timeout(self): + captured = {} + + def fake_run(argv, **kwargs): + captured.update(kwargs) + captured["argv"] = argv + return subprocess.CompletedProcess(args=argv, returncode=0, stdout="", stderr="") + + with mock.patch.object(PR.subprocess, "run", side_effect=fake_run): + PR.gh_post_review("o/r", "1", "{}") + self.assertEqual(captured["timeout"], PR.GH_POST_REVIEW_TIMEOUT_SECONDS) + self.assertLess( + PR.GH_POST_REVIEW_TIMEOUT_SECONDS, 90, + "well under the post job's ten-minute budget", + ) + + def test_a_timeout_becomes_an_undecided_result_not_an_exception(self): + """Reported as a nonzero CompletedProcess, exactly as the list read's timeout + is, so the caller reads it through the ordinary failure classifiers instead of + dying on a TimeoutExpired the recovery path does not catch.""" + with mock.patch.object( + PR.subprocess, "run", + side_effect=subprocess.TimeoutExpired(cmd=["gh"], timeout=60), + ): + result = PR.gh_post_review("o/r", "1", "{}") + self.assertIsInstance(result, subprocess.CompletedProcess) + self.assertNotEqual(result.returncode, 0) + self.assertIn("timed out", result.stderr) + + def test_a_timed_out_post_takes_the_undecided_path(self): + """It carries no HTTP status, so it is neither a throttle nor a read-only + token — it is genuinely undecided, and the PR gets asked.""" + with mock.patch.object( + PR.subprocess, "run", + side_effect=subprocess.TimeoutExpired(cmd=["gh"], timeout=60), + ): + result = PR.gh_post_review("o/r", "1", "{}") + self.assertIsNone( + PR.gh_http_status(result), + "the message must not read as an `HTTP nnn` rendering", + ) + self.assertFalse(PR.is_throttle(result), "and not as a throttle wording either") + self.assertFalse(PR.is_read_only_token_error(result)) + self.assertTrue( + PR.post_may_have_landed(result), + "the request may have been served — ask the PR rather than guess", + ) + self.assertEqual(PR.throttle_delay_seconds(result), PR.THROTTLE_DELAY_DEFAULT_SECONDS) + + class ThrottleBackoffTest(unittest.TestCase): """The wait itself: before the landed-review read, and before the fallback POST. @@ -2107,9 +2536,28 @@ def test_a_throttled_post_whose_review_landed_posts_nothing_more(self): self.assertEqual(summaries, [], "the review is on the PR") self.assertIsNone(driver.exit_code) - def test_a_throttled_post_confirmed_absent_waits_again_then_falls_back(self): + def test_a_throttled_post_confirmed_absent_falls_back_without_a_second_wait(self): """ABSENT is the one answer that authorizes the second write — and it goes out - on the same throttled token, so it waits the window out a second time.""" + against the REMAINDER of the window, not a fresh copy of it. + + One throttled response declares ONE window, and the inline path can wait on it + twice: before the landed-review read, and before the fallback POST. Recomputing + the delay for the second wait sat out a `Retry-After` twice over, and on the + `X-RateLimit-Reset` rule it was worse than that — the reset had gone into the + past during the first sleep, so the rule fell through and the run spent a flat + THROTTLE_DELAY_DEFAULT_SECONDS after the window it had just waited out + demonstrably reopened, with `write_step_summary` still to come. + + Here the read that returned ABSENT is itself evidence the window reopened, so + the remainder is zero and the fallback goes out at once. That also keeps the + ABSENT answer FRESH: every second between the read and this POST is room for a + first write that GitHub did serve to become visible, which is the duplicate + this whole path exists to avoid. + + The fallback is throttled too (the stub answers every POST the same way), so it + declares a NEW window and post_or_degrade's own read waits that one out — a + later throttle is a later window, not the same one. + """ outputs, sleeps, trace = {}, [], [] driver = EndToEndPostTest() posted = driver.run_main( @@ -2125,14 +2573,50 @@ def test_a_throttled_post_confirmed_absent_waits_again_then_falls_back(self): self.assertEqual(len(posted), 2, "inline attempt, then the body-only fallback") self.assertEqual( [step[0] for step in trace], - # The fallback is throttled too (the stub answers every POST the same way), - # so it takes the same wait-then-read treatment through post_or_degrade. - ["post", "sleep", "list", "sleep", "post", "sleep", "list"], + ["post", "sleep", "list", "post", "sleep", "list"], + "no sleep between the ABSENT read and the fallback POST", + ) + self.assertEqual( + sleeps, [30, 30], + "one wait per throttled POST, not one per read", ) - self.assertEqual(sleeps, [30, 30, 30]) self.assertEqual(outputs["delivered"], "false") self.assertEqual(driver.exit_code, 1) + def test_a_partly_elapsed_window_waits_only_the_remainder(self): + """The deadline is absolute, so a wait that has already been half spent + finishes rather than restarts. Driven directly, since the end-to-end path + spends the whole window on its read.""" + deadline = PR.throttle_backoff_deadline( + gh_result(stdout=response("Retry-After: 30")), now=100.0 + ) + self.assertEqual(deadline, 130.0) + slept = [] + with mock.patch.object(PR.time, "sleep", side_effect=slept.append): + self.assertEqual(PR.wait_for_backoff(deadline, "r", now=118.0), 12) + self.assertEqual(PR.wait_for_backoff(deadline, "r", now=130.0), 0) + self.assertEqual(PR.wait_for_backoff(deadline, "r", now=999.0), 0) + self.assertEqual(slept, [12], "past the deadline it does not sleep at all") + + def test_a_fractional_remainder_rounds_up(self): + """The clock is fractional and the deadline is not a whole second past it; a + wait that ended a fraction early would be a retry INTO the window.""" + slept = [] + with mock.patch.object(PR.time, "sleep", side_effect=slept.append): + self.assertEqual(PR.wait_for_backoff(130.0, "r", now=129.01), 1) + self.assertEqual(slept, [1]) + + def test_a_later_throttle_gets_its_own_window(self): + """Sharing is per-response, not per-run: a second throttled POST is a new + window and the deadline from the first says nothing about it.""" + first = PR.throttle_backoff_deadline( + gh_result(stdout=response("Retry-After: 30")), now=100.0 + ) + second = PR.throttle_backoff_deadline( + gh_result(stdout=response("Retry-After: 30")), now=200.0 + ) + self.assertEqual((first, second), (130.0, 230.0)) + def test_an_unknown_answer_declines_the_fallback_and_fails_closed(self): outputs, summaries, notes, sleeps = {}, [], [], [] driver = EndToEndPostTest() @@ -2152,7 +2636,7 @@ def test_an_unknown_answer_declines_the_fallback_and_fails_closed(self): self.assertEqual(outputs["delivered"], "false") self.assertEqual(outputs["posted"], "false") self.assertEqual(len(summaries), 1) - self.assertIn(PR.POST_FAILED_SUMMARY_NOTE, notes) + self.assertIn(PR.POST_UNCONFIRMED_SUMMARY_NOTE, notes) self.assertEqual(driver.exit_code, 1) def test_a_timeout_status_takes_the_read_immediately(self): diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 364fae2..70bf527 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -2144,20 +2144,30 @@ jobs: # one paginated read of the PR's reviews (one `gh` command, but one request PER # PAGE), to tell a review that never landed from one that landed despite the # error (`pull-requests: write` already implies that read, so no permission - # changes here) — minutes of work at most. That read carries its own - # GH_LIST_REVIEWS_TIMEOUT_SECONDS well under this budget, so a wedged list cannot - # eat the job's whole allowance and take the fallback POST down with it. + # changes here) — minutes of work at most. Every one of those calls is bounded: + # the reads by GH_LIST_REVIEWS_TIMEOUT_SECONDS and the POSTs by + # GH_POST_REVIEW_TIMEOUT_SECONDS, both well under this budget, so a wedged call + # cannot eat the job's whole allowance and take the fallback POST — or the job + # summary that is written after it — down with it. # - # A THROTTLED write (429, or one of the 403 wordings that mean "slow down") adds a - # bounded WAIT in front of each of those reads, and one more before the body-only - # fallback — otherwise the read goes out on the token GitHub just throttled and - # comes back "could not tell", which is the answer that used to cost a duplicate - # review. Each wait is `Retry-After` (or the `X-RateLimit-Reset` window) clamped to - # THROTTLE_DELAY_MAX_SECONDS, so the worst case is three 90s waits plus two bounded - # reads — 6.5 minutes, inside this budget with the artifact downloads and the token - # mint alongside it. Nothing here retries an unconfirmed write: a throttle whose - # landed-review read is still unreadable after the wait declines the fallback and - # goes red with the review in the job summary. + # A THROTTLED write (429, one of the 403 wordings that mean "slow down", or a 403 + # carrying its own `Retry-After`) adds a bounded WAIT in front of the landed-review + # read — otherwise that read goes out on the token GitHub just throttled and comes + # back "could not tell", which is the answer that used to cost a duplicate review. + # The wait is `Retry-After` (or the `X-RateLimit-Reset` window) clamped to + # THROTTLE_DELAY_MAX_SECONDS, and it is ONE window per throttled POST: the inline + # path's read and its body-only fallback share a single deadline, so the fallback + # finishes the remainder rather than sitting out a second full wait. Worst case is + # therefore two 90s waits plus two bounded reads and two bounded POSTs — 7 minutes, + # inside this budget with the artifact downloads and the token mint alongside it. + # + # Nothing here retries an unconfirmed write: a throttle whose landed-review read is + # still unreadable after the wait declines the fallback and goes red with the review + # in the job summary. Nor does anything call the API inside a window GitHub closed — + # a declared embargo longer than THROTTLE_DELAY_MAX_SECONDS (a primary rate limit + # sends hour-long values) skips the wait, the read and the fallback outright and + # goes straight to the job summary, since retrying inside it escalates the limit for + # the whole App installation. # # Bounded like every other job here so a rate-limited or hung call cannot hold a # runner for the 6-hour default.