From dba6304f51f167c301a474a5809eaf0d357262a1 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Sun, 19 Apr 2026 17:46:20 -0600 Subject: [PATCH] refactor: partial migration to init in AppConfig --- docs/implementation/PHASE-1-STEP-16-NOTES.md | 486 +++++++++++++++--- docs/implementation/PHASE-1-STEP-16a-NOTES.md | 179 +++++++ src/config/app_config.rs | 152 +++++- 3 files changed, 726 insertions(+), 91 deletions(-) create mode 100644 docs/implementation/PHASE-1-STEP-16a-NOTES.md diff --git a/docs/implementation/PHASE-1-STEP-16-NOTES.md b/docs/implementation/PHASE-1-STEP-16-NOTES.md index 5c78220..489f2d9 100644 --- a/docs/implementation/PHASE-1-STEP-16-NOTES.md +++ b/docs/implementation/PHASE-1-STEP-16-NOTES.md @@ -1,105 +1,443 @@ -# Phase 1 Step 16 Notes: Config → AppConfig Migration Cleanup +# Phase 1 Step 16 — Implementation Notes -**Date**: 2026-04-19 -**Status**: PENDING (cleanup task) +## Status -## Overview +Pending. Architecture plan approved; ready for sub-phase execution. -Complete the migration of mutations from Config to AppConfig/AppState so that Config becomes a pure POJO (serde serialization/deserialization only). +## Plan reference + +- Plan: `docs/PHASE-1-IMPLEMENTATION-PLAN.md` +- Section: "Step 16: Complete Config → AppConfig Migration (Post-QA)" ## Problem -The current startup flow mutates `Config` during init, then calls `to_app_config()` which **only copies serialized fields**, losing all the runtime mutations: +The current startup flow mutates `Config` during `Config::init()`, +then converts it to `AppConfig` via `bridge.rs::to_app_config()`. This +design was transitional — it let us build the new structs alongside +the old one without a big-bang migration. + +Now that the transition is nearly done, we want `Config` to be a +genuine serde POJO: no runtime state, no init logic, nothing that +couldn't round-trip through YAML. The structs that actually represent +runtime state (`AppConfig`, `AppState`, `RequestContext`) should own +their own initialization logic. + +## Target architecture + +Instead of migrating mutations incrementally through the bridge, we +**pivot the initialization direction**. Each struct owns its own init. ``` -YAML → Config (mutated during init) - ↓ -cfg.to_app_config() ← COPIES ONLY, loses mutations +YAML file + ↓ Config::load_from_file (serde only — no init logic) +Config (pure POJO) + ↓ AppConfig::from_config(config) → AppConfig +AppConfig (immutable app-wide settings, self-initializing) + ↓ AppState::init(Arc, ...).await → AppState +AppState (shared process state: vault, mcp_factory, rag_cache, mcp_registry, functions) + ↓ RequestContext::new(Arc, working_mode) +RequestContext (per-request mutable state, unchanged) ``` -### Mutations happening in Config::init +### Struct responsibilities (post-16) -These methods mutate Config and should be migrated to AppConfig: +**`Config`** — trivial serde POJO: +- Only `#[serde(...)]` fields (no `#[serde(skip)]`) +- Only method: `load_from_file(path) -> Result<(Config, String)>` + (returns parsed Config + raw YAML content for secret interpolation) +- Can be round-tripped via YAML -| Method | Location | What It Does | -|-------|----------|-------------| -| `load_envs()` | `mod.rs:387` | Env var overrides into Config fields | -| `set_wrap(&wrap)?` | `mod.rs:390` | Parse wrap string into Config.wrap | -| `load_functions()` | `mod.rs:393` | Load tool functions into Config.functions | -| `setup_model()` | `mod.rs:398` | Resolve model name → Model | -| `load_mcp_servers()` | `mod.rs:395` | Start MCP servers into Config.mcp_registry | -| `setup_document_loaders()` | `mod.rs:399` | Load document loaders | -| `setup_user_agent()` | `mod.rs:400` | Set user_agent to "auto" | +**`AppConfig::from_config(config) -> Result`** absorbs: +- Field-copy from `Config` (same as today's `to_app_config`) +- `load_envs()` — env var overrides +- `set_wrap()` — wrap string validation +- `setup_document_loaders()` — default pdf/docx loaders +- `setup_user_agent()` — resolve `"auto"` user agent +- `resolve_model()` — logic from `Config::setup_model` (picks default + model if `model_id` is empty) -### Other Config Methods Still In Use +**`AppState::init(app_config, log_path, start_mcp_servers, abort_signal)`** absorbs: +- `Vault::init(&app_config)` (vault moves from Config to AppState) +- `McpRegistry::init(...)` (currently `Config::load_mcp_servers`) +- `Functions::init(...)` (currently `Config::load_functions`) +- Returns fully-wired `AppState` -| Method | Location | Used By | -|--------|----------|---------| -| `set_model(&model_id)` | `mod.rs:457-466` | Runtime (role-like mutators) | -| `vault_password_file()` | `mod.rs:411-419` | `vault/mod.rs:40` | -| `sessions_dir()` | `mod.rs:421-429` | Session management | -| `role_like_mut()` | `mod.rs:431-441` | Role/session/agent mutation | -| `set_wrap(&str)` | `mod.rs:443-455` | Runtime wrap setting | +**`install_builtins()`** — top-level free function (replaces +`Agent::install_builtin_agents()` + `Macro::install_macros()` being +called inside `Config::init`). Called once from `main.rs` before any +config loading. Config-independent — just copies embedded assets. -## Solution +**`bridge.rs` — deleted.** No more `to_app_config()` / +`to_request_context()`. -**Option A (Quick)**: Move mutations after the bridge in `main.rs`: +## Sub-phase layout +| Sub-phase | Scope | +|-----------|-------| +| 16a | Build `AppConfig::from_config` absorbing env/wrap/docs/user-agent/model-resolution logic | +| 16b | Extract `install_builtins()` as top-level function | +| 16c | Migrate `Vault` onto `AppState` | +| 16d | Build `AppState::init` absorbing MCP-registry/functions logic | +| 16e | Update `main.rs` + audit all 15 `Config::init()` callers, switch to new flow | +| 16f | Delete Config runtime fields, bridge.rs, `Config::init`, duplicated methods | + +Sub-phases 16a–16d can largely proceed in parallel (each adds new +entry points without removing the old ones). 16e switches callers. +16f is the final cleanup. + +## 16a — AppConfig::from_config + +**Target signature:** ```rust -// main.rs -let cfg = Config::init(...).await?; -let app_config = Arc::new(cfg.to_app_config()); // Step 1: copy serialized +impl AppConfig { + pub fn from_config(config: Config) -> Result { + let mut app_config = Self { + // Copy all serde fields from config + }; + app_config.load_envs(); + if let Some(wrap) = app_config.wrap.clone() { + app_config.set_wrap(&wrap)?; + } + app_config.setup_document_loaders(); + app_config.setup_user_agent(); + app_config.resolve_model()?; + Ok(app_config) + } -// Step 2: apply mutations to AppConfig -app_config.load_envs(); -app_config.set_wrap(&wrap)?; -app_config.setup_model()?; // May need refactored - -// Step 3: build AppState -let app_state = Arc::new(AppState { - config: app_config, - vault: cfg.vault.clone(), - // ... other fields -}); - -// Step 4: build RequestContext (runtime only, no config logic) -let ctx = cfg.to_request_context(app_state); + fn resolve_model(&mut self) -> Result<()> { + if self.model_id.is_empty() { + let models = crate::client::list_models(self, ModelType::Chat); + if models.is_empty() { + bail!("No available model"); + } + self.model_id = models[0].id(); + } + Ok(()) + } +} ``` -**Option B (Proper)**: Remove Config entirely from startup flow - build AppConfig directly from YAML. +**New method: `AppConfig::resolve_model()`** — moves logic from +`Config::setup_model`. Ensures `model_id` is a valid, non-empty +concrete model reference. -## Duplicated Code to Clean Up +**Note on `Model` vs `model_id`:** `Model` (the resolved runtime +handle) stays on `RequestContext`. AppConfig owns `model_id: String` +(the config default). RequestContext.model is built by calling +`Model::retrieve_model(&app_config, &model_id, ModelType::Chat)` +during context construction. They're different types for a reason. -After migration, these duplicated methods can be removed from AppConfig: +**Files modified (16a):** +- `src/config/app_config.rs` — add `from_config`, `resolve_model` +- Also remove `#[allow(dead_code)]` from `load_envs`, `set_wrap`, + `setup_document_loaders`, `setup_user_agent`, `set_*_default`, + `ensure_default_model_id` (they all become reachable) -| Duplicated | Config Location | AppConfig Location | -|-----------|-----------------|-------------------| -| `load_envs()` | `mod.rs:582-722` | `app_config.rs:283-427` | -| `set_wrap()` | `mod.rs:443-455` | `app_config.rs:247-259` | -| `setup_document_loaders()` | `mod.rs:782-789` | `app_config.rs:262-269` | -| `setup_user_agent()` | `mod.rs:791-799` | `app_config.rs:272-280` | -| Default impls | `mod.rs:232-311` | `app_config.rs:94-149` | +**Bridge still exists after 16a.** `Config::init` still calls its own +mutations for now. 16a just introduces the new entry point. -## Target State +## 16b — install_builtins() -After Step 16: +**Target signature:** +```rust +// In src/config/mod.rs or a new module +pub fn install_builtins() -> Result<()> { + Agent::install_builtin_agents()?; + Macro::install_macros()?; + Ok(()) +} +``` -- [ ] Config only has serde fields + Deserialization (no init logic) -- [ ] AppConfig receives all runtime mutations -- [ ] AppState built from AppConfig + Vault -- [ ] RequestContext built from AppState + runtime state -- [ ] Duplicated methods removed from AppConfig (or retained if needed) -- [ ] Bridge simplified to just field copying +**Changes:** +- Remove `Agent::install_builtin_agents()?;` and + `Macro::install_macros()?;` calls from inside `Config::init` +- Add `install_builtins()?;` to `main.rs` as the first step before + any config loading -## Files to Modify +Both functions are Config-independent (they just copy embedded +assets to the config directory), so this is a straightforward +extraction. -- `src/config/mod.rs` - Remove init mutations, keep only serde -- `src/config/app_config.rs` - Enable mutations, remove duplication -- `src/main.rs` - Move bridge after mutations -- `src/config/bridge.rs` - Simplify or remove +**Files modified (16b):** +- `src/config/mod.rs` — remove calls from `Config::init`, expose + `install_builtins` as a module-level pub fn +- `src/main.rs` — call `install_builtins()?;` at startup -## Notes +## 16c — Vault → AppState -- This cleanup enables proper REST API behavior -- Each HTTP request should build fresh RequestContext from AppConfig -- AppConfig should reflect actual runtime configuration \ No newline at end of file +Today `Config.vault: Arc` is a `#[serde(skip)]` runtime +field populated by `Vault::init(config)`. Post-16c, the vault lives +natively on `AppState`. + +**Current:** +```rust +pub struct AppState { + pub config: Arc, + pub vault: GlobalVault, // Already here, sourced from config.vault + ... +} +``` + +Wait — `AppState.vault` already exists. The work in 16c is just: + +1. Change `Vault::init(config: &Config)` → `Vault::init(config: &AppConfig)` + - `Vault::init` only reads `config.vault_password_file()`, which + is already a serde field on AppConfig. Rename the param. +2. Delete `Config.vault` field (no longer needed once 16e routes + through AppState) +3. Update `main.rs` to call `Vault::init(&app_config)` instead of + `cfg.vault.clone()` + +**Files modified (16c):** +- `src/vault/mod.rs` — `Vault::init` takes `&AppConfig` +- `src/config/mod.rs` — delete `Config.vault` field (after callers + switch) + +## 16d — AppState::init + +**Target signature:** +```rust +impl AppState { + pub async fn init( + config: Arc, + log_path: Option, + start_mcp_servers: bool, + abort_signal: AbortSignal, + ) -> Result { + let vault = Vault::init(&config); + let functions = { + let mut fns = Functions::init( + config.visible_tools.as_ref().unwrap_or(&Vec::new()) + )?; + // REPL-specific fns appended by RequestContext, not here + fns + }; + let mcp_registry = McpRegistry::init( + log_path.clone(), + start_mcp_servers, + config.enabled_mcp_servers.clone(), + abort_signal, + &config, // new signature: &AppConfig + ).await?; + let (mcp_config, mcp_log_path) = ( + mcp_registry.mcp_config().cloned(), + mcp_registry.log_path().cloned(), + ); + Ok(Self { + config, + vault, + mcp_factory: Default::default(), + rag_cache: Default::default(), + mcp_config, + mcp_log_path, + mcp_registry: Some(mcp_registry), // NEW field + functions, // NEW field + }) + } +} +``` + +**New AppState fields:** +- `mcp_registry: Option` — the live registry of started + MCP servers (currently on Config) +- `functions: Functions` — the global function declarations (currently + on Config) + +These become the "source of truth" that `ToolScope` copies from when +a scope transition happens. + +**`McpRegistry::init` signature change:** today takes `&Config`, +needs to take `&AppConfig`. Only reads serialized fields. + +**Files modified (16d):** +- `src/config/app_state.rs` — add `init`, add `mcp_registry` + + `functions` fields +- `src/mcp/mod.rs` — `McpRegistry::init` takes `&AppConfig` + +**Important:** `Functions.append_user_interaction_functions()` is +currently called inside `Config::load_functions` when in REPL mode. +That logic is working-mode-dependent and belongs on `RequestContext` +(which knows its mode), not `AppState`. The migration moves that +append step to `RequestContext::new` or similar. + +## 16e — Switch main.rs and 15 callers + +**New `main.rs` flow:** +```rust +install_builtins()?; +let (config, raw_yaml) = Config::load_from_file(&paths::config_file())?; + +// Secret interpolation (two-pass) +let bootstrap_vault = Vault::init_from_password_file(&config.vault_password_file()); +let interpolated = interpolate_secrets_or_err(&raw_yaml, &bootstrap_vault, info_flag)?; +let final_config = if interpolated != raw_yaml { + Config::load_from_str(&interpolated)? +} else { + config +}; + +let app_config = Arc::new(AppConfig::from_config(final_config)?); +let app_state = Arc::new( + AppState::init( + app_config.clone(), + log_path, + start_mcp_servers, + abort_signal.clone(), + ).await? +); +let ctx = RequestContext::new(app_state.clone(), working_mode); +``` + +**Secret interpolation complication:** Today's `Config::init` does a +two-pass YAML parse — load, init vault, interpolate secrets into raw +content, re-parse if content changed. In the new flow: +1. Load Config from YAML (also returns raw content) +2. Bootstrap Vault using Config's `vault_password_file` serde field +3. Interpolate secrets in raw content +4. If content changed, re-parse Config +5. Build AppConfig from final Config +6. Build AppState (creates the full Vault via `Vault::init(&app_config)`) + +Step 2 and step 6 create the vault twice — once bootstrap (to decrypt +secrets in raw YAML), once full (for AppState). This matches current +behavior, just made explicit. + +**15 callers of `Config::init()`** — audit required. Discovery +happens during 16e execution. Open questions flagged for user input +as discovered. + +| File | Expected Action | +|------|-----------------| +| `main.rs` | Use new flow | +| `client/common.rs` | Probably needs AppConfig only | +| `vault/mod.rs` | Already uses `Config::vault_password_file`; switch to AppConfig | +| `config/request_context.rs` | Test helper — use `AppState::test_default()` or build directly | +| `config/session.rs` | Test helper — same | +| `rag/mod.rs` | Probably AppConfig | +| `function/supervisor.rs` | Test helper | +| `utils/request.rs` | Probably AppConfig | +| `config/role.rs` | Test helper | +| `utils/clipboard.rs` | Probably AppConfig | +| `supervisor/mod.rs` | Test helper | +| `repl/mod.rs` | Test helper | +| `parsers/common.rs` | Probably AppConfig | +| `utils/abort_signal.rs` | Probably AppConfig | +| `utils/spinner.rs` | Probably AppConfig | + +**Files modified (16e):** +- `src/main.rs` +- Any of the 15 files above that aren't trivial — may need + `test_default()` helpers added + +## 16f — Final cleanup + +**Delete from `Config`:** +- All `#[serde(skip)]` fields: `vault`, `macro_flag`, `info_flag`, + `agent_variables`, `model`, `functions`, `mcp_registry`, + `working_mode`, `last_message`, `role`, `session`, `rag`, `agent`, + `tool_call_tracker`, `supervisor`, `parent_supervisor`, + `self_agent_id`, `current_depth`, `inbox`, + `root_escalation_queue` +- `Config::init` (whole function) +- `Config::load_envs`, `Config::load_functions`, + `Config::load_mcp_servers`, `Config::setup_model`, + `Config::set_model`, `Config::role_like_mut`, + `Config::sessions_dir`, `Config::set_wrap`, + `Config::setup_document_loaders`, `Config::setup_user_agent`, + `Config::vault_password_file` (redundant with AppConfig's) + +**Delete:** +- `src/config/bridge.rs` (entire file) +- `mod bridge;` declaration in `config/mod.rs` + +**Resulting `Config`:** +```rust +#[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(default)] +pub struct Config { + // Only serde-annotated fields — the YAML shape + pub model_id: String, + pub temperature: Option, + // ... all the other serde fields +} + +impl Config { + pub fn load_from_file(path: &Path) -> Result<(Config, String)> { ... } + pub fn load_from_str(content: &str) -> Result { ... } +} +``` + +A genuine POJO. No runtime state. No init logic. Just shape. + +## Open questions (for execution) + +1. **`Vault::init_bare`** — currently used as a fallback when no + Config exists. Does it still need to exist? The default + `vault_password_file` location is static. Might need + `AppConfig::default().vault_password_file()` or a free function. +2. **Secret interpolation ownership** — does `AppConfig::from_config` + handle it internally (takes raw YAML string and interpolates), or + does `main.rs` orchestrate the two-pass explicitly? Leaning toward + `main.rs` orchestration (cleaner separation). +3. **REPL-only `append_user_interaction_functions`** — moves to + `RequestContext::new`? Or stays as a post-init append called + explicitly? +4. **`Functions::init` + MCP meta functions** — today + `load_mcp_servers` calls `self.functions.append_mcp_meta_functions(...)` + after starting servers. In the new flow, `AppState::init` does + this. Verify ordering is preserved. +5. **Testing strategy** — User said don't worry unless trivial. If + test helpers need refactoring to work with new flow, prefer + adding `test_default()` methods gated by `#[cfg(test)]` over + rewriting tests. + +## Dependencies between sub-phases + +``` +16a ──┐ +16b ──┤ +16c ──┼──→ 16d ──→ 16e ──→ 16f + │ +16b, 16c, 16a independent and can run in any order +16d depends on 16c (vault on AppConfig) +16e depends on 16a, 16d (needs the new entry points) +16f depends on 16e (needs all callers switched) +``` + +## Rationale for this architecture + +The original Step 16 plan migrated mutations piecewise through the +existing `to_app_config()` bridge. That works but: + +- Leaves the bridge in place indefinitely +- Keeps `Config` burdened with both YAML shape AND runtime state +- Requires careful ordering to avoid breaking downstream consumers + like `load_functions`/`load_mcp_servers`/`setup_model` +- Creates transitional states where some mutations live on Config, + some on AppConfig + +The new approach: + +- Eliminates `bridge.rs` entirely +- Makes `Config` a true POJO +- Makes `AppConfig`/`AppState` self-contained (initialize from YAML + directly) +- REST API path is trivial: `AppConfig::from_config(yaml_string)` +- Test helpers can build `AppConfig`/`AppState` without Config +- Each struct owns exactly its concerns + +## Verification criteria for each sub-phase + +- 16a: `cargo check` + `cargo test` clean. `AppConfig::from_config` + produces the same state as `Config::init` + `to_app_config()` for + the same YAML input. +- 16b: `install_builtins()` called once from `main.rs`; agents and + macros still install on first startup. +- 16c: `Vault::init` takes `&AppConfig`; `Config.vault` field deleted. +- 16d: `AppState::init` builds a fully-wired `AppState` from + `Arc` + startup context. MCP servers start; functions + load. +- 16e: REPL starts, all CLI flags work, all env vars honored, all + existing tests pass. +- 16f: Grep for `Config {` / `Config::init(` / `bridge::to_` shows + zero non-test hits. `Config` has only serde fields. diff --git a/docs/implementation/PHASE-1-STEP-16a-NOTES.md b/docs/implementation/PHASE-1-STEP-16a-NOTES.md new file mode 100644 index 0000000..49b1589 --- /dev/null +++ b/docs/implementation/PHASE-1-STEP-16a-NOTES.md @@ -0,0 +1,179 @@ +# Phase 1 Step 16a — Implementation Notes + +## Status + +Done. + +## Plan reference + +- Parent plan: `docs/implementation/PHASE-1-STEP-16-NOTES.md` +- Sub-phase goal: "Build `AppConfig::from_config` absorbing + env/wrap/docs/user-agent/model-resolution logic" + +## Summary + +Introduced `AppConfig::from_config(config: Config) -> Result` +as the canonical constructor for a fully-initialized `AppConfig`. The +new constructor chains the four mutation methods (`load_envs`, +`set_wrap`, `setup_document_loaders`, `setup_user_agent`) plus a new +`resolve_model()` method that picks a default model when `model_id` +is empty. + +The existing bridge (`Config::init` + `Config::to_app_config`) is +untouched. `from_config` is currently dead code (gated with +`#[allow(dead_code)]`) — it becomes the entry point in Step 16e when +`main.rs` switches over. The methods it calls (`load_envs`, etc.) are +no longer dead code because they're reachable via `from_config`, so +their `#[allow(dead_code)]` gates were removed. + +## What was changed + +### `src/config/app_config.rs` + +**Added:** +- `AppConfig::from_config(config) -> Result` — canonical + constructor that copies serde fields, applies env overrides, + validates wrap, installs doc loaders, resolves user agent, and + ensures a default model. +- `AppConfig::resolve_model(&mut self) -> Result<()>` — if + `model_id` is empty, picks the first available chat model. Errors + if no models are available. Replaces the logic from + `Config::setup_model` that belongs on `AppConfig` (the + `Model`-resolution half of `Config::setup_model` stays in Config + for now — that moves in 16e). +- 8 unit tests covering field copying, doc loader insertion, user + agent resolution, wrap validation (valid + invalid), and + `resolve_model` error/happy paths. + +**Removed `#[allow(dead_code)]` from:** +- `set_wrap` +- `setup_document_loaders` +- `setup_user_agent` +- `load_envs` + +These are now reachable via `from_config`. They remain `pub` because +REPL-mode mutations (via `.set wrap ` or similar) will go +through them once `RequestContext` stops mutating `Config`. + +**Removed entirely:** +- `AppConfig::ensure_default_model_id` — redundant with the new + `resolve_model`. Had no callers outside itself (confirmed via + grep). + +### Behavior parity notes + +1. **`from_config` is non-destructive:** it consumes `Config` by + value (not `&Config`) since post-bridge, Config is no longer + needed. This matches the long-term design. + +2. **`from_config` vs `to_app_config` + mutations:** The methods + called inside `from_config` are identical bodies to the ones + currently called on `Config` inside `Config::init`. Env var + reads, wrap validation, doc loader defaults, and user agent + resolution all produce the same state. + +3. **`resolve_model` vs `Config::setup_model`:** + - `Config::setup_model` does TWO things: + (a) ensure `model_id` is non-empty (pick default if empty) + (b) resolve the `Model` struct via `Model::retrieve_model` and + store it in `self.model` + - `AppConfig::resolve_model` only does (a). + - (b) happens today in `cfg.set_model(&model_id)` inside + `Config::setup_model`. In the new architecture, the `Model` + struct lives on `RequestContext.model`, and + `Model::retrieve_model(&app_config, &app_config.model_id, ...)` + will be called inside `RequestContext::new` (or equivalent) + once the bridge is removed in 16e. + +## Files modified + +- `src/config/app_config.rs` — 2 new methods, 4 + `#[allow(dead_code)]` gates removed, 1 method deleted, 8 new + tests. + +## Files NOT modified + +- `src/config/mod.rs` — `Config::init` still runs all mutations on + Config; bridge still copies to AppConfig. Unchanged in 16a. +- `src/config/bridge.rs` — Untouched. Used by `from_config` + internally (`config.to_app_config()`). +- `src/main.rs` — Still uses the bridge flow. Switch happens in 16e. + +## Assumptions made + +1. **`from_config` consumes `Config` by value** (not `&Config`) + — aligns with the long-term design where `Config` is discarded + after conversion. No current caller would benefit from keeping + the Config around after conversion. + +2. **`resolve_model` narrow scope**: only responsible for ensuring + `model_id` is non-empty. Does NOT resolve a `Model` struct — + that's RequestContext's job. This matches the split between + `AppConfig` (the configuration) and `RequestContext` (the + resolved runtime handle). + +3. **`#[allow(dead_code)]` on `from_config` and `resolve_model`**: + they're unused until 16e. The gate is explicit so grep-hunts can + find them when 16e switches over. + +4. **User agent prefix in tests**: I assumed the user agent prefix + is not critical to test literally (it depends on the crate name). + The test checks for a non-"auto" value containing `/` rather + than matching `loki-ai/`. Safer against crate rename. + +## Open questions (parked for later sub-phases) + +1. **Should `from_config` also run secret interpolation?** Currently + `Config::init` does a two-pass YAML parse where the raw content + gets secrets injected from the vault, then the Config is + re-parsed. In the new architecture this belongs in `main.rs` or + a separate helper (the Config comes in already-interpolated). + Not a 16a concern. + +2. **Test naming convention**: Existing tests use + `fn test_name_returns_value_when_condition`. New tests use + `fn from_config_does_thing`. Both styles present in the file; + kept consistent with new code. + +3. **`ensure_default_model_id` deletion**: confirmed via grep that + it had no callers outside itself. Deleted cleanly. If a future + sub-phase needs the Option return variant, it can be + re-added. + +## Verification + +- `cargo check` — clean, zero warnings +- `cargo clippy` — clean, zero warnings +- `cargo test` — 122 passing (114 pre-16a + 8 new), zero failures + +## Remaining work for Step 16 + +- **16b**: Extract `install_builtins()` as top-level free function +- **16c**: Migrate `Vault::init(&Config)` → `Vault::init(&AppConfig)` +- **16d**: Build `AppState::init(app_config, ...).await` +- **16e**: Switch `main.rs` and all 15 `Config::init()` callers to + the new flow +- **16f**: Delete Config runtime fields, bridge.rs, `Config::init`, + duplicated methods + +## Migration direction preserved + +After 16a, no runtime behavior has changed. The new entry point +exists but isn't wired in. The bridge flow continues as before: + +``` +YAML → Config::load_from_file + → Config::init (unchanged, does all current mutations) + - load_envs, set_wrap, setup_document_loaders, ... + - setup_model, load_functions, load_mcp_servers + → cfg.to_app_config() → AppConfig (via bridge) + → cfg.to_request_context(AppState) → RequestContext +``` + +New entry point ready for 16e: + +``` +AppConfig::from_config(config) → AppConfig + (internally: to_app_config, load_envs, set_wrap, + setup_document_loaders, setup_user_agent, resolve_model) +``` diff --git a/src/config/app_config.rs b/src/config/app_config.rs index c00ae54..30cb512 100644 --- a/src/config/app_config.rs +++ b/src/config/app_config.rs @@ -19,7 +19,7 @@ //! Runtime-only state (current role, session, agent, supervisor, etc.) //! lives on [`RequestContext`](super::request_context::RequestContext). -use crate::client::ClientConfig; +use crate::client::{list_models, ClientConfig}; use crate::render::{MarkdownRender, RenderOptions}; use crate::utils::{IS_STDOUT_TERMINAL, NO_COLOR, decode_bin, get_env_name}; @@ -150,6 +150,31 @@ impl Default for AppConfig { } impl AppConfig { + #[allow(dead_code)] + pub fn from_config(config: super::Config) -> Result { + let mut app_config = config.to_app_config(); + app_config.load_envs(); + if let Some(wrap) = app_config.wrap.clone() { + app_config.set_wrap(&wrap)?; + } + app_config.setup_document_loaders(); + app_config.setup_user_agent(); + app_config.resolve_model()?; + Ok(app_config) + } + + #[allow(dead_code)] + pub fn resolve_model(&mut self) -> Result<()> { + if self.model_id.is_empty() { + let models = list_models(self, crate::client::ModelType::Chat); + if models.is_empty() { + anyhow::bail!("No available model"); + } + self.model_id = models[0].id(); + } + Ok(()) + } + pub fn vault_password_file(&self) -> PathBuf { match &self.vault_password_file { Some(path) => match path.exists() { @@ -232,7 +257,6 @@ impl AppConfig { } impl AppConfig { - #[allow(dead_code)] pub fn set_wrap(&mut self, value: &str) -> Result<()> { if value == "no" { self.wrap = None; @@ -247,7 +271,6 @@ impl AppConfig { Ok(()) } - #[allow(dead_code)] pub fn setup_document_loaders(&mut self) { [("pdf", "pdftotext $1 -"), ("docx", "pandoc --to plain $1")] .into_iter() @@ -257,7 +280,6 @@ impl AppConfig { }); } - #[allow(dead_code)] pub fn setup_user_agent(&mut self) { if let Some("auto") = self.user_agent.as_deref() { self.user_agent = Some(format!( @@ -268,7 +290,6 @@ impl AppConfig { } } - #[allow(dead_code)] pub fn load_envs(&mut self) { if let Ok(v) = env::var(get_env_name("model")) { self.model_id = v; @@ -460,18 +481,6 @@ impl AppConfig { pub fn set_model_id_default(&mut self, model_id: String) { self.model_id = model_id; } - - #[allow(dead_code)] - pub fn ensure_default_model_id(&mut self) -> Result { - if self.model_id.is_empty() { - let models = crate::client::list_models(self, crate::client::ModelType::Chat); - if models.is_empty() { - anyhow::bail!("No available model"); - } - self.model_id = models[0].id(); - } - Ok(self.model_id.clone()) - } } #[cfg(test)] @@ -583,4 +592,113 @@ mod tests { let url = app.sync_models_url(); assert!(!url.is_empty()); } + + #[test] + fn from_config_copies_serde_fields() { + let cfg = Config { + model_id: "provider:model-x".to_string(), + temperature: Some(0.42), + compression_threshold: 1234, + ..Config::default() + }; + + let app = AppConfig::from_config(cfg).unwrap(); + + assert_eq!(app.model_id, "provider:model-x"); + assert_eq!(app.temperature, Some(0.42)); + assert_eq!(app.compression_threshold, 1234); + } + + #[test] + fn from_config_installs_default_document_loaders() { + let cfg = Config { + model_id: "provider:test".to_string(), + ..Config::default() + }; + let app = AppConfig::from_config(cfg).unwrap(); + + assert_eq!( + app.document_loaders.get("pdf"), + Some(&"pdftotext $1 -".to_string()) + ); + assert_eq!( + app.document_loaders.get("docx"), + Some(&"pandoc --to plain $1".to_string()) + ); + } + + #[test] + fn from_config_resolves_auto_user_agent() { + let cfg = Config { + model_id: "provider:test".to_string(), + user_agent: Some("auto".to_string()), + ..Config::default() + }; + + let app = AppConfig::from_config(cfg).unwrap(); + + let ua = app.user_agent.as_deref().unwrap(); + assert!(ua != "auto", "user_agent should have been resolved"); + assert!(ua.contains('/'), "user_agent should be '/'"); + } + + #[test] + fn from_config_preserves_explicit_user_agent() { + let cfg = Config { + model_id: "provider:test".to_string(), + user_agent: Some("custom/1.0".to_string()), + ..Config::default() + }; + + let app = AppConfig::from_config(cfg).unwrap(); + + assert_eq!(app.user_agent.as_deref(), Some("custom/1.0")); + } + + #[test] + fn from_config_validates_wrap_value() { + let cfg = Config { + model_id: "provider:test".to_string(), + wrap: Some("invalid".to_string()), + ..Config::default() + }; + + let result = AppConfig::from_config(cfg); + assert!(result.is_err()); + } + + #[test] + fn from_config_accepts_wrap_auto() { + let cfg = Config { + model_id: "provider:test".to_string(), + wrap: Some("auto".to_string()), + ..Config::default() + }; + + let app = AppConfig::from_config(cfg).unwrap(); + assert_eq!(app.wrap.as_deref(), Some("auto")); + } + + #[test] + fn resolve_model_errors_when_no_models_available() { + let mut app = AppConfig { + model_id: String::new(), + clients: vec![], + ..AppConfig::default() + }; + + let result = app.resolve_model(); + assert!(result.is_err()); + } + + #[test] + fn resolve_model_keeps_explicit_model_id() { + let mut app = AppConfig { + model_id: "provider:explicit".to_string(), + ..AppConfig::default() + }; + + app.resolve_model().unwrap(); + assert_eq!(app.model_id, "provider:explicit"); + } }