mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(acp): per-runtime env defaults at spawn — isolate Hermes from configured MCP startup (#3420)
## Summary - add a generic per-runtime env-defaults table, `config::default_agent_env()`, mirroring the existing `default_agent_args()` / `codex_network_env()` precedent, and merge it once in `AcpClient::spawn` with the established precedence: **runtime defaults < persona `extra_env` < inherited parent env** - first (and only) row: Buzz-owned Hermes processes get `HERMES_ACP_SKIP_CONFIGURED_MCP=1`, so Hermes does not preload unrelated profile-configured MCP servers before answering ACP `initialize` (fixes the 10s model-discovery timeout in #3355 — Buzz supplies session MCP servers explicitly through `session/new`, per Hermes's documented host-integration contract for this variable) - normalize Windows `.cmd`/`.bat` shims alongside `.exe` in `normalize_agent_command_identity` (npm installs resolve to those wrappers) - switch the `extra_env` parent-presence check from `var()` to `var_os()` so non-UTF-8 parent values are honored Replaces the runtime-specific approach in #3386: same behavior, but the mechanism is generic runtime spawn metadata in `config.rs` rather than a Hermes/ACP special case in `acp.rs`, and the seam covers every launch path (Desktop spawn, `buzz-acp models`, CLI) because they all funnel through `AcpClient::spawn`. ~15 lines of production code. Fixes #3355 ## Testing - `cargo test -p buzz-acp` — **639 passed, 0 failed** (full package, includes the new `default_agent_env_recognizes_hermes_identities` unit test and `spawn_applies_runtime_env_defaults_with_extra_env_precedence` integration test covering default injection, extra_env override, and non-Hermes exclusion) - `cargo fmt --all -- --check`, `cargo clippy -p buzz-acp --all-targets -- -D warnings` — clean - live-local with real Hermes v0.19.0 (`hermes-acp`): `buzz-acp models` returned **13 models / currentModelId in 2.6–3.0s** (was a 10.0s timeout on the first cold run without isolation); a wrapper probe confirmed the child received `HERMES_ACP_SKIP_CONFIGURED_MCP=1` by default and `0` when the parent env set it explicitly (operator wins) - lefthook pre-push suite green: rust-tests, desktop-check, desktop-test, desktop-tauri-test, mobile-test, branch-skew No UI changes; subprocess environment behavior only. Signed-off-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: Tyler Longwell <tlongwell@block.xyz> Co-authored-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Co-authored-by: Tyler Longwell <tlongwell@block.xyz> Co-authored-by: mr-r0b0t.eth <adam.manning@pro-serveinc.com>
This commit is contained in:
co-authored by
npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d
Tyler Longwell
mr-r0b0t.eth
parent
22be8bb351
commit
6300a6b1d0
@@ -494,12 +494,22 @@ impl AcpClient {
|
||||
// entry falls through to the standard operator-wins treatment below.
|
||||
let codex_merge_active = codex_config_value.is_some();
|
||||
|
||||
// Per-runtime environment defaults (e.g. Hermes MCP-startup isolation).
|
||||
// Applied first so both persona `extra_env` (below, via `Command::env`
|
||||
// key replacement) and inherited parent env (via the parent-presence
|
||||
// check) override them.
|
||||
for &(key, value) in crate::config::default_agent_env(command) {
|
||||
if std::env::var_os(key).is_none() {
|
||||
cmd.env(key, value);
|
||||
}
|
||||
}
|
||||
|
||||
for (key, value) in extra_env {
|
||||
if key == "CODEX_CONFIG" && codex_merge_active {
|
||||
// Handled by build_codex_config_env; skip here to avoid double-setting.
|
||||
continue;
|
||||
}
|
||||
if std::env::var(key).is_err() {
|
||||
if std::env::var_os(key).is_none() {
|
||||
cmd.env(key, value);
|
||||
}
|
||||
}
|
||||
@@ -2891,6 +2901,78 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// Spawn a probe script whose file name carries a runtime identity (e.g.
|
||||
/// `hermes-acp`) and return the value of `var` as the child observed it.
|
||||
/// `<unset>` means the child did not receive the var.
|
||||
#[cfg(unix)]
|
||||
async fn spawn_named_and_read_child_env(
|
||||
file_name: &str,
|
||||
var: &str,
|
||||
extra_env: &[(String, String)],
|
||||
) -> String {
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
|
||||
let dir = std::env::temp_dir().join(format!("buzz-acp-env-probe-{}", uuid::Uuid::new_v4()));
|
||||
std::fs::create_dir_all(&dir).expect("create env probe dir");
|
||||
let path = dir.join(file_name);
|
||||
std::fs::write(
|
||||
&path,
|
||||
format!("#!/bin/sh\nprintf '%s\\n' \"${{{var}:-<unset>}}\"\n"),
|
||||
)
|
||||
.expect("write env probe script");
|
||||
let mut permissions = std::fs::metadata(&path).expect("stat probe").permissions();
|
||||
permissions.set_mode(0o700);
|
||||
std::fs::set_permissions(&path, permissions).expect("chmod probe");
|
||||
|
||||
let mut client = AcpClient::spawn(
|
||||
path.to_str().expect("probe path is UTF-8"),
|
||||
&[],
|
||||
extra_env,
|
||||
false,
|
||||
)
|
||||
.await
|
||||
.expect("spawn env probe script");
|
||||
let observed = client
|
||||
.reader
|
||||
.next()
|
||||
.await
|
||||
.unwrap_or_else(|| panic!("child produced no output for {var}"))
|
||||
.expect("child stdout was not readable");
|
||||
client.shutdown().await;
|
||||
std::fs::remove_dir_all(&dir).expect("remove env probe dir");
|
||||
observed
|
||||
}
|
||||
|
||||
/// Buzz-owned Hermes processes get the configured-MCP isolation default,
|
||||
/// and an explicit persona entry still overrides it (defaults are applied
|
||||
/// before `extra_env`, so the later `Command::env` write wins).
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn spawn_applies_runtime_env_defaults_with_extra_env_precedence() {
|
||||
const VAR: &str = "HERMES_ACP_SKIP_CONFIGURED_MCP";
|
||||
if std::env::var_os(VAR).is_some() {
|
||||
// Inherited parent values win over both layers; the default and
|
||||
// override behavior below is unobservable in such an environment.
|
||||
return;
|
||||
}
|
||||
|
||||
assert_eq!(
|
||||
spawn_named_and_read_child_env("hermes-acp", VAR, &[]).await,
|
||||
"1",
|
||||
"Hermes spawns must default {VAR}=1"
|
||||
);
|
||||
assert_eq!(
|
||||
spawn_named_and_read_child_env("hermes-acp", VAR, &[(VAR.into(), "0".into())]).await,
|
||||
"0",
|
||||
"an explicit extra_env entry must override the runtime default"
|
||||
);
|
||||
assert_eq!(
|
||||
spawn_named_and_read_child_env("other-agent", VAR, &[]).await,
|
||||
"<unset>",
|
||||
"non-Hermes spawns must not receive Hermes defaults"
|
||||
);
|
||||
}
|
||||
|
||||
/// Persona config must not be able to re-enable the scheduler: this is a
|
||||
/// correctness invariant, not an operator-tunable default, so the
|
||||
/// injection is set after (and therefore wins over) the `extra_env` loop.
|
||||
|
||||
@@ -676,7 +676,12 @@ pub(crate) fn normalize_agent_command_identity(command: &str) -> String {
|
||||
.next()
|
||||
.expect("rsplit always yields at least one element");
|
||||
let lower = basename.to_ascii_lowercase();
|
||||
let stem = lower.strip_suffix(".exe").unwrap_or(&lower);
|
||||
// Windows resolves commands through `.exe` binaries and npm's `.cmd`/`.bat`
|
||||
// shims; all three name the same runtime identity.
|
||||
let stem = [".exe", ".cmd", ".bat"]
|
||||
.iter()
|
||||
.find_map(|extension| lower.strip_suffix(extension))
|
||||
.unwrap_or(&lower);
|
||||
stem.chars()
|
||||
.map(|character| match character {
|
||||
' ' | '_' => '-',
|
||||
@@ -694,6 +699,25 @@ fn default_agent_args(command: &str) -> Option<Vec<String>> {
|
||||
}
|
||||
}
|
||||
|
||||
/// Per-runtime environment defaults applied when Buzz owns the agent process.
|
||||
///
|
||||
/// Mirrors [`default_agent_args`]: keyed on the normalized command identity,
|
||||
/// with the merge (in `AcpClient::spawn`) giving explicit persona env and
|
||||
/// inherited parent env precedence over these defaults.
|
||||
///
|
||||
/// Hermes: ACP hosts supply session MCP servers explicitly through
|
||||
/// `session/new`, but Hermes otherwise starts every profile-configured MCP
|
||||
/// server before it responds to `initialize` — which can exhaust the host's
|
||||
/// startup budget (see block/buzz#3355). Skip that unrelated global startup
|
||||
/// by default; an operator or persona can still opt back in by setting the
|
||||
/// variable explicitly.
|
||||
pub(crate) fn default_agent_env(command: &str) -> &'static [(&'static str, &'static str)] {
|
||||
match normalize_agent_command_identity(command).as_str() {
|
||||
"hermes" | "hermes-agent" | "hermes-acp" => &[("HERMES_ACP_SKIP_CONFIGURED_MCP", "1")],
|
||||
_ => &[],
|
||||
}
|
||||
}
|
||||
|
||||
/// Build the `CODEX_CONFIG` environment variable that enables full outbound
|
||||
/// network access in Codex's macOS Seatbelt sandbox.
|
||||
///
|
||||
@@ -1589,6 +1613,15 @@ mod tests {
|
||||
"claude-code"
|
||||
);
|
||||
assert_eq!(normalize_agent_command_identity("Goose.EXE"), "goose");
|
||||
// Windows npm shims resolve to `.cmd`/`.bat` wrappers.
|
||||
assert_eq!(
|
||||
normalize_agent_command_identity(r"C:\Users\test\AppData\Roaming\npm\hermes-acp.cmd"),
|
||||
"hermes-acp"
|
||||
);
|
||||
assert_eq!(
|
||||
normalize_agent_command_identity(r"C:\Tools\Hermes\HERMES-AGENT.BAT"),
|
||||
"hermes-agent"
|
||||
);
|
||||
// Non-ASCII must not panic.
|
||||
assert_eq!(normalize_agent_command_identity("my-agënt"), "my-agënt");
|
||||
// Edge cases: empty, whitespace-only, bare separators.
|
||||
@@ -1598,6 +1631,30 @@ mod tests {
|
||||
assert_eq!(normalize_agent_command_identity("///"), "");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn default_agent_env_recognizes_hermes_identities() {
|
||||
for command in [
|
||||
"hermes",
|
||||
"hermes-agent",
|
||||
"hermes-acp",
|
||||
"/opt/hermes/bin/hermes-acp",
|
||||
r"C:\Users\test\bin\HERMES_ACP.EXE",
|
||||
r"C:\Users\test\AppData\Roaming\npm\hermes-acp.cmd",
|
||||
] {
|
||||
assert_eq!(
|
||||
default_agent_env(command),
|
||||
&[("HERMES_ACP_SKIP_CONFIGURED_MCP", "1")],
|
||||
"unexpected env defaults for {command}"
|
||||
);
|
||||
}
|
||||
for command in ["goose", "codex-acp", "claude-agent-acp", "buzz-agent", ""] {
|
||||
assert!(
|
||||
default_agent_env(command).is_empty(),
|
||||
"non-Hermes command must have no env defaults: {command}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn strips_legacy_acp_arg_case_insensitively() {
|
||||
assert_eq!(
|
||||
|
||||
Reference in New Issue
Block a user