From 17a480daff08a335537a6906b446b3457d8ccb58 Mon Sep 17 00:00:00 2001 From: KC <79471844+wolfyy970@users.noreply.github.com> Date: Sat, 8 Aug 2026 11:33:45 -0400 Subject: [PATCH] fix(acp): preserve permission requests by default Signed-off-by: KC <79471844+wolfyy970@users.noreply.github.com> --- crates/buzz-acp/src/acp.rs | 60 +++++++++++++++++++++++++++++++++++ crates/buzz-acp/src/config.rs | 27 ++++++++++------ crates/buzz-acp/src/pool.rs | 48 +++++++++++++++++++++++----- 3 files changed, 118 insertions(+), 17 deletions(-) diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index 700d5e8dcf..0926e1c2a5 100644 --- a/crates/buzz-acp/src/acp.rs +++ b/crates/buzz-acp/src/acp.rs @@ -2359,6 +2359,66 @@ mod tests { assert_eq!(reject_once.unwrap()["optionId"].as_str(), Some("rej-x")); } + /// Exercise the production prompt read loop rather than duplicating its + /// option-selection logic. In the default adapter mode, the adapter asks + /// for each permission and Buzz must answer that exact request before the + /// prompt can complete. + #[cfg(unix)] + #[tokio::test] + async fn prompt_loop_answers_allow_once_permission_on_wire() { + let capture = std::env::temp_dir().join(format!( + "buzz-acp-default-mode-permission-{}.json", + uuid::Uuid::new_v4() + )); + let script = format!( + "read -r _prompt; \ + printf '%s\\n' '{request}'; \ + read -r decision; \ + printf '%s' \"$decision\" > \"$1\"; \ + printf '%s\\n' '{result}'; \ + sleep 1", + request = r#"{"jsonrpc":"2.0","id":"permission-7","method":"session/request_permission","params":{"options":[{"optionId":"deny-1","name":"Deny","kind":"reject_once"},{"optionId":"grant-7","name":"Allow once","kind":"allow_once"}]}}"#, + result = r#"{"jsonrpc":"2.0","id":0,"result":{"stopReason":"end_turn"}}"#, + ); + let mut client = AcpClient::spawn( + "bash", + &[ + "-c".into(), + script, + "buzz-acp-permission-test".into(), + capture.to_string_lossy().into_owned(), + ], + &[], + false, + ) + .await + .expect("spawn permission test adapter"); + + let stop = client + .session_prompt_with_idle_timeout( + "session-1", + "reply", + std::time::Duration::from_secs(2), + std::time::Duration::from_secs(5), + ) + .await + .expect("permission response should let the prompt complete"); + assert_eq!(stop, StopReason::EndTurn); + + let written = std::fs::read_to_string(&capture).expect("captured permission response"); + let response: serde_json::Value = + serde_json::from_str(&written).expect("permission response is valid JSON"); + assert_eq!(response["id"].as_str(), Some("permission-7")); + assert_eq!(response["result"]["outcome"]["outcome"], "selected"); + assert_eq!( + response["result"]["outcome"]["optionId"].as_str(), + Some("grant-7") + ); + + client.shutdown().await; + std::fs::remove_file(capture).expect("remove permission capture"); + } + #[test] fn request_has_id_field() { let id: u64 = 42; diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index 35aaec188d..3ec631e65f 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -432,13 +432,13 @@ pub struct CliArgs { /// Permission mode for agents that support `session/set_config_option` /// with `configId: "mode"` (e.g. `claude-agent-acp`). /// - /// Defaults to `bypassPermissions` which skips the per-tool-call - /// permission flow. Set to `default` to restore the agent's built-in - /// behaviour. + /// Defaults to `default`, which preserves the per-tool permission flow so + /// the harness can observe and answer each request. Operators can still + /// select `bypassPermissions` explicitly for legacy unattended behavior. #[arg( long, env = "BUZZ_ACP_PERMISSION_MODE", - default_value = "bypass-permissions", + default_value = "default", value_enum )] pub permission_mode: PermissionMode, @@ -1435,7 +1435,7 @@ fn rule_applies_to_channel(rule: &SubscriptionRule, channel_id: Uuid) -> bool { mod tests { use super::*; use crate::filter::{ChannelScope, SubscriptionRule}; - use clap::{Parser, ValueEnum}; + use clap::{CommandFactory, Parser, ValueEnum}; /// Build a minimal Config for testing without CLI parsing. fn test_config(mode: SubscribeMode) -> Config { @@ -1469,7 +1469,7 @@ mod tests { memory_enabled: true, model: None, session_title: None, - permission_mode: PermissionMode::BypassPermissions, + permission_mode: PermissionMode::Default, respond_to: RespondTo::Anyone, respond_to_allowlist: HashSet::new(), allowed_respond_to: Vec::new(), @@ -2319,9 +2319,18 @@ channels = "ALL" } #[test] - fn test_default_config_uses_bypass_permissions() { - let config = test_config(SubscribeMode::Mentions); - assert_eq!(config.permission_mode, PermissionMode::BypassPermissions); + fn test_default_config_preserves_permission_requests() { + let command = CliArgs::command(); + let permission_mode = command + .get_arguments() + .find(|arg| arg.get_id() == "permission_mode") + .expect("permission-mode argument"); + let defaults: Vec<_> = permission_mode + .get_default_values() + .iter() + .map(|value| value.to_string_lossy()) + .collect(); + assert_eq!(defaults, ["default"]); } #[test] diff --git a/crates/buzz-acp/src/pool.rs b/crates/buzz-acp/src/pool.rs index ddc0330d9f..51dcd13bb7 100644 --- a/crates/buzz-acp/src/pool.rs +++ b/crates/buzz-acp/src/pool.rs @@ -1018,9 +1018,7 @@ async fn create_session_and_apply_model( // advertises the requested mode in session/new. Agents that don't support // the mode (e.g., goose crashes on unrecognized set_config_option values) // are safely skipped — the harness auto-approves via handle_permission_request. - if !ctx.permission_mode.is_default() - && agent_supports_mode(&resp.raw, ctx.permission_mode.as_wire_str()) - { + if should_apply_permission_mode(&resp.raw, &ctx.permission_mode) { apply_permission_mode(&mut agent.acp, &resp.session_id, &ctx.permission_mode).await?; } @@ -1130,10 +1128,6 @@ async fn apply_model_switch( Ok(()) } -/// Set the session permission mode via `session/set_config_option`. -/// -/// Non-fatal for most errors: logs and proceeds. The agent falls back -/// to its default permission mode (`"default"`), which still works via /// Check if the agent's `session/new` response advertises a given mode ID /// in `result.modes.availableModes[].id`. Returns `false` if the modes /// field is absent or the mode isn't listed. @@ -1150,7 +1144,20 @@ fn agent_supports_mode(session_new_result: &serde_json::Value, mode_wire: &str) .unwrap_or(false) } -/// per-tool auto-approval in `handle_permission_request`. +/// Decide whether Buzz should override the adapter's session permission mode. +/// The default deliberately leaves the adapter untouched so its individual +/// permission requests reach the harness. +fn should_apply_permission_mode( + session_new_result: &serde_json::Value, + mode: &PermissionMode, +) -> bool { + !mode.is_default() && agent_supports_mode(session_new_result, mode.as_wire_str()) +} + +/// Set the session permission mode via `session/set_config_option`. +/// +/// Non-fatal for application-level errors: the agent remains in its default +/// mode and the harness handles its per-tool permission requests. /// /// **Fatal exception:** if the agent process exits (e.g., goose crashes on /// unrecognized methods), returns `Err(AgentExited)` so the caller can respawn. @@ -4039,6 +4046,31 @@ mod tests { } } + #[test] + fn default_permission_mode_never_sends_a_session_override() { + let session = json!({ + "modes": { + "availableModes": [ + {"id": "default"}, + {"id": "bypassPermissions"} + ] + } + }); + + assert!(!should_apply_permission_mode( + &session, + &PermissionMode::Default + )); + assert!(should_apply_permission_mode( + &session, + &PermissionMode::BypassPermissions + )); + assert!(!should_apply_permission_mode( + &json!({}), + &PermissionMode::BypassPermissions + )); + } + #[test] fn public_session_forwards_channel_origin_to_mcp() { let channel_id = Uuid::new_v4();