mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-10 22:43:37 +00:00
Remove legacy tool.output key and preserve stdout on script failure
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 <noreply@anthropic.com>
This commit is contained in:
parent
7709d0a01c
commit
f16eb49d5b
3 changed files with 82 additions and 32 deletions
|
|
@ -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:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -104,7 +104,6 @@ fn stage_rendered_keys(node_id: &str, outcome: &Outcome) -> HashSet<String> {
|
|||
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<String, Outcome> = 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 ---
|
||||
|
|
|
|||
|
|
@ -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]
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue