diff --git a/Cargo.lock b/Cargo.lock index 76d9aefd0b..ab6fb896e5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3224,6 +3224,12 @@ version = "0.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1435fa1053d8b2fbbe9be7e97eca7f33d37b28409959813daefc1446a14247f1" +[[package]] +name = "downcast-rs" +version = "2.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "117240f60069e65410b3ae1bb213295bd828f707b5bec6596a1afc8793ce0cbc" + [[package]] name = "dprint-swc-ext" version = "0.26.0" @@ -4352,6 +4358,7 @@ dependencies = [ "dashmap 6.1.0", "dirs 5.0.1", "dotenvy", + "downcast-rs", "encoding_rs", "env-lock", "etcetera 0.11.0", @@ -4573,7 +4580,7 @@ dependencies = [ [[package]] name = "goose-sdk" -version = "1.29.0" +version = "1.30.0" dependencies = [ "agent-client-protocol-schema", "sacp", diff --git a/crates/goose-acp/src/server.rs b/crates/goose-acp/src/server.rs index 9751fdc777..6df7b9ae1d 100644 --- a/crates/goose-acp/src/server.rs +++ b/crates/goose-acp/src/server.rs @@ -3,7 +3,7 @@ use crate::fs::AcpTools; use crate::tools::AcpAwareToolMeta; use anyhow::Result; use fs_err as fs; -use goose::acp::PermissionDecision; +use goose::acp::{AcpProvider, PermissionDecision}; use goose::agents::extension::{Envs, PLATFORM_EXTENSIONS}; use goose::agents::mcp_client::McpClientTrait; use goose::agents::platform_extensions::developer::DeveloperClient; @@ -67,6 +67,7 @@ struct GooseAcpSession { pub struct GooseAcpAgent { sessions: Arc>>, provider_factory: ProviderConstructor, + provider: Mutex>>, builtins: Vec, client_fs_capabilities: OnceCell, client_terminal: OnceCell, @@ -400,6 +401,7 @@ impl GooseAcpAgent { Ok(Self { sessions: Arc::new(Mutex::new(HashMap::new())), provider_factory, + provider: Mutex::new(None), builtins, client_fs_capabilities: OnceCell::new(), client_terminal: OnceCell::new(), @@ -882,6 +884,16 @@ impl GooseAcpAgent { Self::add_mcp_extensions(&agent, args.mcp_servers, &goose_session.id).await?; + // Pre-populate the downstream session so build_model_state + // reads cached model info instead of creating a throwaway session. + if let Ok(acp) = provider.clone().downcast_arc::() { + acp.ensure_session(Some(&goose_session.id)) + .await + .map_err(|e| { + sacp::Error::internal_error().data(format!("Failed to ensure session: {}", e)) + })?; + } + let session = GooseAcpSession { agent, messages: Conversation::new_unvalidated(Vec::new()), @@ -909,6 +921,11 @@ impl GooseAcpAgent { } async fn init_provider(&self, agent: &Agent, session: &Session) -> Result> { + let mut cached = self.provider.lock().await; + if let Some(provider) = cached.as_ref() { + agent.update_provider(provider.clone(), &session.id).await?; + return Ok(provider.clone()); + } let model_config = match &session.model_config { Some(config) => config.clone(), None => { @@ -921,6 +938,7 @@ impl GooseAcpAgent { }; let provider = (self.provider_factory)(model_config, Vec::new()).await?; agent.update_provider(provider.clone(), &session.id).await?; + *cached = Some(provider.clone()); Ok(provider) } @@ -1216,6 +1234,8 @@ impl GooseAcpAgent { sacp::Error::internal_error().data(format!("Failed to create provider: {}", e)) })?; + *self.provider.lock().await = Some(provider.clone()); + let agent = self.get_session_agent(session_id, None).await?; agent .update_provider(provider, session_id) diff --git a/crates/goose/Cargo.toml b/crates/goose/Cargo.toml index a3a68b699e..046860d0c2 100644 --- a/crates/goose/Cargo.toml +++ b/crates/goose/Cargo.toml @@ -61,6 +61,7 @@ native-tls = [ workspace = true [dependencies] +downcast-rs = "2.0.2" lru = { workspace = true } rmcp = { workspace = true, features = [ "client", diff --git a/crates/goose/src/acp/provider.rs b/crates/goose/src/acp/provider.rs index 6f9d0f3ba8..76a415f5c4 100644 --- a/crates/goose/src/acp/provider.rs +++ b/crates/goose/src/acp/provider.rs @@ -1,3 +1,4 @@ +use crate::config::search_path::SearchPaths; use agent_client_protocol_schema::AGENT_METHOD_NAMES; use anyhow::{Context, Result}; use async_stream::try_stream; @@ -27,10 +28,12 @@ use tokio_util::compat::{TokioAsyncReadCompatExt, TokioAsyncWriteCompatExt}; use crate::acp::{map_permission_response, PermissionDecision, PermissionMapping}; use crate::config::{ExtensionConfig, GooseMode}; use crate::conversation::message::{Message, MessageContent}; +use crate::conversation::Conversation; use crate::model::ModelConfig; use crate::permission::permission_confirmation::PrincipalType; use crate::permission::{Permission, PermissionConfirmation}; use crate::providers::base::{MessageStream, PermissionRouting, Provider}; +use crate::providers::cli_common::generate_simple_session_description; use crate::providers::errors::ProviderError; use crate::subprocess::configure_subprocess; @@ -38,7 +41,7 @@ use crate::subprocess::configure_subprocess; pub const ACP_CURRENT_MODEL: &str = "current"; pub struct AcpProviderConfig { - pub command: PathBuf, + pub command: String, pub args: Vec, pub env: Vec<(String, String)>, pub env_remove: Vec, @@ -121,7 +124,7 @@ enum AcpUpdate { pub struct AcpProvider { name: String, - model: ModelConfig, + model: Mutex, goose_mode: Arc>, tx: Option>, loop_thread: Option>, @@ -136,7 +139,11 @@ pub struct AcpProvider { session_model: Arc>>, auth_methods: Vec, supports_close: bool, + supports_list: bool, + // Cached NewSessionResponse for model list and config resolution. + // Populated from whichever comes first: ensure_session or get_init_session. init_session: OnceCell, + session_titles: Arc>>, } impl std::fmt::Debug for AcpProvider { @@ -214,6 +221,8 @@ impl AcpProvider { let permission_mapping = config.permission_mapping.clone(); let rejected_tool_calls = Arc::new(TokioMutex::new(HashSet::new())); let goose_mode = Arc::new(Mutex::new(goose_mode)); + let session_titles = Arc::new(TokioMutex::new(HashMap::new())); + let acp_to_goose_id = Arc::new(TokioMutex::new(HashMap::new())); let client_loop = AcpClientLoop::new(config, goose_mode.clone()); let loop_thread = spawn_client_loop(run(client_loop, rx, init_tx)); @@ -221,12 +230,10 @@ impl AcpProvider { .await .context("ACP client initialization cancelled")??; - let supports_close = init_response - .agent_capabilities - .session_capabilities - .close - .is_some(); - let mut provider = Self::new_with_runtime( + let session_caps = &init_response.agent_capabilities.session_capabilities; + let supports_close = session_caps.close.is_some(); + let supports_list = session_caps.list.is_some(); + let provider = Self::new_with_runtime( name, model, goose_mode, @@ -237,13 +244,10 @@ impl AcpProvider { rejected_tool_calls, init_response.auth_methods, supports_close, + supports_list, + session_titles, + acp_to_goose_id, ); - if provider.model.model_name == ACP_CURRENT_MODEL { - let response = provider.get_init_session().await?; - let (current_model, _) = resolve_model_info(&provider.name, response)?; - tracing::info!(from = ACP_CURRENT_MODEL, to = %current_model, "resolved ACP model"); - provider.model.model_name = current_model; - } Ok(provider) } @@ -259,10 +263,13 @@ impl AcpProvider { rejected_tool_calls: Arc>>, auth_methods: Vec, supports_close: bool, + supports_list: bool, + session_titles: Arc>>, + acp_to_goose_id: Arc>>, ) -> Self { Self { name, - model, + model: Mutex::new(model), goose_mode, tx: Some(tx), loop_thread: Some(loop_thread), @@ -271,11 +278,13 @@ impl AcpProvider { rejected_tool_calls, pending_confirmations: Arc::new(TokioMutex::new(HashMap::new())), goose_to_acp_id: Arc::new(TokioMutex::new(HashMap::new())), - acp_to_goose_id: Arc::new(TokioMutex::new(HashMap::new())), + acp_to_goose_id, session_model: Arc::new(TokioMutex::new(HashMap::new())), auth_methods, supports_close, + supports_list, init_session: OnceCell::new(), + session_titles, } } @@ -472,7 +481,10 @@ impl AcpProvider { .lock() .await .entry(session_id.to_string()) - .or_insert(current_model); + .or_insert(current_model.clone()); + + self.cache_session_metadata(response.clone(), current_model) + .await; } Ok(response) @@ -497,6 +509,14 @@ impl AcpProvider { Ok(response_rx) } + async fn cache_session_metadata(&self, response: NewSessionResponse, model: String) { + self.init_session.get_or_init(|| async { response }).await; + let mut m = self.model.lock().unwrap(); + if m.model_name == ACP_CURRENT_MODEL { + m.model_name = model; + } + } + async fn get_init_session(&self) -> Result<&NewSessionResponse> { self.init_session .get_or_try_init(|| async { @@ -505,6 +525,12 @@ impl AcpProvider { self.close_session_by_acp_id(response.session_id.clone()) .await?; } + let (model, _) = + resolve_model_info(&self.name, &response).map_err(anyhow::Error::from)?; + let mut m = self.model.lock().unwrap(); + if m.model_name == ACP_CURRENT_MODEL { + m.model_name = model; + } Ok(response) }) .await @@ -532,7 +558,45 @@ impl Provider for AcpProvider { } fn get_model_config(&self) -> ModelConfig { - self.model.clone() + self.model.lock().unwrap().clone() + } + + async fn generate_session_name( + &self, + session_id: &str, + messages: &Conversation, + ) -> Result { + // No downstream session yet; nothing to name. + if self.goose_to_acp_id.lock().await.is_empty() { + return Ok(String::new()); + } + + if self.supports_list { + if let Some(title) = self.session_titles.lock().await.get(session_id) { + return Ok(title.clone()); + } + + // Fall back to session/list (returns goose-remapped IDs). + // TODO: replace with session/get when available + // https://github.com/agentclientprotocol/agent-client-protocol/discussions/60 + if let Ok(list) = self.list_sessions().await { + let mut titles = self.session_titles.lock().await; + if let Some(title) = resolve_title_from_list(&list, &mut titles, session_id) { + return Ok(title); + } + } + + // No title yet. system_generated_name skips empty, maybe_update_name retries. + return Ok(String::new()); + } + + // No list capability: use first user message as title. + // https://github.com/google-gemini/gemini-cli/issues/22919 + let (msg, _) = generate_simple_session_description( + &self.model.lock().unwrap().model_name, + messages.messages(), + )?; + Ok(msg.as_concat_text()) } async fn update_mode(&self, session_id: &str, mode: GooseMode) -> Result<(), ProviderError> { @@ -570,6 +634,11 @@ impl Provider for AcpProvider { Ok(()) } + // Agentic providers own their own metadata. + fn skip_moim(&self) -> bool { + true + } + fn permission_routing(&self) -> PermissionRouting { PermissionRouting::ActionRequired } @@ -592,11 +661,11 @@ impl Provider for AcpProvider { ) -> Result { let response = self.ensure_session(Some(session_id)).await?; - // Provider trait has no update_model — stream() is the only place to forward model changes. + // Forward model changes to the agent. Skip "current" (resolved lazily by ensure_session). { let new_model = &model_config.model_name; let tracked = self.session_model.lock().await.get(session_id).cloned(); - if tracked.as_deref() != Some(new_model) { + if new_model != ACP_CURRENT_MODEL && tracked.as_deref() != Some(new_model) { if self .session_has_config_option(session_id, SessionConfigOptionCategory::Model) .await @@ -807,6 +876,7 @@ impl AcpClientLoop { { let prompt_response_tx = prompt_response_tx.clone(); let reverse_modes = reverse_modes.clone(); + let goose_mode = goose_mode.clone(); async move |notification: SessionNotification, _cx| { if let Some(ref cb) = notification_callback { cb(notification.clone()); @@ -915,7 +985,7 @@ impl AcpClientLoop { sacp::on_receive_request!(), ) .connect_with(transport, async move |cx: ConnectionTo| { - handle_requests(config, cx, rx, prompt_response_tx, init_tx).await + handle_requests(config, goose_mode, cx, rx, prompt_response_tx, init_tx).await }) .await?; @@ -924,7 +994,9 @@ impl AcpClientLoop { } async fn spawn_acp_process(config: &AcpProviderConfig) -> Result { - let mut cmd = Command::new(&config.command); + // with_npm() includes npm global bin dir (desktop app PATH may not) + let resolved = SearchPaths::builder().with_npm().resolve(&config.command)?; + let mut cmd = Command::new(&resolved); cmd.args(&config.args) .stdin(Stdio::piped()) .stdout(Stdio::piped()) @@ -952,6 +1024,7 @@ fn log_undelivered(result: Result<(), E>, method: &str) { async fn handle_requests( config: AcpProviderConfig, + goose_mode: Arc>, cx: ConnectionTo, rx: &mut mpsc::Receiver, prompt_response_tx: Arc>>>, @@ -997,13 +1070,18 @@ async fn handle_requests( let result = match session { Ok(session) => { session_ids.push(session.session_id.clone()); - apply_session_mode(&config, &cx, session).await + let mode_id = goose_mode + .lock() + .ok() + .and_then(|m| config.mode_mapping.get(&*m).cloned()); + match mode_id { + Some(id) => apply_session_mode(id, &cx, session).await, + None => Err(anyhow::anyhow!("mode_mapping lookup failed")), + } } - Err(err) => Err(anyhow::anyhow!( - "ACP {} failed: {err}", - AGENT_METHOD_NAMES.session_new - )), - }; + Err(err) => Err(anyhow::anyhow!("{err}")), + } + .map_err(|e| anyhow::anyhow!("ACP {} failed: {e}", AGENT_METHOD_NAMES.session_new)); log_undelivered(response_tx.send(result), AGENT_METHOD_NAMES.session_new); } ClientRequest::ListSessions { response_tx } => { @@ -1143,29 +1221,29 @@ async fn handle_requests( } async fn apply_session_mode( - config: &AcpProviderConfig, + desired_mode_id: String, cx: &ConnectionTo, session: NewSessionResponse, ) -> Result { - if let (Some(mode_id), Some(modes)) = (config.session_mode_id.clone(), session.modes.as_ref()) { - if modes.current_mode_id.0.as_ref() != mode_id.as_str() { + if let Some(modes) = session.modes.as_ref() { + if modes.current_mode_id.0.as_ref() != desired_mode_id.as_str() { let available: Vec = modes .available_modes .iter() .map(|mode| mode.id.0.to_string()) .collect(); - if !available.iter().any(|id| id == &mode_id) { + if !available.iter().any(|id| id == &desired_mode_id) { return Err(anyhow::anyhow!( "Requested mode '{}' not offered by agent. Available modes: {}", - mode_id, + desired_mode_id, available.join(", ") )); } let _: SetSessionModeResponse = cx .send_request(SetSessionModeRequest::new( session.session_id.clone(), - mode_id, + desired_mode_id, )) .block_task() .await @@ -1398,6 +1476,22 @@ fn resolve_mode( } } +// Caches titles from session/list and returns the title for session_id if found. +fn resolve_title_from_list( + response: &ListSessionsResponse, + titles: &mut HashMap, + session_id: &str, +) -> Option { + for info in &response.sessions { + if let Some(ref title) = info.title { + titles + .entry(info.session_id.0.to_string()) + .or_insert_with(|| title.clone()); + } + } + titles.get(session_id).cloned() +} + fn permission_decision_from_mode(goose_mode: GooseMode) -> Option { match goose_mode { GooseMode::Auto => Some(PermissionDecision::AllowOnce), @@ -1406,8 +1500,8 @@ fn permission_decision_from_mode(goose_mode: GooseMode) -> Option, @@ -1428,7 +1522,10 @@ fn map_sessions_to_goose_ids( mod tests { use super::*; use crate::agents::extension::Envs; - use sacp::schema::{SessionConfigOption, SessionConfigSelectOption, SessionInfo}; + use sacp::schema::{ + AgentCapabilities, SessionCapabilities, SessionConfigOption, SessionConfigSelectOption, + SessionInfo, SessionListCapabilities, + }; use test_case::test_case; #[test_case( @@ -1743,4 +1840,129 @@ mod tests { assert_eq!(result, expected); } } + + #[test_case( + vec![SessionInfo::new(SessionId::new("s1"), "/tmp").title("Hello")], + HashMap::new(), + "s1", + Some("Hello".to_string()) + ; "returns matching title" + )] + #[test_case( + vec![SessionInfo::new(SessionId::new("s1"), "/tmp").title("Hello")], + HashMap::new(), + "s2", + None + ; "returns none for missing session" + )] + #[test_case( + vec![SessionInfo::new(SessionId::new("s1"), "/tmp")], + HashMap::new(), + "s1", + None + ; "returns none when session has no title" + )] + #[test_case( + vec![SessionInfo::new(SessionId::new("s1"), "/tmp").title("From list")], + HashMap::from([("s1".to_string(), "From notification".to_string())]), + "s1", + Some("From notification".to_string()) + ; "existing title not overwritten by list" + )] + fn test_resolve_title_from_list( + sessions: Vec, + mut existing: HashMap, + session_id: &str, + expected: Option, + ) { + let result = resolve_title_from_list( + &ListSessionsResponse::new(sessions), + &mut existing, + session_id, + ); + assert_eq!(result, expected); + } + + #[test_case( + HashMap::new(), + SessionCapabilities::new().list(SessionListCapabilities::new()), + HashMap::new(), + "s1", Conversation::new_unvalidated(vec![Message::user().with_text("hello")]), + Ok("".to_string()) + ; "no downstream session returns empty" + )] + #[test_case( + HashMap::from([("s1".to_string(), NewSessionResponse::new(SessionId::new("acp-1")))]), + SessionCapabilities::new().list(SessionListCapabilities::new()), + HashMap::from([("s1".to_string(), "cached title".to_string())]), + "s1", Conversation::new_unvalidated(vec![]), + Ok("cached title".to_string()) + ; "returns cached session title" + )] + #[test_case( + HashMap::from([("s1".to_string(), NewSessionResponse::new(SessionId::new("acp-1")))]), + SessionCapabilities::new(), + HashMap::new(), + "s1", Conversation::new_unvalidated(vec![Message::user().with_text("say hello")]), + Ok("say hello".to_string()) + ; "no list uses first user message" + )] + #[test_case( + HashMap::from([("s1".to_string(), NewSessionResponse::new(SessionId::new("acp-1")))]), + SessionCapabilities::new(), + HashMap::new(), + "s1", Conversation::new_unvalidated(vec![]), + Ok("Simple task".to_string()) + ; "no list no messages returns simple task" + )] + #[test_case( + HashMap::from([("s1".to_string(), NewSessionResponse::new(SessionId::new("acp-1")))]), + SessionCapabilities::new(), + HashMap::new(), + "s1", Conversation::new_unvalidated(vec![Message::user().with_text("one two three four five six")]), + Ok("one two three four".to_string()) + ; "no list truncates to 4 words" + )] + #[tokio::test] + async fn test_generate_session_name( + goose_to_acp: HashMap, + session_caps: SessionCapabilities, + session_titles: HashMap, + session_id: &str, + conv: Conversation, + expected: Result, + ) { + let init_response = InitializeResponse::new(ProtocolVersion::LATEST) + .agent_capabilities(AgentCapabilities::new().session_capabilities(session_caps)); + let provider = AcpProvider::start( + "test".into(), + ModelConfig::new("test-model").unwrap(), + GooseMode::Auto, + AcpProviderConfig { + command: "true".to_string(), + args: vec![], + env: vec![], + env_remove: vec![], + work_dir: PathBuf::from("."), + mcp_servers: vec![], + session_mode_id: None, + mode_mapping: HashMap::new(), + permission_mapping: PermissionMapping::default(), + notification_callback: None, + }, + Box::new(move |_cl, _rx, init_tx| { + Box::pin(async move { + let _ = init_tx.send(Ok(init_response)); + }) + }), + ) + .await + .unwrap(); + *provider.goose_to_acp_id.lock().await = goose_to_acp; + *provider.session_titles.lock().await = session_titles; + assert_eq!( + provider.generate_session_name(session_id, &conv).await, + expected + ); + } } diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index 92de31bc4f..33d08a693b 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -1255,18 +1255,23 @@ impl Agent { break; } - let conversation_with_moim = super::moim::inject_moim( - &session_config.id, - conversation.clone(), - &self.extension_manager, - &working_dir, - ).await; + let provider = self.provider().await?; + let messages = if provider.skip_moim() { + conversation.clone() + } else { + super::moim::inject_moim( + &session_config.id, + conversation.clone(), + &self.extension_manager, + &working_dir, + ).await + }; let mut stream = Self::stream_response_from_provider( - self.provider().await?, + provider, &session_config.id, &system_prompt, - conversation_with_moim.messages(), + messages.messages(), &tools, &toolshim_tools, ).await?; diff --git a/crates/goose/src/providers/base.rs b/crates/goose/src/providers/base.rs index 12b3a3c510..f50e788031 100644 --- a/crates/goose/src/providers/base.rs +++ b/crates/goose/src/providers/base.rs @@ -1,5 +1,6 @@ use anyhow::Result; use async_trait::async_trait; +use downcast_rs::{impl_downcast, DowncastSync}; use futures::future::BoxFuture; use futures::Stream; use serde::{Deserialize, Serialize}; @@ -502,7 +503,7 @@ pub enum PermissionRouting { /// Base trait for AI providers (OpenAI, Anthropic, etc) #[async_trait] -pub trait Provider: Send + Sync { +pub trait Provider: DowncastSync { /// Get the name of this provider instance fn get_name(&self) -> &str; @@ -588,6 +589,10 @@ pub trait Provider: Send + Sync { false } + fn skip_moim(&self) -> bool { + false + } + /// Fetch models filtered by canonical registry and usability async fn fetch_recommended_models(&self) -> Result, ProviderError> { let all_models = self.fetch_supported_models().await?; @@ -783,6 +788,7 @@ pub trait Provider: Send + Sync { false } } +impl_downcast!(sync Provider); /// A message stream yields partial text content but complete tool calls, all within the Message object /// So a message with text will contain potentially just a word of a longer response, but tool calls diff --git a/crates/goose/src/providers/claude_acp.rs b/crates/goose/src/providers/claude_acp.rs index 3d8c6a95fa..9e1af9193f 100644 --- a/crates/goose/src/providers/claude_acp.rs +++ b/crates/goose/src/providers/claude_acp.rs @@ -7,7 +7,6 @@ use crate::acp::{ extension_configs_to_mcp_servers, AcpProvider, AcpProviderConfig, PermissionMapping, ACP_CURRENT_MODEL, }; -use crate::config::search_path::SearchPaths; use crate::config::{Config, GooseMode}; use crate::model::ModelConfig; use crate::providers::base::{ProviderDef, ProviderMetadata}; @@ -45,10 +44,6 @@ impl ProviderDef for ClaudeAcpProvider { ) -> BoxFuture<'static, Result> { Box::pin(async move { let config = Config::global(); - // with_npm() includes npm global bin dir (desktop app PATH may not) - let resolved_command = SearchPaths::builder() - .with_npm() - .resolve(CLAUDE_ACP_BINARY)?; let goose_mode = config.get_goose_mode().unwrap_or(GooseMode::Auto); // claude-agent-acp permission option_ids @@ -70,7 +65,7 @@ impl ProviderDef for ClaudeAcpProvider { ]); let provider_config = AcpProviderConfig { - command: resolved_command, + command: CLAUDE_ACP_BINARY.to_string(), args: vec![], env: vec![], // Prevent nested-session detection in claude-agent-acp (wraps Claude Code) diff --git a/crates/goose/src/providers/claude_code.rs b/crates/goose/src/providers/claude_code.rs index ecdb44bb4f..54e4b9a7a1 100644 --- a/crates/goose/src/providers/claude_code.rs +++ b/crates/goose/src/providers/claude_code.rs @@ -689,6 +689,10 @@ impl Provider for ClaudeCodeProvider { Ok(()) } + fn skip_moim(&self) -> bool { + true + } + fn permission_routing(&self) -> PermissionRouting { PermissionRouting::ActionRequired } diff --git a/crates/goose/src/providers/codex.rs b/crates/goose/src/providers/codex.rs index d121097c69..09b35f98fd 100644 --- a/crates/goose/src/providers/codex.rs +++ b/crates/goose/src/providers/codex.rs @@ -730,6 +730,10 @@ impl Provider for CodexProvider { )) } + fn skip_moim(&self) -> bool { + true + } + async fn update_mode(&self, session_id: &str, mode: GooseMode) -> Result<(), ProviderError> { self.mode_by_session .write() diff --git a/crates/goose/src/providers/codex_acp.rs b/crates/goose/src/providers/codex_acp.rs index bdecb7db9e..ca69f915cc 100644 --- a/crates/goose/src/providers/codex_acp.rs +++ b/crates/goose/src/providers/codex_acp.rs @@ -7,7 +7,6 @@ use crate::acp::{ extension_configs_to_mcp_servers, AcpProvider, AcpProviderConfig, PermissionMapping, ACP_CURRENT_MODEL, }; -use crate::config::search_path::SearchPaths; use crate::config::{Config, GooseMode}; use crate::model::ModelConfig; use crate::providers::base::{ProviderDef, ProviderMetadata}; @@ -44,36 +43,20 @@ impl ProviderDef for CodexAcpProvider { ) -> BoxFuture<'static, Result> { Box::pin(async move { let config = Config::global(); - // with_npm() includes npm global bin dir (desktop app PATH may not) - let resolved_command = SearchPaths::builder() - .with_npm() - .resolve(CODEX_ACP_PROVIDER_NAME)?; let work_dir = std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")); - let env = vec![]; let goose_mode = config.get_goose_mode().unwrap_or(GooseMode::Auto); let mcp_servers = extension_configs_to_mcp_servers(&extensions); - // fixed goose mode via -c overrides until session/set-mode works - let (approval_policy, sandbox_mode) = map_goose_mode(goose_mode); - let mut args = vec![ + // Seed read-only so modes() returns a preset match and session/new + // includes modes. session_mode_id upgrades to the real mode. + // https://github.com/zed-industries/codex-acp/issues/221 + let args = vec![ "-c".to_string(), - format!("approval_policy={approval_policy}"), + "sandbox_mode=read-only".to_string(), "-c".to_string(), - format!("sandbox_mode={sandbox_mode}"), + "sandbox_workspace_write.network_access=true".to_string(), ]; - // Codex sandbox blocks network by default. Enable it when HTTP MCP - // servers are configured so codex-acp can connect to them. - let has_http_mcp = mcp_servers - .iter() - .any(|s| matches!(s, sacp::schema::McpServer::Http(_))); - if has_http_mcp { - args.extend([ - "-c".to_string(), - "sandbox_workspace_write.network_access=true".to_string(), - ]); - } - // codex-acp permission option_ids let permission_mapping = PermissionMapping { allow_option_id: Some("approved".to_string()), @@ -90,14 +73,13 @@ impl ProviderDef for CodexAcpProvider { ]); let provider_config = AcpProviderConfig { - command: resolved_command, + command: CODEX_ACP_PROVIDER_NAME.to_string(), args, - env, + env: vec![], env_remove: vec![], work_dir, mcp_servers, - // Disabled until https://github.com/zed-industries/codex-acp/issues/179 is fixed. - session_mode_id: None, + session_mode_id: Some(mode_mapping[&goose_mode].clone()), mode_mapping, permission_mapping, notification_callback: None, @@ -108,31 +90,3 @@ impl ProviderDef for CodexAcpProvider { }) } } - -// Codex sandbox scope determines what needs approval: operations within the -// sandbox are auto-approved, operations outside it trigger on-request prompts. -// So Approve uses read-only sandbox to force write approvals through goose. -fn map_goose_mode(goose_mode: GooseMode) -> (&'static str, &'static str) { - match goose_mode { - GooseMode::Auto => ("never", "danger-full-access"), - GooseMode::SmartApprove => ("on-request", "workspace-write"), - GooseMode::Approve => ("on-request", "read-only"), - GooseMode::Chat => ("never", "read-only"), - } -} - -#[cfg(test)] -mod tests { - use super::*; - use test_case::test_case; - - #[test_case(GooseMode::Auto, "never", "danger-full-access")] - #[test_case(GooseMode::SmartApprove, "on-request", "workspace-write")] - #[test_case(GooseMode::Approve, "on-request", "read-only")] - #[test_case(GooseMode::Chat, "never", "read-only")] - fn test_map_goose_mode(mode: GooseMode, expected_approval: &str, expected_sandbox: &str) { - let (approval, sandbox) = map_goose_mode(mode); - assert_eq!(approval, expected_approval); - assert_eq!(sandbox, expected_sandbox); - } -} diff --git a/crates/goose/src/providers/copilot_acp.rs b/crates/goose/src/providers/copilot_acp.rs index 662a2d65b5..0ec9bbbe66 100644 --- a/crates/goose/src/providers/copilot_acp.rs +++ b/crates/goose/src/providers/copilot_acp.rs @@ -7,7 +7,6 @@ use crate::acp::{ extension_configs_to_mcp_servers, AcpProvider, AcpProviderConfig, PermissionMapping, ACP_CURRENT_MODEL, }; -use crate::config::search_path::SearchPaths; use crate::config::{Config, GooseMode}; use crate::model::ModelConfig; use crate::providers::base::{ProviderDef, ProviderMetadata}; @@ -48,10 +47,6 @@ impl ProviderDef for CopilotAcpProvider { ) -> BoxFuture<'static, Result> { Box::pin(async move { let config = Config::global(); - // with_npm() includes npm global bin dir (desktop app PATH may not) - let resolved_command = SearchPaths::builder() - .with_npm() - .resolve(COPILOT_ACP_BINARY)?; let goose_mode = config.get_goose_mode().unwrap_or(GooseMode::Auto); // Copilot uses standard ACP permission option IDs (allow_once, @@ -74,7 +69,7 @@ impl ProviderDef for CopilotAcpProvider { ]); let provider_config = AcpProviderConfig { - command: resolved_command, + command: COPILOT_ACP_BINARY.to_string(), args, env: vec![], env_remove: vec![], diff --git a/crates/goose/tests/providers.rs b/crates/goose/tests/providers.rs index 36b55d50ff..a2d65cb59d 100644 --- a/crates/goose/tests/providers.rs +++ b/crates/goose/tests/providers.rs @@ -5,6 +5,7 @@ use goose::acp::ACP_CURRENT_MODEL; use goose::agents::{Agent, AgentConfig, AgentEvent, GoosePlatform, PromptManager, SessionConfig}; use goose::config::{ExtensionConfig, GooseMode, PermissionManager}; use goose::conversation::message::{ActionRequiredData, Message, MessageContent}; +use goose::conversation::Conversation; use goose::permission::permission_confirmation::PrincipalType; use goose::permission::{Permission, PermissionConfirmation}; use goose::providers::anthropic::ANTHROPIC_DEFAULT_MODEL; @@ -182,6 +183,11 @@ impl ProviderTestConfig { self } + fn test_mode_update(mut self, v: bool) -> Self { + self.test_mode_update = v; + self + } + fn expect_context_length_exceeded(mut self, v: bool) -> Self { self.expect_context_length_exceeded = v; self @@ -198,7 +204,6 @@ impl ProviderTestConfig { skip, expected_session_id: || Arc::new(IgnoreSessionId), test_smart_approve: false, - test_mode_update: false, test_context_length_exceeded: false, ..Self::with_llm_provider(name, model_name, &[]) } @@ -605,16 +610,46 @@ impl ProviderFixture { self.agent .update_goose_mode(GooseMode::Approve, &self.session_id) .await?; - // Verify tool call now requires permission (ActionRequired). - // Cancel prevents the task from completing → tool fails. + // Use a write prompt so ACP agents trigger sandbox permission requests. + let test_file = tempfile::NamedTempFile::new()?; self.run_permission_test( Permission::Cancel, true, - "Use the get_code tool and output only its result.", + &format!("Write the word 'hello' to {}", test_file.path().display()), "mode_update", ) .await } + + async fn test_session_naming(&self) -> Result<()> { + let model_config = self.provider.get_model_config(); + let mut conv = Conversation::default(); + let prompts = ["say hello", "what is 1+1", "say goodbye"]; + for prompt in &prompts { + let user_msg = Message::user().with_text(*prompt); + conv.push(user_msg.clone()); + let (response, _) = self + .provider + .complete( + &model_config, + &self.session_id, + "You are a helpful assistant.", + conv.messages(), + &[], + ) + .await?; + conv.push(response); + let name = self + .provider + .generate_session_name(&self.session_id, &conv) + .await?; + if !name.is_empty() { + println!("=== {}::session_naming === {}", self.name, name); + return Ok(()); + } + } + panic!("{}: session name not set after 3 prompts", self.name); + } } fn load_env() { @@ -703,6 +738,10 @@ async fn test_provider(config: ProviderTestConfig) -> Result<()> { if config.test_mode_update { run_test(GooseMode::Auto).await?.test_mode_update().await?; } + run_test(GooseMode::Auto) + .await? + .test_session_naming() + .await?; Ok(()) } .await; @@ -871,6 +910,7 @@ async fn test_xai_provider() -> Result<()> { async fn test_claude_code_provider() -> Result<()> { ProviderTestConfig::with_agentic_provider("claude-code", CLAUDE_CODE_DEFAULT_MODEL, "claude") .model_switch_name("sonnet") + .test_mode_update(false) .run() .await } @@ -879,6 +919,7 @@ async fn test_claude_code_provider() -> Result<()> { async fn test_codex_provider() -> Result<()> { ProviderTestConfig::with_agentic_provider("codex", CODEX_DEFAULT_MODEL, "codex") .test_permissions(false) + .test_mode_update(false) .run() .await }