Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 79 additions & 4 deletions crates/tinyagents-harness/src/providers/claude_code/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,70 @@ fn push_bounded(acc: &mut String, chunk: &str, max_bytes: usize) {
}
}

/// Upper bound on the structured stdout error carried into a returned error.
const STDOUT_ERROR_CAP: usize = 2_048;

/// Redact every `sk-` style key in `text`, wherever it sits (after `=`, `:`,
/// quotes, URL separators, ...). A match must start at a non-identifier
/// boundary (so `task-force` is untouched) and run over the key alphabet.
fn redact_sk_keys(text: &str) -> String {
let is_key_char = |c: char| c.is_ascii_alphanumeric() || c == '-' || c == '_';
let mut out = String::with_capacity(text.len());
let mut rest = text;
let mut prev: Option<char> = None;
while let Some(pos) = rest.find("sk-") {
let before = rest[..pos].chars().next_back().or(prev);
let boundary = before.is_none_or(|c| !(c.is_ascii_alphanumeric() || c == '_'));
let tail = &rest[pos + 3..];
let key_len = tail.find(|c: char| !is_key_char(c)).unwrap_or(tail.len());
if boundary && key_len > 0 {
out.push_str(&rest[..pos]);
out.push_str("[redacted]");
prev = Some(']');
rest = &tail[key_len..];
} else {
out.push_str(&rest[..pos + 3]);
prev = Some('-');
rest = tail;
}
}
out.push_str(rest);
out
}

/// Remove credentials from a CLI-reported message and bound its length.
/// Redacts the configured API key wherever it occurs (including inside larger
/// tokens) and any `sk-` style key, then caps the result.
fn sanitize_cli_message(raw: &str, api_key: Option<&str>) -> String {
let mut text = raw.to_string();
if let Some(key) = api_key.filter(|key| !key.is_empty()) {
text = text.replace(key, "[redacted]");
}
let redacted = redact_sk_keys(&text);
let mut out = String::new();
push_bounded(&mut out, &redacted, STDOUT_ERROR_CAP);
out
}

/// Error text for a nonzero CLI exit: the structured stdout error when the
/// CLI printed one (sanitized and bounded), otherwise the bounded stderr.
fn nonzero_exit_message(
code: Option<i32>,
stdout_error: Option<&str>,
stderr: &str,
api_key: Option<&str>,
) -> String {
match stdout_error.map(|e| sanitize_cli_message(e, api_key)) {
Some(message) if !message.is_empty() => {
format!("[claude-code][driver] exit {code:?}: {message}")
}
_ => format!(
"[claude-code][driver] exit {code:?} stderr={}",
sanitize_cli_message(stderr.trim(), api_key)
),
Comment thread
senamakel marked this conversation as resolved.
Comment thread
senamakel marked this conversation as resolved.
}
}

