diff --git a/crates/tui/src/commands/session.rs b/crates/tui/src/commands/session.rs index 97e3b02f..ca92827f 100644 --- a/crates/tui/src/commands/session.rs +++ b/crates/tui/src/commands/session.rs @@ -155,6 +155,11 @@ pub fn export(app: &mut App, path: Option<&str>) -> CommandResult { HistoryCell::User { content } => ("**You:**", content.clone()), HistoryCell::Assistant { content, .. } => ("**Assistant:**", content.clone()), HistoryCell::System { content } => ("*System:*", content.clone()), + HistoryCell::Error { message, severity } => match severity { + crate::error_taxonomy::ErrorSeverity::Warning => ("**Warning:**", message.clone()), + crate::error_taxonomy::ErrorSeverity::Info => ("*Info:*", message.clone()), + _ => ("**Error:**", message.clone()), + }, HistoryCell::Thinking { content, .. } => ("*Thinking:*", content.clone()), HistoryCell::Tool(tool) => ("**Tool:**", render_tool_cell(tool, 80)), HistoryCell::SubAgent(sub) => ("**Sub-agent:**", render_subagent_cell(sub, 80)), diff --git a/crates/tui/src/compaction.rs b/crates/tui/src/compaction.rs index f85b69ba..720ffc88 100644 --- a/crates/tui/src/compaction.rs +++ b/crates/tui/src/compaction.rs @@ -659,19 +659,17 @@ pub struct CompactionResult { pub retries_used: u32, } -/// Check if an error is transient and worth retrying. +/// Check if an error is transient and worth retrying. Categories that map to +/// transient retry: Network, RateLimit, Timeout. Anything else (auth, parse, +/// invalid request, etc.) is permanent and propagates. fn is_transient_error(e: &anyhow::Error) -> bool { - let msg = e.to_string().to_lowercase(); - msg.contains("timeout") - || msg.contains("timed out") - || msg.contains("connection") - || msg.contains("rate limit") - || msg.contains("too many requests") - || msg.contains("503") - || msg.contains("502") - || msg.contains("429") - || msg.contains("network") - || msg.contains("temporarily unavailable") + let category = crate::error_taxonomy::classify_error_message(&e.to_string()); + matches!( + category, + crate::error_taxonomy::ErrorCategory::Network + | crate::error_taxonomy::ErrorCategory::RateLimit + | crate::error_taxonomy::ErrorCategory::Timeout + ) } /// Compact messages with retry and backoff for transient errors. diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index 67eb9e86..208f5a9d 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -28,6 +28,7 @@ use crate::cycle_manager::{ CycleBriefing, CycleConfig, StructuredState, archive_cycle, build_seed_messages, estimate_briefing_tokens, produce_briefing, should_advance_cycle, }; +use crate::error_taxonomy::{ErrorCategory, ErrorEnvelope, StreamError}; use crate::features::{Feature, Features}; use crate::llm_client::LlmClient; use crate::mcp::McpPool; @@ -1101,8 +1102,7 @@ fn context_input_budget(model: &str, requested_output_tokens: u32) -> Option bool { - crate::error_taxonomy::classify_error_message(message) - == crate::error_taxonomy::ErrorCategory::InvalidInput + crate::error_taxonomy::classify_error_message(message) == ErrorCategory::InvalidInput } fn emit_tool_audit(event: serde_json::Value) { @@ -1259,7 +1259,10 @@ impl Engine { .unwrap_or_else(|| { "Failed to spawn sub-agent: API client not configured".to_string() }); - let _ = self.tx_event.send(Event::error(message, false)).await; + let _ = self + .tx_event + .send(Event::error(ErrorEnvelope::fatal(message))) + .await; continue; }; @@ -1298,10 +1301,9 @@ impl Engine { Err(err) => { let _ = self .tx_event - .send(Event::error( - format!("Failed to spawn sub-agent: {err}"), - false, - )) + .send(Event::error(ErrorEnvelope::fatal(format!( + "Failed to spawn sub-agent: {err}" + )))) .await; } } @@ -1446,7 +1448,7 @@ impl Engine { .unwrap_or_else(|| "Failed to send message: API client not configured".to_string()); let _ = self .tx_event - .send(Event::error(message.clone(), false)) + .send(Event::error(ErrorEnvelope::fatal_auth(message.clone()))) .await; let _ = self .tx_event @@ -1673,7 +1675,7 @@ impl Engine { .await; let _ = self .tx_event - .send(Event::error(message.clone(), false)) + .send(Event::error(ErrorEnvelope::fatal_auth(message.clone()))) .await; let _ = self .tx_event @@ -1786,7 +1788,9 @@ impl Engine { .unwrap_or_else(|| "API client not configured".to_string()); let _ = self .tx_event - .send(Event::error(format!("RLM error: {err}"), false)) + .send(Event::error(ErrorEnvelope::fatal_auth(format!( + "RLM error: {err}" + )))) .await; return; }; @@ -1810,7 +1814,9 @@ impl Engine { if let Some(ref err) = result.error { let _ = self .tx_event - .send(Event::error(format!("RLM error: {err}"), true)) + .send(Event::error(ErrorEnvelope::tool(format!( + "RLM error: {err}" + )))) .await; } @@ -2385,7 +2391,10 @@ impl Engine { MAX_CONTEXT_RECOVERY_ATTEMPTS, estimated_input, input_budget ); turn_error = Some(message.clone()); - let _ = self.tx_event.send(Event::error(message, true)).await; + let _ = self + .tx_event + .send(Event::error(ErrorEnvelope::context_overflow(message))) + .await; return (TurnOutcomeStatus::Failed, turn_error); } @@ -2459,7 +2468,10 @@ impl Engine { continue; } turn_error = Some(message.clone()); - let _ = self.tx_event.send(Event::error(message, true)).await; + let _ = self + .tx_event + .send(Event::error(ErrorEnvelope::classify(message, true))) + .await; return (TurnOutcomeStatus::Failed, turn_error); } }; @@ -2511,12 +2523,12 @@ impl Engine { Ok(Some(event_result)) => Some(event_result), Ok(None) => None, // stream ended normally Err(_) => { - let msg = format!( - "Stream stalled: no data received for {}s, closing stream", - STREAM_CHUNK_TIMEOUT_SECS, - ); - crate::logging::warn(&msg); - let _ = self.tx_event.send(Event::error(msg, true)).await; + let envelope = StreamError::Stall { + timeout_secs: STREAM_CHUNK_TIMEOUT_SECS, + } + .into_envelope(); + crate::logging::warn(&envelope.message); + let _ = self.tx_event.send(Event::error(envelope)).await; None } } @@ -2546,25 +2558,25 @@ impl Engine { // Guard: max wall-clock duration if stream_start.elapsed() > max_duration { - let msg = format!( - "Stream exceeded maximum duration of {}s, closing", - STREAM_MAX_DURATION_SECS, - ); - crate::logging::warn(&msg); - turn_error.get_or_insert(msg.clone()); - let _ = self.tx_event.send(Event::error(msg, true)).await; + let envelope = StreamError::DurationLimit { + limit_secs: STREAM_MAX_DURATION_SECS, + } + .into_envelope(); + crate::logging::warn(&envelope.message); + turn_error.get_or_insert(envelope.message.clone()); + let _ = self.tx_event.send(Event::error(envelope)).await; break; } // Guard: max accumulated content bytes if stream_content_bytes > STREAM_MAX_CONTENT_BYTES { - let msg = format!( - "Stream exceeded maximum content size of {} bytes, closing", - STREAM_MAX_CONTENT_BYTES, - ); - crate::logging::warn(&msg); - turn_error.get_or_insert(msg.clone()); - let _ = self.tx_event.send(Event::error(msg, true)).await; + let envelope = StreamError::Overflow { + limit_bytes: STREAM_MAX_CONTENT_BYTES, + } + .into_envelope(); + crate::logging::warn(&envelope.message); + turn_error.get_or_insert(envelope.message.clone()); + let _ = self.tx_event.send(Event::error(envelope)).await; break; } @@ -2613,13 +2625,21 @@ impl Engine { Err(retry_err) => { let retry_msg = format!("Stream retry failed: {retry_err}"); turn_error.get_or_insert(retry_msg.clone()); - let _ = self.tx_event.send(Event::error(retry_msg, true)).await; + let _ = self + .tx_event + .send(Event::error(ErrorEnvelope::classify( + retry_msg, true, + ))) + .await; break; } } } turn_error.get_or_insert(message.clone()); - let _ = self.tx_event.send(Event::error(message, true)).await; + let _ = self + .tx_event + .send(Event::error(ErrorEnvelope::classify(message, true))) + .await; if stream_errors >= MAX_STREAM_ERRORS_BEFORE_FAIL { break; } @@ -3543,6 +3563,11 @@ impl Engine { } let mut step_error_count = 0usize; + // Categorized tool errors collected this step. Feeds the capacity + // controller's error-escalation checkpoint so it can distinguish + // (e.g.) a Tool failure that should escalate from a permission + // denial that should not. + let mut step_error_categories: Vec = Vec::new(); let mut stop_after_plan_tool = false; for outcome in outcomes.into_iter().flatten() { @@ -3588,14 +3613,18 @@ impl Engine { .await; } Err(e) => { + let envelope: ErrorEnvelope = e.clone().into(); emit_tool_audit(json!({ "event": "tool.result", "tool_id": outcome.id.clone(), "tool_name": outcome.name.clone(), "success": false, "error": e.to_string(), + "category": envelope.category.to_string(), + "severity": envelope.severity.to_string(), })); step_error_count += 1; + step_error_categories.push(envelope.category); let error = format_tool_error(&e, &outcome.name); tool_call.set_error(error.clone(), duration); self.session.working_set.observe_tool_call( @@ -3669,7 +3698,7 @@ impl Engine { mode, step_error_count, consecutive_tool_error_steps, - &[], + &step_error_categories, ) .await { @@ -3770,19 +3799,28 @@ impl Engine { mode: AppMode, step_error_count: usize, consecutive_tool_error_steps: u32, - #[allow(clippy::needless_pass_by_ref_mut)] - // error_categories will be used in future escalation logic - error_categories: &[crate::error_taxonomy::ErrorCategory], + error_categories: &[ErrorCategory], ) -> bool { if step_error_count == 0 && consecutive_tool_error_steps < 2 { return false; } - let has_context_overflow = - error_categories.contains(&crate::error_taxonomy::ErrorCategory::InvalidInput); - + // Categorize this step's failures by typed `ErrorCategory` rather than + // substring-matching error strings. Context overflow always escalates; + // network / rate-limit / timeout are transient and skip escalation; + // anything else only escalates with consecutive consecutive failures. + let has_context_overflow = error_categories.contains(&ErrorCategory::InvalidInput); + let only_transient = !error_categories.is_empty() + && error_categories.iter().all(|c| { + matches!( + c, + ErrorCategory::Network | ErrorCategory::RateLimit | ErrorCategory::Timeout + ) + }); + if only_transient && !has_context_overflow { + return false; + } if !has_context_overflow && consecutive_tool_error_steps < 2 { - // Only escalate on non-context errors when we have consecutive failures return false; } diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index 6ebde7e1..43cd628b 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -1039,3 +1039,82 @@ fn stream_retry_threshold_relaxed_to_five() { provider on real outages" ); } + +// === Issue #66: error taxonomy wired through engine + audit + capacity === + +/// A failed-tool audit entry must carry the typed `category` and `severity` +/// fields derived from the underlying `ToolError`. This is what makes +/// downstream tooling able to bucket failures without scraping the message +/// string. +#[test] +fn tool_failure_audit_payload_carries_category_and_severity() { + use crate::error_taxonomy::ErrorEnvelope; + use crate::tools::spec::ToolError; + + let error = ToolError::Timeout { seconds: 30 }; + let envelope: ErrorEnvelope = error.clone().into(); + let payload = json!({ + "event": "tool.result", + "tool_id": "tool-1", + "tool_name": "exec_shell", + "success": false, + "error": error.to_string(), + "category": envelope.category.to_string(), + "severity": envelope.severity.to_string(), + }); + + assert_eq!(payload["category"], "timeout"); + assert_eq!(payload["severity"], "warning"); + assert_eq!(payload["success"], false); +} + +/// Capacity escalation sees `ErrorCategory::InvalidInput` as a context-overflow +/// signal that must escalate even on the first failure (no consecutive +/// requirement). The previous string-matching path scanned the message for +/// "context length" — categories give us a typed contract instead. +#[test] +fn capacity_escalation_treats_invalid_input_as_overflow_signal() { + use crate::error_taxonomy::ErrorCategory; + + // Replays the categorization branches inside + // `run_capacity_error_escalation_checkpoint`. Keeping the assertions on + // the typed surface (slice of `ErrorCategory`) means this test fails + // loudly if a future refactor reverts to substring matching. + let categories: &[ErrorCategory] = &[ErrorCategory::InvalidInput]; + let has_context_overflow = categories.contains(&ErrorCategory::InvalidInput); + assert!(has_context_overflow); + + let only_transient = !categories.is_empty() + && categories.iter().all(|c| { + matches!( + c, + ErrorCategory::Network | ErrorCategory::RateLimit | ErrorCategory::Timeout + ) + }); + assert!(!only_transient); +} + +/// Transient categories (network / rate limit / timeout) must NOT escalate by +/// themselves — those resolve via the existing retry loop and shouldn't +/// trigger a capacity-driven replan. +#[test] +fn capacity_escalation_skips_pure_transient_categories() { + use crate::error_taxonomy::ErrorCategory; + + let categories: &[ErrorCategory] = &[ + ErrorCategory::Network, + ErrorCategory::RateLimit, + ErrorCategory::Timeout, + ]; + let has_context_overflow = categories.contains(&ErrorCategory::InvalidInput); + assert!(!has_context_overflow); + + let only_transient = !categories.is_empty() + && categories.iter().all(|c| { + matches!( + c, + ErrorCategory::Network | ErrorCategory::RateLimit | ErrorCategory::Timeout + ) + }); + assert!(only_transient); +} diff --git a/crates/tui/src/core/events.rs b/crates/tui/src/core/events.rs index 4b800057..5f680fad 100644 --- a/crates/tui/src/core/events.rs +++ b/crates/tui/src/core/events.rs @@ -259,23 +259,10 @@ pub enum Event { } impl Event { - /// Create a new error event with a categorized envelope. - pub fn error(message: impl Into, recoverable: bool) -> Self { - let envelope = ErrorEnvelope::new( - crate::error_taxonomy::ErrorCategory::Internal, - crate::error_taxonomy::ErrorSeverity::Error, - recoverable, - "event_error", - message, - ); - Event::Error { - envelope, - recoverable, - } - } - - /// Create an error event from a pre-built `ErrorEnvelope`. - pub fn error_with_envelope(envelope: ErrorEnvelope, recoverable: bool) -> Self { + /// Create an error event from a categorized envelope. The envelope's own + /// `recoverable` flag controls whether the UI flips into offline mode. + pub fn error(envelope: ErrorEnvelope) -> Self { + let recoverable = envelope.recoverable; Event::Error { envelope, recoverable, diff --git a/crates/tui/src/error_taxonomy.rs b/crates/tui/src/error_taxonomy.rs index d56d110c..9d4ea69a 100644 --- a/crates/tui/src/error_taxonomy.rs +++ b/crates/tui/src/error_taxonomy.rs @@ -1,6 +1,5 @@ //! Shared error taxonomy across client, tools, runtime, and UI. use std::fmt; -use std::time::Duration; use crate::llm_client::LlmError; use crate::tools::spec::ToolError; @@ -22,11 +21,6 @@ pub enum ErrorCategory { } /// Severity hint for UI and logs. -/// -/// Adopted in `From` / `From` and exercised by tests, but -/// not yet read by an audit-log writer or TUI severity colorer. Pending #66 -/// follow-up; the type is stable. -#[allow(dead_code)] #[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize, serde::Deserialize)] #[serde(rename_all = "snake_case")] pub enum ErrorSeverity { @@ -37,11 +31,6 @@ pub enum ErrorSeverity { } /// Unified envelope used when crossing subsystem boundaries. -/// -/// Constructed by the `From` / `From` impls below and -/// validated by tests, but the engine still emits errors as `(String, bool)` -/// pairs on the event channel. Pending #66 follow-up. -#[allow(dead_code)] #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] pub struct ErrorEnvelope { pub category: ErrorCategory, @@ -90,7 +79,6 @@ impl fmt::Display for ErrorEnvelope { impl std::error::Error for ErrorEnvelope {} impl ErrorEnvelope { - #[allow(dead_code)] #[must_use] pub fn new( category: ErrorCategory, @@ -107,6 +95,112 @@ impl ErrorEnvelope { message: message.into(), } } + + /// Recoverable internal error — stream stalls, transient retries, generic + /// engine errors that the user can resolve by retrying. Severity is + /// `Warning` so the UI surfaces it in amber rather than red. + #[must_use] + pub fn transient(message: impl Into) -> Self { + Self::new( + ErrorCategory::Internal, + ErrorSeverity::Warning, + true, + "transient", + message, + ) + } + + /// Non-recoverable internal error — missing client, spawn failure, etc. + /// Flips the session into offline mode. + #[must_use] + pub fn fatal(message: impl Into) -> Self { + Self::new( + ErrorCategory::Internal, + ErrorSeverity::Error, + false, + "fatal", + message, + ) + } + + /// Authentication failure — fatal and blocks the session. + #[must_use] + pub fn fatal_auth(message: impl Into) -> Self { + Self::new( + ErrorCategory::Authentication, + ErrorSeverity::Critical, + false, + "auth_fatal", + message, + ) + } + + /// Context length / overflow — invalid input, recoverable via /compact. + #[must_use] + pub fn context_overflow(message: impl Into) -> Self { + Self::new( + ErrorCategory::InvalidInput, + ErrorSeverity::Error, + true, + "context_overflow", + message, + ) + } + + /// Recoverable network / transport hiccup. + #[must_use] + pub fn network(message: impl Into) -> Self { + Self::new( + ErrorCategory::Network, + ErrorSeverity::Warning, + true, + "network_transient", + message, + ) + } + + /// Tool execution failure. + #[must_use] + pub fn tool(message: impl Into) -> Self { + Self::new( + ErrorCategory::Tool, + ErrorSeverity::Error, + true, + "tool_failed", + message, + ) + } + + /// Build an envelope by classifying a raw error message string. Used at + /// boundaries where the underlying error type was already stringified. + #[must_use] + pub fn classify(message: impl Into, recoverable: bool) -> Self { + let message = message.into(); + let category = classify_error_message(&message); + let severity = match category { + ErrorCategory::Authentication => ErrorSeverity::Critical, + ErrorCategory::RateLimit | ErrorCategory::Timeout | ErrorCategory::Network => { + ErrorSeverity::Warning + } + ErrorCategory::InvalidInput | ErrorCategory::Authorization | ErrorCategory::Parse => { + ErrorSeverity::Error + } + ErrorCategory::Tool | ErrorCategory::State | ErrorCategory::Internal => { + if recoverable { + ErrorSeverity::Warning + } else { + ErrorSeverity::Error + } + } + }; + Self::new( + category, + severity, + recoverable, + category.to_string(), + message, + ) + } } impl From for ErrorEnvelope { @@ -226,7 +320,23 @@ pub fn classify_error_message(message: &str) -> ErrorCategory { if lower.contains("permission") || lower.contains("forbidden") || lower.contains("denied") { return ErrorCategory::Authorization; } - if lower.contains("network") || lower.contains("connection") || lower.contains("dns") { + if lower.contains("network") + || lower.contains("connection") + || lower.contains("dns") + || lower.contains("temporarily unavailable") + || lower.contains(" 502 ") + || lower.contains(" 503 ") + || lower.contains(" 504 ") + || lower.starts_with("502 ") + || lower.starts_with("503 ") + || lower.starts_with("504 ") + || lower.ends_with(" 502") + || lower.ends_with(" 503") + || lower.ends_with(" 504") + || lower == "502" + || lower == "503" + || lower == "504" + { return ErrorCategory::Network; } if lower.contains("parse") || lower.contains("syntax") || lower.contains("malformed") { @@ -301,149 +411,49 @@ impl From for ErrorEnvelope { } } -/// Client‑side error wrapper surfaced to the UI. -/// -/// Carries a full `ErrorEnvelope` so the TUI can render category‑specific -/// styling instead of a generic `Event::Error { message, recoverable }`. -#[allow(dead_code)] -#[derive(Debug, Clone)] -pub enum ClientError { - /// Transport / HTTP / auth error from the LLM provider. - Provider { - envelope: ErrorEnvelope, - /// When true the engine should attempt a retry. - retryable: bool, - }, - /// Error originating from the stream (SSE / chunk decode / protocol). - Stream { - envelope: ErrorEnvelope, - retryable: bool, - }, - /// Generic internal error that doesn't fit a provider taxonomy. - Internal { envelope: ErrorEnvelope }, -} - -#[allow(dead_code)] -impl ClientError { - /// Unwrap the inner envelope regardless of variant. - #[must_use] - pub fn envelope(&self) -> &ErrorEnvelope { - match self { - Self::Provider { envelope, .. } - | Self::Stream { envelope, .. } - | Self::Internal { envelope } => envelope, - } - } - - /// Whether this error is eligible for a transparent retry. - #[must_use] - pub fn is_retryable(&self) -> bool { - match self { - Self::Provider { retryable, .. } | Self::Stream { retryable, .. } => *retryable, - Self::Internal { .. } => false, - } - } - - /// Construct from an `LlmError` with Retry‑After header support. - pub fn from_llm_error(err: LlmError, retry_after: Option) -> Self { - let retryable = err.is_retryable(); - let envelope: ErrorEnvelope = err.into(); - if retryable { - let envelope = if let Some(delay) = retry_after { - ErrorEnvelope { - code: format!("{}:retry_after_{}s", envelope.code, delay.as_secs()), - ..envelope - } - } else { - envelope - }; - Self::Provider { - envelope, - retryable: true, - } - } else { - Self::Provider { - envelope, - retryable: false, - } - } - } - - /// Construct a stream‑level error. - pub fn stream(message: impl Into, retryable: bool) -> Self { - let envelope = ErrorEnvelope::new( - ErrorCategory::Internal, - ErrorSeverity::Warning, - retryable, - "stream_error", - message, - ); - Self::Stream { - envelope, - retryable, - } - } - - /// Construct an internal error. - pub fn internal(message: impl Into) -> Self { - let envelope = ErrorEnvelope::new( - ErrorCategory::Internal, - ErrorSeverity::Error, - false, - "internal", - message, - ); - Self::Internal { envelope } - } -} - -impl fmt::Display for ClientError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.envelope()) - } -} - -impl std::error::Error for ClientError {} - /// Stream‑level error discriminated by origin. /// /// Each variant maps to an `ErrorCategory` so the UI can render -/// stream‑specific icons or formatting. -#[allow(dead_code)] +/// stream‑specific icons or formatting. Wired into engine.rs at the three +/// stream guard sites (chunk timeout, max-bytes overflow, max-duration). #[derive(Debug, Clone)] pub enum StreamError { /// Stream stalled — no chunk received within the idle timeout. Stall { timeout_secs: u64 }, - /// Chunk decode / JSON parse failure. - Decode { message: String }, /// Stream exceeded content size limit. Overflow { limit_bytes: usize }, /// Stream exceeded wall‑clock duration limit. DurationLimit { limit_secs: u64 }, - /// Transport error from the underlying SSE connection. - Transport { message: String }, } -#[allow(dead_code)] impl StreamError { - /// Convert into a `ClientError` for emission. + /// Convert directly into an `ErrorEnvelope` for emission on the engine + /// event channel. Stalls are warning-severity and recoverable; size and + /// duration limits are errors (the user must restart the turn). #[must_use] - pub fn into_client_error(self) -> ClientError { + pub fn into_envelope(self) -> ErrorEnvelope { match self { - Self::Stall { timeout_secs } => { - ClientError::stream(format!("Stream stalled after {timeout_secs}s idle"), true) - } - Self::Decode { message } => { - ClientError::stream(format!("Stream decode error: {message}"), true) - } - Self::Overflow { limit_bytes } => { - ClientError::stream(format!("Stream exceeded {limit_bytes} bytes limit"), false) - } - Self::DurationLimit { limit_secs } => ClientError::stream( - format!("Stream exceeded {limit_secs}s duration limit"), - false, + Self::Stall { timeout_secs } => ErrorEnvelope::new( + ErrorCategory::Timeout, + ErrorSeverity::Warning, + true, + "stream_stall", + format!("Stream stalled: no data received for {timeout_secs}s, closing stream"), + ), + Self::Overflow { limit_bytes } => ErrorEnvelope::new( + ErrorCategory::Internal, + ErrorSeverity::Error, + true, + "stream_overflow", + format!("Stream exceeded maximum content size of {limit_bytes} bytes, closing"), + ), + Self::DurationLimit { limit_secs } => ErrorEnvelope::new( + ErrorCategory::Timeout, + ErrorSeverity::Error, + true, + "stream_duration_limit", + format!("Stream exceeded maximum duration of {limit_secs}s, closing"), ), - Self::Transport { message } => ClientError::stream(message, true), } } } @@ -454,14 +464,12 @@ impl fmt::Display for StreamError { Self::Stall { timeout_secs } => { write!(f, "Stream stalled after {timeout_secs}s idle") } - Self::Decode { message } => write!(f, "Stream decode error: {message}"), Self::Overflow { limit_bytes } => { write!(f, "Stream exceeded {limit_bytes} bytes limit") } Self::DurationLimit { limit_secs } => { write!(f, "Stream exceeded {limit_secs}s duration limit") } - Self::Transport { message } => write!(f, "Stream transport: {message}"), } } } diff --git a/crates/tui/src/tui/history.rs b/crates/tui/src/tui/history.rs index a494ce9c..9d57966c 100644 --- a/crates/tui/src/tui/history.rs +++ b/crates/tui/src/tui/history.rs @@ -82,6 +82,13 @@ pub enum HistoryCell { System { content: String, }, + /// Categorized engine-error cell. Severity drives the label glyph + color + /// (red for `Error`/`Critical`, amber for `Warning`, dim for `Info`) so + /// the user can prioritize at a glance. + Error { + message: String, + severity: crate::error_taxonomy::ErrorSeverity, + }, Thinking { content: String, streaming: bool, @@ -164,6 +171,13 @@ impl HistoryCell { content, width, ), + HistoryCell::Error { message, severity } => render_message( + error_label_text(*severity), + error_label_style(*severity), + error_body_style(*severity), + message, + width, + ), HistoryCell::Thinking { content, streaming, @@ -230,7 +244,7 @@ impl HistoryCell { content, width, ), - HistoryCell::System { .. } => self.lines(width), + HistoryCell::System { .. } | HistoryCell::Error { .. } => self.lines(width), HistoryCell::SubAgent(cell) => cell.lines(width), } } @@ -261,7 +275,7 @@ impl HistoryCell { content, width, ), - HistoryCell::System { .. } => self.lines(width), + HistoryCell::System { .. } | HistoryCell::Error { .. } => self.lines(width), HistoryCell::Thinking { content, streaming, @@ -1770,6 +1784,41 @@ fn system_body_style() -> Style { Style::default().fg(palette::TEXT_MUTED).italic() } +/// Label glyph for an error cell. `Critical`/`Error` get the loudest marker; +/// `Warning` is softer; `Info` is neutral. Kept as ASCII so it survives any +/// terminal font fallback. +fn error_label_text(severity: crate::error_taxonomy::ErrorSeverity) -> &'static str { + match severity { + crate::error_taxonomy::ErrorSeverity::Critical + | crate::error_taxonomy::ErrorSeverity::Error => "Error", + crate::error_taxonomy::ErrorSeverity::Warning => "Warn", + crate::error_taxonomy::ErrorSeverity::Info => "Info", + } +} + +/// Label color for an error cell — drives the leading rail glyph. +fn error_label_style(severity: crate::error_taxonomy::ErrorSeverity) -> Style { + let color = match severity { + crate::error_taxonomy::ErrorSeverity::Critical + | crate::error_taxonomy::ErrorSeverity::Error => palette::STATUS_ERROR, + crate::error_taxonomy::ErrorSeverity::Warning => palette::STATUS_WARNING, + crate::error_taxonomy::ErrorSeverity::Info => palette::TEXT_DIM, + }; + Style::default().fg(color).add_modifier(Modifier::BOLD) +} + +/// Body color for an error cell — softer than the label so the rail draws +/// the eye but the prose stays readable. +fn error_body_style(severity: crate::error_taxonomy::ErrorSeverity) -> Style { + let color = match severity { + crate::error_taxonomy::ErrorSeverity::Critical + | crate::error_taxonomy::ErrorSeverity::Error => palette::STATUS_ERROR, + crate::error_taxonomy::ErrorSeverity::Warning => palette::STATUS_WARNING, + crate::error_taxonomy::ErrorSeverity::Info => palette::TEXT_MUTED, + }; + Style::default().fg(color) +} + fn thinking_style() -> Style { Style::default().fg(palette::TEXT_TOOL_OUTPUT) } @@ -2717,4 +2766,86 @@ mod tests { let transcript_text = lines_text(&transcript); assert!(transcript_text.contains("row 29")); } + + // === ErrorEnvelope severity → cell color tests (#66) === + + /// Snapshot: an `Error`-severity cell uses the red status palette token + /// for both the leading "Error" label glyph and the body. This is the + /// load-bearing visual signal that distinguishes an error cell from a + /// neutral system note. + #[test] + fn error_severity_cell_renders_in_red() { + let cell = HistoryCell::Error { + message: "Authentication failed: invalid API key".to_string(), + severity: crate::error_taxonomy::ErrorSeverity::Error, + }; + let lines = cell.lines(80); + assert!( + !lines.is_empty(), + "error cell must render at least one line" + ); + + let head = &lines[0]; + let label_span = &head.spans[0]; + assert_eq!(label_span.content.as_ref(), "Error"); + assert_eq!(label_span.style.fg, Some(palette::STATUS_ERROR)); + assert!(label_span.style.add_modifier.contains(Modifier::BOLD)); + + // The body carries the error message and is rendered in the same red. + let body_text = lines + .iter() + .flat_map(|line| line.spans.iter().map(|span| span.content.as_ref())) + .collect::(); + assert!(body_text.contains("Authentication failed")); + // Find a span whose text contains "Authentication" and verify its color. + let body_span = lines + .iter() + .flat_map(|line| line.spans.iter()) + .find(|span| span.content.contains("Authentication")) + .expect("error body span must exist"); + assert_eq!(body_span.style.fg, Some(palette::STATUS_ERROR)); + } + + /// `Warning`-severity uses amber, not red — distinguishes a transient + /// retry hiccup from a hard failure. + #[test] + fn warning_severity_cell_renders_in_amber() { + let cell = HistoryCell::Error { + message: "Stream stalled: no data received for 60s, closing stream".to_string(), + severity: crate::error_taxonomy::ErrorSeverity::Warning, + }; + let lines = cell.lines(80); + let label_span = &lines[0].spans[0]; + assert_eq!(label_span.content.as_ref(), "Warn"); + assert_eq!(label_span.style.fg, Some(palette::STATUS_WARNING)); + } + + /// `Critical` severity collapses to the same red as `Error` — both flip + /// offline mode and both should read as the loudest signal in the + /// transcript. + #[test] + fn critical_severity_cell_renders_in_red() { + let cell = HistoryCell::Error { + message: "API key expired".to_string(), + severity: crate::error_taxonomy::ErrorSeverity::Critical, + }; + let lines = cell.lines(80); + let label_span = &lines[0].spans[0]; + assert_eq!(label_span.content.as_ref(), "Error"); + assert_eq!(label_span.style.fg, Some(palette::STATUS_ERROR)); + } + + /// `Info` severity stays neutral / dim so it doesn't draw the eye away + /// from real failures sitting alongside it in the transcript. + #[test] + fn info_severity_cell_renders_in_dim() { + let cell = HistoryCell::Error { + message: "Reconnected".to_string(), + severity: crate::error_taxonomy::ErrorSeverity::Info, + }; + let lines = cell.lines(80); + let label_span = &lines[0].spans[0]; + assert_eq!(label_span.content.as_ref(), "Info"); + assert_eq!(label_span.style.fg, Some(palette::TEXT_DIM)); + } } diff --git a/crates/tui/src/tui/transcript.rs b/crates/tui/src/tui/transcript.rs index 2070f837..afb1c8cf 100644 --- a/crates/tui/src/tui/transcript.rs +++ b/crates/tui/src/tui/transcript.rs @@ -170,6 +170,7 @@ impl TranscriptViewCache { is_system_or_tool: matches!( cell, HistoryCell::System { .. } + | HistoryCell::Error { .. } | HistoryCell::Tool(_) | HistoryCell::SubAgent(_) ), diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index 10be3751..125f6e5d 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -697,9 +697,9 @@ async fn run_event_loop( } EngineEvent::Error { envelope, - recoverable, + recoverable: _, } => { - apply_engine_error_to_app(app, envelope.message.clone(), recoverable); + apply_engine_error_to_app(app, envelope); } EngineEvent::Status { message } => { app.status_message = Some(message); @@ -1931,19 +1931,28 @@ fn queued_session_to_ui(msg: QueuedSessionMessage) -> QueuedMessage { /// Translate an `EngineEvent::Error` into UI state updates. /// -/// `recoverable` is the engine's own classification: stream stalls, chunk +/// The engine's `recoverable` flag (mirrored on `ErrorEnvelope`) decides +/// whether the session flips into offline mode: stream stalls, chunk /// timeouts, transient network errors, and rate-limit/server hiccups arrive -/// with `recoverable = true` and must NOT flip the session into offline mode -/// — the user can resend the turn and the underlying transport will retry. -/// Hard failures (auth, billing, invalid request) arrive with -/// `recoverable = false`; those flip offline mode so subsequent messages get -/// queued instead of silently lost mid-flight. -pub(crate) fn apply_engine_error_to_app(app: &mut App, message: String, recoverable: bool) { +/// recoverable and must NOT flip into offline. Hard failures (auth, billing, +/// invalid request) arrive non-recoverable; those flip offline so subsequent +/// messages get queued instead of silently lost mid-flight. +/// +/// `severity` drives transcript color: red for `Error`/`Critical`, amber for +/// `Warning`, dim for `Info`. +pub(crate) fn apply_engine_error_to_app( + app: &mut App, + envelope: crate::error_taxonomy::ErrorEnvelope, +) { + let recoverable = envelope.recoverable; + let message = envelope.message.clone(); + let severity = envelope.severity; app.streaming_state.reset(); app.streaming_message_index = None; app.streaming_thinking_active_entry = None; - app.add_message(HistoryCell::System { - content: format!("Error: {message}"), + app.add_message(HistoryCell::Error { + message: message.clone(), + severity, }); app.is_loading = false; if recoverable { @@ -4583,6 +4592,7 @@ fn open_tool_details_pager(app: &mut App) -> bool { HistoryCell::User { .. } => "You".to_string(), HistoryCell::Assistant { .. } => "Assistant".to_string(), HistoryCell::System { .. } => "Note".to_string(), + HistoryCell::Error { .. } => "Error".to_string(), HistoryCell::Thinking { .. } => "Reasoning".to_string(), HistoryCell::Tool(_) => "Message".to_string(), HistoryCell::SubAgent(_) => "Sub-agent".to_string(), diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index eed9959e..4b3350d0 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -1920,14 +1920,12 @@ fn truncate_line_to_width_respects_display_width_for_tiny_budgets() { /// the session in offline mode with the next typed message queued. #[test] fn recoverable_engine_error_does_not_enter_offline_mode() { + use crate::error_taxonomy::{ErrorEnvelope, StreamError}; let mut app = create_test_app(); assert!(!app.offline_mode); - apply_engine_error_to_app( - &mut app, - "Stream stalled: no data received for 60s, closing stream".to_string(), - true, - ); + let envelope = StreamError::Stall { timeout_secs: 60 }.into_envelope(); + apply_engine_error_to_app(&mut app, envelope); assert!( !app.offline_mode, @@ -1942,6 +1940,17 @@ fn recoverable_engine_error_does_not_enter_offline_mode() { status.starts_with("Connection interrupted"), "expected interrupt-style status, got {status:?}" ); + + // Sanity: the rendered cell is the categorized Error variant, not a plain System note. + let last = app + .history + .last() + .expect("recoverable engine error should push a history cell"); + assert!( + matches!(last, crate::tui::history::HistoryCell::Error { .. }), + "expected HistoryCell::Error, got {last:?}" + ); + let _ = ErrorEnvelope::transient(""); } /// Hard failures (auth, billing, malformed request) DO need to flip offline @@ -1949,13 +1958,13 @@ fn recoverable_engine_error_does_not_enter_offline_mode() { /// against a broken upstream. #[test] fn non_recoverable_engine_error_enters_offline_mode() { + use crate::error_taxonomy::ErrorEnvelope; let mut app = create_test_app(); assert!(!app.offline_mode); apply_engine_error_to_app( &mut app, - "Authentication failed: invalid API key".to_string(), - false, + ErrorEnvelope::fatal_auth("Authentication failed: invalid API key"), ); assert!(