diff --git a/docs/public/reference/dot-language.mdx b/docs/public/reference/dot-language.mdx index 8c350d1ba..ee44c3668 100644 --- a/docs/public/reference/dot-language.mdx +++ b/docs/public/reference/dot-language.mdx @@ -113,7 +113,7 @@ plan [label="Plan", prompt="Create an implementation plan."] **Node identifiers** must start with a letter or underscore, followed by letters, digits, or underscores (e.g. `run_tests`, `gate_1`, `_private`). -Nodes referenced in edges are auto-created if not explicitly declared. +Every node used by an edge needs its own declaration. Validation fails when an edge names a node the workflow never declares, because that is nearly always a typo or a rename that missed an edge. The declaration can come before or after the edges that use it, and it can live in a subgraph. ### Edge declarations diff --git a/lib/apps/fabro-cli/src/shared/utilities.rs b/lib/apps/fabro-cli/src/shared/utilities.rs index e9cd32bf6..c6b048a1d 100644 --- a/lib/apps/fabro-cli/src/shared/utilities.rs +++ b/lib/apps/fabro-cli/src/shared/utilities.rs @@ -49,54 +49,49 @@ where pub(crate) fn print_diagnostics(diagnostics: &[Diagnostic], styles: &Styles, printer: Printer) { for d in diagnostics { - let location = match (&d.node_id, &d.edge) { - (Some(node), _) => format!(" [node: {node}]"), - (_, Some((from, to))) => format!(" [edge: {from} -> {to}]"), - _ => String::new(), - }; - let source_prefix = source_prefix(d); - 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} ({})", - 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} ({})", - 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) - }), - ), + print_diagnostic(d, styles, printer); + // The fix is the actionable half of a diagnostic, so it follows every + // severity rather than hiding behind --verbose. Rules that have nothing + // useful to suggest leave it unset. + if let Some(fix) = &d.fix { + fabro_util::printerr!(printer, " {} {fix}", styles.dim.apply_to("fix:")); } } } +fn print_diagnostic(d: &Diagnostic, styles: &Styles, printer: Printer) { + let location = match (&d.node_id, &d.edge) { + (Some(node), _) => format!(" [node: {node}]"), + (_, Some((from, to))) => format!(" [edge: {from} -> {to}]"), + _ => 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 => fabro_util::printerr!( + printer, + "{}{body} ({})", + styles.red.apply_to("error"), + styles.dim.apply_to(&d.rule), + ), + Severity::Warning => fabro_util::printerr!( + printer, + "{}{body} ({})", + styles.yellow.apply_to("warning"), + styles.dim.apply_to(&d.rule), + ), + Severity::Info => fabro_util::printerr!( + printer, + "{}", + styles.dim.apply_to(format!("info{body} ({})", d.rule)), + ), + } +} + fn source_prefix(diagnostic: &Diagnostic) -> String { match ( diagnostic.source_path.as_deref(), diff --git a/lib/apps/fabro-cli/tests/it/cmd/graph.rs b/lib/apps/fabro-cli/tests/it/cmd/graph.rs index 6b801928a..fbf48599f 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/graph.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/graph.rs @@ -95,7 +95,9 @@ fn graph_allow_invalid_renders_after_diagnostics() { ----- stdout ----- ----- stderr ----- error: Pipeline must have exactly one start node (shape=Mdiamond or id start/Start) (start_node) + fix: Add a node with shape=Mdiamond or id 'start' error [node: exit]: Exit node 'exit' has 1 outgoing edge(s) but must have none (exit_no_outgoing) + fix: Remove outgoing edges from the exit node "); let svg = read_text(&output_path); @@ -119,7 +121,9 @@ fn graph_invalid_workflow_fails_after_diagnostics() { ----- stdout ----- ----- stderr ----- error: Pipeline must have exactly one start node (shape=Mdiamond or id start/Start) (start_node) + fix: Add a node with shape=Mdiamond or id 'start' error [node: exit]: Exit node 'exit' has 1 outgoing edge(s) but must have none (exit_no_outgoing) + fix: Remove outgoing edges from the exit node × Validation failed "); } diff --git a/lib/apps/fabro-cli/tests/it/cmd/preflight.rs b/lib/apps/fabro-cli/tests/it/cmd/preflight.rs index 8bb035f3a..4a2e44c8a 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/preflight.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/preflight.rs @@ -52,7 +52,9 @@ fn preflight_invalid_workflow_fails_with_validation_output() { Workflow: Invalid (2 nodes, 1 edges) Graph: [FIXTURES]/invalid.fabro error: Pipeline must have exactly one start node (shape=Mdiamond or id start/Start) (start_node) + fix: Add a node with shape=Mdiamond or id 'start' error [node: exit]: Exit node 'exit' has 1 outgoing edge(s) but must have none (exit_no_outgoing) + fix: Remove outgoing edges from the exit node × Validation failed "); } @@ -74,7 +76,9 @@ fn preflight_rejects_unbound_template_inputs() { Goal: Demo error: [FIXTURES]/templated_unbound.fabro:2:26: undefined template variable `inputs.app_dir` in graph attribute `goal` (template_undefined_variable) + fix: bind `inputs.app_dir` via `[run.inputs]` in workflow.toml, or pass `--input inputs.app_dir=` error: [FIXTURES]/templated_unbound.fabro:7:44: undefined template variable `inputs.app_dir` in node `work` attribute `prompt` [node: work] (template_undefined_variable) + fix: bind `inputs.app_dir` via `[run.inputs]` in workflow.toml, or pass `--input inputs.app_dir=` × Validation failed "); } diff --git a/lib/apps/fabro-cli/tests/it/cmd/validate.rs b/lib/apps/fabro-cli/tests/it/cmd/validate.rs index 190c2817b..bf38fe31a 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/validate.rs @@ -82,6 +82,7 @@ fn branching() { Workflow: Branch (6 nodes, 6 edges) Graph: [FIXTURES]/branching.fabro warning [node: implement]: Node 'implement' has goal_gate=true but no retry_target or fallback_retry_target (goal_gate_has_retry) + fix: Add retry_target or fallback_retry_target attribute Validation: OK "); } @@ -163,7 +164,9 @@ fn bare_fabro_with_unbound_inputs_validates_structurally_with_warning() { Workflow: TemplatedUnbound (3 nodes, 2 edges) Graph: [FIXTURES]/templated_unbound.fabro warning: [FIXTURES]/templated_unbound.fabro:2:26: undefined template variable `inputs.app_dir` in graph attribute `goal` (template_undefined_variable) + fix: bind `inputs.app_dir` via `[run.inputs]` in workflow.toml, or pass `--input inputs.app_dir=` warning: [FIXTURES]/templated_unbound.fabro:7:44: undefined template variable `inputs.app_dir` in node `work` attribute `prompt` [node: work] (template_undefined_variable) + fix: bind `inputs.app_dir` via `[run.inputs]` in workflow.toml, or pass `--input inputs.app_dir=` Validation: OK "); } @@ -186,6 +189,7 @@ fn bare_fabro_with_unbound_inputs_in_imported_prompt_validates_structurally_with Workflow: TemplatedUnboundImported (3 nodes, 2 edges) Graph: [FIXTURES]/templated_unbound_imported/workflow.fabro warning: [FIXTURES]/templated_unbound_imported/work.md:1:12: undefined template variable `inputs.app_dir` in node `work` attribute `prompt` [node: work] (template_undefined_variable) + fix: bind `inputs.app_dir` via `[run.inputs]` in workflow.toml, or pass `--input inputs.app_dir=` Validation: OK "); } @@ -207,6 +211,7 @@ fn bare_fabro_with_unbound_inputs_in_template_partial_validates_structurally_wit Workflow: TemplatedUnboundPartial (3 nodes, 2 edges) Graph: [FIXTURES]/templated_unbound_partial/workflow.fabro warning: [FIXTURES]/templated_unbound_partial/test-include.partial.md:1:4: undefined template variable `inputs.hello` in node `test_imported_include` attribute `prompt` [node: test_imported_include] (template_undefined_variable) + fix: bind `inputs.hello` via `[run.inputs]` in workflow.toml, or pass `--input inputs.hello=` Validation: OK "); } @@ -278,6 +283,26 @@ fn validate_reports_missing_template_dependency() { "); } +/// A node named only by an edge is almost always a typo, so validation must +/// fail instead of quietly running it as a default agent stage. +#[test] +fn edge_only_node() { + let context = test_context!(); + let mut cmd = context.validate(); + cmd.arg(fixture("edge_only_node.fabro")); + fabro_snapshot!(context.filters(), cmd, @" + success: false + exit_code: 1 + ----- stdout ----- + ----- stderr ----- + 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 + × Validation failed + "); +} + #[test] fn invalid() { let context = test_context!(); @@ -291,7 +316,9 @@ fn invalid() { Workflow: Invalid (2 nodes, 1 edges) Graph: [FIXTURES]/invalid.fabro error: Pipeline must have exactly one start node (shape=Mdiamond or id start/Start) (start_node) + fix: Add a node with shape=Mdiamond or id 'start' error [node: exit]: Exit node 'exit' has 1 outgoing edge(s) but must have none (exit_no_outgoing) + fix: Remove outgoing edges from the exit node × Validation failed "); } diff --git a/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs b/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs index 658dd1fea..d6852765e 100644 --- a/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs +++ b/lib/apps/fabro-cli/tests/it/workflow/dry_run_examples.rs @@ -19,6 +19,7 @@ fn dry_run_branching() { Goal: Implement and validate a feature warning [node: implement]: Node 'implement' has goal_gate=true but no retry_target or fallback_retry_target (goal_gate_has_retry) + fix: Add retry_target or fallback_retry_target attribute Run: [ULID] Web UI: http://localhost:3000/runs/[ULID] Sandbox: local (ready in [TIME]) diff --git a/lib/components/fabro-graphviz/src/parser/semantic.rs b/lib/components/fabro-graphviz/src/parser/semantic.rs index 8ac364cab..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,29 +57,44 @@ fn derive_class_from_label(label: &str) -> String { .collect() } +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(), } } - fn ensure_node(&mut self, id: &str) { - 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()); - } - self.graph.nodes.insert(id.to_string(), node); - } + node.attrs.clone_from(node_defaults); + node + }) } fn add_class_to_node(node: &mut Node, cls: &str) { @@ -90,12 +105,7 @@ impl SemanticState { } fn process_node(&mut self, node_stmt: &NodeStmt, subgraph_class: Option<&str>) { - self.ensure_node(&node_stmt.id); - 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)); @@ -111,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) { @@ -127,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); + 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); } } @@ -251,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) @@ -515,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 { @@ -524,8 +530,83 @@ mod tests { })], }; + let graph = ast_to_graph(&dot).unwrap(); + assert!(graph.nodes.is_empty()); + assert_eq!(graph.edges, vec![Edge::new("a", "b")]); + } + + #[test] + fn ast_to_graph_includes_only_declared_edge_endpoints() { + let dot = DotGraph { + name: "Declared".into(), + statements: vec![ + Statement::Node(NodeStmt { + id: "a".into(), + attrs: None, + }), + Statement::Edge(EdgeStmt { + nodes: vec!["a".into(), "b".into()], + attrs: None, + }), + ], + }; + let graph = ast_to_graph(&dot).unwrap(); assert!(graph.nodes.contains_key("a")); + assert!(!graph.nodes.contains_key("b")); + } + + #[test] + fn ast_to_graph_declaration_after_edge_still_counts() { + 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()))]), + }), + ], + }; + + let graph = ast_to_graph(&dot).unwrap(); 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] + fn ast_to_graph_subgraph_declaration_counts() { + let dot = DotGraph { + name: "SubgraphDeclared".into(), + statements: vec![ + Statement::Edge(EdgeStmt { + nodes: vec!["start".into(), "plan".into()], + attrs: None, + }), + Statement::Subgraph(SubgraphStmt { + name: Some("cluster_loop".into()), + statements: vec![Statement::Node(NodeStmt { + id: "plan".into(), + attrs: None, + })], + }), + ], + }; + + let graph = ast_to_graph(&dot).unwrap(); + 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 8cfe67282..3cfbe2130 100644 --- a/lib/components/fabro-validate/src/rules/edge_target_exists.rs +++ b/lib/components/fabro-validate/src/rules/edge_target_exists.rs @@ -1,3 +1,5 @@ +use std::collections::HashSet; + use fabro_graphviz::graph::Graph; use crate::{Diagnostic, LintRule, Severity}; @@ -8,6 +10,26 @@ pub(super) fn rule() -> Box { struct Rule; +impl Rule { + fn diagnostic(&self, node_id: &str, from: &str, to: &str) -> Diagnostic { + Diagnostic { + rule: self.name().to_string(), + severity: Severity::Error, + message: format!( + "Node '{node_id}' is referenced by edge '{from} -> {to}' but has no node \ + declaration" + ), + node_id: Some(node_id.to_string()), + edge: Some((from.to_string(), to.to_string())), + fix: Some(format!( + "Declare node '{node_id}' or correct the edge endpoint" + )), + + ..Diagnostic::default() + } + } +} + impl LintRule for Rule { fn name(&self) -> &'static str { "edge_target_exists" @@ -15,36 +37,12 @@ impl LintRule for Rule { fn apply(&self, graph: &Graph) -> Vec { let mut diagnostics = Vec::new(); + let mut reported = HashSet::new(); for edge in &graph.edges { - if !graph.nodes.contains_key(&edge.to) { - diagnostics.push(Diagnostic { - rule: self.name().to_string(), - severity: Severity::Error, - message: format!( - "Edge from '{}' targets non-existent node '{}'", - edge.from, edge.to - ), - node_id: None, - edge: Some((edge.from.clone(), edge.to.clone())), - fix: Some(format!("Define node '{}' or fix the edge target", edge.to)), - - ..Diagnostic::default() - }); - } - if !graph.nodes.contains_key(&edge.from) { - diagnostics.push(Diagnostic { - rule: self.name().to_string(), - severity: Severity::Error, - message: format!("Edge source '{}' references non-existent node", edge.from), - node_id: None, - edge: Some((edge.from.clone(), edge.to.clone())), - fix: Some(format!( - "Define node '{}' or fix the edge source", - edge.from - )), - - ..Diagnostic::default() - }); + for endpoint in [&edge.to, &edge.from] { + if !graph.nodes.contains_key(endpoint) && reported.insert(endpoint) { + diagnostics.push(self.diagnostic(endpoint, &edge.from, &edge.to)); + } } } diagnostics @@ -53,11 +51,112 @@ impl LintRule for Rule { #[cfg(test)] mod tests { - use fabro_graphviz::graph::Edge; + use fabro_graphviz::graph::{Edge, Graph}; + use fabro_graphviz::parser; use super::Rule; use crate::rules::test_support::minimal_graph; - use crate::{LintRule, Severity}; + use crate::{Diagnostic, LintRule, Severity}; + + fn parse(dot: &str) -> Graph { + parser::parse(dot).expect("fixture should parse") + } + + fn undeclared_nodes(graph: &Graph) -> Vec { + Rule.apply(graph) + .into_iter() + .map(|d| d.node_id.expect("diagnostic should name a node")) + .collect() + } + + #[test] + fn edge_only_node_is_rejected() { + let graph = parse( + r"digraph EdgeOnly { + start [shape=Mdiamond] + exit [shape=Msquare] + start -> misspelled_node + misspelled_node -> exit + }", + ); + + let diagnostics = Rule.apply(&graph); + assert_eq!(diagnostics.len(), 1, "diagnostics: {diagnostics:?}"); + let Diagnostic { + severity, + node_id, + edge, + .. + } = &diagnostics[0]; + assert_eq!(*severity, Severity::Error); + assert_eq!(node_id.as_deref(), Some("misspelled_node")); + assert_eq!( + edge.clone(), + Some(("start".to_string(), "misspelled_node".to_string())) + ); + } + + #[test] + fn declaration_after_the_edge_is_accepted() { + let graph = parse( + r#"digraph DeclaredLater { + start -> work + work [prompt="Do the work"] + work -> exit + start [shape=Mdiamond] + exit [shape=Msquare] + }"#, + ); + + assert!(Rule.apply(&graph).is_empty()); + } + + #[test] + fn chained_edges_report_every_undeclared_endpoint() { + let graph = parse( + r"digraph Chained { + start [shape=Mdiamond] + exit [shape=Msquare] + start -> first -> second -> exit + }", + ); + + assert_eq!(undeclared_nodes(&graph), vec!["first", "second"]); + } + + #[test] + fn a_node_is_reported_once_no_matter_how_many_edges_use_it() { + let graph = parse( + r"digraph Repeated { + start [shape=Mdiamond] + exit [shape=Msquare] + start -> typo + typo -> exit + typo -> start + }", + ); + + assert_eq!(undeclared_nodes(&graph), vec!["typo"]); + } + + #[test] + fn subgraph_declaration_is_accepted() { + let graph = parse( + r#"digraph Subgraphed { + start [shape=Mdiamond] + exit [shape=Msquare] + + subgraph cluster_loop { + label = "Loop A" + plan [prompt="Plan the work"] + } + + start -> plan -> exit + }"#, + ); + + assert!(Rule.apply(&graph).is_empty()); + } #[test] fn edge_target_exists_rule_missing_target() { diff --git a/lib/components/fabro-workflow/src/transforms/import.rs b/lib/components/fabro-workflow/src/transforms/import.rs index b5406ddc6..f2c5e74fd 100644 --- a/lib/components/fabro-workflow/src/transforms/import.rs +++ b/lib/components/fabro-workflow/src/transforms/import.rs @@ -339,18 +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.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); } @@ -649,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 + ) } } @@ -909,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")); @@ -951,6 +955,72 @@ mod tests { ); } + #[test] + fn edge_only_node_in_imported_fragment_stays_missing() { + let dir = tempfile::tempdir().unwrap(); + write_file( + &dir.path().join("validate.fabro"), + r#"digraph validate { + start [shape=Mdiamond] + lint [prompt="Run clippy"] + exit [shape=Msquare] + start -> lint -> 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.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] fn import_reports_structural_diagnostic_for_imported_prompt_templates() { let dir = tempfile::tempdir().unwrap(); @@ -1130,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/components/fabro-workflow/tests/it/integration.rs b/lib/components/fabro-workflow/tests/it/integration.rs index dc13bcbf8..ec716adbc 100644 --- a/lib/components/fabro-workflow/tests/it/integration.rs +++ b/lib/components/fabro-workflow/tests/it/integration.rs @@ -388,6 +388,9 @@ fn parse_and_validate_human_gate() { type="human" ] + ship_it [prompt="Ship the change"] + fixes [prompt="Apply the requested fixes"] + start -> review_gate review_gate -> ship_it [label="[A] Approve"] review_gate -> fixes [label="[F] Fix"] diff --git a/lib/foundation/fabro-types/src/run_event/mod.rs b/lib/foundation/fabro-types/src/run_event/mod.rs index d4c8e55c1..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,20 +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(), - })]), - 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", diff --git a/test/edge_only_node.fabro b/test/edge_only_node.fabro new file mode 100644 index 000000000..3d45b3346 --- /dev/null +++ b/test/edge_only_node.fabro @@ -0,0 +1,10 @@ +digraph EdgeOnlyNode { + graph [goal="Reference a node that was never declared"] + + /* `misspelled_node` is only ever named by an edge, never declared. */ + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + + start -> misspelled_node + misspelled_node -> exit +}