From 0748d0c6b83c67ab2d106b71116d17f076c1bf56 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Jul 2026 10:48:18 -0400 Subject: [PATCH 1/3] feat(validate): warn on inert node and edge attributes Add two lint rules so the graph format stops silently accepting attributes that nothing reads: - inert_attribute: handler-specific attributes (script, language, duration, join_policy, max_parallel, output_schema, prompt) placed on node types that never read them. Attributes read by several handlers (timeout), resolved for every node (fidelity, retry_policy), or injectable via model stylesheets (model, reasoning_effort, ...) are deliberately excluded. - parallel_branch_inert_attribute: fidelity/thread_id on parallel branch nodes and fork->branch edges. Branch dispatch bypasses the fidelity lifecycle, so these are dead letters today; the warning points at the parallel node, where fidelity does take effect. Also reconcile the loop_restart docs with actual executor behavior: taking a loop_restart edge restarts from the target with a fresh empty context (visit counts preserved), on success as well as failure; the transient_infra guard applies only to failure crossings. Co-Authored-By: Claude Fable 5 --- docs/public/examples/definition-of-done.mdx | 2 +- docs/public/execution/failures.mdx | 6 +- docs/public/reference/dot-language.mdx | 2 +- .../src/rules/inert_attribute.rs | 220 ++++++++++++++++ lib/crates/fabro-validate/src/rules/mod.rs | 4 + .../rules/parallel_branch_inert_attribute.rs | 235 ++++++++++++++++++ 6 files changed, 466 insertions(+), 3 deletions(-) create mode 100644 lib/crates/fabro-validate/src/rules/inert_attribute.rs create mode 100644 lib/crates/fabro-validate/src/rules/parallel_branch_inert_attribute.rs diff --git a/docs/public/examples/definition-of-done.mdx b/docs/public/examples/definition-of-done.mdx index 5b1ef9091..6291293b1 100644 --- a/docs/public/examples/definition-of-done.mdx +++ b/docs/public/examples/definition-of-done.mdx @@ -262,7 +262,7 @@ Otherwise set preferred_next_label to \"more_work_needed\"." **Three-category triage** — failures are classified as IMPLEMENTABLE (can fix now), STRUCTURAL (needs architecture work), or DEFERRED (needs external resources). This prevents the agent from wasting cycles on items it can't address in a code-only pass. -**Batched fixes** — the `fix_batch` node tackles up to 5 failures per iteration. The self-loop (`fix_batch -> fix_batch` with `loop_restart=true`) allows it to keep going when more fixes remain, while `goal_gate=true` ensures the workflow only succeeds if fixes were actually applied. +**Batched fixes** — the `fix_batch` node tackles up to 5 failures per iteration. The self-loop (`fix_batch -> fix_batch` with `loop_restart=true`) allows it to keep going when more fixes remain, while `goal_gate=true` ensures the workflow only succeeds if fixes were actually applied. Because `loop_restart` begins each round with a fresh, empty context, every iteration re-derives the remaining work from the repository state rather than from accumulated conversation history — see [Failures — Loop restart edges](/execution/failures#loop-restart-edges). **Build gate** — after each fix batch, a script node runs `cargo build` and `cargo test`. If the build breaks, a dedicated `build_fix` node diagnoses and repairs compilation errors before retrying. diff --git a/docs/public/execution/failures.mdx b/docs/public/execution/failures.mdx index 6884d5708..b76ae98e1 100644 --- a/docs/public/execution/failures.mdx +++ b/docs/public/execution/failures.mdx @@ -224,7 +224,11 @@ Failure signature counts are never reset on success. This is intentional — it ### Loop restart edges -Edges marked with `loop_restart=true` trigger a special restart of the workflow from the target node. These have an additional guard: only `transient_infra` failures may cross a `loop_restart` edge. If the failure class is anything else, the run is terminated: +Taking an edge marked with `loop_restart=true` restarts the workflow from the edge's target node. A restart is more than a jump: the completed-stage history, per-node outcomes, and retry counts are cleared, and the run context is replaced with a **fresh, empty context** — the target node starts over as if the run had just begun there, with no preamble of prior stages. Node visit counts are the one thing preserved, so `max_visits` and `max_node_visits` still bound how many times a restart loop can run. + +A **successful** outcome may take a `loop_restart` edge freely. This is the "start another round from a clean slate" pattern — for example, a self-loop that begins a fresh batch of work and re-derives its remaining work from the repository state rather than from accumulated context. + +A **failed** outcome faces an additional guard: only `transient_infra` failures may cross a `loop_restart` edge. If the failure class is anything else, the run is terminated: ``` loop_restart blocked: failure_class=deterministic (requires transient_infra) diff --git a/docs/public/reference/dot-language.mdx b/docs/public/reference/dot-language.mdx index eb162718a..f01fbf2ed 100644 --- a/docs/public/reference/dot-language.mdx +++ b/docs/public/reference/dot-language.mdx @@ -284,7 +284,7 @@ audit [ | `weight` | Integer | Priority for tiebreaking (higher wins, default: 0) | | `fidelity` | String | Override fidelity level for this transition | | `thread_id` | String | Override thread ID for this transition | -| `loop_restart` | Boolean | Mark this edge as a loop restart point | +| `loop_restart` | Boolean | Restart the workflow from this edge's target when taken: stage history and retry counts clear and the context resets to empty (visit counts are kept). Failed outcomes may only take it for `transient_infra` failures — see [Failures](/execution/failures#loop-restart-edges) | | `freeform` | Boolean | When `true` on a human-gate edge, accept free-text input instead of fixed choices | ## Condition expressions diff --git a/lib/crates/fabro-validate/src/rules/inert_attribute.rs b/lib/crates/fabro-validate/src/rules/inert_attribute.rs new file mode 100644 index 000000000..cb3d18035 --- /dev/null +++ b/lib/crates/fabro-validate/src/rules/inert_attribute.rs @@ -0,0 +1,220 @@ +use fabro_graphviz::graph::Graph; + +use crate::{Diagnostic, LintRule, Severity}; + +pub(super) fn rule() -> Box { + Box::new(Rule) +} + +/// Attributes that only specific handler types read, paired with the handler +/// types that consume them. On every other node type the attribute is inert: +/// accepted by the parser and read by nothing at runtime. +/// +/// Attributes read by several handlers (`timeout`), resolved for every node +/// (`fidelity`, `retry_policy`, `max_visits`, `goal_gate`), or injectable via +/// model stylesheets (`model`, `provider`, `reasoning_effort`, `speed`, +/// `backend`) are deliberately not listed. +const HANDLER_SPECIFIC_ATTRS: &[(&str, &[&str])] = &[ + ("script", &["command"]), + ("language", &["command"]), + ("duration", &["wait"]), + ("join_policy", &["parallel"]), + ("max_parallel", &["parallel"]), + ("output_schema", &["agent", "prompt"]), + ("prompt", &["agent", "prompt", "parallel.fan_in"]), +]; + +struct Rule; + +impl LintRule for Rule { + fn name(&self) -> &'static str { + "inert_attribute" + } + + fn apply(&self, graph: &Graph) -> Vec { + let mut diagnostics = Vec::new(); + for node in graph.nodes.values() { + // An unknown shape or type is covered by the type_known rule; a + // node this rule cannot classify is skipped rather than guessed at. + let Some(handler) = node.handler_type() else { + continue; + }; + for (attr, consumers) in HANDLER_SPECIFIC_ATTRS { + if !node.attrs.contains_key(*attr) { + continue; + } + if consumers.contains(&handler) { + continue; + } + diagnostics.push(Diagnostic { + rule: self.name().to_string(), + severity: Severity::Warning, + message: format!( + "Node '{}' (type '{handler}') sets '{attr}', which is only read by {} nodes and has no effect here", + node.id, + consumers.join(", "), + ), + node_id: Some(node.id.clone()), + edge: None, + fix: Some(format!( + "Remove '{attr}' or change the node to a type that reads it ({})", + consumers.join(", "), + )), + ..Diagnostic::default() + }); + } + } + diagnostics + } +} + +#[cfg(test)] +mod tests { + use fabro_graphviz::graph::{AttrValue, Node}; + + use super::Rule; + use crate::rules::test_support::minimal_graph; + use crate::{LintRule, Severity}; + + fn node_with_attr(id: &str, shape: &str, attr: &str, value: &str) -> Node { + let mut node = Node::new(id); + node.attrs + .insert("shape".to_string(), AttrValue::String(shape.to_string())); + node.attrs + .insert(attr.to_string(), AttrValue::String(value.to_string())); + node + } + + #[test] + fn warns_on_script_on_agent_node() { + let mut g = minimal_graph(); + g.nodes.insert( + "work".to_string(), + node_with_attr("work", "box", "script", "echo hi"), + ); + let d = Rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Warning); + assert!(d[0].message.contains("'script'")); + assert!(d[0].message.contains("command")); + assert_eq!(d[0].node_id.as_deref(), Some("work")); + } + + #[test] + fn warns_on_prompt_on_start_and_command_nodes() { + let mut g = minimal_graph(); + g.nodes + .get_mut("start") + .expect("minimal graph has start") + .attrs + .insert( + "prompt".to_string(), + AttrValue::String("do things".to_string()), + ); + g.nodes.insert( + "run".to_string(), + node_with_attr("run", "parallelogram", "prompt", "do things"), + ); + let d = Rule.apply(&g); + assert_eq!(d.len(), 2); + assert!(d.iter().all(|d| d.message.contains("'prompt'"))); + } + + #[test] + fn warns_on_duration_on_command_node() { + let mut g = minimal_graph(); + g.nodes.insert( + "run".to_string(), + node_with_attr("run", "parallelogram", "duration", "30s"), + ); + let d = Rule.apply(&g); + assert_eq!(d.len(), 1); + assert!(d[0].message.contains("'duration'")); + assert!(d[0].message.contains("wait")); + } + + #[test] + fn warns_on_parallel_attrs_on_agent_node() { + let mut g = minimal_graph(); + let mut node = Node::new("work"); + node.attrs.insert( + "join_policy".to_string(), + AttrValue::String("wait_all".to_string()), + ); + node.attrs + .insert("max_parallel".to_string(), AttrValue::Integer(4)); + g.nodes.insert("work".to_string(), node); + let d = Rule.apply(&g); + assert_eq!(d.len(), 2); + } + + #[test] + fn warns_on_output_schema_on_command_node() { + let mut g = minimal_graph(); + g.nodes.insert( + "run".to_string(), + node_with_attr("run", "parallelogram", "output_schema", "routing"), + ); + let d = Rule.apply(&g); + assert_eq!(d.len(), 1); + assert!(d[0].message.contains("'output_schema'")); + } + + #[test] + fn accepts_attrs_on_their_own_handler_types() { + let mut g = minimal_graph(); + g.nodes.insert( + "run".to_string(), + node_with_attr("run", "parallelogram", "script", "echo hi"), + ); + g.nodes.insert( + "pause".to_string(), + node_with_attr("pause", "insulator", "duration", "30s"), + ); + g.nodes.insert( + "work".to_string(), + node_with_attr("work", "box", "prompt", "do things"), + ); + g.nodes.insert( + "fork".to_string(), + node_with_attr("fork", "component", "join_policy", "wait_all"), + ); + g.nodes.insert( + "spec".to_string(), + node_with_attr("spec", "tab", "output_schema", "routing"), + ); + assert!(Rule.apply(&g).is_empty()); + } + + #[test] + fn accepts_prompt_on_shapeless_node_defaulting_to_agent() { + let mut g = minimal_graph(); + let mut node = Node::new("work"); + node.attrs.insert( + "prompt".to_string(), + AttrValue::String("do things".to_string()), + ); + g.nodes.insert("work".to_string(), node); + assert!(Rule.apply(&g).is_empty()); + } + + #[test] + fn accepts_prompt_on_fan_in_judge() { + let mut g = minimal_graph(); + g.nodes.insert( + "merge".to_string(), + node_with_attr("merge", "tripleoctagon", "prompt", "pick the best"), + ); + assert!(Rule.apply(&g).is_empty()); + } + + #[test] + fn ignores_unclassifiable_node_shapes() { + let mut g = minimal_graph(); + g.nodes.insert( + "odd".to_string(), + node_with_attr("odd", "doubleoctagon", "script", "echo hi"), + ); + assert!(Rule.apply(&g).is_empty()); + } +} diff --git a/lib/crates/fabro-validate/src/rules/mod.rs b/lib/crates/fabro-validate/src/rules/mod.rs index 98ece95af..428004240 100644 --- a/lib/crates/fabro-validate/src/rules/mod.rs +++ b/lib/crates/fabro-validate/src/rules/mod.rs @@ -8,9 +8,11 @@ mod fidelity_valid; mod freeform_edge_count; mod goal_gate_has_retry; mod import_error; +mod inert_attribute; mod model_support; mod node_model_known; mod orphan_custom_outcome; +mod parallel_branch_inert_attribute; mod prompt_on_llm_nodes; mod random_selection_no_conditions; mod reachability; @@ -60,6 +62,8 @@ pub fn built_in_rules() -> Vec> { thread_id_requires_fidelity_full::rule(), selection_valid::rule(), random_selection_no_conditions::rule(), + inert_attribute::rule(), + parallel_branch_inert_attribute::rule(), ] } diff --git a/lib/crates/fabro-validate/src/rules/parallel_branch_inert_attribute.rs b/lib/crates/fabro-validate/src/rules/parallel_branch_inert_attribute.rs new file mode 100644 index 000000000..40eff4211 --- /dev/null +++ b/lib/crates/fabro-validate/src/rules/parallel_branch_inert_attribute.rs @@ -0,0 +1,235 @@ +use std::collections::BTreeSet; + +use fabro_graphviz::graph::Graph; + +use crate::{Diagnostic, LintRule, Severity}; + +pub(super) fn rule() -> Box { + Box::new(Rule) +} + +/// Attributes that parallel branch execution does not resolve. Branch nodes +/// are dispatched with a snapshot of the context taken when the parallel node +/// started, so per-branch `fidelity` never changes what a branch sees, and +/// branches never join conversation threads regardless of `thread_id`. +const BRANCH_IGNORED_ATTRS: &[&str] = &["fidelity", "thread_id"]; + +struct Rule; + +fn fix_message(attr: &str, parallel_id: &str) -> String { + match attr { + "fidelity" => format!( + "Set fidelity on the parallel node '{parallel_id}' (or its incoming edge) to control what every branch sees" + ), + _ => format!("Remove '{attr}': parallel branches never join conversation threads"), + } +} + +impl LintRule for Rule { + fn name(&self) -> &'static str { + "parallel_branch_inert_attribute" + } + + fn apply(&self, graph: &Graph) -> Vec { + let parallel_ids: BTreeSet<&str> = graph + .nodes + .values() + .filter(|n| n.handler_type() == Some("parallel")) + .map(|n| n.id.as_str()) + .collect(); + if parallel_ids.is_empty() { + return Vec::new(); + } + + let mut diagnostics = Vec::new(); + + // Branch edges (parallel node -> branch target) carrying an attribute + // that branch dispatch never reads. + for edge in &graph.edges { + if !parallel_ids.contains(edge.from.as_str()) { + continue; + } + for attr in BRANCH_IGNORED_ATTRS { + if !edge.attrs.contains_key(*attr) { + continue; + } + diagnostics.push(Diagnostic { + rule: self.name().to_string(), + severity: Severity::Warning, + message: format!( + "Edge {} -> {} sets '{attr}', which is ignored on parallel branch edges: branches receive the context snapshot taken when '{}' started", + edge.from, edge.to, edge.from, + ), + node_id: None, + edge: Some((edge.from.clone(), edge.to.clone())), + fix: Some(fix_message(attr, &edge.from)), + ..Diagnostic::default() + }); + } + } + + // Branch target nodes carrying such an attribute — but only when every + // incoming edge comes from a parallel node. A node that is also + // reachable through a normal edge resolves the attribute on that path, + // so it is not inert there. + let branch_targets: BTreeSet<&str> = graph + .edges + .iter() + .filter(|e| parallel_ids.contains(e.from.as_str())) + .map(|e| e.to.as_str()) + .collect(); + for target in branch_targets { + let only_branch_entries = graph + .edges + .iter() + .filter(|e| e.to == target) + .all(|e| parallel_ids.contains(e.from.as_str())); + if !only_branch_entries { + continue; + } + let Some(node) = graph.nodes.get(target) else { + continue; + }; + let parallel_id = graph + .edges + .iter() + .find(|e| e.to == target && parallel_ids.contains(e.from.as_str())) + .map_or_else(String::new, |e| e.from.clone()); + for attr in BRANCH_IGNORED_ATTRS { + if !node.attrs.contains_key(*attr) { + continue; + } + diagnostics.push(Diagnostic { + rule: self.name().to_string(), + severity: Severity::Warning, + message: format!( + "Node '{}' sets '{attr}', but it only runs as a parallel branch (of '{parallel_id}'), where '{attr}' is ignored: branches receive the context snapshot taken when the parallel node started", + node.id, + ), + node_id: Some(node.id.clone()), + edge: None, + fix: Some(fix_message(attr, ¶llel_id)), + ..Diagnostic::default() + }); + } + } + + diagnostics + } +} + +#[cfg(test)] +mod tests { + use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; + + use super::Rule; + use crate::rules::test_support::minimal_graph; + use crate::{LintRule, Severity}; + + fn shaped_node(id: &str, shape: &str) -> Node { + let mut node = Node::new(id); + node.attrs + .insert("shape".to_string(), AttrValue::String(shape.to_string())); + node + } + + /// start -> fork -> {branch_a, branch_b} -> merge -> exit + fn parallel_graph() -> Graph { + let mut g = minimal_graph(); + g.nodes + .insert("fork".to_string(), shaped_node("fork", "component")); + g.nodes + .insert("branch_a".to_string(), shaped_node("branch_a", "tab")); + g.nodes + .insert("branch_b".to_string(), shaped_node("branch_b", "tab")); + g.nodes + .insert("merge".to_string(), shaped_node("merge", "tripleoctagon")); + g.edges = vec![ + Edge::new("start", "fork"), + Edge::new("fork", "branch_a"), + Edge::new("fork", "branch_b"), + Edge::new("branch_a", "merge"), + Edge::new("branch_b", "merge"), + Edge::new("merge", "exit"), + ]; + g + } + + #[test] + fn warns_on_fidelity_on_branch_node() { + let mut g = parallel_graph(); + g.nodes + .get_mut("branch_a") + .expect("graph has branch_a") + .attrs + .insert( + "fidelity".to_string(), + AttrValue::String("truncate".to_string()), + ); + let d = Rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!(d[0].severity, Severity::Warning); + assert_eq!(d[0].node_id.as_deref(), Some("branch_a")); + assert!(d[0].message.contains("'fidelity'")); + assert!(d[0].fix.as_deref().is_some_and(|f| f.contains("'fork'"))); + } + + #[test] + fn warns_on_thread_id_on_branch_edge() { + let mut g = parallel_graph(); + g.edges[1].attrs.insert( + "thread_id".to_string(), + AttrValue::String("impl".to_string()), + ); + let d = Rule.apply(&g); + assert_eq!(d.len(), 1); + assert_eq!( + d[0].edge, + Some(("fork".to_string(), "branch_a".to_string())) + ); + assert!(d[0].message.contains("'thread_id'")); + } + + #[test] + fn accepts_fidelity_on_the_parallel_node_itself() { + let mut g = parallel_graph(); + g.nodes + .get_mut("fork") + .expect("graph has fork") + .attrs + .insert( + "fidelity".to_string(), + AttrValue::String("truncate".to_string()), + ); + assert!(Rule.apply(&g).is_empty()); + } + + #[test] + fn accepts_fidelity_on_branch_node_also_reached_by_normal_edge() { + let mut g = parallel_graph(); + // branch_a is also a normal successor of merge, so fidelity resolves + // on that path and is not inert. + g.edges.push(Edge::new("merge", "branch_a")); + g.nodes + .get_mut("branch_a") + .expect("graph has branch_a") + .attrs + .insert( + "fidelity".to_string(), + AttrValue::String("truncate".to_string()), + ); + assert!(Rule.apply(&g).is_empty()); + } + + #[test] + fn accepts_graph_without_parallel_nodes() { + let mut g = minimal_graph(); + let mut node = shaped_node("work", "tab"); + node.attrs.insert( + "fidelity".to_string(), + AttrValue::String("truncate".to_string()), + ); + g.nodes.insert("work".to_string(), node); + assert!(Rule.apply(&g).is_empty()); + } +} From 3af4335098a9505079073ea8100853f29c006758 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Jul 2026 11:04:14 -0400 Subject: [PATCH 2/3] fix(validate): name every parallel parent in branch diagnostics A branch node reachable from multiple parallel nodes previously had its warning and fix hint name an arbitrary first parent. Collect all unique parallel parents (sorted) and render the full list in both. Addresses review feedback on #595. Co-Authored-By: Claude Fable 5 --- .../rules/parallel_branch_inert_attribute.rs | 65 ++++++++++++++++--- 1 file changed, 55 insertions(+), 10 deletions(-) diff --git a/lib/crates/fabro-validate/src/rules/parallel_branch_inert_attribute.rs b/lib/crates/fabro-validate/src/rules/parallel_branch_inert_attribute.rs index 40eff4211..6193f2700 100644 --- a/lib/crates/fabro-validate/src/rules/parallel_branch_inert_attribute.rs +++ b/lib/crates/fabro-validate/src/rules/parallel_branch_inert_attribute.rs @@ -16,11 +16,29 @@ const BRANCH_IGNORED_ATTRS: &[&str] = &["fidelity", "thread_id"]; struct Rule; -fn fix_message(attr: &str, parallel_id: &str) -> String { +/// Renders one or more parallel-node ids as `'a'` or `'a', 'b'`. +fn quoted_list(ids: &[String]) -> String { + ids.iter() + .map(|id| format!("'{id}'")) + .collect::>() + .join(", ") +} + +fn fix_message(attr: &str, parallel_ids: &[String]) -> String { match attr { - "fidelity" => format!( - "Set fidelity on the parallel node '{parallel_id}' (or its incoming edge) to control what every branch sees" - ), + "fidelity" => { + if parallel_ids.len() == 1 { + format!( + "Set fidelity on the parallel node {} (or its incoming edge) to control what every branch sees", + quoted_list(parallel_ids), + ) + } else { + format!( + "Set fidelity on the parallel nodes {} (or their incoming edges) to control what every branch sees", + quoted_list(parallel_ids), + ) + } + } _ => format!("Remove '{attr}': parallel branches never join conversation threads"), } } @@ -62,7 +80,7 @@ impl LintRule for Rule { ), node_id: None, edge: Some((edge.from.clone(), edge.to.clone())), - fix: Some(fix_message(attr, &edge.from)), + fix: Some(fix_message(attr, std::slice::from_ref(&edge.from))), ..Diagnostic::default() }); } @@ -90,11 +108,14 @@ impl LintRule for Rule { let Some(node) = graph.nodes.get(target) else { continue; }; - let parallel_id = graph + let parents: Vec = graph .edges .iter() - .find(|e| e.to == target && parallel_ids.contains(e.from.as_str())) - .map_or_else(String::new, |e| e.from.clone()); + .filter(|e| e.to == target && parallel_ids.contains(e.from.as_str())) + .map(|e| e.from.clone()) + .collect::>() + .into_iter() + .collect(); for attr in BRANCH_IGNORED_ATTRS { if !node.attrs.contains_key(*attr) { continue; @@ -103,12 +124,13 @@ impl LintRule for Rule { rule: self.name().to_string(), severity: Severity::Warning, message: format!( - "Node '{}' sets '{attr}', but it only runs as a parallel branch (of '{parallel_id}'), where '{attr}' is ignored: branches receive the context snapshot taken when the parallel node started", + "Node '{}' sets '{attr}', but it only runs as a parallel branch (of {}), where '{attr}' is ignored: branches receive the context snapshot taken when the parallel node started", node.id, + quoted_list(&parents), ), node_id: Some(node.id.clone()), edge: None, - fix: Some(fix_message(attr, ¶llel_id)), + fix: Some(fix_message(attr, &parents)), ..Diagnostic::default() }); } @@ -221,6 +243,29 @@ mod tests { assert!(Rule.apply(&g).is_empty()); } + #[test] + fn names_every_parallel_parent_of_a_shared_branch_node() { + let mut g = parallel_graph(); + g.nodes + .insert("fork2".to_string(), shaped_node("fork2", "component")); + g.edges.push(Edge::new("start", "fork2")); + g.edges.push(Edge::new("fork2", "branch_a")); + g.nodes + .get_mut("branch_a") + .expect("graph has branch_a") + .attrs + .insert( + "fidelity".to_string(), + AttrValue::String("truncate".to_string()), + ); + let d = Rule.apply(&g); + assert_eq!(d.len(), 1); + assert!(d[0].message.contains("'fork', 'fork2'")); + let fix = d[0].fix.as_deref().expect("diagnostic has a fix"); + assert!(fix.contains("'fork', 'fork2'")); + assert!(fix.contains("parallel nodes")); + } + #[test] fn accepts_graph_without_parallel_nodes() { let mut g = minimal_graph(); From f02ee6aaedec566e7d70473ac2ad08710b0e1e45 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Jul 2026 11:19:44 -0400 Subject: [PATCH 3/3] fix(validate): handle custom types and inherited threads --- .../src/rules/inert_attribute.rs | 21 ++++++++++++++++++- .../rules/parallel_branch_inert_attribute.rs | 13 ++++++++++-- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/lib/crates/fabro-validate/src/rules/inert_attribute.rs b/lib/crates/fabro-validate/src/rules/inert_attribute.rs index cb3d18035..fef9c5454 100644 --- a/lib/crates/fabro-validate/src/rules/inert_attribute.rs +++ b/lib/crates/fabro-validate/src/rules/inert_attribute.rs @@ -1,4 +1,4 @@ -use fabro_graphviz::graph::Graph; +use fabro_graphviz::graph::{self, Graph}; use crate::{Diagnostic, LintRule, Severity}; @@ -39,6 +39,9 @@ impl LintRule for Rule { let Some(handler) = node.handler_type() else { continue; }; + if !graph::is_known_handler_type(handler) { + continue; + } for (attr, consumers) in HANDLER_SPECIFIC_ATTRS { if !node.attrs.contains_key(*attr) { continue; @@ -217,4 +220,20 @@ mod tests { ); assert!(Rule.apply(&g).is_empty()); } + + #[test] + fn ignores_handler_specific_attrs_on_unrecognized_explicit_types() { + let mut g = minimal_graph(); + let mut node = Node::new("custom"); + node.attrs.insert( + "type".to_string(), + AttrValue::String("custom.handler".to_string()), + ); + node.attrs.insert( + "script".to_string(), + AttrValue::String("echo hi".to_string()), + ); + g.nodes.insert("custom".to_string(), node); + assert!(Rule.apply(&g).is_empty()); + } } diff --git a/lib/crates/fabro-validate/src/rules/parallel_branch_inert_attribute.rs b/lib/crates/fabro-validate/src/rules/parallel_branch_inert_attribute.rs index 6193f2700..eb051281e 100644 --- a/lib/crates/fabro-validate/src/rules/parallel_branch_inert_attribute.rs +++ b/lib/crates/fabro-validate/src/rules/parallel_branch_inert_attribute.rs @@ -11,7 +11,7 @@ pub(super) fn rule() -> Box { /// Attributes that parallel branch execution does not resolve. Branch nodes /// are dispatched with a snapshot of the context taken when the parallel node /// started, so per-branch `fidelity` never changes what a branch sees, and -/// branches never join conversation threads regardless of `thread_id`. +/// per-branch `thread_id` never replaces the thread inherited in that snapshot. const BRANCH_IGNORED_ATTRS: &[&str] = &["fidelity", "thread_id"]; struct Rule; @@ -39,7 +39,10 @@ fn fix_message(attr: &str, parallel_ids: &[String]) -> String { ) } } - _ => format!("Remove '{attr}': parallel branches never join conversation threads"), + "thread_id" => format!( + "Remove '{attr}': parallel branches inherit the thread resolved when the parallel node started" + ), + _ => format!("Remove '{attr}'"), } } @@ -210,6 +213,12 @@ mod tests { Some(("fork".to_string(), "branch_a".to_string())) ); assert!(d[0].message.contains("'thread_id'")); + assert_eq!( + d[0].fix.as_deref(), + Some( + "Remove 'thread_id': parallel branches inherit the thread resolved when the parallel node started" + ) + ); } #[test]