diff --git a/.env.example b/.env.example index b9bfcada0e..f4a50528c2 100644 --- a/.env.example +++ b/.env.example @@ -167,6 +167,10 @@ RUST_LOG=buzz_relay=debug,buzz_datastore=info,buzz_db=debug,buzz_auth=debug,buzz # Use `buzz-acp models` to discover available model IDs. # BUZZ_ACP_MODEL= +# Reviewer for eligible Codex sandbox approval requests. +# Values: user, auto-review (alias: auto_review). Unset preserves Codex's default. +# BUZZ_ACP_CODEX_APPROVALS_REVIEWER=auto-review + # ── Timeouts & sessions ────────────────────────────────────────────────────── # Max seconds per agent turn before timeout (default 320 = ~5 min). # BUZZ_ACP_TURN_TIMEOUT=320 diff --git a/crates/buzz-acp/README.md b/crates/buzz-acp/README.md index e6164b02dd..d086fbbd2a 100644 --- a/crates/buzz-acp/README.md +++ b/crates/buzz-acp/README.md @@ -111,6 +111,7 @@ All configuration is via environment variables (or CLI flags — every env var h | `BUZZ_ACP_AGENT_COMMAND` | no | `goose` | Agent binary to spawn. | | `BUZZ_ACP_AGENT_ARGS` | no | `acp` | Agent arguments (comma-separated). | | `BUZZ_ACP_MCP_COMMAND` | no | `""` (empty) | Path to an optional MCP server binary to provide to the agent subprocess. | +| `BUZZ_ACP_CODEX_APPROVALS_REVIEWER` | no | Codex default | Reviewer for eligible Codex sandbox approval requests: `user` or `auto-review` (`auto_review` alias). Auto-review is Codex's “Approve for me” behavior and does not widen the sandbox. | | `BUZZ_ACP_IDLE_TIMEOUT` | no | `620` | Idle timeout: max seconds of silence before cancelling a turn. Resets on any agent stdout activity. | | `BUZZ_ACP_MAX_TURN_DURATION` | no | `7200` | Absolute wall-clock cap per turn (safety valve). | | `BUZZ_API_TOKEN` | no | — | API token (required if relay enforces token auth). | diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index 8a698954a0..783d36e91b 100644 --- a/crates/buzz-acp/src/acp.rs +++ b/crates/buzz-acp/src/acp.rs @@ -4271,6 +4271,8 @@ mod tests { } const GENERATED: &str = r#"{"sandbox_workspace_write":{"network_access":true}}"#; + const GENERATED_AUTO_REVIEW: &str = + r#"{"approvals_reviewer":"auto_review","sandbox_workspace_write":{"network_access":true}}"#; #[test] fn build_codex_config_env_returns_none_when_no_codex_config_in_extra_env() { @@ -4364,6 +4366,45 @@ mod tests { ); } + #[test] + fn build_codex_config_env_merges_auto_review_with_persona_and_parent_precedence() { + let persona = r#"{"approvals_reviewer":"user","some_feature":{"enabled":true}}"#; + let extra = env(&[ + ("CODEX_CONFIG", persona), + ("CODEX_CONFIG", GENERATED_AUTO_REVIEW), + ]); + + let merged = build_codex_config_env(&extra, None, true).unwrap().unwrap(); + let value: serde_json::Value = serde_json::from_str(&merged).unwrap(); + assert_eq!( + value["approvals_reviewer"], "auto_review", + "generated reviewer must overlay the persona value" + ); + assert_eq!( + value["some_feature"]["enabled"], true, + "unrelated persona config must survive" + ); + assert_eq!( + value["sandbox_workspace_write"]["network_access"], true, + "generated relay network access must survive" + ); + + let parent = r#"{"approvals_reviewer":"user","parent_key":"keep"}"#; + let merged = build_codex_config_env(&extra, Some(parent), true) + .unwrap() + .unwrap(); + let value: serde_json::Value = serde_json::from_str(&merged).unwrap(); + assert_eq!( + value["approvals_reviewer"], "user", + "parent CODEX_CONFIG must retain its established collision precedence" + ); + assert_eq!(value["parent_key"], "keep"); + assert_eq!( + value["sandbox_workspace_write"]["network_access"], true, + "forced relay network access remains the final invariant" + ); + } + #[test] fn build_codex_config_env_nested_persona_keys_survive_when_parent_has_same_top_level_key() { // Persona has sandbox_workspace_write.persona_only; parent has diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index dab61be30a..84ac68c83c 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -164,6 +164,37 @@ impl std::fmt::Display for PermissionMode { } } +/// Reviewer for Codex approval requests that cross the active sandbox boundary. +/// +/// This is separate from [`PermissionMode`]: Codex Auto-review keeps an +/// interactive approval policy and changes who reviews eligible requests, +/// while ACP session modes select broader approval/sandbox presets. +#[derive(Debug, Clone, Copy, PartialEq, Eq, clap::ValueEnum)] +pub enum CodexApprovalsReviewer { + /// Surface eligible approval requests to the user. + #[value(alias = "user")] + User, + /// Route eligible approval requests to Codex's automatic reviewer. + #[value(name = "auto-review", alias = "auto_review")] + AutoReview, +} + +impl CodexApprovalsReviewer { + /// Return the Codex config value used by `approvals_reviewer`. + pub fn as_config_str(&self) -> &'static str { + match self { + Self::User => "user", + Self::AutoReview => "auto_review", + } + } +} + +impl std::fmt::Display for CodexApprovalsReviewer { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_config_str()) + } +} + /// CLI args for `buzz-acp models` — query available models from an agent. /// /// This is a standalone `Parser` (not a subcommand variant) because the @@ -443,6 +474,14 @@ pub struct CliArgs { )] pub permission_mode: PermissionMode, + /// Reviewer for eligible Codex approval requests. + /// + /// `auto-review` enables Codex's "Approve for me" behavior without + /// widening its sandbox. Omit this option to preserve Codex's configured + /// default. Ignored by non-Codex agents. + #[arg(long, env = "BUZZ_ACP_CODEX_APPROVALS_REVIEWER", value_enum)] + pub codex_approvals_reviewer: Option, + /// Inbound author gate: which authors' events the harness forwards. /// Modes: owner-only (default), allowlist, anyone, nobody. #[arg( @@ -533,6 +572,9 @@ pub struct Config { pub session_title: Option, /// Permission mode to apply after session creation. `Default` = skip. pub permission_mode: PermissionMode, + /// Codex approval reviewer injected through generated `CODEX_CONFIG`. + /// `None` preserves the adapter's configured default. + pub codex_approvals_reviewer: Option, /// Inbound author gate mode. pub respond_to: RespondTo, /// Validated allowlist of pubkey hex strings (used when respond_to == Allowlist). @@ -725,8 +767,9 @@ pub(crate) fn default_agent_env(command: &str) -> &'static [(&'static str, &'sta /// that blocks all outbound network by default. Without this env var, `buzz-cli` /// requests are blocked before they can reach the relay WebSocket. /// -/// Returns `Some(("CODEX_CONFIG", "{\"sandbox_workspace_write\":{\"network_access\":true}}"))` for -/// Codex agents, or `None` for non-Codex agents or when the relay URL cannot be parsed. +/// Returns a generated `CODEX_CONFIG` entry for Codex agents, optionally including +/// `approvals_reviewer`, or `None` for non-Codex agents or when the relay URL cannot +/// be parsed. /// /// The env var is forwarded by the `@agentclientprotocol/codex-acp` adapter (1.x) as a /// session-level config override (via `CODEX_CONFIG` → `thread/start config`), which is @@ -738,7 +781,11 @@ pub(crate) fn default_agent_env(command: &str) -> &'static [(&'static str, &'sta /// be parsed, avoiding accidental sandbox widening for malformed configs. /// /// Handles `ws://`, `wss://`, `http://`, and `https://` schemes. -pub fn codex_network_env(agent_command: &str, relay_url: &str) -> Option<(String, String)> { +pub fn codex_network_env( + agent_command: &str, + relay_url: &str, + approvals_reviewer: Option, +) -> Option<(String, String)> { match normalize_agent_command_identity(agent_command).as_str() { "codex" | "codex-acp" => {} _ => return None, @@ -763,11 +810,27 @@ pub fn codex_network_env(agent_command: &str, relay_url: &str) -> Option<(String } }; - tracing::debug!(host, "injecting CODEX_CONFIG network_access for relay host"); + tracing::debug!( + host, + approvals_reviewer = approvals_reviewer.map(|reviewer| reviewer.as_config_str()), + "injecting generated CODEX_CONFIG for relay host" + ); + + let mut generated = serde_json::Map::new(); + generated.insert( + "sandbox_workspace_write".into(), + serde_json::json!({"network_access": true}), + ); + if let Some(reviewer) = approvals_reviewer { + generated.insert( + "approvals_reviewer".into(), + serde_json::Value::String(reviewer.as_config_str().into()), + ); + } Some(( "CODEX_CONFIG".into(), - "{\"sandbox_workspace_write\":{\"network_access\":true}}".into(), + serde_json::Value::Object(generated).to_string(), )) } @@ -1043,13 +1106,16 @@ impl Config { // Inject CODEX_CONFIG so the @agentclientprotocol/codex-acp adapter (1.x) // opens the Seatbelt network sandbox for buzz-cli (an MCP subprocess). No-op // for non-Codex agents or unparseable relay URLs. - let has_generated_codex_config = - if let Some(network_env) = codex_network_env(&agent_command, &args.relay_url) { - persona_env_vars.push(network_env); - true - } else { - false - }; + let has_generated_codex_config = if let Some(network_env) = codex_network_env( + &agent_command, + &args.relay_url, + args.codex_approvals_reviewer, + ) { + persona_env_vars.push(network_env); + true + } else { + false + }; validate_multiple_event_handling(args.multiple_event_handling, args.dedup)?; @@ -1092,6 +1158,7 @@ impl Config { .as_deref() .and_then(sanitize_session_title), permission_mode: args.permission_mode, + codex_approvals_reviewer: args.codex_approvals_reviewer, respond_to: args.respond_to, respond_to_allowlist, allowed_respond_to, @@ -1123,7 +1190,7 @@ impl Config { format!(" allowed_respond_to=[{}]", modes.join(",")) }; format!( - "relay={} pubkey={} agent_cmd={} {} mcp_cmd={} idle_timeout={}s max_turn={}s agents={} heartbeat={}s subscribe={:?} dedup={:?} meh={:?} ignore_self={} context_limit={} max_turns_per_session={} presence={} typing={} memory={} model={} permission_mode={} {}{}", + "relay={} pubkey={} agent_cmd={} {} mcp_cmd={} idle_timeout={}s max_turn={}s agents={} heartbeat={}s subscribe={:?} dedup={:?} meh={:?} ignore_self={} context_limit={} max_turns_per_session={} presence={} typing={} memory={} model={} permission_mode={} codex_approvals_reviewer={} {}{}", self.relay_url, self.keys.public_key().to_hex(), self.agent_command, @@ -1144,6 +1211,9 @@ impl Config { self.memory_enabled, self.model.as_deref().unwrap_or("(agent default)"), self.permission_mode, + self.codex_approvals_reviewer + .map(|reviewer| reviewer.as_config_str()) + .unwrap_or("(codex default)"), respond_to_detail, allowed_respond_to_detail, ) @@ -1462,6 +1532,7 @@ mod tests { model: None, session_title: None, permission_mode: PermissionMode::BypassPermissions, + codex_approvals_reviewer: None, respond_to: RespondTo::Anyone, respond_to_allowlist: HashSet::new(), allowed_respond_to: Vec::new(), @@ -1669,7 +1740,7 @@ mod tests { #[test] fn codex_network_env_wss_url() { - let result = codex_network_env("codex-acp", "wss://sprout-oss.stage.blox.sqprod.co"); + let result = codex_network_env("codex-acp", "wss://sprout-oss.stage.blox.sqprod.co", None); assert_eq!( result, Some(("CODEX_CONFIG".to_string(), CODEX_CONFIG_JSON.to_string())) @@ -1678,7 +1749,7 @@ mod tests { #[test] fn codex_network_env_ws_url() { - let result = codex_network_env("codex-acp", "ws://localhost:3000"); + let result = codex_network_env("codex-acp", "ws://localhost:3000", None); assert_eq!( result, Some(("CODEX_CONFIG".to_string(), CODEX_CONFIG_JSON.to_string())) @@ -1687,7 +1758,7 @@ mod tests { #[test] fn codex_network_env_https_url() { - let result = codex_network_env("codex-acp", "https://relay.example.com/path"); + let result = codex_network_env("codex-acp", "https://relay.example.com/path", None); assert_eq!( result, Some(("CODEX_CONFIG".to_string(), CODEX_CONFIG_JSON.to_string())) @@ -1696,7 +1767,7 @@ mod tests { #[test] fn codex_network_env_http_url_with_port() { - let result = codex_network_env("codex-acp", "http://relay.example.com:8080/query"); + let result = codex_network_env("codex-acp", "http://relay.example.com:8080/query", None); assert_eq!( result, Some(("CODEX_CONFIG".to_string(), CODEX_CONFIG_JSON.to_string())) @@ -1706,7 +1777,7 @@ mod tests { #[test] fn codex_network_env_bare_codex_command() { // "codex" (not "codex-acp") should also get the env var. - let result = codex_network_env("codex", "wss://relay.example.com"); + let result = codex_network_env("codex", "wss://relay.example.com", None); assert_eq!( result, Some(("CODEX_CONFIG".to_string(), CODEX_CONFIG_JSON.to_string())) @@ -1716,7 +1787,7 @@ mod tests { #[test] fn codex_network_env_full_path_codex_command() { // Full path like /usr/local/bin/codex-acp should be normalized. - let result = codex_network_env("/usr/local/bin/codex-acp", "wss://relay.example.com"); + let result = codex_network_env("/usr/local/bin/codex-acp", "wss://relay.example.com", None); assert_eq!( result, Some(("CODEX_CONFIG".to_string(), CODEX_CONFIG_JSON.to_string())) @@ -1725,16 +1796,21 @@ mod tests { #[test] fn codex_network_env_non_codex_agent_returns_none() { - assert!(codex_network_env("goose", "wss://relay.example.com").is_none()); - assert!(codex_network_env("claude-agent-acp", "wss://relay.example.com").is_none()); - assert!(codex_network_env("buzz-agent", "wss://relay.example.com").is_none()); + assert!(codex_network_env("goose", "wss://relay.example.com", None).is_none()); + assert!(codex_network_env( + "claude-agent-acp", + "wss://relay.example.com", + Some(CodexApprovalsReviewer::AutoReview), + ) + .is_none()); + assert!(codex_network_env("buzz-agent", "wss://relay.example.com", None).is_none()); } #[test] fn codex_network_env_includes_sandbox_network_access() { // The JSON value must set sandbox_workspace_write.network_access=true — without // it, the Seatbelt sandbox blocks outbound connections in the 1.x adapter. - let result = codex_network_env("codex-acp", "wss://relay.example.com"); + let result = codex_network_env("codex-acp", "wss://relay.example.com", None); let (key, val) = result.expect("expected Some for valid codex + valid url"); assert_eq!(key, "CODEX_CONFIG"); assert!( @@ -1747,16 +1823,50 @@ mod tests { ); } + #[test] + fn codex_network_env_includes_auto_review_reviewer() { + let (_, value) = codex_network_env( + "codex-acp", + "wss://relay.example.com", + Some(CodexApprovalsReviewer::AutoReview), + ) + .expect("valid Codex configuration should produce an environment entry"); + let parsed: serde_json::Value = + serde_json::from_str(&value).expect("generated CODEX_CONFIG should be valid JSON"); + + assert_eq!(parsed["approvals_reviewer"], "auto_review"); + assert_eq!( + parsed["sandbox_workspace_write"]["network_access"], + serde_json::Value::Bool(true) + ); + } + + #[test] + fn codex_network_env_includes_explicit_user_reviewer() { + let (_, value) = codex_network_env( + "codex", + "wss://relay.example.com", + Some(CodexApprovalsReviewer::User), + ) + .expect("valid Codex configuration should produce an environment entry"); + let parsed: serde_json::Value = + serde_json::from_str(&value).expect("generated CODEX_CONFIG should be valid JSON"); + + assert_eq!(parsed["approvals_reviewer"], "user"); + } + #[test] fn codex_network_env_empty_relay_url_returns_none() { // Empty string fails Url::parse — graceful None return. - assert!(codex_network_env("codex-acp", "").is_none()); + assert!( + codex_network_env("codex-acp", "", Some(CodexApprovalsReviewer::AutoReview),).is_none() + ); } #[test] fn codex_network_env_schemeless_string_returns_none() { // A bare string with no scheme fails Url::parse — graceful None return. - assert!(codex_network_env("codex-acp", "not-a-url").is_none()); + assert!(codex_network_env("codex-acp", "not-a-url", None).is_none()); } #[test] @@ -2299,6 +2409,60 @@ channels = "ALL" assert_eq!(config.permission_mode, PermissionMode::BypassPermissions); } + #[test] + fn test_codex_approvals_reviewer_values_and_alias() { + use clap::ValueEnum; + + assert_eq!( + CodexApprovalsReviewer::from_str("auto-review", true).unwrap(), + CodexApprovalsReviewer::AutoReview + ); + assert_eq!( + CodexApprovalsReviewer::from_str("auto_review", true).unwrap(), + CodexApprovalsReviewer::AutoReview + ); + assert_eq!( + CodexApprovalsReviewer::from_str("user", true).unwrap(), + CodexApprovalsReviewer::User + ); + assert_eq!( + CodexApprovalsReviewer::AutoReview.as_config_str(), + "auto_review" + ); + } + + #[test] + fn test_codex_approvals_reviewer_cli_is_optional() { + let key = "0".repeat(64); + let default_args = CliArgs::parse_from(["buzz-acp", "--private-key", &key]); + assert_eq!(default_args.codex_approvals_reviewer, None); + + let auto_review_args = CliArgs::parse_from([ + "buzz-acp", + "--private-key", + &key, + "--codex-approvals-reviewer", + "auto_review", + ]); + assert_eq!( + auto_review_args.codex_approvals_reviewer, + Some(CodexApprovalsReviewer::AutoReview) + ); + } + + #[test] + fn test_summary_includes_codex_approvals_reviewer() { + let mut config = test_config(SubscribeMode::Mentions); + assert!(config + .summary() + .contains("codex_approvals_reviewer=(codex default)")); + + config.codex_approvals_reviewer = Some(CodexApprovalsReviewer::AutoReview); + assert!(config + .summary() + .contains("codex_approvals_reviewer=auto_review")); + } + #[test] fn test_permission_mode_value_enum_kebab_case() { // clap::ValueEnum generates kebab-case by default from PascalCase variants. diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index 403512a322..c6dae462c7 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -5025,6 +5025,7 @@ mod build_mcp_servers_tests { model: None, session_title: None, permission_mode: config::PermissionMode::BypassPermissions, + codex_approvals_reviewer: None, respond_to: config::RespondTo::Anyone, respond_to_allowlist: std::collections::HashSet::new(), allowed_respond_to: vec![], @@ -5246,6 +5247,7 @@ mod error_outcome_emission_tests { model: None, session_title: None, permission_mode: config::PermissionMode::BypassPermissions, + codex_approvals_reviewer: None, respond_to: config::RespondTo::Anyone, respond_to_allowlist: HashSet::new(), allowed_respond_to: vec![],