[PR #277] [MERGED] fix(cursor-review): match the read-only-token 403 by message and let a throttled 403 reach the landed-review check #279

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-12612-narrow-read-only-403-guard

📝 Commits (4)

f770127 fix(cursor-review): match the read-only-token 403 by message, not by status (BE-12612)
486af72 fix(cursor-review): key the read-only 403 guard on status + a throttle allowlist (BE-12612)
87c274f fix(cursor-review): read every gh status rendering, and ask the PR on all three failure paths (BE-12612)
5180568 docs(cursor-review): refresh two test docstrings the round-2 refactor left stale (BE-12612)

📊 Changes

4 files changed (+750 additions, -51 deletions)

View changed files

📝 .github/cursor-review/post-review.py (+218 -39)
📝 .github/cursor-review/tests/test_post_review.py (+468 -2)
📝 .github/cursor-review/tests/test_post_review_delivery.py (+53 -5)
📝 .github/workflows/cursor-review.yml (+11 -5)

📄 Description

ELI-5

When the review bot fails to post a review, GitHub tells it why with an HTTP status. A 403 used to mean one thing to this script — "your token can't write here" — so it quietly wrote the review to the job summary and exited green. But GitHub also answers 403 when it is throttling you, and a throttle isn't a permissions problem: the write may well have gone through. This teaches the script to recognise the throttle wordings specifically, so a throttled 403 now goes and asks the PR what actually happened instead of being mislabelled as a read-only token — while every other 403, which really is a standing refusal, keeps degrading quietly exactly as before.

Description

is_read_only_token_error matched the bare HTTP 403 substring. main() returns from that branch before the landed-review check, so a rate-limit or abuse-detection 403 was reported as a read-only token, written to the job summary, and exited 0 — with the PR never asked whether the review had landed.

Two halves, because narrowing the guard alone would not have fixed it:

The guard keys on status AND message. gh_http_status(result) == 403 and not is_throttled_403(result). The status conjunction matters because the permission wording also travels inside a 422's errors[].message list, which gh joins into the same stderr blob — reading that as a read-only token would be the same silent skip this PR removes, reached from the other direction. The message test is an allowlist of throttle wordings (THROTTLE_403_MESSAGES = ("rate limit", "abuse detection", "submitted too quickly"), matched case-insensitively), not a denylist of the permission phrase; see "Why an allowlist" below.

Conjoining the status made _GH_HTTP_STATUS_RE load-bearing, and it matched only gh's trailing (HTTP 403). go-gh also leads with the status — HTTP 403 (<url>) when the body carried no message, HTTP 422: <msg> (<url>)\n<rest> when it carried errors[] — so those rendered as "no status at all" and a standing permission or SSO 403 fell through to a doomed read, a doomed fallback and SystemExit(1) on every run. The regex now matches both renderings, and gh_http_status 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. is_throttled_403 likewise reads the new gh_error_line(result) — gh's own error line — rather than all of stderr, so a review that discusses rate limiting cannot turn its own permission 403 into a throttle.
403 joins RETRYABLE_4XX_STATUSES ({403, 408, 425, 429}). Without this, a 403 that gets past the guard lands in pre_write_rejection, which treats any 4xx outside that set as absent-by-construction and skips the read anyway. Any 403 reaching that decision is now a throttle and nothing else, and a throttle can be raised on a request the API went on to serve — so it takes the read, exactly like the 429 beside it.

Why an allowlist of throttles, not a denylist of the permission phrase

The first cut of this change matched the permission message and sent everything else 403 into the landed-review read. That is not true of a 403: an SSO/SAML block, an org IP-allowlist refusal, an archived repo, a non-JSON 403 from a proxy or GHES, and any future rewording of the permission message itself are all standing refusals that wrote nothing and that no retry fixes. Routing them into a doomed read plus a doomed fallback POST ends in SystemExit(1) on every run — a permanently red check in exactly the orgs least able to change it, where the caller previously got its review in the job summary and a green step.

Inverting to an allowlist keeps every one of those on the old green degrade, and lets through only the case the ticket is actually about.

Behaviour change worth knowing at review time

A throttled 403 used to exit 0 with the review in the job summary. It now takes one review-list read and, if the review is genuinely absent, posts the fallback — and if that is throttled too, the step goes red with the summary under POST_FAILED_SUMMARY_NOTE. That is the intended correction: a throttle is a real failure, not an environment constraint. Every other 403 is byte-for-byte unchanged at both call sites (main() and post_or_degrade): same log line, emit_delivery(False), write_step_summary(prose_body) under READ_ONLY_SUMMARY_NOTE, exit 0.

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 — gated by a shared post_may_have_landed(result) so the same question is answered the same way on each. Narrowing which 403s reach the two read-less 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. Those two paths now read; they still never repost, and only a PRESENT answer changes anything.

The PRESENT / ABSENT / UNKNOWN branches, post_or_degrade's return contract and emit_delivery are untouched — post_or_degrade's returncode-0 block is factored into a report_posted() closure so its PRESENT answer reports identically. cursor-review.yml changes only in comment and DETAIL prose (see below).

Test coverage

ReadOnlyGuardExcludesThrottlesTest in .github/cursor-review/tests/test_post_review.py, driven through EndToEndPostTest.run_main:

permission 403 unchanged — no list read, exactly one POST, delivered=false, posted=false, one summary write taking the default banner (pinned to be READ_ONLY_SUMMARY_NOTE via the parameter default), exit 0; driven through both of gh's renderings, since the guard now depends on parsing either. (The old lower-cased subtest is gone: the guard never inspects the permission wording, so it exercised an identical path.)
org-policy 403 unchanged (test_a_policy_403_still_degrades_to_the_summary) — the regression guard on the allowlist, driving the OAuth-App-restriction wording, which shares nothing with the permission phrase;
status and message are both load-bearing (test_the_guard_needs_the_status_as_well_as_the_message) — the permission phrase under a 422 is not a read-only token; a transport error with no status is not one either; an archived-repo 403 is;
every throttle wording falls through (test_every_throttle_wording_falls_through_the_guard) — the four wordings GitHub actually sends, each as-sent and lower-cased, asserted to fail the guard and keep a parseable 403;
throttled 403 with the review PRESENT — read once, one POST, delivered=true, no summary;
throttled 403 with the review ABSENT and the fallback throttled too — two POSTs, delivered=false, summary under POST_FAILED_SUMMARY_NOTE, SystemExit(1);
throttled 403 with the list UNKNOWN — fallback posted, nothing tagged lost_to_fallback;
gh_http_status("… secondary rate limit (HTTP 403)") == 403 alongside the existing 422/502 cases;
the status parser over every rendering (test_gh_status_is_read_out_of_every_rendering_gh_uses) — trailing (HTTP 422), leading HTTP 403 (<url>), leading-with-errors[] HTTP 422: … (<url>)\n<rest>, and a GH_DEBUG=api trace whose quoted body must lose to the real error line;
a debug trace cannot manufacture a throttle (test_a_debug_trace_quoting_a_throttle_does_not_make_one) — a permission 403 under a trace echoing a body that says "a rate limit is answered with 403, see abuse detection" is still read-only and still degrades green;
the two newly-reading paths, both answers — test_a_throttled_403_with_no_inline_half_reports_the_landed_review / …_and_nothing_landed_stays_red, and test_a_throttled_403_on_the_no_findings_review_reports_the_landed_review / test_a_standing_403_on_the_no_findings_review_never_reaches_the_read.

In test_post_review_delivery.py, the bare-403 case previously used gh: HTTP 403: Forbidden, which _GH_HTTP_STATUS_RE does not parse — it reached the read through the "no status at all" branch and would have passed with either half of the change reverted. Rewritten around gh's real shape as test_a_throttled_403_is_no_longer_read_as_read_only, paired with test_an_unworded_403_still_degrades_green.

That file's MainDriverMixin also drove main() end to end with no gh_list_reviews stub — harmless while every failure path short-circuited on its status, but the moment a 403 stopped doing so the unit suite shelled out to a real gh. It now stubs the read the same way test_post_review.py's driver does, defaulting to an empty page. Test infrastructure, not a product change.

`cursor-review.yml` prose

The notify-complete comment at ~line 2823 and the user-facing DETAIL string beneath it both described the bare-403 rule ("treats any HTTP 403 as read-only, so an SSO/IP-allowlist, secondary-rate-limit or archived-repo rejection lands here too"). Both are updated: SSO/IP-allowlist and archived-repo still reach that green branch, and a 403 GitHub words as a rate limit no longer does. The DETAIL no longer claims a rate limit never reaches it — an unworded 403 (an edge, a WAF or a GHES proxy with no JSON body) carries nothing to tell a throttle from a standing refusal by, so it is read as standing and does land there. This was the PR's own recorded residual and is now closed.

Residual

The 403 reachability claim is inherited, not re-verified here. That GitHub answers primary and secondary rate limits with 403 (or 429), and that gh api renders it as gh: <message> (HTTP 403), comes from the investigation that produced this plan; this PR did not issue a live throttled request against GitHub to reproduce it, and doing so would mean deliberately exhausting a rate limit, which is out of bounds for verification. The tests exercise the parsing and branch selection against the documented stderr shapes rather than against a live throttle.
No Retry-After backoff before the landed-review read. A throttled POST is answered by an immediate paginated read plus, on ABSENT/UNKNOWN, a second POST on the same token GitHub just asked to slow down — and now, since post_or_degrade reads too, a second read when that fallback POST is throttled as well. This shape is pre-existing — 429 has been in RETRYABLE_4XX_STATUSES all along — and this PR widens which statuses reach it. Fixing it means capturing response headers (gh api does not surface them on the default path), so it is filed as a follow-up (BE-12679) rather than done here.
A throttled read still yields UNKNOWN, and the inline path posts the fallback on UNKNOWN — duplicating a first write that did land. Same root cause as the bullet above and tracked with it; the guard's docstring now names this residual instead of implying its absence.
An unworded 403 is classified as a standing refusal. A secondary-limit or abuse refusal served by an edge with no JSON message renders as gh: Forbidden (HTTP 403) and keeps the green degrade with delivered=false. Routing it into the read instead would restore the permanently-red check for anyone behind a WAF or GHES proxy, which is the regression the allowlist exists to avoid; the header capture in BE-12679 is the discriminator that closes it properly. The DETAIL prose now says so rather than asserting the opposite.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py': 476 tests, OK; python3 -m unittest discover -s .github/agents-md-integrity/tests -p 'test_*.py': 46 tests, OK; python3 -m unittest discover -s .github/groom/tests -p 'test_*.py': 374 tests, OK (1 skipped); python3 .github/agents-md-integrity/check_agents_md.py --root .: passed (2 pre-existing warnings, unrelated); .github/workflows/cursor-review.yml re-parsed as YAML and the edited DETAIL line re-parsed by bash -n after the prose edit. Mutation check on the round-2 commit: with .github/cursor-review/post-review.py reverted to the previous commit, 11 cases fail — including test_the_permission_403_still_degrades_to_the_summary on its go-gh's leading-status rendering subtest, which is the permanently-red regression the round-2 panel raised. Earlier mutation checks on the first commit still hold: reverting the status conjunction alone gave 5 failures, reverting 403 out of RETRYABLE_4XX_STATUSES alone gave 3.
Deviations: the review panel's finding that "everything but the permission phrase is retryable" regresses SSO/IP-allowlist/archived-repo callers to a permanently red check was accepted, which inverted the discriminator from a permission denylist to a throttle allowlist — a design change from the plan this PR started with. Round 2 then took two more: _GH_HTTP_STATUS_RE was widened to go-gh's leading-status renderings (the conjunction had made an unparseable rendering a fall-through, the exact regression the allowlist was protecting against), and the landed-review read was extended to main()'s no-inline-comments branch and post_or_degrade — which the round-1 reply had explicitly declined in favour of scoping the docstring, and which the panel re-raised as documenting the gap rather than closing it. The panel's remaining suggestion on this round, routing an unworded 403 into the read, was declined for the reason the allowlist exists and left to BE-12679. The plan also scoped .github/workflows/cursor-review.yml out; its notify-complete comment and DETAIL string are edited here anyway, since the panel flagged them as stale in the same commit that made them stale. .github/cursor-review/README.md and docs/callers/cursor-review.md describe only "a read-only token", which is still accurate, so neither was changed.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/277 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/9/2026 **Status:** ✅ Merged **Merged:** 9/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-12612-narrow-read-only-403-guard` --- ### 📝 Commits (4) - [`f770127`](https://github.com/Comfy-Org/github-workflows/commit/f7701272bb13c1015dbadc094ed3fb7dc6ee221e) fix(cursor-review): match the read-only-token 403 by message, not by status (BE-12612) - [`486af72`](https://github.com/Comfy-Org/github-workflows/commit/486af720064596721aedf72b239ddf0ad996f08d) fix(cursor-review): key the read-only 403 guard on status + a throttle allowlist (BE-12612) - [`87c274f`](https://github.com/Comfy-Org/github-workflows/commit/87c274fcb21fcd2cec7adb1c94bb68cc7cbfb033) fix(cursor-review): read every gh status rendering, and ask the PR on all three failure paths (BE-12612) - [`5180568`](https://github.com/Comfy-Org/github-workflows/commit/5180568ba417065c1c982f7cf5d0481dd33e150d) docs(cursor-review): refresh two test docstrings the round-2 refactor left stale (BE-12612) ### 📊 Changes **4 files changed** (+750 additions, -51 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/post-review.py` (+218 -39) 📝 `.github/cursor-review/tests/test_post_review.py` (+468 -2) 📝 `.github/cursor-review/tests/test_post_review_delivery.py` (+53 -5) 📝 `.github/workflows/cursor-review.yml` (+11 -5) </details> ### 📄 Description ## ELI-5 When the review bot fails to post a review, GitHub tells it why with an HTTP status. A `403` used to mean one thing to this script — "your token can't write here" — so it quietly wrote the review to the job summary and exited green. But GitHub also answers `403` when it is *throttling* you, and a throttle isn't a permissions problem: the write may well have gone through. This teaches the script to recognise the throttle wordings specifically, so a throttled `403` now goes and asks the PR what actually happened instead of being mislabelled as a read-only token — while every other `403`, which really is a standing refusal, keeps degrading quietly exactly as before. ## Description `is_read_only_token_error` matched the bare `HTTP 403` substring. `main()` returns from that branch before the landed-review check, so a rate-limit or abuse-detection `403` was reported as a read-only token, written to the job summary, and exited 0 — with the PR never asked whether the review had landed. Two halves, because narrowing the guard alone would not have fixed it: 1. **The guard keys on status AND message.** `gh_http_status(result) == 403 and not is_throttled_403(result)`. The status conjunction matters because the permission wording also travels inside a `422`'s `errors[].message` list, which `gh` joins into the same stderr blob — reading that as a read-only token would be the same silent skip this PR removes, reached from the other direction. The message test is an **allowlist of throttle wordings** (`THROTTLE_403_MESSAGES = ("rate limit", "abuse detection", "submitted too quickly")`, matched case-insensitively), not a denylist of the permission phrase; see "Why an allowlist" below. Conjoining the status made `_GH_HTTP_STATUS_RE` load-bearing, and it matched only `gh`'s trailing `(HTTP 403)`. go-gh also **leads** with the status — `HTTP 403 (<url>)` when the body carried no message, `HTTP 422: <msg> (<url>)\n<rest>` when it carried `errors[]` — so those rendered as "no status at all" and a standing permission or SSO `403` fell through to a doomed read, a doomed fallback and `SystemExit(1)` on every run. The regex now matches both renderings, and `gh_http_status` 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. `is_throttled_403` likewise reads the new `gh_error_line(result)` — `gh`'s own error line — rather than all of stderr, so a review that *discusses* rate limiting cannot turn its own permission `403` into a throttle. 2. **403 joins `RETRYABLE_4XX_STATUSES`** (`{403, 408, 425, 429}`). Without this, a `403` that gets past the guard lands in `pre_write_rejection`, which treats any 4xx outside that set as absent-by-construction and skips the read anyway. Any `403` reaching that decision is now a throttle and nothing else, and a throttle can be raised on a request the API went on to serve — so it takes the read, exactly like the `429` beside it. ### Why an allowlist of throttles, not a denylist of the permission phrase The first cut of this change matched the permission message and sent *everything else* `403` into the landed-review read. That is not true of a `403`: an SSO/SAML block, an org IP-allowlist refusal, an archived repo, a non-JSON `403` from a proxy or GHES, and any future rewording of the permission message itself are all **standing** refusals that wrote nothing and that no retry fixes. Routing them into a doomed read plus a doomed fallback POST ends in `SystemExit(1)` on every run — a permanently red check in exactly the orgs least able to change it, where the caller previously got its review in the job summary and a green step. Inverting to an allowlist keeps every one of those on the old green degrade, and lets through only the case the ticket is actually about. ## Behaviour change worth knowing at review time A **throttled** `403` used to exit 0 with the review in the job summary. It now takes one review-list read and, if the review is genuinely absent, posts the fallback — and if that is throttled too, the step goes **red** with the summary under `POST_FAILED_SUMMARY_NOTE`. That is the intended correction: a throttle is a real failure, not an environment constraint. Every other `403` is byte-for-byte unchanged at both call sites (`main()` and `post_or_degrade`): same log line, `emit_delivery(False)`, `write_step_summary(prose_body)` under `READ_ONLY_SUMMARY_NOTE`, exit 0. 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` — gated by a shared `post_may_have_landed(result)` so the same question is answered the same way on each. Narrowing *which* `403`s reach the two read-less 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. Those two paths now **read**; they still never repost, and only a PRESENT answer changes anything. The PRESENT / ABSENT / UNKNOWN branches, `post_or_degrade`'s return contract and `emit_delivery` are untouched — `post_or_degrade`'s returncode-0 block is factored into a `report_posted()` closure so its PRESENT answer reports identically. `cursor-review.yml` changes only in comment and `DETAIL` prose (see below). ## Test coverage `ReadOnlyGuardExcludesThrottlesTest` in `.github/cursor-review/tests/test_post_review.py`, driven through `EndToEndPostTest.run_main`: - permission `403` unchanged — no list read, exactly one POST, `delivered=false`, `posted=false`, one summary write taking the default banner (pinned to be `READ_ONLY_SUMMARY_NOTE` via the parameter default), exit 0; driven through **both** of `gh`'s renderings, since the guard now depends on parsing either. (The old lower-cased subtest is gone: the guard never inspects the permission wording, so it exercised an identical path.) - **org-policy `403` unchanged** (`test_a_policy_403_still_degrades_to_the_summary`) — the regression guard on the allowlist, driving the OAuth-App-restriction wording, which shares nothing with the permission phrase; - **status and message are both load-bearing** (`test_the_guard_needs_the_status_as_well_as_the_message`) — the permission phrase under a `422` is not a read-only token; a transport error with no status is not one either; an archived-repo `403` is; - **every throttle wording falls through** (`test_every_throttle_wording_falls_through_the_guard`) — the four wordings GitHub actually sends, each as-sent and lower-cased, asserted to fail the guard and keep a parseable `403`; - throttled `403` with the review **PRESENT** — read once, one POST, `delivered=true`, no summary; - throttled `403` with the review **ABSENT** and the fallback throttled too — two POSTs, `delivered=false`, summary under `POST_FAILED_SUMMARY_NOTE`, `SystemExit(1)`; - throttled `403` with the list **UNKNOWN** — fallback posted, nothing tagged `lost_to_fallback`; - `gh_http_status("… secondary rate limit (HTTP 403)") == 403` alongside the existing 422/502 cases; - **the status parser over every rendering** (`test_gh_status_is_read_out_of_every_rendering_gh_uses`) — trailing `(HTTP 422)`, leading `HTTP 403 (<url>)`, leading-with-`errors[]` `HTTP 422: … (<url>)\n<rest>`, and a `GH_DEBUG=api` trace whose quoted body must lose to the real error line; - **a debug trace cannot manufacture a throttle** (`test_a_debug_trace_quoting_a_throttle_does_not_make_one`) — a permission `403` under a trace echoing a body that says "a rate limit is answered with 403, see abuse detection" is still read-only and still degrades green; - **the two newly-reading paths, both answers** — `test_a_throttled_403_with_no_inline_half_reports_the_landed_review` / `…_and_nothing_landed_stays_red`, and `test_a_throttled_403_on_the_no_findings_review_reports_the_landed_review` / `test_a_standing_403_on_the_no_findings_review_never_reaches_the_read`. In `test_post_review_delivery.py`, the bare-403 case previously used `gh: HTTP 403: Forbidden`, which `_GH_HTTP_STATUS_RE` does not parse — it reached the read through the "no status at all" branch and would have passed with either half of the change reverted. Rewritten around `gh`'s real shape as `test_a_throttled_403_is_no_longer_read_as_read_only`, paired with `test_an_unworded_403_still_degrades_green`. That file's `MainDriverMixin` also drove `main()` end to end with no `gh_list_reviews` stub — harmless while every failure path short-circuited on its status, but the moment a `403` stopped doing so the unit suite shelled out to a real `gh`. It now stubs the read the same way `test_post_review.py`'s driver does, defaulting to an empty page. Test infrastructure, not a product change. ## `cursor-review.yml` prose The notify-complete comment at ~line 2823 and the user-facing `DETAIL` string beneath it both described the bare-403 rule ("treats any HTTP 403 as read-only, so an SSO/IP-allowlist, secondary-rate-limit or archived-repo rejection lands here too"). Both are updated: SSO/IP-allowlist and archived-repo still reach that green branch, and a `403` GitHub *words* as a rate limit no longer does. The `DETAIL` no longer claims a rate limit never reaches it — an **unworded** `403` (an edge, a WAF or a GHES proxy with no JSON body) carries nothing to tell a throttle from a standing refusal by, so it is read as standing and does land there. This was the PR's own recorded residual and is now closed. ## Residual - **The `403` reachability claim is inherited, not re-verified here.** That GitHub answers primary and secondary rate limits with `403` (or `429`), and that `gh api` renders it as `gh: <message> (HTTP 403)`, comes from the investigation that produced this plan; this PR did not issue a live throttled request against GitHub to reproduce it, and doing so would mean deliberately exhausting a rate limit, which is out of bounds for verification. The tests exercise the parsing and branch selection against the documented stderr shapes rather than against a live throttle. - **No `Retry-After` backoff before the landed-review read.** A throttled POST is answered by an immediate paginated read plus, on ABSENT/UNKNOWN, a second POST on the same token GitHub just asked to slow down — and now, since `post_or_degrade` reads too, a *second* read when that fallback POST is throttled as well. This shape is pre-existing — `429` has been in `RETRYABLE_4XX_STATUSES` all along — and this PR widens which statuses reach it. Fixing it means capturing response headers (`gh api` does not surface them on the default path), so it is filed as a follow-up (BE-12679) rather than done here. - **A throttled read still yields UNKNOWN, and the inline path posts the fallback on UNKNOWN** — duplicating a first write that did land. Same root cause as the bullet above and tracked with it; the guard's docstring now names this residual instead of implying its absence. - **An unworded `403` is classified as a standing refusal.** A secondary-limit or abuse refusal served by an edge with no JSON `message` renders as `gh: Forbidden (HTTP 403)` and keeps the green degrade with `delivered=false`. Routing it into the read instead would restore the permanently-red check for anyone behind a WAF or GHES proxy, which is the regression the allowlist exists to avoid; the header capture in BE-12679 is the discriminator that closes it properly. The `DETAIL` prose now says so rather than asserting the opposite. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'`: 476 tests, OK; `python3 -m unittest discover -s .github/agents-md-integrity/tests -p 'test_*.py'`: 46 tests, OK; `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py'`: 374 tests, OK (1 skipped); `python3 .github/agents-md-integrity/check_agents_md.py --root .`: passed (2 pre-existing warnings, unrelated); `.github/workflows/cursor-review.yml` re-parsed as YAML and the edited `DETAIL` line re-parsed by `bash -n` after the prose edit. Mutation check on the round-2 commit: with `.github/cursor-review/post-review.py` reverted to the previous commit, 11 cases fail — including `test_the_permission_403_still_degrades_to_the_summary` on its `go-gh's leading-status rendering` subtest, which is the permanently-red regression the round-2 panel raised. Earlier mutation checks on the first commit still hold: reverting the status conjunction alone gave 5 failures, reverting `403` out of `RETRYABLE_4XX_STATUSES` alone gave 3. - **Deviations:** the review panel's finding that "everything but the permission phrase is retryable" regresses SSO/IP-allowlist/archived-repo callers to a permanently red check was accepted, which inverted the discriminator from a permission denylist to a throttle allowlist — a design change from the plan this PR started with. Round 2 then took two more: `_GH_HTTP_STATUS_RE` was widened to go-gh's leading-status renderings (the conjunction had made an unparseable rendering a *fall-through*, the exact regression the allowlist was protecting against), and the landed-review read was extended to `main()`'s no-inline-comments branch and `post_or_degrade` — which the round-1 reply had explicitly declined in favour of scoping the docstring, and which the panel re-raised as documenting the gap rather than closing it. The panel's remaining suggestion on this round, routing an *unworded* `403` into the read, was declined for the reason the allowlist exists and left to BE-12679. The plan also scoped `.github/workflows/cursor-review.yml` out; its notify-complete comment and `DETAIL` string are edited here anyway, since the panel flagged them as stale in the same commit that made them stale. `.github/cursor-review/README.md` and `docs/callers/cursor-review.md` describe only "a read-only token", which is still accurate, so neither was changed. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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