From c5d549c211964aa6e71f86964fe7a8b371b159fa Mon Sep 17 00:00:00 2001 From: "Dhanji R. Prasanna" Date: Wed, 21 Jan 2026 07:13:20 +0530 Subject: [PATCH] Readability pass: remove verbose comments and clean up tests - completion.rs: Remove redundant comments, clean up test output (println! -> let _) - g3_status.rs: Condense doc comments, rename from_str() to parse() - streaming.rs: Remove obvious doc comments that duplicate function names - simple_output.rs, ui_writer_impl.rs: Update Status::parse() calls All changes are behavior-preserving. 132 lines removed, code is more scannable. Agent: carmack --- crates/g3-cli/src/completion.rs | 123 +++++----------------------- crates/g3-cli/src/g3_status.rs | 80 +++++------------- crates/g3-cli/src/simple_output.rs | 2 +- crates/g3-cli/src/ui_writer_impl.rs | 2 +- crates/g3-core/src/streaming.rs | 15 +--- 5 files changed, 45 insertions(+), 177 deletions(-) diff --git a/crates/g3-cli/src/completion.rs b/crates/g3-cli/src/completion.rs index d296bfd..a6c12ae 100644 --- a/crates/g3-cli/src/completion.rs +++ b/crates/g3-cli/src/completion.rs @@ -1,12 +1,8 @@ //! Tab completion support for g3 interactive mode. //! //! Provides: -//! - Command completion for `/` commands (at start of line) -//! - File path completion for paths anywhere in the line: -//! - `./` - current directory -//! - `../` - parent directory -//! - `~/` - home directory -//! - `/` (not at start) - root directory +//! - Command completion for `/` commands at line start +//! - File path completion for `./`, `../`, `~/`, `/` prefixes //! - Session ID completion for `/resume` command use rustyline::completion::{Completer, FilenameCompleter, Pair}; @@ -51,8 +47,7 @@ impl G3Helper { fn extract_word<'a>(&self, line: &'a str, pos: usize) -> (usize, &'a str) { let line_to_cursor = &line[..pos]; - // Look backwards for the start of the word - // A word starts after a space (unless quoted or escaped) + // Find word start: after space (unless quoted/escaped) let mut word_start = 0; let mut in_quotes = false; let mut quote_char = ' '; @@ -65,8 +60,6 @@ impl G3Helper { in_quotes = false; } } else if prev_was_backslash { - // This char is escaped, don't treat it as special - // (e.g., backslash-space is part of the word) } else { match c { '"' | '\'' => { @@ -75,7 +68,6 @@ impl G3Helper { word_start = i; } ' ' | '\t' => { - // Space starts a new word (unless escaped) if idx + 1 < chars.len() { word_start = chars[idx + 1].0; } else { @@ -91,9 +83,7 @@ impl G3Helper { (word_start, &line_to_cursor[word_start..]) } - /// Check if a word looks like a path prefix fn is_path_prefix(&self, word: &str) -> bool { - // Strip leading quote if present (for paths like "~/...) let word = word.trim_start_matches('"').trim_start_matches('\''); word.starts_with("./") @@ -105,14 +95,12 @@ impl G3Helper { || word == "~" } - /// Strip quotes from a word for path completion fn strip_quotes<'a>(&self, word: &'a str) -> &'a str { word.trim_start_matches('"').trim_start_matches('\'') .trim_end_matches('"').trim_end_matches('\'') } - /// Unescape backslash-escaped characters in a path - /// e.g., "~/My\ Files" -> "~/My Files" + /// Unescape backslash-escaped chars: "~/My\ Files" -> "~/My Files" fn unescape_path(&self, path: &str) -> String { let mut result = String::with_capacity(path.len()); let mut chars = path.chars().peekable(); @@ -129,8 +117,7 @@ impl G3Helper { result } - /// List available session IDs from .g3/sessions/, sorted by newest first. - /// If `limit` is Some(n), returns at most n sessions. + /// List session IDs from .g3/sessions/, sorted newest-first, with optional limit. fn list_sessions(&self, limit: Option) -> Vec { let sessions_dir = PathBuf::from(".g3/sessions"); if !sessions_dir.is_dir() { @@ -185,13 +172,10 @@ impl Completer for G3Helper { // Extract the current word being typed let (word_start, word) = self.extract_word(line, pos); - // Case 1: Command completion - `/` at the very start of the line - // Only complete commands if we're typing the first word and it starts with `/` + // Case 1: Command completion at line start if word_start == 0 && word.starts_with('/') && !word.contains(' ') { - // Check if this looks like a command (no path separators after the first /) let after_slash = &word[1..]; if !after_slash.contains('/') { - // This is a command like "/com" not a path like "/etc" let matches: Vec = COMMANDS .iter() .filter(|cmd| cmd.starts_with(word)) @@ -204,47 +188,34 @@ impl Completer for G3Helper { if !matches.is_empty() { return Ok((0, matches)); } - // If no command matches, fall through to path completion - // (e.g., "/etc" should complete as a path) } } - // Case 2: Path completion for path-like prefixes - // We handle quotes ourselves since FilenameCompleter doesn't understand our extraction + // Case 2: Path completion for path-like prefixes (handles quotes ourselves) if self.is_path_prefix(word) || (word_start > 0 && line_to_cursor[word_start..].starts_with('/')) { - // Check if word starts with a quote let has_leading_quote = word.starts_with('"') || word.starts_with('\''); let quote_char = if has_leading_quote { &word[..1] } else { "" }; - // Check if word has backslash escapes let has_escapes = word.contains('\\'); - // Strip quotes and unescape backslashes to get the actual path let path_str = self.strip_quotes(word); let path_unescaped = self.unescape_path(path_str); let path: &str = &path_unescaped; - // Complete just the path portion let (_rel_start, completions) = self.file_completer.complete(path, path.len(), ctx)?; if completions.is_empty() { return Ok((pos, vec![])); } - // Adjust completions to account for quotes and word position let adjusted: Vec = completions .into_iter() .map(|pair| { - // If we had a leading quote, add it back - // Also check if the path has spaces - if so, wrap in quotes let has_spaces = pair.replacement.contains(' '); let replacement = if has_leading_quote { - // Preserve the original quote style format!("{}{}{}", quote_char, pair.replacement, quote_char) } else if has_escapes && has_spaces { - // User was using backslash escapes, continue with that style pair.replacement.replace(' ', "\\ ") } else if has_spaces { - // Add quotes around paths with spaces format!("\"{}\"" , pair.replacement) } else { pair.replacement @@ -261,14 +232,11 @@ impl Completer for G3Helper { }) .collect(); - // Return with word_start so the whole word gets replaced return Ok((word_start, adjusted)); } - // Case 3: Check if we're after a command that takes a path argument - if line_to_cursor.starts_with("/run ") - { - // For commands, just use the file completer on the path portion + // Case 3: Path argument for /run command + if line_to_cursor.starts_with("/run ") { let path = self.strip_quotes(word); let (_, completions) = self.file_completer.complete(path, path.len(), ctx)?; return Ok((word_start, completions)); @@ -277,7 +245,6 @@ impl Completer for G3Helper { // Case 4: Session ID completion for /resume command if line_to_cursor.starts_with("/resume ") { let partial = word; - // Get all sessions sorted by newest first, then filter and limit let sessions = self.list_sessions(None); let matches: Vec = sessions .into_iter() @@ -286,7 +253,7 @@ impl Completer for G3Helper { display: s.clone(), replacement: s, }) - .take(8) // Show at most 8 most recent matches + .take(8) .collect(); return Ok((word_start, matches)); } @@ -321,7 +288,6 @@ mod tests { let history = rustyline::history::DefaultHistory::new(); let ctx = Context::new(&history); - // Complete "/com" -> "/compact" let (start, matches) = helper.complete("/com", 4, &ctx).unwrap(); assert_eq!(start, 0); assert_eq!(matches.len(), 1); @@ -334,7 +300,6 @@ mod tests { let history = rustyline::history::DefaultHistory::new(); let ctx = Context::new(&history); - // Complete "/s" -> "/skinnify", "/stats" let (start, matches) = helper.complete("/s", 2, &ctx).unwrap(); assert_eq!(start, 0); assert_eq!(matches.len(), 2); @@ -414,123 +379,84 @@ mod tests { #[test] fn test_actual_completion_with_quotes() { - use rustyline::completion::Completer; - use rustyline::Context; - let helper = G3Helper::new(); let history = rustyline::history::DefaultHistory::new(); let ctx = Context::new(&history); - // Test 1: "~/ - unclosed quote at start of path - println!("\n=== Test 1: Unclosed quote \"~/ ==="); let line = "edit \"~/"; let pos = line.len(); match helper.complete(line, pos, &ctx) { Ok((start, completions)) => { - println!("Line: '{}', pos: {}", line, pos); - println!("Start: {}, num_completions: {}", start, completions.len()); - if !completions.is_empty() { - println!("First few: {:?}", completions.iter().take(3).map(|p| &p.replacement).collect::>()); - } + assert!(start > 0 || completions.is_empty() || true); // Just verify no panic } - Err(e) => println!("Error: {:?}", e), + Err(_) => {} } - // Test 2: ~/My\ - backslash before cursor - println!("\n=== Test 2: Backslash escape ~/My\\ ==="); let line = "edit ~/My\\ "; let pos = line.len(); match helper.complete(line, pos, &ctx) { Ok((start, completions)) => { - println!("Line: '{}', pos: {}", line, pos); - println!("Start: {}, num_completions: {}", start, completions.len()); - if !completions.is_empty() { - println!("First few: {:?}", completions.iter().take(3).map(|p| &p.replacement).collect::>()); - } + let _ = (start, completions); // Just verify no panic } - Err(e) => println!("Error: {:?}", e), + Err(_) => {} } - // Test 3: "~/" - closed quote - println!("\n=== Test 3: Closed quote \"/~/\" ==="); let line = "edit \"~/\""; let pos = line.len(); match helper.complete(line, pos, &ctx) { Ok((start, completions)) => { - println!("Line: '{}', pos: {}", line, pos); - println!("Start: {}, num_completions: {}", start, completions.len()); - if !completions.is_empty() { - println!("First few: {:?}", completions.iter().take(3).map(|p| &p.replacement).collect::>()); - } + let _ = (start, completions); } - Err(e) => println!("Error: {:?}", e), + Err(_) => {} } } #[test] fn test_no_completion_for_bare_quote() { - use rustyline::completion::Completer; - use rustyline::Context; - let helper = G3Helper::new(); let history = rustyline::history::DefaultHistory::new(); let ctx = Context::new(&history); - // Just a quote with no path prefix - should NOT trigger completion let line = "edit \""; let pos = line.len(); let (start, completions) = helper.complete(line, pos, &ctx).unwrap(); - println!("Line: '{}', start: {}, completions: {}", line, start, completions.len()); - - // Should return no completions since "" is not a path prefix + let _ = start; assert_eq!(completions.len(), 0, "Bare quote should not trigger path completion"); } #[test] fn test_no_completion_for_random_text_in_quotes() { - use rustyline::completion::Completer; - use rustyline::Context; - let helper = G3Helper::new(); let history = rustyline::history::DefaultHistory::new(); let ctx = Context::new(&history); - // Random text in quotes - should NOT trigger completion let line = "edit \"hello world"; let pos = line.len(); let (start, completions) = helper.complete(line, pos, &ctx).unwrap(); - println!("Line: '{}', start: {}, completions: {}", line, start, completions.len()); + let _ = start; assert_eq!(completions.len(), 0, "Random quoted text should not trigger path completion"); - // Just "foo - no path prefix let line = "edit \"foo"; let pos = line.len(); let (start, completions) = helper.complete(line, pos, &ctx).unwrap(); - println!("Line: '{}', start: {}, completions: {}", line, start, completions.len()); + let _ = start; assert_eq!(completions.len(), 0, "Quoted non-path should not trigger completion"); } #[test] fn test_resume_completion_lists_sessions() { - use rustyline::completion::Completer; - use rustyline::Context; - let helper = G3Helper::new(); let history = rustyline::history::DefaultHistory::new(); let ctx = Context::new(&history); - // Test against real .g3/sessions in current project - // This test runs from the project root where .g3/sessions exists let line = "/resume "; let pos = line.len(); let (start, completions) = helper.complete(line, pos, &ctx).unwrap(); - println!("Sessions found: {}", completions.len()); + let _ = start; - // If .g3/sessions exists, we should get some completions if std::path::Path::new(".g3/sessions").is_dir() { assert!(completions.len() > 0, "Should list sessions when .g3/sessions exists"); - // Test filtering - use first few chars of first session if let Some(first) = completions.first() { let prefix = &first.replacement[..first.replacement.len().min(5)]; let line = format!("/resume {}", prefix); @@ -541,7 +467,6 @@ mod tests { } } - // Test with non-matching prefix - should return empty let line = "/resume zzz_nonexistent_prefix_"; let pos = line.len(); let (_, completions) = helper.complete(line, pos, &ctx).unwrap(); @@ -551,13 +476,7 @@ mod tests { #[test] fn test_resume_completion_graceful_no_panic() { let helper = G3Helper::new(); - - // Test list_sessions directly - should not panic regardless of whether - // .g3/sessions exists or not let sessions = helper.list_sessions(None); - - // This will either return sessions (if .g3/sessions exists) or empty - // The important thing is it doesn't panic - println!("list_sessions returned {} sessions", sessions.len()); + let _ = sessions; // Just verify no panic } } diff --git a/crates/g3-cli/src/g3_status.rs b/crates/g3-cli/src/g3_status.rs index b724298..0be9b3a 100644 --- a/crates/g3-cli/src/g3_status.rs +++ b/crates/g3-cli/src/g3_status.rs @@ -1,26 +1,8 @@ //! Centralized formatting for g3 system status messages. //! -//! This module provides consistent formatting for all "g3:" prefixed status messages, -//! including progress indicators, completion statuses, and inline updates. -//! -//! # Usage -//! -//! ```ignore -//! use crate::g3_status::G3Status; -//! -//! // Start a progress message (stays on same line, no newline) -//! G3Status::progress("compacting session"); -//! -//! // Complete with status (adds to same line, then newline) -//! G3Status::done(); -//! // or -//! G3Status::failed(); -//! // or -//! G3Status::error("timeout"); -//! -//! // One-shot status message (progress + status on same line) -//! G3Status::complete("compacting session", Status::Done); -//! ``` +//! Provides consistent "g3:" prefixed status messages with progress indicators +//! and completion statuses. Use `progress()` + `done()`/`failed()` for two-step +//! output, or `complete()` for one-shot messages. use crossterm::style::{Attribute, Color, ResetColor, SetAttribute, SetForegroundColor}; use std::io::{self, Write}; @@ -45,8 +27,7 @@ pub enum Status { } impl Status { - /// Parse a status string into a Status enum - pub fn from_str(s: &str) -> Self { + pub fn parse(s: &str) -> Self { match s { "done" => Status::Done, "failed" => Status::Failed, @@ -63,10 +44,7 @@ impl Status { pub struct G3Status; impl G3Status { - /// Print a progress message that stays on the same line. - /// Format: "g3: ..." - /// - "g3:" is bold green - /// - No trailing newline (use `done()`, `failed()`, etc. to complete) + /// Print "g3: ..." (no newline). Complete with `done()` or `failed()`. pub fn progress(message: &str) { print!( "{}{}g3:{}{} {} ...", @@ -79,8 +57,7 @@ impl G3Status { let _ = io::stdout().flush(); } - /// Print a progress message with a newline at the end. - /// Use this when you don't plan to add a status on the same line. + /// Print "g3: ..." with newline (standalone progress). pub fn progress_ln(message: &str) { println!( "{}{}g3:{}{} {} ...", @@ -92,7 +69,6 @@ impl G3Status { ); } - /// Complete a progress message with "[done]" in bold green. pub fn done() { println!( " {}{}[done]{}", @@ -102,7 +78,6 @@ impl G3Status { ); } - /// Complete a progress message with "[failed]" in red. pub fn failed() { println!( " {}[failed]{}", @@ -111,7 +86,6 @@ impl G3Status { ); } - /// Complete a progress message with "[error: ]" in red. pub fn error(msg: &str) { println!( " {}[error: {}]{}", @@ -121,7 +95,6 @@ impl G3Status { ); } - /// Complete a progress message with a custom status. pub fn status(status: &Status) { match status { Status::Done => Self::done(), @@ -155,15 +128,12 @@ impl G3Status { } } - /// Print a complete status message (progress + status) on one line. - /// Format: "g3: ... [status]" + /// Print "g3: ... [status]" (one-shot). pub fn complete(message: &str, status: Status) { Self::progress(message); Self::status(&status); } - /// Print an info message in dimmed/grey text. - /// Format: "... " #[allow(dead_code)] pub fn info(message: &str) { println!( @@ -174,10 +144,8 @@ impl G3Status { ); } - /// Print an info message inline (no newline, for appending to user input). - /// Uses ANSI escape to move cursor up and to end of previous line. + /// Print info inline (moves cursor up, appends to previous line). pub fn info_inline(message: &str) { - // Move cursor up one line, to end of line, then print print!( "\x1b[1A\x1b[999C {}... {}{}\n", SetForegroundColor(Color::DarkGrey), @@ -187,8 +155,7 @@ impl G3Status { let _ = io::stdout().flush(); } - /// Format a status string for inline use (returns the formatted string). - /// Useful when building custom messages. + /// Format a status for inline use (returns formatted string). pub fn format_status(status: &Status) -> String { match status { Status::Done => format!( @@ -232,7 +199,6 @@ impl G3Status { } } - /// Format the "g3:" prefix for inline use. pub fn format_prefix() -> String { format!( "{}{}g3:{}{}", @@ -243,8 +209,7 @@ impl G3Status { ) } - /// Print a resuming session message with session ID highlighted. - /// Format: "... resuming [done/error]" + /// Print "... resuming [status]" with cyan session ID. pub fn resuming(session_id: &str, status: Status) { let status_str = Self::format_status(&status); println!( @@ -256,7 +221,6 @@ impl G3Status { ); } - /// Print a resuming session message with "(summary)" note. pub fn resuming_summary(session_id: &str) { let status_str = Self::format_status(&Status::Done); println!( @@ -268,9 +232,7 @@ impl G3Status { ); } - /// Print a context thinning result. - /// Format: "g3: thinning context ... 70% -> 40% ... [done]" - /// or: "g3: thinning context (full) ... 70% ... [no changes]" + /// Print thinning result: "g3: thinning context ... 70% -> 40% ... [done]" pub fn thin_result(result: &g3_core::ThinResult) { use g3_core::ThinScope; @@ -295,11 +257,7 @@ impl G3Status { } } - /// Print a complete status message with a path highlighted in cyan. - /// Format: "g3: [status]" - /// - "g3:" is bold green - /// - path is cyan - /// - status is formatted per Status type + /// Print "g3: [status]" with cyan path. pub fn complete_with_path(message: &str, path: &str, status: Status) { print!( "{} {} {}{}{}", @@ -319,13 +277,13 @@ mod tests { #[test] fn test_status_from_str() { - assert_eq!(Status::from_str("done"), Status::Done); - assert_eq!(Status::from_str("failed"), Status::Failed); - assert_eq!(Status::from_str("resolved"), Status::Resolved); - assert_eq!(Status::from_str("insufficient"), Status::Insufficient); - assert_eq!(Status::from_str("error: timeout"), Status::Error("timeout".to_string())); - assert_eq!(Status::from_str("error timeout"), Status::Error("timeout".to_string())); - assert_eq!(Status::from_str("custom"), Status::Custom("custom".to_string())); + assert_eq!(Status::parse("done"), Status::Done); + assert_eq!(Status::parse("failed"), Status::Failed); + assert_eq!(Status::parse("resolved"), Status::Resolved); + assert_eq!(Status::parse("insufficient"), Status::Insufficient); + assert_eq!(Status::parse("error: timeout"), Status::Error("timeout".to_string())); + assert_eq!(Status::parse("error timeout"), Status::Error("timeout".to_string())); + assert_eq!(Status::parse("custom"), Status::Custom("custom".to_string())); } #[test] diff --git a/crates/g3-cli/src/simple_output.rs b/crates/g3-cli/src/simple_output.rs index 66377c7..924a284 100644 --- a/crates/g3-cli/src/simple_output.rs +++ b/crates/g3-cli/src/simple_output.rs @@ -27,7 +27,7 @@ impl SimpleOutput { /// Format: "g3: ... [status]" /// Uses centralized G3Status formatting. pub fn print_g3_status(&self, message: &str, status: &str) { - G3Status::complete(message, Status::from_str(status)); + G3Status::complete(message, Status::parse(status)); } /// Print a g3 status message in progress (no status yet) diff --git a/crates/g3-cli/src/ui_writer_impl.rs b/crates/g3-cli/src/ui_writer_impl.rs index 25d84d2..1315a5c 100644 --- a/crates/g3-cli/src/ui_writer_impl.rs +++ b/crates/g3-cli/src/ui_writer_impl.rs @@ -234,7 +234,7 @@ impl UiWriter for ConsoleUiWriter { fn print_g3_status(&self, message: &str, status: &str) { use crate::g3_status::Status; let _ = message; // unused now - progress already printed the message - crate::g3_status::G3Status::status(&Status::from_str(status)); + crate::g3_status::G3Status::status(&Status::parse(status)); } fn print_thin_result(&self, result: &g3_core::ThinResult) { diff --git a/crates/g3-core/src/streaming.rs b/crates/g3-core/src/streaming.rs index 018bdd9..e97aac7 100644 --- a/crates/g3-core/src/streaming.rs +++ b/crates/g3-core/src/streaming.rs @@ -1,8 +1,7 @@ //! Streaming completion logic for the Agent. //! -//! This module provides state management and helper functions for streaming -//! LLM responses, including tool call detection, deduplication, and -//! auto-continue decision logic. +//! State management and helpers for streaming LLM responses: tool call +//! detection, deduplication, and auto-continue logic. use crate::context_window::ContextWindow; use crate::streaming_parser::StreamingToolParser; @@ -11,7 +10,6 @@ use g3_providers::{CompletionRequest, MessageRole}; use std::time::{Duration, Instant}; use tracing::{debug, error}; -/// Constants for streaming behavior pub const MAX_ITERATIONS: usize = 400; /// State tracked across streaming iterations @@ -53,18 +51,13 @@ impl StreamingState { } } -/// Result of formatting a tool's output for display pub enum ToolOutputFormat { - /// Tool handles its own output (e.g., TODO tools) SelfHandled, - /// Compact one-line summary for file operations Compact(String), - /// Regular multi-line output (already displayed via ui_writer) Regular, } -/// Format a tool result for compact display. -/// Returns the appropriate format based on tool type and success. +/// Determine how to format a tool result for display. pub fn format_tool_result_summary( tool_name: &str, tool_result: &str, @@ -97,7 +90,6 @@ pub fn format_tool_result_summary( } } -/// Check if a tool is a "compact" tool that shows one-line summaries pub fn is_compact_tool(tool_name: &str) -> bool { matches!( tool_name, @@ -112,7 +104,6 @@ pub fn is_compact_tool(tool_name: &str) -> bool { ) } -/// Check if a tool handles its own output display pub fn is_self_handled_tool(tool_name: &str) -> bool { tool_name == "todo_read" || tool_name == "todo_write" }