diff --git a/crates/goose-acp/tests/common_tests/mod.rs b/crates/goose-acp/tests/common_tests/mod.rs index 8b62e9550b..58dc961ce2 100644 --- a/crates/goose-acp/tests/common_tests/mod.rs +++ b/crates/goose-acp/tests/common_tests/mod.rs @@ -5,17 +5,19 @@ #[path = "../fixtures/mod.rs"] pub mod fixtures; use fixtures::{ - Connection, FsFixture, OpenAiFixture, PermissionDecision, Session, SessionResult, - TestConnectionConfig, + assert_notifications, Connection, FsFixture, Notification, OpenAiFixture, PermissionDecision, + Session, SessionResult, TerminalCall, TerminalFixture, TestConnectionConfig, }; use fs_err as fs; use goose::config::base::CONFIG_YAML_NAME; use goose::config::GooseMode; use goose::providers::provider_registry::ProviderConstructor; use goose_test_support::{McpFixture, FAKE_CODE, TEST_IMAGE_B64, TEST_MODEL}; -use sacp::schema::{McpServer, McpServerHttp, ModelId, SessionModeId, ToolCallStatus}; +use sacp::schema::{McpServer, McpServerHttp, ModelId, SessionModeId, ToolCallStatus, ToolKind}; use std::sync::Arc; +const SHELL_TEST_CONTENT: &str = "test-shell-content-98765"; + pub async fn run_config_mcp() { let temp_dir = tempfile::tempdir().unwrap(); let expected_session_id = C::expected_session_id(); @@ -54,6 +56,15 @@ pub async fn run_config_mcp() { let output = session.prompt(prompt, PermissionDecision::Cancel).await; assert_eq!(output.text, FAKE_CODE); + assert_notifications( + &session.notifications(), + &[ + Notification::ToolCall, + Notification::ToolCallContent("content".into()), + Notification::ToolCallStatus(ToolCallStatus::Completed), + Notification::AgentMessage, + ], + ); expected_session_id.assert_matches(&session.session_id().0); } @@ -94,6 +105,15 @@ pub async fn run_fs_read_text_file_true() { let output = session.prompt(prompt, PermissionDecision::Cancel).await; assert_eq!(output.text, "test-read-content-12345"); + assert_notifications( + &session.notifications(), + &[ + Notification::ToolCall, + Notification::ToolCallKind(ToolKind::Read), + Notification::ToolCallStatus(ToolCallStatus::Completed), + Notification::AgentMessage, + ], + ); fs.assert_called(); expected_session_id.assert_matches(&session.session_id().0); } @@ -133,6 +153,15 @@ pub async fn run_fs_write_text_file_false() { fs::read_to_string("/tmp/test_acp_write.txt").unwrap(), "test-write-content-67890" ); + assert_notifications( + &session.notifications(), + &[ + Notification::ToolCall, + Notification::ToolCallContent("content".into()), + Notification::ToolCallStatus(ToolCallStatus::Completed), + Notification::AgentMessage, + ], + ); expected_session_id.assert_matches(&session.session_id().0); } @@ -169,6 +198,16 @@ pub async fn run_fs_write_text_file_true() { let output = session.prompt(prompt, PermissionDecision::AllowOnce).await; assert!(!output.text.is_empty()); + assert_notifications( + &session.notifications(), + &[ + Notification::ToolCall, + Notification::ToolCallKind(ToolKind::Edit), + Notification::ToolCallContent("diff".into()), + Notification::ToolCallStatus(ToolCallStatus::Completed), + Notification::AgentMessage, + ], + ); fs.assert_called(); expected_session_id.assert_matches(&session.session_id().0); } @@ -248,6 +287,15 @@ pub async fn run_load_mode() { expected_session_id.set(&loaded.session_id().0); let output = loaded.prompt(prompt, PermissionDecision::Cancel).await; assert_eq!(output.tool_status.unwrap(), ToolCallStatus::Failed); + assert_notifications( + &loaded.notifications(), + &[ + Notification::ToolCall, + Notification::ToolCallContent("content".into()), + Notification::ToolCallStatus(ToolCallStatus::Failed), + Notification::AgentMessage, + ], + ); } pub async fn run_load_model() { @@ -277,6 +325,64 @@ pub async fn run_load_model() { assert_eq!(&*models.unwrap().current_model_id.0, "o4-mini"); } +pub async fn run_load_session_mcp() { + let expected_session_id = C::expected_session_id(); + let prompt = "Use the get_code tool and output only its result."; + let mcp = McpFixture::new(expected_session_id.clone()).await; + let mcp_url = mcp.url.clone(); + + // Two rounds of tool call + tool result: one for new session, one for loaded session. + let openai = OpenAiFixture::new( + vec![ + ( + prompt.to_string(), + include_str!("../test_data/openai_tool_call.txt"), + ), + ( + format!(r#""content":"{FAKE_CODE}""#), + include_str!("../test_data/openai_tool_result.txt"), + ), + ( + prompt.to_string(), + include_str!("../test_data/openai_tool_call.txt"), + ), + ( + format!(r#""content":"{FAKE_CODE}""#), + include_str!("../test_data/openai_tool_result.txt"), + ), + ], + expected_session_id.clone(), + ) + .await; + + let mcp_servers = vec![McpServer::Http(McpServerHttp::new("mcp-fixture", &mcp_url))]; + + let config = TestConnectionConfig { + mcp_servers: mcp_servers.clone(), + ..Default::default() + }; + let mut conn = C::new(config, openai).await; + let SessionResult { mut session, .. } = conn.new_session().await; + expected_session_id.set(&session.session_id().0); + + // First prompt: tool should work in the new session. + let output = session.prompt(prompt, PermissionDecision::Cancel).await; + assert_eq!(output.text, FAKE_CODE, "tool call failed in new session"); + + // Load the same session with MCP servers re-specified. + let session_id = session.session_id().0.to_string(); + let SessionResult { + session: mut loaded_session, + .. + } = conn.load_session(&session_id, mcp_servers).await; + + // Second prompt: tool should work in the loaded session. + let output = loaded_session + .prompt(prompt, PermissionDecision::Cancel) + .await; + assert_eq!(output.text, FAKE_CODE, "tool call failed in loaded session"); +} + pub async fn run_mode_set() { let temp_dir = tempfile::tempdir().unwrap(); let expected_session_id = C::expected_session_id(); @@ -327,12 +433,72 @@ pub async fn run_mode_set() { expected_session_id.set(&session_b.session_id().0); let output = session_b.prompt(prompt, PermissionDecision::Cancel).await; assert_eq!(output.tool_status.unwrap(), ToolCallStatus::Failed); + assert_notifications( + &session_b.notifications(), + &[ + Notification::ToolCall, + Notification::ToolCallContent("content".into()), + Notification::ToolCallStatus(ToolCallStatus::Failed), + Notification::AgentMessage, + ], + ); // Auto mode ignores Cancel — tool succeeds without permission prompt conn.reset_openai(); expected_session_id.set(&session_a.session_id().0); let output = session_a.prompt(prompt, PermissionDecision::Cancel).await; assert_eq!(output.text, FAKE_CODE); + assert_notifications( + &session_a.notifications(), + &[ + Notification::ToolCall, + Notification::ToolCallContent("content".into()), + Notification::ToolCallStatus(ToolCallStatus::Completed), + Notification::AgentMessage, + ], + ); +} + +pub async fn run_mode_set_error( + mode_id: &str, + session_id_override: Option<&str>, + expected: sacp::Error, +) { + let openai = OpenAiFixture::new(vec![], C::expected_session_id()).await; + let mut conn = C::new(TestConnectionConfig::default(), openai).await; + let SessionResult { session, .. } = conn.new_session().await; + + let target_session_id = session_id_override + .map(str::to_string) + .unwrap_or_else(|| session.session_id().0.to_string()); + + let err = conn + .set_mode(&target_session_id, mode_id) + .await + .unwrap_err(); + + let sacp_err = err.downcast::().unwrap(); + assert_eq!(sacp_err, expected); +} + +#[macro_export] +macro_rules! tests_mode_set_error { + ($conn:ty) => { + #[test_case::test_case("not_a_mode", None, sacp::Error::invalid_params().data("Invalid mode: not_a_mode") ; "invalid mode")] + #[test_case::test_case("auto", Some("nonexistent-session-id"), sacp::Error::invalid_params().data("Session not found: nonexistent-session-id") ; "session not found")] + fn test_mode_set_error( + mode_id: &'static str, + session_id: Option<&'static str>, + expected: sacp::Error, + ) { + common_tests::fixtures::run_test(async move { + common_tests::run_mode_set_error::<$conn>( + mode_id, session_id, expected, + ) + .await + }); + } + }; } pub async fn run_model_list() { @@ -482,6 +648,7 @@ pub async fn run_prompt_basic() { .prompt("what is 1+1", PermissionDecision::Cancel) .await; assert_eq!(output.text, "2"); + assert_notifications(&session.notifications(), &[Notification::AgentMessage]); expected_session_id.assert_matches(&session.session_id().0); } @@ -565,6 +732,15 @@ pub async fn run_prompt_image() { ) .await; assert_eq!(output.text, "Hello Goose!\nThis is a test image."); + assert_notifications( + &session.notifications(), + &[ + Notification::ToolCall, + Notification::ToolCallContent("content".into()), + Notification::ToolCallStatus(ToolCallStatus::Completed), + Notification::AgentMessage, + ], + ); expected_session_id.assert_matches(&session.session_id().0); } @@ -592,67 +768,10 @@ pub async fn run_prompt_image_attachment() { ) .await; assert!(output.text.contains("Hello Goose!")); + assert_notifications(&session.notifications(), &[Notification::AgentMessage]); expected_session_id.assert_matches(&session.session_id().0); } -pub async fn run_load_session_mcp() { - let expected_session_id = C::expected_session_id(); - let prompt = "Use the get_code tool and output only its result."; - let mcp = McpFixture::new(expected_session_id.clone()).await; - let mcp_url = mcp.url.clone(); - - // Two rounds of tool call + tool result: one for new session, one for loaded session. - let openai = OpenAiFixture::new( - vec![ - ( - prompt.to_string(), - include_str!("../test_data/openai_tool_call.txt"), - ), - ( - format!(r#""content":"{FAKE_CODE}""#), - include_str!("../test_data/openai_tool_result.txt"), - ), - ( - prompt.to_string(), - include_str!("../test_data/openai_tool_call.txt"), - ), - ( - format!(r#""content":"{FAKE_CODE}""#), - include_str!("../test_data/openai_tool_result.txt"), - ), - ], - expected_session_id.clone(), - ) - .await; - - let mcp_servers = vec![McpServer::Http(McpServerHttp::new("mcp-fixture", &mcp_url))]; - - let config = TestConnectionConfig { - mcp_servers: mcp_servers.clone(), - ..Default::default() - }; - let mut conn = C::new(config, openai).await; - let SessionResult { mut session, .. } = conn.new_session().await; - expected_session_id.set(&session.session_id().0); - - // First prompt: tool should work in the new session. - let output = session.prompt(prompt, PermissionDecision::Cancel).await; - assert_eq!(output.text, FAKE_CODE, "tool call failed in new session"); - - // Load the same session with MCP servers re-specified. - let session_id = session.session_id().0.to_string(); - let SessionResult { - session: mut loaded_session, - .. - } = conn.load_session(&session_id, mcp_servers).await; - - // Second prompt: tool should work in the loaded session. - let output = loaded_session - .prompt(prompt, PermissionDecision::Cancel) - .await; - assert_eq!(output.text, FAKE_CODE, "tool call failed in loaded session"); -} - pub async fn run_prompt_mcp() { let expected_session_id = C::expected_session_id(); let mcp = McpFixture::new(expected_session_id.clone()).await; @@ -686,6 +805,15 @@ pub async fn run_prompt_mcp() { ) .await; assert_eq!(output.text, FAKE_CODE); + assert_notifications( + &session.notifications(), + &[ + Notification::ToolCall, + Notification::ToolCallContent("content".into()), + Notification::ToolCallStatus(ToolCallStatus::Completed), + Notification::AgentMessage, + ], + ); expected_session_id.assert_matches(&session.session_id().0); } @@ -726,44 +854,94 @@ pub async fn run_prompt_skill() { expected_session_id.assert_matches(&session.session_id().0); } -pub async fn run_mode_set_error( - mode_id: &str, - session_id_override: Option<&str>, - expected: sacp::Error, -) { - let openai = OpenAiFixture::new(vec![], C::expected_session_id()).await; - let mut conn = C::new(TestConnectionConfig::default(), openai).await; - let SessionResult { session, .. } = conn.new_session().await; +pub async fn run_shell_terminal_false() { + let expected_session_id = C::expected_session_id(); + let prompt = format!("Run the command echo {SHELL_TEST_CONTENT} and output only its result."); + let openai = OpenAiFixture::new( + vec![ + ( + prompt.clone(), + include_str!("../test_data/openai_shell_tool_call.txt"), + ), + ( + SHELL_TEST_CONTENT.into(), + include_str!("../test_data/openai_shell_tool_result.txt"), + ), + ], + expected_session_id.clone(), + ) + .await; - let target_session_id = session_id_override - .map(str::to_string) - .unwrap_or_else(|| session.session_id().0.to_string()); - - let err = conn - .set_mode(&target_session_id, mode_id) - .await - .unwrap_err(); - - let sacp_err = err.downcast::().unwrap(); - assert_eq!(sacp_err, expected); -} - -#[macro_export] -macro_rules! tests_mode_set_error { - ($conn:ty) => { - #[test_case::test_case("not_a_mode", None, sacp::Error::invalid_params().data("Invalid mode: not_a_mode") ; "invalid mode")] - #[test_case::test_case("auto", Some("nonexistent-session-id"), sacp::Error::invalid_params().data("Session not found: nonexistent-session-id") ; "session not found")] - fn test_mode_set_error( - mode_id: &'static str, - session_id: Option<&'static str>, - expected: sacp::Error, - ) { - common_tests::fixtures::run_test(async move { - common_tests::run_mode_set_error::<$conn>( - mode_id, session_id, expected, - ) - .await - }); - } + let config = TestConnectionConfig { + builtins: vec!["developer".to_string()], + ..Default::default() }; + let mut conn = C::new(config, openai).await; + let SessionResult { mut session, .. } = conn.new_session().await; + expected_session_id.set(&session.session_id().0); + + let output = session.prompt(&prompt, PermissionDecision::AllowOnce).await; + assert!(!output.text.is_empty()); + assert_notifications( + &session.notifications(), + &[ + Notification::ToolCall, + Notification::ToolCallContent("content".into()), + Notification::ToolCallStatus(ToolCallStatus::Completed), + Notification::AgentMessage, + ], + ); + expected_session_id.assert_matches(&session.session_id().0); +} + +pub async fn run_shell_terminal_true() { + let expected_session_id = C::expected_session_id(); + let prompt = format!("Run the command echo {SHELL_TEST_CONTENT} and output only its result."); + let openai = OpenAiFixture::new( + vec![ + ( + prompt.clone(), + include_str!("../test_data/openai_shell_tool_call.txt"), + ), + ( + SHELL_TEST_CONTENT.into(), + include_str!("../test_data/openai_shell_tool_result.txt"), + ), + ], + expected_session_id.clone(), + ) + .await; + + let command = format!("echo {SHELL_TEST_CONTENT}"); + let output_text = format!("{SHELL_TEST_CONTENT}\n"); + let tid = String::from("term-1"); + let terminal = TerminalFixture::new(vec![ + TerminalCall::Create(command.clone(), tid.clone()), + TerminalCall::WaitForExit(tid.clone(), 0), + TerminalCall::Output(tid.clone(), output_text.clone(), 0), + TerminalCall::Release(tid), + ]); + let config = TestConnectionConfig { + builtins: vec!["developer".to_string()], + terminal: Some(terminal.clone()), + ..Default::default() + }; + let mut conn = C::new(config, openai).await; + let SessionResult { mut session, .. } = conn.new_session().await; + expected_session_id.set(&session.session_id().0); + + let output = session.prompt(&prompt, PermissionDecision::AllowOnce).await; + assert_eq!(output.tool_status, Some(ToolCallStatus::Completed)); + assert_notifications( + &session.notifications(), + &[ + Notification::ToolCall, + Notification::ToolCallKind(ToolKind::Execute), + Notification::ToolCallContent("terminal".into()), + Notification::ToolCallStatus(ToolCallStatus::Completed), + Notification::AgentMessage, + ], + ); + terminal.assert_called(); + expected_session_id.assert_matches(&session.session_id().0); } diff --git a/crates/goose-acp/tests/fixtures/mod.rs b/crates/goose-acp/tests/fixtures/mod.rs index 1fe2f74257..0d5c44989b 100644 --- a/crates/goose-acp/tests/fixtures/mod.rs +++ b/crates/goose-acp/tests/fixtures/mod.rs @@ -14,8 +14,11 @@ use goose::session_context::SESSION_ID_HEADER; use goose_acp::server::{serve, GooseAcpAgent}; use goose_test_support::{ExpectedSessionId, TEST_MODEL}; use sacp::schema::{ - AuthMethod, McpServer, ReadTextFileRequest, ReadTextFileResponse, SessionModeState, - SessionModelState, ToolCallStatus, WriteTextFileRequest, WriteTextFileResponse, + AuthMethod, CreateTerminalResponse, KillTerminalCommandResponse, McpServer, + ReadTextFileRequest, ReadTextFileResponse, ReleaseTerminalResponse, SessionModeState, + SessionModelState, SessionUpdate, TerminalExitStatus, TerminalId, TerminalOutputResponse, + ToolCallContent, ToolCallStatus, ToolKind, WaitForTerminalExitResponse, WriteTextFileRequest, + WriteTextFileResponse, }; use std::collections::VecDeque; use std::future::Future; @@ -200,6 +203,74 @@ pub struct TestOutput { pub tool_status: Option, } +#[derive(Debug, PartialEq)] +pub enum Notification { + UserMessage, + AgentMessage, + AgentThought, + ToolCall, + ToolCallKind(ToolKind), + ToolCallContent(String), + ToolCallStatus(ToolCallStatus), + Plan, + AvailableCommands, + CurrentMode, + ConfigOption, +} + +pub fn to_notifications(updates: &[SessionUpdate]) -> Vec { + let mut out = Vec::new(); + for u in updates { + match u { + SessionUpdate::UserMessageChunk(_) => { + if out.last() != Some(&Notification::UserMessage) { + out.push(Notification::UserMessage); + } + } + SessionUpdate::AgentMessageChunk(_) => { + if out.last() != Some(&Notification::AgentMessage) { + out.push(Notification::AgentMessage); + } + } + SessionUpdate::AgentThoughtChunk(_) => { + if out.last() != Some(&Notification::AgentThought) { + out.push(Notification::AgentThought); + } + } + SessionUpdate::ToolCall(_) => out.push(Notification::ToolCall), + SessionUpdate::ToolCallUpdate(upd) => { + if let Some(kind) = upd.fields.kind { + out.push(Notification::ToolCallKind(kind)); + } + if let Some(ref content) = upd.fields.content { + for c in content { + let tag = match c { + ToolCallContent::Content(_) => "content", + ToolCallContent::Diff(_) => "diff", + ToolCallContent::Terminal(_) => "terminal", + _ => "unknown", + }; + out.push(Notification::ToolCallContent(tag.into())); + } + } + if let Some(status) = upd.fields.status { + out.push(Notification::ToolCallStatus(status)); + } + } + SessionUpdate::Plan(_) => out.push(Notification::Plan), + SessionUpdate::AvailableCommandsUpdate(_) => out.push(Notification::AvailableCommands), + SessionUpdate::CurrentModeUpdate(_) => out.push(Notification::CurrentMode), + SessionUpdate::ConfigOptionUpdate(_) => out.push(Notification::ConfigOption), + _ => {} + } + } + out +} + +pub fn assert_notifications(actual: &[Notification], expected: &[Notification]) { + assert_eq!(actual, expected); +} + type ReadTextFileHandler = Arc Result + Send + Sync>; type WriteTextFileHandler = @@ -266,6 +337,124 @@ impl FsFixture { } } +/// Expected terminal calls. Each variant carries (expected_input, return_value) data, +/// like OpenAiFixture's (pattern, response) pairs. +#[derive(Debug, Clone)] +#[allow(dead_code)] +pub enum TerminalCall { + Create(String, String), // (command, terminal_id) + WaitForExit(String, u32), // (terminal_id, exit_code) + Output(String, String, u32), // (terminal_id, text, exit_code) + Release(String), // terminal_id + Kill(String), // terminal_id +} + +impl TerminalCall { + fn name(&self) -> &'static str { + match self { + Self::Create(..) => "create", + Self::WaitForExit(..) => "wait_for_exit", + Self::Output(..) => "output", + Self::Release(_) => "release", + Self::Kill(_) => "kill", + } + } +} + +pub struct TerminalFixture { + queue: Arc>>, + errors: Arc>>, +} + +impl TerminalFixture { + pub fn new(calls: Vec) -> Arc { + Arc::new(Self { + queue: Arc::new(Mutex::new(VecDeque::from(calls))), + errors: Arc::new(Mutex::new(Vec::new())), + }) + } + + fn pop(&self, expected: &str) -> Option { + let Some(call) = self.queue.lock().unwrap().pop_front() else { + self.record_error(format!("unexpected {expected}: queue empty")); + return None; + }; + if call.name() != expected { + self.record_error(format!("expected {expected}, got {}", call.name())); + return None; + } + Some(call) + } + + fn record_error(&self, msg: String) { + self.errors.lock().unwrap().push(msg); + } + + fn validate_terminal_id(&self, method: &str, expected: &str, actual: &TerminalId) { + if expected != actual.0.as_ref() { + self.record_error(format!( + "{method}: expected terminal_id {expected}, got {actual}" + )); + } + } + + pub fn on_create(&self, command: &str) -> CreateTerminalResponse { + if let Some(TerminalCall::Create(expect_command, terminal_id)) = self.pop("create") { + if command != expect_command { + self.record_error(format!( + "create: expected command {expect_command}, got {command}" + )); + } + CreateTerminalResponse::new(TerminalId::new(terminal_id)) + } else { + CreateTerminalResponse::new(TerminalId::new("error")) + } + } + + pub fn on_wait_for_exit(&self, terminal_id: &TerminalId) -> WaitForTerminalExitResponse { + if let Some(TerminalCall::WaitForExit(expected_id, exit_code)) = self.pop("wait_for_exit") { + self.validate_terminal_id("wait_for_exit", &expected_id, terminal_id); + WaitForTerminalExitResponse::new(TerminalExitStatus::new().exit_code(exit_code)) + } else { + WaitForTerminalExitResponse::new(TerminalExitStatus::new().exit_code(1)) + } + } + + pub fn on_output(&self, terminal_id: &TerminalId) -> TerminalOutputResponse { + if let Some(TerminalCall::Output(expected_id, text, exit_code)) = self.pop("output") { + self.validate_terminal_id("output", &expected_id, terminal_id); + TerminalOutputResponse::new(text, false) + .exit_status(TerminalExitStatus::new().exit_code(exit_code)) + } else { + TerminalOutputResponse::new("", false) + } + } + + pub fn on_release(&self, terminal_id: &TerminalId) -> ReleaseTerminalResponse { + if let Some(TerminalCall::Release(expected_id)) = self.pop("release") { + self.validate_terminal_id("release", &expected_id, terminal_id); + } + ReleaseTerminalResponse::new() + } + + pub fn on_kill(&self, terminal_id: &TerminalId) -> KillTerminalCommandResponse { + if let Some(TerminalCall::Kill(expected_id)) = self.pop("kill") { + self.validate_terminal_id("kill", &expected_id, terminal_id); + } + KillTerminalCommandResponse::new() + } + + pub fn assert_called(&self) { + let errors = self.errors.lock().unwrap(); + assert!(errors.is_empty(), "terminal fixture errors: {errors:?}"); + let queue = self.queue.lock().unwrap(); + assert!( + queue.is_empty(), + "terminal fixture has unconsumed calls: {queue:?}" + ); + } +} + pub struct SessionResult { pub session: S, pub models: Option, @@ -281,6 +470,7 @@ pub struct TestConnectionConfig { pub provider_factory: Option, pub read_text_file: Option, pub write_text_file: Option, + pub terminal: Option>, } impl Default for TestConnectionConfig { @@ -294,6 +484,7 @@ impl Default for TestConnectionConfig { provider_factory: None, read_text_file: None, write_text_file: None, + terminal: None, } } } @@ -320,6 +511,7 @@ pub trait Connection: Sized { #[async_trait] pub trait Session { fn session_id(&self) -> &sacp::schema::SessionId; + fn notifications(&self) -> Vec; async fn prompt(&mut self, text: &str, decision: PermissionDecision) -> TestOutput; async fn prompt_with_image( &mut self, diff --git a/crates/goose-acp/tests/fixtures/provider.rs b/crates/goose-acp/tests/fixtures/provider.rs index a255597c73..e5de5896f6 100644 --- a/crates/goose-acp/tests/fixtures/provider.rs +++ b/crates/goose-acp/tests/fixtures/provider.rs @@ -14,17 +14,20 @@ use goose::permission::{Permission, PermissionConfirmation}; use goose::providers::base::Provider; use goose::providers::errors::ProviderError; use goose_test_support::{ExpectedSessionId, IgnoreSessionId, TEST_MODEL}; -use sacp::schema::{AuthMethod, McpServer, ToolCallStatus}; +use sacp::schema::{AuthMethod, McpServer, SessionUpdate, ToolCallStatus}; use std::str::FromStr; use std::sync::Arc; use tokio::sync::Mutex; +pub type NotificationSink = Arc>>; + #[allow(dead_code)] pub struct ClientToProviderConnection { provider: Arc>, permission_manager: Arc, auth_methods: Vec, session_counter: usize, + notification_sink: NotificationSink, _openai: OpenAiFixture, _temp_dir: Option, _cwd: Option, @@ -34,6 +37,7 @@ pub struct ClientToProviderConnection { pub struct ClientToProviderSession { provider: Arc>, session_id: sacp::schema::SessionId, + notification_sink: NotificationSink, } impl ClientToProviderSession { @@ -41,6 +45,7 @@ impl ClientToProviderSession { async fn send_message(&mut self, message: Message, decision: PermissionDecision) -> TestOutput { let session_id = self.session_id.0.clone(); let provider = self.provider.lock().await; + self.notification_sink.lock().unwrap().clear(); let model_config = provider.get_model_config(); let mut stream = provider .stream(&model_config, &session_id, "", &[message], &[]) @@ -135,6 +140,8 @@ impl Connection for ClientToProviderConnection { .map(|td| td.path().to_path_buf()) .unwrap_or(data_root); + let notification_sink: NotificationSink = Arc::new(std::sync::Mutex::new(Vec::new())); + let sink_clone = notification_sink.clone(); let provider_config = AcpProviderConfig { command: "unused".into(), args: vec![], @@ -144,6 +151,9 @@ impl Connection for ClientToProviderConnection { mcp_servers, session_mode_id: None, permission_mapping: PermissionMapping::default(), + notification_callback: Some(Arc::new(move |n| { + sink_clone.lock().unwrap().push(n.update.clone()); + })), }; let provider = AcpProvider::connect_with_transport( @@ -164,6 +174,7 @@ impl Connection for ClientToProviderConnection { permission_manager, auth_methods, session_counter: 0, + notification_sink, _openai: openai, _temp_dir: temp_dir, _cwd: config.cwd, @@ -186,6 +197,7 @@ impl Connection for ClientToProviderConnection { let session = ClientToProviderSession { provider: Arc::clone(&self.provider), session_id: sacp::schema::SessionId::new(goose_id), + notification_sink: self.notification_sink.clone(), }; SessionResult { session, @@ -249,6 +261,11 @@ impl Session for ClientToProviderSession { &self.session_id } + fn notifications(&self) -> Vec { + let updates = self.notification_sink.lock().unwrap(); + super::to_notifications(&updates) + } + async fn prompt(&mut self, prompt: &str, decision: PermissionDecision) -> TestOutput { self.send_message(Message::user().with_text(prompt), decision) .await diff --git a/crates/goose-acp/tests/fixtures/server.rs b/crates/goose-acp/tests/fixtures/server.rs index edd72d2ded..623d0f61c1 100644 --- a/crates/goose-acp/tests/fixtures/server.rs +++ b/crates/goose-acp/tests/fixtures/server.rs @@ -6,10 +6,12 @@ use async_trait::async_trait; use goose::config::PermissionManager; use goose_test_support::{EnforceSessionId, ExpectedSessionId}; use sacp::schema::{ - AuthMethod, ClientCapabilities, ContentBlock, FileSystemCapability, ImageContent, - InitializeRequest, LoadSessionRequest, McpServer, NewSessionRequest, PromptRequest, - ProtocolVersion, ReadTextFileRequest, RequestPermissionRequest, SessionNotification, - SessionUpdate, StopReason, TextContent, ToolCallStatus, WriteTextFileRequest, + AuthMethod, ClientCapabilities, ContentBlock, CreateTerminalRequest, FileSystemCapability, + ImageContent, InitializeRequest, KillTerminalCommandRequest, LoadSessionRequest, McpServer, + NewSessionRequest, PromptRequest, ProtocolVersion, ReadTextFileRequest, ReleaseTerminalRequest, + RequestPermissionRequest, SessionNotification, SessionUpdate, StopReason, + TerminalOutputRequest, TextContent, ToolCallStatus, WaitForTerminalExitRequest, + WriteTextFileRequest, }; use sacp::{ClientToAgent, JrConnectionCx}; use std::sync::{Arc, Mutex}; @@ -36,6 +38,7 @@ pub struct ClientToAgentSession { updates: Arc>>, permission: Arc>, notify: Arc, + _work_dir: tempfile::TempDir, } impl ClientToAgentSession { @@ -125,6 +128,7 @@ impl Connection for ClientToAgentConnection { let permission_clone = permission.clone(); let read_handler = config.read_text_file; let write_handler = config.write_text_file; + let terminal = config.terminal; let cx_holder: Arc>>> = Arc::new(Mutex::new(None)); @@ -185,6 +189,68 @@ impl Connection for ClientToAgentConnection { }, sacp::on_receive_request!(), ) + .on_receive_request( + { + let t = terminal.clone(); + async move |req: CreateTerminalRequest, request_cx, _cx| match t { + Some(ref f) => request_cx.respond(f.on_create(&req.command)), + None => { + request_cx.respond_with_error(sacp::Error::method_not_found()) + } + } + }, + sacp::on_receive_request!(), + ) + .on_receive_request( + { + let t = terminal.clone(); + async move |req: WaitForTerminalExitRequest, request_cx, _cx| match t { + Some(ref f) => { + request_cx.respond(f.on_wait_for_exit(&req.terminal_id)) + } + None => { + request_cx.respond_with_error(sacp::Error::method_not_found()) + } + } + }, + sacp::on_receive_request!(), + ) + .on_receive_request( + { + let t = terminal.clone(); + async move |req: TerminalOutputRequest, request_cx, _cx| match t { + Some(ref f) => request_cx.respond(f.on_output(&req.terminal_id)), + None => { + request_cx.respond_with_error(sacp::Error::method_not_found()) + } + } + }, + sacp::on_receive_request!(), + ) + .on_receive_request( + { + let t = terminal.clone(); + async move |req: ReleaseTerminalRequest, request_cx, _cx| match t { + Some(ref f) => request_cx.respond(f.on_release(&req.terminal_id)), + None => { + request_cx.respond_with_error(sacp::Error::method_not_found()) + } + } + }, + sacp::on_receive_request!(), + ) + .on_receive_request( + { + let t = terminal.clone(); + async move |req: KillTerminalCommandRequest, request_cx, _cx| match t { + Some(ref f) => request_cx.respond(f.on_kill(&req.terminal_id)), + None => { + request_cx.respond_with_error(sacp::Error::method_not_found()) + } + } + }, + sacp::on_receive_request!(), + ) .connect_to(transport) .unwrap() .run_until({ @@ -194,7 +260,11 @@ impl Connection for ClientToAgentConnection { let resp = cx .send_request( InitializeRequest::new(ProtocolVersion::LATEST) - .client_capabilities(ClientCapabilities::new().fs(fs_cap)), + .client_capabilities( + ClientCapabilities::new() + .fs(fs_cap) + .terminal(terminal.is_some()), + ), ) .block_task() .await @@ -252,6 +322,7 @@ impl Connection for ClientToAgentConnection { updates: self.updates.clone(), permission: self.permission.clone(), notify: self.notify.clone(), + _work_dir: work_dir, }; SessionResult { session, @@ -283,6 +354,7 @@ impl Connection for ClientToAgentConnection { updates: self.updates.clone(), permission: self.permission.clone(), notify: self.notify.clone(), + _work_dir: work_dir, }; SessionResult { session, @@ -336,6 +408,17 @@ impl Session for ClientToAgentSession { &self.session_id } + fn notifications(&self) -> Vec { + let updates: Vec<_> = self + .updates + .lock() + .unwrap() + .iter() + .map(|n| n.update.clone()) + .collect(); + super::to_notifications(&updates) + } + async fn prompt(&mut self, text: &str, decision: PermissionDecision) -> TestOutput { self.send_prompt(vec![ContentBlock::Text(TextContent::new(text))], decision) .await diff --git a/crates/goose-acp/tests/provider_test.rs b/crates/goose-acp/tests/provider_test.rs index 316f862f70..ff0a81395f 100644 --- a/crates/goose-acp/tests/provider_test.rs +++ b/crates/goose-acp/tests/provider_test.rs @@ -8,7 +8,7 @@ use common_tests::{ run_fs_write_text_file_true, run_initialize_doesnt_hit_provider, run_load_mode, run_load_model, run_load_session_mcp, run_mode_set, run_model_list, run_model_set, run_permission_persistence, run_prompt_basic, run_prompt_codemode, run_prompt_image, run_prompt_image_attachment, - run_prompt_mcp, run_prompt_skill, + run_prompt_mcp, run_prompt_skill, run_shell_terminal_false, run_shell_terminal_true, }; tests_mode_set_error!(ClientToProviderConnection); @@ -107,3 +107,14 @@ fn test_prompt_mcp() { fn test_prompt_skill() { run_test(async { run_prompt_skill::().await }); } + +#[test] +fn test_shell_terminal_false() { + run_test(async { run_shell_terminal_false::().await }); +} + +#[test] +#[ignore = "provider does not handle terminal delegation requests"] +fn test_shell_terminal_true() { + run_test(async { run_shell_terminal_true::().await }); +} diff --git a/crates/goose-acp/tests/server_test.rs b/crates/goose-acp/tests/server_test.rs index 0ac9d168e3..3f84b23473 100644 --- a/crates/goose-acp/tests/server_test.rs +++ b/crates/goose-acp/tests/server_test.rs @@ -6,7 +6,7 @@ use common_tests::{ run_fs_write_text_file_true, run_initialize_doesnt_hit_provider, run_load_mode, run_load_model, run_load_session_mcp, run_mode_set, run_model_list, run_model_set, run_permission_persistence, run_prompt_basic, run_prompt_codemode, run_prompt_image, run_prompt_image_attachment, - run_prompt_mcp, run_prompt_skill, + run_prompt_mcp, run_prompt_skill, run_shell_terminal_false, run_shell_terminal_true, }; tests_mode_set_error!(ClientToAgentConnection); @@ -100,3 +100,13 @@ fn test_prompt_mcp() { fn test_prompt_skill() { run_test(async { run_prompt_skill::().await }); } + +#[test] +fn test_shell_terminal_false() { + run_test(async { run_shell_terminal_false::().await }); +} + +#[test] +fn test_shell_terminal_true() { + run_test(async { run_shell_terminal_true::().await }); +} diff --git a/crates/goose-acp/tests/test_data/openai_shell_tool_call.txt b/crates/goose-acp/tests/test_data/openai_shell_tool_call.txt new file mode 100644 index 0000000000..f677072b23 --- /dev/null +++ b/crates/goose-acp/tests/test_data/openai_shell_tool_call.txt @@ -0,0 +1,30 @@ +data: {"id":"chatcmpl-DJqmdyg4XAGu7vN41PpMz4U8CMQhI","object":"chat.completion.chunk","created":1773623503,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"role":"assistant","content":null,"tool_calls":[{"index":0,"id":"call_JOtrY1Ec2KAB2a7gI6H6CWfs","type":"function","function":{"name":"shell","arguments":""}}],"refusal":null},"finish_reason":null}],"usage":null,"obfuscation":"QvrA4B"} + +data: {"id":"chatcmpl-DJqmdyg4XAGu7vN41PpMz4U8CMQhI","object":"chat.completion.chunk","created":1773623503,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"{\""}}]},"finish_reason":null}],"usage":null,"obfuscation":"fvwZUJVGPB"} + +data: {"id":"chatcmpl-DJqmdyg4XAGu7vN41PpMz4U8CMQhI","object":"chat.completion.chunk","created":1773623503,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"command"}}]},"finish_reason":null}],"usage":null,"obfuscation":"bHHxtU"} + +data: {"id":"chatcmpl-DJqmdyg4XAGu7vN41PpMz4U8CMQhI","object":"chat.completion.chunk","created":1773623503,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"\":\""}}]},"finish_reason":null}],"usage":null,"obfuscation":"GgtVJCU4"} + +data: {"id":"chatcmpl-DJqmdyg4XAGu7vN41PpMz4U8CMQhI","object":"chat.completion.chunk","created":1773623503,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"echo"}}]},"finish_reason":null}],"usage":null,"obfuscation":"DRTSpxLvb"} + +data: {"id":"chatcmpl-DJqmdyg4XAGu7vN41PpMz4U8CMQhI","object":"chat.completion.chunk","created":1773623503,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":" test"}}]},"finish_reason":null}],"usage":null,"obfuscation":"IwvMxMcs"} + +data: {"id":"chatcmpl-DJqmdyg4XAGu7vN41PpMz4U8CMQhI","object":"chat.completion.chunk","created":1773623503,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"-shell"}}]},"finish_reason":null}],"usage":null,"obfuscation":"y6LBASX"} + +data: {"id":"chatcmpl-DJqmdyg4XAGu7vN41PpMz4U8CMQhI","object":"chat.completion.chunk","created":1773623503,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"-content"}}]},"finish_reason":null}],"usage":null,"obfuscation":"4bIL9"} + +data: {"id":"chatcmpl-DJqmdyg4XAGu7vN41PpMz4U8CMQhI","object":"chat.completion.chunk","created":1773623503,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"-"}}]},"finish_reason":null}],"usage":null,"obfuscation":"cOYkwBZ6YQk9"} + +data: {"id":"chatcmpl-DJqmdyg4XAGu7vN41PpMz4U8CMQhI","object":"chat.completion.chunk","created":1773623503,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"987"}}]},"finish_reason":null}],"usage":null,"obfuscation":"2QASfrMS6w"} + +data: {"id":"chatcmpl-DJqmdyg4XAGu7vN41PpMz4U8CMQhI","object":"chat.completion.chunk","created":1773623503,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"65"}}]},"finish_reason":null}],"usage":null,"obfuscation":"IKmTYztxyp2"} + +data: {"id":"chatcmpl-DJqmdyg4XAGu7vN41PpMz4U8CMQhI","object":"chat.completion.chunk","created":1773623503,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"tool_calls":[{"index":0,"function":{"arguments":"\"}"}}]},"finish_reason":null}],"usage":null,"obfuscation":"tVHIYFZOIs"} + +data: {"id":"chatcmpl-DJqmdyg4XAGu7vN41PpMz4U8CMQhI","object":"chat.completion.chunk","created":1773623503,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{},"finish_reason":"tool_calls"}],"usage":null,"obfuscation":"91dJqR0sRAl"} + +data: {"id":"chatcmpl-DJqmdyg4XAGu7vN41PpMz4U8CMQhI","object":"chat.completion.chunk","created":1773623503,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[],"usage":{"prompt_tokens":3392,"completion_tokens":284,"total_tokens":3676,"prompt_tokens_details":{"cached_tokens":0,"audio_tokens":0},"completion_tokens_details":{"reasoning_tokens":256,"audio_tokens":0,"accepted_prediction_tokens":0,"rejected_prediction_tokens":0}},"obfuscation":"aOmtxq2YavJWo1s"} + +data: [DONE] + diff --git a/crates/goose-acp/tests/test_data/openai_shell_tool_result.txt b/crates/goose-acp/tests/test_data/openai_shell_tool_result.txt new file mode 100644 index 0000000000..82e7f2ea8a --- /dev/null +++ b/crates/goose-acp/tests/test_data/openai_shell_tool_result.txt @@ -0,0 +1,21 @@ +data: {"id":"chatcmpl-DJqmhw9icV2gtFchMIfmzQZxB4sAt","object":"chat.completion.chunk","created":1773623507,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"role":"assistant","content":"","refusal":null},"finish_reason":null}],"usage":null,"obfuscation":"k3ql1"} + +data: {"id":"chatcmpl-DJqmhw9icV2gtFchMIfmzQZxB4sAt","object":"chat.completion.chunk","created":1773623507,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"content":"test"},"finish_reason":null}],"usage":null,"obfuscation":"fi2"} + +data: {"id":"chatcmpl-DJqmhw9icV2gtFchMIfmzQZxB4sAt","object":"chat.completion.chunk","created":1773623507,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"content":"-shell"},"finish_reason":null}],"usage":null,"obfuscation":"l"} + +data: {"id":"chatcmpl-DJqmhw9icV2gtFchMIfmzQZxB4sAt","object":"chat.completion.chunk","created":1773623507,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"content":"-content"},"finish_reason":null}],"usage":null,"obfuscation":"lV6xovjxNlG3ZDv"} + +data: {"id":"chatcmpl-DJqmhw9icV2gtFchMIfmzQZxB4sAt","object":"chat.completion.chunk","created":1773623507,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"content":"-"},"finish_reason":null}],"usage":null,"obfuscation":"EzE9fO"} + +data: {"id":"chatcmpl-DJqmhw9icV2gtFchMIfmzQZxB4sAt","object":"chat.completion.chunk","created":1773623507,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"content":"987"},"finish_reason":null}],"usage":null,"obfuscation":"Ji6A"} + +data: {"id":"chatcmpl-DJqmhw9icV2gtFchMIfmzQZxB4sAt","object":"chat.completion.chunk","created":1773623507,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{"content":"65"},"finish_reason":null}],"usage":null,"obfuscation":"lI6rL"} + +data: {"id":"chatcmpl-DJqmhw9icV2gtFchMIfmzQZxB4sAt","object":"chat.completion.chunk","created":1773623507,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[{"index":0,"delta":{},"finish_reason":"stop"}],"usage":null,"obfuscation":"x"} + +data: {"id":"chatcmpl-DJqmhw9icV2gtFchMIfmzQZxB4sAt","object":"chat.completion.chunk","created":1773623507,"model":"gpt-5-nano-2025-08-07","service_tier":"default","system_fingerprint":null,"choices":[],"usage":{"prompt_tokens":3443,"completion_tokens":9,"total_tokens":3452,"prompt_tokens_details":{"cached_tokens":3200,"audio_tokens":0},"completion_tokens_details":{"reasoning_tokens":0,"audio_tokens":0,"accepted_prediction_tokens":0,"rejected_prediction_tokens":0}},"obfuscation":""} + +data: [DONE] + + diff --git a/crates/goose/src/acp/provider.rs b/crates/goose/src/acp/provider.rs index 833b2440b0..1619d9e3fe 100644 --- a/crates/goose/src/acp/provider.rs +++ b/crates/goose/src/acp/provider.rs @@ -28,7 +28,6 @@ use crate::permission::{Permission, PermissionConfirmation}; use crate::providers::base::{MessageStream, PermissionRouting, Provider}; use crate::providers::errors::ProviderError; -#[derive(Clone, Debug)] pub struct AcpProviderConfig { pub command: PathBuf, pub args: Vec, @@ -38,6 +37,7 @@ pub struct AcpProviderConfig { pub mcp_servers: Vec, pub session_mode_id: Option, pub permission_mapping: PermissionMapping, + pub notification_callback: Option>, } enum ClientRequest { @@ -522,12 +522,16 @@ impl AcpClientLoop { goose_mode, prompt_response_tx, } = self; + let notification_callback = config.notification_callback.clone(); ClientToAgent::builder() .on_receive_notification( { let prompt_response_tx = prompt_response_tx.clone(); async move |notification: SessionNotification, _cx| { + if let Some(ref cb) = notification_callback { + cb(notification.clone()); + } // stream() reads goose_mode at call time, so it must // reflect any prior set_mode before the next prompt. if let SessionUpdate::CurrentModeUpdate(update) = ¬ification.update { diff --git a/crates/goose/src/agents/platform_extensions/developer/shell.rs b/crates/goose/src/agents/platform_extensions/developer/shell.rs index ea7a5c626c..998600a575 100644 --- a/crates/goose/src/agents/platform_extensions/developer/shell.rs +++ b/crates/goose/src/agents/platform_extensions/developer/shell.rs @@ -1,7 +1,7 @@ use std::path::PathBuf; use std::process::Stdio; use std::sync::atomic::{AtomicUsize, Ordering}; -use std::sync::OnceLock; +use std::sync::LazyLock; use std::time::Duration; use rmcp::model::{CallToolResult, Content}; @@ -59,38 +59,51 @@ fn resolve_login_shell_path() -> Option { std::env::var("SHELL").unwrap_or_else(|_| "sh".to_string()) }; - std::process::Command::new(&shell) + let mut child = std::process::Command::new(&shell) .args(["-l", "-i", "-c", "echo $PATH"]) .stdin(Stdio::null()) + .stdout(Stdio::piped()) .stderr(Stdio::null()) - .output() - .ok() - .and_then(|output| { - if output.status.success() { - // Take the last non-empty line — interactive shells may emit - // extra output from profile scripts before our echo. - String::from_utf8_lossy(&output.stdout) - .lines() - .rev() - .find(|line| !line.trim().is_empty()) - .map(|line| line.trim().to_string()) - .filter(|path| !path.is_empty()) - } else { - None - } - }) + .spawn() + .ok()?; + + let mut stdout = child.stdout.take()?; + let (tx, rx) = std::sync::mpsc::channel(); + std::thread::spawn(move || { + let mut buf = Vec::new(); + use std::io::Read; + if stdout.read_to_end(&mut buf).is_ok() { + let _ = tx.send(buf); + } + }); + + match rx.recv_timeout(Duration::from_secs(5)) { + Ok(buf) if child.wait().is_ok_and(|s| s.success()) => { + // Take the last non-empty line — interactive shells may emit + // extra output from profile scripts before our echo. + String::from_utf8_lossy(&buf) + .lines() + .rev() + .find(|line| !line.trim().is_empty()) + .map(|line| line.trim().to_string()) + .filter(|path| !path.is_empty()) + } + _ => { + let _ = child.kill(); + let _ = child.wait(); + None + } + } } -/// Returns the user's full login shell PATH, resolved once and cached. #[cfg(not(windows))] -fn user_login_path() -> Option<&'static str> { - static CACHED: OnceLock> = OnceLock::new(); - CACHED.get_or_init(resolve_login_shell_path).as_deref() -} +static LOGIN_PATH: LazyLock> = LazyLock::new(resolve_login_shell_path); pub struct ShellTool { output_dir: tempfile::TempDir, call_index: AtomicUsize, + #[cfg(not(windows))] + login_path: Option, } impl ShellTool { @@ -98,6 +111,18 @@ impl ShellTool { Ok(Self { output_dir: tempfile::tempdir()?, call_index: AtomicUsize::new(0), + #[cfg(not(windows))] + login_path: LOGIN_PATH.clone(), + }) + } + + #[cfg(test)] + pub fn new_for_test() -> std::io::Result { + Ok(Self { + output_dir: tempfile::tempdir()?, + call_index: AtomicUsize::new(0), + #[cfg(not(windows))] + login_path: None, }) } @@ -114,7 +139,19 @@ impl ShellTool { return Self::error_result("Command cannot be empty.", None); } - let execution = match run_command(¶ms.command, params.timeout_secs, working_dir).await { + #[cfg(not(windows))] + let login_path = self.login_path.as_deref(); + #[cfg(windows)] + let login_path: Option<&str> = None; + + let execution = match run_command( + ¶ms.command, + params.timeout_secs, + working_dir, + login_path, + ) + .await + { Ok(execution) => execution, Err(error) => return Self::error_result(&error, None), }; @@ -226,14 +263,14 @@ async fn run_command( command_line: &str, timeout_secs: Option, working_dir: Option<&std::path::Path>, + login_path: Option<&str>, ) -> Result { let mut command = build_shell_command(command_line); if let Some(path) = working_dir { command.current_dir(path); } - #[cfg(not(windows))] - if let Some(path) = user_login_path() { + if let Some(path) = login_path { command.env("PATH", path); } @@ -468,7 +505,7 @@ mod tests { #[tokio::test] async fn shell_executes_command() { - let tool = ShellTool::new().unwrap(); + let tool = ShellTool::new_for_test().unwrap(); let result = tool .shell(ShellParams { command: "echo hello".to_string(), @@ -483,7 +520,7 @@ mod tests { #[cfg(not(windows))] #[tokio::test] async fn shell_returns_error_for_non_zero_exit() { - let tool = ShellTool::new().unwrap(); + let tool = ShellTool::new_for_test().unwrap(); let result = tool .shell(ShellParams { command: "echo fail && exit 7".to_string(), @@ -499,7 +536,7 @@ mod tests { #[tokio::test] async fn shell_uses_working_dir_for_relative_execution() { let dir = tempfile::tempdir().unwrap(); - let tool = ShellTool::new().unwrap(); + let tool = ShellTool::new_for_test().unwrap(); let result = tool .shell_with_cwd( ShellParams { @@ -598,7 +635,7 @@ mod tests { #[test] fn call_index_cycles_through_slots() { - let tool = ShellTool::new().unwrap(); + let tool = ShellTool::new_for_test().unwrap(); for _cycle in 0..3 { for expected in 0..OUTPUT_SLOTS { let slot = tool.call_index.fetch_add(1, Ordering::Relaxed) % OUTPUT_SLOTS; @@ -609,7 +646,7 @@ mod tests { #[test] fn concurrent_calls_get_distinct_slots() { - let tool = ShellTool::new().unwrap(); + let tool = ShellTool::new_for_test().unwrap(); let mut slots: Vec = (0..OUTPUT_SLOTS) .map(|_| tool.call_index.fetch_add(1, Ordering::Relaxed) % OUTPUT_SLOTS) .collect(); @@ -630,7 +667,7 @@ mod tests { } } - let tool = ShellTool::new().unwrap(); + let tool = ShellTool::new_for_test().unwrap(); let start = std::time::Instant::now(); let result = tool .shell(ShellParams { diff --git a/crates/goose/src/providers/claude_acp.rs b/crates/goose/src/providers/claude_acp.rs index 54ff9aabfb..0003624d45 100644 --- a/crates/goose/src/providers/claude_acp.rs +++ b/crates/goose/src/providers/claude_acp.rs @@ -61,6 +61,7 @@ impl ProviderDef for ClaudeAcpProvider { mcp_servers: extension_configs_to_mcp_servers(&extensions), session_mode_id: Some(map_goose_mode(goose_mode)), permission_mapping, + notification_callback: None, }; let metadata = Self::metadata(); diff --git a/crates/goose/src/providers/codex_acp.rs b/crates/goose/src/providers/codex_acp.rs index f25457475c..bf755ac154 100644 --- a/crates/goose/src/providers/codex_acp.rs +++ b/crates/goose/src/providers/codex_acp.rs @@ -84,6 +84,7 @@ impl ProviderDef for CodexAcpProvider { // Disabled until https://github.com/zed-industries/codex-acp/issues/179 is fixed. session_mode_id: None, permission_mapping, + notification_callback: None, }; let metadata = Self::metadata();