feat(acp): accept extra MCP servers via BUZZ_ACP_EXTRA_MCP_COMMANDS - #6651
feat(acp): accept extra MCP servers via BUZZ_ACP_EXTRA_MCP_COMMANDS#6651BradGroux wants to merge 3 commits into
Conversation
3b695eb to
71f6b6f
Compare
|
Rebased onto the latest No review comments on this PR. CI will verify the build and test gate. |
71f6b6f to
6fde130
Compare
🔐 Codex Security Review
|
|
Rebased onto the latest Still-needed assessment: No upstream commits in the 62-commit window address the same issue. This PR remains needed. Review requests: |
6fde130 to
175624a
Compare
|
Rebased onto current Still needed: Checked all 38 new upstream commits for overlap with this PR's topic. None of the upstream changes address the issue this PR fixes. The PR remains relevant. Verification:
|
|
Operator context, for whatever it's worth: we run nine Desktop-managed agents in a fork, each wired to a per-project memory MCP server alongside Comma delimiter vs shlex. Credential isolation. The cwd. |
175624a to
4973a38
Compare
|
Thanks @rsaulo — this is exactly the kind of production field testing that catches the gaps a single-operator setup can't expose. All three points are addressed below. 1. Comma delimiter vs shlex — fixedYou're right: Fix: changed Added a test ( 2. Derived name collisions on reorder — fixedYou identified that Fix: added an optional Without an explicit name, the existing derive-from-stem + numeric suffix behavior is unchanged. Added two tests:
3. Credential isolation scope — README clarifiedYou're correct that the The README previously stated the isolation unconditionally. It now says:
This is the same hole you and we have in our forks. Plugging it at the adapter spawn boundary (env_clear + selective passthrough on 4. cwd — acknowledged, out of scopeYou're right that Rebased onto latest Thanks again for the thorough field report — the comma delimiter bug in particular would have bitten anyone with non-trivial command args. |
4973a38 to
28b0823
Compare
Rebase and review — 2026-09-03Base commit: Accuracy review: No extra MCP server support via env var on main. |
28b0823 to
ecf0d7b
Compare
Rebase and review update — Sep 3, 2026Rebased onto latest main ( Accuracy review: Main does not have Mergeable status: Confirmed MERGEABLE on GitHub after force-push. |
… credentials (#4) * feat(acp): accept extra MCP servers via BUZZ_ACP_EXTRA_MCP_COMMANDS Desktop-managed Buzz agents could only use one MCP server (buzz-dev-mcp). Users who wanted web search or other third-party MCP tools alongside the built-in local tools had no way to add them — the desktop flow hardcodes a single MCP command and build_mcp_servers() always returned a vec of one entry. Added BUZZ_ACP_EXTRA_MCP_COMMANDS, a comma-separated env var where each entry is split on whitespace into command + args. build_mcp_servers() appends each as a separate McpServer after the primary server. Extra servers do not receive Buzz relay credentials (BUZZ_RELAY_URL, BUZZ_PRIVATE_KEY) or auth tags — they are third-party tools, not Buzz-native MCP servers. The primary mcp_command short-circuit is preserved: if it is empty, no servers are returned at all, even when extra commands are configured. This is option 1 from issue block#6023. It unblocks the web-search use case (e.g. npx -y mcp-remote https://mcp.tavily.com/mcp/...) without desktop UI changes. Closes block#6023 Signed-off-by: dm-builder <f01486f036641ccb52c11bb1e0ff2346ea89a8b4e3b49cd772249948f6fcbae6@digitalmeld.communities.buzz.xyz> Co-authored-by: Brad Groux <bradgroux@users.noreply.github.com> Signed-off-by: Brad Groux <bradgroux@users.noreply.github.com> Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> * Fix extra MCP server name collisions and shell-aware argv parsing Two correctness issues in the extra MCP server path: 1. Server names derived only from the executable stem collided for common multi-server configurations. Two wrappers like 'npx -y first-mcp' and 'npx -y second-mcp' both became 'npx', tripping McpRegistry's duplicate check at spawn. Now disambiguate with a numeric suffix (npx, npx-2). 2. split_whitespace() corrupted quoted executable paths and arguments containing spaces. Replace with shlex::split, which handles standard shell quoting. Malformed entries are skipped with a warning instead of being silently reinterpreted. Also update the config doc comment to document the delimiter/quoting contract. Addresses themiguelamador's review feedback. Co-authored-by: Brad Groux <brad@digitalmeld.com> Signed-off-by: Brad Groux <brad@digitalmeld.com> Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> * 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> Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> * style(buzz-acp): rustfmt the imported PR block#6651 extra-MCP parser The upstream branch predates this repo's rustfmt run, so `just fmt-check` failed on the cherry-picked `build_mcp_servers` and `sanitize_mcp_name` bodies. No behavior change. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> * docs(mcp): name NOSTR_PRIVATE_KEY in the untrusted-server credential lists `is_buzz_identity_env` withholds four variables from an untrusted MCP server, but the doc comments and the buzz-acp README named only three, so a reader could conclude NOSTR_PRIVATE_KEY still reaches a third-party server. Also corrects the McpServer schema note, which said all four fields are required and now has a fifth, optional one. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> * test(buzz-agent): bind the untrusted MCP spawn boundary end to end Adds FAKE_MCP_ENV_REPORT to the fake MCP server: tools/call returns the sorted names (never the values) of the variables the server process was spawned with. Two integration tests drive a real buzz-agent child through session/new and a tool call: - an untrusted server receives none of BUZZ_PRIVATE_KEY, NOSTR_PRIVATE_KEY, BUZZ_RELAY_URL or BUZZ_AUTH_TAG, while a trusted one in the same session receives all four — so the withholding half cannot pass merely because the parent never had them; - the filter stays narrow: an untrusted server keeps PATH, HOME, BUZZ_ACP_DISPLAY_NAME and its wire-declared env. Removing the `!spec.trusted && is_buzz_identity_env(k)` guard fails the first test. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> * fix(acp): cut extra MCP server names to the registry's 128-byte limit `sanitize_mcp_name` truncated with an inclusive range (`sanitized[..=128]`), which keeps 129 bytes. `buzz-agent`'s `McpRegistry::spawn_all` rejects any name over `MAX_NAME_LEN` (128) and fails the whole `session/new`, so an executable path or `name=` prefix longer than 128 characters silently cost the agent every one of its MCP tools, not just the extra server. The disambiguation suffix had the same class of bug: `format!("{base_name}-{i}")` appended to a base name already at the ceiling, pushing it back over. - Cut on an exclusive bound through a shared `fit_mcp_name` helper, which also re-trims a hyphen the cut may expose and takes the cut on a character boundary so it cannot panic. - Re-cut the stem against the suffix width when disambiguating, including two-digit suffixes. - Name the limit `MAX_MCP_NAME_LEN` and point it at the constant it mirrors. Tests, each verified falsifiable against the pre-fix code: - `extra_mcp_commands_long_name_truncated_to_registry_limit` — a 130-byte explicit name and a 200-byte executable stem both come back at exactly 128 bytes (fails at 129 with the inclusive slice restored). - `extra_mcp_commands_disambiguation_suffix_respects_registry_limit` — 11 entries sharing a name already at the ceiling stay unique and within the limit (fails at 130 bytes with the plain `format!` restored). - `untrusted_mcp_env_extra_command_reaches_tool_list` — a new end-to-end test that runs the whole seam in one process: the real `BUZZ_ACP_EXTRA_MCP_COMMANDS` variable, `buzz-acp`'s own argument parser, `build_mcp_servers`, a real `buzz-agent` child, `session/new` off the wire, two real MCP subprocesses, and the tool list the agent offers the model. It lives in its own test binary because it writes a process-global environment variable. Previously the two halves of this seam were tested apart, which is how a 129-byte name reached `session/new` unnoticed. `mcp_servers_wire_json` and the `CliArgs`/`Config`/`ConfigError` re-exports make that end-to-end test possible: it drives the harness's real parser rather than a hand-written copy of the wire shape that could drift from it. Also documents, in the flag help and the README, that a name near the 128-byte ceiling still fails because `buzz-agent` caps the qualified tool name `<server>__<tool>` at 64 bytes. That tighter bound is left as upstream has it — narrowing it would change behaviour the port is carrying. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> * fix(buzz-agent): confine MCP hooks and declared env to trusted servers Two holes in the trust boundary this port introduces, both reachable once BUZZ_ACP_EXTRA_MCP_COMMANDS can put a `trusted: false` server in the same mcpServers array as the built-in one. McpRegistry::call_hooks picked its targets from the MCP_HOOK_SERVERS allowlist alone and never read spec.trusted. Desktop launches hook-capable runtimes with a wildcard allowlist, so an operator-supplied third-party server advertising _Stop or _PostCompact became an agent-control surface: _Stop decides whether a turn may end, and _PostCompact text is spliced into the fresh context after a handoff. call_hooks now requires spec.trusted. spawn_one dropped the four Buzz identity variables from the ambient passthrough set for an untrusted server, then applied the wire-declared spec.env four lines later without the same filter — so a credential named in mcpServers[].env reached the child regardless of the flag. The declared env is filtered on the same rule. Desktop names the built-in server in MCP_HOOK_SERVERS instead of passing "*", derived from the MCP command's file stem the way build_mcp_servers derives it. The comment justifying the wildcard claimed the name was hard-coded to "buzz-mcp", which was never true and is now moot. Tests: hook_never_invoked_on_untrusted_server pins the hook guard (removing it makes the agent loop on the objection: 1 LLM call becomes 2), and untrusted_mcp_env_withheld_when_declared_on_the_wire pins the env guard against an agent process that has no ambient identity variables at all, so the wire declaration is the only possible source. The existing hook tests now declare their fake server trusted, as the harness does for its own. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> * fix(buzz-acp): correct the extra-MCP parser's contracts and bind its wire seam Four defects in build_mcp_servers, three of them inherited verbatim from PR block#6651 and reported upstream as parity bugs, not fork regressions. An extras-only configuration produced zero servers. The function returned early when config.mcp_command was empty, before the extras loop — BUZZ_ACP_MCP_COMMAND defaults to "" and Desktop writes "" whenever the runtime has no MCP command, so setting only BUZZ_ACP_EXTRA_MCP_COMMANDS started the harness clean and offered the model no tools at all. The primary server is now appended conditionally and the extras always parsed; only an entirely unset configuration yields an empty array. The test that pinned the old behaviour as correct is replaced by one asserting the extra server survives. A non-blank entry that shell-split to nothing was dropped at a silent `continue`: `memory=`, whitespace after the `=`, an empty quoted command, or a comment-only line each started the harness with that server missing and nothing logged, while malformed quoting correctly aborted startup. Both shapes now fail closed with the entry index and no echo of the entry. The generated-name cap enforced the wrong contract. MAX_MCP_NAME_LEN mirrored buzz-agent's 128-byte bound on the name alone, but the registry builds `<server>__<tool>` and fails the whole session above its 64-byte MAX_QNAME_LEN — so a name cut to exactly 128 bytes was still fatal the moment the server advertised any tool. The cap is budgeted against the qualified name instead, and assert_registry_name_contract now covers that bound rather than describing the looser one. Buzz origin metadata was injected into untrusted extras. mcp_servers_with_git_origin pushed BUZZ_GIT_ORIGIN_CHANNEL_ID or BUZZ_GIT_ORIGIN_AGENT_NAME into every element of the array; before this port the array held exactly one server, and it now holds third-party ones. The origin goes only to trusted servers, and the pool tests run a mixed array so the guard is falsifiable. The last hop is now bound as well. The e2e test called mcp_servers_wire_json and hand-sent session/new, so replacing the PromptContext assignment with vec![] or breaking the pool's env injection left it green. Both steps live behind one type: McpServerSet::from_config is the only way to fill PromptContext::mcp_servers (from_servers is cfg(test)) and for_session is the only way to read it back, so the wire array cannot be produced any other way and the test drives exactly the code the harness runs. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> * fix(buzz-agent): contain an over-long third-party tool name; narrow the trust claims An untrusted MCP server's tool names are outside this repo's control: `valid_name` accepts 128 bytes, while a server name generated by buzz-acp can only reserve about 30 for the bare tool. A 32-byte server name plus a real 32-byte tool (`get_google_search_console_report`) makes a 66-byte qualified name, and `spawn_all` returned Err for the whole array — so one operator-supplied server took the built-in one down with it and the agent started a session with no tools at all. Drop that one tool, with a warning, when the server is untrusted; a trusted server keeps the hard error, because there an over-long name is our own packaging bug. Narrow what the spawn filter is documented to buy. It removes four variable names from the child's environment. It is not isolation: the child runs under the same UID with HOME and SSH_AUTH_SOCK, so it can read what the agent's user can, including a plaintext nsec in managed-agents.json when the keyring is unreachable. Real per-server isolation belongs to the MCP-registry ticket. `trusted` is `#[serde(default)]`, so an ACP client that predates the extension declares every server untrusted — fail-safe for credentials, a behavior change for hooks. Document it on the field and in the README, and stop failing silently: an allowlisted-but-untrusted server is now logged once per registry, and an untrusted spawn says so. Tests: a real-process test with a 32-byte server name and a 32-byte tool (the session survives, the fitting tools are still offered, the long one is not), and a hook test whose declaration carries no `trusted` key at all. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> * fix(buzz-acp): refuse extra MCP servers an adapter cannot honour, and bound them The credential-withholding guarantee these servers are declared under is `buzz-agent`'s: it alone reads the `trusted` marker. `BUZZ_ACP_AGENT_COMMAND` defaults to `goose`, `AcpClient::spawn` never clears the environment, and the extras went into every `session/new` regardless — so with any other adapter the entries were spawned by a process holding BUZZ_PRIVATE_KEY and BUZZ_RELAY_URL, and the marker, the spawn filter and their tests all sat off that path. Gate the extras on the same normalized command identity `default_agent_args` keys on, and fail at boot with a message that names the adapter. Mirror the registry's 16-server cap. `spawn_all` rejects the whole array past it, so without a startup bound a 17-server configuration produced a harness that came online and then failed every session — the same class the MAX_QNAME_LEN mirror fixed. A re-export from buzz-agent lets a test pin the two constants together instead of a comment. Make the `name=` promise true. Two entries claiming one explicit name are now refused instead of one being silently renamed by position, and a disambiguation suffix is hashed from the entry itself rather than counted off in order, so reordering two names that truncate alike no longer swaps which executable owns each qualified tool name. Build the server set before presence goes online, so an unusable configuration stops the process at boot rather than after it advertises itself. Narrow the doc comments to what the trust marker actually buys, and replace the CLI/README example that put an API key in argv. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> --------- Signed-off-by: dm-builder <f01486f036641ccb52c11bb1e0ff2346ea89a8b4e3b49cd772249948f6fcbae6@digitalmeld.communities.buzz.xyz> Signed-off-by: Brad Groux <bradgroux@users.noreply.github.com> Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> Signed-off-by: Brad Groux <brad@digitalmeld.com> Co-authored-by: Brad Groux <3053586+BradGroux@users.noreply.github.com> Co-authored-by: Brad Groux <bradgroux@users.noreply.github.com> Co-authored-by: Brad Groux <brad@digitalmeld.com>
Desktop-managed Buzz agents could only use one MCP server (buzz-dev-mcp). Users who wanted web search or other third-party MCP tools alongside the built-in local tools had no way to add them — the desktop flow hardcodes a single MCP command and build_mcp_servers() always returned a vec of one entry. Added BUZZ_ACP_EXTRA_MCP_COMMANDS, a comma-separated env var where each entry is split on whitespace into command + args. build_mcp_servers() appends each as a separate McpServer after the primary server. Extra servers do not receive Buzz relay credentials (BUZZ_RELAY_URL, BUZZ_PRIVATE_KEY) or auth tags — they are third-party tools, not Buzz-native MCP servers. The primary mcp_command short-circuit is preserved: if it is empty, no servers are returned at all, even when extra commands are configured. This is option 1 from issue block#6023. It unblocks the web-search use case (e.g. npx -y mcp-remote https://mcp.tavily.com/mcp/...) without desktop UI changes. Closes block#6023 Signed-off-by: dm-builder <f01486f036641ccb52c11bb1e0ff2346ea89a8b4e3b49cd772249948f6fcbae6@digitalmeld.communities.buzz.xyz> Co-authored-by: Brad Groux <bradgroux@users.noreply.github.com> Signed-off-by: Brad Groux <bradgroux@users.noreply.github.com> Signed-off-by: Brad Groux <bradgroux@hotmail.com> Signed-off-by: Brad Groux <3053586+BradGroux@users.noreply.github.com>
Two correctness issues in the extra MCP server path: 1. Server names derived only from the executable stem collided for common multi-server configurations. Two wrappers like 'npx -y first-mcp' and 'npx -y second-mcp' both became 'npx', tripping McpRegistry's duplicate check at spawn. Now disambiguate with a numeric suffix (npx, npx-2). 2. split_whitespace() corrupted quoted executable paths and arguments containing spaces. Replace with shlex::split, which handles standard shell quoting. Malformed entries are skipped with a warning instead of being silently reinterpreted. Also update the config doc comment to document the delimiter/quoting contract. Addresses themiguelamador's review feedback. Co-authored-by: Brad Groux <brad@digitalmeld.com> Signed-off-by: Brad Groux <brad@digitalmeld.com> Signed-off-by: Brad Groux <bradgroux@hotmail.com> Signed-off-by: Brad Groux <3053586+BradGroux@users.noreply.github.com>
…e 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> Signed-off-by: Brad Groux <bradgroux@hotmail.com> Signed-off-by: Brad Groux <3053586+BradGroux@users.noreply.github.com>
ecf0d7b to
a878d1e
Compare
Rebase pass — Sep 5, 2025Rebased onto current upstream main ( Rebase resultConflict resolved in desktop/src-tauri/src/managed_agents/reserved_env_keys.rs (upstream added PI_ACP_PI_COMMAND to reserved keys, PR added BUZZ_ACP_EXTRA_MCP_COMMANDS; kept both). Still-needed assessmentChecked all 62 new main commits for overlap with this PR's topic. None obsoleted:
Verification
|
2 participants
What users saw
Desktop-managed Buzz agents could receive only the built-in
buzz-dev-mcpserver. Operators had no process-level way to attach additional MCP servers to the ACP session.What changed
BUZZ_ACP_EXTRA_MCP_COMMANDSfor newline-separated extra MCP server commands.name=commandprefix so operators can pin server names explicitly — reordering entries no longer silently renames a server and strips the agent of its tools.buzz-agentMCP spawn path.trustedflag covers MCP servers spawned bybuzz-agent'sMcpRegistry. Third-party ACP adapters (claude-agent-acp, codex-acp) that spawn their own MCP children inherit the full parent environment includingBUZZ_PRIVATE_KEY; the README now states this explicitly.Example:
Verification
cargo test -p buzz-acp --lib(918 tests pass, including 22build_mcp_serverstests)cargo test -p buzz-agent --lib(525 pass)cargo clippy -p buzz-acp --all-targets -- -D warnings(clean)Non-goals
Closes #6023