mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-07 08:27:12 +00:00
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<T, String> to Result<T, AgentError>; tool executors convert at boundary via .map_err(|e| e.to_string()) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
c15b532bcf
commit
e57257bd55
8 changed files with 116 additions and 86 deletions
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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<String>,
|
||||
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<String>,
|
||||
pub git_recent_commits: Option<String>,
|
||||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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<String>,
|
||||
}
|
||||
|
||||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -52,12 +52,12 @@ impl SubAgentManager {
|
|||
mut session: Session,
|
||||
task_prompt: String,
|
||||
depth: usize,
|
||||
) -> Result<String, String> {
|
||||
) -> Result<String, AgentError> {
|
||||
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<SubAgentResult, String> {
|
||||
pub async fn wait(&mut self, agent_id: &str) -> Result<SubAgentResult, AgentError> {
|
||||
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]
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue