Skip to content

Commit 31bd445

Browse files
echobtfactorydroid
andauthored
fix(cortex-cli): implement improvements from code analysis (#174)
This commit addresses multiple improvements identified in IMPROVEMENTS_STATUS.md and CORTEX_CLI_ANALYSIS.md: ## CLI Help Text Improvements (Group A) - Enhanced --ask-for-approval help with detailed values (ask/medium/low/yolo) - Expanded --full-auto help explaining sandbox restrictions and use cases - Added --oss flag help listing all supported providers - Improved --search flag help with detailed capabilities - Added --output-schema help with JSON Schema format example - Enhanced --host help in ServeCommand with security warnings - Improved --selector help in scrape command with CSS selector examples - Improved --user-agent help with common examples ## CLI Validation & Error Handling (Group B/H) - MCP server command now returns proper error instead of silent failure ## CLI Features & Behavior (Group C) - Fixed zip dependency to use workspace version (2.2 vs 0.6) - Removed redundant 'bytes' suffix in debug paths size output ## Agent & Execution (Group G) - Added helpful alternatives when trying to remove built-in agents ## Sessions & Minor Fixes (Group D/E/F) - Added confirmation prompt to logout command - Added data loss warning to --force flag in pr command - Enhanced agent create help with detailed examples - Enhanced PR review automation help text Co-authored-by: Droid Agent <droid@factory.ai>
1 parent 45bd7a1 commit 31bd445

8 files changed

Lines changed: 121 additions & 29 deletions

File tree

cortex-cli/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ base64 = { workspace = true }
5252
reqwest = { workspace = true, features = ["json"] }
5353

5454
# For upgrade command (archive extraction)
55-
zip = "0.6"
55+
zip = { workspace = true }
5656
flate2 = "1.0"
5757
tar = "0.4"
5858

cortex-cli/src/agent_cmd.rs

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -72,11 +72,15 @@ pub struct ShowArgs {
7272
about = "Create a new custom agent",
7373
long_about = "Create a new custom agent with a specific persona and capabilities.\n\n\
7474
Agents can be customized with:\n\
75-
- Custom system prompts\n\
76-
- Tool restrictions (allowed/disallowed tools)\n\
77-
- Model preferences\n\
78-
- Color themes for identification\n\n\
79-
Example: cortex agent create my-agent --model gpt-4o --color '#FF5733'"
75+
- Custom system prompts that define the agent's personality and expertise\n\
76+
- Tool restrictions (allowed/disallowed tools) to limit agent capabilities\n\
77+
- Model preferences for specific use cases\n\
78+
- Color themes for visual identification in the UI\n\
79+
- Delegation settings to control sub-agent spawning\n\n\
80+
Examples:\n\
81+
cortex agent create my-agent --model gpt-4o --color '#FF5733'\n\
82+
cortex agent create reviewer --generate \"A code reviewer focused on security\"\n\
83+
cortex agent create --name helper --non-interactive"
8084
)]
8185
pub struct CreateArgs {
8286
/// Agent name (if not provided, interactive mode will prompt).
@@ -1175,7 +1179,16 @@ async fn run_remove(args: RemoveArgs) -> Result<()> {
11751179
.ok_or_else(|| anyhow::anyhow!("Agent '{}' not found", args.name))?;
11761180

11771181
if agent.native {
1178-
bail!("Cannot remove built-in agent '{}'", args.name);
1182+
bail!(
1183+
"Cannot remove built-in agent '{}'.\n\n\
1184+
Built-in agents are part of the Cortex core and cannot be removed.\n\
1185+
Alternative options:\n\
1186+
1. Create a custom agent to replace it: cortex agent create my-{}\n\
1187+
2. Disable specific tools on the built-in agent by creating a wrapper\n\
1188+
3. Use a different agent with: cortex run --agent <other-agent>",
1189+
args.name,
1190+
args.name
1191+
);
11791192
}
11801193

11811194
let path = agent

cortex-cli/src/debug_cmd.rs

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -495,13 +495,13 @@ fn format_size(bytes: u64) -> String {
495495
const GB: u64 = MB * 1024;
496496

497497
if bytes >= GB {
498-
format!("{:.2} GB ({} bytes)", bytes as f64 / GB as f64, bytes)
498+
format!("{:.2} GB", bytes as f64 / GB as f64)
499499
} else if bytes >= MB {
500-
format!("{:.2} MB ({} bytes)", bytes as f64 / MB as f64, bytes)
500+
format!("{:.2} MB", bytes as f64 / MB as f64)
501501
} else if bytes >= KB {
502-
format!("{:.2} KB ({} bytes)", bytes as f64 / KB as f64, bytes)
502+
format!("{:.2} KB", bytes as f64 / KB as f64)
503503
} else {
504-
format!("{} bytes", bytes)
504+
format!("{} B", bytes)
505505
}
506506
}
507507

@@ -1535,8 +1535,9 @@ mod tests {
15351535

15361536
#[test]
15371537
fn test_format_size() {
1538-
assert_eq!(format_size(500), "500 bytes");
1539-
assert_eq!(format_size(1024), "1.00 KB (1024 bytes)");
1540-
assert_eq!(format_size(1048576), "1.00 MB (1048576 bytes)");
1538+
assert_eq!(format_size(500), "500 B");
1539+
assert_eq!(format_size(1024), "1.00 KB");
1540+
assert_eq!(format_size(1048576), "1.00 MB");
1541+
assert_eq!(format_size(1073741824), "1.00 GB");
15411542
}
15421543
}

cortex-cli/src/github_cmd.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,13 @@ pub struct InstallArgs {
4040
#[arg(short, long)]
4141
pub force: bool,
4242

43-
/// Include PR review automation. When enabled, the agent will automatically review pull requests, suggest improvements, and can approve/request changes based on configured policies
43+
/// Include PR review automation in the workflow.
44+
/// When enabled, the agent will:
45+
/// - Automatically review new and updated pull requests
46+
/// - Analyze code changes for bugs, security issues, and best practices
47+
/// - Suggest improvements with inline comments
48+
/// - Respond to review comments and questions
49+
/// Triggered by: pull_request (opened, synchronize, reopened)
4450
#[arg(long, default_value_t = true)]
4551
pub pr_review: bool,
4652

cortex-cli/src/login.rs

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use cortex_login::{
66
safe_format_key, save_auth_with_fallback,
77
};
88
use std::collections::HashSet;
9-
use std::io::{IsTerminal, Read};
9+
use std::io::{BufRead, IsTerminal, Read};
1010
use std::path::PathBuf;
1111

1212
/// Check for duplicate config override keys and warn the user.
@@ -143,6 +143,34 @@ pub async fn run_logout(config_overrides: CliConfigOverrides) -> ! {
143143
check_duplicate_config_overrides(&config_overrides);
144144
let fabric_home = get_fabric_home();
145145

146+
// Check if user is logged in first
147+
match load_auth_with_fallback(&fabric_home) {
148+
Ok(Some(_)) => {
149+
// User is logged in, ask for confirmation if terminal is interactive
150+
if std::io::stdin().is_terminal() {
151+
eprint!("Are you sure you want to log out? This will remove your stored credentials. [y/N]: ");
152+
let _ = std::io::Write::flush(&mut std::io::stderr());
153+
154+
let mut input = String::new();
155+
if std::io::stdin().read_line(&mut input).is_ok() {
156+
let input = input.trim().to_lowercase();
157+
if input != "y" && input != "yes" {
158+
eprintln!("Logout cancelled.");
159+
std::process::exit(0);
160+
}
161+
}
162+
}
163+
}
164+
Ok(None) => {
165+
eprintln!("Not logged in");
166+
std::process::exit(0);
167+
}
168+
Err(e) => {
169+
eprintln!("Error checking login status: {e}");
170+
std::process::exit(1);
171+
}
172+
}
173+
146174
match logout(&fabric_home, CredentialsStoreMode::default()) {
147175
Ok(true) => {
148176
eprintln!("Successfully logged out");

cortex-cli/src/main.rs

Lines changed: 38 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,13 @@ struct InteractiveArgs {
6565
#[arg(short, long)]
6666
model: Option<String>,
6767

68-
/// Use open-source/local LLM providers. Supported: ollama, lmstudio, llamacpp, vllm, openrouter
68+
/// Use open-source/local LLM providers instead of cloud APIs.
69+
/// Supported providers:
70+
/// ollama - Ollama local LLM server (default port: 11434)
71+
/// lmstudio - LM Studio local server
72+
/// llamacpp - llama.cpp server
73+
/// vllm - vLLM inference server
74+
/// openrouter - OpenRouter API (cloud, supports many OSS models)
6975
#[arg(long = "oss", default_value_t = false)]
7076
oss: bool,
7177

@@ -77,11 +83,20 @@ struct InteractiveArgs {
7783
#[arg(long = "sandbox", short = 's')]
7884
sandbox_mode: Option<String>,
7985

80-
/// Set the approval policy for tool executions. Valid values: ask (always ask), medium (risky only), low (rarely ask), yolo (never ask)
81-
#[arg(long = "ask-for-approval", short = 'a')]
86+
/// Set the approval policy for tool executions.
87+
/// Valid values:
88+
/// ask - Always ask for approval before executing tools
89+
/// medium - Ask for approval only for risky operations
90+
/// low - Rarely ask for approval (only for dangerous operations)
91+
/// yolo - Never ask for approval (DANGEROUS: use with caution)
92+
#[arg(long = "ask-for-approval", short = 'a', value_name = "POLICY")]
8293
approval_policy: Option<String>,
8394

84-
/// Enable fully automatic mode with sandboxed execution. Equivalent to --dangerously-skip-permissions combined with workspace-write sandbox. Use for non-destructive automated workflows
95+
/// Enable fully automatic mode with sandboxed execution.
96+
/// Combines approval_policy=yolo with workspace-write sandbox restrictions.
97+
/// The sandbox ensures operations are confined to the workspace directory.
98+
/// Best suited for automated CI/CD pipelines and non-destructive scripted workflows.
99+
/// For interactive use, consider --ask-for-approval=medium instead.
85100
#[arg(long = "full-auto", default_value_t = false)]
86101
full_auto: bool,
87102

@@ -98,7 +113,13 @@ struct InteractiveArgs {
98113
#[arg(long = "cd", short = 'C', value_name = "DIR")]
99114
cwd: Option<PathBuf>,
100115

101-
/// Enable web search capability. Allows the agent to search the web for up-to-date information, documentation, and code examples during the conversation
116+
/// Enable web search capability for the agent.
117+
/// When enabled, the agent can:
118+
/// - Search the web for up-to-date information and documentation
119+
/// - Find code examples and best practices
120+
/// - Look up library/API documentation
121+
/// - Research error messages and solutions
122+
/// Requires network access. Search results are automatically filtered for relevance.
102123
#[arg(long = "search", default_value_t = false)]
103124
web_search: bool,
104125

@@ -215,8 +236,12 @@ struct ExecCommand {
215236
#[arg(short, long, default_value = "human")]
216237
format: String,
217238

218-
/// Path to a JSON Schema file (.json) that defines the expected structure of the output. The schema should follow JSON Schema draft-07 or later format
219-
#[arg(long)]
239+
/// Path to a JSON Schema file (.json) that defines the expected structure of the output.
240+
/// The schema should follow JSON Schema draft-07 or later format.
241+
/// Example schema:
242+
/// {"type": "object", "properties": {"summary": {"type": "string"}}, "required": ["summary"]}
243+
/// The agent will format its response to match the specified schema structure.
244+
#[arg(long, value_name = "SCHEMA_FILE")]
220245
output_schema: Option<PathBuf>,
221246

222247
/// Agent to use for this request
@@ -377,7 +402,11 @@ struct ServeCommand {
377402
#[arg(short, long, default_value = "3000")]
378403
port: u16,
379404

380-
/// Host to bind to
405+
/// Host address to bind the server to.
406+
/// Examples:
407+
/// 127.0.0.1 - Local only (default, most secure)
408+
/// 0.0.0.0 - All interfaces (accessible from network)
409+
/// Warning: Binding to 0.0.0.0 exposes the server to the network.
381410
#[arg(long, default_value = "127.0.0.1")]
382411
host: String,
383412

@@ -485,8 +514,7 @@ async fn main() -> Result<()> {
485514
Some(Commands::Mcp(mcp_cli)) => mcp_cli.run().await,
486515
Some(Commands::Agent(agent_cli)) => agent_cli.run().await,
487516
Some(Commands::McpServer) => {
488-
eprintln!("MCP server mode not yet implemented");
489-
Ok(())
517+
bail!("MCP server mode is not yet implemented. Use 'cortex mcp' for MCP server management.");
490518
}
491519
Some(Commands::Completion(completion_cli)) => {
492520
generate_completions(completion_cli.shell);

cortex-cli/src/pr_cmd.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,9 @@ pub struct PrCli {
6363
#[arg(short, long)]
6464
pub path: Option<PathBuf>,
6565

66-
/// Force checkout even if there are uncommitted changes. WARNING: This may discard uncommitted work
66+
/// Force checkout even if there are uncommitted changes.
67+
/// WARNING: This may result in data loss! Uncommitted changes in your working
68+
/// directory may be overwritten. Consider using 'git stash' first to save your work.
6769
#[arg(short, long)]
6870
pub force: bool,
6971

cortex-cli/src/scrape_cmd.rs

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,13 @@ pub struct ScrapeCommand {
5656
#[arg(short, long, default_value = "30")]
5757
pub timeout: u64,
5858

59-
/// Custom User-Agent string. Examples: 'Mozilla/5.0 (compatible; Googlebot/2.1)', 'curl/7.68.0', 'PostmanRuntime/7.29.0'
59+
/// Custom User-Agent string to identify the request.
60+
/// Common examples:
61+
/// Mozilla/5.0 (compatible; Googlebot/2.1) - Googlebot
62+
/// Mozilla/5.0 (Windows NT 10.0; Win64) - Windows browser
63+
/// curl/7.68.0 - curl client
64+
/// PostmanRuntime/7.29.0 - Postman
65+
/// Some sites block requests from unknown user agents.
6066
#[arg(long)]
6167
pub user_agent: Option<String>,
6268

@@ -72,7 +78,15 @@ pub struct ScrapeCommand {
7278
#[arg(long)]
7379
pub no_links: bool,
7480

75-
/// CSS selector to extract specific elements. Examples: 'article', '.content', '#main', 'div.post > p', 'table tbody tr'
81+
/// CSS selector to extract specific elements from the page.
82+
/// Examples:
83+
/// article - Select all <article> elements
84+
/// .content - Select elements with class "content"
85+
/// #main - Select element with id "main"
86+
/// div.post > p - Select <p> children of div.post
87+
/// table tbody tr - Select table rows in tbody
88+
/// h1, h2, h3 - Select multiple heading levels
89+
/// [data-id="123"] - Select by attribute
7690
#[arg(long, value_name = "SELECTOR")]
7791
pub selector: Option<String>,
7892

0 commit comments

Comments
 (0)