Allows soar apply to track versioning with URL packages.
#129
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughApply now builds a single unified InstallTarget for URL packages, treats URL targets as unpinned during resolution, and uses Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/soar-cli/src/apply.rscrates/soar-cli/src/update.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/soar-cli/src/apply.rs (2)
crates/soar-cli/src/update.rs (1)
perform_update(219-311)crates/soar-core/src/package/install.rs (1)
new(85-153)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: coverage
- GitHub Check: test
🔇 Additional comments (5)
crates/soar-cli/src/update.rs (1)
219-219: LGTM! Visibility change enables apply flow integration.Making
perform_updatepublic allows the apply module to use update logic for handling package updates, which is essential for tracking versioning with URL packages.crates/soar-cli/src/apply.rs (4)
26-26: LGTM! Import enables update flow integration.The import of
perform_updateis necessary for the apply command to utilize update logic when handling package version changes.
103-107: LGTM! Version tracking added to declared packages.Adding version to
declared_keysenables version-aware package tracking, allowing the prune operation to correctly identify and remove old versions that are no longer declared in the configuration.
279-284: Verify prune logic interacts correctly with version-less declarations.The prune logic treats a
Noneversion in declarations as "match any version" (line 283:map_or(true, ...)), meaning packages without a specified version in the config won't be pruned regardless of their installed version.This interacts with the logic at lines 156-157, where
Noneversions currently trigger updates. This creates an inconsistency:
- Apply behavior: Marks packages with
version=Nonefor update- Prune behavior: Treats
version=Noneas matching any installed version (won't prune)After fixing the issue at lines 156-157, ensure the behaviors align:
- If
version=Noneshould mean "accept any version" → both should treat as in-sync/no-prune- If
version=Noneshould mean "always update" → prune should also not matchCurrent recommendation: The prune logic appears correct for the "accept any version" interpretation, but this should align with the apply logic once fixed.
515-515: LGTM! Update flow correctly usesperform_update.The call to
perform_updatewithkeep=trueis appropriate here. Old versions are preserved during the update phase and will be removed by the subsequent prune phase (lines 520-539) if they're no longer declared, which aligns with the version tracking objectives.
| let is_already_installed = installed_packages.iter().any(|ip| ip.is_installed); | ||
|
|
||
| let existing_install = installed_packages.into_iter().next(); | ||
| let target = InstallTarget { | ||
| package: url_pkg.to_package(), | ||
| existing_install: existing_install.clone(), | ||
| with_pkg_id: url_pkg.pkg_type.is_some(), | ||
| pinned: false, | ||
| profile: pkg.profile.clone(), | ||
| portable: pkg.portable.as_ref().and_then(|p| p.path.clone()), | ||
| portable_home: pkg.portable.as_ref().and_then(|p| p.home.clone()), | ||
| portable_config: pkg.portable.as_ref().and_then(|p| p.config.clone()), | ||
| portable_share: pkg.portable.as_ref().and_then(|p| p.share.clone()), | ||
| portable_cache: pkg.portable.as_ref().and_then(|p| p.cache.clone()), | ||
| }; | ||
|
|
||
| if !is_already_installed { | ||
| let existing_install = installed_packages.into_iter().next(); | ||
| let target = InstallTarget { | ||
| package: url_pkg.to_package(), | ||
| existing_install, | ||
| with_pkg_id: url_pkg.pkg_type.is_some(), | ||
| pinned: true, | ||
| profile: pkg.profile.clone(), | ||
| portable: pkg.portable.as_ref().and_then(|p| p.path.clone()), | ||
| portable_home: pkg.portable.as_ref().and_then(|p| p.home.clone()), | ||
| portable_config: pkg.portable.as_ref().and_then(|p| p.config.clone()), | ||
| portable_share: pkg.portable.as_ref().and_then(|p| p.share.clone()), | ||
| portable_cache: pkg.portable.as_ref().and_then(|p| p.cache.clone()), | ||
| }; | ||
| diff.to_install.push((pkg.clone(), target)); | ||
| } else if pkg.version != Some(existing_install.unwrap().version) { | ||
| diff.to_update.push((pkg.clone(), target)); |
There was a problem hiding this comment.
Critical: Logic bug in existing package detection for URL packages.
Lines 138 and 140 have a mismatch that can cause incorrect version comparisons:
- Line 138 checks if any package in the list has
is_installed=true - Line 140 takes the first package from the list via
.next()
These could reference different packages when multiple versions exist in the database. The query at lines 120-133 doesn't filter by is_installed or version, so it can return multiple package records (e.g., different versions where some are installed and others are not).
Example scenario:
Query returns: [PackageV1(is_installed=false), PackageV2(is_installed=true)]
is_already_installed = true(because V2 is installed)existing_install = Some(PackageV1)(wrong package!)- Line 156 compares against V1's version instead of V2's version
This will cause incorrect update/sync decisions and potential unwrap panics.
🔎 Proposed fix
- let is_already_installed = installed_packages.iter().any(|ip| ip.is_installed);
-
- let existing_install = installed_packages.into_iter().next();
+ let existing_install = installed_packages.into_iter().find(|ip| ip.is_installed);
+ let is_already_installed = existing_install.is_some();
+
let target = InstallTarget {
package: url_pkg.to_package(),
existing_install: existing_install.clone(),This ensures existing_install always references the actually installed package (if any), making the subsequent logic correct.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let is_already_installed = installed_packages.iter().any(|ip| ip.is_installed); | |
| let existing_install = installed_packages.into_iter().next(); | |
| let target = InstallTarget { | |
| package: url_pkg.to_package(), | |
| existing_install: existing_install.clone(), | |
| with_pkg_id: url_pkg.pkg_type.is_some(), | |
| pinned: false, | |
| profile: pkg.profile.clone(), | |
| portable: pkg.portable.as_ref().and_then(|p| p.path.clone()), | |
| portable_home: pkg.portable.as_ref().and_then(|p| p.home.clone()), | |
| portable_config: pkg.portable.as_ref().and_then(|p| p.config.clone()), | |
| portable_share: pkg.portable.as_ref().and_then(|p| p.share.clone()), | |
| portable_cache: pkg.portable.as_ref().and_then(|p| p.cache.clone()), | |
| }; | |
| if !is_already_installed { | |
| let existing_install = installed_packages.into_iter().next(); | |
| let target = InstallTarget { | |
| package: url_pkg.to_package(), | |
| existing_install, | |
| with_pkg_id: url_pkg.pkg_type.is_some(), | |
| pinned: true, | |
| profile: pkg.profile.clone(), | |
| portable: pkg.portable.as_ref().and_then(|p| p.path.clone()), | |
| portable_home: pkg.portable.as_ref().and_then(|p| p.home.clone()), | |
| portable_config: pkg.portable.as_ref().and_then(|p| p.config.clone()), | |
| portable_share: pkg.portable.as_ref().and_then(|p| p.share.clone()), | |
| portable_cache: pkg.portable.as_ref().and_then(|p| p.cache.clone()), | |
| }; | |
| diff.to_install.push((pkg.clone(), target)); | |
| } else if pkg.version != Some(existing_install.unwrap().version) { | |
| diff.to_update.push((pkg.clone(), target)); | |
| let existing_install = installed_packages.into_iter().find(|ip| ip.is_installed); | |
| let is_already_installed = existing_install.is_some(); | |
| let target = InstallTarget { | |
| package: url_pkg.to_package(), | |
| existing_install: existing_install.clone(), | |
| with_pkg_id: url_pkg.pkg_type.is_some(), | |
| pinned: false, | |
| profile: pkg.profile.clone(), | |
| portable: pkg.portable.as_ref().and_then(|p| p.path.clone()), | |
| portable_home: pkg.portable.as_ref().and_then(|p| p.home.clone()), | |
| portable_config: pkg.portable.as_ref().and_then(|p| p.config.clone()), | |
| portable_share: pkg.portable.as_ref().and_then(|p| p.share.clone()), | |
| portable_cache: pkg.portable.as_ref().and_then(|p| p.cache.clone()), | |
| }; | |
| if !is_already_installed { | |
| diff.to_install.push((pkg.clone(), target)); | |
| } else if pkg.version != Some(existing_install.unwrap().version) { | |
| diff.to_update.push((pkg.clone(), target)); |
crates/soar-cli/src/apply.rs
Outdated
| } else if pkg.version != Some(existing_install.unwrap().version) { | ||
| diff.to_update.push((pkg.clone(), target)); | ||
| } else { | ||
| diff.in_sync.push(format!("{} (local)", pkg.name)); | ||
| } |
There was a problem hiding this comment.
Major: Incorrect update logic when version is not specified.
When pkg.version is None (version not specified in config), the condition on line 156 evaluates to None != Some(installed.version), which is always true, causing the package to be marked for update even though it's already installed.
Expected behavior: If no version is specified in the configuration, the installed package should be treated as in-sync, not marked for update.
Example:
- Config declares URL package without version:
pkg.version = None - Package already installed with version "1.0"
- Current logic:
None != Some("1.0")= true → marked for update - Expected: Should be marked as in_sync
🔎 Proposed fix
if !is_already_installed {
diff.to_install.push((pkg.clone(), target));
- } else if pkg.version != Some(existing_install.unwrap().version) {
+ } else if pkg.version.as_ref().is_some_and(|v| *v != existing_install.unwrap().version) {
diff.to_update.push((pkg.clone(), target));
} else {
diff.in_sync.push(format!("{} (local)", pkg.name));
}This ensures updates only occur when a version is explicitly declared and differs from the installed version.
🤖 Prompt for AI Agents
In crates/soar-cli/src/apply.rs around lines 156 to 160, the code treats None
(no version specified) as different from the installed version and wrongly
schedules an update; change the logic so we only mark a package for update when
a version is explicitly declared and that declared version differs from the
installed version (e.g., check pkg.version.is_some() or use if let
Some(declared) = pkg.version { if declared != existing_install.unwrap().version
{ push update } else push in_sync } else push in_sync), ensuring packages with
no declared version are treated as in_sync.
…d unpin/unkeep url packages
|
PR should be good now. cleanup seems to be working after unkeeping/unpinning url packages. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/soar-cli/src/install.rs (1)
1150-1243: Signature verification logic appears unrelated to PR objectives.Lines 1150-1243 implement signature verification using minisign, which is not mentioned in the PR objectives or issue #128. This functionality:
- Loads a minisign public key from the repository
- Verifies signatures for downloaded files
- Removes signature files after successful verification
- Warns if no public key is found
The implementation appears functionally correct, but it's unclear whether this is intentionally included in this PR or merged from another branch. If this is a separate feature, consider:
- Moving it to a dedicated PR for focused review and testing
- Adding tests specifically for signature verification
- Documenting the signature verification feature in the PR description
If this is intentional and related to URL package handling, please clarify the connection.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/soar-cli/src/apply.rscrates/soar-cli/src/install.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/soar-cli/src/apply.rs
🔇 Additional comments (1)
crates/soar-cli/src/install.rs (1)
220-274: URL packages are now unpinned for version tracking.The change at line 269 sets
pinned: falsefor all new URL-based packages, which fixes issue #128. This allowssoar applyto detect and apply version changes for URL packages.However, note that URL packages use a different update mechanism than repository packages: they rely on direct version comparison (at apply time) rather than the
pinnedflag logic. For URL packages,soar applycomparesurl_pkg.versionagainstexisting_install.versionto determine if an update is needed—thepinnedfield is not consulted. By contrast, repository packages respect thepinnedflag: they only trigger updates if unpinned or if a version is explicitly specified.Existing URL packages don't require migration: Packages previously installed with
pinned: truewill continue to work correctly because the pinned status doesn't affect the URL package update logic. Version changes will still be detected and applied via the direct version comparison at apply time.
2 participants
Currently partially fixes #128, where running
soar apply --prunecleans up package of different version from what's listed under the toml. However, it doesn't clean it up undersoar clean. Not sure why as pinning is already switched to false.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.