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