From adcf13f082b058be997dd1dbe1bf7c928f1576d7 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 18:28:04 +0200 Subject: [PATCH] =?UTF-8?q?refactor(providers):=20address=20PR=20#772=20re?= =?UTF-8?q?view=20round=203=20=E2=80=94=20hermetic=20test,=20lazy=20loggin?= =?UTF-8?q?g,=20defensive-guard=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- docs/configuration.md | 2 + nextcloud_mcp_server/providers/openai.py | 19 +++++--- tests/unit/providers/test_mistral.py | 55 ++++++++++++++++++++++++ tests/unit/providers/test_registry.py | 5 ++- 4 files changed, 73 insertions(+), 8 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 7608c94b..dd71e300 100644 --- a/docs/configuration.md +++ b/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 | diff --git a/nextcloud_mcp_server/providers/openai.py b/nextcloud_mcp_server/providers/openai.py index 1b044039..536697bd 100644 --- a/nextcloud_mcp_server/providers/openai.py +++ b/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 diff --git a/tests/unit/providers/test_mistral.py b/tests/unit/providers/test_mistral.py index d3c58c15..0e38da15 100644 --- a/tests/unit/providers/test_mistral.py +++ b/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.""" diff --git a/tests/unit/providers/test_registry.py b/tests/unit/providers/test_registry.py index b9be98f8..5d4c3226 100644 --- a/tests/unit/providers/test_registry.py +++ b/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()