fix(acp): pick session mode from the ids the agent actually offers

The codex-acp bridge exists in two generations that advertise different
session mode ids for the same behavior: @zed-industries/codex-acp <=0.16
offers full-access/auto/read-only while @agentclientprotocol/codex-acp
>=1.0 renamed them to agent-full-access/agent/read-only. Goose's
hardcoded single-id mapping could only match one generation, producing
"Requested mode 'full-access' not offered by agent. Available modes:
read-only, agent, agent-full-access" on the other. The previous fix
(4c358e2e1) sidestepped this by opting codex out of mode negotiation
entirely, which left mid-session GOOSE_MODE switches as local-only
no-ops on the wire.

Replace the opt-out with availability-based selection: mode_mapping now
holds candidate ids per goose mode in preference order
(HashMap<GooseMode, Vec<String>>), and goose uses the first candidate
the agent lists in available_modes from session/new. Codex maps Auto to
[full-access, agent-full-access] and SmartApprove to [auto, agent], so
one build negotiates modes correctly against either bridge generation,
and mid-session mode switches reach the agent again. update_mode
resolves candidates against the same session state and fails loudly
when a mapped mode has no offered candidate, matching the fail-fast
validation at session creation, which now reports all rejected
candidates. reverse_mode_mapping fans each candidate id back to its
goose mode so CurrentModeUpdate notifications from either generation
sync local permission routing.

claude, copilot, amp, and pi keep their single-id behavior as
one-element candidate lists, agents that advertise no modes state get
the first candidate as before, and codex's -c approval_policy/
sandbox_mode spawn args stay as the initial pin. An empty mapping still
means full opt-out.

Tests cover candidate selection for both codex bridge generations, the
no-candidate error, update_mode sending the offered candidate, and the
expanded reverse mapping. Verified with cargo test -p goose --lib
acp::provider (47 passed) and cargo clippy -p goose --all-targets
-D warnings; remaining suite failures are a pre-existing environmental
sqlx-core runtime-feature panic also present on HEAD.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
This commit is contained in:
Matt Toohey 2026-07-08 17:33:39 +10:00
parent 1638e34480
commit 06d12e0388
7 changed files with 240 additions and 86 deletions

View file

@ -4,7 +4,7 @@ use agent_client_protocol::schema::{
McpServerStdio, NewSessionRequest, NewSessionResponse, PromptRequest, PromptResponse,
ProtocolVersion, RequestPermissionOutcome, RequestPermissionRequest, RequestPermissionResponse,
SessionConfigKind, SessionConfigOption, SessionConfigOptionCategory,
SessionConfigSelectOptions, SessionId, SessionNotification, SessionUpdate,
SessionConfigSelectOptions, SessionId, SessionModeState, SessionNotification, SessionUpdate,
SetSessionConfigOptionRequest, SetSessionModeRequest, SetSessionModeResponse, StopReason,
TextContent, ToolCallContent, ToolCallStatus, ToolKind,
};
@ -57,7 +57,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<String>,
pub mode_mapping: HashMap<GooseMode, String>,
/// 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<GooseMode, Vec<String>>,
pub notification_callback: Option<Arc<dyn Fn(SessionNotification) + Send + Sync>>,
}
@ -141,7 +144,7 @@ struct HandoffContextClaim {
pub struct AcpProvider {
name: String,
goose_mode: Arc<Mutex<GooseMode>>,
mode_mapping: HashMap<GooseMode, String>,
mode_mapping: HashMap<GooseMode, Vec<String>>,
session: AcpSession,
@ -420,7 +423,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<NewSessionResponse> {
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<String> = 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<String> = 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<GooseMode>,
) -> Option<String> {
) -> Vec<String> {
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<String> {
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<McpServer> {
@ -1556,11 +1585,13 @@ fn resolve_model_info(
}
fn reverse_mode_mapping(
mode_mapping: &HashMap<GooseMode, String>,
mode_mapping: &HashMap<GooseMode, Vec<String>>,
) -> HashMap<String, Vec<GooseMode>> {
let mut reverse: HashMap<String, Vec<GooseMode>> = 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
}
@ -1833,7 +1864,7 @@ mod tests {
}
fn test_acp_config(
mode_mapping: HashMap<GooseMode, String>,
mode_mapping: HashMap<GooseMode, Vec<String>>,
session_mode_id: Option<String>,
) -> AcpProviderConfig {
AcpProviderConfig {
@ -1855,23 +1886,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<String> = 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())
);
}
@ -1893,7 +1979,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
@ -1919,6 +2005,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![
@ -2043,10 +2181,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]),
@ -2058,10 +2196,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]),
@ -2073,20 +2211,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<GooseMode, String>,
forward: HashMap<GooseMode, Vec<String>>,
expected: HashMap<String, Vec<GooseMode>>,
) {
let result = reverse_mode_mapping(&forward);

View file

@ -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,

View file

@ -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,

View file

@ -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,
};

View file

@ -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,

View file

@ -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,

View file

@ -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| {