diff --git a/CHANGELOG.md b/CHANGELOG.md index c858c43..d723238 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,15 @@ already rendering events, check the first item. - `run()` parses a JSONL stream rather than one document. The `Outcome` is unchanged. Pin `Format::Json` to keep the old behaviour. +- **A turn the agent says failed is now an error, not an answer.** All three exit `0` for + some failures and put the explanation where the answer belongs: ask Claude for a model + that does not exist and it exits cleanly, reports `subtype: "success"`, and returns + "There's an issue with the selected model" as its reply. A caller checking `Result::is_ok` + rendered that as the model's answer. These are now `Error::AgentError`, carrying the + agent's own wording and the provider's status where one was given (typically 404 for an + unknown model). `NotAuthenticated` and `RateLimited` keep their own variants, since the + remedy differs; this covers the rest of that family. Codex forwards the upstream body as a + JSON *string*, so its message is unwrapped to the sentence and the status pulled out of it. - **An unauthenticated Copilot run is now reported as one.** Its wording shares no phrase with Claude's or Codex's, so it previously fell through to a generic `Error::Failed` with no login hint. Callers matching on `Error` should expect `NotAuthenticated` from Copilot now. @@ -51,6 +60,12 @@ already rendering events, check the first item. ### Fixed +- **A healthy quota heartbeat is no longer read as a refusal.** Every Claude `stream-json` + run prints a `rate_limit_event` record, and on a healthy run its status is `allowed`. + Classification scanned the raw stream for the substring `rate_limit`, which that record + contains, so any Claude failure came back as `Error::RateLimited`: a caller was told to + back off when the real cause was one they could fix. The parsed signal and the agent's own + prose now decide, and the raw scan is only the fallback for output that produced neither. - **A failure reports its cause rather than a status line.** The first line of stderr was taken as the explanation, but CLIs open with progress chatter: a rejected Codex schema reported "Reading additional input from stdin...", which is not what went wrong. A line that looks @@ -60,22 +75,14 @@ already rendering events, check the first item. ## 0.2.0 Nothing was removed or renamed, so this compiles against any 0.1 code. It is a minor bump -rather than a patch because two changes alter behaviour, and `"0.1"` would have delivered +rather than a patch because some changes alter behaviour, and `"0.1"` would have delivered them through an ordinary `cargo update`. -### Behaviour changes +*Corrected after release: this entry originally also credited 0.2.0 with streaming by default +and token-by-token Claude text. Those landed three hours after 0.2.0 was published and ship in +0.2.1, where they are now listed. The published 0.2.0 does not have them.* -- **Streaming is now the default**, and a named session no longer disables it. `Format` - defaulted to `Json`, under which nothing is observable until the turn ends, so a caller - watching a twenty-minute run saw nothing for twenty minutes. Worse, `Request::session` - *pinned* that format, meaning the multi-turn path a chat UI always uses could not stream - at all. `Stream` carries everything `Json` does, so the default costs only parsing, and - `session()` now validates the format instead of overriding it. Pin `Format::Json` - explicitly if you only want the answer. -- **Claude streams token by token.** `--include-partial-messages` is passed under `Stream`, - without which Claude emits only completed messages and text arrives a paragraph at a time. - Claude sends both the deltas and the finished message; only the deltas are emitted, so a - transcript does not show every answer twice. +### Behaviour changes - **An unauthenticated run is now an error.** Claude exits `0` and puts "Not logged in · Please run /login" in its result text, so a missing login previously came diff --git a/README.md b/README.md index 8fc3ef9..85bfce6 100644 --- a/README.md +++ b/README.md @@ -295,6 +295,10 @@ own variables do not reach the child. as a positional and the deny is silently lost. This crate always emits the combined form. - **Copilot needs `--allow-all-tools` to run headlessly at all**, or it stalls at the first tool confirmation. It is always emitted; `Permission` then narrows via denies. +- **A failed turn still exits 0.** Ask Claude for a model that does not exist and it exits + cleanly, reports `subtype: "success"`, and puts "There's an issue with the selected model" + where the answer belongs. Codex does the same and wraps the upstream body in a JSON string. + Both come back as `Error::AgentError`; see below. - **Claude's `stream-json` requires `--verbose`**, or it refuses to start. Handled. - **Large prompts move to stdin automatically** above 128 KiB, so a long prompt never fails with `E2BIG`. @@ -334,6 +338,35 @@ Under a structured format there is no silent fallback to raw stdout: a run that recognizable records, or never reached its terminal record, returns `Error::Parse` rather than a plausible-looking answer assembled from whatever was printed. +## A clean exit is not a successful turn + +All three agents report some failures with exit code 0, with the explanation where the answer +should be. An unknown model, a rejected schema and an upstream outage all arrive this way, so +a caller checking only `Result::is_ok` renders the error as the model's reply. + +These are `Error::AgentError`, carrying the agent's own wording and the provider's status +where one was reported: + +```rust +match run(&request).await { + Err(Error::AgentError { status: Some(404), message, .. }) => { + // Typically a model the account cannot reach. `message` is the agent's wording. + eprintln!("{message}"); + } + Err(e) if e.is_auth_failure() => { /* prompt for login */ } + Err(e) if e.is_transient() => { /* back off */ } + other => { /* ... */ } +} +``` + +`NotAuthenticated` and `RateLimited` are the two members of this family that predate it and +keep their own variants, because the remedy differs. Everything else in it lands here. + +Codex needs one extra step: it forwards the provider's response body as a *string*, so its +`turn.failed` message is JSON containing the sentence and the status. This crate unwraps it, +so `message` is the sentence and `status` is the code. A message that is not that shape is +passed through untouched. + ## No shell, ever Arguments are built as a `Vec` and passed straight to `exec`. There is no shell in diff --git a/docs/operating-limits.md b/docs/operating-limits.md index f9cd213..45f07aa 100644 --- a/docs/operating-limits.md +++ b/docs/operating-limits.md @@ -15,7 +15,9 @@ of that line, so a future change does not quietly cross it. unedited, and surfaces Claude's `rate_limit_event` as an ordinary `Event::RateLimit` during a run. - **Passes model names through verbatim.** An unknown model is the provider's error to - raise, not this crate's to guess around. + raise, not this crate's to guess around. It is reported as [`Error::AgentError`] with the + provider's status, since the agents answer an unknown model with a clean exit and an + explanation where the answer belongs. ## What this crate deliberately does not do diff --git a/src/error.rs b/src/error.rs index 3225d6a..dd3bac9 100644 --- a/src/error.rs +++ b/src/error.rs @@ -143,6 +143,31 @@ pub enum Error { hint: &'static str, }, + /// The agent ran, exited cleanly, and reported that the turn itself failed. + /// + /// Its own variant because the process succeeding says nothing about the + /// turn succeeding. An unknown model, a schema the provider rejects or an + /// upstream outage all arrive this way: exit code 0, with the failure + /// described in the output. Reporting that as `Ok` hands back an + /// [`crate::Outcome`] whose `text` is an error message, which a caller + /// checking only `Result::is_ok` will render as the answer. + /// + /// The same reasoning already applied to [`Error::NotAuthenticated`] and + /// [`Error::RateLimited`], which are also reported with a zero exit; this + /// covers everything else in that family. + #[error("{agent} reported a failed turn{}: {message}", status.map(|s| format!(" (status {s})")).unwrap_or_default())] + AgentError { + /// The agent that failed. + agent: Agent, + /// The binary that ran. + bin: String, + /// The provider's status code, where the agent reported one. A 404 is + /// typically an unknown model, a 400 a rejected request. + status: Option, + /// The agent's own description, unedited. + message: String, + }, + /// The CLI rejected an argument this crate passed it. /// /// Almost always a version mismatch: the flag was verified against the diff --git a/src/event.rs b/src/event.rs index 9f32379..c848b8b 100644 --- a/src/event.rs +++ b/src/event.rs @@ -274,6 +274,36 @@ pub struct Terminal { pub first_unparsed: Option, /// The schema-conforming answer, where the agent reports one separately. pub structured: Option, + /// The provider status code when the agent reported a failed turn, such as + /// a 404 for an unknown model. + pub error_status: Option, + /// The agent's own description of a failed turn, where it gives one apart + /// from the answer text. Claude puts its explanation in `result`, so this + /// stays `None` there; Codex reports it under `turn.failed`. + pub error_message: Option, +} + +/// Unwrap an error body an agent passed through as a JSON string. +/// +/// Codex 0.145.0 forwards the upstream response verbatim, so `turn.failed` +/// carries `{"type":"error","status":400,"error":{"message":"..."}}` encoded as +/// a *string*. Showing that to a user means showing them JSON, and the status +/// worth branching on is buried inside it. Anything that is not that shape is +/// returned as-is. +fn unwrap_error_body(message: &str) -> (Option, String) { + let Ok(body) = serde_json::from_str::(message) else { + return (None, message.to_string()); + }; + let status = body + .get("status") + .and_then(Value::as_u64) + .and_then(|s| u16::try_from(s).ok()); + let inner = body + .get("error") + .and_then(|e| e.get("message")) + .and_then(Value::as_str) + .map(str::to_string); + (status, inner.unwrap_or_else(|| message.to_string())) } /// Incrementally turns one agent's output into [`Event`]s and a [`Terminal`]. @@ -507,7 +537,13 @@ impl Parser { self.term.structured = Some(value.clone()); } self.term.usage = claude_usage(v); + // `subtype` says "success" even for a failed turn, so + // `is_error` is the field that actually decides. self.term.stop = if v.get("is_error").and_then(Value::as_bool) == Some(true) { + self.term.error_status = v + .get("api_error_status") + .and_then(Value::as_u64) + .and_then(|s| u16::try_from(s).ok()); Stop::Error } else { stop_from(v.get("stop_reason")) @@ -651,6 +687,15 @@ impl Parser { "turn.failed" => { self.seen.terminal = true; self.term.stop = Stop::Error; + if let Some(message) = v + .get("error") + .and_then(|e| e.get("message")) + .and_then(Value::as_str) + { + let (status, message) = unwrap_error_body(message); + self.term.error_status = status; + self.term.error_message = Some(bound_text(message)); + } Vec::new() } "item.started" | "item.updated" | "item.completed" => { @@ -796,8 +841,13 @@ impl Parser { self.term.usage.premium_requests = usage.get("premiumRequests").and_then(Value::as_u64); } - if v.get("exitCode").and_then(Value::as_i64).unwrap_or(0) != 0 { + if let Some(code) = v.get("exitCode").and_then(Value::as_i64) + && code != 0 + { self.term.stop = Stop::Error; + // Copilot reports no explanation with the code, so name the + // code rather than leave the failure blank. + self.term.error_message = Some(format!("copilot exited with code {code}")); } Vec::new() } @@ -1151,6 +1201,46 @@ mod tests { ); } + /// Verbatim from codex-cli 0.145.0 with an unknown model. It exits **0** + /// and forwards the upstream body as a JSON *string*, so the status worth + /// branching on is nested one level inside a field that is itself text. + #[test] + fn a_codex_failed_turn_yields_the_reason_and_the_status() { + let (_, term) = run( + Agent::Codex, + &[ + r#"{"type":"thread.started","thread_id":"019fad62"}"#, + r#"{"type":"turn.failed","error":{"message":"{\"type\":\"error\",\"status\":400,\"error\":{\"type\":\"invalid_request_error\",\"message\":\"The 'bogus-model-xyz' model is not supported when using Codex with a ChatGPT account.\"}}"}}"#, + ], + ); + assert_eq!(term.stop, Stop::Error); + assert_eq!(term.error_status, Some(400)); + assert_eq!( + term.error_message.as_deref(), + Some( + "The 'bogus-model-xyz' model is not supported when using Codex with a ChatGPT account." + ), + "the caller should get the sentence, not the envelope" + ); + } + + /// An error that is not the double-encoded shape must survive untouched + /// rather than be dropped for failing to match it. + #[test] + fn a_plain_codex_failure_message_passes_through() { + let (_, term) = run( + Agent::Codex, + &[ + r#"{"type":"turn.failed","error":{"message":"stream disconnected before completion"}}"#, + ], + ); + assert_eq!(term.error_status, None); + assert_eq!( + term.error_message.as_deref(), + Some("stream disconnected before completion") + ); + } + #[test] fn codex_reads_the_thread_id_and_the_completed_message() { let (events, term) = run( diff --git a/src/run.rs b/src/run.rs index 7de0363..4bade29 100644 --- a/src/run.rs +++ b/src/run.rs @@ -659,7 +659,11 @@ async fn drive( // result text, so checking only the exit code would hand back a successful // Outcome whose answer is "Please run /login". let unauthenticated = looks_unauthenticated(&terminal.text); - if exit_code != 0 || quota_blocked || unauthenticated { + // The agent saying its turn failed is as much a failure as a non-zero exit, + // and Claude reports an unknown model exactly this way: exit 0, `is_error` + // true, and the explanation where the answer would be. + let turn_failed = terminal.stop == Stop::Error; + if exit_code != 0 || quota_blocked || unauthenticated || turn_failed { return Err(classify_run( request.agent, &bin, @@ -740,7 +744,7 @@ fn classify_run( }; } } - classify(bin, code, stderr, stdout, terminal) + classify(agent, bin, code, stderr, stdout, terminal) } /// Whether text is an agent saying it has no usable credentials. @@ -771,16 +775,34 @@ fn looks_unauthenticated(text: &str) -> bool { } /// Turn a non-zero exit into the most specific error available. -fn classify(bin: &str, code: i32, stderr: &str, stdout: &str, terminal: &Terminal) -> Error { +fn classify( + agent: crate::Agent, + bin: &str, + code: i32, + stderr: &str, + stdout: &str, + terminal: &Terminal, +) -> Error { let quota_signalled = terminal .rate_limit .as_ref() .is_some_and(crate::outcome::RateLimit::is_blocking); - if quota_signalled || looks_rate_limited(stderr) || looks_rate_limited(stdout) { + // Scanning the *raw* stream for quota wording is a false-positive machine: + // under `stream-json` Claude prints a `rate_limit_event` record on every + // run, including one whose status is `allowed`, so the substring + // `rate_limit` is present in perfectly healthy output. Where the stream + // parsed, the parsed signal and the agent's own prose decide; the raw scan + // is only the fallback for output that produced neither. + let prose = match (&terminal.error_message, terminal.text.as_str()) { + (Some(message), text) => format!("{message}\n{text}"), + (None, text) if !text.is_empty() => text.to_string(), + _ => stdout.to_string(), + }; + if quota_signalled || looks_rate_limited(stderr) || looks_rate_limited(&prose) { return Error::RateLimited { bin: bin.to_string(), message: first_meaningful_line(stderr) - .or_else(|| first_meaningful_line(stdout)) + .or_else(|| first_meaningful_line(&prose)) .unwrap_or_else(|| "usage limit reached".to_string()), }; } @@ -793,6 +815,24 @@ fn classify(bin: &str, code: i32, stderr: &str, stdout: &str, terminal: &Termina detail, }; } + // Checked before the generic failure but after quota and a rejected flag, + // which are more specific readings of the same output. + if terminal.stop == Stop::Error { + return Error::AgentError { + agent, + bin: bin.to_string(), + status: terminal.error_status, + // Codex reports the reason apart from the answer; Claude puts it + // where the answer would be. + message: terminal + .error_message + .clone() + .or_else(|| first_meaningful_line(&terminal.text)) + .or_else(|| first_meaningful_line(stderr)) + .unwrap_or_else(|| "the agent reported a failure without explaining it".into()), + }; + } + Error::Failed { bin: bin.to_string(), code, @@ -959,7 +999,7 @@ mod tests { ..Terminal::default() }; assert!(matches!( - classify("claude", 1, "", "", &terminal), + classify(Agent::Claude, "claude", 1, "", "", &terminal), Error::RateLimited { .. } )); } @@ -975,11 +1015,116 @@ mod tests { ..Terminal::default() }; assert!(matches!( - classify("claude", 1, "boom", "", &terminal), + classify(Agent::Claude, "claude", 1, "boom", "", &terminal), Error::Failed { .. } )); } + /// Verbatim from a healthy claude 2.1.205 run. Every `stream-json` run + /// carries this record, and its status is `allowed`: nothing is refused. + /// Scanning the raw stream for `rate_limit` matched it anyway, so any + /// Claude failure was reported as a quota refusal, sending a caller to back + /// off when the real cause was something they could fix. + #[test] + fn a_healthy_rate_limit_heartbeat_is_not_a_refusal() { + let stdout = r#"{"type":"rate_limit_event","rate_limit_info":{"status":"allowed","resetsAt":1785331800,"rateLimitType":"five_hour","overageStatus":"rejected","isUsingOverage":false}}"#; + let terminal = Terminal { + stop: Stop::Error, + error_status: Some(404), + text: "There's an issue with the selected model (bogus-model-xyz).".into(), + rate_limit: Some(crate::outcome::RateLimit { + status: "allowed".into(), + window: Some("five_hour".into()), + resets_at: Some(1_785_331_800), + }), + ..Terminal::default() + }; + let err = classify_run(Agent::Claude, "claude", 0, "", stdout, &terminal); + assert!( + matches!(err, Error::AgentError { .. }), + "the heartbeat must not mask the real cause: {err:?}" + ); + } + + /// The counterpart: a refusal the parser did read must still be one, even + /// though it arrives with the same zero exit code. + #[test] + fn a_rejected_quota_signal_is_still_a_refusal() { + let terminal = Terminal { + rate_limit: Some(crate::outcome::RateLimit { + status: "rejected".into(), + window: Some("five_hour".into()), + resets_at: None, + }), + ..Terminal::default() + }; + assert!(matches!( + classify_run(Agent::Claude, "claude", 0, "", "", &terminal), + Error::RateLimited { .. } + )); + } + + /// Verbatim from a real run with an unknown model. Claude exits **0** with + /// `subtype: "success"` while `is_error` is true and the explanation sits + /// where the answer would be, so a caller checking only `Result::is_ok` + /// renders "There's an issue with the selected model" as the answer. + #[test] + fn a_failed_turn_is_an_error_even_though_the_process_exited_cleanly() { + let terminal = Terminal { + stop: Stop::Error, + error_status: Some(404), + text: "There's an issue with the selected model (bogus-model-xyz). \ + It may not exist or you may not have access to it." + .into(), + ..Terminal::default() + }; + let err = classify_run(Agent::Claude, "claude", 0, "", "", &terminal); + let Error::AgentError { + agent, + status, + message, + .. + } = &err + else { + panic!("expected AgentError, got {err:?}") + }; + assert_eq!(*agent, Agent::Claude); + assert_eq!(*status, Some(404), "the provider status must survive"); + assert!(message.contains("selected model"), "{message}"); + } + + /// A quota refusal and a missing login are more specific readings of the + /// same shape, so they must not be swallowed by the general case. + #[test] + fn a_failed_turn_does_not_mask_a_more_specific_cause() { + let auth = Terminal { + stop: Stop::Error, + text: "Not logged in · Please run /login".into(), + ..Terminal::default() + }; + assert!( + classify_run(Agent::Claude, "claude", 0, "", "", &auth).is_auth_failure(), + "an unauthenticated failed turn must stay an auth failure" + ); + + let quota = Terminal { + stop: Stop::Error, + rate_limit: Some(crate::outcome::RateLimit { + status: "rejected".into(), + window: None, + resets_at: None, + }), + ..Terminal::default() + }; + assert!( + matches!( + classify_run(Agent::Claude, "claude", 0, "", "", "a), + Error::RateLimited { .. } + ), + "a quota-blocked failed turn must stay a rate limit" + ); + } + /// Verified against the real CLI: with `USER` withheld, claude answers /// "Not logged in · Please run /login" and exits **0**. Checking only the /// exit code hands back a successful Outcome whose answer is a login @@ -1075,6 +1220,7 @@ mod tests { #[test] fn a_rejected_flag_is_named_as_a_version_mismatch() { let err = classify( + Agent::Codex, "codex", 2, "error: unexpected argument '--sandbox' found", @@ -1097,7 +1243,7 @@ mod tests { ] { assert!( matches!( - classify("codex", 1, stderr, "", &Terminal::default()), + classify(Agent::Codex, "codex", 1, stderr, "", &Terminal::default()), Error::Failed { .. } ), "{stderr:?} should stay a plain failure" @@ -1152,6 +1298,7 @@ mod tests { #[test] fn failures_report_the_first_useful_line() { let err = classify( + Agent::Claude, "claude", 2, "\n\n real problem \nstack", diff --git a/tests/live.rs b/tests/live.rs index c17be6b..a347fd6 100644 --- a/tests/live.rs +++ b/tests/live.rs @@ -61,6 +61,68 @@ async fn claude_answers_and_reports_usage() { assert!(outcome.usage.cost_usd.is_some(), "{:?}", outcome.usage); } +/// The failure this crate most has to get right: claude exits **0** for an +/// unknown model and puts the explanation where the answer goes. Before this +/// was classified, a caller checking `Result::is_ok` rendered "There's an issue +/// with the selected model" as the model's reply. +/// +/// Cheap: the turn is refused before any tokens are spent. +#[tokio::test] +#[ignore = "spawns a real agent"] +async fn an_unknown_model_is_an_error_not_an_answer() { + if !available(Agent::Claude) { + return; + } + let request = ping(Agent::Claude).model("bogus-model-xyz"); + let err = run(&request) + .await + .expect_err("an unknown model must not come back as an answer"); + + let agent_abstraction::Error::AgentError { + status, message, .. + } = &err + else { + panic!("expected AgentError, got {err:?}") + }; + // Not asserted as exactly 404: the status is passed through from the + // provider, and the point is that whatever it sends survives. + assert!( + status.is_some(), + "the provider status should survive: {err:?}" + ); + assert!( + message.to_lowercase().contains("model"), + "the agent's own wording should survive: {message}" + ); +} + +/// Codex fails the same way and adds a wrinkle: it forwards the upstream error +/// body as a JSON string, so without unwrapping, the caller is handed JSON +/// instead of a sentence. +#[tokio::test] +#[ignore = "spawns a real agent"] +async fn codex_reports_an_unknown_model_as_a_sentence_not_json() { + if !available(Agent::Codex) { + return; + } + let request = ping(Agent::Codex).model("bogus-model-xyz"); + let err = run(&request) + .await + .expect_err("an unknown model must not come back as an answer"); + + let agent_abstraction::Error::AgentError { message, .. } = &err else { + panic!("expected AgentError, got {err:?}") + }; + assert!( + !message.trim_start().starts_with('{'), + "the upstream envelope should have been unwrapped: {message}" + ); + assert!( + message.to_lowercase().contains("model"), + "the agent's own wording should survive: {message}" + ); +} + #[tokio::test] #[ignore = "spawns a real agent and consumes quota"] async fn codex_answers_and_reports_usage() {