diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 63853e4..12deb58 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -283,8 +283,42 @@ def gh_post_review(repo: str, pr_number: str, payload: str) -> subprocess.Comple ) +# 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 — 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", + # "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. + + 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: - """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 @@ -292,9 +326,43 @@ 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. + + 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 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. """ - blob = result.stderr or "" - return "Resource not accessible by integration" in blob or "HTTP 403" 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 @@ -304,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. @@ -327,13 +440,34 @@ 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` 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}) + + +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 @@ -643,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` @@ -657,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( @@ -668,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 @@ -1898,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.", @@ -1908,16 +2090,18 @@ 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, 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 + # 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 @@ -1926,18 +2110,12 @@ 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. - 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( @@ -2098,8 +2276,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 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 d048236..6dcdacb 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 @@ -966,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)) @@ -1078,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 @@ -1288,11 +1293,472 @@ 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 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 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 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 + 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 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)" + ) + + 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. + + 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), + ("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): + 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_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") + + # 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` 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(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", + ) + 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 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", + "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. + + 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"), ("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) + 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"), ("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/cursor-review/tests/test_post_review_delivery.py b/.github/cursor-review/tests/test_post_review_delivery.py index edce004..d2959ce 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,15 +354,45 @@ 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_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 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.assertIsNone(self.exit_code) + 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): diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index d52afff..319e3ed 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -2824,15 +2824,21 @@ 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 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 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 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"