diff --git a/docs/public/changelog/2026-08-25.mdx b/docs/public/changelog/2026-08-25.mdx index 23c085465..344d129b8 100644 --- a/docs/public/changelog/2026-08-25.mdx +++ b/docs/public/changelog/2026-08-25.mdx @@ -10,6 +10,11 @@ target exists. The default `on_failure="route"` preserves existing workflow behavior. +A node can also set its own `on_failure` to override the graph policy in +either direction: a best-effort node can use `on_failure="route"` inside an +`exit` graph, or a single critical node can use `on_failure="exit"` while the +rest of the graph keeps the default. + ```dot digraph Build { graph [on_failure="exit"] diff --git a/docs/public/execution/failures.mdx b/docs/public/execution/failures.mdx index 5c33d8028..73b81bddb 100644 --- a/docs/public/execution/failures.mdx +++ b/docs/public/execution/failures.mdx @@ -62,6 +62,8 @@ digraph Build { 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. +Set `on_failure` on a node to control that node alone. The node-level attribute overrides the graph level, in both directions: a node can opt out of a graph-level `exit` with `on_failure="route"`, or stop the run on its own failure with `on_failure="exit"` while the rest of the graph keeps the default. A node without the attribute inherits the graph policy. See [Failed-node routing policy](/workflows/transitions#failed-node-routing-policy). + 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 @@ -330,7 +332,7 @@ A node failure does **not** automatically terminate the run. Fabro follows this 4. **Node retries** — re-execute the entire handler (per the retry policy) 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 +7. **Failure policy** — with an effective `on_failure="exit"` (node-level `on_failure` first, then graph-level), 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 diff --git a/docs/public/reference/dot-language.mdx b/docs/public/reference/dot-language.mdx index 3642fa001..3f42716c2 100644 --- a/docs/public/reference/dot-language.mdx +++ b/docs/public/reference/dot-language.mdx @@ -199,6 +199,7 @@ Other node types still need their shape, because their attributes don't identify | `class` | String | Classes for [stylesheet](/workflows/stylesheets) targeting. Separate multiple classes with spaces. Commas are also accepted for compatibility. | | `timeout` | Duration | Execution timeout (e.g. `900s`). An agent's wait for human input does not consume this budget. On a human node, this is the response deadline. | | `max_visits` | Integer | Max times this node can execute in a run. Overrides the graph-level `max_node_visits` for this node. | +| `on_failure` | String | Failed-node routing policy for this node: `route` or `exit`. Overrides the graph-level `on_failure`. | | `max_retries` | Integer | Override default retry count | | `retry_policy` | String | Named preset: `none`, `standard`, `aggressive`, `linear`, `patient` | | `retry_target` | String | Node ID to jump to on retry | diff --git a/docs/public/workflows/transitions.mdx b/docs/public/workflows/transitions.mdx index 411ae55d9..325ae7ec8 100644 --- a/docs/public/workflows/transitions.mdx +++ b/docs/public/workflows/transitions.mdx @@ -13,7 +13,7 @@ When a node completes, it produces an **outcome** with a [stage outcome](/execut 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. +5. **Failure policy** — For a failed outcome with an effective `on_failure="exit"` policy (node-level `on_failure` first, then graph-level), 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. @@ -21,11 +21,13 @@ If no edge or retry target supplies a next node, the workflow ends. A failed nod ## Failed-node routing policy -The graph-level `on_failure` attribute controls whether a failed node can take an unconditional edge: +The `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. +Set it at the graph level to apply the policy to every node, or on a node to control that node alone. A node-level `on_failure` overrides the graph level. A node without the attribute inherits the graph policy. + This lets a linear workflow stop at the first failed work node: ```dot title="stop-on-failure.fabro" @@ -42,12 +44,30 @@ digraph Build { } ``` +Node-level overrides work in both directions. A strict graph can mark one best-effort node as `route` so its failure continues down the unconditional edge, and a default graph can mark one critical node as `exit`: + +```dot title="mixed-policies.fabro" +digraph Build { + graph [on_failure="exit"] + + start [shape=Mdiamond] + exit [shape=Msquare] + implement [prompt="Implement the change"] + lint [prompt="Run optional lint cleanup" on_failure="route"] + verify [prompt="Verify the implementation"] + + start -> implement -> lint -> 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. +A failed human gate never falls through to an unconditional edge, regardless of policy. Node-level `on_failure="route"` does not change that; route an interrupted gate explicitly with `condition="outcome=failed"`. + 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. diff --git a/lib/apps/fabro-cli/tests/it/cmd/validate.rs b/lib/apps/fabro-cli/tests/it/cmd/validate.rs index 17fe31ac4..dad1ac0eb 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/validate.rs @@ -377,6 +377,24 @@ fn invalid() { "); } +#[test] +fn invalid_node_on_failure_is_a_validation_failure() { + let context = test_context!(); + let mut cmd = context.validate(); + cmd.arg(fixture("on_failure_node_invalid.fabro")); + fabro_snapshot!(context.filters(), cmd, @" + success: false + exit_code: 1 + ----- stdout ----- + ----- stderr ----- + Workflow: InvalidNodeOnFailure (3 nodes, 2 edges) + Graph: [FIXTURES]/on_failure_node_invalid.fabro + error [node: work]: Node 'work' has invalid on_failure value 'stop' (on_failure_valid) + fix: Use one of: route, exit + × Validation failed + "); +} + #[test] fn invalid_on_failure_is_a_validation_failure() { let context = test_context!(); diff --git a/lib/components/fabro-validate/src/rules/on_failure_valid.rs b/lib/components/fabro-validate/src/rules/on_failure_valid.rs index 683d7bb75..8f0d87987 100644 --- a/lib/components/fabro-validate/src/rules/on_failure_valid.rs +++ b/lib/components/fabro-validate/src/rules/on_failure_valid.rs @@ -9,6 +9,35 @@ pub(super) fn rule() -> Box { struct Rule; +fn invalid_value_diagnostic( + rule: &str, + node_id: Option<&str>, + value: &AttrValue, +) -> Option { + let message = match value { + AttrValue::String(value) if value.parse::().is_ok() => return None, + AttrValue::String(value) => match node_id { + Some(node_id) => format!("Node '{node_id}' has invalid on_failure value '{value}'"), + None => format!("Graph has invalid on_failure value '{value}'"), + }, + _ => match node_id { + Some(node_id) => { + format!("Node '{node_id}' attribute 'on_failure' must be a string") + } + None => "Graph attribute 'on_failure' must be a string".to_string(), + }, + }; + + Some(Diagnostic { + rule: rule.to_string(), + severity: Severity::Error, + message, + node_id: node_id.map(str::to_string), + fix: Some(format!("Use one of: {}", OnFailure::expected_values())), + ..Diagnostic::default() + }) +} + impl LintRule for Rule { fn name(&self) -> &'static str { "on_failure_valid" @@ -17,48 +46,36 @@ impl LintRule for Rule { fn apply(&self, graph: &Graph) -> Vec { let mut diagnostics = Vec::new(); - let invalid_value_message = match graph.attrs.get("on_failure") { - None => None, - Some(AttrValue::String(value)) if value.parse::().is_ok() => None, - Some(AttrValue::String(value)) => { - Some(format!("Graph has invalid on_failure value '{value}'")) - } - Some(_) => Some("Graph attribute 'on_failure' must be a string".to_string()), - }; - if let Some(message) = invalid_value_message { - diagnostics.push(Diagnostic { - rule: self.name().to_string(), - severity: Severity::Error, - message, - fix: Some(format!("Use one of: {}", OnFailure::expected_values())), - ..Diagnostic::default() - }); + if let Some(value) = graph.attrs.get("on_failure") { + diagnostics.extend(invalid_value_diagnostic(self.name(), None, value)); } - let misplaced = |subject: String| Diagnostic { - rule: self.name().to_string(), - severity: Severity::Warning, - message: format!( - "{subject} sets 'on_failure', which has no effect outside graph scope" - ), - fix: Some("Move 'on_failure' to the graph attributes".to_string()), - ..Diagnostic::default() - }; - - for node in graph.nodes.values() { - if node.attrs.contains_key("on_failure") { - diagnostics.push(Diagnostic { - node_id: Some(node.id.clone()), - ..misplaced(format!("Node '{}'", node.id)) - }); - } + let mut node_values: Vec<_> = graph + .nodes + .iter() + .filter_map(|(node_id, node)| { + node.attrs + .get("on_failure") + .map(|value| (node_id.as_str(), value)) + }) + .collect(); + node_values.sort_unstable_by_key(|(node_id, _)| *node_id); + for (node_id, value) in node_values { + diagnostics.extend(invalid_value_diagnostic(self.name(), Some(node_id), value)); } 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 on edges", + edge.from, edge.to + ), + fix: Some("Set 'on_failure' on the graph or the source node".to_string()), edge: Some((edge.from.clone(), edge.to.clone())), - ..misplaced(format!("Edge '{} -> {}'", edge.from, edge.to)) + ..Diagnostic::default() }); } } @@ -89,6 +106,18 @@ mod tests { } } + #[test] + fn accepts_supported_node_values() { + let mut graph = minimal_graph(); + for value in ["route", "exit"] { + graph.nodes.insert( + "work".to_string(), + node_with_attrs("work", &[("on_failure", value)]), + ); + assert!(Rule.apply(&graph).is_empty()); + } + } + #[test] fn rejects_unsupported_graph_value() { let mut graph = minimal_graph(); @@ -129,13 +158,49 @@ mod tests { } #[test] - fn warns_for_node_and_edge_placement() { + fn rejects_unsupported_node_value() { let mut graph = minimal_graph(); graph.nodes.insert( "work".to_string(), - node_with_attrs("work", &[("on_failure", "exit")]), + node_with_attrs("work", &[("on_failure", "stop")]), ); + let diagnostics = Rule.apply(&graph); + + assert_eq!(diagnostics.len(), 1); + assert_eq!(diagnostics[0].severity, Severity::Error); + assert_eq!( + diagnostics[0].message, + "Node 'work' has invalid on_failure value 'stop'" + ); + assert_eq!(diagnostics[0].node_id.as_deref(), Some("work")); + assert_eq!( + diagnostics[0].fix.as_deref(), + Some("Use one of: route, exit") + ); + } + + #[test] + fn rejects_non_string_node_value() { + let mut graph = minimal_graph(); + let mut node = node_with_attrs("work", &[]); + node.attrs + .insert("on_failure".to_string(), AttrValue::Boolean(true)); + graph.nodes.insert("work".to_string(), node); + + let diagnostics = Rule.apply(&graph); + + assert_eq!(diagnostics.len(), 1); + assert_eq!(diagnostics[0].severity, Severity::Error); + assert_eq!( + diagnostics[0].message, + "Node 'work' attribute 'on_failure' must be a string" + ); + } + + #[test] + fn warns_for_edge_placement() { + let mut graph = minimal_graph(); let mut edge = Edge::new("start", "work"); edge.attrs.insert( "on_failure".to_string(), @@ -145,24 +210,19 @@ mod tests { let diagnostics = Rule.apply(&graph); - assert_eq!(diagnostics.len(), 2); - assert!( - diagnostics - .iter() - .all(|diagnostic| diagnostic.severity == Severity::Warning) + assert_eq!(diagnostics.len(), 1); + assert_eq!(diagnostics[0].severity, Severity::Warning); + assert_eq!( + diagnostics[0].message, + "Edge 'start -> work' sets 'on_failure', which has no effect on edges" ); - assert!( - diagnostics - .iter() - .all(|diagnostic| diagnostic.message.contains("graph scope")) + assert_eq!( + diagnostics[0].fix.as_deref(), + Some("Set 'on_failure' on the graph or the source node") ); - assert!( - diagnostics - .iter() - .any(|diagnostic| diagnostic.node_id.as_deref() == Some("work")) + assert_eq!( + diagnostics[0].edge, + Some(("start".to_string(), "work".to_string())) ); - 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 50df7bb13..1381828aa 100644 --- a/lib/components/fabro-workflow/src/graph.rs +++ b/lib/components/fabro-workflow/src/graph.rs @@ -6,7 +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 fabro_types::ResolvedOnFailure; use crate::context::Context; use crate::outcome::{BilledModelUsage, Outcome}; @@ -131,7 +131,7 @@ impl Graph for WorkflowGraph { routing::get_retry_target(failed_node_id, self.inner()) } - fn on_failure(&self) -> OnFailure { - self.inner().on_failure() + fn resolve_on_failure(&self, node: &Self::Node) -> ResolvedOnFailure { + self.inner().resolve_on_failure(node.inner()) } } diff --git a/lib/components/fabro-workflow/src/graph/routing.rs b/lib/components/fabro-workflow/src/graph/routing.rs index 925e0b5d3..0de75ab97 100644 --- a/lib/components/fabro-workflow/src/graph/routing.rs +++ b/lib/components/fabro-workflow/src/graph/routing.rs @@ -75,7 +75,7 @@ pub(crate) fn select_edge<'a>( } } - if outcome.status.is_failure() && graph.on_failure() == OnFailure::Exit { + if outcome.status.is_failure() && graph.resolve_on_failure(node).policy() == OnFailure::Exit { return None; } @@ -264,6 +264,13 @@ mod tests { ); } + fn set_node_on_failure(graph: &mut Graph, node_id: &str, value: &str) { + graph.nodes.get_mut(node_id).unwrap().attrs.insert( + "on_failure".to_string(), + AttrValue::String(value.to_string()), + ); + } + #[test] fn normalize_label_lowercase_and_trim() { assert_eq!(normalize_label(" Yes "), "yes"); @@ -524,6 +531,84 @@ mod tests { ); } + #[test] + fn node_exit_overrides_graph_route_and_blocks_unconditional_edge() { + for graph_policy in [None, Some(OnFailure::Route)] { + let mut graph = make_graph_with_edges(vec![Edge::new("a", "b")]); + if let Some(policy) = graph_policy { + set_on_failure(&mut graph, policy); + } + set_node_on_failure(&mut graph, "a", "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 node_route_overrides_graph_exit_and_takes_unconditional_edge() { + let mut graph = make_graph_with_edges(vec![Edge::new("a", "b")]); + set_on_failure(&mut graph, OnFailure::Exit); + set_node_on_failure(&mut graph, "a", "route"); + 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 node_policy_does_not_affect_routing_from_other_nodes() { + let mut graph = make_graph_with_edges(vec![Edge::new("a", "b")]); + set_node_on_failure(&mut graph, "b", "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, "b"); + assert_eq!(selected.reason, "unconditional"); + } + + #[test] + fn invalid_node_on_failure_value_inherits_graph_policy() { + let mut graph = make_graph_with_edges(vec![Edge::new("a", "b")]); + set_on_failure(&mut graph, OnFailure::Exit); + set_node_on_failure(&mut graph, "a", "bogus"); + 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 failed_human_gate_ignores_node_route_override() { + let mut graph = make_graph_with_edges(vec![ + Edge::new("gate", "approve"), + Edge::new("gate", "skip"), + ]); + set_on_failure(&mut graph, OnFailure::Exit); + set_node_on_failure(&mut graph, "gate", "route"); + let gate = graph.nodes.get_mut("gate").unwrap(); + gate.attrs.insert( + "shape".to_string(), + AttrValue::String("hexagon".to_string()), + ); + let node = graph.nodes.get("gate").unwrap().clone(); + let outcome = Outcome::fail_deterministic( + "human interaction interrupted before an answer was provided", + ); + + assert!(select_edge(&node, &outcome, &Context::new(), &graph, "deterministic").is_none()); + } + #[test] fn select_edge_condition_match() { let mut e1 = Edge::new("a", "fail_path"); diff --git a/lib/components/fabro-workflow/tests/it/integration.rs b/lib/components/fabro-workflow/tests/it/integration.rs index e3481d055..4c6ac37b3 100644 --- a/lib/components/fabro-workflow/tests/it/integration.rs +++ b/lib/components/fabro-workflow/tests/it/integration.rs @@ -1292,6 +1292,33 @@ async fn on_failure_exit_uses_retry_target_instead_of_unconditional_edge() { assert_eq!(*run.visits.lock().unwrap(), vec!["work", "recovery"]); } +#[tokio::test] +async fn node_on_failure_exit_overrides_graph_route_policy() { + let graph = on_failure_graph(r#"work [on_failure="exit"]"#); + let run = run_on_failure(&graph, Emitter::default()).await; + + assert_eq!(run.outcome.status, StageOutcome::Failed { + retry_requested: false, + }); + assert_eq!( + run.outcome.failure_reason(), + Some("stage work failed and node on_failure=exit stopped routing") + ); + assert_eq!(*run.visits.lock().unwrap(), vec!["work"]); +} + +#[tokio::test] +async fn node_on_failure_route_overrides_graph_exit_policy() { + let graph = on_failure_graph( + r#"graph [on_failure="exit"] + work [on_failure="route"]"#, + ); + let run = run_on_failure(&graph, Emitter::default()).await; + + assert_eq!(run.outcome.status, StageOutcome::Succeeded); + assert_eq!(*run.visits.lock().unwrap(), vec!["work", "downstream"]); +} + #[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 d1d8a784a..7f976584b 100644 --- a/lib/foundation/fabro-core/src/executor.rs +++ b/lib/foundation/fabro-core/src/executor.rs @@ -283,13 +283,15 @@ impl Executor { NextStep::End => { let mut outcome = last_outcome.clone(); if outcome.status.is_failure() { - let message = match graph.on_failure() { + let resolved = graph.resolve_on_failure(&node); + let message = match resolved.policy() { 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() + "stage {} failed and {} on_failure=exit stopped routing", + node.id(), + resolved.scope() ), }; outcome = Outcome::fail(&message); @@ -2260,6 +2262,68 @@ mod tests { assert_eq!(log.run_end.as_ref(), Some(&outcome)); } + #[tokio::test] + async fn executor_node_exit_policy_overrides_graph_route_and_names_node_scope() { + let graph = TestGraph::new( + vec![ + TestNode::new("work").with_on_failure(OnFailure::Exit), + TestNode::new("downstream"), + TestNode::terminal("end"), + ], + vec![ + TestEdge::new("work", "downstream"), + TestEdge::new("downstream", "end"), + ], + "work", + ); + let state = ExecutionState::new(&graph).unwrap(); + let executor = ExecutorBuilder::new( + Arc::new(AlwaysFailHandler::new("boom")) as Arc> + ) + .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 node on_failure=exit stopped routing") + ); + assert!(!state.node_outcomes.contains_key("downstream")); + } + + #[tokio::test] + async fn executor_node_route_policy_overrides_graph_exit() { + let graph = TestGraph::new( + vec![ + TestNode::new("work").with_on_failure(OnFailure::Route), + 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 handler = DispatchHandler::new(Arc::new(AlwaysSucceedHandler)) + .with_handler("work", Arc::new(AlwaysFailHandler::new("boom"))); + let executor = + ExecutorBuilder::new(Arc::new(handler) as Arc>).build(); + + let (outcome, state) = executor.run(&graph, state).await.unwrap(); + + assert_eq!(outcome.status, StageOutcome::Succeeded); + 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 c327e86f2..e39ccd593 100644 --- a/lib/foundation/fabro-core/src/graph.rs +++ b/lib/foundation/fabro-core/src/graph.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; -use fabro_types::OnFailure; +use fabro_types::ResolvedOnFailure; use crate::context::Context; use crate::error::Result; @@ -42,5 +42,7 @@ 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; + /// Effective failure routing policy for a node: node-level `on_failure` + /// overrides the graph level, and an absent node attribute inherits it. + fn resolve_on_failure(&self, node: &Self::Node) -> ResolvedOnFailure; } diff --git a/lib/foundation/fabro-core/src/test_fixtures.rs b/lib/foundation/fabro-core/src/test_fixtures.rs index 56e82cc59..1a503deb3 100644 --- a/lib/foundation/fabro-core/src/test_fixtures.rs +++ b/lib/foundation/fabro-core/src/test_fixtures.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicU32, Ordering}; use async_trait::async_trait; -use fabro_types::OnFailure; +use fabro_types::{OnFailure, ResolvedOnFailure}; use crate::context::Context; use crate::error::{Error, HandlerErrorDetail, Result}; @@ -20,6 +20,7 @@ pub struct TestNode { pub terminal: bool, pub max_visits: Option, pub goal_gate: Option<(String, StageOutcome)>, + pub on_failure: Option, } impl TestNode { @@ -29,6 +30,7 @@ impl TestNode { terminal: false, max_visits: None, goal_gate: None, + on_failure: None, } } @@ -38,6 +40,7 @@ impl TestNode { terminal: true, max_visits: None, goal_gate: None, + on_failure: None, } } @@ -52,6 +55,12 @@ impl TestNode { self.goal_gate = Some((node_id.to_string(), required_status)); self } + + #[must_use] + pub fn with_on_failure(mut self, on_failure: OnFailure) -> Self { + self.on_failure = Some(on_failure); + self + } } impl NodeSpec for TestNode { @@ -217,7 +226,8 @@ impl Graph for TestGraph { } } - if outcome.status.is_failure() && self.on_failure == OnFailure::Exit { + if outcome.status.is_failure() && self.resolve_on_failure(node).policy() == OnFailure::Exit + { return None; } @@ -257,8 +267,11 @@ impl Graph for TestGraph { self.retry_targets.get(failed_node_id).cloned() } - fn on_failure(&self) -> OnFailure { - self.on_failure + fn resolve_on_failure(&self, node: &Self::Node) -> ResolvedOnFailure { + match node.on_failure { + Some(policy) => ResolvedOnFailure::node(policy), + None => ResolvedOnFailure::graph(self.on_failure), + } } } diff --git a/lib/foundation/fabro-types/src/graph.rs b/lib/foundation/fabro-types/src/graph.rs index e411c4020..4b4f18233 100644 --- a/lib/foundation/fabro-types/src/graph.rs +++ b/lib/foundation/fabro-types/src/graph.rs @@ -36,6 +36,42 @@ impl OnFailure { } } +/// A failure routing policy together with the scope that supplied it, so +/// failure messages can name the attribute that stopped routing. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ResolvedOnFailure { + policy: OnFailure, + scope: AttributeScope, +} + +impl ResolvedOnFailure { + #[must_use] + pub const fn node(policy: OnFailure) -> Self { + Self { + policy, + scope: AttributeScope::Node, + } + } + + #[must_use] + pub const fn graph(policy: OnFailure) -> Self { + Self { + policy, + scope: AttributeScope::Graph, + } + } + + #[must_use] + pub const fn policy(self) -> OnFailure { + self.policy + } + + #[must_use] + pub const fn scope(self) -> AttributeScope { + self.scope + } +} + /// Typed attribute values for nodes, edges, and graph-level attributes. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub enum AttrValue { @@ -313,6 +349,15 @@ impl Node { self.str_attr("fallback_retry_target") } + /// Node-level failure routing policy override. `None` means the node + /// inherits the graph-level policy. Invalid values are rejected during + /// workflow validation, so runtime resolution treats them as absent. + #[must_use] + pub fn on_failure(&self) -> Option { + self.str_attr("on_failure") + .and_then(|value| value.parse().ok()) + } + #[must_use] pub fn fidelity(&self) -> Option<&str> { self.str_attr("fidelity") @@ -581,6 +626,17 @@ impl Graph { .unwrap_or_default() } + /// Effective failure routing policy for a node. A node-level `on_failure` + /// attribute overrides the graph level; an absent (or invalid, hence + /// validation-rejected) node attribute inherits the graph policy. + #[must_use] + pub fn resolve_on_failure(&self, node: &Node) -> ResolvedOnFailure { + match node.on_failure() { + Some(policy) => ResolvedOnFailure::node(policy), + None => ResolvedOnFailure::graph(self.on_failure()), + } + } + /// Graph-level `default_fidelity`. pub fn default_fidelity(&self) -> Option<&str> { self.attrs @@ -634,7 +690,8 @@ impl Graph { } /// Where an attribute appears in a workflow graph. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] +#[derive(Clone, Copy, Debug, Eq, PartialEq, strum::Display, strum::IntoStaticStr)] +#[strum(serialize_all = "snake_case")] pub enum AttributeScope { Graph, Node, @@ -736,6 +793,63 @@ mod tests { assert_eq!(graph.on_failure(), OnFailure::Exit); } + #[test] + fn node_on_failure_parses_valid_values_and_ignores_invalid_ones() { + let mut node = Node::new("work"); + assert_eq!(node.on_failure(), None); + + node.attrs.insert( + "on_failure".to_string(), + AttrValue::String("exit".to_string()), + ); + assert_eq!(node.on_failure(), Some(OnFailure::Exit)); + + node.attrs.insert( + "on_failure".to_string(), + AttrValue::String("stop".to_string()), + ); + assert_eq!(node.on_failure(), None); + + node.attrs + .insert("on_failure".to_string(), AttrValue::Boolean(true)); + assert_eq!(node.on_failure(), None); + } + + #[test] + fn resolve_on_failure_prefers_node_policy_over_graph_policy() { + let mut graph = Graph::new("test"); + graph.attrs.insert( + "on_failure".to_string(), + AttrValue::String("exit".to_string()), + ); + graph.nodes.insert("bare".to_string(), Node::new("bare")); + let mut invalid = Node::new("invalid"); + invalid.attrs.insert( + "on_failure".to_string(), + AttrValue::String("bogus".to_string()), + ); + graph.nodes.insert("invalid".to_string(), invalid); + let mut route = Node::new("route"); + route.attrs.insert( + "on_failure".to_string(), + AttrValue::String("route".to_string()), + ); + graph.nodes.insert("route".to_string(), route); + + // Node attribute wins over the graph policy. + assert_eq!( + graph.resolve_on_failure(&graph.nodes["route"]), + ResolvedOnFailure::node(OnFailure::Route) + ); + // Absent and invalid node attributes inherit the graph policy. + for node_id in ["bare", "invalid"] { + assert_eq!( + graph.resolve_on_failure(&graph.nodes[node_id]), + ResolvedOnFailure::graph(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 ca1e2804d..704c2e9e9 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, OnFailure, - is_known_handler_type, is_llm_handler_type, shape_to_handler_type, + AttrValue, AttributeScope, ContextKeyAttr, Edge, Graph, KNOWN_HANDLER_TYPES, Node, OnFailure, + ResolvedOnFailure, 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_node_invalid.fabro b/test/on_failure_node_invalid.fabro new file mode 100644 index 000000000..0c5b3c44c --- /dev/null +++ b/test/on_failure_node_invalid.fabro @@ -0,0 +1,7 @@ +digraph InvalidNodeOnFailure { + start [shape=Mdiamond] + exit [shape=Msquare] + work [prompt="Do the work" on_failure="stop"] + + start -> work -> exit +}