[#45] System Snapshot and Rollback Feature #341
Conversation
- Implement SnapshotManager with create/list/restore/delete commands - Fix shell injection vulnerabilities in restore operations - Add microseconds to snapshot IDs to prevent collisions - Include 15 unit tests with 66% code coverage - All 662 tests passing Security: Replace shell=True with list-based subprocess calls
|
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 Snapshot subsystem (SnapshotManager + SnapshotMetadata), integrates Changes
Sequence Diagram(s)sequenceDiagram
actor User as User/CLI
participant CLI as CortexCLI
participant SM as SnapshotManager
participant PKG as PackageManagers
participant FS as Filesystem
rect rgb(223,247,223)
Note over User,CLI: Create snapshot
User->>CLI: cortex snapshot create "desc"
CLI->>SM: create_snapshot(desc)
SM->>PKG: detect installed packages (APT/PIP/NPM)
PKG-->>SM: package lists
SM->>SM: collect system info & generate id
SM->>FS: create snapshot dir & write metadata.json (dir 700, file 600) with FileLock
SM->>SM: apply retention policy
SM-->>CLI: (success, snapshot_id, message)
CLI-->>User: print confirmation
end
rect rgb(255,245,209)
Note over User,CLI: Restore (dry-run)
User->>CLI: cortex snapshot restore <id> --dry-run
CLI->>SM: restore_snapshot(id, dry_run=True)
SM->>FS: read metadata.json (safe read with retry, FileLock)
SM->>PKG: detect current packages
SM->>SM: compute diffs & generate commands
SM-->>CLI: planned commands (dry-run)
CLI-->>User: print commands
end
rect rgb(251,239,246)
Note over User,CLI: Restore (execute)
User->>CLI: cortex snapshot restore <id>
CLI->>SM: restore_snapshot(id, dry_run=False)
SM->>SM: verify sudo/permissions
SM->>PKG: execute install/uninstall commands (may use sudo)
PKG-->>SM: execution results
SM-->>CLI: success/failure + executed commands
CLI-->>User: print result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
docs/SNAPSHOT_SYSTEM_IMPLEMENTATION.md (1)
62-67: Add language specifiers to fenced code blocks.Multiple code blocks throughout this document lack language specifiers (e.g., lines 62, 122, 143, 167, 229, 247, etc.). Adding language identifiers improves syntax highlighting and accessibility.
🔎 Example fix
-``` +```text CX ✓ ✅ Snapshot 20251222_160045_531066 created successfully with 1766 packagestests/unit/test_snapshot_manager.py (3)
271-276: Directory permission check is incomplete.The test verifies directory existence but not the actual 700 permissions as claimed. Consider using
statto verify the mode:🔎 Proposed enhancement
def test_directory_security(self): """Test that snapshot directory has secure permissions.""" # Directory should be created with 700 permissions self.assertTrue(self.snapshots_dir.exists()) - # Note: Permission checking requires actual filesystem, - # but we verify the directory exists + # Verify 700 permissions (owner rwx only) + import stat + mode = self.snapshots_dir.stat().st_mode & 0o777 + self.assertEqual(mode, 0o700, f"Expected 700 permissions, got {oct(mode)}")
108-109: Unused@patch.objectdecorator.The
mock_createparameter is declared but never used in the test body. The same applies totest_get_snapshot_success(line 140) andtest_delete_snapshot_success(line 172). Remove unused patches to clarify test intent.🔎 Proposed fix
-@patch.object(SnapshotManager, "create_snapshot") -def test_list_snapshots_with_data(self, mock_create): +def test_list_snapshots_with_data(self): """Test listing snapshots with existing data."""
75-79: Dictionary ordering assumption in NPM test.The test assumes
packages[0]["name"]equals"express", but dictionary iteration order innpm_data["dependencies"]is insertion order (Python 3.7+). While this works, the assertion is fragile if the mock data structure changes. Consider asserting membership instead:names = {pkg["name"] for pkg in packages} self.assertIn("express", names) self.assertIn("lodash", names)cortex/snapshot_manager.py (3)
154-170: Add timeout tounamesubprocess calls.Unlike the package detection methods, the
unamecalls lack timeout protection. Whileunameis typically fast, consistency with the rest of the codebase is preferred.🔎 Proposed fix
result = subprocess.run( ["uname", "-r"], capture_output=True, text=True, check=False, + timeout=self.TIMEOUT, )
74-76: Consider validating snapshot_id against path traversal.The
_get_snapshot_pathmethod directly concatenatessnapshot_idinto the path. While the ID is generated internally, external callers (via CLI) could potentially pass malicious IDs like../../../etc. Consider validating the ID format:🔎 Proposed validation
def _get_snapshot_path(self, snapshot_id: str) -> Path: """Get path to snapshot directory""" + # Validate snapshot_id to prevent path traversal + if not snapshot_id or "/" in snapshot_id or snapshot_id.startswith("."): + raise ValueError(f"Invalid snapshot ID: {snapshot_id}") return self.snapshots_dir / snapshot_id
345-357: Restore can leave system in partial state on failure.If the first APT remove succeeds but APT install fails, the system is left in an inconsistent state. Consider:
- Documenting this limitation clearly
- Or implementing a two-phase approach (dry-run validation first, then execute)
This is noted as a reliability concern for future improvement.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cortex/cli.pycortex/snapshot_manager.pydocs/SNAPSHOT_SYSTEM_IMPLEMENTATION.mdtests/unit/test_snapshot_manager.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/cli.pycortex/snapshot_manager.pytests/unit/test_snapshot_manager.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/unit/test_snapshot_manager.py
🧬 Code graph analysis (3)
cortex/cli.py (3)
cortex/snapshot_manager.py (6)
SnapshotManager(37-429)create_snapshot(177-233)list_snapshots(235-253)get_snapshot(255-273)restore_snapshot(298-411)delete_snapshot(275-296)cortex/first_run_wizard.py (1)
_print_error(746-748)cortex/branding.py (1)
cx_print(49-69)
cortex/snapshot_manager.py (4)
cortex/logging_system.py (2)
warning(202-204)info(198-200)cortex/kernel_features/llm_device.py (1)
open(158-159)cortex/user_preferences.py (1)
load(117-148)cortex/cli.py (1)
snapshot(747-835)
tests/unit/test_snapshot_manager.py (1)
cortex/snapshot_manager.py (5)
SnapshotMetadata(25-34)_detect_apt_packages(82-101)_detect_pip_packages(103-120)_detect_npm_packages(122-140)_generate_snapshot_id(70-72)
🪛 LanguageTool
docs/SNAPSHOT_SYSTEM_IMPLEMENTATION.md
[style] ~4-~4: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ... Implementation Date: December 22, 2025 Status: ✅ Complete and Production...
(MISSING_COMMA_AFTER_YEAR)
[uncategorized] ~634-~634: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...eges. Please run: sudo -v ``` --- ### Medium Priority Issues (Fixed) #### 5. 📝 Poor Error M...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.18.1)
docs/SNAPSHOT_SYSTEM_IMPLEMENTATION.md
62-62: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
122-122: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
143-143: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
167-167: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
202-202: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
229-229: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
247-247: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
285-285: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
294-294: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
302-302: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
328-328: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
344-344: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
357-357: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
369-369: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
391-391: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
725-725: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
755-755: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
785-785: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
825-825: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
842-842: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
879-879: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
889-889: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
906-906: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
930-930: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
940-940: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
983-983: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
993-993: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1033-1033: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1054-1054: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1071-1071: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (7)
cortex/snapshot_manager.py (3)
24-35: LGTM!The
SnapshotMetadatadataclass is well-defined with proper type hints and default values for optional fields.
82-101: LGTM on APT package detection.Proper timeout, error handling, and output parsing. Good use of
check=Falsewith explicit return code validation.
413-428: LGTM on retention policy.The logic correctly sorts by timestamp and deletes the oldest snapshots when exceeding the limit. Error handling is appropriate.
cortex/cli.py (4)
747-756: LGTM on snapshot action validation.Proper handling of missing subcommand with helpful error messages, consistent with other CLI commands like
notify.
784-788: PotentialAttributeErrorifsnapshot_idnot provided.While argparse marks
snapshot_idas required forshow,restore, anddeletesubparsers, if a user bypasses argparse or the parser is misconfigured,args.snapshot_idcould be missing. Consider defensive validation:if not getattr(args, 'snapshot_id', None): self._print_error("Snapshot ID required") return 1However, since argparse enforces this, the risk is low.
983-1005: LGTM on argument parser configuration.Subparsers are correctly configured with appropriate help text and required arguments. The structure mirrors other CLI commands like
notify.
1050-1051: LGTM on command routing.Clean integration following the existing pattern for other commands.
| **Code Coverage**: 66.43% | ||
| **All Tests**: ✅ PASSING |
There was a problem hiding this comment.
Test coverage below required threshold.
The documentation states 66.43% coverage, but the coding guidelines require >80% test coverage for pull requests in tests/**/*.py. Consider adding tests for uncovered paths such as:
- Error handling in
_get_system_info - Actual restore execution (non-dry-run paths)
- Edge cases in package detection failures
🤖 Prompt for AI Agents
In docs/SNAPSHOT_SYSTEM_IMPLEMENTATION.md around lines 683-684 the reported test
coverage is 66.43% which is below the required >80% for tests/**/*.py; add unit
and/or integration tests under tests/**/*.py to raise coverage by covering
currently untested paths: add tests that simulate and assert error handling in
_get_system_info (raise expected exceptions and verify logging/return behavior),
tests that exercise actual restore execution (non-dry-run behavior, state
changes, side effects and cleanup), and tests for package detection edge cases
(corrupt/missing manifests, unsupported formats, partial detections), run the
test suite with coverage to confirm >80% and iterate until coverage and
assertions pass.
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive system snapshot and rollback feature for Cortex Linux, allowing users to create, manage, and restore package state snapshots. The implementation includes support for APT, PIP, and NPM packages with security hardening against shell injection and proper permission management.
Key Changes:
- New
SnapshotManagerclass with snapshot creation, listing, restoration, and deletion capabilities - CLI integration with 5 commands for snapshot management (
create,list,show,restore,delete) - Comprehensive test suite with 15 unit tests achieving 66.43% coverage
- Detailed implementation documentation
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/unit/test_snapshot_manager.py | Comprehensive unit tests for SnapshotManager with 15 test cases covering all major functionality including package detection, snapshot CRUD operations, and retention policy |
| docs/SNAPSHOT_SYSTEM_IMPLEMENTATION.md | Extensive documentation covering commands reference, implementation details, testing documentation, and security considerations |
| cortex/snapshot_manager.py | Core implementation of SnapshotManager class with package detection for APT/PIP/NPM, snapshot creation/restoration with shell injection protection, and retention policy management |
| cortex/cli.py | CLI integration adding snapshot subcommand with parsers for create, list, show, restore, and delete operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/unit/test_snapshot_manager.py
Outdated
| snapshot_id = f"2025010{i % 10}_12000{i}" | ||
| snapshot_path = self.snapshots_dir / snapshot_id | ||
| snapshot_path.mkdir(parents=True) | ||
|
|
||
| metadata = { | ||
| "id": snapshot_id, | ||
| "timestamp": f"2025-01-0{i % 10}T12:00:0{i}", |
There was a problem hiding this comment.
The snapshot IDs created in this test also contain invalid date formats. The pattern "2025010{i % 10}_12000{i}" generates IDs like "20250100_120000" (when i=0) where "20250100" represents "2025-01-00" - an invalid date with day 0. For i=10 and i=11, this creates "20250100_1200010" and "20250101_1200011" which break the expected format entirely. These malformed IDs could cause unexpected behavior in the retention policy logic.
| snapshot_id = f"2025010{i % 10}_12000{i}" | |
| snapshot_path = self.snapshots_dir / snapshot_id | |
| snapshot_path.mkdir(parents=True) | |
| metadata = { | |
| "id": snapshot_id, | |
| "timestamp": f"2025-01-0{i % 10}T12:00:0{i}", | |
| day = i + 1 # Days 01-12 (all valid) | |
| snapshot_id = f"202501{day:02d}_120000" | |
| snapshot_path = self.snapshots_dir / snapshot_id | |
| snapshot_path.mkdir(parents=True) | |
| metadata = { | |
| "id": snapshot_id, | |
| "timestamp": f"2025-01-{day:02d}T12:00:00", |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…fix ID display width
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cortex/snapshot_manager.py (1)
143-177: Consider adding timeouts to subprocess calls for consistency.While
unamecommands typically execute instantly, addingtimeoutparameters would improve consistency with other subprocess calls in this file and provide defense-in-depth.🔎 Proposed enhancement
result = subprocess.run( ["uname", "-r"], capture_output=True, text=True, check=False, + timeout=5, )Apply the same change at line 165 for the
uname -mcall.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pycortex/snapshot_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/snapshot_manager.py
🧬 Code graph analysis (2)
cortex/cli.py (3)
cortex/snapshot_manager.py (6)
SnapshotManager(37-456)create_snapshot(178-235)list_snapshots(237-255)get_snapshot(257-275)restore_snapshot(369-438)delete_snapshot(277-298)cortex/first_run_wizard.py (1)
_print_error(746-748)cortex/branding.py (1)
cx_print(49-69)
cortex/snapshot_manager.py (1)
cortex/cli.py (1)
snapshot(747-835)
🔇 Additional comments (22)
cortex/snapshot_manager.py (13)
1-22: LGTM!Module structure, imports, and documentation are well-organized and follow Python best practices.
24-35: LGTM!The dataclass is well-structured with appropriate type hints and documentation.
37-52: LGTM!Class documentation is clear, and timeout constants are well-defined.
53-70: LGTM!Directory initialization and security enforcement are correctly implemented. The use of
chmod()after directory creation properly addresses umask limitations.
71-82: LGTM!Helper methods are well-documented with appropriate type hints. Microsecond-precision ID generation effectively prevents race conditions.
83-142: LGTM!Package detection methods properly implement timeout protection and graceful error handling. The use of
check=Falsewith explicit return code checking is appropriate.
178-236: LGTM!Snapshot creation is well-implemented with proper security measures. The use of
os.open()with explicit mode flags ensures metadata files are created with secure permissions from the outset, avoiding TOCTOU vulnerabilities.
237-256: LGTM!Snapshot listing logic is correct and provides appropriate error handling.
257-276: LGTM!Snapshot retrieval is straightforward and properly documented.
277-299: LGTM!Snapshot deletion is correctly implemented with appropriate existence checking and error handling.
300-319: LGTM!Command execution helper properly implements timeout protection as per previous feedback. The separation of dry-run logic is clean.
369-439: LGTM!Restoration logic is well-structured with comprehensive error handling. The sudo permission check and timeout protection are properly implemented.
440-456: LGTM!Retention policy implementation correctly maintains the snapshot limit by removing oldest snapshots.
cortex/cli.py (9)
747-756: LGTM!Method signature and initial validation are well-structured. The local import of
SnapshotManageris appropriate for avoiding potential import overhead.
758-765: LGTM!Snapshot creation flow is clean and provides appropriate user feedback.
767-782: LGTM!Snapshot listing display properly accommodates microsecond-precision IDs with the 24-character width, addressing previous feedback.
784-800: LGTM!Snapshot detail display is comprehensive and well-formatted.
802-824: LGTM!Restore flow handles both dry-run and actual execution scenarios with appropriate user feedback.
826-834: LGTM!Snapshot deletion flow is straightforward and provides clear feedback.
864-864: LGTM!Help table entry is consistent with other command descriptions.
982-1006: LGTM!Argument parser configuration is well-structured and follows CLI conventions consistently.
1050-1051: LGTM!Command routing is properly integrated into the main dispatcher.
RIVALHIDE
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cortex/snapshot_manager.py (1)
342-389: APT restore ignores package versions from snapshot.Line 374 defines the APT version format as
lambda name, ver: name, discarding version information. Although snapshots capture APT versions via dpkg output, the restore logic fails to use them. APT packages install at their latest available version rather than the exact snapshot version, contradicting the PR's goal of capturing and restoring exact package states.The correct fix uses APT's version specification syntax with an equals sign:
version_formats = { - "apt": lambda name, ver: name, # APT installs latest by default + "apt": lambda name, ver: f"{name}={ver}" if ver else name, "pip": lambda name, ver: f"{name}=={ver}", "npm": lambda name, ver: f"{name}@{ver}" }Note: This may require error handling if the exact version is unavailable in APT repositories. Consider adding fallback logic or logging when version-specific installation fails.
🧹 Nitpick comments (3)
cortex/snapshot_manager.py (3)
83-112: Add type hint forparser_funcparameter.The
parser_funcparameter lacks a type annotation. Consider addingCallable[[str], list[dict[str, str]]]to improve type safety.🔎 Proposed fix
Import
Callableat the top of the file:from pathlib import Path -from typing import Optional +from typing import Callable, OptionalThen add the type hint:
def _run_package_detection( self, cmd: list[str], - parser_func, + parser_func: Callable[[str], list[dict[str, str]]], manager_name: str ) -> list[dict[str, str]]:
132-139: Consider usingNoneor empty string instead of "unknown" for missing NPM versions.Line 138 uses
"unknown"as a fallback version. UsingNoneor an empty string might be more consistent and avoid potential issues if version strings are used for comparison or restoration.🔎 Proposed change
if "dependencies" in npm_data: for name, info in npm_data["dependencies"].items(): - packages.append({"name": name, "version": info.get("version", "unknown")}) + packages.append({"name": name, "version": info.get("version", "")})
165-198: Add timeout to subprocess calls in system info detection.The
subprocess.runcalls at lines 177 and 187 lack atimeoutparameter. While less critical than package operations, these could still hang if the system is under stress.🔎 Proposed fix
# Kernel version result = subprocess.run( ["uname", "-r"], capture_output=True, text=True, check=False, + timeout=5, ) if result.returncode == 0: info["kernel"] = result.stdout.strip() # Architecture result = subprocess.run( ["uname", "-m"], capture_output=True, text=True, check=False, + timeout=5, )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/snapshot_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/snapshot_manager.py
🧬 Code graph analysis (1)
cortex/snapshot_manager.py (2)
cortex/logging_system.py (2)
warning(202-204)info(198-200)cortex/cli.py (1)
snapshot(747-835)
🔇 Additional comments (10)
cortex/snapshot_manager.py (10)
1-34: LGTM!The module header, imports, and
SnapshotMetadatadataclass are well-structured with proper type hints and documentation.
37-51: LGTM!Class documentation is clear, and the constants are well-defined. The
RESTORE_TIMEOUTof 300 seconds addresses previous feedback about timeout protection.
53-69: LGTM!The directory initialization and security enforcement properly address previous concerns about permission windows by explicitly calling
chmodafter directory creation.
71-81: LGTM!The snapshot ID generation with microsecond precision and path helper methods are correctly implemented.
141-163: LGTM!The package detection methods correctly use the generic wrapper with appropriate commands for each package manager.
200-257: LGTM!Snapshot creation properly implements security hardening with secure file permissions from creation (using
os.openwith0o600at line 241) and directory permissions enforcement. This addresses previous concerns about permission windows.
259-320: LGTM!The snapshot listing, retrieval, and deletion methods are well-implemented with proper error handling and type hints.
322-340: LGTM!The command execution wrapper properly implements timeout protection (300 seconds) and error propagation, addressing previous feedback about indefinite hangs.
391-460: LGTM!The restore logic is well-structured with comprehensive error handling for timeout and subprocess failures. The sudo permission check (lines 409-424) is a good security practice before attempting system modifications.
462-478: LGTM!The retention policy correctly implements the requirement to maintain a maximum of 10 snapshots by deleting the oldest ones.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
TEST_SUMMARY.mdtests/installer/test_parallel_install.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:
tests/installer/test_parallel_install.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/installer/test_parallel_install.py
**/*install*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*install*.py: Dry-run by default for all installations in command execution
No silent sudo execution - require explicit user confirmation
Implement audit logging to ~/.cortex/history.db for all package operations
Files:
tests/installer/test_parallel_install.py
🪛 markdownlint-cli2 (0.18.1)
TEST_SUMMARY.md
76-76: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (3)
tests/installer/test_parallel_install.py (2)
20-22: LGTM! Consistent Python 3 invocation.The updates from
pythontopython3across all test commands ensure explicit Python 3 interpreter usage, improving consistency and reliability.Also applies to: 45-47, 73-75, 101-104, 124-124, 141-141, 154-154, 180-180, 200-202, 221-221
250-253: LGTM! Integration tests consistently updated.The integration test commands are consistently updated to use
python3, maintaining the same explicit Python 3 usage pattern as the unit tests.Also applies to: 279-281
TEST_SUMMARY.md (1)
5-15: The review comment is incorrect and should be retracted.Key findings:
- TEST_SUMMARY.md explicitly states the coverage requirement as "> 0%" (lines 12-15), not ">80%"
- The overall coverage of 60.06% exceeds the stated > 0% requirement and should show ✅ PASS
- Issue #45 is not a coverage requirement issue—it documents the "System Snapshot and Rollback Points" feature
- The 66.43% figure is module-specific coverage (snapshot_manager.py from SNAPSHOT_SYSTEM_IMPLEMENTATION.md), not a discrepancy with the overall 60.06% metric
- CI workflow enforces
--cov-fail-under=0, confirming the acceptance criteria is 0% overall coverageThe document's claim of "All Requirements Met" is accurate based on the stated requirements.
Likely an incorrect or invalid review comment.
|
|
||
| **Date**: December 22, 2025 | ||
|
|
||
| ## ✅ All Requirements Met |
There was a problem hiding this comment.
Misleading claim when acceptance criteria are unmet.
Line 5 states "✅ All Requirements Met," but the coverage requirement of >80% from issue #45 is not satisfied (60.06% reported). Either the coverage requirement has been waived, or this claim should be revised to accurately reflect the status.
Consider updating the headline to reflect that coverage remains below the target or explicitly document the waiver/deferral of this requirement.
🤖 Prompt for AI Agents
In TEST_SUMMARY.md around line 5, the heading "✅ All Requirements Met" is
misleading because test coverage is 60.06% (below the >80% requirement from
issue #45); update the headline to accurately reflect the status (e.g., "⚠️ Some
Requirements Not Met" or "Coverage Below Target") and add one sentence noting
either the unmet coverage percentage or that the coverage requirement was
formally waived/deferred with a reference to the waiver ticket.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cortex/snapshot_manager.py (1)
324-392: LGTM! Timeout protection and version handling properly implemented.The code correctly addresses past review concerns:
- Line 341:
RESTORE_TIMEOUTprevents indefinite hangs- Lines 375-379: APT version format now properly specifies exact versions with
f"{name}={ver}"The conditional version formatting (
if ver else name) provides good defensive handling for edge cases.💡 Optional: Extract command templates to class constants
Lines 365-379 define manager-specific command templates. Consider extracting these to class-level constants for better organization:
class SnapshotManager: RETENTION_LIMIT = 10 TIMEOUT = 30 RESTORE_TIMEOUT = 300 # Package manager command templates REMOVE_COMMANDS = { "apt": ["sudo", "apt-get", "remove", "-y"], "pip": ["pip", "uninstall", "-y"], "npm": ["npm", "uninstall", "-g"] } INSTALL_COMMANDS = { "apt": ["sudo", "apt-get", "install", "-y"], "pip": ["pip", "install"], "npm": ["npm", "install", "-g"] } @staticmethod def _version_format(manager: str, name: str, ver: str) -> str: """Format package name with version for specific manager.""" formats = { "apt": lambda n, v: f"{n}={v}" if v else n, "pip": lambda n, v: f"{n}=={v}" if v else n, "npm": lambda n, v: f"{n}@{v}" if v else n } return formats[manager](name, ver)This improves maintainability but is not critical for current functionality.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/snapshot_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/snapshot_manager.py
🧬 Code graph analysis (1)
cortex/snapshot_manager.py (1)
cortex/cli.py (1)
snapshot(747-835)
🔇 Additional comments (10)
cortex/snapshot_manager.py (10)
1-35: LGTM! Clean structure and proper type hints.The imports, module docstring, and
SnapshotMetadatadataclass are well-structured with comprehensive type hints and documentation.
53-69: LGTM! Proper initialization with security enforcement.The constructor correctly initializes the snapshots directory and enforces secure permissions via
_enforce_directory_security(). The explicitchmodcall addresses umask-related permission issues mentioned in past reviews.
71-81: LGTM! Clean utility methods.Helper methods are well-documented with proper type hints and clear implementations.
165-200: LGTM! Robust system info gathering.System information detection properly handles
/etc/os-releaseparsing withmaxsplit=1, uses timeout protection for subprocess calls, and includes comprehensive exception handling.
202-259: LGTM! Secure snapshot creation with proper permission handling.The method correctly addresses past security concerns:
- Line 217 uses explicit
os.chmod()for directory permissions- Lines 243-245 use
os.open()with mode0o600for secure file creationThe retention policy is properly applied after snapshot creation.
261-299: LGTM! Clean snapshot retrieval methods.Both
list_snapshotsandget_snapshothave proper exception handling and return types matching their signatures. The newest-first sorting is appropriate for user experience.
301-322: LGTM! Safe snapshot deletion.The method properly validates snapshot existence before deletion and includes exception handling.
393-462: LGTM! Comprehensive restore implementation with proper error handling.The restore method includes:
- Sudo validation (lines 411-426) before attempting changes
- Specific exception handling for
TimeoutExpired(lines 450-454) andCalledProcessError(lines 455-459)- Clear messaging for both dry-run and actual restore modes
- Proper command tracking for user visibility
464-480: LGTM! Correct retention policy implementation.The retention policy correctly maintains a maximum of 10 snapshots by deleting the oldest ones. Exception handling is appropriate with warning logs for best-effort cleanup.
1-480: Note: Test coverage gap relative to acceptance criteria.The PR reports 66.43% test coverage for this implementation, while the linked issue #45 acceptance criteria require >80% coverage. Consider expanding unit tests to meet the target, particularly for edge cases in package detection, restoration error paths, and permission handling.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cortex/snapshot_manager.py (3)
239-239: Offer to implement actual size calculation.The
size_bytesfield is currently hardcoded to 0. This could be useful information for users managing disk space.Do you want me to generate code to calculate the actual snapshot size by summing the metadata file size and any additional snapshot data?
271-271: Consider more explicit timestamp-based sorting.The sorting relies on directory names (snapshot IDs) which happen to be timestamp-based. While this works, it's somewhat implicit. For consistency with
_apply_retention_policy(line 471), consider sorting by the parsed timestamp from metadata.🔎 Proposed refactor for explicit sorting
- for snapshot_dir in sorted(self.snapshots_dir.iterdir(), reverse=True): + snapshot_list = [] + for snapshot_dir in self.snapshots_dir.iterdir(): if snapshot_dir.is_dir(): metadata_path = snapshot_dir / "metadata.json" if metadata_path.exists(): with open(metadata_path, "r") as f: data = json.load(f) - snapshots.append(SnapshotMetadata(**data)) + snapshot_list.append(SnapshotMetadata(**data)) + # Sort by timestamp (newest first) + snapshots = sorted(snapshot_list, key=lambda s: s.timestamp, reverse=True)
377-379: Version format check could be more explicit.The lambda functions use
if verto check for version presence, which will evaluate toFalsefor empty strings. While this works, it's somewhat implicit.🔎 More explicit version check
version_formats = { - "apt": lambda name, ver: f"{name}={ver}" if ver else name, - "pip": lambda name, ver: f"{name}=={ver}" if ver else name, - "npm": lambda name, ver: f"{name}@{ver}" if ver else name + "apt": lambda name, ver: f"{name}={ver}" if ver is not None and ver != "" else name, + "pip": lambda name, ver: f"{name}=={ver}" if ver is not None and ver != "" else name, + "npm": lambda name, ver: f"{name}@{ver}" if ver is not None and ver != "" else name }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
TEST_SUMMARY.mdcortex/snapshot_manager.pytests/unit/test_snapshot_manager.py
✅ Files skipped from review due to trivial changes (1)
- TEST_SUMMARY.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/test_snapshot_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/snapshot_manager.py
🧬 Code graph analysis (1)
cortex/snapshot_manager.py (1)
cortex/cli.py (1)
snapshot(747-835)
🔇 Additional comments (1)
cortex/snapshot_manager.py (1)
394-463: Comprehensive restore implementation with excellent error handling.The restore logic is well-structured with:
- Snapshot validation before proceeding
- Sudo permission check with clear user guidance
- Per-manager package diff computation
- Comprehensive exception handling for timeouts and command failures
- Detailed error messages including command context
The dry-run support is properly implemented throughout.
| # Define manager-specific command templates | ||
| remove_cmds = { | ||
| "apt": ["sudo", "apt-get", "remove", "-y"], | ||
| "pip": ["pip", "uninstall", "-y"], |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use sys.executable for pip commands to match detection.
The pip uninstall and install commands use the plain pip executable, but package detection (line 153) uses sys.executable -m pip to ensure consistency with the running Python interpreter. This inconsistency could cause restoration to use a different pip binary than detection, leading to mismatched package states.
🔎 Proposed fix for pip command consistency
remove_cmds = {
"apt": ["sudo", "apt-get", "remove", "-y"],
- "pip": ["pip", "uninstall", "-y"],
+ "pip": [sys.executable, "-m", "pip", "uninstall", "-y"],
"npm": ["npm", "uninstall", "-g"]
}
install_cmds = {
"apt": ["sudo", "apt-get", "install", "-y"],
- "pip": ["pip", "install"],
+ "pip": [sys.executable, "-m", "pip", "install"],
"npm": ["npm", "install", "-g"]
}Also applies to: 373-373
🤖 Prompt for AI Agents
In cortex/snapshot_manager.py around lines 368 and 373, the pip
uninstall/install commands use the literal "pip" executable which is
inconsistent with package detection at line 153 that uses sys.executable -m pip;
update those command lists to invoke pip via the running Python interpreter
(i.e., use sys.executable with "-m", "pip", followed by the existing args such
as "uninstall", "-y" or "install", etc.) so the same interpreter is used for
detection and restoration.
cortex/snapshot_manager.py
Outdated
| remove_cmds = { | ||
| "apt": ["sudo", "apt-get", "remove", "-y"], | ||
| "pip": ["pip", "uninstall", "-y"], | ||
| "npm": ["npm", "uninstall", "-g"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file and understand the context around lines 369-374
cat -n cortex/snapshot_manager.py | sed -n '360,385p'Repository: cortexlinux/cortex
Length of output: 1325
🏁 Script executed:
# Check the _execute_package_command method to see if sudo is applied there
cat -n cortex/snapshot_manager.py | grep -A 20 "_execute_package_command"Repository: cortexlinux/cortex
Length of output: 2121
🏁 Script executed:
# Check what happens around line 411-412 about sudo permissions check
cat -n cortex/snapshot_manager.py | sed -n '411,430p'Repository: cortexlinux/cortex
Length of output: 903
🏁 Script executed:
# Let's check if there's any environment setup or sudo wrapping around the entire restore process
cat -n cortex/snapshot_manager.py | sed -n '429,450p'Repository: cortexlinux/cortex
Length of output: 1453
🏁 Script executed:
# Check if there are any comments or configuration about npm permissions elsewhere in the file
rg -n "npm|sudo" cortex/snapshot_manager.py | head -30Repository: cortexlinux/cortex
Length of output: 1465
🏁 Script executed:
# Check if there's documentation about npm configuration requirements
find . -type f \( -name "README*" -o -name "SETUP*" -o -name "INSTALL*" \) | head -10Repository: cortexlinux/cortex
Length of output: 391
🏁 Script executed:
# Check for npm-related documentation and setup instructions
rg -i "npm|node|global" README.md docs/modules/README_DEPENDENCIES.md | head -20Repository: cortexlinux/cortex
Length of output: 181
🏁 Script executed:
# Let's check how _detect_npm_packages detects npm packages
cat -n cortex/snapshot_manager.py | sed -n '158,170p'Repository: cortexlinux/cortex
Length of output: 582
🏁 Script executed:
# Check if there's any issue tracking or TODO comments about npm sudo
rg -i "TODO|FIXME|BUG|issue" cortex/snapshot_manager.py | grep -i "npm\|sudo"Repository: cortexlinux/cortex
Length of output: 44
🏁 Script executed:
# Check for any tests that might reveal intended behavior
find . -type f -name "*test*.py" -o -name "*_test.py" | grep -i snapshotRepository: cortexlinux/cortex
Length of output: 99
🏁 Script executed:
# Check test file for npm/sudo related tests
rg -n "npm|sudo" tests/unit/test_snapshot_manager.pyRepository: cortexlinux/cortex
Length of output: 3769
🏁 Script executed:
# Check the test_restore_snapshot_live_execution test to see what npm commands are being tested
cat -n tests/unit/test_snapshot_manager.py | sed -n '281,320p'Repository: cortexlinux/cortex
Length of output: 2072
🏁 Script executed:
# Let's check if there are any comments or documentation about npm configuration in the codebase
rg -i "npm.*prefix|npm.*config|node.*global" --type md --type txtRepository: cortexlinux/cortex
Length of output: 126
🏁 Script executed:
# Check if there's any npm configuration or setup documentation
cat -n docs/CONFIGURATION.md | grep -A 10 -B 5 "npm"Repository: cortexlinux/cortex
Length of output: 2440
🏁 Script executed:
# Check if there's any npm configuration or setup documentation in the project
find . -name ".npmrc" -o -name "package.json" -o -name ".nvm*" | head -10Repository: cortexlinux/cortex
Length of output: 44
🏁 Script executed:
# Check if there are any setup instructions or comments about npm configuration
grep -r "npm" cortex/snapshot_manager.py | grep -i "config\|setup\|prefix\|sudo"Repository: cortexlinux/cortex
Length of output: 44
Add sudo prefix to npm global commands to match apt behavior.
The npm commands don't include sudo for global operations (lines 369, 374), despite the code checking sudo availability at the start of restoration and apt commands including sudo in their templates. By default, installing global packages using NPM requires administrator privileges on most Linux systems. The mismatch between apt (which includes sudo) and npm (which doesn't) will cause npm restoration to fail with permission errors on standard system configurations.
Change:
- Line 369:
"npm": ["npm", "uninstall", "-g"]→"npm": ["sudo", "npm", "uninstall", "-g"] - Line 374:
"npm": ["npm", "install", "-g"]→"npm": ["sudo", "npm", "install", "-g"]
🤖 Prompt for AI Agents
In cortex/snapshot_manager.py around line 369 (and similarly at line 374), the
npm command templates lack the sudo prefix for global operations; update the npm
arrays to prefix commands with "sudo" so they read ["sudo", "npm", "uninstall",
"-g"] at line 369 and ["sudo", "npm", "install", "-g"] at line 374 to match apt
behavior and the earlier sudo availability checks.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.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
🔇 Additional comments (4)
cortex/cli.py (4)
787-838: LGTM - Show, restore, and delete actions are well-implemented.The show action provides clear detailed output, the restore action properly handles both dry-run preview and actual restoration with appropriate error reporting, and the delete action follows consistent patterns. Error handling is thorough across all three actions.
867-867: LGTM - Help table updated correctly.The snapshot command is properly documented in the help table following the established pattern.
986-1008: LGTM - Argument parser setup is correct.The snapshot command argument structure is well-defined with appropriate subcommands, required/optional arguments, and help text. The pattern is consistent with the existing
notifycommand's subparser approach.
1053-1054: LGTM - Command routing is correct.The snapshot command is properly routed to the handler method following the established pattern for other commands.
…parator alignment
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/test_snapshot_manager.py (2)
30-102: Add return type hints to helper methods.Per the coding guidelines, type hints are required in Python code. The three helper methods lack return type hints:
_create_mock_snapshot_metadatashould return-> SnapshotMetadata_create_mock_snapshot_on_diskshould return-> Path_setup_mock_packagesshould return-> None🔎 Proposed fix
- def _create_mock_snapshot_metadata(self, snapshot_id="test_snapshot", + def _create_mock_snapshot_metadata(self, snapshot_id: str = "test_snapshot", apt_packages=None, pip_packages=None, - npm_packages=None, description="Test"): + npm_packages=None, description: str = "Test") -> SnapshotMetadata: """Helper method to create mock snapshot metadata. - def _create_mock_snapshot_on_disk(self, snapshot_id, metadata_dict=None): + def _create_mock_snapshot_on_disk(self, snapshot_id: str, metadata_dict: dict = None) -> Path: """Helper method to create a mock snapshot directory and metadata file. - def _setup_mock_packages(self, mock_apt, mock_pip, mock_npm, + def _setup_mock_packages(self, mock_apt, mock_pip, mock_npm, apt_packages=None, pip_packages=None, npm_packages=None): + apt_packages=None, pip_packages=None, npm_packages=None) -> None: """Helper method to setup mock package detection return values.
306-311: Strengthen the directory security test to verify actual permissions.The current test only checks that the directory exists but doesn't verify the 0o700 permissions that are enforced in
create_snapshot. The comment "Permission checking requires actual filesystem" is misleading—tests can verify permissions usingos.stat(path).st_mode.🔎 Proposed enhancement
def test_directory_security(self): """Test that snapshot directory has secure permissions.""" - # Directory should be created with 700 permissions + # Create a snapshot to trigger directory creation with secure permissions + with patch.object(SnapshotManager, "_detect_apt_packages", return_value=[]): + with patch.object(SnapshotManager, "_detect_pip_packages", return_value=[]): + with patch.object(SnapshotManager, "_detect_npm_packages", return_value=[]): + with patch.object(SnapshotManager, "_get_system_info", return_value={}): + success, snapshot_id, _ = self.manager.create_snapshot("Security test") + + self.assertTrue(success) + snapshot_path = self.snapshots_dir / snapshot_id self.assertTrue(self.snapshots_dir.exists()) - # Note: Permission checking requires actual filesystem, - # but we verify the directory exists + + # Verify directory has 700 permissions (owner read/write/execute only) + import stat + mode = os.stat(snapshot_path).st_mode + self.assertEqual(stat.S_IMODE(mode), 0o700)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.pytests/unit/test_snapshot_manager.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cortex/cli.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
tests/unit/test_snapshot_manager.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/unit/test_snapshot_manager.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to tests/**/*.py : Maintain >80% test coverage for pull requests
Applied to files:
tests/unit/test_snapshot_manager.py
🧬 Code graph analysis (1)
tests/unit/test_snapshot_manager.py (1)
cortex/snapshot_manager.py (12)
SnapshotMetadata(26-35)_detect_apt_packages(142-148)_detect_pip_packages(150-156)_detect_npm_packages(158-164)create_snapshot(203-260)list_snapshots(262-280)get_snapshot(282-300)delete_snapshot(302-323)_apply_retention_policy(465-481)restore_snapshot(394-463)_generate_snapshot_id(72-74)_get_system_info(166-201)
🔇 Additional comments (1)
tests/unit/test_snapshot_manager.py (1)
349-556: LGTM! Comprehensive error handling coverage.The error handling tests thoroughly cover various failure scenarios:
- System info detection failures (exceptions and non-zero return codes)
- CalledProcessError with and without stderr
- Timeout scenarios (TimeoutExpired)
- Missing binaries (FileNotFoundError)
- Invalid JSON parsing
- Sudo permission failures
- Snapshot creation exceptions
These tests address the coverage gaps identified in previous reviews and properly verify that the system degrades gracefully under error conditions.
| # Create 12 snapshots manually (exceeds limit of 10) | ||
| for i in range(12): | ||
| snapshot_id = f"2025010{(i % 9) + 1}_12000{i}" | ||
| metadata = { | ||
| "id": snapshot_id, | ||
| "timestamp": f"2025-01-0{(i % 9) + 1}T12:00:0{i}", | ||
| "description": f"Snapshot {i}", | ||
| "packages": {"apt": [], "pip": [], "npm": []}, | ||
| "system_info": {}, | ||
| "file_count": 0, | ||
| "size_bytes": 0 | ||
| } | ||
| self._create_mock_snapshot_on_disk(snapshot_id, metadata) |
There was a problem hiding this comment.
Fix malformed snapshot IDs and timestamps for i >= 10.
The current loop creates invalid snapshot IDs and timestamps when i reaches 10 or 11:
- For i=10:
snapshot_idbecomes"20250102_1200010"(7 digits in time portion instead of 6) - For i=10:
timestampbecomes"2025-01-02T12:00:010"(3 digits in seconds instead of 2)
These malformed identifiers break the expected format and could cause parsing or sorting failures in the retention policy logic.
🔎 Proposed fix
# Create 12 snapshots manually (exceeds limit of 10)
for i in range(12):
- snapshot_id = f"2025010{(i % 9) + 1}_12000{i}"
+ day = (i % 28) + 1 # Valid days 01-28
+ snapshot_id = f"202501{day:02d}_120000"
metadata = {
"id": snapshot_id,
- "timestamp": f"2025-01-0{(i % 9) + 1}T12:00:0{i}",
+ "timestamp": f"2025-01-{day:02d}T12:00:{i:02d}",
"description": f"Snapshot {i}",📝 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.
| # Create 12 snapshots manually (exceeds limit of 10) | |
| for i in range(12): | |
| snapshot_id = f"2025010{(i % 9) + 1}_12000{i}" | |
| metadata = { | |
| "id": snapshot_id, | |
| "timestamp": f"2025-01-0{(i % 9) + 1}T12:00:0{i}", | |
| "description": f"Snapshot {i}", | |
| "packages": {"apt": [], "pip": [], "npm": []}, | |
| "system_info": {}, | |
| "file_count": 0, | |
| "size_bytes": 0 | |
| } | |
| self._create_mock_snapshot_on_disk(snapshot_id, metadata) | |
| # Create 12 snapshots manually (exceeds limit of 10) | |
| for i in range(12): | |
| day = (i % 28) + 1 # Valid days 01-28 | |
| snapshot_id = f"202501{day:02d}_120000" | |
| metadata = { | |
| "id": snapshot_id, | |
| "timestamp": f"2025-01-{day:02d}T12:00:{i:02d}", | |
| "description": f"Snapshot {i}", | |
| "packages": {"apt": [], "pip": [], "npm": []}, | |
| "system_info": {}, | |
| "file_count": 0, | |
| "size_bytes": 0 | |
| } | |
| self._create_mock_snapshot_on_disk(snapshot_id, metadata) |
🤖 Prompt for AI Agents
In tests/unit/test_snapshot_manager.py around lines 246 to 258, the loop
constructs malformed snapshot IDs and timestamps once i >= 10; compute
zero-padded day and seconds instead of string-concatenating raw i: set day_str =
f"{i+1:02d}" and sec_str = f"{i:02d}", then use snapshot_id =
f"202501{day_str}_1200{sec_str}" and timestamp =
f"2025-01-{day_str}T12:00:{sec_str}" so both the date/day and seconds are always
two digits and the time portion remains exactly 6 digits, fixing parsing/sorting
issues.
Issues Fixed: - Missing dependency resolver for package version conflicts - demo.py crashes with AttributeError on SystemInfo object access - snapshot_manager.py has outdated type hints (Optional[X] vs X | None) - CLI snapshot commands have inconsistent formatting - Missing comprehensive tests for resolver functionality - Code quality issues (trailing whitespace, import ordering) Solutions Implemented: - Added DependencyResolver module with semantic version conflict resolution - Implemented smart upgrade and conservative downgrade strategies - Fixed demo.py to access SystemInfo attributes directly instead of dict methods - Updated snapshot_manager.py type hints to modern Python 3.10+ syntax - Applied PEP 8 formatting with ruff and black across all modified files - Added 9 comprehensive resolver tests covering all conflict scenarios - Updated 27 snapshot manager tests with proper formatting - Fixed import ordering and removed all trailing whitespace Technical Details: - New module: cortex/resolver.py (226 lines) - New tests: tests/test_resolver.py (226 lines, 9 test cases) - Modified: cortex/cli.py (45 changes) - Modified: cortex/demo.py (20 changes) - Modified: cortex/snapshot_manager.py (67 changes) - Modified: tests/unit/test_snapshot_manager.py (131 changes) Test Results: - Resolver tests: 9/9 passed - Snapshot tests: 27/27 passed - Full suite: 706 passed, 10 skipped - Code quality: All ruff and black checks passed Closes cortexlinux#45
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cortex/snapshot_manager.py (2)
357-362: Add sudo prefix to npm global commands to match apt behavior.NPM global install/uninstall operations require administrator privileges on most Linux systems. The sudo check at lines 400-408 already validates sudo availability before restore, so adding sudo to npm commands aligns with the apt behavior and past review feedback.
🔎 Proposed fix
remove_cmds = { "apt": ["sudo", "apt-get", "remove", "-y"], "pip": [sys.executable, "-m", "pip", "uninstall", "-y"], - "npm": ["npm", "uninstall", "-g"], + "npm": ["sudo", "npm", "uninstall", "-g"], } install_cmds = { "apt": ["sudo", "apt-get", "install", "-y"], "pip": [sys.executable, "-m", "pip", "install"], - "npm": ["npm", "install", "-g"], + "npm": ["sudo", "npm", "install", "-g"], }
354-362: Pip commands inconsistent with detection; usesys.executable -m pip.Package detection at line 149 uses
[sys.executable, "-m", "pip", ...]but restore commands at lines 356 and 361 use the plainpipexecutable. This could cause packages to be restored under a different Python interpreter than the one used for detection.🔎 Proposed fix
remove_cmds = { "apt": ["sudo", "apt-get", "remove", "-y"], - "pip": ["pip", "uninstall", "-y"], + "pip": [sys.executable, "-m", "pip", "uninstall", "-y"], "npm": ["npm", "uninstall", "-g"], } install_cmds = { "apt": ["sudo", "apt-get", "install", "-y"], - "pip": ["pip", "install"], + "pip": [sys.executable, "-m", "pip", "install"], "npm": ["npm", "install", "-g"], }
🧹 Nitpick comments (3)
cortex/demo.py (1)
19-19: Minor: Redundant rounding ontotal_gb.Per
cortex/hardware_detection.py:86-87,total_gbalready returns a value rounded to 1 decimal place. The additionalround(..., 1)here is harmless but unnecessary.🔎 Optional simplification
- memory_gb = round(hw.memory.total_gb, 1) if hw.memory else 0 + memory_gb = hw.memory.total_gb if hw.memory else 0cortex/resolver.py (2)
49-69: Parsed specs are unused; strategies lack computed version recommendations.
spec_aandspec_bare parsed at lines 51-52 but only serve to validate syntax. The comment at line 71 mentions "Find the intersection or highest compatible version" but no intersection is actually computed. The returned strategies provide generic guidance without concrete version numbers.If this is intentional as a first iteration, consider adding a
# TODOcomment. Otherwise, you could compute the intersection usingsemantic_versionto provide actionable version recommendations.🔎 Example: Add TODO or utilize specs
# Find the intersection or highest compatible version - # For successful parsing, return two strategies + # TODO: Compute intersection of spec_a and spec_b to recommend concrete versions + # For now, return generic strategies strategies = []
114-116: Docstring in__main__block should be a comment.Line 115 uses a docstring inside
if __name__ == "__main__":, which is valid Python but unconventional. A comment would be more idiomatic.🔎 Proposed fix
if __name__ == "__main__": - """Demonstration of the DependencyResolver.""" + # Demonstration of the DependencyResolver resolver = DependencyResolver()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cortex/cli.pycortex/demo.pycortex/resolver.pycortex/snapshot_manager.pytests/test_resolver.pytests/unit/test_snapshot_manager.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/test_snapshot_manager.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/resolver.pycortex/cli.pytests/test_resolver.pycortex/demo.pycortex/snapshot_manager.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/test_resolver.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : No silent sudo execution - require explicit user confirmation
Applied to files:
cortex/snapshot_manager.py
🧬 Code graph analysis (3)
tests/test_resolver.py (1)
cortex/resolver.py (2)
DependencyResolver(11-111)resolve(16-111)
cortex/demo.py (1)
cortex/hardware_detection.py (2)
total_gb(87-88)has_nvidia_gpu(662-664)
cortex/snapshot_manager.py (1)
cortex/cli.py (1)
snapshot(750-839)
🔇 Additional comments (8)
cortex/cli.py (2)
750-839: LGTM - Snapshot CLI integration is well-structured.The snapshot command handler correctly:
- Handles missing action gracefully with helpful error messages
- Uses lazy import for
SnapshotManagerto avoid circular imports- Properly unpacks tuple returns from manager methods
- Supports dry-run mode for restore operations
- Column widths (24 chars for ID) properly accommodate microsecond-precision snapshot IDs
986-1009: LGTM - Subparser configuration is complete.All snapshot subcommands are properly configured with required arguments:
create: optional descriptionlist: no additional args neededshow,restore,delete: requiresnapshot_idrestore: includes--dry-runflagtests/test_resolver.py (1)
10-225: LGTM - Comprehensive test coverage for DependencyResolver.The test suite thoroughly covers:
- Basic conflict resolution with valid semver
- Missing required keys raising
KeyError- Invalid semver producing error strategy
- Default wildcard behavior when versions omitted
- Complex and wildcard version constraints
- Major version conflicts
- Strategy targeting verification
Good use of
setup_methodfor fixture initialization and clear docstrings for each test case.cortex/snapshot_manager.py (4)
85-111: LGTM - Robust package detection with proper error handling.The
_run_package_detectionmethod correctly:
- Uses configurable timeout to prevent hangs
- Catches relevant exceptions including
TimeoutExpired,FileNotFoundError, andJSONDecodeError- Logs warnings without crashing on detection failures
- Returns empty list as safe fallback
195-252: LGTM - Secure snapshot creation with proper permissions.The
create_snapshotmethod addresses previous review feedback:
- Uses
os.chmod(snapshot_path, 0o700)after mkdir to enforce permissions regardless of umask- Uses
os.open()with explicit mode0o600for metadata file to avoid permission race- Applies retention policy after creation
382-448: LGTM - Comprehensive restore with sudo validation and timeout protection.The
restore_snapshotmethod properly:
- Validates sudo permissions before attempting privileged operations (lines 400-408)
- Uses
RESTORE_TIMEOUTfor subprocess calls- Handles
TimeoutExpiredandCalledProcessErrorwith informative error messages- Returns commands list for both success and failure cases
450-465: LGTM - Retention policy correctly deletes oldest snapshots.The retention policy:
- Sorts snapshots by timestamp (oldest first)
- Deletes only the excess over
RETENTION_LIMIT- Logs deletions for auditability
cortex/demo.py (1)
17-28: No issues found. Thehas_nvidia_gpuandhas_amd_gpuattributes exist on theSystemInfoobject returned bydetect_hardware()and are used correctly in the code.
Features: - Added DependencyResolver for semantic version conflict resolution - Made snapshot retention limit user-configurable (default: 10) - Added SnapshotSettings to user preferences Fixes: - Fixed demo.py SystemInfo object access bug - Updated type hints to Python 3.10+ syntax - Applied PEP 8 formatting with ruff/black Files Changed: - New: cortex/resolver.py (226 lines) - New: tests/test_resolver.py (9 tests) - Modified: cortex/snapshot_manager.py (reads from preferences) - Modified: cortex/user_preferences.py (added SnapshotSettings) - Modified: cortex/demo.py, cortex/cli.py - Modified: tests/unit/test_snapshot_manager.py Configuration: - cortex check-pref snapshots.retention_limit - cortex edit-pref set snapshots.retention_limit 20 Tests: 36/36 passed (9 resolver + 27 snapshot) Full suite: 706 passed, 10 skipped Closes cortexlinux#45
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cortex/snapshot_manager.py (1)
374-388: Pip commands should usesys.executablefor consistency with detection.The restore commands use plain
pip(lines 376, 381), but package detection usessys.executable -m pip(line 169). This inconsistency could cause restoration to use a different pip binary than detection, leading to mismatched package states.Also, npm global commands may require
sudoon standard Linux systems to match the sudo requirement checked at line 420.🔎 Proposed fix
remove_cmds = { "apt": ["sudo", "apt-get", "remove", "-y"], - "pip": ["pip", "uninstall", "-y"], - "npm": ["npm", "uninstall", "-g"], + "pip": [sys.executable, "-m", "pip", "uninstall", "-y"], + "npm": ["sudo", "npm", "uninstall", "-g"], } install_cmds = { "apt": ["sudo", "apt-get", "install", "-y"], - "pip": ["pip", "install"], - "npm": ["npm", "install", "-g"], + "pip": [sys.executable, "-m", "pip", "install"], + "npm": ["sudo", "npm", "install", "-g"], }
🧹 Nitpick comments (1)
cortex/snapshot_manager.py (1)
314-335: Consider validatingsnapshot_idto prevent path traversal.The
delete_snapshotmethod usesshutil.rmtreeon a path derived from user-providedsnapshot_id. If an attacker passes something like../../../important_dir, this could delete arbitrary directories.🔎 Proposed validation
def delete_snapshot(self, snapshot_id: str) -> tuple[bool, str]: """ Delete a snapshot. ... """ try: + # Validate snapshot_id to prevent path traversal + if not snapshot_id or "/" in snapshot_id or "\\" in snapshot_id or snapshot_id.startswith("."): + return (False, f"Invalid snapshot ID: {snapshot_id}") + snapshot_path = self._get_snapshot_path(snapshot_id) + # Ensure the resolved path is within snapshots_dir + if not snapshot_path.resolve().is_relative_to(self.snapshots_dir.resolve()): + return (False, f"Invalid snapshot ID: {snapshot_id}") + if not snapshot_path.exists(): return (False, f"Snapshot {snapshot_id} not found")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/snapshot_manager.pycortex/user_preferences.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/user_preferences.pycortex/snapshot_manager.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : No silent sudo execution - require explicit user confirmation
Applied to files:
cortex/snapshot_manager.py
🧬 Code graph analysis (1)
cortex/snapshot_manager.py (2)
cortex/user_preferences.py (2)
get(202-221)load(130-161)cortex/cli.py (1)
snapshot(750-839)
🔇 Additional comments (8)
cortex/user_preferences.py (1)
82-92: Well-structured dataclass with sensible defaults.The
SnapshotSettingsdataclass follows the established pattern in this file and provides appropriate configuration options. The inline comments adequately document each field's purpose.cortex/snapshot_manager.py (7)
26-54: LGTM!The
SnapshotMetadatadataclass andSnapshotManagerclass docstring are well-structured with clear type hints and documentation of features.
86-103: LGTM!The security enforcement and helper methods are correctly implemented. Using explicit
chmod()after directory creation properly addresses the umask concern.
105-176: LGTM!The package detection methods follow a clean pattern with proper timeout handling. The use of
sys.executable -m pipensures consistency with the running Python interpreter.
178-213: LGTM!System info gathering has appropriate timeout protection and graceful error handling for non-critical metadata.
215-272: LGTM!Snapshot creation is well-implemented with proper security measures (explicit
chmodfor directories, secure file creation usingos.openwith mode0o600), and appropriate retention policy enforcement.
402-468: Solid restore implementation with appropriate safeguards.The restore logic properly validates sudo permissions upfront (lines 420-428), provides dry-run capability, and has comprehensive error handling for timeouts and command failures. Based on learnings, sudo validation before execution is appropriate.
One consideration: the method executes all package managers sequentially. If APT restore succeeds but PIP fails, the system is left in a partial state. Consider documenting this behavior or implementing transactional semantics.
470-486: LGTM!The retention policy correctly enforces the configured limit by removing the oldest snapshots first.
| def _load_retention_from_preferences(self) -> int: | ||
| """Load retention limit from user preferences""" | ||
| try: | ||
| from cortex.user_preferences import PreferencesManager | ||
| prefs_manager = PreferencesManager() | ||
| prefs = prefs_manager.get_preferences() | ||
| return prefs.snapshots.retention_limit | ||
| except Exception: | ||
| # Fall back to default if preferences not available | ||
| return self.DEFAULT_RETENTION_LIMIT |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for get_preferences method in user_preferences.py
rg -n "def get_preferences" cortex/Repository: cortexlinux/cortex
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for the PreferencesManager class and its methods
rg -n "class PreferencesManager" cortex/ -A 30Repository: cortexlinux/cortex
Length of output: 2060
🏁 Script executed:
#!/bin/bash
# Search for all method definitions in PreferencesManager
rg -n "def " cortex/user_preferences.py | head -20Repository: cortexlinux/cortex
Length of output: 722
get_preferences() method does not exist on PreferencesManager.
Line 80 calls prefs_manager.get_preferences(), but PreferencesManager exposes the preferences via the .preferences attribute or the .get() method for dot-notation access—not a get_preferences() method. This will raise an AttributeError at runtime.
🔎 Proposed fix
def _load_retention_from_preferences(self) -> int:
"""Load retention limit from user preferences"""
try:
from cortex.user_preferences import PreferencesManager
prefs_manager = PreferencesManager()
- prefs = prefs_manager.get_preferences()
- return prefs.snapshots.retention_limit
+ return prefs_manager.preferences.snapshots.retention_limit
except Exception:
# Fall back to default if preferences not available
return self.DEFAULT_RETENTION_LIMITOr using the dot-notation getter:
return prefs_manager.get("snapshots.retention_limit", self.DEFAULT_RETENTION_LIMIT)🤖 Prompt for AI Agents
In cortex/snapshot_manager.py around lines 75 to 84, the code calls
prefs_manager.get_preferences() which does not exist on PreferencesManager and
will raise AttributeError; replace that call by reading the preferences via the
correct API (either use prefs_manager.preferences to access the preferences
object and then read snapshots.retention_limit with a fallback, or use the
dot-notation getter like prefs_manager.get("snapshots.retention_limit",
self.DEFAULT_RETENTION_LIMIT)) so the retention value is retrieved using the
correct method and a default is returned when the key is missing.
| auto_update: AutoUpdateSettings = field(default_factory=AutoUpdateSettings) | ||
| ai: AISettings = field(default_factory=AISettings) | ||
| packages: PackageSettings = field(default_factory=PackageSettings) | ||
| snapshots: SnapshotSettings = field(default_factory=SnapshotSettings) |
There was a problem hiding this comment.
Snapshots field not persisted in load/save operations.
The snapshots field is added to UserPreferences, but the load() method (lines 142-154) and save() method (lines 174-186) don't include handling for the snapshots key. This means user-configured snapshot preferences won't survive across sessions—they'll reset to defaults each time.
The same issue affects export_json(), import_json(), and get_all_settings().
🔎 Proposed fix for load() and save()
In load() around line 150, add:
packages=PackageSettings(**data.get("packages", {})),
+ snapshots=SnapshotSettings(**data.get("snapshots", {})),
theme=data.get("theme", "default"),In save() around line 182, add:
"packages": asdict(self.preferences.packages),
+ "snapshots": asdict(self.preferences.snapshots),
"theme": self.preferences.theme,Apply similar changes to export_json(), import_json(), and get_all_settings().
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cortex/user_preferences.py around line 103, the new snapshots:
SnapshotSettings field is not persisted because load(), save(), export_json(),
import_json(), and get_all_settings() do not handle the "snapshots" key; update
each of those methods to read/write the snapshots state. Specifically, in load()
parse the snapshots dict (if present) and assign UserPreferences.snapshots =
SnapshotSettings.from_dict(...) or equivalent, in save() include snapshots:
self.snapshots.to_dict() in the serialized dict, in export_json() include
snapshots in the exported payload, in import_json() accept and deserialize
snapshots into SnapshotSettings, and in get_all_settings() return the snapshots
representation alongside other settings so user snapshot preferences persist
across sessions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cortex/snapshot_manager.py (2)
85-94:get_preferences()method still doesn't exist onPreferencesManager.Line 90 calls
prefs_manager.get_preferences(), butPreferencesManagerexposes preferences via the.preferencesattribute, not aget_preferences()method. This will raise anAttributeErrorat runtime.🔎 Proposed fix
def _load_retention_from_preferences(self) -> int: """Load retention limit from user preferences""" try: from cortex.user_preferences import PreferencesManager prefs_manager = PreferencesManager() - prefs = prefs_manager.get_preferences() - return prefs.snapshots.retention_limit + return prefs_manager.preferences.snapshots.retention_limit except Exception: # Fall back to default if preferences not available return self.DEFAULT_RETENTION_LIMIT
494-508: Fix pip and npm command inconsistencies.Two issues remain unresolved from previous reviews:
Pip commands don't use
sys.executable: Lines 496 and 501 use plain"pip"but detection at line 210 usessys.executable -m pip, which can cause restoration to use a different pip binary than detection.NPM commands lack sudo prefix: Lines 497 and 502 don't include
"sudo"for global operations, despite the code checking sudo availability (line 543) and apt commands including sudo. Global npm operations typically require administrator privileges.🔎 Proposed fix
remove_cmds = { "apt": ["sudo", "apt-get", "remove", "-y"], - "pip": ["pip", "uninstall", "-y"], - "npm": ["npm", "uninstall", "-g"], + "pip": [sys.executable, "-m", "pip", "uninstall", "-y"], + "npm": ["sudo", "npm", "uninstall", "-g"], } install_cmds = { "apt": ["sudo", "apt-get", "install", "-y"], - "pip": ["pip", "install"], - "npm": ["npm", "install", "-g"], + "pip": [sys.executable, "-m", "pip", "install"], + "npm": ["sudo", "npm", "install", "-g"], }tests/unit/test_snapshot_manager.py (1)
250-262: Fix malformed snapshot IDs and timestamps for i >= 10.Lines 252 and 255 still create invalid formats when
i >= 10:
- For i=10:
snapshot_id="20250102_1200010"(7 digits in time portion)- For i=10:
timestamp="2025-01-02T12:00:010"(3 digits in seconds)The seconds portion needs zero-padding to maintain exactly 2 digits.
🔎 Proposed fix
# Create 12 snapshots manually (exceeds limit of 10) for i in range(12): - snapshot_id = f"2025010{(i % 9) + 1}_12000{i}" + day = (i % 28) + 1 + snapshot_id = f"202501{day:02d}_1200{i:02d}" metadata = { "id": snapshot_id, - "timestamp": f"2025-01-0{(i % 9) + 1}T12:00:0{i}", + "timestamp": f"2025-01-{day:02d}T12:00:{i:02d}", "description": f"Snapshot {i}",
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cortex/snapshot_manager.pyrequirements.txttest_concurrent_simple.pytest_multiprocess_race.pytests/unit/test_snapshot_manager.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
test_concurrent_simple.pytest_multiprocess_race.pytests/unit/test_snapshot_manager.pycortex/snapshot_manager.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain >80% test coverage for pull requests
Files:
tests/unit/test_snapshot_manager.py
🧠 Learnings (2)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to tests/**/*.py : Maintain >80% test coverage for pull requests
Applied to files:
tests/unit/test_snapshot_manager.py
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : No silent sudo execution - require explicit user confirmation
Applied to files:
cortex/snapshot_manager.py
🧬 Code graph analysis (2)
test_concurrent_simple.py (1)
cortex/sandbox/sandbox_executor.py (1)
success(74-76)
test_multiprocess_race.py (1)
cortex/snapshot_manager.py (1)
create_snapshot(256-352)
🔇 Additional comments (13)
test_concurrent_simple.py (2)
8-66: LGTM! Well-structured concurrency test.The test properly spawns concurrent snapshot creation processes, aggregates results, and checks for JSON corruption patterns. The 60-second timeout per process is reasonable for snapshot operations.
68-76: LGTM! Proper exception handling and exit codes.The main block correctly handles exceptions, prints errors with traceback, and returns appropriate exit codes.
cortex/snapshot_manager.py (4)
117-144: LGTM! Robust JSON reading with retry logic.The retry mechanism with brief delays effectively handles transient JSON corruption that can occur during concurrent writes. Error handling and logging are appropriate.
256-352: LGTM! Well-designed concurrent snapshot creation.The implementation effectively handles multi-process concurrency through:
- UUID-based IDs to prevent collisions
- File locking only for the critical metadata write section
- Atomic writes via temp file + rename
- Proper cleanup on failure
- Secure permissions (700/600)
522-588: LGTM! Comprehensive restore logic with proper error handling.The restore implementation includes:
- Sudo permission check before attempting restore
- Timeout protection (300s) for package operations
- Proper exception handling for TimeoutExpired and CalledProcessError
- Dry-run support for preview
- Detailed error messages
The command execution is correct once the command templates in
_restore_package_manager()are fixed.
590-610: LGTM! Correct retention policy implementation.The retention logic properly:
- Sorts snapshots by timestamp (oldest first)
- Deletes excess snapshots beyond the limit
- Uses unsafe methods correctly (already within locked context)
- Handles errors gracefully with logging
test_multiprocess_race.py (2)
9-30: LGTM! Clean subprocess wrapper with proper error handling.The helper function correctly:
- Executes snapshot creation in a separate process
- Applies a reasonable 60-second timeout
- Returns structured result with all relevant information
- Handles exceptions gracefully
32-87: LGTM! Effective multi-process race condition test.The test properly:
- Uses ProcessPoolExecutor for true parallel process execution
- Aggregates results as futures complete
- Detects JSON corruption patterns
- Provides detailed summary with timing and success/failure counts
- Returns appropriate exit codes
tests/unit/test_snapshot_manager.py (5)
1-109: LGTM! Well-structured test foundation.The test setup includes:
- Proper temporary directory management
- Reusable helper methods for creating mock snapshots and metadata
- Clean separation of concerns
- Appropriate use of mocks
110-240: LGTM! Comprehensive CRUD and detection tests.The tests cover:
- Package detection for all three managers (APT, PIP, NPM)
- Snapshot creation with proper verification
- Listing, retrieval, and deletion operations
- Edge cases like empty lists and non-existent snapshots
271-366: LGTM! Thorough restore functionality tests.The tests validate:
- Dry-run mode with command preview
- Snapshot not found error handling
- Snapshot ID format requirements (date, time, UUID parts)
- Live restore execution with subprocess mocking
- Proper command execution verification
368-533: LGTM! Excellent error handling test coverage.The tests comprehensively cover:
- System info detection failures
- CalledProcessError with and without stderr
- Sudo permission check failures and exceptions
- Package detection timeouts, FileNotFoundError, and JSON decode errors
- Exception handling throughout the snapshot lifecycle
This contributes significantly to the >80% coverage requirement.
535-587: LGTM! Complete error recovery test coverage.The final tests validate:
- Snapshot creation exception handling with proper cleanup
- Restore timeout handling with informative error messages
- Timeout duration included in error message
The test suite provides comprehensive coverage of both happy paths and error scenarios, meeting the >80% coverage requirement.
cortex -h # Standard argparse help cortex install -h # Install command details cortex snapshot -h # Snapshot subcommands cortex stack -h # Stack management details
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cortex/cli.py (1)
763-770: Minor: Redundant fallback for description.At line 764,
args.description or ""is technically redundant since the argparse definition at line 1129 already setsdefault="". However, this is harmless defensive coding that protects against potential future changes to the argument definition.🔎 Optional simplification
if args.snapshot_action == "create": - success, snapshot_id, message = manager.create_snapshot(args.description or "") + success, snapshot_id, message = manager.create_snapshot(args.description)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cortex/cli.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
🔇 Additional comments (4)
cortex/cli.py (4)
750-839: LGTM - Snapshot method implementation is well-structured.The method correctly handles all snapshot actions with appropriate error handling and return values. The lazy import of
SnapshotManageris acceptable for reducing startup time. Previous feedback on ID column width has been addressed (24 chars at line 779).Minor observations:
- Line 839's
return 1serves as a defensive fallback for any unhandled action, which is good practice.- Error messages are consistent and informative.
842-952: LGTM - Rich help display is comprehensive and well-organized.The updated help includes snapshot commands in the Core Commands panel with clear action descriptions. The three-panel structure (Core, Utility, Configuration) improves discoverability, and the examples section now demonstrates snapshot usage.
1115-1161: LGTM - Snapshot subparsers are well-defined.The snapshot command hierarchy is properly structured with:
createwith optional descriptionlistfor viewing all snapshotsshowrequiring snapshot_idrestorewith --dry-run supportdeleterequiring snapshot_idThe descriptions are informative and help text follows conventions used elsewhere in the CLI.
1206-1207: LGTM - Snapshot command routing follows established patterns.The routing is consistent with how other subcommand-based commands (stack, notify) are handled.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
requirements.txt (1)
9-9: Remove duplicate PyYAML dependency with conflicting version specifiers.The file contains two separate PyYAML entries:
- Line 9:
PyYAML>=6.0.0- Line 22:
PyYAML==6.0.3This creates ambiguity in dependency resolution and violates the DRY principle. The pinned version (==6.0.3) should be the single source of truth. Remove line 9 to keep only the explicit version constraint on line 22.
🔎 Proposed fix: Remove duplicate PyYAML entry
# Configuration -PyYAML>=6.0.0 # Environment variable loading from .env filesAlso applies to: 22-22
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
requirements.txt
🔇 Additional comments (1)
requirements.txt (1)
17-18: ✅ Filelock CVE-2025-68146 is properly patched.The upgrade to
filelock>=3.20.1correctly addresses the critical TOCTOU (Time-of-Check-Time-of-Use) vulnerability reported in the previous review. The version constraint is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MULTIPROCESS_RACE_FIX.md (1)
7-12: Add language identifier to code block for markdown compliance.The fenced code block is missing a language identifier. Add
bashto the opening code fence to comply with markdown linting standards.🔎 Proposed fix
-``` +```bash for i in {1..5}; do cortex snapshot create "Race test $i" & done wait -``` +```bash
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
COMMIT_MSG.txtMULTIPROCESS_RACE_FIX.mdcortex/demo.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cortex/demo.py
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
MULTIPROCESS_RACE_FIX.md
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
COMMIT_MSG.txt (1)
1-35: Good structure, but verify that implementation files are included in review scope.The commit message is well-organized and clearly documents the problem, root cause, and solution. However, since the actual implementation file (
cortex/snapshot_manager.py) and test files referenced here are not included in the files under review, the complete validation of these changes cannot be performed.Please confirm whether
cortex/snapshot_manager.py,test_concurrent_simple.py, andtest_multiprocess_race.pyshould be included in this review. The commit message references concrete changes to these files that need to be validated against the actual code.MULTIPROCESS_RACE_FIX.md (1)
1-202: Excellent technical documentation with clear problem-to-solution narrative.The documentation comprehensively explains the multi-process race condition, root cause analysis, and the optimized solution with file-based locking. The performance metrics (50x faster lock duration, unlimited concurrent capacity) and protection layers table provide strong evidence of the fix's effectiveness.
Key strengths:
- Clear before/after comparison with concrete examples
- Realistic performance improvements documented
- Verification steps are comprehensive and actionable
- Edge cases properly identified
However, the actual implementation in
cortex/snapshot_manager.pyis not provided, so the specific integration ofFileLock,_safe_read_json(), and_file_lock_path()cannot be validated against this documentation.Please include
cortex/snapshot_manager.pyin the review so that:
- The actual FileLock usage matches the described timeout and retry logic
- The
_safe_read_json()implementation handles the 3-attempt retry with 100ms delays as documented- The lock placement truly minimizes critical section time (aiming for ~0.2s as claimed)
- All create/list/delete operations properly wrap JSON operations with FileLock contexts
|
3 participants
Related Issue
Closes #45
Summary
This PR implements a comprehensive system snapshot and rollback feature for Cortex Linux. Users can now create system snapshots before installations, list existing snapshots, and restore to previous states if something goes wrong.
Key Features
Implementation Details
SnapshotManagerclass incortex/snapshot_manager.py(398 lines)create,list,show,restore,delete~/.cortex/snapshots/with JSON metadataSecurity Fixes
Usage Examples
# List all snapshots cortex snapshot list# Restore to a previous state (dry-run) cortex snapshot restore 20251222_160045_531066 --dry-run# Restore to a previous state cortex snapshot restore 20251222_160045_531066# Delete a snapshot cortex snapshot delete 20251222_160045_531066Test Results
Dependency Resolver Tests (9/9 passed)
test_basic_conflict_resolution - Semantic version conflict resolution
test_missing_keys_raises_keyerror - Required field validation
test_invalid_semver_produces_error_strategy - Invalid version handling
test_resolution_with_default_version - Default version support
test_complex_version_constraints - Complex constraint resolution
test_wildcard_version - Wildcard version support
test_major_version_conflict - Major version conflict handling
test_recommended_strategy_targets_package_b - Strategy validation
test_alternative_strategy_targets_dependency - Alternative strategies
Snapshot Manager Tests (27/27 passed)
Snapshot creation and exception handling
List, get, and delete snapshot operations
APT/PIP/NPM package detection
Dry-run and live restore operations
Error handling and timeout scenarios
System info retrieval and validation
Retention policy enforcement
Overall Test Suite
706 passed, 10 skipped in 40.28s
Summary by CodeRabbit
New Features
Documentation
Other Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.