Revert "fix(acp): reject unattended permission requests" (#5323)

Reverts block/buzz#4609

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<serde_json::Value>,
     /// 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<serde_json::Value, AcpError> {
-    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::Value> {
-        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::Value> = 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::Value> = 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::Value> = 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]

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<u64>, turn: Option<u64>) -> u64 {

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![],

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(_) => {