From 3e6c6cb5ee6f882b5eb3581a84585901675de008 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Fri, 10 Jul 2026 21:23:47 -0400 Subject: [PATCH] fix(codex): swap to @agentclientprotocol/codex-acp 1.x + detect outdated adapter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The @zed-industries/codex-acp package is deprecated (0.16.0, frozen 2026-06-08); its README redirects to @agentclientprotocol/codex-acp (1.1.2, 2026-07-09). The 1.x adapter changed config injection: the old adapter accepted `-c key=value` CLI args; the 1.x arg dispatch only handles `--version`, `login`, and `cli` — all other args fall through to startAcpServer(), which reads config exclusively from `process.env["CODEX_CONFIG"]` (a JSON blob). Our three `-c` flags were silently dropped, breaking relay connectivity (buzz-cli MCP subprocess blocked by the Seatbelt sandbox). `network_proxy.mode` and `network_proxy.domains` are gone from the 1.x schema; only `sandbox_workspace_write.network_access` is needed. Without a version gate, users with the old 0.16.x adapter still on PATH would see it as Available, receive the new CODEX_CONFIG spawn contract, and get silently broken relay connectivity (0.16.x reads -c args, ignores env vars). Added AdapterOutdated: version-probe (codex-acp --version) in discover_acp_runtimes and cli_login_requirements blocks the old adapter and surfaces the reinstall nudge. Note: #1745's config-parse classifier keys on codex's `error loading configuration` + `unknown variant` stderr wording. 1.x bundles @openai/codex@0.144.0; the wording comes from that layer. Likely unchanged, noted as a dependency, not fixed here. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-acp/README.md | 10 +- crates/buzz-acp/src/acp.rs | 415 +++++++++++++++++- crates/buzz-acp/src/config.rs | 226 +++++----- crates/buzz-acp/src/lib.rs | 21 +- crates/buzz-acp/src/pool.rs | 22 +- crates/buzz-acp/src/setup_mode.rs | 12 + desktop/scripts/check-file-sizes.mjs | 17 +- .../src-tauri/src/commands/agent_discovery.rs | 131 +++++- .../src-tauri/src/managed_agents/discovery.rs | 128 +++++- .../src/managed_agents/discovery/tests.rs | 209 ++++++++- .../src-tauri/src/managed_agents/readiness.rs | 19 +- desktop/src-tauri/src/managed_agents/types.rs | 2 + .../agents/ui/AgentDefinitionDialog.tsx | 8 +- .../agents/ui/personaDialogPickers.tsx | 14 +- .../src/features/onboarding/ui/SetupStep.tsx | 23 + .../settings/ui/DoctorSettingsPanel.tsx | 34 ++ desktop/src/shared/api/types.ts | 1 + desktop/src/shared/lib/configNudge.test.mjs | 23 + desktop/src/shared/lib/configNudge.ts | 2 + .../src/shared/ui/config-nudge-attachment.tsx | 2 + 20 files changed, 1146 insertions(+), 173 deletions(-) diff --git a/crates/buzz-acp/README.md b/crates/buzz-acp/README.md index 9399766f17..b06891dd84 100644 --- a/crates/buzz-acp/README.md +++ b/crates/buzz-acp/README.md @@ -9,7 +9,7 @@ Buzz Relay ──WS──→ buzz-acp ──stdio──→ Your Agent (send_message, etc.) ``` -Supports any agent that speaks [ACP](https://agentclientprotocol.com/) over stdio: **goose**, **codex** (via [codex-acp](https://github.com/zed-industries/codex-acp)), and **claude code** (via [claude-agent-acp](https://github.com/agentclientprotocol/claude-agent-acp)). +Supports any agent that speaks [ACP](https://agentclientprotocol.com/) over stdio: **goose**, **codex** (via [codex-acp](https://github.com/agentclientprotocol/codex-acp)), and **claude code** (via [claude-agent-acp](https://github.com/agentclientprotocol/claude-agent-acp)). ## Prerequisites @@ -57,16 +57,14 @@ That's it. The harness spawns `goose acp`, connects to the relay, discovers chan ## Running with Codex -[codex-acp](https://github.com/zed-industries/codex-acp) wraps OpenAI Codex in an ACP interface. +[codex-acp](https://github.com/agentclientprotocol/codex-acp) wraps OpenAI Codex in an ACP interface. ```bash -# Build the adapter (requires Rust 1.91+) -cd /path/to/codex-acp && cargo build --release +# Install the adapter (npm package — no Rust build required) +npm install -g @agentclientprotocol/codex-acp # Run export OPENAI_API_KEY="sk-..." # required — use an OpenAI API key, not a ChatGPT subscription -export BUZZ_ACP_AGENT_COMMAND="/path/to/codex-acp/target/release/codex-acp" -export BUZZ_ACP_AGENT_ARGS='-c,permissions.approval_policy="never"' buzz-acp ``` diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index 78e23c8f8d..2754f91cfe 100644 --- a/crates/buzz-acp/src/acp.rs +++ b/crates/buzz-acp/src/acp.rs @@ -188,6 +188,148 @@ pub struct AcpClient { goose_usage: UsageTracker, } +/// Recursively merge `overlay` into `base`, with `overlay` winning on scalar/shape +/// collisions. When both sides have an object for the same key, the merge recurses so +/// unrelated nested keys from `base` are preserved. +fn deep_merge( + base: &mut serde_json::Map, + overlay: serde_json::Map, +) { + for (k, overlay_val) in overlay { + match base.get_mut(&k) { + Some(serde_json::Value::Object(base_obj)) + if matches!(overlay_val, serde_json::Value::Object(_)) => + { + // Both sides are objects — recurse to preserve unrelated nested keys. + if let serde_json::Value::Object(overlay_obj) = overlay_val { + deep_merge(base_obj, overlay_obj); + } + } + _ => { + // Scalar, array, type mismatch, or new key — overlay wins. + base.insert(k, overlay_val); + } + } + } +} + +/// Build the merged `CODEX_CONFIG` environment-variable value for a Codex agent spawn. +/// +/// Returns `Some(json_string)` when `has_generated_codex_config` is true (Buzz injected a +/// `CODEX_CONFIG` entry via `codex_network_env()`), `None` otherwise. +/// +/// # Merge contract (when `has_generated_codex_config` is true) +/// +/// 1. **Persona base** — the first `CODEX_CONFIG` value in `extra_env` is taken as +/// the base object (all keys preserved, recursively). When there is no persona entry, +/// the generated entry serves as the base. +/// 2. **Generated overlay** — all subsequent `CODEX_CONFIG` entries are deep-merged into +/// the base so unrelated nested persona keys survive. +/// 3. **Parent-env precedence** — if `parent_codex_config` is `Some`, its keys are +/// deep-merged into the result (parent wins on colliding keys at every nesting level; +/// unrelated keys from either side survive). +/// 4. **Forced overlay** — `sandbox_workspace_write.network_access = true` is applied +/// last so relay access is guaranteed regardless of operator / persona config. +/// +/// When `has_generated_codex_config` is false, the function returns `None` and the +/// caller handles any persona-supplied `CODEX_CONFIG` with ordinary operator-wins +/// semantics (no merging, no sandbox widening). +/// +/// # Errors +/// +/// Returns `Err(AcpError::Protocol)` when `has_generated_codex_config` is true and any +/// `CODEX_CONFIG` value is not valid JSON or is not a JSON object, or when +/// `sandbox_workspace_write` is present but not an object after all merges. +pub(crate) fn build_codex_config_env( + extra_env: &[(String, String)], + parent_codex_config: Option<&str>, + has_generated_codex_config: bool, +) -> Result, AcpError> { + // Without an explicit Buzz-generated overlay signal, skip the merge entirely. + // Any persona CODEX_CONFIG is handled by the caller with operator-wins semantics. + if !has_generated_codex_config { + return Ok(None); + } + + // Collect all CODEX_CONFIG entries from extra_env in order. + let codex_entries: Vec<&str> = extra_env + .iter() + .filter(|(k, _)| k == "CODEX_CONFIG") + .map(|(_, v)| v.as_str()) + .collect(); + + if codex_entries.is_empty() { + // has_generated_codex_config is true but no entry in extra_env — shouldn't + // happen in practice, but treat as no-op rather than panic. + return Ok(None); + } + + // Parse all entries; first one is the persona base (or the generated entry if no + // persona CODEX_CONFIG was set), rest are additional generated entries. + let mut parsed_entries: Vec> = Vec::new(); + for (i, raw) in codex_entries.iter().enumerate() { + match serde_json::from_str::(raw) { + Ok(serde_json::Value::Object(obj)) => parsed_entries.push(obj), + Ok(_) => { + let source = if i == 0 { "persona" } else { "generated" }; + return Err(AcpError::Protocol(format!( + "CODEX_CONFIG {source} value is valid JSON but not an object" + ))); + } + Err(e) => { + let source = if i == 0 { "persona" } else { "generated" }; + return Err(AcpError::Protocol(format!( + "CODEX_CONFIG {source} value is not valid JSON: {e}" + ))); + } + } + } + + // Start from first entry, deep-merge remaining entries. + let mut base = parsed_entries.remove(0); + for overlay in parsed_entries { + deep_merge(&mut base, overlay); + } + + // Deep-merge parent env (parent wins on colliding keys at every nesting level). + if let Some(parent_raw) = parent_codex_config { + match serde_json::from_str::(parent_raw) { + Ok(serde_json::Value::Object(parent_obj)) => { + deep_merge(&mut base, parent_obj); + } + Ok(_) => { + return Err(AcpError::Protocol( + "CODEX_CONFIG in parent environment is valid JSON but not an object".into(), + )); + } + Err(e) => { + return Err(AcpError::Protocol(format!( + "CODEX_CONFIG in parent environment is not valid JSON: {e}" + ))); + } + } + } + + // Force sandbox_workspace_write.network_access = true (our invariant, always wins). + let sws_entry = base + .entry("sandbox_workspace_write") + .or_insert_with(|| serde_json::json!({})); + match sws_entry { + serde_json::Value::Object(sws_obj) => { + sws_obj.insert("network_access".to_string(), serde_json::Value::Bool(true)); + } + other => { + return Err(AcpError::Protocol(format!( + "CODEX_CONFIG sandbox_workspace_write is not an object (got {}); \ + cannot set network_access=true", + other + ))); + } + } + + Ok(Some(serde_json::Value::Object(base).to_string())) +} + impl AcpClient { /// Kill the agent subprocess and wait for it to exit (no zombies). /// @@ -220,11 +362,17 @@ impl AcpClient { /// Spawn the agent binary as a subprocess and connect to its stdio pipes. /// + /// `has_generated_codex_config` must be true when `codex_network_env()` successfully + /// injected a `CODEX_CONFIG` entry into `extra_env`. The spawn path uses it to + /// trigger the recursive merge + forced `network_access=true` in + /// `build_codex_config_env`. Pass `false` for test spawns and non-Codex agents. + /// /// After spawning, call [`initialize`](Self::initialize) before any other method. pub async fn spawn( command: &str, args: &[String], extra_env: &[(String, String)], + has_generated_codex_config: bool, ) -> Result { use std::process::Stdio; @@ -239,12 +387,41 @@ impl AcpClient { .kill_on_drop(true); // Per-persona env vars (e.g., GOOSE_PROVIDER, BUZZ_AGENT_PROVIDER). - // Only injected if not already set in parent env (operator precedence). + // For most keys, operator precedence wins: skip injection if already set + // in the parent environment. + // + // CODEX_CONFIG is handled specially via build_codex_config_env: + // • has_generated_codex_config=true: merge all CODEX_CONFIG entries + parent + // recursively and force network_access=true. + // • has_generated_codex_config=false: return None; any persona-supplied + // CODEX_CONFIG falls through to the normal operator-wins loop below. + let has_codex_config = extra_env.iter().any(|(k, _)| k == "CODEX_CONFIG"); + let parent_codex_config = if has_generated_codex_config && has_codex_config { + std::env::var("CODEX_CONFIG").ok() + } else { + None + }; + let codex_config_value = build_codex_config_env( + extra_env, + parent_codex_config.as_deref(), + has_generated_codex_config, + )?; + // When the merge path was not taken (None returned), any persona CODEX_CONFIG + // entry falls through to the standard operator-wins treatment below. + let codex_merge_active = codex_config_value.is_some(); + 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() { cmd.env(key, value); } } + if let Some(merged) = codex_config_value { + cmd.env("CODEX_CONFIG", merged); + } // Spawn the agent in its own process group so SIGKILL doesn't propagate // to the harness's own process group on Unix. @@ -2325,7 +2502,7 @@ mod tests { } async fn spawn_script(script: &str) -> AcpClient { - AcpClient::spawn("bash", &["-c".into(), script.into()], &[]) + AcpClient::spawn("bash", &["-c".into(), script.into()], &[], false) .await .expect("failed to spawn test script") } @@ -2688,7 +2865,7 @@ mod tests { /// which is fine — these tests don't read from the agent, they just /// feed JSON into the parser. async fn spawn_inert_client() -> AcpClient { - AcpClient::spawn("cat", &[], &[]) + AcpClient::spawn("cat", &[], &[], false) .await .expect("spawn cat as inert client") } @@ -3049,4 +3226,236 @@ mod tests { other => panic!("expected AgentError, got {other:?}"), } } + + // ── build_codex_config_env ──────────────────────────────────────────────── + + fn env(pairs: &[(&str, &str)]) -> Vec<(String, String)> { + pairs + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect() + } + + const GENERATED: &str = r#"{"sandbox_workspace_write":{"network_access":true}}"#; + + #[test] + fn build_codex_config_env_returns_none_when_no_codex_config_in_extra_env() { + // Non-Codex agents: extra_env has no CODEX_CONFIG → None regardless of signal. + let extra = env(&[("GOOSE_PROVIDER", "openai")]); + let result = build_codex_config_env(&extra, None, false).unwrap(); + assert_eq!( + result, None, + "no CODEX_CONFIG in extra_env must return None" + ); + } + + #[test] + fn build_codex_config_env_generated_only_single_entry_with_signal_true_merges_with_parent() { + // No persona: Buzz injects one CODEX_CONFIG; signal=true. + // Parent may have its own CODEX_CONFIG — deep_merge applies, network_access forced. + let extra = env(&[("CODEX_CONFIG", GENERATED)]); + let parent = + r#"{"some_operator_key":"val","sandbox_workspace_write":{"operator_key":"keep"}}"#; + let merged = build_codex_config_env(&extra, Some(parent), true) + .unwrap() + .unwrap(); + let v: serde_json::Value = serde_json::from_str(&merged).unwrap(); + // network_access forced true even though only one entry in extra_env. + assert_eq!( + v["sandbox_workspace_write"]["network_access"], true, + "network_access must be forced true with signal=true" + ); + // Operator key preserved via deep_merge. + assert_eq!( + v["sandbox_workspace_write"]["operator_key"], "keep", + "operator nested key must survive" + ); + assert_eq!( + v["some_operator_key"], "val", + "operator top-level key must survive" + ); + } + + #[test] + fn build_codex_config_env_persona_only_signal_false_returns_none() { + // Persona set CODEX_CONFIG; Buzz did not inject a generated overlay (signal=false). + // Must return None — no merging, no sandbox widening. + let persona = r#"{"some_feature":"on"}"#; + let extra = env(&[("CODEX_CONFIG", persona)]); + let result = build_codex_config_env(&extra, None, false).unwrap(); + assert_eq!( + result, None, + "persona-only CODEX_CONFIG with signal=false must return None" + ); + } + + #[test] + fn build_codex_config_env_returns_none_for_persona_only_no_generated_overlay() { + // Alias: same scenario as above, confirms the old count-based path no longer exists. + let persona = r#"{"some_feature":"on"}"#; + let extra = env(&[("CODEX_CONFIG", persona)]); + let result = build_codex_config_env(&extra, None, false).unwrap(); + assert_eq!( + result, None, + "persona-only CODEX_CONFIG with signal=false must return None" + ); + } + + #[test] + fn build_codex_config_env_sets_network_access_from_scratch() { + // Persona + generated overlay, signal=true: network_access is forced true. + let persona = r#"{}"#; + let extra = env(&[("CODEX_CONFIG", persona), ("CODEX_CONFIG", GENERATED)]); + let merged = build_codex_config_env(&extra, None, true).unwrap().unwrap(); + let v: serde_json::Value = serde_json::from_str(&merged).unwrap(); + assert_eq!(v["sandbox_workspace_write"]["network_access"], true); + } + + #[test] + fn build_codex_config_env_persona_keys_survive_merge() { + // Persona has CODEX_CONFIG with unrelated keys; generated overlay must + // force network_access=true without erasing persona keys. + let persona_cfg = r#"{"some_feature":{"enabled":true}}"#; + // Config::from_args appends generated AFTER persona env vars. + let extra = env(&[("CODEX_CONFIG", persona_cfg), ("CODEX_CONFIG", GENERATED)]); + let merged = build_codex_config_env(&extra, None, true).unwrap().unwrap(); + let v: serde_json::Value = serde_json::from_str(&merged).unwrap(); + assert_eq!( + v["some_feature"]["enabled"], true, + "persona key must survive merge" + ); + assert_eq!( + v["sandbox_workspace_write"]["network_access"], true, + "network_access must be forced true" + ); + } + + #[test] + fn build_codex_config_env_nested_persona_keys_survive_when_parent_has_same_top_level_key() { + // Persona has sandbox_workspace_write.persona_only; parent has + // sandbox_workspace_write.parent_only. A flat top-level spread would drop + // persona_only. deep_merge must preserve both nested keys, and + // network_access must be forced true last. + let persona_cfg = r#"{"sandbox_workspace_write":{"persona_only":"keep_me"}}"#; + let extra = env(&[("CODEX_CONFIG", persona_cfg), ("CODEX_CONFIG", GENERATED)]); + let parent = r#"{"sandbox_workspace_write":{"parent_only":"also_here"}}"#; + let merged = build_codex_config_env(&extra, Some(parent), true) + .unwrap() + .unwrap(); + let v: serde_json::Value = serde_json::from_str(&merged).unwrap(); + // Both nested keys survive — no flat-spread drop. + assert_eq!( + v["sandbox_workspace_write"]["persona_only"], "keep_me", + "nested persona key must survive when parent has the same top-level key" + ); + assert_eq!( + v["sandbox_workspace_write"]["parent_only"], "also_here", + "nested parent key must be present" + ); + // Forced last. + assert_eq!( + v["sandbox_workspace_write"]["network_access"], true, + "network_access must be forced true" + ); + } + + #[test] + fn build_codex_config_env_parent_env_wins_on_collisions_persona_keys_survive() { + // Parent env has CODEX_CONFIG with some keys; persona has different keys. + // Parent wins on collision; unrelated persona keys survive. + // network_access is always forced true. + let persona_cfg = r#"{"persona_key":"persona_val","shared_key":"persona_version"}"#; + // Config::from_args appends generated AFTER persona env vars. + let extra = env(&[("CODEX_CONFIG", persona_cfg), ("CODEX_CONFIG", GENERATED)]); + let parent = r#"{"parent_key":"parent_val","shared_key":"parent_version"}"#; + let merged = build_codex_config_env(&extra, Some(parent), true) + .unwrap() + .unwrap(); + let v: serde_json::Value = serde_json::from_str(&merged).unwrap(); + // Parent-only key present + assert_eq!( + v["parent_key"], "parent_val", + "parent-only key must be present" + ); + // Unrelated persona key survives (no collision with parent) + assert_eq!( + v["persona_key"], "persona_val", + "unrelated persona key must survive" + ); + // Collision: parent wins + assert_eq!( + v["shared_key"], "parent_version", + "parent must win on colliding key" + ); + // network_access always true (forced last) + assert_eq!(v["sandbox_workspace_write"]["network_access"], true); + } + + #[test] + fn build_codex_config_env_parent_has_existing_sandbox_other_keys_survive() { + // Parent env has sandbox_workspace_write with extra keys; after merge + // those extra keys survive alongside network_access=true. + let persona = r#"{}"#; + let extra = env(&[("CODEX_CONFIG", persona), ("CODEX_CONFIG", GENERATED)]); + let parent = + r#"{"sandbox_workspace_write":{"network_access":false,"other_sandbox_key":"val"}}"#; + let merged = build_codex_config_env(&extra, Some(parent), true) + .unwrap() + .unwrap(); + let v: serde_json::Value = serde_json::from_str(&merged).unwrap(); + // network_access forced true even though parent set false + assert_eq!(v["sandbox_workspace_write"]["network_access"], true); + // other_sandbox_key survives (parent's sws merged, then network_access forced) + assert_eq!(v["sandbox_workspace_write"]["other_sandbox_key"], "val"); + } + + #[test] + fn build_codex_config_env_errors_on_invalid_persona_json() { + // Bad persona JSON + generated overlay, signal=true → parse error before merging. + let extra = env(&[("CODEX_CONFIG", "not-json"), ("CODEX_CONFIG", GENERATED)]); + let result = build_codex_config_env(&extra, None, true); + assert!(result.is_err(), "invalid persona JSON must return Err"); + let msg = format!("{}", result.unwrap_err()); + assert!( + msg.contains("CODEX_CONFIG"), + "error must mention CODEX_CONFIG" + ); + } + + #[test] + fn build_codex_config_env_errors_on_non_object_persona_json() { + // Non-object persona JSON + generated overlay, signal=true → parse error. + let extra = env(&[("CODEX_CONFIG", "[1,2,3]"), ("CODEX_CONFIG", GENERATED)]); + let result = build_codex_config_env(&extra, None, true); + assert!(result.is_err(), "non-object persona JSON must return Err"); + } + + #[test] + fn build_codex_config_env_errors_on_invalid_parent_json() { + let persona = r#"{}"#; + let extra = env(&[("CODEX_CONFIG", persona), ("CODEX_CONFIG", GENERATED)]); + let result = build_codex_config_env(&extra, Some("bad-json"), true); + assert!(result.is_err(), "invalid parent env JSON must return Err"); + } + + #[test] + fn build_codex_config_env_errors_on_non_object_sandbox_workspace_write() { + // sandbox_workspace_write must be an object for network_access forcing. + // If the parent env sets it to a non-object scalar, deep_merge replaces + // our object with the scalar, and the force step must fail clearly. + let persona = r#"{}"#; + let extra = env(&[("CODEX_CONFIG", persona), ("CODEX_CONFIG", GENERATED)]); + // Parent replaces the object with a scalar — deep_merge: scalar overlay wins. + let parent = r#"{"sandbox_workspace_write": 42}"#; + let result = build_codex_config_env(&extra, Some(parent), true); + assert!( + result.is_err(), + "non-object sandbox_workspace_write must return Err" + ); + let msg = format!("{}", result.unwrap_err()); + assert!( + msg.contains("sandbox_workspace_write"), + "error must mention sandbox_workspace_write" + ); + } } diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index d139dc6cd1..7014d89bd8 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -485,6 +485,12 @@ pub struct Config { /// Per-persona env vars to inject at agent spawn time (e.g., GOOSE_PROVIDER, GOOSE_MODEL, BUZZ_AGENT_MODEL). /// Populated from persona pack resolution. Empty when no pack is configured. pub persona_env_vars: Vec<(String, String)>, + /// Whether `codex_network_env()` successfully injected a `CODEX_CONFIG` entry into + /// `persona_env_vars`. When true, `AcpClient::spawn` merges all `CODEX_CONFIG` entries + /// and forces `sandbox_workspace_write.network_access = true` via `build_codex_config_env`. + /// When false (non-Codex agents or rejected relay URL), the helper returns None and + /// any persona-supplied `CODEX_CONFIG` is handled with ordinary operator-wins semantics. + pub has_generated_codex_config: bool, /// Whether to publish encrypted observer frames through the relay. pub relay_observer: bool, /// Agent owner pubkey (hex). Used for `--respond-to=owner-only` gate. @@ -572,57 +578,54 @@ fn default_agent_args(command: &str) -> Option> { /// /// Codex sandboxes MCP subprocesses (including `buzz-cli`) behind a Seatbelt sandbox /// that blocks all outbound network by default, plus a managed proxy with a domain -/// allowlist. Without these flags, `buzz-cli` requests to the relay are blocked before -/// they reach WARP or any other outbound network path. +/// sandbox gate for `buzz-cli` (an MCP subprocess that connects to the relay over +/// WebSocket). Without this, `buzz-cli` requests are blocked by Codex's macOS Seatbelt +/// sandbox before they can reach the network. /// -/// Returns `["-c", "sandbox_workspace_write.network_access=true", "-c", -/// "network_proxy.mode=\"full\"", "-c", "network_proxy.domains.\"\"=\"allow\""]` -/// for Codex agents, or an empty vec for non-Codex agents or when the hostname cannot -/// be parsed from the relay URL. +/// Returns `Some(("CODEX_CONFIG", "{\"sandbox_workspace_write\":{\"network_access\":true}}"))` for +/// Codex agents, or `None` for non-Codex agents or when the relay URL cannot be parsed. /// -/// The flags form a layered defense: -/// - `sandbox_workspace_write.network_access=true` opens the Seatbelt sandbox gate so -/// outbound TCP/TLS connections are allowed at the OS level -/// - `network_proxy.mode="full"` enables the managed proxy for all outbound traffic -/// - `network_proxy.domains.""="allow"` adds the relay hostname to the allowlist +/// The env var is forwarded by the `@agentclientprotocol/codex-acp` adapter (1.x) as a +/// session-level config override (via `CODEX_CONFIG` → `thread/start config`), which is +/// equivalent to the TOML override `sandbox_workspace_write.network_access = true`. +/// That sets `NetworkSandboxPolicy::Enabled`, causing the Seatbelt policy to include +/// `(allow network-outbound)` — full outbound TCP/TLS at the OS level. /// -/// Handles `ws://`, `wss://`, `http://`, and `https://` schemes. Port is stripped — -/// Codex's domain allowlist matches on hostname only. -pub fn codex_network_args(agent_command: &str, relay_url: &str) -> Vec { +/// URL validation is preserved as a guard: injection is skipped when the relay URL cannot +/// be parsed, avoiding accidental sandbox widening for malformed configs. +/// +/// Handles `ws://`, `wss://`, `http://`, and `https://` schemes. +pub fn codex_network_env(agent_command: &str, relay_url: &str) -> Option<(String, String)> { match normalize_agent_command_identity(agent_command).as_str() { "codex" | "codex-acp" => {} - _ => return vec![], + _ => return None, } - // Use the `url` crate so ws://, wss://, http://, https:// are all handled - // correctly. On parse failure, skip injection rather than panicking. + // Validate the relay URL before injecting broader network access. On parse failure, + // skip injection rather than panicking or widening the sandbox unconditionally. let host = match Url::parse(relay_url) { Ok(u) => match u.host_str() { Some(h) => h.to_owned(), None => { tracing::warn!( relay_url, - "codex network allowlist: no host in relay URL — skipping injection" + "codex network config: no host in relay URL — skipping injection" ); - return vec![]; + return None; } }, Err(e) => { - tracing::warn!(relay_url, error = %e, "codex network allowlist: failed to parse relay URL — skipping injection"); - return vec![]; + tracing::warn!(relay_url, error = %e, "codex network config: failed to parse relay URL — skipping injection"); + return None; } }; - tracing::debug!(host, "injecting codex network allowlist for host"); + tracing::debug!(host, "injecting CODEX_CONFIG network_access for relay host"); - vec![ - "-c".into(), - "sandbox_workspace_write.network_access=true".into(), - "-c".into(), - "network_proxy.mode=\"full\"".into(), - "-c".into(), - format!("network_proxy.domains.\"{host}\"=\"allow\""), - ] + Some(( + "CODEX_CONFIG".into(), + "{\"sandbox_workspace_write\":{\"network_access\":true}}".into(), + )) } pub fn normalize_agent_args(command: &str, agent_args: Vec) -> Vec { @@ -760,16 +763,7 @@ impl Config { )); } - let mut agent_args = normalize_agent_args(&agent_command, args.agent_args); - - // Prepend Codex network allowlist flags so buzz-cli (an MCP subprocess) - // can reach the relay through Codex's sandbox proxy. No-op for non-Codex agents. - let network_args = codex_network_args(&agent_command, &args.relay_url); - if !network_args.is_empty() { - let mut merged = network_args; - merged.extend(agent_args); - agent_args = merged; - } + let agent_args = normalize_agent_args(&agent_command, args.agent_args); // Finding #49b — warn on invalid UUIDs in --channels. if let Some(ref channels) = args.channels { @@ -899,7 +893,7 @@ impl Config { // // Precedence: CLI/env args > persona values > built-in defaults. // Persona fills in what's missing. Explicit flags always win. - let (persona_system_prompt, persona_model, persona_env_vars) = + let (persona_system_prompt, persona_model, mut persona_env_vars) = match (&args.persona_pack, &args.persona_name) { (Some(pack_dir), Some(name)) => { let pack = buzz_persona::resolve::resolve_pack(pack_dir).map_err(|e| { @@ -943,6 +937,17 @@ impl Config { } let model = args.model.or(persona_model); + // Inject CODEX_CONFIG so the @agentclientprotocol/codex-acp adapter (1.x) + // opens the Seatbelt network sandbox for buzz-cli (an MCP subprocess). No-op + // for non-Codex agents or unparseable relay URLs. + let has_generated_codex_config = + if let Some(network_env) = codex_network_env(&agent_command, &args.relay_url) { + persona_env_vars.push(network_env); + true + } else { + false + }; + validate_multiple_event_handling(args.multiple_event_handling, args.dedup)?; let config = Config { @@ -978,6 +983,7 @@ impl Config { respond_to_allowlist, allowed_respond_to, persona_env_vars, + has_generated_codex_config, relay_observer: args.relay_observer, agent_owner: args.agent_owner.map(|s| s.trim().to_ascii_lowercase()), no_base_prompt: args.no_base_prompt, @@ -1348,6 +1354,7 @@ mod tests { respond_to_allowlist: HashSet::new(), allowed_respond_to: Vec::new(), persona_env_vars: vec![], + has_generated_codex_config: false, relay_observer: false, agent_owner: None, no_base_prompt: false, @@ -1510,133 +1517,100 @@ mod tests { ); } - // --- codex_network_args tests --- + // --- codex_network_env tests --- + + const CODEX_CONFIG_JSON: &str = "{\"sandbox_workspace_write\":{\"network_access\":true}}"; #[test] - fn codex_network_args_wss_url() { - let args = codex_network_args("codex-acp", "wss://sprout-oss.stage.blox.sqprod.co"); + fn codex_network_env_wss_url() { + let result = codex_network_env("codex-acp", "wss://sprout-oss.stage.blox.sqprod.co"); assert_eq!( - args, - vec![ - "-c", - "sandbox_workspace_write.network_access=true", - "-c", - "network_proxy.mode=\"full\"", - "-c", - "network_proxy.domains.\"sprout-oss.stage.blox.sqprod.co\"=\"allow\"", - ] + result, + Some(("CODEX_CONFIG".to_string(), CODEX_CONFIG_JSON.to_string())) ); } #[test] - fn codex_network_args_ws_url() { - let args = codex_network_args("codex-acp", "ws://localhost:3000"); + fn codex_network_env_ws_url() { + let result = codex_network_env("codex-acp", "ws://localhost:3000"); assert_eq!( - args, - vec![ - "-c", - "sandbox_workspace_write.network_access=true", - "-c", - "network_proxy.mode=\"full\"", - "-c", - "network_proxy.domains.\"localhost\"=\"allow\"", - ] + result, + Some(("CODEX_CONFIG".to_string(), CODEX_CONFIG_JSON.to_string())) ); } #[test] - fn codex_network_args_https_url() { - let args = codex_network_args("codex-acp", "https://relay.example.com/path"); + fn codex_network_env_https_url() { + let result = codex_network_env("codex-acp", "https://relay.example.com/path"); assert_eq!( - args, - vec![ - "-c", - "sandbox_workspace_write.network_access=true", - "-c", - "network_proxy.mode=\"full\"", - "-c", - "network_proxy.domains.\"relay.example.com\"=\"allow\"", - ] + result, + Some(("CODEX_CONFIG".to_string(), CODEX_CONFIG_JSON.to_string())) ); } #[test] - fn codex_network_args_http_url_with_port() { - let args = codex_network_args("codex-acp", "http://relay.example.com:8080/query"); + fn codex_network_env_http_url_with_port() { + let result = codex_network_env("codex-acp", "http://relay.example.com:8080/query"); assert_eq!( - args, - vec![ - "-c", - "sandbox_workspace_write.network_access=true", - "-c", - "network_proxy.mode=\"full\"", - "-c", - "network_proxy.domains.\"relay.example.com\"=\"allow\"", - ] + result, + Some(("CODEX_CONFIG".to_string(), CODEX_CONFIG_JSON.to_string())) ); } #[test] - fn codex_network_args_bare_codex_command() { - // "codex" (not "codex-acp") should also get the args. - let args = codex_network_args("codex", "wss://relay.example.com"); + fn codex_network_env_bare_codex_command() { + // "codex" (not "codex-acp") should also get the env var. + let result = codex_network_env("codex", "wss://relay.example.com"); assert_eq!( - args, - vec![ - "-c", - "sandbox_workspace_write.network_access=true", - "-c", - "network_proxy.mode=\"full\"", - "-c", - "network_proxy.domains.\"relay.example.com\"=\"allow\"", - ] + result, + Some(("CODEX_CONFIG".to_string(), CODEX_CONFIG_JSON.to_string())) ); } #[test] - fn codex_network_args_full_path_codex_command() { + fn codex_network_env_full_path_codex_command() { // Full path like /usr/local/bin/codex-acp should be normalized. - let args = codex_network_args("/usr/local/bin/codex-acp", "wss://relay.example.com"); + let result = codex_network_env("/usr/local/bin/codex-acp", "wss://relay.example.com"); assert_eq!( - args, - vec![ - "-c", - "sandbox_workspace_write.network_access=true", - "-c", - "network_proxy.mode=\"full\"", - "-c", - "network_proxy.domains.\"relay.example.com\"=\"allow\"", - ] + result, + Some(("CODEX_CONFIG".to_string(), CODEX_CONFIG_JSON.to_string())) ); } #[test] - fn codex_network_args_non_codex_agent_returns_empty() { - assert!(codex_network_args("goose", "wss://relay.example.com").is_empty()); - assert!(codex_network_args("claude-agent-acp", "wss://relay.example.com").is_empty()); - assert!(codex_network_args("buzz-agent", "wss://relay.example.com").is_empty()); + fn codex_network_env_non_codex_agent_returns_none() { + assert!(codex_network_env("goose", "wss://relay.example.com").is_none()); + assert!(codex_network_env("claude-agent-acp", "wss://relay.example.com").is_none()); + assert!(codex_network_env("buzz-agent", "wss://relay.example.com").is_none()); } #[test] - fn codex_network_args_includes_sandbox_network_access() { - // The sandbox gate must be the first flag pair — without it, the Seatbelt - // sandbox blocks outbound connections before the proxy can intercept them. - let args = codex_network_args("codex-acp", "wss://relay.example.com"); - assert_eq!(args.len(), 6, "expected 3 flag pairs (6 elements)"); - assert_eq!(args[0], "-c"); - assert_eq!(args[1], "sandbox_workspace_write.network_access=true"); + fn codex_network_env_includes_sandbox_network_access() { + // The JSON value must set sandbox_workspace_write.network_access=true — without + // it, the Seatbelt sandbox blocks outbound connections in the 1.x adapter. + let result = codex_network_env("codex-acp", "wss://relay.example.com"); + let (key, val) = result.expect("expected Some for valid codex + valid url"); + assert_eq!(key, "CODEX_CONFIG"); + assert!( + val.contains("\"sandbox_workspace_write\""), + "JSON must contain sandbox_workspace_write" + ); + assert!( + val.contains("\"network_access\":true"), + "JSON must set network_access=true" + ); } #[test] - fn codex_network_args_empty_relay_url_returns_empty() { - // Empty string fails Url::parse — graceful empty return. - assert!(codex_network_args("codex-acp", "").is_empty()); + fn codex_network_env_empty_relay_url_returns_none() { + // Empty string fails Url::parse — graceful None return. + assert!(codex_network_env("codex-acp", "").is_none()); } #[test] - fn codex_network_args_schemeless_string_returns_empty() { - // A bare string with no scheme fails Url::parse — graceful empty return. - assert!(codex_network_args("codex-acp", "not-a-url").is_empty()); + fn codex_network_env_schemeless_string_returns_none() { + // A bare string with no scheme fails Url::parse — graceful None return. + assert!(codex_network_env("codex-acp", "not-a-url").is_none()); } #[test] diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index 8627d1d42a..1835b78104 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -1143,6 +1143,7 @@ async fn tokio_main() -> Result<()> { &config.agent_command, &config.agent_args, &config.persona_env_vars, + config.has_generated_codex_config, ) .await; match spawn_result { @@ -1581,10 +1582,11 @@ async fn tokio_main() -> Result<()> { let cmd = config.agent_command.clone(); let args = config.agent_args.clone(); let env = config.persona_env_vars.clone(); + let has_codex = config.has_generated_codex_config; let observer = observer.clone(); let guard = RespawnGuard::new(idx, respawn_tx.clone()); respawn_tasks.spawn(async move { - let result = spawn_and_init(&cmd, &args, &env, idx, observer).await; + let result = spawn_and_init(&cmd, &args, &env, has_codex, idx, observer).await; guard.send(result); }); } @@ -3010,12 +3012,13 @@ fn recover_panicked_agent( let cmd = config.agent_command.clone(); let args = config.agent_args.clone(); let env = config.persona_env_vars.clone(); + let has_codex = config.has_generated_codex_config; let guard = RespawnGuard::new(i, respawn_tx.clone()); respawn_tasks.spawn(async move { if !delay.is_zero() { tokio::time::sleep(delay).await; } - let result = spawn_and_init(&cmd, &args, &env, i, observer).await; + let result = spawn_and_init(&cmd, &args, &env, has_codex, i, observer).await; guard.send(result); }); } @@ -3158,6 +3161,7 @@ fn spawn_respawn_task( let cmd = config.agent_command.clone(); let args = config.agent_args.clone(); let env = config.persona_env_vars.clone(); + let has_codex = config.has_generated_codex_config; let guard = RespawnGuard::new(index, respawn_tx.clone()); respawn_tasks.spawn(async move { // Shutdown old agent (reap child, prevent zombie). @@ -3169,7 +3173,7 @@ fn spawn_respawn_task( tokio::time::sleep(delay).await; } - let result = spawn_and_init(&cmd, &args, &env, index, observer).await; + let result = spawn_and_init(&cmd, &args, &env, has_codex, index, observer).await; guard.send(result); }); @@ -3185,10 +3189,11 @@ async fn spawn_and_init( command: &str, args: &[String], extra_env: &[(String, String)], + has_generated_codex_config: bool, agent_index: usize, observer: Option, ) -> Result<(AcpClient, u32, bool)> { - let mut acp = AcpClient::spawn(command, args, extra_env) + let mut acp = AcpClient::spawn(command, args, extra_env, has_generated_codex_config) .await .map_err(|e| anyhow::anyhow!("failed to spawn agent: {e}"))?; acp.set_observer(observer, agent_index); @@ -3230,8 +3235,8 @@ async fn run_models(args: ModelsArgs) -> Result<()> { .to_string(); // Spawn outside the timeout so we always own the child for cleanup. - // `models` subcommand doesn't use persona packs — no extra env. - let mut client = match AcpClient::spawn(&args.agent_command, &agent_args, &[]).await { + // `models` subcommand doesn't use persona packs — no extra env, no codex config. + let mut client = match AcpClient::spawn(&args.agent_command, &agent_args, &[], false).await { Ok(c) => c, Err(e) => { eprintln!("error: failed to spawn agent: {e}"); @@ -3856,6 +3861,7 @@ mod build_mcp_servers_tests { respond_to_allowlist: std::collections::HashSet::new(), allowed_respond_to: vec![], persona_env_vars: vec![], + has_generated_codex_config: false, relay_observer: false, agent_owner: None, no_base_prompt: false, @@ -4015,6 +4021,7 @@ mod error_outcome_emission_tests { respond_to_allowlist: HashSet::new(), allowed_respond_to: vec![], persona_env_vars: vec![], + has_generated_codex_config: false, relay_observer: false, agent_owner: None, no_base_prompt: false, @@ -4028,7 +4035,7 @@ mod error_outcome_emission_tests { async fn dummy_agent(index: usize) -> OwnedAgent { OwnedAgent { index, - acp: AcpClient::spawn("cat", &[], &[]) + acp: AcpClient::spawn("cat", &[], &[], false) .await .expect("spawn cat as inert agent"), state: Default::default(), diff --git a/crates/buzz-acp/src/pool.rs b/crates/buzz-acp/src/pool.rs index 5555fd6468..e859788c2a 100644 --- a/crates/buzz-acp/src/pool.rs +++ b/crates/buzz-acp/src/pool.rs @@ -3818,9 +3818,14 @@ mod tests { /// `install_steer_rx` does not panic. #[tokio::test] async fn test_send_prompt_result_clears_steer_rx_on_early_return() { - let acp = AcpClient::spawn("bash", &["-c".to_string(), "sleep 10".to_string()], &[]) - .await - .expect("failed to spawn test agent"); + let acp = AcpClient::spawn( + "bash", + &["-c".to_string(), "sleep 10".to_string()], + &[], + false, + ) + .await + .expect("failed to spawn test agent"); let mut agent = OwnedAgent { index: 0, acp, @@ -3868,9 +3873,14 @@ mod tests { /// and the next `install_steer_rx` does not panic. #[tokio::test] async fn test_send_prompt_result_is_noop_when_steer_rx_already_consumed() { - let acp = AcpClient::spawn("bash", &["-c".to_string(), "sleep 10".to_string()], &[]) - .await - .expect("failed to spawn test agent"); + let acp = AcpClient::spawn( + "bash", + &["-c".to_string(), "sleep 10".to_string()], + &[], + false, + ) + .await + .expect("failed to spawn test agent"); let agent = OwnedAgent { index: 0, acp, diff --git a/crates/buzz-acp/src/setup_mode.rs b/crates/buzz-acp/src/setup_mode.rs index 284f163fc0..da6d2995a1 100644 --- a/crates/buzz-acp/src/setup_mode.rs +++ b/crates/buzz-acp/src/setup_mode.rs @@ -62,6 +62,8 @@ pub(crate) enum AcpAvailabilityStatus { Available, /// ACP adapter binary missing; underlying CLI may be present. AdapterMissing, + /// ACP adapter binary is from the deprecated package (< 1.0). Reinstall required. + AdapterOutdated, /// CLI binary missing; ACP adapter may be present. CliMissing, /// Neither adapter nor CLI found. @@ -139,6 +141,16 @@ impl RequirementPayload { harness ) } + AcpAvailabilityStatus::AdapterOutdated => { + let harness = probe_args + .first() + .map(String::as_str) + .unwrap_or("the agent"); + format!( + "reinstall the {} ACP adapter — the installed version is outdated (open Doctor in Settings to diagnose)", + harness + ) + } AcpAvailabilityStatus::CliMissing => { let harness = probe_args .first() diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 166b1dc779..d313507cb5 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -146,7 +146,9 @@ const overrides = new Map([ // +18: CliConfigInvalid requirement surface for config-parse probe classification — // new Requirement variant + updated cli_login_requirements + 3 new probe-layer tests. // Load-bearing UX fix (bad config → clear diagnostic, not "run codex login"). - ["src-tauri/src/managed_agents/readiness.rs", 1583], + // codex-acp-package-swap: AdapterOutdated version-probe in cli_login_requirements + // (+22 lines). Load-bearing — blocks login gate for deprecated 0.16.x adapter. + ["src-tauri/src/managed_agents/readiness.rs", 1605], // applyWorkspace reposDir parameter plus the validateReposDir binding, // threaded through Tauri invokes for configurable repos_dir, plus the // harness-persona-sync `harnessOverride` create-input bit — load-bearing @@ -175,7 +177,8 @@ const overrides = new Map([ // passthrough (+2 lines). ["src/shared/api/tauri.ts", 1273], // doctor-npm-eacces-preflight: hint field added to InstallStepResult (+1 line). - ["src/shared/api/types.ts", 1001], + // codex-acp-package-swap: "adapter_outdated" variant added to AcpAvailabilityStatus (+1 line). + ["src/shared/api/types.ts", 1002], // readiness-gate: PersonaDialog.tsx threads computeLocalModeGate + // requiredCredentialEnvKeys + RequiredFieldLabel so the "New agent" dialog // shows required markers and credential amber rows (parity with @@ -209,7 +212,15 @@ const overrides = new Map([ // agent-config-propagation: the agent_command_override decision family // (divergent / create-time / update-time / apply) moved to // discovery/overrides.rs; ratcheting 802 -> 685 to bank the headroom. - ["src-tauri/src/managed_agents/discovery.rs", 685], + // codex-acp-package-swap: probe_codex_acp_major_version (+24 lines) + + // AdapterOutdated version-gate in discover_acp_runtimes (+22 lines). Both + // load-bearing — required to detect the deprecated 0.16.x adapter and + // prevent silent relay breakage after the spawn-contract change. + // codex-acp-package-swap follow-up: tempfile-based bounded stdout read + // (+18 lines), codex_adapter_availability/is_outdated helpers (+16 lines), + // cross-platform probe contract. All load-bearing — required for correct + // probe behaviour on Windows and descendant-process edge cases. + ["src-tauri/src/managed_agents/discovery.rs", 810], // identity-import-keyring: the identity resolution state machine's behavioral // matrix (46 tests over FakeIdentityStore — probe × marker × file cells, // adoption / read-back-corruption / marker-failure arms, recovery-mode diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index 26dca1444d..1d7e15ff56 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -19,6 +19,34 @@ fn active_installs() -> &'static std::sync::Mutex( + runtime_id: &str, + adapter_path: Option<&std::path::Path>, + adapter_install_commands: &'c [&'c str], +) -> Option<&'c [&'c str]> { + let needs_install = match adapter_path { + None => true, + Some(path) if runtime_id == "codex" => { + crate::managed_agents::codex_adapter_is_outdated(path) + } + Some(_) => false, + }; + if needs_install { + Some(adapter_install_commands) + } else { + None + } +} + #[tauri::command] pub async fn discover_acp_providers() -> Result, String> { tokio::task::spawn_blocking(|| { @@ -88,13 +116,20 @@ fn install_acp_runtime_blocking(runtime_id: &str) -> Result..` on stdout and exits 0. +/// The old 0.16.x adapter (`@zed-industries/codex-acp`) is a Rust binary that does +/// not recognise `--version` and exits non-zero. +/// +/// Returns the major version on success, `None` on any failure (non-zero exit, +/// unparseable output, timeout, or missing binary). +/// +/// The probe is bounded by a 5-second deadline. The child is polled with +/// [`std::process::Child::try_wait`] (the repo's standard deadline pattern) and +/// killed if it does not exit in time. +/// +/// Stdout is redirected to a temporary file rather than a pipe. A pipe's +/// `read_to_end` blocks until all file descriptors holding the write-end are +/// closed — including those inherited by forked descendants. A regular file does +/// not have this property: `read` returns EOF at the current write position +/// regardless of how many processes still have the file open. This makes the +/// post-exit read guaranteed to complete without blocking, and works identically +/// on Windows and Unix. +pub(crate) fn probe_codex_acp_major_version(binary_path: &Path) -> Option { + use std::io::{Read as _, Seek as _, SeekFrom}; + use std::time::{Duration, Instant}; + + /// Hard ceiling on how long the version probe may block discovery. + const VERSION_PROBE_TIMEOUT: Duration = Duration::from_secs(5); + + // Redirect stdout to a temporary file instead of a pipe. When the child + // exits its write-end is closed; any forked descendant that inherited the + // file descriptor can keep writing, but `read` on a regular file returns + // EOF at the current file size — not blocking on a "writer present" check. + // This is the only cross-platform way to bound the post-exit stdout read + // without O_NONBLOCK (which is Unix-only) or a reader thread. + let mut tmp = tempfile::tempfile().ok()?; + + let mut child = Command::new(binary_path) + .arg("--version") + .stdout(tmp.try_clone().ok()?) + .stderr(std::process::Stdio::null()) + .spawn() + .ok()?; + + // Poll try_wait with a deadline — the repo's standard bounded-subprocess + // pattern (see backend.rs). This returns as soon as the process exits + // rather than blocking on stdout EOF. + let deadline = Instant::now() + VERSION_PROBE_TIMEOUT; + let exit_status = loop { + match child.try_wait() { + Ok(Some(status)) => break status, + Ok(None) => { + if Instant::now() >= deadline { + let _ = child.kill(); + let _ = child.wait(); + return None; + } + std::thread::sleep(Duration::from_millis(50)); + } + Err(_) => { + let _ = child.kill(); + let _ = child.wait(); + return None; + } + } + }; + + if !exit_status.success() { + return None; + } + + // Seek to the start and read at most 4 KiB. A regular file returns EOF + // at the current write position even if a descendant still has the fd + // open, so this read is guaranteed to complete without blocking. + tmp.seek(SeekFrom::Start(0)).ok()?; + let mut buf = Vec::with_capacity(128); + let _ = (&mut tmp as &mut dyn std::io::Read) + .take(4096) + .read_to_end(&mut buf); + + let stdout = String::from_utf8_lossy(&buf); + // Output format: " .." + let version_str = stdout.trim().split_whitespace().last()?; + let major_str = version_str.split('.').next()?; + major_str.parse::().ok() +} + +/// Classifies a resolved codex-acp binary path as [`AcpAvailabilityStatus::Available`] +/// or [`AcpAvailabilityStatus::AdapterOutdated`]. +/// +/// The 0.16.x adapter (`@zed-industries/codex-acp`) does not recognise `--version` +/// and exits non-zero — that probe failure yields `AdapterOutdated`. The 1.x adapter +/// (`@agentclientprotocol/codex-acp`) prints its version and exits 0; major ≥ 1 +/// yields `Available`. +/// +/// Used by `discover_acp_runtimes`, `cli_login_requirements`, and +/// `install_acp_runtime_blocking` so the version-gate logic is not duplicated. +pub(crate) fn codex_adapter_availability(path: &Path) -> AcpAvailabilityStatus { + match probe_codex_acp_major_version(path) { + Some(major) if major >= 1 => AcpAvailabilityStatus::Available, + _ => AcpAvailabilityStatus::AdapterOutdated, + } +} + +/// Returns `true` when the codex-acp binary at `path` is outdated (major version < 1) +/// or cannot be probed. Thin wrapper around [`codex_adapter_availability`]. +pub(crate) fn codex_adapter_is_outdated(path: &Path) -> bool { + codex_adapter_availability(path) == AcpAvailabilityStatus::AdapterOutdated +} + pub fn discover_acp_runtimes() -> Vec { KNOWN_ACP_RUNTIMES .iter() @@ -624,9 +733,21 @@ pub fn discover_acp_runtimes() -> Vec { .underlying_cli .map(|cli| find_command(cli).is_some()) .unwrap_or(false); - let (availability, command, binary_path) = + let (mut availability, command, binary_path) = classify_runtime(adapter_result, runtime.underlying_cli, underlying_cli_found); + // For codex-acp: when the adapter resolves as Available, probe the + // version. An adapter with major version < 1 is treated as outdated — + // the CODEX_CONFIG spawn contract requires 1.x. + if runtime.id == "codex" + && availability == AcpAvailabilityStatus::Available + && command.as_deref() == Some("codex-acp") + { + if let Some(path_str) = &binary_path { + availability = codex_adapter_availability(&PathBuf::from(path_str)); + } + } + let underlying_cli_path = runtime .underlying_cli .and_then(find_command) @@ -646,6 +767,7 @@ pub fn discover_acp_runtimes() -> Vec { AcpAvailabilityStatus::Available => cli_hint.to_string(), AcpAvailabilityStatus::CliMissing => cli_hint.to_string(), AcpAvailabilityStatus::AdapterMissing => adapter_hint.to_string(), + AcpAvailabilityStatus::AdapterOutdated => adapter_hint.to_string(), AcpAvailabilityStatus::NotInstalled => { if !cli_hint.is_empty() && !adapter_hint.is_empty() { format!("{cli_hint} {adapter_hint}") diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests.rs b/desktop/src-tauri/src/managed_agents/discovery/tests.rs index 10413397e2..024eff8d2d 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests.rs @@ -2,10 +2,11 @@ use std::path::PathBuf; use super::overrides::{divergent_agent_command_override, update_time_agent_command_override}; use super::{ - apply_agent_command_update, classify_runtime, create_time_agent_command_override, - default_agent_command, effective_agent_command, find_via_login_shell, managed_agent_avatar_url, - normalize_agent_args, record_agent_command, BUZZ_AGENT_AVATAR_URL, CLAUDE_CODE_AVATAR_URL, - CODEX_AVATAR_URL, GOOSE_AVATAR_URL, + apply_agent_command_update, classify_runtime, codex_adapter_availability, + codex_adapter_is_outdated, create_time_agent_command_override, default_agent_command, + effective_agent_command, find_via_login_shell, managed_agent_avatar_url, normalize_agent_args, + probe_codex_acp_major_version, record_agent_command, BUZZ_AGENT_AVATAR_URL, + CLAUDE_CODE_AVATAR_URL, CODEX_AVATAR_URL, GOOSE_AVATAR_URL, }; use crate::managed_agents::AcpAvailabilityStatus; @@ -605,3 +606,203 @@ fn apply_agent_command_update_concrete_pin_keeps_materialized_runtime() { assert_eq!(record.runtime.as_deref(), Some("claude")); assert_eq!(record_agent_command(&record, &personas), "codex-acp"); } + +// ── probe_codex_acp_major_version ───────────────────────────────────────────── + +#[cfg(unix)] +#[test] +fn probe_codex_acp_major_version_parses_1x_output() { + use std::os::unix::fs::PermissionsExt; + + // Simulate `@agentclientprotocol/codex-acp 1.1.2` output (1.x adapter) + let dir = std::env::temp_dir().join(format!("buzz-probe-1x-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).expect("create temp dir"); + let bin = dir.join("codex-acp"); + std::fs::write( + &bin, + "#!/bin/sh\necho '@agentclientprotocol/codex-acp 1.1.2'\nexit 0\n", + ) + .expect("write script"); + std::fs::set_permissions(&bin, std::fs::Permissions::from_mode(0o755)).expect("chmod script"); + + let major = probe_codex_acp_major_version(&bin); + let _ = std::fs::remove_dir_all(dir); + + assert_eq!(major, Some(1), "1.x adapter must return major version 1"); +} + +#[cfg(unix)] +#[test] +fn probe_codex_acp_major_version_returns_none_for_nonzero_exit() { + use std::os::unix::fs::PermissionsExt; + + // Simulate old 0.16.x adapter: `--version` is unrecognised, exits non-zero + let dir = std::env::temp_dir().join(format!("buzz-probe-0x-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).expect("create temp dir"); + let bin = dir.join("codex-acp"); + std::fs::write(&bin, "#!/bin/sh\nexit 1\n").expect("write script"); + std::fs::set_permissions(&bin, std::fs::Permissions::from_mode(0o755)).expect("chmod script"); + + let major = probe_codex_acp_major_version(&bin); + let _ = std::fs::remove_dir_all(dir); + + assert_eq!( + major, None, + "old 0.16.x adapter (non-zero exit) must return None" + ); +} + +#[cfg(unix)] +#[test] +fn probe_codex_acp_major_version_returns_none_for_missing_binary() { + let path = std::path::Path::new("/nonexistent/path/codex-acp-does-not-exist"); + let major = probe_codex_acp_major_version(path); + assert_eq!(major, None, "missing binary must return None"); +} + +// ── codex_adapter_availability / codex_adapter_is_outdated ─────────────────── +// +// Outcome-level classification: verify helpers map probe results to the correct +// AcpAvailabilityStatus and boolean without duplicating version-gate logic. + +#[cfg(unix)] +#[test] +fn codex_adapter_availability_available_for_1x_binary() { + use std::os::unix::fs::PermissionsExt; + + let dir = std::env::temp_dir().join(format!("buzz-avail-1x-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).expect("create temp dir"); + let bin = dir.join("codex-acp"); + std::fs::write( + &bin, + "#!/bin/sh\necho '@agentclientprotocol/codex-acp 1.1.2'\nexit 0\n", + ) + .expect("write script"); + std::fs::set_permissions(&bin, std::fs::Permissions::from_mode(0o755)).expect("chmod script"); + + let status = codex_adapter_availability(&bin); + let _ = std::fs::remove_dir_all(dir); + + assert_eq!( + status, + AcpAvailabilityStatus::Available, + "1.x adapter must classify as Available" + ); +} + +#[cfg(unix)] +#[test] +fn codex_adapter_availability_outdated_for_0x_binary() { + use std::os::unix::fs::PermissionsExt; + + // Simulate old 0.16.x: `--version` exits non-zero (unrecognised flag) + let dir = std::env::temp_dir().join(format!("buzz-avail-0x-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).expect("create temp dir"); + let bin = dir.join("codex-acp"); + std::fs::write(&bin, "#!/bin/sh\nexit 1\n").expect("write script"); + std::fs::set_permissions(&bin, std::fs::Permissions::from_mode(0o755)).expect("chmod script"); + + let status = codex_adapter_availability(&bin); + let _ = std::fs::remove_dir_all(dir); + + assert_eq!( + status, + AcpAvailabilityStatus::AdapterOutdated, + "0.x adapter (non-zero exit) must classify as AdapterOutdated" + ); +} + +#[cfg(unix)] +#[test] +fn codex_adapter_availability_outdated_for_missing_binary() { + let path = std::path::Path::new("/nonexistent/codex-acp-probe-test"); + assert_eq!( + codex_adapter_availability(path), + AcpAvailabilityStatus::AdapterOutdated, + "missing binary must classify as AdapterOutdated" + ); + // Thin wrapper consistency + assert!( + codex_adapter_is_outdated(path), + "missing binary must be classified as outdated via thin wrapper" + ); +} + +#[cfg(unix)] +#[test] +fn probe_codex_acp_major_version_returns_none_for_hung_direct_child() { + use std::os::unix::fs::PermissionsExt; + use std::time::Instant; + + // Simulate a process that writes version to stdout then blocks forever. + // The probe reads stdout only after the child exits, so it will time out. + // `exec sleep 300` replaces the shell so killing the child reaps `sleep` too. + let dir = std::env::temp_dir().join(format!("buzz-probe-hung-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).expect("create temp dir"); + let bin = dir.join("codex-acp"); + std::fs::write( + &bin, + "#!/bin/sh\nprintf '@agentclientprotocol/codex-acp 1.1.2\\n'\nexec sleep 300\n", + ) + .expect("write script"); + std::fs::set_permissions(&bin, std::fs::Permissions::from_mode(0o755)).expect("chmod script"); + + let start = Instant::now(); + let major = probe_codex_acp_major_version(&bin); + let elapsed = start.elapsed(); + let _ = std::fs::remove_dir_all(dir); + + assert_eq!( + major, None, + "hung binary must return None (timeout kills child)" + ); + // The timeout is 5 s; give a 2 s margin for slow CI. + assert!( + elapsed.as_secs() < 7, + "probe must complete within timeout bound; elapsed: {elapsed:?}" + ); +} + +#[cfg(unix)] +#[test] +fn probe_codex_acp_major_version_returns_version_when_descendant_holds_pipe_open() { + use std::os::unix::fs::PermissionsExt; + use std::time::Instant; + + // Simulate a process that forks a background child which inherits stdout + // and stays alive, while the parent writes version and exits 0. + // + // Without O_NONBLOCK the pipe stays open (the descendant holds the write + // end), so read_to_end() blocks indefinitely. With O_NONBLOCK the probe + // reads whatever is in the kernel buffer after the parent exits and returns + // immediately — NOT waiting for the descendant to close its fd. + // + // `(exec sleep 60 &)` forks a subshell that execs `sleep 60`; the subshell + // inherits the parent's stdout fd and keeps it open. + let dir = std::env::temp_dir().join(format!("buzz-probe-descendant-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).expect("create temp dir"); + let bin = dir.join("codex-acp"); + std::fs::write( + &bin, + "#!/bin/sh\necho '@agentclientprotocol/codex-acp 1.1.2'\n(exec sleep 60 &)\nexit 0\n", + ) + .expect("write script"); + std::fs::set_permissions(&bin, std::fs::Permissions::from_mode(0o755)).expect("chmod script"); + + let start = Instant::now(); + let major = probe_codex_acp_major_version(&bin); + let elapsed = start.elapsed(); + let _ = std::fs::remove_dir_all(dir); + + // Must return within ~1 s: non-blocking read, no waiting for descendant. + // Give a 3 s margin for slow CI. + assert!( + elapsed.as_secs() < 3, + "probe must not block on descendant pipe; elapsed: {elapsed:?}" + ); + assert_eq!( + major, + Some(1), + "1.x version must be parsed even when descendant holds pipe open" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/readiness.rs b/desktop/src-tauri/src/managed_agents/readiness.rs index f7a1c16fb3..fb977c8051 100644 --- a/desktop/src-tauri/src/managed_agents/readiness.rs +++ b/desktop/src-tauri/src/managed_agents/readiness.rs @@ -39,6 +39,7 @@ //! UI display only. use std::collections::BTreeMap; +use std::path::Path; use serde::{Deserialize, Serialize}; @@ -46,7 +47,8 @@ use crate::managed_agents::{ agent_env::baked_build_env, config_bridge::read_goose_file_config, discovery::{ - classify_runtime, find_command, known_acp_runtime, resolve_command, KnownAcpRuntime, + classify_runtime, codex_adapter_availability, find_command, known_acp_runtime, + resolve_command, KnownAcpRuntime, }, env_vars::merged_user_env, global_config::GlobalAgentConfig, @@ -500,9 +502,22 @@ fn cli_login_requirements( .map(|cli| find_command(cli).is_some()) .unwrap_or(false); - let (availability, _cmd, _path) = + let (availability, _cmd, adapter_path) = classify_runtime(adapter_result, runtime.underlying_cli, underlying_cli_found); + // For codex-acp: if the adapter resolved as Available, probe the version. + // An adapter with major version < 1 is the deprecated package and must be + // treated as outdated (blocks login probe — the agent can't reach the relay). + let availability = if runtime.id == "codex" && availability == AcpAvailabilityStatus::Available + { + adapter_path + .as_deref() + .map(|path_str| codex_adapter_availability(Path::new(path_str))) + .unwrap_or(availability) + } else { + availability + }; + match availability { AcpAvailabilityStatus::Available => { // Both adapter and CLI are present — probe login status. diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index a6c4aa1c7c..63fb6dfadd 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -530,6 +530,8 @@ pub struct ManagedAgentLogResponse { pub enum AcpAvailabilityStatus { Available, AdapterMissing, + /// Adapter binary is present but is from the deprecated package (< 1.0). Reinstall required. + AdapterOutdated, CliMissing, NotInstalled, } diff --git a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx index bc5ce3b8e9..e4f1dfcac9 100644 --- a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx +++ b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx @@ -612,9 +612,11 @@ export function AgentDefinitionDialog({

