-
-
Notifications
You must be signed in to change notification settings - Fork 52
FIX: cortex doctor - resolve dependency detection + add polished UI #349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cortex/doctor.py (3)
14-14: Unused import:StatusThe
Statusclass is imported but not used directly. The spinner at line 74 usesconsole.status()which is a method on the Console object and doesn't require theStatusimport.🔎 Proposed fix
-from rich.status import Status
60-61: Remove commented-out code.The commented-out
show_banner()call appears to be dead code from a previous implementation. If it's no longer needed, remove it to keep the codebase clean.🔎 Proposed fix
- # Show banner once - # show_banner()
181-184: Version specifier parsing may miss edge cases.The current parsing doesn't handle all pip version specifiers (e.g.,
>=,~=,!=) or extras syntax likerequests[security]. For example,pkg[extra]>=1.0would yieldpkg[extraafter splitting.Consider using a more robust approach:
🔎 Proposed fix using regex
+import re + def _check_dependencies(self) -> None: """Check packages from requirements.txt.""" missing: list[str] = [] requirements_path = Path("requirements.txt") ... try: with open(requirements_path) as f: for line in f: line = line.strip() if line and not line.startswith("#"): - raw_name = line.split("==")[0].split(">")[0].split("<")[0].strip() + # Extract package name, handling version specifiers and extras + match = re.match(r'^([a-zA-Z0-9_-]+)', line) + if not match: + continue + raw_name = match.group(1) pkg_name = name_overrides.get( raw_name.lower(), raw_name.lower().replace("-", "_") )Alternatively, consider using
packaging.requirements.Requirementfrom thepackaginglibrary for robust parsing if it's already a dependency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/doctor.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/doctor.py
⏰ 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.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
🔇 Additional comments (5)
cortex/doctor.py (5)
73-92: Potential visual artifacts from printing inside spinner context.Printing to the console while inside a
console.status()spinner context can cause visual glitches—the spinner may overwrite or interleave with the printed output. Rich's status spinner is typically used for operations that don't produce intermediate output.Consider either:
- Running checks silently within the spinner, then printing all results afterward, or
- Removing the spinner and keeping the sequential output as before.
Please verify the terminal output behavior to ensure the spinner and check messages display cleanly without flickering or overlap.
101-103: LGTM!Good refactor—delegating to
cx_header()centralizes header styling and follows DRY principles.
169-174: Good fix for the case-insensitive package detection issue.The addition of
python-dotenv→dotenvmapping and the case-normalized lookup with hyphen-to-underscore fallback correctly addresses issue #332's false missing-package reports.
239-243: LGTM!Clear, informative warning message that correctly indicates CPU-only mode is supported while noting the performance impact.
324-328: LGTM!Helpful suggestion that provides users with two options for configuring their API key.
|
Thanks @kr16h |



Related Issue
Closes #332
Summary
PyYAML→yaml,python-dotenv→dotenv...raw_name.replace("-", "_")fallback for hyphensNew Features:
cx_header()for all sectionsdemo
Screen.Recording.2025-12-21.234242.mp4
Checklist
pytest tests/test_cortex_doctor)[#XX] DescriptionSummary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.