From dc564b01bb0b3cfd23b8bdf0e170ff60df570375 Mon Sep 17 00:00:00 2001 From: Jan <1673276@uab.cat> Date: Wed, 21 Oct 2026 01:16:59 +0200 Subject: [PATCH 01/14] feat: add Ollama model pull and list downloadable models --- app/api/routes/llm.py | 65 ++++++++++++++++++++++++-- app/api/schemas/system.py | 37 ++++++++++++++- app/core/config.py | 2 + app/services/llm/__init__.py | 3 +- app/services/llm/base.py | 7 +++ app/services/llm/factory.py | 7 ++- app/services/llm/ollama.py | 88 +++++++++++++++++++++++++++++++++++- app/services/llm/registry.py | 43 +++++++++++++++++- app/services/llm/settings.py | 20 +++++++- app/services/llm/specs.py | 8 ++++ docker/.env.example | 5 ++ tests/test_llm_registry.py | 27 +++++++---- 12 files changed, 293 insertions(+), 19 deletions(-) diff --git a/app/api/routes/llm.py b/app/api/routes/llm.py index c68d45a..e625348 100644 --- a/app/api/routes/llm.py +++ b/app/api/routes/llm.py @@ -1,18 +1,31 @@ -"""LLM endpoints: GET /api/v1/system/llm/models, POST /api/v1/system/llm/test. +"""LLM endpoints: GET /api/v1/system/llm/models, POST /api/v1/system/llm/test, POST /api/v1/system/llm/pull. Contract: contracts/path/system.yaml + contracts/schemas/system.yaml """ from fastapi import APIRouter +from fastapi.responses import StreamingResponse -from app.api.schemas.system import LLMModelChoice, LLMModels, LLMProviderModels, LLMTestRequest, LLMTestResult +from app.api.schemas.enums import LLMProvider +from app.api.schemas.system import ( + DownloadableModel, + LLMModelChoice, + LLMModels, + LLMProviderModels, + LLMPullRequest, + LLMTestRequest, + LLMTestResult, + ModelInfo, +) from app.services import llm +from app.services.llm.errors import LLMNotAvailableError +from app.services.llm.ollama import OllamaLLM router = APIRouter(prefix="/system/llm", tags=["system"]) # Fields without a value are left out: hosted providers only give a model name, -# Ollama adds size, quantization and whether the model is loaded. +# Ollama adds size, quantization, whether the model is loaded, and downloadable models. @router.get( "/models", response_model=LLMModels, @@ -21,9 +34,27 @@ ) def list_llm_models(refresh: bool = False): registry = llm.get_registry() + providers: list[LLMProviderModels] = [] + for c in registry.catalog(refresh): + downloadable = ( + [DownloadableModel.model_validate(d) for d in c.downloadable] + if c.downloadable + else None + ) + providers.append( + LLMProviderModels( + provider=c.provider, + available=c.available, + default_model=c.default_model, + detail=c.detail, + error_code=c.error_code, + models=[ModelInfo.model_validate(m) for m in c.models], + downloadable=downloadable, + ) + ) return LLMModels( default=LLMModelChoice.model_validate(registry.default), - providers=[LLMProviderModels.model_validate(c) for c in registry.catalog(refresh)], + providers=providers, ) @@ -37,3 +68,29 @@ def test_llm_model(body: LLMTestRequest | None = None): body = body or LLMTestRequest() check = llm.ModelValidator(llm.get_registry()).probe(body.provider, body.model) return LLMTestResult.model_validate(check) + + +@router.post( + "/pull", + response_class=StreamingResponse, + responses={ + 200: {"content": {"application/x-ndjson": {}}}, + 422: {"description": "Invalid pull request or model not installable"}, + 503: {"description": "Ollama service unavailable"}, + }, + summary="Pull an AI model from Ollama", +) +def pull_llm_model(req: LLMPullRequest): + registry = llm.get_registry() + provider_val = req.provider or "ollama" + registry.validate_pull(provider_val, req.model) + + client = registry.client(LLMProvider.ollama) + if not isinstance(client, OllamaLLM): + raise LLMNotAvailableError("Model pulling is only supported for Ollama.") + + stream = client.pull_model( + req.model, + on_success=lambda: registry.provider_catalog(LLMProvider.ollama, refresh=True), + ) + return StreamingResponse(stream, media_type="application/x-ndjson") diff --git a/app/api/schemas/system.py b/app/api/schemas/system.py index 1949c71..65f7c2f 100644 --- a/app/api/schemas/system.py +++ b/app/api/schemas/system.py @@ -1,7 +1,23 @@ -from pydantic import BaseModel, ConfigDict +import re +from pydantic import BaseModel, ConfigDict, field_validator from app.api.schemas.enums import HealthState, LLMProvider +_MODEL_NAME_PATTERN = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._\-/:]*$") + + +def validate_model_name(value: str | None) -> str: + if value is None: + raise ValueError("model name cannot be empty") + value = value.strip() + if not value: + raise ValueError("model name cannot be empty") + if len(value) > 128: + raise ValueError("model name must be at most 128 characters") + if not _MODEL_NAME_PATTERN.fullmatch(value): + raise ValueError("model name contains invalid characters") + return value + class ModelInfo(BaseModel): model_config = ConfigDict(from_attributes=True) @@ -10,6 +26,14 @@ class ModelInfo(BaseModel): size_gb: float | None = None quantization: str | None = None loaded: bool | None = None + installed: bool | None = None + + +class DownloadableModel(BaseModel): + model_config = ConfigDict(from_attributes=True) + + name: str + installed: bool = False class CurrentLoad(BaseModel): @@ -59,6 +83,7 @@ class LLMProviderModels(BaseModel): detail: str | None = None error_code: str | None = None models: list[ModelInfo] + downloadable: list[DownloadableModel] | None = None class LLMModels(BaseModel): @@ -66,6 +91,16 @@ class LLMModels(BaseModel): providers: list[LLMProviderModels] +class LLMPullRequest(BaseModel): + model: str + provider: str | None = "ollama" + + @field_validator("model") + @classmethod + def _validate_model_name(cls, value: str) -> str: + return validate_model_name(value) + + class LLMTestRequest(BaseModel): provider: LLMProvider | None = None model: str | None = None diff --git a/app/core/config.py b/app/core/config.py index 83eaa1a..5ff0327 100644 --- a/app/core/config.py +++ b/app/core/config.py @@ -38,6 +38,8 @@ OLLAMA_HOST = os.getenv("OLLAMA_HOST", "http://localhost:11434").rstrip("/") OLLAMA_MODEL = os.getenv("OLLAMA_MODEL", "qwen2.5:1.5b") OLLAMA_TIMEOUT = int(os.getenv("OLLAMA_TIMEOUT", "300")) +OLLAMA_PULL_TIMEOUT = int(os.getenv("OLLAMA_PULL_TIMEOUT", "120")) +OLLAMA_INSTALLABLE_MODELS = os.getenv("OLLAMA_INSTALLABLE_MODELS", "").strip() # --- LLM providers -------------------------------------------------------- # Every provider with a non empty _MODELS is enabled, so several can diff --git a/app/services/llm/__init__.py b/app/services/llm/__init__.py index e4d40ad..7cb42c1 100644 --- a/app/services/llm/__init__.py +++ b/app/services/llm/__init__.py @@ -14,7 +14,7 @@ from functools import lru_cache from app.api.schemas.enums import LLMProvider -from app.services.llm.base import LLM, LLMHealth, ModelInfo +from app.services.llm.base import DownloadableModelInfo, LLM, LLMHealth, ModelInfo from app.services.llm.errors import ( LLMAuthError, LLMConfigError, @@ -38,6 +38,7 @@ def get_registry() -> LLMRegistry: __all__ = [ + "DownloadableModelInfo", "LLM", "LLMAuthError", "LLMConfigError", diff --git a/app/services/llm/base.py b/app/services/llm/base.py index d3e4cd6..4363150 100644 --- a/app/services/llm/base.py +++ b/app/services/llm/base.py @@ -17,6 +17,13 @@ class ModelInfo: size_gb: float | None = None quantization: str | None = None loaded: bool | None = None + installed: bool | None = None + + +@dataclass(frozen=True) +class DownloadableModelInfo: + name: str + installed: bool = False @dataclass(frozen=True) diff --git a/app/services/llm/factory.py b/app/services/llm/factory.py index 744b2cf..eb36981 100644 --- a/app/services/llm/factory.py +++ b/app/services/llm/factory.py @@ -10,7 +10,12 @@ def create_llm(settings: ProviderSettings) -> LLM: # Ollama has its own transport, every other provider speaks the OpenAI API if settings.provider is LLMProvider.ollama: - return OllamaLLM(host=settings.base_url, model=settings.default_model, timeout=settings.timeout) + return OllamaLLM( + host=settings.base_url, + model=settings.default_model, + timeout=settings.timeout, + pull_timeout=settings.pull_timeout, + ) return OpenAICompatibleLLM( provider=settings.provider, base_url=settings.base_url, diff --git a/app/services/llm/ollama.py b/app/services/llm/ollama.py index de58a54..bc38c4a 100644 --- a/app/services/llm/ollama.py +++ b/app/services/llm/ollama.py @@ -4,7 +4,10 @@ context size or the seed. Large forms need the 32k context, so we stay native. """ +import json +import logging import time +from collections.abc import Callable, Iterator import requests @@ -12,6 +15,8 @@ from app.services.llm.base import LLM, PROBE_TIMEOUT_SECONDS, LLMHealth, ModelInfo from app.services.llm.errors import LLMError, LLMModelNotFoundError, LLMResponseError, LLMUnavailableError +logger = logging.getLogger(__name__) + OPTIONS = { "num_ctx": 32768, # Backstop for runaway generation; the largest real output (ICS 209) is ~8k tokens @@ -24,9 +29,10 @@ class OllamaLLM(LLM): - def __init__(self, host: str, model: str | None, timeout: int): + def __init__(self, host: str, model: str | None, timeout: int, pull_timeout: int = 120): super().__init__(LLMProvider.ollama, model, timeout) self.host = host.rstrip("/") + self.pull_timeout = pull_timeout def _generate(self, prompt: str, schema: dict, model: str) -> str: # The schema goes in as `format`, so Ollama constrains the output to it @@ -83,11 +89,91 @@ def _to_models(tags: dict, running: list[str]) -> list[ModelInfo]: size_gb=round(m.get("size", 0) / (1024 ** 3), 2), quantization=m.get("details", {}).get("quantization_level"), loaded=m["name"] in running, + installed=True, ) for m in tags.get("models", []) if m.get("name") ] + def pull_model(self, model: str, on_success: Callable[[], None] | None = None) -> Iterator[str]: + """Stream pull progress from Ollama as newline-delimited JSON. + + Opens the connection before returning the generator so a down Ollama raises + LLMUnavailableError (HTTP 503) instead of committing a 200 response. + """ + response = self._open_pull_stream(model) + return self._stream_pull(response, on_success=on_success) + + def _open_pull_stream(self, model: str) -> requests.Response: + url = self.host + "/api/pull" + try: + response = requests.post( + url, + json={"model": model, "stream": True}, + stream=True, + timeout=(10, self.pull_timeout), + ) + response.raise_for_status() + return response + except requests.Timeout as exc: + raise LLMUnavailableError(f"Ollama at {self.host} timed out connecting to pull {model}") from exc + except requests.RequestException as exc: + raise LLMUnavailableError(f"Could not reach Ollama at {self.host}. Check OLLAMA_HOST: {exc}") from exc + + def _stream_pull( + self, response: requests.Response, on_success: Callable[[], None] | None = None + ) -> Iterator[str]: + saw_error = False + saw_success = False + buffer = "" + + def inspect_line(line: str) -> None: + nonlocal saw_error, saw_success + if not line.strip(): + return + try: + event = json.loads(line) + if isinstance(event, dict): + saw_error = saw_error or ("error" in event) + saw_success = saw_success or (event.get("status") == "success") + except ValueError: + pass + + try: + with response: + for chunk in response.iter_content(chunk_size=None): + if not chunk: + continue + if isinstance(chunk, bytes): + text_chunk = chunk.decode("utf-8", errors="replace") + else: + text_chunk = str(chunk) + buffer += text_chunk + lines = buffer.split("\n") + buffer = lines.pop() or "" + for line in lines: + if not line.strip(): + continue + inspect_line(line) + yield line + "\n" + + if buffer.strip(): + inspect_line(buffer) + yield buffer.strip() + "\n" + + if not saw_error and not saw_success: + yield json.dumps({"status": "success"}) + "\n" + saw_success = True + except requests.RequestException as exc: + saw_error = True + yield json.dumps({"error": str(exc)}) + "\n" + + if saw_success and not saw_error and on_success: + try: + on_success() + except Exception: + logger.exception("Failed to refresh catalog after pull") + def _request( self, method: str, path: str, timeout: int = PROBE_TIMEOUT_SECONDS, model: str | None = None, **kwargs ) -> dict: diff --git a/app/services/llm/registry.py b/app/services/llm/registry.py index 739820b..73f1d38 100644 --- a/app/services/llm/registry.py +++ b/app/services/llm/registry.py @@ -16,10 +16,11 @@ from dataclasses import dataclass, field from app.api.schemas.enums import LLMProvider -from app.services.llm.base import LLM, ModelInfo +from app.services.llm.base import DownloadableModelInfo, LLM, ModelInfo from app.services.llm.errors import LLMError, LLMNotAvailableError from app.services.llm.factory import create_llm from app.services.llm.settings import LLMSettings, ProviderSettings +from app.services.llm.specs import DEFAULT_OLLAMA_INSTALLABLE_MODELS logger = logging.getLogger(__name__) @@ -46,6 +47,7 @@ class ProviderCatalog: error_code: str | None = None # Names in _MODELS the provider doesn't offer, left out of models missing: tuple[str, ...] = () + downloadable: tuple[DownloadableModelInfo, ...] = () error: LLMError | None = field(default=None, compare=False, repr=False) def offers(self, model: str) -> bool: @@ -165,14 +167,53 @@ def _read(self, settings: ProviderSettings) -> ProviderCatalog: error=exc, ) kept, missing = settings.models.pick(offered) + offered_names = {m.name for m in offered} + + downloadable: list[DownloadableModelInfo] = [] + if settings.provider is LLMProvider.ollama: + if settings.models.wildcard: + candidate_names = settings.installable_models or DEFAULT_OLLAMA_INSTALLABLE_MODELS + downloadable = [ + DownloadableModelInfo(name=name, installed=False) + for name in candidate_names + if name not in offered_names + ] + else: + downloadable = [ + DownloadableModelInfo(name=name, installed=False) + for name in missing + ] + return ProviderCatalog( settings.provider, available=True, models=tuple(kept), default_model=settings.default_model, missing=tuple(missing), + downloadable=tuple(downloadable), ) + def validate_pull(self, provider: LLMProvider | str | None, model: str) -> None: + """Validate that provider is ollama, enabled, and model is in the installable set.""" + provider_enum = self._parse_provider(provider) + if provider_enum is not LLMProvider.ollama: + raise LLMNotAvailableError("Model pulling is only supported for Ollama.") + if LLMProvider.ollama not in self._clients: + raise LLMNotAvailableError(self._not_enabled(LLMProvider.ollama)) + + settings = self._settings_for(LLMProvider.ollama) + if settings.models.wildcard: + allowed = settings.installable_models or DEFAULT_OLLAMA_INSTALLABLE_MODELS + if model not in allowed: + raise LLMNotAvailableError( + f"{model} cannot be pulled. With OLLAMA_MODELS=*, allowed models are: {', '.join(allowed)}." + ) + else: + if not settings.models.allows(model): + raise LLMNotAvailableError( + f"{model} is not in OLLAMA_MODELS. Allowed: {', '.join(settings.models.names)}." + ) + def _settings_for(self, provider: LLMProvider) -> ProviderSettings: try: return self.settings.providers[provider] diff --git a/app/services/llm/settings.py b/app/services/llm/settings.py index ac652f7..b5e5910 100644 --- a/app/services/llm/settings.py +++ b/app/services/llm/settings.py @@ -16,7 +16,7 @@ from app.core import config from app.services.llm.errors import LLMConfigError from app.services.llm.selection import ModelSelection -from app.services.llm.specs import SPECS, ProviderSpec +from app.services.llm.specs import DEFAULT_OLLAMA_INSTALLABLE_MODELS, SPECS, ProviderSpec @dataclass(frozen=True) @@ -27,6 +27,8 @@ class ProviderSettings: default_model: str | None api_key: str = "" timeout: int = 300 + pull_timeout: int = 120 + installable_models: tuple[str, ...] = () max_retries: int = 3 temperature: float | None = None structured_output: bool = True @@ -133,6 +135,20 @@ def _load_provider( "Set LLM_ALLOW_EXTERNAL=true to allow it." ) + pull_timeout = 120 + installable_models = () + if spec.provider is LLMProvider.ollama: + pull_timeout = _parse_int( + "OLLAMA_PULL_TIMEOUT", + str(getattr(cfg, "OLLAMA_PULL_TIMEOUT", 120)), + 120, + ) or 120 + installable_raw = getattr(cfg, "OLLAMA_INSTALLABLE_MODELS", "") or env.get("INSTALLABLE_MODELS", "") + if installable_raw: + installable_models = tuple(dict.fromkeys(n.strip() for n in installable_raw.split(",") if n.strip())) + else: + installable_models = DEFAULT_OLLAMA_INSTALLABLE_MODELS + return ProviderSettings( spec=spec, base_url=base_url, @@ -140,6 +156,8 @@ def _load_provider( default_model=_default_model(spec, models, cfg, is_default), api_key=api_key, timeout=shared.timeout, + pull_timeout=pull_timeout, + installable_models=installable_models, max_retries=shared.max_retries, temperature=shared.temperature, structured_output=_parse_bool( diff --git a/app/services/llm/specs.py b/app/services/llm/specs.py index c15a2f8..d10b4f0 100644 --- a/app/services/llm/specs.py +++ b/app/services/llm/specs.py @@ -47,3 +47,11 @@ def setting(self, key: str) -> str: ), LLMProvider.custom: ProviderSpec(LLMProvider.custom, needs_key=False), } + +DEFAULT_OLLAMA_INSTALLABLE_MODELS: tuple[str, ...] = ( + "qwen2.5:1.5b", + "qwen2.5:3b", + "qwen2.5:7b", + "llama3.2:3b", +) + diff --git a/docker/.env.example b/docker/.env.example index 5c11098..6263a8b 100644 --- a/docker/.env.example +++ b/docker/.env.example @@ -53,6 +53,11 @@ OLLAMA_MODELS=* # Ollama's default model, and the one make fireform pulls. OLLAMA_MODEL=qwen2.5:1.5b OLLAMA_TIMEOUT=300 +# Read timeout in seconds for downloading models via POST /system/llm/pull. Default 120. +OLLAMA_PULL_TIMEOUT=120 +# Models that can be downloaded when OLLAMA_MODELS=*. If empty, defaults to +# qwen2.5:1.5b,qwen2.5:3b,qwen2.5:7b,llama3.2:3b. +OLLAMA_INSTALLABLE_MODELS= # OpenAI OPENAI_API_KEY= diff --git a/tests/test_llm_registry.py b/tests/test_llm_registry.py index d30be36..3da7819 100644 --- a/tests/test_llm_registry.py +++ b/tests/test_llm_registry.py @@ -320,18 +320,27 @@ def offer(*names): class TestLLMEndpoints: def test_list_models(self, client, ollama_offers): + # qwen2.5:1.5b is offered, so it won't appear in downloadable. + # qwen2.5:3b, qwen2.5:7b and llama3.2:3b are in the default installable set + # but not offered → they appear in downloadable. ollama_offers("qwen2.5:1.5b", "llama3:8b") resp = client.get(f"{API_PREFIX}/system/llm/models") assert resp.status_code == 200 - assert resp.json() == { - "default": {"provider": "ollama", "model": "qwen2.5:1.5b"}, - "providers": [{ - "provider": "ollama", - "available": True, - "default_model": "qwen2.5:1.5b", - "models": [{"name": "qwen2.5:1.5b"}, {"name": "llama3:8b"}], - }], - } + body = resp.json() + assert body["default"] == {"provider": "ollama", "model": "qwen2.5:1.5b"} + assert len(body["providers"]) == 1 + provider = body["providers"][0] + assert provider["provider"] == "ollama" + assert provider["available"] is True + assert provider["default_model"] == "qwen2.5:1.5b" + # Model names must match; extra null fields (size_gb, etc.) are fine + assert [m["name"] for m in provider["models"]] == ["qwen2.5:1.5b", "llama3:8b"] + # Downloadable: default installable set minus what Ollama already has + assert provider["downloadable"] == [ + {"name": "qwen2.5:3b", "installed": False}, + {"name": "qwen2.5:7b", "installed": False}, + {"name": "llama3.2:3b", "installed": False}, + ] def test_list_models_provider_down(self, client, monkeypatch): import requests From 44a20f08a046661e0a4d8c0812c2dd9c2253f6a4 Mon Sep 17 00:00:00 2001 From: Jan <1673276@uab.cat> Date: Wed, 21 Oct 2026 02:32:20 +0200 Subject: [PATCH 02/14] test: cover Ollama model pull, downloadable models and pull endpoint --- tests/test_llm_registry.py | 187 ++++++++++++++++++++++--- tests/test_llm_service.py | 270 +++++++++++++++++++++++++++++++++++++ 2 files changed, 438 insertions(+), 19 deletions(-) diff --git a/tests/test_llm_registry.py b/tests/test_llm_registry.py index 3da7819..e71e127 100644 --- a/tests/test_llm_registry.py +++ b/tests/test_llm_registry.py @@ -5,6 +5,7 @@ """ import pytest +import requests from sqlmodel import select from app.api.schemas.enums import InputStatus, InputType @@ -25,7 +26,7 @@ ) from app.services.llm.registry import CACHE_SECONDS, FAILURE_CACHE_SECONDS from app.services.llm.selection import ModelSelection -from app.services.llm.specs import SPECS +from app.services.llm.specs import DEFAULT_OLLAMA_INSTALLABLE_MODELS, SPECS OLLAMA, GEMINI, OPENAI = LLMProvider.ollama, LLMProvider.gemini, LLMProvider.openai @@ -66,12 +67,13 @@ def __call__(self): return self.now -def _provider(provider, models, default_model=None): +def _provider(provider, models, default_model=None, installable_models=()): return ProviderSettings( spec=SPECS[provider], base_url="http://local:1", models=ModelSelection.parse(models), default_model=default_model, + installable_models=installable_models, ) @@ -263,6 +265,82 @@ def test_generate_json_goes_to_the_picked_provider(self, registry, ollama, gemin assert ollama.generated == ["qwen"] +class TestDownloadableCatalog: + + def test_wildcard_downloadable_is_default_installable_minus_offered(self, ollama): + """OLLAMA_MODELS=* → downloadable = default installable set not yet on Ollama.""" + registry = _registry({OLLAMA: ollama}) + cat = registry.provider_catalog(OLLAMA) + offered = {m.name for m in cat.models} + expected = [ + name for name in DEFAULT_OLLAMA_INSTALLABLE_MODELS if name not in offered + ] + assert [d.name for d in cat.downloadable] == expected + assert all(d.installed is False for d in cat.downloadable) + + def test_wildcard_respects_ollama_installable_models_override(self, ollama): + settings = {OLLAMA: _provider(OLLAMA, "*", "qwen", installable_models=("tiny:1b", "other:2b"))} + registry = _registry({OLLAMA: ollama}, settings=settings) + cat = registry.provider_catalog(OLLAMA) + offered = {m.name for m in cat.models} + assert [d.name for d in cat.downloadable] == [n for n in ("tiny:1b", "other:2b") if n not in offered] + + def test_explicit_models_list_downloadable_is_missing_not_offered(self, ollama): + settings = {OLLAMA: _provider(OLLAMA, "qwen,llama,missing", "qwen")} + registry = _registry({OLLAMA: ollama}, settings=settings) + cat = registry.provider_catalog(OLLAMA) + assert cat.missing == ("missing",) + assert [d.name for d in cat.downloadable] == ["missing"] + + def test_non_ollama_has_no_downloadable_or_installed(self, ollama, gemini): + registry = _registry({OLLAMA: ollama, GEMINI: gemini}) + cat = registry.provider_catalog(GEMINI) + assert cat.downloadable == () + assert all(getattr(m, "installed", None) is None for m in cat.models) + + def test_resolve_rejects_downloadable_only_model(self, ollama): + """A model listed as downloadable but not installed cannot be used for fills.""" + settings = {OLLAMA: _provider(OLLAMA, "*", "qwen")} + ollama.offered = ["qwen"] + registry = _registry({OLLAMA: ollama}, settings=settings) + with pytest.raises(LLMNotAvailableError, match="ollama does not offer qwen2.5:3b"): + registry.resolve(OLLAMA, "qwen2.5:3b") + + +class TestValidatePull: + + def test_non_ollama_provider_rejected(self, registry): + with pytest.raises(LLMNotAvailableError, match="only supported for Ollama"): + registry.validate_pull("gemini", "g1") + + def test_ollama_disabled_rejected(self): + settings = {GEMINI: _provider(GEMINI, "g1", "g1")} + llm_settings = LLMSettings(providers=settings, default_provider=GEMINI, warnings=()) + registry = LLMRegistry(llm_settings, {GEMINI: FakeLLM(GEMINI, ["g1"])}) + with pytest.raises(LLMNotAvailableError, match="ollama is not enabled"): + registry.validate_pull("ollama", "qwen2.5:3b") + + def test_wildcard_rejects_model_outside_installable_set(self, ollama): + registry = _registry({OLLAMA: ollama}) + with pytest.raises(LLMNotAvailableError, match="cannot be pulled"): + registry.validate_pull("ollama", "secret:model") + + def test_explicit_list_rejects_model_not_in_ollama_models(self, ollama): + settings = {OLLAMA: _provider(OLLAMA, "qwen,llama", "qwen")} + registry = _registry({OLLAMA: ollama}, settings=settings) + with pytest.raises(LLMNotAvailableError, match="is not in OLLAMA_MODELS"): + registry.validate_pull("ollama", "mistral:7b") + + def test_valid_wildcard_installable_passes(self, ollama): + registry = _registry({OLLAMA: ollama}) + registry.validate_pull("ollama", "qwen2.5:3b") + + def test_valid_explicit_configured_missing_passes(self, ollama): + settings = {OLLAMA: _provider(OLLAMA, "qwen,llama", "qwen")} + registry = _registry({OLLAMA: ollama}, settings=settings) + registry.validate_pull("ollama", "llama") + + # --------------------------------------------------------------------------- # ModelValidator # --------------------------------------------------------------------------- @@ -320,27 +398,28 @@ def offer(*names): class TestLLMEndpoints: def test_list_models(self, client, ollama_offers): - # qwen2.5:1.5b is offered, so it won't appear in downloadable. - # qwen2.5:3b, qwen2.5:7b and llama3.2:3b are in the default installable set + # ollama_offers uses ModelInfo(name=n) without extra fields; FastAPI omits + # null fields, so models appear as {"name": "..."}. + # qwen2.5:1.5b is offered → not downloadable. + # qwen2.5:3b / :7b and llama3.2:3b are in the default installable set # but not offered → they appear in downloadable. ollama_offers("qwen2.5:1.5b", "llama3:8b") resp = client.get(f"{API_PREFIX}/system/llm/models") assert resp.status_code == 200 - body = resp.json() - assert body["default"] == {"provider": "ollama", "model": "qwen2.5:1.5b"} - assert len(body["providers"]) == 1 - provider = body["providers"][0] - assert provider["provider"] == "ollama" - assert provider["available"] is True - assert provider["default_model"] == "qwen2.5:1.5b" - # Model names must match; extra null fields (size_gb, etc.) are fine - assert [m["name"] for m in provider["models"]] == ["qwen2.5:1.5b", "llama3:8b"] - # Downloadable: default installable set minus what Ollama already has - assert provider["downloadable"] == [ - {"name": "qwen2.5:3b", "installed": False}, - {"name": "qwen2.5:7b", "installed": False}, - {"name": "llama3.2:3b", "installed": False}, - ] + assert resp.json() == { + "default": {"provider": "ollama", "model": "qwen2.5:1.5b"}, + "providers": [{ + "provider": "ollama", + "available": True, + "default_model": "qwen2.5:1.5b", + "models": [{"name": "qwen2.5:1.5b"}, {"name": "llama3:8b"}], + "downloadable": [ + {"name": "qwen2.5:3b", "installed": False}, + {"name": "qwen2.5:7b", "installed": False}, + {"name": "llama3.2:3b", "installed": False}, + ], + }], + } def test_list_models_provider_down(self, client, monkeypatch): import requests @@ -460,3 +539,73 @@ def test_jobs_refuse_disabled_provider(self, client, db, monkeypatch): assert resp.status_code == 422 assert queued == [] + +class TestPullEndpoint: + + _PULL = f"{API_PREFIX}/system/llm/pull" + + def test_pull_success_ndjson_and_refreshes_catalog(self, client, test_llm, monkeypatch): + import json + + offered = ["qwen2.5:1.5b"] + monkeypatch.setattr( + test_llm, "list_models", lambda: [ModelInfo(name=n) for n in offered] + ) + + def pull(model, on_success=None): + offered.append("qwen2.5:3b") + if on_success: + on_success() + yield '{"status": "pulling manifest"}\n' + yield '{"status": "success"}\n' + + monkeypatch.setattr(test_llm, "pull_model", pull) + + resp = client.post(self._PULL, json={"model": "qwen2.5:3b"}) + assert resp.status_code == 200 + assert "application/x-ndjson" in resp.headers["content-type"] + lines = [json.loads(l) for l in resp.text.strip().split("\n") if l.strip()] + assert lines[-1]["status"] == "success" + + models = client.get(f"{API_PREFIX}/system/llm/models").json()["providers"][0]["models"] + assert {"name": "qwen2.5:3b"} in models + + def test_pull_rejects_non_ollama_provider(self, client): + resp = client.post(self._PULL, json={"provider": "gemini", "model": "gemini-2.5-flash"}) + assert resp.status_code == 422 + assert resp.json()["error_code"] == "LLM_NOT_AVAILABLE" + + def test_pull_rejects_unauthorized_model(self, client): + resp = client.post(self._PULL, json={"model": "not-in-installable-set:99b"}) + assert resp.status_code == 422 + assert resp.json()["error_code"] == "LLM_NOT_AVAILABLE" + + def test_pull_rejects_invalid_model_name(self, client): + resp = client.post(self._PULL, json={"model": "bad name!"}) + assert resp.status_code == 422 + body = resp.json() + assert body["error_code"] == "VALIDATION_ERROR" + + def test_pull_503_when_ollama_down_before_stream(self, client, monkeypatch): + monkeypatch.setattr( + "app.services.llm.ollama.requests.post", + lambda *a, **k: (_ for _ in ()).throw(requests.exceptions.ConnectionError("down")), + ) + resp = client.post(self._PULL, json={"model": "qwen2.5:3b"}) + assert resp.status_code == 503 + assert resp.json()["error_code"] == "LLM_UNAVAILABLE" + + def test_pull_mid_stream_error_without_success(self, client, test_llm, monkeypatch): + import json + + def pull(model, on_success=None): + yield '{"status": "downloading"}\n' + yield '{"error": "disk full"}\n' + + monkeypatch.setattr(test_llm, "pull_model", pull) + resp = client.post(self._PULL, json={"model": "qwen2.5:3b"}) + assert resp.status_code == 200 + lines = [json.loads(l) for l in resp.text.strip().split("\n") if l.strip()] + assert any("error" in p for p in lines) + assert not any(p.get("status") == "success" for p in lines) + diff --git a/tests/test_llm_service.py b/tests/test_llm_service.py index 1c78cca..02f737e 100644 --- a/tests/test_llm_service.py +++ b/tests/test_llm_service.py @@ -368,6 +368,29 @@ def fake(method, url, **kwargs): assert health.provider == "ollama" assert "Could not reach Ollama" in health.detail + def test_list_models_marks_every_tag_as_installed(self, monkeypatch): + """Each model from /api/tags carries installed=True.""" + tags = { + "models": [ + {"name": "qwen2.5:1.5b", "size": 987_654_321, "details": {"quantization_level": "Q4_K_M"}}, + {"name": "llama3:8b", "size": 4_700_000_000}, + ] + } + + def fake(method, url, **kwargs): + if url.endswith("/api/tags"): + return _response(json_data=tags) + if url.endswith("/api/ps"): + return _response(json_data={"models": [{"name": "qwen2.5:1.5b"}]}) + raise AssertionError(f"unexpected {method} {url}") + + monkeypatch.setattr("app.services.llm.ollama.requests.request", fake) + models = self._llm().list_models() + assert [m.name for m in models] == ["qwen2.5:1.5b", "llama3:8b"] + assert all(m.installed is True for m in models) + assert models[0].loaded is True + assert models[1].loaded is False + # --------------------------------------------------------------------------- # OpenAICompatibleLLM @@ -518,3 +541,250 @@ def test_health_unhealthy_when_down(self): assert health.status == "unhealthy" assert "Could not reach custom" in health.detail assert "Check CUSTOM_BASE_URL" in health.detail + + +# --------------------------------------------------------------------------- +# OllamaLLM.pull_model — streaming, no real network +# --------------------------------------------------------------------------- + +class _FakeStreamResponse: + """Simulates requests.Response.iter_content for a pull stream.""" + + def __init__(self, chunks: list[bytes], raise_at: int | None = None, raise_exc=None): + self._chunks = chunks + self._raise_at = raise_at # raise raise_exc after this many chunks + self._raise_exc = raise_exc + self.status_code = 200 + + def raise_for_status(self): + pass + + def iter_content(self, chunk_size=None): + for i, chunk in enumerate(self._chunks): + if self._raise_at is not None and i >= self._raise_at: + raise self._raise_exc + yield chunk + + def __enter__(self): + return self + + def __exit__(self, *a): + pass + + +def _pull_llm(pull_timeout=30): + return OllamaLLM(host="http://ollama:11434", model="qwen2.5:1.5b", timeout=300, pull_timeout=pull_timeout) + + +class TestOllamaPull: + + def test_happy_path_streams_lines_and_calls_callback(self, monkeypatch): + """Lines are forwarded; callback fires once on success.""" + import json + + chunks = [ + b'{"status": "pulling manifest"}\n', + b'{"status": "downloading", "completed": 50, "total": 100}\n', + b'{"status": "success"}\n', + ] + resp = _FakeStreamResponse(chunks) + monkeypatch.setattr("app.services.llm.ollama.requests.post", lambda *a, **k: resp) + + fired = [] + lines = list(_pull_llm().pull_model("qwen2.5:3b", on_success=lambda: fired.append(1))) + + parsed = [json.loads(l) for l in lines] + assert parsed[0]["status"] == "pulling manifest" + assert parsed[-1]["status"] == "success" + assert fired == [1] + + def test_error_in_stream_no_success_no_callback(self, monkeypatch): + """An {\"error\": ...} line means no success sentinel and callback is skipped.""" + import json + + chunks = [ + b'{"status": "pulling manifest"}\n', + b'{"error": "pull access denied"}\n', + ] + resp = _FakeStreamResponse(chunks) + monkeypatch.setattr("app.services.llm.ollama.requests.post", lambda *a, **k: resp) + + fired = [] + lines = list(_pull_llm().pull_model("secret:model", on_success=lambda: fired.append(1))) + + parsed = [json.loads(l) for l in lines] + statuses = [p.get("status") for p in parsed] + assert "success" not in statuses + assert fired == [] + + def test_read_timeout_mid_stream_yields_error_no_success(self, monkeypatch): + """A ReadTimeout after the stream starts produces an error line, not success.""" + import json + + chunks = [b'{"status": "downloading"}\n', b"more"] + resp = _FakeStreamResponse(chunks, raise_at=1, raise_exc=requests.exceptions.ReadTimeout("slow")) + monkeypatch.setattr("app.services.llm.ollama.requests.post", lambda *a, **k: resp) + + fired = [] + lines = list(_pull_llm().pull_model("qwen2.5:3b", on_success=lambda: fired.append(1))) + + assert any("error" in json.loads(l) for l in lines) + assert not any(json.loads(l).get("status") == "success" for l in lines) + assert fired == [] + + @pytest.mark.parametrize("exc", [ + requests.exceptions.ChunkedEncodingError("connection broken"), + requests.exceptions.ConnectionError("connection reset"), + ]) + def test_connection_lost_mid_stream_yields_error_no_success(self, monkeypatch, exc): + """ChunkedEncodingError or ConnectionError mid-stream: error line, no success, no callback.""" + import json + + chunks = [b'{"status": "downloading"}\n', b"partial"] + resp = _FakeStreamResponse(chunks, raise_at=1, raise_exc=exc) + monkeypatch.setattr("app.services.llm.ollama.requests.post", lambda *a, **k: resp) + + fired = [] + lines = list(_pull_llm().pull_model("qwen2.5:3b", on_success=lambda: fired.append(1))) + + errors = [json.loads(l) for l in lines if "error" in json.loads(l)] + assert errors + assert errors[0]["error"] + assert not any(json.loads(l).get("status") == "success" for l in lines) + assert fired == [] + + def test_ollama_down_before_stream_raises_unavailable(self, monkeypatch): + """ConnectionError before any data → LLMUnavailableError (HTTP 503).""" + monkeypatch.setattr( + "app.services.llm.ollama.requests.post", + lambda *a, **k: (_ for _ in ()).throw(requests.exceptions.ConnectionError("down")), + ) + with pytest.raises(LLMUnavailableError, match="Could not reach Ollama"): + _pull_llm().pull_model("qwen2.5:3b") + + def test_connect_timeout_before_stream_raises_unavailable(self, monkeypatch): + """Timeout before receiving any data → LLMUnavailableError.""" + monkeypatch.setattr( + "app.services.llm.ollama.requests.post", + lambda *a, **k: (_ for _ in ()).throw(requests.exceptions.Timeout("timeout")), + ) + with pytest.raises(LLMUnavailableError): + _pull_llm().pull_model("qwen2.5:3b") + + def test_uses_correct_timeout_and_body(self, monkeypatch): + """requests.post receives timeout=(10, pull_timeout) and body with model name.""" + calls = [] + + def fake_post(url, **kwargs): + calls.append(kwargs) + return _FakeStreamResponse([b'{"status": "success"}\n']) + + monkeypatch.setattr("app.services.llm.ollama.requests.post", fake_post) + list(_pull_llm(pull_timeout=77).pull_model("llama3.2:3b")) + + assert calls[0]["timeout"] == (10, 77) + assert calls[0]["json"] == {"model": "llama3.2:3b", "stream": True} + assert calls[0]["stream"] is True + + def test_json_line_split_across_chunks(self, monkeypatch): + """A JSON line split over two chunks is reassembled correctly.""" + import json + + half1 = b'{"status": "pull' + half2 = b'ing manifest"}\n{"status": "success"}\n' + resp = _FakeStreamResponse([half1, half2]) + monkeypatch.setattr("app.services.llm.ollama.requests.post", lambda *a, **k: resp) + + lines = list(_pull_llm().pull_model("qwen2.5:3b")) + parsed = [json.loads(l) for l in lines] + statuses = [p["status"] for p in parsed] + assert "pulling manifest" in statuses + assert "success" in statuses + + def test_empty_lines_and_utf8_content(self, monkeypatch): + """Empty lines are skipped; UTF-8 bytes are decoded.""" + import json + + chunks = [ + b"\n", + b'{"status": "verifying checksum"}\n', + b"\n\n", + b'{"status": "success"}\n', + ] + resp = _FakeStreamResponse(chunks) + monkeypatch.setattr("app.services.llm.ollama.requests.post", lambda *a, **k: resp) + + lines = list(_pull_llm().pull_model("qwen2.5:3b")) + # No empty lines yielded + assert all(l.strip() for l in lines) + parsed = [json.loads(l) for l in lines] + assert parsed[-1]["status"] == "success" + + def test_no_success_event_adds_sentinel(self, monkeypatch): + """Stream that ends without {\"status\":\"success\"} gets one appended.""" + import json + + chunks = [b'{"status": "pulling manifest"}\n'] + resp = _FakeStreamResponse(chunks) + monkeypatch.setattr("app.services.llm.ollama.requests.post", lambda *a, **k: resp) + + fired = [] + lines = list(_pull_llm().pull_model("qwen2.5:3b", on_success=lambda: fired.append(1))) + + assert json.loads(lines[-1])["status"] == "success" + assert fired == [1] + + +# --------------------------------------------------------------------------- +# load_settings: Ollama pull_timeout and installable_models +# --------------------------------------------------------------------------- + +class TestOllamaSettings: + + def _ollama_cfg(self, **overrides): + base = { + "OLLAMA_MODEL": "qwen2.5:1.5b", + "OLLAMA_TIMEOUT": 300, + "OLLAMA_PULL_TIMEOUT": 120, + "OLLAMA_INSTALLABLE_MODELS": "", + "LLM_PROVIDER": "ollama", + "LLM_MODEL": "", + "LLM_TIMEOUT": "", + "LLM_MAX_RETRIES": "", + "LLM_ALLOW_EXTERNAL": "false", + "LLM_TEMPERATURE": "0", + "LLM_PROVIDER_ENV": {"ollama": {"BASE_URL": "http://ollama:11434", "MODELS": "*"}}, + } + base.update(overrides) + return SimpleNamespace(**base) + + def test_default_pull_timeout(self): + settings = load_settings(self._ollama_cfg()) + assert settings.providers[LLMProvider.ollama].pull_timeout == 120 + + def test_custom_pull_timeout(self): + settings = load_settings(self._ollama_cfg(OLLAMA_PULL_TIMEOUT=60)) + assert settings.providers[LLMProvider.ollama].pull_timeout == 60 + + def test_invalid_pull_timeout_names_setting(self): + with pytest.raises(LLMConfigError, match="OLLAMA_PULL_TIMEOUT"): + load_settings(self._ollama_cfg(OLLAMA_PULL_TIMEOUT="fast")) + + def test_empty_installable_uses_default_list(self): + from app.services.llm.specs import DEFAULT_OLLAMA_INSTALLABLE_MODELS + settings = load_settings(self._ollama_cfg(OLLAMA_INSTALLABLE_MODELS="")) + assert settings.providers[LLMProvider.ollama].installable_models == DEFAULT_OLLAMA_INSTALLABLE_MODELS + + def test_explicit_installable_models_parsed(self): + settings = load_settings(self._ollama_cfg(OLLAMA_INSTALLABLE_MODELS="llama3.2:3b, qwen2.5:7b")) + assert settings.providers[LLMProvider.ollama].installable_models == ("llama3.2:3b", "qwen2.5:7b") + + def test_installable_models_deduplication(self): + settings = load_settings(self._ollama_cfg(OLLAMA_INSTALLABLE_MODELS="a:1b,a:1b,b:2b")) + assert settings.providers[LLMProvider.ollama].installable_models == ("a:1b", "b:2b") + + def test_create_llm_passes_pull_timeout(self): + settings = load_settings(self._ollama_cfg(OLLAMA_PULL_TIMEOUT=45)) + llm = create_llm(settings.providers[LLMProvider.ollama]) + assert isinstance(llm, OllamaLLM) + assert llm.pull_timeout == 45 From 830337685fe6acd657cdea0c7d6b5aabd8a12e05 Mon Sep 17 00:00:00 2001 From: Jan <1673276@uab.cat> Date: Wed, 21 Oct 2026 02:32:21 +0200 Subject: [PATCH 03/14] docs: add llm pull endpoint and downloadable models to contracts --- contracts/openapi.yaml | 2 + contracts/path/system.yaml | 78 +++++++++++++++++++++++++++++++++++ contracts/schemas/system.yaml | 37 +++++++++++++++++ 3 files changed, 117 insertions(+) diff --git a/contracts/openapi.yaml b/contracts/openapi.yaml index ae2ad41..3ebc8f4 100644 --- a/contracts/openapi.yaml +++ b/contracts/openapi.yaml @@ -66,6 +66,8 @@ paths: $ref: "path/system.yaml#/llm_models" /api/v1/system/llm/test: $ref: "path/system.yaml#/llm_test" + /api/v1/system/llm/pull: + $ref: "path/system.yaml#/llm_pull" # ── Layer 8: Async Jobs ──────────────────────────────────────── /api/v1/jobs/{job_id}: diff --git a/contracts/path/system.yaml b/contracts/path/system.yaml index 08c5728..aeadbb0 100644 --- a/contracts/path/system.yaml +++ b/contracts/path/system.yaml @@ -4,6 +4,7 @@ # GET /api/v1/schema/incident/versions # GET /api/v1/system/llm/models # POST /api/v1/system/llm/test +# POST /api/v1/system/llm/pull health: get: @@ -197,6 +198,12 @@ llm_models: size_gb: 0.92 quantization: "Q4_K_M" loaded: true + installed: true + downloadable: + - name: "qwen2.5:3b" + installed: false + - name: "llama3.2:3b" + installed: false - provider: "gemini" available: true default_model: "gemini-2.5-flash" @@ -281,3 +288,74 @@ llm_test: example: error_code: "LLM_CONFIG_ERROR" message: "GEMINI_API_KEY is set but GEMINI_MODELS is empty. Add GEMINI_MODELS=gemini-2.5-flash, a comma separated list, or * for every model gemini lists." + +llm_pull: + post: + operationId: pullLLMModel + summary: Pull an AI model from Ollama + description: | + Streams Ollama pull progress as newline-delimited JSON (application/x-ndjson). + HTTP 200 only means the stream started; a pull succeeded only when a line + contains `"status":"success"`. An `"error"` line or a truncated stream means + failure even though the response status is 200. The provider must be ollama + (default). The model must be allowed by OLLAMA_MODELS and, when + OLLAMA_MODELS=*, by OLLAMA_INSTALLABLE_MODELS. After a successful pull the + model catalog cache is refreshed so GET /system/llm/models lists the new + model without waiting for the cache TTL. + tags: + - system + requestBody: + required: true + content: + application/json: + schema: + $ref: "../schemas/system.yaml#/LLMPullRequest" + example: + provider: "ollama" + model: "qwen2.5:3b" + responses: + "200": + description: Pull stream (success or failure is in the NDJSON lines) + content: + application/x-ndjson: + schema: + type: string + description: One JSON object per line, as returned by Ollama /api/pull + examples: + success: + summary: Successful pull + value: | + {"status": "pulling manifest"} + {"status": "downloading", "completed": 50, "total": 100} + {"status": "success"} + failed: + summary: Pull failed mid-stream + value: | + {"status": "pulling manifest"} + {"error": "pull access denied"} + "422": + description: Invalid model name, provider is not ollama, or model not installable + content: + application/json: + schema: + $ref: "../schemas/common.yaml#/ErrorResponse" + examples: + not_installable: + summary: Model not in the installable set + value: + error_code: "LLM_NOT_AVAILABLE" + message: "secret:model cannot be pulled. With OLLAMA_MODELS=*, allowed models are: qwen2.5:1.5b, qwen2.5:3b, qwen2.5:7b, llama3.2:3b." + validation: + summary: Invalid model name + value: + error_code: "VALIDATION_ERROR" + message: "Request validation failed" + "503": + description: Ollama could not be reached before the stream started + content: + application/json: + schema: + $ref: "../schemas/common.yaml#/ErrorResponse" + example: + error_code: "LLM_UNAVAILABLE" + message: "Could not reach Ollama at http://ollama:11434. Check OLLAMA_HOST: Connection refused" diff --git a/contracts/schemas/system.yaml b/contracts/schemas/system.yaml index e4f1419..f174c3e 100644 --- a/contracts/schemas/system.yaml +++ b/contracts/schemas/system.yaml @@ -87,6 +87,22 @@ LLMModel: type: boolean nullable: true description: Only Ollama reports this + installed: + type: boolean + nullable: true + description: True when the model is present on the Ollama host (Ollama only) + +DownloadableModel: + type: object + required: + - name + - installed + properties: + name: + type: string + installed: + type: boolean + description: Always false in downloadable; the model is not on Ollama yet LLMModelChoice: type: object @@ -135,6 +151,27 @@ LLMProviderModels: out of the response, here and on the provider. items: $ref: "#/LLMModel" + downloadable: + type: array + nullable: true + description: Ollama models that may be pulled but are not installed yet. + Omitted for non-Ollama providers and when empty. + items: + $ref: "#/DownloadableModel" + +LLMPullRequest: + type: object + required: + - model + properties: + model: + type: string + description: Ollama model tag to pull + provider: + type: string + nullable: true + default: ollama + description: Must be ollama; other providers are rejected LLMModels: type: object From ab9473864ba6ddedeb99e80ed48216b730eb1940 Mon Sep 17 00:00:00 2001 From: Jan <1673276@uab.cat> Date: Wed, 21 Oct 2026 02:32:21 +0200 Subject: [PATCH 04/14] docs: document model pull, installable models and pull timeout --- docs/multi-llm-support/api.md | 43 +++++++++++++++++++++-- docs/multi-llm-support/configuration.md | 12 +++++++ docs/multi-llm-support/troubleshooting.md | 2 +- 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/docs/multi-llm-support/api.md b/docs/multi-llm-support/api.md index c819a69..c893b28 100644 --- a/docs/multi-llm-support/api.md +++ b/docs/multi-llm-support/api.md @@ -21,8 +21,13 @@ Add `?refresh=true` to skip the cache. Lists are cached for 60 seconds, failures "name": "qwen2.5:1.5b", "size_gb": 0.92, "quantization": "Q4_K_M", - "loaded": false + "loaded": false, + "installed": true } + ], + "downloadable": [ + { "name": "qwen2.5:3b", "installed": false }, + { "name": "llama3.2:3b", "installed": false } ] }, { @@ -43,10 +48,42 @@ Add `?refresh=true` to skip the cache. Lists are cached for 60 seconds, failures ``` - Only enabled providers appear. -- Hosted models carry `name` only. Ollama adds `size_gb`, `quantization` and `loaded`. -- With an explicit `_MODELS` list, names the provider does not offer are left out. +- Hosted models carry `name` only. Ollama adds `size_gb`, `quantization`, `loaded`, and `installed`. +- `downloadable` lists Ollama models that may be pulled but are not on the host yet (`installed: false`). It appears only for Ollama. Hosted providers omit `downloadable` and `installed`. +- With an explicit `_MODELS` list, names the provider does not offer are left out of `models` and appear in `downloadable` (Ollama only). - Empty fields are left out of the response. +## Pull an Ollama model + +`POST /api/v1/system/llm/pull` + +Streams Ollama `/api/pull` progress as **application/x-ndjson** (one JSON object per line). The body defaults `provider` to `ollama`. + +```json +{ "model": "qwen2.5:3b" } +``` + +**HTTP 200 does not mean the pull succeeded.** Treat the stream as successful only when a line contains `"status":"success"`. An `"error"` field or a connection drop without that line is a failure, even though the status code is 200. + +Successful stream: + +```text +{"status": "pulling manifest"} +{"status": "downloading", "completed": 50, "total": 100} +{"status": "success"} +``` + +Failed stream (still HTTP 200): + +```text +{"status": "pulling manifest"} +{"error": "pull access denied"} +``` + +If Ollama cannot be reached before the stream opens, the API returns **503** with `LLM_UNAVAILABLE`. Wrong provider, model not in the installable set, or an invalid model name returns **422**. + +After a successful pull, the server refreshes the Ollama catalog cache, so the new model shows up on `GET /system/llm/models` immediately (no need to wait 60 seconds or pass `refresh=true`). + ## Test a model `POST /api/v1/system/llm/test` diff --git a/docs/multi-llm-support/configuration.md b/docs/multi-llm-support/configuration.md index 3d45624..c5999e0 100644 --- a/docs/multi-llm-support/configuration.md +++ b/docs/multi-llm-support/configuration.md @@ -40,6 +40,18 @@ Every LLM setting, as read from the env file. Empty means unset. | `OLLAMA_MODELS` | `*` | Every pulled model. Empty turns Ollama off. | | `OLLAMA_MODEL` | `qwen2.5:1.5b` | Ollama's default model, and the one `make fireform` pulls. | | `OLLAMA_TIMEOUT` | `300` | Fallback for `LLM_TIMEOUT`. | +| `OLLAMA_PULL_TIMEOUT` | `120` | Inactivity timeout while streaming a pull (seconds). Not a total time limit: the connect phase uses 10 seconds; each chunk from Ollama resets the inactivity clock. | +| `OLLAMA_INSTALLABLE_MODELS` | empty (built-in default list) | Comma-separated models users may pull when `OLLAMA_MODELS=*`. Ignored when `OLLAMA_MODELS` is an explicit list; then only names in that list can be pulled. | + +## Ollama model states + +| Term | Meaning | +| ---- | ------- | +| **Default model** | `OLLAMA_MODEL` or `LLM_MODEL` when Ollama is the default provider. Used when a request names no model. | +| **Allowed model** | Named in `OLLAMA_MODELS`, or any tag when `OLLAMA_MODELS=*`. | +| **Installed** | Present on the Ollama host (`ollama list`). Shown in `models` with `installed: true`. | +| **Loaded** | Loaded in memory right now. Ollama adds `loaded: true` on `models`. | +| **Downloadable** | Allowed and not yet installed. Listed under `downloadable` on `GET /system/llm/models`. With `OLLAMA_MODELS=*`, candidates come from `OLLAMA_INSTALLABLE_MODELS` (or the built-in default). With an explicit list, downloadable entries are configured names Ollama does not have yet. | ## Provider defaults diff --git a/docs/multi-llm-support/troubleshooting.md b/docs/multi-llm-support/troubleshooting.md index f0dab79..f13f7d3 100644 --- a/docs/multi-llm-support/troubleshooting.md +++ b/docs/multi-llm-support/troubleshooting.md @@ -22,7 +22,7 @@ Check `make logs-app` for the app and `make logs-worker` for queued fills. Every | Code | Usual cause | Fix | | --------------------- | ----------------------------------------------------- | -------------------------------------------------------------------------------- | | `LLM_AUTH_FAILED` | Wrong or expired key | Replace `_API_KEY`, then `make up`. | -| `LLM_MODEL_NOT_FOUND` | Typo, retired model, or not pulled in Ollama | Pick a model from `GET /api/v1/system/llm/models`, or `ollama pull `. | +| `LLM_MODEL_NOT_FOUND` | Typo, retired model, or not pulled in Ollama | Pick a model from `GET /api/v1/system/llm/models`. If it appears under `downloadable`, install it with `POST /api/v1/system/llm/pull` (when the server allows that model), or run `ollama pull ` on the host. | | `LLM_UNAVAILABLE` | Server down, wrong URL, or timeout | Check `_BASE_URL` or `OLLAMA_HOST`. Raise `LLM_TIMEOUT` for slow models. | | `LLM_RATE_LIMITED` | Provider quota or rate limit | Wait, or check the plan on the provider side. | | `LLM_BAD_RESPONSE` | Server rejected an option, or the answer was cut off | Set `_STRUCTURED_OUTPUT=false`, or raise `_MAX_TOKENS`. | From 7c5c2dfbc97d2d04c0d2a46a5b02e8c3092c05ed Mon Sep 17 00:00:00 2001 From: Jan <1673276@uab.cat> Date: Wed, 21 Oct 2026 03:01:22 +0200 Subject: [PATCH 05/14] fix: report pull success when Ollama sends it and clarify pull success --- app/services/llm/ollama.py | 35 +++++++++++++++++------------ contracts/schemas/system.yaml | 2 +- docs/multi-llm-support/api.md | 2 +- tests/test_llm_registry.py | 24 +++++++++++++++++--- tests/test_llm_service.py | 42 ++++++++++++++++++++++++++++++----- 5 files changed, 80 insertions(+), 25 deletions(-) diff --git a/app/services/llm/ollama.py b/app/services/llm/ollama.py index bc38c4a..7ca8fdd 100644 --- a/app/services/llm/ollama.py +++ b/app/services/llm/ollama.py @@ -124,20 +124,24 @@ def _stream_pull( self, response: requests.Response, on_success: Callable[[], None] | None = None ) -> Iterator[str]: saw_error = False - saw_success = False + success_line: str | None = None buffer = "" - def inspect_line(line: str) -> None: - nonlocal saw_error, saw_success + def inspect_line(line: str) -> bool: + nonlocal saw_error, success_line if not line.strip(): - return + return False try: event = json.loads(line) if isinstance(event, dict): saw_error = saw_error or ("error" in event) - saw_success = saw_success or (event.get("status") == "success") + if event.get("status") == "success": + if success_line is None: + success_line = line.strip() + return True except ValueError: pass + return False try: with response: @@ -154,25 +158,28 @@ def inspect_line(line: str) -> None: for line in lines: if not line.strip(): continue - inspect_line(line) - yield line + "\n" + if not inspect_line(line): + yield line + "\n" if buffer.strip(): - inspect_line(buffer) - yield buffer.strip() + "\n" - - if not saw_error and not saw_success: - yield json.dumps({"status": "success"}) + "\n" - saw_success = True + if not inspect_line(buffer): + yield buffer.strip() + "\n" except requests.RequestException as exc: saw_error = True yield json.dumps({"error": str(exc)}) + "\n" - if saw_success and not saw_error and on_success: + if saw_error: + return + if success_line is None: + yield json.dumps({"error": "Ollama closed the stream before reporting success"}) + "\n" + return + + if on_success: try: on_success() except Exception: logger.exception("Failed to refresh catalog after pull") + yield success_line + "\n" def _request( self, method: str, path: str, timeout: int = PROBE_TIMEOUT_SECONDS, model: str | None = None, **kwargs diff --git a/contracts/schemas/system.yaml b/contracts/schemas/system.yaml index f174c3e..d8ff589 100644 --- a/contracts/schemas/system.yaml +++ b/contracts/schemas/system.yaml @@ -155,7 +155,7 @@ LLMProviderModels: type: array nullable: true description: Ollama models that may be pulled but are not installed yet. - Omitted for non-Ollama providers and when empty. + Omitted for non-Ollama providers and when no models are downloadable. items: $ref: "#/DownloadableModel" diff --git a/docs/multi-llm-support/api.md b/docs/multi-llm-support/api.md index c893b28..fb33ad1 100644 --- a/docs/multi-llm-support/api.md +++ b/docs/multi-llm-support/api.md @@ -49,7 +49,7 @@ Add `?refresh=true` to skip the cache. Lists are cached for 60 seconds, failures - Only enabled providers appear. - Hosted models carry `name` only. Ollama adds `size_gb`, `quantization`, `loaded`, and `installed`. -- `downloadable` lists Ollama models that may be pulled but are not on the host yet (`installed: false`). It appears only for Ollama. Hosted providers omit `downloadable` and `installed`. +- `downloadable` lists Ollama models that may be pulled but are not on the host yet (`installed: false`). It appears only for Ollama and is omitted when no models are downloadable (for example, when all installable models are already installed). Hosted providers omit `downloadable` and `installed`. - With an explicit `_MODELS` list, names the provider does not offer are left out of `models` and appear in `downloadable` (Ollama only). - Empty fields are left out of the response. diff --git a/tests/test_llm_registry.py b/tests/test_llm_registry.py index e71e127..819fbe3 100644 --- a/tests/test_llm_registry.py +++ b/tests/test_llm_registry.py @@ -421,6 +421,15 @@ def test_list_models(self, client, ollama_offers): }], } + def test_downloadable_omitted_when_all_installable_models_are_installed(self, client, ollama_offers): + ollama_offers(*DEFAULT_OLLAMA_INSTALLABLE_MODELS) + + resp = client.get(f"{API_PREFIX}/system/llm/models") + + assert resp.status_code == 200 + provider = resp.json()["providers"][0] + assert "downloadable" not in provider + def test_list_models_provider_down(self, client, monkeypatch): import requests @@ -552,12 +561,21 @@ def test_pull_success_ndjson_and_refreshes_catalog(self, client, test_llm, monke test_llm, "list_models", lambda: [ModelInfo(name=n) for n in offered] ) + before = client.get(f"{API_PREFIX}/system/llm/models") + assert before.status_code == 200 + before_models = before.json()["providers"][0]["models"] + assert {"name": "qwen2.5:3b"} not in before_models + + events = [] + def pull(model, on_success=None): + yield '{"status": "pulling manifest"}\n' + yield '{"status": "success"}\n' + events.append("success-line-consumed") offered.append("qwen2.5:3b") if on_success: on_success() - yield '{"status": "pulling manifest"}\n' - yield '{"status": "success"}\n' + events.append("catalog-refreshed") monkeypatch.setattr(test_llm, "pull_model", pull) @@ -566,6 +584,7 @@ def pull(model, on_success=None): assert "application/x-ndjson" in resp.headers["content-type"] lines = [json.loads(l) for l in resp.text.strip().split("\n") if l.strip()] assert lines[-1]["status"] == "success" + assert events == ["success-line-consumed", "catalog-refreshed"] models = client.get(f"{API_PREFIX}/system/llm/models").json()["providers"][0]["models"] assert {"name": "qwen2.5:3b"} in models @@ -608,4 +627,3 @@ def pull(model, on_success=None): lines = [json.loads(l) for l in resp.text.strip().split("\n") if l.strip()] assert any("error" in p for p in lines) assert not any(p.get("status") == "success" for p in lines) - diff --git a/tests/test_llm_service.py b/tests/test_llm_service.py index 02f737e..f0cdd49 100644 --- a/tests/test_llm_service.py +++ b/tests/test_llm_service.py @@ -598,12 +598,13 @@ def test_happy_path_streams_lines_and_calls_callback(self, monkeypatch): assert parsed[-1]["status"] == "success" assert fired == [1] - def test_error_in_stream_no_success_no_callback(self, monkeypatch): - """An {\"error\": ...} line means no success sentinel and callback is skipped.""" + def test_error_after_success_suppresses_success_and_callback(self, monkeypatch): + """An error anywhere in the stream prevents success, even after Ollama's success line.""" import json chunks = [ b'{"status": "pulling manifest"}\n', + b'{"status": "success"}\n', b'{"error": "pull access denied"}\n', ] resp = _FakeStreamResponse(chunks) @@ -720,8 +721,8 @@ def test_empty_lines_and_utf8_content(self, monkeypatch): parsed = [json.loads(l) for l in lines] assert parsed[-1]["status"] == "success" - def test_no_success_event_adds_sentinel(self, monkeypatch): - """Stream that ends without {\"status\":\"success\"} gets one appended.""" + def test_no_success_event_returns_error_without_callback(self, monkeypatch): + """A clean EOF without a success event is an error, not an implicit success.""" import json chunks = [b'{"status": "pulling manifest"}\n'] @@ -731,8 +732,37 @@ def test_no_success_event_adds_sentinel(self, monkeypatch): fired = [] lines = list(_pull_llm().pull_model("qwen2.5:3b", on_success=lambda: fired.append(1))) - assert json.loads(lines[-1])["status"] == "success" - assert fired == [1] + parsed = [json.loads(line) for line in lines] + assert parsed[-1] == {"error": "Ollama closed the stream before reporting success"} + assert not any(event.get("status") == "success" for event in parsed) + assert fired == [] + + def test_ollama_success_is_forwarded_once_after_callback(self, monkeypatch): + """Duplicate Ollama success events produce one line after the callback.""" + import json + + chunks = [ + b'{"status": "pulling manifest"}\n', + b'{"status": "success"}\n', + b'{"status": "success"}\n', + ] + resp = _FakeStreamResponse(chunks) + monkeypatch.setattr("app.services.llm.ollama.requests.post", lambda *a, **k: resp) + + events = [] + lines = [] + + def on_success(): + events.append("callback") + + for line in _pull_llm().pull_model("qwen2.5:3b", on_success=on_success): + lines.append(line) + if json.loads(line).get("status") == "success": + events.append("success") + + parsed = [json.loads(line) for line in lines] + assert [event for event in parsed if event.get("status") == "success"] == [{"status": "success"}] + assert events == ["callback", "success"] # --------------------------------------------------------------------------- From bcc666610dae119be4a52da54903e51c82588a3d Mon Sep 17 00:00:00 2001 From: Jan <1673276@uab.cat> Date: Wed, 21 Oct 2026 11:56:54 +0200 Subject: [PATCH 06/14] docs: clarify OLLAMA_PULL_TIMEOUT in .env.example --- docker/.env.example | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docker/.env.example b/docker/.env.example index 6263a8b..8d77a21 100644 --- a/docker/.env.example +++ b/docker/.env.example @@ -53,7 +53,9 @@ OLLAMA_MODELS=* # Ollama's default model, and the one make fireform pulls. OLLAMA_MODEL=qwen2.5:1.5b OLLAMA_TIMEOUT=300 -# Read timeout in seconds for downloading models via POST /system/llm/pull. Default 120. +# Seconds to wait without receiving any progress from Ollama before a model +# download via POST /system/llm/pull is aborted. Not a limit on the total +# download time (connecting is limited to 10 s). Default 120. OLLAMA_PULL_TIMEOUT=120 # Models that can be downloaded when OLLAMA_MODELS=*. If empty, defaults to # qwen2.5:1.5b,qwen2.5:3b,qwen2.5:7b,llama3.2:3b. From d43ec3a02cdf263737475437529a4e02ab711b17 Mon Sep 17 00:00:00 2001 From: Jan <1673276@uab.cat> Date: Sat, 24 Oct 2026 11:52:22 +0200 Subject: [PATCH 07/14] feat: add encrypted provider credentials, manual models and external consent --- .gitignore | 3 + .../006_add_llm_user_configuration.py | 52 +++ app/api/router.py | 3 +- app/api/routes/jobs.py | 3 +- app/api/routes/llm.py | 15 +- app/api/routes/llm_config.py | 363 ++++++++++++++++++ app/api/schemas/system.py | 65 +++- app/core/config.py | 7 + app/core/errors/handlers.py | 2 +- app/models/__init__.py | 6 + app/models/models.py | 33 +- app/services/form.py | 10 +- app/services/form_filler/filler.py | 9 +- app/services/llm/__init__.py | 7 + app/services/llm/base.py | 1 + app/services/llm/errors.py | 14 + app/services/llm/openai_compatible.py | 18 +- app/services/llm/registry.py | 32 +- app/services/llm/settings.py | 14 +- app/services/llm/user_config.py | 277 +++++++++++++ app/services/llm/validator.py | 7 +- docker/.env.example | 5 + docker/dev/compose.yml | 4 + docker/prod/compose.yml | 4 + requirements.txt | 1 + tests/test_llm_service.py | 119 ++++++ 26 files changed, 1035 insertions(+), 39 deletions(-) create mode 100644 alembic/versions/006_add_llm_user_configuration.py create mode 100644 app/api/routes/llm_config.py create mode 100644 app/services/llm/user_config.py diff --git a/.gitignore b/.gitignore index d0b1234..673b21d 100644 --- a/.gitignore +++ b/.gitignore @@ -45,3 +45,6 @@ benchmark/results/ data/inputs/* !data/inputs/ics_209.pdf !data/inputs/file_template_manual.pdf + +# Locally generated LLM credential-encryption key +data/secrets/ diff --git a/alembic/versions/006_add_llm_user_configuration.py b/alembic/versions/006_add_llm_user_configuration.py new file mode 100644 index 0000000..d3c459a --- /dev/null +++ b/alembic/versions/006_add_llm_user_configuration.py @@ -0,0 +1,52 @@ +"""add local LLM provider configuration + +Revision ID: 006 +Revises: 005 +""" +from typing import Sequence + +import sqlalchemy as sa +from alembic import op + + +revision: str = "006" +down_revision: str | None = "005" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + op.create_table( + "llm_provider_credentials", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("provider", sa.String(), nullable=False), + sa.Column("api_key_encrypted", sa.String(), nullable=True), + sa.Column("key_hint", sa.String(), nullable=True), + sa.Column("base_url", sa.String(), nullable=True), + sa.Column("structured_output", sa.Boolean(), nullable=True), + sa.Column("updated_at", sa.DateTime(timezone=True), nullable=False), + sa.PrimaryKeyConstraint("id"), + sa.UniqueConstraint("provider", name="uq_llm_provider_credential_provider"), + ) + op.create_table( + "llm_manual_models", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("provider", sa.String(), nullable=False), + sa.Column("model", sa.String(), nullable=False), + sa.Column("created_at", sa.DateTime(timezone=True), nullable=False), + sa.PrimaryKeyConstraint("id"), + sa.UniqueConstraint("provider", "model", name="uq_llm_manual_model_provider_model"), + ) + op.create_table( + "llm_runtime_config", + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("allow_external", sa.Boolean(), nullable=False), + sa.Column("updated_at", sa.DateTime(timezone=True), nullable=False), + sa.PrimaryKeyConstraint("id"), + ) + + +def downgrade() -> None: + op.drop_table("llm_runtime_config") + op.drop_table("llm_manual_models") + op.drop_table("llm_provider_credentials") diff --git a/app/api/router.py b/app/api/router.py index 0515c63..79f8396 100644 --- a/app/api/router.py +++ b/app/api/router.py @@ -1,6 +1,6 @@ from fastapi import APIRouter -from app.api.routes import forms, input, jobs, llm, system, templates, weather, zipcode +from app.api.routes import forms, input, jobs, llm, llm_config, system, templates, weather, zipcode from app.core.config import API_PREFIX api_router = APIRouter() @@ -8,6 +8,7 @@ api_router.include_router(forms.router, prefix=API_PREFIX) api_router.include_router(system.router, prefix=API_PREFIX) api_router.include_router(llm.router, prefix=API_PREFIX) +api_router.include_router(llm_config.router, prefix=API_PREFIX) api_router.include_router(jobs.router, prefix=API_PREFIX) api_router.include_router(weather.router, prefix=API_PREFIX) api_router.include_router(zipcode.router, prefix=API_PREFIX) diff --git a/app/api/routes/jobs.py b/app/api/routes/jobs.py index 2280215..6cecafa 100644 --- a/app/api/routes/jobs.py +++ b/app/api/routes/jobs.py @@ -47,7 +47,7 @@ def submit_async_form_fill(form: AsyncFormFill, db: Session = Depends(get_db)): transcript = InputService().resolve_transcript(db, form.input_id) # Checked here so a bad pick fails the request, not every queued job - choice = llm.get_registry().resolve(form.provider, form.model) + choice = llm.effective_registry(db).resolve(form.provider, form.model) jobs: list[AsyncJobSubmitResponse] = [] for tid in form.template_ids: @@ -100,4 +100,3 @@ def submit_async_form_fill(form: AsyncFormFill, db: Session = Depends(get_db)): )) return AsyncFormFillResponse(jobs=jobs) - diff --git a/app/api/routes/llm.py b/app/api/routes/llm.py index e625348..3a206ed 100644 --- a/app/api/routes/llm.py +++ b/app/api/routes/llm.py @@ -4,8 +4,11 @@ """ from fastapi import APIRouter +from fastapi import Depends from fastapi.responses import StreamingResponse +from sqlmodel import Session +from app.api.deps import get_db from app.api.schemas.enums import LLMProvider from app.api.schemas.system import ( DownloadableModel, @@ -32,8 +35,8 @@ response_model_exclude_none=True, summary="List the LLM providers and models that can fill forms", ) -def list_llm_models(refresh: bool = False): - registry = llm.get_registry() +def list_llm_models(refresh: bool = False, db: Session = Depends(get_db)): + registry = llm.effective_registry(db) providers: list[LLMProviderModels] = [] for c in registry.catalog(refresh): downloadable = ( @@ -64,9 +67,9 @@ def list_llm_models(refresh: bool = False): response_model_exclude_none=True, summary="Check that a model actually answers", ) -def test_llm_model(body: LLMTestRequest | None = None): +def test_llm_model(body: LLMTestRequest | None = None, db: Session = Depends(get_db)): body = body or LLMTestRequest() - check = llm.ModelValidator(llm.get_registry()).probe(body.provider, body.model) + check = llm.ModelValidator(llm.effective_registry(db)).probe(body.provider, body.model) return LLMTestResult.model_validate(check) @@ -80,8 +83,8 @@ def test_llm_model(body: LLMTestRequest | None = None): }, summary="Pull an AI model from Ollama", ) -def pull_llm_model(req: LLMPullRequest): - registry = llm.get_registry() +def pull_llm_model(req: LLMPullRequest, db: Session = Depends(get_db)): + registry = llm.effective_registry(db) provider_val = req.provider or "ollama" registry.validate_pull(provider_val, req.model) diff --git a/app/api/routes/llm_config.py b/app/api/routes/llm_config.py new file mode 100644 index 0000000..7e56c62 --- /dev/null +++ b/app/api/routes/llm_config.py @@ -0,0 +1,363 @@ +from datetime import datetime, timezone +from urllib.parse import urlparse + +from fastapi import APIRouter, Depends +from pydantic import SecretStr +from sqlmodel import Session, select + +from app.api.deps import get_db +from app.api.schemas.enums import LLMProvider +from app.api.schemas.system import ( + LLMExternalConsent, + LLMExternalConsentRequest, + LLMManualModelRequest, + LLMManualModelResult, + LLMProviderCredentialList, + LLMProviderCredentialRequest, + LLMProviderCredentialStatus, + LLMProviderValidationRequest, + LLMProviderValidationResult, + ModelInfo, +) +from app.core import config +from app.models import LLMManualModel, LLMProviderCredential, LLMRuntimeConfig +from app.services import llm +from app.services.llm.errors import ( + LLMError, + LLMExternalConsentError, + LLMNotAvailableError, +) +from app.services.llm.settings import is_local_url +from app.services.llm.specs import SPECS +from app.services.llm.user_config import ( + EDITABLE_PROVIDERS, + decrypt_api_key, + effective_registry, + encrypt_api_key, + invalidate_registry_cache, + has_decryptable_credential, + registry_for_candidate, + validate_provider, +) + +router = APIRouter(prefix="/system/llm", tags=["system"]) +_EXTERNAL_ENV_ALLOWED = str(config.LLM_ALLOW_EXTERNAL).lower() in {"true", "1", "yes"} + + +@router.get("/providers/config", response_model=LLMProviderCredentialList) +def get_provider_configs(db: Session = Depends(get_db)): + credentials = {row.provider: row for row in db.exec(select(LLMProviderCredential)).all()} + try: + registry = effective_registry(db) + enabled = set(registry.providers) + except LLMError: + enabled = set() + + statuses = [] + for provider in sorted(EDITABLE_PROVIDERS, key=lambda item: item.value): + spec = SPECS[provider] + record = credentials.get(provider.value) + env = config.LLM_PROVIDER_ENV[provider.value] + env_key = env.get("API_KEY", "") + ui_key = bool( + record and record.api_key_encrypted and has_decryptable_credential(provider.value) + ) + ui_custom_url = bool(provider is LLMProvider.custom and record and record.base_url) + source = "ui" if ui_key or ui_custom_url else "env" if env_key or env.get("BASE_URL") else "none" + base_url = (record.base_url if ui_custom_url else None) or env.get("BASE_URL") or spec.default_base_url + key_hint = record.key_hint if ui_key else env_key[-4:] if env_key else None + statuses.append( + LLMProviderCredentialStatus( + provider=provider, + configured=source != "none", + source=source, + key_hint=key_hint, + enabled=provider in enabled, + external=_is_external_url(base_url) if base_url else False, + base_url=_display_base_url(base_url) if provider is LLMProvider.custom else None, + structured_output=( + record.structured_output + if provider is LLMProvider.custom and record and record.structured_output is not None + else None + ), + ) + ) + return LLMProviderCredentialList(providers=statuses) + + +@router.put("/providers/config/{provider}", response_model=LLMProviderCredentialStatus) +def save_provider_config( + provider: LLMProvider, + body: LLMProviderCredentialRequest, + db: Session = Depends(get_db), +): + validate_provider(provider) + record = db.exec( + select(LLMProviderCredential).where(LLMProviderCredential.provider == provider.value) + ).first() + env = config.LLM_PROVIDER_ENV[provider.value] + + if provider is not LLMProvider.custom and body.structured_output is not None: + raise LLMNotAvailableError("Only the custom provider accepts a structured_output override.") + if provider is not LLMProvider.custom and body.base_url is not None: + raise LLMNotAvailableError("Only the custom provider accepts a configurable base URL.") + + base_url = body.base_url if provider is LLMProvider.custom and body.base_url is not None else None + if provider is LLMProvider.custom: + base_url = base_url or (record.base_url if record else None) or env.get("BASE_URL") or None + if not base_url: + raise LLMNotAvailableError("CUSTOM_BASE_URL is required.") + base_url = _validate_custom_url(base_url) + + api_key_was_provided = "api_key" in body.model_fields_set and body.api_key is not None + api_key = _provided_secret(body.api_key) if api_key_was_provided else None + if not api_key_was_provided and record and record.api_key_encrypted: + api_key = decrypt_api_key(record.api_key_encrypted, provider.value) + if not api_key: + api_key = env.get("API_KEY") or None + if SPECS[provider].needs_key and not api_key: + raise LLMNotAvailableError(f"{provider.value} requires an API key.") + + _require_external_consent(db, provider, base_url or env.get("BASE_URL") or SPECS[provider].default_base_url) + + if record is None: + record = LLMProviderCredential(provider=provider.value) + if api_key_was_provided: + secret = _provided_secret(body.api_key) + record.api_key_encrypted = encrypt_api_key(secret) if secret else None + record.key_hint = secret[-4:] if secret else None + if provider is LLMProvider.custom: + record.base_url = base_url + if body.structured_output is not None: + record.structured_output = body.structured_output + record.updated_at = datetime.now(timezone.utc) + db.add(record) + db.commit() + db.refresh(record) + invalidate_registry_cache() + return next(status for status in get_provider_configs(db).providers if status.provider is provider) + + +@router.delete("/providers/config/{provider}") +def delete_provider_config(provider: LLMProvider, db: Session = Depends(get_db)): + validate_provider(provider) + record = db.exec( + select(LLMProviderCredential).where(LLMProviderCredential.provider == provider.value) + ).first() + if record: + db.delete(record) + for manual in db.exec(select(LLMManualModel).where(LLMManualModel.provider == provider.value)).all(): + db.delete(manual) + db.commit() + invalidate_registry_cache() + return {"status": "success"} + + +@router.get("/external-consent", response_model=LLMExternalConsent) +def get_external_consent(db: Session = Depends(get_db)): + record = db.get(LLMRuntimeConfig, 1) + if _EXTERNAL_ENV_ALLOWED: + return LLMExternalConsent(allow_external=True, source="env") + if record and record.allow_external: + return LLMExternalConsent(allow_external=True, source="ui") + return LLMExternalConsent(allow_external=False, source="none") + + +@router.put("/external-consent", response_model=LLMExternalConsent) +def set_external_consent(body: LLMExternalConsentRequest, db: Session = Depends(get_db)): + record = db.get(LLMRuntimeConfig, 1) + if record is None: + record = LLMRuntimeConfig(id=1) + record.allow_external = body.allow_external + record.updated_at = datetime.now(timezone.utc) + db.add(record) + db.commit() + invalidate_registry_cache() + return get_external_consent(db) + + +@router.post("/providers/config/{provider}/validate", response_model=LLMProviderValidationResult) +def validate_provider_config( + provider: LLMProvider, + body: LLMProviderValidationRequest, + db: Session = Depends(get_db), +): + validate_provider(provider) + if provider is not LLMProvider.custom and body.structured_output is not None: + return LLMProviderValidationResult( + provider=provider, + ok=False, + detail="Only the custom provider accepts a structured_output override.", + error_code="LLM_CONFIG_ERROR", + ) + api_key = _provided_secret(body.api_key) + if SPECS[provider].needs_key and not api_key: + return LLMProviderValidationResult( + provider=provider, + ok=False, + detail="Enter an API key to validate the provider configuration.", + error_code="LLM_AUTH_FAILED", + ) + base_url = body.base_url + if provider is LLMProvider.custom: + if not base_url: + return LLMProviderValidationResult( + provider=provider, + ok=False, + detail="CUSTOM_BASE_URL is required.", + error_code="LLM_CONFIG_ERROR", + ) + try: + base_url = _validate_custom_url(base_url) + except LLMError as exc: + return LLMProviderValidationResult(provider=provider, ok=False, detail=exc.message, + error_code=exc.error_code) + elif base_url is not None: + return LLMProviderValidationResult( + provider=provider, + ok=False, + detail="Only the custom provider accepts a configurable base URL.", + error_code="LLM_CONFIG_ERROR", + ) + + try: + _require_external_consent( + db, + provider, + base_url or config.LLM_PROVIDER_ENV[provider.value].get("BASE_URL") or SPECS[provider].default_base_url, + ) + registry = registry_for_candidate( + db, + provider, + api_key=api_key, + base_url=base_url, + structured_output=body.structured_output, + ) + models = registry.client(provider).list_models() + return LLMProviderValidationResult( + provider=provider, ok=True, models=[ModelInfo.model_validate(model) for model in models] + ) + except LLMError as exc: + detail = _redact(exc.message, api_key) + if provider is LLMProvider.custom and "has no model list" in detail: + detail = "This server does not expose a model list. Validate a model ID manually." + return LLMProviderValidationResult( + provider=provider, ok=False, detail=detail, error_code=exc.error_code + ) + + +@router.post("/providers/config/{provider}/models", response_model=LLMManualModelResult) +def validate_and_add_manual_model( + provider: LLMProvider, + body: LLMManualModelRequest, + db: Session = Depends(get_db), +): + validate_provider(provider) + try: + existing_config = db.exec( + select(LLMProviderCredential).where(LLMProviderCredential.provider == provider.value) + ).first() + env = config.LLM_PROVIDER_ENV[provider.value] + base_url = ( + existing_config.base_url if provider is LLMProvider.custom and existing_config else None + ) or env.get("BASE_URL") or SPECS[provider].default_base_url + _require_external_consent(db, provider, base_url) + candidate = registry_for_candidate( + db, + provider, + api_key=None, + manual_model=body.model, + ) + check = llm.ModelValidator(candidate).probe(provider, body.model) + except LLMError as exc: + return LLMManualModelResult( + provider=provider, model=body.model, ok=False, detail=exc.message, error_code=exc.error_code + ) + if not check.ok: + return LLMManualModelResult( + provider=provider, model=body.model, ok=False, detail=check.detail, error_code=check.error_code + ) + existing = db.exec( + select(LLMManualModel).where( + LLMManualModel.provider == provider.value, + LLMManualModel.model == body.model, + ) + ).first() + if existing is None: + db.add(LLMManualModel(provider=provider.value, model=body.model)) + db.commit() + invalidate_registry_cache() + return LLMManualModelResult(provider=provider, model=body.model, ok=True) + + +@router.delete("/providers/config/{provider}/models") +def delete_manual_model( + provider: LLMProvider, + body: LLMManualModelRequest, + db: Session = Depends(get_db), +): + validate_provider(provider) + existing = db.exec( + select(LLMManualModel).where( + LLMManualModel.provider == provider.value, + LLMManualModel.model == body.model, + ) + ).first() + if existing: + db.delete(existing) + db.commit() + invalidate_registry_cache() + return {"status": "success"} + + +def _provided_secret(value: SecretStr | None) -> str: + return value.get_secret_value().strip() if value else "" + + +def _validate_custom_url(value: str) -> str: + try: + parsed = urlparse(value) + except ValueError: + raise LLMNotAvailableError("CUSTOM_BASE_URL must be an HTTP or HTTPS URL with a hostname.") from None + if parsed.username is not None or parsed.password is not None: + raise LLMNotAvailableError("CUSTOM_BASE_URL must not include embedded credentials.") + if parsed.query or parsed.fragment: + raise LLMNotAvailableError("CUSTOM_BASE_URL must not include query parameters or a fragment.") + from app.services.llm.settings import _check_url + from app.services.llm.errors import LLMConfigError + + try: + return _check_url("CUSTOM_BASE_URL", value) + except LLMConfigError: + raise LLMNotAvailableError("CUSTOM_BASE_URL must be an HTTP or HTTPS URL with a hostname.") from None + + +def _display_base_url(value: str) -> str: + try: + parsed = urlparse(value) + host = parsed.hostname or "" + port = parsed.port + except ValueError: + return "Invalid custom URL" + if port: + host = f"{host}:{port}" + return f"{parsed.scheme}://{host}{parsed.path}" + + +def _is_external_url(value: str) -> bool: + try: + return not is_local_url(value) + except ValueError: + return True + + +def _require_external_consent(db: Session, provider: LLMProvider, base_url: str) -> None: + record = db.get(LLMRuntimeConfig, 1) + if _is_external_url(base_url) and not _EXTERNAL_ENV_ALLOWED and not (record and record.allow_external): + raise LLMExternalConsentError( + "Allow external processing before configuring a provider outside the local network." + ) + + +def _redact(message: str, secret: str) -> str: + return message.replace(secret, "[redacted]") if secret else message diff --git a/app/api/schemas/system.py b/app/api/schemas/system.py index 65f7c2f..666e548 100644 --- a/app/api/schemas/system.py +++ b/app/api/schemas/system.py @@ -1,5 +1,6 @@ import re -from pydantic import BaseModel, ConfigDict, field_validator +from typing import Literal +from pydantic import BaseModel, ConfigDict, Field, SecretStr, field_validator from app.api.schemas.enums import HealthState, LLMProvider @@ -27,6 +28,7 @@ class ModelInfo(BaseModel): quantization: str | None = None loaded: bool | None = None installed: bool | None = None + manual: bool | None = None class DownloadableModel(BaseModel): @@ -117,6 +119,67 @@ class LLMTestResult(BaseModel): error_code: str | None = None +class LLMProviderCredentialRequest(BaseModel): + model_config = ConfigDict(extra="forbid") + + api_key: SecretStr | None = None + base_url: str | None = None + structured_output: bool | None = None + + +class LLMProviderCredentialStatus(BaseModel): + provider: LLMProvider + configured: bool + source: Literal["env", "ui", "none"] + key_hint: str | None = None + enabled: bool + external: bool + base_url: str | None = None + structured_output: bool | None = None + + +class LLMProviderCredentialList(BaseModel): + providers: list[LLMProviderCredentialStatus] + + +class LLMExternalConsent(BaseModel): + allow_external: bool + source: Literal["env", "ui", "none"] + + +class LLMExternalConsentRequest(BaseModel): + allow_external: bool + + +class LLMProviderValidationRequest(LLMProviderCredentialRequest): + pass + + +class LLMProviderValidationResult(BaseModel): + provider: LLMProvider + ok: bool + models: list[ModelInfo] = Field(default_factory=list) + detail: str | None = None + error_code: str | None = None + + +class LLMManualModelRequest(BaseModel): + model: str + + @field_validator("model") + @classmethod + def _validate_model_name(cls, value: str) -> str: + return validate_model_name(value) + + +class LLMManualModelResult(BaseModel): + provider: LLMProvider + model: str + ok: bool + detail: str | None = None + error_code: str | None = None + + class SchemaVersion(BaseModel): version: str released_at: str diff --git a/app/core/config.py b/app/core/config.py index 5ff0327..201616f 100644 --- a/app/core/config.py +++ b/app/core/config.py @@ -98,6 +98,13 @@ def _provider_env(prefix: str, **defaults: str) -> dict[str, str]: # --- Security & Access Control --------------------------------------------- FIREFORM_API_KEY = os.getenv("FIREFORM_API_KEY", "") +LLM_SECRET_KEY = os.getenv("LLM_SECRET_KEY", "").strip() +_DEFAULT_LLM_SECRET_KEY_FILE = ( + Path("/data/secrets/llm_secret.key") + if Path("/data/secrets").is_dir() + else BASE_DIR / "data" / "secrets" / "llm_secret.key" +) +LLM_SECRET_KEY_FILE = Path(os.getenv("LLM_SECRET_KEY_FILE", str(_DEFAULT_LLM_SECRET_KEY_FILE))).expanduser() # --- Data Retention -------------------------------------------------------- RETENTION_PERIOD_DAYS = int(os.getenv("RETENTION_PERIOD_DAYS", "30")) diff --git a/app/core/errors/handlers.py b/app/core/errors/handlers.py index 5b2e277..a401384 100644 --- a/app/core/errors/handlers.py +++ b/app/core/errors/handlers.py @@ -36,7 +36,7 @@ async def validation_error_handler(request: Request, exc: RequestValidationError validation_errors.append({ "field": field or None, "issue": error.get("msg"), - "value": error.get("input"), + "value": "[redacted]" if "api_key" in field.lower() else error.get("input"), }) return JSONResponse( status_code=422, diff --git a/app/models/__init__.py b/app/models/__init__.py index 9e736ec..343f48b 100644 --- a/app/models/__init__.py +++ b/app/models/__init__.py @@ -7,6 +7,9 @@ Incident, Input, Job, + LLMManualModel, + LLMProviderCredential, + LLMRuntimeConfig, Report, Template, ) @@ -18,6 +21,9 @@ "Incident", "Input", "Job", + "LLMManualModel", + "LLMProviderCredential", + "LLMRuntimeConfig", "Report", "Template", ] diff --git a/app/models/models.py b/app/models/models.py index 15c1cc1..9eb8f66 100644 --- a/app/models/models.py +++ b/app/models/models.py @@ -2,7 +2,7 @@ from datetime import date, datetime, timezone from uuid import UUID, uuid4 -from sqlalchemy import JSON, Column +from sqlalchemy import JSON, Column, UniqueConstraint from sqlmodel import Field, SQLModel from sqlmodel.sql.sqltypes import AutoString @@ -60,6 +60,37 @@ class Job(SQLModel, table=True): updated_at: datetime = Field(default_factory=lambda: datetime.now(timezone.utc)) +class LLMProviderCredential(SQLModel, table=True): + __tablename__ = "llm_provider_credentials" + __table_args__ = (UniqueConstraint("provider", name="uq_llm_provider_credential_provider"),) + + id: int | None = Field(default=None, primary_key=True) + provider: str + api_key_encrypted: str | None = None + key_hint: str | None = None + base_url: str | None = None + structured_output: bool | None = None + updated_at: datetime = Field(default_factory=lambda: datetime.now(timezone.utc)) + + +class LLMManualModel(SQLModel, table=True): + __tablename__ = "llm_manual_models" + __table_args__ = (UniqueConstraint("provider", "model", name="uq_llm_manual_model_provider_model"),) + + id: int | None = Field(default=None, primary_key=True) + provider: str + model: str + created_at: datetime = Field(default_factory=lambda: datetime.now(timezone.utc)) + + +class LLMRuntimeConfig(SQLModel, table=True): + __tablename__ = "llm_runtime_config" + + id: int = Field(default=1, primary_key=True) + allow_external: bool = False + updated_at: datetime = Field(default_factory=lambda: datetime.now(timezone.utc)) + + # --------------------------------------------------------------------------- # v1 contract models # --------------------------------------------------------------------------- diff --git a/app/services/form.py b/app/services/form.py index 9ceb001..2393bb2 100644 --- a/app/services/form.py +++ b/app/services/form.py @@ -46,7 +46,8 @@ def fill_form( model: str | None = None, ) -> FormSubmission: # Before anything is stored, so a bad pick leaves no stray input behind - choice = llm.get_registry().resolve(provider, model) + registry = llm.effective_registry(session) + choice = registry.resolve(provider, model) if input_id is not None and not input_text: transcript = self.input_service.resolve_transcript(session, input_id) elif input_text: @@ -67,7 +68,9 @@ def fill_form( input_id = input_record.input_id else: raise AppError("Either input_id or input_text is required", status_code=422, error_code="VALIDATION_ERROR") - return self.fill_and_persist(session, template, transcript, input_id, choice.model, choice.provider) + return self.fill_and_persist( + session, template, transcript, input_id, choice.model, choice.provider, registry=registry + ) def fill_and_persist( self, @@ -77,7 +80,9 @@ def fill_and_persist( input_id: UUID, model: str | None = None, provider: LLMProvider | None = None, + registry=None, ) -> FormSubmission: + registry = registry or llm.effective_registry(session) resolved_pdf = paths._resolve_project_file(template.pdf_path) if not resolved_pdf.exists(): raise FileNotFoundError(f"PDF template not found at {template.pdf_path}") @@ -93,6 +98,7 @@ def fill_and_persist( provider=provider, model=model, description=template.description, + registry=registry, ) relative_out = out_target.relative_to(paths.PROJECT_ROOT).as_posix() diff --git a/app/services/form_filler/filler.py b/app/services/form_filler/filler.py index 57cde66..653e066 100644 --- a/app/services/form_filler/filler.py +++ b/app/services/form_filler/filler.py @@ -24,7 +24,7 @@ PROMPT = f.read() -def fill(pdf_path, narrative, out_path=None, model=None, description="", provider=None): +def fill(pdf_path, narrative, out_path=None, model=None, description="", provider=None, registry=None): """Fill pdf_path from the narrative. Writes the PDF when out_path is given. Returns {schema, tables, groups, prompt, response_text, values}: the @@ -35,7 +35,7 @@ def fill(pdf_path, narrative, out_path=None, model=None, description="", provide strip_empty_descriptions(schema["properties"]) prompt = build_prompt(narrative, schema, description) - response_text = call_model(prompt, schema, model, provider) + response_text = call_model(prompt, schema, model, provider, registry=registry) # Tables come back as arrays of rows; map them to widget names values = template.expand_output(json.loads(response_text), tables, groups) @@ -76,9 +76,10 @@ def build_prompt(narrative, schema, description=""): return PROMPT.rstrip() + "\n\n" + user_content -def call_model(prompt, schema, model=None, provider=None): +def call_model(prompt, schema, model=None, provider=None, registry=None): """The answer as JSON text shaped by the schema. None falls back to the defaults.""" - return llm.get_registry().generate_json(prompt, schema, provider, model) + active_registry = registry or llm.get_registry() + return active_registry.generate_json(prompt, schema, provider, model) def write_pdf(pdf_path, values, out_path): diff --git a/app/services/llm/__init__.py b/app/services/llm/__init__.py index 7cb42c1..280c462 100644 --- a/app/services/llm/__init__.py +++ b/app/services/llm/__init__.py @@ -19,16 +19,19 @@ LLMAuthError, LLMConfigError, LLMError, + LLMExternalConsentError, LLMModelNotFoundError, LLMNotAvailableError, LLMRateLimitError, LLMResponseError, + LLMSecretKeyError, LLMUnavailableError, ) from app.services.llm.factory import create_llm from app.services.llm.registry import LLMRegistry, ModelChoice, ProviderCatalog from app.services.llm.settings import LLMSettings, ProviderSettings, load_settings from app.services.llm.validator import ModelCheck, ModelValidator +from app.services.llm.user_config import effective_registry, registry_for_candidate @lru_cache(maxsize=1) @@ -43,6 +46,7 @@ def get_registry() -> LLMRegistry: "LLMAuthError", "LLMConfigError", "LLMError", + "LLMExternalConsentError", "LLMHealth", "LLMModelNotFoundError", "LLMNotAvailableError", @@ -50,6 +54,7 @@ def get_registry() -> LLMRegistry: "LLMRateLimitError", "LLMRegistry", "LLMResponseError", + "LLMSecretKeyError", "LLMSettings", "LLMUnavailableError", "ModelCheck", @@ -59,6 +64,8 @@ def get_registry() -> LLMRegistry: "ProviderCatalog", "ProviderSettings", "create_llm", + "effective_registry", "get_registry", "load_settings", + "registry_for_candidate", ] diff --git a/app/services/llm/base.py b/app/services/llm/base.py index 4363150..ab69e89 100644 --- a/app/services/llm/base.py +++ b/app/services/llm/base.py @@ -18,6 +18,7 @@ class ModelInfo: quantization: str | None = None loaded: bool | None = None installed: bool | None = None + manual: bool | None = None @dataclass(frozen=True) diff --git a/app/services/llm/errors.py b/app/services/llm/errors.py index b842e06..adb7d61 100644 --- a/app/services/llm/errors.py +++ b/app/services/llm/errors.py @@ -22,6 +22,20 @@ class LLMConfigError(LLMError): error_code = "LLM_CONFIG_ERROR" +class LLMSecretKeyError(LLMError): + """The key required to encrypt provider credentials cannot be accessed.""" + + status_code = 503 + error_code = "LLM_SECRET_KEY_UNAVAILABLE" + + +class LLMExternalConsentError(LLMError): + """External processing has not been explicitly allowed.""" + + status_code = 422 + error_code = "LLM_EXTERNAL_CONSENT_REQUIRED" + + class LLMNotAvailableError(LLMError): """The request named a provider or model that isn't enabled or offered.""" diff --git a/app/services/llm/openai_compatible.py b/app/services/llm/openai_compatible.py index d86eb56..c8e838d 100644 --- a/app/services/llm/openai_compatible.py +++ b/app/services/llm/openai_compatible.py @@ -117,7 +117,8 @@ def _translate_errors(self, model: str | None = None): raise self._auth_error(exc) from exc except openai.RateLimitError as exc: raise LLMRateLimitError( - f"{name} is still rate limiting after retries, or the account is out of quota: {_reason(exc)}" + f"{name} is still rate limiting after retries, or the account is out of quota: " + f"{_reason(exc, self.client.api_key)}" ) from exc except openai.APITimeoutError as exc: raise LLMUnavailableError(f"{name} at {self.base_url} timed out") from exc @@ -126,7 +127,7 @@ def _translate_errors(self, model: str | None = None): f"Could not reach {name} at {self.base_url}. Check {spec.setting('BASE_URL')} and the network." ) from exc except openai.APIStatusError as exc: - reason = _reason(exc) + reason = _reason(exc, self.client.api_key) # Gemini answers a bad key with 400 instead of 401 if exc.status_code == 400 and "api key" in reason.lower(): raise self._auth_error(exc) from exc @@ -142,12 +143,12 @@ def _translate_errors(self, model: str | None = None): def _auth_error(self, exc: openai.APIStatusError) -> LLMAuthError: return LLMAuthError( - f"{self.provider.value} rejected the API key: {_reason(exc)}. " + f"{self.provider.value} rejected the API key: {_reason(exc, self.client.api_key)}. " f"Check {SPECS[self.provider].setting('API_KEY')}." ) -def _reason(exc: openai.APIStatusError) -> str: +def _reason(exc: openai.APIStatusError, secret: str = "") -> str: """The provider's own error message, without the JSON around it. Bodies differ: OpenAI sends {"error": {"message"}}, Gemini a list of those, @@ -159,7 +160,10 @@ def _reason(exc: openai.APIStatusError) -> str: if isinstance(body, dict): error = body.get("error", body) if isinstance(error, dict) and error.get("message"): - return str(error["message"]).rstrip(".") + message = str(error["message"]).rstrip(".") + return message.replace(secret, "[redacted]") if secret else message if isinstance(error, str): - return error.rstrip(".") - return exc.message.rstrip(".") + message = error.rstrip(".") + return message.replace(secret, "[redacted]") if secret else message + message = exc.message.rstrip(".") + return message.replace(secret, "[redacted]") if secret else message diff --git a/app/services/llm/registry.py b/app/services/llm/registry.py index 73f1d38..225d53a 100644 --- a/app/services/llm/registry.py +++ b/app/services/llm/registry.py @@ -17,7 +17,7 @@ from app.api.schemas.enums import LLMProvider from app.services.llm.base import DownloadableModelInfo, LLM, ModelInfo -from app.services.llm.errors import LLMError, LLMNotAvailableError +from app.services.llm.errors import LLMError, LLMNotAvailableError, LLMResponseError from app.services.llm.factory import create_llm from app.services.llm.settings import LLMSettings, ProviderSettings from app.services.llm.specs import DEFAULT_OLLAMA_INSTALLABLE_MODELS @@ -157,15 +157,27 @@ def _read(self, settings: ProviderSettings) -> ProviderCatalog: try: offered = self._clients[settings.provider].list_models() except LLMError as exc: - return ProviderCatalog( - settings.provider, - available=False, - models=(), - default_model=settings.default_model, - detail=exc.message, - error_code=exc.error_code, - error=exc, - ) + if ( + settings.manual_models + and isinstance(exc, LLMResponseError) + and "has no model list" in exc.message + ): + offered = [] + else: + return ProviderCatalog( + settings.provider, + available=False, + models=(), + default_model=settings.default_model, + detail=exc.message, + error_code=exc.error_code, + error=exc, + ) + if settings.manual_models: + offered_by_name = {model.name: model for model in offered} + for name in settings.manual_models: + offered_by_name.setdefault(name, ModelInfo(name=name, manual=True)) + offered = list(offered_by_name.values()) kept, missing = settings.models.pick(offered) offered_names = {m.name for m in offered} diff --git a/app/services/llm/settings.py b/app/services/llm/settings.py index b5e5910..a946118 100644 --- a/app/services/llm/settings.py +++ b/app/services/llm/settings.py @@ -25,7 +25,7 @@ class ProviderSettings: base_url: str models: ModelSelection default_model: str | None - api_key: str = "" + api_key: str = field(default="", repr=False) timeout: int = 300 pull_timeout: int = 120 installable_models: tuple[str, ...] = () @@ -33,6 +33,7 @@ class ProviderSettings: temperature: float | None = None structured_output: bool = True max_tokens: int | None = None + manual_models: tuple[str, ...] = () @property def provider(self) -> LLMProvider: @@ -199,12 +200,19 @@ def is_local_url(url: str) -> bool: try: ip = ipaddress.ip_address(host) except ValueError: - return host == "localhost" or "." not in host or host.endswith((".local", ".internal", ".lan")) + return ( + host in {"localhost", "host.docker.internal"} + or "." not in host + or host.endswith((".local", ".internal", ".lan")) + ) return ip.is_loopback or ip.is_private def _check_url(name: str, url: str) -> str: - parsed = urlparse(url) + try: + parsed = urlparse(url) + except ValueError: + raise LLMConfigError(f"{name} is not a valid URL. It should look like http://host:port.") from None if parsed.scheme not in ("http", "https") or not parsed.hostname: raise LLMConfigError(f"{name}={url!r} is not a valid URL. It should look like http://host:port.") return url diff --git a/app/services/llm/user_config.py b/app/services/llm/user_config.py new file mode 100644 index 0000000..f3383b6 --- /dev/null +++ b/app/services/llm/user_config.py @@ -0,0 +1,277 @@ +"""Runtime credentials for FireForm's local, single-user installation.""" + +import copy +import logging +import os +import threading +import time +from dataclasses import replace +from types import SimpleNamespace +from pathlib import Path + +from cryptography.fernet import Fernet, InvalidToken +from sqlmodel import Session, select + +from app.api.schemas.enums import LLMProvider +from app.core import config +from app.models import LLMManualModel, LLMProviderCredential, LLMRuntimeConfig +from app.services.llm.errors import LLMSecretKeyError +from app.services.llm.registry import LLMRegistry +from app.services.llm.settings import load_settings + +logger = logging.getLogger(__name__) + +EDITABLE_PROVIDERS = frozenset({ + LLMProvider.openai, + LLMProvider.gemini, + LLMProvider.anthropic, + LLMProvider.custom, +}) +_cache_lock = threading.Lock() +_cached_registry: LLMRegistry | None = None +_cached_fingerprint: tuple | None = None +_invalid_credentials: set[str] = set() + + +def _fernet(*, create: bool = False) -> Fernet: + if config.LLM_SECRET_KEY: + try: + key = config.LLM_SECRET_KEY.encode("ascii") + except UnicodeEncodeError: + raise LLMSecretKeyError("LLM master key is unavailable.") from None + return _build_fernet(key) + + key_file = Path(config.LLM_SECRET_KEY_FILE) + if create: + key = _read_or_create_key_file(key_file) + else: + try: + key = key_file.read_bytes() + except OSError: + raise LLMSecretKeyError("LLM master key is unavailable.") from None + return _build_fernet(key) + + +def _build_fernet(key: bytes) -> Fernet: + try: + return Fernet(key.strip()) + except (TypeError, ValueError): + raise LLMSecretKeyError("LLM master key is unavailable.") from None + + +def _read_or_create_key_file(path: Path) -> bytes: + try: + key = path.read_bytes() + if key: + return key + except FileNotFoundError: + pass + except OSError: + raise LLMSecretKeyError("LLM master key is unavailable.") from None + + try: + path.parent.mkdir(parents=True, exist_ok=True) + descriptor = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) + except FileExistsError: + return _read_key_after_create_race(path) + except OSError: + raise LLMSecretKeyError("LLM master key is unavailable.") from None + + key = Fernet.generate_key() + try: + with os.fdopen(descriptor, "wb") as key_file: + key_file.write(key) + key_file.flush() + os.fsync(key_file.fileno()) + except OSError: + try: + path.unlink() + except OSError: + pass + raise LLMSecretKeyError("LLM master key is unavailable.") from None + return key + + +def _read_key_after_create_race(path: Path) -> bytes: + for attempt in range(10): + try: + key = path.read_bytes() + except OSError: + key = b"" + if key: + return key + if attempt < 9: + time.sleep(0.01) + raise LLMSecretKeyError("LLM master key is unavailable.") + + +def encrypt_api_key(value: str) -> str: + return _fernet(create=True).encrypt(value.encode("utf-8")).decode("ascii") + + +def decrypt_api_key(value: str, provider: str) -> str | None: + try: + decrypted = _fernet().decrypt(value.encode("ascii")).decode("utf-8") + _invalid_credentials.discard(provider) + return decrypted + except LLMSecretKeyError: + _invalid_credentials.add(provider) + logger.warning("Stored API key for %s cannot be decrypted because LLM_SECRET_KEY is unavailable.", provider) + except (InvalidToken, UnicodeDecodeError, ValueError): + _invalid_credentials.add(provider) + logger.warning("Stored API key for %s cannot be decrypted; ignoring the saved credential.", provider) + return None + + +def has_decryptable_credential(provider: str) -> bool: + return provider not in _invalid_credentials + + +def validate_provider(provider: LLMProvider) -> None: + if provider not in EDITABLE_PROVIDERS: + from app.services.llm.errors import LLMNotAvailableError + + raise LLMNotAvailableError("Only OpenAI, Gemini, Anthropic, and custom providers can be configured here.") + + +def registry_for_candidate( + session: Session, + provider: LLMProvider, + *, + api_key: str | None, + base_url: str | None = None, + structured_output: bool | None = None, + manual_model: str | None = None, +) -> LLMRegistry: + credentials = {row.provider: row for row in session.exec(select(LLMProviderCredential)).all()} + manual_models = session.exec(select(LLMManualModel)).all() + runtime_config = session.get(LLMRuntimeConfig, 1) + override = { + "api_key": api_key, + "base_url": base_url, + "structured_output": structured_output, + } + return _build_registry( + credentials, + manual_models, + runtime_config.allow_external if runtime_config else False, + candidate=(provider, override, manual_model), + ) + + +def effective_registry(session: Session) -> LLMRegistry: + """Return a process-local registry rebuilt when persisted provider state changes.""" + global _cached_registry, _cached_fingerprint + with _cache_lock: + credentials = session.exec(select(LLMProviderCredential)).all() + manual_models = session.exec(select(LLMManualModel)).all() + runtime_config = session.get(LLMRuntimeConfig, 1) + allow_external = runtime_config.allow_external if runtime_config else False + if not credentials and not manual_models and not allow_external: + from app.services import llm + + _cached_registry = None + _cached_fingerprint = None + return llm.get_registry() + + fingerprint = ( + tuple(sorted((row.provider, row.updated_at, row.key_hint, row.base_url, row.structured_output) for row in credentials)), + tuple(sorted((row.provider, row.model, row.created_at) for row in manual_models)), + runtime_config.updated_at if runtime_config else None, + allow_external, + ) + if fingerprint == _cached_fingerprint and _cached_registry is not None: + return _cached_registry + + _cached_registry = _build_registry( + {row.provider: row for row in credentials}, + manual_models, + allow_external, + ) + _cached_fingerprint = fingerprint + return _cached_registry + + +def invalidate_registry_cache() -> None: + global _cached_registry, _cached_fingerprint + with _cache_lock: + _cached_registry = None + _cached_fingerprint = None + + +def _build_registry( + credentials: dict[str, LLMProviderCredential], + manual_models: list[LLMManualModel], + allow_external: bool, + candidate: tuple[LLMProvider, dict, str | None] | None = None, +) -> LLMRegistry: + provider_env = copy.deepcopy(config.LLM_PROVIDER_ENV) + runtime_allow_external = allow_external or str(config.LLM_ALLOW_EXTERNAL).lower() in {"true", "1", "yes"} + manual_by_provider: dict[LLMProvider, list[str]] = {} + unavailable_credentials: set[str] = set() + + for record in manual_models: + try: + provider = LLMProvider(record.provider) + except ValueError: + continue + manual_by_provider.setdefault(provider, []).append(record.model) + + for provider_name, record in credentials.items(): + try: + provider = LLMProvider(provider_name) + except ValueError: + continue + provider_settings = provider_env[provider.value] + api_key = decrypt_api_key(record.api_key_encrypted, provider.value) if record.api_key_encrypted else None + if record.api_key_encrypted and not api_key: + unavailable_credentials.add(provider.value) + if api_key: + provider_settings["API_KEY"] = api_key + if provider is LLMProvider.custom and record.base_url: + provider_settings["BASE_URL"] = record.base_url + if provider is LLMProvider.custom and record.structured_output is not None: + provider_settings["STRUCTURED_OUTPUT"] = str(record.structured_output).lower() + if not provider_settings.get("MODELS") and (api_key or (provider is LLMProvider.custom and record.base_url)): + provider_settings["MODELS"] = "*" + + if candidate: + provider, override, extra_model = candidate + provider_settings = provider_env[provider.value] + if override["api_key"] is not None: + provider_settings["API_KEY"] = override["api_key"] + if provider is LLMProvider.custom and override["base_url"] is not None: + provider_settings["BASE_URL"] = override["base_url"] + if provider is LLMProvider.custom and override["structured_output"] is not None: + provider_settings["STRUCTURED_OUTPUT"] = str(override["structured_output"]).lower() + if not provider_settings.get("MODELS"): + provider_settings["MODELS"] = "*" + if extra_model: + manual_by_provider.setdefault(provider, []).append(extra_model) + + for provider, names in manual_by_provider.items(): + provider_settings = provider_env[provider.value] + configured = provider_settings.get("MODELS", "") + if configured and configured.strip() != "*": + provider_settings["MODELS"] = ",".join(dict.fromkeys([*configured.split(","), *names])) + elif not configured: + provider_settings["MODELS"] = ",".join(dict.fromkeys(names)) + + effective_config = SimpleNamespace(**vars(config)) + effective_config.LLM_PROVIDER_ENV = provider_env + effective_config.LLM_ALLOW_EXTERNAL = "true" if runtime_allow_external else "false" + if ( + config.LLM_PROVIDER in unavailable_credentials + and not provider_env[config.LLM_PROVIDER].get("API_KEY") + and provider_env[LLMProvider.ollama.value].get("MODELS") + ): + effective_config.LLM_PROVIDER = LLMProvider.ollama.value + effective_config.LLM_MODEL = "" + settings = load_settings(effective_config) + for provider, names in manual_by_provider.items(): + if provider in settings.providers: + settings.providers[provider] = replace( + settings.providers[provider], + manual_models=tuple(dict.fromkeys(names)), + ) + return LLMRegistry.from_settings(settings) diff --git a/app/services/llm/validator.py b/app/services/llm/validator.py index 958946d..379d3df 100644 --- a/app/services/llm/validator.py +++ b/app/services/llm/validator.py @@ -62,7 +62,12 @@ def probe(self, provider: LLMProvider | str | None = None, model: str | None = N try: self.registry.client(choice.provider).generate_json(PROBE_PROMPT, PROBE_SCHEMA, choice.model) except LLMError as exc: - logger.warning("LLM test of %s %s failed: %s", choice.provider.value, choice.model, exc.message) + logger.warning( + "LLM test of %s %s failed with %s", + choice.provider.value, + choice.model, + exc.error_code, + ) return ModelCheck( choice.provider, choice.model, ok=False, latency_ms=_since(t0), detail=exc.message, error_code=exc.error_code, diff --git a/docker/.env.example b/docker/.env.example index 8d77a21..5262795 100644 --- a/docker/.env.example +++ b/docker/.env.example @@ -39,6 +39,11 @@ LLM_MAX_RETRIES= # Empty sends none. Some models (OpenAI reasoning models) reject temperature. LLM_TEMPERATURE=0 +# Optional override for the API/worker key file; Docker's persistent volume is +# mounted at this path by default. If LLM_SECRET_KEY is set, it takes precedence. +# LLM_SECRET_KEY= +# LLM_SECRET_KEY_FILE=/data/secrets/llm_secret.key + # Ollama # Bundled (default): Ollama runs in the Docker stack -> make fireform # OLLAMA_HOST=http://ollama:11434 diff --git a/docker/dev/compose.yml b/docker/dev/compose.yml index 5b8e695..139fd99 100644 --- a/docker/dev/compose.yml +++ b/docker/dev/compose.yml @@ -104,6 +104,7 @@ services: volumes: - ../..:/app - fireform_uploads:/data/uploads + - llm_secrets:/data/secrets ports: - "${APP_PORT:-8000}:8000" # Everything in the env file reaches the container, so a new setting works @@ -143,6 +144,7 @@ services: volumes: - ../..:/app - fireform_uploads:/data/uploads + - llm_secrets:/data/secrets env_file: - ../.env.dev environment: @@ -166,6 +168,8 @@ volumes: driver: local fireform_uploads: driver: local + llm_secrets: + driver: local redis_data: driver: local diff --git a/docker/prod/compose.yml b/docker/prod/compose.yml index eb3b689..10f66a5 100644 --- a/docker/prod/compose.yml +++ b/docker/prod/compose.yml @@ -68,6 +68,7 @@ services: # Under /app (the project root) so uploaded and filled PDFs pass the # "inside the project" path guard and persist across container updates. - fireform_uploads:/app/data + - llm_secrets:/data/secrets ports: - "${APP_PORT}:8000" # Everything in the env file reaches the container, so a new setting works @@ -107,6 +108,7 @@ services: command: ["celery", "-A", "app.core.celery:celery_app", "worker", "--loglevel=info", "--concurrency=4"] volumes: - fireform_uploads:/app/data + - llm_secrets:/data/secrets # Same env file as the app, so the worker sees the same LLM providers env_file: - ../.env.prod @@ -132,6 +134,8 @@ volumes: driver: local fireform_uploads: driver: local + llm_secrets: + driver: local redis_data: driver: local diff --git a/requirements.txt b/requirements.txt index 6d66287..f66e03b 100644 --- a/requirements.txt +++ b/requirements.txt @@ -11,6 +11,7 @@ pytest httpx numpy<2 openai +cryptography==50.0.2 pypdf pdfplumber python-multipart diff --git a/tests/test_llm_service.py b/tests/test_llm_service.py index f0cdd49..e95e3f6 100644 --- a/tests/test_llm_service.py +++ b/tests/test_llm_service.py @@ -5,6 +5,10 @@ their own file, test_llm_registry.py. """ +import os +import stat +import subprocess +import sys from types import SimpleNamespace from unittest.mock import MagicMock @@ -12,10 +16,12 @@ import openai import pytest import requests +from cryptography.fernet import Fernet from app.services.llm import ( LLMAuthError, LLMConfigError, + LLMSecretKeyError, LLMModelNotFoundError, LLMProvider, LLMRateLimitError, @@ -765,6 +771,119 @@ def on_success(): assert events == ["callback", "success"] +# --------------------------------------------------------------------------- +# load_settings: Ollama pull_timeout and installable_models +# --------------------------------------------------------------------------- + +class TestLLMSecretKey: + + def _configure_file(self, monkeypatch, path): + from app.core import config + + monkeypatch.setattr(config, "LLM_SECRET_KEY", "") + monkeypatch.setattr(config, "LLM_SECRET_KEY_FILE", path) + + def test_api_generates_key_file_on_first_credential_save(self, tmp_path, monkeypatch): + from app.services.llm.user_config import decrypt_api_key, encrypt_api_key + + key_file = tmp_path / "secrets" / "llm_secret.key" + self._configure_file(monkeypatch, key_file) + + assert not key_file.exists() + encrypted = encrypt_api_key("provider-secret") + + assert key_file.is_file() + assert decrypt_api_key(encrypted, "openai") == "provider-secret" + if os.name == "posix": + assert stat.S_IMODE(key_file.stat().st_mode) == 0o600 + + def test_key_file_is_reused_by_a_separate_process(self, tmp_path, monkeypatch): + from app.services.llm.user_config import encrypt_api_key + + key_file = tmp_path / "secrets" / "llm_secret.key" + self._configure_file(monkeypatch, key_file) + encrypted = encrypt_api_key("provider-secret") + + env = os.environ.copy() + env["LLM_SECRET_KEY"] = "" + env["LLM_SECRET_KEY_FILE"] = str(key_file) + result = subprocess.run( + [ + sys.executable, + "-c", + "from app.services.llm.user_config import decrypt_api_key; " + f"print(decrypt_api_key({encrypted!r}, 'openai') or '')", + ], + check=True, + capture_output=True, + text=True, + env=env, + ) + + assert result.stdout.strip() == "provider-secret" + + def test_environment_key_overrides_file_and_does_not_create_it(self, tmp_path, monkeypatch): + from app.core import config + from app.services.llm.user_config import decrypt_api_key, encrypt_api_key + + key_file = tmp_path / "missing.key" + monkeypatch.setattr(config, "LLM_SECRET_KEY", Fernet.generate_key().decode("ascii")) + monkeypatch.setattr(config, "LLM_SECRET_KEY_FILE", key_file) + + encrypted = encrypt_api_key("provider-secret") + + assert decrypt_api_key(encrypted, "openai") == "provider-secret" + assert not key_file.exists() + + def test_missing_file_is_warning_for_reader_and_not_created(self, tmp_path, monkeypatch, caplog): + from app.services.llm.user_config import decrypt_api_key + + key_file = tmp_path / "missing.key" + self._configure_file(monkeypatch, key_file) + + assert decrypt_api_key("not-a-valid-token", "openai") is None + assert not key_file.exists() + assert "cannot be decrypted" in caplog.text + + def test_corrupt_file_is_warning_for_reader_and_cannot_be_overwritten(self, tmp_path, monkeypatch, caplog): + from app.services.llm.user_config import decrypt_api_key, encrypt_api_key + + key_file = tmp_path / "corrupt.key" + key_file.write_text("not-a-fernet-key", encoding="ascii") + self._configure_file(monkeypatch, key_file) + + assert decrypt_api_key("not-a-valid-token", "openai") is None + assert "cannot be decrypted" in caplog.text + with pytest.raises(LLMSecretKeyError, match="master key is unavailable"): + encrypt_api_key("provider-secret") + assert key_file.read_text(encoding="ascii") == "not-a-fernet-key" + + def test_unreadable_default_credential_falls_back_to_ollama(self, tmp_path, monkeypatch, caplog): + from copy import deepcopy + + from app.api.schemas.enums import LLMProvider + from app.core import config + from app.models import LLMProviderCredential + from app.services.llm.user_config import _build_registry + + self._configure_file(monkeypatch, tmp_path / "missing.key") + monkeypatch.setattr(config, "LLM_PROVIDER", "openai") + monkeypatch.setattr(config, "LLM_MODEL", "") + provider_env = deepcopy(config.LLM_PROVIDER_ENV) + provider_env["openai"]["MODELS"] = "" + provider_env["openai"]["API_KEY"] = "" + monkeypatch.setattr(config, "LLM_PROVIDER_ENV", provider_env) + + registry = _build_registry( + {"openai": LLMProviderCredential(provider="openai", api_key_encrypted="saved-ciphertext")}, + [], + False, + ) + + assert registry.default.provider is LLMProvider.ollama + assert "cannot be decrypted" in caplog.text + + # --------------------------------------------------------------------------- # load_settings: Ollama pull_timeout and installable_models # --------------------------------------------------------------------------- From ea542a63bb948c2346718f24a64dcef88481d33b Mon Sep 17 00:00:00 2001 From: Jan <1673276@uab.cat> Date: Sat, 24 Oct 2026 12:56:16 +0200 Subject: [PATCH 08/14] feat: add encrypted provider credentials, manual models and external consent --- app/api/routes/llm_config.py | 9 +++-- app/api/schemas/system.py | 1 + app/services/llm/openai_compatible.py | 21 ++++++------ app/services/llm/registry.py | 7 ++-- app/services/llm/user_config.py | 49 ++++++++++++++++++++++----- docker/.env.example | 7 ++-- 6 files changed, 69 insertions(+), 25 deletions(-) diff --git a/app/api/routes/llm_config.py b/app/api/routes/llm_config.py index 7e56c62..d4e2625 100644 --- a/app/api/routes/llm_config.py +++ b/app/api/routes/llm_config.py @@ -74,6 +74,9 @@ def get_provider_configs(db: Session = Depends(get_db)): key_hint=key_hint, enabled=provider in enabled, external=_is_external_url(base_url) if base_url else False, + credential_status=( + "available" if has_decryptable_credential(provider.value) else "unavailable" + ) if record and record.api_key_encrypted else None, base_url=_display_base_url(base_url) if provider is LLMProvider.custom else None, structured_output=( record.structured_output @@ -359,5 +362,7 @@ def _require_external_consent(db: Session, provider: LLMProvider, base_url: str) ) -def _redact(message: str, secret: str) -> str: - return message.replace(secret, "[redacted]") if secret else message +def _redact(message: str, secret: object) -> str: + if isinstance(secret, str) and len(secret) >= 8 and secret != "not-needed": + return message.replace(secret, "[redacted]") + return message diff --git a/app/api/schemas/system.py b/app/api/schemas/system.py index 666e548..18e2360 100644 --- a/app/api/schemas/system.py +++ b/app/api/schemas/system.py @@ -134,6 +134,7 @@ class LLMProviderCredentialStatus(BaseModel): key_hint: str | None = None enabled: bool external: bool + credential_status: Literal["available", "unavailable"] | None = None base_url: str | None = None structured_output: bool | None = None diff --git a/app/services/llm/openai_compatible.py b/app/services/llm/openai_compatible.py index c8e838d..e6d92e4 100644 --- a/app/services/llm/openai_compatible.py +++ b/app/services/llm/openai_compatible.py @@ -40,13 +40,14 @@ def __init__( ): super().__init__(provider, model, timeout) self.base_url = base_url + self._secret = api_key or "" self.structured_output = structured_output self.max_tokens = max_tokens self.temperature = temperature self.listed_id_prefix = listed_id_prefix self.client = openai.OpenAI( base_url=base_url, - api_key=api_key or "not-needed", + api_key=self._secret or "not-needed", timeout=timeout, max_retries=max_retries, default_headers=headers, @@ -118,7 +119,7 @@ def _translate_errors(self, model: str | None = None): except openai.RateLimitError as exc: raise LLMRateLimitError( f"{name} is still rate limiting after retries, or the account is out of quota: " - f"{_reason(exc, self.client.api_key)}" + f"{_reason(exc, self._secret)}" ) from exc except openai.APITimeoutError as exc: raise LLMUnavailableError(f"{name} at {self.base_url} timed out") from exc @@ -127,7 +128,7 @@ def _translate_errors(self, model: str | None = None): f"Could not reach {name} at {self.base_url}. Check {spec.setting('BASE_URL')} and the network." ) from exc except openai.APIStatusError as exc: - reason = _reason(exc, self.client.api_key) + reason = _reason(exc, self._secret) # Gemini answers a bad key with 400 instead of 401 if exc.status_code == 400 and "api key" in reason.lower(): raise self._auth_error(exc) from exc @@ -143,12 +144,12 @@ def _translate_errors(self, model: str | None = None): def _auth_error(self, exc: openai.APIStatusError) -> LLMAuthError: return LLMAuthError( - f"{self.provider.value} rejected the API key: {_reason(exc, self.client.api_key)}. " + f"{self.provider.value} rejected the API key: {_reason(exc, self._secret)}. " f"Check {SPECS[self.provider].setting('API_KEY')}." ) -def _reason(exc: openai.APIStatusError, secret: str = "") -> str: +def _reason(exc: openai.APIStatusError, secret: object = "") -> str: """The provider's own error message, without the JSON around it. Bodies differ: OpenAI sends {"error": {"message"}}, Gemini a list of those, @@ -157,13 +158,13 @@ def _reason(exc: openai.APIStatusError, secret: str = "") -> str: body = exc.body if isinstance(body, list) and body: body = body[0] + message = exc.message.rstrip(".") if isinstance(body, dict): error = body.get("error", body) if isinstance(error, dict) and error.get("message"): message = str(error["message"]).rstrip(".") - return message.replace(secret, "[redacted]") if secret else message - if isinstance(error, str): + elif isinstance(error, str): message = error.rstrip(".") - return message.replace(secret, "[redacted]") if secret else message - message = exc.message.rstrip(".") - return message.replace(secret, "[redacted]") if secret else message + if isinstance(secret, str) and len(secret) >= 8 and secret != "not-needed": + message = message.replace(secret, "[redacted]") + return message diff --git a/app/services/llm/registry.py b/app/services/llm/registry.py index 225d53a..e151e4c 100644 --- a/app/services/llm/registry.py +++ b/app/services/llm/registry.py @@ -13,7 +13,7 @@ import time from collections.abc import Callable, Mapping from concurrent.futures import ThreadPoolExecutor -from dataclasses import dataclass, field +from dataclasses import dataclass, field, replace from app.api.schemas.enums import LLMProvider from app.services.llm.base import DownloadableModelInfo, LLM, ModelInfo @@ -176,7 +176,10 @@ def _read(self, settings: ProviderSettings) -> ProviderCatalog: if settings.manual_models: offered_by_name = {model.name: model for model in offered} for name in settings.manual_models: - offered_by_name.setdefault(name, ModelInfo(name=name, manual=True)) + if name in offered_by_name: + offered_by_name[name] = replace(offered_by_name[name], manual=True) + else: + offered_by_name[name] = ModelInfo(name=name, manual=True) offered = list(offered_by_name.values()) kept, missing = settings.models.pick(offered) offered_names = {m.name for m in offered} diff --git a/app/services/llm/user_config.py b/app/services/llm/user_config.py index f3383b6..1422117 100644 --- a/app/services/llm/user_config.py +++ b/app/services/llm/user_config.py @@ -28,9 +28,14 @@ LLMProvider.custom, }) _cache_lock = threading.Lock() +_credential_state_lock = threading.Lock() _cached_registry: LLMRegistry | None = None _cached_fingerprint: tuple | None = None +_cache_checked_at = 0.0 +_REGISTRY_CHECK_SECONDS = 3.0 +_BASE_CONFIG_FINGERPRINT = ("base-config",) _invalid_credentials: set[str] = set() +_warned_credential_errors: dict[str, str] = {} def _fernet(*, create: bool = False) -> Fernet: @@ -112,19 +117,32 @@ def encrypt_api_key(value: str) -> str: def decrypt_api_key(value: str, provider: str) -> str | None: try: decrypted = _fernet().decrypt(value.encode("ascii")).decode("utf-8") - _invalid_credentials.discard(provider) + with _credential_state_lock: + _invalid_credentials.discard(provider) + _warned_credential_errors.pop(provider, None) return decrypted except LLMSecretKeyError: - _invalid_credentials.add(provider) - logger.warning("Stored API key for %s cannot be decrypted because LLM_SECRET_KEY is unavailable.", provider) + with _credential_state_lock: + _invalid_credentials.add(provider) + _warn_credential_error(provider, value, "LLM_SECRET_KEY is unavailable") except (InvalidToken, UnicodeDecodeError, ValueError): - _invalid_credentials.add(provider) - logger.warning("Stored API key for %s cannot be decrypted; ignoring the saved credential.", provider) + with _credential_state_lock: + _invalid_credentials.add(provider) + _warn_credential_error(provider, value, "ignoring the saved credential") return None +def _warn_credential_error(provider: str, encrypted_value: str, reason: str) -> None: + with _credential_state_lock: + if _warned_credential_errors.get(provider) == encrypted_value: + return + _warned_credential_errors[provider] = encrypted_value + logger.warning("Stored API key for %s cannot be decrypted; %s.", provider, reason) + + def has_decryptable_credential(provider: str) -> bool: - return provider not in _invalid_credentials + with _credential_state_lock: + return provider not in _invalid_credentials def validate_provider(provider: LLMProvider) -> None: @@ -161,8 +179,17 @@ def registry_for_candidate( def effective_registry(session: Session) -> LLMRegistry: """Return a process-local registry rebuilt when persisted provider state changes.""" - global _cached_registry, _cached_fingerprint + global _cache_checked_at, _cached_registry, _cached_fingerprint with _cache_lock: + now = time.monotonic() + if now - _cache_checked_at < _REGISTRY_CHECK_SECONDS: + if _cached_fingerprint == _BASE_CONFIG_FINGERPRINT: + from app.services import llm + + return llm.get_registry() + if _cached_registry is not None: + return _cached_registry + credentials = session.exec(select(LLMProviderCredential)).all() manual_models = session.exec(select(LLMManualModel)).all() runtime_config = session.get(LLMRuntimeConfig, 1) @@ -171,7 +198,8 @@ def effective_registry(session: Session) -> LLMRegistry: from app.services import llm _cached_registry = None - _cached_fingerprint = None + _cached_fingerprint = _BASE_CONFIG_FINGERPRINT + _cache_checked_at = now return llm.get_registry() fingerprint = ( @@ -181,6 +209,7 @@ def effective_registry(session: Session) -> LLMRegistry: allow_external, ) if fingerprint == _cached_fingerprint and _cached_registry is not None: + _cache_checked_at = now return _cached_registry _cached_registry = _build_registry( @@ -189,14 +218,16 @@ def effective_registry(session: Session) -> LLMRegistry: allow_external, ) _cached_fingerprint = fingerprint + _cache_checked_at = now return _cached_registry def invalidate_registry_cache() -> None: - global _cached_registry, _cached_fingerprint + global _cache_checked_at, _cached_registry, _cached_fingerprint with _cache_lock: _cached_registry = None _cached_fingerprint = None + _cache_checked_at = 0.0 def _build_registry( diff --git a/docker/.env.example b/docker/.env.example index 5262795..9f96d77 100644 --- a/docker/.env.example +++ b/docker/.env.example @@ -39,9 +39,12 @@ LLM_MAX_RETRIES= # Empty sends none. Some models (OpenAI reasoning models) reject temperature. LLM_TEMPERATURE=0 -# Optional override for the API/worker key file; Docker's persistent volume is -# mounted at this path by default. If LLM_SECRET_KEY is set, it takes precedence. +# Optional master key override. If unset, the API generates a key on first save +# in the persistent API/worker volume; LLM_SECRET_KEY takes precedence if set. +# Back up that generated key. Removing Compose volumes removes it and saved +# provider credentials must be entered again. # LLM_SECRET_KEY= +# Optional key-file path override (default: /data/secrets/llm_secret.key in Docker). # LLM_SECRET_KEY_FILE=/data/secrets/llm_secret.key # Ollama From 85987c14c9d5d70e1b81585e16a4a14f7e86ffcb Mon Sep 17 00:00:00 2001 From: Jan <1673276@uab.cat> Date: Sat, 24 Oct 2026 12:56:33 +0200 Subject: [PATCH 09/14] test: cover provider credentials and manual models --- tests/test_llm_registry.py | 297 ++++++++++++++++++++++++++++++++++++- tests/test_llm_service.py | 207 ++++++++++++++++++++++++-- 2 files changed, 487 insertions(+), 17 deletions(-) diff --git a/tests/test_llm_registry.py b/tests/test_llm_registry.py index 819fbe3..d801c3f 100644 --- a/tests/test_llm_registry.py +++ b/tests/test_llm_registry.py @@ -10,7 +10,7 @@ from app.api.schemas.enums import InputStatus, InputType from app.core.config import API_PREFIX -from app.models import Input +from app.models import Input, LLMManualModel, LLMProviderCredential, LLMRuntimeConfig from app.services.llm import ( LLM, LLMAuthError, @@ -190,6 +190,29 @@ def test_name_outside_the_list_is_refused_without_asking(self, registry, gemini) registry.resolve(GEMINI, "g9") assert gemini.list_calls == 0 + def test_manual_model_is_allowed_offered_and_resolvable(self): + settings = { + OLLAMA: _provider(OLLAMA, "*", "qwen"), + GEMINI: ProviderSettings( + spec=SPECS[GEMINI], + base_url="http://local:1", + models=ModelSelection.parse("g1,manual-model"), + default_model="g1", + manual_models=("manual-model",), + ), + } + gemini = FakeLLM(GEMINI, ["g1"]) + registry = _registry( + {OLLAMA: FakeLLM(OLLAMA, ["qwen"]), GEMINI: gemini}, + settings=settings, + ) + + catalog = registry.provider_catalog(GEMINI) + assert settings[GEMINI].models.allows("manual-model") + assert catalog.offers("manual-model") + assert next(model for model in catalog.models if model.name == "manual-model").manual is True + assert registry.resolve(GEMINI, "manual-model") == ModelChoice(GEMINI, "manual-model") + def test_unreachable_provider_is_503(self, ollama): registry = _registry({OLLAMA: ollama, GEMINI: FakeLLM(GEMINI, down=True)}) with pytest.raises(LLMUnavailableError, match="Could not reach gemini") as info: @@ -397,6 +420,274 @@ def offer(*names): class TestLLMEndpoints: + def test_save_status_and_delete_credential_never_return_key(self, client, db, monkeypatch, tmp_path): + from cryptography.fernet import Fernet + + from app.core import config + from app.services.llm import user_config + + secret = "custom-provider-secret-value" + monkeypatch.setattr(config, "LLM_SECRET_KEY", Fernet.generate_key().decode("ascii")) + monkeypatch.setattr(config, "LLM_SECRET_KEY_FILE", tmp_path / "unused.key") + db.add(LLMRuntimeConfig(id=1, allow_external=True)) + db.commit() + user_config.invalidate_registry_cache() + try: + saved = client.put( + f"{API_PREFIX}/system/llm/providers/config/custom", + json={"api_key": secret, "base_url": "http://vllm:8000/v1"}, + ) + assert saved.status_code == 200 + assert secret not in saved.text + assert saved.json()["source"] == "ui" + + status = client.get(f"{API_PREFIX}/system/llm/providers/config") + assert status.status_code == 200 + assert secret not in status.text + + deleted = client.delete(f"{API_PREFIX}/system/llm/providers/config/custom") + assert deleted.status_code == 200 + assert db.exec(select(LLMProviderCredential)).all() == [] + assert db.exec(select(LLMManualModel)).all() == [] + finally: + user_config.invalidate_registry_cache() + + def test_validate_provider_candidate_does_not_save_configuration(self, client, db, monkeypatch): + from app.api.routes import llm_config + + custom = LLMProvider.custom + settings = { + OLLAMA: _provider(OLLAMA, "*", "qwen"), + custom: ProviderSettings( + spec=SPECS[custom], + base_url="http://vllm:8000/v1", + models=ModelSelection.parse("model-a"), + default_model="model-a", + ), + } + candidate = _registry( + {OLLAMA: FakeLLM(OLLAMA, ["qwen"]), custom: FakeLLM(custom, ["model-a"])}, + settings=settings, + ) + monkeypatch.setattr(llm_config, "registry_for_candidate", lambda *_args, **_kwargs: candidate) + + response = client.post( + f"{API_PREFIX}/system/llm/providers/config/custom/validate", + json={"base_url": "http://vllm:8000/v1"}, + ) + + assert response.status_code == 200 + assert response.json()["ok"] is True + assert [model["name"] for model in response.json()["models"]] == ["model-a"] + assert db.exec(select(LLMProviderCredential)).all() == [] + + def test_validation_error_redacts_candidate_key(self, client, db, monkeypatch, caplog): + from app.api.routes import llm_config + + secret = "validation-secret-value" + error_registry = _registry( + { + OLLAMA: FakeLLM(OLLAMA, ["qwen"]), + LLMProvider.custom: FakeLLM( + LLMProvider.custom, + list_error=LLMAuthError(f"custom rejected key {secret}"), + ), + }, + settings={ + OLLAMA: _provider(OLLAMA, "*", "qwen"), + LLMProvider.custom: ProviderSettings( + spec=SPECS[LLMProvider.custom], + base_url="http://vllm:8000/v1", + models=ModelSelection.parse("*"), + default_model=None, + ), + }, + ) + monkeypatch.setattr(llm_config, "registry_for_candidate", lambda *_args, **_kwargs: error_registry) + + response = client.post( + f"{API_PREFIX}/system/llm/providers/config/custom/validate", + json={"api_key": secret, "base_url": "http://vllm:8000/v1"}, + ) + + assert response.status_code == 200 + assert response.json()["ok"] is False + assert secret not in response.text + assert "[redacted]" in response.json()["detail"] + assert secret not in caplog.text + assert db.exec(select(LLMProviderCredential)).all() == [] + + def test_custom_local_url_needs_no_consent_but_remote_does(self, client, db): + local = client.put( + f"{API_PREFIX}/system/llm/providers/config/custom", + json={"base_url": "http://host.docker.internal:8000/v1"}, + ) + assert local.status_code == 200 + + blocked = client.put( + f"{API_PREFIX}/system/llm/providers/config/custom", + json={"base_url": "https://llm.example/v1"}, + ) + assert blocked.status_code == 422 + assert blocked.json()["error_code"] == "LLM_EXTERNAL_CONSENT_REQUIRED" + + consent = client.put( + f"{API_PREFIX}/system/llm/external-consent", + json={"allow_external": True}, + ) + assert consent.status_code == 200 + assert consent.json()["allow_external"] is True + + allowed = client.put( + f"{API_PREFIX}/system/llm/providers/config/custom", + json={"base_url": "https://llm.example/v1"}, + ) + assert allowed.status_code == 200 + + def test_external_consent_422_does_not_echo_candidate_key(self, client, db, caplog): + secret = "candidate-secret-never-echo" + + response = client.put( + f"{API_PREFIX}/system/llm/providers/config/openai", + json={"api_key": secret}, + ) + + assert response.status_code == 422 + assert response.json()["error_code"] == "LLM_EXTERNAL_CONSENT_REQUIRED" + assert secret not in response.text + assert secret not in caplog.text + assert db.exec(select(LLMProviderCredential)).all() == [] + + def test_secret_key_unavailable_is_503_without_echoing_key(self, client, db, monkeypatch, tmp_path, caplog): + from app.core import config + from app.services.llm import user_config + + blocker = tmp_path / "not-a-directory" + blocker.write_text("x", encoding="ascii") + monkeypatch.setattr(config, "LLM_SECRET_KEY", "") + monkeypatch.setattr(config, "LLM_SECRET_KEY_FILE", blocker / "key") + secret = "candidate-secret-not-echoed" + user_config.invalidate_registry_cache() + try: + response = client.put( + f"{API_PREFIX}/system/llm/providers/config/custom", + json={"api_key": secret, "base_url": "http://vllm:8000/v1"}, + ) + + assert response.status_code == 503 + assert response.json()["error_code"] == "LLM_SECRET_KEY_UNAVAILABLE" + assert secret not in response.text + assert secret not in caplog.text + assert db.exec(select(LLMProviderCredential)).all() == [] + finally: + user_config.invalidate_registry_cache() + + def test_credential_status_reports_unreadable_saved_key(self, client, db, monkeypatch, test_registry, tmp_path): + from app.core import config + from app.services.llm import user_config + + encrypted = "saved-ciphertext-never-returned" + db.add(LLMProviderCredential(provider="openai", api_key_encrypted=encrypted)) + db.commit() + monkeypatch.setattr(config, "LLM_SECRET_KEY", "") + monkeypatch.setattr(config, "LLM_SECRET_KEY_FILE", tmp_path / "missing.key") + user_config._invalid_credentials.clear() + user_config._warned_credential_errors.clear() + assert user_config.decrypt_api_key(encrypted, "openai") is None + monkeypatch.setattr(user_config, "_build_registry", lambda *_args, **_kwargs: test_registry) + user_config.invalidate_registry_cache() + try: + response = client.get(f"{API_PREFIX}/system/llm/providers/config") + + assert response.status_code == 200 + openai_status = next( + item for item in response.json()["providers"] if item["provider"] == "openai" + ) + assert openai_status["credential_status"] == "unavailable" + assert encrypted not in response.text + finally: + user_config.invalidate_registry_cache() + + def test_models_endpoint_marks_manual_model(self, client, db, monkeypatch): + db.add(LLMManualModel(provider="gemini", model="manual-model")) + db.commit() + settings = { + OLLAMA: _provider(OLLAMA, "*", "qwen"), + GEMINI: ProviderSettings( + spec=SPECS[GEMINI], + base_url="http://local:1", + models=ModelSelection.parse("g1,manual-model"), + default_model="g1", + manual_models=("manual-model",), + ), + } + manual_registry = _registry( + {OLLAMA: FakeLLM(OLLAMA, ["qwen"]), GEMINI: FakeLLM(GEMINI, ["g1"])}, + settings=settings, + ) + monkeypatch.setattr("app.services.llm.effective_registry", lambda _db: manual_registry) + + response = client.get(f"{API_PREFIX}/system/llm/models") + + assert response.status_code == 200 + model = next( + item + for provider in response.json()["providers"] + if provider["provider"] == "gemini" + for item in provider["models"] + if item["name"] == "manual-model" + ) + assert model["manual"] is True + + def test_manual_model_endpoint_validates_persists_and_lists_model(self, client, db, monkeypatch): + from app.api.routes import llm_config + + db.add(LLMProviderCredential(provider="custom", base_url="http://vllm:8000/v1")) + db.commit() + settings = { + OLLAMA: _provider(OLLAMA, "*", "qwen"), + LLMProvider.custom: ProviderSettings( + spec=SPECS[LLMProvider.custom], + base_url="http://vllm:8000/v1", + models=ModelSelection.parse("manual-model"), + default_model=None, + manual_models=("manual-model",), + ), + } + manual_registry = _registry( + { + OLLAMA: FakeLLM(OLLAMA, ["qwen"]), + LLMProvider.custom: FakeLLM(LLMProvider.custom, []), + }, + settings=settings, + ) + monkeypatch.setattr(llm_config, "registry_for_candidate", lambda *_args, **_kwargs: manual_registry) + monkeypatch.setattr("app.services.llm.effective_registry", lambda _db: manual_registry) + + response = client.post( + f"{API_PREFIX}/system/llm/providers/config/custom/models", + json={"model": "manual-model"}, + ) + + assert response.status_code == 200 + assert response.json()["ok"] is True + assert [model.model for model in db.exec(select(LLMManualModel)).all()] == ["manual-model"] + + catalog = client.get(f"{API_PREFIX}/system/llm/models").json()["providers"] + custom = next(provider for provider in catalog if provider["provider"] == "custom") + assert {"name": "manual-model", "manual": True} in custom["models"] + + def test_deleting_provider_config_also_deletes_manual_models(self, client, db): + db.add(LLMProviderCredential(provider="gemini", key_hint="hint")) + db.add(LLMManualModel(provider="gemini", model="manual-model")) + db.commit() + + response = client.delete(f"{API_PREFIX}/system/llm/providers/config/gemini") + + assert response.status_code == 200 + assert db.exec(select(LLMProviderCredential)).all() == [] + assert db.exec(select(LLMManualModel)).all() == [] + def test_list_models(self, client, ollama_offers): # ollama_offers uses ModelInfo(name=n) without extra fields; FastAPI omits # null fields, so models appear as {"name": "..."}. @@ -582,7 +873,7 @@ def pull(model, on_success=None): resp = client.post(self._PULL, json={"model": "qwen2.5:3b"}) assert resp.status_code == 200 assert "application/x-ndjson" in resp.headers["content-type"] - lines = [json.loads(l) for l in resp.text.strip().split("\n") if l.strip()] + lines = [json.loads(line) for line in resp.text.strip().split("\n") if line.strip()] assert lines[-1]["status"] == "success" assert events == ["success-line-consumed", "catalog-refreshed"] @@ -624,6 +915,6 @@ def pull(model, on_success=None): monkeypatch.setattr(test_llm, "pull_model", pull) resp = client.post(self._PULL, json={"model": "qwen2.5:3b"}) assert resp.status_code == 200 - lines = [json.loads(l) for l in resp.text.strip().split("\n") if l.strip()] + lines = [json.loads(line) for line in resp.text.strip().split("\n") if line.strip()] assert any("error" in p for p in lines) assert not any(p.get("status") == "success" for p in lines) diff --git a/tests/test_llm_service.py b/tests/test_llm_service.py index e95e3f6..40cbe05 100644 --- a/tests/test_llm_service.py +++ b/tests/test_llm_service.py @@ -90,6 +90,14 @@ def test_default_is_ollama_listing_every_model(self): assert llm.host == "http://ollama:11434" assert llm.model == "qwen2.5:1.5b" + def test_provider_settings_repr_never_includes_api_key(self): + secret = "provider-secret-value" + settings = load_settings( + _only("gemini", {"API_KEY": secret, "MODELS": "g"}, LLM_ALLOW_EXTERNAL="true") + ) + + assert secret not in repr(settings.providers[LLMProvider.gemini]) + def test_llm_model_and_timeout_override(self): settings = load_settings(_cfg(LLM_MODEL="llama3:8b", LLM_TIMEOUT="60")) assert settings.default_model == "llama3:8b" @@ -494,6 +502,35 @@ def test_gemini_bad_key_is_an_auth_error(self): assert info.value.message == "gemini rejected the API key: Please pass a valid API key. Check GEMINI_API_KEY." assert info.value.error_code == "LLM_AUTH_FAILED" + def test_api_key_in_provider_error_is_redacted(self): + secret = "sk-example-secret-value" + llm = OpenAICompatibleLLM(LLMProvider.openai, "https://api.example/v1", secret, "gpt", 60) + llm.client = MagicMock() + llm.client.chat.completions.create.side_effect = _status_error( + openai.AuthenticationError, 401, {"error": {"message": f"Invalid key {secret}"}} + ) + with pytest.raises(LLMAuthError) as info: + llm.generate_json("prompt", SCHEMA) + assert secret not in info.value.message + assert "[redacted]" in info.value.message + + def test_non_string_error_redaction_key_is_ignored(self): + from app.services.llm.openai_compatible import _reason + + error = _status_error(openai.BadRequestError, 400, {"error": {"message": "Bad request"}}) + assert _reason(error, MagicMock()) == "Bad request" + + @pytest.mark.parametrize("secret", ["short", "not-needed"]) + def test_short_and_placeholder_keys_are_not_used_for_redaction(self, secret): + from app.services.llm.openai_compatible import _reason + + error = _status_error( + openai.BadRequestError, + 400, + {"error": {"message": f"Request mentioned {secret}"}}, + ) + assert _reason(error, secret) == f"Request mentioned {secret}" + def test_missing_model_names_it(self): llm = _openai_llm() body = {"error": {"message": "The model `nope` does not exist"}} @@ -599,7 +636,7 @@ def test_happy_path_streams_lines_and_calls_callback(self, monkeypatch): fired = [] lines = list(_pull_llm().pull_model("qwen2.5:3b", on_success=lambda: fired.append(1))) - parsed = [json.loads(l) for l in lines] + parsed = [json.loads(line) for line in lines] assert parsed[0]["status"] == "pulling manifest" assert parsed[-1]["status"] == "success" assert fired == [1] @@ -619,7 +656,7 @@ def test_error_after_success_suppresses_success_and_callback(self, monkeypatch): fired = [] lines = list(_pull_llm().pull_model("secret:model", on_success=lambda: fired.append(1))) - parsed = [json.loads(l) for l in lines] + parsed = [json.loads(line) for line in lines] statuses = [p.get("status") for p in parsed] assert "success" not in statuses assert fired == [] @@ -635,8 +672,8 @@ def test_read_timeout_mid_stream_yields_error_no_success(self, monkeypatch): fired = [] lines = list(_pull_llm().pull_model("qwen2.5:3b", on_success=lambda: fired.append(1))) - assert any("error" in json.loads(l) for l in lines) - assert not any(json.loads(l).get("status") == "success" for l in lines) + assert any("error" in json.loads(line) for line in lines) + assert not any(json.loads(line).get("status") == "success" for line in lines) assert fired == [] @pytest.mark.parametrize("exc", [ @@ -654,10 +691,10 @@ def test_connection_lost_mid_stream_yields_error_no_success(self, monkeypatch, e fired = [] lines = list(_pull_llm().pull_model("qwen2.5:3b", on_success=lambda: fired.append(1))) - errors = [json.loads(l) for l in lines if "error" in json.loads(l)] + errors = [json.loads(line) for line in lines if "error" in json.loads(line)] assert errors assert errors[0]["error"] - assert not any(json.loads(l).get("status") == "success" for l in lines) + assert not any(json.loads(line).get("status") == "success" for line in lines) assert fired == [] def test_ollama_down_before_stream_raises_unavailable(self, monkeypatch): @@ -703,7 +740,7 @@ def test_json_line_split_across_chunks(self, monkeypatch): monkeypatch.setattr("app.services.llm.ollama.requests.post", lambda *a, **k: resp) lines = list(_pull_llm().pull_model("qwen2.5:3b")) - parsed = [json.loads(l) for l in lines] + parsed = [json.loads(line) for line in lines] statuses = [p["status"] for p in parsed] assert "pulling manifest" in statuses assert "success" in statuses @@ -723,8 +760,8 @@ def test_empty_lines_and_utf8_content(self, monkeypatch): lines = list(_pull_llm().pull_model("qwen2.5:3b")) # No empty lines yielded - assert all(l.strip() for l in lines) - parsed = [json.loads(l) for l in lines] + assert all(line.strip() for line in lines) + parsed = [json.loads(line) for line in lines] assert parsed[-1]["status"] == "success" def test_no_success_event_returns_error_without_callback(self, monkeypatch): @@ -779,9 +816,12 @@ class TestLLMSecretKey: def _configure_file(self, monkeypatch, path): from app.core import config + from app.services.llm import user_config monkeypatch.setattr(config, "LLM_SECRET_KEY", "") monkeypatch.setattr(config, "LLM_SECRET_KEY_FILE", path) + user_config._invalid_credentials.clear() + user_config._warned_credential_errors.clear() def test_api_generates_key_file_on_first_credential_save(self, tmp_path, monkeypatch): from app.services.llm.user_config import decrypt_api_key, encrypt_api_key @@ -822,18 +862,76 @@ def test_key_file_is_reused_by_a_separate_process(self, tmp_path, monkeypatch): assert result.stdout.strip() == "provider-secret" - def test_environment_key_overrides_file_and_does_not_create_it(self, tmp_path, monkeypatch): + def test_worker_process_sees_provider_credential_saved_in_database(self, tmp_path, monkeypatch): + from sqlmodel import Session, SQLModel, create_engine + + from app.core import config + from app.models import LLMProviderCredential + from app.services.llm.user_config import encrypt_api_key + + database_path = tmp_path / "worker-state.sqlite" + database_url = f"sqlite:///{database_path.as_posix()}" + engine = create_engine(database_url) + SQLModel.metadata.create_all(engine) + secret_key = Fernet.generate_key().decode("ascii") + monkeypatch.setattr(config, "LLM_SECRET_KEY", secret_key) + monkeypatch.setattr(config, "LLM_SECRET_KEY_FILE", tmp_path / "unused.key") + encrypted = encrypt_api_key("worker-provider-secret") + with Session(engine) as session: + session.add( + LLMProviderCredential( + provider="openai", + api_key_encrypted=encrypted, + key_hint="cret", + ) + ) + session.commit() + + env = os.environ.copy() + env.update({ + "DATABASE_URL": database_url, + "LLM_SECRET_KEY": secret_key, + "LLM_PROVIDER": "ollama", + "LLM_ALLOW_EXTERNAL": "true", + "OLLAMA_HOST": "http://localhost:11434", + "OLLAMA_MODELS": "*", + "OPENAI_MODELS": "", + "OPENAI_API_KEY": "", + }) + result = subprocess.run( + [ + sys.executable, + "-c", + "from sqlmodel import Session, create_engine; " + "from app.services.llm.user_config import effective_registry; " + "engine = create_engine(__import__('os').environ['DATABASE_URL']); " + "session = Session(engine); " + "print(','.join(p.value for p in effective_registry(session).providers)); " + "session.close()", + ], + check=True, + capture_output=True, + text=True, + env=env, + ) + + assert "openai" in result.stdout.strip().split(",") + assert "worker-provider-secret" not in result.stdout + result.stderr + + def test_environment_key_overrides_existing_key_file(self, tmp_path, monkeypatch): from app.core import config from app.services.llm.user_config import decrypt_api_key, encrypt_api_key - key_file = tmp_path / "missing.key" + key_file = tmp_path / "existing.key" + file_key = Fernet.generate_key() + key_file.write_bytes(file_key) monkeypatch.setattr(config, "LLM_SECRET_KEY", Fernet.generate_key().decode("ascii")) monkeypatch.setattr(config, "LLM_SECRET_KEY_FILE", key_file) encrypted = encrypt_api_key("provider-secret") assert decrypt_api_key(encrypted, "openai") == "provider-secret" - assert not key_file.exists() + assert key_file.read_bytes() == file_key def test_missing_file_is_warning_for_reader_and_not_created(self, tmp_path, monkeypatch, caplog): from app.services.llm.user_config import decrypt_api_key @@ -864,7 +962,7 @@ def test_unreadable_default_credential_falls_back_to_ollama(self, tmp_path, monk from app.api.schemas.enums import LLMProvider from app.core import config from app.models import LLMProviderCredential - from app.services.llm.user_config import _build_registry + from app.services.llm.user_config import _build_registry, decrypt_api_key self._configure_file(monkeypatch, tmp_path / "missing.key") monkeypatch.setattr(config, "LLM_PROVIDER", "openai") @@ -881,7 +979,88 @@ def test_unreadable_default_credential_falls_back_to_ollama(self, tmp_path, monk ) assert registry.default.provider is LLMProvider.ollama - assert "cannot be decrypted" in caplog.text + assert decrypt_api_key("saved-ciphertext", "openai") is None + assert sum("cannot be decrypted" in record.getMessage() for record in caplog.records) == 1 + + def test_environment_only_registry_remains_the_default(self, monkeypatch, test_registry): + from app.services.llm import user_config + + class EmptySession: + reads = 0 + + def exec(self, _statement): + self.reads += 1 + return SimpleNamespace(all=lambda: []) + + def get(self, _model, _key): + return None + + session = EmptySession() + monkeypatch.setattr(user_config.time, "monotonic", lambda: 100.0) + user_config.invalidate_registry_cache() + try: + assert user_config.effective_registry(session) is test_registry + assert user_config.effective_registry(session) is test_registry + assert session.reads == 2 + finally: + user_config.invalidate_registry_cache() + + def test_effective_registry_checks_database_only_every_few_seconds(self, monkeypatch, test_registry): + from datetime import datetime, timezone + from types import SimpleNamespace + + from app.services.llm import user_config + + now = [100.0] + credential = SimpleNamespace( + provider="gemini", + updated_at=datetime(2026, 1, 1, tzinfo=timezone.utc), + key_hint=None, + base_url=None, + structured_output=None, + api_key_encrypted=None, + ) + + class FakeSession: + def __init__(self): + self.reads = 0 + + def exec(self, _statement): + records = [credential] if self.reads % 2 == 0 else [] + self.reads += 1 + return SimpleNamespace(all=lambda: records) + + def get(self, _model, _key): + return None + + session = FakeSession() + builds = [] + monkeypatch.setattr(user_config.time, "monotonic", lambda: now[0]) + monkeypatch.setattr( + user_config, + "_build_registry", + lambda *_args, **_kwargs: builds.append(1) or test_registry, + ) + user_config.invalidate_registry_cache() + + try: + assert user_config.effective_registry(session) is test_registry + assert user_config.effective_registry(session) is test_registry + assert session.reads == 2 + assert len(builds) == 1 + + now[0] += user_config._REGISTRY_CHECK_SECONDS + 0.1 + user_config.effective_registry(session) + assert session.reads == 4 + assert len(builds) == 1 + + credential.updated_at = datetime(2026, 1, 2, tzinfo=timezone.utc) + now[0] += user_config._REGISTRY_CHECK_SECONDS + 0.1 + user_config.effective_registry(session) + assert session.reads == 6 + assert len(builds) == 2 + finally: + user_config.invalidate_registry_cache() # --------------------------------------------------------------------------- From a9283c182cbb1df9ca4c82e55fd7d58aa28b0cb1 Mon Sep 17 00:00:00 2001 From: Jan <1673276@uab.cat> Date: Sat, 24 Oct 2026 12:56:48 +0200 Subject: [PATCH 10/14] docs: document provider credentials and their limits --- contracts/openapi.yaml | 10 + contracts/path/system.yaml | 212 +++++++++++++++++++ contracts/schemas/system.yaml | 123 +++++++++++ docs/multi-llm-support/README.md | 10 +- docs/multi-llm-support/api.md | 6 + docs/multi-llm-support/configuration.md | 9 +- docs/multi-llm-support/user-configuration.md | 87 ++++++++ 7 files changed, 450 insertions(+), 7 deletions(-) create mode 100644 docs/multi-llm-support/user-configuration.md diff --git a/contracts/openapi.yaml b/contracts/openapi.yaml index 3ebc8f4..e86c308 100644 --- a/contracts/openapi.yaml +++ b/contracts/openapi.yaml @@ -68,6 +68,16 @@ paths: $ref: "path/system.yaml#/llm_test" /api/v1/system/llm/pull: $ref: "path/system.yaml#/llm_pull" + /api/v1/system/llm/providers/config: + $ref: "path/system.yaml#/llm_provider_configs" + /api/v1/system/llm/providers/config/{provider}: + $ref: "path/system.yaml#/llm_provider_config" + /api/v1/system/llm/providers/config/{provider}/validate: + $ref: "path/system.yaml#/llm_provider_validation" + /api/v1/system/llm/providers/config/{provider}/models: + $ref: "path/system.yaml#/llm_manual_models" + /api/v1/system/llm/external-consent: + $ref: "path/system.yaml#/llm_external_consent" # ── Layer 8: Async Jobs ──────────────────────────────────────── /api/v1/jobs/{job_id}: diff --git a/contracts/path/system.yaml b/contracts/path/system.yaml index aeadbb0..f6c1d4f 100644 --- a/contracts/path/system.yaml +++ b/contracts/path/system.yaml @@ -5,6 +5,11 @@ # GET /api/v1/system/llm/models # POST /api/v1/system/llm/test # POST /api/v1/system/llm/pull +# GET /api/v1/system/llm/providers/config +# PUT/DELETE /api/v1/system/llm/providers/config/{provider} +# POST /api/v1/system/llm/providers/config/{provider}/validate +# POST/DELETE /api/v1/system/llm/providers/config/{provider}/models +# GET/PUT /api/v1/system/llm/external-consent health: get: @@ -359,3 +364,210 @@ llm_pull: example: error_code: "LLM_UNAVAILABLE" message: "Could not reach Ollama at http://ollama:11434. Check OLLAMA_HOST: Connection refused" + +llm_provider_configs: + get: + operationId: getLLMProviderConfigs + summary: List provider configuration status + description: | + Returns configuration metadata for editable external providers. API keys + are never returned. credential_status is unavailable when a saved key + cannot be decrypted with the current master key. + tags: [system] + responses: + "200": + description: Provider configuration status + content: + application/json: + schema: + $ref: "../schemas/system.yaml#/LLMProviderCredentialList" + +llm_provider_config: + parameters: + - name: provider + in: path + required: true + schema: + $ref: "../schemas/enums.yaml#/LLMProvider" + put: + operationId: saveLLMProviderConfig + summary: Save provider credentials and custom settings + description: | + Stores API keys encrypted at rest and returns only status metadata. A + remote provider requires external-processing consent. The custom provider + may set a base URL and structured-output behavior. + tags: [system] + requestBody: + required: true + content: + application/json: + schema: + $ref: "../schemas/system.yaml#/LLMProviderCredentialRequest" + example: + api_key: "write-only" + responses: + "200": + description: Saved provider status; API key is never included + content: + application/json: + schema: + $ref: "../schemas/system.yaml#/LLMProviderCredentialStatus" + "422": + description: Invalid provider configuration or external consent is required + content: + application/json: + schema: + $ref: "../schemas/common.yaml#/ErrorResponse" + examples: + external_consent: + value: + error_code: "LLM_EXTERNAL_CONSENT_REQUIRED" + message: "Allow external processing before configuring a provider outside the local network." + "503": + description: The master encryption key cannot be read or created + content: + application/json: + schema: + $ref: "../schemas/common.yaml#/ErrorResponse" + example: + error_code: "LLM_SECRET_KEY_UNAVAILABLE" + message: "LLM master key is unavailable." + delete: + operationId: deleteLLMProviderConfig + summary: Delete provider configuration and its manual models + tags: [system] + responses: + "200": + description: Configuration and associated manual model IDs deleted + content: + application/json: + example: + status: success + "422": + description: Provider cannot be configured through this endpoint + content: + application/json: + schema: + $ref: "../schemas/common.yaml#/ErrorResponse" + +llm_provider_validation: + parameters: + - name: provider + in: path + required: true + schema: + $ref: "../schemas/enums.yaml#/LLMProvider" + post: + operationId: validateLLMProviderConfig + summary: Validate candidate credentials without saving them + description: Queries the provider's model list. Credentials in this request + are used only for validation and are not persisted. + tags: [system] + requestBody: + required: true + content: + application/json: + schema: + $ref: "../schemas/system.yaml#/LLMProviderCredentialRequest" + responses: + "200": + description: Validation outcome; failures are returned with ok false + content: + application/json: + schema: + $ref: "../schemas/system.yaml#/LLMProviderValidationResult" + "422": + description: Provider is not configurable or request fields are invalid + content: + application/json: + schema: + $ref: "../schemas/common.yaml#/ErrorResponse" + +llm_manual_models: + parameters: + - name: provider + in: path + required: true + schema: + $ref: "../schemas/enums.yaml#/LLMProvider" + post: + operationId: validateAndAddLLMManualModel + summary: Validate and save a manually entered model ID + description: | + Sends a short test generation to the selected model before saving its ID. + This is useful for custom OpenAI-compatible servers that do not expose a + model-list endpoint. + tags: [system] + requestBody: + required: true + content: + application/json: + schema: + $ref: "../schemas/system.yaml#/LLMManualModelRequest" + responses: + "200": + description: Validation outcome; successful IDs are persisted + content: + application/json: + schema: + $ref: "../schemas/system.yaml#/LLMManualModelResult" + "422": + description: Provider or model ID is invalid + content: + application/json: + schema: + $ref: "../schemas/common.yaml#/ErrorResponse" + delete: + operationId: deleteLLMManualModel + summary: Delete a manually entered model ID + tags: [system] + requestBody: + required: true + content: + application/json: + schema: + $ref: "../schemas/system.yaml#/LLMManualModelRequest" + responses: + "200": + description: Manual model ID deleted if present + content: + application/json: + example: + status: success + "422": + description: Provider or model ID is invalid + content: + application/json: + schema: + $ref: "../schemas/common.yaml#/ErrorResponse" + +llm_external_consent: + get: + operationId: getLLMExternalConsent + summary: Get external processing consent + tags: [system] + responses: + "200": + description: Effective consent and its source + content: + application/json: + schema: + $ref: "../schemas/system.yaml#/LLMExternalConsent" + put: + operationId: setLLMExternalConsent + summary: Set local external processing consent + description: This setting is local to the installation and is not user-scoped. + tags: [system] + requestBody: + required: true + content: + application/json: + schema: + $ref: "../schemas/system.yaml#/LLMExternalConsentRequest" + responses: + "200": + description: Updated consent status + content: + application/json: + schema: + $ref: "../schemas/system.yaml#/LLMExternalConsent" diff --git a/contracts/schemas/system.yaml b/contracts/schemas/system.yaml index d8ff589..b1869a8 100644 --- a/contracts/schemas/system.yaml +++ b/contracts/schemas/system.yaml @@ -91,6 +91,10 @@ LLMModel: type: boolean nullable: true description: True when the model is present on the Ollama host (Ollama only) + manual: + type: boolean + nullable: true + description: True when the model ID was manually entered and validated DownloadableModel: type: object @@ -159,6 +163,125 @@ LLMProviderModels: items: $ref: "#/DownloadableModel" +LLMProviderCredentialRequest: + type: object + additionalProperties: false + properties: + api_key: + type: string + nullable: true + writeOnly: true + description: Optional provider API key. Never returned by the API. + base_url: + type: string + nullable: true + description: Only accepted for the custom provider + structured_output: + type: boolean + nullable: true + description: Only accepted for the custom provider + +LLMProviderCredentialStatus: + type: object + required: [provider, configured, source, enabled, external] + properties: + provider: + $ref: "enums.yaml#/LLMProvider" + configured: + type: boolean + source: + type: string + enum: [env, ui, none] + key_hint: + type: string + nullable: true + description: Last four characters only; the full key is never returned + enabled: + type: boolean + external: + type: boolean + credential_status: + type: string + nullable: true + enum: [available, unavailable] + base_url: + type: string + nullable: true + structured_output: + type: boolean + nullable: true + +LLMProviderCredentialList: + type: object + required: [providers] + properties: + providers: + type: array + items: + $ref: "#/LLMProviderCredentialStatus" + +LLMExternalConsent: + type: object + required: [allow_external, source] + properties: + allow_external: + type: boolean + source: + type: string + enum: [env, ui, none] + +LLMExternalConsentRequest: + type: object + required: [allow_external] + properties: + allow_external: + type: boolean + +LLMProviderValidationResult: + type: object + required: [provider, ok, models] + properties: + provider: + $ref: "enums.yaml#/LLMProvider" + ok: + type: boolean + models: + type: array + items: + $ref: "#/LLMModel" + detail: + type: string + nullable: true + error_code: + type: string + nullable: true + +LLMManualModelRequest: + type: object + required: [model] + properties: + model: + type: string + maxLength: 128 + pattern: "^[A-Za-z0-9][A-Za-z0-9._\\-/:]*$" + +LLMManualModelResult: + type: object + required: [provider, model, ok] + properties: + provider: + $ref: "enums.yaml#/LLMProvider" + model: + type: string + ok: + type: boolean + detail: + type: string + nullable: true + error_code: + type: string + nullable: true + LLMPullRequest: type: object required: diff --git a/docs/multi-llm-support/README.md b/docs/multi-llm-support/README.md index c9f90b4..c4c3481 100644 --- a/docs/multi-llm-support/README.md +++ b/docs/multi-llm-support/README.md @@ -15,10 +15,11 @@ FireForm can fill forms with several LLM providers at once. Each fill request pi ## How it works 1. At startup the backend reads the env file and turns on every provider whose `_MODELS` is set. -2. It asks each provider for its model list and logs a warning for anything that does not match. -3. `GET /api/v1/system/llm/models` returns the providers and the models that can be picked. -4. A fill request sends `provider` and `model`. The backend checks the pair before it stores or queues anything. -5. A request with neither uses the default, set by `LLM_PROVIDER` and `LLM_MODEL`. +2. The local configuration API can additionally save provider credentials and custom provider settings; those settings are shared by the API and worker. +3. It asks each provider for its model list and logs a warning for anything that does not match. +4. `GET /api/v1/system/llm/models` returns the providers and the models that can be picked. +5. A fill request sends `provider` and `model`. The backend checks the pair before it stores or queues anything. +6. A request with neither uses the default, set by `LLM_PROVIDER` and `LLM_MODEL`, except that an unreadable saved default credential falls back to Ollama when Ollama is enabled. ## Privacy @@ -29,4 +30,5 @@ Incident text holds names, addresses and medical detail. Hosted providers stay o - [Setup](setup.md): turn providers on in the backend. - [Configuration](configuration.md): every env setting. - [API](api.md): endpoints, request fields and error codes. +- [User configuration](user-configuration.md): locally stored credentials, consent, manual models, and limitations. - [Troubleshooting](troubleshooting.md): startup warnings and failed fills. diff --git a/docs/multi-llm-support/api.md b/docs/multi-llm-support/api.md index fb33ad1..781b887 100644 --- a/docs/multi-llm-support/api.md +++ b/docs/multi-llm-support/api.md @@ -2,6 +2,10 @@ Contract: `contracts/path/system.yaml` and `contracts/path/forms.yaml`. +Provider credentials, custom endpoints, external-processing consent, and +manually entered model IDs can also be managed through the endpoints described +in [User configuration](user-configuration.md). + ## List providers and models `GET /api/v1/system/llm/models` @@ -133,6 +137,8 @@ The pair is checked before anything is stored or queued. For jobs, the provider | `LLM_BAD_RESPONSE` | 502 | The provider refused the request or did not return JSON. | | `LLM_UNAVAILABLE` | 503 | Could not reach the provider, or it timed out. | | `LLM_RATE_LIMITED` | 503 | Still rate limited after retries, or out of quota. | +| `LLM_SECRET_KEY_UNAVAILABLE` | 503 | The API cannot read or create the credential-encryption master key. | +| `LLM_EXTERNAL_CONSENT_REQUIRED` | 422 | External-processing consent is required before saving a remote provider. | Every error body has `error_code` and `message`. 503 responses add `retry_after_seconds`. Failed jobs store the same code and message in `error`. diff --git a/docs/multi-llm-support/configuration.md b/docs/multi-llm-support/configuration.md index c5999e0..49663e6 100644 --- a/docs/multi-llm-support/configuration.md +++ b/docs/multi-llm-support/configuration.md @@ -4,7 +4,7 @@ Every LLM setting, as read from the env file. Empty means unset. ## Rules -- A provider is on when its `_MODELS` is not empty. +- An environment-configured provider is on when its `_MODELS` is not empty. A provider configured through the local API is enabled from its saved key or custom URL and uses provider-discovered models. - Hosted providers also need their API key and `LLM_ALLOW_EXTERNAL=true`. - A mistake in the default provider blocks every fill until it is fixed. The app still starts and answers with `LLM_CONFIG_ERROR`. - A mistake in any other provider turns that provider off and logs a warning. The rest keep working. @@ -27,7 +27,7 @@ Every LLM setting, as read from the env file. Empty means unset. | Setting | Meaning | | ---------------------------- | ---------------------------------------------------------------------------------------------- | | `_MODELS` | `*` for every listed model, or a comma separated list. Empty turns the provider off. | -| `_API_KEY` | Required for OpenAI, Gemini and Anthropic. Optional for custom. Not used by Ollama. | +| `_API_KEY` | Environment key: required for OpenAI, Gemini and Anthropic. A local API-saved key can be used instead. Optional for custom. Not used by Ollama. | | `_BASE_URL` | Overrides the provider address. Required for custom. Ollama uses `OLLAMA_HOST` instead. | | `_MAX_TOKENS` | Output token cap. Empty sends none. | | `_STRUCTURED_OUTPUT` | `true` sends the form's JSON schema as `response_format`. Turn off for servers that reject it. | @@ -68,7 +68,10 @@ A URL is local when its host is `localhost`, a loopback or private IP, a bare na ## Docker -Both compose files load the env file into the app and the worker, so every setting above reaches both containers. Do not repeat LLM settings under `environment:` in a compose file. Values there override the env file. +Both compose files load the env file into the app and the worker, so every setting above reaches both containers. They also share a persistent `llm_secrets` volume for the generated master key. Do not repeat LLM settings under `environment:` in a compose file. Values there override the env file. + +The local provider API, credential encryption key, and single-user limitations +are described in [User configuration](user-configuration.md). ## Removed settings diff --git a/docs/multi-llm-support/user-configuration.md b/docs/multi-llm-support/user-configuration.md new file mode 100644 index 0000000..3892868 --- /dev/null +++ b/docs/multi-llm-support/user-configuration.md @@ -0,0 +1,87 @@ +# Local provider configuration + +The following endpoints configure provider credentials and model IDs for one +FireForm installation: + +| Method and path | Purpose | +| --- | --- | +| `GET /api/v1/system/llm/providers/config` | Read provider configuration status; never returns full API keys | +| `PUT /api/v1/system/llm/providers/config/{provider}` | Save an API key and, for `custom`, a base URL and structured-output option | +| `DELETE /api/v1/system/llm/providers/config/{provider}` | Remove the provider configuration and its manually added models | +| `POST /api/v1/system/llm/providers/config/{provider}/validate` | Test candidate credentials without saving them | +| `POST /api/v1/system/llm/providers/config/{provider}/models` | Test a model ID with a generation request and save it when successful | +| `DELETE /api/v1/system/llm/providers/config/{provider}/models` | Remove a manually added model ID | +| `GET /api/v1/system/llm/external-consent` | Read the installation-wide external-processing consent | +| `PUT /api/v1/system/llm/external-consent` | Change that consent | + +The `provider` path value is `openai`, `gemini`, `anthropic`, or `custom`. +Ollama continues to be configured through its local service settings. A saved +credential takes precedence over the corresponding environment key. A +manually entered model ID is checked by sending a short test generation; it is +then included in `/api/v1/system/llm/models` with `"manual": true`. For custom +OpenAI-compatible servers, manual entry is the usual route when the server +does not expose a compatible model-list endpoint. + +## Master-key lifecycle + +If `LLM_SECRET_KEY` is set in the API and worker environments, it takes +precedence. Otherwise, when the API first saves a credential, it generates a +Fernet key and creates `LLM_SECRET_KEY_FILE` with exclusive creation and +restrictive permissions. The worker reads that file from the shared persistent +`llm_secrets` Docker volume; it never creates the key. The default container +path is `/data/secrets/llm_secret.key`. An unreadable or invalid key means saved +credentials are ignored and reported as unavailable rather than crashing the +worker. If the configured default provider depends on such a credential and +Ollama is enabled, FireForm falls back to Ollama. + +Back up the generated key securely and separately from the database. A +database-only backup cannot decrypt saved provider credentials without the +master key. This is not protection from someone who can access the machine or +the running containers: that person may be able to read the key file and +credentials. If `docker compose down -v` removes the `llm_secrets` volume, the +saved encrypted credentials become unusable; provide the keys again through +the configuration interface. Restoring the volume/key is required to restore +the existing credentials. + +## Limitations and deployment safety + +This is a **local, single-user** configuration feature, not user-account +management. The provider configuration, manual models, and external-processing +consent are global to the installation. The configuration routes do not have +authentication, and the Compose app port is published on `0.0.0.0:8000` by +default. For a local-only installation, bind the app port to loopback, for +example `127.0.0.1:8000:8000`, and do not expose it to an untrusted network. + +Before deploying this design as a multi-user client-server service, it would +need at least: + +- authentication and authorization for configuration and model routes; +- credentials and manually selected models scoped to each user; +- external-processing consent set by an authorized administrator, rather than + a global user-controlled flag; +- an authorized credential reference attached to jobs (never the key itself); +- a registry/cache scoped to the authenticated user; +- strict custom-URL policy and network-level egress controls; and +- managed master-key provisioning, rotation, backup, and recovery. + +Custom URL validation rejects malformed URLs and embedded credentials, but it +is **not complete SSRF protection**. DNS changes, rebinding, redirects, and +network routing can still make an apparently acceptable hostname reach an +unintended destination. Do not accept arbitrary custom URLs from untrusted +clients. + +The consent flag only gates calls to external providers; it is not an +authentication boundary. A custom local server does not require external +consent. From inside Docker, address a server on the host as +`http://host.docker.internal:/v1` (not `localhost`); use a service name +for another Compose service. + +Only the full API key is secret: API responses expose status metadata and, when +available, a last-four-character hint. Keys are encrypted in the database and +are not included in validation errors or logs. `LLM_SECRET_KEY_UNAVAILABLE` +means the API could not read or create the master key; the worker treats an +unreadable saved credential as unconfigured and logs a warning. + +The `LLM_EXTERNAL_CONSENT_REQUIRED` error is HTTP 422 and blocks saving a +remote provider or custom URL until consent is enabled. A failure to access the +master key is HTTP 503 with `LLM_SECRET_KEY_UNAVAILABLE`. From 6c9d4bd482eec1fd9f57d1166461e1764817ab02 Mon Sep 17 00:00:00 2001 From: Jan <1673276@uab.cat> Date: Mon, 26 Oct 2026 10:50:32 +0100 Subject: [PATCH 11/14] feat: list only text-generating Gemini models --- app/services/llm/openai_compatible.py | 134 ++++++++++++++- app/services/llm/specs.py | 30 +++- docs/multi-llm-support/configuration.md | 27 ++++ docs/multi-llm-support/troubleshooting.md | 6 +- tests/test_llm_registry.py | 125 ++++++++++++++ tests/test_llm_service.py | 189 +++++++++++++++++++++- 6 files changed, 500 insertions(+), 11 deletions(-) diff --git a/app/services/llm/openai_compatible.py b/app/services/llm/openai_compatible.py index e6d92e4..df5e5d1 100644 --- a/app/services/llm/openai_compatible.py +++ b/app/services/llm/openai_compatible.py @@ -5,10 +5,12 @@ Only the base URL and the API key differ. """ +import logging import time from contextlib import contextmanager import openai +import requests from app.api.schemas.enums import HealthState, LLMProvider from app.services.llm.base import LLM, PROBE_TIMEOUT_SECONDS, LLMHealth, ModelInfo @@ -22,6 +24,8 @@ ) from app.services.llm.specs import SPECS +logger = logging.getLogger(__name__) + class OpenAICompatibleLLM(LLM): def __init__( @@ -44,7 +48,7 @@ def __init__( self.structured_output = structured_output self.max_tokens = max_tokens self.temperature = temperature - self.listed_id_prefix = listed_id_prefix + self.listed_id_prefix = listed_id_prefix or SPECS[provider].listed_id_prefix self.client = openai.OpenAI( base_url=base_url, api_key=self._secret or "not-needed", @@ -86,9 +90,115 @@ def _generate(self, prompt: str, schema: dict, model: str) -> str: def list_models(self) -> list[ModelInfo]: probe = self.client.with_options(timeout=PROBE_TIMEOUT_SECONDS, max_retries=0) + spec = SPECS[self.provider] + if spec.native_model_list_url: + models = self._list_native_models(spec) + if models is None: + logger.warning( + "%s native model listing returned 404 or non-JSON; " + "falling back to the compatible model list without capability filtering", + self.provider.value, + ) + return self._list_compatible_models(probe) + else: + models = self._list_compatible_models(probe) + + markers = tuple(marker.lower() for marker in spec.excluded_id_markers) + if markers: + models = [ + model for model in models + if not any(marker in model.name.lower() for marker in markers) + ] + return models + + def _list_compatible_models(self, probe) -> list[ModelInfo]: with self._translate_errors(): return [ModelInfo(name=m.id.removeprefix(self.listed_id_prefix)) for m in probe.models.list()] + def _list_native_models(self, spec) -> list[ModelInfo] | None: + """List a provider's models with capability metadata; None requests fallback.""" + models: list[ModelInfo] = [] + page_token: str | None = None + + for _ in range(spec.model_list_page_limit): + params = {"pageToken": page_token} if page_token else {} + try: + response = requests.get( + spec.native_model_list_url, + headers={spec.native_model_list_api_key_header: self._secret}, + params=params, + timeout=PROBE_TIMEOUT_SECONDS, + ) + except requests.Timeout as exc: + raise LLMUnavailableError(f"{self.provider.value} model listing timed out") from exc + except requests.RequestException as exc: + raise LLMUnavailableError( + f"Could not reach {self.provider.value} at {spec.native_model_list_url}." + ) from exc + + status = response.status_code + if status == 404: + return None + if status in (401, 403): + raise self._native_auth_error(response) + if status == 400: + reason = _native_error_reason(response, self._secret) + if "api key" in reason.lower(): + raise self._native_auth_error(response) + raise LLMResponseError(f"{self.provider.value} refused the model list request with 400: {reason}") + if status == 429: + reason = _native_error_reason(response, self._secret) + raise LLMRateLimitError(f"{self.provider.value} is rate limiting model-list requests: {reason}") + if status >= 500: + raise LLMUnavailableError(f"{self.provider.value} had a server error {status}") + if status >= 400: + reason = _native_error_reason(response, self._secret) + raise LLMResponseError( + f"{self.provider.value} refused the model list request with {status}: {reason}" + ) + + try: + body = response.json() + except ValueError: + return None + if not isinstance(body, dict) or not isinstance(body.get("models"), list): + raise LLMResponseError(f"{self.provider.value} returned an invalid model-list response") + + for item in body["models"]: + if not isinstance(item, dict) or not isinstance(item.get("name"), str): + continue + methods = item.get("supportedGenerationMethods") + if ( + spec.required_generation_method + and "supportedGenerationMethods" in item + and ( + not isinstance(methods, list) + or spec.required_generation_method not in methods + ) + ): + continue + models.append(ModelInfo(name=item["name"].removeprefix(self.listed_id_prefix))) + + page_token = body.get("nextPageToken") + if not page_token: + break + else: + if page_token: + logger.warning( + "%s native model listing reached the %d-page limit; results may be incomplete", + self.provider.value, + spec.model_list_page_limit, + ) + + return models + + def _native_auth_error(self, response: requests.Response) -> LLMAuthError: + return LLMAuthError( + f"{self.provider.value} rejected the API key: " + f"{_native_error_reason(response, self._secret)}. " + f"Check {SPECS[self.provider].setting('API_KEY')}." + ) + def health(self) -> LLMHealth: t0 = time.monotonic() try: @@ -165,6 +275,26 @@ def _reason(exc: openai.APIStatusError, secret: object = "") -> str: message = str(error["message"]).rstrip(".") elif isinstance(error, str): message = error.rstrip(".") + return _redact_secret(message, secret) + + +def _native_error_reason(response: requests.Response, secret: str) -> str: + try: + body = response.json() + except ValueError: + body = None + if isinstance(body, list) and body: + body = body[0] + if isinstance(body, dict): + error = body.get("error", body) + if isinstance(error, dict) and isinstance(error.get("message"), str): + return _redact_secret(error["message"].rstrip("."), secret) + if isinstance(error, str): + return _redact_secret(error.rstrip("."), secret) + return _redact_secret("request rejected", secret) + + +def _redact_secret(message: str, secret: object) -> str: if isinstance(secret, str) and len(secret) >= 8 and secret != "not-needed": - message = message.replace(secret, "[redacted]") + return message.replace(secret, "[redacted]") return message diff --git a/app/services/llm/specs.py b/app/services/llm/specs.py index d10b4f0..1a48bec 100644 --- a/app/services/llm/specs.py +++ b/app/services/llm/specs.py @@ -19,6 +19,11 @@ class ProviderSpec: max_tokens: int | None = None headers: tuple[tuple[str, str], ...] = () listed_id_prefix: str = "" + native_model_list_url: str = "" + native_model_list_api_key_header: str = "" + required_generation_method: str = "" + model_list_page_limit: int = 20 + excluded_id_markers: tuple[str, ...] = () @property def prefix(self) -> str: @@ -34,9 +39,29 @@ def setting(self, key: str) -> str: SPECS: dict[LLMProvider, ProviderSpec] = { LLMProvider.ollama: ProviderSpec(LLMProvider.ollama, needs_key=False), - LLMProvider.openai: ProviderSpec(LLMProvider.openai, "https://api.openai.com/v1"), + LLMProvider.openai: ProviderSpec( + LLMProvider.openai, + "https://api.openai.com/v1", + excluded_id_markers=( + "embedding", + "whisper", + "tts", + "dall-e", + "moderation", + "transcribe", + "realtime", + "image", + "audio", + ), + ), LLMProvider.gemini: ProviderSpec( - LLMProvider.gemini, "https://generativelanguage.googleapis.com/v1beta/openai/", listed_id_prefix="models/" + LLMProvider.gemini, + "https://generativelanguage.googleapis.com/v1beta/openai/", + listed_id_prefix="models/", + native_model_list_url="https://generativelanguage.googleapis.com/v1beta/models", + native_model_list_api_key_header="x-goog-api-key", + required_generation_method="generateContent", + model_list_page_limit=20, ), LLMProvider.anthropic: ProviderSpec( LLMProvider.anthropic, @@ -54,4 +79,3 @@ def setting(self, key: str) -> str: "qwen2.5:7b", "llama3.2:3b", ) - diff --git a/docs/multi-llm-support/configuration.md b/docs/multi-llm-support/configuration.md index 49663e6..8b267b1 100644 --- a/docs/multi-llm-support/configuration.md +++ b/docs/multi-llm-support/configuration.md @@ -62,6 +62,33 @@ Every LLM setting, as read from the env file. Empty means unset. | Anthropic | `https://api.anthropic.com/v1/` | off | `12288` | | Custom | `CUSTOM_BASE_URL` | on | none | +## Models shown in the catalog + +Gemini models are listed through Gemini's native model-list API. FireForm +removes a model only when Gemini provides `supportedGenerationMethods` and it +does not include `generateContent`; models without that field are retained. +The API key is sent in the `x-goog-api-key` header. If native listing returns +404 or non-JSON, FireForm warns and falls back to the compatible model list +without the capability filter. Listing follows `nextPageToken` for at most 20 +pages and warns if that limit is reached. + +OpenAI only provides model IDs in its list, so FireForm approximately excludes +IDs containing `embedding`, `whisper`, `tts`, `dall-e`, `moderation`, +`transcribe`, `realtime`, `image` or `audio` (case-insensitive). This is a +name-based heuristic and can be wrong as model names change. Other providers, +including Anthropic, custom servers and Ollama, are not filtered by these +rules. A filtered model can still be entered and tested as a manual model. + +Filtering applies at the provider model-list source. It therefore affects the +catalog, startup `check_config()` and candidate configuration validation. A +filtered configured name is treated as missing and gets the usual warning. +The configured default model is not replaced automatically: startup warns that +it is not offered and fills will fail, and FireForm does not switch to another +provider. The existing `probe(..., verify=False)` path used by +`POST /system/llm/test` and manual-model registration remains an intentional +escape hatch; a model added manually is included in the catalog even if the +provider's list filter would exclude it. + ## What counts as local A URL is local when its host is `localhost`, a loopback or private IP, a bare name such as a Docker service (`vllm`, `ollama`), or ends in `.local`, `.internal` or `.lan`. Anything else needs `LLM_ALLOW_EXTERNAL=true`. diff --git a/docs/multi-llm-support/troubleshooting.md b/docs/multi-llm-support/troubleshooting.md index f13f7d3..9cd589d 100644 --- a/docs/multi-llm-support/troubleshooting.md +++ b/docs/multi-llm-support/troubleshooting.md @@ -47,4 +47,8 @@ The log also records when a provider goes down and when it comes back. ## Noisy model list -`*` returns everything the provider lists, including speech, image and embedding models. Use an explicit `_MODELS` list to show only the models that can fill forms. +Gemini is filtered using its `generateContent` capability, and OpenAI uses an +approximate case-insensitive ID-marker filter. Other providers may list +non-text models; use an explicit `_MODELS` list to limit those. See +[Models shown in the catalog](configuration.md#models-shown-in-the-catalog) +for the filter behavior and its manual-model escape hatch. diff --git a/tests/test_llm_registry.py b/tests/test_llm_registry.py index d801c3f..ec288da 100644 --- a/tests/test_llm_registry.py +++ b/tests/test_llm_registry.py @@ -4,6 +4,9 @@ The registry tests use FakeLLM, so nothing goes over the network. """ +from types import SimpleNamespace +from unittest.mock import MagicMock + import pytest import requests from sqlmodel import select @@ -25,6 +28,7 @@ ProviderSettings, ) from app.services.llm.registry import CACHE_SECONDS, FAILURE_CACHE_SECONDS +from app.services.llm.openai_compatible import OpenAICompatibleLLM from app.services.llm.selection import ModelSelection from app.services.llm.specs import DEFAULT_OLLAMA_INSTALLABLE_MODELS, SPECS @@ -88,6 +92,21 @@ def _registry(clients, settings=None, warnings=(), clock=None): return LLMRegistry(llm_settings, clients, clock=clock or Clock()) +def _openai_listing(ids): + client = OpenAICompatibleLLM( + OPENAI, + "https://api.openai.com/v1", + "test-key", + "gpt-4o-mini", + 5, + ) + client.client = MagicMock() + client.client.with_options.return_value.models.list.return_value = [ + SimpleNamespace(id=model_id) for model_id in ids + ] + return client + + @pytest.fixture def ollama(): return FakeLLM(OLLAMA, ["qwen", "llama"]) @@ -213,6 +232,48 @@ def test_manual_model_is_allowed_offered_and_resolvable(self): assert next(model for model in catalog.models if model.name == "manual-model").manual is True assert registry.resolve(GEMINI, "manual-model") == ModelChoice(GEMINI, "manual-model") + def test_openai_filtered_model_is_not_offered_or_resolvable(self): + openai_client = _openai_listing(["text-embedding-3-small", "gpt-4o-mini"]) + settings = { + OLLAMA: _provider(OLLAMA, "*", "qwen"), + OPENAI: _provider(OPENAI, "*", "gpt-4o-mini"), + } + registry = _registry( + {OLLAMA: FakeLLM(OLLAMA, ["qwen"]), OPENAI: openai_client}, + settings=settings, + ) + + catalog = registry.provider_catalog(OPENAI) + + assert [model.name for model in catalog.models] == ["gpt-4o-mini"] + with pytest.raises(LLMNotAvailableError, match="does not offer text-embedding-3-small"): + registry.resolve(OPENAI, "text-embedding-3-small") + + def test_manual_model_can_explicitly_restore_filtered_model(self): + openai_client = _openai_listing(["text-embedding-3-small", "gpt-4o-mini"]) + settings = { + OLLAMA: _provider(OLLAMA, "*", "qwen"), + OPENAI: ProviderSettings( + spec=SPECS[OPENAI], + base_url="http://local:1", + models=ModelSelection.parse("*"), + default_model="gpt-4o-mini", + manual_models=("text-embedding-3-small",), + ), + } + registry = _registry( + {OLLAMA: FakeLLM(OLLAMA, ["qwen"]), OPENAI: openai_client}, + settings=settings, + ) + + catalog = registry.provider_catalog(OPENAI) + + assert [model.name for model in catalog.models] == ["gpt-4o-mini", "text-embedding-3-small"] + assert catalog.models[1].manual is True + assert registry.resolve(OPENAI, "text-embedding-3-small") == ModelChoice( + OPENAI, "text-embedding-3-small" + ) + def test_unreachable_provider_is_503(self, ollama): registry = _registry({OLLAMA: ollama, GEMINI: FakeLLM(GEMINI, down=True)}) with pytest.raises(LLMUnavailableError, match="Could not reach gemini") as info: @@ -381,6 +442,24 @@ def test_check_config(self): "GEMINI_MODELS has g2, but gemini does not offer it. It is left out of the model list.", ] + def test_filtered_default_warns_without_crashing_or_switching_provider(self): + openai_client = _openai_listing(["text-embedding-3-small", "gpt-4o-mini"]) + settings = { + OPENAI: _provider(OPENAI, "*", "text-embedding-3-small"), + } + registry = LLMRegistry( + LLMSettings(providers=settings, default_provider=OPENAI), + {OPENAI: openai_client}, + ) + + warnings = ModelValidator(registry).check_config() + + assert warnings == [ + "openai does not offer the default model text-embedding-3-small. Fills will fail." + ] + assert registry.resolve() == ModelChoice(OPENAI, "text-embedding-3-small") + assert registry.default.provider is OPENAI + def test_check_config_unreachable(self, ollama): registry = _registry({OLLAMA: ollama, GEMINI: FakeLLM(GEMINI, down=True)}) assert ModelValidator(registry).check_config() == [ @@ -481,6 +560,52 @@ def test_validate_provider_candidate_does_not_save_configuration(self, client, d assert [model["name"] for model in response.json()["models"]] == ["model-a"] assert db.exec(select(LLMProviderCredential)).all() == [] + def test_models_endpoint_uses_provider_source_filter(self, client, monkeypatch): + registry = _registry( + { + OLLAMA: FakeLLM(OLLAMA, ["qwen"]), + OPENAI: _openai_listing(["text-embedding-3-small", "gpt-4o-mini"]), + }, + settings={ + OLLAMA: _provider(OLLAMA, "*", "qwen"), + OPENAI: _provider(OPENAI, "*", "gpt-4o-mini"), + }, + ) + monkeypatch.setattr("app.services.llm.effective_registry", lambda _db: registry) + + response = client.get(f"{API_PREFIX}/system/llm/models") + + assert response.status_code == 200 + openai_provider = next( + item for item in response.json()["providers"] if item["provider"] == "openai" + ) + assert [model["name"] for model in openai_provider["models"]] == ["gpt-4o-mini"] + + def test_candidate_validation_uses_provider_source_filter(self, client, monkeypatch): + from app.api.routes import llm_config + + candidate = _registry( + { + OLLAMA: FakeLLM(OLLAMA, ["qwen"]), + OPENAI: _openai_listing(["text-embedding-3-small", "gpt-4o-mini"]), + }, + settings={ + OLLAMA: _provider(OLLAMA, "*", "qwen"), + OPENAI: _provider(OPENAI, "*", "gpt-4o-mini"), + }, + ) + monkeypatch.setattr(llm_config, "registry_for_candidate", lambda *_args, **_kwargs: candidate) + monkeypatch.setattr(llm_config, "_require_external_consent", lambda *_args, **_kwargs: None) + + response = client.post( + f"{API_PREFIX}/system/llm/providers/config/openai/validate", + json={"api_key": "candidate-key"}, + ) + + assert response.status_code == 200 + assert response.json()["ok"] is True + assert [model["name"] for model in response.json()["models"]] == ["gpt-4o-mini"] + def test_validation_error_redacts_candidate_key(self, client, db, monkeypatch, caplog): from app.api.routes import llm_config diff --git a/tests/test_llm_service.py b/tests/test_llm_service.py index 40cbe05..7fa45ab 100644 --- a/tests/test_llm_service.py +++ b/tests/test_llm_service.py @@ -210,13 +210,21 @@ def test_hosted_preset_url_and_key(self): assert llm.temperature == 0.0 assert llm.max_tokens is None - def test_gemini_ids_lose_their_prefix(self): + def test_gemini_ids_lose_their_prefix(self, monkeypatch): settings = load_settings(_only("gemini", {"API_KEY": "gk", "MODELS": "g"}, LLM_ALLOW_EXTERNAL="true")) llm = create_llm(settings.providers[LLMProvider.gemini]) - llm.client = MagicMock() - llm.client.with_options.return_value.models.list.return_value = [ - SimpleNamespace(id="models/gemini-2.5-flash"), SimpleNamespace(id="gemini-3.8-flash"), - ] + response = MagicMock() + response.status_code = 200 + response.json.return_value = { + "models": [ + {"name": "models/gemini-2.5-flash"}, + {"name": "gemini-3.8-flash"}, + ], + } + monkeypatch.setattr( + "app.services.llm.openai_compatible.requests.get", + lambda *_args, **_kwargs: response, + ) assert [m.name for m in llm.list_models()] == ["gemini-2.5-flash", "gemini-3.8-flash"] def test_anthropic_sends_version_header(self): @@ -433,6 +441,177 @@ def _status_error(cls, code, body=None): class TestOpenAICompatibleLLM: + @staticmethod + def _gemini(): + return OpenAICompatibleLLM( + LLMProvider.gemini, + "https://generativelanguage.googleapis.com/v1beta/openai/", + "gemini-secret-value", + "gemini-2.5-flash", + 60, + ) + + @staticmethod + def _response(status_code=200, body=None, text=""): + response = MagicMock() + response.status_code = status_code + response.text = text + response.json.side_effect = ValueError("not JSON") if body is None else None + if body is not None: + response.json.return_value = body + return response + + def test_gemini_native_list_filters_capabilities_and_paginates(self, monkeypatch): + llm = self._gemini() + responses = iter([ + self._response(200, { + "models": [ + {"name": "models/gemini-text", "supportedGenerationMethods": ["generateContent"]}, + {"name": "models/gemini-image", "supportedGenerationMethods": ["predict"]}, + {"name": "models/gemini-unknown"}, + ], + "nextPageToken": "next-page", + }), + self._response(200, { + "models": [ + {"name": "models/gemini-second", "supportedGenerationMethods": ["generateContent"]}, + ], + }), + ]) + calls = [] + + def fake_get(url, **kwargs): + calls.append((url, kwargs)) + return next(responses) + + monkeypatch.setattr("app.services.llm.openai_compatible.requests.get", fake_get) + + assert [model.name for model in llm.list_models()] == [ + "gemini-text", "gemini-unknown", "gemini-second", + ] + assert len(calls) == 2 + assert calls[0][0] == "https://generativelanguage.googleapis.com/v1beta/models" + assert calls[0][1]["headers"] == {"x-goog-api-key": "gemini-secret-value"} + assert calls[0][1]["params"] == {} + assert calls[1][1]["params"] == {"pageToken": "next-page"} + assert all("pageSize" not in call[1]["params"] for call in calls) + assert all("gemini-secret-value" not in call[0] for call in calls) + + def test_gemini_native_single_page(self, monkeypatch): + llm = self._gemini() + monkeypatch.setattr( + "app.services.llm.openai_compatible.requests.get", + lambda *_args, **_kwargs: self._response(200, {"models": [{"name": "models/one"}]}), + ) + assert [model.name for model in llm.list_models()] == ["one"] + + def test_gemini_native_listing_stops_at_page_limit(self, monkeypatch, caplog): + llm = self._gemini() + calls = [] + + def fake_get(_url, **kwargs): + calls.append(kwargs) + return self._response(200, { + "models": [{"name": f"models/page-{len(calls)}"}], + "nextPageToken": f"token-{len(calls)}", + }) + + monkeypatch.setattr("app.services.llm.openai_compatible.requests.get", fake_get) + + models = llm.list_models() + + assert len(calls) == 20 + assert len(models) == 20 + assert "20-page limit" in caplog.text + + @pytest.mark.parametrize("status_code, body", [ + (400, [{"error": {"message": "API key not valid: gemini-secret-value"}}]), + (403, {"error": {"message": "permission denied"}}), + ]) + def test_gemini_native_auth_errors_are_redacted(self, monkeypatch, caplog, status_code, body): + llm = self._gemini() + monkeypatch.setattr( + "app.services.llm.openai_compatible.requests.get", + lambda *_args, **_kwargs: self._response(status_code, body), + ) + + with pytest.raises(LLMAuthError) as info: + llm.list_models() + + assert "gemini-secret-value" not in info.value.message + assert "gemini-secret-value" not in caplog.text + assert info.value.error_code == "LLM_AUTH_FAILED" + + def test_gemini_native_rate_limit_error(self, monkeypatch): + llm = self._gemini() + monkeypatch.setattr( + "app.services.llm.openai_compatible.requests.get", + lambda *_args, **_kwargs: self._response(429, {"error": {"message": "slow down"}}), + ) + with pytest.raises(LLMRateLimitError): + llm.list_models() + + @pytest.mark.parametrize("failure", [ + requests.Timeout("simulated timeout"), + requests.ConnectionError("simulated connection failure"), + ]) + def test_gemini_native_network_errors_are_unavailable(self, monkeypatch, failure): + llm = self._gemini() + monkeypatch.setattr( + "app.services.llm.openai_compatible.requests.get", + MagicMock(side_effect=failure), + ) + with pytest.raises(LLMUnavailableError): + llm.list_models() + + def test_gemini_native_server_error_is_unavailable(self, monkeypatch): + llm = self._gemini() + monkeypatch.setattr( + "app.services.llm.openai_compatible.requests.get", + lambda *_args, **_kwargs: self._response(503, {"error": {"message": "maintenance"}}), + ) + with pytest.raises(LLMUnavailableError): + llm.list_models() + + @pytest.mark.parametrize("response", [ + _response.__func__(404, {"error": {"message": "not found"}}), + _response.__func__(200, None, "not JSON"), + ]) + def test_gemini_native_falls_back_to_compatible_list(self, monkeypatch, caplog, response): + llm = self._gemini() + llm.client = MagicMock() + llm.client.with_options.return_value.models.list.return_value = [ + SimpleNamespace(id="models/legacy-model"), + ] + monkeypatch.setattr( + "app.services.llm.openai_compatible.requests.get", + lambda *_args, **_kwargs: response, + ) + + assert [model.name for model in llm.list_models()] == ["legacy-model"] + assert "without capability filtering" in caplog.text + + def test_openai_excludes_non_text_model_id_markers_only_for_openai(self): + ids = [ + "text-embedding-3-small", "whisper-1", "tts-1", "dall-e-3", + "moderation-latest", "gpt-transcribe", "gpt-realtime", "gpt-image", + "gpt-audio", "gpt-4o-mini", + ] + openai_llm = OpenAICompatibleLLM( + LLMProvider.openai, "https://api.openai.com/v1", "key", "gpt-4o-mini", 60 + ) + custom_llm = OpenAICompatibleLLM( + LLMProvider.custom, "http://local:8000/v1", "", "gpt-4o-mini", 60 + ) + for llm in (openai_llm, custom_llm): + llm.client = MagicMock() + llm.client.with_options.return_value.models.list.return_value = [ + SimpleNamespace(id=model_id) for model_id in ids + ] + + assert [model.name for model in openai_llm.list_models()] == ["gpt-4o-mini"] + assert [model.name for model in custom_llm.list_models()] == ids + def test_structured_output_sends_schema(self): llm = _openai_llm(structured_output=True, temperature=0.0) assert llm.generate_json("prompt", SCHEMA) == '{"name": "Ana"}' From f1ebfc2700497ec03443b4c04b8ac6b26fd93e43 Mon Sep 17 00:00:00 2001 From: Jan <1673276@uab.cat> Date: Mon, 26 Oct 2026 16:06:34 +0100 Subject: [PATCH 12/14] fix(llm): filter non-text Gemini models by ID fragments --- app/services/llm/openai_compatible.py | 15 +++- app/services/llm/specs.py | 16 ++++ docs/multi-llm-support/configuration.md | 20 +++-- docs/multi-llm-support/troubleshooting.md | 4 +- tests/test_llm_registry.py | 23 ++++++ tests/test_llm_service.py | 93 ++++++++++++++++++++++- 6 files changed, 158 insertions(+), 13 deletions(-) diff --git a/app/services/llm/openai_compatible.py b/app/services/llm/openai_compatible.py index df5e5d1..193567b 100644 --- a/app/services/llm/openai_compatible.py +++ b/app/services/llm/openai_compatible.py @@ -6,6 +6,7 @@ """ import logging +import re import time from contextlib import contextmanager @@ -99,7 +100,7 @@ def list_models(self) -> list[ModelInfo]: "falling back to the compatible model list without capability filtering", self.provider.value, ) - return self._list_compatible_models(probe) + models = self._list_compatible_models(probe) else: models = self._list_compatible_models(probe) @@ -107,7 +108,7 @@ def list_models(self) -> list[ModelInfo]: if markers: models = [ model for model in models - if not any(marker in model.name.lower() for marker in markers) + if not _has_excluded_id_marker(model.name, markers) ] return models @@ -298,3 +299,13 @@ def _redact_secret(message: str, secret: object) -> str: if isinstance(secret, str) and len(secret) >= 8 and secret != "not-needed": return message.replace(secret, "[redacted]") return message + + +def _has_excluded_id_marker(model_id: str, markers: tuple[str, ...]) -> bool: + fragments = re.split(r"[-._]+", model_id.lower()) + for marker in markers: + marker_fragments = re.split(r"[-._]+", marker.lower()) + width = len(marker_fragments) + if any(fragments[index:index + width] == marker_fragments for index in range(len(fragments) - width + 1)): + return True + return False diff --git a/app/services/llm/specs.py b/app/services/llm/specs.py index 1a48bec..4bcd326 100644 --- a/app/services/llm/specs.py +++ b/app/services/llm/specs.py @@ -62,6 +62,22 @@ def setting(self, key: str) -> str: native_model_list_api_key_header="x-goog-api-key", required_generation_method="generateContent", model_list_page_limit=20, + excluded_id_markers=( + "tts", + "image", + "banana", + "lyria", + "transcribe", + "robotics", + "computer", + "research", + "antigravity", + "embedding", + "imagen", + "veo", + "live", + "aqa", + ), ), LLMProvider.anthropic: ProviderSpec( LLMProvider.anthropic, diff --git a/docs/multi-llm-support/configuration.md b/docs/multi-llm-support/configuration.md index 8b267b1..0907707 100644 --- a/docs/multi-llm-support/configuration.md +++ b/docs/multi-llm-support/configuration.md @@ -67,17 +67,23 @@ Every LLM setting, as read from the env file. Empty means unset. Gemini models are listed through Gemini's native model-list API. FireForm removes a model only when Gemini provides `supportedGenerationMethods` and it does not include `generateContent`; models without that field are retained. +It also approximately excludes IDs containing a complete hyphen-, dot- or +underscore-separated fragment matching `tts`, `image`, `banana`, `lyria`, +`transcribe`, `robotics`, `computer`, `research`, `antigravity`, `embedding`, +`imagen`, `veo`, `live` or `aqa`. This short ID-based list may exclude a +text-capable model or miss a non-text model as provider names change. The API key is sent in the `x-goog-api-key` header. If native listing returns 404 or non-JSON, FireForm warns and falls back to the compatible model list -without the capability filter. Listing follows `nextPageToken` for at most 20 -pages and warns if that limit is reached. +without capability filtering; the ID-fragment filter still applies. Listing +follows `nextPageToken` for at most 20 pages and warns if that limit is reached. OpenAI only provides model IDs in its list, so FireForm approximately excludes -IDs containing `embedding`, `whisper`, `tts`, `dall-e`, `moderation`, -`transcribe`, `realtime`, `image` or `audio` (case-insensitive). This is a -name-based heuristic and can be wrong as model names change. Other providers, -including Anthropic, custom servers and Ollama, are not filtered by these -rules. A filtered model can still be entered and tested as a manual model. +IDs containing a complete hyphen-, dot- or underscore-separated fragment +matching `embedding`, `whisper`, `tts`, `dall-e`, `moderation`, `transcribe`, +`realtime`, `image` or `audio`. This is a name-based heuristic and can be +wrong as model names change. Other providers, including Anthropic, custom +servers and Ollama, are not filtered by these rules. A filtered model can +still be entered and tested as a manual model. Filtering applies at the provider model-list source. It therefore affects the catalog, startup `check_config()` and candidate configuration validation. A diff --git a/docs/multi-llm-support/troubleshooting.md b/docs/multi-llm-support/troubleshooting.md index 9cd589d..409c502 100644 --- a/docs/multi-llm-support/troubleshooting.md +++ b/docs/multi-llm-support/troubleshooting.md @@ -47,8 +47,8 @@ The log also records when a provider goes down and when it comes back. ## Noisy model list -Gemini is filtered using its `generateContent` capability, and OpenAI uses an -approximate case-insensitive ID-marker filter. Other providers may list +Gemini is filtered using its `generateContent` capability and approximate ID +fragments; OpenAI uses approximate ID fragments only. Other providers may list non-text models; use an explicit `_MODELS` list to limit those. See [Models shown in the catalog](configuration.md#models-shown-in-the-catalog) for the filter behavior and its manual-model escape hatch. diff --git a/tests/test_llm_registry.py b/tests/test_llm_registry.py index ec288da..bcab69d 100644 --- a/tests/test_llm_registry.py +++ b/tests/test_llm_registry.py @@ -274,6 +274,29 @@ def test_manual_model_can_explicitly_restore_filtered_model(self): OPENAI, "text-embedding-3-small" ) + def test_manual_gemini_model_can_restore_excluded_id(self): + model = "gemini-3-pro-image-preview" + settings = { + OLLAMA: _provider(OLLAMA, "*", "qwen"), + GEMINI: ProviderSettings( + spec=SPECS[GEMINI], + base_url="http://local:1", + models=ModelSelection.parse(f"g1,{model}"), + default_model="g1", + manual_models=(model,), + ), + } + registry = _registry( + {OLLAMA: FakeLLM(OLLAMA, ["qwen"]), GEMINI: FakeLLM(GEMINI, ["g1"])}, + settings=settings, + ) + + catalog = registry.provider_catalog(GEMINI) + + assert catalog.offers(model) + assert next(item for item in catalog.models if item.name == model).manual is True + assert registry.resolve(GEMINI, model) == ModelChoice(GEMINI, model) + def test_unreachable_provider_is_503(self, ollama): registry = _registry({OLLAMA: ollama, GEMINI: FakeLLM(GEMINI, down=True)}) with pytest.raises(LLMUnavailableError, match="Could not reach gemini") as info: diff --git a/tests/test_llm_service.py b/tests/test_llm_service.py index 7fa45ab..ad8043a 100644 --- a/tests/test_llm_service.py +++ b/tests/test_llm_service.py @@ -505,6 +505,92 @@ def test_gemini_native_single_page(self, monkeypatch): ) assert [model.name for model in llm.list_models()] == ["one"] + def test_gemini_catalog_excludes_non_text_id_fragments(self, monkeypatch): + llm = self._gemini() + model_ids = [ + "gemini-2.5-flash", + "gemini-2.5-pro", + "gemma-4-26b-a4b-it", + "gemma-4-31b-it", + "gemini-flash-latest", + "gemini-flash-lite-latest", + "gemini-pro-latest", + "gemini-2.5-flash-lite", + "gemini-3-flash-preview", + "gemini-3.1-pro-preview", + "gemini-3.1-pro-preview-customtools", + "gemini-3.1-flash-lite-preview", + "gemini-3.1-flash-lite", + "gemini-3.5-flash", + "gemini-3.5-flash-lite", + "gemini-omni-flash-preview", + "gemini-omni-1.1-flash", + "gemini-3.6-flash", + "gemini-3.7-flash", + "gemini-3.8-flash", + "gemini-4-argon", + "gemini-2.5-flash-preview-tts", + "gemini-2.5-pro-preview-tts", + "gemini-2.5-flash-image", + "gemini-3-pro-image-preview", + "gemini-3-pro-image", + "nano-banana-pro-preview", + "gemini-3.1-flash-image-preview", + "gemini-3.1-flash-image", + "gemini-3.1-flash-lite-image", + "gemini-nano-banana-2.1", + "gemini-3.5-transcribe", + "lyria-3-clip-preview", + "lyria-3-pro-preview", + "lyria-3.5", + "gemini-3.1-flash-tts-preview", + "gemini-3.8-flash-tts", + "gemini-3.8-flash-lite-tts", + "gemini-robotics-er-2-preview", + "gemini-2.5-computer-use-preview-10-2025", + "antigravity-preview-05-2026", + "antigravity-preview-09-2026", + "antigravity-preview-latest", + "deep-research-max-preview-04-2026", + "deep-research-preview-04-2026", + "deep-research-pro-preview-12-2025", + ] + response = self._response(200, { + "models": [ + { + "name": f"models/{model_id}", + "supportedGenerationMethods": ["generateContent"], + } + for model_id in model_ids + ], + }) + monkeypatch.setattr( + "app.services.llm.openai_compatible.requests.get", + lambda *_args, **_kwargs: response, + ) + + assert [model.name for model in llm.list_models()] == model_ids[:21] + + def test_id_markers_match_only_complete_fragments(self, monkeypatch): + llm = self._gemini() + ids = [ + "gemini-3-flash-live-preview", + "gemini-imaginary-flash", + "gemini-flash-imageish", + "gemini-image.flash", + "gemini_tts_preview", + ] + monkeypatch.setattr( + "app.services.llm.openai_compatible.requests.get", + lambda *_args, **_kwargs: self._response(200, { + "models": [{"name": f"models/{model_id}"} for model_id in ids], + }), + ) + + assert [model.name for model in llm.list_models()] == [ + "gemini-imaginary-flash", "gemini-flash-imageish", + ] + def test_gemini_native_listing_stops_at_page_limit(self, monkeypatch, caplog): llm = self._gemini() calls = [] @@ -582,6 +668,7 @@ def test_gemini_native_falls_back_to_compatible_list(self, monkeypatch, caplog, llm.client = MagicMock() llm.client.with_options.return_value.models.list.return_value = [ SimpleNamespace(id="models/legacy-model"), + SimpleNamespace(id="models/gemini-image"), ] monkeypatch.setattr( "app.services.llm.openai_compatible.requests.get", @@ -595,7 +682,7 @@ def test_openai_excludes_non_text_model_id_markers_only_for_openai(self): ids = [ "text-embedding-3-small", "whisper-1", "tts-1", "dall-e-3", "moderation-latest", "gpt-transcribe", "gpt-realtime", "gpt-image", - "gpt-audio", "gpt-4o-mini", + "gpt-audio", "gpt-4o-mini", "embeddingish-encoder", "dalle-3", "gpt-imageish", ] openai_llm = OpenAICompatibleLLM( LLMProvider.openai, "https://api.openai.com/v1", "key", "gpt-4o-mini", 60 @@ -609,7 +696,9 @@ def test_openai_excludes_non_text_model_id_markers_only_for_openai(self): SimpleNamespace(id=model_id) for model_id in ids ] - assert [model.name for model in openai_llm.list_models()] == ["gpt-4o-mini"] + assert [model.name for model in openai_llm.list_models()] == [ + "gpt-4o-mini", "embeddingish-encoder", "dalle-3", "gpt-imageish", + ] assert [model.name for model in custom_llm.list_models()] == ids def test_structured_output_sends_schema(self): From 5a16f669c58bae3fe113c8a8a42e60f9f5a436fa Mon Sep 17 00:00:00 2001 From: Jan <1673276@uab.cat> Date: Mon, 26 Oct 2026 18:47:58 +0100 Subject: [PATCH 13/14] test: isolate registry cache between tests --- tests/conftest.py | 8 ++++++-- tests/test_jobs.py | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 7c94ed2..e63ce18 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -25,7 +25,7 @@ Report, Template, ) -from app.services.llm import LLMRegistry, LLMSettings, ProviderSettings +from app.services.llm import LLMRegistry, LLMSettings, ProviderSettings, user_config from app.services.llm.ollama import OllamaLLM from app.services.llm.selection import ModelSelection from app.services.llm.specs import SPECS @@ -75,8 +75,12 @@ def test_registry(monkeypatch): default_provider=LLMProvider.ollama, ) registry = LLMRegistry(settings, {LLMProvider.ollama: OllamaLLM("http://ollama:11434", "qwen2.5:1.5b", 5)}) + user_config.invalidate_registry_cache() monkeypatch.setattr("app.services.llm.get_registry", lambda: registry) - return registry + try: + yield registry + finally: + user_config.invalidate_registry_cache() @pytest.fixture diff --git a/tests/test_jobs.py b/tests/test_jobs.py index b221414..6192b39 100644 --- a/tests/test_jobs.py +++ b/tests/test_jobs.py @@ -28,7 +28,7 @@ def _seed_template( "fields": {"name": "string"}, }, ) - assert resp.status_code == 200, resp.json() + assert resp.status_code == 200 return resp.json()["id"] def _seed_input( @@ -167,7 +167,7 @@ def test_submit_with_model_override(self, mock_task, client, db, test_llm, monke "input_id": str(input_id), "model": "mistral:latest", }) - assert resp.status_code == 200 + assert resp.status_code == 200, resp.json() mock_task.apply_async.assert_called_once_with( (tpl_id, "test", str(input_id), "mistral:latest", "ollama"), task_id=ANY ) From fcc22b344399b49d84b5c7e2215bdffb5d041d16 Mon Sep 17 00:00:00 2001 From: Jan <1673276@uab.cat> Date: Tue, 27 Oct 2026 11:04:18 +0100 Subject: [PATCH 14/14] fix: resolve Alembic revision conflict after development merge --- ...configuration.py => 007_add_llm_user_configuration.py} | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) rename alembic/versions/{006_add_llm_user_configuration.py => 007_add_llm_user_configuration.py} (95%) diff --git a/alembic/versions/006_add_llm_user_configuration.py b/alembic/versions/007_add_llm_user_configuration.py similarity index 95% rename from alembic/versions/006_add_llm_user_configuration.py rename to alembic/versions/007_add_llm_user_configuration.py index d3c459a..2f6e394 100644 --- a/alembic/versions/006_add_llm_user_configuration.py +++ b/alembic/versions/007_add_llm_user_configuration.py @@ -1,7 +1,7 @@ """add local LLM provider configuration -Revision ID: 006 -Revises: 005 +Revision ID: 007 +Revises: 006 """ from typing import Sequence @@ -9,8 +9,8 @@ from alembic import op -revision: str = "006" -down_revision: str | None = "005" +revision: str = "007" +down_revision: str | None = "006" branch_labels: str | Sequence[str] | None = None depends_on: str | Sequence[str] | None = None