diff --git a/crates/goose/src/acp/provider.rs b/crates/goose/src/acp/provider.rs index b39b60cc6c..94c51a0dae 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,10 @@ 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, + /// Candidate ACP mode ids per goose mode, in preference order. The first + /// id the agent actually offers is used, letting one mapping cover agent + /// versions that advertise different ids for the same behavior. + pub mode_mapping: HashMap>, pub notification_callback: Option>, } @@ -142,7 +145,7 @@ struct HandoffContextClaim { pub struct AcpProvider { name: String, goose_mode: Arc>, - mode_mapping: HashMap, + mode_mapping: HashMap>, session: AcpSession, @@ -421,7 +424,14 @@ impl Provider for AcpProvider { async fn update_mode(&self, session_id: &str, mode: GooseMode) -> Result<(), ProviderError> { // An unmapped mode means the provider opted out of ACP mode negotiation; // still track it locally since it drives permission routing. - if let Some(mode_str) = self.mode_mapping.get(&mode).cloned() { + 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 @@ -1212,50 +1222,69 @@ async fn apply_session_mode( session: NewSessionResponse, ) -> Result { let current_mode = goose_mode.lock().ok().map(|mode| *mode); - let requested_mode_id = initial_session_mode_id(config, current_mode); + 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) } -// None means the provider opted out of mode negotiation; no session/set_mode is sent. -fn initial_session_mode_id( +// Empty means the provider opted out of mode negotiation; no session/set_mode is sent. +fn initial_mode_candidates( config: &AcpProviderConfig, current_mode: Option, -) -> Option { +) -> Vec { current_mode .and_then(|mode| config.mode_mapping.get(&mode).cloned()) - .or_else(|| config.session_mode_id.clone()) + .or_else(|| config.session_mode_id.clone().map(|id| vec![id])) + .unwrap_or_default() +} + +/// Picks the first candidate id the agent offers. Agents that don't advertise +/// modes can't be filtered against, so the first candidate is used as-is. +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 { @@ -1547,11 +1576,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 } @@ -1822,7 +1853,7 @@ mod tests { } fn test_acp_config( - mode_mapping: HashMap, + mode_mapping: HashMap>, session_mode_id: Option, ) -> AcpProviderConfig { AcpProviderConfig { @@ -1844,23 +1875,78 @@ mod tests { #[test_case(GooseMode::Approve)] #[test_case(GooseMode::SmartApprove)] #[test_case(GooseMode::Chat)] - fn initial_session_mode_id_none_when_mode_negotiation_disabled(mode: GooseMode) { + fn initial_mode_candidates_empty_when_mode_negotiation_disabled(mode: GooseMode) { let config = test_acp_config(HashMap::new(), None); - assert_eq!(initial_session_mode_id(&config, Some(mode)), None); + assert!(initial_mode_candidates(&config, Some(mode)).is_empty()); } #[test] - fn initial_session_mode_id_prefers_mapping_then_fallback() { - let mapping = HashMap::from([(GooseMode::Auto, "bypassPermissions".to_string())]); + 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_session_mode_id(&config, Some(GooseMode::Auto)), - Some("bypassPermissions".to_string()) + initial_mode_candidates(&config, Some(GooseMode::Auto)), + vec!["bypassPermissions".to_string()] ); assert_eq!( - initial_session_mode_id(&config, Some(GooseMode::Chat)), - Some("default".to_string()) + initial_mode_candidates(&config, Some(GooseMode::Chat)), + vec!["default".to_string()] + ); + } + + fn mode_state(current: &str, available: &[&str]) -> SessionModeState { + SessionModeState::new( + agent_client_protocol::schema::SessionModeId::new(current), + available + .iter() + .map(|id| { + agent_client_protocol::schema::SessionMode::new( + agent_client_protocol::schema::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()) ); } @@ -1882,7 +1968,7 @@ mod tests { 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())]); + provider.mode_mapping = HashMap::from([(GooseMode::Chat, vec!["plan".to_string()])]); let handle = tokio::spawn(async move { provider @@ -1908,6 +1994,58 @@ mod tests { 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![ @@ -2032,10 +2170,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]), @@ -2047,10 +2185,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]), @@ -2062,20 +2200,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]), ]) - ; "duplicate read-only ids collect both modes" + ; "codex candidates for both bridge generations" )] fn test_reverse_mode_mapping( - forward: HashMap, + forward: HashMap>, expected: HashMap>, ) { let result = reverse_mode_mapping(&forward); diff --git a/crates/goose/src/providers/amp_acp.rs b/crates/goose/src/providers/amp_acp.rs index f32c662c33..b3a23f160d 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 46efe7d538..d01ae69653 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 033b6fcf7b..8fa08875db 100644 --- a/crates/goose/src/providers/codex_acp.rs +++ b/crates/goose/src/providers/codex_acp.rs @@ -62,9 +62,7 @@ impl ProviderDef for CodexAcpProvider { let goose_mode = config.get_goose_mode().unwrap_or(GooseMode::Auto); let mcp_servers = extension_configs_to_mcp_servers(&extensions); - // 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). + // -c overrides pin the initial behavior at spawn. let (approval_policy, sandbox_mode) = map_goose_mode(goose_mode); let mut args = vec![ "-c".to_string(), @@ -85,6 +83,23 @@ impl ProviderDef for CodexAcpProvider { ]); } + // Mode ids differ across codex-acp bridges: @zed-industries <=0.16 + // advertises full-access/auto, @agentclientprotocol >=1.0 renamed + // them to agent-full-access/agent. List both; whichever the agent + // offers is used. + let mode_mapping = HashMap::from([ + ( + 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 { command: resolved_command, args, @@ -92,11 +107,10 @@ impl ProviderDef for CodexAcpProvider { env_remove: vec![], work_dir: working_dir, mcp_servers, - // Opt out of session/set_mode; the -c args above pin behavior. session_mode_id: None, session_config_options: vec![], model_config_option_id: None, - mode_mapping: HashMap::new(), + mode_mapping, notification_callback: None, }; diff --git a/crates/goose/src/providers/copilot_acp.rs b/crates/goose/src/providers/copilot_acp.rs index 8685994ed0..e9f38f40ae 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 ff249dcee7..38d8b16f2f 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 595d9f0028..eecd83ce76 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| {