diff --git a/lib/apps/fabro-cli/src/shared/utilities.rs b/lib/apps/fabro-cli/src/shared/utilities.rs index 8cfb05af7..c6b048a1d 100644 --- a/lib/apps/fabro-cli/src/shared/utilities.rs +++ b/lib/apps/fabro-cli/src/shared/utilities.rs @@ -66,43 +66,28 @@ fn print_diagnostic(d: &Diagnostic, styles: &Styles, printer: Printer) { _ => String::new(), }; let source_prefix = source_prefix(d); + let body = if source_prefix.is_empty() { + format!("{location}: {}", d.message) + } else { + format!(": {source_prefix}{}{location}", d.message) + }; match d.severity { - Severity::Error if source_prefix.is_empty() => fabro_util::printerr!( - printer, - "{}{location}: {} ({})", - styles.red.apply_to("error"), - d.message, - styles.dim.apply_to(&d.rule), - ), Severity::Error => fabro_util::printerr!( printer, - "{}: {source_prefix}{}{location} ({})", + "{}{body} ({})", styles.red.apply_to("error"), - d.message, - styles.dim.apply_to(&d.rule), - ), - Severity::Warning if source_prefix.is_empty() => fabro_util::printerr!( - printer, - "{}{location}: {} ({})", - styles.yellow.apply_to("warning"), - d.message, styles.dim.apply_to(&d.rule), ), Severity::Warning => fabro_util::printerr!( printer, - "{}: {source_prefix}{}{location} ({})", + "{}{body} ({})", styles.yellow.apply_to("warning"), - d.message, styles.dim.apply_to(&d.rule), ), Severity::Info => fabro_util::printerr!( printer, "{}", - styles.dim.apply_to(if source_prefix.is_empty() { - format!("info{location}: {} ({})", d.message, d.rule) - } else { - format!("info: {source_prefix}{}{location} ({})", d.message, d.rule) - }), + styles.dim.apply_to(format!("info{body} ({})", d.rule)), ), } } diff --git a/lib/apps/fabro-cli/tests/it/cmd/validate.rs b/lib/apps/fabro-cli/tests/it/cmd/validate.rs index caeab11ca..bf38fe31a 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/validate.rs @@ -295,12 +295,10 @@ fn edge_only_node() { exit_code: 1 ----- stdout ----- ----- stderr ----- - Workflow: EdgeOnlyNode (3 nodes, 2 edges) + Workflow: EdgeOnlyNode (2 nodes, 2 edges) Graph: [FIXTURES]/edge_only_node.fabro error [node: misspelled_node]: Node 'misspelled_node' is referenced by edge 'start -> misspelled_node' but has no node declaration (edge_target_exists) fix: Declare node 'misspelled_node' or correct the edge endpoint - warning [node: misspelled_node]: LLM node 'misspelled_node' has no prompt or label attribute (prompt_on_llm_nodes) - fix: Add a prompt or label attribute × Validation failed "); } diff --git a/lib/components/fabro-graphviz/src/parser/semantic.rs b/lib/components/fabro-graphviz/src/parser/semantic.rs index 4e9433e6b..ed68a0fa8 100644 --- a/lib/components/fabro-graphviz/src/parser/semantic.rs +++ b/lib/components/fabro-graphviz/src/parser/semantic.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::time::Duration; use crate::error::Error; @@ -57,49 +57,44 @@ fn derive_class_from_label(label: &str) -> String { .collect() } -/// How a statement named a node. A node stays implicit only while every -/// mention of it is an edge endpoint, so declaration order does not matter. -#[derive(Clone, Copy, PartialEq, Eq)] -enum Mention { - Declaration, - EdgeEndpoint, +fn collect_declared_node_ids(statements: &[Statement], node_ids: &mut HashSet) { + for statement in statements { + match statement { + Statement::Node(node) => { + node_ids.insert(node.id.clone()); + } + Statement::Subgraph(subgraph) => { + collect_declared_node_ids(&subgraph.statements, node_ids); + } + _ => {} + } + } } struct SemanticState { - graph: Graph, - node_defaults: HashMap, - edge_defaults: HashMap, + graph: Graph, + declared_node_ids: HashSet, + node_defaults: HashMap, + edge_defaults: HashMap, } impl SemanticState { - fn new(name: String) -> Self { + fn new(name: String, declared_node_ids: HashSet) -> Self { Self { - graph: Graph::new(name), + graph: Graph::new(name), + declared_node_ids, node_defaults: HashMap::new(), edge_defaults: HashMap::new(), } } - /// Insert the node if this is the first statement to mention it, and record - /// whether the workflow ever declares it. - fn ensure_node(&mut self, id: &str, mention: Mention) { - if !self.graph.nodes.contains_key(id) { + fn ensure_node(&mut self, id: &str) -> &mut Node { + let node_defaults = &self.node_defaults; + self.graph.nodes.entry(id.to_string()).or_insert_with(|| { let mut node = Node::new(id); - for (k, v) in &self.node_defaults { - node.attrs.insert(k.clone(), v.clone()); - } - node.implicit = mention == Mention::EdgeEndpoint; - self.graph.nodes.insert(id.to_string(), node); - return; - } - if mention == Mention::Declaration { - let node = self - .graph - .nodes - .get_mut(id) - .expect("contains_key returned true, so get_mut cannot return None"); - node.implicit = false; - } + node.attrs.clone_from(node_defaults); + node + }) } fn add_class_to_node(node: &mut Node, cls: &str) { @@ -110,12 +105,7 @@ impl SemanticState { } fn process_node(&mut self, node_stmt: &NodeStmt, subgraph_class: Option<&str>) { - self.ensure_node(&node_stmt.id, Mention::Declaration); - let node = self - .graph - .nodes - .get_mut(&node_stmt.id) - .expect("node was just inserted by ensure_node, so get_mut cannot return None"); + let node = self.ensure_node(&node_stmt.id); if let Some(attrs) = &node_stmt.attrs { for (k, v) in attrs { node.attrs.insert(k.clone(), convert_value(v)); @@ -131,11 +121,6 @@ impl SemanticState { .and_then(AttrValue::as_str) .map(String::from); if let Some(class_str) = class_str { - let node = self - .graph - .nodes - .get_mut(&node_stmt.id) - .expect("node was just inserted by ensure_node, so get_mut cannot return None"); for cls in class_str.split(',') { let cls = cls.trim().to_string(); if !cls.is_empty() && !node.classes.contains(&cls) { @@ -147,12 +132,11 @@ impl SemanticState { fn process_edge(&mut self, edge_stmt: &EdgeStmt, subgraph_class: Option<&str>) { for id in &edge_stmt.nodes { - self.ensure_node(id, Mention::EdgeEndpoint); + if !self.declared_node_ids.contains(id) { + continue; + } + let node = self.ensure_node(id); if let Some(cls) = subgraph_class { - let node = - self.graph.nodes.get_mut(id).expect( - "node was just inserted by ensure_node, so get_mut cannot return None", - ); Self::add_class_to_node(node, cls); } } @@ -271,7 +255,9 @@ impl SemanticState { /// /// Returns an error if the AST cannot be converted to a valid graph. pub fn ast_to_graph(dot: &DotGraph) -> Result { - let mut state = SemanticState::new(dot.name.clone()); + let mut declared_node_ids = HashSet::new(); + collect_declared_node_ids(&dot.statements, &mut declared_node_ids); + let mut state = SemanticState::new(dot.name.clone(), declared_node_ids); let empty = HashMap::new(); state.process_statements(&dot.statements, None, &empty, &empty); Ok(state.graph) @@ -535,7 +521,7 @@ mod tests { } #[test] - fn ast_to_graph_implicit_nodes_from_edges() { + fn ast_to_graph_keeps_undeclared_edge_endpoints_out_of_nodes() { let dot = DotGraph { name: "Implicit".into(), statements: vec![Statement::Edge(EdgeStmt { @@ -545,14 +531,12 @@ mod tests { }; let graph = ast_to_graph(&dot).unwrap(); - assert!(graph.nodes.contains_key("a")); - assert!(graph.nodes.contains_key("b")); - assert!(graph.nodes["a"].implicit); - assert!(graph.nodes["b"].implicit); + assert!(graph.nodes.is_empty()); + assert_eq!(graph.edges, vec![Edge::new("a", "b")]); } #[test] - fn ast_to_graph_marks_declared_nodes_explicit() { + fn ast_to_graph_includes_only_declared_edge_endpoints() { let dot = DotGraph { name: "Declared".into(), statements: vec![ @@ -568,8 +552,8 @@ mod tests { }; let graph = ast_to_graph(&dot).unwrap(); - assert!(!graph.nodes["a"].implicit); - assert!(graph.nodes["b"].implicit); + assert!(graph.nodes.contains_key("a")); + assert!(!graph.nodes.contains_key("b")); } #[test] @@ -577,10 +561,12 @@ mod tests { let dot = DotGraph { name: "DeclaredLater".into(), statements: vec![ + Statement::NodeDefaults(vec![("model".into(), AstValue::Str("first".into()))]), Statement::Edge(EdgeStmt { nodes: vec!["a".into(), "b".into()], attrs: None, }), + Statement::NodeDefaults(vec![("model".into(), AstValue::Str("second".into()))]), Statement::Node(NodeStmt { id: "b".into(), attrs: Some(vec![("prompt".into(), AstValue::Str("Do it".into()))]), @@ -589,7 +575,15 @@ mod tests { }; let graph = ast_to_graph(&dot).unwrap(); - assert!(!graph.nodes["b"].implicit); + assert!(graph.nodes.contains_key("b")); + assert!(!graph.nodes.contains_key("a")); + assert_eq!( + graph.nodes["b"] + .attrs + .get("model") + .and_then(AttrValue::as_str), + Some("first") + ); } #[test] @@ -612,7 +606,7 @@ mod tests { }; let graph = ast_to_graph(&dot).unwrap(); - assert!(!graph.nodes["plan"].implicit); - assert!(graph.nodes["start"].implicit); + assert!(graph.nodes.contains_key("plan")); + assert!(!graph.nodes.contains_key("start")); } } diff --git a/lib/components/fabro-validate/src/rules/edge_target_exists.rs b/lib/components/fabro-validate/src/rules/edge_target_exists.rs index adc70283b..3cfbe2130 100644 --- a/lib/components/fabro-validate/src/rules/edge_target_exists.rs +++ b/lib/components/fabro-validate/src/rules/edge_target_exists.rs @@ -11,13 +11,6 @@ pub(super) fn rule() -> Box { struct Rule; impl Rule { - /// An edge endpoint is only usable when the workflow declares it. A node - /// the parser synthesized from the edge itself carries no attributes, so it - /// would silently run as a default agent stage. - fn is_declared(graph: &Graph, node_id: &str) -> bool { - graph.nodes.get(node_id).is_some_and(|node| !node.implicit) - } - fn diagnostic(&self, node_id: &str, from: &str, to: &str) -> Diagnostic { Diagnostic { rule: self.name().to_string(), @@ -47,7 +40,7 @@ impl LintRule for Rule { let mut reported = HashSet::new(); for edge in &graph.edges { for endpoint in [&edge.to, &edge.from] { - if !Self::is_declared(graph, endpoint) && reported.insert(endpoint) { + if !graph.nodes.contains_key(endpoint) && reported.insert(endpoint) { diagnostics.push(self.diagnostic(endpoint, &edge.from, &edge.to)); } } @@ -71,8 +64,8 @@ mod tests { fn undeclared_nodes(graph: &Graph) -> Vec { Rule.apply(graph) - .iter() - .map(|d| d.node_id.clone().expect("diagnostic should name a node")) + .into_iter() + .map(|d| d.node_id.expect("diagnostic should name a node")) .collect() } diff --git a/lib/components/fabro-workflow/src/transforms/import.rs b/lib/components/fabro-workflow/src/transforms/import.rs index cfda146c3..f2c5e74fd 100644 --- a/lib/components/fabro-workflow/src/transforms/import.rs +++ b/lib/components/fabro-workflow/src/transforms/import.rs @@ -339,19 +339,18 @@ impl ImportTransform { .edges .retain(|edge| edge.from != placeholder_id && edge.to != placeholder_id); - for (node_id, node) in imported_graph.nodes { + for (node_id, mut merged_node) in imported_graph.nodes { if node_id == start_id || node_id == exit_id { continue; } let prefixed_id = format!("{placeholder_id}.{node_id}"); - let mut merged_node = Node::new(&prefixed_id); - merged_node.implicit = node.implicit; + merged_node.id.clone_from(&prefixed_id); + let imported_attrs = std::mem::take(&mut merged_node.attrs); merged_node.attrs.clone_from(&placeholder.default_attrs); - merged_node.attrs.extend(node.attrs); + merged_node.attrs.extend(imported_attrs); Self::remap_retry_target(&mut merged_node.attrs, placeholder_id); - merged_node.classes = node.classes; for class_name in &placeholder.class_names { Self::push_class(&mut merged_node.classes, class_name); } @@ -650,7 +649,10 @@ impl PreparedImport { self.graph.nodes.iter().all(|(node_id, node)| { ImportTransform::is_start_sentinel(node_id, node) || ImportTransform::is_exit_sentinel(node_id, node) - }) + }) && matches!( + self.graph.edges.as_slice(), + [edge] if edge.from == self.start_id && edge.to == self.exit_id + ) } } @@ -910,6 +912,7 @@ mod tests { assert!(!graph.nodes.contains_key("validate")); assert!(graph.nodes.contains_key("validate.lint")); assert!(graph.nodes.contains_key("validate.test")); + assert_eq!(graph.nodes["validate.lint"].id, "validate.lint"); assert!(!graph.nodes.contains_key("validate.start")); assert!(!graph.nodes.contains_key("validate.exit")); @@ -953,27 +956,7 @@ mod tests { } #[test] - fn imported_node_declarations_survive_splicing() { - let dir = tempfile::tempdir().unwrap(); - write_file(&dir.path().join("validate.fabro"), basic_import_source()); - - let graph = apply_import( - r#"digraph Deploy { - start [shape=Mdiamond] - validate [import="./validate.fabro"] - exit [shape=Msquare] - start -> validate -> exit - }"#, - dir.path(), - None, - ); - - assert!(!graph.nodes["validate.lint"].implicit); - assert!(!graph.nodes["validate.test"].implicit); - } - - #[test] - fn edge_only_node_in_imported_fragment_stays_undeclared() { + fn edge_only_node_in_imported_fragment_stays_missing() { let dir = tempfile::tempdir().unwrap(); write_file( &dir.path().join("validate.fabro"), @@ -996,8 +979,46 @@ mod tests { None, ); - assert!(graph.nodes["validate.typo"].implicit); - assert!(!graph.nodes["validate.lint"].implicit); + assert!(!graph.nodes.contains_key("validate.typo")); + assert!(graph.nodes.contains_key("validate.lint")); + } + + #[test] + fn edge_only_body_is_not_treated_as_empty_import() { + let dir = tempfile::tempdir().unwrap(); + write_file( + &dir.path().join("validate.fabro"), + r"digraph validate { + start [shape=Mdiamond] + exit [shape=Msquare] + start -> typo -> exit + }", + ); + + let graph = apply_import( + r#"digraph Deploy { + start [shape=Mdiamond] + validate [import="./validate.fabro"] + exit [shape=Msquare] + start -> validate -> exit + }"#, + dir.path(), + None, + ); + + assert!(!graph.nodes.contains_key("validate.typo")); + assert!( + graph + .edges + .iter() + .any(|edge| edge.from == "start" && edge.to == "validate.typo") + ); + assert!( + graph + .edges + .iter() + .any(|edge| edge.from == "validate.typo" && edge.to == "exit") + ); } #[test] @@ -1179,6 +1200,48 @@ mod tests { ); } + #[test] + fn imported_start_and_exit_sentinels_must_be_declared() { + let dir = tempfile::tempdir().unwrap(); + let host = r#"digraph Deploy { + start [shape=Mdiamond] + validate [import="./validate.fabro"] + exit [shape=Msquare] + start -> validate -> exit + }"#; + let cases = [ + ( + r#"digraph validate { + work [prompt="Run checks"] + exit [shape=Msquare] + start -> work -> exit + }"#, + "imported workflow must have exactly one start node, found 0", + ), + ( + r#"digraph validate { + start [shape=Mdiamond] + work [prompt="Run checks"] + start -> work -> exit + }"#, + "imported workflow must have exactly one exit node, found 0", + ), + ]; + + for (source, expected_error) in cases { + write_file(&dir.path().join("validate.fabro"), source); + let graph = apply_import(host, dir.path(), None); + + assert_eq!( + graph.nodes["validate"] + .attrs + .get("import_error") + .and_then(AttrValue::as_str), + Some(expected_error) + ); + } + } + #[test] fn multiple_entry_nodes_poison_placeholder() { let dir = tempfile::tempdir().unwrap(); diff --git a/lib/foundation/fabro-types/src/graph.rs b/lib/foundation/fabro-types/src/graph.rs index 3ed6851a6..7ef9bad99 100644 --- a/lib/foundation/fabro-types/src/graph.rs +++ b/lib/foundation/fabro-types/src/graph.rs @@ -119,26 +119,20 @@ pub fn shape_to_handler_type(shape: &str) -> Option<&'static str> { /// A node in the workflow graph. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct Node { - pub id: String, - pub attrs: HashMap, + pub id: String, + pub attrs: HashMap, /// CSS-like classes for model stylesheet targeting (from `class` attr and /// subgraph derivation). #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub classes: Vec, - /// True when the node was synthesized from an edge endpoint instead of a - /// node declaration. Validation rejects these because an edge-only node in - /// an executable workflow is almost always a typo. - #[serde(default, skip_serializing_if = "std::ops::Not::not")] - pub implicit: bool, + pub classes: Vec, } impl Node { pub fn new(id: impl Into) -> Self { Self { - id: id.into(), - attrs: HashMap::new(), - classes: Vec::new(), - implicit: false, + id: id.into(), + attrs: HashMap::new(), + classes: Vec::new(), } } diff --git a/lib/foundation/fabro-types/src/run_event/mod.rs b/lib/foundation/fabro-types/src/run_event/mod.rs index e4e99e59f..6680fff2e 100644 --- a/lib/foundation/fabro-types/src/run_event/mod.rs +++ b/lib/foundation/fabro-types/src/run_event/mod.rs @@ -926,8 +926,6 @@ impl<'de> Deserialize<'de> for RunEvent { #[cfg(test)] mod tests { - use std::collections::HashMap; - use serde_json::json; use super::*; @@ -992,21 +990,9 @@ mod tests { #[test] fn run_event_deserializes_adjacent_layout() { let settings = WorkflowSettings::default(); - let graph = Graph { - name: "test".to_string(), - nodes: HashMap::from([("start".to_string(), Node { - id: "start".to_string(), - attrs: HashMap::new(), - classes: Vec::new(), - implicit: false, - })]), - edges: vec![Edge { - from: "start".to_string(), - to: "done".to_string(), - attrs: HashMap::new(), - }], - attrs: HashMap::new(), - }; + let mut graph = Graph::new("test"); + graph.nodes.insert("start".to_string(), Node::new("start")); + graph.edges.push(Edge::new("start", "done")); let line = json!({ "id": "evt_2",