Skip to content

Conversation

@lu11y0
Copy link
Contributor

@lu11y0 lu11y0 commented Dec 12, 2025

Related Issue

Closes #252

Summary

  • Added stacks.json with mentioned stacks:
    • cortex stack ml / cortex stack ml-cpu – installs the ML stack; ml automatically falls back to ml-cpu on systems without a detected NVIDIA GPU
    • cortex stack webdev – installs the web development stack
    • cortex stack devops – installs the DevOps tooling stack
    • cortex stack data – installs the data analysis stack
  • Implemented StackManager to load, describe, and resolve hardware-aware stacks.
  • Added cortex stack CLI subcommands:
    • cortex stack --list - list all stacks
    • cortex stack --describe <name> - show stack details
    • cortex stack <name> --dry-run - preview packages that would be installed
    • cortex stack <name> - install the stack via the existing install flow

Demo

Checklist

  • Tests pass (pytest)
  • PR title format: [#252] Pre-built Smart Stacks
  • MVP label added

Summary by CodeRabbit

  • New Features

    • Adds a curated registry of pre-built package stacks plus a CLI subcommand to list, describe, preview (dry-run), and install stacks; integrated into CLI help.
  • Hardware-aware behavior

    • Automatic GPU/CPU-aware stack suggestion with fallback to a CPU-optimized stack when no GPU is detected.
  • Tests

    • Tests verifying hardware-aware stack suggestion behavior.
  • Documentation

    • User guide describing Smart Stacks, usage, and examples.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 12, 2025

Walkthrough

Adds Smart Stacks: a new StackManager loading cortex/stacks.json (hardware-aware), a cortex stack CLI command with list/describe/install/dry-run handlers, tests for GPU-aware stack suggestion, and documentation describing the feature.

Changes

Cohort / File(s) Summary
Stack Manager Core
cortex/stack_manager.py
New StackManager class: loads and caches stacks.json, lists and finds stacks, returns package lists, suggests hardware-aware variants using has_nvidia_gpu, and formats stack descriptions with error handling.
CLI Integration
cortex/cli.py
Adds stack(self, args) and private handlers: _handle_stack_list, _handle_stack_describe, _handle_stack_install, _handle_stack_dry_run, _handle_stack_real_install; parses --list, --describe, --dry-run, routes stack installs into existing install flow, and updates rich help.
Stack Registry
cortex/stacks.json
New JSON registry defining stacks (ml, ml-cpu, webdev, devops, data) with ids, names, descriptions, package lists, and hardware target fields.
Tests
test/test_smart_stacks.py, tests/test_smart_stacks.py
New tests asserting StackManager.suggest_stack("ml") returns "ml-cpu" when has_nvidia_gpu is False and "ml" when True (uses monkeypatch to simulate GPU presence).
Docs
docs/smart_stack.md
New documentation describing Smart Stacks usage, available stacks, GPU-detection fallback, involved files, tests, and a demo reference.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant U as User/CLI
    participant C as CortexCLI
    participant S as StackManager
    participant H as HardwareDetector
    participant P as PackageInstaller

    U->>C: cortex stack ml [--list|--describe|--dry-run|install]
    C->>S: suggest_stack("ml")
    S->>H: has_nvidia_gpu()
    alt GPU present
        H-->>S: true
        S-->>C: "ml"
    else no GPU
        H-->>S: false
        S-->>C: "ml-cpu"
    end
    C->>S: get_stack_packages(<suggested>)
    S-->>C: [packages]
    alt --list / --describe
        C-->>U: formatted list or description
    else --dry-run
        C-->>U: preview package list (no execute)
    else install
        C->>P: install(packages)
        P-->>C: progress/results per package
        C-->>U: final status
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Review JSON loading/caching and error handling in cortex/stack_manager.py.
  • Verify suggest_stack logic and tests that monkeypatch has_nvidia_gpu.
  • Inspect CLI parsing, argument validation, and install routing in cortex/cli.py.
  • Validate cortex/stacks.json contents match docs and tests.

"I hop and nibble through the code so bright,
I find the stacks and set them right.
GPU or none, I choose the pair,
Packages queued with tender care.
🐇📦"

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 73.68% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly summarizes the main feature: Smart Stacks for multiple domains (ML, Web, DevOps) and references the issue being closed.
Description check ✅ Passed Description covers the Related Issue, provides a detailed Summary of changes and features, includes a demo link, and completes most checklist items.
Linked Issues check ✅ Passed All coding requirements from issue #252 are met: stacks.json with packages defined, StackManager implementation, CLI commands (list, describe, install, dry-run), and hardware-aware ml/ml-cpu variants.
Out of Scope Changes check ✅ Passed All changes are in-scope: StackManager, CLI integration, stacks.json data file, tests, and documentation directly address issue #252 objectives.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (3)
cortex/stacks.json (1)

10-16: Inconsistency: ml-cpu missing matplotlib.

The GPU variant includes matplotlib but the CPU variant does not. Consider adding it for consistency unless there's a specific reason to omit it.

     {
       "id": "ml-cpu",
       "name": "Machine Learning (CPU)",
       "description": "PyTorch CPU-only version",
-      "packages": ["pytorch-cpu", "jupyter", "numpy", "pandas"],
+      "packages": ["pytorch-cpu", "jupyter", "numpy", "pandas", "matplotlib"],
       "hardware": "cpu"
     },
cortex/stack_manager.py (2)

45-58: Consider more specific type hints.

Both methods work correctly. Optional improvement: use Dict[str, Any] instead of bare Dict for more precise type hints.

-    def find_stack(self, stack_id: str) -> Optional[Dict]:
+    def find_stack(self, stack_id: str) -> Optional[Dict[str, Any]]:
         """Find a stack by ID"""

Add the import at the top:

-from typing import Dict, List, Optional
+from typing import Any, Dict, List, Optional

60-72: Consider making hardware suggestions more extensible.

The current hardcoded approach works for the "ml" stack but isn't easily extensible to other hardware-aware stacks in the future. Consider using the "hardware" field from stacks.json for more data-driven logic.

Example of a more extensible approach:

def suggest_stack(self, base_stack: str) -> str:
    """
    Suggest appropriate variant based on hardware.
    E.g., if user asks for 'ml' but has no GPU, suggest 'ml-cpu'
    """
    stack = self.find_stack(base_stack)
    if not stack:
        return base_stack
    
    # If stack requires GPU but none detected, look for CPU variant
    if stack.get("hardware") == "gpu" and not has_nvidia_gpu():
        cpu_variant = f"{base_stack}-cpu"
        if self.find_stack(cpu_variant):
            return cpu_variant
    
    return base_stack

This approach would automatically handle any future GPU/CPU stack pairs without hardcoding.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5a1755c and fed0072.

📒 Files selected for processing (4)
  • cortex/cli.py (5 hunks)
  • cortex/stack_manager.py (1 hunks)
  • cortex/stacks.json (1 hunks)
  • test/test_smart_stacks.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:

  • test/test_smart_stacks.py
  • cortex/stack_manager.py
  • cortex/cli.py
🧬 Code graph analysis (3)
test/test_smart_stacks.py (1)
cortex/stack_manager.py (2)
  • StackManager (18-93)
  • suggest_stack (60-72)
cortex/stack_manager.py (2)
cortex/hardware_detection.py (1)
  • has_nvidia_gpu (668-670)
cortex/cli.py (1)
  • stack (180-252)
cortex/cli.py (1)
cortex/stack_manager.py (5)
  • StackManager (18-93)
  • list_stacks (40-43)
  • describe_stack (74-93)
  • suggest_stack (60-72)
  • find_stack (45-51)
🔇 Additional comments (10)
test/test_smart_stacks.py (1)

1-2: LGTM!

The dual import pattern is correct: line 1 imports the class for instantiation, while line 2 imports the module to enable monkeypatching has_nvidia_gpu at module scope.

cortex/cli.py (2)

43-43: LGTM!

The import is correctly placed and follows the existing pattern.


696-701: LGTM!

The argument parser configuration is correct: optional positional name allows --list and --describe to work without specifying a stack, and all flags are properly defined.

cortex/stack_manager.py (4)

1-8: LGTM!

Excellent module-level documentation with clear usage examples.


10-15: LGTM!

Imports are clean and all are used within the module.


40-43: LGTM!

Clean implementation with defensive programming using .get() with a default value.


74-93: LGTM with optional style improvement.

The method correctly formats stack descriptions with good error handling. The string concatenation approach works fine for this use case.

Optional: For slightly better performance with many packages, consider using a list and '\n'.join():

def describe_stack(self, stack_id: str) -> str:
    """Get formatted stack description"""
    stack = self.find_stack(stack_id)
    if not stack:
        return f"Stack '{stack_id}' not found"
    
    lines = [
        f"\n📦 Stack: {stack['name']}",
        f"Description: {stack['description']}\n",
        "Packages included:"
    ]
    
    for idx, pkg in enumerate(stack.get("packages", []), 1):
        lines.append(f"  {idx}. {pkg}")
    
    tags = stack.get("tags", [])
    if tags:
        lines.append(f"\nTags:  {', '.join(tags)}")
    
    hardware = stack.get("hardware", "any")
    lines.append(f"Hardware: {hardware}")
    
    return '\n'.join(lines) + '\n'
cortex/stacks.json (3)

24-37: Package names in stacks are handled by AI interpretation, not used directly.

The stack installation flow (lines 233-245 of cli.py) passes each package through self.install(), which uses CommandInterpreter to parse the package name as natural language and generate appropriate commands via Claude/OpenAI. This means:

  • "docker" → AI translates to "docker-ce", "docker.io", or distro-appropriate variant
  • "pandas", "sqlalchemy" → AI knows these are Python libraries and generates pip/apt commands accordingly
  • The system is designed to handle distro-specific names and package type variations

Your concerns about distro-specific names and mixed package types are already addressed by the architecture.


3-9: Verify CommandInterpreter correctly handles "pytorch" and "cuda" package name interpretation.

The stack installation flow relies on an LLM-based CommandInterpreter to convert semantic package names ("pytorch", "cuda") into appropriate bash install commands. While this approach is flexible, there's risk that the LLM may generate incorrect commands:

  • "pytorch": LLM might generate apt install pytorch (incorrect) instead of pip install torch
  • "cuda": LLM might generate apt install cuda (incorrect) instead of the actual package like nvidia-cuda-toolkit

The system prompt emphasizes apt-based installation but lacks concrete examples for Python packages. Consider:

  1. Adding explicit examples to the system prompt showing how to handle Python packages vs system packages
  2. Testing that actual LLM responses for "install pytorch" and "install cuda" produce correct commands
  3. Alternatively, replace semantic package names with specific package manager identifiers (e.g., "torch" instead of "pytorch")

17-23: No changes needed. Both package listings are correct for Ubuntu 22.04+ / Debian 12+:

  • npm: Not bundled with nodejs in official Debian/Ubuntu repositories; must be listed separately as a distinct package.
  • postgresql: Correct metapackage name that resolves to the appropriate version (postgresql-15 on Debian 12, postgresql-14 on Ubuntu 22.04).

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
cortex/cli.py (1)

179-252: Add type hints + docstring, and handle missing/invalid stacks.json gracefully.

This is a public CLI API and currently violates the project guideline (“Type hints required” + “Docstrings required for all public APIs”). Also, StackManager.load_stacks() can raise FileNotFoundError / ValueError; catching these here would avoid the generic top-level “Unexpected error”.

-    #Handle 'cortex stack' commands using StackManager
-    def stack(self, args):
+    # Handle 'cortex stack' commands using StackManager
+    def stack(self, args: argparse.Namespace) -> int:
+        """Handle `cortex stack` commands (list/describe/install/dry-run)."""
         manager = StackManager()
+
+        try:
+            # List stacks (default when no name/describe)
+            if args.list or (not args.name and not args.describe):
+                stacks = manager.list_stacks()
+                ...
+                return 0
+        except (FileNotFoundError, ValueError) as e:
+            self._print_error(str(e))
+            return 1

Per-package install triggers repeated planning overhead; consider batching/caching.

🧹 Nitpick comments (1)
cortex/cli.py (1)

696-702: Prefer mutually-exclusive args for stack (--list vs --describe vs install).

Consider an argparse mutually exclusive group so users can’t pass conflicting combinations (e.g., cortex stack ml --describe webdev), and the handler logic can be simplified.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fed0072 and 405ebfe.

📒 Files selected for processing (2)
  • cortex/cli.py (5 hunks)
  • cortex/stack_manager.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • cortex/stack_manager.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/stack_manager.py (5)
  • StackManager (18-93)
  • list_stacks (40-43)
  • describe_stack (74-93)
  • suggest_stack (60-72)
  • find_stack (45-51)
🔇 Additional comments (3)
cortex/cli.py (3)

43-43: Import is fine; consider catching StackManager load errors at call sites.


731-734: Dispatch wiring for stack looks correct.


745-746: sys.exit(main()) entrypoint is standard and fine.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 405ebfe and c821864.

📒 Files selected for processing (1)
  • docs/smart_stack.md (1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/smart_stack.md

79-79: Bare URL used

(MD034, no-bare-urls)

🔇 Additional comments (1)
docs/smart_stack.md (1)

1-85: Documentation quality is solid and aligns with PR objectives.

The documentation accurately describes the Smart Stacks feature, stack definitions, usage examples, and integration points. All five stacks (ml, ml-cpu, webdev, devops, data) and their package lists match the PR requirements. The "How It Works" section correctly describes the hardware-aware fallback mechanism (GPU detection → ml-cpu) and the dry-run capability. File references (stacks.json, stack_manager.py, cli.py, test_smart_stacks.py) align with the implementation scope.

@lu11y0 lu11y0 marked this pull request as draft December 12, 2025 16:31
@lu11y0 lu11y0 marked this pull request as ready for review December 12, 2025 17:08
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (2)
cortex/cli.py (2)

245-262: Per-package self.install(...) likely triggers multiple LLM calls (cost/latency)
This repeats the earlier concern: each loop iteration calls install() which instantiates CommandInterpreter and parses separately. Consider batching into a single plan (one LLM call) for the whole stack, or adding an install_many(packages=[...]) path.


741-744: Fix inconsistent indentation in main() dispatch (regressed)
This was previously flagged and appears reintroduced; it’s not PEP8 and may fail stricter linters.

-        elif args.command == 'stack':
-                    return cli.stack(args)
+        elif args.command == 'stack':
+            return cli.stack(args)
🧹 Nitpick comments (4)
cortex/cli.py (4)

6-6: Tighten typing + remove trailing whitespace in StackManager import
Dict without key/value types bleeds Any implicitly; also Line 43 has trailing whitespace (often trips linters).

-from typing import List, Optional, Dict
+from typing import Any, Dict, List, Optional
@@
-from cortex.stack_manager import StackManager 
+from cortex.stack_manager import StackManager

Also applies to: 43-43


195-206: Guard against malformed stacks.json entries to avoid KeyError
stack['id'], stack['name'], stack['description'] will crash if the config is missing fields. Since this is CLI-facing, consider defensive .get(...) with a fallback.


236-244: Type stack more precisely than Dict
stack: Dict loses all structure. Prefer Mapping[str, Any] or a TypedDict for id/name/description/packages/tags/hardware.


706-712: Make stack CLI flags mutually exclusive (or validate combinations)
As-is, users can pass conflicting combinations like cortex stack ml --list. Consider an argparse mutually-exclusive group for --list vs --describe vs “install by name”, or validate in stack().

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c821864 and 6f05370.

📒 Files selected for processing (1)
  • cortex/cli.py (6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/stack_manager.py (5)
  • StackManager (18-93)
  • list_stacks (40-43)
  • describe_stack (74-93)
  • suggest_stack (60-72)
  • find_stack (45-51)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/cli.py

[warning] 189-189: Fix this condition that always evaluates to true.

See more on https://sonarcloud.io/project/issues?id=cortexlinux_cortex&issues=AZsTiQxJ_6Ev2GiM_URJ&open=AZsTiQxJ_6Ev2GiM_URJ&pullRequest=296

🔇 Additional comments (1)
cortex/cli.py (1)

755-756: sys.exit(main()) wiring looks good
No issues.

@lu11y0 lu11y0 marked this pull request as draft December 12, 2025 17:37
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (3)
cortex/cli.py (3)

254-254: Type hint requires missing import.

Dict[str, Any] requires Any to be imported from typing. See comment on line 6 for the fix.


265-265: Type hint requires missing import.

Dict[str, Any] requires Any to be imported from typing. See comment on line 6 for the fix.


270-278: LLM overhead: per-package installation triggers multiple API calls.

The loop at lines 270-278 calls self.install(pkg) for each package individually. Each call instantiates a new CommandInterpreter (line 310 in the install method), which invokes parse() and makes a separate LLM API call. For a 6-package stack, this results in 6 LLM API calls, significantly increasing cost and latency.

This issue was flagged in previous reviews and remains unaddressed. Consider:

  • Batching: Pass all packages to a single install() call or create a batch installation method
  • Pre-generation: For known stack packages, pre-generate and cache commands to avoid LLM calls entirely
  • Coordinator enhancement: Extend InstallationCoordinator to handle package lists directly

Based on past review feedback, this architectural issue should be addressed to improve user experience and reduce API costs.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6f05370 and a602b9f.

📒 Files selected for processing (1)
  • cortex/cli.py (6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/stack_manager.py (5)
  • StackManager (18-93)
  • list_stacks (40-43)
  • find_stack (45-51)
  • describe_stack (74-93)
  • suggest_stack (60-72)
cortex/branding.py (1)
  • cx_print (55-75)
🔇 Additional comments (6)
cortex/cli.py (6)

179-197: Good implementation of stack command routing.

The method correctly validates arguments, handles the list/describe/install flows, and delegates to focused helper methods. Type hints and docstring are present per coding guidelines.


200-211: Clean implementation of stack listing.

Proper type hints, docstring, and user-friendly output with package counts.


214-222: Proper error handling for missing stacks.

Correctly returns non-zero exit code when stack is not found, addressing previous review feedback.


225-251: Well-structured stack installation handler.

Hardware-aware suggestion logic provides good UX, and empty package validation prevents silent failures. Addresses previous review feedback.


728-733: Well-structured argparse configuration for stack command.

Clear help text and appropriate argument structure for all stack subcommand options (list, describe, install, dry-run).


764-765: Proper command dispatch for stack subcommand.

Correctly routes stack commands to the handler with proper indentation and argument passing.

@lu11y0 lu11y0 marked this pull request as ready for review December 12, 2025 17:42
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (1)
cortex/cli.py (1)

265-283: Per-package self.install(...) will trigger multiple LLM plans; consider batching/caching.
This repeats the earlier concern: each package likely incurs a separate CommandInterpreter.parse() call, which is slow/expensive for multi-package stacks.

🧹 Nitpick comments (3)
cortex/cli.py (3)

200-212: List output looks good; consider a clear message for “no stacks configured”.
If stacks.json exists but contains zero stacks, the command currently prints a header then nothing—maybe print “No stacks available” for clarity.


225-252: Make _handle_stack_install defensively reject missing args.name.
Today it’s logically unreachable (because stack() routes earlier), but it prevents accidental misuse if _handle_stack_install() is called from elsewhere and also avoids “Optional[str] passed where str expected” issues.

 def _handle_stack_install(self, manager: StackManager, args: argparse.Namespace) -> int:
     """Install a stack with optional hardware-aware selection."""
+    if not args.name:
+        self._print_error("No stack name provided. Use --list to see available stacks.")
+        return 1
     original_name = args.name
     suggested_name = manager.suggest_stack(args.name)

728-734: Argparse wiring looks correct; consider mutual-exclusion + add stack to show_rich_help().
Right now users can provide name + --list together; a mutually-exclusive group for --list/--describe reduces ambiguous invocations. Also, the Rich help table currently doesn’t advertise the new stack command.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a602b9f and 089d4d1.

📒 Files selected for processing (1)
  • cortex/cli.py (6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/cli.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*.py : Type hints required in Python code

Applied to files:

  • cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/stack_manager.py (5)
  • StackManager (18-93)
  • list_stacks (40-43)
  • find_stack (45-51)
  • describe_stack (74-93)
  • suggest_stack (60-72)
cortex/branding.py (1)
  • cx_print (55-75)
🔇 Additional comments (4)
cortex/cli.py (4)

6-6: Good: Any is now imported, fixing runtime/type-hint resolution.
This addresses the earlier Dict[str, Any] annotation issue cleanly. As per coding guidelines, type hints must be valid.


214-223: Good: missing stack returns non-zero and messaging is consistent.
This is scripting-friendly and matches other CLI error patterns.


254-263: Dry-run UX is clear and scoped (no side effects).
Output is concise and matches the CLI intent.


778-778: Entry-point exit wiring is correct.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
cortex/cli.py (2)

43-43: Verify trailing whitespace removal on import line.

A past review flagged trailing whitespace on this import. Ensure no trailing space exists after StackManager.

#!/bin/bash
# Check for trailing whitespace on line 43
sed -n '43p' cortex/cli.py | cat -A | grep -E '\s+\$$' && echo "Trailing whitespace found" || echo "No trailing whitespace"

274-291: Note: Per-package LLM overhead remains.

Each self.install(pkg) call instantiates a new CommandInterpreter and makes a separate LLM API call. For a 6-package stack, this results in 6 API calls with associated latency and cost. This was flagged in a previous review. Consider batching packages or pre-generating commands for known stack packages in a future iteration.

🧹 Nitpick comments (4)
cortex/cli.py (4)

199-207: Include original error details in exception messages.

The exception handlers provide generic messages but lose the original error context. Consider including str(e) in the error message for better debugging.

         except FileNotFoundError:
-            self._print_error("stacks.json not found. Make sure it exists in the expected location.")
+            self._print_error("stacks.json not found. Ensure cortex/stacks.json exists.")
             return 1
-        except ValueError:
-            self._print_error("stacks.json is invalid or malformed.")
+        except ValueError as e:
+            self._print_error(f"stacks.json is invalid or malformed: {e}")
             return 1
-
-        
-

Also, the extra blank lines at 205-207 should be removed for PEP 8 compliance (two blank lines max between top-level definitions).


209-220: LGTM with minor defensive coding suggestion.

The method has proper type hints and docstring. Consider using .get() for defensive access to stack fields in case of malformed entries, though this may be better addressed by JSON schema validation in StackManager.

         for stack in stacks:
             pkg_count = len(stack.get("packages", []))
-            console.print(f"  [green]{stack['id']}[/green]")
-            console.print(f"    {stack['name']}")
-            console.print(f"    {stack['description']}")
+            console.print(f"  [green]{stack.get('id', 'unknown')}[/green]")
+            console.print(f"    {stack.get('name', 'Unnamed Stack')}")
+            console.print(f"    {stack.get('description', 'No description')}")
             console.print(f"    [dim]({pkg_count} packages)[/dim]\n")

737-742: Consider clarifying mutually exclusive arguments.

The parser allows both name and --describe STACK to be provided simultaneously (e.g., cortex stack ml --describe webdev). In stack(), --describe takes precedence over name, which may confuse users. Consider making them mutually exclusive or documenting the precedence clearly.

     # Stack command
     stack_parser = subparsers.add_parser('stack', help='Manage pre-built package stacks')
-    stack_parser.add_argument('name', nargs='?', help='Stack name (ml, ml-cpu, webdev, devops, data)')
-    stack_parser.add_argument('--list', '-l', action='store_true', help='List all available stacks')
-    stack_parser.add_argument('--describe', '-d', metavar='STACK', help='Show details about a stack')
-    stack_parser.add_argument('--dry-run', action='store_true', help='Show what would be installed')
+    stack_parser.add_argument('name', nargs='?', help='Stack name to install (ml, ml-cpu, webdev, devops, data)')
+    stack_group = stack_parser.add_mutually_exclusive_group()
+    stack_group.add_argument('--list', '-l', action='store_true', help='List all available stacks')
+    stack_group.add_argument('--describe', '-d', metavar='STACK', help='Show details about a stack')
+    stack_parser.add_argument('--dry-run', action='store_true', help='Show what would be installed (requires stack name)')

771-774: Clean up trailing whitespace.

Lines 771 and 774 appear to have trailing whitespace. Remove trailing spaces for PEP 8 compliance.

         elif args.command == 'notify':
-            return cli.notify(args)                
+            return cli.notify(args)
         elif args.command == 'stack':
             return cli.stack(args)
-        
+
         else:
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 089d4d1 and 1995118.

📒 Files selected for processing (1)
  • cortex/cli.py (6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/cli.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*.py : Type hints required in Python code

Applied to files:

  • cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (2)
cortex/stack_manager.py (5)
  • StackManager (18-93)
  • list_stacks (40-43)
  • find_stack (45-51)
  • describe_stack (74-93)
  • suggest_stack (60-72)
cortex/branding.py (1)
  • cx_print (55-75)
🔇 Additional comments (3)
cortex/cli.py (3)

223-231: LGTM!

Proper type hints, docstring, and error handling for missing stacks (returns 1 as requested in past review).


234-261: LGTM!

Clean implementation with hardware-aware stack suggestion, proper empty package validation (as requested in past review), and clear delegation to dry-run or real install handlers.


263-271: LGTM!

Proper type hints, docstring, and clear dry-run preview output.

…e, updated stack commands, and refined _handle_stack_list.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
cortex/cli.py (1)

274-292: Address per-package LLM overhead (unresolved from previous reviews).

Lines 279-281 call self.install() individually for each package, which creates a new CommandInterpreter instance per call (see line 319 in the install() method), triggering a separate LLM API invocation for each package. For a 6-package stack, this means 6 API calls, significantly increasing cost and latency.

Consider these solutions:

  1. Batch processing: Modify install() to accept a list of packages and make a single LLM call for the entire batch
  2. Command caching: For known stack packages, pre-generate or cache commands to avoid LLM calls
  3. Stack-aware interpreter: Add a StackManager.get_commands(stack_id) method that generates all commands in one LLM call

Example batch approach:

def _handle_stack_real_install(self, stack: Dict[str, Any], packages: List[str]) -> int:
    """Install all packages in the stack."""
    cx_print(f"\n🚀 Installing stack: {stack['name']}\n", "success")
    
    # Install all packages in a single batch to minimize LLM calls
    packages_str = " ".join(packages)
    result = self.install(software=packages_str, execute=True, dry_run=False)
    
    if result != 0:
        self._print_error(f"Failed to install stack '{stack['name']}'")
        return 1
    
    self._print_success(f"\n✅ Stack '{stack['name']}' installed successfully!")
    console.print(f"Installed {len(packages)} packages")
    return 0

Note: This requires verifying that CommandInterpreter.parse() can handle multiple packages in a single request.

🧹 Nitpick comments (1)
cortex/cli.py (1)

178-207: Remove excessive blank lines.

Lines 206-207 contain two consecutive blank lines after the exception handling block. PEP 8 recommends using blank lines sparingly within methods.

Apply this diff:

         except ValueError as e:
             self._print_error(f"stacks.json is invalid or malformed: {e}")
             return 1
-
-        
-
+

As per coding guidelines, follow PEP 8 style.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1995118 and 72b0a7e.

📒 Files selected for processing (1)
  • cortex/cli.py (6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/cli.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*.py : Type hints required in Python code

Applied to files:

  • cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/stack_manager.py (5)
  • StackManager (18-93)
  • list_stacks (40-43)
  • find_stack (45-51)
  • describe_stack (74-93)
  • suggest_stack (60-72)
🔇 Additional comments (6)
cortex/cli.py (6)

209-221: LGTM!

The list handler correctly retrieves and displays stack information with proper formatting. Type hints and docstring are present as required.


223-232: LGTM!

The describe handler properly validates stack existence and returns appropriate exit codes. Error handling and type annotations are correct.


234-261: LGTM!

The install handler implements hardware-aware stack selection and validates both stack existence and non-empty package lists. All coding guidelines followed.


263-272: LGTM!

The dry-run handler correctly previews packages without execution. Type hints and docstring present as required.


737-743: LGTM!

The argument parser setup for the stack command is correctly configured with appropriate flags, help text, and mutual exclusion for --list and --describe.


772-773: LGTM!

The command dispatch for stack is correctly integrated into the main command router with proper indentation and return handling.

@lu11y0
Copy link
Contributor Author

lu11y0 commented Dec 13, 2025

@mikejmorgan-ai Ready to merge?

@mikejmorgan-ai
Copy link
Member

CI Failing - Action Required

Hey @kr16h! Thanks for the Smart Stacks feature. Before we can merge, please fix:

  • CodeQL Analysis: failing

Please push fixes and let us know when ready for re-review!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
cortex/cli.py (1)

273-290: Per-package LLM overhead remains unaddressed.

Lines 278-280 call self.install() for each package individually, triggering a new CommandInterpreter instance and separate LLM API call per package. For a 6-package stack, this results in 6 API calls rather than 1 batched request, incurring unnecessary cost and latency.

Consider batching all packages into a single install request or implementing a batch installation method that reuses the CommandInterpreter instance. For example:

def _handle_stack_real_install(self, stack: dict[str, Any], packages: list[str]) -> int:
    """Install all packages in the stack."""
    cx_print(f"\n🚀 Installing stack: {stack['name']}\n", "success")
    
    # Batch installation: single LLM call for all packages
    packages_str = " ".join(packages)
    result = self.install(software=packages_str, execute=True, dry_run=False)
    
    if result != 0:
        self._print_error(f"Failed to install stack '{stack['name']}'")
        return 1
    
    self._print_success(f"\n✅ Stack '{stack['name']}' installed successfully!")
    console.print(f"Installed {len(packages)} packages")
    return 0

Based on past review feedback that was not yet addressed.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 72b0a7e and 84c3658.

📒 Files selected for processing (1)
  • cortex/cli.py (5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/cli.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*.py : Type hints required in Python code

Applied to files:

  • cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (3)
cortex/notification_manager.py (1)
  • NotificationManager (13-149)
cortex/stack_manager.py (5)
  • StackManager (18-93)
  • list_stacks (40-43)
  • find_stack (45-51)
  • describe_stack (74-93)
  • suggest_stack (60-72)
cortex/branding.py (1)
  • cx_print (49-69)
🪛 GitHub Actions: CI
cortex/cli.py

[error] 1-1: Ruff check failed: I001 Import block is un-sorted or un-formatted. Command used: 'ruff check . --output-format=github'.

🪛 GitHub Check: Lint
cortex/cli.py

[failure] 3-3: Ruff (UP035)
cortex/cli.py:3:1: UP035 typing.Dict is deprecated, use dict instead


[failure] 3-3: Ruff (UP035)
cortex/cli.py:3:1: UP035 typing.List is deprecated, use list instead


[failure] 32-32: Ruff (F811)
cortex/cli.py:32:41: F811 Redefinition of unused NotificationManager from line 21


[failure] 15-33: Ruff (I001)
cortex/cli.py:15:1: I001 Import block is un-sorted or un-formatted


[failure] 255-255: Ruff (W293)
cortex/cli.py:255:1: W293 Blank line contains whitespace


[failure] 250-250: Ruff (W293)
cortex/cli.py:250:1: W293 Blank line contains whitespace


[failure] 243-243: Ruff (W293)
cortex/cli.py:243:1: W293 Blank line contains whitespace


[failure] 237-237: Ruff (W293)
cortex/cli.py:237:1: W293 Blank line contains whitespace


[failure] 205-205: Ruff (W293)
cortex/cli.py:205:1: W293 Blank line contains whitespace

🔇 Additional comments (3)
cortex/cli.py (3)

177-203: LGTM: Clean command routing with proper error handling.

The method correctly validates --dry-run requires a stack name, routes to appropriate handlers (list/describe/install), and wraps stack operations in try-catch for FileNotFoundError and ValueError with user-friendly error messages.


778-784: LGTM: Clean argparse configuration for stack command.

The subparser correctly defines the stack command with optional name argument, mutually exclusive list/describe flags, and separate dry-run flag. Help text is clear and informative.


819-820: LGTM: Proper command dispatch.

The stack command dispatch follows the established pattern and correctly routes to cli.stack(args).

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

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)

1-29: Fix import sorting to resolve CI failure.

The CI pipeline reports "Import block is un-sorted or un-formatted" which is blocking the build. While the imports appear mostly correct, they may violate isort's specific ordering rules.

Run the following to identify and fix the import ordering issue:

#!/bin/bash
# Check import sorting with isort
cd "$(git rev-parse --show-toplevel)"
isort --check-only --diff cortex/cli.py || echo "Import sorting violations found"

# Optionally show what the correct order should be
isort --diff cortex/cli.py

After running the script, apply isort's suggested fixes to resolve the CI failure:

#!/bin/bash
isort cortex/cli.py

As per coding guidelines, follow PEP 8 style and ensure imports are properly sorted.

♻️ Duplicate comments (1)
cortex/cli.py (1)

20-20: Remove trailing whitespace to fix CI lint failure.

Line 20 has trailing whitespace after the import statement, causing a Ruff W291 violation that blocks CI.

Apply this diff:

-from cortex.stack_manager import StackManager  
+from cortex.stack_manager import StackManager

As per coding guidelines, follow PEP 8 style.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 84c3658 and 9d76f1c.

📒 Files selected for processing (1)
  • cortex/cli.py (5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/cli.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*.py : Type hints required in Python code

Applied to files:

  • cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (4)
cortex/stack_manager.py (5)
  • StackManager (18-93)
  • list_stacks (40-43)
  • find_stack (45-51)
  • describe_stack (74-93)
  • suggest_stack (60-72)
cortex/first_run_wizard.py (1)
  • _print_error (746-748)
cortex/branding.py (1)
  • cx_print (49-69)
cortex/user_preferences.py (1)
  • get (189-208)
🪛 GitHub Actions: CI
cortex/cli.py

[error] 1-1: Ruff check failed: Import block is un-sorted or un-formatted.

🪛 GitHub Check: Lint
cortex/cli.py

[failure] 20-20: Ruff (W291)
cortex/cli.py:20:46: W291 Trailing whitespace


[failure] 248-248: Ruff (W293)
cortex/cli.py:248:1: W293 Blank line contains whitespace


[failure] 245-245: Ruff (W293)
cortex/cli.py:245:1: W293 Blank line contains whitespace


[failure] 240-240: Ruff (W293)
cortex/cli.py:240:1: W293 Blank line contains whitespace


[failure] 233-233: Ruff (W293)
cortex/cli.py:233:1: W293 Blank line contains whitespace


[failure] 228-228: Ruff (W293)
cortex/cli.py:228:1: W293 Blank line contains whitespace


[failure] 199-199: Ruff (W291)
cortex/cli.py:199:21: W291 Trailing whitespace


[failure] 172-172: Ruff (W293)
cortex/cli.py:172:1: W293 Blank line contains whitespace

🔇 Additional comments (3)
cortex/cli.py (3)

265-267: Nice batching solution to avoid per-package LLM overhead!

The implementation correctly batches all stack packages into a single install() call, avoiding the expensive per-package LLM invocation pattern flagged in previous reviews. This significantly reduces cost and latency for multi-package stacks.


760-766: LGTM! Clean parser setup with proper mutual exclusion.

The stack subcommand parser is well-structured with clear help text and proper use of add_mutually_exclusive_group to prevent conflicting --list and --describe flags.


801-802: LGTM! Consistent command dispatch.

The stack command dispatch follows the established pattern and integrates cleanly with the existing command routing logic.

@lu11y0 lu11y0 marked this pull request as draft December 17, 2025 08:01
@lu11y0 lu11y0 marked this pull request as ready for review December 17, 2025 14:15
@lu11y0
Copy link
Contributor Author

lu11y0 commented Dec 17, 2025

Hi @mikejmorgan-ai - all checks have been passed, review it please!, Had some issues with black format and ruff that are now resolved!!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
tests/test_smart_stacks.py (1)

5-12: Consider adding type hints and docstring for test clarity.

While the test logic correctly validates hardware-aware stack suggestion, adding a type hint for monkeypatch and a docstring would improve readability and align with coding guidelines.

Apply this diff:

-def test_suggest_stack_ml_gpu_and_cpu(monkeypatch):
+def test_suggest_stack_ml_gpu_and_cpu(monkeypatch: pytest.MonkeyPatch) -> None:
+    """Test that 'ml' stack falls back to 'ml-cpu' when no GPU is detected."""
     manager = StackManager()

Then add the import at the top:

+import pytest
+
 import cortex.stack_manager as stack_manager

Based on learnings, type hints are required in Python code.

cortex/cli.py (1)

287-298: Consider refactoring hardcoded package workaround to reduce maintenance burden.

This hardcoded special-case for PyTorch CPU installation addresses a real issue (LLM generating outdated commands) but creates tight coupling and technical debt. As more stacks are added, maintaining these workarounds in the CLI will become unwieldy.

Consider alternatives:

  • Move install commands to stacks.json as pre-validated templates
  • Add a post-processing layer that detects/fixes known problematic patterns
  • Use package version constraints in the stack definitions

This works for now but may warrant a follow-up issue to design a more scalable solution.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9d76f1c and 0be2ae0.

📒 Files selected for processing (4)
  • cortex/cli.py (5 hunks)
  • cortex/stack_manager.py (1 hunks)
  • test/test_smart_stacks.py (1 hunks)
  • tests/test_smart_stacks.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/test_smart_stacks.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/stack_manager.py
  • tests/test_smart_stacks.py
  • cortex/cli.py
tests/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Maintain >80% test coverage for pull requests

Files:

  • tests/test_smart_stacks.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*.py : Type hints required in Python code

Applied to files:

  • cortex/cli.py
🧬 Code graph analysis (2)
tests/test_smart_stacks.py (1)
cortex/stack_manager.py (2)
  • StackManager (17-80)
  • suggest_stack (56-59)
cortex/cli.py (2)
cortex/stack_manager.py (5)
  • StackManager (17-80)
  • list_stacks (39-42)
  • find_stack (44-49)
  • describe_stack (61-80)
  • suggest_stack (56-59)
cortex/first_run_wizard.py (1)
  • _print_error (746-748)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
cortex/cli.py (1)

287-298: Consider moving stack-specific install overrides to StackManager.

This special-case hardcoding for ml-cpu has several maintainability concerns:

  • Exact string match is fragile (won't handle package reordering or variations)
  • Tight coupling between CLI and stack implementation details
  • Hardcoded commands make updates difficult
  • Workaround for LLM issues rather than addressing root cause

Consider refactoring to support command overrides in stacks.json or StackManager, so stack-specific installation recipes are data-driven rather than hardcoded in the CLI. This would make it easier to maintain and extend for other stacks that need custom install commands.

💡 Possible approach

In stacks.json, allow optional install_command field:

{
  "id": "ml-cpu",
  "packages": ["pytorch-cpu", "jupyter", "numpy", "pandas"],
  "install_command": "pip3 install torch torchvision torchaudio --index-url https://download.pytorch.org/whl/cpu && pip3 install jupyter numpy pandas"
}

Then in _handle_stack_real_install, check for stack.get("install_command") and use it if present, falling back to the standard LLM-based install otherwise.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6f70afa and c5540b8.

📒 Files selected for processing (1)
  • cortex/cli.py (6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/cli.py
🧠 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 **/*.py : Type hints required in Python code

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 : Follow PEP 8 style guide

Applied to files:

  • cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/stack_manager.py (5)
  • StackManager (17-102)
  • list_stacks (39-42)
  • find_stack (44-49)
  • describe_stack (72-102)
  • suggest_stack (56-70)
🔇 Additional comments (3)
cortex/cli.py (3)

174-202: LGTM! Well-structured command handler with proper validation.

The method correctly:

  • Includes type hints and docstring (coding guidelines)
  • Validates --dry-run requires a stack name
  • Handles FileNotFoundError/ValueError from StackManager with user-friendly messages
  • Routes to appropriate handlers based on flags

204-279: LGTM! Handler methods follow best practices.

All stack handlers properly:

  • Use modern type hints (dict, list) per coding guidelines
  • Include clear docstrings
  • Handle error cases with appropriate exit codes
  • Validate inputs (stack existence, non-empty packages)
  • Provide user-friendly messages with cx_print/console

The batch installation (line 270-271) correctly joins packages into a single install request, addressing the previous LLM overhead concern.


684-684: LGTM! CLI integration is clean and consistent.

The stack command integration properly:

  • Adds help text to the command table
  • Configures argparse with appropriate flags and mutual exclusivity
  • Follows the same dispatch pattern as other commands

Also applies to: 777-787, 822-823

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

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)

1-853: Fix Black formatting to resolve CI failure.

The pipeline shows a Black formatting check failure. This is blocking CI and must be resolved before merge.

Run Black to reformat the code:

#!/bin/bash
# Format the code with Black
black cortex/cli.py
🧹 Nitpick comments (1)
cortex/cli.py (1)

293-304: Consider making the PyTorch CPU special case more robust.

The exact string match for "pytorch-cpu jupyter numpy pandas" (line 299) is brittle—if the stack's package list order changes or contains variations, the workaround won't trigger. Consider matching against the set of packages or using a more flexible pattern.

🔎 More flexible matching approach
-        normalized = " ".join(software.split()).lower()
-
-        if normalized == "pytorch-cpu jupyter numpy pandas":
+        # Normalize to set for order-independent matching
+        packages_set = set(software.lower().split())
+        ml_cpu_packages = {"pytorch-cpu", "jupyter", "numpy", "pandas"}
+
+        if packages_set == ml_cpu_packages:
             software = (
                 "pip3 install torch torchvision torchaudio "
                 "--index-url https://download.pytorch.org/whl/cpu && "
                 "pip3 install jupyter numpy pandas"
             )
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c5540b8 and 20ba60b.

📒 Files selected for processing (1)
  • cortex/cli.py (7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/cli.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*.py : Type hints required in Python code

Applied to files:

  • cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/stack_manager.py (5)
  • StackManager (17-102)
  • list_stacks (39-42)
  • find_stack (44-49)
  • describe_stack (72-102)
  • suggest_stack (56-70)
🪛 GitHub Actions: CI
cortex/cli.py

[error] 1-1: Black formatting check failed. 1 file would be reformatted. Run 'black .' to format the code.

🔇 Additional comments (2)
cortex/cli.py (2)

265-279: Excellent batching optimization!

Lines 270-271 batch all stack packages into a single LLM request instead of per-package calls, which elegantly addresses the performance and cost concerns from earlier reviews. This is a great solution that maintains simplicity while avoiding expensive per-package API calls.


174-253: Well-structured stack command implementation.

The stack command flow is clean and well-organized:

  • Proper validation (--dry-run requires name)
  • Comprehensive error handling (FileNotFoundError, ValueError)
  • Hardware-aware fallback (ml → ml-cpu without GPU)
  • All methods have type hints and docstrings per coding guidelines

Nice work on addressing all the previous review feedback!

@sonarqubecloud
Copy link

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
cortex/cli.py (2)

294-305: Consider a more flexible matching approach for PyTorch CPU workaround.

The exact string match normalized == "pytorch-cpu jupyter numpy pandas" works for the ml-cpu stack but is fragile:

  • Won't match if packages are in different order
  • Won't match if requesting just pytorch-cpu without other packages
  • Requires maintenance if stack packages change

Consider a more flexible approach:

🔎 View suggested improvements

Option 1: Pattern-based matching

-        normalized = " ".join(software.split()).lower()
-
-        if normalized == "pytorch-cpu jupyter numpy pandas":
+        tokens = set(software.lower().split())
+        
+        if "pytorch-cpu" in tokens or "torch-cpu" in tokens or ("pytorch" in tokens and "cpu" in tokens):
             software = (
                 "pip3 install torch torchvision torchaudio "
                 "--index-url https://download.pytorch.org/whl/cpu && "
-                "pip3 install jupyter numpy pandas"
+                f"pip3 install {' '.join(t for t in software.split() if t.lower() not in ['pytorch-cpu', 'torch-cpu', 'pytorch', 'torch'])}"
             )

Option 2: Move to StackManager

Add a get_install_command(stack_id: str) method in StackManager that returns pre-validated install commands for known problematic stacks, keeping workarounds centralized in the stack configuration layer.


691-691: Optional: Clarify stack command help text.

The help text "Install the stack" doesn't mention list/describe capabilities. Consider:

-    table.add_row("stack <name>", "Install the stack")
+    table.add_row("stack <name>", "Install pre-built package stacks (--list to see all)")
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 20ba60b and 842f972.

📒 Files selected for processing (1)
  • cortex/cli.py (7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs

Files:

  • cortex/cli.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*.py : Type hints required in Python code

Applied to files:

  • cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/stack_manager.py (5)
  • StackManager (17-102)
  • list_stacks (39-42)
  • find_stack (44-49)
  • describe_stack (72-102)
  • suggest_stack (56-70)
🔇 Additional comments (9)
cortex/cli.py (9)

7-7: LGTM!

The imports are clean and follow modern Python conventions (using built-in list/dict instead of deprecated typing.List/typing.Dict).

Also applies to: 20-20


174-202: LGTM!

The method properly validates inputs, handles exceptions gracefully, and routes to appropriate handlers. Type hints and docstring are present as required by coding guidelines.


204-215: LGTM!

The list handler is clean and properly formatted. Type hints and docstring are present as required.


217-225: LGTM!

The describe handler correctly returns non-zero exit code when the stack is not found, as requested in previous review feedback.


227-253: LGTM!

The install handler properly implements hardware-aware stack selection and validates the stack has packages before proceeding, addressing previous review feedback.


255-263: LGTM!

The dry-run handler provides a clear preview of packages without executing anything. Uses modern type hints correctly.


265-279: LGTM! Nice optimization.

The real install handler now batches all packages into a single LLM request (line 270-271), addressing the previous review concern about expensive per-package LLM API calls.


788-798: LGTM!

The argparse configuration is well-structured with proper mutual exclusion between --list and --describe, and clear help text.


833-834: LGTM!

The command routing is clean and consistent with other commands in the file.

@mikejmorgan-ai mikejmorgan-ai merged commit c482df9 into cortexlinux:main Dec 19, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[MVP] Pre-built package stacks (ML, Web, DevOps)

2 participants