diff --git a/bt-daemon/src/translate/claude.rs b/bt-daemon/src/translate/claude.rs index e212ae4..93a4b06 100644 --- a/bt-daemon/src/translate/claude.rs +++ b/bt-daemon/src/translate/claude.rs @@ -949,16 +949,7 @@ impl AgentTranslator for ClaudeTranslator { "PostToolUseFailure" => self.finish_tool( event, ToolApproval::Approved, - tool_error(&event.payload) - .or_else(|| { - event - .payload - .pointer("/tool_response/output") - .and_then(Value::as_str) - .filter(|value| !value.is_empty()) - .map(str::to_owned) - }) - .or_else(|| Some("Tool execution failed".into())), + tool_failure_text(&event.payload).or_else(|| Some("Tool execution failed".into())), &mut ops, ), "PermissionDenied" => self.finish_tool(event, ToolApproval::Denied, None, &mut ops), @@ -988,7 +979,8 @@ impl AgentTranslator for ClaudeTranslator { "Stop" => self.stop_turn(event, None, &mut ops), "StopFailure" => self.stop_turn( event, - tool_error(&event.payload).or_else(|| Some("Claude Code turn failed".into())), + tool_failure_text(&event.payload) + .or_else(|| Some("Claude Code turn failed".into())), &mut ops, ), "SessionEnd" => self.end_session(event, &mut ops), @@ -1800,51 +1792,48 @@ fn tool_span_name(tool: &str, input: &Value) -> String { } fn tool_error(payload: &Value) -> Option { - for value in [ + let response = payload.get("tool_response"); + let explicit_error = [ payload.get("error"), - payload.get("message"), - payload.pointer("/tool_response/error"), - payload.pointer("/tool_response/stderr"), - payload.pointer("/tool_response/message"), + response.and_then(|response| response.get("error")), ] .into_iter() .flatten() - { - if let Some(text) = nonempty_error_text(value) { - return Some(text); - } + .find_map(nonempty_error_text); + if let Some(error) = explicit_error { + return Some(error); } - let response = payload.get("tool_response")?; - let failed = response - .get("interrupted") - .and_then(Value::as_bool) - .unwrap_or(false) - || response - .get("is_error") - .or_else(|| response.get("isError")) + let failed = response.is_some_and(|response| { + response + .get("interrupted") .and_then(Value::as_bool) .unwrap_or(false) - || response - .get("status") - .and_then(Value::as_str) - .is_some_and(|value| matches!(value, "error" | "failed")); - if !failed { - return None; - } - for value in [ - response.get("error"), - response.get("stderr"), - response.get("message"), - response.get("output"), + || response + .get("is_error") + .or_else(|| response.get("isError")) + .and_then(Value::as_bool) + .unwrap_or(false) + || response + .get("status") + .and_then(Value::as_str) + .is_some_and(|value| matches!(value, "error" | "failed")) + }); + failed + .then(|| tool_failure_text(payload).unwrap_or_else(|| "Tool execution failed".to_string())) +} + +fn tool_failure_text(payload: &Value) -> Option { + [ + payload.get("error"), + payload.get("message"), + payload.pointer("/tool_response/error"), + payload.pointer("/tool_response/stderr"), + payload.pointer("/tool_response/message"), + payload.pointer("/tool_response/output"), ] .into_iter() .flatten() - { - if let Some(text) = nonempty_error_text(value) { - return Some(text); - } - } - Some("Tool execution failed".to_string()) + .find_map(nonempty_error_text) } #[cfg(test)] diff --git a/bt-daemon/src/translate/codex.rs b/bt-daemon/src/translate/codex.rs index 4700464..cb5f456 100644 --- a/bt-daemon/src/translate/codex.rs +++ b/bt-daemon/src/translate/codex.rs @@ -19,7 +19,7 @@ use super::git::GitMetadataCache; use super::recent::{RecentMap, RecentSet}; -use super::tool::{error_text, tool_approval_metadata, ToolApproval}; +use super::tool::{error_text, nonempty_error_text, tool_approval_metadata, ToolApproval}; use super::{ local_username, AgentTranslator, SessionCtx, SpanOp, SpanRow, SpanType, TranslatorFactory, }; @@ -1465,6 +1465,17 @@ fn args_object(args: Option<&Value>) -> Option> { } } +fn has_error_marker(value: &Value) -> bool { + match value { + Value::Null => false, + Value::Bool(value) => *value, + Value::String(_) => nonempty_error_text(value).is_some(), + Value::Array(values) => !values.is_empty(), + Value::Object(values) => !values.is_empty(), + Value::Number(_) => true, + } +} + fn classify_tool_output(output: &Value) -> Option { if let Some(object) = output.as_object() { if object.get("is_error").and_then(Value::as_bool) == Some(true) @@ -1476,7 +1487,7 @@ fn classify_tool_output(output: &Value) -> Option { { return Some(error_text(Some(output), "Tool execution failed")); } - if let Some(error) = object.get("error") { + if let Some(error) = object.get("error").filter(|error| has_error_marker(error)) { return Some(error_text(Some(error), "Tool execution failed")); } if let Some(exit_code) = object diff --git a/bt-daemon/src/translate/tool.rs b/bt-daemon/src/translate/tool.rs index 1432e1e..f52b222 100644 --- a/bt-daemon/src/translate/tool.rs +++ b/bt-daemon/src/translate/tool.rs @@ -52,27 +52,17 @@ pub fn error_text(value: Option<&Value>, fallback: &str) -> String { /// Extracts a concise message when the source has already identified a value /// as an error. Callers must not use this to infer failure from normal output. pub fn nonempty_error_text(value: &Value) -> Option { - if let Some(text) = value.as_str().filter(|text| !text.is_empty()) { - return Some(text.lines().next().unwrap_or(text).to_string()); + if let Some(text) = value.as_str() { + return text + .lines() + .map(str::trim) + .find(|line| !line.is_empty()) + .map(str::to_string); } let object = value.as_object()?; for key in ["error", "message", "stderr", "output", "result"] { - let Some(candidate) = object.get(key) else { - continue; - }; - if let Some(text) = candidate.as_str().filter(|text| !text.is_empty()) { - return Some(text.lines().next().unwrap_or(text).to_string()); - } - if let Some(nested) = candidate.as_object() { - for key in ["error", "message"] { - if let Some(text) = nested - .get(key) - .and_then(Value::as_str) - .filter(|text| !text.is_empty()) - { - return Some(text.lines().next().unwrap_or(text).to_string()); - } - } + if let Some(text) = object.get(key).and_then(nonempty_error_text) { + return Some(text); } } None @@ -101,5 +91,9 @@ mod tests { "disk full" ); assert_eq!(error_text(None, "fallback"), "fallback"); + assert_eq!( + nonempty_error_text(&json!("\n disk full\ntrace")), + Some("disk full".into()) + ); } } diff --git a/bt-daemon/tests/claude_translator.rs b/bt-daemon/tests/claude_translator.rs index 6baad8d..35a7497 100644 --- a/bt-daemon/tests/claude_translator.rs +++ b/bt-daemon/tests/claude_translator.rs @@ -10,6 +10,22 @@ fn fixture(name: &str) -> PathBuf { .join(name) } +fn claude_event(session_id: &str, event: &str, ts_ms: i64, payload: Value) -> Envelope { + Envelope { + source: "claude-code".into(), + source_version: None, + plugin_version: None, + session_id: session_id.into(), + event: event.into(), + ts_ms, + managed_run_id: None, + payload, + route: None, + config: None, + capture: None, + } +} + /// How a replayed event points at its transcript bytes. #[derive(Clone, Copy, PartialEq)] enum Source { @@ -496,19 +512,7 @@ fn claude_permission_denied_and_failed_tools_are_first_class_spans() { session_id: "s".into(), config: None, }; - let event = |name: &str, payload: Value| Envelope { - source: "claude-code".into(), - source_version: None, - plugin_version: None, - session_id: "s".into(), - event: name.into(), - ts_ms: 1, - managed_run_id: None, - payload, - route: None, - config: None, - capture: None, - }; + let event = |name: &str, payload: Value| claude_event("s", name, 1, payload); let mut ops = translator .handle( &event( @@ -540,7 +544,7 @@ fn claude_permission_denied_and_failed_tools_are_first_class_spans() { .handle( &event( "PostToolUse", - json!({"session_id":"s","tool_name":"Write","tool_use_id":"c","tool_input":{"file_path":"x"},"tool_response":{"is_error":true,"stderr":"disk full"}}), + json!({"session_id":"s","tool_name":"Write","tool_use_id":"c","tool_input":{"file_path":"x"},"tool_response":{"is_error":true,"stderr":"\ndisk full"}}), ), &ctx, ) @@ -592,6 +596,78 @@ fn claude_permission_denied_and_failed_tools_are_first_class_spans() { })); } +#[test] +fn claude_successful_tool_outputs_do_not_populate_error() { + let registry = Registry::default_agents(); + let mut translator = registry.create("claude-code", "successful-tools"); + let ctx = SessionCtx { + session_id: "successful-tools".into(), + config: None, + }; + let event = |name: &str, ts_ms: i64, payload: Value| { + claude_event("successful-tools", name, ts_ms, payload) + }; + let mut ops = translator + .handle( + &event( + "UserPromptSubmit", + 10, + json!({"session_id":"successful-tools","prompt":"run tools"}), + ), + &ctx, + ) + .unwrap(); + for envelope in [ + event( + "PostToolUse", + 20, + json!({ + "session_id":"successful-tools", + "tool_name":"Bash", + "tool_use_id":"bash-1", + "tool_input":{"command":"pwd"}, + "tool_response":{ + "stdout":"/tmp", + "stderr":"\nShell cwd was reset to /tmp", + "interrupted":false + } + }), + ), + event( + "PostToolUse", + 30, + json!({ + "session_id":"successful-tools", + "tool_name":"TaskStop", + "tool_use_id":"stop-1", + "tool_input":{"task_id":"abc"}, + "tool_response":{ + "task_id":"abc", + "message":"Successfully stopped task: abc" + } + }), + ), + ] { + ops.extend(translator.handle(&envelope, &ctx).unwrap()); + } + + let rows = reduce(ops); + let tools = rows + .values() + .filter(|row| row.span_type == SpanType::Tool) + .collect::>(); + assert_eq!(tools.len(), 2); + assert!(tools.iter().all(|row| row.error.is_none())); + assert!(tools.iter().any(|row| { + row.metadata.as_ref().unwrap()["tool_name"] == json!("Bash") + && row.output.as_ref().unwrap()["stdout"] == json!("/tmp") + })); + assert!(tools.iter().any(|row| { + row.metadata.as_ref().unwrap()["tool_name"] == json!("TaskStop") + && row.output.as_ref().unwrap()["message"] == json!("Successfully stopped task: abc") + })); +} + #[test] fn claude_pairs_tool_lifecycle_and_marks_explicit_skills_and_stop_failures() { let registry = Registry::default_agents(); @@ -600,18 +676,10 @@ fn claude_pairs_tool_lifecycle_and_marks_explicit_skills_and_stop_failures() { session_id: "lifecycle".into(), config: None, }; - let event = |name: &str, ts_ms: i64, payload: Value| Envelope { - source: "claude-code".into(), - source_version: Some("2.0.0".into()), - plugin_version: None, - session_id: "lifecycle".into(), - event: name.into(), - ts_ms, - managed_run_id: None, - payload, - route: None, - config: None, - capture: None, + let event = |name: &str, ts_ms: i64, payload: Value| { + let mut event = claude_event("lifecycle", name, ts_ms, payload); + event.source_version = Some("2.0.0".into()); + event }; let mut ops = Vec::new(); for envelope in [ @@ -633,7 +701,7 @@ fn claude_pairs_tool_lifecycle_and_marks_explicit_skills_and_stop_failures() { event( "StopFailure", 40, - json!({"session_id":"lifecycle","error":"model process exited"}), + json!({"session_id":"lifecycle","message":"model process exited"}), ), ] { ops.extend(translator.handle(&envelope, &ctx).unwrap()); diff --git a/bt-daemon/tests/codex_translator.rs b/bt-daemon/tests/codex_translator.rs index d2874e2..8fe8404 100644 --- a/bt-daemon/tests/codex_translator.rs +++ b/bt-daemon/tests/codex_translator.rs @@ -805,6 +805,71 @@ fn tool_and_llm_payloads_preserve_original_contract() { ); } +#[test] +fn codex_successful_structured_tool_outputs_do_not_populate_error() { + let tmp = tempfile::tempdir().unwrap(); + let transcript = tmp.path().join("rollout.jsonl"); + for record in [ + json!({ "timestamp": "2026-01-01T00:00:01Z", "type": "session_meta", + "payload": { "id": "s", "cwd": "/x/app" } }), + json!({ "timestamp": "2026-01-01T00:00:02Z", "type": "event_msg", + "payload": { "type": "task_started", "turn_id": "t1" } }), + json!({ "timestamp": "2026-01-01T00:00:03Z", "type": "response_item", + "payload": { "type": "function_call", "call_id": "c1", "name": "first_tool", + "arguments": "{}", "metadata": { "turn_id": "t1" } } }), + json!({ "timestamp": "2026-01-01T00:00:04Z", "type": "response_item", + "payload": { "type": "function_call_output", "call_id": "c1", + "output": { "result": "ok", "error": null } } }), + json!({ "timestamp": "2026-01-01T00:00:05Z", "type": "response_item", + "payload": { "type": "function_call", "call_id": "c2", "name": "second_tool", + "arguments": "{}", "metadata": { "turn_id": "t1" } } }), + json!({ "timestamp": "2026-01-01T00:00:06Z", "type": "response_item", + "payload": { "type": "function_call_output", "call_id": "c2", + "output": { "result": "ok", "error": "" } } }), + json!({ "timestamp": "2026-01-01T00:00:07Z", "type": "response_item", + "payload": { "type": "function_call", "call_id": "c3", "name": "third_tool", + "arguments": "{}", "metadata": { "turn_id": "t1" } } }), + json!({ "timestamp": "2026-01-01T00:00:08Z", "type": "response_item", + "payload": { "type": "function_call_output", "call_id": "c3", + "output": { "result": "ok", "error": false } } }), + json!({ "timestamp": "2026-01-01T00:00:09Z", "type": "response_item", + "payload": { "type": "function_call", "call_id": "c4", "name": "fourth_tool", + "arguments": "{}", "metadata": { "turn_id": "t1" } } }), + json!({ "timestamp": "2026-01-01T00:00:10Z", "type": "response_item", + "payload": { "type": "function_call_output", "call_id": "c4", + "output": { "error": true } } }), + json!({ "timestamp": "2026-01-01T00:00:11Z", "type": "event_msg", + "payload": { "type": "task_complete", "turn_id": "t1", + "last_agent_message": "done" } }), + ] { + append(&transcript, record); + } + let registry = Registry::default_agents(); + let mut translator = registry.create("codex", "s"); + let ctx = SessionCtx { + session_id: "s".into(), + config: None, + }; + let rows = reduce( + translator + .handle( + &envelope("s", "SessionStart", transcript.to_str().unwrap(), json!({})), + &ctx, + ) + .unwrap(), + ); + + for name in ["first_tool", "second_tool", "third_tool"] { + let tool = find(&rows, SpanType::Tool, name); + assert_eq!(tool.error, None, "{name} should be successful"); + assert_eq!(tool.output.as_ref().unwrap()["result"], json!("ok")); + } + assert_eq!( + find(&rows, SpanType::Tool, "fourth_tool").error.as_deref(), + Some("Tool execution failed") + ); +} + #[test] fn missing_tool_output_is_an_error() { let tmp = tempfile::tempdir().unwrap();