From cdb8b0f5eb78857d68d4e193081576e40ba47069 Mon Sep 17 00:00:00 2001 From: "Dhanji R. Prasanna" Date: Tue, 27 Jan 2026 12:01:12 +1100 Subject: [PATCH] refactor(g3-core): consolidate Agent construction into single canonical path Eliminate code-path aliasing in Agent construction methods by introducing a single `build_agent()` helper that all constructors delegate to. Before: 3 nearly-identical `Ok(Self { ... })` blocks (~30 lines each) with subtle differences in auto_compact, is_autonomous, quiet, and computer_controller fields - prone to drift over time. After: Single canonical `build_agent()` method that constructs Agent with all fields. All public constructors delegate to this single path: - new_for_test() -> new_for_test_with_readme() -> build_agent() - new_with_mode_and_readme() -> build_agent() Changes: - Add `build_agent()` private helper method (single source of truth) - Simplify `new_for_test()` to delegate to `new_for_test_with_readme()` - Update `new_for_test_with_readme()` to use `build_agent()` - Update `new_with_mode_and_readme()` to use `build_agent()` Net reduction: ~43 lines (-109/+66) All 190 tests pass. Agent: fowler --- crates/g3-core/src/lib.rs | 175 ++++++++++++++------------------------ 1 file changed, 66 insertions(+), 109 deletions(-) diff --git a/crates/g3-core/src/lib.rs b/crates/g3-core/src/lib.rs index 41d7371..1a77fa4 100644 --- a/crates/g3-core/src/lib.rs +++ b/crates/g3-core/src/lib.rs @@ -157,6 +157,55 @@ impl Agent { // CONSTRUCTION METHODS // ========================================================================= + /// Internal helper to construct an Agent with all fields. + /// This is the single canonical constructor - all public constructors delegate here. + /// This eliminates code duplication and ensures all Agent instances are built consistently. + fn build_agent( + config: Config, + ui_writer: W, + providers: ProviderRegistry, + context_window: ContextWindow, + auto_compact: bool, + is_autonomous: bool, + quiet: bool, + computer_controller: Option>, + ) -> Self { + Self { + providers, + context_window, + auto_compact, + pending_90_compaction: false, + thinning_events: Vec::new(), + compaction_events: Vec::new(), + first_token_times: Vec::new(), + cache_stats: CacheStats::default(), + config, + session_id: None, + tool_call_metrics: Vec::new(), + ui_writer, + todo_content: std::sync::Arc::new(tokio::sync::RwLock::new(String::new())), + is_autonomous, + quiet, + computer_controller, + webdriver_session: std::sync::Arc::new(tokio::sync::RwLock::new(None)), + webdriver_process: std::sync::Arc::new(tokio::sync::RwLock::new(None)), + tool_call_count: 0, + tool_calls_this_turn: Vec::new(), + requirements_sha: None, + working_dir: None, + background_process_manager: std::sync::Arc::new( + background_process::BackgroundProcessManager::new( + paths::get_background_processes_dir(), + ), + ), + pending_images: Vec::new(), + is_agent_mode: false, + agent_name: None, + auto_memory: false, + acd_enabled: false, + } + } + pub async fn new(config: Config, ui_writer: W) -> Result { Self::new_with_mode(config, ui_writer, false, false).await } @@ -212,57 +261,12 @@ impl Agent { ui_writer: W, providers: ProviderRegistry, ) -> Result { - use crate::context_window::ContextWindow; - use crate::prompts::get_system_prompt_for_native; - use g3_providers::{Message, MessageRole}; - - // Use a reasonable default context length for tests - let context_length = config.agent.max_context_length.unwrap_or(200_000); - let mut context_window = ContextWindow::new(context_length); - - // Add system prompt - let system_prompt = get_system_prompt_for_native(); - let system_message = Message::new(MessageRole::System, system_prompt); - context_window.add_message(system_message); - - Ok(Self { - providers, - context_window, - auto_compact: false, - pending_90_compaction: false, - thinning_events: Vec::new(), - compaction_events: Vec::new(), - first_token_times: Vec::new(), - cache_stats: CacheStats::default(), - config, - session_id: None, - tool_call_metrics: Vec::new(), - ui_writer, - todo_content: std::sync::Arc::new(tokio::sync::RwLock::new(String::new())), - is_autonomous: false, - quiet: true, - computer_controller: None, - webdriver_session: std::sync::Arc::new(tokio::sync::RwLock::new(None)), - webdriver_process: std::sync::Arc::new(tokio::sync::RwLock::new(None)), - tool_call_count: 0, - tool_calls_this_turn: Vec::new(), - requirements_sha: None, - working_dir: None, - background_process_manager: std::sync::Arc::new( - background_process::BackgroundProcessManager::new( - paths::get_background_processes_dir(), - ), - ), - pending_images: Vec::new(), - is_agent_mode: false, - agent_name: None, - auto_memory: false, - acd_enabled: false, - }) + Self::new_for_test_with_readme(config, ui_writer, providers, None).await } /// Create a new agent for testing with README content. /// This allows tests to verify context window structure with combined content. + #[doc(hidden)] pub async fn new_for_test_with_readme( config: Config, ui_writer: W, @@ -287,40 +291,17 @@ impl Agent { context_window.add_message(readme_message); } - Ok(Self { + // For tests: auto_compact=false, is_autonomous=false, quiet=true, no computer_controller + Ok(Self::build_agent( + config, + ui_writer, providers, context_window, - auto_compact: false, - pending_90_compaction: false, - thinning_events: Vec::new(), - compaction_events: Vec::new(), - first_token_times: Vec::new(), - cache_stats: CacheStats::default(), - config, - session_id: None, - tool_call_metrics: Vec::new(), - ui_writer, - todo_content: std::sync::Arc::new(tokio::sync::RwLock::new(String::new())), - is_autonomous: false, - quiet: true, - computer_controller: None, - webdriver_session: std::sync::Arc::new(tokio::sync::RwLock::new(None)), - webdriver_process: std::sync::Arc::new(tokio::sync::RwLock::new(None)), - tool_call_count: 0, - tool_calls_this_turn: Vec::new(), - requirements_sha: None, - working_dir: None, - background_process_manager: std::sync::Arc::new( - background_process::BackgroundProcessManager::new( - paths::get_background_processes_dir(), - ), - ), - pending_images: Vec::new(), - is_agent_mode: false, - agent_name: None, - auto_memory: false, - acd_enabled: false, - }) + false, // auto_compact + false, // is_autonomous + true, // quiet + None, // computer_controller + )) } async fn new_with_mode( @@ -403,41 +384,17 @@ impl Agent { None }; - Ok(Self { + let auto_compact = config.agent.auto_compact; + Ok(Self::build_agent( + config, + ui_writer, providers, context_window, - auto_compact: config.agent.auto_compact, - pending_90_compaction: false, - thinning_events: Vec::new(), - compaction_events: Vec::new(), - first_token_times: Vec::new(), - cache_stats: CacheStats::default(), - config, - session_id: None, - tool_call_metrics: Vec::new(), - ui_writer, - // TODO content starts empty - session-scoped TODOs are loaded via todo_read - todo_content: std::sync::Arc::new(tokio::sync::RwLock::new(String::new())), + auto_compact, is_autonomous, quiet, computer_controller, - webdriver_session: std::sync::Arc::new(tokio::sync::RwLock::new(None)), - webdriver_process: std::sync::Arc::new(tokio::sync::RwLock::new(None)), - tool_call_count: 0, - tool_calls_this_turn: Vec::new(), - requirements_sha: None, - working_dir: None, - background_process_manager: std::sync::Arc::new( - background_process::BackgroundProcessManager::new( - paths::get_background_processes_dir(), - ), - ), - pending_images: Vec::new(), - is_agent_mode: false, - agent_name: None, - auto_memory: false, - acd_enabled: false, - }) + )) } // =========================================================================