From f7701272bb13c1015dbadc094ed3fb7dc6ee221e Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 17:09:05 -0700 Subject: [PATCH 1/4] 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/4] 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/4] 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/4] 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",