From 4339116a4d372690a44afe52f3b987d23534ab3d Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 30 Jun 2026 18:13:57 -0400 Subject: [PATCH 01/30] feat(desktop): add EffectiveAgentEnv resolver and agent_readiness predicate Introduces managed_agents/readiness.rs with: - EffectiveAgentEnv: resolved process env a spawn would receive (baked floor -> runtime metadata -> user env_vars, last-wins) - resolve_effective_agent_env(): assembles EffectiveAgentEnv from record + personas + KnownAcpRuntime; no AppHandle dependency - Requirement enum with surface discriminator: NormalizedField (provider/ model dropdowns), EnvKey (credential env rows), CliLogin (claude/codex) - AgentReadiness: Ready | NotReady(Vec) - agent_readiness(): evaluates effective env against runtime requirements (buzz-agent/goose: provider+model+creds; claude/codex: CLI login probe; unknown command: always Ready) - Databricks token is NOT required (OAuth PKCE is the normal path) - 17 unit tests covering all providers and surface variants Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/managed_agents/mod.rs | 3 + .../src-tauri/src/managed_agents/readiness.rs | 728 ++++++++++++++++++ 2 files changed, 731 insertions(+) create mode 100644 desktop/src-tauri/src/managed_agents/readiness.rs diff --git a/desktop/src-tauri/src/managed_agents/mod.rs b/desktop/src-tauri/src/managed_agents/mod.rs index 68ab8f5a6..7306cf70c 100644 --- a/desktop/src-tauri/src/managed_agents/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mod.rs @@ -6,6 +6,7 @@ pub(crate) mod config_bridge; mod discovery; mod env_vars; mod nest; +pub(crate) mod readiness; mod persona_avatars; mod persona_card; pub(crate) mod persona_events; @@ -30,6 +31,8 @@ pub use env_vars::*; pub use nest::*; pub use persona_card::*; pub use personas::*; +pub(crate) use readiness::{agent_readiness, AgentReadiness, EffectiveAgentEnv, Requirement, + resolve_effective_agent_env}; #[cfg(windows)] pub use process_lifecycle::*; #[cfg(feature = "mesh-llm")] diff --git a/desktop/src-tauri/src/managed_agents/readiness.rs b/desktop/src-tauri/src/managed_agents/readiness.rs new file mode 100644 index 000000000..203d1912a --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/readiness.rs @@ -0,0 +1,728 @@ +//! Agent readiness evaluation. +//! +//! # Overview +//! +//! Before spawning a managed agent (or before deciding whether to enter +//! setup-mode nudge), the desktop must know whether the agent has every +//! piece of configuration it will need to start successfully. This module +//! provides: +//! +//! * [`EffectiveAgentEnv`] — the resolved environment a spawn would actually +//! see: baked build defaults (floor) → runtime metadata env vars → merged +//! user env_vars (last-wins) → reserved-key filtered. A separate +//! `config_file` tier tracks fields the harness reads from its config file +//! rather than the process env. +//! * [`resolve_effective_agent_env`] — assembles an `EffectiveAgentEnv` from +//! a record + personas + runtime catalog; no `AppHandle` dependency so it +//! is fully unit-testable. +//! * [`Requirement`] / [`RequirementSurface`] — structured predicates that +//! carry enough surface-discrimination for the UI to route each gap to the +//! right affordance (dropdown field vs env-var row vs CLI login step). +//! * [`AgentReadiness`] / [`agent_readiness`] — evaluates the effective env +//! against the requirements for the resolved runtime and returns `Ready` or +//! `NotReady(Vec)`. +//! +//! ## Env-assembly precedence (mirrors `spawn_agent_child`) +//! +//! 1. Baked build defaults (`baked_build_env()`) — injected first so the +//! layers above can override them. +//! 2. Runtime metadata env vars (`runtime_metadata_env_vars`) — provider / +//! model env keys derived from the record's `model`/`provider` fields and +//! the runtime's `model_env_var`/`provider_env_var`. +//! 3. Merged user env (`merged_user_env`) — the record's `env_vars` after +//! reserved-key and malformed-key filtering. Last-wins on collision. +//! +//! The config-file tier (Goose `~/.config/goose/config.yaml`) is tracked +//! separately because it is not part of the process env — the harness reads +//! it at startup. We do not evaluate it here; it is exposed for future +//! UI display only. + +use std::collections::BTreeMap; + +use serde::{Deserialize, Serialize}; + +use crate::managed_agents::{ + agent_env::baked_build_env, + discovery::{known_acp_runtime, KnownAcpRuntime}, + effective_agent_command, + env_vars::merged_user_env, + types::{ManagedAgentRecord, PersonaRecord}, +}; + +// ── EffectiveAgentEnv ───────────────────────────────────────────────────────── + +/// The resolved environment that a spawn of `record` would actually receive. +/// +/// Assembled from: baked build defaults (floor) → runtime metadata env vars +/// → merged user env_vars (last-wins) → reserved-key filtered. +/// +/// `config_file_path` is the harness config file path (if any) — not part of +/// the process env but relevant for display and future write-back dispatch. +/// `effective_command` is the resolved harness binary name (e.g. `"buzz-agent"`, +/// `"goose"`) after persona and override resolution. +#[derive(Debug, Clone)] +pub(crate) struct EffectiveAgentEnv { + /// The process-env map the spawned harness would receive. + pub env: BTreeMap, + /// Harness config file path, if any (e.g. `~/.config/goose/config.yaml`). + pub config_file_path: Option<&'static str>, + /// The resolved harness binary name (e.g. `"buzz-agent"`, `"goose"`). + pub effective_command: String, +} + +/// Assemble the effective agent env from a record, personas, and optional +/// known-runtime metadata — without an `AppHandle` so it is unit-testable. +/// +/// # Arguments +/// * `record` — the managed agent record (model/provider/env_vars/…) +/// * `personas` — all current persona records (for persona-backed resolution) +/// * `runtime` — the `KnownAcpRuntime` for the effective command, if any +pub(crate) fn resolve_effective_agent_env( + record: &ManagedAgentRecord, + personas: &[PersonaRecord], + runtime: Option<&KnownAcpRuntime>, +) -> EffectiveAgentEnv { + let effective_command = effective_agent_command( + record.persona_id.as_deref(), + personas, + record.agent_command_override.as_deref(), + ); + + // Layer 1: baked build defaults (floor — internal builds only; OSS = empty). + let mut env = baked_build_env(); + + // Layer 2: runtime metadata env vars (model / provider keys derived from + // the record's structured fields). + let effective_model = record.model.as_deref(); + let effective_provider = record.provider.as_deref(); + if let Some(rt) = runtime { + for (key, value) in super::runtime::runtime_metadata_env_vars( + rt.model_env_var, + rt.provider_env_var, + rt.provider_locked, + effective_model, + effective_provider, + ) { + env.insert(key.to_string(), value.to_string()); + } + } + + // Layer 3: merged user env (agent env_vars after reserved/malformed-key + // filtering; last-wins on collision). + let user_env = merged_user_env(&BTreeMap::new(), &record.env_vars); + env.extend(user_env); + + EffectiveAgentEnv { + env, + config_file_path: runtime.and_then(|r| r.config_file_path), + effective_command, + } +} + +// ── Requirement types ───────────────────────────────────────────────────────── + +/// A single missing piece of configuration, tagged with the UI surface that +/// owns it so the UI can route each gap to the right affordance. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "surface", rename_all = "snake_case")] +pub enum Requirement { + /// A normalized dropdown field (provider or model) that is missing. + /// Routes to the provider/model dropdown in the Edit Agent dialog. + NormalizedField { + /// Camel-case field name matching `NormalizedConfig` ("provider", "model"). + field: String, + }, + /// An env-backed credential that is absent from the effective env. + /// Routes to the env-var row editor in the Edit Agent dialog. + EnvKey { + /// The env var key name (e.g. `"ANTHROPIC_API_KEY"`). + key: String, + }, + /// A CLI authentication step that must be completed interactively. + /// Routes to a setup instruction panel in the Edit Agent dialog. + CliLogin { + /// Arguments for the login-status probe (e.g. `["claude", "auth", "status"]`). + probe_args: Vec, + /// Human-readable instruction for completing the login + /// (e.g. `"run \`codex login --with-api-key\`"`). + setup_copy: String, + }, +} + +impl Requirement { + /// Short label for logging/nudge copy. + pub(crate) fn label(&self) -> String { + match self { + Requirement::NormalizedField { field } => format!("missing {field}"), + Requirement::EnvKey { key } => format!("missing env {key}"), + Requirement::CliLogin { setup_copy, .. } => setup_copy.clone(), + } + } +} + +// ── AgentReadiness ──────────────────────────────────────────────────────────── + +/// Whether a managed agent has all required configuration to start. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "status", rename_all = "snake_case")] +pub enum AgentReadiness { + /// All required configuration is present — safe to spawn normally. + Ready, + /// One or more requirements are missing. + NotReady { + /// Surface-discriminated list of what is missing. + requirements: Vec, + }, +} + +impl AgentReadiness { + /// Returns `true` if the agent is ready to spawn. + pub(crate) fn is_ready(&self) -> bool { + matches!(self, AgentReadiness::Ready) + } + + /// Returns the missing requirements, or an empty slice if ready. + pub(crate) fn requirements(&self) -> &[Requirement] { + match self { + AgentReadiness::Ready => &[], + AgentReadiness::NotReady { requirements } => requirements, + } + } +} + +// ── agent_readiness ─────────────────────────────────────────────────────────── + +/// Evaluate whether a managed agent has all required configuration to start. +/// +/// Checks the `effective` env surface against the requirements for the +/// resolved runtime: +/// +/// * **buzz-agent / goose**: provider + model are required (both must be +/// present in the effective env or as structured fields). Additionally, +/// provider-specific credentials are required: +/// - `anthropic` → `ANTHROPIC_API_KEY` +/// - `openai` → `OPENAI_COMPAT_API_KEY` +/// - `databricks` / `databricks_v2` → `DATABRICKS_HOST` (token optional — +/// OAuth PKCE is the fallback) +/// * **claude**: a successful `claude auth status` probe. +/// * **codex**: a successful `codex login status` probe (checks the codex +/// credential store — NOT `OPENAI_API_KEY`). +/// * **unknown / custom command**: always `Ready` (no requirements known). +/// +/// Databricks note: `DATABRICKS_TOKEN` is `.unwrap_or_default()` in +/// `buzz-agent/src/config.rs:143` — it is an escape hatch for static tokens +/// but the normal path is OAuth PKCE. We intentionally do NOT mark the +/// token as required to avoid a false NotReady for users on OAuth. +pub(crate) fn agent_readiness(effective: &EffectiveAgentEnv) -> AgentReadiness { + let runtime = known_acp_runtime(&effective.effective_command); + let missing = collect_missing_requirements(effective, runtime); + if missing.is_empty() { + AgentReadiness::Ready + } else { + AgentReadiness::NotReady { + requirements: missing, + } + } +} + +/// Collect all missing requirements for the given effective env + runtime. +fn collect_missing_requirements( + effective: &EffectiveAgentEnv, + runtime: Option<&KnownAcpRuntime>, +) -> Vec { + let Some(rt) = runtime else { + // Unknown/custom command — no requirements to check. + return vec![]; + }; + + match rt.id { + "buzz-agent" => buzz_agent_requirements(effective), + "goose" => goose_requirements(effective), + "claude" => cli_login_requirements( + &["claude", "auth", "status"], + "complete Claude Code authentication by running the Claude CLI", + ), + "codex" => cli_login_requirements( + &["codex", "login", "status"], + "run `codex login --with-api-key`", + ), + _ => vec![], + } +} + +/// Requirements for buzz-agent (provider + model + provider-specific creds). +fn buzz_agent_requirements(effective: &EffectiveAgentEnv) -> Vec { + let mut missing = Vec::new(); + + // Provider is required — maps to BUZZ_AGENT_PROVIDER in the effective env. + let provider = effective.env.get("BUZZ_AGENT_PROVIDER").map(String::as_str); + if provider.is_none() { + missing.push(Requirement::NormalizedField { + field: "provider".to_string(), + }); + } + + // Model is required — maps to BUZZ_AGENT_MODEL in the effective env. + let model = effective.env.get("BUZZ_AGENT_MODEL").map(String::as_str); + if model.is_none() { + missing.push(Requirement::NormalizedField { + field: "model".to_string(), + }); + } + + // Provider-specific credential requirements. + match provider { + Some("anthropic") => { + if !effective.env.contains_key("ANTHROPIC_API_KEY") { + missing.push(Requirement::EnvKey { + key: "ANTHROPIC_API_KEY".to_string(), + }); + } + } + Some("openai") => { + if !effective.env.contains_key("OPENAI_COMPAT_API_KEY") { + missing.push(Requirement::EnvKey { + key: "OPENAI_COMPAT_API_KEY".to_string(), + }); + } + } + Some("databricks") | Some("databricks_v2") => { + // DATABRICKS_HOST is hard-required; DATABRICKS_TOKEN is optional + // (OAuth PKCE is the normal path — see buzz-agent/src/config.rs:143). + if !effective.env.contains_key("DATABRICKS_HOST") { + missing.push(Requirement::EnvKey { + key: "DATABRICKS_HOST".to_string(), + }); + } + } + _ => { + // Unknown provider or no provider yet — only the NormalizedField + // requirement above captures this gap. + } + } + + missing +} + +/// Requirements for goose (provider + model + provider-specific creds). +/// +/// Mirrors buzz-agent requirements but uses GOOSE_PROVIDER / GOOSE_MODEL. +fn goose_requirements(effective: &EffectiveAgentEnv) -> Vec { + let mut missing = Vec::new(); + + let provider = effective + .env + .get("GOOSE_PROVIDER") + .map(String::as_str); + if provider.is_none() { + missing.push(Requirement::NormalizedField { + field: "provider".to_string(), + }); + } + + let model = effective.env.get("GOOSE_MODEL").map(String::as_str); + if model.is_none() { + missing.push(Requirement::NormalizedField { + field: "model".to_string(), + }); + } + + // Provider-specific credentials — same logic as buzz-agent. + match provider { + Some("anthropic") => { + if !effective.env.contains_key("ANTHROPIC_API_KEY") { + missing.push(Requirement::EnvKey { + key: "ANTHROPIC_API_KEY".to_string(), + }); + } + } + Some("openai") => { + if !effective.env.contains_key("OPENAI_COMPAT_API_KEY") { + missing.push(Requirement::EnvKey { + key: "OPENAI_COMPAT_API_KEY".to_string(), + }); + } + } + Some("databricks") | Some("databricks_v2") => { + if !effective.env.contains_key("DATABRICKS_HOST") { + missing.push(Requirement::EnvKey { + key: "DATABRICKS_HOST".to_string(), + }); + } + } + _ => {} + } + + missing +} + +/// Requirements for CLI-login runtimes (claude, codex). +/// +/// We probe the CLI's login-status command synchronously. These probes are +/// fast (<300ms) and the results are memoized by the caller for the session +/// lifetime if desired. +fn cli_login_requirements(probe_args: &[&str], setup_copy: &str) -> Vec { + // Run the probe. A zero exit code means logged in; anything else means not. + let logged_in = std::process::Command::new(probe_args[0]) + .args(&probe_args[1..]) + .output() + .map(|o| o.status.success()) + .unwrap_or(false); + + if logged_in { + vec![] + } else { + vec![Requirement::CliLogin { + probe_args: probe_args.iter().map(|s| s.to_string()).collect(), + setup_copy: setup_copy.to_string(), + }] + } +} + +// ── Tests ───────────────────────────────────────────────────────────────────── + +#[cfg(test)] +mod tests { + use std::collections::BTreeMap; + + use super::*; + use crate::managed_agents::discovery::known_acp_runtime_exact; + + /// Build a minimal `EffectiveAgentEnv` with the given env map and command. + fn make_env(command: &str, env: BTreeMap) -> EffectiveAgentEnv { + let runtime = known_acp_runtime_exact(command); + EffectiveAgentEnv { + env, + config_file_path: runtime.and_then(|r| r.config_file_path), + effective_command: command.to_string(), + } + } + + fn env_with(pairs: &[(&str, &str)]) -> BTreeMap { + pairs + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect() + } + + // ── buzz-agent tests ────────────────────────────────────────────────── + + #[test] + fn buzz_agent_missing_provider_returns_not_ready_with_normalized_field() { + let env = make_env( + "buzz-agent", + env_with(&[("BUZZ_AGENT_MODEL", "claude-opus-4-5")]), + ); + let result = agent_readiness(&env); + assert!( + !result.is_ready(), + "missing BUZZ_AGENT_PROVIDER should be NotReady" + ); + let reqs = result.requirements(); + assert!( + reqs.contains(&Requirement::NormalizedField { + field: "provider".to_string() + }), + "requirements should include NormalizedField(provider); got {reqs:?}" + ); + } + + #[test] + fn buzz_agent_missing_model_returns_not_ready_with_normalized_field() { + let env = make_env( + "buzz-agent", + env_with(&[ + ("BUZZ_AGENT_PROVIDER", "anthropic"), + ("ANTHROPIC_API_KEY", "sk-test"), + ]), + ); + let result = agent_readiness(&env); + assert!(!result.is_ready()); + assert!(result.requirements().contains(&Requirement::NormalizedField { + field: "model".to_string() + })); + } + + #[test] + fn buzz_agent_missing_anthropic_key_returns_not_ready_with_env_key() { + let env = make_env( + "buzz-agent", + env_with(&[ + ("BUZZ_AGENT_PROVIDER", "anthropic"), + ("BUZZ_AGENT_MODEL", "claude-opus-4-5"), + ]), + ); + let result = agent_readiness(&env); + assert!(!result.is_ready()); + assert!(result.requirements().contains(&Requirement::EnvKey { + key: "ANTHROPIC_API_KEY".to_string() + })); + } + + #[test] + fn buzz_agent_missing_openai_key_returns_not_ready() { + let env = make_env( + "buzz-agent", + env_with(&[ + ("BUZZ_AGENT_PROVIDER", "openai"), + ("BUZZ_AGENT_MODEL", "gpt-4o"), + ]), + ); + let result = agent_readiness(&env); + assert!(!result.is_ready()); + assert!(result.requirements().contains(&Requirement::EnvKey { + key: "OPENAI_COMPAT_API_KEY".to_string() + })); + } + + #[test] + fn buzz_agent_anthropic_with_all_fields_is_ready() { + let env = make_env( + "buzz-agent", + env_with(&[ + ("BUZZ_AGENT_PROVIDER", "anthropic"), + ("BUZZ_AGENT_MODEL", "claude-opus-4-5"), + ("ANTHROPIC_API_KEY", "sk-test"), + ]), + ); + assert!(agent_readiness(&env).is_ready()); + } + + #[test] + fn buzz_agent_databricks_with_host_and_model_is_ready_without_token() { + // DATABRICKS_TOKEN is NOT required — OAuth PKCE is the normal path. + // No token present, no OAuth cache present → still Ready because we + // cannot evaluate OAuth state from the env map alone. + let env = make_env( + "buzz-agent", + env_with(&[ + ("BUZZ_AGENT_PROVIDER", "databricks"), + ("BUZZ_AGENT_MODEL", "dbrx-instruct"), + ("DATABRICKS_HOST", "https://dbc.example.com"), + // NOTE: no DATABRICKS_TOKEN + ]), + ); + assert!( + agent_readiness(&env).is_ready(), + "Databricks with HOST+model but no TOKEN should still be Ready (OAuth path)" + ); + } + + #[test] + fn buzz_agent_databricks_missing_host_returns_not_ready() { + let env = make_env( + "buzz-agent", + env_with(&[ + ("BUZZ_AGENT_PROVIDER", "databricks"), + ("BUZZ_AGENT_MODEL", "dbrx-instruct"), + // NOTE: no DATABRICKS_HOST + ]), + ); + let result = agent_readiness(&env); + assert!(!result.is_ready()); + assert!(result.requirements().contains(&Requirement::EnvKey { + key: "DATABRICKS_HOST".to_string() + })); + } + + #[test] + fn buzz_agent_databricks_v2_missing_host_returns_not_ready() { + let env = make_env( + "buzz-agent", + env_with(&[ + ("BUZZ_AGENT_PROVIDER", "databricks_v2"), + ("BUZZ_AGENT_MODEL", "databricks/meta-llama-4-maverick-17b-instruct"), + ]), + ); + let result = agent_readiness(&env); + assert!(!result.is_ready()); + assert!(result.requirements().contains(&Requirement::EnvKey { + key: "DATABRICKS_HOST".to_string() + })); + } + + // ── goose tests ─────────────────────────────────────────────────────── + + #[test] + fn goose_missing_provider_returns_not_ready() { + let env = make_env( + "goose", + env_with(&[("GOOSE_MODEL", "claude-opus-4-5")]), + ); + let result = agent_readiness(&env); + assert!(!result.is_ready()); + assert!(result.requirements().contains(&Requirement::NormalizedField { + field: "provider".to_string() + })); + } + + #[test] + fn goose_with_provider_and_model_and_key_is_ready() { + let env = make_env( + "goose", + env_with(&[ + ("GOOSE_PROVIDER", "anthropic"), + ("GOOSE_MODEL", "claude-opus-4-5"), + ("ANTHROPIC_API_KEY", "sk-test"), + ]), + ); + assert!(agent_readiness(&env).is_ready()); + } + + // ── codex tests ─────────────────────────────────────────────────────── + + #[test] + fn codex_not_ready_copy_does_not_mention_openai_api_key() { + // codex uses its own credential store via `codex login --with-api-key`. + // The nudge copy must NOT say "set OPENAI_API_KEY". + // We test the static not-logged-in path by constructing the requirement + // directly (the binary may or may not be installed in CI). + let reqs = cli_login_requirements( + &["codex", "login", "status"], + "run `codex login --with-api-key`", + ); + // Whether codex is installed or not, the copy (if any) must not mention OPENAI_API_KEY. + for req in &reqs { + if let Requirement::CliLogin { setup_copy, .. } = req { + assert!( + !setup_copy.contains("OPENAI_API_KEY"), + "codex nudge copy must not mention OPENAI_API_KEY; got: {setup_copy:?}" + ); + assert!( + setup_copy.contains("codex login"), + "codex nudge copy should mention `codex login`; got: {setup_copy:?}" + ); + } + } + } + + // ── custom/unknown command ───────────────────────────────────────────── + + #[test] + fn unknown_command_is_always_ready() { + let env = make_env("my-custom-harness", BTreeMap::new()); + assert!( + agent_readiness(&env).is_ready(), + "unknown/custom command should always be Ready (no requirements)" + ); + } + + // ── AgentReadiness helpers ───────────────────────────────────────────── + + #[test] + fn agent_readiness_ready_has_empty_requirements() { + assert!(AgentReadiness::Ready.requirements().is_empty()); + } + + #[test] + fn agent_readiness_not_ready_exposes_requirements() { + let r = AgentReadiness::NotReady { + requirements: vec![Requirement::EnvKey { + key: "FOO".to_string(), + }], + }; + assert!(!r.is_ready()); + assert_eq!(r.requirements().len(), 1); + } + + // ── Requirement serialization ───────────────────────────────────────── + + #[test] + fn requirement_serializes_with_surface_tag() { + let r = Requirement::NormalizedField { + field: "provider".to_string(), + }; + let json = serde_json::to_value(&r).unwrap(); + assert_eq!(json["surface"], "normalized_field"); + assert_eq!(json["field"], "provider"); + } + + #[test] + fn env_key_requirement_serializes_correctly() { + let r = Requirement::EnvKey { + key: "ANTHROPIC_API_KEY".to_string(), + }; + let json = serde_json::to_value(&r).unwrap(); + assert_eq!(json["surface"], "env_key"); + assert_eq!(json["key"], "ANTHROPIC_API_KEY"); + } + + #[test] + fn cli_login_requirement_serializes_correctly() { + let r = Requirement::CliLogin { + probe_args: vec!["codex".to_string(), "login".to_string(), "status".to_string()], + setup_copy: "run `codex login --with-api-key`".to_string(), + }; + let json = serde_json::to_value(&r).unwrap(); + assert_eq!(json["surface"], "cli_login"); + assert!(json["probe_args"].is_array()); + assert!(json["setup_copy"].as_str().unwrap().contains("codex login")); + } + + // ── resolve_effective_agent_env ───────────────────────────────────────── + + #[test] + fn resolve_effective_agent_env_user_env_wins_over_structured_fields() { + // A record whose env_vars explicitly set provider/model must win over + // any baked defaults. In OSS test builds the baked map is empty, so + // this test validates the user-env layer is present in the output. + let mut env_vars = BTreeMap::new(); + env_vars.insert("BUZZ_AGENT_PROVIDER".to_string(), "anthropic".to_string()); + env_vars.insert("BUZZ_AGENT_MODEL".to_string(), "claude-opus-4-5".to_string()); + + // Minimal record: only the fields resolve_effective_agent_env reads. + let record = crate::managed_agents::types::ManagedAgentRecord { + pubkey: "test-pubkey".to_string(), + name: "test-agent".to_string(), + persona_id: None, + private_key_nsec: String::new(), + auth_tag: None, + relay_url: String::new(), + avatar_url: None, + acp_command: "buzz-acp".to_string(), + agent_command: "buzz-agent".to_string(), + agent_command_override: None, + agent_args: vec![], + mcp_command: String::new(), + turn_timeout_seconds: 320, + idle_timeout_seconds: None, + max_turn_duration_seconds: None, + parallelism: 1, + system_prompt: None, + model: None, + provider: None, + persona_source_version: None, + mcp_toolsets: None, + env_vars, + start_on_app_launch: false, + runtime_pid: None, + backend: Default::default(), + backend_agent_id: None, + provider_binary_path: None, + persona_team_dir: None, + persona_name_in_team: None, + created_at: String::new(), + updated_at: String::new(), + last_started_at: None, + last_stopped_at: None, + last_exit_code: None, + last_error: None, + respond_to: Default::default(), + respond_to_allowlist: vec![], + relay_mesh: None, + }; + + let runtime = known_acp_runtime_exact("buzz-agent"); + let effective = resolve_effective_agent_env(&record, &[], runtime); + + // User env_vars must be present in the output (last-write-wins). + assert_eq!( + effective.env.get("BUZZ_AGENT_PROVIDER").map(String::as_str), + Some("anthropic") + ); + assert_eq!( + effective.env.get("BUZZ_AGENT_MODEL").map(String::as_str), + Some("claude-opus-4-5") + ); + } +} From f0bc528b80731f93b3ee82a63dc0c13ff52a3a7e Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 30 Jun 2026 18:18:35 -0400 Subject: [PATCH 02/30] feat(desktop): surface required credential env-key rows in Edit Agent dialog Adds provider-aware required credential rows to EnvVarsEditor: - requiredCredentialEnvKeys() in personaDialogPickers.tsx: pure function mapping runtime+provider to required env keys (mirrors Rust readiness.rs) - EnvVarsEditor gains requiredKeys prop: locked rows at top with amber highlight, read-only key name, editable masked value, Required badge when empty, inherited-value hint when persona has the key set - EditAgentDialog wires requiredEnvKeys memo (selectedRuntime + provider) into EnvVarsEditor so the required set updates live as provider changes - Databricks shows DATABRICKS_HOST only (DATABRICKS_TOKEN not required) - claude/codex show no required env rows (handled via CLI login surface) - 10 new tests covering all provider+runtime combinations Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../features/agents/ui/EditAgentDialog.tsx | 14 +++ .../src/features/agents/ui/EnvVarsEditor.tsx | 90 ++++++++++++++++++- .../ui/editAgentProviderDiscovery.test.mjs | 61 +++++++++++++ .../agents/ui/personaDialogPickers.tsx | 39 ++++++++ 4 files changed, 202 insertions(+), 2 deletions(-) diff --git a/desktop/src/features/agents/ui/EditAgentDialog.tsx b/desktop/src/features/agents/ui/EditAgentDialog.tsx index 7aa5b1073..e61d9e349 100644 --- a/desktop/src/features/agents/ui/EditAgentDialog.tsx +++ b/desktop/src/features/agents/ui/EditAgentDialog.tsx @@ -32,6 +32,7 @@ import { hasPersonaModelOption, NO_RUNTIME_DROPDOWN_VALUE, runtimeSupportsLlmProviderSelection, + requiredCredentialEnvKeys, shouldClearKnownModelForSelectionScope, sortPersonaRuntimes, type PersonaDropdownOption, @@ -230,6 +231,18 @@ export function EditAgentDialog({ const providerForDiscovery = llmProviderFieldVisible ? provider : ""; + // Required credential env keys for the currently selected runtime + provider. + // These are surfaced as first-class required rows in the EnvVarsEditor so the + // user sees exactly what is missing before attempting to start the agent. + const requiredEnvKeys = React.useMemo( + () => + requiredCredentialEnvKeys( + selectedRuntime?.id ?? selectedRuntimeId, + providerForDiscovery, + ), + [selectedRuntime?.id, selectedRuntimeId, providerForDiscovery], + ); + const { discoveredModelOptions, modelDiscoveryLoading, @@ -686,6 +699,7 @@ export function EditAgentDialog({ inheritedFrom={inheritedEnvVars} inheritedLabel="persona" onChange={setEnvVars} + requiredKeys={requiredEnvKeys} value={envVars} /> diff --git a/desktop/src/features/agents/ui/EnvVarsEditor.tsx b/desktop/src/features/agents/ui/EnvVarsEditor.tsx index fa4cc543e..0d85a1bd8 100644 --- a/desktop/src/features/agents/ui/EnvVarsEditor.tsx +++ b/desktop/src/features/agents/ui/EnvVarsEditor.tsx @@ -1,4 +1,4 @@ -import { Plus, X } from "lucide-react"; +import { AlertCircle, Lock, Plus, X } from "lucide-react"; import * as React from "react"; import { Button } from "@/shared/ui/button"; @@ -27,6 +27,15 @@ type EnvVarsEditorProps = { helperText?: string; /** Disables all editing. */ disabled?: boolean; + /** + * Env var keys that are required for the agent to start with the currently + * selected runtime + provider. Each key renders as a locked first-class row + * at the top of the editor: the key is pre-filled and read-only; the value + * is editable. If the value is already set in `value`, it is shown; otherwise + * the row is empty and marked with a "Required" badge so the user knows to + * fill it in. + */ + requiredKeys?: readonly string[]; }; type Row = { id: string; key: string; value: string }; @@ -47,6 +56,7 @@ export function EnvVarsEditor({ label = "Environment variables", helperText, disabled = false, + requiredKeys = [], }: EnvVarsEditorProps) { // Local ordered row state. Synced from `value` on mount and when the // parent supplies a value we did NOT just emit (e.g., dialog reopened @@ -82,6 +92,13 @@ export function EnvVarsEditor({ emit([...rows, { id: crypto.randomUUID(), key: "", value: "" }]); } + // Required rows are rendered before the user-editable rows. They are not + // part of `rows` state — they read from / write to `value` directly via + // `onChange`, using their key as the stable identity. + function updateRequiredValue(key: string, newValue: string) { + onChange({ ...value, [key]: newValue }); + } + return (
@@ -91,7 +108,76 @@ export function EnvVarsEditor({ ) : null}
- {rows.length === 0 ? ( + {/* Required credential rows — shown first, key is read-only */} + {requiredKeys.map((key) => { + const currentValue = value[key] ?? ""; + const isMissing = currentValue.length === 0; + return ( +
+
+
+ + + {key} + + {isMissing ? ( + + + Required + + ) : null} +
+
+ + updateRequiredValue(key, event.target.value) + } + placeholder="value" + type="password" + value={currentValue} + /> +
+ {/* Spacer to align with the remove-button column */} +
+
+ {(() => { + const inheritedValue = inheritedFrom?.[key]; + return inheritedValue !== undefined ? ( +

+ Overrides {inheritedLabel} value{" "} + + {maskInherited(inheritedValue)} + +

+ ) : null; + })()} +
+ ); + })} + + {/* User-managed rows */} + {rows.length === 0 && requiredKeys.length === 0 ? (

No variables set.

diff --git a/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs b/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs index 857d07abf..3b9a570af 100644 --- a/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs +++ b/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs @@ -4,6 +4,7 @@ import test from "node:test"; import { runtimeSupportsLlmProviderSelection, getPersonaProviderOptions, + requiredCredentialEnvKeys, } from "./personaDialogPickers.tsx"; import { shouldClearModelForRuntimeChange } from "./personaRuntimeModel.ts"; @@ -993,3 +994,63 @@ test("editAgent_bugA_unknownCommandStillFallsBackToCustom", () => { "unknown command/id must still fall back to 'custom'", ); }); + +// ── requiredCredentialEnvKeys ────────────────────────────────────────────── +// +// Guards the provider-aware credential requirements surface so Phase 2 +// required env rows stay correct as providers and runtimes change. + +test("requiredCredentialEnvKeys: buzz-agent + anthropic → ANTHROPIC_API_KEY", () => { + const keys = requiredCredentialEnvKeys("buzz-agent", "anthropic"); + assert.deepEqual(keys, ["ANTHROPIC_API_KEY"]); +}); + +test("requiredCredentialEnvKeys: buzz-agent + openai → OPENAI_COMPAT_API_KEY", () => { + const keys = requiredCredentialEnvKeys("buzz-agent", "openai"); + assert.deepEqual(keys, ["OPENAI_COMPAT_API_KEY"]); +}); + +test("requiredCredentialEnvKeys: buzz-agent + databricks → DATABRICKS_HOST only (no token)", () => { + const keys = requiredCredentialEnvKeys("buzz-agent", "databricks"); + // DATABRICKS_TOKEN is NOT required — OAuth PKCE is the normal path. + assert.deepEqual(keys, ["DATABRICKS_HOST"]); + assert.ok( + !keys.includes("DATABRICKS_TOKEN"), + "DATABRICKS_TOKEN must not be required (OAuth PKCE is the normal auth path)", + ); +}); + +test("requiredCredentialEnvKeys: buzz-agent + databricks_v2 → DATABRICKS_HOST only", () => { + const keys = requiredCredentialEnvKeys("buzz-agent", "databricks_v2"); + assert.deepEqual(keys, ["DATABRICKS_HOST"]); +}); + +test("requiredCredentialEnvKeys: goose + anthropic → ANTHROPIC_API_KEY", () => { + const keys = requiredCredentialEnvKeys("goose", "anthropic"); + assert.deepEqual(keys, ["ANTHROPIC_API_KEY"]); +}); + +test("requiredCredentialEnvKeys: goose + openai → OPENAI_COMPAT_API_KEY", () => { + const keys = requiredCredentialEnvKeys("goose", "openai"); + assert.deepEqual(keys, ["OPENAI_COMPAT_API_KEY"]); +}); + +test("requiredCredentialEnvKeys: buzz-agent + no provider → empty (provider not yet selected)", () => { + const keys = requiredCredentialEnvKeys("buzz-agent", ""); + assert.deepEqual(keys, []); +}); + +test("requiredCredentialEnvKeys: claude → empty (uses CLI login, not env keys)", () => { + const keys = requiredCredentialEnvKeys("claude", ""); + assert.deepEqual(keys, []); +}); + +test("requiredCredentialEnvKeys: codex → empty (uses CLI login, not env keys)", () => { + const keys = requiredCredentialEnvKeys("codex", ""); + assert.deepEqual(keys, []); +}); + +test("requiredCredentialEnvKeys: custom/unknown runtime → empty", () => { + const keys = requiredCredentialEnvKeys("my-custom-harness", "anthropic"); + assert.deepEqual(keys, []); +}); diff --git a/desktop/src/features/agents/ui/personaDialogPickers.tsx b/desktop/src/features/agents/ui/personaDialogPickers.tsx index e3b51cfcb..96eaf082a 100644 --- a/desktop/src/features/agents/ui/personaDialogPickers.tsx +++ b/desktop/src/features/agents/ui/personaDialogPickers.tsx @@ -75,6 +75,45 @@ function isKnownLlmProvider( return (KNOWN_LLM_PROVIDER_IDS as readonly string[]).includes(providerId); } +/** + * Returns the credential env-var keys that are required for a given + * runtime + provider combination. These are the keys that must be present + * in the agent's effective env for it to start successfully. + * + * Used by EnvVarsEditor to render first-class "required" rows that make the + * gap visible before the user tries to save or start the agent. + * + * Mirrors the Rust `readiness::buzz_agent_requirements` / + * `readiness::goose_requirements` logic — keep in sync. + */ +export function requiredCredentialEnvKeys( + runtimeId: string, + provider: string, +): readonly string[] { + const normalizedRuntime = runtimeId.trim(); + const normalizedProvider = provider.trim().toLowerCase(); + + // buzz-agent and goose both use provider-specific credentials. + if ( + normalizedRuntime === "buzz-agent" || + normalizedRuntime === "goose" + ) { + if (normalizedProvider === "anthropic") return ["ANTHROPIC_API_KEY"]; + if (normalizedProvider === "openai") return ["OPENAI_COMPAT_API_KEY"]; + if ( + normalizedProvider === "databricks" || + normalizedProvider === "databricks_v2" + ) { + // DATABRICKS_TOKEN is NOT required — OAuth PKCE is the normal path. + return ["DATABRICKS_HOST"]; + } + } + + // claude and codex handle auth via CLI login (not env keys) — those + // requirements are surfaced separately via the CliLogin surface. + return []; +} + export function runtimeSupportsLlmProviderSelection(runtimeId: string) { return runtimeId === "buzz-agent" || runtimeId === "goose"; } From 055c261b934b9bf7264d09a44a0c2615b1a01a4a Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 30 Jun 2026 18:31:56 -0400 Subject: [PATCH 03/30] feat(desktop,buzz-acp): add harness-agnostic setup-listener mode (Phase 3) When a managed agent is missing required credentials, provider, or model, the desktop now spawns buzz-acp in setup-listener mode rather than the normal agent pool. Agents in setup mode respond to @mentions with a surface-correct nudge message that names exactly what to configure and where. Desktop side (runtime.rs): - After resolving runtime_meta, calls resolve_effective_agent_env() + agent_readiness() to detect missing requirements - If NotReady, serializes requirements as BUZZ_ACP_SETUP_PAYLOAD JSON (format mirrors SetupPayload serde tags in buzz-acp) - Normal pool env vars are still set; buzz-acp detects the payload and branches before starting agents buzz-acp side (setup_mode.rs + lib.rs): - New setup_mode module: SetupPayload / RequirementPayload deserialization, run_setup_listener() event loop - setup_mode is entered via early branch in tokio_main when BUZZ_ACP_SETUP_PAYLOAD is present; normal pool path unchanged - Listener: connects to relay, subscribes to channels (mentions-only), applies author gate + event_mentions_agent filter, emits a nudge reply naming each missing requirement and the UI surface to fix it - Per-channel 30s nudge cooldown; per-event-id dedup guards replay - Membership add/remove events handled so newly-joined channels get subscriptions without a restart - 6 unit tests covering payload parse, nudge body, codex CLI copy, etc. Also extends the frontend config-surface path: - isMissingRequiredDropdownField() helper in personaDialogPickers.tsx - EditAgentDialog shows required (*) labels on model/provider dropdowns when the normalized config surface reports them as missing - reader.rs: unwrap_or fallback on resolve_with_override to tolerate agents with no provider configured (avoids panic on unset agent) Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-acp/src/lib.rs | 16 + crates/buzz-acp/src/setup_mode.rs | 591 ++++++++++++++++++ .../managed_agents/config_bridge/reader.rs | 6 +- .../config_bridge/reader_tests.rs | 15 + desktop/src-tauri/src/managed_agents/mod.rs | 2 +- .../src-tauri/src/managed_agents/runtime.rs | 59 ++ .../features/agents/ui/EditAgentDialog.tsx | 49 +- .../ui/editAgentProviderDiscovery.test.mjs | 22 + .../agents/ui/personaDialogPickers.tsx | 7 + 9 files changed, 761 insertions(+), 6 deletions(-) create mode 100644 crates/buzz-acp/src/setup_mode.rs diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index be873bdd0..9a7f99675 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -8,6 +8,7 @@ mod observer; mod pool; mod queue; mod relay; +mod setup_mode; use std::collections::{HashMap, HashSet}; use std::sync::Arc; @@ -1091,6 +1092,21 @@ async fn tokio_main() -> Result<()> { .init(); let mut config = Config::from_cli().map_err(|e| anyhow::anyhow!("configuration error: {e}"))?; + + // ── Setup-mode early branch ─────────────────────────────────────────────── + // + // When the desktop determines an agent is not ready (missing credentials, + // model, or provider), it spawns buzz-acp with BUZZ_ACP_SETUP_PAYLOAD set. + // We enter the minimal setup-listener path and never start the agent pool. + if let Some(payload) = setup_mode::SetupPayload::from_env() + .map_err(|e| anyhow::anyhow!("setup payload error: {e}"))? + { + tracing::info!( + "buzz-acp: setup payload present, entering setup-listener mode" + ); + return setup_mode::run_setup_listener(config, payload).await; + } + tracing::info!("buzz-acp starting: {}", config.summary()); let observer = config diff --git a/crates/buzz-acp/src/setup_mode.rs b/crates/buzz-acp/src/setup_mode.rs new file mode 100644 index 000000000..54fd3f6d2 --- /dev/null +++ b/crates/buzz-acp/src/setup_mode.rs @@ -0,0 +1,591 @@ +//! Setup-mode listener for not-ready agents. +//! +//! When the desktop determines an agent is `NotReady` (missing provider, +//! model, or credentials), it spawns `buzz-acp` with a setup-mode payload +//! instead of starting the normal agent pool. This module implements that +//! early-branch path: +//! +//! ```text +//! Config::from_cli() +//! └─ SetupPayload::from_env()? +//! ├─ Some(payload) → run_setup_listener(config, payload) [this module] +//! └─ None → normal pool path (unchanged) +//! ``` +//! +//! # Contract (NON-NEGOTIABLE) +//! +//! * **Desktop is the ONLY readiness source.** `buzz-acp` trusts the payload +//! passed by the desktop and does NOT re-derive readiness. +//! * **Normal startup gains no second readiness path.** The early branch is +//! entered only when `BUZZ_ACP_SETUP_PAYLOAD` is set. +//! * `spawn_key_refusal`-class identity failures are outside this path: no +//! valid key → no safe process to post as the agent. +//! +//! # Nudge mechanics +//! +//! Connect → subscribe → on each matching event: +//! 1. Apply `ignore_self` gate. +//! 2. Apply `author_allowed` gate (same as normal mode) so the nudge goes +//! only to authors the real agent would answer. +//! 3. Require an explicit @mention via `event_mentions_agent`. +//! 4. Apply `filter::match_event` so channel/kind rules still constrain. +//! 5. Build and publish a nudge reply (surface-correct copy). +//! 6. Deduplicate by event-id (reconnect replay must not double-nudge). +//! 7. Per-channel cooldown (30s) for identical payloads (hygiene). + +use std::collections::{HashMap, HashSet}; +use std::time::{Duration, Instant}; + +use anyhow::Result; +use buzz_core::kind::{ + KIND_MEMBER_ADDED_NOTIFICATION, KIND_MEMBER_REMOVED_NOTIFICATION, KIND_STREAM_MESSAGE, + KIND_WORKFLOW_APPROVAL_REQUESTED, +}; +use nostr::EventId; +use serde::{Deserialize, Serialize}; +use uuid::Uuid; + +use crate::{ + author_allowed, + config::Config, + event_mentions_agent, + filter, + relay::{HarnessRelay, RelayEventPublisher}, +}; + +// ── Payload ─────────────────────────────────────────────────────────────────── + +/// Env var carrying the JSON-encoded setup payload. +pub(crate) const SETUP_PAYLOAD_ENV_VAR: &str = "BUZZ_ACP_SETUP_PAYLOAD"; + +/// A single missing requirement, surface-discriminated so the nudge copy +/// names exactly what to set and where. +/// +/// This is the Rust counterpart to the desktop's `Requirement` type +/// (`managed_agents/readiness.rs`). Desktop serializes it; `buzz-acp` +/// deserializes and renders copy from it. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(tag = "surface", rename_all = "snake_case")] +pub(crate) enum RequirementPayload { + /// A normalized dropdown field (provider or model) missing. + NormalizedField { field: String }, + /// An env-backed credential that is absent. + EnvKey { key: String }, + /// A CLI authentication step that must be completed interactively. + CliLogin { + probe_args: Vec, + setup_copy: String, + }, +} + +impl RequirementPayload { + /// Human-readable instruction fragment for the nudge copy. + fn instruction(&self) -> String { + match self { + RequirementPayload::NormalizedField { field } => { + format!("set the **{}** field in Edit Agent dropdowns", field) + } + RequirementPayload::EnvKey { key } => { + format!("set `{}` in Edit Agent → Environment variables", key) + } + RequirementPayload::CliLogin { setup_copy, .. } => setup_copy.clone(), + } + } +} + +/// The full setup payload passed by the desktop when spawning in setup mode. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub(crate) struct SetupPayload { + /// Human-readable agent display name (for the nudge message). + pub agent_name: String, + /// Surface-discriminated list of missing requirements. + pub requirements: Vec, +} + +impl SetupPayload { + /// Read and deserialize the setup payload from the env var, if present. + /// + /// Returns `Ok(None)` when the env var is absent (normal mode). + /// Returns `Err` if the var is present but malformed. + pub(crate) fn from_env() -> Result> { + let raw = match std::env::var(SETUP_PAYLOAD_ENV_VAR) { + Ok(v) if !v.is_empty() => v, + _ => return Ok(None), + }; + let payload = serde_json::from_str::(&raw) + .map_err(|e| anyhow::anyhow!("malformed {SETUP_PAYLOAD_ENV_VAR}: {e}"))?; + Ok(Some(payload)) + } + + /// Build the nudge message body from the requirements. + fn nudge_body(&self) -> String { + if self.requirements.is_empty() { + return format!( + "**{}** needs configuration before it can respond. Open Edit Agent to configure it.", + self.agent_name, + ); + } + + let steps: Vec = self + .requirements + .iter() + .map(|r| format!("- {}", r.instruction())) + .collect(); + + format!( + "**{}** needs configuration before it can respond:\n{}\n\nOpen Edit Agent in the Buzz app to set these.", + self.agent_name, + steps.join("\n"), + ) + } +} + +// ── setup listener ──────────────────────────────────────────────────────────── + +/// Run the setup-mode event loop. +/// +/// Connects to the relay, subscribes to channels, and responds to @mentions +/// of this agent with a surface-correct setup nudge. Never starts the agent +/// pool. Runs until the relay connection drops. +pub(crate) async fn run_setup_listener(config: Config, payload: SetupPayload) -> Result<()> { + tracing::info!( + agent = %payload.agent_name, + requirements = payload.requirements.len(), + "buzz-acp entering setup mode" + ); + + let pubkey_hex = config.keys.public_key().to_hex(); + + // Parse BUZZ_AUTH_TAG for relay membership / NIP-OA. + let relay_auth_tag: Option = std::env::var("BUZZ_AUTH_TAG") + .ok() + .filter(|s| !s.is_empty()) + .and_then(|s| buzz_sdk::nip_oa::parse_auth_tag(&s).ok()); + + let startup_watermark: u64 = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_secs(); + + let mut relay = + HarnessRelay::connect(&config.relay_url, &config.keys, &pubkey_hex, relay_auth_tag) + .await + .map_err(|e| anyhow::anyhow!("setup-mode relay connect error: {e}"))?; + + if let Err(e) = relay.set_startup_watermark(startup_watermark).await { + tracing::warn!("setup-mode: failed to set startup watermark: {e}"); + } + + relay + .subscribe_membership_notifications() + .await + .map_err(|e| anyhow::anyhow!("setup-mode membership subscribe error: {e}"))?; + + tracing::info!("setup-mode: connected and subscribed to membership notifications"); + + // Resolve owner for author-gate (same priority as normal mode). + let startup_owner = crate::resolve_agent_owner(&config); + let owner_cache = crate::OwnerCache::new(startup_owner); + + // Discover channels and subscribe (using a "mentions" rule so we get + // notified when someone @-mentions the agent). + let channel_info_map = relay + .discover_channels() + .await + .map_err(|e| anyhow::anyhow!("setup-mode channel discovery error: {e}"))?; + + tracing::info!( + "setup-mode: discovered {} channel(s)", + channel_info_map.len() + ); + + let channel_ids: Vec = channel_info_map.keys().copied().collect(); + + // Build subscription rules: mentions only (setup mode must not react to + // every message in a channel). + let rules = build_setup_subscription_rules(&config); + + let channel_filters = + crate::config::resolve_channel_filters(&config, &channel_ids, &rules); + + if channel_filters.is_empty() { + tracing::warn!("setup-mode: no channel subscriptions resolved — nudge listener will sit idle"); + } + + for (channel_id, filter) in &channel_filters { + if let Err(e) = relay.subscribe_channel(*channel_id, filter.clone()).await { + tracing::warn!("setup-mode: failed to subscribe to channel {channel_id}: {e}"); + } else { + tracing::info!("setup-mode: subscribed to channel {channel_id}"); + } + } + + let publisher = relay.event_publisher(); + let rest_client = relay.rest_client(); + + // Deduplicate by event-id so reconnect replay cannot double-nudge. + let mut nudged_event_ids: HashSet = HashSet::new(); + // Per-channel cooldown: channel_id → last nudge instant. + let mut channel_last_nudge: HashMap = HashMap::new(); + const NUDGE_COOLDOWN: Duration = Duration::from_secs(30); + + loop { + let Some(buzz_event) = relay.next_event().await else { + tracing::info!("setup-mode: relay closed — exiting"); + break; + }; + + let kind_u32 = buzz_event.event.kind.as_u16() as u32; + + // Handle membership notifications so we subscribe to new channels + // and drop removed ones — no session/queue drain needed (no pool). + if kind_u32 == KIND_MEMBER_ADDED_NOTIFICATION + || kind_u32 == KIND_MEMBER_REMOVED_NOTIFICATION + { + handle_setup_membership(&mut relay, &buzz_event, &config, &rules, &channel_ids).await; + continue; + } + + // Ignore non-message kinds (relay housekeeping, etc.). + if kind_u32 != KIND_STREAM_MESSAGE && kind_u32 != KIND_WORKFLOW_APPROVAL_REQUESTED { + continue; + } + + // ignore_self: don't react to our own messages. + if buzz_event.event.pubkey.to_hex() == pubkey_hex { + continue; + } + + // Require an explicit @mention of this agent — setup mode must not + // nudge on every channel event even if subscribe_mode is "all". + if !event_mentions_agent(&buzz_event.event, &pubkey_hex) { + continue; + } + + // Apply the same author gate as normal mode so the nudge only goes + // to authors the real agent would have answered. + let author_hex = buzz_event.event.pubkey.to_hex(); + let allowed = author_allowed( + &config.respond_to, + &config.respond_to_allowlist, + &author_hex, + &owner_cache, + &rest_client, + ) + .await; + if !allowed { + tracing::debug!( + author = %author_hex, + "setup-mode: event filtered by author gate" + ); + continue; + } + + // Apply channel/kind filter rules. + let matched = filter::match_event( + &buzz_event.event, + buzz_event.channel_id, + &rules, + &pubkey_hex, + ) + .await; + if matched.is_none() { + continue; + } + + // Deduplicate: one nudge per mention event-id. + if !nudged_event_ids.insert(buzz_event.event.id) { + tracing::debug!( + event_id = %buzz_event.event.id, + "setup-mode: skipping already-nudged event" + ); + continue; + } + + // Per-channel cooldown. + let now = Instant::now(); + if let Some(last) = channel_last_nudge.get(&buzz_event.channel_id) { + if now.duration_since(*last) < NUDGE_COOLDOWN { + tracing::debug!( + channel_id = %buzz_event.channel_id, + "setup-mode: within cooldown window, skipping nudge" + ); + continue; + } + } + + channel_last_nudge.insert(buzz_event.channel_id, now); + + // Build and publish the setup nudge. + if let Err(e) = publish_setup_nudge( + &publisher, + &config.keys, + buzz_event.channel_id, + &buzz_event.event, + &payload, + ) + .await + { + tracing::warn!("setup-mode: failed to publish nudge: {e}"); + } else { + tracing::info!( + channel_id = %buzz_event.channel_id, + event_id = %buzz_event.event.id, + "setup-mode: nudge published" + ); + } + } + + Ok(()) +} + +/// Build the subscription rules used in setup mode. +/// +/// Always uses "mentions" mode: setup mode must not react to every event. +/// Even if the agent's normal config is `subscribe_mode = all`, setup mode +/// enforces explicit @mention filtering (see `event_mentions_agent` call in +/// the loop above). +fn build_setup_subscription_rules(config: &Config) -> Vec { + use crate::config::SubscribeMode; + + let kinds = config.kinds_override.clone().unwrap_or_else(|| { + vec![ + KIND_STREAM_MESSAGE, + KIND_WORKFLOW_APPROVAL_REQUESTED, + ] + }); + + match &config.subscribe_mode { + // Config mode: load the actual rules, but they will be filtered by + // the explicit event_mentions_agent check in the main loop anyway. + SubscribeMode::Config => { + match crate::config::load_rules(&config.config_path) { + Ok(rules) => rules, + Err(e) => { + tracing::warn!( + "setup-mode: could not load config rules ({e}); falling back to mentions" + ); + vec![mentions_rule(kinds)] + } + } + } + _ => vec![mentions_rule(kinds)], + } +} + +fn mentions_rule(kinds: Vec) -> filter::SubscriptionRule { + use std::sync::{atomic::AtomicU32, Arc}; + filter::SubscriptionRule { + name: "setup-mentions".into(), + channels: filter::ChannelScope::All("all".into()), + kinds, + require_mention: true, + filter: None, + compiled_filter: None, + consecutive_timeouts: Arc::new(AtomicU32::new(0)), + prompt_tag: Some("@mention".into()), + } +} + +/// Handle membership add/remove events in setup mode. +/// +/// Subscribe new channels; unsubscribe removed ones. No queue/session +/// teardown — there is no pool. +async fn handle_setup_membership( + relay: &mut HarnessRelay, + buzz_event: &crate::relay::BuzzEvent, + config: &Config, + rules: &[filter::SubscriptionRule], + _initial_channel_ids: &[Uuid], +) { + let kind_u32 = buzz_event.event.kind.as_u16() as u32; + let channel_id = buzz_event.channel_id; + + if kind_u32 == KIND_MEMBER_ADDED_NOTIFICATION { + // Subscribe to the newly-joined channel. + let mut ids = vec![channel_id]; + let filters = crate::config::resolve_channel_filters(config, &ids, rules); + for (cid, filter) in filters { + if let Err(e) = relay.subscribe_channel(cid, filter).await { + tracing::warn!("setup-mode: failed to subscribe to new channel {cid}: {e}"); + } else { + tracing::info!("setup-mode: subscribed to new channel {cid}"); + } + } + } else if kind_u32 == KIND_MEMBER_REMOVED_NOTIFICATION { + if let Err(e) = relay.unsubscribe_channel(channel_id).await { + tracing::warn!("setup-mode: failed to unsubscribe from channel {channel_id}: {e}"); + } + } +} + +/// Build and publish a setup nudge reply to the triggering event. +/// +/// Threading: flat reply to the thread root if one exists; otherwise reply +/// to the triggering event itself. P-tags the asker. +async fn publish_setup_nudge( + publisher: &RelayEventPublisher, + keys: &nostr::Keys, + channel_id: Uuid, + triggering_event: &nostr::Event, + payload: &SetupPayload, +) -> Result<()> { + use buzz_sdk::ThreadRef; + + // Parse NIP-10 thread tags to determine reply target. + let thread_tags = crate::queue::parse_thread_tags(triggering_event); + + let thread_ref = if let Some(root_str) = &thread_tags.root_event_id { + // Threaded event: reply flat to the root. + let root_id = nostr::EventId::from_hex(root_str) + .map_err(|e| anyhow::anyhow!("invalid root event id: {e}"))?; + Some(ThreadRef { + root_event_id: root_id, + parent_event_id: root_id, + }) + } else { + // Top-level event: reply to the triggering event. + Some(ThreadRef { + root_event_id: triggering_event.id, + parent_event_id: triggering_event.id, + }) + }; + + let body = payload.nudge_body(); + let author_hex = triggering_event.pubkey.to_hex(); + + let event_builder = buzz_sdk::build_message( + channel_id, + &body, + thread_ref.as_ref(), + &[&author_hex], // p-tag the asker + false, + &[], + ) + .map_err(|e| anyhow::anyhow!("failed to build setup nudge: {e}"))?; + + let signed = event_builder + .sign_with_keys(keys) + .map_err(|e| anyhow::anyhow!("failed to sign setup nudge: {e}"))?; + + publisher + .publish_event(signed) + .await + .map_err(|e| anyhow::anyhow!("failed to publish setup nudge: {e}"))?; + + Ok(()) +} + +// ── Tests ───────────────────────────────────────────────────────────────────── + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn setup_payload_from_env_returns_none_when_unset() { + // The env var is not set in test builds — should return None. + // We rely on the var not being set in the test environment. + // If it IS set (e.g., running under buzz-acp itself), skip. + if std::env::var(SETUP_PAYLOAD_ENV_VAR).is_ok() { + return; + } + let result = SetupPayload::from_env().unwrap(); + assert!(result.is_none()); + } + + #[test] + fn setup_payload_from_env_returns_err_on_malformed_json() { + // Temporarily set the env var to malformed JSON. + // Note: env var mutation in tests is not safe under parallel test + // runners, but `cargo test` runs tests in the same process so this + // is deterministic as long as the var name is unique. + // We only set it momentarily and restore immediately. + let original = std::env::var(SETUP_PAYLOAD_ENV_VAR).ok(); + std::env::set_var(SETUP_PAYLOAD_ENV_VAR, "not-valid-json{{{"); + let result = SetupPayload::from_env(); + // Restore first. + match &original { + Some(v) => std::env::set_var(SETUP_PAYLOAD_ENV_VAR, v), + None => std::env::remove_var(SETUP_PAYLOAD_ENV_VAR), + } + assert!(result.is_err(), "malformed JSON must return Err"); + } + + #[test] + fn setup_payload_deserializes_correctly() { + let json = r#"{ + "agent_name": "Fizz", + "requirements": [ + {"surface": "normalized_field", "field": "provider"}, + {"surface": "env_key", "key": "ANTHROPIC_API_KEY"} + ] + }"#; + let payload: SetupPayload = serde_json::from_str(json).unwrap(); + assert_eq!(payload.agent_name, "Fizz"); + assert_eq!(payload.requirements.len(), 2); + } + + #[test] + fn nudge_body_names_all_requirements() { + let payload = SetupPayload { + agent_name: "Fizz".to_string(), + requirements: vec![ + RequirementPayload::NormalizedField { + field: "provider".to_string(), + }, + RequirementPayload::EnvKey { + key: "ANTHROPIC_API_KEY".to_string(), + }, + ], + }; + let body = payload.nudge_body(); + assert!( + body.contains("provider"), + "nudge body should mention the missing provider field" + ); + assert!( + body.contains("ANTHROPIC_API_KEY"), + "nudge body should mention the missing env key" + ); + assert!( + body.contains("Fizz"), + "nudge body should name the agent" + ); + } + + #[test] + fn nudge_body_codex_copy_does_not_mention_openai_api_key() { + let payload = SetupPayload { + agent_name: "Codex".to_string(), + requirements: vec![RequirementPayload::CliLogin { + probe_args: vec![ + "codex".to_string(), + "login".to_string(), + "status".to_string(), + ], + setup_copy: "run `codex login --with-api-key`".to_string(), + }], + }; + let body = payload.nudge_body(); + assert!( + !body.contains("OPENAI_API_KEY"), + "codex nudge must not mention OPENAI_API_KEY; got: {body:?}" + ); + assert!( + body.contains("codex login"), + "codex nudge must mention `codex login`; got: {body:?}" + ); + } + + #[test] + fn nudge_body_empty_requirements_falls_back_to_generic() { + let payload = SetupPayload { + agent_name: "Fizz".to_string(), + requirements: vec![], + }; + let body = payload.nudge_body(); + assert!(body.contains("Fizz")); + assert!(body.contains("needs configuration")); + } +} diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/reader.rs b/desktop/src-tauri/src/managed_agents/config_bridge/reader.rs index 1c23bc6e3..778f32dab 100644 --- a/desktop/src-tauri/src/managed_agents/config_bridge/reader.rs +++ b/desktop/src-tauri/src/managed_agents/config_bridge/reader.rs @@ -360,7 +360,11 @@ fn build_provider_field( (record_provider.as_deref(), ConfigOrigin::BuzzExplicit), (file_provider.as_deref(), ConfigOrigin::ConfigFile), ]; - let (value, origin, overridden_value, overridden_origin) = resolve_with_override(tiers)?; + let (value, origin, overridden_value, overridden_origin) = match resolve_with_override(tiers) { + Some(resolved) => resolved, + None if is_required => (None, ConfigOrigin::EnvVar, None, None), + None => return None, + }; let write_via = if let Some(env_key) = provider_env_var { ConfigWriteMechanism::RespawnWithEnvVar { diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs b/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs index b35623d6a..cfb7f29c1 100644 --- a/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs +++ b/desktop/src-tauri/src/managed_agents/config_bridge/reader_tests.rs @@ -659,3 +659,18 @@ fn buzz_agent_thinking_effort_env_var_not_double_surfaced_in_advanced() { "thinking_effort must not appear in advanced when normalized" ); } + +#[test] +fn missing_required_provider_still_returns_dropdown_field() { + let provider = build_provider_field(&None, &None, Some("GOOSE_PROVIDER"), false, true) + .expect("required provider field should be surfaced even when empty"); + + assert_eq!(provider.value, None); + assert_eq!(provider.origin, ConfigOrigin::EnvVar); + assert!(provider.is_required); +} + +#[test] +fn missing_optional_provider_stays_hidden() { + assert!(build_provider_field(&None, &None, Some("GOOSE_PROVIDER"), false, false).is_none()); +} diff --git a/desktop/src-tauri/src/managed_agents/mod.rs b/desktop/src-tauri/src/managed_agents/mod.rs index 7306cf70c..56ff74172 100644 --- a/desktop/src-tauri/src/managed_agents/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mod.rs @@ -31,7 +31,7 @@ pub use env_vars::*; pub use nest::*; pub use persona_card::*; pub use personas::*; -pub(crate) use readiness::{agent_readiness, AgentReadiness, EffectiveAgentEnv, Requirement, +pub(crate) use readiness::{agent_readiness, AgentReadiness, Requirement, resolve_effective_agent_env}; #[cfg(windows)] pub use process_lifecycle::*; diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index de38c3b1b..241610a55 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -1747,6 +1747,65 @@ pub fn spawn_agent_child( if runtime_meta.is_some_and(|r| r.mcp_hooks) { command.env("MCP_HOOK_SERVERS", "*"); } + + // ── Readiness check: set setup-payload if agent is not ready ───────────── + // + // Build the effective env the agent would have at start-time, run the + // readiness predicate, and if anything is missing, serialize the payload + // into BUZZ_ACP_SETUP_PAYLOAD. buzz-acp detects this env var on startup + // and enters the minimal setup-listener mode instead of the agent pool. + // + // The JSON format mirrors `setup_mode::SetupPayload` in buzz-acp: + // { "agent_name": "...", "requirements": [{ "surface": "...", ... }] } + { + use crate::managed_agents::{ + agent_readiness, resolve_effective_agent_env, AgentReadiness, Requirement, + }; + + let effective = resolve_effective_agent_env(record, &personas, runtime_meta); + if let AgentReadiness::NotReady { requirements } = agent_readiness(&effective) { + let reqs: Vec = requirements + .into_iter() + .map(|r| match r { + Requirement::NormalizedField { field } => serde_json::json!({ + "surface": "normalized_field", + "field": field, + }), + Requirement::EnvKey { key } => serde_json::json!({ + "surface": "env_key", + "key": key, + }), + Requirement::CliLogin { + probe_args, + setup_copy, + } => serde_json::json!({ + "surface": "cli_login", + "probe_args": probe_args, + "setup_copy": setup_copy, + }), + }) + .collect(); + let payload = serde_json::json!({ + "agent_name": record.name, + "requirements": reqs, + }); + match serde_json::to_string(&payload) { + Ok(json) => { + command.env("BUZZ_ACP_SETUP_PAYLOAD", json); + eprintln!( + "buzz-desktop: agent {} not ready — spawning in setup-listener mode", + record.name + ); + } + Err(e) => { + eprintln!( + "buzz-desktop: failed to serialize setup payload for {}: {e}", + record.name + ); + } + } + } + } // Only emit BUZZ_ACP_IDLE_TIMEOUT when the user has explicitly set an // override. When unset, the buzz-acp harness applies its own default // (see `DEFAULT_IDLE_TIMEOUT_SECS` in crates/buzz-acp/src/config.rs), diff --git a/desktop/src/features/agents/ui/EditAgentDialog.tsx b/desktop/src/features/agents/ui/EditAgentDialog.tsx index e61d9e349..29821a165 100644 --- a/desktop/src/features/agents/ui/EditAgentDialog.tsx +++ b/desktop/src/features/agents/ui/EditAgentDialog.tsx @@ -2,6 +2,7 @@ import * as React from "react"; import { useAcpRuntimesQuery, + useAgentConfigSurface, usePersonasQuery, useUpdateManagedAgentMutation, } from "@/features/agents/hooks"; @@ -30,6 +31,7 @@ import { getPersonaProviderOptions, getProviderApiKeyEnvVar, hasPersonaModelOption, + isMissingRequiredDropdownField, NO_RUNTIME_DROPDOWN_VALUE, runtimeSupportsLlmProviderSelection, requiredCredentialEnvKeys, @@ -51,6 +53,27 @@ import { usePersonaModelDiscovery, } from "./usePersonaModelDiscovery"; +function RequiredFieldLabel({ + children, + htmlFor, + isRequired, +}: { + children: React.ReactNode; + htmlFor: string; + isRequired: boolean; +}) { + return ( + + ); +} + export function EditAgentDialog({ agent, open, @@ -64,6 +87,7 @@ export function EditAgentDialog({ }) { const updateMutation = useUpdateManagedAgentMutation(); const runtimesQuery = useAcpRuntimesQuery({ enabled: open }); + const configSurfaceQuery = useAgentConfigSurface(open ? agent.pubkey : null); const runtimes = runtimesQuery.data ?? []; const [name, setName] = React.useState(agent.name); @@ -230,6 +254,15 @@ export function EditAgentDialog({ ); const providerForDiscovery = llmProviderFieldVisible ? provider : ""; + const normalizedConfig = configSurfaceQuery.data?.normalized; + const modelRequired = isMissingRequiredDropdownField( + normalizedConfig?.model, + model, + ); + const providerRequired = isMissingRequiredDropdownField( + normalizedConfig?.provider, + provider, + ); // Required credential env keys for the currently selected runtime + provider. // These are surfaced as first-class required rows in the EnvVarsEditor so the @@ -586,6 +619,7 @@ export function EditAgentDialog({ disabled={updateMutation.isPending} discoveredModelOptions={discoveredModelOptions} isCustomModelEditing={isCustomModelEditing} + isRequired={modelRequired} model={model} modelDiscoveryLoading={modelDiscoveryLoading} modelDiscoveryStatus={modelDiscoveryStatus} @@ -597,6 +631,7 @@ export function EditAgentDialog({ - + { // for deciding whether to show options is sound: when discovery is null, we // fall back to staticModelOptions (length > 0), so we always have options. +test("editAgent_requiredDropdownField_onlyMarksMissingKnownField", async () => { + const { isMissingRequiredDropdownField } = await import( + "./personaDialogPickers.tsx" + ); + + assert.equal( + isMissingRequiredDropdownField({ isRequired: true }, ""), + true, + "missing required dropdown value must be marked required", + ); + assert.equal( + isMissingRequiredDropdownField({ isRequired: true }, "configured"), + false, + "configured required dropdown value must not show the missing-required mark", + ); + assert.equal( + isMissingRequiredDropdownField(null, ""), + false, + "unknown normalized field names are ignored because they do not map to a dropdown", + ); +}); + test("editAgent_modelFallback_staticOptionsWhenDiscoveryNull", () => { const staticModelOptions = [{ id: "", label: "Default model" }]; // Simulate: discoveredModelOptions === null → effectiveModelOptions is static fallback diff --git a/desktop/src/features/agents/ui/personaDialogPickers.tsx b/desktop/src/features/agents/ui/personaDialogPickers.tsx index 96eaf082a..4d09837c5 100644 --- a/desktop/src/features/agents/ui/personaDialogPickers.tsx +++ b/desktop/src/features/agents/ui/personaDialogPickers.tsx @@ -114,6 +114,13 @@ export function requiredCredentialEnvKeys( return []; } +export function isMissingRequiredDropdownField( + field: { isRequired: boolean } | null | undefined, + value: string, +) { + return field?.isRequired === true && value.trim().length === 0; +} + export function runtimeSupportsLlmProviderSelection(runtimeId: string) { return runtimeId === "buzz-agent" || runtimeId === "goose"; } From f3fceb0a9e937b1aba9e10ccb1799e18fedcafeb Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 30 Jun 2026 18:33:52 -0400 Subject: [PATCH 04/30] refactor(desktop): derive required-dropdown state from runtime ID statically Replace the async useAgentConfigSurface() query with a pure static check based on runtime ID. Only buzz-agent and goose require normalized model and provider fields; the set is known at load time and does not change. - Add runtimeRequiresNormalizedField(runtimeId, field): pure fn that returns true for buzz-agent/goose + model/provider combinations - Simplify isMissingRequiredDropdownField signature: takes a boolean isRequired flag instead of a field descriptor object - Remove useAgentConfigSurface call from EditAgentDialog: no longer needed; required-mark computation is now synchronous - Update test to call runtimeRequiresNormalizedField in the unknown-field case so the test stays accurate under the new signature Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src/features/agents/ui/EditAgentDialog.tsx | 9 ++++----- .../agents/ui/editAgentProviderDiscovery.test.mjs | 14 ++++++++------ .../features/agents/ui/personaDialogPickers.tsx | 15 +++++++++++++-- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/desktop/src/features/agents/ui/EditAgentDialog.tsx b/desktop/src/features/agents/ui/EditAgentDialog.tsx index 29821a165..59310f10a 100644 --- a/desktop/src/features/agents/ui/EditAgentDialog.tsx +++ b/desktop/src/features/agents/ui/EditAgentDialog.tsx @@ -2,7 +2,6 @@ import * as React from "react"; import { useAcpRuntimesQuery, - useAgentConfigSurface, usePersonasQuery, useUpdateManagedAgentMutation, } from "@/features/agents/hooks"; @@ -33,6 +32,7 @@ import { hasPersonaModelOption, isMissingRequiredDropdownField, NO_RUNTIME_DROPDOWN_VALUE, + runtimeRequiresNormalizedField, runtimeSupportsLlmProviderSelection, requiredCredentialEnvKeys, shouldClearKnownModelForSelectionScope, @@ -87,7 +87,6 @@ export function EditAgentDialog({ }) { const updateMutation = useUpdateManagedAgentMutation(); const runtimesQuery = useAcpRuntimesQuery({ enabled: open }); - const configSurfaceQuery = useAgentConfigSurface(open ? agent.pubkey : null); const runtimes = runtimesQuery.data ?? []; const [name, setName] = React.useState(agent.name); @@ -254,13 +253,13 @@ export function EditAgentDialog({ ); const providerForDiscovery = llmProviderFieldVisible ? provider : ""; - const normalizedConfig = configSurfaceQuery.data?.normalized; + const effectiveRuntimeId = selectedRuntime?.id ?? selectedRuntimeId; const modelRequired = isMissingRequiredDropdownField( - normalizedConfig?.model, + runtimeRequiresNormalizedField(effectiveRuntimeId, "model"), model, ); const providerRequired = isMissingRequiredDropdownField( - normalizedConfig?.provider, + runtimeRequiresNormalizedField(effectiveRuntimeId, "provider"), provider, ); diff --git a/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs b/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs index bd013ed34..0a2ce3d36 100644 --- a/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs +++ b/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs @@ -93,22 +93,24 @@ test("editAgent_providerOptions_includesCurrentIfCustom", () => { // fall back to staticModelOptions (length > 0), so we always have options. test("editAgent_requiredDropdownField_onlyMarksMissingKnownField", async () => { - const { isMissingRequiredDropdownField } = await import( - "./personaDialogPickers.tsx" - ); + const { isMissingRequiredDropdownField, runtimeRequiresNormalizedField } = + await import("./personaDialogPickers.tsx"); assert.equal( - isMissingRequiredDropdownField({ isRequired: true }, ""), + isMissingRequiredDropdownField(true, ""), true, "missing required dropdown value must be marked required", ); assert.equal( - isMissingRequiredDropdownField({ isRequired: true }, "configured"), + isMissingRequiredDropdownField(true, "configured"), false, "configured required dropdown value must not show the missing-required mark", ); assert.equal( - isMissingRequiredDropdownField(null, ""), + isMissingRequiredDropdownField( + runtimeRequiresNormalizedField("buzz-agent", "unknownField"), + "", + ), false, "unknown normalized field names are ignored because they do not map to a dropdown", ); diff --git a/desktop/src/features/agents/ui/personaDialogPickers.tsx b/desktop/src/features/agents/ui/personaDialogPickers.tsx index 4d09837c5..b4a76a023 100644 --- a/desktop/src/features/agents/ui/personaDialogPickers.tsx +++ b/desktop/src/features/agents/ui/personaDialogPickers.tsx @@ -114,11 +114,22 @@ export function requiredCredentialEnvKeys( return []; } +export function runtimeRequiresNormalizedField( + runtimeId: string, + field: string, +) { + const normalizedRuntime = runtimeId.trim(); + return ( + (normalizedRuntime === "buzz-agent" || normalizedRuntime === "goose") && + (field === "model" || field === "provider") + ); +} + export function isMissingRequiredDropdownField( - field: { isRequired: boolean } | null | undefined, + isRequired: boolean, value: string, ) { - return field?.isRequired === true && value.trim().length === 0; + return isRequired && value.trim().length === 0; } export function runtimeSupportsLlmProviderSelection(runtimeId: string) { From 5ad9a2e0b8bf4b522de0dc4564fb5c16b47ab1f0 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 30 Jun 2026 18:38:09 -0400 Subject: [PATCH 05/30] fix(desktop): restore config-surface-driven required dropdown marks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restore useAgentConfigSurface() as the source for model/provider required-mark state, replacing the static runtimeRequiresNormalizedField() predicate introduced in the previous commit. The static helper duplicated backend runtime knowledge in TypeScript. A new runtime or changed required-field set on the Rust side would be correct in KnownAcpRuntime.required_normalized_fields and the config-bridge reader, but silently unbadged in EditAgentDialog because the TS predicate was not updated. The config-surface path is already used by AgentConfigPanel and ModelPicker; NormalizedField.isRequired flows from: KnownAcpRuntime.required_normalized_fields → read_config_surface() required_fields.contains() → build_provider_field(is_required) / build_model_field(is_required) → NormalizedField { is_required } → useAgentConfigSurface().data?.normalized.{model,provider}.isRequired Restore isMissingRequiredDropdownField(field: { isRequired: boolean } | null | undefined, value) signature and remove runtimeRequiresNormalizedField. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src/features/agents/ui/EditAgentDialog.tsx | 9 +++++---- .../agents/ui/editAgentProviderDiscovery.test.mjs | 14 ++++++-------- .../features/agents/ui/personaDialogPickers.tsx | 15 ++------------- 3 files changed, 13 insertions(+), 25 deletions(-) diff --git a/desktop/src/features/agents/ui/EditAgentDialog.tsx b/desktop/src/features/agents/ui/EditAgentDialog.tsx index 59310f10a..29821a165 100644 --- a/desktop/src/features/agents/ui/EditAgentDialog.tsx +++ b/desktop/src/features/agents/ui/EditAgentDialog.tsx @@ -2,6 +2,7 @@ import * as React from "react"; import { useAcpRuntimesQuery, + useAgentConfigSurface, usePersonasQuery, useUpdateManagedAgentMutation, } from "@/features/agents/hooks"; @@ -32,7 +33,6 @@ import { hasPersonaModelOption, isMissingRequiredDropdownField, NO_RUNTIME_DROPDOWN_VALUE, - runtimeRequiresNormalizedField, runtimeSupportsLlmProviderSelection, requiredCredentialEnvKeys, shouldClearKnownModelForSelectionScope, @@ -87,6 +87,7 @@ export function EditAgentDialog({ }) { const updateMutation = useUpdateManagedAgentMutation(); const runtimesQuery = useAcpRuntimesQuery({ enabled: open }); + const configSurfaceQuery = useAgentConfigSurface(open ? agent.pubkey : null); const runtimes = runtimesQuery.data ?? []; const [name, setName] = React.useState(agent.name); @@ -253,13 +254,13 @@ export function EditAgentDialog({ ); const providerForDiscovery = llmProviderFieldVisible ? provider : ""; - const effectiveRuntimeId = selectedRuntime?.id ?? selectedRuntimeId; + const normalizedConfig = configSurfaceQuery.data?.normalized; const modelRequired = isMissingRequiredDropdownField( - runtimeRequiresNormalizedField(effectiveRuntimeId, "model"), + normalizedConfig?.model, model, ); const providerRequired = isMissingRequiredDropdownField( - runtimeRequiresNormalizedField(effectiveRuntimeId, "provider"), + normalizedConfig?.provider, provider, ); diff --git a/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs b/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs index 0a2ce3d36..bd013ed34 100644 --- a/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs +++ b/desktop/src/features/agents/ui/editAgentProviderDiscovery.test.mjs @@ -93,24 +93,22 @@ test("editAgent_providerOptions_includesCurrentIfCustom", () => { // fall back to staticModelOptions (length > 0), so we always have options. test("editAgent_requiredDropdownField_onlyMarksMissingKnownField", async () => { - const { isMissingRequiredDropdownField, runtimeRequiresNormalizedField } = - await import("./personaDialogPickers.tsx"); + const { isMissingRequiredDropdownField } = await import( + "./personaDialogPickers.tsx" + ); assert.equal( - isMissingRequiredDropdownField(true, ""), + isMissingRequiredDropdownField({ isRequired: true }, ""), true, "missing required dropdown value must be marked required", ); assert.equal( - isMissingRequiredDropdownField(true, "configured"), + isMissingRequiredDropdownField({ isRequired: true }, "configured"), false, "configured required dropdown value must not show the missing-required mark", ); assert.equal( - isMissingRequiredDropdownField( - runtimeRequiresNormalizedField("buzz-agent", "unknownField"), - "", - ), + isMissingRequiredDropdownField(null, ""), false, "unknown normalized field names are ignored because they do not map to a dropdown", ); diff --git a/desktop/src/features/agents/ui/personaDialogPickers.tsx b/desktop/src/features/agents/ui/personaDialogPickers.tsx index b4a76a023..4d09837c5 100644 --- a/desktop/src/features/agents/ui/personaDialogPickers.tsx +++ b/desktop/src/features/agents/ui/personaDialogPickers.tsx @@ -114,22 +114,11 @@ export function requiredCredentialEnvKeys( return []; } -export function runtimeRequiresNormalizedField( - runtimeId: string, - field: string, -) { - const normalizedRuntime = runtimeId.trim(); - return ( - (normalizedRuntime === "buzz-agent" || normalizedRuntime === "goose") && - (field === "model" || field === "provider") - ); -} - export function isMissingRequiredDropdownField( - isRequired: boolean, + field: { isRequired: boolean } | null | undefined, value: string, ) { - return isRequired && value.trim().length === 0; + return field?.isRequired === true && value.trim().length === 0; } export function runtimeSupportsLlmProviderSelection(runtimeId: string) { From 8d9ca85a7b43f150ac7ac94a0de53648600d5792 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 30 Jun 2026 18:49:06 -0400 Subject: [PATCH 06/30] fix(desktop,buzz-acp): close setup-payload handoff invariant defects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from Thufir's Pass-1 seam review: [CRITICAL] BUZZ_ACP_SETUP_PAYLOAD was not in RESERVED_ENV_KEYS and the Ready path did not remove it, so a saved agent env var or ambient parent-process value could forge setup mode on a Ready agent or suppress it on a NotReady one. Fix: add the key to RESERVED_ENV_KEYS; compute the optional payload first, then unconditionally env_remove("BUZZ_ACP_SETUP_PAYLOAD") after user env is written, and set it only when desktop computed NotReady. [IMPORTANT] run_setup_listener() broke on relay close instead of reconnecting, making the advertised nudged_event_ids replay-dedup guard unreachable. Fix: mirror the normal-mode reconnect branch (relay.reconnect().await, exit only if background task is gone). [IMPORTANT] The six existing setup-mode tests covered payload parsing and nudge copy only — not the loop-wiring for the two safety-critical guards. Fix: extract should_nudge_for_event() as a pure helper that captures the author-gate verdict, event-id dedup, and per-channel cooldown; refactor the loop to call it; add two targeted tests: test_non_allowlisted_author_returns_no_nudge (author_allowed=false → no nudge, dedup set stays empty) and test_same_event_id_twice_nudges_ exactly_once (replay dedup via should_nudge_for_event). Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-acp/src/setup_mode.rs | 176 ++++++++++++++---- .../src-tauri/src/managed_agents/env_vars.rs | 4 + .../src-tauri/src/managed_agents/runtime.rs | 99 ++++++---- 3 files changed, 205 insertions(+), 74 deletions(-) diff --git a/crates/buzz-acp/src/setup_mode.rs b/crates/buzz-acp/src/setup_mode.rs index 54fd3f6d2..032484356 100644 --- a/crates/buzz-acp/src/setup_mode.rs +++ b/crates/buzz-acp/src/setup_mode.rs @@ -146,7 +146,9 @@ impl SetupPayload { /// /// Connects to the relay, subscribes to channels, and responds to @mentions /// of this agent with a surface-correct setup nudge. Never starts the agent -/// pool. Runs until the relay connection drops. +/// pool. Reconnects on relay close (mirroring normal mode) so the nudge +/// listener survives transient disconnects; `nudged_event_ids` deduplication +/// guards against replay on reconnect. pub(crate) async fn run_setup_listener(config: Config, payload: SetupPayload) -> Result<()> { tracing::info!( agent = %payload.agent_name, @@ -231,8 +233,12 @@ pub(crate) async fn run_setup_listener(config: Config, payload: SetupPayload) -> loop { let Some(buzz_event) = relay.next_event().await else { - tracing::info!("setup-mode: relay closed — exiting"); - break; + tracing::warn!("setup-mode: relay event stream ended — requesting reconnect"); + if let Err(e) = relay.reconnect().await { + tracing::error!("setup-mode: relay background task is gone: {e} — exiting"); + break; + } + continue; }; let kind_u32 = buzz_event.event.kind.as_u16() as u32; @@ -273,48 +279,31 @@ pub(crate) async fn run_setup_listener(config: Config, payload: SetupPayload) -> &rest_client, ) .await; - if !allowed { - tracing::debug!( - author = %author_hex, - "setup-mode: event filtered by author gate" - ); - continue; - } // Apply channel/kind filter rules. - let matched = filter::match_event( + let filter_matched = filter::match_event( &buzz_event.event, buzz_event.channel_id, &rules, &pubkey_hex, ) - .await; - if matched.is_none() { - continue; - } + .await + .is_some(); - // Deduplicate: one nudge per mention event-id. - if !nudged_event_ids.insert(buzz_event.event.id) { - tracing::debug!( - event_id = %buzz_event.event.id, - "setup-mode: skipping already-nudged event" - ); + // Pure gate: author gate verdict + event-id dedup + per-channel cooldown. + if !should_nudge_for_event( + buzz_event.event.id, + buzz_event.channel_id, + allowed, + filter_matched, + &mut nudged_event_ids, + &channel_last_nudge, + NUDGE_COOLDOWN, + ) { continue; } - // Per-channel cooldown. - let now = Instant::now(); - if let Some(last) = channel_last_nudge.get(&buzz_event.channel_id) { - if now.duration_since(*last) < NUDGE_COOLDOWN { - tracing::debug!( - channel_id = %buzz_event.channel_id, - "setup-mode: within cooldown window, skipping nudge" - ); - continue; - } - } - - channel_last_nudge.insert(buzz_event.channel_id, now); + channel_last_nudge.insert(buzz_event.channel_id, Instant::now()); // Build and publish the setup nudge. if let Err(e) = publish_setup_nudge( @@ -339,6 +328,49 @@ pub(crate) async fn run_setup_listener(config: Config, payload: SetupPayload) -> Ok(()) } +/// Outcome of the pure per-event gate checks in setup mode. +/// +/// Callers compute the async gates (`author_allowed`, `filter::match_event`) +/// up-front, then pass the boolean results here. This helper handles +/// everything that is synchronous and stateful: the author gate verdict, +/// event-id dedup, and per-channel cooldown. +/// +/// Returns `true` when the event should produce a nudge; the caller must then +/// also record the cooldown via `channel_last_nudge.insert(channel_id, now)`. +#[must_use] +pub(crate) fn should_nudge_for_event( + event_id: EventId, + channel_id: Uuid, + author_allowed: bool, + filter_matched: bool, + nudged_event_ids: &mut HashSet, + channel_last_nudge: &HashMap, + nudge_cooldown: Duration, +) -> bool { + if !author_allowed { + tracing::debug!("setup-mode: event filtered by author gate"); + return false; + } + if !filter_matched { + return false; + } + if !nudged_event_ids.insert(event_id) { + tracing::debug!(%event_id, "setup-mode: skipping already-nudged event"); + return false; + } + let now = Instant::now(); + if let Some(last) = channel_last_nudge.get(&channel_id) { + if now.duration_since(*last) < nudge_cooldown { + tracing::debug!(%channel_id, "setup-mode: within cooldown window, skipping nudge"); + // Undo the dedup insert so a post-cooldown retry is treated as + // a new nudge opportunity. + nudged_event_ids.remove(&event_id); + return false; + } + } + true +} + /// Build the subscription rules used in setup mode. /// /// Always uses "mentions" mode: setup mode must not react to every event. @@ -588,4 +620,80 @@ mod tests { assert!(body.contains("Fizz")); assert!(body.contains("needs configuration")); } + + // ── should_nudge_for_event gate tests ───────────────────────────────────── + // + // These tests exercise the loop-wiring for the two safety-critical guards: + // (a) non-allowlisted author → no nudge, (b) same event-id → exactly one + // nudge. They use the extracted `should_nudge_for_event` helper, which is + // the exact code the live loop calls. + + fn fake_event_id(byte: u8) -> EventId { + EventId::from_byte_array([byte; 32]) + } + + #[test] + fn test_non_allowlisted_author_returns_no_nudge() { + // author_allowed = false → should return false regardless of other args. + let mut dedup: HashSet = HashSet::new(); + let cooldown_map: HashMap = HashMap::new(); + let event_id = fake_event_id(0xAA); + let channel_id = Uuid::nil(); + + let result = should_nudge_for_event( + event_id, + channel_id, + false, // author NOT allowed + true, // filter matched — would otherwise nudge + &mut dedup, + &cooldown_map, + Duration::from_secs(30), + ); + + assert!( + !result, + "non-allowlisted author must not produce a nudge" + ); + // Dedup set must remain empty — no phantom insertion for blocked author. + assert!( + dedup.is_empty(), + "dedup set must not record event for blocked author" + ); + } + + #[test] + fn test_same_event_id_twice_nudges_exactly_once() { + // The first call with a given event-id should return true; the second + // call with the identical id must return false (replay dedup). + let mut dedup: HashSet = HashSet::new(); + let cooldown_map: HashMap = HashMap::new(); + let event_id = fake_event_id(0xBB); + let channel_id = Uuid::nil(); + + let first = should_nudge_for_event( + event_id, + channel_id, + true, // allowed + true, // matched + &mut dedup, + &cooldown_map, + Duration::from_secs(30), + ); + assert!(first, "first occurrence must be accepted"); + + // Simulate reconnect replay: same event arrives again. + let second = should_nudge_for_event( + event_id, + channel_id, + true, // allowed + true, // matched + &mut dedup, + &cooldown_map, + Duration::from_secs(30), + ); + assert!( + !second, + "replay of the same event-id must be rejected (dedup)" + ); + } } diff --git a/desktop/src-tauri/src/managed_agents/env_vars.rs b/desktop/src-tauri/src/managed_agents/env_vars.rs index 4383086c1..57ed8739b 100644 --- a/desktop/src-tauri/src/managed_agents/env_vars.rs +++ b/desktop/src-tauri/src/managed_agents/env_vars.rs @@ -94,6 +94,10 @@ pub(crate) const RESERVED_ENV_KEYS: &[&str] = &[ "BUZZ_ACP_RESPOND_TO", "BUZZ_ACP_RESPOND_TO_ALLOWLIST", "BUZZ_ACP_AGENT_OWNER", + // Readiness handoff: desktop is the ONLY readiness source. A saved or + // ambient env var must not be able to forge setup mode (NotReady) on a + // Ready agent or suppress it (empty/stale payload) on a NotReady one. + "BUZZ_ACP_SETUP_PAYLOAD", ]; pub(crate) fn is_reserved_env_key(key: &str) -> bool { diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 241610a55..ae6fcb177 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -1755,6 +1755,12 @@ pub fn spawn_agent_child( // into BUZZ_ACP_SETUP_PAYLOAD. buzz-acp detects this env var on startup // and enters the minimal setup-listener mode instead of the agent pool. // + // SECURITY: BUZZ_ACP_SETUP_PAYLOAD is in RESERVED_ENV_KEYS so user env + // cannot set it, but we also explicitly remove it after writing user env + // to guard against the parent-process environment. We then set it only + // when desktop has computed NotReady — the desktop is the sole readiness + // source and buzz-acp only transports the payload. + // // The JSON format mirrors `setup_mode::SetupPayload` in buzz-acp: // { "agent_name": "...", "requirements": [{ "surface": "...", ... }] } { @@ -1763,47 +1769,60 @@ pub fn spawn_agent_child( }; let effective = resolve_effective_agent_env(record, &personas, runtime_meta); - if let AgentReadiness::NotReady { requirements } = agent_readiness(&effective) { - let reqs: Vec = requirements - .into_iter() - .map(|r| match r { - Requirement::NormalizedField { field } => serde_json::json!({ - "surface": "normalized_field", - "field": field, - }), - Requirement::EnvKey { key } => serde_json::json!({ - "surface": "env_key", - "key": key, - }), - Requirement::CliLogin { - probe_args, - setup_copy, - } => serde_json::json!({ - "surface": "cli_login", - "probe_args": probe_args, - "setup_copy": setup_copy, - }), - }) - .collect(); - let payload = serde_json::json!({ - "agent_name": record.name, - "requirements": reqs, - }); - match serde_json::to_string(&payload) { - Ok(json) => { - command.env("BUZZ_ACP_SETUP_PAYLOAD", json); - eprintln!( - "buzz-desktop: agent {} not ready — spawning in setup-listener mode", - record.name - ); - } - Err(e) => { - eprintln!( - "buzz-desktop: failed to serialize setup payload for {}: {e}", - record.name - ); + // Compute the optional payload before touching the command. + let setup_payload_json = + if let AgentReadiness::NotReady { requirements } = agent_readiness(&effective) { + let reqs: Vec = requirements + .into_iter() + .map(|r| match r { + Requirement::NormalizedField { field } => serde_json::json!({ + "surface": "normalized_field", + "field": field, + }), + Requirement::EnvKey { key } => serde_json::json!({ + "surface": "env_key", + "key": key, + }), + Requirement::CliLogin { + probe_args, + setup_copy, + } => serde_json::json!({ + "surface": "cli_login", + "probe_args": probe_args, + "setup_copy": setup_copy, + }), + }) + .collect(); + let payload = serde_json::json!({ + "agent_name": record.name, + "requirements": reqs, + }); + match serde_json::to_string(&payload) { + Ok(json) => Some(json), + Err(e) => { + eprintln!( + "buzz-desktop: failed to serialize setup payload for {}: {e}", + record.name + ); + None + } } - } + } else { + None + }; + + // Always remove the key after user env has been written (above) so + // that a saved agent env var or an ambient parent-process value + // cannot forge or suppress setup mode. + command.env_remove("BUZZ_ACP_SETUP_PAYLOAD"); + + // Set the payload only when desktop computed NotReady. + if let Some(json) = setup_payload_json { + command.env("BUZZ_ACP_SETUP_PAYLOAD", json); + eprintln!( + "buzz-desktop: agent {} not ready — spawning in setup-listener mode", + record.name + ); } } // Only emit BUZZ_ACP_IDLE_TIMEOUT when the user has explicitly set an From dda84a75ab8721a2f5ff6c37af13f442e50f9d8e Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 30 Jun 2026 18:59:45 -0400 Subject: [PATCH 07/30] fix(buzz-acp): eliminate env-mutating tests via pure parser seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two setup-payload tests raced on BUZZ_ACP_SETUP_PAYLOAD under Rust's default parallel test runner: setup_payload_from_env_returns_none_when_ unset read the global env while setup_payload_from_env_returns_err_on_ malformed_json was mutating it with set_var/remove_var, causing non-deterministic failures on filtered runs. Fix: extract SetupPayload::from_raw_env_value(raw: Option) as the pure parser core; refactor from_env() to delegate to it (no behavior change). Rewrite the two flaky tests to call from_raw_env_value directly with None / Some("not-valid-json{{{"): no global env mutation, safe to run concurrently. Add a third test for the empty-string case. Delete the misleading safety comment that claimed same-process test serialization (wrong: cargo test is multi-threaded by default). Also fix a stale comment at the env_remove call site in runtime.rs that said the key is removed "after user env has been written (above)" — merged_user_env() actually writes below. Rewrote it to name the two real guards: RESERVED_ENV_KEYS strip (guard 1, handles user/persona env) and env_remove (guard 2, clears ambient parent-process env), with a note that ordering relative to merged_user_env() is NOT what makes this safe. Also drop unused mut on ids vec in handle_setup_membership(). Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-acp/src/setup_mode.rs | 54 ++++++++++--------- .../src-tauri/src/managed_agents/runtime.rs | 13 +++-- 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/crates/buzz-acp/src/setup_mode.rs b/crates/buzz-acp/src/setup_mode.rs index 032484356..015544eb3 100644 --- a/crates/buzz-acp/src/setup_mode.rs +++ b/crates/buzz-acp/src/setup_mode.rs @@ -108,8 +108,21 @@ impl SetupPayload { /// Returns `Ok(None)` when the env var is absent (normal mode). /// Returns `Err` if the var is present but malformed. pub(crate) fn from_env() -> Result> { - let raw = match std::env::var(SETUP_PAYLOAD_ENV_VAR) { - Ok(v) if !v.is_empty() => v, + Self::from_raw_env_value(std::env::var(SETUP_PAYLOAD_ENV_VAR).ok()) + } + + /// Parse an optional raw env-var value into a `SetupPayload`. + /// + /// `None` or empty string → `Ok(None)` (normal mode, no setup payload). + /// Non-empty, valid JSON → `Ok(Some(payload))`. + /// Non-empty, malformed JSON → `Err`. + /// + /// This is the pure core of `from_env()` and is the preferred target for + /// unit tests — it requires no global env mutation and is safe to call + /// concurrently. + pub(crate) fn from_raw_env_value(raw: Option) -> Result> { + let raw = match raw { + Some(v) if !v.is_empty() => v, _ => return Ok(None), }; let payload = serde_json::from_str::(&raw) @@ -435,7 +448,7 @@ async fn handle_setup_membership( if kind_u32 == KIND_MEMBER_ADDED_NOTIFICATION { // Subscribe to the newly-joined channel. - let mut ids = vec![channel_id]; + let ids = vec![channel_id]; let filters = crate::config::resolve_channel_filters(config, &ids, rules); for (cid, filter) in filters { if let Err(e) = relay.subscribe_channel(cid, filter).await { @@ -515,32 +528,23 @@ mod tests { use super::*; #[test] - fn setup_payload_from_env_returns_none_when_unset() { - // The env var is not set in test builds — should return None. - // We rely on the var not being set in the test environment. - // If it IS set (e.g., running under buzz-acp itself), skip. - if std::env::var(SETUP_PAYLOAD_ENV_VAR).is_ok() { - return; - } - let result = SetupPayload::from_env().unwrap(); + fn setup_payload_from_raw_returns_none_when_absent() { + // None → Ok(None): normal startup, no setup payload. + let result = SetupPayload::from_raw_env_value(None).unwrap(); assert!(result.is_none()); } #[test] - fn setup_payload_from_env_returns_err_on_malformed_json() { - // Temporarily set the env var to malformed JSON. - // Note: env var mutation in tests is not safe under parallel test - // runners, but `cargo test` runs tests in the same process so this - // is deterministic as long as the var name is unique. - // We only set it momentarily and restore immediately. - let original = std::env::var(SETUP_PAYLOAD_ENV_VAR).ok(); - std::env::set_var(SETUP_PAYLOAD_ENV_VAR, "not-valid-json{{{"); - let result = SetupPayload::from_env(); - // Restore first. - match &original { - Some(v) => std::env::set_var(SETUP_PAYLOAD_ENV_VAR, v), - None => std::env::remove_var(SETUP_PAYLOAD_ENV_VAR), - } + fn setup_payload_from_raw_returns_none_on_empty_string() { + // Empty string → Ok(None): treated the same as absent. + let result = SetupPayload::from_raw_env_value(Some(String::new())).unwrap(); + assert!(result.is_none()); + } + + #[test] + fn setup_payload_from_raw_returns_err_on_malformed_json() { + // Malformed JSON → Err: no global env mutation, safe to run concurrently. + let result = SetupPayload::from_raw_env_value(Some("not-valid-json{{{".into())); assert!(result.is_err(), "malformed JSON must return Err"); } diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index ae6fcb177..d4bfac6ec 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -1811,9 +1811,16 @@ pub fn spawn_agent_child( None }; - // Always remove the key after user env has been written (above) so - // that a saved agent env var or an ambient parent-process value - // cannot forge or suppress setup mode. + // Strip the key from the process-spawned command on every path. + // Two independent guards protect the invariant: + // 1. BUZZ_ACP_SETUP_PAYLOAD is in RESERVED_ENV_KEYS, so + // merged_user_env() can never write it via saved/persona env. + // 2. This env_remove() clears any ambient parent-process value + // inherited by std::process::Command before we conditionally + // set the desktop-computed trusted value below. + // Note: merged_user_env() is written further below in this function; + // ordering relative to that call is NOT what makes this safe — the + // reserved-key strip (guard 1) handles user env regardless of order. command.env_remove("BUZZ_ACP_SETUP_PAYLOAD"); // Set the payload only when desktop computed NotReady. From 6fd78acf0f94f01444197485ff62f3c31548feb4 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 30 Jun 2026 19:05:54 -0400 Subject: [PATCH 08/30] chore: apply rust fmt and biome format fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run cargo fmt and biome check --write to satisfy CI formatter gates. No logic changes — formatting only. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-acp/src/lib.rs | 4 +- crates/buzz-acp/src/setup_mode.rs | 48 ++++++++----------- .../features/agents/ui/EditAgentDialog.tsx | 2 +- .../src/features/agents/ui/EnvVarsEditor.tsx | 5 +- .../agents/ui/personaDialogPickers.tsx | 5 +- 5 files changed, 26 insertions(+), 38 deletions(-) diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index 9a7f99675..fd91d9efe 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -1101,9 +1101,7 @@ async fn tokio_main() -> Result<()> { if let Some(payload) = setup_mode::SetupPayload::from_env() .map_err(|e| anyhow::anyhow!("setup payload error: {e}"))? { - tracing::info!( - "buzz-acp: setup payload present, entering setup-listener mode" - ); + tracing::info!("buzz-acp: setup payload present, entering setup-listener mode"); return setup_mode::run_setup_listener(config, payload).await; } diff --git a/crates/buzz-acp/src/setup_mode.rs b/crates/buzz-acp/src/setup_mode.rs index 015544eb3..91296feb5 100644 --- a/crates/buzz-acp/src/setup_mode.rs +++ b/crates/buzz-acp/src/setup_mode.rs @@ -48,8 +48,7 @@ use uuid::Uuid; use crate::{ author_allowed, config::Config, - event_mentions_agent, - filter, + event_mentions_agent, filter, relay::{HarnessRelay, RelayEventPublisher}, }; @@ -220,11 +219,12 @@ pub(crate) async fn run_setup_listener(config: Config, payload: SetupPayload) -> // every message in a channel). let rules = build_setup_subscription_rules(&config); - let channel_filters = - crate::config::resolve_channel_filters(&config, &channel_ids, &rules); + let channel_filters = crate::config::resolve_channel_filters(&config, &channel_ids, &rules); if channel_filters.is_empty() { - tracing::warn!("setup-mode: no channel subscriptions resolved — nudge listener will sit idle"); + tracing::warn!( + "setup-mode: no channel subscriptions resolved — nudge listener will sit idle" + ); } for (channel_id, filter) in &channel_filters { @@ -393,27 +393,23 @@ pub(crate) fn should_nudge_for_event( fn build_setup_subscription_rules(config: &Config) -> Vec { use crate::config::SubscribeMode; - let kinds = config.kinds_override.clone().unwrap_or_else(|| { - vec![ - KIND_STREAM_MESSAGE, - KIND_WORKFLOW_APPROVAL_REQUESTED, - ] - }); + let kinds = config + .kinds_override + .clone() + .unwrap_or_else(|| vec![KIND_STREAM_MESSAGE, KIND_WORKFLOW_APPROVAL_REQUESTED]); match &config.subscribe_mode { // Config mode: load the actual rules, but they will be filtered by // the explicit event_mentions_agent check in the main loop anyway. - SubscribeMode::Config => { - match crate::config::load_rules(&config.config_path) { - Ok(rules) => rules, - Err(e) => { - tracing::warn!( - "setup-mode: could not load config rules ({e}); falling back to mentions" - ); - vec![mentions_rule(kinds)] - } + SubscribeMode::Config => match crate::config::load_rules(&config.config_path) { + Ok(rules) => rules, + Err(e) => { + tracing::warn!( + "setup-mode: could not load config rules ({e}); falling back to mentions" + ); + vec![mentions_rule(kinds)] } - } + }, _ => vec![mentions_rule(kinds)], } } @@ -584,10 +580,7 @@ mod tests { body.contains("ANTHROPIC_API_KEY"), "nudge body should mention the missing env key" ); - assert!( - body.contains("Fizz"), - "nudge body should name the agent" - ); + assert!(body.contains("Fizz"), "nudge body should name the agent"); } #[test] @@ -654,10 +647,7 @@ mod tests { Duration::from_secs(30), ); - assert!( - !result, - "non-allowlisted author must not produce a nudge" - ); + assert!(!result, "non-allowlisted author must not produce a nudge"); // Dedup set must remain empty — no phantom insertion for blocked author. assert!( dedup.is_empty(), diff --git a/desktop/src/features/agents/ui/EditAgentDialog.tsx b/desktop/src/features/agents/ui/EditAgentDialog.tsx index 29821a165..d58577236 100644 --- a/desktop/src/features/agents/ui/EditAgentDialog.tsx +++ b/desktop/src/features/agents/ui/EditAgentDialog.tsx @@ -66,7 +66,7 @@ function RequiredFieldLabel({