diff --git a/lib/crates/fabro-cli/src/commands/preflight.rs b/lib/crates/fabro-cli/src/commands/preflight.rs index 37cba56d2..428907af7 100644 --- a/lib/crates/fabro-cli/src/commands/preflight.rs +++ b/lib/crates/fabro-cli/src/commands/preflight.rs @@ -5,7 +5,7 @@ use fabro_util::terminal::Styles; use crate::args::PreflightArgs; use crate::command_context::CommandContext; use crate::commands::run::output::{ - api_check_report_to_local, api_diagnostics_to_local, print_preflight_workflow_summary, + api_check_report_to_local, api_diagnostics_to_local, print_workflow_summary, }; use crate::commands::run::overrides::preflight_args_overrides; use crate::manifest_builder::{ManifestBuildInput, build_run_manifest, preflight_manifest_args}; @@ -59,7 +59,7 @@ pub(crate) async fn execute( if ctx.json_output() { print_json_pretty(&response)?; } else { - print_preflight_workflow_summary( + print_workflow_summary( &response.workflow, Some(&manifest.target_path), styles, diff --git a/lib/crates/fabro-cli/src/commands/run/create.rs b/lib/crates/fabro-cli/src/commands/run/create.rs index 50972569f..f6fcf0511 100644 --- a/lib/crates/fabro-cli/src/commands/run/create.rs +++ b/lib/crates/fabro-cli/src/commands/run/create.rs @@ -1,8 +1,11 @@ +use anyhow::bail; +use fabro_config::RunLayer; use fabro_config::user::active_settings_path; +use fabro_server::manifest_validation; use fabro_types::RunId; use fabro_util::terminal::Styles; -use super::output::{api_diagnostics_to_local, print_preflight_workflow_summary}; +use super::output::{api_diagnostics_to_local, print_workflow_summary}; use super::overrides::run_args_overrides; use crate::args::RunArgs; use crate::command_context::CommandContext; @@ -44,24 +47,26 @@ pub(crate) async fn create_run( run_id, user_settings_path: Some(active_settings_path(None)), })?; - let client = ctx.server().await?; if !quiet { let printer = ctx.printer(); - let preflight = client.run_preflight(built.manifest.clone()).await?; - let diagnostics = api_diagnostics_to_local(&preflight.workflow.diagnostics); - if !diagnostics + 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, + ); + if diagnostics .iter() .any(|diagnostic| diagnostic.severity == fabro_validate::Severity::Error) { - print_preflight_workflow_summary( - &preflight.workflow, - Some(&built.target_path), - styles, - printer, - ); + bail!("Validation failed"); } } + let client = ctx.server().await?; let created_run_id = client.create_run_from_manifest(built.manifest).await?; Ok(CreatedRun { diff --git a/lib/crates/fabro-cli/src/commands/run/output.rs b/lib/crates/fabro-cli/src/commands/run/output.rs index 6880fcbcb..4b4cec3d4 100644 --- a/lib/crates/fabro-cli/src/commands/run/output.rs +++ b/lib/crates/fabro-cli/src/commands/run/output.rs @@ -19,7 +19,7 @@ use indicatif::HumanDuration; use crate::server_client; use crate::shared::{format_tokens_human, format_usd_micros, print_diagnostics, relative_path}; -pub(crate) fn print_preflight_workflow_summary( +pub(crate) fn print_workflow_summary( workflow: &types::PreflightWorkflowSummary, graph_path_override: Option<&Path>, styles: &Styles, diff --git a/lib/crates/fabro-cli/tests/it/cmd/run.rs b/lib/crates/fabro-cli/tests/it/cmd/run.rs index 08b4534a5..58ce83937 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/run.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/run.rs @@ -27,24 +27,6 @@ fn run_status_response(run_id: &str, status: &str) -> serde_json::Value { }) } -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!({ "spec": null, @@ -472,11 +454,11 @@ fn remote_foreground_run_consumes_paginated_events_and_prints_server_backed_summ let context = test_context!(); let server = MockServer::start(); let run_id = unique_run_id(); - server.mock(|when, then| { + let preflight = server.mock(|when, then| { when.method("POST").path("/api/v1/preflight"); - then.status(200) + then.status(500) .header("Content-Type", "application/json") - .body(preflight_response().to_string()); + .body(serde_json::json!({ "error": "preflight should not run" }).to_string()); }); server.mock(|when, then| { when.method("POST").path("/api/v1/runs"); @@ -568,6 +550,7 @@ fn remote_foreground_run_consumes_paginated_events_and_prints_server_backed_summ String::from_utf8_lossy(&output.stdout), String::from_utf8_lossy(&output.stderr) ); + preflight.assert_calls(0); first_page.assert(); second_page.assert(); @@ -585,6 +568,45 @@ fn remote_foreground_run_consumes_paginated_events_and_prints_server_backed_summ assert!(!stderr.contains("=== Artifacts ==="), "{stderr}"); } +#[test] +fn foreground_run_rejects_invalid_workflow_before_creating_remote_run() { + let context = test_context!(); + let server = MockServer::start(); + let create = server.mock(|when, then| { + when.method("POST").path("/api/v1/runs"); + then.status(500) + .header("Content-Type", "application/json") + .body(serde_json::json!({ "error": "run should not be created" }).to_string()); + }); + + let workflow = context.install_fixture("invalid.fabro"); + let output = context + .run_cmd() + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + workflow.to_str().unwrap(), + ]) + .output() + .expect("command should execute"); + + assert!( + !output.status.success(), + "invalid run should fail:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + create.assert_calls(0); + + let stderr = output_stderr(&output); + assert!(stderr.contains("Workflow: Invalid"), "{stderr}"); + assert!( + stderr.contains("Pipeline must have exactly one start node"), + "{stderr}" + ); + assert!(stderr.contains("Validation failed"), "{stderr}"); +} + #[test] fn local_foreground_run_prints_artifact_paths_from_server_artifact_list() { let context = test_context!(); @@ -693,7 +715,7 @@ fn dry_run_simple() { fn dry_run_with_goal_file_reads_contents_into_goal() { // Regression test for the `--goal-file` flag that was previously // being silently ignored in the v2 path. The file content must end - // up in the effective goal displayed in the preflight summary. + // up in the effective goal displayed in the workflow summary. let context = test_context!(); let goal_dir = tempfile::tempdir().unwrap(); @@ -715,7 +737,7 @@ fn dry_run_with_goal_file_reads_contents_into_goal() { let stderr = String::from_utf8_lossy(&output.stderr); assert!( stderr.contains("Ship the rate-limiting feature end to end."), - "goal file content should appear in preflight summary, got:\n{stderr}" + "goal file content should appear in workflow summary, got:\n{stderr}" ); }