From 59b1c2e59ffca7cf5f491c4fe65e7650afb48a28 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Jul 2026 13:53:54 -0400 Subject: [PATCH] Reject nodes referenced by an edge but never declared MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DOT parser created a node for every edge endpoint, and nothing recorded whether a node came from a declaration or was synthesized from an edge. The edge_target_exists rule only checked whether the node id was present in the graph, which was always true by then, so a misspelled endpoint became an attribute-free node that defaulted to shape=box — an LLM stage. Validation emitted a prompt_on_llm_nodes warning and exited 0. Node now carries `implicit`, set only when the parser synthesizes the node from an edge endpoint. A declaration anywhere in the workflow clears it, so order does not matter and subgraph declarations count. Node::new leaves it false, so programmatic construction and graphs deserialized from older checkpoints read as declared. edge_target_exists treats an endpoint as valid only when it exists and is declared, reporting each undeclared node once. The near-identical missing-source and missing-target branches collapse into one path. The import transform copies the flag onto spliced nodes so an edge-only node inside an imported fragment is caught too. parse_and_validate_human_gate had two edge-only nodes and now declares them; it was an instance of the bug rather than a casualty of the fix. No shipped workflow, docs example, or CLI fixture relied on the old behavior. Co-Authored-By: Claude Opus 5 (1M context) --- docs/public/reference/dot-language.mdx | 2 +- lib/apps/fabro-cli/tests/it/cmd/validate.rs | 20 +++ .../fabro-graphviz/src/parser/semantic.rs | 93 +++++++++- .../src/rules/edge_target_exists.rs | 168 ++++++++++++++---- .../fabro-workflow/src/transforms/import.rs | 49 +++++ .../fabro-workflow/tests/it/integration.rs | 3 + lib/foundation/fabro-types/src/graph.rs | 18 +- .../fabro-types/src/run_event/mod.rs | 7 +- test/edge_only_node.fabro | 10 ++ 9 files changed, 326 insertions(+), 44 deletions(-) create mode 100644 test/edge_only_node.fabro 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/tests/it/cmd/validate.rs b/lib/apps/fabro-cli/tests/it/cmd/validate.rs index 190c2817b..98b0f9cfe 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/validate.rs @@ -278,6 +278,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 (3 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) + warning [node: misspelled_node]: LLM node 'misspelled_node' has no prompt or label attribute (prompt_on_llm_nodes) + × Validation failed + "); +} + #[test] fn invalid() { let context = test_context!(); diff --git a/lib/components/fabro-graphviz/src/parser/semantic.rs b/lib/components/fabro-graphviz/src/parser/semantic.rs index 8ac364cab..4e9433e6b 100644 --- a/lib/components/fabro-graphviz/src/parser/semantic.rs +++ b/lib/components/fabro-graphviz/src/parser/semantic.rs @@ -57,6 +57,14 @@ 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, +} + struct SemanticState { graph: Graph, node_defaults: HashMap, @@ -72,13 +80,25 @@ impl SemanticState { } } - fn ensure_node(&mut self, id: &str) { + /// 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) { 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; } } @@ -90,7 +110,7 @@ impl SemanticState { } fn process_node(&mut self, node_stmt: &NodeStmt, subgraph_class: Option<&str>) { - self.ensure_node(&node_stmt.id); + self.ensure_node(&node_stmt.id, Mention::Declaration); let node = self .graph .nodes @@ -127,7 +147,7 @@ impl SemanticState { fn process_edge(&mut self, edge_stmt: &EdgeStmt, subgraph_class: Option<&str>) { for id in &edge_stmt.nodes { - self.ensure_node(id); + self.ensure_node(id, Mention::EdgeEndpoint); if let Some(cls) = subgraph_class { let node = self.graph.nodes.get_mut(id).expect( @@ -527,5 +547,72 @@ 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); + } + + #[test] + fn ast_to_graph_marks_declared_nodes_explicit() { + 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["a"].implicit); + assert!(graph.nodes["b"].implicit); + } + + #[test] + fn ast_to_graph_declaration_after_edge_still_counts() { + let dot = DotGraph { + name: "DeclaredLater".into(), + statements: vec![ + Statement::Edge(EdgeStmt { + nodes: vec!["a".into(), "b".into()], + attrs: None, + }), + 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["b"].implicit); + } + + #[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["plan"].implicit); + assert!(graph.nodes["start"].implicit); } } 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..adc70283b 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,33 @@ 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(), + 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 +44,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 !Self::is_declared(graph, endpoint) && reported.insert(endpoint) { + diagnostics.push(self.diagnostic(endpoint, &edge.from, &edge.to)); + } } } diagnostics @@ -53,11 +58,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) + .iter() + .map(|d| d.node_id.clone().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..cfda146c3 100644 --- a/lib/components/fabro-workflow/src/transforms/import.rs +++ b/lib/components/fabro-workflow/src/transforms/import.rs @@ -346,6 +346,7 @@ impl ImportTransform { let prefixed_id = format!("{placeholder_id}.{node_id}"); let mut merged_node = Node::new(&prefixed_id); + merged_node.implicit = node.implicit; merged_node.attrs.clone_from(&placeholder.default_attrs); merged_node.attrs.extend(node.attrs); Self::remap_retry_target(&mut merged_node.attrs, placeholder_id); @@ -951,6 +952,54 @@ 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() { + 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["validate.typo"].implicit); + assert!(!graph.nodes["validate.lint"].implicit); + } + #[test] fn import_reports_structural_diagnostic_for_imported_prompt_templates() { 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/graph.rs b/lib/foundation/fabro-types/src/graph.rs index 7ef9bad99..3ed6851a6 100644 --- a/lib/foundation/fabro-types/src/graph.rs +++ b/lib/foundation/fabro-types/src/graph.rs @@ -119,20 +119,26 @@ 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, + 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, } impl Node { pub fn new(id: impl Into) -> Self { Self { - id: id.into(), - attrs: HashMap::new(), - classes: Vec::new(), + id: id.into(), + attrs: HashMap::new(), + classes: Vec::new(), + implicit: false, } } diff --git a/lib/foundation/fabro-types/src/run_event/mod.rs b/lib/foundation/fabro-types/src/run_event/mod.rs index d4c8e55c1..e4e99e59f 100644 --- a/lib/foundation/fabro-types/src/run_event/mod.rs +++ b/lib/foundation/fabro-types/src/run_event/mod.rs @@ -995,9 +995,10 @@ mod tests { let graph = Graph { name: "test".to_string(), nodes: HashMap::from([("start".to_string(), Node { - id: "start".to_string(), - attrs: HashMap::new(), - classes: Vec::new(), + id: "start".to_string(), + attrs: HashMap::new(), + classes: Vec::new(), + implicit: false, })]), edges: vec![Edge { from: "start".to_string(), 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 +}