From 26dae1d1010c7487bbf9fe326a26ef18855f57a8 Mon Sep 17 00:00:00 2001 From: Polish Hammer Date: Fri, 21 Aug 2026 13:35:03 +0200 Subject: [PATCH 1/3] AI: add a self-hosted Ollama backend, and let the picker choose the model The bot could talk to OpenAI or Anthropic; this adds Ollama as a third provider so it can run against models hosted on our own box, and extends the switch command to pick WHICH model - not just which backend. Provider: Ollama exposes an OpenAI-compatible /v1 surface, so the client is just openai.AsyncOpenAI(base_url=OLLAMA_URL + "/v1"). That reuses the existing message format and the whole _map_openai_error mapping instead of forking a second error taxonomy. There is no API key - the endpoint IS the configuration, so the backend stays dormant (and refuses to be selected, with a message naming the variable) until CONJURER_OLLAMA_URL is set, the same way the Conan bridge behaves. Model selection: * list_provider_models() asks the SERVER for Ollama (/v1/models), so the picker shows what is actually pulled on the box rather than a hardcoded list. Hosted providers just report what they are wired to. * set_active_model() pins the config's latest_model and persists it; cheap_model is left alone so the MUSIC path keeps its cheaper backend. * $gadaj_teraz now takes " [model]", and a new read-only $modele_ai lists what is available. Pinning an id Ollama does not have is rejected up front with the real list - otherwise the typo only surfaces later as a failed reply. Two fixes this exposed: * AI_CONFIGS now merges built-in defaults with the settings-file block instead of letting the file win outright. Every provider switch persists a "configs" block, so a file written by an older build would have permanently hidden ollama from the picker after an upgrade. * _persist_active_ai_config assigns "configs" instead of setdefault, so a pinned model actually survives a restart. * the hardcoded 120s response timeout is now CONJURER_AI_TIMEOUT_SECONDS - a self-hosted model on a modest GPU can legitimately need longer. Tests cover: ollama appears in the picker, select_model maps the legacy gpt-4o default instead of leaking it, model listing (server-queried, sorted, de-duplicated, failure -> AIError, unconfigured -> auth), pinning (latest only, blank/unknown rejected), and that provider_generate routes to the new path. Suite: 68 unit + 70 integration green. Co-Authored-By: Claude Opus 4.8 --- ai_commands.py | 93 +++++++++++++-- ai_functions.py | 76 +++++++++++- constants.py | 32 +++++- docker/env/bot.env.example | 20 +++- tests/unit/test_ai_provider_switch.py | 159 ++++++++++++++++++++++++++ 5 files changed, 366 insertions(+), 14 deletions(-) diff --git a/ai_commands.py b/ai_commands.py index 826bfc5..fbec71a 100644 --- a/ai_commands.py +++ b/ai_commands.py @@ -174,19 +174,57 @@ class Events(commands.Cog): await ctx.reply("Nope. Nie wiesz jak użyć") @commands.hybrid_command( - name="gadaj_teraz", - description="Pokaż/przełącz backend AI (bez argumentu = status). Przełączanie: Vykidailo.", + name="modele_ai", + description="Pokaż modele dostępne dla danego backendu (Ollamę pyta na żywo).", ) - async def gadaj_teraz(self, ctx, nazwa_konfigu: Optional[str] = None): + async def modele_ai(self, ctx, nazwa_konfigu: Optional[str] = None): + """Read-only model listing. For Ollama this queries the server, so it + shows exactly what is pulled on the box right now.""" + async with ctx.channel.typing(): + target = nazwa_konfigu or ai_functions.get_active_ai_config() + if target not in ai_functions.list_ai_configs(): + await discord_friendly_reply( + ctx, + f"Nie znam configu '{target}'. Dostępne: " + f"{', '.join(ai_functions.list_ai_configs())}", + ) + return + try: + models = await ai_functions.list_provider_models(target) + except ai_functions.AIError as exc: + await discord_friendly_reply( + ctx, f"Nie mogę pobrać modeli dla '{target}': {exc}" + ) + return + if not models: + await discord_friendly_reply(ctx, f"Brak modeli dla '{target}'.") + return + await discord_friendly_reply( + ctx, + f"Modele dla **{target}**: {', '.join(models)}\n" + f"Wepniesz przez `$gadaj_teraz {target} ` (tylko Vykidailo).", + ) + + @commands.hybrid_command( + name="gadaj_teraz", + description="Pokaż/przełącz backend AI i model (bez argumentu = status). Przełączanie: Vykidailo.", + ) + async def gadaj_teraz( + self, ctx, nazwa_konfigu: Optional[str] = None, model: Optional[str] = None + ): async with ctx.channel.typing(): available = ai_functions.list_ai_configs() # No argument -> report the active backend (read-only, open to all). if not nazwa_konfigu: active = ai_functions.get_active_ai_config() + active_cfg = ai_functions.AI_CONFIGS.get(active, {}) await discord_friendly_reply( ctx, - f"Teraz gadam przez **{active}**. Dostępne: {', '.join(available)}. " - "Przełączysz przez `$gadaj_teraz ` (tylko Vykidailo).", + f"Teraz gadam przez **{active}** " + f"({active_cfg.get('provider')} / {active_cfg.get('latest_model')}). " + f"Dostępne: {', '.join(available)}. " + "Przełączysz przez `$gadaj_teraz [model]` (tylko Vykidailo), " + "modele zobaczysz przez `$modele_ai`.", ) return is_admin = isinstance(ctx.author, discord.Member) and any( @@ -208,13 +246,50 @@ class Events(commands.Cog): ctx, f"Nie mogę przełączyć na '{nazwa_konfigu}': {exc}" ) return + + # Optional second argument pins the model. For a backend we can + # enumerate (Ollama), reject an unknown id up front with the list - + # otherwise the typo only surfaces later as a failed reply. + if model: + try: + known = await ai_functions.list_provider_models(nazwa_konfigu) + except ai_functions.AIError: + known = [] # cannot enumerate -> accept verbatim + if known and cfg.get("provider") == "ollama" and model not in known: + await discord_friendly_reply( + ctx, + f"Model '{model}' nie jest wgrany na Ollamie. " + f"Dostępne: {', '.join(known)}", + ) + return + try: + cfg = ai_functions.set_active_model(model, nazwa_konfigu) + except (KeyError, ValueError) as exc: + await discord_friendly_reply( + ctx, f"Nie mogę wpiąć modelu '{model}': {exc}" + ) + return + self.logger.info( - "Przełączono AI na config %s (%s)", nazwa_konfigu, cfg.get("provider") + "Przełączono AI na config %s (%s / %s)", + nazwa_konfigu, cfg.get("provider"), cfg.get("latest_model"), ) - await discord_friendly_reply( - ctx, - f"Teraz gadam przez **{nazwa_konfigu}** — {cfg.get('provider')} / {cfg.get('latest_model')}.", + message = ( + f"Teraz gadam przez **{nazwa_konfigu}** — " + f"{cfg.get('provider')} / {cfg.get('latest_model')}." ) + # Switched without pinning a model: show what else is on offer. + if not model: + try: + others = await ai_functions.list_provider_models(nazwa_konfigu) + except ai_functions.AIError: + others = [] + if len(others) > 1: + message += ( + f"\nDostępne modele: {', '.join(others)} " + f"(`$gadaj_teraz {nazwa_konfigu} `)." + ) + await discord_friendly_reply(ctx, message) @commands.hybrid_command( name="armia_hammera", diff --git a/ai_functions.py b/ai_functions.py index c44c1c1..d1ab08d 100644 --- a/ai_functions.py +++ b/ai_functions.py @@ -9,6 +9,7 @@ import time from other_functions import discord_friendly_send from constants import ( AI_CONFIGS, + AI_TIMEOUT_SECONDS, ASSISTANTS, CLAUDECLIENT, CYCLIC_WORDS, @@ -19,6 +20,7 @@ from constants import ( MEMORY_FIVE_SIARA, MESSAGE_TABLE, MESSAGE_TABLE_MUZYKA, + OLLAMACLIENT, OPENAICLIENT, SYSTEM_GPT_SETTINGS, WORD_REACTIONS, @@ -92,11 +94,53 @@ def set_active_ai_config(name: str) -> dict: raise RuntimeError("klient Anthropic nie jest skonfigurowany (brak ANTHROPIC_API_KEY)") if provider == "openai" and OPENAICLIENT is None: raise RuntimeError("klient OpenAI nie jest skonfigurowany (brak OPENAI_API_KEY)") + if provider == "ollama" and OLLAMACLIENT is None: + raise RuntimeError("Ollama nie jest skonfigurowana (ustaw CONJURER_OLLAMA_URL)") _ACTIVE_CONFIG_NAME = name _persist_active_ai_config(name) return cfg +async def list_provider_models(name: str = None): + """Model ids selectable for a config. + + For Ollama this ASKS THE SERVER (its OpenAI-compatible /v1/models), so the + picker always reflects what is actually pulled on the box rather than a + hardcoded list. Hosted providers are not enumerated - we only report what + the config is wired to. + """ + cfg = AI_CONFIGS.get(name or _ACTIVE_CONFIG_NAME) or _active_config() + if cfg.get("provider") == "ollama": + if OLLAMACLIENT is None: + raise AIError( + "auth", + RuntimeError("Ollama nie jest skonfigurowana (ustaw CONJURER_OLLAMA_URL)"), + ) + try: + resp = await OLLAMACLIENT.models.list() + except Exception as exc: # pylint: disable=broad-except + raise _map_openai_error(exc) + return sorted({item.id for item in resp.data}) + return [m for m in (cfg.get("latest_model"), cfg.get("cheap_model")) if m] + + +def set_active_model(model: str, name: str = None) -> dict: + """Pin the model a config uses for normal replies, and persist it. + + Only ``latest_model`` is changed; ``cheap_model`` stays as configured so the + MUSIC path keeps its cheaper backend. + """ + cfg_name = name or _ACTIVE_CONFIG_NAME + if cfg_name not in AI_CONFIGS: + raise KeyError(cfg_name) + if not model or not model.strip(): + raise ValueError("pusta nazwa modelu") + cfg = AI_CONFIGS[cfg_name] + cfg["latest_model"] = model.strip() + _persist_active_ai_config(_ACTIVE_CONFIG_NAME) + return cfg + + def _persist_active_ai_config(name: str) -> None: """Best-effort write of the active-config choice into system_gpt_settings.json. @@ -117,7 +161,10 @@ def _persist_active_ai_config(name: str) -> None: return if len(data) > 2 and isinstance(data[2], dict): data[2]["active"] = name - data[2].setdefault("configs", AI_CONFIGS) + # Assign (not setdefault): AI_CONFIGS is the in-memory truth and may + # carry a model pinned via set_active_model, which setdefault would + # silently drop on restart. + data[2]["configs"] = AI_CONFIGS else: data = data[:2] + [{"active": name, "configs": AI_CONFIGS}] try: @@ -214,12 +261,37 @@ async def _anthropic_call(messages, model, cfg): return text.strip() +async def _ollama_call(messages, model, cfg): + """Self-hosted counterpart of openai_call. Returns a plain string. + + Ollama exposes an OpenAI-compatible /v1 surface, so the same message format + and the same error mapping apply - only the base_url and the model ids + differ. Chat Completions (not the Responses API) is what Ollama implements. + """ + if OLLAMACLIENT is None: + raise AIError( + "auth", + RuntimeError("Ollama nie jest skonfigurowana (ustaw CONJURER_OLLAMA_URL)"), + ) + try: + resp = await OLLAMACLIENT.chat.completions.create( + model=model, + messages=messages, + temperature=float(cfg.get("temperature", 0.2)), + ) + except Exception as exc: # pylint: disable=broad-except + raise _map_openai_error(exc) + return (resp.choices[0].message.content or "").strip() + + async def provider_generate(messages, model, temperature=0.2): """Dispatch a chat completion to the active backend, normalising errors.""" cfg = _active_config() try: if cfg.get("provider") == "anthropic": return await _anthropic_call(messages, model, cfg) + if cfg.get("provider") == "ollama": + return await _ollama_call(messages, model, cfg) return await openai_call(messages, model, temperature) except AIError: raise @@ -459,7 +531,7 @@ async def handle_response( try: # ...przygotowanie messages/system prompt/itp. jak masz... # retry/backoff + deadline (zachowuje Twoją semantykę logowania) - timeout_sec = 120 + timeout_sec = AI_TIMEOUT_SECONDS deadline = time.time() + timeout_sec response = await asyncio.wait_for( provider_generate(messages=history_msgs, model=model_to_use), diff --git a/constants.py b/constants.py index 7c135f4..4741d43 100644 --- a/constants.py +++ b/constants.py @@ -400,6 +400,25 @@ if anthropic and ANTHROPIC_API_KEY: else: CLAUDECLIENT = None +# Ollama (self-hosted models). There is no API key - the endpoint IS the whole +# configuration, so the feature stays dormant until CONJURER_OLLAMA_URL is set +# (same pattern as the Conan bridge). We talk to Ollama's OpenAI-COMPATIBLE +# surface (/v1) with the openai SDK we already depend on, which means the +# existing message format and _map_openai_error handling work unchanged. +# How long handle_response waits for ANY backend before giving up. 120s was +# hardcoded and is fine for hosted APIs, but a self-hosted model on a modest GPU +# can legitimately take longer, so it is now tunable. +AI_TIMEOUT_SECONDS = int(os.getenv("CONJURER_AI_TIMEOUT_SECONDS", "120")) + +OLLAMA_URL = os.getenv("CONJURER_OLLAMA_URL", "").rstrip("/") +OLLAMA_LATEST_MODEL = os.getenv("CONJURER_OLLAMA_MODEL", "llama3.1:8b") +OLLAMA_CHEAP_MODEL = os.getenv("CONJURER_OLLAMA_CHEAP_MODEL", OLLAMA_LATEST_MODEL) +if openai and OLLAMA_URL: + # api_key is required by the SDK but ignored by Ollama. + OLLAMACLIENT = openai.AsyncOpenAI(base_url=f"{OLLAMA_URL}/v1", api_key="ollama") +else: + OLLAMACLIENT = None + TOKEN = _resolve_token("discord", "DISCORD_TOKEN") # Voice recognition (AssemblyAI). None = the voice cog reports and disables. @@ -489,6 +508,12 @@ def _default_ai_configs(): # sent for Claude (Opus 4.8 / Sonnet 5 reject sampling params). "max_tokens": 2048, }, + "ollama": { + "provider": "ollama", + "latest_model": OLLAMA_LATEST_MODEL, + "cheap_model": OLLAMA_CHEAP_MODEL, + "temperature": 0.2, + }, # Template for wiring further providers. Copy it, rename the key, point # "provider" at a backend ai_functions.provider_generate implements, and # fill in the model ids. Keys starting with "_" are treated as inert @@ -508,7 +533,12 @@ _ai_block = ( if isinstance(GPT_SETTINGS, list) and len(GPT_SETTINGS) > 2 and isinstance(GPT_SETTINGS[2], dict) else {} ) -AI_CONFIGS = _ai_block.get("configs") or _default_ai_configs() +# Built-in defaults FIRST, then whatever the settings file defines on top. The +# file cannot simply win outright: every provider switch persists a "configs" +# block, so a file written by an older build would permanently hide providers +# added later (ollama) from the picker. +AI_CONFIGS = _default_ai_configs() +AI_CONFIGS.update(_ai_block.get("configs") or {}) # Single switch: env var wins, then the settings-file "active" key, then "gpt". DEFAULT_AI_CONFIG = ( os.getenv("CONJURER_AI_CONFIG") diff --git a/docker/env/bot.env.example b/docker/env/bot.env.example index cacae3d..24d2efe 100644 --- a/docker/env/bot.env.example +++ b/docker/env/bot.env.example @@ -13,10 +13,26 @@ CONJURER_NETRC_FILE=/secrets/.netrc # --- AI backend switch -------------------------------------------------- # Which AI config from system_gpt_settings.json is active at startup -# (e.g. "gpt" or "claude"). Runtime switch: $gadaj_teraz . Unset = -# whatever the settings file's "active" key says, falling back to "gpt". +# (e.g. "gpt", "claude" or "ollama"). Runtime switch: +# $gadaj_teraz [model]. Unset = whatever the settings file's "active" +# key says, falling back to "gpt". # CONJURER_AI_CONFIG=gpt +# --- Ollama (self-hosted models) ---------------------------------------- +# The endpoint IS the whole configuration - no API key. Leave unset and the +# "ollama" backend simply refuses to be selected. In-cluster, use the Service +# DNS name; from outside, host:port. Port 11434 is Ollama's default. +# CONJURER_OLLAMA_URL=http://ollama.ollama.svc.cluster.local:11434 +# CONJURER_OLLAMA_URL= +# Model used for normal replies. $modele_ai lists what the server actually has +# pulled, and $gadaj_teraz ollama pins one at runtime (persisted). +# CONJURER_OLLAMA_MODEL=llama3.1:8b +# Model used for the cheaper MUSIC path; defaults to CONJURER_OLLAMA_MODEL. +# CONJURER_OLLAMA_CHEAP_MODEL= +# How long to wait for ANY backend to answer. 120s suits hosted APIs; a +# self-hosted model on a modest GPU may need more. +# CONJURER_AI_TIMEOUT_SECONDS=120 + # --- Data --------------------------------------------------------------- # Single mounted volume; all writable state is rooted here. CONJURER_DATA_DIR=/data diff --git a/tests/unit/test_ai_provider_switch.py b/tests/unit/test_ai_provider_switch.py index ff3d27f..aead31e 100644 --- a/tests/unit/test_ai_provider_switch.py +++ b/tests/unit/test_ai_provider_switch.py @@ -136,3 +136,162 @@ def test_map_openai_error_categories(): assert ai_functions._map_openai_error(_bare(openai.RateLimitError)).category == "rate_limit" assert ai_functions._map_openai_error(_bare(openai.APITimeoutError)).category == "timeout" assert ai_functions._map_openai_error(ValueError("x")).category == "api" + + +# ---------------------------------------------------------------- Ollama +# The self-hosted backend is wired through Ollama's OpenAI-compatible surface, +# so it reuses the message format and the error mapping above. What is new and +# worth pinning: it must appear in the picker even on an upgraded settings file, +# models come from the SERVER, and pinning one must stick. +import asyncio # noqa: E402 + + +class _FakeModel: + def __init__(self, ident): + self.id = ident + + +class _FakeModels: + def __init__(self, ids, raises=None): + self._ids = ids + self._raises = raises + + async def list(self): + if self._raises: + raise self._raises + return types.SimpleNamespace(data=[_FakeModel(i) for i in self._ids]) + + +class _FakeOllamaClient: + def __init__(self, ids=(), raises=None): + self.models = _FakeModels(list(ids), raises) + + +def test_ollama_config_is_offered_in_the_picker(): + assert "ollama" in ai_functions.list_ai_configs() + cfg = ai_functions.AI_CONFIGS["ollama"] + assert cfg["provider"] == "ollama" + + +def test_select_model_uses_ollama_models_when_active(monkeypatch): + monkeypatch.setitem( + ai_functions.AI_CONFIGS, + "ollama", + {"provider": "ollama", "latest_model": "llama3.1:8b", "cheap_model": "qwen2.5:3b"}, + ) + _reset_active("ollama") + try: + # the legacy gpt-4o default must auto-map, not leak to Ollama + assert ai_functions.select_model("GENERAL", "gpt-4o") == "llama3.1:8b" + assert ai_functions.select_model("MUSIC", "gpt-4o") == "qwen2.5:3b" + # an explicit id is still honoured verbatim + assert ai_functions.select_model("GENERAL", "mistral:7b") == "mistral:7b" + finally: + _reset_active("gpt") + + +def test_list_provider_models_queries_the_ollama_server(monkeypatch): + monkeypatch.setattr( + ai_functions, "OLLAMACLIENT", _FakeOllamaClient(["b:2", "a:1", "a:1"]) + ) + models = asyncio.run(ai_functions.list_provider_models("ollama")) + assert models == ["a:1", "b:2"] # sorted + de-duplicated + + +def test_list_provider_models_for_hosted_provider_reports_configured_ids(): + models = asyncio.run(ai_functions.list_provider_models("gpt")) + assert models == [ + ai_functions.AI_CONFIGS["gpt"]["latest_model"], + ai_functions.AI_CONFIGS["gpt"]["cheap_model"], + ] + + +def test_list_provider_models_wraps_server_failure(monkeypatch): + monkeypatch.setattr( + ai_functions, "OLLAMACLIENT", _FakeOllamaClient(raises=ValueError("boom")) + ) + try: + asyncio.run(ai_functions.list_provider_models("ollama")) + except ai_functions.AIError as exc: + assert exc.category == "api" + else: + raise AssertionError("a server failure must surface as AIError") + + +def test_list_provider_models_without_endpoint_is_an_auth_error(monkeypatch): + monkeypatch.setattr(ai_functions, "OLLAMACLIENT", None) + try: + asyncio.run(ai_functions.list_provider_models("ollama")) + except ai_functions.AIError as exc: + assert exc.category == "auth" + else: + raise AssertionError("an unconfigured Ollama must surface as AIError") + + +def test_set_active_model_pins_latest_and_keeps_cheap(monkeypatch): + monkeypatch.setitem( + ai_functions.AI_CONFIGS, + "ollama", + {"provider": "ollama", "latest_model": "old:1", "cheap_model": "cheap:1"}, + ) + written = {} + monkeypatch.setattr( + ai_functions, "_persist_active_ai_config", lambda name: written.update(name=name) + ) + cfg = ai_functions.set_active_model("mistral:7b", "ollama") + assert cfg["latest_model"] == "mistral:7b" + assert cfg["cheap_model"] == "cheap:1" # MUSIC path untouched + assert written # the choice was persisted + + +def test_set_active_model_rejects_blank_and_unknown_config(monkeypatch): + monkeypatch.setattr(ai_functions, "_persist_active_ai_config", lambda _n: None) + for bad in ("", " "): + try: + ai_functions.set_active_model(bad, "gpt") + except ValueError: + pass + else: + raise AssertionError("a blank model id must be rejected") + try: + ai_functions.set_active_model("x", "nie-ma-takiego") + except KeyError: + pass + else: + raise AssertionError("an unknown config must be rejected") + + +def test_switching_to_ollama_without_endpoint_explains_itself(monkeypatch): + monkeypatch.setattr(ai_functions, "OLLAMACLIENT", None) + try: + ai_functions.set_active_ai_config("ollama") + except RuntimeError as exc: + assert "CONJURER_OLLAMA_URL" in str(exc) + else: + raise AssertionError("switching to an unconfigured Ollama must raise") + finally: + _reset_active("gpt") + + +def test_provider_generate_routes_to_ollama(monkeypatch): + monkeypatch.setitem( + ai_functions.AI_CONFIGS, + "ollama", + {"provider": "ollama", "latest_model": "m:1", "cheap_model": "m:1"}, + ) + _reset_active("ollama") + seen = {} + + async def _fake_ollama_call(messages, model, cfg): + seen.update(model=model, messages=messages) + return "odpowiedź z domu" + + monkeypatch.setattr(ai_functions, "_ollama_call", _fake_ollama_call) + try: + out = asyncio.run( + ai_functions.provider_generate([{"role": "user", "content": "hej"}], "m:1") + ) + finally: + _reset_active("gpt") + assert out == "odpowiedź z domu" + assert seen["model"] == "m:1" -- 2.52.0 From bf7c3d90930dd8e431815e7e4e1f3fe3620ba49e Mon Sep 17 00:00:00 2001 From: Polish Hammer Date: Mon, 24 Aug 2026 14:07:24 +0200 Subject: [PATCH 2/3] AI: persist only the pinned field, not the whole config block Adversarial review of the previous commit found a real regression it introduced, reproduced against the actual code rather than inferred. Changing _persist_active_ai_config from setdefault("configs", ...) to a direct assignment made every backend switch write the whole in-memory AI_CONFIGS over the settings file. Because AI_CONFIGS is now the built-in defaults merged UNDER the file, that meant: * an operator's hand edits were destroyed - and hand editing is the only way to change cheap_model / temperature / max_tokens, since set_active_model writes latest_model and there is no command for the rest, * a config deliberately deleted from the file was re-seeded from the defaults and written back, permanently, * pinning a model for one provider silently reverted another provider's entry, * CONJURER_OLLAMA_MODEL stopped having any effect once the env-derived block had been persisted once. The original motivation was still valid (plain setdefault would drop a pinned model), so the fix is narrower rather than a revert: persist ONLY the field this process actually changed. _persist_active_ai_config takes model_for and writes back just that config's latest_model; everything else in the on-disk block is left exactly as found. The constants.py merge stays - it is what keeps a newly added provider visible after an upgrade - and is now in-memory only, so it cannot reach the file. Tests: the disk-write path had ZERO coverage, which is precisely how this got in. Added four tests that drive the real _persist_active_ai_config against a temp settings file: the pin lands while operator edits survive and a deleted config is not resurrected; a plain switch leaves the configs block byte-identical; a pin survives a re-read; a corrupt file does not raise. Verified they have teeth - reintroducing the regression fails two of them. Also hardened two weak tests the review caught: the pin test asserted on the object set_active_model returns, which IS the mutated dict (so it passed regardless), and the unconfigured-endpoint test monkeypatched OLLAMACLIENT to None when it was already None, passing vacuously. Suite: 72 unit + 70 integration green. Co-Authored-By: Claude Opus 4.8 --- ai_functions.py | 18 +++-- docker/env/bot.env.example | 7 +- tests/unit/test_ai_provider_switch.py | 111 ++++++++++++++++++++++++-- 3 files changed, 122 insertions(+), 14 deletions(-) diff --git a/ai_functions.py b/ai_functions.py index d1ab08d..b629c36 100644 --- a/ai_functions.py +++ b/ai_functions.py @@ -137,11 +137,11 @@ def set_active_model(model: str, name: str = None) -> dict: raise ValueError("pusta nazwa modelu") cfg = AI_CONFIGS[cfg_name] cfg["latest_model"] = model.strip() - _persist_active_ai_config(_ACTIVE_CONFIG_NAME) + _persist_active_ai_config(_ACTIVE_CONFIG_NAME, model_for=cfg_name) return cfg -def _persist_active_ai_config(name: str) -> None: +def _persist_active_ai_config(name: str, model_for: str = None) -> None: """Best-effort write of the active-config choice into system_gpt_settings.json. Keeps the historical two-element structure intact: updates index 2 if it @@ -161,10 +161,16 @@ def _persist_active_ai_config(name: str) -> None: return if len(data) > 2 and isinstance(data[2], dict): data[2]["active"] = name - # Assign (not setdefault): AI_CONFIGS is the in-memory truth and may - # carry a model pinned via set_active_model, which setdefault would - # silently drop on restart. - data[2]["configs"] = AI_CONFIGS + # Write back ONLY what this process actually changed. Assigning the whole + # in-memory AI_CONFIGS here would clobber operator hand-edits (the only + # way to change cheap_model/temperature/max_tokens) and re-seed configs + # deliberately deleted from the file, because AI_CONFIGS is the built-in + # defaults merged under the file. setdefault alone is not enough either: + # it would drop a model pinned via set_active_model, hence model_for. + block = data[2].setdefault("configs", AI_CONFIGS) + if model_for and model_for in AI_CONFIGS: + entry = block.setdefault(model_for, dict(AI_CONFIGS[model_for])) + entry["latest_model"] = AI_CONFIGS[model_for]["latest_model"] else: data = data[:2] + [{"active": name, "configs": AI_CONFIGS}] try: diff --git a/docker/env/bot.env.example b/docker/env/bot.env.example index 24d2efe..68808a0 100644 --- a/docker/env/bot.env.example +++ b/docker/env/bot.env.example @@ -24,8 +24,11 @@ CONJURER_NETRC_FILE=/secrets/.netrc # DNS name; from outside, host:port. Port 11434 is Ollama's default. # CONJURER_OLLAMA_URL=http://ollama.ollama.svc.cluster.local:11434 # CONJURER_OLLAMA_URL= -# Model used for normal replies. $modele_ai lists what the server actually has -# pulled, and $gadaj_teraz ollama pins one at runtime (persisted). +# DEFAULT model for normal replies. $modele_ai lists what the server actually +# has pulled. Pinning one at runtime with `$gadaj_teraz ollama ` is +# persisted into system_gpt_settings.json and from then on WINS over this +# variable - the pin is the more recent, more explicit choice. Clear the +# "latest_model" of the ollama entry in that file to fall back to this default. # CONJURER_OLLAMA_MODEL=llama3.1:8b # Model used for the cheaper MUSIC path; defaults to CONJURER_OLLAMA_MODEL. # CONJURER_OLLAMA_CHEAP_MODEL= diff --git a/tests/unit/test_ai_provider_switch.py b/tests/unit/test_ai_provider_switch.py index aead31e..0d30069 100644 --- a/tests/unit/test_ai_provider_switch.py +++ b/tests/unit/test_ai_provider_switch.py @@ -219,6 +219,11 @@ def test_list_provider_models_wraps_server_failure(monkeypatch): def test_list_provider_models_without_endpoint_is_an_auth_error(monkeypatch): + # Contrast, so the assertion cannot pass vacuously: with a client present the + # call succeeds, and ONLY setting it to None turns it into an auth error. + monkeypatch.setattr(ai_functions, "OLLAMACLIENT", _FakeOllamaClient(["a:1"])) + assert asyncio.run(ai_functions.list_provider_models("ollama")) == ["a:1"] + monkeypatch.setattr(ai_functions, "OLLAMACLIENT", None) try: asyncio.run(ai_functions.list_provider_models("ollama")) @@ -236,16 +241,25 @@ def test_set_active_model_pins_latest_and_keeps_cheap(monkeypatch): ) written = {} monkeypatch.setattr( - ai_functions, "_persist_active_ai_config", lambda name: written.update(name=name) + ai_functions, + "_persist_active_ai_config", + lambda name, model_for=None: written.update(name=name, model_for=model_for), + ) - cfg = ai_functions.set_active_model("mistral:7b", "ollama") - assert cfg["latest_model"] == "mistral:7b" - assert cfg["cheap_model"] == "cheap:1" # MUSIC path untouched - assert written # the choice was persisted + ai_functions.set_active_model("mistral:7b", "ollama") + # Assert on the shared registry, not on the returned object - that object IS + # the mutated dict, so asserting on it would pass even if nothing was stored. + stored = ai_functions.AI_CONFIGS["ollama"] + assert stored["latest_model"] == "mistral:7b" + assert stored["cheap_model"] == "cheap:1" # MUSIC path untouched + assert written["name"] # the choice was persisted... + assert written["model_for"] == "ollama" # ...scoped to the config we changed def test_set_active_model_rejects_blank_and_unknown_config(monkeypatch): - monkeypatch.setattr(ai_functions, "_persist_active_ai_config", lambda _n: None) + monkeypatch.setattr( + ai_functions, "_persist_active_ai_config", lambda _n, model_for=None: None + ) for bad in ("", " "): try: ai_functions.set_active_model(bad, "gpt") @@ -295,3 +309,88 @@ def test_provider_generate_routes_to_ollama(monkeypatch): _reset_active("gpt") assert out == "odpowiedź z domu" assert seen["model"] == "m:1" + + +# ------------------------------------------------- persistence (real disk path) +# This path had NO coverage, which is exactly how a config-clobbering regression +# got in: persisting the whole in-memory AI_CONFIGS (built-in defaults merged +# under the file) overwrote operator hand-edits and resurrected deleted configs. +import json # noqa: E402 + + +def _settings_file(tmp_path, configs, active="gpt"): + path = tmp_path / "system_gpt_settings.json" + path.write_text( + json.dumps( + [ + {"role": "system", "content": "sys"}, + {"someuser": [1, "a", "b", "c", "asst_x"]}, + {"active": active, "configs": configs}, + ] + ), + encoding="utf-8", + ) + return path + + +def test_persist_writes_the_pin_without_clobbering_operator_edits(tmp_path, monkeypatch): + # The file is authoritative for everything the bot does not itself change: + # a hand-tuned cheap_model, and a config deliberately deleted from it. + settings = _settings_file( + tmp_path, + {"gpt": {"provider": "openai", "latest_model": "gpt-4.1", "cheap_model": "hand-tuned"}}, + ) + monkeypatch.setattr(ai_functions, "SYSTEM_GPT_SETTINGS", str(settings)) + monkeypatch.setitem( + ai_functions.AI_CONFIGS, + "ollama", + {"provider": "ollama", "latest_model": "mistral:7b", "cheap_model": "c:1"}, + ) + + ai_functions._persist_active_ai_config("ollama", model_for="ollama") + + data = json.loads(settings.read_text(encoding="utf-8")) + configs = data[2]["configs"] + assert data[2]["active"] == "ollama" + assert configs["ollama"]["latest_model"] == "mistral:7b" # the pin landed + assert configs["gpt"]["latest_model"] == "gpt-4.1" # edit survived + assert configs["gpt"]["cheap_model"] == "hand-tuned" # edit survived + assert "claude" not in configs # a deleted config is NOT resurrected + assert data[0]["content"] == "sys" and "someuser" in data[1] # rest intact + + +def test_plain_switch_leaves_the_configs_block_untouched(tmp_path, monkeypatch): + original = {"gpt": {"provider": "openai", "latest_model": "gpt-4.1", "cheap_model": "hand-tuned"}} + settings = _settings_file(tmp_path, original, active="claude") + monkeypatch.setattr(ai_functions, "SYSTEM_GPT_SETTINGS", str(settings)) + + # Switching backend without pinning a model must only move "active". + ai_functions._persist_active_ai_config("gpt") + + data = json.loads(settings.read_text(encoding="utf-8")) + assert data[2]["active"] == "gpt" + assert data[2]["configs"] == original + + +def test_pinned_model_survives_a_restart(tmp_path, monkeypatch): + # The whole point of persisting: re-reading the file must yield the pin. + settings = _settings_file(tmp_path, {"ollama": {"provider": "ollama", "latest_model": "old:1"}}) + monkeypatch.setattr(ai_functions, "SYSTEM_GPT_SETTINGS", str(settings)) + monkeypatch.setitem( + ai_functions.AI_CONFIGS, + "ollama", + {"provider": "ollama", "latest_model": "new:2", "cheap_model": "c:1"}, + ) + + ai_functions._persist_active_ai_config("ollama", model_for="ollama") + + reread = json.loads(settings.read_text(encoding="utf-8"))[2] + assert reread["configs"]["ollama"]["latest_model"] == "new:2" + + +def test_persist_survives_an_unreadable_settings_file(tmp_path, monkeypatch): + # Best-effort by contract: a broken file must not raise into the command. + broken = tmp_path / "broken.json" + broken.write_text("{ not json", encoding="utf-8") + monkeypatch.setattr(ai_functions, "SYSTEM_GPT_SETTINGS", str(broken)) + ai_functions._persist_active_ai_config("gpt", model_for="gpt") # must not raise -- 2.52.0 From 3279bb923d744ab316456f6a9688f36aa9b0b47e Mon Sep 17 00:00:00 2001 From: Polish Hammer Date: Mon, 24 Aug 2026 14:54:08 +0200 Subject: [PATCH 3/3] AI: warn when the selected Ollama model is not on the server Switching to a backend whose configured model the server does not have succeeded silently, and then every reply failed with "model not found" with nothing explaining why. The switch already fetches the model list to show what else is available, so use it: if the config's model is absent, say so and list what IS there. Found while probing the real server (192.168.1.72): it has exactly one model, gemma4:e2b, so the built-in llama3.1:8b default would have hit this on the first switch. Co-Authored-By: Claude Opus 4.8 --- ai_commands.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/ai_commands.py b/ai_commands.py index fbec71a..50d28ce 100644 --- a/ai_commands.py +++ b/ai_commands.py @@ -284,7 +284,17 @@ class Events(commands.Cog): others = await ai_functions.list_provider_models(nazwa_konfigu) except ai_functions.AIError: others = [] - if len(others) > 1: + current = cfg.get("latest_model") + if others and current not in others: + # The configured/pinned model is not on the server: every + # reply would fail with "model not found" and nothing would + # say why. Flag it here, where the list is already in hand. + message += ( + f"\n⚠ Uwaga: '{current}' nie jest wgrany na serwerze. " + f"Dostępne: {', '.join(others)} " + f"(`$gadaj_teraz {nazwa_konfigu} `)." + ) + elif len(others) > 1: message += ( f"\nDostępne modele: {', '.join(others)} " f"(`$gadaj_teraz {nazwa_konfigu} `)." -- 2.52.0