diff --git a/src/cortex-cli/src/agent_cmd/tests.rs b/src/cortex-cli/src/agent_cmd/tests.rs index e2ff07f9f..18f7ba753 100644 --- a/src/cortex-cli/src/agent_cmd/tests.rs +++ b/src/cortex-cli/src/agent_cmd/tests.rs @@ -3,10 +3,9 @@ #[cfg(test)] mod tests { use crate::agent_cmd::cli::{CopyArgs, ExportArgs}; - use crate::agent_cmd::loader::{ - load_builtin_agents, parse_frontmatter, read_file_with_encoding, - }; + use crate::agent_cmd::loader::{load_builtin_agents, parse_frontmatter}; use crate::agent_cmd::types::AgentMode; + use crate::utils::file::read_file_with_encoding; #[test] fn test_read_file_with_utf8() { diff --git a/src/cortex-cli/src/mcp_cmd/handlers.rs b/src/cortex-cli/src/mcp_cmd/handlers.rs index a2d9189cf..29bcca4e2 100644 --- a/src/cortex-cli/src/mcp_cmd/handlers.rs +++ b/src/cortex-cli/src/mcp_cmd/handlers.rs @@ -18,6 +18,13 @@ use super::validation::{ validate_env_var_value, validate_server_name, validate_url, validate_url_internal, }; +fn existing_server_advice(name: &str) -> String { + format!( + "MCP server '{}' already exists. Use --force to overwrite, or 'cortex mcp remove {}' first.", + name, name + ) +} + /// Run the list command. pub(crate) async fn run_list(args: ListArgs) -> Result<()> { let servers = get_mcp_servers()?; @@ -253,11 +260,7 @@ pub(crate) async fn run_add(args: AddArgs) -> Result<()> { // Check if server already exists let server_exists = get_mcp_server(&name)?.is_some(); if server_exists && !force { - bail!( - "MCP server '{}' already exists. Use --force to overwrite, or 'cortex mcp remove {}' first.", - name, - name - ); + bail!("{}", existing_server_advice(&name)); } // Check for conflicting transport types @@ -722,11 +725,8 @@ pub(crate) async fn run_rename(args: RenameArgs) -> Result<()> { } // Check if new name already exists - if get_mcp_server(&args.new_name)?.is_some() { - bail!( - "MCP server '{}' already exists. Remove it first or choose a different name.", - args.new_name - ); + if get_mcp_server(&args.new_name)?.is_some() && !args.force { + bail!("{}", existing_server_advice(&args.new_name)); } let cortex_home = @@ -746,6 +746,9 @@ pub(crate) async fn run_rename(args: RenameArgs) -> Result<()> { if let Some(mcp_servers) = config.get_mut("mcp_servers").and_then(|v| v.as_table_mut()) && let Some(server_config) = mcp_servers.remove(&args.old_name) { + if args.force { + mcp_servers.remove(&args.new_name); + } mcp_servers.insert(args.new_name.clone(), server_config); } @@ -889,9 +892,11 @@ mod tests { let args = RenameArgs { old_name: "old-name".to_string(), new_name: "new-name".to_string(), + force: false, }; assert_eq!(args.old_name, "old-name"); assert_eq!(args.new_name, "new-name"); + assert!(!args.force); } #[test] @@ -899,10 +904,19 @@ mod tests { let args = RenameArgs { old_name: "same".to_string(), new_name: "same".to_string(), + force: false, }; assert_eq!(args.old_name, args.new_name); } + #[test] + fn test_existing_server_advice_is_shared_by_add_and_rename() { + assert_eq!( + existing_server_advice("my-server"), + "MCP server 'my-server' already exists. Use --force to overwrite, or 'cortex mcp remove my-server' first." + ); + } + // ======================================================================== // AddArgs tests // ======================================================================== diff --git a/src/cortex-cli/src/mcp_cmd/types.rs b/src/cortex-cli/src/mcp_cmd/types.rs index f03f8ee58..cae38df9c 100644 --- a/src/cortex-cli/src/mcp_cmd/types.rs +++ b/src/cortex-cli/src/mcp_cmd/types.rs @@ -24,7 +24,6 @@ pub enum McpSubcommand { List(ListArgs), /// List configured MCP servers (alias for list). - #[command(visible_alias = "ls")] Ls(ListArgs), /// Show details for a configured MCP server. @@ -116,7 +115,7 @@ pub struct AddArgs { #[command( group( ArgGroup::new("transport") - .args(["command", "url", "sse"]) + .args(["command", "url", "sse_url"]) .required(true) .multiple(false) ) @@ -174,7 +173,7 @@ pub struct AddMcpSseArgs { #[arg( long = "sse-bearer-token-env-var", value_name = "ENV_VAR", - requires = "sse" + requires = "sse_url" )] pub sse_bearer_token_env_var: Option, } @@ -213,6 +212,10 @@ pub struct RenameArgs { /// New name for the MCP server. pub new_name: String, + + /// Overwrite an existing server with the new name. + #[arg(long, short = 'f')] + pub force: bool, } /// Auth command with subcommands. @@ -293,6 +296,7 @@ pub struct DebugArgs { #[cfg(test)] mod tests { use super::*; + use clap::CommandFactory; // ========================================================================= // ListArgs tests @@ -666,9 +670,11 @@ mod tests { let args = RenameArgs { old_name: "old-server".to_string(), new_name: "new-server".to_string(), + force: false, }; assert_eq!(args.old_name, "old-server"); assert_eq!(args.new_name, "new-server"); + assert!(!args.force); } #[test] @@ -676,10 +682,21 @@ mod tests { let args = RenameArgs { old_name: "server".to_string(), new_name: "server".to_string(), + force: false, }; assert_eq!(args.old_name, args.new_name); } + #[test] + fn test_rename_args_force_flag() { + let args = RenameArgs { + old_name: "old-server".to_string(), + new_name: "existing-server".to_string(), + force: true, + }; + assert!(args.force); + } + // ========================================================================= // AuthCommand tests // ========================================================================= @@ -897,6 +914,11 @@ mod tests { assert!(matches!(subcommand, McpSubcommand::Ls(_))); } + #[test] + fn test_mcp_cli_command_aliases_are_valid() { + McpCli::command().debug_assert(); + } + #[test] fn test_subcommand_get_variant() { let subcommand = McpSubcommand::Get(GetArgs { @@ -951,6 +973,7 @@ mod tests { let subcommand = McpSubcommand::Rename(RenameArgs { old_name: "old".to_string(), new_name: "new".to_string(), + force: false, }); assert!(matches!(subcommand, McpSubcommand::Rename(_))); }