diff --git a/pr-triage/tests/test_e2e.py b/pr-triage/tests/test_e2e.py index 95ea98f..244b5ef 100644 --- a/pr-triage/tests/test_e2e.py +++ b/pr-triage/tests/test_e2e.py @@ -25,6 +25,7 @@ def __init__(self): self.calls = [] # (method, path, body_dict_or_None) self.model_reply = None # str content, or Exception subclass to raise, or dict for raw payload self.model_status = 200 + self.model_mode = "json" # json | redirect | html self.files = [{"filename": "docs/architecture.md", "status": "modified"}] self.diff = "--- a/docs/architecture.md\n+++ b/docs/architecture.md\n@@ -1 +1 @@\n-old\n+new\n" self.permission = "admin" @@ -72,12 +73,25 @@ def _route(self, method): if path == "/v1/chat/completions": s.model_headers = {k.lower(): v for k, v in self.headers.items()} + if s.model_mode == "redirect": + self.send_response(302) + self.send_header("Location", f"http://127.0.0.1:{self.server.server_address[1]}/cdn-cgi/access/login/llm.ionite.io?kid=abc") + self.send_header("Content-Length", "0") + self.end_headers() + return + if s.model_mode == "html": + page = "Sign in ยท Cloudflare Access" + "login " * 40 + "" + assert len(page) > 200 # the 120-char cap must be measurable + return self._send(200, page, "text/html; charset=utf-8") if s.model_status != 200: return self._send(s.model_status, {"error": "nope"}) if isinstance(s.model_reply, dict): return self._send(200, s.model_reply) return self._send(200, {"choices": [{"message": {"role": "assistant", "content": s.model_reply}}]}) + if path.startswith("/cdn-cgi/access/login"): + return self._send(200, "followed the redirect", "text/html") + prefix = "/github/repos/LykosAI/Test" if not path.startswith(prefix): return self._send(404, {"message": "unknown"}) @@ -317,6 +331,82 @@ def test_empty_content_with_only_reasoning_is_the_offline_path(self): self.assertEqual(self.calls("POST", "/pulls/7/reviews"), []) self.assertEqual(len(self.calls("POST", "/issues/7/comments")), 1) + def test_access_redirect_is_reported_as_a_302_and_never_followed(self): + self.state.model_mode = "redirect" + result = self.run_script() + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.calls("POST", "/pulls/7/reviews"), []) + self.assertIn("could not reach my model", self.calls("POST", "/issues/7/comments")[0][2]["body"]) + self.assertEqual(len(self.calls("POST", "/v1/chat/completions")), 1) + self.assertEqual([c for c in self.state.calls if c[1].startswith("/cdn-cgi/")], []) + warning = [l for l in result.stdout.splitlines() if "::warning::Model unreachable" in l][0] + self.assertIn("ModelReplyError", warning) + self.assertIn("HTTP 302 redirect to 127.0.0.1", warning) + self.assertNotIn("kid=abc", warning) + for secret in ("cf-id-value", "cf-secret-value", "llm-key-value"): + self.assertNotIn(secret, result.stdout + result.stderr) + + def test_html_200_is_reported_with_status_url_content_type_and_capped_body(self): + self.state.model_mode = "html" + result = self.run_script() + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.calls("POST", "/pulls/7/reviews"), []) + self.assertIn("could not reach my model", self.calls("POST", "/issues/7/comments")[0][2]["body"]) + warning = [l for l in result.stdout.splitlines() if "::warning::Model unreachable" in l][0] + self.assertIn("non-JSON reply: HTTP 200 from http://127.0.0.1:", warning) + self.assertIn("/v1/chat/completions", warning) + self.assertIn("Content-Type text/html; charset=utf-8", warning) + self.assertIn("Sign in", warning) + self.assertNotIn("</html>", warning) # capped well before the end of the body + for secret in ("cf-id-value", "cf-secret-value", "llm-key-value"): + self.assertNotIn(secret, result.stdout + result.stderr) + + def test_credential_presence_line_carries_booleans_and_lengths_only(self): + self.state.model_reply = '{"verdict": "human", "reason": "hm"}' + result = self.run_script() + line = [l for l in result.stdout.splitlines() if l.startswith("Model credentials in the environment:")][0] + self.assertIn("CF_ACCESS_CLIENT_ID present=True len=11", line) + self.assertIn("CF_ACCESS_CLIENT_SECRET present=True len=15", line) + self.assertIn("LLM_API_KEY present=True len=13", line) + for secret in ("cf-id-value", "cf-secret-value", "llm-key-value"): + self.assertNotIn(secret, line) + + def test_missing_credentials_show_as_absent(self): + self.state.model_reply = '{"verdict": "human", "reason": "hm"}' + env = dict( + os.environ, + GITHUB_EVENT_PATH=self.event_path, + GITHUB_REPOSITORY="LykosAI/Test", + GITHUB_API_URL=f"http://127.0.0.1:{self.port}/github", + GITHUB_TOKEN="ghs_fake", + TRIAGE_MODEL="fake-model", + TRIAGE_ENDPOINT=f"http://127.0.0.1:{self.port}/v1", + CF_ACCESS_CLIENT_ID="", + PYTHONIOENCODING="utf-8", + ) + env.pop("CF_ACCESS_CLIENT_SECRET", None) + env.pop("LLM_API_KEY", None) + result = subprocess.run([sys.executable, SCRIPT], env=env, capture_output=True, text=True, encoding="utf-8", timeout=60) + line = [l for l in result.stdout.splitlines() if l.startswith("Model credentials in the environment:")][0] + self.assertIn("CF_ACCESS_CLIENT_ID present=False len=0", line) + self.assertIn("CF_ACCESS_CLIENT_SECRET present=False len=0", line) + self.assertIn("LLM_API_KEY present=False len=0", line) + + def test_ask_model_raises_a_diagnosable_error_on_redirect_and_html(self): + sys.path.insert(0, os.path.join(HERE, "..")) + import triage + + endpoint = f"http://127.0.0.1:{self.port}/v1" + self.state.model_mode = "redirect" + with self.assertRaises(triage.ModelReplyError) as ctx: + triage.ask_model(endpoint, "m", "k", "i", "s", "sys", "user") + self.assertIn("HTTP 302 redirect to 127.0.0.1", str(ctx.exception)) + self.state.model_mode = "html" + with self.assertRaises(triage.ModelReplyError) as ctx: + triage.ask_model(endpoint, "m", "k", "i", "s", "sys", "user") + self.assertIn("non-JSON reply: HTTP 200", str(ctx.exception)) + self.assertIn("text/html", str(ctx.exception)) + def test_github_outage_exits_zero(self): self.state.model_reply = '{"verdict": "approve", "reason": "ok"}' env_override = f"http://127.0.0.1:{self.port}/nowhere" diff --git a/pr-triage/triage.py b/pr-triage/triage.py index 5ea782f..404ae05 100644 --- a/pr-triage/triage.py +++ b/pr-triage/triage.py @@ -28,6 +28,7 @@ import re import sys import urllib.error +import urllib.parse import urllib.request from dataclasses import dataclass, field from typing import Any @@ -315,10 +316,32 @@ def render_comment_body( class HttpError(Exception): - def __init__(self, status: int, body: str): + def __init__(self, status: int, body: str, headers: dict[str, str] | None = None): super().__init__(f"HTTP {status}: {body[:300]}") self.status = status self.body = body + self.headers = headers or {} + + +class ModelReplyError(Exception): + """The model endpoint answered with something other than a chat completion. + + The message is safe to log: status, final URL, content type, redirect host and a + capped body prefix; never a request header value. + """ + + +class _NoRedirect(urllib.request.HTTPRedirectHandler): + def redirect_request(self, req, fp, code, msg, headers, newurl): + return None + + +@dataclass(frozen=True) +class HttpResponse: + status: int + body: str + headers: dict[str, str] + url: str def http( @@ -327,18 +350,20 @@ def http( headers: dict[str, str], body: Any = None, timeout: float = GITHUB_TIMEOUT_SECONDS, -) -> tuple[int, str, dict[str, str]]: + follow_redirects: bool = True, +) -> HttpResponse: data = None req_headers = dict(headers) if body is not None: data = json.dumps(body).encode("utf-8") req_headers["Content-Type"] = "application/json" req = urllib.request.Request(url, data=data, method=method, headers=req_headers) + opener = urllib.request.build_opener() if follow_redirects else urllib.request.build_opener(_NoRedirect()) try: - with urllib.request.urlopen(req, timeout=timeout) as resp: - return resp.status, resp.read().decode("utf-8", "replace"), dict(resp.headers) + with opener.open(req, timeout=timeout) as resp: + return HttpResponse(resp.status, resp.read().decode("utf-8", "replace"), dict(resp.headers), resp.geturl()) except urllib.error.HTTPError as e: - raise HttpError(e.code, e.read().decode("utf-8", "replace")) from e + raise HttpError(e.code, e.read().decode("utf-8", "replace"), dict(e.headers)) from e class GitHub: @@ -356,8 +381,7 @@ def _url(self, path: str) -> str: return f"{self.api_url}/repos/{self.repo}{path}" def get_json(self, path: str) -> Any: - _, text, _ = http("GET", self._url(path), self.headers) - return json.loads(text) + return json.loads(http("GET", self._url(path), self.headers).body) def get_paginated(self, path: str) -> list[Any]: items: list[Any] = [] @@ -372,8 +396,7 @@ def get_paginated(self, path: str) -> list[Any]: def get_diff(self, number: int) -> str: headers = dict(self.headers, Accept="application/vnd.github.diff") - _, text, _ = http("GET", self._url(f"/pulls/{number}"), headers) - return text + return http("GET", self._url(f"/pulls/{number}"), headers).body def author_permission(self, login: str) -> str | None: try: @@ -433,6 +456,17 @@ def ask_model( system_prompt: str, user_content: str, ) -> str | None: + log( + "Model credentials in the environment: " + + ", ".join( + f"{name} present={bool(value)} len={len(value)}" + for name, value in ( + ("CF_ACCESS_CLIENT_ID", cf_id), + ("CF_ACCESS_CLIENT_SECRET", cf_secret), + ("LLM_API_KEY", api_key), + ) + ) + ) headers = { "Authorization": f"Bearer {api_key}", "CF-Access-Client-Id": cf_id, @@ -449,14 +483,30 @@ def ask_model( "max_tokens": 2500, "reasoning_effort": "low", } - _, text, _ = http( - "POST", - endpoint.rstrip("/") + "/chat/completions", - headers, - payload, - timeout=MODEL_TIMEOUT_SECONDS, - ) - data = json.loads(text) + try: + resp = http( + "POST", + endpoint.rstrip("/") + "/chat/completions", + headers, + payload, + timeout=MODEL_TIMEOUT_SECONDS, + follow_redirects=False, + ) + except HttpError as e: + if 300 <= e.status < 400: + location = e.headers.get("Location") or e.headers.get("location") or "" + host = urllib.parse.urlsplit(location).netloc or "(no Location header)" + raise ModelReplyError(f"HTTP {e.status} redirect to {host}; an Access login page means the service token was not accepted") from e + raise + content_type = resp.headers.get("Content-Type") or resp.headers.get("content-type") or "(none)" + try: + data = json.loads(resp.body) + except json.JSONDecodeError as e: + raise ModelReplyError( + f"non-JSON reply: HTTP {resp.status} from {resp.url}, Content-Type {content_type}, body starts {resp.body[:120]!r}" + ) from e + if not isinstance(data, dict): + raise ModelReplyError(f"JSON reply is not an object: HTTP {resp.status} from {resp.url}, body starts {resp.body[:120]!r}") choices = data.get("choices") or [] if not choices: return None