Skip to content

Commit 239e0af

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 239e0af

File tree

7 files changed

+130
-107
lines changed

7 files changed

+130
-107
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: 13 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,11 @@ 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!(
135+
"{}",
136+
serde_json::to_string_pretty(&output)
137+
.expect("JSON serialization should not fail for DagExecutionStats")
138+
);
134139
}
135140
DagOutputFormat::Compact => {
136141
println!(
@@ -181,7 +186,7 @@ pub fn print_ascii_graph(dag: &TaskDag, spec: &DagSpecInput) {
181186
for task_id in tasks_at_depth {
182187
if let Some(task) = dag.get_task(*task_id) {
183188
let deps = dag.get_dependencies(*task_id);
184-
let arrow = if deps.map(|d| !d.is_empty()).unwrap_or(false) {
189+
let arrow = if deps.is_some_and(|d| !d.is_empty()) {
185190
"└─► "
186191
} else {
187192
"● "
@@ -206,7 +211,7 @@ pub fn print_dot_graph(dag: &TaskDag, spec: &DagSpecInput) {
206211
println!();
207212

208213
for task in dag.all_tasks() {
209-
let task_id = task.id.unwrap();
214+
let Some(task_id) = task.id else { continue };
210215
let color = match task.status {
211216
TaskStatus::Completed => "green",
212217
TaskStatus::Failed => "red",
@@ -225,7 +230,7 @@ pub fn print_dot_graph(dag: &TaskDag, spec: &DagSpecInput) {
225230
println!();
226231

227232
for task in dag.all_tasks() {
228-
let task_id = task.id.unwrap();
233+
let Some(task_id) = task.id else { continue };
229234
if let Some(deps) = dag.get_dependencies(task_id) {
230235
for dep_id in deps {
231236
println!(" \"{}\" -> \"{}\";", dep_id.inner(), task_id.inner());
@@ -245,14 +250,14 @@ pub fn print_mermaid_graph(dag: &TaskDag, spec: &DagSpecInput) {
245250
// Create task ID to safe name mapping
246251
let mut id_to_name: HashMap<TaskId, String> = HashMap::new();
247252
for task in dag.all_tasks() {
248-
let task_id = task.id.unwrap();
253+
let Some(task_id) = task.id else { continue };
249254
let safe_name = task.name.replace([' ', '-'], "_");
250255
id_to_name.insert(task_id, safe_name.clone());
251256
println!(" {}[{}]", safe_name, task.name);
252257
}
253258

254259
for task in dag.all_tasks() {
255-
let task_id = task.id.unwrap();
260+
let Some(task_id) = task.id else { continue };
256261
if let Some(deps) = dag.get_dependencies(task_id) {
257262
for dep_id in deps {
258263
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/debug_cmd/handlers/file.rs

Lines changed: 65 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -28,73 +28,73 @@ pub async fn run_file(args: FileArgs) -> Result<()> {
2828
let special_file_type = detect_special_file_type(&path);
2929

3030
let (metadata, error) = match std::fs::metadata(&path) {
31-
Ok(meta) => {
32-
let modified = meta
33-
.modified()
31+
Ok(meta) => {
32+
let modified = meta
33+
.modified()
34+
.ok()
35+
.map(|t| chrono::DateTime::<chrono::Utc>::from(t).to_rfc3339());
36+
let created = meta
37+
.created()
38+
.ok()
39+
.map(|t| chrono::DateTime::<chrono::Utc>::from(t).to_rfc3339());
40+
41+
// Get symlink target if applicable
42+
let symlink_target = if meta.file_type().is_symlink() {
43+
std::fs::read_link(&path)
3444
.ok()
35-
.map(|t| chrono::DateTime::<chrono::Utc>::from(t).to_rfc3339());
36-
let created = meta
37-
.created()
38-
.ok()
39-
.map(|t| chrono::DateTime::<chrono::Utc>::from(t).to_rfc3339());
45+
.map(|p| p.to_string_lossy().to_string())
46+
} else {
47+
None
48+
};
4049

41-
// Get symlink target if applicable
42-
let symlink_target = if meta.file_type().is_symlink() {
43-
std::fs::read_link(&path)
44-
.ok()
45-
.map(|p| p.to_string_lossy().to_string())
46-
} else {
47-
None
48-
};
49-
50-
// Check if the current user can actually write to the file
51-
// This is more accurate than just checking permission bits
52-
// Skip this check for special files (FIFOs, etc.) to avoid blocking
53-
let readonly = if special_file_type.is_some() {
54-
false // Don't check writability for special files
55-
} else {
56-
!is_writable_by_current_user(&path)
57-
};
58-
59-
// Check if this is a virtual filesystem (procfs, sysfs, etc.)
60-
// These report size=0 in stat() but may have actual content
61-
let is_virtual_fs = is_virtual_filesystem(&path);
62-
let stat_size = meta.len();
63-
64-
// For virtual filesystem files that report 0 size, try to read actual content size
65-
let actual_size = if is_virtual_fs && stat_size == 0 && meta.is_file() {
66-
// Try to read the file to get actual content size
67-
// Limit read to 1MB to avoid hanging on infinite streams
68-
match std::fs::read(&path) {
69-
Ok(content) if !content.is_empty() => Some(content.len() as u64),
70-
_ => None,
71-
}
72-
} else {
73-
None
74-
};
75-
76-
// Get file permissions
77-
let (permissions, mode) = get_unix_permissions(&meta);
78-
79-
(
80-
Some(FileMetadata {
81-
size: stat_size,
82-
actual_size,
83-
is_virtual_fs: if is_virtual_fs { Some(true) } else { None },
84-
is_file: meta.is_file(),
85-
is_dir: meta.is_dir(),
86-
is_symlink: meta.file_type().is_symlink(),
87-
file_type: special_file_type.clone(),
88-
symlink_target,
89-
modified,
90-
created,
91-
readonly,
92-
permissions,
93-
mode,
94-
}),
95-
None,
96-
)
97-
}
50+
// Check if the current user can actually write to the file
51+
// This is more accurate than just checking permission bits
52+
// Skip this check for special files (FIFOs, etc.) to avoid blocking
53+
let readonly = if special_file_type.is_some() {
54+
false // Don't check writability for special files
55+
} else {
56+
!is_writable_by_current_user(&path)
57+
};
58+
59+
// Check if this is a virtual filesystem (procfs, sysfs, etc.)
60+
// These report size=0 in stat() but may have actual content
61+
let is_virtual_fs = is_virtual_filesystem(&path);
62+
let stat_size = meta.len();
63+
64+
// For virtual filesystem files that report 0 size, try to read actual content size
65+
let actual_size = if is_virtual_fs && stat_size == 0 && meta.is_file() {
66+
// Try to read the file to get actual content size
67+
// Limit read to 1MB to avoid hanging on infinite streams
68+
match std::fs::read(&path) {
69+
Ok(content) if !content.is_empty() => Some(content.len() as u64),
70+
_ => None,
71+
}
72+
} else {
73+
None
74+
};
75+
76+
// Get file permissions
77+
let (permissions, mode) = get_unix_permissions(&meta);
78+
79+
(
80+
Some(FileMetadata {
81+
size: stat_size,
82+
actual_size,
83+
is_virtual_fs: if is_virtual_fs { Some(true) } else { None },
84+
is_file: meta.is_file(),
85+
is_dir: meta.is_dir(),
86+
is_symlink: meta.file_type().is_symlink(),
87+
file_type: special_file_type.clone(),
88+
symlink_target,
89+
modified,
90+
created,
91+
readonly,
92+
permissions,
93+
mode,
94+
}),
95+
None,
96+
)
97+
}
9898
Err(e) => (None, Some(e.to_string())),
9999
};
100100

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
}

src/cortex-tui/src/runner/login_screen.rs

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ impl LoginScreen {
223223
return Ok(result);
224224
}
225225
}
226-
226+
227227
// Handle async messages from token polling
228228
msg = async {
229229
if let Some(ref mut rx) = self.async_rx {
@@ -237,7 +237,7 @@ impl LoginScreen {
237237
self.handle_async_message(msg);
238238
}
239239
}
240-
240+
241241
// Periodic render tick to keep UI responsive
242242
_ = render_interval.tick() => {
243243
// Just continue to re-render
@@ -713,7 +713,7 @@ async fn poll_for_token_async(
713713
tx: mpsc::Sender<AsyncMessage>,
714714
) {
715715
tracing::debug!("Token polling started");
716-
716+
717717
let client = match cortex_engine::create_default_client() {
718718
Ok(c) => c,
719719
Err(e) => {
@@ -736,7 +736,11 @@ async fn poll_for_token_async(
736736
return;
737737
}
738738

739-
tracing::trace!("Polling for token (attempt {}/{})", attempt + 1, max_attempts);
739+
tracing::trace!(
740+
"Polling for token (attempt {}/{})",
741+
attempt + 1,
742+
max_attempts
743+
);
740744

741745
let response = match client
742746
.post(format!("{}/auth/device/token", API_BASE_URL))
@@ -756,7 +760,7 @@ async fn poll_for_token_async(
756760

757761
if status.is_success() {
758762
tracing::debug!("Token response received (success)");
759-
763+
760764
#[derive(serde::Deserialize)]
761765
struct TokenResponse {
762766
access_token: String,

0 commit comments

Comments
 (0)