[PR #323] [MERGED] fix(cursor-review): keep the -i status line and headers when a review POST fails #325

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-15634-cursor-review-post-failure-diagnostic

📝 Commits (2)

aa72381 fix(cursor-review): keep the -i status line and rate-limit headers when a review POST fails
465bd25 fix(cursor-review): decode the timeout's partial stdout, and stop the diagnostic overclaiming

📊 Changes

2 files changed (+314 additions, -8 deletions)

View changed files

📝 .github/cursor-review/post-review.py (+138 -8)
📝 .github/cursor-review/tests/test_post_review.py (+176 -0)

📄 Description

ELI-5

When our review bot tries to post its findings to a PR and the post fails, it used to print only the error message and throw away the receipt — the HTTP status line and the rate-limit headers that GitHub's CLI (gh -i) had already captured. So when a real 6-model panel's findings failed to post twice with the cryptic unexpected end of JSON input, nobody could tell from the log whether the request even reached GitHub. This change keeps the receipt in the failure message, so the next failure is diagnosable instead of a mystery.

What changed

.github/cursor-review/post-review.py

New format_post_failure(context, result, payload) builds the <context> POST failed: log line and now also includes:
- the stdout status line gh -i wrote (HTTP/2.0 nnn …), or an explicit response status: no status line captured on stdout … when none arrived — the distinction the old stderr-only line could not express;
- the Retry-After / X-RateLimit-Remaining / X-RateLimit-Reset response headers when present;
- a json.loads self-check of the request body, which settles whether an unexpected end of JSON input is request-side (our payload) or response-side (GitHub's empty error body) in one line.
Both POST-failure sites now route through it: the primary review POST, and post_or_degrade (used by the inline-less fallback) — the two POSTs that failed identically in the reported run.
Headers are read via the existing gh_response_headers, which stops at the blank line before the response body, so a finding body that quotes a Retry-After: header can never forge one into the diagnostic (covered by a test).

Tests: +7 cases (plus +5 from the review pass below) (PostFailureDiagnosticTest, plus a status-less transport-error regression test that reproduces the reported scenario end-to-end and asserts the findings still reach the job summary). Full suite: 607 passed. Lint parity: CI runs unittest + shellcheck; no shell touched.

Review pass (panel findings, commit 2)

The 6-model panel raised three ways the new diagnostic could mislead — or crash — on the very paths it exists to serve. All three were valid and are fixed:

(High) The timeout path could throw away the findings entirely. 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 / gh_response_headers, whose .split("\n") raises TypeError. Since format_post_failure now runs on every POST failure, that traceback would have skipped the landed-review read, the fallback POST and the job-summary write — losing the findings on the path built to preserve them. Reproduced on CPython 3.12 before fixing. Decoded once at the source (_as_text, errors="replace", since a kill can sever a multi-byte character); the two stdout readers go through it as well so a future construction site can't reintroduce it. The pre-existing timeout tests built TimeoutExpired with no output, which is why this was uncovered.
(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, and a maintainer who believed it would re-trigger into 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.
(Low) the decode error is response-side fired unconditionally. Both call sites pass a json.dumps result, so the valid-JSON branch always won — announcing a decode error on 403 throttles, 422 rejections and 500s that involve no decode at all. Now gated on gh actually reporting one of Go's encoding/json wordings; absent that it states what it checked and stops.

Acceptance / artifacts exercised

post-review.py gh_post_review docstring + the stderr-only failure print — read and fixed; the -i contract is that "the status line + headers + body all go to STDOUT", which was being discarded.
The reported symptom unexpected end of JSON input on both the primary and fallback POST — reproduced as a unit + end-to-end test (test_a_status_less_failure_still_lands_the_findings_in_the_summary, PostFailureDiagnosticTest).

Artifacts named in the ticket that I could not exercise (no access from this environment; named per policy): the CI run and its cursor-review-consolidated artifact, the caller PR, and the two check-run ids / head SHA cited for the gate-hole — all live in a caller repo / expired artifact storage. No secrets or private detail from them are reproduced here.

Residual

Item 2 (the merge-gate hole) is deliberately NOT implemented here — the ticket itself scopes it out: "Item 2 needs a call on skip semantics before anyone codes it… Decision needed (not agent-scoped)." The open question is whether a Post review that recorded delivered=false should be sticky (re-emitted as failure by any later run at the same SHA, or recorded where the branch-protection rollup — which takes the latest check run per name — cannot supersede it), so a later SKIPPED run can't erase the red check and let a PR merge with undelivered findings. This needs a human decision on skip semantics and is left for follow-up.
Item 3 (write consolidated findings to the job summary on delivery failure) required no production code change — it is already satisfied on the current default branch by the throttle/landed work that landed after the reported 2026‑09‑15 run. I swept every delivery-failure exit: all 7 emit_delivery(False) sites are paired with a write_step_summary, so no delivered=false path drops the findings. Rather than a redundant edit, I added a regression test pinning the ticket's exact status-less transport-error scenario so this can't silently regress. If reviewers consider the ticket's item 3 to require an explicit unconditional write at a single choke point (rather than the current per-path pairing), that is a refactor worth its own discussion.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' → 611 passed; shellcheck -x .github/cursor-review/install-cursor-cli.sh .github/cursor-review/slack-notify.sh clean (CI parity for this path; no shell changed). Repo-wide lints also green: check_workflow_pins.py, check_agents_md.py --root ., check-org-repo-literals.sh. The two bytes-capture regressions were confirmed to fail with the source decode reverted, so they are not vacuous.
Deviations: none. The commit-1 message and the original body text both described the status-absence wording as no reply reached gh; commit 2 narrows that claim, and this body is updated to match — the commit-1 message itself cannot be amended without a force-push.

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

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/323 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/18/2026 **Status:** ✅ Merged **Merged:** 9/19/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15634-cursor-review-post-failure-diagnostic` --- ### 📝 Commits (2) - [`aa72381`](https://github.com/Comfy-Org/github-workflows/commit/aa72381108bf06adc7816137535c114ea95e10d4) fix(cursor-review): keep the -i status line and rate-limit headers when a review POST fails - [`465bd25`](https://github.com/Comfy-Org/github-workflows/commit/465bd25d47d9e74b7cb032d46a517c1275157d10) fix(cursor-review): decode the timeout's partial stdout, and stop the diagnostic overclaiming ### 📊 Changes **2 files changed** (+314 additions, -8 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/post-review.py` (+138 -8) 📝 `.github/cursor-review/tests/test_post_review.py` (+176 -0) </details> ### 📄 Description ## ELI-5 When our review bot tries to post its findings to a PR and the post fails, it used to print only the *error message* and throw away the *receipt* — the HTTP status line and the rate-limit headers that GitHub's CLI (`gh -i`) had already captured. So when a real 6-model panel's findings failed to post twice with the cryptic `unexpected end of JSON input`, nobody could tell from the log whether the request even reached GitHub. This change keeps the receipt in the failure message, so the next failure is diagnosable instead of a mystery. ## What changed `.github/cursor-review/post-review.py` - New `format_post_failure(context, result, payload)` builds the `<context> POST failed:` log line and now also includes: - the **stdout status line** `gh -i` wrote (`HTTP/2.0 nnn …`), or an explicit `response status: no status line captured on stdout …` when none arrived — the distinction the old stderr-only line could not express; - the **`Retry-After` / `X-RateLimit-Remaining` / `X-RateLimit-Reset`** response headers when present; - a **`json.loads` self-check of the request body**, which settles whether an `unexpected end of JSON input` is request-side (our payload) or response-side (GitHub's empty error body) in one line. - Both POST-failure sites now route through it: the primary review POST, and `post_or_degrade` (used by the inline-less fallback) — the two POSTs that failed identically in the reported run. - Headers are read via the existing `gh_response_headers`, which stops at the blank line before the response body, so a finding body that quotes a `Retry-After:` header can never forge one into the diagnostic (covered by a test). Tests: `+7` cases (plus `+5` from the review pass below) (`PostFailureDiagnosticTest`, plus a status-less transport-error regression test that reproduces the reported scenario end-to-end and asserts the findings still reach the job summary). Full suite: `607 passed`. Lint parity: CI runs `unittest` + `shellcheck`; no shell touched. ## Review pass (panel findings, commit 2) The 6-model panel raised three ways the new diagnostic could mislead — or crash — on the very paths it exists to serve. All three were valid and are fixed: - **(High) The timeout path could throw away the findings entirely.** `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` / `gh_response_headers`, whose `.split("\n")` raises `TypeError`. Since `format_post_failure` now runs on **every** POST failure, that traceback would have skipped the landed-review read, the fallback POST and the job-summary write — losing the findings on the path built to preserve them. Reproduced on CPython 3.12 before fixing. Decoded once at the source (`_as_text`, `errors="replace"`, since a kill can sever a multi-byte character); the two stdout readers go through it as well so a future construction site can't reintroduce it. The pre-existing timeout tests built `TimeoutExpired` with no output, which is why this was uncovered. - **(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, and a maintainer who believed it would re-trigger into 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`. - **(Low) `the decode error is response-side` fired unconditionally.** Both call sites pass a `json.dumps` result, so the valid-JSON branch always won — announcing a decode error on 403 throttles, 422 rejections and 500s that involve no decode at all. Now gated on `gh` actually reporting one of Go's `encoding/json` wordings; absent that it states what it checked and stops. ## Acceptance / artifacts exercised - `post-review.py` gh_post_review docstring + the stderr-only failure print — **read and fixed**; the `-i` contract is that "the status line + headers + body all go to STDOUT", which was being discarded. - The reported symptom `unexpected end of JSON input` on both the primary and fallback POST — **reproduced** as a unit + end-to-end test (`test_a_status_less_failure_still_lands_the_findings_in_the_summary`, `PostFailureDiagnosticTest`). Artifacts named in the ticket that I could **not** exercise (no access from this environment; named per policy): the CI run and its `cursor-review-consolidated` artifact, the caller PR, and the two check-run ids / head SHA cited for the gate-hole — all live in a caller repo / expired artifact storage. No secrets or private detail from them are reproduced here. ## Residual - **Item 2 (the merge-gate hole) is deliberately NOT implemented here** — the ticket itself scopes it out: *"Item 2 needs a call on skip semantics before anyone codes it… Decision needed (not agent-scoped)."* The open question is whether a `Post review` that recorded `delivered=false` should be **sticky** (re-emitted as failure by any later run at the same SHA, or recorded where the branch-protection rollup — which takes the latest check run per name — cannot supersede it), so a later SKIPPED run can't erase the red check and let a PR merge with undelivered findings. This needs a human decision on skip semantics and is left for follow-up. - **Item 3 (write consolidated findings to the job summary on delivery failure) required no production code change** — it is already satisfied on the current default branch by the throttle/landed work that landed after the reported 2026‑09‑15 run. I swept every delivery-failure exit: **all 7** `emit_delivery(False)` sites are paired with a `write_step_summary`, so no `delivered=false` path drops the findings. Rather than a redundant edit, I added a regression test pinning the ticket's exact status-less transport-error scenario so this can't silently regress. If reviewers consider the ticket's item 3 to require an explicit unconditional write at a single choke point (rather than the current per-path pairing), that is a refactor worth its own discussion. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'` → **611 passed**; `shellcheck -x .github/cursor-review/install-cursor-cli.sh .github/cursor-review/slack-notify.sh` clean (CI parity for this path; no shell changed). Repo-wide lints also green: `check_workflow_pins.py`, `check_agents_md.py --root .`, `check-org-repo-literals.sh`. The two bytes-capture regressions were confirmed to **fail** with the source decode reverted, so they are not vacuous. - **Deviations:** none. The commit-1 message and the original body text both described the status-absence wording as `no reply reached gh`; commit 2 narrows that claim, and this body is updated to match — the commit-1 message itself cannot be amended without a force-push. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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