diff --git a/crates/goose/src/acp/provider.rs b/crates/goose/src/acp/provider.rs index 86352c5cbe..d6de279727 100644 --- a/crates/goose/src/acp/provider.rs +++ b/crates/goose/src/acp/provider.rs @@ -419,20 +419,23 @@ impl Provider for AcpProvider { } async fn update_mode(&self, session_id: &str, mode: GooseMode) -> Result<(), ProviderError> { - let mode_str = self - .mode_mapping - .get(&mode) - .cloned() - .unwrap_or_else(|| format!("{mode:?}")); - - if self.session_has_config_option(SessionConfigOptionCategory::Mode) { - self.send_set_config_option(session_id, "mode".into(), mode_str) - .await - .map_err(|e| ProviderError::RequestFailed(format!("Failed to set mode: {e}")))?; - } else { - self.send_set_mode(session_id, mode_str) - .await - .map_err(|e| ProviderError::RequestFailed(format!("Failed to set mode: {e}")))?; + // An unmapped mode means the provider opted out of ACP mode + // negotiation (e.g. codex-acp); the mode is still tracked locally + // since it drives permission routing. + if let Some(mode_str) = self.mode_mapping.get(&mode).cloned() { + if self.session_has_config_option(SessionConfigOptionCategory::Mode) { + self.send_set_config_option(session_id, "mode".into(), mode_str) + .await + .map_err(|e| { + ProviderError::RequestFailed(format!("Failed to set mode: {e}")) + })?; + } else { + self.send_set_mode(session_id, mode_str) + .await + .map_err(|e| { + ProviderError::RequestFailed(format!("Failed to set mode: {e}")) + })?; + } } if let Ok(mut guard) = self.goose_mode.lock() { @@ -1210,9 +1213,7 @@ async fn apply_session_mode( session: NewSessionResponse, ) -> Result { let current_mode = goose_mode.lock().ok().map(|mode| *mode); - let requested_mode_id = current_mode - .and_then(|mode| config.mode_mapping.get(&mode).cloned()) - .or_else(|| config.session_mode_id.clone()); + let requested_mode_id = initial_session_mode_id(config, current_mode); if let (Some(mode_id), Some(modes)) = (requested_mode_id, session.modes.as_ref()) { if modes.current_mode_id.0.as_ref() != mode_id.as_str() { @@ -1248,6 +1249,18 @@ async fn apply_session_mode( Ok(session) } +// An empty mode_mapping with no session_mode_id means the provider opted out +// of ACP mode negotiation entirely (e.g. codex-acp, which pins behavior via +// CLI args); no session/set_mode is sent for a new session. +fn initial_session_mode_id( + config: &AcpProviderConfig, + current_mode: Option, +) -> Option { + current_mode + .and_then(|mode| config.mode_mapping.get(&mode).cloned()) + .or_else(|| config.session_mode_id.clone()) +} + pub fn extension_configs_to_mcp_servers(configs: &[ExtensionConfig]) -> Vec { let mut servers = Vec::new(); @@ -1811,6 +1824,93 @@ mod tests { assert!(rx.try_recv().is_err()); } + fn test_acp_config( + mode_mapping: HashMap, + session_mode_id: Option, + ) -> AcpProviderConfig { + AcpProviderConfig { + command: PathBuf::new(), + args: vec![], + env: vec![], + env_remove: vec![], + work_dir: PathBuf::new(), + mcp_servers: vec![], + session_mode_id, + session_config_options: vec![], + model_config_option_id: None, + mode_mapping, + notification_callback: None, + } + } + + #[test_case(GooseMode::Auto)] + #[test_case(GooseMode::Approve)] + #[test_case(GooseMode::SmartApprove)] + #[test_case(GooseMode::Chat)] + fn initial_session_mode_id_none_when_mode_negotiation_disabled(mode: GooseMode) { + let config = test_acp_config(HashMap::new(), None); + assert_eq!(initial_session_mode_id(&config, Some(mode)), None); + } + + #[test] + fn initial_session_mode_id_prefers_mapping_then_fallback() { + let mapping = HashMap::from([(GooseMode::Auto, "bypassPermissions".to_string())]); + let config = test_acp_config(mapping, Some("default".to_string())); + + assert_eq!( + initial_session_mode_id(&config, Some(GooseMode::Auto)), + Some("bypassPermissions".to_string()) + ); + assert_eq!( + initial_session_mode_id(&config, Some(GooseMode::Chat)), + Some("default".to_string()) + ); + } + + #[tokio::test] + async fn update_mode_without_mapping_skips_acp_request_but_tracks_mode() { + let (tx, mut rx) = mpsc::channel(1); + let (provider, _) = test_provider_with_tx(Some(tx)); + + provider + .update_mode("session", GooseMode::Chat) + .await + .unwrap(); + + assert!(rx.try_recv().is_err()); + assert_eq!(*provider.goose_mode.lock().unwrap(), GooseMode::Chat); + } + + #[tokio::test] + async fn update_mode_with_mapping_sends_set_mode() { + let (tx, mut rx) = mpsc::channel(1); + let (mut provider, _) = test_provider_with_tx(Some(tx)); + provider.mode_mapping = HashMap::from([(GooseMode::Chat, "plan".to_string())]); + + let handle = tokio::spawn(async move { + provider + .update_mode("session", GooseMode::Chat) + .await + .unwrap(); + provider + }); + + match rx.recv().await.expect("expected a SetMode request") { + ClientRequest::SetMode { + mode_id, + response_tx, + .. + } => { + assert_eq!(mode_id, "plan"); + let _ = response_tx.send(Ok(())); + } + _ => panic!("unexpected request kind"), + } + + let provider = handle.await.unwrap(); + assert_eq!(*provider.goose_mode.lock().unwrap(), GooseMode::Chat); + } + #[test] fn messages_to_prompt_includes_all_prior_handoff_context() { let messages = vec![ @@ -1975,7 +2075,7 @@ mod tests { ("read-only".to_string(), vec![GooseMode::Approve, GooseMode::Chat]), ("auto".to_string(), vec![GooseMode::SmartApprove]), ]) - ; "codex duplicate read-only" + ; "duplicate read-only ids collect both modes" )] fn test_reverse_mode_mapping( forward: HashMap, @@ -2035,7 +2135,7 @@ mod tests { resolve_model_info("test", &response) } - fn codex_reverse_modes() -> HashMap> { + fn duplicate_read_only_reverse_modes() -> HashMap> { HashMap::from([ ("full-access".to_string(), vec![GooseMode::Auto]), ( @@ -2067,7 +2167,7 @@ mod tests { ; "unknown mode id returns None" )] fn test_resolve_mode(mode_id: &str, current: GooseMode, expected: Option) { - let reverse_modes = codex_reverse_modes(); + let reverse_modes = duplicate_read_only_reverse_modes(); let current = Arc::new(Mutex::new(current)); let result = resolve_mode(&reverse_modes, mode_id, ¤t); if mode_id == "read-only" && expected == Some(GooseMode::Approve) { diff --git a/crates/goose/src/providers/codex_acp.rs b/crates/goose/src/providers/codex_acp.rs index e61eb8faf7..b7e6c95384 100644 --- a/crates/goose/src/providers/codex_acp.rs +++ b/crates/goose/src/providers/codex_acp.rs @@ -62,7 +62,10 @@ impl ProviderDef for CodexAcpProvider { 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 + // Goose mode is pinned via -c overrides instead of ACP mode + // negotiation: session mode ids differ across codex-acp bridges + // (@zed-industries <=0.16 vs @agentclientprotocol >=1.0), so any + // hardcoded id set breaks one of them. let (approval_policy, sandbox_mode) = map_goose_mode(goose_mode); let mut args = vec![ "-c".to_string(), @@ -83,14 +86,6 @@ impl ProviderDef for CodexAcpProvider { ]); } - // Chat and Approve both map to "read-only". - let mode_mapping = HashMap::from([ - (GooseMode::Auto, "full-access".to_string()), - (GooseMode::Approve, "read-only".to_string()), - (GooseMode::SmartApprove, "auto".to_string()), - (GooseMode::Chat, "read-only".to_string()), - ]); - let provider_config = AcpProviderConfig { command: resolved_command, args, @@ -98,11 +93,13 @@ impl ProviderDef for CodexAcpProvider { env_remove: vec![], work_dir: working_dir, mcp_servers, - // Disabled until https://github.com/zed-industries/codex-acp/issues/179 is fixed. + // No session_mode_id and an empty mode_mapping keep goose + // from ever sending session/set_mode; behavior is pinned via + // the -c args above. session_mode_id: None, session_config_options: vec![], model_config_option_id: None, - mode_mapping, + mode_mapping: HashMap::new(), notification_callback: None, };