From 6a17d035f79ad582ca3f4f3cdc38d376f2c4087f Mon Sep 17 00:00:00 2001 From: Wes Date: Sat, 8 Aug 2026 09:43:44 -0600 Subject: [PATCH] Revert "fix(acp): reject unattended permission requests" (#5323) Reverts block/buzz#4609 --- crates/buzz-acp/src/acp.rs | 193 ++++++++++++++-------------------- crates/buzz-acp/src/config.rs | 51 +++++---- crates/buzz-acp/src/lib.rs | 4 +- crates/buzz-acp/src/pool.rs | 16 +-- 4 files changed, 116 insertions(+), 148 deletions(-) diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index 93109fa94..700d5e8dc 100644 --- a/crates/buzz-acp/src/acp.rs +++ b/crates/buzz-acp/src/acp.rs @@ -155,7 +155,7 @@ pub struct AcpClient { /// a `cancelled` outcome before the agent returns from `session/prompt`. pending_permission_id: Option, /// Whether we have already sent a response to the pending permission request. - /// Guards against double-response if a timeout fires after the rejection + /// Guards against double-response if a timeout fires after the allow_once /// response was written but before `pending_permission_id` was cleared. permission_responded: bool, /// The JSON-RPC id of the most recently sent `session/prompt` request. @@ -1162,8 +1162,7 @@ impl AcpClient { /// /// While waiting, handles: /// - `session/update` notifications → logged via tracing - /// - `session/request_permission` requests → rejected unless an owner has - /// already selected a non-interactive permission mode at session setup + /// - `session/request_permission` requests → auto-approved with `allow_once` /// - Any other messages → debug-logged and ignored; if they carry an `id` /// (i.e. they are requests, not notifications), a JSON-RPC -32601 error is sent. /// @@ -1871,12 +1870,12 @@ impl AcpClient { } } - /// Reject a `session/request_permission` request from the agent. + /// Auto-approve a `session/request_permission` request from the agent. /// - /// Buzz has no human permission prompt in this harness, so selecting - /// `allow_once` would turn any admitted prompt into an implicit approval. - /// Find `reject_once` by kind when the adapter offers it; otherwise use the - /// protocol's cancelled outcome, which is also fail-closed. + /// Finds the option with `kind == "allow_once"` and responds with its `optionId`. + /// If no `allow_once` option exists, falls back to `reject_once`. + /// + /// **Critical:** Never hardcode `optionId` — always find it dynamically by `kind`. /// /// The request `id` is stored as `serde_json::Value` to support both numeric /// and string IDs per JSON-RPC 2.0. @@ -1902,7 +1901,40 @@ impl AcpClient { options.len() ); - let response = permission_denial_response(&id, options)?; + // Find allow_once by kind — NEVER hardcode optionId. + let allow_once = options + .iter() + .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("allow_once")); + + let response = if let Some(opt) = allow_once { + let option_id = opt["optionId"] + .as_str() + .ok_or_else(|| AcpError::Protocol("allow_once option missing optionId".into()))?; + tracing::info!( + target: "acp::permission", + "auto-approving permission id={id} with allow_once optionId={option_id:?}" + ); + permission_response_selected(&id, option_id) + } else { + // No allow_once — fall back to reject_once. + tracing::warn!( + target: "acp::permission", + "no allow_once option found in permission request id={id}, falling back to reject_once" + ); + let reject = options + .iter() + .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("reject_once")); + + if let Some(opt) = reject { + let option_id = opt["optionId"].as_str().unwrap_or("reject"); + permission_response_selected(&id, option_id) + } else { + return Err(AcpError::Protocol( + "no suitable permission option found (neither allow_once nor reject_once)" + .into(), + )); + } + }; // Write the response first, then mark as responded. // @@ -2014,42 +2046,6 @@ fn permission_response_cancelled(id: &serde_json::Value) -> serde_json::Value { }) } -/// Choose the fail-closed response to a `session/request_permission` request. -/// -/// Buzz has no human permission prompt in this harness, so selecting -/// `allow_once` would turn any admitted prompt into an implicit approval. -/// Prefer the adapter's `reject_once` option — matched by `kind`, never by a -/// hardcoded `optionId` — and fall back to the protocol's cancelled outcome for -/// adapters that do not offer one. Both answers deny. -/// -/// Kept free of the client so the decision is testable without an agent -/// subprocess: `AcpClient` owns a real `Child` and its stdio pipes. -fn permission_denial_response( - id: &serde_json::Value, - options: &[serde_json::Value], -) -> Result { - let reject_once = options - .iter() - .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("reject_once")); - - let Some(opt) = reject_once else { - tracing::warn!( - target: "acp::permission", - "no reject_once option found in permission request id={id}, cancelling" - ); - return Ok(permission_response_cancelled(id)); - }; - - let option_id = opt["optionId"] - .as_str() - .ok_or_else(|| AcpError::Protocol("reject_once option missing optionId".into()))?; - tracing::info!( - target: "acp::permission", - "rejecting permission id={id} with reject_once optionId={option_id:?}" - ); - Ok(permission_response_selected(id, option_id)) -} - /// Full `session/new` response — session ID plus the raw JSON result. /// /// Callers use the extractor helpers to pull model info from `raw`. @@ -2304,96 +2300,63 @@ mod tests { assert_eq!(StopReason::from_str("Refusal"), Some(StopReason::Refusal)); } - fn options(json: &str) -> Vec { - serde_json::from_str(json).expect("option list") - } - - fn outcome(response: &serde_json::Value) -> Option<&str> { - response["result"]["outcome"]["outcome"].as_str() - } - - /// The offered `allow_once` and `allow_always` options must be ignored: - /// there is no human to click them, so choosing either would make every - /// admitted prompt an implicit approval. `optionId`s are deliberately - /// non-obvious to prove they are matched by `kind`, never hardcoded. #[test] - fn permission_requests_select_reject_once_not_allow_once() { - let options = options( + fn find_allow_once_by_kind_not_by_option_id() { + // optionId values are intentionally non-obvious to prove we don't hardcode them. + let options: Vec = serde_json::from_str( r#"[ {"optionId": "opt-reject-42", "name": "Reject", "kind": "reject_once"}, {"optionId": "opt-allow-99", "name": "Allow once", "kind": "allow_once"}, {"optionId": "opt-always-7", "name": "Always allow", "kind": "allow_always"} ]"#, - ); + ) + .unwrap(); - let response = - permission_denial_response(&serde_json::json!(7), &options).expect("denial response"); + let allow_once = options + .iter() + .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("allow_once")); - assert_eq!(outcome(&response), Some("selected")); - assert_eq!( - response["result"]["outcome"]["optionId"].as_str(), - Some("opt-reject-42"), - "must select reject_once even when allow options are offered" - ); + assert!(allow_once.is_some(), "should find allow_once option"); + let opt = allow_once.unwrap(); + // Found by kind, not by hardcoded optionId + assert_eq!(opt["kind"].as_str(), Some("allow_once")); + assert_eq!(opt["optionId"].as_str(), Some("opt-allow-99")); } - /// Fail-closed backstop: an adapter that offers no `reject_once` must still - /// be denied, via the protocol's cancelled outcome rather than an error or - /// an approval. #[test] - fn permission_request_without_reject_once_is_cancelled() { - let options = options( + fn find_allow_once_returns_none_when_absent() { + let options: Vec = serde_json::from_str( r#"[ - {"optionId": "opt-allow-99", "name": "Allow once", "kind": "allow_once"}, - {"optionId": "opt-always-7", "name": "Always allow", "kind": "allow_always"} + {"optionId": "reject-1", "name": "Reject", "kind": "reject_once"}, + {"optionId": "reject-always", "name": "Always reject", "kind": "reject_always"} ]"#, - ); + ) + .unwrap(); - let response = permission_denial_response(&serde_json::json!("req-1"), &options) - .expect("cancelled response"); + let allow_once = options + .iter() + .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("allow_once")); - assert_eq!(outcome(&response), Some("cancelled")); - assert_eq!( - response["id"].as_str(), - Some("req-1"), - "string ids must round-trip per JSON-RPC 2.0" - ); - } - - /// An empty option list is the degenerate form of the same backstop. - #[test] - fn permission_request_with_no_options_is_cancelled() { - let response = - permission_denial_response(&serde_json::json!(1), &[]).expect("cancelled response"); - - assert_eq!(outcome(&response), Some("cancelled")); - } - - /// A `reject_once` option missing its `optionId` is a protocol violation. - /// Erroring propagates to the caller, which tears the turn down — still no - /// approval is ever sent. - #[test] - fn reject_once_without_option_id_is_a_protocol_error() { - let options = options(r#"[{"name": "Reject", "kind": "reject_once"}]"#); - - let err = permission_denial_response(&serde_json::json!(1), &options) - .expect_err("missing optionId must error"); - - assert!(matches!(err, AcpError::Protocol(_)), "got {err:?}"); + assert!(allow_once.is_none()); } #[test] - fn find_reject_once_by_kind() { - let options = - options(r#"[{"optionId": "rej-x", "name": "Reject", "kind": "reject_once"}]"#); + fn find_reject_once_fallback_when_no_allow_once() { + let options: Vec = serde_json::from_str( + r#"[{"optionId": "rej-x", "name": "Reject", "kind": "reject_once"}]"#, + ) + .unwrap(); - let response = - permission_denial_response(&serde_json::json!(1), &options).expect("denial response"); + let allow_once = options + .iter() + .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("allow_once")); + assert!(allow_once.is_none()); - assert_eq!( - response["result"]["outcome"]["optionId"].as_str(), - Some("rej-x") - ); + let reject_once = options + .iter() + .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("reject_once")); + assert!(reject_once.is_some()); + assert_eq!(reject_once.unwrap()["optionId"].as_str(), Some("rej-x")); } #[test] diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index d95968584..35aaec188 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -116,6 +116,7 @@ impl std::fmt::Display for RespondTo { /// /// - `default` — agent's built-in behaviour (permission requests per tool call). /// - `acceptEdits` — auto-approve file edits, still ask for other tools. +/// - `bypassPermissions` — skip the permission flow entirely. /// - `dontAsk` — never prompt; reject anything that would require permission. /// - `plan` — planning-only mode (no tool execution). #[derive(Debug, Clone, Copy, PartialEq, clap::ValueEnum)] @@ -126,6 +127,9 @@ pub enum PermissionMode { /// Auto-approve file edits, still ask for other tools. #[value(alias = "acceptEdits")] AcceptEdits, + /// Skip the permission flow entirely. + #[value(alias = "bypassPermissions")] + BypassPermissions, /// Never prompt; reject anything that would require permission. #[value(alias = "dontAsk")] DontAsk, @@ -141,6 +145,7 @@ impl PermissionMode { match self { Self::Default => "default", Self::AcceptEdits => "acceptEdits", + Self::BypassPermissions => "bypassPermissions", Self::DontAsk => "dontAsk", Self::Plan => "plan", } @@ -427,12 +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 `dontAsk`, which rejects operations that need interactive - /// approval because Buzz does not expose a human permission prompt. + /// Defaults to `bypassPermissions` which skips the per-tool-call + /// permission flow. Set to `default` to restore the agent's built-in + /// behaviour. #[arg( long, env = "BUZZ_ACP_PERMISSION_MODE", - default_value = "dont-ask", + default_value = "bypass-permissions", value_enum )] pub permission_mode: PermissionMode, @@ -1463,7 +1469,7 @@ mod tests { memory_enabled: true, model: None, session_title: None, - permission_mode: PermissionMode::DontAsk, + permission_mode: PermissionMode::BypassPermissions, respond_to: RespondTo::Anyone, respond_to_allowlist: HashSet::new(), allowed_respond_to: Vec::new(), @@ -2264,6 +2270,10 @@ channels = "ALL" fn test_permission_mode_wire_strings() { assert_eq!(PermissionMode::Default.as_wire_str(), "default"); assert_eq!(PermissionMode::AcceptEdits.as_wire_str(), "acceptEdits"); + assert_eq!( + PermissionMode::BypassPermissions.as_wire_str(), + "bypassPermissions" + ); assert_eq!(PermissionMode::DontAsk.as_wire_str(), "dontAsk"); assert_eq!(PermissionMode::Plan.as_wire_str(), "plan"); } @@ -2271,6 +2281,7 @@ channels = "ALL" #[test] fn test_permission_mode_is_default() { assert!(PermissionMode::Default.is_default()); + assert!(!PermissionMode::BypassPermissions.is_default()); assert!(!PermissionMode::AcceptEdits.is_default()); assert!(!PermissionMode::DontAsk.is_default()); assert!(!PermissionMode::Plan.is_default()); @@ -2278,17 +2289,20 @@ channels = "ALL" #[test] fn test_permission_mode_display() { - assert_eq!(format!("{}", PermissionMode::DontAsk), "dontAsk"); + assert_eq!( + format!("{}", PermissionMode::BypassPermissions), + "bypassPermissions" + ); assert_eq!(format!("{}", PermissionMode::Default), "default"); } #[test] fn test_summary_includes_permission_mode() { let mut config = test_config(SubscribeMode::Mentions); - config.permission_mode = PermissionMode::DontAsk; + config.permission_mode = PermissionMode::BypassPermissions; let s = config.summary(); assert!( - s.contains("permission_mode=dontAsk"), + s.contains("permission_mode=bypassPermissions"), "summary should include permission_mode, got: {s}" ); } @@ -2305,9 +2319,9 @@ channels = "ALL" } #[test] - fn test_default_config_rejects_interactive_permissions() { + fn test_default_config_uses_bypass_permissions() { let config = test_config(SubscribeMode::Mentions); - assert_eq!(config.permission_mode, PermissionMode::DontAsk); + assert_eq!(config.permission_mode, PermissionMode::BypassPermissions); } #[test] @@ -2318,6 +2332,7 @@ channels = "ALL" let cases = [ ("default", PermissionMode::Default), ("accept-edits", PermissionMode::AcceptEdits), + ("bypass-permissions", PermissionMode::BypassPermissions), ("dont-ask", PermissionMode::DontAsk), ("plan", PermissionMode::Plan), ]; @@ -2332,12 +2347,14 @@ channels = "ALL" #[test] fn test_permission_mode_value_enum_camel_case_aliases() { - // Operators may set env vars using the camelCase wire-format strings. - // The #[value(alias)] attributes ensure these parse correctly. + // Operators may set env vars using the camelCase wire-format strings + // (e.g. BUZZ_ACP_PERMISSION_MODE=bypassPermissions). The #[value(alias)] + // attributes ensure these parse correctly. use clap::ValueEnum; let cases = [ ("default", PermissionMode::Default), ("acceptEdits", PermissionMode::AcceptEdits), + ("bypassPermissions", PermissionMode::BypassPermissions), ("dontAsk", PermissionMode::DontAsk), ("plan", PermissionMode::Plan), ]; @@ -2350,18 +2367,6 @@ channels = "ALL" } } - #[test] - fn test_permission_mode_rejects_unattended_bypass() { - use clap::ValueEnum; - - for input in ["bypass-permissions", "bypassPermissions"] { - assert!( - PermissionMode::from_str(input, true).is_err(), - "{input:?} must not disable the ACP permission boundary" - ); - } - } - /// Helper: resolve idle_timeout_secs using the same precedence logic as Config::from_args. /// Precedence: explicit --idle-timeout > --turn-timeout (deprecated) > `DEFAULT_IDLE_TIMEOUT_SECS`. fn resolve_idle_timeout(idle: Option, turn: Option) -> u64 { diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index 65c9dd620..b69a1f453 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -6199,7 +6199,7 @@ mod build_mcp_servers_tests { memory_enabled: false, model: None, session_title: None, - permission_mode: config::PermissionMode::DontAsk, + permission_mode: config::PermissionMode::BypassPermissions, respond_to: config::RespondTo::Anyone, respond_to_allowlist: std::collections::HashSet::new(), allowed_respond_to: vec![], @@ -6421,7 +6421,7 @@ mod error_outcome_emission_tests { memory_enabled: false, model: None, session_title: None, - permission_mode: config::PermissionMode::DontAsk, + permission_mode: config::PermissionMode::BypassPermissions, respond_to: config::RespondTo::Anyone, respond_to_allowlist: HashSet::new(), allowed_respond_to: vec![], diff --git a/crates/buzz-acp/src/pool.rs b/crates/buzz-acp/src/pool.rs index 8430307d9..ddc0330d9 100644 --- a/crates/buzz-acp/src/pool.rs +++ b/crates/buzz-acp/src/pool.rs @@ -1017,7 +1017,7 @@ async fn create_session_and_apply_model( // Apply permission mode if not the agent's built-in default AND the agent // 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 rejects interactive permission requests. + // 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()) { @@ -1130,7 +1130,11 @@ async fn apply_model_switch( Ok(()) } -/// Check whether the agent's `session/new` response advertises a given mode ID +/// 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. fn agent_supports_mode(session_new_result: &serde_json::Value, mode_wire: &str) -> bool { @@ -1146,11 +1150,7 @@ fn agent_supports_mode(session_new_result: &serde_json::Value, mode_wire: &str) .unwrap_or(false) } -/// 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 mode, and any interactive permission request is rejected by -/// `handle_permission_request`. +/// per-tool auto-approval in `handle_permission_request`. /// /// **Fatal exception:** if the agent process exits (e.g., goose crashes on /// unrecognized methods), returns `Err(AgentExited)` so the caller can respawn. @@ -1190,7 +1190,7 @@ async fn apply_permission_mode( Ok(Err(e)) => { tracing::warn!( target: "pool::permission", - "failed to set permission mode {wire:?}: {e} — falling back to per-tool rejection" + "failed to set permission mode {wire:?}: {e} — falling back to per-tool auto-approval" ); } Err(_) => {