fix(review-at-head): a hung gh call times out as a failed API call - #4023
Conversation
…he strictest-round test proves which round spoke `_gh` had no subprocess timeout, so a hung `gh` held the required job until the Actions job timeout. It now passes timeout=120s, and TimeoutExpired raises the same RuntimeError as a failed call, which both modes already handle fail-closed. Outside the vendored block. The strictest-round test now asserts the FAIL came from the earlier round, not a later one. Part of protoLabsAI/pr-reviewer-plugin#254 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughGitHub CLI calls now have a 120-second timeout. Timed-out calls are reported as failures, and a timed-out reviews read does not post a status. Tests also check verdict precedence across review rounds. ChangesGitHub CLI timeout
Review-round selection test
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~8 minutes Sequence Diagram(s)sequenceDiagram
participant main
participant _gh
participant subprocess.run
main->>_gh: Read reviews
_gh->>subprocess.run: Run gh with a 120-second timeout
subprocess.run-->>_gh: Raise TimeoutExpired
_gh-->>main: Raise RuntimeError
main->>main: Report timeout and exit zero without posting status
Merge Risk: ⚪ Minimal · up to GitHub CLI calls now have a 120-second limit, and timed-out review reads do not publish a status. No actionable merge-blocking risk is established by the supplied change context. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![protoreview[bot]](https://avatars.githubusercontent.com/in/4233969?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA panel review — PASS
code-review · head 9e890531b831 · formal
PR #4023 came back clean across all four review angles — no defects were identified by the panel, and the verifier confirmed there was nothing to verify. No blockers, majors, minors, or nits to report. The change is safe to merge from a code-review standpoint.
No findings — the review came back clean.
findings JSON (machine-readable)
[]
There was a problem hiding this comment.
QA panel review — PASS
code-review · head 9e890531b831 · formal
PR #4023 is clean: all four review lanes (correctness, cross-file, style, and tool-sourced) returned no findings, and the verifier confirmed there is nothing to re-check. No defects, no disagreements, no coverage gaps. Nothing to fix before merge.
No findings — the review came back clean.
findings JSON (machine-readable)
[]
1 participant
Part of protoLabsAI/pr-reviewer-plugin#254 (the plugin side is protoLabsAI/pr-reviewer-plugin#256).
scripts/review_at_head.py's_ghcalledsubprocess.runwith no timeout, so a hungghheld the requiredReview at headjob until the Actions job timeout, and the status it should have posted never came._ghnow passestimeout=GH_TIMEOUT_S(120s), and aTimeoutExpiredis re-raised as the sameRuntimeErrora failed call raises. Both modes already handle that fail-closed ("a broken API call must not be mistaken for no verdict"): single-PR mode posts no status from a partial read and exits 0, and the sweep moves on to the next PR. The change is outside the vendored supersede block, so the block's hash is unchanged and this is a plain edit, the same as in the plugin's copy.It also strengthens
test_the_STRICTEST_round_for_a_head_wins_not_the_latest(item 3). The rounds are tagged, and the test asserts that the earlier round 1 FAIL is the one picked over round 2's later PASS, plus the exact failure description. It also asserts that the newest of equally strict rounds speaks, and that an earlier WARN beats a later PASS. Before, it only checkednot okand "FAIL" in the description, which a wrong pick could also satisfy.Scope: the script, its test, and the
changelog.dfragment.Tests: a hung
ghraises a timeoutRuntimeError, and the timeout is actually passed tosubprocess.run. A timed-out reviews read posts no status and still exits 0. Locally:pytest tests/test_review_at_head.pypasses (55 tests), andruff==0.15.10 checkis clean on both files.🤖 Generated with Claude Code
Summary by CodeRabbit