feat: cortex doctor - System Health Check #306
Conversation
WalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cortex/doctor.py (3)
141-152: Address UP036 lint error - addnoqacomment or configure ruff.The lint error
UP036: Version block is outdated for minimum Python versionfires because the project's minimum Python is already 3.10+, making this check always true at import time.However,
cortex doctoris explicitly designed to validate runtime environments and report issues to users. This check is intentional—it informs users about their Python version even when it passes. To silence the lint:🔎 Apply this diff to suppress the lint warning:
- if sys.version_info >= (3, 10): + if sys.version_info >= (3, 10): # noqa: UP036 self._print_check("PASS", f"Python {version}")Alternatively, configure
ruff.tomlto ignore UP036 for this file if this is a project-wide pattern.
154-161: Relative path forrequirements.txtmay fail when run from different directories.Using
Path("requirements.txt")assumes the current working directory is the project root. If a user runscortex doctorfrom a subdirectory or a different location, this check will incorrectly warn about missing requirements.Consider using the package's installation location or searching upward for the project root:
🔎 Suggested approach:
def _check_dependencies(self) -> None: """Check packages from requirements.txt.""" missing: list[str] = [] - requirements_path = Path("requirements.txt") + # Try to find requirements.txt relative to this module's location + module_dir = Path(__file__).parent.parent + requirements_path = module_dir / "requirements.txt" + + # Fallback to CWD if not found + if not requirements_path.exists(): + requirements_path = Path("requirements.txt") if not requirements_path.exists(): self._print_check("WARN", "No requirements.txt found") return
169-179: Version specifier parsing doesn't handle all pip formats.The current parsing
line.split("==")[0].split(">")[0].split("<")[0]works for common cases but fails on:
pkg[extra]>=1.0→ extractspkg[extrainstead ofpkgpkg~=1.0→ extractspkg~instead ofpkgpkg @ https://...→ extracts full URL portionConsider using
packaging.requirementsfor robust parsing, or add bracket/tilde handling:🔎 Quick fix without adding dependencies:
if line and not line.startswith("#"): - raw_name = line.split("==")[0].split(">")[0].split("<")[0] + # Strip version specifiers and extras + raw_name = line.split("==")[0].split(">=")[0].split("<=")[0] + raw_name = raw_name.split(">")[0].split("<")[0].split("~=")[0] + raw_name = raw_name.split("[")[0].split("@")[0].split(";")[0] + raw_name = raw_name.strip() pkg_name = name_overrides.get(raw_name, raw_name)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/cli.py(4 hunks)cortex/doctor.py(1 hunks)tests/test_doctor.py(1 hunks)
🧰 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/cli.pycortex/doctor.pytests/test_doctor.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_doctor.py
🧬 Code graph analysis (3)
cortex/cli.py (1)
cortex/doctor.py (2)
SystemDoctor(20-448)run_checks(47-93)
cortex/doctor.py (2)
cortex/validators.py (1)
validate_api_key(20-48)cortex/cli.py (1)
doctor(175-179)
tests/test_doctor.py (1)
cortex/doctor.py (5)
SystemDoctor(20-448)_check_python(141-152)_check_dependencies(154-197)_check_gpu_driver(199-234)run_checks(47-93)
🪛 GitHub Actions: CI
cortex/doctor.py
[error] 145-145: ruff check failed. UP036 Version block is outdated for minimum Python version.
🪛 GitHub Actions: Cortex Automation
cortex/doctor.py
[error] 1-1: PythonVersionCheck WARN scenario did not produce the expected warning for Python 3.9.0. Test looked for a warning containing 'Python 3.9.0 (3.10+ required)' but none was found. Command: 'pytest tests/ -v --cov=cortex --cov-report=xml --cov-report=term-missing'.
tests/test_doctor.py
[error] 45-45: TestPythonVersionCheck: WARN scenario did not produce expected warning 'Python 3.9.0'. The test expected a warning about Python 3.9.0 (3.10+ required) but none was found.
🪛 GitHub Check: Lint
cortex/doctor.py
[failure] 145-145: Ruff (UP036)
cortex/doctor.py:145:12: UP036 Version block is outdated for minimum Python version
🔇 Additional comments (7)
cortex/cli.py (2)
174-179: LGTM! Clean integration of the doctor command.The lazy import pattern keeps startup fast, and the method correctly delegates to
SystemDoctor.run_checks()and returns its exit code. This aligns well with the CLI's existing patterns.
620-622: Subparser wiring looks correct.The
doctorsubparser is properly registered and the command dispatch at lines 702-703 correctly routes tocli.doctor().tests/test_doctor.py (1)
93-129: Good isolation strategy for exit code tests.Mocking all individual check methods and then setting
passes/warnings/failuresdirectly is a clean way to test the exit code logic without depending on the real system state.cortex/doctor.py (4)
451-463: LGTM! Clean public entry point.The
run_doctor()function provides a clean API entry point, and the__main__block correctly propagates exit codes for direct script execution.
281-306: Ollama check implementation looks solid.Good practices observed:
- Lazy import of
requestsonly when needed- Reasonable 2-second timeout for the API check
- Clear messaging for installed-but-not-running vs not-installed states
199-234: GPU driver check is well-structured.The check correctly:
- Tries NVIDIA first, then AMD ROCm
- Uses timeouts to prevent hangs
- Treats missing GPU as a warning (not failure) since CPU-only mode is supported
- Provides helpful suggestions
The broad
except Exceptionis acceptable here for defensive error handling in a diagnostic tool.
314-319: Fix typo in suggestion:ANTHROPOL_API_KEY→ANTHROPIC_API_KEY.The suggestion contains a typo that would mislead users trying to configure their API key.
🔎 Apply this diff:
else: self._print_check( "WARN", "No API keys configured (required for cloud models)", - "Configure API key: export ANTHROPOL_API_KEY=sk-... " "or run 'cortex wizard'", + "Configure API key: export ANTHROPIC_API_KEY=sk-ant-... or run 'cortex wizard'", )Note: Also updated
sk-...tosk-ant-...to match the Anthropic key format validated incortex/validators.py.Likely an incorrect or invalid review comment.
Cortex Automation / test errors
2 participants
Related Issue
Closes #245
Summary
Create a diagnostic command that checks system readiness and suggests fixes.
Command:
cortex doctorChecks:
Screen.Recording.2025-12-18.120538.mp4
Checklist
pytest tests/test_doctor.py)[#XX] DescriptionSummary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.