scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-08-28 05:27:41 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

refactor: reuse ModelResolutionTransform instead of a parallel options type

Browse source 
`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<ModelResolutionTransform>` 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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-07-28 14:24:26 -04:00
parent 8592a34968
commit d6ac910e73
No known key found for this signature in database
8 changed files with 101 additions and 151 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

57
lib/components/fabro-workflow/src/handler/manager_loop.rs
View file

@ -65,22 +65,14 @@ fn parse_child_graph(node: &Node, services: &EngineServices) -> Result<ParsedChi
.get("stack.child_dot_source")
.and_then(|v| v.as_str())
{
let mut validated = validate_with_catalog(
ValidateInput {
workflow: WorkflowInput::DotSource {
source: dot.to_string(),
base_dir: None,
},
settings: WorkflowSettings::default(),
vars: std::collections::HashMap::new(),
cwd: cwd.clone(),
custom_transforms: Vec::new(),
let graph = validate_child_workflow(
WorkflowInput::DotSource {
source: dot.to_string(),
base_dir: None,
},
&services.run.catalog,
cwd,
services,
)?;
validated.promote_template_undefined_variables_to_errors();
validated.raise_on_errors()?;
let (graph, _, _) = validated.into_parts();
return Ok(ParsedChildWorkflow {
graph,
workflow_path: None,
@ -115,19 +107,7 @@ fn parse_child_graph(node: &Node, services: &EngineServices) -> Result<ParsedChi
WorkflowInput::Bundled(workflow) => 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<ParsedChi
Err(Error::handler("No child workflow source".to_string()))
}
/// Validate a child workflow against the run's catalog, failing on any error
/// diagnostic (undefined template variables included).
fn validate_child_workflow(
workflow: WorkflowInput,
cwd: PathBuf,
services: &EngineServices,
) -> Result<Graph, Error> {
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(

26
lib/components/fabro-workflow/src/operations/create.rs
View file

@ -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())),
}
}

55
lib/components/fabro-workflow/src/operations/validate.rs
View file

@ -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<Box<dyn Transform>>,
}
/// 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<Catalog>,
eligible_providers: HashSet<ProviderId>,
/// 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<Validated, Error> {
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<Catalog>,
) -> Result<Validated, Error> {
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<Catalog>,
ready_providers: &[ProviderId],
) -> Result<Validated, Error> {
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<CatalogScope<'_>>,
model_resolution: Option<ModelResolutionTransform>,
) -> Result<Validated, Error> {
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(

6
lib/components/fabro-workflow/src/pipeline/mod.rs
View file

@ -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;

65
lib/components/fabro-workflow/src/pipeline/transform.rs
View file

@ -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<Transform
.apply_with_diagnostics(graph)?;
diagnostics.extend(transform_diagnostics);
let graph = StylesheetApplicationTransform.apply(graph)?;
let graph = if let Some(model_resolution) = &options.model_resolution {
ModelResolutionTransform::for_eligible(
Arc::clone(&model_resolution.catalog),
model_resolution.eligible_providers.clone(),
)
.with_default_provider(model_resolution.default_provider.clone())
.with_catalog_fallback(model_resolution.catalog_fallback)
.apply(graph)?
} else {
graph
let graph = match &options.model_resolution {
Some(model_resolution) => 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();

29
lib/components/fabro-workflow/src/pipeline/types.rs
View file

@ -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<Box<dyn Transform>>,
/// Catalog-backed model resolution to perform. `None` preserves authored
/// model and provider selectors for catalog-free structural validation.
pub model_resolution: Option<ModelResolutionOptions>,
}
/// Catalog-backed model resolution options for the TRANSFORM phase.
pub struct ModelResolutionOptions {
pub catalog: Arc<Catalog>,
pub default_provider: Option<ProviderId>,
pub eligible_providers: HashSet<ProviderId>,
/// 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<Catalog>) -> Self {
let eligible_providers = catalog.all_provider_ids();
Self {
catalog,
default_provider: None,
eligible_providers,
catalog_fallback: false,
}
}
pub model_resolution: Option<ModelResolutionTransform>,
}
/// Options for the FINALIZE phase.

7
lib/components/fabro-workflow/src/transforms/model_resolution.rs
View file

@ -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,

7
lib/components/fabro-workflow/tests/it/integration.rs
View file

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