feat: Add system health monitoring module (#128) #292
Conversation
WalkthroughAdds a new system health feature: a Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CortexCLI
participant Monitor as HealthMonitor
participant Disk as DiskCheck
participant Perf as PerformanceCheck
participant Sec as SecurityCheck
participant Upd as UpdateCheck
participant FS as FileSystem/OS
participant Console as Console
User->>CLI: cortex health
CLI->>Monitor: run_all()
Monitor->>Monitor: import/register checks (Disk,Perf,Sec,Upd)
par Run checks concurrently
Monitor->>Disk: run() (reads FS)
Disk-->>Monitor: CheckResult
Monitor->>Perf: run() (reads load/mem)
Perf-->>Monitor: CheckResult
Monitor->>Sec: run() (calls systemctl, reads sshd_config)
Sec-->>Monitor: CheckResult
Monitor->>Upd: run() (executes apt)
Upd-->>Monitor: CheckResult
end
Monitor->>Monitor: compute weighted total_score
Monitor->>Monitor: build report + timestamp
Monitor->>Monitor: _save_history(report) -> FS
Monitor-->>CLI: report dict
CLI->>Console: render summary, factors, recommendations
Console-->>User: formatted health report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 3 inconclusive)
✅ Passed checks (1 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: 4
🧹 Nitpick comments (15)
cortex/health/checks/updates.py (3)
4-5: Add docstrings for public class and method.As per coding guidelines, docstrings are required for all public APIs. The
UpdateCheckclass andrunmethod lack documentation.class UpdateCheck(HealthCheck): + """Check for pending system updates and security patches.""" + def run(self) -> CheckResult: + """ + Run the update check by querying apt for upgradable packages. + + Returns: + CheckResult with score based on pending updates count. + """ score = 100
37-38: Avoid silently swallowing exceptions.The bare
except Exception: passhides potential issues like subprocess failures or parsing errors. Consider logging the error or returning a result that indicates the check could not complete reliably.- except Exception: - pass # Ignore errors + except Exception as e: + # Log or handle gracefully; return indeterminate result + return CheckResult( + name="System Updates", + category="updates", + score=100, + status="SKIP", + details=f"Check failed: {e}", + weight=0.30 + )
14-17: Add timeout to subprocess.run to prevent hangs.If
apthangs (e.g., waiting for a lock), the health check will block indefinitely. Consider adding a timeout.res = subprocess.run( ["apt", "list", "--upgradable"], - capture_output=True, text=True + capture_output=True, text=True, timeout=30 )scripts/verify_ubuntu_compatibility.py (4)
8-9: Use absolute path for history file.
HISTORY_FILEis a relative path, so the JSON file will be written to whatever directory the script is run from. Consider storing it in a predictable location like the user's home directory.+import pathlib + # File name to store history data -HISTORY_FILE = "security_history.json" +HISTORY_FILE = pathlib.Path.home() / ".cortex" / "security_history.json"You'll also need to ensure the directory exists before writing:
HISTORY_FILE.parent.mkdir(exist_ok=True)
62-62: Remove unnecessary f-string.This f-string has no placeholders. Use a regular string instead.
else: - print(f" Trend: ➡️ Stable") + print(" Trend: ➡️ Stable")
138-239: Refactorverify_security_logicto reduce cognitive complexity.Static analysis reports cognitive complexity of 33 (limit is 15). Extract the firewall check and SSH check logic into separate helper functions to improve readability and maintainability.
def _check_firewall() -> tuple[bool, bool]: """Check UFW status. Returns (is_active, needs_fix).""" print("\n[1] Checking Firewall (UFW)...") try: print(" Running: systemctl is-active ufw") res = subprocess.run( ["systemctl", "is-active", "ufw"], capture_output=True, text=True, timeout=10 ) output = res.stdout.strip() print(f" Output: '{output}'") if res.returncode == 0 and output == "active": print(" -> JUDGEMENT: Firewall is ACTIVE (Score: 100)") return True, False else: print(" -> JUDGEMENT: Firewall is INACTIVE (Score: 0)") return False, True except FileNotFoundError: print(" -> ERROR: 'systemctl' command not found.") return False, False except Exception as e: print(f" -> ERROR: {e}") return False, False def _check_ssh_config(ssh_config: str) -> tuple[int, bool]: """Check SSH config. Returns (score_penalty, needs_fix).""" # ... extracted logicThen call these from
verify_security_logic().
75-82: Add timeout to subprocess calls.The
subprocess.runcall for enabling UFW could hang. Add a timeout parameter.try: # Depends on execution environment, sudo might be required - subprocess.run(["sudo", "ufw", "enable"], check=True) + subprocess.run(["sudo", "ufw", "enable"], check=True, timeout=30) print(" -> ✅ Success: Firewall enabled.") return True - except subprocess.CalledProcessError as e: + except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as e: print(f" -> ❌ Failed to enable firewall: {e}") return Falsecortex/health/checks/disk.py (1)
4-6: Add docstrings and use_for unused variable.Per coding guidelines, docstrings are required for public APIs. Also, rename
freeto_since it's unused.class DiskCheck(HealthCheck): + """Check disk space usage on root filesystem.""" + def run(self) -> CheckResult: - total, used, free = shutil.disk_usage("/") + """ + Check disk usage percentage and return health score. + + Returns: + CheckResult with score based on disk usage thresholds. + """ + total, used, _ = shutil.disk_usage("/")cortex/cli.py (1)
177-181: Consider wrapping HealthMonitor in try-except.If any health check raises an unexpected exception, it could crash the CLI. While
HealthMonitor.run_allhas internal exception handling, wrapping the call adds defense-in-depth.from cortex.health.monitor import HealthMonitor self._print_status("🔍", "Running system health checks...") - monitor = HealthMonitor() - report = monitor.run_all() + try: + monitor = HealthMonitor() + report = monitor.run_all() + except Exception as e: + self._print_error(f"Health check failed: {e}") + return 1cortex/health/checks/security.py (3)
5-6: Add docstrings for public class and method.Per coding guidelines, docstrings are required for all public APIs.
class SecurityCheck(HealthCheck): + """Check security configuration including firewall and SSH settings.""" + def run(self) -> CheckResult: + """ + Run security checks for firewall status and SSH configuration. + + Returns: + CheckResult with security score based on detected issues. + """ score = 100
14-17: Add timeout to subprocess.run.The subprocess call could hang if systemctl is unresponsive.
res = subprocess.run( ["systemctl", "is-active", "ufw"], - capture_output=True, text=True + capture_output=True, text=True, timeout=10 )You'll also need to handle
subprocess.TimeoutExpired:- except FileNotFoundError: + except (FileNotFoundError, subprocess.TimeoutExpired): pass # Environment without systemctl (e.g., Docker or non-systemd)
11-27: Consider extracting firewall check to reduce complexity.Static analysis reports cognitive complexity of 16 (limit 15). While only marginally over, extracting the firewall check into a helper method would improve readability and make future security checks easier to add.
def _check_firewall(self) -> tuple[bool, str, str]: """Check if UFW firewall is active. Returns: Tuple of (is_active, issue_text, recommendation_text) """ try: res = subprocess.run( ["systemctl", "is-active", "ufw"], capture_output=True, text=True, timeout=10 ) if res.returncode == 0 and res.stdout.strip() == "active": return True, None, None except (FileNotFoundError, subprocess.TimeoutExpired): pass return False, "Firewall Inactive", "Enable UFW Firewall"tests/test_health_monitor.py (3)
32-60: Consider adding a test for the high-memory penalty path in PerformanceCheckYou cover the “no penalty” and “high load” scenarios, but never drive the
used_percent > 80branch. A small additional test would lock in behaviour for memory pressure and guard future changes, e.g. using a lowMemAvailableto assert a reduced score and"High Memory"indetails.
61-84: Optionally add coverage for SSH root-login penalty in SecurityCheckCurrent tests validate UFW inactive/active flows but not the
PermitRootLogin yescase that should subtract 50 points and add a"Root SSH Allowed"issue. A focused test that patchesos.path.existsandopenforsshd_configwould tighten coverage on a security-sensitive branch.
110-135: HealthMonitor aggregation test is solid; you could also assert status/details if desiredThe weighted-average calculation and result-count assertions verify the core aggregation logic and history-skipping behaviour. As a small enhancement, you could also assert that the per-check entries in
report["results"]preserve the inputname/categoryand thattotal_scoreis anint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cortex/cli.py(9 hunks)cortex/health/checks/disk.py(1 hunks)cortex/health/checks/performance.py(1 hunks)cortex/health/checks/security.py(1 hunks)cortex/health/checks/updates.py(1 hunks)cortex/health/monitor.py(1 hunks)scripts/verify_ubuntu_compatibility.py(1 hunks)tests/test_health_monitor.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/health/checks/disk.pyscripts/verify_ubuntu_compatibility.pytests/test_health_monitor.pycortex/cli.pycortex/health/monitor.pycortex/health/checks/performance.pycortex/health/checks/security.pycortex/health/checks/updates.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_health_monitor.py
🧬 Code graph analysis (6)
cortex/health/checks/disk.py (3)
cortex/health/monitor.py (2)
HealthCheck(22-26)CheckResult(12-20)cortex/health/checks/performance.py (1)
run(6-63)cortex/health/checks/updates.py (1)
run(5-56)
cortex/cli.py (2)
cortex/first_run_wizard.py (1)
save_config(146-152)cortex/health/monitor.py (2)
HealthMonitor(28-102)run_all(52-87)
cortex/health/monitor.py (4)
cortex/health/checks/security.py (1)
SecurityCheck(5-57)cortex/health/checks/updates.py (1)
UpdateCheck(4-56)cortex/health/checks/performance.py (1)
PerformanceCheck(5-63)cortex/health/checks/disk.py (1)
DiskCheck(4-33)
cortex/health/checks/performance.py (2)
cortex/health/monitor.py (2)
HealthCheck(22-26)CheckResult(12-20)cortex/health/checks/disk.py (1)
run(5-33)
cortex/health/checks/security.py (3)
cortex/health/monitor.py (1)
CheckResult(12-20)cortex/health/checks/disk.py (1)
run(5-33)cortex/health/checks/updates.py (1)
run(5-56)
cortex/health/checks/updates.py (1)
cortex/health/monitor.py (2)
HealthCheck(22-26)CheckResult(12-20)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/health/checks/disk.py
[warning] 6-6: Replace the unused local variable "free" with "_".
[warning] 20-20: Fix this condition that always evaluates to false.
scripts/verify_ubuntu_compatibility.py
[warning] 62-62: Add replacement fields or use a normal string instead of an f-string.
[failure] 138-138: Refactor this function to reduce its Cognitive Complexity from 33 to the 15 allowed.
tests/test_health_monitor.py
[warning] 59-59: Remove this commented out code.
cortex/cli.py
[warning] 175-175: Remove the unused function parameter "args".
[warning] 666-666: Remove the unused local variable "health_parser".
cortex/health/checks/security.py
[failure] 6-6: Refactor this function to reduce its Cognitive Complexity from 16 to the 15 allowed.
🔇 Additional comments (6)
cortex/health/checks/disk.py (1)
15-24: LGTM!The threshold-based scoring logic is clear and correctly implements the specification. The static analysis warning about line 20 being always false is a false positive—the
elifbranch is correctly reachable when80 < usage_percent <= 90.cortex/cli.py (2)
174-222: LGTM! Health command is well-structured.The health command implementation is clean:
- Proper integration with
HealthMonitor- Good visual formatting with color-coded scores
- Clear display of factors and recommendations
The unused
argsparameter (flagged by static analysis) is acceptable for consistency with other CLI command method signatures, allowing future extension without signature changes.
664-667: Unusedhealth_parservariable is acceptable.Static analysis flags this as unused, but the pattern matches other subparsers (e.g.,
demo_parser,status_parser). This is fine—keeping the variable allows future addition of arguments to the health command without restructuring.cortex/health/checks/security.py (1)
36-41: LGTM on SSH check logic.Good use of
"yes" in line.split()to avoid matching commented lines like#PermitRootLogin yesand to ensure exact word match rather than substring.tests/test_health_monitor.py (2)
9-31: DiskCheck tests correctly exercise all threshold branchesThe three scenarios (50%, 85%, 95% usage) map cleanly to OK/WARNING/CRITICAL and validate both score and status; mocking of
disk_usageis scoped and appropriate.
85-109: UpdateCheck scoring test matches implementation and is robustThe constructed
apt list --upgradableoutput and the expected84score align with the production scoring formula, and asserting"3 pending"indetailsensures basic messaging is correct.
| with open(config_path, 'w') as f: | ||
| f.writelines(new_lines) | ||
| print(" -> ✅ Success: sshd_config updated.") | ||
|
|
||
| # Attempt to restart SSH service | ||
| print(" -> Restarting sshd service...") | ||
| subprocess.run(["sudo", "systemctl", "restart", "ssh"], check=False) | ||
| return True |
There was a problem hiding this comment.
SSH restart may fail silently and leave config in inconsistent state.
If SSH restart fails (check=False), the user isn't clearly warned that the service may need manual restart. Also consider that if write succeeds but restart fails, the system could be in a partially-updated state.
# Attempt to restart SSH service
print(" -> Restarting sshd service...")
- subprocess.run(["sudo", "systemctl", "restart", "ssh"], check=False)
+ result = subprocess.run(
+ ["sudo", "systemctl", "restart", "ssh"],
+ capture_output=True, text=True, timeout=30
+ )
+ if result.returncode != 0:
+ print(" -> ⚠️ SSH service restart failed. Please restart manually.")
return True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with open(config_path, 'w') as f: | |
| f.writelines(new_lines) | |
| print(" -> ✅ Success: sshd_config updated.") | |
| # Attempt to restart SSH service | |
| print(" -> Restarting sshd service...") | |
| subprocess.run(["sudo", "systemctl", "restart", "ssh"], check=False) | |
| return True | |
| with open(config_path, 'w') as f: | |
| f.writelines(new_lines) | |
| print(" -> ✅ Success: sshd_config updated.") | |
| # Attempt to restart SSH service | |
| print(" -> Restarting sshd service...") | |
| result = subprocess.run( | |
| ["sudo", "systemctl", "restart", "ssh"], | |
| capture_output=True, text=True, timeout=30 | |
| ) | |
| if result.returncode != 0: | |
| print(" -> ⚠️ SSH service restart failed. Please restart manually.") | |
| return True |
🤖 Prompt for AI Agents
In scripts/verify_ubuntu_compatibility.py around lines 119 to 126, the script
currently restarts sshd with subprocess.run(..., check=False) which can fail
silently after writing the new sshd_config; change this to explicitly check the
restart result, create a backup of the original config before writing, attempt
the restart and if it fails restore the backup, print a clear error/warning
instructing manual intervention (or exit non-zero), and ensure you only return
True after a successful restart so the user is not misled.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cortex/health/checks/security.py (3)
11-34: Firewall check implementation is solid.The timeout, exact string match, and error handling for missing systemctl environments are well thought out. The scoring logic correctly sets score to 0 when the firewall is inactive per requirements.
Consider logging or narrowing the generic
except Exceptionblock (lines 28-29) to avoid silently suppressing unexpected errors, though the current approach prioritizes graceful degradation.
36-52: SSH root login check works but could be more precise.The current logic on line 44 checks if
"yes"appears anywhere in the split line. While this works for most well-formed configs, consider verifying that"yes"immediately followsPermitRootLogin:- if line.startswith("PermitRootLogin") and "yes" in line.split(): + parts = line.split() + if len(parts) >= 2 and parts[0] == "PermitRootLogin" and parts[1] == "yes": score -= 50This prevents false positives from malformed lines like
PermitRootLogin no yes # comment.
6-66: Consider extracting helper methods to reduce cognitive complexity.SonarCloud flagged this method with a cognitive complexity of 19 (threshold: 15). Extracting the firewall check (lines 11-34) and SSH check (lines 36-52) into separate helper methods would improve readability and maintainability:
def _check_firewall(self) -> tuple[int, list[str], list[str]]: """Check UFW firewall status.""" # Lines 11-34 logic here # Return (score_delta, issues, recommendations) ... def _check_ssh_root_login(self) -> tuple[int, list[str], list[str]]: """Check SSH root login configuration.""" # Lines 36-52 logic here # Return (score_delta, issues, recommendations) ... def run(self) -> CheckResult: """Execute security checks and return aggregated results.""" score = 100 issues = [] recommendations = [] fw_score, fw_issues, fw_recs = self._check_firewall() score += fw_score # or score = fw_score if firewall sets absolute value issues.extend(fw_issues) recommendations.extend(fw_recs) ssh_score, ssh_issues, ssh_recs = self._check_ssh_root_login() score += ssh_score issues.extend(ssh_issues) recommendations.extend(ssh_recs) # Status determination and return...This refactor would also make the logic easier to unit test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/health/checks/security.py(1 hunks)
🧰 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/health/checks/security.py
🧬 Code graph analysis (1)
cortex/health/checks/security.py (1)
cortex/health/monitor.py (2)
HealthCheck(22-26)CheckResult(12-20)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/health/checks/security.py
[failure] 6-6: Refactor this function to reduce its Cognitive Complexity from 19 to the 15 allowed.
🔇 Additional comments (3)
cortex/health/checks/security.py (3)
1-3: Imports look good.The imports are appropriate for the functionality implemented.
54-56: Status derivation logic is correct.The three-tier status mapping (OK, WARNING, CRITICAL) correctly reflects the score thresholds.
58-66: CheckResult construction looks correct.The score clamping with
max(0, score)properly handles negative scores, and the conditional logic for details and recommendations is sound.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cortex/health/checks/disk.py (1)
7-38: Disk check logic looks solid; consider adding error handling and confirm threshold boundariesThe implementation and scoring bands are clear and consistent;
DiskCheck.rundoes what it claims. The main gap is robustness ifshutil.disk_usage("/")were ever to raise (e.g., unexpected environment or permissions issue). Right now that would bubble up and potentially break the whole health run, unlikeupdates.run, which already guards its external interaction.You could make the check more defensive by wrapping the disk usage call and returning a
CRITICALresult on failure, for example:- # Use _ for unused variable (free space) - total, used, _ = shutil.disk_usage("/") - usage_percent = (used / total) * 100 - + # Use _ for unused variable (free space) + try: + total, used, _ = shutil.disk_usage("/") + usage_percent = (used / total) * 100 + except OSError as exc: + return CheckResult( + name="Disk Usage", + category="disk", + score=0, + status="CRITICAL", + details=f"Failed to read disk usage: {exc}", + recommendation="Check disk configuration and permissions", + weight=0.20, + ) + score = 100 status = "OK" rec = NoneAlso, please double‑check that the boundary behavior is what you want: with
> 90/> 80, exactly 90% usage is treated asWARNINGand exactly 80% asOK. If you intended 90% to be critical and 80% to be warning, switching to>=would make that explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/health/checks/disk.py(1 hunks)cortex/health/checks/security.py(1 hunks)cortex/health/checks/updates.py(1 hunks)
🧰 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/health/checks/disk.pycortex/health/checks/updates.pycortex/health/checks/security.py
🧬 Code graph analysis (3)
cortex/health/checks/disk.py (2)
cortex/health/monitor.py (2)
HealthCheck(22-26)CheckResult(12-20)cortex/health/checks/updates.py (1)
run(7-59)
cortex/health/checks/updates.py (4)
cortex/health/monitor.py (2)
HealthCheck(22-26)CheckResult(12-20)cortex/health/checks/disk.py (1)
run(7-39)cortex/health/checks/security.py (1)
run(8-45)cortex/health/checks/performance.py (1)
run(6-63)
cortex/health/checks/security.py (1)
cortex/health/monitor.py (2)
HealthCheck(22-26)CheckResult(12-20)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/health/checks/disk.py
[warning] 26-26: Fix this condition that always evaluates to false.
cortex/health/checks/security.py
[warning] 58-58: Remove this redundant Exception class; it derives from another which is already caught.
[warning] 58-58: Remove this redundant Exception class; it derives from another which is already caught.
[warning] 73-73: Remove this redundant Exception class; it derives from another which is already caught.
🔇 Additional comments (1)
cortex/health/checks/security.py (1)
5-45: SecurityCheckrunimplementation and public API shape look solidThe scoring, status mapping, and aggregation into
CheckResultare straightforward and consistent with the 0–100 scale, and the class/method docstrings plusrun(self) -> CheckResultsignature satisfy the public API requirements. I don’t see functional or security issues in this segment.As per coding guidelines, docstring coverage and type hints for the public API here look correct.
hyaku0121
changed the title
CI Failing - Action RequiredHey @hyaku0121! Thanks for the health monitoring module. Before we can merge, please fix:
Please push fixes and let us know when ready for re-review! |
There was a problem hiding this comment.
Actionable comments posted: 2
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)
14-31: Sort and format imports — CI pipeline is failing.The import block needs to be sorted per Ruff I001. After removing the duplicate import, ensure imports are properly organized (stdlib → third-party → local).
from cortex.branding import VERSION, console, cx_header, cx_print, show_banner from cortex.coordinator import InstallationCoordinator, StepStatus +from cortex.health.monitor import HealthMonitor from cortex.installation_history import InstallationHistory, InstallationStatus, InstallationType from cortex.llm.interpreter import CommandInterpreter - -# Import the new Notification Manager from cortex.notification_manager import NotificationManager from cortex.user_preferences import ( PreferencesManager, format_preference_value, print_all_preferences, ) from cortex.validators import ( validate_api_key, validate_install_request, ) -# Import Notification Manager -from cortex.notification_manager import NotificationManagerThis also allows removing the lazy import on line 176 since
HealthMonitorwould be imported at the top.
128-134: Inconsistent use of private method_save_config().Line 132 uses
mgr._save_config()while lines 138 and 157 correctly usemgr.save_config(). Use the public method consistently.elif args.notify_action == "enable": mgr.config["enabled"] = True - # Addressing CodeRabbit feedback: Ideally should use a public method instead of private _save_config, - # but keeping as is for a simple fix (or adding a save method to NotificationManager would be best). - mgr._save_config() + mgr.save_config() self._print_success("Notifications enabled") return 0
♻️ Duplicate comments (1)
scripts/verify_ubuntu_compatibility.py (1)
117-125: ReturningTrueafter SSH restart failure is misleading.When the SSH restart fails (line 122-123), the function still returns
True(line 124). This misleads callers into thinking the fix was fully successful when the system is actually in a partially-updated state (config written but service not restarted).if res.returncode != 0: print(f" -> ⚠️ SSH restart failed: {res.stderr}") - return True + print(" -> Config was updated. Please restart SSH manually: sudo systemctl restart ssh") + return False return True
🧹 Nitpick comments (7)
scripts/verify_ubuntu_compatibility.py (6)
12-20: Add type hints to function signatures.Per coding guidelines, type hints are required for Python code. The
load_historyfunction should have a return type annotation.-def load_history(): +def load_history() -> list: """Load past execution history"""
22-39: Add type hints to function signature.-def save_history(score, status, details): +def save_history(score: int, status: str, details: str) -> list: """Save execution result to history"""
41-64: Add type hints to function signature.-def show_trend(history): +def show_trend(history: list) -> None: """Show historical trend (Trend Tracking)"""
134-157: Add return type hint.-def _check_firewall_status(): +def _check_firewall_status() -> bool: """Helper to check firewall status."""
159-185: Add return type hint.-def _check_ssh_status(ssh_config): +def _check_ssh_status(ssh_config: str) -> tuple[int, bool]: """Helper to check SSH status."""
187-232: Add return type hint.-def verify_security_logic(): +def verify_security_logic() -> None: print("=== Ubuntu Security Logic Verification ===")cortex/cli.py (1)
173-221: Clean up trailing whitespace on blank lines.Static analysis flags whitespace on blank lines (W293) at lines 177, 181, 184, 189, 193, 196, 201, 203. While minor, these contribute to lint noise.
The
health()method implementation itself is well-structured with good color-coded output and clear recommendations display.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cortex/cli.py(8 hunks)cortex/health/checks/disk.py(1 hunks)cortex/health/checks/security.py(1 hunks)cortex/health/checks/updates.py(1 hunks)cortex/health/monitor.py(1 hunks)scripts/verify_ubuntu_compatibility.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cortex/health/checks/security.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/health/checks/updates.pycortex/health/monitor.pyscripts/verify_ubuntu_compatibility.pycortex/cli.pycortex/health/checks/disk.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Implement audit logging to ~/.cortex/history.db for all package operations
Applied to files:
scripts/verify_ubuntu_compatibility.py
🧬 Code graph analysis (2)
cortex/health/checks/updates.py (4)
cortex/health/monitor.py (2)
HealthCheck(22-28)CheckResult(12-20)cortex/health/checks/security.py (1)
run(28-80)cortex/health/checks/disk.py (1)
run(7-60)cortex/health/checks/performance.py (1)
run(6-63)
cortex/health/checks/disk.py (1)
cortex/health/monitor.py (2)
HealthCheck(22-28)CheckResult(12-20)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 14-14: ruff check failed. Import block is unsorted or unformatted (I001).
🪛 GitHub Check: Lint
cortex/cli.py
[failure] 31-31: Ruff (F811)
cortex/cli.py:31:41: F811 Redefinition of unused NotificationManager from line 20
[failure] 14-31: Ruff (I001)
cortex/cli.py:14:1: I001 Import block is un-sorted or un-formatted
[failure] 203-203: Ruff (W293)
cortex/cli.py:203:1: W293 Blank line contains whitespace
[failure] 201-201: Ruff (W293)
cortex/cli.py:201:1: W293 Blank line contains whitespace
[failure] 196-196: Ruff (W293)
cortex/cli.py:196:1: W293 Blank line contains whitespace
[failure] 193-193: Ruff (W293)
cortex/cli.py:193:1: W293 Blank line contains whitespace
[failure] 189-189: Ruff (W293)
cortex/cli.py:189:1: W293 Blank line contains whitespace
[failure] 184-184: Ruff (W293)
cortex/cli.py:184:1: W293 Blank line contains whitespace
[failure] 181-181: Ruff (W293)
cortex/cli.py:181:1: W293 Blank line contains whitespace
[failure] 177-177: Ruff (W293)
cortex/cli.py:177:1: W293 Blank line contains whitespace
🔇 Additional comments (7)
cortex/health/checks/updates.py (2)
1-6: LGTM! Well-structured imports and constants.The use of a full path constant
APT_CMDfor the apt binary is a good security practice, preventing PATH injection attacks.
8-99: LGTM! Robust implementation with proper error handling.The
UpdateCheckclass follows the established pattern from other health checks. Good improvements from past reviews:
- Security and regular packages are now counted separately (lines 43-46)
- Comprehensive exception handling with meaningful CRITICAL results instead of silent pass
- Consistent weight (0.25) and status thresholds with other checks
cortex/health/checks/disk.py (1)
1-60: LGTM! Clean implementation with proper error handling.The
DiskCheckclass follows PEP 8, includes type hints, has a comprehensive docstring, and implements proper exception handling. The threshold-based scoring with explicit early returns is clear and maintainable.cortex/health/monitor.py (4)
1-10: LGTM! Well-organized imports and console setup.The imports are properly organized and the module-level console instance allows consistent Rich output across the module.
11-20: LGTM! Well-defined data class with clear documentation.The
CheckResultdataclass has comprehensive type hints and inline documentation for each field. The default values forrecommendationandweightare appropriate.
22-28: LGTM! Clean abstract base class definition.
30-131: LGTM! Robust orchestrator with proper error handling and history persistence.The
HealthMonitorclass is well-implemented:
- Dynamic imports in
__init__properly avoid circular dependenciesrun_all()handles per-check exceptions gracefully without stopping the entire run- Weighted scoring algorithm is correct
- History persistence is robust with graceful handling of corrupted JSON and write failures
- The 100-record limit prevents unbounded growth
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
cortex/health/monitor.py (1)
78-88: Failed checks are silently excluded from results.When a check raises an exception, it's logged to console but not included in
results. This means the weighted average is computed only from successful checks, which may inflate the overall score if a critical check fails.Consider adding a fallback result for failed checks:
for check in self.checks: try: result = check.run() results.append(result) total_weighted_score += result.score * result.weight total_weight += result.weight except Exception as e: console.print( f"[red]Error running check {check.__class__.__name__}: {e}[/red]" ) + # Include failed check with zero score to avoid inflating total + results.append(CheckResult( + name=check.__class__.__name__, + category="unknown", + score=0, + status="CRITICAL", + details=f"Check failed: {e}", + recommendation="Investigate check failure", + weight=1.0 + )) + total_weight += 1.0
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cortex/cli.py(8 hunks)cortex/health/checks/disk.py(1 hunks)cortex/health/checks/performance.py(1 hunks)cortex/health/checks/security.py(1 hunks)cortex/health/checks/updates.py(1 hunks)cortex/health/monitor.py(1 hunks)tests/test_health_monitor.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_health_monitor.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/health/checks/security.pycortex/cli.pycortex/health/checks/disk.pycortex/health/checks/performance.pycortex/health/checks/updates.pycortex/health/monitor.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*.py : Docstrings required for all public APIs
Applied to files:
cortex/health/checks/performance.py
🧬 Code graph analysis (6)
cortex/health/checks/security.py (1)
cortex/health/monitor.py (3)
CheckResult(13-22)HealthCheck(25-31)run(29-31)
cortex/cli.py (4)
cortex/notification_manager.py (1)
NotificationManager(13-149)cortex/validators.py (2)
validate_api_key(20-48)validate_install_request(79-106)cortex/first_run_wizard.py (1)
save_config(155-161)cortex/health/monitor.py (2)
HealthMonitor(34-134)run_all(67-110)
cortex/health/checks/disk.py (1)
cortex/health/monitor.py (3)
CheckResult(13-22)HealthCheck(25-31)run(29-31)
cortex/health/checks/performance.py (4)
cortex/health/monitor.py (3)
CheckResult(13-22)HealthCheck(25-31)run(29-31)cortex/health/checks/disk.py (1)
run(9-61)cortex/health/checks/security.py (1)
run(29-83)cortex/health/checks/updates.py (1)
run(21-102)
cortex/health/checks/updates.py (1)
cortex/health/monitor.py (3)
CheckResult(13-22)HealthCheck(25-31)run(29-31)
cortex/health/monitor.py (4)
cortex/health/checks/disk.py (2)
run(9-61)DiskCheck(6-61)cortex/health/checks/performance.py (2)
run(10-72)PerformanceCheck(7-72)cortex/health/checks/security.py (2)
run(29-83)SecurityCheck(12-113)cortex/health/checks/updates.py (2)
run(21-102)UpdateCheck(9-102)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 186-186: ruff check failed. W293 Blank line contains whitespace. Remove trailing whitespace on this line. Command: 'ruff check . --output-format=github'.
🪛 GitHub Check: Lint
cortex/health/checks/security.py
[failure] 113-113: Ruff (W292)
cortex/health/checks/security.py:113:45: W292 No newline at end of file
cortex/cli.py
[failure] 205-205: Ruff (W293)
cortex/cli.py:205:1: W293 Blank line contains whitespace
[failure] 200-200: Ruff (W293)
cortex/cli.py:200:1: W293 Blank line contains whitespace
[failure] 198-198: Ruff (W293)
cortex/cli.py:198:1: W293 Blank line contains whitespace
[failure] 193-193: Ruff (W293)
cortex/cli.py:193:1: W293 Blank line contains whitespace
[failure] 186-186: Ruff (W293)
cortex/cli.py:186:1: W293 Blank line contains whitespace
cortex/health/checks/disk.py
[failure] 61-61: Ruff (W292)
cortex/health/checks/disk.py:61:10: W292 No newline at end of file
cortex/health/checks/performance.py
[failure] 72-72: Ruff (W292)
cortex/health/checks/performance.py:72:10: W292 No newline at end of file
cortex/health/checks/updates.py
[failure] 102-102: Ruff (W292)
cortex/health/checks/updates.py:102:10: W292 No newline at end of file
cortex/health/monitor.py
[failure] 134-134: Ruff (W292)
cortex/health/monitor.py:134:91: W292 No newline at end of file
🔇 Additional comments (4)
cortex/cli.py (4)
171-218: Health monitoring integration looks good.The health command implementation properly integrates with the
HealthMonitor, provides clear visual feedback with color-coded scores and status icons, and displays actionable recommendations.
686-704: Parser definitions are well-structured.The new
notifyandhealthcommand parsers follow argparse best practices and are consistent with the existing command definitions in the file.
738-740: Command dispatcher properly routes to health handler.The dispatcher correctly routes the
healthcommand tocli.health(args), following the established pattern used for other commands.
609-609: Help table entry clearly documents the health command.The help table entry provides a concise description that accurately reflects the command's purpose.
cortex/health/checks/updates.py
Outdated
| import subprocess | ||
|
|
||
| from ..monitor import CheckResult, HealthCheck | ||
|
|
||
| # Command constants (full paths for security) | ||
| APT_CMD = "/usr/bin/apt" | ||
|
|
||
|
|
||
| class UpdateCheck(HealthCheck): | ||
| """Check for pending system updates and security patches. | ||
| This check evaluates the number of available package updates | ||
| and applies score penalties accordingly: | ||
| - Regular packages: -2 points each | ||
| - Security updates: -10 points each | ||
| Attributes: | ||
| None | ||
| """ | ||
|
|
||
| def run(self) -> CheckResult: | ||
| """Check for available updates using apt. | ||
| Returns: | ||
| CheckResult with score based on pending updates. | ||
| """ | ||
| score = 100 | ||
| pkg_count = 0 | ||
| sec_count = 0 | ||
|
|
||
| try: | ||
| # Add timeout to prevent hangs | ||
| res = subprocess.run( | ||
| [APT_CMD, "list", "--upgradable"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=30 | ||
| ) | ||
| lines = res.stdout.splitlines() | ||
|
|
||
| # apt list output header usually takes first line | ||
| for line in lines[1:]: | ||
| if line.strip(): | ||
| if "security" in line.lower(): | ||
| sec_count += 1 | ||
| else: | ||
| pkg_count += 1 | ||
|
|
||
| # Scoring | ||
| score -= (pkg_count * 2) | ||
| score -= (sec_count * 10) | ||
|
|
||
| except subprocess.TimeoutExpired as e: | ||
| return CheckResult( | ||
| name="System Updates", | ||
| category="updates", | ||
| score=0, | ||
| status="CRITICAL", | ||
| details=f"Check timed out: {e}", | ||
| recommendation="Verify package manager configuration", | ||
| weight=0.25 | ||
| ) | ||
| except FileNotFoundError: | ||
| return CheckResult( | ||
| name="System Updates", | ||
| category="updates", | ||
| score=0, | ||
| status="CRITICAL", | ||
| details="apt command not found", | ||
| recommendation="This check requires apt package manager", | ||
| weight=0.25 | ||
| ) | ||
| except OSError as e: | ||
| return CheckResult( | ||
| name="System Updates", | ||
| category="updates", | ||
| score=0, | ||
| status="CRITICAL", | ||
| details=f"Check failed: {e}", | ||
| recommendation="Verify package manager configuration", | ||
| weight=0.25 | ||
| ) | ||
|
|
||
| status = "OK" | ||
| if score < 50: | ||
| status = "CRITICAL" | ||
| elif score < 90: | ||
| status = "WARNING" | ||
|
|
||
| details = f"{pkg_count} packages, {sec_count} security updates pending" | ||
| if pkg_count == 0 and sec_count == 0: | ||
| details = "System up to date" | ||
|
|
||
| return CheckResult( | ||
| name="System Updates", | ||
| category="updates", | ||
| score=max(0, score), | ||
| status=status, | ||
| details=details, | ||
| recommendation="Run 'apt upgrade'" if score < 100 else None, | ||
| weight=0.25 | ||
| ) No newline at end of file |
There was a problem hiding this comment.
Clean implementation with proper error handling.
The update check correctly differentiates between regular and security updates (using if/else to avoid double-counting). The explicit exception handling for TimeoutExpired, FileNotFoundError, and OSError returns appropriate CRITICAL results rather than silently failing. Docstrings and type hints meet coding guidelines.
One minor fix needed per static analysis:
weight=0.25
- )
+ )
+Add a newline at the end of the file to resolve the Ruff W292 lint failure.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Lint
[failure] 102-102: Ruff (W292)
cortex/health/checks/updates.py:102:10: W292 No newline at end of file
🤖 Prompt for AI Agents
In cortex/health/checks/updates.py around lines 1 to 102, the file is missing a
trailing newline which triggers a lint error (Ruff W292); fix it by adding a
single newline character at the end of the file so the file ends with a blank
line.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cortex/cli.py (2)
133-137: Fix AttributeError —save_config()does not exist.Lines 135 and 154 call
mgr.save_config(), butNotificationManageronly provides_save_config(). This will raiseAttributeErrorat runtime. Line 129 correctly usesmgr._save_config().Apply this diff to fix both occurrences:
elif args.notify_action == "disable": mgr.config["enabled"] = False - mgr.save_config() + mgr._save_config() self._print_success("Notifications disabled (Critical alerts will still show)")Also at line 154:
mgr.config["dnd_start"] = args.start mgr.config["dnd_end"] = args.end - mgr.save_config() + mgr._save_config() self._print_success(f"DND Window updated: {args.start} - {args.end}")
182-189: Remove trailing whitespace on blank line — CI is failing.Line 186 contains a blank line with trailing whitespace, causing Ruff W293 error.
score_color = "green" if score < 60: score_color = "red" elif score < 80: score_color = "yellow" - + console.print()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cortex/cli.py(8 hunks)cortex/health/checks/disk.py(1 hunks)cortex/health/checks/performance.py(1 hunks)cortex/health/checks/security.py(1 hunks)cortex/health/checks/updates.py(1 hunks)cortex/health/monitor.py(1 hunks)tests/test_health_monitor.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cortex/health/checks/disk.py
- tests/test_health_monitor.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/health/checks/updates.pycortex/cli.pycortex/health/checks/security.pycortex/health/checks/performance.pycortex/health/monitor.py
🧬 Code graph analysis (4)
cortex/health/checks/updates.py (1)
cortex/health/monitor.py (3)
CheckResult(13-22)HealthCheck(25-31)run(29-31)
cortex/health/checks/security.py (1)
cortex/health/monitor.py (3)
CheckResult(13-22)HealthCheck(25-31)run(29-31)
cortex/health/checks/performance.py (1)
cortex/health/monitor.py (3)
CheckResult(13-22)HealthCheck(25-31)run(29-31)
cortex/health/monitor.py (4)
cortex/health/checks/disk.py (2)
run(9-61)DiskCheck(6-61)cortex/health/checks/performance.py (2)
run(10-72)PerformanceCheck(7-72)cortex/health/checks/security.py (2)
run(29-83)SecurityCheck(12-113)cortex/health/checks/updates.py (2)
run(21-102)UpdateCheck(9-102)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 186-186: ruff check . --output-format=github: W293 Blank line contains whitespace.
🪛 GitHub Actions: Cortex Automation
cortex/cli.py
[error] 199-199: IndentationError: unexpected indent in console.print line.
🪛 GitHub Check: Lint
cortex/cli.py
[failure] 199-199: Ruff (invalid-syntax)
cortex/cli.py:199:1: invalid-syntax: Unexpected indentation
[failure] 198-198: Ruff (W293)
cortex/cli.py:198:1: W293 Blank line contains whitespace
[failure] 186-186: Ruff (W293)
cortex/cli.py:186:1: W293 Blank line contains whitespace
🔇 Additional comments (5)
cortex/health/checks/security.py (1)
1-113: Well-structured security check implementation.The implementation follows security best practices with full command paths, timeouts on subprocess calls, and proper exception handling. Docstrings and type hints meet coding guidelines. The separation of SSH config checking into a private helper method improves maintainability.
One minor fix still needed: ensure the file ends with a trailing newline to satisfy Ruff W292.
except OSError: pass # Other file system errors +cortex/health/checks/performance.py (1)
1-72: Solid cross-platform implementation with appropriate fallbacks.The check correctly handles non-Linux/Unix environments by silently skipping unavailable metrics. Score clamping prevents negative values. Docstrings and type hints meet coding guidelines.
Ensure the file ends with a trailing newline to satisfy Ruff W292:
weight=0.20 # 20% ) +cortex/cli.py (1)
170-178: Good use of deferred import to avoid circular dependencies.The lazy import of
HealthMonitorinside the method prevents potential circular import issues with the health subsystem. This is a sensible pattern for CLI entry points.cortex/health/checks/updates.py (1)
1-102: Clean implementation with proper error handling.The update check correctly differentiates between regular and security updates (using if/else to avoid double-counting). The explicit exception handling for
TimeoutExpired,FileNotFoundError, andOSErrorreturns appropriate CRITICAL results rather than silently failing. Docstrings and type hints meet coding guidelines.Ensure the file ends with a trailing newline to satisfy Ruff W292:
weight=0.25 ) +cortex/health/monitor.py (1)
1-134: Well-designed health monitoring framework.The implementation provides a clean abstraction with:
CheckResultdataclass for standardized check outputsHealthCheckABC enforcing consistent interfaceHealthMonitororchestrator with weighted scoring and history persistenceThe exception handling in
run_allensures one failing check doesn't break the entire health report. The history trimming to 100 entries prevents unbounded file growth.Ensure the file ends with a trailing newline to satisfy Ruff W292:
except Exception as e: console.print(f"[yellow]Warning: Could not save health history: {e}[/yellow]") +
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (2)
cortex/cli.py (2)
152-155: Critical: Same AttributeError issue in DND handler.Same problem as line 135 — calling non-existent
mgr.save_config()instead of the private_save_config()method.Apply this fix:
mgr.config["dnd_start"] = args.start mgr.config["dnd_end"] = args.end - mgr.save_config() + mgr._save_config() self._print_success(f"DND Window updated: {args.start} - {args.end}")Note: If a public
save_config()method is intended, add it toNotificationManagerfirst, then update all three call sites (lines 129, 135, 154) consistently.
133-136: Critical: Calling non-existent method causes AttributeError at runtime.Line 135 calls
mgr.save_config(), butNotificationManageronly provides the private_save_config()method. This will cause anAttributeErrorwhen users runcortex notify disable. Line 129 correctly usesmgr._save_config(), creating an inconsistency.The same issue exists on line 154 in the DND handler.
Apply this fix:
elif args.notify_action == "disable": mgr.config["enabled"] = False - mgr.save_config() + mgr._save_config() self._print_success("Notifications disabled (Critical alerts will still show)")
🧹 Nitpick comments (11)
scripts/github/review-contributor-prs.sh (1)
251-251: Minor: Unused variabledateon line 251.The variable
dateis assigned but never used. Consider removing it or using it in the loop logic.- tail -n +2 "$BOUNTIES_FILE" | while IFS=',' read -r pr dev feature amount date status; do + tail -n +2 "$BOUNTIES_FILE" | while IFS=',' read -r pr dev feature amount _unused status; doscripts/automation/manage_cortex_prs.sh (2)
128-128: Inconsistent quoting in gh command.Line 128 passes reviewer names without quoting:
gh pr edit $pr_num --add-reviewer dhvil,mikejmorgan-ai 2>/dev/null || echo " (Reviewers already assigned)"While this works for simple names, it's safer to quote the variable
$pr_num:gh pr edit "$pr_num" --add-reviewer dhvil,mikejmorgan-ai 2>/dev/null || echo " (Reviewers already assigned)"Apply this quoting pattern consistently throughout the script for robustness.
171-175: Hardcoded bounty mapping is fragile.The bounty amount is determined by simple keyword matching on PR titles. This approach is brittle and doesn't scale. Consider:
- Moving bounty amounts to a configuration file (JSON, CSV, or associative array)
- Fetching bounty amounts from PR labels or linked issues
- Making the mapping logic more robust with exact label matching
scripts/demo_script.sh (1)
14-14: Consider adding error handling forcd.If the directory doesn't exist, the script will continue executing in the wrong directory.
-cd /home/dhaval/projects/open-source/cortex/src +cd /home/dhaval/projects/open-source/cortex/src || { echo "Directory not found"; exit 1; }scripts/deployment/audit_cortex_status.sh (1)
10-13: Unused color variables.
RED,GREEN,YELLOW, andNCare defined but never used in the script output.Either remove the unused variables or use them in the echo statements (e.g.,
echo -e "${GREEN}✅ Workflows directory exists${NC}").scripts/verify_ubuntu_compatibility.py (6)
19-27: Add return type hint.Per coding guidelines, type hints are required. This function returns a list of dictionaries.
-def load_history(): - """Load past execution history""" +def load_history() -> list[dict]: + """Load past execution history. + + Returns: + list[dict]: List of historical execution records. + """
50-73: Add type hint for parameter.-def show_trend(history): - """Show historical trend (Trend Tracking)""" +def show_trend(history: list[dict]) -> None: + """Show historical trend (Trend Tracking)."""
105-110: Add type hints to function signature.-def fix_ssh_config(config_path): - """Disable SSH root login (Automated Fix). +def fix_ssh_config(config_path: str) -> bool: + """Disable SSH root login (Automated Fix).
180-204: Add return type hint.-def _check_firewall_status(): - """Helper to check firewall status.""" +def _check_firewall_status() -> bool: + """Helper to check firewall status."""
207-233: Add type hints for parameter and return tuple.-def _check_ssh_status(ssh_config): - """Helper to check SSH status.""" +def _check_ssh_status(ssh_config: str) -> tuple[int, bool]: + """Helper to check SSH status. + + Args: + ssh_config: Path to sshd_config file. + + Returns: + Tuple of (score_penalty, needs_fix). + """
273-285: Consider adding non-interactive mode for CI/automation.The
input()call will block in non-interactive contexts (CI, cron jobs). Consider adding a flag or environment variable to auto-apply or skip fixes.+ # Allow non-interactive mode via environment variable + auto_fix = os.environ.get("CORTEX_AUTO_FIX", "").lower() == "true" + if ufw_needs_fix or ssh_needs_fix: print("\n=== 🛠️ Automated Fixes Available ===") print("Issues detected that can be automatically fixed.") - user_input = input("Do you want to apply fixes now? (y/n): ").strip().lower() + + if auto_fix: + user_input = "y" + print("Auto-fix enabled via CORTEX_AUTO_FIX environment variable.") + else: + user_input = input("Do you want to apply fixes now? (y/n): ").strip().lower() if user_input == "y":
cortex/utils/commands.py
Outdated
| elif char == "&&" or char == "||": | ||
| # Allow chained commands, but validate each part | ||
| continue |
There was a problem hiding this comment.
Chained commands are allowed without validating each part.
The code allows && and || operators at lines 175-177 with a comment indicating "validate each part," but the implementation simply continues without actually performing per-command validation. This creates a potential security bypass where a dangerous command could be chained after a safe one.
Example: apt update && rm -rf / would pass allowlist validation for apt but would not validate the second dangerous command.
Consider one of these approaches:
- Split and validate each part (recommended):
elif char == "&&" or char == "||":
- # Allow chained commands, but validate each part
- continue
+ # Split chained commands and validate each part
+ parts = re.split(r'\s*(?:&&|\|\|)\s*', command)
+ for part in parts:
+ part_valid, part_error = validate_command(part.strip(), strict=strict)
+ if not part_valid:
+ return False, f"Invalid command in chain: {part_error}"
+ # All parts validated, continue outer loop
+ continue- Disallow chaining entirely (simpler, more secure):
elif char == "&&" or char == "||":
- # Allow chained commands, but validate each part
- continue
+ # Disallow command chaining for security
+ return False, f"Command chaining not allowed: {char}"🤖 Prompt for AI Agents
In cortex/utils/commands.py around lines 175-177, the code currently accepts
"&&" and "||" but does not validate each chained command part; update the logic
to split the command string on the chaining operators (preserving operators or
using a regex to split on '&&' and '||') and run the existing per-command
allowlist/validation routine against every resulting sub-command, rejecting the
whole input if any sub-command fails validation; if splitting is infeasible in
the current flow, alternatively raise an error to disallow chaining entirely and
return a clear validation failure instead.
| def save_history(score, status, details): | ||
| """Save execution result to history""" | ||
| HISTORY_FILE.parent.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| history = load_history() | ||
| record = { | ||
| "timestamp": datetime.datetime.now().isoformat(), | ||
| "score": score, | ||
| "status": status, | ||
| "details": details, | ||
| } | ||
| history.append(record) | ||
| history = history[-10:] | ||
|
|
||
| with open(HISTORY_FILE, "w") as f: | ||
| json.dump(history, f, indent=4) | ||
|
|
||
| return history |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints and error handling for file write.
Missing type hints (required per guidelines). Also, the file write operation can fail silently if disk is full or permissions change after directory creation.
-def save_history(score, status, details):
- """Save execution result to history"""
+def save_history(score: int, status: str, details: str) -> list[dict]:
+ """Save execution result to history.
+
+ Args:
+ score: Security score (0-100).
+ status: Status string (OK, WARNING, CRITICAL).
+ details: Comma-separated issue details.
+
+ Returns:
+ list[dict]: Updated history list.
+ """
HISTORY_FILE.parent.mkdir(parents=True, exist_ok=True)
history = load_history()
record = {
"timestamp": datetime.datetime.now().isoformat(),
"score": score,
"status": status,
"details": details,
}
history.append(record)
history = history[-10:]
- with open(HISTORY_FILE, "w") as f:
- json.dump(history, f, indent=4)
+ try:
+ with open(HISTORY_FILE, "w") as f:
+ json.dump(history, f, indent=4)
+ except OSError as e:
+ print(f" -> ⚠️ Failed to save history: {e}")
return history| if fixed: | ||
| # Atomic write using temporary file | ||
| dir_path = os.path.dirname(config_path) | ||
| with tempfile.NamedTemporaryFile( | ||
| mode="w", dir=dir_path, delete=False, suffix=".tmp" | ||
| ) as tmp_file: | ||
| tmp_file.writelines(new_lines) | ||
| tmp_path = tmp_file.name | ||
|
|
||
| # Atomic replace | ||
| os.replace(tmp_path, config_path) | ||
| print(" -> ✅ Success: sshd_config updated.") | ||
|
|
||
| print(" -> Restarting sshd service...") | ||
| res = subprocess.run( | ||
| [SUDO_CMD, "-n", SYSTEMCTL_CMD, "restart", SSH_SERVICE], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=30, | ||
| ) | ||
| if res.returncode != 0: | ||
| print(f" -> ⚠️ SSH restart failed: {res.stderr}") | ||
| print(" -> Please restart SSH service manually.") | ||
| return True |
There was a problem hiding this comment.
Temp file may leak if os.replace fails; also preserve file permissions.
If os.replace raises an exception, the temp file remains on disk. Also, the atomic write doesn't preserve the original file's permissions, which is important for sshd_config (typically 644).
if fixed:
# Atomic write using temporary file
dir_path = os.path.dirname(config_path)
+ original_mode = os.stat(config_path).st_mode
with tempfile.NamedTemporaryFile(
mode="w", dir=dir_path, delete=False, suffix=".tmp"
) as tmp_file:
tmp_file.writelines(new_lines)
tmp_path = tmp_file.name
- # Atomic replace
- os.replace(tmp_path, config_path)
+ try:
+ os.chmod(tmp_path, original_mode)
+ os.replace(tmp_path, config_path)
+ except OSError:
+ os.unlink(tmp_path) # Clean up temp file on failure
+ raise
print(" -> ✅ Success: sshd_config updated.")🤖 Prompt for AI Agents
In scripts/verify_ubuntu_compatibility.py around lines 144-167, the temporary
file may leak if os.replace raises and the new file doesn’t preserve the
original sshd_config permissions; wrap the atomic write/replace in a
try/except/finally, capture the original file mode/uid/gid via
os.stat(config_path), set the tmp file's permissions and ownership (os.chmod and
os.chown or shutil.copystat) before calling os.replace, and in the
except/finally block remove the tmp file if it still exists to avoid leaks and
re-raise or handle the error appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/test_health_monitor.py (3)
11-33: Consider adding type hints and docstrings to test methods.Per coding guidelines, type hints and docstrings are required for Python code. While test methods are typically less strictly documented, adding return type hints (
-> None) and brief docstrings would improve consistency and maintainability.Example:
- @patch("shutil.disk_usage") - def test_disk_usage_scoring(self, mock_usage): + @patch("shutil.disk_usage") + def test_disk_usage_scoring(self, mock_usage) -> None: + """Test disk usage scoring across healthy, warning, and critical thresholds."""
35-63: Consider adding test coverage for high memory usage scenario.The current tests cover load average penalties but don't test the high memory usage path (>80%). Adding a test case with high memory would improve coverage of the PerformanceCheck logic.
Example test case:
@patch("os.getloadavg") @patch("multiprocessing.cpu_count") def test_high_memory_penalty(self, mock_cpu, mock_load) -> None: """Test memory usage penalty when >80% memory is used.""" mock_cpu.return_value = 4 mock_load.return_value = (1.0, 1.0, 1.0) # Normal load # High memory: 90% used mem_data = "MemTotal: 1000 kB\nMemAvailable: 100 kB\n" with patch("builtins.open", mock_open(read_data=mem_data)): check = PerformanceCheck() result = check.run() # Expected: 100 - 10 (penalty for 90% - 80%) = 90 self.assertEqual(result.score, 90)
65-88: Consider adding test coverage for SSH root login scenario.The tests cover UFW status and missing sshd_config, but don't test the SSH root login check when
PermitRootLogin yesis configured. This scenario applies a -50 point penalty according to the SecurityCheck implementation.Example test case:
@patch("subprocess.run") def test_ssh_root_login_enabled(self, mock_run) -> None: """Test penalty when SSH root login is enabled.""" mock_run.return_value.stdout = "active" mock_run.return_value.returncode = 0 sshd_config_content = "PermitRootLogin yes\n" with patch("os.path.exists", return_value=True), \ patch("builtins.open", mock_open(read_data=sshd_config_content)): check = SecurityCheck() result = check.run() self.assertEqual(result.score, 50) # 100 - 50 penalty self.assertIn("Root SSH Allowed", result.details)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_health_monitor.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:
tests/test_health_monitor.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_health_monitor.py
🧬 Code graph analysis (1)
tests/test_health_monitor.py (5)
cortex/health/checks/disk.py (2)
DiskCheck(6-61)run(9-61)cortex/health/checks/performance.py (2)
PerformanceCheck(7-72)run(10-72)cortex/health/checks/security.py (2)
SecurityCheck(12-106)run(29-80)cortex/health/checks/updates.py (2)
UpdateCheck(9-99)run(21-99)cortex/health/monitor.py (4)
CheckResult(13-22)HealthMonitor(34-132)run(29-31)run_all(67-108)
🪛 GitHub Actions: CI
tests/test_health_monitor.py
[error] 114-114: TestUpdateCheck::test_apt_updates failed: Expected '3 pending' in details, but got '2 packages, 1 security updates pending'.
🪛 GitHub Actions: Cortex Automation
tests/test_health_monitor.py
[error] 114-114: apt updates count mismatch in test_apt_updates. Expected string containing "3 pending" but got "2 packages, 1 security updates pending".
🔇 Additional comments (1)
tests/test_health_monitor.py (1)
117-142: LGTM!The weighted average aggregation test correctly validates the HealthMonitor's core scoring logic. Mocking
_save_historyand overwritingmonitor.checksare appropriate techniques for isolated unit testing.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.py(1 hunks)tests/test_health_monitor.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/test_health_monitor.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 (7)
cortex/branding.py (2)
cx_print(49-69)show_banner(35-46)cortex/coordinator.py (3)
InstallationCoordinator(65-338)StepStatus(31-36)execute(244-293)cortex/installation_history.py (5)
InstallationStatus(34-40)InstallationType(24-31)_extract_packages_from_commands(210-242)record_installation(250-309)rollback(458-565)cortex/notification_manager.py (3)
NotificationManager(13-149)_save_config(48-51)send(92-136)cortex/stack_manager.py (5)
StackManager(17-102)list_stacks(39-42)find_stack(44-49)describe_stack(72-102)suggest_stack(56-70)cortex/validators.py (1)
validate_api_key(20-48)cortex/health/monitor.py (2)
HealthMonitor(34-132)run_all(67-108)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 920-920: Command 'ruff check . --output-format=github' failed with lint error: W292 No newline at end of file.
🪛 GitHub Check: Lint
cortex/cli.py
[failure] 920-920: Ruff (W292)
cortex/cli.py:920:21: W292 No newline at end of file
🔇 Additional comments (2)
cortex/cli.py (2)
173-228: LGTM! Well-structured health command implementation.The health check integration is clean and user-friendly:
- Proper color-coded score display based on severity thresholds
- Clear status icons for each check result
- Actionable recommendations collected and displayed
- Good integration with the HealthMonitor framework
750-750: LGTM! Health command properly wired throughout the CLI.The command is correctly integrated at all three required touch points:
- Help table entry (line 750)
- Argument parser definition (lines 848-849)
- Command dispatch (lines 897-898)
Also applies to: 848-849, 897-898
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.py(1 hunks)
🧰 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 (8)
cortex/branding.py (2)
cx_print(49-69)show_banner(35-46)cortex/notification_manager.py (3)
NotificationManager(13-149)_save_config(48-51)send(92-136)cortex/stack_manager.py (4)
StackManager(17-102)list_stacks(39-42)find_stack(44-49)describe_stack(72-102)cortex/validators.py (2)
validate_api_key(20-48)validate_install_request(79-106)cortex/health/monitor.py (2)
HealthMonitor(34-132)run_all(67-108)cortex/sandbox/sandbox_executor.py (2)
execute(547-675)success(67-69)cortex/packages.py (1)
parse(383-427)cortex/shell_integration.py (1)
suggest_command(9-46)
🔇 Additional comments (2)
cortex/cli.py (2)
750-750: LGTM! Health command integration is complete.The health command is properly integrated into the CLI:
- Added to the help table with clear description (line 750)
- Parser defined correctly (line 848)
- Dispatched following the established pattern (lines 897-898)
The integration is consistent with other commands in the codebase.
Also applies to: 848-848, 897-898
173-173: Add type hints and remove unusedargsparameter.The
argsparameter is accepted but never used in the method body. Add type annotations following the pattern used bystack():def health(self, args: argparse.Namespace) -> int:. Alternatively, if this parameter won't be used, remove it likedoctor()does and just usedef health(self) -> int:.
cortex/cli.py
Outdated
| def health(self, args): | ||
| """Run system health checks and show recommendations""" | ||
| from cortex.health.monitor import HealthMonitor |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints and expand docstring per coding guidelines.
The method signature is missing required type hints, and the docstring doesn't meet the project's documentation standards for public APIs.
🔎 Proposed fix
- def health(self, args):
- """Run system health checks and show recommendations"""
+ def health(self, args: argparse.Namespace) -> int:
+ """Run system health checks and show recommendations.
+
+ Executes all registered health checks, calculates an overall health score,
+ and displays color-coded results with actionable recommendations.
+
+ Args:
+ args: Parsed command-line arguments (currently unused).
+
+ Returns:
+ int: Exit code (0 for success).
+ """As per coding guidelines, type hints and comprehensive docstrings are required for all public APIs.
🤖 Prompt for AI Agents
In cortex/cli.py around lines 173 to 175, the public method health lacks type
annotations and a comprehensive docstring; update the signature to include
explicit types (for example args: argparse.Namespace or a more specific type
used elsewhere, and -> None) and add/import any required typing/argparse
symbols; expand the docstring to the project's standard by describing the method
purpose, parameters (Args), return value (Returns), and possible exceptions
(Raises) with concise examples or notes if required.
|
|
…, error handling)
- Add command constants for security (avoids PATH manipulation) - Use specific exception types instead of bare Exception - Add comprehensive docstrings for classes and methods - Extract SSH check logic to helper method for better maintainability - Apply consistent code formatting
- I001: Sort and organize imports - UP035/UP006: Modernize type hints (List/Dict -> list/dict) - UP045: Use X | None syntax for optional types - UP015: Remove unnecessary mode arguments from open() - W293: Remove whitespace from blank lines - W292: Add newline at end of files - F811: Remove duplicate imports
- Add missing newlines at end of files - Remove whitespace from blank lines
- Add sudo -n for non-interactive execution - Use atomic write pattern for sshd_config - Use full paths for system commands to prevent PATH attacks - Fix ruff linter errors (imports, whitespace, mode args)
- Fix invalid indentation in cli.py:health() - Add missing newline at end of scripts/verify_ubuntu_compatibility.py
hyaku0121
force-pushed
the
feature/health-score-128
branch
from
61c2743 to
7a6d42d
Compare
|
@mikejmorgan-ai |
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
scripts/verify_ubuntu_compatibility.py (2)
30-47: Add type hints and error handling for file write.Missing type hints (required per guidelines). The file write operation can fail silently if disk is full or permissions change.
🔎 Proposed fix
-def save_history(score, status, details): - """Save execution result to history""" +def save_history(score: int, status: str, details: str) -> list[dict]: + """Save execution result to history. + + Args: + score: Security score (0-100). + status: Status string (OK, WARNING, CRITICAL). + details: Comma-separated issue details. + + Returns: + list[dict]: Updated history list. + """ HISTORY_FILE.parent.mkdir(parents=True, exist_ok=True) history = load_history() record = { "timestamp": datetime.datetime.now().isoformat(), "score": score, "status": status, "details": details, } history.append(record) history = history[-10:] - with open(HISTORY_FILE, "w") as f: - json.dump(history, f, indent=4) + try: + with open(HISTORY_FILE, "w") as f: + json.dump(history, f, indent=4) + except OSError as e: + print(f" -> ⚠️ Failed to save history: {e}") return history
105-178: Add return type hint and handle temp file cleanup on failure.The function is missing a return type annotation. Additionally, if
os.replacefails, the temporary file remains on disk. The atomic write also doesn't preserve the original file's permissions (important forsshd_config).🔎 Proposed fix
-def fix_ssh_config(config_path): +def fix_ssh_config(config_path: str) -> bool: """Disable SSH root login (Automated Fix).For the temp file cleanup:
if fixed: # Atomic write using temporary file dir_path = os.path.dirname(config_path) + original_mode = os.stat(config_path).st_mode with tempfile.NamedTemporaryFile( mode="w", dir=dir_path, delete=False, suffix=".tmp" ) as tmp_file: tmp_file.writelines(new_lines) tmp_path = tmp_file.name - # Atomic replace - os.replace(tmp_path, config_path) + try: + os.chmod(tmp_path, original_mode) + os.replace(tmp_path, config_path) + except OSError: + os.unlink(tmp_path) # Clean up temp file on failure + raise print(" -> ✅ Success: sshd_config updated.")cortex/cli.py (1)
175-184: Add type hint forargsparameter.Per coding guidelines, type hints are required. The docstring documents the parameter but the signature lacks the type annotation.
🔎 Proposed fix
- def health(self, args): + def health(self, args: argparse.Namespace) -> int: """Run system health checks and show recommendations. Args: - args: Parsed command line arguments. + args (argparse.Namespace): Parsed command line arguments. Returns: int: 0 on success, 1 on failure. """
🧹 Nitpick comments (7)
scripts/verify_ubuntu_compatibility.py (6)
19-27: Add type hints toload_historyfunction.Per coding guidelines, type hints are required for Python code. This function is missing return type annotation.
🔎 Proposed fix
-def load_history(): - """Load past execution history""" +def load_history() -> list[dict]: + """Load past execution history. + + Returns: + list[dict]: List of historical execution records. + """ if HISTORY_FILE.exists(): try: with open(HISTORY_FILE) as f: return json.load(f) except json.JSONDecodeError: return [] return []
50-74: Add type hints toshow_trendfunction.Per coding guidelines, type hints are required for Python code.
🔎 Proposed fix
-def show_trend(history): - """Show historical trend (Trend Tracking)""" +def show_trend(history: list[dict]) -> None: + """Show historical trend (Trend Tracking). + + Args: + history: List of historical execution records. + """ print("\n=== 📊 Historical Trend Analysis ===")
76-102: Add return type hint tofix_firewallfunction.The function has a docstring but is missing the return type annotation in the signature.
🔎 Proposed fix
-def fix_firewall(): +def fix_firewall() -> bool: """Enable UFW firewall (Automated Fix).
180-204: Add return type hint to helper function.Per coding guidelines, type hints are required.
🔎 Proposed fix
-def _check_firewall_status(): +def _check_firewall_status() -> bool: """Helper to check firewall status."""
207-233: Add type hints to_check_ssh_statusfunction.Per coding guidelines, type hints are required for Python code.
🔎 Proposed fix
-def _check_ssh_status(ssh_config): +def _check_ssh_status(ssh_config: str) -> tuple[int, bool]: """Helper to check SSH status."""
236-286: Add return type hint toverify_security_logicfunction.Per coding guidelines, type hints are required for Python code.
🔎 Proposed fix
-def verify_security_logic(): +def verify_security_logic() -> None: print("=== Ubuntu Security Logic Verification ===")cortex/health/monitor.py (1)
110-131: Consider adding file locking for concurrent access.The
_save_historymethod reads and writes the history file without synchronization. If multiplecortex healthcommands run concurrently, one process could overwrite another's changes in a read-modify-write race condition.For a single-user CLI tool where concurrent execution is unlikely, this is acceptable, but document this limitation or add file locking if concurrent invocation becomes expected.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cortex/cli.py(11 hunks)cortex/health/checks/disk.py(1 hunks)cortex/health/checks/performance.py(1 hunks)cortex/health/checks/security.py(1 hunks)cortex/health/checks/updates.py(1 hunks)cortex/health/monitor.py(1 hunks)scripts/verify_ubuntu_compatibility.py(1 hunks)tests/test_health_monitor.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- cortex/health/checks/updates.py
- cortex/health/checks/disk.py
- cortex/health/checks/security.py
- cortex/health/checks/performance.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/health/monitor.pyscripts/verify_ubuntu_compatibility.pycortex/cli.pytests/test_health_monitor.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_health_monitor.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Implement audit logging to ~/.cortex/history.db for all package operations
Applied to files:
cortex/cli.py
🧬 Code graph analysis (1)
tests/test_health_monitor.py (6)
cortex/cli.py (2)
health(176-243)status(776-807)cortex/health/checks/disk.py (2)
DiskCheck(6-61)run(9-61)cortex/health/checks/performance.py (2)
PerformanceCheck(7-72)run(10-72)cortex/health/checks/security.py (2)
SecurityCheck(12-106)run(29-80)cortex/health/checks/updates.py (2)
UpdateCheck(9-99)run(21-99)cortex/health/monitor.py (4)
CheckResult(13-22)HealthMonitor(34-132)run(29-31)run_all(67-108)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 1-1: Ruff check failed: Import block is unsorted or unformatted (I001) in cortex/cli.py. Command: 'ruff check . --output-format=github'.
🔇 Additional comments (16)
tests/test_health_monitor.py (6)
1-9: LGTM! Well-structured test file with proper imports.The test file correctly imports all necessary health check classes and the HealthMonitor from the appropriate modules.
11-33: LGTM! Comprehensive disk usage tests.The test covers all three threshold cases (healthy at 50%, warning at 85%, critical at 95%) and correctly validates both score and status fields.
35-63: LGTM! Performance check tests with proper mocking.Tests correctly mock
os.getloadavg,multiprocessing.cpu_count, and/proc/meminfoto validate load average scoring logic.
65-88: LGTM! Security check tests cover key scenarios.Tests validate both UFW inactive (score 0) and UFW active with missing sshd_config (score 100) scenarios. The mock setup correctly simulates
subprocess.runreturn values.
90-115: LGTM! Update check test validates scoring calculation.The test correctly verifies that 2 regular packages (-4 pts) and 1 security update (-10 pts) result in a score of 86, and validates the details format.
117-145: LGTM! Monitor aggregation test validates weighted scoring.The test correctly sets up mock checks with distinct weights (0.5 each) and scores (100, 0), then verifies the weighted average calculation yields 50 points. Mocking
_save_historyprevents file I/O during tests.cortex/cli.py (4)
185-243: LGTM! Health command implementation with proper error handling.The implementation correctly:
- Uses try/except for ImportError and general exceptions
- Dynamically imports HealthMonitor to avoid circular dependencies
- Color-codes the score based on thresholds
- Displays per-check results with status icons
- Collects and displays recommendations
- Returns appropriate exit codes
842-846: LGTM! Help table entries added for new commands.The
notifyandhealthcommands are correctly added to the Rich help table with appropriate descriptions.
952-954: LGTM! Health command parser registration.The health subparser is correctly registered with an appropriate help message.
1005-1006: LGTM! Health command dispatch.The command dispatch correctly routes to
cli.health(args)and returns the result.cortex/health/monitor.py (6)
1-10: LGTM! Clean imports and console setup.The imports are well-organized and the console instance is properly initialized for Rich output.
12-22: LGTM! Well-designed CheckResult dataclass.The dataclass has appropriate fields with clear inline comments, sensible defaults for optional fields (
recommendationandweight), and uses modern Python 3.10+ type union syntax.
25-31: LGTM! Clean abstract base class definition.The
HealthCheckABC correctly defines the contract for health check implementations with proper type hints and docstring.
34-57: LGTM! HealthMonitor initialization with dynamic imports.The constructor properly:
- Sets up history file path with parent directory creation
- Uses dynamic imports to avoid circular references
- Registers all default health checks
59-65: LGTM! Well-documented register_check method.The method has proper type hints and a clear docstring following the coding guidelines.
67-108: LGTM! Robust run_all implementation.The method correctly:
- Handles exceptions per-check without stopping the entire run
- Calculates weighted average score
- Builds a structured report with timestamp and results
- Persists history after each run
| from cortex.installation_history import ( | ||
| InstallationHistory, | ||
| InstallationStatus, | ||
| InstallationType, | ||
| ) | ||
| from cortex.demo import run_demo | ||
| from cortex.installation_history import InstallationHistory, InstallationStatus, InstallationType |
There was a problem hiding this comment.
Fix duplicate import causing CI failure.
Lines 11-15 import InstallationHistory, InstallationStatus, InstallationType from cortex.installation_history, but line 17 imports the same symbols again. This causes the Ruff I001 error (import block is unsorted/unformatted) and is blocking CI.
🔎 Proposed fix
Remove the duplicate import on line 17:
from cortex.installation_history import (
InstallationHistory,
InstallationStatus,
InstallationType,
)
from cortex.demo import run_demo
-from cortex.installation_history import InstallationHistory, InstallationStatus, InstallationType
from cortex.llm.interpreter import CommandInterpreter🧰 Tools
🪛 GitHub Check: Lint
[failure] 17-17: Ruff (F811)
cortex/cli.py:17:82: F811 Redefinition of unused InstallationType from line 14
[failure] 17-17: Ruff (F811)
cortex/cli.py:17:62: F811 Redefinition of unused InstallationStatus from line 13
[failure] 17-17: Ruff (F811)
cortex/cli.py:17:41: F811 Redefinition of unused InstallationHistory from line 12
🤖 Prompt for AI Agents
In cortex/cli.py around lines 11 to 17, there is a duplicate import of
InstallationHistory, InstallationStatus, and InstallationType from
cortex.installation_history (lines 11-15 and again on line 17) causing a Ruff
I001 import-block error; remove the redundant import on line 17 so each symbol
is imported only once and ensure the remaining imports are properly
sorted/formatted.
|
Even if I resolve the conflicts, new ones keep appearing. Since I'm not very familiar with GitHub, it seems unlikely I can achieve a complete fix. Therefore, I'll have to close this PR for now. I'm sorry. |
2 participants
Closes #128
Summary
I have implemented the
System Healthmodule including Security, Disk, Performance, and Update checks.Features Implemented
tests/test_health_monitor.py.Note on Advanced Requirements (Auto-fix & History)
To avoid altering the core architecture of the
HealthCheckbase class, I implemented the Automated Fixes and Trend Tracking features in a standalone utility script located atscripts/verify_ubuntu_compatibility.py.This script demonstrates:
You can run this script to verify these capabilities immediately.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.