fix(agent): persist /model selection to .env, TOML, and DB

[ilblackdragon]

Summary

• Root cause: /model wrote selected_model to the DB and config.toml, but the ~/.ironclaw/.env file (e.g. NEARAI_MODEL=old-model) has the highest priority in LlmConfig::resolve_model() and was never updated — so it always shadowed the new model on restart.
• persist_selected_model now updates all three persistence layers: .env (backend-specific model var), config.toml (created if absent), and DB settings.
• Adds diagnostic logging when the DB store is unavailable.

Test plan

• db_single_key_model_update_survives_roundtrip — verifies /model's single-key DB write survives from_db_map() roundtrip
• toml_overlay_preserves_matching_model — TOML overlay doesn't clobber when DB and TOML match
• stale_toml_overwrites_db_model — documents TOML > DB priority (proves why TOML must be kept in sync)
• toml_created_when_missing_for_model_persist — verifies config.toml creation when absent
• Manual: run /model <name>, quit, restart, verify model persists

🤖 Generated with Claude Code

[Copilot]

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

[serrrfirat]

High Severity — .env line detection uses starts_with without = delimiter

The check for whether the env var exists in .env uses:

line.trim_start().starts_with(model_env)

Where model_env is e.g. "NEARAI_MODEL". This matches any line that starts with that prefix, including unrelated vars like NEARAI_MODEL_VERSION=foo.

Meanwhile, upsert_bootstrap_var correctly uses format!("{}=", key) as its prefix match. So the detection and update use different matching strategies — the detection could false-positive, causing the code to update the wrong var or trigger an update when it shouldn't.

Fix:

let prefix = format!("{model_env}=");
content.lines().any(|line| line.trim_start().starts_with(&prefix))

[ilblackdragon]

Fixed in d824875. Now uses format\!("{}=", model_env) prefix and skips #-commented lines, matching the same logic as upsert_bootstrap_var_to.

[serrrfirat]

Medium Severity — concurrent /model commands could corrupt .env or TOML

The .env read-then-conditionally-write and the TOML load-then-write are not atomic. If two /model commands execute concurrently (e.g., from different channels — Telegram + web gateway), they could interleave reads/writes and corrupt either file.

Low likelihood for a single-user assistant, but worth noting since the agent supports multi-channel access. Consider flock-style advisory locking, or at minimum a comment documenting this as a known limitation.

[ilblackdragon]

Acknowledged. This is a single-user assistant and concurrent /model commands are extremely unlikely. The TOML write is a full-file replace (atomic on most filesystems), and upsert_bootstrap_var does a read-modify-write that is also a single fs::write. Adding flock would add complexity disproportionate to the risk here.

[serrrfirat]

Medium Severity — backend re-resolved from disk instead of using in-memory value

The closure re-resolves the LLM backend by reading LLM_BACKEND env var then re-loading config.toml from the default path. But the agent already has the resolved backend in memory (in its Config). This creates two issues:

1. Redundant I/O — ProviderRegistry::load() re-reads providers.json from disk, and Settings::load_toml() re-reads the TOML.
2. Custom config path — if the user specified a non-default config path, this reads the wrong file (or no file), potentially resolving a different backend than the one actually in use.

Suggestion: Capture the already-resolved backend and model_env var before entering spawn_blocking, then move the strings into the closure:

let registry = crate::llm::ProviderRegistry::load();
let backend = self.config().llm.backend.clone(); // or however the agent exposes it
let model_env = registry.model_env_var(&backend).to_string();
let model_owned = model.to_string();
tokio::task::spawn_blocking(move || {
    // use model_env directly, no re-resolution needed
    ...
})

[ilblackdragon]

Fixed in d824875. Backend is now read from AgentDeps.llm_backend — the already-resolved value from startup (env > DB > TOML > default). This eliminates the redundant ProviderRegistry::load() and Settings::load_toml() calls, and correctly handles custom --config paths and DB-only backends.

[serrrfirat]

Medium Severity — newly created config.toml from Default::default() may leak non-default values

When no config.toml exists, this creates one with Settings { selected_model: Some(...), ..Default::default() }. On next startup, merge_from overlays the TOML onto DB settings.

merge_from skips fields that match Default::default(), so this is probably safe. But if any `Settings` field has a non-trivial default (e.g., `gateway_enabled` defaults to `true` via `default_true`), the newly created TOML could unintentionally override DB settings for those fields.

The existing test `toml_created_when_missing_for_model_persist` only verifies `selected_model` round-trips — it doesn't verify that other fields in the created TOML don't interfere with `merge_from`.

