Self update version mgmt #199
Conversation
|
Hi @dhvll! This PR has merge conflicts with the latest main branch. Could you please rebase your branch to resolve them? git fetch origin
git rebase origin/main
# resolve conflicts
git push --force-with-leaseOnce rebased, we'll merge this right away - the implementation looks great! |
|
Thanks for this work! Please rebase onto main to resolve conflicts: |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR implements a comprehensive self-update and version management system for Cortex, including dynamic version resolution, update manifest parsing with compatibility rules, update service orchestration with dry-run and rollback support, and version comparison utilities. It also adds native llama.cpp integration for local model inference with GPU backend support, model management, and a preload daemon, along with supporting scripts and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UpdateService
participant UpdateManifest
participant SystemInfo
participant RemoteManifest as Remote<br/>Manifest API
Client->>UpdateService: check_for_updates(force=false)
activate UpdateService
UpdateService->>UpdateService: load_channel_from_state()
UpdateService->>RemoteManifest: fetch_manifest(url)
RemoteManifest-->>UpdateService: manifest_json
UpdateService->>UpdateManifest: parse from_dict(manifest_json)
activate UpdateManifest
UpdateManifest->>SystemInfo: detect current environment
UpdateManifest->>UpdateManifest: iter_releases(channel, system)
UpdateManifest->>UpdateManifest: compatibility check per release
UpdateManifest-->>UpdateService: filtered ReleaseEntry
deactivate UpdateManifest
UpdateService->>UpdateService: find_latest(current_version)
UpdateService->>UpdateService: cache result if available
UpdateService-->>Client: UpdateCheckResult(available, release)
deactivate UpdateService
Client->>UpdateService: perform_update(dry_run=false)
activate UpdateService
UpdateService->>UpdateService: download_artifact(release.url)
UpdateService->>UpdateService: verify_checksum(sha256)
alt Dry-run
UpdateService-->>Client: UpdatePerformResult(success, updated=false, message="dry-run")
else Live Update
UpdateService->>UpdateService: install_via_pip(artifact)
UpdateService->>UpdateService: verify_new_version()
UpdateService->>UpdateService: log_upgrade_metadata()
UpdateService-->>Client: UpdatePerformResult(success=true, updated=true)
end
deactivate UpdateService
sequenceDiagram
participant User
participant LlamaCppModel
participant ModelManager
participant LlamaCPP as llama.cpp<br/>Backend
participant FileSystem
User->>ModelManager: load_model(model_name)
activate ModelManager
ModelManager->>FileSystem: check models.json config
ModelManager->>ModelManager: get_optimal_config(model_path)
ModelManager->>LlamaCppModel: initialize with config
activate LlamaCppModel
LlamaCppModel->>LlamaCPP: construct Llama instance
LlamaCPP->>FileSystem: load GGUF model file
LlamaCppModel-->>ModelManager: ready
deactivate LlamaCppModel
ModelManager-->>User: LlamaCppModel instance
deactivate ModelManager
User->>LlamaCppModel: generate(prompt, config)
activate LlamaCppModel
LlamaCppModel->>LlamaCPP: __call__ with parameters
LlamaCPP-->>LlamaCppModel: tokens_output
LlamaCppModel->>LlamaCppModel: compute timing metrics
LlamaCppModel-->>User: InferenceResult(content, tokens, timing)
deactivate LlamaCppModel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
cortex/__init__.py (1)
1-14: Duplicate assignments override dynamic version logic.The file has two
__version__assignments and two__all__assignments:
- Lines 3-6 dynamically resolve version from metadata (good)
- Line 12 overwrites it with static
"0.1.0"(defeats the purpose)- Line 8 sets
__all__ = ["__version__"]- Line 14 overwrites
__all__without including"__version__"This appears to be a merge artifact or incomplete refactoring. The dynamic version logic at lines 3-6 is rendered useless.
from importlib import metadata try: __version__ = metadata.version("cortex-linux") except metadata.PackageNotFoundError: __version__ = "0.1.0" -__all__ = ["__version__"] from .cli import main from .packages import PackageManager, PackageManagerType -__version__ = "0.1.0" - -__all__ = ["main", "PackageManager", "PackageManagerType"] +__all__ = ["__version__", "main", "PackageManager", "PackageManagerType"]
🧹 Nitpick comments (5)
test/test_update_service.py (1)
103-104: Use_for the unusedselfparameter to satisfy linter.The lambda replaces an instance method but doesn't use
self. Convention is to use_for intentionally unused parameters.- monkeypatch.setattr(UpdateService, "_fetch_manifest", lambda self: manifest) + monkeypatch.setattr(UpdateService, "_fetch_manifest", lambda _: manifest)cortex/versioning.py (1)
53-58: Consider narrowing the exception catch.The bare
Exceptioncatch works but is overly broad. Since this fallback handles import/attribute access, narrowing to specific exceptions would be more precise without losing safety.try: from cortex import __version__ as package_version # type: ignore raw_version = package_version - except Exception: + except (ImportError, AttributeError): raw_version = "0.0.0"cortex/cli.py (1)
195-239: Consider extracting release notes display to reduce duplication.The release notes display logic (lines 225-228 and 234-237) is duplicated. This also contributes to the cognitive complexity flagged by SonarCloud. Consider extracting a helper method.
+ def _print_release_notes(self, release): + if release.release_notes: + self._print_status("🆕", "What's new:") + for line in release.release_notes.strip().splitlines(): + print(f" {line}")Then replace both occurrences with
self._print_release_notes(release).cortex/updater.py (1)
270-271: Minor: Use list unpacking for cleaner syntax.- cmd = [sys.executable, "-m", "pip"] + args + cmd = [sys.executable, "-m", "pip", *args]cortex/update_manifest.py (1)
129-143: Consider implementing manifest signature verification.The
signaturefield is parsed but never validated. For a complete security model, the manifest should be cryptographically signed and verified before trusting its contents. The current implementation relies solely on HTTPS transport security.Would you like me to help design a signature verification approach, or should this be tracked as a follow-up issue?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
LLM/requirements.txt(1 hunks)README.md(1 hunks)README_DEPENDENCIES (1).md(0 hunks)cortex/__init__.py(1 hunks)cortex/cli.py(6 hunks)cortex/update_manifest.py(1 hunks)cortex/updater.py(1 hunks)cortex/versioning.py(1 hunks)test/test_update_service.py(1 hunks)
💤 Files with no reviewable changes (1)
- README_DEPENDENCIES (1).md
🧰 Additional context used
🧬 Code graph analysis (4)
cortex/cli.py (3)
cortex/update_manifest.py (2)
UpdateChannel(18-28)from_string(23-28)cortex/updater.py (8)
ChecksumMismatch(59-60)InstallError(63-64)UpdateError(55-56)UpdateService(67-296)get_channel(100-106)perform_update(175-231)check_for_updates(130-172)set_channel(108-111)cortex/versioning.py (1)
from_string(30-35)
test/test_update_service.py (3)
cortex/update_manifest.py (5)
UpdateChannel(18-28)UpdateManifest(130-177)SystemInfo(32-45)current(39-45)find_latest(158-177)cortex/versioning.py (1)
CortexVersion(23-38)cortex/updater.py (3)
UpdateService(67-296)get_channel(100-106)perform_update(175-231)
cortex/versioning.py (1)
cortex/update_manifest.py (2)
from_string(23-28)current(39-45)
cortex/update_manifest.py (1)
cortex/versioning.py (3)
CortexVersion(23-38)is_newer_version(63-66)from_string(30-35)
🪛 GitHub Actions: Cortex Automation
test/test_update_service.py
[error] 1-1: Test collection failed due to import error: missing 'requests' dependency in environment, causing several test modules to fail during collection.
cortex/updater.py
[error] 19-19: ModuleNotFoundError: No module named 'requests'
🪛 GitHub Check: SonarCloud Code Analysis
cortex/cli.py
[failure] 195-195: Refactor this function to reduce its Cognitive Complexity from 18 to the 15 allowed.
🪛 Ruff (0.14.6)
cortex/cli.py
213-213: Do not catch blind exception: Exception
(BLE001)
218-218: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
229-229: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
247-247: Do not catch blind exception: Exception
(BLE001)
260-260: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
271-271: Possible SQL injection vector through string-based query construction
(S608)
test/test_update_service.py
104-104: Unused lambda argument: self
(ARG005)
cortex/versioning.py
34-34: Avoid specifying long messages outside the exception class
(TRY003)
57-57: Do not catch blind exception: Exception
(BLE001)
cortex/updater.py
90-90: Do not catch blind exception: Exception
(BLE001)
257-259: Avoid specifying long messages outside the exception class
(TRY003)
271-271: Consider [sys.executable, "-m", "pip", *args] instead of concatenation
Replace with [sys.executable, "-m", "pip", *args]
(RUF005)
274-274: subprocess call: check for execution of untrusted input
(S603)
283-283: Avoid specifying long messages outside the exception class
(TRY003)
cortex/update_manifest.py
28-28: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Do not catch blind exception: Exception
(BLE001)
72-72: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (22)
test/test_update_service.py (5)
10-30: LGTM!The
make_manifesthelper is well-structured with sensible defaults for testing purposes.
33-39: LGTM!Good practice to use a fixed
SystemInfofor deterministic test behavior.
42-65: LGTM!Good coverage of the version selection logic, ensuring the highest eligible version is selected.
68-87: LGTM!Good coverage of channel persistence, verifying both the API and the underlying file state.
90-111: LGTM!Good test for dry-run behavior — confirms the release is identified but no update is applied.
cortex/versioning.py (2)
22-38: LGTM!Clean dataclass design with immutability (
frozen=True) and a well-structured factory method for validation.
63-66: LGTM!Simple and correct version comparison using the parsed
Versionobjects.LLM/requirements.txt (1)
3-4: Unfortunately, I'm unable to access the repository to verify the review comment. The repository clone operation failed, which prevents me from:
- Confirming whether
cortex/updater.pyandcortex/versioning.pyactually use therequestsandpackagingdependencies- Checking the repository's dependency structure (root
requirements.txt,setup.py,pyproject.toml)- Determining whether
LLM/requirements.txtis separate from or integrated with root dependencies- Verifying the test configuration and why the pipeline might be failing
Without access to the codebase, I cannot definitively verify whether the review comment's claims are accurate or if the dependencies are indeed in the wrong location.
Dependencies may be added to wrong requirements file — needs verification.
The review identifies that
requestsandpackagingdependencies were added toLLM/requirements.txtbut claims they're needed bycortex/updater.pyandcortex/versioning.py. However, without repository access, I cannot confirm:
- Whether these modules actually import these dependencies
- The repository's dependency structure (root vs. subdirectory requirements)
- Whether the pipeline failure is caused by missing dependencies in the root scope
A maintainer should verify if these dependencies need to be in the root
requirements.txtorsetup.pyinstead of (or in addition to)LLM/requirements.txt.cortex/cli.py (3)
241-256: LGTM!The broad exception catch at line 247 is appropriate here since update notifications are non-critical and should never disrupt the primary install workflow. Silently failing is the correct behavior.
258-272: LGTM!Channel management methods are well-structured with proper validation and error handling.
606-616: LGTM!The CLI argument parsing for
updateandchannelcommands is well-structured, with proper choices derived from theUpdateChannelenum.cortex/updater.py (6)
93-97: LGTM: Atomic state file writes.The write-to-temp-then-replace pattern prevents data corruption if the process is interrupted during write.
130-172: LGTM!The update check logic is well-structured with appropriate caching to avoid excessive network calls, and properly handles timezone-aware datetimes.
209-231: LGTM: Robust update workflow with rollback.Good implementation with proper cleanup in
finallyblock and automatic rollback on failure before re-raising the exception.
234-248: LGTM!Streaming download with chunked writes and checksum verification provides both memory efficiency and security.
266-268: Rollback depends on PyPI availability.The rollback reinstalls from PyPI using
{PACKAGE_NAME}=={previous.raw}. If the previous version is yanked or unavailable, the rollback will fail. Consider documenting this limitation or caching the previous wheel before upgrade.
299-319: LGTM!Proper serialization helper for caching release data to state file.
cortex/update_manifest.py (5)
18-28: LGTM!Clean enum with case-insensitive parsing and helpful error message listing valid options.
48-54: LGTM!Lazy import with graceful fallback handles the optional
distropackage correctly.
81-94: LGTM!Compatibility checking logic is correct: empty constraint lists match any system, while non-empty lists require membership.
107-127: LGTM!
ReleaseEntry.from_dictproperly parses nested structures, andis_compatiblecorrectly implements OR semantics across compatibility rules.
158-177: LGTM!
find_latestcorrectly filters for compatible releases newer than the current version and returns the highest version.
dhvll
force-pushed
the
self-update-version-mgmt
branch
from
923ccc0 to
aaca54e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
496-902: Remove massive content duplication.Lines 496-902 duplicate the entire Features, Quick Start, Installation, Usage, Configuration, Architecture, Development, Contributing, Roadmap, FAQ, and Community sections that already appear at lines 73-495. This nearly doubles the README file size unnecessarily.
Remove lines 496-902 entirely, keeping only the content at lines 1-495.
♻️ Duplicate comments (2)
cortex/updater.py (1)
19-19: Verifyrequestslibrary is in dependencies.Past review comments flagged that
requestsis imported but may not be declared in project dependencies, causing pipeline failures. Ensure it's added torequirements.txt,setup.py, orpyproject.toml.#!/bin/bash # Check if requests is declared in dependency files echo "=== Checking for 'requests' in dependencies ===" fd -t f 'requirements.*\.txt$|setup\.py$|pyproject\.toml$' --exec grep -l "requests" {}README.md (1)
423-438: Fix broken HTML structure in FAQ.The
<details>block for "What operating systems are supported?" (line 423) is never closed, and the "Keeping Cortex Up to Date" section (lines 426-435) is incorrectly placed inside this FAQ entry. The FAQ answer appears after the update section at line 437.Based on past review comments, this issue was marked as addressed but is still present. Apply this fix:
<details> <summary><strong>What operating systems are supported?</strong></summary> -## Keeping Cortex Up to Date - -- Cortex automatically checks for new releases (stable by default) when you run `cortex install ...`. Disable with `CORTEX_UPDATE_CHECK=0`. -- See the current channel or switch tracks: - - `cortex channel show` - - `cortex channel set beta` -- Upgrade in-place with release notes, checksum verification, and automatic rollback on failure: - - `cortex update` (use `--dry-run` to preview, `--channel beta` to override per run). -- Update metadata lives in `~/.config/cortex/updater/`, including logs and last upgrade state. - -### Join the Community Currently Ubuntu 24.04 LTS. Other Debian-based distributions coming soon. </details> + +## Keeping Cortex Up to Date + +- Cortex automatically checks for new releases (stable by default) when you run `cortex install ...`. Disable with `CORTEX_UPDATE_CHECK=0`. +- See the current channel or switch tracks: + - `cortex channel show` + - `cortex channel set beta` +- Upgrade in-place with release notes, checksum verification, and automatic rollback on failure: + - `cortex update` (use `--dry-run` to preview, `--channel beta` to override per run). +- Update metadata lives in `~/.config/cortex/updater/`, including logs and last upgrade state.
🧹 Nitpick comments (3)
cortex/__init__.py (1)
11-11: Optional: Sort__all__for consistency.Static analysis suggests sorting
__all__alphabetically for better maintainability.-__all__ = ["__version__", "main", "PackageManager", "PackageManagerType"] +__all__ = ["PackageManager", "PackageManagerType", "__version__", "main"]cortex/cli.py (1)
195-239: Consider refactoring to reduce cognitive complexity.The
updatemethod has a cognitive complexity of 18, slightly above the recommended threshold of 15. While the logic is clear, extracting release display logic into a helper method would improve maintainability.Consider extracting the release notes display logic (lines 225-228, 234-237) into a helper method:
def _display_release_notes(self, release: ReleaseEntry): if release.release_notes: self._print_status("🆕", "What's new:") for line in release.release_notes.strip().splitlines(): print(f" {line}")This would reduce complexity and enable reuse across update flows.
cortex/updater.py (1)
270-271: Optional: Use unpacking for list concatenation.Static analysis suggests using unpacking instead of concatenation for cleaner code.
- cmd = [sys.executable, "-m", "pip"] + args + cmd = [sys.executable, "-m", "pip", *args]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
LLM/requirements.txt(1 hunks)README.md(1 hunks)README_DEPENDENCIES (1).md(0 hunks)cortex/__init__.py(1 hunks)cortex/cli.py(6 hunks)cortex/update_manifest.py(1 hunks)cortex/updater.py(1 hunks)cortex/versioning.py(1 hunks)test/test_update_service.py(1 hunks)
💤 Files with no reviewable changes (1)
- README_DEPENDENCIES (1).md
🚧 Files skipped from review as they are similar to previous changes (1)
- LLM/requirements.txt
🧰 Additional context used
🧬 Code graph analysis (5)
cortex/updater.py (2)
cortex/update_manifest.py (10)
ReleaseEntry(98-126)SystemInfo(32-45)UpdateChannel(18-28)UpdateManifest(130-177)current(39-45)from_string(23-28)from_dict(65-79)from_dict(108-120)from_dict(136-143)find_latest(158-177)cortex/versioning.py (3)
CortexVersion(23-38)get_installed_version(41-60)from_string(30-35)
cortex/cli.py (3)
cortex/update_manifest.py (2)
UpdateChannel(18-28)from_string(23-28)cortex/updater.py (8)
ChecksumMismatch(59-60)InstallError(63-64)UpdateError(55-56)UpdateService(67-296)get_channel(100-106)perform_update(175-231)check_for_updates(130-172)set_channel(108-111)cortex/versioning.py (1)
from_string(30-35)
cortex/__init__.py (1)
cortex/packages.py (2)
PackageManager(23-452)PackageManagerType(16-20)
cortex/update_manifest.py (1)
cortex/versioning.py (3)
CortexVersion(23-38)is_newer_version(63-66)from_string(30-35)
test/test_update_service.py (2)
cortex/update_manifest.py (5)
UpdateChannel(18-28)UpdateManifest(130-177)SystemInfo(32-45)current(39-45)find_latest(158-177)cortex/updater.py (3)
UpdateService(67-296)get_channel(100-106)perform_update(175-231)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/cli.py
[failure] 195-195: Refactor this function to reduce its Cognitive Complexity from 18 to the 15 allowed.
🪛 LanguageTool
README.md
[style] ~372-~372: Consider using a more formal and expressive alternative to ‘amazing’.
Context: ...g-feature) 3. **Commit** your changes (git commit -m 'Add amazing feature') 4. **Push** to the branch (...
(AWESOME)
🪛 markdownlint-cli2 (0.18.1)
README.md
246-246: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
333-333: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.6)
cortex/updater.py
90-90: Do not catch blind exception: Exception
(BLE001)
257-259: Avoid specifying long messages outside the exception class
(TRY003)
271-271: Consider [sys.executable, "-m", "pip", *args] instead of concatenation
Replace with [sys.executable, "-m", "pip", *args]
(RUF005)
274-274: subprocess call: check for execution of untrusted input
(S603)
283-283: Avoid specifying long messages outside the exception class
(TRY003)
cortex/versioning.py
34-34: Avoid specifying long messages outside the exception class
(TRY003)
57-57: Do not catch blind exception: Exception
(BLE001)
cortex/cli.py
213-213: Do not catch blind exception: Exception
(BLE001)
218-218: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
229-229: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
247-247: Do not catch blind exception: Exception
(BLE001)
260-260: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
271-271: Possible SQL injection vector through string-based query construction
(S608)
cortex/__init__.py
11-11: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
cortex/update_manifest.py
28-28: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Do not catch blind exception: Exception
(BLE001)
72-72: Avoid specifying long messages outside the exception class
(TRY003)
test/test_update_service.py
104-104: Unused lambda argument: self
(ARG005)
🔇 Additional comments (37)
test/test_update_service.py (6)
10-31: LGTM!The
make_manifesthelper provides a clean, reusable way to construct test manifests with sensible defaults.
33-40: LGTM!The
current_systemhelper provides consistent test fixtures for system information.
42-66: LGTM!The test correctly verifies that the manifest selection logic chooses the newest compatible release (0.2.0) over an older one (0.1.5).
68-88: LGTM!The test validates channel persistence across set operations and confirms the state is correctly serialized to JSON.
90-112: Remove unused lambda parameter.Line 104: The lambda function includes an unused
selfparameter. Sinceget_installed_versionis a module-level function (not a method), the monkeypatch doesn't need theselfparameter.- monkeypatch.setattr("cortex.updater.get_installed_version", lambda: CortexVersion.from_string("0.1.0")) + monkeypatch.setattr("cortex.updater.get_installed_version", lambda: CortexVersion.from_string("0.1.0"))Actually, the current code is correct. The static analysis warning is a false positive—the lambda doesn't have a
selfparameter. Ignore the hint.
90-112: LGTM!The test correctly validates dry-run behavior: the release is identified, no update is applied, and the result message indicates dry run mode.
cortex/cli.py (7)
13-14: LGTM!Clean imports of the update subsystem components.
28-32: LGTM!UpdateService initialization is properly placed in the constructor.
67-68: LGTM!Good user experience: checking for updates before install operations.
241-256: LGTM!Silent failure on update check exceptions is appropriate here—update notifications should not disrupt the main install flow. The broad exception catch at line 247 is justified for this use case.
258-272: LGTM!Channel management methods are straightforward and include proper error handling.
607-616: LGTM!CLI argument parser additions for update and channel commands are well-structured with appropriate help text and argument constraints.
654-660: LGTM!Command dispatch logic correctly routes update and channel subcommands to the appropriate handler methods.
cortex/versioning.py (5)
1-19: LGTM!Module structure with clear exports and appropriate imports.
22-38: LGTM!The
CortexVersiondataclass provides a clean wrapper for version handling with proper validation infrom_string.
41-60: LGTM!The fallback chain in
get_installed_versionis well-designed: metadata lookup → package attribute → hardcoded fallback. The broad exception catch at line 57 is appropriate for the final fallback case.
63-66: LGTM!Simple and correct version comparison logic.
11-11: I apologize, but I'm unable to complete the verification due to repository access limitations. The codebase repository could not be cloned in the sandbox environment, and web searches did not return specific information about the cortexlinux/cortex project's dependency files.However, I can provide a rewritten review comment based on the original concern:
Verify
packaginglibrary is declared in project dependencies.The code imports
from packaging.version(line 11), but thepackagingdependency must be explicitly declared inpyproject.toml,setup.py,setup.cfg, orrequirements.txt. Confirm this dependency is included before merging.cortex/updater.py (12)
28-33: LGTM!Configuration constants are well-defined with sensible defaults, including XDG-compliant paths and reasonable cache TTL.
35-53: LGTM!Result dataclasses provide clear contracts for update check and perform operations with all necessary fields.
55-65: LGTM!Exception hierarchy is well-designed with specific exception types for checksum and installation failures.
67-82: LGTM!Constructor properly initializes service with environment variable fallbacks and ensures required directories exist.
84-111: LGTM!State persistence and channel management are well-implemented with atomic writes (temp file + replace) and appropriate error handling. The broad exception catch at line 90 is justified for corrupted state files.
114-172: LGTM!Update checking logic correctly implements caching with TTL, manifest fetching, and release selection. The cache behavior prevents excessive network requests while allowing force refreshes.
175-231: LGTM!The
perform_updatemethod implements a robust update workflow with proper dry-run support, rollback on failure, and cleanup in the finally block. Error handling correctly catchesUpdateErrorsubtypes and ensures cleanup.
234-260: LGTM!Download and checksum verification are correctly implemented with streaming download and chunked hashing. SHA-256 provides strong integrity guarantees.
262-283: LGTM!Installation and rollback logic correctly use
pipviasubprocess, and the rollback mechanism provides a safety net for failed upgrades.
270-283: Subprocess execution is safe; static analysis warning is a false positive.Line 274: The subprocess security warning (S603) is a false positive. The command uses
sys.executable(the current Python interpreter) and user-controlledargsfor pip operations, which is the standard safe pattern for self-updating applications.
285-296: LGTM!Upgrade recording and logging are well-implemented with proper timestamps and file handling.
299-320: LGTM!Release serialization helper correctly converts
ReleaseEntryobjects to dictionary format for JSON persistence.cortex/update_manifest.py (7)
1-16: LGTM!Clean module structure with appropriate imports for manifest parsing and version handling.
18-28: LGTM!
UpdateChannelenum withfrom_stringfactory provides type-safe channel handling with helpful error messages.
31-45: LGTM!
SystemInfodataclass correctly captures system environment details, and thecurrent()factory uses platform detection to populate fields.
48-54: LGTM!The
_detect_distrohelper gracefully handles the optionaldistropackage. The broad exception catch at line 53 is appropriate since distro detection is non-critical.
57-94: LGTM!
CompatibilityRulecorrectly validates Python version specifiers and performs multi-dimensional compatibility checks (Python version, OS, architecture, distro).
97-127: LGTM!
ReleaseEntrydataclass properly encapsulates release metadata with version, channel, download URL, checksum, and compatibility rules. Theis_compatiblecheck correctly treats empty compatibility lists as "compatible with all."
129-177: LGTM!
UpdateManifestprovides excellent manifest processing with:
from_dictfor parsing JSON manifestsiter_releasesfor filtered iterationfind_latestfor selecting the newest compatible releaseThe logic correctly filters by channel and system compatibility, then sorts by version to find the latest.
|
@dhvll This PR has merge conflicts. Could you rebase it against main? Let me know if you need any help. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
README.md (2)
63-70: Remove duplicate Feature table.Lines 63–70 duplicate the Feature table at lines 55–61. Remove the duplicate to eliminate redundant content.
| **Installation History** | Track and rollback any installation | -| Feature | Description | -|---------|-------------| -| **Natural Language Commands** | System understands intent, not syntax | -| **Hardware-Aware Optimization** | Automatically configures for your GPU/CPU | -| **Self-Healing Configuration** | Fixes broken dependencies automatically | -| **Enterprise-Grade Security** | AI actions are sandboxed and validated | -| **Installation History** | Track and rollback any installation | - ---
421-438: Restructure malformed FAQ block.The
<details>block for "What operating systems are supported?" is not properly closed before the update documentation section. The update content and "Join the Community" text should appear outside the FAQ block. The OS support answer (line 437) should immediately follow the question.<details> <summary><strong>What operating systems are supported?</strong></summary> +Currently Ubuntu 24.04 LTS with other Debian-based distributions coming soon. +</details> + ## Keeping Cortex Up to Date - Cortex automatically checks for new releases (stable by default) when you run `cortex install ...`. Disable with `CORTEX_UPDATE_CHECK=0`. - See the current channel or switch tracks: - `cortex channel show` - `cortex channel set beta` - Upgrade in-place with release notes, checksum verification, and automatic rollback on failure: - `cortex update` (use `--dry-run` to preview, `--channel beta` to override per run). - Update metadata lives in `~/.config/cortex/updater/`, including logs and last upgrade state. -### Join the Community -Currently Ubuntu 24.04 LTS. Other Debian-based distributions coming soon. -</details>
🧹 Nitpick comments (1)
README.md (1)
372-372: Consider more formal language in code example.The word "amazing" in the git commit example is colloquial. Consider using a more formal alternative like "important", "excellent", or simply "Add some AmazingFeature" to match the placeholder style used elsewhere.
-2. **Create** a feature branch (`git checkout -b feature/amazing-feature`) -3. **Commit** your changes (`git commit -m 'Add amazing feature'`) +2. **Create** a feature branch (`git checkout -b feature/important-feature`) +3. **Commit** your changes (`git commit -m 'Add important feature'`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~372-~372: Consider using a more formal and expressive alternative to ‘amazing’.
Context: ...g-feature) 3. **Commit** your changes (git commit -m 'Add amazing feature') 4. **Push** to the branch (...
(AWESOME)
🪛 markdownlint-cli2 (0.18.1)
README.md
7-7: No empty links
(MD042, no-empty-links)
246-246: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
333-333: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
498-498: Multiple headings with the same content
(MD024, no-duplicate-heading)
|
@mikejmorgan-ai I have rebase, can we merge now ? |
1 similar comment
|
@mikejmorgan-ai I have rebase, can we merge now ? |
Merge Conflicts - Action RequiredHey @dhvll! Your PR has merge conflicts with the main branch. Please:
Once resolved, we can review for merge! |
dhvll
force-pushed
the
self-update-version-mgmt
branch
from
521ad9a to
8ad7e50
Compare
dhvll
force-pushed
the
self-update-version-mgmt
branch
from
8ad7e50 to
da3e635
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
15-27: Critical: Unresolved merge conflicts break the file.This file contains unresolved merge conflict markers (
<<<<<<< HEAD,=======,>>>>>>>) at multiple locations (lines 15-27, 34-38, 280-281, 580, 593-603, 630-631, 657, 666-706). The file will not parse and the application cannot run.Resolve all merge conflicts before this PR can be merged. The PR comments indicate a rebase was requested—ensure all conflicts are properly resolved.
♻️ Duplicate comments (2)
README.md (2)
548-548: Add language specification to code block.The architecture diagram code block needs a language identifier.
-``` +```text User Input
635-635: Add language specification to code block.The project structure code block needs a language identifier.
-``` +```text cortex/
🧹 Nitpick comments (13)
nginx-demo-output.txt (1)
1-14: Consider excluding this debug output file from version control.This appears to be a captured demo/test run output containing user-specific paths (
/Users/allbots/.cortex/) and API traces. Such ephemeral artifacts typically should not be committed to the repository.If this file is intended as documentation of expected output, consider:
- Moving it to a
docs/examples/directory with sanitized paths- Adding it to
.gitignoreif it's generated during testing- Including it in the PR description or wiki instead
POST_MVP_AUDIT.md (2)
255-255: Add language specifier to fenced code blocks for consistent rendering.Several code blocks (lines 255, 360, 392, 422, 700) lack language specifiers, which affects syntax highlighting and linter compliance.
Example fix:
-``` +```text cortexlinux.com/Apply similar changes to other bare code blocks, using
textor appropriate language identifiers.
5-5: Convert bare URLs to proper Markdown links.Per markdownlint (MD034), bare URLs at lines 5, 763-765 should be wrapped in angle brackets or converted to proper links.
-**Repository:** https://github.com/cortexlinux/cortex +**Repository:** <https://github.com/cortexlinux/cortex>README_backup_20251202_033440.md (2)
238-266: Add language specification to code block.The architecture diagram code block is missing a language identifier. Add
textfor clarity and to satisfy markdown linting.-``` +```text User Input
325-345: Add language specification to code block.The project structure code block is missing a language identifier.
-``` +```text cortex/manage_cortex_prs.sh (1)
363-370: Unused variables in CSV read loop.The
statusandpaymentvariables from the CSV read are unused. Either use them in the output or remove them from the read.- tail -n +2 "$BOUNTY_CSV" | grep "Pending" 2>/dev/null | while IFS=, read -r pr author amount status payment date; do + tail -n +2 "$BOUNTY_CSV" | grep "Pending" 2>/dev/null | while IFS=, read -r pr author amount _status _payment date; docortex/branding.py (4)
37-52: Add return type hint.Per coding guidelines, type hints are required in Python code.
-def show_banner(show_version: bool = True): +def show_banner(show_version: bool = True) -> None:
78-95: Add return type hints to functions.Per coding guidelines, type hints are required.
-def cx_step(step_num: int, total: int, message: str): +def cx_step(step_num: int, total: int, message: str) -> None:-def cx_header(title: str): +def cx_header(title: str) -> None:
97-101: Add return type hint.-def cx_table_header(): +def cx_table_header() -> tuple[str, str, str]:
104-121: Add return type hints.-def show_welcome(): +def show_welcome() -> None:-def show_goodbye(): +def show_goodbye() -> None:test/test_update_service.py (2)
10-30: Add type hints and docstrings to helper functions.Per coding guidelines, type hints are required for Python code, and docstrings are required for public APIs. The helper functions
make_manifestandcurrent_systemare missing return type annotations and docstrings.Apply this diff:
-def make_manifest(version: str = "0.2.0", channel: str = "stable"): +def make_manifest(version: str = "0.2.0", channel: str = "stable") -> UpdateManifest: + """Create a test UpdateManifest with a single release entry.""" return UpdateManifest.from_dict( { "releases": [ { "version": version, "channel": channel, "download_url": "https://example.com/cortex.whl", "sha256": "0" * 64, "release_notes": "Test release", "compatibility": [ { "python": ">=3.8", "os": ["linux"], "arch": ["x86_64"], } ], } ] } ) -def current_system(): +def current_system() -> SystemInfo: + """Return a fixed SystemInfo for deterministic testing.""" return SystemInfo( python_version=Version("3.10.0"), os_name="linux", architecture="x86_64", distro="ubuntu", )
103-104: Consider using_for the unused parameter to clarify intent.The
selfparameter is required for the monkeypatch to work on an instance method but is intentionally unused. Using_makes this explicit and silences the linter.- monkeypatch.setattr(UpdateService, "_fetch_manifest", lambda self: manifest) + monkeypatch.setattr(UpdateService, "_fetch_manifest", lambda _: manifest)cortex/updater.py (1)
270-271: Consider using list unpacking for clarity.This is a minor stylistic improvement suggested by the linter.
- cmd = [sys.executable, "-m", "pip"] + args + cmd = [sys.executable, "-m", "pip", *args]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
bounties_paid.csvis excluded by!**/*.csvimages/logo.pngis excluded by!**/*.pngimages/screenshot.pngis excluded by!**/*.png
📒 Files selected for processing (17)
.github/workflows/automation.yml(3 hunks).gitignore(1 hunks)Home.md(2 hunks)LLM/requirements.txt(1 hunks)POST_MVP_AUDIT.md(1 hunks)README.md(2 hunks)README_DEPENDENCIES (1).md(0 hunks)README_backup_20251202_033440.md(1 hunks)cortex/__init__.py(1 hunks)cortex/branding.py(1 hunks)cortex/cli.py(11 hunks)cortex/update_manifest.py(1 hunks)cortex/updater.py(1 hunks)cortex/versioning.py(1 hunks)manage_cortex_prs.sh(1 hunks)nginx-demo-output.txt(1 hunks)test/test_update_service.py(1 hunks)
💤 Files with no reviewable changes (1)
- README_DEPENDENCIES (1).md
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 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/__init__.pycortex/branding.pycortex/cli.pycortex/updater.pycortex/versioning.pycortex/update_manifest.pytest/test_update_service.py
{setup.py,setup.cfg,pyproject.toml,**/__init__.py}
📄 CodeRabbit inference engine (AGENTS.md)
Use Python 3.10 or higher as the minimum supported version
Files:
cortex/__init__.py
🧠 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 **/*install*.py : Dry-run by default for all installations in command execution
Applied to files:
cortex/cli.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 : Implement audit logging to ~/.cortex/history.db for all package operations
Applied to files:
cortex/cli.py
🧬 Code graph analysis (2)
cortex/cli.py (3)
cortex/update_manifest.py (2)
UpdateChannel(18-28)from_string(23-28)cortex/updater.py (8)
ChecksumMismatch(59-60)InstallError(63-64)UpdateError(55-56)UpdateService(67-296)get_channel(100-106)perform_update(175-231)check_for_updates(130-172)set_channel(108-111)cortex/versioning.py (1)
from_string(30-35)
test/test_update_service.py (3)
cortex/update_manifest.py (5)
UpdateChannel(18-28)UpdateManifest(130-177)SystemInfo(32-45)current(39-45)find_latest(158-177)cortex/versioning.py (1)
CortexVersion(23-38)cortex/updater.py (4)
UpdateService(67-296)set_channel(108-111)get_channel(100-106)perform_update(175-231)
🪛 LanguageTool
README.md
[uncategorized] ~364-~364: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ps team needs this Business Model: Open source community edition + Enterprise subscrip...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~672-~672: Consider using a more formal and expressive alternative to ‘amazing’.
Context: ...g-feature) 3. **Commit** your changes (git commit -m 'Add amazing feature') 4. **Push** to the branch (...
(AWESOME)
[grammar] ~757-~757: Use a hyphen to join words.
Context: ... --- ======= Contributions make the open source community amazing. Any contributi...
(QB_NEW_EN_HYPHEN)
README_backup_20251202_033440.md
[style] ~357-~357: Consider using a more formal and expressive alternative to ‘amazing’.
Context: ...g-feature) 3. **Commit** your changes (git commit -m 'Add amazing feature') 4. **Push** to the branch (...
(AWESOME)
POST_MVP_AUDIT.md
[style] ~460-~460: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... Already approved but never merged -
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
README.md
3-3: Heading style
Expected: atx; Actual: setext
(MD003, heading-style)
99-99: No empty links
(MD042, no-empty-links)
116-116: Link fragments should be valid
(MD051, link-fragments)
127-127: Link fragments should be valid
(MD051, link-fragments)
353-353: Bare URL used
(MD034, no-bare-urls)
354-354: Bare URL used
(MD034, no-bare-urls)
468-468: Multiple headings with the same content
(MD024, no-duplicate-heading)
548-548: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
635-635: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
692-692: Multiple headings with the same content
(MD024, no-duplicate-heading)
README_backup_20251202_033440.md
7-7: No empty links
(MD042, no-empty-links)
238-238: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
325-325: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
POST_MVP_AUDIT.md
5-5: Bare URL used
(MD034, no-bare-urls)
255-255: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
360-360: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
392-392: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
422-422: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
700-700: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
763-763: Bare URL used
(MD034, no-bare-urls)
764-764: Bare URL used
(MD034, no-bare-urls)
765-765: Bare URL used
(MD034, no-bare-urls)
769-769: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Ruff (0.14.8)
cortex/cli.py
15-15: Expected a statement
(invalid-syntax)
15-15: Expected a statement
(invalid-syntax)
15-15: Expected a statement
(invalid-syntax)
15-15: Expected a statement
(invalid-syntax)
16-16: Expected a statement
(invalid-syntax)
16-16: Expected a statement
(invalid-syntax)
16-16: Expected a statement
(invalid-syntax)
16-16: Expected a statement
(invalid-syntax)
27-27: Expected a statement
(invalid-syntax)
27-27: Expected a statement
(invalid-syntax)
27-27: Expected a statement
(invalid-syntax)
27-27: Expected a statement
(invalid-syntax)
27-27: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
27-27: Expected ,, found name
(invalid-syntax)
27-27: Expected ,, found name
(invalid-syntax)
27-28: Expected ), found NonLogicalNewline
(invalid-syntax)
34-34: Expected a statement
(invalid-syntax)
34-34: Expected a statement
(invalid-syntax)
34-34: Expected a statement
(invalid-syntax)
34-34: Expected a statement
(invalid-syntax)
35-35: Expected a statement
(invalid-syntax)
35-35: Expected a statement
(invalid-syntax)
35-35: Expected a statement
(invalid-syntax)
35-35: Expected a statement
(invalid-syntax)
35-36: Expected a statement
(invalid-syntax)
36-36: Unexpected indentation
(invalid-syntax)
38-38: Expected a statement
(invalid-syntax)
38-38: Expected a statement
(invalid-syntax)
38-38: Expected a statement
(invalid-syntax)
38-38: Expected a statement
(invalid-syntax)
38-38: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
38-38: Expected ,, found name
(invalid-syntax)
38-38: Expected ,, found name
(invalid-syntax)
280-280: Expected a statement
(invalid-syntax)
280-280: Expected a statement
(invalid-syntax)
280-280: Expected a statement
(invalid-syntax)
280-280: Expected a statement
(invalid-syntax)
281-281: Expected a statement
(invalid-syntax)
281-281: Expected a statement
(invalid-syntax)
281-281: Expected a statement
(invalid-syntax)
281-281: Expected a statement
(invalid-syntax)
580-580: Expected a statement
(invalid-syntax)
580-580: Expected a statement
(invalid-syntax)
580-580: Expected a statement
(invalid-syntax)
580-580: Expected a statement
(invalid-syntax)
580-580: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
580-580: Expected ,, found name
(invalid-syntax)
580-580: Expected ,, found name
(invalid-syntax)
580-581: Expected ), found NonLogicalNewline
(invalid-syntax)
630-630: Expected a statement
(invalid-syntax)
630-630: Expected a statement
(invalid-syntax)
630-630: Expected a statement
(invalid-syntax)
630-630: Expected a statement
(invalid-syntax)
631-631: Expected a statement
(invalid-syntax)
631-631: Expected a statement
(invalid-syntax)
631-631: Expected a statement
(invalid-syntax)
631-631: Expected a statement
(invalid-syntax)
657-657: Expected a statement
(invalid-syntax)
657-657: Expected a statement
(invalid-syntax)
657-657: Expected a statement
(invalid-syntax)
657-657: Expected a statement
(invalid-syntax)
657-657: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
657-657: Expected ,, found name
(invalid-syntax)
657-657: Expected ,, found name
(invalid-syntax)
666-666: Expected a statement
(invalid-syntax)
666-666: Expected a statement
(invalid-syntax)
666-666: Expected a statement
(invalid-syntax)
666-666: Expected a statement
(invalid-syntax)
667-667: Unexpected indentation
(invalid-syntax)
678-678: Expected a statement
(invalid-syntax)
678-678: Expected a statement
(invalid-syntax)
678-678: Expected a statement
(invalid-syntax)
678-678: Expected a statement
(invalid-syntax)
678-679: Expected a statement
(invalid-syntax)
679-679: Unexpected indentation
(invalid-syntax)
706-706: Expected a statement
(invalid-syntax)
706-706: Expected a statement
(invalid-syntax)
706-706: Expected a statement
(invalid-syntax)
706-706: Expected a statement
(invalid-syntax)
706-706: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
706-706: Expected ,, found name
(invalid-syntax)
706-706: Expected ,, found name
(invalid-syntax)
706-707: Expected ), found NonLogicalNewline
(invalid-syntax)
cortex/updater.py
90-90: Do not catch blind exception: Exception
(BLE001)
257-259: Avoid specifying long messages outside the exception class
(TRY003)
271-271: Consider [sys.executable, "-m", "pip", *args] instead of concatenation
Replace with [sys.executable, "-m", "pip", *args]
(RUF005)
274-274: subprocess call: check for execution of untrusted input
(S603)
283-283: Avoid specifying long messages outside the exception class
(TRY003)
cortex/versioning.py
34-34: Avoid specifying long messages outside the exception class
(TRY003)
57-57: Do not catch blind exception: Exception
(BLE001)
cortex/update_manifest.py
28-28: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Do not catch blind exception: Exception
(BLE001)
72-72: Avoid specifying long messages outside the exception class
(TRY003)
test/test_update_service.py
104-104: Unused lambda argument: self
(ARG005)
🪛 Shellcheck (0.11.0)
manage_cortex_prs.sh
[warning] 15-15: GITHUB_TOKEN appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 74-74: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 80-80: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 83-83: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 86-86: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 92-92: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[error] 273-273: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 273-273: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[error] 273-273: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 273-273: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[warning] 365-365: status appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 365-365: payment appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (21)
cortex/branding.py (1)
55-76: LGTM!The
cx_printfunction is well-documented with a clear docstring explaining parameters and status options. The status icon mapping is clean and extensible.cortex/__init__.py (1)
1-8: LGTM!Clean implementation of dynamic version retrieval using
importlib.metadata. The fallback to"0.1.0"onPackageNotFoundErrorhandles development installs correctly. The previous dead code issue (duplicate__version__and__all__) has been properly addressed.README_backup_20251202_033440.md (1)
1-8: The fileREADME_backup_20251202_033440.mddoes not exist in the repository. The review comment references specific lines and issues (empty link at line 7, missing language specs) in a file that is not present in version control. If this file was intended to be reviewed, it may have already been removed or was never committed.Likely an incorrect or invalid review comment.
test/test_update_service.py (2)
42-65: LGTM!The test correctly verifies that
find_latestselects the newest compatible release when multiple versions are available.
68-87: LGTM!Good use of
tmp_pathfixture for test isolation and direct verification of persisted state.cortex/versioning.py (3)
22-38: LGTM!
CortexVersiondataclass is well-designed with proper error handling and a clean API. Thefrozen=Truemakes instances immutable, which is appropriate for version objects.
41-60: LGTM!The fallback chain (metadata →
__version__→"0.0.0") is a reasonable defensive pattern for version detection across different installation scenarios (pip install, editable install, development).
63-66: LGTM!Simple and correct version comparison leveraging
packaging.version.Versionsemantics.cortex/updater.py (5)
67-81: LGTM!Constructor properly initializes paths and creates necessary directories. Good use of environment variable override for manifest URL and sensible defaults.
93-97: LGTM!Good use of atomic write pattern (write to
.tmp, thenreplace) to prevent state file corruption on crashes.
129-172: LGTM!Well-structured update check with caching to reduce network overhead. The cache validation in
_should_use_cacheensures the date parsing at line 144 is safe.
234-260: LGTM!Artifact download with streaming and SHA256 verification is correctly implemented. The fallback for artifact name extraction handles edge cases like trailing slashes.
266-283: File does not exist in repositoryThe file
cortex/updater.pyreferenced in this review comment does not exist in the codebase. The cortex directory contains other modules (cli.py, coordinator.py, packages.py, user_preferences.py, and kernel_features subdirectory) but no updater.py file.Likely an incorrect or invalid review comment.
cortex/cli.py (3)
201-245: Well-structured update command with comprehensive error handling.The
updatemethod properly handles the exception hierarchy (ChecksumMismatch,InstallError,UpdateError) and provides user-friendly messages. Release notes display is a good UX touch.However, based on learnings, installations should be "dry-run by default." Consider whether the
updatecommand should also default to--dry-runfor consistency, requiring--executeor similar to actually perform the update.
247-262: LGTM!Silent exception handling is appropriate here—update notifications should never block or crash the main command. The
CORTEX_UPDATE_CHECKenvironment variable provides a clean opt-out for CI environments.
34-38: VerifyUpdateServiceinitialization is resolved after conflict resolution.Within the merge conflict block,
self.update_service = UpdateService()should be retained. The eager initialization is acceptable sinceUpdateService.__init__only creates directories without network calls.cortex/update_manifest.py (5)
18-28: LGTM!
UpdateChannelenum withfrom_stringfactory provides clean channel validation with helpful error messages listing valid options.
31-54: LGTM!
SystemInfowith optional distro detection is well-designed. The broad exception catch in_detect_distrois appropriate since thedistropackage is optional and detection should fail silently.
81-94: LGTM!The
is_compatiblemethod correctly implements inclusive matching: empty constraint lists allow any value, while non-empty lists require the system value to be present.
97-126: LGTM!
ReleaseEntrywith proper parsing and OR-based compatibility matching (any rule passes → compatible) is correctly implemented.
158-177: LGTM!
find_latestcorrectly filters releases by channel, system compatibility, and version comparison, then returns the highest eligible version.
|
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
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 (1)
.github/workflows/automation.yml (1)
36-40: Verify Codecov action downgrade rationale.The Codecov action was downgraded from v5 to v4, which contradicts the official recommendation to use v5. While v4 is stable, confirm this change is intentional and document the reason (e.g., environment constraints, known v5 issues). Ensure your repository configuration is compatible with v4's requirements, particularly token handling.
♻️ Duplicate comments (3)
.github/workflows/automation.yml (1)
1-1: Critical: Remove placeholder text that breaks YAML parsing.The placeholder
[paste the simplified workflow here]at line 1 prevents GitHub Actions from parsing this workflow file. This completely blocks CI/CD execution.Apply this diff to remove the placeholder:
-[paste the simplified workflow here] - name: Cortex AutomationREADME.md (1)
534-534: Add language specification to code blocks.Code blocks at lines 534 and 621 are missing language identifiers. Add
textfor clarity:-``` +```text (content)Also applies to: 621-621
cortex/cli.py (1)
651-661: Critical: Unreachable code breaks existing commands.The early
return 0at line 661 makes lines 662-688 completely unreachable, breakinghistory,rollback,check-pref, andedit-prefcommands. Lines 651-660 duplicate the logic in lines 663-671 within the try block.Apply this diff to fix the control flow by removing the duplicate code block:
- if args.command == 'install': - return cli.install(args.software, execute=args.execute, dry_run=args.dry_run) - if args.command == 'update': - return cli.update(channel=args.channel, force=args.force, dry_run=args.dry_run) - if args.command == 'channel': - if args.channel_command == 'show': - return cli.show_channel() - if args.channel_command == 'set': - return cli.set_channel(args.channel) - - return 0 try: if args.command == 'install':Note: The past review comment indicates this was addressed in commits 2bc579f to aaca54e, but the issue has regressed. Please rebase onto the latest main to ensure the fix is properly merged.
🧹 Nitpick comments (1)
README.md (1)
339-340: Consider formatting bare URLs as markdown links.Lines 339-340 contain bare URLs. For better markdown hygiene, wrap them in link syntax:
-- **Discord**: https://discord.gg/uCqHvxjU83 -- **Email**: mike@cortexlinux.com +- **Discord**: [Join our server](https://discord.gg/uCqHvxjU83) +- **Email**: [mike@cortexlinux.com](mailto:mike@cortexlinux.com)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/automation.yml(2 hunks)LLM/requirements.txt(1 hunks)README.md(3 hunks)cortex/cli.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- LLM/requirements.txt
🧰 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
🧠 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 : Dry-run by default for all installations in command execution
Applied to files:
cortex/cli.py
🧬 Code graph analysis (1)
cortex/cli.py (4)
cortex/update_manifest.py (2)
UpdateChannel(18-28)from_string(23-28)cortex/updater.py (8)
ChecksumMismatch(59-60)InstallError(63-64)UpdateError(55-56)UpdateService(67-296)get_channel(100-106)perform_update(175-231)check_for_updates(130-172)set_channel(108-111)cortex/versioning.py (1)
from_string(30-35)cortex/coordinator.py (1)
execute(252-299)
🪛 actionlint (1.7.9)
.github/workflows/automation.yml
1-1: "jobs" section is missing in workflow
(syntax-check)
1-1: "on" section is missing in workflow
(syntax-check)
1-1: workflow is sequence node but mapping node is expected
(syntax-check)
🪛 LanguageTool
README.md
[uncategorized] ~350-~350: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ps team needs this Business Model: Open source community edition + Enterprise subscrip...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~656-~656: Consider using a more formal and expressive alternative to ‘amazing’.
Context: ...g-feature) 3. **Commit** your changes (git commit -m 'Add amazing feature') 4. **Push** to the branch (...
(AWESOME)
[grammar] ~740-~740: Use a hyphen to join words.
Context: ...stions. --- Contributions make the open source community amazing. Any contributi...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
README.md
95-95: No empty links
(MD042, no-empty-links)
112-112: Link fragments should be valid
(MD051, link-fragments)
123-123: Link fragments should be valid
(MD051, link-fragments)
339-339: Bare URL used
(MD034, no-bare-urls)
340-340: Bare URL used
(MD034, no-bare-urls)
454-454: Multiple headings with the same content
(MD024, no-duplicate-heading)
534-534: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
621-621: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
676-676: Multiple headings with the same content
(MD024, no-duplicate-heading)
🪛 Ruff (0.14.8)
cortex/cli.py
213-213: Do not catch blind exception: Exception
(BLE001)
218-218: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
229-229: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
247-247: Do not catch blind exception: Exception
(BLE001)
260-260: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
271-271: Possible SQL injection vector through string-based query construction
(S608)
🔇 Additional comments (5)
cortex/cli.py (5)
13-14: LGTM: Clean update service integration.The import of update-related classes and initialization of
UpdateServicein__init__follow good practices. The integration is clean and well-structured.Also applies to: 31-31
68-68: LGTM: Update notification on install.Calling
_notify_update_if_available()at the start of the install flow provides good UX by alerting users to available updates without blocking the operation.
195-239: LGTM: Well-structured update method.The
update()method implementation is solid:
- Clean channel resolution with proper error handling
- Specific exception handling for known error types
- Clear user messaging for all result states
- Release notes display when available
The catch of broad
Exceptionat line 213 is acceptable for this top-level command handler to prevent crashes, though it could log the exception type for debugging.
241-256: LGTM: Non-intrusive update notification.The
_notify_update_if_available()method is well-designed:
- Respects opt-out via
CORTEX_UPDATE_CHECKenvironment variable- Silent exception handling prevents update checks from disrupting user operations
- Clear, informative notification message
The broad exception handling at line 247 is appropriate here to ensure notification failures never break the user's workflow.
258-272: LGTM: Clean channel management methods.Both
show_channel()andset_channel()are well-implemented with:
- Simple delegation to
UpdateService- Proper validation and error handling
- Clear user feedback
…rtexlinux#208) Comprehensive code review and improvement of the Cortex Linux repository. - Added command validation in coordinator.py to prevent shell injection - Expanded dangerous command patterns in sandbox_executor.py (20+ new patterns) - Created cortex/utils/commands.py with secure command execution utilities - Created ASSESSMENT.md with full code audit report - Created ROADMAP.md with prioritized improvement plan - Rewrote README.md with comprehensive documentation - Updated CONTRIBUTING.md with detailed guidelines - Created CHANGELOG.md following Keep a Changelog format - Fixed automation.yml (wrong test directory tests/ → test/) - Added Python version matrix (3.10, 3.11, 3.12) - Added lint job (black, pylint) - Added security job (bandit, safety) - Added coverage reporting with Codecov - Created root requirements.txt with core dependencies - Created requirements-dev.txt with dev dependencies - Updated setup.py to use root requirements.txt - Standardized Python version to >=3.10 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Mike Morgan <allbots@allbots.io> Co-authored-by: Claude <noreply@anthropic.com>
|
@dhvll Can you please resolve these merge conflicts? We'll then merge this immediately |
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@dhvll Kindly resolve Conflicts. Also remove unnecessary files.
There was a problem hiding this comment.
why ? Remove this one from the changes.
CLA Verification FailedThe following contributors have not signed the Contributor License Agreement:
How to Sign
Verified Signers
This check runs automatically. Maintainers can update |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.github/workflows/automation.yml (1)
1-4: Unresolved merge conflict markers break CI.These Git conflict markers (
<<<<<<< HEAD,=======,>>>>>>> eece4f6) and the placeholder text will cause GitHub Actions to fail parsing this YAML file. Remove lines 1–4 entirely so the file starts withname: Cortex Automation.🔎 Proposed fix
-<<<<<<< HEAD -======= -[paste the simplified workflow here] ->>>>>>> eece4f6 (fix) name: Cortex Automation
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/automation.ymlREADME.mdcortex/cli.pycortex/llm_router.pyexamples/parallel_llm_demo.pyrequirements.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- requirements.txt
- README.md
🧰 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:
examples/parallel_llm_demo.pycortex/cli.pycortex/llm_router.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 **/*install*.py : Implement audit logging to ~/.cortex/history.db for all package operations
Applied to files:
cortex/cli.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 **/*.py : Docstrings required for all public APIs
Applied to files:
cortex/cli.py
🪛 GitHub Actions: CI
cortex/cli.py
[error] 1926-1926: Command 'ruff check . --output-format=github' failed. Ruff: W292 No newline at end of file (cortex/cli.py:1926:21).
🪛 GitHub Check: Lint
cortex/cli.py
[failure] 1926-1926: Ruff (W292)
cortex/cli.py:1926:21: W292 No newline at end of file
cortex/llm_router.py
[failure] 1065-1065: Ruff (W292)
cortex/llm_router.py:1065:39: W292 No newline at end of file
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@dhvll Please remove unnecessary changes from your PR and resolve my comments only when you have addressed them fully.
dhvll
force-pushed
the
self-update-version-mgmt
branch
from
2a66ace to
1467b25
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In @cortex/llm/llamacpp_backend.py:
- Line 27: The import line uses Callable and Iterator from typing which triggers
UP035; change the import so Callable and Iterator are imported from
collections.abc while keeping Any from typing (e.g., keep Any from typing and
import Callable, Iterator from collections.abc) so symbols Any, Callable, and
Iterator are referenced correctly in this module (llamacpp_backend.py).
- Around line 519-564: The load_model method currently returns None when
preload=True but is annotated to return LlamaCppModel and its docstring claims
it always returns a loaded model; update the signature of load_model to return
Optional[LlamaCppModel] (or LlamaCppModel | None) and adjust the docstring to
state that it returns a loaded model or None when preload=True (and mention that
background loading is started), and update any local type hints/usages in this
module that assume a non-None return to handle the None case.
In @cortex/llm/model_downloader.py:
- Around line 30-106: CURATED_MODELS lacks SHA256 checksums and downloads are
not integrity-verified; add a "sha256" field to each model entry in
CURATED_MODELS and update the model download flow (e.g., the function that saves
files — download_model / ModelDownloader.download) to compute SHA256 (streaming,
chunked read) and compare against the registry value, deleting the file and
raising an error on mismatch; reuse the checksum logic from updater.py's
_verify_checksum (same semantic behavior) and ensure the download path is
removed on failure and success returns only after verification.
- Around line 299-302: Replace uses of the deprecated alias IOError with OSError
in the exception handling of the downloader: in model_downloader.py change the
raise in the except block that currently does "raise IOError(f'Failed to
download {model_name}: {e}') from e" to raise OSError instead, and make the same
replacement at the other occurrence mentioned around line 365 so all IO-related
exception raises use OSError.
- Line 20: The import currently brings Callable from typing; update the import
in model_downloader.py to import Callable from collections.abc instead (replace
"from typing import Callable" with "from collections.abc import Callable") so it
satisfies Ruff UP035 and Python 3.9+ expectations; ensure any usages of Callable
in functions or type hints within model_downloader.py remain unchanged.
In @cortex/llm/preload_daemon.py:
- Around line 381-402: The reload handler currently calls _load_model which may
call sys.exit and raise SystemExit, terminating the daemon; modify
_handle_reload to explicitly catch SystemExit (in addition to Exception) and
return a PreloadResponse(success=False, error=str(e)) so the client gets an
error instead of the process exiting, or alternatively refactor _load_model to
raise regular exceptions instead of calling sys.exit and update callers
accordingly; ensure references to _handle_reload, _load_model, PreloadResponse
and SystemExit are used so the change is applied in the correct places.
- Around line 624-643: stop_daemon currently sends SIGTERM to PID from PID_FILE
and always returns True even if the process never exits; update stop_daemon to
verify process termination after the wait loop by attempting os.kill(pid, 0) (or
equivalent) and returning False if the process is still alive, and only return
True once the pid no longer exists; also remove the PID_FILE when you confirm
the process has exited; keep references to PID_FILE, stop_daemon, os.kill and
signal.SIGTERM to locate the logic to change.
In @docs/guides/Home.md:
- Line 43: Change the stray '+' bullet back to a '-' so the Community list is
consistent: replace the line containing "**Discussions:**
https://github.com/cortexlinux/cortex/discussions" that currently starts with
'+' with a '-' to match the other bullets.
In @examples/parallel_llm_demo.py:
- Line 255: The script calls asyncio.run(main()) twice causing main() to run
twice; remove the duplicate asyncio.run(main()) invocation so main() is only
executed once (locate the second asyncio.run(main()) call in examples that
follows the main() definition and delete it), ensuring the program runs demos a
single time.
In @manage_cortex_prs.sh:
- Around line 67-70: The macOS-specific call using date -j -f to convert the
ISO8601 string to epoch (used when setting created_ts from created) is not
portable; replace that conversion with a cross-platform approach (e.g., use
Python3 to parse the ISO8601 timestamp from the created variable and output the
epoch seconds) and assign that output to created_ts; do the same for the
duplicate occurrence later in the script where the same date parsing is
performed. Ensure the fallback behavior remains (return 0 on parse failure) and
keep references to the same variables (created and created_ts) so the subsequent
now_ts and age_days calculations work unchanged.
- Around line 15-16: The script insecurely and non-portably extracts
GITHUB_TOKEN by grepping ~/.zshrc and also leaves the variable unused; replace
that parsing with a robust approach: read the token from the environment (use
the existing GITHUB_TOKEN env variable) or, if you need to validate auth, call
the GitHub CLI (e.g. use gh auth status or gh auth token) to obtain/verify
credentials, and remove the GITHUB_TOKEN assignment if the token is not actually
used to avoid the unused-variable warning (SC2034); update any code that expects
the token to reference the env var or the gh CLI result instead of grepping
~/.zshrc.
- Around line 271-275: The membership check using [[ " ${draft_prs[@]} " =~ "
${pr_num} " ]] is unreliable; implement a helper function (e.g.,
contains_element) that accepts a value and an array (signature: contains_element
value "${array[@]}") and returns success if any element equals the value, then
replace the current condition in the for pr_num loop with calls to
contains_element for draft_prs and critical_prs (if contains_element "$pr_num"
"${draft_prs[@]}" || contains_element "$pr_num" "${critical_prs[@]}"; then
continue; fi) so exact-element comparisons are used instead of the flawed
regex-on-concatenated-array approach.
In @requirements.txt:
- Line 26: Remove the duplicate dependency entry "requests>=2.31.0" (the
secondary entry) from requirements.txt and keep the existing higher-minimum
entry "requests>=2.32.4" (the primary entry) so there is only a single requests
requirement; simply delete the lower-version line to avoid duplicate/conflicting
dependency specifications.
🧹 Nitpick comments (18)
README_backup_20251202_033440.md (1)
238-266: Add language identifier to fenced code block.The ASCII architecture diagram code block lacks a language specifier. Consider adding
textorplaintextto satisfy markdown linting and improve rendering consistency.📝 Suggested fix
-``` +```text User Inputscripts/build_llamacpp.sh (2)
84-98: Declare and assign separately to avoid masking return values.The
local var=$(command)pattern masks the exit status of the command. Ifuname -mfails, the error would be hidden.📝 Suggested fix
get_arch() { - local arch=$(uname -m) + local arch + arch=$(uname -m) case $arch in x86_64|amd64) echo "x86_64"
100-130: Declare and assign separately in build_llamacpp function.Same SC2155 issue applies here at lines 103 and 117.
📝 Suggested fix
build_llamacpp() { local backend=$1 - local arch=$(get_arch) + local arch + arch=$(get_arch) print_step "Building llama-cpp-python for ${backend} on ${arch}"And for CUDA version detection:
if command -v nvcc &> /dev/null; then - local cuda_version=$(nvcc --version | grep release | awk '{print $5}' | cut -d',' -f1) + local cuda_version + cuda_version=$(nvcc --version | grep release | awk '{print $5}' | cut -d',' -f1) print_step "Detected CUDA version: ${cuda_version}" fimanage_cortex_prs.sh (1)
72-93: Quote array assignments to prevent word splitting.The array assignments should quote the value to prevent word splitting issues with special characters in PR numbers (though PR numbers are typically numeric, defensive coding is preferred).
📝 Suggested fix
if [ "$is_draft" = "true" ]; then - draft_prs+=($pr_num) + draft_prs+=("$pr_num") continue fi if [[ "$title" == *"package"* ]] || [[ "$title" == *"Package"* ]] || [ "$pr_num" -eq 195 ]; then - critical_prs+=($pr_num) + critical_prs+=("$pr_num")Apply similar quoting to lines 83, 86, and 92.
cortex/versioning.py (2)
9-9: Unused import.
Optionalis imported but the code usesstr | Nonesyntax instead (PEP 604). This import can be removed.📝 Suggested fix
from dataclasses import dataclass from importlib import metadata -from typing import Optional from packaging.version import InvalidVersion, Version
29-35: Add docstring to public classmethod.Per coding guidelines, docstrings are required for all public APIs.
from_stringis a public classmethod that would benefit from documentation.📝 Suggested docstring
@classmethod def from_string(cls, raw_version: str) -> CortexVersion: + """ + Parse a version string into a CortexVersion instance. + + Args: + raw_version: Version string (e.g., "1.2.3"). + + Returns: + CortexVersion with both raw and parsed representations. + + Raises: + ValueError: If the version string is invalid. + """ try: parsed = Version(raw_version)cortex/updater.py (2)
68-85: Add docstrings to UpdateService public methods.Per coding guidelines, docstrings are required for all public APIs. The
UpdateServiceclass and its public methods (__init__,get_channel,set_channel,check_for_updates,perform_update) need documentation.📝 Suggested docstring for __init__
class UpdateService: + """ + Coordinates update checking and installation for Cortex. + + Manages manifest fetching, version comparison, artifact download, + checksum verification, and installation with automatic rollback on failure. + """ + def __init__( self, *, manifest_url: str | None = None, state_file: Path | None = None, system_info: SystemInfo | None = None, log_file: Path | None = None, ) -> None: + """ + Initialize the update service. + + Args: + manifest_url: URL to fetch update manifest from. + state_file: Path to persist update state. + system_info: System information for compatibility checks. + log_file: Path to write update logs. + """ self.manifest_url = manifest_url or os.environ.get(
116-121: Consider adding retry logic for manifest fetch.Network requests can fail due to transient issues. Consider adding retry logic with exponential backoff for the manifest fetch, especially since this is a critical operation.
📝 Example with basic retry
+ def _fetch_manifest(self, max_retries: int = 3) -> UpdateManifest: + last_error: Exception | None = None + for attempt in range(max_retries): + try: + response = requests.get(self.manifest_url, timeout=10) + response.raise_for_status() + payload = response.json() + return UpdateManifest.from_dict(payload) + except requests.RequestException as e: + last_error = e + if attempt < max_retries - 1: + import time + time.sleep(2 ** attempt) # Exponential backoff + raise UpdateError(f"Failed to fetch manifest: {last_error}") from last_error - def _fetch_manifest(self) -> UpdateManifest: - response = requests.get(self.manifest_url, timeout=10) - response.raise_for_status() - payload = response.json() - return UpdateManifest.from_dict(payload)cortex/update_manifest.py (2)
82-95: Clarify behavior when system.distro is None.When
system.distroisNoneandself.distrosis non-empty, the checksystem.distro not in self.distroswill returnTrue(sinceNoneis not in the list), causingis_compatibleto returnFalse. This is likely correct behavior, but consider adding a comment to clarify the intent.📝 Suggested clarification
def is_compatible(self, system: SystemInfo) -> bool: if self.python_spec and system.python_version not in self.python_spec: return False if self.os_names and system.os_name not in self.os_names: return False if self.architectures and system.architecture not in self.architectures: return False + # If distro requirements are specified but system distro is unknown, + # treat as incompatible to be conservative if self.distros and system.distro not in self.distros: return False return True
32-46: Add docstrings to public dataclasses.Per coding guidelines, docstrings are required for all public APIs.
SystemInfo,CompatibilityRule,ReleaseEntry, andUpdateManifestwould benefit from class-level documentation describing their purpose and fields.📝 Example for SystemInfo
@dataclass class SystemInfo: + """ + Information about the current system for compatibility checking. + + Attributes: + python_version: Parsed Python version. + os_name: Operating system name (lowercase). + architecture: CPU architecture (lowercase). + distro: Linux distribution ID, or None if unknown. + """ python_version: Version os_name: strcortex/llm/preload_daemon.py (6)
17-20: Fix trailing whitespace on blank lines.Static analysis flagged lines 17 and 20 as containing whitespace on blank lines (W293).
🧹 Proposed fix
# Start daemon cortex-preload start - + # Stop daemon cortex-preload stop - +
102-128: Fix trailing whitespace and add type annotations for instance variables.Line 105 contains trailing whitespace. Consider adding type annotations to instance variables for better clarity.
🧹 Proposed fix for whitespace and type hints
class PreloadDaemon: """ Daemon process that keeps models preloaded for fast inference. - + Uses Unix domain sockets for minimal IPC latency. """ def __init__(self, model_name: str | None = None): """ Initialize the daemon. Args: model_name: Model to preload (uses default if None) """ - self.model_name = model_name - self.model = None - self.manager = None - self.socket = None - self.running = False + self.model_name: str | None = model_name + self.model: Any = None + self.manager: Any = None + self.socket: socket.socket | None = None + self.running: bool = False self._lock = threading.Lock()
200-201: Remove unnecessary mode argument.The
"r"mode is the default foropen()and is flagged as unnecessary by the linter (UP015).🧹 Proposed fix
- with open("/dev/null", "r") as devnull: + with open("/dev/null") as devnull: os.dup2(devnull.fileno(), sys.stdin.fileno())
273-276: Replacesocket.timeoutwithTimeoutError.In modern Python (3.10+),
socket.timeoutis an alias forTimeoutError. The linter recommends using the builtin exception directly (UP041).🧹 Proposed fix
try: conn, _ = self.socket.accept() - except socket.timeout: + except TimeoutError: continue
290-326: Consider adding connection timeout and message size limit.The connection handler lacks a timeout on
recv()calls and doesn't validate the message length, which could lead to resource exhaustion if a client sends a very large length or stalls mid-transmission.🛡️ Proposed fix
def _handle_connection(self, conn: socket.socket) -> None: """Handle a client connection.""" try: + conn.settimeout(30.0) # Connection timeout + # Read request length length_data = conn.recv(4) if not length_data: return length = struct.unpack("!I", length_data)[0] + + # Prevent excessive memory allocation + max_message_size = 10 * 1024 * 1024 # 10 MB + if length > max_message_size: + raise ValueError(f"Message too large: {length} bytes") # Read request data data = b""
695-695: Remove f-string prefix from string without placeholders.Line 695 uses an f-string but has no placeholders, which is flagged by the linter (F541).
🧹 Proposed fix
- print(f"🔌 Preload Daemon Status") + print("🔌 Preload Daemon Status")cortex/llm/llamacpp_backend.py (2)
219-226: Unused variablefirst_token_timein non-streaming path.The variable
first_token_timeis initialized but never used in the non-streaming code path (lines 227-262).🧹 Proposed fix
with self._lock: try: start_time = time.perf_counter() - first_token_time = None if stream: return self._generate_stream(prompt, config, start_time)
627-665: Consider using existing hardware detection module.This function duplicates logic that exists in
cortex/kernel_features/hardware_detect.py(seedetect_accelerators). Additionally,subprocessis imported twice within the function.♻️ Suggested improvement
Move the
import subprocessto the top of the file, or better yet, leverage the existing hardware detection:def detect_best_backend() -> LlamaCppBackend: """Detect the best available compute backend.""" from cortex.kernel_features.hardware_detect import detect_accelerators, AcceleratorType hw = detect_accelerators() for acc in hw.accelerators: if acc.type == AcceleratorType.NVIDIA: return LlamaCppBackend.CUDA elif acc.type == AcceleratorType.AMD: return LlamaCppBackend.ROCM elif acc.type == AcceleratorType.APPLE: return LlamaCppBackend.METAL return LlamaCppBackend.CPU
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
POST_MVP_AUDIT.mdREADME.mdREADME_backup_20251202_033440.mdcortex/__init__.pycortex/llm/llamacpp_backend.pycortex/llm/model_downloader.pycortex/llm/preload_daemon.pycortex/update_manifest.pycortex/updater.pycortex/versioning.pydocs/LLAMACPP_INTEGRATION.mddocs/guides/Home.mdexamples/parallel_llm_demo.pymanage_cortex_prs.shrequirements.txtscripts/build_llamacpp.shtests/test_update_service.py
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- tests/test_update_service.py
- cortex/init.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:
examples/parallel_llm_demo.pycortex/versioning.pycortex/updater.pycortex/update_manifest.pycortex/llm/llamacpp_backend.pycortex/llm/model_downloader.pycortex/llm/preload_daemon.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 **/*.py : Type hints required in Python code
Applied to files:
requirements.txt
📚 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 **/*.py : Docstrings required for all public APIs
Applied to files:
cortex/updater.pycortex/update_manifest.py
🧬 Code graph analysis (5)
examples/parallel_llm_demo.py (2)
examples/progress_demo.py (1)
main(168-207)examples/standalone_demo.py (1)
main(123-178)
cortex/update_manifest.py (1)
cortex/versioning.py (3)
CortexVersion(23-38)is_newer_version(63-66)from_string(30-35)
cortex/llm/llamacpp_backend.py (2)
cortex/llm/preload_daemon.py (2)
_load_model(209-250)start(129-168)cortex/kernel_features/hardware_detect.py (1)
detect_accelerators(437-477)
cortex/llm/model_downloader.py (1)
cortex/kernel_features/hardware_detect.py (1)
detect_accelerators(437-477)
cortex/llm/preload_daemon.py (2)
cortex/kernel_features/model_lifecycle.py (1)
register(116-123)cortex/llm/llamacpp_backend.py (10)
_load_model(138-192)ModelManager(423-624)default_model(614-616)default_model(619-624)list_available_models(606-611)register_model(490-517)load_model(519-569)GenerationConfig(84-95)generate(194-265)chat(294-360)
🪛 GitHub Actions: CI
cortex/llm/llamacpp_backend.py
[error] 27-27: ruff check failed: UP035 Import from 'collections.abc' instead: 'Callable', 'Iterator'. Replace 'from collections import Callable, Iterator' with 'from collections.abc import Callable, Iterator'.
🪛 GitHub Actions: Cortex Automation
cortex/llm/llamacpp_backend.py
[error] 27-27: Ruff UP035: Import from collections.abc should be used for Callable and Iterator in cortex/llm/llamacpp_backend.py:27:1.
🪛 GitHub Check: lint
cortex/llm/llamacpp_backend.py
[failure] 27-27: Ruff (UP035)
cortex/llm/llamacpp_backend.py:27:1: UP035 Import from collections.abc instead: Callable, Iterator
cortex/llm/model_downloader.py
[failure] 365-365: Ruff (UP024)
cortex/llm/model_downloader.py:365:19: UP024 Replace aliased errors with OSError
[failure] 301-301: Ruff (UP024)
cortex/llm/model_downloader.py:301:19: UP024 Replace aliased errors with OSError
[failure] 20-20: Ruff (UP035)
cortex/llm/model_downloader.py:20:1: UP035 Import from collections.abc instead: Callable
cortex/llm/preload_daemon.py
[failure] 695-695: Ruff (F541)
cortex/llm/preload_daemon.py:695:15: F541 f-string without any placeholders
[failure] 275-275: Ruff (UP041)
cortex/llm/preload_daemon.py:275:24: UP041 Replace aliased errors with TimeoutError
[failure] 200-200: Ruff (UP015)
cortex/llm/preload_daemon.py:200:32: UP015 Unnecessary mode argument
[failure] 105-105: Ruff (W293)
cortex/llm/preload_daemon.py:105:1: W293 Blank line contains whitespace
[failure] 20-20: Ruff (W293)
cortex/llm/preload_daemon.py:20:1: W293 Blank line contains whitespace
[failure] 17-17: Ruff (W293)
cortex/llm/preload_daemon.py:17:1: W293 Blank line contains whitespace
🪛 GitHub Check: Lint
cortex/llm/llamacpp_backend.py
[failure] 27-27: Ruff (UP035)
cortex/llm/llamacpp_backend.py:27:1: UP035 Import from collections.abc instead: Callable, Iterator
cortex/llm/model_downloader.py
[failure] 365-365: Ruff (UP024)
cortex/llm/model_downloader.py:365:19: UP024 Replace aliased errors with OSError
[failure] 301-301: Ruff (UP024)
cortex/llm/model_downloader.py:301:19: UP024 Replace aliased errors with OSError
[failure] 20-20: Ruff (UP035)
cortex/llm/model_downloader.py:20:1: UP035 Import from collections.abc instead: Callable
cortex/llm/preload_daemon.py
[failure] 695-695: Ruff (F541)
cortex/llm/preload_daemon.py:695:15: F541 f-string without any placeholders
[failure] 275-275: Ruff (UP041)
cortex/llm/preload_daemon.py:275:24: UP041 Replace aliased errors with TimeoutError
[failure] 200-200: Ruff (UP015)
cortex/llm/preload_daemon.py:200:32: UP015 Unnecessary mode argument
[failure] 105-105: Ruff (W293)
cortex/llm/preload_daemon.py:105:1: W293 Blank line contains whitespace
[failure] 20-20: Ruff (W293)
cortex/llm/preload_daemon.py:20:1: W293 Blank line contains whitespace
[failure] 17-17: Ruff (W293)
cortex/llm/preload_daemon.py:17:1: W293 Blank line contains whitespace
🪛 LanguageTool
POST_MVP_AUDIT.md
[style] ~460-~460: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... Already approved but never merged -
(EN_WEAK_ADJECTIVE)
README_backup_20251202_033440.md
[style] ~357-~357: Consider using a more formal and expressive alternative to ‘amazing’.
Context: ...g-feature) 3. **Commit** your changes (git commit -m 'Add amazing feature') 4. **Push** to the branch (...
(AWESOME)
🪛 markdownlint-cli2 (0.18.1)
README_backup_20251202_033440.md
5-5: Bare URL used
(MD034, no-bare-urls)
255-255: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
360-360: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
392-392: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
422-422: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/LLAMACPP_INTEGRATION.md
5-5: Bare URL used
(MD034, no-bare-urls)
255-255: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/guides/Home.md
43-43: Bare URL used
(MD034, no-bare-urls)
🪛 Shellcheck (0.11.0)
scripts/build_llamacpp.sh
[warning] 86-86: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 103-103: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 117-117: Declare and assign separately to avoid masking return values.
(SC2155)
manage_cortex_prs.sh
[warning] 15-15: GITHUB_TOKEN appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 74-74: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 80-80: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 83-83: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 86-86: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 92-92: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[error] 273-273: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 273-273: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[error] 273-273: Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @).
(SC2199)
[warning] 273-273: Remove quotes from right-hand side of =~ to match as a regex rather than literally.
(SC2076)
[warning] 365-365: status appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 365-365: payment appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (13)
docs/LLAMACPP_INTEGRATION.md (1)
1-352: LGTM! Comprehensive llama.cpp integration documentation.This documentation provides excellent coverage of the native llama.cpp integration, including:
- Clear comparison with the Ollama approach
- Step-by-step quick start guide
- Architecture diagrams
- Hardware-specific build instructions
- Troubleshooting guidance
- API reference and CLI commands
The content is well-organized and will help users understand and adopt the native integration.
Note: The static analysis warning about a missing language specifier at line 255 can be safely ignored—it's an ASCII art diagram block, not a code snippet.
POST_MVP_AUDIT.md (1)
1-769: LGTM! Well-structured post-MVP audit report.This comprehensive audit document provides excellent visibility into:
- Current MVP completion status and gaps
- Security and repository settings that need attention
- Open PR triage with specific action items
- Contributor pipeline and outstanding bounties
- Timeline and risk assessment for funding readiness
The structured format with CLI commands and actionable tasks makes this a valuable project management artifact.
Note: The LanguageTool style suggestion about "very old" is a minor stylistic preference and doesn't affect the clarity of the document.
README_backup_20251202_033440.md (1)
1-1: Backup file committed to repository.This appears to be a backup of the README with a timestamp in the filename (
README_backup_20251202_033440.md). Backup files are typically not committed to version control. Was this intentional, or should this file be removed or added to.gitignore?scripts/build_llamacpp.sh (1)
26-48: LGTM!Clean, well-organized output helper functions with consistent color scheme and formatting.
cortex/versioning.py (1)
41-60: LGTM!Well-structured fallback chain for version detection with proper exception handling. The progression from package metadata →
__version__→ "0.0.0" fallback is robust.cortex/updater.py (2)
216-238: LGTM!Good implementation of the update flow with proper cleanup in
finallyblock and rollback on failure. The temporary directory management is correct.
273-276: This concern is based on a misconception about pip's behavior. According to PEP 592, pip can install yanked versions when an exact version is explicitly specified (using==). Since the code uses{PACKAGE_NAME}=={previous.raw}, the rollback mechanism will successfully install even if the previous version has been yanked from PyPI — pip will issue a warning but proceed with the installation. No changes needed.Likely an incorrect or invalid review comment.
cortex/update_manifest.py (1)
159-177: LGTM!
find_latestcorrectly filters by channel, system compatibility, and version comparison. The sorting by parsed version ensures the newest eligible release is returned.cortex/llm/model_downloader.py (1)
201-236: LGTM!Good implementation of resumable downloads with proper error handling and progress reporting. The
forceparameter and early return for existing files are well-designed.cortex/llm/preload_daemon.py (1)
413-478: LGTM!The completion and chat handlers correctly use locking for thread-safe access to the model and properly update statistics. The serialized inference is appropriate for a single-model daemon.
cortex/llm/llamacpp_backend.py (3)
61-110: LGTM!The dataclasses are well-structured with appropriate defaults, correct use of
field(default_factory=list)for the mutablestoplist, and comprehensive type hints.
434-449: LGTM!The singleton pattern with double-checked locking is correctly implemented for thread-safe initialization.
752-839: LGTM!The CLI is well-structured with appropriate subcommands, user-friendly output with emojis, and proper handling of keyboard interrupts in the interactive chat mode.
There was a problem hiding this comment.
@dhvll Address coderabbitai comments also CI checks are failing. Making it draft until then.
|
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@dhvll Are u still Interested to continue the PR ? If not let me know.
|
|
@dhvll These are some of the changes address them: Also address all coderabbitai comments, CI checks also failing. |
6 participants
Summary
cortex/versioning.py,update_manifest.py,updater.py) that fetches manifests, enforces compatibility, downloads artifacts with SHA256 verification, and installs them via pip with automatic rollback.cortex update(--dry-run,--force,--channel) pluscortex channel show/set, and wired startup notifications intocortex install.test/test_update_service.py) covering manifest selection, channel persistence, and dry-run behavior.Checking & Installing Updates
Testing
Type of Change
Closes #30
Screencast.from.18-11-25.07.28.20.PM.IST.webm
AI Disclosure
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.