diff --git a/src/cli/registry.rs b/src/cli/registry.rs index ce44de9..aacc515 100644 --- a/src/cli/registry.rs +++ b/src/cli/registry.rs @@ -1,4 +1,4 @@ -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use super::Command; @@ -6,19 +6,89 @@ use super::Command; #[derive(Default, Clone)] pub(crate) struct CommandRegistry { commands: BTreeMap, + aliases: BTreeMap, } impl CommandRegistry { pub(crate) fn get(&self, name: &str) -> Option { - self.commands.get(name).cloned() + if let Some(command) = self.commands.get(name) { + return Some(command.clone()); + } + + self.aliases + .get(name) + .and_then(|canonical| self.commands.get(canonical)) + .cloned() } - pub(crate) fn register(&mut self, command: Command) -> &mut CommandRegistry { - self.commands.insert(command.metadata.name.clone(), command); - self + pub(crate) fn register(&mut self, command: Command) -> Result<&mut CommandRegistry, String> { + self.validate_alias_collisions(&command)?; + + let command_name = command.metadata.name.clone(); + + if let Some(previous) = self.commands.remove(&command_name) { + for alias in previous.metadata.aliases { + self.aliases.remove(&alias); + } + } + + for alias in &command.metadata.aliases { + self.aliases.insert(alias.clone(), command_name.clone()); + } + + self.commands.insert(command_name, command); + Ok(self) } pub(crate) fn get_all(&self) -> Vec { self.commands.values().cloned().collect() } + + fn validate_alias_collisions(&self, command: &Command) -> Result<(), String> { + if let Some(existing_owner) = self.aliases.get(&command.metadata.name) + && existing_owner != &command.metadata.name + { + return Err(format!( + "command name '{}' conflicts with existing alias owned by '{}'", + command.metadata.name, existing_owner + )); + } + + let mut seen_aliases = BTreeSet::new(); + for alias in &command.metadata.aliases { + if alias == &command.metadata.name { + return Err(format!( + "alias '{}' duplicates command name '{}'", + alias, command.metadata.name + )); + } + + if !seen_aliases.insert(alias.clone()) { + return Err(format!( + "alias '{}' is declared more than once for command '{}'", + alias, command.metadata.name + )); + } + + if let Some(existing_command) = self.commands.get(alias) + && existing_command.metadata.name != command.metadata.name + { + return Err(format!( + "alias '{}' conflicts with existing command name '{}'", + alias, existing_command.metadata.name + )); + } + + if let Some(existing_owner) = self.aliases.get(alias) + && existing_owner != &command.metadata.name + { + return Err(format!( + "alias '{}' conflicts with existing alias owned by '{}'", + alias, existing_owner + )); + } + } + + Ok(()) + } } diff --git a/src/core.rs b/src/core.rs index 627f304..e0d889a 100644 --- a/src/core.rs +++ b/src/core.rs @@ -2,19 +2,6 @@ use std::{error::Error, fmt, sync::Arc}; use crate::{Argument, Command, StrategyError, Switch, cli::CommandRegistry}; -/// Controls how [`CliCore`] responds when the registry lock is poisoned. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -#[repr(u8)] -pub enum LockPoisonPolicy { - /// Panic immediately when a poisoned lock is encountered. - /// - /// This is the default for CLI applications where lock poisoning indicates a - /// serious bug and silent recovery would hide inconsistent state. - FailFast = 0, - /// Recover by taking the poisoned inner value. - Recover = 1, -} - /// Renders user-facing help output from registered command metadata. pub trait HelpRenderer: Send + Sync { fn render(&self, caller: &str, registered_commands: &[Command]) -> String; @@ -404,6 +391,8 @@ impl Default for CoreConfig { /// Error returned by CMDkit during command routing and strategy execution. #[derive(Debug)] pub enum CMDKitError { + /// Command registration failed due to invalid or conflicting metadata. + Registration { message: String }, /// No command name was provided in argv. MissingCommand { help: String }, /// The command name does not exist in the registry. @@ -420,6 +409,9 @@ pub enum CMDKitError { impl fmt::Display for CMDKitError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { + Self::Registration { message } => { + write!(f, "Command registration failed: {message}") + } Self::MissingCommand { help } => { write!(f, "No command provided.\n\n{help}") } @@ -468,6 +460,10 @@ impl CMDKit { } /// Runs the CLI with pre-built commands and prints user-facing errors. + /// + /// This is a convenience wrapper that does not surface registration failures + /// to the caller. Prefer [`CMDKit::try_run_with_commands`] when callers need + /// to handle command-registration collisions programmatically. pub fn run_with_commands(commands: &[Command]) { if let Err(e) = Self::try_run_with_commands(commands) { eprintln!("{e}"); @@ -475,9 +471,13 @@ impl CMDKit { } /// Runs the CLI with pre-built commands and recoverable errors. + /// + /// This is the preferred entrypoint for embedding and library use because it + /// returns structured registration errors (for example alias/name collisions) + /// instead of panicking. pub fn try_run_with_commands(commands: &[Command]) -> Result<(), CMDKitError> { Self::builder() - .with_commands(commands) + .try_with_commands(commands)? .build() .try_run_from_env() } @@ -568,9 +568,26 @@ impl CMDKitBuilder { } /// Registers a command into this runtime instance. - pub fn register(mut self, command: Command) -> Self { - self.registry.register(command); - self + /// + /// Prefer [`CMDKitBuilder::try_register`] when command metadata can come from + /// external or dynamic sources. + /// + /// # Panics + /// Panics when registration fails (for example, alias/name collisions). + pub fn register(self, command: Command) -> Self { + self.try_register(command) + .expect("command registration should succeed") + } + + /// Registers a command and returns a structured error on failure. + /// + /// This is the preferred registration API because command registration is + /// fallible: aliases and command names are validated for collisions. + pub fn try_register(mut self, command: Command) -> Result { + self.registry + .register(command) + .map_err(|message| CMDKitError::Registration { message })?; + Ok(self) } fn new() -> CMDKitBuilder { @@ -580,11 +597,30 @@ impl CMDKitBuilder { } } - pub fn with_commands(mut self, commands: &[Command]) -> Self { + /// Registers multiple commands into this runtime instance. + /// + /// Prefer [`CMDKitBuilder::try_with_commands`] in library and embedding + /// scenarios so registration failures can be handled by the caller. + /// + /// # Panics + /// Panics when any command registration fails (for example, alias/name + /// collisions). + pub fn with_commands(self, commands: &[Command]) -> Self { + self.try_with_commands(commands) + .expect("bulk command registration should succeed") + } + + /// Registers multiple commands and returns a structured error on failure. + /// + /// This is the preferred bulk-registration API because registration is + /// fallible and should generally be handled by the caller. + pub fn try_with_commands(mut self, commands: &[Command]) -> Result { for cmd in commands { - self.registry.register(cmd.clone()); + self.registry + .register(cmd.clone()) + .map_err(|message| CMDKitError::Registration { message })?; } - self + Ok(self) } pub fn with_argument_interpreter(mut self, interpreter: I) -> Self diff --git a/src/core/tests.rs b/src/core/tests.rs index 80eac35..1aef458 100644 --- a/src/core/tests.rs +++ b/src/core/tests.rs @@ -1,8 +1,7 @@ use std::sync::{Arc, Mutex}; use crate::{ - CMDKit, CMDKitError, Command, CoreConfig, InvocationArgs, PlainTextHelpRenderer, StrategyError, - argument, command, + CMDKit, CMDKitError, Command, CoreConfig, InvocationArgs, StrategyError, argument, command, }; struct MarkerHelpRenderer; @@ -153,11 +152,3 @@ fn builder_with_config_uses_custom_renderer_for_missing_command_help() { _ => panic!("expected missing command error"), } } - -#[test] -fn lock_poison_policy_values_are_stable() { - assert_eq!(crate::core::LockPoisonPolicy::FailFast as u8, 0); - assert_eq!(crate::core::LockPoisonPolicy::Recover as u8, 1); - - let _ = PlainTextHelpRenderer; -} diff --git a/src/lib.rs b/src/lib.rs index 7bbf542..93c9e1f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,16 +11,22 @@ pub use cli::{ }; pub use core::{ ArgumentInterpreter, CMDKit, CMDKitBuilder, CMDKitError, CoreConfig, HelpRenderer, - InvocationArgs, InvocationElement, LockPoisonPolicy, PlainTextArgumentInterpreter, - PlainTextHelpRenderer, + InvocationArgs, InvocationElement, PlainTextArgumentInterpreter, PlainTextHelpRenderer, }; /// Runs a fresh default [`CMDKit`] instance with pre-built commands. +/// +/// This is a convenience wrapper that prints errors. Prefer +/// [`try_run_with_commands`] when callers should handle registration failures +/// (such as alias/name collisions) programmatically. pub fn run_with_commands(commands: &[Command]) { core::CMDKit::run_with_commands(commands) } /// Runs a fresh default [`CMDKit`] instance with pre-built commands. +/// +/// This is the preferred wrapper for library use because it returns +/// [`CMDKitError`] instead of hiding failure paths. pub fn try_run_with_commands(commands: &[Command]) -> Result<(), CMDKitError> { core::CMDKit::try_run_with_commands(commands) } diff --git a/tests/clicore_suite.rs b/tests/clicore_suite.rs index 913fc72..800bf2c 100644 --- a/tests/clicore_suite.rs +++ b/tests/clicore_suite.rs @@ -93,6 +93,154 @@ fn duplicate_registration_overwrites_previous_entry() { assert_eq!(got.metadata.description, "second"); } +#[test] +fn top_level_command_alias_invocation_succeeds() { + let calls = Arc::new(Mutex::new(Vec::new())); + + let core = CMDKit::builder() + .register( + command("build", "build project") + .with_aliases(vec!["b", "compile"]) + .handler(RecorderStrategy { + calls: Arc::clone(&calls), + error: None, + }) + .build(), + ) + .build(); + + let args = vec!["app".to_string(), "b".to_string()]; + assert!(core.try_run_from_args(&args).is_ok()); + + let guard = calls.lock().expect("call log lock poisoned"); + assert_eq!(guard.len(), 1); +} + +#[test] +fn nested_subcommand_alias_invocation_succeeds() { + let calls = Arc::new(Mutex::new(Vec::new())); + let calls_for_strategy = Arc::clone(&calls); + + let core = CMDKit::builder() + .register( + command("tool", "tool root") + .subcommand( + command("run", "run command") + .with_aliases(vec!["r"]) + .handler_fn(move |options, arguments, params| { + let mut guard = + calls_for_strategy.lock().expect("call log lock poisoned"); + guard.push((options, arguments, params)); + Ok(()) + }) + .build(), + ) + .build(), + ) + .build(); + + let args = vec!["app".to_string(), "tool".to_string(), "r".to_string()]; + assert!(core.try_run_from_args(&args).is_ok()); + + let guard = calls.lock().expect("call log lock poisoned"); + assert_eq!(guard.len(), 1); +} + +#[test] +fn registration_rejects_alias_conflicting_with_existing_command_name() { + let result = CMDKit::builder() + .try_register( + command("build", "build") + .handler_fn(|_, _, _| Ok(())) + .build(), + ) + .and_then(|builder| { + builder.try_register( + command("deploy", "deploy") + .with_aliases(vec!["build"]) + .handler_fn(|_, _, _| Ok(())) + .build(), + ) + }); + + match result { + Err(CMDKitError::Registration { message }) => { + assert!(message.contains("alias 'build' conflicts with existing command name 'build'")); + } + _ => panic!("expected registration collision error"), + } +} + +#[test] +fn registration_rejects_alias_conflicting_with_existing_alias() { + let result = CMDKit::builder() + .try_register( + command("build", "build") + .with_aliases(vec!["b"]) + .handler_fn(|_, _, _| Ok(())) + .build(), + ) + .and_then(|builder| { + builder.try_register( + command("bundle", "bundle") + .with_aliases(vec!["b"]) + .handler_fn(|_, _, _| Ok(())) + .build(), + ) + }); + + match result { + Err(CMDKitError::Registration { message }) => { + assert!(message.contains("alias 'b' conflicts with existing alias owned by 'build'")); + } + _ => panic!("expected registration collision error"), + } +} + +#[test] +fn registration_rejects_command_name_conflicting_with_existing_alias() { + let result = CMDKit::builder() + .try_register( + command("build", "build") + .with_aliases(vec!["b"]) + .handler_fn(|_, _, _| Ok(())) + .build(), + ) + .and_then(|builder| { + builder.try_register(command("b", "b cmd").handler_fn(|_, _, _| Ok(())).build()) + }); + + match result { + Err(CMDKitError::Registration { message }) => { + assert!( + message.contains("command name 'b' conflicts with existing alias owned by 'build'") + ); + } + _ => panic!("expected registration collision error"), + } +} + +#[test] +fn bulk_registration_rejects_collisions() { + let cmd_a = command("build", "build") + .with_aliases(vec!["b"]) + .handler_fn(|_, _, _| Ok(())) + .build(); + let cmd_b = command("bundle", "bundle") + .with_aliases(vec!["b"]) + .handler_fn(|_, _, _| Ok(())) + .build(); + + let result = CMDKit::builder().try_with_commands(&[cmd_a, cmd_b]); + + match result { + Err(CMDKitError::Registration { message }) => { + assert!(message.contains("alias 'b' conflicts with existing alias owned by 'build'")); + } + _ => panic!("expected registration collision error"), + } +} + #[test] fn run_from_args_routes_trailing_arguments_to_strategy() { let calls = Arc::new(Mutex::new(Vec::new()));