install script #313
install script #313
Conversation
|
Warning Rate limit exceeded@dhvll has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 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 (1)
WalkthroughThe PR introduces comprehensive batch package installation orchestration with dependency graph analysis and parallel execution, extends CLI to support multi-package workflows, hardens the installation history module against database failures, and adds a one-liner Linux installer script. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI (install command)
participant BI as BatchInstaller
participant DR as DependencyResolver
participant IC as InstallationCoordinator
CLI->>BI: install_batch(packages)
activate BI
BI->>BI: analyze_packages() [concurrent]
activate BI
BI->>DR: resolve_dependencies(package)
DR-->>BI: DependencyGraph
BI->>BI: record PackageInstallation
deactivate BI
BI->>BI: optimize_dependency_graph()
activate BI
BI->>BI: identify shared dependencies
BI->>BI: topological_sort()
BI->>BI: generate per-package commands
deactivate BI
BI->>BI: prepare batch [dry-run or execute]
alt execute=true
BI->>IC: execute_installations() [parallel]
activate IC
IC->>IC: install shared dependencies
IC->>IC: install packages in parallel
IC-->>BI: results & statuses
deactivate IC
else dry_run=true
BI->>BI: mark packages SKIPPED
end
BI->>BI: compute statistics
BI-->>CLI: BatchInstallationResult
deactivate BI
CLI->>CLI: record history & report status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/installation_history.py (1)
406-408: Inconsistent error handling breaks the "history is optional" pattern.
update_installationre-raises exceptions on line 408, while other methods likerecord_installationgracefully returnNone. This could cause unexpected crashes in batch installation flows when history DB becomes unavailable mid-operation.🔎 Proposed fix
except Exception as e: - logger.error(f"Failed to update installation: {e}") - raise + logger.warning(f"Failed to update installation (non-fatal): {e}") + self._db_available = False
🧹 Nitpick comments (4)
install.sh (2)
286-289: Consider sourcing.envinstead of duplicating the API key in shell rc.Writing the API key to both
~/.cortex/.envand the shell rc file creates duplication and potential sync issues. A more maintainable approach would be to source the.envfile from the shell rc.🔎 Proposed alternative
if ! grep -q "ANTHROPIC_API_KEY" "$HOME/.${SHELL_NAME}rc" 2>/dev/null; then echo "" >> "$HOME/.${SHELL_NAME}rc" echo "# Added by Cortex Linux installer" >> "$HOME/.${SHELL_NAME}rc" - echo "export ANTHROPIC_API_KEY=\"$API_KEY\"" >> "$HOME/.${SHELL_NAME}rc" + echo "[ -f \"\$HOME/.cortex/.env\" ] && export \$(grep -v '^#' \"\$HOME/.cortex/.env\" | xargs)" >> "$HOME/.${SHELL_NAME}rc" fiThis sources all variables from
.envand avoids duplicating secrets in multiple files.
148-159: Error output is suppressed, making failures hard to diagnose.Redirecting stderr to
/dev/nullon lines 149 and 156 hides pip errors. When installation fails, users won't see why. Consider showing errors on failure.🔎 Proposed fix
# Try to install from PyPI first - if "$PIP_CMD" install --quiet cortex-linux 2>/dev/null; then + if "$PIP_CMD" install --quiet cortex-linux; then print_success "cortex-linux installed successfully from PyPI" return 0 fi # Fallback: Try installing from GitHub print_info "PyPI package not found, installing from GitHub..." - if "$PIP_CMD" install --quiet "git+https://github.com/cortexlinux/cortex.git" 2>/dev/null; then + if "$PIP_CMD" install --quiet "git+https://github.com/cortexlinux/cortex.git"; then print_success "cortex-linux installed successfully from GitHub" return 0 fiThe
--quietflag already suppresses normal output; removing2>/dev/nulllets errors through.tests/test_batch_installer.py (1)
43-73: Tests cover core happy paths well.The
test_analyze_packagesand related tests properly mockDependencyResolverand verify the expected status transitions. Consider adding a test for whenresolve_dependenciesraises an exception to verify error handling.def test_analyze_packages_with_failure(self, mock_resolver_class): """Test package analysis when dependency resolution fails""" mock_resolver = Mock() mock_resolver_class.return_value = mock_resolver mock_resolver.resolve_dependencies.side_effect = Exception("Network error") installer = BatchInstaller() packages = installer.analyze_packages(["nginx"]) self.assertEqual(packages["nginx"].status, PackageStatus.FAILED) self.assertIn("Network error", packages["nginx"].error_message)cortex/batch_installer.py (1)
296-304: Dry-run duplicates shared dependency commands across all packages.In dry-run mode,
_shared_depscommands are added to every package's command list (lines 301-303). This could confuse users as each package appears to independently install shared dependencies. Consider showing shared deps separately in the output.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cortex/batch_installer.py(1 hunks)cortex/cli.py(7 hunks)cortex/installation_history.py(6 hunks)install.sh(1 hunks)tests/test_batch_installer.py(1 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:
cortex/cli.pytests/test_batch_installer.pycortex/installation_history.pycortex/batch_installer.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_batch_installer.py
**/*install*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*install*.py: Dry-run by default for all installations in command execution
No silent sudo execution - require explicit user confirmation
Implement audit logging to ~/.cortex/history.db for all package operations
Files:
tests/test_batch_installer.pycortex/installation_history.pycortex/batch_installer.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 : Implement audit logging to ~/.cortex/history.db for all package operations
Applied to files:
cortex/cli.pycortex/installation_history.pycortex/batch_installer.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
🧬 Code graph analysis (2)
tests/test_batch_installer.py (1)
cortex/dependency_resolver.py (4)
DependencyGraph(29-36)Dependency(18-25)resolve_dependencies(202-260)is_package_installed(95-97)
cortex/batch_installer.py (2)
cortex/dependency_resolver.py (4)
DependencyResolver(39-377)DependencyGraph(29-36)resolve_dependencies(202-260)is_package_installed(95-97)cortex/coordinator.py (1)
InstallationCoordinator(65-338)
🔇 Additional comments (13)
cortex/installation_history.py (1)
76-83: Resilient DB initialization looks good.The try/except wrapper around DB initialization with the
_db_availableflag ensures history functionality is optional and won't break core installation flows. Based on learnings, this aligns with the audit logging requirement while gracefully handling environments where~/.cortex/history.dbisn't writable.install.sh (3)
6-6: Good use of strict mode.
set -euo pipefailensures the script fails fast on errors, unset variables, and pipe failures. This is essential for a curl | bash installer where silent failures could leave the system in an inconsistent state.
114-117: Automatic venv removal in non-interactive mode is appropriate for CI.The warning message is printed before removal, which is the right balance for CI/CD environments where clean installations are expected.
358-372: Clean sequential installation flow.The main function orchestrates all steps in a logical order. The
set -eensures early exit on failures. This aligns with the PR objectives for a zero-friction installer.tests/test_batch_installer.py (2)
1-19: Good test structure and imports.The test file follows Python testing conventions with unittest, proper mocking, and clear test class organization. The
sys.pathmanipulation (line 11) ensures imports work from the tests directory.
254-258: Rollback test correctly verifies subprocess execution.The test patches
subprocess.runand verifies it's called when rollback is enabled. This ensures the rollback commands are actually executed.cortex/cli.py (3)
185-193: Good routing logic for single vs batch installation.The install method cleanly splits input and routes to
_install_batchfor multiple packages while preserving the existing single-package flow. This maintains backward compatibility.
751-754: Default behavior correctly requires explicit --execute flag.Per coding guidelines, installations are dry-run by default. The CLI requires the
--executeflag to actually run commands, which aligns with the "no silent execution" principle. Based on learnings from AGENTS.md.
346-362: Progress callback covers key statuses with clear emoji indicators.The callback handles the main states (SUCCESS, FAILED, INSTALLING, SKIPPED) with distinct emojis. Transitional states fall through to
"⏳"which is appropriate.cortex/batch_installer.py (4)
38-56: PackageInstallation dataclass well-structured with helper method.The dataclass cleanly captures installation state with appropriate defaults. The
duration()helper is a good pattern for computed properties.
423-453: Parallel execution with ThreadPoolExecutor is well-implemented.The parallel execution correctly uses
as_completedfor progress tracking and handles the shared dependencies flag to avoid duplicate installations. Theaptpackage manager's built-in locking will prevent race conditions at the system level.
224-254: Simplified topological sort is adequate for current use case.The implementation places all dependencies before packages. Since
apt-gethandles transitive dependency ordering internally, this approach is sufficient. The docstring accurately describes the behavior.
79-100: BatchInstaller class is well-documented with proper docstrings and type hints.All public methods include comprehensive docstrings with Args and Returns sections. Type hints are present throughout, meeting coding guidelines requirements.
cortex/batch_installer.py
Outdated
| for cmd in reversed(pkg.rollback_commands): | ||
| logger.info(f"Rolling back {pkg.name}: {cmd}") | ||
| # Execute rollback command | ||
| import subprocess | ||
| subprocess.run(cmd, shell=True, capture_output=True, timeout=60) |
There was a problem hiding this comment.
Security: shell=True enables shell injection; import should be at module level.
Using shell=True with rollback commands is a security risk. Additionally, importing subprocess inside the loop is inefficient.
🔎 Proposed fix
Add import at module level:
import subprocessThen refactor the rollback execution:
for cmd in reversed(pkg.rollback_commands):
logger.info(f"Rolling back {pkg.name}: {cmd}")
# Execute rollback command
- import subprocess
- subprocess.run(cmd, shell=True, capture_output=True, timeout=60)
+ subprocess.run(cmd.split(), capture_output=True, timeout=60, check=False)Note: If commands contain quoted arguments, use shlex.split(cmd) instead of cmd.split().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for cmd in reversed(pkg.rollback_commands): | |
| logger.info(f"Rolling back {pkg.name}: {cmd}") | |
| # Execute rollback command | |
| import subprocess | |
| subprocess.run(cmd, shell=True, capture_output=True, timeout=60) | |
| for cmd in reversed(pkg.rollback_commands): | |
| logger.info(f"Rolling back {pkg.name}: {cmd}") | |
| # Execute rollback command | |
| subprocess.run(cmd.split(), capture_output=True, timeout=60, check=False) |
🤖 Prompt for AI Agents
In cortex/batch_installer.py around lines 476 to 480, the code imports
subprocess inside the loop and calls subprocess.run(..., shell=True) which is a
security risk and inefficient; move "import subprocess" to module level, remove
shell=True and pass a list of argv instead (use shlex.split(cmd) if
pkg.rollback_commands are strings with quoted args, or ensure commands are
already lists), call subprocess.run(args_list, capture_output=True, timeout=60)
and catch subprocess.SubprocessError (or TimeoutExpired) to log stdout/stderr
and the error; ensure you preserve the logger.info line and log failures with
logger.error including command output for debugging.
cortex/cli.py
Outdated
| install_id = history.record_installation( | ||
| InstallationType.INSTALL, | ||
| package_names, | ||
| all_commands, | ||
| datetime.now(), | ||
| ) |
There was a problem hiding this comment.
History records incorrect start time for batch installations.
datetime.now() on line 417 captures the current time (after execution), not the actual installation start time. This differs from the single-package flow which captures start_time = datetime.now() before installation begins.
🔎 Proposed fix
def _install_batch(
self, package_names: list[str], execute: bool = False, dry_run: bool = False
) -> int:
+ start_time = datetime.now() # Capture before installation
self._print_status("🧠", f"Analyzing {len(package_names)} packages...")
# ... rest of method ...
if execute or dry_run:
try:
history = InstallationHistory()
# ...
install_id = history.record_installation(
InstallationType.INSTALL,
package_names,
all_commands,
- datetime.now(),
+ start_time,
)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cortex/cli.py around lines 413 to 418, the batch-install branch records
installation start time using datetime.now() at the point of logging (after
installs complete) instead of the actual start time; capture start_time =
datetime.now() before the batch installation begins (same spot pattern as
single-package flow) and pass that start_time into history.record_installation
instead of calling datetime.now() inline so the recorded start time reflects
when the installation started.
cortex/installation_history.py
Outdated
| except sqlite3.OperationalError as e: | ||
| # Database might be locked or corrupted | ||
| logger.warning(f"Database operational error (non-fatal): {e}") | ||
| # Don't raise - allow operations to continue without history |
There was a problem hiding this comment.
OperationalError handler doesn't mark DB as unavailable.
When sqlite3.OperationalError is caught (e.g., locked or corrupted DB), _db_available is not set to False, but the method returns early in the generic except block. This inconsistency could lead to subsequent operations attempting DB access and failing repeatedly.
🔎 Proposed fix
except sqlite3.OperationalError as e:
# Database might be locked or corrupted
logger.warning(f"Database operational error (non-fatal): {e}")
- # Don't raise - allow operations to continue without history
+ self._db_available = False
+ return
except Exception as e:🤖 Prompt for AI Agents
In cortex/installation_history.py around lines 158 to 161, the
sqlite3.OperationalError handler logs the error but does not mark the database
as unavailable; update the except block to set self._db_available = False (and
optionally clear/close any active connection handles if applicable) before
returning so subsequent operations know the DB is not usable and will avoid
further DB access attempts.
| if [ -L "$CORTEX_SYMLINK" ] || [ -f "$CORTEX_SYMLINK" ]; then | ||
| rm -f "$CORTEX_SYMLINK" | ||
| fi | ||
|
|
||
| ln -s "$CORTEX_BIN/cortex" "$CORTEX_SYMLINK" | ||
| print_success "Created symlink: $CORTEX_SYMLINK" |
There was a problem hiding this comment.
Symlink created without verifying target binary exists.
If the pip installation silently fails or the binary name differs, this creates a broken symlink. Add a verification step.
🔎 Proposed fix
# Create symlink
if [ -L "$CORTEX_SYMLINK" ] || [ -f "$CORTEX_SYMLINK" ]; then
rm -f "$CORTEX_SYMLINK"
fi
+ if [ ! -f "$CORTEX_BIN/cortex" ]; then
+ print_error "Cortex binary not found at $CORTEX_BIN/cortex"
+ exit 1
+ fi
+
ln -s "$CORTEX_BIN/cortex" "$CORTEX_SYMLINK"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -L "$CORTEX_SYMLINK" ] || [ -f "$CORTEX_SYMLINK" ]; then | |
| rm -f "$CORTEX_SYMLINK" | |
| fi | |
| ln -s "$CORTEX_BIN/cortex" "$CORTEX_SYMLINK" | |
| print_success "Created symlink: $CORTEX_SYMLINK" | |
| if [ -L "$CORTEX_SYMLINK" ] || [ -f "$CORTEX_SYMLINK" ]; then | |
| rm -f "$CORTEX_SYMLINK" | |
| fi | |
| if [ ! -f "$CORTEX_BIN/cortex" ]; then | |
| print_error "Cortex binary not found at $CORTEX_BIN/cortex" | |
| exit 1 | |
| fi | |
| ln -s "$CORTEX_BIN/cortex" "$CORTEX_SYMLINK" | |
| print_success "Created symlink: $CORTEX_SYMLINK" |
🤖 Prompt for AI Agents
In install.sh around lines 185 to 190, the script creates the CORTEX_SYMLINK
without verifying that the target binary "$CORTEX_BIN/cortex" actually exists or
is executable; update the logic to first check that "$CORTEX_BIN/cortex" exists
and is executable (e.g. [ -x "$CORTEX_BIN/cortex" ]) and if not, print an error
and exit non-zero, only proceeding to remove any existing symlink and create the
new symlink when the target check passes; ensure the failure path does not leave
a broken symlink and returns a non-zero status for CI/automation to catch.
| # Save to .env file | ||
| mkdir -p "$HOME/.cortex" | ||
| echo "ANTHROPIC_API_KEY=$API_KEY" >> "$HOME/.cortex/.env" | ||
| export ANTHROPIC_API_KEY="$API_KEY" | ||
|
|
||
| # Also add to shell rc for persistence | ||
| SHELL_NAME=$(basename "$SHELL") | ||
| if [ -f "$HOME/.${SHELL_NAME}rc" ]; then | ||
| if ! grep -q "ANTHROPIC_API_KEY" "$HOME/.${SHELL_NAME}rc" 2>/dev/null; then | ||
| echo "" >> "$HOME/.${SHELL_NAME}rc" | ||
| echo "# Added by Cortex Linux installer" >> "$HOME/.${SHELL_NAME}rc" | ||
| echo "export ANTHROPIC_API_KEY=\"$API_KEY\"" >> "$HOME/.${SHELL_NAME}rc" | ||
| fi | ||
| fi | ||
|
|
||
| print_success "API key saved to ~/.cortex/.env and ~/.${SHELL_NAME}rc" | ||
| fi |
There was a problem hiding this comment.
API key may be duplicated in .env file on repeated runs.
Line 279 appends the API key to ~/.cortex/.env without checking if it already exists, unlike the shell rc handling on lines 285-289. Repeated installer runs could create duplicate entries.
🔎 Proposed fix
if [ -n "$API_KEY" ]; then
# Save to .env file
mkdir -p "$HOME/.cortex"
+ # Remove existing key if present, then add new one
+ if [ -f "$HOME/.cortex/.env" ]; then
+ sed -i '/^ANTHROPIC_API_KEY=/d' "$HOME/.cortex/.env"
+ fi
- echo "ANTHROPIC_API_KEY=$API_KEY" >> "$HOME/.cortex/.env"
+ echo "ANTHROPIC_API_KEY=$API_KEY" >> "$HOME/.cortex/.env"
export ANTHROPIC_API_KEY="$API_KEY"Apply the same pattern to the OPENAI_API_KEY block (lines 298-315).
🤖 Prompt for AI Agents
In install.sh around lines 277 to 293, the installer unconditionally appends
ANTHROPIC_API_KEY to ~/.cortex/.env which can create duplicate entries on
repeated runs; change the logic to check ~/.cortex/.env for an existing
ANTHROPIC_API_KEY (e.g., use grep -q) and if present either replace the existing
line (e.g., with sed) or skip appending, otherwise append the key; keep the
export ANTHROPIC_API_KEY="$API_KEY" behavior and mirror this same
non-duplicating pattern for the OPENAI_API_KEY block as noted in the review.
- Introduced install.sh for a streamlined installation process of Cortex Linux. - The script includes OS detection, Python version checks, virtual environment setup, and installation of the cortex-linux package from PyPI or GitHub. - Added functionality for API key configuration and PATH adjustments for user convenience. - Enhanced error handling and user feedback throughout the installation process.
|
|
Closing as duplicate - #315 was merged for the one-liner installer. Thanks for contributing! |
2 participants
Closes #250
Features:
Usage:
curl -fsSL https://cortexlinux.com/install.sh | bashExample Output: 🧠 Cortex Linux Installer
ℹ️ Detected: Ubuntu 24.04
ℹ️ Python version: 3.10.12
ℹ️ Installing to ~/.cortex...
✅ Virtual environment created
✅ cortex-linux installed successfully
✅ Created symlink: ~/.local/bin/cortex
✅ Installed! Run: cortex install nginx
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.