[pin] Implement package version pinning system #627
Conversation
DipeshDalmia
requested review from
Anshgrover23,
Suyashd999 and
mikejmorgan-ai
as code owners
|
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 package-version pinning system and CLI integration: new PinManager module with persistence, validation, apt synchronization, import/export, update-blocking checks; CLI gains Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CortexCLI
participant PM as PinManager
participant Storage as "Pin File (JSON)"
participant APT as apt-mark
User->>CLI: cortex pin add postgresql@14.10 --reason "stability"
CLI->>PM: parse_package_spec("postgresql@14.10")
CLI->>PM: add_pin("postgresql","14.10",reason,PinType.EXACT,...)
PM->>PM: validate package & version
PM->>Storage: load pins
PM->>Storage: write updated pins
alt sync_apt true
PM->>APT: apt-mark hold postgresql
APT-->>PM: success
end
PM-->>CLI: (True, "Pinned postgresql to 14.10")
CLI-->>User: ✓ Pinned postgresql to 14.10
sequenceDiagram
participant User
participant CLI as CortexCLI
participant PM as PinManager
participant Installer as Package Installer
participant Storage as "Pin File (JSON)"
User->>CLI: cortex install postgresql
CLI->>PM: get_pin_manager()
CLI->>PM: check_update_allowed("postgresql", candidate_version)
PM->>Storage: load pins
PM->>PM: match candidate_version vs pin constraints
alt pinned and not forced
PM-->>CLI: PinCheckResult(allowed=false, reason="pinned to 14.10")
CLI-->>User: ⊘ blocked: postgresql pinned to 14.10 (use --force to override)
else forced
CLI-->>User: ⚠️ Confirm override? [y/N]
User->>CLI: y
CLI->>Installer: proceed install
Installer-->>CLI: installation result
CLI-->>User: ✓ Installed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CLA Verification PassedAll contributors have signed the CLA.
|
Summary of ChangesHello @DipeshDalmia, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a critical new feature to Cortex: a robust package version pinning system. This system empowers users to lock down specific package versions, thereby preventing unintended updates that could compromise system stability or compatibility. It provides a complete set of command-line tools for managing these pins and seamlessly integrates with the existing installation workflow to enforce version constraints, offering greater control and predictability over the software environment. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive package version pinning system, which is a valuable feature for ensuring environment stability. The implementation is well-structured, with a dedicated manager class, CLI integration, tests, and documentation. However, I've identified a few issues, including a critical bug in the version matching logic, some inconsistencies in the CLI, and documentation that doesn't match the implementation. Addressing these points will significantly improve the robustness and user experience of this new feature.
| def _match_minor(self, pinned: str, candidate: str) -> bool: | ||
| """ | ||
| Match minor version pattern. | ||
| E.g., "14.*" matches "14.0", "14.10", "14.10.1" | ||
| """ | ||
| # Handle patterns like "14.*" or just "14" | ||
| pattern = pinned.replace(".*", "").replace("*", "") | ||
|
|
||
| # Extract major.minor from pinned version | ||
| pinned_parts = pattern.split(".") | ||
| candidate_parts = candidate.split(".") | ||
|
|
||
| # Compare major and minor (first two parts) | ||
| try: | ||
| for i in range(min(2, len(pinned_parts))): | ||
| if i >= len(candidate_parts): | ||
| return False | ||
| # Remove any non-numeric suffix for comparison | ||
| pinned_num = re.match(r"(\d+)", pinned_parts[i]) | ||
| candidate_num = re.match(r"(\d+)", candidate_parts[i]) | ||
|
|
||
| if not pinned_num or not candidate_num: | ||
| return False | ||
|
|
||
| if pinned_num.group(1) != candidate_num.group(1): | ||
| return False | ||
|
|
||
| return True | ||
| except (IndexError, ValueError): | ||
| return False |
There was a problem hiding this comment.
The implementation of _match_minor is overly complex and contains a bug. For a pinned version like "14.*", pinned_parts becomes ['14', '']. When the loop gets to the second part (''), re.match(r"(\d+)", '') returns None, causing the function to incorrectly return False for any candidate version.
This logic can be greatly simplified and corrected. A minor version pin should just check that the candidate version's parts match the pinned version's defined parts.
Here is a suggested simpler and correct implementation:
def _match_minor(self, pinned: str, candidate: str) -> bool:
"""
Match minor version pattern.
E.g., "14.*" matches "14.0", "14.10", "14.10.1"
"""
# Handle patterns like "14.*" or "14.1*"
base_pinned = pinned.split('*')[0]
if base_pinned.endswith('.'):
base_pinned = base_pinned[:-1]
pinned_parts = base_pinned.split('.')
candidate_parts = candidate.split('.')
if len(candidate_parts) < len(pinned_parts):
return False
# Compare the defined parts
for i in range(len(pinned_parts)):
# This simple comparison assumes version parts are numeric and doesn't handle 'rc', 'beta' etc.
# For more complex versions, a proper semver library is recommended.
if pinned_parts[i] != candidate_parts[i]:
return False
return True| # Auto-detect from version string | ||
| if "," in version or any(op in version for op in [">=", "<=", ">", "<", "!="]): | ||
| pin_type = PinType.RANGE | ||
| elif version.endswith(".*") or "*" in version: | ||
| pin_type = PinType.MINOR | ||
| else: | ||
| pin_type = PinType.EXACT |
There was a problem hiding this comment.
The auto-detection logic for the pin type is incomplete. It correctly identifies range and minor pins, but it will incorrectly classify a major version pin (e.g., postgresql@14) as exact. This can lead to unexpected behavior for users.
The pin_manager.py file contains a more robust detection logic in its private _detect_pin_type method. To fix this and avoid code duplication, I recommend making that logic available as a public utility function in pin_manager.py and calling it here.
For example, you could create a standalone function detect_pin_type_from_version in pin_manager.py and use it here to ensure pin types are detected consistently and correctly.
| def _match_range(self, pinned: str, candidate: str) -> bool: | ||
| """ | ||
| Match semver range constraints. | ||
| E.g., ">=3.11,<3.12" or ">=1.0.0" | ||
| """ | ||
| try: | ||
| candidate_tuple = self._parse_version(candidate) | ||
| if candidate_tuple is None: | ||
| return False | ||
|
|
||
| # Parse constraints | ||
| constraints = [c.strip() for c in pinned.split(",")] | ||
|
|
||
| for constraint in constraints: | ||
| if not self._check_constraint(constraint, candidate_tuple): | ||
| return False | ||
|
|
||
| return True | ||
| except Exception: | ||
| return False | ||
|
|
||
| def _parse_version(self, version: str) -> tuple[int, ...] | None: | ||
| """Parse version string to tuple of integers""" | ||
| try: | ||
| # Extract numeric parts | ||
| match = re.match(r"(\d+)(?:\.(\d+))?(?:\.(\d+))?", version) | ||
| if not match: | ||
| return None | ||
|
|
||
| parts = [int(p) if p else 0 for p in match.groups()] | ||
| return tuple(parts) | ||
| except (ValueError, AttributeError): | ||
| return None | ||
|
|
||
| def _check_constraint(self, constraint: str, version_tuple: tuple[int, ...]) -> bool: | ||
| """Check a single version constraint""" | ||
| # Parse operator and version | ||
| match = re.match(r"(>=|<=|>|<|==|!=|~=)?\s*(.+)", constraint) | ||
| if not match: | ||
| return False | ||
|
|
||
| operator = match.group(1) or "==" | ||
| constraint_version = self._parse_version(match.group(2)) | ||
|
|
||
| if constraint_version is None: | ||
| return False | ||
|
|
||
| # Pad tuples to same length | ||
| max_len = max(len(version_tuple), len(constraint_version)) | ||
| v = version_tuple + (0,) * (max_len - len(version_tuple)) | ||
| c = constraint_version + (0,) * (max_len - len(constraint_version)) | ||
|
|
||
| if operator == ">=": | ||
| return v >= c | ||
| elif operator == "<=": | ||
| return v <= c | ||
| elif operator == ">": | ||
| return v > c | ||
| elif operator == "<": | ||
| return v < c | ||
| elif operator == "==": | ||
| return v == c | ||
| elif operator == "!=": | ||
| return v != c | ||
| elif operator == "~=": | ||
| # Compatible release: ~=1.4.2 means >=1.4.2,<1.5.0 | ||
| return v >= c and v[:2] == c[:2] | ||
|
|
||
| return False |
There was a problem hiding this comment.
The _match_range method and its helpers (_parse_version, _check_constraint) implement a custom semantic version range checker. Rolling your own semver logic is notoriously difficult and prone to errors, especially with edge cases like pre-release tags (-beta, -rc1) and build metadata (+build123), which are not handled here.
To ensure correctness and robustness, I strongly recommend using a dedicated, well-tested library like semver.
Here's how you could refactor _match_range using the semver library:
# At the top of the file:
# import semver
# ...
def _match_range(self, pinned: str, candidate: str) -> bool:
"""
Match semver range constraints using the 'semver' library.
E.g., ">=3.11,<3.12" or ">=1.0.0"
"""
try:
# The semver library can parse and match directly.
return semver.match(candidate, pinned)
except (TypeError, ValueError):
# Handle invalid version strings gracefully.
logger.warning(f"Could not compare versions '{candidate}' and '{pinned}' with semver.")
return FalseThis would make the implementation much simpler, more reliable, and easier to maintain. You would need to add semver to your project's dependencies.
| # Check if update is blocked | ||
| cortex pin check nginx 1.25.0 | ||
| ``` |
There was a problem hiding this comment.
The documentation mentions a cortex pin check command, but this subcommand is not implemented in cortex/cli.py. The same applies to cortex pin show mentioned later in the document. This will lead to user confusion. Please either implement these commands or remove them from the documentation.
| cx_header("Pinned Packages") | ||
| console.print() | ||
|
|
||
| from rich.table import Table |
There was a problem hiding this comment.
The import from rich.table import Table is performed inside the _pin_list method. It's generally better practice to place all imports at the top of the file for clarity, readability, and to avoid repeated import overhead. Since rich is a core dependency, this import can be safely moved to the top of cortex/cli.py.
| force = getattr(args, "force", False) | ||
|
|
||
| if not force: | ||
| confirm = input("⚠️ Clear ALL pins? (y/N): ") |
There was a problem hiding this comment.
For consistency with the rest of the CLI, please use console.input() from the rich library instead of the built-in input(). Other parts of the code, like the confirmation for install --force, already use console.input().
| confirm = input("⚠️ Clear ALL pins? (y/N): ") | |
| confirm = console.input("⚠️ Clear ALL pins? (y/N): ") |
| version=data["version"], | ||
| pin_type=PinType(data.get("pin_type", "exact")), | ||
| source=PackageSource(data.get("source", "apt")), | ||
| pinned_at=data.get("pinned_at", datetime.now().isoformat()), |
There was a problem hiding this comment.
When creating a PinConfiguration from a dictionary, if pinned_at is missing, it defaults to datetime.now().isoformat(). This can be misleading when importing older pin configurations that don't have this field, as it will make them appear as if they were pinned today.
A better approach would be to allow pinned_at to be optional. Consider changing the type hint for pinned_at to str | None and adjusting the logic accordingly.
| pinned_at=data.get("pinned_at", datetime.now().isoformat()), | |
| pinned_at=data.get("pinned_at"), |
| "pins": [pin.to_dict() for pin in self._pins.values()], | ||
| "metadata": { | ||
| "last_modified": datetime.now().isoformat(), | ||
| "cortex_version": "0.2.0", |
There was a problem hiding this comment.
The cortex_version is hardcoded as "0.2.0" in the metadata when saving pins. This should be dynamically retrieved from a central version definition to avoid it becoming stale. You can import VERSION from cortex.branding for this.
| "cortex_version": "0.2.0", | |
| "cortex_version": VERSION, |
| **Example Output:** | ||
|
|
||
| ``` | ||
| 📌 Pinned Packages | ||
| postgresql: 14.10 (apt) | ||
| pinned 5 days ago | ||
| Reason: Production database | ||
| python3: 3.11.* (minor version) (apt) | ||
| pinned 10 days ago | ||
| Reason: ML dependencies require Python 3.11 | ||
| ✓ Synced with apt-mark hold | ||
| flask: 2.0.0 (pip) | ||
| pinned today | ||
| Total: 3 pinned package(s) | ||
| ``` |
There was a problem hiding this comment.
The example output for cortex pin list in the documentation is a formatted text list. However, the actual implementation in cortex/cli.py (_pin_list method) generates a rich table. The documentation should be updated to reflect the actual output of the command to avoid confusing users.
![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
🤖 Fix all issues with AI agents
In `@cortex/pin_manager.py`:
- Around line 657-683: Both _apt_mark_hold and _apt_mark_unhold currently run
"sudo" silently; change them to require explicit confirmation by adding a
boolean parameter (e.g., confirmed: bool = False) and skip execution (return
False and log a warning) unless confirmed is True, so the CLI layer can prompt
the user and call _apt_mark_hold(package, confirmed=True) or
_apt_mark_unhold(package, confirmed=True); update both function signatures and
their callers accordingly (do not perform sudo inside these functions when
confirmed is False).
- Around line 163-183: The pin persistence methods currently write to disk but
do not record audit entries; update _save_pins (and the mutating entry points
add_pin, remove_pin, clear_all_pins, import_pins) to create an
InstallationHistory audit record using InstallationHistory.record(...) with
InstallationType.CONFIG after a successful save. Specifically, after _save_pins
returns True, call InstallationHistory.record with a concise message describing
the mutation (e.g., "added pin <id>", "removed pin <id>", "cleared all pins", or
"imported N pins") and include any relevant metadata (pin ids/counts) so the
audit DB (~/.cortex/history.db) reflects the change; follow the pattern used in
role_manager.py for invoking InstallationHistory and InstallationType.CONFIG.
Ensure failures to save do not write an audit entry.
- Around line 476-478: The compatible-release branch handling operator "~="
currently fixes the prefix length to two components (v[:2] == c[:2]) which fails
for specifiers with different component counts; update the check in the "~="
branch to compare all components except the last one by replacing the hardcoded
slice with a dynamic slice using len(c) (i.e., ensure v[:len(c)-1] ==
c[:len(c)-1]) so that for any constraint components in variable c the prefix
match uses all but the final component while keeping the existing >= comparison
(v >= c).
In `@tests/unit/test_pin_manager.py`:
- Around line 374-409: Add a new unit test in tests/unit/test_pin_manager.py
that exercises the compatible-release operator "~=" using the same style as
existing range tests: create a PinConfiguration with pin_type=PinType.RANGE and
version="~=X.Y.Z" (e.g., "~=1.4.2") and call pin_manager.version_matches_pin to
assert that versions >=1.4.2 and <1.5.0 return True while versions below 1.4.2
or at/above 1.5.0 return False; reference the PinConfiguration class,
PinType.RANGE, and the version_matches_pin method to locate where to add the
test.
🧹 Nitpick comments (6)
docs/PIN_MANAGEMENT.md (2)
105-121: Add language specifier to fenced code blocks.The static analysis tool flagged these example output blocks as missing language specifiers. For shell/terminal output examples, use
textorconsoleas the language identifier for better markdown rendering.📝 Suggested fix
-``` +```text 📌 Pinned PackagesSimilarly for lines 133-144.
370-373: Clarify sudo requirement for apt-mark sync.The troubleshooting section suggests running
sudo cortex pin sync, but according to the coding guidelines, silent sudo should be avoided and explicit user confirmation is required for privilege escalation. The documentation should clarify that the command will prompt for confirmation rather than suggesting users run the entire command under sudo.📝 Suggested clarification
### apt-mark sync failed? -Ensure you have sudo permissions: -```bash -sudo cortex pin sync -``` +The sync operation requires elevated privileges. When running `cortex pin sync`, +you will be prompted to confirm the sudo operation: +```bash +cortex pin sync +# Prompts: "Run apt-mark hold for N packages? (y/N)" +``` </details> </blockquote></details> <details> <summary>cortex/pin_manager.py (2)</summary><blockquote> `147-161`: **Consider handling file permission errors more gracefully.** When loading pins fails due to JSON decode errors, the code silently resets to empty pins. This could cause data loss if a user has a corrupted file but expects their pins to still be there. Consider backing up the corrupted file or warning the user more prominently. ```diff except (json.JSONDecodeError, KeyError) as e: logger.error(f"Error loading pins file: {e}") + # Backup corrupted file for recovery + if self.pin_file.exists(): + backup_path = self.pin_file.with_suffix(".json.bak") + try: + import shutil + shutil.copy(self.pin_file, backup_path) + logger.warning(f"Backed up corrupted pins file to {backup_path}") + except OSError: + pass self._pins = {}
704-711: Package name validation regex may be too permissive.The regex
^[@A-Za-z0-9][\w\-\./@]*$allows forward slashes anywhere in the package name, which could potentially be exploited for path traversal when the package name is used in file operations or shell commands.Consider tightening the validation to only allow
@at the start (for scoped npm packages) and/only after the initial@:- pattern = r"^[`@a-zA-Z0-9`][\w\-\./@]*$" + # Allow scoped npm packages (`@scope/name`) but prevent path traversal + pattern = r"^(@[a-zA-Z0-9][\w\-\.]*\/)?[a-zA-Z0-9][\w\-\.]*$"tests/unit/test_pin_manager.py (1)
160-173: Consider usingpytest.fixturewithtmp_pathinstead of manual cleanup.The current fixture manually creates and cleans up temporary files. Using pytest's built-in
tmp_pathfixture would be cleaner and automatically handle cleanup:♻️ Suggested refactor
`@pytest.fixture` -def temp_pin_file(self): - """Create a temporary pin file for testing.""" - with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as f: - f.write('{"version": "1.0", "pins": []}') - temp_path = Path(f.name) - yield temp_path - if temp_path.exists(): - temp_path.unlink() +def temp_pin_file(self, tmp_path): + """Create a temporary pin file for testing.""" + temp_file = tmp_path / "pins.json" + temp_file.write_text('{"version": "1.0", "pins": []}') + return temp_fileThis pattern is repeated in multiple test classes. However, this is a minor improvement and the current approach works correctly.
cortex/cli.py (1)
883-929: Pin checking integration looks correct but could use audit logging.The pin check integration in the install flow:
- Correctly blocks installation when packages are pinned and
--forceis not provided- Prompts for confirmation when
--forceis used with pinned packages- Handles KeyboardInterrupt and EOFError gracefully
However, per the coding guidelines, pin override operations should be logged for audit purposes. Consider recording when a user forces installation of a pinned package.
📝 Suggested addition
if pinned_packages: # ... existing confirmation logic ... # Log the force override for audit if force and execute: history = InstallationHistory() # Record pin override in audit log logger.info(f"User overrode pins for: {[pkg for pkg, _ in pinned_packages]}")
| def _save_pins(self) -> bool: | ||
| """Save pins to file""" | ||
| try: | ||
| data = { | ||
| "version": self.PIN_FILE_VERSION, | ||
| "pins": [pin.to_dict() for pin in self._pins.values()], | ||
| "metadata": { | ||
| "last_modified": datetime.now().isoformat(), | ||
| "cortex_version": "0.2.0", | ||
| }, | ||
| } | ||
|
|
||
| with open(self.pin_file, "w") as f: | ||
| json.dump(data, f, indent=2) | ||
|
|
||
| logger.info(f"Saved {len(self._pins)} pins to {self.pin_file}") | ||
| return True | ||
|
|
||
| except OSError as e: | ||
| logger.error(f"Error saving pins file: {e}") | ||
| return False |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, search for InstallationHistory and audit logging patterns
rg -n "InstallationHistory" --type=py -C2Repository: cortexlinux/cortex
Length of output: 4278
🏁 Script executed:
# Search for references to the audit database file
rg -n "history\.db" --type=py -C2Repository: cortexlinux/cortex
Length of output: 4544
🏁 Script executed:
# Check the pin_manager.py file more broadly for any audit logging
rg -n "audit|history" cortex/pin_manager.py --type=pyRepository: cortexlinux/cortex
Length of output: 44
🏁 Script executed:
# Search for common audit logging patterns
rg -n "\.cortex/history" --type=pyRepository: cortexlinux/cortex
Length of output: 44
🏁 Script executed:
# Read pin_manager.py to understand its full context
wc -l cortex/pin_manager.pyRepository: cortexlinux/cortex
Length of output: 87
🏁 Script executed:
# Get the full pin_manager.py file (check size first)
cat cortex/pin_manager.py | head -200Repository: cortexlinux/cortex
Length of output: 6096
🏁 Script executed:
# Check what operations exist in pin_manager.py and if any use audit logging
rg -n "def " cortex/pin_manager.py --type=pyRepository: cortexlinux/cortex
Length of output: 2433
🏁 Script executed:
# Check if pin_manager.py imports InstallationHistory or any audit logging
rg -n "import" cortex/pin_manager.py --type=py | head -20Repository: cortexlinux/cortex
Length of output: 764
🏁 Script executed:
# Check InstallationHistory to understand what operation types are available
grep -n "class InstallationType" cortex/installation_history.py -A 20Repository: cortexlinux/cortex
Length of output: 525
🏁 Script executed:
# Check if there are any other references to pin operations or audit logging
rg -n "pin" cortex/installation_history.py --type=py -iRepository: cortexlinux/cortex
Length of output: 44
🏁 Script executed:
# Verify all mutating methods in pin_manager and their current logging
rg -n "def (add_pin|remove_pin|clear_all|import_pins)" cortex/pin_manager.py -A 15 --type=py | head -80Repository: cortexlinux/cortex
Length of output: 2341
Add audit logging for pin configuration mutations.
The _save_pins method and related mutating operations (add_pin, remove_pin, clear_all_pins, import_pins) bypass the required audit logging to ~/.cortex/history.db. Per the coding guidelines, all operations must be logged to the audit database for compliance. Use InstallationHistory with InstallationType.CONFIG (as demonstrated in role_manager.py) to record pin configuration changes.
🤖 Prompt for AI Agents
In `@cortex/pin_manager.py` around lines 163 - 183, The pin persistence methods
currently write to disk but do not record audit entries; update _save_pins (and
the mutating entry points add_pin, remove_pin, clear_all_pins, import_pins) to
create an InstallationHistory audit record using InstallationHistory.record(...)
with InstallationType.CONFIG after a successful save. Specifically, after
_save_pins returns True, call InstallationHistory.record with a concise message
describing the mutation (e.g., "added pin <id>", "removed pin <id>", "cleared
all pins", or "imported N pins") and include any relevant metadata (pin
ids/counts) so the audit DB (~/.cortex/history.db) reflects the change; follow
the pattern used in role_manager.py for invoking InstallationHistory and
InstallationType.CONFIG. Ensure failures to save do not write an audit entry.
| elif operator == "~=": | ||
| # Compatible release: ~=1.4.2 means >=1.4.2,<1.5.0 | ||
| return v >= c and v[:2] == c[:2] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PEP 440 compatible release operator ~= specification
💡 Result:
Summary — the PEP 440 "compatible release" operator ~=
- Syntax: ~= . It means "this version or later, but only up to the next incompatible public release." [1]
- Semantics (short): ~=V.N is roughly equivalent to ">= V.N, == V.*". Appending more segments tightens the upper bound on the next-incompatible segment. [1][2]
- Common examples:
- ~=3.1 → >=3.1, <4.0 (i.e., >=3.1, ==3.*).
- ~=3.1.2 → >=3.1.2, <3.2.0 (i.e., >=3.1.2, ==3.1.*).
- Rules: it MUST NOT be used with a single-segment version like ~=1. If a pre/post/dev suffix appears in the specifier, the suffix is ignored for determining the prefix match (the lower bound still includes the suffix). [1][2]
Sources: PEP 440 (Version Identification and Dependency Specification). [1] PEP discussion/examples and community explanation. [2]
🏁 Script executed:
# Find and examine the cortex/pin_manager.py file around lines 476-478
find . -type f -name "pin_manager.py" | head -5Repository: cortexlinux/cortex
Length of output: 85
🏁 Script executed:
# Read the cortex/pin_manager.py file around lines 476-478 for full context
sed -n '460,500p' ./cortex/pin_manager.pyRepository: cortexlinux/cortex
Length of output: 1415
Fix the ~= compatible release operator to handle all version component lengths.
The current implementation hardcodes v[:2] == c[:2], which only works correctly for 3-or-more-component version specifiers (e.g., ~=1.4.2). Per PEP 440, the compatible release operator should compare all components except the last one: the prefix must match while allowing variation only in the final component.
For example:
~=1.4.2requires>=1.4.2, ==1.4.*(prefix is1.4) — works by accident with current code~=1.4requires>=1.4, ==1.*(prefix is1) — fails because code checksv[:2] == c[:2]
Change the comparison to v[:len(c)-1] == c[:len(c)-1] to correctly handle all cases.
🤖 Prompt for AI Agents
In `@cortex/pin_manager.py` around lines 476 - 478, The compatible-release branch
handling operator "~=" currently fixes the prefix length to two components
(v[:2] == c[:2]) which fails for specifiers with different component counts;
update the check in the "~=" branch to compare all components except the last
one by replacing the hardcoded slice with a dynamic slice using len(c) (i.e.,
ensure v[:len(c)-1] == c[:len(c)-1]) so that for any constraint components in
variable c the prefix match uses all but the final component while keeping the
existing >= comparison (v >= c).
| def _apt_mark_hold(self, package: str) -> bool: | ||
| """Run apt-mark hold on a package""" | ||
| try: | ||
| result = subprocess.run( | ||
| ["sudo", "apt-mark", "hold", package], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=30, | ||
| ) | ||
| return result.returncode == 0 | ||
| except (subprocess.TimeoutExpired, FileNotFoundError) as e: | ||
| logger.error(f"apt-mark hold failed for {package}: {e}") | ||
| return False | ||
|
|
||
| def _apt_mark_unhold(self, package: str) -> bool: | ||
| """Run apt-mark unhold on a package""" | ||
| try: | ||
| result = subprocess.run( | ||
| ["sudo", "apt-mark", "unhold", package], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=30, | ||
| ) | ||
| return result.returncode == 0 | ||
| except (subprocess.TimeoutExpired, FileNotFoundError) as e: | ||
| logger.error(f"apt-mark unhold failed for {package}: {e}") | ||
| return False |
There was a problem hiding this comment.
Silent sudo execution violates coding guidelines.
The _apt_mark_hold and _apt_mark_unhold methods execute sudo apt-mark commands without explicit user confirmation. Per the coding guidelines: "Do not use silent sudo - require explicit user confirmation for privilege escalation."
These methods should either:
- Prompt for user confirmation before executing sudo commands
- Accept a
confirmedparameter that the CLI layer sets after prompting the user - Return the command to be executed and let the caller handle confirmation
🔒 Suggested approach
- def _apt_mark_hold(self, package: str) -> bool:
+ def _apt_mark_hold(self, package: str, confirmed: bool = False) -> bool:
"""Run apt-mark hold on a package"""
+ if not confirmed:
+ logger.warning(f"apt-mark hold for {package} requires confirmation")
+ return False
try:
result = subprocess.run(
["sudo", "apt-mark", "hold", package],The CLI layer should prompt for confirmation before calling these methods with confirmed=True.
🤖 Prompt for AI Agents
In `@cortex/pin_manager.py` around lines 657 - 683, Both _apt_mark_hold and
_apt_mark_unhold currently run "sudo" silently; change them to require explicit
confirmation by adding a boolean parameter (e.g., confirmed: bool = False) and
skip execution (return False and log a warning) unless confirmed is True, so the
CLI layer can prompt the user and call _apt_mark_hold(package, confirmed=True)
or _apt_mark_unhold(package, confirmed=True); update both function signatures
and their callers accordingly (do not perform sudo inside these functions when
confirmed is False).
| def test_match_range_gte(self, pin_manager): | ||
| """Test range matching with >= constraint.""" | ||
| pin = PinConfiguration( | ||
| package="python3", | ||
| version=">=3.10", | ||
| pin_type=PinType.RANGE, | ||
| ) | ||
|
|
||
| assert pin_manager.version_matches_pin(pin, "3.10.0") is True | ||
| assert pin_manager.version_matches_pin(pin, "3.11.0") is True | ||
| assert pin_manager.version_matches_pin(pin, "3.9.0") is False | ||
|
|
||
| def test_match_range_lt(self, pin_manager): | ||
| """Test range matching with < constraint.""" | ||
| pin = PinConfiguration( | ||
| package="python3", | ||
| version="<3.12", | ||
| pin_type=PinType.RANGE, | ||
| ) | ||
|
|
||
| assert pin_manager.version_matches_pin(pin, "3.11.5") is True | ||
| assert pin_manager.version_matches_pin(pin, "3.12.0") is False | ||
| assert pin_manager.version_matches_pin(pin, "3.12.1") is False | ||
|
|
||
| def test_match_range_combined(self, pin_manager): | ||
| """Test range matching with multiple constraints.""" | ||
| pin = PinConfiguration( | ||
| package="python3", | ||
| version=">=3.11,<3.12", | ||
| pin_type=PinType.RANGE, | ||
| ) | ||
|
|
||
| assert pin_manager.version_matches_pin(pin, "3.11.0") is True | ||
| assert pin_manager.version_matches_pin(pin, "3.11.5") is True | ||
| assert pin_manager.version_matches_pin(pin, "3.10.0") is False | ||
| assert pin_manager.version_matches_pin(pin, "3.12.0") is False |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if ~= operator is tested anywhere
rg -n "~=" tests/Repository: cortexlinux/cortex
Length of output: 111
🏁 Script executed:
# Get the actual test file to confirm the snippet
wc -l tests/unit/test_pin_manager.pyRepository: cortexlinux/cortex
Length of output: 96
🏁 Script executed:
# Check the broader context around those lines and look for other test methods
sed -n '360,420p' tests/unit/test_pin_manager.pyRepository: cortexlinux/cortex
Length of output: 2416
🏁 Script executed:
# Check the PinConfiguration and PinType implementation to understand supported operators
fd -t f "pin.*config\|pin.*type" --include="*.py" | grep -v test | head -20Repository: cortexlinux/cortex
Length of output: 282
🏁 Script executed:
# Find PinConfiguration and related implementation files
rg -l "class PinConfiguration\|class PinType" --type py | grep -v testRepository: cortexlinux/cortex
Length of output: 44
🏁 Script executed:
# Search for ~= operator support in the codebase
rg -n "compatible_release\|~=" --type py | grep -v testRepository: cortexlinux/cortex
Length of output: 44
🏁 Script executed:
# Check the version_matches_pin implementation
rg -A 30 "def version_matches_pin" --type py | grep -v test | head -100Repository: cortexlinux/cortex
Length of output: 1899
🏁 Script executed:
# Find PinType definition
rg -A 20 "class PinType\|^class PinType" --type py cortex/Repository: cortexlinux/cortex
Length of output: 44
🏁 Script executed:
# Check the _match_range implementation
rg -A 30 "def _match_range" --type py cortex/Repository: cortexlinux/cortex
Length of output: 1784
🏁 Script executed:
# Search more broadly for any ~= handling
rg "~" --type py cortex/ | grep -v ".pyc" | head -30Repository: cortexlinux/cortex
Length of output: 2715
Add test coverage for the ~= (compatible release) operator.
The tests cover >=, <, and combined range constraints, but the implementation supports the ~= compatible release operator (with semantics like ~=1.4.2 meaning >=1.4.2,<1.5.0). Add a test case to verify this operator functions correctly.
🤖 Prompt for AI Agents
In `@tests/unit/test_pin_manager.py` around lines 374 - 409, Add a new unit test
in tests/unit/test_pin_manager.py that exercises the compatible-release operator
"~=" using the same style as existing range tests: create a PinConfiguration
with pin_type=PinType.RANGE and version="~=X.Y.Z" (e.g., "~=1.4.2") and call
pin_manager.version_matches_pin to assert that versions >=1.4.2 and <1.5.0
return True while versions below 1.4.2 or at/above 1.5.0 return False; reference
the PinConfiguration class, PinType.RANGE, and the version_matches_pin method to
locate where to add the test.
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@DipeshDalmia Kindly address coderabbitai comments and add a demonstrating video in the PR description, thanks.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_cli_extended.py (2)
300-306: Inconsistent assertion: missingforceparameter.This test does not include the
force=Falseparameter in the assertion, while the subsequent tests on lines 314 and 322 do include it. For consistency and to match the updatedinstallAPI, addforce=Falsehere as well.Proposed fix
- mock_install.assert_called_once_with("docker", execute=False, dry_run=False, parallel=False) + mock_install.assert_called_once_with("docker", execute=False, dry_run=False, parallel=False, force=False)
308-314: Test assertion does not match CLI input.The test patches
sys.argvwith--executeflag, but the assertion expectsexecute=False. When--executeis passed,args.executeshould beTrue.Proposed fix
- mock_install.assert_called_once_with("docker", execute=False, dry_run=False, parallel=False, force=False) + mock_install.assert_called_once_with("docker", execute=True, dry_run=False, parallel=False, force=False)
🤖 Fix all issues with AI agents
In `@tests/test_cli_extended.py`:
- Around line 316-322: Fix the indentation and expected dry-run behavior in
test_main_install_with_dry_run: correct the indentation of the final assertion
line so it aligns with the other test body lines, and update the mocked call
assertion on mock_install.assert_called_once_with in
test_main_install_with_dry_run to expect dry_run=True (since sys.argv includes
"--dry-run"); ensure the assertion still checks the same other kwargs
(execute=False, parallel=False, force=False) and that the test calls main() and
patches CortexCLI.install as before.
In `@tests/test_cli.py`:
- Around line 217-223: The test patches sys.argv to include "--execute" but
asserts execute=False; update the expectation in test_main_install_with_execute
so mock_install.assert_called_once_with("docker", execute=True, dry_run=False,
parallel=False, force=False) to match the parsed args from main() (function
names: test_main_install_with_execute, mock_install, main).
- Around line 225-231: The test test_main_install_with_dry_run sets sys.argv to
include "--dry-run" but asserts mock_install was called with dry_run=False;
update the assertion in test_main_install_with_dry_run to expect dry_run=True
when calling CortexCLI.install (i.e., change the assert_called_once_with on
mock_install for the dry_run parameter) so the assertion matches the parsed args
from main().
| @patch("sys.argv", ["cortex", "install", "docker", "--dry-run"]) | ||
| @patch("cortex.cli.CortexCLI.install") | ||
| def test_main_install_with_dry_run(self, mock_install) -> None: | ||
| mock_install.return_value = 0 | ||
| result = main() | ||
| self.assertEqual(result, 0) | ||
| mock_install.assert_called_once_with("docker", execute=False, dry_run=True, parallel=False) | ||
| mock_install.assert_called_once_with("docker", execute=False, dry_run=False, parallel=False, force=False) |
There was a problem hiding this comment.
Indentation error and incorrect assertion.
Line 322 has an indentation error (7 spaces instead of 8) causing pipeline failures. Additionally, the test patches sys.argv with --dry-run flag, but the assertion expects dry_run=False—it should be True.
Proposed fix
`@patch`("sys.argv", ["cortex", "install", "docker", "--dry-run"])
`@patch`("cortex.cli.CortexCLI.install")
def test_main_install_with_dry_run(self, mock_install) -> None:
mock_install.return_value = 0
result = main()
self.assertEqual(result, 0)
- mock_install.assert_called_once_with("docker", execute=False, dry_run=False, parallel=False, force=False)
+ mock_install.assert_called_once_with("docker", execute=False, dry_run=True, parallel=False, force=False)📝 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.
| @patch("sys.argv", ["cortex", "install", "docker", "--dry-run"]) | |
| @patch("cortex.cli.CortexCLI.install") | |
| def test_main_install_with_dry_run(self, mock_install) -> None: | |
| mock_install.return_value = 0 | |
| result = main() | |
| self.assertEqual(result, 0) | |
| mock_install.assert_called_once_with("docker", execute=False, dry_run=True, parallel=False) | |
| mock_install.assert_called_once_with("docker", execute=False, dry_run=False, parallel=False, force=False) | |
| `@patch`("sys.argv", ["cortex", "install", "docker", "--dry-run"]) | |
| `@patch`("cortex.cli.CortexCLI.install") | |
| def test_main_install_with_dry_run(self, mock_install) -> None: | |
| mock_install.return_value = 0 | |
| result = main() | |
| self.assertEqual(result, 0) | |
| mock_install.assert_called_once_with("docker", execute=False, dry_run=True, parallel=False, force=False) |
🧰 Tools
🪛 GitHub Actions: autofix.ci
[error] 321-321: Black formatting failed to parse for target Python 3.12: DedentDoesNotMatchAnyOuterIndent. File could not be reformatted.
🪛 GitHub Actions: CI
[error] 322-322: ruff check failed: invalid-syntax: unindent does not match any outer indentation level (tests/test_cli_extended.py:322).
🪛 GitHub Actions: Cortex Automation
[error] 322-322: IndentationError: unindent does not match any outer indentation level.
🪛 GitHub Check: lint
[failure] 322-322: Ruff (invalid-syntax)
tests/test_cli_extended.py:322:1: invalid-syntax: unindent does not match any outer indentation level
🪛 GitHub Check: Lint
[failure] 322-322: Ruff (invalid-syntax)
tests/test_cli_extended.py:322:1: invalid-syntax: unindent does not match any outer indentation level
🤖 Prompt for AI Agents
In `@tests/test_cli_extended.py` around lines 316 - 322, Fix the indentation and
expected dry-run behavior in test_main_install_with_dry_run: correct the
indentation of the final assertion line so it aligns with the other test body
lines, and update the mocked call assertion on
mock_install.assert_called_once_with in test_main_install_with_dry_run to expect
dry_run=True (since sys.argv includes "--dry-run"); ensure the assertion still
checks the same other kwargs (execute=False, parallel=False, force=False) and
that the test calls main() and patches CortexCLI.install as before.
| @patch("sys.argv", ["cortex", "install", "docker", "--execute"]) | ||
| @patch("cortex.cli.CortexCLI.install") | ||
| def test_main_install_with_execute(self, mock_install): | ||
| mock_install.return_value = 0 | ||
| result = main() | ||
| self.assertEqual(result, 0) | ||
| mock_install.assert_called_once_with("docker", execute=True, dry_run=False, parallel=False) | ||
| mock_install.assert_called_once_with("docker", execute=False, dry_run=False, parallel=False, force=False) |
There was a problem hiding this comment.
Test assertion does not match CLI input.
The test patches sys.argv with --execute flag, but the assertion expects execute=False. When --execute is passed, args.execute should be True.
Proposed fix
- mock_install.assert_called_once_with("docker", execute=False, dry_run=False, parallel=False, force=False)
+ mock_install.assert_called_once_with("docker", execute=True, dry_run=False, parallel=False, force=False)🤖 Prompt for AI Agents
In `@tests/test_cli.py` around lines 217 - 223, The test patches sys.argv to
include "--execute" but asserts execute=False; update the expectation in
test_main_install_with_execute so mock_install.assert_called_once_with("docker",
execute=True, dry_run=False, parallel=False, force=False) to match the parsed
args from main() (function names: test_main_install_with_execute, mock_install,
main).
| @patch("sys.argv", ["cortex", "install", "docker", "--dry-run"]) | ||
| @patch("cortex.cli.CortexCLI.install") | ||
| def test_main_install_with_dry_run(self, mock_install): | ||
| mock_install.return_value = 0 | ||
| result = main() | ||
| self.assertEqual(result, 0) | ||
| mock_install.assert_called_once_with("docker", execute=False, dry_run=True, parallel=False) | ||
| mock_install.assert_called_once_with("docker", execute=False, dry_run=False, parallel=False, force=False) |
There was a problem hiding this comment.
Test assertion does not match CLI input.
The test patches sys.argv with --dry-run flag, but the assertion expects dry_run=False. When --dry-run is passed, args.dry_run should be True.
Proposed fix
- mock_install.assert_called_once_with("docker", execute=False, dry_run=False, parallel=False, force=False)
+ mock_install.assert_called_once_with("docker", execute=False, dry_run=True, parallel=False, force=False)📝 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.
| @patch("sys.argv", ["cortex", "install", "docker", "--dry-run"]) | |
| @patch("cortex.cli.CortexCLI.install") | |
| def test_main_install_with_dry_run(self, mock_install): | |
| mock_install.return_value = 0 | |
| result = main() | |
| self.assertEqual(result, 0) | |
| mock_install.assert_called_once_with("docker", execute=False, dry_run=True, parallel=False) | |
| mock_install.assert_called_once_with("docker", execute=False, dry_run=False, parallel=False, force=False) | |
| `@patch`("sys.argv", ["cortex", "install", "docker", "--dry-run"]) | |
| `@patch`("cortex.cli.CortexCLI.install") | |
| def test_main_install_with_dry_run(self, mock_install): | |
| mock_install.return_value = 0 | |
| result = main() | |
| self.assertEqual(result, 0) | |
| mock_install.assert_called_once_with("docker", execute=False, dry_run=True, parallel=False, force=False) |
🤖 Prompt for AI Agents
In `@tests/test_cli.py` around lines 225 - 231, The test
test_main_install_with_dry_run sets sys.argv to include "--dry-run" but asserts
mock_install was called with dry_run=False; update the assertion in
test_main_install_with_dry_run to expect dry_run=True when calling
CortexCLI.install (i.e., change the assert_called_once_with on mock_install for
the dry_run parameter) so the assertion matches the parsed args from main().
|
@DipeshDalmia Thanks for the contribution. The issue has been migrated to Please open a new PR against |
2 participants
Related Issue
Closes cortexlinux/cortex-distro#39
Summary
Implements a comprehensive package version pinning system for Cortex that allows users to pin specific package versions to prevent unwanted updates. The implementation includes CLI commands for managing pins, integration with the install flow to prevent accidental updates, and support for export/import of pin configurations.
AI Disclosure
Used Cursor AI agent for code generation and implementation assistance. The agent helped with:
Type of Change
Checklist
type(scope): descriptionor[scope] descriptionpytest tests/unit/test_pin_manager.py)Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.