diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 1b2648a3..8eef9234 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -278,6 +278,229 @@ 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 + + +# 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 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}) + + +# 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. + + 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. 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. + """ + 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( + 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 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: + # 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 + # 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(raw) + except ValueError: + return None + # `--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: + if not isinstance(review, dict): + continue + if review.get("state") not in SUBMITTED_REVIEW_STATES: + continue + # 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 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 normalized == _normalize_review_body(posted_body): + 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 +1717,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 +1742,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 +1778,54 @@ 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 β€” 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 + # none). UNKNOWN is why the read failing is not answered as a `False`: that would + # be indistinguishable from a confirmed-absent review and would relabel findings on + # the strength of a transient blip. + status = gh_http_status(result) + pre_write_rejection = ( + status is not None + and 400 <= status < 500 + and status not in RETRYABLE_4XX_STATUSES + ) + if pre_write_rejection: + landed = False + else: + 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 +1857,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 +1880,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..480fb700 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -201,14 +201,45 @@ 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): + panel=None, existing_reviews=None, list_returncode=0, list_calls=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.""" + 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. 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. + """ posted = [] def fake_post(repo, pr_number, payload): @@ -217,10 +248,36 @@ 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)) + # 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=stdout, + 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 +287,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, @@ -242,13 +300,23 @@ 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), \ + 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 +898,401 @@ 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): + """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": ECHO_POSTED_BODY, + } + 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. + + 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( + 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_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, "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 + 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( + 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..d52afff6 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -2140,8 +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 β€” 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