Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions crates/buzz-acp/src/mcp_registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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.
Expand Down
1 change: 1 addition & 0 deletions desktop/playwright.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
"**/channel-files-index.spec.ts",
"**/auxiliary-pane-close-visibility.spec.ts",
"**/channel-composer-overflow.spec.ts",
Expand Down
2 changes: 2 additions & 0 deletions desktop/src-tauri/src/app_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ pub struct AppState {
/// Never perform network I/O while holding this lock.
pub managed_agent_runtime_transition: Mutex<()>,
pub managed_agents_store_lock: Mutex<()>,
pub mcp_registry_store_lock: Mutex<()>,
pub channel_templates_store_lock: Mutex<()>,
pub managed_agent_processes: Mutex<HashMap<ManagedAgentRuntimeKey, ManagedAgentPairRuntime>>,
pub provider_deploy_locks: Mutex<HashMap<String, Arc<tokio::sync::Mutex<()>>>>,
Expand Down Expand Up @@ -221,6 +222,7 @@ pub fn build_app_state() -> AppState {
managed_agent_runtime_transition: Mutex::new(()),
identity_mutation: Mutex::new(()),
managed_agents_store_lock: Mutex::new(()),
mcp_registry_store_lock: Mutex::new(()),
channel_templates_store_lock: Mutex::new(()),
managed_agent_processes: Mutex::new(HashMap::new()),
provider_deploy_locks: Mutex::new(HashMap::new()),
Expand Down
1 change: 1 addition & 0 deletions desktop/src-tauri/src/commands/agent_config_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
40 changes: 39 additions & 1 deletion desktop/src-tauri/src/commands/agents.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -1056,6 +1057,41 @@ pub async fn stop_managed_agent(

// Async so the blocking body (disk reads/writes, process termination, keyring
// delete, nest regeneration) runs off the main UI thread via spawn_blocking.
/// Converge the mcp registry against `records` (which already omit the
/// just-deleted agent) and turn a convergence failure into a propagated
/// error instead of a printed-and-swallowed one.
///
/// A deleted agent's `mcp:` state (selections, and any credentials keyed to
/// it) is only actually retired by this convergence. Catching the failure
/// here and returning `Ok` anyway — the prior behaviour — reported a clean
/// delete while the agent's registry state and credentials could be left
/// stale, which is exactly the catch-log-and-return-success pattern
/// AGENTS.md Review-Proven Rule 1 forbids (Sol T7c round 2, item 7).
///
/// # Errors
/// A message naming the agent and the convergence failure. The agent record
/// itself has already been removed by the time this runs; the error tells
/// the caller its mcp state may not have followed.
fn propagate_mcp_convergence_after_deletion<R: tauri::Runtime>(
app: &AppHandle<R>,
pubkey: &str,
records: &[ManagedAgentRecord],
) -> Result<(), String> {
crate::managed_agents::mcp_registry::apply::converge_now_with_records(
app,
records,
&std::collections::BTreeMap::new(),
)
.map(|_| ())
.map_err(|e| {
format!(
"agent {pubkey} was removed, but its mcp registry state could not be converged \
afterward: {e}; its mcp registry selection and any credentials may be stale until \
a later registry edit or toggle retries convergence"
)
})
}

fn run_managed_agent_deletion<T>(
base_dir: &std::path::Path,
pubkey: &str,
Expand Down Expand Up @@ -1133,7 +1169,9 @@ pub async fn delete_managed_agent(
}
state.clear_agent_session_caches(&pubkey);
records.retain(|record| record.pubkey != pubkey);
save_managed_agents(&app, records)
save_managed_agents(&app, records)?;
propagate_mcp_convergence_after_deletion(&app, &pubkey, records)?;
Ok(())
})?;
crate::managed_agents::delete_agent_key(&pubkey);
// Tombstone after confirmed removal (inside lock; every published
Expand Down
92 changes: 92 additions & 0 deletions desktop/src-tauri/src/commands/agents_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -790,3 +791,94 @@ fn owner_only_access_deploy_payload_clamps_stale_access() {
"owner-only-access deploy payload retained a stale allowlist"
);
}

// ── Item 7: agent deletion propagates a swallowed mcp convergence failure ──

/// Isolated env with no `buzz-mcp-launch` reachable anywhere on `PATH`, so
/// `converge_now_with_records` fails deterministically at launcher
/// resolution — the same real failure a build without the bundled launcher
/// sidecar hits, not a synthetic mock.
struct McpConvergenceEnvGuard {
_path_guard: std::sync::MutexGuard<'static, ()>,
_temp: tempfile::TempDir,
old_home: Option<std::ffi::OsString>,
old_xdg: Option<std::ffi::OsString>,
old_path: Option<std::ffi::OsString>,
}

impl McpConvergenceEnvGuard {
fn new() -> Self {
let path_guard = crate::managed_agents::lock_path_mutex();
crate::managed_agents::clear_resolve_cache();
let temp = tempfile::tempdir().unwrap_or_else(|error| panic!("temp dir: {error}"));
let home = temp.path().join("home");
let empty_bin = temp.path().join("empty-bin");
std::fs::create_dir_all(&home).unwrap_or_else(|error| panic!("create home: {error}"));
std::fs::create_dir_all(&empty_bin)
.unwrap_or_else(|error| panic!("create empty bin: {error}"));

let old_home = std::env::var_os("HOME");
let old_xdg = std::env::var_os("XDG_DATA_HOME");
let old_path = std::env::var_os("PATH");
std::env::set_var("HOME", &home);
std::env::set_var("XDG_DATA_HOME", &home);
std::env::set_var("PATH", &empty_bin);

Self {
_path_guard: path_guard,
_temp: temp,
old_home,
old_xdg,
old_path,
}
}
}

impl Drop for McpConvergenceEnvGuard {
fn drop(&mut self) {
crate::managed_agents::clear_resolve_cache();
if let Some(ref old) = self.old_home {
std::env::set_var("HOME", old);
} else {
std::env::remove_var("HOME");
}
if let Some(ref old) = self.old_xdg {
std::env::set_var("XDG_DATA_HOME", old);
} else {
std::env::remove_var("XDG_DATA_HOME");
}
if let Some(ref old) = self.old_path {
std::env::set_var("PATH", old);
} else {
std::env::remove_var("PATH");
}
}
}

#[test]
fn agents_delete_propagates_mcp_convergence_failure_instead_of_swallowing_it() {
let _guard = McpConvergenceEnvGuard::new();
let state = crate::app_state::build_app_state();
*state.keys.lock().unwrap() = nostr::Keys::generate();
let app = tauri::test::mock_builder()
.manage(state)
.build(tauri::test::mock_context(tauri::test::noop_assets()))
.expect("mock app builds headless");

let records: Vec<ManagedAgentRecord> = vec![];
let err = propagate_mcp_convergence_after_deletion(app.handle(), "agent-x", &records)
.expect_err("convergence must fail without a resolvable launcher");

assert!(
err.contains("agent-x"),
"error should name the agent, got: {err}"
);
assert!(
err.contains("removed"),
"error should say the agent was already removed, got: {err}"
);
assert!(
err.contains("stale"),
"error should warn its mcp state may be stale, got: {err}"
);
}
Loading
Loading