Feat: unified package manager #632
Conversation
Addresses #450 - Snap/Flatpak Unified Manager Features: - Package source detection (deb/snap/flatpak) - Package comparison across formats - Permission management (snap interfaces, flatpak permissions) - Snap redirect detection and removal - Storage analysis by package format New CLI commands: - cortex pkg sources <package> - cortex pkg compare <package> - cortex pkg list [--format] - cortex pkg permissions <package> - cortex pkg storage - cortex pkg snap-redirects [--disable] Includes 30+ unit tests and documentation.
Addresses CodeRabbit feedback
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Piyushrathoree
requested review from
Anshgrover23 and
Suyashd999
as code owners
CLA Verification FailedThe following contributors have not signed the Contributor License Agreement:
How to Sign
Verified SignersThis check runs automatically. Maintainers can update |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR introduces a Unified Package Manager enabling cross-format package management (DEB, Snap, Flatpak) with CLI integration. It includes package source detection, cross-format comparison, installed package listing, permission management, snap redirect handling, and storage analysis across formats. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CortexCLI
participant UPM as UnifiedPackageManager
participant APT as apt (DEB)
participant Snap as snap (Snap)
participant Flatpak as flatpak (Flatpak)
User->>CLI: cortex pkg sources myapp
CLI->>UPM: detect_package_sources("myapp")
par DEB Detection
UPM->>APT: apt-cache policy myapp
APT-->>UPM: version, size metadata
and Snap Detection
UPM->>Snap: snap info myapp
Snap-->>UPM: version, size, description
and Flatpak Detection
UPM->>Flatpak: flatpak search myapp
Flatpak-->>UPM: version, ID, size
end
UPM-->>CLI: dict[format -> PackageInfo]
CLI->>User: Display sources per format
sequenceDiagram
participant User
participant CLI as CortexCLI
participant UPM as UnifiedPackageManager
participant Tools as System Tools
User->>CLI: cortex pkg permissions snapname
CLI->>UPM: list_snap_permissions("snapname")
UPM->>Tools: snap connections snapname
Tools-->>UPM: permission data
UPM-->>CLI: dict[interface -> list[permissions]]
CLI->>User: Display permissions by type
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested Reviewers
Poem
✨ Finishing touches
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 |
Summary of ChangesHello @Piyushrathoree, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature: a Unified Package Manager for Cortex Linux. This manager aims to provide users with greater transparency and control over their system's software by offering a consolidated interface to interact with packages from APT/DEB, Snap, and Flatpak ecosystems. It empowers users to understand package origins, compare installation options, manage sandboxed application permissions, and gain insights into storage consumption across different packaging technologies. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Piyushrathoree
changed the title
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-structured feature for unified package management across deb, snap, and flatpak. The implementation is comprehensive, covering package detection, comparison, permissions, and storage analysis. The code is generally clean and includes good documentation and extensive unit tests. I've identified a few areas for improvement, primarily concerning the accuracy of snap storage analysis, consistency in CLI command design for non-interactive use, and a gap in test coverage. Overall, this is a strong contribution that adds a lot of value.
| # Analyze Snap packages | ||
| if self._snap_available: | ||
| success, stdout, _ = self._run_command(["snap", "list", "--all"]) | ||
| if success: | ||
| # Get snap directory size | ||
| snap_dir = Path("/var/lib/snapd/snaps") | ||
| if snap_dir.exists(): | ||
| for snap_file in snap_dir.glob("*.snap"): | ||
| size = snap_file.stat().st_size | ||
| name = snap_file.stem.rsplit("_", 1)[0] | ||
| analysis.snap_total += size | ||
| analysis.snap_packages.append((name, size)) | ||
|
|
||
| analysis.snap_packages.sort(key=lambda x: x[1], reverse=True) | ||
| # Deduplicate (keep largest per name) and limit to top 10 | ||
| seen = set() | ||
| deduped = [] | ||
| for name, size in analysis.snap_packages: | ||
| if name not in seen: | ||
| seen.add(name) | ||
| deduped.append((name, size)) | ||
| analysis.snap_packages = deduped[:10] |
There was a problem hiding this comment.
The current implementation for analyzing snap storage is inefficient and provides inaccurate information. It scans the /var/lib/snapd/snaps directory and sums the sizes of the compressed .snap files, which does not represent the actual installed disk usage. The snap list --all command, which is already being executed, provides the installed size and should be parsed instead. This would be more efficient (avoiding filesystem I/O) and more accurate.
# Analyze Snap packages
if self._snap_available:
success, stdout, _ = self._run_command(["snap", "list", "--all"])
if success:
lines = stdout.strip().split("\n")
if len(lines) > 1 and "Size" in lines[0]:
header = lines[0]
name_end_pos = header.find("Version")
size_start_pos = header.find("Size")
if name_end_pos != -1 and size_start_pos != -1:
for line in lines[1:]:
if not line.strip():
continue
name = line[:name_end_pos].strip()
# Split to handle potential extra notes after size
size_str = line[size_start_pos:].strip().split()[0]
size = self._parse_size_string(size_str) if size_str != '-' else 0
analysis.snap_total += size
analysis.snap_packages.append((name, size))
analysis.snap_packages.sort(key=lambda x: x[1], reverse=True)
# Deduplicate (keep largest per name) and limit to top 10
seen = set()
deduped = []
for name, size in analysis.snap_packages:
if name not in seen:
seen.add(name)
deduped.append((name, size))
analysis.snap_packages = deduped[:10]|
|
||
| return 0 | ||
|
|
||
| def _pkg_snap_redirects(self, upm, args: argparse.Namespace) -> int: |
There was a problem hiding this comment.
The pkg snap-redirects --disable command requires interactive confirmation, which prevents its use in scripts. For consistency with other destructive commands like cortex remove, a --yes/-y flag should be added to bypass the prompt.
This would involve two changes:
- In
main(), add the--yesargument to thepkg_redirects_parser:pkg_redirects_parser.add_argument( "--yes", "-y", action="store_true", help="Skip confirmation prompt", )
- In
_pkg_snap_redirects, check for this flag to skip theinput()call:def _pkg_snap_redirects(self, upm, args: argparse.Namespace) -> int: disable = getattr(args, "disable", False) skip_confirm = getattr(args, "yes", False) # Add this if disable: # ... print warnings ... if not skip_confirm: # Add this check try: confirm = input(...) # ... except (EOFError, KeyboardInterrupt): # ...
| @patch.object(UnifiedPackageManager, "_list_deb_packages") | ||
| @patch.object(UnifiedPackageManager, "_list_flatpak_packages") | ||
| @patch.object(UnifiedPackageManager, "_run_command") | ||
| def test_analyze_storage(self, mock_run, mock_flatpak, mock_deb): |
There was a problem hiding this comment.
The test test_analyze_storage does not cover the snap storage analysis logic. It mocks pathlib.Path.exists to return False, which causes the snap analysis part of upm.analyze_storage() to be skipped entirely. This leaves the filesystem scanning and name parsing logic for snaps untested.
The test should be improved to cover this logic, for example by using unittest.mock.patch on Path.glob and Path.stat to simulate the filesystem structure, or by providing mock output for the snap list --all command if the implementation is changed to parse that.
There was a problem hiding this comment.
Pull request overview
This pull request implements a unified package manager for Cortex Linux that provides transparency and control over package sources across deb, snap, and flatpak formats. The feature addresses issue #450 by adding comprehensive package management capabilities including source detection, permission management, storage analysis, and snap redirect handling.
Changes:
- Added
UnifiedPackageManagerclass with full support for detecting, comparing, and managing packages across three formats - Implemented CLI commands under
cortex pkgnamespace for all package management operations - Added comprehensive test suite with 42 test cases covering all major functionality
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| cortex/unified_package_manager.py | Core implementation of unified package manager with detection, comparison, permission, and storage analysis features |
| tests/test_unified_package_manager.py | Comprehensive test suite with mocked subprocess calls testing all functionality |
| cortex/cli.py | CLI integration adding pkg subcommand with 6 sub-actions (sources, compare, list, permissions, storage, snap-redirects) |
| cortex/init.py | Updated module exports to include new package manager classes and types |
| docs/UNIFIED_PACKAGE_MANAGER.md | User-facing documentation with usage examples and technical details |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| console.print(f" [{format_name.upper()}] {status}") | ||
| console.print(f" Version: {info.version or 'N/A'}") | ||
| if info.description: | ||
| console.print(f" {info.description[:60]}...") |
There was a problem hiding this comment.
The description truncation on line 2883 uses a magic number 60 without explanation. This should be either defined as a named constant (e.g., MAX_DESCRIPTION_LENGTH) or documented with a comment explaining why this specific length was chosen.
|
|
||
| if success: | ||
| return True, f"Successfully {action}ed {interface} for {snap_name}" | ||
| else: |
There was a problem hiding this comment.
The error handling for snap permission modifications doesn't distinguish between permission denied errors and other failures. When snap operations require sudo, the error message should explicitly guide users to run the command with sudo privileges, similar to how disable_snap_redirects handles this case.
| else: | |
| else: | |
| # Detect common permission-related errors and guide the user to use sudo, | |
| # similar to how disable_snap_redirects handles this case. | |
| lower_stderr = stderr.lower() if isinstance(stderr, str) else str(stderr).lower() | |
| permission_indicators = ( | |
| "permission denied", | |
| "operation not permitted", | |
| "not permitted", | |
| "are you root", | |
| "need to be root", | |
| "must be root", | |
| "authentication error", | |
| ) | |
| if any(indicator in lower_stderr for indicator in permission_indicators): | |
| sudo_cmd = "sudo " + " ".join(cmd) | |
| return ( | |
| False, | |
| "Permission denied while trying to " | |
| f"{action} {interface} for {snap_name}. " | |
| "This operation may require elevated privileges. " | |
| f"Try running:\n {sudo_cmd}\n" | |
| f"Original error: {stderr}" | |
| ) |
| elif sources.get("snap") and sources["snap"].installed: | ||
| fmt = "snap" | ||
| else: | ||
| fmt = "flatpak" if "." in package and package.count(".") >= 2 else "snap" |
There was a problem hiding this comment.
The format detection logic assumes that package names with dots and at least 2 dots are flatpak apps (line 2961). However, this heuristic is not reliable - some snap packages can have dots in their names, and some flatpak app IDs might not follow the standard reverse-DNS naming convention. Consider checking the installed packages first to determine the format, which is already done, but the fallback logic should be more explicit about its limitations or return an error asking the user to specify the format explicitly.
| fmt = "flatpak" if "." in package and package.count(".") >= 2 else "snap" | |
| # Ambiguous or unknown format: avoid unreliable heuristics and require explicit format | |
| self._print_error( | |
| "Unable to determine package format automatically for " | |
| f"'{package}'. Please re-run with '--format flatpak' or " | |
| "'--format snap'." | |
| ) | |
| return 1 |
| python -m pytest tests/test_unified_package_manager.py -v | ||
| ``` | ||
|
|
||
| The test suite includes 30+ test cases covering all functionality with mocked subprocess calls. |
There was a problem hiding this comment.
The documentation states that this includes "30+ test cases" but there are actually 42 test methods defined in the test file. While this is not a critical issue, documentation should be accurate or use more general language like "comprehensive test suite" to avoid maintenance burden.
| The test suite includes 30+ test cases covering all functionality with mocked subprocess calls. | |
| The test suite is comprehensive and covers all functionality with mocked subprocess calls. |
| permissions[current_section] = {} | ||
|
|
||
| if isinstance(permissions[current_section], dict): | ||
| key, value = line.split("=", 1) | ||
| permissions[current_section][key] = value | ||
| elif current_section: | ||
| if current_section not in permissions: | ||
| permissions[current_section] = [] | ||
|
|
||
| if isinstance(permissions[current_section], list): | ||
| permissions[current_section].append(line) |
There was a problem hiding this comment.
The flatpak permission parsing logic assumes that within a section, lines are either all key=value pairs or all standalone values. However, this assumption is stated in the comment but not enforced or validated. If flatpak output changes or includes mixed formats, the parsing could silently fail or produce incorrect results. Consider adding validation or error handling for unexpected permission formats.
| permissions[current_section] = {} | |
| if isinstance(permissions[current_section], dict): | |
| key, value = line.split("=", 1) | |
| permissions[current_section][key] = value | |
| elif current_section: | |
| if current_section not in permissions: | |
| permissions[current_section] = [] | |
| if isinstance(permissions[current_section], list): | |
| permissions[current_section].append(line) | |
| # First entry for this section: treat it as a dict section | |
| permissions[current_section] = {} | |
| elif not isinstance(permissions[current_section], dict): | |
| # We already saw this section as a list-based section; mixed formats | |
| # indicate unexpected flatpak output, so fail fast. | |
| raise RuntimeError( | |
| f"Unexpected mixed permission format in section '{current_section}': " | |
| "found key=value entry after standalone values" | |
| ) | |
| key, value = line.split("=", 1) | |
| permissions[current_section][key] = value | |
| elif current_section: | |
| if current_section not in permissions: | |
| # First entry for this section: treat it as a list section | |
| permissions[current_section] = [] | |
| elif not isinstance(permissions[current_section], list): | |
| # We already saw this section as a dict-based section; mixed formats | |
| # indicate unexpected flatpak output, so fail fast. | |
| raise RuntimeError( | |
| f"Unexpected mixed permission format in section '{current_section}': " | |
| "found standalone value after key=value entries" | |
| ) | |
| permissions[current_section].append(line) |
| for line in stdout.split("\n"): | ||
| if line.startswith("stable:"): | ||
| version = line.split()[1] if len(line.split()) > 1 else "" | ||
| elif line.startswith("summary:"): | ||
| description = line.split(":", 1)[1].strip() |
There was a problem hiding this comment.
The check for "stable:" may not handle all snap channel types. Snaps can be installed from different channels (stable, candidate, beta, edge). When checking available versions, the code should handle cases where the stable channel might not exist or be the preferred channel.
| for line in stdout.split("\n"): | |
| if line.startswith("stable:"): | |
| version = line.split()[1] if len(line.split()) > 1 else "" | |
| elif line.startswith("summary:"): | |
| description = line.split(":", 1)[1].strip() | |
| # Prefer stable, then candidate, beta, edge if stable is not available | |
| preferred_channels = ["stable:", "candidate:", "beta:", "edge:"] | |
| channel_versions: dict[str, str] = {} | |
| for line in stdout.split("\n"): | |
| # Capture versions from known channels | |
| for channel_prefix in preferred_channels: | |
| if line.startswith(channel_prefix): | |
| parts = line.split() | |
| # Snap uses "-" when a channel has no release | |
| channel_version = parts[1] if len(parts) > 1 and parts[1] != "-" else "" | |
| channel_versions[channel_prefix] = channel_version | |
| break | |
| else: | |
| # Not a channel line; check for summary | |
| if line.startswith("summary:"): | |
| description = line.split(":", 1)[1].strip() | |
| # Select the best available channel version | |
| for channel_prefix in preferred_channels: | |
| candidate_version = channel_versions.get(channel_prefix, "") | |
| if candidate_version: | |
| version = candidate_version | |
| break |
| # Get snap directory size | ||
| snap_dir = Path("/var/lib/snapd/snaps") | ||
| if snap_dir.exists(): | ||
| for snap_file in snap_dir.glob("*.snap"): | ||
| size = snap_file.stat().st_size | ||
| name = snap_file.stem.rsplit("_", 1)[0] | ||
| analysis.snap_total += size | ||
| analysis.snap_packages.append((name, size)) |
There was a problem hiding this comment.
The snap storage analysis reads directly from /var/lib/snapd/snaps and counts all .snap files, which will count multiple revisions of the same snap package. While the code attempts to deduplicate by keeping only the largest version per name (lines 798-804), the snap_total will still include all revisions. This means the total storage calculation will be inflated compared to the actual disk usage. Consider using 'snap list --all' to get accurate revision information and only count non-disabled revisions, or clarify in documentation that this includes all revisions.
| # Get snap directory size | |
| snap_dir = Path("/var/lib/snapd/snaps") | |
| if snap_dir.exists(): | |
| for snap_file in snap_dir.glob("*.snap"): | |
| size = snap_file.stat().st_size | |
| name = snap_file.stem.rsplit("_", 1)[0] | |
| analysis.snap_total += size | |
| analysis.snap_packages.append((name, size)) | |
| # Determine active (non-disabled) snap revisions from `snap list --all` | |
| active_revisions = set() | |
| lines = stdout.strip().splitlines() | |
| if len(lines) > 1: | |
| # Skip header line | |
| for line in lines[1:]: | |
| parts = line.split() | |
| if len(parts) < 3: | |
| # Unexpected format; skip | |
| continue | |
| # Expected format (at least): name version rev [tracking publisher notes...] | |
| name = parts[0] | |
| # version = parts[1] # Unused | |
| rev = parts[2] | |
| notes = "" | |
| if len(parts) >= 6: | |
| # notes are in the remaining columns | |
| notes = " ".join(parts[5:]) | |
| if "disabled" not in notes.lower(): | |
| active_revisions.add((name, rev)) | |
| # Get snap directory size, counting only active revisions | |
| snap_dir = Path("/var/lib/snapd/snaps") | |
| if snap_dir.exists() and active_revisions: | |
| for snap_file in snap_dir.glob("*.snap"): | |
| stem_parts = snap_file.stem.rsplit("_", 1) | |
| if len(stem_parts) != 2: | |
| # Unexpected filename; skip | |
| continue | |
| snap_name, snap_rev = stem_parts | |
| if (snap_name, snap_rev) not in active_revisions: | |
| continue | |
| size = snap_file.stat().st_size | |
| analysis.snap_total += size | |
| analysis.snap_packages.append((snap_name, size)) |
| # Build command with optional slot specification | ||
| plug_spec = f"{snap_name}:{interface}" | ||
| if slot: | ||
| cmd = ["snap", action, plug_spec, slot] | ||
| else: | ||
| cmd = ["snap", action, plug_spec] |
There was a problem hiding this comment.
The permission modification function constructs command arguments in an unsafe way when the slot parameter is provided by user input. While snap commands do their own validation, this could be a security concern if special characters or command injection attempts are not properly handled. Consider adding input validation for the slot parameter to ensure it matches expected patterns.
| """ | ||
| if not self._flatpak_available: | ||
| return False, "Flatpak is not available on this system" | ||
|
|
There was a problem hiding this comment.
Similar to the snap permission modification, the flatpak override command constructs arguments using user-provided permission and value parameters without validation. This could potentially lead to command injection if special characters are not properly escaped. Add input validation to ensure permission and value parameters match expected patterns.
| # Validate permission and value to prevent malformed or unsafe arguments | |
| if not re.fullmatch(r"[A-Za-z0-9_-]+", permission): | |
| logger.warning("Invalid flatpak permission name: %r", permission) | |
| return False, "Invalid flatpak permission name" | |
| if not re.fullmatch(r"[\w./-]+", value): | |
| logger.warning("Invalid flatpak permission value: %r", value) | |
| return False, "Invalid flatpak permission value" |
| # backup exists, config doesn't exist | ||
| mock_exists.side_effect = lambda: True # backup exists | ||
|
|
||
| with patch("pathlib.Path.exists") as mock_path_exists: | ||
| # First call for backup_path.exists() = True, second for config_path.exists() = False | ||
| mock_path_exists.side_effect = [True, False] | ||
| mock_move.return_value = None | ||
|
|
||
| success, message = self.upm.restore_snap_redirects() | ||
|
|
||
| self.assertTrue(success) | ||
| self.assertIn("restored", message.lower()) | ||
| mock_move.assert_called_once() | ||
|
|
There was a problem hiding this comment.
The test for restore_snap_redirects_success uses a confusing mock setup. Line 415 sets mock_exists.side_effect to a lambda that always returns True, but then line 418 overrides it with mock_path_exists.side_effect. This makes the test hard to understand and maintain. Consider simplifying the mock setup to be more explicit about which paths exist at different points in the test.
| # backup exists, config doesn't exist | |
| mock_exists.side_effect = lambda: True # backup exists | |
| with patch("pathlib.Path.exists") as mock_path_exists: | |
| # First call for backup_path.exists() = True, second for config_path.exists() = False | |
| mock_path_exists.side_effect = [True, False] | |
| mock_move.return_value = None | |
| success, message = self.upm.restore_snap_redirects() | |
| self.assertTrue(success) | |
| self.assertIn("restored", message.lower()) | |
| mock_move.assert_called_once() | |
| # backup exists, then config doesn't exist | |
| # First call for backup_path.exists() = True, second for config_path.exists() = False | |
| mock_exists.side_effect = [True, False] | |
| mock_move.return_value = None | |
| success, message = self.upm.restore_snap_redirects() | |
| self.assertTrue(success) | |
| self.assertIn("restored", message.lower()) | |
| mock_move.assert_called_once() | |
1 participant
Related Issue
Closes #450
Summary
Features:
New CLI commands:
video.mp4
AI Disclosure
used Github Copilot(claude sonnet 4.5) for unit tests, docs and autocomplete suggestions.
Checklist
type(scope): descriptionor[scope] descriptionpytest tests/)Summary by CodeRabbit
Release Notes
New Features
pkgCLI command with subcommands:sources,compare,list,permissions,storage, andsnap-redirectsDocumentation
✏️ Tip: You can customize this high-level summary in your review settings.