diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 2716ecc..0c87db5 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -50,7 +50,7 @@ pub enum McpScopeArg { "model", "prompt", "role", "session", "agent", "rag", "rebuild_rag", "macro_name", "execute", "code", "file", "no_stream", "no_memory", "init_memory", "dry_run", "info", "build_tools", "install", - "install_from", "sync_models", "list_models", "list_roles", + "install_from", "install_builtins", "sync_models", "list_models", "list_roles", "list_sessions", "list_agents", "list_rags", "list_macros", "list_skills", "list_bundles", "skill", "tail_logs", "completions", "update", "update_bundle", "uninstall", @@ -61,6 +61,11 @@ pub enum McpScopeArg { .args(["mcp_add", "mcp_remove", "mcp_list", "mcp_get"]) .multiple(false) ), + group( + ArgGroup::new("remote-install") + .args(["install", "install_from"]) + .multiple(false) + ), )] pub struct Cli { /// Input text @@ -180,30 +185,43 @@ 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 + #[arg( + long, + value_name = "GIT_URL|NAME", + help_heading = "Installation & Updates" + )] + pub install: Option, /// Reinstall bundled assets, overwriting any local changes #[arg( long, value_name = "CATEGORY", value_enum, + conflicts_with_all = ["install", "install_from"], help_heading = "Installation & Updates" )] - pub install: Option, + pub install_builtins: Option, /// Install assets from a remote git repository (URL may be suffixed with #) - #[arg(long, value_name = "GIT_URL", help_heading = "Installation & Updates")] + #[arg( + long, + value_name = "GIT_URL", + hide = true, + help_heading = "Installation & Updates" + )] pub install_from: Option, - /// Restrict --install-from to a single asset category + /// Restrict a remote install to a single asset category #[arg( long, value_name = "CATEGORY", value_enum, - requires = "install_from", + requires = "remote-install", help_heading = "Installation & Updates" )] pub filter: Option, - /// Overwrite all conflicts without prompting (used with --install-from) + /// Overwrite all conflicts without prompting (used with --install) #[arg( long, - requires = "install_from", + requires = "remote-install", help_heading = "Installation & Updates" )] pub install_force: bool, @@ -540,6 +558,94 @@ mod tests { assert!(Cli::try_parse_from(["coyote", "--yes"]).is_err()); } + #[test] + fn parse_install_flag_takes_url_or_name() { + assert_eq!( + parse(&["--install", "https://github.com/x/y"]) + .install + .as_deref(), + Some("https://github.com/x/y") + ); + } + + #[test] + fn parse_install_builtins_flag_takes_category() { + assert_eq!( + parse(&["--install-builtins", "agents"]).install_builtins, + Some(AssetCategory::Agents) + ); + assert_eq!( + parse(&["--install-builtins", "mcp_config"]).install_builtins, + Some(AssetCategory::McpConfig) + ); + } + + #[test] + fn parse_install_from_flag_still_works() { + assert_eq!( + parse(&["--install-from", "https://github.com/x/y"]) + .install_from + .as_deref(), + Some("https://github.com/x/y") + ); + } + + #[test] + fn parse_install_conflicts_with_install_from() { + assert!(Cli::try_parse_from(["coyote", "--install", "x", "--install-from", "y"]).is_err()); + } + + #[test] + fn parse_install_builtins_conflicts_with_remote_install_flags() { + assert!( + Cli::try_parse_from(["coyote", "--install-builtins", "agents", "--install", "x"]) + .is_err() + ); + assert!( + Cli::try_parse_from([ + "coyote", + "--install-builtins", + "agents", + "--install-from", + "y" + ]) + .is_err() + ); + } + + #[test] + fn parse_filter_requires_a_remote_install_flag() { + assert!(Cli::try_parse_from(["coyote", "--filter", "agents"]).is_err()); + assert_eq!( + parse(&["--install", "https://github.com/x/y", "--filter", "agents"]).filter, + Some(InstallFilter::Agents) + ); + assert_eq!( + parse(&[ + "--install-from", + "https://github.com/x/y", + "--filter", + "agents" + ]) + .filter, + Some(InstallFilter::Agents) + ); + } + + #[test] + fn parse_install_force_requires_a_remote_install_flag() { + assert!(Cli::try_parse_from(["coyote", "--install-force"]).is_err()); + assert!(parse(&["--install", "https://github.com/x/y", "--install-force"]).install_force); + } + + #[test] + fn help_hides_install_from_and_shows_install_builtins() { + use clap::CommandFactory; + let help = Cli::command().render_long_help().to_string(); + assert!(!help.contains("--install-from"), "help: {help}"); + assert!(help.contains("--install-builtins")); + } + #[test] fn parse_multiple_skill_flags_preserves_order() { assert_eq!( diff --git a/src/config/bundles.rs b/src/config/bundles.rs index 0c6d1b8..fc3b211 100644 --- a/src/config/bundles.rs +++ b/src/config/bundles.rs @@ -542,7 +542,7 @@ pub fn list_installed_bundles() -> Result<()> { let store = BundleStore::load()?; let rows = bundle_list_rows(&store, &paths::config_dir()); if rows.is_empty() { - println!("No bundles installed. Install one with `coyote --install-from `."); + println!("No bundles installed. Install one with `coyote --install `."); return Ok(()); } diff --git a/src/config/install_remote.rs b/src/config/install_remote.rs index 1eb28c8..9bf6a84 100644 --- a/src/config/install_remote.rs +++ b/src/config/install_remote.rs @@ -2,7 +2,7 @@ use super::bundles::{ BundleStore, FileAction, FileRecord, InstallMetadata, McpAction, McpServerRecord, hash_bytes, hash_file, }; -use crate::config::{InstallFilter, paths}; +use crate::config::{AssetCategory, InstallFilter, paths}; #[cfg(not(windows))] use crate::function::Language; use crate::mcp::{McpServer, McpServersConfig}; @@ -10,6 +10,7 @@ use crate::utils; use crate::utils::IS_STDOUT_TERMINAL; use crate::vault::{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}; @@ -80,6 +81,14 @@ pub fn install_remote_from_repl_args(args: &str) -> Result<()> { ) })?; + let (filter, force) = parse_repl_install_flags(".install remote", iter)?; + install_remote(&url, filter, force) +} + +fn parse_repl_install_flags( + command: &str, + mut iter: impl Iterator, +) -> Result<(Option, bool)> { let mut filter: Option = None; let mut force = false; @@ -98,11 +107,117 @@ pub fn install_remote_from_repl_args(args: &str) -> Result<()> { s if s.starts_with("--filter=") => { filter = Some(parse_filter(&s["--filter=".len()..])?); } - other => bail!("Unexpected argument to '.install remote': {other}"), + other => bail!("Unexpected argument to '{command}': {other}"), } } - install_remote(&url, filter, force) + Ok((filter, force)) +} + +#[derive(Debug, Clone, PartialEq)] +enum InstallTarget { + Category(AssetCategory), + InstalledBundle, + RemoteSource, + Unknown, +} + +fn classify_install_target(value: &str, installed_names: &[String]) -> InstallTarget { + if let Some(category) = AssetCategory::parse(value) { + return InstallTarget::Category(category); + } + let name = strip_ref_suffix(value); + if installed_names.iter().any(|installed| installed == name) { + return InstallTarget::InstalledBundle; + } + if looks_like_remote_source(value) { + return InstallTarget::RemoteSource; + } + InstallTarget::Unknown +} + +/// A value is treated as a source when it is a URL, an scp-style +/// `[user@]host:path`, or an explicit local path. Bare names never are: they +/// must match an installed bundle. +fn looks_like_remote_source(value: &str) -> bool { + if value.contains("://") + || value.starts_with("./") + || value.starts_with("../") + || value.starts_with('/') + || value.starts_with('~') + { + return true; + } + match value.split_once(':') { + Some((host, _)) => { + !host.is_empty() && !host.contains('/') && !host.chars().any(char::is_whitespace) + } + None => false, + } +} + +/// Unified entry point behind `--install` and the REPL's `.install `: +/// asset categories are redirected to `--install-builtins`, installed bundle +/// names become updates, and anything shaped like a source is installed. +pub fn install_or_update(value: &str, filter: Option, force: bool) -> Result<()> { + let store = BundleStore::load()?; + let installed: Vec = store + .bundle_names() + .into_iter() + .map(str::to_string) + .collect(); + + match classify_install_target(value, &installed) { + InstallTarget::Category(category) => { + let name = category + .to_possible_value() + .map_or_else(|| value.to_string(), |v| v.get_name().to_string()); + let update_hint = if installed.iter().any(|installed| installed == value) { + format!(" To update the installed bundle '{value}', use `--update-bundle {value}`.") + } else { + String::new() + }; + bail!( + "'{value}' is an asset category, not a bundle; did you mean \ + `--install-builtins {name}`? (categories are reinstalled from \ + the assets built into coyote, not from a remote){update_hint}" + ); + } + InstallTarget::InstalledBundle => { + if filter.is_some() || force { + bail!("--filter/--install-force only apply to remote installs, not bundle updates"); + } + update_bundle(value) + } + InstallTarget::RemoteSource => install_remote(value, filter, force), + InstallTarget::Unknown => { + let hint = "a remote source must be a git URL, an scp-style host:path, \ + or an explicit local path (./dir, /abs, ~)"; + if installed.is_empty() { + bail!("no bundle named '{value}' is installed; none are installed ({hint})"); + } + bail!( + "no bundle named '{value}' is installed; installed bundles: {} ({hint})", + installed.join(", ") + ) + } + } +} + +pub fn install_or_update_from_repl_args(args: &str) -> Result<()> { + let tokens = shell_words::split(args) + .with_context(|| format!("failed to parse '.install' args: {args}"))?; + + let mut iter = tokens.into_iter(); + let value = iter.next().with_context(|| { + format!( + "Usage: .install [--filter <{}>] [--force]", + InstallFilter::NAMES.join("|") + ) + })?; + + let (filter, force) = parse_repl_install_flags(".install", iter)?; + install_or_update(&value, filter, force) } /// Update an installed bundle from its recorded source. `spec` is the bundle @@ -3461,6 +3576,160 @@ mod tests { let _ = fs::remove_dir_all(&src_root); } + fn owned_names(names: &[&str]) -> Vec { + names.iter().map(|s| s.to_string()).collect() + } + + #[test] + fn classify_urls_and_paths_as_remote_sources() { + for value in [ + "https://github.com/x/y", + "git@github.com:x/y.git", + "./bundle-dir", + "../up", + "/abs/path", + "~/home-rel", + ] { + assert_eq!( + classify_install_target(value, &[]), + InstallTarget::RemoteSource, + "value: {value}" + ); + } + } + + #[test] + fn classify_categories_win_over_installed_names() { + assert_eq!( + classify_install_target("agents", &owned_names(&["agents"])), + InstallTarget::Category(AssetCategory::Agents) + ); + assert_eq!( + classify_install_target("mcp_config", &[]), + InstallTarget::Category(AssetCategory::McpConfig) + ); + } + + #[test] + fn classify_installed_names_with_optional_ref() { + let installed = owned_names(&["omc"]); + assert_eq!( + classify_install_target("omc", &installed), + InstallTarget::InstalledBundle + ); + assert_eq!( + classify_install_target("omc#v2", &installed), + InstallTarget::InstalledBundle + ); + } + + #[test] + fn classify_bare_names_without_a_match_as_unknown() { + assert_eq!(classify_install_target("omc", &[]), InstallTarget::Unknown); + assert_eq!( + classify_install_target("not-a-bundle", &owned_names(&["omc"])), + InstallTarget::Unknown + ); + } + + #[test] + #[serial] + fn install_or_update_redirects_categories_to_install_builtins() { + let _guard = TestVaultConfigGuard::new("iou-category"); + + let err = install_or_update("agents", None, false).unwrap_err(); + + assert!( + err.to_string().contains("--install-builtins agents"), + "got: {err}" + ); + } + + #[test] + #[serial] + fn install_or_update_unknown_name_lists_installed_bundles() { + let _guard = TestVaultConfigGuard::new("iou-unknown"); + + let err = install_or_update("not-a-bundle", None, false).unwrap_err(); + assert!(err.to_string().contains("none are installed"), "got: {err}"); + + let src_root = fresh_temp_dir("iou-unknown-src-"); + let repo = src_root.join("bundle"); + write_src(&repo, BUNDLE_MANIFEST_FILE, "name: known-bundle\n"); + write_src(&repo, "macros/m.yaml", "a: 1\n"); + init_bundle_repo(&repo); + install_remote(repo.to_str().unwrap(), None, false).unwrap(); + + let err = install_or_update("not-a-bundle", None, false).unwrap_err(); + + assert!( + err.to_string().contains("no bundle named 'not-a-bundle'"), + "got: {err}" + ); + assert!( + err.to_string().contains("installed bundles: known-bundle"), + "got: {err}" + ); + let _ = fs::remove_dir_all(&src_root); + } + + #[test] + #[serial] + fn install_or_update_category_error_hints_update_for_shadowed_bundle() { + let _guard = TestVaultConfigGuard::new("iou-shadow"); + + let mut store = BundleStore::load().unwrap(); + store + .upsert_bundle( + "agents", + InstallMetadata { + source: "https://github.com/x/agents".to_string(), + git_ref: None, + commit: "abc123".to_string(), + version: None, + description: None, + homepage: None, + }, + ) + .unwrap(); + + let err = install_or_update("agents", None, false).unwrap_err(); + + assert!( + err.to_string().contains("--install-builtins agents"), + "got: {err}" + ); + assert!( + err.to_string().contains("--update-bundle agents"), + "got: {err}" + ); + } + + #[test] + #[serial] + fn install_or_update_rejects_remote_flags_for_updates() { + let _guard = TestVaultConfigGuard::new("iou-flags"); + let src_root = fresh_temp_dir("iou-flags-src-"); + let repo = src_root.join("bundle"); + write_src(&repo, BUNDLE_MANIFEST_FILE, "name: flag-bundle\n"); + write_src(&repo, "macros/m.yaml", "a: 1\n"); + init_bundle_repo(&repo); + install_remote(repo.to_str().unwrap(), None, false).unwrap(); + + let err = install_or_update("flag-bundle", Some(InstallFilter::Macros), false).unwrap_err(); + assert!( + err.to_string().contains("only apply to remote installs"), + "got: {err}" + ); + + let err = install_or_update("flag-bundle", None, true).unwrap_err(); + assert!( + err.to_string().contains("only apply to remote installs"), + "got: {err}" + ); + let _ = fs::remove_dir_all(&src_root); + } + fn owned_file_record(path: &str, contents: &str) -> FileRecord { FileRecord { path: path.to_string(), diff --git a/src/config/mod.rs b/src/config/mod.rs index e1d822e..a8ceb60 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -33,7 +33,8 @@ pub use self::app_state::AppState; pub use self::bundles::list_installed_bundles; pub use self::input::Input; pub use self::install_remote::{ - install_remote, install_remote_from_repl_args, uninstall_bundle, update_bundle, + install_or_update, install_or_update_from_repl_args, install_remote, + install_remote_from_repl_args, uninstall_bundle, update_bundle, }; pub use self::macro_policy::{ MacroAllowlistLevel, MacroPolicy, MacroSource, MacroState, RESERVED_MACRO_NAMES, ResolvedMacro, diff --git a/src/config/request_context.rs b/src/config/request_context.rs index 9416dce..3b54b5d 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -3264,6 +3264,17 @@ impl RequestContext { let mut values: Vec = AssetCategory::NAMES.iter().map(|s| s.to_string()).collect(); values.push("remote".to_string()); + values.extend( + BundleStore::load() + .map(|store| { + store + .bundle_names() + .into_iter() + .map(str::to_string) + .collect::>() + }) + .unwrap_or_default(), + ); super::map_completion_values(values) } ".uninstall" => { @@ -4979,6 +4990,36 @@ mod tests { ); } + #[test] + #[serial] + fn repl_complete_install_offers_categories_remote_and_bundles() { + let _guard = TestConfigDirGuard::new(); + let mut store = BundleStore::load().unwrap(); + store + .upsert_bundle( + "omc", + crate::config::bundles::InstallMetadata { + source: "https://github.com/x/omc".to_string(), + git_ref: None, + commit: "abc123".to_string(), + version: None, + description: None, + homepage: None, + }, + ) + .unwrap(); + let ctx = create_test_ctx(); + + let values = ctx.repl_complete(".install", &[""], ""); + + for expected in ["agents", "remote", "omc"] { + assert!( + values.iter().any(|(name, _)| name == expected), + "missing '{expected}'; got: {values:?}" + ); + } + } + #[test] #[serial] fn exit_agent_clears_all_agent_state() { diff --git a/src/main.rs b/src/main.rs index 377e211..eb36f2d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -130,11 +130,18 @@ async fn main() -> Result<()> { install_builtins()?; - if let Some(category) = cli.install { + if let Some(category) = cli.install_builtins { return config::install_assets(category); } + if let Some(value) = cli.install.as_deref() { + return config::install_or_update(value, cli.filter, cli.install_force); + } + if let Some(url) = cli.install_from.as_deref() { + eprintln!( + "warning: --install-from is deprecated and will be removed in a future release; use --install " + ); return config::install_remote(url, cli.filter, cli.install_force); } diff --git a/src/repl/mod.rs b/src/repl/mod.rs index dfc209d..f67bfde 100644 --- a/src/repl/mod.rs +++ b/src/repl/mod.rs @@ -317,7 +317,7 @@ static REPL_COMMANDS: LazyLock<[ReplCommand; 61]> = LazyLock::new(|| { ), ReplCommand::new( ".install", - "Reinstall bundled assets, or install assets from a remote git repo (.install remote )", + "Reinstall bundled assets, install a bundle from a git repo, or update an installed bundle", AssertState::pass(), ), ReplCommand::new( @@ -870,27 +870,20 @@ pub async fn run_repl_command( replay::render(app.as_ref(), &compressed, &active)?; } } - ".install" => { - let trimmed = args.map(str::trim).unwrap_or(""); - let mut parts = trimmed.splitn(2, char::is_whitespace); - match parts.next() { - Some("remote") => { - let rest = parts.next().unwrap_or("").trim(); - config::install_remote_from_repl_args(rest)?; - } - Some(name) if !name.is_empty() => match AssetCategory::parse(name) { - Some(category) => config::install_assets(category)?, - None => println!( - "Unknown asset category '{name}'. Valid categories: {}", - AssetCategory::NAMES.join(", ") - ), - }, - _ => println!( - "Usage: .install <{}> | .install remote ", - AssetCategory::NAMES.join("|") - ), + ".install" => match parse_repl_install(args) { + ReplInstallDispatch::Builtins(category) => config::install_assets(category)?, + ReplInstallDispatch::Remote(rest) => { + config::install_remote_from_repl_args(rest)?; } - } + ReplInstallDispatch::Unified(value) => { + config::install_or_update_from_repl_args(value)?; + } + ReplInstallDispatch::Usage => println!( + "Usage: .install <{}> | .install | \ + .install remote [--filter ] [--force]", + AssetCategory::NAMES.join("|") + ), + }, ".update" => { if ctx.macro_flag { bail!("Cannot perform this operation because you are in a macro") @@ -1581,6 +1574,27 @@ fn unknown_command() -> Result<()> { bail!(r#"Unknown command. Type ".help" for additional help."#); } +#[derive(Debug, PartialEq)] +enum ReplInstallDispatch<'a> { + Builtins(AssetCategory), + Remote(&'a str), + Unified(&'a str), + Usage, +} + +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("remote") => ReplInstallDispatch::Remote(parts.next().unwrap_or("").trim()), + Some(name) if !name.is_empty() => match AssetCategory::parse(name) { + Some(category) => ReplInstallDispatch::Builtins(category), + None => ReplInstallDispatch::Unified(trimmed), + }, + _ => ReplInstallDispatch::Usage, + } +} + pub fn builtin_command_names() -> Vec<&'static str> { let mut names: Vec<&'static str> = REPL_COMMANDS .iter() @@ -1806,6 +1820,36 @@ mod tests { assert_eq!(REPL_COMMANDS.len(), 61); } + #[test] + fn parse_repl_install_keeps_category_and_remote_back_compat() { + assert_eq!( + parse_repl_install(Some("agents")), + ReplInstallDispatch::Builtins(AssetCategory::Agents) + ); + assert_eq!( + parse_repl_install(Some("remote https://x --force")), + ReplInstallDispatch::Remote("https://x --force") + ); + } + + #[test] + fn parse_repl_install_routes_other_values_to_unified_dispatch() { + assert_eq!( + parse_repl_install(Some("https://github.com/x/y")), + ReplInstallDispatch::Unified("https://github.com/x/y") + ); + assert_eq!( + parse_repl_install(Some("my-bundle")), + ReplInstallDispatch::Unified("my-bundle") + ); + } + + #[test] + fn parse_repl_install_empty_args_ask_for_usage() { + assert_eq!(parse_repl_install(None), ReplInstallDispatch::Usage); + assert_eq!(parse_repl_install(Some(" ")), ReplInstallDispatch::Usage); + } + #[test] fn builtin_command_names_are_sorted_deduped_first_words_without_dots() { let names = builtin_command_names();