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
This commit is contained in:
@@ -166,10 +166,12 @@ fn find_fallback_title(content: &str) -> Option<String> {
|
||||
|
||||
/// 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");
|
||||
|
||||
@@ -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<W: UiWriter>(
|
||||
agent: &mut Agent<W>,
|
||||
@@ -66,35 +79,16 @@ pub async fn execute_task_with_retry<W: UiWriter>(
|
||||
|
||||
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::<f64>() - 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
|
||||
|
||||
@@ -22,6 +22,45 @@ pub struct ConsoleUiWriter {
|
||||
last_output_was_tool: std::sync::Mutex<bool>,
|
||||
}
|
||||
|
||||
/// 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::<u32>() {
|
||||
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::<f64>() {
|
||||
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::<u32>() {
|
||||
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::<f64>() {
|
||||
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;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user