From 4324d551d6be3442359b3542d18df3eca422c1de Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Fri, 21 Aug 2026 19:39:06 -0600 Subject: [PATCH] fix: reserve category names, confirm fork-name collisions, report secrets on uninstall Bundle names that collide with an asset category (agents, roles, skills, macros, functions, mcp_config) are now owner-qualified at install time, whether derived from the repo or declared by a manifest, so no bundle can shadow a category by name. A manifest name that collides with a bundle from a different source now prompts for confirmation interactively (a fork or typo-squat is the likely cause); declining aborts before anything is written, and non-interactive runs keep the deterministic owner-qualification. Uninstall summaries now list the vault secrets the bundle's MCP servers reference, noting they are installed by the bundle but not removed. Also removes the dead ResolvedBundleName.migrated_from field. --- src/config/bundles.rs | 83 ++++++++++++++++++++++++++---------- src/config/install_remote.rs | 70 +++++++++++++++++++++++++++++- 2 files changed, 129 insertions(+), 24 deletions(-) diff --git a/src/config/bundles.rs b/src/config/bundles.rs index 7405761..aefd4f2 100644 --- a/src/config/bundles.rs +++ b/src/config/bundles.rs @@ -2,10 +2,13 @@ use super::install_remote::{ canonical_source_url, owner_qualifier, repo_name_slug, validate_bundle_name, }; use super::paths; +use crate::config::AssetCategory; use crate::function::write_file_atomic; +use crate::utils::IS_STDOUT_TERMINAL; use anyhow::{Context, Result, bail}; use chrono::{SecondsFormat, Utc}; +use inquire::Confirm; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use std::collections::BTreeMap; @@ -92,9 +95,6 @@ pub(crate) struct ResolvedBundleName { pub(crate) name: String, /// The unqualified name this install asked for, when it had to be owner-qualified. pub(crate) qualified_from: Option, - /// The record key this source was previously tracked under, when it changed. - #[allow(dead_code)] - pub(crate) migrated_from: Option, /// Source URL of the different-source bundle that already holds the unqualified name. pub(crate) same_name_other_source: Option, } @@ -216,15 +216,21 @@ impl BundleStore { let mut resolved = ResolvedBundleName { name: base.clone(), qualified_from: None, - migrated_from: None, same_name_other_source: None, }; - if let Some(other_source) = self.source_of_other_bundle(&base, &canonical) { + let reserved = AssetCategory::parse(&base).is_some(); + let collision = self.source_of_other_bundle(&base, &canonical); + if reserved || collision.is_some() { + let reason = match &collision { + Some(other_source) => { + format!("is already used by an install from '{other_source}'") + } + None => "is reserved for an asset category".to_string(), + }; if base.contains('/') { bail!( - "bundle name '{base}' is already used by an install from \ - '{other_source}' and cannot be qualified further; \ + "bundle name '{base}' {reason} and cannot be qualified further; \ uninstall it or pick a different manifest name" ); } @@ -233,21 +239,37 @@ impl BundleStore { .filter(|owner| !owner.is_empty()); let Some(owner) = owner else { bail!( - "bundle name '{base}' is already used by an install from \ - '{other_source}', and no owner qualifier can be derived from '{url}'" + "bundle name '{base}' {reason}, and no owner qualifier \ + can be derived from '{url}'" ); }; let qualified = format!("{owner}/{base}"); if let Some(source) = self.source_of_other_bundle(&qualified, &canonical) { bail!( - "bundle names '{base}' and '{qualified}' are both used by installs \ - from other sources ('{other_source}', '{source}'); \ + "bundle name '{base}' {reason}, and '{qualified}' is already \ + used by an install from '{source}'; \ uninstall one or pick a different manifest name" ); } + if let Some(other_source) = &collision + && manifest_name.is_some() + && *IS_STDOUT_TERMINAL + { + let proceed = Confirm::new(&format!( + "Bundle name '{base}' is already used by an install from \ + '{other_source}' (a fork or typo-squat?). Track this install \ + as '{qualified}'?" + )) + .with_default(false) + .prompt() + .with_context(|| "failed to read bundle name confirmation")?; + if !proceed { + bail!("install aborted: bundle name '{base}' {reason}"); + } + } resolved.name = qualified; resolved.qualified_from = Some(base); - resolved.same_name_other_source = Some(other_source); + resolved.same_name_other_source = collision; } let already_recorded = existing_key.as_deref() == Some(resolved.name.as_str()); @@ -263,18 +285,16 @@ impl BundleStore { "Bundle '{old_key}' from {url} is now tracked as '{}'.", resolved.name ); - resolved.migrated_from = Some(old_key); self.save()?; } if let (Some(from), false) = (&resolved.qualified_from, already_recorded) { - let other = resolved - .same_name_other_source - .as_deref() - .unwrap_or_default(); + let detail = match resolved.same_name_other_source.as_deref() { + Some(other) => format!("is already used by an install from '{other}'"), + None => "is reserved for an asset category".to_string(), + }; println!( - "Bundle name '{from}' is already used by an install from '{other}'; \ - tracking this install as '{}'.", + "Bundle name '{from}' {detail}; tracking this install as '{}'.", resolved.name ); } @@ -877,7 +897,6 @@ mod tests { .unwrap(); assert_eq!(resolved.name, "omc"); - assert_eq!(resolved.migrated_from, None); assert_eq!(resolved.qualified_from, None); } @@ -897,7 +916,6 @@ mod tests { .unwrap(); assert_eq!(resolved.name, "oh-my-coyote"); - assert_eq!(resolved.migrated_from.as_deref(), Some("omc")); let reloaded = dir.store(); assert!(reloaded.get("omc").is_none()); assert_eq!(reloaded.get("oh-my-coyote").unwrap().files.len(), 1); @@ -955,7 +973,7 @@ mod tests { .unwrap(); assert_eq!(resolved.name, "b/repo"); - assert_eq!(resolved.migrated_from, None); + assert_eq!(resolved.qualified_from.as_deref(), Some("repo")); } #[test] @@ -970,6 +988,27 @@ mod tests { assert_eq!(resolved.name, "next-js"); } + #[test] + fn resolve_reserves_asset_category_names() { + let dir = TempStoreDir::new("bundles-reserved"); + let mut store = dir.store(); + + let resolved = store + .resolve_bundle_name("https://github.com/x/agents", None) + .unwrap(); + + assert_eq!(resolved.name, "x/agents"); + assert_eq!(resolved.qualified_from.as_deref(), Some("agents")); + assert!(resolved.same_name_other_source.is_none()); + + let resolved = store + .resolve_bundle_name("https://github.com/y/repo", Some("mcp_config")) + .unwrap(); + + assert_eq!(resolved.name, "y/mcp_config"); + assert_eq!(resolved.qualified_from.as_deref(), Some("mcp_config")); + } + #[test] fn resolve_rejects_invalid_manifest_names() { let dir = TempStoreDir::new("bundles-invalid-name"); diff --git a/src/config/install_remote.rs b/src/config/install_remote.rs index fd6a912..65e9b10 100644 --- a/src/config/install_remote.rs +++ b/src/config/install_remote.rs @@ -8,14 +8,14 @@ use crate::function::Language; use crate::mcp::{McpServer, McpServersConfig}; use crate::utils; use crate::utils::IS_STDOUT_TERMINAL; -use crate::vault::{Vault, create_vault_password_file, interpolate_secrets}; +use crate::vault::{SECRET_RE, Vault, create_vault_password_file, interpolate_secrets}; use anyhow::{Context, Result, anyhow, bail}; use clap::ValueEnum; use indexmap::IndexMap; use indoc::formatdoc; use inquire::{Confirm, Select}; use serde::Deserialize; -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeSet, HashMap, HashSet}; use std::ffi::{OsStr, OsString}; use std::fs; use std::path::{Component, Path, PathBuf}; @@ -472,6 +472,7 @@ struct UninstallFileSummary { struct UninstallMcpSummary { removed: Vec, kept: Vec, + secrets: Vec, } /// Kept and failed items stay in the record, so a re-run offers them again. @@ -569,6 +570,13 @@ pub fn uninstall_bundle(spec: &str, assume_yes: bool) -> Result<()> { if !mcp.kept.is_empty() { println!(" = kept servers: {}", mcp.kept.join(", ")); } + if !mcp.secrets.is_empty() { + println!( + " ~ vault secrets referenced by this bundle's servers \ + (installed by this bundle, not removed): {}", + mcp.secrets.join(", ") + ); + } Ok(()) } @@ -787,6 +795,24 @@ fn uninstall_mcp_entries( None }; + let mut secret_names: BTreeSet = BTreeSet::new(); + for server in servers { + if let Some(entry) = config + .as_ref() + .and_then(|cfg| cfg.mcp_servers.get(server.effective_key())) + && let Ok(serialized) = serde_json::to_string(entry) + { + for capture in SECRET_RE.captures_iter(&serialized) { + if let Ok(capture) = capture + && let Some(name) = capture.get(1) + { + secret_names.insert(name.as_str().to_string()); + } + } + } + } + summary.secrets = secret_names.into_iter().collect(); + let mut changed = false; let mut released = Vec::new(); let mut sticky: Option = None; @@ -4265,6 +4291,46 @@ mod tests { let _ = fs::remove_dir_all(&dir); } + #[test] + fn uninstall_mcp_reports_referenced_secrets_without_removing_them() { + let dir = fresh_temp_dir("uninst-mcp-secrets-"); + let mut store = BundleStore::load_from(dir.join("installed-bundles.yaml")).unwrap(); + store + .upsert_bundle("omc", test_metadata("https://github.com/x/omc")) + .unwrap(); + let mcp = dir.join("mcp.json"); + write_mcp( + &mcp, + r#"{"mcpServers": { + "srv": { + "type": "stdio", + "command": "echo", + "env": {"TOKEN": "{{OMC_TOKEN}}", "ORG": "{{OMC_ORG}}"} + } + }}"#, + ); + let parsed: McpServersConfig = + serde_json::from_str(&fs::read_to_string(&mcp).unwrap()).unwrap(); + let hash = hash_bytes( + serde_json::to_string(parsed.mcp_servers.get("srv").unwrap()) + .unwrap() + .as_bytes(), + ); + store + .record_mcp_servers( + "omc", + vec![mcp_server_record("srv", McpAction::Added, Some(hash))], + ) + .unwrap(); + let servers = store.get("omc").unwrap().mcp_servers.clone(); + + let summary = uninstall_mcp_entries(&mut store, "omc", &servers, &mcp, true).unwrap(); + + assert_eq!(summary.removed, vec!["srv"]); + assert_eq!(summary.secrets, vec!["OMC_ORG", "OMC_TOKEN"]); + let _ = fs::remove_dir_all(&dir); + } + #[test] fn uninstall_mcp_keeps_modified_entries_and_their_records() { let dir = fresh_temp_dir("uninst-mcp-modified-");