From 20f1770794a605785bfd3b14146630799a22c764 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 18:11:57 +0200 Subject: [PATCH] =?UTF-8?q?refactor(providers):=20address=20PR=20#772=20re?= =?UTF-8?q?view=20round=202=20=E2=80=94=20guard,=20naming,=20docs,=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - _retry.py: replace `assert last_error is not None` with explicit `if last_error is None: raise RuntimeError(...)` so the original rate-limit error is preserved under `python -O`. - openai.py: drop the `_retry_factory` alias chain; rename the bound decorator to `_retry_429` to match the pattern in mistral.py. - mistral.py: comment the imports so future reviewers understand why `from mistralai.client import …` is the canonical path on 2.x (no top-level `__init__.py`; no `mistralai.models` subpackage either). - docs/configuration.md: add `OPENAI_GENERATION_MODEL` and `OLLAMA_GENERATION_MODEL` rows to the env-var reference table. - test_mistral.py: add direct unit test for the `_is_rate_limit` predicate (429 → True, 500 → False, missing-attr → False). - test_registry.py: stub `mistralai.client.Mistral` in the registry picker test, mirroring the Ollama sibling, so the test doesn't depend on the SDK accepting arbitrary keys. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/configuration.md | 2 ++ nextcloud_mcp_server/providers/_retry.py | 3 ++- nextcloud_mcp_server/providers/mistral.py | 4 ++++ nextcloud_mcp_server/providers/openai.py | 10 +++++----- tests/unit/providers/test_mistral.py | 16 ++++++++++++++++ tests/unit/providers/test_registry.py | 5 ++++- 6 files changed, 33 insertions(+), 7 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index bb2c79bb..7608c94b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -583,10 +583,12 @@ equivalent.** Operators who need a runtime toggle should open an issue. | `VECTOR_SYNC_QUEUE_MAX_SIZE` | ⚠️ Optional | `10000` | Max queued documents | | `OLLAMA_BASE_URL` | ⚠️ Optional | - | Ollama API endpoint for embeddings | | `OLLAMA_EMBEDDING_MODEL` | ⚠️ Optional | `nomic-embed-text` | Embedding model to use | +| `OLLAMA_GENERATION_MODEL` | ⚠️ Optional | - | Ollama model for text generation | | `OLLAMA_VERIFY_SSL` | ⚠️ Optional | `true` | Verify SSL certificates | | `OPENAI_API_KEY` | ⚠️ Optional | - | OpenAI API key (selects OpenAI provider) | | `OPENAI_BASE_URL` | ⚠️ Optional | - | OpenAI base URL override (for compatible APIs) | | `OPENAI_EMBEDDING_MODEL` | ⚠️ Optional | `text-embedding-3-small` | OpenAI embedding model | +| `OPENAI_GENERATION_MODEL` | ⚠️ Optional | - | OpenAI model for text generation | | `MISTRAL_API_KEY` | ⚠️ Optional | - | Mistral API key (selects Mistral provider) | | `MISTRAL_EMBEDDING_MODEL` | ⚠️ Optional | `mistral-embed` | Mistral embedding model (1024-dim) | | `MISTRAL_BASE_URL` | ⚠️ Optional | - | Mistral base URL override (proxies, on-prem) | diff --git a/nextcloud_mcp_server/providers/_retry.py b/nextcloud_mcp_server/providers/_retry.py index d1ced096..8fb8011e 100644 --- a/nextcloud_mcp_server/providers/_retry.py +++ b/nextcloud_mcp_server/providers/_retry.py @@ -69,7 +69,8 @@ def retry_on_rate_limit( logger.error( "%s rate limit exceeded after %d attempts", provider_name, MAX_RETRIES ) - assert last_error is not None # The loop above always assigns it. + if last_error is None: # pragma: no cover — loop above always sets this + raise RuntimeError("retry loop exited without capturing an error") raise last_error return wrapper diff --git a/nextcloud_mcp_server/providers/mistral.py b/nextcloud_mcp_server/providers/mistral.py index 26702e93..b5f61584 100644 --- a/nextcloud_mcp_server/providers/mistral.py +++ b/nextcloud_mcp_server/providers/mistral.py @@ -6,6 +6,10 @@ can be added later if needed; see ADR-015. import logging +# mistralai 2.x ships no top-level __init__.py, so `from mistralai import …` +# raises ImportError. The canonical public paths are `mistralai.client` (which +# re-exports the SDK class via `client/__init__.py`) and `mistralai.client.errors` +# (which lazy-loads SDKError). There is no `mistralai.models` subpackage either. from mistralai.client import Mistral from mistralai.client.errors import SDKError diff --git a/nextcloud_mcp_server/providers/openai.py b/nextcloud_mcp_server/providers/openai.py index df0407be..1b044039 100644 --- a/nextcloud_mcp_server/providers/openai.py +++ b/nextcloud_mcp_server/providers/openai.py @@ -10,14 +10,14 @@ import logging from openai import AsyncOpenAI, RateLimitError -from ._retry import retry_on_rate_limit as _retry_factory +from ._retry import retry_on_rate_limit from .base import Provider logger = logging.getLogger(__name__) # OpenAI's RateLimitError is itself a 429-specific class, so the default # is_rate_limit predicate ("always True") matches the previous behavior. -retry_on_rate_limit = _retry_factory(RateLimitError, provider_name="OpenAI") +_retry_429 = retry_on_rate_limit(RateLimitError, provider_name="OpenAI") # Well-known embedding dimensions for OpenAI models @@ -92,7 +92,7 @@ class OpenAIProvider(Provider): """Whether this provider supports text generation.""" return self.generation_model is not None - @retry_on_rate_limit + @_retry_429 async def embed(self, text: str) -> list[float]: """ Generate embedding vector for text. @@ -173,7 +173,7 @@ class OpenAIProvider(Provider): return all_embeddings - @retry_on_rate_limit + @_retry_429 async def _embed_batch_request(self, batch: list[str]) -> list[list[float]]: """Make a single batch embedding request with retry logic.""" assert self.embedding_model is not None # Type narrowing @@ -208,7 +208,7 @@ class OpenAIProvider(Provider): ) return self._dimension - @retry_on_rate_limit + @_retry_429 async def generate(self, prompt: str, max_tokens: int = 500) -> str: """ Generate text from a prompt. diff --git a/tests/unit/providers/test_mistral.py b/tests/unit/providers/test_mistral.py index 29f4392e..d3c58c15 100644 --- a/tests/unit/providers/test_mistral.py +++ b/tests/unit/providers/test_mistral.py @@ -3,11 +3,13 @@ from unittest.mock import AsyncMock, MagicMock import pytest +from mistralai.client.errors import SDKError from nextcloud_mcp_server.providers.mistral import ( BATCH_SIZE, MISTRAL_EMBEDDING_DIMENSIONS, MistralProvider, + _is_rate_limit, ) @@ -196,3 +198,17 @@ async def test_mistral_base_url_passed_to_sdk(mocker): api_key="test-key", server_url="https://example.com/mistral", ) + + +@pytest.mark.unit +def test_mistral_is_rate_limit_predicate(): + """_is_rate_limit returns True only for SDKErrors with status_code == 429.""" + err_429 = MagicMock(spec=SDKError) + err_429.status_code = 429 + err_500 = MagicMock(spec=SDKError) + err_500.status_code = 500 + + assert _is_rate_limit(err_429) is True + assert _is_rate_limit(err_500) is False + # ValueError has no status_code attr → getattr returns None → False. + assert _is_rate_limit(ValueError()) is False diff --git a/tests/unit/providers/test_registry.py b/tests/unit/providers/test_registry.py index dc871834..b9be98f8 100644 --- a/tests/unit/providers/test_registry.py +++ b/tests/unit/providers/test_registry.py @@ -69,8 +69,11 @@ def test_registry_picks_simple_with_custom_dimension(clean_provider_env): @pytest.mark.unit -def test_registry_picks_mistral_when_api_key_set(clean_provider_env): +def test_registry_picks_mistral_when_api_key_set(clean_provider_env, mocker): """MISTRAL_API_KEY alone is enough to select MistralProvider.""" + # MistralProvider eagerly constructs the SDK client in __init__; stub it + # so the test doesn't depend on the SDK accepting arbitrary keys. + mocker.patch("nextcloud_mcp_server.providers.mistral.Mistral") clean_provider_env.setenv("MISTRAL_API_KEY", "test-key") _reload_config()