From 65d2f55046dd6349db3c54f02b4a8a644dccf8f0 Mon Sep 17 00:00:00 2001 From: Brad Groux <3053586+BradGroux@users.noreply.github.com> Date: Sun, 16 Aug 2026 08:00:39 -0500 Subject: [PATCH 1/4] fix(acp): shell-split space-separated BUZZ_ACP_AGENT_ARGS entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BUZZ_ACP_AGENT_ARGS uses clap's value_delimiter = ',' so a value like "-m my-model --reasoning low" arrives as a single comma-delimited entry containing spaces. The old normalize_agent_args passed it through as one argv element, which the agent binary could not parse — every agent with multi-flag args timed out at 60s with a misleading error. Add a minimal shell_split() that handles single/double quotes, backslash escapes, and whitespace delimiting. When a comma-delimited entry contains a space, it is shell-split into separate argv elements. Entries without spaces pass through unchanged, preserving the existing comma-delimited behavior. No new dependency added (shlex is only a transitive build-dep via cc/cmake/aws-lc-sys). AGENTS.md says do not add production dependencies without approval, so the splitter is implemented inline. 7 new tests: shell_split unit tests (space-separated, single-arg, double-quoted, single-quoted, backslash escape, empty) plus normalize_agent_args integration tests. Closes #6017 Co-authored-by: Brad Groux Signed-off-by: Brad Groux Signed-off-by: dm-builder Signed-off-by: Brad Groux Signed-off-by: Brad Groux <3053586+BradGroux@users.noreply.github.com> --- crates/buzz-acp/src/config.rs | 147 +++++++++++++++++++++++++++++++++- 1 file changed, 145 insertions(+), 2 deletions(-) diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index b4d27903c62..19142ad6beb 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -871,8 +871,24 @@ 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 if trimmed.contains(' ') { + // `BUZZ_ACP_AGENT_ARGS` splits on commas (clap + // `value_delimiter`), so a space-separated value like + // "-m my-model --reasoning low" arrives as one entry. + // Shell-split it so each flag becomes its own argv + // element — matching what every shell user reaches for + // first. Without this, the agent receives the entire + // string as a single argument, fails to parse it, and + // times out at 60s with a misleading error (#6017). + shell_split(&trimmed) + } else { + vec![trimmed] + } + }) .collect::>(); let Some(default_args) = default_agent_args(command) else { @@ -894,6 +910,60 @@ pub fn normalize_agent_args(command: &str, agent_args: Vec) -> Vec Vec { + let mut tokens = Vec::new(); + let mut current = String::new(); + let mut in_single = false; + let mut in_double = false; + let mut escape = false; + let mut has_content = false; + + for ch in input.chars() { + if escape { + current.push(ch); + has_content = true; + escape = false; + continue; + } + match ch { + '\\' if !in_single => { + escape = true; + } + '\'' if !in_double => { + in_single = !in_single; + has_content = true; + } + '"' if !in_single => { + in_double = !in_double; + has_content = true; + } + c if c.is_whitespace() && !in_single && !in_double => { + if has_content { + tokens.push(std::mem::take(&mut current)); + has_content = false; + } + } + c => { + current.push(c); + has_content = true; + } + } + } + if has_content { + tokens.push(current); + } + tokens +} + /// Propagate legacy env-var aliases to their canonical names. /// /// Must be called **before** the tokio runtime starts — i.e. from the sync @@ -1688,6 +1758,79 @@ mod tests { ); } + // --- shell_split fallback for space-separated args (#6017) --- + + #[test] + fn shell_split_space_separated_args() { + // The most common case: a user passes space-separated flags as one + // comma-delimited entry. shell_split must produce separate argv + // elements. + assert_eq!( + shell_split("-m my-model --reasoning low"), + vec!["-m", "my-model", "--reasoning", "low"] + ); + } + + #[test] + fn shell_split_preserves_single_arg_without_spaces() { + assert_eq!(shell_split("acp"), vec!["acp"]); + assert_eq!(shell_split("-c"), vec!["-c"]); + } + + #[test] + fn shell_split_handles_double_quoted_strings() { + // Standard shell behavior: quotes are consumed, not preserved. + // `model="gpt-5"` → `model=gpt-5` (mid-token quotes stripped). + assert_eq!( + shell_split("-c model=\"gpt-5\" --flag"), + vec!["-c", "model=gpt-5", "--flag"] + ); + // `"my model name"` → `my model name` (standalone quotes stripped, + // spaces inside preserved). + assert_eq!( + shell_split(r#"-c "my model name""#), + vec!["-c", "my model name"] + ); + } + + #[test] + fn shell_split_handles_single_quoted_strings() { + assert_eq!( + shell_split("-c 'my model name'"), + vec!["-c", "my model name"] + ); + } + + #[test] + fn shell_split_handles_backslash_escape() { + assert_eq!(shell_split(r"model=gpt\ 5"), vec!["model=gpt 5"]); + } + + #[test] + fn shell_split_empty_input() { + assert!(shell_split("").is_empty()); + assert!(shell_split(" ").is_empty()); + } + + #[test] + fn normalize_agent_args_shell_splits_space_separated_entries() { + // Integration: a single comma-delimited entry containing spaces is + // shell-split into multiple argv elements. + assert_eq!( + normalize_agent_args("custom-agent", vec!["-m my-model --reasoning low".into()]), + vec!["-m", "my-model", "--reasoning", "low"] + ); + } + + #[test] + fn normalize_agent_args_preserves_comma_delimited_no_spaces() { + // Existing comma-delimited entries without spaces are unchanged. + assert_eq!( + normalize_agent_args("codex-acp", vec!["-c".into(), "model=gpt-5".into()]), + vec!["-c", "model=gpt-5"] + ); + } + #[test] fn normalizes_buzz_agent_args_to_empty() { assert_eq!( From 69049757be8f360e8ded1e3b5a921034acfc6792 Mon Sep 17 00:00:00 2001 From: Brad Groux <3053586+BradGroux@users.noreply.github.com> Date: Mon, 17 Aug 2026 00:23:54 -0500 Subject: [PATCH 2/4] Replace hand-written shell splitter with shlex crate The hand-written shell_split had three issues identified in review: - Malformed shell syntax (unmatched quote, trailing backslash) was silently reinterpreted, removing the quote/backslash and launching with different argv than the operator supplied. - Only literal space was treated as a delimiter; tabs and other whitespace were not split. - The README still said arguments were comma-only. Replace shell_split with shlex::split (already a transitive build-dependency via cc). Malformed entries are now preserved as-is with a warning instead of being mutated. All whitespace is detected. README documents both comma- and space-separated forms and the quoting contract. Addresses themiguelamador's review feedback. Co-authored-by: Brad Groux Signed-off-by: Brad Groux Signed-off-by: Brad Groux Signed-off-by: Brad Groux <3053586+BradGroux@users.noreply.github.com> --- Cargo.lock | 1 + crates/buzz-acp/Cargo.toml | 3 + crates/buzz-acp/README.md | 12 ++- crates/buzz-acp/src/config.rs | 138 ++++++++++------------------------ 4 files changed, 52 insertions(+), 102 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d5f80a5fc82..5ead7fc91af 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -847,6 +847,7 @@ dependencies = [ "serde", "serde_json", "sha2 0.11.0", + "shlex", "thiserror 2.0.18", "tokio", "tokio-tungstenite 0.29.0", diff --git a/crates/buzz-acp/Cargo.toml b/crates/buzz-acp/Cargo.toml index d047849806f..0d27259e9ed 100644 --- a/crates/buzz-acp/Cargo.toml +++ b/crates/buzz-acp/Cargo.toml @@ -68,6 +68,9 @@ clap = { version = "4", features = ["derive", "env"] } # Config file toml = "1.0" +# Shell-style argv splitting for BUZZ_ACP_AGENT_ARGS +shlex = "1.3" + # Filter expressions evalexpr = { workspace = true } diff --git a/crates/buzz-acp/README.md b/crates/buzz-acp/README.md index 41d9a214bdd..6954f319c47 100644 --- a/crates/buzz-acp/README.md +++ b/crates/buzz-acp/README.md @@ -109,13 +109,21 @@ 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; entries with whitespace are shell-split). | | `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 then +shell-split if it contains whitespace, so both forms work: + +- Comma-separated: `-c,key="value"` +- Space-separated (shell-quoted): `-c key="value"` (a single entry with spaces is + split into separate argv elements using standard shell quoting rules) + +Entries with malformed quoting (e.g. an unmatched single quote) are preserved as-is +with a warning rather than silently reinterpreted. **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 19142ad6beb..3471c88f948 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -875,7 +875,7 @@ pub fn normalize_agent_args(command: &str, agent_args: Vec) -> Vec) -> Vec parts, + None => { + // Malformed shell syntax (unmatched quote, trailing + // backslash): preserve the original entry rather than + // silently dropping or mutating it, and warn so the + // operator can fix the configuration. + tracing::warn!( + entry = %trimmed, + "BUZZ_ACP_AGENT_ARGS entry has malformed shell quoting; preserving as-is" + ); + vec![trimmed] + } + } } else { vec![trimmed] } @@ -910,60 +923,6 @@ pub fn normalize_agent_args(command: &str, agent_args: Vec) -> Vec Vec { - let mut tokens = Vec::new(); - let mut current = String::new(); - let mut in_single = false; - let mut in_double = false; - let mut escape = false; - let mut has_content = false; - - for ch in input.chars() { - if escape { - current.push(ch); - has_content = true; - escape = false; - continue; - } - match ch { - '\\' if !in_single => { - escape = true; - } - '\'' if !in_double => { - in_single = !in_single; - has_content = true; - } - '"' if !in_single => { - in_double = !in_double; - has_content = true; - } - c if c.is_whitespace() && !in_single && !in_double => { - if has_content { - tokens.push(std::mem::take(&mut current)); - has_content = false; - } - } - c => { - current.push(c); - has_content = true; - } - } - } - if has_content { - tokens.push(current); - } - tokens -} - /// Propagate legacy env-var aliases to their canonical names. /// /// Must be called **before** the tokio runtime starts — i.e. from the sync @@ -1758,76 +1717,55 @@ mod tests { ); } - // --- shell_split fallback for space-separated args (#6017) --- + // --- shlex-based splitting for space-separated args (#6017) --- #[test] - fn shell_split_space_separated_args() { - // The most common case: a user passes space-separated flags as one - // comma-delimited entry. shell_split must produce separate argv - // elements. + fn normalize_agent_args_shell_splits_space_separated_entries() { + // The most common case: a single comma-delimited entry containing + // spaces is shell-split into separate argv elements. assert_eq!( - shell_split("-m my-model --reasoning low"), + normalize_agent_args("custom-agent", vec!["-m my-model --reasoning low".into()]), vec!["-m", "my-model", "--reasoning", "low"] ); } #[test] - fn shell_split_preserves_single_arg_without_spaces() { - assert_eq!(shell_split("acp"), vec!["acp"]); - assert_eq!(shell_split("-c"), vec!["-c"]); + fn normalize_agent_args_preserves_comma_delimited_no_spaces() { + // Existing comma-delimited entries without spaces are unchanged. + assert_eq!( + normalize_agent_args("codex-acp", vec!["-c".into(), "model=gpt-5".into()]), + vec!["-c", "model=gpt-5"] + ); } #[test] - fn shell_split_handles_double_quoted_strings() { - // Standard shell behavior: quotes are consumed, not preserved. - // `model="gpt-5"` → `model=gpt-5` (mid-token quotes stripped). + fn normalize_agent_args_shell_splits_quoted_strings() { + // Quoted strings with spaces are preserved as single argv elements. assert_eq!( - shell_split("-c model=\"gpt-5\" --flag"), - vec!["-c", "model=gpt-5", "--flag"] - ); - // `"my model name"` → `my model name` (standalone quotes stripped, - // spaces inside preserved). - assert_eq!( - shell_split(r#"-c "my model name""#), + normalize_agent_args("custom-agent", vec![r#"-c "my model name""#.into()]), vec!["-c", "my model name"] ); - } - - #[test] - fn shell_split_handles_single_quoted_strings() { assert_eq!( - shell_split("-c 'my model name'"), + normalize_agent_args("custom-agent", vec!["-c 'my model name'".into()]), vec!["-c", "my model name"] ); } #[test] - fn shell_split_handles_backslash_escape() { - assert_eq!(shell_split(r"model=gpt\ 5"), vec!["model=gpt 5"]); - } - - #[test] - fn shell_split_empty_input() { - assert!(shell_split("").is_empty()); - assert!(shell_split(" ").is_empty()); - } - - #[test] - fn normalize_agent_args_shell_splits_space_separated_entries() { - // Integration: a single comma-delimited entry containing spaces is - // shell-split into multiple argv elements. + fn normalize_agent_args_shell_splits_tab_delimited() { + // Tabs are whitespace too — shlex treats them as delimiters. assert_eq!( - normalize_agent_args("custom-agent", vec!["-m my-model --reasoning low".into()]), - vec!["-m", "my-model", "--reasoning", "low"] + normalize_agent_args("custom-agent", vec!["-m\tmy-model".into()]), + vec!["-m", "my-model"] ); } #[test] - fn normalize_agent_args_preserves_comma_delimited_no_spaces() { - // Existing comma-delimited entries without spaces are unchanged. + fn normalize_agent_args_preserves_malformed_quoting() { + // Unmatched quote: shlex returns None, we preserve the original entry. assert_eq!( - normalize_agent_args("codex-acp", vec!["-c".into(), "model=gpt-5".into()]), - vec!["-c", "model=gpt-5"] + normalize_agent_args("custom-agent", vec!["-c 'unmatched".into()]), + vec!["-c 'unmatched"] ); } From 23bdc933a1ab27ee590b1417ba67805ab41e1345 Mon Sep 17 00:00:00 2001 From: Brad Groux <3053586+BradGroux@users.noreply.github.com> Date: Tue, 18 Aug 2026 15:06:52 -0500 Subject: [PATCH 3/4] Replace shlex splitting with BUZZ_ACP_AGENT_ARGS_JSON structured transport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ravarora2's review identified that shell-splitting every BUZZ_ACP_AGENT_ARGS entry containing whitespace breaks backward compatibility with structured argument producers. Desktop and the Kubernetes controller already hold agent arguments as Vec/lists, and an element containing whitespace may intentionally be one argv element. Once the list crosses the comma-delimited environment boundary, Buzz cannot reliably infer whether whitespace means one argument containing spaces or several shell words. The shlex approach also corrupted Windows paths (removing backslashes) and silently dropped text after # comment syntax. Replaced the heuristic with an explicit, lossless design: - BUZZ_ACP_AGENT_ARGS_JSON: new canonical JSON array of strings, parsed strictly with serde_json. No shell splitting, no character reinterpretation. Preserves spaces, backslashes, quotes, and empty arguments exactly. - BUZZ_ACP_AGENT_ARGS: legacy comma-delimited behavior, unchanged. Entries are preserved as-is without shell splitting. - Selection: neither set → default; JSON only → strict parse; legacy only → unchanged; both set → startup error; invalid JSON → startup error. - BUZZ_ACP_AGENT_ARGS_JSON added to Desktop reserved env keys (code-execution surface, same category as BUZZ_ACP_AGENT_ARGS). - shlex dependency removed from buzz-acp. Co-authored-by: Brad Groux Signed-off-by: Brad Groux Signed-off-by: Brad Groux Signed-off-by: Brad Groux <3053586+BradGroux@users.noreply.github.com> --- Cargo.lock | 1 - crates/buzz-acp/Cargo.toml | 3 - crates/buzz-acp/README.md | 29 ++- crates/buzz-acp/src/config.rs | 219 +++++++++++++----- .../src/managed_agents/env_vars/tests.rs | 1 + .../src/managed_agents/reserved_env_keys.rs | 1 + 6 files changed, 186 insertions(+), 68 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5ead7fc91af..d5f80a5fc82 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -847,7 +847,6 @@ dependencies = [ "serde", "serde_json", "sha2 0.11.0", - "shlex", "thiserror 2.0.18", "tokio", "tokio-tungstenite 0.29.0", diff --git a/crates/buzz-acp/Cargo.toml b/crates/buzz-acp/Cargo.toml index 0d27259e9ed..d047849806f 100644 --- a/crates/buzz-acp/Cargo.toml +++ b/crates/buzz-acp/Cargo.toml @@ -68,9 +68,6 @@ clap = { version = "4", features = ["derive", "env"] } # Config file toml = "1.0" -# Shell-style argv splitting for BUZZ_ACP_AGENT_ARGS -shlex = "1.3" - # Filter expressions evalexpr = { workspace = true } diff --git a/crates/buzz-acp/README.md b/crates/buzz-acp/README.md index 6954f319c47..7d895211134 100644 --- a/crates/buzz-acp/README.md +++ b/crates/buzz-acp/README.md @@ -109,21 +109,34 @@ 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; entries with whitespace are shell-split). | +| `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. Each comma-delimited entry is then -shell-split if it contains whitespace, so both forms work: +**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: -- Comma-separated: `-c,key="value"` -- Space-separated (shell-quoted): `-c key="value"` (a single entry with spaces is - split into separate argv elements using standard shell quoting rules) +```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"]' +``` -Entries with malformed quoting (e.g. an unmatched single quote) are preserved as-is -with a warning rather than silently reinterpreted. +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. **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 3471c88f948..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, @@ -875,29 +883,6 @@ pub fn normalize_agent_args(command: &str, agent_args: Vec) -> Vec parts, - None => { - // Malformed shell syntax (unmatched quote, trailing - // backslash): preserve the original entry rather than - // silently dropping or mutating it, and warn so the - // operator can fix the configuration. - tracing::warn!( - entry = %trimmed, - "BUZZ_ACP_AGENT_ARGS entry has malformed shell quoting; preserving as-is" - ); - vec![trimmed] - } - } } else { vec![trimmed] } @@ -923,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 @@ -1031,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 { @@ -1717,56 +1759,121 @@ mod tests { ); } - // --- shlex-based splitting for space-separated args (#6017) --- + // --- BUZZ_ACP_AGENT_ARGS_JSON structured transport (#6017) --- #[test] - fn normalize_agent_args_shell_splits_space_separated_entries() { - // The most common case: a single comma-delimited entry containing - // spaces is shell-split into separate argv elements. - assert_eq!( - normalize_agent_args("custom-agent", vec!["-m my-model --reasoning low".into()]), - vec!["-m", "my-model", "--reasoning", "low"] - ); + 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 normalize_agent_args_preserves_comma_delimited_no_spaces() { - // Existing comma-delimited entries without spaces are unchanged. - assert_eq!( - normalize_agent_args("codex-acp", vec!["-c".into(), "model=gpt-5".into()]), - vec!["-c", "model=gpt-5"] - ); + 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 normalize_agent_args_shell_splits_quoted_strings() { - // Quoted strings with spaces are preserved as single argv elements. - assert_eq!( - normalize_agent_args("custom-agent", vec![r#"-c "my model name""#.into()]), - vec!["-c", "my model name"] - ); + 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!( - normalize_agent_args("custom-agent", vec!["-c 'my model name'".into()]), - vec!["-c", "my model name"] + args, + vec!["--config=C:\\Program Files\\Agent\\config.toml"] ); } #[test] - fn normalize_agent_args_shell_splits_tab_delimited() { - // Tabs are whitespace too — shlex treats them as delimiters. - assert_eq!( - normalize_agent_args("custom-agent", vec!["-m\tmy-model".into()]), - vec!["-m", "my-model"] + 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 normalize_agent_args_preserves_malformed_quoting() { - // Unmatched quote: shlex returns None, we preserve the original entry. - assert_eq!( - normalize_agent_args("custom-agent", vec!["-c 'unmatched".into()]), - vec!["-c 'unmatched"] + 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] 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..16d791a4f9b 100644 --- a/desktop/src-tauri/src/managed_agents/env_vars/tests.rs +++ b/desktop/src-tauri/src/managed_agents/env_vars/tests.rs @@ -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"); 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. From 1aff9ebe4ef86e34fa90ee964dd44a0850ab5b88 Mon Sep 17 00:00:00 2001 From: Brad Groux <3053586+BradGroux@users.noreply.github.com> Date: Thu, 20 Aug 2026 09:18:14 -0500 Subject: [PATCH 4/4] Wire Desktop and Kubernetes producers to BUZZ_ACP_AGENT_ARGS_JSON MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ravarora2's review identified that the PR only added the JSON env var parsing in buzz-acp but did not update the actual producer paths — Desktop spawn, model probe, auth, and the Kubernetes backend still emit only the legacy comma-delimited BUZZ_ACP_AGENT_ARGS. This means normal Desktop/K8s configuration is not fixed; only manual env var users benefit. Wire all four producer paths to use set_agent_args_env, which selects the transport based on whether any argument contains a comma: - No commas: legacy comma-delimited BUZZ_ACP_AGENT_ARGS (backward compatible) - Any comma: JSON-serialized BUZZ_ACP_AGENT_ARGS_JSON + legacy set to default "acp" (which the harness treats as "not configured") Added agent_args_env() as a testable decision function returning (legacy_value, Option), with 4 unit tests covering comma-safe, empty, single-comma, and multi-comma cases. Added a K8s test for the comma-in-arg JSON transport path and updated the existing args test to verify JSON is absent for comma-safe args. Updated buzz-acp README to document producer behavior. Co-authored-by: Brad Groux Signed-off-by: Brad Groux Co-authored-by: Brad Groux Signed-off-by: Brad Groux Signed-off-by: Brad Groux Signed-off-by: Brad Groux <3053586+BradGroux@users.noreply.github.com> --- crates/buzz-acp/README.md | 8 ++++ crates/buzz-backend-kubernetes/src/env.rs | 38 +++++++++++++++--- desktop/src-tauri/src/commands/agent_auth.rs | 5 ++- .../src/commands/agent_model_process.rs | 4 +- .../src-tauri/src/managed_agents/env_vars.rs | 36 +++++++++++++++++ .../src/managed_agents/env_vars/tests.rs | 39 ++++++++++++++++++- desktop/src-tauri/src/managed_agents/mod.rs | 1 + .../src-tauri/src/managed_agents/runtime.rs | 2 +- 8 files changed, 121 insertions(+), 12 deletions(-) diff --git a/crates/buzz-acp/README.md b/crates/buzz-acp/README.md index 7d895211134..378b9ddd09d 100644 --- a/crates/buzz-acp/README.md +++ b/crates/buzz-acp/README.md @@ -138,6 +138,14 @@ 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. ### Parallel Agents & Heartbeat 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 16d791a4f9b..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, }; @@ -521,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/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);