diff --git a/WORK-RECORDS.md b/WORK-RECORDS.md index 042a44d..676e007 100644 --- a/WORK-RECORDS.md +++ b/WORK-RECORDS.md @@ -184,7 +184,7 @@ | task | ACTIVITY-WP-0029-T02 | done | — | workplans/ACTIVITY-WP-0029-hub-port-alignment.md | | task | ACTIVITY-WP-0029-T03 | wait | — | workplans/ACTIVITY-WP-0029-hub-port-alignment.md | | task | ACTIVITY-WP-0029-T04 | done | — | workplans/ACTIVITY-WP-0029-hub-port-alignment.md | -| task | ACTIVITY-WP-0030-T01 | wait | — | workplans/ACTIVITY-WP-0030-daily-sbom-catchup.md | +| task | ACTIVITY-WP-0030-T01 | done | — | workplans/ACTIVITY-WP-0030-daily-sbom-catchup.md | | task | ACTIVITY-WP-0030-T02 | wait | — | workplans/ACTIVITY-WP-0030-daily-sbom-catchup.md | | task | ACTIVITY-WP-0030-T03 | progress | — | workplans/ACTIVITY-WP-0030-daily-sbom-catchup.md | | task | ACTIVITY-WP-0030-T04 | wait | — | workplans/ACTIVITY-WP-0030-daily-sbom-catchup.md | diff --git a/src/activity_core/llm_client.py b/src/activity_core/llm_client.py index 7bc00ff..952d843 100644 --- a/src/activity_core/llm_client.py +++ b/src/activity_core/llm_client.py @@ -55,7 +55,8 @@ class LLMConnectClient: json=payload, timeout=self.timeout_seconds, ) - resp.raise_for_status() + if resp.status_code >= 400: + raise RuntimeError(_llm_connect_error_text(resp)) data = resp.json() self.last_response_metadata = _extract_response_metadata(data) content = data.get("content") @@ -64,6 +65,43 @@ class LLMConnectClient: return content +_SAFE_ERROR_KEYS = ("error", "provider_status", "provider", "model") +_MAX_ERROR_MESSAGE_CHARS = 400 + + +def _llm_connect_error_text(resp: httpx.Response) -> str: + """Describe an llm-connect error using its own diagnostic body. + + llm-connect maps every provider API error onto HTTP 502 and puts the real + cause in the body (``error``, ``provider_status``) — see + ``llm_connect/server.py::_error_response``. Reporting only the transport + status makes a rejected provider key look identical to a dead gateway, + which is exactly the ambiguity ACTIVITY-WP-0031-T03 evidence must not have. + + llm-connect sanitizes its own ``message`` before returning it; this keeps a + bounded copy of that text plus an allowlist of non-secret fields. + """ + base = f"llm-connect returned HTTP {resp.status_code}" + try: + body = resp.json() + except ValueError: + return base + if not isinstance(body, dict): + return base + + parts = [ + f"{key}={body[key]}" + for key in _SAFE_ERROR_KEYS + if body.get(key) not in (None, "") + ] + message = body.get("message") + if isinstance(message, str) and message.strip(): + parts.append(f"message={message.strip()[:_MAX_ERROR_MESSAGE_CHARS]}") + if not parts: + return base + return f"{base}: " + "; ".join(parts) + + _SAFE_RESPONSE_METADATA_KEYS = { "finish_reason", "usage", diff --git a/tests/test_llm_client.py b/tests/test_llm_client.py index 655c4b4..87cc0ba 100644 --- a/tests/test_llm_client.py +++ b/tests/test_llm_client.py @@ -1,6 +1,7 @@ from __future__ import annotations import httpx +import pytest from activity_core.llm_client import LLMConnectClient @@ -9,8 +10,7 @@ def test_llm_connect_client_forwards_run_config(monkeypatch) -> None: captured: dict = {} class Response: - def raise_for_status(self) -> None: - pass + status_code = 200 def json(self) -> dict: return { @@ -59,3 +59,103 @@ def test_llm_connect_client_forwards_run_config(monkeypatch) -> None: "finish_reason": "stop", "usage": {"input_tokens": 10, "output_tokens": 20}, } + + +class ErrorResponse: + """Models an llm-connect error reply, which carries the real cause.""" + + def __init__(self, status_code: int, body) -> None: + self.status_code = status_code + self._body = body + + def json(self): + if self._body is _NON_JSON: + raise ValueError("not json") + return self._body + + +_NON_JSON = object() + + +def _client(monkeypatch, response) -> LLMConnectClient: + monkeypatch.setattr(httpx, "post", lambda url, json, timeout: response) + return LLMConnectClient("http://llm-connect.local") + + +def test_rejected_provider_key_is_distinguishable_from_a_dead_gateway(monkeypatch) -> None: + """llm-connect maps every provider API error onto 502 (server.py:230). + + ACTIVITY-WP-0031: reporting only "502 Bad Gateway" made a revoked + OpenRouter key look like a downed llm-connect for four days. + """ + client = _client( + monkeypatch, + ErrorResponse( + 502, + { + "error": "provider_api_error", + "provider_status": 401, + "provider": "openrouter", + "message": "No auth credentials found", + }, + ), + ) + + with pytest.raises(RuntimeError) as excinfo: + client.complete("Prompt") + + text = str(excinfo.value) + assert "HTTP 502" in text + assert "error=provider_api_error" in text + assert "provider_status=401" in text + assert "provider=openrouter" in text + assert "message=No auth credentials found" in text + + +def test_error_text_keeps_only_allowlisted_fields(monkeypatch) -> None: + client = _client( + monkeypatch, + ErrorResponse( + 502, + { + "error": "provider_api_error", + "provider_status": 401, + "api_key": "sk-should-never-appear", + "raw_response": {"authorization": "Bearer secret"}, + }, + ), + ) + + with pytest.raises(RuntimeError) as excinfo: + client.complete("Prompt") + + text = str(excinfo.value) + assert "sk-should-never-appear" not in text + assert "authorization" not in text + assert "provider_status=401" in text + + +def test_error_message_is_bounded(monkeypatch) -> None: + client = _client( + monkeypatch, + ErrorResponse(502, {"error": "provider_api_error", "message": "x" * 5000}), + ) + + with pytest.raises(RuntimeError) as excinfo: + client.complete("Prompt") + + assert len(str(excinfo.value)) < 600 + + +def test_error_without_a_usable_body_still_reports_the_status(monkeypatch) -> None: + client = _client(monkeypatch, ErrorResponse(504, _NON_JSON)) + + with pytest.raises(RuntimeError, match="llm-connect returned HTTP 504"): + client.complete("Prompt") + + +def test_error_with_a_non_dict_body_still_reports_the_status(monkeypatch) -> None: + client = _client(monkeypatch, ErrorResponse(500, ["unexpected"])) + + with pytest.raises(RuntimeError, match="llm-connect returned HTTP 500"): + client.complete("Prompt") diff --git a/workplans/ACTIVITY-WP-0031-production-execution-reliability-cleanup.md b/workplans/ACTIVITY-WP-0031-production-execution-reliability-cleanup.md index 074396e..b285af0 100644 --- a/workplans/ACTIVITY-WP-0031-production-execution-reliability-cleanup.md +++ b/workplans/ACTIVITY-WP-0031-production-execution-reliability-cleanup.md @@ -8,7 +8,7 @@ status: active owner: codex topic_slug: activity-core created: "2026-08-20" -updated: "2026-08-20" +updated: "2026-08-21" related: - ACTIVITY-WP-0021 - ACTIVITY-WP-0022 @@ -49,6 +49,32 @@ still returns sanitized OpenRouter HTTP 401, proving the canonical key itself is rejected. T01 remains `wait` on an attended OpenRouter account owner to mint and safely provision a replacement key; no key value was read or printed. +Progress 2026-08-21 (activity-core half, no custody boundary crossed): live +evidence confirms a single fault, not two. FI, Binky, and daily triage still +fail today (`05:20` / `05:30` / `06:23` UTC) with `502 Bad Gateway` from +llm-connect — **after** yesterday's rollout (`llm-connect` pod is `Running` +1/1, endpoints healthy). That 502 is not a dead gateway: +`llm_connect/server.py::_error_response` maps **every** `LLMAPIError` onto 502 +and carries the real cause in the body (`error`, `provider_status`). So the +502s and the reported upstream 401 are the same rejected key. + +`LLMConnectClient.complete` called `raise_for_status()` and discarded that +body, so four days of production evidence read as "llm-connect is down" when it +meant "the provider rejected the key". Fixed: the client now raises with +`error`, `provider_status`, `provider`, `model`, and a bounded copy of +llm-connect's already-sanitized `message`, under a field allowlist so no +provider blob or key material can reach the run artefact. Five tests cover the +401-behind-502 case, the allowlist, bounding, and unusable bodies. + +The identical body-discarding call exists in +`rein-aharness/rein_aharness/llm_connect_client.py:65`, which is what produces +the opaque ops_run failure text in the status table. Not ours to edit — +handed to rein-aharness. + +T01 still `wait`: the key rotation itself remains attended and +railiance-platform-owned. What changed is that the next failure will name its +own cause. + ## Emergency-pause weekly SBOM fan-out ```task