fix(llamacpp): surface a clear error for non-router-mode servers (FR-006)

LlamaCppProvider.list_models() caught every exception and returned [],
indistinguishable from "no models installed". Fixed _get_json to raise
on HTTP error status (matching _post_json's existing behavior — its
docstring already claimed this), and list_models() to distinguish
OSError (genuinely unreachable, degrades to []) from RuntimeError
(server responded with an error — surfaced with a router-mode hint).

Found by beacon-reviewer ahead of PR open.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0132ojafeazQ3ephcBejEWFj

specs/008-llamacpp-integration/tasks.md

@@ -149,3 +149,28 @@ Unlike the prerequisite epic, **this decomposition genuinely holds** —
 there is no shared "output type" migration forcing an atomic cutover, because
 `LlamaCppClient` is new, not a change to an existing node. Each phase really
 can land independently.
+
+---
+
+## Post-implementation review finding (fixed)
+
+A `beacon-reviewer` pass ahead of PR open found `LlamaCppProvider.list_models()`
+caught *every* exception and returned `[]`, silently indistinguishable from
+"no models installed" — violating FR-006 and `contracts/llamacpp_provider_conformance.md`'s
+explicit requirement that a non-router-mode `llama-server` (unreachable
+endpoints → HTTP error on `GET /models`) surface a clear, specific error.
+
+Fixed: `_get_json` (shared with `OllamaProvider`, in `ollama_provider.py`) now
+raises `RuntimeError` on an HTTP error status, matching `_post_json`'s
+existing behavior — its docstring already claimed this, it just didn't do it.
+`LlamaCppProvider.list_models()` now distinguishes `OSError` (genuinely
+unreachable — connection refused, DNS failure, timeout; all aiohttp
+connection-level exceptions are `OSError` subclasses) from `RuntimeError`
+(server responded, but with an error): the former still degrades gracefully
+to `[]` (consistent with `OllamaProvider`'s existing UX), the latter is
+re-raised naming router mode as the likely cause. Regression test added:
+`test_list_models_non_router_mode_raises_clear_error` in
+`tests/test_llamacpp_provider.py`. `OllamaProvider`'s own `list_models()`/
+`_fetch_models()` still catch broadly and degrade to `[]` unchanged — no
+spec requirement asks Ollama to make this distinction, and this fix doesn't
+force it to.

src/comfydv/_llm/llamacpp_provider.py

@@ -55,11 +55,27 @@ class LlamaCppProvider:
 
         try:
             data = await _get_json(f"{self.host}/models", headers=self.headers)
-        except Exception as exc:
+        except OSError as exc:
+            # Genuinely unreachable (connection refused, DNS failure, timed
+            # out — aiohttp's connection-level exceptions are all OSError
+            # subclasses) — degrade gracefully like OllamaProvider does, so
+            # a not-yet-started server just shows an empty dropdown rather
+            # than a hard error.
             logger.warning(
                 "Could not fetch llama.cpp models from %s: %s", self.host, exc
             )
             return []
+        except RuntimeError as exc:
+            # The server answered but with an HTTP error status — GET
+            # /models only exists in router mode, so this is almost always
+            # a llama-server launched without --models-dir/--models-preset.
+            # Surfacing this distinctly (FR-006) matters: silently returning
+            # [] here would be indistinguishable from "no models installed".
+            raise RuntimeError(
+                f"llama-server at {self.host} did not return a model list from "
+                f"GET {self.host}/models — is it running in router mode "
+                f"(--models-dir or --models-preset)? Underlying error: {exc}"
+            ) from exc
 
         models = []
         for m in data.get("data", []):

src/comfydv/_llm/ollama_provider.py

@@ -132,7 +132,14 @@ async def _post_json(
 async def _get_json(
     url: str, *, timeout: float = 5.0, headers: dict | None = None
 ) -> dict:
-    """GET url, return parsed response dict. Raises on connection/HTTP error."""
+    """GET url, return parsed response dict.
+
+    Raises RuntimeError on an HTTP error status (distinct message, so callers
+    can tell "server responded with an error" from "couldn't reach it at
+    all" — aiohttp connection/timeout errors propagate unwrapped for that
+    reason). Message is generic, not backend-branded: this helper is shared
+    by every LLMProvider implementation.
+    """
     import aiohttp
 
     async with aiohttp.ClientSession() as session:
@@ -141,6 +148,11 @@ async def _get_json(
             headers=headers or None,
             timeout=aiohttp.ClientTimeout(total=timeout),
         ) as resp:
+            if resp.status >= 400:
+                body = await resp.text()
+                raise RuntimeError(
+                    f"Server returned HTTP {resp.status} for {url}: {body[:300]}"
+                )
             return await resp.json()
 
 

tests/test_llamacpp_provider.py

@@ -103,6 +103,22 @@ def test_list_models_unreachable_returns_empty(monkeypatch):
     assert models == []
 
 
+def test_list_models_non_router_mode_raises_clear_error(monkeypatch):
+    """FR-006: a llama-server that IS reachable but wasn't launched with
+    --models-dir/--models-preset answers GET /models with an HTTP error
+    (the endpoint doesn't exist outside router mode). That must surface as
+    a specific, actionable error — not silently degrade to an empty list,
+    which would be indistinguishable from "server has no models"."""
+
+    async def fake_get(url, *, timeout=5.0, headers=None):
+        raise RuntimeError("Server returned HTTP 404 for http://x/models: not found")
+
+    monkeypatch.setattr(provider_mod, "_get_json", fake_get)
+
+    with pytest.raises(RuntimeError, match="router mode"):
+        _run_async(LlamaCppProvider("http://localhost:8080").list_models())
+
+
 def test_list_models_cached_second_call(monkeypatch):
     calls = {"n": 0}