From cb58b18385861a674872581f61076ea6087df518 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 6 Apr 2026 11:57:14 -0400 Subject: [PATCH] refactor(cli): separate local socket and storage defaults Keep local server targeting based on explicit server targets instead of implicitly deriving a socket from storage_dir. This makes ~/.fabro/fabro.sock the default local socket again, keeps storage under ~/.fabro/storage, threads FABRO_CONFIG through server autostart paths, and updates the CLI test harness for the new split. --- lib/crates/fabro-cli/src/args.rs | 50 +---- lib/crates/fabro-cli/src/commands/doctor.rs | 4 +- lib/crates/fabro-cli/src/commands/graph.rs | 5 +- lib/crates/fabro-cli/src/commands/install.rs | 8 +- lib/crates/fabro-cli/src/commands/model.rs | 5 +- .../fabro-cli/src/commands/preflight.rs | 7 +- .../fabro-cli/src/commands/repo/init.rs | 4 +- .../fabro-cli/src/commands/run/command.rs | 6 +- .../fabro-cli/src/commands/run/create.rs | 19 +- lib/crates/fabro-cli/src/commands/run/mod.rs | 2 +- .../fabro-cli/src/commands/server/mod.rs | 20 +- .../fabro-cli/src/commands/server/record.rs | 39 +++- .../fabro-cli/src/commands/server/start.rs | 39 +++- .../fabro-cli/src/commands/server/stop.rs | 7 +- lib/crates/fabro-cli/src/commands/validate.rs | 5 +- lib/crates/fabro-cli/src/main.rs | 134 +----------- lib/crates/fabro-cli/src/manifest_builder.rs | 4 +- lib/crates/fabro-cli/src/server_client.rs | 79 +++++--- lib/crates/fabro-cli/src/user_config.rs | 190 +++++------------- lib/crates/fabro-cli/tests/it/cmd/config.rs | 2 +- lib/crates/fabro-cli/tests/it/cmd/create.rs | 66 +++--- lib/crates/fabro-cli/tests/it/cmd/doctor.rs | 15 +- lib/crates/fabro-cli/tests/it/cmd/graph.rs | 21 +- lib/crates/fabro-cli/tests/it/cmd/install.rs | 2 +- lib/crates/fabro-cli/tests/it/cmd/model.rs | 62 +++++- .../fabro-cli/tests/it/cmd/model_list.rs | 19 +- .../fabro-cli/tests/it/cmd/model_test.rs | 21 +- lib/crates/fabro-cli/tests/it/cmd/pr_view.rs | 33 ++- .../fabro-cli/tests/it/cmd/preflight.rs | 25 ++- .../fabro-cli/tests/it/cmd/provider_login.rs | 17 +- lib/crates/fabro-cli/tests/it/cmd/repo.rs | 15 +- .../fabro-cli/tests/it/cmd/repo_init.rs | 15 +- lib/crates/fabro-cli/tests/it/cmd/run.rs | 70 +++---- lib/crates/fabro-cli/tests/it/cmd/runner.rs | 2 +- lib/crates/fabro-cli/tests/it/cmd/secret.rs | 15 +- .../fabro-cli/tests/it/cmd/server_start.rs | 70 +++++-- .../fabro-cli/tests/it/cmd/server_status.rs | 2 +- .../fabro-cli/tests/it/cmd/server_stop.rs | 2 +- .../fabro-cli/tests/it/cmd/store_dump.rs | 2 +- lib/crates/fabro-cli/tests/it/cmd/support.rs | 51 +++-- .../fabro-cli/tests/it/cmd/system_df.rs | 2 +- .../fabro-cli/tests/it/cmd/system_prune.rs | 2 +- lib/crates/fabro-cli/tests/it/cmd/validate.rs | 15 +- lib/crates/fabro-cli/tests/it/scenario/mod.rs | 41 +++- lib/crates/fabro-cli/tests/it/workflow/mod.rs | 41 +++- lib/crates/fabro-config/src/user.rs | 65 +++++- lib/crates/fabro-server/src/serve.rs | 6 +- lib/crates/fabro-test/src/lib.rs | 26 ++- lib/crates/fabro-types/src/settings/mod.rs | 20 +- 49 files changed, 721 insertions(+), 651 deletions(-) diff --git a/lib/crates/fabro-cli/src/args.rs b/lib/crates/fabro-cli/src/args.rs index 002bbb4ee..0218496df 100644 --- a/lib/crates/fabro-cli/src/args.rs +++ b/lib/crates/fabro-cli/src/args.rs @@ -46,7 +46,7 @@ impl GlobalArgs { #[derive(Args, Debug, Clone, Default)] pub(crate) struct StorageDirArgs { - /// Local storage directory (default: ~/.fabro) + /// Local storage directory (default: ~/.fabro/storage) #[arg(long, env = "FABRO_STORAGE_DIR")] pub(crate) storage_dir: Option, } @@ -74,34 +74,6 @@ impl ServerTargetArgs { } } -#[derive(Args, Debug, Clone, Default)] -pub(crate) struct ServerConnectionArgs { - /// Local storage directory (default: ~/.fabro) - #[arg(long, env = "FABRO_STORAGE_DIR")] - pub(crate) storage_dir: Option, - - /// Fabro server target: http(s) URL or absolute Unix socket path - #[arg(long = "server", env = "FABRO_SERVER")] - pub(crate) server: Option, - - #[arg(skip)] - pub(crate) storage_dir_explicit: bool, -} - -impl ServerConnectionArgs { - pub(crate) fn storage_dir(&self) -> Option<&Path> { - self.storage_dir.as_deref() - } - - pub(crate) fn server(&self) -> Option<&str> { - self.server.as_deref() - } - - pub(crate) fn storage_dir_is_explicit(&self) -> bool { - self.storage_dir_explicit - } -} - #[derive(Debug, Clone, Copy, ValueEnum)] pub(crate) enum CliSandboxProvider { Local, @@ -132,7 +104,7 @@ impl From for CliSandboxProvider { #[derive(Args)] pub(crate) struct RunArgs { #[command(flatten)] - pub(crate) target: ServerConnectionArgs, + pub(crate) target: ServerTargetArgs, /// Path to a .fabro workflow file or .toml task config #[arg(required = true)] @@ -194,7 +166,7 @@ pub(crate) struct RunArgs { #[derive(Args)] pub(crate) struct PreflightArgs { #[command(flatten)] - pub(crate) target: ServerConnectionArgs, + pub(crate) target: ServerTargetArgs, /// Path to a .fabro workflow file or .toml task config pub(crate) workflow: PathBuf, @@ -298,7 +270,7 @@ pub(crate) struct LogsArgs { #[derive(Args)] pub(crate) struct ValidateArgs { #[command(flatten)] - pub(crate) target: ServerConnectionArgs, + pub(crate) target: ServerTargetArgs, /// Path to the .fabro workflow file pub(crate) workflow: PathBuf, @@ -348,7 +320,7 @@ impl fmt::Display for GraphOutputFormat { #[derive(Args)] pub(crate) struct GraphArgs { #[command(flatten)] - pub(crate) target: ServerConnectionArgs, + pub(crate) target: ServerTargetArgs, /// Path to the .fabro workflow file, .toml task config, or project workflow name pub(crate) workflow: PathBuf, @@ -600,7 +572,7 @@ pub(crate) struct WorkflowCreateArgs { #[derive(Args)] pub(crate) struct ProviderLoginArgs { #[command(flatten)] - pub(crate) target: ServerConnectionArgs, + pub(crate) target: ServerTargetArgs, /// LLM provider to authenticate with #[arg(long)] @@ -760,7 +732,7 @@ pub(crate) struct RunnerArgs { #[derive(Args, Debug, Clone, Default)] pub(crate) struct ModelListArgs { #[command(flatten)] - pub(crate) target: ServerConnectionArgs, + pub(crate) target: ServerTargetArgs, /// Filter by provider #[arg(short, long)] @@ -774,7 +746,7 @@ pub(crate) struct ModelListArgs { #[derive(Args, Debug, Clone, Default)] pub(crate) struct ModelTestArgs { #[command(flatten)] - pub(crate) target: ServerConnectionArgs, + pub(crate) target: ServerTargetArgs, /// Filter by provider #[arg(short, long)] @@ -1116,7 +1088,7 @@ pub(crate) enum StoreCommand { #[derive(Args)] pub(crate) struct SecretNamespace { #[command(flatten)] - pub(crate) target: ServerConnectionArgs, + pub(crate) target: ServerTargetArgs, #[command(subcommand)] pub(crate) command: SecretCommand, @@ -1245,7 +1217,7 @@ pub(crate) enum RepoCommand { #[derive(Args)] pub(crate) struct RepoInitArgs { #[command(flatten)] - pub(crate) target: ServerConnectionArgs, + pub(crate) target: ServerTargetArgs, /// Also install the fabro-create-workflow skill #[arg(long, hide = true)] @@ -1255,7 +1227,7 @@ pub(crate) struct RepoInitArgs { #[derive(Args)] pub(crate) struct DoctorArgs { #[command(flatten)] - pub(crate) target: ServerConnectionArgs, + pub(crate) target: ServerTargetArgs, /// Show detailed information for each check #[arg(short, long)] diff --git a/lib/crates/fabro-cli/src/commands/doctor.rs b/lib/crates/fabro-cli/src/commands/doctor.rs index b82263fc6..c50b8bc9f 100644 --- a/lib/crates/fabro-cli/src/commands/doctor.rs +++ b/lib/crates/fabro-cli/src/commands/doctor.rs @@ -6,7 +6,7 @@ use anyhow::Result; use fabro_api::types as api_types; use fabro_config::legacy_env; use fabro_config::user::{ - default_settings_path, legacy_old_user_config_path, legacy_server_config_path, + active_settings_path, legacy_old_user_config_path, legacy_server_config_path, legacy_user_config_path, }; pub(crate) use fabro_util::check_report::{ @@ -314,7 +314,7 @@ pub(crate) async fn run_doctor( Some(spinner) }; - let settings_config_path = default_settings_path(); + let settings_config_path = active_settings_path(None); let legacy_config_paths = [ legacy_user_config_path(), legacy_old_user_config_path(), diff --git a/lib/crates/fabro-cli/src/commands/graph.rs b/lib/crates/fabro-cli/src/commands/graph.rs index b6492ff61..0ad4f70db 100644 --- a/lib/crates/fabro-cli/src/commands/graph.rs +++ b/lib/crates/fabro-cli/src/commands/graph.rs @@ -11,7 +11,6 @@ use crate::commands::run::output::api_diagnostics_to_local; use crate::manifest_builder::{ManifestBuildInput, build_run_manifest}; use crate::server_client; use crate::shared::{absolute_or_current, print_diagnostics, print_json_pretty, relative_path}; -use crate::user_config::{self, load_settings_with_storage_dir}; pub(crate) async fn run( args: &GraphArgs, @@ -22,8 +21,6 @@ pub(crate) async fn run( globals.require_no_json()?; } - let settings = load_settings_with_storage_dir(args.target.storage_dir())?; - let connection = user_config::server_backed_command_connection(&args.target, &settings)?; let cwd = std::env::current_dir()?; let built = build_run_manifest(ManifestBuildInput { workflow: args.workflow.clone(), @@ -32,7 +29,7 @@ pub(crate) async fn run( args: None, run_id: None, })?; - let client = server_client::connect_server_connection(&connection).await?; + let client = server_client::connect_server_only(&args.target).await?; let preflight = client.run_preflight(built.manifest.clone()).await?; let diagnostics = api_diagnostics_to_local(&preflight.workflow.diagnostics); diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index ee7f775a3..bcd04be33 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -24,7 +24,7 @@ use tokio::sync::oneshot; use tokio::task::spawn_blocking; use super::doctor; -use crate::args::{DoctorArgs, GlobalArgs, InstallArgs, ServerConnectionArgs}; +use crate::args::{DoctorArgs, GlobalArgs, InstallArgs, ServerTargetArgs}; use crate::commands::server::record; use crate::server_client; use crate::shared::provider_auth::{ @@ -856,11 +856,7 @@ pub(crate) async fn run_install(args: &InstallArgs, globals: &GlobalArgs) -> Res if run_doctor { eprintln!(); let doctor_args = DoctorArgs { - target: ServerConnectionArgs { - storage_dir: Some(storage_dir.clone()), - server: None, - storage_dir_explicit: true, - }, + target: ServerTargetArgs::default(), verbose: true, }; let _ = doctor::run_doctor(&doctor_args, true, globals).await?; diff --git a/lib/crates/fabro-cli/src/commands/model.rs b/lib/crates/fabro-cli/src/commands/model.rs index fde213d1e..08a347abc 100644 --- a/lib/crates/fabro-cli/src/commands/model.rs +++ b/lib/crates/fabro-cli/src/commands/model.rs @@ -9,7 +9,6 @@ use serde::de::DeserializeOwned; use crate::args::{GlobalArgs, ModelListArgs, ModelTestArgs, ModelsCommand}; use crate::server_client; -use crate::user_config; #[derive(Serialize)] #[serde(rename_all = "snake_case")] @@ -43,9 +42,7 @@ pub(crate) async fn execute(command: Option, globals: &GlobalArgs ModelsCommand::List(args) => &args.target, ModelsCommand::Test(args) => &args.target, }; - let cli_settings = user_config::load_settings_with_storage_dir(target_args.storage_dir())?; - let connection = user_config::model_server_connection(target_args, &cli_settings)?; - let client = server_client::connect_resolved_api_client(&connection).await?; + let client = server_client::connect_server_backed_api_client(target_args).await?; run_models(command, client, globals.json).await } diff --git a/lib/crates/fabro-cli/src/commands/preflight.rs b/lib/crates/fabro-cli/src/commands/preflight.rs index bea33d589..55ba46e1c 100644 --- a/lib/crates/fabro-cli/src/commands/preflight.rs +++ b/lib/crates/fabro-cli/src/commands/preflight.rs @@ -9,13 +9,12 @@ use crate::commands::run::output::{ use crate::manifest_builder::{ManifestBuildInput, build_run_manifest, preflight_manifest_args}; use crate::server_client; use crate::shared::print_json_pretty; -use crate::user_config::{self, load_settings_with_storage_dir}; +use crate::user_config; pub(crate) async fn execute(mut args: PreflightArgs, globals: &GlobalArgs) -> anyhow::Result<()> { let styles: &'static Styles = Box::leak(Box::new(Styles::detect_stderr())); - let cli_settings = load_settings_with_storage_dir(args.target.storage_dir())?; + let cli_settings = user_config::load_settings()?; args.verbose = args.verbose || cli_settings.verbose_enabled(); - let connection = user_config::server_backed_command_connection(&args.target, &cli_settings)?; let cwd = std::env::current_dir()?; let manifest = build_run_manifest(ManifestBuildInput { @@ -25,7 +24,7 @@ pub(crate) async fn execute(mut args: PreflightArgs, globals: &GlobalArgs) -> an args: preflight_manifest_args(&args), run_id: None, })?; - let client = server_client::connect_server_connection(&connection).await?; + let client = server_client::connect_server_only(&args.target).await?; let response = client.run_preflight(manifest.manifest).await?; let diagnostics = api_diagnostics_to_local(&response.workflow.diagnostics); diff --git a/lib/crates/fabro-cli/src/commands/repo/init.rs b/lib/crates/fabro-cli/src/commands/repo/init.rs index 3997ebca7..1023668c9 100644 --- a/lib/crates/fabro-cli/src/commands/repo/init.rs +++ b/lib/crates/fabro-cli/src/commands/repo/init.rs @@ -3,7 +3,7 @@ use std::path::PathBuf; use anyhow::{Context, Result, bail}; use tokio::task::spawn_blocking; -use crate::args::{GlobalArgs, RepoInitArgs, ServerConnectionArgs}; +use crate::args::{GlobalArgs, RepoInitArgs, ServerTargetArgs}; use crate::server_client; pub(super) fn git_repo_root() -> Result { @@ -131,7 +131,7 @@ draft = true Ok(created) } -async fn check_github_app_installation(target: &ServerConnectionArgs) { +async fn check_github_app_installation(target: &ServerTargetArgs) { // Get the git remote origin URL let output = match std::process::Command::new("git") .args(["remote", "get-url", "origin"]) diff --git a/lib/crates/fabro-cli/src/commands/run/command.rs b/lib/crates/fabro-cli/src/commands/run/command.rs index ffd934c9c..4803fbf03 100644 --- a/lib/crates/fabro-cli/src/commands/run/command.rs +++ b/lib/crates/fabro-cli/src/commands/run/command.rs @@ -8,8 +8,8 @@ use crate::user_config::{self, settings_layer_with_storage_dir}; pub(crate) async fn execute(mut args: RunArgs, globals: &GlobalArgs) -> Result<()> { let styles: &'static Styles = Box::leak(Box::new(Styles::detect_stderr())); - let cli_settings = user_config::load_settings_with_storage_dir(args.target.storage_dir())?; - let cli = settings_layer_with_storage_dir(args.target.storage_dir())?; + let cli_settings = user_config::load_settings()?; + let cli = settings_layer_with_storage_dir(None)?; args.verbose = args.verbose || cli_settings.verbose_enabled(); let quiet = args.detach; @@ -22,7 +22,7 @@ pub(crate) async fn execute(mut args: RunArgs, globals: &GlobalArgs) -> Result<( #[cfg(not(feature = "sleep_inhibitor"))] let _ = prevent_idle_sleep; - let client = server_client::connect_server_connection(&created_run.connection).await?; + let client = server_client::connect_server_only(&args.target).await?; super::start::start_run_with_client(&client, &created_run.run_id, false).await?; if args.detach { diff --git a/lib/crates/fabro-cli/src/commands/run/create.rs b/lib/crates/fabro-cli/src/commands/run/create.rs index 842d2ba77..ab37a982f 100644 --- a/lib/crates/fabro-cli/src/commands/run/create.rs +++ b/lib/crates/fabro-cli/src/commands/run/create.rs @@ -9,12 +9,11 @@ use fabro_util::terminal::Styles; use super::output::{api_diagnostics_to_local, print_preflight_workflow_summary}; use crate::manifest_builder::{ManifestBuildInput, build_run_manifest, run_manifest_args}; use crate::server_client; -use crate::user_config::{self, ServerConnection}; +use crate::user_config::{self, ServerTarget}; pub(crate) struct CreatedRun { pub(crate) run_id: RunId, pub(crate) local_run_dir: Option, - pub(crate) connection: ServerConnection, } /// Create a workflow run: allocate run directory, persist RunRecord, return (run_id, run_dir). @@ -32,11 +31,12 @@ pub(crate) async fn create_run( .ok_or_else(|| anyhow::anyhow!("--workflow is required"))?; let cli_args_config = ConfigLayer::try_from(args)?; let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")); - let settings: Settings = cli_args_config + let _settings: Settings = cli_args_config .clone() .combine(ConfigLayer::for_workflow(workflow_path, &cwd)?) .combine(cli_defaults) .resolve()?; + let machine_settings = user_config::load_settings()?; let run_id = args .run_id .as_deref() @@ -51,8 +51,8 @@ pub(crate) async fn create_run( args: run_manifest_args(args), run_id, })?; - let connection = user_config::server_backed_command_connection(&args.target, &settings)?; - let client = server_client::connect_server_connection(&connection).await?; + let target = user_config::resolve_server_target(&args.target, &machine_settings)?; + let client = server_client::connect_server_only(&args.target).await?; if !quiet { let preflight = client.run_preflight(built.manifest.clone()).await?; let diagnostics = api_diagnostics_to_local(&preflight.workflow.diagnostics); @@ -65,19 +65,18 @@ pub(crate) async fn create_run( } let created_run_id = client.create_run_from_manifest(built.manifest).await?; - let local_run_dir = match &connection { - ServerConnection::Local { storage_dir } => Some( - Storage::new(storage_dir) + let local_run_dir = match &target { + ServerTarget::UnixSocket(_) => Some( + Storage::new(machine_settings.storage_dir()) .run_scratch(&created_run_id) .root() .to_path_buf(), ), - ServerConnection::Target(_) => None, + ServerTarget::HttpUrl { .. } => None, }; Ok(CreatedRun { run_id: created_run_id, local_run_dir, - connection, }) } diff --git a/lib/crates/fabro-cli/src/commands/run/mod.rs b/lib/crates/fabro-cli/src/commands/run/mod.rs index c8f8d725e..25e70d6b0 100644 --- a/lib/crates/fabro-cli/src/commands/run/mod.rs +++ b/lib/crates/fabro-cli/src/commands/run/mod.rs @@ -39,7 +39,7 @@ pub(crate) async fn dispatch(cmd: RunCommands, globals: &GlobalArgs) -> Result<( RunCommands::Create(mut args) => { apply_json_defaults(&mut args, globals); let styles: &'static Styles = Box::leak(Box::new(Styles::detect_stderr())); - let cli = settings_layer_with_storage_dir(args.target.storage_dir())?; + let cli = settings_layer_with_storage_dir(None)?; let created_run = Box::pin(create::create_run(&args, cli, styles, true)).await?; if globals.json { print_json_pretty(&serde_json::json!({ "run_id": created_run.run_id }))?; diff --git a/lib/crates/fabro-cli/src/commands/server/mod.rs b/lib/crates/fabro-cli/src/commands/server/mod.rs index bc567f741..a9f15a3cb 100644 --- a/lib/crates/fabro-cli/src/commands/server/mod.rs +++ b/lib/crates/fabro-cli/src/commands/server/mod.rs @@ -9,6 +9,7 @@ use std::time::Duration; use anyhow::Result; use fabro_server::bind; use fabro_server::bind::Bind; +use fabro_server::serve::ServeArgs; use fabro_util::terminal::Styles; use crate::args::{ @@ -23,11 +24,14 @@ pub(crate) async fn dispatch(command: ServerCommand, _globals: &GlobalArgs) -> R foreground, serve_args, }) => { - let settings = user_config::load_settings_with_storage_dir(storage_dir.as_deref())?; + let settings = user_config::load_settings_with_config_and_storage_dir( + serve_args.config.as_deref(), + storage_dir.as_deref(), + )?; let storage_dir = settings.storage_dir(); let bind_addr = match serve_args.bind.as_deref() { Some(s) => bind::parse_bind(s)?, - None => Bind::Unix(storage_dir.join("fabro.sock")), + None => Bind::Unix(user_config::default_socket_path()), }; let styles: &'static Styles = Box::leak(Box::new(Styles::detect_stderr())); start::execute(bind_addr, foreground, serve_args, storage_dir, styles).await @@ -51,18 +55,24 @@ pub(crate) async fn dispatch(command: ServerCommand, _globals: &GlobalArgs) -> R record_path, serve_args, }) => { + let active_config_path = serve_args + .config + .clone() + .or_else(|| user_config::active_settings_path(None)); let bind_addr = if let Some(s) = serve_args.bind.as_deref() { bind::parse_bind(s)? } else { // __serve should always receive an explicit --bind from the parent, // but fall back to the storage dir default if missing. - let settings = user_config::load_settings_with_storage_dir(storage_dir.as_deref())?; - Bind::Unix(settings.storage_dir().join("fabro.sock")) + Bind::Unix(user_config::default_socket_path()) }; let styles: &'static Styles = Box::leak(Box::new(Styles::detect_stderr())); foreground::execute( record_path, - serve_args, + ServeArgs { + config: active_config_path, + ..serve_args + }, bind_addr, storage_dir.clone_path(), styles, diff --git a/lib/crates/fabro-cli/src/commands/server/record.rs b/lib/crates/fabro-cli/src/commands/server/record.rs index 0574b218b..bcc314190 100644 --- a/lib/crates/fabro-cli/src/commands/server/record.rs +++ b/lib/crates/fabro-cli/src/commands/server/record.rs @@ -3,6 +3,7 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; use chrono::{DateTime, Utc}; use fabro_config::Storage; +use fabro_config::user::legacy_default_storage_root; use fabro_server::bind::Bind; use serde::{Deserialize, Serialize}; @@ -14,6 +15,12 @@ pub(crate) struct ServerRecord { pub started_at: DateTime, } +#[derive(Debug, Clone)] +pub(crate) struct ActiveServerRecord { + pub record: ServerRecord, + pub record_path: PathBuf, +} + pub(crate) fn write_server_record(path: &Path, record: &ServerRecord) -> Result<()> { if let Some(parent) = path.parent() { std::fs::create_dir_all(parent)?; @@ -35,17 +42,43 @@ pub(crate) fn server_record_is_running(record: &ServerRecord) -> bool { fabro_proc::process_alive(record.pid) && server_process_matches(record) } -pub(crate) fn active_server_record(storage_dir: &Path) -> Option { - let path = Storage::new(storage_dir).server_state().record_path(); +fn server_record_path(storage_dir: &Path) -> PathBuf { + Storage::new(storage_dir).server_state().record_path() +} + +fn legacy_record_path(storage_dir: &Path) -> Option { + let default_storage_dir = legacy_default_storage_root().join("storage"); + if storage_dir == default_storage_dir { + Some(server_record_path(&legacy_default_storage_root())) + } else { + None + } +} + +fn active_server_record_at_path(path: PathBuf) -> Option { let record = read_server_record(&path)?; if server_record_is_running(&record) { - Some(record) + Some(ActiveServerRecord { + record, + record_path: path, + }) } else { remove_server_record(&path); None } } +pub(crate) fn active_server_record_details(storage_dir: &Path) -> Option { + let primary_path = server_record_path(storage_dir); + active_server_record_at_path(primary_path).or_else(|| { + legacy_record_path(storage_dir).and_then(|path| active_server_record_at_path(path)) + }) +} + +pub(crate) fn active_server_record(storage_dir: &Path) -> Option { + active_server_record_details(storage_dir).map(|active| active.record) +} + #[cfg(unix)] fn server_process_matches(record: &ServerRecord) -> bool { let output = match std::process::Command::new("ps") diff --git a/lib/crates/fabro-cli/src/commands/server/start.rs b/lib/crates/fabro-cli/src/commands/server/start.rs index cb16008e9..e95883b12 100644 --- a/lib/crates/fabro-cli/src/commands/server/start.rs +++ b/lib/crates/fabro-cli/src/commands/server/start.rs @@ -5,6 +5,7 @@ use std::time::Duration; use anyhow::{Result, bail}; use chrono::Utc; use fabro_config::Storage; +use fabro_config::user::default_socket_path; use fabro_server::bind::Bind; use fabro_server::serve; use fabro_server::serve::ServeArgs; @@ -28,12 +29,44 @@ pub(crate) async fn execute( } } -pub(crate) fn ensure_server_running(storage_dir: &Path) -> Result { +pub(crate) fn ensure_server_running_for_storage( + storage_dir: &Path, + config_path: &Path, +) -> Result { if let Some(existing) = record::active_server_record(storage_dir) { return Ok(existing.bind); } - let bind = Bind::Unix(storage_dir.join("fabro.sock")); + let bind = Bind::Unix(default_socket_path()); + ensure_server_running_with_bind(bind, config_path, storage_dir) +} + +pub(crate) fn ensure_server_running_on_socket( + socket_path: &Path, + config_path: &Path, + storage_dir: &Path, +) -> Result<()> { + let bind = Bind::Unix(socket_path.to_path_buf()); + let _ = ensure_server_running_with_bind(bind, config_path, storage_dir)?; + Ok(()) +} + +fn ensure_server_running_with_bind( + bind: Bind, + config_path: &Path, + storage_dir: &Path, +) -> Result { + if let Some(existing) = record::active_server_record(storage_dir) { + if existing.bind == bind { + return Ok(existing.bind); + } + bail!( + "Server already running (pid {}) on {}", + existing.pid, + existing.bind + ); + } + let serve_args = ServeArgs { bind: None, model: None, @@ -41,7 +74,7 @@ pub(crate) fn ensure_server_running(storage_dir: &Path) -> Result { dry_run: false, sandbox: None, max_concurrent_runs: server_max_concurrent_runs_override(), - config: None, + config: Some(config_path.to_path_buf()), }; match execute_daemon(&bind, &serve_args, storage_dir, false) { diff --git a/lib/crates/fabro-cli/src/commands/server/stop.rs b/lib/crates/fabro-cli/src/commands/server/stop.rs index 6f3334697..067639b8c 100644 --- a/lib/crates/fabro-cli/src/commands/server/stop.rs +++ b/lib/crates/fabro-cli/src/commands/server/stop.rs @@ -2,16 +2,16 @@ use std::path::Path; use std::thread; use std::time::Duration; -use fabro_config::Storage; use fabro_server::bind::Bind; use super::record; pub(crate) fn execute(storage_dir: &Path, timeout: Duration) { - let Some(record) = record::active_server_record(storage_dir) else { + let Some(active) = record::active_server_record_details(storage_dir) else { eprintln!("Server is not running"); std::process::exit(1); }; + let record = active.record; fabro_proc::sigterm(record.pid); @@ -30,8 +30,7 @@ pub(crate) fn execute(storage_dir: &Path, timeout: Duration) { thread::sleep(Duration::from_millis(100)); } - let record_path = Storage::new(storage_dir).server_state().record_path(); - record::remove_server_record(&record_path); + record::remove_server_record(&active.record_path); if let Bind::Unix(ref path) = record.bind { let _ = std::fs::remove_file(path); diff --git a/lib/crates/fabro-cli/src/commands/validate.rs b/lib/crates/fabro-cli/src/commands/validate.rs index 5f3d0315d..6050de7b8 100644 --- a/lib/crates/fabro-cli/src/commands/validate.rs +++ b/lib/crates/fabro-cli/src/commands/validate.rs @@ -7,15 +7,12 @@ use crate::commands::run::output::api_diagnostics_to_local; use crate::manifest_builder::{ManifestBuildInput, build_run_manifest}; use crate::server_client; use crate::shared::{print_diagnostics, print_json_pretty, relative_path}; -use crate::user_config::{self, load_settings_with_storage_dir}; pub(crate) async fn run( args: &ValidateArgs, styles: &Styles, globals: &GlobalArgs, ) -> anyhow::Result<()> { - let settings = load_settings_with_storage_dir(args.target.storage_dir())?; - let connection = user_config::server_backed_command_connection(&args.target, &settings)?; let cwd = std::env::current_dir()?; let built = build_run_manifest(ManifestBuildInput { workflow: args.workflow.clone(), @@ -24,7 +21,7 @@ pub(crate) async fn run( args: None, run_id: None, })?; - let client = server_client::connect_server_connection(&connection).await?; + let client = server_client::connect_server_only(&args.target).await?; let response = client.run_preflight(built.manifest).await?; let diagnostics = api_diagnostics_to_local(&response.workflow.diagnostics); diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index 169ab9ed3..3f87af016 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -13,12 +13,13 @@ mod user_config; use anyhow::Result; use args::{Commands, GlobalArgs, LONG_VERSION, RunCommands, ServerCommand, ServerNamespace}; -use clap::{CommandFactory, FromArgMatches, Parser, error::ErrorKind, parser::ValueSource}; +use clap::{CommandFactory, Parser}; use fabro_config::user::load_settings_config; use fabro_telemetry::{git, panic as tel_panic, sanitize, sender}; use fabro_util::printer::Printer; use fabro_util::terminal::Styles; use rustls::crypto::ring::default_provider; +#[cfg(test)] use std::ffi::OsString; use tracing::debug; @@ -32,121 +33,19 @@ struct Cli { command: Box, } -#[derive(Clone, Copy, Debug, Default)] -struct ServerConnectionValueSources { - storage_dir: Option, - server: Option, -} - -impl ServerConnectionValueSources { - fn command_line_conflict(self) -> bool { - self.storage_dir == Some(ValueSource::CommandLine) - && self.server == Some(ValueSource::CommandLine) - } - - fn storage_dir_is_explicit(self) -> bool { - self.storage_dir == Some(ValueSource::CommandLine) - } -} - impl Cli { fn parse() -> Self { - Self::try_parse_from(std::env::args_os()).unwrap_or_else(|err| err.exit()) + ::parse() } + #[cfg(test)] fn try_parse_from(args: I) -> Result where I: IntoIterator, T: Into + Clone, { - let args: Vec = args.into_iter().map(Into::into).collect(); - let mut command = Self::command(); - let mut matches = command.try_get_matches_from_mut(args)?; - let sources = server_connection_value_sources(&matches); - if sources.command_line_conflict() { - return Err(command.error( - ErrorKind::ArgumentConflict, - "the argument '--server ' cannot be used with '--storage-dir '", - )); - } - - let mut cli = ::from_arg_matches_mut(&mut matches)?; - cli.apply_server_connection_value_sources(sources); - Ok(cli) + ::try_parse_from(args) } - - fn apply_server_connection_value_sources(&mut self, sources: ServerConnectionValueSources) { - self.command.apply_server_connection_value_sources(sources); - } -} - -impl Commands { - fn apply_server_connection_value_sources(&mut self, sources: ServerConnectionValueSources) { - match self { - Self::RunCmd(RunCommands::Run(args) | RunCommands::Create(args)) => { - args.target.storage_dir_explicit = sources.storage_dir_is_explicit(); - } - Self::Preflight(args) => { - args.target.storage_dir_explicit = sources.storage_dir_is_explicit(); - } - Self::Validate(args) => { - args.target.storage_dir_explicit = sources.storage_dir_is_explicit(); - } - Self::Graph(args) => { - args.target.storage_dir_explicit = sources.storage_dir_is_explicit(); - } - Self::Model { - command: Some(args::ModelsCommand::List(args)), - } => { - args.target.storage_dir_explicit = sources.storage_dir_is_explicit(); - } - Self::Model { - command: Some(args::ModelsCommand::Test(args)), - } => { - args.target.storage_dir_explicit = sources.storage_dir_is_explicit(); - } - Self::Doctor(args) => { - args.target.storage_dir_explicit = sources.storage_dir_is_explicit(); - } - Self::Repo(args::RepoNamespace { - command: args::RepoCommand::Init(args), - }) => { - args.target.storage_dir_explicit = sources.storage_dir_is_explicit(); - } - Self::Provider(args::ProviderNamespace { - command: args::ProviderCommand::Login(args), - }) => { - args.target.storage_dir_explicit = sources.storage_dir_is_explicit(); - } - Self::Secret(args::SecretNamespace { target, .. }) => { - target.storage_dir_explicit = sources.storage_dir_is_explicit(); - } - _ => {} - } - } -} - -fn server_connection_value_sources(matches: &clap::ArgMatches) -> ServerConnectionValueSources { - let sources = ServerConnectionValueSources { - storage_dir: matches - .try_contains_id("storage_dir") - .ok() - .and_then(|present| present.then(|| matches.value_source("storage_dir"))) - .flatten(), - server: matches - .try_contains_id("server") - .ok() - .and_then(|present| present.then(|| matches.value_source("server"))) - .flatten(), - }; - if sources.storage_dir.is_some() || sources.server.is_some() { - return sources; - } - - matches - .subcommand() - .map(|(_, subcommand_matches)| server_connection_value_sources(subcommand_matches)) - .unwrap_or_default() } #[tokio::main] @@ -451,28 +350,15 @@ mod tests { } #[test] - fn parse_run_storage_dir_after_subcommand() { - let cli = Cli::try_parse_from([ + fn parse_run_storage_dir_after_subcommand_is_rejected() { + let result = Cli::try_parse_from([ "fabro", "run", "test/simple.fabro", "--storage-dir", "/tmp/fabro", - ]) - .expect("should parse"); - match *cli.command { - Commands::RunCmd(RunCommands::Run(args)) => { - assert_eq!( - args.target.storage_dir(), - Some(std::path::Path::new("/tmp/fabro")) - ); - assert_eq!( - args.workflow.as_deref(), - Some(std::path::Path::new("test/simple.fabro")) - ); - } - _ => panic!("unexpected command variant"), - } + ]); + assert!(result.is_err(), "should reject run --storage-dir"); } #[test] @@ -488,7 +374,7 @@ mod tests { match *cli.command { Commands::Model { command: Some(ModelsCommand::List(args)), - } => assert_eq!(args.target.server(), Some("http://localhost:3000/api/v1")), + } => assert_eq!(args.target.as_deref(), Some("http://localhost:3000/api/v1")), _ => panic!("unexpected command variant"), } } diff --git a/lib/crates/fabro-cli/src/manifest_builder.rs b/lib/crates/fabro-cli/src/manifest_builder.rs index c34b0ec31..8de1c76b1 100644 --- a/lib/crates/fabro-cli/src/manifest_builder.rs +++ b/lib/crates/fabro-cli/src/manifest_builder.rs @@ -7,7 +7,7 @@ use fabro_config::ConfigLayer; use fabro_config::project::{self, discover_project_config, resolve_workflow_path}; use fabro_config::run::parse_run_config; use fabro_config::sandbox::DockerfileSource; -use fabro_config::user::default_settings_path; +use fabro_config::user::active_settings_path; use fabro_graphviz::graph::AttrValue; use fabro_graphviz::parser; use fabro_sandbox::daytona::detect_repo_info; @@ -86,7 +86,7 @@ pub(crate) fn build_run_manifest(input: ManifestBuildInput) -> Result Result Result { +pub(crate) async fn connect_server_only(args: &ServerTargetArgs) -> Result { + let settings = user_config::load_settings()?; + let target = user_config::resolve_server_target(args, &settings)?; + let runtime = LocalServerRuntime { + active_config_path: user_config::active_settings_path(None) + .unwrap_or_else(|| PathBuf::from(".fabro/settings.toml")), + storage_dir: settings.storage_dir(), + }; Ok(ServerStoreClient { - client: connect_resolved_api_client(connection).await?, + client: connect_target_api_client(&target, &runtime).await?, }) } -pub(crate) async fn connect_server_only(args: &ServerTargetArgs) -> Result { - let storage_dir = std::env::var_os("FABRO_STORAGE_DIR").map(std::path::PathBuf::from); - let settings = user_config::load_settings_with_storage_dir(storage_dir.as_deref())?; - let connection = user_config::server_only_command_connection(args, &settings)?; - connect_server_connection(&connection).await -} - pub(crate) async fn connect_api_client(storage_dir: &Path) -> Result { - let bind = start::ensure_server_running(storage_dir) + let config_path = user_config::active_settings_path(None) + .unwrap_or_else(|| PathBuf::from(".fabro/settings.toml")); + let bind = start::ensure_server_running_for_storage(storage_dir, &config_path) .with_context(|| format!("Failed to start fabro server for {}", storage_dir.display()))?; match bind { Bind::Unix(path) => connect_unix_socket_api_client(&path).await, @@ -133,29 +139,44 @@ pub(crate) async fn connect_api_client(storage_dir: &Path) -> Result Result { - match connection { - user_config::ServerConnection::Local { storage_dir } => { - connect_api_client(storage_dir).await + match target { + user_config::ServerTarget::HttpUrl { api_url, tls } => { + Ok(connect_remote_api_client(api_url, tls.as_ref())?) } - user_config::ServerConnection::Target(user_config::ServerTarget::HttpUrl { - api_url, - tls, - }) => connect_remote_api_client(api_url, tls.as_ref()), - user_config::ServerConnection::Target(user_config::ServerTarget::UnixSocket(path)) => { - connect_unix_socket_api_client(path).await + user_config::ServerTarget::UnixSocket(path) => { + match connect_unix_socket_api_client(path).await { + Ok(client) => Ok(client), + Err(_) => { + start::ensure_server_running_on_socket( + path, + &runtime.active_config_path, + &runtime.storage_dir, + ) + .with_context(|| { + format!("Failed to start fabro server for {}", path.display()) + })?; + connect_unix_socket_api_client(path).await + } + } } } } pub(crate) async fn connect_server_backed_api_client( - args: &ServerConnectionArgs, + args: &ServerTargetArgs, ) -> Result { - let settings = user_config::load_settings_with_storage_dir(args.storage_dir())?; - let connection = user_config::server_backed_command_connection(args, &settings)?; - connect_resolved_api_client(&connection).await + let settings = user_config::load_settings()?; + let target = user_config::resolve_server_target(args, &settings)?; + let runtime = LocalServerRuntime { + active_config_path: user_config::active_settings_path(None) + .unwrap_or_else(|| PathBuf::from(".fabro/settings.toml")), + storage_dir: settings.storage_dir(), + }; + connect_target_api_client(&target, &runtime).await } pub(crate) fn connect_remote_api_client( diff --git a/lib/crates/fabro-cli/src/user_config.rs b/lib/crates/fabro-cli/src/user_config.rs index 6b76227fa..c52176f4f 100644 --- a/lib/crates/fabro-cli/src/user_config.rs +++ b/lib/crates/fabro-cli/src/user_config.rs @@ -7,17 +7,24 @@ use fabro_config::ConfigLayer; use fabro_types::Settings; use tracing::debug; -use crate::args::{ServerConnectionArgs, ServerTargetArgs}; +use crate::args::ServerTargetArgs; pub(crate) fn load_settings() -> anyhow::Result { - ConfigLayer::settings()?.resolve() + load_settings_with_config_and_storage_dir(None, None) +} + +pub(crate) fn settings_layer_with_config_and_storage_dir( + config_path: Option<&Path>, + storage_dir: Option<&Path>, +) -> anyhow::Result { + let layer = load_settings_config(config_path)?; + Ok(apply_storage_dir_override(layer, storage_dir)) } pub(crate) fn settings_layer_with_storage_dir( storage_dir: Option<&Path>, ) -> anyhow::Result { - let layer = ConfigLayer::settings()?; - Ok(apply_storage_dir_override(layer, storage_dir)) + settings_layer_with_config_and_storage_dir(None, storage_dir) } pub(crate) fn load_settings_with_storage_dir( @@ -26,6 +33,13 @@ pub(crate) fn load_settings_with_storage_dir( settings_layer_with_storage_dir(storage_dir)?.resolve() } +pub(crate) fn load_settings_with_config_and_storage_dir( + config_path: Option<&Path>, + storage_dir: Option<&Path>, +) -> anyhow::Result { + settings_layer_with_config_and_storage_dir(config_path, storage_dir)?.resolve() +} + pub(crate) fn apply_storage_dir_override( mut layer: ConfigLayer, storage_dir: Option<&Path>, @@ -46,12 +60,6 @@ pub(crate) enum ServerTarget { UnixSocket(PathBuf), } -#[derive(Debug, Clone, PartialEq)] -pub(crate) enum ServerConnection { - Local { storage_dir: PathBuf }, - Target(ServerTarget), -} - fn configured_server_target(settings: &Settings) -> Result> { settings .server @@ -69,6 +77,10 @@ fn configured_server_target(settings: &Settings) -> Result> .transpose() } +pub(crate) fn default_server_target() -> ServerTarget { + ServerTarget::UnixSocket(default_socket_path()) +} + fn parse_server_target(value: &str, tls: Option) -> Result { if value.starts_with("http://") || value.starts_with("https://") { return Ok(ServerTarget::HttpUrl { @@ -102,40 +114,13 @@ fn explicit_server_target( .transpose() } -fn resolve_server_connection( - args: &ServerConnectionArgs, +pub(crate) fn resolve_server_target( + args: &ServerTargetArgs, settings: &Settings, - use_config_target: bool, -) -> Result { - let connection = if let Some(value) = args.server() { - ServerConnection::Target(parse_server_target( - value, - settings - .server - .as_ref() - .and_then(|server| server.tls.clone()), - )?) - } else if args.storage_dir_is_explicit() { - let storage_dir = args.storage_dir().ok_or_else(|| { - anyhow::anyhow!("--storage-dir flag was present but no value was parsed") - })?; - ServerConnection::Local { - storage_dir: storage_dir.to_path_buf(), - } - } else if use_config_target { - configured_server_target(settings)?.map_or_else( - || ServerConnection::Local { - storage_dir: settings.storage_dir(), - }, - ServerConnection::Target, - ) - } else { - ServerConnection::Local { - storage_dir: settings.storage_dir(), - } - }; - debug!(?connection, "Resolved server connection"); - Ok(connection) +) -> Result { + explicit_server_target(args, settings)? + .or(configured_server_target(settings)?) + .map_or_else(|| Ok(default_server_target()), Ok) } pub(crate) fn exec_server_target( @@ -147,37 +132,6 @@ pub(crate) fn exec_server_target( Ok(target) } -pub(crate) fn server_only_command_connection( - args: &ServerTargetArgs, - settings: &Settings, -) -> Result { - let connection = if let Some(target) = explicit_server_target(args, settings)? { - ServerConnection::Target(target) - } else if let Some(target) = configured_server_target(settings)? { - ServerConnection::Target(target) - } else { - ServerConnection::Local { - storage_dir: settings.storage_dir(), - } - }; - debug!(?connection, "Resolved server-only command connection"); - Ok(connection) -} - -pub(crate) fn model_server_connection( - args: &ServerConnectionArgs, - settings: &Settings, -) -> Result { - resolve_server_connection(args, settings, true) -} - -pub(crate) fn server_backed_command_connection( - args: &ServerConnectionArgs, - settings: &Settings, -) -> Result { - resolve_server_connection(args, settings, true) -} - pub(crate) fn build_server_client( tls: Option<&ClientTlsSettings>, ) -> anyhow::Result { @@ -212,7 +166,7 @@ pub(crate) fn build_server_client( #[cfg(test)] mod tests { use super::*; - use crate::args::{ServerConnectionArgs, ServerTargetArgs}; + use crate::args::ServerTargetArgs; fn server_target_args(value: Option<&str>) -> ServerTargetArgs { ServerTargetArgs { @@ -220,17 +174,6 @@ mod tests { } } - fn server_connection_args( - storage_dir: Option<&str>, - server: Option<&str>, - ) -> ServerConnectionArgs { - ServerConnectionArgs { - storage_dir: storage_dir.map(PathBuf::from), - server: server.map(str::to_string), - storage_dir_explicit: storage_dir.is_some(), - } - } - #[test] fn exec_has_no_server_target_by_default() { let settings = Settings::default(); @@ -281,7 +224,7 @@ mod tests { } #[test] - fn model_uses_configured_server_target() { + fn resolve_server_target_uses_configured_server_target() { let settings = Settings { server: Some(ServerSettings { target: Some("https://config.example.com".to_string()), @@ -290,16 +233,16 @@ mod tests { ..Settings::default() }; assert_eq!( - model_server_connection(&server_connection_args(None, None), &settings).unwrap(), - ServerConnection::Target(ServerTarget::HttpUrl { + resolve_server_target(&server_target_args(None), &settings).unwrap(), + ServerTarget::HttpUrl { api_url: "https://config.example.com".to_string(), tls: None, - }) + } ); } #[test] - fn server_only_command_uses_configured_server_target() { + fn resolve_server_target_explicit_target_overrides_config_target() { let settings = Settings { server: Some(ServerSettings { target: Some("https://config.example.com".to_string()), @@ -308,47 +251,24 @@ mod tests { ..Settings::default() }; assert_eq!( - server_only_command_connection(&server_target_args(None), &settings).unwrap(), - ServerConnection::Target(ServerTarget::HttpUrl { - api_url: "https://config.example.com".to_string(), - tls: None, - }) - ); - } - - #[test] - fn server_only_command_explicit_target_overrides_config_target() { - let settings = Settings { - server: Some(ServerSettings { - target: Some("https://config.example.com".to_string()), - tls: None, - }), - ..Settings::default() - }; - assert_eq!( - server_only_command_connection( + resolve_server_target( &server_target_args(Some("https://cli.example.com")), - &settings, + &settings ) .unwrap(), - ServerConnection::Target(ServerTarget::HttpUrl { + ServerTarget::HttpUrl { api_url: "https://cli.example.com".to_string(), tls: None, - }) + } ); } #[test] - fn server_only_command_defaults_to_local_storage_dir() { - let settings = Settings { - storage_dir: Some(PathBuf::from("/tmp/fabro")), - ..Settings::default() - }; + fn resolve_server_target_defaults_to_default_unix_socket_target() { + let settings = Settings::default(); assert_eq!( - server_only_command_connection(&server_target_args(None), &settings).unwrap(), - ServerConnection::Local { - storage_dir: PathBuf::from("/tmp/fabro"), - } + resolve_server_target(&server_target_args(None), &settings).unwrap(), + ServerTarget::UnixSocket(dirs::home_dir().unwrap().join(".fabro/fabro.sock")) ); } @@ -362,32 +282,14 @@ mod tests { ..Settings::default() }; assert_eq!( - model_server_connection( - &server_connection_args(None, Some("https://cli.example.com")), - &settings, + resolve_server_target( + &server_target_args(Some("https://cli.example.com")), + &settings ) .unwrap(), - ServerConnection::Target(ServerTarget::HttpUrl { + ServerTarget::HttpUrl { api_url: "https://cli.example.com".to_string(), tls: None, - }) - ); - } - - #[test] - fn storage_dir_suppresses_configured_remote_target() { - let settings = Settings { - server: Some(ServerSettings { - target: Some("https://config.example.com".to_string()), - tls: None, - }), - ..Settings::default() - }; - assert_eq!( - model_server_connection(&server_connection_args(Some("/tmp/fabro"), None), &settings) - .unwrap(), - ServerConnection::Local { - storage_dir: PathBuf::from("/tmp/fabro"), } ); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/config.rs b/lib/crates/fabro-cli/tests/it/cmd/config.rs index 1accf26fa..753028ec7 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/config.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/config.rs @@ -26,7 +26,7 @@ fn help() { Options: --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] + --storage-dir Local storage directory (default: ~/.fabro/storage) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] --quiet Suppress non-essential output [env: FABRO_QUIET=] diff --git a/lib/crates/fabro-cli/tests/it/cmd/create.rs b/lib/crates/fabro-cli/tests/it/cmd/create.rs index 533856109..b707d4026 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/create.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/create.rs @@ -33,25 +33,24 @@ fn help() { Path to a .fabro workflow file or .toml task config Options: - --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --dry-run Execute with simulated LLM backend - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --auto-approve Auto-approve all human gates - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --goal Override the workflow goal (exposed as $goal in prompts) - --goal-file Read the workflow goal from a file - --model Override default LLM model - --provider Override default LLM provider - -v, --verbose Enable verbose output - --sandbox Sandbox for agent tools [possible values: local, docker, daytona] - --label Attach a label to this run (repeatable, format: KEY=VALUE) - --no-retro Skip retro generation after the run - --preserve-sandbox Keep the sandbox alive after the run finishes (for debugging) - -d, --detach Run the workflow in the background and print the run ID - -h, --help Print help + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --dry-run Execute with simulated LLM backend + --auto-approve Auto-approve all human gates + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --goal Override the workflow goal (exposed as $goal in prompts) + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --goal-file Read the workflow goal from a file + --model Override default LLM model + --provider Override default LLM provider + -v, --verbose Enable verbose output + --sandbox Sandbox for agent tools [possible values: local, docker, daytona] + --label Attach a label to this run (repeatable, format: KEY=VALUE) + --no-retro Skip retro generation after the run + --preserve-sandbox Keep the sandbox alive after the run finishes (for debugging) + -d, --detach Run the workflow in the background and print the run ID + -h, --help Print help ----- stderr ----- "); } @@ -122,28 +121,13 @@ fn create_uses_configured_server_target_without_server_flag() { } #[test] -fn create_storage_dir_suppresses_configured_server_target() { +fn create_rejects_storage_dir_flag() { let context = test_context!(); - let server = MockServer::start(); - let mock = server.mock(|when, then| { - when.method("POST").path("/api/v1/runs"); - then.status(500) - .body("configured-server-should-not-be-used"); - }); - let local_storage = std::path::PathBuf::from(format!( - "/tmp/fabro-create-{}", - &context.test_case_id()[..8] - )); - context.write_home( - ".fabro/settings.toml", - format!("[server]\ntarget = \"{}/api/v1\"\n", server.base_url()), - ); - let output = context .create_cmd() .args([ "--storage-dir", - local_storage.to_str().unwrap(), + "/tmp/fabro-create", "--dry-run", fixture("simple.fabro").to_str().unwrap(), ]) @@ -151,13 +135,11 @@ fn create_storage_dir_suppresses_configured_server_target() { .expect("command should execute"); assert!( - output.status.success(), - "command failed:\nstdout:\n{}\nstderr:\n{}", - String::from_utf8_lossy(&output.stdout), - String::from_utf8_lossy(&output.stderr) + !output.status.success(), + "command should reject --storage-dir" ); - mock.assert_calls(0); - assert!(!output_stdout(&output).trim().is_empty()); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("unexpected argument '--storage-dir'")); } #[test] diff --git a/lib/crates/fabro-cli/tests/it/cmd/doctor.rs b/lib/crates/fabro-cli/tests/it/cmd/doctor.rs index ddcba30f6..f12d44fe8 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/doctor.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/doctor.rs @@ -25,14 +25,13 @@ fn help() { Usage: fabro doctor [OPTIONS] Options: - --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - -v, --verbose Show detailed information for each check - --quiet Suppress non-essential output [env: FABRO_QUIET=] - -h, --help Print help + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + -v, --verbose Show detailed information for each check + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --quiet Suppress non-essential output [env: FABRO_QUIET=] + -h, --help Print help ----- stderr ----- "); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/graph.rs b/lib/crates/fabro-cli/tests/it/cmd/graph.rs index caf51ba8b..5f4a3dc14 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/graph.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/graph.rs @@ -25,21 +25,16 @@ fn help() { [env: FABRO_JSON=] - --storage-dir - Local storage directory (default: ~/.fabro) + --server + Fabro server target: http(s) URL or absolute Unix socket path - [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] + [env: FABRO_SERVER=] --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --server - Fabro server target: http(s) URL or absolute Unix socket path - - [env: FABRO_SERVER=] - --format Output format @@ -54,11 +49,6 @@ fn help() { -o, --output Output file path (defaults to stdout) - --quiet - Suppress non-essential output - - [env: FABRO_QUIET=] - -d, --direction Graph layout direction (overrides the DOT file's rankdir) @@ -66,6 +56,11 @@ fn help() { - lr: Left to right - tb: Top to bottom + --quiet + Suppress non-essential output + + [env: FABRO_QUIET=] + --verbose Enable verbose output diff --git a/lib/crates/fabro-cli/tests/it/cmd/install.rs b/lib/crates/fabro-cli/tests/it/cmd/install.rs index 202e94ed7..7ff718a42 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/install.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/install.rs @@ -15,7 +15,7 @@ fn help() { Options: --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] + --storage-dir Local storage directory (default: ~/.fabro/storage) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] --web-url Base URL for the web UI (used for OAuth callback URLs) [default: http://localhost:3000] --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] diff --git a/lib/crates/fabro-cli/tests/it/cmd/model.rs b/lib/crates/fabro-cli/tests/it/cmd/model.rs index 700134d9e..0b602c8c1 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/model.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/model.rs @@ -222,7 +222,6 @@ fn list_uses_configured_server_target_without_server_flag() { ); let mut cmd = context.model(); - cmd.env_remove("FABRO_STORAGE_DIR"); cmd.args(["list", "--json"]); let output = cmd.assert().success().get_output().stdout.clone(); let models: serde_json::Value = @@ -232,3 +231,64 @@ fn list_uses_configured_server_target_without_server_flag() { assert_eq!(models.as_array().map(Vec::len), Some(1)); assert_eq!(models[0]["id"].as_str(), Some("remote-model")); } + +#[test] +fn list_uses_fabro_config_for_machine_settings() { + let context = test_context!(); + let server = MockServer::start(); + let mock = server.mock(|when, then| { + when.method("GET"); + then.status(200) + .header("Content-Type", "application/json") + .body( + serde_json::json!({ + "data": [{ + "id": "remote-model", + "display_name": "Remote Model", + "provider": "openai", + "family": "test", + "aliases": ["remote"], + "limits": { + "context_window": 131_072, + "max_output": 4096 + }, + "training": null, + "knowledge_cutoff": null, + "features": { + "tools": true, + "vision": false, + "reasoning": false, + "effort": false + }, + "costs": { + "input_cost_per_mtok": 1.0, + "output_cost_per_mtok": 2.0, + "cache_input_cost_per_mtok": null + }, + "estimated_output_tps": 42.0, + "default": false + }], + "meta": { "has_more": false } + }) + .to_string(), + ); + }); + let config_dir = tempfile::tempdir().unwrap(); + let config_path = config_dir.path().join("custom-settings.toml"); + std::fs::write( + &config_path, + format!("[server]\ntarget = \"{}/api/v1\"\n", server.base_url()), + ) + .unwrap(); + + let mut cmd = context.model(); + cmd.args(["list", "--json"]); + cmd.env("FABRO_CONFIG", &config_path); + let output = cmd.assert().success().get_output().stdout.clone(); + let models: serde_json::Value = + serde_json::from_slice(&output).expect("model list json should parse"); + + mock.assert(); + assert_eq!(models.as_array().map(Vec::len), Some(1)); + assert_eq!(models[0]["id"].as_str(), Some("remote-model")); +} diff --git a/lib/crates/fabro-cli/tests/it/cmd/model_list.rs b/lib/crates/fabro-cli/tests/it/cmd/model_list.rs index d63099d67..1f6be050d 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/model_list.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/model_list.rs @@ -14,16 +14,15 @@ fn help() { Usage: fabro model list [OPTIONS] Options: - --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - -p, --provider Filter by provider - -q, --query Search for models matching this string - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --verbose Enable verbose output [env: FABRO_VERBOSE=] - -h, --help Print help + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + -p, --provider Filter by provider + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + -q, --query Search for models matching this string + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help ----- stderr ----- "); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/model_test.rs b/lib/crates/fabro-cli/tests/it/cmd/model_test.rs index d8111d5b2..af8efe243 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/model_test.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/model_test.rs @@ -14,17 +14,16 @@ fn help() { Usage: fabro model test [OPTIONS] Options: - --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - -p, --provider Filter by provider - -m, --model Test a specific model - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --deep Run a multi-turn tool-use test (catches reasoning round-trip bugs) - --verbose Enable verbose output [env: FABRO_VERBOSE=] - -h, --help Print help + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + -p, --provider Filter by provider + -m, --model Test a specific model + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --deep Run a multi-turn tool-use test (catches reasoning round-trip bugs) + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help ----- stderr ----- "); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs b/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs index 7ef07e083..9698c3d5b 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs @@ -1,11 +1,18 @@ #![allow(clippy::absolute_paths)] +use fabro_config::Storage; +use fabro_server::bind::Bind; use fabro_test::{fabro_snapshot, test_context}; use fabro_types::run_event::PullRequestCreatedProps; use fabro_types::{EventBody, RunEvent, RunId}; use super::support::setup_completed_fast_dry_run; +#[derive(Debug, serde::Deserialize)] +struct TestServerRecord { + bind: Bind, +} + #[test] fn help() { let context = test_context!(); @@ -58,11 +65,25 @@ fn pr_view_reads_pull_request_from_store_without_pull_request_json() { let runtime = tokio::runtime::Runtime::new().unwrap(); runtime.block_on(async { - let client = reqwest::ClientBuilder::new() - .unix_socket(context.storage_dir.join("fabro.sock")) - .no_proxy() - .build() - .unwrap(); + let record_path = Storage::new(&context.storage_dir) + .server_state() + .record_path(); + let record: TestServerRecord = + serde_json::from_str(&std::fs::read_to_string(record_path).unwrap()).unwrap(); + let (client, base_url) = match record.bind { + Bind::Unix(path) => ( + reqwest::ClientBuilder::new() + .unix_socket(path) + .no_proxy() + .build() + .unwrap(), + "http://fabro".to_string(), + ), + Bind::Tcp(addr) => ( + reqwest::ClientBuilder::new().no_proxy().build().unwrap(), + format!("http://{addr}"), + ), + }; let event = RunEvent { id: ulid::Ulid::new().to_string(), ts: chrono::Utc::now(), @@ -83,7 +104,7 @@ fn pr_view_reads_pull_request_from_store_without_pull_request_json() { }), }; client - .post(format!("http://fabro/api/v1/runs/{run_id}/events")) + .post(format!("{base_url}/api/v1/runs/{run_id}/events")) .json(&event) .send() .await diff --git a/lib/crates/fabro-cli/tests/it/cmd/preflight.rs b/lib/crates/fabro-cli/tests/it/cmd/preflight.rs index e615a1872..7f4f2a4e9 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/preflight.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/preflight.rs @@ -20,19 +20,18 @@ fn help() { Path to a .fabro workflow file or .toml task config Options: - --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --goal Override the workflow goal (exposed as $goal in prompts) - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --goal-file Read the workflow goal from a file - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --model Override default LLM model - --provider Override default LLM provider - -v, --verbose Enable verbose output - --sandbox Sandbox for agent tools [possible values: local, docker, daytona] - -h, --help Print help + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --goal Override the workflow goal (exposed as $goal in prompts) + --goal-file Read the workflow goal from a file + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --model Override default LLM model + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --provider Override default LLM provider + -v, --verbose Enable verbose output + --sandbox Sandbox for agent tools [possible values: local, docker, daytona] + -h, --help Print help ----- stderr ----- "); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/provider_login.rs b/lib/crates/fabro-cli/tests/it/cmd/provider_login.rs index 64333f397..3693af3a9 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/provider_login.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/provider_login.rs @@ -14,15 +14,14 @@ fn help() { Usage: fabro provider login [OPTIONS] --provider Options: - --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --provider LLM provider to authenticate with - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --verbose Enable verbose output [env: FABRO_VERBOSE=] - -h, --help Print help + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --provider LLM provider to authenticate with + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help ----- stderr ----- "); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/repo.rs b/lib/crates/fabro-cli/tests/it/cmd/repo.rs index f96a1d25f..5711a44df 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/repo.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/repo.rs @@ -105,14 +105,13 @@ fn test_repo_init_help_does_not_show_skill() { Usage: fabro repo init [OPTIONS] Options: - --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --verbose Enable verbose output [env: FABRO_VERBOSE=] - -h, --help Print help + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help ----- stderr ----- "); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/repo_init.rs b/lib/crates/fabro-cli/tests/it/cmd/repo_init.rs index 64d914c5f..125802476 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/repo_init.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/repo_init.rs @@ -16,14 +16,13 @@ fn help() { Usage: fabro repo init [OPTIONS] Options: - --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --verbose Enable verbose output [env: FABRO_VERBOSE=] - -h, --help Print help + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help ----- stderr ----- "); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/run.rs b/lib/crates/fabro-cli/tests/it/cmd/run.rs index e7bad1dbb..c0fcd2b73 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/run.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/run.rs @@ -117,25 +117,24 @@ fn help() { Path to a .fabro workflow file or .toml task config Options: - --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --dry-run Execute with simulated LLM backend - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --auto-approve Auto-approve all human gates - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --goal Override the workflow goal (exposed as $goal in prompts) - --goal-file Read the workflow goal from a file - --model Override default LLM model - --provider Override default LLM provider - -v, --verbose Enable verbose output - --sandbox Sandbox for agent tools [possible values: local, docker, daytona] - --label Attach a label to this run (repeatable, format: KEY=VALUE) - --no-retro Skip retro generation after the run - --preserve-sandbox Keep the sandbox alive after the run finishes (for debugging) - -d, --detach Run the workflow in the background and print the run ID - -h, --help Print help + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --dry-run Execute with simulated LLM backend + --auto-approve Auto-approve all human gates + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --goal Override the workflow goal (exposed as $goal in prompts) + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --goal-file Read the workflow goal from a file + --model Override default LLM model + --provider Override default LLM provider + -v, --verbose Enable verbose output + --sandbox Sandbox for agent tools [possible values: local, docker, daytona] + --label Attach a label to this run (repeatable, format: KEY=VALUE) + --no-retro Skip retro generation after the run + --preserve-sandbox Keep the sandbox alive after the run finishes (for debugging) + -d, --detach Run the workflow in the background and print the run ID + -h, --help Print help ----- stderr ----- "); } @@ -236,31 +235,13 @@ fn detach_uses_configured_server_target_without_server_flag() { } #[test] -fn detach_storage_dir_suppresses_configured_server_target() { +fn detach_rejects_storage_dir_flag() { let context = test_context!(); - let server = MockServer::start(); - let create_mock = server.mock(|when, then| { - when.method("POST").path("/api/v1/runs"); - then.status(500) - .body("configured-server-should-not-be-used"); - }); - let start_mock = server.mock(|when, then| { - when.method("POST").path_includes("/api/v1/runs/"); - then.status(500) - .body("configured-server-should-not-be-used"); - }); - let local_storage = - std::path::PathBuf::from(format!("/tmp/fabro-run-{}", &context.test_case_id()[..8])); - context.write_home( - ".fabro/settings.toml", - format!("[server]\ntarget = \"{}/api/v1\"\n", server.base_url()), - ); - let output = context .run_cmd() .args([ "--storage-dir", - local_storage.to_str().unwrap(), + "/tmp/fabro-run", "--detach", "--dry-run", "--auto-approve", @@ -271,14 +252,11 @@ fn detach_storage_dir_suppresses_configured_server_target() { .expect("command should execute"); assert!( - output.status.success(), - "command failed:\nstdout:\n{}\nstderr:\n{}", - String::from_utf8_lossy(&output.stdout), - String::from_utf8_lossy(&output.stderr) + !output.status.success(), + "command should reject --storage-dir" ); - create_mock.assert_calls(0); - start_mock.assert_calls(0); - assert!(!String::from_utf8_lossy(&output.stdout).trim().is_empty()); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("unexpected argument '--storage-dir'")); } #[test] diff --git a/lib/crates/fabro-cli/tests/it/cmd/runner.rs b/lib/crates/fabro-cli/tests/it/cmd/runner.rs index 4eb547557..bc3f986b3 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/runner.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/runner.rs @@ -20,7 +20,7 @@ fn help() { Options: --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] + --storage-dir Local storage directory (default: ~/.fabro/storage) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] --run-id Run ID --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] diff --git a/lib/crates/fabro-cli/tests/it/cmd/secret.rs b/lib/crates/fabro-cli/tests/it/cmd/secret.rs index 981692492..b18b19f0c 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/secret.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/secret.rs @@ -23,14 +23,13 @@ fn help() { help Print this message or the help of the given subcommand(s) Options: - --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --verbose Enable verbose output [env: FABRO_VERBOSE=] - -h, --help Print help + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help ----- stderr ----- "); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs index eb3c588eb..9185d241c 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs @@ -26,7 +26,7 @@ fn help() { --json Output as JSON [env: FABRO_JSON=] --storage-dir - Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] + Local storage directory (default: ~/.fabro/storage) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] --foreground @@ -96,19 +96,51 @@ fn start_already_running_exits_with_error() { .success(); } +#[test] +fn start_without_bind_uses_home_socket_instead_of_storage_socket() { + let context = test_context!(); + let expected_socket = context.home_dir.join(".fabro").join("fabro.sock"); + let storage_socket = context.storage_dir.join("fabro.sock"); + + context + .command() + .args(["server", "start", "--dry-run"]) + .assert() + .success(); + + let output = context + .command() + .args(["server", "status", "--json"]) + .assert() + .success() + .get_output() + .stdout + .clone(); + let json: serde_json::Value = serde_json::from_slice(&output).unwrap(); + + assert_eq!(json["bind"].as_str(), expected_socket.to_str()); + assert_ne!(json["bind"].as_str(), storage_socket.to_str()); + + context + .command() + .args(["server", "stop"]) + .assert() + .success(); +} + #[test] fn concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up() { fn run_ps_json( home_dir: &std::path::Path, temp_dir: &std::path::Path, - storage_dir: &std::path::Path, + config_path: &std::path::Path, ) -> std::process::Output { std::process::Command::new(env!("CARGO_BIN_EXE_fabro")) .current_dir(temp_dir) .env("NO_COLOR", "1") .env("HOME", home_dir) + .env("FABRO_CONFIG", config_path) .env("FABRO_NO_UPGRADE_CHECK", "true") - .env("FABRO_STORAGE_DIR", storage_dir) .args(["ps", "-a", "--json"]) .output() .expect("ps command should execute") @@ -130,7 +162,19 @@ fn concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up() { let storage_root = isolated_storage_dir(); let storage_dir = storage_root.path().join("storage"); - let socket_path = storage_dir.join("fabro.sock").display().to_string(); + let socket_path = storage_root.path().join("shared.sock"); + let socket_path_str = socket_path.display().to_string(); + let config_dir = tempfile::tempdir_in("/tmp").unwrap(); + let config_path = config_dir.path().join("settings.toml"); + std::fs::write( + &config_path, + format!( + "storage_dir = \"{}\"\n[server]\ntarget = \"{}\"\n", + storage_dir.display(), + socket_path.display() + ), + ) + .unwrap(); let home_a = tempfile::tempdir_in("/tmp").unwrap(); let home_b = tempfile::tempdir_in("/tmp").unwrap(); let temp_a = tempfile::tempdir_in("/tmp").unwrap(); @@ -138,17 +182,17 @@ fn concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up() { let barrier = Arc::new(Barrier::new(3)); let barrier_a = Arc::clone(&barrier); - let storage_a = storage_dir.clone(); + let config_a = config_path.clone(); let thread_a = std::thread::spawn(move || { barrier_a.wait(); - run_ps_json(home_a.path(), temp_a.path(), &storage_a) + run_ps_json(home_a.path(), temp_a.path(), &config_a) }); let barrier_b = Arc::clone(&barrier); - let storage_b = storage_dir.clone(); + let config_b = config_path.clone(); let thread_b = std::thread::spawn(move || { barrier_b.wait(); - run_ps_json(home_b.path(), temp_b.path(), &storage_b) + run_ps_json(home_b.path(), temp_b.path(), &config_b) }); barrier.wait(); @@ -169,7 +213,7 @@ fn concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up() { let deadline = Instant::now() + Duration::from_secs(5); while Instant::now() < deadline { - if storage_dir.join("server.json").exists() && daemon_match_count(&socket_path) == 1 { + if storage_dir.join("server.json").exists() && daemon_match_count(&socket_path_str) == 1 { break; } std::thread::sleep(Duration::from_millis(50)); @@ -179,15 +223,15 @@ fn concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up() { "shared storage should have an active server record" ); assert_eq!( - daemon_match_count(&socket_path), + daemon_match_count(&socket_path_str), 1, "concurrent auto-start should converge on one daemon" ); let stop = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")) .env("NO_COLOR", "1") + .env("FABRO_CONFIG", &config_path) .env("FABRO_NO_UPGRADE_CHECK", "true") - .env("FABRO_STORAGE_DIR", &storage_dir) .args(["server", "stop"]) .output() .expect("server stop should execute"); @@ -200,7 +244,7 @@ fn concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up() { let deadline = Instant::now() + Duration::from_secs(5); while Instant::now() < deadline { - if !storage_dir.join("server.json").exists() && daemon_match_count(&socket_path) == 0 { + if !storage_dir.join("server.json").exists() && daemon_match_count(&socket_path_str) == 0 { break; } std::thread::sleep(Duration::from_millis(50)); @@ -210,7 +254,7 @@ fn concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up() { "last TestContext drop should remove the server record" ); assert_eq!( - daemon_match_count(&socket_path), + daemon_match_count(&socket_path_str), 0, "last TestContext drop should clean up the shared daemon" ); diff --git a/lib/crates/fabro-cli/tests/it/cmd/server_status.rs b/lib/crates/fabro-cli/tests/it/cmd/server_status.rs index a0ff93156..0fac59c15 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/server_status.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/server_status.rs @@ -20,7 +20,7 @@ fn help() { Usage: fabro server status [OPTIONS] Options: - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] + --storage-dir Local storage directory (default: ~/.fabro/storage) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] --json Output as JSON --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] diff --git a/lib/crates/fabro-cli/tests/it/cmd/server_stop.rs b/lib/crates/fabro-cli/tests/it/cmd/server_stop.rs index 06764141a..4860adda5 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/server_stop.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/server_stop.rs @@ -21,7 +21,7 @@ fn help() { Options: --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] + --storage-dir Local storage directory (default: ~/.fabro/storage) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] --timeout Seconds to wait for graceful shutdown before SIGKILL [default: 10] --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] diff --git a/lib/crates/fabro-cli/tests/it/cmd/store_dump.rs b/lib/crates/fabro-cli/tests/it/cmd/store_dump.rs index f31412840..b8b71b06c 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/store_dump.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/store_dump.rs @@ -22,7 +22,7 @@ fn help() { Options: --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] + --storage-dir Local storage directory (default: ~/.fabro/storage) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] -o, --output Output directory (must not exist or be empty) --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] diff --git a/lib/crates/fabro-cli/tests/it/cmd/support.rs b/lib/crates/fabro-cli/tests/it/cmd/support.rs index 4995f3da7..0ef23e054 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/support.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/support.rs @@ -9,6 +9,8 @@ use std::path::{Path, PathBuf}; use std::process::Output; use std::time::{Duration, Instant}; +use fabro_config::Storage; +use fabro_server::bind::Bind; use fabro_store::EventEnvelope; use fabro_test::TestContext; use fabro_types::{ @@ -632,12 +634,34 @@ fn block_on(future: impl std::future::Future) -> T { .block_on(future) } -fn server_http_client(storage_dir: &Path) -> reqwest::Client { - reqwest::ClientBuilder::new() - .unix_socket(storage_dir.join("fabro.sock")) - .no_proxy() - .build() - .expect("test HTTP client should build") +#[derive(Debug, serde::Deserialize)] +struct TestServerRecord { + bind: Bind, +} + +fn server_endpoint(storage_dir: &Path) -> Option<(reqwest::Client, String)> { + let record_path = Storage::new(storage_dir).server_state().record_path(); + let record = std::fs::read_to_string(record_path) + .ok() + .and_then(|content| serde_json::from_str::(&content).ok())?; + match record.bind { + Bind::Unix(path) if path.exists() => Some(( + reqwest::ClientBuilder::new() + .unix_socket(path) + .no_proxy() + .build() + .expect("test Unix-socket HTTP client should build"), + "http://fabro".to_string(), + )), + Bind::Unix(_) => None, + Bind::Tcp(addr) => Some(( + reqwest::ClientBuilder::new() + .no_proxy() + .build() + .expect("test TCP HTTP client should build"), + format!("http://{addr}"), + )), + } } async fn get_server_json(run_dir: &Path, path: &str) -> T { @@ -650,14 +674,8 @@ async fn try_get_server_json_for_storage( storage_dir: &Path, path: &str, ) -> Option { - if !storage_dir.join("fabro.sock").exists() { - return None; - } - let response = server_http_client(storage_dir) - .get(format!("http://fabro{path}")) - .send() - .await - .ok()?; + let (client, base_url) = server_endpoint(storage_dir)?; + let response = client.get(format!("{base_url}{path}")).send().await.ok()?; if !response.status().is_success() { return None; } @@ -668,8 +686,9 @@ async fn get_server_json_for_storage( storage_dir: &Path, path: &str, ) -> T { - let response = server_http_client(storage_dir) - .get(format!("http://fabro{path}")) + let (client, base_url) = server_endpoint(storage_dir).expect("server endpoint should exist"); + let response = client + .get(format!("{base_url}{path}")) .send() .await .expect("server request should succeed"); diff --git a/lib/crates/fabro-cli/tests/it/cmd/system_df.rs b/lib/crates/fabro-cli/tests/it/cmd/system_df.rs index e0d7f54e0..d4a6ba2da 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/system_df.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/system_df.rs @@ -18,7 +18,7 @@ fn help() { Options: --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] + --storage-dir Local storage directory (default: ~/.fabro/storage) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] -v, --verbose Show per-run breakdown --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] diff --git a/lib/crates/fabro-cli/tests/it/cmd/system_prune.rs b/lib/crates/fabro-cli/tests/it/cmd/system_prune.rs index 70f93538a..f0bc8bc9e 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/system_prune.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/system_prune.rs @@ -17,7 +17,7 @@ fn help() { Options: --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] + --storage-dir Local storage directory (default: ~/.fabro/storage) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] --before Only include runs started before this date (YYYY-MM-DD prefix match) --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] diff --git a/lib/crates/fabro-cli/tests/it/cmd/validate.rs b/lib/crates/fabro-cli/tests/it/cmd/validate.rs index ef2ca2f72..b9b989333 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/validate.rs @@ -24,14 +24,13 @@ fn help() { Path to the .fabro workflow file Options: - --json Output as JSON [env: FABRO_JSON=] - --storage-dir Local storage directory (default: ~/.fabro) [env: FABRO_STORAGE_DIR=[STORAGE_DIR]] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --verbose Enable verbose output [env: FABRO_VERBOSE=] - -h, --help Print help + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help ----- stderr ----- "); } diff --git a/lib/crates/fabro-cli/tests/it/scenario/mod.rs b/lib/crates/fabro-cli/tests/it/scenario/mod.rs index d1cd02137..d28e171ce 100644 --- a/lib/crates/fabro-cli/tests/it/scenario/mod.rs +++ b/lib/crates/fabro-cli/tests/it/scenario/mod.rs @@ -10,6 +10,8 @@ use std::path::{Path, PathBuf}; use std::time::Duration; use crate::cmd::support::RunProjection; +use fabro_config::Storage; +use fabro_server::bind::Bind; pub(super) fn fixture(name: &str) -> PathBuf { Path::new(env!("CARGO_MANIFEST_DIR")) .join("tests/it/workflow/fixtures") @@ -37,20 +39,43 @@ fn infer_run_id(run_dir: &Path) -> String { .expect("run dir should contain resolvable run id") } -fn server_http_client(storage_dir: &Path) -> reqwest::Client { - reqwest::ClientBuilder::new() - .unix_socket(storage_dir.join("fabro.sock")) - .no_proxy() - .build() - .expect("test HTTP client should build") +#[derive(Debug, serde::Deserialize)] +struct TestServerRecord { + bind: Bind, +} + +fn server_endpoint(storage_dir: &Path) -> (reqwest::Client, String) { + let record_path = Storage::new(storage_dir).server_state().record_path(); + let record: TestServerRecord = serde_json::from_str( + &std::fs::read_to_string(record_path).expect("server record should exist"), + ) + .expect("server record should parse"); + match record.bind { + Bind::Unix(path) => ( + reqwest::ClientBuilder::new() + .unix_socket(path) + .no_proxy() + .build() + .expect("test Unix-socket HTTP client should build"), + "http://fabro".to_string(), + ), + Bind::Tcp(addr) => ( + reqwest::ClientBuilder::new() + .no_proxy() + .build() + .expect("test TCP HTTP client should build"), + format!("http://{addr}"), + ), + } } async fn get_server_json_for_storage( storage_dir: &Path, path: &str, ) -> T { - let response = server_http_client(storage_dir) - .get(format!("http://fabro{path}")) + let (client, base_url) = server_endpoint(storage_dir); + let response = client + .get(format!("{base_url}{path}")) .send() .await .expect("server request should succeed"); diff --git a/lib/crates/fabro-cli/tests/it/workflow/mod.rs b/lib/crates/fabro-cli/tests/it/workflow/mod.rs index 76442ed92..3415a08ff 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/mod.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/mod.rs @@ -14,6 +14,8 @@ use std::path::{Path, PathBuf}; use std::time::Duration; use crate::cmd::support::RunProjection; +use fabro_config::Storage; +use fabro_server::bind::Bind; use fabro_test::TestContext; use serde_json::Value; @@ -123,20 +125,43 @@ fn block_on(future: impl std::future::Future) -> T { .block_on(future) } -fn server_http_client(storage_dir: &Path) -> reqwest::Client { - reqwest::ClientBuilder::new() - .unix_socket(storage_dir.join("fabro.sock")) - .no_proxy() - .build() - .expect("test HTTP client should build") +#[derive(Debug, serde::Deserialize)] +struct TestServerRecord { + bind: Bind, +} + +fn server_endpoint(storage_dir: &Path) -> (reqwest::Client, String) { + let record_path = Storage::new(storage_dir).server_state().record_path(); + let record: TestServerRecord = serde_json::from_str( + &std::fs::read_to_string(record_path).expect("server record should exist"), + ) + .expect("server record should parse"); + match record.bind { + Bind::Unix(path) => ( + reqwest::ClientBuilder::new() + .unix_socket(path) + .no_proxy() + .build() + .expect("test Unix-socket HTTP client should build"), + "http://fabro".to_string(), + ), + Bind::Tcp(addr) => ( + reqwest::ClientBuilder::new() + .no_proxy() + .build() + .expect("test TCP HTTP client should build"), + format!("http://{addr}"), + ), + } } async fn get_server_json_for_storage( storage_dir: &Path, path: &str, ) -> T { - let response = server_http_client(storage_dir) - .get(format!("http://fabro{path}")) + let (client, base_url) = server_endpoint(storage_dir); + let response = client + .get(format!("{base_url}{path}")) .send() .await .expect("server request should succeed"); diff --git a/lib/crates/fabro-config/src/user.rs b/lib/crates/fabro-config/src/user.rs index 5fdcdb3de..ca7bd2a1d 100644 --- a/lib/crates/fabro-config/src/user.rs +++ b/lib/crates/fabro-config/src/user.rs @@ -16,6 +16,7 @@ pub const SETTINGS_CONFIG_FILENAME: &str = "settings.toml"; pub const LEGACY_USER_CONFIG_FILENAME: &str = "cli.toml"; pub const LEGACY_OLD_USER_CONFIG_FILENAME: &str = "user.toml"; pub const LEGACY_SERVER_CONFIG_FILENAME: &str = "server.toml"; +pub const FABRO_CONFIG_ENV: &str = "FABRO_CONFIG"; static WARNED_LEGACY_USER_CONFIGS: OnceLock>> = OnceLock::new(); @@ -84,6 +85,20 @@ pub fn default_settings_path() -> Option { Some(Home::from_env().user_config()) } +pub fn default_socket_path() -> PathBuf { + Home::from_env().root().join("fabro.sock") +} + +pub fn legacy_default_storage_root() -> PathBuf { + Home::from_env().root().to_path_buf() +} + +pub fn active_settings_path(path: Option<&Path>) -> Option { + path.map(Path::to_path_buf) + .or_else(|| std::env::var_os(FABRO_CONFIG_ENV).map(PathBuf::from)) + .or_else(default_settings_path) +} + pub fn legacy_user_config_path() -> Option { Some(Home::from_env().root().join(LEGACY_USER_CONFIG_FILENAME)) } @@ -115,8 +130,11 @@ fn should_warn_about_legacy_user_config(path: &Path) -> bool { /// default file doesn't exist. An explicit path that doesn't exist is an error. #[allow(clippy::print_stderr)] pub fn load_settings_config(path: Option<&Path>) -> anyhow::Result { - if let Some(explicit) = path { - return crate::load_config_file(Some(explicit), SETTINGS_CONFIG_FILENAME); + if let Some(explicit) = path + .map(Path::to_path_buf) + .or_else(|| std::env::var_os(FABRO_CONFIG_ENV).map(PathBuf::from)) + { + return crate::load_config_file(Some(&explicit), SETTINGS_CONFIG_FILENAME); } for legacy_path in [ @@ -144,12 +162,37 @@ pub fn load_settings_config(path: Option<&Path>) -> anyhow::Result #[cfg(test)] mod tests { use super::{ - LEGACY_OLD_USER_CONFIG_FILENAME, LEGACY_SERVER_CONFIG_FILENAME, - LEGACY_USER_CONFIG_FILENAME, SETTINGS_CONFIG_FILENAME, default_settings_path, - legacy_old_user_config_path, legacy_server_config_path, legacy_user_config_path, - should_warn_about_legacy_user_config, + FABRO_CONFIG_ENV, LEGACY_OLD_USER_CONFIG_FILENAME, LEGACY_SERVER_CONFIG_FILENAME, + LEGACY_USER_CONFIG_FILENAME, SETTINGS_CONFIG_FILENAME, active_settings_path, + default_settings_path, default_socket_path, legacy_old_user_config_path, + legacy_server_config_path, legacy_user_config_path, should_warn_about_legacy_user_config, }; + struct EnvGuard { + key: &'static str, + original: Option, + } + + impl EnvGuard { + fn set(key: &'static str, value: Option<&std::path::Path>) -> Self { + let original = std::env::var_os(key); + match value { + Some(value) => std::env::set_var(key, value), + None => std::env::remove_var(key), + } + Self { key, original } + } + } + + impl Drop for EnvGuard { + fn drop(&mut self) { + match &self.original { + Some(value) => std::env::set_var(self.key, value), + None => std::env::remove_var(self.key), + } + } + } + #[test] fn should_warn_about_legacy_user_config_once_per_path() { let dir = tempfile::tempdir().unwrap(); @@ -169,6 +212,7 @@ mod tests { default_settings_path(), Some(home.join(".fabro").join(SETTINGS_CONFIG_FILENAME)) ); + assert_eq!(default_socket_path(), home.join(".fabro/fabro.sock")); assert_eq!( legacy_user_config_path(), Some(home.join(".fabro").join(LEGACY_USER_CONFIG_FILENAME)) @@ -196,4 +240,13 @@ mod tests { assert!(!should_warn_about_legacy_user_config(&server)); assert!(should_warn_about_legacy_user_config(&cli)); } + + #[test] + fn active_settings_path_honors_fabro_config_env() { + let dir = tempfile::tempdir().unwrap(); + let custom_path = dir.path().join("custom-settings.toml"); + let _guard = EnvGuard::set(FABRO_CONFIG_ENV, Some(&custom_path)); + + assert_eq!(active_settings_path(None), Some(custom_path)); + } } diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 481dc6d02..247899689 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -4,7 +4,7 @@ use std::time::Duration; use fabro_config::Storage; use fabro_config::server::resolve_storage_dir; -use fabro_config::user::{default_settings_path, load_settings_config}; +use fabro_config::user::{active_settings_path, load_settings_config}; use fabro_util::terminal::Styles; use object_store::ObjectStore; use object_store::local::LocalFileSystem; @@ -61,9 +61,7 @@ fn load_settings(path: Option<&Path>) -> anyhow::Result { } fn resolved_config_path(path: Option<&Path>) -> PathBuf { - path.map(Path::to_path_buf) - .or_else(default_settings_path) - .unwrap_or_else(|| PathBuf::from(".fabro/settings.toml")) + active_settings_path(path).unwrap_or_else(|| PathBuf::from(".fabro/settings.toml")) } fn apply_serve_overrides(base: &Settings, args: &ServeArgs, dry_run_mode: bool) -> Settings { diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index 1e2042351..42095cb74 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -413,11 +413,21 @@ impl TestContext { let temp_dir = root_path.join("temp"); let home_dir = root_path.join("home"); - let storage_dir = session_paths.storage_dir.clone(); + let storage_dir = root_path.join("storage"); let test_case_id = test_case_id(); std::fs::create_dir_all(&temp_dir).expect("failed to create temp_dir"); std::fs::create_dir_all(&home_dir).expect("failed to create home_dir"); + std::fs::create_dir_all(&storage_dir).expect("failed to create storage_dir"); + let settings_path = home_dir.join(".fabro/settings.toml"); + if let Some(parent) = settings_path.parent() { + std::fs::create_dir_all(parent).expect("failed to create fabro settings dir"); + } + std::fs::write( + &settings_path, + format!("storage_dir = \"{}\"\n", storage_dir.display()), + ) + .expect("failed to write default fabro settings"); let filters = vec![ ( @@ -686,11 +696,23 @@ impl TestContext { path: impl AsRef, content: impl AsRef<[u8]>, ) -> &Self { + let path = path.as_ref(); let full = self.home_dir.join(path); if let Some(parent) = full.parent() { std::fs::create_dir_all(parent).expect("failed to create parent dirs"); } - std::fs::write(&full, content).expect("failed to write file"); + let content = content.as_ref(); + let effective_content = if path == std::path::Path::new(".fabro/settings.toml") + && !String::from_utf8_lossy(content).contains("storage_dir") + { + let mut merged = + format!("storage_dir = \"{}\"\n", self.storage_dir.display()).into_bytes(); + merged.extend_from_slice(content); + merged + } else { + content.to_vec() + }; + std::fs::write(&full, effective_content).expect("failed to write file"); self } diff --git a/lib/crates/fabro-types/src/settings/mod.rs b/lib/crates/fabro-types/src/settings/mod.rs index f31d0348b..bb3285fe4 100644 --- a/lib/crates/fabro-types/src/settings/mod.rs +++ b/lib/crates/fabro-types/src/settings/mod.rs @@ -183,9 +183,25 @@ impl Settings { pub fn storage_dir(&self) -> PathBuf { self.storage_dir.clone().unwrap_or_else(|| { std::env::var_os("FABRO_HOME") - .map(PathBuf::from) + .map(|root| PathBuf::from(root).join("storage")) .or_else(|| dirs::home_dir().map(|home| home.join(".fabro"))) - .unwrap_or_else(|| PathBuf::from(".fabro")) + .map(|root| root.join("storage")) + .unwrap_or_else(|| PathBuf::from(".fabro/storage")) }) } } + +#[cfg(test)] +mod tests { + use super::Settings; + + #[test] + fn storage_dir_defaults_to_home_storage_subdir() { + let home = dirs::home_dir().expect("home directory should be available for tests"); + + assert_eq!( + Settings::default().storage_dir(), + home.join(".fabro/storage") + ); + } +}