feat(cli): install packages from soar:// links - #197
Merged
Conversation
Deploying soar-docs with
|
| Latest commit: |
92b71e3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4d45c1c1.soar-docs.pages.dev |
| Branch Preview URL: | https://url-handler.soar-docs.pages.dev |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe CLI adds a ChangesSoar URL handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant SoarCLI
participant url_handler
participant Terminal
participant PackageInstallation
Browser->>SoarCLI: Launch soar:// install link
SoarCLI->>url_handler: handle(context, url, register_only)
url_handler->>url_handler: parse(url)
url_handler->>Terminal: Relaunch browser request
Terminal->>SoarCLI: Start soar in terminal
url_handler->>PackageInstallation: Install confirmed package
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
crates/soar-cli/src/cli.rs (1)
498-508: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
arg_required_else_helpfor theurlsubcommand.
soar urlwith no argument parses successfully. The failure then comes fromurl_handler::handleas a runtime error. Sibling subcommands such asinstallandsearchuse#[command(arg_required_else_help = true)]and print help instead. This keeps the CLI behavior consistent.Note that clap
conflicts_with = "register"is symmetric, so the mutual exclusion is already enforced in both directions.♻️ Proposed change
/// Act on a soar:// link, or register soar as its handler + #[command(arg_required_else_help = true)] #[clap(name = "url")] Url {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/soar-cli/src/cli.rs` around lines 498 - 508, Update the Url subcommand declaration to enable arg_required_else_help, so invoking soar url without a URL or registration option displays clap help instead of reaching url_handler::handle; keep the existing URL/register conflict configuration unchanged.crates/soar-cli/src/url_handler.rs (4)
180-190: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
whichaccepts a non-executable file.
whichselects the first PATH entry wherecandidate.is_file()is true. It does not check the executable bit. The same applies to thefs::metadata(name).is_ok()branch for a path that contains/. A non-executable file that shares a terminal name shadows a real terminal later on PATH, and the spawn then fails.♻️ Proposed change
+use std::os::unix::fs::PermissionsExt; + +fn is_executable(path: &std::path::Path) -> bool { + fs::metadata(path) + .map(|m| m.is_file() && m.permissions().mode() & 0o111 != 0) + .unwrap_or(false) +} + fn which(name: &str) -> Option<String> { if name.contains('/') { - return fs::metadata(name).is_ok().then(|| name.to_string()); + return is_executable(std::path::Path::new(name)).then(|| name.to_string()); } env::var_os("PATH").and_then(|paths| { env::split_paths(&paths) .map(|dir| dir.join(name)) - .find(|candidate| candidate.is_file()) + .find(|candidate| is_executable(candidate)) .map(|candidate| candidate.to_string_lossy().into_owned()) }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/soar-cli/src/url_handler.rs` around lines 180 - 190, Update which to require executability in both branches: for names containing “/”, validate the resolved path is an executable file rather than merely existing; for PATH candidates, replace the is_file check with an executable-file check. Preserve the existing first-match and Option<String> behavior.
307-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the non-parse helpers.
The parse tests are thorough. The untested code is where the remaining risk sits.
Please add tests for:
desktop_entrywith an executable path that contains%, a space, and". This covers the escaping issue raised on lines 129 to 150.validate_specboundary cases, for example a spec that ends with.or-, and a spec with an empty version after@.find_terminalwithTERMINALset to an absolute path and to an unknown name.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/soar-cli/src/url_handler.rs` around lines 307 - 366, Add focused tests in the existing tests module for desktop_entry escaping executable paths containing %, spaces, and quotes; validate_spec rejecting specs ending in . or - and specs with an empty version after @; and find_terminal handling TERMINAL as both an absolute path and an unknown name. Assert the expected escaped output or validation/lookup results while isolating environment-variable changes between tests.
293-299: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
wait_for_close()?can mask the install error.Line 298 uses
?. Ifinteractive_askfails, for example when stdin reaches EOF,handlereturns the stdin error and dropsresult. The caller then reports a stdin failure instead of the install failure.♻️ Proposed change
if let Err(ref err) = result { info!("{err}"); } - wait_for_close()?; + let _ = wait_for_close(); result🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/soar-cli/src/url_handler.rs` around lines 293 - 299, Update the result-handling flow in handle so wait_for_close() cannot replace an existing interactive_ask install error: preserve and return result when it is already Err, while still propagating wait_for_close() failures when the operation succeeded.
194-217: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider a case-insensitive scheme check and a wait on the spawned terminal.
Two points on the relaunch path.
parseat lines 76 to 77 accepts only the exact stringssoar://andSOAR://. RFC 3986 defines the scheme as case-insensitive. Most browsers normalize the scheme to lowercase, so the risk is low. A mixed-case form such asSoar://install/ripgrepis still rejected. Useurl.get(..7).is_some_and(|p| p.eq_ignore_ascii_case("soar://"))instead.spawnreturns immediately andhandlethen returnsOk(()). The parent exits while the terminal starts. That is the intended behavior for a browser launch. Confirm that noSoarContextlock or SQLite handle held by the parent is still needed by the child, because the child runs the real install.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/soar-cli/src/url_handler.rs` around lines 194 - 217, Update the URL scheme validation in parse to accept any casing of the seven-character “soar://” prefix using a case-insensitive comparison. Keep relaunch_in_terminal’s non-blocking spawn behavior for browser launches, and verify the parent does not retain any SoarContext lock or SQLite handle required by the child process.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/soar-cli/src/url_handler.rs`:
- Around line 258-269: Update the warning block in the URL-handling flow before
interactive_ask to use the same direct stdout output path as the prompt, rather
than tracing::info!, so it remains visible regardless of log-level, quiet, or
JSON filtering. Preserve the existing warning text and confirmation behavior.
- Around line 129-150: Update desktop_entry to encode exe according to
desktop-entry Exec rules: escape every literal % as %% and quote paths
containing any reserved Exec character, escaping embedded ", `, $, and \ with
the required desktop-entry string layer. Ensure literal backslashes and dollar
signs produce the specified file-level escaping, and add unit tests covering %,
", \, $, and another reserved character.
---
Nitpick comments:
In `@crates/soar-cli/src/cli.rs`:
- Around line 498-508: Update the Url subcommand declaration to enable
arg_required_else_help, so invoking soar url without a URL or registration
option displays clap help instead of reaching url_handler::handle; keep the
existing URL/register conflict configuration unchanged.
In `@crates/soar-cli/src/url_handler.rs`:
- Around line 180-190: Update which to require executability in both branches:
for names containing “/”, validate the resolved path is an executable file
rather than merely existing; for PATH candidates, replace the is_file check with
an executable-file check. Preserve the existing first-match and Option<String>
behavior.
- Around line 307-366: Add focused tests in the existing tests module for
desktop_entry escaping executable paths containing %, spaces, and quotes;
validate_spec rejecting specs ending in . or - and specs with an empty version
after @; and find_terminal handling TERMINAL as both an absolute path and an
unknown name. Assert the expected escaped output or validation/lookup results
while isolating environment-variable changes between tests.
- Around line 293-299: Update the result-handling flow in handle so
wait_for_close() cannot replace an existing interactive_ask install error:
preserve and return result when it is already Err, while still propagating
wait_for_close() failures when the operation succeeded.
- Around line 194-217: Update the URL scheme validation in parse to accept any
casing of the seven-character “soar://” prefix using a case-insensitive
comparison. Keep relaunch_in_terminal’s non-blocking spawn behavior for browser
launches, and verify the parent does not retain any SoarContext lock or SQLite
handle required by the child process.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 70f3f5b6-2643-4c33-9f6f-474bc4138ebc
📒 Files selected for processing (3)
crates/soar-cli/src/cli.rscrates/soar-cli/src/main.rscrates/soar-cli/src/url_handler.rs
Open
github-actions Bot
pushed a commit
to Azathothas/soar
that referenced
this pull request
Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
soar://links.soar://links.