From 3a585fd8eddb9e3307cd9433b877f10af674ff0d Mon Sep 17 00:00:00 2001 From: ss-dev-00 Date: Fri, 21 Aug 2026 17:46:09 -0700 Subject: [PATCH] broker: back the agents.* capabilities with the real Desktop commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements `AgentService` and `AgentRoster` against the same commands the UI calls — `create_managed_agent`, `update_managed_agent`, `delete_managed_agent` — rather than reimplementing the mutations. Two agent creation routines would drift, and the broker's would be the one nobody looks at. The credential boundary is narrowed here: `create_managed_agent` returns the minted `private_key_nsec`, and this is the one stack frame that sees it. The outcome type has no field that could carry it, so the secret cannot reach the pipeline, the execution log, or the wire. Request translation is pure and tested directly, because the policy it encodes is worth pinning: - A brokered create neither spawns a process nor sets `start_on_app_launch`. A remote requester asking for an agent to exist has not asked for a local process to run on a schedule nobody at the keyboard chose. - It links no definition or team — that would pin someone else's config snapshot onto the new agent, a decision the request never made. - An update sends `None` for every field the request did not name. The broker has no "clear to default" verb, so it never emits the explicit null that would wipe stored config. - A requested runtime is resolved through the catalog before it can become a stored harness pin, and an unknown id fails here instead of surfacing later as an unrelated spawn error. - A delete leaves `force_remote_delete` unset, so the deployed-remote guard still holds: orphaning provisioned infrastructure stays a human decision. Channel attachment is its own failable step after the agent exists, so it maps to `Unknown` rather than `Failed` — telling a requester nothing happened while an agent sits in the roster would be the worse lie. Name targets resolve case-insensitively but must be unambiguous. Two agents can legitimately share a name, and guessing would mutate or delete an agent the requester never identified. The roster refuses to answer for an owner this host does not hold keys for, rather than authorizing against the wrong agent set. Co-authored-by: Bradley Axen Signed-off-by: Bradley Axen --- .../src-tauri/src/broker/desktop_agents.rs | 390 ++++++++++++++++++ .../src/broker/desktop_agents/tests.rs | 176 ++++++++ desktop/src-tauri/src/broker/mod.rs | 1 + 3 files changed, 567 insertions(+) create mode 100644 desktop/src-tauri/src/broker/desktop_agents.rs create mode 100644 desktop/src-tauri/src/broker/desktop_agents/tests.rs diff --git a/desktop/src-tauri/src/broker/desktop_agents.rs b/desktop/src-tauri/src/broker/desktop_agents.rs new file mode 100644 index 0000000000..e0169d2622 --- /dev/null +++ b/desktop/src-tauri/src/broker/desktop_agents.rs @@ -0,0 +1,390 @@ +//! The Desktop-backed [`AgentService`] and [`AgentRoster`]. +//! +//! This is the only place in the broker that knows about Tauri. It translates a +//! validated capability request into the same commands the UI calls — +//! `create_managed_agent`, `update_managed_agent`, `delete_managed_agent` — so a +//! brokered mutation and a hand-driven one take one code path. Reimplementing +//! the mutation here would mean two agent-creation routines drifting apart, and +//! the broker's would be the one nobody looks at. +//! +//! # Where the credential boundary sits +//! +//! Nothing here returns secret material. `create_managed_agent` hands back a +//! `CreateManagedAgentResponse` that *does* contain the minted +//! `private_key_nsec`; [`DesktopAgentService::create`] reads only the summary +//! and drops the response, and [`AgentsCreateOutcome`] has no field that could +//! hold it. That is the narrowing point: the secret exists on one stack frame +//! inside this file and never reaches the pipeline, the store, or the wire. +//! +//! # Why the request translation is a separate pure function +//! +//! [`create_request`] and [`update_request`] are pure and tested directly. +//! Everything they decide — which optional fields become `None`, that a +//! brokered create never spawns a process — is policy worth pinning in a test, +//! and none of it needs an `AppHandle`. + +use buzz_sdk_pkg::broker::{ + AgentTarget, AgentsCreateArgs, AgentsCreateOutcome, AgentsDeleteArgs, AgentsDeleteOutcome, + AgentsUpdateArgs, AgentsUpdateOutcome, +}; +use tauri::{AppHandle, Manager}; + +use crate::app_state::AppState; +use crate::managed_agents::{ + load_managed_agents, CreateManagedAgentRequest, ManagedAgentRecord, RespondTo, + UpdateManagedAgentRequest, DEFAULT_ACP_COMMAND, +}; + +use super::agents_policy::AgentRoster; +use super::handlers::{AgentService, ServiceError}; +use super::pipeline::BoxFuture; + +/// Build the create request the Desktop command expects. +/// +/// A brokered create is deliberately *not* spawned (`spawn_after_create: +/// false`) and does not auto-start (`start_on_app_launch: false`). A remote +/// requester asking for an agent to exist has not asked for a local process to +/// be running, and starting one would consume host resources on a schedule +/// nobody at the keyboard chose. The owner starts it from the UI. +/// +/// `persona_id` and `team_id` stay `None`: linking a definition pins a snapshot +/// of someone else's config onto the new agent, which is a second decision the +/// request never made. +fn create_request(args: &AgentsCreateArgs) -> Result { + let respond_to = args + .respond_to + .as_deref() + .map(RespondTo::parse_wire) + .transpose()?; + + Ok(CreateManagedAgentRequest { + name: args.display_name.clone(), + persona_id: None, + team_id: None, + relay_url: None, + acp_command: Some(DEFAULT_ACP_COMMAND.to_string()), + // `agent_command` is the harness pin. A requested runtime is resolved to + // its command here rather than passed through raw, so an arbitrary + // string can never become an executed command. + agent_command: args + .runtime + .as_deref() + .map(resolve_runtime_command) + .transpose()?, + harness_override: args.runtime.is_some(), + agent_args: Vec::new(), + mcp_command: None, + turn_timeout_seconds: None, + idle_timeout_seconds: None, + max_turn_duration_seconds: None, + parallelism: None, + system_prompt: Some(args.system_prompt.clone()), + avatar_url: None, + model: args.model.clone(), + provider: args.provider.clone(), + env_vars: std::collections::BTreeMap::new(), + spawn_after_create: false, + start_on_app_launch: false, + backend: crate::managed_agents::BackendKind::Local, + respond_to, + respond_to_allowlist: Vec::new(), + relay_mesh: None, + }) +} + +/// Resolve a requested runtime id to its harness command. +/// +/// Refuses an id the catalog does not know instead of storing it: an +/// unresolvable pin produces an agent that cannot start, and the failure would +/// surface much later as a spawn error with no connection to this request. +fn resolve_runtime_command(runtime: &str) -> Result { + crate::managed_agents::command_for_runtime_id(runtime) + .ok_or_else(|| format!("unknown runtime \"{runtime}\"")) +} + +/// Build the update request, and report which fields it will change. +/// +/// The field list is derived from the request rather than from a diff of the +/// record, because the requester asked for these fields specifically. The +/// outcome names exactly what was asked for and applied. +fn update_request( + pubkey: &str, + args: &AgentsUpdateArgs, +) -> Result<(UpdateManagedAgentRequest, Vec), String> { + let mut changed = Vec::new(); + if args.display_name.is_some() { + changed.push("displayName".to_string()); + } + if args.system_prompt.is_some() { + changed.push("systemPrompt".to_string()); + } + if args.runtime.is_some() { + changed.push("runtime".to_string()); + } + if args.provider.is_some() { + changed.push("provider".to_string()); + } + if args.model.is_some() { + changed.push("model".to_string()); + } + if args.respond_to.is_some() { + changed.push("respondTo".to_string()); + } + changed.sort(); + + let respond_to = args + .respond_to + .as_deref() + .map(RespondTo::parse_wire) + .transpose()?; + + let request = UpdateManagedAgentRequest { + pubkey: pubkey.to_string(), + name: args.display_name.clone(), + // `Option>`: absent leaves the stored value alone. The broker + // has no "clear to default" verb, so it never sends `Some(None)` — a + // requester cannot blank a field it did not name. + model: args.model.clone().map(Some), + system_prompt: args.system_prompt.clone().map(Some), + env_vars: None, + parallelism: None, + turn_timeout_seconds: None, + relay_url: None, + acp_command: None, + agent_command: args + .runtime + .as_deref() + .map(resolve_runtime_command) + .transpose()?, + harness_override: args.runtime.is_some(), + agent_args: None, + mcp_command: None, + provider: args.provider.clone().map(Some), + respond_to, + // Allowlist mode is not reachable through this capability (the SDK + // rejects the mode), so there is never a list to replace. + respond_to_allowlist: None, + }; + Ok((request, changed)) +} + +/// Add `agent_pubkey` to `channel_id` as a bot member. +/// +/// The same owner-signed kind-9000 event `add_channel_members` publishes. It is +/// spelled out here rather than routed through that command because the command +/// swallows per-pubkey failures into a result array, and a broker create must +/// treat a failed attachment as a reportable outcome, not a field in a summary. +async fn attach_to_channel( + app: &AppHandle, + channel_id: &str, + agent_pubkey: &str, +) -> Result<(), String> { + let uuid = uuid::Uuid::parse_str(channel_id) + .map_err(|_| format!("invalid channel UUID: {channel_id}"))?; + let builder = crate::events::build_add_member(uuid, agent_pubkey, Some("bot"))?; + let state = app.state::(); + crate::relay::submit_event(builder, &state).await?; + Ok(()) +} + +/// Resolve a target selector against the owner's roster. +/// +/// Name matching is case-insensitive and must be unambiguous: two agents +/// sharing a name is a real state, and picking one arbitrarily would delete or +/// rewrite the wrong agent. +fn resolve_target( + records: &[ManagedAgentRecord], + target: &AgentTarget, +) -> Result { + match target { + AgentTarget::Pubkey(pubkey) => records + .iter() + .find(|record| record.pubkey.eq_ignore_ascii_case(pubkey)) + .cloned() + .ok_or_else(|| format!("no managed agent with pubkey {pubkey}")), + AgentTarget::Name(name) => { + let mut matches = records + .iter() + .filter(|record| record.name.eq_ignore_ascii_case(name)); + let first = matches + .next() + .cloned() + .ok_or_else(|| format!("no managed agent named \"{name}\""))?; + if matches.next().is_some() { + return Err(format!( + "more than one managed agent is named \"{name}\"; target it by pubkey" + )); + } + Ok(first) + } + } +} + +/// [`AgentService`] over the Desktop agent commands. +pub struct DesktopAgentService { + app: AppHandle, +} + +impl DesktopAgentService { + /// Bind the service to a running app. + pub fn new(app: AppHandle) -> Self { + Self { app } + } + + /// Snapshot the owner's agent records. + fn records(&self) -> Result, String> { + let state = self.app.state::(); + let _guard = state + .managed_agents_store_lock + .lock() + .map_err(|error| error.to_string())?; + load_managed_agents(&self.app) + } +} + +impl AgentService for DesktopAgentService { + fn create<'a>( + &'a self, + _owner_pubkey: &'a str, + args: &'a AgentsCreateArgs, + ) -> BoxFuture<'a, Result> { + Box::pin(async move { + let request = create_request(args).map_err(ServiceError::Failed)?; + let response = crate::commands::create_managed_agent( + request, + self.app.clone(), + self.app.state::(), + ) + .await + .map_err(ServiceError::Failed)?; + + let agent_pubkey = response.agent.pubkey.clone(); + let display_name = response.agent.name.clone(); + // The minted nsec dies with `response` at the end of this scope. + drop(response); + + // Attachment is a second, separately failable step. The agent + // already exists at this point, so a failure here is + // `Unknown`, not `Failed`: reporting "failed" would tell the + // requester nothing happened while an agent sits in the roster. + attach_to_channel(&self.app, &args.channel_id, &agent_pubkey) + .await + .map_err(|error| { + ServiceError::Unknown(format!( + "agent {agent_pubkey} was created but could not be added to channel \ + {}: {error}", + args.channel_id + )) + })?; + + Ok(AgentsCreateOutcome { + agent_pubkey, + display_name, + channel_id: args.channel_id.clone(), + }) + }) + } + + fn update<'a>( + &'a self, + _owner_pubkey: &'a str, + args: &'a AgentsUpdateArgs, + ) -> BoxFuture<'a, Result> { + Box::pin(async move { + let records = self.records().map_err(ServiceError::Failed)?; + let record = resolve_target(&records, &args.target).map_err(ServiceError::Failed)?; + let (request, updated_fields) = + update_request(&record.pubkey, args).map_err(ServiceError::Failed)?; + + let response = crate::commands::update_managed_agent( + request, + self.app.clone(), + self.app.state::(), + ) + .await + .map_err(ServiceError::Failed)?; + + Ok(AgentsUpdateOutcome { + agent_pubkey: response.agent.pubkey, + display_name: response.agent.name, + updated_fields, + }) + }) + } + + fn delete<'a>( + &'a self, + _owner_pubkey: &'a str, + args: &'a AgentsDeleteArgs, + ) -> BoxFuture<'a, Result> { + Box::pin(async move { + let records = self.records().map_err(ServiceError::Failed)?; + let record = resolve_target(&records, &args.target).map_err(ServiceError::Failed)?; + let agent_pubkey = record.pubkey.clone(); + let display_name = record.name.clone(); + + // `force_remote_delete: None` keeps the deployed-remote guard in + // force. Orphaning provisioned remote infrastructure is a decision + // for a human looking at the warning, not something a brokered + // request may force. + crate::commands::delete_managed_agent(agent_pubkey.clone(), None, self.app.clone()) + .await + .map_err(ServiceError::Failed)?; + + Ok(AgentsDeleteOutcome { + agent_pubkey, + display_name, + }) + }) + } +} + +/// [`AgentRoster`] reading the authoritative Desktop agent store. +pub struct DesktopAgentRoster { + app: AppHandle, +} + +impl DesktopAgentRoster { + /// Bind the roster to a running app. + pub fn new(app: AppHandle) -> Self { + Self { app } + } +} + +impl AgentRoster for DesktopAgentRoster { + fn agent_pubkeys<'a>( + &'a self, + owner_pubkey: &'a str, + ) -> BoxFuture<'a, Result, String>> { + Box::pin(async move { + // The store holds exactly one owner's agents — the owner whose keys + // this app instance holds. A request naming a different owner + // cannot be answered from here, and answering it from this roster + // anyway would authorize against the wrong set. + let state = self.app.state::(); + let host_owner = state + .keys + .lock() + .map_err(|error| error.to_string())? + .public_key() + .to_hex(); + if !host_owner.eq_ignore_ascii_case(owner_pubkey) { + return Err(format!( + "this host holds credentials for {host_owner}, not {owner_pubkey}" + )); + } + + let _guard = state + .managed_agents_store_lock + .lock() + .map_err(|error| error.to_string())?; + Ok(load_managed_agents(&self.app)? + .into_iter() + .map(|record| record.pubkey) + .collect()) + }) + } +} + +#[cfg(test)] +mod tests; diff --git a/desktop/src-tauri/src/broker/desktop_agents/tests.rs b/desktop/src-tauri/src/broker/desktop_agents/tests.rs new file mode 100644 index 0000000000..a826e2f9cc --- /dev/null +++ b/desktop/src-tauri/src/broker/desktop_agents/tests.rs @@ -0,0 +1,176 @@ +//! Tests for request translation and target resolution. +//! +//! These cover the decisions this module makes on its own — which fields a +//! brokered mutation may touch, and which agent a selector resolves to. The +//! Tauri-calling parts are not covered here: they hold no logic beyond +//! forwarding and the `Failed`/`Unknown` split, and testing them would mean +//! booting an app to observe a call it already delegates. + +use buzz_sdk_pkg::broker::AgentTarget; + +use super::*; + +const CHANNEL: &str = "b2c38ca8-9ec3-411e-bab5-f9deab34d52e"; + +fn create_args() -> AgentsCreateArgs { + AgentsCreateArgs { + channel_id: CHANNEL.into(), + display_name: "Helper".into(), + system_prompt: "Help with things.".into(), + runtime: None, + provider: None, + model: None, + respond_to: None, + } +} + +fn update_args() -> AgentsUpdateArgs { + AgentsUpdateArgs { + target: AgentTarget::Name("helper".into()), + display_name: None, + system_prompt: None, + runtime: None, + provider: None, + model: None, + respond_to: None, + } +} + +/// Records are built from JSON, matching `managed_agents::reconcile::tests` — +/// `ManagedAgentRecord` has no `Default`, and enumerating its fifty-odd fields +/// here would break on every unrelated schema addition. +fn record(pubkey: &str, name: &str) -> ManagedAgentRecord { + serde_json::from_str(&format!( + r#"{{ + "pubkey": "{pubkey}", + "name": "{name}", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": "You are a test agent.", + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z" + }}"# + )) + .unwrap() +} + +/// A remote request for an agent to exist is not a request to run a process on +/// the owner's machine, now or at every launch. +#[test] +fn a_brokered_create_neither_spawns_nor_auto_starts() { + let request = create_request(&create_args()).unwrap(); + assert!(!request.spawn_after_create); + assert!(!request.start_on_app_launch); +} + +/// Linking a definition would pin someone else's config snapshot onto the new +/// agent — a decision the request never expressed. +#[test] +fn a_brokered_create_links_no_definition_or_team() { + let request = create_request(&create_args()).unwrap(); + assert!(request.persona_id.is_none()); + assert!(request.team_id.is_none()); + assert!(request.env_vars.is_empty()); + assert_eq!(request.backend, crate::managed_agents::BackendKind::Local); +} + +#[test] +fn create_carries_the_requested_name_and_prompt() { + let request = create_request(&create_args()).unwrap(); + assert_eq!(request.name, "Helper"); + assert_eq!(request.system_prompt.as_deref(), Some("Help with things.")); +} + +#[test] +fn create_translates_a_respond_to_mode() { + let mut args = create_args(); + args.respond_to = Some("anyone".into()); + let request = create_request(&args).unwrap(); + assert_eq!(request.respond_to, Some(RespondTo::Anyone)); +} + +/// An unresolvable runtime must fail here, not later as a spawn error with no +/// connection back to the request that caused it. +#[test] +fn create_refuses_a_runtime_the_catalog_does_not_know() { + let mut args = create_args(); + args.runtime = Some("not-a-real-runtime".into()); + let error = create_request(&args).unwrap_err(); + assert!(error.contains("unknown runtime"), "unexpected: {error}"); +} + +/// The broker has no "clear this field" verb, so an unnamed field must stay +/// absent rather than becoming an explicit null that wipes stored config. +#[test] +fn update_leaves_unrequested_fields_untouched() { + let (request, changed) = update_request("abc", &update_args()).unwrap(); + assert!(changed.is_empty()); + assert!(request.name.is_none()); + assert!(request.model.is_none()); + assert!(request.system_prompt.is_none()); + assert!(request.provider.is_none()); + assert!(request.respond_to.is_none()); + // Fields the capability does not expose at all. + assert!(request.env_vars.is_none()); + assert!(request.relay_url.is_none()); + assert!(request.agent_args.is_none()); + assert!(request.respond_to_allowlist.is_none()); +} + +#[test] +fn update_sets_only_the_named_fields_and_reports_them_sorted() { + let mut args = update_args(); + args.model = Some("some-model".into()); + args.display_name = Some("Renamed".into()); + let (request, changed) = update_request("abc", &args).unwrap(); + + assert_eq!(changed, vec!["displayName", "model"]); + assert_eq!(request.name.as_deref(), Some("Renamed")); + assert_eq!(request.model, Some(Some("some-model".to_string()))); + assert!(request.system_prompt.is_none()); +} + +#[test] +fn update_resolves_a_runtime_to_a_command_and_marks_it_a_pin() { + let mut args = update_args(); + args.runtime = Some("buzz-agent".into()); + let (request, changed) = update_request("abc", &args).unwrap(); + assert_eq!(changed, vec!["runtime"]); + assert!(request.agent_command.is_some()); + assert!(request.harness_override); +} + +#[test] +fn a_pubkey_target_resolves_case_insensitively() { + let records = vec![record("aabb", "helper")]; + let resolved = resolve_target(&records, &AgentTarget::Pubkey("AABB".into())).unwrap(); + assert_eq!(resolved.pubkey, "aabb"); +} + +#[test] +fn a_name_target_resolves_case_insensitively() { + let records = vec![record("aabb", "Helper")]; + let resolved = resolve_target(&records, &AgentTarget::Name("helper".into())).unwrap(); + assert_eq!(resolved.pubkey, "aabb"); +} + +/// Two agents can legitimately share a name. Picking one would mutate or delete +/// an agent the requester did not identify. +#[test] +fn an_ambiguous_name_is_refused_rather_than_guessed() { + let records = vec![record("aabb", "helper"), record("ccdd", "Helper")]; + let error = resolve_target(&records, &AgentTarget::Name("helper".into())).unwrap_err(); + assert!(error.contains("more than one"), "unexpected: {error}"); + assert!(error.contains("by pubkey"), "unexpected: {error}"); +} + +#[test] +fn a_missing_target_is_an_error() { + let records = vec![record("aabb", "helper")]; + assert!(resolve_target(&records, &AgentTarget::Name("ghost".into())).is_err()); + assert!(resolve_target(&records, &AgentTarget::Pubkey("ffff".into())).is_err()); +} diff --git a/desktop/src-tauri/src/broker/mod.rs b/desktop/src-tauri/src/broker/mod.rs index af99b3659b..b5748d9d81 100644 --- a/desktop/src-tauri/src/broker/mod.rs +++ b/desktop/src-tauri/src/broker/mod.rs @@ -33,6 +33,7 @@ use sha2::{Digest, Sha256}; pub mod agents_policy; +pub mod desktop_agents; pub mod handlers; pub mod pipeline; pub mod store;