From a1fd66c8a80d95fe96e68c7ac7cdffc4f3d9f0c8 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 5 Apr 2026 21:22:04 -0400 Subject: [PATCH] refactor(cli): target run and create commands via server connection Allow run and create to resolve the same explicit or configured server connection model used by preflight, validate, and graph. This removes the last local-only submission assumption from the CLI surface while keeping local storage-backed behavior intact when no remote target is selected. --- lib/crates/fabro-cli/src/args.rs | 13 +- lib/crates/fabro-cli/src/commands/install.rs | 1 + .../fabro-cli/src/commands/run/attach.rs | 56 ++- .../fabro-cli/src/commands/run/command.rs | 30 +- .../fabro-cli/src/commands/run/create.rs | 25 +- lib/crates/fabro-cli/src/commands/run/mod.rs | 8 +- .../fabro-cli/src/commands/run/output.rs | 68 +-- .../fabro-cli/src/commands/run/start.rs | 8 + lib/crates/fabro-cli/src/main.rs | 123 +++++- lib/crates/fabro-cli/src/server_client.rs | 19 +- lib/crates/fabro-cli/src/user_config.rs | 6 +- lib/crates/fabro-cli/tests/it/cmd/create.rs | 165 ++++++- lib/crates/fabro-cli/tests/it/cmd/run.rs | 410 +++++++++++++++++- 13 files changed, 843 insertions(+), 89 deletions(-) diff --git a/lib/crates/fabro-cli/src/args.rs b/lib/crates/fabro-cli/src/args.rs index 6fa3f671e..b3797c6a8 100644 --- a/lib/crates/fabro-cli/src/args.rs +++ b/lib/crates/fabro-cli/src/args.rs @@ -77,12 +77,15 @@ impl ServerTargetArgs { #[derive(Args, Debug, Clone, Default)] pub(crate) struct ServerConnectionArgs { /// Local storage directory (default: ~/.fabro) - #[arg(long, env = "FABRO_STORAGE_DIR", conflicts_with = "server")] + #[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", conflicts_with = "storage_dir")] + #[arg(long = "server", env = "FABRO_SERVER")] pub(crate) server: Option, + + #[arg(skip)] + pub(crate) storage_dir_explicit: bool, } impl ServerConnectionArgs { @@ -93,6 +96,10 @@ impl ServerConnectionArgs { 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)] @@ -125,7 +132,7 @@ impl From for CliSandboxProvider { #[derive(Args)] pub(crate) struct RunArgs { #[command(flatten)] - pub(crate) storage_dir: StorageDirArgs, + pub(crate) target: ServerConnectionArgs, /// Path to a .fabro workflow file or .toml task config #[arg(required = true)] diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index 20420ec73..0742814ae 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -808,6 +808,7 @@ pub(crate) async fn run_install(args: &InstallArgs, globals: &GlobalArgs) -> Res target: ServerConnectionArgs { storage_dir: Some(storage_dir.clone()), server: None, + storage_dir_explicit: true, }, verbose: true, }; diff --git a/lib/crates/fabro-cli/src/commands/run/attach.rs b/lib/crates/fabro-cli/src/commands/run/attach.rs index 4201e1336..e9ac16d4f 100644 --- a/lib/crates/fabro-cli/src/commands/run/attach.rs +++ b/lib/crates/fabro-cli/src/commands/run/attach.rs @@ -47,29 +47,7 @@ pub(crate) async fn attach_run( if let (Some(storage_dir), Some(run_id)) = (storage_dir.as_deref(), run_id.as_ref()) { let client = server_client::connect_server(storage_dir).await?; - let state = client.get_run_state(run_id).await?; - let verbose = state - .run - .as_ref() - .is_some_and(|record| record.settings.verbose_enabled()); - let events = client.list_run_events(run_id, None, None).await?; - let event_lines = events - .iter() - .map(event_payload_line) - .collect::>>()?; - let initial_exit_code = events.iter().rev().find_map(event_exit_code); - return attach_run_server( - &client, - run_id, - verbose, - event_lines, - events.last().map_or(0, |event| event.seq), - initial_exit_code, - kill_on_detach, - styles, - json_output, - ) - .await; + return attach_run_with_client(&client, run_id, kill_on_detach, styles, json_output).await; } Err(anyhow::anyhow!( @@ -77,6 +55,38 @@ pub(crate) async fn attach_run( )) } +pub(crate) async fn attach_run_with_client( + client: &server_client::ServerStoreClient, + run_id: &RunId, + kill_on_detach: bool, + styles: &'static Styles, + json_output: bool, +) -> Result { + let state = client.get_run_state(run_id).await?; + let verbose = state + .run + .as_ref() + .is_some_and(|record| record.settings.verbose_enabled()); + let events = client.list_run_events(run_id, None, None).await?; + let event_lines = events + .iter() + .map(event_payload_line) + .collect::>>()?; + let initial_exit_code = events.iter().rev().find_map(event_exit_code); + attach_run_server( + client, + run_id, + verbose, + event_lines, + events.last().map_or(0, |event| event.seq), + initial_exit_code, + kill_on_detach, + styles, + json_output, + ) + .await +} + async fn attach_run_server( client: &server_client::ServerStoreClient, run_id: &RunId, diff --git a/lib/crates/fabro-cli/src/commands/run/command.rs b/lib/crates/fabro-cli/src/commands/run/command.rs index 8bd81a368..fab1d50da 100644 --- a/lib/crates/fabro-cli/src/commands/run/command.rs +++ b/lib/crates/fabro-cli/src/commands/run/command.rs @@ -2,19 +2,19 @@ use anyhow::Result; use fabro_util::terminal::Styles; use crate::args::{GlobalArgs, RunArgs}; +use crate::server_client; use crate::shared::print_json_pretty; use crate::user_config::{self, user_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_user_settings_with_storage_dir(args.storage_dir.as_deref())?; - let cli = user_layer_with_storage_dir(args.storage_dir.as_deref())?; + let cli_settings = user_config::load_user_settings_with_storage_dir(args.target.storage_dir())?; + let cli = user_layer_with_storage_dir(args.target.storage_dir())?; args.verbose = args.verbose || cli_settings.verbose_enabled(); let quiet = args.detach; let prevent_idle_sleep = cli_settings.prevent_idle_sleep_enabled(); - let (run_id, run_dir) = Box::pin(super::create::create_run(&args, cli, styles, quiet)).await?; + let created_run = Box::pin(super::create::create_run(&args, cli, styles, quiet)).await?; #[cfg(feature = "sleep_inhibitor")] let _sleep_guard = crate::sleep_inhibitor::guard(prevent_idle_sleep); @@ -22,29 +22,29 @@ pub(crate) async fn execute(mut args: RunArgs, globals: &GlobalArgs) -> Result<( #[cfg(not(feature = "sleep_inhibitor"))] let _ = prevent_idle_sleep; - super::start::start_run(&run_id, &cli_settings.storage_dir(), false).await?; + let client = server_client::connect_server_connection(&created_run.connection).await?; + super::start::start_run_with_client(&client, &created_run.run_id, false).await?; if args.detach { if globals.json { - print_json_pretty(&serde_json::json!({ "run_id": run_id }))?; + print_json_pretty(&serde_json::json!({ "run_id": created_run.run_id }))?; } else { - println!("{run_id}"); + println!("{}", created_run.run_id); } } else { - let exit_code = super::attach::attach_run( - &run_dir, - Some(cli_settings.storage_dir().as_path()), - Some(&run_id), + let exit_code = super::attach::attach_run_with_client( + &client, + &created_run.run_id, true, styles, globals.json, ) .await?; if !globals.json { - super::output::print_run_summary( - cli_settings.storage_dir().as_path(), - &run_dir, - run_id, + super::output::print_run_summary_with_client( + &client, + &created_run.run_id, + created_run.local_run_dir.as_deref(), styles, ) .await?; diff --git a/lib/crates/fabro-cli/src/commands/run/create.rs b/lib/crates/fabro-cli/src/commands/run/create.rs index e8fe8ac5d..0e8c484d0 100644 --- a/lib/crates/fabro-cli/src/commands/run/create.rs +++ b/lib/crates/fabro-cli/src/commands/run/create.rs @@ -9,6 +9,13 @@ use fabro_workflow::operations::make_run_dir; 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}; + +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). /// @@ -18,7 +25,7 @@ pub(crate) async fn create_run( cli_defaults: ConfigLayer, styles: &Styles, quiet: bool, -) -> anyhow::Result<(RunId, PathBuf)> { +) -> anyhow::Result { let workflow_path = args .workflow .as_ref() @@ -45,7 +52,8 @@ pub(crate) async fn create_run( run_id, })?; - let client = server_client::connect_server(settings.storage_dir().as_path()).await?; + let connection = user_config::server_backed_command_connection(&args.target, &settings)?; + let client = server_client::connect_server_connection(&connection).await?; if !quiet { let preflight = client.run_preflight(built.manifest.clone()).await?; let diagnostics = api_diagnostics_to_local(&preflight.workflow.diagnostics); @@ -58,7 +66,16 @@ pub(crate) async fn create_run( } let created_run_id = client.create_run_from_manifest(built.manifest).await?; - let run_dir = make_run_dir(&settings.storage_dir().join("runs"), &created_run_id); + let local_run_dir = match &connection { + ServerConnection::Local { storage_dir } => { + Some(make_run_dir(&storage_dir.join("runs"), &created_run_id)) + } + ServerConnection::Target(_) => None, + }; - Ok((created_run_id, run_dir)) + 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 9446c32c2..17dd2272b 100644 --- a/lib/crates/fabro-cli/src/commands/run/mod.rs +++ b/lib/crates/fabro-cli/src/commands/run/mod.rs @@ -39,12 +39,12 @@ 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 = user_layer_with_storage_dir(args.storage_dir.as_deref())?; - let (run_id, _run_dir) = Box::pin(create::create_run(&args, cli, styles, true)).await?; + let cli = user_layer_with_storage_dir(args.target.storage_dir())?; + let created_run = Box::pin(create::create_run(&args, cli, styles, true)).await?; if globals.json { - print_json_pretty(&serde_json::json!({ "run_id": run_id }))?; + print_json_pretty(&serde_json::json!({ "run_id": created_run.run_id }))?; } else { - println!("{run_id}"); + println!("{}", created_run.run_id); } Ok(()) } diff --git a/lib/crates/fabro-cli/src/commands/run/output.rs b/lib/crates/fabro-cli/src/commands/run/output.rs index ab04a0c55..623be7cf3 100644 --- a/lib/crates/fabro-cli/src/commands/run/output.rs +++ b/lib/crates/fabro-cli/src/commands/run/output.rs @@ -128,42 +128,50 @@ pub(crate) async fn print_run_summary( run_id: impl std::fmt::Display, styles: &Styles, ) -> Result<()> { - let run_id = run_id.to_string(); - let (checkpoint, conclusion, pr_url) = match run_id.parse() { - Ok(parsed_run_id) => { - let client = server_client::connect_server(storage_dir).await?; - let run_state = client.get_run_state(&parsed_run_id).await?; - let checkpoint = run_state.checkpoint.clone(); - let conclusion = run_state.conclusion.clone(); - let pr_url = run_state - .pull_request - .as_ref() - .map(|record: &PullRequestRecord| record.html_url.clone()); - (checkpoint, conclusion, pr_url) - } - Err(_) => (None, None, None), - }; + let run_id = run_id + .to_string() + .parse() + .map_err(|err| anyhow::anyhow!("invalid run ID: {err}"))?; + let client = server_client::connect_server(storage_dir).await?; + print_run_summary_with_client(&client, &run_id, Some(run_dir), styles).await +} + +pub(crate) async fn print_run_summary_with_client( + client: &server_client::ServerStoreClient, + run_id: &fabro_types::RunId, + local_run_dir: Option<&Path>, + styles: &Styles, +) -> Result<()> { + let run_state = client.get_run_state(run_id).await?; + let checkpoint = run_state.checkpoint.clone(); + let conclusion = run_state.conclusion.clone(); + let pr_url = run_state + .pull_request + .as_ref() + .map(|record: &PullRequestRecord| record.html_url.clone()); let Some(conclusion) = conclusion else { return Ok(()); }; print_run_conclusion( &conclusion, - &run_id, - run_dir, + run_id, + local_run_dir, None, pr_url.as_deref(), styles, ); - print_final_output(checkpoint.as_ref(), run_dir, styles); - print_assets(run_dir, styles); + print_final_output(checkpoint.as_ref(), styles); + if let Some(run_dir) = local_run_dir { + print_assets(run_dir, styles); + } Ok(()) } pub(crate) fn print_run_conclusion( conclusion: &Conclusion, run_id: impl std::fmt::Display, - run_dir: &Path, + run_dir: Option<&Path>, pushed_branch: Option<&str>, pr_url: Option<&str>, styles: &Styles, @@ -227,12 +235,14 @@ pub(crate) fn print_run_conclusion( } } - eprintln!( - "{}", - styles - .dim - .apply_to(format!("Run: {}", tilde_path(run_dir))) - ); + if let Some(run_dir) = run_dir { + eprintln!( + "{}", + styles + .dim + .apply_to(format!("Run: {}", tilde_path(run_dir))) + ); + } if let Some(ref failure) = conclusion.failure_reason { eprintln!("Failure: {}", styles.red.apply_to(failure)); @@ -249,11 +259,7 @@ pub(crate) fn print_run_conclusion( } } -pub(crate) fn print_final_output( - checkpoint: Option<&fabro_types::Checkpoint>, - _run_dir: &Path, - styles: &Styles, -) { +pub(crate) fn print_final_output(checkpoint: Option<&fabro_types::Checkpoint>, styles: &Styles) { let Some(checkpoint) = checkpoint else { return; }; diff --git a/lib/crates/fabro-cli/src/commands/run/start.rs b/lib/crates/fabro-cli/src/commands/run/start.rs index 3066e48a7..6931c0d14 100644 --- a/lib/crates/fabro-cli/src/commands/run/start.rs +++ b/lib/crates/fabro-cli/src/commands/run/start.rs @@ -8,5 +8,13 @@ use crate::server_client; /// Queue a run for server-owned execution. pub(crate) async fn start_run(run_id: &RunId, storage_dir: &Path, resume: bool) -> Result<()> { let client = server_client::connect_server(storage_dir).await?; + start_run_with_client(&client, run_id, resume).await +} + +pub(crate) async fn start_run_with_client( + client: &server_client::ServerStoreClient, + run_id: &RunId, + resume: bool, +) -> Result<()> { client.start_run(run_id, resume).await } diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index 3561eac6c..a6c7e5d65 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, Parser}; +use clap::{CommandFactory, FromArgMatches, Parser, error::ErrorKind, parser::ValueSource}; use fabro_config::server::load_server_settings; 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; +use std::ffi::OsString; use tracing::debug; #[derive(Parser)] @@ -31,6 +32,123 @@ 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()) + } + + 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) + } + + 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] async fn main() { tel_panic::install_panic_hook(); @@ -273,7 +391,6 @@ mod tests { use args::{ Commands, ModelsCommand, ProviderCommand, ProviderNamespace, StoreCommand, StoreNamespace, }; - use clap::Parser; #[test] fn parse_provider_login_openai() { @@ -344,7 +461,7 @@ mod tests { match *cli.command { Commands::RunCmd(RunCommands::Run(args)) => { assert_eq!( - args.storage_dir.as_deref(), + args.target.storage_dir(), Some(std::path::Path::new("/tmp/fabro")) ); assert_eq!( diff --git a/lib/crates/fabro-cli/src/server_client.rs b/lib/crates/fabro-cli/src/server_client.rs index 8ac65b880..bdbf5d34d 100644 --- a/lib/crates/fabro-cli/src/server_client.rs +++ b/lib/crates/fabro-cli/src/server_client.rs @@ -114,6 +114,14 @@ pub(crate) async fn connect_server_backed( }) } +pub(crate) async fn connect_server_connection( + connection: &user_config::ServerConnection, +) -> Result { + Ok(ServerStoreClient { + client: connect_resolved_api_client(connection).await?, + }) +} + pub(crate) async fn connect_api_client(storage_dir: &Path) -> Result { let bind = start::ensure_server_running(storage_dir) .with_context(|| format!("Failed to start fabro server for {}", storage_dir.display()))?; @@ -155,7 +163,16 @@ pub(crate) fn connect_remote_api_client( tls: Option<&user_config::ClientTlsSettings>, ) -> Result { let http_client = user_config::build_server_client(tls)?; - Ok(fabro_api::Client::new_with_client(api_url, http_client)) + let normalized = normalize_remote_server_target(api_url); + Ok(fabro_api::Client::new_with_client(&normalized, http_client)) +} + +fn normalize_remote_server_target(api_url: &str) -> String { + api_url + .trim_end_matches('/') + .strip_suffix("/api/v1") + .unwrap_or(api_url.trim_end_matches('/')) + .to_string() } pub(crate) async fn connect_unix_socket_api_client(path: &Path) -> Result { diff --git a/lib/crates/fabro-cli/src/user_config.rs b/lib/crates/fabro-cli/src/user_config.rs index 2a17aa8ec..0fbe22ebe 100644 --- a/lib/crates/fabro-cli/src/user_config.rs +++ b/lib/crates/fabro-cli/src/user_config.rs @@ -115,7 +115,10 @@ fn resolve_server_connection( .as_ref() .and_then(|server| server.tls.clone()), )?) - } else if let Some(storage_dir) = args.storage_dir() { + } 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(), } @@ -207,6 +210,7 @@ mod tests { ServerConnectionArgs { storage_dir: storage_dir.map(PathBuf::from), server: server.map(str::to_string), + storage_dir_explicit: storage_dir.is_some(), } } diff --git a/lib/crates/fabro-cli/tests/it/cmd/create.rs b/lib/crates/fabro-cli/tests/it/cmd/create.rs index ae52a6017..30fe40957 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/create.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/create.rs @@ -1,3 +1,4 @@ +use httpmock::MockServer; use insta::assert_snapshot; use serde_json::json; @@ -7,6 +8,14 @@ use crate::support::{fabro_json_snapshot, unique_run_id}; use super::support::{fixture, output_stdout, resolve_run, run_count_for_test_case, run_state}; +fn run_status_response(run_id: &str, status: &str) -> serde_json::Value { + serde_json::json!({ + "id": run_id, + "status": status, + "created_at": "2026-04-05T12:00:00Z" + }) +} + #[test] fn help() { let context = test_context!(); @@ -27,11 +36,12 @@ fn help() { --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 - --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) + --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 @@ -46,6 +56,157 @@ fn help() { "); } +#[test] +fn create_uses_explicit_server_target_and_prints_remote_run_id() { + let context = test_context!(); + let server = MockServer::start(); + let run_id = unique_run_id(); + let mock = server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + then.status(201) + .header("Content-Type", "application/json") + .body(run_status_response(run_id.as_str(), "submitted").to_string()); + }); + + let output = context + .create_cmd() + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + "--dry-run", + fixture("simple.fabro").to_str().unwrap(), + ]) + .output() + .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) + ); + mock.assert(); + assert_eq!(output_stdout(&output).trim(), run_id.as_str()); +} + +#[test] +fn create_uses_configured_server_target_without_server_flag() { + let context = test_context!(); + let server = MockServer::start(); + let run_id = unique_run_id(); + let mock = server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + then.status(201) + .header("Content-Type", "application/json") + .body(run_status_response(run_id.as_str(), "submitted").to_string()); + }); + context.write_home( + ".fabro/user.toml", + format!("[server]\ntarget = \"{}/api/v1\"\n", server.base_url()), + ); + + let output = context + .create_cmd() + .args(["--dry-run", fixture("simple.fabro").to_str().unwrap()]) + .output() + .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) + ); + mock.assert(); + assert_eq!(output_stdout(&output).trim(), run_id.as_str()); +} + +#[test] +fn create_storage_dir_suppresses_configured_server_target() { + 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/user.toml", + format!("[server]\ntarget = \"{}/api/v1\"\n", server.base_url()), + ); + + let output = context + .create_cmd() + .args([ + "--storage-dir", + local_storage.to_str().unwrap(), + "--dry-run", + fixture("simple.fabro").to_str().unwrap(), + ]) + .output() + .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) + ); + mock.assert_calls(0); + assert!(!output_stdout(&output).trim().is_empty()); +} + +#[test] +fn create_cli_server_target_overrides_configured_server_target() { + let context = test_context!(); + let config_server = MockServer::start(); + let config_mock = config_server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + then.status(500) + .body("configured-server-should-not-be-used"); + }); + let cli_server = MockServer::start(); + let run_id = unique_run_id(); + let cli_mock = cli_server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + then.status(201) + .header("Content-Type", "application/json") + .body(run_status_response(run_id.as_str(), "submitted").to_string()); + }); + context.write_home( + ".fabro/user.toml", + format!( + "[server]\ntarget = \"{}/api/v1\"\n", + config_server.base_url() + ), + ); + + let output = context + .create_cmd() + .args([ + "--server", + &format!("{}/api/v1", cli_server.base_url()), + "--dry-run", + fixture("simple.fabro").to_str().unwrap(), + ]) + .output() + .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) + ); + cli_mock.assert(); + config_mock.assert_calls(0); + assert_eq!(output_stdout(&output).trim(), run_id.as_str()); +} + #[test] fn create_persists_directory_workflow_slug_and_cached_graph() { let context = test_context!(); diff --git a/lib/crates/fabro-cli/tests/it/cmd/run.rs b/lib/crates/fabro-cli/tests/it/cmd/run.rs index c19d451b0..f78918964 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/run.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/run.rs @@ -1,5 +1,6 @@ use fabro_test::{fabro_snapshot, test_context}; use fabro_types::StatusReason; +use httpmock::MockServer; use serde_json::Value; use super::support::{ @@ -10,6 +11,95 @@ use crate::support::{example_fixture, fabro_json_snapshot, run_output_filters, u const SHARED_DAEMON_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30); +fn run_status_response(run_id: &str, status: &str) -> serde_json::Value { + serde_json::json!({ + "id": run_id, + "status": status, + "created_at": "2026-04-05T12:00:00Z" + }) +} + +fn preflight_response() -> serde_json::Value { + serde_json::json!({ + "ok": true, + "workflow": { + "name": "Simple", + "graph_path": null, + "nodes": 4, + "edges": 3, + "goal": "Run tests and report results", + "diagnostics": [] + }, + "checks": { + "title": "Preflight", + "sections": [] + } + }) +} + +fn remote_run_state_response() -> serde_json::Value { + serde_json::json!({ + "run": null, + "graph_source": null, + "start": null, + "status": null, + "checkpoint": { + "timestamp": "2026-04-05T12:00:01Z", + "current_node": "exit", + "completed_nodes": ["report"], + "node_retries": {}, + "context_values": { + "response.report": "Remote output" + }, + "node_outcomes": {}, + "next_node_id": null, + "git_commit_sha": null, + "loop_failure_signatures": {}, + "restart_failure_signatures": {}, + "node_visits": {} + }, + "checkpoints": [], + "conclusion": { + "timestamp": "2026-04-05T12:00:01Z", + "status": "success", + "duration_ms": 12, + "stages": [], + "total_cost": null, + "total_retries": 0, + "total_input_tokens": 0, + "total_output_tokens": 0, + "total_cache_read_tokens": 0, + "total_cache_write_tokens": 0, + "total_reasoning_tokens": 0, + "has_pricing": false + }, + "retro": null, + "retro_prompt": null, + "retro_response": null, + "sandbox": null, + "final_patch": null, + "pull_request": null, + "nodes": {} + }) +} + +fn run_completed_event(run_id: &str) -> serde_json::Value { + serde_json::json!({ + "seq": 1, + "payload": { + "event": "run.completed", + "id": "evt-run-completed", + "run_id": run_id, + "ts": "2026-04-05T12:00:01Z", + "properties": { + "duration_ms": 12, + "artifact_count": 0, + "status": "success" + } + } + }) +} + #[test] fn help() { let context = test_context!(); @@ -30,11 +120,12 @@ fn help() { --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 - --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) + --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 @@ -49,6 +140,321 @@ fn help() { "); } +#[test] +fn detach_uses_explicit_server_target_and_prints_remote_run_id() { + let context = test_context!(); + let server = MockServer::start(); + let run_id = unique_run_id(); + let create_mock = server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + then.status(201) + .header("Content-Type", "application/json") + .body(run_status_response(run_id.as_str(), "submitted").to_string()); + }); + let start_mock = server.mock(|when, then| { + when.method("POST") + .path(format!("/api/v1/runs/{run_id}/start")); + then.status(200) + .header("Content-Type", "application/json") + .body(run_status_response(run_id.as_str(), "queued").to_string()); + }); + + let output = context + .run_cmd() + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + "--detach", + "--dry-run", + "--auto-approve", + example_fixture("simple.fabro").to_str().unwrap(), + ]) + .output() + .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) + ); + create_mock.assert(); + start_mock.assert(); + assert_eq!(output_stderr(&output), ""); + assert_eq!( + String::from_utf8_lossy(&output.stdout).trim(), + run_id.as_str() + ); +} + +#[test] +fn detach_uses_configured_server_target_without_server_flag() { + let context = test_context!(); + let server = MockServer::start(); + let run_id = unique_run_id(); + let create_mock = server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + then.status(201) + .header("Content-Type", "application/json") + .body(run_status_response(run_id.as_str(), "submitted").to_string()); + }); + let start_mock = server.mock(|when, then| { + when.method("POST") + .path(format!("/api/v1/runs/{run_id}/start")); + then.status(200) + .header("Content-Type", "application/json") + .body(run_status_response(run_id.as_str(), "queued").to_string()); + }); + context.write_home( + ".fabro/user.toml", + format!("[server]\ntarget = \"{}/api/v1\"\n", server.base_url()), + ); + + let output = context + .run_cmd() + .args([ + "--detach", + "--dry-run", + "--auto-approve", + example_fixture("simple.fabro").to_str().unwrap(), + ]) + .output() + .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) + ); + create_mock.assert(); + start_mock.assert(); + assert_eq!( + String::from_utf8_lossy(&output.stdout).trim(), + run_id.as_str() + ); +} + +#[test] +fn detach_storage_dir_suppresses_configured_server_target() { + 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/user.toml", + format!("[server]\ntarget = \"{}/api/v1\"\n", server.base_url()), + ); + + let output = context + .run_cmd() + .args([ + "--storage-dir", + local_storage.to_str().unwrap(), + "--detach", + "--dry-run", + "--auto-approve", + "--no-retro", + example_fixture("simple.fabro").to_str().unwrap(), + ]) + .output() + .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) + ); + create_mock.assert_calls(0); + start_mock.assert_calls(0); + assert!(!String::from_utf8_lossy(&output.stdout).trim().is_empty()); +} + +#[test] +fn detach_cli_server_target_overrides_configured_server_target() { + let context = test_context!(); + let config_server = MockServer::start(); + let config_create = config_server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + then.status(500) + .body("configured-server-should-not-be-used"); + }); + let config_start = config_server.mock(|when, then| { + when.method("POST").path_includes("/api/v1/runs/"); + then.status(500) + .body("configured-server-should-not-be-used"); + }); + let cli_server = MockServer::start(); + let run_id = unique_run_id(); + let cli_create = cli_server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + then.status(201) + .header("Content-Type", "application/json") + .body(run_status_response(run_id.as_str(), "submitted").to_string()); + }); + let cli_start = cli_server.mock(|when, then| { + when.method("POST") + .path(format!("/api/v1/runs/{run_id}/start")); + then.status(200) + .header("Content-Type", "application/json") + .body(run_status_response(run_id.as_str(), "queued").to_string()); + }); + context.write_home( + ".fabro/user.toml", + format!( + "[server]\ntarget = \"{}/api/v1\"\n", + config_server.base_url() + ), + ); + + let output = context + .run_cmd() + .args([ + "--server", + &format!("{}/api/v1", cli_server.base_url()), + "--detach", + "--dry-run", + "--auto-approve", + example_fixture("simple.fabro").to_str().unwrap(), + ]) + .output() + .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) + ); + cli_create.assert(); + cli_start.assert(); + config_create.assert_calls(0); + config_start.assert_calls(0); + assert_eq!( + String::from_utf8_lossy(&output.stdout).trim(), + run_id.as_str() + ); +} + +#[test] +fn remote_foreground_run_prints_server_backed_summary_without_local_run_dir() { + let context = test_context!(); + let server = MockServer::start(); + let run_id = unique_run_id(); + server.mock(|when, then| { + when.method("POST").path("/api/v1/preflight"); + then.status(200) + .header("Content-Type", "application/json") + .body(preflight_response().to_string()); + }); + server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + then.status(201) + .header("Content-Type", "application/json") + .body(run_status_response(run_id.as_str(), "submitted").to_string()); + }); + server.mock(|when, then| { + when.method("POST") + .path(format!("/api/v1/runs/{run_id}/start")); + then.status(200) + .header("Content-Type", "application/json") + .body(run_status_response(run_id.as_str(), "queued").to_string()); + }); + server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/runs/{run_id}/events")) + .query_param_missing("since_seq"); + then.status(200) + .header("Content-Type", "application/json") + .body( + serde_json::json!({ + "data": [run_completed_event(run_id.as_str())], + "meta": { "has_more": false } + }) + .to_string(), + ); + }); + server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/runs/{run_id}/events")) + .query_param("since_seq", "2"); + then.status(200) + .header("Content-Type", "application/json") + .body( + serde_json::json!({ + "data": [], + "meta": { "has_more": false } + }) + .to_string(), + ); + }); + server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/runs/{run_id}/questions")) + .query_param("page[limit]", "100") + .query_param("page[offset]", "0"); + then.status(200) + .header("Content-Type", "application/json") + .body( + serde_json::json!({ + "data": [], + "meta": { "has_more": false } + }) + .to_string(), + ); + }); + server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/runs/{run_id}/state")); + then.status(200) + .header("Content-Type", "application/json") + .body(remote_run_state_response().to_string()); + }); + + let output = context + .run_cmd() + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + "--dry-run", + "--auto-approve", + example_fixture("simple.fabro").to_str().unwrap(), + ]) + .output() + .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) + ); + + let stderr = output_stderr(&output); + assert!(stderr.contains("=== Run Result ==="), "{stderr}"); + assert!(stderr.contains("Remote output"), "{stderr}"); + assert_eq!( + stderr + .lines() + .filter(|line| line.trim_start().starts_with("Run:")) + .count(), + 1, + "{stderr}" + ); + assert!(!stderr.contains("=== Artifacts ==="), "{stderr}"); +} + #[test] fn dry_run_simple() { let context = test_context!();