From 0b24649e7617d372d1ff1738ee258719fb767240 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 26 Jul 2026 09:25:47 -0400 Subject: [PATCH 1/3] fix(cli): keep offline validation catalog-free --- lib/apps/fabro-cli/src/commands/run/create.rs | 7 +- lib/apps/fabro-cli/src/commands/run/runner.rs | 7 +- lib/apps/fabro-cli/src/commands/validate.rs | 6 +- lib/apps/fabro-cli/tests/it/cmd/create.rs | 46 +++++++ lib/apps/fabro-cli/tests/it/cmd/validate.rs | 33 +++++ .../fabro-mcp-server/src/manifest_builder.rs | 2 +- .../fabro-server/src/manifest_validation.rs | 23 +++- lib/apps/fabro-server/src/run_manifest.rs | 40 +++--- .../fabro-server/src/run_tool_manifest.rs | 11 +- .../fabro-server/src/server/handler/graph.rs | 2 +- .../fabro-server/src/server/handler/runs.rs | 22 ++- .../src/handler/manager_loop.rs | 42 +++--- .../fabro-workflow/src/operations/create.rs | 109 +++++++++++---- .../fabro-workflow/src/operations/mod.rs | 2 +- .../fabro-workflow/src/operations/validate.rs | 73 +++++++--- .../fabro-workflow/src/pipeline/mod.rs | 8 +- .../fabro-workflow/src/pipeline/transform.rs | 128 +++++++++++------- .../fabro-workflow/src/pipeline/types.rs | 34 ++++- .../fabro-workflow/src/pipeline/validate.rs | 47 ++++--- .../fabro-workflow/tests/it/integration.rs | 23 ++-- 20 files changed, 449 insertions(+), 216 deletions(-) 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"); From 8592a34968b465fa2e408dfe22a1955574dee762 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 28 Jul 2026 14:13:45 -0400 Subject: [PATCH 2/3] refactor: collapse the split validation paths Follow-up cleanup on the catalog-free validation split. Same behavior, fewer parallel code paths. - Make the catalog an explicit `Option<&Catalog>` on `pipeline::validate` instead of a `validate` / `validate_with_catalog` pair, so each call site states whether catalog rules run. - Collapse `preprocess_and_validate`, `preprocess_and_validate_structural`, and `preprocess` into one function that takes `TransformOptions`. Its `model_resolution` field is now the single source of truth for catalog awareness, which drops a 12-argument signature and the `too_many_arguments` allow. - Replace the duplicated resolve-and-preprocess block in `operations::validate` with one `validate_in_scope` helper, and drop the HashSet -> Vec -> HashSet round trip on the catalog path. - Extract `configured_default_provider`, previously duplicated between `operations::create` and `operations::validate`. - Delete `validate_manifest_with_environment_defaults`, which had no callers outside its own module. - Share the `server-model.fabro` fixture between the two CLI tests instead of inlining it twice. The validate test now asserts the rendered output through the usual snapshot helper, which also removes a hand-rolled `std::fs::write` and its clippy allow. Co-Authored-By: Claude Opus 5 (1M context) --- lib/apps/fabro-cli/tests/it/cmd/create.rs | 14 +- lib/apps/fabro-cli/tests/it/cmd/validate.rs | 43 ++-- .../fabro-server/src/manifest_validation.rs | 35 ++- .../fabro-workflow/src/operations/create.rs | 221 +++++++----------- .../fabro-workflow/src/operations/validate.rs | 114 +++++---- .../fabro-workflow/src/pipeline/mod.rs | 2 +- .../fabro-workflow/src/pipeline/validate.rs | 36 ++- .../fabro-workflow/tests/it/integration.rs | 6 +- test/server-model.fabro | 10 + 9 files changed, 191 insertions(+), 290 deletions(-) create mode 100644 test/server-model.fabro diff --git a/lib/apps/fabro-cli/tests/it/cmd/create.rs b/lib/apps/fabro-cli/tests/it/cmd/create.rs index 462a0996f..488f6a1f5 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/create.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/create.rs @@ -114,25 +114,13 @@ fn create_defers_provider_validation_to_the_server() { .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(), + fixture("server-model.fabro").to_str().unwrap(), ]) .output() .expect("command should execute"); diff --git a/lib/apps/fabro-cli/tests/it/cmd/validate.rs b/lib/apps/fabro-cli/tests/it/cmd/validate.rs index 2bbec8c76..fcdaec83d 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/validate.rs @@ -69,37 +69,22 @@ 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] -#[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), - ); + 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] diff --git a/lib/apps/fabro-server/src/manifest_validation.rs b/lib/apps/fabro-server/src/manifest_validation.rs index 3016fde65..3d872954f 100644 --- a/lib/apps/fabro-server/src/manifest_validation.rs +++ b/lib/apps/fabro-server/src/manifest_validation.rs @@ -3,53 +3,46 @@ 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, ) -> Result { - validate_manifest_with_environment_defaults( - manifest_run_defaults, - &fabro_environment::seeded_catalog_layer(), - manifest, - ) + 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)) } +/// Validate a manifest including the catalog-backed model and provider rules. 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 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)) } -pub fn validate_manifest_with_environment_defaults( +fn prepare( manifest_run_defaults: &RunLayer, - manifest_environment_defaults: &MergeMap, manifest: &types::RunManifest, -) -> Result { - let prepared = run_manifest::prepare_manifest_with_environment_defaults( +) -> Result { + run_manifest::prepare_manifest_with_environment_defaults( manifest_run_defaults, - manifest_environment_defaults, + &fabro_environment::seeded_catalog_layer(), &HashMap::new(), manifest, - )?; - let validated = run_manifest::validate_prepared_manifest_structural(&prepared) - .map_err(anyhow::Error::new)?; - Ok(run_manifest::validate_response(&prepared, &validated)) + ) } pub fn promote_template_undefined_variables_to_errors(response: &mut types::ValidateResponse) { diff --git a/lib/components/fabro-workflow/src/operations/create.rs b/lib/components/fabro-workflow/src/operations/create.rs index 4fff69898..3e976e824 100644 --- a/lib/components/fabro-workflow/src/operations/create.rs +++ b/lib/components/fabro-workflow/src/operations/create.rs @@ -24,13 +24,11 @@ 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, ModelResolutionOptions, Persisted, TransformOptions, Transformed, Validated, -}; +use crate::pipeline::{self, ModelResolutionOptions, 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::RenderMode; use crate::workflow_bundle::{RunDefinition, WorkflowBundle}; #[derive(Clone, Debug)] @@ -295,28 +293,20 @@ 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(ModelResolutionOptions { + catalog: Arc::clone(&options.catalog), + default_provider: configured_default_provider(&options.settings), + eligible_providers: options.configured_providers.iter().cloned().collect(), + catalog_fallback: false, + }), + })?; validated.promote_template_undefined_variables_to_errors(); if validated.has_errors() { @@ -328,96 +318,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 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); - let transformed = pipeline::transform(parsed, &TransformOptions { - current_dir, - file_resolver, - template_context, - source_name, - render_mode, - custom_transforms, - model_resolution, - })?; - Ok(transformed) + let transformed = pipeline::transform(parsed, options)?; + let catalog = options + .model_resolution + .as_ref() + .map(|resolution| resolution.catalog.as_ref()); + 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( @@ -526,6 +457,7 @@ mod tests { use super::*; 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( @@ -637,21 +569,40 @@ 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(ModelResolutionOptions { + catalog: test_catalog(), + default_provider: None, + eligible_providers: test_provider_ids().into_iter().collect(), + catalog_fallback: false, + }), + } + } + const MINIMAL_DOT: &str = r#"digraph Test { graph [goal="Build feature"] start [shape=Mdiamond] @@ -808,17 +759,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"); @@ -845,19 +792,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"); diff --git a/lib/components/fabro-workflow/src/operations/validate.rs b/lib/components/fabro-workflow/src/operations/validate.rs index 7ac5e4531..4000443e1 100644 --- a/lib/components/fabro-workflow/src/operations/validate.rs +++ b/lib/components/fabro-workflow/src/operations/validate.rs @@ -1,17 +1,15 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::path::PathBuf; use std::sync::Arc; use fabro_model::{Catalog, ProviderId}; use fabro_types::WorkflowSettings; -use super::create::{ - preprocess_and_validate, preprocess_and_validate_structural, 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::pipeline::{ModelResolutionOptions, TransformOptions, Validated}; use crate::transforms::Transform; pub struct ValidateInput { @@ -24,39 +22,25 @@ pub struct ValidateInput { pub custom_transforms: Vec>, } +/// Which providers catalog-backed model resolution may select from. The +/// workflow's own default provider is read from the resolved settings, so it +/// is not part of the caller's request. +struct CatalogScope<'a> { + catalog: &'a Arc, + eligible_providers: HashSet, + /// Fall back to the full catalog when the eligible providers cannot + /// supply a requested model, instead of erroring. + catalog_fallback: bool, +} + /// Parse, transform, and structurally validate a DOT source string without a -/// model catalog. +/// 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 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, - ) + validate_in_scope(input, None) } /// Parse, transform, and validate a DOT source string against `catalog`. @@ -64,8 +48,14 @@ 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) + validate_in_scope( + input, + Some(CatalogScope { + catalog, + eligible_providers: catalog.all_provider_ids(), + catalog_fallback: false, + }), + ) } /// Parse, transform, and validate, resolving models against the ready @@ -76,14 +66,19 @@ pub fn validate_with_ready_providers( catalog: &Arc, ready_providers: &[ProviderId], ) -> Result { - validate_with_eligible_providers(input, catalog, ready_providers, true) + validate_in_scope( + input, + Some(CatalogScope { + catalog, + eligible_providers: ready_providers.iter().cloned().collect(), + catalog_fallback: true, + }), + ) } -fn validate_with_eligible_providers( +fn validate_in_scope( input: ValidateInput, - catalog: &Arc, - eligible_providers: &[ProviderId], - catalog_fallback: bool, + scope: Option>, ) -> Result { let ValidateInput { workflow, @@ -99,28 +94,27 @@ fn validate_with_eligible_providers( }) .map_err(|err| Error::Parse(err.to_string()))?; + let model_resolution = scope.map(|scope| ModelResolutionOptions { + catalog: Arc::clone(scope.catalog), + default_provider: configured_default_provider(&resolved.settings), + eligible_providers: scope.eligible_providers, + catalog_fallback: scope.catalog_fallback, + }); + preprocess_and_validate( &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, - resolved - .settings - .run - .model - .provider - .as_deref() - .filter(|provider| !provider.is_empty()) - .map(fabro_model::ProviderId::new), - eligible_providers, - catalog_fallback, - 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/mod.rs b/lib/components/fabro-workflow/src/pipeline/mod.rs index c1cc3b4f9..46c271907 100644 --- a/lib/components/fabro-workflow/src/pipeline/mod.rs +++ b/lib/components/fabro-workflow/src/pipeline/mod.rs @@ -26,4 +26,4 @@ pub use types::{ ModelResolutionOptions, Parsed, Persisted, PullRequestOptions, ResumeState, SandboxEnvSpec, TEMPLATE_UNDEFINED_VARIABLE_RULE, TransformOptions, Transformed, Validated, }; -pub use validate::{validate, validate_with_catalog}; +pub use validate::validate; diff --git a/lib/components/fabro-workflow/src/pipeline/validate.rs b/lib/components/fabro-workflow/src/pipeline/validate.rs index 00601b4a4..32bfaf69c 100644 --- a/lib/components/fabro-workflow/src/pipeline/validate.rs +++ b/lib/components/fabro-workflow/src/pipeline/validate.rs @@ -1,28 +1,19 @@ +use fabro_model::Catalog; use fabro_validate::LintRule; use super::types::{Transformed, Validated}; -/// VALIDATE phase: run catalog-free lint rules against the transformed graph. +/// 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, 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( +pub fn validate( transformed: Transformed, - catalog: &fabro_model::Catalog, + catalog: Option<&Catalog>, extra_rules: &[&dyn LintRule], ) -> Validated { let Transformed { @@ -30,11 +21,10 @@ pub fn validate_with_catalog( 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) } @@ -57,7 +47,7 @@ mod tests { model_resolution: None, }) .unwrap(); - validate(transformed, &[]) + validate(transformed, None, &[]) } #[test] diff --git a/lib/components/fabro-workflow/tests/it/integration.rs b/lib/components/fabro-workflow/tests/it/integration.rs index b07f600fe..51030ec05 100644 --- a/lib/components/fabro-workflow/tests/it/integration.rs +++ b/lib/components/fabro-workflow/tests/it/integration.rs @@ -4853,9 +4853,7 @@ async fn manager_loop_child_workflow_e2e() { #[tokio::test] async fn import_e2e_through_engine() { - use fabro_workflow::pipeline::{ - ModelResolutionOptions, TransformOptions, transform, validate_with_catalog, - }; + use fabro_workflow::pipeline::{ModelResolutionOptions, TransformOptions, transform, validate}; let dir = tempfile::tempdir().unwrap(); let catalog = std::sync::Arc::new( @@ -4910,7 +4908,7 @@ async fn import_e2e_through_engine() { model_resolution: Some(ModelResolutionOptions::new(std::sync::Arc::clone(&catalog))), }) .unwrap(); - let validated = validate_with_catalog(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 +} From d6ac910e73639256dbf15a7cd29bd91c2e1bbf65 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 28 Jul 2026 14:24:26 -0400 Subject: [PATCH 3/3] refactor: reuse ModelResolutionTransform instead of a parallel options type `ModelResolutionOptions` was a field-for-field duplicate of the existing public `ModelResolutionTransform`, down to a verbatim copy of its `new()`. `pipeline::transform` then unpacked one to rebuild the other, cloning the catalog Arc and the eligible-provider set on the way. - Delete `ModelResolutionOptions`. `TransformOptions.model_resolution` now holds an `Option` directly, so the TRANSFORM step is `resolution.apply(graph)?` with no rebuild and no clones. This is consistent with `custom_transforms`, which already holds transforms. - Add `ModelResolutionTransform::catalog()` so the VALIDATE step can reach the same catalog for its lint rules. That is the only new code needed. - Drop `CatalogScope` from `operations::validate`, which was a third copy of the same fields. The three entry points now hand a partially built transform to `validate_resolving_models`, which completes it with the workflow's default provider once the workflow is resolved. - Extract `validate_child_workflow` in `manager_loop`, collapsing two near-identical validate-and-unwrap blocks. - Point the transform tests at their own `transform_options()` helper via struct-update syntax instead of respelling all seven fields, and drop a HashSet -> Vec -> HashSet round trip from the create test helper. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/handler/manager_loop.rs | 57 ++++++++-------- .../fabro-workflow/src/operations/create.rs | 26 ++++---- .../fabro-workflow/src/operations/validate.rs | 55 ++++++---------- .../fabro-workflow/src/pipeline/mod.rs | 6 +- .../fabro-workflow/src/pipeline/transform.rs | 65 ++++++------------- .../fabro-workflow/src/pipeline/types.rs | 29 +-------- .../src/transforms/model_resolution.rs | 7 ++ .../fabro-workflow/tests/it/integration.rs | 7 +- 8 files changed, 101 insertions(+), 151 deletions(-) diff --git a/lib/components/fabro-workflow/src/handler/manager_loop.rs b/lib/components/fabro-workflow/src/handler/manager_loop.rs index ddefe84be..73c068a73 100644 --- a/lib/components/fabro-workflow/src/handler/manager_loop.rs +++ b/lib/components/fabro-workflow/src/handler/manager_loop.rs @@ -65,22 +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_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(); + let graph = validate_child_workflow(workflow, cwd, services)?; return Ok(ParsedChildWorkflow { graph, workflow_path, @@ -136,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 3e976e824..5d5478b24 100644 --- a/lib/components/fabro-workflow/src/operations/create.rs +++ b/lib/components/fabro-workflow/src/operations/create.rs @@ -24,11 +24,11 @@ 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, ModelResolutionOptions, Persisted, TransformOptions, Validated}; +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; +use crate::transforms::{ModelResolutionTransform, RenderMode}; use crate::workflow_bundle::{RunDefinition, WorkflowBundle}; #[derive(Clone, Debug)] @@ -300,12 +300,13 @@ fn create_from_source( source_name: options.source_name.clone(), render_mode: RenderMode::Structural, custom_transforms: Vec::new(), - model_resolution: Some(ModelResolutionOptions { - catalog: Arc::clone(&options.catalog), - default_provider: configured_default_provider(&options.settings), - eligible_providers: options.configured_providers.iter().cloned().collect(), - catalog_fallback: false, - }), + 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(); @@ -336,7 +337,7 @@ pub(super) fn preprocess_and_validate( let catalog = options .model_resolution .as_ref() - .map(|resolution| resolution.catalog.as_ref()); + .map(ModelResolutionTransform::catalog); Ok(pipeline::validate(transformed, catalog, &[])) } @@ -594,12 +595,7 @@ reasoning = false source_name: Some("workflow.fabro".to_string()), render_mode, custom_transforms: Vec::new(), - model_resolution: Some(ModelResolutionOptions { - catalog: test_catalog(), - default_provider: None, - eligible_providers: test_provider_ids().into_iter().collect(), - catalog_fallback: false, - }), + model_resolution: Some(ModelResolutionTransform::new(test_catalog())), } } diff --git a/lib/components/fabro-workflow/src/operations/validate.rs b/lib/components/fabro-workflow/src/operations/validate.rs index 4000443e1..41413eb9e 100644 --- a/lib/components/fabro-workflow/src/operations/validate.rs +++ b/lib/components/fabro-workflow/src/operations/validate.rs @@ -1,4 +1,4 @@ -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; @@ -9,8 +9,8 @@ use super::create::{configured_default_provider, preprocess_and_validate, templa use super::source::{ResolveWorkflowInput, WorkflowInput, resolve_workflow}; use crate::error::Error; use crate::operations::RenderMode; -use crate::pipeline::{ModelResolutionOptions, TransformOptions, Validated}; -use crate::transforms::Transform; +use crate::pipeline::{TransformOptions, Validated}; +use crate::transforms::{ModelResolutionTransform, Transform}; pub struct ValidateInput { pub workflow: WorkflowInput, @@ -22,17 +22,6 @@ pub struct ValidateInput { pub custom_transforms: Vec>, } -/// Which providers catalog-backed model resolution may select from. The -/// workflow's own default provider is read from the resolved settings, so it -/// is not part of the caller's request. -struct CatalogScope<'a> { - catalog: &'a Arc, - eligible_providers: HashSet, - /// Fall back to the full catalog when the eligible providers cannot - /// supply a requested model, instead of erroring. - catalog_fallback: bool, -} - /// 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. @@ -40,7 +29,7 @@ struct CatalogScope<'a> { /// 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 { - validate_in_scope(input, None) + validate_resolving_models(input, None) } /// Parse, transform, and validate a DOT source string against `catalog`. @@ -48,13 +37,9 @@ pub fn validate_with_catalog( input: ValidateInput, catalog: &Arc, ) -> Result { - validate_in_scope( + validate_resolving_models( input, - Some(CatalogScope { - catalog, - eligible_providers: catalog.all_provider_ids(), - catalog_fallback: false, - }), + Some(ModelResolutionTransform::new(Arc::clone(catalog))), ) } @@ -66,19 +51,24 @@ pub fn validate_with_ready_providers( catalog: &Arc, ready_providers: &[ProviderId], ) -> Result { - validate_in_scope( + validate_resolving_models( input, - Some(CatalogScope { - catalog, - eligible_providers: ready_providers.iter().cloned().collect(), - catalog_fallback: true, - }), + Some( + ModelResolutionTransform::for_eligible( + Arc::clone(catalog), + ready_providers.iter().cloned().collect(), + ) + .with_catalog_fallback(true), + ), ) } -fn validate_in_scope( +/// 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, - scope: Option>, + model_resolution: Option, ) -> Result { let ValidateInput { workflow, @@ -94,11 +84,8 @@ fn validate_in_scope( }) .map_err(|err| Error::Parse(err.to_string()))?; - let model_resolution = scope.map(|scope| ModelResolutionOptions { - catalog: Arc::clone(scope.catalog), - default_provider: configured_default_provider(&resolved.settings), - eligible_providers: scope.eligible_providers, - catalog_fallback: scope.catalog_fallback, + let model_resolution = model_resolution.map(|resolution| { + resolution.with_default_provider(configured_default_provider(&resolved.settings)) }); preprocess_and_validate( diff --git a/lib/components/fabro-workflow/src/pipeline/mod.rs b/lib/components/fabro-workflow/src/pipeline/mod.rs index 46c271907..d0ba5ae1b 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, - ModelResolutionOptions, Parsed, Persisted, PullRequestOptions, ResumeState, SandboxEnvSpec, - TEMPLATE_UNDEFINED_VARIABLE_RULE, TransformOptions, Transformed, Validated, + Concluded, Executed, FinalizeOptions, Finalized, InitOptions, Initialized, LlmSpec, Parsed, + Persisted, PullRequestOptions, ResumeState, SandboxEnvSpec, TEMPLATE_UNDEFINED_VARIABLE_RULE, + TransformOptions, Transformed, Validated, }; pub use validate::validate; diff --git a/lib/components/fabro-workflow/src/pipeline/transform.rs b/lib/components/fabro-workflow/src/pipeline/transform.rs index d73b26275..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,16 +63,9 @@ pub fn transform(parsed: Parsed, options: &TransformOptions) -> Result model_resolution.apply(graph)?, + None => graph, }; // Custom transforms @@ -101,9 +94,8 @@ mod tests { use super::*; use crate::file_resolver::FilesystemFileResolver; use crate::pipeline::parse::parse; - use crate::pipeline::types::{ - GOAL_SELF_REFERENCE_RULE, ModelResolutionOptions, TEMPLATE_UNDEFINED_VARIABLE_RULE, - }; + 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() { @@ -124,7 +116,7 @@ mod tests { source_name: None, render_mode: crate::operations::RenderMode::Strict, custom_transforms: vec![], - model_resolution: Some(ModelResolutionOptions::new(test_catalog())), + model_resolution: Some(ModelResolutionTransform::new(test_catalog())), } } @@ -180,13 +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![], - model_resolution: Some(ModelResolutionOptions::new(test_catalog())), + current_dir: Some(dir.path().to_path_buf()), + file_resolver: Some(Arc::new(FilesystemFileResolver::new(None))), + ..transform_options() }) .unwrap(); @@ -227,18 +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![], - model_resolution: Some(ModelResolutionOptions::new(test_catalog())), + ..transform_options() }) .unwrap(); @@ -357,13 +342,8 @@ mod tests { }"#; 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, + model_resolution: None, + ..transform_options() }) .unwrap(); let work = &transformed.graph.nodes["work"]; @@ -394,13 +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![], - model_resolution: Some(ModelResolutionOptions::new(test_catalog())), + 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 29f5fbf9e..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. @@ -367,30 +367,7 @@ pub struct TransformOptions { 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 - /// supply a requested model, instead of erroring. - 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, - } - } + pub model_resolution: Option, } /// Options for the FINALIZE phase. 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 51030ec05..781bbf463 100644 --- a/lib/components/fabro-workflow/tests/it/integration.rs +++ b/lib/components/fabro-workflow/tests/it/integration.rs @@ -4853,7 +4853,8 @@ async fn manager_loop_child_workflow_e2e() { #[tokio::test] async fn import_e2e_through_engine() { - use fabro_workflow::pipeline::{ModelResolutionOptions, TransformOptions, transform, validate}; + use fabro_workflow::pipeline::{TransformOptions, transform, validate}; + use fabro_workflow::transforms::ModelResolutionTransform; let dir = tempfile::tempdir().unwrap(); let catalog = std::sync::Arc::new( @@ -4905,7 +4906,9 @@ async fn import_e2e_through_engine() { source_name: None, render_mode: fabro_workflow::operations::RenderMode::Strict, custom_transforms: vec![], - model_resolution: Some(ModelResolutionOptions::new(std::sync::Arc::clone(&catalog))), + model_resolution: Some(ModelResolutionTransform::new(std::sync::Arc::clone( + &catalog, + ))), }) .unwrap(); let validated = validate(transformed, Some(catalog.as_ref()), &[]);