From c11eb352fe7b17214cd759a7106a4020084b5291 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Sun, 19 Apr 2026 18:14:16 -0600 Subject: [PATCH] refactor: migrate functions and MCP servers to AppConfig --- docs/implementation/PHASE-1-STEP-16d-NOTES.md | 271 ++++++++++++++++++ src/config/app_state.rs | 68 ++++- src/config/mod.rs | 4 +- src/config/request_context.rs | 2 + src/config/session.rs | 2 + src/function/supervisor.rs | 2 + src/main.rs | 4 + src/mcp/mod.rs | 10 +- 8 files changed, 355 insertions(+), 8 deletions(-) create mode 100644 docs/implementation/PHASE-1-STEP-16d-NOTES.md diff --git a/docs/implementation/PHASE-1-STEP-16d-NOTES.md b/docs/implementation/PHASE-1-STEP-16d-NOTES.md new file mode 100644 index 0000000..3b4aef5 --- /dev/null +++ b/docs/implementation/PHASE-1-STEP-16d-NOTES.md @@ -0,0 +1,271 @@ +# Phase 1 Step 16d — Implementation Notes + +## Status + +Done. + +## Plan reference + +- Parent plan: `docs/implementation/PHASE-1-STEP-16-NOTES.md` +- Sub-phase goal: "Build `AppState::init(app_config, ...).await` + absorbing MCP registry startup and global functions loading" + +## Summary + +Added `AppState::init()` async constructor that self-initializes all +process-wide shared state from an `Arc` and startup +context. Two new fields on `AppState`: `mcp_registry` (holds initial +MCP server Arcs alive) and `functions` (the global base +`Functions`). Changed `McpRegistry::init` to take `&AppConfig + +&Vault` instead of `&Config`. + +The constructor is dead-code-gated (`#[allow(dead_code)]`) until +Step 16e switches `main.rs` over to call it. The bridge flow +continues to populate the new fields via Config's existing +`functions` and `mcp_registry` so nothing breaks. + +## What was changed + +### `src/mcp/mod.rs` + +**`McpRegistry::init` signature change:** +```rust +// Before +pub async fn init( + log_path: Option, + start_mcp_servers: bool, + enabled_mcp_servers: Option, + abort_signal: AbortSignal, + config: &Config, +) -> Result + +// After +pub async fn init( + log_path: Option, + start_mcp_servers: bool, + enabled_mcp_servers: Option, + abort_signal: AbortSignal, + app_config: &AppConfig, + vault: &Vault, +) -> Result +``` + +The function reads two things from its config argument: +- `config.vault` for secret interpolation → now takes `&Vault` + directly +- `config.mcp_server_support` for the start-servers gate → a serde + field already present on `AppConfig` + +Both dependencies are now explicit. No Config reference anywhere in +the MCP module. + +**Imports updated:** +- Removed `use crate::config::Config` +- Added `use crate::config::AppConfig` +- Added `use crate::vault::Vault` + +### `src/config/mod.rs` + +**`Config::load_mcp_servers` updated** to build a temporary AppConfig +via `self.to_app_config()` and pass it plus `&self.vault` to +`McpRegistry::init`. This is a transitional bridge — disappears in +16e when `main.rs` stops calling `Config::init` and +`AppState::init` becomes the sole entry point. + +### `src/config/app_state.rs` + +**New fields:** +```rust +pub mcp_registry: Option>, +pub functions: Functions, +``` + +**New `AppState::init()` async constructor** absorbs: +- `Vault::init(&config)` (replaces the old + `AppState { vault: cfg.vault.clone() }` pattern) +- `McpRegistry::init(...)` (previously inside `Config::init`) +- Registers initial MCP servers with `McpFactory` via + `insert_active(McpServerKey, &handle)` — this is NEW behavior, see + below. +- `Functions::init(config.visible_tools)` (previously inside + `Config::init`) +- `functions.append_mcp_meta_functions(...)` when MCP support is on + and servers started +- Wraps `McpRegistry` in `Arc` to keep initial server handles alive + across scope transitions (see below) + +**Imports expanded:** +- `McpServerKey` from `super::mcp_factory` +- `Functions` from `crate::function` +- `McpRegistry` from `crate::mcp` +- `AbortSignal` from `crate::utils` +- `Vault` from `crate::vault` +- `anyhow::Result` + +### `src/main.rs` + +**AppState struct literal extended** to populate the two new fields +from `cfg.mcp_registry` and `cfg.functions`. This keeps the bridge +flow working unchanged. When 16e replaces this struct literal with +`AppState::init(...)`, these field references go away entirely. + +### `src/function/supervisor.rs` + +**Child AppState construction extended** to propagate the new +fields from parent: `mcp_registry: ctx.app.mcp_registry.clone()` and +`functions: ctx.app.functions.clone()`. This maintains parent-child +sharing of the MCP factory cache (which was already fixed earlier in +this work stream). + +### `src/config/request_context.rs` and `src/config/session.rs` + +**Test helper AppState construction extended** to include the two +new fields with safe defaults (`None`, and `cfg.functions.clone()` +respectively). + +## New behavior: McpFactory pre-registration + +`AppState::init` registers every initial server with `McpFactory` via +`insert_active`. This fixes a latent issue in the current bridge +flow: + +- Before: initial servers were held on `Config.mcp_registry.servers`; + when the first scope transition (e.g., `.role coder`) ran + `rebuild_tool_scope`, it called `McpFactory::acquire(name, spec, + log_path)` which saw an empty cache and **spawned duplicate + servers**. The original servers died when the initial ToolScope + was replaced. +- After (via `AppState::init`): the factory's Weak map is seeded + with the initial server Arcs. The registry itself is wrapped in + Arc and held on AppState so the Arcs stay alive. Scope + transitions now hit the factory cache and reuse the same + subprocesses. + +This is a real improvement that shows up once `main.rs` switches to +`AppState::init` in 16e. During the 16d bridge window, nothing +reads the factory pre-registration yet. + +## Behavior parity (16d window) + +- `main.rs` still calls `Config::init`, still uses the bridge; all + new fields populated from Config's own state. +- `AppState::init` is present but unused in production code paths. +- Test helpers still use struct literals; they pass `None` for + `mcp_registry` and clone `cfg.functions` which is the same as + what the bridge was doing. +- No observable runtime change for users. + +## Files modified + +- `src/mcp/mod.rs` — `McpRegistry::init` signature; imports. +- `src/config/mod.rs` — `Config::load_mcp_servers` bridges to new + signature. +- `src/config/app_state.rs` — added 2 fields, added `init` + constructor, expanded imports. +- `src/main.rs` — struct literal populates 2 new fields. +- `src/function/supervisor.rs` — child struct literal populates 2 + new fields. +- `src/config/request_context.rs` — test helper populates 2 new + fields. +- `src/config/session.rs` — test helper populates 2 new fields. + +## Assumptions made + +1. **`McpFactory::insert_active` with Weak is sufficient to seed the + cache.** The initial ServerArcs live on `AppState.mcp_registry` + (wrapped in Arc to enable clone across child states). Scope + transitions call `McpFactory::acquire` which does + `try_get_active(key).unwrap_or_else(spawn_new)`. The Weak in + factory upgrades because Arc is alive in `mcp_registry`. Verified + by reading code paths; not yet verified at runtime since bridge + still drives today's flow. + +2. **`functions: Functions` is Clone-safe.** The struct contains + `Vec` and related fields; cloning is cheap + enough at startup and child-agent spawn. Inspected the + definition; no references to check. + +3. **`mcp_server_support` gate still applies.** `Config::init` used + to gate the MCP meta function append with both `is_empty()` and + `mcp_server_support`; `AppState::init` preserves both checks. + Parity confirmed. + +4. **Mode-specific function additions (REPL-only + `append_user_interaction_functions`) do NOT live on AppState.** + They are added per-scope in `rebuild_tool_scope` and in the + initial `RequestContext::new` path. `AppState.functions` is the + mode-agnostic base. This matches the long-term design + (RequestContext owns mode-aware additions). + +5. **`mcp_registry: Option>` vs `McpRegistry`.** + Went with `Option>` so: + - `None` when no MCP config exists (can skip work) + - `Arc` so parent/child AppStates share the same registry + (keeping initial server handles alive across the tree) + +## Open questions + +1. **Registry on AppState vs factory-owned lifecycle**. The factory + holds Weak; the registry holds Arc. Keeping the registry alive + on AppState extends server lifetime to the process lifetime. + This differs from the current "servers die on scope transition" + behavior. In practice this is what users expect — start the + servers once, keep them alive. But it means long-running REPL + sessions retain all server subprocesses even if the user switches + away from them. Acceptable trade-off for Phase 1. + +2. **Should `AppState::init` return `Arc` directly?** + Currently returns `Self`. Caller wraps in Arc. Symmetric with + other init functions; caller has full flexibility. Keep as-is. + +3. **Unit tests for `AppState::init`.** Didn't add any because the + function is heavily async, touches filesystem (paths), + subprocess startup (MCP), and the vault. A meaningful unit test + would require mocking. Integration-level validation happens in + 16e when main.rs switches over. Deferred. + +## Verification + +- `cargo check` — clean, zero warnings +- `cargo clippy` — clean, zero warnings +- `cargo test` — 122 passing, zero failures +- New `AppState::init` gated with `#[allow(dead_code)]` — no + warnings for being unused + +## Remaining work for Step 16 + +- **16e**: Switch `main.rs` to call + `AppState::init(app_config, log_path, start_mcp_servers, + abort_signal).await?` instead of the bridge pattern. Audit the + 15 `Config::init()` callers. Remove `#[allow(dead_code)]` from + `AppConfig::from_config`, `AppConfig::resolve_model`, and + `AppState::init`. +- **16f**: Delete `Config.vault`, `Config.functions`, + `Config.mcp_registry`, all other `#[serde(skip)]` runtime fields. + Delete `Config::init`, `Config::load_envs`, `Config::load_functions`, + `Config::load_mcp_servers`, `Config::setup_model`, + `Config::set_model`, etc. Delete `bridge.rs`. + +## Migration direction preserved + +Before 16d: +``` +AppState { + config, vault, mcp_factory, rag_cache, mcp_config, mcp_log_path +} +``` +Constructed only via struct literal from Config fields via bridge. + +After 16d: +``` +AppState { + config, vault, mcp_factory, rag_cache, mcp_config, mcp_log_path, + mcp_registry, functions +} +impl AppState { + pub async fn init(config, log_path, start_mcp_servers, abort_signal) + -> Result +} +``` +New fields present on all code paths. New self-initializing +constructor ready for 16e's switchover. diff --git a/src/config/app_state.rs b/src/config/app_state.rs index b86ecaf..6118dbc 100644 --- a/src/config/app_state.rs +++ b/src/config/app_state.rs @@ -28,12 +28,15 @@ //! additive scaffolding that Step 8+ will connect when the entry //! points migrate. See `docs/PHASE-1-IMPLEMENTATION-PLAN.md`. -use super::mcp_factory::McpFactory; +use super::mcp_factory::{McpFactory, McpServerKey}; use super::rag_cache::RagCache; use crate::config::AppConfig; -use crate::mcp::McpServersConfig; -use crate::vault::GlobalVault; +use crate::function::Functions; +use crate::mcp::{McpRegistry, McpServersConfig}; +use crate::utils::AbortSignal; +use crate::vault::{GlobalVault, Vault}; +use anyhow::Result; use std::path::PathBuf; use std::sync::Arc; @@ -46,4 +49,63 @@ pub struct AppState { pub rag_cache: Arc, pub mcp_config: Option, pub mcp_log_path: Option, + pub mcp_registry: Option>, + pub functions: Functions, +} + +impl AppState { + #[allow(dead_code)] + pub async fn init( + config: Arc, + log_path: Option, + start_mcp_servers: bool, + abort_signal: AbortSignal, + ) -> Result { + let vault = Arc::new(Vault::init(&config)); + + let mcp_registry = McpRegistry::init( + log_path, + start_mcp_servers, + config.enabled_mcp_servers.clone(), + abort_signal, + &config, + &vault, + ) + .await?; + + let mcp_config = mcp_registry.mcp_config().cloned(); + let mcp_log_path = mcp_registry.log_path().cloned(); + + let mcp_factory = Arc::new(McpFactory::default()); + if let Some(mcp_servers_config) = &mcp_config { + for (id, handle) in mcp_registry.running_servers() { + if let Some(spec) = mcp_servers_config.mcp_servers.get(id) { + let key = McpServerKey::from_spec(id, spec); + mcp_factory.insert_active(key, handle); + } + } + } + + let mut functions = Functions::init(config.visible_tools.as_ref().unwrap_or(&Vec::new()))?; + if !mcp_registry.is_empty() && config.mcp_server_support { + functions.append_mcp_meta_functions(mcp_registry.list_started_servers()); + } + + let mcp_registry = if mcp_registry.is_empty() { + None + } else { + Some(Arc::new(mcp_registry)) + }; + + Ok(Self { + config, + vault, + mcp_factory, + rag_cache: Arc::new(RagCache::default()), + mcp_config, + mcp_log_path, + mcp_registry, + functions, + }) + } } diff --git a/src/config/mod.rs b/src/config/mod.rs index 1d5d70f..cb4dca4 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -729,12 +729,14 @@ impl Config { start_mcp_servers: bool, abort_signal: AbortSignal, ) -> Result<()> { + let app_config = self.to_app_config(); let mcp_registry = McpRegistry::init( log_path, start_mcp_servers, self.enabled_mcp_servers.clone(), abort_signal.clone(), - self, + &app_config, + &self.vault, ) .await?; match mcp_registry.is_empty() { diff --git a/src/config/request_context.rs b/src/config/request_context.rs index 62c5457..b4fa0f5 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -2408,6 +2408,8 @@ mod tests { rag_cache: Arc::new(RagCache::default()), mcp_config: None, mcp_log_path: None, + mcp_registry: None, + functions: cfg.functions.clone(), }) } diff --git a/src/config/session.rs b/src/config/session.rs index 1df54ce..edd3e8e 100644 --- a/src/config/session.rs +++ b/src/config/session.rs @@ -697,6 +697,8 @@ mod tests { rag_cache: Arc::new(rag_cache::RagCache::default()), mcp_config: None, mcp_log_path: None, + mcp_registry: None, + functions: cfg.functions.clone(), }); let ctx = cfg.to_request_context(app_state); let session = Session::new_from_ctx(&ctx, &app_config, "test-session"); diff --git a/src/function/supervisor.rs b/src/function/supervisor.rs index 790344b..741257a 100644 --- a/src/function/supervisor.rs +++ b/src/function/supervisor.rs @@ -486,6 +486,8 @@ async fn handle_spawn(ctx: &mut RequestContext, args: &Value) -> Result { rag_cache: ctx.app.rag_cache.clone(), mcp_config: ctx.app.mcp_config.clone(), mcp_log_path: ctx.app.mcp_log_path.clone(), + mcp_registry: ctx.app.mcp_registry.clone(), + functions: ctx.app.functions.clone(), }); let agent = Agent::init( app_config.as_ref(), diff --git a/src/main.rs b/src/main.rs index 68ddfce..0662b3a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -111,6 +111,8 @@ async fn main() -> Result<()> { 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(), @@ -118,6 +120,8 @@ async fn main() -> Result<()> { rag_cache: Default::default(), mcp_config, mcp_log_path, + mcp_registry, + functions, }); let ctx = cfg.to_request_context(app_state); diff --git a/src/mcp/mod.rs b/src/mcp/mod.rs index 1810e41..6dc22c7 100644 --- a/src/mcp/mod.rs +++ b/src/mcp/mod.rs @@ -1,6 +1,7 @@ -use crate::config::Config; +use crate::config::AppConfig; use crate::config::paths; use crate::utils::{AbortSignal, abortable_run_with_spinner}; +use crate::vault::Vault; use crate::vault::interpolate_secrets; use anyhow::{Context, Result, anyhow}; use futures_util::{StreamExt, TryStreamExt, stream}; @@ -78,7 +79,8 @@ impl McpRegistry { start_mcp_servers: bool, enabled_mcp_servers: Option, abort_signal: AbortSignal, - config: &Config, + app_config: &AppConfig, + vault: &Vault, ) -> Result { let mut registry = Self { log_path, @@ -111,7 +113,7 @@ impl McpRegistry { return Ok(registry); } - let (parsed_content, missing_secrets) = interpolate_secrets(&content, &config.vault); + let (parsed_content, missing_secrets) = interpolate_secrets(&content, vault); if !missing_secrets.is_empty() { return Err(anyhow!(formatdoc!( @@ -126,7 +128,7 @@ impl McpRegistry { serde_json::from_str(&parsed_content).with_context(err)?; registry.config = Some(mcp_servers_config); - if start_mcp_servers && config.mcp_server_support { + if start_mcp_servers && app_config.mcp_server_support { abortable_run_with_spinner( registry.start_select_mcp_servers(enabled_mcp_servers), "Loading MCP servers",