From 02437fb18a3ff3dc2d92f1084929fc7c50d19429 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Jul 2026 15:28:19 -0400 Subject: [PATCH] fix(workflow): constrain last-file routing fallback --- docs/public/agents/outputs.mdx | 2 + docs/public/reference/dot-language.mdx | 2 +- .../fabro-workflow/src/handler/agent.rs | 202 ++++++++++++++---- .../src/handler/structured_output.rs | 34 +++ 4 files changed, 195 insertions(+), 45 deletions(-) diff --git a/docs/public/agents/outputs.mdx b/docs/public/agents/outputs.mdx index e03b4e08f..d62073dea 100644 --- a/docs/public/agents/outputs.mdx +++ b/docs/public/agents/outputs.mdx @@ -95,6 +95,8 @@ Agent nodes can provide routing directives through fallback files. Fabro checks This fallback chain applies to normal routing extraction and to `output_schema="routing"`. For validated routing, Fabro only advances to the next source when the current source has no JSON object or no object with recognized routing fields. If the current source contains malformed routing JSON or valid JSON with wrong routing field types, validation fails and Fabro starts the repair loop instead. +The last-file fallback only reads `.json` and `.md` files (case-insensitive), and the routing JSON must be the final standalone object in the file, followed only by whitespace. Fabro ignores other file types and routing JSON followed by any other content. These restrictions do not apply to the dedicated `status.json` fallback. + Prompt nodes do not use file fallbacks; they validate or extract routing directives from the response text only. If no source provides routing directives, the transition falls through to condition matching, unconditional edges, or weight-based tiebreaking as described in [Transitions](/workflows/transitions). diff --git a/docs/public/reference/dot-language.mdx b/docs/public/reference/dot-language.mdx index f01fbf2ed..2184ab1d4 100644 --- a/docs/public/reference/dot-language.mdx +++ b/docs/public/reference/dot-language.mdx @@ -235,7 +235,7 @@ audit [ - On validation failure, Fabro sends validation feedback to the same active context before failing: prompt nodes keep the prior assistant response in the message list, and API-backed agent nodes repair in the same live session. - `output_retries` defaults to `2` and controls only these corrective structured-output turns. Negative values are treated as `0`. It is not the same as `max_retries` and does not consume workflow retry attempts. - Custom schema output is stored in context at `output.{node_id}`. Routing schema output updates routing fields and any `context_updates`. -- Agent routing fallbacks still apply to `output_schema="routing"`: response text first, then `status.json`, then the last file touched by the agent. Custom schemas and prompt nodes validate response text only. +- Agent routing fallbacks still apply to `output_schema="routing"`: response text first, then `status.json`, then the last file touched by the agent. The last-file fallback only accepts `.json` and `.md` files (case-insensitive) whose final standalone JSON object contains the routing directive; only whitespace may follow it. Custom schemas and prompt nodes validate response text only. - `backend="acp"` with `output_schema` is unsupported in this release. ### Command nodes diff --git a/lib/crates/fabro-workflow/src/handler/agent.rs b/lib/crates/fabro-workflow/src/handler/agent.rs index 16dbe0230..9ba8c2fe1 100644 --- a/lib/crates/fabro-workflow/src/handler/agent.rs +++ b/lib/crates/fabro-workflow/src/handler/agent.rs @@ -19,6 +19,8 @@ use crate::event::{Emitter, Event, StageScope}; use crate::interview_runtime::WorkflowAgentQuestionRuntime; use crate::outcome::{BilledModelUsage, Outcome, OutcomeExt}; +const LAST_FILE_ROUTING_EXTENSIONS: &[&str] = &["json", "md"]; + /// Result from a `CodergenBackend` invocation. #[allow( clippy::large_enum_variant, @@ -169,8 +171,8 @@ pub(crate) async fn validate_agent_output_sources( } if let Some(path) = last_file_touched { - if let Some(contents) = read_sandbox_file(sandbox, path).await { - return structured_output::validate_response_text(schema, &contents); + if let Some(routing_json) = read_last_file_routing_json(sandbox, path).await { + return structured_output::validate_response_text(schema, &routing_json); } } @@ -181,6 +183,19 @@ async fn read_sandbox_file(sandbox: &Arc, path: &str) -> Option, path: &str) -> Option { + let extension = Path::new(path).extension()?.to_str()?; + if !LAST_FILE_ROUTING_EXTENSIONS + .iter() + .any(|allowed| extension.eq_ignore_ascii_case(allowed)) + { + return None; + } + + let contents = read_sandbox_file(sandbox, path).await?; + structured_output::terminal_json_object(&contents).map(str::to_owned) +} + /// Truncate a string to at most `max_chars` characters (char-boundary safe). pub(crate) fn truncate(s: &str, max_chars: usize) -> &str { if s.len() <= max_chars { @@ -382,7 +397,7 @@ impl Handler for AgentHandler { } else { // 7b. Parse routing directives from response text, falling back to // status.json written by the agent into the sandbox CWD, then to - // the last file the agent wrote. + // a terminal JSON object in an eligible last-written file. let found_in_response = extract_status_fields(&response_text, &mut outcome); if !found_in_response { let mut found_in_status_json = false; @@ -393,9 +408,10 @@ impl Handler for AgentHandler { } if !found_in_status_json { if let Some(ref path) = last_file_touched { - if let Some(contents) = read_sandbox_file(&services.run.sandbox, path).await + if let Some(routing_json) = + read_last_file_routing_json(&services.run.sandbox, path).await { - extract_status_fields(&contents, &mut outcome); + extract_status_fields(&routing_json, &mut outcome); } } } @@ -506,6 +522,68 @@ mod tests { context } + struct LastFileBackend { + path: String, + } + + #[async_trait] + impl CodergenBackend for LastFileBackend { + async fn run(&self, _request: CodergenRunRequest<'_>) -> Result { + Ok(CodergenResult::Text { + text: "Done writing results.".to_string(), + usage: None, + files_touched: vec![self.path.clone()], + last_file_touched: Some(self.path.clone()), + timing: StageTiming::default(), + }) + } + } + + async fn execute_with_last_file(path: &str, contents: &str) -> Outcome { + let sandbox_dir = TempDir::new().unwrap(); + std::fs::write(sandbox_dir.path().join(path), contents).unwrap(); + + let handler = AgentHandler::new(Some(Box::new(LastFileBackend { + path: path.to_string(), + }))); + let node = Node::new("step"); + let context = test_context(); + let graph = Graph::new("test"); + let tmp = TempDir::new().unwrap(); + + let mut services = EngineServices::test_default(); + services.run = + services + .run + .with_sandbox(std::sync::Arc::new(fabro_agent::LocalSandbox::new( + sandbox_dir.path().to_path_buf(), + ))); + + handler + .execute(&node, &context, &graph, tmp.path(), &services) + .await + .unwrap() + } + + async fn validate_routing_with_last_file( + path: &str, + contents: &str, + ) -> Result { + let sandbox_dir = TempDir::new().unwrap(); + std::fs::write(sandbox_dir.path().join(path), contents).unwrap(); + let sandbox: Arc = Arc::new(fabro_agent::LocalSandbox::new( + sandbox_dir.path().to_path_buf(), + )); + + validate_agent_output_sources( + &OutputSchemaKind::Routing, + "Done writing results.", + &sandbox, + Some(path), + ) + .await + } + #[tokio::test] async fn codergen_handler_simulate() { let handler = AgentHandler::new(None); @@ -731,49 +809,13 @@ mod tests { #[tokio::test] async fn codergen_handler_extracts_status_from_last_file_touched() { - struct LastFileBackend; - - #[async_trait] - impl CodergenBackend for LastFileBackend { - async fn run(&self, _request: CodergenRunRequest<'_>) -> Result { - Ok(CodergenResult::Text { - text: "Done writing results.".to_string(), - usage: None, - files_touched: vec!["results.md".to_string()], - last_file_touched: Some("results.md".to_string()), - timing: StageTiming::default(), - }) - } - } - - let sandbox_dir = TempDir::new().unwrap(); - // Write status fields into the file the agent "touched" — no status.json - std::fs::write( - sandbox_dir.path().join("results.md"), + let outcome = execute_with_last_file( + "results.md", r#"# Results {"context_updates": {"verified": "true"}} "#, ) - .unwrap(); - - let handler = AgentHandler::new(Some(Box::new(LastFileBackend))); - let node = Node::new("step"); - let context = test_context(); - let graph = Graph::new("test"); - let tmp = TempDir::new().unwrap(); - - let mut services = EngineServices::test_default(); - services.run = - services - .run - .with_sandbox(std::sync::Arc::new(fabro_agent::LocalSandbox::new( - sandbox_dir.path().to_path_buf(), - ))); - - let outcome = handler - .execute(&node, &context, &graph, tmp.path(), &services) - .await - .unwrap(); + .await; assert_eq!(outcome.status, crate::outcome::StageOutcome::Succeeded); assert_eq!( @@ -782,6 +824,33 @@ mod tests { ); } + #[tokio::test] + async fn codergen_handler_ignores_nonterminal_status_in_last_markdown_file() { + let outcome = execute_with_last_file( + "results.md", + r#"{"outcome":"failed","failure_reason":"tests failed"} + +All checks passed. +"#, + ) + .await; + + assert_eq!(outcome.status, crate::outcome::StageOutcome::Succeeded); + assert!(outcome.failure.is_none()); + } + + #[tokio::test] + async fn codergen_handler_ignores_terminal_status_in_disallowed_last_file() { + let outcome = execute_with_last_file( + "command.rs", + r#"{"outcome":"failed","failure_reason":"tests failed"}"#, + ) + .await; + + assert_eq!(outcome.status, crate::outcome::StageOutcome::Succeeded); + assert!(outcome.failure.is_none()); + } + #[tokio::test] async fn codergen_handler_output_schema_routing_uses_status_json_fallback_when_response_has_no_json() { @@ -819,6 +888,51 @@ mod tests { assert_eq!(outcome.preferred_label.as_deref(), Some("review")); } + #[tokio::test] + async fn validated_routing_accepts_terminal_status_in_json_file_case_insensitively() { + let validated = validate_routing_with_last_file( + "results.JSON", + "# Results\n\n{\"preferred_next_label\":\"review\"}\n", + ) + .await + .unwrap(); + + assert_eq!( + validated.value, + serde_json::json!({"preferred_next_label": "review"}), + ); + } + + #[tokio::test] + async fn validated_routing_ignores_nonterminal_status_in_last_markdown_file() { + let error = validate_routing_with_last_file( + "results.md", + "{\"outcome\":\"failed\",\"failure_reason\":\"tests failed\"}\nAll checks passed.", + ) + .await + .unwrap_err(); + + assert_eq!( + error.kind(), + structured_output::StructuredOutputErrorKind::NoJsonObject, + ); + } + + #[tokio::test] + async fn validated_routing_ignores_terminal_status_in_disallowed_last_file() { + let error = validate_routing_with_last_file( + "command.rs", + r#"{"outcome":"failed","failure_reason":"tests failed"}"#, + ) + .await + .unwrap_err(); + + assert_eq!( + error.kind(), + structured_output::StructuredOutputErrorKind::NoJsonObject, + ); + } + #[tokio::test] async fn codergen_handler_output_schema_routing_rejects_malformed_response_before_status_json_fallback() { diff --git a/lib/crates/fabro-workflow/src/handler/structured_output.rs b/lib/crates/fabro-workflow/src/handler/structured_output.rs index 546124b43..ea2c2aad1 100644 --- a/lib/crates/fabro-workflow/src/handler/structured_output.rs +++ b/lib/crates/fabro-workflow/src/handler/structured_output.rs @@ -264,6 +264,15 @@ fn find_json_objects(text: &str) -> Vec<&str> { results } +/// Return the outermost balanced JSON object that ends the text, ignoring +/// trailing whitespace. +pub(crate) fn terminal_json_object(text: &str) -> Option<&str> { + let trimmed = text.trim_end(); + find_json_objects(trimmed) + .into_iter() + .find(|candidate| trimmed.ends_with(candidate)) +} + pub(crate) fn extract_status_fields(text: &str, outcome: &mut Outcome) -> bool { let candidates = find_json_objects(text); @@ -493,6 +502,31 @@ mod tests { assert!(error.messages()[0].contains("recognized routing field")); } + #[test] + fn terminal_json_object_accepts_final_object_after_prose() { + let object = + terminal_json_object("# Results\n\n{\"context_updates\":{\"verified\":true}}\n\n"); + + assert_eq!(object, Some(r#"{"context_updates":{"verified":true}}"#),); + } + + #[test] + fn terminal_json_object_returns_outermost_nested_object() { + let object = terminal_json_object(r#"Results: {"context_updates":{"verified":true}}"#); + + assert_eq!(object, Some(r#"{"context_updates":{"verified":true}}"#),); + } + + #[test] + fn terminal_json_object_rejects_object_followed_by_content() { + assert_eq!( + terminal_json_object( + "{\"outcome\":\"failed\",\"failure_reason\":\"tests failed\"}\nMore details", + ), + None, + ); + } + #[test] fn routing_json_with_wrong_field_type_is_invalid() { let error =