Merge pull request #595 from fabro-sh/validate-inert-attributes

feat(validate): warn on inert node and edge attributes

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.
 

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)

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

lib/crates/fabro-validate/src/rules/inert_attribute.rs

@@ -0,0 +1,239 @@
+use fabro_graphviz::graph::{self, Graph};
+
+use crate::{Diagnostic, LintRule, Severity};
+
+pub(super) fn rule() -> Box<dyn LintRule> {
+    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<Diagnostic> {
+        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;
+            };
+            if !graph::is_known_handler_type(handler) {
+                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());
+    }
+
+    #[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());
+    }
+}

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<Box<dyn LintRule>> {
         thread_id_requires_fidelity_full::rule(),
         selection_valid::rule(),
         random_selection_no_conditions::rule(),
+        inert_attribute::rule(),
+        parallel_branch_inert_attribute::rule(),
     ]
 }
 

lib/crates/fabro-validate/src/rules/parallel_branch_inert_attribute.rs

@@ -0,0 +1,289 @@
+use std::collections::BTreeSet;
+
+use fabro_graphviz::graph::Graph;
+
+use crate::{Diagnostic, LintRule, Severity};
+
+pub(super) fn rule() -> Box<dyn LintRule> {
+    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
+/// per-branch `thread_id` never replaces the thread inherited in that snapshot.
+const BRANCH_IGNORED_ATTRS: &[&str] = &["fidelity", "thread_id"];
+
+struct Rule;
+
+/// 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::<Vec<_>>()
+        .join(", ")
+}
+
+fn fix_message(attr: &str, parallel_ids: &[String]) -> String {
+    match attr {
+        "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),
+                )
+            }
+        }
+        "thread_id" => format!(
+            "Remove '{attr}': parallel branches inherit the thread resolved when the parallel node started"
+        ),
+        _ => format!("Remove '{attr}'"),
+    }
+}
+
+impl LintRule for Rule {
+    fn name(&self) -> &'static str {
+        "parallel_branch_inert_attribute"
+    }
+
+    fn apply(&self, graph: &Graph) -> Vec<Diagnostic> {
+        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, std::slice::from_ref(&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 parents: Vec<String> = graph
+                .edges
+                .iter()
+                .filter(|e| e.to == target && parallel_ids.contains(e.from.as_str()))
+                .map(|e| e.from.clone())
+                .collect::<BTreeSet<_>>()
+                .into_iter()
+                .collect();
+            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 {}), 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, &parents)),
+                    ..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'"));
+        assert_eq!(
+            d[0].fix.as_deref(),
+            Some(
+                "Remove 'thread_id': parallel branches inherit the thread resolved when the parallel node started"
+            )
+        );
+    }
+
+    #[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 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();
+        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());
+    }
+}