Installation simulation mode fix #103 #264
Conversation
|
Warning Rate limit exceeded@Sahilbhatane has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (7)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an installation Simulation Mode (--simulate) to the CLI and a new PreflightChecker subsystem for system checks, report formatting/exporting, and optional LLM-driven package estimates; refactors LLM routing behavior and hardens cross-platform/sandbox execution and tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as cortex/cli.py
participant Preflight as PreflightChecker
participant OS as System
participant LLM as LLM Provider (optional)
participant Formatter as format_report
participant Out as stdout
User->>CLI: cortex install docker --simulate
CLI->>CLI: _run_simulation("docker")
CLI->>Preflight: PreflightChecker(api_key?, provider)
CLI->>Preflight: run_all_checks("docker")
Preflight->>OS: check_os_info(), check_basic_system_info(), check_disk_space(), check_package(...)
OS-->>Preflight: system, disk, package data
alt API key present
Preflight->>LLM: request package sizes/deps
LLM-->>Preflight: package metadata (sizes, deps)
else
Preflight-->>Preflight: fallback estimates / warnings
end
Preflight-->>CLI: PreflightReport
CLI->>Formatter: format_report(report, "docker")
Formatter-->>CLI: formatted text
CLI->>Out: print report
CLI-->>User: exit code (0/1)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
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 (9)
test/test_preflight_checker.py (2)
54-54: UseassertGreaterfor clearer failure messages.Per static analysis, prefer
assertGreateroverassertTrue(len(...) > 0)for better error messages on failure.- self.assertTrue(len(disk_info) > 0) + self.assertGreater(len(disk_info), 0)
56-59: Redundant assertion—consider simplifying.The assertion
cwd_checked or len(disk_info) > 0is always true if line 54 passes, making it ineffective at verifying the current directory is actually included. Consider assertingcwd_checkeddirectly if that's the intent:- # Check that current directory is included - cwd = os.getcwd() - cwd_checked = any(d.path == cwd or cwd.startswith(d.path) for d in disk_info) - self.assertTrue(cwd_checked or len(disk_info) > 0) + # Check that current directory or a parent path is included + cwd = os.getcwd() + cwd_checked = any(d.path == cwd or cwd.startswith(d.path) for d in disk_info) + self.assertTrue(cwd_checked, "Current directory should be covered by disk checks")docs/SIMULATION_MODE.md (1)
37-53: Add language specifiers to fenced code blocks.Several code blocks showing example CLI output are missing language specifiers, which affects syntax highlighting and linting. While these are output examples (not shell commands), using
textorconsolewould satisfy the linter.-``` +```text 🔍 Simulation mode: No changes will be madeApply the same fix to code blocks at lines 57, 116, and 124.
cortex/cli.py (2)
216-218: Use explicit conversion flag in f-string.Per linting (Ruff RUF010), prefer explicit conversion flag over
str()call.- self._print_error(f"Simulation failed: {str(e)}") + self._print_error(f"Simulation failed: {e!s}")
23-23:export_reportis imported but unused.The
export_reportfunction is imported but not used anywhere incli.py. Consider removing it unless there are plans to add report export functionality soon.-from cortex.preflight_checker import PreflightChecker, format_report, export_report +from cortex.preflight_checker import PreflightChecker, format_reportcortex/preflight_checker.py (4)
79-81: Annotate class-level mutable attributes withClassVar.
DISK_CHECK_PATHSis a mutable list at class level. Per Ruff (RUF012), annotate withClassVarto indicate it's not an instance attribute and prevent accidental mutation issues.+from typing import ClassVar + class PreflightChecker: # Paths to check for disk space - DISK_CHECK_PATHS = ['/', '/var/lib/docker', '/opt'] + DISK_CHECK_PATHS: ClassVar[List[str]] = ['/', '/var/lib/docker', '/opt']
109-123: Avoidshell=Trueto prevent potential command injection.
_run_shell_commandusesshell=True(line 114), and_get_filesystem_typepasses a path into a shell command. While current paths come from trusted sources, this pattern is fragile. Consider using subprocess without shell:def _get_filesystem_type(self, path: str) -> str: """Get filesystem type for a path on Linux""" - success, output = self._run_shell_command(f"df -T '{path}' | tail -1 | awk '{{print $2}}'") - if success and output: - return output + success, output = self._run_command(['df', '-T', path]) + if success and output: + lines = output.strip().split('\n') + if len(lines) >= 2: + # Parse second line, second column (filesystem type) + parts = lines[1].split() + if len(parts) >= 2: + return parts[1] return 'unknown'This avoids shell injection risks entirely.
Also applies to: 256-261
526-588: Consider extracting helper functions to reduce complexity.
format_reporthas cognitive complexity of 25 (exceeds the 15 threshold). While the logic is linear, extracting section formatters would improve maintainability:def _format_system_info(report: PreflightReport) -> List[str]: """Format system information section.""" lines = ["System Information:"] lines.append(f" OS: {report.os_info.get('distro', 'Unknown')} {report.os_info.get('distro_version', '')}") if report.kernel_info.get('version'): lines.append(f" Kernel: {report.kernel_info.get('version')}") if report.cpu_arch: lines.append(f" Architecture: {report.cpu_arch}") return lines def _format_packages_section(report: PreflightReport, software: str, using_estimates: bool) -> List[str]: # ... similar extraction for packages section passThen compose them in
format_report. This can be deferred if time-constrained.
168-169: Consider logging silently swallowed exceptions.Multiple
try-except-passblocks silently swallow exceptions (lines 168, 240, 369). While graceful fallbacks are appropriate for preflight checks, consider logging at DEBUG level for troubleshooting:import logging logger = logging.getLogger(__name__) # Then in exception handlers: except Exception as e: logger.debug(f"Failed to get distro info: {e}")This maintains the graceful degradation while providing visibility for debugging.
Also applies to: 240-241, 369-371
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cortex/cli.py(6 hunks)cortex/preflight_checker.py(1 hunks)docs/SIMULATION_MODE.md(1 hunks)requirements.txt(1 hunks)test/test_cli.py(1 hunks)test/test_preflight_checker.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
test/test_cli.py (1)
cortex/cli.py (1)
main(519-607)
cortex/cli.py (1)
cortex/preflight_checker.py (4)
PreflightChecker(72-523)format_report(526-588)export_report(591-605)run_all_checks(500-523)
cortex/preflight_checker.py (1)
LLM/interpreter.py (1)
CommandInterpreter(12-158)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/cli.py
[failure] 65-65: Refactor this function to reduce its Cognitive Complexity from 45 to the 15 allowed.
test/test_preflight_checker.py
[warning] 54-54: Consider using "assertGreater" instead.
cortex/preflight_checker.py
[warning] 469-469: Remove the unused local variable "software_lower".
[warning] 529-529: Add replacement fields or use a normal string instead of an f-string.
[warning] 321-321: Remove the unused local variable "distro".
[warning] 544-544: Add replacement fields or use a normal string instead of an f-string.
[failure] 526-526: Refactor this function to reduce its Cognitive Complexity from 25 to the 15 allowed.
🪛 markdownlint-cli2 (0.18.1)
docs/SIMULATION_MODE.md
37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
57-57: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
116-116: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
124-124: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.7)
cortex/cli.py
214-214: Consider moving this statement to an else block
(TRY300)
216-216: Do not catch blind exception: Exception
(BLE001)
217-217: Use explicit conversion flag
Replace with conversion flag
(RUF010)
cortex/preflight_checker.py
80-80: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
95-95: subprocess call: check for execution of untrusted input
(S603)
106-106: Do not catch blind exception: Exception
(BLE001)
112-112: subprocess call with shell=True identified, security issue
(S602)
122-122: Do not catch blind exception: Exception
(BLE001)
168-169: try-except-pass detected, consider logging the exception
(S110)
168-168: Do not catch blind exception: Exception
(BLE001)
240-241: try-except-pass detected, consider logging the exception
(S110)
240-240: Do not catch blind exception: Exception
(BLE001)
297-297: Consider moving this statement to an else block
(TRY300)
298-298: Do not catch blind exception: Exception
(BLE001)
308-309: try-except-pass detected, consider logging the exception
(S110)
308-308: Do not catch blind exception: Exception
(BLE001)
321-321: Local variable distro is assigned to but never used
Remove assignment to unused variable distro
(F841)
369-371: try-except-pass detected, consider logging the exception
(S110)
369-369: Do not catch blind exception: Exception
(BLE001)
469-469: Local variable software_lower is assigned to but never used
Remove assignment to unused variable software_lower
(F841)
529-529: f-string without any placeholders
Remove extraneous f prefix
(F541)
544-544: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ 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). (1)
- GitHub Check: Agent
🔇 Additional comments (7)
test/test_cli.py (1)
181-181: LGTM!The test assertions correctly verify that
simulate=Falseis passed by default when the--simulateflag is not provided, aligning with the updatedCortexCLI.installsignature.Also applies to: 189-189, 197-197
test/test_preflight_checker.py (1)
164-197: Well-structured integration tests for simulate flag.The CLI integration tests properly mock
PreflightChecker, verify the--simulateflag parsing, and confirm the correct parameters are passed toCortexCLI.install. Good coverage for the new feature.docs/SIMULATION_MODE.md (1)
1-228: Comprehensive documentation for the simulation feature.The documentation thoroughly covers usage, outputs, API reference, and the distinction from
--dry-run. This aligns well with the acceptance criteria from Issue #103.cortex/cli.py (1)
553-553: CLI argument wiring is correct.The
--simulateflag is properly added to the argument parser and correctly passed through toCortexCLI.install. The implementation aligns with the documented behavior.Also applies to: 590-590
cortex/preflight_checker.py (2)
20-69: Well-structured dataclasses for preflight reporting.The dataclasses are correctly defined with appropriate type hints and use
field(default_factory=...)for mutable defaults, avoiding the common pitfall of shared mutable state.
284-311: LLM client fallback logic is well-designed.The fallback from primary to secondary LLM provider (OpenAI ↔ Claude) provides good resilience. The implementation handles failures gracefully and falls back to estimates when no LLM is available.
requirements.txt (1)
9-9: Pin PyYAML version and verify its necessity.PyYAML is added without version pinning, which can cause reproducibility issues. Additionally, PyYAML is not imported or used in the new preflight checker or CLI changes—the
export_reportfunction uses JSON, not YAML.If PyYAML is needed, pin it like other dependencies:
-PyYAML +PyYAML>=6.0.0Otherwise, consider removing this dependency if it's not actively used in the codebase.
There was a problem hiding this comment.
Pull request overview
This PR implements a simulation mode for Cortex installations (Issue #103), adding a --simulate flag that allows users to preview what would be installed without making changes. The feature performs real system checks and optionally queries LLMs for accurate package information.
Key Changes
- Added preflight checking system that performs real-time OS detection, disk space analysis, and package status verification
- Integrated optional LLM querying (OpenAI/Anthropic) to fetch accurate package sizes with automatic fallback to estimates when no API key is provided
- Extended CLI with
--simulateflag that takes precedence over other execution flags
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 24 comments.
Show a summary per file
| File | Description |
|---|---|
| cortex/preflight_checker.py | Core preflight checking implementation with system detection, disk analysis, and LLM integration for package information |
| cortex/cli.py | Added --simulate flag support and _run_simulation() method to handle simulation mode execution |
| test/test_preflight_checker.py | Comprehensive unit tests for preflight checker functionality and CLI integration |
| test/test_cli.py | Updated existing tests to include the new simulate parameter in assertions |
| docs/SIMULATION_MODE.md | Complete documentation covering usage, features, API reference, and differences from --dry-run |
| requirements.txt | Added PyYAML dependency (already used elsewhere in codebase) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cortex/preflight_checker.py (1)
527-527: Remove extraneousfprefix from string without placeholders.This was flagged in a previous review. The f-string on line 527 has no placeholders.
🧹 Nitpick comments (7)
cortex/preflight_checker.py (5)
79-81: Annotate mutable class attributes withClassVar.
DISK_CHECK_PATHSandMIN_DISK_SPACE_MBare class-level constants that shouldn't be instance attributes. Annotating them withClassVarmakes intent clearer and satisfies static analysis.+from typing import List, Dict, Optional, Tuple, ClassVar ... class PreflightChecker: ... # Paths to check for disk space - DISK_CHECK_PATHS = ['/', '/var/lib/docker', '/opt'] - MIN_DISK_SPACE_MB = 500 + DISK_CHECK_PATHS: ClassVar[List[str]] = ['/', '/var/lib/docker', '/opt'] + MIN_DISK_SPACE_MB: ClassVar[int] = 500
168-169: Consider logging exceptions instead of silently ignoring them.Multiple
try-except-passblocks throughout the file (lines 168, 240, 308, 368) silently swallow exceptions. While this is acceptable for non-critical paths, consider adding debug logging to aid troubleshooting.+import logging + +logger = logging.getLogger(__name__) ... except Exception: - pass + logger.debug("Failed to read /etc/os-release", exc_info=True)
293-311: Fallback logic importsCommandInterpretertwice.The import statement
from LLM.interpreter import CommandInterpreterappears both at line 295 and line 305. Since the import is at module-level scope within the method, the second import is redundant but harmless.def _get_llm_client(self): """Lazy initialize LLM client with fallback""" if self._llm_client is not None: return self._llm_client if not self.api_key: return None + # Import once for both primary and fallback + try: + sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..')) + from LLM.interpreter import CommandInterpreter + except ImportError: + return None + # Try to initialize with primary provider try: - sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..')) - from LLM.interpreter import CommandInterpreter self._llm_client = CommandInterpreter(api_key=self.api_key, provider=self.provider) return self._llm_client except Exception: # Fallback to other provider try: fallback_provider = 'claude' if self.provider == 'openai' else 'openai' fallback_key = os.environ.get('ANTHROPIC_API_KEY' if fallback_provider == 'claude' else 'OPENAI_API_KEY') if fallback_key: - from LLM.interpreter import CommandInterpreter self._llm_client = CommandInterpreter(api_key=fallback_key, provider=fallback_provider) return self._llm_client except Exception: pass return None
466-485: Unusedsoftwareparameter incalculate_requirements.The
softwareparameter is never used in this method. Either remove it or use it for software-specific calculations.- def calculate_requirements(self, software: str) -> None: + def calculate_requirements(self) -> None: """Calculate installation requirements based on software to install"""Then update the call site at line 519:
- self.calculate_requirements(software) + self.calculate_requirements()
524-586: High cognitive complexity informat_reportfunction.SonarCloud flags this function with cognitive complexity of 25 (allowed: 15). Consider extracting helper functions for each section (system info, packages, disk space, config changes, issues).
Example extraction:
def _format_system_info(report: PreflightReport) -> List[str]: """Format system information section.""" lines = ["System Information:"] lines.append(f" OS: {report.os_info.get('distro', 'Unknown')} {report.os_info.get('distro_version', '')}") if report.kernel_info.get('version'): lines.append(f" Kernel: {report.kernel_info.get('version')}") if report.cpu_arch: lines.append(f" Architecture: {report.cpu_arch}") return lines def _format_packages_section(report: PreflightReport, software: str, using_estimates: bool) -> List[str]: """Format packages to install section.""" # ... extracted logiccortex/cli.py (2)
196-218: Simulation method addresses previous review feedback.The previous review flagged that
_get_api_key()would print an error when no key exists. This is now correctly handled by usingos.environ.get()directly (line 200), which silently returnsNonewhen no key is set.One minor improvement per static analysis - use explicit conversion flag in the f-string:
except Exception as e: - self._print_error(f"Simulation failed: {str(e)}") + self._print_error(f"Simulation failed: {e!s}") return 1
65-194: Very high cognitive complexity ininstallmethod.SonarCloud flags cognitive complexity of 45 (allowed: 15). While the simulation changes are minimal (lines 66-68), this method was already complex. Consider extracting the execution logic into separate helper methods in a follow-up.
Potential extraction points:
_execute_installation(commands, software, history, install_id)for the execute block (lines 121-173)_record_installation_start(history, commands, execute, dry_run)for history setup
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.py(6 hunks)cortex/preflight_checker.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cortex/cli.py (1)
cortex/preflight_checker.py (4)
PreflightChecker(72-521)format_report(524-586)export_report(589-600)run_all_checks(498-521)
cortex/preflight_checker.py (1)
LLM/interpreter.py (1)
CommandInterpreter(12-158)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/cli.py
[failure] 65-65: Refactor this function to reduce its Cognitive Complexity from 45 to the 15 allowed.
cortex/preflight_checker.py
[failure] 524-524: Refactor this function to reduce its Cognitive Complexity from 25 to the 15 allowed.
🪛 Ruff (0.14.7)
cortex/cli.py
214-214: Consider moving this statement to an else block
(TRY300)
216-216: Do not catch blind exception: Exception
(BLE001)
217-217: Use explicit conversion flag
Replace with conversion flag
(RUF010)
cortex/preflight_checker.py
80-80: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
95-95: subprocess call: check for execution of untrusted input
(S603)
106-106: Do not catch blind exception: Exception
(BLE001)
112-112: subprocess call with shell=True identified, security issue
(S602)
122-122: Do not catch blind exception: Exception
(BLE001)
168-169: try-except-pass detected, consider logging the exception
(S110)
168-168: Do not catch blind exception: Exception
(BLE001)
240-241: try-except-pass detected, consider logging the exception
(S110)
240-240: Do not catch blind exception: Exception
(BLE001)
297-297: Consider moving this statement to an else block
(TRY300)
298-298: Do not catch blind exception: Exception
(BLE001)
308-309: try-except-pass detected, consider logging the exception
(S110)
308-308: Do not catch blind exception: Exception
(BLE001)
368-370: try-except-pass detected, consider logging the exception
(S110)
368-368: Do not catch blind exception: Exception
(BLE001)
466-466: Unused method argument: software
(ARG002)
🔇 Additional comments (6)
cortex/preflight_checker.py (3)
20-70: Well-structured dataclasses for preflight reporting.The dataclass definitions are clean and provide good type safety. Using
field(default_factory=...)for mutable defaults is the correct pattern.
109-123: Security concern:shell=Truewith user-influenced input.
_run_shell_commandusesshell=True, which can be a security risk if thecmdstring contains unsanitized user input. Currently it's used withpathfrom_get_filesystem_type. Whilepathoriginates from known paths inDISK_CHECK_PATHSoros.getcwd(), ensure this method isn't called with arbitrary user input in the future.Consider adding a comment documenting this assumption, or switching to
shlex.quote()for the path:def _get_filesystem_type(self, path: str) -> str: """Get filesystem type for a path on Linux""" - success, output = self._run_shell_command(f"df -T '{path}' | tail -1 | awk '{{print $2}}'") + import shlex + success, output = self._run_shell_command(f"df -T {shlex.quote(path)} | tail -1 | awk '{{print $2}}'")
589-600: Redundant manual conversion afterasdictalready fixed.The code now correctly relies on
asdict(report)without redundant conversions. Good fix from the previous review.cortex/cli.py (3)
23-23: Import added for preflight checker integration.Clean import of the new preflight functionality.
65-68: Good early return pattern for simulation mode.The simulation check at the start of
install()provides a clean separation of concerns and doesn't require an API key, which aligns with the design intent.
528-528: CLI integration for--simulateflag is complete.The flag is properly added to the argument parser, documented in the epilog, and correctly passed through to
cli.install().Also applies to: 553-553, 590-590
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
cortex/preflight_checker.py (5)
11-11: Remove unusedsocketimport.The
socketmodule is imported but never used in this file.Apply this diff:
-import socket
390-480: Consider extracting duplicated logic into a helper method.The
check_docker,check_containerd, andcheck_softwaremethods share a common pattern:
- Check if package is installed
- If not installed and API key exists, query LLM for package info
- Fall back to estimates if LLM fails or no API key
Extract this logic into a shared helper method to improve maintainability and reduce duplication.
Example helper method:
def _check_and_add_package(self, pkg: PackageInfo, software: str, default_estimates: List[Dict[str, str]], default_config_changes: Optional[List[str]] = None) -> PackageInfo: """Helper to check package and add to install list if not present""" self.report.package_status.append(pkg) if not pkg.installed: if self.api_key: pkg_info = self._get_package_info_from_llm(software, self.report.os_info) if pkg_info.get('packages'): self.report.packages_to_install.extend(pkg_info['packages']) if pkg_info.get('config_changes'): self.report.config_changes.extend(pkg_info['config_changes']) else: self.report.packages_to_install.extend(default_estimates) if default_config_changes: self.report.config_changes.extend(default_config_changes) else: self.report.packages_to_install.extend(default_estimates) if default_config_changes: self.report.config_changes.extend(default_config_changes) if not any('estimate' in str(p.get('size_mb', '')) for p in self.report.packages_to_install): self.report.warnings.append('Package sizes are estimates - provide API key for accurate sizes') return pkg
616-626: Remove redundant module import.The
jsonmodule is already imported at the module level (line 14), so there's no need to import it again locally. Remove the redundant import and use the module-level import.Apply this diff:
def export_report(report: PreflightReport, filepath: str) -> None: """Export preflight report to a JSON file""" - import json from dataclasses import asdict # Convert dataclass to dict report_dict = asdict(report) # `asdict` already converts nested dataclasses recursively, so we can # directly write the result to JSON. with open(filepath, 'w') as f: json.dump(report_dict, f, indent=2)
328-328: Fix type hint: useAnyinstead ofany.The return type annotation uses the built-in
anytype instead oftyping.Any. ImportAnyfrom the typing module (already imported at line 16) and use it correctly.Apply this diff:
- def _get_package_info_from_llm(self, software: str, os_info: Dict[str, str]) -> Dict[str, any]: + def _get_package_info_from_llm(self, software: str, os_info: Dict[str, str]) -> Dict[str, Any]: """Query LLM for real package information including sizes"""Note:
Anyis already imported fromtypingat line 16.
270-275: Fix command injection vulnerability in shell command.Using an f-string with a user-controlled
pathin a shell command creates a command injection risk. While the path is wrapped in single quotes, special characters could still be exploited.Apply this diff to use
shlex.quote()for proper escaping:+import shlex + def _get_filesystem_type(self, path: str) -> str: """Get filesystem type for a path on Linux""" - success, output = self._run_shell_command(f"df -T '{path}' | tail -1 | awk '{{print $2}}'") + success, output = self._run_shell_command(f"df -T {shlex.quote(path)} | tail -1 | awk '{{print $2}}'") if success and output: return output return 'unknown'docs/SIMULATION_MODE.md (1)
213-214: Fix type documentation to match implementation.The documentation states that
total_download_mbandtotal_disk_required_mbare floats, but in the code (preflight_checker.py lines 67-68), they are declared asint.Apply this diff:
-- `total_download_mb`: Total download size (float) -- `total_disk_required_mb`: Total disk space needed (float) +- `total_download_mb`: Total download size (int) +- `total_disk_required_mb`: Total disk space needed (int)
🧹 Nitpick comments (7)
cortex/preflight_checker.py (3)
80-80: AddClassVarannotation for mutable class attribute.The
DISK_CHECK_PATHSclass attribute is mutable and should be annotated withtyping.ClassVarto indicate it's a class-level variable that shouldn't be treated as an instance attribute.Apply this diff:
+from typing import List, Dict, Optional, Tuple, ClassVar + class PreflightChecker: """ Real system checker for Cortex installation preflight checks. All checks are performed against the actual system - no simulation. """ # Default paths - will be set based on platform in __init__ - DISK_CHECK_PATHS = [] + DISK_CHECK_PATHS: ClassVar[List[str]] = [] MIN_DISK_SPACE_MB = 500
482-517: Remove unusedsoftwareparameter.The
softwareparameter incalculate_requirementsis not used in the method body. The method calculates requirements based onself.report.packages_to_install, which is already populated.Apply this diff:
- def calculate_requirements(self, software: str) -> None: + def calculate_requirements(self) -> None: """Calculate installation requirements based on software to install"""And update the call site at line 539:
# Calculate requirements - self.calculate_requirements(software) + self.calculate_requirements()
544-613: Consider splitting this function to reduce complexity.SonarCloud reports cognitive complexity of 27 (threshold: 15). While the function is readable, consider extracting sections into helper functions for better maintainability:
_format_system_info(report)_format_packages_to_install(report)_format_disk_space(report)_format_issues(report)This is optional, as the current implementation is functional and clear.
docs/SIMULATION_MODE.md (4)
37-53: Add language specifier to code block.For better syntax highlighting and clarity, add a language specifier to this code block.
Apply this diff:
-``` +```text 🔍 Simulation mode: No changes will be made
57-75: Add language specifier to code block.For better syntax highlighting and clarity, add a language specifier to this code block.
Apply this diff:
-``` +```text 🔍 Simulation mode: No changes will be made
116-120: Add language specifier to code block.For better syntax highlighting and clarity, add a language specifier to this code block.
Apply this diff:
-``` +```text ❌ Errors: - Insufficient disk space: 200 MB available, 500 MB required
124-128: Add language specifier to code block.For better syntax highlighting and clarity, add a language specifier to this code block.
Apply this diff:
-``` +```text ⚠️ Warnings: - API key not found, using estimates for package sizes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/preflight_checker.py(1 hunks)docs/SIMULATION_MODE.md(1 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
cortex/preflight_checker.py
[warning] 386-386: Remove this unneeded "pass".
[failure] 544-544: Refactor this function to reduce its Cognitive Complexity from 27 to the 15 allowed.
🪛 markdownlint-cli2 (0.18.1)
docs/SIMULATION_MODE.md
37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
57-57: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
116-116: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
124-124: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.7)
cortex/preflight_checker.py
80-80: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
107-107: subprocess call: check for execution of untrusted input
(S603)
118-118: Do not catch blind exception: Exception
(BLE001)
124-124: subprocess call with shell=True identified, security issue
(S602)
134-134: Do not catch blind exception: Exception
(BLE001)
180-182: try-except-pass detected, consider logging the exception
(S110)
180-180: Do not catch blind exception: Exception
(BLE001)
253-255: try-except-pass detected, consider logging the exception
(S110)
253-253: Do not catch blind exception: Exception
(BLE001)
311-311: Consider moving this statement to an else block
(TRY300)
312-312: Do not catch blind exception: Exception
(BLE001)
322-324: try-except-pass detected, consider logging the exception
(S110)
322-322: Do not catch blind exception: Exception
(BLE001)
383-383: Do not catch blind exception: Exception
(BLE001)
482-482: Unused method argument: software
(ARG002)
🔇 Additional comments (2)
cortex/preflight_checker.py (1)
198-268: LGTM - Good handling of disk space checks.The disk space checking logic properly handles:
- Path deduplication
- Non-existent paths by walking up to nearest parent
- Low disk space warnings
- Silent failures with documented reasons and sensible defaults
docs/SIMULATION_MODE.md (1)
221-228: LGTM - API examples are accurate.The
export_reportexample correctly shows the function writing to a file, which matches the implementation inpreflight_checker.py.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cortex/cli.py (1)
28-28: Unusedexport_reportimport and incomplete wiring for report export
export_reportis imported but never used here. Given thatexport_reportexists incortex.preflight_checkerand the issue acceptance criteria mention exporting simulation reports, this looks either like a leftover or an uncompleted CLI feature.I’d suggest either:
- Remove
export_reportfrom this import if export is intentionally handled elsewhere, or- Add a CLI option (e.g.
--export-report PATH) forcortex install ... --simulateand callexport_report(report, PATH)inside_run_simulation.
🧹 Nitpick comments (2)
cortex/cli.py (2)
255-277: Tighten error handling in_run_simulationand address static-analysis noiseFunctionality-wise,
_run_simulation()looks fine, but a couple of refinements would improve quality and clear lints:
You’re catching a blind
Exception(Line 275). For a CLI, it’s often better to either:
- Catch specific expected exceptions (e.g.
OSError,subprocess.TimeoutExpiredraised insidePreflightCheckerhelpers), or- Let unexpected errors propagate to the global
main()handler, which already catchesExceptionand prints a generic “Unexpected error”.Ruff’s TRY300 complaint about the
if report.errors: return 1followed byreturn 0can be trivially satisfied (or ignored), but if you care about the warning, an explicitelsekeeps the structure clearer:- # Return error code if blocking issues found - if report.errors: - return 1 - return 0 + # Return error code if blocking issues found + if report.errors: + return 1 + else: + return 0
- For better diagnostics you might prefer
repr(e)or an explicit conversion flag:- except Exception as e: - self._print_error(f"Simulation failed: {str(e)}") + except Exception as e: + self._print_error(f"Simulation failed: {e!r}")These are non-functional, but they will appease the linters and make debugging simulation failures easier.
793-818: Document--simulateconsistently across help text and examplesThe new
--simulateflag is wired into argparse (Line 878) and showcased in the epilog examples (Lines 831‑837), which is good. Two small UX/doc tweaks will make it more discoverable and less redundant:
- Rich help flags section (Lines 815‑818)
Theshow_rich_help()“Flags” list doesn’t mention--simulate. Consider adding it:- console.print("[bold cyan]Flags:[/bold cyan]") - console.print(" --execute Actually run the commands (default: preview only)") - console.print(" --dry-run Show what would happen without doing anything") + console.print("[bold cyan]Flags:[/bold cyan]") + console.print(" --execute Actually run the commands (default: preview only)") + console.print(" --dry-run Show what would happen without doing anything") + console.print(" --simulate Run preflight checks and disk/package analysis only") console.print(" --verbose Show debug information")
- Avoid duplicated example blocks in the epilog (Lines 831‑845 vs 845‑850)
The epilog now has two example groups that both includecortex install dockervariants. You might want to merge them into a single concise examples block that includes--execute,--dry-run, and--simulatetogether to prevent confusion.These are documentation-level changes but will help users understand the new simulation mode more quickly.
Also applies to: 831-845, 874-879
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.py(6 hunks)requirements.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 Additional context used
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/preflight_checker.py (4)
PreflightChecker(72-541)format_report(544-613)export_report(616-626)run_all_checks(518-541)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/cli.py
[warning] 115-115: Remove the unused function parameter "execute".
[warning] 115-115: Remove the unused function parameter "dry_run".
🪛 Ruff (0.14.7)
cortex/cli.py
115-115: Unused method argument: execute
(ARG002)
115-115: Unused method argument: dry_run
(ARG002)
273-273: Consider moving this statement to an else block
(TRY300)
275-275: Do not catch blind exception: Exception
(BLE001)
276-276: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (1)
cortex/cli.py (1)
115-120: Fix duplicatedinstall()definition and brokensimulateparameter wiringYou currently define
CortexCLI.installtwice:
- Lines 115‑119:
install(..., simulate: bool = False)that short-circuits to_run_simulation.- Line 120 onward:
install(..., dry_run: bool = False)(original implementation), which overwrites the first definition in Python.As a result:
- The active
install()method does not acceptsimulate, butmain()callscli.install(..., simulate=args.simulate)(lines 914‑915),which will raise
TypeError: install() got an unexpected keyword argument 'simulate'at runtime.Additionally, the second
elif args.command == 'install'branch at lines 922‑923 is now dead/unreachable.Recommend:
- Merge simulation handling into a single
install()implementation:- def install(self, software: str, execute: bool = False, dry_run: bool = False, simulate: bool = False): - # Handle simulation mode first - no API key needed - if simulate: - return self._run_simulation(software) - - - def install(self, software: str, execute: bool = False, dry_run: bool = False): + def install(self, software: str, execute: bool = False, dry_run: bool = False, simulate: bool = False): + # Handle simulation mode first - no API key needed + if simulate: + return self._run_simulation(software) + # Validate input first is_valid, error = validate_install_request(software)
- Clean up the dispatcher to remove the duplicate
installbranch:- elif args.command == 'install': - return cli.install(args.software, execute=args.execute, dry_run=args.dry_run) elif args.command == 'status': return cli.status()This resolves the runtime error, removes dead code, and clears static-analysis warnings about unused parameters.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cortex/cli.py (3)
27-27: Unusedexport_reportimport (and missing CLI hook for report export)
export_reportis imported here but never used in this module. Either wire it into_run_simulation(e.g., behind a CLI flag to export the simulation report) or remove it to avoid an unused import and keep the API surface clear.
320-342: Simulation path works but can be tightened and better aligned with CLI style
_run_simulationcorrectly:
- Uses an optional API key from the environment (avoiding
_get_api_key’s hard error).- Instantiates
PreflightChecker, runs checks, prints a formatted report, and mapsreport.errorsto exit codes.Two follow-ups would improve this:
- Add the return type hint for clarity and to align with your type-hint guideline:
- def _run_simulation(self, software: str) -> int: + def _run_simulation(self, software: str) -> int:
- If simulation-report export is a requirement for this CLI (since
export_reportis imported), consider adding an optional export/output flag at the CLI level and callingexport_report(report, path_or_format)here. Otherwise, drop the unused import as noted above.These are non-blocking but will make the feature more consistent and maintainable.
606-611:--simulateflag wiring is correct, but consider mutual exclusivity with execute/dry-runThe new
--simulateflag is added to the install subparser and passed through frommain()toCortexCLI.install, which is the right wiring for the feature.Once the single
installmethod is fixed as suggested above,--simulatewill short-circuit to_run_simulation, ignoring--execute/--dry-run. To avoid confusing combinations likecortex install docker --execute --simulate, consider making these flags mutually exclusive via anargparsemutually exclusive group on the install subparser.This is a usability improvement rather than a correctness blocker.
Also applies to: 659-662
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/cli.py(5 hunks)requirements.txt(1 hunks)test/test_cli.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 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.pytest/test_cli.py
🧬 Code graph analysis (1)
test/test_cli.py (1)
cortex/cli.py (1)
main(583-689)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/cli.py
[warning] 116-116: Remove the unused function parameter "dry_run".
[warning] 116-116: Remove the unused function parameter "execute".
🔇 Additional comments (1)
test/test_cli.py (1)
177-199: Main → CortexCLI.install wiring withsimulatelooks correctThe updated expectations for
mock_install.assert_called_once_with(..., simulate=False)in these three tests match the newmain()wiring and keep the tests focused on CLI argument propagation. No issues here.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
requirements.txt (1)
11-11: Remove duplicatePyYAMLdependency.Lines 11 and 15 both specify the same package with inconsistent casing (
pyyamlvs.PyYAML). While pip treats these as equivalent, having duplicate entries violates the DRY principle and creates maintenance confusion. Standardize on a single entry with consistent casing.Apply this diff to remove the duplicate:
# Configuration pyyaml>=6.0.0 # Type hints for older Python versions typing-extensions>=4.0.0 -PyYAML>=6.0.0Also applies to: 15-15
🧹 Nitpick comments (4)
LLM/test_interpreter.py (1)
10-10: Verify the subprocess import is needed.The
subprocessimport was added but doesn't appear to be used in this test file. If it's not needed, consider removing it to keep imports clean.cortex/cli.py (3)
27-27: Remove unused import.The
export_reportimport doesn't appear to be used in this file. Consider removing it to keep imports clean.-from cortex.preflight_checker import PreflightChecker, format_report, export_report +from cortex.preflight_checker import PreflightChecker, format_reportBased on past review comments, this import was previously flagged as unused.
180-183: Add return type hint for consistency.The
installmethod is missing a return type hint. For consistency with_run_simulation(line 319), add-> intto the signature.- def install(self, software: str, execute: bool = False, dry_run: bool = False, simulate: bool = False): + def install(self, software: str, execute: bool = False, dry_run: bool = False, simulate: bool = False) -> int: # Handle simulation mode first - no API key needed if simulate: return self._run_simulation(software)As per coding guidelines, type hints are required in Python code.
319-341: Enhance docstring and exception handling.The
_run_simulationmethod correctly addresses the past review concern about API key errors by directly checking environment variables. However, the implementation could be improved:
The docstring should document the parameter, return value, and behavior:
"""Run preflight simulation check for installation. Args: software: Name of the software package to simulate installing Returns: 0 if simulation succeeds with no blocking issues, 1 otherwise Note: This method works without an API key by using estimates. If an API key is available, it will use LLM-powered package information for better accuracy. """The broad
Exceptioncatch on line 339 should ideally be more specific or at least log the exception type for debugging.As per coding guidelines, docstrings are required for all public APIs (and recommended for private methods in complex modules).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/ci.yml(1 hunks)LLM/test_interpreter.py(1 hunks)cortex/cli.py(5 hunks)cortex/hardware_detection.py(0 hunks)requirements.txt(1 hunks)tests/test_hardware_detection.py(4 hunks)
💤 Files with no reviewable changes (1)
- cortex/hardware_detection.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:
tests/test_hardware_detection.pycortex/cli.pyLLM/test_interpreter.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_hardware_detection.py
🧠 Learnings (3)
📓 Common learnings
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 : Dry-run by default for all installations in command execution
📚 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 : Type hints required in Python code
Applied to files:
requirements.txt
📚 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 : Dry-run by default for all installations in command execution
Applied to files:
cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/preflight_checker.py (4)
PreflightChecker(72-541)format_report(544-613)export_report(616-626)run_all_checks(518-541)
🔇 Additional comments (6)
tests/test_hardware_detection.py (3)
10-10: LGTM!The
subprocessimport is appropriate here as the tests referencesubprocess.TimeoutExpired(line 561) and patchsubprocess.runthroughout.
302-302: LGTM!Adding
create=Trueto theos.statvfsandos.unamepatches is good practice for cross-platform compatibility, as these attributes may not exist on all platforms (e.g., Windows).Also applies to: 338-338
572-572: LGTM!Simplifying the mock to a single
FileNotFoundErrormakes the test more focused and easier to understand, directly testing the nvidia-smi not found scenario..github/workflows/ci.yml (1)
38-38: LGTM!Removing the fallback echo improves CI reliability by ensuring test failures are properly surfaced. The explicit
python -m pytest test/ tests/invocation is more robust and searches both test directories. SinceANTHROPIC_API_KEYis provided (line 36), tests should execute successfully.cortex/cli.py (2)
609-609: LGTM!The
--simulateargument is well-defined with clear help text and consistent with the existing--executeand--dry-runflags.
659-660: LGTM!The install command correctly passes the
simulateargument through to theinstallmethod. The past review concern about duplicateinstallcommand handling has been resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/conftest.py (2)
19-23: Only prepend paths that exist; consider normalizing to avoid duplicate spellings.Guard with
if path.exists() and path.is_dir()before inserting; optionally usepath.resolve()inside_prepend_sys_path()so the “already in sys.path” check is more robust. Suggested diff:def _prepend_sys_path(path: Path) -> None: - path_str = str(path) + if not path.exists() or not path.is_dir(): + return + path_str = str(path.resolve()) if path_str not in sys.path: sys.path.insert(0, path_str)Also applies to: 29-31
1-31: Consider pytest-native configuration instead of runtimesys.pathmutation.If feasible, configuring test import roots via
pytest.ini/pyproject.toml(e.g.,pythonpath) or making tests importcortex.*consistently is less order-sensitive than mutatingsys.pathin code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/codeql.yml(0 hunks)tests/conftest.py(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/codeql.yml
🧰 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/conftest.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/conftest.py
🧠 Learnings (1)
📓 Common learnings
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 : Dry-run by default for all installations in command execution
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/llm_router.py (1)
108-138: Fix theOptional[str] = _UNSETtype-hint mismatch, define_UnsetType, and handleenable_fallbackproperly.The type annotations
claude_api_key: Optional[str] = _UNSETandkimi_api_key: Optional[str] = _UNSETare invalid—the default value_UNSET(an object) doesn't match the declared typeOptional[str](str or None), which will cause mypy/pyright failures. Additionally, theenable_fallbackparameter is accepted but always forced toFalsewithout warning, making the public API misleading. The class docstring (line 81) claims "Includes fallback logic if primary LLM fails," contradicting the actual behavior.Define
_UnsetTypeas a proper type and update the type hints and parameter handling:class _UnsetType: """Sentinel type for unset values.""" pass _UNSET = _UnsetType()Then update
__init__:def __init__( self, claude_api_key: Optional[str] | _UnsetType = _UNSET, kimi_api_key: Optional[str] | _UnsetType = _UNSET, default_provider: LLMProvider = LLMProvider.CLAUDE, enable_fallback: bool = True, track_costs: bool = True ): # ... (api key setup unchanged) # Fallback support is intentionally disabled for now (Kimi fallback not implemented). self.enable_fallback = False if enable_fallback: logger.warning("⚠️ enable_fallback=True requested, but fallback is not supported yet") self.track_costs = track_costsAlso update the class docstring to remove the fallback claim or clarify it's not yet implemented.
♻️ Duplicate comments (1)
cortex/cli.py (1)
27-27: Unused import:export_report.
export_reportis imported but not used in this file.-from cortex.preflight_checker import PreflightChecker, format_report, export_report +from cortex.preflight_checker import PreflightChecker, format_report
🧹 Nitpick comments (7)
tests/test_llm_router.py (3)
95-104: Consider asserting the RuntimeError message (not just type) to avoid false positives.
Right now anyRuntimeErrorwould satisfy this; asserting"Claude API not configured"would make the test more regression-proof.- with self.assertRaises(RuntimeError): - router.route_task(TaskType.USER_CHAT) + with self.assertRaisesRegex(RuntimeError, r"Claude API not configured"): + router.route_task(TaskType.USER_CHAT)
106-115: Consider asserting the RuntimeError message (not just type) to avoid false positives.
Same rationale as above; pinning to"Kimi K2 API not configured"makes intent explicit.- with self.assertRaises(RuntimeError): - router.route_task(TaskType.SYSTEM_OPERATION) + with self.assertRaisesRegex(RuntimeError, r"Kimi K2 API not configured"): + router.route_task(TaskType.SYSTEM_OPERATION)
461-492: AvoidassertRaises(Exception)—prefer a narrower expected exception type.
Catching allExceptioncan mask unrelated bugs (e.g.,AttributeError,TypeError). If the intent is “API failure propagates”, consider asserting the specific exception/message you inject ("API Error").- with self.assertRaises(Exception): + with self.assertRaisesRegex(Exception, r"API Error"): router.complete( messages=[{"role": "user", "content": "Install CUDA"}], task_type=TaskType.SYSTEM_OPERATION )cortex/llm_router.py (2)
29-30: Give_UNSETa proper sentinel type to keep type-checkers honest.
Right now it’s an untypedobject()used as a default forOptional[str], which is a type mismatch.-_UNSET = object() +class _UnsetType: + pass + +_UNSET = _UnsetType()
257-260: LGTM: exception propagation matches “no fallback” semantics—consider logging traceback.
If you want richer debugging,logger.exception(...)will include stack trace automatically.- except Exception as e: - logger.error(f"❌ Error with {routing.provider.value}: {e}") - raise + except Exception: + logger.exception(f"❌ Error with {routing.provider.value}") + raisesrc/config_manager.py (1)
74-81: Good cross-platform guard, but consider logging on security fallback.The platform detection is correct. However, silently swallowing
OSErroron line 79-80 means security enforcement failures go unnoticed. For a security-sensitive function, consider logging a debug/warning message when the chmod fails.if not (hasattr(os, 'getuid') and hasattr(os, 'getgid') and hasattr(os, 'chown')): try: os.chmod(directory, 0o700) except OSError: - pass + # Non-POSIX platform: security enforcement is best-effort + import logging + logging.getLogger(__name__).debug( + "Could not enforce directory security on non-POSIX platform: %s", directory + ) returntests/conftest.py (1)
1-34: Addresses past review feedback on sys.path manipulation.The implementation now only adds the minimal required source roots (
src/andcortex/) instead of the entire repo root, which was the concern in the previous review. The docstring clearly explains the rationale.Minor: Per coding guidelines, consider adding a type annotation for the module-level constant:
-_REPO_ROOT = Path(__file__).resolve().parents[1] +_REPO_ROOT: Path = Path(__file__).resolve().parents[1]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/ci.yml(2 hunks)cortex/cli.py(6 hunks)cortex/llm_router.py(6 hunks)requirements-dev.txt(1 hunks)src/config_manager.py(1 hunks)src/progress_tracker.py(1 hunks)src/sandbox_executor.py(3 hunks)tests/conftest.py(1 hunks)tests/test_llm_router.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/conftest.pysrc/progress_tracker.pytests/test_llm_router.pycortex/llm_router.pycortex/cli.pysrc/config_manager.pysrc/sandbox_executor.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/conftest.pytests/test_llm_router.py
**/*sandbox*.py
📄 CodeRabbit inference engine (AGENTS.md)
Implement Firejail sandboxing for package operations
Files:
src/sandbox_executor.py
🧠 Learnings (3)
📓 Common learnings
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 : Dry-run by default for all installations in command execution
📚 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 : Dry-run by default for all installations in command execution
Applied to files:
cortex/cli.py
📚 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 **/*sandbox*.py : Implement Firejail sandboxing for package operations
Applied to files:
src/sandbox_executor.py
🧬 Code graph analysis (2)
tests/test_llm_router.py (1)
cortex/llm_router.py (3)
LLMRouter(71-412)route_task(167-208)TaskType(32-41)
cortex/cli.py (2)
cortex/preflight_checker.py (3)
format_report(544-613)export_report(616-626)run_all_checks(518-541)cortex/validators.py (1)
validate_api_key(21-41)
🔇 Additional comments (7)
requirements-dev.txt (1)
4-4: Reasonable dependency addition for async test support.The
pytest-asyncio>=0.23.0version is stable and well-established. The addition aligns with the PR's new test requirements for async functionality, and the version constraint is appropriately permissive to allow newer patch releases while maintaining compatibility..github/workflows/ci.yml (1)
26-26: The addition ofpytest-asynciois necessary. The tests in./tests/unit/test_progress_tracker.pyextensively use async patterns with@pytest.mark.asynciodecorators,async deftest functions, andawaitstatements throughout, confirming this dependency is actively required and not overhead.src/progress_tracker.py (1)
40-45: LGTM! Good defensive fix.Defining
plyer_notification = Nonein theImportErrorbranch ensures the symbol is always available, preventing potentialNameErrorif code paths are refactored or if the guard at line 512 is ever bypassed.src/sandbox_executor.py (2)
24-27: LGTM! Proper platform guard forresourcemodule.The
resourcemodule is POSIX-only. Guarding the import and falling back toNoneensures Windows compatibility.
549-568: Good POSIX-specific resource limiting with proper guards.The implementation correctly:
- Guards on
os.name == 'posix'andresource is not None- Sets appropriate limits (memory, CPU time, file size)
- Logs warnings on failure rather than crashing
The closure capturing
selfattributes works correctly sincepreexec_fnruns in the forked child process afterfork()but beforeexec().cortex/cli.py (2)
197-201: Simulation mode correctly short-circuits before API key validation.The early return when
simulate=Trueaddresses the past review concern about misleading "API key not found" errors during simulation.
676-677: CLI wiring for--simulateis correct.The
simulateargument is properly passed through tocli.install().
|
@mikejmorgan-ai we can merge this PR |
Merge Conflicts - Action RequiredHey @Sahilbhatane! This PR has merge conflicts with the main branch. Please:
Once resolved, we'll review for merge! |
Sahilbhatane
force-pushed
the
issue-103
branch
from
88fcc8f to
1178aa3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
14-30: Fix import block sorting to resolve pipeline failure.The import block is unsorted, causing the Ruff linting check to fail in CI. This is blocking the pipeline.
Run the following command to automatically fix the import ordering:
#!/bin/bash ruff check --fix cortex/cli.py
♻️ Duplicate comments (8)
tests/test_preflight_checker.py (4)
11-11: Remove unused import.The
platformmodule is imported but never used in this test file.Apply this diff:
import sys import os -import platform sys.path.insert(0, os.path.join(os.path.dirname(__file__), '..'))
20-22: Remove unused imports.
ServiceInfoandexport_reportare imported but never used in the test file.Apply this diff:
from cortex.preflight_checker import ( PreflightChecker, PreflightReport, DiskInfo, PackageInfo, - ServiceInfo, - format_report, - export_report + format_report )
54-54: Use assertGreater for better error messages.Using
assertTrue(len(disk_info) > 0)provides less informative failure messages thanassertGreater(len(disk_info), 0).Apply this diff:
- self.assertTrue(len(disk_info) > 0) + self.assertGreater(len(disk_info), 0)
49-80: Consider adding cross-platform compatibility tests.The code has platform-specific logic (
_is_windows,_is_mac) and the PR description shows Windows output, but there are no tests verifying behavior on non-Linux platforms. Consider adding tests that mock different platforms to verify disk space checking and path handling work correctly on Windows and macOS.Example test structure:
@patch('platform.system', return_value='Windows') def test_check_disk_space_windows(self, mock_system): """Test disk space checking on Windows""" disk_info = self.checker.check_disk_space() # Verify Windows-specific behavior self.assertIsInstance(disk_info, list)cortex/cli.py (1)
30-30: Remove unused import.The
export_reportfunction is imported but never used in this file. OnlyPreflightCheckerandformat_reportare actually used in the_run_simulationmethod.Apply this diff:
-from cortex.preflight_checker import PreflightChecker, format_report, export_report +from cortex.preflight_checker import PreflightChecker, format_reportdocs/SIMULATION_MODE.md (2)
213-214: Fix incorrect type documentation.The documentation states that
total_download_mbandtotal_disk_required_mbare floats, but they are declared asintin thePreflightReportdataclass (cortex/preflight_checker.py lines 67-68).Apply this diff:
-- `total_download_mb`: Total download size (float) -- `total_disk_required_mb`: Total disk space needed (float) +- `total_download_mb`: Total download size (int) +- `total_disk_required_mb`: Total disk space needed (int)
221-223: Fix incorrect export_report usage example.The example shows
json_str = export_report(report), butexport_reportwrites to a file and returnsNone, not a JSON string. The correct signature isexport_report(report: PreflightReport, filepath: str) -> None.Apply this diff:
-# Export report to JSON -from cortex.preflight_checker import export_report -export_report(report, "report.json") +# Export report to JSON file +from cortex.preflight_checker import export_report +export_report(report, "report.json") # Writes to file, returns Nonecortex/preflight_checker.py (1)
328-328: Fix type hint: useAnyinstead ofany.The return type hint should use
Anyfrom the typing module (already imported on line 16), not the built-inanytype.Apply this diff:
- def _get_package_info_from_llm(self, software: str, os_info: Dict[str, str]) -> Dict[str, any]: + def _get_package_info_from_llm(self, software: str, os_info: Dict[str, str]) -> Dict[str, Any]: """Query LLM for real package information including sizes"""
🧹 Nitpick comments (11)
cortex/sandbox/sandbox_executor.py (2)
622-623: Redundant initialization ofprocess.Line 622 initializes
process = None, but line 624 immediately reassigns it to thePopenresult. This initialization is only useful ifPopenraises an exception before assignment, but in that case theexcept Exceptionblock at line 669 would handle it without referencingprocess.However, this does provide safety for the
TimeoutExpiredhandler at line 655. The initialization is acceptable as defensive coding.Consider combining into a single comment explaining the purpose:
- process = None - - process = subprocess.Popen( + process = None # Initialize for timeout handler safety + process = subprocess.Popen(
167-167: Simplify regex pattern to remove unnecessary complexity.The pattern
\\?\+sis overly permissive. It matches bothchmod +sandchmod \+s, but the test input at line 318 intests/unit/test_sandbox_executor.pyuses only"chmod +s /tmp/x"without an escaped backslash. Additionally, comparable patterns incortex/utils/commands.pyandcortex/coordinator.pyuse the simpler formr"chmod\s+\+s"without the optional backslash. Consider removing the\\?to align with the other patterns in the codebase and match only the actual chmod syntax used in practice.tests/unit/test_sandbox_executor.py (2)
303-345: Good data-driven test approach, but missing blank line before next method.The refactoring to a dictionary-based approach ensures all dangerous patterns have corresponding test cases. The
assertRegexsanity check (line 342) is valuable for catching mismatches between patterns and examples.However, there's a missing blank line between the end of this method (line 345) and the next method definition (line 346), which violates PEP 8's two-blank-line rule between top-level definitions within a class.
is_valid, violation = self.executor.validate_command(test_cmd) self.assertFalse(is_valid, f"Pattern should be blocked: {pattern}") + def test_path_traversal_protection(self):As per coding guidelines, follow PEP 8 style guide.
337-339: Test will fail if a pattern is added to production without updating the test.The assertion on line 339 ensures that every pattern in
DANGEROUS_PATTERNShas a corresponding example in theexamplesdict. This is excellent for catching drift between production and test code.However, consider adding a reverse check to ensure no stale examples exist in the dict that don't correspond to actual patterns:
for pattern in self.executor.DANGEROUS_PATTERNS: test_cmd = examples.get(pattern) self.assertIsNotNone(test_cmd, f"Missing example command for pattern: {pattern}") + + # Ensure no stale examples exist + for pattern in examples: + self.assertIn(pattern, self.executor.DANGEROUS_PATTERNS, + f"Stale example for removed pattern: {pattern}").github/workflows/ci.yml (1)
72-72: Remove redundant linting tools from test dependencies.The test job now installs
blackandruff, but these are already installed and executed in the separatelintjob (lines 26-35). Installing formatting/linting tools in the test job is redundant and wastes CI time.Apply this diff to remove the redundant tools:
- pip install pytest pytest-cov pytest-timeout pytest-asyncio pytest-mock black ruff + pip install pytest pytest-cov pytest-timeout pytest-asyncio pytest-mockdocs/SIMULATION_MODE.md (1)
37-37: Add language specifiers to code blocks.Code blocks at lines 37, 57, 116, and 124 are missing language specifiers, which prevents proper syntax highlighting and may cause linting warnings.
Add language specifiers (e.g.,
text,bash, or appropriate language) to all code blocks to improve documentation quality and satisfy markdownlint rules.Also applies to: 57-57, 116-116, 124-124
cortex/preflight_checker.py (5)
11-11: Remove unused socket import.The
socketmodule is imported but never used in this file.Apply this diff:
import os import sys import shutil -import socket import platform
270-275: Consider usingshlex.quote()to prevent command injection.Line 272 embeds a user-controlled
pathvariable directly into a shell command using an f-string. While single quotes provide some protection, special characters could still be exploited.Apply this diff:
+ import shlex + def _get_filesystem_type(self, path: str) -> str: """Get filesystem type for a path on Linux""" - success, output = self._run_shell_command(f"df -T '{path}' | tail -1 | awk '{{print $2}}'") + success, output = self._run_shell_command(f"df -T {shlex.quote(path)} | tail -1 | awk '{{print $2}}'") if success and output: return output return 'unknown'
390-480: Consider extracting shared logic into a helper method.The
check_docker,check_containerd, andcheck_softwaremethods follow a very similar pattern:
- Check if package is installed
- If not, query LLM for package info
- Fall back to estimates if LLM fails or no API key
Extracting this pattern into a helper method would reduce duplication and improve maintainability.
Example helper signature:
def _check_and_plan_install( self, package_name: str, version_cmd: Optional[List[str]], fallback_estimate: Dict[str, str], software_display_name: Optional[str] = None ) -> PackageInfo: """Check package and plan installation if not present""" # ... shared logic
616-626: LGTM! Export function correctly usesasdictfor recursive conversion.The function properly uses
asdict(report)which recursively converts nested dataclasses to dicts, then writes to JSON. The previous issue with redundant manual conversions has been resolved.For consistency, consider moving the
asdictimport to the module level (line 15) alongside other imports:from dataclasses import dataclass, field +from dataclasses import asdictThen remove it from line 618.
8-17: Consider sorting imports per PEP 8 / Ruff conventions.Static analysis indicates the import block is not formatted according to the project's linting rules. Running
ruff check --fixwould automatically sort these imports.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/ci.yml(1 hunks)cortex/cli.py(7 hunks)cortex/config_manager.py(3 hunks)cortex/llm_router.py(9 hunks)cortex/preflight_checker.py(1 hunks)cortex/progress_tracker.py(1 hunks)cortex/sandbox/sandbox_executor.py(6 hunks)docs/SIMULATION_MODE.md(1 hunks)tests/conftest.py(1 hunks)tests/test_cli.py(1 hunks)tests/test_llm_router.py(3 hunks)tests/test_preflight_checker.py(1 hunks)tests/unit/test_sandbox_executor.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/conftest.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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_llm_router.pycortex/cli.pycortex/progress_tracker.pycortex/llm_router.pytests/unit/test_sandbox_executor.pytests/test_cli.pytests/test_preflight_checker.pycortex/sandbox/sandbox_executor.pycortex/config_manager.pycortex/preflight_checker.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_llm_router.pytests/unit/test_sandbox_executor.pytests/test_cli.pytests/test_preflight_checker.py
**/*sandbox*.py
📄 CodeRabbit inference engine (AGENTS.md)
Implement Firejail sandboxing for package operations
Files:
tests/unit/test_sandbox_executor.pycortex/sandbox/sandbox_executor.py
🧠 Learnings (5)
📓 Common learnings
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 : Dry-run by default for all installations in command execution
📚 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 : Dry-run by default for all installations in command execution
Applied to files:
cortex/cli.py
📚 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 tests/**/*.py : Maintain >80% test coverage for pull requests
Applied to files:
.github/workflows/ci.yml
📚 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/config_manager.py
📚 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 : Type hints required in Python code
Applied to files:
cortex/preflight_checker.py
🧬 Code graph analysis (5)
tests/test_llm_router.py (1)
cortex/llm_router.py (4)
LLMRouter(82-426)route_task(179-219)TaskType(39-49)complete(221-266)
tests/unit/test_sandbox_executor.py (2)
cortex/sandbox/sandbox_executor.py (1)
validate_command(294-342)cortex/utils/commands.py (1)
validate_command(134-196)
tests/test_cli.py (1)
cortex/cli.py (1)
main(647-794)
cortex/config_manager.py (1)
cortex/logging_system.py (1)
debug(194-196)
cortex/preflight_checker.py (1)
cortex/llm/interpreter.py (1)
CommandInterpreter(17-275)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 14-14: Ruff check failed. Import block is un-sorted or un-formatted. I001 Import block is un-sorted or un-formatted. Run 'ruff check --fix' to fix imports.
🪛 GitHub Actions: Cortex Automation
cortex/llm_router.py
[error] 317-318: IndentationError: expected an indented block after 'if' statement on line 317. This caused pytest collection to fail during 'pytest tests/ -v --cov=cortex --cov-report=xml --cov-report=term-missing'.
🪛 GitHub Check: Lint
cortex/cli.py
[failure] 14-30: Ruff (I001)
cortex/cli.py:14:1: I001 Import block is un-sorted or un-formatted
[failure] 364-364: Ruff (W293)
cortex/cli.py:364:1: W293 Blank line contains whitespace
[failure] 359-359: Ruff (W293)
cortex/cli.py:359:1: W293 Blank line contains whitespace
[failure] 355-355: Ruff (W293)
cortex/cli.py:355:1: W293 Blank line contains whitespace
[failure] 351-351: Ruff (W293)
cortex/cli.py:351:1: W293 Blank line contains whitespace
[failure] 337-337: Ruff (W293)
cortex/cli.py:337:1: W293 Blank line contains whitespace
cortex/llm_router.py
[failure] 207-207: Ruff (W293)
cortex/llm_router.py:207:1: W293 Blank line contains whitespace
[failure] 318-318: Ruff (invalid-syntax)
cortex/llm_router.py:318:13: invalid-syntax: Expected an indented block after if statement
cortex/preflight_checker.py
[failure] 8-17: Ruff (I001)
cortex/preflight_checker.py:8:1: I001 Import block is un-sorted or un-formatted
🪛 markdownlint-cli2 (0.18.1)
docs/SIMULATION_MODE.md
37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
57-57: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
116-116: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
124-124: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (31)
cortex/sandbox/sandbox_executor.py (3)
25-28: LGTM! Good cross-platform compatibility fix.The guarded import of
resourcehandles non-POSIX environments (e.g., Windows) gracefully. The# type: ignorecomment appropriately silences type checker warnings for this conditional import pattern.
600-604: LGTM! Proper conditional for POSIX-only resource limits.The condition correctly checks all three requirements before setting resource limits:
- Not using Firejail (which handles its own limits)
- Running on POSIX (
os.name == "posix")resourcemodule is available
655-659: Good defensive handling for process termination.The None-check and try/except guard around
process.kill()prevents crashes when:
Popenfailed before assignment- The process already terminated naturally
This is a solid improvement for robustness.
tests/unit/test_sandbox_executor.py (1)
318-318: Verify the pattern-example pairing is intentional.The example
"chmod +s /tmp/x"uses a literal+, while the patternr"chmod\s+\\?\+s"was specifically modified to also match an escaped\+.If the intent is to test escaped input matching, the example should be
"chmod \\+s /tmp/x". If the intent is to test normal input, the original patternr"chmod\s+\+s"would suffice.Clarify whether the regex change was to support escaped shell input or for another reason. The current test only validates the unescaped case.
cortex/progress_tracker.py (1)
45-51: LGTM: Explicit None assignment improves clarity.The explicit
plyer_notification = Noneassignment in the ImportError block ensures the variable is always defined, improving code clarity. While not strictly necessary given thePLYER_AVAILABLEguard at line 514, this defensive coding practice prevents potential NameError issues if the code is refactored.cortex/config_manager.py (2)
9-21: LGTM: Logging setup supports cross-platform error handling.The logging setup enables graceful degradation on non-POSIX platforms. This aligns with the defensive changes in
_enforce_directory_securitybelow.
79-86: LGTM: Graceful handling of non-POSIX platforms.The added platform detection and best-effort chmod provides cross-platform compatibility without compromising the Linux-first security model. Using debug-level logging for non-critical failures on unsupported platforms is appropriate.
tests/test_cli.py (1)
187-189: LGTM: Test expectations correctly updated for new signature.The test assertions have been properly updated to expect the new
simulate=Falseparameter in allCortexCLI.installcalls. This maintains test coverage while accommodating the extended API signature introduced in cortex/cli.py.Also applies to: 197-199, 207-209
tests/test_llm_router.py (1)
82-87: LGTM: Test expectations updated to match disabled fallback behavior.The tests have been correctly updated to reflect the new LLMRouter behavior where fallback is disabled. All tests now properly expect
RuntimeErrorwhen providers are unavailable, which aligns with the changes in cortex/llm_router.py. The error message assertions match the actual error messages raised by the router.Also applies to: 89-94, 97-104, 108-115, 420-447
cortex/cli.py (3)
47-76: LGTM: Improved API key retrieval logic.The refactored
_get_api_keymethod now clearly documents its behavior and properly handles Ollama mode. The provider-specific key lookup is more explicit and maintainable.
193-208: LGTM: Install signature properly extended for simulate mode.The
installmethod signature now includes thesimulateparameter with appropriate default value, and the early return for simulation mode is correctly placed before API key validation (since simulation doesn't require an API key).
695-702: LGTM: Mutually exclusive group properly configured.The
--simulate,--execute, and--dry-runflags are correctly configured as mutually exclusive options, preventing conflicting modes from being specified together.cortex/llm_router.py (8)
29-36: LGTM! Sentinel pattern correctly implemented.The
_UnsetTypesingleton with empty__slots__is a clean way to distinguish "unset" fromNoneor a provided key.
121-145: LGTM! Unset semantics correctly implemented.The conditional logic properly distinguishes between "not provided" (_UNSET → use environment) and "explicitly set to None" (use None directly). The inline comments clarify the intent.
147-150: LGTM! Fallback disabled with clear user communication.The warning appropriately notifies users when their
enable_fallbacksetting is ignored, and the inline comment documents the intentional behavior.
193-197: LGTM! Force provider validation prevents late failures.The early validation ensures that a forced provider has a configured client, preventing confusing
AttributeErrorexceptions later in thecomplete()method.
208-214: LGTM! No-fallback behavior correctly implemented.The provider availability checks correctly raise errors instead of attempting fallback, aligning with the disabled fallback behavior. The comments clearly explain the rationale.
264-266: LGTM! Exception handling correctly logs and propagates errors.Using
logger.exception()automatically captures the full traceback, and re-raising the exception allows callers to handle failures appropriately without silent fallback.
268-310: LGTM! Claude message normalization robustly handles edge cases.The refactored logic:
- Validates roles explicitly (system, user, assistant)
- Accumulates multiple system messages and joins them
- Ensures at least one non-system message is present
- Builds kwargs correctly with optional system parameter
This addresses the previous review concerns about multiple system messages and role validation.
325-333: LGTM! Claude response correctly constructed.The response object uses the extracted
content_textand includes all required metadata. The efficient list collection and join for text assembly is a good practice.cortex/preflight_checker.py (11)
20-70: LGTM! Dataclass definitions follow best practices.All dataclasses use appropriate type hints and correctly use
field(default_factory=...)for mutable default values (lists and dicts).
83-102: LGTM! Platform-specific disk paths correctly configured.The initialization properly detects the platform and sets appropriate disk check paths for Windows, macOS, and Linux. This addresses previous concerns about hardcoded paths.
104-135: LGTM! Command execution helpers have proper error handling.Both methods correctly handle timeouts and exceptions, returning tuples with success status and output/error message.
180-182: LGTM! Exception handling is appropriately documented.The inline comment on line 181 adequately explains why the exception is caught and ignored (malformed or unreadable
/etc/os-release), with sensible fallback to defaults.
253-255: LGTM! Exception handling is appropriately documented.The inline comment adequately explains why the exception is caught (path not accessible or permission denied), with sensible fallback to zero values.
322-324: LGTM! Exception handling is appropriately documented.The inline comment adequately explains that when the fallback provider also fails, the method returns None and the caller will use estimates instead.
383-386: LGTM! Exception now logged with warning.The exception is caught and a warning is appended to the report, providing visibility into LLM failures while gracefully falling back to estimates. This addresses the previous review concern.
488-500: LGTM! Exception handling now provides visibility.When size parsing fails, a warning is added to the report identifying the problematic package. This provides debugging visibility while allowing the process to continue.
505-511: LGTM! Cross-platform root disk detection.Using
Path(os.getcwd()).anchorcorrectly handles both Windows (e.g., 'C:\') and Unix-like systems ('/'), with proper case-insensitive path comparison. This addresses previous Windows compatibility concerns.
518-541: LGTM! Orchestration logic correctly coordinates all checks.The
run_all_checksmethod properly sequences OS detection, disk checks, software checks, and requirement calculation. Thesoftware_lowervariable (line 529) is used in the conditional on line 531.
577-584: LGTM! Cross-platform root disk path handling.The platform-specific logic correctly determines the root disk path for Windows (SystemDrive + '\') and Unix-like systems ('/'), ensuring the disk space check works across platforms.
|
@mikejmorgan-ai rebased and we can merge this PR |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
cortex/sandbox/sandbox_executor.py (2)
167-167: Clarify the intent of the regex pattern change.The pattern
r"chmod\s+\\?\+s"matches bothchmod +sandchmod \+s(with escaped plus). The test example"chmod +s /tmp/x"matches correctly, but the motivation for also matching the escaped form is unclear. Consider adding a comment explaining why both forms need to be blocked.- r"chmod\s+\\?\+s", # Setuid bit (also matches escaped + from tests) + r"chmod\s+\\?\+s", # Setuid bit (+s or \+s)
623-624: Defensive initialization, but consider removing the redundant line.Initializing
process = NonebeforePopenis defensive, but line 623 (process = None) is immediately followed by line 625 (process = subprocess.Popen(...)). The initialization only matters ifPopenraises an exception and execution jumps to theexcept TimeoutExpiredblock—which wouldn't happen sinceTimeoutExpiredis raised bycommunicate(), notPopen.Consider removing the redundant initialization or moving the process kill logic inside a check that verifies
processwas successfully created.- process = None - process = subprocess.Popen(Alternatively, keep it but add a brief comment explaining the defensive purpose.
tests/test_interpreter.py (1)
174-174: Add a comment explaining the test intent.Setting
interpreter.cache = Nonetests the behavior when no cache is configured. Consider adding a brief comment to clarify this is intentional test setup for the no-cache scenario.interpreter = CommandInterpreter(api_key=self.api_key, provider="openai") interpreter.client = mock_client + # Ensure parse_with_context handles missing cache interpreter.cache = Nonecortex/cli.py (1)
21-21: Remove unusedexport_reportimport.The
export_reportfunction is imported but never used in this file. The simulation mode only usesformat_reportto print the report—exporting to a file is not implemented.-from cortex.preflight_checker import PreflightChecker, export_report, format_report +from cortex.preflight_checker import PreflightChecker, format_reportcortex/llm_router.py (1)
134-136: Update docstring to reflect disabled fallback.The docstring still describes
enable_fallbackas functional ("Try alternate LLM if primary fails"), but the parameter is now ignored and fallback is disabled. This could confuse users.default_provider: Fallback provider if routing fails - enable_fallback: Try alternate LLM if primary fails + enable_fallback: DEPRECATED - fallback is currently disabled track_costs: Track token usage and costscortex/preflight_checker.py (1)
388-470: Consider extracting shared LLM query logic.The
check_docker,check_containerd, andcheck_softwaremethods share a common pattern: query LLM for package info, fall back to estimates if unavailable. Extracting this into a helper method would reduce duplication.Example helper approach:
def _get_or_estimate_package_info( self, software: str, fallback_packages: list[dict[str, str]], fallback_config_changes: list[str] | None = None, ) -> None: """Query LLM for package info or use fallback estimates.""" if self.api_key: pkg_info = self._get_package_info_from_llm(software, self.report.os_info) if pkg_info.get("packages"): self.report.packages_to_install.extend(pkg_info["packages"]) if pkg_info.get("config_changes"): self.report.config_changes.extend(pkg_info["config_changes"]) return # Fallback to estimates self.report.packages_to_install.extend(fallback_packages) if fallback_config_changes: self.report.config_changes.extend(fallback_config_changes) if not self.api_key: self.report.warnings.append( "Package sizes are estimates - provide API key for accurate sizes" )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cortex/cli.py(7 hunks)cortex/config_manager.py(3 hunks)cortex/llm_router.py(8 hunks)cortex/preflight_checker.py(1 hunks)cortex/sandbox/sandbox_executor.py(6 hunks)docs/SIMULATION_MODE.md(1 hunks)tests/test_cli.py(1 hunks)tests/test_hardware_detection.py(2 hunks)tests/test_interpreter.py(1 hunks)tests/test_llm_router.py(3 hunks)tests/test_preflight_checker.py(1 hunks)tests/unit/test_sandbox_executor.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_cli.py
- tests/test_preflight_checker.py
- tests/test_llm_router.py
- cortex/config_manager.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/unit/test_sandbox_executor.pycortex/sandbox/sandbox_executor.pytests/test_interpreter.pycortex/llm_router.pycortex/cli.pytests/test_hardware_detection.pycortex/preflight_checker.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/unit/test_sandbox_executor.pytests/test_interpreter.pytests/test_hardware_detection.py
**/*sandbox*.py
📄 CodeRabbit inference engine (AGENTS.md)
Implement Firejail sandboxing for package operations
Files:
tests/unit/test_sandbox_executor.pycortex/sandbox/sandbox_executor.py
🧠 Learnings (3)
📚 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 **/*sandbox*.py : Implement Firejail sandboxing for package operations
Applied to files:
cortex/sandbox/sandbox_executor.py
📚 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 : Dry-run by default for all installations in command execution
Applied to files:
cortex/cli.py
📚 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 : Type hints required in Python code
Applied to files:
cortex/preflight_checker.py
🧬 Code graph analysis (4)
tests/unit/test_sandbox_executor.py (2)
cortex/sandbox/sandbox_executor.py (1)
validate_command(294-342)cortex/utils/commands.py (1)
validate_command(134-196)
tests/test_interpreter.py (1)
tests/test_graceful_degradation.py (1)
cache(28-31)
cortex/cli.py (1)
cortex/preflight_checker.py (4)
PreflightChecker(76-536)export_report(627-637)format_report(539-624)run_all_checks(513-536)
cortex/preflight_checker.py (1)
cortex/llm/interpreter.py (1)
CommandInterpreter(17-275)
⏰ 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). (1)
- GitHub Check: Build Package
🔇 Additional comments (24)
tests/test_hardware_detection.py (2)
287-294: LGTM! Good cross-platform test fix.Adding
create=Truecorrectly allows mockingos.statvfson non-POSIX platforms where this function doesn't exist, preventingAttributeErrorduring test collection.
320-331: LGTM! Consistent cross-platform fix foros.uname.tests/unit/test_sandbox_executor.py (1)
303-345: Good refactor to data-driven testing.This approach ensures the test stays synchronized with
DANGEROUS_PATTERNSin production code. The sanity check on line 342 (assertRegex) catches mismatches between patterns and examples early.One consideration: if a new pattern is added to
DANGEROUS_PATTERNSwithout updating this test'sexamplesdict, line 339 will fail with a clear error message, which is the intended behavior.cortex/sandbox/sandbox_executor.py (3)
24-28: Good cross-platform compatibility fix.Guarding the
resourceimport handles non-POSIX environments (Windows) where this module is unavailable. The# type: ignorecomment appropriately suppresses type checker warnings for the conditional import.
600-621: Well-structured cross-platform resource limiting.The three-way guard (
not self.firejail_path,os.name == "posix",resource is not None) correctly handles all edge cases:
- Firejail already handles limits when available
preexec_fnonly works on POSIXresourcemodule only exists on POSIX
655-660: LGTM! Safe process termination handling.The None check and try/except wrapper correctly handle edge cases where the process may have already terminated or was never created successfully.
docs/SIMULATION_MODE.md (2)
213-214: No changes needed. The type documentation is correct—total_download_mbandtotal_disk_required_mbare both defined asintin thePreflightReportdataclass atcortex/preflight_checker.pylines 71-72, matching the documentation.
221-223: Documentation example is accurate. Theexport_reportfunction signature incortex/preflight_checker.pymatches the documented usage: it accepts a report and filename string, writes to a JSON file, and returns None as shown in the example.cortex/cli.py (4)
192-208: Consolidatedinstallmethod looks correct.The previous duplicate
installmethod issue has been resolved. The single method now handles thesimulateflag early (line 206-207) before requiring an API key, which correctly addresses the simulation mode flow.
337-366: Simulation method implementation is sound.The
_run_simulationmethod correctly:
- Retrieves API key based on provider (optional for simulation)
- Falls back to
'openai'provider for PreflightChecker when using Ollama- Handles exceptions gracefully with user-friendly error messages
- Returns appropriate exit codes based on blocking errors
694-701: Good use of mutually exclusive group for install modes.Making
--execute,--dry-run, and--simulatemutually exclusive prevents confusing or conflicting flag combinations.
763-768: CLI wiring for simulation mode is correct.The
simulateargument is properly passed through tocli.install()with a safegetattrfallback.cortex/llm_router.py (4)
30-37: Sentinel pattern is well-implemented.Using
_UnsetTypewith__slots__ = ()and a__repr__is a clean way to distinguish between "not provided" and explicitNone. This allows callers to explicitly disable environment variable fallback by passingNone.
193-215: Proper validation of provider availability.The
route_taskmethod now validates that forced or routed providers are configured, raising clearRuntimeErrormessages instead of silently failing later. This addresses the previous review feedback about bypassing availability checks.
265-267: Exception handling now logs and re-raises.Using
logger.exception()ensures the full stack trace is logged for debugging, and re-raising allows callers to handle the failure appropriately. This is cleaner than silent fallback.
277-321: Claude message handling improvements are comprehensive.The refactored
_complete_claudemethod now:
- Accumulates multiple system messages (line 307)
- Validates roles against Claude's supported set (lines 283-284)
- Requires at least one non-system message (lines 296-297)
- Uses efficient list collection + join for content extraction (lines 315-321)
These changes address all the concerns from previous reviews.
cortex/preflight_checker.py (8)
20-74: Dataclass definitions are well-structured.The
DiskInfo,PackageInfo,ServiceInfo, andPreflightReportdataclasses are properly typed and documented. Usingfield(default_factory=...)for mutable defaults is correct.
83-106: Platform-specific disk paths are now handled correctly.The initialization properly sets platform-appropriate disk check paths for Windows, macOS, and Linux. This addresses the previous feedback about hardcoded Linux paths.
Minor note:
DISK_CHECK_PATHSis declared as a class attribute (line 84) but overwritten as an instance attribute in__init__. This works but could be clearer by using a different name or removing the class-level declaration.
262-269: Command injection risk is mitigated withshlex.quote.The use of
shlex.quote(path)properly escapes the path before embedding it in the shell command, addressing the command injection concern from previous reviews.
175-177: Silent exception handling is now documented.The
except Exception: passblock now includes an explanatory comment clarifying the fallback behavior. This addresses the previous feedback about undocumented silent exception handling.
524-531: Variablesoftware_loweris now used.The
software_lowervariable is assigned and used in the conditional check on line 526. The previous unused variable concern is resolved.
539-624: Report formatting is clean and correctly implemented.The
format_reportfunction properly:
- Uses regular strings where no interpolation is needed (resolving the f-string issue)
- Handles platform-specific root path detection (lines 581-594)
- Limits warnings and suggestions to prevent overwhelming output
627-637: Export function correctly usesasdictwithout redundant conversion.The
export_reportfunction now correctly relies onasdict()for recursive conversion, with an explanatory comment. The redundant manual conversion from previous versions has been removed.
76-94: Public API is well-documented with docstrings.All public methods in
PreflightCheckerand the module-level functions (format_report,export_report) have docstrings as required by the coding guidelines.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
cortex/cli.py (2)
21-21: Remove unused import.The
export_reportfunction is imported but never used in this file.🔎 Apply this diff to remove the unused import:
-from cortex.preflight_checker import PreflightChecker, export_report, format_report +from cortex.preflight_checker import PreflightChecker, format_report
192-205: Critical: Fix syntax error and duplicate method definitions.The code has a critical syntax error causing the pipeline failure:
- The
installmethod definition at lines 192-198 has no body (ends with:but no indented block follows).- The
doctormethod at line 199 appears immediately after, causing the "Expected an indented block" error at line 200.- A second
installmethod definition exists at line 206, creating a duplicate that shadows the first.This structure causes:
- Pipeline failure: Syntax error at line 200 (invalid-syntax: Expected an indented block)
- Runtime TypeError: The second
install()definition (line 206) doesn't accept thesimulateparameter, so--simulatewill fail withTypeError: install() got an unexpected keyword argument 'simulate'🔎 Consolidate to a single install method:
- def install( - self, - software: str, - execute: bool = False, - dry_run: bool = False, - simulate: bool = False, - ): # Run system health checks def doctor(self): from cortex.doctor import SystemDoctor doctor = SystemDoctor() return doctor.run_checks() - def install(self, software: str, execute: bool = False, dry_run: bool = False): + def install( + self, + software: str, + execute: bool = False, + dry_run: bool = False, + simulate: bool = False, + ): # Validate input first is_valid, error = validate_install_request(software) if not is_valid: self._print_error(error) return 1 - # Handle simulation mode early (no install execution) + # Handle simulation mode first - no API key needed if simulate: return self._run_simulation(software) api_key = self._get_api_key()cortex/preflight_checker.py (1)
285-319: Critical: Fix incorrect import path in fallback handler.Line 309 uses an incorrect import path
from LLM.interpreter import CommandInterpreterwhich will cause anImportError. The correct path isfrom cortex.llm.interpreter import CommandInterpreter(as used correctly on line 296).🔎 Apply this diff to fix the import:
) if fallback_key: - from LLM.interpreter import CommandInterpreter + from cortex.llm.interpreter import CommandInterpreter self._llm_client = CommandInterpreter(
🧹 Nitpick comments (2)
cortex/preflight_checker.py (2)
388-441: Consider extracting common LLM query pattern.The
check_docker()andcheck_containerd()methods share nearly identical logic for querying LLM and falling back to estimates. Consider extracting a helper method like_check_and_estimate_package(name: str, version_cmd: List[str], fallback_size_mb: int)to reduce duplication.This would also apply to
check_software()at lines 442-470, which follows the same pattern.
120-130: Document security considerations for shell command execution.The
_run_shell_commandmethod usesshell=True, which can pose security risks if user-controlled input is not properly sanitized. While the current usage at line 265 properly usesshlex.quote(), consider adding a docstring warning that callers must sanitize inputs.Alternatively, consider refactoring to avoid
shell=Truewhere possible by using list-form commands with_run_commandinstead.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.py(8 hunks)cortex/preflight_checker.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.pycortex/preflight_checker.py
🧠 Learnings (2)
📚 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 : Dry-run by default for all installations in command execution
Applied to files:
cortex/cli.py
📚 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 : Type hints required in Python code
Applied to files:
cortex/preflight_checker.py
🧬 Code graph analysis (2)
cortex/cli.py (3)
cortex/preflight_checker.py (3)
export_report(627-637)format_report(539-624)run_all_checks(513-536)cortex/validators.py (1)
validate_api_key(20-48)cortex/first_run_wizard.py (1)
_print_error(746-748)
cortex/preflight_checker.py (1)
cortex/llm/interpreter.py (1)
CommandInterpreter(17-275)
🪛 GitHub Actions: CI
cortex/cli.py
[error] 200-200: Ruff check failed. invalid-syntax: Expected an indented block after function definition in cortex/cli.py:200.
⏰ 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). (1)
- GitHub Check: test (3.11)
🔇 Additional comments (11)
cortex/cli.py (3)
660-679: LGTM!The help text correctly documents the new
--simulateflag with clear examples.
706-713: LGTM!The mutually exclusive group properly ensures only one of
--execute,--dry-run, or--simulatecan be specified at a time.
775-780: LGTM once install method is fixed.The call site correctly passes the
simulateparameter, but this will only work after consolidating the duplicateinstall()definitions.cortex/preflight_checker.py (8)
1-18: LGTM!Module documentation is clear, and imports are properly organized with appropriate type hints.
Based on coding guidelines, type hints are correctly used in this file.
20-74: LGTM!The dataclass definitions follow Python best practices with proper type hints and default factories for mutable fields.
Based on coding guidelines, type hints are properly included for all fields.
87-107: LGTM!The platform-specific initialization of
DISK_CHECK_PATHSproperly addresses different operating systems, fixing the issue of hardcoded Linux paths mentioned in previous reviews.
379-386: LGTM!The exception handling now properly adds a warning message when LLM response parsing fails, addressing the concern from a previous review about silent failures.
472-512: LGTM!The disk space calculation uses
Path.anchorto determine the root path, making it cross-platform compatible. Exception handling properly warns about unparseable package sizes.
513-537: LGTM!The orchestration method properly sequences all checks and uses platform-appropriate logic. The
software_lowervariable at line 524 is correctly used in the conditional at line 526.
580-598: LGTM!The platform-specific root path determination properly handles Windows (using
SystemDrive), addressing previous concerns about hardcoded Unix paths.
627-637: LGTM!The export function correctly uses
asdict()which handles nested dataclasses automatically, avoiding the redundant conversions flagged in previous reviews.
|
@mikejmorgan-ai done we can merge this now. |
|
|
|
|
closing cause it is redundant |
|
Approved checked after discussion |
3 participants
Summary
--simulateflag enables preview mode for installations, showing what would be installed without making any changes. This helps users:Type of Change
Checklist
Testing
can be testes with
pytestor simply runtest_preflight.pyfor this implementation test only or can run this command to check it as user perspective -output example -
Note -
make sure to test it with API and without API for both scenarios.
video
https://drive.google.com/file/d/12duXG6eFfzVUKfpZxEyLClhEsw13a54s/view?usp=sharing
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.