From a2ab2f85288e76a889f532def9effd947bb51d1c Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sat, 28 Mar 2026 19:02:22 -0400 Subject: [PATCH] Return graphs from workflow transforms --- .../fabro-workflows/src/operations/create.rs | 5 ++- .../fabro-workflows/src/pipeline/transform.rs | 33 +++++++++++-------- .../src/transforms/file_inlining.rs | 9 +++-- .../fabro-workflows/src/transforms/import.rs | 20 ++++++----- .../fabro-workflows/src/transforms/mod.rs | 3 +- .../src/transforms/model_resolution.rs | 17 ++++++---- .../src/transforms/preamble.rs | 13 +++++--- .../src/transforms/stylesheet_application.rs | 12 ++++--- .../src/transforms/variable_expansion.rs | 15 +++++---- .../fabro-workflows/tests/it/integration.rs | 22 ++++++------- 10 files changed, 87 insertions(+), 62 deletions(-) diff --git a/lib/crates/fabro-workflows/src/operations/create.rs b/lib/crates/fabro-workflows/src/operations/create.rs index 40a3536cb..dafb3ef55 100644 --- a/lib/crates/fabro-workflows/src/operations/create.rs +++ b/lib/crates/fabro-workflows/src/operations/create.rs @@ -450,11 +450,14 @@ mod tests { struct TagTransform; impl Transform for TagTransform { - fn apply(&self, graph: &mut fabro_graphviz::graph::Graph) { + fn apply(&self, graph: fabro_graphviz::graph::Graph) -> fabro_graphviz::graph::Graph { + let mut graph = graph; for node in graph.nodes.values_mut() { node.attrs .insert("tagged".to_string(), AttrValue::Boolean(true)); } + + graph } } diff --git a/lib/crates/fabro-workflows/src/pipeline/transform.rs b/lib/crates/fabro-workflows/src/pipeline/transform.rs index 44a85f540..a017e8cf6 100644 --- a/lib/crates/fabro-workflows/src/pipeline/transform.rs +++ b/lib/crates/fabro-workflows/src/pipeline/transform.rs @@ -7,30 +7,35 @@ use super::types::{Parsed, TransformOptions, Transformed}; /// TRANSFORM phase: apply built-in and custom transforms to a parsed graph. /// -/// Infallible. Returns `Transformed` with a mutable `graph` for post-transform +/// Infallible. Returns `Transformed` with a graph for post-transform /// adjustments (e.g. goal override) before validation. pub fn transform(parsed: Parsed, options: &TransformOptions) -> Transformed { - let Parsed { mut graph, source } = parsed; + let Parsed { graph, source } = parsed; // Built-in transforms (PreambleTransform moved to engine execution time) - if let Some(ref dir) = options.base_dir { + let graph = if let Some(ref dir) = options.base_dir { let fallback = dirs::home_dir().map(|home| home.join(".fabro")); - ImportTransform::new(dir.clone(), fallback).apply(&mut graph); - } + ImportTransform::new(dir.clone(), fallback).apply(graph) + } else { + graph + }; - if let Some(ref dir) = options.base_dir { + let graph = if let Some(ref dir) = options.base_dir { let fallback = dirs::home_dir().map(|home| home.join(".fabro")); - FileInliningTransform::new(dir.clone(), fallback).apply(&mut graph); - } + FileInliningTransform::new(dir.clone(), fallback).apply(graph) + } else { + graph + }; - VariableExpansionTransform.apply(&mut graph); - StylesheetApplicationTransform.apply(&mut graph); - ModelResolutionTransform.apply(&mut graph); + let graph = VariableExpansionTransform.apply(graph); + let graph = StylesheetApplicationTransform.apply(graph); + let graph = ModelResolutionTransform.apply(graph); // Custom transforms - for t in &options.custom_transforms { - t.apply(&mut graph); - } + let graph = options + .custom_transforms + .iter() + .fold(graph, |graph, transform| transform.apply(graph)); Transformed { graph, source } } diff --git a/lib/crates/fabro-workflows/src/transforms/file_inlining.rs b/lib/crates/fabro-workflows/src/transforms/file_inlining.rs index 66dbeacf5..e2518eca7 100644 --- a/lib/crates/fabro-workflows/src/transforms/file_inlining.rs +++ b/lib/crates/fabro-workflows/src/transforms/file_inlining.rs @@ -72,7 +72,8 @@ impl FileInliningTransform { } impl Transform for FileInliningTransform { - fn apply(&self, graph: &mut Graph) { + fn apply(&self, graph: Graph) -> Graph { + let mut graph = graph; let fallback = self.fallback_dir.as_deref(); // Inline @file refs in node prompts @@ -95,6 +96,8 @@ impl Transform for FileInliningTransform { .insert("goal".to_string(), AttrValue::String(resolved)); } } + + graph } } @@ -173,7 +176,7 @@ mod tests { graph.nodes.insert("work".to_string(), node); let transform = FileInliningTransform::new(dir.path().to_path_buf(), None); - transform.apply(&mut graph); + let graph = transform.apply(graph); assert_eq!( graph.nodes["work"] @@ -284,7 +287,7 @@ mod tests { base.path().to_path_buf(), Some(fallback.path().to_path_buf()), ); - transform.apply(&mut graph); + let graph = transform.apply(graph); assert_eq!( graph.nodes["work"] diff --git a/lib/crates/fabro-workflows/src/transforms/import.rs b/lib/crates/fabro-workflows/src/transforms/import.rs index 24c788b57..c73ccecc4 100644 --- a/lib/crates/fabro-workflows/src/transforms/import.rs +++ b/lib/crates/fabro-workflows/src/transforms/import.rs @@ -168,8 +168,8 @@ impl ImportTransform { let import_base_dir = resolved_path .parent() .map_or_else(|| PathBuf::from("."), Path::to_path_buf); - FileInliningTransform::new(import_base_dir.clone(), self.fallback_dir.clone()) - .apply(&mut graph); + graph = FileInliningTransform::new(import_base_dir.clone(), self.fallback_dir.clone()) + .apply(graph); if let Some(message) = Self::unresolved_imported_prompt_error(&graph) { return Err(message); @@ -585,19 +585,22 @@ impl PreparedImport { } impl Transform for ImportTransform { - fn apply(&self, graph: &mut Graph) { - let imports = Self::collect_import_nodes(graph); + fn apply(&self, graph: Graph) -> Graph { + let mut graph = graph; + let imports = Self::collect_import_nodes(&graph); let mut import_stack = Vec::new(); for (placeholder_id, import_path) in imports { self.expand_import( - graph, + &mut graph, &placeholder_id, &import_path, &self.base_dir, &mut import_stack, ); } + + graph } } @@ -622,10 +625,9 @@ mod tests { } fn apply_import(dot: &str, base_dir: &Path, fallback_dir: Option<&Path>) -> Graph { - let mut graph = parse_graph(dot); + let graph = parse_graph(dot); ImportTransform::new(base_dir.to_path_buf(), fallback_dir.map(Path::to_path_buf)) - .apply(&mut graph); - graph + .apply(graph) } fn basic_import_source() -> &'static str { @@ -1327,7 +1329,7 @@ mod tests { graph .nodes .insert("validate.lint".to_string(), colliding_node); - ImportTransform::new(dir.path().to_path_buf(), None).apply(&mut graph); + let graph = ImportTransform::new(dir.path().to_path_buf(), None).apply(graph); assert_eq!( graph.nodes["validate"] diff --git a/lib/crates/fabro-workflows/src/transforms/mod.rs b/lib/crates/fabro-workflows/src/transforms/mod.rs index befd3b037..46715c7d2 100644 --- a/lib/crates/fabro-workflows/src/transforms/mod.rs +++ b/lib/crates/fabro-workflows/src/transforms/mod.rs @@ -2,7 +2,8 @@ use fabro_graphviz::graph::Graph; /// A transform that modifies the pipeline graph after parsing and before validation. pub trait Transform { - fn apply(&self, graph: &mut Graph); + #[must_use] + fn apply(&self, graph: Graph) -> Graph; } mod file_inlining; diff --git a/lib/crates/fabro-workflows/src/transforms/model_resolution.rs b/lib/crates/fabro-workflows/src/transforms/model_resolution.rs index 9683181af..495936095 100644 --- a/lib/crates/fabro-workflows/src/transforms/model_resolution.rs +++ b/lib/crates/fabro-workflows/src/transforms/model_resolution.rs @@ -6,7 +6,8 @@ use super::Transform; pub struct ModelResolutionTransform; impl Transform for ModelResolutionTransform { - fn apply(&self, graph: &mut Graph) { + fn apply(&self, graph: Graph) -> Graph { + let mut graph = graph; for node in graph.nodes.values_mut() { let model = node .attrs @@ -29,6 +30,8 @@ impl Transform for ModelResolutionTransform { } } } + + graph } } @@ -48,7 +51,7 @@ mod tests { ); graph.nodes.insert("a".to_string(), node); - ModelResolutionTransform.apply(&mut graph); + let graph = ModelResolutionTransform.apply(graph); assert_eq!( graph.nodes["a"] @@ -73,7 +76,7 @@ mod tests { ); graph.nodes.insert("a".to_string(), node); - ModelResolutionTransform.apply(&mut graph); + let graph = ModelResolutionTransform.apply(graph); assert_eq!( graph.nodes["a"] @@ -94,7 +97,7 @@ mod tests { ); graph.nodes.insert("a".to_string(), node); - ModelResolutionTransform.apply(&mut graph); + let graph = ModelResolutionTransform.apply(graph); assert_eq!(graph.nodes["a"].attrs.get("provider"), None); } @@ -105,7 +108,7 @@ mod tests { let node = Node::new("a"); graph.nodes.insert("a".to_string(), node); - ModelResolutionTransform.apply(&mut graph); + let graph = ModelResolutionTransform.apply(graph); assert_eq!(graph.nodes["a"].attrs.get("provider"), None); } @@ -118,7 +121,7 @@ mod tests { .insert("model".to_string(), AttrValue::String("gpt-54".to_string())); graph.nodes.insert("a".to_string(), node); - ModelResolutionTransform.apply(&mut graph); + let graph = ModelResolutionTransform.apply(graph); assert_eq!( graph.nodes["a"] @@ -146,7 +149,7 @@ mod tests { ); graph.nodes.insert("a".to_string(), node); - ModelResolutionTransform.apply(&mut graph); + let graph = ModelResolutionTransform.apply(graph); assert_eq!( graph.nodes["a"] diff --git a/lib/crates/fabro-workflows/src/transforms/preamble.rs b/lib/crates/fabro-workflows/src/transforms/preamble.rs index 06f19bf07..3ab6f2a63 100644 --- a/lib/crates/fabro-workflows/src/transforms/preamble.rs +++ b/lib/crates/fabro-workflows/src/transforms/preamble.rs @@ -6,9 +6,10 @@ use super::Transform; pub struct PreambleTransform; impl Transform for PreambleTransform { - fn apply(&self, graph: &mut Graph) { + fn apply(&self, graph: Graph) -> Graph { use crate::context::keys::Fidelity; + let mut graph = graph; let default_fidelity = graph .default_fidelity() .and_then(|s| s.parse::().ok()) @@ -28,6 +29,8 @@ impl Transform for PreambleTransform { .insert("prompt".to_string(), AttrValue::String(new_prompt)); } } + + graph } } @@ -51,7 +54,7 @@ mod tests { ); graph.nodes.insert("work".to_string(), node); - PreambleTransform.apply(&mut graph); + let graph = PreambleTransform.apply(graph); let prompt = graph.nodes["work"] .attrs @@ -75,7 +78,7 @@ mod tests { ); graph.nodes.insert("work".to_string(), node); - PreambleTransform.apply(&mut graph); + let graph = PreambleTransform.apply(graph); let prompt = graph.nodes["work"] .attrs @@ -99,7 +102,7 @@ mod tests { ); graph.nodes.insert("work".to_string(), node); - PreambleTransform.apply(&mut graph); + let graph = PreambleTransform.apply(graph); let prompt = graph.nodes["work"] .attrs @@ -119,7 +122,7 @@ mod tests { ); graph.nodes.insert("work".to_string(), node); - PreambleTransform.apply(&mut graph); + let graph = PreambleTransform.apply(graph); assert!(!graph.nodes["work"].attrs.contains_key("prompt")); } diff --git a/lib/crates/fabro-workflows/src/transforms/stylesheet_application.rs b/lib/crates/fabro-workflows/src/transforms/stylesheet_application.rs index 6d44471f7..b7d7d4519 100644 --- a/lib/crates/fabro-workflows/src/transforms/stylesheet_application.rs +++ b/lib/crates/fabro-workflows/src/transforms/stylesheet_application.rs @@ -7,15 +7,17 @@ use super::stylesheet::{apply_stylesheet, parse_stylesheet}; pub struct StylesheetApplicationTransform; impl Transform for StylesheetApplicationTransform { - fn apply(&self, graph: &mut Graph) { + fn apply(&self, graph: Graph) -> Graph { + let mut graph = graph; let stylesheet_text = graph.model_stylesheet().to_string(); if stylesheet_text.is_empty() { - return; + return graph; } let Ok(stylesheet) = parse_stylesheet(&stylesheet_text) else { - return; + return graph; }; - apply_stylesheet(&stylesheet, graph); + apply_stylesheet(&stylesheet, &mut graph); + graph } } @@ -32,6 +34,6 @@ mod tests { let transform = StylesheetApplicationTransform; // Should not panic with empty stylesheet - transform.apply(&mut graph); + let _graph = transform.apply(graph); } } diff --git a/lib/crates/fabro-workflows/src/transforms/variable_expansion.rs b/lib/crates/fabro-workflows/src/transforms/variable_expansion.rs index ac55c5d1e..597321edc 100644 --- a/lib/crates/fabro-workflows/src/transforms/variable_expansion.rs +++ b/lib/crates/fabro-workflows/src/transforms/variable_expansion.rs @@ -49,7 +49,8 @@ pub fn expand_vars(source: &str, vars: &HashMap) -> anyhow::Resu pub struct VariableExpansionTransform; impl Transform for VariableExpansionTransform { - fn apply(&self, graph: &mut Graph) { + fn apply(&self, graph: Graph) -> Graph { + let mut graph = graph; let goal = graph.goal().to_string(); let vars = HashMap::from([("goal".to_string(), goal)]); for node in graph.nodes.values_mut() { @@ -62,6 +63,8 @@ impl Transform for VariableExpansionTransform { } } } + + graph } } @@ -126,7 +129,7 @@ mod tests { graph.nodes.insert("plan".to_string(), node); let transform = VariableExpansionTransform; - transform.apply(&mut graph); + let graph = transform.apply(graph); let prompt = graph.nodes["plan"] .attrs @@ -152,7 +155,7 @@ mod tests { graph.nodes.insert("plan".to_string(), node); let transform = VariableExpansionTransform; - transform.apply(&mut graph); + let graph = transform.apply(graph); let prompt = graph.nodes["plan"] .attrs @@ -173,7 +176,7 @@ mod tests { graph.nodes.insert("plan".to_string(), node); let transform = VariableExpansionTransform; - transform.apply(&mut graph); + let graph = transform.apply(graph); let prompt = graph.nodes["plan"] .attrs @@ -195,7 +198,7 @@ mod tests { let transform = VariableExpansionTransform; // Should not panic - transform.apply(&mut graph); + let graph = transform.apply(graph); assert!(!graph.nodes["plan"].attrs.contains_key("prompt")); } @@ -215,7 +218,7 @@ mod tests { graph.nodes.insert("plan".to_string(), node); let transform = VariableExpansionTransform; - transform.apply(&mut graph); + let graph = transform.apply(graph); let prompt = graph.nodes["plan"] .attrs diff --git a/lib/crates/fabro-workflows/tests/it/integration.rs b/lib/crates/fabro-workflows/tests/it/integration.rs index d021413be..a6f65dded 100644 --- a/lib/crates/fabro-workflows/tests/it/integration.rs +++ b/lib/crates/fabro-workflows/tests/it/integration.rs @@ -963,7 +963,7 @@ fn variable_expansion_replaces_goal_in_prompts() { graph.nodes.insert("report".to_string(), no_var_node); let transform = VariableExpansionTransform; - transform.apply(&mut graph); + let graph = transform.apply(graph); let plan_prompt = graph.nodes["plan"] .attrs @@ -1028,7 +1028,7 @@ fn stylesheet_application_by_specificity() { graph.nodes.insert("explicit_node".to_string(), explicit); let transform = StylesheetApplicationTransform; - transform.apply(&mut graph); + let graph = transform.apply(graph); // plan: universal -> claude-sonnet-4-5 assert_eq!( @@ -1084,11 +1084,11 @@ fn stylesheet_application_via_parsed_graph() { start -> work -> exit }"#; - let mut graph = parse(input).expect("parse should succeed"); + let graph = parse(input).expect("parse should succeed"); validate_or_raise(&graph, &[]).expect("validation should pass"); let transform = StylesheetApplicationTransform; - transform.apply(&mut graph); + let graph = transform.apply(graph); // All nodes without explicit model should get "sonnet" assert_eq!( @@ -2482,9 +2482,9 @@ async fn scenario_ship_a_feature() { review -> exit [label="[A] Approve"] review -> implement [label="[F] Fix"] }"#; - let mut graph = parse(dot).expect("parse"); + let graph = parse(dot).expect("parse"); validate_or_raise(&graph, &[]).expect("validate"); - VariableExpansionTransform.apply(&mut graph); + let graph = VariableExpansionTransform.apply(graph); assert_eq!( graph.nodes["plan"].prompt().unwrap(), "Plan to achieve: Ship the widget" @@ -3418,9 +3418,9 @@ async fn stylesheet_applies_model_override() { work [shape=box, prompt="Do work"] start -> work -> exit }"#; - let mut graph = parse(input).expect("parse"); + let graph = parse(input).expect("parse"); validate_or_raise(&graph, &[]).expect("validate"); - StylesheetApplicationTransform.apply(&mut graph); + let graph = StylesheetApplicationTransform.apply(graph); assert_eq!(graph.nodes["work"].model(), Some("custom-model")); let dir = tempfile::tempdir().unwrap(); @@ -3527,7 +3527,7 @@ async fn integration_smoke_plan_implement_review_done() { }"#; // Parse and validate - let mut graph = parse(dot).expect("parse"); + let graph = parse(dot).expect("parse"); let diagnostics = validate_or_raise(&graph, &[]).expect("validate"); let errors: Vec<_> = diagnostics .iter() @@ -3536,8 +3536,8 @@ async fn integration_smoke_plan_implement_review_done() { assert!(errors.is_empty()); // Apply transforms - VariableExpansionTransform.apply(&mut graph); - StylesheetApplicationTransform.apply(&mut graph); + let graph = VariableExpansionTransform.apply(graph); + let graph = StylesheetApplicationTransform.apply(graph); // Verify transforms applied assert_eq!(