fix: normalize LLM provider key lookup and skip disabled HyDE ping (final review)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

backend/app/api/routes/status.py

@@ -8,7 +8,7 @@ from fastapi import APIRouter
 
 from app.config.settings import settings
 from app.domain.retrieval import RetrievedChunk
-from app.services.llm.llm_factory import get_llm_client
+from app.services.llm.llm_factory import get_llm_client, get_llm_factory
 from app.shared.bootstrap import (
     get_bm25_retriever,
     get_binary_store,
@@ -129,6 +129,27 @@ async def get_health():
     }
 
 
+def _normalize_llm_provider(raw_provider: str) -> str:
+    """Normalize a raw LLM_PROVIDER/HYDE_LLM_PROVIDER settings string to the
+    canonical LLMProvider enum value, the SAME way LLMFactory.create() does.
+
+    TrackedLLMClient.chat() (tracked_client.py) always records usage under
+    `self._inner.config.provider.value` — the NORMALIZED enum value produced by
+    LLMFactory._parse_provider() — never the raw string a caller passed to
+    get_llm_client(). Reusing that same normalization here (instead of
+    duplicating the alias table) guarantees the tracker key this route reads
+    always agrees with the key TrackedLLMClient wrote, even when the raw
+    settings value is a non-canonical alias (e.g. "deepseek-v3") or different
+    casing. Falls back to the raw string, unchanged, if it does not match any
+    known provider/alias, so this passive status endpoint still renders
+    (as "never_called") instead of raising on a misconfigured provider string.
+    """
+    try:
+        return get_llm_factory()._parse_provider(raw_provider).value
+    except ValueError:
+        return raw_provider
+
+
 def _resolve_role_provider_model(role: str) -> tuple[str, str]:
     """Return the (provider, model) pair currently configured for one AI model role.
 
@@ -138,10 +159,10 @@ def _resolve_role_provider_model(role: str) -> tuple[str, str]:
     recorded when HyDE actually ran.
     """
     if role == "main_llm":
-        return settings.llm_provider, settings.llm_model
+        return _normalize_llm_provider(settings.llm_provider), settings.llm_model
     if role == "hyde_llm":
         return (
-            settings.hyde_llm_provider or settings.llm_provider,
+            _normalize_llm_provider(settings.hyde_llm_provider or settings.llm_provider),
             settings.hyde_llm_model or settings.llm_model,
         )
     if role == "embedding":
@@ -204,7 +225,16 @@ async def get_model_statuses():
 
 
 async def _ping_main_or_hyde(role: str) -> None:
-    """Send one minimal chat completion to the LLM configured for `role`."""
+    """Send one minimal chat completion to the LLM configured for `role`.
+
+    Skipped entirely for "hyde_llm" when settings.hyde_enabled is False,
+    mirroring _ping_reranker()'s disabled-skip pattern: when HyDE is turned
+    off (or reuses the main LLM, the default), issuing this ping would just be
+    a redundant duplicate chat call against the same model for no benefit.
+    "main_llm" is always pinged regardless of this check.
+    """
+    if role == "hyde_llm" and not settings.hyde_enabled:
+        return
     provider, model = _resolve_role_provider_model(role)
     try:
         client = get_llm_client(provider=provider, model=model)

tests/test_status_models_routes.py

@@ -12,6 +12,7 @@ import pytest
 from fastapi.testclient import TestClient
 
 from app.services.llm.base_client import LLMResponse
+from app.services.llm.llm_factory import LLMFactory, get_llm_client
 from app.shared.model_usage_tracker import get_model_usage_tracker
 
 
@@ -23,6 +24,20 @@ def _reset_tracker():
     get_model_usage_tracker()._entries.clear()
 
 
+@pytest.fixture
+def _reset_llm_factory_instances():
+    """Clear LLMFactory's process-wide client cache before/after this test only.
+
+    Mirrors backend/tests/observability/test_llm_factory_tracking.py's
+    _reset_singletons fixture: LLMFactory._global_instances persists for the
+    life of the process, so without this the cache entry created by driving a
+    real get_llm_client() call in a test would leak into other tests.
+    """
+    LLMFactory._global_instances.clear()
+    yield
+    LLMFactory._global_instances.clear()
+
+
 @pytest.fixture
 def client():
     """Return a TestClient for the real app (status routes require no auth)."""
