From 6e07f688ab3d63c2da2c21e3fa22d04fe592cce7 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 22 Apr 2026 00:16:49 -0400 Subject: [PATCH] refactor(cli): collapse duplicate server-settings resolvers through local_server Route install/uninstall through local_server::storage_dir instead of hand- rolled copies, drop dead connect_api_client and run_dir plumbing, eliminate double-resolve in prepare_server_bootstrap, and tighten the boundary allowlist now that uninstall no longer needs the exemption. Co-Authored-By: Claude Opus 4.7 (1M context) --- bin/dev/check-boundary.sh | 1 - lib/crates/fabro-cli/src/commands/install.rs | 28 +++---------------- .../fabro-cli/src/commands/pr/create.rs | 2 -- lib/crates/fabro-cli/src/commands/pr/mod.rs | 4 --- .../fabro-cli/src/commands/run/output.rs | 16 +---------- .../fabro-cli/src/commands/uninstall.rs | 25 ++++------------- lib/crates/fabro-cli/src/local_server.rs | 5 ---- lib/crates/fabro-cli/src/main.rs | 2 +- lib/crates/fabro-cli/src/server_client.rs | 10 ------- 9 files changed, 11 insertions(+), 82 deletions(-) diff --git a/bin/dev/check-boundary.sh b/bin/dev/check-boundary.sh index 6842a5aea..d1cfb3f50 100755 --- a/bin/dev/check-boundary.sh +++ b/bin/dev/check-boundary.sh @@ -6,7 +6,6 @@ cd "$(dirname "$0")/../.." symbol_allowlist=( "lib/crates/fabro-cli/src/local_server.rs" "lib/crates/fabro-cli/src/commands/install.rs" - "lib/crates/fabro-cli/src/commands/uninstall.rs" "lib/crates/fabro-cli/src/commands/run/runner.rs" "lib/crates/fabro-cli/src/commands/pr/mod.rs" "lib/crates/fabro-cli/src/commands/pr/create.rs" diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index 18d9e43fd..776756bdd 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -5,7 +5,7 @@ use std::future::Future; use std::net::SocketAddr; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::process::Stdio; use std::time::Duration; @@ -57,7 +57,7 @@ use crate::shared::provider_auth::{ ApiKeySource, authenticate_provider, authenticate_provider_with_api_key_source, authenticate_provider_with_method, prompt_confirm, prompt_password, provider_display_name, }; -use crate::{server_client, user_config}; +use crate::{local_server, server_client, user_config}; const GITHUB_TOKEN_SECRET_KEY: &str = "GITHUB_TOKEN"; const GITHUB_APP_PRIVATE_KEY_KEY: &str = "GITHUB_APP_PRIVATE_KEY"; @@ -1292,22 +1292,6 @@ fn render_server_resolve_errors(errors: Vec) -> anyhow::Error { ) } -fn resolved_server_storage_dir(settings: &SettingsLayer) -> Result { - let resolved = - fabro_config::resolve_server_from_file(settings).map_err(render_server_resolve_errors)?; - let resolved_root = resolved - .storage - .root - .resolve(|name| std::env::var(name).ok()) - .map_err(|err| { - anyhow::anyhow!( - "failed to resolve {}: {err}", - resolved.storage.root.as_source() - ) - })?; - Ok(PathBuf::from(resolved_root.value)) -} - async fn write_artifact_store_metadata( settings: &SettingsLayer, fabro_version: &str, @@ -1510,9 +1494,7 @@ async fn run_install_github_inner( .context("failed to parse existing settings.toml")?, args.storage_dir.as_deref(), ); - // Install is a documented server-host exception: it may read [server.*] - // directly. - let storage_dir = resolved_server_storage_dir(&parsed_settings).unwrap_or_else(|_| { + let storage_dir = local_server::storage_dir(&parsed_settings).unwrap_or_else(|_| { args.storage_dir .clone_path() .unwrap_or_else(default_storage_dir) @@ -1659,9 +1641,7 @@ async fn run_install_inner( let s = Styles::detect_stderr(); let emoji = console::Emoji("⚒️ ", ""); let cli_settings = user_config::load_settings_with_storage_dir(args.storage_dir.as_deref())?; - // Install is a documented server-host exception: it may read [server.*] - // directly. - let storage_dir = resolved_server_storage_dir(&cli_settings)?; + let storage_dir = local_server::storage_dir(&cli_settings)?; let server_was_running = record::active_server_record(&storage_dir)?.is_some(); let fabro_dir = fabro_util::Home::from_env().root().to_path_buf(); let config_path = fabro_dir.join(SETTINGS_CONFIG_FILENAME); diff --git a/lib/crates/fabro-cli/src/commands/pr/create.rs b/lib/crates/fabro-cli/src/commands/pr/create.rs index 78d9703ad..3e269ba79 100644 --- a/lib/crates/fabro-cli/src/commands/pr/create.rs +++ b/lib/crates/fabro-cli/src/commands/pr/create.rs @@ -105,8 +105,6 @@ pub(super) async fn create_command( ); } - // boundary-exempt(pr-api): remove with follow-up #1 when PR ops move - // server-side. let vault = user_config::storage_dir(ctx.machine_settings()) .ok() .and_then(|dir| Vault::load(Storage::new(&dir).secrets_path()).ok()) diff --git a/lib/crates/fabro-cli/src/commands/pr/mod.rs b/lib/crates/fabro-cli/src/commands/pr/mod.rs index 84b6a0b57..bf7a3671b 100644 --- a/lib/crates/fabro-cli/src/commands/pr/mod.rs +++ b/lib/crates/fabro-cli/src/commands/pr/mod.rs @@ -47,8 +47,6 @@ fn load_github_credentials_required( printer: Printer, ) -> Result { let ctx = CommandContext::base(printer, cli.clone(), cli_layer)?; - // boundary-exempt(pr-api): remove with follow-up #1 when PR ops move - // server-side. let server_settings = fabro_config::resolve_server_from_file(ctx.machine_settings()).map_err(|errors| { anyhow!( @@ -60,8 +58,6 @@ fn load_github_credentials_required( .join("\n") ) })?; - // boundary-exempt(pr-api): remove with follow-up #1 when PR ops move - // server-side. let vault = user_config::storage_dir(ctx.machine_settings()) .ok() .and_then(|dir| fabro_vault::Vault::load(Storage::new(&dir).secrets_path()).ok()); diff --git a/lib/crates/fabro-cli/src/commands/run/output.rs b/lib/crates/fabro-cli/src/commands/run/output.rs index d44cbd3ba..6880fcbcb 100644 --- a/lib/crates/fabro-cli/src/commands/run/output.rs +++ b/lib/crates/fabro-cli/src/commands/run/output.rs @@ -17,9 +17,7 @@ use fabro_workflow::records::Conclusion; use indicatif::HumanDuration; use crate::server_client; -use crate::shared::{ - format_tokens_human, format_usd_micros, print_diagnostics, relative_path, tilde_path, -}; +use crate::shared::{format_tokens_human, format_usd_micros, print_diagnostics, relative_path}; pub(crate) fn print_preflight_workflow_summary( workflow: &types::PreflightWorkflowSummary, @@ -151,7 +149,6 @@ pub(crate) async fn print_run_summary_with_client( &conclusion, run_id, None, - None, pr_url.as_deref(), styles, printer, @@ -166,7 +163,6 @@ pub(crate) async fn print_run_summary_with_client( pub(crate) fn print_run_conclusion( conclusion: &Conclusion, run_id: impl std::fmt::Display, - run_dir: Option<&Path>, pushed_branch: Option<&str>, pr_url: Option<&str>, styles: &Styles, @@ -248,16 +244,6 @@ pub(crate) fn print_run_conclusion( } } - if let Some(run_dir) = run_dir { - fabro_util::printerr!( - printer, - "{}", - styles - .dim - .apply_to(format!("Run: {}", tilde_path(run_dir))) - ); - } - if let Some(ref failure) = conclusion.failure_reason { fabro_util::printerr!(printer, "Failure: {}", styles.red.apply_to(failure)); } diff --git a/lib/crates/fabro-cli/src/commands/uninstall.rs b/lib/crates/fabro-cli/src/commands/uninstall.rs index 52a4b25e6..3a5be57d5 100644 --- a/lib/crates/fabro-cli/src/commands/uninstall.rs +++ b/lib/crates/fabro-cli/src/commands/uninstall.rs @@ -23,7 +23,7 @@ use tracing::warn; use crate::args::UninstallArgs; use crate::commands::server; use crate::shared::{format_size, print_json_pretty, tilde_path}; -use crate::user_config; +use crate::{local_server, user_config}; #[derive(Debug, Serialize)] struct Inventory { @@ -59,25 +59,10 @@ pub(crate) async fn run_uninstall( return Ok(()); } - let storage_dir = user_config::load_settings().map_or_else( - |_| user_config::default_storage_dir(), - |settings| { - // Uninstall is a documented server-host exception: it may read - // [server.*] directly. - fabro_config::resolve_server_from_file(&settings) - .ok() - .and_then(|resolved| { - resolved - .storage - .root - .resolve(|name| std::env::var(name).ok()) - .ok() - }) - .map_or_else(user_config::default_storage_dir, |resolved_root| { - PathBuf::from(resolved_root.value) - }) - }, - ); + let storage_dir = user_config::load_settings() + .ok() + .and_then(|settings| local_server::storage_dir(&settings).ok()) + .unwrap_or_else(user_config::default_storage_dir); let inventory = build_inventory(&home_root, &storage_dir)?; diff --git a/lib/crates/fabro-cli/src/local_server.rs b/lib/crates/fabro-cli/src/local_server.rs index 1c087c382..bef5b9617 100644 --- a/lib/crates/fabro-cli/src/local_server.rs +++ b/lib/crates/fabro-cli/src/local_server.rs @@ -7,7 +7,6 @@ use std::path::PathBuf; use anyhow::Result; -use fabro_config::ServerRuntimeState; use fabro_server::bind::BindRequest; use fabro_server::serve::resolve_bind_request_from_settings; use fabro_types::settings::{ServerAuthMethod, SettingsLayer}; @@ -39,10 +38,6 @@ pub(crate) fn storage_dir(settings: &SettingsLayer) -> Result { Ok(PathBuf::from(resolved_root.value)) } -pub(crate) fn runtime_state(settings: &SettingsLayer) -> Result { - Ok(ServerRuntimeState::new(storage_dir(settings)?)) -} - pub(crate) fn bind_request( settings: &SettingsLayer, cli_override: Option<&str>, diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index 017e16f6e..df2e44f49 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -481,7 +481,7 @@ async fn prepare_server_bootstrap( let settings = user_config::load_settings_with_config_and_storage_dir(config_path, storage_dir)?; let storage_dir = local_server::storage_dir(&settings)?; - let runtime_state = local_server::runtime_state(&settings)?; + let runtime_state = fabro_config::ServerRuntimeState::new(storage_dir.clone()); let foreground_server_log_bootstrap = if foreground { Some(commands::server::start::prepare_foreground_server_log(&storage_dir).await?) } else { diff --git a/lib/crates/fabro-cli/src/server_client.rs b/lib/crates/fabro-cli/src/server_client.rs index 9fbccdf8e..182dcad23 100644 --- a/lib/crates/fabro-cli/src/server_client.rs +++ b/lib/crates/fabro-cli/src/server_client.rs @@ -140,16 +140,6 @@ async fn connect_local_api_client_bundle( } } -#[allow( - dead_code, - reason = "Retained for pending storage-backed internal callers and referenced in existing design docs." -)] -pub(crate) async fn connect_api_client(storage_dir: &Path) -> Result { - connect_local_api_client_bundle(storage_dir, &user_config::active_settings_path(None)) - .await - .map(|client| client.api_client()) -} - async fn connect_target_api_client_bundle(target: &ServerTarget) -> Result { let credential = resolve_target_credential(target, None, local_dev_token_fallback(target))?; let oauth_session = refreshable_oauth(target, credential.as_ref());