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
This commit is contained in:
@@ -157,6 +157,55 @@ impl<W: UiWriter> Agent<W> {
|
|||||||
// CONSTRUCTION METHODS
|
// 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<Box<dyn g3_computer_control::ComputerController>>,
|
||||||
|
) -> 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> {
|
pub async fn new(config: Config, ui_writer: W) -> Result<Self> {
|
||||||
Self::new_with_mode(config, ui_writer, false, false).await
|
Self::new_with_mode(config, ui_writer, false, false).await
|
||||||
}
|
}
|
||||||
@@ -212,57 +261,12 @@ impl<W: UiWriter> Agent<W> {
|
|||||||
ui_writer: W,
|
ui_writer: W,
|
||||||
providers: ProviderRegistry,
|
providers: ProviderRegistry,
|
||||||
) -> Result<Self> {
|
) -> Result<Self> {
|
||||||
use crate::context_window::ContextWindow;
|
Self::new_for_test_with_readme(config, ui_writer, providers, None).await
|
||||||
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,
|
|
||||||
})
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Create a new agent for testing with README content.
|
/// Create a new agent for testing with README content.
|
||||||
/// This allows tests to verify context window structure with combined content.
|
/// This allows tests to verify context window structure with combined content.
|
||||||
|
#[doc(hidden)]
|
||||||
pub async fn new_for_test_with_readme(
|
pub async fn new_for_test_with_readme(
|
||||||
config: Config,
|
config: Config,
|
||||||
ui_writer: W,
|
ui_writer: W,
|
||||||
@@ -287,40 +291,17 @@ impl<W: UiWriter> Agent<W> {
|
|||||||
context_window.add_message(readme_message);
|
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,
|
providers,
|
||||||
context_window,
|
context_window,
|
||||||
auto_compact: false,
|
false, // auto_compact
|
||||||
pending_90_compaction: false,
|
false, // is_autonomous
|
||||||
thinning_events: Vec::new(),
|
true, // quiet
|
||||||
compaction_events: Vec::new(),
|
None, // computer_controller
|
||||||
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,
|
|
||||||
})
|
|
||||||
}
|
}
|
||||||
|
|
||||||
async fn new_with_mode(
|
async fn new_with_mode(
|
||||||
@@ -403,41 +384,17 @@ impl<W: UiWriter> Agent<W> {
|
|||||||
None
|
None
|
||||||
};
|
};
|
||||||
|
|
||||||
Ok(Self {
|
let auto_compact = config.agent.auto_compact;
|
||||||
|
Ok(Self::build_agent(
|
||||||
|
config,
|
||||||
|
ui_writer,
|
||||||
providers,
|
providers,
|
||||||
context_window,
|
context_window,
|
||||||
auto_compact: config.agent.auto_compact,
|
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())),
|
|
||||||
is_autonomous,
|
is_autonomous,
|
||||||
quiet,
|
quiet,
|
||||||
computer_controller,
|
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,
|
|
||||||
})
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// =========================================================================
|
// =========================================================================
|
||||||
|
|||||||
Reference in New Issue
Block a user