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) <noreply@anthropic.com>
2026-10-08 03:10:26 +00:00 · 2026-04-29 15:45:37 -04:00 · 2026-04-29 15:45:37 -04:00 · ef81c1c245
commit ef81c1c245
parent cfadfe65db
9 changed files with 58 additions and 124 deletions
--- 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<PathBuf>) -> 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);

--- 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<String> {
 ///
 /// Returns `(owner, username)`.
 async fn prompt_github_app_owner(_s: &Styles) -> Result<(GitHubAppOwner, Option<String>)> {
-    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();
--- 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?;
--- 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?;
--- 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<std::borrow::Cow<'static, str>>) -> 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<String> {
    std::fs::read_to_string(path)
        .map_err(|e| anyhow::anyhow!("Failed to read {}: {e}", path.display()))
--- 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;

--- 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();
    };
--- 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)]
--- 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
    settings.sandbox.docker.as_ref().map(runtime_docker_config)
 }

-fn preflight_docker_config(config: &DockerSandboxOptions) -> 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<String>,
 }

-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<fabro_github::GitHubCredentials>) -> Fut,
    Fut: Future<Output = Result<(), String>>,
 {
-    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<types::GitContext>,
    ) -> (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")),
        );