mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-08-28 05:27:41 +00:00
Reject nodes referenced by an edge but never declared
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) <noreply@anthropic.com>
This commit is contained in:
parent
6efba896f4
commit
59b1c2e59f
9 changed files with 326 additions and 44 deletions
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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!();
|
||||
|
|
|
|||
|
|
@ -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<String, AttrValue>,
|
||||
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<dyn LintRule> {
|
|||
|
||||
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<Diagnostic> {
|
||||
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<String> {
|
||||
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() {
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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"]
|
||||
|
|
|
|||
|
|
@ -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<String, AttrValue>,
|
||||
pub id: String,
|
||||
pub attrs: HashMap<String, AttrValue>,
|
||||
/// CSS-like classes for model stylesheet targeting (from `class` attr and
|
||||
/// subgraph derivation).
|
||||
#[serde(default, skip_serializing_if = "Vec::is_empty")]
|
||||
pub classes: Vec<String>,
|
||||
pub classes: Vec<String>,
|
||||
/// 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<String>) -> Self {
|
||||
Self {
|
||||
id: id.into(),
|
||||
attrs: HashMap::new(),
|
||||
classes: Vec::new(),
|
||||
id: id.into(),
|
||||
attrs: HashMap::new(),
|
||||
classes: Vec::new(),
|
||||
implicit: false,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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(),
|
||||
|
|
|
|||
10
test/edge_only_node.fabro
Normal file
10
test/edge_only_node.fabro
Normal file
|
|
@ -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
|
||||
}
|
||||
Loading…
Add table
Reference in a new issue