fix(review-at-head): a hung gh call times out as a failed API call

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).

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

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"<!-- protoagent-qa-review head={MERGED} verdict={verdict} promoted=false round={n} -->"
+        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