feat: Unified Snap/Flatpak Package Manager (Bounty #450) #462
Conversation
|
Warning Rate limit exceeded
⌛ 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 (5)
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. 📝 WalkthroughWalkthroughAdded a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CortexCLI
participant PM as UnifiedPackageManager
participant Sys as System
User->>CLI: pkg install <package> [--dry-run]
CLI->>PM: install(package, dry_run)
PM->>PM: check_backends()
alt both backends available
PM->>User: prompt backend choice
User-->>PM: choose backend
else one backend available
PM-->>PM: select available backend
else no backends
PM-->>CLI: warn no backends
end
PM->>PM: _get_install_cmd(backend, package)
alt dry-run
PM-->>CLI: print dry-run info
else execute
PM->>Sys: run install command (subprocess)
Sys-->>PM: success / error
end
PM-->>CLI: return exit code
CLI-->>User: display status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
CLA Verification PassedAll contributors have signed the CLA.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
cortex/package_manager.py (2)
8-11: Consider expanding the docstring.The class docstring could benefit from examples or a brief description of key methods. This would improve developer ergonomics, especially for a new feature.
Example enhanced docstring
class UnifiedPackageManager: """ Unified manager for Snap and Flatpak packages. + + Provides a consistent interface to install, remove, and list packages + from both Snap and Flatpak backends. Automatically detects available + backends and prompts user to choose when both are present. + + Example: + mgr = UnifiedPackageManager() + mgr.install("firefox", dry_run=True) + mgr.list_packages() """
63-76: Verify the fallback behavior when no backends are available.The method returns
"snap"(line 76) when neither backend is available. While the comment mentions this is for "testing/dry-run", this could be confusing in practice:
- If a user runs
cortex pkg install firefoxwithout either backend, they'll get prompted to choose, then see "Executable not found: sudo" (from line 103)- The warning from
check_backends()appears earlier, but the flow continues as if snap is availableConsider returning
Nonehere to make the "no backend available" case more explicit, or at minimum, add a clear warning message.Alternative approach
def _choose_backend(self, action: str) -> Optional[str]: if self.snap_avail and self.flatpak_avail: return Prompt.ask( f"Choose backend to {action}", choices=["snap", "flatpak"], default="snap" ) elif self.snap_avail: return "snap" elif self.flatpak_avail: return "flatpak" else: - # Fallback for testing/dry-run if forced, or just default to snap for print - return "snap" + cx_print("No package backend available. Cannot proceed.", "error") + return NoneThis makes the error handling more explicit and prevents attempting operations that will fail.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/package_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.pycortex/package_manager.py
🧬 Code graph analysis (2)
cortex/cli.py (3)
cortex/package_manager.py (4)
UnifiedPackageManager(8-103)install(21-29)remove(31-39)list_packages(41-61)cortex/sandbox/docker_sandbox.py (1)
install(419-481)cortex/first_run_wizard.py (1)
_print_error(746-748)
cortex/package_manager.py (1)
cortex/branding.py (2)
cx_print(49-69)cx_header(82-88)
⏰ 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 (3)
cortex/cli.py (3)
1578-1578: LGTM!The help table entry is clear and consistent with other commands.
1883-1902: LGTM!The parser configuration correctly defines the pkg subcommand structure with appropriate arguments and follows the established pattern in the codebase.
1949-1950: LGTM!The dispatch logic correctly routes both the primary command and its alias to the pkg handler.
| def pkg(self, args: argparse.Namespace) -> int: | ||
| """Handle package manager commands.""" | ||
| try: | ||
| from cortex.package_manager import UnifiedPackageManager | ||
|
|
||
| mgr = UnifiedPackageManager() | ||
| action = getattr(args, "pkg_action", None) | ||
|
|
||
| if action == "install": | ||
| mgr.install(args.package, dry_run=args.dry_run) | ||
| elif action == "remove": | ||
| mgr.remove(args.package, dry_run=args.dry_run) | ||
| elif action == "list": | ||
| mgr.list_packages() | ||
| else: | ||
| self._print_error("Unknown package manager action") | ||
| return 1 | ||
| return 0 | ||
| except Exception as e: | ||
| self._print_error(f"Package Manager failed: {e}") | ||
| return 1 | ||
|
|
There was a problem hiding this comment.
Handle missing pkg_action gracefully.
If pkg_action is None (e.g., when user runs cortex pkg without a subcommand), the current code will reach the else block and print "Unknown package manager action", which is misleading. Consider adding an explicit check at the start similar to the notify command pattern (lines 110-112).
🔎 Suggested fix
def pkg(self, args: argparse.Namespace) -> int:
"""Handle package manager commands."""
try:
from cortex.package_manager import UnifiedPackageManager
mgr = UnifiedPackageManager()
action = getattr(args, "pkg_action", None)
+
+ if not action:
+ self._print_error("Please specify a subcommand (install/remove/list)")
+ return 1
if action == "install":
mgr.install(args.package, dry_run=args.dry_run)
elif action == "remove":
mgr.remove(args.package, dry_run=args.dry_run)
elif action == "list":
mgr.list_packages()
else:
self._print_error("Unknown package manager action")
return 1
return 0🤖 Prompt for AI Agents
In cortex/cli.py around lines 1524 to 1545, the pkg command does not explicitly
handle a missing pkg_action and falls through to a misleading "Unknown package
manager action" message; add an immediate check after obtaining action (action =
getattr(args, "pkg_action", None)) that if action is None prints a clear error
(e.g., "No package manager action specified" or shows the pkg usage) and returns
1, before the install/remove/list branches so the no-subcommand case is handled
cleanly.
| def __init__(self): | ||
| self.snap_avail = shutil.which("snap") is not None | ||
| self.flatpak_avail = shutil.which("flatpak") is not None |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints and docstring per coding guidelines.
The __init__ method is missing both type hints and a docstring, which are required by the project's coding guidelines for Python code.
🔎 Proposed enhancement
- def __init__(self):
+ def __init__(self) -> None:
+ """Initialize the package manager and detect available backends."""
self.snap_avail = shutil.which("snap") is not None
self.flatpak_avail = shutil.which("flatpak") is not NoneAs per coding guidelines: "Type hints required in Python code" and "Docstrings required for all public APIs"
📝 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.
| def __init__(self): | |
| self.snap_avail = shutil.which("snap") is not None | |
| self.flatpak_avail = shutil.which("flatpak") is not None | |
| def __init__(self) -> None: | |
| """Initialize the package manager and detect available backends.""" | |
| self.snap_avail = shutil.which("snap") is not None | |
| self.flatpak_avail = shutil.which("flatpak") is not None |
🤖 Prompt for AI Agents
In cortex/package_manager.py around lines 12 to 14, the __init__ method lacks
type hints and a docstring; add explicit type annotations for the method
signature (def __init__(self) -> None) and annotate the instance attributes
(self.snap_avail: bool and self.flatpak_avail: bool) and include a short
docstring describing what the initializer does and what the attributes represent
(e.g., checks availability of snap and flatpak). Ensure annotations use built-in
types and the docstring follows the project's docstring style.
| def check_backends(self): | ||
| if not self.snap_avail and not self.flatpak_avail: | ||
| cx_print("Warning: Neither 'snap' nor 'flatpak' found on this system.", "warning") | ||
| cx_print("Commands will run in DRY-RUN mode or fail.", "info") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints and docstring per coding guidelines.
This public method is missing both type hints and a docstring, which are required by the project's coding guidelines.
🔎 Proposed enhancement
- def check_backends(self):
+ def check_backends(self) -> None:
+ """Check if snap or flatpak backends are available and warn if neither is found."""
if not self.snap_avail and not self.flatpak_avail:
cx_print("Warning: Neither 'snap' nor 'flatpak' found on this system.", "warning")
cx_print("Commands will run in DRY-RUN mode or fail.", "info")As per coding guidelines.
🤖 Prompt for AI Agents
In cortex/package_manager.py around lines 16–19, the public method
check_backends is missing type hints and a docstring; update the signature to
include a return type (-> None) and, if applicable, an explicit self type (e.g.,
self) and add a concise triple-quoted docstring that states what the method
checks (presence of snap and flatpak), its side effects (calls cx_print with
warning/info), and any important behavior (e.g., when it triggers DRY-RUN mode
or failure); ensure the docstring follows project style and update imports only
if you add explicit typing requiring them.
cortex/package_manager.py
Outdated
| def install(self, package: str, dry_run: bool = False): | ||
| self.check_backends() | ||
|
|
||
| backend = self._choose_backend("install") | ||
| if not backend: | ||
| return | ||
|
|
||
| cmd = self._get_install_cmd(backend, package) | ||
| self._run_cmd(cmd, dry_run) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints and docstring per coding guidelines.
This public method is missing both type hints and a docstring, which are required by the project's coding guidelines.
🔎 Proposed enhancement
- def install(self, package: str, dry_run: bool = False):
+ def install(self, package: str, dry_run: bool = False) -> None:
+ """
+ Install a package using Snap or Flatpak.
+
+ Args:
+ package: Name of the package to install
+ dry_run: If True, only show what would be executed without running it
+ """
self.check_backends()
backend = self._choose_backend("install")
if not backend:
return
cmd = self._get_install_cmd(backend, package)
self._run_cmd(cmd, dry_run)As per coding guidelines.
📝 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.
| def install(self, package: str, dry_run: bool = False): | |
| self.check_backends() | |
| backend = self._choose_backend("install") | |
| if not backend: | |
| return | |
| cmd = self._get_install_cmd(backend, package) | |
| self._run_cmd(cmd, dry_run) | |
| def install(self, package: str, dry_run: bool = False) -> None: | |
| """ | |
| Install a package using Snap or Flatpak. | |
| Args: | |
| package: Name of the package to install | |
| dry_run: If True, only show what would be executed without running it | |
| """ | |
| self.check_backends() | |
| backend = self._choose_backend("install") | |
| if not backend: | |
| return | |
| cmd = self._get_install_cmd(backend, package) | |
| self._run_cmd(cmd, dry_run) |
🤖 Prompt for AI Agents
In cortex/package_manager.py around lines 21 to 29, the public method install is
missing a docstring and a return type annotation; update the signature to
include an explicit return type (-> None) and add a concise docstring describing
what the method does, its parameters (package: str, dry_run: bool), the return
value (None), and any exceptions it may raise (e.g., if no backend found or
command fails); keep the docstring style consistent with the project's docstring
conventions (summary line, params, returns, raises).
cortex/package_manager.py
Outdated
| def remove(self, package: str, dry_run: bool = False): | ||
| self.check_backends() | ||
|
|
||
| backend = self._choose_backend("remove") | ||
| if not backend: | ||
| return | ||
|
|
||
| cmd = self._get_remove_cmd(backend, package) | ||
| self._run_cmd(cmd, dry_run) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints and docstring per coding guidelines.
This public method is missing both type hints and a docstring, which are required by the project's coding guidelines.
🔎 Proposed enhancement
- def remove(self, package: str, dry_run: bool = False):
+ def remove(self, package: str, dry_run: bool = False) -> None:
+ """
+ Remove a package using Snap or Flatpak.
+
+ Args:
+ package: Name of the package to remove
+ dry_run: If True, only show what would be executed without running it
+ """
self.check_backends()
backend = self._choose_backend("remove")
if not backend:
return
cmd = self._get_remove_cmd(backend, package)
self._run_cmd(cmd, dry_run)As per coding guidelines.
📝 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.
| def remove(self, package: str, dry_run: bool = False): | |
| self.check_backends() | |
| backend = self._choose_backend("remove") | |
| if not backend: | |
| return | |
| cmd = self._get_remove_cmd(backend, package) | |
| self._run_cmd(cmd, dry_run) | |
| def remove(self, package: str, dry_run: bool = False) -> None: | |
| """ | |
| Remove a package using Snap or Flatpak. | |
| Args: | |
| package: Name of the package to remove | |
| dry_run: If True, only show what would be executed without running it | |
| """ | |
| self.check_backends() | |
| backend = self._choose_backend("remove") | |
| if not backend: | |
| return | |
| cmd = self._get_remove_cmd(backend, package) | |
| self._run_cmd(cmd, dry_run) |
🤖 Prompt for AI Agents
In cortex/package_manager.py around lines 31 to 39, the public method remove has
parameter type hints but is missing a return type annotation and a docstring;
add a return type of -> None to the signature and insert a concise triple-quoted
docstring immediately below the def describing what the method does, its
parameters (package: str, dry_run: bool), and that it returns None (and note any
exceptions it may raise if applicable), keeping wording brief and following the
project's docstring style.
cortex/package_manager.py
Outdated
| def list_packages(self): | ||
| cx_header("Installed Packages (Snap & Flatpak)") | ||
| table = Table(show_header=True, header_style="bold magenta") | ||
| table.add_column("Package") | ||
| table.add_column("Backend") | ||
| table.add_column("Version", style="dim") | ||
|
|
||
| # Listings would require parsing output commands like `snap list` and `flatpak list` | ||
| # For MVP, we just show support status | ||
| if self.snap_avail: | ||
| table.add_row("Snap Support", "Detected", "Active") | ||
| else: | ||
| table.add_row("Snap Support", "Not Found", "-") | ||
|
|
||
| if self.flatpak_avail: | ||
| table.add_row("Flatpak Support", "Detected", "Active") | ||
| else: | ||
| table.add_row("Flatpak Support", "Not Found", "-") | ||
|
|
||
| console.print(table) | ||
| console.print("[dim](Full package listing implementation pending parsing logic)[/dim]") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints and docstring per coding guidelines.
This public method is missing both type hints and a docstring. Additionally, consider documenting in the docstring that this is currently a placeholder showing backend status rather than actual package listings.
🔎 Proposed enhancement
- def list_packages(self):
+ def list_packages(self) -> None:
+ """
+ List installed packages from Snap and Flatpak.
+
+ Note: Currently shows backend availability status only.
+ Full package listing requires parsing snap/flatpak command output.
+ """
cx_header("Installed Packages (Snap & Flatpak)")
table = Table(show_header=True, header_style="bold magenta")
table.add_column("Package")
table.add_column("Backend")
table.add_column("Version", style="dim")
# Listings would require parsing output commands like `snap list` and `flatpak list`
# For MVP, we just show support status
if self.snap_avail:
table.add_row("Snap Support", "Detected", "Active")
else:
table.add_row("Snap Support", "Not Found", "-")
if self.flatpak_avail:
table.add_row("Flatpak Support", "Detected", "Active")
else:
table.add_row("Flatpak Support", "Not Found", "-")
console.print(table)
console.print("[dim](Full package listing implementation pending parsing logic)[/dim]")As per coding guidelines.
🤖 Prompt for AI Agents
In cortex/package_manager.py around lines 41 to 61, the public method
list_packages is missing type hints and a docstring; update the signature to
include proper type hints (e.g., def list_packages(self) -> None) and add a
concise Google- or numpydoc-style docstring that states what the method does,
its parameters (self), return type (None), and explicitly documents that this is
a placeholder showing backend support status for Snap/Flatpak rather than a full
package listing until parsing logic is implemented; keep the docstring brief and
place it immediately under the def line.
cortex/package_manager.py
Outdated
| def _get_install_cmd(self, backend: str, package: str) -> List[str]: | ||
| if backend == "snap": | ||
| return ["sudo", "snap", "install", package] | ||
| else: | ||
| return ["flatpak", "install", "-y", package] | ||
|
|
||
| def _get_remove_cmd(self, backend: str, package: str) -> List[str]: | ||
| if backend == "snap": | ||
| return ["sudo", "snap", "remove", package] | ||
| else: | ||
| return ["flatpak", "uninstall", "-y", package] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
flatpak install command syntax remote package documentation
💡 Result:
Quick summary — syntax and common forms
-
Basic form: flatpak install [OPTION...] REMOTE NAME [BRANCH]. REMOTE is the configured remote name (or a file:// URI); NAME is the app/runtime ref (e.g. org.gnome.Gedit); BRANCH is optional when you need a non-default branch. [1][2]
-
Common options:
- --user / --system / --installation=NAME — choose installation scope. [1][2]
- --app or --runtime — restrict search to apps or runtimes. [2]
- --bundle FILENAME or --from <.flatpakref|.flatpak> — install from a bundle or .flatpakref instead of a remote. [2]
- -y/--assumeyes, --noninteractive — automation switches. [2]
-
Example:
- flatpak install flathub org.gnome.Gedit
- flatpak --user install test-repo org.gnome.GEdit
- flatpak install --bundle /path/to/app.flatpak
Sources:
[1] Flatpak command reference / Using Flatpak (official docs).
[2] flatpak-install man page (flatpak install SYNOPSIS and OPTIONS).
Flatpak install command is missing the required remote source.
The flatpak install command at line 82 is incomplete. According to the official flatpak documentation, the syntax requires specifying a remote (e.g., "flathub") before the package name:
flatpak install [OPTIONS] REMOTE NAMEThe current command ["flatpak", "install", "-y", package] omits the REMOTE argument, which will cause the command to fail.
Suggested fix
def _get_install_cmd(self, backend: str, package: str) -> List[str]:
if backend == "snap":
return ["sudo", "snap", "install", package]
else:
- return ["flatpak", "install", "-y", package]
+ return ["flatpak", "install", "-y", "flathub", package]Alternatively, consider making the remote configurable if different sources are needed.
🤖 Prompt for AI Agents
In cortex/package_manager.py around lines 78 to 88, the flatpak install command
is missing the required REMOTE argument so it will fail; update _get_install_cmd
to include a default remote (e.g., "flathub") before the package name
(["flatpak","install","-y","flathub", package]) or refactor to accept a
configurable remote (class attribute or additional parameter) and use that
value; leave _get_remove_cmd as-is (flatpak uninstall does not need a remote)
and ensure any callers are updated if you add a parameter.
cortex/package_manager.py
Outdated
| def _run_cmd(self, cmd: List[str], dry_run: bool): | ||
| cmd_str = " ".join(cmd) | ||
| if dry_run: | ||
| cx_print(f"[Dry Run] would execute: [bold]{cmd_str}[/bold]", "info") | ||
| return | ||
|
|
||
| cx_print(f"Running: {cmd_str}...", "info") | ||
| try: | ||
| subprocess.check_call(cmd) | ||
| cx_print("Command executed successfully.", "success") | ||
| except subprocess.CalledProcessError as e: | ||
| cx_print(f"Command failed: {e}", "error") | ||
| except FileNotFoundError: | ||
| cx_print(f"Executable not found: {cmd[0]}", "error") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints and docstring per coding guidelines.
This private method is missing a return type hint and a docstring. While docstrings for private methods are less critical, type hints are required by the coding guidelines.
🔎 Proposed enhancement
- def _run_cmd(self, cmd: List[str], dry_run: bool):
+ def _run_cmd(self, cmd: List[str], dry_run: bool) -> None:
+ """
+ Execute or simulate execution of a command.
+
+ Args:
+ cmd: Command and arguments as a list
+ dry_run: If True, only print the command without executing it
+ """
cmd_str = " ".join(cmd)
if dry_run:
cx_print(f"[Dry Run] would execute: [bold]{cmd_str}[/bold]", "info")
return
cx_print(f"Running: {cmd_str}...", "info")
try:
subprocess.check_call(cmd)
cx_print("Command executed successfully.", "success")
except subprocess.CalledProcessError as e:
cx_print(f"Command failed: {e}", "error")
except FileNotFoundError:
cx_print(f"Executable not found: {cmd[0]}", "error")As per coding guidelines: "Type hints required in Python code"
🤖 Prompt for AI Agents
In cortex/package_manager.py around lines 90 to 103, the private method _run_cmd
has parameter type hints but is missing a return type hint and a docstring;
update the signature to include a return type (-> None) and add a concise
docstring describing the method's purpose, parameters (cmd, dry_run), behavior
(executes subprocess, prints dry-run message), and exceptions it may
raise/handle (subprocess.CalledProcessError, FileNotFoundError) so it adheres to
the coding guidelines.
There was a problem hiding this comment.
Pull request overview
This PR adds a unified package manager interface for Snap and Flatpak package management through a new cortex pkg command. This addresses bounty #450 by providing a seamless way to install and remove applications using either Snap or Flatpak backends.
Key Changes:
- New
UnifiedPackageManagerclass that abstracts Snap/Flatpak operations - CLI integration with
pkgcommand supporting install, remove, and list subcommands - Interactive backend selection when both Snap and Flatpak are available
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 18 comments.
| File | Description |
|---|---|
| cortex/package_manager.py | Implements the core UnifiedPackageManager class with backend detection, command generation, and execution logic for Snap/Flatpak operations |
| cortex/cli.py | Adds CLI handler for the pkg command, argument parsers for pkg subcommands (install/remove/list), and help table entry |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cortex/package_manager.py
Outdated
| def install(self, package: str, dry_run: bool = False): | ||
| self.check_backends() | ||
|
|
||
| backend = self._choose_backend("install") | ||
| if not backend: | ||
| return | ||
|
|
||
| cmd = self._get_install_cmd(backend, package) | ||
| self._run_cmd(cmd, dry_run) |
There was a problem hiding this comment.
Missing input validation for the package parameter. The package name should be validated to prevent command injection attacks. For example, a package name like "; rm -rf /" could be dangerous when passed to subprocess. Consider adding validation to ensure the package name contains only allowed characters (alphanumeric, hyphens, underscores, etc.).
cortex/package_manager.py
Outdated
| # Listings would require parsing output commands like `snap list` and `flatpak list` | ||
| # For MVP, we just show support status | ||
| if self.snap_avail: | ||
| table.add_row("Snap Support", "Detected", "Active") | ||
| else: | ||
| table.add_row("Snap Support", "Not Found", "-") | ||
|
|
||
| if self.flatpak_avail: | ||
| table.add_row("Flatpak Support", "Detected", "Active") | ||
| else: | ||
| table.add_row("Flatpak Support", "Not Found", "-") | ||
|
|
||
| console.print(table) | ||
| console.print("[dim](Full package listing implementation pending parsing logic)[/dim]") |
There was a problem hiding this comment.
The documentation comment is misleading. The list_packages method does not actually list installed packages as the docstring implies. Instead, it only shows whether Snap and Flatpak are detected on the system. The comment on line 61 acknowledges this is pending implementation, but the function name and header message suggest full functionality. Consider renaming the method to check_backends_status or similar to accurately reflect what it does.
| # Listings would require parsing output commands like `snap list` and `flatpak list` | |
| # For MVP, we just show support status | |
| if self.snap_avail: | |
| table.add_row("Snap Support", "Detected", "Active") | |
| else: | |
| table.add_row("Snap Support", "Not Found", "-") | |
| if self.flatpak_avail: | |
| table.add_row("Flatpak Support", "Detected", "Active") | |
| else: | |
| table.add_row("Flatpak Support", "Not Found", "-") | |
| console.print(table) | |
| console.print("[dim](Full package listing implementation pending parsing logic)[/dim]") | |
| # Snap packages | |
| if self.snap_avail: | |
| try: | |
| output = subprocess.check_output(["snap", "list"], text=True) | |
| lines = output.strip().splitlines() | |
| # First line is usually a header: "Name Version Rev ..." | |
| for line in lines[1:]: | |
| parts = line.split() | |
| if len(parts) >= 2: | |
| name = parts[0] | |
| version = parts[1] | |
| table.add_row(name, "snap", version) | |
| if len(table.rows) == 0: | |
| # No snap packages found despite snap being available | |
| table.add_row("No Snap packages found", "snap", "-") | |
| except (subprocess.CalledProcessError, FileNotFoundError): | |
| table.add_row("Error listing Snap packages", "snap", "-") | |
| else: | |
| table.add_row("Snap Support", "Not Found", "-") | |
| # Flatpak packages | |
| if self.flatpak_avail: | |
| try: | |
| output = subprocess.check_output( | |
| ["flatpak", "list", "--columns=application,version"], | |
| text=True, | |
| ) | |
| lines = output.strip().splitlines() | |
| for line in lines: | |
| if not line.strip(): | |
| continue | |
| # flatpak typically separates columns with tabs; fall back to whitespace | |
| if "\t" in line: | |
| parts = line.split("\t") | |
| else: | |
| parts = line.split() | |
| if len(parts) >= 1: | |
| name = parts[0] | |
| version = parts[1] if len(parts) > 1 and parts[1] else "-" | |
| table.add_row(name, "flatpak", version) | |
| # If flatpak is available but no rows were added for it | |
| flatpak_rows = [r for r in table.rows if len(r.cells) >= 2 and r.cells[1].plain == "flatpak"] | |
| if not flatpak_rows: | |
| table.add_row("No Flatpak packages found", "flatpak", "-") | |
| except (subprocess.CalledProcessError, FileNotFoundError): | |
| table.add_row("Error listing Flatpak packages", "flatpak", "-") | |
| else: | |
| table.add_row("Flatpak Support", "Not Found", "-") | |
| console.print(table) |
cortex/package_manager.py
Outdated
| def _get_install_cmd(self, backend: str, package: str) -> List[str]: | ||
| if backend == "snap": | ||
| return ["sudo", "snap", "install", package] | ||
| else: | ||
| return ["flatpak", "install", "-y", package] | ||
|
|
||
| def _get_remove_cmd(self, backend: str, package: str) -> List[str]: | ||
| if backend == "snap": | ||
| return ["sudo", "snap", "remove", package] | ||
| else: | ||
| return ["flatpak", "uninstall", "-y", package] |
There was a problem hiding this comment.
Flatpak commands may require additional parameters for system-wide vs user installation. The current implementation assumes default behavior, but flatpak typically requires specifying "--user" or "--system" scope. Without this, the commands may fail or behave inconsistently. Consider adding a parameter to specify the installation scope or defaulting to "--user" which doesn't require sudo.
cortex/package_manager.py
Outdated
| class UnifiedPackageManager: | ||
| """ | ||
| Unified manager for Snap and Flatpak packages. | ||
| """ | ||
| def __init__(self): | ||
| self.snap_avail = shutil.which("snap") is not None | ||
| self.flatpak_avail = shutil.which("flatpak") is not None | ||
|
|
||
| def check_backends(self): | ||
| if not self.snap_avail and not self.flatpak_avail: | ||
| cx_print("Warning: Neither 'snap' nor 'flatpak' found on this system.", "warning") | ||
| cx_print("Commands will run in DRY-RUN mode or fail.", "info") | ||
|
|
||
| def install(self, package: str, dry_run: bool = False): | ||
| self.check_backends() | ||
|
|
||
| backend = self._choose_backend("install") | ||
| if not backend: | ||
| return | ||
|
|
||
| cmd = self._get_install_cmd(backend, package) | ||
| self._run_cmd(cmd, dry_run) | ||
|
|
||
| def remove(self, package: str, dry_run: bool = False): | ||
| self.check_backends() | ||
|
|
||
| backend = self._choose_backend("remove") | ||
| if not backend: | ||
| return | ||
|
|
||
| cmd = self._get_remove_cmd(backend, package) | ||
| self._run_cmd(cmd, dry_run) | ||
|
|
||
| def list_packages(self): | ||
| cx_header("Installed Packages (Snap & Flatpak)") | ||
| table = Table(show_header=True, header_style="bold magenta") | ||
| table.add_column("Package") | ||
| table.add_column("Backend") | ||
| table.add_column("Version", style="dim") | ||
|
|
||
| # Listings would require parsing output commands like `snap list` and `flatpak list` | ||
| # For MVP, we just show support status | ||
| if self.snap_avail: | ||
| table.add_row("Snap Support", "Detected", "Active") | ||
| else: | ||
| table.add_row("Snap Support", "Not Found", "-") | ||
|
|
||
| if self.flatpak_avail: | ||
| table.add_row("Flatpak Support", "Detected", "Active") | ||
| else: | ||
| table.add_row("Flatpak Support", "Not Found", "-") | ||
|
|
||
| console.print(table) | ||
| console.print("[dim](Full package listing implementation pending parsing logic)[/dim]") | ||
|
|
||
| def _choose_backend(self, action: str) -> Optional[str]: | ||
| if self.snap_avail and self.flatpak_avail: | ||
| return Prompt.ask( | ||
| f"Choose backend to {action}", | ||
| choices=["snap", "flatpak"], | ||
| default="snap" | ||
| ) | ||
| elif self.snap_avail: | ||
| return "snap" | ||
| elif self.flatpak_avail: | ||
| return "flatpak" | ||
| else: | ||
| # Fallback for testing/dry-run if forced, or just default to snap for print | ||
| return "snap" | ||
|
|
||
| def _get_install_cmd(self, backend: str, package: str) -> List[str]: | ||
| if backend == "snap": | ||
| return ["sudo", "snap", "install", package] | ||
| else: | ||
| return ["flatpak", "install", "-y", package] | ||
|
|
||
| def _get_remove_cmd(self, backend: str, package: str) -> List[str]: | ||
| if backend == "snap": | ||
| return ["sudo", "snap", "remove", package] | ||
| else: | ||
| return ["flatpak", "uninstall", "-y", package] | ||
|
|
||
| def _run_cmd(self, cmd: List[str], dry_run: bool): | ||
| cmd_str = " ".join(cmd) | ||
| if dry_run: | ||
| cx_print(f"[Dry Run] would execute: [bold]{cmd_str}[/bold]", "info") | ||
| return | ||
|
|
||
| cx_print(f"Running: {cmd_str}...", "info") | ||
| try: | ||
| subprocess.check_call(cmd) | ||
| cx_print("Command executed successfully.", "success") | ||
| except subprocess.CalledProcessError as e: | ||
| cx_print(f"Command failed: {e}", "error") | ||
| except FileNotFoundError: | ||
| cx_print(f"Executable not found: {cmd[0]}", "error") |
There was a problem hiding this comment.
The new UnifiedPackageManager class lacks test coverage. Given that the repository has comprehensive test coverage for other features (as evidenced by tests/ directory with extensive test files), this new functionality should include tests for: backend detection, command generation, install/remove operations, dry-run mode, error handling, and the list_packages method. Consider adding a test file like tests/test_package_manager.py.
cortex/package_manager.py
Outdated
| except subprocess.CalledProcessError as e: | ||
| cx_print(f"Command failed: {e}", "error") | ||
| except FileNotFoundError: | ||
| cx_print(f"Executable not found: {cmd[0]}", "error") |
There was a problem hiding this comment.
Inconsistent indentation detected. The 'except FileNotFoundError:' block uses 5 spaces instead of the standard 4 spaces for indentation, which breaks Python's indentation consistency.
| cx_print(f"Executable not found: {cmd[0]}", "error") | |
| cx_print(f"Executable not found: {cmd[0]}", "error") |
cortex/package_manager.py
Outdated
| self.check_backends() | ||
|
|
||
| backend = self._choose_backend("install") | ||
| if not backend: | ||
| return | ||
|
|
||
| cmd = self._get_install_cmd(backend, package) | ||
| self._run_cmd(cmd, dry_run) | ||
|
|
||
| def remove(self, package: str, dry_run: bool = False): | ||
| self.check_backends() | ||
|
|
||
| backend = self._choose_backend("remove") | ||
| if not backend: | ||
| return | ||
|
|
||
| cmd = self._get_remove_cmd(backend, package) |
There was a problem hiding this comment.
Code duplication in install and remove methods. Both methods have identical structure: check_backends, choose_backend, get command, run command. Consider extracting this common pattern into a single method like _execute_action(action_type, package, dry_run) to improve maintainability and reduce duplication.
| self.check_backends() | |
| backend = self._choose_backend("install") | |
| if not backend: | |
| return | |
| cmd = self._get_install_cmd(backend, package) | |
| self._run_cmd(cmd, dry_run) | |
| def remove(self, package: str, dry_run: bool = False): | |
| self.check_backends() | |
| backend = self._choose_backend("remove") | |
| if not backend: | |
| return | |
| cmd = self._get_remove_cmd(backend, package) | |
| self._execute_action("install", package, dry_run) | |
| def remove(self, package: str, dry_run: bool = False): | |
| self._execute_action("remove", package, dry_run) | |
| def _execute_action(self, action: str, package: str, dry_run: bool = False): | |
| """ | |
| Execute a package management action (e.g., install, remove) using the | |
| selected backend, honoring dry-run mode. | |
| """ | |
| self.check_backends() | |
| backend = self._choose_backend(action) | |
| if not backend: | |
| return | |
| if action == "install": | |
| cmd = self._get_install_cmd(backend, package) | |
| elif action == "remove": | |
| cmd = self._get_remove_cmd(backend, package) | |
| else: | |
| raise ValueError(f"Unsupported action: {action}") |
cortex/cli.py
Outdated
| pkg_remove.add_argument("--dry-run", action="store_true") | ||
|
|
||
| # pkg list | ||
| pkg_subs.add_parser("list", help="List packages") |
There was a problem hiding this comment.
The help text for the "list" subcommand is misleading. It says "List packages" but based on the implementation in package_manager.py (line 41-61), this command only shows backend support status, not actual installed packages. The help text should be updated to something like "Show backend status" or "Check Snap/Flatpak availability" to accurately reflect what the command does.
| pkg_subs.add_parser("list", help="List packages") | |
| pkg_subs.add_parser("list", help="Show Snap/Flatpak backend status") |
cortex/package_manager.py
Outdated
|
|
||
| cx_print(f"Running: {cmd_str}...", "info") | ||
| try: | ||
| subprocess.check_call(cmd) |
There was a problem hiding this comment.
Missing timeout parameter in subprocess.check_call. The codebase consistently uses timeout parameters in subprocess calls (as seen in coordinator.py line 182 and many other files) to prevent hanging on unresponsive commands. This is particularly important for package manager commands which may hang waiting for network or user input. Consider using subprocess.run with a timeout parameter instead of subprocess.check_call.
| if self.snap_avail and self.flatpak_avail: | ||
| return Prompt.ask( | ||
| f"Choose backend to {action}", | ||
| choices=["snap", "flatpak"], | ||
| default="snap" | ||
| ) |
There was a problem hiding this comment.
Interactive prompt in non-interactive contexts could cause hangs. The _choose_backend method uses Prompt.ask which blocks for user input. If this code is called in a non-interactive environment (CI/CD, scripts, etc.), it will hang indefinitely. Consider adding a non-interactive mode parameter or environment variable check to automatically select a default backend when stdin is not a TTY.
cortex/package_manager.py
Outdated
| import shutil | ||
| import subprocess | ||
| from typing import List, Optional | ||
| from rich.prompt import Confirm, Prompt |
There was a problem hiding this comment.
Unused import detected. The Confirm class is imported from rich.prompt but never used in the code. Consider removing it to keep imports clean, or if it was intended for future use (e.g., confirming package removal), add a TODO comment explaining its planned purpose.
| from rich.prompt import Confirm, Prompt | |
| from rich.prompt import Prompt |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
cortex/cli.py (4)
1524-1544: Handle missingpkg_actiongracefully.When a user runs
cortex pkgwithout a subcommand,pkg_actionisNone, causing the code to fall through to the else block with the misleading error "Unknown package manager action". Add an explicit check at the start (after line 1530) to handle this case clearly, similar to thenotifycommand pattern (lines 110-112) and theenvcommand pattern (lines 938-942).🔎 Suggested fix
def pkg(self, args: argparse.Namespace) -> int: """Handle package manager commands.""" try: from cortex.package_manager import UnifiedPackageManager mgr = UnifiedPackageManager() action = getattr(args, "pkg_action", None) + + if not action: + self._print_error("Please specify a subcommand (install/remove/list/storage/permissions)") + return 1 if action == "install": mgr.install(args.package, dry_run=args.dry_run)
1532-1545: Validate operation success before reporting success.The
install,remove,list_packages,storage_analysis, andcheck_permissionsmethods ofUnifiedPackageManagerdon't return values (they implicitly returnNone), but the handler always returns 0 for success. If these operations encounter errors handled internally (e.g., whencheck_backendsshows warnings or commands fail), the CLI will still report success. Consider having these methods return boolean success indicators or raising exceptions on failure.Verify whether the
UnifiedPackageManagermethods can fail silently:#!/bin/bash # Check the return types and error handling of UnifiedPackageManager methods ast-grep --pattern $'def install($$$): $$$' ast-grep --pattern $'def remove($$$): $$$' ast-grep --pattern $'def list_packages($$$): $$$'
1582-1582: Document the "apps" alias in help output.The "apps" alias for the "pkg" command is not documented in the help table. Line 1890 defines both "pkg" and "apps" as valid commands via
aliases=["apps"], but users won't discover the alias from the help output. Consider updating the help text to "pkg/apps" to match the pattern used elsewhere.🔎 Suggested fix
- table.add_row("pkg", "Manage Snap/Flatpak packages") + table.add_row("pkg/apps", "Manage Snap/Flatpak packages")
1905-1905: Clarify the "list" subcommand help text.The help text "List packages" is misleading. Based on the
UnifiedPackageManager.list_packages()implementation (package_manager.py lines 41-61), this command shows backend support status (whether Snap/Flatpak are available), not a list of installed packages. Update the help text to accurately reflect this behavior, e.g., "Show Snap/Flatpak backend status" or "Check backend availability".🔎 Suggested fix
- pkg_subs.add_parser("list", help="List packages") + pkg_subs.add_parser("list", help="Show Snap/Flatpak backend status")
🧹 Nitpick comments (1)
cortex/cli.py (1)
1546-1548: Narrow the exception handling scope.The broad
except Exceptioncatches all exceptions, which can mask programming errors and make debugging difficult. Consider catching specific exceptions (e.g.,ImportErrorfor the missing module,AttributeErrorfor incorrect args) or at least log the full traceback in verbose mode similar to other commands (e.g., lines 782-786).🔎 Suggested improvement
except Exception as e: self._print_error(f"Package Manager failed: {e}") + if self.verbose: + import traceback + traceback.print_exc() return 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/package_manager.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cortex/package_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/package_manager.py (6)
UnifiedPackageManager(8-184)install(27-42)remove(44-59)list_packages(61-82)storage_analysis(84-108)check_permissions(110-125)
🔇 Additional comments (1)
cortex/cli.py (1)
1960-1961: LGTM!The command routing correctly handles both the "pkg" command and its "apps" alias by routing them to the same handler method. This is consistent with how the parser is configured (line 1890).
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@murataslan1 This PR doesn’t follow the contributing.md guidelines yet.
Tests, documentation, a demo video, and the AI usage section in the PR description are missing.
Marking this as draft until those are added. Please update and ping once done.
|
@Anshgrover23 I have updated the PR with the required changes (Tests, Docs, Demo Video, and AI Disclosure). Ready for re-review. |
|
here's the video: https://drive.google.com/file/d/1H6ppgTK-bp03zGDpjPGQ7R5pzzti5Aiq/view?usp=sharing working on the lint fixes now, will push soon |
- Remove deprecated typing imports (List, Optional) - Use modern Python 3.10+ type annotations (list, X | None) - Fix import sorting (I001) - Remove trailing whitespace (W291, W293) - Clean up blank lines with whitespace
|
@Anshgrover23 I have fixed all lint errors in package_manager.py:
|
Resolved conflicts in cortex/cli.py by keeping both: - Our pkg command definitions (Snap/Flatpak package manager) - New env subcommands from main (audit, check, path management)
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@murataslan1 Kindly Address coderabbit comments and CI checks are failing as well.
| table.add_row("cache stats", "Show LLM cache statistics") | ||
| table.add_row("stack <name>", "Install the stack") | ||
| table.add_row("sandbox <cmd>", "Test packages in Docker sandbox") | ||
| table.add_row("pkg", "Manage Snap/Flatpak packages") |
|
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@murataslan1 Kindly resolve conflicts.
3 participants
Closes #450.
Uploading Screen Recording 2026-01-08 at 14.31.53.mov…
AI Usage
This PR uses AI-generated code for:
docs/modules/README_PACKAGE_MANAGER.md)CLA
I have read and signed the CLA.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.