Skip to content

Commit a73f5fc

Browse files
committed
fix(cortex-cli): address validation feedback from PR review
- Fix clippy::bind_instead_of_map in debug_cmd/handlers/config.rs by replacing and_then() with map() since closure always returns Some() - Fix duplicate error output in alias_cmd.rs and plugin_cmd.rs by using std::process::exit(1) after JSON error output instead of bail!() to avoid duplicating error message to stderr - Replace raw .unwrap() calls with safer patterns in dag_cmd/helpers.rs using if-let guards and Option::and_then() instead of panicking - Improve is_read_only_command() in exec_cmd/autonomy.rs to use exact word matching instead of prefix matching to prevent false positives (e.g., 'catfile' no longer matches 'cat')
1 parent 7cf59f0 commit a73f5fc

File tree

5 files changed

+52
-37
lines changed

5 files changed

+52
-37
lines changed

src/cortex-cli/src/alias_cmd.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,8 @@ async fn run_show(args: AliasShowArgs) -> Result<()> {
249249
"error": format!("Alias '{}' does not exist", args.name)
250250
});
251251
println!("{}", serde_json::to_string_pretty(&error)?);
252+
// Exit with error code but don't duplicate error message via bail!()
253+
std::process::exit(1);
252254
}
253255
bail!("Alias '{}' does not exist.", args.name);
254256
}

