From e57257bd55ae13114828bc9b91c995556366300e Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 22 Feb 2026 12:36:50 -0400 Subject: [PATCH] Simplify coding-agent-loop: BaseProfile, &str returns, EnvContext renames, typed errors - 2.2: Introduce BaseProfile struct with shared id/model/registry fields; AnthropicProfile, GeminiProfile, OpenAiProfile now delegate to it - 5.4: ProviderProfile::id() and ::model() return &str instead of owned String, eliminating unnecessary heap allocations on every call - 6.1: Rename EnvContext fields: date -> current_date, model_name -> model for consistency with trait method names - 8.1: Mark build_env_context_block (no-context variant) as #[cfg(test)] since it is only used in one test - 9.1: Convert SubAgentManager methods (spawn, send_input, wait, close) from Result to Result; tool executors convert at boundary via .map_err(|e| e.to_string()) Co-Authored-By: Claude Opus 4.6 --- .../src/profiles/anthropic.rs | 27 ++++---- .../coding-agent-loop/src/profiles/gemini.rs | 23 ++++--- crates/coding-agent-loop/src/profiles/mod.rs | 28 ++++++--- .../coding-agent-loop/src/profiles/openai.rs | 23 ++++--- .../coding-agent-loop/src/provider_profile.rs | 12 ++-- crates/coding-agent-loop/src/session.rs | 12 ++-- crates/coding-agent-loop/src/subagent.rs | 61 +++++++++++-------- crates/coding-agent-loop/src/test_support.rs | 16 ++--- 8 files changed, 116 insertions(+), 86 deletions(-) diff --git a/crates/coding-agent-loop/src/profiles/anthropic.rs b/crates/coding-agent-loop/src/profiles/anthropic.rs index 558a4e376..98d254ce8 100644 --- a/crates/coding-agent-loop/src/profiles/anthropic.rs +++ b/crates/coding-agent-loop/src/profiles/anthropic.rs @@ -1,6 +1,7 @@ use crate::config::SessionConfig; use crate::execution_env::ExecutionEnvironment; use crate::profiles::assemble_system_prompt; +use crate::profiles::BaseProfile; use crate::provider_profile::{ProfileCapabilities, ProviderProfile}; use crate::tool_registry::ToolRegistry; use crate::tools::{ @@ -11,8 +12,7 @@ use crate::tools::{ use super::EnvContext; pub struct AnthropicProfile { - model: String, - registry: ToolRegistry, + base: BaseProfile, } impl AnthropicProfile { @@ -32,27 +32,30 @@ impl AnthropicProfile { registry.register(make_glob_tool()); Self { - model: model.into(), - registry, + base: BaseProfile { + id: "anthropic", + model: model.into(), + registry, + }, } } } impl ProviderProfile for AnthropicProfile { - fn id(&self) -> String { - "anthropic".into() + fn id(&self) -> &str { + self.base.id } - fn model(&self) -> String { - self.model.clone() + fn model(&self) -> &str { + &self.base.model } fn tool_registry(&self) -> &ToolRegistry { - &self.registry + &self.base.registry } fn tool_registry_mut(&mut self) -> &mut ToolRegistry { - &mut self.registry + &mut self.base.registry } fn build_system_prompt( @@ -236,8 +239,8 @@ mod tests { let ctx = EnvContext { git_branch: Some("feature-branch".into()), is_git_repo: true, - date: "2026-02-20".into(), - model_name: "claude-opus-4-6".into(), + current_date: "2026-02-20".into(), + model: "claude-opus-4-6".into(), knowledge_cutoff: "May 2025".into(), git_status_short: None, git_recent_commits: None, diff --git a/crates/coding-agent-loop/src/profiles/gemini.rs b/crates/coding-agent-loop/src/profiles/gemini.rs index bd84c4274..51e077f0b 100644 --- a/crates/coding-agent-loop/src/profiles/gemini.rs +++ b/crates/coding-agent-loop/src/profiles/gemini.rs @@ -1,5 +1,6 @@ use crate::execution_env::ExecutionEnvironment; use crate::profiles::assemble_system_prompt; +use crate::profiles::BaseProfile; use crate::provider_profile::{ProfileCapabilities, ProviderProfile}; use crate::tool_registry::ToolRegistry; use crate::tools::{ @@ -11,8 +12,7 @@ use crate::tools::{ use super::EnvContext; pub struct GeminiProfile { - model: String, - registry: ToolRegistry, + base: BaseProfile, } impl GeminiProfile { @@ -32,27 +32,30 @@ impl GeminiProfile { registry.register(make_web_fetch_tool()); Self { - model: model.into(), - registry, + base: BaseProfile { + id: "gemini", + model: model.into(), + registry, + }, } } } impl ProviderProfile for GeminiProfile { - fn id(&self) -> String { - "gemini".into() + fn id(&self) -> &str { + self.base.id } - fn model(&self) -> String { - self.model.clone() + fn model(&self) -> &str { + &self.base.model } fn tool_registry(&self) -> &ToolRegistry { - &self.registry + &self.base.registry } fn tool_registry_mut(&mut self) -> &mut ToolRegistry { - &mut self.registry + &mut self.base.registry } fn build_system_prompt( diff --git a/crates/coding-agent-loop/src/profiles/mod.rs b/crates/coding-agent-loop/src/profiles/mod.rs index b2be81067..d1bac83de 100644 --- a/crates/coding-agent-loop/src/profiles/mod.rs +++ b/crates/coding-agent-loop/src/profiles/mod.rs @@ -7,14 +7,25 @@ pub use gemini::GeminiProfile; pub use openai::OpenAiProfile; use crate::execution_env::ExecutionEnvironment; +use crate::tool_registry::ToolRegistry; + +/// Common fields shared by all provider profiles. +/// +/// Each concrete profile embeds this struct and delegates `id()`, `model()`, +/// `tool_registry()`, and `tool_registry_mut()` to it. +pub struct BaseProfile { + pub id: &'static str, + pub model: String, + pub registry: ToolRegistry, +} /// Additional context for building environment blocks #[derive(Default)] pub struct EnvContext { pub git_branch: Option, pub is_git_repo: bool, - pub date: String, - pub model_name: String, + pub current_date: String, + pub model: String, pub knowledge_cutoff: String, pub git_status_short: Option, pub git_recent_commits: Option, @@ -47,6 +58,7 @@ pub fn assemble_system_prompt( format!("{prompt}{docs_section}{user_section}") } +#[cfg(test)] #[must_use] pub fn build_env_context_block(env: &dyn ExecutionEnvironment) -> String { build_env_context_block_with(env, &EnvContext::default()) @@ -67,11 +79,11 @@ pub fn build_env_context_block_with(env: &dyn ExecutionEnvironment, ctx: &EnvCon lines.push(format!("Platform: {}", env.platform())); lines.push(format!("OS version: {}", env.os_version())); - if !ctx.date.is_empty() { - lines.push(format!("Today's date: {}", ctx.date)); + if !ctx.current_date.is_empty() { + lines.push(format!("Today's date: {}", ctx.current_date)); } - if !ctx.model_name.is_empty() { - lines.push(format!("Model: {}", ctx.model_name)); + if !ctx.model.is_empty() { + lines.push(format!("Model: {}", ctx.model)); } if !ctx.knowledge_cutoff.is_empty() { lines.push(format!("Knowledge cutoff: {}", ctx.knowledge_cutoff)); @@ -119,8 +131,8 @@ mod tests { let ctx = EnvContext { git_branch: Some("main".into()), is_git_repo: true, - date: "2026-02-20".into(), - model_name: "claude-opus-4-6".into(), + current_date: "2026-02-20".into(), + model: "claude-opus-4-6".into(), knowledge_cutoff: "May 2025".into(), git_status_short: None, git_recent_commits: None, diff --git a/crates/coding-agent-loop/src/profiles/openai.rs b/crates/coding-agent-loop/src/profiles/openai.rs index 2fe1e9e05..6387ae19b 100644 --- a/crates/coding-agent-loop/src/profiles/openai.rs +++ b/crates/coding-agent-loop/src/profiles/openai.rs @@ -1,5 +1,6 @@ use crate::execution_env::ExecutionEnvironment; use crate::profiles::assemble_system_prompt; +use crate::profiles::BaseProfile; use crate::provider_profile::{ProfileCapabilities, ProviderProfile}; use crate::tool_registry::{RegisteredTool, ToolRegistry}; use unified_llm::types::ToolDefinition; @@ -11,8 +12,7 @@ use std::sync::Arc; use super::EnvContext; pub struct OpenAiProfile { - model: String, - registry: ToolRegistry, + base: BaseProfile, reasoning_effort: Option, } @@ -29,8 +29,11 @@ impl OpenAiProfile { registry.register(make_apply_patch_tool()); Self { - model: model.into(), - registry, + base: BaseProfile { + id: "openai", + model: model.into(), + registry, + }, reasoning_effort: None, } } @@ -41,20 +44,20 @@ impl OpenAiProfile { } impl ProviderProfile for OpenAiProfile { - fn id(&self) -> String { - "openai".into() + fn id(&self) -> &str { + self.base.id } - fn model(&self) -> String { - self.model.clone() + fn model(&self) -> &str { + &self.base.model } fn tool_registry(&self) -> &ToolRegistry { - &self.registry + &self.base.registry } fn tool_registry_mut(&mut self) -> &mut ToolRegistry { - &mut self.registry + &mut self.base.registry } fn build_system_prompt( diff --git a/crates/coding-agent-loop/src/provider_profile.rs b/crates/coding-agent-loop/src/provider_profile.rs index 99c7f5d5f..91c9fa38e 100644 --- a/crates/coding-agent-loop/src/provider_profile.rs +++ b/crates/coding-agent-loop/src/provider_profile.rs @@ -17,8 +17,8 @@ pub struct ProfileCapabilities { } pub trait ProviderProfile: Send + Sync { - fn id(&self) -> String; - fn model(&self) -> String; + fn id(&self) -> &str; + fn model(&self) -> &str; fn tool_registry(&self) -> &ToolRegistry; fn tool_registry_mut(&mut self) -> &mut ToolRegistry; fn build_system_prompt( @@ -96,11 +96,11 @@ mod tests { } impl ProviderProfile for ProviderTestProfile { - fn id(&self) -> String { - "test-provider".into() + fn id(&self) -> &str { + "test-provider" } - fn model(&self) -> String { - "test-model".into() + fn model(&self) -> &str { + "test-model" } fn tool_registry(&self) -> &ToolRegistry { &self.registry diff --git a/crates/coding-agent-loop/src/session.rs b/crates/coding-agent-loop/src/session.rs index 980ec72e2..d0d6cb678 100644 --- a/crates/coding-agent-loop/src/session.rs +++ b/crates/coding-agent-loop/src/session.rs @@ -71,7 +71,7 @@ impl Session { self.execution_env.as_ref(), &doc_root, self.execution_env.working_directory(), - &self.provider_profile.id(), + self.provider_profile.id(), ) .await; @@ -81,7 +81,7 @@ impl Session { async fn build_env_context(&self) -> EnvContext { let today = chrono::Local::now().format("%Y-%m-%d").to_string(); - let model_name = self.provider_profile.model(); + let model_name = self.provider_profile.model().to_string(); // Detect git info via execution environment let git_branch = self @@ -121,8 +121,8 @@ impl Session { EnvContext { git_branch, is_git_repo, - date: today, - model_name, + current_date: today, + model: model_name, knowledge_cutoff: self.provider_profile.knowledge_cutoff().to_string(), git_status_short, git_recent_commits, @@ -382,9 +382,9 @@ impl Session { let has_tools = !tools.is_empty(); Request { - model: self.provider_profile.model(), + model: self.provider_profile.model().to_string(), messages, - provider: Some(self.provider_profile.id()), + provider: Some(self.provider_profile.id().to_string()), tools: if has_tools { Some(tools) } else { None }, tool_choice: if has_tools { Some(ToolChoice::Auto) diff --git a/crates/coding-agent-loop/src/subagent.rs b/crates/coding-agent-loop/src/subagent.rs index a61ca7921..9dcc75535 100644 --- a/crates/coding-agent-loop/src/subagent.rs +++ b/crates/coding-agent-loop/src/subagent.rs @@ -52,12 +52,12 @@ impl SubAgentManager { mut session: Session, task_prompt: String, depth: usize, - ) -> Result { + ) -> Result { if depth >= self.max_depth { - return Err(format!( + return Err(AgentError::InvalidState(format!( "Maximum subagent depth ({}) reached", self.max_depth - )); + ))); } let agent_id = uuid::Uuid::new_v4().to_string(); @@ -65,9 +65,8 @@ impl SubAgentManager { let abort_flag = session.abort_flag_handle(); let task = tokio::spawn(async move { - let result = session.process_input(&task_prompt).await; + session.process_input(&task_prompt).await?; let turns = session.history().turns(); - let turns_used = turns.len(); let last_text = turns.iter().rev().find_map(|t| { if let Turn::Assistant { content, .. } = t { Some(content.clone()) @@ -75,14 +74,10 @@ impl SubAgentManager { None } }); - let success = result.is_ok(); - if let Err(e) = result { - return Err(e); - } Ok(SubAgentResult { output: last_text.unwrap_or_default(), - success, - turns_used, + success: true, + turns_used: turns.len(), }) }); @@ -100,11 +95,13 @@ impl SubAgentManager { Ok(agent_id) } - pub fn send_input(&self, agent_id: &str, message: &str) -> Result<(), String> { + pub fn send_input(&self, agent_id: &str, message: &str) -> Result<(), AgentError> { let agent = self .agents .get(agent_id) - .ok_or_else(|| format!("No agent found with id: {agent_id}"))?; + .ok_or_else(|| { + AgentError::InvalidState(format!("No agent found with id: {agent_id}")) + })?; agent .followup_queue @@ -115,26 +112,34 @@ impl SubAgentManager { Ok(()) } - pub async fn wait(&mut self, agent_id: &str) -> Result { + pub async fn wait(&mut self, agent_id: &str) -> Result { let mut agent = self .agents .remove(agent_id) - .ok_or_else(|| format!("No agent found with id: {agent_id}"))?; + .ok_or_else(|| { + AgentError::InvalidState(format!("No agent found with id: {agent_id}")) + })?; match agent.task.take() { Some(join_handle) => match join_handle.await { - Ok(result) => result.map_err(|e| e.to_string()), - Err(e) => Err(format!("Agent task panicked: {e}")), + Ok(result) => result, + Err(e) => Err(AgentError::InvalidState(format!( + "Agent task panicked: {e}" + ))), }, - None => Err(format!("Agent {agent_id} has no running task")), + None => Err(AgentError::InvalidState(format!( + "Agent {agent_id} has no running task" + ))), } } - pub fn close(&mut self, agent_id: &str) -> Result<(), String> { + pub fn close(&mut self, agent_id: &str) -> Result<(), AgentError> { let agent = self .agents .remove(agent_id) - .ok_or_else(|| format!("No agent found with id: {agent_id}"))?; + .ok_or_else(|| { + AgentError::InvalidState(format!("No agent found with id: {agent_id}")) + })?; agent.abort_flag.store(true, Ordering::SeqCst); @@ -204,6 +209,7 @@ pub fn make_spawn_agent_tool( session.set_max_turns(max_turns.unwrap_or(50)); let mut mgr = manager.lock().await; mgr.spawn(session, task.to_string(), current_depth) + .map_err(|e| e.to_string()) }) }), } @@ -244,7 +250,8 @@ pub fn make_send_input_tool( .ok_or_else(|| "Missing required parameter: message".to_string())?; let mgr = manager.lock().await; - mgr.send_input(agent_id, message)?; + mgr.send_input(agent_id, message) + .map_err(|e| e.to_string())?; Ok(format!("Message sent to agent {agent_id}")) }) }), @@ -278,7 +285,8 @@ pub fn make_wait_tool( .ok_or_else(|| "Missing required parameter: agent_id".to_string())?; let mut mgr = manager.lock().await; - let result = mgr.wait(agent_id).await?; + let result = mgr.wait(agent_id).await + .map_err(|e| e.to_string())?; Ok(format!( "Agent completed (success: {}, turns: {})\n\n{}", result.success, result.turns_used, result.output @@ -315,7 +323,8 @@ pub fn make_close_agent_tool( .ok_or_else(|| "Missing required parameter: agent_id".to_string())?; let mut mgr = manager.lock().await; - mgr.close(agent_id)?; + mgr.close(agent_id) + .map_err(|e| e.to_string())?; Ok(format!("Agent {agent_id} closed")) }) }), @@ -354,7 +363,7 @@ mod tests { let session = make_session(vec![text_response("Hello")]).await; let result = manager.spawn(session, "Do something".into(), 2); assert!(result.is_err()); - assert!(result.unwrap_err().contains("Maximum subagent depth")); + assert!(result.unwrap_err().to_string().contains("Maximum subagent depth")); } #[tokio::test] @@ -374,7 +383,7 @@ mod tests { let manager = SubAgentManager::new(3); let result = manager.send_input("nonexistent-id", "hello"); assert!(result.is_err()); - assert!(result.unwrap_err().contains("No agent found")); + assert!(result.unwrap_err().to_string().contains("No agent found")); } #[tokio::test] @@ -382,7 +391,7 @@ mod tests { let mut manager = SubAgentManager::new(3); let result = manager.wait("nonexistent-id").await; assert!(result.is_err()); - assert!(result.unwrap_err().contains("No agent found")); + assert!(result.unwrap_err().to_string().contains("No agent found")); } #[tokio::test] diff --git a/crates/coding-agent-loop/src/test_support.rs b/crates/coding-agent-loop/src/test_support.rs index 95678f22e..4f7abc871 100644 --- a/crates/coding-agent-loop/src/test_support.rs +++ b/crates/coding-agent-loop/src/test_support.rs @@ -142,12 +142,12 @@ impl TestProfile { } impl ProviderProfile for TestProfile { - fn id(&self) -> String { - "mock".into() + fn id(&self) -> &str { + "mock" } - fn model(&self) -> String { - "mock-model".into() + fn model(&self) -> &str { + "mock-model" } fn tool_registry(&self) -> &ToolRegistry { @@ -379,12 +379,12 @@ impl ParallelTestProfile { } impl ProviderProfile for ParallelTestProfile { - fn id(&self) -> String { - "mock".into() + fn id(&self) -> &str { + "mock" } - fn model(&self) -> String { - "mock-model".into() + fn model(&self) -> &str { + "mock-model" } fn tool_registry(&self) -> &ToolRegistry {