diff --git a/docs/implementation/PHASE-1-STEP-16b-NOTES.md b/docs/implementation/PHASE-1-STEP-16b-NOTES.md new file mode 100644 index 0000000..8ed1f49 --- /dev/null +++ b/docs/implementation/PHASE-1-STEP-16b-NOTES.md @@ -0,0 +1,170 @@ +# Phase 1 Step 16b — Implementation Notes + +## Status + +Done. + +## Plan reference + +- Parent plan: `docs/implementation/PHASE-1-STEP-16-NOTES.md` +- Sub-phase goal: "Extract `install_builtins()` as top-level function" + +## Summary + +Extracted `Agent::install_builtin_agents()` and `Macro::install_macros()` +from inside `Config::init` into a new top-level free function +`config::install_builtins()`. Called once from `main.rs` before any +config-loading path. + +Both functions are Config-independent — they just copy embedded +agent/macro assets from the binary into the user's config directory. +Extracting them clears the way for `Config::init`'s eventual +deletion in Step 16f. + +## What was changed + +### `src/config/mod.rs` + +**Added:** +```rust +pub fn install_builtins() -> Result<()> { + Agent::install_builtin_agents()?; + Macro::install_macros()?; + Ok(()) +} +``` + +Placed after the `Config::Default` impl, before the `impl Config` +block. Module-level `pub fn` (not a method on any type). + +**Removed from `Config::init` (inside the async `setup` closure):** +- `Agent::install_builtin_agents()?;` (was at top of setup block) +- `Macro::install_macros()?;` (was at bottom of setup block) + +### `src/main.rs` + +**Added:** +- `install_builtins` to the `use crate::config::{...}` import list +- `install_builtins()?;` call after `setup_logger()?` and before any + of the three config-loading paths (oauth, vault flags, main config) + +### Placement rationale + +The early-return paths (`cli.completions`, `cli.tail_logs`) +legitimately don't need builtins — they return before touching any +config. Those skip the install. + +The three config paths (oauth via `Config::init_bare`, vault flags +via `Config::init_bare`, main via `Config::init`) all benefit from +builtins being installed once at startup. `install_builtins()` is +idempotent — it checks file existence and skips if already present — +so calling it unconditionally in the common path is safe. + +## Behavior parity + +- `install_builtin_agents` and `install_macros` are static methods + with no `&self` or Config arguments. Nothing observable changes + about their execution. +- The two functions ran on every `Config::init` call before. Now + they run once per `main.rs` invocation, which is equivalent for + the REPL and CLI paths. +- `Config::init_bare()` no longer triggers the installs + transitively. The oauth and vault-flag paths now rely on `main.rs` + having called `install_builtins()?` first. This is a minor + behavior shift — those paths previously installed builtins as a + side effect of calling `Config::init_bare`. Since we now call + `install_builtins()` unconditionally in `main.rs` before those + paths, the observable behavior is identical. + +## Files modified + +- `src/config/mod.rs` — added `install_builtins()` free function; + removed 2 calls from `Config::init`. +- `src/main.rs` — added import; added `install_builtins()?` call + after logger setup. + +## Assumptions made + +1. **`install_builtins` should always run unconditionally.** Even + if the user is only running `--completions` or `--tail-logs` + (early-return paths), those return before the install call. + The three config-using paths all benefit from it. No downside to + running it early. + +2. **Module-level `pub fn` is the right API surface.** Could have + made it a method on `AppState` or `AppConfig`, but: + - It's called before any config/state exists + - It has no `self` parameter + - It's a static side-effectful operation (filesystem) + A free function at the module level is the honest signature. + +3. **No tests added.** `install_builtins` is a thin wrapper around + two side-effectful functions that write files. Testing would + require filesystem mocking or temp dirs, which is + disproportionate for a 3-line function. The underlying + `install_builtin_agents` and `install_macros` functions have + existing behavior in the codebase; the extraction doesn't change + their contracts. + +## Open questions + +1. **Should `install_builtins` accept a "skip install" flag?** + Currently it always runs. For a server/REST API deployment, you + might want to skip this to avoid writing to the user's config + dir at startup. Deferring this question until REST API path + exists — can add a flag or a `_skip_install()` variant later. + +2. **Do CI/test environments break because of the filesystem write?** + The install functions already existed in the codebase and ran on + every Config::init. No new risk introduced. Watch for flaky + tests after this change, but expected clean. + +## Verification + +- `cargo check` — clean, zero warnings +- `cargo clippy` — clean, zero warnings +- `cargo test` — 122 passing, zero failures +- Grep confirmation: + - `install_builtin_agents` only defined in `src/config/agent.rs` + and called only via `install_builtins` + - `install_macros` only defined in `src/config/macros.rs` and + called only via `install_builtins` + - `install_builtins` has one caller (`main.rs`) + +## Remaining work for Step 16 + +- **16c**: Migrate `Vault::init(&Config)` → `Vault::init(&AppConfig)`; + eventually move vault ownership to `AppState`. +- **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 16b, `Config::init` no longer handles builtin-asset installation. +This is a small but meaningful piece of responsibility removal — when +`Config::init` is eventually deleted in 16f, we don't need to worry +about orphaning the install logic. + +Startup flow now: +``` +main() + → install_builtins()? [NEW: extracted from Config::init] + → if oauth: Config::init_bare → oauth flow + → if vault flags: Config::init_bare → vault handler + → else: Config::init → to_app_config → AppState → ctx → run +``` + +The `Config::init` calls still do: +- load_envs +- set_wrap +- load_functions +- load_mcp_servers +- setup_model +- setup_document_loaders +- setup_user_agent + +Those move to `AppConfig::from_config` (already built in 16a but +unused) and `AppState::init` (16d) in later sub-phases. diff --git a/src/config/agent.rs b/src/config/agent.rs index edcd733..6f0f302 100644 --- a/src/config/agent.rs +++ b/src/config/agent.rs @@ -5,6 +5,7 @@ use crate::{ function::{Functions, run_llm_function}, }; +use super::rag_cache::RagKey; use crate::config::paths; use crate::config::prompts::{ DEFAULT_SPAWN_INSTRUCTIONS, DEFAULT_TEAMMATE_INSTRUCTIONS, DEFAULT_TODO_INSTRUCTIONS, @@ -17,7 +18,6 @@ use inquire::{Text, validator::Validation}; use rust_embed::Embed; use serde::{Deserialize, Serialize}; use std::{ffi::OsStr, path::Path}; -use super::rag_cache::RagKey; const DEFAULT_AGENT_NAME: &str = "rag"; diff --git a/src/config/app_config.rs b/src/config/app_config.rs index 30cb512..10cbf1e 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::{list_models, ClientConfig}; +use crate::client::{ClientConfig, list_models}; use crate::render::{MarkdownRender, RenderOptions}; use crate::utils::{IS_STDOUT_TERMINAL, NO_COLOR, decode_bin, get_env_name}; diff --git a/src/config/mod.rs b/src/config/mod.rs index a795b45..fb7409b 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -309,6 +309,13 @@ impl Default for Config { } } +pub fn install_builtins() -> Result<()> { + Agent::install_builtin_agents()?; + Macro::install_macros()?; + Functions::install_builtin_global_tools()?; + Ok(()) +} + impl Config { pub fn init_bare() -> Result { let h = Handle::current(); @@ -381,8 +388,6 @@ impl Config { config.info_flag = info_flag; config.vault = Arc::new(vault); - Agent::install_builtin_agents()?; - config.load_envs(); if let Some(wrap) = config.wrap.clone() { @@ -397,7 +402,6 @@ impl Config { config.setup_model()?; config.setup_document_loaders(); config.setup_user_agent(); - Macro::install_macros()?; Ok(()) }; let ret = setup(&mut config).await; diff --git a/src/function/mod.rs b/src/function/mod.rs index 13b4e90..98e31b6 100644 --- a/src/function/mod.rs +++ b/src/function/mod.rs @@ -192,7 +192,7 @@ pub struct Functions { } impl Functions { - fn install_global_tools() -> Result<()> { + pub fn install_builtin_global_tools() -> Result<()> { info!( "Installing global built-in functions in {}", paths::functions_dir().display() @@ -241,7 +241,6 @@ impl Functions { } pub fn init(visible_tools: &[String]) -> Result { - Self::install_global_tools()?; Self::clear_global_functions_bin_dir()?; let declarations = Self { @@ -258,7 +257,6 @@ impl Functions { } pub fn init_agent(name: &str, global_tools: &[String]) -> Result { - Self::install_global_tools()?; Self::clear_agent_bin_dir(name)?; let global_tools_declarations = if !global_tools.is_empty() { diff --git a/src/main.rs b/src/main.rs index d4e9736..3aaaf7e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -22,8 +22,8 @@ use crate::client::{ use crate::config::paths; use crate::config::{ Agent, AppConfig, AppState, CODE_ROLE, Config, EXPLAIN_SHELL_ROLE, Input, RequestContext, - SHELL_ROLE, TEMP_SESSION_NAME, WorkingMode, ensure_parent_exists, list_agents, load_env_file, - macro_execute, + SHELL_ROLE, TEMP_SESSION_NAME, WorkingMode, ensure_parent_exists, install_builtins, + list_agents, load_env_file, macro_execute, }; use crate::render::{prompt_theme, render_error}; use crate::repl::Repl; @@ -82,6 +82,8 @@ async fn main() -> Result<()> { let log_path = setup_logger()?; + 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)?; diff --git a/src/rag/mod.rs b/src/rag/mod.rs index f348464..c09c9c6 100644 --- a/src/rag/mod.rs +++ b/src/rag/mod.rs @@ -103,23 +103,14 @@ impl Rag { Ok(rag) } - pub fn load( - app: &AppConfig, - name: &str, - path: &Path, - ) -> Result { + pub fn load(app: &AppConfig, name: &str, path: &Path) -> Result { let err = || format!("Failed to load rag '{name}' at '{}'", path.display()); let content = fs::read_to_string(path).with_context(err)?; let data: RagData = serde_yaml::from_str(&content).with_context(err)?; Self::create(app, name, path, data) } - pub fn create( - app: &AppConfig, - name: &str, - path: &Path, - data: RagData, - ) -> Result { + pub fn create(app: &AppConfig, name: &str, path: &Path, data: RagData) -> Result { let hnsw = data.build_hnsw(); let bm25 = data.build_bm25(); let embedding_model =