diff --git a/hive-claude/src/classify.rs b/hive-claude/src/classify.rs index 45a82e20..461a2bff 100644 --- a/hive-claude/src/classify.rs +++ b/hive-claude/src/classify.rs @@ -5,9 +5,18 @@ //! are empirically stable across CLI versions; if one drifts the run degrades //! gracefully (a clean turn, or a hard [`crate::Error::Exit`] on a non-zero //! exit) rather than misbehaving. +//! +//! Stdout events are parsed into a typed [`StreamEvent`] (a serde-tagged enum) +//! rather than walked as a `serde_json::Value` — the event-type dispatch and +//! the terminal result event's fields (`is_error`, `api_error_status`) are +//! typed, so a shape drift is a clean fallback instead of a silent +//! mis-detection. The marker *strings* still gate detection (they're the +//! API's error text); typing removes the fragile field-probing around them. use std::sync::atomic::{AtomicBool, Ordering}; +use serde::Deserialize; + /// Emitted when the prompt/context exceeds the model's window. const PROMPT_TOO_LONG_MARKER: &str = "Prompt is too long"; @@ -39,6 +48,53 @@ const SESSION_NOT_FOUND_MARKERS: [&str; 2] = [ "No conversation found with session ID", ]; +/// The claude-code stdout stream-json events we classify, internally tagged +/// on `type`. Typing the dispatch (and the `result` event's fields below) +/// replaces walking a `serde_json::Value` with `get("field").and_then(...)`, +/// so a field-shape drift is a compile error / a clean fallback rather than a +/// silent mis-detection. Unknown event types fall into [`StreamEvent::Other`] +/// (scanned raw like any other control event). +#[derive(serde::Deserialize)] +#[serde(tag = "type", rename_all = "snake_case")] +enum StreamEvent { + /// Model-authored turn output — never scanned (it can quote any marker). + Assistant, + /// Model-authored tool results — never scanned. + User, + /// The terminal result event (see [`ResultEvent`]). + Result(ResultEvent), + /// A control `error` event — the only event a rate-limit marker is + /// trusted on (a model *discussing* a rate limit can't forge this type). + Error, + /// `system` and any other/unknown control event — claude-authored, so + /// scanned raw for the failure markers. + #[serde(other)] + Other, +} + +/// The terminal `result` event's fields we act on. `subtype` is deliberately +/// absent: it reads `"success"` even on a hard failure, so it's not a usable +/// error signal — `is_error` is the discriminator (the CLI sets it from the +/// real outcome; the model can't forge it). On `is_error` the `result` / +/// `error` fields carry claude-code's own failure text. +#[derive(serde::Deserialize)] +struct ResultEvent { + #[serde(default)] + is_error: bool, + /// claude-code's failure text on `is_error` (a genuine "Prompt is too + /// long" / auth message); the model's final answer on success. + #[serde(default)] + result: Option, + /// Structured error payload (a string, or an object like + /// `{"type":"rate_limit_error"}`) present on some failures. + #[serde(default)] + error: Option, + /// API status on an auth failure (e.g. `401`) — a typed signal that + /// doesn't depend on the human-readable string landing in `result`. + #[serde(default)] + api_error_status: Option, +} + /// Shared, lock-free sentinel flags accumulated while both output streams are /// pumped concurrently. Read once after the child exits. #[derive(Default)] @@ -71,43 +127,52 @@ impl Sentinels { /// an assistant/user message. This holds whatever exact shape claude-code /// uses for the message, so the gate can't suppress a genuine signal. pub(crate) fn scan_stdout_json(&self, event: &serde_json::Value, raw: &str) { - let ty = event.get("type").and_then(|t| t.as_str()); - if matches!(ty, Some("assistant" | "user")) { - return; - } - // A `result` event's `result` field is model-authored answer text on a - // SUCCESSFUL turn — same trust level as an assistant message. A marker - // quoted there (the model explaining *this* code, or echoing an error - // string back) would falsely trip a sentinel and needlessly kill / - // downgrade the turn, so for a successful result we scrub `result` - // before scanning and rely on the other (claude-authored) fields. - // - // On a FAILED result the same `result` field instead carries - // claude-code's OWN failure text, emitted *instead of* a model answer. - // The real event (verified against captured stream-json) is: - // {"type":"result","is_error":true,"subtype":"success", - // "result":"Prompt is too long","terminal_reason":"blocking_limit"} - // Note `subtype` is "success" even on a hard failure — so `is_error` is - // the only reliable discriminator, and the model can't forge it (the CLI - // sets it from the real outcome). Scan those raw. Scrubbing `result` - // unconditionally (the original fix) blinded prompt-too-long / auth - // detection. Other control events are claude-authored throughout, so - // they also scan raw. - let error_result = ty == Some("result") - && event.get("is_error").and_then(serde_json::Value::as_bool) == Some(true); - if ty == Some("result") && !error_result { - let mut scrubbed = event.clone(); - if let Some(obj) = scrubbed.as_object_mut() { - obj.remove("result"); + // Type the event instead of walking the `Value`. A parse failure + // (missing / non-string `type`) is rare and falls through to a raw + // scan — the safe default, matching the old "unknown type" branch. + match StreamEvent::deserialize(event) { + // Model-authored output — its serialized content can quote any + // marker verbatim (an agent discussing this very code), so it is + // never scanned. A genuine signal is emitted *instead of* a model + // turn, so it never rides an assistant/user event. + Ok(StreamEvent::Assistant | StreamEvent::User) => {} + + // The terminal result event. On success its `result` field is the + // model's final answer (same trust level as an assistant message + // — must NOT scan). On a FAILED result (`is_error`) the `result` / + // `error` fields carry claude-code's OWN failure text — the real + // event (verified against captured stream-json) is + // {"type":"result","is_error":true,"subtype":"success", + // "result":"Prompt is too long","terminal_reason":"blocking_limit"} + // — note `subtype` is "success" even on a hard failure, so + // `is_error` is the discriminator (the model can't forge it). + Ok(StreamEvent::Result(r)) => { + if r.is_error { + if let Some(text) = &r.result { + self.scan_failure_markers(text); + } + if let Some(err) = &r.error { + self.scan_failure_markers(&err.to_string()); + } + // Typed 401: an auth failure that set the status code but + // may not have put the human-readable string in `result`. + if r.api_error_status == Some(401) { + self.auth_failed.store(true, Ordering::Relaxed); + } + } } - self.scan_failure_markers(&scrubbed.to_string()); - } else { - self.scan_failure_markers(raw); - } - // Rate-limit stays scoped to `error` events (unchanged): the only - // control event that carries a rate-limit marker. - if ty == Some("error") && RATE_LIMIT_MARKERS.iter().any(|m| raw.contains(m)) { - self.rate_limited.store(true, Ordering::Relaxed); + + // A control `error` event — claude-authored end to end, and the + // only place a rate-limit marker is trusted. + Ok(StreamEvent::Error) => { + self.scan_failure_markers(raw); + if RATE_LIMIT_MARKERS.iter().any(|m| raw.contains(m)) { + self.rate_limited.store(true, Ordering::Relaxed); + } + } + + // `system` / unknown control events (claude-authored) → scan raw. + Ok(StreamEvent::Other) | Err(_) => self.scan_failure_markers(raw), } } @@ -183,6 +248,28 @@ mod tests { assert!(matches!(s.soft_error(), Some(Error::AuthFailed))); } + #[test] + fn typed_api_error_status_401_trips_auth_without_string_marker() { + // The typed `api_error_status` field catches an auth failure even when + // the human-readable 401 string isn't in `result` — a signal the old + // substring scan would have missed. + let s = Sentinels::default(); + let raw = r#"{"type":"result","is_error":true,"subtype":"success","result":"request failed","api_error_status":401}"#; + s.scan_stdout_json(&json(raw), raw); + assert!(matches!(s.soft_error(), Some(Error::AuthFailed))); + } + + #[test] + fn error_payload_object_on_result_is_scanned() { + // A failure result can carry the marker in a structured `error` + // payload rather than `result`; the typed `error: Value` is stringified + // and scanned. + let s = Sentinels::default(); + let raw = r#"{"type":"result","is_error":true,"error":"Prompt is too long","result":"ok"}"#; + s.scan_stdout_json(&json(raw), raw); + assert!(matches!(s.soft_error(), Some(Error::PromptTooLong))); + } + #[test] fn result_field_model_text_quoting_marker_is_ignored() { // The terminal `result` event's `result` field is the model's final