From 3a0b656161672ea0d4d41baa4967b21ea950c53e Mon Sep 17 00:00:00 2001 From: "Dhanji R. Prasanna" Date: Mon, 12 Jan 2026 08:57:49 +0530 Subject: [PATCH] refactor(g3-cli): eliminate code-path aliasing in config and project content loading Consolidate duplicated logic into canonical shared functions: - Extract load_config_with_cli_overrides() to utils.rs - Was duplicated in lib.rs and accumulative.rs with subtle differences - lib.rs version had Chrome diagnostics + provider validation - accumulative.rs version was missing both - Now all callers use the complete canonical implementation - Extract combine_project_content() to project_files.rs - Was duplicated inline in lib.rs and agent_mode.rs - Simplified implementation using iterator flatten - Added unit tests for all cases This eliminates drift risk where the duplicated implementations could diverge over time (accumulative.rs was already missing Chrome diagnostics and provider validation). Agent: fowler --- crates/g3-cli/src/accumulative.rs | 34 +------------- crates/g3-cli/src/agent_mode.rs | 24 +++------- crates/g3-cli/src/lib.rs | 75 +----------------------------- crates/g3-cli/src/project_files.rs | 42 +++++++++++++++++ crates/g3-cli/src/utils.rs | 55 ++++++++++++++++++++++ 5 files changed, 106 insertions(+), 124 deletions(-) diff --git a/crates/g3-cli/src/accumulative.rs b/crates/g3-cli/src/accumulative.rs index c92b7c1..7f8f544 100644 --- a/crates/g3-cli/src/accumulative.rs +++ b/crates/g3-cli/src/accumulative.rs @@ -7,7 +7,6 @@ use rustyline::DefaultEditor; use std::path::PathBuf; use tracing::error; -use g3_config::Config; use g3_core::project::Project; use g3_core::Agent; @@ -16,6 +15,7 @@ use crate::cli_args::Cli; use crate::interactive::run_interactive; use crate::simple_output::SimpleOutput; use crate::ui_writer_impl::ConsoleUiWriter; +use crate::utils::load_config_with_cli_overrides; /// Run accumulative autonomous mode - accumulates requirements from user input /// and runs autonomous mode after each input. @@ -309,35 +309,3 @@ async fn handle_command( _ => Ok(CommandResult::Unknown), } } - -fn load_config_with_cli_overrides(cli: &Cli) -> Result { - let mut config = Config::load_with_overrides( - cli.config.as_deref(), - cli.provider.clone(), - cli.model.clone(), - )?; - - // Apply webdriver flag override - if cli.webdriver { - config.webdriver.enabled = true; - } - - // Apply chrome-headless flag override - if cli.chrome_headless { - config.webdriver.enabled = true; - config.webdriver.browser = g3_config::WebDriverBrowser::ChromeHeadless; - } - - // Apply safari flag override - if cli.safari { - config.webdriver.enabled = true; - config.webdriver.browser = g3_config::WebDriverBrowser::Safari; - } - - // Apply no-auto-compact flag override - if cli.manual_compact { - config.agent.auto_compact = false; - } - - Ok(config) -} diff --git a/crates/g3-cli/src/agent_mode.rs b/crates/g3-cli/src/agent_mode.rs index 9a30649..a6f5c0e 100644 --- a/crates/g3-cli/src/agent_mode.rs +++ b/crates/g3-cli/src/agent_mode.rs @@ -7,7 +7,7 @@ use tracing::debug; use g3_core::ui_writer::UiWriter; use g3_core::Agent; -use crate::project_files::{read_agents_config, read_project_memory, read_project_readme}; +use crate::project_files::{combine_project_content, read_agents_config, read_project_memory, read_project_readme}; use crate::simple_output::SimpleOutput; use crate::ui_writer_impl::ConsoleUiWriter; @@ -177,23 +177,11 @@ pub async fn run_agent_mode( )); // Combine all content for the agent's context - let combined_content = { - let mut parts = Vec::new(); - if let Some(agents) = agents_content_opt { - parts.push(agents); - } - if let Some(readme) = readme_content_opt { - parts.push(readme); - } - if let Some(memory) = memory_content_opt { - parts.push(memory); - } - if parts.is_empty() { - None - } else { - Some(parts.join("\n\n")) - } - }; + let combined_content = combine_project_content( + agents_content_opt, + readme_content_opt, + memory_content_opt, + ); // Create agent with custom system prompt let ui_writer = ConsoleUiWriter::new(); diff --git a/crates/g3-cli/src/lib.rs b/crates/g3-cli/src/lib.rs index 8979247..0aa639a 100644 --- a/crates/g3-cli/src/lib.rs +++ b/crates/g3-cli/src/lib.rs @@ -31,10 +31,10 @@ use accumulative::run_accumulative_mode; use agent_mode::run_agent_mode; use autonomous::run_autonomous; use interactive::run_interactive; -use project_files::{read_agents_config, read_project_memory, read_project_readme}; +use project_files::{combine_project_content, read_agents_config, read_project_memory, read_project_readme}; use simple_output::SimpleOutput; use ui_writer_impl::ConsoleUiWriter; -use utils::setup_workspace_directory; +use utils::{load_config_with_cli_overrides, setup_workspace_directory}; pub async fn run() -> Result<()> { let cli = Cli::parse(); @@ -162,77 +162,6 @@ fn create_project(cli: &Cli, workspace_dir: &PathBuf) -> Result { } } -fn load_config_with_cli_overrides(cli: &Cli) -> Result { - let mut config = Config::load_with_overrides( - cli.config.as_deref(), - cli.provider.clone(), - cli.model.clone(), - )?; - - // Apply webdriver flag override - if cli.webdriver { - config.webdriver.enabled = true; - } - - // Apply chrome-headless flag override - if cli.chrome_headless { - config.webdriver.enabled = true; - config.webdriver.browser = g3_config::WebDriverBrowser::ChromeHeadless; - - // Run Chrome diagnostics on first use - let report = - g3_computer_control::run_chrome_diagnostics(config.webdriver.chrome_binary.as_deref()); - println!("{}", report.format_report()); - } - - // Apply safari flag override - if cli.safari { - config.webdriver.enabled = true; - config.webdriver.browser = g3_config::WebDriverBrowser::Safari; - } - - // Apply no-auto-compact flag override - if cli.manual_compact { - config.agent.auto_compact = false; - } - - // Validate provider if specified - if let Some(ref provider) = cli.provider { - let valid_providers = ["anthropic", "databricks", "embedded", "openai"]; - if !valid_providers.contains(&provider.as_str()) { - return Err(anyhow::anyhow!( - "Invalid provider '{}'. Valid options: {:?}", - provider, - valid_providers - )); - } - } - - Ok(config) -} - -fn combine_project_content( - agents_content: Option, - readme_content: Option, - memory_content: Option, -) -> Option { - let mut parts = Vec::new(); - if let Some(agents) = agents_content { - parts.push(agents); - } - if let Some(readme) = readme_content { - parts.push(readme); - } - if let Some(memory) = memory_content { - parts.push(memory); - } - if parts.is_empty() { - None - } else { - Some(parts.join("\n\n")) - } -} - async fn run_console_mode( cli: Cli, config: Config, diff --git a/crates/g3-cli/src/project_files.rs b/crates/g3-cli/src/project_files.rs index e74887d..740f140 100644 --- a/crates/g3-cli/src/project_files.rs +++ b/crates/g3-cli/src/project_files.rs @@ -82,6 +82,26 @@ pub fn read_project_memory(workspace_dir: &Path) -> Option { } } +/// Combine AGENTS.md, README, and memory content into a single string. +/// +/// Returns None if all inputs are None, otherwise joins non-None parts with double newlines. +pub fn combine_project_content( + agents_content: Option, + readme_content: Option, + memory_content: Option, +) -> Option { + let parts: Vec = [agents_content, readme_content, memory_content] + .into_iter() + .flatten() + .collect(); + + if parts.is_empty() { + None + } else { + Some(parts.join("\n\n")) + } +} + /// Format a byte size for display. fn format_size(len: usize) -> String { if len < 1000 { @@ -178,4 +198,26 @@ mod tests { assert!(truncated.ends_with("...")); assert_eq!(truncated.len(), 100); } + + #[test] + fn test_combine_project_content_all_some() { + let result = combine_project_content( + Some("agents".to_string()), + Some("readme".to_string()), + Some("memory".to_string()), + ); + assert_eq!(result, Some("agents\n\nreadme\n\nmemory".to_string())); + } + + #[test] + fn test_combine_project_content_partial() { + let result = combine_project_content(None, Some("readme".to_string()), None); + assert_eq!(result, Some("readme".to_string())); + } + + #[test] + fn test_combine_project_content_all_none() { + let result = combine_project_content(None, None, None); + assert_eq!(result, None); + } } diff --git a/crates/g3-cli/src/utils.rs b/crates/g3-cli/src/utils.rs index caab742..985770b 100644 --- a/crates/g3-cli/src/utils.rs +++ b/crates/g3-cli/src/utils.rs @@ -2,10 +2,12 @@ use anyhow::Result; use crossterm::style::{Color, ResetColor, SetForegroundColor}; +use g3_config::Config; use g3_core::ui_writer::UiWriter; use g3_core::Agent; use std::path::PathBuf; +use crate::cli_args::Cli; use crate::simple_output::SimpleOutput; /// Display context window progress bar. @@ -89,3 +91,56 @@ pub fn setup_workspace_directory() -> Result { Ok(workspace_dir) } + +/// Load configuration with CLI argument overrides applied. +/// +/// This is the canonical function for loading config with CLI overrides. +/// All CLI entry points should use this to ensure consistent behavior. +pub fn load_config_with_cli_overrides(cli: &Cli) -> Result { + let mut config = Config::load_with_overrides( + cli.config.as_deref(), + cli.provider.clone(), + cli.model.clone(), + )?; + + // Apply webdriver flag override + if cli.webdriver { + config.webdriver.enabled = true; + } + + // Apply chrome-headless flag override + if cli.chrome_headless { + config.webdriver.enabled = true; + config.webdriver.browser = g3_config::WebDriverBrowser::ChromeHeadless; + + // Run Chrome diagnostics on first use + let report = + g3_computer_control::run_chrome_diagnostics(config.webdriver.chrome_binary.as_deref()); + println!("{}", report.format_report()); + } + + // Apply safari flag override + if cli.safari { + config.webdriver.enabled = true; + config.webdriver.browser = g3_config::WebDriverBrowser::Safari; + } + + // Apply no-auto-compact flag override + if cli.manual_compact { + config.agent.auto_compact = false; + } + + // Validate provider if specified + if let Some(ref provider) = cli.provider { + let valid_providers = ["anthropic", "databricks", "embedded", "openai"]; + if !valid_providers.contains(&provider.as_str()) { + return Err(anyhow::anyhow!( + "Invalid provider '{}'. Valid options: {:?}", + provider, + valid_providers + )); + } + } + + Ok(config) +}