From aa72381108bf06adc7816137535c114ea95e10d4 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 18 Sep 2026 22:48:08 +0000 Subject: [PATCH 1/2] fix(cursor-review): keep the -i status line and rate-limit headers when a review POST fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failed review POST reported result.stderr alone — gh's error line — and threw away the stdout that -i exists to capture: the response status line and the Retry-After / X-RateLimit-* headers. When both the primary and inline-less fallback POST fail with `unexpected end of JSON input`, stderr alone cannot tell a request GitHub never received from a response whose empty error body failed to decode, so the one artifact that would say which was discarded at the moment it was needed. Add format_post_failure(): it keeps the existing stderr line and appends the stdout status line (or an explicit "no reply reached gh" when none arrived), any rate-limit headers, and a json.loads self-check of the request body that pins the decode error as response-side vs request-side. Both POST-failure sites (the primary review POST and post_or_degrade, which the fallback uses) now route through it. Headers are read via gh_response_headers, which stops at the blank line, so a finding body that quotes a Retry-After header can never forge one. --- .github/cursor-review/post-review.py | 62 ++++++++++- .../cursor-review/tests/test_post_review.py | 103 ++++++++++++++++++ 2 files changed, 163 insertions(+), 2 deletions(-) diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 8d82c41d..5ba9bae8 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -784,6 +784,64 @@ def gh_error_line(result: subprocess.CompletedProcess) -> str: return blob[start:] if end == -1 else blob[start:end] +def gh_status_line(result: subprocess.CompletedProcess) -> str: + """The `HTTP/x.y nnn Reason` line `gh -i` wrote to STDOUT, or "". + + The one artifact that says whether the request reached GitHub at all: `gh -i` + renders the response's status line first thing on stdout, so its PRESENCE means a + response arrived (however GitHub then answered) and its ABSENCE means the write + never got a reply — a transport error, a timeout, a stub. Empty for anything that + is not a status line, gated by the same `_GH_STATUS_LINE_RE` the header parser + uses, so a bare JSON body's first line is never mistaken for one. + """ + first = (result.stdout or "").split("\n", 1)[0].rstrip("\r") + return first if _GH_STATUS_LINE_RE.match(first) else "" + + +# The response headers worth surfacing beside a POST failure: they say whether the +# failure was a throttle and how long a retry must wait. Lower-cased to match +# `gh_response_headers`, which canonicalizes to Go's casing. +_POST_FAILURE_HEADERS = ("retry-after", "x-ratelimit-remaining", "x-ratelimit-reset") + + +def format_post_failure( + context: str, result: subprocess.CompletedProcess, payload: str | None = None +) -> str: + """The ` POST failed:` diagnostic, keeping what `gh -i` put on STDOUT + (BE-15634). + + stderr carries only `gh`'s error line; the status line and the response headers + that say whether GitHub was even reached — and, on a throttle, how long to wait — + go to stdout, and reporting stderr alone threw them away at the one moment they + are needed. `unexpected end of JSON input` is the case that motivated this: on + stderr alone it cannot be told apart from a request GitHub never received, but the + stdout status line settles it. When `payload` is given, a `json.loads` self-check + settles the OTHER half — whether the body we sent was well-formed JSON — so the + same error can be attributed to the response decode rather than our request. + + Reads headers through `gh_response_headers`, which stops at the blank line, so a + finding body that quotes a `Retry-After:` header can never reach this diagnostic. + """ + parts = [f"{context} POST failed: {result.stderr}"] + status_line = gh_status_line(result) + parts.append( + f" response status: {status_line}" + if status_line + else " response status: none captured on stdout (no reply reached gh)" + ) + headers = gh_response_headers(result) + surfaced = [f"{name}: {headers[name]}" for name in _POST_FAILURE_HEADERS if name in headers] + if surfaced: + parts.append(" response headers: " + "; ".join(surfaced)) + if payload is not None: + try: + json.loads(payload) + parts.append(" request body: valid JSON — the decode error is response-side") + except (ValueError, TypeError) as exc: + parts.append(f" request body: INVALID JSON ({exc}) — the decode error is request-side") + return "\n".join(parts) + + # 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, @@ -1237,7 +1295,7 @@ def report_posted(): emit_delivery(False) write_step_summary(summary_markdown) return True - print(f"{context} POST failed: {result.stderr}", file=sys.stderr) + print(format_post_failure(context, result, payload), file=sys.stderr) # A nonzero `gh` is not proof the write was refused. Once the throttle wordings # stopped being read as a read-only token (BE-12612), the 403 GitHub raises on a # request it went on to SERVE reaches here — and every caller answers a False by @@ -2928,7 +2986,7 @@ def finish_posted_review(): write_step_summary(prose_body) return - print(f"Review POST failed: {result.stderr}", file=sys.stderr) + print(format_post_failure("Review", result, payload), file=sys.stderr) if not comments: # There is no inline half to drop, so a fallback POST would carry the same # findings as the request that just failed (only the demotion intro and the diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index 908e2255..7a3b1c96 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -1472,6 +1472,30 @@ def test_a_transport_error_with_no_http_status_goes_through_the_read(self): # Once for the inline POST, once for the fallback the stub fails identically. self.assertEqual(calls, [("o/r", "1"), ("o/r", "1")]) + def test_a_status_less_failure_still_lands_the_findings_in_the_summary(self): + """The BE-15634 scenario: both POSTs fail with a status-less transport error + (`gh: unexpected end of JSON input`), the read confirms the review is absent, + and the run must NOT drop the findings. They reach the job summary — the run + artifact is no longer their only surviving copy — under the POST-failed note, + and the step goes red.""" + outputs, summaries, notes, calls = {}, [], [], [] + driver = EndToEndPostTest() + driver.run_main( + self.ANCHORED, + post_returncode=1, + stderr="gh: unexpected end of JSON input", + existing_reviews=[], + list_calls=calls, + outputs=outputs, + summaries=summaries, + notes=notes, + ) + self.assertEqual(outputs["delivered"], "false") + self.assertEqual(len(summaries), 1, "the findings reached the job summary") + self.assertIn("finding on app.py:11", summaries[0], "and the findings are in it") + self.assertIn(PR.POST_FAILED_SUMMARY_NOTE, notes) + self.assertEqual(driver.exit_code, 1, "the step goes red") + 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 @@ -2312,6 +2336,85 @@ def test_a_header_block_with_no_body_still_parses(self): self.assertEqual(headers, {"retry-after": "60"}) +class PostFailureDiagnosticTest(unittest.TestCase): + """What a ` POST failed:` line carries (BE-15634). + + The failure that motivated this reported stderr alone — `gh: unexpected end of + JSON input` — and discarded the stdout `-i` captured, which is the one place the + status line and rate-limit headers live. Two identical failures 1.4s apart could + not then be told apart from a request GitHub never received. These pin that the + status line, the throttle headers, and the request-body self-check all reach the + log. + """ + + # `gh` fails an empty error body exactly this way, and it is the case where the + # status line on stdout is the only thing that says a reply arrived at all. + EMPTY_JSON_STDERR = "gh: unexpected end of JSON input" + + def test_the_stdout_status_line_is_surfaced(self): + line = PR.format_post_failure( + "Review", + gh_result(stdout=GH_INCLUDE_STDOUT, stderr=self.EMPTY_JSON_STDERR), + ) + self.assertIn(self.EMPTY_JSON_STDERR, line) + self.assertIn("response status: HTTP/2.0 404 Not Found", line) + + def test_rate_limit_headers_are_surfaced_when_present(self): + throttled_stdout = response( + "Retry-After: 42", + "X-Ratelimit-Remaining: 0", + status="HTTP/2.0 403 Forbidden", + ) + line = PR.format_post_failure( + "Fallback review", + gh_result( + stdout=throttled_stdout, + stderr="gh: You have exceeded a secondary rate limit. (HTTP 403)", + ), + ) + self.assertIn("retry-after: 42", line) + self.assertIn("x-ratelimit-remaining: 0", line) + + def test_no_reply_reached_gh_is_stated_not_left_blank(self): + """A transport error / timeout writes no status line to stdout. Saying so + explicitly is the point — it distinguishes 'GitHub rejected us' from 'the + write never got a reply', which is exactly what the old stderr-only line + could not do.""" + line = PR.format_post_failure( + "Review", gh_result(stdout="", stderr="gh: dial tcp: i/o timeout") + ) + self.assertIn("no reply reached gh", line) + + def test_a_well_formed_payload_points_the_finger_at_the_response(self): + line = PR.format_post_failure( + "Review", + gh_result(stderr=self.EMPTY_JSON_STDERR), + payload=json.dumps({"body": "x", "event": "COMMENT"}), + ) + self.assertIn("request body: valid JSON", line) + self.assertIn("response-side", line) + + def test_a_malformed_payload_points_the_finger_at_the_request(self): + line = PR.format_post_failure( + "Review", gh_result(stderr=self.EMPTY_JSON_STDERR), payload="{not json" + ) + self.assertIn("request body: INVALID JSON", line) + self.assertIn("request-side", line) + + def test_a_finding_that_quotes_a_retry_after_cannot_forge_a_header(self): + """The response body is past the blank line, so `gh_response_headers` never + reads it — a finding body echoed into the JSON response cannot dictate what + this diagnostic reports as a throttle window.""" + spoof = response( + "Retry-After: 1", + status="HTTP/2.0 422 Unprocessable Entity", + body='{"message":"Retry-After: 3600 X-Ratelimit-Remaining: 0"}', + ) + line = PR.format_post_failure("Review", gh_result(stdout=spoof)) + self.assertIn("retry-after: 1", line) + self.assertNotIn("3600", line) + + class ThrottleDelayTest(unittest.TestCase): """How long a throttled write waits, and why it is never longer than that. From 465bd25d47d9e74b7cb032d46a517c1275157d10 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 18 Sep 2026 23:01:08 +0000 Subject: [PATCH 2/2] fix(cursor-review): decode the timeout's partial stdout, and stop the diagnostic overclaiming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel review of the BE-15634 diagnostic found three ways it could mislead — or crash — on the exact paths it exists to serve. 1. (High) `TimeoutExpired.stdout` is the RAW BYTES the pipe had buffered even under `text=True`: CPython decodes only on the normal-completion path. `gh_post_review`'s timeout handler stored it verbatim, so a POST that timed out AFTER `gh` wrote anything handed a bytes-bearing CompletedProcess to `gh_status_line` and `gh_response_headers`, whose `.split("\n")` raises TypeError. Because format_post_failure now runs on EVERY POST failure, that traceback would skip the landed-review read, the fallback POST and the job-summary write — losing the findings from both channels on the path built to preserve them. Verified on CPython 3.12: the bytes and the TypeError both reproduce. Decoded once at the source via `_as_text` (errors="replace", since a kill can sever a multi-byte character), and the two stdout readers go through it too so no future construction site can reintroduce the crash. 2. (Low) "no reply reached gh" asserted more than an absent status line proves. `_GH_STATUS_LINE_RE`'s own comment names the counter-cases — a `gh` predating `-i`, or one invoked without it, writes a bare JSON body — and a timeout can kill `gh` after GitHub served the write. On the timeout path it contradicted the stderr beside it, which says the outcome is unknown; a maintainer who believed it would re-trigger and produce the duplicate review the landed-review read exists to avoid. Now reports "no status line captured on stdout" and cross-checks `gh_http_status`, so a status on stderr says plainly that a reply arrived and only a genuine silence reads as UNKNOWN. 3. (Low) "the decode error is response-side" fired unconditionally: both call sites pass a `json.dumps` result, so the valid-JSON branch always wins and announced a decode error on 403 throttles, 422 rejections and 500s that involve no decode at all. Gated on `gh` actually reporting one of Go's `encoding/json` wordings; absent that it reports what it checked and stops. Tests: both bytes-capture regressions fail without the source decode, plus the stderr cross-check, the UNKNOWN wording, and the no-decode-error attribution. Co-Authored-By: Claude Opus 5 --- .github/cursor-review/post-review.py | 112 ++++++++++++++---- .../cursor-review/tests/test_post_review.py | 87 ++++++++++++-- 2 files changed, 172 insertions(+), 27 deletions(-) diff --git a/.github/cursor-review/post-review.py b/.github/cursor-review/post-review.py index 5ba9bae8..05c701b9 100644 --- a/.github/cursor-review/post-review.py +++ b/.github/cursor-review/post-review.py @@ -314,6 +314,22 @@ def neutralize_mentions(text: str) -> str: GH_POST_REVIEW_TIMEOUT_SECONDS = 60 +def _as_text(captured) -> str: + """A captured stdout/stderr as `str`, whatever `subprocess` handed back. + + `subprocess.run(..., text=True)` decodes only on the NORMAL-completion path: when + the child is killed on a timeout, `TimeoutExpired.stdout` is the raw BYTES read off + the pipe before the kill. A `CompletedProcess` built from one therefore carries + bytes where every reader here expects str, and `"".split` on it raises `TypeError` + — which, on a path whose whole job is to report a failure, would replace the + diagnostic with a traceback and lose the findings it was protecting. So the decode + happens once, here, and the str readers go through it too. + """ + if isinstance(captured, (bytes, bytearray)): + return bytes(captured).decode("utf-8", errors="replace") + return captured or "" + + def gh_post_review(repo: str, pr_number: str, payload: str) -> subprocess.CompletedProcess: """POST the review, keeping the RESPONSE HEADERS (BE-12691). @@ -353,11 +369,15 @@ def gh_post_review(repo: str, pr_number: str, payload: str) -> subprocess.Comple return subprocess.CompletedProcess( args=argv, returncode=124, - # Whatever `gh` had written before the kill. Headers are only readable once - # the status line has arrived, and `gh_response_headers` returns {} short - # of that, so a partial capture degrades to "no headers" rather than to a - # wrong `Retry-After`. - stdout=exc.stdout or "", + # Whatever `gh` had written before the kill, DECODED: `TimeoutExpired` + # carries the raw BYTES the pipe had buffered even under `text=True` — + # CPython decodes only on the normal-completion path — so handing + # `exc.stdout` straight through would make every str reader of this result + # (`gh_status_line`, `gh_response_headers`) raise `TypeError` the moment a + # timeout killed `gh` after it wrote anything. `errors="replace"` because a + # kill can sever a multi-byte character mid-sequence, and a diagnostic must + # never be the thing that raises. + stdout=_as_text(exc.stdout), stderr=( f"gh api timed out after {GH_POST_REVIEW_TIMEOUT_SECONDS}s posting the " f"review to {repo}#{pr_number} — whether the write was served is " @@ -387,7 +407,7 @@ def gh_response_headers(result: subprocess.CompletedProcess) -> dict[str, str]: must not have to guess which spelling arrived. A repeated header keeps its LAST value, which is what an HTTP client would use. """ - blob = result.stdout or "" + blob = _as_text(result.stdout) lines = blob.split("\n") if not _GH_STATUS_LINE_RE.match(lines[0].rstrip("\r")): return {} @@ -787,14 +807,18 @@ def gh_error_line(result: subprocess.CompletedProcess) -> str: def gh_status_line(result: subprocess.CompletedProcess) -> str: """The `HTTP/x.y nnn Reason` line `gh -i` wrote to STDOUT, or "". - The one artifact that says whether the request reached GitHub at all: `gh -i` - renders the response's status line first thing on stdout, so its PRESENCE means a - response arrived (however GitHub then answered) and its ABSENCE means the write - never got a reply — a transport error, a timeout, a stub. Empty for anything that - is not a status line, gated by the same `_GH_STATUS_LINE_RE` the header parser - uses, so a bare JSON body's first line is never mistaken for one. + `gh -i` renders the response's status line first thing on stdout, so its PRESENCE + is proof a response arrived, however GitHub then answered. Its ABSENCE proves only + that none was CAPTURED, and is not evidence the write went unserved: a `gh` + predating `-i` or invoked without it writes a bare JSON body, a stub writes + whatever it likes, and a timeout can kill `gh` after GitHub already served the + write. Read it one way only — asserting "no reply reached GitHub" from an empty + return would invite the duplicate review the landed-review read exists to avoid. + Empty for anything that is not a status line, gated by the same + `_GH_STATUS_LINE_RE` the header parser uses, so a bare JSON body's first line is + never mistaken for one. """ - first = (result.stdout or "").split("\n", 1)[0].rstrip("\r") + first = _as_text(result.stdout).split("\n", 1)[0].rstrip("\r") return first if _GH_STATUS_LINE_RE.match(first) else "" @@ -804,6 +828,23 @@ def gh_status_line(result: subprocess.CompletedProcess) -> str: _POST_FAILURE_HEADERS = ("retry-after", "x-ratelimit-remaining", "x-ratelimit-reset") +# Go's `encoding/json` wordings `gh` surfaces when it cannot parse a JSON document — +# the ONLY failures for which "request-side or response-side?" is even a question. A +# 403 throttle, a 422 anchor rejection and a 500 involve no decode at all, so on those +# the request-body self-check may report what it checked and nothing more. +_JSON_DECODE_WORDINGS = ( + "unexpected end of json input", + "invalid character", + "cannot unmarshal", +) + + +def _looks_like_json_decode_failure(result: subprocess.CompletedProcess) -> bool: + """Whether `gh` failed DECODING JSON, rather than merely reporting an HTTP error.""" + blob = (result.stderr or "").lower() + return any(wording in blob for wording in _JSON_DECODE_WORDINGS) + + def format_post_failure( context: str, result: subprocess.CompletedProcess, payload: str | None = None ) -> str: @@ -819,26 +860,57 @@ def format_post_failure( settles the OTHER half — whether the body we sent was well-formed JSON — so the same error can be attributed to the response decode rather than our request. + Every claim here is bounded by what was actually observed: a missing status line is + reported as "not captured" and cross-checked against the stderr status rather than + asserted to mean nothing was served, and the request-body check attributes a decode + error only when `gh` reported one. + Reads headers through `gh_response_headers`, which stops at the blank line, so a finding body that quotes a `Retry-After:` header can never reach this diagnostic. """ parts = [f"{context} POST failed: {result.stderr}"] status_line = gh_status_line(result) - parts.append( - f" response status: {status_line}" - if status_line - else " response status: none captured on stdout (no reply reached gh)" - ) + if status_line: + parts.append(f" response status: {status_line}") + else: + # An absent status line is NOT proof nothing was served — see `gh_status_line`. + # `gh_http_status` reads the `(HTTP nnn)` gh renders on stderr, and a status + # there settles it from the other side, so the two are reported together + # rather than the stdout half alone being read as a verdict. + reported = gh_http_status(result) + parts.append( + f" response status: no status line captured on stdout, but gh reported " + f"HTTP {reported} on stderr — a reply DID arrive" + if reported is not None + else " response status: no status line captured on stdout and no HTTP " + "status on stderr — whether the write was served is UNKNOWN; ask the PR, " + "do not re-trigger blind" + ) headers = gh_response_headers(result) surfaced = [f"{name}: {headers[name]}" for name in _POST_FAILURE_HEADERS if name in headers] if surfaced: parts.append(" response headers: " + "; ".join(surfaced)) if payload is not None: + # Attribution is claimed ONLY when `gh` actually failed to decode JSON. Both + # call sites hand over a `json.dumps` result, so the valid branch always wins; + # printing "the decode error is response-side" unconditionally would announce a + # decode error on every 403, 422 and 500 — misdirecting the triage this exists + # to serve. Absent a decode failure it reports what it checked, and stops. + decode_failure = _looks_like_json_decode_failure(result) try: json.loads(payload) - parts.append(" request body: valid JSON — the decode error is response-side") except (ValueError, TypeError) as exc: - parts.append(f" request body: INVALID JSON ({exc}) — the decode error is request-side") + parts.append( + f" request body: INVALID JSON ({exc}) — the decode error is request-side" + if decode_failure + else f" request body: INVALID JSON ({exc})" + ) + else: + parts.append( + " request body: valid JSON — so the decode error is response-side" + if decode_failure + else " request body: valid JSON" + ) return "\n".join(parts) diff --git a/.github/cursor-review/tests/test_post_review.py b/.github/cursor-review/tests/test_post_review.py index 7a3b1c96..3e7ade21 100644 --- a/.github/cursor-review/tests/test_post_review.py +++ b/.github/cursor-review/tests/test_post_review.py @@ -2375,15 +2375,50 @@ def test_rate_limit_headers_are_surfaced_when_present(self): self.assertIn("retry-after: 42", line) self.assertIn("x-ratelimit-remaining: 0", line) - def test_no_reply_reached_gh_is_stated_not_left_blank(self): - """A transport error / timeout writes no status line to stdout. Saying so - explicitly is the point — it distinguishes 'GitHub rejected us' from 'the - write never got a reply', which is exactly what the old stderr-only line - could not do.""" + def test_a_missing_status_line_is_reported_as_not_captured_not_as_no_reply(self): + """Saying the status line is absent is the point — it distinguishes 'GitHub + rejected us' from 'we cannot tell', which the old stderr-only line could not + do. But absence is NOT proof nothing was served: a `gh` without `-i` and a + timeout that killed `gh` after GitHub served the write both land here, so the + outcome is reported as UNKNOWN rather than asserted to be a non-delivery a + maintainer would answer by re-triggering into a duplicate review.""" line = PR.format_post_failure( "Review", gh_result(stdout="", stderr="gh: dial tcp: i/o timeout") ) - self.assertIn("no reply reached gh", line) + self.assertIn("no status line captured on stdout", line) + self.assertIn("UNKNOWN", line) + self.assertNotIn( + "no reply reached gh", line, "absence of a status line proves no such thing" + ) + + def test_a_stderr_status_is_cross_checked_when_stdout_carried_no_status_line(self): + """`gh` invoked without `-i` — or any stub — writes a bare body, so stdout has + no status line while stderr's `(HTTP nnn)` says plainly that GitHub answered. + Reporting the stdout half alone would call that a non-delivery.""" + line = PR.format_post_failure( + "Review", + gh_result( + stdout='{"message":"Unprocessable Entity"}', + stderr="gh: Unprocessable Entity (HTTP 422)", + ), + ) + self.assertIn("no status line captured on stdout", line) + self.assertIn("HTTP 422", line) + self.assertIn("a reply DID arrive", line) + self.assertNotIn("UNKNOWN", line) + + def test_a_failure_with_no_decode_error_claims_no_decode_attribution(self): + """A 403 throttle, a 422 rejection and a 500 involve no JSON decode at all. + Both call sites always hand over a `json.dumps` result, so an unconditional + "the decode error is response-side" would fire on EVERY failure and announce a + decode error that never happened — misdirecting the triage this exists for.""" + line = PR.format_post_failure( + "Review", + gh_result(stderr="gh: You have exceeded a secondary rate limit. (HTTP 403)"), + payload=json.dumps({"body": "x", "event": "COMMENT"}), + ) + self.assertIn("request body: valid JSON", line) + self.assertNotIn("decode error", line) def test_a_well_formed_payload_points_the_finger_at_the_response(self): line = PR.format_post_failure( @@ -2392,7 +2427,10 @@ def test_a_well_formed_payload_points_the_finger_at_the_response(self): payload=json.dumps({"body": "x", "event": "COMMENT"}), ) self.assertIn("request body: valid JSON", line) - self.assertIn("response-side", line) + self.assertIn( + "response-side", line, + "gh DID report a decode failure here, so the attribution is earned", + ) def test_a_malformed_payload_points_the_finger_at_the_request(self): line = PR.format_post_failure( @@ -2904,6 +2942,41 @@ def test_a_timeout_becomes_an_undecided_result_not_an_exception(self): self.assertNotEqual(result.returncode, 0) self.assertIn("timed out", result.stderr) + def test_a_timeout_that_captured_output_decodes_it_instead_of_carrying_bytes(self): + """`TimeoutExpired.stdout` is the RAW BYTES the pipe had buffered even under + `text=True` — CPython decodes only when the child exits normally. Carrying that + through would hand a bytes-bearing `CompletedProcess` to str readers, and since + `format_post_failure` now runs on EVERY POST failure, the resulting `TypeError` + would skip the landed-review read, the fallback POST and the job-summary write + — losing the findings on the very path built to preserve them. + """ + partial = b"HTTP/2.0 403 Forbidden\r\nRetry-After: 60\r\n\r\n" + with mock.patch.object( + PR.subprocess, "run", + side_effect=subprocess.TimeoutExpired(cmd=["gh"], timeout=60, output=partial), + ): + result = PR.gh_post_review("o/r", "1", "{}") + self.assertIsInstance(result.stdout, str, "decoded at the source, once") + self.assertEqual(PR.gh_status_line(result), "HTTP/2.0 403 Forbidden") + self.assertEqual(PR.gh_response_headers(result).get("retry-after"), "60") + # The whole point: the diagnostic renders rather than raising. + line = PR.format_post_failure("Review", result, payload="{}") + self.assertIn("HTTP/2.0 403 Forbidden", line) + self.assertIn("retry-after: 60", line) + + def test_a_severed_multibyte_capture_degrades_rather_than_raising(self): + """A kill can land mid-character. A diagnostic must never be the thing that + raises, so the decode replaces rather than strict-errors.""" + with mock.patch.object( + PR.subprocess, "run", + side_effect=subprocess.TimeoutExpired( + cmd=["gh"], timeout=60, output=b"HTTP/2.0 502 Bad Gateway\r\n\r\n\xe2\x82", + ), + ): + result = PR.gh_post_review("o/r", "1", "{}") + self.assertIsInstance(result.stdout, str) + self.assertIn("502", PR.format_post_failure("Review", result)) + def test_a_timed_out_post_takes_the_undecided_path(self): """It carries no HTTP status, so it is neither a throttle nor a read-only token — it is genuinely undecided, and the PR gets asked."""