From dbf920b27df9dbe742cd74764a3c9db53cf9e042 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 11:12:53 -0700 Subject: [PATCH 1/3] fix(cursor-review): confirm the first review is absent before tagging findings lost_to_fallback (BE-12528) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A nonzero `gh` exit on the review POST is not proof the review was never committed server-side. BE-10002 tagged every anchored finding `lost_to_fallback` on that exit alone and wrote the hole down as a residual: when the write HAD landed, the fallback posted a second review and the next round's ledger carried each anchored finding twice — once with its real thread, once as a cap-exempt [post-failed] entry claiming nobody could have answered it. The failure path now establishes the answer, cheapest sufficient evidence first. A 4xx is GitHub validating and rejecting before writing (every firing observed in the field is a 422 over an inline position), so the review is absent by construction and no read is spent. Anything else — a 5xx, or a transport error carrying no status at all — takes one paginated GET of the PR's reviews, matched on the same four-part discriminator the blocking gate and the workflow's dup-check use. Paginated because the newest review sits on the last page of an oldest-first list. Three outcomes: PRESENT reports the review delivered and posts nothing more (the success tail is now a shared `finish_posted_review` closure, so both paths that leave a body on the PR report it through one implementation); ABSENT is this path exactly as before; UNKNOWN posts the fallback but tags nothing, because an unreadable list is not a confirmed absence (BE-4785). The 422 path is byte-for-byte unchanged — verified by driving both the pre-change and post-change modules over the same fixture and diffing the POST payloads. --- .github/cursor-review/post-review.py | 173 ++++++++++++-- .../cursor-review/tests/test_post_review.py | 225 +++++++++++++++++- .github/workflows/cursor-review.yml | 8 +- 3 files changed, 386 insertions(+), 20 deletions(-) diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 1b2648a3..142de5be 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -278,6 +278,93 @@ def is_read_only_token_error(result: subprocess.CompletedProcess) -> bool: return "Resource not accessible by integration" in blob or "HTTP 403" in blob +# The discriminator for "a review of THIS panel is already on the PR". Mirrors +# gate-unresolved.py's constant of the same name (and the inline jq the workflow's +# dup-check uses) — duplicated as a literal rather than imported because neither module +# imports the other, and pinned equal to the gate's by test_post_review.py, exactly the +# 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})\)") + + +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 + + +def gh_list_reviews(repo: str, pr_number: str) -> subprocess.CompletedProcess: + """Every review on the PR, oldest first, ALL pages. + + Paginated deliberately: the review this asks about is the newest one, so on a PR + with more than a page of reviews it sits on the LAST page. The workflow's own + dup-check (`cursor-review.yml`, the `already_reviewed` step) is the same + discriminator without pagination — it can afford that, because it only has to + notice a review that already exists before spending the panel, while a wrong + answer here decides whether findings are labelled lost. + + `--slurp` wraps each page in an outer array (gh >= 2.43; the runner uses a current + gh), so the caller flattens one level. + """ + return subprocess.run( + [ + "gh", + "api", + "--paginate", + "--slurp", + f"/repos/{repo}/pulls/{pr_number}/reviews", + ], + text=True, + capture_output=True, + ) + + +def review_already_posted( + repo: str, pr_number: str, commit_sha: str, posted_body: str +): + """Did the review this run tried to post actually land on the PR? + + Three-valued on purpose, because two of the three drive different behaviour and + the third must never be mistaken for either: ``True`` a review from this run is on + the PR, ``False`` confirmed absent, ``None`` could not tell (the list read failed + or came back unparseable). A read that failed is not a zero — see BE-4785 — so the + caller degrades rather than claiming the review is missing. + + The same four-part discriminator the gate and the workflow use: not DISMISSED, a + Bot author (the poster is caller-configurable, so the TYPE is the only thing this + can know), the run's own head SHA, and the panel marker. Matched on the marker + rather than only on byte-equality with `posted_body` because GitHub may normalize + what it stored (line endings, trailing whitespace); byte-equality is still tried + first since it is the strongest evidence available. + """ + result = gh_list_reviews(repo, pr_number) + if result.returncode != 0: + return None + try: + pages = json.loads(result.stdout or "[]") + except ValueError: + return None + if not isinstance(pages, list): + return None + reviews = [r for page in pages for r in (page if isinstance(page, list) else [])] + for review in reviews: + if not isinstance(review, dict) or review.get("state") == "DISMISSED": + continue + if ((review.get("user") or {}).get("type") or "") != "Bot": + continue + if review.get("commit_id") != commit_sha: + continue + body = review.get("body") or "" + if body == posted_body or body.startswith(CONSOLIDATED_MARKER): + return True + return False + + READ_ONLY_SUMMARY_NOTE = ( "> ℹ️ This review could not be posted on the PR because the run's " "`GITHUB_TOKEN` is read-only (e.g. read-only default workflow " @@ -1494,9 +1581,14 @@ def main(): } ) - result = gh_post_review(args.repo, args.pr_number, payload) + def finish_posted_review(): + """Report the inline review as delivered, and write the clamp's job summary. - if result.returncode == 0: + Shared by the two paths on which THIS body is on the PR: the `gh` POST + returned 0, and the POST errored but the review turned out to have landed + anyway (below). Those two outcomes are the same fact about the PR, so they + report it through one implementation rather than two that can drift. + """ # The split the gate needs: `comments` are the findings that got a thread a # human can resolve; `body_only_items` reached the body and can never be # resolved. A round where the second is non-empty and the first is empty is @@ -1514,6 +1606,11 @@ def main(): file=sys.stderr, ) write_step_summary(prose_body, note=TRUNCATED_SUMMARY_NOTE) + + result = gh_post_review(args.repo, args.pr_number, payload) + + if result.returncode == 0: + finish_posted_review() return # A read-only token rejects any write, so the inline-less fallback below @@ -1545,6 +1642,46 @@ def main(): write_step_summary(prose_body, note=POST_FAILED_SUMMARY_NOTE) 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. + # + # 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. 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) + if status is not None and 400 <= status < 500: + landed = False + else: + landed = review_already_posted( + args.repo, args.pr_number, args.commit_sha, posted_body + ) + + if landed is True: + print( + f"Review: the POST errored ({(result.stderr or '').strip()[:200]}) but a " + f"review for {args.commit_sha[:7]} is on the PR — not reposting; treating " + "as delivered.", + file=sys.stderr, + ) + finish_posted_review() + return + if landed is None: + print( + "Review: could not confirm whether the first POST landed (review list " + "unreadable) — posting the fallback with no finding tagged [post-failed].", + file=sys.stderr, + ) + # Fallback: same findings without inline anchors. Typical cause is line # numbers that fall outside the diff context — often the model picked # a line near the change but not on the change. @@ -1576,18 +1713,16 @@ def main(): # disclosed the degradation loudly and recovered ZERO entries — including for the # findings that anchored perfectly well and lost their thread only to the failed # POST. Every finding of the round is OFFERED to it — the ones from `inline_items` - # tagged `lost_to_fallback`, the ones already unanchorable left untagged, since the - # POST outcome changed nothing for them — and the size guard below decides how many - # of them the body can actually afford to carry. + # tagged `lost_to_fallback` only when the first review is confirmed ABSENT, the + # ones already unanchorable left untagged, since the POST outcome changed nothing + # for them — and the size guard below decides how many of them the body can + # actually afford to carry. # - # Residual (BE-10002): a nonzero `gh` result is not PROOF the review was not - # committed server-side — the `not comments` branch above declines the fallback for - # exactly that reason. If the first POST did land, its findings have real threads - # and also land in this sentinel, so next round's ledger carries each of them - # twice: once with its thread and any reply on it, once as a cap-exempt - # [post-failed] entry saying nobody could have answered it. Keying the tag on a - # confirmed-absent thread would need a read of the PR's reviews this script does - # not do, so it is written down here rather than fixed. + # That confirmation is the check above, and it has three outcomes: PRESENT returns + # before reaching here (nothing is reposted and nothing is relabelled), ABSENT is + # this path with the tag applied, and UNKNOWN is this path with the tag withheld — + # the fallback still carries every finding, each reading as [unanchorable], which + # is what an unread review list can honestly support. # # Tagged by identity, not by value: `inline_items` and `body_only_items` hold the # very objects `enriched` does, and two findings can be equal without being the @@ -1601,7 +1736,17 @@ def main(): # [unanchorable]: the conservative reading, and the one this path gave them before # BE-10002. The claim the flag makes is "this passed the diff-anchor check", and # that is a claim only a real check can make. - lost_ids = {id(item) for item in inline_items} if anchors is not None else set() + # + # Both conditions are required, and for the same reason: `lost_to_fallback` says + # "this finding anchored, and the failed POST is what cost it its thread". The + # anchor half needs a real diff check (`anchors is not None`); the lost half needs + # the first review to be confirmed ABSENT (`landed is False`), since a review that + # landed — or one nobody could look for — leaves that second claim unsupported. + lost_ids = ( + {id(item) for item in inline_items} + if (anchors is not None and landed is False) + else set() + ) sentinel_items = [ {**item, "lost_to_fallback": True} if id(item) in lost_ids else item for item in enriched diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index b0be1524..1bbfd2b2 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -205,10 +205,24 @@ class EndToEndPostTest(unittest.TestCase): """Drive main() with a stubbed `gh` and read the payload it would have sent.""" def run_main(self, findings, with_diff=True, post_returncode=0, stderr="", summaries=None, - panel=None): + panel=None, existing_reviews=None, list_returncode=0, list_calls=None, + outputs=None, notes=None): """Return the POSTed payloads. Pass `summaries` (a list) to collect step-summary writes, or `panel` to control the panel summary — the one finding-INDEPENDENT - part of the review head that a caller can make large.""" + part of the review head that a caller can make large. + + The review-list read (BE-12528) is ALWAYS stubbed, never merely when a case + cares about it: this harness drives main() end to end, so an unpatched + `gh_list_reviews` would shell out to a real `gh` from the unit suite the moment + a failure path stopped being a 4xx. The default answer is an empty page, i.e. + "confirmed absent", which is what every pre-existing case here already assumed. + + `existing_reviews` is the flat list of review objects the PR carries; it is + wrapped in one `--slurp` page, the shape the real command returns. + `list_calls`, `outputs` and `notes` are optional out-parameters: the calls the + list read received, the parsed $GITHUB_OUTPUT, and the `note=` each + write_step_summary got. + """ posted = [] def fake_post(repo, pr_number, payload): @@ -217,10 +231,26 @@ def fake_post(repo, pr_number, payload): args=["gh"], returncode=post_returncode, stdout="", stderr=stderr ) + def fake_list(repo, pr_number): + if list_calls is not None: + list_calls.append((repo, pr_number)) + return subprocess.CompletedProcess( + args=["gh"], + returncode=list_returncode, + stdout=json.dumps([existing_reviews or []]), + stderr="", + ) + def fake_summary(markdown, note=None): if summaries is not None: summaries.append(markdown) + if notes is not None: + notes.append(note) + # Once-per-process by design (the paths fall through each other and duplicate + # keys in $GITHUB_OUTPUT are ambiguous), so it has to be reset per case or the + # second run_main in a process emits nothing at all. + PR._DELIVERY_EMITTED = False with tempfile.TemporaryDirectory() as d: fpath = os.path.join(d, "consolidated.json") with open(fpath, "w", encoding="utf-8") as f: @@ -230,6 +260,7 @@ def fake_summary(markdown, note=None): {"model": "m", "review_type": "adversarial", "status": "ok"} ], }, f) + outpath = os.path.join(d, "github_output") argv = [ "post-review.py", "--findings", fpath, @@ -243,12 +274,21 @@ def fake_summary(markdown, note=None): f.write(DIFF) argv += ["--diff", dpath] 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.dict(os.environ, {"GITHUB_OUTPUT": outpath}, clear=False), \ mock.patch.object(PR, "write_step_summary", side_effect=fake_summary): try: PR.main() - except SystemExit: - pass + self.exit_code = None + except SystemExit as exc: + self.exit_code = exc.code + if outputs is not None and os.path.exists(outpath): + with open(outpath, encoding="utf-8") as f: + for raw in f.read().splitlines(): + if "=" in raw: + key, _, value = raw.partition("=") + outputs[key] = value return posted def test_the_field_regression_nine_anchor_one_lands_in_the_body(self): @@ -830,6 +870,183 @@ def test_no_finding_is_rendered_twice_in_the_fallback(self): self.assertEqual(body.count("demoted one"), 1) +class FirstReviewConfirmationTest(unittest.TestCase): + """Before tagging findings `lost_to_fallback`, confirm the first review is ABSENT. + + BE-10002 wrote the tag on the strength of a nonzero `gh` exit alone, and recorded + the hole it left as a residual: a nonzero exit is not PROOF the review was not + committed server-side. When it WAS, the fallback posted a second review and the + next round's ledger carried every anchored finding twice — once with the real + thread and any reply on it, once as a cap-exempt `[post-failed]` entry claiming + nobody could have answered it. + + So the failure path now asks, cheapest sufficient evidence first: a 4xx is GitHub + rejecting the request before writing (the observed firing is always a 422 over an + inline position), so no read is needed; anything else — a 5xx, or a transport error + with no status at all — takes one paginated read of the PR's reviews. Three + outcomes, and the third is the one an over-simplified version loses: PRESENT, + ABSENT, and UNKNOWN (BE-4785 — a guard that cannot read its input must not answer + a zero). + """ + + ANCHORED = [finding("app.py", 11), finding("app.py", 12)] + + def landed_review(self, **overrides): + review = { + "state": "COMMENTED", + "commit_id": "deadbeef", + "user": {"type": "Bot"}, + "body": f"{PR.CONSOLIDATED_MARKER}\n\nFound **2** finding(s).", + } + review.update(overrides) + return review + + def test_a_4xx_rejection_tags_post_failed_without_reading_the_reviews(self): + """The 422 path, byte-for-byte as before — and it spends no extra API call. + + GitHub validated and refused this request before writing anything, so the + review is absent by construction; asking the PR could only agree, at the cost + of a round trip on the most common failure this script sees. + """ + calls = [] + posted = EndToEndPostTest().run_main( + self.ANCHORED, + post_returncode=1, + stderr="gh: Unprocessable Entity (HTTP 422)", + list_calls=calls, + ) + self.assertEqual(calls, [], "a 4xx needs no read") + self.assertEqual(len(posted), 2, "inline attempt, then the body-only fallback") + ledger = ledger_from_posted_body(posted[1]["body"]) + self.assertEqual(ledger["post_failed_count"], len(self.ANCHORED)) + + def test_a_5xx_with_the_review_confirmed_absent_tags_post_failed(self): + """A 5xx says nothing about whether the write landed, so this one asks — and + an empty review list is the confirmation the tag needs.""" + calls = [] + posted = EndToEndPostTest().run_main( + self.ANCHORED, + post_returncode=1, + stderr="gh: Bad Gateway (HTTP 502)", + existing_reviews=[], + list_calls=calls, + ) + self.assertEqual(calls, [("o/r", "1")], "asked the PR exactly once") + self.assertEqual(len(posted), 2) + ledger = ledger_from_posted_body(posted[1]["body"]) + self.assertEqual(ledger["post_failed_count"], len(self.ANCHORED)) + + def test_a_5xx_with_the_review_present_skips_the_fallback(self): + """The residual, closed: the write DID land, so there is nothing to repost and + nothing was lost. One review on the PR, `delivered=true`, exit 0.""" + outputs, summaries, notes, calls = {}, [], [], [] + driver = EndToEndPostTest() + posted = driver.run_main( + self.ANCHORED, + post_returncode=1, + stderr="gh: Bad Gateway (HTTP 502)", + existing_reviews=[self.landed_review()], + list_calls=calls, + outputs=outputs, + summaries=summaries, + notes=notes, + ) + self.assertEqual(calls, [("o/r", "1")]) + self.assertEqual(len(posted), 1, "no duplicate review is posted") + self.assertNotIn( + PR.POST_FAILED_SUMMARY_NOTE, notes, + "nothing failed to reach the PR, so nothing degrades to the summary", + ) + self.assertEqual(outputs["delivered"], "true") + self.assertEqual(outputs["gated_findings"], "2", "both findings kept a thread") + self.assertEqual(outputs["posted"], "true") + self.assertIsNone(driver.exit_code, "the step is green: the review is on the PR") + + def test_a_present_review_by_a_human_or_on_another_commit_does_not_count(self): + """The same four-part discriminator the gate and the workflow's dup-check use. + + A human's review, a review of a different head SHA, and a DISMISSED one are all + reviews on the PR that are NOT this run's — reading any of them as "it landed" + would suppress a fallback the round genuinely needs and lose every finding. + """ + for label, review in ( + ("a human author", self.landed_review(user={"type": "User"})), + ("another commit", self.landed_review(commit_id="cafebabe")), + ("dismissed", self.landed_review(state="DISMISSED")), + ): + with self.subTest(review=label): + posted = EndToEndPostTest().run_main( + self.ANCHORED, + post_returncode=1, + stderr="gh: Bad Gateway (HTTP 502)", + existing_reviews=[review], + ) + self.assertEqual(len(posted), 2, "the fallback still posts") + ledger = ledger_from_posted_body(posted[1]["body"]) + self.assertEqual(ledger["post_failed_count"], len(self.ANCHORED)) + + def test_an_unreadable_review_list_posts_the_fallback_untagged(self): + """UNKNOWN is neither of the other two. The findings still reach the ledger — + losing them is never the answer — but as `[unanchorable]`, the conservative + reading, because `lost_to_fallback` is a claim and nothing here supports it.""" + stderr_buf = io.StringIO() + with contextlib.redirect_stderr(stderr_buf): + posted = EndToEndPostTest().run_main( + self.ANCHORED, + post_returncode=1, + stderr="gh: Bad Gateway (HTTP 502)", + list_returncode=1, + ) + self.assertEqual(len(posted), 2, "the fallback still posts") + fallback = posted[1]["body"] + self.assertIn(PR.BODY_ONLY_SENTINEL_PREFIX, fallback, "the findings still reach it") + ledger = ledger_from_posted_body(fallback) + self.assertEqual(ledger["entry_count"], len(self.ANCHORED)) + for entry in ledger["entries"]: + self.assertNotIn("lost_to_fallback", entry) + self.assertEqual(ledger["post_failed_count"], 0) + self.assertEqual(ledger["unanchorable_count"], len(self.ANCHORED)) + self.assertIn("could not confirm whether the first POST landed", stderr_buf.getvalue()) + + def test_a_transport_error_with_no_http_status_goes_through_the_read(self): + """No status at all is UNDECIDED, not "not a 4xx we recognize" — a connection + that dropped after the request left may well have been served.""" + calls = [] + EndToEndPostTest().run_main( + self.ANCHORED, + post_returncode=1, + stderr="error connecting to api.github.com", + existing_reviews=[], + list_calls=calls, + ) + self.assertEqual(calls, [("o/r", "1")]) + + def test_the_consolidated_marker_matches_gate_unresolved(self): + """One discriminator, three readers (the gate, the ledger, and now this) — so + a reword that moved only one of them would make this path stop recognizing the + review it just posted. Copied rather than imported (neither module imports the + other), pinned here exactly as test_build_ledger.py pins the ledger's copy.""" + spec = importlib.util.spec_from_file_location( + "gate_unresolved", + os.path.join(os.path.dirname(__file__), "..", "gate-unresolved.py"), + ) + gate_unresolved = importlib.util.module_from_spec(spec) + spec.loader.exec_module(gate_unresolved) + self.assertEqual(PR.CONSOLIDATED_MARKER, gate_unresolved.CONSOLIDATED_MARKER) + + def test_gh_http_status_parses_gh_stderr(self): + def status(text): + return PR.gh_http_status( + subprocess.CompletedProcess(args=["gh"], returncode=1, stderr=text) + ) + + self.assertEqual(status("gh: Unprocessable Entity (HTTP 422)"), 422) + self.assertEqual(status("gh: Bad Gateway (HTTP 502)"), 502) + 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 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 09603e35..5a92f396 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -2140,8 +2140,12 @@ jobs: runs-on: ubuntu-latest # Three artifact downloads, a token mint, and post-review.py's ONE # `gh api POST /pulls/N/reviews` carrying every inline comment in a single - # payload — minutes of work at most. Bounded like every other job here so a - # rate-limited or hung call cannot hold a runner for the 6-hour default. + # payload — plus, on a non-4xx POST failure only, at most one paginated GET of + # the PR's reviews, to tell a review that never landed from one that landed + # despite the error (`pull-requests: write` already implies that read, so no + # permission changes here) — minutes of work at most. Bounded like every other + # job here so a rate-limited or hung call cannot hold a runner for the 6-hour + # default. timeout-minutes: 10 permissions: contents: read From 2f35f4009964622d9f327830311e741361a221a0 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 13:12:33 -0700 Subject: [PATCH 2/3] fix(cursor-review): identify the landed review by body, not by the panel marker (BE-12528) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review-panel findings on #272, all against `review_already_posted`. - Identity, not family (6/6 reviewers). `body.startswith(CONSOLIDATED_MARKER)` answers "some bot panel review exists at this SHA" — which a previous round's body-only fallback and any `post_error_review` body both satisfy, being Bot-authored with the same `commit_id`. Matching one returned True, so the caller skipped the fallback and reported `delivered=true` while THIS round's findings reached neither the PR nor the job summary: a loose match traded a duplicate review for a silent loss. Now the stored body must BE `posted_body`, up to CRLF/trailing-whitespace normalization; the marker survives as a cheap prefix reject ahead of that (pinned by a test, since every head variant — attribution, `--notice`, `--ledger-note` — appends rather than prepends). - PENDING is not landed. `GET /pulls/{n}/reviews` returns the authenticated identity's own unsubmitted reviews, and that identity is the bot whose POST just errored — so the half-committed write this path detects could surface as PENDING, invisible to everyone and publishing no resolvable thread. Matched against a submitted-state allowlist instead of excluding DISMISSED alone. - A read that inspected nothing may not answer False (BE-4785). Exit-0 empty stdout no longer defaults to `[]`, and a payload that is not a list of pages is UNKNOWN rather than silently flattened to no reviews. - 408/425/429 take the read. The no-read short-circuit assumes a 4xx means "validated and refused before writing", which does not hold for statuses an edge or proxy can return for a request GitHub went on to serve. - The list read is bounded (60s) and degrades to UNKNOWN. It sits ahead of the fallback POST and the summary write, so an unbounded hang lost the round from both channels when `timeout-minutes: 10` fired. The 422 path is unchanged and re-verified byte-for-byte against origin/main, still spending no extra API call. Each fix is mutation-checked: reverting any one of them turns a test red. --- .github/cursor-review/post-review.py | 158 +++++++++++--- .../cursor-review/tests/test_post_review.py | 198 +++++++++++++++++- .github/workflows/cursor-review.yml | 15 +- 3 files changed, 335 insertions(+), 36 deletions(-) diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 142de5be..39665fb8 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -298,6 +298,28 @@ def gh_http_status(result: subprocess.CompletedProcess): return int(match.group(1)) if match else None +# 4xx statuses that are NOT evidence the request was rejected before it was written. +# The no-read short-circuit rests on a 4xx meaning "GitHub validated this and refused +# it", which holds for the rejections it was built for (422 over an inline position, +# and the 4xx family of malformed/unauthorized/absent) but NOT for these: a timeout, +# a rate limit or an early-hint refusal can come from an edge or a proxy in front of +# GitHub, about a request the API went on to serve. Treating one of those as "absent +# by construction" would repost the fallback and tag every anchored finding +# `lost_to_fallback` on an assumption that does not apply, so they take the read like +# a 5xx does. +RETRYABLE_4XX_STATUSES = frozenset({408, 425, 429}) + + +# 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 +# code posted the fallback immediately. Bounded well under that budget so a slow or +# wedged read degrades to the UNKNOWN branch (fallback posted, nothing tagged) instead +# of costing the round entirely. Generous enough that ordinary pagination over a busy +# PR finishes inside it. +GH_LIST_REVIEWS_TIMEOUT_SECONDS = 60 + + def gh_list_reviews(repo: str, pr_number: str) -> subprocess.CompletedProcess: """Every review on the PR, oldest first, ALL pages. @@ -306,22 +328,60 @@ def gh_list_reviews(repo: str, pr_number: str) -> subprocess.CompletedProcess: dup-check (`cursor-review.yml`, the `already_reviewed` step) is the same discriminator without pagination — it can afford that, because it only has to notice a review that already exists before spending the panel, while a wrong - answer here decides whether findings are labelled lost. + answer here decides whether findings are labelled lost. Pagination means this is + one *command* but not necessarily one HTTP request. `--slurp` wraps each page in an outer array (gh >= 2.43; the runner uses a current gh), so the caller flattens one level. + + A timeout is reported as a nonzero CompletedProcess rather than raised, so the + caller reads it through the same "could not tell" branch as any other failed read. """ - return subprocess.run( - [ - "gh", - "api", - "--paginate", - "--slurp", - f"/repos/{repo}/pulls/{pr_number}/reviews", - ], - text=True, - capture_output=True, - ) + argv = [ + "gh", + "api", + "--paginate", + "--slurp", + f"/repos/{repo}/pulls/{pr_number}/reviews", + ] + try: + return subprocess.run( + argv, + text=True, + capture_output=True, + timeout=GH_LIST_REVIEWS_TIMEOUT_SECONDS, + ) + except subprocess.TimeoutExpired: + return subprocess.CompletedProcess( + args=argv, + returncode=124, + stdout="", + stderr=( + f"gh api timed out after {GH_LIST_REVIEWS_TIMEOUT_SECONDS}s listing " + f"reviews for {repo}#{pr_number}" + ), + ) + + +# The states GitHub uses for a review that has actually been SUBMITTED. Matched as an +# allowlist rather than by excluding DISMISSED, because `GET /pulls/{n}/reviews` +# also returns the authenticated identity's own PENDING (unsubmitted) reviews — and +# that identity is the very bot whose POST just errored, so a half-committed write is +# exactly what could appear here as PENDING. A pending review is invisible to everyone +# else and publishes no resolvable thread, so reading one as "landed" would suppress +# the fallback and report `delivered=true` over findings no thread query can find. +SUBMITTED_REVIEW_STATES = frozenset({"COMMENTED", "APPROVED", "CHANGES_REQUESTED"}) + + +def _normalize_review_body(text) -> str: + """`text` with the differences GitHub is known to introduce when it stores a body. + + Line endings (it rewrites CRLF) and trailing whitespace only. Deliberately NOT a + loose normalization: the comparison this feeds is the identity check, so anything + that makes two DIFFERENT review bodies compare equal defeats it. + """ + lines = (text or "").replace("\r\n", "\n").split("\n") + return "\n".join(line.rstrip() for line in lines).strip() def review_already_posted( @@ -335,32 +395,70 @@ def review_already_posted( or came back unparseable). A read that failed is not a zero — see BE-4785 — so the caller degrades rather than claiming the review is missing. - The same four-part discriminator the gate and the workflow use: not DISMISSED, a - Bot author (the poster is caller-configurable, so the TYPE is the only thing this - can know), the run's own head SHA, and the panel marker. Matched on the marker - rather than only on byte-equality with `posted_body` because GitHub may normalize - what it stored (line endings, trailing whitespace); byte-equality is still tried - first since it is the strongest evidence available. + The gate's and the workflow's three-part filter first — a SUBMITTED state, a Bot + author (the poster is caller-configurable, so the TYPE is the only thing this can + know), and the run's own head SHA — and then, unlike them, an identity check on + the BODY: it must be `posted_body`, up to the normalization GitHub applies when it + stores one. + + That last part is what this cannot borrow from the gate. `CONSOLIDATED_MARKER` is + a fine discriminator for the question THEY ask ("does a panel review already exist + at this SHA, so should we spend the panel at all?"), but it is the wrong one here. + A previous round's body-only fallback, and any `post_error_review` body, both open + with the marker, are Bot-authored and carry this same `commit_id` — so accepting a + prefix match would answer "yes, your review landed" on the strength of some OTHER + review entirely. That answer is not a harmless duplicate: the caller returns + without posting the fallback and reports `delivered=true` with + `gated_findings=len(comments)`, so THIS round's findings reach neither the PR nor + the job summary while the blocking gate goes green over threads that belong to a + different round. The pre-change behaviour in that same scenario was a duplicate + review — noisy, but with the findings still visible — so a loose match here would + trade a duplicate for a silent loss. It is reachable, too: the workflow's + `already_reviewed` dup-check fails OPEN on an API error, and two runs can both pass + it before either posts. + + Requiring the body means the residual is now the strictly narrower "a previous + round posted a byte-identical body at the same head SHA", which is the same + findings, in the same order, with the same anchors — a review whose threads do + carry this round's findings. """ result = gh_list_reviews(repo, pr_number) if result.returncode != 0: return None + # An exit-0 read with nothing in it INSPECTED nothing; defaulting it to `[]` would + # launder that into "this PR has no reviews" and tag every finding lost on the + # strength of it. Same rule as the unparseable and wrong-shape cases below + # (BE-4785): only a list this actually read can answer False. + raw = (result.stdout or "").strip() + if not raw: + return None try: - pages = json.loads(result.stdout or "[]") + pages = json.loads(raw) except ValueError: return None - if not isinstance(pages, list): + # `--slurp` promises a list OF PAGES, each itself a list. Anything else — a flat + # array of reviews, a single object, a bare scalar — is a payload shape this does + # not know how to read, so it is UNKNOWN rather than empty. Silently dropping the + # pages that fail the check would turn an unrecognized shape into "no reviews". + if not isinstance(pages, list) or not all(isinstance(page, list) for page in pages): return None - reviews = [r for page in pages for r in (page if isinstance(page, list) else [])] + reviews = [r for page in pages for r in page] for review in reviews: - if not isinstance(review, dict) or review.get("state") == "DISMISSED": + if not isinstance(review, dict): + continue + if review.get("state") not in SUBMITTED_REVIEW_STATES: continue if ((review.get("user") or {}).get("type") or "") != "Bot": continue if review.get("commit_id") != commit_sha: continue + # Cheap prefix reject before the normalization work; the equality below is + # what actually decides. Every body this script posts opens with the marker, + # so this can only skip reviews the identity check would reject anyway. body = review.get("body") or "" - if body == posted_body or body.startswith(CONSOLIDATED_MARKER): + if not body.startswith(CONSOLIDATED_MARKER): + continue + if _normalize_review_body(body) == _normalize_review_body(posted_body): return True return False @@ -1650,8 +1748,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. 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, 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: # 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 @@ -1659,7 +1760,12 @@ def finish_posted_review(): # be indistinguishable from a confirmed-absent review and would relabel findings on # the strength of a transient blip. status = gh_http_status(result) - if status is not None and 400 <= status < 500: + 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: landed = review_already_posted( diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index 1bbfd2b2..e3ebc736 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -201,12 +201,22 @@ def test_a_real_diff_returns_a_map(self): self.assertEqual(PR.load_anchors(p)["util.py"], {5, 6}) +# Stands in, inside an `existing_reviews` fixture, for "the body this run actually +# POSTed" — which a case cannot write out, since main() assembles it from the findings. +# Compared by IDENTITY in the harness, so it can never collide with a real body. +ECHO_POSTED_BODY = "" + +# Distinguishes "the case said nothing about stdout" from "the case asked for empty +# stdout", which is itself one of the behaviours under test. +_UNSET = object() + + class EndToEndPostTest(unittest.TestCase): """Drive main() with a stubbed `gh` and read the payload it would have sent.""" def run_main(self, findings, with_diff=True, post_returncode=0, stderr="", summaries=None, panel=None, existing_reviews=None, list_returncode=0, list_calls=None, - outputs=None, notes=None): + outputs=None, notes=None, raw_stdout=_UNSET, extra_argv=()): """Return the POSTed payloads. Pass `summaries` (a list) to collect step-summary writes, or `panel` to control the panel summary — the one finding-INDEPENDENT part of the review head that a caller can make large. @@ -218,7 +228,14 @@ def run_main(self, findings, with_diff=True, post_returncode=0, stderr="", summa "confirmed absent", which is what every pre-existing case here already assumed. `existing_reviews` is the flat list of review objects the PR carries; it is - wrapped in one `--slurp` page, the shape the real command returns. + wrapped in one `--slurp` page, the shape the real command returns. A review + whose `body` is `ECHO_POSTED_BODY` gets the body this run actually POSTed, + which is the only body the identity check accepts. `raw_stdout` replaces that + whole payload with a literal string, for the cases that test a MALFORMED one. + + `extra_argv` appends to the command line, for the head-shaping options + (`--notice`, `--triggered-by`, `--ledger-note`) a case needs to vary. + `list_calls`, `outputs` and `notes` are optional out-parameters: the calls the list read received, the parsed $GITHUB_OUTPUT, and the `note=` each write_step_summary got. @@ -234,10 +251,20 @@ def fake_post(repo, pr_number, payload): def fake_list(repo, pr_number): if list_calls is not None: list_calls.append((repo, pr_number)) + # A review only counts as THIS run's when its body IS the body this run + # posted (BE-12528), which the case cannot spell out ahead of time — it is + # assembled by main() from the findings. ECHO_POSTED_BODY stands in for it + # and is resolved here, after the POST, from the payload actually sent. + reviews = [] + for review in existing_reviews or []: + if review.get("body") is ECHO_POSTED_BODY: + review = {**review, "body": posted[0]["body"]} + reviews.append(review) + stdout = json.dumps([reviews]) if raw_stdout is _UNSET else raw_stdout return subprocess.CompletedProcess( args=["gh"], returncode=list_returncode, - stdout=json.dumps([existing_reviews or []]), + stdout=stdout, stderr="", ) @@ -273,6 +300,7 @@ def fake_summary(markdown, note=None): with open(dpath, "w", encoding="utf-8") as f: f.write(DIFF) argv += ["--diff", dpath] + argv += list(extra_argv) with mock.patch.object(PR, "gh_post_review", side_effect=fake_post), \ mock.patch.object(PR, "gh_list_reviews", side_effect=fake_list), \ mock.patch.object(PR.sys, "argv", argv), \ @@ -892,11 +920,18 @@ class FirstReviewConfirmationTest(unittest.TestCase): ANCHORED = [finding("app.py", 11), finding("app.py", 12)] def landed_review(self, **overrides): + """The review this run posted, as the PR would carry it back. + + The body is ECHO_POSTED_BODY, not a hand-written body that merely opens with + the marker: since the identity check the marker-prefix match was replaced by, + only the body this run actually POSTed answers True, and a fixture that + asserted otherwise would be asserting against the old behaviour. + """ review = { "state": "COMMENTED", "commit_id": "deadbeef", "user": {"type": "Bot"}, - "body": f"{PR.CONSOLIDATED_MARKER}\n\nFound **2** finding(s).", + "body": ECHO_POSTED_BODY, } review.update(overrides) return review @@ -968,11 +1003,35 @@ def test_a_present_review_by_a_human_or_on_another_commit_does_not_count(self): A human's review, a review of a different head SHA, and a DISMISSED one are all reviews on the PR that are NOT this run's — reading any of them as "it landed" would suppress a fallback the round genuinely needs and lose every finding. + + So are the four the marker-prefix match used to accept. A PENDING review is the + sharpest: `GET /pulls/{n}/reviews` returns the authenticated identity's own + unsubmitted reviews, and that identity is this same bot, so the half-committed + write this path exists to detect is precisely what could show up as PENDING — + invisible to everyone else, publishing no resolvable thread. And a previous + round's fallback body or a `post_error_review` body both OPEN with the marker, + are Bot-authored, and carry this same `commit_id`; accepting either would report + `delivered=true` over another round's threads while this round's findings + reached nowhere at all. """ for label, review in ( ("a human author", self.landed_review(user={"type": "User"})), ("another commit", self.landed_review(commit_id="cafebabe")), ("dismissed", self.landed_review(state="DISMISSED")), + ("pending", self.landed_review(state="PENDING")), + ("a state this does not recognize", self.landed_review(state="")), + ( + "another round's fallback body at the same SHA", + self.landed_review( + body=f"{PR.CONSOLIDATED_MARKER}\n\nFound **9** finding(s)." + ), + ), + ( + "an error review at the same SHA", + self.landed_review( + body=f"{PR.CONSOLIDATED_MARKER}\n\nThe review could not run." + ), + ), ): with self.subTest(review=label): posted = EndToEndPostTest().run_main( @@ -1034,6 +1093,137 @@ def test_the_consolidated_marker_matches_gate_unresolved(self): spec.loader.exec_module(gate_unresolved) self.assertEqual(PR.CONSOLIDATED_MARKER, gate_unresolved.CONSOLIDATED_MARKER) + def test_a_body_github_normalized_still_counts_as_this_runs_review(self): + """The identity check tolerates what GitHub rewrites, and only that. + + CRLF line endings and trailing whitespace are storage artefacts, not a + different review — if they defeated the match, the very case this path exists + for (the write DID land) would repost the duplicate anyway. + """ + body = f"{PR.CONSOLIDATED_MARKER}\n\nFound **2** finding(s).\n" + stored = self.landed_review(body=body.replace("\n", "\r\n") + " \r\n") + self.assertIs( + self.confirm(json.dumps([[stored]]), posted_body=body), True, + "CRLF and trailing whitespace are storage artefacts, not another review", + ) + # …and only that. A body differing by anything a reader would SEE is a + # different review, which is the whole point of matching on the body at all. + for label, other in ( + ("one more finding", body.replace("**2**", "**3**")), + ("an extra paragraph", body + "\nAlso: something else.\n"), + ): + with self.subTest(differs_by=label): + self.assertIs( + self.confirm( + json.dumps([[self.landed_review(body=other)]]), posted_body=body + ), + False, + ) + + def test_a_retryable_4xx_is_not_treated_as_a_pre_write_rejection(self): + """408/429 can come from an edge or a proxy about a request GitHub SERVED. + + The no-read short-circuit is sound only for a status that means "validated and + refused before writing", so these take the read like a 5xx — and when it says + the review landed, no duplicate is posted and nothing is tagged lost. + """ + for status, label in ((408, "Request Timeout"), (429, "Too Many Requests")): + with self.subTest(status=status): + calls = [] + posted = EndToEndPostTest().run_main( + self.ANCHORED, + post_returncode=1, + stderr=f"gh: {label} (HTTP {status})", + existing_reviews=[self.landed_review()], + list_calls=calls, + ) + self.assertEqual(calls, [("o/r", "1")], "this status must be read, not assumed") + self.assertEqual(len(posted), 1, "the review landed — no duplicate") + + def test_a_degenerate_review_list_payload_is_unknown_not_empty(self): + """A read that inspected NOTHING must not answer "confirmed absent". + + Exit 0 with empty stdout, a flat array, a bare object and a scalar are all + payloads this cannot read; defaulting any of them to `[]` would apply + `lost_to_fallback` to every anchored finding on the strength of a read that + established nothing (BE-4785). + """ + for label, raw in ( + ("empty stdout", ""), + ("whitespace only", " \n"), + ("a flat array of reviews", json.dumps([self.landed_review(body="x")])), + ("a single object", json.dumps({"state": "COMMENTED"})), + ("a bare scalar", json.dumps(7)), + ): + with self.subTest(payload=label): + self.assertIsNone( + self.confirm(raw), f"{label} must read as UNKNOWN" + ) + + def test_a_well_formed_empty_page_is_still_a_confirmed_absence(self): + """The degenerate-shape guard must not swallow the real answer: `[[]]` is what + `--slurp` returns for a PR with no reviews, and that IS "confirmed absent".""" + self.assertIs(self.confirm(json.dumps([[]])), False) + self.assertIs(self.confirm(json.dumps([])), False, "no pages at all") + + def test_the_review_list_read_is_bounded_and_a_timeout_reads_as_unknown(self): + """It sits ahead of the fallback POST and the summary write, so an unbounded + hang would take the round out of both channels when the job timer fires. The + timeout comes back as a nonzero result, i.e. through the UNKNOWN branch.""" + self.assertLess( + PR.GH_LIST_REVIEWS_TIMEOUT_SECONDS, 10 * 60, + "must be well under the job's timeout-minutes: 10", + ) + with mock.patch.object( + PR.subprocess, "run", + side_effect=subprocess.TimeoutExpired(cmd=["gh"], timeout=PR.GH_LIST_REVIEWS_TIMEOUT_SECONDS), + ) as run: + result = PR.gh_list_reviews("o/r", "1") + self.assertEqual( + run.call_args.kwargs.get("timeout"), PR.GH_LIST_REVIEWS_TIMEOUT_SECONDS + ) + self.assertNotEqual(result.returncode, 0, "a timeout is not a successful read") + with mock.patch.object(PR, "gh_list_reviews", return_value=result): + self.assertIsNone(PR.review_already_posted("o/r", "1", "deadbeef", "body")) + + def test_every_head_variant_still_opens_with_the_marker(self): + """The cheap prefix reject ahead of the identity check assumes it. + + `--notice` and `--ledger-note` APPEND to the header rather than prepend, and + the trigger attribution follows the title — so every body this script posts + opens with CONSOLIDATED_MARKER. If one ever stopped doing so, the prefix reject + would skip the run's OWN review and answer "absent" for a review that landed, + which is the bug this whole path exists to fix. Pinned rather than assumed. + """ + driver = EndToEndPostTest() + posted = driver.run_main( + self.ANCHORED, + existing_reviews=[], + extra_argv=[ + "--triggered-by", "someone", + "--notice", "The judge failed; these are raw panel findings.", + "--ledger-note", "Round 2 — ledger: 3 prior findings.", + ], + ) + self.assertTrue(posted[0]["body"].startswith(PR.CONSOLIDATED_MARKER)) + # …and end to end: that same decorated body is recognized as this run's. + self.assertIs( + self.confirm( + json.dumps([[self.landed_review(body=posted[0]["body"])]]), + posted_body=posted[0]["body"], + ), + True, + ) + + def confirm(self, raw_stdout, posted_body="body"): + """`review_already_posted` over a literal `gh` stdout, so the answer is the + payload's doing and nothing else's.""" + result = subprocess.CompletedProcess( + args=["gh"], returncode=0, stdout=raw_stdout, stderr="" + ) + with mock.patch.object(PR, "gh_list_reviews", return_value=result): + return PR.review_already_posted("o/r", "1", "deadbeef", posted_body) + def test_gh_http_status_parses_gh_stderr(self): def status(text): return PR.gh_http_status( diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 5a92f396..d52afff6 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -2140,12 +2140,15 @@ jobs: runs-on: ubuntu-latest # Three artifact downloads, a token mint, and post-review.py's ONE # `gh api POST /pulls/N/reviews` carrying every inline comment in a single - # payload — plus, on a non-4xx POST failure only, at most one paginated GET of - # the PR's reviews, to tell a review that never landed from one that landed - # despite the error (`pull-requests: write` already implies that read, so no - # permission changes here) — minutes of work at most. Bounded like every other - # job here so a rate-limited or hung call cannot hold a runner for the 6-hour - # default. + # payload — plus, on a POST failure that is not a pre-write rejection, at most + # one paginated read of the PR's reviews (one `gh` command, but one request PER + # PAGE), to tell a review that never landed from one that landed despite the + # error (`pull-requests: write` already implies that read, so no permission + # changes here) — minutes of work at most. That read carries its own + # GH_LIST_REVIEWS_TIMEOUT_SECONDS well under this budget, so a wedged list cannot + # eat the job's whole allowance and take the fallback POST down with it. Bounded + # like every other job here so a rate-limited or hung call cannot hold a runner + # for the 6-hour default. timeout-minutes: 10 permissions: contents: read From fbcbb7f1546856d0e3d63ba9771e206edeee7018 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 8 Sep 2026 13:39:54 -0700 Subject: [PATCH 3/3] fix(cursor-review): close the round-2 panel findings on the landed-review check (BE-12528) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `[]` no longer answers "confirmed absent". `all()` is vacuously true over it, so a zero-page payload fell through to `reviews = []` and tagged every anchored finding lost on a read that inspected no page — the same laundering the empty-stdout guard rejects. `[[]]`, what `--slurp` really returns for a PR with no reviews, remains the genuine absence. - The cheap prefix reject now runs on the NORMALIZED body. It ran on the raw one while the equality it guards normalized both sides, making the guard stricter than the check: a stored body differing only by a leading newline passed the equality but never reached it, answering "absent" for the run's own landed review. - Field types are trusted no further than payload shapes. A `user` that is a string, or a non-string `body`, raised AttributeError out of `review_already_posted` and killed the process ahead of both the fallback POST and `write_step_summary` — the both-channel loss the timeout exists to prevent. - A failed list read logs its exit code and stderr. UNKNOWN reposts the fallback and withholds the tag without explanation, so a 60s timeout, an auth failure and an older `gh` without `--slurp` were indistinguishable to an operator. - Comment correction: 403 is absent from RETRYABLE_4XX_STATUSES because `is_read_only_token_error` returns from `main()` before this decision is reached, not because a throttled 403 cannot happen. Listing it would be dead code that reads as coverage. Each fix is mutation-checked; reverting any one turns a test red. --- .github/cursor-review/post-review.py | 68 +++++++++++++++---- .../cursor-review/tests/test_post_review.py | 58 +++++++++++++++- 2 files changed, 110 insertions(+), 16 deletions(-) diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 39665fb8..8eef9234 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -302,11 +302,18 @@ def gh_http_status(result: subprocess.CompletedProcess): # The no-read short-circuit rests on a 4xx meaning "GitHub validated this and refused # it", which holds for the rejections it was built for (422 over an inline position, # and the 4xx family of malformed/unauthorized/absent) but NOT for these: a timeout, -# a rate limit or an early-hint refusal can come from an edge or a proxy in front of -# GitHub, about a request the API went on to serve. Treating one of those as "absent -# by construction" would repost the fallback and tag every anchored finding +# a secondary rate limit or an early-hint refusal can come from an edge or a proxy in +# front of GitHub, about a request the API went on to serve. Treating one of those as +# "absent by construction" would repost the fallback and tag every anchored finding # `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}) @@ -424,6 +431,15 @@ def review_already_posted( """ result = gh_list_reviews(repo, pr_number) if result.returncode != 0: + # The UNKNOWN branch withholds `lost_to_fallback` and reposts the fallback + # without saying why, so the reason has to be logged HERE or it exists nowhere: + # a 60s timeout, an auth failure and an older `gh` without `--slurp` are + # indistinguishable to an operator otherwise. + print( + f"Review: could not list reviews for {repo}#{pr_number} " + f"(exit {result.returncode}): {(result.stderr or '').strip()[:300]}", + file=sys.stderr, + ) return None # An exit-0 read with nothing in it INSPECTED nothing; defaulting it to `[]` would # launder that into "this PR has no reviews" and tag every finding lost on the @@ -436,11 +452,21 @@ def review_already_posted( pages = json.loads(raw) except ValueError: return None - # `--slurp` promises a list OF PAGES, each itself a list. Anything else — a flat - # array of reviews, a single object, a bare scalar — is a payload shape this does - # not know how to read, so it is UNKNOWN rather than empty. Silently dropping the - # pages that fail the check would turn an unrecognized shape into "no reviews". - if not isinstance(pages, list) or not all(isinstance(page, list) for page in pages): + # `--slurp` promises a NON-EMPTY list OF PAGES, each itself a list. Anything else — + # a flat array of reviews, a single object, a bare scalar — is a payload shape this + # does not know how to read, so it is UNKNOWN rather than empty. Silently dropping + # the pages that fail the check would turn an unrecognized shape into "no reviews". + # + # `[]` is in that set, and deliberately: `all()` is vacuously true over it, so it + # would otherwise fall through to `reviews = []` and answer "confirmed absent" — + # the same laundering the empty-stdout guard above rejects, on a read that + # inspected no page at all. `[[]]` is what --slurp really returns for a PR with no + # reviews, and that is the genuine absence. + if ( + not isinstance(pages, list) + or not pages + or not all(isinstance(page, list) for page in pages) + ): return None reviews = [r for page in pages for r in page] for review in reviews: @@ -448,17 +474,29 @@ def review_already_posted( continue if review.get("state") not in SUBMITTED_REVIEW_STATES: continue - if ((review.get("user") or {}).get("type") or "") != "Bot": + # Types are trusted no further than shapes were: this runs on a payload the + # process cannot re-fetch, and an AttributeError here escapes `main()` and + # kills it ahead of BOTH the fallback POST and write_step_summary — the same + # both-channel loss the timeout above exists to prevent. + user = review.get("user") + if not isinstance(user, dict) or user.get("type") != "Bot": continue if review.get("commit_id") != commit_sha: continue - # Cheap prefix reject before the normalization work; the equality below is - # what actually decides. Every body this script posts opens with the marker, - # so this can only skip reviews the identity check would reject anyway. - body = review.get("body") or "" - if not body.startswith(CONSOLIDATED_MARKER): + # Cheap prefix reject before the equality; every body this script posts opens + # with the marker, so it can only skip reviews the identity check would reject + # anyway. Applied to the NORMALIZED body, not the raw one, or it would be + # STRICTER than the check it guards: `_normalize_review_body` strips leading + # whitespace, so a stored body differing only by a leading newline would pass + # the equality yet never reach it — answering "absent" for the run's own landed + # review, which is precisely this path's worst outcome. + body = review.get("body") + if not isinstance(body, str): + continue + normalized = _normalize_review_body(body) + if not normalized.startswith(CONSOLIDATED_MARKER): continue - if _normalize_review_body(body) == _normalize_review_body(posted_body): + if normalized == _normalize_review_body(posted_body): return True return False diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index e3ebc736..480fb700 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -1164,7 +1164,63 @@ def test_a_well_formed_empty_page_is_still_a_confirmed_absence(self): """The degenerate-shape guard must not swallow the real answer: `[[]]` is what `--slurp` returns for a PR with no reviews, and that IS "confirmed absent".""" self.assertIs(self.confirm(json.dumps([[]])), False) - self.assertIs(self.confirm(json.dumps([])), False, "no pages at all") + self.assertIs( + self.confirm(json.dumps([[], []])), False, "several empty pages" + ) + + def test_a_zero_page_payload_is_unknown_not_absent(self): + """`[]` is not `[[]]`. `all()` is vacuously true over it, so without an + explicit non-empty check it falls through to "no reviews" and tags every + anchored finding lost on a read that inspected no PAGE at all — the same + laundering the empty-stdout guard rejects.""" + self.assertIsNone(self.confirm(json.dumps([]))) + + def test_a_review_with_hostile_field_types_does_not_kill_the_process(self): + """Types are trusted no further than shapes. An AttributeError here escapes + main() and kills it ahead of BOTH the fallback POST and the summary write.""" + for label, review in ( + ("user is a string", self.landed_review(user="ghost")), + ("user is null", self.landed_review(user=None)), + ("body is a number", self.landed_review(body=7)), + ("body is null", self.landed_review(body=None)), + ): + with self.subTest(review=label): + self.assertIs(self.confirm(json.dumps([[review]])), False) + + def test_the_prefix_reject_is_never_stricter_than_the_equality(self): + """The cheap reject runs on the NORMALIZED body, so it cannot skip a review the + identity check would have accepted. A raw `startswith` could: normalization + strips leading whitespace, so a stored body differing only by a leading newline + would pass the equality and never reach it — answering "absent" for the run's + own landed review, this path's worst outcome.""" + body = f"{PR.CONSOLIDATED_MARKER}\n\nFound **2** finding(s)." + for label, stored in ( + ("a leading newline", "\n" + body), + ("leading spaces", " " + body), + ("both ends", "\n " + body + " \n"), + ): + with self.subTest(stored=label): + self.assertIs( + self.confirm( + json.dumps([[self.landed_review(body=stored)]]), + posted_body=body, + ), + True, + ) + + def test_a_failed_review_list_read_logs_why(self): + """UNKNOWN reposts the fallback and withholds the tag without saying why, so + the reason has to be logged here or it exists in no channel at all.""" + result = subprocess.CompletedProcess( + args=["gh"], returncode=124, stdout="", + stderr="gh api timed out after 60s listing reviews for o/r#1", + ) + err = io.StringIO() + with mock.patch.object(PR, "gh_list_reviews", return_value=result), \ + contextlib.redirect_stderr(err): + self.assertIsNone(PR.review_already_posted("o/r", "1", "deadbeef", "b")) + self.assertIn("timed out after 60s", err.getvalue()) + self.assertIn("exit 124", err.getvalue()) def test_the_review_list_read_is_bounded_and_a_timeout_reads_as_unknown(self): """It sits ahead of the fallback POST and the summary write, so an unbounded