From ef81c1c245ff0ca4b0547aa8f00dc687b6bae238 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 29 Apr 2026 15:45:37 -0400 Subject: [PATCH] fix(run): enforce validation when create_run is quiet The bail!("Validation failed") was nested inside `if !quiet`, so `fabro run --detach` and `fabro run create` (both pass quiet=true) silently created runs from invalid workflows. Move the bail outside the gate; only the workflow summary print remains gated on !quiet. Also simplifies the surrounding preflight code: - Extract `cyan_spinner` helper in fabro-cli's shared utilities; collapse three copy-pasted 13-line spinner setups in preflight.rs, doctor.rs, and install.rs. - Add `SandboxProvider::is_clone_based()`; replace the local `is_clone_based_provider` helper and two inline `matches!(_, Docker | Daytona)` sites in run_manifest.rs. - Promote `fabro_sandbox::redact::redact_auth_url` to pub and reuse it; delete the duplicate `redact_remote_output` in run_manifest.rs. - Inline the one-liner `preflight_docker_config` / `preflight_daytona_config` helpers and drop their dedicated tests. - Type the `prepared_and_resolved_for_sandbox` test helper with `SandboxProvider` instead of `&str`. - Drop git ls-remote preflight timeout from 30s to 10s. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-cli/src/commands/doctor.rs | 16 +--- lib/crates/fabro-cli/src/commands/install.rs | 10 +- .../fabro-cli/src/commands/preflight.rs | 16 +--- .../fabro-cli/src/commands/run/create.rs | 20 ++-- lib/crates/fabro-cli/src/shared/utilities.rs | 13 +++ lib/crates/fabro-sandbox/src/lib.rs | 2 +- lib/crates/fabro-sandbox/src/redact.rs | 5 +- .../fabro-sandbox/src/sandbox_provider.rs | 7 ++ lib/crates/fabro-server/src/run_manifest.rs | 93 +++++-------------- 9 files changed, 58 insertions(+), 124 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/doctor.rs b/lib/crates/fabro-cli/src/commands/doctor.rs index ee0c48e91..4765c7da1 100644 --- a/lib/crates/fabro-cli/src/commands/doctor.rs +++ b/lib/crates/fabro-cli/src/commands/doctor.rs @@ -14,7 +14,7 @@ use fabro_util::version::FABRO_VERSION; use crate::args::DoctorArgs; use crate::command_context::CommandContext; -use crate::shared::print_json_pretty; +use crate::shared::{cyan_spinner, print_json_pretty}; pub(crate) fn check_config(settings_path: Option) -> CheckResult { match settings_path { @@ -250,19 +250,7 @@ pub(crate) async fn run_doctor( let printer = base_ctx.printer(); let styles = Styles::detect_stdout(); let json = base_ctx.json_output(); - let spinner = if json { - None - } else { - let spinner = indicatif::ProgressBar::new_spinner(); - spinner.set_style( - indicatif::ProgressStyle::with_template("{spinner:.cyan} {msg}") - .expect("valid template") - .tick_strings(&["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏", ""]), - ); - spinner.set_message("Running checks..."); - spinner.enable_steady_tick(std::time::Duration::from_millis(80)); - Some(spinner) - }; + let spinner = (!json).then(|| cyan_spinner("Running checks...")); let settings_config_path = active_settings_path(None); diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index a5bba5c24..89de9c20e 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -58,6 +58,7 @@ use crate::args::{ use crate::command_context::CommandContext; use crate::commands::server::{start, stop}; use crate::gh::GhCli; +use crate::shared::cyan_spinner; use crate::shared::provider_auth::{ ApiKeySource, authenticate_provider, authenticate_provider_with_api_key_source, authenticate_provider_with_method, prompt_confirm, prompt_password, provider_display_name, @@ -791,14 +792,7 @@ async fn best_effort_github_username() -> Option { /// /// Returns `(owner, username)`. async fn prompt_github_app_owner(_s: &Styles) -> Result<(GitHubAppOwner, Option)> { - let spinner = indicatif::ProgressBar::new_spinner(); - spinner.set_style( - indicatif::ProgressStyle::with_template("{spinner:.cyan} {msg}") - .expect("valid template") - .tick_strings(&["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏", ""]), - ); - spinner.set_message("Checking GitHub CLI..."); - spinner.enable_steady_tick(std::time::Duration::from_millis(80)); + let spinner = cyan_spinner("Checking GitHub CLI..."); let Some(gh) = GhCli::detect().await else { spinner.finish_and_clear(); diff --git a/lib/crates/fabro-cli/src/commands/preflight.rs b/lib/crates/fabro-cli/src/commands/preflight.rs index 428907af7..1ea977593 100644 --- a/lib/crates/fabro-cli/src/commands/preflight.rs +++ b/lib/crates/fabro-cli/src/commands/preflight.rs @@ -9,7 +9,7 @@ use crate::commands::run::output::{ }; use crate::commands::run::overrides::preflight_args_overrides; use crate::manifest_builder::{ManifestBuildInput, build_run_manifest, preflight_manifest_args}; -use crate::shared::print_json_pretty; +use crate::shared::{cyan_spinner, print_json_pretty}; pub(crate) async fn execute( mut args: PreflightArgs, @@ -31,19 +31,7 @@ pub(crate) async fn execute( user_settings_path: Some(active_settings_path(None)), })?; - let spinner = if ctx.json_output() { - None - } else { - let spinner = indicatif::ProgressBar::new_spinner(); - spinner.set_style( - indicatif::ProgressStyle::with_template("{spinner:.cyan} {msg}") - .expect("valid template") - .tick_strings(&["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏", ""]), - ); - spinner.set_message("Running checks..."); - spinner.enable_steady_tick(std::time::Duration::from_millis(80)); - Some(spinner) - }; + let spinner = (!ctx.json_output()).then(|| cyan_spinner("Running checks...")); let result = async { let client = ctx.server().await?; diff --git a/lib/crates/fabro-cli/src/commands/run/create.rs b/lib/crates/fabro-cli/src/commands/run/create.rs index f6fcf0511..4aac223ce 100644 --- a/lib/crates/fabro-cli/src/commands/run/create.rs +++ b/lib/crates/fabro-cli/src/commands/run/create.rs @@ -47,23 +47,21 @@ pub(crate) async fn create_run( run_id, user_settings_path: Some(active_settings_path(None)), })?; + let validation = manifest_validation::validate_manifest(&RunLayer::default(), &built.manifest)?; + let diagnostics = api_diagnostics_to_local(&validation.workflow.diagnostics); if !quiet { - let printer = ctx.printer(); - let validation = - manifest_validation::validate_manifest(&RunLayer::default(), &built.manifest)?; - let diagnostics = api_diagnostics_to_local(&validation.workflow.diagnostics); print_workflow_summary( &validation.workflow, Some(&built.target_path), styles, - printer, + ctx.printer(), ); - if diagnostics - .iter() - .any(|diagnostic| diagnostic.severity == fabro_validate::Severity::Error) - { - bail!("Validation failed"); - } + } + if diagnostics + .iter() + .any(|diagnostic| diagnostic.severity == fabro_validate::Severity::Error) + { + bail!("Validation failed"); } let client = ctx.server().await?; diff --git a/lib/crates/fabro-cli/src/shared/utilities.rs b/lib/crates/fabro-cli/src/shared/utilities.rs index 2dd06587f..d682e4e58 100644 --- a/lib/crates/fabro-cli/src/shared/utilities.rs +++ b/lib/crates/fabro-cli/src/shared/utilities.rs @@ -16,8 +16,21 @@ use fabro_types::RunStatus; use fabro_util::printer::Printer; use fabro_util::terminal::Styles; use fabro_validate::{Diagnostic, Severity}; +use indicatif::{ProgressBar, ProgressStyle}; use serde::Serialize; +pub(crate) fn cyan_spinner(message: impl Into>) -> ProgressBar { + let spinner = ProgressBar::new_spinner(); + spinner.set_style( + ProgressStyle::with_template("{spinner:.cyan} {msg}") + .expect("valid template") + .tick_strings(&["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏", ""]), + ); + spinner.set_message(message); + spinner.enable_steady_tick(Duration::from_millis(80)); + spinner +} + pub(crate) fn read_workflow_file(path: &Path) -> anyhow::Result { std::fs::read_to_string(path) .map_err(|e| anyhow::anyhow!("Failed to read {}: {e}", path.display())) diff --git a/lib/crates/fabro-sandbox/src/lib.rs b/lib/crates/fabro-sandbox/src/lib.rs index efd6e1038..3b4a77aab 100644 --- a/lib/crates/fabro-sandbox/src/lib.rs +++ b/lib/crates/fabro-sandbox/src/lib.rs @@ -9,7 +9,7 @@ mod clone_source; pub mod read_guard; #[cfg(any(feature = "docker", feature = "daytona", test))] -pub(crate) mod redact; +pub mod redact; pub mod reconnect; diff --git a/lib/crates/fabro-sandbox/src/redact.rs b/lib/crates/fabro-sandbox/src/redact.rs index f74ac6194..12afa2fd6 100644 --- a/lib/crates/fabro-sandbox/src/redact.rs +++ b/lib/crates/fabro-sandbox/src/redact.rs @@ -1,7 +1,4 @@ -pub(crate) fn redact_auth_url( - text: &str, - auth_url: Option<&fabro_redact::DisplaySafeUrl>, -) -> String { +pub fn redact_auth_url(text: &str, auth_url: Option<&fabro_redact::DisplaySafeUrl>) -> String { let Some(auth_url) = auth_url else { return text.to_string(); }; diff --git a/lib/crates/fabro-sandbox/src/sandbox_provider.rs b/lib/crates/fabro-sandbox/src/sandbox_provider.rs index f9cfe32c4..587b19786 100644 --- a/lib/crates/fabro-sandbox/src/sandbox_provider.rs +++ b/lib/crates/fabro-sandbox/src/sandbox_provider.rs @@ -20,6 +20,13 @@ impl SandboxProvider { pub fn is_local(&self) -> bool { matches!(self, Self::Local) } + + /// True for providers that clone repository sources into their workspace + /// (Docker, Daytona). Used by preflight to decide whether repository + /// access checks apply. + pub fn is_clone_based(&self) -> bool { + matches!(self, Self::Docker | Self::Daytona) + } } #[cfg(test)] diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 2dfe891ab..044e3a576 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -16,11 +16,11 @@ use fabro_graphviz::graph::{Graph, is_llm_handler_type}; use fabro_graphviz::render::apply_direction; use fabro_llm::Provider; use fabro_model::Catalog; -use fabro_redact::DisplaySafeUrl; use fabro_sandbox::config::{ DaytonaNetwork, DaytonaSnapshotSettings, DockerfileSource as SandboxDockerfileSource, }; use fabro_sandbox::daytona::DaytonaConfig; +use fabro_sandbox::redact::redact_auth_url; use fabro_sandbox::{DockerSandboxOptions, Sandbox, SandboxProvider, SandboxSpec}; use fabro_static::EnvVars; use fabro_types::settings::ServerNamespace; @@ -489,10 +489,8 @@ async fn build_preflight_report( } else { sandbox_provider }; - let needs_github_credentials = matches!( - sandbox_provider, - SandboxProvider::Docker | SandboxProvider::Daytona - ) || !github_integration.permissions.is_empty(); + let needs_github_credentials = + sandbox_provider.is_clone_based() || !github_integration.permissions.is_empty(); let github_app = if needs_github_credentials { state .github_credentials(github_integration) @@ -618,28 +616,12 @@ fn resolve_docker_config(settings: &RunNamespace) -> Option DockerSandboxOptions { - let mut config = config.clone(); - config.skip_clone = true; - config -} - -fn preflight_daytona_config(config: &DaytonaConfig) -> DaytonaConfig { - let mut config = config.clone(); - config.skip_clone = true; - config -} - #[derive(Clone, Debug, PartialEq, Eq)] struct GitRemoteRefCheck { origin_url: String, branch: Option, } -fn is_clone_based_provider(provider: SandboxProvider) -> bool { - matches!(provider, SandboxProvider::Docker | SandboxProvider::Daytona) -} - fn clone_disabled_for_provider(provider: SandboxProvider, resolved_run: &RunNamespace) -> bool { match provider { SandboxProvider::Docker => resolved_run @@ -694,7 +676,7 @@ where F: FnOnce(GitRemoteRefCheck, Option) -> Fut, Fut: Future>, { - if !is_clone_based_provider(sandbox_provider) + if !sandbox_provider.is_clone_based() || clone_disabled_for_provider(sandbox_provider, resolved_run) { return true; @@ -778,9 +760,9 @@ async fn check_git_remote_ref( command.arg(branch); } - let output = time::timeout(Duration::from_secs(30), command.output()) + let output = time::timeout(Duration::from_secs(10), command.output()) .await - .map_err(|_| "git ls-remote timed out after 30s".to_string())? + .map_err(|_| "git ls-remote timed out after 10s".to_string())? .map_err(|err| format!("Failed to run git ls-remote: {err}"))?; if output.status.success() { @@ -796,14 +778,7 @@ async fn check_git_remote_ref( } else { format!("git ls-remote exited with status {}", output.status) }; - Err(redact_remote_output(&message, auth_url.as_ref())) -} - -fn redact_remote_output(text: &str, auth_url: Option<&DisplaySafeUrl>) -> String { - auth_url.map_or_else( - || text.to_string(), - |url| text.replace(url.as_raw_url().as_str(), &url.redacted_string()), - ) + Err(redact_auth_url(&message, auth_url.as_ref())) } fn preflight_sandbox_spec( @@ -824,9 +799,10 @@ fn preflight_sandbox_spec( working_directory: prepared.source_directory.clone(), }, SandboxProvider::Docker => { - let config = resolve_docker_config(resolved_run).unwrap_or_default(); + let mut config = resolve_docker_config(resolved_run).unwrap_or_default(); + config.skip_clone = true; SandboxSpec::Docker { - config: preflight_docker_config(&config), + config, github_app, run_id: None, clone_origin_url, @@ -834,9 +810,10 @@ fn preflight_sandbox_spec( } } SandboxProvider::Daytona => { - let config = resolve_daytona_config(resolved_run).unwrap_or_default(); + let mut config = resolve_daytona_config(resolved_run).unwrap_or_default(); + config.skip_clone = true; SandboxSpec::Daytona { - config: preflight_daytona_config(&config), + config, github_app, run_id: None, clone_origin_url, @@ -874,10 +851,8 @@ async fn run_sandbox_check( Ok(sandbox) => match sandbox.initialize().await { Ok(()) => { let mut details = vec![CheckDetail::new(format!("Provider: {sandbox_provider}"))]; - if matches!( - sandbox_provider, - SandboxProvider::Docker | SandboxProvider::Daytona - ) && prepared.git.is_none() + if sandbox_provider.is_clone_based() + && prepared.git.is_none() && !clone_disabled_for_provider(sandbox_provider, resolved_run) { details.push(CheckDetail { @@ -1386,7 +1361,7 @@ mod tests { } fn prepared_and_resolved_for_sandbox( - provider: &str, + provider: SandboxProvider, skip_clone: bool, git: Option, ) -> (PreparedManifest, RunNamespace) { @@ -1425,36 +1400,10 @@ skip_clone = {skip_clone} (prepared, resolved) } - #[test] - fn preflight_docker_config_forces_skip_clone_without_mutating_runtime_config() { - let runtime = DockerSandboxOptions { - skip_clone: false, - ..DockerSandboxOptions::default() - }; - - let preflight = preflight_docker_config(&runtime); - - assert!(preflight.skip_clone); - assert!(!runtime.skip_clone); - } - - #[test] - fn preflight_daytona_config_forces_skip_clone_without_mutating_runtime_config() { - let runtime = DaytonaConfig { - skip_clone: false, - ..DaytonaConfig::default() - }; - - let preflight = preflight_daytona_config(&runtime); - - assert!(preflight.skip_clone); - assert!(!runtime.skip_clone); - } - #[tokio::test] async fn repository_access_check_skips_when_clone_is_disabled() { let (prepared, resolved) = prepared_and_resolved_for_sandbox( - "docker", + SandboxProvider::Docker, true, Some(git_context("https://github.com/acme/widgets", "main")), ); @@ -1483,7 +1432,7 @@ skip_clone = {skip_clone} #[tokio::test] async fn repository_access_check_rejects_non_github_origins_before_remote_probe() { let (prepared, resolved) = prepared_and_resolved_for_sandbox( - "docker", + SandboxProvider::Docker, false, Some(git_context("https://gitlab.com/acme/widgets", "main")), ); @@ -1521,7 +1470,7 @@ skip_clone = {skip_clone} #[tokio::test] async fn repository_access_check_probes_normalized_github_branch() { let (prepared, resolved) = prepared_and_resolved_for_sandbox( - "docker", + SandboxProvider::Docker, false, Some(git_context( "git@github.com:acme/widgets.git", @@ -1558,7 +1507,7 @@ skip_clone = {skip_clone} #[tokio::test] async fn repository_access_check_surfaces_remote_probe_failure() { let (prepared, resolved) = prepared_and_resolved_for_sandbox( - "docker", + SandboxProvider::Docker, false, Some(git_context("https://github.com/acme/widgets", "missing")), ); @@ -1590,7 +1539,7 @@ skip_clone = {skip_clone} #[test] fn preflight_sandbox_spec_disables_docker_clone_but_preserves_clone_metadata() { let (prepared, resolved) = prepared_and_resolved_for_sandbox( - "docker", + SandboxProvider::Docker, false, Some(git_context("https://github.com/acme/widgets", "main")), );