From 5c9404e292cde624804a3d6a81bea9795df81647 Mon Sep 17 00:00:00 2001 From: "Dhanji R. Prasanna" Date: Tue, 13 Jan 2026 05:58:54 +0530 Subject: [PATCH] Refactor: improve readability in CLI modules - project_files.rs: Fix UTF-8 safety in truncate_for_display (use char boundaries instead of byte slicing), add test for multi-byte chars - task_execution.rs: Extract recoverable_error_name() helper, use shared calculate_retry_delay() from error_handling.rs to eliminate duplication - ui_writer_impl.rs: Extract duration_color() helper for timing display, add clear_tool_state() to consolidate repeated mutex clearing patterns Agent: carmack --- crates/g3-cli/src/project_files.rs | 17 +++++- crates/g3-cli/src/task_execution.rs | 48 +++++++-------- crates/g3-cli/src/ui_writer_impl.rs | 90 ++++++++++++++--------------- 3 files changed, 77 insertions(+), 78 deletions(-) diff --git a/crates/g3-cli/src/project_files.rs b/crates/g3-cli/src/project_files.rs index d13318e..363669b 100644 --- a/crates/g3-cli/src/project_files.rs +++ b/crates/g3-cli/src/project_files.rs @@ -166,10 +166,12 @@ fn find_fallback_title(content: &str) -> Option { /// Truncate a string for display, adding ellipsis if needed. fn truncate_for_display(s: &str, max_len: usize) -> String { - if s.len() > max_len { - format!("{}...", &s[..max_len - 3]) - } else { + if s.chars().count() <= max_len { s.to_string() + } else { + // Truncate at character boundary, not byte boundary + let truncated: String = s.chars().take(max_len.saturating_sub(3)).collect(); + format!("{}...", truncated) } } @@ -204,6 +206,15 @@ mod tests { assert_eq!(truncated.len(), 100); } + #[test] + fn test_truncate_for_display_utf8() { + // Multi-byte characters should not cause panics + let emoji_text = "Hello 👋 World 🌍 Test ✨ More text here and more"; + let truncated = truncate_for_display(emoji_text, 15); + assert!(truncated.ends_with("...")); + assert!(truncated.chars().count() <= 15); + } + #[test] fn test_combine_project_content_all_some() { let workspace = std::path::PathBuf::from("/test/workspace"); diff --git a/crates/g3-cli/src/task_execution.rs b/crates/g3-cli/src/task_execution.rs index d741845..c028dbd 100644 --- a/crates/g3-cli/src/task_execution.rs +++ b/crates/g3-cli/src/task_execution.rs @@ -1,6 +1,6 @@ //! Task execution with retry logic for G3 CLI. -use g3_core::error_handling::{classify_error, ErrorType, RecoverableError}; +use g3_core::error_handling::{calculate_retry_delay, classify_error, ErrorType, RecoverableError}; use g3_core::ui_writer::UiWriter; use g3_core::Agent; use tokio_util::sync::CancellationToken; @@ -11,6 +11,19 @@ use crate::simple_output::SimpleOutput; /// Maximum number of retry attempts for recoverable errors const MAX_RETRIES: u32 = 3; +/// Get a human-readable name for a recoverable error type. +fn recoverable_error_name(err: &RecoverableError) -> &'static str { + match err { + RecoverableError::RateLimit => "Rate limit", + RecoverableError::ServerError => "Server error", + RecoverableError::NetworkError => "Network error", + RecoverableError::Timeout => "Timeout", + RecoverableError::ModelBusy => "Model busy", + RecoverableError::TokenLimit => "Token limit", + RecoverableError::ContextLengthExceeded => "Context length", + } +} + /// Execute a task with retry logic for recoverable errors. pub async fn execute_task_with_retry( agent: &mut Agent, @@ -66,35 +79,16 @@ pub async fn execute_task_with_retry( if let ErrorType::Recoverable(recoverable_error) = error_type { if attempt < MAX_RETRIES { - // Calculate retry delay with exponential backoff + jitter - let base_delay_ms = match recoverable_error { - RecoverableError::RateLimit => 5000, // Rate limits need longer waits - RecoverableError::ServerError => 2000, - RecoverableError::NetworkError => 1000, - RecoverableError::Timeout => 1000, - RecoverableError::ModelBusy => 3000, - RecoverableError::TokenLimit => 1000, - RecoverableError::ContextLengthExceeded => 1000, - }; - let delay_ms = base_delay_ms * (2_u64.pow(attempt - 1)); - // Add jitter (±20%) - let jitter = (delay_ms as f64 * 0.2 * (rand::random::() - 0.5)) as i64; - let delay_ms = (delay_ms as i64 + jitter).max(100) as u64; - let delay = std::time::Duration::from_millis(delay_ms); - - let error_name = match recoverable_error { - RecoverableError::RateLimit => "Rate limit", - RecoverableError::ServerError => "Server error", - RecoverableError::NetworkError => "Network error", - RecoverableError::Timeout => "Timeout", - RecoverableError::ModelBusy => "Model busy", - RecoverableError::TokenLimit => "Token limit", - RecoverableError::ContextLengthExceeded => "Context length", - }; + // Use shared retry delay calculation (non-autonomous mode) + let delay = calculate_retry_delay(attempt, false); + let delay_ms = delay.as_millis(); output.print(&format!( "⚠️ {} detected (attempt {}/{}). Retrying in {:.1}s...", - error_name, attempt, MAX_RETRIES, delay_ms as f64 / 1000.0 + recoverable_error_name(&recoverable_error), + attempt, + MAX_RETRIES, + delay_ms as f64 / 1000.0 )); // Wait before retrying diff --git a/crates/g3-cli/src/ui_writer_impl.rs b/crates/g3-cli/src/ui_writer_impl.rs index 976784d..7c8b870 100644 --- a/crates/g3-cli/src/ui_writer_impl.rs +++ b/crates/g3-cli/src/ui_writer_impl.rs @@ -22,6 +22,45 @@ pub struct ConsoleUiWriter { last_output_was_tool: std::sync::Mutex, } +/// ANSI color code for duration display based on elapsed time. +/// Returns empty string for fast operations, yellow/orange/red for slower ones. +fn duration_color(duration_str: &str) -> &'static str { + // Format: "500ms", "1.5s", "2m 30.0s" + if duration_str.ends_with("ms") { + return ""; // Sub-second: no color + } + + if let Some(m_pos) = duration_str.find('m') { + // Contains minutes (e.g., "2m 30.0s") + if let Ok(minutes) = duration_str[..m_pos].trim().parse::() { + return match minutes { + 5.. => "\x1b[31m", // Red: >= 5 minutes + 1.. => "\x1b[38;5;208m", // Orange: 1-4 minutes + _ => "", + }; + } + } else if let Some(s_value) = duration_str.strip_suffix('s') { + // Seconds only (e.g., "1.5s") + if let Ok(seconds) = s_value.trim().parse::() { + if seconds >= 1.0 { + return "\x1b[33m"; // Yellow: >= 1 second + } + } + } + + "" // Default: no color +} + +impl ConsoleUiWriter { + /// Clear all stored tool state after output is complete. + fn clear_tool_state(&self) { + *self.current_tool_name.lock().unwrap() = None; + self.current_tool_args.lock().unwrap().clear(); + *self.current_output_line.lock().unwrap() = None; + *self.output_line_printed.lock().unwrap() = false; + } +} + impl ConsoleUiWriter { pub fn new() -> Self { Self { @@ -375,55 +414,13 @@ impl UiWriter for ConsoleUiWriter { // Clear the stored tool info drop(args); // Release the lock before clearing - *self.current_tool_name.lock().unwrap() = None; - self.current_tool_args.lock().unwrap().clear(); - *self.current_output_line.lock().unwrap() = None; - *self.output_line_printed.lock().unwrap() = false; + self.clear_tool_state(); true } fn print_tool_timing(&self, duration_str: &str, tokens_delta: u32, context_percentage: f32) { - // Parse the duration string to determine color - // Format is like "1.5s", "500ms", "2m 30.0s" - let color_code = if duration_str.ends_with("ms") { - // Milliseconds - use default color (< 1s) - "" - } else if duration_str.contains('m') { - // Contains minutes - // Extract minutes value - if let Some(m_pos) = duration_str.find('m') { - if let Ok(minutes) = duration_str[..m_pos].trim().parse::() { - if minutes >= 5 { - "\x1b[31m" // Red for >= 5 minutes - } else { - "\x1b[38;5;208m" // Orange for >= 1 minute but < 5 minutes - } - } else { - "" // Default color if parsing fails - } - } else { - "" // Default color if 'm' not found (shouldn't happen) - } - } else if duration_str.ends_with('s') { - // Seconds only - if let Some(s_value) = duration_str.strip_suffix('s') { - if let Ok(seconds) = s_value.trim().parse::() { - if seconds >= 1.0 { - "\x1b[33m" // Yellow for >= 1 second - } else { - "" // Default color for < 1 second - } - } else { - "" // Default color if parsing fails - } - } else { - "" // Default color - } - } else { - // Milliseconds or other format - use default color - "" - }; + let color_code = duration_color(duration_str); // Add blank line before footer for research tool (its output is a full report) if let Some(tool_name) = self.current_tool_name.lock().unwrap().as_ref() { @@ -444,10 +441,7 @@ impl UiWriter for ConsoleUiWriter { } // Clear the stored tool info - *self.current_tool_name.lock().unwrap() = None; - self.current_tool_args.lock().unwrap().clear(); - *self.current_output_line.lock().unwrap() = None; - *self.output_line_printed.lock().unwrap() = false; + self.clear_tool_state(); *self.is_shell_compact.lock().unwrap() = false; }