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()), &[]);