review_runner: surface real crash cause instead of "(see pod log)" (#55)

## Problem

A UI review of `huggingface/peft#3395` on `zai-org/GLM-5.2` failed twice
with only:

```
LLMResponseError: review crashed (see pod log)
```

…and the pod log was **unrecoverable**. Two compounding issues:

1. **`review_runner.py` discarded the detail.** Its catch-all reported
the generic `"{type}: review crashed (see pod log)"`, throwing away
`str(exc)` — which for `LLMResponseError` already carries the LLM
endpoint's status + body excerpt. (`task_runner.py`, the write-capable
path, already handled this correctly; the read-only review path had
regressed.)
2. **`(see pod log)` is a dead reference.** On the self-reported-error
path the Job watcher sees `status=error` from the callback and reaps the
runner pod immediately — it never waits for `ttlSecondsAfterFinished`,
and skips the `collect_task_result` log-tail capture the *crash* path
uses. The pod (and its traceback) is gone within seconds.

Net effect: an upstream HF-router failure for GLM-5.2 left the operator
with no legible cause.

## Fix

Bring `review_runner.run()` to parity with `task_runner`:

- `except LLMResponseError` → `format_llm_error(exc)` (status + body
excerpt: 429/400/auth become legible).
- `except requests.HTTPError` → `format_github_http_error(exc)`.
- catch-all now embeds `crash_detail(exc)` (cause + crashing frame)
instead of `(see pod log)`.
- `crash_detail` moves to `errors.py` (its shared home); `task_runner`
imports it — one copy for both entrypoints.

## Also

`make format`/`test` built a **Python 3.9** venv because bare `python3`
is 3.9 on stock macOS, violating `requires-python >=3.10` (and stalling
dependency resolution). The Makefile now selects the newest available
≥3.10 interpreter and hard-fails with a clear message otherwise. CI
(3.12) unaffected.

## Tests

`tests/test_review_runner.py`: strengthened the crash test (asserts the
cause travels, never `"see pod log"`) and added one proving a 429's
status + body land on the job. Full suite: **398 passed**, ruff clean.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: tarekziade

Makefile

@@ -1,11 +1,18 @@
 .PHONY: format style test
 
-PYTHON ?= python3
+# reviewbot needs Python >= 3.10 (pyproject requires-python). Bare `python3` is
+# 3.9 on stock macOS, which silently builds a broken venv — so prefer an
+# explicit 3.1x if present, and hard-fail below if the resolved one is too old.
+PYTHON ?= $(shell command -v python3.12 || command -v python3.11 || command -v python3.10 || command -v python3)
 VENV ?= .venv
 VENV_PYTHON := $(VENV)/bin/python
 RUFF := $(VENV)/bin/ruff
 
 $(VENV)/.installed: pyproject.toml
+	@$(PYTHON) -c 'import sys; sys.exit(sys.version_info[:2] < (3, 10))' || { \
+	  echo "Error: $(PYTHON) is $$($(PYTHON) -V 2>&1); reviewbot needs Python >= 3.10."; \
+	  echo "Install it (e.g. 'brew install python@3.10') or run 'make PYTHON=python3.10 ...'."; \
+	  exit 1; }
 	$(PYTHON) -m venv $(VENV)
 	$(VENV_PYTHON) -m pip install --upgrade pip
 	$(VENV_PYTHON) -m pip install -e '.[web]' pytest ruff

reviewbot/errors.py

@@ -11,11 +11,27 @@
 
 from __future__ import annotations
 
+import os
+
 import requests
 
 from .llm_client import LLMResponseError
 
 
