fix: batch fix for cortex-cli bugs (exit codes, conflicting flags, JSON output, security) #3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add validation for mutually exclusive flags across commands: - agent list: --primary vs --subagents - login: --token, --sso, --device-auth, --with-api-key - exec: --enabled-tools vs --disabled-tools - plugin list: --enabled vs --disabled - mcp add: --url vs --sse - mcp logout: --name vs --all - Improve error handling with JSON support: - alias show: JSON error output - mcp get: JSON error output - plugin show: JSON error output - debug wait: JSON error output - run cmd: auth error JSON output - Fix bug in autonomy.rs: is_read_only_command now uses command string - Add session ID validation in lock_cmd - Support 'last' as SESSION_ID in resume command - Add duplicate task ID check in DAG helpers - Add --jobs validation in DAG execute - Add sort value validation in models list - Add empty plugin name validation - Update help text URLs to point to correct repo - Replace emojis with text in PR checkout - Move confirmation before backup in uninstall - Convert GitHub URLs to raw URLs in upgrade changelog - Support config.json in debug config command - Add dry_run JSON output in compact and DAG commands - Make run_whoami return Result<()> for proper error handling - Change config get/unset to bail! on errors Issues: #3646, #3651, #3682, #3696, #3700, #3716, #3722, #3815
- 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')
echobt
force-pushed
the
fix/cortex-cli-issue-batch-1
branch
from
a73f5fc to
239e0af
Compare
Contributor
Author
|
@greptile |
Greptile OverviewGreptile SummaryThis PR addresses 26+ bug fixes across cortex-cli, focusing on proper exit codes, JSON output consistency, input validation, and a critical security fix in autonomy level checking. Key Changes:
Issues Found:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| src/cortex-cli/src/exec_cmd/autonomy.rs | Fixed critical security bug where risk level string was passed instead of command to is_read_only_command(), added comprehensive tests |
| src/cortex-cli/src/exec_cmd/runner.rs | Added validation for conflicting --enabled-tools and --disabled-tools flags, properly passes command string to allows_risk() |
| src/cortex-cli/src/cli/handlers.rs | Added validation for conflicting login auth methods, fixed whoami error handling, added support for 'last' as session ID, fixed config get/unset error codes |
| src/cortex-cli/src/dag_cmd/helpers.rs | Added duplicate task ID validation using HashSet, improved null-safety with modern Rust patterns (is_some_and, Some(x) else) |
| src/cortex-cli/src/debug_cmd/handlers/config.rs | Fixed detection of both config.toml and config.json, ensured --diff --json outputs pure JSON without mixed text |
| src/cortex-cli/src/plugin_cmd.rs | Added validation for conflicting --enabled/--disabled flags, empty plugin names, and JSON error output for plugin show; uses std::process::exit(1) instead of bail!() |
| src/cortex-cli/src/alias_cmd.rs | Added JSON error output for alias show when alias doesn't exist; uses std::process::exit(1) instead of bail!() |
| src/cortex-cli/src/upgrade_cmd.rs | Fixed invalid channel handling to return error code, implemented GitHub URL to raw content conversion for --changelog |
Sequence Diagram
sequenceDiagram
participant User
participant ExecCli
participant EventLoop
participant AutonomyLevel
participant ReadOnlyCheck
User->>ExecCli: cortex exec --auto read-only
ExecCli->>ExecCli: validate conflicting tool flags
ExecCli->>EventLoop: start execution loop
EventLoop->>EventLoop: receive ExecApprovalRequest
Note over EventLoop: Extract command and risk level
EventLoop->>AutonomyLevel: allows_risk(risk_level, command_str)
AutonomyLevel->>ReadOnlyCheck: is_read_only_command(command_str)
Note over ReadOnlyCheck: FIXED: Now receives actual command<br/>instead of risk level string
ReadOnlyCheck->>ReadOnlyCheck: Extract first word from command
ReadOnlyCheck->>ReadOnlyCheck: Check against whitelist
ReadOnlyCheck-->>AutonomyLevel: true/false
AutonomyLevel-->>EventLoop: permission granted/denied
alt Permission Denied
EventLoop->>User: Error: Command not allowed
else Permission Granted
EventLoop->>EventLoop: auto-approve command
EventLoop->>User: Execute command
end
…lidation Address Greptile review feedback by actually calling the allows_risk() method in the exec command approval flow. Previously, the security fix only added the method but did not integrate it into the command execution path. Changes: - Replace simple AutonomyLevel::ReadOnly check with allows_risk(risk, command) - Extract risk_level from sandbox_assessment if available - Pass actual command string to validate read-only commands properly - Provide clearer error messages including risk level and autonomy mode
echobt
force-pushed
the
fix/cortex-cli-issue-batch-1
branch
from
6d3b2f6 to
d3dea9c
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+407
to
+413
| if args.json { | ||
| let error = serde_json::json!({ | ||
| "error": format!("Plugin '{}' is not installed", args.name) | ||
| }); | ||
| println!("{}", serde_json::to_string_pretty(&error)?); | ||
| // Exit with error code but don't duplicate error message via bail!() | ||
| std::process::exit(1); |
There was a problem hiding this comment.
using std::process::exit(1) here bypasses proper error handling and resource cleanup. The mcp_cmd/handlers.rs:133 uses bail!() after printing JSON, which is the correct pattern
Suggested change
| if args.json { | |
| let error = serde_json::json!({ | |
| "error": format!("Plugin '{}' is not installed", args.name) | |
| }); | |
| println!("{}", serde_json::to_string_pretty(&error)?); | |
| // Exit with error code but don't duplicate error message via bail!() | |
| std::process::exit(1); | |
| 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); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cortex-cli/src/plugin_cmd.rs
Line: 407:413
Comment:
using `std::process::exit(1)` here bypasses proper error handling and resource cleanup. The `mcp_cmd/handlers.rs:133` uses `bail!()` after printing JSON, which is the correct pattern
```suggestion
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);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+247
to
+253
| if args.json { | ||
| let error = serde_json::json!({ | ||
| "error": format!("Alias '{}' does not exist", args.name) | ||
| }); | ||
| println!("{}", serde_json::to_string_pretty(&error)?); | ||
| // Exit with error code but don't duplicate error message via bail!() | ||
| std::process::exit(1); |
There was a problem hiding this comment.
using std::process::exit(1) here bypasses proper error handling and resource cleanup. The mcp_cmd/handlers.rs:133 uses bail!() after printing JSON, which is the correct pattern
Suggested change
| if args.json { | |
| let error = serde_json::json!({ | |
| "error": format!("Alias '{}' does not exist", args.name) | |
| }); | |
| println!("{}", serde_json::to_string_pretty(&error)?); | |
| // Exit with error code but don't duplicate error message via bail!() | |
| std::process::exit(1); | |
| 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); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cortex-cli/src/alias_cmd.rs
Line: 247:253
Comment:
using `std::process::exit(1)` here bypasses proper error handling and resource cleanup. The `mcp_cmd/handlers.rs:133` uses `bail!()` after printing JSON, which is the correct pattern
```suggestion
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);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+352
to
+354
| return format!("{}/raw/main/CHANGELOG.md", repo_part) | ||
| .replace("github.com", "raw.githubusercontent.com") | ||
| .replace("/raw/", "/"); |
There was a problem hiding this comment.
the /raw/ replacement on line 354 removes the /raw/ that was just added on line 352, resulting in an incorrect URL format. The correct raw URL format should be raw.githubusercontent.com/user/repo/main/CHANGELOG.md (without /raw/)
Suggested change
| return format!("{}/raw/main/CHANGELOG.md", repo_part) | |
| .replace("github.com", "raw.githubusercontent.com") | |
| .replace("/raw/", "/"); | |
| return format!("{}/main/CHANGELOG.md", repo_part) | |
| .replace("github.com", "raw.githubusercontent.com"); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cortex-cli/src/upgrade_cmd.rs
Line: 352:354
Comment:
the `/raw/` replacement on line 354 removes the `/raw/` that was just added on line 352, resulting in an incorrect URL format. The correct raw URL format should be `raw.githubusercontent.com/user/repo/main/CHANGELOG.md` (without `/raw/`)
```suggestion
return format!("{}/main/CHANGELOG.md", repo_part)
.replace("github.com", "raw.githubusercontent.com");
```
How can I resolve this? If you propose a fix, please make it concise.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses multiple open issues from the PlatformNetwork/bounty-challenge repository affecting cortex-cli.
Issues Fixed
Exit Code Issues
cortex debug filenow returns non-zero exit code when file does not existwhoamicommand now returns error when checking login status failsupgradecommand now returns error for invalid channel (stable/beta/nightly)config get/unsetnow return error when key not foundConflicting Flags Validation
execcommand validates that--enabled-toolsand--disabled-toolscannot both be specifiedmcp logoutvalidates that--nameand--allcannot both be specifiedagent listvalidates that--primaryand--subagentscannot both be specifiedplugin listvalidates that--enabledand--disabledcannot both be specifiedmcp addvalidates that--urland--ssecannot both be specifiedloginvalidates that only one auth method can be specifiedJSON Output Issues
debug config --diff --jsonnow outputs pure JSONdag run --strategy dry-runrespects--formatflagcompact run --dry-run --jsonoutputs JSONdag list --format jsonoutputs proper JSONalias show --jsonoutputs JSON on errorsplugin show --jsonoutputs JSON on errorsdebug wait --jsonoutputs JSON on errorsmcp get --jsonoutputs JSON on errorsrun --format jsonoutputs JSON on auth errorsSecurity Fix
allows_risk()to pass actual command tois_read_only_command()instead of risk levelInput Validation Issues
models list --sortvalidates sort values (name, provider, context, created, id)dag run --jobs 0now errors instead of hangingplugin install ""now returns proper error for empty namelock addnow validates session ID format (UUID or 8-char prefix)User Experience Fixes
resumecommand now accepts 'last' as SESSION_ID per help textupgrade --changelognow fetches raw content instead of HTMLdebug confignow correctly detects config.json filesuninstall --backupnow creates backup after user confirmationprcommand output (replaced with text alternatives)Changes
bail!()macro--jsonflag output valid JSON even on errorsTesting
cargo check -p cortex-clipassesNote
This PR is for review only - DO NOT MERGE until reviewed.