diff --git a/lib/apps/fabro-cli/src/commands/run/create.rs b/lib/apps/fabro-cli/src/commands/run/create.rs index ad4360384..159160d7e 100644 --- a/lib/apps/fabro-cli/src/commands/run/create.rs +++ b/lib/apps/fabro-cli/src/commands/run/create.rs @@ -61,11 +61,8 @@ pub(crate) async fn create_run( None }; - let mut validation = manifest_validation::validate_manifest( - &RunLayer::default(), - &built.manifest, - ctx.catalog()?, - )?; + let mut validation = + manifest_validation::validate_manifest(&RunLayer::default(), &built.manifest)?; manifest_validation::promote_template_undefined_variables_to_errors(&mut validation); let diagnostics = api_diagnostics_to_local(&validation.workflow.diagnostics); if !quiet { diff --git a/lib/apps/fabro-cli/src/commands/run/runner.rs b/lib/apps/fabro-cli/src/commands/run/runner.rs index 4e6b72b8b..4c6fcf578 100644 --- a/lib/apps/fabro-cli/src/commands/run/runner.rs +++ b/lib/apps/fabro-cli/src/commands/run/runner.rs @@ -263,12 +263,7 @@ impl fabro_tool::RunManifestBuilder for WorkerRunManifestBuilder { cwd: &Path, user_settings_path: &Path, ) -> fabro_tool::ToolResult { - run_tool_manifest::build_run_tool_manifest( - spec, - cwd, - user_settings_path, - Arc::clone(&self.catalog), - ) + run_tool_manifest::build_run_tool_manifest(spec, cwd, user_settings_path, &self.catalog) } } diff --git a/lib/apps/fabro-cli/src/commands/validate.rs b/lib/apps/fabro-cli/src/commands/validate.rs index b87460f28..e157b50b3 100644 --- a/lib/apps/fabro-cli/src/commands/validate.rs +++ b/lib/apps/fabro-cli/src/commands/validate.rs @@ -23,11 +23,7 @@ pub(crate) fn run( user_settings_path: Some(active_settings_path(None)), ..Default::default() })?; - let response = manifest_validation::validate_manifest( - &RunLayer::default(), - &built.manifest, - base_ctx.catalog()?, - )?; + let response = manifest_validation::validate_manifest(&RunLayer::default(), &built.manifest)?; let diagnostics = api_diagnostics_to_local(&response.workflow.diagnostics); if base_ctx.json_output() { diff --git a/lib/apps/fabro-cli/tests/it/cmd/create.rs b/lib/apps/fabro-cli/tests/it/cmd/create.rs index 5a9a4402f..462a0996f 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/create.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/create.rs @@ -101,6 +101,52 @@ fn create_uses_explicit_server_target_and_prints_remote_run_id() { assert_eq!(output_stdout(&output).trim(), run_id.as_str()); } +#[test] +fn create_defers_provider_validation_to_the_server() { + 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") + .body_includes(r#"provider=\"server-only\""#); + then.status(201) + .header("Content-Type", "application/json") + .body(run_status_response(run_id.as_str(), "submitted").to_string()); + }); + let workflow_path = context.temp_dir.join("server-model.fabro"); + context.write_temp( + "server-model.fabro", + r#"digraph ServerModel { + graph [goal="Use a server-owned model"] + start [shape=Mdiamond] + work [prompt="Do work", model="private-model", provider="server-only"] + exit [shape=Msquare] + start -> work -> exit + }"#, + ); + + let output = context + .create_cmd() + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + "--dry-run", + workflow_path.to_str().unwrap(), + ]) + .output() + .expect("command should execute"); + + assert!( + output.status.success(), + "local validation should not reject a server-owned provider\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!(); diff --git a/lib/apps/fabro-cli/tests/it/cmd/validate.rs b/lib/apps/fabro-cli/tests/it/cmd/validate.rs index 190c2817b..2bbec8c76 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/validate.rs @@ -69,6 +69,39 @@ fn simple_does_not_connect_to_configured_server() { ); } +#[test] +#[expect( + clippy::disallowed_methods, + reason = "sync CLI test writes one workflow fixture before spawning the subprocess" +)] +fn server_owned_provider_is_not_rejected_by_offline_validation() { + let cli = LightweightCli::new(); + let workflow = cli.home().join("server-model.fabro"); + std::fs::write( + &workflow, + r#"digraph ServerModel { + graph [goal="Use a server-owned model"] + start [shape=Mdiamond] + work [prompt="Do work", model="private-model", provider="server-only"] + exit [shape=Msquare] + start -> work -> exit + }"#, + ) + .expect("workflow fixture should be written"); + let mut cmd = cli.command(); + cmd.env("FABRO_SERVER", "http://127.0.0.1:9") + .arg("validate") + .arg(&workflow); + + let output = cmd.output().expect("validate should execute"); + assert!( + output.status.success(), + "offline validation should leave provider availability to the server\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); +} + #[test] fn branching() { let context = test_context!(); diff --git a/lib/apps/fabro-mcp-server/src/manifest_builder.rs b/lib/apps/fabro-mcp-server/src/manifest_builder.rs index 53e899f1e..8c1cb8125 100644 --- a/lib/apps/fabro-mcp-server/src/manifest_builder.rs +++ b/lib/apps/fabro-mcp-server/src/manifest_builder.rs @@ -32,5 +32,5 @@ fn build_mcp_run_manifest( Catalog::from_builtin_with_overrides(&llm_catalog_settings) .map_err(|err| ToolError::message(err.to_string()))?, ); - run_tool_manifest::build_run_tool_manifest(spec, cwd, user_settings_path, catalog) + run_tool_manifest::build_run_tool_manifest(spec, cwd, user_settings_path, &catalog) } diff --git a/lib/apps/fabro-server/src/manifest_validation.rs b/lib/apps/fabro-server/src/manifest_validation.rs index baca3f014..3016fde65 100644 --- a/lib/apps/fabro-server/src/manifest_validation.rs +++ b/lib/apps/fabro-server/src/manifest_validation.rs @@ -12,21 +12,34 @@ use crate::run_manifest; pub fn validate_manifest( manifest_run_defaults: &RunLayer, manifest: &types::RunManifest, - catalog: Arc, ) -> Result { validate_manifest_with_environment_defaults( manifest_run_defaults, &fabro_environment::seeded_catalog_layer(), manifest, - catalog, ) } +pub fn validate_manifest_with_catalog( + manifest_run_defaults: &RunLayer, + manifest: &types::RunManifest, + catalog: &Arc, +) -> Result { + let prepared = run_manifest::prepare_manifest_with_environment_defaults( + manifest_run_defaults, + &fabro_environment::seeded_catalog_layer(), + &HashMap::new(), + manifest, + )?; + let validated = + run_manifest::validate_prepared_manifest(&prepared, catalog).map_err(anyhow::Error::new)?; + Ok(run_manifest::validate_response(&prepared, &validated)) +} + pub fn validate_manifest_with_environment_defaults( manifest_run_defaults: &RunLayer, manifest_environment_defaults: &MergeMap, manifest: &types::RunManifest, - catalog: Arc, ) -> Result { let prepared = run_manifest::prepare_manifest_with_environment_defaults( manifest_run_defaults, @@ -34,8 +47,8 @@ pub fn validate_manifest_with_environment_defaults( &HashMap::new(), manifest, )?; - let validated = - run_manifest::validate_prepared_manifest(&prepared, catalog).map_err(anyhow::Error::new)?; + let validated = run_manifest::validate_prepared_manifest_structural(&prepared) + .map_err(anyhow::Error::new)?; Ok(run_manifest::validate_response(&prepared, &validated)) } diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index 65cc6d36e..76a81db51 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -35,7 +35,8 @@ use fabro_util::check_report::{CheckDetail, CheckReport, CheckResult, CheckSecti use fabro_validate::Severity; use fabro_workflow::Error as WorkflowError; use fabro_workflow::operations::{ - CreateRunInput, ValidateInput, WorkflowInput, validate, validate_with_ready_providers, + CreateRunInput, ValidateInput, WorkflowInput, validate, validate_with_catalog, + validate_with_ready_providers, }; use fabro_workflow::pipeline::Validated; use fabro_workflow::run_materialization::materialize_run_with_ready_providers; @@ -187,34 +188,40 @@ pub(crate) fn prepare_manifest_with_environment_defaults( pub(crate) fn validate_prepared_manifest( prepared: &PreparedManifest, - catalog: Arc, + catalog: &Arc, ) -> Result { validate_prepared_manifest_with_vars(prepared, catalog, HashMap::new()) } +pub(crate) fn validate_prepared_manifest_structural( + prepared: &PreparedManifest, +) -> Result { + validate(manifest_validate_input(prepared, HashMap::new())) +} + pub(crate) fn validate_prepared_manifest_with_vars( prepared: &PreparedManifest, - catalog: Arc, + catalog: &Arc, vars: HashMap, ) -> Result { - validate(manifest_validate_input(prepared, catalog, vars)) + validate_with_catalog(manifest_validate_input(prepared, vars), catalog) } pub(crate) fn validate_prepared_manifest_for_preflight( prepared: &PreparedManifest, - catalog: Arc, + catalog: &Arc, vars: HashMap, ready_providers: &[ProviderId], ) -> Result { validate_with_ready_providers( - manifest_validate_input(prepared, catalog, vars), + manifest_validate_input(prepared, vars), + catalog, ready_providers, ) } fn manifest_validate_input( prepared: &PreparedManifest, - catalog: Arc, vars: HashMap, ) -> ValidateInput { ValidateInput { @@ -223,7 +230,6 @@ fn manifest_validate_input( vars, cwd: prepared.cwd.clone(), custom_transforms: Vec::new(), - catalog, } } @@ -1548,7 +1554,7 @@ digraph Demo {{ .unwrap(); let validated = validate_prepared_manifest_for_preflight( &prepared, - state.catalog(), + &state.catalog(), HashMap::new(), &ready_providers, ) @@ -1633,7 +1639,7 @@ enabled = {clone_enabled} &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); let resolved = materialize_run( prepared.settings.clone(), validated.graph(), @@ -2193,7 +2199,7 @@ name = "Control Plane" &invalid_manifest(), ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); assert!(validated.has_errors()); @@ -2238,7 +2244,7 @@ issues = "read" &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); assert!(!validated.has_errors()); let (response, _ok) = resolve_and_run_preflight(state.as_ref(), &prepared, &validated) @@ -2288,7 +2294,7 @@ id = "local" &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); assert!(!validated.has_errors()); @@ -2397,7 +2403,7 @@ id = "daytona" &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); let (response, _ok) = resolve_and_run_preflight(state.as_ref(), &prepared, &validated) .await @@ -2465,7 +2471,7 @@ digraph Demo { &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); let (response, ok) = resolve_and_run_preflight(state.as_ref(), &prepared, &validated) .await @@ -2579,7 +2585,7 @@ digraph Demo { &manifest, ) .unwrap(); - let Err(error) = validate_prepared_manifest(&prepared, test_catalog()) else { + let Err(error) = validate_prepared_manifest(&prepared, &test_catalog()) else { panic!("unknown provider should fail static validation"); }; @@ -2646,7 +2652,7 @@ digraph Demo { assert!(ready_providers.is_empty()); let validated = validate_prepared_manifest_for_preflight( &prepared, - state.catalog(), + &state.catalog(), HashMap::new(), &ready_providers, ) diff --git a/lib/apps/fabro-server/src/run_tool_manifest.rs b/lib/apps/fabro-server/src/run_tool_manifest.rs index 8e38e8317..66204e208 100644 --- a/lib/apps/fabro-server/src/run_tool_manifest.rs +++ b/lib/apps/fabro-server/src/run_tool_manifest.rs @@ -14,7 +14,7 @@ pub fn build_run_tool_manifest( spec: &ValidatedCreateRunSpec, cwd: &Path, user_settings_path: &Path, - catalog: Arc, + catalog: &Arc, ) -> ToolResult { let built = fabro_manifest::build_run_manifest(ManifestBuildInput { workflow: PathBuf::from(&spec.workflow), @@ -29,9 +29,12 @@ pub fn build_run_tool_manifest( }) .map_err(|err| ToolError::from_anyhow(&err))?; - let mut validation = - manifest_validation::validate_manifest(&RunLayer::default(), &built.manifest, catalog) - .map_err(|err| ToolError::from_anyhow(&err))?; + let mut validation = manifest_validation::validate_manifest_with_catalog( + &RunLayer::default(), + &built.manifest, + catalog, + ) + .map_err(|err| ToolError::from_anyhow(&err))?; manifest_validation::promote_template_undefined_variables_to_errors(&mut validation); if !validation.ok { return Err(ToolError::message("workflow manifest validation failed")); diff --git a/lib/apps/fabro-server/src/server/handler/graph.rs b/lib/apps/fabro-server/src/server/handler/graph.rs index 364b19cad..ea95b674a 100644 --- a/lib/apps/fabro-server/src/server/handler/graph.rs +++ b/lib/apps/fabro-server/src/server/handler/graph.rs @@ -52,7 +52,7 @@ async fn render_graph_from_manifest( Ok(prepared) => prepared, Err(err) => return ApiError::bad_request(err.to_string()).into_response(), }; - let validated = match run_manifest::validate_prepared_manifest(&prepared, state.catalog()) { + let validated = match run_manifest::validate_prepared_manifest(&prepared, &state.catalog()) { Ok(validated) => validated, Err(err) => return ApiError::bad_request(err.to_string()).into_response(), }; diff --git a/lib/apps/fabro-server/src/server/handler/runs.rs b/lib/apps/fabro-server/src/server/handler/runs.rs index 0e2c8cbcb..a0cbcbf70 100644 --- a/lib/apps/fabro-server/src/server/handler/runs.rs +++ b/lib/apps/fabro-server/src/server/handler/runs.rs @@ -829,7 +829,7 @@ async fn run_preflight( let (llm_result, ready_providers) = state.resolve_llm_client_with_ready_ids().await; let mut validated = match run_manifest::validate_prepared_manifest_for_preflight( &prepared, - state.catalog(), + &state.catalog(), vars, &ready_providers, ) { @@ -879,17 +879,15 @@ async fn validate_run_manifest( return ApiError::bad_request(format!("Run config variable interpolation failed: {err}")) .into_response(); } - let validated = match run_manifest::validate_prepared_manifest_with_vars( - &prepared, - state.catalog(), - vars, - ) { - Ok(validated) => validated, - Err(WorkflowError::Parse(_)) => { - return ApiError::bad_request("Validation failed").into_response(); - } - Err(err) => return ApiError::bad_request(err.to_string()).into_response(), - }; + let validated = + match run_manifest::validate_prepared_manifest_with_vars(&prepared, &state.catalog(), vars) + { + Ok(validated) => validated, + Err(WorkflowError::Parse(_)) => { + return ApiError::bad_request("Validation failed").into_response(); + } + Err(err) => return ApiError::bad_request(err.to_string()).into_response(), + }; ( StatusCode::OK, Json(run_manifest::validate_response(&prepared, &validated)), diff --git a/lib/components/fabro-workflow/src/handler/manager_loop.rs b/lib/components/fabro-workflow/src/handler/manager_loop.rs index 98804d5e9..ddefe84be 100644 --- a/lib/components/fabro-workflow/src/handler/manager_loop.rs +++ b/lib/components/fabro-workflow/src/handler/manager_loop.rs @@ -16,7 +16,7 @@ use crate::artifact_upload::ArtifactSink; use crate::condition::evaluate_condition; use crate::context::{Context, WorkflowContext, context_diff_public, keys}; use crate::error::Error; -use crate::operations::{ValidateInput, WorkflowInput, validate}; +use crate::operations::{ValidateInput, WorkflowInput, validate_with_catalog}; use crate::outcome::{Outcome, OutcomeExt, StageOutcome}; use crate::pipeline::types::Initialized; use crate::run_options::RunOptions; @@ -65,17 +65,19 @@ fn parse_child_graph(node: &Node, services: &EngineServices) -> Result Result Some(workflow.path.clone()), WorkflowInput::Path(_) | WorkflowInput::DotSource { .. } => None, }; - let mut validated = validate(ValidateInput { - workflow, - settings: WorkflowSettings::default(), - vars: std::collections::HashMap::new(), - cwd, - custom_transforms: Vec::new(), - catalog: Arc::clone(&services.run.catalog), - })?; + let mut validated = validate_with_catalog( + ValidateInput { + workflow, + settings: WorkflowSettings::default(), + vars: std::collections::HashMap::new(), + cwd, + custom_transforms: Vec::new(), + }, + &services.run.catalog, + )?; validated.promote_template_undefined_variables_to_errors(); validated.raise_on_errors()?; let (graph, _, _) = validated.into_parts(); diff --git a/lib/components/fabro-workflow/src/operations/create.rs b/lib/components/fabro-workflow/src/operations/create.rs index 2ec40d443..4fff69898 100644 --- a/lib/components/fabro-workflow/src/operations/create.rs +++ b/lib/components/fabro-workflow/src/operations/create.rs @@ -24,7 +24,9 @@ use crate::error::Error; use crate::event::{Event, append_event, to_run_event_at}; use crate::file_resolver::FileResolver; use crate::pipeline::types::PersistOptions; -use crate::pipeline::{self, Persisted, TransformOptions, Validated}; +use crate::pipeline::{ + self, ModelResolutionOptions, Persisted, TransformOptions, Transformed, Validated, +}; use crate::records::RunSpec; use crate::run_lookup::default_scratch_base; use crate::run_materialization::materialize_run; @@ -340,6 +342,69 @@ pub(super) fn preprocess_and_validate( catalog_fallback: bool, catalog: &Arc, ) -> Result { + let model_resolution = ModelResolutionOptions { + catalog: Arc::clone(catalog), + default_provider, + eligible_providers: eligible_providers.iter().cloned().collect(), + catalog_fallback, + }; + let transformed = preprocess( + dot_source, + source_name, + current_dir, + file_resolver, + custom_transforms, + template_context, + goal_override, + render_mode, + Some(model_resolution), + )?; + Ok(pipeline::validate_with_catalog( + transformed, + catalog.as_ref(), + &[], + )) +} + +pub(super) fn preprocess_and_validate_structural( + dot_source: &str, + source_name: Option, + current_dir: Option, + file_resolver: Option>, + custom_transforms: Vec>, + template_context: TemplateContext, + goal_override: Option<&str>, + render_mode: RenderMode, +) -> Result { + let transformed = preprocess( + dot_source, + source_name, + current_dir, + file_resolver, + custom_transforms, + template_context, + goal_override, + render_mode, + None, + )?; + Ok(pipeline::validate(transformed, &[])) +} + +#[expect( + clippy::too_many_arguments, + reason = "pipeline stages have distinct source, rendering, and model-resolution inputs" +)] +fn preprocess( + dot_source: &str, + source_name: Option, + current_dir: Option, + file_resolver: Option>, + custom_transforms: Vec>, + template_context: TemplateContext, + goal_override: Option<&str>, + render_mode: RenderMode, + model_resolution: Option, +) -> Result { let mut parsed = pipeline::parse(dot_source)?; apply_goal_override(&mut parsed.graph, goal_override); @@ -350,12 +415,9 @@ pub(super) fn preprocess_and_validate( source_name, render_mode, custom_transforms, - catalog: Arc::clone(catalog), - default_provider, - eligible_providers: eligible_providers.iter().cloned().collect(), - catalog_fallback, + model_resolution, })?; - Ok(pipeline::validate(transformed, catalog.as_ref(), &[])) + Ok(transformed) } pub(super) fn template_context( @@ -462,7 +524,7 @@ mod tests { use object_store::memory::InMemory; use super::*; - use crate::operations::{ValidateInput, validate}; + use crate::operations::{ValidateInput, validate, validate_with_catalog}; use crate::pipeline::types::{GOAL_SELF_REFERENCE_RULE, TEMPLATE_UNDEFINED_VARIABLE_RULE}; use crate::workflow_bundle::BundledWorkflow; fn memory_store() -> Arc { @@ -553,17 +615,19 @@ reasoning = false } fn validate_dot(dot_source: &str, settings: WorkflowSettings) -> Validated { - validate(ValidateInput { - workflow: WorkflowInput::DotSource { - source: dot_source.to_string(), - base_dir: None, + validate_with_catalog( + ValidateInput { + workflow: WorkflowInput::DotSource { + source: dot_source.to_string(), + base_dir: None, + }, + settings, + vars: HashMap::new(), + cwd: PathBuf::from("."), + custom_transforms: Vec::new(), }, - settings, - vars: HashMap::new(), - cwd: PathBuf::from("."), - custom_transforms: Vec::new(), - catalog: test_catalog(), - }) + &test_catalog(), + ) .unwrap() } @@ -852,7 +916,6 @@ reasoning = false vars: HashMap::new(), cwd: PathBuf::from("."), custom_transforms: Vec::new(), - catalog: test_catalog(), }); assert!(result.is_err()); @@ -901,7 +964,6 @@ reasoning = false vars: HashMap::new(), cwd: dir.path().to_path_buf(), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); let file_missing = validate(ValidateInput { @@ -920,7 +982,6 @@ reasoning = false vars: HashMap::new(), cwd: dir.path().to_path_buf(), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); assert_eq!( @@ -944,7 +1005,6 @@ reasoning = false vars: HashMap::new(), cwd: dir.path().to_path_buf(), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); let file_goal = validate(ValidateInput { @@ -963,7 +1023,6 @@ reasoning = false vars: HashMap::new(), cwd: dir.path().to_path_buf(), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); assert_eq!( @@ -1055,7 +1114,6 @@ reasoning = false vars: HashMap::new(), cwd: PathBuf::from("."), custom_transforms: Vec::new(), - catalog: test_catalog(), }); assert!(result.is_err()); } @@ -1100,7 +1158,6 @@ reasoning = false vars: HashMap::new(), cwd: PathBuf::from("."), custom_transforms: vec![Box::new(TagTransform)], - catalog: test_catalog(), }) .unwrap(); validated.raise_on_errors().unwrap(); @@ -1135,7 +1192,6 @@ reasoning = false vars: HashMap::new(), cwd: dir.path().to_path_buf(), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); validated.raise_on_errors().unwrap(); @@ -1177,7 +1233,6 @@ reasoning = false vars: HashMap::new(), cwd: dir.path().to_path_buf(), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); @@ -1227,7 +1282,6 @@ reasoning = false vars: HashMap::new(), cwd: PathBuf::from("."), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); @@ -1278,7 +1332,6 @@ reasoning = false vars: HashMap::new(), cwd: PathBuf::from("."), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); diff --git a/lib/components/fabro-workflow/src/operations/mod.rs b/lib/components/fabro-workflow/src/operations/mod.rs index 38b7d9aaa..9523c5ff5 100644 --- a/lib/components/fabro-workflow/src/operations/mod.rs +++ b/lib/components/fabro-workflow/src/operations/mod.rs @@ -22,7 +22,7 @@ pub use rewind::{RewindInput, RewindOutcome, rewind}; pub use source::WorkflowInput; pub use start::{StartServices, Started, start}; pub use timeline::{ForkTarget, RunTimeline, TimelineEntry, build_timeline, timeline}; -pub use validate::{ValidateInput, validate, validate_with_ready_providers}; +pub use validate::{ValidateInput, validate, validate_with_catalog, validate_with_ready_providers}; pub use crate::pipeline::{LlmSpec, SandboxEnvSpec}; pub use crate::transforms::RenderMode; diff --git a/lib/components/fabro-workflow/src/operations/validate.rs b/lib/components/fabro-workflow/src/operations/validate.rs index c2b990f5c..7ac5e4531 100644 --- a/lib/components/fabro-workflow/src/operations/validate.rs +++ b/lib/components/fabro-workflow/src/operations/validate.rs @@ -5,7 +5,9 @@ use std::sync::Arc; use fabro_model::{Catalog, ProviderId}; use fabro_types::WorkflowSettings; -use super::create::{preprocess_and_validate, template_context}; +use super::create::{ + preprocess_and_validate, preprocess_and_validate_structural, template_context, +}; use super::source::{ResolveWorkflowInput, WorkflowInput, resolve_workflow}; use crate::error::Error; use crate::operations::RenderMode; @@ -20,20 +22,50 @@ pub struct ValidateInput { pub vars: HashMap, pub cwd: PathBuf, pub custom_transforms: Vec>, - pub catalog: Arc, } -/// Parse, transform, and validate a DOT source string. +/// Parse, transform, and structurally validate a DOT source string without a +/// model catalog. /// /// Returns `Validated` even when validation produced errors. Call /// `validated.raise_on_errors()` if the caller wants to fail fast. pub fn validate(input: ValidateInput) -> Result { - let eligible_providers = input - .catalog - .all_provider_ids() - .into_iter() - .collect::>(); - validate_with_eligible_providers(input, &eligible_providers, false) + let ValidateInput { + workflow, + settings, + vars, + cwd, + custom_transforms, + } = input; + let resolved = resolve_workflow(ResolveWorkflowInput { + workflow, + settings, + cwd, + }) + .map_err(|err| Error::Parse(err.to_string()))?; + + preprocess_and_validate_structural( + &resolved.raw_source, + resolved + .dot_path + .as_ref() + .map(|path| path.display().to_string()), + resolved.current_dir, + resolved.file_resolver, + custom_transforms, + template_context(Some(&resolved.settings), vars), + resolved.goal_override.as_deref(), + RenderMode::Structural, + ) +} + +/// Parse, transform, and validate a DOT source string against `catalog`. +pub fn validate_with_catalog( + input: ValidateInput, + catalog: &Arc, +) -> Result { + let eligible_providers = catalog.all_provider_ids().into_iter().collect::>(); + validate_with_eligible_providers(input, catalog, &eligible_providers, false) } /// Parse, transform, and validate, resolving models against the ready @@ -41,20 +73,29 @@ pub fn validate(input: ValidateInput) -> Result { /// provider-readiness selection failures. pub fn validate_with_ready_providers( input: ValidateInput, + catalog: &Arc, ready_providers: &[ProviderId], ) -> Result { - validate_with_eligible_providers(input, ready_providers, true) + validate_with_eligible_providers(input, catalog, ready_providers, true) } fn validate_with_eligible_providers( input: ValidateInput, + catalog: &Arc, eligible_providers: &[ProviderId], catalog_fallback: bool, ) -> Result { + let ValidateInput { + workflow, + settings, + vars, + cwd, + custom_transforms, + } = input; let resolved = resolve_workflow(ResolveWorkflowInput { - workflow: input.workflow, - settings: input.settings, - cwd: input.cwd, + workflow, + settings, + cwd, }) .map_err(|err| Error::Parse(err.to_string()))?; @@ -66,8 +107,8 @@ fn validate_with_eligible_providers( .map(|path| path.display().to_string()), resolved.current_dir, resolved.file_resolver, - input.custom_transforms, - template_context(Some(&resolved.settings), input.vars), + custom_transforms, + template_context(Some(&resolved.settings), vars), resolved.goal_override.as_deref(), RenderMode::Structural, resolved @@ -80,6 +121,6 @@ fn validate_with_eligible_providers( .map(fabro_model::ProviderId::new), eligible_providers, catalog_fallback, - &input.catalog, + catalog, ) } diff --git a/lib/components/fabro-workflow/src/pipeline/mod.rs b/lib/components/fabro-workflow/src/pipeline/mod.rs index d0ba5ae1b..c1cc3b4f9 100644 --- a/lib/components/fabro-workflow/src/pipeline/mod.rs +++ b/lib/components/fabro-workflow/src/pipeline/mod.rs @@ -22,8 +22,8 @@ pub use pull_request::{ }; pub use transform::transform; pub use types::{ - Concluded, Executed, FinalizeOptions, Finalized, InitOptions, Initialized, LlmSpec, Parsed, - Persisted, PullRequestOptions, ResumeState, SandboxEnvSpec, TEMPLATE_UNDEFINED_VARIABLE_RULE, - TransformOptions, Transformed, Validated, + Concluded, Executed, FinalizeOptions, Finalized, InitOptions, Initialized, LlmSpec, + ModelResolutionOptions, Parsed, Persisted, PullRequestOptions, ResumeState, SandboxEnvSpec, + TEMPLATE_UNDEFINED_VARIABLE_RULE, TransformOptions, Transformed, Validated, }; -pub use validate::validate; +pub use validate::{validate, validate_with_catalog}; diff --git a/lib/components/fabro-workflow/src/pipeline/transform.rs b/lib/components/fabro-workflow/src/pipeline/transform.rs index b399d6637..d73b26275 100644 --- a/lib/components/fabro-workflow/src/pipeline/transform.rs +++ b/lib/components/fabro-workflow/src/pipeline/transform.rs @@ -63,13 +63,17 @@ pub fn transform(parsed: Parsed, options: &TransformOptions) -> Result TransformOptions { TransformOptions { - current_dir: None, - file_resolver: None, - template_context: fabro_template::TemplateContext::new(), - source_name: None, - render_mode: crate::operations::RenderMode::Strict, - custom_transforms: vec![], - catalog: test_catalog(), - default_provider: None, - eligible_providers: Catalog::builtin().all_provider_ids(), - catalog_fallback: false, + current_dir: None, + file_resolver: None, + template_context: fabro_template::TemplateContext::new(), + source_name: None, + render_mode: crate::operations::RenderMode::Strict, + custom_transforms: vec![], + model_resolution: Some(ModelResolutionOptions::new(test_catalog())), } } @@ -177,16 +180,13 @@ mod tests { ) .unwrap(); let transformed = transform(parsed, &TransformOptions { - current_dir: Some(dir.path().to_path_buf()), - file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), - template_context: fabro_template::TemplateContext::new(), - source_name: None, - render_mode: crate::operations::RenderMode::Strict, - custom_transforms: vec![], - catalog: test_catalog(), - default_provider: None, - eligible_providers: Catalog::builtin().all_provider_ids(), - catalog_fallback: false, + current_dir: Some(dir.path().to_path_buf()), + file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), + template_context: fabro_template::TemplateContext::new(), + source_name: None, + render_mode: crate::operations::RenderMode::Strict, + custom_transforms: vec![], + model_resolution: Some(ModelResolutionOptions::new(test_catalog())), }) .unwrap(); @@ -227,21 +227,18 @@ mod tests { ) .unwrap(); let transformed = transform(parsed, &TransformOptions { - current_dir: Some(dir.path().to_path_buf()), - file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), - template_context: fabro_template::TemplateContext::new().with_inputs(HashMap::from( - [( + current_dir: Some(dir.path().to_path_buf()), + file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), + template_context: fabro_template::TemplateContext::new().with_inputs(HashMap::from([ + ( "task".to_string(), toml::Value::String("Launch".to_string()), - )], - )), - source_name: None, - render_mode: crate::operations::RenderMode::Strict, - custom_transforms: vec![], - catalog: test_catalog(), - default_provider: None, - eligible_providers: Catalog::builtin().all_provider_ids(), - catalog_fallback: false, + ), + ])), + source_name: None, + render_mode: crate::operations::RenderMode::Strict, + custom_transforms: vec![], + model_resolution: Some(ModelResolutionOptions::new(test_catalog())), }) .unwrap(); @@ -349,6 +346,38 @@ mod tests { ); } + #[test] + fn structural_transform_preserves_catalog_owned_model_selection() { + let dot = r#"digraph Test { + graph [goal="Test"] + start [shape=Mdiamond] + work [prompt="Do work", model="private-model", provider="server-only"] + exit [shape=Msquare] + start -> work -> exit + }"#; + let parsed = parse(dot).unwrap(); + let transformed = transform(parsed, &TransformOptions { + current_dir: None, + file_resolver: None, + template_context: fabro_template::TemplateContext::new(), + source_name: None, + render_mode: crate::operations::RenderMode::Strict, + custom_transforms: vec![], + model_resolution: None, + }) + .unwrap(); + let work = &transformed.graph.nodes["work"]; + + assert_eq!( + work.attrs.get("model").and_then(AttrValue::as_str), + Some("private-model") + ); + assert_eq!( + work.attrs.get("provider").and_then(AttrValue::as_str), + Some("server-only") + ); + } + #[test] fn transform_reports_goal_self_reference_once_across_passes() { // FileInlining renders the goal for prompt context, but TemplateTransform @@ -365,16 +394,13 @@ mod tests { ) .unwrap(); let transformed = transform(parsed, &TransformOptions { - current_dir: Some(dir.path().to_path_buf()), - file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), - template_context: fabro_template::TemplateContext::new(), - source_name: None, - render_mode: crate::operations::RenderMode::Structural, - custom_transforms: vec![], - catalog: test_catalog(), - default_provider: None, - eligible_providers: Catalog::builtin().all_provider_ids(), - catalog_fallback: false, + current_dir: Some(dir.path().to_path_buf()), + file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), + template_context: fabro_template::TemplateContext::new(), + source_name: None, + render_mode: crate::operations::RenderMode::Structural, + custom_transforms: vec![], + model_resolution: Some(ModelResolutionOptions::new(test_catalog())), }) .unwrap(); diff --git a/lib/components/fabro-workflow/src/pipeline/types.rs b/lib/components/fabro-workflow/src/pipeline/types.rs index 425cba9aa..29f5fbf9e 100644 --- a/lib/components/fabro-workflow/src/pipeline/types.rs +++ b/lib/components/fabro-workflow/src/pipeline/types.rs @@ -359,13 +359,20 @@ pub struct Finalized { /// Options for the TRANSFORM phase. pub struct TransformOptions { - pub current_dir: Option, - pub file_resolver: Option>, - pub template_context: TemplateContext, - pub source_name: Option, - pub render_mode: RenderMode, - pub custom_transforms: Vec>, - pub catalog: Arc, + pub current_dir: Option, + pub file_resolver: Option>, + pub template_context: TemplateContext, + pub source_name: Option, + pub render_mode: RenderMode, + pub custom_transforms: Vec>, + /// Catalog-backed model resolution to perform. `None` preserves authored + /// model and provider selectors for catalog-free structural validation. + pub model_resolution: Option, +} + +/// Catalog-backed model resolution options for the TRANSFORM phase. +pub struct ModelResolutionOptions { + pub catalog: Arc, pub default_provider: Option, pub eligible_providers: HashSet, /// Fall back to the full catalog when the eligible providers cannot @@ -373,6 +380,19 @@ pub struct TransformOptions { pub catalog_fallback: bool, } +impl ModelResolutionOptions { + #[must_use] + pub fn new(catalog: Arc) -> Self { + let eligible_providers = catalog.all_provider_ids(); + Self { + catalog, + default_provider: None, + eligible_providers, + catalog_fallback: false, + } + } +} + /// Options for the FINALIZE phase. pub struct FinalizeOptions { pub run_dir: PathBuf, diff --git a/lib/components/fabro-workflow/src/pipeline/validate.rs b/lib/components/fabro-workflow/src/pipeline/validate.rs index f0cd51dbd..00601b4a4 100644 --- a/lib/components/fabro-workflow/src/pipeline/validate.rs +++ b/lib/components/fabro-workflow/src/pipeline/validate.rs @@ -1,15 +1,28 @@ -use fabro_model::Catalog; use fabro_validate::LintRule; use super::types::{Transformed, Validated}; -/// VALIDATE phase: run lint rules against the transformed graph. +/// VALIDATE phase: run catalog-free lint rules against the transformed graph. /// /// **Infallible.** Always returns `Validated` with diagnostics. Caller decides /// whether to fail via `validated.raise_on_errors()`. -pub fn validate( +pub fn validate(transformed: Transformed, extra_rules: &[&dyn LintRule]) -> Validated { + let Transformed { + graph, + source, + mut diagnostics, + } = transformed; + diagnostics.extend(fabro_validate::validate(&graph, extra_rules)); + Validated::new(graph, source, diagnostics) +} + +/// VALIDATE phase: run catalog-free and catalog-backed lint rules. +/// +/// **Infallible.** Always returns `Validated` with diagnostics. Caller decides +/// whether to fail via `validated.raise_on_errors()`. +pub fn validate_with_catalog( transformed: Transformed, - catalog: &Catalog, + catalog: &fabro_model::Catalog, extra_rules: &[&dyn LintRule], ) -> Validated { let Transformed { @@ -27,34 +40,24 @@ pub fn validate( #[cfg(test)] mod tests { - use fabro_model::Catalog; - use super::*; use crate::pipeline::parse::parse; use crate::pipeline::transform; use crate::pipeline::types::TransformOptions; - fn test_catalog() -> std::sync::Arc { - std::sync::Arc::new(Catalog::from_builtin().unwrap()) - } - fn run_pipeline(dot: &str) -> Validated { - let catalog = test_catalog(); let parsed = parse(dot).unwrap(); let transformed = transform::transform(parsed, &TransformOptions { - current_dir: None, - file_resolver: None, - template_context: fabro_template::TemplateContext::new(), - source_name: None, - render_mode: crate::operations::RenderMode::Strict, - custom_transforms: vec![], - catalog: std::sync::Arc::clone(&catalog), - default_provider: None, - eligible_providers: catalog.all_provider_ids(), - catalog_fallback: false, + current_dir: None, + file_resolver: None, + template_context: fabro_template::TemplateContext::new(), + source_name: None, + render_mode: crate::operations::RenderMode::Strict, + custom_transforms: vec![], + model_resolution: None, }) .unwrap(); - validate(transformed, catalog.as_ref(), &[]) + validate(transformed, &[]) } #[test] diff --git a/lib/components/fabro-workflow/tests/it/integration.rs b/lib/components/fabro-workflow/tests/it/integration.rs index dc13bcbf8..b07f600fe 100644 --- a/lib/components/fabro-workflow/tests/it/integration.rs +++ b/lib/components/fabro-workflow/tests/it/integration.rs @@ -4853,7 +4853,9 @@ async fn manager_loop_child_workflow_e2e() { #[tokio::test] async fn import_e2e_through_engine() { - use fabro_workflow::pipeline::{TransformOptions, transform, validate}; + use fabro_workflow::pipeline::{ + ModelResolutionOptions, TransformOptions, transform, validate_with_catalog, + }; let dir = tempfile::tempdir().unwrap(); let catalog = std::sync::Arc::new( @@ -4897,21 +4899,18 @@ async fn import_e2e_through_engine() { ) .expect("parse should succeed"); let transformed = transform(parsed, &TransformOptions { - current_dir: Some(dir.path().to_path_buf()), - file_resolver: Some(std::sync::Arc::new( + current_dir: Some(dir.path().to_path_buf()), + file_resolver: Some(std::sync::Arc::new( fabro_workflow::file_resolver::FilesystemFileResolver::new(None), )), - template_context: fabro_template::TemplateContext::new(), - source_name: None, - render_mode: fabro_workflow::operations::RenderMode::Strict, - custom_transforms: vec![], - catalog: std::sync::Arc::clone(&catalog), - default_provider: None, - eligible_providers: catalog.all_provider_ids(), - catalog_fallback: false, + template_context: fabro_template::TemplateContext::new(), + source_name: None, + render_mode: fabro_workflow::operations::RenderMode::Strict, + custom_transforms: vec![], + model_resolution: Some(ModelResolutionOptions::new(std::sync::Arc::clone(&catalog))), }) .unwrap(); - let validated = validate(transformed, catalog.as_ref(), &[]); + let validated = validate_with_catalog(transformed, catalog.as_ref(), &[]); validated .raise_on_errors() .expect("validation should pass after imports expand");