From db32f00cd07da3563280f35c71a4d58eb7ee9219 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 20 Feb 2026 16:34:29 -0400 Subject: [PATCH] Simplify coding-agent-loop: deduplicate mocks, narrow traits, type events - 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 --- Cargo.lock | 42 + crates/coding-agent-loop/Cargo.toml | 1 + crates/coding-agent-loop/src/error.rs | 9 - crates/coding-agent-loop/src/event.rs | 45 +- crates/coding-agent-loop/src/execution_env.rs | 85 +- crates/coding-agent-loop/src/history.rs | 14 +- crates/coding-agent-loop/src/lib.rs | 7 +- crates/coding-agent-loop/src/local_env.rs | 23 +- .../src/profiles/anthropic.rs | 147 +-- .../coding-agent-loop/src/profiles/gemini.rs | 149 +-- crates/coding-agent-loop/src/profiles/mod.rs | 103 +- .../coding-agent-loop/src/profiles/openai.rs | 155 +-- crates/coding-agent-loop/src/project_docs.rs | 83 +- .../coding-agent-loop/src/provider_profile.rs | 179 ++-- crates/coding-agent-loop/src/session.rs | 928 ++++-------------- crates/coding-agent-loop/src/subagent.rs | 210 +--- crates/coding-agent-loop/src/test_support.rs | 478 +++++++++ crates/coding-agent-loop/src/tool_registry.rs | 65 +- crates/coding-agent-loop/src/tools.rs | 189 +--- crates/coding-agent-loop/src/truncation.rs | 90 +- crates/coding-agent-loop/src/types.rs | 131 +-- .../coding-agent-loop-simplification.md | 476 +++++++++ 22 files changed, 1581 insertions(+), 2028 deletions(-) create mode 100644 crates/coding-agent-loop/src/test_support.rs create mode 100644 docs/agent/reviews/coding-agent-loop-simplification.md diff --git a/Cargo.lock b/Cargo.lock index 2716094bf..dd84cda23 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -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" diff --git a/crates/coding-agent-loop/Cargo.toml b/crates/coding-agent-loop/Cargo.toml index fa6dd56ba..45812e73f 100644 --- a/crates/coding-agent-loop/Cargo.toml +++ b/crates/coding-agent-loop/Cargo.toml @@ -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" diff --git a/crates/coding-agent-loop/src/error.rs b/crates/coding-agent-loop/src/error.rs index 061fb18c2..7af71778f 100644 --- a/crates/coding-agent-loop/src/error.rs +++ b/crates/coding-agent-loop/src/error.rs @@ -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; diff --git a/crates/coding-agent-loop/src/event.rs b/crates/coding-agent-loop/src/event.rs index d773dde1f..625498339 100644 --- a/crates/coding-agent-loop/src/event.rs +++ b/crates/coding-agent-loop/src/event.rs @@ -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, - ) { + 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] diff --git a/crates/coding-agent-loop/src/execution_env.rs b/crates/coding-agent-loop/src/execution_env.rs index 504ba82a1..1b3bc07d5 100644 --- a/crates/coding-agent-loop/src/execution_env.rs +++ b/crates/coding-agent-loop/src/execution_env.rs @@ -27,6 +27,7 @@ pub struct GrepOptions { pub trait ExecutionEnvironment: Send + Sync { async fn read_file(&self, path: &str, offset: Option, limit: Option) -> Result; 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; async fn list_directory(&self, path: &str, depth: Option) -> Result, 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, _limit: Option) -> Result { - Ok("hello".into()) - } - async fn write_file(&self, _path: &str, _content: &str) -> Result<(), String> { - Ok(()) - } - async fn file_exists(&self, _path: &str) -> Result { - Ok(true) - } - async fn list_directory(&self, _path: &str, _depth: Option) -> Result, 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>, - ) -> Result { - 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, String> { - Ok(vec!["match".into()]) - } - async fn glob(&self, _pattern: &str, _path: Option<&str>) -> Result, 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 = Arc::new(MockEnv); + let mut files = HashMap::new(); + files.insert("test.rs".into(), "hello".into()); + let env: Arc = 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 = Arc::new(MockEnv); + let env: Arc = 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 = Arc::new(MockEnv); + let env: Arc = 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"); } } diff --git a/crates/coding-agent-loop/src/history.rs b/crates/coding-agent-loop/src/history.rs index c66e3ad4d..23a101979 100644 --- a/crates/coding-agent-loop/src/history.rs +++ b/crates/coding-agent-loop/src/history.rs @@ -19,10 +19,6 @@ impl History { &self.turns } - pub fn count_turns(&self) -> usize { - self.turns.len() - } - pub fn convert_to_messages(&self) -> Vec { 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] diff --git a/crates/coding-agent-loop/src/lib.rs b/crates/coding-agent-loop/src/lib.rs index b886d3012..358d6c8df 100644 --- a/crates/coding-agent-loop/src/lib.rs +++ b/crates/coding-agent-loop/src/lib.rs @@ -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; diff --git a/crates/coding-agent-loop/src/local_env.rs b/crates/coding-agent-loop/src/local_env.rs index bd387a1c6..29fd247b8 100644 --- a/crates/coding-agent-loop/src/local_env.rs +++ b/crates/coding-agent-loop/src/local_env.rs @@ -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 { 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 = stdout.lines().map(String::from).filter(|l| !l.is_empty()).collect(); + let mut results: Vec = 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| { diff --git a/crates/coding-agent-loop/src/profiles/anthropic.rs b/crates/coding-agent-loop/src/profiles/anthropic.rs index 2bbf2defb..558a4e376 100644 --- a/crates/coding-agent-loop/src/profiles/anthropic.rs +++ b/crates/coding-agent-loop/src/profiles/anthropic.rs @@ -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>, - 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 { - 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 { @@ -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, _: Option) -> Result { - Ok(String::new()) - } - async fn write_file(&self, _: &str, _: &str) -> Result<(), String> { - Ok(()) - } - async fn file_exists(&self, _: &str) -> Result { - Ok(false) - } - async fn list_directory(&self, _: &str, _: Option) -> Result, String> { - Ok(vec![]) - } - async fn exec_command( - &self, - _: &str, - _: u64, - _: Option<&str>, - _: Option<&std::collections::HashMap>, - ) -> Result { - 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, String> { - Ok(vec![]) - } - async fn glob(&self, _: &str, _: Option<&str>) -> Result, 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("")); @@ -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")); diff --git a/crates/coding-agent-loop/src/profiles/gemini.rs b/crates/coding-agent-loop/src/profiles/gemini.rs index 3f06ef736..bd84c4274 100644 --- a/crates/coding-agent-loop/src/profiles/gemini.rs +++ b/crates/coding-agent-loop/src/profiles/gemini.rs @@ -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>, - 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 { - 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 { @@ -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, _: Option) -> Result { - Ok(String::new()) - } - async fn write_file(&self, _: &str, _: &str) -> Result<(), String> { - Ok(()) - } - async fn file_exists(&self, _: &str) -> Result { - Ok(false) - } - async fn list_directory(&self, _: &str, _: Option) -> Result, String> { - Ok(vec![]) - } - async fn exec_command( - &self, - _: &str, - _: u64, - _: Option<&str>, - _: Option<&std::collections::HashMap>, - ) -> Result { - 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, String> { - Ok(vec![]) - } - async fn glob(&self, _: &str, _: Option<&str>) -> Result, 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("")); assert!(prompt.contains("linux")); diff --git a/crates/coding-agent-loop/src/profiles/mod.rs b/crates/coding-agent-loop/src/profiles/mod.rs index 3148fe4f5..b2be81067 100644 --- a/crates/coding-agent-loop/src/profiles/mod.rs +++ b/crates/coding-agent-loop/src/profiles/mod.rs @@ -20,6 +20,33 @@ pub struct EnvContext { pub git_recent_commits: Option, } +/// 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("".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, _: Option) -> Result { - Ok(String::new()) - } - async fn write_file(&self, _: &str, _: &str) -> Result<(), String> { - Ok(()) - } - async fn file_exists(&self, _: &str) -> Result { - Ok(false) - } - async fn list_directory(&self, _: &str, _: Option) -> Result, String> { - Ok(vec![]) - } - async fn exec_command( - &self, - _: &str, - _: u64, - _: Option<&str>, - _: Option<&std::collections::HashMap>, - ) -> Result { - 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, String> { - Ok(vec![]) - } - async fn glob(&self, _: &str, _: Option<&str>) -> Result, 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("")); assert!(block.contains("")); @@ -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, diff --git a/crates/coding-agent-loop/src/profiles/openai.rs b/crates/coding-agent-loop/src/profiles/openai.rs index 3eeb0e166..2fe1e9e05 100644 --- a/crates/coding-agent-loop/src/profiles/openai.rs +++ b/crates/coding-agent-loop/src/profiles/openai.rs @@ -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) { self.reasoning_effort = effort; } - - pub fn register_subagent_tools( - &mut self, - manager: Arc>, - 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 { - 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 { @@ -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, _: Option) -> Result { - Ok(String::new()) - } - async fn write_file(&self, _: &str, _: &str) -> Result<(), String> { - Ok(()) - } - async fn file_exists(&self, _: &str) -> Result { - Ok(false) - } - async fn list_directory(&self, _: &str, _: Option) -> Result, String> { - Ok(vec![]) - } - async fn exec_command( - &self, - _: &str, - _: u64, - _: Option<&str>, - _: Option<&std::collections::HashMap>, - ) -> Result { - 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, String> { - Ok(vec![]) - } - async fn glob(&self, _: &str, _: Option<&str>) -> Result, 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>, } @@ -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 { 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("")); @@ -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")); diff --git a/crates/coding-agent-loop/src/project_docs.rs b/crates/coding-agent-loop/src/project_docs.rs index 4b9ed8d40..610db61ea 100644 --- a/crates/coding-agent-loop/src/project_docs.rs +++ b/crates/coding-agent-loop/src/project_docs.rs @@ -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, - } - - #[async_trait] - impl ExecutionEnvironment for DocEnv { - async fn read_file(&self, path: &str, _: Option, _: Option) -> Result { - 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 { - Ok(self.files.contains_key(path)) - } - async fn list_directory(&self, _: &str, _: Option) -> Result, String> { - Ok(vec![]) - } - async fn exec_command(&self, _: &str, _: u64, _: Option<&str>, _: Option<&std::collections::HashMap>) -> Result { - 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, String> { - Ok(vec![]) - } - async fn glob(&self, _: &str, _: Option<&str>) -> Result, 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 = Arc::new(DocEnv { files }); + let env: Arc = 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 = Arc::new(DocEnv { + let env: Arc = 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 = Arc::new(DocEnv { + let env: Arc = 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 = Arc::new(DocEnv { files }); + let env: Arc = 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 = Arc::new(DocEnv { files }); + let env: Arc = 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 = Arc::new(DocEnv { files }); + let env: Arc = 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); diff --git a/crates/coding-agent-loop/src/provider_profile.rs b/crates/coding-agent-loop/src/provider_profile.rs index 63b13a119..99c7f5d5f 100644 --- a/crates/coding-agent-loop/src/provider_profile.rs +++ b/crates/coding-agent-loop/src/provider_profile.rs @@ -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; - fn provider_options(&self) -> Option; - 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 { + self.tool_registry().definitions() + } + + fn provider_options(&self) -> Option { + 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>, + 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, _: Option) -> Result { - Ok(String::new()) - } - async fn write_file(&self, _: &str, _: &str) -> Result<(), String> { - Ok(()) - } - async fn file_exists(&self, _: &str) -> Result { - Ok(false) - } - async fn list_directory(&self, _: &str, _: Option) -> Result, String> { - Ok(vec![]) - } - async fn exec_command( - &self, - _: &str, - _: u64, - _: Option<&str>, - _: Option<&std::collections::HashMap>, - ) -> Result { - 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, String> { - Ok(vec![]) - } - async fn glob(&self, _: &str, _: Option<&str>) -> Result, 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 { - self.registry.definitions() - } - fn provider_options(&self) -> Option { - 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()); } } diff --git a/crates/coding-agent-loop/src/session.rs b/crates/coding-agent-loop/src/session.rs index 42607aefb..980ec72e2 100644 --- a/crates/coding-agent-loop/src/session.rs +++ b/crates/coding-agent-loop/src/session.rs @@ -1,14 +1,16 @@ use crate::config::SessionConfig; use crate::error::AgentError; use crate::event::EventEmitter; +use crate::execution_env::ExecutionEnvironment; use crate::history::History; use crate::loop_detection::detect_loop; use crate::profiles::EnvContext; use crate::project_docs::discover_project_docs; use crate::provider_profile::ProviderProfile; +use crate::tool_registry::ToolRegistry; use crate::truncation::truncate_tool_output; -use crate::types::{EventKind, SessionEvent, SessionState, Turn}; -use std::collections::{HashMap, VecDeque}; +use crate::types::{EventData, EventKind, SessionState, Turn}; +use std::collections::VecDeque; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex}; use std::time::SystemTime; @@ -16,8 +18,6 @@ use unified_llm::client::Client; use unified_llm::error::{ProviderErrorKind, SdkError}; use unified_llm::types::{Message, Request, ToolChoice, ToolResult}; -use crate::execution_env::ExecutionEnvironment; - pub struct Session { id: String, config: SessionConfig, @@ -133,7 +133,7 @@ impl Session { self.state } - pub fn subscribe(&self) -> tokio::sync::broadcast::Receiver { + pub fn subscribe(&self) -> tokio::sync::broadcast::Receiver { self.event_emitter.subscribe() } @@ -184,11 +184,8 @@ impl Session { return Err(AgentError::SessionClosed); } - self.event_emitter.emit( - EventKind::SessionStart, - self.id.clone(), - HashMap::new(), - ); + self.event_emitter + .emit(EventKind::SessionStart, self.id.clone(), EventData::Empty); // Use a queue to avoid recursive async calls for followups let mut current_input = input.to_string(); @@ -211,11 +208,8 @@ impl Session { } self.state = SessionState::Idle; - self.event_emitter.emit( - EventKind::SessionEnd, - self.id.clone(), - HashMap::new(), - ); + self.event_emitter + .emit(EventKind::SessionEnd, self.id.clone(), EventData::Empty); Ok(()) } @@ -232,11 +226,8 @@ impl Session { content: input.to_string(), timestamp: SystemTime::now(), }); - self.event_emitter.emit( - EventKind::UserInput, - self.id.clone(), - HashMap::new(), - ); + self.event_emitter + .emit(EventKind::UserInput, self.id.clone(), EventData::Empty); // Drain steering queue before first LLM call self.drain_steering(); @@ -246,32 +237,23 @@ impl Session { loop { // Check max_tool_rounds_per_input if round_count >= self.config.max_tool_rounds_per_input { - self.event_emitter.emit( - EventKind::TurnLimit, - self.id.clone(), - HashMap::new(), - ); + self.event_emitter + .emit(EventKind::TurnLimit, self.id.clone(), EventData::Empty); break; } // Check max_turns - if self.config.max_turns > 0 && self.history.count_turns() >= self.config.max_turns { - self.event_emitter.emit( - EventKind::TurnLimit, - self.id.clone(), - HashMap::new(), - ); + if self.config.max_turns > 0 && self.history.turns().len() >= self.config.max_turns { + self.event_emitter + .emit(EventKind::TurnLimit, self.id.clone(), EventData::Empty); break; } // Check abort flag if self.abort_flag.load(Ordering::SeqCst) { self.state = SessionState::Closed; - self.event_emitter.emit( - EventKind::SessionEnd, - self.id.clone(), - HashMap::new(), - ); + self.event_emitter + .emit(EventKind::SessionEnd, self.id.clone(), EventData::Empty); return Err(AgentError::Aborted); } @@ -282,22 +264,19 @@ impl Session { self.event_emitter.emit( EventKind::AssistantTextStart, self.id.clone(), - HashMap::new(), + EventData::Empty, ); // Call LLM let response = match self.llm_client.complete(&request).await { Ok(resp) => resp, Err(err) => { - let mut error_data = HashMap::new(); - error_data.insert( - "error".to_string(), - serde_json::json!(err.to_string()), - ); self.event_emitter.emit( EventKind::Error, self.id.clone(), - error_data, + EventData::Error { + error: err.to_string(), + }, ); if is_auth_error(&err) { self.state = SessionState::Closed; @@ -325,7 +304,7 @@ impl Session { self.event_emitter.emit( EventKind::AssistantTextEnd, self.id.clone(), - HashMap::new(), + EventData::Empty, ); // Check context window usage @@ -361,7 +340,7 @@ impl Session { self.event_emitter.emit( EventKind::LoopDetection, self.id.clone(), - HashMap::new(), + EventData::Empty, ); } } @@ -384,7 +363,7 @@ impl Session { self.event_emitter.emit( EventKind::SteeringInjected, self.id.clone(), - HashMap::new(), + EventData::Empty, ); } } @@ -400,16 +379,17 @@ impl Session { messages.extend(self.history.convert_to_messages()); let tools = self.provider_profile.tools(); + let has_tools = !tools.is_empty(); Request { model: self.provider_profile.model(), messages, provider: Some(self.provider_profile.id()), - tools: if tools.is_empty() { None } else { Some(tools) }, - tool_choice: if self.provider_profile.tools().is_empty() { - None - } else { + tools: if has_tools { Some(tools) } else { None }, + tool_choice: if has_tools { Some(ToolChoice::Auto) + } else { + None }, response_format: None, temperature: None, @@ -422,56 +402,6 @@ impl Session { } } - async fn execute_single_tool( - &self, - tool_call_id: &str, - tool_name: &str, - arguments: &serde_json::Value, - ) -> ToolResult { - let registry = self.provider_profile.tool_registry(); - match registry.get(tool_name) { - Some(registered_tool) => { - // Validate arguments against schema - if let Err(validation_error) = - validate_tool_args(®istered_tool.definition.parameters, arguments) - { - return ToolResult { - tool_call_id: tool_call_id.to_string(), - content: serde_json::json!(validation_error), - is_error: true, - image_data: None, - image_media_type: None, - }; - } - - let executor = ®istered_tool.executor; - match executor(arguments.clone(), self.execution_env.clone()).await { - Ok(output) => ToolResult { - tool_call_id: tool_call_id.to_string(), - content: serde_json::json!(output), - is_error: false, - image_data: None, - image_media_type: None, - }, - Err(err) => ToolResult { - tool_call_id: tool_call_id.to_string(), - content: serde_json::json!(err), - is_error: true, - image_data: None, - image_media_type: None, - }, - } - } - None => ToolResult { - tool_call_id: tool_call_id.to_string(), - content: serde_json::json!(format!("Unknown tool: {tool_name}")), - is_error: true, - image_data: None, - image_media_type: None, - }, - } - } - async fn execute_tool_calls( &mut self, tool_calls: &[unified_llm::types::ToolCall], @@ -489,7 +419,37 @@ impl Session { ) -> Vec { let mut results = Vec::new(); for tc in tool_calls { - results.push(self.emit_execute_and_truncate(tc).await); + self.event_emitter.emit( + EventKind::ToolCallStart, + self.id.clone(), + EventData::ToolCall { + tool_name: tc.name.clone(), + tool_call_id: tc.id.clone(), + }, + ); + + let result = execute_one_tool( + &tc.id, + &tc.name, + &tc.arguments, + self.provider_profile.tool_registry(), + self.execution_env.clone(), + ) + .await; + + self.event_emitter.emit( + EventKind::ToolCallEnd, + self.id.clone(), + EventData::ToolCallEnd { + tool_name: tc.name.clone(), + tool_call_id: tc.id.clone(), + output: result.content.clone(), + is_error: result.is_error, + }, + ); + + let truncated = truncate_tool_result(&result, &tc.name, &self.config); + results.push(truncated); } results } @@ -514,97 +474,36 @@ impl Session { let config = config.clone(); let tc = tc.clone(); async move { - // Emit ToolCallStart - let mut start_data = HashMap::new(); - start_data - .insert("tool_name".to_string(), serde_json::json!(&tc.name)); - start_data - .insert("tool_call_id".to_string(), serde_json::json!(&tc.id)); emitter.emit( EventKind::ToolCallStart, session_id.clone(), - start_data, - ); - - // Execute tool - let registry = profile.tool_registry(); - let result = match registry.get(&tc.name) { - Some(registered_tool) => { - // Validate arguments against schema - if let Err(validation_error) = validate_tool_args( - ®istered_tool.definition.parameters, - &tc.arguments, - ) { - ToolResult { - tool_call_id: tc.id.clone(), - content: serde_json::json!(validation_error), - is_error: true, - image_data: None, - image_media_type: None, - } - } else { - match (registered_tool.executor)(tc.arguments.clone(), env).await { - Ok(output) => ToolResult { - tool_call_id: tc.id.clone(), - content: serde_json::json!(output), - is_error: false, - image_data: None, - image_media_type: None, - }, - Err(err) => ToolResult { - tool_call_id: tc.id.clone(), - content: serde_json::json!(err), - is_error: true, - image_data: None, - image_media_type: None, - }, - } - } - } - None => ToolResult { + EventData::ToolCall { + tool_name: tc.name.clone(), tool_call_id: tc.id.clone(), - content: serde_json::json!(format!("Unknown tool: {}", tc.name)), - is_error: true, - image_data: None, - image_media_type: None, }, - }; - - // Emit ToolCallEnd - let mut end_data = HashMap::new(); - end_data - .insert("tool_name".to_string(), serde_json::json!(&tc.name)); - end_data - .insert("tool_call_id".to_string(), serde_json::json!(&tc.id)); - match &result.content { - serde_json::Value::String(s) => { - end_data.insert("output".to_string(), serde_json::json!(s)); - } - other => { - end_data.insert("output".to_string(), other.clone()); - } - } - end_data.insert( - "is_error".to_string(), - serde_json::json!(result.is_error), ); - emitter.emit(EventKind::ToolCallEnd, session_id, end_data); - // Truncate for history - let truncated_content = match &result.content { - serde_json::Value::String(s) => { - serde_json::json!(truncate_tool_output(s, &tc.name, &config)) - } - other => other.clone(), - }; + let result = execute_one_tool( + &tc.id, + &tc.name, + &tc.arguments, + profile.tool_registry(), + env, + ) + .await; - ToolResult { - tool_call_id: result.tool_call_id, - content: truncated_content, - is_error: result.is_error, - image_data: result.image_data, - image_media_type: result.image_media_type, - } + emitter.emit( + EventKind::ToolCallEnd, + session_id, + EventData::ToolCallEnd { + tool_name: tc.name.clone(), + tool_call_id: tc.id.clone(), + output: result.content.clone(), + is_error: result.is_error, + }, + ); + + truncate_tool_result(&result, &tc.name, &config) } }) .collect(); @@ -612,61 +511,6 @@ impl Session { futures::future::join_all(futures).await } - async fn emit_execute_and_truncate( - &self, - tc: &unified_llm::types::ToolCall, - ) -> ToolResult { - // Emit ToolCallStart - let mut start_data = HashMap::new(); - start_data.insert("tool_name".to_string(), serde_json::json!(tc.name)); - start_data.insert("tool_call_id".to_string(), serde_json::json!(tc.id)); - self.event_emitter.emit( - EventKind::ToolCallStart, - self.id.clone(), - start_data, - ); - - let result = self - .execute_single_tool(&tc.id, &tc.name, &tc.arguments) - .await; - - // Emit ToolCallEnd with full untruncated output - let mut end_data = HashMap::new(); - end_data.insert("tool_name".to_string(), serde_json::json!(tc.name)); - end_data.insert("tool_call_id".to_string(), serde_json::json!(tc.id)); - match &result.content { - serde_json::Value::String(s) => { - end_data.insert("output".to_string(), serde_json::json!(s)); - } - other => { - end_data.insert("output".to_string(), other.clone()); - } - } - end_data.insert("is_error".to_string(), serde_json::json!(result.is_error)); - self.event_emitter.emit( - EventKind::ToolCallEnd, - self.id.clone(), - end_data, - ); - - // Truncate output for history - let truncated_content = match &result.content { - serde_json::Value::String(s) => { - let truncated = truncate_tool_output(s, &tc.name, &self.config); - serde_json::json!(truncated) - } - other => other.clone(), - }; - - ToolResult { - tool_call_id: result.tool_call_id, - content: truncated_content, - is_error: result.is_error, - image_data: result.image_data, - image_media_type: result.image_media_type, - } - } - fn estimate_token_count(&self) -> usize { let system_prompt = self.provider_profile.build_system_prompt( self.execution_env.as_ref(), @@ -714,28 +558,91 @@ impl Session { let threshold = context_window * 80 / 100; if estimated_tokens > threshold { - let mut data = HashMap::new(); - data.insert( - "estimated_tokens".to_string(), - serde_json::json!(estimated_tokens), - ); - data.insert( - "context_window_size".to_string(), - serde_json::json!(context_window), - ); - data.insert( - "usage_percent".to_string(), - serde_json::json!(estimated_tokens * 100 / context_window), - ); self.event_emitter.emit( EventKind::ContextWindowWarning, self.id.clone(), - data, + EventData::ContextWarning { + estimated_tokens, + context_window_size: context_window, + usage_percent: estimated_tokens * 100 / context_window, + }, ); } } } +/// Execute a single tool call: registry lookup, argument validation, and execution. +/// Shared by both sequential and parallel execution paths. +async fn execute_one_tool( + tool_call_id: &str, + tool_name: &str, + arguments: &serde_json::Value, + registry: &ToolRegistry, + env: Arc, +) -> ToolResult { + match registry.get(tool_name) { + Some(registered_tool) => { + if let Err(validation_error) = + validate_tool_args(®istered_tool.definition.parameters, arguments) + { + return ToolResult { + tool_call_id: tool_call_id.to_string(), + content: serde_json::json!(validation_error), + is_error: true, + image_data: None, + image_media_type: None, + }; + } + + match (registered_tool.executor)(arguments.clone(), env).await { + Ok(output) => ToolResult { + tool_call_id: tool_call_id.to_string(), + content: serde_json::json!(output), + is_error: false, + image_data: None, + image_media_type: None, + }, + Err(err) => ToolResult { + tool_call_id: tool_call_id.to_string(), + content: serde_json::json!(err), + is_error: true, + image_data: None, + image_media_type: None, + }, + } + } + None => ToolResult { + tool_call_id: tool_call_id.to_string(), + content: serde_json::json!(format!("Unknown tool: {tool_name}")), + is_error: true, + image_data: None, + image_media_type: None, + }, + } +} + +/// Truncate tool output for history storage while preserving identity fields. +fn truncate_tool_result( + result: &ToolResult, + tool_name: &str, + config: &SessionConfig, +) -> ToolResult { + let truncated_content = match &result.content { + serde_json::Value::String(s) => { + serde_json::json!(truncate_tool_output(s, tool_name, config)) + } + other => other.clone(), + }; + + ToolResult { + tool_call_id: result.tool_call_id.clone(), + content: truncated_content, + is_error: result.is_error, + image_data: result.image_data.clone(), + image_media_type: result.image_media_type.clone(), + } +} + fn is_auth_error(err: &SdkError) -> bool { matches!( err.provider_kind(), @@ -769,370 +676,12 @@ fn validate_tool_args(schema: &serde_json::Value, args: &serde_json::Value) -> R #[cfg(test)] mod tests { use super::*; - use crate::execution_env::*; + use crate::test_support::*; use crate::tool_registry::{RegisteredTool, ToolRegistry}; use async_trait::async_trait; - use std::collections::HashMap; - use std::sync::atomic::AtomicUsize; use unified_llm::error::ProviderErrorDetail; use unified_llm::provider::{ProviderAdapter, StreamEventStream}; - use unified_llm::types::{ - ContentPart, FinishReason, Response, ToolCall, ToolDefinition, Usage, - }; - - // --- Mock LLM Provider --- - - struct MockLlmProvider { - responses: Vec, - call_index: AtomicUsize, - } - - impl MockLlmProvider { - fn new(responses: Vec) -> 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 { - let idx = self.call_index.fetch_add(1, Ordering::SeqCst); - if idx < self.responses.len() { - Ok(self.responses[idx].clone()) - } else { - // Return last response if we exceed - Ok(self.responses[self.responses.len() - 1].clone()) - } - } - - async fn stream( - &self, - _request: &Request, - ) -> Result { - Err(SdkError::Configuration { - message: "streaming not supported in mock".into(), - }) - } - } - - // --- Mock Error Provider --- - - struct MockErrorProvider { - error: SdkError, - } - - #[async_trait] - impl ProviderAdapter for MockErrorProvider { - fn name(&self) -> &str { - "mock" - } - - async fn complete(&self, _request: &Request) -> Result { - Err(self.error.clone()) - } - - async fn stream( - &self, - _request: &Request, - ) -> Result { - Err(SdkError::Configuration { - message: "streaming not supported in mock".into(), - }) - } - } - - // --- Memory Execution Environment --- - - struct MemoryExecutionEnvironment { - files: HashMap, - } - - impl MemoryExecutionEnvironment { - fn new() -> Self { - Self { - files: HashMap::new(), - } - } - } - - #[async_trait] - impl ExecutionEnvironment for MemoryExecutionEnvironment { - async fn read_file(&self, path: &str, _offset: Option, _limit: Option) -> Result { - 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 file_exists(&self, path: &str) -> Result { - Ok(self.files.contains_key(path)) - } - - async fn list_directory(&self, _path: &str, _depth: Option) -> Result, String> { - Ok(vec![]) - } - - async fn exec_command( - &self, - _command: &str, - _timeout_ms: u64, - _working_dir: Option<&str>, - _env_vars: Option<&std::collections::HashMap>, - ) -> Result { - 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, String> { - Ok(vec![]) - } - - async fn glob(&self, _pattern: &str, _path: Option<&str>) -> Result, 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(), - } - } - - 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: &crate::profiles::EnvContext, - _project_docs: &[String], - _user_instructions: Option<&str>, - ) -> String { - "You are a test assistant.".into() - } - - fn tools(&self) -> Vec { - self.registry.definitions() - } - - fn provider_options(&self) -> Option { - 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, - } - } - - fn tool_call_response(tool_name: &str, tool_call_id: &str, args: serde_json::Value) -> Response { - Response { - id: format!("resp_{tool_call_id}"), - model: "mock-model".into(), - provider: "mock".into(), - message: Message { - role: unified_llm::types::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, - } - } - - fn make_echo_tool() -> RegisteredTool { - 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}")) - }) - }), - } - } - - fn make_error_tool() -> RegisteredTool { - 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()) }) - }), - } - } - - async fn make_client(provider: Arc) -> 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) -> 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::new()); - Session::new(client, profile, env, SessionConfig::default()) - } - - async fn make_session_with_tools( - responses: Vec, - 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(MemoryExecutionEnvironment::new()); - Session::new(client, profile, env, SessionConfig::default()) - } - - async fn make_session_with_config( - responses: Vec, - 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(MemoryExecutionEnvironment::new()); - Session::new(client, profile, env, config) - } - - async fn make_session_with_tools_and_config( - responses: Vec, - 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(MemoryExecutionEnvironment::new()); - Session::new(client, profile, env, config) - } + use unified_llm::types::{Response, ToolDefinition}; // --- Tests --- @@ -1232,12 +781,12 @@ mod tests { // First input: adds User + Assistant = 2 turns session.process_input("one").await.unwrap(); - assert_eq!(session.history().count_turns(), 2); + assert_eq!(session.history().turns().len(), 2); // Second input: adds User (now 3 turns), then max_turns check triggers session.process_input("two").await.unwrap(); // Should have 3 turns total (User + Asst + User), max_turns hit before LLM call - assert_eq!(session.history().count_turns(), 3); + assert_eq!(session.history().turns().len(), 3); } #[tokio::test] @@ -1319,8 +868,12 @@ mod tests { } assert_eq!(tool_end_events.len(), 1); - let output = tool_end_events[0].data.get("output").unwrap(); - assert_eq!(output, &serde_json::json!("echo: hello world")); + match &tool_end_events[0].data { + EventData::ToolCallEnd { output, .. } => { + assert_eq!(output, &serde_json::json!("echo: hello world")); + } + _ => panic!("Expected ToolCallEnd event data"), + } } #[tokio::test] @@ -1475,7 +1028,7 @@ mod tests { 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(MemoryExecutionEnvironment::new()); + let env = Arc::new(MockExecutionEnvironment::default()); let config = SessionConfig { enable_loop_detection: false, ..Default::default() @@ -1510,7 +1063,7 @@ mod tests { }); let client = make_client(error_provider).await; let profile = Arc::new(TestProfile::new()); - let env = Arc::new(MemoryExecutionEnvironment::new()); + let env = Arc::new(MockExecutionEnvironment::default()); let mut session = Session::new(client, profile, env, SessionConfig::default()); let result = session.process_input("Hello").await; @@ -1573,116 +1126,6 @@ mod tests { ); } - // --- Parallel execution support --- - - struct ParallelTestProfile { - registry: ToolRegistry, - context_window: usize, - } - - impl ParallelTestProfile { - fn with_tools(registry: ToolRegistry) -> Self { - Self { - registry, - context_window: 200_000, - } - } - - 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: &crate::profiles::EnvContext, - _project_docs: &[String], - _user_instructions: Option<&str>, - ) -> String { - "You are a test assistant.".into() - } - - fn tools(&self) -> Vec { - self.registry.definitions() - } - - fn provider_options(&self) -> Option { - None - } - - fn supports_reasoning(&self) -> bool { - false - } - - fn supports_streaming(&self) -> bool { - false - } - - fn supports_parallel_tool_calls(&self) -> bool { - true - } - - fn context_window_size(&self) -> usize { - self.context_window - } - fn knowledge_cutoff(&self) -> &str { - "May 2025" - } - } - - fn multi_tool_call_response(calls: Vec<(&str, &str, serde_json::Value)>) -> Response { - 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: unified_llm::types::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, - } - } - #[tokio::test] async fn parallel_tool_execution_all_results_returned() { let mut registry = ToolRegistry::new(); @@ -1700,7 +1143,7 @@ mod tests { let provider = Arc::new(MockLlmProvider::new(responses)); let client = make_client(provider).await; let profile = Arc::new(ParallelTestProfile::with_tools(registry)); - let env = Arc::new(MemoryExecutionEnvironment::new()); + let env = Arc::new(MockExecutionEnvironment::default()); let mut session = Session::new(client, profile, env, SessionConfig::default()); let mut rx = session.subscribe(); @@ -1753,7 +1196,7 @@ mod tests { let profile = Arc::new(ParallelTestProfile::with_tools_and_context_window( registry, 100, )); - let env = Arc::new(MemoryExecutionEnvironment::new()); + let env = Arc::new(MockExecutionEnvironment::default()); let mut session = Session::new(client, profile, env, SessionConfig::default()); let mut rx = session.subscribe(); @@ -1763,14 +1206,15 @@ mod tests { while let Ok(event) = rx.try_recv() { if event.kind == EventKind::ContextWindowWarning { found_warning = true; - // Verify event data - assert!(event.data.contains_key("estimated_tokens")); - assert!(event.data.contains_key("context_window_size")); - assert!(event.data.contains_key("usage_percent")); - assert_eq!( - event.data["context_window_size"], - serde_json::json!(100) - ); + match &event.data { + EventData::ContextWarning { + context_window_size, + .. + } => { + assert_eq!(*context_window_size, 100); + } + _ => panic!("Expected ContextWarning event data"), + } } } assert!(found_warning); @@ -1815,7 +1259,7 @@ mod tests { let provider = Arc::new(CapturingLlmProvider::new(captured_effort.clone())); let client = make_client(provider).await; let profile = Arc::new(TestProfile::new()); - let env = Arc::new(MemoryExecutionEnvironment::new()); + let env = Arc::new(MockExecutionEnvironment::default()); let mut session = Session::new(client, profile, env, SessionConfig::default()); // Default reasoning_effort is None @@ -1837,7 +1281,7 @@ mod tests { let profile = Arc::new(ParallelTestProfile::with_tools_and_context_window( registry, 200_000, )); - let env = Arc::new(MemoryExecutionEnvironment::new()); + let env = Arc::new(MockExecutionEnvironment::default()); let mut session = Session::new(client, profile, env, SessionConfig::default()); let mut rx = session.subscribe(); @@ -1992,7 +1436,7 @@ mod tests { }); let client = make_client(provider).await; let profile = Arc::new(TestProfile::new()); - let env = Arc::new(MemoryExecutionEnvironment::new()); + let env = Arc::new(MockExecutionEnvironment::default()); let config = SessionConfig { user_instructions: Some("Always use TDD".into()), ..Default::default() diff --git a/crates/coding-agent-loop/src/subagent.rs b/crates/coding-agent-loop/src/subagent.rs index 8d06fe1ef..a61ca7921 100644 --- a/crates/coding-agent-loop/src/subagent.rs +++ b/crates/coding-agent-loop/src/subagent.rs @@ -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, - call_index: AtomicUsize, - } - - impl MockLlmProvider { - fn new(responses: Vec) -> 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 { - 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 { - 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, _limit: Option) -> Result { - Ok(String::new()) - } - async fn write_file(&self, _path: &str, _content: &str) -> Result<(), String> { - Ok(()) - } - async fn file_exists(&self, _path: &str) -> Result { - Ok(false) - } - async fn list_directory(&self, _path: &str, _depth: Option) -> Result, String> { - Ok(vec![]) - } - async fn exec_command( - &self, - _command: &str, - _timeout_ms: u64, - _working_dir: Option<&str>, - _env_vars: Option<&std::collections::HashMap>, - ) -> Result { - 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, String> { - Ok(vec![]) - } - async fn glob(&self, _pattern: &str, _path: Option<&str>) -> Result, 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 { - self.registry.definitions() - } - fn provider_options(&self) -> Option { - 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) -> 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) -> 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 --- diff --git a/crates/coding-agent-loop/src/test_support.rs b/crates/coding-agent-loop/src/test_support.rs new file mode 100644 index 000000000..95678f22e --- /dev/null +++ b/crates/coding-agent-loop/src/test_support.rs @@ -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, + pub exec_result: ExecResult, + pub grep_results: Vec, + pub glob_results: Vec, + 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, + _limit: Option, + ) -> Result { + 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 { + Ok(self.files.contains_key(path)) + } + + async fn list_directory( + &self, + _path: &str, + _depth: Option, + ) -> Result, String> { + Ok(vec![]) + } + + async fn exec_command( + &self, + _command: &str, + _timeout_ms: u64, + _working_dir: Option<&str>, + _env_vars: Option<&std::collections::HashMap>, + ) -> Result { + Ok(self.exec_result.clone()) + } + + async fn grep( + &self, + _pattern: &str, + _path: &str, + _options: &GrepOptions, + ) -> Result, String> { + Ok(self.grep_results.clone()) + } + + async fn glob(&self, _pattern: &str, _path: Option<&str>) -> Result, 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, + pub call_index: AtomicUsize, +} + +impl MockLlmProvider { + pub fn new(responses: Vec) -> 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 { + 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 { + 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) -> 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) -> 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, + 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, + 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, + 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 { + Err(self.error.clone()) + } + + async fn stream(&self, _request: &Request) -> Result { + 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, + } +} diff --git a/crates/coding-agent-loop/src/tool_registry.rs b/crates/coding-agent-loop/src/tool_registry.rs index 84eed4b5c..c62d28da6 100644 --- a/crates/coding-agent-loop/src/tool_registry.rs +++ b/crates/coding-agent-loop/src/tool_registry.rs @@ -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, _: Option) -> Result { - Ok(String::new()) - } - async fn write_file(&self, _: &str, _: &str) -> Result<(), String> { - Ok(()) - } - async fn file_exists(&self, _: &str) -> Result { - Ok(false) - } - async fn list_directory(&self, _: &str, _: Option) -> Result, String> { - Ok(vec![]) - } - async fn exec_command( - &self, - _: &str, - _: u64, - _: Option<&str>, - _: Option<&std::collections::HashMap>, - ) -> Result { - 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, String> { - Ok(vec![]) - } - async fn glob(&self, _: &str, _: Option<&str>) -> Result, 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 = Arc::new(DummyEnv); + let env: Arc = Arc::new(MockExecutionEnvironment::default()); let result = (tool.executor)(serde_json::json!({}), env).await; assert_eq!(result.unwrap(), "ok"); } diff --git a/crates/coding-agent-loop/src/tools.rs b/crates/coding-agent-loop/src/tools.rs index b076fbc24..405a7c65c 100644 --- a/crates/coding-agent-loop/src/tools.rs +++ b/crates/coding-agent-loop/src/tools.rs @@ -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 { 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 { 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 { 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, _limit: Option) -> Result { - Ok(String::new()) - } - async fn write_file(&self, _: &str, _: &str) -> Result<(), String> { - Ok(()) - } - async fn file_exists(&self, _: &str) -> Result { - Ok(false) - } - async fn list_directory(&self, _: &str, _depth: Option) -> Result, String> { - Ok(vec![]) - } - async fn exec_command(&self, _: &str, _: u64, _: Option<&str>, _: Option<&std::collections::HashMap>) -> Result { - Ok(self.result.clone()) - } - async fn grep(&self, _: &str, _: &str, _: &GrepOptions) -> Result, String> { - Ok(vec![]) - } - async fn glob(&self, _: &str, _path: Option<&str>) -> Result, 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>, @@ -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 { Ok(false) } @@ -672,105 +643,6 @@ mod tests { } } - struct GrepEnv { - results: Vec, - } - - #[async_trait] - impl ExecutionEnvironment for GrepEnv { - async fn read_file(&self, _: &str, _offset: Option, _limit: Option) -> Result { - Ok(String::new()) - } - async fn write_file(&self, _: &str, _: &str) -> Result<(), String> { - Ok(()) - } - async fn file_exists(&self, _: &str) -> Result { - Ok(false) - } - async fn list_directory(&self, _: &str, _depth: Option) -> Result, String> { - Ok(vec![]) - } - async fn exec_command(&self, _: &str, _: u64, _: Option<&str>, _: Option<&std::collections::HashMap>) -> Result { - 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, String> { - Ok(self.results.clone()) - } - async fn glob(&self, _: &str, _path: Option<&str>) -> Result, 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, - } - - #[async_trait] - impl ExecutionEnvironment for GlobEnv { - async fn read_file(&self, _: &str, _offset: Option, _limit: Option) -> Result { - Ok(String::new()) - } - async fn write_file(&self, _: &str, _: &str) -> Result<(), String> { - Ok(()) - } - async fn file_exists(&self, _: &str) -> Result { - Ok(false) - } - async fn list_directory(&self, _: &str, _depth: Option) -> Result, String> { - Ok(vec![]) - } - async fn exec_command(&self, _: &str, _: u64, _: Option<&str>, _: Option<&std::collections::HashMap>) -> Result { - 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, String> { - Ok(vec![]) - } - async fn glob(&self, _: &str, _path: Option<&str>) -> Result, 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 = Arc::new(ShellEnv { - result: ExecResult { + let env: Arc = 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 = Arc::new(ShellEnv { - result: ExecResult { + let env: Arc = 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 = Arc::new(ShellEnv { - result: ExecResult { + let env: Arc = 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 = Arc::new(GrepEnv { - results: vec!["src/main.rs:10:fn main()".into(), "src/lib.rs:5:pub fn".into()], + let env: Arc = 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 = Arc::new(GlobEnv { - results: vec!["src/main.rs".into(), "src/lib.rs".into()], + let env: Arc = 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(); diff --git a/crates/coding-agent-loop/src/truncation.rs b/crates/coding-agent-loop/src/truncation.rs index 3114b3f05..54efff461 100644 --- a/crates/coding-agent-loop/src/truncation.rs +++ b/crates/coding-agent-loop/src/truncation.rs @@ -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 { + 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 { + 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] diff --git a/crates/coding-agent-loop/src/types.rs b/crates/coding-agent-loop/src/types.rs index 3e6c7a149..7a9460869 100644 --- a/crates/coding-agent-loop/src/types.rs +++ b/crates/coding-agent-loop/src/types.rs @@ -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, + 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)); } } diff --git a/docs/agent/reviews/coding-agent-loop-simplification.md b/docs/agent/reviews/coding-agent-loop-simplification.md new file mode 100644 index 000000000..0a21b06f5 --- /dev/null +++ b/docs/agent/reviews/coding-agent-loop-simplification.md @@ -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, + pub exec_result: Option, + pub grep_results: Vec, + pub glob_results: Vec, + // ... +} +``` + +**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; + fn provider_options(&self) -> Option; + 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 { 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>, + 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>, + 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` Instead of Typed Variants + +**What:** Every event emitted throughout the codebase constructs a `HashMap` 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> = 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)