diff --git a/docs/public/changelog/2026-08-25.mdx b/docs/public/changelog/2026-08-25.mdx new file mode 100644 index 000000000..7a031fdee --- /dev/null +++ b/docs/public/changelog/2026-08-25.mdx @@ -0,0 +1,24 @@ +--- +title: "Stop workflows when a node fails" +date: "2026-08-25" +--- + +Workflows can now set graph-level `on_failure="exit"` to stop after a failed +node when no explicit recovery route matches. Fabro skips the unconditional +edge, checks configured retry targets, and ends the run as failed if no retry +target exists. + +The default `on_failure="route"` preserves existing workflow behavior. + +```dot +digraph Build { + graph [on_failure="exit"] + start [shape=Mdiamond] + exit [shape=Msquare] + plan [prompt="Plan the work"] + implement [prompt="Implement the plan"] + verify [prompt="Verify the implementation"] + + start -> plan -> implement -> verify -> exit +} +``` diff --git a/docs/public/docs.json b/docs/public/docs.json index fedc54eca..28938510c 100644 --- a/docs/public/docs.json +++ b/docs/public/docs.json @@ -309,6 +309,7 @@ "group": "August 2026", "icon": "clock-rotate-left", "pages": [ + "changelog/2026-08-25", "changelog/2026-08-23", "changelog/2026-08-21", "changelog/2026-08-20", diff --git a/docs/public/execution/failures.mdx b/docs/public/execution/failures.mdx index 9cdad1d1e..5c33d8028 100644 --- a/docs/public/execution/failures.mdx +++ b/docs/public/execution/failures.mdx @@ -40,6 +40,30 @@ approve -> manual_review [condition="outcome=failed"] If no `outcome=failed` edge or `retry_target` exists, the run stops rather than advancing past the approval gate. +## Stop linear workflows on failure + +By default, Fabro uses `on_failure="route"`. A failed node can take an unconditional edge when no explicit route matches. This compatibility default lets existing workflows decide how later nodes handle the failure. + +Set graph-level `on_failure="exit"` to stop a linear workflow at a failed node: + +```dot title="stop-on-failure.fabro" +digraph Build { + graph [on_failure="exit"] + + start [shape=Mdiamond] + exit [shape=Msquare] + plan [prompt="Plan the work"] + implement [prompt="Implement the plan"] + verify [prompt="Verify the implementation"] + + start -> plan -> implement -> verify -> exit +} +``` + +Fabro still uses an explicit recovery edge, such as `condition="outcome=failed"`, before it applies this policy. Matching preferred labels and suggested next node IDs also remain explicit routes. If no explicit edge matches, `exit` skips the unconditional edge and checks retry targets. The run ends as failed only when no retry target exists. + +The policy applies only to `failed`. Other outcomes keep their normal routing behavior. For parallel nodes, the policy uses the completed parallel node's final outcome. It does not stop or cancel individual branches early. + ## Retry layers Fabro retries failures at three levels: **LLM retries** handle transient API errors inside a single model call, **turn-level retries** recover from dropped streams mid-response, and **node retries** re-execute the entire node handler when the first two levels aren't enough. These layers are independent — a node retry re-runs the full handler, which gets its own fresh set of LLM and turn-level retries. @@ -304,9 +328,14 @@ A node failure does **not** automatically terminate the run. Fabro follows this 2. **Turn-level retries** — dropped streams retry the same agent turn (up to 3 retries), preserving conversation history 3. **Provider failover** — if configured, switch to a fallback provider 4. **Node retries** — re-execute the entire handler (per the retry policy) -4. **Edge routing** — if the node ultimately fails, look for an outgoing edge that matches (e.g., `condition="outcome=failed"`) -5. **Retry target** — if no matching edge exists, check `retry_target` / `fallback_retry_target` on the node and graph -6. **Run failure** — if none of the above produces a path forward, the run terminates +5. **Direct jump** — use `jump_to_node` when the outcome supplies one +6. **Explicit edge routing** — look for a matching condition, preferred label, or suggested next node +7. **Failure policy** — with `on_failure="exit"`, skip the unconditional edge; with `route` or no attribute, keep normal fallback routing +8. **Unconditional edge** — in `route` mode, use an edge without a condition as the fallback +9. **Retry target** — if no edge was selected, check `retry_target` and `fallback_retry_target` on the node, then on the graph +10. **Run failure** — if none of the above produces a path forward, the run terminates + +When a retry target sends the run back to a failing path, use graph-level `max_node_visits` or node-level `max_visits` to stop an unbounded cycle. The run also terminates immediately for: diff --git a/docs/public/reference/dot-language.mdx b/docs/public/reference/dot-language.mdx index d4194346d..ac04faeed 100644 --- a/docs/public/reference/dot-language.mdx +++ b/docs/public/reference/dot-language.mdx @@ -77,6 +77,7 @@ rankdir=LR | `rankdir` | Identifier | Layout direction: `LR` (left-to-right) or `TB` (top-to-bottom) | | `model_stylesheet` | String | CSS-like rules for model assignment (see [Model Stylesheets](/workflows/stylesheets)) | | `default_max_retries` | Integer | Default retry count for all nodes (default: 0) | +| `on_failure` | String | Failed-node routing policy: `route` (default) or `exit` | | `retry_target` | String | Default node ID to jump to on retry | | `fallback_retry_target` | String | Fallback retry target if primary target fails | | `default_fidelity` | String | Default [fidelity level](/execution/context) for all nodes | diff --git a/docs/public/workflows/transitions.mdx b/docs/public/workflows/transitions.mdx index 20744e9cf..411ae55d9 100644 --- a/docs/public/workflows/transitions.mdx +++ b/docs/public/workflows/transitions.mdx @@ -7,14 +7,50 @@ After each node finishes, Fabro must decide which edge to follow to the next nod ## How transitions work -When a node completes, it produces an **outcome** with a [stage outcome](/execution/outcomes) (`succeeded`, `failed`, `partially_succeeded`, or `skipped`) and optional signals like a preferred label or suggested next node. Fabro evaluates the outgoing edges in a fixed priority order: +When a node completes, it produces an **outcome** with a [stage outcome](/execution/outcomes) (`succeeded`, `failed`, `partially_succeeded`, or `skipped`) and optional signals like a preferred label or suggested next node. A node's retry policy runs before routing starts. Fabro then selects the next step in this order: -1. **Condition match** — Edges with a `condition` attribute are evaluated first. If one or more conditions match, the edge with the highest `weight` wins (lexical tiebreak on target node ID). -2. **Preferred label** — If the node's outcome includes a preferred label (e.g. from a human gate selection), the edge whose `label` matches is chosen. -3. **Suggested next** — If the node suggests a specific next node ID, the edge pointing to that node is chosen. -4. **Unconditional fallback** — Edges without conditions are considered last, again using `weight` then lexical tiebreak. +1. **Direct jump** — An outcome's `jump_to_node` value bypasses edge selection. +2. **Condition match** — Edges with a `condition` attribute are evaluated first. If one or more conditions match, the edge with the highest `weight` wins (lexical tiebreak on target node ID). +3. **Preferred label** — If the node's outcome includes a preferred label (for example, from a human gate selection), the edge whose `label` matches is chosen. +4. **Suggested next** — If the node suggests a specific next node ID, the edge pointing to that node is chosen. +5. **Failure policy** — For a failed outcome and graph-level `on_failure="exit"`, Fabro skips the unconditional fallback. +6. **Unconditional fallback** — Edges without conditions are considered last, again using `weight` then lexical tiebreak. +7. **Retry target** — For a failed outcome with no selected edge, Fabro checks node-level and graph-level `retry_target` and `fallback_retry_target` values. -If no edge matches at all, the workflow halts with an error. +If no edge or retry target supplies a next node, the workflow ends. A failed node produces a failed run outcome. + +## Failed-node routing policy + +The graph-level `on_failure` attribute controls whether a failed node can take an unconditional edge: + +- `route` keeps the existing routing behavior. It is the default. +- `exit` stops normal fallback routing after explicit routes fail to match. + +This lets a linear workflow stop at the first failed work node: + +```dot title="stop-on-failure.fabro" +digraph Build { + graph [on_failure="exit"] + + start [shape=Mdiamond] + exit [shape=Msquare] + plan [prompt="Plan the work"] + implement [prompt="Implement the plan"] + verify [prompt="Verify the implementation"] + + start -> plan -> implement -> verify -> exit +} +``` + +The `exit` policy only applies to the `failed` outcome. It does not change routing for `succeeded`, `partially_succeeded`, or `skipped` outcomes. + +Conditioned edges, matching preferred labels, and matching suggested node IDs are explicit recovery routes. They still take priority under `exit`. An unmatched preferred label or suggested node ID does not make an unconditional edge explicit. + +Retry targets also remain available. Fabro checks them after it skips the unconditional fallback. Use graph-level `max_node_visits` or node-level `max_visits` to bound workflows whose retry targets return to a failing path. + +When `exit` stops routing, Fabro checkpoints the failed node without a next node and ends the run as failed. It does not execute the graph's exit node or emit an edge selection for an edge it did not take. An explicit recovery route can still reach the exit node normally. + +For a parallel node, `exit` sees the final outcome returned by the parallel handler. It can stop routing for a failed parallel outcome. It does not add branch-level fail-fast behavior, and a `partially_succeeded` parallel outcome continues normally. ## Edge attributes @@ -132,7 +168,7 @@ The `[A]`, `[R]`, `[S]` prefixes are keyboard accelerators — Fabro strips them ## Unconditional edges -An edge without a `condition` attribute always matches. When a node has a single outgoing edge, it doesn't need a condition: +An edge without a `condition` attribute is the normal fallback. When a node has a single outgoing edge, it doesn't need a condition: ```dot start -> plan -> implement -> exit @@ -145,6 +181,8 @@ gate -> fast_path [condition="outcome=succeeded"] gate -> slow_path ``` +For a failed outcome, `on_failure="exit"` skips this fallback after explicit routes are checked. The default `on_failure="route"` keeps the behavior shown above. + ## Weight tiebreaking When multiple edges match (e.g. two unconditional edges), `weight` determines the winner. Higher weight wins: diff --git a/lib/apps/fabro-cli/tests/it/cmd/validate.rs b/lib/apps/fabro-cli/tests/it/cmd/validate.rs index dd469c4a5..1f67ebe3f 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/validate.rs @@ -358,3 +358,21 @@ fn invalid() { × Validation failed "); } + +#[test] +fn invalid_on_failure_is_a_validation_failure() { + let context = test_context!(); + let mut cmd = context.validate(); + cmd.arg(fixture("on_failure_invalid.fabro")); + fabro_snapshot!(context.filters(), cmd, @" + success: false + exit_code: 1 + ----- stdout ----- + ----- stderr ----- + Workflow: InvalidOnFailure (2 nodes, 1 edges) + Graph: [FIXTURES]/on_failure_invalid.fabro + error: Graph has invalid on_failure value 'stop' (on_failure_valid) + fix: Use one of: route, exit + × Validation failed + "); +} diff --git a/lib/components/fabro-validate/src/rules/mod.rs b/lib/components/fabro-validate/src/rules/mod.rs index f9e31edfc..2d0366ab7 100644 --- a/lib/components/fabro-validate/src/rules/mod.rs +++ b/lib/components/fabro-validate/src/rules/mod.rs @@ -14,6 +14,7 @@ mod inert_attribute; mod join_policy_removed; mod model_support; mod node_model_known; +mod on_failure_valid; mod orphan_custom_outcome; mod parallel_branch; mod parallel_branch_inert_attribute; @@ -63,6 +64,7 @@ pub fn built_in_rules() -> Vec> { reserved_keyword_node_id::rule(), all_conditional_edges::rule(), orphan_custom_outcome::rule(), + on_failure_valid::rule(), script_absolute_cd::rule(), command_requires_script::rule(), import_error::rule(), diff --git a/lib/components/fabro-validate/src/rules/on_failure_valid.rs b/lib/components/fabro-validate/src/rules/on_failure_valid.rs new file mode 100644 index 000000000..8c84a0b72 --- /dev/null +++ b/lib/components/fabro-validate/src/rules/on_failure_valid.rs @@ -0,0 +1,176 @@ +use fabro_graphviz::graph::{AttrValue, Graph}; +use fabro_types::OnFailure; + +use crate::{Diagnostic, LintRule, Severity}; + +pub(super) fn rule() -> Box { + Box::new(Rule) +} + +struct Rule; + +impl LintRule for Rule { + fn name(&self) -> &'static str { + "on_failure_valid" + } + + fn apply(&self, graph: &Graph) -> Vec { + let mut diagnostics = Vec::new(); + let expected_values = OnFailure::expected_values(); + + match graph.attrs.get("on_failure") { + None => {} + Some(AttrValue::String(value)) if value.parse::().is_ok() => {} + Some(AttrValue::String(value)) => diagnostics.push(Diagnostic { + rule: self.name().to_string(), + severity: Severity::Error, + message: format!("Graph has invalid on_failure value '{value}'"), + fix: Some(format!("Use one of: {expected_values}")), + ..Diagnostic::default() + }), + Some(_) => diagnostics.push(Diagnostic { + rule: self.name().to_string(), + severity: Severity::Error, + message: "Graph attribute 'on_failure' must be a string".to_string(), + fix: Some(format!("Use one of: {expected_values}")), + ..Diagnostic::default() + }), + } + + for node in graph.nodes.values() { + if node.attrs.contains_key("on_failure") { + diagnostics.push(Diagnostic { + rule: self.name().to_string(), + severity: Severity::Warning, + message: format!( + "Node '{}' sets 'on_failure', which has no effect outside graph scope", + node.id + ), + node_id: Some(node.id.clone()), + fix: Some("Move 'on_failure' to the graph attributes".to_string()), + ..Diagnostic::default() + }); + } + } + + for edge in &graph.edges { + if edge.attrs.contains_key("on_failure") { + diagnostics.push(Diagnostic { + rule: self.name().to_string(), + severity: Severity::Warning, + message: format!( + "Edge '{} -> {}' sets 'on_failure', which has no effect outside graph scope", + edge.from, edge.to + ), + edge: Some((edge.from.clone(), edge.to.clone())), + fix: Some("Move 'on_failure' to the graph attributes".to_string()), + ..Diagnostic::default() + }); + } + } + + diagnostics + } +} + +#[cfg(test)] +mod tests { + use fabro_graphviz::graph::{AttrValue, Edge, Node}; + + use super::Rule; + use crate::rules::test_support::minimal_graph; + use crate::{LintRule, Severity}; + + #[test] + fn accepts_absent_and_supported_graph_values() { + let mut graph = minimal_graph(); + assert!(Rule.apply(&graph).is_empty()); + + for value in ["route", "exit"] { + graph.attrs.insert( + "on_failure".to_string(), + AttrValue::String(value.to_string()), + ); + assert!(Rule.apply(&graph).is_empty()); + } + } + + #[test] + fn rejects_unsupported_graph_value() { + let mut graph = minimal_graph(); + graph.attrs.insert( + "on_failure".to_string(), + AttrValue::String("stop".to_string()), + ); + + let diagnostics = Rule.apply(&graph); + + assert_eq!(diagnostics.len(), 1); + assert_eq!(diagnostics[0].severity, Severity::Error); + assert_eq!( + diagnostics[0].message, + "Graph has invalid on_failure value 'stop'" + ); + assert_eq!( + diagnostics[0].fix.as_deref(), + Some("Use one of: route, exit") + ); + } + + #[test] + fn rejects_non_string_graph_value() { + let mut graph = minimal_graph(); + graph + .attrs + .insert("on_failure".to_string(), AttrValue::Boolean(true)); + + let diagnostics = Rule.apply(&graph); + + assert_eq!(diagnostics.len(), 1); + assert_eq!(diagnostics[0].severity, Severity::Error); + assert_eq!( + diagnostics[0].message, + "Graph attribute 'on_failure' must be a string" + ); + } + + #[test] + fn warns_for_node_and_edge_placement() { + let mut graph = minimal_graph(); + let mut node = Node::new("work"); + node.attrs.insert( + "on_failure".to_string(), + AttrValue::String("exit".to_string()), + ); + graph.nodes.insert("work".to_string(), node); + + let mut edge = Edge::new("start", "work"); + edge.attrs.insert( + "on_failure".to_string(), + AttrValue::String("exit".to_string()), + ); + graph.edges.push(edge); + + let diagnostics = Rule.apply(&graph); + + assert_eq!(diagnostics.len(), 2); + assert!( + diagnostics + .iter() + .all(|diagnostic| diagnostic.severity == Severity::Warning) + ); + assert!( + diagnostics + .iter() + .all(|diagnostic| diagnostic.message.contains("graph scope")) + ); + assert!( + diagnostics + .iter() + .any(|diagnostic| diagnostic.node_id.as_deref() == Some("work")) + ); + assert!(diagnostics.iter().any(|diagnostic| { + diagnostic.edge == Some(("start".to_string(), "work".to_string())) + })); + } +} diff --git a/lib/components/fabro-workflow/src/graph.rs b/lib/components/fabro-workflow/src/graph.rs index c38491028..50df7bb13 100644 --- a/lib/components/fabro-workflow/src/graph.rs +++ b/lib/components/fabro-workflow/src/graph.rs @@ -6,6 +6,7 @@ use std::sync::Arc; use fabro_core::error::{Error as CoreError, Result as CoreResult}; use fabro_core::graph::{EdgeSelection as CoreEdgeSelection, EdgeSpec, Graph, NodeSpec}; use fabro_graphviz::graph::types::{Edge as GvEdge, Graph as GvGraph, Node as GvNode}; +use fabro_types::OnFailure; use crate::context::Context; use crate::outcome::{BilledModelUsage, Outcome}; @@ -129,4 +130,8 @@ impl Graph for WorkflowGraph { fn get_retry_target(&self, failed_node_id: &str) -> Option { routing::get_retry_target(failed_node_id, self.inner()) } + + fn on_failure(&self) -> OnFailure { + self.inner().on_failure() + } } diff --git a/lib/components/fabro-workflow/src/graph/routing.rs b/lib/components/fabro-workflow/src/graph/routing.rs index cb294aa6d..925e0b5d3 100644 --- a/lib/components/fabro-workflow/src/graph/routing.rs +++ b/lib/components/fabro-workflow/src/graph/routing.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use fabro_graphviz::graph::types::{Edge as GvEdge, Graph as GvGraph, Node as GvNode}; +use fabro_types::OnFailure; use rand::Rng; use crate::condition::evaluate_condition; @@ -74,6 +75,10 @@ pub(crate) fn select_edge<'a>( } } + if outcome.status.is_failure() && graph.on_failure() == OnFailure::Exit { + return None; + } + if blocks_unconditional_failure_fallthrough(node, outcome) { return None; } @@ -252,6 +257,13 @@ mod tests { g } + fn set_on_failure(graph: &mut Graph, on_failure: OnFailure) { + graph.attrs.insert( + "on_failure".to_string(), + AttrValue::String(on_failure.to_string()), + ); + } + #[test] fn normalize_label_lowercase_and_trim() { assert_eq!(normalize_label(" Yes "), "yes"); @@ -378,6 +390,140 @@ mod tests { assert_eq!(sel.reason, "unconditional"); } + #[test] + fn failed_outcome_takes_unconditional_edge_in_default_and_route_modes() { + for policy in [None, Some(OnFailure::Route)] { + let mut graph = make_graph_with_edges(vec![Edge::new("a", "b")]); + if let Some(policy) = policy { + set_on_failure(&mut graph, policy); + } + let node = graph.nodes.get("a").unwrap(); + let outcome = Outcome::fail_classify("boom"); + + let selected = + select_edge(node, &outcome, &Context::new(), &graph, "deterministic").unwrap(); + + assert_eq!(selected.edge.to, "b"); + assert_eq!(selected.reason, "unconditional"); + } + } + + #[test] + fn exit_policy_blocks_unconditional_edge_for_failed_outcome() { + let mut graph = make_graph_with_edges(vec![Edge::new("a", "b")]); + set_on_failure(&mut graph, OnFailure::Exit); + let node = graph.nodes.get("a").unwrap(); + let outcome = Outcome::fail_classify("boom"); + + assert!(select_edge(node, &outcome, &Context::new(), &graph, "deterministic").is_none()); + } + + #[test] + fn exit_policy_allows_unconditional_edge_for_non_failed_outcomes() { + let mut graph = make_graph_with_edges(vec![Edge::new("a", "b")]); + set_on_failure(&mut graph, OnFailure::Exit); + let node = graph.nodes.get("a").unwrap(); + let mut partial = Outcome::success(); + partial.status = StageOutcome::PartiallySucceeded; + + for outcome in [Outcome::success(), partial, Outcome::skipped("not needed")] { + let selected = + select_edge(node, &outcome, &Context::new(), &graph, "deterministic").unwrap(); + assert_eq!(selected.edge.to, "b"); + assert_eq!(selected.reason, "unconditional"); + } + } + + #[test] + fn exit_policy_allows_matching_failure_condition() { + let mut recovery = Edge::new("a", "recover"); + recovery.attrs.insert( + "condition".to_string(), + AttrValue::String("outcome=failed".to_string()), + ); + let mut graph = make_graph_with_edges(vec![recovery, Edge::new("a", "fallback")]); + set_on_failure(&mut graph, OnFailure::Exit); + let node = graph.nodes.get("a").unwrap(); + let outcome = Outcome::fail_classify("boom"); + + let selected = + select_edge(node, &outcome, &Context::new(), &graph, "deterministic").unwrap(); + + assert_eq!(selected.edge.to, "recover"); + assert_eq!(selected.reason, "condition"); + } + + #[test] + fn exit_policy_allows_matching_preferred_and_suggested_routes() { + let mut preferred = Edge::new("a", "preferred"); + preferred.attrs.insert( + "label".to_string(), + AttrValue::String("Recover".to_string()), + ); + let mut graph = make_graph_with_edges(vec![preferred, Edge::new("a", "suggested")]); + set_on_failure(&mut graph, OnFailure::Exit); + let node = graph.nodes.get("a").unwrap(); + + let mut preferred_outcome = Outcome::fail_classify("boom"); + preferred_outcome.preferred_label = Some("Recover".to_string()); + let selected = select_edge( + node, + &preferred_outcome, + &Context::new(), + &graph, + "deterministic", + ) + .unwrap(); + assert_eq!(selected.edge.to, "preferred"); + assert_eq!(selected.reason, "preferred_label"); + + let mut suggested_outcome = Outcome::fail_classify("boom"); + suggested_outcome.suggested_next_ids = vec!["suggested".to_string()]; + let selected = select_edge( + node, + &suggested_outcome, + &Context::new(), + &graph, + "deterministic", + ) + .unwrap(); + assert_eq!(selected.edge.to, "suggested"); + assert_eq!(selected.reason, "suggested_next"); + } + + #[test] + fn exit_policy_blocks_fallback_for_unmatched_routing_hints() { + let mut graph = make_graph_with_edges(vec![Edge::new("a", "fallback")]); + set_on_failure(&mut graph, OnFailure::Exit); + let node = graph.nodes.get("a").unwrap(); + + let mut preferred_outcome = Outcome::fail_classify("boom"); + preferred_outcome.preferred_label = Some("missing".to_string()); + assert!( + select_edge( + node, + &preferred_outcome, + &Context::new(), + &graph, + "deterministic" + ) + .is_none() + ); + + let mut suggested_outcome = Outcome::fail_classify("boom"); + suggested_outcome.suggested_next_ids = vec!["missing".to_string()]; + assert!( + select_edge( + node, + &suggested_outcome, + &Context::new(), + &graph, + "deterministic" + ) + .is_none() + ); + } + #[test] fn select_edge_condition_match() { let mut e1 = Edge::new("a", "fail_path"); @@ -514,21 +660,24 @@ mod tests { #[test] fn select_edge_failed_human_gate_does_not_fall_through_to_unconditional() { - let g = make_graph_with_edges(vec![ - Edge::new("gate", "approve"), - Edge::new("gate", "skip"), - ]); - let mut node = g.nodes.get("gate").unwrap().clone(); - node.attrs.insert( - "shape".to_string(), - AttrValue::String("hexagon".to_string()), - ); - let outcome = Outcome::fail_deterministic( - "human interaction interrupted before an answer was provided", - ); - let context = Context::new(); + for policy in [OnFailure::Route, OnFailure::Exit] { + let mut graph = make_graph_with_edges(vec![ + Edge::new("gate", "approve"), + Edge::new("gate", "skip"), + ]); + set_on_failure(&mut graph, policy); + let mut node = graph.nodes.get("gate").unwrap().clone(); + node.attrs.insert( + "shape".to_string(), + AttrValue::String("hexagon".to_string()), + ); + let outcome = Outcome::fail_deterministic( + "human interaction interrupted before an answer was provided", + ); + let context = Context::new(); - assert!(select_edge(&node, &outcome, &context, &g, "deterministic").is_none()); + assert!(select_edge(&node, &outcome, &context, &graph, "deterministic").is_none()); + } } #[test] @@ -539,20 +688,23 @@ mod tests { AttrValue::String("outcome=failed".to_string()), ); let approve = Edge::new("gate", "approve"); - let g = make_graph_with_edges(vec![fail, approve]); - let mut node = g.nodes.get("gate").unwrap().clone(); - node.attrs.insert( - "shape".to_string(), - AttrValue::String("hexagon".to_string()), - ); - let outcome = Outcome::fail_deterministic( - "human interaction interrupted before an answer was provided", - ); - let context = Context::new(); + for policy in [OnFailure::Route, OnFailure::Exit] { + let mut graph = make_graph_with_edges(vec![fail.clone(), approve.clone()]); + set_on_failure(&mut graph, policy); + let mut node = graph.nodes.get("gate").unwrap().clone(); + node.attrs.insert( + "shape".to_string(), + AttrValue::String("hexagon".to_string()), + ); + let outcome = Outcome::fail_deterministic( + "human interaction interrupted before an answer was provided", + ); + let context = Context::new(); - let sel = select_edge(&node, &outcome, &context, &g, "deterministic").unwrap(); - assert_eq!(sel.edge.to, "retry"); - assert_eq!(sel.reason, "condition"); + let sel = select_edge(&node, &outcome, &context, &graph, "deterministic").unwrap(); + assert_eq!(sel.edge.to, "retry"); + assert_eq!(sel.reason, "condition"); + } } #[test] diff --git a/lib/components/fabro-workflow/tests/it/integration.rs b/lib/components/fabro-workflow/tests/it/integration.rs index 2aee85909..1fdd577df 100644 --- a/lib/components/fabro-workflow/tests/it/integration.rs +++ b/lib/components/fabro-workflow/tests/it/integration.rs @@ -1127,6 +1127,210 @@ impl Handler for AlwaysFailHandler { } } +struct OnFailureRecordingHandler { + visits: Arc>>, +} + +#[async_trait::async_trait] +impl Handler for OnFailureRecordingHandler { + async fn execute( + &self, + node: &Node, + _context: &fabro_workflow::context::Context, + _graph: &Graph, + _run_dir: &Path, + _services: &fabro_workflow::handler::EngineServices, + ) -> Result { + self.visits.lock().unwrap().push(node.id.clone()); + if node.id == "work" { + Ok(Outcome::fail_classify("forced work failure")) + } else { + Ok(Outcome::success()) + } + } +} + +fn on_failure_graph(policy: Option<&str>) -> Graph { + let mut graph = Graph::new("OnFailureTest"); + if let Some(policy) = policy { + graph.attrs.insert( + "on_failure".to_string(), + AttrValue::String(policy.to_string()), + ); + } + + let mut start = Node::new("start"); + start.attrs.insert( + "shape".to_string(), + AttrValue::String("Mdiamond".to_string()), + ); + graph.nodes.insert("start".to_string(), start); + + let mut exit = Node::new("exit"); + exit.attrs.insert( + "shape".to_string(), + AttrValue::String("Msquare".to_string()), + ); + graph.nodes.insert("exit".to_string(), exit); + + for node_id in ["work", "downstream", "recovery"] { + graph.nodes.insert(node_id.to_string(), Node::new(node_id)); + } + graph.edges.push(Edge::new("start", "work")); + graph +} + +fn on_failure_registry(visits: Arc>>) -> HandlerRegistry { + let mut registry = HandlerRegistry::new(Box::new(OnFailureRecordingHandler { visits })); + registry.register("start", Box::new(StartHandler)); + registry.register("exit", Box::new(ExitHandler)); + registry +} + +#[tokio::test] +async fn on_failure_exit_stops_linear_workflow_and_records_failed_lifecycle() { + let mut graph = on_failure_graph(Some("exit")); + graph.edges.push(Edge::new("work", "downstream")); + graph.edges.push(Edge::new("downstream", "exit")); + + let visits = Arc::new(std::sync::Mutex::new(Vec::new())); + let emitter = Emitter::default(); + let events = collect_events(&emitter); + let engine = WorkflowRunner::new( + on_failure_registry(Arc::clone(&visits)), + Arc::new(emitter), + local_env(), + ); + let dir = tempfile::tempdir().unwrap(); + let (outcome, state) = engine + .run_with_state(&graph, &make_run_options(dir.path())) + .await + .expect("policy termination should return a failed workflow outcome"); + + assert_eq!(outcome.status, StageOutcome::Failed { + retry_requested: false, + }); + assert_eq!( + outcome.failure_reason(), + Some("stage work failed and graph on_failure=exit stopped routing") + ); + assert_eq!(*visits.lock().unwrap(), vec!["work"]); + + let checkpoint = state + .current_checkpoint() + .expect("failed work should be checkpointed"); + assert_eq!(checkpoint.current_node, "work"); + assert_eq!(checkpoint.next_node_id, None); + assert!(!checkpoint.node_outcomes.contains_key("downstream")); + + let events = events.lock().unwrap(); + assert!( + events + .iter() + .any(|event| matches!(&event.body, EventBody::RunFailed(_))) + ); + assert!(events.iter().any(|event| { + matches!( + &event.body, + EventBody::CheckpointCompleted(properties) + if properties.current_node == "work" && properties.next_node_id.is_none() + ) + })); + assert!(!events.iter().any(|event| { + matches!( + &event.body, + EventBody::EdgeSelected(properties) if properties.from_node == "work" + ) + })); +} + +#[tokio::test] +async fn on_failure_route_and_absent_policy_preserve_unconditional_fallback() { + for policy in [None, Some("route")] { + let mut graph = on_failure_graph(policy); + graph.edges.push(Edge::new("work", "downstream")); + graph.edges.push(Edge::new("downstream", "exit")); + + let visits = Arc::new(std::sync::Mutex::new(Vec::new())); + let engine = WorkflowRunner::new( + on_failure_registry(Arc::clone(&visits)), + Arc::new(Emitter::default()), + local_env(), + ); + let dir = tempfile::tempdir().unwrap(); + let (outcome, state) = engine + .run_with_state(&graph, &make_run_options(dir.path())) + .await + .expect("route policy should preserve unconditional fallback"); + + assert_eq!(outcome.status, StageOutcome::Succeeded); + assert_eq!(*visits.lock().unwrap(), vec!["work", "downstream"]); + assert!( + state + .current_checkpoint() + .expect("downstream should be checkpointed") + .node_outcomes + .contains_key("downstream") + ); + } +} + +#[tokio::test] +async fn on_failure_exit_allows_explicit_failure_recovery_edge() { + let mut graph = on_failure_graph(Some("exit")); + graph.edges.push(Edge::new("work", "downstream")); + let mut recovery_edge = Edge::new("work", "recovery"); + recovery_edge.attrs.insert( + "condition".to_string(), + AttrValue::String("outcome=failed".to_string()), + ); + graph.edges.push(recovery_edge); + graph.edges.push(Edge::new("recovery", "exit")); + graph.edges.push(Edge::new("downstream", "exit")); + + let visits = Arc::new(std::sync::Mutex::new(Vec::new())); + let engine = WorkflowRunner::new( + on_failure_registry(Arc::clone(&visits)), + Arc::new(Emitter::default()), + local_env(), + ); + let dir = tempfile::tempdir().unwrap(); + let (outcome, _) = engine + .run_with_state(&graph, &make_run_options(dir.path())) + .await + .expect("explicit failure recovery should complete"); + + assert_eq!(outcome.status, StageOutcome::Succeeded); + assert_eq!(*visits.lock().unwrap(), vec!["work", "recovery"]); +} + +#[tokio::test] +async fn on_failure_exit_uses_retry_target_instead_of_unconditional_edge() { + let mut graph = on_failure_graph(Some("exit")); + graph.nodes.get_mut("work").unwrap().attrs.insert( + "retry_target".to_string(), + AttrValue::String("recovery".to_string()), + ); + graph.edges.push(Edge::new("work", "downstream")); + graph.edges.push(Edge::new("recovery", "exit")); + graph.edges.push(Edge::new("downstream", "exit")); + + let visits = Arc::new(std::sync::Mutex::new(Vec::new())); + let engine = WorkflowRunner::new( + on_failure_registry(Arc::clone(&visits)), + Arc::new(Emitter::default()), + local_env(), + ); + let dir = tempfile::tempdir().unwrap(); + let (outcome, _) = engine + .run_with_state(&graph, &make_run_options(dir.path())) + .await + .expect("retry target should run before policy termination"); + + assert_eq!(outcome.status, StageOutcome::Succeeded); + assert_eq!(*visits.lock().unwrap(), vec!["work", "recovery"]); +} + #[tokio::test] async fn goal_gate_routes_to_retry_target_on_failure() { // Pipeline: diff --git a/lib/foundation/fabro-core/src/executor.rs b/lib/foundation/fabro-core/src/executor.rs index 6701e30be..9aba44156 100644 --- a/lib/foundation/fabro-core/src/executor.rs +++ b/lib/foundation/fabro-core/src/executor.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use std::sync::atomic::Ordering; use std::time::Instant; +use fabro_types::OnFailure; use tokio::time::sleep; use tokio_util::sync::CancellationToken; @@ -282,10 +283,16 @@ impl Executor { NextStep::End => { let mut outcome = last_outcome.clone(); if outcome.status.is_failure() { - outcome = Outcome::fail(&format!( - "stage {} failed with no outgoing fail edge", - node.id() - )); + let message = match graph.on_failure() { + OnFailure::Route => { + format!("stage {} failed with no outgoing fail edge", node.id()) + } + OnFailure::Exit => format!( + "stage {} failed and graph on_failure=exit stopped routing", + node.id() + ), + }; + outcome = Outcome::fail(&message); } self.lifecycle.on_run_end(&outcome, &state).await; return Ok((outcome, state)); @@ -2180,6 +2187,113 @@ mod tests { assert_eq!(handler.calls(), 2); } + #[tokio::test] + async fn executor_exit_policy_ends_failed_run_with_policy_message_and_no_next_node() { + #[derive(Default)] + struct ExitPolicyLog { + checkpoints: Vec<(String, Option)>, + run_end: Option, + } + + struct ExitPolicyLifecycle(Arc>); + + #[async_trait] + impl RunLifecycle for ExitPolicyLifecycle { + async fn on_checkpoint( + &self, + node: &TestNode, + _result: &NodeResult, + next_node_id: Option<&str>, + _state: &ExecutionState, + ) -> Result<()> { + self.0 + .lock() + .unwrap() + .checkpoints + .push((node.id().to_string(), next_node_id.map(ToOwned::to_owned))); + Ok(()) + } + + async fn on_run_end(&self, outcome: &Outcome, _state: &ExecutionState) { + self.0.lock().unwrap().run_end = Some(outcome.clone()); + } + } + + let graph = TestGraph::new( + vec![ + TestNode::new("work"), + TestNode::new("downstream"), + TestNode::terminal("end"), + ], + vec![ + TestEdge::new("work", "downstream"), + TestEdge::new("downstream", "end"), + ], + "work", + ) + .with_on_failure(OnFailure::Exit); + let state = ExecutionState::new(&graph).unwrap(); + let log = Arc::new(Mutex::new(ExitPolicyLog::default())); + let executor = ExecutorBuilder::new( + Arc::new(AlwaysFailHandler::new("boom")) as Arc> + ) + .lifecycle(Box::new(ExitPolicyLifecycle(Arc::clone(&log)))) + .build(); + + let (outcome, state) = executor.run(&graph, state).await.unwrap(); + + assert_eq!(outcome.status, StageOutcome::Failed { + retry_requested: false, + }); + assert_eq!( + outcome + .failure + .as_ref() + .map(|failure| failure.message.as_str()), + Some("stage work failed and graph on_failure=exit stopped routing") + ); + assert!(state.node_outcomes.contains_key("work")); + assert!(!state.node_outcomes.contains_key("downstream")); + + let log = log.lock().unwrap(); + assert_eq!(log.checkpoints, vec![("work".to_string(), None)]); + assert_eq!(log.run_end.as_ref(), Some(&outcome)); + } + + #[tokio::test] + async fn executor_exit_policy_uses_retry_target_before_termination() { + let handler = Arc::new(CountingHandler::new(vec![ + Ok(Outcome::fail("boom")), + Ok(Outcome::success()), + ])); + let graph = TestGraph::new( + vec![ + TestNode::new("work"), + TestNode::new("downstream"), + TestNode::new("recovery"), + TestNode::terminal("end"), + ], + vec![ + TestEdge::new("work", "downstream"), + TestEdge::new("downstream", "end"), + TestEdge::new("recovery", "end"), + ], + "work", + ) + .with_retry_target("work", "recovery") + .with_on_failure(OnFailure::Exit); + let state = ExecutionState::new(&graph).unwrap(); + let executor = + ExecutorBuilder::new(Arc::clone(&handler) as Arc>).build(); + + let (outcome, state) = executor.run(&graph, state).await.unwrap(); + + assert_eq!(outcome.status, StageOutcome::Succeeded); + assert_eq!(handler.calls(), 2); + assert!(state.node_outcomes.contains_key("recovery")); + assert!(!state.node_outcomes.contains_key("downstream")); + } + #[tokio::test] async fn executor_goal_gate_retry_target_to_terminal_fails_without_looping() { let terminal_visits = Arc::new(AtomicU32::new(0)); diff --git a/lib/foundation/fabro-core/src/graph.rs b/lib/foundation/fabro-core/src/graph.rs index 304e266a8..c327e86f2 100644 --- a/lib/foundation/fabro-core/src/graph.rs +++ b/lib/foundation/fabro-core/src/graph.rs @@ -1,5 +1,7 @@ use std::collections::HashMap; +use fabro_types::OnFailure; + use crate::context::Context; use crate::error::Result; use crate::outcome::{Outcome, OutcomeMeta}; @@ -40,4 +42,5 @@ pub trait Graph: Send + Sync { outcomes: &HashMap>, ) -> std::result::Result<(), String>; fn get_retry_target(&self, failed_node_id: &str) -> Option; + fn on_failure(&self) -> OnFailure; } diff --git a/lib/foundation/fabro-core/src/test_fixtures.rs b/lib/foundation/fabro-core/src/test_fixtures.rs index 8ccefe4a7..56e82cc59 100644 --- a/lib/foundation/fabro-core/src/test_fixtures.rs +++ b/lib/foundation/fabro-core/src/test_fixtures.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicU32, Ordering}; use async_trait::async_trait; +use fabro_types::OnFailure; use crate::context::Context; use crate::error::{Error, HandlerErrorDetail, Result}; @@ -122,6 +123,7 @@ pub struct TestGraph { pub edges: Vec, pub start_node_id: String, pub retry_targets: HashMap, + pub on_failure: OnFailure, } impl TestGraph { @@ -131,6 +133,7 @@ impl TestGraph { edges, start_node_id: start.to_string(), retry_targets: HashMap::new(), + on_failure: OnFailure::Route, } } @@ -139,6 +142,12 @@ impl TestGraph { self.retry_targets.insert(from.to_string(), to.to_string()); self } + + #[must_use] + pub fn with_on_failure(mut self, on_failure: OnFailure) -> Self { + self.on_failure = on_failure; + self + } } impl Graph for TestGraph { @@ -208,6 +217,10 @@ impl Graph for TestGraph { } } + if outcome.status.is_failure() && self.on_failure == OnFailure::Exit { + return None; + } + // Fourth: unconditional (no label) if let Some(e) = edges.iter().find(|e| e.label.is_none()) { return Some(EdgeSelection { @@ -243,6 +256,10 @@ impl Graph for TestGraph { fn get_retry_target(&self, failed_node_id: &str) -> Option { self.retry_targets.get(failed_node_id).cloned() } + + fn on_failure(&self) -> OnFailure { + self.on_failure + } } // ---- Test handlers ---- diff --git a/lib/foundation/fabro-types/src/graph.rs b/lib/foundation/fabro-types/src/graph.rs index ded69954d..e411c4020 100644 --- a/lib/foundation/fabro-types/src/graph.rs +++ b/lib/foundation/fabro-types/src/graph.rs @@ -2,9 +2,40 @@ use std::collections::HashMap; use std::time::Duration; use serde::{Deserialize, Serialize}; +use strum::VariantNames; use crate::AgentBackend; +/// Policy for routing a failed node when no explicit recovery edge matches. +#[derive( + Debug, + Clone, + Copy, + Default, + PartialEq, + Eq, + Serialize, + Deserialize, + strum::Display, + strum::EnumString, + strum::IntoStaticStr, + strum::VariantNames, +)] +#[serde(rename_all = "snake_case")] +#[strum(serialize_all = "snake_case")] +pub enum OnFailure { + #[default] + Route, + Exit, +} + +impl OnFailure { + #[must_use] + pub fn expected_values() -> String { + ::VARIANTS.join(", ") + } +} + /// Typed attribute values for nodes, edges, and graph-level attributes. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub enum AttrValue { @@ -538,6 +569,18 @@ impl Graph { .and_then(AttrValue::as_str) } + /// Graph-level failure routing policy. Invalid values are rejected during + /// workflow validation, so runtime resolution can use the compatibility + /// default. + #[must_use] + pub fn on_failure(&self) -> OnFailure { + self.attrs + .get("on_failure") + .and_then(AttrValue::as_str) + .and_then(|value| value.parse().ok()) + .unwrap_or_default() + } + /// Graph-level `default_fidelity`. pub fn default_fidelity(&self) -> Option<&str> { self.attrs @@ -666,6 +709,33 @@ pub fn reference_kind_for_attribute( mod tests { use super::*; + #[test] + fn on_failure_parses_and_displays_supported_values() { + assert_eq!("route".parse::().unwrap(), OnFailure::Route); + assert_eq!("exit".parse::().unwrap(), OnFailure::Exit); + assert_eq!(OnFailure::Route.to_string(), "route"); + assert_eq!(OnFailure::Exit.to_string(), "exit"); + assert_eq!(OnFailure::expected_values(), "route, exit"); + } + + #[test] + fn graph_on_failure_defaults_to_route_and_resolves_explicit_values() { + let mut graph = Graph::new("test"); + assert_eq!(graph.on_failure(), OnFailure::Route); + + graph.attrs.insert( + "on_failure".to_string(), + AttrValue::String("route".to_string()), + ); + assert_eq!(graph.on_failure(), OnFailure::Route); + + graph.attrs.insert( + "on_failure".to_string(), + AttrValue::String("exit".to_string()), + ); + assert_eq!(graph.on_failure(), OnFailure::Exit); + } + #[test] fn attr_value_as_str() { let val = AttrValue::String("hello".to_string()); diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index 1ac3ddae5..ca1e2804d 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -78,8 +78,8 @@ pub use event_envelope::EventEnvelope; pub use fabro_model::ReasoningEffort; pub use failure_signature::FailureSignature; pub use graph::{ - AttrValue, ContextKeyAttr, Edge, Graph, KNOWN_HANDLER_TYPES, Node, is_known_handler_type, - is_llm_handler_type, shape_to_handler_type, + AttrValue, ContextKeyAttr, Edge, Graph, KNOWN_HANDLER_TYPES, Node, OnFailure, + is_known_handler_type, is_llm_handler_type, shape_to_handler_type, }; pub use input_scalar::{JsonScalarToTomlError, json_scalar_to_toml_value}; pub use interview::{ diff --git a/test/on_failure_invalid.fabro b/test/on_failure_invalid.fabro new file mode 100644 index 000000000..97da02290 --- /dev/null +++ b/test/on_failure_invalid.fabro @@ -0,0 +1,8 @@ +digraph InvalidOnFailure { + graph [on_failure="stop"] + + start [shape=Mdiamond] + exit [shape=Msquare] + + start -> exit +}