From fb49f06c80a28205ea2ba3e9436240e7f2032dec Mon Sep 17 00:00:00 2001 From: wiggdevin <202901685+wiggdevin@users.noreply.github.com> Date: Sat, 5 Sep 2026 20:04:45 -0700 Subject: [PATCH 1/9] feat(agents): make the MCP registry live in a shipped build (T7c backend) `converge` had no production caller and `GenerationStore::reconcile` never ran, so `plan_for_spawn` found no adopted generation and the registry was inert in a shipped build. This adds the missing half. - `mcp_registry::apply` is the one place a registry edit or a per-agent toggle becomes an adopted generation. It asserts at the wiring seam that the launcher path is absolute, exists and is a regular file (PR 23 follow-up 2), and it passes every managed agent, because a convergence is whole-set. - `lib.rs` runs `reconcile_at_start` before any agent is restored, so a crash mid-change no longer strands a revoked credential in the keychain. - Five Tauri commands behind the panel and the toggles, each one atomic: the document is rewritten whole and the configuration agents spawn from moves in one pointer rename. - `ManagedAgentRecord.mcp_servers` is the versioned enabled-server list, absent distinct from empty (memo decision 8), and deliberately outside the kind:30177 projection. - Secret values travel one way: `GenerationInputs.pending` carries what the operator typed into the adopted generation's keyspace under the reserved `mcp:` prefix. Nothing reads one back. Sol carry-overs from the T7b audit: N8 (`GenerationPlan` and `Deletion` now self-validate, so a journal on disk cannot name `identity` or `agent:*`), N9 (a staged artefact's type is read from `symlink_metadata` and anything but a regular file is refused, so a planted FIFO cannot block a spawn forever), N10 (`journal.json.next` and `current.next` are created `O_NOFOLLOW`), W4 (the desktop takes the stricter of the two sides' name, argument and env bounds, and the generated argv count is bounded by what `buzz-acp` reads). Also: PR 23 follow-up 3 (`write_atomically` fsyncs the temp file and the parent directory), follow-up 4 (the confinement test asserts the cause), follow-up 6 and 8 (the memo records the shipped launcher behaviour and the `args` shape decision). A staged generation directory is now created even when a plan stages no files, which is what turning every server off produces. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_012Z6iidtozXxgx58BUZUKnu Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> --- crates/buzz-acp/src/mcp_registry.rs | 28 +- .../src/commands/agent_config_tests.rs | 1 + desktop/src-tauri/src/commands/agents.rs | 1 + .../src-tauri/src/commands/agents_tests.rs | 1 + .../src-tauri/src/commands/mcp_registry.rs | 303 +++++ desktop/src-tauri/src/commands/mod.rs | 2 + .../commands/personas/delete_cascade_tests.rs | 1 + .../personas/inbound/inbound_tests.rs | 1 + .../commands/personas/prompt_source/tests.rs | 1 + .../personas/snapshot/fidelity_tests.rs | 1 + .../src/commands/personas/snapshot/import.rs | 1 + .../src/commands/personas/snapshot/tests.rs | 1 + .../personas/update/name_propagation_tests.rs | 1 + .../src-tauri/src/commands/team_snapshot.rs | 1 + .../src/commands/team_snapshot/tests.rs | 1 + desktop/src-tauri/src/lib.rs | 23 + .../src/managed_agents/agent_events.rs | 1 + .../managed_agents/agent_snapshot_envelope.rs | 1 + .../managed_agents/agent_snapshot_tests.rs | 1 + .../config_bridge/effort_tests.rs | 1 + .../config_bridge/reader_tests.rs | 1 + .../src/managed_agents/discovery/tests.rs | 1 + .../managed_agents/effective_config/tests.rs | 1 + .../src/managed_agents/global_config/tests.rs | 1 + .../src/managed_agents/mcp_registry/apply.rs | 304 +++++ .../mcp_registry/apply_tests.rs | 1029 +++++++++++++++++ .../managed_agents/mcp_registry/converge.rs | 51 +- .../managed_agents/mcp_registry/generate.rs | 30 + .../managed_agents/mcp_registry/generation.rs | 198 +++- .../src/managed_agents/mcp_registry/load.rs | 101 +- .../src/managed_agents/mcp_registry/mod.rs | 6 + .../src/managed_agents/mcp_registry/schema.rs | 47 +- .../src/managed_agents/mcp_registry/spawn.rs | 107 +- .../mcp_registry/wiring_tests.rs | 165 ++- desktop/src-tauri/src/managed_agents/mod.rs | 4 +- .../src/managed_agents/nest/render_tests.rs | 1 + .../src/managed_agents/parallelism.rs | 1 + .../managed_agents/persona_events/tests.rs | 1 + .../src-tauri/src/managed_agents/readiness.rs | 1 + .../src-tauri/src/managed_agents/runtime.rs | 2 +- .../managed_agents/runtime/test_fixtures.rs | 1 + .../managed_agents/spawn_snapshot/tests.rs | 1 + .../src/managed_agents/team_snapshot.rs | 1 + .../src/managed_agents/teams_tests.rs | 1 + desktop/src-tauri/src/managed_agents/types.rs | 32 + docs/plans/2026-09-04-mcp-registry-design.md | 8 + 46 files changed, 2369 insertions(+), 99 deletions(-) create mode 100644 desktop/src-tauri/src/commands/mcp_registry.rs create mode 100644 desktop/src-tauri/src/managed_agents/mcp_registry/apply.rs create mode 100644 desktop/src-tauri/src/managed_agents/mcp_registry/apply_tests.rs diff --git a/crates/buzz-acp/src/mcp_registry.rs b/crates/buzz-acp/src/mcp_registry.rs index 74225f3d617..2230cd266d0 100644 --- a/crates/buzz-acp/src/mcp_registry.rs +++ b/crates/buzz-acp/src/mcp_registry.rs @@ -892,18 +892,26 @@ mod tests { let good = staged_path(base.path(), "agent-a", 7); confine_registry_path(&good, &capability).expect("the staged path is accepted"); - for (path, why) in [ + // Each case names the cause it must be refused for, not merely that it + // was refused (PR 23 follow-up 4). `..` in particular is refused by the + // shape check rather than by the tail comparison, and asserting only + // `is_err()` would keep passing if the shape check were deleted and the + // tail happened to disagree for an unrelated reason. + for (path, why, cause) in [ ( staged_path(base.path(), "agent-b", 7), "another agent's directory", + "outside this agent's directory", ), ( staged_path(base.path(), "agent-a", 6), "a superseded generation", + "outside this agent's directory", ), ( base.path().join("elsewhere").join(REGISTRY_FILE_NAME), "a path outside the staging tree", + "outside this agent's directory", ), ( base.path() @@ -915,19 +923,21 @@ mod tests { .join("agent-b") .join(REGISTRY_FILE_NAME), "a `..` traversal", + "holds a `.` or `..` component", + ), + ( + PathBuf::from("relative/registry.json"), + "a relative path", + "is not an absolute path", ), ] { + let error = confine_registry_path(&path, &capability) + .expect_err(&format!("{why} was accepted: {}", path.display())); assert!( - confine_registry_path(&path, &capability).is_err(), - "{why} was accepted: {}", - path.display() + error.contains(cause), + "{why} was refused for the wrong reason; expected {cause:?}, got {error}" ); } - - assert!( - confine_registry_path(Path::new("relative/registry.json"), &capability).is_err(), - "a relative path was accepted" - ); } /// The open refuses a symlink and anything that is not a regular file. diff --git a/desktop/src-tauri/src/commands/agent_config_tests.rs b/desktop/src-tauri/src/commands/agent_config_tests.rs index 698f35d91a0..cf1c92cb219 100644 --- a/desktop/src-tauri/src/commands/agent_config_tests.rs +++ b/desktop/src-tauri/src/commands/agent_config_tests.rs @@ -74,6 +74,7 @@ fn goose_runtime() -> &'static KnownAcpRuntime { fn agent_record() -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: "agent".to_string(), name: "Agent".to_string(), diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index 0ad7fd321c5..3752c74d0e3 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -661,6 +661,7 @@ pub async fn create_managed_agent( last_exit_code: None, last_error: None, last_error_code: None, + mcp_servers: None, respond_to: minted.respond_to, respond_to_allowlist: minted.respond_to_allowlist.clone(), display_name: None, diff --git a/desktop/src-tauri/src/commands/agents_tests.rs b/desktop/src-tauri/src/commands/agents_tests.rs index 59e04b09ff0..927d4d8bb09 100644 --- a/desktop/src-tauri/src/commands/agents_tests.rs +++ b/desktop/src-tauri/src/commands/agents_tests.rs @@ -9,6 +9,7 @@ fn bare_agent_record( use crate::managed_agents::{BackendKind, RespondTo}; use std::collections::BTreeMap; ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: "agent".to_string(), name: "Agent".to_string(), diff --git a/desktop/src-tauri/src/commands/mcp_registry.rs b/desktop/src-tauri/src/commands/mcp_registry.rs new file mode 100644 index 00000000000..01331fd9e4a --- /dev/null +++ b/desktop/src-tauri/src/commands/mcp_registry.rs @@ -0,0 +1,303 @@ +//! Tauri commands behind the MCP servers Settings panel and the per-agent +//! toggles. +//! +//! Every command here is a *user action*, and each one is one atomic persist +//! (AGENTS.md Review-Proven Rule 5): the registry document is rewritten whole, +//! and the configuration every agent will spawn from moves in one pointer +//! rename inside [`converge_now`]. A failure at any step leaves the previous +//! generation adopted, so agents keep running the configuration they were +//! started with rather than a half-applied one. +//! +//! Secret **values** travel one way. `save_mcp_registry_server` takes them, +//! hands them straight to the convergence, which writes them under the +//! reserved `mcp:` prefix bound to the generation it adopts. No command +//! returns one, and the DTOs below carry reference *names* only. + +use std::collections::BTreeMap; + +use tauri::AppHandle; + +use crate::managed_agents::mcp_registry::apply; +use crate::managed_agents::mcp_registry::apply::converge_now; +use crate::managed_agents::mcp_registry::load::{load_registry, LoadedEntry}; +use crate::managed_agents::mcp_registry::schema::{ + RegistryDocument, RegistryEntry, RegistryTransport, MAX_DOCUMENT_SERVERS, +}; +use crate::managed_agents::types::{AgentMcpServers, AGENT_MCP_SERVERS_VERSION}; +use buzz_secret_store_pkg::{looks_like_reference, McpSecretRef}; + +/// One registry entry as the panel renders it. +/// +/// The approve step needs the *exact* command line or URL the operator is +/// about to authorize, so it is projected verbatim. What is never projected is +/// a secret value: `env` is reduced to the variable name and the reference it +/// names, so a value cannot reach the renderer even if one were somehow +/// stored inline. +#[derive(Debug, Clone, serde::Serialize)] +pub struct McpRegistryEntryView { + /// Stable id; the agent record's enabled list refers to this. + pub id: String, + /// Display name, which is also the generated config key. + pub name: String, + /// `"stdio"` or `"http"`. + pub transport: String, + /// Absolute command path for a stdio entry, else `None`. + pub command: Option, + /// Command arguments for a stdio entry. + pub args: Vec, + /// Upstream URL for an http entry, else `None`. + pub url: Option, + /// Auth scheme for an http entry that declares one. + pub auth_scheme: Option, + /// Declared environment, as `(name, reference-or-literal)` pairs. A + /// reference is the `mcp:` spelling; a literal is one the sentinel + /// scan already cleared as non-credential. + pub env: Vec, + /// The loader's reason this entry is disabled, or `None` when it is + /// usable. Rendered beside the entry, and it is the same string a spawn + /// refuses with. + pub rejection: Option, +} + +/// One declared environment entry, names only. +#[derive(Debug, Clone, serde::Serialize)] +pub struct McpRegistryEnvView { + /// Variable name. + pub name: String, + /// The `mcp:` reference, when this entry names one. + pub reference: Option, + /// The literal value, when this entry carries one. Only values the + /// sentinel scan cleared as non-credential ever reach here; a + /// credential-shaped literal rejects the entry at load. + pub literal: Option, +} + +/// What the panel loads. +#[derive(Debug, Clone, serde::Serialize)] +pub struct McpRegistryView { + /// Every declared entry, in document order, each with its status. + pub servers: Vec, + /// Absolute path of the document, for the "reveal in Finder" affordance. + pub document_path: String, +} + +fn view_of(loaded: &LoadedEntry) -> McpRegistryEntryView { + let entry = &loaded.entry; + let (transport, command, args, url, auth_scheme) = match &entry.transport { + RegistryTransport::Stdio { command, args } => { + ("stdio", Some(command.clone()), args.clone(), None, None) + } + RegistryTransport::Http { url, auth } => ( + "http", + None, + Vec::new(), + Some(url.clone()), + auth.as_ref().map(|auth| auth.scheme.clone()), + ), + }; + McpRegistryEntryView { + id: entry.id.clone(), + name: entry.name.clone(), + transport: transport.to_string(), + command, + args, + url, + auth_scheme, + env: entry + .env + .iter() + .map(|(name, value)| { + let is_reference = looks_like_reference(value); + McpRegistryEnvView { + name: name.clone(), + reference: is_reference.then(|| value.clone()), + literal: (!is_reference).then(|| value.clone()), + } + }) + .collect(), + rejection: loaded.rejection.clone(), + } +} + +fn document_path(app: &AppHandle) -> Result { + let paths = apply::registry_paths(app)?.ok_or_else(|| { + "this build cannot resolve the agent working directory, so mcp server settings are \ + unavailable" + .to_string() + })?; + Ok(paths.document()) +} + +/// Read the registry document and its per-entry status. +/// +/// # Errors +/// A message when the document breaches a whole-document rule (a duplicate id +/// or name, or a byte cap). A per-entry failure is not an error: the entry is +/// returned with its `rejection` string, which is the same message a spawn +/// refuses with. +#[tauri::command] +pub fn list_mcp_registry_servers(app: AppHandle) -> Result { + let path = document_path(&app)?; + let registry = load_registry(&path).map_err(|e| e.to_string())?; + Ok(McpRegistryView { + servers: registry.entries.iter().map(view_of).collect(), + document_path: path.display().to_string(), + }) +} + +/// Insert or replace one registry entry, then adopt a new generation. +/// +/// `secrets` maps a reference id (the part after `mcp:`) to the value the +/// operator typed. It is consumed here and never read back. +/// +/// The order is deliberate and every prefix of it is a consistent state: the +/// entry is validated first, so a rejected one changes nothing; the document +/// is written next, which no running agent reads; and the convergence is last, +/// because it is the single write — one pointer rename — that changes what an +/// agent will spawn with. A failure at the convergence leaves the previous +/// generation adopted and the new entry visible but unadopted, which is what +/// the panel then shows. +/// +/// # Errors +/// A message when the entry is unusable, when the document cannot be written, +/// or when the convergence fails. +#[tauri::command] +pub fn save_mcp_registry_server( + app: AppHandle, + entry: RegistryEntry, + secrets: BTreeMap, +) -> Result { + for id in secrets.keys() { + // The reference id is operator-typed, so it is validated against the + // same closed namespace a generated config uses. `identity` and + // `agent:*` are refused there, which is what stops a typed reference + // from naming a private key's blob record. + McpSecretRef::parse(&format!("mcp:{id}")) + .map_err(|e| format!("`{id}` is not a usable secret name: {e}"))?; + } + let path = document_path(&app)?; + let mut document = read_document(&path)?; + match document.servers.iter().position(|e| e.id == entry.id) { + Some(index) => document.servers[index] = entry, + None => { + if document.servers.len() >= MAX_DOCUMENT_SERVERS { + return Err(format!( + "the registry already declares {MAX_DOCUMENT_SERVERS} servers, which is the cap" + )); + } + document.servers.push(entry); + } + } + write_document(&path, &document)?; + converge_now(&app, &secrets)?; + list_mcp_registry_servers(app) +} + +/// Delete one registry entry, drop its id from every agent, and adopt a new +/// generation. +/// +/// The agent records are rewritten *before* the convergence, so the generation +/// this call adopts is built from the records as they now are. The reverse +/// order would stage a generation naming a server no record enables any more, +/// and the deleted server's credential would be carried onto it. +/// +/// # Errors +/// A message when the document or the agent store cannot be written, or when +/// the convergence fails. +#[tauri::command] +pub fn delete_mcp_registry_server(app: AppHandle, id: String) -> Result { + let path = document_path(&app)?; + let mut document = read_document(&path)?; + document.servers.retain(|entry| entry.id != id); + let mut records = crate::managed_agents::load_managed_agents(&app)?; + let mut touched = false; + for record in &mut records { + if let Some(selection) = record.mcp_servers.as_mut() { + let before = selection.enabled.len(); + selection.enabled.retain(|enabled| enabled != &id); + touched |= selection.enabled.len() != before; + } + } + if touched { + crate::managed_agents::save_managed_agents(&app, &records)?; + } + write_document(&path, &document)?; + converge_now(&app, &BTreeMap::new())?; + list_mcp_registry_servers(app) +} + +/// Set one agent's enabled registry servers, then adopt a new generation. +/// +/// Writing the value — rather than clearing it when the list is empty — is +/// what makes memo decision 8's absent-versus-empty distinction real: an +/// operator who turns every server off leaves `Some([])`, which is a decision, +/// while a record that never reached this command keeps `None`. +/// +/// # Errors +/// A message when the agent is unknown, when the store cannot be written, or +/// when the convergence fails — including the refusal an unsupported transport +/// produces, which is surfaced rather than silently dropping the entry. +#[tauri::command] +pub fn set_agent_mcp_servers( + app: AppHandle, + pubkey: String, + enabled: Vec, +) -> Result { + let mut records = crate::managed_agents::load_managed_agents(&app)?; + let record = records + .iter_mut() + .find(|record| record.pubkey == pubkey) + .ok_or_else(|| format!("no agent with pubkey {pubkey}"))?; + record.mcp_servers = Some(AgentMcpServers { + version: AGENT_MCP_SERVERS_VERSION, + enabled, + }); + crate::managed_agents::save_managed_agents(&app, &records)?; + converge_now(&app, &BTreeMap::new())?; + list_mcp_registry_servers(app) +} + +/// One agent's current selection, for the definition dialog. +/// +/// # Errors +/// A message when the agent store cannot be read. +#[tauri::command] +pub fn get_agent_mcp_servers( + app: AppHandle, + pubkey: String, +) -> Result>, String> { + let records = crate::managed_agents::load_managed_agents(&app)?; + Ok(records + .iter() + .find(|record| record.pubkey == pubkey) + .and_then(|record| record.mcp_servers.as_ref()) + .map(|selection| selection.enabled.clone())) +} + +fn read_document(path: &std::path::Path) -> Result { + match std::fs::read(path) { + Ok(bytes) => serde_json::from_slice(&bytes).map_err(|e| { + format!( + "the mcp registry at {} is not valid json: {e}", + path.display() + ) + }), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(RegistryDocument { + version: 1, + servers: Vec::new(), + }), + Err(e) => Err(format!("cannot read {}: {e}", path.display())), + } +} + +fn write_document(path: &std::path::Path, document: &RegistryDocument) -> Result<(), String> { + let body = serde_json::to_vec_pretty(document) + .map_err(|e| format!("cannot serialize the mcp registry: {e}"))?; + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent) + .map_err(|e| format!("cannot create {}: {e}", parent.display()))?; + } + // The same atomic-write helper the agent store uses: a reader never sees a + // prefix, and a failed write leaves the previous document intact. + crate::managed_agents::atomic_write_json(path, &body) +} diff --git a/desktop/src-tauri/src/commands/mod.rs b/desktop/src-tauri/src/commands/mod.rs index e7674c15d94..8b5f3cdce60 100644 --- a/desktop/src-tauri/src/commands/mod.rs +++ b/desktop/src-tauri/src/commands/mod.rs @@ -28,6 +28,7 @@ mod join_policy; mod legacy_storage; mod link_preview; mod managed_agent_definition; +pub(crate) mod mcp_registry; pub(crate) mod media; mod media_animated; mod media_download; @@ -104,6 +105,7 @@ pub use identity_archive::*; pub use join_policy::*; pub use legacy_storage::*; pub use link_preview::*; +pub use mcp_registry::*; pub use media::*; pub use media_download::*; pub use media_fetch_cancellation::*; diff --git a/desktop/src-tauri/src/commands/personas/delete_cascade_tests.rs b/desktop/src-tauri/src/commands/personas/delete_cascade_tests.rs index 6a10a1f9ee2..81a2e8bb308 100644 --- a/desktop/src-tauri/src/commands/personas/delete_cascade_tests.rs +++ b/desktop/src-tauri/src/commands/personas/delete_cascade_tests.rs @@ -17,6 +17,7 @@ fn make_agent( runtime_pid: Option, ) -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: pubkey.to_string(), name: "Test Agent".to_string(), diff --git a/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs b/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs index e90df637314..1da7895cbaa 100644 --- a/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs +++ b/desktop/src-tauri/src/commands/personas/inbound/inbound_tests.rs @@ -163,6 +163,7 @@ const AGENT_PUBKEY: &str = "agentpubkeyhex00000000000000000000000000000000000000 /// event must NEVER be able to overwrite. fn local_agent() -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: AGENT_PUBKEY.to_string(), name: "Local Agent".to_string(), diff --git a/desktop/src-tauri/src/commands/personas/prompt_source/tests.rs b/desktop/src-tauri/src/commands/personas/prompt_source/tests.rs index 0c46b6c693a..10d93e047f2 100644 --- a/desktop/src-tauri/src/commands/personas/prompt_source/tests.rs +++ b/desktop/src-tauri/src/commands/personas/prompt_source/tests.rs @@ -793,6 +793,7 @@ impl Drop for EnvVarGuard { fn linked_record(persona_id: &str) -> crate::managed_agents::ManagedAgentRecord { use crate::managed_agents::{BackendKind, ManagedAgentRecord, RespondTo}; ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: "agent-pk".to_string(), name: "PM".to_string(), diff --git a/desktop/src-tauri/src/commands/personas/snapshot/fidelity_tests.rs b/desktop/src-tauri/src/commands/personas/snapshot/fidelity_tests.rs index 55a64db59bc..177c0315b98 100644 --- a/desktop/src-tauri/src/commands/personas/snapshot/fidelity_tests.rs +++ b/desktop/src-tauri/src/commands/personas/snapshot/fidelity_tests.rs @@ -11,6 +11,7 @@ use std::collections::BTreeMap; fn make_definition(slug: &str) -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: String::new(), slug: Some(slug.to_string()), diff --git a/desktop/src-tauri/src/commands/personas/snapshot/import.rs b/desktop/src-tauri/src/commands/personas/snapshot/import.rs index 041a0b91dc9..a93835d364e 100644 --- a/desktop/src-tauri/src/commands/personas/snapshot/import.rs +++ b/desktop/src-tauri/src/commands/personas/snapshot/import.rs @@ -647,6 +647,7 @@ pub async fn confirm_agent_snapshot_import( // Instance-level behavioral defaults agree with the resolved // definition: both come from the single minted struct so they // are always consistent at mint time. + mcp_servers: None, respond_to: minted.respond_to, respond_to_allowlist: minted.respond_to_allowlist.clone(), is_builtin: false, diff --git a/desktop/src-tauri/src/commands/personas/snapshot/tests.rs b/desktop/src-tauri/src/commands/personas/snapshot/tests.rs index abf4bef443d..753f23188ef 100644 --- a/desktop/src-tauri/src/commands/personas/snapshot/tests.rs +++ b/desktop/src-tauri/src/commands/personas/snapshot/tests.rs @@ -20,6 +20,7 @@ use std::collections::BTreeMap; /// persona_id. fn make_definition(slug: &str) -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: String::new(), slug: Some(slug.to_string()), diff --git a/desktop/src-tauri/src/commands/personas/update/name_propagation_tests.rs b/desktop/src-tauri/src/commands/personas/update/name_propagation_tests.rs index 7aedcb25ef5..ba43887bfda 100644 --- a/desktop/src-tauri/src/commands/personas/update/name_propagation_tests.rs +++ b/desktop/src-tauri/src/commands/personas/update/name_propagation_tests.rs @@ -5,6 +5,7 @@ use super::*; fn agent(persona_id: &str, name: &str, display_name: Option<&str>) -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: format!("pubkey-{name}"), name: name.to_string(), diff --git a/desktop/src-tauri/src/commands/team_snapshot.rs b/desktop/src-tauri/src/commands/team_snapshot.rs index 9c57ce12b53..bb71299f6d7 100644 --- a/desktop/src-tauri/src/commands/team_snapshot.rs +++ b/desktop/src-tauri/src/commands/team_snapshot.rs @@ -604,6 +604,7 @@ pub async fn confirm_team_snapshot_import( last_exit_code: None, last_error: None, last_error_code: None, + mcp_servers: None, respond_to: { use crate::managed_agents::RespondTo; respond_to_wire diff --git a/desktop/src-tauri/src/commands/team_snapshot/tests.rs b/desktop/src-tauri/src/commands/team_snapshot/tests.rs index 13c7f6ae810..b72d75709b1 100644 --- a/desktop/src-tauri/src/commands/team_snapshot/tests.rs +++ b/desktop/src-tauri/src/commands/team_snapshot/tests.rs @@ -193,6 +193,7 @@ fn team_export_with_instance_and_memory_level_uses_supplied_entries() { // Build a fake instance record tied to this team+persona. let instance = ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: "a".repeat(64), name: "Alice".to_string(), diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index a5b7cd8a1bd..8396ab8a2cf 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -321,6 +321,24 @@ pub fn run() { eprintln!("buzz-desktop: persona-snapshot backfill failed: {e}"); } + // Finish whatever the last MCP registry change left owed, before + // any agent is restored. A crash mid-change leaves either a + // half-staged generation the pointer never adopted, or an adopted + // one whose keychain deletions did not all succeed — a credential + // the operator already revoked, still in the store, with nothing + // left to retry it. Best-effort and loud: a start must not be + // blocked by configuration debt, and the next Settings action + // retries the same work. + match managed_agents::mcp_registry::apply::reconcile_at_start(&app_handle) { + Ok(managed_agents::mcp_registry::generation::Reconciled::Nothing) => {} + Ok(outcome) => { + eprintln!("buzz-desktop: mcp registry reconcile at start: {outcome:?}"); + } + Err(e) => { + eprintln!("buzz-desktop: mcp registry reconcile at start failed: {e}"); + } + } + // Warm the loaded-harness registry BEFORE restore so cold-launch // agent spawns can resolve custom/preset runtime ids without // waiting for the frontend's discover_acp_providers call. This is @@ -532,6 +550,11 @@ pub fn run() { Ok(()) }) .invoke_handler(tauri::generate_handler![ + list_mcp_registry_servers, + save_mcp_registry_server, + delete_mcp_registry_server, + set_agent_mcp_servers, + get_agent_mcp_servers, terminal_runtime::terminal_attach, terminal_runtime::terminal_detach, terminal_runtime::terminal_close, diff --git a/desktop/src-tauri/src/managed_agents/agent_events.rs b/desktop/src-tauri/src/managed_agents/agent_events.rs index 85f34260ce7..f1819ced71e 100644 --- a/desktop/src-tauri/src/managed_agents/agent_events.rs +++ b/desktop/src-tauri/src/managed_agents/agent_events.rs @@ -164,6 +164,7 @@ mod tests { fn sample_agent() -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: "agentpubkeyhex".to_string(), name: "Test Agent".to_string(), diff --git a/desktop/src-tauri/src/managed_agents/agent_snapshot_envelope.rs b/desktop/src-tauri/src/managed_agents/agent_snapshot_envelope.rs index 131966409b0..0967b608b86 100644 --- a/desktop/src-tauri/src/managed_agents/agent_snapshot_envelope.rs +++ b/desktop/src-tauri/src/managed_agents/agent_snapshot_envelope.rs @@ -366,6 +366,7 @@ mod tests { /// pubkey/nsec pair matters here. fn record_with_keys(pubkey: String, private_key_nsec: String) -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey, name: "Locked Test".to_string(), diff --git a/desktop/src-tauri/src/managed_agents/agent_snapshot_tests.rs b/desktop/src-tauri/src/managed_agents/agent_snapshot_tests.rs index da881f64f5a..9f73169df37 100644 --- a/desktop/src-tauri/src/managed_agents/agent_snapshot_tests.rs +++ b/desktop/src-tauri/src/managed_agents/agent_snapshot_tests.rs @@ -11,6 +11,7 @@ use std::collections::BTreeMap; /// relevant to snapshot export are filled; the rest use defaults. fn minimal_record() -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: "deadbeef".to_string(), name: "Test Agent".to_string(), diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/effort_tests.rs b/desktop/src-tauri/src/managed_agents/config_bridge/effort_tests.rs index 9c4568fceb4..08e6f0c29fe 100644 --- a/desktop/src-tauri/src/managed_agents/config_bridge/effort_tests.rs +++ b/desktop/src-tauri/src/managed_agents/config_bridge/effort_tests.rs @@ -28,6 +28,7 @@ fn buzz_agent() -> &'static KnownAcpRuntime { pub(super) fn record() -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, pubkey: "test".to_string(), name: "Test Agent".to_string(), persona_id: None, 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 aec492e7234..b2c158a5597 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 @@ -72,6 +72,7 @@ fn test_runtime() -> &'static KnownAcpRuntime { fn test_record() -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: "test".to_string(), name: "Test Agent".to_string(), diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests.rs b/desktop/src-tauri/src/managed_agents/discovery/tests.rs index 585a276f0d7..9ad6ac7f840 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests.rs @@ -206,6 +206,7 @@ fn record_with( override_cmd: Option<&str>, ) -> crate::managed_agents::types::ManagedAgentRecord { crate::managed_agents::types::ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: String::new(), name: "r".to_string(), diff --git a/desktop/src-tauri/src/managed_agents/effective_config/tests.rs b/desktop/src-tauri/src/managed_agents/effective_config/tests.rs index 1ed44ace946..5c077ce8834 100644 --- a/desktop/src-tauri/src/managed_agents/effective_config/tests.rs +++ b/desktop/src-tauri/src/managed_agents/effective_config/tests.rs @@ -41,6 +41,7 @@ fn record( ) -> ManagedAgentRecord { use crate::managed_agents::{BackendKind, RespondTo}; ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: "agent-pk".to_string(), name: "Agent".to_string(), diff --git a/desktop/src-tauri/src/managed_agents/global_config/tests.rs b/desktop/src-tauri/src/managed_agents/global_config/tests.rs index 5f39b7b75f2..e26388cb068 100644 --- a/desktop/src-tauri/src/managed_agents/global_config/tests.rs +++ b/desktop/src-tauri/src/managed_agents/global_config/tests.rs @@ -299,6 +299,7 @@ fn default_global_config_serializes_all_fields() { fn bare_record() -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: "agent".to_string(), name: "Agent".to_string(), diff --git a/desktop/src-tauri/src/managed_agents/mcp_registry/apply.rs b/desktop/src-tauri/src/managed_agents/mcp_registry/apply.rs new file mode 100644 index 00000000000..cfad2b1b589 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/mcp_registry/apply.rs @@ -0,0 +1,304 @@ +//! The production caller for [`converge`], and the app-start reconcile. +//! +//! T7a built the core and T7b wired the spawn seam, but nothing in a shipped +//! build ever staged a generation: `plan_for_spawn` looked through the +//! `current` pointer and found nothing, so the registry was inert. This module +//! is the missing half — the one place a registry edit or a per-agent toggle +//! turns into an adopted generation, and the one place a start finishes +//! whatever a crash left owed. +//! +//! Three things are asserted here rather than assumed, because this is the +//! seam where a value stops being a suggestion and becomes what a child +//! process runs: +//! +//! 1. **The launcher path is absolute.** A relative launcher would make +//! `buzz-acp` refuse to boot for every registry-enabled agent, and would be +//! re-resolved against the session's own working directory if it did not. +//! 2. **The launcher exists and is a regular file.** A generated config naming +//! a missing binary is a configuration every one of those agents fails on, +//! discovered at spawn instead of at save. +//! 3. **The convergence is whole-set.** Every managed agent is passed, even +//! one with no selection: one pointer names one generation for all of them, +//! so an agent left out would be revoked *and* left without artefacts. + +use std::path::Path; + +use tauri::AppHandle; + +use super::converge::{ + converge, AgentSelection, Converged, GenerationInputs, SecretStoreIo, UuidNonces, +}; +use super::generation::{GenerationStore, Reconciled, SecretRemover}; +use super::load::load_registry; +use super::paths::RegistryPaths; +use crate::managed_agents::discovery::KnownAcpRuntime; +use crate::managed_agents::types::ManagedAgentRecord; + +/// File name of the bundled launcher, as it is bundled beside the app binary. +#[cfg(windows)] +pub const LAUNCHER_COMMAND: &str = "buzz-mcp-launch.exe"; + +/// File name of the bundled launcher, as it is bundled beside the app binary. +#[cfg(not(windows))] +pub const LAUNCHER_COMMAND: &str = "buzz-mcp-launch"; + +/// Check `path` before it is written into every generated config. +/// +/// # Errors +/// A message when the path is not absolute, does not exist, or is not a +/// regular file. Each is a configuration that would fail at spawn for every +/// registry-enabled agent instead of at the save that caused it (PR 23 +/// follow-up 2). +pub fn checked_launcher(path: &Path) -> Result { + if !path.is_absolute() { + return Err(format!( + "the bundled mcp launcher resolved to {}, which is not an absolute path; every \ + generated server names it, and the agent harness refuses a relative command", + path.display() + )); + } + let meta = std::fs::symlink_metadata(path).map_err(|e| { + format!( + "the bundled mcp launcher {} cannot be read: {e}", + path.display() + ) + })?; + // A symlink is resolved, not refused: the app bundle legitimately ships + // one. What is refused is a path that is not, in the end, a regular file. + let resolved = if meta.file_type().is_symlink() { + std::fs::canonicalize(path).map_err(|e| { + format!( + "the bundled mcp launcher {} is a link that cannot be resolved: {e}", + path.display() + ) + })? + } else { + path.to_path_buf() + }; + let target = std::fs::metadata(&resolved).map_err(|e| { + format!( + "the bundled mcp launcher {} cannot be read: {e}", + resolved.display() + ) + })?; + if !target.is_file() { + return Err(format!( + "the bundled mcp launcher {} is not a regular file", + resolved.display() + )); + } + path.to_str().map(str::to_string).ok_or_else(|| { + format!( + "the bundled mcp launcher's path {} is not valid UTF-8, so it cannot be named in a \ + generated configuration file", + path.display() + ) + }) +} + +/// This agent's contribution to a convergence. +/// +/// Every managed agent gets one, including an agent whose runtime the registry +/// cannot configure: the convergence is whole-set, so an agent left out would +/// fail [`super::converge::ConvergeError::MissingAgent`] rather than simply +/// receiving nothing. A runtime the registry may not configure, or an unknown +/// runtime, contributes an empty selection — which stages no artefacts and +/// carries no capability. +/// +/// Memo decision 8's absent-versus-empty distinction is read here and nowhere +/// else: `None` and `Some(empty)` both resolve to no servers today, but they +/// are different records and only one of them was chosen by an operator. +pub fn selection_for_record( + record: &ManagedAgentRecord, + runtime_meta: Option<&KnownAcpRuntime>, + runtime_id: &str, +) -> AgentSelection { + let configurable = runtime_meta.is_some_and(|meta| meta.mcp_registry_available); + AgentSelection { + agent_id: record.pubkey.clone(), + runtime_id: runtime_id.to_string(), + transports: runtime_meta + .map(|meta| meta.mcp_transports.to_vec()) + .unwrap_or_default(), + placement: runtime_meta + .map(|meta| meta.mcp_config_placement) + .unwrap_or(crate::managed_agents::McpConfigPlacement::Unsupported), + enabled: if configurable { + record + .mcp_servers + .as_ref() + .map(|selection| selection.enabled.clone()) + .unwrap_or_default() + } else { + // Not "drop the selection": the operator's list stays on the + // record, so turning the runtime on later restores it. What is + // withheld is the generation, because decision 9 has not verified + // this runtime's isolated configuration root. + Vec::new() + }, + } +} + +/// The durable secret store, as the generation store and the convergence see +/// it. +/// +/// Read and write both go through the desktop's own `SecretStore`, which holds +/// the interprocess blob lock — so a convergence cannot interleave with +/// another Buzz process's write. +pub struct DesktopSecrets { + service: &'static str, +} + +impl DesktopSecrets { + /// Bind to the keychain service this build stores its blob under. + pub fn new(service: &'static str) -> Self { + Self { service } + } + + fn store(&self) -> &'static crate::secret_store::SecretStore { + crate::secret_store::SecretStore::shared(self.service) + } +} + +impl SecretRemover for DesktopSecrets { + fn remove(&self, key: &str) -> Result<(), String> { + self.store().mutate_checked(|map| { + map.remove(key); + Ok(()) + }) + } + + fn write_all( + &self, + entries: &std::collections::BTreeMap, + ) -> Result<(), String> { + if entries.is_empty() { + return Ok(()); + } + self.store().mutate_checked(|map| { + for (key, value) in entries { + map.insert(key.clone(), value.clone()); + } + Ok(()) + }) + } +} + +impl SecretStoreIo for DesktopSecrets { + fn read_all(&self) -> Result, String> { + // `Ok(None)` is "no blob has ever been written", which is genuinely + // empty. An unavailable backend is an `Err` from the store and stays + // one: reported as empty it would drop every carried-forward secret + // and adopt a generation whose servers cannot authenticate. + Ok(self + .store() + .load_all_readonly()? + .map(|records| records.into_iter().collect()) + .unwrap_or_default()) + } +} + +/// Stage and adopt one generation from the registry document and every agent's +/// current selection. +/// +/// Called after a registry edit and after a per-agent toggle change — the two +/// user actions that change what an agent's next spawn will read. +/// +/// `pending` holds secret values the operator entered in this action, keyed by +/// reference id. They travel one way: into the store, under the reserved +/// `mcp:` prefix, bound to the generation this call adopts. +/// +/// # Errors +/// A message when the nest is unavailable, the launcher fails +/// [`checked_launcher`], the registry document cannot be read, or the +/// convergence itself fails. Every one of them leaves the previous generation +/// adopted: nothing is half-applied. +pub fn converge_now( + app: &AppHandle, + pending: &std::collections::BTreeMap, +) -> Result { + let Some(paths) = crate::managed_agents::runtime::mcp_registry_paths(app)? else { + return Err( + "this build cannot resolve the agent working directory, so mcp server settings \ + cannot be applied" + .to_string(), + ); + }; + let launcher_path = crate::managed_agents::discovery::resolve_command(LAUNCHER_COMMAND) + .ok_or_else(|| { + format!( + "the bundled mcp launcher `{LAUNCHER_COMMAND}` was not found beside this app, so \ + no mcp server could be started" + ) + })?; + let launcher = checked_launcher(&launcher_path)?; + + let registry = load_registry(&paths.document()).map_err(|e| e.to_string())?; + let records = crate::managed_agents::load_managed_agents(app)?; + let personas = crate::managed_agents::load_personas(app).unwrap_or_default(); + let global = crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); + + let selections: Vec = records + .iter() + .map(|record| { + // A dangling harness id degrades to the record's own snapshot + // rather than failing the whole convergence: the agent is already + // unspawnable for a reason the spawn path reports, and dropping it + // here would breach the whole-set rule and revoke every other + // agent's configuration too. + let runtime_id = crate::managed_agents::resolve_effective_harness_descriptor( + record, &personas, &global, + ) + .map(|descriptor| descriptor.command) + .unwrap_or_else(|_| record.agent_command.clone()); + let meta = crate::managed_agents::known_acp_runtime(&runtime_id); + selection_for_record(record, meta, &runtime_id) + }) + .collect(); + + let secrets = DesktopSecrets::new(crate::app_state::keyring_service()); + converge( + &paths, + ®istry, + &selections, + &GenerationInputs { + launcher: &launcher, + keychain_service: crate::app_state::keyring_service(), + pending, + }, + &secrets, + &UuidNonces, + ) + .map_err(|e| e.to_string()) +} + +/// Finish whatever the last configuration change left owed. +/// +/// Runs once at app start, before any agent is restored: a `PREPARED` journal +/// with no flip is discarded, and a `FLIPPED` one's keychain deletions are +/// retried until they succeed. Without it a crash mid-change leaves a +/// revoked credential in the keychain with nothing left to retry it, and the +/// next Settings action is refused because a deletion is still owed. +/// +/// # Errors +/// A message when the staging tree cannot be opened or a deletion is still +/// owed after the retry. The caller logs it: a start must not be blocked by +/// configuration debt, and the next Settings action retries the same work. +pub fn reconcile_at_start(app: &AppHandle) -> Result { + let Some(paths) = crate::managed_agents::runtime::mcp_registry_paths(app)? else { + return Ok(Reconciled::Nothing); + }; + let store = GenerationStore::open(&paths.generations_root()).map_err(|e| e.to_string())?; + let secrets = DesktopSecrets::new(crate::app_state::keyring_service()); + store + .reconcile(&secrets, &super::generation::NoHooks) + .map_err(|e| e.to_string()) +} + +/// Roots for this app. Re-exported so a caller outside `runtime` can build the +/// same pair the spawn path uses. +pub fn registry_paths( + app: &AppHandle, +) -> Result, String> { + crate::managed_agents::runtime::mcp_registry_paths(app) +} diff --git a/desktop/src-tauri/src/managed_agents/mcp_registry/apply_tests.rs b/desktop/src-tauri/src/managed_agents/mcp_registry/apply_tests.rs new file mode 100644 index 00000000000..9249b6a93cf --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/mcp_registry/apply_tests.rs @@ -0,0 +1,1029 @@ +//! Tests for the production caller (T7c): the seam where a registry edit or a +//! per-agent toggle becomes an adopted generation, the guards that seam +//! asserts, and the producer/consumer bounds it has to agree with. +//! +//! Every one drives shipped code — `apply::checked_launcher`, +//! `apply::selection_for_record`, `converge`, `GenerationStore::reconcile`, +//! `plan_for_spawn`, the shipped loader — and each names the guard whose +//! removal fails it. + +use std::collections::BTreeMap; +use std::path::{Path, PathBuf}; +use std::sync::Mutex; + +use buzz_secret_store_pkg::capability::NONCE_LEN; + +use super::apply::{checked_launcher, selection_for_record}; +use super::converge::{converge, AgentSelection, GenerationInputs, NonceSource, SecretStoreIo}; +use super::generation::{ + Deletion, FlipHooks, FlipStep, GenerationError, GenerationPlan, GenerationStore, JournalPhase, + NoHooks, Reconciled, SecretRemover, MAX_PLAN_FILES, +}; +use super::load::parse_registry; +use super::paths::RegistryPaths; +use super::schema::{ + MAX_ARG_LEN, MAX_ENV_NAME_LEN, MAX_ENV_VALUE_LEN, MAX_GENERATED_ARGS, MAX_NAME_LEN, +}; +use super::spawn::plan_for_spawn; +use crate::managed_agents::{McpConfigPlacement, McpTransport}; + +/// The bundled launcher's stand-in. Absolute on both hosts. +#[cfg(unix)] +const LAUNCHER: &str = "/Applications/Buzz.app/Contents/MacOS/buzz-mcp-launch"; +#[cfg(windows)] +const LAUNCHER: &str = "C:/Buzz/buzz-mcp-launch.exe"; + +const SERVICE: &str = "buzz-desktop-dev"; + +#[cfg(unix)] +const SERVER_BIN: &str = "/usr/local/bin"; +#[cfg(windows)] +const SERVER_BIN: &str = "C:/buzz/bin"; + +const AGENT: &str = "aaaabbbbccccdddd"; + +/// A store a test drives instead of the machine keychain. +#[derive(Default)] +struct FakeStore { + records: Mutex>, + fail_removes: Mutex, +} + +impl SecretRemover for FakeStore { + fn remove(&self, key: &str) -> Result<(), String> { + if *self.fail_removes.lock().unwrap() { + return Err(format!("injected: cannot remove {key}")); + } + self.records.lock().unwrap().remove(key); + Ok(()) + } + + fn write_all(&self, entries: &BTreeMap) -> Result<(), String> { + let mut guard = self.records.lock().unwrap(); + for (key, value) in entries { + guard.insert(key.clone(), value.clone()); + } + Ok(()) + } +} + +impl SecretStoreIo for FakeStore { + fn read_all(&self) -> Result, String> { + Ok(self.records.lock().unwrap().clone()) + } +} + +struct FixedNonce; + +impl NonceSource for FixedNonce { + fn nonce(&self) -> [u8; NONCE_LEN] { + [7u8; NONCE_LEN] + } +} + +fn document(servers: &str) -> String { + format!( + "{{\"version\":1,\"servers\":[{}]}}", + servers.replace("/usr/local/bin", SERVER_BIN) + ) +} + +fn stdio(id: &str, name: &str) -> String { + format!( + "{{\"id\":\"{id}\",\"name\":\"{name}\",\"transport\":\"stdio\",\"command\":\"/usr/local/bin/{name}-mcp\",\"args\":[\"--stdio\"]}}" + ) +} + +fn paths(root: &Path) -> RegistryPaths { + RegistryPaths::new(root.join("base"), root.join("nest")) +} + +fn selection(enabled: &[&str], transports: &[McpTransport]) -> AgentSelection { + AgentSelection { + agent_id: AGENT.to_string(), + runtime_id: "buzz-agent".to_string(), + transports: transports.to_vec(), + placement: McpConfigPlacement::Unsupported, + enabled: enabled.iter().map(|id| (*id).to_string()).collect(), + } +} + +/// A minimal agent record, built through the same deserializer the store uses +/// so the test cannot drift from what a real `managed-agents.json` produces. +fn record() -> crate::managed_agents::types::ManagedAgentRecord { + serde_json::from_value(serde_json::json!({ + "pubkey": AGENT, + "name": "copy", + "relay_url": "wss://relay.example", + "acp_command": "buzz-acp", + "agent_command": "buzz-agent", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 0, + "system_prompt": null, + "created_at": "2026-09-05T00:00:00Z", + "updated_at": "2026-09-05T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null, + })) + .expect("the record fixture deserializes") +} + +fn binding_reader(store: &FakeStore) -> impl Fn(&str) -> Result, String> + '_ { + move |key: &str| Ok(store.records.lock().unwrap().get(key).cloned()) +} + +fn converge_with( + root: &Path, + document_body: &str, + selections: &[AgentSelection], + store: &FakeStore, + pending: &BTreeMap, +) -> Result { + let registry = parse_registry(document_body.as_bytes()).expect("document loads"); + converge( + &paths(root), + ®istry, + selections, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending, + }, + store, + &FixedNonce, + ) +} + +// ── The wiring seam's own assertions ────────────────────────────────────── + +/// PR 23 follow-up 2. Every generated server names the launcher, and +/// `buzz-acp` refuses a relative command — so a relative or missing launcher +/// would make every registry-enabled agent fail to boot, discovered at spawn +/// instead of at the save that caused it. Deleting either arm of +/// `checked_launcher` makes one half of this pass a bad value through. +#[test] +fn mcp_registry_the_wiring_seam_refuses_a_launcher_that_is_not_an_absolute_file() { + let temporary = tempfile::tempdir().expect("tempdir"); + + let relative = PathBuf::from("buzz-mcp-launch"); + let error = checked_launcher(&relative).expect_err("a relative launcher is refused"); + assert!( + error.contains("not an absolute path"), + "the refusal must name the defect, got {error}" + ); + + let missing = temporary.path().join("buzz-mcp-launch"); + let error = checked_launcher(&missing).expect_err("a missing launcher is refused"); + assert!( + error.contains("cannot be read"), + "the refusal must name the defect, got {error}" + ); + + let directory = temporary.path().join("not-a-binary"); + std::fs::create_dir(&directory).expect("mkdir"); + let error = checked_launcher(&directory).expect_err("a directory is refused"); + assert!( + error.contains("not a regular file"), + "the refusal must name the defect, got {error}" + ); + + let real = temporary.path().join("buzz-mcp-launch-real"); + std::fs::write(&real, b"#!/bin/sh\n").expect("write"); + assert_eq!( + checked_launcher(&real).expect("a real absolute file is accepted"), + real.display().to_string() + ); +} + +/// Memo decision 9 through the shipped selection builder: what an agent brings +/// to a convergence comes from its record and from the runtime catalog, and a +/// runtime the registry may not configure contributes an empty selection — +/// never a dropped agent, because a convergence is whole-set. +#[test] +fn mcp_registry_a_selection_reads_the_record_and_the_runtime_gate() { + let mut record = record(); + record.mcp_servers = Some(crate::managed_agents::types::AgentMcpServers { + version: crate::managed_agents::types::AGENT_MCP_SERVERS_VERSION, + enabled: vec!["fake".to_string()], + }); + + let available = crate::managed_agents::known_acp_runtime_exact("buzz-agent") + .expect("buzz-agent is in the catalog"); + assert!( + available.mcp_registry_available, + "this test's premise is that buzz-agent is the verified runtime" + ); + let selection = selection_for_record(&record, Some(available), "buzz-agent"); + assert_eq!(selection.enabled, vec!["fake".to_string()]); + assert_eq!(selection.transports, vec![McpTransport::Stdio]); + + let withheld = + crate::managed_agents::known_acp_runtime_exact("claude").expect("claude is in the catalog"); + assert!( + !withheld.mcp_registry_available, + "decision 9 keeps claude unverified" + ); + let selection = selection_for_record(&record, Some(withheld), "claude"); + assert!( + selection.enabled.is_empty(), + "an unverified runtime contributes nothing, but is still passed" + ); + assert_eq!(selection.agent_id, AGENT, "and is never dropped"); + + let unknown = selection_for_record(&record, None, "somebody-elses-harness"); + assert!(unknown.enabled.is_empty()); + assert_eq!(unknown.placement, McpConfigPlacement::Unsupported); +} + +/// Memo decision 8's absent-versus-empty distinction, at the one place it is +/// durable: the record's own serialization. Absent is a record written before +/// the registry existed; an empty list is an operator who turned everything +/// off. Collapsing the two would make a later default change silently reach +/// records nobody chose it for. +#[test] +fn mcp_registry_an_absent_selection_is_a_different_record_from_an_empty_one() { + let mut record = record(); + + let absent = serde_json::to_value(&record).expect("serialize"); + assert!( + absent.get("mcp_servers").is_none(), + "an absent selection writes no key at all" + ); + + record.mcp_servers = Some(crate::managed_agents::types::AgentMcpServers { + version: crate::managed_agents::types::AGENT_MCP_SERVERS_VERSION, + enabled: Vec::new(), + }); + let empty = serde_json::to_value(&record).expect("serialize"); + assert_eq!( + empty["mcp_servers"]["enabled"], + serde_json::json!([]), + "an empty selection writes an empty list" + ); + assert_eq!(empty["mcp_servers"]["version"], 1, "and its version"); + + let round_tripped: crate::managed_agents::types::ManagedAgentRecord = + serde_json::from_value(empty).expect("deserialize"); + assert_eq!( + round_tripped.mcp_servers, + Some(crate::managed_agents::types::AgentMcpServers { + version: 1, + enabled: Vec::new() + }), + "and reads back as chosen-empty, not as absent" + ); +} + +// ── The two user actions ────────────────────────────────────────────────── + +/// A registry edit adopts a new generation, and the previous one's artefact +/// stops being what a spawn reads. This is the production behaviour PR 23 +/// could not have: `converge` had no caller, so `plan_for_spawn` found no +/// adopted generation and the registry was inert in a shipped build. +#[test] +fn mcp_registry_a_registry_edit_adopts_a_new_generation() { + let temporary = tempfile::tempdir().expect("tempdir"); + let root = temporary.path(); + let store = FakeStore::default(); + let selections = [selection(&["fake"], &[McpTransport::Stdio])]; + + let first = converge_with( + root, + &document(&stdio("fake", "fake")), + &selections, + &store, + &BTreeMap::new(), + ) + .expect("first convergence"); + assert_eq!(first.generation, 1); + + let plan = plan_for_spawn( + &paths(root), + AGENT, + McpConfigPlacement::Unsupported, + binding_reader(&store), + ) + .expect("a spawn resolves the adopted generation"); + let handover = plan + .set + .iter() + .find(|(name, _)| name == "BUZZ_ACP_MCP_REGISTRY") + .map(|(_, value)| value.clone()) + .expect("the handover file is named"); + assert!( + std::fs::read_to_string(&handover) + .expect("read") + .contains("fake"), + "generation 1 carries the server the document declared" + ); + + // The operator deletes the server from the document but leaves the toggle + // on. The generation still moves, and it carries the refusal — never a + // silently shorter server list. + let second = converge_with(root, &document(""), &selections, &store, &BTreeMap::new()) + .expect("the convergence succeeds; the agent carries a refusal"); + assert_eq!(second.generation, 2); + assert_eq!(second.refused.len(), 1, "{second:?}"); + let error = plan_for_spawn( + &paths(root), + AGENT, + McpConfigPlacement::Unsupported, + binding_reader(&store), + ) + .expect_err("and the spawn refuses rather than starting one server short"); + assert!(error.contains("no longer declares it"), "got {error}"); + + // The complete edit: the toggle goes too. + let third = converge_with( + root, + &document(""), + &[selection(&[], &[McpTransport::Stdio])], + &store, + &BTreeMap::new(), + ) + .expect("third convergence"); + assert_eq!(third.generation, 3, "the pointer moved"); + let plan = plan_for_spawn( + &paths(root), + AGENT, + McpConfigPlacement::Unsupported, + binding_reader(&store), + ) + .expect("a spawn resolves the new generation"); + assert!( + plan.is_empty(), + "generation 2 stages nothing for this agent, so its next spawn gets no server" + ); +} + +/// A per-agent toggle change adopts a new generation whose artefact holds +/// exactly the new selection — with the document untouched. Deleting the +/// convergence call from the toggle command leaves the agent spawning from the +/// old generation, which this test catches by reading the file a spawn reads. +#[test] +fn mcp_registry_a_toggle_change_adopts_a_new_generation() { + let temporary = tempfile::tempdir().expect("tempdir"); + let root = temporary.path(); + let store = FakeStore::default(); + let body = document(&format!("{},{}", stdio("one", "one"), stdio("two", "two"))); + + converge_with( + root, + &body, + &[selection(&["one"], &[McpTransport::Stdio])], + &store, + &BTreeMap::new(), + ) + .expect("first convergence"); + + let read_handover = || { + let plan = plan_for_spawn( + &paths(root), + AGENT, + McpConfigPlacement::Unsupported, + binding_reader(&store), + ) + .expect("spawn plan"); + let path = plan + .set + .iter() + .find(|(name, _)| name == "BUZZ_ACP_MCP_REGISTRY") + .map(|(_, value)| value.clone()) + .expect("handover file"); + std::fs::read_to_string(path).expect("read") + }; + + let before = read_handover(); + assert!(before.contains("\"one\""), "{before}"); + assert!(!before.contains("\"two\""), "{before}"); + + let toggled = converge_with( + root, + &body, + &[selection(&["one", "two"], &[McpTransport::Stdio])], + &store, + &BTreeMap::new(), + ) + .expect("toggle convergence"); + assert_eq!(toggled.generation, 2); + + let after = read_handover(); + assert!(after.contains("\"one\""), "{after}"); + assert!(after.contains("\"two\""), "{after}"); +} + +/// Memo decision 2 through the whole production seam: an http entry toggled on +/// for a stdio-only runtime is *refused*, and the refusal is staged so the +/// spawn carries it too. Never silently dropped — an agent short a server it +/// was told to have is a behaviour change the operator cannot see. +#[test] +fn mcp_registry_an_http_entry_on_buzz_agent_is_refused_not_dropped() { + let temporary = tempfile::tempdir().expect("tempdir"); + let root = temporary.path(); + let store = FakeStore::default(); + let body = document( + "{\"id\":\"remote\",\"name\":\"remote\",\"transport\":\"http\",\"url\":\"https://mcp.example/v1\"}", + ); + + let converged = converge_with( + root, + &body, + &[selection(&["remote"], &[McpTransport::Stdio])], + &store, + &BTreeMap::new(), + ) + .expect("the convergence itself succeeds; the agent carries a refusal"); + assert_eq!(converged.refused.len(), 1, "{converged:?}"); + let (agent, message) = &converged.refused[0]; + assert_eq!(agent, AGENT); + assert!( + message.contains("http") && message.contains("buzz-agent"), + "the panel's message must name the transport and the runtime, got {message}" + ); + + let error = plan_for_spawn( + &paths(root), + AGENT, + McpConfigPlacement::Unsupported, + binding_reader(&store), + ) + .expect_err("the spawn refuses rather than starting one server short"); + assert!(error.contains("http"), "got {error}"); +} + +// ── Start-up reconcile ──────────────────────────────────────────────────── + +/// The app-start reconcile PR 23 deferred. A crash between the pointer rename +/// and the `FLIPPED` journal write leaves a `PREPARED` journal naming the +/// generation the pointer already resolves to — an adopted change one write +/// short of its record. The reconcile must finish it, not discard it: +/// discarding would delete the live configuration and leave `current` naming a +/// directory that no longer exists. +#[test] +fn mcp_registry_reconcile_at_start_finishes_an_adopted_generation() { + struct FailAfterRename; + impl FlipHooks for FailAfterRename { + fn after(&self, step: FlipStep) -> Result<(), String> { + match step { + FlipStep::PointerRenamed => Err("crash".to_string()), + _ => Ok(()), + } + } + } + + let temporary = tempfile::tempdir().expect("tempdir"); + let store = GenerationStore::open(&temporary.path().join("mcp")).expect("open"); + let secrets = FakeStore::default(); + secrets + .records + .lock() + .unwrap() + .insert("mcp:stale:0:token".to_string(), "value".to_string()); + + let error = store + .commit( + |_base, _dir| { + Ok(GenerationPlan { + files: vec![(PathBuf::from("agents/x/file.json"), "{}".to_string())], + deletions: vec![Deletion::Secret { + key: "mcp:stale:0:token".to_string(), + }], + secrets: BTreeMap::new(), + }) + }, + &secrets, + &FailAfterRename, + ) + .expect_err("the injected crash aborts the commit"); + assert!( + matches!(error, GenerationError::Injected { .. }), + "{error:?}" + ); + + assert_eq!( + store.current().expect("pointer"), + Some(1), + "already adopted" + ); + let journal = store.journal().expect("journal").expect("outstanding"); + assert_eq!(journal.phase, JournalPhase::Prepared); + assert_eq!(journal.generation, 1); + assert!( + secrets + .records + .lock() + .unwrap() + .contains_key("mcp:stale:0:token"), + "the deletion is still owed" + ); + + let outcome = store.reconcile(&secrets, &NoHooks).expect("reconcile"); + assert_eq!( + outcome, + Reconciled::CompletedCleanup { + generation: 1, + deletions: 1 + } + ); + assert_eq!( + store.current().expect("pointer"), + Some(1), + "the adopted generation survives the reconcile" + ); + assert!( + store.generation_dir(1).join("agents/x/file.json").exists(), + "and so does the configuration it staged" + ); + assert!( + !secrets + .records + .lock() + .unwrap() + .contains_key("mcp:stale:0:token"), + "and the owed deletion is done" + ); + assert!(store.journal().expect("journal").is_none(), "and CLEANED"); +} + +// ── Sol N8, N9, N10 ─────────────────────────────────────────────────────── + +/// Sol N8. A `Deletion` reaches the store from a journal *file*, so the key is +/// re-read rather than re-derived. `identity` is the human nsec and +/// `agent:` is an agent's; a tampered or corrupted journal that named +/// either would destroy a private key nothing on this machine can rebuild. +/// Removing `Deletion::validate` lets the fake store observe the delete. +#[test] +fn mcp_registry_a_deletion_outside_the_mcp_namespace_is_refused() { + for key in ["identity", "agent:deadbeef", "some-other-key"] { + let error = Deletion::Secret { + key: key.to_string(), + } + .validate() + .expect_err("a key outside the namespace is refused"); + assert!( + error.contains(key), + "the refusal must name the key, got {error}" + ); + } + Deletion::Secret { + key: "mcp:agentid:1:token".to_string(), + } + .validate() + .expect("an `mcp:` key is the registry's own"); + + // And the refusal reaches the retry loop rather than the store: the + // journal is on disk, so this is the only place that can catch it. + let temporary = tempfile::tempdir().expect("tempdir"); + let store = GenerationStore::open(&temporary.path().join("mcp")).expect("open"); + let secrets = FakeStore::default(); + secrets + .records + .lock() + .unwrap() + .insert("identity".to_string(), "nsec1...".to_string()); + let error = store + .commit( + |_base, _dir| { + Ok(GenerationPlan { + files: Vec::new(), + deletions: vec![Deletion::Secret { + key: "identity".to_string(), + }], + secrets: BTreeMap::new(), + }) + }, + &secrets, + &NoHooks, + ) + .expect_err("a plan naming the human nsec is refused"); + assert!( + matches!(error, GenerationError::Plan(_)), + "refused before anything is staged, got {error:?}" + ); + assert!( + secrets.records.lock().unwrap().contains_key("identity"), + "and the human nsec is still there" + ); + assert_eq!( + store.current().expect("pointer"), + None, + "nothing was adopted" + ); +} + +/// Sol N8, the plan's own shape. A staged path is joined onto the generation +/// directory, so an absolute path or a `..` component would write outside the +/// tree the pointer rename covers. The count and the total bytes are bounded +/// because both are what the write actually costs. +#[test] +fn mcp_registry_a_plan_that_escapes_the_generation_is_refused() { + let escaping = GenerationPlan { + files: vec![(PathBuf::from("agents/../../etc/passwd"), "x".to_string())], + ..GenerationPlan::default() + }; + let error = escaping.validate().expect_err("`..` is refused"); + assert!(format!("{error}").contains("relative path"), "{error}"); + + let too_many = GenerationPlan { + files: (0..MAX_PLAN_FILES + 1) + .map(|n| (PathBuf::from(format!("agents/a/{n}.json")), String::new())) + .collect(), + ..GenerationPlan::default() + }; + let error = too_many.validate().expect_err("the file count is bounded"); + assert!(format!("{error}").contains("over the"), "{error}"); + + let oversized = GenerationPlan { + files: vec![( + PathBuf::from("agents/a/b.json"), + "x".repeat(super::paths::MAX_ARTEFACT_BYTES + 1), + )], + ..GenerationPlan::default() + }; + let error = oversized + .validate() + .expect_err("a file over what a spawn will read is refused"); + assert!(format!("{error}").contains("cap"), "{error}"); + + GenerationPlan { + files: vec![(PathBuf::from("agents/a/b.json"), "{}".to_string())], + secrets: BTreeMap::from([("mcp:a:1:token".to_string(), "v".to_string())]), + deletions: vec![Deletion::Generation { number: 1 }], + } + .validate() + .expect("an ordinary plan passes"); +} + +/// Sol N9. `Path::exists` answers "a `stat` succeeded", which is true of a +/// FIFO — and opening a FIFO blocks until a writer appears, with no timeout +/// anywhere on the spawn path. The type is read from `symlink_metadata` and +/// anything but a regular file is refused, so a planted FIFO is a refusal +/// rather than a spawn that never returns. +#[cfg(unix)] +#[test] +fn mcp_registry_a_fifo_artefact_is_refused_rather_than_opened() { + use std::os::unix::ffi::OsStrExt as _; + + let temporary = tempfile::tempdir().expect("tempdir"); + let root = temporary.path(); + let store = FakeStore::default(); + converge_with( + root, + &document(&stdio("fake", "fake")), + &[selection(&["fake"], &[McpTransport::Stdio])], + &store, + &BTreeMap::new(), + ) + .expect("convergence"); + + let generations = GenerationStore::open(&paths(root).generations_root()).expect("open"); + let staged = generations.generation_dir(1).join("agents").join(AGENT); + let artefact = staged.join(super::paths::BUZZ_ACP_REGISTRY_FILE); + std::fs::remove_file(&artefact).expect("remove the real artefact"); + let c_path = std::ffi::CString::new(artefact.as_os_str().as_bytes()).expect("cstring"); + // SAFETY-free: a plain libc call with a NUL-terminated path and a mode. + assert_eq!( + unsafe { libc::mkfifo(c_path.as_ptr(), 0o600) }, + 0, + "mkfifo failed: {}", + std::io::Error::last_os_error() + ); + + let error = plan_for_spawn( + &paths(root), + AGENT, + McpConfigPlacement::Unsupported, + binding_reader(&store), + ) + .expect_err("a FIFO where an artefact belongs refuses the spawn"); + assert!( + error.contains("not a regular file"), + "the refusal must say why, got {error}" + ); +} + +/// Sol N10. `journal.json.next` and `current.next` are fixed names in a +/// directory the whole user account can write. `File::create` follows a +/// symlink, so a link planted at either name would have the staging write +/// truncate the link's target. `O_NOFOLLOW` fails the open instead, and the +/// failure is propagated rather than swallowed. +#[cfg(unix)] +#[test] +fn mcp_registry_a_symlink_at_a_staging_name_fails_loudly() { + let temporary = tempfile::tempdir().expect("tempdir"); + let victim = temporary.path().join("victim.txt"); + std::fs::write(&victim, b"do not truncate me").expect("write"); + + let root = temporary.path().join("mcp"); + let store = GenerationStore::open(&root).expect("open"); + std::os::unix::fs::symlink(&victim, root.join("journal.json.next")).expect("symlink"); + + let secrets = FakeStore::default(); + let error = store + .commit( + |_base, _dir| { + Ok(GenerationPlan { + files: vec![(PathBuf::from("agents/x/file.json"), "{}".to_string())], + ..GenerationPlan::default() + }) + }, + &secrets, + &NoHooks, + ) + .expect_err("a planted symlink is a loud failure"); + assert!( + matches!( + error, + GenerationError::Io { + operation: "create", + .. + } + ), + "{error:?}" + ); + assert_eq!( + std::fs::read_to_string(&victim).expect("read"), + "do not truncate me", + "and the link's target is untouched" + ); + assert_eq!( + store.current().expect("pointer"), + None, + "nothing was adopted" + ); +} + +/// PR 23 follow-up 3, the observable half. `write_atomically` installs by +/// rename, leaves no temporary behind, and — because the temporary is created +/// without following a link — refuses a symlink planted at its fixed `.tmp` +/// name rather than truncating what the link points at. The `fsync` calls +/// beside them are not observable from a test; the rename and the no-follow +/// are, and they are the same function's contract. +#[cfg(unix)] +#[test] +fn mcp_registry_a_spawn_write_installs_by_rename_and_follows_no_link() { + let temporary = tempfile::tempdir().expect("tempdir"); + let root = temporary.path(); + let store = FakeStore::default(); + let mut agent = selection(&["fake"], &[McpTransport::Stdio]); + agent.placement = McpConfigPlacement::ProjectFileInWorkdir { file: ".mcp.json" }; + agent.runtime_id = "claude".to_string(); + + converge_with( + root, + &document(&stdio("fake", "fake")), + &[agent], + &store, + &BTreeMap::new(), + ) + .expect("convergence"); + + let plan = plan_for_spawn( + &paths(root), + AGENT, + McpConfigPlacement::ProjectFileInWorkdir { file: ".mcp.json" }, + binding_reader(&store), + ) + .expect("spawn plan"); + let workdir = plan.workdir.clone().expect("a project placement moves cwd"); + assert!(workdir.join(".mcp.json").exists(), "the file is installed"); + assert!( + !workdir.join(".tmp").exists() && !workdir.join(".mcp.tmp").exists(), + "and no temporary is left behind" + ); + + // Plant a link at the temporary's fixed name and rewrite. + let victim = temporary.path().join("victim.txt"); + std::fs::write(&victim, b"do not truncate me").expect("write"); + std::os::unix::fs::symlink(&victim, workdir.join(".mcp.tmp")).expect("symlink"); + let error = plan_for_spawn( + &paths(root), + AGENT, + McpConfigPlacement::ProjectFileInWorkdir { file: ".mcp.json" }, + binding_reader(&store), + ) + .expect_err("a planted link at the temporary name refuses the spawn"); + assert!(error.contains("failed to create"), "got {error}"); + assert_eq!( + std::fs::read_to_string(&victim).expect("read"), + "do not truncate me" + ); +} + +// ── Sol W4: producer and consumer bounds ────────────────────────────────── + +/// Sol W4, the arithmetic half. Every bound the desktop applies to a string +/// that crosses into `buzz-acp` is pinned to that crate's own constant, so a +/// change on either side fails here instead of at a customer's next launch. +#[test] +fn mcp_registry_argument_bounds_match_the_consumer() { + assert_eq!( + MAX_ARG_LEN, + buzz_acp_pkg::mcp_registry::MAX_REGISTRY_ARG_LEN, + "an argument the desktop accepts must be one the harness accepts" + ); + assert_eq!( + MAX_GENERATED_ARGS, + buzz_acp_pkg::mcp_registry::MAX_REGISTRY_ARGS, + "the generated command line is bounded by what the harness reads" + ); + assert_eq!( + MAX_ENV_NAME_LEN + 1 + MAX_ENV_VALUE_LEN, + MAX_ARG_LEN, + "a declared variable is generated as one `NAME=VALUE` argument, so the worst case has \ + to fit by construction" + ); +} + +/// Sol W4, the behavioural half, driven on **both** sides. A name the harness +/// refuses must be refused by the desktop loader first: the harness rejects +/// the whole handover document, so one over-long or underscored name accepted +/// here would stop every registry-enabled agent from starting. +#[test] +fn mcp_registry_name_bounds_match_the_consumer_on_both_sides() { + let launcher = std::path::Path::new(LAUNCHER); + let handover = |name: &str| { + serde_json::json!({ + "version": 1, + "servers": [{ "name": name, "command": LAUNCHER, "args": [] }] + }) + .to_string() + }; + let consumer_refuses = |name: &str| { + buzz_acp_pkg::mcp_registry::parse_registry_file(handover(name).as_bytes(), 16, launcher) + .err() + }; + + for (name, why) in [ + ("a".repeat(MAX_NAME_LEN + 1), "a name one byte over the cap"), + ("has_underscore".to_string(), "an underscored name"), + ] { + let consumer = consumer_refuses(&name) + .unwrap_or_else(|| panic!("the harness must refuse {why}: {name}")); + assert!( + consumer.contains("name"), + "the harness's refusal must be about the name, got {consumer}" + ); + + let entry = format!( + "{{\"id\":\"x\",\"name\":\"{name}\",\"transport\":\"stdio\",\"command\":\"/usr/local/bin/x\",\"args\":[]}}" + ); + let loaded = parse_registry(document(&entry).as_bytes()).expect("the document loads"); + let rejection = loaded.entries[0] + .rejection + .clone() + .unwrap_or_else(|| panic!("the desktop loader must refuse {why}: {name}")); + assert!( + rejection.contains("name"), + "the desktop's refusal must be about the name, got {rejection}" + ); + } + + // And the shape both sides do accept still loads. + let ok = parse_registry(document(&stdio("fine", "fine-server")).as_bytes()).expect("loads"); + assert!(ok.entries[0].rejection.is_none(), "{:?}", ok.entries[0]); +} + +/// The bound that actually costs is the generated command line, not the +/// entry's own `args`: the generator prepends flags and adds two arguments per +/// declared variable, and the harness bounds the sum. Counting it beside the +/// generator is what keeps the count and the generation in step — a flag added +/// to one without the other fails here. +#[test] +fn mcp_registry_generated_arg_count_matches_the_generator() { + let body = document( + "{\"id\":\"one\",\"name\":\"one\",\"transport\":\"stdio\",\"command\":\"/usr/local/bin/one\",\ + \"args\":[\"--a\",\"--b\"],\"env\":{\"A\":\"literal\",\"B\":\"mcp:token\"}},\ + {\"id\":\"two\",\"name\":\"two\",\"transport\":\"http\",\"url\":\"https://mcp.example/v1\",\ + \"auth\":{\"scheme\":\"bearer\",\"secret\":\"mcp:token\"}}", + ); + let loaded = parse_registry(body.as_bytes()).expect("loads"); + for entry in &loaded.entries { + let generated = super::generate::generate_server(LAUNCHER, SERVICE, &entry.entry); + assert_eq!( + super::generate::generated_arg_count(&entry.entry), + generated.args.len(), + "the count and the generator disagree for `{}`", + entry.entry.id + ); + } + + // And an entry that would generate more than the harness reads is refused + // per entry, with the rest of the registry still loading. + let args: Vec = (0..MAX_GENERATED_ARGS) + .map(|n| format!("\"--a{n}\"")) + .collect(); + let fat = format!( + "{{\"id\":\"fat\",\"name\":\"fat\",\"transport\":\"stdio\",\"command\":\"/usr/local/bin/fat\",\"args\":[{}]}}", + args.join(",") + ); + let loaded = parse_registry(document(&format!("{fat},{}", stdio("ok", "ok"))).as_bytes()) + .expect("the document still loads"); + let rejection = loaded.entries[0] + .rejection + .clone() + .expect("the fat entry is refused"); + assert!(rejection.contains("launcher command line"), "{rejection}"); + assert!( + loaded.entries[1].rejection.is_none(), + "the rest still loads" + ); +} + +/// Follow-up 8's shape guard, decided and bound: a NUL cannot cross `execve`, +/// so a command or an argument carrying one is refused at the loader where the +/// operator can see why, rather than truncating at the OS boundary. +#[test] +fn mcp_registry_a_nul_in_a_command_line_is_refused() { + let with_nul = "{\"id\":\"n\",\"name\":\"n\",\"transport\":\"stdio\",\ + \"command\":\"/usr/local/bin/n\",\"args\":[\"--flag\\u0000hidden\"]}"; + let loaded = parse_registry(document(with_nul).as_bytes()).expect("the document loads"); + let rejection = loaded.entries[0] + .rejection + .clone() + .expect("an argument with a NUL is refused"); + assert!(rejection.contains("NUL"), "{rejection}"); + + let long_command = format!( + "{{\"id\":\"l\",\"name\":\"l\",\"transport\":\"stdio\",\"command\":\"/usr/local/bin/{}\",\"args\":[]}}", + "x".repeat(MAX_ARG_LEN) + ); + let loaded = parse_registry(document(&long_command).as_bytes()).expect("loads"); + let rejection = loaded.entries[0] + .rejection + .clone() + .expect("an over-long command is refused"); + assert!(rejection.contains("over the"), "{rejection}"); +} + +/// A secret the operator types is written into the adopted generation's +/// keyspace under the reserved `mcp:` prefix, bound to the agent and the +/// generation — and appears in no generated file. This is the whole of "stored +/// once, never echoed back". +#[test] +fn mcp_registry_a_typed_secret_reaches_the_store_and_no_generated_file() { + let temporary = tempfile::tempdir().expect("tempdir"); + let root = temporary.path(); + let store = FakeStore::default(); + let body = document( + "{\"id\":\"auth\",\"name\":\"auth\",\"transport\":\"stdio\",\ + \"command\":\"/usr/local/bin/auth\",\"args\":[],\"env\":{\"API_KEY\":\"mcp:api-key\"}}", + ); + let pending = BTreeMap::from([("api-key".to_string(), "sk-live-do-not-log".to_string())]); + + converge_with( + root, + &body, + &[selection(&["auth"], &[McpTransport::Stdio])], + &store, + &pending, + ) + .expect("convergence"); + + let records = store.records.lock().unwrap().clone(); + let key = format!("mcp:{AGENT}:1:api-key"); + assert_eq!( + records.get(&key).map(String::as_str), + Some("sk-live-do-not-log"), + "the value is bound to (agent, generation) under the reserved prefix; got {:?}", + records.keys().collect::>() + ); + + let generations = GenerationStore::open(&paths(root).generations_root()).expect("open"); + let staged = generations.generation_dir(1); + let mut checked = 0; + for entry in walk(&staged) { + let body = std::fs::read_to_string(&entry).expect("read"); + assert!( + !body.contains("sk-live-do-not-log"), + "{} carries the value", + entry.display() + ); + assert!( + body.contains("mcp:api-key"), + "{} lost the reference", + entry.display() + ); + checked += 1; + } + assert!(checked > 0, "no staged file was checked"); +} + +fn walk(root: &Path) -> Vec { + let mut found = Vec::new(); + let Ok(entries) = std::fs::read_dir(root) else { + return found; + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + found.extend(walk(&path)); + } else { + found.push(path); + } + } + found +} diff --git a/desktop/src-tauri/src/managed_agents/mcp_registry/converge.rs b/desktop/src-tauri/src/managed_agents/mcp_registry/converge.rs index 9bb4a496e5e..de6af2aacb3 100644 --- a/desktop/src-tauri/src/managed_agents/mcp_registry/converge.rs +++ b/desktop/src-tauri/src/managed_agents/mcp_registry/converge.rs @@ -147,8 +147,34 @@ pub struct Converged { pub refused: Vec<(String, String)>, } +/// What a convergence needs besides the registry and the agents. +/// +/// Grouped rather than passed one by one because the three travel together and +/// only ever come from the same place: the app's own bundle and the operator's +/// current Settings action. +pub struct GenerationInputs<'a> { + /// Absolute path of the bundled launcher, already checked by + /// `apply::checked_launcher`. Every generated stdio and http entry names + /// it as its command. + pub launcher: &'a str, + /// Keychain service the desktop stores its blob under, written into every + /// generated argv so the launcher reads the same store the desktop wrote. + pub keychain_service: &'a str, + /// Secret values the operator entered in this action, keyed by reference + /// id. Written into the adopted generation's keyspace and never read back. + pub pending: &'a BTreeMap, +} + /// Stage and adopt one generation covering every agent in `agents`. /// +/// `pending` carries secret **values** the operator has just entered, keyed by +/// reference id. They are written into the new generation's keyspace for every +/// agent that has a server naming them, and are the only channel a value takes: +/// nothing reads one back, and no generated file or log line holds one. A +/// reference absent from `pending` is carried forward from the base generation +/// instead, and a reference in neither simply has no record — the launcher +/// then refuses to start that server rather than starting it unauthenticated. +/// /// # Errors /// [`ConvergeError`]. Nothing is adopted unless the pointer moved; a failure /// after it leaves the new generation adopted with its deletions still owed in @@ -157,11 +183,15 @@ pub fn converge( paths: &RegistryPaths, registry: &LoadedRegistry, agents: &[AgentSelection], - launcher: &str, - keychain_service: &str, + generation: &GenerationInputs<'_>, secrets: &S, nonces: &dyn NonceSource, ) -> Result { + let GenerationInputs { + launcher, + keychain_service, + pending, + } = generation; if agents.len() > MAX_CONVERGED_AGENTS { return Err(ConvergeError::TooManyAgents { count: agents.len(), @@ -247,6 +277,7 @@ pub fn converge( base, &resolved.servers, &existing, + pending, &mut carried, ); @@ -311,13 +342,23 @@ fn carry_secrets( base: Option, servers: &[RegistryEntry], existing: &BTreeMap, + pending: &BTreeMap, carried: &mut BTreeMap, ) { - let Some(base) = base else { - return; - }; for entry in servers { for reference in entry_references(entry) { + // A value the operator has just typed wins over the stored one: + // that is what "edit this server's credential" means, and it is + // also the only way a first convergence (`base` is `None`) can + // have a secret at all. It is never echoed back — the panel sends + // it once, this is where it lands, and nothing reads it out again. + if let Some(value) = pending.get(reference.id()) { + carried.insert(storage_key(capability, &reference), value.clone()); + continue; + } + let Some(base) = base else { + continue; + }; let from = format!( "{MCP_NAMESPACE_PREFIX}{}:{base}:{}", capability.agent_id(), diff --git a/desktop/src-tauri/src/managed_agents/mcp_registry/generate.rs b/desktop/src-tauri/src/managed_agents/mcp_registry/generate.rs index b0ffb2227e5..b309750b910 100644 --- a/desktop/src-tauri/src/managed_agents/mcp_registry/generate.rs +++ b/desktop/src-tauri/src/managed_agents/mcp_registry/generate.rs @@ -36,6 +36,36 @@ pub struct GeneratedServer { pub args: Vec, } +/// How many arguments [`generate_server`] will produce for `entry`. +/// +/// The loader's bound (`MAX_GENERATED_ARGS`) is on the generated command line +/// rather than on the entry's own `args`, because the generated line is what +/// `buzz-acp` reads and bounds. Counting it here, beside the generator, +/// is what keeps the count and the generation in step: a new flag added below +/// without a matching term here fails +/// `mcp_registry_generated_arg_count_matches_the_generator`. +pub fn generated_arg_count(entry: &RegistryEntry) -> usize { + // `--service `. + let mut count = 2; + match &entry.transport { + RegistryTransport::Stdio { args, .. } => { + // `launch`, `--server `, `--`, the command, then its args. + count += 1 + 2 + 1 + 1 + args.len(); + // One flag and one `NAME=VALUE` per declared variable. + count += 2 * entry.env.len(); + } + RegistryTransport::Http { auth, .. } => { + // `proxy`, `--url `. + count += 1 + 2; + if auth.is_some() { + // `--auth-scheme `, `--secret `. + count += 4; + } + } + } + count +} + /// Build the launcher invocation for one registry entry. /// /// `keychain_service` is the service name the **desktop** stores its secret diff --git a/desktop/src-tauri/src/managed_agents/mcp_registry/generation.rs b/desktop/src-tauri/src/managed_agents/mcp_registry/generation.rs index 4195f6e69de..2b2e1453621 100644 --- a/desktop/src-tauri/src/managed_agents/mcp_registry/generation.rs +++ b/desktop/src-tauri/src/managed_agents/mcp_registry/generation.rs @@ -30,8 +30,11 @@ use std::collections::BTreeMap; use std::fs::File; use std::io::{Read, Write}; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; +use buzz_secret_store_pkg::namespace::{ + MCP_NAMESPACE_PREFIX, RESERVED_AGENT_KEY_PREFIX, RESERVED_IDENTITY_KEY, +}; use serde::{Deserialize, Serialize}; /// Generations kept on disk: the current one plus one to roll back to. @@ -68,6 +71,50 @@ pub enum Deletion { }, } +impl Deletion { + /// Whether this deletion is one the registry is allowed to perform. + /// + /// The registry owns the reserved `mcp:` namespace and nothing else, but a + /// deletion travels through a journal file on disk and back — so the key + /// that reaches [`SecretRemover::remove`] is re-derived from a document, + /// not from the code that planned it. `identity` is the human nsec and + /// `agent:` is an agent's; deleting either would destroy a private + /// key no backup on this machine can rebuild. Checked here, at the one + /// place a `Deletion` becomes an action, rather than at each caller + /// (Sol N8). + /// + /// # Errors + /// A message naming the key when it is outside the `mcp:` namespace. + pub fn validate(&self) -> Result<(), String> { + let Deletion::Secret { key } = self else { + return Ok(()); + }; + if key == RESERVED_IDENTITY_KEY || key.starts_with(RESERVED_AGENT_KEY_PREFIX) { + return Err(format!( + "refusing to delete the reserved secret `{key}`: the mcp registry owns the \ + `{MCP_NAMESPACE_PREFIX}` namespace and nothing else" + )); + } + if !key.starts_with(MCP_NAMESPACE_PREFIX) { + return Err(format!( + "refusing to delete `{key}`: it is outside the `{MCP_NAMESPACE_PREFIX}` namespace \ + the mcp registry owns" + )); + } + Ok(()) + } +} + +/// Largest number of files one staged generation may carry. +/// +/// Two per agent — the generated artefact or the refusal — at the convergence +/// cap of 256 agents, with headroom. It bounds the quantity that costs: the +/// number of files written and fsynced under one mutation lock. +pub const MAX_PLAN_FILES: usize = 1024; + +/// Largest total staged bytes in one generation. +pub const MAX_PLAN_BYTES: usize = 64 * 1024 * 1024; + /// The durable record of an in-flight change. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct Journal { @@ -104,6 +151,68 @@ pub struct GenerationPlan { pub secrets: BTreeMap, } +impl GenerationPlan { + /// Whether this plan is one the store may write. + /// + /// A plan is built by a caller and then written under the mutation lock, + /// so every property the store depends on is checked here rather than + /// assumed: each file path is relative to the generation directory and + /// free of `.` and `..`, so no staged write can leave the tree; the file + /// count and the total bytes are bounded, because both are what the write + /// actually costs; and every key names the reserved `mcp:` namespace, so a + /// plan cannot overwrite the human nsec or an agent key (Sol N8). + /// + /// # Errors + /// [`GenerationError::Plan`] naming the first breach. + pub fn validate(&self) -> Result<(), GenerationError> { + let plan = |message: String| GenerationError::Plan(message); + if self.files.len() > MAX_PLAN_FILES { + return Err(plan(format!( + "the configuration change stages {} files, over the {MAX_PLAN_FILES} cap", + self.files.len() + ))); + } + let mut total = 0usize; + for (relative, body) in &self.files { + if relative.is_absolute() + || relative + .components() + .any(|c| !matches!(c, Component::Normal(_))) + { + return Err(plan(format!( + "the configuration change stages {}, which is not a plain relative path \ + inside the generation directory", + relative.display() + ))); + } + if body.len() > super::paths::MAX_ARTEFACT_BYTES { + return Err(plan(format!( + "the configuration change stages {} at {} bytes, over the {}-byte cap a \ + spawn will read", + relative.display(), + body.len(), + super::paths::MAX_ARTEFACT_BYTES + ))); + } + total = total.saturating_add(body.len()); + } + if total > MAX_PLAN_BYTES { + return Err(plan(format!( + "the configuration change stages {total} bytes, over the {MAX_PLAN_BYTES}-byte cap" + ))); + } + for key in self.secrets.keys() { + Deletion::Secret { key: key.clone() } + .validate() + .map_err(|e| plan(e.replace("delete", "write")))?; + } + for deletion in &self.deletions { + deletion.validate().map_err(plan)?; + } + Ok(()) + } +} + /// Removes a secret from the durable store. /// /// A trait so the generation store can be tested against a store the test @@ -376,6 +485,10 @@ impl GenerationStore { let base = self.current()?; let base_dir = base.map(|number| self.generation_dir(number)); let plan = build(base, base_dir.as_deref())?; + // Before the journal, before any write: a plan that breaches a bound + // or names a key outside the registry's namespace is refused with the + // previous generation still adopted and nothing staged. + plan.validate()?; let next = base.map(|number| number + 1).unwrap_or(1); let mut deletions = plan.deletions.clone(); @@ -422,6 +535,17 @@ impl GenerationStore { // obsolete files to agents. It is therefore propagated, never // discarded. remove_tree(&staged)?; + // Created before the first file rather than by it: a plan can + // legitimately stage nothing — every agent deselected, which is what + // "turn every registry server off" produces — and the directory + // `current` is about to name has to exist all the same. Without this + // the fsync below fails on a directory that was never created and the + // whole change is refused for a state that is perfectly valid. + std::fs::create_dir_all(&staged).map_err(|e| GenerationError::Io { + operation: "create", + path: staged.display().to_string(), + reason: e.to_string(), + })?; for (index, (relative, contents)) in plan.files.iter().enumerate() { write_file_synced(&staged.join(relative), contents)?; self.inject(hooks, FlipStep::FileWritten(index))?; @@ -532,11 +656,18 @@ impl GenerationStore { let mut remaining = Vec::new(); let mut first_failure = None; for (index, deletion) in deletions.iter().enumerate() { - let outcome = match deletion { - Deletion::Secret { key } => secrets.remove(key), - Deletion::Generation { number } => { - remove_tree(&self.generation_dir(*number)).map_err(|e| e.to_string()) - } + let outcome = match deletion.validate() { + // The journal is a file on disk, so the key that reaches the + // store is re-read rather than re-derived. Validating here + // makes a tampered or corrupted entry a surfaced, retried + // failure instead of a delete of the human nsec. + Err(reason) => Err(reason), + Ok(()) => match deletion { + Deletion::Secret { key } => secrets.remove(key), + Deletion::Generation { number } => { + remove_tree(&self.generation_dir(*number)).map_err(|e| e.to_string()) + } + }, }; match outcome { Ok(()) => { @@ -645,11 +776,14 @@ fn retry_deletions( let mut remaining = Vec::new(); let mut first_failure = None; for deletion in deletions { - let outcome = match deletion { - Deletion::Secret { key } => secrets.remove(key), - Deletion::Generation { number } => { - remove_tree(&store.generation_dir(*number)).map_err(|e| e.to_string()) - } + let outcome = match deletion.validate() { + Err(reason) => Err(reason), + Ok(()) => match deletion { + Deletion::Secret { key } => secrets.remove(key), + Deletion::Generation { number } => { + remove_tree(&store.generation_dir(*number)).map_err(|e| e.to_string()) + } + }, }; if let Err(reason) = outcome { first_failure.get_or_insert(reason); @@ -695,7 +829,43 @@ fn remove_tree(path: &Path) -> Result<(), GenerationError> { } } -fn write_file_synced(path: &Path, contents: &str) -> Result<(), GenerationError> { +/// Create `path` for writing without following a symlink at its final +/// component. +/// +/// `File::create` follows a link, so a symlink planted at `journal.json.next` +/// or `current.next` — both fixed names inside a directory the whole user +/// account can write on a shared machine — would have the staging write +/// truncate the link's target instead. `O_NOFOLLOW` fails the open with +/// `ELOOP` instead, so the planted link is a loud error rather than a silent +/// write somewhere else (Sol N10). Windows has no equivalent open flag, so the +/// check there is a `symlink_metadata` call before the open. +pub(super) fn create_no_follow(path: &Path) -> std::io::Result { + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .custom_flags(libc::O_NOFOLLOW) + .open(path) + } + #[cfg(not(unix))] + { + match std::fs::symlink_metadata(path) { + Ok(meta) if meta.file_type().is_symlink() => { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "the path is a symbolic link", + )) + } + Ok(_) | Err(_) => {} + } + File::create(path) + } +} + +pub(super) fn write_file_synced(path: &Path, contents: &str) -> Result<(), GenerationError> { if let Some(parent) = path.parent() { std::fs::create_dir_all(parent).map_err(|e| GenerationError::Io { operation: "create", @@ -703,7 +873,7 @@ fn write_file_synced(path: &Path, contents: &str) -> Result<(), GenerationError> reason: e.to_string(), })?; } - let mut file = File::create(path).map_err(|e| GenerationError::Io { + let mut file = create_no_follow(path).map_err(|e| GenerationError::Io { operation: "create", path: path.display().to_string(), reason: e.to_string(), @@ -718,7 +888,7 @@ fn write_file_synced(path: &Path, contents: &str) -> Result<(), GenerationError> } /// fsync a directory so a rename or a create is durable, not just visible. -fn sync_dir(path: &Path) -> Result<(), GenerationError> { +pub(super) fn sync_dir(path: &Path) -> Result<(), GenerationError> { #[cfg(unix)] { let dir = File::open(path).map_err(|e| GenerationError::Io { diff --git a/desktop/src-tauri/src/managed_agents/mcp_registry/load.rs b/desktop/src-tauri/src/managed_agents/mcp_registry/load.rs index b68b7d9c8a6..4ee2fd01cf0 100644 --- a/desktop/src-tauri/src/managed_agents/mcp_registry/load.rs +++ b/desktop/src-tauri/src/managed_agents/mcp_registry/load.rs @@ -22,10 +22,12 @@ use std::path::Path; use buzz_secret_store_pkg::sentinel::{is_credential_name, scan_argv, scan_value}; use buzz_secret_store_pkg::McpSecretRef; +use super::generate::generated_arg_count; use super::schema::{ RegistryDocument, RegistryEntry, RegistryTransport, BUILTIN_SERVER_NAMES, MAX_ARGS, MAX_ARG_LEN, MAX_DOCUMENT_BYTES, MAX_DOCUMENT_SERVERS, MAX_ENTRY_BYTES, MAX_ENV_ENTRIES, - MAX_ENV_VALUE_LEN, MAX_NAME_LEN, RESERVED_NAME_PREFIX, + MAX_ENV_NAME_LEN, MAX_ENV_VALUE_LEN, MAX_GENERATED_ARGS, MAX_ID_LEN, MAX_NAME_LEN, + RESERVED_NAME_PREFIX, }; /// The pinned auth schemes an HTTP entry may name. @@ -233,8 +235,8 @@ fn read_bounded_no_follow(path: &Path) -> Result>, RegistryError> /// Validate one entry. `Err` carries the operator-facing rejection reason. fn validate_entry(entry: &RegistryEntry) -> Result<(), String> { - check_identifier("id", &entry.id)?; - check_identifier("name", &entry.name)?; + check_id(&entry.id)?; + check_name(&entry.name)?; if entry.name.starts_with(RESERVED_NAME_PREFIX) { return Err(format!( "`{}` uses the reserved `{RESERVED_NAME_PREFIX}` prefix", @@ -250,18 +252,31 @@ fn validate_entry(entry: &RegistryEntry) -> Result<(), String> { validate_env(&entry.env)?; match &entry.transport { - RegistryTransport::Stdio { command, args } => validate_stdio(command, args), - RegistryTransport::Http { url, auth } => validate_http(url, auth.as_ref()), + RegistryTransport::Stdio { command, args } => validate_stdio(command, args)?, + RegistryTransport::Http { url, auth } => validate_http(url, auth.as_ref())?, } + + // The bound that actually costs is the generated launcher argv, which is + // what `buzz-acp` reads and bounds. Counted from the generator itself, so + // the two cannot drift. + let generated = generated_arg_count(entry); + if generated > MAX_GENERATED_ARGS { + return Err(format!( + "it generates a launcher command line of {generated} arguments, over the \ + {MAX_GENERATED_ARGS} cap the agent harness accepts" + )); + } + Ok(()) } -fn check_identifier(field: &str, value: &str) -> Result<(), String> { +/// The registry id: desktop-local, so it keeps the wider charset. +fn check_id(value: &str) -> Result<(), String> { if value.is_empty() { - return Err(format!("its {field} is empty")); + return Err("its id is empty".to_string()); } - if value.len() > MAX_NAME_LEN { + if value.len() > MAX_ID_LEN { return Err(format!( - "its {field} is {} bytes, over the {MAX_NAME_LEN}-byte cap", + "its id is {} bytes, over the {MAX_ID_LEN}-byte cap", value.len() )); } @@ -269,14 +284,65 @@ fn check_identifier(field: &str, value: &str) -> Result<(), String> { .bytes() .all(|b| b.is_ascii_lowercase() || b.is_ascii_digit() || b == b'_' || b == b'-') { + return Err("its id may only use lowercase letters, digits, `_` and `-`".to_string()); + } + Ok(()) +} + +/// The server name: the one operator string that crosses into `buzz-acp`, so +/// it takes the stricter of the two sides' bounds (Sol W4). `buzz-acp` caps a +/// name at 32 bytes over `[A-Za-z0-9-]` and refuses the whole handover +/// document past either, so an underscore or a 33rd byte accepted here would +/// stop every registry-enabled agent from starting. +fn check_name(value: &str) -> Result<(), String> { + if value.is_empty() { + return Err("its name is empty".to_string()); + } + if value.len() > MAX_NAME_LEN { return Err(format!( - "its {field} may only use lowercase letters, digits, `_` and `-`" + "its name is {} bytes, over the {MAX_NAME_LEN}-byte cap the agent harness accepts", + value.len() )); } + if !value + .bytes() + .all(|b| b.is_ascii_lowercase() || b.is_ascii_digit() || b == b'-') + { + return Err( + "its name may only use lowercase letters, digits and `-`; the agent harness refuses \ + any other character in a server name" + .to_string(), + ); + } Ok(()) } fn validate_stdio(command: &str, args: &[String]) -> Result<(), String> { + // Shape guard for the document-supplied command and args (PR 23 follow-up + // 8). A NUL cannot be passed to `execve`, so a value carrying one is a + // command line that silently truncates at the OS boundary or fails far + // from here; it is refused where the operator can see why. Count and + // length are bounded below and by the generated-argv cap in + // `validate_entry`. + if command.as_bytes().contains(&0) { + return Err("its command holds a NUL byte, which no command line can carry".to_string()); + } + if command.len() > MAX_ARG_LEN { + return Err(format!( + "its command is {} bytes, over the {MAX_ARG_LEN}-byte cap; the command is generated \ + as one launcher argument", + command.len() + )); + } + if let Some((index, _)) = args + .iter() + .enumerate() + .find(|(_, arg)| arg.as_bytes().contains(&0)) + { + return Err(format!( + "argument {index} holds a NUL byte, which no command line can carry" + )); + } if !Path::new(command).is_absolute() { return Err(format!( "`{command}` is not an absolute path; the launcher clears PATH, so a bare name would resolve through an environment nobody controls" @@ -329,6 +395,21 @@ fn validate_env(env: &BTreeMap) -> Result<(), String> { )); } for (name, value) in env { + if name.is_empty() || name.len() > MAX_ENV_NAME_LEN { + return Err(format!( + "the name of one env entry is {} bytes; a name must be 1 to \ + {MAX_ENV_NAME_LEN} bytes", + name.len() + )); + } + if name.as_bytes().contains(&0) + || name.as_bytes().contains(&b'=') + || value.as_bytes().contains(&0) + { + return Err(format!( + "`{name}` holds a NUL or `=`, which no `NAME=VALUE` argument can carry" + )); + } if value.len() > MAX_ENV_VALUE_LEN { return Err(format!( "the value of `{name}` is {} bytes, over the {MAX_ENV_VALUE_LEN}-byte cap", diff --git a/desktop/src-tauri/src/managed_agents/mcp_registry/mod.rs b/desktop/src-tauri/src/managed_agents/mcp_registry/mod.rs index 832396b9930..32c3a0be92e 100644 --- a/desktop/src-tauri/src/managed_agents/mcp_registry/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mcp_registry/mod.rs @@ -22,10 +22,13 @@ //! * [`paths`] — the roots, and the one place an agent id becomes a path. //! * [`converge`] — one adopted generation from the document plus each agent's //! selection, with each agent's secrets re-keyed onto it before the flip. +//! * [`apply`] — the production caller: a registry edit or a toggle change +//! becomes one adopted generation, and a start finishes what a crash left. //! * [`spawn`] — what one spawn takes from the adopted generation: the //! artefacts at the placement its runtime names, its own working directory //! when that placement needs one, and the capability. +pub mod apply; pub mod converge; pub mod generate; pub mod generation; @@ -35,6 +38,9 @@ pub mod resolve; pub mod schema; pub mod spawn; +#[cfg(test)] +mod apply_tests; + #[cfg(test)] mod tests; diff --git a/desktop/src-tauri/src/managed_agents/mcp_registry/schema.rs b/desktop/src-tauri/src/managed_agents/mcp_registry/schema.rs index 83b981c3fa9..3d64de13f6b 100644 --- a/desktop/src-tauri/src/managed_agents/mcp_registry/schema.rs +++ b/desktop/src-tauri/src/managed_agents/mcp_registry/schema.rs @@ -25,20 +25,61 @@ pub const MAX_DOCUMENT_SERVERS: usize = 256; /// Inherited from buzz-acp (`crates/buzz-acp/src/lib.rs:5888`). pub const MAX_SERVERS_PER_AGENT: usize = 16; -/// Largest accepted server name or id, in bytes. -pub const MAX_NAME_LEN: usize = 64; +/// Largest accepted registry id, in bytes. +/// +/// An id never leaves the desktop: it keys the agent record's enabled list and +/// nothing downstream reads it, so it keeps the wider bound. +pub const MAX_ID_LEN: usize = 64; + +/// Largest accepted server name, in bytes. +/// +/// Unified with the consumer (Sol W4). A name is the one operator string that +/// crosses into `buzz-acp`, which caps a generated name at +/// `MAX_MCP_NAME_LEN` — 32 bytes, budgeted against `buzz-agent`'s 64-byte +/// qualified `__` name — and refuses the **whole** handover +/// document past it. A desktop that accepted 64 would write a document every +/// registry-enabled agent then failed to start on. +/// `mcp_registry_name_bounds_match_the_consumer` pins the two together. +pub const MAX_NAME_LEN: usize = 32; /// Largest accepted number of command arguments on one entry. pub const MAX_ARGS: usize = 64; /// Largest accepted length of one command argument, in bytes. +/// +/// Unified with `buzz_acp::mcp_registry::MAX_REGISTRY_ARG_LEN`: every one of +/// these strings is copied verbatim into the generated launcher argv, which +/// the consumer bounds at the same number. pub const MAX_ARG_LEN: usize = 1024; /// Largest accepted number of `env` entries on one server. pub const MAX_ENV_ENTRIES: usize = 32; +/// Largest accepted `env` variable name, in bytes. +/// +/// The launcher's own bound (`buzz_mcp_launch::cli::MAX_ENV_NAME_LEN`); a +/// longer name would be refused by the process the desktop generated the +/// argument for. +pub const MAX_ENV_NAME_LEN: usize = 128; + /// Largest accepted `env` value, in bytes. -pub const MAX_ENV_VALUE_LEN: usize = 4 * 1024; +/// +/// Derived, not chosen. Each declared variable is generated as one +/// `NAME=VALUE` launcher argument, and `buzz-acp` refuses the whole handover +/// document when any argument passes [`MAX_ARG_LEN`]. Deriving the value cap +/// from the name cap and the argument cap makes the worst case fit by +/// construction rather than by a number somebody has to keep in step. +pub const MAX_ENV_VALUE_LEN: usize = MAX_ARG_LEN - MAX_ENV_NAME_LEN - 1; + +/// Largest accepted number of arguments on one **generated** launcher command +/// line. +/// +/// The bound that actually costs is the generated argv, not the entry's own +/// `args`: the generator prepends the service flags, the mode, the server name +/// and two arguments per declared variable, and `buzz-acp` bounds the sum. +/// Bounding `args` alone let an entry with 64 arguments and 32 variables +/// generate 135 and take every registry-enabled agent down at startup. +pub const MAX_GENERATED_ARGS: usize = 64; /// Reserved name prefix; no registry server may use it. pub const RESERVED_NAME_PREFIX: &str = "buzz-"; diff --git a/desktop/src-tauri/src/managed_agents/mcp_registry/spawn.rs b/desktop/src-tauri/src/managed_agents/mcp_registry/spawn.rs index a55f7c7db29..be9912c0e6b 100644 --- a/desktop/src-tauri/src/managed_agents/mcp_registry/spawn.rs +++ b/desktop/src-tauri/src/managed_agents/mcp_registry/spawn.rs @@ -17,7 +17,7 @@ use std::path::{Path, PathBuf}; use buzz_secret_store_pkg::{binding_key_for, AgentCapability, CAPABILITY_ENV_VAR}; use super::generate::BUZZ_ACP_REGISTRY_ENV_VAR; -use super::generation::GenerationStore; +use super::generation::{create_no_follow, sync_dir, GenerationStore}; use super::paths::{RegistryPaths, BUZZ_ACP_REGISTRY_FILE, MAX_ARTEFACT_BYTES, REFUSAL_FILE}; use crate::managed_agents::McpConfigPlacement; @@ -108,7 +108,7 @@ where .map_err(|e| e.to_string())?; let refusal = staged.join(REFUSAL_FILE); - if refusal.exists() { + if staged_file_present(&refusal)? { return Err(read_artefact(&refusal)?); } @@ -185,18 +185,63 @@ fn artefact( McpConfigPlacement::EnvRootedDir { file, .. } => file, }; let path = staged.join(name); - if !path.exists() { + if !staged_file_present(&path)? { return Ok(None); } let body = read_artefact(&path)?; Ok(Some((path, body))) } +/// Whether a staged artefact is present **and** is a plain file. +/// +/// `Path::exists` answers "a `stat` succeeded", which is true of a FIFO, a +/// device and a symlink to either. Opening a FIFO blocks until a writer +/// appears, so a spawn that probed with `exists` and then opened would hang +/// forever rather than refuse — with no timeout anywhere on the path. The type +/// is read from `symlink_metadata`, which does not traverse the final +/// component, so a link planted over the artefact is refused rather than +/// followed (Sol N9). The open itself repeats the regular-file check on the +/// **opened handle**, so a swap between this call and the open cannot slip +/// through. +/// +/// # Errors +/// A message when the path exists but is not a regular file, or when its type +/// cannot be read at all. Neither is reported as absent: read as "no +/// artefact", a planted FIFO would silently turn a configured agent into an +/// unconfigured one. +fn staged_file_present(path: &Path) -> Result { + match std::fs::symlink_metadata(path) { + Ok(meta) if meta.file_type().is_file() => Ok(true), + Ok(meta) => Err(format!( + "{} is not a regular file ({:?}); this agent's mcp configuration was not written by \ + this desktop", + path.display(), + meta.file_type() + )), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(false), + Err(e) => Err(format!( + "failed to read the type of {}: {e}", + path.display() + )), + } +} + /// Read one staged file, bounded at [`MAX_ARTEFACT_BYTES`]. +/// +/// The open does not follow a symlink at the final component and the opened +/// handle must be a regular file, so neither a link nor a FIFO swapped in +/// after [`staged_file_present`] can redirect or block the read. fn read_artefact(path: &Path) -> Result { use std::io::Read as _; - let file = - std::fs::File::open(path).map_err(|e| format!("failed to open {}: {e}", path.display()))?; + let file = open_artefact_no_follow(path) + .map_err(|e| format!("failed to open {}: {e}", path.display()))?; + let kind = file + .metadata() + .map_err(|e| format!("failed to read the type of {}: {e}", path.display()))? + .file_type(); + if !kind.is_file() { + return Err(format!("{} is not a regular file", path.display())); + } // One byte past the cap, so the exhausted limit is what reports an // oversized file rather than a truncated read passing as a whole one. let mut bounded = file.take(MAX_ARTEFACT_BYTES as u64 + 1); @@ -221,19 +266,65 @@ fn create_workdir(paths: &RegistryPaths, agent_id: &str) -> Result std::io::Result { + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW) + .open(path) + } + #[cfg(not(unix))] + { + if std::fs::symlink_metadata(path)?.file_type().is_symlink() { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "the path is a symbolic link", + )); + } + std::fs::File::open(path) + } +} + /// Write `body` to `path` through a temporary file and a rename. /// /// A harness reading the file while it is rewritten must see one whole /// generation's configuration or the previous one, never a prefix of the new. +/// +/// The temporary file's bytes and the parent directory's new entry are both +/// fsynced (PR 23 follow-up 3). Without the first, a crash after the rename +/// can leave the harness's config file present, named and empty; without the +/// second, the rename itself can be lost while the temporary file survives, so +/// the agent starts from the previous generation while the desktop believes it +/// installed this one. `write_file_synced` in `generation.rs` already had this +/// shape; this is the same one, and it is also what refuses a symlink planted +/// at the fixed `.tmp` name. fn write_atomically(path: &Path, body: &str) -> Result<(), String> { + use std::io::Write as _; let temporary = path.with_extension("tmp"); - std::fs::write(&temporary, body) - .map_err(|e| format!("failed to write {}: {e}", temporary.display()))?; + let install = |result: std::io::Result<()>, what: &str| { + result.map_err(|e| format!("failed to {what} {}: {e}", temporary.display())) + }; + let mut file = create_no_follow(&temporary) + .map_err(|e| format!("failed to create {}: {e}", temporary.display()))?; + install(file.write_all(body.as_bytes()), "write")?; + install(file.sync_all(), "sync")?; + drop(file); std::fs::rename(&temporary, path).map_err(|e| { // A failed rename leaves the temporary file behind; remove it so a // retry does not read a stale one, and report the rename failure // rather than the cleanup. let _ = std::fs::remove_file(&temporary); format!("failed to install {}: {e}", path.display()) - }) + })?; + // The rename is durable only once the directory entry is. Reported, never + // discarded: a lost rename is the torn state this function exists to + // prevent. + if let Some(parent) = path.parent() { + sync_dir(parent).map_err(|e| e.to_string())?; + } + Ok(()) } diff --git a/desktop/src-tauri/src/managed_agents/mcp_registry/wiring_tests.rs b/desktop/src-tauri/src/managed_agents/mcp_registry/wiring_tests.rs index 6dd210ef994..02f7037e3de 100644 --- a/desktop/src-tauri/src/managed_agents/mcp_registry/wiring_tests.rs +++ b/desktop/src-tauri/src/managed_agents/mcp_registry/wiring_tests.rs @@ -15,7 +15,9 @@ use buzz_secret_store_pkg::{ CAPABILITY_ENV_VAR, }; -use super::converge::{converge, AgentSelection, ConvergeError, NonceSource, SecretStoreIo}; +use super::converge::{ + converge, AgentSelection, ConvergeError, GenerationInputs, NonceSource, SecretStoreIo, +}; use super::generate::BUZZ_ACP_REGISTRY_ENV_VAR; use super::generation::SecretRemover; use super::load::{parse_registry, LoadedRegistry}; @@ -180,8 +182,11 @@ fn mcp_registry_deleted_server_stops_authenticating() { McpConfigPlacement::Unsupported, &["gh", "sn"], )], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &nonces, ) @@ -224,8 +229,11 @@ fn mcp_registry_deleted_server_stops_authenticating() { &paths, &one, &[selection(AGENT_A, McpConfigPlacement::Unsupported, &["sn"])], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &nonces, ) @@ -307,8 +315,11 @@ fn mcp_registry_a_failed_revocation_is_owed_not_abandoned() { McpConfigPlacement::Unsupported, &["gh", "sn"], )], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &nonces, ) @@ -331,8 +342,11 @@ fn mcp_registry_a_failed_revocation_is_owed_not_abandoned() { &paths, &one, &[selection(AGENT_A, McpConfigPlacement::Unsupported, &["sn"])], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &nonces, ) @@ -347,8 +361,11 @@ fn mcp_registry_a_failed_revocation_is_owed_not_abandoned() { &paths, &one, &[selection(AGENT_A, McpConfigPlacement::Unsupported, &["sn"])], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &nonces, ) @@ -370,8 +387,11 @@ fn mcp_registry_spawn_hands_over_the_generation_file_and_the_capability() { &paths, ®istry(&stdio("gh", "github")), &[selection(AGENT_A, McpConfigPlacement::Unsupported, &["gh"])], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &CountingNonces::default(), ) @@ -440,8 +460,11 @@ fn mcp_registry_a_toggle_changes_only_the_named_agents_config() { selection(AGENT_A, placement, &["gh"]), selection(AGENT_B, placement, &["sn"]), ], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &nonces, ) @@ -478,8 +501,11 @@ fn mcp_registry_a_toggle_changes_only_the_named_agents_config() { selection(AGENT_A, placement, &["gh", "sn"]), selection(AGENT_B, placement, &["sn"]), ], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &nonces, ) @@ -526,8 +552,11 @@ fn mcp_registry_a_regenerated_config_is_installed_by_rename() { &paths, &both, &[selection(AGENT_A, placement, &["gh"])], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &nonces, ) @@ -545,8 +574,11 @@ fn mcp_registry_a_regenerated_config_is_installed_by_rename() { &paths, &both, &[selection(AGENT_A, placement, &["gh", "sn"])], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &nonces, ) @@ -601,8 +633,11 @@ fn mcp_registry_an_env_rooted_placement_gets_its_own_root() { &paths, ®istry(&stdio("gh", "github")), &[selection(AGENT_A, placement, &["gh"])], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &CountingNonces::default(), ) @@ -642,8 +677,11 @@ fn mcp_registry_a_staged_refusal_refuses_the_spawn() { ®istry(&stdio("gh", "github")), // `sn` is not in the registry at all. &[selection(AGENT_A, McpConfigPlacement::Unsupported, &["sn"])], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &CountingNonces::default(), ) @@ -673,8 +711,11 @@ fn mcp_registry_generated_servers_without_a_binding_refuse_the_spawn() { &paths, ®istry(&stdio("gh", "github")), &[selection(AGENT_A, McpConfigPlacement::Unsupported, &["gh"])], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &CountingNonces::default(), ) @@ -717,8 +758,11 @@ fn mcp_registry_an_agent_with_no_servers_gets_an_empty_plan() { &paths, ®istry(&stdio("gh", "github")), &[selection(AGENT_A, McpConfigPlacement::Unsupported, &["gh"])], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &CountingNonces::default(), ) @@ -759,8 +803,11 @@ fn mcp_registry_managed_variables_cover_what_a_plan_sets() { &paths, ®istry(&stdio("gh", "github")), &[selection(AGENT_A, placement, &["gh"])], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &CountingNonces::default(), ) @@ -844,8 +891,11 @@ fn mcp_registry_converge_refuses_a_partial_agent_set() { selection(AGENT_A, McpConfigPlacement::Unsupported, &["gh"]), selection(AGENT_B, McpConfigPlacement::Unsupported, &["ln"]), ], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &CountingNonces::default(), ) @@ -857,8 +907,11 @@ fn mcp_registry_converge_refuses_a_partial_agent_set() { &paths, ®istry, &[selection(AGENT_A, McpConfigPlacement::Unsupported, &["gh"])], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &CountingNonces::default(), ) @@ -899,8 +952,11 @@ fn mcp_registry_generated_argv_names_the_desktops_keychain_service() { &paths, ®istry(&stdio("gh", "github")), &[selection(AGENT_A, McpConfigPlacement::Unsupported, &["gh"])], - LAUNCHER, - service, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: service, + pending: &BTreeMap::new(), + }, &store, &CountingNonces::default(), ) @@ -961,8 +1017,11 @@ fn mcp_registry_a_hostile_agent_id_reaches_no_path() { &paths, ®istry(&stdio("gh", "github")), &[selection(hostile, McpConfigPlacement::Unsupported, &["gh"])], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &CountingNonces::default(), ), @@ -989,8 +1048,11 @@ fn mcp_registry_converge_bounds_agents_and_refuses_duplicates() { selection(AGENT_A, McpConfigPlacement::Unsupported, &["gh"]), selection(AGENT_A, McpConfigPlacement::Unsupported, &[]), ], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &CountingNonces::default(), ), @@ -1011,8 +1073,11 @@ fn mcp_registry_converge_bounds_agents_and_refuses_duplicates() { &paths, ®istry, &many, - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &CountingNonces::default(), ), @@ -1032,8 +1097,11 @@ fn mcp_registry_no_generated_file_carries_a_secret() { &paths, ®istry(&stdio("gh", "github")), &[selection(AGENT_A, placement, &["gh"])], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &CountingNonces::default(), ) @@ -1102,8 +1170,11 @@ fn mcp_registry_the_spawn_seam_strips_before_it_sets() { &paths, ®istry(&stdio("gh", "github")), &[selection(AGENT_A, McpConfigPlacement::Unsupported, &["gh"])], - LAUNCHER, - SERVICE, + &GenerationInputs { + launcher: LAUNCHER, + keychain_service: SERVICE, + pending: &BTreeMap::new(), + }, &store, &CountingNonces::default(), ) diff --git a/desktop/src-tauri/src/managed_agents/mod.rs b/desktop/src-tauri/src/managed_agents/mod.rs index 37c2affd09a..a50663964e7 100644 --- a/desktop/src-tauri/src/managed_agents/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mod.rs @@ -43,7 +43,7 @@ mod relay_mesh; mod repos; mod restore; pub mod retention; -mod runtime; +pub(crate) mod runtime; mod runtime_commands; mod runtime_types; mod session_policy; @@ -55,7 +55,7 @@ pub(crate) mod team_events; mod team_repair; pub(crate) use team_repair::team_persona_key; mod teams; -mod types; +pub(crate) mod types; // Shared lock for tests that call `lock_path_mutex` or `lock_env_mutex`. // Both helpers delegate here so any two tests using either helper are mutually diff --git a/desktop/src-tauri/src/managed_agents/nest/render_tests.rs b/desktop/src-tauri/src/managed_agents/nest/render_tests.rs index c712b2525d4..7ba9d94a3ce 100644 --- a/desktop/src-tauri/src/managed_agents/nest/render_tests.rs +++ b/desktop/src-tauri/src/managed_agents/nest/render_tests.rs @@ -38,6 +38,7 @@ fn make_persona(id: &str, display_name: &str) -> AgentDefinition { fn make_agent(name: &str, persona_id: Option<&str>) -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: String::new(), name: name.to_string(), diff --git a/desktop/src-tauri/src/managed_agents/parallelism.rs b/desktop/src-tauri/src/managed_agents/parallelism.rs index f0806c8bc04..99d21587efa 100644 --- a/desktop/src-tauri/src/managed_agents/parallelism.rs +++ b/desktop/src-tauri/src/managed_agents/parallelism.rs @@ -64,6 +64,7 @@ mod tests { fn record_with(runtime: Option<&str>, parallelism: u32) -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: String::new(), name: "r".to_string(), diff --git a/desktop/src-tauri/src/managed_agents/persona_events/tests.rs b/desktop/src-tauri/src/managed_agents/persona_events/tests.rs index 9367ad463e2..354a0dbc9b3 100644 --- a/desktop/src-tauri/src/managed_agents/persona_events/tests.rs +++ b/desktop/src-tauri/src/managed_agents/persona_events/tests.rs @@ -5,6 +5,7 @@ use crate::managed_agents::{BackendKind, ManagedAgentRecord, RespondTo}; /// state right after creation, before any snapshot apply. pub(super) fn sample_record() -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: "p".repeat(64), name: "agent".into(), diff --git a/desktop/src-tauri/src/managed_agents/readiness.rs b/desktop/src-tauri/src/managed_agents/readiness.rs index 95a06b4affc..828c49e2608 100644 --- a/desktop/src-tauri/src/managed_agents/readiness.rs +++ b/desktop/src-tauri/src/managed_agents/readiness.rs @@ -1437,6 +1437,7 @@ mod tests { ); // Minimal record: only the fields resolve_effective_agent_env reads. let record = crate::managed_agents::types::ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: "test-pubkey".to_string(), name: "test-agent".to_string(), diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 124e3487f81..135b5f1d0f2 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -546,7 +546,7 @@ pub(crate) fn spawn_with_effort_proof( /// `None` when the nest cannot be resolved, which is the same sandboxed case /// `default_agent_workdir` already returns `None` for; a spawn there gets no /// registry servers rather than a directory outside the nest. -fn mcp_registry_paths( +pub(crate) fn mcp_registry_paths( app: &AppHandle, ) -> Result, String> { let Some(nest) = super::default_agent_workdir() else { diff --git a/desktop/src-tauri/src/managed_agents/runtime/test_fixtures.rs b/desktop/src-tauri/src/managed_agents/runtime/test_fixtures.rs index 05e11fc4cdf..a4e4385715f 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/test_fixtures.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/test_fixtures.rs @@ -36,6 +36,7 @@ pub(super) fn fixture( auth_tag: Option, ) -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: "p".into(), name: "n".into(), diff --git a/desktop/src-tauri/src/managed_agents/spawn_snapshot/tests.rs b/desktop/src-tauri/src/managed_agents/spawn_snapshot/tests.rs index 388256e01c6..746b56609eb 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_snapshot/tests.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_snapshot/tests.rs @@ -43,6 +43,7 @@ fn snap(record: &ManagedAgentRecord) -> serde_json::Value { fn record() -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: "p".repeat(64), name: "agent".into(), diff --git a/desktop/src-tauri/src/managed_agents/team_snapshot.rs b/desktop/src-tauri/src/managed_agents/team_snapshot.rs index fdeb54c4f27..08524f95f35 100644 --- a/desktop/src-tauri/src/managed_agents/team_snapshot.rs +++ b/desktop/src-tauri/src/managed_agents/team_snapshot.rs @@ -254,6 +254,7 @@ mod tests { /// Build a minimal `ManagedAgentRecord` for use as a team member. fn agent_record(name: &str) -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: format!("{name}-pubkey"), name: name.to_string(), diff --git a/desktop/src-tauri/src/managed_agents/teams_tests.rs b/desktop/src-tauri/src/managed_agents/teams_tests.rs index fc6f0f1a97b..fd7db1b6d6e 100644 --- a/desktop/src-tauri/src/managed_agents/teams_tests.rs +++ b/desktop/src-tauri/src/managed_agents/teams_tests.rs @@ -167,6 +167,7 @@ fn validate_team_deletion_rejects_built_ins() { fn managed_agent(name: &str) -> ManagedAgentRecord { ManagedAgentRecord { + mcp_servers: None, description: None, pubkey: name.to_string(), name: name.to_string(), diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index c8959deff9b..378b222d15f 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -148,6 +148,7 @@ impl AgentDefinition { last_exit_code: None, last_error: None, last_error_code: None, + mcp_servers: None, respond_to: RespondTo::default(), respond_to_allowlist: Vec::new(), display_name: Some(self.display_name), @@ -227,6 +228,25 @@ pub struct RelayAgentInfo { #[serde(default)] pub respond_to_allowlist: Vec, } +/// One agent's MCP registry selection. +/// +/// Versioned so a later default change is a version bump rather than a +/// reinterpretation of what is already stored, and separate from the record's +/// other fields so "never configured" (the whole value absent) stays +/// distinguishable from "configured with nothing" (`enabled` empty). +#[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq, Eq)] +pub struct AgentMcpServers { + /// Schema version of this selection. Read, never inferred. + pub version: u32, + /// Registry entry ids, by `RegistryEntry::id`, never by name — renaming a + /// server must not silently disable it. + #[serde(default)] + pub enabled: Vec, +} + +/// Schema version this build writes for [`AgentMcpServers`]. +pub const AGENT_MCP_SERVERS_VERSION: u32 = 1; + #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct ManagedAgentRecord { pub pubkey: String, @@ -361,6 +381,18 @@ pub struct ManagedAgentRecord { pub last_error: Option, #[serde(default)] pub last_error_code: Option, + /// MCP registry servers this agent has enabled, versioned (memo decision + /// 8). + /// + /// `None` means "no registry servers" and is a **distinct** state from + /// `Some` with an empty list: absence is a record written before the + /// registry existed, while an empty list is an operator who turned every + /// server off. A later default change therefore reads the version rather + /// than reinterpreting absence. Deliberately not added to the kind:30177 + /// projection, which is a field-by-field allowlist, so the selection stays + /// machine-local. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub mcp_servers: Option, /// Inbound author gate mode. Translates to `BUZZ_ACP_RESPOND_TO`. #[serde(default)] pub respond_to: RespondTo, diff --git a/docs/plans/2026-09-04-mcp-registry-design.md b/docs/plans/2026-09-04-mcp-registry-design.md index 6393b231a4b..9cd9d310e4c 100644 --- a/docs/plans/2026-09-04-mcp-registry-design.md +++ b/docs/plans/2026-09-04-mcp-registry-design.md @@ -42,6 +42,12 @@ Reason: `default_agent_workdir()` is a process-wide `OnceLock` returning one pat **10 · Per-server environment on the buzz-agent path.** Decision: buzz-agent servers are declared from the registry with an `env` block carrying `mcp:` references, never values, and the registry never generates a `BUZZ_ACP_EXTRA_MCP_COMMANDS` argv. The handoff is a structured one: a new variable `BUZZ_ACP_MCP_REGISTRY` names a file, a bounded JSON document holding each server's name, command, args and reference-valued `env`, which buzz-acp reads at startup and turns into `McpServer` entries. That file carries references only; the capability of decision 5 never appears in it and reaches the launcher through the inherited spawn environment instead. buzz-acp resolves nothing; the launcher of decision 3, which is the command on every generated entry, is the side that resolves — so no resolved value ever enters buzz-acp's address space or the ACP wire. On top of that, the `acp::wire` logging seam redacts structurally: the serializer used for that log replaces every `mcpServers[].env[].value` with a fixed placeholder, rather than relying on the seam staying at `info`. **Amended 2026-09-05 (T7b), superseding the reference-valued `env` block:** a generated entry carries **no** `env` block at all, and buzz-acp refuses a document that declares one. `buzz-agent` applies `McpServer.env` to the **launcher's own** `Command` — not the server's — so a declared `DYLD_INSERT_LIBRARIES` or `LD_PRELOAD` would run attacker code inside the launcher, before `main`, while decision 5's capability is in that process's environment; the launcher's own `env_clear` for its child is already too late, and the capability authorizes every one of that agent's records, so one entry would read every other server's credential. Nothing is lost: each declared variable was already carried, from the same source, in the launcher's `--set`/`--secret` argv, which is the channel the launcher actually reads and which it applies to the child after stripping the capability. The launcher process therefore holds platform essentials and the capability, nothing else. **Also amended 2026-09-05 (T7b):** the marker that makes buzz-agent forward the capability (`registry_launched`) is no longer set on the desktop's say-so. buzz-acp confines the accepted path to this agent's directory inside the adopted generation — `generations//agents//buzz-acp-registry.json`, with both fields taken from `BUZZ_MCP_CAPABILITY`, which only the spawn seam sets — opens it without following a symlink and requires a regular file, and refuses any entry whose command is not the launcher resolved beside its own binary. Both variables are added to the desktop's `RESERVED_ENV_KEYS`, so no persona, agent record, harness definition or imported definition can supply either. Tests: `registry_file_refuses_a_declared_env_block`, `registry_file_refuses_a_command_that_is_not_the_bundled_launcher`, `registry_path_is_confined_to_the_adopted_generation`, `registry_file_open_refuses_a_symlink`, `mcp_registry_generated_entries_carry_no_env_block`, `mcp_registry_a_user_env_layer_cannot_reach_the_spawned_command`, and the end-to-end `registry_file_capability_reaches_the_spawned_server`, which asserts a declared loader variable never reaches the launcher process. +**Supersedes (2026-09-05, T7c):** decision 10's "generated config names the resolved absolute path of the bundled binary" is superseded by the stronger behaviour that shipped in T7b and is now asserted at the wiring seam. Three parts. First, `buzz-acp` stores the **derived** launcher path — the sibling of its own binary, canonicalized where the filesystem can resolve it — and never the declared string; the declared command is compared and then discarded, so a link swapped after the comparison, or a relative path re-resolved against the session's `cwd`, cannot redirect the exec that carries the agent's capability (`crates/buzz-acp/src/mcp_registry.rs`, `launcher_command`). Second, a declared command must be **plain absolute** — absolute with no `.` and no `..` component — the same shape `confine_registry_path` requires of the registry file's own path, because both are strings that reach an `open` or an `exec`. Third, the desktop no longer trusts its own resolution either: `mcp_registry::apply::checked_launcher` refuses a launcher path that is not absolute, does not exist, or is not a regular file, at the save that caused it rather than at every subsequent spawn. The original wording described only what the desktop writes; this describes what the consumer runs, which is the property that matters. + +**Amended (2026-09-05, T7c) — shape guard on document-supplied `args`.** An entry's `command` and `args` are operator-supplied and are copied verbatim into the generated launcher argv, so the loader bounds them at four things and nothing more: no NUL byte in the command, in any argument, or in an `env` name or value, because no `execve` argument can carry one and a value holding one truncates silently at the OS boundary; the command capped at `MAX_ARG_LEN`, which it did not have before, because the command becomes an argument of the generated line; the per-argument length and the argument count, which were already enforced; and — new, and the one that actually costs — the **generated** argument count, computed by `generate::generated_arg_count` beside the generator itself and capped at `buzz-acp`'s `MAX_REGISTRY_ARGS`. Deliberately *not* guarded: the shape or content of the arguments themselves. A registry server's arguments are how an operator configures a third-party binary, an allow-list of shapes would be a guess about somebody else's CLI, and the credential risk in argv is already answered by the sentinel scan of decision 5. What the guard buys is that a document the desktop accepts is a document the harness can read: before it, an entry with 64 arguments and 32 declared variables generated 135 and took every registry-enabled agent down at startup. + +**Amended (2026-09-05, T7c) — producer and consumer bounds unified (Sol W4).** The two sides disagreed on the one operator string that crosses between them. The desktop accepted a server name of 64 bytes over `[a-z0-9_-]`; `buzz-acp` accepts 32 bytes over `[A-Za-z0-9-]` and refuses the **whole** handover document past either. The desktop loader now takes the stricter of the two — 32 bytes over `[a-z0-9-]`, the intersection — and the registry `id`, which never leaves the desktop, keeps the wider bound. Likewise `MAX_ENV_VALUE_LEN` is no longer chosen: it is derived as `MAX_ARG_LEN - MAX_ENV_NAME_LEN - 1`, so a declared variable's generated `NAME=VALUE` argument fits by construction. `mcp_registry_argument_bounds_match_the_consumer` and `mcp_registry_name_bounds_match_the_consumer_on_both_sides` pin every one of these to `buzz-acp`'s own constants and drive the refusal on both sides. + Reason: an entry's argv is readable by `ps` and by any crash dump, which the config surface itself warns (`crates/buzz-acp/src/config.rs:284-286`) — hence the argv sentinel scan of decision 5. The extras path hardcodes `env: vec![]` with `trusted: false` (`crates/buzz-acp/src/lib.rs:6191-6196`), so it cannot carry the block at all. `send_request` logs the whole serialized message and `McpServer.env` is not skipped (`crates/buzz-acp/src/acp.rs:44,1127`). The declared `env` block is filtered on the same identity rule as the ambient one (`crates/buzz-agent/src/mcp.rs:835-846`). **11 · Capability facts.** @@ -74,6 +80,8 @@ The T7 clauses in `docs/plans/2026-09-04-zs-implementation-plan.md` naming `BUZZ Within this memo, decisions 5 and 9 carry 2026-09-05 (T7b) amendments in place: the capability is minted per generation rather than per spawn; secret writes are journalled before they happen and rolled back on a discard; convergence does not terminate launcher or proxy processes and there is no pending-restart state; the per-agent working directory becomes the spawn cwd only where the placement needs it; and registry availability is an explicit catalog fact rather than a reading of `mcp_transports`. Each is marked at the decision, with the reason. +Within this memo, decisions 3, 8, 9 and 10 carry 2026-09-05 (T7c) amendments in place: decision 10 records the shipped launcher behaviour (a derived canonical path stored, a plain-absolute declared command, and the desktop's own `checked_launcher` at the wiring seam), the shape guard on document-supplied `args`, and the unified producer/consumer bounds; decision 8's staged tree is created even when a plan stages no files at all, which is what "every registry server turned off" produces and what the first production caller surfaced; and the app-start `GenerationStore::reconcile` decision 8 always described now runs, in `lib.rs`'s setup, before any agent is restored. + Within this memo, decision 10's reference-valued `env` block on a generated entry is superseded by the 2026-09-05 (T7b) amendment recorded at that decision: a generated entry carries no `env` block at all, because `buzz-agent` applies that block to the launcher's own process rather than the server's. Decision 10 also gains the three checks that make `registry_launched` earned rather than assumed — a confined path, a no-follow regular-file open, and a command equal to the launcher resolved beside `buzz-acp`'s own binary — and both `BUZZ_ACP_MCP_REGISTRY` and `BUZZ_MCP_CAPABILITY` join `RESERVED_ENV_KEYS`. Within this memo, decision 3's Windows mechanism — `CREATE_SUSPENDED`, `AssignProcessToJobObject`, `ResumeThread` — is superseded by the shipped shape amended into decision 3 on 2026-09-05 (T7b): the launcher joins the kill-on-close Job Object itself before it spawns the server. The guarantee is unchanged; only the mechanism is. From 10ed69d9dfd3907ce30a0b6ef2d80b8a3222744d Mon Sep 17 00:00:00 2001 From: wiggdevin <202901685+wiggdevin@users.noreply.github.com> Date: Sat, 5 Sep 2026 22:43:20 -0700 Subject: [PATCH 2/9] feat(agents): MCP servers Settings panel and per-agent toggles (T7c UI) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The operator-facing half of the registry. - Settings, Agents, MCP servers: list, add, edit and delete stdio and HTTP entries. Nothing is written until an approve step has shown the exact command line, or the exact URL, plus the *name* of every credential the entry will resolve. `approvalSummary` reads the reference names and the keys of the typed values, never a value, so no argument shape can put one on screen. - A credential is typed once and handed to the save, which stores it under the reserved `mcp:` prefix bound to the generation that save adopts. No command returns one, and `entryToDraft` starts an edit with none, so an edit that does not retype a value leaves the stored one untouched. - Per-agent toggles on the agent edit dialog, writing the versioned enabled-server list on the record. Absent stays distinct from empty. - Whether a server may be offered is read from the runtime catalog's `mcpTransports` and nowhere else, per `features/agents/AGENTS.md`. An HTTP entry on a buzz-agent runtime shows an Unsupported badge and the toggle is refused with the reason; toggling *off* always succeeds, so a refused entry can still be removed. - Every failure is surfaced: a failed convergence leaves the previous generation adopted, and the panel says so rather than looking saved. `AgentInstanceEditDialog` is over the file-size ceiling and may not grow, so the section is its own component and the advanced-fields motion choice — the same three lines in both agent dialogs — moves to `agentConfigOptions` as `advancedFieldsMotion`. The dialog ends one line smaller than it started. Tests: 15 node cases on the panel logic (the approve step shows the exact command, no value reaches it, every draft rule matches the loader's) and 5 on the badge; `tests/e2e/mcp-registry-settings.spec.ts` in the smoke allow-list adds a stdio server from Settings, toggles it on for one agent, reads the configuration that agent's next spawn would use, and toggles it off. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_012Z6iidtozXxgx58BUZUKnu Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> --- desktop/playwright.config.ts | 1 + .../agents/ui/AgentDefinitionDialog.tsx | 6 +- .../agents/ui/AgentInstanceEditDialog.tsx | 7 +- .../agents/ui/AgentMcpServersField.tsx | 171 +++++++ .../agents/ui/AgentMcpServersSection.tsx | 60 +++ .../agents/ui/EditAgentAdvancedFields.tsx | 11 + .../features/agents/ui/agentConfigOptions.tsx | 10 + .../agents/ui/agentMcpServersField.test.mjs | 62 +++ .../settings/ui/AgentsSettingsPanel.tsx | 2 + .../settings/ui/McpServersSettingsPanel.tsx | 453 ++++++++++++++++++ .../settings/ui/mcpRegistryLogic.test.mjs | 297 ++++++++++++ .../features/settings/ui/mcpRegistryLogic.ts | 376 +++++++++++++++ desktop/src/shared/api/tauriMcpRegistry.ts | 113 +++++ desktop/src/testing/e2eBridge.ts | 181 +++++++ .../tests/e2e/mcp-registry-settings.spec.ts | 173 +++++++ 15 files changed, 1915 insertions(+), 8 deletions(-) create mode 100644 desktop/src/features/agents/ui/AgentMcpServersField.tsx create mode 100644 desktop/src/features/agents/ui/AgentMcpServersSection.tsx create mode 100644 desktop/src/features/agents/ui/agentMcpServersField.test.mjs create mode 100644 desktop/src/features/settings/ui/McpServersSettingsPanel.tsx create mode 100644 desktop/src/features/settings/ui/mcpRegistryLogic.test.mjs create mode 100644 desktop/src/features/settings/ui/mcpRegistryLogic.ts create mode 100644 desktop/src/shared/api/tauriMcpRegistry.ts create mode 100644 desktop/tests/e2e/mcp-registry-settings.spec.ts diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index 19d94c564d0..21fac02fbaa 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -33,6 +33,7 @@ export default defineConfig({ "**/channels.spec.ts", "**/channel-shared-header-backdrop.spec.ts", "**/channel-files-tab.spec.ts", + "**/mcp-registry-settings.spec.ts", "**/auxiliary-pane-close-visibility.spec.ts", "**/channel-composer-overflow.spec.ts", "**/badge.spec.ts", diff --git a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx index b0ef34cb6a7..7887905b469 100644 --- a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx +++ b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx @@ -31,7 +31,7 @@ import { personaBehaviorDraftValid, } from "./personaBehaviorDraft"; import { - ADVANCED_FIELDS_MOTION_TRANSITION, + advancedFieldsMotion, AUTO_MODEL_DROPDOWN_VALUE, AUTO_PROVIDER_DROPDOWN_VALUE, BLOCK_BUILD_HIDDEN_PROVIDER_IDS, @@ -646,9 +646,7 @@ export function AgentDefinitionDialog({ {runtimeWarningText} Visit Settings > Agents to set it up.

) : null; - const advancedFieldsTransition = shouldReduceMotion - ? { duration: 0 } - : ADVANCED_FIELDS_MOTION_TRANSITION; + const advancedFieldsTransition = advancedFieldsMotion(shouldReduceMotion); React.useEffect(() => { if ( diff --git a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx index 205cf13a449..bd4c64b0bab 100644 --- a/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx +++ b/desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx @@ -30,7 +30,7 @@ import { setManagedAgentAutoRestart } from "@/shared/api/tauriManagedAgents"; import { EffortPickerField } from "./EffortPickerField"; import { EditAgentAdvancedFields } from "./EditAgentAdvancedFields"; import { - ADVANCED_FIELDS_MOTION_TRANSITION, + advancedFieldsMotion, AUTO_PROVIDER_DROPDOWN_VALUE, BLOCK_BUILD_HIDDEN_PROVIDER_IDS, CUSTOM_PROVIDER_DROPDOWN_VALUE, @@ -901,9 +901,7 @@ export function AgentInstanceEditDialog({ const previewLabel = name.trim() || "Agent name"; const previewAvatarUrl = avatarUrl.trim() || null; - const advancedFieldsTransition = shouldReduceMotion - ? { duration: 0 } - : ADVANCED_FIELDS_MOTION_TRANSITION; + const advancedFieldsTransition = advancedFieldsMotion(shouldReduceMotion); // Displayed inline when either the locked update or a standalone setter fails. // setterError takes precedence — the update already committed when it fires. const displayError = @@ -1173,6 +1171,7 @@ export function AgentInstanceEditDialog({ transition={advancedFieldsTransition} > void; + /** `null` for a definition that has not been instantiated yet. */ + pubkey: string | null; + runtime: AcpRuntimeCatalogEntry | null; +}) { + const registry = useMcpRegistryQuery(); + const queryClient = useQueryClient(); + const [refusal, setRefusal] = React.useState(null); + const selection = enabled ?? []; + const servers = registry.data?.servers ?? []; + + if (registry.isError) { + return ( +

+ {String(registry.error)} +

+ ); + } + if (servers.length === 0) { + return ( +

+ No MCP servers are registered. Add one in Settings, Agents, MCP servers. +

+ ); + } + + const apply = async (next: string[]) => { + onEnabledChange(next); + if (pubkey === null) return; + try { + await setAgentMcpServers(pubkey, next); + setRefusal(null); + void queryClient.invalidateQueries({ queryKey: MCP_REGISTRY_QUERY_KEY }); + } catch (error) { + // Surfaced, not swallowed: the write failed, so the agent will spawn + // with the previous selection and the operator has to know that. + setRefusal(String(error)); + } + }; + + return ( +
+ {servers.map((entry) => { + const support = serverSupport(entry, runtime); + const badge = supportBadge(support); + const checked = selection.includes(entry.id); + const inputId = `agent-mcp-server-${entry.id}`; + return ( +
+
+ +

+ {entry.transport === "stdio" + ? [entry.command, ...entry.args].join(" ") + : entry.url} +

+ {support.kind !== "supported" ? ( +

+ {support.reason} +

+ ) : null} +
+ { + const result = toggleServer(selection, entry.id, next, support); + if ("refused" in result) { + setRefusal(result.refused); + return; + } + setRefusal(null); + void apply(result.enabled); + }} + /> +
+ ); + })} + {refusal !== null ? ( +

+ {refusal} +

+ ) : null} +
+ ); +} diff --git a/desktop/src/features/agents/ui/AgentMcpServersSection.tsx b/desktop/src/features/agents/ui/AgentMcpServersSection.tsx new file mode 100644 index 00000000000..bb22c0eefec --- /dev/null +++ b/desktop/src/features/agents/ui/AgentMcpServersSection.tsx @@ -0,0 +1,60 @@ +import * as React from "react"; + +import { getAgentMcpServers } from "@/shared/api/tauriMcpRegistry"; +import type { AcpRuntimeCatalogEntry } from "@/shared/api/types"; + +import { AgentMcpServersField } from "./AgentMcpServersField"; + +/** + * The MCP registry section of the agent edit dialog (memo decision 8). + * + * The selection lives on this agent's record, so it belongs on the instance + * surface — a definition has no pubkey to write to. Each toggle writes the + * record and adopts a configuration generation, which is the single write that + * changes what the agent's next spawn reads; staging that behind the dialog's + * unrelated Save would leave the panel and the agent disagreeing about what is + * enabled. + * + * `null` is "this record has never been configured", which memo decision 8 + * keeps distinct from an empty list. A failed load stays `null` rather than + * becoming `[]`: guessing the other state here would write the wrong one on + * the next toggle. + */ +export function AgentMcpServersSection({ + open, + pubkey, + runtime, +}: { + open: boolean; + pubkey: string; + runtime: AcpRuntimeCatalogEntry | null; +}) { + const [enabled, setEnabled] = React.useState(null); + + React.useEffect(() => { + if (!open) return; + let current = true; + void getAgentMcpServers(pubkey) + .then((selection) => { + if (current) setEnabled(selection); + }) + .catch(() => { + if (current) setEnabled(null); + }); + return () => { + current = false; + }; + }, [open, pubkey]); + + return ( +
+

MCP servers

+ +
+ ); +} diff --git a/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx b/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx index 9577c6b0b4e..f28d6947349 100644 --- a/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx +++ b/desktop/src/features/agents/ui/EditAgentAdvancedFields.tsx @@ -29,7 +29,10 @@ import { type RuntimeCatalogStatus, } from "../lib/agentConfigCore"; +import { AgentMcpServersSection } from "./AgentMcpServersSection"; + export function EditAgentAdvancedFields({ + agentPubkey, acpCommand, agentArgs, autoRestartOnConfigChange, @@ -57,6 +60,8 @@ export function EditAgentAdvancedFields({ onAutoRestartChange, onSystemPromptChange, }: { + /** The agent record this dialog edits; MCP registry toggles write to it. */ + agentPubkey: string; acpCommand: string; agentArgs: string; autoRestartOnConfigChange: boolean; @@ -380,6 +385,12 @@ export function EditAgentAdvancedFields({ provider={provider} /> ) : null} + + ); } diff --git a/desktop/src/features/agents/ui/agentConfigOptions.tsx b/desktop/src/features/agents/ui/agentConfigOptions.tsx index 5c515a05073..392035cc69f 100644 --- a/desktop/src/features/agents/ui/agentConfigOptions.tsx +++ b/desktop/src/features/agents/ui/agentConfigOptions.tsx @@ -36,6 +36,16 @@ export const ADVANCED_FIELDS_MOTION_TRANSITION = { ease: [0.23, 1, 0.32, 1], } as const; +/** + * The advanced-fields transition for one dialog, honouring reduced motion. + * + * Both agent dialogs made the same choice inline; this is the second + * repetition, so it moves here beside the easing it selects. + */ +export function advancedFieldsMotion(reduceMotion: boolean | null) { + return reduceMotion ? { duration: 0 } : ADVANCED_FIELDS_MOTION_TRANSITION; +} + export const AUTO_MODEL_DROPDOWN_VALUE = "__auto_model__"; export const CUSTOM_MODEL_DROPDOWN_VALUE = "__custom_model__"; export const AUTO_PROVIDER_DROPDOWN_VALUE = "__auto_provider__"; diff --git a/desktop/src/features/agents/ui/agentMcpServersField.test.mjs b/desktop/src/features/agents/ui/agentMcpServersField.test.mjs new file mode 100644 index 00000000000..7e750e88196 --- /dev/null +++ b/desktop/src/features/agents/ui/agentMcpServersField.test.mjs @@ -0,0 +1,62 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { supportBadge } from "./AgentMcpServersField"; +import { serverSupport } from "@/features/settings/ui/mcpRegistryLogic"; + +function entry(overrides = {}) { + return { + id: "fake", + name: "fake", + transport: "stdio", + command: "/usr/local/bin/fake-mcp", + args: ["--stdio"], + url: null, + auth_scheme: null, + env: [], + rejection: null, + ...overrides, + }; +} + +const BUZZ_AGENT = { + id: "buzz-agent", + label: "Buzz Agent", + mcpTransports: ["stdio"], +}; + +const CLAUDE = { + id: "claude", + label: "Claude", + mcpTransports: ["stdio", "http"], +}; + +test("a usable stdio entry on a stdio runtime gets no badge", () => { + assert.equal(supportBadge(serverSupport(entry(), BUZZ_AGENT)), null); +}); + +test("an http entry on a buzz-agent runtime is badged Unsupported", () => { + const http = entry({ id: "remote", name: "remote", transport: "http" }); + const badge = supportBadge(serverSupport(http, BUZZ_AGENT)); + assert.deepEqual(badge, { label: "Unsupported", tone: "warn" }); +}); + +test("the same http entry on a runtime whose catalog declares http is not badged", () => { + const http = entry({ id: "remote", name: "remote", transport: "http" }); + assert.equal(supportBadge(serverSupport(http, CLAUDE)), null); +}); + +test("a loader-rejected entry is badged Disabled whatever the runtime", () => { + const rejected = entry({ rejection: "its command is not an absolute path" }); + assert.deepEqual(supportBadge(serverSupport(rejected, CLAUDE)), { + label: "Disabled", + tone: "warn", + }); +}); + +test("an agent whose harness the registry cannot configure is badged, not hidden", () => { + assert.deepEqual(supportBadge(serverSupport(entry(), null)), { + label: "Not configurable", + tone: "warn", + }); +}); diff --git a/desktop/src/features/settings/ui/AgentsSettingsPanel.tsx b/desktop/src/features/settings/ui/AgentsSettingsPanel.tsx index b2fde755ff2..e57ef8fd320 100644 --- a/desktop/src/features/settings/ui/AgentsSettingsPanel.tsx +++ b/desktop/src/features/settings/ui/AgentsSettingsPanel.tsx @@ -5,6 +5,7 @@ import { } from "@/features/messages/lib/autoPinMentionedAgentsPreference"; import { Switch } from "@/shared/ui/switch"; import { HarnessesSettingsPanel } from "./HarnessesSettingsPanel"; +import { McpServersSettingsPanel } from "./McpServersSettingsPanel"; import { PreventSleepSettingsCard } from "./PreventSleepSettingsCard"; import { SettingsOptionGroup, @@ -50,6 +51,7 @@ export function AgentsSettingsPanel() { + diff --git a/desktop/src/features/settings/ui/McpServersSettingsPanel.tsx b/desktop/src/features/settings/ui/McpServersSettingsPanel.tsx new file mode 100644 index 00000000000..ef15750ecfa --- /dev/null +++ b/desktop/src/features/settings/ui/McpServersSettingsPanel.tsx @@ -0,0 +1,453 @@ +import * as React from "react"; +import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; +import { Plus, ShieldAlert, Trash2 } from "lucide-react"; + +import { + deleteMcpRegistryServer, + listMcpRegistryServers, + saveMcpRegistryServer, + type McpRegistryEntry, +} from "@/shared/api/tauriMcpRegistry"; +import { Button } from "@/shared/ui/button"; +import { Input } from "@/shared/ui/input"; +import { cn } from "@/shared/lib/cn"; + +import { + approvalSummary, + draftProblem, + draftToInput, + emptyDraft, + entryToDraft, + type McpServerDraft, +} from "./mcpRegistryLogic"; +import { SettingsOptionGroup } from "./SettingsOptionGroup"; + +/** Query key for the registry document, so a save invalidates every reader. */ +export const MCP_REGISTRY_QUERY_KEY = ["mcp-registry"] as const; + +/** Read the registry document and each entry's status. */ +export function useMcpRegistryQuery() { + return useQuery({ + queryKey: MCP_REGISTRY_QUERY_KEY, + queryFn: listMcpRegistryServers, + }); +} + +function TransportChoice({ + draft, + onChange, +}: { + draft: McpServerDraft; + onChange: (draft: McpServerDraft) => void; +}) { + return ( +
+ {(["stdio", "http"] as const).map((transport) => ( + + ))} +
+ ); +} + +/** + * The approve step. + * + * Nothing is written until the operator has seen the exact command line, or + * the exact URL, plus the *name* of every credential the entry will resolve. + * The values themselves are in `draft.secrets` and reach only the save call. + */ +function ApproveStep({ + draft, + onBack, + onConfirm, + pending, +}: { + draft: McpServerDraft; + onBack: () => void; + onConfirm: () => void; + pending: boolean; +}) { + const summary = approvalSummary(draft); + return ( +
+

{summary.headline}

+ 
+        {summary.target}
+
+ {summary.references.length > 0 ? ( +
+

+ It resolves these credentials by name. The values stay in the + keychain and are never written to a config file: +

+
    + {summary.references.map((reference) => ( +
  • {reference}
    • + ))} +
+
+ ) : null} + {summary.newSecrets.length > 0 ? ( +

+ {`Saving stores ${summary.newSecrets.length} new credential value${summary.newSecrets.length === 1 ? "" : "s"} (${summary.newSecrets.join(", ")}). You will not be able to read them back.`} +

+ ) : null} +
+ + +
+
+ ); +} + +function ServerForm({ + draft, + onCancel, + onChange, + onReview, +}: { + draft: McpServerDraft; + onCancel: () => void; + onChange: (draft: McpServerDraft) => void; + onReview: () => void; +}) { + const problem = draftProblem(draft); + return ( +
+ +
+ + +
+ {draft.transport === "stdio" ? ( + <> + +