{selectedRuntime.availability === "adapter_missing" ? `${selectedRuntime.label} CLI is installed but the ACP adapter is missing.` - : selectedRuntime.availability === "cli_missing" - ? `${selectedRuntime.label} ACP adapter is installed but the CLI is missing.` - : `${selectedRuntime.label} is not installed.`}{" "} + : selectedRuntime.availability === "adapter_outdated" + ? `${selectedRuntime.label} ACP adapter is outdated — reinstall to continue.` + : selectedRuntime.availability === "cli_missing" + ? `${selectedRuntime.label} ACP adapter is installed but the CLI is missing.` + : `${selectedRuntime.label} is not installed.`}{" "} Visit Settings > Doctor to set it up.

) : null; diff --git a/desktop/src/features/agents/ui/personaDialogPickers.tsx b/desktop/src/features/agents/ui/personaDialogPickers.tsx index 0cdb967f4c..f62866b37f 100644 --- a/desktop/src/features/agents/ui/personaDialogPickers.tsx +++ b/desktop/src/features/agents/ui/personaDialogPickers.tsx @@ -349,11 +349,13 @@ export function formatRuntimeOptionLabel(runtime: AcpRuntimeCatalogEntry) { const suffix = runtime.availability === "adapter_missing" ? " (adapter missing)" - : runtime.availability === "cli_missing" - ? " (CLI missing)" - : runtime.availability === "not_installed" - ? " (not installed)" - : ""; + : runtime.availability === "adapter_outdated" + ? " (adapter outdated)" + : runtime.availability === "cli_missing" + ? " (CLI missing)" + : runtime.availability === "not_installed" + ? " (not installed)" + : ""; return `${runtime.label}${suffix}`; } @@ -369,6 +371,8 @@ function runtimeAvailabilitySortRank( return 2; case "adapter_missing": return 3; + case "adapter_outdated": + return 3; } } diff --git a/desktop/src/features/onboarding/ui/SetupStep.tsx b/desktop/src/features/onboarding/ui/SetupStep.tsx index d28c9f7bcb..19c960ebcc 100644 --- a/desktop/src/features/onboarding/ui/SetupStep.tsx +++ b/desktop/src/features/onboarding/ui/SetupStep.tsx @@ -193,6 +193,29 @@ function RuntimeDetails({ runtime }: { runtime: AcpRuntimeCatalogEntry }) { ); } + if (runtime.availability === "adapter_outdated") { + return ( + <> +

+ ACP adapter detected but outdated — reinstall required. +

+

+ This updates the machine-global{" "} + codex-acp{" "} + adapter. Older Buzz releases using the legacy adapter contract may + lose relay access until{" "} + + @zed-industries/codex-acp@0.16.0 + {" "} + is restored. +

+

+ {runtime.installHint} +

+ + ); + } + if (runtime.availability === "cli_missing") { return ( <> diff --git a/desktop/src/features/settings/ui/DoctorSettingsPanel.tsx b/desktop/src/features/settings/ui/DoctorSettingsPanel.tsx index 6d584956a4..93eeca931f 100644 --- a/desktop/src/features/settings/ui/DoctorSettingsPanel.tsx +++ b/desktop/src/features/settings/ui/DoctorSettingsPanel.tsx @@ -31,6 +31,8 @@ function StatusIcon({ return ; case "adapter_missing": return ; + case "adapter_outdated": + return ; case "cli_missing": return ; case "not_installed": @@ -97,6 +99,7 @@ function RuntimeRow({ runtime.availability === "available" ? "bg-background/60" : runtime.availability === "adapter_missing" || + runtime.availability === "adapter_outdated" || runtime.availability === "cli_missing" ? "bg-amber-500/5" : "bg-muted/20", @@ -174,6 +177,37 @@ function RuntimeRow({ runtime={runtime} /> + ) : runtime.availability === "adapter_outdated" ? ( + <> +

+ ACP adapter found at{" "} + + {runtime.binaryPath ?? "unknown path"} + {" "} + but it is from the deprecated package. Reinstall to enable relay + connectivity. +

+

+ This updates the machine-global{" "} + + codex-acp + {" "} + adapter. Older Buzz releases using the legacy adapter contract may + lose relay access until{" "} + + @zed-industries/codex-acp@0.16.0 + {" "} + is restored. +

+

+ {runtime.installHint} +

+ + ) : runtime.availability === "cli_missing" ? ( <>

diff --git a/desktop/src/shared/api/types.ts b/desktop/src/shared/api/types.ts index 0c95c75cd0..5cd1d8b024 100644 --- a/desktop/src/shared/api/types.ts +++ b/desktop/src/shared/api/types.ts @@ -546,6 +546,7 @@ export type ControlResultFrame = { export type AcpAvailabilityStatus = | "available" | "adapter_missing" + | "adapter_outdated" | "cli_missing" | "not_installed"; diff --git a/desktop/src/shared/lib/configNudge.test.mjs b/desktop/src/shared/lib/configNudge.test.mjs index a2967fd956..8d159f9270 100644 --- a/desktop/src/shared/lib/configNudge.test.mjs +++ b/desktop/src/shared/lib/configNudge.test.mjs @@ -70,6 +70,29 @@ test("extractConfigNudge parses cli_login requirement", () => { assert.deepEqual(extractConfigNudge(withSentinel("prose", payload)), payload); }); +test("extractConfigNudge parses cli_login with adapter_outdated availability", () => { + // Backend emits availability: "adapter_outdated" when codex-acp is old (0.16.x). + // isConfigNudgeRequirement must accept this literal — regression test for the + // validator omission that caused it to be silently rejected. + const payload = { + agent_name: "Codex", + agent_pubkey: CODEX_PUBKEY, + requirements: [ + { + surface: "cli_login", + probe_args: ["codex", "login", "status"], + setup_copy: "run `codex login`", + availability: "adapter_outdated", + }, + ], + }; + assert.deepEqual( + extractConfigNudge(withSentinel("prose", payload)), + payload, + "adapter_outdated availability must be accepted by the validator", + ); +}); + test("extractConfigNudge returns null for cli_login without availability", () => { // availability is required — old-format payloads (no availability field) // must not parse so stale nudge JSON from before the Doctor-CTA update diff --git a/desktop/src/shared/lib/configNudge.ts b/desktop/src/shared/lib/configNudge.ts index 7de7190c85..dc9bab1d8c 100644 --- a/desktop/src/shared/lib/configNudge.ts +++ b/desktop/src/shared/lib/configNudge.ts @@ -32,6 +32,7 @@ export type ConfigNudgeRequirement = * Determines which message and CTA the nudge card shows: * - "available" → tooling installed, needs login * - "adapter_missing" → CLI installed but ACP adapter missing + * - "adapter_outdated" → ACP adapter present but from deprecated package; reinstall required * - "cli_missing" → ACP adapter installed but CLI missing * - "not_installed" → neither adapter nor CLI found */ @@ -140,6 +141,7 @@ function isConfigNudgeRequirement(v: unknown): v is ConfigNudgeRequirement { typeof r.setup_copy === "string" && (r.availability === "available" || r.availability === "adapter_missing" || + r.availability === "adapter_outdated" || r.availability === "cli_missing" || r.availability === "not_installed") ); diff --git a/desktop/src/shared/ui/config-nudge-attachment.tsx b/desktop/src/shared/ui/config-nudge-attachment.tsx index c1fa0cf341..8d272b63ee 100644 --- a/desktop/src/shared/ui/config-nudge-attachment.tsx +++ b/desktop/src/shared/ui/config-nudge-attachment.tsx @@ -93,6 +93,8 @@ function cliLoginMessage( return `${harness} CLI is missing`; case "adapter_missing": return `${harness} ACP adapter isn't installed`; + case "adapter_outdated": + return `${harness} ACP adapter is outdated — reinstall required`; case "available": // Tooling is present but authentication is needed — fall back to // the backend-supplied copy which has the exact login command.