diff --git a/src/cortex-cli/src/agent_cmd/handlers/list.rs b/src/cortex-cli/src/agent_cmd/handlers/list.rs index d81f25d9..7a27eec4 100644 --- a/src/cortex-cli/src/agent_cmd/handlers/list.rs +++ b/src/cortex-cli/src/agent_cmd/handlers/list.rs @@ -1,6 +1,6 @@ //! Handler for the `agent list` command. -use anyhow::Result; +use anyhow::{Result, bail}; use crate::agent_cmd::cli::ListArgs; use crate::agent_cmd::loader::load_all_agents; @@ -9,6 +9,13 @@ use crate::agent_cmd::utils::matches_pattern; /// List agents command. pub async fn run_list(args: ListArgs) -> Result<()> { + // Validate mutually exclusive flags + if args.primary && args.subagents { + bail!( + "Cannot specify both --primary and --subagents. Choose one filter or use neither for all agents." + ); + } + // Handle --remote flag if args.remote { println!("Remote agent registry:"); diff --git a/src/cortex-cli/src/alias_cmd.rs b/src/cortex-cli/src/alias_cmd.rs index 979c3e4e..f5f573b0 100644 --- a/src/cortex-cli/src/alias_cmd.rs +++ b/src/cortex-cli/src/alias_cmd.rs @@ -241,10 +241,18 @@ async fn run_remove(args: AliasRemoveArgs) -> Result<()> { async fn run_show(args: AliasShowArgs) -> Result<()> { let config = load_aliases()?; - let alias = config - .aliases - .get(&args.name) - .ok_or_else(|| anyhow::anyhow!("Alias '{}' does not exist.", args.name))?; + let alias = match config.aliases.get(&args.name) { + Some(a) => a, + None => { + if args.json { + let error = serde_json::json!({ + "error": format!("Alias '{}' does not exist", args.name) + }); + println!("{}", serde_json::to_string_pretty(&error)?); + } + bail!("Alias '{}' does not exist.", args.name); + } + }; if args.json { println!("{}", serde_json::to_string_pretty(alias)?); diff --git a/src/cortex-cli/src/cli/handlers.rs b/src/cortex-cli/src/cli/handlers.rs index 58ca8589..2c6d04bc 100644 --- a/src/cortex-cli/src/cli/handlers.rs +++ b/src/cortex-cli/src/cli/handlers.rs @@ -27,7 +27,7 @@ pub async fn dispatch_command(cli: Cli) -> Result<()> { Some(Commands::Login(login_cli)) => handle_login(login_cli).await, Some(Commands::Logout(logout_cli)) => handle_logout(logout_cli).await, Some(Commands::Whoami) => { - run_whoami().await; + run_whoami().await?; Ok(()) } Some(Commands::Mcp(mcp_cli)) => mcp_cli.run().await, @@ -182,6 +182,23 @@ async fn run_init(init_cli: InitCommand) -> Result<()> { /// Handle login command. async fn handle_login(login_cli: LoginCommand) -> Result<()> { + // Validate mutually exclusive authentication methods + let auth_methods_count = [ + login_cli.token.is_some(), + login_cli.use_sso, + login_cli.use_device_code, + login_cli.with_api_key, + ] + .iter() + .filter(|&&x| x) + .count(); + + if auth_methods_count > 1 { + bail!( + "Cannot specify multiple authentication methods. Choose one: --token, --sso, --device-auth, or --with-api-key." + ); + } + match login_cli.action { Some(LoginSubcommand::Status) => run_login_status(login_cli.config_overrides).await, None => { @@ -512,7 +529,7 @@ fn install_completions(shell: Shell) -> Result<()> { // ============================================================================ /// Show current logged-in user. -pub async fn run_whoami() { +pub async fn run_whoami() -> Result<()> { use cortex_login::{AuthMode, load_auth_with_fallback, safe_format_key}; let cortex_home = dirs::home_dir() @@ -527,7 +544,7 @@ pub async fn run_whoami() { "Authenticated via CORTEX_AUTH_TOKEN: {}", safe_format_key(&token) ); - return; + return Ok(()); } if let Ok(token) = std::env::var("CORTEX_API_KEY") @@ -537,7 +554,7 @@ pub async fn run_whoami() { "Authenticated via CORTEX_API_KEY: {}", safe_format_key(&token) ); - return; + return Ok(()); } // Load stored credentials @@ -565,9 +582,10 @@ pub async fn run_whoami() { println!("Not logged in. Run 'cortex login' to authenticate."); } Err(e) => { - eprintln!("Error checking login status: {}", e); + return Err(anyhow::anyhow!("Error checking login status: {}", e)); } } + Ok(()) } /// Resume a previous session. @@ -585,6 +603,16 @@ pub async fn run_resume(resume_cli: ResumeCommand) -> Result<()> { let config = cortex_engine::Config::default(); let id_str = match (resume_cli.session_id, resume_cli.last, resume_cli.pick) { + // Support "last" as SESSION_ID as documented in help text (Issue #3646) + (Some(id), _, _) if id.to_lowercase() == "last" => { + let sessions = cortex_engine::list_sessions(&config.cortex_home)?; + if sessions.is_empty() { + print_info("No sessions found to resume."); + return Ok(()); + } + print_info("Resuming most recent session..."); + sessions[0].id.clone() + } (Some(id), _, _) => id, (None, true, _) => { let sessions = cortex_engine::list_sessions(&config.cortex_home)?; @@ -810,10 +838,10 @@ pub async fn show_config(config_cli: ConfigCommand) -> Result<()> { if let Some(value) = config.get(&args.key) { println!("{}", value); } else { - println!("Key '{}' not found.", args.key); + bail!("Key '{}' not found in configuration", args.key); } } else { - println!("No configuration file found."); + bail!("No configuration file found"); } } Some(ConfigSubcommand::Set(args)) => { @@ -841,10 +869,10 @@ pub async fn show_config(config_cli: ConfigCommand) -> Result<()> { std::fs::write(&config_path, content)?; print_success(&format!("Removed key: {}", args.key)); } else { - print_warning(&format!("Key '{}' not found.", args.key)); + bail!("Key '{}' not found in configuration", args.key); } } else { - print_warning("No configuration file found."); + bail!("No configuration file found"); } } None => { diff --git a/src/cortex-cli/src/cli/styles.rs b/src/cortex-cli/src/cli/styles.rs index f3f72b97..4032f94e 100644 --- a/src/cortex-cli/src/cli/styles.rs +++ b/src/cortex-cli/src/cli/styles.rs @@ -49,8 +49,8 @@ pub const AFTER_HELP: &str = color_print::cstr!( Agents ~/.cortex/agents/ (personal), .cortex/agents/ (project) 🔗 LEARN MORE - https://docs.cortex.foundation Documentation - https://github.com/CortexLM/cortex-cli Source & Issues"# + https://docs.cortex.foundation Documentation + https://github.com/CortexLM/cortex Source & Issues"# ); /// Before-help section with ASCII art banner. diff --git a/src/cortex-cli/src/compact_cmd.rs b/src/cortex-cli/src/compact_cmd.rs index 8ebc008f..a908724c 100644 --- a/src/cortex-cli/src/compact_cmd.rs +++ b/src/cortex-cli/src/compact_cmd.rs @@ -407,33 +407,32 @@ async fn run_compact(args: CompactRunArgs) -> Result<()> { } if args.dry_run { - println!("Dry run mode - no changes will be made."); - println!(); + let log_files_count = dir_stats(&logs_dir).0; + let session_files_count = dir_stats(&sessions_dir).0; + let history_files_count = dir_stats(&history_dir).0; + let orphaned_history_count = count_orphaned_history(&sessions_dir, &history_dir); - let status = CompactionStatus { - data_dir: data_dir.clone(), - logs_dir: logs_dir.clone(), - sessions_dir: sessions_dir.clone(), - history_dir: history_dir.clone(), - log_files_count: dir_stats(&logs_dir).0, - log_files_size: dir_stats(&logs_dir).1, - log_files_size_human: format_size(dir_stats(&logs_dir).1), - session_files_count: dir_stats(&sessions_dir).0, - history_files_count: dir_stats(&history_dir).0, - orphaned_history_count: count_orphaned_history(&sessions_dir, &history_dir), - total_data_size: 0, - total_data_size_human: String::new(), - lock_held: false, - }; - - println!("Would process:"); - println!(" Log files: {}", status.log_files_count); - println!(" Session files: {}", status.session_files_count); - println!(" History files: {}", status.history_files_count); - println!( - " Orphaned files to clean: {}", - status.orphaned_history_count - ); + if args.json { + let output = serde_json::json!({ + "dry_run": true, + "would_process": { + "log_files": log_files_count, + "session_files": session_files_count, + "history_files": history_files_count, + "orphaned_files_to_clean": orphaned_history_count + }, + "message": "Dry run completed - no changes made" + }); + println!("{}", serde_json::to_string_pretty(&output)?); + } else { + println!("Dry run mode - no changes will be made."); + println!(); + println!("Would process:"); + println!(" Log files: {}", log_files_count); + println!(" Session files: {}", session_files_count); + println!(" History files: {}", history_files_count); + println!(" Orphaned files to clean: {}", orphaned_history_count); + } return Ok(()); } diff --git a/src/cortex-cli/src/dag_cmd/commands.rs b/src/cortex-cli/src/dag_cmd/commands.rs index 53e32e0a..dc20992f 100644 --- a/src/cortex-cli/src/dag_cmd/commands.rs +++ b/src/cortex-cli/src/dag_cmd/commands.rs @@ -71,6 +71,11 @@ pub async fn run_create(args: DagCreateArgs) -> Result<()> { /// Execute a DAG. pub async fn run_execute(args: DagRunArgs) -> Result<()> { + // Validate --jobs argument (Issue #3716) + if args.max_concurrent == 0 { + bail!("--jobs must be at least 1. Use --jobs 1 for sequential execution."); + } + let spec = load_spec(&args.file)?; let specs = convert_specs(&spec); @@ -86,14 +91,33 @@ pub async fn run_execute(args: DagRunArgs) -> Result<()> { if matches!(args.strategy, ExecutionStrategy::DryRun) { // Dry run - just validate - dag.topological_sort() + let order = dag + .topological_sort() .map_err(|e| anyhow::anyhow!("DAG validation failed: {}", e))?; - print_success(&format!( - "✓ DAG is valid ({} tasks in execution order)", - dag.len() - )); - println!(); - print_execution_order(&dag)?; + + match args.format { + DagOutputFormat::Json => { + let task_names: Vec = order + .iter() + .filter_map(|id| dag.get_task(*id).map(|t| t.name.clone())) + .collect(); + let output = serde_json::json!({ + "dry_run": true, + "valid": true, + "task_count": dag.len(), + "execution_order": task_names + }); + println!("{}", serde_json::to_string_pretty(&output)?); + } + _ => { + print_success(&format!( + "✓ DAG is valid ({} tasks in execution order)", + dag.len() + )); + println!(); + print_execution_order(&dag)?; + } + } return Ok(()); } @@ -251,7 +275,14 @@ pub async fn run_list(args: DagListArgs) -> Result<()> { let ids = store.list().await?; if ids.is_empty() { - print_info("No DAGs found"); + match args.format { + DagOutputFormat::Json => { + println!("[]"); + } + _ => { + print_info("No DAGs found"); + } + } return Ok(()); } diff --git a/src/cortex-cli/src/dag_cmd/helpers.rs b/src/cortex-cli/src/dag_cmd/helpers.rs index 78ea7aab..2c3b9e2e 100644 --- a/src/cortex-cli/src/dag_cmd/helpers.rs +++ b/src/cortex-cli/src/dag_cmd/helpers.rs @@ -28,6 +28,17 @@ pub fn load_spec(path: &PathBuf) -> Result { serde_yaml::from_str(&content).context("Failed to parse YAML")? }; + // Check for duplicate task IDs (Issue #3815) + let mut seen_names = std::collections::HashSet::new(); + for task in &spec.tasks { + if !seen_names.insert(&task.name) { + anyhow::bail!( + "Duplicate task ID '{}' found in DAG specification. Task IDs must be unique.", + task.name + ); + } + } + Ok(spec) } diff --git a/src/cortex-cli/src/debug_cmd/handlers/config.rs b/src/cortex-cli/src/debug_cmd/handlers/config.rs index 9ba99782..32624a55 100644 --- a/src/cortex-cli/src/debug_cmd/handlers/config.rs +++ b/src/cortex-cli/src/debug_cmd/handlers/config.rs @@ -22,10 +22,27 @@ pub async fn run_config(args: ConfigArgs) -> Result<()> { ) })?; - let global_config = config.cortex_home.join("config.toml"); - let local_config = std::env::current_dir() + // Check for both config.toml and config.json (Issue #3150) + // Cortex actually uses config.json, not config.toml + let global_config_json = config.cortex_home.join("config.json"); + let global_config_toml = config.cortex_home.join("config.toml"); + let global_config = if global_config_json.exists() { + global_config_json + } else { + global_config_toml + }; + + let local_config_json = std::env::current_dir() + .ok() + .map(|d| d.join(".cortex/config.json")); + let local_config_toml = std::env::current_dir() .ok() .map(|d| d.join(".cortex/config.toml")); + let local_config = if local_config_json.as_ref().is_some_and(|p| p.exists()) { + local_config_json + } else { + local_config_toml + }; let resolved = ResolvedConfig { model: config.model.clone(), @@ -140,14 +157,26 @@ pub async fn run_config(args: ConfigArgs) -> Result<()> { // Handle --diff flag: compare local and global configs if args.diff { - println!(); - println!("Config Diff (Global vs Local)"); - println!("{}", "=".repeat(50)); + // Check for both config.json and config.toml (Issue #3150) + let global_json_path = config.cortex_home.join("config.json"); + let global_toml_path = config.cortex_home.join("config.toml"); + let global_path = if global_json_path.exists() { + global_json_path + } else { + global_toml_path + }; - let global_path = config.cortex_home.join("config.toml"); - let local_path = std::env::current_dir() + let local_json_path = std::env::current_dir() + .ok() + .map(|d| d.join(".cortex/config.json")); + let local_toml_path = std::env::current_dir() .ok() .map(|d| d.join(".cortex/config.toml")); + let local_path = if local_json_path.as_ref().is_some_and(|p| p.exists()) { + local_json_path + } else { + local_toml_path + }; let global_content = if global_path.exists() { std::fs::read_to_string(&global_path).ok() @@ -163,40 +192,67 @@ pub async fn run_config(args: ConfigArgs) -> Result<()> { } }); - match (global_content.as_ref(), local_content.as_ref()) { - (None, None) => { - println!(" No config files found."); - } - (Some(_global), None) => { - println!(" Only global config exists."); - if args.json { - let diff_output = serde_json::json!({ + if args.json { + // Pure JSON output when --json is specified + let diff_output = match (global_content.as_ref(), local_content.as_ref()) { + (None, None) => { + serde_json::json!({ + "global_only": false, + "local_only": false, + "no_configs": true, + "differences": [], + }) + } + (Some(_global), None) => { + serde_json::json!({ "global_only": true, "local_only": false, "differences": [], - }); - println!("{}", serde_json::to_string_pretty(&diff_output)?); + }) } - } - (None, Some(_local)) => { - println!(" Only local config exists."); - if args.json { - let diff_output = serde_json::json!({ + (None, Some(_local)) => { + serde_json::json!({ "global_only": false, "local_only": true, "differences": [], - }); - println!("{}", serde_json::to_string_pretty(&diff_output)?); + }) } - } - (Some(global), Some(local)) => { - if global == local { - println!(" Configs are identical."); - } else { - let diff = compute_config_diff(global, local); - if args.json { - println!("{}", serde_json::to_string_pretty(&diff)?); + (Some(global), Some(local)) => { + if global == local { + serde_json::json!({ + "global_only": false, + "local_only": false, + "identical": true, + "differences": [], + }) + } else { + let diff = compute_config_diff(global, local); + serde_json::to_value(&diff)? + } + } + }; + println!("{}", serde_json::to_string_pretty(&diff_output)?); + } else { + // Text output + println!(); + println!("Config Diff (Global vs Local)"); + println!("{}", "=".repeat(50)); + + match (global_content.as_ref(), local_content.as_ref()) { + (None, None) => { + println!(" No config files found."); + } + (Some(_global), None) => { + println!(" Only global config exists."); + } + (None, Some(_local)) => { + println!(" Only local config exists."); + } + (Some(global), Some(local)) => { + if global == local { + println!(" Configs are identical."); } else { + let diff = compute_config_diff(global, local); println!(); if !diff.only_in_global.is_empty() { println!("Lines only in global config:"); diff --git a/src/cortex-cli/src/debug_cmd/handlers/file.rs b/src/cortex-cli/src/debug_cmd/handlers/file.rs index 6c96e8b3..e5fffb0e 100644 --- a/src/cortex-cli/src/debug_cmd/handlers/file.rs +++ b/src/cortex-cli/src/debug_cmd/handlers/file.rs @@ -1,6 +1,6 @@ //! File command handler. -use anyhow::Result; +use anyhow::{Result, bail}; use crate::debug_cmd::commands::FileArgs; use crate::debug_cmd::types::{FileDebugOutput, FileMetadata}; @@ -19,91 +19,87 @@ pub async fn run_file(args: FileArgs) -> Result<()> { let exists = path.exists(); + if !exists { + bail!("File does not exist: {}", path.display()); + } + // Detect special file types using stat() BEFORE attempting any reads // This prevents blocking on FIFOs, sockets, and other special files - let special_file_type = if exists { - detect_special_file_type(&path) - } else { - None - }; - - let (metadata, error) = if exists { - match std::fs::metadata(&path) { - Ok(meta) => { - let modified = meta - .modified() + let special_file_type = detect_special_file_type(&path); + + let (metadata, error) = match std::fs::metadata(&path) { + Ok(meta) => { + let modified = meta + .modified() + .ok() + .map(|t| chrono::DateTime::::from(t).to_rfc3339()); + let created = meta + .created() + .ok() + .map(|t| chrono::DateTime::::from(t).to_rfc3339()); + + // Get symlink target if applicable + let symlink_target = if meta.file_type().is_symlink() { + std::fs::read_link(&path) .ok() - .map(|t| chrono::DateTime::::from(t).to_rfc3339()); - let created = meta - .created() - .ok() - .map(|t| chrono::DateTime::::from(t).to_rfc3339()); + .map(|p| p.to_string_lossy().to_string()) + } else { + None + }; - // Get symlink target if applicable - let symlink_target = if meta.file_type().is_symlink() { - std::fs::read_link(&path) - .ok() - .map(|p| p.to_string_lossy().to_string()) - } else { - None - }; - - // Check if the current user can actually write to the file - // This is more accurate than just checking permission bits - // Skip this check for special files (FIFOs, etc.) to avoid blocking - let readonly = if special_file_type.is_some() { - false // Don't check writability for special files - } else { - !is_writable_by_current_user(&path) - }; - - // Check if this is a virtual filesystem (procfs, sysfs, etc.) - // These report size=0 in stat() but may have actual content - let is_virtual_fs = is_virtual_filesystem(&path); - let stat_size = meta.len(); - - // For virtual filesystem files that report 0 size, try to read actual content size - let actual_size = if is_virtual_fs && stat_size == 0 && meta.is_file() { - // Try to read the file to get actual content size - // Limit read to 1MB to avoid hanging on infinite streams - match std::fs::read(&path) { - Ok(content) if !content.is_empty() => Some(content.len() as u64), - _ => None, - } - } else { - None - }; - - // Get file permissions - let (permissions, mode) = get_unix_permissions(&meta); - - ( - Some(FileMetadata { - size: stat_size, - actual_size, - is_virtual_fs: if is_virtual_fs { Some(true) } else { None }, - is_file: meta.is_file(), - is_dir: meta.is_dir(), - is_symlink: meta.file_type().is_symlink(), - file_type: special_file_type.clone(), - symlink_target, - modified, - created, - readonly, - permissions, - mode, - }), - None, - ) - } - Err(e) => (None, Some(e.to_string())), + // Check if the current user can actually write to the file + // This is more accurate than just checking permission bits + // Skip this check for special files (FIFOs, etc.) to avoid blocking + let readonly = if special_file_type.is_some() { + false // Don't check writability for special files + } else { + !is_writable_by_current_user(&path) + }; + + // Check if this is a virtual filesystem (procfs, sysfs, etc.) + // These report size=0 in stat() but may have actual content + let is_virtual_fs = is_virtual_filesystem(&path); + let stat_size = meta.len(); + + // For virtual filesystem files that report 0 size, try to read actual content size + let actual_size = if is_virtual_fs && stat_size == 0 && meta.is_file() { + // Try to read the file to get actual content size + // Limit read to 1MB to avoid hanging on infinite streams + match std::fs::read(&path) { + Ok(content) if !content.is_empty() => Some(content.len() as u64), + _ => None, + } + } else { + None + }; + + // Get file permissions + let (permissions, mode) = get_unix_permissions(&meta); + + ( + Some(FileMetadata { + size: stat_size, + actual_size, + is_virtual_fs: if is_virtual_fs { Some(true) } else { None }, + is_file: meta.is_file(), + is_dir: meta.is_dir(), + is_symlink: meta.file_type().is_symlink(), + file_type: special_file_type.clone(), + symlink_target, + modified, + created, + readonly, + permissions, + mode, + }), + None, + ) } - } else { - (None, Some("File does not exist".to_string())) + Err(e) => (None, Some(e.to_string())), }; // Detect MIME type from extension - skip for special files - let mime_type = if exists && path.is_file() && special_file_type.is_none() { + let mime_type = if path.is_file() && special_file_type.is_none() { path.extension() .and_then(|ext| ext.to_str()) .map(guess_mime_type) @@ -112,7 +108,7 @@ pub async fn run_file(args: FileArgs) -> Result<()> { }; // Detect encoding and binary status - SKIP for special files to avoid blocking - let (encoding, is_binary) = if exists && path.is_file() && special_file_type.is_none() { + let (encoding, is_binary) = if path.is_file() && special_file_type.is_none() { detect_encoding_and_binary(&path) } else { (None, None) @@ -120,7 +116,7 @@ pub async fn run_file(args: FileArgs) -> Result<()> { // Check if the file appears to be actively modified by comparing // metadata from two reads with a small delay - let active_modification_warning = if exists && path.is_file() { + let active_modification_warning = if path.is_file() { // Get initial size let initial_size = std::fs::metadata(&path).ok().map(|m| m.len()); // Brief delay to detect active writes diff --git a/src/cortex-cli/src/debug_cmd/handlers/wait.rs b/src/cortex-cli/src/debug_cmd/handlers/wait.rs index 89cbbaa2..799e2d75 100644 --- a/src/cortex-cli/src/debug_cmd/handlers/wait.rs +++ b/src/cortex-cli/src/debug_cmd/handlers/wait.rs @@ -102,7 +102,15 @@ pub async fn run_wait(args: WaitArgs) -> Result<()> { (condition, success, error) } else { - bail!("No condition specified. Use --lsp-ready, --server-ready, or --port"); + let error_message = "No condition specified. Use --lsp-ready, --server-ready, or --port"; + if args.json { + let error = serde_json::json!({ + "error": error_message, + "success": false + }); + println!("{}", serde_json::to_string_pretty(&error)?); + } + bail!("{}", error_message); }; let waited_ms = start.elapsed().as_millis() as u64; diff --git a/src/cortex-cli/src/exec_cmd/autonomy.rs b/src/cortex-cli/src/exec_cmd/autonomy.rs index cc374190..aaab5257 100644 --- a/src/cortex-cli/src/exec_cmd/autonomy.rs +++ b/src/cortex-cli/src/exec_cmd/autonomy.rs @@ -74,9 +74,13 @@ impl AutonomyLevel { } /// Check if a command risk level is allowed. - pub fn allows_risk(&self, risk: &str) -> bool { + /// + /// # Arguments + /// * `risk` - The risk level of the command ("low", "medium", "high") + /// * `command` - The actual command string to check if it's read-only + pub fn allows_risk(&self, risk: &str, command: &str) -> bool { match self { - AutonomyLevel::ReadOnly => risk == "low" && is_read_only_command(risk), + AutonomyLevel::ReadOnly => risk == "low" && is_read_only_command(command), AutonomyLevel::Low => risk == "low", AutonomyLevel::Medium => risk == "low" || risk == "medium", AutonomyLevel::High => true, @@ -149,4 +153,34 @@ mod tests { assert!(!is_read_only_command("rm -rf /")); assert!(!is_read_only_command("git push")); } + + #[test] + fn test_allows_risk() { + // ReadOnly mode: only allows low-risk read-only commands + assert!(AutonomyLevel::ReadOnly.allows_risk("low", "cat file.txt")); + assert!(AutonomyLevel::ReadOnly.allows_risk("low", "ls -la")); + assert!(AutonomyLevel::ReadOnly.allows_risk("low", "git status")); + // ReadOnly should NOT allow low-risk non-read-only commands + assert!(!AutonomyLevel::ReadOnly.allows_risk("low", "rm file.txt")); + assert!(!AutonomyLevel::ReadOnly.allows_risk("low", "git push")); + // ReadOnly should NOT allow medium/high risk + assert!(!AutonomyLevel::ReadOnly.allows_risk("medium", "cat file.txt")); + assert!(!AutonomyLevel::ReadOnly.allows_risk("high", "cat file.txt")); + + // Low mode: allows any low-risk command (regardless of command type) + assert!(AutonomyLevel::Low.allows_risk("low", "rm file.txt")); + assert!(AutonomyLevel::Low.allows_risk("low", "cat file.txt")); + assert!(!AutonomyLevel::Low.allows_risk("medium", "cat file.txt")); + assert!(!AutonomyLevel::Low.allows_risk("high", "cat file.txt")); + + // Medium mode: allows low and medium risk + assert!(AutonomyLevel::Medium.allows_risk("low", "rm file.txt")); + assert!(AutonomyLevel::Medium.allows_risk("medium", "npm install")); + assert!(!AutonomyLevel::Medium.allows_risk("high", "git push")); + + // High mode: allows everything + assert!(AutonomyLevel::High.allows_risk("low", "cat file.txt")); + assert!(AutonomyLevel::High.allows_risk("medium", "npm install")); + assert!(AutonomyLevel::High.allows_risk("high", "git push --force")); + } } diff --git a/src/cortex-cli/src/exec_cmd/runner.rs b/src/cortex-cli/src/exec_cmd/runner.rs index 510f8c70..acbcce61 100644 --- a/src/cortex-cli/src/exec_cmd/runner.rs +++ b/src/cortex-cli/src/exec_cmd/runner.rs @@ -24,6 +24,13 @@ use super::output::{ExecInputFormat, ExecOutputFormat}; impl ExecCli { /// Run the exec command. pub async fn run(self) -> Result<()> { + // Validate mutually exclusive tool flags + if !self.enabled_tools.is_empty() && !self.disabled_tools.is_empty() { + bail!( + "Cannot specify both --enabled-tools and --disabled-tools. Choose one to filter tools." + ); + } + // Ensure UTF-8 locale for proper text handling let _ = ensure_utf8_locale(); diff --git a/src/cortex-cli/src/lock_cmd.rs b/src/cortex-cli/src/lock_cmd.rs index 50d719eb..41109500 100644 --- a/src/cortex-cli/src/lock_cmd.rs +++ b/src/cortex-cli/src/lock_cmd.rs @@ -184,7 +184,30 @@ impl LockCli { } } +/// Validate session ID format (must be valid UUID or 8-char prefix) +fn validate_session_id(session_id: &str) -> Result<()> { + // Check if it's a valid UUID + if uuid::Uuid::parse_str(session_id).is_ok() { + return Ok(()); + } + + // Check if it's a valid 8-character hex prefix + if session_id.len() == 8 && session_id.chars().all(|c| c.is_ascii_hexdigit()) { + return Ok(()); + } + + bail!( + "Invalid session ID: '{}'. Expected a full UUID (e.g., '550e8400-e29b-41d4-a716-446655440000') or an 8-character prefix.", + session_id + ) +} + async fn run_add(args: LockAddArgs) -> Result<()> { + // Validate all session IDs first (Issue #3696) + for session_id in &args.session_ids { + validate_session_id(session_id)?; + } + let mut lock_file = load_lock_file()?; let timestamp = chrono::Utc::now().to_rfc3339(); diff --git a/src/cortex-cli/src/mcp_cmd/auth.rs b/src/cortex-cli/src/mcp_cmd/auth.rs index 44bebb6b..25d68599 100644 --- a/src/cortex-cli/src/mcp_cmd/auth.rs +++ b/src/cortex-cli/src/mcp_cmd/auth.rs @@ -203,6 +203,13 @@ async fn run_auth( pub(crate) async fn run_logout(args: LogoutArgs) -> Result<()> { use cortex_engine::mcp; + // Validate mutually exclusive flags + if args.name.is_some() && args.all { + bail!( + "Cannot specify both --name and --all. Use --name for a specific server or --all for all servers." + ); + } + if args.all { // Logout from all servers let servers = get_mcp_servers()?; diff --git a/src/cortex-cli/src/mcp_cmd/handlers.rs b/src/cortex-cli/src/mcp_cmd/handlers.rs index eca2f253..9ce5060c 100644 --- a/src/cortex-cli/src/mcp_cmd/handlers.rs +++ b/src/cortex-cli/src/mcp_cmd/handlers.rs @@ -120,8 +120,19 @@ pub(crate) async fn run_list(args: ListArgs) -> Result<()> { /// Run the get command. pub(crate) async fn run_get(args: GetArgs) -> Result<()> { - let server = get_mcp_server(&args.name)? - .ok_or_else(|| anyhow::anyhow!("No MCP server named '{}' found", args.name))?; + let server = match get_mcp_server(&args.name)? { + Some(s) => s, + None => { + let error_message = format!("No MCP server named '{}' found", args.name); + if args.json { + let error = serde_json::json!({ + "error": error_message + }); + println!("{}", serde_json::to_string_pretty(&error)?); + } + bail!("{}", error_message); + } + }; if args.json { let json = serde_json::to_string_pretty(&server)?; @@ -239,6 +250,21 @@ pub(crate) async fn run_add(args: AddArgs) -> Result<()> { validate_server_name(&name)?; + // Check for conflicting transport types + let has_url = transport_args + .streamable_http + .as_ref() + .map(|h| !h.url.is_empty()) + .unwrap_or(false); + let has_sse = transport_args + .sse_transport + .as_ref() + .and_then(|s| s.sse_url.as_ref()) + .is_some(); + if has_url && has_sse { + bail!("Cannot specify both --url and --sse. Choose one transport type."); + } + // Check if server already exists let server_exists = get_mcp_server(&name)?.is_some(); if server_exists && !force { diff --git a/src/cortex-cli/src/models_cmd.rs b/src/cortex-cli/src/models_cmd.rs index 4b044125..9632aede 100644 --- a/src/cortex-cli/src/models_cmd.rs +++ b/src/cortex-cli/src/models_cmd.rs @@ -422,6 +422,16 @@ async fn run_list( sort_by: &str, show_full: bool, ) -> Result<()> { + // Validate sort value (Issue #3722) + const VALID_SORT_VALUES: &[&str] = &["name", "provider", "context", "created", "id"]; + if !VALID_SORT_VALUES.contains(&sort_by) { + anyhow::bail!( + "Invalid sort value '{}'. Valid values are: {}", + sort_by, + VALID_SORT_VALUES.join(", ") + ); + } + let mut models = get_available_models(); // Parse sort order (Issue #1993) diff --git a/src/cortex-cli/src/plugin_cmd.rs b/src/cortex-cli/src/plugin_cmd.rs index 1e558212..bfa2fe2d 100644 --- a/src/cortex-cli/src/plugin_cmd.rs +++ b/src/cortex-cli/src/plugin_cmd.rs @@ -144,6 +144,13 @@ impl PluginCli { } async fn run_list(args: PluginListArgs) -> Result<()> { + // Validate mutually exclusive flags + if args.enabled && args.disabled { + bail!( + "Cannot specify both --enabled and --disabled. Choose one filter or use neither for all plugins." + ); + } + let plugins_dir = get_plugins_dir(); if !plugins_dir.exists() { @@ -243,6 +250,11 @@ async fn run_list(args: PluginListArgs) -> Result<()> { } async fn run_install(args: PluginInstallArgs) -> Result<()> { + // Validate plugin name is not empty (Issue #3700) + if args.name.trim().is_empty() { + bail!("Plugin name cannot be empty. Please provide a valid plugin name."); + } + let plugins_dir = get_plugins_dir(); // Create plugins directory if it doesn't exist @@ -392,6 +404,12 @@ async fn run_show(args: PluginShowArgs) -> Result<()> { let manifest_path = plugin_path.join("plugin.toml"); if !manifest_path.exists() { + if args.json { + let error = serde_json::json!({ + "error": format!("Plugin '{}' is not installed", args.name) + }); + println!("{}", serde_json::to_string_pretty(&error)?); + } bail!("Plugin '{}' is not installed.", args.name); } diff --git a/src/cortex-cli/src/pr_cmd.rs b/src/cortex-cli/src/pr_cmd.rs index f9948c7e..8b9874a9 100644 --- a/src/cortex-cli/src/pr_cmd.rs +++ b/src/cortex-cli/src/pr_cmd.rs @@ -130,7 +130,7 @@ async fn run_pr_checkout(args: PrCli) -> Result<()> { let repository = format!("{}/{}", owner, repo); - println!("🔀 Pull Request #{}", pr_number); + println!("[PR] Pull Request #{}", pr_number); println!("{}", "=".repeat(40)); println!("Repository: {}", repository); println!(); @@ -328,7 +328,7 @@ async fn run_pr_checkout(args: PrCli) -> Result<()> { validate_refspec(&refspec)?; println!("Fetching PR #{}...", pr_number); - print!(" ⏳ Downloading PR data from origin..."); + print!(" [WAIT] Downloading PR data from origin..."); std::io::Write::flush(&mut std::io::stdout()).ok(); // SECURITY: Arguments are passed as separate array elements, not interpolated into a string @@ -345,7 +345,7 @@ async fn run_pr_checkout(args: PrCli) -> Result<()> { } // Checkout the branch - print!(" ⏳ Checking out branch '{}'...", branch_name); + print!(" [WAIT] Checking out branch '{}'...", branch_name); std::io::Write::flush(&mut std::io::stdout()).ok(); let checkout_args = if args.force { diff --git a/src/cortex-cli/src/run_cmd/execution.rs b/src/cortex-cli/src/run_cmd/execution.rs index 5ec34066..18bb58d5 100644 --- a/src/cortex-cli/src/run_cmd/execution.rs +++ b/src/cortex-cli/src/run_cmd/execution.rs @@ -275,7 +275,44 @@ impl RunCli { tracing::warn!("Failed to scan custom commands: {}", e); } - let (mut session, handle) = Session::new(config.clone())?; + // Create session, handling auth errors specially for JSON output + let (mut session, handle) = match Session::new(config.clone()) { + Ok(result) => result, + Err(e) => { + let error_message = e.to_string(); + let is_auth_error = error_message.contains("Authentication required") + || error_message.contains("authentication") + || error_message.contains("auth") + || error_message.contains("API key") + || error_message.contains("login"); + + if is_json { + let error_type = if is_auth_error { + "authentication_error" + } else { + "session_error" + }; + + let mut error_json = serde_json::json!({ + "error": { + "type": error_type, + "message": error_message, + } + }); + + // Add hint for auth errors + if is_auth_error { + error_json["error"]["hint"] = serde_json::Value::String( + "Run 'cortex login' to authenticate".to_string(), + ); + } + + println!("{}", serde_json::to_string_pretty(&error_json)?); + } + + return Err(e.into()); + } + }; // Handle session resumption if needed let session_id = match session_mode { diff --git a/src/cortex-cli/src/uninstall_cmd.rs b/src/cortex-cli/src/uninstall_cmd.rs index 747f96b5..aa4db430 100644 --- a/src/cortex-cli/src/uninstall_cmd.rs +++ b/src/cortex-cli/src/uninstall_cmd.rs @@ -181,7 +181,16 @@ impl UninstallCli { return Ok(()); } - // Backup if requested + // Confirm before proceeding (Issue #3682: confirm BEFORE creating backup) + if !self.force && !self.yes { + println!("\nAre you sure you want to uninstall Cortex CLI? [y/N]"); + if !prompt_yes_no()? { + println!("Uninstall cancelled."); + return Ok(()); + } + } + + // Backup if requested (only after user confirms) if self.backup { print_info("Creating backup..."); if let Err(e) = create_backup(&items_to_remove) { @@ -196,15 +205,6 @@ impl UninstallCli { } } - // Confirm before proceeding - if !self.force && !self.yes { - println!("\nAre you sure you want to uninstall Cortex CLI? [y/N]"); - if !prompt_yes_no()? { - println!("Uninstall cancelled."); - return Ok(()); - } - } - // Perform the uninstall println!("\nUninstalling..."); let mut errors: Vec<(PathBuf, String)> = Vec::new(); diff --git a/src/cortex-cli/src/upgrade_cmd.rs b/src/cortex-cli/src/upgrade_cmd.rs index 5c0bcfda..6e556328 100644 --- a/src/cortex-cli/src/upgrade_cmd.rs +++ b/src/cortex-cli/src/upgrade_cmd.rs @@ -3,7 +3,7 @@ //! Uses the Cortex Software Distribution API at software.cortex.foundation //! to check for updates and download new versions. -use anyhow::{Context, Result}; +use anyhow::{Context, Result, bail}; use clap::Parser; use cortex_engine::create_default_client; use std::io::{Write, stdout}; @@ -73,11 +73,10 @@ impl UpgradeCli { "beta" => ReleaseChannel::Beta, "nightly" => ReleaseChannel::Nightly, _ => { - eprintln!( + bail!( "Invalid channel: {}. Use: stable, beta, or nightly", self.channel ); - return Ok(()); } } }; @@ -342,9 +341,12 @@ async fn fetch_and_display_changelog(url: &str) -> Result<()> { // Create HTTP client let client = create_default_client().context("Failed to create HTTP client")?; + // Convert GitHub URLs to raw content URLs (Issue #3651) + let raw_url = convert_to_raw_url(url); + // Fetch the changelog content let response = client - .get(url) + .get(&raw_url) .send() .await .context("Failed to send HTTP request")?; @@ -375,10 +377,12 @@ async fn fetch_and_display_changelog(url: &str) -> Result<()> { return Ok(()); } - // If it's a GitHub URL, we might need to fetch the raw content - let display_content = if url.contains("github.com") && !url.contains("/raw/") { - // Try to extract useful content from HTML (basic approach) - // For GitHub releases, the content might be in HTML format + // Check if content still looks like HTML (fallback stripping) + let display_content = if content.trim_start().starts_with("") + { + // Content is HTML, strip tags strip_html_tags(&content) } else { content @@ -411,6 +415,29 @@ async fn fetch_and_display_changelog(url: &str) -> Result<()> { Ok(()) } +/// Convert GitHub URLs to raw content URLs (Issue #3651) +fn convert_to_raw_url(url: &str) -> String { + // Handle github.com/user/repo/blob/branch/file -> raw.githubusercontent.com/user/repo/branch/file + if url.contains("github.com") && url.contains("/blob/") { + return url + .replace("github.com", "raw.githubusercontent.com") + .replace("/blob/", "/"); + } + + // Handle github.com/user/repo/releases for changelog + if url.contains("github.com") && url.contains("/releases") { + // Try to construct a raw CHANGELOG.md URL + if let Some(repo_part) = url.split("/releases").next() { + return format!("{}/raw/main/CHANGELOG.md", repo_part) + .replace("github.com", "raw.githubusercontent.com") + .replace("/raw/", "/"); + } + } + + // Return original URL if no conversion needed + url.to_string() +} + /// Strip HTML tags from content (basic implementation) fn strip_html_tags(html: &str) -> String { let mut result = String::new();