+def crash_detail(exc: BaseException) -> str:
+    """A one-line cause plus the crashing frame — enough to see *why* an
+    exception fired in the job's ``error`` without needing the (already-reaped)
+    pod log. Used by both runner entrypoints for their catch-all handlers."""
+    frame = ""
+    tb = exc.__traceback__
+    while tb is not None:  # walk to the innermost frame
+        code = tb.tb_frame.f_code
+        frame = f"{os.path.basename(code.co_filename)}:{tb.tb_lineno} in {code.co_name}"
+        tb = tb.tb_next
+    detail = f"{type(exc).__name__}: {exc}".strip()
+    return f"{detail} (at {frame})" if frame else detail
+
+
 def format_llm_error(exc: LLMResponseError) -> str:
     """Render an LLMResponseError for the SSE client. Surfaces the status
     code + a body excerpt so the UI shows whether it was a 429 (rate

reviewbot/review_runner.py

@@ -21,8 +21,12 @@
 import time
 from typing import Optional
 
+import requests
+
 from .clone_cache import Checkout, CloneCache
+from .errors import crash_detail, format_github_http_error, format_llm_error
 from .github_client import GitHubClient
+from .llm_client import LLMResponseError
 from .reviewer import ReviewRequest, _UnparseableLLMOutput, prepare_review
 from .store import encode_draft
 from .task_runner import CallbackEmitter, RunnerSpec, build_runner_config
@@ -106,11 +110,22 @@ def emit(kind: str, text: str) -> None:
             raw_llm_output=getattr(exc, "content", None),
         )
         return 1
+    except LLMResponseError as exc:
+        # The runner pod is reaped right after it reports, so "(see pod log)"
+        # is a dead reference. Surface the LLM endpoint's own status + body
+        # excerpt (429 rate-limit, 400 bad schema, auth, …) on the job itself.
+        log.warning(
+            "LLM endpoint returned %d for review %s", exc.status_code, spec.job_id
+        )
+        emitter.finish("error", error=format_llm_error(exc))
+        return 1
+    except requests.HTTPError as exc:
+        log.warning("review %s GitHub API error: %s", spec.job_id, exc)
+        emitter.finish("error", error=format_github_http_error(exc))
+        return 1
     except Exception as exc:  # noqa: BLE001 — always report a terminal outcome
         log.exception("review runner crashed for job %s", spec.job_id)
-        emitter.finish(
-            "error", error=f"{type(exc).__name__}: review crashed (see pod log)"
-        )
+        emitter.finish("error", error=f"review crashed: {crash_detail(exc)}")
         return 1
     finally:
         if checkout is not None:

reviewbot/task_runner.py

@@ -40,7 +40,7 @@
 from . import sandbox
 from .clone_cache import Checkout, CloneCache
 from .config import Config
-from .errors import format_github_http_error, format_llm_error
+from .errors import crash_detail, format_github_http_error, format_llm_error
 from .github_client import GitHubClient
 from .llm_client import LLMResponseError
 from .reviewer import _UnparseableLLMOutput
@@ -375,7 +375,7 @@ def emit(kind: str, text: str) -> None:
         return _fail(emitter, format_github_http_error(exc))
     except Exception as exc:  # noqa: BLE001
         log.exception("task runner crashed for job %s", spec.job_id)
-        return _fail(emitter, f"task runner crashed: {_crash_detail(exc)}")
+        return _fail(emitter, f"task runner crashed: {crash_detail(exc)}")
     finally:
         if clone_cache is not None:
             clone_cache.release(checkout)
@@ -391,19 +391,6 @@ def _fail(
     return 1
 
 
-def _crash_detail(exc: BaseException) -> str:
-    """A one-line cause plus the crashing frame — enough to see *why* a startup
-    exception fired in the job's ``error`` without needing the pod log."""
-    frame = ""
-    tb = exc.__traceback__
-    while tb is not None:  # walk to the innermost frame
-        code = tb.tb_frame.f_code
-        frame = f"{os.path.basename(code.co_filename)}:{tb.tb_lineno} in {code.co_name}"
-        tb = tb.tb_next
-    detail = f"{type(exc).__name__}: {exc}".strip()
-    return f"{detail} (at {frame})" if frame else detail
-
-
 # ---------------------------------------------------------------------------
 # Entrypoint
 # ---------------------------------------------------------------------------
@@ -446,7 +433,7 @@ def main(argv: Optional[list[str]] = None) -> int:
         try:
             CallbackEmitter(
                 spec.callback.get("url"), spec.callback.get("token"), spec.job_id
-            ).finish("error", error=f"task runner crashed: {_crash_detail(exc)}")
+            ).finish("error", error=f"task runner crashed: {crash_detail(exc)}")
         except Exception:  # noqa: BLE001
             log.exception("could not report crash for job %s", spec.job_id)
         return 1

tests/test_review_runner.py

@@ -143,7 +143,26 @@ def test_run_reports_error_on_crash(self):
         code, emitter = self._run(prepare_side_effect=RuntimeError("boom"))
         self.assertEqual(code, 1)
         self.assertEqual(emitter.terminal["status"], "error")
+        # The runner pod is reaped as soon as it self-reports, so the crash
+        # cause must travel in the reported error itself — not "(see pod log)".
         self.assertIn("review crashed", emitter.terminal["error"])
+        self.assertIn("boom", emitter.terminal["error"])
+        self.assertNotIn("see pod log", emitter.terminal["error"])
+
+    def test_run_surfaces_llm_error(self):
+        from reviewbot.llm_client import LLMResponseError
+
+        exc = LLMResponseError(
+            429, "Too Many Requests", "https://router/v1/chat", "rate limit exceeded"
+        )
+        code, emitter = self._run(prepare_side_effect=exc)
+        self.assertEqual(code, 1)
+        self.assertEqual(emitter.terminal["status"], "error")
+        # The LLM endpoint's own status + body excerpt lands on the job, so a
+        # 429/400/auth failure is legible without the (reaped) pod log.
+        self.assertIn("429", emitter.terminal["error"])
+        self.assertIn("rate limit exceeded", emitter.terminal["error"])
+        self.assertNotIn("see pod log", emitter.terminal["error"])
 
 
 if __name__ == "__main__":