[codex] Stabilize web settings LLM hot reload - #2765
Conversation
github-actions
Bot
added
scope: channel/web
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the LLM hot-reload mechanism to resolve only the LLM configuration instead of the full application config, centralizing logic in new resolve_llm_with_secrets and load_db_backed_settings methods. Feedback indicates that environment-loading calls within the hot-reload path should be removed to ensure thread safety and adhere to the requirement that hot-reloads only process database-persisted settings.
There was a problem hiding this comment.
Pull request overview
This PR stabilizes the web-settings-triggered LLM provider hot-reload by re-resolving only LlmConfig while preserving the same DB/TOML layering semantics used at startup, and by hardening the associated tests against ambient env/TOML state.
Changes:
- Refactors DB-backed settings merge logic into a shared helper and reuses it for both full config loads and LLM-only resolution.
- Updates the web settings reload path to resolve
LlmConfigdirectly (including secrets hydration) instead of rebuilding fullConfig. - Hardens hot-reload tests by locking env access and ensuring an explicit (empty) TOML config file exists when a TOML path is injected.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/config/mod.rs | Extracts shared DB/TOML/admin/user settings layering and introduces an LLM-only resolver used by hot reload. |
| src/channels/web/features/settings/mod.rs | Switches reload path to use the LLM-only resolver; strengthens tests with env locking and explicit empty TOML injection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
Bot
added
size: L
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Err(e) if strict_db_reads => { | ||
| return Err(ConfigError::ParseError(format!( | ||
| "Failed to load admin-scope settings from DB: {e}" | ||
| ))); |
There was a problem hiding this comment.
In strict_db_reads mode, DB read failures are surfaced as ConfigError::ParseError. Because ConfigError::ParseError formats as "Failed to parse configuration: ...", callers (and the settings UI via 422 body) will see a misleading parse error for what is actually a DB connectivity/query failure. Consider introducing a dedicated ConfigError variant for config-source read failures (or otherwise returning an error that doesn’t prefix with parse semantics) so user-facing error strings are accurate.
|
|
||
| /// Build the settings overlay used for DB-backed config reads. | ||
| /// | ||
| /// Resolution order is profile -> TOML -> admin DB -> per-user DB. |
There was a problem hiding this comment.
The doc comment for load_db_backed_settings says resolution order starts at "profile", but the implementation begins from Settings::default() and then applies the profile and overlays. Update the comment to include the defaults layer so it matches the actual merge stack.
| /// Resolution order is profile -> TOML -> admin DB -> per-user DB. | |
| /// Resolution order is defaults -> profile -> TOML -> admin DB -> per-user DB. |
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
What looks good:
- The refactor narrows hot reload to
LlmConfiginstead of rebuilding unrelated config. - The owner/admin layering is now centralized in one resolver path instead of being duplicated between full-config and hot-reload flows.
- The hot-reload path now fails closed on DB read errors, which matches the review concern and preserves rollback behavior.
No verified findings.
Low-priority notes:
src/config/mod.rsplussrc/channels/web/features/settings/mod.rs: the DB-read failure behavior looks fixed in code, but there still is not one end-to-end handler test that injectsget_all_settingsfailure and proves422 + rollbackthroughsettings_set_handler.
Summary:
- Recommended verdict: Approve
- Prior feedback status: partially unresolved
- Residual risk: reviewed diff-only because sandboxed
git fetchcould not update.git/FETCH_HEAD, and the targeted reload-path tests were inspected but not executed locally.
|
already got an approval (look at comment above just not an approval click overriding and merging) |
* Stabilize web settings LLM hot reload * Keep web settings hot reload DB-scoped * fix(config): preserve TOML overlay in llm re-resolve * test(config): allow env lock in async toml re-resolve test * fix(web): fail closed on hot reload db read errors
What changed
LlmConfigwhile preserving the same owner/admin DB merge semantics used at startup./.envand~/.ironclaw/.envstateWhy
The failing web settings tests were exercising the LLM provider hot-reload path. The fix needed to keep owner-scope layering intact without rebuilding unrelated config sections, while also preserving the previous behavior where a settings-triggered reload would re-read env-file state.
User impact
LLM settings changes in the web UI now rebuild the provider chain against the same effective configuration that startup uses, including owner overlays and refreshed env-backed credentials/base URLs.
Root cause
The original hot-reload path rebuilt full config from the owner scope. The refactor to narrow that down to LLM-only resolution fixed the owner/admin layering issue, but initially dropped the dotenv/bootstrap refresh step. That caused hot reload to resolve against stale startup env when env files had changed.
Validation
cargo fmt --allgit diff --checkCARGO_TARGET_DIR=/tmp/ironclaw-settings-test-target cargo test re_resolve_llm_keeps_admin_only_keys_for_operator -- --nocaptureCARGO_TARGET_DIR=/tmp/ironclaw-settings-test-target cargo test settings_set_handler_triggers_llm_provider_hot_reload -- --nocaptureCARGO_TARGET_DIR=/tmp/ironclaw-settings-test-target cargo test --lib settings_set_handler_owner_scope_triggers_reload -- --nocaptureCARGO_TARGET_DIR=/tmp/ironclaw-settings-test-target cargo test --lib reload_rebuilds_from_owner_scope_not_effective_scope -- --nocaptureCARGO_TARGET_DIR=/tmp/ironclaw-settings-test-target cargo test --lib settings_set_handler_rolls_back_on_reload_failure -- --nocapture