diff --git a/changelog.d/4023.fixed.md b/changelog.d/4023.fixed.md new file mode 100644 index 000000000..8208c0b50 --- /dev/null +++ b/changelog.d/4023.fixed.md @@ -0,0 +1 @@ +- **A hung `gh` call can no longer stall the required `Review at head` check (#4023).** `scripts/review_at_head.py` ran `gh` with no timeout, so a hung call held the job until the Actions job timeout and no status was posted. Each call is now bounded at 120 seconds, and a timeout is treated like any failed API call: the job posts no status from a partial read and exits cleanly, as it already did for API errors (pr-reviewer-plugin#254). diff --git a/scripts/review_at_head.py b/scripts/review_at_head.py index eacf62219..364ec6395 100644 --- a/scripts/review_at_head.py +++ b/scripts/review_at_head.py @@ -453,10 +453,21 @@ def decide( # ── I/O ─────────────────────────────────────────────────────────────────────── +# Seconds one `gh` call may take (pr-reviewer-plugin#254). Without a bound, a hung `gh` held +# the required job until the Actions job timeout, and the status it should have posted never +# came. A paginated reviews read finishes in seconds, so this is generous. +GH_TIMEOUT_S = 120 + + def _gh(*args: str) -> str: """`gh` with the ambient token. Raises on failure — a broken API call must not be - mistaken for "no verdict" and silently fail a PR that was in fact reviewed.""" - result = subprocess.run(["gh", *args], capture_output=True, text=True) + mistaken for "no verdict" and silently fail a PR that was in fact reviewed. A call that + outlives `GH_TIMEOUT_S` is a failed call too: it raises the same `RuntimeError`, which + every caller already handles fail-closed (no status posted from a partial read).""" + try: + result = subprocess.run(["gh", *args], capture_output=True, text=True, timeout=GH_TIMEOUT_S) + except subprocess.TimeoutExpired as exc: + raise RuntimeError(f"gh {' '.join(args)} timed out after {GH_TIMEOUT_S}s") from exc if result.returncode != 0: raise RuntimeError(f"gh {' '.join(args)} failed: {result.stderr.strip()}") return result.stdout diff --git a/tests/test_review_at_head.py b/tests/test_review_at_head.py index 61fac0f92..8bfa26347 100644 --- a/tests/test_review_at_head.py +++ b/tests/test_review_at_head.py @@ -122,9 +122,23 @@ def test_a_promotion_speaks_only_for_a_head_with_no_round(): def test_the_STRICTEST_round_for_a_head_wins_not_the_latest(): # pr-reviewer-plugin#239: after a FAIL, a re-review PASS on the same head used to turn this - # check green while the plugin's `QA panel` (strictest per head) stayed red. - decision = rah.decide([review(MERGED, "FAIL"), review(MERGED, "PASS")], MERGED, []) - assert not decision.ok and "FAIL" in decision.description + # check green while the plugin's `QA panel` (strictest per head) stayed red. Each round is + # tagged so the test proves WHICH round spoke, not only that something said FAIL + # (pr-reviewer-plugin#254). + def tagged(verdict, n): + marker = f"" + return review(MERGED, body=f"{marker}\n## QA panel review\n") + + reviews = [tagged("FAIL", 1), tagged("PASS", 2)] + assert rah.parse_marker(reviews[-1]["body"])["verdict"] == "PASS" # what "latest" read + pick = rah.verdict_for_head(reviews, MERGED) + assert (pick["verdict"], pick["round"]) == ("FAIL", "1") # the EARLIER round speaks + decision = rah.decide(reviews, MERGED, []) + assert not decision.ok and decision.description == f"QA panel returned FAIL for {MERGED[:12]}" + # Among equally strict rounds the newest speaks; a stricter earlier one beats a later one. + assert rah.verdict_for_head([tagged("FAIL", 1), tagged("PASS", 2), tagged("FAIL", 3)], MERGED)["round"] == "3" + pick = rah.verdict_for_head([tagged("WARN", 1), tagged("PASS", 2)], MERGED) + assert (pick["verdict"], pick["round"]) == ("WARN", "1") # ── what must NOT count as a verdict ─────────────────────────────────────────── @@ -573,3 +587,37 @@ def test_the_vendored_supersede_block_is_unedited(): block = text[begin:end] recorded = re.search(r"block-ast-sha256:\s*(\S+)", block).group(1) assert hashlib.sha256(_canonical(ast.parse(block).body).encode()).hexdigest() == recorded + + +# ── a hung `gh` is a failed call, not a hung gate (pr-reviewer-plugin#254) ───── + + +def test_a_hung_gh_call_times_out_as_a_failed_api_call(monkeypatch): + seen = {} + + def hung(cmd, **kwargs): + seen.update(kwargs) + raise rah.subprocess.TimeoutExpired(cmd, kwargs.get("timeout")) + + monkeypatch.setattr(rah.subprocess, "run", hung) + with pytest.raises(RuntimeError, match="timed out"): + rah._gh("api", "repos/o/r/pulls/1/reviews") + assert seen["timeout"] == rah.GH_TIMEOUT_S > 0 + + +def test_a_timed_out_reviews_read_posts_no_status_and_still_exits_zero(monkeypatch, capsys): + calls = [] + + def hung(cmd, **kwargs): + calls.append(cmd) + raise rah.subprocess.TimeoutExpired(cmd, kwargs.get("timeout")) + + monkeypatch.setattr(rah.subprocess, "run", hung) + monkeypatch.setenv("PR_NUMBER", "123") + monkeypatch.setenv("HEAD_SHA", "a" * 40) + monkeypatch.setenv("PR_LABELS", "") + monkeypatch.delenv("DRY_RUN", raising=False) + assert rah.main() == 0 + # Fail closed: the read failed, so no verdict was decided and no status was written. + assert len(calls) == 1 and "statuses" not in " ".join(calls[0]) + assert "timed out" in capsys.readouterr().err