From 7facdce6b6f3b9d6ddbcc594e3bfbd60b5ee044e Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Sun, 19 Apr 2026 18:27:33 -0600 Subject: [PATCH] refactor: Deprecated old Config struct initialization logic --- docs/implementation/PHASE-1-STEP-16e-NOTES.md | 228 ++++++++++++++++++ src/cli/completer.rs | 40 +-- src/config/app_config.rs | 2 - src/config/app_state.rs | 2 - src/config/bridge.rs | 1 + src/config/mod.rs | 72 +++++- src/config/request_context.rs | 46 ++++ src/main.rs | 50 ++-- 8 files changed, 387 insertions(+), 54 deletions(-) create mode 100644 docs/implementation/PHASE-1-STEP-16e-NOTES.md diff --git a/docs/implementation/PHASE-1-STEP-16e-NOTES.md b/docs/implementation/PHASE-1-STEP-16e-NOTES.md new file mode 100644 index 0000000..3240441 --- /dev/null +++ b/docs/implementation/PHASE-1-STEP-16e-NOTES.md @@ -0,0 +1,228 @@ +# Phase 1 Step 16e — Implementation Notes + +## Status + +Done. + +## Plan reference + +- Parent plan: `docs/implementation/PHASE-1-STEP-16-NOTES.md` +- Sub-phase goal: "Switch main.rs and all Config::init() callers + to the new flow" + +## Summary + +`main.rs` and `cli/completer.rs` no longer call `Config::init` or +`Config::init_bare` — they use the new flow: +`Config::load_with_interpolation` → `AppConfig::from_config` → +`AppState::init` → `RequestContext::bootstrap`. + +The bridge `Config::to_request_context` and the old `Config::init` +are now dead code, gated with `#[allow(dead_code)]` pending deletion +in 16f. + +## What was changed + +### New helpers + +**`Config::load_with_interpolation(info_flag: bool) -> Result`** in +`src/config/mod.rs` — absorbs the two-pass YAML parse with secret +interpolation. Handles: +1. Missing config file (creates via `create_config_file` if TTY, or + `load_dynamic` from env vars) +2. Reading the raw YAML content +3. Bootstrapping a Vault from the freshly-parsed Config +4. Interpolating secrets +5. Re-parsing Config if interpolation changed anything +6. Sets `config.vault` (legacy field — deleted in 16f) + +**`config::default_sessions_dir() -> PathBuf`** and +**`config::list_sessions() -> Vec`** free functions — +provide session listing without needing a Config instance. Used by +the session completer. + +**`RequestContext::bootstrap(app: Arc, working_mode, +info_flag) -> Result`** in `src/config/request_context.rs` — +the new entry point for creating the initial RequestContext. Builds: +- Resolved `Model` from `app.config.model_id` +- `ToolScope.functions` cloned from `app.functions` with + `append_user_interaction_functions` added in REPL mode +- `ToolScope.mcp_runtime` synced from `app.mcp_registry` + +### Made public in Config for new flow + +- `Config::load_from_file` (was `fn`) +- `Config::load_from_str` (was `fn`) +- `Config::load_dynamic` (was `fn`) +- `config::create_config_file` (was `async fn`) + +### src/main.rs + +Three startup paths rewired: + +```rust +// Path 1: --authenticate +let cfg = Config::load_with_interpolation(true).await?; +let app_config = AppConfig::from_config(cfg)?; +let (client_name, provider) = + resolve_oauth_client(client_arg.as_deref(), &app_config.clients)?; +oauth::run_oauth_flow(&*provider, &client_name).await?; + +// Path 2: vault flags +let cfg = Config::load_with_interpolation(true).await?; +let app_config = AppConfig::from_config(cfg)?; +let vault = Vault::init(&app_config); +return Vault::handle_vault_flags(cli, &vault); + +// Path 3: main +let cfg = Config::load_with_interpolation(info_flag).await?; +let app_config: Arc = Arc::new(AppConfig::from_config(cfg)?); +let app_state: Arc = Arc::new( + AppState::init(app_config, log_path, start_mcp_servers, abort_signal.clone()).await? +); +let ctx = RequestContext::bootstrap(app_state, working_mode, info_flag)?; +``` + +No more `Config::init`, `Config::to_app_config`, `cfg.mcp_registry`, +or `cfg.to_request_context` references in `main.rs`. + +### src/cli/completer.rs + +Three completers that needed config access updated: + +- `model_completer` → uses new `load_app_config_for_completion()` + helper (runs `Config::load_with_interpolation` synchronously from + the completion context; async via `Handle::try_current` or a fresh + runtime) +- `session_completer` → uses the new free function + `list_sessions()` (no Config needed) +- `secrets_completer` → uses `Vault::init(&app_config)` directly + +### #[allow(dead_code)] removed + +- `AppConfig::from_config` +- `AppConfig::resolve_model` +- `AppState::init` +- `AppState.rag_cache` (was flagged dead; now wired in) + +### #[allow(dead_code)] added (temporary, deleted in 16f) + +- `Config::init_bare` — no longer called +- `Config::sessions_dir` — replaced by free function +- `Config::list_sessions` — replaced by free function +- `Config::to_request_context` — replaced by `RequestContext::bootstrap` + +## Behavior parity + +- `main.rs` startup now invokes: + - `install_builtins()` (installs builtin global tools, agents, + macros — same files get copied as before, Step 16b) + - `Config::load_with_interpolation` (same YAML loading + secret + interpolation as old `Config::init`) + - `AppConfig::from_config` (same env/wrap/docs/user-agent/model + resolution as old Config mutations) + - `AppState::init` (same vault init + MCP registry startup + + global Functions loading as old Config methods, now owned by + AppState; also pre-registers initial servers with McpFactory — + new behavior that fixes a latent cache miss bug) + - `RequestContext::bootstrap` (same initial state as old bridge + `to_request_context`: resolved Model, Functions with REPL + extensions, MCP runtime from registry) + +- Completer paths now use a lighter-weight config load (no MCP + startup) which is appropriate since shell completion isn't + supposed to start subprocesses. + +## Files modified + +- `src/config/mod.rs` — added `load_with_interpolation`, + `default_sessions_dir`, `list_sessions`; made 3 methods public; + added `#[allow(dead_code)]` to `Config::init_bare`, + `sessions_dir`, `list_sessions`. +- `src/config/request_context.rs` — added `bootstrap`. +- `src/config/app_config.rs` — removed 2 `#[allow(dead_code)]` + gates. +- `src/config/app_state.rs` — removed 2 `#[allow(dead_code)]` + gates. +- `src/config/bridge.rs` — added `#[allow(dead_code)]` to + `to_request_context`. +- `src/main.rs` — rewired three startup paths. +- `src/cli/completer.rs` — rewired three completers. + +## Assumptions made + +1. **Completer helper runtime handling**: The three completers run + in a sync context (clap completion). The new + `load_app_config_for_completion` uses + `Handle::try_current().ok()` to detect if a tokio runtime + exists; if so, uses `block_in_place`; otherwise creates a + fresh runtime. This matches the old `Config::init_bare` pattern + (which also used `block_in_place` + `block_on`). + +2. **`Config::to_request_context` kept with `#[allow(dead_code)]`**: + It's unused now but 16f deletes it cleanly. Leaving it in place + keeps 16e a non-destructive switchover. + +3. **`RequestContext::bootstrap` returns `Result` not + `Arc`**: caller decides wrapping. main.rs doesn't wrap; + the REPL wraps `Arc>` a few lines later. + +4. **`install_builtin_global_tools` added to `install_builtins`**: + A function added in user's 16b commit extracted builtin tool + installation out of `Functions::init` into a standalone function. + My Step 16b commit that extracted `install_builtins` missed + including this function — fixed in this step. + +## Verification + +- `cargo check` — clean, zero warnings +- `cargo clippy` — clean, zero warnings +- `cargo test` — 122 passing, zero failures +- Grep confirmation: + - `Config::init(` — only called from `Config::init_bare` (which + is now dead) + - `Config::init_bare` — no external callers (test helper uses + `#[allow(dead_code)]`) + - `to_request_context` — zero callers outside bridge.rs + - `cfg.mcp_registry` / `cfg.functions` / `cfg.vault` references + in main.rs — zero + +## Remaining work for Step 16 + +- **16f**: Delete all `#[allow(dead_code)]` scaffolding: + - `Config::init`, `Config::init_bare` + - `Config::sessions_dir`, `Config::list_sessions` + - `Config::set_wrap`, `Config::setup_document_loaders`, + `Config::setup_user_agent`, `Config::load_envs`, + `Config::load_functions`, `Config::load_mcp_servers`, + `Config::setup_model`, `Config::set_model`, + `Config::role_like_mut`, `Config::vault_password_file` + - `bridge.rs` — delete entirely + - All `#[serde(skip)]` runtime fields on `Config` + - `mod bridge;` declaration + +After 16f, `Config` will be a pure serde POJO with only serialized +fields and `load_from_file` / `load_from_str` / `load_dynamic` / +`load_with_interpolation` methods. + +## Migration direction achieved + +Before 16e: +``` +main.rs: Config::init → to_app_config → AppState {...} → to_request_context +``` + +After 16e: +``` +main.rs: + install_builtins() + Config::load_with_interpolation → AppConfig::from_config + AppState::init(app_config, ...).await + RequestContext::bootstrap(app_state, working_mode, info_flag) +``` + +No more god-init. Each struct owns its initialization. The REST +API path is now trivial: skip `install_builtins()` if not desired, +call `AppConfig::from_config(yaml_string)`, call +`AppState::init(...)`, create per-request `RequestContext` as +needed. diff --git a/src/cli/completer.rs b/src/cli/completer.rs index bc0695b..1809bbc 100644 --- a/src/cli/completer.rs +++ b/src/cli/completer.rs @@ -1,6 +1,7 @@ use crate::client::{ModelType, list_models}; use crate::config::paths; -use crate::config::{Config, list_agents}; +use crate::config::{AppConfig, Config, list_agents, list_sessions}; +use crate::vault::Vault; use clap_complete::{CompletionCandidate, Shell, generate}; use clap_complete_nushell::Nushell; use std::ffi::OsStr; @@ -33,8 +34,8 @@ impl ShellCompletion { pub(super) fn model_completer(current: &OsStr) -> Vec { let cur = current.to_string_lossy(); - match Config::init_bare() { - Ok(config) => list_models(&config.to_app_config(), ModelType::Chat) + match load_app_config_for_completion() { + Ok(app_config) => list_models(&app_config, ModelType::Chat) .into_iter() .filter(|&m| m.id().starts_with(&*cur)) .map(|m| CompletionCandidate::new(m.id())) @@ -43,6 +44,20 @@ pub(super) fn model_completer(current: &OsStr) -> Vec { } } +fn load_app_config_for_completion() -> anyhow::Result { + let h = tokio::runtime::Handle::try_current().ok(); + let cfg = match h { + Some(handle) => { + tokio::task::block_in_place(|| handle.block_on(Config::load_with_interpolation(true)))? + } + None => { + let rt = tokio::runtime::Runtime::new()?; + rt.block_on(Config::load_with_interpolation(true))? + } + }; + AppConfig::from_config(cfg) +} + pub(super) fn role_completer(current: &OsStr) -> Vec { let cur = current.to_string_lossy(); paths::list_roles(true) @@ -81,22 +96,17 @@ pub(super) fn macro_completer(current: &OsStr) -> Vec { pub(super) fn session_completer(current: &OsStr) -> Vec { let cur = current.to_string_lossy(); - match Config::init_bare() { - Ok(config) => config - .list_sessions() - .into_iter() - .filter(|s| s.starts_with(&*cur)) - .map(CompletionCandidate::new) - .collect(), - Err(_) => vec![], - } + list_sessions() + .into_iter() + .filter(|s| s.starts_with(&*cur)) + .map(CompletionCandidate::new) + .collect() } pub(super) fn secrets_completer(current: &OsStr) -> Vec { let cur = current.to_string_lossy(); - match Config::init_bare() { - Ok(config) => config - .vault + match load_app_config_for_completion() { + Ok(app_config) => Vault::init(&app_config) .list_secrets(false) .unwrap_or_default() .into_iter() diff --git a/src/config/app_config.rs b/src/config/app_config.rs index 10cbf1e..1157578 100644 --- a/src/config/app_config.rs +++ b/src/config/app_config.rs @@ -150,7 +150,6 @@ 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(); @@ -163,7 +162,6 @@ impl AppConfig { 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); diff --git a/src/config/app_state.rs b/src/config/app_state.rs index 6118dbc..dfc4b50 100644 --- a/src/config/app_state.rs +++ b/src/config/app_state.rs @@ -45,7 +45,6 @@ pub struct AppState { pub config: Arc, pub vault: GlobalVault, pub mcp_factory: Arc, - #[allow(dead_code)] pub rag_cache: Arc, pub mcp_config: Option, pub mcp_log_path: Option, @@ -54,7 +53,6 @@ pub struct AppState { } impl AppState { - #[allow(dead_code)] pub async fn init( config: Arc, log_path: Option, diff --git a/src/config/bridge.rs b/src/config/bridge.rs index 7826253..b589cbb 100644 --- a/src/config/bridge.rs +++ b/src/config/bridge.rs @@ -63,6 +63,7 @@ impl Config { } } + #[allow(dead_code)] pub fn to_request_context(&self, app: Arc) -> RequestContext { let mut mcp_runtime = super::tool_scope::McpRuntime::default(); if let Some(registry) = &self.mcp_registry { diff --git a/src/config/mod.rs b/src/config/mod.rs index cb4dca4..0ba5efb 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -310,13 +310,25 @@ impl Default for Config { } pub fn install_builtins() -> Result<()> { + Functions::install_builtin_global_tools()?; Agent::install_builtin_agents()?; Macro::install_macros()?; - Functions::install_builtin_global_tools()?; Ok(()) } +pub fn default_sessions_dir() -> PathBuf { + match env::var(get_env_name("sessions_dir")) { + Ok(value) => PathBuf::from(value), + Err(_) => paths::local_path(SESSIONS_DIR_NAME), + } +} + +pub fn list_sessions() -> Vec { + list_file_names(default_sessions_dir(), ".yaml") +} + impl Config { + #[allow(dead_code)] pub fn init_bare() -> Result { let h = Handle::current(); tokio::task::block_in_place(|| { @@ -411,6 +423,7 @@ impl Config { Ok(config) } + #[allow(dead_code)] pub fn sessions_dir(&self) -> PathBuf { match &self.agent { None => match env::var(get_env_name("sessions_dir")) { @@ -458,6 +471,7 @@ impl Config { Ok(()) } + #[allow(dead_code)] pub fn list_sessions(&self) -> Vec { list_file_names(self.sessions_dir(), ".yaml") } @@ -515,7 +529,55 @@ impl Config { Ok(()) } - fn load_from_file(config_path: &Path) -> Result<(Self, String)> { + pub async fn load_with_interpolation(info_flag: bool) -> Result { + let config_path = paths::config_file(); + let (mut config, content) = if !config_path.exists() { + match env::var(get_env_name("provider")) + .ok() + .or_else(|| env::var(get_env_name("platform")).ok()) + { + Some(v) => (Self::load_dynamic(&v)?, String::new()), + None => { + if *IS_STDOUT_TERMINAL { + create_config_file(&config_path).await?; + } + Self::load_from_file(&config_path)? + } + } + } else { + Self::load_from_file(&config_path)? + }; + + let vault = Vault::init(&config.to_app_config()); + let (parsed_config, missing_secrets) = interpolate_secrets(&content, &vault); + if !missing_secrets.is_empty() && !info_flag { + debug!( + "Global config references secrets that are missing from the vault: {missing_secrets:?}" + ); + return Err(anyhow!(formatdoc!( + " + Global config file references secrets that are missing from the vault: {:?} + Please add these secrets to the vault and try again.", + missing_secrets + ))); + } + if !parsed_config.is_empty() && !info_flag { + debug!("Global config is invalid once secrets are injected: {parsed_config}"); + let new_config = Self::load_from_str(&parsed_config).with_context(|| { + formatdoc!( + " + Global config is invalid once secrets are injected. + Double check the secret values and file syntax, then try again. + " + ) + })?; + config = new_config; + } + config.vault = Arc::new(vault); + Ok(config) + } + + pub fn load_from_file(config_path: &Path) -> Result<(Self, String)> { let err = || format!("Failed to load config at '{}'", config_path.display()); let content = read_to_string(config_path).with_context(err)?; let config = Self::load_from_str(&content).with_context(err)?; @@ -523,7 +585,7 @@ impl Config { Ok((config, content)) } - fn load_from_str(content: &str) -> Result { + pub fn load_from_str(content: &str) -> Result { if PASSWORD_FILE_SECRET_RE.is_match(content)? { bail!("secret injection cannot be done on the vault_password_file property"); } @@ -549,7 +611,7 @@ impl Config { Ok(config) } - fn load_dynamic(model_id: &str) -> Result { + pub fn load_dynamic(model_id: &str) -> Result { let provider = match model_id.split_once(':') { Some((v, _)) => v, _ => model_id, @@ -892,7 +954,7 @@ impl AssertState { } } -async fn create_config_file(config_path: &Path) -> Result<()> { +pub async fn create_config_file(config_path: &Path) -> Result<()> { let ans = Confirm::new("No config file, create a new one?") .with_default(true) .prompt()?; diff --git a/src/config/request_context.rs b/src/config/request_context.rs index b4fa0f5..130971b 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -147,6 +147,52 @@ impl RequestContext { } } + pub fn bootstrap( + app: Arc, + working_mode: WorkingMode, + info_flag: bool, + ) -> Result { + let model = Model::retrieve_model(&app.config, &app.config.model_id, ModelType::Chat)?; + + let mut functions = app.functions.clone(); + if working_mode.is_repl() { + functions.append_user_interaction_functions(); + } + + let mut mcp_runtime = McpRuntime::default(); + if let Some(registry) = &app.mcp_registry { + mcp_runtime.sync_from_registry(registry); + } + + Ok(Self { + app, + macro_flag: false, + info_flag, + working_mode, + model, + agent_variables: None, + role: None, + session: None, + rag: None, + agent: None, + last_message: None, + tool_scope: ToolScope { + functions, + mcp_runtime, + tool_tracker: ToolCallTracker::default(), + }, + supervisor: None, + parent_supervisor: None, + self_agent_id: None, + inbox: None, + escalation_queue: None, + current_depth: 0, + auto_continue_count: 0, + todo_list: TodoList::default(), + last_continuation_response: None, + }) + } + pub fn new_for_child( app: Arc, parent: &Self, diff --git a/src/main.rs b/src/main.rs index 0662b3a..a71023c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -85,45 +85,35 @@ async fn main() -> Result<()> { install_builtins()?; if let Some(client_arg) = &cli.authenticate { - let config = Config::init_bare()?; - let (client_name, provider) = resolve_oauth_client(client_arg.as_deref(), &config.clients)?; + let cfg = Config::load_with_interpolation(true).await?; + let app_config = AppConfig::from_config(cfg)?; + let (client_name, provider) = + resolve_oauth_client(client_arg.as_deref(), &app_config.clients)?; oauth::run_oauth_flow(&*provider, &client_name).await?; return Ok(()); } if vault_flags { - let cfg = Config::init_bare()?; - return Vault::handle_vault_flags(cli, &cfg.vault); + let cfg = Config::load_with_interpolation(true).await?; + let app_config = AppConfig::from_config(cfg)?; + let vault = Vault::init(&app_config); + return Vault::handle_vault_flags(cli, &vault); } let abort_signal = create_abort_signal(); let start_mcp_servers = cli.agent.is_none() && cli.role.is_none(); - let cfg = Config::init( - working_mode, - info_flag, - start_mcp_servers, - log_path, - abort_signal.clone(), - ) - .await?; - let app_config: Arc = Arc::new(cfg.to_app_config()); - let (mcp_config, mcp_log_path) = match &cfg.mcp_registry { - Some(reg) => (reg.mcp_config().cloned(), reg.log_path().cloned()), - None => (None, None), - }; - let mcp_registry = cfg.mcp_registry.clone().map(Arc::new); - let functions = cfg.functions.clone(); - let app_state: Arc = Arc::new(AppState { - config: app_config, - vault: cfg.vault.clone(), - mcp_factory: Default::default(), - rag_cache: Default::default(), - mcp_config, - mcp_log_path, - mcp_registry, - functions, - }); - let ctx = cfg.to_request_context(app_state); + let cfg = Config::load_with_interpolation(info_flag).await?; + let app_config: Arc = Arc::new(AppConfig::from_config(cfg)?); + let app_state: Arc = Arc::new( + AppState::init( + app_config, + log_path, + start_mcp_servers, + abort_signal.clone(), + ) + .await?, + ); + let ctx = RequestContext::bootstrap(app_state, working_mode, info_flag)?; { let app = &*ctx.app.config;