From f16eb49d5b19a8a4bea2b2dff2ae8969e2233127 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 27 Feb 2026 14:46:36 -0500 Subject: [PATCH] Remove legacy tool.output key and preserve stdout on script failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the `tool.output` context key (unused alias from tool→script rename) and rewrite the failure branch to include stdout in both `failure_reason` and `script.output` context, so build/test output isn't lost on failure. Co-Authored-By: Claude Opus 4.6 --- crates/attractor/src/handler/script.rs | 96 +++++++++++++++++++++----- crates/attractor/src/preamble.rs | 13 +--- crates/attractor/tests/integration.rs | 5 -- 3 files changed, 82 insertions(+), 32 deletions(-) diff --git a/crates/attractor/src/handler/script.rs b/crates/attractor/src/handler/script.rs index 23d759ffb..871a800a0 100644 --- a/crates/attractor/src/handler/script.rs +++ b/crates/attractor/src/handler/script.rs @@ -129,24 +129,28 @@ impl Handler for ScriptHandler { outcome .context_updates .insert("script.output".to_string(), serde_json::json!(stdout)); - outcome - .context_updates - .insert("tool.output".to_string(), serde_json::json!(stdout)); outcome .context_updates .insert("script.stderr".to_string(), serde_json::json!(stderr)); outcome.notes = Some(format!("Script completed: {script}")); Ok(outcome) } else { - let reason = if stderr.is_empty() { - format!( - "Script failed with exit code: {}", - output.status.code().unwrap_or(-1) - ) - } else { - format!("Script failed: {}", stderr.trim()) - }; + let mut reason = format!( + "Script failed with exit code: {}", + output.status.code().unwrap_or(-1) + ); + if !stdout.trim().is_empty() { + reason.push_str("\n\n## stdout\n"); + reason.push_str(&stdout); + } + if !stderr.trim().is_empty() { + reason.push_str("\n\n## stderr\n"); + reason.push_str(&stderr); + } let mut outcome = Outcome::fail(reason); + outcome + .context_updates + .insert("script.output".to_string(), serde_json::json!(stdout)); outcome .context_updates .insert("script.stderr".to_string(), serde_json::json!(stderr)); @@ -579,7 +583,7 @@ mod tests { } #[tokio::test] - async fn tool_output_context_key_dual_write() { + async fn tool_output_context_key_not_emitted() { let handler = ScriptHandler; let mut node = Node::new("script_node"); node.attrs.insert( @@ -595,9 +599,69 @@ mod tests { .await .unwrap(); assert_eq!(outcome.status, StageStatus::Success); - let script_output = outcome.context_updates.get("script.output").unwrap(); - let tool_output = outcome.context_updates.get("tool.output").unwrap(); - assert_eq!(script_output, tool_output); - assert!(script_output.as_str().unwrap().contains("dual")); + assert!(outcome.context_updates.get("script.output").is_some()); + assert!( + outcome.context_updates.get("tool.output").is_none(), + "tool.output should not be emitted" + ); + } + + #[tokio::test] + async fn script_handler_failure_includes_stdout() { + let handler = ScriptHandler; + let mut node = Node::new("script_node"); + node.attrs.insert( + "script".to_string(), + AttrValue::String(r#"echo "build output" && echo "oops" >&2 && exit 1"#.to_string()), + ); + let context = Context::new(); + let graph = Graph::new("test"); + let logs_root = tempfile::tempdir().unwrap(); + + let outcome = handler + .execute(&node, &context, &graph, logs_root.path(), &make_services()) + .await + .unwrap(); + assert_eq!(outcome.status, StageStatus::Fail); + let reason = outcome.failure_reason.as_deref().unwrap(); + assert!( + reason.contains("build output"), + "failure_reason should contain stdout, got: {reason}" + ); + assert!( + reason.contains("oops"), + "failure_reason should contain stderr, got: {reason}" + ); + assert!( + reason.contains("exit code: 1"), + "failure_reason should contain exit code, got: {reason}" + ); + } + + #[tokio::test] + async fn script_handler_failure_sets_script_output() { + let handler = ScriptHandler; + let mut node = Node::new("script_node"); + node.attrs.insert( + "script".to_string(), + AttrValue::String(r#"echo "build output" && exit 1"#.to_string()), + ); + let context = Context::new(); + let graph = Graph::new("test"); + let logs_root = tempfile::tempdir().unwrap(); + + let outcome = handler + .execute(&node, &context, &graph, logs_root.path(), &make_services()) + .await + .unwrap(); + assert_eq!(outcome.status, StageStatus::Fail); + let script_output = outcome + .context_updates + .get("script.output") + .expect("script.output should be set on failure"); + assert!( + script_output.as_str().unwrap().contains("build output"), + "script.output should contain stdout, got: {script_output:?}" + ); } } diff --git a/crates/attractor/src/preamble.rs b/crates/attractor/src/preamble.rs index 98c9a682b..2e4b323dd 100644 --- a/crates/attractor/src/preamble.rs +++ b/crates/attractor/src/preamble.rs @@ -104,7 +104,6 @@ fn stage_rendered_keys(node_id: &str, outcome: &Outcome) -> HashSet { let candidates = [ "script.output".to_string(), "script.stderr".to_string(), - "tool.output".to_string(), "last_stage".to_string(), "last_response".to_string(), format!("response.{node_id}"), @@ -889,7 +888,6 @@ mod tests { let context = Context::new(); // script.output is set in context (the engine copies context_updates to context) context.set("script.output", serde_json::json!("hi\n")); - context.set("tool.output", serde_json::json!("hi\n")); context.set("script.stderr", serde_json::json!("")); let completed_nodes = vec!["step".to_string()]; let mut node_outcomes: HashMap = HashMap::new(); @@ -898,9 +896,6 @@ mod tests { "script.output".to_string(), serde_json::json!("hi\n"), ); - outcome - .context_updates - .insert("tool.output".to_string(), serde_json::json!("hi\n")); outcome .context_updates .insert("script.stderr".to_string(), serde_json::json!("")); @@ -914,8 +909,8 @@ mod tests { &node_outcomes, ); - // script.output and tool.output should NOT appear in the Context section - // because they're already rendered inline under the stage + // script.output should NOT appear in the Context section + // because it's already rendered inline under the stage let context_section = preamble .split("## Context") .nth(1) @@ -924,10 +919,6 @@ mod tests { !context_section.contains("script.output"), "script.output should be deduplicated from context section" ); - assert!( - !context_section.contains("tool.output"), - "tool.output should be deduplicated from context section" - ); } // --- summary:low mode --- diff --git a/crates/attractor/tests/integration.rs b/crates/attractor/tests/integration.rs index 96df17c35..3887123bc 100644 --- a/crates/attractor/tests/integration.rs +++ b/crates/attractor/tests/integration.rs @@ -1731,11 +1731,6 @@ async fn tool_handler_e2e() { .get("script.output") .expect("script.output should exist"); assert!(script_output.as_str().unwrap().contains("hello-from-script")); - let tool_output = cp - .context_values - .get("tool.output") - .expect("tool.output should exist"); - assert_eq!(script_output, tool_output); } #[tokio::test]