-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(agent): persist /model selection to .env, TOML, and DB #1581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -841,12 +841,50 @@ impl Agent { | |
| .await | ||
| { | ||
| tracing::warn!("Failed to persist model to DB: {}", e); | ||
| } else { | ||
| tracing::debug!("Persisted selected_model to DB: {}", model); | ||
| } | ||
| } else { | ||
| tracing::warn!("No database store available — model choice will not persist to DB"); | ||
| } | ||
|
|
||
| // 2. Update TOML config file if it exists (sync I/O in spawn_blocking). | ||
| // 2. Update .env and TOML config file (sync I/O in spawn_blocking). | ||
| let model_owned = model.to_string(); | ||
| let backend = self.deps.llm_backend.clone(); | ||
| if let Err(e) = tokio::task::spawn_blocking(move || { | ||
| // 2a. Update the backend-specific model env var in ~/.ironclaw/.env. | ||
| // | ||
| // Env vars have the HIGHEST priority in LlmConfig::resolve_model() | ||
| // (env var > TOML > DB > default). If the .env file has e.g. | ||
| // NEARAI_MODEL=old-model, it shadows everything else. We must | ||
| // update this var or the /model change is invisible on restart. | ||
| let registry = crate::llm::ProviderRegistry::load(); | ||
| let model_env = registry.model_env_var(&backend); | ||
|
Comment on lines
+861
to
+862
|
||
| let env_var_prefix = format!("{}=", model_env); | ||
|
|
||
| // Only update the .env file if the var is actually set there | ||
| // (avoid injecting new vars the user never configured). | ||
| let env_path = crate::bootstrap::ironclaw_env_path(); | ||
| let env_has_var = std::fs::read_to_string(&env_path) | ||
| .ok() | ||
| .is_some_and(|content| { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. High Severity — The check for whether the env var exists in line.trim_start().starts_with(model_env)Where Meanwhile, Fix: let prefix = format!("{model_env}=");
content.lines().any(|line| line.trim_start().starts_with(&prefix))
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in d824875. Now uses |
||
| content.lines().any(|line| { | ||
| let trimmed = line.trim_start(); | ||
| !trimmed.starts_with('#') && trimmed.starts_with(&env_var_prefix) | ||
| }) | ||
| }); | ||
| if env_has_var { | ||
| if let Err(e) = crate::bootstrap::upsert_bootstrap_var(model_env, &model_owned) { | ||
| tracing::warn!("Failed to update {} in .env: {}", model_env, e); | ||
| } else { | ||
| tracing::debug!("Updated {} in .env to {}", model_env, model_owned); | ||
| } | ||
| } | ||
|
|
||
| // 2b. Update (or create) the TOML config file. | ||
| // | ||
| // The TOML overlay has higher priority than DB settings on | ||
| // startup, so it MUST stay in sync with the DB. | ||
| let toml_path = crate::settings::Settings::default_toml_path(); | ||
| match crate::settings::Settings::load_toml(&toml_path) { | ||
| Ok(Some(mut settings)) => { | ||
|
|
@@ -856,7 +894,15 @@ impl Agent { | |
| } | ||
| } | ||
| Ok(None) => { | ||
| // No config file on disk; nothing to update. | ||
| // No config file yet — create one so the model choice | ||
| // survives restarts even when the DB is unavailable. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Medium Severity — newly created When no
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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is safe by construction. |
||
| let settings = crate::settings::Settings { | ||
| selected_model: Some(model_owned), | ||
| ..Default::default() | ||
| }; | ||
| if let Err(e) = settings.save_toml(&toml_path) { | ||
| tracing::warn!("Failed to create config.toml for model persistence: {}", e); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!("Failed to load config.toml for model persistence: {}", e); | ||
|
|
@@ -865,7 +911,7 @@ impl Agent { | |
| }) | ||
| .await | ||
| { | ||
| tracing::warn!("Model TOML persistence task failed: {}", e); | ||
| tracing::warn!("Model persistence task failed: {}", e); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Medium Severity — concurrent
/modelcommands could corrupt.envor TOMLThe
.envread-then-conditionally-write and the TOML load-then-write are not atomic. If two/modelcommands 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.