AI: persist only the pinned field, not the whole config block
CI / compile (pull_request) Successful in 7s
CI / unit (pull_request) Successful in 23s
CI / integration (pull_request) Failing after 24s

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:
2026-08-24 14:07:24 +02:00
parent 26dae1d101
commit bf7c3d9093
3 changed files with 122 additions and 14 deletions
+105 -6
View File
@@ -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