feat: Consolidate system status and health checks into a single command #369
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CortexCLI
participant Doctor as SystemDoctor
participant Sys as System
User->>CLI: cortex status
CLI->>Doctor: run_checks()
rect rgb(240,248,255)
Note over Doctor,Sys: System Configuration
Doctor->>Sys: _check_api_keys()
Sys-->>Doctor: provider / Ollama status
Doctor->>Sys: _check_security_tools()
Sys-->>Doctor: Firejail path or missing
end
rect rgb(245,245,220)
Note over Doctor,Sys: Diagnostics & Health Checks
Doctor->>Sys: _check_python_version()
Sys-->>Doctor: version result
Doctor->>Sys: _check_gpu_drivers()
Sys-->>Doctor: GPU result
Doctor->>Sys: other checks...
Sys-->>Doctor: aggregated results
end
Doctor-->>CLI: aggregated report (PASS/WARN/FAIL) + exit code
CLI-->>User: formatted status output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR consolidates the cortex doctor and cortex status commands into a single, comprehensive cortex status command, eliminating redundancy and reducing user confusion.
Key Changes:
- The
cortex statuscommand now includes all health checks previously performed bycortex doctor, providing comprehensive system diagnostics including API configuration, security tools, Python dependencies, GPU/CUDA detection, Ollama status, and system resources - Added
_check_security_tools()method to check Firejail availability as part of system configuration checks - Removed the standalone
cortex doctorcommand from CLI routing and argument parsing
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
cortex/doctor.py |
Enhanced with new _check_security_tools() method and reorganized check execution order to prioritize system configuration (API keys and security tools) |
cortex/cli.py |
Removed doctor() method and command routing; updated status() to invoke comprehensive health checks via SystemDoctor |
tests/test_doctor.py |
Added test coverage for new security tools check functionality and updated exit code tests to patch the new method |
docs/COMMANDS.md |
Consolidated documentation by removing separate cortex doctor section and expanding cortex status documentation with comprehensive health check details |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cortex/cli.py (2)
1196-1196: Remove staledoctorcommand from help table.The
doctorcommand has been removed and its functionality merged intostatus, but the help table still lists it. This will confuse users who try to runcortex doctor.🔎 Proposed fix
table.add_row("cache stats", "Show LLM cache statistics") table.add_row("stack <name>", "Install the stack") - table.add_row("doctor", "System health check")
1238-1238: UpdateNETWORK_COMMANDSto replacedoctorwithstatus.The
doctorcommand no longer exists. Sincestatusnow runs comprehensive health checks (including Ollama API check), consider replacingdoctorwithstatusin this list.🔎 Proposed fix
- NETWORK_COMMANDS = ["install", "update", "upgrade", "search", "doctor", "stack"] + NETWORK_COMMANDS = ["install", "update", "upgrade", "search", "status", "stack"]
🧹 Nitpick comments (1)
cortex/doctor.py (1)
21-40: Update class docstring to include security tools check.The docstring lists all checks performed but doesn't mention the new Firejail/security tools check added in
_check_security_tools(). Consider adding it for completeness.Checks for: - Python version compatibility - Required Python dependencies - GPU drivers (NVIDIA/AMD) - CUDA/ROCm availability - Ollama installation and status - API key configuration + - Security tools (Firejail) - Disk space availability - System memory
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cortex/cli.pycortex/doctor.pydocs/COMMANDS.mdtests/test_doctor.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/doctor.pytests/test_doctor.pycortex/cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_doctor.py
🧬 Code graph analysis (2)
tests/test_doctor.py (1)
cortex/doctor.py (1)
_check_security_tools(339-349)
cortex/cli.py (1)
cortex/doctor.py (2)
SystemDoctor(21-478)run_checks(48-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Package
- GitHub Check: Agent
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (9)
cortex/doctor.py (3)
75-78: LGTM!Good reorganization - placing system configuration checks (API provider and security tools) before dependency and GPU checks provides a logical flow for diagnostics.
327-337: LGTM!The Ollama provider fallback logic is correctly implemented - checks the
CORTEX_PROVIDERenvironment variable and provides appropriate messaging for local Ollama users versus those who need to configure API keys.
339-349: LGTM!The
_check_security_tools()method follows the established pattern of other check methods, correctly usesshutil.which()for tool detection, and provides actionable installation guidance.docs/COMMANDS.md (2)
102-136: LGTM!Documentation is comprehensive and accurately reflects the consolidated
cortex statuscommand. The check categories, descriptions, and exit codes align with theSystemDoctorimplementation.
386-390: LGTM!The First-Time Setup workflow is correctly updated to use the consolidated
cortex statuscommand, removing the previously separate health check step.tests/test_doctor.py (2)
88-99: LGTM!Good test coverage for the new
_check_security_tools()method - tests both the success path (Firejail available) and the warning path (Firejail not installed) with appropriate mocking.
109-119: LGTM!The
_check_security_toolspatch is correctly added to maintain test isolation and follows the existing pattern for mocking check methods in exit code tests.cortex/cli.py (2)
790-798: LGTM!The
status()method correctly delegates toSystemDoctor.run_checks(), consolidating all health check functionality into the status command as intended by the PR.
1270-1273: LGTM!The status parser help text correctly reflects the consolidated functionality.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
1196-1196: Remove staledoctorcommand from help table.The
doctorcommand was removed and merged intostatus, but it's still listed in the help output. This will confuse users who try to runcortex doctorand get an error.Proposed fix
table.add_row("notify", "Manage desktop notifications") table.add_row("env", "Manage environment variables") table.add_row("cache stats", "Show LLM cache statistics") table.add_row("stack <name>", "Install the stack") - table.add_row("doctor", "System health check")
🧹 Nitpick comments (2)
cortex/cli.py (2)
790-798: Add return type hint.Per coding guidelines, type hints are required. The method should specify
-> intsincerun_checks()returns an integer exit code.Proposed fix
- def status(self): + def status(self) -> int: """Show comprehensive system status and run health checks""" from cortex.doctor import SystemDoctor
1238-1238: Remove staledoctorfromNETWORK_COMMANDS.The
doctorcommand was removed, but it's still listed inNETWORK_COMMANDS. While harmless, this is dead code that should be cleaned up for consistency.Proposed fix
- NETWORK_COMMANDS = ["install", "update", "upgrade", "search", "doctor", "stack"] + NETWORK_COMMANDS = ["install", "update", "upgrade", "search", "stack"]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/doctor.py (2)
SystemDoctor(21-478)run_checks(48-103)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted. Command: 'black --check . --exclude "(venv|.venv|build|dist)"'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
🔇 Additional comments (2)
cortex/cli.py (2)
1270-1273: LGTM!The status command parser is correctly configured without storing to an unused variable, and the help text accurately reflects the consolidated functionality.
1270-1273: Add docstring and type hints to themain()function.The
main()function (line 1220) is missing both a docstring and a return type hint, violating the coding guidelines requiring type hints in all Python code and docstrings for all public APIs. Lines 1270-1273 themselves are properly formatted.Likely an incorrect or invalid review comment.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
1196-1196: Remove obsolete "doctor" command from help table.The help table still displays the
doctorcommand at line 1196, but this command was removed and merged intostatusas per the PR objectives. This creates a misleading user experience where the help suggests a command that no longer exists.🔎 Proposed fix
- table.add_row("doctor", "System health check")The
statuscommand already covers health checks, so this line should be removed entirely.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/doctor.py (2)
SystemDoctor(21-478)run_checks(48-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Package
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
2 participants
Related Issue
Closes #367
Summary
Merged [cortex doctor] into [cortex status] to eliminate redundant commands and reduce user confusion. The [cortex status]command now provides comprehensive system health checks including:
Changes:
Old Output:
cortex statuscortex doctorNew Output:
Summary by CodeRabbit
New Features
Removed
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.