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 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user