Address P0/P1 review feedback: credential isolation, fail-closed, name sanitization

Four issues from wesbillman/Carl's review:

P0 — Extra MCP processes received Buzz identity credentials despite the
PR's isolation claim. build_mcp_servers gave extras an empty per-server
env, but buzz-agent's spawn_one cleared and repopulated every MCP child's
environment from PASSTHROUGH_ENV, which includes BUZZ_PRIVATE_KEY,
BUZZ_RELAY_URL, and BUZZ_AUTH_TAG. Added a trusted flag to McpServer and
McpServerStdio; spawn_one now withholds identity credentials from
untrusted servers. The primary buzz-dev-mcp server is trusted; extras are
not.

P1 — BUZZ_ACP_EXTRA_MCP_COMMANDS was absent from Desktop's reserved env
key list, allowing a portable persona or per-agent env to inject an
arbitrary command. Added to reserved_env_keys.rs and its test.

P1 — Malformed quoting logged the raw command (which may contain an
embedded API key) and silently skipped the entry. Now fails closed with
only the entry index; the raw command is never echoed.

P1 — Generated names used the executable stem verbatim, violating
McpRegistry's ASCII alphanumeric/hyphen and 128-byte contract. Added
sanitize_mcp_name to replace non-conforming characters with hyphens,
strip leading/trailing hyphens, and truncate to 128 bytes.

Co-authored-by: Brad Groux <brad@digitalmeld.com>
Signed-off-by: Brad Groux <brad@digitalmeld.com>

Author: 2 people

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_EXTRA_MCP_COMMANDS` | no | — | Newline-separated additional MCP server commands. Each entry is shell-split with POSIX quoting (e.g. `npx -y my-mcp-server`). An optional `name=` prefix sets the server name explicitly (e.g. `memory=npx -y memory-mcp`), so reordering entries does not silently rename a server and strip the agent of its tools. Without a prefix, names are derived from the executable stem, sanitized to ASCII alphanumeric/hyphen, and disambiguated with numeric suffixes. Extra servers do **not** receive `BUZZ_PRIVATE_KEY`, `BUZZ_RELAY_URL`, or `BUZZ_AUTH_TAG` on the `buzz-agent` MCP spawn path — they are third-party tools, not Buzz-native MCP. **Note:** the credential isolation applies to MCP servers spawned directly by `buzz-agent`'s `McpRegistry`. When using a third-party ACP adapter (e.g. `claude-agent-acp`, `codex-acp`) that spawns its own MCP children, the adapter process inherits the full parent environment including `BUZZ_PRIVATE_KEY`; operators should assume those children can access Buzz credentials unless the adapter itself isolates them. Malformed quoting fails startup with the entry index (the raw command is not logged). |
 | `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). |

crates/buzz-acp/src/acp.rs

@@ -26,12 +26,23 @@ const MAX_LINE_SIZE: usize = 10_000_000; // 10 MB
 ///
 /// Corresponds to the `McpServerStdio` variant in the ACP schema.
 /// All four fields are **required** by the schema (`args` and `env` may be empty arrays).
+/// `trusted` controls whether the agent runtime passes Buzz identity credentials
+/// (`BUZZ_PRIVATE_KEY`, `BUZZ_RELAY_URL`, `BUZZ_AUTH_TAG`) into the child process.
+/// Only the built-in `buzz-dev-mcp` server is trusted; extra MCP servers
+/// configured via `BUZZ_ACP_EXTRA_MCP_COMMANDS` are untrusted and receive no
+/// Buzz credentials.
 #[derive(Debug, Clone, serde::Serialize)]
 pub struct McpServer {
     pub name: String,
     pub command: String,
     pub args: Vec<String>,
     pub env: Vec<EnvVar>,
+    #[serde(default, skip_serializing_if = "is_false")]
+    pub trusted: bool,
+}
+
+fn is_false(b: &bool) -> bool {
+    !b
 }
 
 /// A single environment variable for an MCP server.
@@ -2538,6 +2549,7 @@ mod tests {
                     value: "nsec1abc".into(),
                 },
             ],
+            trusted: true,
         };
         let serialized = serde_json::to_value(&server).unwrap();
         assert_eq!(serialized["name"].as_str(), Some("test-mcp"));

crates/buzz-acp/src/config.rs

@@ -268,14 +268,18 @@ pub struct CliArgs {
     pub mcp_command: String,
 
     /// Additional MCP server commands to pass to the agent session alongside
-    /// the primary MCP server. Entries are comma-separated; each entry is
+    /// the primary MCP server. Entries are newline-separated; each entry is
     /// shell-split (shlex) into a command and its args, so quoted paths and
-    /// arguments with spaces are preserved. Server names are derived from the
-    /// executable stem and disambiguated with a numeric suffix if duplicates
-    /// occur (e.g. two `npx` wrappers become `npx` and `npx-2`). Entries with
-    /// malformed quoting are skipped with a warning. Example:
-    /// `npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=...,other-server`
-    #[arg(long, env = "BUZZ_ACP_EXTRA_MCP_COMMANDS", value_delimiter = ',')]
+    /// arguments with spaces are preserved. An optional `name=` prefix sets
+    /// the server name explicitly (e.g. `memory=npx -y memory-mcp`); without
+    /// it, the name is derived from the executable stem. Duplicate names are
+    /// disambiguated with a numeric suffix (e.g. two `npx` wrappers become
+    /// `npx` and `npx-2`), but explicit names are preferred so reordering
+    /// entries does not silently rename a server. Entries with malformed
+    /// quoting fail startup with the entry index (the raw command is not
+    /// echoed). Example:
+    /// `memory=npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=...\nother-server --port 8080`
+    #[arg(long, env = "BUZZ_ACP_EXTRA_MCP_COMMANDS", value_delimiter = '\n')]
     pub extra_mcp_commands: Vec<String>,
 
     /// Idle timeout: max seconds of silence before killing a turn.