mirror of
https://github.com/block/buzz.git
synced 2026-10-31 22:41:12 +00:00
feat(acp): wrap workers at the subprocess launch boundary (#7985)
## Summary Replacing `BUZZ_ACP_AGENT_COMMAND` with a sandbox launcher hides the real adapter from Buzz: for example, Goose loses its default `acp` argument. Add optional `BUZZ_ACP_LAUNCH_PREFIX`, a JSON argument array applied at the shared subprocess boundary, after normal worker configuration. On Unix, every spawn runs `prefix... worker args...` without shell interpolation. Worker identity, environment setup, stdio and existing process cleanup are preserved. Invalid or unavailable prefixes fail launch without falling back to the worker; unset keeps direct launch on every platform. Configured prefixes fail closed on non-Unix because per-worker process-tree cleanup is unavailable. Desktop reserves the key against saved user-environment overrides, and spawn failures name the executable. ### Related issue Prerequisite for [Buzz-App #415](https://github.com/block/buzz-app/pull/415). One commit directly on `main`, independent of #7942–#7944. No duplicate launch-prefix PR found. Policy enforcement, verified launcher staging, protected paths and the supporting runtime pin remain Buzz-App/plugin work. ### Testing - Original head `9ae9d82f8`: complete ACP suite (990 passed, three existing ignored), shipped local-task test, real-Goose probe, and full repository-wide `just ci` passed. - Review fixes: five launch tests and 44 Desktop environment-filter tests pass locally, covering the real production platform gate, save rejection, both merge layers, case variants, and executable error context. - Subprocess tests cover direct/wrapped argument defaults, Pi skills, Hermes/Codex environment, adapter identity, repeated spawns and invalid/missing prefix refusal. - Shipped `buzz-acp run` entrypoint completes a task through the wrapper and deterministic ACP peer. - Real Goose 1.52.0: direct and wrapped `auth-methods` return identical results; the wrapper receives `goose acp` with initially empty worker args. - Independent agent review: no blocking findings. - Required push checks passed on the updated head. The existing native Windows CI job now explicitly runs the platform-contract tests. Its result is pending; the non-Unix refusal branch was not exercised on this macOS host. Repeated-spawn coverage is not a full lazy-pool wake/crash test. Protected Goose in the Buzz-App UI and human acceptance remain pending. To try the hook after building `buzz-acp`, use an installed Goose path: ```sh BUZZ_ACP_LAUNCH_PREFIX='["/usr/bin/env"]' target/debug/buzz-acp auth-methods \ --agent-command /absolute/path/to/goose --agent-args '' --json ``` Expect the same authentication methods as without the prefix. Changing the prefix to `["/missing-launcher"]` must fail. Draft pending human testing. Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
This commit is contained in:
3
.github/workflows/_ci-rust.yml
vendored
3
.github/workflows/_ci-rust.yml
vendored
@@ -151,6 +151,9 @@ jobs:
|
||||
# Serial: windows_resolver_tests mutate process-global env
|
||||
# (BUZZ_SHELL/GIT_BASH/SystemRoot) that SharedState::new reads.
|
||||
run: cargo test -p buzz-dev-mcp --target $env:TARGET -- --test-threads=1
|
||||
- name: Test (ACP launch-prefix platform contract)
|
||||
# Execute the non-Unix refusal branch; a cross-compile cannot test it.
|
||||
run: cargo test -p buzz-acp --target $env:TARGET --lib acp::launch::platform_tests
|
||||
- name: Test (buzz-agent auth coordinator)
|
||||
# The auth coordinator single-flights on an OS advisory lock, which is
|
||||
# LockFileEx on Windows; this integration suite drives real second
|
||||
|
||||
@@ -197,6 +197,34 @@ buzz-acp --respond-to anyone
|
||||
buzz-acp --respond-to nobody --heartbeat-interval 300
|
||||
```
|
||||
|
||||
### Host-controlled launch wrapper
|
||||
|
||||
On Unix, a trusted host can set `BUZZ_ACP_LAUNCH_PREFIX` to a JSON argument array, for
|
||||
example `["/absolute/launcher", "--policy", "/absolute/policy.json", "--"]`.
|
||||
Keep `BUZZ_ACP_AGENT_COMMAND` set to the real worker (such as Goose). The harness
|
||||
launches `prefix... worker normalized-args...` without a shell, preserving worker
|
||||
identity for argument defaults, environment setup, managed skills and ACP handling.
|
||||
|
||||
Configured prefixes are rejected on non-Unix platforms, including Windows, because
|
||||
per-worker process-tree cleanup is not available there. Unset direct launches
|
||||
remain available on every platform.
|
||||
|
||||
Hosts must reserve this key against user-supplied environment settings and apply
|
||||
the trusted prefix after merging user environment. Desktop's shared reserved-key
|
||||
filter covers saved persona/agent settings and local/remote launch environments.
|
||||
|
||||
The first prefix element must be an absolute executable path. Unset means direct
|
||||
launch; empty, malformed or unusable prefixes fail launch with no direct fallback.
|
||||
The shared spawn path applies the prefix to every worker creation, including pool
|
||||
startup, wake, crash replacement, local tasks, model discovery and authentication.
|
||||
The variable is removed from the child's environment to avoid recursive wrapping.
|
||||
|
||||
The wrapper must preserve ACP stdin/stdout and either exec the worker or supervise
|
||||
it within the inherited process group. This hook does not enforce a sandbox or
|
||||
verify executable contents. The host owns policy, immutable launcher staging and
|
||||
its lifetime across delayed launches/restarts. Hosts requiring this hook must pin
|
||||
a supporting runtime; older binaries do not understand the environment setting.
|
||||
|
||||
### Configuration Examples
|
||||
|
||||
**Single agent, no heartbeat (default):**
|
||||
|
||||
@@ -8,6 +8,8 @@
|
||||
//! 4. [`AcpClient::session_prompt_with_idle_timeout`] — send prompt with idle/hard deadline, return stop reason
|
||||
//! 5. [`AcpClient::session_cancel`] / [`AcpClient::cancel_with_cleanup`] — cancel in-flight turn
|
||||
|
||||
mod launch;
|
||||
|
||||
use futures_util::StreamExt;
|
||||
use tokio::io::AsyncWriteExt;
|
||||
use tokio::process::{Child, ChildStdin, ChildStdout};
|
||||
@@ -474,8 +476,7 @@ impl AcpClient {
|
||||
) -> Result<Self, AcpError> {
|
||||
use std::process::Stdio;
|
||||
|
||||
let mut cmd = tokio::process::Command::new(command);
|
||||
cmd.args(args);
|
||||
let mut cmd = launch::command(command, args)?;
|
||||
if crate::config::normalize_agent_command_identity(command) == BUZZ_PI_ACP_NAME {
|
||||
if !args.iter().any(|arg| arg == "--") {
|
||||
cmd.arg("--");
|
||||
@@ -585,7 +586,13 @@ impl AcpClient {
|
||||
_ => None,
|
||||
};
|
||||
cmd.envs(launch_env.iter().cloned());
|
||||
let mut child = cmd.spawn()?;
|
||||
cmd.env_remove(launch::PREFIX_ENV);
|
||||
let mut child = cmd.spawn().map_err(|error| {
|
||||
std::io::Error::new(
|
||||
error.kind(),
|
||||
format!("failed to spawn {:?}: {error}", cmd.as_std().get_program()),
|
||||
)
|
||||
})?;
|
||||
|
||||
let stdin = child
|
||||
.stdin
|
||||
|
||||
83
crates/buzz-acp/src/acp/launch.rs
Normal file
83
crates/buzz-acp/src/acp/launch.rs
Normal file
@@ -0,0 +1,83 @@
|
||||
//! Host-controlled wrapping at the shared ACP subprocess boundary.
|
||||
use super::AcpError;
|
||||
use tokio::process::Command;
|
||||
|
||||
pub(super) const PREFIX_ENV: &str = "BUZZ_ACP_LAUNCH_PREFIX";
|
||||
|
||||
pub(super) fn command(worker: &str, args: &[String]) -> Result<Command, AcpError> {
|
||||
command_with_prefix(worker, args, std::env::var_os(PREFIX_ENV))
|
||||
}
|
||||
|
||||
fn command_with_prefix(
|
||||
worker: &str,
|
||||
args: &[String],
|
||||
prefix: Option<std::ffi::OsString>,
|
||||
) -> Result<Command, AcpError> {
|
||||
// Non-Unix cleanup only owns the immediate child, not a supervised worker.
|
||||
if prefix.is_some() && !cfg!(unix) {
|
||||
return Err(AcpError::Protocol(format!(
|
||||
"{PREFIX_ENV} is supported only on Unix; refusing to launch an uncontained worker"
|
||||
)));
|
||||
}
|
||||
let mut command = match prefix {
|
||||
None => Command::new(worker),
|
||||
Some(value) => {
|
||||
let invalid = || {
|
||||
AcpError::Protocol(format!(
|
||||
"{PREFIX_ENV} must be a nonempty JSON string array with an absolute executable path"
|
||||
))
|
||||
};
|
||||
let prefix: Vec<String> =
|
||||
serde_json::from_str(&value.into_string().map_err(|_| invalid())?)
|
||||
.map_err(|_| invalid())?;
|
||||
let executable = prefix
|
||||
.first()
|
||||
.filter(|path| std::path::Path::new(path).is_absolute())
|
||||
.ok_or_else(invalid)?;
|
||||
if prefix.iter().any(|arg| arg.contains('\0')) {
|
||||
return Err(invalid());
|
||||
}
|
||||
let mut command = Command::new(executable);
|
||||
command.args(&prefix[1..]).arg(worker);
|
||||
command
|
||||
}
|
||||
};
|
||||
command.args(args);
|
||||
Ok(command)
|
||||
}
|
||||
|
||||
#[cfg(all(test, unix))]
|
||||
mod tests;
|
||||
|
||||
#[cfg(test)]
|
||||
mod platform_tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn unset_prefix_keeps_direct_launch_on_every_platform() {
|
||||
let command = command_with_prefix("worker", &["arg".into()], None).unwrap();
|
||||
assert_eq!(command.as_std().get_program(), "worker");
|
||||
assert_eq!(command.as_std().get_args().collect::<Vec<_>>(), ["arg"]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn configured_prefix_requires_unix_process_containment() {
|
||||
// A real absolute executable prevents a bad path from hiding the guard.
|
||||
let executable = std::env::current_exe().unwrap();
|
||||
let prefix = serde_json::to_string(&[&executable]).unwrap();
|
||||
let result = command_with_prefix("worker", &[], Some(prefix.into()));
|
||||
if cfg!(unix) {
|
||||
assert_eq!(
|
||||
result.unwrap().as_std().get_program(),
|
||||
executable.as_os_str()
|
||||
);
|
||||
} else {
|
||||
assert!(result
|
||||
.unwrap_err()
|
||||
.to_string()
|
||||
.contains("supported only on Unix"));
|
||||
// Even an empty setting must refuse rather than downgrade to direct launch.
|
||||
assert!(command_with_prefix("worker", &[], Some("".into())).is_err());
|
||||
}
|
||||
}
|
||||
}
|
||||
225
crates/buzz-acp/src/acp/launch/tests.rs
Normal file
225
crates/buzz-acp/src/acp/launch/tests.rs
Normal file
@@ -0,0 +1,225 @@
|
||||
//! Subprocess isolation keeps the harness environment immutable in parallel tests.
|
||||
use super::PREFIX_ENV;
|
||||
use crate::{
|
||||
acp::AcpClient,
|
||||
config::{CliArgs, Config},
|
||||
usage::StandardAdapterKind,
|
||||
};
|
||||
use clap::Parser;
|
||||
use serde_json::{json, Value};
|
||||
use std::{fs, os::unix::fs::PermissionsExt, path::PathBuf, process::Command};
|
||||
|
||||
const CHILD: &str = "BUZZ_LAUNCH_TEST_CHILD";
|
||||
const LITERAL: &str = "policy with spaces, ; $(not-a-shell)";
|
||||
|
||||
struct Fixture(PathBuf);
|
||||
impl Fixture {
|
||||
fn new() -> Self {
|
||||
let dir = std::env::temp_dir().join(format!("buzz launch {}", uuid::Uuid::new_v4()));
|
||||
fs::create_dir(&dir).unwrap();
|
||||
let wrapper = dir.join("wrapper.py");
|
||||
fs::write(
|
||||
&wrapper,
|
||||
r#"import json, os, sys
|
||||
with open('wrapper.jsonl', 'a') as log:
|
||||
log.write(json.dumps(sys.argv[1:]) + '\n')
|
||||
os.execv(sys.argv[2], sys.argv[2:])
|
||||
"#,
|
||||
)
|
||||
.unwrap();
|
||||
for name in [
|
||||
"goose",
|
||||
"codex-acp",
|
||||
"claude-agent-acp",
|
||||
"hermes-acp",
|
||||
"buzz-pi-acp",
|
||||
] {
|
||||
let path = dir.join(name);
|
||||
fs::write(
|
||||
&path,
|
||||
r#"#!/usr/bin/env python3
|
||||
import json, os, sys
|
||||
with open('worker-started', 'a') as log:
|
||||
log.write('started\n')
|
||||
for line in sys.stdin:
|
||||
request = json.loads(line)
|
||||
result = {'protocolVersion': 1, 'argv': sys.argv[1:],
|
||||
'hermes': os.environ.get('HERMES_ACP_SKIP_CONFIGURED_MCP'),
|
||||
'codex': os.environ.get('CODEX_CONFIG'),
|
||||
'prefix': os.environ.get('BUZZ_ACP_LAUNCH_PREFIX')}
|
||||
print(json.dumps({'jsonrpc': '2.0', 'id': request['id'], 'result': result}), flush=True)
|
||||
"#,
|
||||
)
|
||||
.unwrap();
|
||||
fs::set_permissions(path, fs::Permissions::from_mode(0o700)).unwrap();
|
||||
}
|
||||
Self(dir)
|
||||
}
|
||||
fn prefix(&self) -> String {
|
||||
json!([
|
||||
"/usr/bin/env",
|
||||
"python3",
|
||||
self.0.join("wrapper.py"),
|
||||
LITERAL
|
||||
])
|
||||
.to_string()
|
||||
}
|
||||
fn run(&self, mode: &str, prefix: Option<&str>) {
|
||||
let mut cmd = Command::new(std::env::current_exe().unwrap());
|
||||
for (name, _) in std::env::vars_os() {
|
||||
if name.to_string_lossy().starts_with("BUZZ_") {
|
||||
cmd.env_remove(name);
|
||||
}
|
||||
}
|
||||
cmd.args(["--exact", "acp::launch::tests::spawn_child", "--nocapture"])
|
||||
.env(CHILD, mode)
|
||||
.env_remove("CODEX_CONFIG")
|
||||
.env_remove("HERMES_ACP_SKIP_CONFIGURED_MCP")
|
||||
.current_dir(&self.0);
|
||||
if let Some(prefix) = prefix {
|
||||
cmd.env(PREFIX_ENV, prefix);
|
||||
}
|
||||
let output = cmd.output().unwrap();
|
||||
assert!(
|
||||
output.status.success(),
|
||||
"{}\n{}",
|
||||
String::from_utf8_lossy(&output.stdout),
|
||||
String::from_utf8_lossy(&output.stderr)
|
||||
);
|
||||
}
|
||||
}
|
||||
impl Drop for Fixture {
|
||||
fn drop(&mut self) {
|
||||
let _ = fs::remove_dir_all(&self.0);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn wrapper_preserves_worker_identity_and_runs_on_each_spawn() {
|
||||
for wrapped in [false, true] {
|
||||
let f = Fixture::new();
|
||||
let prefix = f.prefix();
|
||||
f.run("success", wrapped.then_some(prefix.as_str()));
|
||||
assert_eq!(
|
||||
fs::read_to_string(f.0.join("worker-started"))
|
||||
.unwrap()
|
||||
.lines()
|
||||
.count(),
|
||||
10
|
||||
);
|
||||
if wrapped {
|
||||
let calls: Vec<Value> = fs::read_to_string(f.0.join("wrapper.jsonl"))
|
||||
.unwrap()
|
||||
.lines()
|
||||
.map(|line| serde_json::from_str(line).unwrap())
|
||||
.collect();
|
||||
assert_eq!(calls.len(), 10);
|
||||
for call in calls {
|
||||
assert_eq!(call[0], LITERAL);
|
||||
}
|
||||
} else {
|
||||
assert!(!f.0.join("wrapper.jsonl").exists());
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn invalid_or_unavailable_wrapper_never_falls_back_to_worker() {
|
||||
let f = Fixture::new();
|
||||
for prefix in [
|
||||
"",
|
||||
"[]",
|
||||
"null",
|
||||
"{}",
|
||||
"[1]",
|
||||
"[\"\"]",
|
||||
"[\"relative\"]",
|
||||
"[\"/tmp/\\u0000\"]",
|
||||
"[\"/missing-buzz-launcher\"]",
|
||||
] {
|
||||
f.run("failure", Some(prefix));
|
||||
assert!(!f.0.join("worker-started").exists());
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn spawn_child() {
|
||||
let Ok(mode) = std::env::var(CHILD) else {
|
||||
return;
|
||||
};
|
||||
// Bound a broken wrapper/stdio regression; no arbitrary timing gates.
|
||||
tokio::time::timeout(std::time::Duration::from_secs(10), async {
|
||||
for name in [
|
||||
"goose",
|
||||
"codex-acp",
|
||||
"claude-agent-acp",
|
||||
"hermes-acp",
|
||||
"buzz-pi-acp",
|
||||
] {
|
||||
let worker = std::env::current_dir().unwrap().join(name);
|
||||
let args = CliArgs::try_parse_from([
|
||||
"buzz-acp",
|
||||
"--private-key",
|
||||
"0000000000000000000000000000000000000000000000000000000000000001",
|
||||
"--agent-command",
|
||||
worker.to_str().unwrap(),
|
||||
"--agent-args",
|
||||
"",
|
||||
])
|
||||
.unwrap();
|
||||
let config = Config::from_args(args).unwrap();
|
||||
// A second spawn uses the same host configuration after shutdown,
|
||||
// just as a replacement worker does. No prefix is consumed once.
|
||||
for _ in 0..2 {
|
||||
let spawned = AcpClient::spawn(
|
||||
&config.agent_command,
|
||||
&config.agent_args,
|
||||
&config.persona_env_vars,
|
||||
config.has_generated_codex_config,
|
||||
)
|
||||
.await;
|
||||
if mode == "failure" {
|
||||
let error = spawned.err().expect("invalid wrapper launched a worker");
|
||||
if std::env::var(PREFIX_ENV)
|
||||
.unwrap()
|
||||
.contains("/missing-buzz-launcher")
|
||||
{
|
||||
assert!(error.to_string().contains("/missing-buzz-launcher"));
|
||||
}
|
||||
return;
|
||||
}
|
||||
let mut client = spawned.unwrap();
|
||||
let probe = client.initialize().await.unwrap();
|
||||
let expected = match name {
|
||||
"goose" => json!(["acp"]),
|
||||
"hermes-acp" => json!([]),
|
||||
"buzz-pi-acp" => json!([
|
||||
"--",
|
||||
"--skill",
|
||||
std::env::current_dir().unwrap().join(".agents/skills")
|
||||
]),
|
||||
_ => json!([]),
|
||||
};
|
||||
assert_eq!(probe["argv"], expected, "{name}");
|
||||
assert!(probe["prefix"].is_null());
|
||||
if name == "hermes-acp" {
|
||||
assert_eq!(probe["hermes"], "1");
|
||||
}
|
||||
let adapter = match name {
|
||||
"codex-acp" => {
|
||||
let config: Value =
|
||||
serde_json::from_str(probe["codex"].as_str().unwrap()).unwrap();
|
||||
assert_eq!(config["sandbox_workspace_write"]["network_access"], true);
|
||||
Some(StandardAdapterKind::Codex)
|
||||
}
|
||||
"claude-agent-acp" => Some(StandardAdapterKind::Claude),
|
||||
_ => None,
|
||||
};
|
||||
assert_eq!(client.standard_adapter, adapter);
|
||||
client.shutdown().await;
|
||||
}
|
||||
}
|
||||
})
|
||||
.await
|
||||
.expect("wrapped ACP handshake exceeded test deadline");
|
||||
}
|
||||
@@ -577,3 +577,37 @@ async fn memory_is_loaded_before_session_and_opt_out_makes_no_request() {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn launch_prefix_wraps_the_shipped_local_task_entrypoint() {
|
||||
let f = Fixture::new();
|
||||
let mut command = f.command();
|
||||
command
|
||||
.env(
|
||||
"BUZZ_ACP_LAUNCH_PREFIX",
|
||||
json!([
|
||||
"/bin/sh",
|
||||
"-c",
|
||||
"printf invoked > wrapper-called; exec \"$@\"",
|
||||
"fixture-launcher"
|
||||
])
|
||||
.to_string(),
|
||||
)
|
||||
.args(["--no-memory", "--task", "-"]);
|
||||
let mut child = command.spawn().unwrap();
|
||||
child
|
||||
.stdin
|
||||
.take()
|
||||
.unwrap()
|
||||
.write_all(f.task().to_string().as_bytes())
|
||||
.unwrap();
|
||||
terminal(&wait(child), 0, "completed");
|
||||
assert_eq!(
|
||||
fs::read_to_string(f.dir.join("wrapper-called")).unwrap(),
|
||||
"invoked"
|
||||
);
|
||||
assert!(f
|
||||
.wire()
|
||||
.iter()
|
||||
.any(|message| message["method"] == "session/prompt"));
|
||||
}
|
||||
|
||||
@@ -190,6 +190,7 @@ fn reserved_keys_include_code_execution_surface() {
|
||||
"BUZZ_ACP_AGENT_COMMAND",
|
||||
"BUZZ_ACP_AGENT_ARGS",
|
||||
"BUZZ_ACP_MCP_COMMAND",
|
||||
"BUZZ_ACP_LAUNCH_PREFIX",
|
||||
] {
|
||||
assert!(is_reserved_env_key(key), "{key} should be reserved");
|
||||
}
|
||||
@@ -512,3 +513,19 @@ fn deploy_model_precedence_none_when_both_absent() {
|
||||
let effective = persona_model.clone().or(record_model.clone());
|
||||
assert_eq!(effective, None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn launch_prefix_cannot_be_saved_or_merged_from_user_environment() {
|
||||
for key in [
|
||||
"BUZZ_ACP_LAUNCH_PREFIX",
|
||||
"buzz_acp_launch_prefix",
|
||||
"Buzz_Acp_Launch_Prefix",
|
||||
] {
|
||||
let env = map(&[(key, r#"["/usr/bin/env"]"#)]);
|
||||
assert!(validate_user_env_keys(&env)
|
||||
.unwrap_err()
|
||||
.contains("reserved"));
|
||||
assert!(merged_user_env(&env, &BTreeMap::new()).is_empty());
|
||||
assert!(merged_user_env(&BTreeMap::new(), &env).is_empty());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -41,6 +41,7 @@ pub(crate) const RESERVED_ENV_KEYS: &[&str] = &[
|
||||
"BUZZ_ACP_AGENT_COMMAND",
|
||||
"BUZZ_ACP_AGENT_ARGS",
|
||||
"BUZZ_ACP_MCP_COMMAND",
|
||||
"BUZZ_ACP_LAUNCH_PREFIX",
|
||||
// Control-plane parallelism: the Desktop resolves the effective
|
||||
// worker-pool size (applying any per-harness cap) and writes it into
|
||||
// launch.policy_env. A user-supplied BUZZ_ACP_AGENTS would bypass the
|
||||
|
||||
Reference in New Issue
Block a user