refactor(#2359): typed stream-json parsing for sentinel detection

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<String>,
+    /// Structured error payload (a string, or an object like
+    /// `{"type":"rate_limit_error"}`) present on some failures.
+    #[serde(default)]
+    error: Option<serde_json::Value>,
+    /// 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<u32>,
+}
+
 /// 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());
+        // Type the event instead of walking the `Value`. A parse failure
-        if matches!(ty, Some("assistant" | "user")) {
+        // (missing / non-string `type`) is rare and falls through to a raw
-            return;
+        // scan — the safe default, matching the old "unknown type" branch.
-        }
+        match StreamEvent::deserialize(event) {
-        // A `result` event's `result` field is model-authored answer text on a
+            // Model-authored output — its serialized content can quote any
-        // SUCCESSFUL turn — same trust level as an assistant message. A marker
+            // marker verbatim (an agent discussing this very code), so it is
-        // quoted there (the model explaining *this* code, or echoing an error
+            // never scanned. A genuine signal is emitted *instead of* a model
-        // string back) would falsely trip a sentinel and needlessly kill /
+            // turn, so it never rides an assistant/user event.
-        // downgrade the turn, so for a successful result we scrub `result`
+            Ok(StreamEvent::Assistant | StreamEvent::User) => {}
-        // before scanning and rely on the other (claude-authored) fields.
+
-        //
+            // The terminal result event. On success its `result` field is the
-        // On a FAILED result the same `result` field instead carries
+            // model's final answer (same trust level as an assistant message
-        // claude-code's OWN failure text, emitted *instead of* a model answer.
+            // — must NOT scan). On a FAILED result (`is_error`) the `result` /
-        // The real event (verified against captured stream-json) is:
+            // `error` fields carry claude-code's OWN failure text — the real
-        //   {"type":"result","is_error":true,"subtype":"success",
+            // event (verified against captured stream-json) is
-        //    "result":"Prompt is too long","terminal_reason":"blocking_limit"}
+            //   {"type":"result","is_error":true,"subtype":"success",
-        // Note `subtype` is "success" even on a hard failure — so `is_error` is
+            //    "result":"Prompt is too long","terminal_reason":"blocking_limit"}
-        // the only reliable discriminator, and the model can't forge it (the CLI
+            // — note `subtype` is "success" even on a hard failure, so
-        // sets it from the real outcome). Scan those raw. Scrubbing `result`
+            // `is_error` is the discriminator (the model can't forge it).
-        // unconditionally (the original fix) blinded prompt-too-long / auth
+            Ok(StreamEvent::Result(r)) => {
-        // detection. Other control events are claude-authored throughout, so
+                if r.is_error {
-        // they also scan raw.
+                    if let Some(text) = &r.result {
-        let error_result = ty == Some("result")
+                        self.scan_failure_markers(text);
-            && event.get("is_error").and_then(serde_json::Value::as_bool) == Some(true);
+                    }
-        if ty == Some("result") && !error_result {
+                    if let Some(err) = &r.error {
-            let mut scrubbed = event.clone();
+                        self.scan_failure_markers(&err.to_string());
-            if let Some(obj) = scrubbed.as_object_mut() {
+                    }
-                obj.remove("result");
+                    // 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 {
+            // A control `error` event — claude-authored end to end, and the
-            self.scan_failure_markers(raw);
+            // only place a rate-limit marker is trusted.
-        }
+            Ok(StreamEvent::Error) => {
-        // Rate-limit stays scoped to `error` events (unchanged): the only
+                self.scan_failure_markers(raw);
-        // control event that carries a rate-limit marker.
+                if RATE_LIMIT_MARKERS.iter().any(|m| raw.contains(m)) {
-        if ty == Some("error") && RATE_LIMIT_MARKERS.iter().any(|m| raw.contains(m)) {
+                    self.rate_limited.store(true, Ordering::Relaxed);
-            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