diff --git a/crates/buzz-acp/README.md b/crates/buzz-acp/README.md index 41d9a214bdd..378b9ddd09d 100644 --- a/crates/buzz-acp/README.md +++ b/crates/buzz-acp/README.md @@ -109,13 +109,42 @@ All configuration is via environment variables (or CLI flags — every env var h | `BUZZ_PRIVATE_KEY` | **yes** | — | Agent's Nostr private key (`nsec1...`). Used for relay auth and agent identity. | | `BUZZ_RELAY_URL` | no | `ws://localhost:3000` | Relay WebSocket URL. | | `BUZZ_ACP_AGENT_COMMAND` | no | `goose` | Agent binary to spawn. | -| `BUZZ_ACP_AGENT_ARGS` | no | `acp` | Agent arguments (comma-separated). | +| `BUZZ_ACP_AGENT_ARGS` | no | `acp` | Agent arguments (comma-separated). Legacy behavior — entries are not shell-split. | +| `BUZZ_ACP_AGENT_ARGS_JSON` | no | — | Structured agent arguments as a JSON array of strings. Takes precedence over `BUZZ_ACP_AGENT_ARGS` when set. Preserves arguments containing spaces, backslashes, and quotes without shell reinterpretation. Example: `["-m","my-model","--reasoning","low"]` | | `BUZZ_ACP_MCP_COMMAND` | no | `""` (empty) | Path to an optional MCP server binary to provide to the agent subprocess. | | `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). | -**Note:** `BUZZ_ACP_AGENT_ARGS` splits on commas. For args with values, use: `-c,key="value"`. +**Note:** `BUZZ_ACP_AGENT_ARGS` splits on commas. Each comma-delimited entry is +preserved as-is — no shell splitting is applied. For arguments containing +spaces, backslashes, or quotes, use `BUZZ_ACP_AGENT_ARGS_JSON` instead: + +```bash +# Legacy comma-separated (no spaces in values): +export BUZZ_ACP_AGENT_ARGS='-c,key=value' + +# Structured JSON (handles spaces, backslashes, quotes): +export BUZZ_ACP_AGENT_ARGS_JSON='["-m","my-model","--reasoning","low"]' + +# Single argument containing a space: +export BUZZ_ACP_AGENT_ARGS_JSON='["--label=hello world"]' + +# Windows path with backslashes: +export BUZZ_ACP_AGENT_ARGS_JSON='["--config=C:\\Program Files\\Agent\\config.toml"]' +``` + +When both `BUZZ_ACP_AGENT_ARGS_JSON` and a non-default `BUZZ_ACP_AGENT_ARGS` are +set, startup fails with an error explaining the conflict. Invalid JSON also +fails startup — the agent does not launch with ambiguous arguments. + +**Producer behavior:** Desktop and the Kubernetes backend automatically select +the appropriate transport. When no argument contains a comma, they write +`BUZZ_ACP_AGENT_ARGS` (comma-joined). When any argument contains a comma, they +serialize the argument list as JSON into `BUZZ_ACP_AGENT_ARGS_JSON` and set +`BUZZ_ACP_AGENT_ARGS` to the default `"acp"`, which the harness treats as +"not configured." This is transparent to the user — no manual env var +configuration is needed. **Legacy env vars:** `BUZZ_ACP_PRIVATE_KEY`, `BUZZ_ACP_API_TOKEN`, and `BUZZ_ACP_TURN_TIMEOUT` (replaced by `BUZZ_ACP_IDLE_TIMEOUT`) are still accepted as fallbacks. diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index b4d27903c62..e6a1f0c01d6 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -264,6 +264,14 @@ pub struct CliArgs { )] pub agent_args: Vec, + /// Structured agent arguments as a JSON array of strings. + /// When set, takes precedence over `BUZZ_ACP_AGENT_ARGS` (which must be + /// unset or left at its default). Parses strictly with no shell splitting, + /// preserving arguments containing spaces, backslashes, and quotes. + /// Example: `BUZZ_ACP_AGENT_ARGS_JSON='["-m","my-model","--reasoning","low"]'` + #[arg(long, env = "BUZZ_ACP_AGENT_ARGS_JSON")] + pub agent_args_json: Option, + #[arg(long, env = "BUZZ_ACP_MCP_COMMAND", default_value = "")] pub mcp_command: String, @@ -871,8 +879,14 @@ pub fn codex_network_env(agent_command: &str, relay_url: &str) -> Option<(String pub fn normalize_agent_args(command: &str, agent_args: Vec) -> Vec { let normalized = agent_args .into_iter() - .map(|arg| arg.trim().to_string()) - .filter(|arg| !arg.is_empty()) + .flat_map(|arg| { + let trimmed = arg.trim().to_string(); + if trimmed.is_empty() { + Vec::new() + } else { + vec![trimmed] + } + }) .collect::>(); let Some(default_args) = default_agent_args(command) else { @@ -894,6 +908,59 @@ pub fn normalize_agent_args(command: &str, agent_args: Vec) -> Vec`, no shell splitting +/// - legacy only → today's comma-delimited behavior, unchanged +/// - both set → startup error explaining the conflict +/// - invalid JSON → startup error; do not launch the agent +pub fn resolve_agent_args( + command: &str, + legacy_args: Vec, + json_args: Option<&str>, +) -> Result, ConfigError> { + match json_args { + Some(json) if !json.trim().is_empty() => { + if !(legacy_args.is_empty() + || legacy_args.len() == 1 && legacy_args[0] == "acp") + { + return Err(ConfigError::ConfigFile( + "BUZZ_ACP_AGENT_ARGS and BUZZ_ACP_AGENT_ARGS_JSON are both set; \ + use only one. JSON is the canonical structured transport; \ + the legacy comma-delimited variable must be unset or removed." + .into(), + )); + } + let parsed: Vec = serde_json::from_str(json).map_err(|e| { + ConfigError::ConfigFile(format!( + "BUZZ_ACP_AGENT_ARGS_JSON is not a valid JSON array of strings: {e}" + )) + })?; + // Apply the same default-value fallback as normalize_agent_args, + // but do NOT filter empty strings — JSON is a lossless transport + // where an explicit "" is a meaningful argv element. + let Some(default_args) = default_agent_args(command) else { + return Ok(parsed); + }; + if parsed.is_empty() { + return Ok(default_args); + } + if parsed.len() == 1 + && parsed[0].eq_ignore_ascii_case("acp") + && default_args.is_empty() + { + return Ok(default_args); + } + Ok(parsed) + } + _ => Ok(normalize_agent_args(command, legacy_args)), + } +} + /// Propagate legacy env-var aliases to their canonical names. /// /// Must be called **before** the tokio runtime starts — i.e. from the sync @@ -1002,7 +1069,11 @@ impl Config { )); } - let agent_args = normalize_agent_args(&agent_command, args.agent_args); + let agent_args = resolve_agent_args( + &agent_command, + args.agent_args, + args.agent_args_json.as_deref(), + )?; if let Some(ref channels) = args.channels { for ch in channels { @@ -1688,6 +1759,123 @@ mod tests { ); } + // --- BUZZ_ACP_AGENT_ARGS_JSON structured transport (#6017) --- + + #[test] + fn resolve_agent_args_json_parses_array() { + // JSON array is parsed strictly — no shell splitting. + let args = resolve_agent_args( + "custom-agent", + vec!["acp".into()], + Some(r#"["-m","my-model","--reasoning","low"]"#), + ) + .unwrap(); + assert_eq!(args, vec!["-m", "my-model", "--reasoning", "low"]); + } + + #[test] + fn resolve_agent_args_json_preserves_spaces_in_values() { + // An argument containing a space is one argv element. + let args = resolve_agent_args( + "custom-agent", + vec!["acp".into()], + Some(r#"["--label=hello world"]"#), + ) + .unwrap(); + assert_eq!(args, vec!["--label=hello world"]); + } + + #[test] + fn resolve_agent_args_json_preserves_windows_paths() { + // Backslashes in Windows paths are preserved, not interpreted as + // POSIX escape characters. + let args = resolve_agent_args( + "custom-agent", + vec!["acp".into()], + Some(r#"["--config=C:\\Program Files\\Agent\\config.toml"]"#), + ) + .unwrap(); + assert_eq!( + args, + vec!["--config=C:\\Program Files\\Agent\\config.toml"] + ); + } + + #[test] + fn resolve_agent_args_json_preserves_empty_args_and_quotes() { + // Empty strings and values starting with -- are preserved exactly. + let args = resolve_agent_args( + "custom-agent", + vec!["acp".into()], + Some(r#"["","--verbose","--label=has \"quotes\""]"#), + ) + .unwrap(); + assert_eq!(args, vec!["", "--verbose", "--label=has \"quotes\""]); + } + + #[test] + fn resolve_agent_args_legacy_only_unchanged() { + // Legacy comma-delimited behavior is unchanged — no shell splitting. + let args = resolve_agent_args( + "custom-agent", + vec!["-c".into(), "model=gpt-5".into()], + None, + ) + .unwrap(); + assert_eq!(args, vec!["-c", "model=gpt-5"]); + } + + #[test] + fn resolve_agent_args_both_set_is_error() { + // Both JSON and non-default legacy args is a startup error. + let result = resolve_agent_args( + "custom-agent", + vec!["-c".into(), "model=gpt-5".into()], + Some(r#"["--verbose"]"#), + ); + assert!(result.is_err()); + let msg = format!("{}", result.unwrap_err()); + assert!(msg.contains("both set"), "error should mention conflict: {msg}"); + } + + #[test] + fn resolve_agent_args_json_with_default_legacy_is_ok() { + // JSON takes precedence when legacy is at its default ("acp"). + let args = resolve_agent_args( + "custom-agent", + vec!["acp".into()], + Some(r#"["--verbose"]"#), + ) + .unwrap(); + assert_eq!(args, vec!["--verbose"]); + } + + #[test] + fn resolve_agent_args_invalid_json_is_error() { + let result = resolve_agent_args( + "custom-agent", + vec!["acp".into()], + Some("not valid json"), + ); + assert!(result.is_err()); + let msg = format!("{}", result.unwrap_err()); + assert!(msg.contains("not a valid JSON array"), "error should mention invalid JSON: {msg}"); + } + + #[test] + fn resolve_agent_args_neither_set_uses_default() { + // No JSON, legacy at default for goose → ["acp"]. + let args = resolve_agent_args("goose", vec!["acp".into()], None).unwrap(); + assert_eq!(args, vec!["acp"]); + } + + #[test] + fn resolve_agent_args_empty_json_uses_default() { + // Empty JSON array → provider default. + let args = resolve_agent_args("goose", vec!["acp".into()], Some("[]")).unwrap(); + assert_eq!(args, vec!["acp"]); + } + #[test] fn normalizes_buzz_agent_args_to_empty() { assert_eq!( diff --git a/crates/buzz-backend-kubernetes/src/env.rs b/crates/buzz-backend-kubernetes/src/env.rs index 5fc27ab9055..59d580eb6d9 100644 --- a/crates/buzz-backend-kubernetes/src/env.rs +++ b/crates/buzz-backend-kubernetes/src/env.rs @@ -14,7 +14,8 @@ use std::collections::BTreeMap; /// before writing its own values, so a key the authoritative tier has no value /// for is **removed** rather than left holding a lower-tier value. Plain /// overwrite is not enough — most of these are written conditionally -/// (`BUZZ_ACP_AGENT_ARGS` only when `launch.args` is non-empty, +/// (`BUZZ_ACP_AGENT_ARGS` / `BUZZ_ACP_AGENT_ARGS_JSON` only when `launch.args` +/// is non-empty, /// `BUZZ_ACP_RESPOND_TO` only when set), and without the clear, a lower tier /// could supply the value for exactly the cases the authoritative tier stays /// silent on. Clearing is also what the local spawn does: the desktop strips @@ -28,6 +29,7 @@ const AUTHORITATIVE_KEYS: &[&str] = &[ "BUZZ_ACP_AGENT_OWNER", "BUZZ_ACP_AGENT_COMMAND", "BUZZ_ACP_AGENT_ARGS", + "BUZZ_ACP_AGENT_ARGS_JSON", "BUZZ_ACP_RESPOND_TO", "BUZZ_ACP_RESPOND_TO_ALLOWLIST", "BUZZ_ACP_MCP_COMMAND", @@ -258,11 +260,15 @@ pub fn build_env( env.insert("BUZZ_ACP_AGENT_COMMAND".into(), command.to_string()); } if !launch.args.is_empty() { - // Comma-joined because that is what the harness's CLI parser decodes, - // and what the desktop's local spawn does. An argument containing a - // comma is unrepresentable in both paths; inventing an escaping - // scheme here would produce args the harness cannot decode. - env.insert("BUZZ_ACP_AGENT_ARGS".into(), launch.args.join(",")); + if launch.args.iter().any(|a| a.contains(',')) { + env.insert( + "BUZZ_ACP_AGENT_ARGS_JSON".into(), + serde_json::to_string(&launch.args).expect("Vec is always serializable"), + ); + env.insert("BUZZ_ACP_AGENT_ARGS".into(), "acp".into()); + } else { + env.insert("BUZZ_ACP_AGENT_ARGS".into(), launch.args.join(",")); + } } env.insert("BUZZ_ACP_MCP_COMMAND".into(), "buzz-dev-mcp".into()); @@ -589,11 +595,30 @@ mod tests { })); let env = build(&agent).unwrap(); assert_eq!(env["BUZZ_ACP_AGENT_ARGS"], "run,--no-session"); + assert!(!env.contains_key("BUZZ_ACP_AGENT_ARGS_JSON")); let agent = payload_json(serde_json::json!({ "launch": {"command": "goose", "args": [], "owner_pubkey": "b"} })); assert!(!build(&agent).unwrap().contains_key("BUZZ_ACP_AGENT_ARGS")); + assert!(!build(&agent).unwrap().contains_key("BUZZ_ACP_AGENT_ARGS_JSON")); + } + + #[test] + fn args_containing_commas_use_json_transport() { + let agent = payload_json(serde_json::json!({ + "launch": { + "command": "goose", + "args": ["--config=a,b", "--safe", "on"], + "owner_pubkey": "b" + } + })); + let env = build(&agent).unwrap(); + assert_eq!( + env["BUZZ_ACP_AGENT_ARGS_JSON"], + r#"["--config=a,b","--safe","on"]"# + ); + assert_eq!(env["BUZZ_ACP_AGENT_ARGS"], "acp"); } /// The top-level `model`/`provider` fields are display inputs; their @@ -678,6 +703,7 @@ mod tests { // merely different. for absent in [ "BUZZ_ACP_AGENT_ARGS", + "BUZZ_ACP_AGENT_ARGS_JSON", "BUZZ_ACP_RESPOND_TO", "BUZZ_ACP_RESPOND_TO_ALLOWLIST", ] { diff --git a/desktop/src-tauri/src/commands/agent_auth.rs b/desktop/src-tauri/src/commands/agent_auth.rs index bba0f338602..7a37f24e4cf 100644 --- a/desktop/src-tauri/src/commands/agent_auth.rs +++ b/desktop/src-tauri/src/commands/agent_auth.rs @@ -184,8 +184,9 @@ fn run_buzz_acp_auth_command_with_paths( let mut command = Command::new(acp_path); command .args(args) - .env("BUZZ_ACP_AGENT_COMMAND", adapter_path.as_os_str()) - .env("BUZZ_ACP_AGENT_ARGS", agent_args.join(",")) + .env("BUZZ_ACP_AGENT_COMMAND", adapter_path.as_os_str()); + crate::managed_agents::set_agent_args_env(&mut command, &agent_args); + command .stdout(Stdio::piped()) .stderr(Stdio::piped()); if let Some(workdir) = default_agent_workdir() { diff --git a/desktop/src-tauri/src/commands/agent_model_process.rs b/desktop/src-tauri/src/commands/agent_model_process.rs index f671983bbc6..5649178585d 100644 --- a/desktop/src-tauri/src/commands/agent_model_process.rs +++ b/desktop/src-tauri/src/commands/agent_model_process.rs @@ -38,8 +38,8 @@ pub(super) async fn run_agent_models_command( } cmd.arg("models") .arg("--json") - .env("BUZZ_ACP_AGENT_COMMAND", &agent_command) - .env("BUZZ_ACP_AGENT_ARGS", agent_args.join(",")); + .env("BUZZ_ACP_AGENT_COMMAND", &agent_command); + crate::managed_agents::set_agent_args_env(&mut cmd, &agent_args); if let Some(meta) = known_acp_runtime(&agent_command) { for (key, value) in meta.default_env { if std::env::var(key).is_err() { diff --git a/desktop/src-tauri/src/managed_agents/env_vars.rs b/desktop/src-tauri/src/managed_agents/env_vars.rs index 6b12fbcd2be..39dd46c10fb 100644 --- a/desktop/src-tauri/src/managed_agents/env_vars.rs +++ b/desktop/src-tauri/src/managed_agents/env_vars.rs @@ -45,6 +45,42 @@ pub(crate) fn is_derived_provider_model_key(key: &str) -> bool { // See `reserved_env_keys.rs` for why this is `include!`d rather than a module. include!("reserved_env_keys.rs"); +/// Decide which environment variables to set for agent arguments. +/// +/// When any argument contains a comma, the legacy comma-delimited +/// `BUZZ_ACP_AGENT_ARGS` cannot represent it losslessly — the comma would +/// be interpreted as a delimiter. In that case the arguments are serialized +/// as a JSON array into `BUZZ_ACP_AGENT_ARGS_JSON` (the canonical structured +/// transport), and `BUZZ_ACP_AGENT_ARGS` is set to the default `"acp"` so the +/// harness's conflict guard (legacy must be unset or default) passes. +/// +/// When no argument contains a comma, the legacy comma-delimited form is +/// used for full backward compatibility with older harness builds. +/// +/// Returns `(legacy_value, json_value)` where `legacy_value` is always set +/// and `json_value` is `Some` only when the JSON transport is used. +fn agent_args_env(agent_args: &[String]) -> (String, Option) { + if agent_args.iter().any(|a| a.contains(',')) { + let json = serde_json::to_string(agent_args).unwrap_or_else(|_| agent_args.join(",")); + ("acp".to_string(), Some(json)) + } else { + (agent_args.join(","), None) + } +} + +/// Write agent arguments to the child process environment using the +/// appropriate transport. See [`agent_args_env`] for the selection logic. +pub(crate) fn set_agent_args_env( + command: &mut std::process::Command, + agent_args: &[String], +) { + let (legacy, json) = agent_args_env(agent_args); + command.env("BUZZ_ACP_AGENT_ARGS", legacy); + if let Some(json) = json { + command.env("BUZZ_ACP_AGENT_ARGS_JSON", json); + } +} + /// Returns true if `key` is a well-formed POSIX-shaped env var name: /// `[A-Za-z_][A-Za-z0-9_]*`. This is a hard requirement, not a stylistic /// nit: Rust's `Command::env` will happily accept a key containing `=` diff --git a/desktop/src-tauri/src/managed_agents/env_vars/tests.rs b/desktop/src-tauri/src/managed_agents/env_vars/tests.rs index 7aa886c672a..87c9c9044fa 100644 --- a/desktop/src-tauri/src/managed_agents/env_vars/tests.rs +++ b/desktop/src-tauri/src/managed_agents/env_vars/tests.rs @@ -2,7 +2,7 @@ use std::collections::BTreeMap; use super::{ display_invalid_key, is_derived_provider_model_key, is_reserved_env_key, - is_well_formed_env_key, merged_user_env, validate_user_env_keys, + is_well_formed_env_key, merged_user_env, set_agent_args_env, validate_user_env_keys, DERIVED_PROVIDER_MODEL_ENV_KEYS, MAX_ENV_TOTAL_BYTES, MAX_ENV_VALUE_BYTES, RESERVED_ENV_KEYS, }; @@ -197,6 +197,7 @@ fn reserved_keys_include_code_execution_surface() { for key in [ "BUZZ_ACP_AGENT_COMMAND", "BUZZ_ACP_AGENT_ARGS", + "BUZZ_ACP_AGENT_ARGS_JSON", "BUZZ_ACP_MCP_COMMAND", ] { assert!(is_reserved_env_key(key), "{key} should be reserved"); @@ -520,3 +521,40 @@ fn deploy_model_precedence_none_when_both_absent() { let effective = persona_model.clone().or(record_model.clone()); assert_eq!(effective, None); } + +// ── set_agent_args_env: JSON transport for comma-containing args ──── + +#[test] +fn agent_args_env_comma_safe_args_use_legacy_transport() { + let (legacy, json) = super::agent_args_env(&[ + "-m".into(), + "my-model".into(), + "--reasoning".into(), + "low".into(), + ]); + assert_eq!(legacy, "-m,my-model,--reasoning,low"); + assert_eq!(json, None); +} + +#[test] +fn agent_args_env_empty_args_use_legacy_transport() { + let (legacy, json) = super::agent_args_env(&[]); + assert_eq!(legacy, ""); + assert_eq!(json, None); +} + +#[test] +fn agent_args_env_comma_in_arg_switches_to_json_transport() { + let (legacy, json) = super::agent_args_env(&["--config=a,b".into(), "--safe".into(), "on".into()]); + assert_eq!(legacy, "acp"); + let json = json.expect("JSON transport should be used when an arg contains a comma"); + assert_eq!(json, r#"["--config=a,b","--safe","on"]"#); +} + +#[test] +fn agent_args_env_multiple_commas_in_one_arg() { + let (legacy, json) = super::agent_args_env(&["a,b,c".into()]); + assert_eq!(legacy, "acp"); + let json = json.expect("JSON transport for comma-containing arg"); + assert_eq!(json, r#"["a,b,c"]"#); +} diff --git a/desktop/src-tauri/src/managed_agents/mod.rs b/desktop/src-tauri/src/managed_agents/mod.rs index a66f9c75ba2..7ffb191afb1 100644 --- a/desktop/src-tauri/src/managed_agents/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mod.rs @@ -79,6 +79,7 @@ pub(crate) use definition_validation::{ }; pub use discovery::*; pub use env_vars::*; +pub(crate) use env_vars::set_agent_args_env; #[cfg(windows)] pub(crate) use git_bash::git_bash_available; pub(crate) use git_bash::{discover_git_bash, GitBashPrerequisite}; diff --git a/desktop/src-tauri/src/managed_agents/reserved_env_keys.rs b/desktop/src-tauri/src/managed_agents/reserved_env_keys.rs index 107171b3d47..5bfc8602036 100644 --- a/desktop/src-tauri/src/managed_agents/reserved_env_keys.rs +++ b/desktop/src-tauri/src/managed_agents/reserved_env_keys.rs @@ -40,6 +40,7 @@ pub(crate) const RESERVED_ENV_KEYS: &[&str] = &[ // binaries/args as the agent process. "BUZZ_ACP_AGENT_COMMAND", "BUZZ_ACP_AGENT_ARGS", + "BUZZ_ACP_AGENT_ARGS_JSON", "BUZZ_ACP_MCP_COMMAND", // pi-acp's executable override is reserved for Buzz's generated launcher, // which injects the managed system prompt and skills. diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index b8d586b32af..fd6df7e6439 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -582,7 +582,7 @@ pub fn spawn_agent_child( // loop by `apply_replay_floor_env` so saved user env cannot shadow it. command.env_remove(REPLAY_FLOOR_ENV_VAR); command.env("BUZZ_ACP_AGENT_COMMAND", &resolved_agent_command); - command.env("BUZZ_ACP_AGENT_ARGS", agent_args.join(",")); + super::set_agent_args_env(&mut command, agent_args); match &resolved_mcp_command { Some(mcp_cmd) => { command.env("BUZZ_ACP_MCP_COMMAND", mcp_cmd);