Installation simulation mode fix #103 #264
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a Simulation Mode to the CLI: Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as cortex/cli.py
participant Checker as PreflightChecker
participant Host as Host System
participant LLM as LLM Client
participant Reporter as format_report
User->>CLI: run "cortex install <software> --simulate"
CLI->>Checker: instantiate (api_key, provider)
CLI->>Checker: run_all_checks(software)
Checker->>Host: detect OS / distro / kernel
Host-->>Checker: os info
Checker->>Host: check disk paths, packages, services
Host-->>Checker: DiskInfo / PackageInfo / ServiceInfo
alt provider/API key available
Checker->>LLM: request package metadata (sizes, config)
LLM-->>Checker: metadata / suggestions
end
Checker->>Checker: calculate_requirements()
Checker-->>CLI: PreflightReport
CLI->>Reporter: format_report(report, software)
Reporter-->>CLI: formatted text
CLI->>User: print report and exit code (0 success / 1 failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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.
| 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) | ||
|
|
There was a problem hiding this comment.
Duplicate install definitions cause a TypeError with --simulate
There are two CortexCLI.install methods:
- Lines 116–120: a stub that only handles
simulateand then returns. - Lines 185–303: the full install implementation that does not accept
simulate.
Because Python uses the last definition, the stub at 116–120 is completely shadowed. The active signature is install(self, software, execute=False, dry_run=False). However, main() now calls:
return cli.install(args.software, execute=args.execute, dry_run=args.dry_run, simulate=args.simulate)This will raise TypeError: install() got an unexpected keyword argument 'simulate' at runtime. The new tests don’t catch this because they patch CortexCLI.install.
You should consolidate to a single install method that supports simulate and remove the duplicate definition and unreachable elif args.command == 'install' branch. For example:
@@
- def _clear_line(self):
- sys.stdout.write('\r\033[K')
- sys.stdout.flush()
-
- 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 _clear_line(self):
+ sys.stdout.write('\r\033[K')
+ sys.stdout.flush()
@@
- def install(self, software: str, execute: bool = False, dry_run: bool = False):
- # Validate input first
+ 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)
+
+ # Validate input first
is_valid, error = validate_install_request(software)
@@
- try:
- if args.command == 'install':
- return cli.install(args.software, execute=args.execute, dry_run=args.dry_run, simulate=args.simulate)
+ try:
+ if args.command == 'install':
+ return cli.install(
+ args.software,
+ execute=args.execute,
+ dry_run=args.dry_run,
+ simulate=args.simulate,
+ )
@@
- elif args.command == 'status':
- return cli.status()
- 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 preserves all existing behavior, fixes the --simulate runtime error, and resolves the static-analysis warnings about unused parameters in the stub.
Also applies to: 185-303, 660-669
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 116-116: Remove the unused function parameter "dry_run".
[warning] 116-116: Remove the unused function parameter "execute".
🤖 Prompt for AI Agents
In cortex/cli.py around lines 116-120 (duplicate stub), lines 185-303 (full
implementation), and lines 660-669 (another duplicate), there are multiple
install() definitions causing a TypeError when --simulate is passed because the
active method signature lacks the simulate parameter; remove the duplicate
stub(s) and consolidate into a single install(self, software: str, execute: bool
= False, dry_run: bool = False, simulate: bool = False) implementation that
preserves the full install behavior and handles the simulate branch up front
(returning _run_simulation when simulate is True), and also delete the
now-unreachable elif args.command == 'install' branch to avoid dead code and
static-analysis warnings.
1 participant
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
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.