diff --git a/README.md b/README.md index 02f2901..330b01e 100644 --- a/README.md +++ b/README.md @@ -23,7 +23,7 @@ Coming from [AIChat](https://github.com/sigoden/aichat)? Follow the [migration g * [AIChat Migration Guide](https://github.com/Dark-Alex-17/coyote/wiki/AIChat-Migration): Coming from AIChat? Follow the migration guide to get started. * [Installation](#install): Install Coyote * [Getting Started](#getting-started): Get started with Coyote by doing first-run setup steps. -* [Sharing Configurations](https://github.com/Dark-Alex-17/coyote/wiki/Sharing-Configurations): Install bundles of agents, roles, macros, tools, and MCP servers from any git repo, and share your own. Bundles are Coyote's equivalent of plugins in other CLI agents. +* [Sharing Configurations](https://github.com/Dark-Alex-17/coyote/wiki/Sharing-Configurations): Install bundles of agents, roles, skills, macros, tools, and MCP servers from any git repo, and share your own. Bundles are Coyote's equivalent of plugins in other CLI agents. * [REPL](https://github.com/Dark-Alex-17/coyote/wiki/REPL): Interactive Read-Eval-Print Loop for conversational interactions with LLMs and Coyote. * [Custom REPL Prompt](https://github.com/Dark-Alex-17/coyote/wiki/REPL-Prompt): Customize the REPL prompt to provide useful contextual information. * [Vault](https://github.com/Dark-Alex-17/coyote/wiki/Vault): Securely store and manage sensitive information such as API keys and credentials. diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 91d66e2..6cc9c67 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -180,27 +180,37 @@ pub struct Cli { #[arg(long, help_heading = "List & Discovery")] pub list_bundles: bool, - /// Install assets from a remote git repository (URL may be suffixed with #), or update an already-installed bundle by name + /// Install assets from a remote git repository (a URL or / shorthand, optionally suffixed with #), or update an already-installed bundle by name #[arg( long, - value_name = "GIT_URL|NAME", + value_name = "GIT_URL|OWNER/REPO|NAME", + conflicts_with_all = ["install_builtins", "update_bundle", "uninstall"], help_heading = "Installation & Updates" )] pub install: Option, - /// Git host used to expand / shorthand values passed to --install + /// Git host used to expand / shorthand values passed to --install (also forces the value to be treated as a source when it matches an installed bundle name) #[arg( long, value_name = "HOST", requires = "install", + conflicts_with_all = ["install_builtins", "update_bundle", "uninstall"], help_heading = "Installation & Updates" )] pub git_host: Option, - /// Reinstall bundled assets, overwriting any local changes + /// Removed; use --install instead + #[arg( + long, + hide = true, + value_name = "GIT_URL", + help_heading = "Installation & Updates" + )] + pub install_from: Option, + /// Reinstall bundled assets for a category (asks before overwriting your local changes) #[arg( long, value_name = "CATEGORY", value_enum, - conflicts_with_all = ["install"], + conflicts_with_all = ["update_bundle", "uninstall"], help_heading = "Installation & Updates" )] pub install_builtins: Option, @@ -210,20 +220,36 @@ pub struct Cli { value_name = "CATEGORY", value_enum, requires = "install", + conflicts_with_all = ["install_builtins", "update_bundle", "uninstall"], help_heading = "Installation & Updates" )] pub filter: Option, /// Overwrite all conflicts without prompting (used with --install) - #[arg(long, requires = "install", help_heading = "Installation & Updates")] + #[arg( + long, + requires = "install", + conflicts_with_all = ["install_builtins", "update_bundle", "uninstall"], + help_heading = "Installation & Updates" + )] pub install_force: bool, /// Update an installed bundle from its recorded source (NAME may be suffixed with # to move a pin) - #[arg(long, value_name = "NAME", help_heading = "Installation & Updates")] + #[arg( + long, + value_name = "NAME", + conflicts_with_all = ["uninstall"], + help_heading = "Installation & Updates" + )] pub update_bundle: Option, /// Uninstall a bundle: delete its owned files and remove its mcp.json entries #[arg(long, value_name = "NAME", help_heading = "Installation & Updates")] pub uninstall: Option, /// Skip uninstall confirmation prompts (locally modified items are still kept) - #[arg(long, requires = "uninstall", help_heading = "Installation & Updates")] + #[arg( + long, + requires = "uninstall", + conflicts_with_all = ["install", "install_builtins", "update_bundle"], + help_heading = "Installation & Updates" + )] pub yes: bool, /// Sync models updates #[arg(long, help_heading = "Installation & Updates")] @@ -572,10 +598,11 @@ mod tests { } #[test] - fn parse_install_from_is_no_longer_a_flag() { + fn parse_install_from_is_a_tombstone() { let cli = parse(&["--install-from", "https://github.com/x/y"]); assert!(cli.install.is_none()); - assert_eq!(cli.text, vec!["--install-from", "https://github.com/x/y"]); + assert_eq!(cli.install_from.as_deref(), Some("https://github.com/x/y")); + assert!(cli.text.is_empty()); } #[test] @@ -586,6 +613,28 @@ mod tests { ); } + #[test] + fn parse_lifecycle_flags_are_mutually_exclusive() { + assert!(Cli::try_parse_from(["coyote", "--install", "x", "--uninstall", "y"]).is_err()); + assert!( + Cli::try_parse_from(["coyote", "--update-bundle", "x", "--uninstall", "y"]).is_err() + ); + assert!( + Cli::try_parse_from(["coyote", "--install-builtins", "agents", "--uninstall", "y"]) + .is_err() + ); + assert!(Cli::try_parse_from(["coyote", "--install", "x", "--update-bundle", "y"]).is_err()); + } + + #[test] + fn parse_companion_flags_conflict_with_other_lifecycle_actions() { + assert!( + Cli::try_parse_from(["coyote", "--update-bundle", "x", "--filter", "agents"]).is_err() + ); + assert!(Cli::try_parse_from(["coyote", "--uninstall", "x", "--install-force"]).is_err()); + assert!(Cli::try_parse_from(["coyote", "--install", "x", "--yes"]).is_err()); + } + #[test] fn parse_filter_requires_install() { assert!(Cli::try_parse_from(["coyote", "--filter", "agents"]).is_err()); @@ -604,6 +653,10 @@ mod tests { #[test] fn parse_git_host_requires_install() { assert!(Cli::try_parse_from(["coyote", "--git-host", "git.x.com"]).is_err()); + assert!( + Cli::try_parse_from(["coyote", "--git-host", "gitlab.com", "--update-bundle", "x"]) + .is_err() + ); assert_eq!( parse(&["--install", "someuser/omc", "--git-host", "git.x.com"]) .git_host diff --git a/src/config/bundles.rs b/src/config/bundles.rs index aefd4f2..da51ebd 100644 --- a/src/config/bundles.rs +++ b/src/config/bundles.rs @@ -45,6 +45,7 @@ pub(crate) struct FileRecord { pub(crate) struct McpServerRecord { pub(crate) name: String, pub(crate) action: McpAction, + #[serde(default, skip_serializing_if = "Option::is_none")] pub(crate) renamed_to: Option, /// Hash of the mcp.json entry as written; a later mismatch means the user /// modified it. Absent on records made before entry hashing existed. @@ -299,6 +300,10 @@ impl BundleStore { ); } + debug_assert!( + validate_bundle_name(&resolved.name).is_ok(), + "derived bundle names must satisfy validate_bundle_name" + ); Ok(resolved) } @@ -391,8 +396,10 @@ impl BundleStore { } /// An entry whose key any bundle already owns transfers to `bundle`, and - /// its `replaced` action upgrades to `transferred`; plain `replaced` - /// marks a pre-existing user entry that uninstall must never delete. + /// its `replaced` action upgrades to `transferred` only when the prior + /// record proves bundle origin. A prior `replaced` record marks a + /// pre-existing user entry that uninstall must never delete, and that + /// marker survives re-records and cross-bundle transfers. pub(crate) fn record_mcp_servers( &mut self, bundle: &str, @@ -402,14 +409,21 @@ impl BundleStore { for mut entry in entries { let key = entry.effective_key().to_string(); let mut previously_owned = false; + let mut prior_user_origin = false; for record in self.bundles.values_mut() { let before = record.mcp_servers.len(); - record - .mcp_servers - .retain(|owned| owned.effective_key() != key); + record.mcp_servers.retain(|owned| { + if owned.effective_key() != key { + return true; + } + if owned.action == McpAction::Replaced { + prior_user_origin = true; + } + false + }); previously_owned |= record.mcp_servers.len() != before; } - if previously_owned && entry.action == McpAction::Replaced { + if previously_owned && !prior_user_origin && entry.action == McpAction::Replaced { entry.action = McpAction::Transferred; } self.bundles @@ -852,6 +866,64 @@ mod tests { assert_eq!(servers[0].sha256.as_deref(), Some("deadbeef")); } + #[test] + fn mcp_rerecord_of_own_replaced_entry_stays_replaced() { + let dir = TempStoreDir::new("bundles-mcp-sticky-self"); + let mut store = dir.store(); + store + .upsert_bundle("omc", metadata("https://github.com/x/omc", "abc123")) + .unwrap(); + store + .record_mcp_servers( + "omc", + vec![mcp_record("user-srv", McpAction::Replaced, None)], + ) + .unwrap(); + + store + .record_mcp_servers( + "omc", + vec![mcp_record("user-srv", McpAction::Replaced, None)], + ) + .unwrap(); + + assert_eq!( + store.get("omc").unwrap().mcp_servers[0].action, + McpAction::Replaced + ); + } + + #[test] + fn mcp_transfer_of_replaced_entry_keeps_user_origin_marker() { + let dir = TempStoreDir::new("bundles-mcp-sticky-transfer"); + let mut store = dir.store(); + store + .upsert_bundle("alpha", metadata("https://github.com/a/alpha", "abc123")) + .unwrap(); + store + .upsert_bundle("beta", metadata("https://github.com/b/beta", "def456")) + .unwrap(); + store + .record_mcp_servers( + "alpha", + vec![mcp_record("user-srv", McpAction::Replaced, None)], + ) + .unwrap(); + + store + .record_mcp_servers( + "beta", + vec![mcp_record("user-srv", McpAction::Replaced, None)], + ) + .unwrap(); + + assert!(store.get("alpha").unwrap().mcp_servers.is_empty()); + assert_eq!( + store.get("beta").unwrap().mcp_servers[0].action, + McpAction::Replaced + ); + } + #[test] fn mcp_transfer_matches_renamed_entries_by_effective_key() { let dir = TempStoreDir::new("bundles-mcp-renamed"); @@ -893,7 +965,7 @@ mod tests { .unwrap(); let resolved = store - .resolve_bundle_name("git@github.com:X/omc.git", None) + .resolve_bundle_name("git@github.com:x/omc.git", None) .unwrap(); assert_eq!(resolved.name, "omc"); diff --git a/src/config/install_remote.rs b/src/config/install_remote.rs index 65e9b10..8529fa6 100644 --- a/src/config/install_remote.rs +++ b/src/config/install_remote.rs @@ -151,7 +151,7 @@ fn looks_like_remote_source(value: &str) -> bool { } } -const DEFAULT_GIT_HOST: &str = "github.com"; +pub(crate) const DEFAULT_GIT_HOST: &str = "github.com"; fn is_repo_shorthand(value: &str) -> bool { let path = strip_ref_suffix(value); @@ -174,7 +174,7 @@ fn expand_repo_shorthand(value: &str, git_host: Option<&str>) -> Result .or_else(|| raw.strip_prefix("http://")) .unwrap_or(raw) .trim_matches('/'); - if host.is_empty() || host.contains('/') || host.chars().any(char::is_whitespace) { + if host.is_empty() || host.contains(['/', '#', '?']) || host.chars().any(char::is_whitespace) { bail!("invalid --git-host '{raw}': expected a bare host like git.somedomain.com"); } Ok(format!("https://{host}/{value}")) @@ -951,12 +951,18 @@ fn clone_to_temp(url: &str, reference: Option<&str>) -> Result { "1".into(), "--branch".into(), r.into(), + "--".into(), url.into(), dest_arg, ])?; } Some(r) => { - run_git(vec!["clone".into(), url.into(), dest_arg.clone()])?; + run_git(vec![ + "clone".into(), + "--".into(), + url.into(), + dest_arg.clone(), + ])?; run_git(vec!["-C".into(), dest_arg, "checkout".into(), r.into()])?; } None => { @@ -964,6 +970,7 @@ fn clone_to_temp(url: &str, reference: Option<&str>) -> Result { "clone".into(), "--depth".into(), "1".into(), + "--".into(), url.into(), dest_arg, ])?; @@ -1223,14 +1230,17 @@ fn sanitize_host(host: &str) -> String { .to_string() } +/// The host compares case-insensitively but the path keeps its case: many +/// self-hosted forges treat repository paths as case-sensitive, and collapsing +/// distinct repos into one record misdirects updates and uninstalls. pub(crate) fn canonical_source_url(url: &str) -> String { let (host, path) = split_host_and_path(url); - let mut path = path.to_ascii_lowercase(); - if let Some(stripped) = path.strip_suffix(".git") - && !stripped.is_empty() - && !stripped.ends_with('/') - { - path = stripped.to_string(); + let mut path = path; + if path.to_ascii_lowercase().ends_with(".git") { + let stripped = &path[..path.len() - 4]; + if !stripped.is_empty() && !stripped.ends_with('/') { + path.truncate(path.len() - 4); + } } let host = host.to_ascii_lowercase(); if host.is_empty() { @@ -2837,10 +2847,14 @@ mod tests { } #[test] - fn canonical_source_url_lowercases_host_and_path() { + fn canonical_source_url_lowercases_host_but_not_path() { assert_eq!( canonical_source_url("https://GitHub.COM/X/R.git"), - "github.com/x/r" + "github.com/X/R" + ); + assert_ne!( + canonical_source_url("https://gitlab.example.com/team/Repo"), + canonical_source_url("https://gitlab.example.com/team/repo") ); } diff --git a/src/config/request_context.rs b/src/config/request_context.rs index e255ec9..0b75021 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -3501,7 +3501,9 @@ impl RequestContext { InstallFilter::NAMES.iter().map(|s| s.to_string()).collect(), ); } else if prev == "--git-host" { - values = super::map_completion_values(vec!["github.com".to_string()]); + values = super::map_completion_values(vec![ + super::install_remote::DEFAULT_GIT_HOST.to_string(), + ]); } else { let has_filter = args.iter().enumerate().any(|(i, a)| { a.starts_with("--filter=") || (*a == "--filter" && i < args.len() - 1) diff --git a/src/function/mod.rs b/src/function/mod.rs index fc8d7cb..7c0b81e 100644 --- a/src/function/mod.rs +++ b/src/function/mod.rs @@ -165,7 +165,16 @@ pub(crate) fn write_file_atomic( std::process::id(), TMP_COUNTER.fetch_add(1, Ordering::Relaxed) )); - fs::write(&tmp, content)?; + let write_synced = || -> std::io::Result<()> { + use std::io::Write; + let mut file = fs::File::create(&tmp)?; + file.write_all(content.as_bytes())?; + file.sync_all() + }; + if let Err(err) = write_synced() { + let _ = fs::remove_file(&tmp); + return Err(err.into()); + } #[cfg(unix)] if let Some(mode) = mode { diff --git a/src/main.rs b/src/main.rs index eb2b06d..4befadc 100644 --- a/src/main.rs +++ b/src/main.rs @@ -107,7 +107,6 @@ async fn main() -> Result<()> { || cli.list_rags || cli.list_macros || cli.list_skills - || cli.list_bundles || cli.list_sessions; let vault_flags = cli.add_secret.is_some() || cli.get_secret.is_some() @@ -128,6 +127,10 @@ async fn main() -> Result<()> { return sandbox::launch(name.clone(), cli.fresh); } + if cli.install_from.is_some() { + bail!("--install-from was removed; use --install instead"); + } + install_builtins()?; if let Some(category) = cli.install_builtins { @@ -151,6 +154,10 @@ async fn main() -> Result<()> { return config::uninstall_bundle(name, cli.yes); } + if cli.list_bundles { + return config::list_installed_bundles(); + } + if let Some(client_arg) = &cli.authenticate { let cfg = Config::load_with_interpolation(true).await?; let app_config = AppConfig::from_config(cfg)?; @@ -323,9 +330,6 @@ async fn run( println!("{skills}"); return Ok(()); } - if cli.list_bundles { - return config::list_installed_bundles(); - } let skills = cli.skills(); if skills.len() == 1 { let name = &skills[0]; diff --git a/src/repl/mod.rs b/src/repl/mod.rs index 4e48529..32f9ef7 100644 --- a/src/repl/mod.rs +++ b/src/repl/mod.rs @@ -875,6 +875,9 @@ pub async fn run_repl_command( ReplInstallDispatch::Unified(value) => { config::install_or_update_from_repl_args(value)?; } + ReplInstallDispatch::RemovedRemote => println!( + "'.install remote ' was removed; use '.install ' directly." + ), ReplInstallDispatch::Usage => println!( "Usage: .install <{}> | .install \ [--git-host ] [--filter ] [--force]", @@ -1197,12 +1200,20 @@ pub async fn run_repl_command( println!("Usage: .delete ") } }, - ".uninstall" => match args { - Some(args) => { - config::uninstall_bundle(args.trim(), false)?; + ".uninstall" => { + let mut assume_yes = false; + let mut names = Vec::new(); + for token in args.unwrap_or("").split_whitespace() { + match token { + "--yes" | "-y" => assume_yes = true, + other => names.push(other), + } } - _ => println!("Usage: .uninstall "), - }, + match names.as_slice() { + [name] => config::uninstall_bundle(name, assume_yes)?, + _ => println!("Usage: .uninstall [--yes]"), + } + } ".list" => match args { Some(args) => { ctx.list_assets(args.trim())?; @@ -1575,6 +1586,7 @@ fn unknown_command() -> Result<()> { enum ReplInstallDispatch<'a> { Builtins(AssetCategory), Unified(&'a str), + RemovedRemote, Usage, } @@ -1582,10 +1594,15 @@ fn parse_repl_install(args: Option<&str>) -> ReplInstallDispatch<'_> { let trimmed = args.map(str::trim).unwrap_or(""); let mut parts = trimmed.splitn(2, char::is_whitespace); match parts.next() { - Some(name) if !name.is_empty() => match AssetCategory::parse(name) { - Some(category) => ReplInstallDispatch::Builtins(category), - None => ReplInstallDispatch::Unified(trimmed), - }, + Some(name) if !name.is_empty() => { + let rest = parts.next().map(str::trim).unwrap_or(""); + match AssetCategory::parse(name) { + Some(category) if rest.is_empty() => ReplInstallDispatch::Builtins(category), + Some(_) => ReplInstallDispatch::Usage, + None if name == "remote" && !rest.is_empty() => ReplInstallDispatch::RemovedRemote, + None => ReplInstallDispatch::Unified(trimmed), + } + } _ => ReplInstallDispatch::Usage, } } @@ -1841,6 +1858,30 @@ mod tests { assert_eq!(parse_repl_install(Some(" ")), ReplInstallDispatch::Usage); } + #[test] + fn parse_repl_install_rejects_extra_tokens_after_a_category() { + assert_eq!( + parse_repl_install(Some("agents --force")), + ReplInstallDispatch::Usage + ); + assert_eq!( + parse_repl_install(Some("agents extra")), + ReplInstallDispatch::Usage + ); + } + + #[test] + fn parse_repl_install_hints_on_removed_remote_form() { + assert_eq!( + parse_repl_install(Some("remote https://github.com/x/y")), + ReplInstallDispatch::RemovedRemote + ); + assert_eq!( + parse_repl_install(Some("remote")), + ReplInstallDispatch::Unified("remote") + ); + } + #[test] fn builtin_command_names_are_sorted_deduped_first_words_without_dots() { let names = builtin_command_names();