use super::bridge::{ChatMessage, ChatResponse, ProviderDelta};
use super::event_mapper::EventMapper;
use super::input_builder::build_stdin;
Expand Down Expand Up @@ -612,14 +676,25 @@ pub(crate) async fn run_turn(ctx: TurnContext<'_>) -> anyhow::Result<ChatRespons
let stderr_text = stderr_task.await.unwrap_or_default();

if !status.success() {
// The CLI reports most failures (bad auth, unknown model, invalid
// session) as a structured `result`/`error` event on stdout and often
// leaves stderr empty. Prefer that message; fall back to stderr only
// when stdout carried none.
anyhow::bail!(
"[claude-code][driver] exit {:?} stderr={}",
status.code(),
stderr_text.trim()
"{}",
nonzero_exit_message(
status.code(),
mapper.error.as_deref(),
&stderr_text,
ctx.anthropic_api_key.as_deref(),
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
);
}
if let Some(err) = mapper.error.clone() {
anyhow::bail!("[claude-code][driver] {}", err);
anyhow::bail!(
"[claude-code][driver] {}",
sanitize_cli_message(&err, ctx.anthropic_api_key.as_deref())
);
}

// Do not make a session durable until Claude has accepted the launch and
Expand Down
163 changes: 163 additions & 0 deletions crates/tinyagents-harness/src/providers/claude_code/driver_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,3 +350,166 @@ fn the_shape_of_the_line_is_reported() {
assert!(shape("panic: claude-code crashed").contains("non-json"));
assert!(shape(" ").contains("blank"));
}

// ---- nonzero exit with a structured stdout error (openhuman#5712) ----

#[cfg(unix)]
async fn run_fake_claude(script_body: &str, api_key: Option<&str>) -> anyhow::Result<ChatResponse> {
use std::os::unix::fs::PermissionsExt;
let dir = tempfile::tempdir().expect("tempdir");
let bin = dir.path().join("claude");
std::fs::write(&bin, format!("#!/bin/sh\ncat >/dev/null\n{script_body}\n")).expect("script");
std::fs::set_permissions(&bin, std::fs::Permissions::from_mode(0o755)).expect("chmod");
let messages = [ChatMessage::user("hello")];
run_turn(TurnContext {
bin_path: bin,
workspace_dir: dir.path().join("ws"),
project_dir: dir.path().join("project"),
thread_id: "t-5712".into(),
model: "sonnet".into(),
append_system_prompt: None,
messages: &messages,
session_store: Arc::new(SessionStore::open(&dir.path().join("ws"))),
stream: None,
anthropic_api_key: api_key.map(str::to_string),
persist_session: false,
mcp_provider: None,
})
.await
}

#[cfg(unix)]
#[tokio::test]
async fn nonzero_exit_surfaces_structured_stdout_error() {
let err = run_fake_claude(
r#"echo '{"type":"result","subtype":"success","is_error":true,"result":"Invalid API key - please run /login"}'; exit 1"#,
None,
)
.await
.expect_err("nonzero exit must fail");
let text = err.to_string();
assert!(
text.contains("Invalid API key - please run /login"),
"{text}"
);
assert!(text.contains("exit Some(1)"), "{text}");
}

#[cfg(unix)]
#[tokio::test]
async fn nonzero_exit_surfaces_error_event_message() {
let err = run_fake_claude(
r#"echo '{"type":"error","error":"model not found"}'; exit 2"#,
Comment thread
senamakel marked this conversation as resolved.
None,
)
.await
.expect_err("nonzero exit must fail");
assert!(err.to_string().contains("model not found"), "{err}");
}

#[cfg(unix)]
#[tokio::test]
async fn nonzero_exit_without_stdout_error_falls_back_to_stderr() {
let err = run_fake_claude(r#"echo 'boom on stderr' >&2; exit 1"#, None)
.await
.expect_err("nonzero exit must fail");
let text = err.to_string();
assert!(text.contains("stderr=boom on stderr"), "{text}");
}

#[cfg(unix)]
#[tokio::test]
async fn nonzero_exit_stdout_error_keeps_secrets_out() {
let err = run_fake_claude(
r#"echo '{"type":"result","is_error":true,"result":"auth failed for key sk-ant-live-key-1234 and my-configured-secret"}'; exit 1"#,
Some("my-configured-secret"),
)
.await
.expect_err("nonzero exit must fail");
let text = err.to_string();
assert!(text.contains("auth failed"), "{text}");
assert!(!text.contains("sk-ant-live-key-1234"), "{text}");
assert!(!text.contains("my-configured-secret"), "{text}");
}

#[test]
fn stdout_error_is_bounded() {
let long = "e".repeat(STDOUT_ERROR_CAP * 3);
let message = nonzero_exit_message(Some(1), Some(&long), "", None);
assert!(message.len() < STDOUT_ERROR_CAP + 128, "{}", message.len());
}

#[test]
fn sk_keys_are_redacted_after_separators() {
for raw in [
"key:sk-ant-abc123 failed",
"token=sk-ant-abc123",
"https://x.test/?k=sk-ant-abc123&y=1",
"\"sk-ant-abc123\"",
"ANTHROPIC_API_KEY=sk-ant-abc123,",
] {
let out = sanitize_cli_message(raw, None);
assert!(!out.contains("abc123"), "{raw} -> {out}");
assert!(out.contains("[redacted]"), "{raw} -> {out}");
}
assert_eq!(
sanitize_cli_message("task-force ask-me sk-", None),
"task-force ask-me sk-"
);
}

#[test]
fn configured_key_embedded_in_larger_token_is_redacted() {
let out = sanitize_cli_message("bad my-secret_suffix and pre-my-secret.", Some("my-secret"));
assert!(!out.contains("my-secret"), "{out}");
}

#[cfg(unix)]
#[tokio::test]
async fn zero_exit_is_error_result_is_sanitized_and_bounded() {
let big = "x".repeat(5_000);
let script = format!(
r#"echo '{{"type":"result","subtype":"success","is_error":true,"result":"bad key topsecret99 sk-ant-zzz {big}"}}'; exit 0"#
);
let err = run_fake_claude(&script, Some("topsecret99"))
.await
.expect_err("is_error must fail");
let text = err.to_string();
assert!(
!text.contains("topsecret99") && !text.contains("sk-ant-zzz"),
"{text}"
);
assert!(text.len() < 2_300, "len {}", text.len());
}

#[cfg(unix)]
#[tokio::test]
async fn nonzero_exit_stderr_fallback_is_redacted_and_bounded() {
let script = r#"echo "auth failed key=topsecret99 sk-ant-live-1234" >&2; head -c 5000 /dev/zero | tr '\0' x >&2; exit 1"#;
let err = run_fake_claude(script, Some("topsecret99"))
.await
.expect_err("nonzero exit must fail");
let text = err.to_string();
assert!(
!text.contains("topsecret99") && !text.contains("sk-ant-live-1234"),
"{text}"
);
assert!(text.len() < 2_300, "len {}", text.len());
}

#[cfg(unix)]
#[tokio::test]
async fn nonzero_exit_surfaces_object_error_message_sanitized() {
let err = run_fake_claude(
r#"echo '{"type":"error","error":{"message":"denied for topsecret99 sk-ant-xyz"}}'; exit 2"#,
Some("topsecret99"),
)
.await
.expect_err("nonzero exit must fail");
let text = err.to_string();
assert!(text.contains("denied for"), "{text}");
assert!(
!text.contains("topsecret99") && !text.contains("sk-ant-xyz"),
"{text}"
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ impl EventMapper {
subtype,
usage,
total_cost_usd,
..
raw,
} => {
let mut parsed = usage.as_ref().map(parse_usage);
// CC stream emits `total_cost_usd` on the terminal `result`
Expand All @@ -120,8 +120,20 @@ impl EventMapper {
usage.charged_amount_usd = cost;
}
self.usage = parsed;
if subtype.as_deref() == Some("error") && self.error.is_none() {
self.error = Some("claude reported `result.subtype=error`".into());
// A failed turn is reported as `subtype=error` or as
// `is_error=true` with the human-readable reason in `result`.
let failed = subtype.as_deref() == Some("error")
|| raw.get("is_error").and_then(Value::as_bool) == Some(true);
if failed && self.error.is_none() {
Comment thread
senamakel marked this conversation as resolved.
let reason = raw
.get("result")
.and_then(Value::as_str)
.map(str::trim)
.filter(|reason| !reason.is_empty());
Comment on lines +128 to +132

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Read failure details from the result errors array

When Claude returns a failed result with diagnostics only in errors—for example, error_during_execution or error_max_turns—this probes only result and replaces the real reason with the generic subtype message. The official ResultMessage contract exposes errors and specifically documents execution-error reasons there (Claude Agent SDK reference); on a nonzero exit, that generic value is then preferred over even a useful stderr message. Extract and sanitize the nonempty errors entries before using the generic fallback.

Useful? React with 👍 / 👎.

self.error = Some(match reason {
Comment thread
senamakel marked this conversation as resolved.
Some(reason) => reason.to_string(),
Comment thread
senamakel marked this conversation as resolved.
None => "claude reported `result.subtype=error`".into(),
});
}
self.finished = true;
Vec::new()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,3 +160,15 @@ fn final_assistant_message_is_skipped() {
});
assert!(deltas.is_empty());
}

#[test]
fn is_error_result_carries_its_reason_as_the_error() {
let mut m = EventMapper::new();
m.handle(ClaudeCodeEvent::Result {
subtype: Some("success".into()),
usage: None,
total_cost_usd: None,
raw: json!({"type":"result","subtype":"success","is_error":true,"result":"Invalid API key"}),
});
assert_eq!(m.error.as_deref(), Some("Invalid API key"));
}
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,11 @@ impl StreamJsonParser {
"error" => ClaudeCodeEvent::Error {
message: v
.get("error")
.and_then(Value::as_str)
.and_then(|e| {
e.as_str()
.or_else(|| e.get("message").and_then(Value::as_str))
})
.or_else(|| v.get("message").and_then(Value::as_str))
.unwrap_or("claude-code error")
.to_string(),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,3 +243,13 @@ fn feed_bytes_still_replaces_genuinely_invalid_bytes() {
"0xFF must decode to the replacement character, not vanish"
);
}

#[test]
fn error_event_with_object_payload_keeps_its_message() {
let mut parser = StreamJsonParser::new();
let events = parser.feed("{\"type\":\"error\",\"error\":{\"message\":\"rate limited\"}}\n");
assert!(matches!(
events.as_slice(),
[ClaudeCodeEvent::Error { message }] if message == "rate limited"
));
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,15 @@ const KNOWN_GENERIC_CLAUDE_CODE_CHAT_MESSAGE_DEBT: &[(&str, usize)] = &[
),
(
"crates/tinyagents-harness/src/providers/claude_code/driver.rs",
71,
135,
),
(
"crates/tinyagents-harness/src/providers/claude_code/driver.rs",
240,
304,
),
(
"crates/tinyagents-harness/src/providers/claude_code/driver_tests.rs",
363,
),
(
"crates/tinyagents-harness/src/providers/claude_code/input_builder.rs",
Expand Down
Loading