scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-10 22:43:37 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Simplify coding-agent-loop: deduplicate mocks, narrow traits, type events

Browse source 
- Extract shared test infrastructure (MockExecutionEnvironment, TestProfile,
  MockLlmProvider) replacing 11 duplicate mock implementations across tests
- Deduplicate tool execution logic between sequential and parallel paths
- Narrow ProviderProfile trait from 14 to 7 required methods via
  ProfileCapabilities struct and default implementations
- Replace stringly-typed HashMap event data with typed EventData enum
- Extract shared assemble_system_prompt helper and register_subagent_tools
  default method, eliminating copy-paste across all 3 profiles
- Replace fragile shell-based glob with glob crate, fix rg detection
- Add delete_file to ExecutionEnvironment, wire git context into env block
- Remove dead code (AgentError::Io, count_turns, trivial derived-trait tests)
- Use match-based lookups in truncation instead of per-call HashMap allocation

Net reduction: -1,401 lines across 20 files. All 180 tests pass.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-02-20 16:34:29 -04:00
parent 1ffc954a0f
commit db32f00cd0
22 changed files with 1581 additions and 2028 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

42
Cargo.lock generated
View file

@ -128,6 +128,24 @@ version = "1.1.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "1505bd5d3d116872e7271a6d4e16d81d0c8570876c8de68093a09ac269d8aac0"
[[package]]
name = "attractor"
version = "0.1.0"
dependencies = [
"async-trait",
"chrono",
"coding-agent-loop",
"nom",
"rand",
"serde",
"serde_json",
"tempfile",
"thiserror 2.0.18",
"tokio",
"unified-llm",
"uuid",
]
[[package]]
name = "autocfg"
version = "1.5.0"
@ -251,6 +269,7 @@ dependencies = [
"iana-time-zone",
"js-sys",
"num-traits",
"serde",
"wasm-bindgen",
"windows-link",
]
@ -311,6 +330,7 @@ dependencies = [
"async-trait",
"chrono",
"futures",
"glob",
"jsonschema",
"libc",
"serde",
@ -659,6 +679,12 @@ dependencies = [
"wasip3",
]
[[package]]
name = "glob"
version = "0.3.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "0cc23270f6e1808e30a928bdc84dea0b9b4136a8bc82338574f23baf47bbd280"
[[package]]
name = "h2"
version = "0.4.13"
@ -1122,6 +1148,12 @@ version = "0.3.17"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "6877bb514081ee2a7ff5ef9de3281f14a4dd4bceac4c09388074a6b5df8a139a"
[[package]]
name = "minimal-lexical"
version = "0.2.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "68354c5c6bd36d73ff3feceb05efa59b6acb7626617f4962be322a825e61f79a"
[[package]]
name = "mio"
version = "1.1.1"
@ -1150,6 +1182,16 @@ dependencies = [
"tempfile",
]
[[package]]
name = "nom"
version = "7.1.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "d273983c5a657a70a3e8f2a01329822f3b8c8172b73826411a55751e404a0a4a"
dependencies = [
"memchr",
"minimal-lexical",
]
[[package]]
name = "normalize-line-endings"
version = "0.3.0"

1
crates/coding-agent-loop/Cargo.toml
View file

@ -20,6 +20,7 @@ futures.workspace = true
async-trait.workspace = true
jsonschema.workspace = true
chrono.workspace = true
glob = "0.3"
[target.'cfg(unix)'.dependencies]
libc = "0.2"

9
crates/coding-agent-loop/src/error.rs
View file

@ -14,9 +14,6 @@ pub enum AgentError {
#[error("Tool execution error: {0}")]
ToolExecution(String),
#[error("IO error: {0}")]
Io(String),
#[error("Aborted")]
Aborted,
}
@ -53,12 +50,6 @@ mod tests {
assert_eq!(err.to_string(), "Tool execution error: command failed");
}
#[test]
fn io_error_display() {
let err = AgentError::Io("file not found".into());
assert_eq!(err.to_string(), "IO error: file not found");
}
#[test]
fn aborted_display() {
let err = AgentError::Aborted;

45
crates/coding-agent-loop/src/event.rs
View file

@ -1,5 +1,4 @@
use crate::types::{EventKind, SessionEvent};
use std::collections::HashMap;
use crate::types::{EventData, EventKind, SessionEvent};
use std::time::SystemTime;
use tokio::sync::broadcast;
@ -15,12 +14,7 @@ impl EventEmitter {
Self { sender }
}
pub fn emit(
&self,
kind: EventKind,
session_id: String,
data: HashMap<String, serde_json::Value>,
) {
pub fn emit(&self, kind: EventKind, session_id: String, data: EventData) {
let event = SessionEvent {
kind,
timestamp: SystemTime::now(),
@ -52,16 +46,12 @@ mod tests {
let emitter = EventEmitter::new();
let mut receiver = emitter.subscribe();
emitter.emit(
EventKind::SessionStart,
"sess-1".into(),
HashMap::new(),
);
emitter.emit(EventKind::SessionStart, "sess-1".into(), EventData::Empty);
let event = receiver.recv().await.unwrap();
assert_eq!(event.kind, EventKind::SessionStart);
assert_eq!(event.session_id, "sess-1");
assert!(event.data.is_empty());
assert!(matches!(event.data, EventData::Empty));
}
#[tokio::test]
@ -69,14 +59,19 @@ mod tests {
let emitter = EventEmitter::new();
let mut receiver = emitter.subscribe();
let mut data = HashMap::new();
data.insert("text".into(), serde_json::json!("hello world"));
emitter.emit(EventKind::AssistantTextDelta, "sess-2".into(), data);
emitter.emit(
EventKind::Error,
"sess-2".into(),
EventData::Error {
error: "something went wrong".into(),
},
);
let event = receiver.recv().await.unwrap();
assert_eq!(event.kind, EventKind::AssistantTextDelta);
assert_eq!(event.data["text"], serde_json::json!("hello world"));
assert_eq!(event.kind, EventKind::Error);
assert!(
matches!(&event.data, EventData::Error { error } if error == "something went wrong")
);
}
#[tokio::test]
@ -85,7 +80,7 @@ mod tests {
let mut rx1 = emitter.subscribe();
let mut rx2 = emitter.subscribe();
emitter.emit(EventKind::SessionEnd, "sess-3".into(), HashMap::new());
emitter.emit(EventKind::SessionEnd, "sess-3".into(), EventData::Empty);
let e1 = rx1.recv().await.unwrap();
let e2 = rx2.recv().await.unwrap();
@ -98,7 +93,13 @@ mod tests {
#[test]
fn emit_without_subscribers_does_not_panic() {
let emitter = EventEmitter::new();
emitter.emit(EventKind::Error, "sess-4".into(), HashMap::new());
emitter.emit(
EventKind::Error,
"sess-4".into(),
EventData::Error {
error: "test".into(),
},
);
}
#[test]

85
crates/coding-agent-loop/src/execution_env.rs
View file

@ -27,6 +27,7 @@ pub struct GrepOptions {
pub trait ExecutionEnvironment: Send + Sync {
async fn read_file(&self, path: &str, offset: Option<usize>, limit: Option<usize>) -> Result<String, String>;
async fn write_file(&self, path: &str, content: &str) -> Result<(), String>;
async fn delete_file(&self, path: &str) -> Result<(), String>;
async fn file_exists(&self, path: &str) -> Result<bool, String>;
async fn list_directory(&self, path: &str, depth: Option<usize>) -> Result<Vec<DirEntry>, String>;
async fn exec_command(
@ -53,81 +54,25 @@ pub trait ExecutionEnvironment: Send + Sync {
#[cfg(test)]
mod tests {
use super::*;
use crate::test_support::MockExecutionEnvironment;
use std::collections::HashMap;
use std::sync::Arc;
struct MockEnv;
#[async_trait]
impl ExecutionEnvironment for MockEnv {
async fn read_file(&self, _path: &str, _offset: Option<usize>, _limit: Option<usize>) -> Result<String, String> {
Ok("hello".into())
}
async fn write_file(&self, _path: &str, _content: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, _path: &str) -> Result<bool, String> {
Ok(true)
}
async fn list_directory(&self, _path: &str, _depth: Option<usize>) -> Result<Vec<DirEntry>, String> {
Ok(vec![DirEntry {
name: "test.rs".into(),
is_dir: false,
size: Some(100),
}])
}
async fn exec_command(
&self,
_command: &str,
_timeout_ms: u64,
_working_dir: Option<&str>,
_env_vars: Option<&std::collections::HashMap<String, String>>,
) -> Result<ExecResult, String> {
Ok(ExecResult {
stdout: "output".into(),
stderr: String::new(),
exit_code: 0,
timed_out: false,
duration_ms: 10,
})
}
async fn grep(
&self,
_pattern: &str,
_path: &str,
_options: &GrepOptions,
) -> Result<Vec<String>, String> {
Ok(vec!["match".into()])
}
async fn glob(&self, _pattern: &str, _path: Option<&str>) -> Result<Vec<String>, String> {
Ok(vec!["file.rs".into()])
}
async fn initialize(&self) -> Result<(), String> {
Ok(())
}
async fn cleanup(&self) -> Result<(), String> {
Ok(())
}
fn working_directory(&self) -> &str {
"/tmp"
}
fn platform(&self) -> &str {
"darwin"
}
fn os_version(&self) -> String {
"Darwin 24.0.0".into()
}
}
#[tokio::test]
async fn mock_env_read_file() {
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockEnv);
let mut files = HashMap::new();
files.insert("test.rs".into(), "hello".into());
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockExecutionEnvironment {
files,
..Default::default()
});
let result = env.read_file("test.rs", None, None).await.unwrap();
assert_eq!(result, "hello");
}
#[tokio::test]
async fn mock_env_exec_command() {
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockEnv);
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockExecutionEnvironment::default());
let result = env.exec_command("echo", 5000, None, None).await.unwrap();
assert_eq!(result.exit_code, 0);
assert!(!result.timed_out);
@ -135,11 +80,9 @@ mod tests {
#[tokio::test]
async fn mock_env_list_directory() {
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockEnv);
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockExecutionEnvironment::default());
let entries = env.list_directory("/tmp", None).await.unwrap();
assert_eq!(entries.len(), 1);
assert_eq!(entries[0].name, "test.rs");
assert!(!entries[0].is_dir);
assert_eq!(entries.len(), 0);
}
#[test]
@ -178,9 +121,9 @@ mod tests {
#[test]
fn mock_env_platform() {
let env = MockEnv;
let env = MockExecutionEnvironment::default();
assert_eq!(env.platform(), "darwin");
assert_eq!(env.working_directory(), "/tmp");
assert_eq!(env.working_directory(), "/tmp/test");
assert_eq!(env.os_version(), "Darwin 24.0.0");
}
}

14
crates/coding-agent-loop/src/history.rs
View file

@ -19,10 +19,6 @@ impl History {
&self.turns
}
pub fn count_turns(&self) -> usize {
self.turns.len()
}
pub fn convert_to_messages(&self) -> Vec<Message> {
self.turns
.iter()
@ -93,7 +89,7 @@ mod tests {
fn empty_history_produces_empty_messages() {
let history = History::new();
assert!(history.convert_to_messages().is_empty());
assert_eq!(history.count_turns(), 0);
assert_eq!(history.turns().len(), 0);
}
#[test]
@ -215,14 +211,14 @@ mod tests {
}
#[test]
fn count_turns_matches_push_count() {
fn turns_len_matches_push_count() {
let mut history = History::new();
assert_eq!(history.count_turns(), 0);
assert_eq!(history.turns().len(), 0);
history.push(Turn::User {
content: "First".into(),
timestamp: SystemTime::now(),
});
assert_eq!(history.count_turns(), 1);
assert_eq!(history.turns().len(), 1);
history.push(Turn::Assistant {
content: "Second".into(),
tool_calls: vec![],
@ -231,7 +227,7 @@ mod tests {
response_id: "resp_1".into(),
timestamp: SystemTime::now(),
});
assert_eq!(history.count_turns(), 2);
assert_eq!(history.turns().len(), 2);
}
#[test]

7
crates/coding-agent-loop/src/lib.rs
View file

@ -24,7 +24,7 @@ pub use local_env::LocalExecutionEnvironment;
pub use loop_detection::detect_loop;
pub use project_docs::discover_project_docs;
pub use profiles::{AnthropicProfile, EnvContext, GeminiProfile, OpenAiProfile};
pub use provider_profile::ProviderProfile;
pub use provider_profile::{ProfileCapabilities, ProviderProfile};
pub use session::Session;
pub use subagent::{SubAgent, SubAgentManager, SubAgentResult};
pub use tool_registry::ToolRegistry;
@ -33,4 +33,7 @@ pub use tools::{
make_shell_tool_with_config, make_write_file_tool,
};
pub use truncation::{truncate_lines, truncate_output, truncate_tool_output, TruncationMode};
pub use types::{EventKind, SessionEvent, SessionState, Turn};
pub use types::{EventData, EventKind, SessionEvent, SessionState, Turn};
#[cfg(test)]
pub(crate) mod test_support;

23
crates/coding-agent-loop/src/local_env.rs
View file

@ -77,6 +77,13 @@ impl ExecutionEnvironment for LocalExecutionEnvironment {
.map_err(|e| format!("Failed to write {}: {e}", full_path.display()))
}
async fn delete_file(&self, path: &str) -> Result<(), String> {
let full_path = self.resolve_path(path);
tokio::fs::remove_file(&full_path)
.await
.map_err(|e| format!("Failed to delete {}: {e}", full_path.display()))
}
async fn file_exists(&self, path: &str) -> Result<bool, String> {
let full_path = self.resolve_path(path);
Ok(full_path.exists())
@ -248,7 +255,8 @@ impl ExecutionEnvironment for LocalExecutionEnvironment {
.stdout(std::process::Stdio::null())
.stderr(std::process::Stdio::null())
.status()
.is_ok();
.map(|s| s.success())
.unwrap_or(false);
let output = if use_rg {
let mut args = vec!["-n".to_string()];
@ -309,14 +317,11 @@ impl ExecutionEnvironment for LocalExecutionEnvironment {
format!("{}/{pattern}", base_dir.display())
};
// Use shell globbing via ls
let output = std::process::Command::new("sh")
.args(["-c", &format!("ls -d {full_pattern} 2>/dev/null")])
.output()
.map_err(|e| format!("Failed to run glob: {e}"))?;
let stdout = String::from_utf8_lossy(&output.stdout);
let mut results: Vec<String> = stdout.lines().map(String::from).filter(|l| !l.is_empty()).collect();
let mut results: Vec<String> = glob::glob(&full_pattern)
.map_err(|e| format!("Invalid glob pattern: {e}"))?
.filter_map(Result::ok)
.map(|p| p.to_string_lossy().into_owned())
.collect();
// Sort by mtime (newest first)
results.sort_by(|a, b| {

147
crates/coding-agent-loop/src/profiles/anthropic.rs
View file

@ -1,19 +1,14 @@
use crate::config::SessionConfig;
use crate::execution_env::ExecutionEnvironment;
use crate::provider_profile::ProviderProfile;
use crate::subagent::{
make_close_agent_tool, make_send_input_tool, make_spawn_agent_tool, make_wait_tool,
SessionFactory, SubAgentManager,
};
use crate::profiles::assemble_system_prompt;
use crate::provider_profile::{ProfileCapabilities, ProviderProfile};
use crate::tool_registry::ToolRegistry;
use crate::tools::{
make_edit_file_tool, make_glob_tool, make_grep_tool, make_read_file_tool,
make_shell_tool_with_config, make_write_file_tool,
};
use std::sync::Arc;
use unified_llm::types::ToolDefinition;
use super::{build_env_context_block_with, EnvContext};
use super::EnvContext;
pub struct AnthropicProfile {
model: String,
@ -41,23 +36,6 @@ impl AnthropicProfile {
registry,
}
}
pub fn register_subagent_tools(
&mut self,
manager: Arc<tokio::sync::Mutex<SubAgentManager>>,
session_factory: SessionFactory,
current_depth: usize,
) {
self.registry.register(make_spawn_agent_tool(
manager.clone(),
session_factory,
current_depth,
));
self.registry
.register(make_send_input_tool(manager.clone()));
self.registry.register(make_wait_tool(manager.clone()));
self.registry.register(make_close_agent_tool(manager));
}
}
impl ProviderProfile for AnthropicProfile {
@ -84,19 +62,7 @@ impl ProviderProfile for AnthropicProfile {
project_docs: &[String],
user_instructions: Option<&str>,
) -> String {
let env_block = build_env_context_block_with(env, env_context);
let docs_section = if project_docs.is_empty() {
String::new()
} else {
format!("\n\n{}", project_docs.join("\n\n"))
};
let user_section = match user_instructions {
Some(instructions) => format!("\n\n# User Instructions\n{instructions}"),
None => String::new(),
};
format!(
"\
let core_prompt = "\
You are Claude, an AI coding assistant made by Anthropic. You help users with software \
engineering tasks including solving bugs, adding new functionality, refactoring code, \
explaining code, and more.
@ -163,14 +129,18 @@ finding files rather than using shell find or ls commands.
# Coding Best Practices
Write clean, maintainable code. Handle errors appropriately. Follow existing code conventions \
in the project. Keep changes minimal and focused on the task.\
{docs_section}\
{user_section}"
)
in the project. Keep changes minimal and focused on the task.";
assemble_system_prompt(core_prompt, env, env_context, project_docs, user_instructions)
}
fn tools(&self) -> Vec<ToolDefinition> {
self.registry.definitions()
fn capabilities(&self) -> ProfileCapabilities {
ProfileCapabilities {
supports_reasoning: true,
supports_streaming: true,
supports_parallel_tool_calls: true,
context_window_size: 200_000,
}
}
fn provider_options(&self) -> Option<serde_json::Value> {
@ -181,22 +151,6 @@ in the project. Keep changes minimal and focused on the task.\
}))
}
fn supports_reasoning(&self) -> bool {
true
}
fn supports_streaming(&self) -> bool {
true
}
fn supports_parallel_tool_calls(&self) -> bool {
true
}
fn context_window_size(&self) -> usize {
200_000
}
fn knowledge_cutoff(&self) -> &str {
"May 2025"
}
@ -205,65 +159,14 @@ in the project. Keep changes minimal and focused on the task.\
#[cfg(test)]
mod tests {
use super::*;
use crate::execution_env::*;
use async_trait::async_trait;
use crate::test_support::MockExecutionEnvironment;
struct TestEnv;
#[async_trait]
impl ExecutionEnvironment for TestEnv {
async fn read_file(&self, _: &str, _: Option<usize>, _: Option<usize>) -> Result<String, String> {
Ok(String::new())
}
async fn write_file(&self, _: &str, _: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, _: &str) -> Result<bool, String> {
Ok(false)
}
async fn list_directory(&self, _: &str, _: Option<usize>) -> Result<Vec<DirEntry>, String> {
Ok(vec![])
}
async fn exec_command(
&self,
_: &str,
_: u64,
_: Option<&str>,
_: Option<&std::collections::HashMap<String, String>>,
) -> Result<ExecResult, String> {
Ok(ExecResult {
stdout: String::new(),
stderr: String::new(),
exit_code: 0,
timed_out: false,
duration_ms: 0,
})
}
async fn grep(
&self,
_: &str,
_: &str,
_: &GrepOptions,
) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn glob(&self, _: &str, _: Option<&str>) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn initialize(&self) -> Result<(), String> {
Ok(())
}
async fn cleanup(&self) -> Result<(), String> {
Ok(())
}
fn working_directory(&self) -> &str {
"/home/test"
}
fn platform(&self) -> &str {
"linux"
}
fn os_version(&self) -> String {
"Linux 6.1.0".into()
fn linux_env() -> MockExecutionEnvironment {
MockExecutionEnvironment {
working_dir: "/home/test",
platform_str: "linux",
os_version_str: "Linux 6.1.0".into(),
..Default::default()
}
}
@ -286,7 +189,7 @@ mod tests {
#[test]
fn anthropic_system_prompt_contains_env_context() {
let profile = AnthropicProfile::new("claude-sonnet-4-20250514");
let env = TestEnv;
let env = linux_env();
let prompt = profile.build_system_prompt(&env, &EnvContext::default(), &[], None);
assert!(prompt.contains("You are Claude, an AI coding assistant made by Anthropic"));
assert!(prompt.contains("<environment>"));
@ -319,7 +222,7 @@ mod tests {
#[test]
fn anthropic_system_prompt_includes_project_docs() {
let profile = AnthropicProfile::new("claude-sonnet-4-20250514");
let env = TestEnv;
let env = linux_env();
let docs = vec!["# Project README".into(), "# CONTRIBUTING guide".into()];
let prompt = profile.build_system_prompt(&env, &EnvContext::default(), &docs, None);
assert!(prompt.contains("# Project README"));
@ -329,7 +232,7 @@ mod tests {
#[test]
fn anthropic_system_prompt_includes_env_context() {
let profile = AnthropicProfile::new("claude-opus-4-6");
let env = TestEnv;
let env = linux_env();
let ctx = EnvContext {
git_branch: Some("feature-branch".into()),
is_git_repo: true,
@ -350,7 +253,7 @@ mod tests {
#[test]
fn anthropic_system_prompt_includes_user_instructions() {
let profile = AnthropicProfile::new("claude-opus-4-6");
let env = TestEnv;
let env = linux_env();
let ctx = EnvContext::default();
let prompt = profile.build_system_prompt(&env, &ctx, &[], Some("Always write tests first"));
assert!(prompt.contains("Always write tests first"));

149
crates/coding-agent-loop/src/profiles/gemini.rs
View file

@ -1,19 +1,14 @@
use crate::execution_env::ExecutionEnvironment;
use crate::provider_profile::ProviderProfile;
use crate::subagent::{
make_close_agent_tool, make_send_input_tool, make_spawn_agent_tool, make_wait_tool,
SessionFactory, SubAgentManager,
};
use crate::profiles::assemble_system_prompt;
use crate::provider_profile::{ProfileCapabilities, ProviderProfile};
use crate::tool_registry::ToolRegistry;
use crate::tools::{
make_edit_file_tool, make_glob_tool, make_grep_tool, make_list_dir_tool,
make_read_file_tool, make_read_many_files_tool, make_shell_tool, make_web_fetch_tool,
make_web_search_tool, make_write_file_tool,
};
use std::sync::Arc;
use unified_llm::types::ToolDefinition;
use super::{build_env_context_block_with, EnvContext};
use super::EnvContext;
pub struct GeminiProfile {
model: String,
@ -41,23 +36,6 @@ impl GeminiProfile {
registry,
}
}
pub fn register_subagent_tools(
&mut self,
manager: Arc<tokio::sync::Mutex<SubAgentManager>>,
session_factory: SessionFactory,
current_depth: usize,
) {
self.registry.register(make_spawn_agent_tool(
manager.clone(),
session_factory,
current_depth,
));
self.registry
.register(make_send_input_tool(manager.clone()));
self.registry.register(make_wait_tool(manager.clone()));
self.registry.register(make_close_agent_tool(manager));
}
}
impl ProviderProfile for GeminiProfile {
@ -84,19 +62,7 @@ impl ProviderProfile for GeminiProfile {
project_docs: &[String],
user_instructions: Option<&str>,
) -> String {
let env_block = build_env_context_block_with(env, env_context);
let docs_section = if project_docs.is_empty() {
String::new()
} else {
format!("\n\n{}", project_docs.join("\n\n"))
};
let user_section = match user_instructions {
Some(instructions) => format!("\n\n# User Instructions\n{instructions}"),
None => String::new(),
};
format!(
"\
let core_prompt = "\
You are Gemini CLI, an interactive CLI agent specializing in software engineering tasks \
including solving bugs, adding new functionality, refactoring code, and explaining code. \
Your primary goal is to help users safely and effectively.
@ -210,14 +176,18 @@ These are foundational mandates that take precedence over defaults in this promp
# Coding Best Practices
Write clean, maintainable code. Handle errors appropriately. Follow existing code conventions \
in the project.\
{docs_section}\
{user_section}"
)
in the project.";
assemble_system_prompt(core_prompt, env, env_context, project_docs, user_instructions)
}
fn tools(&self) -> Vec<ToolDefinition> {
self.registry.definitions()
fn capabilities(&self) -> ProfileCapabilities {
ProfileCapabilities {
supports_reasoning: true,
supports_streaming: true,
supports_parallel_tool_calls: true,
context_window_size: 1_000_000,
}
}
fn provider_options(&self) -> Option<serde_json::Value> {
@ -231,22 +201,6 @@ in the project.\
}))
}
fn supports_reasoning(&self) -> bool {
true
}
fn supports_streaming(&self) -> bool {
true
}
fn supports_parallel_tool_calls(&self) -> bool {
true
}
fn context_window_size(&self) -> usize {
1_000_000
}
fn knowledge_cutoff(&self) -> &str {
"January 2025"
}
@ -255,66 +209,15 @@ in the project.\
#[cfg(test)]
mod tests {
use super::*;
use crate::execution_env::*;
use async_trait::async_trait;
use crate::test_support::MockExecutionEnvironment;
use std::sync::Arc;
struct TestEnv;
#[async_trait]
impl ExecutionEnvironment for TestEnv {
async fn read_file(&self, _: &str, _: Option<usize>, _: Option<usize>) -> Result<String, String> {
Ok(String::new())
}
async fn write_file(&self, _: &str, _: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, _: &str) -> Result<bool, String> {
Ok(false)
}
async fn list_directory(&self, _: &str, _: Option<usize>) -> Result<Vec<DirEntry>, String> {
Ok(vec![])
}
async fn exec_command(
&self,
_: &str,
_: u64,
_: Option<&str>,
_: Option<&std::collections::HashMap<String, String>>,
) -> Result<ExecResult, String> {
Ok(ExecResult {
stdout: String::new(),
stderr: String::new(),
exit_code: 0,
timed_out: false,
duration_ms: 0,
})
}
async fn grep(
&self,
_: &str,
_: &str,
_: &GrepOptions,
) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn glob(&self, _: &str, _: Option<&str>) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn initialize(&self) -> Result<(), String> {
Ok(())
}
async fn cleanup(&self) -> Result<(), String> {
Ok(())
}
fn working_directory(&self) -> &str {
"/home/test"
}
fn platform(&self) -> &str {
"linux"
}
fn os_version(&self) -> String {
"Linux 6.1.0".into()
fn linux_env() -> MockExecutionEnvironment {
MockExecutionEnvironment {
working_dir: "/home/test",
platform_str: "linux",
os_version_str: "Linux 6.1.0".into(),
..Default::default()
}
}
@ -337,7 +240,7 @@ mod tests {
#[test]
fn gemini_system_prompt_contains_identity() {
let profile = GeminiProfile::new("gemini-2.0-flash");
let env = TestEnv;
let env = linux_env();
let prompt = profile.build_system_prompt(&env, &EnvContext::default(), &[], None);
assert!(prompt.contains("You are Gemini CLI"));
assert!(prompt.contains("solving bugs"));
@ -349,7 +252,7 @@ mod tests {
#[test]
fn gemini_system_prompt_contains_tool_guidance() {
let profile = GeminiProfile::new("gemini-2.0-flash");
let env = TestEnv;
let env = linux_env();
let prompt = profile.build_system_prompt(&env, &EnvContext::default(), &[], None);
assert!(prompt.contains("read_file"));
assert!(prompt.contains("read_many_files"));
@ -367,7 +270,7 @@ mod tests {
#[test]
fn gemini_system_prompt_contains_project_docs_convention() {
let profile = GeminiProfile::new("gemini-2.0-flash");
let env = TestEnv;
let env = linux_env();
let prompt = profile.build_system_prompt(&env, &EnvContext::default(), &[], None);
assert!(prompt.contains("GEMINI.md"));
assert!(prompt.contains("AGENTS.md"));
@ -376,7 +279,7 @@ mod tests {
#[test]
fn gemini_system_prompt_contains_coding_best_practices() {
let profile = GeminiProfile::new("gemini-2.0-flash");
let env = TestEnv;
let env = linux_env();
let prompt = profile.build_system_prompt(&env, &EnvContext::default(), &[], None);
assert!(prompt.contains("clean, maintainable code"));
assert!(prompt.contains("Handle errors appropriately"));
@ -386,7 +289,7 @@ mod tests {
#[test]
fn gemini_system_prompt_contains_env_context() {
let profile = GeminiProfile::new("gemini-2.0-flash");
let env = TestEnv;
let env = linux_env();
let prompt = profile.build_system_prompt(&env, &EnvContext::default(), &[], None);
assert!(prompt.contains("<environment>"));
assert!(prompt.contains("linux"));

103
crates/coding-agent-loop/src/profiles/mod.rs
View file

@ -20,6 +20,33 @@ pub struct EnvContext {
pub git_recent_commits: Option<String>,
}
/// Assembles a complete system prompt from a core prompt template and standard sections.
///
/// The `core_prompt` should contain `{env_block}` as a placeholder where the environment
/// context block will be inserted. Project docs and user instructions are appended at the end.
#[must_use]
pub fn assemble_system_prompt(
core_prompt: &str,
env: &dyn ExecutionEnvironment,
env_context: &EnvContext,
project_docs: &[String],
user_instructions: Option<&str>,
) -> String {
let env_block = build_env_context_block_with(env, env_context);
let docs_section = if project_docs.is_empty() {
String::new()
} else {
format!("\n\n{}", project_docs.join("\n\n"))
};
let user_section = match user_instructions {
Some(instructions) => format!("\n\n# User Instructions\n{instructions}"),
None => String::new(),
};
let prompt = core_prompt.replace("{env_block}", &env_block);
format!("{prompt}{docs_section}{user_section}")
}
#[must_use]
pub fn build_env_context_block(env: &dyn ExecutionEnvironment) -> String {
build_env_context_block_with(env, &EnvContext::default())
@ -50,6 +77,13 @@ pub fn build_env_context_block_with(env: &dyn ExecutionEnvironment, ctx: &EnvCon
lines.push(format!("Knowledge cutoff: {}", ctx.knowledge_cutoff));
}
if let Some(ref status) = ctx.git_status_short {
lines.push(format!("Git status:\n{status}"));
}
if let Some(ref commits) = ctx.git_recent_commits {
lines.push(format!("Recent commits:\n{commits}"));
}
lines.push("</environment>".to_string());
lines.join("\n")
}
@ -57,71 +91,20 @@ pub fn build_env_context_block_with(env: &dyn ExecutionEnvironment, ctx: &EnvCon
#[cfg(test)]
mod tests {
use super::*;
use crate::execution_env::*;
use async_trait::async_trait;
use crate::test_support::MockExecutionEnvironment;
struct TestEnv;
#[async_trait]
impl ExecutionEnvironment for TestEnv {
async fn read_file(&self, _: &str, _: Option<usize>, _: Option<usize>) -> Result<String, String> {
Ok(String::new())
}
async fn write_file(&self, _: &str, _: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, _: &str) -> Result<bool, String> {
Ok(false)
}
async fn list_directory(&self, _: &str, _: Option<usize>) -> Result<Vec<DirEntry>, String> {
Ok(vec![])
}
async fn exec_command(
&self,
_: &str,
_: u64,
_: Option<&str>,
_: Option<&std::collections::HashMap<String, String>>,
) -> Result<ExecResult, String> {
Ok(ExecResult {
stdout: String::new(),
stderr: String::new(),
exit_code: 0,
timed_out: false,
duration_ms: 0,
})
}
async fn grep(
&self,
_: &str,
_: &str,
_: &GrepOptions,
) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn glob(&self, _: &str, _: Option<&str>) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn initialize(&self) -> Result<(), String> {
Ok(())
}
async fn cleanup(&self) -> Result<(), String> {
Ok(())
}
fn working_directory(&self) -> &str {
"/home/test"
}
fn platform(&self) -> &str {
"linux"
}
fn os_version(&self) -> String {
"Linux 6.1.0".into()
fn linux_env() -> MockExecutionEnvironment {
MockExecutionEnvironment {
working_dir: "/home/test",
platform_str: "linux",
os_version_str: "Linux 6.1.0".into(),
..Default::default()
}
}
#[test]
fn env_context_block_contains_platform() {
let env = TestEnv;
let env = linux_env();
let block = build_env_context_block(&env);
assert!(block.contains("<environment>"));
assert!(block.contains("</environment>"));
@ -132,7 +115,7 @@ mod tests {
#[test]
fn env_context_block_with_extra_context() {
let env = TestEnv;
let env = linux_env();
let ctx = EnvContext {
git_branch: Some("main".into()),
is_git_repo: true,

155
crates/coding-agent-loop/src/profiles/openai.rs
View file

@ -1,17 +1,14 @@
use crate::execution_env::ExecutionEnvironment;
use crate::provider_profile::ProviderProfile;
use crate::subagent::{
make_close_agent_tool, make_send_input_tool, make_spawn_agent_tool, make_wait_tool,
SessionFactory, SubAgentManager,
};
use crate::profiles::assemble_system_prompt;
use crate::provider_profile::{ProfileCapabilities, ProviderProfile};
use crate::tool_registry::{RegisteredTool, ToolRegistry};
use unified_llm::types::ToolDefinition;
use crate::tools::{
make_glob_tool, make_grep_tool, make_read_file_tool, make_shell_tool, make_write_file_tool,
};
use std::sync::Arc;
use unified_llm::types::ToolDefinition;
use super::{build_env_context_block_with, EnvContext};
use super::EnvContext;
pub struct OpenAiProfile {
model: String,
@ -41,23 +38,6 @@ impl OpenAiProfile {
pub fn set_reasoning_effort(&mut self, effort: Option<String>) {
self.reasoning_effort = effort;
}
pub fn register_subagent_tools(
&mut self,
manager: Arc<tokio::sync::Mutex<SubAgentManager>>,
session_factory: SessionFactory,
current_depth: usize,
) {
self.registry.register(make_spawn_agent_tool(
manager.clone(),
session_factory,
current_depth,
));
self.registry
.register(make_send_input_tool(manager.clone()));
self.registry.register(make_wait_tool(manager.clone()));
self.registry.register(make_close_agent_tool(manager));
}
}
impl ProviderProfile for OpenAiProfile {
@ -84,19 +64,7 @@ impl ProviderProfile for OpenAiProfile {
project_docs: &[String],
user_instructions: Option<&str>,
) -> String {
let env_block = build_env_context_block_with(env, env_context);
let docs_section = if project_docs.is_empty() {
String::new()
} else {
format!("\n\n{}", project_docs.join("\n\n"))
};
let user_section = match user_instructions {
Some(instructions) => format!("\n\n# User Instructions\n{instructions}"),
None => String::new(),
};
format!(
"\
let core_prompt = "\
You are a coding agent powered by OpenAI, running in a terminal-based agentic coding assistant. \
You are expected to be precise, safe, and helpful.
@ -174,14 +142,18 @@ Find files by name pattern.
# Coding Best Practices
Write clean, maintainable code. Handle errors appropriately. Follow existing code conventions \
in the project.\
{docs_section}\
{user_section}"
)
in the project.";
assemble_system_prompt(core_prompt, env, env_context, project_docs, user_instructions)
}
fn tools(&self) -> Vec<ToolDefinition> {
self.registry.definitions()
fn capabilities(&self) -> ProfileCapabilities {
ProfileCapabilities {
supports_reasoning: true,
supports_streaming: true,
supports_parallel_tool_calls: true,
context_window_size: 128_000,
}
}
fn provider_options(&self) -> Option<serde_json::Value> {
@ -196,22 +168,6 @@ in the project.\
})
}
fn supports_reasoning(&self) -> bool {
true
}
fn supports_streaming(&self) -> bool {
true
}
fn supports_parallel_tool_calls(&self) -> bool {
true
}
fn context_window_size(&self) -> usize {
128_000
}
fn knowledge_cutoff(&self) -> &str {
"April 2025"
}
@ -357,7 +313,7 @@ pub async fn apply_patch_operations(
results.push(format!("Added file: {path}"));
}
PatchOperation::Delete { path } => {
env.write_file(path, "").await?;
env.delete_file(path).await?;
results.push(format!("Deleted file: {path}"));
}
PatchOperation::Update { path, hunks } => {
@ -461,69 +417,22 @@ fn make_apply_patch_tool() -> RegisteredTool {
mod tests {
use super::*;
use crate::execution_env::*;
use crate::test_support::MockExecutionEnvironment;
use async_trait::async_trait;
use std::collections::HashMap;
use std::sync::Mutex;
struct TestEnv;
#[async_trait]
impl ExecutionEnvironment for TestEnv {
async fn read_file(&self, _: &str, _: Option<usize>, _: Option<usize>) -> Result<String, String> {
Ok(String::new())
}
async fn write_file(&self, _: &str, _: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, _: &str) -> Result<bool, String> {
Ok(false)
}
async fn list_directory(&self, _: &str, _: Option<usize>) -> Result<Vec<DirEntry>, String> {
Ok(vec![])
}
async fn exec_command(
&self,
_: &str,
_: u64,
_: Option<&str>,
_: Option<&std::collections::HashMap<String, String>>,
) -> Result<ExecResult, String> {
Ok(ExecResult {
stdout: String::new(),
stderr: String::new(),
exit_code: 0,
timed_out: false,
duration_ms: 0,
})
}
async fn grep(
&self,
_: &str,
_: &str,
_: &GrepOptions,
) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn glob(&self, _: &str, _: Option<&str>) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn initialize(&self) -> Result<(), String> {
Ok(())
}
async fn cleanup(&self) -> Result<(), String> {
Ok(())
}
fn working_directory(&self) -> &str {
"/home/test"
}
fn platform(&self) -> &str {
"linux"
}
fn os_version(&self) -> String {
"Linux 6.1.0".into()
fn linux_env() -> MockExecutionEnvironment {
MockExecutionEnvironment {
working_dir: "/home/test",
platform_str: "linux",
os_version_str: "Linux 6.1.0".into(),
..Default::default()
}
}
/// A specialized mock with Mutex-protected files for apply_patch tests that
/// need mutable write/delete operations.
struct MockFileEnv {
files: Mutex<HashMap<String, String>>,
}
@ -553,6 +462,10 @@ mod tests {
.insert(path.to_string(), content.to_string());
Ok(())
}
async fn delete_file(&self, path: &str) -> Result<(), String> {
self.files.lock().unwrap().remove(path);
Ok(())
}
async fn file_exists(&self, path: &str) -> Result<bool, String> {
Ok(self.files.lock().unwrap().contains_key(path))
}
@ -621,7 +534,7 @@ mod tests {
#[test]
fn openai_system_prompt_contains_env_context() {
let profile = OpenAiProfile::new("o3-mini");
let env = TestEnv;
let env = linux_env();
let prompt = profile.build_system_prompt(&env, &EnvContext::default(), &[], None);
assert!(prompt.contains("You are a coding agent powered by OpenAI"));
assert!(prompt.contains("<environment>"));
@ -633,7 +546,7 @@ mod tests {
#[test]
fn openai_system_prompt_contains_tool_guidance() {
let profile = OpenAiProfile::new("o3-mini");
let env = TestEnv;
let env = linux_env();
let prompt = profile.build_system_prompt(&env, &EnvContext::default(), &[], None);
assert!(prompt.contains("read_file"));
assert!(prompt.contains("apply_patch"));
@ -647,7 +560,7 @@ mod tests {
#[test]
fn openai_system_prompt_contains_coding_best_practices() {
let profile = OpenAiProfile::new("o3-mini");
let env = TestEnv;
let env = linux_env();
let prompt = profile.build_system_prompt(&env, &EnvContext::default(), &[], None);
assert!(prompt.contains("clean, maintainable code"));
assert!(prompt.contains("existing code conventions"));
@ -656,7 +569,7 @@ mod tests {
#[test]
fn openai_system_prompt_includes_project_docs() {
let profile = OpenAiProfile::new("o3-mini");
let env = TestEnv;
let env = linux_env();
let docs = vec!["# Project README".into(), "# CONTRIBUTING guide".into()];
let prompt = profile.build_system_prompt(&env, &EnvContext::default(), &docs, None);
assert!(prompt.contains("# Project README"));
@ -666,7 +579,7 @@ mod tests {
#[test]
fn openai_system_prompt_includes_user_instructions() {
let profile = OpenAiProfile::new("o3-mini");
let env = TestEnv;
let env = linux_env();
let prompt = profile.build_system_prompt(&env, &EnvContext::default(), &[], Some("Always write tests first"));
assert!(prompt.contains("Always write tests first"));
assert!(prompt.contains("# User Instructions"));

83
crates/coding-agent-loop/src/project_docs.rs
View file

@ -86,69 +86,19 @@ fn truncate_to_budget(content: &str, budget: usize) -> String {
#[cfg(test)]
mod tests {
use super::*;
use crate::execution_env::*;
use async_trait::async_trait;
use crate::execution_env::ExecutionEnvironment;
use crate::test_support::MockExecutionEnvironment;
use std::collections::HashMap;
use std::sync::Arc;
struct DocEnv {
files: HashMap<String, String>,
}
#[async_trait]
impl ExecutionEnvironment for DocEnv {
async fn read_file(&self, path: &str, _: Option<usize>, _: Option<usize>) -> Result<String, String> {
self.files
.get(path)
.cloned()
.ok_or_else(|| format!("not found: {path}"))
}
async fn write_file(&self, _: &str, _: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, path: &str) -> Result<bool, String> {
Ok(self.files.contains_key(path))
}
async fn list_directory(&self, _: &str, _: Option<usize>) -> Result<Vec<DirEntry>, String> {
Ok(vec![])
}
async fn exec_command(&self, _: &str, _: u64, _: Option<&str>, _: Option<&std::collections::HashMap<String, String>>) -> Result<ExecResult, String> {
Ok(ExecResult {
stdout: String::new(),
stderr: String::new(),
exit_code: 0,
timed_out: false,
duration_ms: 0,
})
}
async fn grep(&self, _: &str, _: &str, _: &GrepOptions) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn glob(&self, _: &str, _: Option<&str>) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn initialize(&self) -> Result<(), String> {
Ok(())
}
async fn cleanup(&self) -> Result<(), String> {
Ok(())
}
fn working_directory(&self) -> &str {
"/tmp"
}
fn platform(&self) -> &str {
"darwin"
}
fn os_version(&self) -> String {
String::new()
}
}
#[tokio::test]
async fn discovers_agents_md() {
let mut files = HashMap::new();
files.insert("/repo/AGENTS.md".into(), "Agent instructions".into());
let env: Arc<dyn ExecutionEnvironment> = Arc::new(DocEnv { files });
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockExecutionEnvironment {
files,
..Default::default()
});
let docs = discover_project_docs(env.as_ref(), "/repo", "/repo", "anthropic").await;
assert_eq!(docs.len(), 1);
assert_eq!(docs[0], "Agent instructions");
@ -165,8 +115,9 @@ mod tests {
);
files.insert("/repo/GEMINI.md".into(), "gemini".into());
let env: Arc<dyn ExecutionEnvironment> = Arc::new(DocEnv {
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockExecutionEnvironment {
files: files.clone(),
..Default::default()
});
let anthropic_docs =
discover_project_docs(env.as_ref(), "/repo", "/repo", "anthropic").await;
@ -174,15 +125,19 @@ mod tests {
assert_eq!(anthropic_docs[0], "agents");
assert_eq!(anthropic_docs[1], "claude");
let env: Arc<dyn ExecutionEnvironment> = Arc::new(DocEnv {
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockExecutionEnvironment {
files: files.clone(),
..Default::default()
});
let openai_docs = discover_project_docs(env.as_ref(), "/repo", "/repo", "openai").await;
assert_eq!(openai_docs.len(), 2);
assert_eq!(openai_docs[0], "agents");
assert_eq!(openai_docs[1], "copilot");
let env: Arc<dyn ExecutionEnvironment> = Arc::new(DocEnv { files });
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockExecutionEnvironment {
files,
..Default::default()
});
let gemini_docs = discover_project_docs(env.as_ref(), "/repo", "/repo", "gemini").await;
assert_eq!(gemini_docs.len(), 2);
assert_eq!(gemini_docs[0], "agents");
@ -198,7 +153,10 @@ mod tests {
files.insert("/repo/AGENTS.md".into(), large_content.clone());
files.insert("/repo/CLAUDE.md".into(), second_content);
let env: Arc<dyn ExecutionEnvironment> = Arc::new(DocEnv { files });
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockExecutionEnvironment {
files,
..Default::default()
});
let docs = discover_project_docs(env.as_ref(), "/repo", "/repo", "anthropic").await;
assert_eq!(docs.len(), 2);
assert_eq!(docs[0], large_content);
@ -214,7 +172,10 @@ mod tests {
files.insert("/repo/src/AGENTS.md".into(), "src agents".into());
files.insert("/repo/src/app/AGENTS.md".into(), "app agents".into());
let env: Arc<dyn ExecutionEnvironment> = Arc::new(DocEnv { files });
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockExecutionEnvironment {
files,
..Default::default()
});
let docs =
discover_project_docs(env.as_ref(), "/repo", "/repo/src/app", "anthropic").await;
assert_eq!(docs.len(), 3);

179
crates/coding-agent-loop/src/provider_profile.rs
View file

@ -1,8 +1,21 @@
use crate::execution_env::ExecutionEnvironment;
use crate::profiles::EnvContext;
use crate::subagent::{
make_close_agent_tool, make_send_input_tool, make_spawn_agent_tool, SessionFactory,
SubAgentManager,
};
use crate::tool_registry::ToolRegistry;
use std::sync::Arc;
use unified_llm::types::ToolDefinition;
/// Static capabilities of a provider profile.
pub struct ProfileCapabilities {
pub supports_reasoning: bool,
pub supports_streaming: bool,
pub supports_parallel_tool_calls: bool,
pub context_window_size: usize,
}
pub trait ProviderProfile: Send + Sync {
fn id(&self) -> String;
fn model(&self) -> String;
@ -15,85 +28,66 @@ pub trait ProviderProfile: Send + Sync {
project_docs: &[String],
user_instructions: Option<&str>,
) -> String;
fn tools(&self) -> Vec<ToolDefinition>;
fn provider_options(&self) -> Option<serde_json::Value>;
fn supports_reasoning(&self) -> bool;
fn supports_streaming(&self) -> bool;
fn supports_parallel_tool_calls(&self) -> bool;
fn context_window_size(&self) -> usize;
fn capabilities(&self) -> ProfileCapabilities;
fn knowledge_cutoff(&self) -> &str;
fn tools(&self) -> Vec<ToolDefinition> {
self.tool_registry().definitions()
}
fn provider_options(&self) -> Option<serde_json::Value> {
None
}
fn supports_reasoning(&self) -> bool {
self.capabilities().supports_reasoning
}
fn supports_streaming(&self) -> bool {
self.capabilities().supports_streaming
}
fn supports_parallel_tool_calls(&self) -> bool {
self.capabilities().supports_parallel_tool_calls
}
fn context_window_size(&self) -> usize {
self.capabilities().context_window_size
}
fn register_subagent_tools(
&mut self,
manager: Arc<tokio::sync::Mutex<SubAgentManager>>,
session_factory: SessionFactory,
current_depth: usize,
) {
self.tool_registry_mut().register(make_spawn_agent_tool(
manager.clone(),
session_factory,
current_depth,
));
self.tool_registry_mut()
.register(make_send_input_tool(manager.clone()));
self.tool_registry_mut()
.register(crate::subagent::make_wait_tool(manager.clone()));
self.tool_registry_mut()
.register(make_close_agent_tool(manager));
}
}
#[cfg(test)]
mod tests {
use super::*;
use crate::execution_env::*;
use async_trait::async_trait;
use crate::execution_env::ExecutionEnvironment;
use crate::test_support::MockExecutionEnvironment;
struct TestEnv;
#[async_trait]
impl ExecutionEnvironment for TestEnv {
async fn read_file(&self, _: &str, _: Option<usize>, _: Option<usize>) -> Result<String, String> {
Ok(String::new())
}
async fn write_file(&self, _: &str, _: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, _: &str) -> Result<bool, String> {
Ok(false)
}
async fn list_directory(&self, _: &str, _: Option<usize>) -> Result<Vec<DirEntry>, String> {
Ok(vec![])
}
async fn exec_command(
&self,
_: &str,
_: u64,
_: Option<&str>,
_: Option<&std::collections::HashMap<String, String>>,
) -> Result<ExecResult, String> {
Ok(ExecResult {
stdout: String::new(),
stderr: String::new(),
exit_code: 0,
timed_out: false,
duration_ms: 0,
})
}
async fn grep(
&self,
_: &str,
_: &str,
_: &GrepOptions,
) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn glob(&self, _: &str, _: Option<&str>) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn initialize(&self) -> Result<(), String> {
Ok(())
}
async fn cleanup(&self) -> Result<(), String> {
Ok(())
}
fn working_directory(&self) -> &str {
"/home/test"
}
fn platform(&self) -> &str {
"linux"
}
fn os_version(&self) -> String {
"Linux 6.1.0".into()
}
}
struct TestProfile {
/// A specialized profile for provider_profile tests that uses distinct id/model
/// and a custom build_system_prompt (unlike the shared TestProfile).
struct ProviderTestProfile {
registry: ToolRegistry,
}
impl TestProfile {
impl ProviderTestProfile {
fn new() -> Self {
Self {
registry: ToolRegistry::new(),
@ -101,7 +95,7 @@ mod tests {
}
}
impl ProviderProfile for TestProfile {
impl ProviderProfile for ProviderTestProfile {
fn id(&self) -> String {
"test-provider".into()
}
@ -131,23 +125,13 @@ mod tests {
None => base,
}
}
fn tools(&self) -> Vec<ToolDefinition> {
self.registry.definitions()
}
fn provider_options(&self) -> Option<serde_json::Value> {
None
}
fn supports_reasoning(&self) -> bool {
true
}
fn supports_streaming(&self) -> bool {
true
}
fn supports_parallel_tool_calls(&self) -> bool {
false
}
fn context_window_size(&self) -> usize {
200_000
fn capabilities(&self) -> ProfileCapabilities {
ProfileCapabilities {
supports_reasoning: true,
supports_streaming: true,
supports_parallel_tool_calls: false,
context_window_size: 200_000,
}
}
fn knowledge_cutoff(&self) -> &str {
"May 2025"
@ -156,14 +140,14 @@ mod tests {
#[test]
fn profile_id_and_model() {
let profile = TestProfile::new();
let profile = ProviderTestProfile::new();
assert_eq!(profile.id(), "test-provider");
assert_eq!(profile.model(), "test-model");
}
#[test]
fn profile_capabilities() {
let profile = TestProfile::new();
let profile = ProviderTestProfile::new();
assert!(profile.supports_reasoning());
assert!(profile.supports_streaming());
assert!(!profile.supports_parallel_tool_calls());
@ -172,8 +156,13 @@ mod tests {
#[test]
fn profile_build_system_prompt() {
let profile = TestProfile::new();
let env = TestEnv;
let profile = ProviderTestProfile::new();
let env = MockExecutionEnvironment {
working_dir: "/home/test",
platform_str: "linux",
os_version_str: "Linux 6.1.0".into(),
..Default::default()
};
let ctx = EnvContext::default();
let docs = vec!["README.md contents".into()];
let prompt = profile.build_system_prompt(&env, &ctx, &docs, None);
@ -183,8 +172,8 @@ mod tests {
#[test]
fn profile_build_system_prompt_with_user_instructions() {
let profile = TestProfile::new();
let env = TestEnv;
let profile = ProviderTestProfile::new();
let env = MockExecutionEnvironment::default();
let ctx = EnvContext::default();
let prompt = profile.build_system_prompt(&env, &ctx, &[], Some("Always use TDD"));
assert!(prompt.contains("Always use TDD"));
@ -192,13 +181,13 @@ mod tests {
#[test]
fn profile_provider_options_none() {
let profile = TestProfile::new();
let profile = ProviderTestProfile::new();
assert!(profile.provider_options().is_none());
}
#[test]
fn profile_tools_empty_registry() {
let profile = TestProfile::new();
let profile = ProviderTestProfile::new();
assert!(profile.tools().is_empty());
}
}

928
crates/coding-agent-loop/src/session.rs
View file

File diff suppressed because it is too large Load diff

210
crates/coding-agent-loop/src/subagent.rs
View file

@ -325,215 +325,7 @@ pub fn make_close_agent_tool(
#[cfg(test)]
mod tests {
use super::*;
use crate::config::SessionConfig;
use crate::execution_env::*;
use crate::provider_profile::ProviderProfile;
use crate::tool_registry::ToolRegistry;
use async_trait::async_trait;
use std::sync::atomic::AtomicUsize;
use unified_llm::client::Client;
use unified_llm::error::SdkError;
use unified_llm::provider::{ProviderAdapter, StreamEventStream};
use unified_llm::types::{FinishReason, Message, Response, Usage};
// --- Mock LLM Provider ---
struct MockLlmProvider {
responses: Vec<Response>,
call_index: AtomicUsize,
}
impl MockLlmProvider {
fn new(responses: Vec<Response>) -> Self {
Self {
responses,
call_index: AtomicUsize::new(0),
}
}
}
#[async_trait]
impl ProviderAdapter for MockLlmProvider {
fn name(&self) -> &str {
"mock"
}
async fn complete(
&self,
_request: &unified_llm::types::Request,
) -> Result<Response, SdkError> {
let idx = self.call_index.fetch_add(1, Ordering::SeqCst);
if idx < self.responses.len() {
Ok(self.responses[idx].clone())
} else {
Ok(self.responses[self.responses.len() - 1].clone())
}
}
async fn stream(
&self,
_request: &unified_llm::types::Request,
) -> Result<StreamEventStream, SdkError> {
Err(SdkError::Configuration {
message: "streaming not supported in mock".into(),
})
}
}
// --- Memory Execution Environment ---
struct MemoryExecutionEnvironment;
#[async_trait]
impl ExecutionEnvironment for MemoryExecutionEnvironment {
async fn read_file(&self, _path: &str, _offset: Option<usize>, _limit: Option<usize>) -> Result<String, String> {
Ok(String::new())
}
async fn write_file(&self, _path: &str, _content: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, _path: &str) -> Result<bool, String> {
Ok(false)
}
async fn list_directory(&self, _path: &str, _depth: Option<usize>) -> Result<Vec<DirEntry>, String> {
Ok(vec![])
}
async fn exec_command(
&self,
_command: &str,
_timeout_ms: u64,
_working_dir: Option<&str>,
_env_vars: Option<&std::collections::HashMap<String, String>>,
) -> Result<ExecResult, String> {
Ok(ExecResult {
stdout: "mock output".into(),
stderr: String::new(),
exit_code: 0,
timed_out: false,
duration_ms: 10,
})
}
async fn grep(
&self,
_pattern: &str,
_path: &str,
_options: &GrepOptions,
) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn glob(&self, _pattern: &str, _path: Option<&str>) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn initialize(&self) -> Result<(), String> {
Ok(())
}
async fn cleanup(&self) -> Result<(), String> {
Ok(())
}
fn working_directory(&self) -> &str {
"/tmp/test"
}
fn platform(&self) -> &str {
"darwin"
}
fn os_version(&self) -> String {
"Darwin 24.0.0".into()
}
}
// --- Test Profile ---
struct TestProfile {
registry: ToolRegistry,
}
impl TestProfile {
fn new() -> Self {
Self {
registry: ToolRegistry::new(),
}
}
}
impl ProviderProfile for TestProfile {
fn id(&self) -> String {
"mock".into()
}
fn model(&self) -> String {
"mock-model".into()
}
fn tool_registry(&self) -> &ToolRegistry {
&self.registry
}
fn tool_registry_mut(&mut self) -> &mut ToolRegistry {
&mut self.registry
}
fn build_system_prompt(
&self,
_env: &dyn ExecutionEnvironment,
_env_context: &crate::profiles::EnvContext,
_project_docs: &[String],
_user_instructions: Option<&str>,
) -> String {
"You are a test assistant.".into()
}
fn tools(&self) -> Vec<ToolDefinition> {
self.registry.definitions()
}
fn provider_options(&self) -> Option<serde_json::Value> {
None
}
fn supports_reasoning(&self) -> bool {
false
}
fn supports_streaming(&self) -> bool {
false
}
fn supports_parallel_tool_calls(&self) -> bool {
false
}
fn context_window_size(&self) -> usize {
200_000
}
fn knowledge_cutoff(&self) -> &str {
"May 2025"
}
}
// --- Helper functions ---
fn text_response(text: &str) -> Response {
Response {
id: format!("resp_{text}"),
model: "mock-model".into(),
provider: "mock".into(),
message: Message::assistant(text),
finish_reason: FinishReason::Stop,
usage: Usage {
input_tokens: 10,
output_tokens: 5,
total_tokens: 15,
..Default::default()
},
raw: None,
warnings: vec![],
rate_limit: None,
}
}
async fn make_client(provider: Arc<dyn ProviderAdapter>) -> Client {
let mut providers = HashMap::new();
providers.insert(provider.name().to_string(), provider);
Client::new(providers, Some("mock".into()), vec![])
}
async fn make_session(responses: Vec<Response>) -> Session {
let provider = Arc::new(MockLlmProvider::new(responses));
let client = make_client(provider).await;
let profile = Arc::new(TestProfile::new());
let env = Arc::new(MemoryExecutionEnvironment);
Session::new(client, profile, env, SessionConfig::default())
}
use crate::test_support::*;
// --- Tests ---

478
crates/coding-agent-loop/src/test_support.rs Normal file
View file

@ -0,0 +1,478 @@
use crate::config::SessionConfig;
use crate::execution_env::*;
use crate::profiles::EnvContext;
use crate::provider_profile::{ProfileCapabilities, ProviderProfile};
use crate::session::Session;
use crate::tool_registry::ToolRegistry;
use async_trait::async_trait;
use std::collections::HashMap;
use std::sync::atomic::{AtomicUsize, Ordering};
use std::sync::Arc;
use unified_llm::client::Client;
use unified_llm::error::SdkError;
use unified_llm::provider::{ProviderAdapter, StreamEventStream};
use unified_llm::types::{FinishReason, Message, Request, Response, Usage};
// --- MockExecutionEnvironment ---
pub(crate) struct MockExecutionEnvironment {
pub files: HashMap<String, String>,
pub exec_result: ExecResult,
pub grep_results: Vec<String>,
pub glob_results: Vec<String>,
pub working_dir: &'static str,
pub platform_str: &'static str,
pub os_version_str: String,
}
impl Default for MockExecutionEnvironment {
fn default() -> Self {
Self {
files: HashMap::new(),
exec_result: ExecResult {
stdout: "mock output".into(),
stderr: String::new(),
exit_code: 0,
timed_out: false,
duration_ms: 10,
},
grep_results: vec![],
glob_results: vec![],
working_dir: "/tmp/test",
platform_str: "darwin",
os_version_str: "Darwin 24.0.0".into(),
}
}
}
#[async_trait]
impl ExecutionEnvironment for MockExecutionEnvironment {
async fn read_file(
&self,
path: &str,
_offset: Option<usize>,
_limit: Option<usize>,
) -> Result<String, String> {
self.files
.get(path)
.cloned()
.ok_or_else(|| format!("File not found: {path}"))
}
async fn write_file(&self, _path: &str, _content: &str) -> Result<(), String> {
Ok(())
}
async fn delete_file(&self, _path: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, path: &str) -> Result<bool, String> {
Ok(self.files.contains_key(path))
}
async fn list_directory(
&self,
_path: &str,
_depth: Option<usize>,
) -> Result<Vec<DirEntry>, String> {
Ok(vec![])
}
async fn exec_command(
&self,
_command: &str,
_timeout_ms: u64,
_working_dir: Option<&str>,
_env_vars: Option<&std::collections::HashMap<String, String>>,
) -> Result<ExecResult, String> {
Ok(self.exec_result.clone())
}
async fn grep(
&self,
_pattern: &str,
_path: &str,
_options: &GrepOptions,
) -> Result<Vec<String>, String> {
Ok(self.grep_results.clone())
}
async fn glob(&self, _pattern: &str, _path: Option<&str>) -> Result<Vec<String>, String> {
Ok(self.glob_results.clone())
}
async fn initialize(&self) -> Result<(), String> {
Ok(())
}
async fn cleanup(&self) -> Result<(), String> {
Ok(())
}
fn working_directory(&self) -> &str {
self.working_dir
}
fn platform(&self) -> &str {
self.platform_str
}
fn os_version(&self) -> String {
self.os_version_str.clone()
}
}
// --- TestProfile ---
pub(crate) struct TestProfile {
pub registry: ToolRegistry,
}
impl TestProfile {
pub fn new() -> Self {
Self {
registry: ToolRegistry::new(),
}
}
pub fn with_tools(registry: ToolRegistry) -> Self {
Self { registry }
}
}
impl ProviderProfile for TestProfile {
fn id(&self) -> String {
"mock".into()
}
fn model(&self) -> String {
"mock-model".into()
}
fn tool_registry(&self) -> &ToolRegistry {
&self.registry
}
fn tool_registry_mut(&mut self) -> &mut ToolRegistry {
&mut self.registry
}
fn build_system_prompt(
&self,
_env: &dyn ExecutionEnvironment,
_env_context: &EnvContext,
_project_docs: &[String],
_user_instructions: Option<&str>,
) -> String {
"You are a test assistant.".into()
}
fn capabilities(&self) -> ProfileCapabilities {
ProfileCapabilities {
supports_reasoning: false,
supports_streaming: false,
supports_parallel_tool_calls: false,
context_window_size: 200_000,
}
}
fn knowledge_cutoff(&self) -> &str {
"May 2025"
}
}
// --- MockLlmProvider ---
pub(crate) struct MockLlmProvider {
pub responses: Vec<Response>,
pub call_index: AtomicUsize,
}
impl MockLlmProvider {
pub fn new(responses: Vec<Response>) -> Self {
Self {
responses,
call_index: AtomicUsize::new(0),
}
}
}
#[async_trait]
impl ProviderAdapter for MockLlmProvider {
fn name(&self) -> &str {
"mock"
}
async fn complete(&self, _request: &Request) -> Result<Response, SdkError> {
let idx = self.call_index.fetch_add(1, Ordering::SeqCst);
if idx < self.responses.len() {
Ok(self.responses[idx].clone())
} else {
Ok(self.responses[self.responses.len() - 1].clone())
}
}
async fn stream(&self, _request: &Request) -> Result<StreamEventStream, SdkError> {
Err(SdkError::Configuration {
message: "streaming not supported in mock".into(),
})
}
}
// --- Helper functions ---
pub(crate) fn text_response(text: &str) -> Response {
Response {
id: format!("resp_{text}"),
model: "mock-model".into(),
provider: "mock".into(),
message: Message::assistant(text),
finish_reason: FinishReason::Stop,
usage: Usage {
input_tokens: 10,
output_tokens: 5,
total_tokens: 15,
..Default::default()
},
raw: None,
warnings: vec![],
rate_limit: None,
}
}
pub(crate) async fn make_client(provider: Arc<dyn ProviderAdapter>) -> Client {
let mut providers = HashMap::new();
providers.insert(provider.name().to_string(), provider);
Client::new(providers, Some("mock".into()), vec![])
}
pub(crate) async fn make_session(responses: Vec<Response>) -> Session {
let provider = Arc::new(MockLlmProvider::new(responses));
let client = make_client(provider).await;
let profile = Arc::new(TestProfile::new());
let env = Arc::new(MockExecutionEnvironment::default());
Session::new(client, profile, env, SessionConfig::default())
}
pub(crate) async fn make_session_with_tools(
responses: Vec<Response>,
registry: ToolRegistry,
) -> Session {
let provider = Arc::new(MockLlmProvider::new(responses));
let client = make_client(provider).await;
let profile = Arc::new(TestProfile::with_tools(registry));
let env = Arc::new(MockExecutionEnvironment::default());
Session::new(client, profile, env, SessionConfig::default())
}
pub(crate) async fn make_session_with_config(
responses: Vec<Response>,
config: SessionConfig,
) -> Session {
let provider = Arc::new(MockLlmProvider::new(responses));
let client = make_client(provider).await;
let profile = Arc::new(TestProfile::new());
let env = Arc::new(MockExecutionEnvironment::default());
Session::new(client, profile, env, config)
}
pub(crate) async fn make_session_with_tools_and_config(
responses: Vec<Response>,
registry: ToolRegistry,
config: SessionConfig,
) -> Session {
let provider = Arc::new(MockLlmProvider::new(responses));
let client = make_client(provider).await;
let profile = Arc::new(TestProfile::with_tools(registry));
let env = Arc::new(MockExecutionEnvironment::default());
Session::new(client, profile, env, config)
}
pub(crate) fn tool_call_response(
tool_name: &str,
tool_call_id: &str,
args: serde_json::Value,
) -> Response {
use unified_llm::types::{ContentPart, Role, ToolCall};
Response {
id: format!("resp_{tool_call_id}"),
model: "mock-model".into(),
provider: "mock".into(),
message: Message {
role: Role::Assistant,
content: vec![
ContentPart::text("Let me use a tool."),
ContentPart::ToolCall(ToolCall::new(tool_call_id, tool_name, args)),
],
name: None,
tool_call_id: None,
},
finish_reason: FinishReason::ToolCalls,
usage: Usage {
input_tokens: 10,
output_tokens: 5,
total_tokens: 15,
..Default::default()
},
raw: None,
warnings: vec![],
rate_limit: None,
}
}
pub(crate) fn make_echo_tool() -> crate::tool_registry::RegisteredTool {
use unified_llm::types::ToolDefinition;
crate::tool_registry::RegisteredTool {
definition: ToolDefinition {
name: "echo".into(),
description: "Echoes the input".into(),
parameters: serde_json::json!({"type": "object", "properties": {"text": {"type": "string"}}}),
},
executor: Arc::new(|args, _env| {
Box::pin(async move {
let text = args
.get("text")
.and_then(|v| v.as_str())
.unwrap_or("no text");
Ok(format!("echo: {text}"))
})
}),
}
}
pub(crate) fn make_error_tool() -> crate::tool_registry::RegisteredTool {
use unified_llm::types::ToolDefinition;
crate::tool_registry::RegisteredTool {
definition: ToolDefinition {
name: "fail_tool".into(),
description: "Always fails".into(),
parameters: serde_json::json!({"type": "object"}),
},
executor: Arc::new(|_args, _env| {
Box::pin(async move { Err("tool execution failed".to_string()) })
}),
}
}
// --- ParallelTestProfile ---
pub(crate) struct ParallelTestProfile {
pub registry: ToolRegistry,
pub context_window: usize,
}
impl ParallelTestProfile {
pub fn with_tools(registry: ToolRegistry) -> Self {
Self {
registry,
context_window: 200_000,
}
}
pub fn with_tools_and_context_window(registry: ToolRegistry, context_window: usize) -> Self {
Self {
registry,
context_window,
}
}
}
impl ProviderProfile for ParallelTestProfile {
fn id(&self) -> String {
"mock".into()
}
fn model(&self) -> String {
"mock-model".into()
}
fn tool_registry(&self) -> &ToolRegistry {
&self.registry
}
fn tool_registry_mut(&mut self) -> &mut ToolRegistry {
&mut self.registry
}
fn build_system_prompt(
&self,
_env: &dyn ExecutionEnvironment,
_env_context: &EnvContext,
_project_docs: &[String],
_user_instructions: Option<&str>,
) -> String {
"You are a test assistant.".into()
}
fn capabilities(&self) -> ProfileCapabilities {
ProfileCapabilities {
supports_reasoning: false,
supports_streaming: false,
supports_parallel_tool_calls: true,
context_window_size: self.context_window,
}
}
fn knowledge_cutoff(&self) -> &str {
"May 2025"
}
}
// --- MockErrorProvider ---
pub(crate) struct MockErrorProvider {
pub error: SdkError,
}
#[async_trait]
impl ProviderAdapter for MockErrorProvider {
fn name(&self) -> &str {
"mock"
}
async fn complete(&self, _request: &Request) -> Result<Response, SdkError> {
Err(self.error.clone())
}
async fn stream(&self, _request: &Request) -> Result<StreamEventStream, SdkError> {
Err(SdkError::Configuration {
message: "streaming not supported in mock".into(),
})
}
}
pub(crate) fn multi_tool_call_response(
calls: Vec<(&str, &str, serde_json::Value)>,
) -> Response {
use unified_llm::types::{ContentPart, Role, ToolCall};
let mut content = vec![ContentPart::text("Let me use multiple tools.")];
for (tool_name, tool_call_id, args) in &calls {
content.push(ContentPart::ToolCall(ToolCall::new(
*tool_call_id,
*tool_name,
args.clone(),
)));
}
Response {
id: "resp_multi".into(),
model: "mock-model".into(),
provider: "mock".into(),
message: Message {
role: Role::Assistant,
content,
name: None,
tool_call_id: None,
},
finish_reason: FinishReason::ToolCalls,
usage: Usage {
input_tokens: 10,
output_tokens: 5,
total_tokens: 15,
..Default::default()
},
raw: None,
warnings: vec![],
rate_limit: None,
}
}

65
crates/coding-agent-loop/src/tool_registry.rs
View file

@ -163,69 +163,10 @@ mod tests {
let tool = registry.get("echo").unwrap();
use crate::execution_env::*;
use async_trait::async_trait;
use crate::execution_env::ExecutionEnvironment;
use crate::test_support::MockExecutionEnvironment;
struct DummyEnv;
#[async_trait]
impl ExecutionEnvironment for DummyEnv {
async fn read_file(&self, _: &str, _: Option<usize>, _: Option<usize>) -> Result<String, String> {
Ok(String::new())
}
async fn write_file(&self, _: &str, _: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, _: &str) -> Result<bool, String> {
Ok(false)
}
async fn list_directory(&self, _: &str, _: Option<usize>) -> Result<Vec<DirEntry>, String> {
Ok(vec![])
}
async fn exec_command(
&self,
_: &str,
_: u64,
_: Option<&str>,
_: Option<&std::collections::HashMap<String, String>>,
) -> Result<ExecResult, String> {
Ok(ExecResult {
stdout: String::new(),
stderr: String::new(),
exit_code: 0,
timed_out: false,
duration_ms: 0,
})
}
async fn grep(
&self,
_: &str,
_: &str,
_: &GrepOptions,
) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn glob(&self, _: &str, _: Option<&str>) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn initialize(&self) -> Result<(), String> {
Ok(())
}
async fn cleanup(&self) -> Result<(), String> {
Ok(())
}
fn working_directory(&self) -> &str {
"/tmp"
}
fn platform(&self) -> &str {
"darwin"
}
fn os_version(&self) -> String {
String::new()
}
}
let env: Arc<dyn ExecutionEnvironment> = Arc::new(DummyEnv);
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockExecutionEnvironment::default());
let result = (tool.executor)(serde_json::json!({}), env).await;
assert_eq!(result.unwrap(), "ok");
}

189
crates/coding-agent-loop/src/tools.rs
View file

@ -276,7 +276,7 @@ pub fn make_glob_tool() -> RegisteredTool {
}
#[must_use]
pub fn make_read_many_files_tool() -> RegisteredTool {
pub(crate) fn make_read_many_files_tool() -> RegisteredTool {
RegisteredTool {
definition: ToolDefinition {
name: "read_many_files".into(),
@ -320,7 +320,7 @@ pub fn make_read_many_files_tool() -> RegisteredTool {
}
#[must_use]
pub fn make_list_dir_tool() -> RegisteredTool {
pub(crate) fn make_list_dir_tool() -> RegisteredTool {
RegisteredTool {
definition: ToolDefinition {
name: "list_dir".into(),
@ -363,7 +363,7 @@ pub fn make_list_dir_tool() -> RegisteredTool {
}
#[must_use]
pub fn make_web_search_tool() -> RegisteredTool {
pub(crate) fn make_web_search_tool() -> RegisteredTool {
RegisteredTool {
definition: ToolDefinition {
name: "web_search".into(),
@ -386,7 +386,7 @@ pub fn make_web_search_tool() -> RegisteredTool {
}
#[must_use]
pub fn make_web_fetch_tool() -> RegisteredTool {
pub(crate) fn make_web_fetch_tool() -> RegisteredTool {
RegisteredTool {
definition: ToolDefinition {
name: "web_fetch".into(),
@ -411,9 +411,11 @@ pub fn make_web_fetch_tool() -> RegisteredTool {
mod tests {
use super::*;
use crate::execution_env::*;
use crate::test_support::MockExecutionEnvironment;
use async_trait::async_trait;
use std::sync::Mutex;
/// A specialized mock that applies offset/limit to file content (for read_file tool tests).
struct ReadFileEnv {
content: String,
}
@ -430,6 +432,9 @@ mod tests {
async fn write_file(&self, _: &str, _: &str) -> Result<(), String> {
Ok(())
}
async fn delete_file(&self, _: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, _: &str) -> Result<bool, String> {
Ok(false)
}
@ -481,6 +486,9 @@ mod tests {
*self.written.lock().unwrap() = Some((path.into(), content.into()));
Ok(())
}
async fn delete_file(&self, _: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, _: &str) -> Result<bool, String> {
Ok(false)
}
@ -533,6 +541,9 @@ mod tests {
*self.written.lock().unwrap() = Some(content.into());
Ok(())
}
async fn delete_file(&self, _: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, _: &str) -> Result<bool, String> {
Ok(false)
}
@ -571,49 +582,6 @@ mod tests {
}
}
struct ShellEnv {
result: ExecResult,
}
#[async_trait]
impl ExecutionEnvironment for ShellEnv {
async fn read_file(&self, _: &str, _offset: Option<usize>, _limit: Option<usize>) -> Result<String, String> {
Ok(String::new())
}
async fn write_file(&self, _: &str, _: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, _: &str) -> Result<bool, String> {
Ok(false)
}
async fn list_directory(&self, _: &str, _depth: Option<usize>) -> Result<Vec<DirEntry>, String> {
Ok(vec![])
}
async fn exec_command(&self, _: &str, _: u64, _: Option<&str>, _: Option<&std::collections::HashMap<String, String>>) -> Result<ExecResult, String> {
Ok(self.result.clone())
}
async fn grep(&self, _: &str, _: &str, _: &GrepOptions) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn glob(&self, _: &str, _path: Option<&str>) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn initialize(&self) -> Result<(), String> {
Ok(())
}
async fn cleanup(&self) -> Result<(), String> {
Ok(())
}
fn working_directory(&self) -> &str {
"/tmp"
}
fn platform(&self) -> &str {
"darwin"
}
fn os_version(&self) -> String {
String::new()
}
}
struct ShellCapturingEnv {
captured_timeout: Mutex<Option<u64>>,
@ -627,6 +595,9 @@ mod tests {
async fn write_file(&self, _: &str, _: &str) -> Result<(), String> {
Ok(())
}
async fn delete_file(&self, _: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, _: &str) -> Result<bool, String> {
Ok(false)
}
@ -672,105 +643,6 @@ mod tests {
}
}
struct GrepEnv {
results: Vec<String>,
}
#[async_trait]
impl ExecutionEnvironment for GrepEnv {
async fn read_file(&self, _: &str, _offset: Option<usize>, _limit: Option<usize>) -> Result<String, String> {
Ok(String::new())
}
async fn write_file(&self, _: &str, _: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, _: &str) -> Result<bool, String> {
Ok(false)
}
async fn list_directory(&self, _: &str, _depth: Option<usize>) -> Result<Vec<DirEntry>, String> {
Ok(vec![])
}
async fn exec_command(&self, _: &str, _: u64, _: Option<&str>, _: Option<&std::collections::HashMap<String, String>>) -> Result<ExecResult, String> {
Ok(ExecResult {
stdout: String::new(),
stderr: String::new(),
exit_code: 0,
timed_out: false,
duration_ms: 0,
})
}
async fn grep(&self, _: &str, _: &str, _: &GrepOptions) -> Result<Vec<String>, String> {
Ok(self.results.clone())
}
async fn glob(&self, _: &str, _path: Option<&str>) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn initialize(&self) -> Result<(), String> {
Ok(())
}
async fn cleanup(&self) -> Result<(), String> {
Ok(())
}
fn working_directory(&self) -> &str {
"/tmp"
}
fn platform(&self) -> &str {
"darwin"
}
fn os_version(&self) -> String {
String::new()
}
}
struct GlobEnv {
results: Vec<String>,
}
#[async_trait]
impl ExecutionEnvironment for GlobEnv {
async fn read_file(&self, _: &str, _offset: Option<usize>, _limit: Option<usize>) -> Result<String, String> {
Ok(String::new())
}
async fn write_file(&self, _: &str, _: &str) -> Result<(), String> {
Ok(())
}
async fn file_exists(&self, _: &str) -> Result<bool, String> {
Ok(false)
}
async fn list_directory(&self, _: &str, _depth: Option<usize>) -> Result<Vec<DirEntry>, String> {
Ok(vec![])
}
async fn exec_command(&self, _: &str, _: u64, _: Option<&str>, _: Option<&std::collections::HashMap<String, String>>) -> Result<ExecResult, String> {
Ok(ExecResult {
stdout: String::new(),
stderr: String::new(),
exit_code: 0,
timed_out: false,
duration_ms: 0,
})
}
async fn grep(&self, _: &str, _: &str, _: &GrepOptions) -> Result<Vec<String>, String> {
Ok(vec![])
}
async fn glob(&self, _: &str, _path: Option<&str>) -> Result<Vec<String>, String> {
Ok(self.results.clone())
}
async fn initialize(&self) -> Result<(), String> {
Ok(())
}
async fn cleanup(&self) -> Result<(), String> {
Ok(())
}
fn working_directory(&self) -> &str {
"/tmp"
}
fn platform(&self) -> &str {
"darwin"
}
fn os_version(&self) -> String {
String::new()
}
}
#[tokio::test]
async fn read_file_returns_content() {
@ -903,14 +775,15 @@ mod tests {
#[tokio::test]
async fn shell_basic_command() {
let tool = make_shell_tool();
let env: Arc<dyn ExecutionEnvironment> = Arc::new(ShellEnv {
result: ExecResult {
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockExecutionEnvironment {
exec_result: ExecResult {
stdout: "hello".into(),
stderr: String::new(),
exit_code: 0,
timed_out: false,
duration_ms: 10,
},
..Default::default()
});
let result = (tool.executor)(serde_json::json!({"command": "echo hello"}), env).await;
let output = result.unwrap();
@ -936,14 +809,15 @@ mod tests {
#[tokio::test]
async fn shell_nonzero_exit_code() {
let tool = make_shell_tool();
let env: Arc<dyn ExecutionEnvironment> = Arc::new(ShellEnv {
result: ExecResult {
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockExecutionEnvironment {
exec_result: ExecResult {
stdout: String::new(),
stderr: "error".into(),
exit_code: 1,
timed_out: false,
duration_ms: 10,
},
..Default::default()
});
let result = (tool.executor)(serde_json::json!({"command": "false"}), env).await;
let output = result.unwrap();
@ -954,14 +828,15 @@ mod tests {
#[tokio::test]
async fn shell_timeout_output() {
let tool = make_shell_tool();
let env: Arc<dyn ExecutionEnvironment> = Arc::new(ShellEnv {
result: ExecResult {
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockExecutionEnvironment {
exec_result: ExecResult {
stdout: String::new(),
stderr: String::new(),
exit_code: -1,
timed_out: true,
duration_ms: 10000,
},
..Default::default()
});
let result = (tool.executor)(serde_json::json!({"command": "sleep 100"}), env).await;
let output = result.unwrap();
@ -971,8 +846,9 @@ mod tests {
#[tokio::test]
async fn grep_basic() {
let tool = make_grep_tool();
let env: Arc<dyn ExecutionEnvironment> = Arc::new(GrepEnv {
results: vec!["src/main.rs:10:fn main()".into(), "src/lib.rs:5:pub fn".into()],
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockExecutionEnvironment {
grep_results: vec!["src/main.rs:10:fn main()".into(), "src/lib.rs:5:pub fn".into()],
..Default::default()
});
let result = (tool.executor)(serde_json::json!({"pattern": "fn"}), env).await;
let output = result.unwrap();
@ -983,8 +859,9 @@ mod tests {
#[tokio::test]
async fn glob_basic() {
let tool = make_glob_tool();
let env: Arc<dyn ExecutionEnvironment> = Arc::new(GlobEnv {
results: vec!["src/main.rs".into(), "src/lib.rs".into()],
let env: Arc<dyn ExecutionEnvironment> = Arc::new(MockExecutionEnvironment {
glob_results: vec!["src/main.rs".into(), "src/lib.rs".into()],
..Default::default()
});
let result = (tool.executor)(serde_json::json!({"pattern": "src/**/*.rs"}), env).await;
let output = result.unwrap();

90
crates/coding-agent-loop/src/truncation.rs
View file

@ -1,5 +1,4 @@
use crate::config::SessionConfig;
use std::collections::HashMap;
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum TruncationMode {
@ -7,35 +6,34 @@ pub enum TruncationMode {
Tail,
}
fn default_char_limits() -> HashMap<&'static str, usize> {
let mut m = HashMap::new();
m.insert("read_file", 50_000);
m.insert("shell", 30_000);
m.insert("grep", 20_000);
m.insert("glob", 20_000);
m.insert("edit_file", 10_000);
m.insert("write_file", 1_000);
m.insert("apply_patch", 10_000);
m.insert("spawn_agent", 20_000);
m
fn default_char_limit(tool_name: &str) -> Option<usize> {
match tool_name {
"read_file" => Some(50_000),
"shell" => Some(30_000),
"grep" => Some(20_000),
"glob" => Some(20_000),
"edit_file" => Some(10_000),
"write_file" => Some(1_000),
"apply_patch" => Some(10_000),
"spawn_agent" => Some(20_000),
_ => None,
}
}
fn default_line_limits() -> HashMap<&'static str, usize> {
let mut m = HashMap::new();
m.insert("shell", 256);
m.insert("grep", 200);
m.insert("glob", 500);
m
fn default_line_limit(tool_name: &str) -> Option<usize> {
match tool_name {
"shell" => Some(256),
"grep" => Some(200),
"glob" => Some(500),
_ => None,
}
}
fn default_truncation_modes() -> HashMap<&'static str, TruncationMode> {
let mut m = HashMap::new();
m.insert("grep", TruncationMode::Tail);
m.insert("glob", TruncationMode::Tail);
m.insert("edit_file", TruncationMode::Tail);
m.insert("apply_patch", TruncationMode::Tail);
m.insert("write_file", TruncationMode::Tail);
m
fn default_truncation_mode(tool_name: &str) -> TruncationMode {
match tool_name {
"grep" | "glob" | "edit_file" | "apply_patch" | "write_file" => TruncationMode::Tail,
_ => TruncationMode::HeadTail,
}
}
pub fn truncate_output(output: &str, max_chars: usize, mode: TruncationMode) -> String {
@ -85,22 +83,14 @@ pub fn truncate_lines(output: &str, max_lines: usize) -> String {
}
pub fn truncate_tool_output(output: &str, tool_name: &str, config: &SessionConfig) -> String {
let builtin_char_limits = default_char_limits();
let builtin_line_limits = default_line_limits();
let builtin_modes = default_truncation_modes();
// Determine truncation mode for this tool (default HeadTail)
let mode = builtin_modes
.get(tool_name)
.copied()
.unwrap_or(TruncationMode::HeadTail);
let mode = default_truncation_mode(tool_name);
// Char truncation first
let char_limit = config
.tool_output_limits
.get(tool_name)
.copied()
.or_else(|| builtin_char_limits.get(tool_name).copied());
.or_else(|| default_char_limit(tool_name));
let after_chars = match char_limit {
Some(limit) => truncate_output(output, limit, mode),
@ -112,7 +102,7 @@ pub fn truncate_tool_output(output: &str, tool_name: &str, config: &SessionConfi
.tool_line_limits
.get(tool_name)
.copied()
.or_else(|| builtin_line_limits.get(tool_name).copied());
.or_else(|| default_line_limit(tool_name));
match line_limit {
Some(limit) => truncate_lines(&after_chars, limit),
@ -211,23 +201,23 @@ mod tests {
#[test]
fn default_char_limits_match_spec() {
let limits = default_char_limits();
assert_eq!(limits.get("read_file"), Some(&50_000));
assert_eq!(limits.get("shell"), Some(&30_000));
assert_eq!(limits.get("grep"), Some(&20_000));
assert_eq!(limits.get("glob"), Some(&20_000));
assert_eq!(limits.get("edit_file"), Some(&10_000));
assert_eq!(limits.get("write_file"), Some(&1_000));
assert_eq!(limits.get("apply_patch"), Some(&10_000));
assert_eq!(limits.get("spawn_agent"), Some(&20_000));
assert_eq!(default_char_limit("read_file"), Some(50_000));
assert_eq!(default_char_limit("shell"), Some(30_000));
assert_eq!(default_char_limit("grep"), Some(20_000));
assert_eq!(default_char_limit("glob"), Some(20_000));
assert_eq!(default_char_limit("edit_file"), Some(10_000));
assert_eq!(default_char_limit("write_file"), Some(1_000));
assert_eq!(default_char_limit("apply_patch"), Some(10_000));
assert_eq!(default_char_limit("spawn_agent"), Some(20_000));
assert_eq!(default_char_limit("unknown"), None);
}
#[test]
fn default_line_limits_match_spec() {
let limits = default_line_limits();
assert_eq!(limits.get("shell"), Some(&256));
assert_eq!(limits.get("grep"), Some(&200));
assert_eq!(limits.get("glob"), Some(&500));
assert_eq!(default_line_limit("shell"), Some(256));
assert_eq!(default_line_limit("grep"), Some(200));
assert_eq!(default_line_limit("glob"), Some(500));
assert_eq!(default_line_limit("unknown"), None);
}
#[test]

131
crates/coding-agent-loop/src/types.rs
View file

@ -1,4 +1,3 @@
use std::collections::HashMap;
use std::time::SystemTime;
use unified_llm::types::{ToolCall, ToolResult, Usage};
@ -56,129 +55,51 @@ pub enum EventKind {
Error,
}
#[derive(Debug, Clone)]
pub enum EventData {
Empty,
ToolCall {
tool_name: String,
tool_call_id: String,
},
ToolCallEnd {
tool_name: String,
tool_call_id: String,
output: serde_json::Value,
is_error: bool,
},
Error {
error: String,
},
ContextWarning {
estimated_tokens: usize,
context_window_size: usize,
usage_percent: usize,
},
}
#[derive(Debug, Clone)]
pub struct SessionEvent {
pub kind: EventKind,
pub timestamp: SystemTime,
pub session_id: String,
pub data: HashMap<String, serde_json::Value>,
pub data: EventData,
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn turn_user_construction() {
let turn = Turn::User {
content: "Hello".into(),
timestamp: SystemTime::now(),
};
match &turn {
Turn::User { content, .. } => assert_eq!(content, "Hello"),
_ => panic!("Expected User turn"),
}
}
#[test]
fn turn_assistant_construction() {
let turn = Turn::Assistant {
content: "Hi there".into(),
tool_calls: vec![],
reasoning: None,
usage: Usage::default(),
response_id: "resp_1".into(),
timestamp: SystemTime::now(),
};
match &turn {
Turn::Assistant {
content,
tool_calls,
reasoning,
response_id,
..
} => {
assert_eq!(content, "Hi there");
assert!(tool_calls.is_empty());
assert!(reasoning.is_none());
assert_eq!(response_id, "resp_1");
}
_ => panic!("Expected Assistant turn"),
}
}
#[test]
fn turn_tool_results_construction() {
let result = ToolResult {
tool_call_id: "call_1".into(),
content: serde_json::json!("result"),
is_error: false,
image_data: None,
image_media_type: None,
};
let turn = Turn::ToolResults {
results: vec![result],
timestamp: SystemTime::now(),
};
match &turn {
Turn::ToolResults { results, .. } => {
assert_eq!(results.len(), 1);
assert_eq!(results[0].tool_call_id, "call_1");
}
_ => panic!("Expected ToolResults turn"),
}
}
#[test]
fn turn_system_construction() {
let turn = Turn::System {
content: "System prompt".into(),
timestamp: SystemTime::now(),
};
match &turn {
Turn::System { content, .. } => assert_eq!(content, "System prompt"),
_ => panic!("Expected System turn"),
}
}
#[test]
fn turn_steering_construction() {
let turn = Turn::Steering {
content: "Focus on the task".into(),
timestamp: SystemTime::now(),
};
match &turn {
Turn::Steering { content, .. } => assert_eq!(content, "Focus on the task"),
_ => panic!("Expected Steering turn"),
}
}
#[test]
fn session_state_equality() {
assert_eq!(SessionState::Idle, SessionState::Idle);
assert_eq!(SessionState::Processing, SessionState::Processing);
assert_eq!(SessionState::AwaitingInput, SessionState::AwaitingInput);
assert_eq!(SessionState::Closed, SessionState::Closed);
assert_ne!(SessionState::Idle, SessionState::Closed);
}
#[test]
fn event_kind_equality() {
assert_eq!(EventKind::SessionStart, EventKind::SessionStart);
assert_ne!(EventKind::SessionStart, EventKind::SessionEnd);
assert_eq!(EventKind::LoopDetection, EventKind::LoopDetection);
}
#[test]
fn session_event_construction() {
let event = SessionEvent {
kind: EventKind::SessionStart,
timestamp: SystemTime::now(),
session_id: "sess_1".into(),
data: HashMap::new(),
data: EventData::Empty,
};
assert_eq!(event.kind, EventKind::SessionStart);
assert_eq!(event.session_id, "sess_1");
assert!(event.data.is_empty());
assert!(matches!(event.data, EventData::Empty));
}
}

476
docs/agent/reviews/coding-agent-loop-simplification.md Normal file
View file

@ -0,0 +1,476 @@
# coding-agent-loop Simplification Analysis
Date: 2026-02-20
## Executive Summary
The `coding-agent-loop` crate is approximately 4,800 lines of production code and tests across 19 source files. The architecture is generally sound, but there are significant opportunities to reduce complexity, eliminate duplication, and improve maintainability. The most impactful findings center on massive test mock duplication, duplicated tool execution logic in `session.rs`, and the `ProviderProfile` trait being too wide.
---
## HIGH Severity Findings
### 1. Massive Mock `ExecutionEnvironment` Duplication Across Tests
**What:** The `ExecutionEnvironment` trait has 12 methods, and a full mock implementation is copy-pasted into nearly every test module. I count at least **11 separate mock implementations** of `ExecutionEnvironment` spread across:
- `execution_env.rs` (`MockEnv`)
- `tool_registry.rs` (`DummyEnv`)
- `tools.rs` (`ReadFileEnv`, `WriteFileEnv`, `EditFileEnv`, `ShellEnv`, `ShellCapturingEnv`, `GrepEnv`, `GlobEnv`)
- `provider_profile.rs` (`TestEnv`)
- `project_docs.rs` (`DocEnv`)
- `profiles/mod.rs` (`TestEnv`)
- `profiles/anthropic.rs` (`TestEnv`)
- `profiles/gemini.rs` (`TestEnv`)
- `profiles/openai.rs` (`TestEnv`, `MockFileEnv`)
- `subagent.rs` (`MemoryExecutionEnvironment`)
- `session.rs` (`MemoryExecutionEnvironment`)
Each one is 30-60 lines of boilerplate implementing every trait method. Most implementations are identical stubs returning empty/default values, with only 1-2 methods customized per mock.
**Where:** Every file with `#[cfg(test)]` modules.
**Simplification:** Create a single `MockExecutionEnvironment` in a shared test utility module (e.g., `src/test_support.rs` behind `#[cfg(test)]`) that provides sensible defaults. Specific tests can then wrap or override individual methods using composition or builder patterns. This would eliminate approximately **500-700 lines** of duplicated test code.
```rust
// src/test_support.rs
#[cfg(test)]
pub struct MockExecutionEnvironment {
pub files: std::collections::HashMap<String, String>,
pub exec_result: Option<ExecResult>,
pub grep_results: Vec<String>,
pub glob_results: Vec<String>,
// ...
}
```
**Impact:** HIGH -- this is the single largest source of unnecessary code in the crate. It also makes adding new methods to `ExecutionEnvironment` extremely painful since every mock must be updated.
---
### 2. Duplicated Tool Execution Logic Between Sequential and Parallel Paths
**What:** `session.rs` contains two nearly identical implementations of tool execution:
1. `execute_single_tool` + `emit_execute_and_truncate` (used by the sequential path)
2. The inline closure in `execute_tool_calls_parallel` (lines 507-613)
Both paths:
- Emit `ToolCallStart` events
- Look up the tool in the registry
- Validate arguments against the schema
- Execute the tool
- Handle success/error into `ToolResult`
- Emit `ToolCallEnd` events with output data
- Truncate the output for history
The parallel path duplicates all of this logic inside a closure, including identical `ToolResult` construction, identical event emission, and identical truncation.
**Where:** `/crates/coding-agent-loop/src/session.rs`, lines 425-668.
**Simplification:** Extract a shared `execute_one_tool` function that takes the necessary context (emitter, registry, env, config, session_id) and returns the truncated `ToolResult`. Both the sequential and parallel paths should call this same function. The parallel path simply runs multiple instances concurrently with `join_all`.
This would eliminate approximately **80-100 lines** of duplicated logic and ensure bug fixes apply to both paths.
**Impact:** HIGH -- duplicated business logic is a correctness risk; fixing a bug in one path but not the other is easy.
---
### 3. `ProviderProfile` Trait Is Too Wide (14 Methods)
**What:** The `ProviderProfile` trait requires implementing 14 methods:
```rust
pub trait ProviderProfile: Send + Sync {
fn id(&self) -> String;
fn model(&self) -> String;
fn tool_registry(&self) -> &ToolRegistry;
fn tool_registry_mut(&mut self) -> &mut ToolRegistry;
fn build_system_prompt(...) -> String;
fn tools(&self) -> Vec<ToolDefinition>;
fn provider_options(&self) -> Option<serde_json::Value>;
fn supports_reasoning(&self) -> bool;
fn supports_streaming(&self) -> bool;
fn supports_parallel_tool_calls(&self) -> bool;
fn context_window_size(&self) -> usize;
fn knowledge_cutoff(&self) -> &str;
}
```
Several of these are pure data fields that don't need virtual dispatch. The `tools()` method is always just `self.registry.definitions()`. The `tool_registry()` and `tool_registry_mut()` methods exist only to allow external registration of subagent tools. This forces every test to implement all 14 methods even when only 1-2 matter.
**Where:** `/crates/coding-agent-loop/src/provider_profile.rs`
**Simplification:** Consider replacing the trait with a struct that holds data fields plus a closure/trait for the only truly polymorphic behavior (`build_system_prompt`). Alternatively, add default implementations where possible (e.g., `fn tools(&self) -> Vec<ToolDefinition> { self.tool_registry().definitions() }`). At minimum, `tools()` should have a default implementation since it's identical in all 3 profiles and every test profile.
The `supports_*` methods and `context_window_size` could be a `ProfileCapabilities` struct to reduce the trait surface.
**Impact:** HIGH -- affects every test file and every new profile implementation.
---
## MEDIUM Severity Findings
### 4. `register_subagent_tools` Is Copy-Pasted Across All Three Profiles
**What:** The `register_subagent_tools` method is identical in `AnthropicProfile`, `GeminiProfile`, and `OpenAiProfile`:
```rust
pub fn register_subagent_tools(
&mut self,
manager: Arc<tokio::sync::Mutex<SubAgentManager>>,
session_factory: SessionFactory,
current_depth: usize,
) {
self.registry.register(make_spawn_agent_tool(manager.clone(), session_factory, current_depth));
self.registry.register(make_send_input_tool(manager.clone()));
self.registry.register(make_wait_tool(manager.clone()));
self.registry.register(make_close_agent_tool(manager));
}
```
**Where:** `profiles/anthropic.rs:45-60`, `profiles/gemini.rs:45-60`, `profiles/openai.rs:45-60`
**Simplification:** Move this to a free function or a method on `ToolRegistry`:
```rust
pub fn register_subagent_tools(
registry: &mut ToolRegistry,
manager: Arc<tokio::sync::Mutex<SubAgentManager>>,
session_factory: SessionFactory,
current_depth: usize,
) { ... }
```
Or add it as a default method on `ProviderProfile` since the trait already has `tool_registry_mut()`.
**Impact:** MEDIUM -- 3x duplication of 8 lines each. Easy to drift.
---
### 5. `build_system_prompt` Duplicated Structure Across Profiles
**What:** All three profiles' `build_system_prompt` methods share identical preamble and postamble logic:
```rust
let env_block = build_env_context_block_with(env, env_context);
let docs_section = if project_docs.is_empty() {
String::new()
} else {
format!("\n\n{}", project_docs.join("\n\n"))
};
let user_section = match user_instructions {
Some(instructions) => format!("\n\n# User Instructions\n{instructions}"),
None => String::new(),
};
```
This identical block appears in `anthropic.rs:87-96`, `gemini.rs:87-96`, and `openai.rs:87-96`. Only the core prompt text differs.
**Where:** All three profile files.
**Simplification:** Extract a helper that takes the core prompt as a parameter:
```rust
fn assemble_system_prompt(
core_prompt: &str,
env: &dyn ExecutionEnvironment,
env_context: &EnvContext,
project_docs: &[String],
user_instructions: Option<&str>,
) -> String { ... }
```
Each profile would then only need to provide its unique prompt text.
**Impact:** MEDIUM -- reduces ~15 lines per profile, more importantly makes the structure consistent.
---
### 6. `SessionEvent.data` Uses `HashMap<String, serde_json::Value>` Instead of Typed Variants
**What:** Every event emitted throughout the codebase constructs a `HashMap<String, serde_json::Value>` manually:
```rust
let mut data = HashMap::new();
data.insert("tool_name".to_string(), serde_json::json!(&tc.name));
data.insert("tool_call_id".to_string(), serde_json::json!(&tc.id));
```
This pattern is repeated 15+ times across `session.rs`. The keys are stringly-typed and there's no compile-time guarantee about what data each event kind carries.
**Where:** `/crates/coding-agent-loop/src/session.rs` (throughout), `types.rs`
**Simplification:** Use typed event data enums:
```rust
pub enum EventData {
Empty,
ToolCall { tool_name: String, tool_call_id: String },
ToolCallEnd { tool_name: String, tool_call_id: String, output: serde_json::Value, is_error: bool },
Error { error: String },
ContextWarning { estimated_tokens: usize, context_window_size: usize, usage_percent: usize },
}
```
This removes all the `HashMap::new()` / `.insert()` boilerplate and provides type safety.
**Impact:** MEDIUM -- affects readability and correctness of event handling code.
---
### 7. `tools.rs` Exports `make_read_many_files_tool`, `make_list_dir_tool`, `make_web_search_tool`, `make_web_fetch_tool` But They Are Not Re-exported from `lib.rs`
**What:** `lib.rs` only re-exports:
```rust
pub use tools::{
make_edit_file_tool, make_glob_tool, make_grep_tool, make_read_file_tool, make_shell_tool,
make_shell_tool_with_config, make_write_file_tool,
};
```
But `tools.rs` also defines `make_read_many_files_tool`, `make_list_dir_tool`, `make_web_search_tool`, and `make_web_fetch_tool`. These are used internally by profiles (Gemini uses all of them, OpenAI uses `apply_patch`) but are not available to external consumers.
**Where:** `/crates/coding-agent-loop/src/lib.rs:31-34`, `/crates/coding-agent-loop/src/tools.rs`
**Simplification:** Either re-export all tools from `lib.rs` for consistency, or make the non-exported ones `pub(crate)` to clarify they're internal. The current state is ambiguous -- they're `pub` in `tools.rs` but not re-exported, suggesting an oversight.
**Impact:** MEDIUM -- confusing public API surface.
---
### 8. `TestProfile` / `MockLlmProvider` Duplicated Between `session.rs` and `subagent.rs`
**What:** Both `session.rs` and `subagent.rs` define their own:
- `MockLlmProvider` (identical implementation)
- `TestProfile` (identical implementation)
- `MemoryExecutionEnvironment` (nearly identical)
- `text_response` helper (identical)
- `make_client` helper (identical)
- `make_session` helper (identical)
**Where:** `session.rs` tests (lines 785-1135) and `subagent.rs` tests (lines 340-536).
**Simplification:** Extract these into a shared test support module. This would save approximately **200 lines** of duplicated test infrastructure.
**Impact:** MEDIUM -- significant duplication that makes maintenance harder.
---
### 9. `Io(String)` Error Variant Is Never Constructed
**What:** `AgentError::Io(String)` is defined and tested but never actually used anywhere in the production code. No code path constructs this variant.
**Where:** `/crates/coding-agent-loop/src/error.rs:18-19`
**Simplification:** Remove the variant (and its test) if it's truly unused. If it's intended for future use, add a `#[allow(dead_code)]` with a comment explaining when it will be needed.
**Impact:** MEDIUM -- dead code.
---
### 10. `History::new()` and `Default` Redundancy
**What:** `History` derives `Default` and also has a `new()` method that does the same thing. Both `new()` and `default()` return `Self { turns: Vec::new() }`.
**Where:** `/crates/coding-agent-loop/src/history.rs:4-11`
**Simplification:** Remove the manual `new()` and use `Default::default()` everywhere, or keep `new()` and remove the `Default` derive. The codebase uses `History::new()` everywhere, so keeping `new()` is fine, but having both is unnecessary. The `#[derive(Default)]` could be kept for flexibility since it's zero-cost.
**Impact:** LOW (but worth noting for consistency).
---
## LOW Severity Findings
### 11. `count_turns` Method Is Just `len()` by Another Name
**What:** `History::count_turns()` simply returns `self.turns.len()`. The name `count_turns` doesn't add semantic value over `len()` given the method already returns `&[Turn]` via `turns()`.
**Where:** `/crates/coding-agent-loop/src/history.rs:22-24`
**Simplification:** Replace `count_turns()` calls with `turns().len()` and remove the method, or rename to `len()` to follow Rust convention.
**Impact:** LOW.
---
### 12. `build_request` Calls `self.provider_profile.tools()` Twice
**What:** In `session.rs` `build_request()`:
```rust
let tools = self.provider_profile.tools();
// ...
tools: if tools.is_empty() { None } else { Some(tools) },
tool_choice: if self.provider_profile.tools().is_empty() { // <-- second call
None
} else {
Some(ToolChoice::Auto)
},
```
The second `self.provider_profile.tools()` call re-collects all tool definitions from the registry when it could just reuse the `tools` variable.
**Where:** `/crates/coding-agent-loop/src/session.rs:402-413`
**Simplification:**
```rust
let tools = self.provider_profile.tools();
let has_tools = !tools.is_empty();
// ...
tools: if has_tools { Some(tools) } else { None },
tool_choice: if has_tools { Some(ToolChoice::Auto) } else { None },
```
**Impact:** LOW -- minor inefficiency and readability issue.
---
### 13. `EnvContext` Fields `git_status_short` and `git_recent_commits` Are Populated But Never Used
**What:** `Session::build_env_context()` populates `git_status_short` and `git_recent_commits` from git commands, but `build_env_context_block_with()` never reads these fields. They are stored in the `EnvContext` struct but have no effect on the system prompt or any other behavior.
**Where:** `/crates/coding-agent-loop/src/session.rs:97-119`, `/crates/coding-agent-loop/src/profiles/mod.rs:29-55`
**Simplification:** Either use these fields in the environment context block (which seems to be the intent), or remove them and the git commands that populate them. Currently they cause two unnecessary shell invocations on every session initialization.
**Impact:** LOW -- dead code causing unnecessary I/O.
---
### 14. `truncation.rs` Rebuilds Default Limit HashMaps on Every Call
**What:** `truncate_tool_output` calls `default_char_limits()`, `default_line_limits()`, and `default_truncation_modes()` which each allocate and populate a new `HashMap` on every invocation.
**Where:** `/crates/coding-agent-loop/src/truncation.rs:87-121`
**Simplification:** Use `LazyLock` (stable in Rust 1.80+) or `const` arrays with a lookup function to avoid repeated allocation:
```rust
static DEFAULT_CHAR_LIMITS: LazyLock<HashMap<&str, usize>> = LazyLock::new(|| {
// ...
});
```
Alternatively, replace the `HashMap` lookups with simple match statements since the key sets are small and fixed.
**Impact:** LOW -- minor allocation overhead per tool call, but tool calls are not in a hot path.
---
### 15. `GrepOptions` Uses `grep` CLI Fallback That Will Always Succeed (Hiding `rg` Not Found)
**What:** In `local_env.rs`, the `grep` method checks if `rg --version` succeeds. But `std::process::Command::new("rg").arg("--version").status().is_ok()` returns `Ok` as long as the process was *launched*, not necessarily that it succeeded. The `.is_ok()` check is on the `Result` from `status()`, not on the exit code.
**Where:** `/crates/coding-agent-loop/src/local_env.rs:246-251`
**Simplification:** Check the exit code:
```rust
let use_rg = std::process::Command::new("rg")
.arg("--version")
.stdout(std::process::Stdio::null())
.stderr(std::process::Stdio::null())
.status()
.map(|s| s.success())
.unwrap_or(false);
```
**Impact:** LOW -- subtle correctness issue on systems where `rg` exists but returns an error.
---
### 16. `glob` Implementation Uses Shell Globbing via `ls -d` Which Is Fragile
**What:** The `glob` method in `local_env.rs` uses `sh -c "ls -d {pattern} 2>/dev/null"` to expand glob patterns. This is fragile because:
- Filenames with spaces or special characters will break
- The pattern is not shell-escaped
- `ls -d` behaves differently across platforms
**Where:** `/crates/coding-agent-loop/src/local_env.rs:300-333`
**Simplification:** Use the `glob` crate (a Rust-native glob implementation) instead of shelling out. This would be more reliable, cross-platform, and avoid shell injection concerns.
**Impact:** LOW for now (this is local-only), but worth addressing before any security-sensitive use.
---
### 17. Types Tests Are Overly Trivial
**What:** `types.rs` contains tests that merely construct enum variants and check that `PartialEq` works:
```rust
fn session_state_equality() {
assert_eq!(SessionState::Idle, SessionState::Idle);
assert_ne!(SessionState::Idle, SessionState::Closed);
}
```
These test the `#[derive(PartialEq)]` macro, which is guaranteed by the compiler.
**Where:** `/crates/coding-agent-loop/src/types.rs:67-184`
**Simplification:** Remove these tests. They add ~80 lines of code that test derived functionality and provide no value. The construction tests for `Turn` variants are slightly more useful as documentation but still marginal.
**Impact:** LOW -- no correctness value, just noise.
---
### 18. `apply_patch` Delete Operation Writes Empty String Instead of Deleting
**What:** `PatchOperation::Delete` is handled by writing an empty string to the file:
```rust
PatchOperation::Delete { path } => {
env.write_file(path, "").await?;
results.push(format!("Deleted file: {path}"));
}
```
This leaves a zero-byte file on disk rather than actually deleting it.
**Where:** `/crates/coding-agent-loop/src/profiles/openai.rs:359-362`
**Simplification:** Add a `delete_file` method to `ExecutionEnvironment`, or use `exec_command("rm ...")`. Writing empty content and calling it "deleted" is misleading.
**Impact:** LOW -- the current behavior may be intentional to avoid adding a `delete_file` method to the trait, but it's semantically wrong.
---
## Structural Observations
### File Organization
The module structure is reasonable. A few observations:
1. **`profiles/openai.rs` contains the entire v4a patch parser** (~200 lines). This is OpenAI-specific tooling that could be its own module (`src/patch_v4a.rs`) for clarity, since it's a self-contained parser/applier.
2. **`tools.rs` and `subagent.rs` both define tool factories** (functions that return `RegisteredTool`). The tools in `tools.rs` are "standard" tools while `subagent.rs` has subagent-specific tools. This split makes sense but the non-standard tools (`make_list_dir_tool`, `make_read_many_files_tool`, etc.) are only used by specific profiles and could be co-located with those profiles.
3. **`provider_profile.rs` and `profiles/mod.rs`** -- the trait is in one file and the `EnvContext` struct + `build_env_context_block` functions are in another. These are tightly coupled and could be consolidated.
### Approximate Line Count Savings
| Finding | Estimated Lines Saved |
|---------|----------------------|
| #1 Shared test mock | 500-700 |
| #2 Deduplicate tool execution | 80-100 |
| #4 Shared subagent registration | 20 |
| #5 Shared prompt assembly | 40 |
| #8 Shared test infrastructure | 200 |
| #9 Remove dead Io variant | 10 |
| #17 Remove trivial tests | 80 |
| **Total** | **~930-1150 lines** |
This represents roughly 20-25% of the crate's total size, with the vast majority coming from test deduplication.
---
## Recommended Priority Order
1. **Shared test mock for `ExecutionEnvironment`** (#1, #8) -- highest impact, eliminates the most duplication
2. **Deduplicate tool execution in session.rs** (#2) -- correctness risk
3. **Extract shared prompt assembly** (#5) + **shared subagent registration** (#4)
4. **Narrow the `ProviderProfile` trait** (#3) -- architectural improvement
5. **Fix unused `EnvContext` fields** (#13) -- removes unnecessary I/O
6. **Type the event data** (#6) -- readability improvement
7. **Clean up minor issues** (#9, #11, #12, #14, #15, #17)