refactor(cli): use operations from shared crate#158
Conversation
📝 WalkthroughWalkthroughThis PR shifts the CLI from a global AppState to a context-driven SoarContext, centralizes progress/event handling, delegates package operations to soar-operations, consolidates workspace dependency declarations, and removes state.rs and many in-file orchestration helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (main)
participant Ctx as SoarContext
participant Events as EventHandler/Progress
participant Ops as soar-operations
participant DB as DB/Metadata
CLI->>Ctx: create_context()
Ctx->>Events: spawn_event_handler()
CLI->>Ctx: call command(ctx, ...)
CLI->>Ops: delegate operation (e.g., install::resolve/perform)
Ops->>DB: query/resolve metadata
Ops->>Events: emit progress events
Events->>CLI: update progress bars via ProgressGuard
Ops-->>CLI: return Report (Install/Update/Apply/Run)
CLI->>CLI: display_*_report(report)
CLI->>Events: stop/suspend/finish ProgressGuard (on exit)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/soar-cli/src/use.rs (1)
17-44:⚠️ Potential issue | 🟡 MinorDisplay glitch when
pkg_typeisNone: output renders as(-version).When
package.pkg_typeisNone, argument 5 becomes an empty string, and the format string({}-{})produces(-1.0.0)— the leading dash without a type looks unintended.Consider conditionally including the type-version group:
Suggested fix
"[{}] {}#{}:{} ({}{}) ({}){}", idx + 1, Colored(Blue, &package.pkg_name), Colored(Cyan, &package.pkg_id), Colored(Cyan, &package.repo_name), package .pkg_type .as_ref() - .map(|pkg_type| format!(":{}", Colored(Magenta, &pkg_type))) + .map(|pkg_type| format!("{}:", Colored(Magenta, &pkg_type))) .unwrap_or_default(), Colored(Magenta, &package.version),Or restructure to omit the dash entirely when
pkg_typeis absent.crates/soar-cli/src/inspect.rs (1)
205-211:⚠️ Potential issue | 🟡 Minor
{inspect_type}is not interpolated in the prompt string.Line 207 passes a string literal
"The {inspect_type} file is too large..."tointeractive_ask, but this is not aformat!macro —{inspect_type}will appear literally in the output rather than being replaced with"log"or"script".Proposed fix
- let response = interactive_ask( - "The {inspect_type} file is too large. Do you really want to view it (y/N)?", - )?; + let response = interactive_ask( + &format!("The {inspect_type} file is too large. Do you really want to view it (y/N)?"), + )?;
🤖 Fix all issues with AI agents
In `@crates/soar-cli/src/health.rs`:
- Around line 109-111: The log message "Removed all broken packages" is emitted
whenever report.removed is non-empty even if some removals failed; update the
condition around the info! call in the health.rs code handling the report
(checking report.removed and report.failed) so it only logs "Removed all broken
packages" when report.removed is non-empty AND report.failed.is_empty(), and add
an else path that logs or warns about which removals failed (use report.failed)
to accurately reflect partial failures.
In `@crates/soar-cli/src/install.rs`:
- Around line 152-159: The fallback path in install_with_show that calls
install::resolve_packages currently only collects ResolveResult::Resolved and
silently drops NotFound, AlreadyInstalled, and Ambiguous results; update the
loop that handles results from install::resolve_packages(ctx,
std::slice::from_ref(package), options).await to mirror the main
install_packages handling by matching all ResolveResult variants (Resolved,
NotFound, AlreadyInstalled, Ambiguous), pushing targets for Resolved and
emitting the same user-facing messages or warnings for NotFound,
AlreadyInstalled and Ambiguous (using the same logging/print helpers used in
install_packages) so no result is silently ignored and output behavior is
consistent.
In `@crates/soar-cli/src/run.rs`:
- Around line 47-55: The comment about interactive checksum handling is
stale—remove or update it so it no longer claims special checksum handling
around run::execute_binary; instead document that prepare_run handles checksum
failures upstream. Also avoid calling std::process::exit which skips
destructors: replace the direct std::process::exit(run_result.exit_code)
behavior by returning an appropriate exit code from the run function (propagate
run_result.exit_code up to main) so ctx drop and progress_guard.finish() in
main.rs execute; reference run::execute_binary, run_result.exit_code, and remove
the misleading comment lines above the execute_binary call.
In `@crates/soar-cli/src/update.rs`:
- Line 17: The CLI flag is ignored because update_packages declares _no_verify
but doesn't forward it and perform_update constructs InstallOptions with
no_verify: false; fix by renaming _no_verify to no_verify in update_packages and
pass that boolean into the update flow (e.g., into perform_update call), then
update perform_update to accept a no_verify argument (or thread it through its
existing params) and use that value when constructing InstallOptions instead of
hardcoding false; ensure the InstallOptions no_verify field is set from the
propagated no_verify parameter so the CLI flag actually controls checksum
verification.
In `@crates/soar-cli/src/use.rs`:
- Line 38: package.size is an Option<u64> but format_bytes requires u64; update
the call where Colored(Magenta, format_bytes(package.size, 2)) is used to pass a
concrete u64 instead (e.g., package.size.copied().unwrap_or(0) or
package.size.unwrap_or(0)) so format_bytes receives a u64; keep the rest of the
Colored(...) expression unchanged and use the same symbol names (format_bytes
and package.size) when making the fix.
🧹 Nitpick comments (13)
crates/soar-operations/src/install.rs (2)
925-927: Generic error message loses per-package detail within the error string itself.The old message presumably included package name/id in the error text. The new message is generic. While this is fine because the caller (Line 646-657) wraps it into
FailedInfowithpkg_name/pkg_id, and the event at Line 647 carries the same, any code path that only has access to theSoarError(e.g., logging the error independently) will lack package context.Consider embedding the package identity if the error is ever surfaced in isolation:
Suggested improvement
- return Err(SoarError::Custom( - "Invalid checksum, skipped installation.".into(), - )); + return Err(SoarError::Custom(format!( + "{}#{}: Invalid checksum, skipped installation.", + pkg.pkg_name, pkg.pkg_id + )));
682-684:Arc::try_unwrap/into_innerunwraps are safe here but fragile.After all handles are successfully awaited, no other clones remain, so these unwraps won't panic on the happy path. If the early-return on JoinError changes in the future (e.g., collecting all errors instead of returning on first), the
try_unwrapwould panic. A defensive approach:Optional: handle failure gracefully
- let installed = Arc::try_unwrap(installed).unwrap().into_inner().unwrap(); - let failed = Arc::try_unwrap(failed).unwrap().into_inner().unwrap(); - let warnings = Arc::try_unwrap(warnings).unwrap().into_inner().unwrap(); + let installed = Arc::try_unwrap(installed) + .map_err(|_| SoarError::Custom("internal: installed Arc still shared".into()))? + .into_inner() + .unwrap_or_default(); + let failed = Arc::try_unwrap(failed) + .map_err(|_| SoarError::Custom("internal: failed Arc still shared".into()))? + .into_inner() + .unwrap_or_default(); + let warnings = Arc::try_unwrap(warnings) + .map_err(|_| SoarError::Custom("internal: warnings Arc still shared".into()))? + .into_inner() + .unwrap_or_default();crates/soar-cli/src/remove.rs (1)
74-79: Minor formatting inconsistency: success messages use plain text while confirmation usesColored.The confirmation display (Lines 32-38) uses
Colored(Blue, ...),Colored(Cyan, ...), etc., but the success messages at Lines 74-79 use plainformat!with no coloring. Consider aligning for a consistent user experience.crates/soar-cli/src/utils.rs (1)
159-164: Variable shadowing ofsizein closures reduces readability.Both closures shadow the function parameter
sizewith their closure argument. Consider using a distinct name likesorbytesfor the closure parameter to avoid confusion.Suggested clarity improvement
pub fn pretty_package_size(ghcr_size: Option<u64>, size: Option<u64>) -> String { ghcr_size - .map(|size| format!("{}", Colored(Magenta, format_bytes(size, 2)))) - .or_else(|| size.map(|size| format!("{}", Colored(Magenta, format_bytes(size, 2))))) + .map(|s| format!("{}", Colored(Magenta, format_bytes(s, 2)))) + .or_else(|| size.map(|s| format!("{}", Colored(Magenta, format_bytes(s, 2))))) .unwrap_or_default() }crates/soar-cli/src/apply.rs (1)
197-207:total_changesincludesto_remove.len()regardless ofpruneflag.On Line 197,
total_changessums all three categories unconditionally, but on Line 203 the display correctly guardsto_removebehindprune. Sincetotal_changesis only used to decide whether to show the summary line at all (Line 198), this is harmless — but it could cause the summary to print whenprune=falseand only removable packages exist (no installs/updates/in-sync), showing all zeros. Minor cosmetic issue.crates/soar-cli/src/progress.rs (2)
33-47:ProgressGuardlacks aDropimpl — forgettingfinish()silently leaks the handler thread.If a caller drops the guard without calling
finish(), theJoinHandleis dropped (detaching the thread). The thread will continue until the channel disconnects, but won't be joined. For a short-lived CLI this is harmless, but adding aDropimpl would make the contract self-enforcing:Suggested Drop impl
+impl Drop for ProgressGuard { + fn drop(&mut self) { + if let Some(handle) = self.handle.take() { + handle.join().ok(); + } + } +}With this,
finish()becomes a convenience alias for explicit cleanup, and accidental drops still join.
414-421:unreachable!()inSyncStagewildcard arm will panic if a new variant is added.If
SyncStagegains a new variant in the future, this will panic at runtime. Consider logging an unknown stage instead:Safer fallback
- _ => unreachable!(), + other => format!("{repo_name}: {other:?}"),This applies similarly to the
VerifyStagewildcard at Line 275, though that one is inside a branch already restricted toChecksum | Signature, making it truly unreachable.crates/soar-cli/src/inspect.rs (1)
60-62:ProgressGuardis dropped immediately due to_binding.
let (ctx, _) = crate::create_context();— the underscore pattern_drops theProgressGuardimmediately (Rust does not bind_to a variable). Ifprogress_enabled()is true, the event handler thread is spawned and then immediately joined/dropped. This won't cause a functional issue here sinceinspect_loguses direct spinner APIs rather than the event system, but it's worth noting the inconsistency with how other commands receive context frommain.rs.Consider either passing
ctxfrom the caller (as other commands do) or using_guardto keep the guard alive:- let (ctx, _) = crate::create_context(); + let (ctx, _guard) = crate::create_context();crates/soar-cli/src/run.rs (1)
13-13:command[0]will panic ifcommandis empty.If
commandis an empty slice, indexing at[0]will panic. While clap likely guarantees at least one argument, a defensive check would prevent a confusing panic.Proposed defensive access
- let package_name = &command[0]; + let package_name = command.first().ok_or_else(|| { + soar_core::error::SoarError::Custom("No package name provided".into()) + })?;crates/soar-cli/src/download.rs (1)
145-146: Context is created inside a per-link loop iteration — wasteful for multiple non-URL links.
crate::create_context()is called inside thefor link in &linksloop (line 90). If multiple non-URL links are provided, each iteration creates a freshSoarContext(re-reading config, potentially spawning/joining an event handler thread). Consider accepting a&SoarContextas a parameter or hoisting context creation outside the loop.Additionally,
_drops theProgressGuardimmediately (same pattern as ininspect.rs).crates/soar-cli/src/main.rs (1)
289-294:inspect_logandLog/Inspectcommands don't receivectx— inconsistent with other commands.All other commands receive
&ctx, butLogandInspectcreate their own context internally viacrate::create_context(). This means they have a separate event sink and progress guard lifecycle. Consider passing&ctxfor consistency, which would also avoid the_ProgressGuard drop issue noted ininspect.rs.crates/soar-cli/src/install.rs (2)
42-55: Unnecessary.clone()on ownedOption<String>values.The
portable*parameters are ownedOption<String>values. Since they are not used after being passed intoInstallOptions, they can be moved directly instead of cloned. The same applies toname_override,version_override,pkg_type_override, andpkg_id_override— which are already moved without cloning on lines 51–54.Suggested diff
let options = InstallOptions { force, - portable: portable.clone(), - portable_home: portable_home.clone(), - portable_config: portable_config.clone(), - portable_share: portable_share.clone(), - portable_cache: portable_cache.clone(), + portable, + portable_home, + portable_config, + portable_share, + portable_cache, binary_only, no_verify, name_override,
127-135: Unused_yesparameter.The
_yesparameter is accepted but never used ininstall_with_show. In the maininstall_packagespath,yescontrols auto-selection of the first candidate for ambiguous results (line 71). The--showpath always presents an interactive selector, so if--yesis passed alongside--show, the user still gets an interactive prompt (line 236), which may be unexpected.Consider either using
yesto auto-select the first match (consistent with the non-show path) or documenting why--yesis intentionally ignored here.
| .unlinked | ||
| .then(String::new) | ||
| .unwrap_or_else(|| format!(" {}", Colored(Red, "*"))) | ||
| Colored(Magenta, format_bytes(package.size, 2)), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the type of `size` field in the Package struct
ast-grep --pattern 'struct Package {
$$$
}'Repository: pkgforge/soar
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Also search for Package struct definition with rg in case ast-grep doesn't find it
rg 'struct Package' -A 10Repository: pkgforge/soar
Length of output: 11123
🏁 Script executed:
#!/bin/bash
# Find the format_bytes function to check its parameter types
rg 'fn format_bytes' -A 3Repository: pkgforge/soar
Length of output: 331
🏁 Script executed:
#!/bin/bash
# Check the imports and context in use.rs around line 38
head -50 crates/soar-cli/src/use.rsRepository: pkgforge/soar
Length of output: 1641
🏁 Script executed:
#!/bin/bash
# Get the full Package struct from crates/soar-core/src/database/models.rs
sed -n '/^pub struct Package {/,/^}/p' crates/soar-core/src/database/models.rsRepository: pkgforge/soar
Length of output: 1591
🏁 Script executed:
#!/bin/bash
# Double check the exact line 38 context and verify there's no cast or unwrap we missed
sed -n '35,42p' crates/soar-cli/src/use.rsRepository: pkgforge/soar
Length of output: 416
Handle Option<u64> type mismatch: package.size cannot be passed directly to format_bytes.
package.size is Option<u64>, but format_bytes expects u64. This will not compile. Use .unwrap_or(0), .copied().unwrap_or(0), or similar to handle the Option.
🤖 Prompt for AI Agents
In `@crates/soar-cli/src/use.rs` at line 38, package.size is an Option<u64> but
format_bytes requires u64; update the call where Colored(Magenta,
format_bytes(package.size, 2)) is used to pass a concrete u64 instead (e.g.,
package.size.copied().unwrap_or(0) or package.size.unwrap_or(0)) so format_bytes
receives a u64; keep the rest of the Colored(...) expression unchanged and use
the same symbol names (format_bytes and package.size) when making the fix.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/soar-cli/src/install.rs`:
- Around line 78-88: The re-resolution loop only collects
ResolveResult::Resolved and silently drops other variants (notably
ResolveResult::AlreadyInstalled), so when the fully-qualified query resolves to
AlreadyInstalled the package vanishes without feedback; update the loop that
calls install::resolve_packages (the one building install_targets from
re_results for specific_query built from pkg.pkg_name/pkg.pkg_id/pkg.repo_name
and options) to match on all ResolveResult variants: preserve Resolved into
install_targets as before, and explicitly handle AlreadyInstalled (e.g., record
the package into a separate already_installed list or emit a user-facing message
via the same ctx/logger so the user sees that selection is already installed),
and handle/propagate any other variants (Ambiguous/NotFound/Error) appropriately
instead of dropping them silently.
🧹 Nitpick comments (5)
crates/soar-operations/src/update.rs (1)
470-470: Error fromremove_old_versionsis silently discarded.
let _ = remove_old_versions(...)swallows any cleanup failure. If old version removal fails (e.g., permission error, file in use), the user gets no feedback. Consider at least logging a warning.Suggested fix
- let _ = remove_old_versions(pkg, &diesel_db, false); + if let Err(e) = remove_old_versions(pkg, &diesel_db, false) { + warn!( + "Failed to remove old versions of {}#{}: {}", + pkg.pkg_name, pkg.pkg_id, e + ); + }crates/soar-cli/src/health.rs (1)
12-13:display_healthisasyncbut contains no.await.
health::check_health(ctx)is synchronous. Theasyncqualifier adds unnecessary overhead (future state machine). If it's kept for signature consistency across CLI commands, that's fine — just a nit.crates/soar-cli/src/run.rs (1)
40-43: SilentOk(0)on unexpectedPrepareRunResultvariant after re-resolution.If
prepare_runwith a specificrepo_nameandpkg_idunexpectedly returnsAmbiguousagain, this silently returns success. Consider logging a warning or debug message for this "shouldn't happen" case.Suggestion
match result { PrepareRunResult::Ready(path) => path, - _ => return Ok(0), + other => { + tracing::warn!("Unexpected result after re-resolution: {other:?}"); + return Ok(1); + } }crates/soar-cli/src/main.rs (1)
56-56:create_contextispubbut appears to be crate-internal.If this function is not intended for use outside the crate, consider
pub(crate).crates/soar-cli/src/install.rs (1)
65-108: Duplicate resolution-result handling logic acrossinstall_packagesandinstall_with_show.The match on
ResolveResultvariants (lines 65-108) is duplicated nearly verbatim ininstall_with_show(lines 154-197). Consider extracting a shared helper to avoid drift between these two paths.
| // Re-resolve with the specific selected package | ||
| let specific_query = | ||
| format!("{}#{}:{}", pkg.pkg_name, pkg.pkg_id, pkg.repo_name); | ||
| let re_results = | ||
| install::resolve_packages(ctx, &[specific_query], &options).await?; | ||
| for r in re_results { | ||
| if let ResolveResult::Resolved(targets) = r { | ||
| install_targets.extend(targets); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Non-Resolved re-resolution results are silently dropped after ambiguous selection.
After the user picks a specific package from the ambiguous set, re-resolution at lines 81-87 only collects Resolved targets. If the fully-qualified query returns AlreadyInstalled, the user gets no feedback — the package silently disappears from the install set.
This is unlikely for NotFound/Ambiguous (query is fully qualified), but AlreadyInstalled is a realistic scenario (user picks a variant that's already installed without --force).
Suggested improvement
for r in re_results {
- if let ResolveResult::Resolved(targets) = r {
- install_targets.extend(targets);
+ match r {
+ ResolveResult::Resolved(targets) => {
+ install_targets.extend(targets);
+ }
+ ResolveResult::AlreadyInstalled { pkg_name, pkg_id, repo_name, version } => {
+ warn!(
+ "{}#{}:{} ({}) is already installed - skipping",
+ pkg_name, pkg_id, repo_name, version,
+ );
+ if !force {
+ info!("Hint: Use --force to reinstall");
+ }
+ }
+ ResolveResult::NotFound(name) => {
+ error!("Package {} not found", name);
+ }
+ _ => {}
}
}🤖 Prompt for AI Agents
In `@crates/soar-cli/src/install.rs` around lines 78 - 88, The re-resolution loop
only collects ResolveResult::Resolved and silently drops other variants (notably
ResolveResult::AlreadyInstalled), so when the fully-qualified query resolves to
AlreadyInstalled the package vanishes without feedback; update the loop that
calls install::resolve_packages (the one building install_targets from
re_results for specific_query built from pkg.pkg_name/pkg.pkg_id/pkg.repo_name
and options) to match on all ResolveResult variants: preserve Resolved into
install_targets as before, and explicitly handle AlreadyInstalled (e.g., record
the package into a separate already_installed list or emit a user-facing message
via the same ctx/logger so the user sees that selection is already installed),
and handle/propagate any other variants (Ambiguous/NotFound/Error) appropriately
instead of dropping them silently.
1 participant
Summary by CodeRabbit
New Features
Improvements