refactor(providers): address PR #772 review round 2 — guard, naming, docs, tests

- _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) <noreply@anthropic.com>

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) |

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

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
 

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.

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

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()