From 491babe5dacda2f7253d3a5e2b3aa9d904945ed1 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 25 Aug 2026 18:55:39 -0400 Subject: [PATCH 1/2] Add node-level on_failure override A node can now set its own on_failure attribute to override the graph-level failed-node routing policy in either direction: a best-effort node can keep route inside an exit graph, and a critical node can exit while the rest of the graph keeps the default. An absent node attribute inherits the graph policy. - Node::on_failure returns Option so absence means inherit - Graph::resolve_on_failure(node_id) is the single resolution point, returning ResolvedOnFailure { policy, scope } so the executor's end-of-run message names the scope that stopped routing - The core Graph trait method becomes resolve_on_failure(node_id); the graph-scope failure message is unchanged - The failed-human-gate fallthrough block stays independent of a node-level route override - Validation now accepts and value-checks node-level on_failure (it previously warned that node placement had no effect) and keeps the edge-placement warning with updated wording - Document precedence in transitions, failures, and the DOT reference, and extend today's changelog entry Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Chraa21RK7i2KqHdZSJLb8 --- docs/public/changelog/2026-08-25.mdx | 5 + docs/public/execution/failures.mdx | 4 +- docs/public/reference/dot-language.mdx | 1 + docs/public/workflows/transitions.mdx | 24 ++- lib/apps/fabro-cli/tests/it/cmd/validate.rs | 18 +++ .../src/rules/on_failure_valid.rs | 140 ++++++++++++------ lib/components/fabro-workflow/src/graph.rs | 6 +- .../fabro-workflow/src/graph/routing.rs | 87 ++++++++++- .../fabro-workflow/tests/it/integration.rs | 27 ++++ lib/foundation/fabro-core/src/executor.rs | 72 ++++++++- lib/foundation/fabro-core/src/graph.rs | 6 +- .../fabro-core/src/test_fixtures.rs | 37 +++-- lib/foundation/fabro-types/src/graph.rs | 100 +++++++++++++ lib/foundation/fabro-types/src/lib.rs | 4 +- test/on_failure_node_invalid.fabro | 7 + 15 files changed, 469 insertions(+), 69 deletions(-) create mode 100644 test/on_failure_node_invalid.fabro diff --git a/docs/public/changelog/2026-08-25.mdx b/docs/public/changelog/2026-08-25.mdx index 7a031fdee..70de6ccef 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 ac04faeed..0f638bf14 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 1f67ebe3f..e011f2e76 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/validate.rs @@ -359,6 +359,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..a880f33bb 100644 --- a/lib/components/fabro-validate/src/rules/on_failure_valid.rs +++ b/lib/components/fabro-validate/src/rules/on_failure_valid.rs @@ -17,48 +17,53 @@ 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 { + let invalid_value = |subject: &str, value: &AttrValue| -> Option { + let message = match value { + AttrValue::String(value) if value.parse::().is_ok() => return None, + AttrValue::String(value) => { + format!("{subject} has invalid on_failure value '{value}'") + } + _ => format!("{subject} attribute 'on_failure' must be a string"), + }; + Some(Diagnostic { rule: self.name().to_string(), severity: Severity::Error, message, fix: Some(format!("Use one of: {}", OnFailure::expected_values())), ..Diagnostic::default() - }); - } - - 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)) - }); + if let Some(value) = graph.attrs.get("on_failure") { + diagnostics.extend(invalid_value("Graph", value)); + } + + let mut node_ids: Vec<&String> = graph.nodes.keys().collect(); + node_ids.sort(); + for node_id in node_ids { + let node = &graph.nodes[node_id]; + if let Some(value) = node.attrs.get("on_failure") { + if let Some(diagnostic) = invalid_value(&format!("Node '{node_id}'"), value) { + diagnostics.push(Diagnostic { + node_id: Some(node_id.clone()), + ..diagnostic + }); + } } } 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 +94,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 +146,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 +198,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..00db34755 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_id: &str) -> ResolvedOnFailure { + self.inner().resolve_on_failure(node_id) } } diff --git a/lib/components/fabro-workflow/src/graph/routing.rs b/lib/components/fabro-workflow/src/graph/routing.rs index 925e0b5d3..c77594436 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.id).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 7786dd49d..e04ef3e37 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..4d5d96fc4 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.id()); + 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,70 @@ 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"), + TestNode::new("downstream"), + TestNode::terminal("end"), + ], + vec![ + TestEdge::new("work", "downstream"), + TestEdge::new("downstream", "end"), + ], + "work", + ) + .with_node_on_failure("work", OnFailure::Exit); + 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"), + TestNode::new("downstream"), + TestNode::terminal("end"), + ], + vec![ + TestEdge::new("work", "downstream"), + TestEdge::new("downstream", "end"), + ], + "work", + ) + .with_on_failure(OnFailure::Exit) + .with_node_on_failure("work", OnFailure::Route); + 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..841edc9f8 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_id: &str) -> ResolvedOnFailure; } diff --git a/lib/foundation/fabro-core/src/test_fixtures.rs b/lib/foundation/fabro-core/src/test_fixtures.rs index 56e82cc59..7b9b6de79 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, OnFailureScope, ResolvedOnFailure}; use crate::context::Context; use crate::error::{Error, HandlerErrorDetail, Result}; @@ -119,11 +119,12 @@ impl EdgeSpec for TestEdge { #[derive(Debug, Clone)] pub struct TestGraph { - pub nodes: Vec, - pub edges: Vec, - pub start_node_id: String, - pub retry_targets: HashMap, - pub on_failure: OnFailure, + pub nodes: Vec, + pub edges: Vec, + pub start_node_id: String, + pub retry_targets: HashMap, + pub on_failure: OnFailure, + pub node_on_failure: HashMap, } impl TestGraph { @@ -134,6 +135,7 @@ impl TestGraph { start_node_id: start.to_string(), retry_targets: HashMap::new(), on_failure: OnFailure::Route, + node_on_failure: HashMap::new(), } } @@ -148,6 +150,12 @@ impl TestGraph { self.on_failure = on_failure; self } + + #[must_use] + pub fn with_node_on_failure(mut self, node_id: &str, on_failure: OnFailure) -> Self { + self.node_on_failure.insert(node_id.to_string(), on_failure); + self + } } impl Graph for TestGraph { @@ -217,7 +225,9 @@ impl Graph for TestGraph { } } - if outcome.status.is_failure() && self.on_failure == OnFailure::Exit { + if outcome.status.is_failure() + && self.resolve_on_failure(node.id()).policy == OnFailure::Exit + { return None; } @@ -257,8 +267,17 @@ 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_id: &str) -> ResolvedOnFailure { + match self.node_on_failure.get(node_id) { + Some(policy) => ResolvedOnFailure { + policy: *policy, + scope: OnFailureScope::Node, + }, + None => ResolvedOnFailure { + policy: self.on_failure, + scope: OnFailureScope::Graph, + }, + } } } diff --git a/lib/foundation/fabro-types/src/graph.rs b/lib/foundation/fabro-types/src/graph.rs index e411c4020..9c8ed6b0d 100644 --- a/lib/foundation/fabro-types/src/graph.rs +++ b/lib/foundation/fabro-types/src/graph.rs @@ -36,6 +36,22 @@ impl OnFailure { } } +/// The scope whose `on_failure` attribute supplied a resolved policy. +#[derive(Debug, Clone, Copy, PartialEq, Eq, strum::Display, strum::IntoStaticStr)] +#[strum(serialize_all = "snake_case")] +pub enum OnFailureScope { + Node, + Graph, +} + +/// 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 { + pub policy: OnFailure, + pub scope: OnFailureScope, +} + /// Typed attribute values for nodes, edges, and graph-level attributes. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub enum AttrValue { @@ -313,6 +329,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 +606,24 @@ 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_id: &str) -> ResolvedOnFailure { + let node_policy = self.nodes.get(node_id).and_then(Node::on_failure); + match node_policy { + Some(policy) => ResolvedOnFailure { + policy, + scope: OnFailureScope::Node, + }, + None => ResolvedOnFailure { + policy: self.on_failure(), + scope: OnFailureScope::Graph, + }, + } + } + /// Graph-level `default_fidelity`. pub fn default_fidelity(&self) -> Option<&str> { self.attrs @@ -736,6 +779,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("route"), ResolvedOnFailure { + policy: OnFailure::Route, + scope: OnFailureScope::Node, + }); + // Absent, invalid, and unknown nodes inherit the graph policy. + for node_id in ["bare", "invalid", "missing"] { + assert_eq!(graph.resolve_on_failure(node_id), ResolvedOnFailure { + policy: OnFailure::Exit, + scope: OnFailureScope::Graph, + }); + } + } + #[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..0c7302546 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, ContextKeyAttr, Edge, Graph, KNOWN_HANDLER_TYPES, Node, OnFailure, OnFailureScope, + 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 +} From 105f180d3d10d0ec42ca428b20c435bb44c039b2 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 25 Aug 2026 20:10:05 -0400 Subject: [PATCH 2/2] Simplify node failure policy resolution --- .../src/rules/on_failure_valid.rs | 72 ++++++++++------- lib/components/fabro-workflow/src/graph.rs | 4 +- .../fabro-workflow/src/graph/routing.rs | 2 +- lib/foundation/fabro-core/src/executor.rs | 16 ++-- lib/foundation/fabro-core/src/graph.rs | 2 +- .../fabro-core/src/test_fixtures.rs | 46 +++++------ lib/foundation/fabro-types/src/graph.rs | 78 +++++++++++-------- lib/foundation/fabro-types/src/lib.rs | 2 +- 8 files changed, 120 insertions(+), 102 deletions(-) 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 a880f33bb..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,39 +46,22 @@ impl LintRule for Rule { fn apply(&self, graph: &Graph) -> Vec { let mut diagnostics = Vec::new(); - let invalid_value = |subject: &str, value: &AttrValue| -> Option { - let message = match value { - AttrValue::String(value) if value.parse::().is_ok() => return None, - AttrValue::String(value) => { - format!("{subject} has invalid on_failure value '{value}'") - } - _ => format!("{subject} attribute 'on_failure' must be a string"), - }; - Some(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("Graph", value)); + diagnostics.extend(invalid_value_diagnostic(self.name(), None, value)); } - let mut node_ids: Vec<&String> = graph.nodes.keys().collect(); - node_ids.sort(); - for node_id in node_ids { - let node = &graph.nodes[node_id]; - if let Some(value) = node.attrs.get("on_failure") { - if let Some(diagnostic) = invalid_value(&format!("Node '{node_id}'"), value) { - diagnostics.push(Diagnostic { - node_id: Some(node_id.clone()), - ..diagnostic - }); - } - } + 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 { diff --git a/lib/components/fabro-workflow/src/graph.rs b/lib/components/fabro-workflow/src/graph.rs index 00db34755..1381828aa 100644 --- a/lib/components/fabro-workflow/src/graph.rs +++ b/lib/components/fabro-workflow/src/graph.rs @@ -131,7 +131,7 @@ impl Graph for WorkflowGraph { routing::get_retry_target(failed_node_id, self.inner()) } - fn resolve_on_failure(&self, node_id: &str) -> ResolvedOnFailure { - self.inner().resolve_on_failure(node_id) + 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 c77594436..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.resolve_on_failure(&node.id).policy == OnFailure::Exit { + if outcome.status.is_failure() && graph.resolve_on_failure(node).policy() == OnFailure::Exit { return None; } diff --git a/lib/foundation/fabro-core/src/executor.rs b/lib/foundation/fabro-core/src/executor.rs index 4d5d96fc4..7f976584b 100644 --- a/lib/foundation/fabro-core/src/executor.rs +++ b/lib/foundation/fabro-core/src/executor.rs @@ -283,15 +283,15 @@ impl Executor { NextStep::End => { let mut outcome = last_outcome.clone(); if outcome.status.is_failure() { - let resolved = graph.resolve_on_failure(node.id()); - let message = match resolved.policy { + 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 {} on_failure=exit stopped routing", node.id(), - resolved.scope + resolved.scope() ), }; outcome = Outcome::fail(&message); @@ -2266,7 +2266,7 @@ mod tests { async fn executor_node_exit_policy_overrides_graph_route_and_names_node_scope() { let graph = TestGraph::new( vec![ - TestNode::new("work"), + TestNode::new("work").with_on_failure(OnFailure::Exit), TestNode::new("downstream"), TestNode::terminal("end"), ], @@ -2275,8 +2275,7 @@ mod tests { TestEdge::new("downstream", "end"), ], "work", - ) - .with_node_on_failure("work", OnFailure::Exit); + ); let state = ExecutionState::new(&graph).unwrap(); let executor = ExecutorBuilder::new( Arc::new(AlwaysFailHandler::new("boom")) as Arc> @@ -2302,7 +2301,7 @@ mod tests { async fn executor_node_route_policy_overrides_graph_exit() { let graph = TestGraph::new( vec![ - TestNode::new("work"), + TestNode::new("work").with_on_failure(OnFailure::Route), TestNode::new("downstream"), TestNode::terminal("end"), ], @@ -2312,8 +2311,7 @@ mod tests { ], "work", ) - .with_on_failure(OnFailure::Exit) - .with_node_on_failure("work", OnFailure::Route); + .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"))); diff --git a/lib/foundation/fabro-core/src/graph.rs b/lib/foundation/fabro-core/src/graph.rs index 841edc9f8..e39ccd593 100644 --- a/lib/foundation/fabro-core/src/graph.rs +++ b/lib/foundation/fabro-core/src/graph.rs @@ -44,5 +44,5 @@ pub trait Graph: Send + Sync { fn get_retry_target(&self, failed_node_id: &str) -> Option; /// 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_id: &str) -> ResolvedOnFailure; + 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 7b9b6de79..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, OnFailureScope, ResolvedOnFailure}; +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 { @@ -119,12 +128,11 @@ impl EdgeSpec for TestEdge { #[derive(Debug, Clone)] pub struct TestGraph { - pub nodes: Vec, - pub edges: Vec, - pub start_node_id: String, - pub retry_targets: HashMap, - pub on_failure: OnFailure, - pub node_on_failure: HashMap, + pub nodes: Vec, + pub edges: Vec, + pub start_node_id: String, + pub retry_targets: HashMap, + pub on_failure: OnFailure, } impl TestGraph { @@ -135,7 +143,6 @@ impl TestGraph { start_node_id: start.to_string(), retry_targets: HashMap::new(), on_failure: OnFailure::Route, - node_on_failure: HashMap::new(), } } @@ -150,12 +157,6 @@ impl TestGraph { self.on_failure = on_failure; self } - - #[must_use] - pub fn with_node_on_failure(mut self, node_id: &str, on_failure: OnFailure) -> Self { - self.node_on_failure.insert(node_id.to_string(), on_failure); - self - } } impl Graph for TestGraph { @@ -225,8 +226,7 @@ impl Graph for TestGraph { } } - if outcome.status.is_failure() - && self.resolve_on_failure(node.id()).policy == OnFailure::Exit + if outcome.status.is_failure() && self.resolve_on_failure(node).policy() == OnFailure::Exit { return None; } @@ -267,16 +267,10 @@ impl Graph for TestGraph { self.retry_targets.get(failed_node_id).cloned() } - fn resolve_on_failure(&self, node_id: &str) -> ResolvedOnFailure { - match self.node_on_failure.get(node_id) { - Some(policy) => ResolvedOnFailure { - policy: *policy, - scope: OnFailureScope::Node, - }, - None => ResolvedOnFailure { - policy: self.on_failure, - scope: OnFailureScope::Graph, - }, + 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 9c8ed6b0d..4b4f18233 100644 --- a/lib/foundation/fabro-types/src/graph.rs +++ b/lib/foundation/fabro-types/src/graph.rs @@ -36,20 +36,40 @@ impl OnFailure { } } -/// The scope whose `on_failure` attribute supplied a resolved policy. -#[derive(Debug, Clone, Copy, PartialEq, Eq, strum::Display, strum::IntoStaticStr)] -#[strum(serialize_all = "snake_case")] -pub enum OnFailureScope { - Node, - Graph, -} - /// 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 { - pub policy: OnFailure, - pub scope: OnFailureScope, + 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. @@ -610,17 +630,10 @@ impl Graph { /// 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_id: &str) -> ResolvedOnFailure { - let node_policy = self.nodes.get(node_id).and_then(Node::on_failure); - match node_policy { - Some(policy) => ResolvedOnFailure { - policy, - scope: OnFailureScope::Node, - }, - None => ResolvedOnFailure { - policy: self.on_failure(), - scope: OnFailureScope::Graph, - }, + pub fn resolve_on_failure(&self, node: &Node) -> ResolvedOnFailure { + match node.on_failure() { + Some(policy) => ResolvedOnFailure::node(policy), + None => ResolvedOnFailure::graph(self.on_failure()), } } @@ -677,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, @@ -823,16 +837,16 @@ mod tests { graph.nodes.insert("route".to_string(), route); // Node attribute wins over the graph policy. - assert_eq!(graph.resolve_on_failure("route"), ResolvedOnFailure { - policy: OnFailure::Route, - scope: OnFailureScope::Node, - }); - // Absent, invalid, and unknown nodes inherit the graph policy. - for node_id in ["bare", "invalid", "missing"] { - assert_eq!(graph.resolve_on_failure(node_id), ResolvedOnFailure { - policy: OnFailure::Exit, - scope: OnFailureScope::Graph, - }); + 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) + ); } } diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index 0c7302546..704c2e9e9 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -78,7 +78,7 @@ 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, OnFailureScope, + 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};