@@ -131,3 +146,74 @@ def test_ping_records_get_llm_client_failure_instead_of_dropping_it(client):
     assert hyde_row["status"] == "error"
     assert hyde_row["call_count_error"] == 1
     assert hyde_row["last_error"] == "missing api key"
+
+
+def test_ping_skips_hyde_llm_when_hyde_disabled(client):
+    """POST /status/models/ping must NOT ping hyde_llm when settings.hyde_enabled is False:
+    HyDE often reuses the main LLM, so a live ping would just be a redundant duplicate chat
+    call against the same model for no benefit. main_llm must still always be pinged."""
+    from app.config.settings import settings
+
+    mock_llm_response = LLMResponse(content="pong", model="test-model", usage={"total_tokens": 1})
+    mock_llm_client = MagicMock()
+    mock_llm_client.chat.return_value = mock_llm_response
+    mock_embedding = MagicMock()
+    mock_embedding.embed_query.return_value = [0.1]
+
+    with patch.object(settings, "hyde_enabled", False), \
+         patch("app.api.routes.status.get_llm_client", return_value=mock_llm_client) as mock_get_llm_client, \
+         patch("app.api.routes.status.get_embedding_provider", return_value=mock_embedding), \
+         patch("app.api.routes.status.get_reranker", return_value=None):
+        resp = client.post("/api/v1/status/models/ping")
+
+    assert resp.status_code == 200
+    # Only main_llm's ping should reach get_llm_client()/.chat(); hyde_llm's must be skipped.
+    assert mock_get_llm_client.call_count == 1
+    assert mock_llm_client.chat.call_count == 1
+    hyde_row = next(m for m in resp.json()["models"] if m["role"] == "hyde_llm")
+    assert hyde_row["status"] == "disabled"
+
+
+def test_ping_write_path_and_status_read_path_agree_for_non_canonical_provider_alias(
+    client, _reset_llm_factory_instances
+):
+    """Regression for the tracker key mismatch bug: TrackedLLMClient.chat() always records
+    usage under the NORMALIZED LLMProvider enum value from LLMFactory._parse_provider()
+    (see tracked_client.py), never the raw provider string a caller passed to
+    get_llm_client(). Before the fix, _resolve_role_provider_model() returned
+    settings.llm_provider verbatim with no normalization. So whenever LLM_PROVIDER held a
+    non-canonical alias (e.g. "deepseek-v3" instead of "deepseek"), the read-side lookup key
+    ("deepseek-v3:<model>") stopped matching the write-side key TrackedLLMClient actually
+    recorded under ("deepseek:<model>"), and /status/models showed main_llm as perpetually
+    "never_called" even though it was being actively tracked.
+
+    This drives the FULL real path (LLMFactory.create() -> TrackedLLMClient ->
+    ModelUsageTracker -> the /status/models route), the same way real call sites like
+    compliance.py invoke get_llm_client(provider=settings.llm_provider, model=settings.llm_model)
+    -- unlike test_get_models_reflects_recorded_usage above, which shortcuts by recording
+    directly into the tracker.
+    """
+    from app.config.settings import settings
+
+    mock_response = LLMResponse(content="pong", model=settings.llm_model, usage={"total_tokens": 7})
+
+    def _fake_deepseek_client(config):
+        # Carry the REAL LLMConfig built by LLMFactory.create() so TrackedLLMClient.chat()
+        # records under config.provider.value exactly like it does in production.
+        fake = MagicMock()
+        fake.config = config
+        fake.chat.return_value = mock_response
+        return fake
+
+    with patch.object(settings, "llm_provider", "deepseek-v3"), \
+         patch("app.services.llm.llm_factory.DeepSeekClient", side_effect=_fake_deepseek_client):
+        tracked_client = get_llm_client(provider=settings.llm_provider, model=settings.llm_model, api_key="test-key")
+        tracked_client.chat([{"role": "user", "content": "hi"}])
+
+        resp = client.get("/api/v1/status/models")
+
+    assert resp.status_code == 200
+    main_row = next(m for m in resp.json()["models"] if m["role"] == "main_llm")
+    assert main_row["status"] == "ok"
+    assert main_row["total_tokens"] == 7
+    assert main_row["provider"] == "deepseek"