Repository navigation
feat(acp): wrap workers at the subprocess launch boundary #7985
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
|
Comment on lines
+40
to
+41
There was a problem hiding this comment.
When AGENTS.md reference: AGENTS.md:L240-L244 Useful? React with 👍 / 👎. |
||
| 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()); | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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"); | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 Desktop doesn't reserve this key.
RESERVED_ENV_KEYSindesktop/src-tauri/src/managed_agents/reserved_env_keys.rslistsBUZZ_ACP_AGENT_COMMAND,BUZZ_ACP_AGENT_ARGSandBUZZ_ACP_MCP_COMMANDas the code-execution surface, andmerged_user_envonly filters what's on that list.managed_agents/runtime.rswrites the layered user env (definition → global → persona → agent) onto the harness command last. So a saved persona or agent env var can setBUZZ_ACP_LAUNCH_PREFIXand choose the executable that every worker starts through. The remote deploy path (commands/agents_deploy.rs) puts the same merged env intolaunch.env.this also works against the sandbox use case. if a host sets the prefix and then applies user env on top of it, the way Desktop orders things, a saved env var can replace the launcher with
["/usr/bin/env"]and the worker runs unprotected.could we add it to
RESERVED_ENV_KEYS, with the matching assertion inmanaged_agents/env_vars/tests.rs, and note in the README that hosts have to apply the prefix after any user-supplied env? I think this should land with this PR, since the key only becomes dangerous once this PR ships.