fix(cursor-review): keep the -i status line and headers when a review POST fails - #323
mattmillerai wants to merge 2 commits into
Conversation
…en a review POST fails 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.
mattmillerai
added
cursor-review
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 21 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 21 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 47 minutes for your next included review. Limit details: You’ve used the included review currently available. Your 138 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughThe POST review flow now reports response status, rate-limit headers, stderr, and request JSON validity when posting fails. Both posting paths use the diagnostic. Tests cover response parsing, header spoofing, status-less failures, job summaries, delivery state, and exit status. ChangesPOST Failure Diagnostics
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The POST failure diagnostics retain the intended status and rate-limit information, with no identified merge-blocking regression. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 3 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟢 Low | 2 |
Panel: 6/6 reviewers contributed findings.
… diagnostic overclaiming
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 <noreply@anthropic.com>
2 participants
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 crypticunexpected 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.pyformat_post_failure(context, result, payload)builds the<context> POST failed:log line and now also includes:gh -iwrote (HTTP/2.0 nnn …), or an explicitresponse status: no status line captured on stdout …when none arrived — the distinction the old stderr-only line could not express;Retry-After/X-RateLimit-Remaining/X-RateLimit-Resetresponse headers when present;json.loadsself-check of the request body, which settles whether anunexpected end of JSON inputis request-side (our payload) or response-side (GitHub's empty error body) in one line.post_or_degrade(used by the inline-less fallback) — the two POSTs that failed identically in the reported run.gh_response_headers, which stops at the blank line before the response body, so a finding body that quotes aRetry-After:header can never forge one into the diagnostic (covered by a test).Tests:
+7cases (plus+5from 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 runsunittest+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:
TimeoutExpired.stdoutis the raw bytes the pipe had buffered even undertext=True— CPython decodes only on the normal-completion path.gh_post_review's timeout handler stored it verbatim, so a POST that timed out afterghwrote anything handed a bytes-bearingCompletedProcesstogh_status_line/gh_response_headers, whose.split("\n")raisesTypeError. Sinceformat_post_failurenow 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 builtTimeoutExpiredwith no output, which is why this was uncovered.no reply reached ghasserted more than an absent status line proves._GH_STATUS_LINE_RE's own comment names the counter-cases — aghpredating-i, or one invoked without it, writes a bare JSON body — and a timeout can killghafter 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 reportsno status line captured on stdoutand cross-checksgh_http_status, so a status on stderr says plainly that a reply arrived and only a genuine silence reads asUNKNOWN.the decode error is response-sidefired unconditionally. Both call sites pass ajson.dumpsresult, 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 onghactually reporting one of Go'sencoding/jsonwordings; absent that it states what it checked and stops.Acceptance / artifacts exercised
post-review.pygh_post_review docstring + the stderr-only failure print — read and fixed; the-icontract is that "the status line + headers + body all go to STDOUT", which was being discarded.unexpected end of JSON inputon 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-consolidatedartifact, 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
Post reviewthat recordeddelivered=falseshould 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.emit_delivery(False)sites are paired with awrite_step_summary, so nodelivered=falsepath 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
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.shclean (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.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.