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..488f6a1f5 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/create.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/create.rs @@ -101,6 +101,40 @@ 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 output = context + .create_cmd() + .args([ + "--server", + &format!("{}/api/v1", server.base_url()), + "--dry-run", + fixture("server-model.fabro").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 bf38fe31a..0741c85f1 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/validate.rs @@ -69,6 +69,24 @@ fn simple_does_not_connect_to_configured_server() { ); } +/// Offline validation has no model catalog, so a model or provider the server +/// owns must pass rather than be reported as unknown. +#[test] +fn server_owned_provider_is_not_rejected_by_offline_validation() { + let context = test_context!(); + let mut cmd = context.validate(); + cmd.arg(fixture("server-model.fabro")); + fabro_snapshot!(context.filters(), cmd, @" + success: true + exit_code: 0 + ----- stdout ----- + ----- stderr ----- + Workflow: ServerModel (3 nodes, 2 edges) + Graph: [FIXTURES]/server-model.fabro + Validation: OK + "); +} + #[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..3d872954f 100644 --- a/lib/apps/fabro-server/src/manifest_validation.rs +++ b/lib/apps/fabro-server/src/manifest_validation.rs @@ -3,42 +3,48 @@ use std::sync::Arc; use anyhow::Result; use fabro_api::types; -use fabro_config::{EnvironmentLayer, MergeMap, RunLayer}; +use fabro_config::RunLayer; use fabro_model::Catalog; use fabro_workflow::pipeline::TEMPLATE_UNDEFINED_VARIABLE_RULE; use crate::run_manifest; +/// Validate a manifest without a model catalog. Model and provider +/// availability is deferred to the server, which owns the catalog. 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, - ) + let prepared = prepare(manifest_run_defaults, manifest)?; + let validated = run_manifest::validate_prepared_manifest_structural(&prepared) + .map_err(anyhow::Error::new)?; + Ok(run_manifest::validate_response(&prepared, &validated)) } -pub fn validate_manifest_with_environment_defaults( +/// Validate a manifest including the catalog-backed model and provider rules. +pub fn validate_manifest_with_catalog( manifest_run_defaults: &RunLayer, - manifest_environment_defaults: &MergeMap, manifest: &types::RunManifest, - catalog: Arc, + catalog: &Arc, ) -> Result { - let prepared = run_manifest::prepare_manifest_with_environment_defaults( - manifest_run_defaults, - manifest_environment_defaults, - &HashMap::new(), - manifest, - )?; + let prepared = prepare(manifest_run_defaults, manifest)?; let validated = run_manifest::validate_prepared_manifest(&prepared, catalog).map_err(anyhow::Error::new)?; Ok(run_manifest::validate_response(&prepared, &validated)) } +fn prepare( + manifest_run_defaults: &RunLayer, + manifest: &types::RunManifest, +) -> Result { + run_manifest::prepare_manifest_with_environment_defaults( + manifest_run_defaults, + &fabro_environment::seeded_catalog_layer(), + &HashMap::new(), + manifest, + ) +} + pub fn promote_template_undefined_variables_to_errors(response: &mut types::ValidateResponse) { let mut promoted = false; for diagnostic in &mut response.workflow.diagnostics { 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..73c068a73 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,20 +65,14 @@ 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), - })?; - validated.promote_template_undefined_variables_to_errors(); - validated.raise_on_errors()?; - let (graph, _, _) = validated.into_parts(); + let graph = validate_child_workflow(workflow, cwd, services)?; return Ok(ParsedChildWorkflow { graph, workflow_path, @@ -132,6 +116,29 @@ fn parse_child_graph(node: &Node, services: &EngineServices) -> Result Result { + let mut validated = validate_with_catalog( + ValidateInput { + workflow, + settings: WorkflowSettings::default(), + vars: 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(); + Ok(graph) +} + #[async_trait] impl Handler for SubWorkflowHandler { async fn execute( diff --git a/lib/components/fabro-workflow/src/operations/create.rs b/lib/components/fabro-workflow/src/operations/create.rs index 2ec40d443..5d5478b24 100644 --- a/lib/components/fabro-workflow/src/operations/create.rs +++ b/lib/components/fabro-workflow/src/operations/create.rs @@ -28,7 +28,7 @@ use crate::pipeline::{self, Persisted, TransformOptions, Validated}; use crate::records::RunSpec; use crate::run_lookup::default_scratch_base; use crate::run_materialization::materialize_run; -use crate::transforms::{RenderMode, Transform}; +use crate::transforms::{ModelResolutionTransform, RenderMode}; use crate::workflow_bundle::{RunDefinition, WorkflowBundle}; #[derive(Clone, Debug)] @@ -293,28 +293,21 @@ fn create_from_source( file_resolver: Option>, goal_override: Option<&str>, ) -> Result { - let template_context = template_context(Some(&options.settings), vars); - let mut validated = preprocess_and_validate( - dot_source, - options.source_name.clone(), + let mut validated = preprocess_and_validate(dot_source, goal_override, &TransformOptions { current_dir, file_resolver, - Vec::new(), - template_context, - goal_override, - RenderMode::Structural, - options - .settings - .run - .model - .provider - .as_deref() - .filter(|provider| !provider.is_empty()) - .map(ProviderId::new), - &options.configured_providers, - false, - &options.catalog, - )?; + template_context: template_context(Some(&options.settings), vars), + source_name: options.source_name.clone(), + render_mode: RenderMode::Structural, + custom_transforms: Vec::new(), + model_resolution: Some( + ModelResolutionTransform::for_eligible( + Arc::clone(&options.catalog), + options.configured_providers.iter().cloned().collect(), + ) + .with_default_provider(configured_default_provider(&options.settings)), + ), + })?; validated.promote_template_undefined_variables_to_errors(); if validated.has_errors() { @@ -326,36 +319,37 @@ fn create_from_source( persist_validated(validated, options) } +/// Parse, transform, and validate `dot_source`. +/// +/// `options.model_resolution` drives both halves of catalog awareness: it +/// selects concrete models during TRANSFORM and enables the catalog-backed +/// lint rules during VALIDATE. `None` leaves authored model and provider +/// selectors untouched for offline structural validation. pub(super) fn preprocess_and_validate( 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, - default_provider: Option, - eligible_providers: &[ProviderId], - catalog_fallback: bool, - catalog: &Arc, + options: &TransformOptions, ) -> Result { let mut parsed = pipeline::parse(dot_source)?; apply_goal_override(&mut parsed.graph, goal_override); - let transformed = pipeline::transform(parsed, &TransformOptions { - current_dir, - file_resolver, - template_context, - source_name, - render_mode, - custom_transforms, - catalog: Arc::clone(catalog), - default_provider, - eligible_providers: eligible_providers.iter().cloned().collect(), - catalog_fallback, - })?; - Ok(pipeline::validate(transformed, catalog.as_ref(), &[])) + let transformed = pipeline::transform(parsed, options)?; + let catalog = options + .model_resolution + .as_ref() + .map(ModelResolutionTransform::catalog); + Ok(pipeline::validate(transformed, catalog, &[])) +} + +/// The workflow-level default provider, treating an empty setting as unset. +pub(super) fn configured_default_provider(settings: &WorkflowSettings) -> Option { + settings + .run + .model + .provider + .as_deref() + .filter(|provider| !provider.is_empty()) + .map(ProviderId::new) } pub(super) fn template_context( @@ -462,8 +456,9 @@ 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::transforms::Transform; use crate::workflow_bundle::BundledWorkflow; fn memory_store() -> Arc { Arc::new(Database::new( @@ -553,17 +548,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() } @@ -573,21 +570,35 @@ reasoning = false fn validate_dot_with_vars(dot_source: &str, vars: HashMap) -> Validated { preprocess_and_validate( dot_source, - Some("workflow.fabro".to_string()), - Some(PathBuf::from(".")), None, - Vec::new(), - template_context(Some(&WorkflowSettings::default()), vars), - None, - RenderMode::Structural, - None, - &test_provider_ids(), - false, - &test_catalog(), + &test_transform_options( + PathBuf::from("."), + None, + RenderMode::Structural, + template_context(Some(&WorkflowSettings::default()), vars), + ), ) .unwrap() } + /// Catalog-backed TRANSFORM options for the built-in test catalog. + fn test_transform_options( + current_dir: PathBuf, + file_resolver: Option>, + render_mode: RenderMode, + template_context: TemplateContext, + ) -> TransformOptions { + TransformOptions { + current_dir: Some(current_dir), + file_resolver, + template_context, + source_name: Some("workflow.fabro".to_string()), + render_mode, + custom_transforms: Vec::new(), + model_resolution: Some(ModelResolutionTransform::new(test_catalog())), + } + } + const MINIMAL_DOT: &str = r#"digraph Test { graph [goal="Build feature"] start [shape=Mdiamond] @@ -744,17 +755,13 @@ reasoning = false let result = preprocess_and_validate( dot, - Some("workflow.fabro".to_string()), - Some(PathBuf::from(".")), None, - Vec::new(), - template_context(Some(&WorkflowSettings::default()), HashMap::new()), - None, - RenderMode::Strict, - None, - &test_provider_ids(), - false, - &test_catalog(), + &test_transform_options( + PathBuf::from("."), + None, + RenderMode::Strict, + template_context(Some(&WorkflowSettings::default()), HashMap::new()), + ), ); let Err(err) = result else { panic!("expected strict mode to hard-fail on unbound inline prompt"); @@ -781,19 +788,15 @@ reasoning = false let result = preprocess_and_validate( dot, - Some("workflow.fabro".to_string()), - Some(dir.path().to_path_buf()), - Some(Arc::new(crate::file_resolver::FilesystemFileResolver::new( - None, - ))), - Vec::new(), - template_context(Some(&WorkflowSettings::default()), HashMap::new()), None, - RenderMode::Strict, - None, - &test_provider_ids(), - false, - &test_catalog(), + &test_transform_options( + dir.path().to_path_buf(), + Some(Arc::new(crate::file_resolver::FilesystemFileResolver::new( + None, + ))), + RenderMode::Strict, + template_context(Some(&WorkflowSettings::default()), HashMap::new()), + ), ); let Err(err) = result else { panic!("expected strict mode to hard-fail on unbound imported prompt"); @@ -852,7 +855,6 @@ reasoning = false vars: HashMap::new(), cwd: PathBuf::from("."), custom_transforms: Vec::new(), - catalog: test_catalog(), }); assert!(result.is_err()); @@ -901,7 +903,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 +921,6 @@ reasoning = false vars: HashMap::new(), cwd: dir.path().to_path_buf(), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); assert_eq!( @@ -944,7 +944,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 +962,6 @@ reasoning = false vars: HashMap::new(), cwd: dir.path().to_path_buf(), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); assert_eq!( @@ -1055,7 +1053,6 @@ reasoning = false vars: HashMap::new(), cwd: PathBuf::from("."), custom_transforms: Vec::new(), - catalog: test_catalog(), }); assert!(result.is_err()); } @@ -1100,7 +1097,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 +1131,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 +1172,6 @@ reasoning = false vars: HashMap::new(), cwd: dir.path().to_path_buf(), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); @@ -1227,7 +1221,6 @@ reasoning = false vars: HashMap::new(), cwd: PathBuf::from("."), custom_transforms: Vec::new(), - catalog: test_catalog(), }) .unwrap(); @@ -1278,7 +1271,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..41413eb9e 100644 --- a/lib/components/fabro-workflow/src/operations/validate.rs +++ b/lib/components/fabro-workflow/src/operations/validate.rs @@ -5,12 +5,12 @@ use std::sync::Arc; use fabro_model::{Catalog, ProviderId}; use fabro_types::WorkflowSettings; -use super::create::{preprocess_and_validate, template_context}; +use super::create::{configured_default_provider, preprocess_and_validate, template_context}; use super::source::{ResolveWorkflowInput, WorkflowInput, resolve_workflow}; use crate::error::Error; use crate::operations::RenderMode; -use crate::pipeline::Validated; -use crate::transforms::Transform; +use crate::pipeline::{TransformOptions, Validated}; +use crate::transforms::{ModelResolutionTransform, Transform}; pub struct ValidateInput { pub workflow: WorkflowInput, @@ -20,20 +20,27 @@ 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. Model and provider availability is left to the caller that +/// owns a catalog — typically the server. /// /// 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) + validate_resolving_models(input, None) +} + +/// Parse, transform, and validate a DOT source string against `catalog`. +pub fn validate_with_catalog( + input: ValidateInput, + catalog: &Arc, +) -> Result { + validate_resolving_models( + input, + Some(ModelResolutionTransform::new(Arc::clone(catalog))), + ) } /// Parse, transform, and validate, resolving models against the ready @@ -41,45 +48,60 @@ 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_resolving_models( + input, + Some( + ModelResolutionTransform::for_eligible( + Arc::clone(catalog), + ready_providers.iter().cloned().collect(), + ) + .with_catalog_fallback(true), + ), + ) } -fn validate_with_eligible_providers( +/// The workflow's own default provider is only known once the workflow is +/// resolved, so callers hand in a partially built transform and it is +/// completed here. +fn validate_resolving_models( input: ValidateInput, - eligible_providers: &[ProviderId], - catalog_fallback: bool, + model_resolution: Option, ) -> 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()))?; + let model_resolution = model_resolution.map(|resolution| { + resolution.with_default_provider(configured_default_provider(&resolved.settings)) + }); + preprocess_and_validate( &resolved.raw_source, - resolved - .dot_path - .as_ref() - .map(|path| path.display().to_string()), - resolved.current_dir, - resolved.file_resolver, - input.custom_transforms, - template_context(Some(&resolved.settings), input.vars), resolved.goal_override.as_deref(), - RenderMode::Structural, - resolved - .settings - .run - .model - .provider - .as_deref() - .filter(|provider| !provider.is_empty()) - .map(fabro_model::ProviderId::new), - eligible_providers, - catalog_fallback, - &input.catalog, + &TransformOptions { + current_dir: resolved.current_dir, + file_resolver: resolved.file_resolver, + template_context: template_context(Some(&resolved.settings), vars), + source_name: resolved + .dot_path + .as_ref() + .map(|path| path.display().to_string()), + render_mode: RenderMode::Structural, + custom_transforms, + model_resolution, + }, ) } diff --git a/lib/components/fabro-workflow/src/pipeline/transform.rs b/lib/components/fabro-workflow/src/pipeline/transform.rs index b399d6637..bf3b82471 100644 --- a/lib/components/fabro-workflow/src/pipeline/transform.rs +++ b/lib/components/fabro-workflow/src/pipeline/transform.rs @@ -3,8 +3,8 @@ use std::sync::Arc; use super::types::{Parsed, TransformOptions, Transformed}; use crate::error::Error; use crate::transforms::{ - FileInliningTransform, ImportTransform, ModelResolutionTransform, - StylesheetApplicationTransform, TemplateTransform, Transform, + FileInliningTransform, ImportTransform, StylesheetApplicationTransform, TemplateTransform, + Transform, }; /// TRANSFORM phase: apply built-in and custom transforms to a parsed graph. @@ -63,13 +63,10 @@ pub fn transform(parsed: Parsed, options: &TransformOptions) -> Result model_resolution.apply(graph)?, + None => graph, + }; // Custom transforms let graph = options @@ -98,6 +95,7 @@ mod tests { use crate::file_resolver::FilesystemFileResolver; use crate::pipeline::parse::parse; use crate::pipeline::types::{GOAL_SELF_REFERENCE_RULE, TEMPLATE_UNDEFINED_VARIABLE_RULE}; + use crate::transforms::ModelResolutionTransform; fn write_file(path: &Path, contents: &str) { if let Some(parent) = path.parent() { @@ -112,16 +110,13 @@ mod tests { fn transform_options() -> 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(ModelResolutionTransform::new(test_catalog())), } } @@ -177,16 +172,9 @@ 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))), + ..transform_options() }) .unwrap(); @@ -227,21 +215,15 @@ 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, + ), + ])), + ..transform_options() }) .unwrap(); @@ -349,6 +331,33 @@ 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 { + model_resolution: None, + ..transform_options() + }) + .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 +374,10 @@ 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))), + render_mode: crate::operations::RenderMode::Structural, + ..transform_options() }) .unwrap(); diff --git a/lib/components/fabro-workflow/src/pipeline/types.rs b/lib/components/fabro-workflow/src/pipeline/types.rs index 425cba9aa..9ff9b83fd 100644 --- a/lib/components/fabro-workflow/src/pipeline/types.rs +++ b/lib/components/fabro-workflow/src/pipeline/types.rs @@ -1,4 +1,4 @@ -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -28,7 +28,7 @@ use crate::runtime_store::RunStoreHandle; use crate::services::{EngineServices, FabroRunToolServices, RunServices}; use crate::stage_execution::StageExecutionSeed; use crate::steering_hub::SteeringHub; -use crate::transforms::{RenderMode, Transform}; +use crate::transforms::{ModelResolutionTransform, RenderMode, Transform}; use crate::workflow_bundle::WorkflowBundle; /// Output of the PARSE phase. @@ -359,18 +359,15 @@ 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 default_provider: Option, - pub eligible_providers: HashSet, - /// Fall back to the full catalog when the eligible providers cannot - /// supply a requested model, instead of erroring. - pub catalog_fallback: bool, + 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, } /// Options for the FINALIZE phase. diff --git a/lib/components/fabro-workflow/src/pipeline/validate.rs b/lib/components/fabro-workflow/src/pipeline/validate.rs index f0cd51dbd..32bfaf69c 100644 --- a/lib/components/fabro-workflow/src/pipeline/validate.rs +++ b/lib/components/fabro-workflow/src/pipeline/validate.rs @@ -5,11 +5,15 @@ use super::types::{Transformed, Validated}; /// VALIDATE phase: run lint rules against the transformed graph. /// +/// Catalog-backed rules (model and provider availability) run only when +/// `catalog` is `Some`. Offline callers pass `None` so a workflow naming a +/// server-owned model is left for the server to judge. +/// /// **Infallible.** Always returns `Validated` with diagnostics. Caller decides /// whether to fail via `validated.raise_on_errors()`. pub fn validate( transformed: Transformed, - catalog: &Catalog, + catalog: Option<&Catalog>, extra_rules: &[&dyn LintRule], ) -> Validated { let Transformed { @@ -17,44 +21,33 @@ pub fn validate( source, mut diagnostics, } = transformed; - diagnostics.extend(fabro_validate::validate_with_catalog( - &graph, - catalog, - extra_rules, - )); + diagnostics.extend(match catalog { + Some(catalog) => fabro_validate::validate_with_catalog(&graph, catalog, extra_rules), + None => fabro_validate::validate(&graph, extra_rules), + }); Validated::new(graph, source, diagnostics) } #[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, None, &[]) } #[test] diff --git a/lib/components/fabro-workflow/src/transforms/model_resolution.rs b/lib/components/fabro-workflow/src/transforms/model_resolution.rs index 12f29ac5a..7fb61b9d9 100644 --- a/lib/components/fabro-workflow/src/transforms/model_resolution.rs +++ b/lib/components/fabro-workflow/src/transforms/model_resolution.rs @@ -52,6 +52,13 @@ impl ModelResolutionTransform { self } + /// The catalog this transform resolves against, so callers can run the + /// matching catalog-backed lint rules. + #[must_use] + pub fn catalog(&self) -> &Catalog { + &self.catalog + } + fn resolve_model( &self, model: &str, diff --git a/lib/components/fabro-workflow/tests/it/integration.rs b/lib/components/fabro-workflow/tests/it/integration.rs index ec716adbc..e4f64f17c 100644 --- a/lib/components/fabro-workflow/tests/it/integration.rs +++ b/lib/components/fabro-workflow/tests/it/integration.rs @@ -4857,6 +4857,7 @@ 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::transforms::ModelResolutionTransform; let dir = tempfile::tempdir().unwrap(); let catalog = std::sync::Arc::new( @@ -4900,21 +4901,20 @@ 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(ModelResolutionTransform::new(std::sync::Arc::clone( + &catalog, + ))), }) .unwrap(); - let validated = validate(transformed, catalog.as_ref(), &[]); + let validated = validate(transformed, Some(catalog.as_ref()), &[]); validated .raise_on_errors() .expect("validation should pass after imports expand"); diff --git a/test/server-model.fabro b/test/server-model.fabro new file mode 100644 index 000000000..099f3c8d9 --- /dev/null +++ b/test/server-model.fabro @@ -0,0 +1,10 @@ +digraph ServerModel { + graph [goal="Use a server-owned model"] + + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + + work [label="Work", prompt="Do work", model="private-model", provider="server-only"] + + start -> work -> exit +}