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..fef9c5454 --- /dev/null +++ b/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 { + 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; + }; + 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()); + } +} 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..eb051281e --- /dev/null +++ b/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 { + 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::>() + .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 { + 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 = graph + .edges + .iter() + .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; + } + 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()); + } +}