Suggestion: Add a test that creates TOML via this code path, then `merge_from`s it onto a Settings with different values, and asserts the non-model fields are unchanged.

[ilblackdragon]

This is safe by construction. merge_non_default compares each field against Default::default() and only copies values that differ. The created TOML has Settings { selected_model: Some("..."), ..Default::default() }, so every field except selected_model matches the default and is skipped during merge. Fields with non-trivial defaults (e.g. default_true()) are equal to Default::default() by definition, so they are also skipped.

[serrrfirat]

Overall Review

Good fix for a real and subtle persistence bug — the root cause analysis (.env env var shadowing TOML and DB) is spot-on, and writing to all three layers is the correct approach.

One real bug: The .env line detection at line 878 uses starts_with(model_env) without an = delimiter, which could false-positive on similarly-prefixed vars. This should be a quick fix.

Three design considerations:

• The backend is re-resolved from disk inside spawn_blocking instead of using the already-resolved in-memory value — minor inefficiency but also a correctness risk with custom config paths.
• Creating a fresh config.toml from Default::default() needs verification that merge_from correctly ignores the default-valued fields on next startup.
• The file read/write sequence isn't atomic, so concurrent /model from different channels could race (low risk in practice).

Tests are good — the regression tests clearly document the priority model (env > TOML > DB) and verify the round-trip behavior. Missing: a test for the .env update path itself.

[zmanian]

Code Review — persist /model selection to .env, TOML, and DB

+164 / -3 across 2 files. Fixes the root cause where .env model var shadowed DB/TOML on restart. Good test coverage of the priority chain.

---

Issues

1. .env line matching is prefix-only (high — already flagged)

line.trim_start().starts_with(model_env) (commands.rs:881) matches prefixed vars — e.g., NEARAI_MODEL_FALLBACK would match when looking for NEARAI_MODEL. Also matches commented-out lines like # NEARAI_MODEL=....

Fix: check for starts_with(&format!("{model_env}=")) or starts_with(&format!("{model_env} =")) and skip lines starting with #.

2. Backend resolved from disk, not in-memory config (medium — already flagged)

The spawn_blocking closure re-resolves the LLM backend by reading LLM_BACKEND env var then re-parsing config.toml (commands.rs:862-870). The Agent already has the resolved backend in its config. Pass it into the closure instead of re-deriving it — avoids the double TOML parse and potential divergence.

3. Default::default() in new TOML may include non-default values (medium)

When no config.toml exists (commands.rs:901-908), the code creates one with Settings { selected_model: Some(model), ..Default::default() }. If Default::default() for Settings ever includes non-empty values for other fields, they'll be persisted to disk as if the user configured them. Consider using a minimal TOML write that only contains selected_model, or use save_toml with a settings struct that only serializes non-None fields.

4. No concurrency protection (low-medium)

Two concurrent /model commands could interleave reads and writes to .env and config.toml. Unlikely in practice (CLI single-user) but upsert_bootstrap_var should ideally use atomic write (write to temp + rename). Documenting this as a known limitation is also fine.

5. Double TOML load in the closure (low)

Settings::load_toml is called twice in the same spawn_blocking closure — once to infer llm_backend (line 866) and once to update selected_model (line 897). Load once, use both.

---

What's good

• Root cause analysis is correct: .env > TOML > DB priority chain meant only updating DB was useless
• Conservative .env update: only writes if the var already exists (line 882), avoiding injecting new vars
• Tests cover the priority chain: stale_toml_overwrites_db_model documents the TOML > DB priority and proves why the fix is necessary
• TOML creation on missing file: solves the fresh-install case where no wizard ran
• Diagnostic logging: warns when DB store is unavailable

Verdict

Approve with required changes. Fix the .env prefix matching (item 1) before merge — it's a real bug that could corrupt unrelated env vars. Items 2 and 5 (double TOML load) are easy wins.

[ilblackdragon]

All three required changes from @zmanian's review are addressed in d824875:

Item 1 (.env prefix matching): Now uses format!("{}=", model_env) and skips #-commented lines — matches upsert_bootstrap_var_to logic exactly.

Item 2 (backend from in-memory config): Added llm_backend: String to AgentDeps, populated from config.llm.backend at startup. The closure captures it directly — no disk re-read.

Item 5 (double TOML load): Eliminated. Backend comes from AgentDeps, TOML is loaded once for the model update.

Item 3 (Default::default() in new TOML): Safe by construction — merge_non_default compares each field against Default::default() and only copies values that differ. Every field except selected_model matches the default and is skipped.

Item 4 (concurrency): Acknowledged as known limitation. Single-user assistant makes this extremely unlikely.