feat(update): allow updating remote URL packages #137
Conversation
📝 WalkthroughWalkthroughAdds a remote update subsystem to core (GitHub/GitLab/URL/Command), exposes UpdateSource in config with persistence for URL-backed package updates, and integrates URL-update detection and URL-specific install-target creation into CLI update/apply/install flows. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (update/apply)
participant Config as PackagesConfig
participant Remote as remote_update::check_for_update
participant Source as Remote Source (GitHub/GitLab/URL/Command)
participant TOML as packages.toml
CLI->>Config: load resolved packages (with UpdateSource)
loop for each installed local URL-backed package
CLI->>Remote: check_for_update(update_source, current_version)
Remote->>Source: fetch data (API/JSON/command)
Source-->>Remote: return new_version + download_url
Remote-->>CLI: RemoteUpdate (if newer)
CLI->>CLI: build InstallTarget (create_url_install_target)
end
alt updates applied
CLI->>TOML: update_package_url(package, new_url, new_version)
TOML-->>CLI: success / warning
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
🤖 Fix all issues with AI agents
In @crates/soar-cli/src/apply.rs:
- Around line 304-323: In create_url_install_target replace the current
with_pkg_id logic that uses url_pkg.pkg_type.is_some() with a check consistent
with regular packages (use resolved.pkg_id.is_some()), i.e., set with_pkg_id to
resolved.pkg_id.is_some() so URL packages use the same semantic (explicit pkg_id
present) as non-URL packages; update the InstallTarget construction inside the
create_url_install_target function accordingly.
In @crates/soar-config/src/packages.rs:
- Around line 368-373: The current fallback branch that converts a simple-string
package to an inline table replaces the original version, losing version info;
update the branch handling (the _ => block that mutates package) to preserve
version by extracting the original string version (from the existing
toml_edit::Item::Value if it was a Value::String) or, if provided, use the
new_version parameter, and then insert that into the InlineTable as "version"
alongside the "url" (i.e., set table.insert("version", version.into())). Ensure
you still set table.insert("url", new_url.into()) and assign *package =
toml_edit::Item::Value(toml_edit::Value::InlineTable(table)) as before.
🧹 Nitpick comments (2)
crates/soar-core/Cargo.toml (1)
24-24: Addsemverto workspace dependencies for consistency.The
semverdependency uses an explicit version"1.0"while all other dependencies useworkspace = true. Additionally,semver = "1.0.27"in soar-cli uses a different version spec than soar-core. For centralized version management and consistency across crates, addsemverto[workspace.dependencies]in the root Cargo.toml and update this crate to use{ workspace = true }.crates/soar-core/src/package/remote_update.rs (1)
279-295: Non-semver fallback may cause spurious update notifications.When semver parsing fails, the function treats any version difference as "newer" (line 293). This means:
is_newer_version("2.0", "1.0")returnstrue(incorrect - downgrade)is_newer_version("abc", "def")returnstrue(arbitrary)This is documented and tested, so it appears intentional. Consider whether a stricter approach (e.g., returning
falsewhen semver fails, requiring users to manage non-semver packages manually) would reduce unexpected update prompts.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/soar-cli/src/apply.rscrates/soar-cli/src/update.rscrates/soar-config/src/error.rscrates/soar-config/src/packages.rscrates/soar-core/Cargo.tomlcrates/soar-core/src/package/mod.rscrates/soar-core/src/package/remote_update.rscrates/soar-core/src/package/url.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/soar-core/src/package/remote_update.rs (2)
crates/soar-dl/src/http.rs (1)
json(122-134)crates/soar-dl/src/platform.rs (1)
parse(72-113)
⏰ 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: test
- GitHub Check: coverage
🔇 Additional comments (26)
crates/soar-config/src/error.rs (1)
112-114: LGTM! Generic custom error variant added.The
Custom(String)error variant provides flexibility for handling various error scenarios that don't fit into specific error types. The implementation is correct and follows the existing error pattern.crates/soar-core/src/package/mod.rs (1)
3-3: LGTM! Module export for remote update functionality.The addition of the
remote_updatemodule is straightforward and properly exposes the new update checking functionality.crates/soar-core/src/package/url.rs (2)
28-35: LGTM! Improved URL validation with proper parsing.The updated
is_urlfunction now usesurl::Url::parsefor robust validation in addition to prefix checking. This is more reliable than the previous approach that only checked prefixes.
189-192: LGTM! Consistent version normalization.Stripping the 'v' prefix from version overrides ensures consistent version formatting across the codebase. The comment clearly explains the normalization.
crates/soar-cli/src/apply.rs (1)
141-145: Good refactoring with improved reference handling.The extraction of
create_url_install_targetreduces code duplication effectively. The change fromexisting_install.clone().unwrap().versiontoexisting_install.as_ref().unwrap().versionon Line 143 is an improvement that avoids unnecessary cloning.crates/soar-core/src/package/remote_update.rs (10)
16-25: LGTM!The
RemoteUpdatestruct is well-designed with appropriate fields for tracking update information.
37-75: LGTM!Clean dispatcher pattern that delegates to the appropriate update checker based on the
UpdateSourcevariant.
117-155: LGTM!Consistent implementation with the GitHub counterpart. The same consideration about release ordering applies here as well.
157-200: LGTM!The URL-based update check correctly fetches JSON, extracts values using dot-path notation, and validates the download URL before returning.
202-251: Command execution is inherently trusted; ensure documentation reflects this.The
check_commandfunction executes arbitrary shell commands. While this is by design for flexibility, ensure the documentation clearly states that users should only configure trusted commands in theirpackages.toml, as this executes with the user's privileges.The implementation correctly validates the output format and download URL.
253-273: LGTM!Solid URL validation that correctly rejects non-HTTP(S) protocols and malformed URLs.
297-325: LGTM!Good use of glob matching with helpful error messages that list available assets when no match is found.
327-349: LGTM!Clean JSON path extraction with support for array indexing. The implementation correctly handles nested paths and array access patterns like
assets[0].name.
351-420: LGTM!Comprehensive unit tests covering version comparison, JSON extraction, and URL validation edge cases.
84-95: The API request efficiency concern is overstated; the code already uses efficient short-circuit evaluation.The code fetches up to 100 releases per request (via
per_page=100) but uses.find()to return the first matching release, which short-circuits iteration—not fetching all releases. Since GitHub's API returns releases sorted bycreated_atin descending order (newest first), the code correctly selects the most recent non-prerelease. Theis_newer_version()function uses semver comparison, which handles version ordering correctly even if releases are published out of order.However, if a repository has more than 100 releases, older releases won't be considered since pagination isn't implemented. This is rarely problematic in practice.
crates/soar-cli/src/update.rs (6)
52-58: LGTM!Simple struct for tracking URL package updates that need to be persisted to
packages.toml.
71-76: LGTM!Gracefully handles missing
packages.tomlby falling back to empty resolved packages, allowing updates to proceed for repository-backed packages.
103-116: LGTM!Correctly handles local packages by checking for updates via the configured update source, with appropriate logging when no source is configured.
169-201: LGTM!Clean separation of local and repository package update checking. The defensive check at lines 199-201 ensures local packages aren't processed twice even if
list_updatableincludes them.
272-304: LGTM!Good defensive approach: the code verifies the new version is actually installed before updating
packages.toml, preventing false updates if the installation failed. Warning on failure rather than hard error is appropriate for this non-critical post-update step.
344-356: Verify: Config values override installed package settings.The
InstallTargetuses values fromresolved(config) rather thanpkg(installed package) forpinned,profile,portable, andentrypoint. This means config is treated as the source of truth.If a user pins a package via CLI but their config has
pinned = false, the update would unpin it. Ensure this is the intended behavior or consider preserving the installed package'spinnedstate.crates/soar-config/src/packages.rs (5)
106-109: LGTM!Clean addition of the optional
updatefield for configuring remote update sources.
130-171: LGTM!Well-designed tagged enum for update sources with appropriate documentation. The
serde(tag = "type")enables clean TOML syntax likeupdate = { type = "github", repo = "owner/repo" }.
188-188: LGTM!Correctly propagates the update configuration to resolved packages.
221-223: Breaking behavior change: URL packages no longer auto-pinned.Previously, packages with a
urlfield were implicitly pinned. Now, only packages with an explicit version (and no URL) are auto-pinned. This enables URL packages to receive updates but may change behavior for existing users who expected URL packages to remain static.Consider documenting this change in release notes or migration guides.
215-216: LGTM!Correctly resolves the update field -
Nonefor simple specs, propagated from options for detailed specs.Also applies to: 246-247
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @crates/soar-cli/src/apply.rs:
- Around line 137-145: The logic uses installed_packages.iter().any(...) and
existing_install = installed_packages.into_iter().next(), which can lead to
unwrap() panics or comparing the wrong entry; change to locate the actual
installed entry via a find on installed_packages (e.g., find(|ip|
ip.is_installed)) and use that Option for both the existence check and version
comparison before calling create_url_install_target; ensure you handle the None
case safely (push to to_install) and only unwrap/access the found installed
package when Some, then push to to_update if versions differ, using the same
found item for both checks.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/soar-cli/src/apply.rscrates/soar-config/src/packages.rs
⏰ 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: test
- GitHub Check: coverage
🔇 Additional comments (3)
crates/soar-cli/src/apply.rs (1)
304-323: LGTM! Good refactoring to reduce duplication.The new helper function properly extracts URL install target creation logic and mirrors the pattern used in
create_install_targetfor metadata packages. This improves maintainability and consistency.crates/soar-config/src/packages.rs (2)
130-171: LGTM! Well-structured enum for update source configuration.The
UpdateSourceenum appropriately models different update mechanisms (GitHub, GitLab, custom URL endpoints, and shell commands). The tagged representation with serde is clean and allows for easy configuration in TOML.
221-223: Behavioral change: URL packages no longer auto-pinned.The pinning logic change enables the PR objective of allowing URL package updates. Previously, URL packages were always pinned; now they can be updated unless explicitly marked as pinned. This is a sensible change, but existing URL packages in configurations will now be eligible for updates where they weren't before.
Consider documenting this behavioral change in release notes or migration guides, as users may observe different update behavior for their URL-based packages.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @crates/soar-cli/src/apply.rs:
- Around line 137-145: The code incorrectly uses
installed_packages.into_iter().next() as existing_install while
is_already_installed checks any(|ip| ip.is_installed), which can pick a
non-installed record; change the logic to find the actual installed record
(e.g., installed_packages.iter().find(|ip| ip.is_installed)) and use that Option
(existing_install) for both the version comparison (compare url_pkg.version
against existing_install.as_ref().map(|e| &e.version)) and when calling
create_url_install_target so the update detection and target creation use the
true installed package record rather than the first list entry.
🧹 Nitpick comments (1)
crates/soar-cli/src/apply.rs (1)
304-323: Consider consolidating target creation logic.The
create_url_install_targetandcreate_install_targetfunctions (lines 284-302) are nearly identical, differing only in the package source. While the separation provides clarity, you could reduce duplication by extracting common field mapping logic.♻️ Optional refactor to reduce duplication
/// Create an InstallTarget from resolved package info fn create_install_target( resolved: &ResolvedPackage, package: Package, existing: Option<InstalledPackage>, ) -> InstallTarget { build_install_target(package, resolved, existing) } /// Create an InstallTarget for a URL package fn create_url_install_target( url_pkg: &UrlPackage, resolved: &ResolvedPackage, existing: Option<InstalledPackage>, ) -> InstallTarget { build_install_target(url_pkg.to_package(), resolved, existing) } /// Common logic for building InstallTarget fn build_install_target( package: Package, resolved: &ResolvedPackage, existing: Option<InstalledPackage>, ) -> InstallTarget { InstallTarget { package, existing_install: existing, with_pkg_id: false, pinned: resolved.pinned, profile: resolved.profile.clone(), portable: resolved.portable.as_ref().and_then(|p| p.path.clone()), portable_home: resolved.portable.as_ref().and_then(|p| p.home.clone()), portable_config: resolved.portable.as_ref().and_then(|p| p.config.clone()), portable_share: resolved.portable.as_ref().and_then(|p| p.share.clone()), portable_cache: resolved.portable.as_ref().and_then(|p| p.cache.clone()), entrypoint: resolved.entrypoint.clone(), } }
📜 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
⏰ 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 (3)
crates/soar-cli/src/install.rs (2)
269-276: LGTM - Consistent URL package handling.Setting
with_pkg_id: falsefor remote/URL packages aligns with the newcreate_url_install_targethelper introduced in apply.rs and provides consistent handling across the codebase.
396-403: Thewith_pkg_id: falsesetting at line 399 differs from lines 576/673 and affects filtering behavior.Packages with
with_pkg_id: falsecan only match in remove/update operations when explicitly named; they don't match in bulk operations. Verify that interactive--showselections should have this stricter matching behavior rather than being treated as package-ID tracked (which would requiretrue). If--showselections should behave like package ID lookups, changewith_pkg_id: falsetotruefor consistency.crates/soar-cli/src/apply.rs (1)
284-302: LGTM - Enhanced install target with configuration fields.The updated
create_install_targetfunction properly propagates configuration fromResolvedPackage, including pinned status, profile, portable paths, and entrypoint. Settingwith_pkg_id: falsealigns with the new URL-based package handling approach.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @crates/soar-cli/src/apply.rs:
- Around line 137-153: The code borrows `installed` as `ref existing` then moves
`installed` into `create_url_install_target`, causing a borrow/move error; fix
by not moving `installed` — either pass `existing.clone()` or pass
`installed.clone()` into `create_url_install_target` (or clone `installed` into
a local before the `if let`), so the borrowed `existing` remains valid; adjust
the `if let Some(ref existing) = installed` branch to call
`create_url_install_target(&url_pkg, pkg, existing.clone())` (or use `let
existing = installed.clone().unwrap();` and then use `existing`) to eliminate
the move.
🧹 Nitpick comments (1)
crates/soar-cli/src/apply.rs (1)
289-328: Nice cleanup with URL target helper; consider deduping the two constructors + double-checkwith_pkg_id: false.
create_install_targetandcreate_url_install_targetmap the sameResolvedPackagefields intoInstallTarget; the only real difference is howpackageis produced. Also, changingwith_pkg_idtofalse(Line 297, 318) may have downstream implications—worth ensuring this matches how apply/install/update expect pkg-id behavior for both metadata and URL packages.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/soar-cli/src/apply.rs
⏰ 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
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.