vrr/goose
2
0
Fork 0
mirror of https://github.com/block/goose.git synced 2026-07-09 16:09:22 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 22

fix(acp): stop sending session/set_mode for codex-acp

Browse source 
codex-acp opted out of ACP mode negotiation (session_mode_id: None),
pinning behavior via -c approval_policy/sandbox_mode args instead. But
apply_session_mode consulted mode_mapping before that fallback, so
codex's populated map re-enabled the set_mode path with mode ids
hardcoded from @zed-industries/codex-acp <=0.16. The renamed ids in
@agentclientprotocol/codex-acp >=1.0 (agent/agent-full-access) then
made provider creation hard-fail with "Requested mode 'full-access'
not offered by agent" for GOOSE_MODE auto and smart_approve.

Make the opt-out actually opt out by emptying codex's mode_mapping:
with no map entry and no session_mode_id, goose never sends
session/set_mode, so one build works against both bridge generations.
update_mode now also honors a missing mapping by skipping the outbound
set_mode/set_config_option call (replacing the never-valid
format!("{mode:?}") fallback id) while still tracking the mode
locally for permission routing. The "not offered by agent" error
stays intact for providers that do request modes, and claude, copilot,
amp, and pi keep their complete mappings and behavior.

Also extracts initial_session_mode_id for the new-session id selection,
adds tests pinning the codex opt-out and the update_mode skip/send
paths, and renames the reverse-mapping test fixtures that attributed
the duplicate read-only map to codex, which no longer ships one.

Verified via the Berd WebSocket replay: session/set_config_option
{provider: codex-acp} with GOOSE_MODE auto succeeds against both
@agentclientprotocol/codex-acp 1.1.0 and @zed-industries/codex-acp
0.16.0, with wire logs showing no session/set_mode sent; the 0.16.0
bridge honors the -c args and starts in full-access on its own.

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 15:55:16 +10:00
parent e09289cbac
commit d8cdc8a60b
2 changed files with 128 additions and 31 deletions
Download patch file Download diff file Expand all files Collapse all files

140
crates/goose/src/acp/provider.rs
View file

@ -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<NewSessionResponse> {
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<GooseMode>,
) -> Option<String> {
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<McpServer> {
let mut servers = Vec::new();
@ -1811,6 +1824,93 @@ mod tests {
assert!(rx.try_recv().is_err());
}
fn test_acp_config(
mode_mapping: HashMap<GooseMode, String>,
session_mode_id: Option<String>,
) -> 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<GooseMode, String>,
@ -2035,7 +2135,7 @@ mod tests {
resolve_model_info("test", &response)
}
fn codex_reverse_modes() -> HashMap<String, Vec<GooseMode>> {
fn duplicate_read_only_reverse_modes() -> HashMap<String, Vec<GooseMode>> {
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<GooseMode>) {
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, &current);
if mode_id == "read-only" && expected == Some(GooseMode::Approve) {

19
crates/goose/src/providers/codex_acp.rs
View file

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