feat: unified package manager #564
Conversation
Addresses cortexlinux#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
|
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 (2)
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. 📝 WalkthroughWalkthroughAdds a Unified Package Manager: new module Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as cortex CLI
participant UPM as UnifiedPackageManager
participant System as System Commands
User->>CLI: cortex pkg sources <package>
CLI->>UPM: detect_package_sources(package)
UPM->>System: apt/snap/flatpak queries
System-->>UPM: format-specific responses
UPM-->>CLI: dict(format → PackageInfo)
CLI-->>User: formatted output
sequenceDiagram
actor User
participant CLI as cortex CLI
participant UPM as UnifiedPackageManager
participant System as System Commands
User->>CLI: cortex pkg storage
CLI->>UPM: analyze_storage()
UPM->>System: dpkg-query / snap list / flatpak list
System-->>UPM: package lists & sizes
UPM->>UPM: aggregate & sort
UPM-->>CLI: StorageAnalysis
CLI->>UPM: format_storage_analysis()
CLI-->>User: storage summary
sequenceDiagram
actor User
participant CLI as cortex CLI
participant UPM as UnifiedPackageManager
participant APT as APT Cache
participant Config as /etc/apt/apt.conf.d
User->>CLI: cortex pkg snap-redirects --disable
CLI->>UPM: disable_snap_redirects(backup=true)
UPM->>APT: discover redirect packages
APT-->>UPM: redirect candidates
UPM->>Config: backup & modify 20snapd.conf
Config-->>UPM: backup/modify result
UPM-->>CLI: (success/failure, message)
CLI-->>User: status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 FailedThe following contributors have not signed the Contributor License Agreement:
How to Sign
Verified SignersThis check runs automatically. Maintainers can update |
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 significantly enhances the 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
force-pushed
the
feat/unified-package-manager
branch
from
774186a to
c37662c
Compare
![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 is a great addition, introducing a unified package manager that addresses a common pain point for users dealing with multiple package formats. The implementation is well-structured with a dedicated UnifiedPackageManager class, clear CLI commands, and good documentation. The unit tests are also comprehensive. I've found one high-severity bug in the Flatpak permission parsing and a couple of medium-severity items to improve code quality and test coverage. Overall, excellent work.
Piyushrathoree
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/unified_package_manager.py:
- Around line 207-219: The substring check `package_name in stdout` can yield
false positives; instead parse the snap list output lines (from the `stdout`
returned by `_run_command`) and compare the package name against the first
whitespace-separated column of each non-header line (e.g., split each line and
check `parts[0] == package_name`) or use a regex with word boundaries to match
the package name exactly; update the block that handles `success, stdout, _ =
self._run_command(["snap", "list", package_name])` to locate the matching line,
extract the version from that line, and only then construct and return the
`PackageInfo(name=..., format=PackageFormat.SNAP, version=..., installed=True)`.
🧹 Nitpick comments (10)
tests/test_unified_package_manager.py (3)
19-20: Consider using pytest's path configuration instead of sys.path manipulation.The
sys.path.insertis a common pattern but can be fragile. Consider configuring the project as an installable package or usingpyproject.toml's[tool.pytest.ini_options]withpythonpathsetting for cleaner test discovery.
452-468: Remove unused@patchdecorators.The
@patch.objectdecorators on these tests are unnecessary since the tests set_snap_available/_flatpak_availabletoFalsedirectly, causing early return before_run_commandwould be called. Themock_runparameter is never used.♻️ Suggested cleanup
- @patch.object(UnifiedPackageManager, "_run_command") - def test_list_snap_permissions_unavailable(self, mock_run): + def test_list_snap_permissions_unavailable(self): """Test listing permissions when snap unavailable.""" self.upm._snap_available = False result = self.upm.list_snap_permissions("test") self.assertIn("error", result) - @patch.object(UnifiedPackageManager, "_run_command") - def test_list_flatpak_permissions_unavailable(self, mock_run): + def test_list_flatpak_permissions_unavailable(self): """Test listing permissions when flatpak unavailable.""" self.upm._flatpak_available = False result = self.upm.list_flatpak_permissions("org.test.App") self.assertIn("error", result)
491-498: Consider adding assertion fordescriptiondefault.The test verifies most default values but omits
description. For completeness:♻️ Add missing assertion
self.assertEqual(info.version, "") self.assertEqual(info.size, 0) self.assertFalse(info.installed) + self.assertEqual(info.description, "") self.assertEqual(info.permissions, [])cortex/cli.py (2)
2004-2041: Consider adding error handling for UnifiedPackageManager initialization.If none of the package managers (apt, snap, flatpak) are available, the user should receive a helpful message rather than potentially confusing empty results.
♻️ Add initialization feedback
def pkg(self, args: argparse.Namespace) -> int: """Handle unified package manager commands for snap/flatpak/deb.""" from cortex.unified_package_manager import ( PackageFormat, UnifiedPackageManager, ) upm = UnifiedPackageManager() + + # Warn if no package managers are available + if not any([upm._apt_available, upm._snap_available, upm._flatpak_available]): + cx_print("No supported package managers found (apt, snap, flatpak)", "warning") + return 1 + action = getattr(args, "pkg_action", None)
2043-2063: Add type hint forupmparameter.Per coding guidelines, type hints are required in Python code.
♻️ Add type annotation
- def _pkg_sources(self, upm, args: argparse.Namespace) -> int: + def _pkg_sources(self, upm: "UnifiedPackageManager", args: argparse.Namespace) -> int:Note: Use string annotation to avoid circular import, or add the import at the top of the file within
TYPE_CHECKINGblock.cortex/unified_package_manager.py (5)
12-12: Remove unused import.The
jsonmodule is imported but never used in this file.♻️ Remove unused import
-import json
247-262: Consider stricter matching for installed flatpak detection.The case-insensitive substring match
package_name.lower() in line.lower()could match unrelated packages. Since this is for detecting installed packages (not search), consider exact matching on the app ID or a more precise check.
475-516: Return type inconsistency when unavailable.The method declares return type
dict[str, list[dict[str, str]]]but returns{"error": str}when snap is unavailable. Consider using a union type or raising an exception for consistency.♻️ Option 1: Update type hint
def list_snap_permissions(self, snap_name: str) -> dict[str, Any]:Or Option 2: Raise exception instead of returning error dict:
if not self._snap_available: raise RuntimeError("Snap is not available on this system")
747-768: Consider simplifying snap storage analysis.The
snap list --allcommand result isn't used - the code only checksif successbefore proceeding. Consider removing this check or using it to verify snap availability instead of re-checking filesystem:♻️ Simplified approach
# 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(): + # Get snap directory size + snap_dir = Path("/var/lib/snapd/snaps") + if snap_dir.exists():
813-823: Top DEB packages not displayed despite being collected.The method collects
deb_packagesand truncates to top 10 (line 745), but the output only shows top Snap and Flatpak packages. Consider adding DEB packages to the output or updating the analysis logic to not collect them if not displayed.♻️ Add DEB packages to output
+ if analysis.deb_packages: + lines.append("\nTop DEB Packages:") + for name, size in analysis.deb_packages[:5]: + lines.append(f" {name}: {format_size(size)}") + # Top packages per format if analysis.snap_packages: lines.append("\nTop Snap Packages:")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cortex/__init__.pycortex/cli.pycortex/unified_package_manager.pydocs/UNIFIED_PACKAGE_MANAGER.mdtests/test_unified_package_manager.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/__init__.pycortex/cli.pytests/test_unified_package_manager.pycortex/unified_package_manager.py
{setup.py,setup.cfg,pyproject.toml,**/__init__.py}
📄 CodeRabbit inference engine (AGENTS.md)
Use Python 3.10 or higher as the minimum supported version
Files:
cortex/__init__.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_unified_package_manager.py
🧬 Code graph analysis (2)
cortex/__init__.py (1)
cortex/unified_package_manager.py (4)
PackageFormat(26-31)PackageInfo(35-44)StorageAnalysis(48-56)UnifiedPackageManager(59-824)
tests/test_unified_package_manager.py (1)
cortex/unified_package_manager.py (22)
PackageFormat(26-31)PackageInfo(35-44)StorageAnalysis(48-56)UnifiedPackageManager(59-824)_check_deb_package(146-200)_check_snap_package(202-240)_check_flatpak_package(242-288)detect_package_sources(130-144)_list_deb_packages(319-347)_list_snap_packages(349-376)_list_flatpak_packages(378-408)list_installed_packages(294-317)compare_package_options(438-469)list_snap_permissions(475-516)list_flatpak_permissions(518-558)modify_snap_permission(560-602)modify_flatpak_permission(604-628)check_snap_redirects(634-668)disable_snap_redirects(670-700)analyze_storage(728-779)_parse_size_string(410-432)format_storage_analysis(781-824)
🔇 Additional comments (14)
docs/UNIFIED_PACKAGE_MANAGER.md (1)
1-116: Documentation is clear and comprehensive.The documentation provides a solid introduction to the Unified Package Manager, covering all CLI commands with helpful examples. The safety warning about
--disablemodifying system configuration is appropriately prominent.One minor note: The backup path is described as creating
20snapd.conf.disabled, but verify this matches the actual implementation (.with_suffix(".conf.disabled")in the code would produce20snapd.disabledrather than20snapd.conf.disabled).cortex/__init__.py (1)
4-22: LGTM!The new exports are correctly added following the existing pattern. The
__all__list properly exposes the new public API surface (UnifiedPackageManager,PackageFormat,PackageInfo,StorageAnalysis), maintaining consistency with the module structure.tests/test_unified_package_manager.py (2)
33-38: Good test fixture setup.Mocking
_check_command_availableensures tests are deterministic and don't depend on the actual system having snap/flatpak/apt installed.
392-412: Good integration test for storage analysis.This test effectively validates the storage calculation logic while properly isolating from filesystem access by mocking
Path.exists. The expected values correctly sum the mocked package sizes.cortex/cli.py (5)
2093-2118: LGTM!The package listing logic correctly handles format filtering and displays a reasonable subset (top 10) of packages per format to avoid overwhelming output.
2120-2158: Permissions detection heuristic is reasonable.The
"." in packagecheck for auto-detecting flatpak app IDs (likeorg.mozilla.firefox) is a pragmatic approach. Users can always use--formatto explicitly specify if needed.
2161-2198: LGTM!Storage analysis and snap redirect commands are cleanly implemented with appropriate user feedback.
2469-2512: LGTM!The argparse configuration is complete, with appropriate subcommands, arguments, and help text that align with the documentation and handler implementations.
2775-2776: LGTM!Routing correctly follows the existing CLI dispatch pattern.
cortex/unified_package_manager.py (5)
34-57: LGTM!Dataclass definitions correctly use
field(default_factory=list)for mutable default values, avoiding the classic Python mutable default argument pitfall.
95-124: LGTM!The command execution helper has solid error handling with timeouts, avoids
shell=Truefor security, and properly logs failures. The broad exception catch is acceptable here as a safety net since subprocess can raise various OS-level exceptions.
146-200: LGTM!Good detection of transitional/dummy packages that redirect to snap. The size conversion from KB to bytes is correct for dpkg-query's
Installed-Sizefield.
410-432: LGTM with optional enhancement note.The size parsing covers common units. Consider adding "TB" and binary prefixes (KiB, MiB, GiB) if flatpak ever reports sizes in those formats, but current implementation should handle typical outputs.
691-722: LGTM!The backup path handling is consistent between
disable_snap_redirects(line 693) andrestore_snap_redirects(line 709). Both correctly produce the same path20snapd.conf.disabled.
There was a problem hiding this comment.
Pull request overview
This PR implements a unified package manager feature to address user confusion around Snap, Flatpak, and DEB packages on Ubuntu/Debian systems. The implementation provides transparency about package sources and enables users to manage permissions and detect snap redirects.
Changes:
- Added
UnifiedPackageManagerclass with support for detecting, listing, and comparing packages across deb/snap/flatpak formats - Implemented CLI commands under
cortex pkgnamespace for package source detection, comparison, permission management, storage analysis, and snap redirect management - Added comprehensive unit tests with 30+ 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 with package detection, listing, comparison, permissions, and storage analysis functionality |
| cortex/cli.py | CLI integration adding cortex pkg command with subcommands for sources, compare, list, permissions, storage, and snap-redirects |
| cortex/init.py | Updated exports to include new UnifiedPackageManager classes and enums |
| tests/test_unified_package_manager.py | Comprehensive test suite with 30+ unit tests covering all functionality |
| docs/UNIFIED_PACKAGE_MANAGER.md | Documentation for the unified package manager feature with usage examples and technical details |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/unified_package_manager.py:
- Around line 541-561: The parsing loop is duplicated and the function never
returns the parsed data: remove the redundant second `for line in
stdout.strip().split("\n"):` so only one loop iterates `stdout` and ensure the
function returns the `permissions` object at the end; keep the existing logic
that sets `current_section`, creates dicts or lists for
`permissions[current_section]`, splits `key, value = line.split("=", 1)` when
appropriate, and append lines for list sections, then add a final `return
permissions`.
🧹 Nitpick comments (1)
cortex/unified_package_manager.py (1)
475-486: Return type annotation mismatch for error case.The return type
dict[str, list[dict[str, str]]]doesn't match the error return{"error": "..."}which isdict[str, str]. Consider using a union type or returning a structured error.♻️ Suggested fix using union type
- def list_snap_permissions(self, snap_name: str) -> dict[str, list[dict[str, str]]]: + def list_snap_permissions(self, snap_name: str) -> dict[str, list[dict[str, str]] | str]:Alternatively, raise an exception for the error case to maintain the cleaner return type.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/unified_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/unified_package_manager.py
🧬 Code graph analysis (1)
cortex/unified_package_manager.py (5)
cortex/logging_system.py (2)
warning(204-206)info(200-202)cortex/sandbox/sandbox_executor.py (1)
success(74-76)cortex/graceful_degradation.py (1)
match(280-313)cortex/cli.py (1)
pkg(2005-2041)cortex/semantic_cache.py (1)
total(32-34)
🔇 Additional comments (9)
cortex/unified_package_manager.py (9)
1-24: LGTM!Module docstring is clear, imports are appropriate, and the logger is correctly initialized at module level.
26-56: LGTM!The enum and dataclasses are well-structured with proper docstrings, type hints, and correct use of
field(default_factory=list)for mutable defaults.
59-124: LGTM!The class is well-documented and the helper methods are implemented safely:
_run_commandcorrectly avoids shell injection by using list arguments.- Proper exception handling for timeout, file not found, and generic errors.
- Reasonable 30-second timeout default.
130-288: LGTM!Package source detection is well-implemented:
- Proper command availability checks before execution.
- Good handling of transitional/dummy packages for snap redirect detection.
- Flatpak app ID validation with "." check prevents false positives.
- Consistent return of
Nonewhen packages aren't found.
294-432: LGTM!Package listing methods are robust with proper:
- Format filtering support.
- Header line skipping for tabular output parsing.
- Flexible size string parsing supporting multiple unit formats.
- Availability checks preventing unnecessary command failures.
438-469: LGTM!The comparison logic is clean. Note that if a package is installed in multiple formats (uncommon but possible),
installed_aswill reflect only the last one encountered due to the loop order (flatpak). This is likely acceptable given the use case.
562-604: LGTM!The
modify_snap_permissionmethod has good validation for the action parameter and properly constructs the command with optional slot specification. The docstring examples are helpful.
636-724: LGTM!Snap redirect management is well-implemented with:
- Permission check before attempting modification.
- Backup option for safe rollback.
- Clear warning in docstring about requiring root.
Minor note: Line 711 uses string concatenation (
self.SNAP_REDIRECT_CONFIG + ".disabled") while line 695 usesPath.with_suffix(). Consider usingPath(self.SNAP_REDIRECT_CONFIG).with_suffix(".conf.disabled")for consistency.
730-826: LGTM!Storage analysis is well-implemented:
- Proper deduplication for snap packages (keeping largest per name).
- Consistent top-10 limiting across formats.
- Clean
format_sizehelper for human-readable output.- Good separation between data collection (
analyze_storage) and presentation (format_storage_analysis).
|
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
Piyushrathoree
changed the title
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@Piyushrathoree Kindly complete your CLA verification by creating a new PR different from this one following the pattern of this PR #401 .
Follow the contributing.md guidelines( i.e. add demonstrating video in pr description, add Documentation for the feature you're creating, add tests and AI Usage as well, thanks.
Also, kindly address CODERABBITAI comments.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @cortex/unified_package_manager.py:
- Around line 491-492: The method list_snap_permissions returns {"error": "..."}
when self._snap_available is false but is annotated to return dict[str,
list[dict[str, str]]], causing a type mismatch; update the implementation to
either raise a specific exception (e.g., raise RuntimeError or a custom
SnapUnavailableError) when not available or adjust the return annotation to a
union that includes an error shape (e.g., dict[str, list[dict[str, str]]] |
dict[str, str] or use typing.Union), and ensure all callers of
list_snap_permissions handle the chosen error path accordingly.
- Around line 524-567: The function list_flatpak_permissions builds a
permissions dict but never returns it; add a final "return permissions" at the
end of list_flatpak_permissions so the parsed permissions are returned, and also
remove the duplicated "for line in stdout.strip().split("\n"):" loop line in the
body to avoid a syntax/logic error; keep the existing variables and structure
(permissions, current_section, stdout) unchanged.
- Around line 547-548: There is a duplicated loop header "for line in
stdout.strip().split("\n")" causing an unintended nested iteration; in the
function in unified_package_manager.py where stdout is parsed, remove the
duplicate line so only a single for loop iterates over
stdout.strip().split("\n"), preserving the body that processes each line
(references: the duplicated "for line in stdout.strip().split(\"\\n\")"
statements).
🧹 Nitpick comments (3)
cortex/unified_package_manager.py (3)
116-123: Consider using lazy logging format strings.Using f-strings in logging calls evaluates the string regardless of whether the log level is enabled. This is a minor performance consideration.
♻️ Suggested fix
except subprocess.TimeoutExpired: - logger.warning(f"Command timed out: {' '.join(cmd)}") + logger.warning("Command timed out: %s", ' '.join(cmd)) return False, "", "Command timed out" except FileNotFoundError: - logger.warning(f"Command not found: {cmd[0]}") + logger.warning("Command not found: %s", cmd[0]) return False, "", f"Command not found: {cmd[0]}" except Exception as e: - logger.error(f"Error running command {cmd}: {e}") + logger.error("Error running command %s: %s", cmd, e) return False, "", str(e)
416-439: Size unit parsing could be more robust.The multipliers include unusual variants like
mBandgB. Consider adding more standard units likeM,G,MiB,GiBfor broader compatibility.♻️ Suggested enhancement
multipliers = { "B": 1, "KB": 1024, + "K": 1024, "MB": 1024**2, + "M": 1024**2, + "MiB": 1024**2, "GB": 1024**3, + "G": 1024**3, + "GiB": 1024**3, "kB": 1024, - "mB": 1024**2, - "gB": 1024**3, }
713-734: Inconsistent backup path construction betweendisable_snap_redirectsandrestore_snap_redirects.Line 702 uses
config_path.with_suffix(".conf.disabled")while line 720 uses string concatenationSNAP_REDIRECT_CONFIG + ".disabled". These happen to produce the same result for this specific filename, but the inconsistency could cause issues if the config path changes.♻️ Suggested fix for consistency
def restore_snap_redirects(self) -> tuple[bool, str]: ... - backup_path = Path(self.SNAP_REDIRECT_CONFIG + ".disabled") config_path = Path(self.SNAP_REDIRECT_CONFIG) + backup_path = config_path.with_suffix(".conf.disabled")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/unified_package_manager.pytests/test_unified_package_manager.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_unified_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/unified_package_manager.py
🧬 Code graph analysis (1)
cortex/unified_package_manager.py (5)
cortex/logging_system.py (2)
warning(204-206)info(200-202)cortex/sandbox/sandbox_executor.py (1)
success(74-76)cortex/graceful_degradation.py (1)
match(280-313)cortex/cli.py (1)
pkg(2005-2041)cortex/semantic_cache.py (1)
total(32-34)
🔇 Additional comments (14)
cortex/unified_package_manager.py (14)
1-23: LGTM!Imports are well-organized and appropriate for the module's functionality. Standard library usage is correct.
25-31: LGTM!Clean enum definition with appropriate docstring.
33-44: LGTM!Well-structured dataclass with proper use of
field(default_factory=list)for the mutablepermissionsfield.
46-56: LGTM!Properly defined dataclass for storage analysis with correct mutable default handling.
145-199: LGTM!The DEB package detection logic correctly handles both installed packages (via
dpkg-query) and available packages (viaapt-cache show), with appropriate transitional package detection.
201-244: LGTM!Snap package detection correctly distinguishes between installed packages (
snap list) and available packages (snap info).
246-295: LGTM!Flatpak package detection with case-insensitive matching and proper validation of app IDs (checking for
.in the ID).
300-324: LGTM!Clean implementation of installed package listing with proper format filtering.
444-476: LGTM with minor edge case.Clean comparison logic. Note that if a package is installed in multiple formats (rare but possible),
installed_aswill reflect only the last format checked (flatpak), since it gets overwritten in the loop.
568-611: LGTM!The
modify_snap_permissionmethod has good documentation, validates the action parameter, and properly constructs the snap command with optional slot specification.
612-638: LGTM!The
modify_flatpak_permissionmethod correctly usesflatpak override --userfor permission modifications.
700-711: LGTM!Good safety checks before modifying system configuration, including permission verification and backup option.
739-791: LGTM!Good implementation of storage analysis with proper deduplication for snaps (handling multiple versions) and limiting to top 10 packages per format.
792-835: LGTM!Clean formatting method with a well-implemented nested
format_sizehelper function.
sure will do that soon , hopefully complete everything today |
Piyushrathoree
force-pushed
the
feat/unified-package-manager
branch
from
c578ff5 to
abc067c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 2171-2182: The disable branch of _pkg_snap_redirects currently
performs a privileged modification when args.disable is true without asking the
user to confirm privilege escalation; add an explicit confirmation prompt (reuse
the codebase's existing confirm/confirmation helper or implement an interactive
yes/no prompt) immediately before calling upm.disable_snap_redirects(), and
abort (return non-zero) if the user declines so that sudo is never invoked
silently; ensure the prompt text clearly states that
/etc/apt/apt.conf.d/20snapd.conf will be modified and that elevated privileges
are required.
In @cortex/unified_package_manager.py:
- Around line 59-89: The CLI call site _pkg_snap_redirects should record an
audit entry to InstallationHistory whenever it invokes
UnifiedPackageManager.disable_snap_redirects; wrap the call so you capture
success/failure and write a record describing the operation type
("disable_snap_redirects") and the result (success or error message) to the
existing history store (used for ~/.cortex/history.db) using the
InstallationHistory API; ensure errors thrown by disable_snap_redirects are
caught, logged to InstallationHistory with the error details, and re-raised or
surfaced to the user as appropriate.
🧹 Nitpick comments (4)
cortex/cli.py (2)
2043-2063: Add type hint forupmparameter.For consistency with the codebase and per coding guidelines requiring type hints in Python code:
- def _pkg_sources(self, upm, args: argparse.Namespace) -> int: + def _pkg_sources(self, upm: "UnifiedPackageManager", args: argparse.Namespace) -> int:You'll need to add a forward reference import or use
TYPE_CHECKINGblock.
2127-2142: Improve format detection logic for permissions.The current heuristic
"." in packageto detect flatpak may misclassify snap packages with dots in their names. The--formatargument is already available but only partially used:- if fmt == "flatpak" or "." in package: + if fmt == "flatpak" or (fmt is None and "." in package): # Likely a flatpak app ID perms = upm.list_flatpak_permissions(package)Consider also checking if the package exists as snap/flatpak first when format is not specified, to avoid false positives.
cortex/unified_package_manager.py (2)
150-164: Consider handling edge cases in dpkg-query output parsing.The pipe-separated output parsing could fail if package version contains
|character (unlikely but possible in some edge cases). The size parsing is correct for typical cases.- parts = stdout.strip().split("|") + parts = stdout.strip().split("|", 2) # Limit splits to handle edge casesThis is a minor edge case but worth noting.
476-517: Inconsistent error handling between snap and flatpak permission methods.
list_snap_permissionsraisesRuntimeErrorwhen snap is unavailable, whilelist_flatpak_permissionsreturns an error dict. This creates inconsistency in the API and requires different handling in the CLI:# Snap - raises exception def list_snap_permissions(self, snap_name: str) -> dict[str, list[dict[str, str]]]: if not self._snap_available: raise RuntimeError("Snap is not available on this system") # Line 487 # Flatpak - returns error dict def list_flatpak_permissions(self, app_id: str) -> dict[str, Any]: if not self._flatpak_available: return {"error": "Flatpak is not available on this system"} # Line 530Consider making both methods consistent - either both raise exceptions or both return error dicts.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cortex/__init__.pycortex/cli.pycortex/unified_package_manager.pydocs/UNIFIED_PACKAGE_MANAGER.mdtests/test_unified_package_manager.py
🚧 Files skipped from review as they are similar to previous changes (2)
- cortex/init.py
- tests/test_unified_package_manager.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/cli.pycortex/unified_package_manager.py
🧬 Code graph analysis (2)
cortex/cli.py (1)
cortex/unified_package_manager.py (11)
PackageFormat(26-31)UnifiedPackageManager(59-829)detect_package_sources(128-142)compare_package_options(439-470)list_installed_packages(295-318)list_flatpak_permissions(519-557)list_snap_permissions(476-517)analyze_storage(733-784)format_storage_analysis(786-829)disable_snap_redirects(675-705)check_snap_redirects(633-673)
cortex/unified_package_manager.py (1)
cortex/cli.py (1)
pkg(2005-2041)
⏰ 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). (4)
- GitHub Check: Test (Python 3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (11)
docs/UNIFIED_PACKAGE_MANAGER.md (1)
1-116: Documentation looks comprehensive and well-structured.The documentation covers all key features, provides clear usage examples, and includes appropriate safety warnings for the
--disableoption. The explicitsudousage in the example aligns with the coding guidelines requiring explicit user confirmation for privilege escalation.cortex/cli.py (3)
2004-2041: LGTM - Clean command routing structure.The
pkg()method follows the established CLI pattern with lazy imports and action routing. The help text when no action is provided is helpful for discoverability.
2469-2510: Parser definitions are well-structured.The subparser setup is complete with appropriate help text and argument definitions. The
--disableflag correctly notes that sudo is required.
2773-2774: Routing integration looks correct.The pkg command routing follows the established pattern used by other commands.
cortex/unified_package_manager.py (7)
1-24: Module structure and imports are well-organized.Good use of type hints and clean module docstring. The imports are appropriately organized.
34-56: Dataclass definitions are correct and type-safe.Good use of
field(default_factory=list)for mutable default values, avoiding the common Python mutable default argument pitfall.
95-122: Command execution is reasonably safe.The implementation correctly avoids
shell=Trueand includes timeout handling. Since this executes trusted system package manager commands (apt,dpkg,snap,flatpak), Firejail sandboxing is not required per the coding guidelines (which specify sandboxing for "untrusted code").
559-601: LGTM - Permission modification API is well-designed.The method correctly validates the action and constructs the snap command. The docstring with examples is helpful. Confirmation for privileged operations should be handled at the CLI layer.
675-705: System modification method has proper safeguards.Good implementation with:
- Write permission check before attempting modification
- Backup creation by default
- Clear error messages for permission issues
The method correctly defers to the caller (CLI) for user confirmation, which aligns with separation of concerns.
752-773: Good snap storage analysis with deduplication.The snap storage analysis correctly handles multiple snap revisions by keeping the largest size per package name. This approach of reading actual file sizes from
/var/lib/snapd/snapsis more accurate than parsingsnap listoutput.
786-828: Clean formatting implementation.The
format_storage_analysismethod produces clear, readable output. The nestedformat_sizehelper is appropriate for this context.
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@Piyushrathoree Kindly address bot comments, do not resolve them only.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
@Anshgrover23 I have addressed every comment by reviewer please take a look , and this cla checks are failing because of gemini bot is not verified. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
cortex/unified_package_manager.py (2)
84-122: Missing audit logging for operations.Per coding guidelines, all operations should be logged to
~/.cortex/history.dbfor audit purposes. The_run_commandmethod executes system commands but doesn't log them. Consider adding audit logging for traceability.Based on learnings, operations should be logged to
~/.cortex/history.db.♻️ Suggested approach for audit logging
def _run_command(self, cmd: list[str], timeout: int = 30) -> tuple[bool, str, str]: """ Run a shell command and return success status, stdout, stderr. ... """ # Log command execution for audit logger.info(f"Executing command: {' '.join(cmd)}") try: result = subprocess.run( cmd, capture_output=True, text=True, timeout=timeout, ) # Consider integrating with InstallationHistory for persistent audit return result.returncode == 0, result.stdout, result.stderr # ... rest of exception handlingFor persistent audit logging, consider passing an optional
InstallationHistoryinstance or using a dedicated audit logger that writes to~/.cortex/history.db.
421-444: Consider handling additional size units.The size parser handles common units but might encounter others like "KiB", "MiB", "GiB" (IEC units) from some tools. The current fallback to
1(bytes) is safe, so this is optional.♻️ Optional: Add IEC binary units
multipliers = { "B": 1, "KB": 1024, "MB": 1024**2, "GB": 1024**3, "kB": 1024, "mB": 1024**2, "gB": 1024**3, + "KiB": 1024, + "MiB": 1024**2, + "GiB": 1024**3, }tests/test_unified_package_manager.py (2)
19-21: Avoidsys.path.insertmanipulation in tests.The
sys.path.insert(0, ...)pattern is fragile and can cause issues in different execution contexts. Consider using proper package installation (pip install -e .) or pytest'spythonpathconfiguration instead.♻️ Alternative approaches
- Use
pyproject.tomlorsetup.pywithpip install -e .for development- Configure
pytest.iniorpyproject.tomlwith:[tool.pytest.ini_options] pythonpath = ["."]
- Use relative imports if tests are part of the package
396-413: Mock setup has redundant pattern in restore test.Line 401 uses a lambda that always returns
True, but it's immediately overwritten by the nestedpatchon line 403. The outer mock at line 401 is effectively unused. This works but is confusing.♻️ Cleaner test structure
- @patch("pathlib.Path.exists") @patch("shutil.move") - def test_restore_snap_redirects_success(self, mock_move, mock_exists): + def test_restore_snap_redirects_success(self, mock_move): """Test successful restore of snap redirects from backup.""" - # 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()cortex/cli.py (1)
2296-2305: Format auto-detection heuristic is reasonable but could be improved.The fallback heuristic (line 2304) uses dot count to distinguish flatpak IDs (
org.mozilla.firefox) from snap names. This works for most cases but could misidentify packages with dots in their names.♻️ Consider more robust detection
# Current heuristic: fmt = "flatpak" if "." in package and package.count(".") >= 2 else "snap" # Alternative: Check if it looks like a reverse-DNS ID pattern import re flatpak_pattern = re.compile(r'^[a-zA-Z][a-zA-Z0-9_]*(\.[a-zA-Z][a-zA-Z0-9_]*)+$') fmt = "flatpak" if flatpak_pattern.match(package) else "snap"However, the current approach is acceptable for most real-world usage patterns.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cortex/cli.pycortex/unified_package_manager.pytests/test_unified_package_manager.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
tests/test_unified_package_manager.pycortex/unified_package_manager.pycortex/cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain test coverage above 80% for pull requests
Files:
tests/test_unified_package_manager.py
🧠 Learnings (2)
📚 Learning: 2026-01-12T20:51:13.817Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.817Z
Learning: Applies to **/*.py : Log all operations to ~/.cortex/history.db for audit purposes
Applied to files:
cortex/unified_package_manager.py
📚 Learning: 2026-01-12T20:51:13.817Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.817Z
Learning: Applies to **/*.py : Do not use silent sudo - require explicit user confirmation for privilege escalation
Applied to files:
cortex/cli.py
🧬 Code graph analysis (2)
tests/test_unified_package_manager.py (1)
cortex/unified_package_manager.py (23)
PackageFormat(25-30)PackageInfo(34-43)StorageAnalysis(47-55)UnifiedPackageManager(58-859)_check_deb_package(143-197)_check_snap_package(199-240)_check_flatpak_package(242-299)detect_package_sources(127-141)_list_deb_packages(330-358)_list_snap_packages(360-387)_list_flatpak_packages(389-419)list_installed_packages(305-328)compare_package_options(449-480)list_snap_permissions(486-527)list_flatpak_permissions(529-576)modify_snap_permission(578-620)modify_flatpak_permission(622-647)check_snap_redirects(653-693)disable_snap_redirects(695-730)restore_snap_redirects(732-752)analyze_storage(758-809)_parse_size_string(421-443)format_storage_analysis(811-859)
cortex/unified_package_manager.py (1)
cortex/cli.py (1)
pkg(2173-2209)
🔇 Additional comments (16)
cortex/unified_package_manager.py (5)
1-23: LGTM - Module setup follows best practices.The module docstring clearly explains the purpose, imports are well-organized, and the logger is properly configured at module level.
25-56: LGTM - Data models are well-designed.Good use of
@dataclasswithfield(default_factory=list)for mutable defaults, proper type hints, and clear docstrings.
143-197: LGTM - DEB package detection is robust.Good handling of both installed packages (via
dpkg-query) and available packages (viaapt-cache), with proper filtering of transitional/dummy packages that redirect to snap.
695-731: LGTM - Snap redirect management has appropriate safeguards.Good permission checks before modification, backup functionality with
.disabledsuffix, and clear error messages directing users to run with sudo. The CLI layer (_pkg_snap_redirects) properly handles user confirmation before calling this method.
778-799: LGTM - Snap storage analysis handles revisions correctly.Good approach to analyze snap storage by reading
.snapfiles directly from/var/lib/snapd/snaps/, with proper deduplication to keep only the largest revision per package name.tests/test_unified_package_manager.py (5)
30-39: LGTM - Test setup properly mocks command availability.Good use of
patch.objectto mock_check_command_availableduringsetUp, ensuring tests run regardless of system package managers installed.
165-238: LGTM - Package listing tests cover all formats.Good test coverage for DEB, Snap, and Flatpak listing, including format filtering. Mock outputs match expected command output formats.
279-302: LGTM - Flatpak permission parsing test is comprehensive.Excellent test case covering the complex permission format with both key=value sections (Context, Session Bus Policy) and list sections (filesystems). Validates proper data structure types.
461-467: LGTM - Size parsing tests cover key cases.Good coverage of different size units (GB, MB, KB) and edge case (empty string). Consider adding a test for malformed input like
"abc"to verify the fallback behavior returns 0.
520-575: LGTM - Dataclass tests verify creation and defaults.Good coverage of both explicit initialization and default values for
PackageInfoandStorageAnalysisdataclasses.cortex/cli.py (6)
35-35: LGTM - TYPE_CHECKING import is correct.Proper use of
TYPE_CHECKINGfor type hints without runtime import overhead.
2172-2209: LGTM - Command dispatcher is well-organized.Good lazy import pattern for startup performance, clear help text, and clean routing to subcommand handlers.
2211-2259: LGTM - Package source and comparison display is clear.Good use of Rich console for formatted output with color-coded status indicators. Proper error handling when package is not found.
2351-2402: Excellent implementation of confirmation and audit logging.This handler properly follows coding guidelines:
- Shows clear warning about system modification (lines 2357-2360)
- Requires explicit user confirmation (lines 2362-2370)
- Logs operation to
InstallationHistoryfor audit (lines 2377-2402)Based on learnings, this correctly implements "no silent sudo" and audit logging requirements.
2713-2754: LGTM - CLI parser is well-structured.Comprehensive subcommand setup with appropriate arguments and clear help text. The
--disableflag help text correctly notes the sudo requirement.
3018-3019: LGTM - Command routing follows existing patterns.Consistent with other command routing in
main().
Piyushrathoree
force-pushed
the
feat/unified-package-manager
branch
from
1afdd70 to
9716d33
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cortex/cli.py:
- Around line 2351-2418: The except block in _pkg_snap_redirects may reference
install_id before it's set if InstallationHistory.record_installation raises;
initialize install_id = None before the try that calls
InstallationHistory.record_installation (or set it immediately after a
successful call) and in the outer except only call
history.update_installation(install_id, ...) when install_id is not None (or use
a safe conditional/log fallback), and ensure any error logging
(self._print_error(...)) still runs when install_id is unset.
🧹 Nitpick comments (4)
cortex/unified_package_manager.py (2)
421-443: Consider adding more size unit variants.The parser handles common formats but could miss some valid outputs:
🔧 Extend size unit support
multipliers = { "B": 1, "KB": 1024, "MB": 1024**2, "GB": 1024**3, "kB": 1024, "mB": 1024**2, "gB": 1024**3, + "K": 1024, + "M": 1024**2, + "G": 1024**3, + "KiB": 1024, + "MiB": 1024**2, + "GiB": 1024**3, }
701-736: Missing audit logging for system configuration change.Per coding guidelines, operations should be logged to
~/.cortex/history.dbfor audit purposes. This method modifies system configuration but doesn't log the operation itself. The CLI layer does log it (viaInstallationHistory), but consider adding logging here for direct API usage.📝 Add logging for audit trail
+ def disable_snap_redirects(self, backup: bool = True) -> tuple[bool, str]: """ Disable snap redirects by moving the APT config file. WARNING: This modifies system configuration and requires root. + + Note: Callers should log this operation to ~/.cortex/history.db + for audit purposes. ...Or add internal logging:
try: if backup: backup_path = config_path.with_suffix(".conf.disabled") if backup_path.exists(): return ( True, f"Snap redirects already disabled. Existing backup preserved at: {backup_path}", ) shutil.move(str(config_path), str(backup_path)) + logger.info(f"Disabled snap redirects, backup at {backup_path}") return True, f"Snap redirects disabled. Backup saved to: {backup_path}"Based on learnings: "Log all operations to ~/.cortex/history.db for audit purposes"
cortex/cli.py (2)
2233-2259: Add type hint forupmparameter.The method signature is missing the type hint for consistency with the codebase.
📝 Add type hint
- def _pkg_compare(self, upm, args: argparse.Namespace) -> int: + def _pkg_compare(self, upm: "UnifiedPackageManager", args: argparse.Namespace) -> int:
2261-2286: Duplicate import of PackageFormat.
PackageFormatis already imported in thepkg()method (line 2176). Consider passing it as a parameter or using a class-level TYPE_CHECKING import to avoid redundancy.📝 Avoid duplicate import
Option 1: Pass PackageFormat to helper methods
- def _pkg_list(self, upm, args: argparse.Namespace) -> int: + def _pkg_list(self, upm: "UnifiedPackageManager", args: argparse.Namespace, PackageFormat: type) -> int: """List installed packages by format.""" - from cortex.unified_package_manager import PackageFormatOption 2: Import once at module level under TYPE_CHECKING
if TYPE_CHECKING: from cortex.shell_env_analyzer import ShellEnvironmentAnalyzer from cortex.unified_package_manager import UnifiedPackageManager + from cortex.unified_package_manager import PackageFormat
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/unified_package_manager.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/unified_package_manager.pycortex/cli.py
🧠 Learnings (2)
📚 Learning: 2026-01-12T20:51:13.817Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.817Z
Learning: Applies to **/*.py : Log all operations to ~/.cortex/history.db for audit purposes
Applied to files:
cortex/unified_package_manager.py
📚 Learning: 2026-01-12T20:51:13.817Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-12T20:51:13.817Z
Learning: Applies to **/*.py : Do not use silent sudo - require explicit user confirmation for privilege escalation
Applied to files:
cortex/unified_package_manager.pycortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (1)
cortex/unified_package_manager.py (8)
PackageFormat(25-30)detect_package_sources(127-141)compare_package_options(449-480)list_installed_packages(305-328)list_snap_permissions(486-527)analyze_storage(764-815)format_storage_analysis(817-865)disable_snap_redirects(701-736)
⏰ 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). (4)
- GitHub Check: Build Package
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🔇 Additional comments (24)
cortex/unified_package_manager.py (17)
1-23: Module structure and imports look good.The module has a clear docstring, appropriate imports, and proper logger setup. Type hints are used throughout.
25-56: Data structures are well-designed.The dataclasses use proper type hints, docstrings, and correctly avoid mutable default arguments with
field(default_factory=list).
58-92: Class initialization is defensive and well-documented.Good use of
shutil.which()for command availability checks. The class docstring clearly explains the purpose and capabilities.
94-121: Command execution is secure and robust.Good security practices:
- Uses list arguments (no shell injection)
- No
shell=True- Proper timeout handling
- Comprehensive exception handling with logging
143-197: DEB package detection logic is correct.Good handling of transitional/dummy packages to detect snap redirects. The size calculation correctly interprets
dpkg-query's Installed-Size as KB.
199-240: Snap package detection is well-implemented.The parsing logic for
snap listandsnap infooutput handles common formats correctly.
242-299: Flatpak package detection is thoughtfully implemented.Good user experience with basename matching (e.g., "firefox" finds "org.mozilla.firefox"). The docstring correctly notes that only the first match is returned.
305-419: Package listing methods are well-structured.The format filter works correctly. Note that snap package sizes aren't captured from
snap listoutput (comment at line 377 mentions this but implementation doesn't include it), but this is a minor limitation since snap sizes require additional commands to retrieve accurately.
449-480: Package comparison logic is clean and correct.Good composition using
detect_package_sourcesand building a well-structured comparison result.
486-527: Snap permissions listing is correctly implemented.The parsing of
snap connectionsoutput is accurate. Minor redundancy at line 513 (checkinglen(parts) > 3after already requiringlen(parts) >= 4), but it's harmless.
529-576: Flatpak permissions parsing is well-designed.Good handling of the INI-like format. The comment at lines 553-555 clearly documents that mixed-format sections are not supported, which is acceptable since flatpak doesn't use mixed formats.
578-626: Snap permission modification follows privilege guidelines.The docstring correctly notes that the CLI layer should handle user confirmation for privilege escalation, following the "no silent sudo" guideline. Based on learnings and coding guidelines.
628-653: Flatpak permission modification uses user-level overrides.Good choice using
--userflag which doesn't require privilege escalation.
659-699: Snap redirect detection is thorough.Good categorization of redirect types (transitional packages, snap metadata mentions, system config). This is a read-only operation with no security concerns.
738-758: Restore operation is safe with good preconditions.Properly checks that backup exists and config doesn't already exist before restoring. Same audit logging consideration applies here.
764-815: Storage analysis is comprehensive.Good approach using filesystem for snap sizes (more accurate than parsing
snap list). The deduplication logic correctly handles multiple snap revisions by keeping the largest.
817-864: Storage analysis formatting is clear and informative.The nested
format_sizehelper is clean. Good presentation with totals and top packages per format.cortex/cli.py (7)
33-35: TYPE_CHECKING import is correct.Proper use of
TYPE_CHECKINGto avoid circular imports while maintaining type hints.
2172-2209: Thepkgmethod follows established CLI patterns.Good runtime import to avoid overhead when the command isn't used. Help display is consistent with other commands.
2211-2231: Package sources display is clean and informative.Good handling of both found and not-found scenarios with appropriate user feedback.
2288-2338: Permissions display with auto-detection is well-implemented.Good heuristic for auto-detecting format based on app ID structure. Proper error handling for both snap and flatpak permission retrieval.
2341-2349: Storage analysis handler is clean.Simple delegation to the manager's analysis and formatting methods.
2713-2754: Argparse definitions are correct and complete.All subcommands have appropriate arguments with clear help text. The
--formatchoices correctly limit valid inputs.
3018-3019: Routing integration is correct.Consistent with the established command routing pattern in
main().
Piyushrathoree
force-pushed
the
feat/unified-package-manager
branch
from
335eb52 to
9d82673
Compare
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@Piyushrathoree There are still some copilot and coderabbit comments left, kindly address them as well.
Piyushrathoree
force-pushed
the
feat/unified-package-manager
branch
from
1fa56a4 to
2e92c3a
Compare
Piyushrathoree
force-pushed
the
feat/unified-package-manager
branch
2 times, most recently
from
1303125 to
39b7b46
Compare
|
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@Piyushrathoree Kindly resolve conflicts.
2 participants
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
New Features
Public API
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.