feat(cli): add system-wide package management #141
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds system-wide package management capability to soar by introducing a global system mode flag, a new Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Main as main.rs
participant Config as soar_config
participant PkgOps as Package Ops
participant Paths as Path Utils
User->>Main: soar --system [args]
Main->>Main: Check system flag (args.system)
alt System flag set
Main->>Main: handle_system_mode()
Main->>Main: Check for root privileges
alt Not root
Main->>Main: Escalate with doas/sudo
Main->>Main: Re-execute process
end
end
Main->>Config: enable_system_mode()
Config->>Config: Set SYSTEM_MODE = true
Config->>Config: Update CONFIG_PATH to /etc/soar
PkgOps->>Config: is_system_mode()
Config-->>PkgOps: Returns true/false
PkgOps->>Paths: desktop_dir(is_system_mode())
Paths->>Paths: Return system or user path
Paths-->>PkgOps: Path based on system mode
PkgOps->>PkgOps: Process packages with system paths
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. 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! |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/soar-core/src/package/remove.rs`:
- Line 12: CI failed rustfmt on the single-line import in remove.rs; reformat
the use statement for soar_utils so it conforms to rustfmt (either run cargo fmt
or expand the import across multiple lines) and ensure the identifiers
FileSystemResult, walk_dir, and the nested path items desktop_dir and icons_dir
are correctly grouped under soar_utils::path; keep the same imported symbols but
adjust spacing/line breaks to satisfy rustfmt.
🧹 Nitpick comments (3)
crates/soar-config/src/config.rs (2)
110-113: Consider documenting panic behavior for poisoned lock.The
.unwrap()onRwLock::read()is acceptable here since lock poisoning typically indicates a panic in another thread, which is a programming error. However, for a public API, consider adding a doc comment noting this behavior or using.expect()with a descriptive message for clearer debugging.♻️ Optional: Add descriptive expect message
pub fn is_system_mode() -> bool { - *SYSTEM_MODE.read().unwrap() + *SYSTEM_MODE.read().expect("SYSTEM_MODE lock poisoned") }
115-123: The ordering is already correct, but add documentation as a safeguard.The code already calls
enable_system_mode()(viahandle_system_mode()at line 126 in main.rs) beforeconfig::init()(line 175), so the hypothetical issue of loading from the user path does not occur in the current implementation. However, the suggested documentation improvement is still valuable to prevent accidental future changes that could introduce this problem.📝 Suggested documentation improvement
+/// Enable system mode and set appropriate paths. +/// +/// **Important**: This must be called before `init()` or `Config::new()` to ensure +/// the system configuration path is used. pub fn enable_system_mode() {crates/soar-cli/src/main.rs (1)
68-76: Improve privilege escalation tool detection.The current detection runs
doas trueandsudo truewhich will prompt for a password just to check availability. If the first tool fails authentication, the user may be prompted again for the second tool.Consider checking for the existence of the executable instead:
♻️ Proposed improvement
- let escalation_cmd = if Command::new("doas").arg("true").status().is_ok() { - "doas" - } else if Command::new("sudo").arg("true").status().is_ok() { - "sudo" - } else { + let escalation_cmd = if which::which("doas").is_ok() { + "doas" + } else if which::which("sudo").is_ok() { + "sudo" + } else { return Err(SoarError::Custom( "System mode requires root privileges. Neither 'doas' nor 'sudo' found.".into(), )); };Alternatively, if you don't want to add the
whichcrate dependency:- let escalation_cmd = if Command::new("doas").arg("true").status().is_ok() { - "doas" - } else if Command::new("sudo").arg("true").status().is_ok() { - "sudo" - } else { + let escalation_cmd = if Command::new("which").arg("doas").status().map(|s| s.success()).unwrap_or(false) { + "doas" + } else if Command::new("which").arg("sudo").status().map(|s| s.success()).unwrap_or(false) { + "sudo" + } else { return Err(SoarError::Custom( "System mode requires root privileges. Neither 'doas' nor 'sudo' found.".into(), )); };
📜 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 (10)
crates/soar-cli/Cargo.tomlcrates/soar-cli/src/cli.rscrates/soar-cli/src/health.rscrates/soar-cli/src/main.rscrates/soar-config/src/config.rscrates/soar-core/src/package/install.rscrates/soar-core/src/package/remove.rscrates/soar-core/src/utils.rscrates/soar-package/src/formats/common.rscrates/soar-utils/src/path.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: QaidVoid
Repo: pkgforge/soar PR: 137
File: crates/soar-cli/src/apply.rs:149-153
Timestamp: 2026-01-09T16:14:21.516Z
Learning: In crates/soar-cli/src/apply.rs, for URL packages, passing non-installed records (is_installed=false) as existing_install is intentional. The query filters by repo_name, pkg_name, and pkg_id, ensuring the record is for the same package. This allows resuming partial installs and preserving settings (portable paths, profile) from previous installs.
📚 Learning: 2026-01-09T16:14:21.516Z
Learnt from: QaidVoid
Repo: pkgforge/soar PR: 137
File: crates/soar-cli/src/apply.rs:149-153
Timestamp: 2026-01-09T16:14:21.516Z
Learning: In crates/soar-cli/src/apply.rs, for URL packages, passing non-installed records (is_installed=false) as existing_install is intentional. The query filters by repo_name, pkg_name, and pkg_id, ensuring the record is for the same package. This allows resuming partial installs and preserving settings (portable paths, profile) from previous installs.
Applied to files:
crates/soar-core/src/package/install.rs
🧬 Code graph analysis (6)
crates/soar-core/src/package/remove.rs (3)
crates/soar-config/src/config.rs (2)
get_config(144-156)is_system_mode(111-113)crates/soar-utils/src/fs.rs (1)
walk_dir(172-206)crates/soar-utils/src/path.rs (2)
desktop_dir(141-147)icons_dir(150-156)
crates/soar-package/src/formats/common.rs (2)
crates/soar-config/src/config.rs (1)
is_system_mode(111-113)crates/soar-utils/src/path.rs (2)
icons_dir(150-156)desktop_dir(141-147)
crates/soar-config/src/config.rs (1)
crates/soar-utils/src/path.rs (1)
xdg_data_home(115-119)
crates/soar-cli/src/main.rs (1)
crates/soar-config/src/config.rs (16)
enable_system_mode(116-123)get_config(144-156)set_current_profile(165-173)Config(140-140)Config(523-523)Config(544-544)Config(553-553)Config(570-570)Config(590-590)Config(619-619)Config(635-635)Config(646-646)Config(659-659)Config(682-682)Config(699-699)Config(710-710)
crates/soar-cli/src/health.rs (2)
crates/soar-config/src/config.rs (2)
get_config(144-156)is_system_mode(111-113)crates/soar-utils/src/path.rs (2)
desktop_dir(141-147)icons_dir(150-156)
crates/soar-core/src/utils.rs (3)
crates/soar-config/src/config.rs (2)
get_config(144-156)is_system_mode(111-113)crates/soar-utils/src/fs.rs (1)
walk_dir(172-206)crates/soar-utils/src/path.rs (2)
desktop_dir(141-147)icons_dir(150-156)
🪛 GitHub Actions: CI
crates/soar-core/src/package/remove.rs
[error] 9-9: cargo fmt --all -- --check failed. Formatting changes are available. Run 'cargo fmt' to fix code style issues.
🔇 Additional comments (15)
crates/soar-cli/Cargo.toml (1)
26-26: LGTM!The
nixcrate addition is appropriate for implementing system-wide package management, as it provides Unix-specific functionality needed for privilege checks and system operations.crates/soar-config/src/config.rs (2)
125-128: LGTM!The
system_root()function returns/usr/lib/soar, which follows Linux FHS conventions for application data. This is appropriate for system-wide package installations.
178-183: LGTM!The branching logic correctly handles both system and user modes while maintaining backward compatibility. The
SOAR_ROOTenvironment variable override works in both modes, providing flexibility for custom deployments.crates/soar-utils/src/path.rs (2)
140-156: LGTM!The path resolution correctly follows Linux conventions:
- User mode uses XDG Base Directory paths (
~/.local/share/...)- System mode uses
/usr/local/share/...which is the appropriate location for locally-installed (non-distro-packaged) system-wide applications per FHSThe implementation is clean and the boolean parameter provides a clear interface.
458-480: LGTM!The tests properly cover both user mode (
system = false) and system mode (system = true) paths, ensuring the branching logic works correctly.crates/soar-package/src/formats/common.rs (3)
16-16: LGTM!The import change correctly adds
is_system_modeto access the global system mode state.
108-108: LGTM!The
icons_dir(is_system_mode())call correctly passes the system mode flag to determine the appropriate icons directory.
188-189: Privilege escalation for system-mode symlink creation is already handled.The
handle_system_mode()function (crates/soar-cli/src/main.rs, lines 59-89) checks for root privileges and re-executes the entire command withsudoordoasif needed, returning an error if neither is available. This is called before any system mode operations proceed (line 126), ensuring the process runs with root privileges by the time the symlink is created at lines 188-189 in common.rs. No action needed.crates/soar-cli/src/cli.rs (1)
58-60: The-Sshort option does not conflict with the sync command alias.The
--systemflag is properly implemented. The-Sshort option and the sync command'svisible_alias = "S"operate in different parsing contexts—flags require the dash prefix (e.g.,soar -S sync), while subcommand aliases don't (e.g.,soar S). Clap handles these separately, so there is no conflict or user confusion risk.Likely an incorrect or invalid review comment.
crates/soar-cli/src/health.rs (1)
4-4: LGTM!The import of
is_system_modeand its usage indesktop_dir()andicons_dir()calls is consistent with the system-mode feature being added across the codebase. The broken symlinks detection will now correctly scan the appropriate directories based on whether system mode is enabled.Also applies to: 137-138
crates/soar-core/src/utils.rs (1)
8-8: LGTM!The
remove_broken_symlinksfunction now correctly usesis_system_mode()to determine which directories to scan, ensuring consistency with the system-mode feature across the codebase.Also applies to: 85-86
crates/soar-core/src/package/remove.rs (1)
164-175: LGTM!The package removal logic now correctly uses
is_system_mode()to determine the appropriate desktop and icons directories for cleanup. The log message improvement ("removing icon symlink") also adds clarity.crates/soar-core/src/package/install.rs (1)
13-13: LGTM!The installation recording logic now correctly uses
is_system_mode()when cleaning up symlinks from alternate packages. This ensures proper directory targeting based on the installation context.Also applies to: 837-837, 847-847
crates/soar-cli/src/main.rs (2)
125-127: Consider order of operations: system mode vs config path.The
--systemflag handling (lines 125-127) occurs before custom--configprocessing (lines 129-142). However,enable_system_mode()setsCONFIG_PATHto/etc/soar/config.toml. If a user specifies both--systemand--config, the custom config path will correctly override the system default due to the ordering.If the intent is for
--systemto always use/etc/soar/config.tomlregardless of--config, consider adding a note or adjusting the logic. The current behavior allows--configto override, which may or may not be desired.
59-89: Overall system mode handling looks good.The privilege escalation logic correctly:
- Checks if already running as root before attempting escalation
- Re-executes the entire command with the escalation tool
- Exits with the appropriate status code after re-execution
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
1 participant
Summary by CodeRabbit
--system(-S) flag to manage system-wide packages✏️ Tip: You can customize this high-level summary in your review settings.