From 55e0e95419eaf4d339476a54473b7b6bb7fce3f2 Mon Sep 17 00:00:00 2001 From: Jordan Mecom Date: Fri, 31 Jul 2026 14:03:48 -0700 Subject: [PATCH 1/3] Reject unattended ACP permission requests Default managed sessions to dontAsk and reject or cancel permission prompts instead of selecting allow_once. Explicit owner-selected non-interactive modes remain available. Co-authored-by: Jordan Mecom Signed-off-by: Jordan Mecom --- crates/buzz-acp/src/acp.rs | 65 ++++++++++++----------------------- crates/buzz-acp/src/config.rs | 13 ++++--- crates/buzz-acp/src/pool.rs | 16 ++++----- 3 files changed, 36 insertions(+), 58 deletions(-) diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index 700d5e8dcf..1feb834e9f 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 allow_once + /// Guards against double-response if a timeout fires after the rejection /// 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,7 +1162,8 @@ impl AcpClient { /// /// While waiting, handles: /// - `session/update` notifications → logged via tracing - /// - `session/request_permission` requests → auto-approved with `allow_once` + /// - `session/request_permission` requests → rejected unless an owner has + /// already selected a non-interactive permission mode at session setup /// - 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. /// @@ -1870,12 +1871,12 @@ impl AcpClient { } } - /// Auto-approve a `session/request_permission` request from the agent. + /// Reject a `session/request_permission` request from the agent. /// - /// 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`. + /// 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. /// /// The request `id` is stored as `serde_json::Value` to support both numeric /// and string IDs per JSON-RPC 2.0. @@ -1901,39 +1902,25 @@ impl AcpClient { options.len() ); - // Find allow_once by kind — NEVER hardcode optionId. - let allow_once = options + let reject_once = options .iter() - .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("allow_once")); + .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("reject_once")); - let response = if let Some(opt) = allow_once { + let response = if let Some(opt) = reject_once { let option_id = opt["optionId"] .as_str() - .ok_or_else(|| AcpError::Protocol("allow_once option missing optionId".into()))?; + .ok_or_else(|| AcpError::Protocol("reject_once option missing optionId".into()))?; tracing::info!( target: "acp::permission", - "auto-approving permission id={id} with allow_once optionId={option_id:?}" + "rejecting permission id={id} with reject_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" + "no reject_once option found in permission request id={id}, cancelling" ); - 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(), - )); - } + permission_response_cancelled(&id) }; // Write the response first, then mark as responded. @@ -2301,8 +2288,7 @@ mod tests { } #[test] - fn find_allow_once_by_kind_not_by_option_id() { - // optionId values are intentionally non-obvious to prove we don't hardcode them. + fn permission_requests_select_reject_once_not_allow_once() { let options: Vec = serde_json::from_str( r#"[ {"optionId": "opt-reject-42", "name": "Reject", "kind": "reject_once"}, @@ -2312,15 +2298,13 @@ mod tests { ) .unwrap(); - let allow_once = options + let reject_once = options .iter() - .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("allow_once")); + .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("reject_once")); - 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")); + let opt = reject_once.expect("should find reject_once option"); + assert_eq!(opt["kind"].as_str(), Some("reject_once")); + assert_eq!(opt["optionId"].as_str(), Some("opt-reject-42")); } #[test] @@ -2341,17 +2325,12 @@ mod tests { } #[test] - fn find_reject_once_fallback_when_no_allow_once() { + fn find_reject_once_by_kind() { let options: Vec = serde_json::from_str( r#"[{"optionId": "rej-x", "name": "Reject", "kind": "reject_once"}]"#, ) .unwrap(); - let allow_once = options - .iter() - .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("allow_once")); - assert!(allow_once.is_none()); - let reject_once = options .iter() .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("reject_once")); diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index 35aaec188d..532ed9c522 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -432,13 +432,12 @@ 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 `dontAsk`, which rejects operations that need interactive + /// approval because Buzz does not expose a human permission prompt. #[arg( long, env = "BUZZ_ACP_PERMISSION_MODE", - default_value = "bypass-permissions", + default_value = "dont-ask", value_enum )] pub permission_mode: PermissionMode, @@ -1469,7 +1468,7 @@ mod tests { memory_enabled: true, model: None, session_title: None, - permission_mode: PermissionMode::BypassPermissions, + permission_mode: PermissionMode::DontAsk, respond_to: RespondTo::Anyone, respond_to_allowlist: HashSet::new(), allowed_respond_to: Vec::new(), @@ -2319,9 +2318,9 @@ channels = "ALL" } #[test] - fn test_default_config_uses_bypass_permissions() { + fn test_default_config_rejects_interactive_permissions() { let config = test_config(SubscribeMode::Mentions); - assert_eq!(config.permission_mode, PermissionMode::BypassPermissions); + assert_eq!(config.permission_mode, PermissionMode::DontAsk); } #[test] diff --git a/crates/buzz-acp/src/pool.rs b/crates/buzz-acp/src/pool.rs index 64edf68ee2..6fcddfdf57 100644 --- a/crates/buzz-acp/src/pool.rs +++ b/crates/buzz-acp/src/pool.rs @@ -1009,7 +1009,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 auto-approves via handle_permission_request. + // are safely skipped — the harness rejects interactive permission requests. if !ctx.permission_mode.is_default() && agent_supports_mode(&resp.raw, ctx.permission_mode.as_wire_str()) { @@ -1094,11 +1094,7 @@ 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 +/// Check whether 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 { @@ -1114,7 +1110,11 @@ fn agent_supports_mode(session_new_result: &serde_json::Value, mode_wire: &str) .unwrap_or(false) } -/// per-tool auto-approval in `handle_permission_request`. +/// 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`. /// /// **Fatal exception:** if the agent process exits (e.g., goose crashes on /// unrecognized methods), returns `Err(AgentExited)` so the caller can respawn. @@ -1154,7 +1154,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 auto-approval" + "failed to set permission mode {wire:?}: {e} — falling back to per-tool rejection" ); } Err(_) => { From 870ecaaec0e8e0788f2a134e95235c3ce26e6f42 Mon Sep 17 00:00:00 2001 From: Jordan Mecom Date: Fri, 31 Jul 2026 14:55:19 -0700 Subject: [PATCH 2/3] Remove ACP permission bypass mode Co-authored-by: Jordan Mecom Signed-off-by: Jordan Mecom --- crates/buzz-acp/src/config.rs | 38 ++++++++++++++++------------------- crates/buzz-acp/src/lib.rs | 4 ++-- 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index 532ed9c522..d959685846 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -116,7 +116,6 @@ 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)] @@ -127,9 +126,6 @@ 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, @@ -145,7 +141,6 @@ impl PermissionMode { match self { Self::Default => "default", Self::AcceptEdits => "acceptEdits", - Self::BypassPermissions => "bypassPermissions", Self::DontAsk => "dontAsk", Self::Plan => "plan", } @@ -2269,10 +2264,6 @@ 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"); } @@ -2280,7 +2271,6 @@ 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()); @@ -2288,20 +2278,17 @@ channels = "ALL" #[test] fn test_permission_mode_display() { - assert_eq!( - format!("{}", PermissionMode::BypassPermissions), - "bypassPermissions" - ); + assert_eq!(format!("{}", PermissionMode::DontAsk), "dontAsk"); assert_eq!(format!("{}", PermissionMode::Default), "default"); } #[test] fn test_summary_includes_permission_mode() { let mut config = test_config(SubscribeMode::Mentions); - config.permission_mode = PermissionMode::BypassPermissions; + config.permission_mode = PermissionMode::DontAsk; let s = config.summary(); assert!( - s.contains("permission_mode=bypassPermissions"), + s.contains("permission_mode=dontAsk"), "summary should include permission_mode, got: {s}" ); } @@ -2331,7 +2318,6 @@ channels = "ALL" let cases = [ ("default", PermissionMode::Default), ("accept-edits", PermissionMode::AcceptEdits), - ("bypass-permissions", PermissionMode::BypassPermissions), ("dont-ask", PermissionMode::DontAsk), ("plan", PermissionMode::Plan), ]; @@ -2346,14 +2332,12 @@ channels = "ALL" #[test] fn test_permission_mode_value_enum_camel_case_aliases() { - // 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. + // Operators may set env vars using the camelCase wire-format strings. + // 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), ]; @@ -2366,6 +2350,18 @@ 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 811253e4ac..0c4e5f158c 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -5125,7 +5125,7 @@ mod build_mcp_servers_tests { memory_enabled: false, model: None, session_title: None, - permission_mode: config::PermissionMode::BypassPermissions, + permission_mode: config::PermissionMode::DontAsk, respond_to: config::RespondTo::Anyone, respond_to_allowlist: std::collections::HashSet::new(), allowed_respond_to: vec![], @@ -5347,7 +5347,7 @@ mod error_outcome_emission_tests { memory_enabled: false, model: None, session_title: None, - permission_mode: config::PermissionMode::BypassPermissions, + permission_mode: config::PermissionMode::DontAsk, respond_to: config::RespondTo::Anyone, respond_to_allowlist: HashSet::new(), allowed_respond_to: vec![], From 16fff4dac4370e1a260739d59cdf7b1dda124e4e Mon Sep 17 00:00:00 2001 From: Eli Foster Date: Tue, 4 Aug 2026 10:07:25 -0700 Subject: [PATCH 3/3] test(acp): drive permission denial through the real decision path The permission tests re-implemented the `reject_once` lookup in the test body rather than calling the code under test, so they would all still pass if the harness went back to selecting `allow_once`. They could not call it directly: `handle_permission_request` is a method on `AcpClient`, which owns a live `Child` and its stdio pipes. Extract the choice into `permission_denial_response` and point the tests at it. No behaviour change. This covers the cancelled fallback, which had no test despite being the fail-closed backstop for adapters that offer no `reject_once`, plus the empty-option-list and missing-`optionId` edges. Also drops `find_allow_once_returns_none_when_absent`, which asserted a property of a search no production path performs any more. Co-Authored-By: Claude Opus 5 Signed-off-by: Eli Foster --- crates/buzz-acp/src/acp.rs | 154 +++++++++++++++++++++++++------------ 1 file changed, 106 insertions(+), 48 deletions(-) diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index 1feb834e9f..93109fa94d 100644 --- a/crates/buzz-acp/src/acp.rs +++ b/crates/buzz-acp/src/acp.rs @@ -1902,26 +1902,7 @@ impl AcpClient { options.len() ); - let reject_once = options - .iter() - .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("reject_once")); - - let response = if let Some(opt) = reject_once { - 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:?}" - ); - permission_response_selected(&id, option_id) - } else { - tracing::warn!( - target: "acp::permission", - "no reject_once option found in permission request id={id}, cancelling" - ); - permission_response_cancelled(&id) - }; + let response = permission_denial_response(&id, options)?; // Write the response first, then mark as responded. // @@ -2033,6 +2014,42 @@ 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`. @@ -2287,55 +2304,96 @@ 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: Vec = serde_json::from_str( + let options = options( 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 reject_once = options - .iter() - .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("reject_once")); + let response = + permission_denial_response(&serde_json::json!(7), &options).expect("denial response"); - let opt = reject_once.expect("should find reject_once option"); - assert_eq!(opt["kind"].as_str(), Some("reject_once")); - assert_eq!(opt["optionId"].as_str(), Some("opt-reject-42")); + 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" + ); } + /// 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 find_allow_once_returns_none_when_absent() { - let options: Vec = serde_json::from_str( + fn permission_request_without_reject_once_is_cancelled() { + let options = options( r#"[ - {"optionId": "reject-1", "name": "Reject", "kind": "reject_once"}, - {"optionId": "reject-always", "name": "Always reject", "kind": "reject_always"} + {"optionId": "opt-allow-99", "name": "Allow once", "kind": "allow_once"}, + {"optionId": "opt-always-7", "name": "Always allow", "kind": "allow_always"} ]"#, - ) - .unwrap(); + ); - let allow_once = options - .iter() - .find(|opt| opt.get("kind").and_then(|k| k.as_str()) == Some("allow_once")); + let response = permission_denial_response(&serde_json::json!("req-1"), &options) + .expect("cancelled response"); + + 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!(allow_once.is_none()); + 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:?}"); } #[test] fn find_reject_once_by_kind() { - let options: Vec = serde_json::from_str( - r#"[{"optionId": "rej-x", "name": "Reject", "kind": "reject_once"}]"#, - ) - .unwrap(); + let options = + options(r#"[{"optionId": "rej-x", "name": "Reject", "kind": "reject_once"}]"#); - 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")); + let response = + permission_denial_response(&serde_json::json!(1), &options).expect("denial response"); + + assert_eq!( + response["result"]["outcome"]["optionId"].as_str(), + Some("rej-x") + ); } #[test]