From 2cc4cd5c936541a236028eb480d176d5c7611a75 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 10 Jul 2026 10:48:56 +1000 Subject: [PATCH] fix(acp): pick session mode from the ids the agent actually offers (#10320) Signed-off-by: Matt Toohey Co-authored-by: Claude Fable 5 --- crates/goose/src/acp/provider.rs | 354 ++++++++++++++++---- crates/goose/src/providers/amp_acp.rs | 10 +- crates/goose/src/providers/claude_acp.rs | 10 +- crates/goose/src/providers/codex_acp.rs | 16 +- crates/goose/src/providers/copilot_acp.rs | 10 +- crates/goose/src/providers/pi_acp.rs | 10 +- crates/goose/tests/acp_fixtures/provider.rs | 2 +- 7 files changed, 321 insertions(+), 91 deletions(-) diff --git a/crates/goose/src/acp/provider.rs b/crates/goose/src/acp/provider.rs index 86352c5cb..57cae0ae2 100644 --- a/crates/goose/src/acp/provider.rs +++ b/crates/goose/src/acp/provider.rs @@ -4,7 +4,7 @@ use agent_client_protocol::schema::v1::{ McpServerStdio, NewSessionRequest, NewSessionResponse, PromptRequest, PromptResponse, RequestPermissionOutcome, RequestPermissionRequest, RequestPermissionResponse, SessionConfigKind, SessionConfigOption, SessionConfigOptionCategory, - SessionConfigSelectOptions, SessionId, SessionNotification, SessionUpdate, + SessionConfigSelectOptions, SessionId, SessionModeState, SessionNotification, SessionUpdate, SetSessionConfigOptionRequest, SetSessionModeRequest, SetSessionModeResponse, StopReason, TextContent, ToolCallContent, ToolCallStatus, ToolKind, }; @@ -58,7 +58,7 @@ pub struct AcpProviderConfig { /// provider re-applies this option from the per-completion `ModelConfig` /// whenever the active session model changes. pub model_config_option_id: Option, - pub mode_mapping: HashMap, + pub mode_mapping: HashMap>, pub notification_callback: Option>, } @@ -142,7 +142,7 @@ struct HandoffContextClaim { pub struct AcpProvider { name: String, goose_mode: Arc>, - mode_mapping: HashMap, + mode_mapping: HashMap>, session: AcpSession, @@ -419,20 +419,27 @@ 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}")))?; + if let Some(candidates) = self.mode_mapping.get(&mode) { + let mode_str = select_mode_id(candidates, self.session.response.modes.as_ref()) + .ok_or_else(|| { + ProviderError::RequestFailed(format!( + "None of the mode ids [{}] are offered by the agent", + candidates.join(", ") + )) + })?; + 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,44 +1217,68 @@ 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 candidates = initial_mode_candidates(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() { - let available: Vec = modes - .available_modes - .iter() - .map(|mode| mode.id.0.to_string()) - .collect(); - - if !available.iter().any(|id| id == &mode_id) { + if let Some(modes) = session.modes.as_ref() { + if !candidates.is_empty() { + let Some(mode_id) = select_mode_id(&candidates, Some(modes)) else { + let available: Vec = modes + .available_modes + .iter() + .map(|mode| mode.id.0.to_string()) + .collect(); return Err(anyhow::anyhow!( - "Requested mode '{}' not offered by agent. Available modes: {}", - mode_id, + "Requested mode(s) [{}] not offered by agent. Available modes: {}", + candidates.join(", "), available.join(", ") )); + }; + if modes.current_mode_id.0.as_ref() != mode_id.as_str() { + let _: SetSessionModeResponse = cx + .send_request(SetSessionModeRequest::new( + session.session_id.clone(), + mode_id, + )) + .block_task() + .await + .map_err(|err| { + anyhow::anyhow!( + "ACP agent rejected {}: {err}", + AGENT_METHOD_NAMES.session_set_mode + ) + })?; } - let _: SetSessionModeResponse = cx - .send_request(SetSessionModeRequest::new( - session.session_id.clone(), - mode_id, - )) - .block_task() - .await - .map_err(|err| { - anyhow::anyhow!( - "ACP agent rejected {}: {err}", - AGENT_METHOD_NAMES.session_set_mode - ) - })?; } } Ok(session) } +fn initial_mode_candidates( + config: &AcpProviderConfig, + current_mode: Option, +) -> Vec { + current_mode + .and_then(|mode| config.mode_mapping.get(&mode).cloned()) + .or_else(|| config.session_mode_id.clone().map(|id| vec![id])) + .unwrap_or_default() +} + +fn select_mode_id(candidates: &[String], modes: Option<&SessionModeState>) -> Option { + match modes { + Some(state) => candidates + .iter() + .find(|candidate| { + state + .available_modes + .iter() + .any(|mode| mode.id.0.as_ref() == candidate.as_str()) + }) + .cloned(), + None => candidates.first().cloned(), + } +} + pub fn extension_configs_to_mcp_servers(configs: &[ExtensionConfig]) -> Vec { let mut servers = Vec::new(); @@ -1537,11 +1568,13 @@ fn resolve_model_info( } fn reverse_mode_mapping( - mode_mapping: &HashMap, + mode_mapping: &HashMap>, ) -> HashMap> { let mut reverse: HashMap> = HashMap::new(); - for (mode, id) in mode_mapping { - reverse.entry(id.clone()).or_default().push(*mode); + for (mode, ids) in mode_mapping { + for id in ids { + reverse.entry(id.clone()).or_default().push(*mode); + } } reverse } @@ -1575,7 +1608,9 @@ fn permission_decision_from_mode(goose_mode: GooseMode) -> Option &str { @@ -1811,6 +1846,195 @@ 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_mode_candidates_empty_when_mode_negotiation_disabled(mode: GooseMode) { + let config = test_acp_config(HashMap::new(), None); + assert!(initial_mode_candidates(&config, Some(mode)).is_empty()); + } + + #[test] + fn initial_mode_candidates_prefer_mapping_then_fallback() { + let mapping = HashMap::from([(GooseMode::Auto, vec!["bypassPermissions".to_string()])]); + let config = test_acp_config(mapping, Some("default".to_string())); + + assert_eq!( + initial_mode_candidates(&config, Some(GooseMode::Auto)), + vec!["bypassPermissions".to_string()] + ); + assert_eq!( + initial_mode_candidates(&config, Some(GooseMode::Chat)), + vec!["default".to_string()] + ); + } + + fn mode_state(current: &str, available: &[&str]) -> SessionModeState { + SessionModeState::new( + SessionModeId::new(current), + available + .iter() + .map(|id| SessionMode::new(SessionModeId::new(*id), *id)) + .collect(), + ) + } + + #[test_case( + &["full-access", "agent-full-access"], + &["read-only", "auto", "full-access"], + Some("full-access") + ; "zed era ids" + )] + #[test_case( + &["full-access", "agent-full-access"], + &["read-only", "agent", "agent-full-access"], + Some("agent-full-access") + ; "agentclientprotocol era ids" + )] + #[test_case( + &["full-access", "agent-full-access"], + &["something-else"], + None + ; "no candidate offered" + )] + fn select_mode_id_picks_first_offered_candidate( + candidates: &[&str], + available: &[&str], + expected: Option<&str>, + ) { + let candidates: Vec = candidates.iter().map(|s| s.to_string()).collect(); + let modes = mode_state(available[0], available); + assert_eq!( + select_mode_id(&candidates, Some(&modes)), + expected.map(|s| s.to_string()) + ); + } + + #[test] + fn select_mode_id_first_candidate_when_agent_has_no_modes() { + let candidates = vec!["full-access".to_string(), "agent-full-access".to_string()]; + assert_eq!( + select_mode_id(&candidates, None), + Some("full-access".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, vec!["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); + } + + #[tokio::test] + async fn update_mode_sends_candidate_offered_by_agent() { + let (tx, mut rx) = mpsc::channel(1); + let (mut provider, _) = test_provider_with_tx(Some(tx)); + provider.mode_mapping = HashMap::from([( + GooseMode::Auto, + vec!["full-access".to_string(), "agent-full-access".to_string()], + )]); + provider.session.response = NewSessionResponse::new("test-session").modes(mode_state( + "read-only", + &["read-only", "agent", "agent-full-access"], + )); + + let handle = tokio::spawn(async move { + provider + .update_mode("session", GooseMode::Auto) + .await + .unwrap(); + provider + }); + + match rx.recv().await.expect("expected a SetMode request") { + ClientRequest::SetMode { + mode_id, + response_tx, + .. + } => { + assert_eq!(mode_id, "agent-full-access"); + let _ = response_tx.send(Ok(())); + } + _ => panic!("unexpected request kind"), + } + + let provider = handle.await.unwrap(); + assert_eq!(*provider.goose_mode.lock().unwrap(), GooseMode::Auto); + } + + #[tokio::test] + async fn update_mode_errors_when_no_candidate_offered() { + let (tx, mut rx) = mpsc::channel(1); + let (mut provider, _) = test_provider_with_tx(Some(tx)); + provider.mode_mapping = HashMap::from([(GooseMode::Chat, vec!["read-only".to_string()])]); + provider.session.response = NewSessionResponse::new("test-session") + .modes(mode_state("agent", &["agent", "agent-full-access"])); + + let result = provider.update_mode("session", GooseMode::Chat).await; + + assert!(result.is_err()); + assert!(rx.try_recv().is_err()); + assert_eq!(*provider.goose_mode.lock().unwrap(), GooseMode::Auto); + } + #[test] fn messages_to_prompt_includes_all_prior_handoff_context() { let messages = vec![ @@ -1935,10 +2159,10 @@ mod tests { #[test_case( HashMap::from([ - (GooseMode::Auto, "yolo".to_string()), - (GooseMode::Approve, "default".to_string()), - (GooseMode::SmartApprove, "auto_edit".to_string()), - (GooseMode::Chat, "plan".to_string()), + (GooseMode::Auto, vec!["yolo".to_string()]), + (GooseMode::Approve, vec!["default".to_string()]), + (GooseMode::SmartApprove, vec!["auto_edit".to_string()]), + (GooseMode::Chat, vec!["plan".to_string()]), ]), HashMap::from([ ("yolo".to_string(), vec![GooseMode::Auto]), @@ -1950,10 +2174,10 @@ mod tests { )] #[test_case( HashMap::from([ - (GooseMode::Auto, "bypassPermissions".to_string()), - (GooseMode::Approve, "default".to_string()), - (GooseMode::SmartApprove, "acceptEdits".to_string()), - (GooseMode::Chat, "plan".to_string()), + (GooseMode::Auto, vec!["bypassPermissions".to_string()]), + (GooseMode::Approve, vec!["default".to_string()]), + (GooseMode::SmartApprove, vec!["acceptEdits".to_string()]), + (GooseMode::Chat, vec!["plan".to_string()]), ]), HashMap::from([ ("bypassPermissions".to_string(), vec![GooseMode::Auto]), @@ -1965,20 +2189,22 @@ mod tests { )] #[test_case( 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()), + (GooseMode::Auto, vec!["full-access".to_string(), "agent-full-access".to_string()]), + (GooseMode::Approve, vec!["read-only".to_string()]), + (GooseMode::SmartApprove, vec!["auto".to_string(), "agent".to_string()]), + (GooseMode::Chat, vec!["read-only".to_string()]), ]), HashMap::from([ ("full-access".to_string(), vec![GooseMode::Auto]), + ("agent-full-access".to_string(), vec![GooseMode::Auto]), ("read-only".to_string(), vec![GooseMode::Approve, GooseMode::Chat]), ("auto".to_string(), vec![GooseMode::SmartApprove]), + ("agent".to_string(), vec![GooseMode::SmartApprove]), ]) - ; "codex duplicate read-only" + ; "codex candidates for both bridge generations" )] fn test_reverse_mode_mapping( - forward: HashMap, + forward: HashMap>, expected: HashMap>, ) { let result = reverse_mode_mapping(&forward); @@ -2035,7 +2261,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 +2293,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/amp_acp.rs b/crates/goose/src/providers/amp_acp.rs index f32c662c3..b3a23f160 100644 --- a/crates/goose/src/providers/amp_acp.rs +++ b/crates/goose/src/providers/amp_acp.rs @@ -62,11 +62,11 @@ impl ProviderDef for AmpAcpProvider { let mode_mapping = HashMap::from([ // "bypass" skips confirmations, closest to autonomous mode. - (GooseMode::Auto, "bypass".to_string()), + (GooseMode::Auto, vec!["bypass".to_string()]), // "default" prompts before risky actions. - (GooseMode::Approve, "default".to_string()), - (GooseMode::SmartApprove, "default".to_string()), - (GooseMode::Chat, "default".to_string()), + (GooseMode::Approve, vec!["default".to_string()]), + (GooseMode::SmartApprove, vec!["default".to_string()]), + (GooseMode::Chat, vec!["default".to_string()]), ]); let provider_config = AcpProviderConfig { @@ -76,7 +76,7 @@ impl ProviderDef for AmpAcpProvider { env_remove: vec![], work_dir: working_dir, mcp_servers: extension_configs_to_mcp_servers(&extensions), - session_mode_id: Some(mode_mapping[&goose_mode].clone()), + session_mode_id: mode_mapping[&goose_mode].first().cloned(), session_config_options: vec![], model_config_option_id: None, mode_mapping, diff --git a/crates/goose/src/providers/claude_acp.rs b/crates/goose/src/providers/claude_acp.rs index 46efe7d53..d01ae6965 100644 --- a/crates/goose/src/providers/claude_acp.rs +++ b/crates/goose/src/providers/claude_acp.rs @@ -63,13 +63,13 @@ impl ProviderDef for ClaudeAcpProvider { let mode_mapping = HashMap::from([ // Closest to "autonomous": bypassPermissions skips confirmations. - (GooseMode::Auto, "bypassPermissions".to_string()), + (GooseMode::Auto, vec!["bypassPermissions".to_string()]), // Claude Code's default matches "ask before risky actions". - (GooseMode::Approve, "default".to_string()), + (GooseMode::Approve, vec!["default".to_string()]), // acceptEdits auto-accepts file edits but still prompts for risky ops. - (GooseMode::SmartApprove, "acceptEdits".to_string()), + (GooseMode::SmartApprove, vec!["acceptEdits".to_string()]), // Plan mode disables tool execution, aligning with chat-only intent. - (GooseMode::Chat, "plan".to_string()), + (GooseMode::Chat, vec!["plan".to_string()]), ]); let provider_config = AcpProviderConfig { @@ -80,7 +80,7 @@ impl ProviderDef for ClaudeAcpProvider { env_remove: vec!["CLAUDECODE".to_string()], work_dir: working_dir, mcp_servers: extension_configs_to_mcp_servers(&extensions), - session_mode_id: Some(mode_mapping[&goose_mode].clone()), + session_mode_id: mode_mapping[&goose_mode].first().cloned(), session_config_options: vec![], model_config_option_id: None, mode_mapping, diff --git a/crates/goose/src/providers/codex_acp.rs b/crates/goose/src/providers/codex_acp.rs index e61eb8faf..93502a71c 100644 --- a/crates/goose/src/providers/codex_acp.rs +++ b/crates/goose/src/providers/codex_acp.rs @@ -83,12 +83,17 @@ 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()), + ( + GooseMode::Auto, + vec!["full-access".to_string(), "agent-full-access".to_string()], + ), + ( + GooseMode::SmartApprove, + vec!["auto".to_string(), "agent".to_string()], + ), + (GooseMode::Approve, vec!["read-only".to_string()]), + (GooseMode::Chat, vec!["read-only".to_string()]), ]); let provider_config = AcpProviderConfig { @@ -98,7 +103,6 @@ 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. session_mode_id: None, session_config_options: vec![], model_config_option_id: None, diff --git a/crates/goose/src/providers/copilot_acp.rs b/crates/goose/src/providers/copilot_acp.rs index 8685994ed..e9f38f40a 100644 --- a/crates/goose/src/providers/copilot_acp.rs +++ b/crates/goose/src/providers/copilot_acp.rs @@ -77,10 +77,10 @@ impl ProviderDef for CopilotAcpProvider { // Copilot modes are full protocol URIs. // No approve-specific mode; permissions are handled separately. let mode_mapping = HashMap::from([ - (GooseMode::Auto, MODE_AGENT.to_string()), - (GooseMode::Approve, MODE_AGENT.to_string()), - (GooseMode::SmartApprove, MODE_AGENT.to_string()), - (GooseMode::Chat, MODE_PLAN.to_string()), + (GooseMode::Auto, vec![MODE_AGENT.to_string()]), + (GooseMode::Approve, vec![MODE_AGENT.to_string()]), + (GooseMode::SmartApprove, vec![MODE_AGENT.to_string()]), + (GooseMode::Chat, vec![MODE_PLAN.to_string()]), ]); let provider_config = AcpProviderConfig { @@ -90,7 +90,7 @@ impl ProviderDef for CopilotAcpProvider { env_remove: vec![], work_dir: working_dir, mcp_servers: extension_configs_to_mcp_servers(&extensions), - session_mode_id: Some(mode_mapping[&goose_mode].clone()), + session_mode_id: mode_mapping[&goose_mode].first().cloned(), session_config_options, model_config_option_id: Some("model".to_string()), mode_mapping, diff --git a/crates/goose/src/providers/pi_acp.rs b/crates/goose/src/providers/pi_acp.rs index ff249dcee..38d8b16f2 100644 --- a/crates/goose/src/providers/pi_acp.rs +++ b/crates/goose/src/providers/pi_acp.rs @@ -60,10 +60,10 @@ impl ProviderDef for PiAcpProvider { let goose_mode = config.get_goose_mode().unwrap_or(GooseMode::Auto); let mode_mapping = HashMap::from([ - (GooseMode::Auto, "auto".to_string()), - (GooseMode::Approve, "approve".to_string()), - (GooseMode::SmartApprove, "smart-approve".to_string()), - (GooseMode::Chat, "chat".to_string()), + (GooseMode::Auto, vec!["auto".to_string()]), + (GooseMode::Approve, vec!["approve".to_string()]), + (GooseMode::SmartApprove, vec!["smart-approve".to_string()]), + (GooseMode::Chat, vec!["chat".to_string()]), ]); let provider_config = AcpProviderConfig { @@ -73,7 +73,7 @@ impl ProviderDef for PiAcpProvider { env_remove: vec![], work_dir: working_dir, mcp_servers: extension_configs_to_mcp_servers(&extensions), - session_mode_id: Some(mode_mapping[&goose_mode].clone()), + session_mode_id: mode_mapping[&goose_mode].first().cloned(), session_config_options: vec![], model_config_option_id: None, mode_mapping, diff --git a/crates/goose/tests/acp_fixtures/provider.rs b/crates/goose/tests/acp_fixtures/provider.rs index 595d9f002..eecd83ce7 100644 --- a/crates/goose/tests/acp_fixtures/provider.rs +++ b/crates/goose/tests/acp_fixtures/provider.rs @@ -189,7 +189,7 @@ impl Connection for AcpProviderConnection { .iter() .map(|v| { let mode = GooseMode::from_str(v).unwrap(); - (mode, mode.to_string()) + (mode, vec![mode.to_string()]) }) .collect(), notification_callback: Some(Arc::new(move |n| {