Skip to content

Allows soar apply to track versioning with URL packages.#129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
QaidVoid merged 4 commits into from
Dec 28, 2025
Merged

Allows soar apply to track versioning with URL packages. #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
96b7793
cli: (wip), aim to fix #128. doesn't clean up previous package
Opposite34 Dec 28, 2025
d3ecc50
cli: partial fix to #128 (works when using prune)
Opposite34 Dec 28, 2025
9671fec
cli: fully fix #128, reverted prune related version checks and unpin/…
Opposite34 Dec 28, 2025
3adb99d
fmt
QaidVoid Dec 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 18 additions & 14 deletions crates/soar-cli/src/apply.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -24,6 +24,7 @@ use tracing::{error, info, warn};
use crate::{
install::{create_install_context, perform_installation},
state::AppState,
update::perform_update,
utils::{display_settings, icon_or, Colored, Icons},
};

Expand DownExpand Up@@ -136,21 +137,24 @@ async fn compute_diff(

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 url_pkg.version != existing_install.unwrap().version {
diff.to_update.push((pkg.clone(), target));
Comment on lines 138 to +157

@coderabbitaicoderabbitaiBotDec 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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));
}elseif 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));
}elseif pkg.version != Some(existing_install.unwrap().version){
diff.to_update.push((pkg.clone(), target));

} else {
diff.in_sync.push(format!("{} (local)", pkg.name));
}
Expand DownExpand Up@@ -507,7 +511,7 @@ async fn execute_apply(state: &AppState, diff: ApplyDiff, no_verify: bool) -> So
no_verify,
);

perform_installation(ctx.clone(), targets, diesel_db.clone(), true).await?;
perform_update(ctx.clone(), targets, diesel_db.clone(), false).await?;
updated_count = ctx.installed_count.load(Ordering::Relaxed) as usize;
failed_count += ctx.failed.load(Ordering::Relaxed) as usize;
}
Expand Down
2 changes: 1 addition & 1 deletion crates/soar-cli/src/install.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -266,7 +266,7 @@ fn resolve_packages(
package: url_pkg.to_package(),
existing_install,
with_pkg_id: url_pkg.pkg_type.is_some(),
pinned: true, // URL packages are always pinned
pinned: false,
profile: None,
..Default::default()
});
Expand Down
2 changes: 1 addition & 1 deletion crates/soar-cli/src/update.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -216,7 +216,7 @@ pub async fn update_packages(
Ok(())
}

async fn perform_update(
pub async fn perform_update(
ctx: InstallContext,
targets: Vec<InstallTarget>,
diesel_db: DieselDatabase,
Expand Down