refactor(providers): address PR #772 review round 3 — hermetic test, lazy logging, defensive-guard tests

- test_registry.py: stub `mistralai.client.Mistral` in
  `test_registry_mistral_wins_over_ollama`, mirroring the sibling
  picker test, so the test doesn't depend on the SDK accepting
  arbitrary keys.
- openai.py: convert remaining f-string `logger.info(...)` calls to
  lazy `%s` formatting, aligning with the pattern in mistral.py and
  the repo's logging convention.
- test_mistral.py: add four tests covering the defensive RuntimeError
  guards in `embed()` and `_embed_batch_request()` — empty
  response.data, single null embedding, batch null embedding, and
  count-mismatch.
- docs/configuration.md: add `AWS_ACCESS_KEY_ID` and
  `AWS_SECRET_ACCESS_KEY` rows to the env-var reference table; they
  were already mentioned in prose but missing from the table.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

docs/configuration.md

@@ -593,6 +593,8 @@ equivalent.** Operators who need a runtime toggle should open an issue.
 | `MISTRAL_EMBEDDING_MODEL` | ⚠️ Optional | `mistral-embed` | Mistral embedding model (1024-dim) |
 | `MISTRAL_BASE_URL` | ⚠️ Optional | - | Mistral base URL override (proxies, on-prem) |
 | `AWS_REGION` | ⚠️ Optional | - | AWS region (selects Bedrock provider) |
+| `AWS_ACCESS_KEY_ID` | ⚠️ Optional | - | AWS access key (boto3 credential chain fallback) |
+| `AWS_SECRET_ACCESS_KEY` | ⚠️ Optional | - | AWS secret key (boto3 credential chain fallback) |
 | `BEDROCK_EMBEDDING_MODEL` | ⚠️ Optional | - | Bedrock embedding model ID |
 | `BEDROCK_GENERATION_MODEL` | ⚠️ Optional | - | Bedrock generation model ID |
 | `SIMPLE_EMBEDDING_DIMENSION` | ⚠️ Optional | `384` | Dimension for the fallback Simple provider |

nextcloud_mcp_server/providers/openai.py

@@ -77,9 +77,12 @@ class OpenAIProvider(Provider):
             self._dimension = OPENAI_EMBEDDING_DIMENSIONS[embedding_model]
 
         logger.info(
-            f"Initialized OpenAI provider: base_url={base_url or 'default'} "
-            f"(embedding_model={embedding_model}, generation_model={generation_model}, "
-            f"dimension={self._dimension})"
+            "Initialized OpenAI provider: base_url=%s "
+            "(embedding_model=%s, generation_model=%s, dimension=%s)",
+            base_url or "default",
+            embedding_model,
+            generation_model,
+            self._dimension,
         )
 
     @property
@@ -123,8 +126,9 @@ class OpenAIProvider(Provider):
         if self._dimension is None:
             self._dimension = len(embedding)
             logger.info(
-                f"Detected embedding dimension: {self._dimension} "
-                f"for model {self.embedding_model}"
+                "Detected embedding dimension: %d for model %s",
+                self._dimension,
+                self.embedding_model,
             )
 
         return embedding
@@ -167,8 +171,9 @@ class OpenAIProvider(Provider):
             if self._dimension is None and batch_embeddings:
                 self._dimension = len(batch_embeddings[0])
                 logger.info(
-                    f"Detected embedding dimension: {self._dimension} "
-                    f"for model {self.embedding_model}"
+                    "Detected embedding dimension: %d for model %s",
+                    self._dimension,
+                    self.embedding_model,
                 )
 
         return all_embeddings

tests/unit/providers/test_mistral.py

@@ -200,6 +200,61 @@ async def test_mistral_base_url_passed_to_sdk(mocker):
     )
 
 
+@pytest.mark.unit
+async def test_mistral_embed_raises_on_empty_response_data(mock_mistral_client):
+    """embed(): empty response.data triggers the defensive RuntimeError guard."""
+    empty_response = MagicMock()
+    empty_response.data = []
+    mock_mistral_client.embeddings.create_async = AsyncMock(return_value=empty_response)
+
+    provider = MistralProvider(api_key="test-key", embedding_model="mistral-embed")
+    with pytest.raises(RuntimeError, match="returned no embedding"):
+        await provider.embed("test")
+
+
+@pytest.mark.unit
+async def test_mistral_embed_raises_on_null_embedding(mock_mistral_client):
+    """embed(): a single response item with embedding=None is rejected."""
+    null_item = MagicMock()
+    null_item.embedding = None
+    null_item.index = 0
+    null_response = MagicMock()
+    null_response.data = [null_item]
+    mock_mistral_client.embeddings.create_async = AsyncMock(return_value=null_response)
+
+    provider = MistralProvider(api_key="test-key", embedding_model="mistral-embed")
+    with pytest.raises(RuntimeError, match="returned no embedding"):
+        await provider.embed("test")
+
+
+@pytest.mark.unit
+async def test_mistral_batch_raises_on_null_embedding(mock_mistral_client):
+    """_embed_batch_request: a null embedding inside a batch raises explicitly."""
+    good = _make_data([0.1, 0.2], 0)
+    bad = MagicMock()
+    bad.embedding = None
+    bad.index = 1
+    response = MagicMock()
+    response.data = [good, bad]
+    mock_mistral_client.embeddings.create_async = AsyncMock(return_value=response)
+
+    provider = MistralProvider(api_key="test-key", embedding_model="mistral-embed")
+    with pytest.raises(RuntimeError, match="null embedding"):
+        await provider.embed_batch(["a", "b"])
+
+
+@pytest.mark.unit
+async def test_mistral_batch_raises_on_count_mismatch(mock_mistral_client):
+    """_embed_batch_request: fewer embeddings returned than inputs sent."""
+    # Two inputs sent, one embedding returned.
+    response = _make_response([[0.1, 0.2]])
+    mock_mistral_client.embeddings.create_async = AsyncMock(return_value=response)
+
+    provider = MistralProvider(api_key="test-key", embedding_model="mistral-embed")
+    with pytest.raises(RuntimeError, match="returned 1 embeddings for 2 inputs"):
+        await provider.embed_batch(["a", "b"])
+
+
 @pytest.mark.unit
 def test_mistral_is_rate_limit_predicate():
     """_is_rate_limit returns True only for SDKErrors with status_code == 429."""

tests/unit/providers/test_registry.py

@@ -108,8 +108,11 @@ def test_registry_openai_wins_over_mistral_and_ollama(clean_provider_env):
 
 
 @pytest.mark.unit
-def test_registry_mistral_wins_over_ollama(clean_provider_env):
+def test_registry_mistral_wins_over_ollama(clean_provider_env, mocker):
     """Mistral takes priority over Ollama when both are configured."""
+    # Stub the Mistral SDK constructor for the same reason as the sibling
+    # picker test — keeps the registry test independent of SDK key validation.
+    mocker.patch("nextcloud_mcp_server.providers.mistral.Mistral")
     clean_provider_env.setenv("MISTRAL_API_KEY", "mistral-key")
     clean_provider_env.setenv("OLLAMA_BASE_URL", "http://localhost:11434")
     _reload_config()