src/cortex-cli/src/dag_cmd/helpers.rs

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,9 @@ pub fn convert_specs(input: &DagSpecInput) -> Vec<TaskSpec> {
7878
pub fn print_dag_summary(dag: &TaskDag) {
7979
println!("Tasks:");
8080
for task in dag.all_tasks() {
81-
let deps = dag
82-
.get_dependencies(task.id.unwrap())
81+
let deps = task
82+
.id
83+
.and_then(|id| dag.get_dependencies(id))
8384
.map(|d| d.len())
8485
.unwrap_or(0);
8586
let dep_str = if deps > 0 {
@@ -130,7 +131,7 @@ pub fn print_execution_summary(stats: &DagExecutionStats, format: DagOutputForma
130131
"skipped": stats.skipped_tasks,
131132
"duration_secs": stats.total_duration.as_secs_f64()
132133
});
133-
println!("{}", serde_json::to_string_pretty(&output).unwrap());
134+
println!("{}", serde_json::to_string_pretty(&output).expect("JSON serialization should not fail for DagExecutionStats"));
134135
}
135136
DagOutputFormat::Compact => {
136137
println!(
@@ -181,7 +182,7 @@ pub fn print_ascii_graph(dag: &TaskDag, spec: &DagSpecInput) {
181182
for task_id in tasks_at_depth {
182183
if let Some(task) = dag.get_task(*task_id) {
183184
let deps = dag.get_dependencies(*task_id);
184-
let arrow = if deps.map(|d| !d.is_empty()).unwrap_or(false) {
185+
let arrow = if deps.is_some_and(|d| !d.is_empty()) {
185186
"└─► "
186187
} else {
187188
"● "
@@ -206,7 +207,7 @@ pub fn print_dot_graph(dag: &TaskDag, spec: &DagSpecInput) {
206207
println!();
207208

208209
for task in dag.all_tasks() {
209-
let task_id = task.id.unwrap();
210+
let Some(task_id) = task.id else { continue };
210211
let color = match task.status {
211212
TaskStatus::Completed => "green",
212213
TaskStatus::Failed => "red",
@@ -225,7 +226,7 @@ pub fn print_dot_graph(dag: &TaskDag, spec: &DagSpecInput) {
225226
println!();
226227

227228
for task in dag.all_tasks() {
228-
let task_id = task.id.unwrap();
229+
let Some(task_id) = task.id else { continue };
229230
if let Some(deps) = dag.get_dependencies(task_id) {
230231
for dep_id in deps {
231232
println!(" \"{}\" -> \"{}\";", dep_id.inner(), task_id.inner());
@@ -245,14 +246,14 @@ pub fn print_mermaid_graph(dag: &TaskDag, spec: &DagSpecInput) {
245246
// Create task ID to safe name mapping
246247
let mut id_to_name: HashMap<TaskId, String> = HashMap::new();
247248
for task in dag.all_tasks() {
248-
let task_id = task.id.unwrap();
249+
let Some(task_id) = task.id else { continue };
249250
let safe_name = task.name.replace([' ', '-'], "_");
250251
id_to_name.insert(task_id, safe_name.clone());
251252
println!(" {}[{}]", safe_name, task.name);
252253
}
253254

254255
for task in dag.all_tasks() {
255-
let task_id = task.id.unwrap();
256+
let Some(task_id) = task.id else { continue };
256257
if let Some(deps) = dag.get_dependencies(task_id) {
257258
for dep_id in deps {
258259
if let (Some(from), Some(to)) = (id_to_name.get(dep_id), id_to_name.get(&task_id)) {

src/cortex-cli/src/debug_cmd/handlers/config.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,15 +33,15 @@ pub async fn run_config(args: ConfigArgs) -> Result<()> {
3333
global_config_toml // Default to .toml path for display
3434
};
3535

36-
let local_config = std::env::current_dir().ok().and_then(|d| {
36+
let local_config = std::env::current_dir().ok().map(|d| {
3737
let local_toml = d.join(".cortex/config.toml");
3838
let local_json = d.join(".cortex/config.json");
3939
if local_toml.exists() {
40-
Some(local_toml)
40+
local_toml
4141
} else if local_json.exists() {
42-
Some(local_json)
42+
local_json
4343
} else {
44-
Some(local_toml) // Default to .toml path for display
44+
local_toml // Default to .toml path for display
4545
}
4646
});
4747

src/cortex-cli/src/exec_cmd/autonomy.rs

Lines changed: 35 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -90,34 +90,31 @@ impl AutonomyLevel {
9090

9191
/// Check if a command is read-only (safe for read-only mode).
9292
pub fn is_read_only_command(cmd: &str) -> bool {
93-
let read_only_patterns = [
94-
"cat",
95-
"less",
96-
"head",
97-
"tail",
98-
"ls",
99-
"pwd",
100-
"echo",
101-
"whoami",
102-
"date",
103-
"uname",
104-
"ps",
105-
"top",
106-
"git status",
107-
"git log",
108-
"git diff",
109-
"git branch",
110-
"find",
111-
"grep",
112-
"rg",
113-
"fd",
114-
"tree",
115-
"wc",
116-
"file",
93+
let read_only_commands = [
94+
"cat", "less", "head", "tail", "ls", "pwd", "echo", "whoami", "date", "uname", "ps", "top",
95+
"find", "grep", "rg", "fd", "tree", "wc", "file",
11796
];
97+
let read_only_git_subcommands = ["git status", "git log", "git diff", "git branch"];
11898

11999
let cmd_lower = cmd.to_lowercase();
120-
read_only_patterns.iter().any(|p| cmd_lower.starts_with(p))
100+
101+
// Check git subcommands first (they contain spaces)
102+
if read_only_git_subcommands
103+
.iter()
104+
.any(|p| cmd_lower.starts_with(p))
105+
{
106+
return true;
107+
}
108+
109+
// Extract the first word (the command itself) for exact matching
110+
let first_word = cmd_lower.split_whitespace().next().unwrap_or("");
111+
112+
// Also check for absolute paths (e.g., /bin/cat, /usr/bin/ls)
113+
let command_name = first_word.rsplit('/').next().unwrap_or(first_word);
114+
115+
read_only_commands
116+
.iter()
117+
.any(|p| command_name == *p || first_word == *p)
121118
}
122119

123120
#[cfg(test)]
@@ -146,12 +143,25 @@ mod tests {
146143

147144
#[test]
148145
fn test_is_read_only_command() {
146+
// Basic read-only commands
149147
assert!(is_read_only_command("cat file.txt"));
150148
assert!(is_read_only_command("ls -la"));
151149
assert!(is_read_only_command("git status"));
152150
assert!(is_read_only_command("git log --oneline"));
151+
assert!(is_read_only_command("pwd"));
152+
assert!(is_read_only_command("echo hello"));
153+
assert!(is_read_only_command("/bin/cat file.txt"));
154+
assert!(is_read_only_command("/usr/bin/ls -la"));
155+
156+
// Non-read-only commands
153157
assert!(!is_read_only_command("rm -rf /"));
154158
assert!(!is_read_only_command("git push"));
159+
160+
// Ensure prefix matching doesn't cause false positives (Issue #3820)
161+
assert!(!is_read_only_command("catfile")); // Not "cat file"
162+
assert!(!is_read_only_command("lsmod")); // Not "ls mod"
163+
assert!(!is_read_only_command("datestamp")); // Not "date stamp"
164+
assert!(!is_read_only_command("categorical-analysis")); // Not "cat"
155165
}
156166

157167
#[test]

src/cortex-cli/src/plugin_cmd.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -409,6 +409,8 @@ async fn run_show(args: PluginShowArgs) -> Result<()> {
409409
"error": format!("Plugin '{}' is not installed", args.name)
410410
});
411411
println!("{}", serde_json::to_string_pretty(&error)?);
412+
// Exit with error code but don't duplicate error message via bail!()
413+
std::process::exit(1);
412414
}
413415
bail!("Plugin '{}' is not installed.", args.name);
414416
}

0 commit comments

Comments
 (0)