feat(crate): init soar-dl crate #102
Conversation
|
Warning Rate limit exceeded@QaidVoid has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 14 seconds before requesting another review. ⌛ 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 (4)
WalkthroughAdds a new workspace crate Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant RD as ReleaseDownload
participant P as Platform
participant H as Http Client
participant D as Download
participant FS as Filesystem
U->>RD: new(project).tag().filter().execute()
RD->>P: fetch_releases(project, tag)
P->>H: GET API endpoints
H-->>P: JSON releases
P-->>RD: Vec<Release>
RD->>RD: select release, filter assets
loop per asset
RD->>D: Download::new(url).output(...).execute()
D->>H: HEAD (etag/length)
H-->>D: headers
D->>FS: read_resume (xattr)
FS-->>D: ResumeInfo?
D->>H: GET (Range if resuming)
H-->>D: stream chunks
D->>FS: write file, update resume xattr
D->>FS: post-process (chmod/extract), remove resume xattr
D-->>RD: PathBuf
end
RD-->>U: Vec<PathBuf>
sequenceDiagram
participant U as User
participant O as OciDownload
participant H as Http Client
participant R as Registry
participant FS as Filesystem
U->>O: new(reference).filter().parallel(n).execute()
O->>H: GET manifest
H->>R: request manifest
R-->>H: manifest JSON
H-->>O: manifest
O->>O: parse layers, apply filter
alt parallel
par Layer A
O->>H: GET blob A (Range/etag resume)
H-->>FS: write blob A (progress)
and Layer B
O->>H: GET blob B
H-->>FS: write blob B (progress)
end
else sequential
loop layers
O->>H: GET blob
H-->>FS: write blob (progress)
end
end
O->>FS: set ELF perms / extract
FS-->>O: results
O-->>U: Vec<PathBuf>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 13
🧹 Nitpick comments (1)
crates/soar-utils/src/string.rs (1)
5-29: Consider adding examples to the documentation.The URI decoding implementation is correct and handles edge cases appropriately with
from_utf8_lossy. However, adding usage examples would improve clarity.Consider adding doc examples:
/// https://users.rust-lang.org/t/encode-decode-uri/90017/16 /// Decode URI-encoded string +/// +/// # Examples +/// ``` +/// use soar_utils::string::decode_uri; +/// +/// let encoded = "hello%20world%21"; +/// assert_eq!(decode_uri(encoded), "hello world!"); +/// ``` pub fn decode_uri(s: &str) -> String {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.toml(1 hunks)crates/soar-dl/Cargo.toml(1 hunks)crates/soar-dl/src/download.rs(1 hunks)crates/soar-dl/src/error.rs(1 hunks)crates/soar-dl/src/filter.rs(1 hunks)crates/soar-dl/src/github.rs(1 hunks)crates/soar-dl/src/gitlab.rs(1 hunks)crates/soar-dl/src/http.rs(1 hunks)crates/soar-dl/src/http_client.rs(1 hunks)crates/soar-dl/src/lib.rs(1 hunks)crates/soar-dl/src/oci.rs(1 hunks)crates/soar-dl/src/platform.rs(1 hunks)crates/soar-dl/src/release.rs(1 hunks)crates/soar-dl/src/traits.rs(1 hunks)crates/soar-dl/src/types.rs(1 hunks)crates/soar-dl/src/utils.rs(1 hunks)crates/soar-dl/src/xattr.rs(1 hunks)crates/soar-utils/Cargo.toml(1 hunks)crates/soar-utils/src/fs.rs(1 hunks)crates/soar-utils/src/lib.rs(1 hunks)crates/soar-utils/src/string.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
crates/soar-dl/src/utils.rs (4)
crates/soar-dl/src/github.rs (1)
url(85-87)crates/soar-dl/src/gitlab.rs (1)
url(92-94)crates/soar-dl/src/traits.rs (1)
url(6-6)crates/soar-dl/src/download.rs (1)
output(46-49)
crates/soar-dl/src/release.rs (4)
crates/soar-dl/src/download.rs (7)
new(35-44)output(46-49)overwrite(51-54)extract(56-59)extract_to(61-64)progress(66-72)execute(74-133)crates/soar-dl/src/github.rs (3)
tag(59-61)fetch_releases(34-49)assets(71-73)crates/soar-dl/src/gitlab.rs (3)
tag(66-68)fetch_releases(38-56)assets(78-80)crates/soar-dl/src/traits.rs (3)
tag(13-13)fetch_releases(26-29)assets(16-16)
crates/soar-dl/src/traits.rs (2)
crates/soar-dl/src/github.rs (9)
name(55-57)name(77-79)size(81-83)url(85-87)tag(59-61)is_prerelease(63-65)published_at(67-69)assets(71-73)fetch_releases(34-49)crates/soar-dl/src/gitlab.rs (9)
name(62-64)name(84-86)size(88-90)url(92-94)tag(66-68)is_prerelease(70-72)published_at(74-76)assets(78-80)fetch_releases(38-56)
crates/soar-dl/src/download.rs (4)
crates/soar-utils/src/fs.rs (1)
is_elf(275-284)crates/soar-dl/src/utils.rs (3)
filename_from_header(19-26)filename_from_url(9-16)resolve_output_path(29-60)crates/soar-dl/src/xattr.rs (3)
read_resume(7-12)remove_resume(18-20)write_resume(14-16)crates/soar-dl/src/http.rs (1)
fetch(8-23)
crates/soar-dl/src/platform.rs (4)
crates/soar-utils/src/string.rs (1)
decode_uri(11-29)crates/soar-dl/src/github.rs (3)
url(85-87)tag(59-61)fetch_with_fallback(43-43)crates/soar-dl/src/gitlab.rs (3)
url(92-94)tag(66-68)fetch_with_fallback(50-50)crates/soar-dl/src/http.rs (1)
json(25-32)
crates/soar-dl/src/http.rs (1)
crates/soar-dl/src/error.rs (1)
from(63-65)
crates/soar-dl/src/github.rs (2)
crates/soar-dl/src/platform.rs (1)
fetch_with_fallback(98-152)crates/soar-dl/src/traits.rs (9)
fetch_releases(26-29)tag(13-13)name(4-4)name(12-12)is_prerelease(14-14)published_at(15-15)assets(16-16)size(5-5)url(6-6)
crates/soar-dl/src/lib.rs (4)
soar-cli/src/download.rs (1)
download(46-69)crates/soar-dl/src/oci.rs (1)
filter(126-129)crates/soar-dl/src/release.rs (1)
filter(43-46)crates/soar-utils/src/system.rs (1)
platform(9-16)
crates/soar-dl/src/gitlab.rs (3)
crates/soar-dl/src/github.rs (10)
fetch_with_fallback(43-43)fetch_releases(34-49)tag(59-61)name(55-57)name(77-79)is_prerelease(63-65)published_at(67-69)assets(71-73)size(81-83)url(85-87)crates/soar-dl/src/platform.rs (1)
fetch_with_fallback(98-152)crates/soar-dl/src/traits.rs (9)
fetch_releases(26-29)tag(13-13)name(4-4)name(12-12)is_prerelease(14-14)published_at(15-15)assets(16-16)size(5-5)url(6-6)
crates/soar-dl/src/filter.rs (3)
crates/soar-dl/src/github.rs (2)
name(55-57)name(77-79)crates/soar-dl/src/gitlab.rs (2)
name(62-64)name(84-86)crates/soar-dl/src/traits.rs (2)
name(4-4)name(12-12)
crates/soar-dl/src/oci.rs (3)
crates/soar-utils/src/fs.rs (1)
is_elf(275-284)crates/soar-dl/src/download.rs (3)
extract_archive(250-252)output(46-49)extract(56-59)crates/soar-dl/src/xattr.rs (3)
read_resume(7-12)remove_resume(18-20)write_resume(14-16)
⏰ 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 (8)
crates/soar-utils/src/lib.rs (1)
7-7: LGTM!The module export is correctly added to expose the new string utilities.
crates/soar-utils/Cargo.toml (1)
16-16: LGTM!The regex dependency is correctly added as a workspace dependency to support URI decoding in the string module.
crates/soar-utils/src/fs.rs (1)
292-292: LGTM!Test imports are correctly updated.
crates/soar-dl/Cargo.toml (1)
1-22: LGTM!The crate manifest is well-structured with appropriate workspace configuration and dependencies for download functionality.
crates/soar-dl/src/types.rs (1)
1-26: LGTM!The type definitions are clean and well-designed with appropriate derives for their use cases. Progress events, overwrite modes, and resume information are clearly modeled.
crates/soar-dl/src/xattr.rs (1)
1-20: LGTM!The extended attribute helpers correctly implement resume state persistence using JSON serialization and appropriate error handling.
crates/soar-dl/src/utils.rs (2)
9-16: LGTM!The URL filename extraction correctly uses URL parsing and path segments.
29-60: Review the path resolution logic for edge cases.The logic handles multiple scenarios but consider these edge cases:
- Line 36:
p.ends_with('/')treats string-ending slash as directory intent, but line 44 checkspath.is_dir()which requires the path to already exist. This could lead to inconsistent behavior if a non-existent path without trailing slash is intended as a directory.- The function relies on filesystem checks (
path.is_dir()) which couples behavior to runtime filesystem state.Ensure this matches the intended UX, particularly for non-existent paths.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Rabindra Dhakal <qaidvoid@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Rabindra Dhakal <qaidvoid@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Rabindra Dhakal <qaidvoid@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Rabindra Dhakal <qaidvoid@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/soar-dl/src/utils.rs (2)
44-48: Consider deferring directory check to avoid TOCTOU.The filesystem check
path.is_dir()at line 44 creates a time-of-check-time-of-use (TOCTOU) race condition. The directory status could change between this check and the actual file write. Additionally, if the directory doesn't exist yet (but will be created), this check returns false, treating it as a file path.Consider one of these approaches:
- Always use the trailing
/convention to indicate directories (simpler, more explicit)- Or attempt to write directly and handle errors, avoiding the pre-check
36-48: Inconsistent directory detection logic.The function uses two different methods to detect directories:
- Lines 36-41: String-based check (
p.ends_with('/'))- Lines 44-48: Filesystem-based check (
path.is_dir())This means
/tmp/is always treated as a directory (string check), while/tmpdepends on the current filesystem state. This inconsistency could be confusing for users.For better consistency and predictability, consider standardizing on the trailing
/convention throughout, or document the behavior clearly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/soar-dl/src/utils.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/soar-dl/src/utils.rs (7)
crates/soar-dl/src/github.rs (1)
url(85-87)crates/soar-dl/src/gitlab.rs (1)
url(92-94)crates/soar-dl/src/traits.rs (1)
url(6-6)crates/soar-dl/src/platform.rs (1)
parse(45-83)crates/soar-dl/src/oci.rs (1)
output(131-134)crates/soar-dl/src/release.rs (1)
output(48-51)crates/soar-dl/src/download.rs (1)
output(46-49)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/soar-dl/src/platform.rs (1)
148-154: Clarify fallback-first ordering or consider reversing.The function tries
fallbackfirst (without token), thenprimary(with token) on errors. This is atypical—"primary" usually implies the preferred source. If this ordering is intentional (e.g., fallback is a free/public endpoint to reduce rate limits), consider renaming parameters or adding a doc comment to clarify the design.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Cargo.toml(1 hunks)crates/soar-dl/src/platform.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/soar-dl/src/platform.rs (5)
crates/soar-utils/src/string.rs (1)
decode_uri(11-29)crates/soar-dl/src/gitlab.rs (3)
url(92-94)tag(66-68)fetch_with_fallback(50-50)crates/soar-dl/src/github.rs (3)
url(85-87)tag(59-61)fetch_with_fallback(43-43)crates/soar-dl/src/oci.rs (1)
new(107-119)crates/soar-dl/src/http.rs (1)
json(25-32)
🪛 GitHub Actions: CI
crates/soar-dl/src/platform.rs
[error] 1-1: cargo fmt --check reported formatting differences in this file. Run 'cargo fmt' to format.
Cargo.toml
[error] 1-1: Process completed with exit code 1. Command failed: /home/runner/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/cargo test --tests --manifest-path /home/runner/work/soar/soar/Cargo.toml --target-dir /home/runner/work/soar/soar/target/llvm-cov-target --locked --all-features --workspace
🔇 Additional comments (11)
Cargo.toml (4)
1-9: Workspace configuration looks good.Resolver v2, new workspace member, and metadata updates are correctly integrated.
48-49: Path dependency fix applied correctly.The
soar-dlworkspace dependency now correctly points to the local crate path instead of the published crate version. This resolves the prior review concern and ensures workspace members build against the new implementation.
57-62: Release profile optimizations are sensible.The aggressive optimizations (
opt-level=3,lto=true,strip=true,panic="abort",codegen-units=1) are appropriate for a production CLI/binary project. These settings improve performance and reduce binary size without impacting test/debug builds.
20-55: Workspace dependencies properly declared; soar-dl lacks test modules—verify compilation and test execution in your environment.Dependency verification shows all workspace dependencies in Cargo.toml are correctly declared and actively used in both crates: soar-dl imports ureq, url, xattr, thiserror, miette, and fast-glob for download/archive functionality; soar-utils imports blake3 and nix. Module structure is sound with proper exports. However, soar-dl contains zero test modules while soar-utils has comprehensive test coverage across 8 files. This asymmetry may contribute to pipeline test failures. Since sandbox environment prevents cargo build/test execution, manually verify compilation and test runs locally to confirm the root cause of the exit code 1 failure.
crates/soar-dl/src/platform.rs (7)
15-31: LGTM!The enum structure is well-designed with appropriate variants for different platform types.
33-42: LGTM!The regex patterns and LazyLock usage are appropriate for matching GitHub and GitLab URLs.
64-76: LGTM!The GitLab API path check now correctly uses
"api/"prefix to avoid misclassifying legitimate repositories like "api-team/project".
88-98: LGTM!The helper correctly extracts and normalizes project and tag information from regex captures, including proper URL decoding.
135-145: LGTM!The JSON normalization correctly handles both single-object and array responses by converting them to a uniform
Vec<T>format.
157-160: LGTM!The helper correctly identifies retriable HTTP statuses (rate limit, auth, and server errors) that warrant a fallback attempt.
48-55: The review comment is based on incorrect analysis.The code correctly handles the OCI reference format. When the string containing
"ghcr.io/"is passed toOciDownload::new(), it converts toOciReferenceviaFrom<&str>::from()(oci.rs line 30), which explicitly strips the"ghcr.io/"prefix at line 32 before parsing the package and tag. The resultingOciReferencefields contain the correct values without the registry host in the package field.Likely an incorrect or invalid review comment.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @QaidVoid. * #102 (comment) The following files were modified: * `crates/soar-dl/src/download.rs` * `crates/soar-dl/src/error.rs` * `crates/soar-dl/src/filter.rs` * `crates/soar-dl/src/github.rs` * `crates/soar-dl/src/gitlab.rs` * `crates/soar-dl/src/http.rs` * `crates/soar-dl/src/http_client.rs` * `crates/soar-dl/src/oci.rs` * `crates/soar-dl/src/platform.rs` * `crates/soar-dl/src/release.rs` * `crates/soar-dl/src/utils.rs` * `crates/soar-dl/src/xattr.rs` * `crates/soar-utils/src/fs.rs` * `crates/soar-utils/src/string.rs`
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
crates/soar-dl/src/download.rs (1)
119-119: Ignore missing resume xattrs instead of aborting.When the download size never hit a MiB boundary (common for small files), no resume metadata is written.
remove_resumethen returnsErrorKind::NotFound, causing the whole download to fail on cleanup.Apply this diff:
- remove_resume(&output_path)?; + if let Err(err) = remove_resume(&output_path) { + if err.kind() != std::io::ErrorKind::NotFound { + return Err(err.into()); + } + }crates/soar-dl/src/oci.rs (3)
338-348: Remove the hard-coded Authorization header.Every manifest request sends
Authorization: Bearer QQ==, which is not a valid token. Registries (including ghcr.io) will respond with 401/403 even for public blobs when this bogus header is present, preventing the downloader from fetching manifests.Apply this diff:
let mut resp = SHARED_AGENT .get(&url) .header( ACCEPT, "application/vnd.docker.distribution.manifest.v2+json, \ application/vnd.docker.distribution.manifest.list.v2+json, \ application/vnd.oci.image.manifest.v1+json, \ application/vnd.oci.image.index.v1+json", ) - .header(AUTHORIZATION, "Bearer QQ==") .call()?;
434-441: Remove the hard-coded Authorization header from blob requests.The blob download path also includes the bogus
Authorization: Bearer QQ==header, which will cause registries to reject requests. Remove this header and rely on the shared client configuration for authentication when credentials are available.Apply this diff:
- let mut req = SHARED_AGENT.get(&url).header(AUTHORIZATION, "Bearer QQ=="); + let mut req = SHARED_AGENT.get(&url);
506-507: Handle missing resume metadata gracefully.
remove_resume(path)?will error withErrorKind::NotFoundwhen no xattr was written, which is the common case for blobs smaller than 1 MiB (line 481 only writes on 1 MiB boundaries). This turns a successful download into a failure.Apply this diff:
- remove_resume(path)?; + if let Err(err) = remove_resume(path) { + if err.kind() != std::io::ErrorKind::NotFound { + return Err(err.into()); + } + }
🧹 Nitpick comments (2)
crates/soar-dl/src/http.rs (1)
29-36: Consider preserving JSON deserialization error details.Mapping all JSON read errors to
InvalidResponseloses error context that could help with debugging. Consider capturing the underlying error message.You could preserve more error context like this:
pub fn json<T: serde::de::DeserializeOwned>(url: &str) -> Result<T, DownloadError> { SHARED_AGENT .get(url) .call()? .body_mut() .read_json() - .map_err(|_| DownloadError::InvalidResponse) + .map_err(|e| { + eprintln!("JSON parse error: {}", e); + DownloadError::InvalidResponse + }) }crates/soar-dl/src/filter.rs (1)
24-57: Consider optimizing case-sensitive path.The keyword matching logic is correct. However, lines 30 and 44 call
to_string()unnecessarily whencase_sensitiveistrue, since bothnameandpartare already&str. While the performance impact is minor, you could avoid the allocation by restructuring to skip conversion in the case-sensitive branch or usingCow<str>.Example optimization:
fn matches_keywords(&self, name: &str, keywords: &[String], must_match: bool) -> bool { if keywords.is_empty() { return true; } - let haystack = if self.case_sensitive { - name.to_string() - } else { - name.to_lowercase() - }; - keywords.iter().all(|kw| { let parts: Vec<_> = kw .split(',') .map(str::trim) .filter(|s| !s.is_empty()) .collect(); let any_match = parts.iter().any(|&part| { - let needle = if self.case_sensitive { - part.to_string() + if self.case_sensitive { + name.contains(part) } else { - part.to_lowercase() - }; - haystack.contains(&needle) + name.to_lowercase().contains(&part.to_lowercase()) + } }); if must_match { any_match } else { !any_match } }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Cargo.toml(1 hunks)crates/soar-dl/Cargo.toml(1 hunks)crates/soar-dl/src/download.rs(1 hunks)crates/soar-dl/src/error.rs(1 hunks)crates/soar-dl/src/filter.rs(1 hunks)crates/soar-dl/src/github.rs(1 hunks)crates/soar-dl/src/http.rs(1 hunks)crates/soar-dl/src/http_client.rs(1 hunks)crates/soar-dl/src/oci.rs(1 hunks)crates/soar-dl/src/platform.rs(1 hunks)crates/soar-dl/src/release.rs(1 hunks)crates/soar-dl/src/utils.rs(1 hunks)crates/soar-utils/src/bytes.rs(1 hunks)crates/soar-utils/src/error.rs(12 hunks)crates/soar-utils/src/fs.rs(1 hunks)crates/soar-utils/src/hash.rs(2 hunks)crates/soar-utils/src/path.rs(3 hunks)crates/soar-utils/src/system.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/soar-utils/src/bytes.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/soar-dl/Cargo.toml
- crates/soar-dl/src/release.rs
🧰 Additional context used
🧬 Code graph analysis (6)
crates/soar-dl/src/github.rs (2)
crates/soar-dl/src/platform.rs (1)
fetch_with_fallback(107-161)crates/soar-dl/src/traits.rs (9)
fetch_releases(26-29)tag(13-13)name(4-4)name(12-12)is_prerelease(14-14)published_at(15-15)assets(16-16)size(5-5)url(6-6)
crates/soar-dl/src/http.rs (2)
crates/soar-dl/src/http_client.rs (1)
head(68-76)crates/soar-dl/src/error.rs (1)
from(67-69)
crates/soar-dl/src/platform.rs (4)
crates/soar-dl/src/github.rs (4)
url(41-41)url(92-94)tag(66-68)fetch_with_fallback(50-50)crates/soar-dl/src/gitlab.rs (3)
url(92-94)tag(66-68)fetch_with_fallback(50-50)crates/soar-dl/src/http_client.rs (1)
new(64-66)crates/soar-dl/src/http.rs (1)
json(29-36)
crates/soar-dl/src/http_client.rs (1)
crates/soar-dl/src/http.rs (1)
head(8-10)
crates/soar-dl/src/oci.rs (3)
crates/soar-utils/src/fs.rs (1)
is_elf(275-280)crates/soar-dl/src/xattr.rs (3)
read_resume(7-12)remove_resume(18-20)write_resume(14-16)crates/soar-dl/src/download.rs (5)
output(46-49)overwrite(51-54)extract(56-59)extract_to(61-64)execute(74-133)
crates/soar-dl/src/download.rs (4)
crates/soar-utils/src/fs.rs (1)
is_elf(275-280)crates/soar-dl/src/utils.rs (3)
filename_from_header(25-38)filename_from_url(10-22)resolve_output_path(41-72)crates/soar-dl/src/xattr.rs (3)
read_resume(7-12)remove_resume(18-20)write_resume(14-16)crates/soar-dl/src/http.rs (2)
head(8-10)fetch(12-27)
⏰ 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 (26)
crates/soar-utils/src/system.rs (1)
66-68: LGTM!The
uid_namemethod implementation correctly completes theUsernameSourcetrait for the test mock, returningNoneto simulate unavailable username sources as expected.Cargo.toml (3)
1-9: Workspace configuration correctly initialized for new soar-dl crate.Resolver v2 is the modern, deterministic standard for workspaces. The new workspace member addition is properly positioned.
20-57: Workspace dependencies properly configured; previous issue resolved.The critical concern from the prior review—soar-dl pointing to the published crate instead of the local path—has been correctly addressed (line 50 now uses
path = "crates/soar-dl"). The new dependencies align with a download manager's needs (archive extraction, HTTP, URL handling, error formatting). Feature selections are disciplined:regexandreqwestexplicitly disable defaults and cherry-pick features, reducing unnecessary transitive bloat. This is idiomatic Rust practice.
59-64: Release profile configuration is well-suited for a CLI binary tool.The aggressive optimization settings (opt-level=3, lto=true, codegen-units=1, strip=true, panic=abort) are standard and appropriate for release builds of command-line tools. The trade-off of slower build times for smaller, faster binaries and predictable error handling is justified for a package downloader.
crates/soar-utils/src/fs.rs (1)
274-280: LGTM! Past review comment addressed.The
is_elffunction correctly implements the suggestion from the previous review by usingread_file_signatureto read only the first 4 bytes instead of reading the entire file. This is efficient for large binaries.crates/soar-utils/src/error.rs (1)
11-17: LGTM! Consistent formatting refactor.The refactoring to struct-like multi-field patterns with trailing commas is consistent throughout all error enums and their implementations. This improves maintainability without changing any functionality.
Also applies to: 32-39, 45-49, 69-95, 173-262, 269-298, 325-328, 384-446, 551-553
crates/soar-utils/src/path.rs (1)
56-60: LGTM! Formatting consistency.The block-style error mapping aligns with the formatting changes in the error module and improves readability.
Also applies to: 217-222, 235-235
crates/soar-utils/src/hash.rs (1)
34-39: LGTM! Formatting consistency.The error mapping style matches the project-wide formatting updates.
Also applies to: 78-81
crates/soar-dl/src/http_client.rs (4)
12-43: LGTM! Well-designed client configuration.The
ClientConfigprovides a clean API for configuring the HTTP client with reasonable defaults. Thebuild()method correctly applies all configuration options to create theureq::Agent.
50-58: LGTM! Thread-safe shared state pattern.Using
LazyLockwithArc<RwLock<SharedClient>>provides efficient, thread-safe access to the shared HTTP client. The lazy initialization ensures the client is created only when first accessed.
63-117: LGTM! Efficient lock management.Each HTTP method acquires a read lock only to construct the
RequestBuilderand apply headers, then releases it before returning. This design allows concurrent request construction while protecting the shared configuration.
130-140: LGTM! Safe configuration updates.The write lock ensures exclusive access during configuration updates. Rebuilding the agent after configuration changes ensures consistency between the config and agent state.
crates/soar-dl/src/http.rs (2)
7-10: LGTM! Clean HEAD request implementation.The method correctly delegates to
SHARED_AGENTand maps errors to the crate'sDownloadErrortype.
12-27: LGTM! Proper resumable download support.The
fetchmethod correctly implements HTTP resume functionality with theRangeheader, and includesIf-Rangeto validate the resource hasn't changed when resuming.crates/soar-dl/src/utils.rs (3)
9-22: LGTM! Past review comment addressed.The function correctly percent-decodes URL path segments (lines 15-20), resolving the previous concern about filenames with special characters like spaces.
24-38: LGTM! Path traversal protection implemented.Lines 32-37 correctly sanitize the extracted filename by splitting on path separators and taking the last segment. This prevents directory traversal attacks via malicious
Content-Dispositionheaders, addressing the previous review comment.
40-72: LGTM! Robust output path resolution.The logic correctly handles all output scenarios: stdout (
"-"), directory paths (with trailing/or existing directories), explicit file paths, and fallback to extracted filenames. Error handling withNoFilenameis appropriate.crates/soar-dl/src/error.rs (2)
4-62: LGTM! Well-designed error surface.The
DownloadErrorenum provides comprehensive error variants with:
- Clear diagnostic codes for structured error reporting
- Helpful error messages and suggestions
- Appropriate use of
#[source]for error chaining- Boxing of
ureq::Errorto avoid large enum variantsThe integration with
mietteandthiserrorenables rich error diagnostics for end users.
66-70: LGTM! Efficient error conversion.Boxing the
ureq::Errorin the conversion keeps theDownloadErrorenum size reasonable, which is a best practice for error types that are frequently passed by value.crates/soar-dl/src/github.rs (1)
38-48: Tag encoding properly implemented.The percent-encoding for tags with special characters (/, +, spaces) is now correctly handled using
url::form_urlencoded::byte_serialize. This ensures tags likerelease/v1.0.0will properly encode and hit the correct GitHub endpoint.crates/soar-dl/src/platform.rs (3)
49-56: GHCR URL scheme handling properly implemented.The normalization now correctly strips
https://andhttp://prefixes before checking forghcr.io/, ensuring both scheme-less and scheme-full inputs are handled consistently.
66-66: GitLab API detection tightened.The check now uses
starts_with("api/")instead ofstarts_with("api"), preventing legitimate group names likeapi-teamfrom being misclassified as API endpoints.
120-123: Token properly trimmed before authorization.Environment variable tokens are now trimmed before formatting into the Bearer header, preventing trailing newlines or whitespace from breaking authentication.
crates/soar-dl/src/filter.rs (3)
1-2: LGTM!The imports are appropriate for implementing the filtering logic.
4-11: LGTM!The struct design is clean with appropriate derives. The
Defaultimplementation (empty vectors, case_sensitive=false) will correctly match all names once the exclude logic bug is fixed.
17-17: Empty globs now correctly handled.The previous review comment about empty globs rejecting all assets has been addressed. The short-circuit OR ensures that an empty glob list matches everything.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Rabindra Dhakal <contact@qaidvoid.dev>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
crates/soar-dl/src/download.rs (1)
228-228: Handle missing resume metadata gracefully.When downloads complete without ever hitting a 1 MiB boundary (common for small files), no resume metadata xattr is written. Calling
remove_resumethen fails withErrorKind::NotFound, turning a successful download into a failure. Ignore this specific error.Apply this diff:
- remove_resume(&output_path)?; + if let Err(err) = remove_resume(&output_path) { + if err.kind() != std::io::ErrorKind::NotFound { + return Err(err.into()); + } + }crates/soar-dl/src/oci.rs (1)
842-847: Handle missing resume metadata gracefully.The unconditional
remove_resumecall (line 846) fails withErrorKind::NotFoundwhen no resume metadata exists, which happens for blobs smaller than 1 MiB that never trigger the periodic persistence at line 821. This turns successful downloads into failures.Apply this diff:
- remove_resume(path)?; + if let Err(err) = remove_resume(path) { + if err.kind() != std::io::ErrorKind::NotFound { + return Err(err.into()); + } + }
🧹 Nitpick comments (1)
crates/soar-dl/src/oci.rs (1)
219-249: Minor formatting inconsistencies.Lines 219-234 contain extraneous blank lines and whitespace, and line 249 uses an em dash ("—") instead of a standard comment marker. While these don't break functionality, cleaning them up would improve readability.
Apply this diff to normalize the formatting:
/// Sets the output directory path where downloaded files will be written. - /// - /// The provided path is stored and used as the destination for downloaded blobs and extracted contents. - /// - /// # Examples - /// - /// ``` - /// let dl = OciDownload::new("ghcr.io/org/repo:tag").output("downloads/"); - /// ``` pub fn output(mut self, path: impl Into<String>) -> Self { self.output = Some(path.into()); self } /// Sets how existing files are handled when writing downloads. /// /// This configures the downloader's overwrite behavior to the provided `mode`. /// /// # Examples /// /// ``` /// let dl = OciDownload::new("ghcr.io/example/repo:tag").overwrite(OverwriteMode::Prompt); /// ``` - — Returns the modified downloader with the given overwrite mode. + /// Returns the modified downloader with the given overwrite mode. pub fn overwrite(mut self, mode: OverwriteMode) -> Self {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
crates/soar-dl/src/download.rs(1 hunks)crates/soar-dl/src/error.rs(1 hunks)crates/soar-dl/src/filter.rs(1 hunks)crates/soar-dl/src/github.rs(1 hunks)crates/soar-dl/src/gitlab.rs(1 hunks)crates/soar-dl/src/http.rs(1 hunks)crates/soar-dl/src/http_client.rs(1 hunks)crates/soar-dl/src/oci.rs(1 hunks)crates/soar-dl/src/platform.rs(1 hunks)crates/soar-dl/src/release.rs(1 hunks)crates/soar-dl/src/utils.rs(1 hunks)crates/soar-dl/src/xattr.rs(1 hunks)crates/soar-utils/src/fs.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/soar-dl/src/utils.rs
- crates/soar-utils/src/fs.rs
- crates/soar-dl/src/platform.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-01T05:06:05.979Z
Learnt from: QaidVoid
Repo: pkgforge/soar PR: 102
File: crates/soar-dl/src/oci.rs:338-449
Timestamp: 2025-11-01T05:06:05.979Z
Learning: In Rust code interacting with ghcr.io (GitHub Container Registry), the Authorization header with "Bearer QQ==" is a valid anonymous token pattern used for accessing public manifests and blobs. This is not a placeholder or security issue—it's an intentional pattern for anonymous registry access.
Applied to files:
crates/soar-dl/src/oci.rs
🧬 Code graph analysis (8)
crates/soar-dl/src/http.rs (2)
crates/soar-dl/src/http_client.rs (1)
head(103-111)crates/soar-dl/src/error.rs (1)
from(82-84)
crates/soar-dl/src/release.rs (4)
crates/soar-dl/src/download.rs (7)
new(51-60)output(76-79)overwrite(95-98)extract(111-114)extract_to(129-132)progress(151-157)execute(183-242)crates/soar-dl/src/github.rs (3)
tag(126-128)fetch_releases(53-75)assets(195-197)crates/soar-dl/src/gitlab.rs (3)
tag(111-113)fetch_releases(61-79)assets(177-179)crates/soar-dl/src/traits.rs (3)
tag(13-13)fetch_releases(26-29)assets(16-16)
crates/soar-dl/src/github.rs (2)
crates/soar-dl/src/platform.rs (1)
fetch_with_fallback(165-219)crates/soar-dl/src/traits.rs (9)
fetch_releases(26-29)tag(13-13)name(4-4)name(12-12)is_prerelease(14-14)published_at(15-15)assets(16-16)size(5-5)url(6-6)
crates/soar-dl/src/http_client.rs (1)
crates/soar-dl/src/http.rs (1)
head(8-10)
crates/soar-dl/src/download.rs (4)
crates/soar-utils/src/fs.rs (1)
is_elf(296-301)crates/soar-dl/src/utils.rs (3)
filename_from_header(68-81)filename_from_url(42-54)resolve_output_path(118-149)crates/soar-dl/src/xattr.rs (3)
read_resume(22-27)remove_resume(60-62)write_resume(45-47)crates/soar-dl/src/http.rs (2)
head(8-10)fetch(34-49)
crates/soar-dl/src/gitlab.rs (3)
crates/soar-dl/src/github.rs (11)
fetch_with_fallback(69-69)fetch_releases(53-75)tag(126-128)name(104-106)name(217-219)is_prerelease(148-150)published_at(166-168)assets(195-197)size(233-235)url(60-60)url(249-251)crates/soar-dl/src/platform.rs (1)
fetch_with_fallback(165-219)crates/soar-dl/src/traits.rs (9)
fetch_releases(26-29)tag(13-13)name(4-4)name(12-12)is_prerelease(14-14)published_at(15-15)assets(16-16)size(5-5)url(6-6)
crates/soar-dl/src/oci.rs (3)
crates/soar-utils/src/fs.rs (1)
is_elf(296-301)crates/soar-dl/src/xattr.rs (3)
read_resume(22-27)remove_resume(60-62)write_resume(45-47)crates/soar-dl/src/download.rs (7)
new(51-60)output(76-79)overwrite(95-98)extract(111-114)extract_to(129-132)progress(151-157)execute(183-242)
crates/soar-dl/src/filter.rs (3)
crates/soar-dl/src/github.rs (2)
name(104-106)name(217-219)crates/soar-dl/src/gitlab.rs (2)
name(93-95)name(191-193)crates/soar-dl/src/traits.rs (2)
name(4-4)name(12-12)
⏰ 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). (1)
- GitHub Check: coverage
🔇 Additional comments (20)
crates/soar-dl/src/github.rs (4)
1-25: LGTM! Well-structured GitHub API data models.The import organization is clean, and the structs correctly map to GitHub's release and asset API schema. The use of
Option<String>for the release name field properly handles cases where releases lack names.
27-76: LGTM! Tag encoding properly implemented.The percent-encoding of tags (lines 59-60) correctly handles special characters like
/,+, and spaces in tag names, resolving the previous concern about tags likerelease/v1.0.0breaking the API path. The implementation properly encodes the tag before URL construction.
78-198: LGTM! Comprehensive trait implementation with excellent documentation.The Release trait implementation is complete and well-documented. The handling of optional release names (line 105) with a fallback to an empty string is a sensible default. All accessor methods correctly expose the underlying fields through the trait interface.
200-252: LGTM! Complete Asset trait implementation.The Asset trait implementation correctly exposes all asset metadata. Wrapping
sizeinSome(line 234) maintains consistency with the trait signature while acknowledging that GitHub reliably provides asset sizes.crates/soar-dl/src/download.rs (5)
1-32: LGTM! Clean builder structure with appropriate field types.The Download struct provides a solid foundation for the builder pattern with well-chosen field types. The use of
Box<dyn Fn(Progress) + Send + Sync>for the progress callback ensures thread-safety while maintaining flexibility.
34-157: LGTM! Well-implemented builder pattern with excellent documentation.All builder methods follow the consume-and-return pattern correctly, enabling fluent chaining. The comprehensive docstrings with examples make the API easy to understand and use.
230-239: LGTM! Extraction properly implemented.The extraction logic now correctly calls
compak::extract_archive(addressing the previous concern that extraction was a no-op). The fallback to the parent directory or current directory whenextract_tois not set is a sensible default.
244-405: LGTM! Robust download implementation with resume support.The download helpers are well-implemented:
download_to_stdoutefficiently streams to stdoutdownload_to_fileproperly implements resumable downloads with ETag validation and periodic metadata persistenceparse_content_lengthcorrectly prioritizesContent-RangeoverContent-Lengthfor partial responsesThe resume logic correctly handles both fresh downloads and resumption from prior interrupted attempts.
407-436: LGTM! Clean user prompt implementation.The overwrite prompt implementation is straightforward and correct. The case-insensitive matching using
matches!with both "y" and "yes" provides good UX.crates/soar-dl/src/http_client.rs (4)
1-42: LGTM! Clean HTTP client configuration structure.The
ClientConfigstruct design is well-suited for configuring a shared HTTP client. The default user agent "pkgforge/soar" is appropriate, and making all fields optional provides flexibility.
44-84: LGTM! Proper agent configuration and thread-safe initialization.The
build()method correctly constructs aureq::Agentfrom the configuration, and theLazyLock<Arc<RwLock<SharedClient>>>pattern ensures thread-safe lazy initialization of the shared client state.
202-232: LGTM! Clean header application and agent initialization.The
apply_headershelper correctly iterates through the optionalHeaderMapand applies each header to the request. TheSHARED_AGENTstatic is properly initialized usingLazyLock.
234-257: LGTM! Thread-safe client reconfiguration.The
configure_http_clientfunction properly uses a write lock to atomically update both the configuration and the rebuilt agent, ensuring thread-safe runtime reconfiguration of the shared HTTP client.crates/soar-dl/src/oci.rs (7)
1-83: LGTM! Flexible OCI reference parsing with comprehensive examples.The
OciReferenceparsing correctly handles multiple input formats (digest-based, tag-based, and implicit latest). The implementation properly strips the optionalghcr.io/prefix and extracts package and tag components. The extensive docstrings make the parsing behavior clear.
85-142: LGTM! Correct OCI manifest data structures.The structs accurately model the OCI image manifest specification with appropriate
serderename attributes for JSON field mapping. Thetitle()accessor provides a convenient way to extract human-readable layer names from the standard annotation.
301-334: LGTM! Proper parallel configuration and progress callback setup.The
parallelmethod correctly enforces a minimum of 1 worker (line 311), and theprogressmethod appropriately wraps the callback in anArcfor thread-safe sharing across download workers.
336-415: LGTM! Comprehensive execute implementation with proper flow control.The
executemethod correctly distinguishes between direct blob downloads (digest-based references) and manifest-based layer downloads. The layer filtering, directory creation, and dispatch to parallel or sequential download paths are all properly orchestrated. Progress callbacks are consistently invoked at the start and completion.
417-576: LGTM! Well-implemented sequential and parallel download orchestration.Both download methods are properly implemented:
download_layers_sequentialprovides straightforward iteration with error propagationdownload_layers_parallelcorrectly chunks work across threads, uses thread-safe synchronization primitives (Arc<Mutex>), and aggregates errors into a single resultThe parallel implementation properly handles extraction within worker threads and collects both successful paths and errors.
578-667: LGTM! Correct manifest fetching and blob download delegation.The
fetch_manifestimplementation properly constructs the OCI API URL and sets appropriate Accept headers for OCI/Docker manifest formats. TheAuthorization: Bearer QQ==header on line 608 is intentional for anonymous access to ghcr.io public images. Based on learnings.The
download_blobmethod appropriately delegates to theDownloadbuilder for single-blob downloads.
712-845: LGTM! Robust blob download with comprehensive resume support.The
download_layer_implfunction implements a complete download solution:
- Proper resume support with Range and If-Range headers (lines 776-781)
- Periodic persistence of resume metadata every 1 MiB (line 821)
- Thread-safe progress reporting using shared counters (lines 817-819, 833-839)
- Automatic executable permission setting for ELF binaries (lines 842-844)
The implementation correctly handles both fresh downloads and partial resumption.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/soar-dl/src/download.rs (1)
343-343: Resume metadata write condition may miss MiB boundaries.The condition
downloaded % (1024 * 1024) == 0only fires whendownloadedis an exact multiple of 1 MiB. If a read chunk spans a MiB boundary (e.g., going from 1,048,000 to 1,056,192 bytes), the condition never evaluates to true and resume metadata is not persisted. This reduces resume reliability for interrupted downloads.Track the last-written boundary and write whenever crossing a new MiB threshold:
let mut reader = resp.into_body().into_reader(); let mut buffer = [0u8; 8192]; let mut downloaded = resume_from.unwrap_or(0); + let mut last_checkpoint = downloaded / (1024 * 1024); loop { let n = reader.read(&mut buffer)?; if n == 0 { break; } file.write_all(&buffer[..n])?; downloaded += n as u64; - if downloaded % (1024 * 1024) == 0 { + let current_checkpoint = downloaded / (1024 * 1024); + if current_checkpoint > last_checkpoint { + last_checkpoint = current_checkpoint; write_resume(crates/soar-dl/src/oci.rs (2)
803-803: Resume checkpoint logic may skip MiB boundaries.The condition
(*local_downloaded).is_multiple_of(1024 * 1024)only triggers when the downloaded byte count is exactly divisible by 1 MiB. If a read spans a boundary, resume metadata won't be written at that checkpoint, reducing resilience.Track the last checkpoint and write whenever crossing a new MiB threshold:
let mut reader = resp.into_body().into_reader(); let mut buffer = [0u8; 8192]; *local_downloaded = resume_from.unwrap_or(0); + let mut last_checkpoint = *local_downloaded / (1024 * 1024); loop { let n = reader.read(&mut buffer)?; if n == 0 { break; } file.write_all(&buffer[..n])?; *local_downloaded += n as u64; { let mut shared = shared_downloaded.lock().unwrap(); *shared += n as u64; } - if (*local_downloaded).is_multiple_of(1024 * 1024) { + let current_checkpoint = *local_downloaded / (1024 * 1024); + if current_checkpoint > last_checkpoint { + last_checkpoint = current_checkpoint; write_resume(
565-567: Thread panics are silently ignored.Calling
handle.join().ok()discards thread panics without logging or reporting them. While download errors are captured in theerrorsvector, panics caused by bugs (e.g., index out of bounds, unwrap failures) are lost, making debugging difficult.Consider logging panics or at least counting them:
for handle in handles { - handle.join().ok(); + if let Err(e) = handle.join() { + eprintln!("Worker thread panicked: {:?}", e); + errors.lock().unwrap().push("Worker thread panicked".to_string()); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
crates/soar-dl/src/download.rs(1 hunks)crates/soar-dl/src/error.rs(1 hunks)crates/soar-dl/src/filter.rs(1 hunks)crates/soar-dl/src/github.rs(1 hunks)crates/soar-dl/src/gitlab.rs(1 hunks)crates/soar-dl/src/http.rs(1 hunks)crates/soar-dl/src/http_client.rs(1 hunks)crates/soar-dl/src/oci.rs(1 hunks)crates/soar-dl/src/platform.rs(1 hunks)crates/soar-dl/src/release.rs(1 hunks)crates/soar-dl/src/utils.rs(1 hunks)crates/soar-dl/src/xattr.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/soar-dl/src/filter.rs
- crates/soar-dl/src/xattr.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-01T05:06:05.979Z
Learnt from: QaidVoid
Repo: pkgforge/soar PR: 102
File: crates/soar-dl/src/oci.rs:338-449
Timestamp: 2025-11-01T05:06:05.979Z
Learning: In Rust code interacting with ghcr.io (GitHub Container Registry), the Authorization header with "Bearer QQ==" is a valid anonymous token pattern used for accessing public manifests and blobs. This is not a placeholder or security issue—it's an intentional pattern for anonymous registry access.
Applied to files:
crates/soar-dl/src/oci.rscrates/soar-dl/src/platform.rs
🧬 Code graph analysis (8)
crates/soar-dl/src/github.rs (3)
crates/soar-dl/src/gitlab.rs (11)
fetch_with_fallback(81-81)fetch_releases(64-87)tag(125-127)name(104-106)name(217-219)is_prerelease(150-152)published_at(171-173)assets(200-202)size(237-239)url(72-72)url(255-257)crates/soar-dl/src/platform.rs (1)
fetch_with_fallback(153-207)crates/soar-dl/src/traits.rs (9)
fetch_releases(26-29)tag(13-13)name(4-4)name(12-12)is_prerelease(14-14)published_at(15-15)assets(16-16)size(5-5)url(6-6)
crates/soar-dl/src/oci.rs (3)
crates/soar-utils/src/fs.rs (1)
is_elf(296-301)crates/soar-dl/src/xattr.rs (3)
read_resume(23-28)remove_resume(70-75)write_resume(53-55)crates/soar-dl/src/download.rs (5)
output(79-82)overwrite(98-101)extract(116-119)extract_to(136-139)execute(192-251)
crates/soar-dl/src/release.rs (4)
crates/soar-dl/src/download.rs (7)
new(53-62)output(79-82)overwrite(98-101)extract(116-119)extract_to(136-139)progress(158-164)execute(192-251)crates/soar-dl/src/github.rs (3)
tag(135-137)fetch_releases(56-78)assets(211-213)crates/soar-dl/src/gitlab.rs (3)
tag(125-127)fetch_releases(64-87)assets(200-202)crates/soar-dl/src/traits.rs (3)
tag(13-13)fetch_releases(26-29)assets(16-16)
crates/soar-dl/src/gitlab.rs (2)
crates/soar-dl/src/platform.rs (1)
fetch_with_fallback(153-207)crates/soar-dl/src/traits.rs (9)
fetch_releases(26-29)tag(13-13)name(4-4)name(12-12)is_prerelease(14-14)published_at(15-15)assets(16-16)size(5-5)url(6-6)
crates/soar-dl/src/download.rs (4)
crates/soar-utils/src/fs.rs (1)
is_elf(296-301)crates/soar-dl/src/utils.rs (3)
filename_from_header(56-69)filename_from_url(28-40)resolve_output_path(107-138)crates/soar-dl/src/xattr.rs (3)
read_resume(23-28)remove_resume(70-75)write_resume(53-55)crates/soar-dl/src/http.rs (2)
head(8-10)fetch(34-49)
crates/soar-dl/src/http_client.rs (1)
crates/soar-dl/src/http.rs (1)
head(8-10)
crates/soar-dl/src/http.rs (2)
crates/soar-dl/src/http_client.rs (1)
head(107-115)crates/soar-dl/src/error.rs (1)
from(82-84)
crates/soar-dl/src/platform.rs (5)
crates/soar-dl/src/github.rs (4)
url(63-63)url(274-276)tag(135-137)fetch_with_fallback(72-72)crates/soar-dl/src/gitlab.rs (4)
url(72-72)url(255-257)tag(125-127)fetch_with_fallback(81-81)crates/soar-dl/src/oci.rs (1)
new(180-192)crates/soar-dl/src/release.rs (2)
new(45-57)tag(72-75)crates/soar-dl/src/http.rs (1)
json(67-74)
⏰ 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 (6)
crates/soar-dl/src/download.rs (2)
192-251: LGTM! Well-structured download orchestration.The execute flow correctly handles the HEAD-then-fetch pattern mentioned in past reviews, properly resolves output paths, respects overwrite modes, supports resume, handles ELF permissions, and integrates extraction. The logic is clear and comprehensive.
293-370: Download implementation handles resume and progress correctly.The resume logic properly validates server support (status 206 check at line 306) and falls back to full download when resume isn't available. Progress callbacks are invoked at appropriate points, and the streaming approach efficiently handles large files.
crates/soar-dl/src/oci.rs (4)
30-85: OciReference parsing handles common patterns correctly.The From<&str> implementation properly handles digest references (@sha256:), tagged references (:tag), and bare package names, defaulting to "latest" and "ghcr.io" appropriately. The logic is clear and well-documented.
365-418: Execute method orchestrates manifest and blob downloads effectively.The flow correctly distinguishes direct blob downloads (sha256: digests) from manifest-based layered downloads, applies filtering, validates that at least one layer matches, and dispatches to sequential or parallel paths based on configuration. Progress events are emitted at the right lifecycle points.
612-612: Bearer QQ== is valid for ghcr.io anonymous access.Based on learnings, this is the expected anonymous token pattern for accessing public content on GitHub Container Registry and is not a placeholder or security issue.
497-578: Parallel download implementation is well-structured.The chunking logic evenly distributes layers across workers, uses Arc<Mutex<_>> for safe shared state, and properly aggregates both results and errors. The extraction integration within worker threads is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/soar-dl/src/oci.rs (2)
569-571: Consider logging or propagating thread panics.
handle.join().ok()silently discards thread panics. While errors are collected via theerrorsArc, a panic would bypass that mechanism and the failure would appear as a missing result rather than an explicit error.Consider logging panics or propagating them:
for handle in handles { - handle.join().ok(); + if let Err(e) = handle.join() { + errors.lock().unwrap().push(format!("Thread panicked: {:?}", e)); + } }
678-728: Clarifydownloadedparameter documentation.The docstring states that the function "increments it by the number of bytes written for this layer," but the implementation resets
local_downloadedto the resume position (line 795) before accumulating. The final value represents the total bytes downloaded for the layer (including resumed bytes), not an increment to the input value.Consider rewording for clarity:
/// - `downloaded`: Mutable cumulative byte counter; this function increments it by the -/// number of bytes written for this layer. +/// number of bytes written for this layer. On entry, any existing value is replaced +/// with the total bytes downloaded (including resumed bytes) for this layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/soar-dl/src/oci.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-01T05:06:05.979Z
Learnt from: QaidVoid
Repo: pkgforge/soar PR: 102
File: crates/soar-dl/src/oci.rs:338-449
Timestamp: 2025-11-01T05:06:05.979Z
Learning: In Rust code interacting with ghcr.io (GitHub Container Registry), the Authorization header with "Bearer QQ==" is a valid anonymous token pattern used for accessing public manifests and blobs. This is not a placeholder or security issue—it's an intentional pattern for anonymous registry access.
Applied to files:
crates/soar-dl/src/oci.rs
🧬 Code graph analysis (1)
crates/soar-dl/src/oci.rs (4)
crates/soar-utils/src/fs.rs (1)
is_elf(296-301)crates/soar-dl/src/xattr.rs (3)
read_resume(23-28)remove_resume(70-75)write_resume(53-55)crates/soar-dl/src/error.rs (1)
from(82-84)crates/soar-dl/src/download.rs (7)
new(53-62)output(79-82)overwrite(98-101)extract(116-119)extract_to(136-139)progress(158-164)execute(192-251)
⏰ 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 (4)
crates/soar-dl/src/oci.rs (4)
30-85: LGTM! Well-documented OCI reference parser.The parsing logic correctly handles digest references, tagged references, and bare package names with sensible fallback to "latest". The extensive docstring and examples are helpful.
113-145: LGTM! Clean annotation accessor.The method correctly retrieves the OCI image title annotation with clear documentation.
599-629: LGTM! Manifest fetching correctly implemented.The method properly handles authentication for ghcr.io, checks HTTP status, and deserializes the manifest with appropriate error handling.
Based on learnings.
730-838: LGTM! Robust resume-aware download implementation.The implementation correctly handles:
- Resume state with ETag validation and Range requests
- Periodic resume metadata persistence (every 1 MiB)
- Progress reporting with shared counter coordination
- ELF permission adjustment and resume metadata cleanup
- Error handling throughout
| fn download_layers_sequential( | ||
| &self, | ||
| layers: &[&OciLayer], | ||
| output_dir: &Path, | ||
| ) -> Result<Vec<PathBuf>, DownloadError> { | ||
| let mut paths = Vec::new(); | ||
| let mut downloaded = 0u64; | ||
| let total_size: u64 = layers.iter().map(|l| l.size).sum(); | ||
|
|
||
| for layer in layers { | ||
| let filename = layer.title().unwrap(); | ||
| let path = output_dir.join(filename); | ||
|
|
||
| self.download_layer(layer, &path, &mut downloaded, total_size)?; | ||
|
|
||
| if self.extract { | ||
| let extract_dir = self | ||
| .extract_to | ||
| .clone() | ||
| .unwrap_or_else(|| output_dir.to_path_buf()); | ||
| compak::extract_archive(&path, &extract_dir)?; | ||
| } | ||
|
|
||
| paths.push(path); | ||
| } | ||
|
|
||
| Ok(paths) | ||
| } |
There was a problem hiding this comment.
Fix progress tracking in sequential downloads.
The sequential download path calls the public download_layer method (line 451), which internally creates a fresh Arc::new(Mutex::new(0u64)) for each layer (line 724). This causes the progress callback to report current values that restart from 0 for each layer instead of accumulating across all layers, unlike the parallel path which correctly creates a shared Arc once (line 503).
Refactor download_layers_sequential to call download_layer_impl directly (like download_layers_parallel does) with a shared Arc:
fn download_layers_sequential(
&self,
layers: &[&OciLayer],
output_dir: &Path,
) -> Result<Vec<PathBuf>, DownloadError> {
let mut paths = Vec::new();
- let mut downloaded = 0u64;
+ let shared_downloaded = Arc::new(Mutex::new(0u64));
let total_size: u64 = layers.iter().map(|l| l.size).sum();
for layer in layers {
let filename = layer.title().unwrap();
let path = output_dir.join(filename);
- self.download_layer(layer, &path, &mut downloaded, total_size)?;
+ let mut local_downloaded = 0u64;
+ download_layer_impl(
+ &self.api,
+ &self.reference,
+ layer,
+ &path,
+ &mut local_downloaded,
+ self.on_progress.as_ref(),
+ &shared_downloaded,
+ total_size,
+ )?;
if self.extract {
let extract_dir = self
.extract_to
.clone()
.unwrap_or_else(|| output_dir.to_path_buf());
compak::extract_archive(&path, &extract_dir)?;
}
paths.push(path);
}
Ok(paths)
}📝 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.
| fn download_layers_sequential( | |
| &self, | |
| layers: &[&OciLayer], | |
| output_dir: &Path, | |
| ) -> Result<Vec<PathBuf>, DownloadError> { | |
| let mut paths = Vec::new(); | |
| let mut downloaded = 0u64; | |
| let total_size: u64 = layers.iter().map(|l| l.size).sum(); | |
| for layer in layers { | |
| let filename = layer.title().unwrap(); | |
| let path = output_dir.join(filename); | |
| self.download_layer(layer, &path, &mut downloaded, total_size)?; | |
| if self.extract { | |
| let extract_dir = self | |
| .extract_to | |
| .clone() | |
| .unwrap_or_else(|| output_dir.to_path_buf()); | |
| compak::extract_archive(&path, &extract_dir)?; | |
| } | |
| paths.push(path); | |
| } | |
| Ok(paths) | |
| } | |
| fn download_layers_sequential( | |
| &self, | |
| layers: &[&OciLayer], | |
| output_dir: &Path, | |
| ) -> Result<Vec<PathBuf>, DownloadError> { | |
| let mut paths = Vec::new(); | |
| let shared_downloaded = Arc::new(Mutex::new(0u64)); | |
| let total_size: u64 = layers.iter().map(|l| l.size).sum(); | |
| for layer in layers { | |
| let filename = layer.title().unwrap(); | |
| let path = output_dir.join(filename); | |
| let mut local_downloaded = 0u64; | |
| download_layer_impl( | |
| &self.api, | |
| &self.reference, | |
| layer, | |
| &path, | |
| &mut local_downloaded, | |
| self.on_progress.as_ref(), | |
| &shared_downloaded, | |
| total_size, | |
| )?; | |
| if self.extract { | |
| let extract_dir = self | |
| .extract_to | |
| .clone() | |
| .unwrap_or_else(|| output_dir.to_path_buf()); | |
| compak::extract_archive(&path, &extract_dir)?; | |
| } | |
| paths.push(path); | |
| } | |
| Ok(paths) | |
| } |
🤖 Prompt for AI Agents
In crates/soar-dl/src/oci.rs around lines 438-465, the sequential path currently
calls the public download_layer which creates a fresh progress counter per
layer; instead create a single shared progress counter (Arc<Mutex<u64>>) before
the loop and call the internal download_layer_impl for each layer, passing
Arc::clone(&downloaded) and the same total_size so progress accumulates across
layers; keep the existing extract logic and paths.push behavior, and ensure
std::sync::{Arc, Mutex} is in scope and you no longer call the public
download_layer inside this loop.
|
@coderabbitai generate unit tests |
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Unit Test Generation process completed. You can view the generated tests in this PR: View PR |
✅ Actions performedUnit Test Generation started... |
… for soar-dl crate (#104) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Rabindra Dhakal <contact@qaidvoid.dev>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/soar-dl/src/oci.rs (2)
646-676: Use the path returned byDownload::execute().The function ignores the
PathBufreturned byDownload::execute()at line 673 and instead returns the locally constructedpathat line 675. IfDownloadresolves or modifies the output path (e.g., handling conflicts or special cases), the returned path will be incorrect.Apply this diff:
- dl.execute()?; + let downloaded_path = dl.execute()?; - Ok(vec![path]) + Ok(vec![downloaded_path])
438-465: Fix progress tracking in sequential downloads.The sequential download path calls
download_layerat line 451, which internally creates a freshArc::new(Mutex::new(0u64))at line 724 for each layer. This causes the progress callback to report per-layer bytes instead of cumulative progress across all layers, unlike the parallel path which correctly shares a singleArc(line 503).Refactor to call
download_layer_impldirectly with a shared progress counter:fn download_layers_sequential( &self, layers: &[&OciLayer], output_dir: &Path, ) -> Result<Vec<PathBuf>, DownloadError> { let mut paths = Vec::new(); - let mut downloaded = 0u64; + let shared_downloaded = Arc::new(Mutex::new(0u64)); let total_size: u64 = layers.iter().map(|l| l.size).sum(); for layer in layers { let filename = layer.title().unwrap(); let path = output_dir.join(filename); - self.download_layer(layer, &path, &mut downloaded, total_size)?; + let mut local_downloaded = 0u64; + download_layer_impl( + &self.api, + &self.reference, + layer, + &path, + &mut local_downloaded, + self.on_progress.as_ref(), + &shared_downloaded, + total_size, + )?; if self.extract { let extract_dir = self .extract_to .clone() .unwrap_or_else(|| output_dir.to_path_buf()); compak::extract_archive(&path, &extract_dir)?; } paths.push(path); } Ok(paths) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/soar-dl/Cargo.toml(1 hunks)crates/soar-dl/src/error.rs(1 hunks)crates/soar-dl/src/filter.rs(1 hunks)crates/soar-dl/src/http_client.rs(1 hunks)crates/soar-dl/src/oci.rs(1 hunks)crates/soar-dl/src/platform.rs(1 hunks)crates/soar-dl/src/types.rs(1 hunks)crates/soar-dl/src/utils.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/soar-dl/src/filter.rs
- crates/soar-dl/src/types.rs
- crates/soar-dl/Cargo.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-01T05:06:05.979Z
Learnt from: QaidVoid
Repo: pkgforge/soar PR: 102
File: crates/soar-dl/src/oci.rs:338-449
Timestamp: 2025-11-01T05:06:05.979Z
Learning: In Rust code interacting with ghcr.io (GitHub Container Registry), the Authorization header with "Bearer QQ==" is a valid anonymous token pattern used for accessing public manifests and blobs. This is not a placeholder or security issue—it's an intentional pattern for anonymous registry access.
Applied to files:
crates/soar-dl/src/platform.rscrates/soar-dl/src/oci.rs
🧬 Code graph analysis (3)
crates/soar-dl/src/platform.rs (3)
crates/soar-dl/src/gitlab.rs (3)
url(72-72)url(255-257)fetch_with_fallback(81-81)crates/soar-dl/src/github.rs (3)
url(63-63)url(274-276)fetch_with_fallback(72-72)crates/soar-dl/src/http.rs (1)
json(67-74)
crates/soar-dl/src/http_client.rs (1)
crates/soar-dl/src/http.rs (1)
head(8-10)
crates/soar-dl/src/oci.rs (4)
crates/soar-utils/src/fs.rs (1)
is_elf(296-301)crates/soar-dl/src/xattr.rs (3)
read_resume(23-28)remove_resume(70-75)write_resume(53-55)crates/soar-dl/src/download.rs (8)
new(53-62)output(79-82)overwrite(98-101)extract(116-119)extract_to(136-139)progress(158-164)execute(192-251)len(390-390)crates/soar-dl/src/http.rs (1)
json(67-74)
🪛 GitHub Actions: CI
crates/soar-dl/src/platform.rs
[error] 1-1: cargo fmt --all -- --check detected formatting changes needed in soar-dl/src/platform.rs.
crates/soar-dl/src/oci.rs
[error] 1-1: cargo fmt --all -- --check detected formatting changes needed in soar-dl/src/oci.rs.
🔇 Additional comments (7)
crates/soar-dl/src/error.rs (1)
1-193: LGTM! Well-structured error types with excellent diagnostic support.The error enum provides comprehensive coverage of download failure scenarios with helpful diagnostic codes and messages. The From conversions are correctly implemented, and the test coverage validates formatting and error chaining.
crates/soar-dl/src/utils.rs (1)
1-322: LGTM! Utility functions are well-implemented with proper security measures.The functions correctly handle URL decoding, path traversal sanitization, and output path resolution. Previous security concerns have been addressed, and test coverage is comprehensive.
crates/soar-dl/src/http_client.rs (1)
1-397: LGTM! Well-designed shared HTTP client with proper synchronization.The implementation correctly uses
RwLockto manage shared state andLazyLockfor initialization. Read locks are held only during request builder creation (not during I/O), and theconfigure_http_clientfunction safely updates the global configuration.crates/soar-dl/src/platform.rs (1)
10-218: LGTM! Platform URL parsing and fallback fetch logic are well-implemented.The URL parsing correctly handles OCI references, GitHub, GitLab, and direct URLs with proper normalization. The
fetch_with_fallbackfunction implements sensible retry logic for rate limits and server errors. Previous security concerns (token trimming, URL handling) have been addressed.crates/soar-dl/src/oci.rs (3)
599-629: LGTM! Manifest fetching correctly uses anonymous token.The
fetch_manifestmethod properly requests OCI manifests with appropriate Accept headers and uses the valid anonymous Bearer token pattern for ghcr.io access.Based on learnings.
741-838: LGTM! Layer download implementation with robust resume support.The
download_layer_implfunction correctly implements:
- Resume support using Range/If-Range headers and xattr metadata
- Progress reporting with shared counters
- Periodic resume checkpoint persistence (every 1 MiB)
- ELF detection and permission setting
- Cleanup of resume metadata on success
The resume metadata cleanup at line 836 is safe because
remove_resumechecks for existence first (per xattr.rs implementation).
840-1046: LGTM! Comprehensive test coverage for OCI types and parsing.Tests validate OCI reference parsing (with/without prefixes, digests, tags), layer title extraction, download builder configuration, and manifest deserialization. Good coverage of edge cases.
| @@ -0,0 +1,495 @@ | |||
| use std::{env, sync::LazyLock}; | |||
There was a problem hiding this comment.
Run cargo fmt to fix formatting issues.
The CI pipeline reports formatting differences in this file. Run cargo fmt to automatically format the code according to the project's Rustfmt configuration before merging.
#!/bin/bash
# Run cargo fmt to fix formatting issues
cargo fmt --all🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: cargo fmt --all -- --check detected formatting changes needed in soar-dl/src/platform.rs.
🤖 Prompt for AI Agents
In crates/soar-dl/src/platform.rs around lines 1 to 1, the file fails the
project's formatting checks; run the Rust formatter and commit the result.
Execute `cargo fmt --all` (or your repo's preferred rustfmt command), review the
changes to crates/soar-dl/src/platform.rs, and add/commit the formatted file so
CI passes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/soar-dl/src/oci.rs (1)
438-465: Fix sequential download progress tracking.The sequential download path calls the public
download_layermethod (line 451), which internally creates a freshArc::new(Mutex::new(0u64))for each layer (line 727 in the public method). This causes progress callbacks to restart from 0 for each layer instead of accumulating, unlike the parallel path which correctly uses a shared Arc.Refactor to call
download_layer_impldirectly with a shared progress counter, similar to the parallel path:fn download_layers_sequential( &self, layers: &[&OciLayer], output_dir: &Path, ) -> Result<Vec<PathBuf>, DownloadError> { let mut paths = Vec::new(); - let mut downloaded = 0u64; + let shared_downloaded = Arc::new(Mutex::new(0u64)); let total_size: u64 = layers.iter().map(|l| l.size).sum(); for layer in layers { let filename = layer.title().unwrap(); let path = output_dir.join(filename); - self.download_layer(layer, &path, &mut downloaded, total_size)?; + let mut local_downloaded = 0u64; + download_layer_impl( + &self.api, + &self.reference, + layer, + &path, + &mut local_downloaded, + self.on_progress.as_ref(), + &shared_downloaded, + total_size, + )?; if self.extract { let extract_dir = self .extract_to .clone() .unwrap_or_else(|| output_dir.to_path_buf()); compak::extract_archive(&path, &extract_dir)?; } paths.push(path); } Ok(paths) }
🧹 Nitpick comments (1)
crates/soar-dl/src/gitlab.rs (1)
68-68: Use consistent URL encoding for the project identifier.The manual
replace('/', "%2F")only handles forward slashes, while other reserved characters (spaces,+,@, etc.) remain unencoded. For consistency with the tag encoding on lines 71-72, useurl::form_urlencoded::byte_serialize.Apply this diff:
- let encoded_project = project.replace('/', "%2F"); + let encoded_project = + url::form_urlencoded::byte_serialize(project.as_bytes()).collect::<String>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/soar-dl/src/download.rs(1 hunks)crates/soar-dl/src/github.rs(1 hunks)crates/soar-dl/src/gitlab.rs(1 hunks)crates/soar-dl/src/oci.rs(1 hunks)crates/soar-dl/src/utils.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-01T05:06:05.979Z
Learnt from: QaidVoid
Repo: pkgforge/soar PR: 102
File: crates/soar-dl/src/oci.rs:338-449
Timestamp: 2025-11-01T05:06:05.979Z
Learning: In Rust code interacting with ghcr.io (GitHub Container Registry), the Authorization header with "Bearer QQ==" is a valid anonymous token pattern used for accessing public manifests and blobs. This is not a placeholder or security issue—it's an intentional pattern for anonymous registry access.
Applied to files:
crates/soar-dl/src/oci.rs
🧬 Code graph analysis (4)
crates/soar-dl/src/download.rs (4)
crates/soar-utils/src/fs.rs (1)
is_elf(296-301)crates/soar-dl/src/utils.rs (3)
filename_from_header(56-69)filename_from_url(28-40)resolve_output_path(107-130)crates/soar-dl/src/xattr.rs (3)
read_resume(23-28)remove_resume(70-75)write_resume(53-55)crates/soar-dl/src/http.rs (2)
head(8-10)fetch(34-49)
crates/soar-dl/src/gitlab.rs (3)
crates/soar-dl/src/github.rs (11)
fetch_with_fallback(72-72)fetch_releases(56-78)tag(135-137)name(110-112)name(236-238)is_prerelease(160-162)published_at(181-183)assets(211-213)size(255-257)url(63-63)url(274-276)crates/soar-dl/src/platform.rs (1)
fetch_with_fallback(154-208)crates/soar-dl/src/traits.rs (9)
fetch_releases(26-29)tag(13-13)name(4-4)name(12-12)is_prerelease(14-14)published_at(15-15)assets(16-16)size(5-5)url(6-6)
crates/soar-dl/src/github.rs (3)
crates/soar-dl/src/gitlab.rs (11)
fetch_with_fallback(81-81)fetch_releases(64-87)tag(125-127)name(104-106)name(217-219)is_prerelease(150-152)published_at(171-173)assets(200-202)size(237-239)url(72-72)url(255-257)crates/soar-dl/src/platform.rs (1)
fetch_with_fallback(154-208)crates/soar-dl/src/traits.rs (9)
fetch_releases(26-29)tag(13-13)name(4-4)name(12-12)is_prerelease(14-14)published_at(15-15)assets(16-16)size(5-5)url(6-6)
crates/soar-dl/src/oci.rs (5)
crates/soar-utils/src/fs.rs (1)
is_elf(296-301)crates/soar-dl/src/xattr.rs (3)
read_resume(23-28)remove_resume(70-75)write_resume(53-55)crates/soar-dl/src/error.rs (1)
from(82-84)crates/soar-dl/src/download.rs (7)
new(53-62)output(79-82)overwrite(98-101)extract(116-119)extract_to(136-139)execute(192-251)len(393-393)crates/soar-dl/src/http.rs (1)
json(67-74)
🔇 Additional comments (22)
crates/soar-dl/src/gitlab.rs (1)
205-258: LGTM!The
Assetimplementation forGitLabAssetis correct. The methods properly expose the asset's name and URL, and the documentation accurately notes that GitLab assets don't provide size information.crates/soar-dl/src/github.rs (3)
56-79: LGTM!The
fetch_releasesimplementation correctly handles tag encoding usingurl::form_urlencoded::byte_serializeto prevent issues with special characters in tag names. The project parameter is correctly used without encoding since GitHub expects the "owner/repo" format with a literal slash.
81-214: LGTM!The
Releaseimplementation forGithubReleasecorrectly implements all trait methods with appropriate fallbacks (empty string forNonename) and comprehensive documentation with working examples.
216-277: LGTM!The
Assetimplementation forGithubAssetis correct, with all accessor methods properly exposing the asset's name, size, and download URL. The documentation accurately reflects that GitHub assets include size information.crates/soar-dl/src/utils.rs (4)
28-40: LGTM!The function correctly extracts and decodes filenames from URLs, handling percent-encoded characters and edge cases appropriately.
56-69: LGTM!The function properly extracts filenames from Content-Disposition headers and includes path traversal sanitization by stripping directory components.
107-130: LGTM!The path resolution logic is comprehensive, correctly handling stdout, directories, and filename fallbacks with proper error handling.
132-314: LGTM!Comprehensive test coverage verifying all functionality including edge cases, percent-encoding, path traversal protection, and error conditions.
crates/soar-dl/src/download.rs (6)
25-164: LGTM!The builder pattern is well-implemented with proper defaults, comprehensive documentation, and correct use of trait bounds for the progress callback.
192-251: LGTM!The execute method correctly orchestrates the download flow, using HEAD to avoid double-fetching, properly handling resume state, overwrite modes, and extraction. The remove_resume call is safe as the function internally checks for missing xattrs.
268-277: LGTM!Straightforward stdout streaming implementation with proper flushing.
293-373: LGTM!The download-to-file implementation correctly handles resumable downloads with appropriate checkpointing, progress reporting, and fallback to full download when resume is not supported.
384-396: LGTM!Correct parsing of response size from Content-Range (for partial content) with fallback to Content-Length.
407-415: LGTM!Clean interactive overwrite prompt with appropriate case-insensitive matching.
crates/soar-dl/src/oci.rs (8)
23-145: LGTM!The OCI reference parsing and manifest types are well-designed with proper deserialization support and clear handling of digest, tag, and default cases.
147-337: LGTM!The OciDownload builder is well-structured with sensible defaults and proper thread-safety for the progress callback using Arc.
365-418: LGTM!The execute method properly orchestrates OCI downloads with appropriate branching for blob vs. manifest downloads, layer filtering, and parallel/sequential execution.
498-587: LGTM!The parallel download implementation correctly uses shared state for progress tracking and error aggregation across worker threads.
603-633: LGTM!The manifest fetch correctly uses the anonymous Bearer token for ghcr.io (as confirmed in learnings) and properly handles multiple manifest format types.
Based on learnings
650-679: LGTM!The blob download correctly uses the path returned by
Download::execute()(line 676) rather than the locally constructed path.
744-844: LGTM!The layer download implementation correctly handles resume state, progress tracking with the shared counter, and post-download steps including ELF permission setting and resume cleanup.
846-1052: LGTM!Comprehensive test coverage for OCI reference parsing, layer title extraction, builder pattern, and JSON deserialization.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
soar-core/src/package/formats/common.rs (1)
159-171: Rename the downloaded icon before symlinking
Download::execute()writes the file to.DirIcon, but we immediately overwriteicon_output_pathwith<pkg>.<ext>without moving the data.symlink_iconwill then try to open a non-existent file and fail.Capture the path returned by
execute(), determine the extension from that file, and rename it into the final<pkg>.<ext>location before callingsymlink_icon.- Download::new(icon_url) - .output(icon_output_path.to_string_lossy()) - .overwrite(OverwriteMode::Skip) - .execute()?; - - let ext = if read_file_signature(icon_output_path, 8)? == PNG_MAGIC_BYTES { + let downloaded_icon_path = Download::new(icon_url) + .output(icon_output_path.to_string_lossy()) + .overwrite(OverwriteMode::Skip) + .execute()?; + + let ext = if read_file_signature(&downloaded_icon_path, 8)? == PNG_MAGIC_BYTES { "png" } else { "svg" }; - icon_output_path = package_path.join(format!("{}.{}", package.pkg_name, ext)); + let final_icon_path = package_path.join(format!("{}.{}", package.pkg_name, ext)); + if downloaded_icon_path != final_icon_path { + fs::rename(&downloaded_icon_path, &final_icon_path)?; + } + icon_output_path = final_icon_path;
♻️ Duplicate comments (1)
crates/soar-dl/src/oci.rs (1)
715-729: Sequential progress still resets per layer
download_layerallocates a freshArc<Mutex<u64>>each call, sodownload_layers_sequentialreportsProgress::Chunktotals that jump back to zero on every layer. This was flagged earlier and remains unresolved.Reuse a shared counter across the sequential loop (e.g., allocate
Arc<Mutex<u64>>once indownload_layers_sequentialand pass it intodownload_layer_impl, or extenddownload_layerto accept the shared counter) socurrentkeeps accumulating like the parallel path.
🧹 Nitpick comments (1)
soar-cli/src/download.rs (1)
176-204: Consider using context-based overwrite mode for package downloads.Lines 176 and 192 hardcode
OverwriteMode::Forcefor package downloads. Consider whether this should respect the context's overwrite settings instead, especially if users provide--skip-existingor want to be prompted.If context-based overwrite is desired:
- let mut dl = OciDownload::new(url.as_str()).overwrite(OverwriteMode::Force); + let mut dl = OciDownload::new(url.as_str()).overwrite(ctx.get_overwrite_mode());And similarly for the standard download:
- let mut dl = - Download::new(&package.download_url).overwrite(OverwriteMode::Force); + let mut dl = + Download::new(&package.download_url).overwrite(ctx.get_overwrite_mode());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
crates/soar-dl/src/download.rs(1 hunks)crates/soar-dl/src/oci.rs(1 hunks)crates/soar-dl/src/types.rs(1 hunks)crates/soar-utils/src/fs.rs(2 hunks)soar-cli/Cargo.toml(1 hunks)soar-cli/src/download.rs(8 hunks)soar-cli/src/install.rs(3 hunks)soar-cli/src/list.rs(2 hunks)soar-cli/src/main.rs(9 hunks)soar-cli/src/nest.rs(1 hunks)soar-cli/src/progress.rs(5 hunks)soar-cli/src/run.rs(4 hunks)soar-cli/src/self_actions.rs(3 hunks)soar-cli/src/utils.rs(2 hunks)soar-core/src/config.rs(3 hunks)soar-core/src/database/connection.rs(3 hunks)soar-core/src/database/migration.rs(2 hunks)soar-core/src/database/models.rs(1 hunks)soar-core/src/database/nests/repository.rs(2 hunks)soar-core/src/database/packages/query.rs(3 hunks)soar-core/src/error.rs(3 hunks)soar-core/src/package/formats/appimage.rs(1 hunks)soar-core/src/package/formats/common.rs(4 hunks)soar-core/src/package/formats/wrappe.rs(1 hunks)soar-core/src/package/install.rs(4 hunks)soar-core/src/package/remove.rs(1 hunks)soar-core/src/toml.rs(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- soar-core/src/database/connection.rs
- soar-core/src/package/formats/appimage.rs
- soar-core/src/toml.rs
- soar-core/src/database/migration.rs
- soar-core/src/database/models.rs
- soar-cli/src/nest.rs
- soar-cli/src/list.rs
- soar-core/src/database/packages/query.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/soar-dl/src/types.rs
- crates/soar-utils/src/fs.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-01T05:06:05.979Z
Learnt from: QaidVoid
Repo: pkgforge/soar PR: 102
File: crates/soar-dl/src/oci.rs:338-449
Timestamp: 2025-11-01T05:06:05.979Z
Learning: In Rust code interacting with ghcr.io (GitHub Container Registry), the Authorization header with "Bearer QQ==" is a valid anonymous token pattern used for accessing public manifests and blobs. This is not a placeholder or security issue—it's an intentional pattern for anonymous registry access.
Applied to files:
crates/soar-dl/src/oci.rs
🧬 Code graph analysis (10)
soar-cli/src/self_actions.rs (2)
soar-cli/src/download.rs (1)
download(67-90)crates/soar-dl/src/download.rs (1)
new(55-64)
soar-cli/src/utils.rs (2)
crates/soar-utils/src/fs.rs (1)
is_elf(296-301)crates/soar-utils/src/system.rs (1)
platform(9-16)
crates/soar-dl/src/download.rs (4)
crates/soar-utils/src/fs.rs (1)
is_elf(296-301)crates/soar-dl/src/utils.rs (3)
filename_from_header(56-69)filename_from_url(28-40)resolve_output_path(107-130)crates/soar-dl/src/xattr.rs (3)
read_resume(23-28)remove_resume(70-75)write_resume(53-55)crates/soar-dl/src/http.rs (2)
head(8-10)fetch(34-49)
soar-core/src/error.rs (1)
crates/soar-utils/src/error.rs (4)
source(43-49)source(90-97)source(267-301)source(482-488)
soar-core/src/package/formats/common.rs (4)
crates/soar-utils/src/fs.rs (2)
create_symlink(114-135)read_file_signature(217-227)soar-core/src/config.rs (2)
get_config(238-250)new(352-375)crates/soar-dl/src/download.rs (1)
new(55-64)crates/soar-dl/src/oci.rs (1)
new(181-193)
soar-cli/src/run.rs (3)
crates/soar-dl/src/download.rs (1)
new(55-64)crates/soar-dl/src/oci.rs (1)
new(181-193)soar-core/src/utils.rs (1)
get_extract_dir(91-94)
soar-cli/src/download.rs (6)
crates/soar-dl/src/oci.rs (3)
filter(224-227)new(181-193)output(240-243)soar-cli/src/utils.rs (3)
interactive_ask(30-43)select_package_interactively(74-92)response(67-67)crates/soar-dl/src/platform.rs (1)
parse(72-113)crates/soar-dl/src/download.rs (2)
new(55-64)output(81-84)soar-core/src/config.rs (2)
new(352-375)get_config(238-250)crates/soar-dl/src/filter.rs (1)
matches(39-54)
crates/soar-dl/src/oci.rs (4)
crates/soar-utils/src/fs.rs (1)
is_elf(296-301)crates/soar-dl/src/xattr.rs (3)
read_resume(23-28)remove_resume(70-75)write_resume(53-55)crates/soar-dl/src/download.rs (8)
new(55-64)output(81-84)overwrite(100-103)extract(118-121)extract_to(138-141)progress(160-166)execute(194-253)len(395-395)crates/soar-dl/src/http_client.rs (2)
new(103-105)default(36-43)
soar-core/src/package/install.rs (5)
crates/soar-dl/src/oci.rs (2)
filter(224-227)new(181-193)crates/soar-dl/src/download.rs (1)
new(55-64)crates/soar-dl/src/filter.rs (1)
matches(39-54)soar-core/src/utils.rs (1)
get_extract_dir(91-94)crates/soar-utils/src/hash.rs (1)
calculate_checksum(31-41)
soar-cli/src/main.rs (1)
crates/soar-dl/src/http_client.rs (1)
configure_http_client(236-246)
⏰ 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 (18)
soar-core/src/package/remove.rs (1)
24-29: LGTM: Formatting improvement.The multi-line struct initialization is consistent with common Rust formatting conventions and improves readability.
soar-cli/Cargo.toml (1)
43-43: Dependency declaration is correct and properly used.Verification confirms:
ureqis defined in the workspace Cargo.toml (line 55:version = "3.1.2")soar-clidirectly imports from ureq (use ureq::Proxy;in soar-cli/src/main.rs:26)- The
{ workspace = true }pattern correctly references the shared definitionThe explicit dependency is appropriate and necessary. No issues.
soar-core/src/database/nests/repository.rs (1)
18-26: LGTM! Clean refactoring of the filter_map closure.The expanded format improves readability while preserving the exact same behavior—successful results are kept, errors are logged to stderr and filtered out.
soar-core/src/error.rs (3)
133-146: LGTM! Improved readability with multi-line formatting.The formatting changes for
ReqwestErrorandRusqliteErrorroot cause handling improve code readability by using multi-line blocks instead of single-line expressions. The functional behavior remains unchanged.
170-175: LGTM! Consistent formatting improvement.The multi-line formatting of the
IoErrorconstruction in theErrorContextimplementation improves readability and maintains consistency with the formatting style used elsewhere in the file.
84-85: No issues found - error refactoring is complete.The verification confirms the refactoring is complete and correct:
- No remaining references to
PlatformErrorexist in the codebaseDownloadErroris properly integrated as a new variant insoar-core/src/error.rs(line 84) with automatic conversion via#[from]- All consuming code correctly imports and handles
DownloadErrorfrom thesoar_dlcrate- Error handling patterns are properly implemented (e.g., retry logic for HTTP 429 and network errors)
soar-cli/src/progress.rs (2)
32-49: LGTM!The migration from
DownloadStatetoProgressis correctly implemented with proper handling of all variants.
51-163: Enhanced retry state tracking looks good.The addition of
Progress::Error,Progress::Aborted, andProgress::Recoveredvariants provides better visibility into download retry states with proper atomic counter management for concurrent safety.soar-cli/src/run.rs (2)
83-91: Verify builder pattern usage for progress callback.The progress callback is set up correctly with the closure, and the builder chain properly reassigns
dl. The pattern looks correct.
94-124: Extraction flow is well-implemented.The download, extraction, and cleanup flow correctly:
- Uses the new builder API
- Handles extraction directory setup
- Moves files from extract_dir to cache_bin
- Cleans up temporary directories
soar-core/src/package/install.rs (2)
180-222: Extraction flow correctly implemented.The download and extraction logic properly:
- Uses the builder API with appropriate configuration
- Handles extraction directory setup
- Moves extracted files to the install directory
- Cleans up temporary directories
- Returns checksum for verification
152-174: No action needed; original review comment is incorrect.The
OciDownloadstruct explicitly derivesCloneand contains no expensive fields for a retry scenario. Theon_progressfield is already wrapped inArc, making its clones cheap (only reference count increment). All other fields (strings, primitives, vectors) have acceptable clone costs given the 5-second retry delay. Cloning on retry is idiomatic Rust; reconstructing the builder would be unnecessarily complex.Likely an incorrect or invalid review comment.
soar-cli/src/download.rs (6)
45-65: Well-structured helper methods.The
get_overwrite_mode()andcreate_filter()methods properly encapsulate context-to-configuration mapping, making the code more maintainable and reducing duplication.
99-150: Direct download handling correctly implemented.The
handle_direct_downloadsfunction properly:
- Uses
PlatformUrl::parse()to identify download types- Routes to appropriate handlers (direct, GitHub, GitLab, OCI)
- Falls back to package resolution when URL parsing fails
- Applies appropriate overwrite modes per context
229-258: Retry logic with proper error classification.The retry implementation correctly:
- Distinguishes retriable errors (HTTP 429, Network) from non-retriable ones
- Limits retries to 5 attempts
- Includes backoff delay (5 seconds)
- Signals
Progress::Recoveredon retry andProgress::Erroron final failure
273-340: GitHub release handling well-structured.The function properly:
- Fetches releases with optional tag filtering
- Applies user-defined filters to assets
- Provides clear error messages when no matching assets found
- Handles interactive asset selection
- Configures download with extraction and output options
342-379: GitLab release flow uses builder pattern effectively.The
ReleaseDownload::<GitLab>builder provides a clean abstraction for GitLab downloads with proper filter, overwrite, and extraction configuration.
433-454: Interactive asset selection provides good UX.The function displays assets with size information and validates user input in a loop until a valid selection is made.
| configure_http_client(|config| { | ||
| if let Some(proxy) = proxy.as_deref() { | ||
| config.proxy = Some(Proxy::new(proxy).unwrap()); | ||
| } | ||
|
|
||
| if let Some(user_agent) = user_agent { | ||
| config.user_agent = Some(user_agent); | ||
| } | ||
|
|
||
| if let Some(headers) = header { | ||
| config.headers = Some(create_http_header_map(headers)); | ||
| } | ||
| }) { | ||
| error!("Error configuring HTTP client: {}", err); | ||
| if let Some(source) = err.source() { | ||
| error!(" Caused by: {}", source); | ||
| let headers = headers | ||
| .into_iter() | ||
| .filter_map(|header| { | ||
| let (key, value) = header.split_once(':')?; | ||
| Some((key.parse().unwrap(), value.parse().unwrap())) | ||
| }) | ||
| .collect(); | ||
| config.headers = Some(headers); | ||
| } | ||
| std::process::exit(1); | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Handle potential panics in HTTP client configuration.
Lines 103, 115, and 115 contain unwrap() calls that can panic if the proxy URL is invalid or if header parsing fails. Consider propagating errors or providing user-friendly error messages.
Apply this diff to handle errors gracefully:
- configure_http_client(|config| {
- if let Some(proxy) = proxy.as_deref() {
- config.proxy = Some(Proxy::new(proxy).unwrap());
- }
+ let result = (|| -> SoarResult<()> {
+ configure_http_client(|config| {
+ if let Some(proxy) = proxy.as_deref() {
+ config.proxy = Some(
+ Proxy::new(proxy)
+ .map_err(|e| SoarError::Custom(format!("Invalid proxy URL: {}", e)))?
+ );
+ }
- if let Some(user_agent) = user_agent {
- config.user_agent = Some(user_agent);
- }
+ if let Some(user_agent) = user_agent {
+ config.user_agent = Some(user_agent);
+ }
- if let Some(headers) = header {
- let headers = headers
- .into_iter()
- .filter_map(|header| {
- let (key, value) = header.split_once(':')?;
- Some((key.parse().unwrap(), value.parse().unwrap()))
- })
- .collect();
- config.headers = Some(headers);
- }
- });
+ if let Some(headers) = header {
+ let headers = headers
+ .into_iter()
+ .filter_map(|header| {
+ let (key, value) = header.split_once(':')?;
+ match (key.parse(), value.parse()) {
+ (Ok(k), Ok(v)) => Some((k, v)),
+ _ => None,
+ }
+ })
+ .collect();
+ config.headers = Some(headers);
+ }
+ Ok(())
+ })
+ })();
+
+ if let Err(e) = result {
+ error!("Failed to configure HTTP client: {}", e);
+ return Err(e);
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
soar-cli/src/download.rs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
soar-cli/src/download.rs (5)
crates/soar-dl/src/oci.rs (3)
filter(224-227)new(181-193)output(240-243)soar-cli/src/utils.rs (3)
interactive_ask(30-43)select_package_interactively(74-92)response(67-67)crates/soar-dl/src/platform.rs (1)
parse(72-113)crates/soar-dl/src/download.rs (2)
new(55-64)output(81-84)crates/soar-dl/src/filter.rs (1)
matches(39-54)
| let selected_asset = if assets.len() == 1 || ctx.yes { | ||
| assets[0] | ||
| } else { | ||
| &select_asset_interactively(assets)? | ||
| }; | ||
|
|
||
| info!("Downloading asset: {}", selected_asset.name()); | ||
|
|
||
| let mut dl = Download::new(selected_asset.url()) | ||
| .overwrite(ctx.get_overwrite_mode()) | ||
| .extract(ctx.extract); | ||
|
|
||
| if let Some(ref out) = ctx.output { | ||
| dl = dl.output(out); | ||
| } | ||
|
|
||
| if let Some(ref extract_dir) = ctx.extract_dir { | ||
| dl = dl.extract_to(extract_dir); | ||
| } | ||
|
|
||
| let cb = ctx.progress_callback.clone(); | ||
| dl = dl.progress(move |p| { | ||
| cb(p); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Break the temporary borrow when choosing release assets.
&select_asset_interactively(assets)? produces a reference to a temporary A, so both the GitHub and GitLab branches fail to compile. Clone the pre-filtered asset when auto-selecting, and keep the helper returning an owned A so we always operate on an owned value.
Apply this diff:
@@
- let selected_asset = if assets.len() == 1 || ctx.yes {
- assets[0]
- } else {
- &select_asset_interactively(assets)?
- };
+ let selected_asset = if assets.len() == 1 || ctx.yes {
+ assets[0].clone()
+ } else {
+ select_asset_interactively(assets)?
+ };
@@
- let mut dl = Download::new(selected_asset.url())
+ let mut dl = Download::new(selected_asset.url())
@@
- let selected_asset = if assets.len() == 1 || ctx.yes {
- assets[0]
- } else {
- &select_asset_interactively(assets)?
- };
+ let selected_asset = if assets.len() == 1 || ctx.yes {
+ assets[0].clone()
+ } else {
+ select_asset_interactively(assets)?
+ };
@@
- let mut dl = Download::new(selected_asset.url())
+ let mut dl = Download::new(selected_asset.url())Also applies to: 380-406
🤖 Prompt for AI Agents
In soar-cli/src/download.rs around lines 311 to 335 (also applies to 380-406):
the code takes a reference to a temporary from
&select_asset_interactively(assets)? which borrows a temporary and causes
compilation failures; change the logic so selected_asset is an owned value by
cloning the pre-filtered asset in the auto-select branch (e.g., use
assets[0].clone() or to_owned()), and update the select_asset_interactively
helper to return an owned A (not a reference) so both branches produce an owned
asset that can be moved into Download::new and subsequent calls.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
soar-cli/src/main.rs (1)
101-118: Don’t panic on invalid proxy or header input
Proxy::new(proxy).unwrap()and the twoparse().unwrap()calls run on user-supplied CLI arguments. A typo now aborts the entire CLI with a panic instead of returning a helpful error. Please validate these values before entering theconfigure_http_clientclosure (or propagate an error from it) so we produce aSoarError/SoarResultrather than unwinding.- configure_http_client(|config| { - if let Some(proxy) = proxy.as_deref() { - config.proxy = Some(Proxy::new(proxy).unwrap()); - } + let proxy = proxy + .as_deref() + .map(|p| Proxy::new(p).map_err(|err| SoarError::Custom(format!("invalid proxy URL: {err}")))) + .transpose()?; + let headers = header + .map(|values| { + values + .into_iter() + .map(|header| { + let (key, value) = header + .split_once(':') + .ok_or_else(|| SoarError::Custom(format!("invalid header: {header}")))?; + Ok((key.trim().parse()?, value.trim().parse()?)) + }) + .collect::<SoarResult<_>>() + }) + .transpose()?; + + configure_http_client(|config| { + if let Some(proxy) = &proxy { + config.proxy = Some(proxy.clone()); + } if let Some(user_agent) = user_agent { config.user_agent = Some(user_agent); } - if let Some(headers) = header { - let headers = headers - .into_iter() - .filter_map(|header| { - let (key, value) = header.split_once(':')?; - Some((key.parse().unwrap(), value.parse().unwrap())) - }) - .collect(); - config.headers = Some(headers); - } + if let Some(headers) = &headers { + config.headers = Some(headers.clone()); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/soar-dl/src/download.rs(1 hunks)crates/soar-dl/src/gitlab.rs(1 hunks)soar-cli/src/download.rs(7 hunks)soar-cli/src/main.rs(10 hunks)soar-core/src/error.rs(4 hunks)soar-core/src/package/formats/common.rs(2 hunks)soar-core/src/package/install.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
soar-core/src/error.rs (1)
crates/soar-utils/src/error.rs (8)
from(382-448)from(492-494)from(498-500)from(504-506)source(43-49)source(90-97)source(267-301)source(482-488)
soar-cli/src/main.rs (2)
crates/soar-dl/src/http_client.rs (1)
configure_http_client(236-246)soar-cli/src/download.rs (1)
create_regex_patterns(411-421)
soar-core/src/package/install.rs (6)
soar-cli/src/download.rs (1)
download(67-90)crates/soar-dl/src/oci.rs (3)
filter(224-227)new(181-193)from(58-84)crates/soar-dl/src/download.rs (1)
new(55-64)crates/soar-dl/src/filter.rs (1)
matches(39-54)soar-core/src/utils.rs (1)
get_extract_dir(91-94)crates/soar-utils/src/hash.rs (1)
calculate_checksum(31-41)
crates/soar-dl/src/gitlab.rs (3)
crates/soar-dl/src/github.rs (11)
fetch_with_fallback(72-72)fetch_releases(56-78)tag(135-137)name(110-112)name(236-238)is_prerelease(160-162)published_at(181-183)assets(211-213)size(255-257)url(63-63)url(274-276)crates/soar-dl/src/platform.rs (1)
fetch_with_fallback(154-208)crates/soar-dl/src/traits.rs (9)
fetch_releases(26-29)tag(13-13)name(4-4)name(12-12)is_prerelease(14-14)published_at(15-15)assets(16-16)size(5-5)url(6-6)
soar-core/src/package/formats/common.rs (6)
crates/soar-utils/src/fs.rs (2)
create_symlink(114-135)walk_dir(165-193)crates/soar-utils/src/path.rs (2)
desktop_dir(141-143)icons_dir(146-148)soar-core/src/package/formats/appimage.rs (1)
integrate_appimage(11-76)soar-core/src/package/formats/mod.rs (1)
get_file_type(26-67)soar-core/src/package/formats/wrappe.rs (1)
setup_wrappe_portable_dir(6-23)soar-core/src/config.rs (1)
get_config(238-250)
soar-cli/src/download.rs (7)
soar-core/src/config.rs (2)
get_config(238-250)new(352-375)crates/soar-dl/src/oci.rs (3)
filter(224-227)new(181-193)output(240-243)soar-cli/src/utils.rs (3)
interactive_ask(30-43)select_package_interactively(74-92)response(67-67)crates/soar-dl/src/platform.rs (1)
parse(72-113)crates/soar-dl/src/gitlab.rs (8)
url(72-72)url(270-272)tag(140-142)fetch_releases(64-87)assets(215-217)name(119-121)name(232-234)size(252-254)crates/soar-dl/src/download.rs (2)
new(55-64)output(81-84)crates/soar-dl/src/filter.rs (1)
matches(39-54)
crates/soar-dl/src/download.rs (4)
crates/soar-utils/src/fs.rs (1)
is_elf(296-301)crates/soar-dl/src/utils.rs (3)
filename_from_header(56-69)filename_from_url(28-40)resolve_output_path(107-130)crates/soar-dl/src/xattr.rs (3)
read_resume(23-28)remove_resume(70-75)write_resume(53-55)crates/soar-dl/src/http.rs (2)
head(8-10)fetch(34-49)
🪛 GitHub Actions: CI
crates/soar-dl/src/gitlab.rs
[error] 97-112: unresolved import soar_dl::gitlab::GitlabAssets; no GitlabAssets in gitlab (try GitLabAssets), and type mismatches in GitLabRelease::name: found Option<_> where String is expected
crates/soar-dl/src/download.rs
[error] 149-155: non-exhaustive patterns: Progress::Error, Progress::Aborted and Progress::Recovered not covered
🔇 Additional comments (1)
crates/soar-dl/src/gitlab.rs (1)
11-18: Fix type mismatch: changenamefield toOption<String>.The
namefield is declared asStringbut the docstring examples at lines 102 and 111 correctly treat it asOption<String>(usingSome(...)andNone). GitLab's API returns a nullable name field, and your docstring at line 93 acknowledges this by saying "or an empty string if the release has no name."Apply this diff to fix the struct definition:
#[derive(Debug, Clone, Deserialize)] pub struct GitLabRelease { - pub name: String, + pub name: Option<String>, pub tag_name: String, pub upcoming_release: bool, pub released_at: String, pub assets: GitLabAssets, }Then update the
name()implementation at line 119-121 to handle theOption:fn name(&self) -> &str { - &self.name + self.name.as_deref().unwrap_or("") }Likely an incorrect or invalid review comment.
| let selected_asset = if assets.len() == 1 || ctx.yes { | ||
| assets[0] | ||
| } else { | ||
| &select_asset_interactively(assets)? | ||
| }; |
There was a problem hiding this comment.
select_asset_interactively result is dropped immediately
&select_asset_interactively(assets)? creates a reference to a temporary owned value, so the compiler rightfully complains about a dangling borrow (GitHub and GitLab branches both do this). Own the selected asset instead and work with that owned value in the rest of the function.
- let selected_asset = if assets.len() == 1 || ctx.yes {
- assets[0]
- } else {
- &select_asset_interactively(assets)?
- };
- info!("Downloading asset: {}", selected_asset.name());
- let mut dl = Download::new(selected_asset.url())
+ let selected_asset = if assets.len() == 1 || ctx.yes {
+ assets[0].clone()
+ } else {
+ select_asset_interactively(assets)?
+ };
+ info!("Downloading asset: {}", selected_asset.name());
+ let mut dl = Download::new(selected_asset.url())Apply the same change to the GitLab branch below so both paths keep an owned asset.
Also applies to: 381-385
🤖 Prompt for AI Agents
In soar-cli/src/download.rs around lines 312-316 (and similarly update lines
381-385 for the GitLab branch), the code takes a reference to a temporary owned
value (&select_asset_interactively(assets)?), which causes a dangling borrow;
instead, make selected_asset an owned value: call the interactive selector and
take ownership (e.g., return/assign an owned Asset from
select_asset_interactively or clone/move the chosen asset into selected_asset)
so the rest of the function works with the owned Asset; apply the same ownership
change in the GitLab branch as well.
| let mut retries = 0; | ||
| loop { | ||
| if retries > 5 { | ||
| if let Some(ref callback) = progress_callback { | ||
| callback(DownloadState::Aborted); | ||
| if let Some(ref callback) = self.progress_callback { | ||
| callback(Progress::Aborted); | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
Retry exhaustion currently reports success
When we exhaust retries we break out of the loop and immediately return Ok(None), so the install pipeline treats a failed download as success. Please return an error instead (and map it to SoarError) after notifying the progress callback.
- if retries > 5 {
- if let Some(ref callback) = self.progress_callback {
- callback(Progress::Aborted);
- }
- break;
- }
+ if retries > 5 {
+ if let Some(ref callback) = self.progress_callback {
+ callback(Progress::Aborted);
+ }
+ return Err(SoarError::Custom(
+ "OCI download retries exhausted".to_string(),
+ ));
+ }📝 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.
| let mut retries = 0; | |
| loop { | |
| if retries > 5 { | |
| if let Some(ref callback) = progress_callback { | |
| callback(DownloadState::Aborted); | |
| if let Some(ref callback) = self.progress_callback { | |
| callback(Progress::Aborted); | |
| } | |
| break; | |
| } | |
| let mut retries = 0; | |
| loop { | |
| if retries > 5 { | |
| if let Some(ref callback) = self.progress_callback { | |
| callback(Progress::Aborted); | |
| } | |
| return Err(SoarError::Custom( | |
| "OCI download retries exhausted".to_string(), | |
| )); | |
| } |
🤖 Prompt for AI Agents
In soar-core/src/package/install.rs around lines 142 to 149, the loop currently
breaks and later returns Ok(None) when retries are exhausted which incorrectly
signals success; instead, after calling the progress callback with
Progress::Aborted (if present) immediately return an Err mapped to the crate's
SoarError (e.g., a DownloadFailed or appropriate variant created via
SoarError::new/from) so the install pipeline receives a failure; ensure the
callback is still invoked before returning the error and adjust the function's
return path/signature usage accordingly.
| sleep(Duration::from_secs(5)); | ||
| retries += 1; | ||
| if retries > 1 { | ||
| if let Some(ref callback) = self.progress_callback { | ||
| callback(Progress::Error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid blocking the async runtime with std::thread::sleep
download_package is async, but std::thread::sleep blocks the Tokio worker thread for five seconds per retry. Switch to tokio::time::sleep(...).await so other tasks can progress during backoff.
- sleep(Duration::from_secs(5));
+ tokio::time::sleep(Duration::from_secs(5)).await;And drop the thread::sleep import in favour of tokio::time::sleep.
📝 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.
| sleep(Duration::from_secs(5)); | |
| retries += 1; | |
| if retries > 1 { | |
| if let Some(ref callback) = self.progress_callback { | |
| callback(Progress::Error); | |
| } | |
| } | |
| tokio::time::sleep(Duration::from_secs(5)).await; | |
| retries += 1; | |
| if retries > 1 { | |
| if let Some(ref callback) = self.progress_callback { | |
| callback(Progress::Error); | |
| } | |
| } |
🤖 Prompt for AI Agents
In soar-core/src/package/install.rs around lines 160 to 166, the code uses
std::thread::sleep(Duration::from_secs(5)) inside the async download_package
function which blocks the Tokio runtime; replace that call with
tokio::time::sleep(Duration::from_secs(5)).await so the task yields during
backoff, remove the std::thread::sleep import and any now-unused imports, and
ensure the function remains async (no further changes needed beyond adding
.await to the sleep call).
| return Err(err)?; | ||
| } |
There was a problem hiding this comment.
return Err(err)?; does not compile
return Err(err)?; fails to type-check and was already called out before—? performs the early return for you, so wrapping it in return keeps the function from compiling. Convert the DownloadError into a SoarError explicitly and return it directly.
- return Err(err)?;
+ return Err(err.into());📝 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.
| return Err(err)?; | |
| } | |
| return Err(err.into()); | |
| } |
🤖 Prompt for AI Agents
In soar-core/src/package/install.rs around lines 168–169, the code uses `return
Err(err)?;` which doesn't compile because `?` already returns and wrapping it
with `return` is invalid; replace this with a direct conversion of the
DownloadError into a SoarError and return it (for example, `return
Err(err.into())` or `return Err(SoarError::from(err))` depending on your
conversions), i.e., remove the `?` and explicitly convert the error type when
returning.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
soar-cli/src/inspect.rs (1)
146-151: Consider simplifying Content-Length parsing.The header parsing chain is verbose. While functionally correct, a helper function could improve readability:
- let content_length = resp - .headers() - .get(CONTENT_LENGTH) - .and_then(|h| h.to_str().ok()) - .and_then(|len| len.parse::<u64>().ok()) - .unwrap_or(0); + let content_length = resp + .headers() + .get(CONTENT_LENGTH) + .and_then(|h| h.parse().ok()) + .unwrap_or(0);Note: Defaulting to
0when the Content-Length header is missing means the size check on line 153 won't trigger for responses without this header.soar-core/src/metadata.rs (3)
30-42: Consider refactoring URL validation pattern.The function parses the URL (line 40) solely for validation but discards the result. This pattern is not idiomatic and creates unnecessary allocation.
Apply this diff to make the validation more explicit:
-fn construct_nest_url(url: &str) -> SoarResult<String> { +fn construct_and_validate_nest_url(url: &str) -> SoarResult<String> { let url = if let Some(repo) = url.strip_prefix("github:") { format!( "https://github.com/{}/releases/download/soar-nest/{}.json", repo, platform() ) } else { url.to_string() }; - Url::parse(&url).map_err(|err| SoarError::Custom(err.to_string()))?; - Ok(url) + // Validate URL format + let _ = Url::parse(&url).map_err(|err| SoarError::Custom(err.to_string()))?; + Ok(url) }Alternatively, return
Urldirectly if it's needed by callers.
202-219: Migration to Download builder looks correct, but minor redundancy.The function signature change and Download builder usage are correct for the HTTP client migration. However,
OverwriteMode::Force(line 215) is redundant since line 206-209 already returns early if the file exists.Consider simplifying:
if pubkey_file.exists() { - // skip if we already have the public key file return Ok(()); } info!("Fetching public key from {}", pubkey_url); Download::new(pubkey_url) .output(pubkey_file.to_string_lossy().to_string()) - .overwrite(OverwriteMode::Force) .execute()?;
252-252: Same URL validation pattern as construct_nest_url.Line 252 parses the URL solely for validation but discards the result, similar to the pattern in
construct_nest_url. Consider using an explicit validation helper or let-binding with_prefix to clarify intent.- Url::parse(&repo.url).map_err(|err| SoarError::Custom(err.to_string()))?; + // Validate URL format + let _ = Url::parse(&repo.url).map_err(|err| SoarError::Custom(err.to_string()))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locksoar-cli/Cargo.lockis excluded by!**/*.locksoar-core/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.toml(1 hunks)soar-cli/Cargo.toml(1 hunks)soar-cli/src/inspect.rs(4 hunks)soar-core/Cargo.toml(1 hunks)soar-core/src/error.rs(5 hunks)soar-core/src/metadata.rs(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
soar-core/src/error.rs (1)
crates/soar-utils/src/error.rs (8)
from(382-448)from(492-494)from(498-500)from(504-506)source(43-49)source(90-97)source(267-301)source(482-488)
soar-core/src/metadata.rs (2)
crates/soar-utils/src/fs.rs (1)
read_file_signature(217-227)crates/soar-utils/src/system.rs (1)
platform(9-16)
⏰ 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 (12)
soar-core/src/error.rs (2)
125-126: LGTM!The addition of
RegexErrorvariant properly supports regex-related error paths in the new download components. The automatic conversion via#[from]follows the established pattern in this enum.
80-81: Migration from reqwest to ureq is complete and verified.The transition to
ureq::Errorthrough the newsoar-dlcrate is properly implemented:
- No stale
reqwestimports orReqwestErrorreferences remain in the codebaseUreqErroris correctly defined with the#[from]attribute insoar-core/src/error.rsline 81- HTTP operations are centralized in
soar-dl'shttp_clientmodule with consistenturequsage- Error handling chain is intact:
ureq::Error→soar_dl::DownloadError→soar_core::UreqErrorPlatformErrorvariant has been successfully removed as notedNo further changes required.
soar-cli/src/inspect.rs (2)
135-136: Verify blocking I/O impact in async context.The synchronous
SHARED_AGENT.get(url).call()?executes blocking I/O within an async function, which can degrade async runtime performance by holding executor threads.For a CLI application with user-initiated inspection, this may be acceptable, but verify that this blocking call won't cause issues if multiple async operations are expected.
Consider whether spawning blocking tasks is needed:
tokio::task::spawn_blocking(move || { SHARED_AGENT.get(url).call() }) .await??
168-168: LGTM - body reading approach.Reading the entire response with
read_to_vec()is appropriate here, as the size check (line 153) prompts the user for confirmation when content exceeds 1MB.Cargo.toml (3)
1-2: Verify workspace resolver compatibility.Adding
resolver = "2"changes Cargo's dependency resolution algorithm and requires Rust 1.64+. Ensure your MSRV (minimum supported Rust version) meets this requirement, or document the change in release notes.What is the project's MSRV? If it's below Rust 1.64,
resolver = "2"may cause issues for users on older toolchains.
42-44: Clarify soar-core path dependency impact.Moving
soar-coreto a path dependency (path = "soar-core") means external consumers (if soar-core is published to crates.io) will need the local directory present. If soar-core is intended as a published library, consider keeping it version-based for published releases or document this as a workspace-only structure.Is soar-core (or any of these crates) published to crates.io? If so, verify that path dependencies don't break published packages. If workspace-only, this is fine.
52-57: Verify release profile doesn't break debug builds.The new
[profile.release]block uses aggressive settings (LTO, single codegen unit, abort on panic). Ensure this doesn't negatively impact:
- Debug build turnaround times (these settings only apply to release builds, so minimal impact)
- Error handling and panic recovery in production
- Debuggability of release binaries (strip = true removes symbols)
This is generally acceptable for a CLI app, but verify that panic = "abort" aligns with your error handling strategy.
Confirm that panic = "abort" in release builds is acceptable for your deployment model. If graceful shutdown is needed, consider catching panics at the top level or reverting this setting.
soar-cli/Cargo.toml (1)
38-38: Tokio "sync" and "time" features are confirmed to be used.Verification shows both features are actively used:
tokio::sync::Semaphoreis imported and used insoar-cli/src/install.rs(line 34)tokio::time::sleepis imported and used insoar-cli/src/download.rs(line 22)The HTTP client migration to ureq is also confirmed complete (reqwest removed, ureq used in
main.rsandinspect.rs).soar-core/Cargo.toml (1)
29-30: The original review comment is incorrect.soar-core DOES directly use
ureqandurldependencies and should keep them as direct dependencies. Specifically:
soar-core/src/metadata.rsimports and usesureq::httpfor making HTTP requests (e.g.,.get()calls to fetch metadata)soar-core/src/metadata.rsimports and usesurl::Urlfor URL validation withUrl::parse()soar-core/src/error.rsincludes error handling withUreqError(#[from] ureq::Error)These are not transitive dependencies pulled only through soar-dl; they are actively used in soar-core's own source code for metadata fetching operations. The dependencies should remain in soar-core's Cargo.toml.
Likely an incorrect or invalid review comment.
soar-core/src/metadata.rs (3)
8-8: LGTM: Import changes align with HTTP client migration.The imports correctly introduce the ureq-based HTTP client (SHARED_AGENT), Download builder from soar_dl, and necessary HTTP types for the migration from reqwest.
Also applies to: 11-15
258-269: LGTM: HTTP client migration implemented correctly.The migration to SHARED_AGENT with ureq-based HTTP handling, status code checking, and header extraction is correctly implemented and consistent with the patterns in
fetch_nest_metadata.Also applies to: 271-289
113-113: Error handling is correctly implemented.The
read_to_vec()method returnsResult<Vec<u8>, ureq::Error>, and theSoarErrorenum includes#[from] ureq::Erroron itsUreqErrorvariant. The?operator automatically convertsureq::ErrortoSoarErrorat lines 113, 289, and inspect.rs:168. No additional error handling is required.
| [workspace.dependencies] | ||
| diesel = { version = "2.3.2", features = ["64-column-tables", "sqlite", "returning_clauses_for_sqlite_3_35"] } | ||
| compak = "0.1.0" | ||
| diesel = { version = "2.3.2", features = [ | ||
| "64-column-tables", | ||
| "returning_clauses_for_sqlite_3_35", | ||
| "sqlite" | ||
| ] } | ||
| diesel_migrations = { version = "2.3.0", features = ["sqlite"] } | ||
| fast-glob = "1.0.0" | ||
| futures = "0.3.31" | ||
| miette = { version = "7.6.0", features = ["fancy"] } | ||
| percent-encoding = "2.3.2" | ||
| rayon = "1.11.0" | ||
| regex = { version = "1.11.2", default-features = false, features = ["unicode-case", "unicode-perl", "std"] } | ||
| reqwest = { version = "0.12.23", default-features = false, features = ["rustls-tls", "blocking", "http2", "json", "stream", "gzip"] } | ||
| regex = { version = "1.11.2", default-features = false, features = [ | ||
| "std", | ||
| "unicode-case", | ||
| "unicode-perl" | ||
| ] } | ||
| rusqlite = { version = "0.37.0", features = ["bundled", "rusqlite-macros"] } | ||
| serde = { version = "1.0.225", features = ["derive"] } | ||
| serde_json = { version = "1.0.145", features = ["indexmap"] } | ||
| serial_test = "3.2.0" | ||
| soar-dl = { version = "0.6.3" } | ||
| soar-core = { path = "soar-core" } | ||
| soar-dl = { path = "crates/soar-dl" } | ||
| soar-utils = { path = "crates/soar-utils" } | ||
| tempfile = "3.10.1" | ||
| thiserror = "2.0.17" | ||
| tracing = { version = "0.1.41", default-features = false } | ||
| ureq = { version = "3.1.2", features = ["json"] } | ||
| url = "2.5.7" | ||
| xattr = "1.6.1" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify new workspace dependencies are actually used.
Several new dependencies were added to the workspace manifest (compak, fast-glob, miette, percent-encoding, thiserror, ureq, url, xattr) but we haven't verified they're all consumed by workspace members. Unused dependencies increase build time and binary size.
🏁 Script executed:
#!/bin/bash
# Check which workspace members actually depend on the new workspace deps
new_deps=("compak" "fast-glob" "miette" "percent-encoding" "thiserror" "ureq" "url" "xattr")
for dep in "${new_deps[@]}"; do
echo "=== Checking usage of $dep ==="
# Search for imports or references in source files
rg -t rs "use\s+$dep|from\s+$dep" crates/ soar-cli/ soar-core/ --count || echo "Not found in source"
# Also check if any Cargo.toml references it
fd "Cargo.toml" crates/ soar-cli/ soar-core/ --exec grep -l "$dep" {} \;
doneLength of output: 940
I need to improve the verification with a corrected script. The original command had issues with ripgrep's file type detection. Let me search more thoroughly for actual dependency usage.
🏁 Script executed:
#!/bin/bash
# Improved verification: check Cargo.toml declarations and source imports
new_deps=("compak" "fast-glob" "miette" "percent-encoding" "thiserror" "ureq" "url" "xattr")
for dep in "${new_deps[@]}"; do
echo "=== Checking $dep ==="
# First, check if declared in any member Cargo.toml
echo "Declared in Cargo.toml files:"
fd "Cargo.toml" crates/ soar-cli/ soar-core/ --exec grep -H "^$dep\s*=\|\"$dep\"" {} \;
# Search for actual imports/uses with correct patterns
echo "Source code references:"
rg "use\s+$dep|extern\s+crate\s+$dep|::$dep::" --no-heading || true
echo ""
doneLength of output: 2401
Let me verify whether xattr.rs actually imports the xattr crate or if those are just documentation examples:
🏁 Script executed:
#!/bin/bash
# Check if xattr.rs actually imports and uses the xattr crate
echo "=== Content of crates/soar-dl/src/xattr.rs (first 50 lines) ==="
head -50 crates/soar-dl/src/xattr.rs
echo ""
echo "=== Searching for 'use xattr' or 'extern crate xattr' in xattr.rs ==="
grep -n "use\s*xattr\|extern\s*crate\s*xattr" crates/soar-dl/src/xattr.rs || echo "No xattr imports found"Length of output: 1671
Remove unused workspace dependencies: compak, fast-glob, and percent-encoding.
Verification confirms that three new workspace dependencies are declared but never consumed by any workspace member:
compak: declared incrates/soar-dl/Cargo.toml, no source referencesfast-glob: declared incrates/soar-dl/Cargo.toml, no source referencespercent-encoding: declared incrates/soar-dl/Cargo.toml, no source references
The remaining dependencies (miette, thiserror, ureq, url, xattr) are all actively used across workspace members. Unused dependencies unnecessarily increase build time and binary bloat.
🤖 Prompt for AI Agents
In Cargo.toml around lines 20 to 50: the workspace declares unused dependencies
compak, fast-glob, and percent-encoding which are not consumed by any workspace
member; remove their entries from the [workspace.dependencies] table (and also
remove any matching entries from crates/soar-dl/Cargo.toml if present), run
cargo metadata or cargo check to verify no member refers to them, and commit the
cleaned Cargo.toml to avoid unnecessary build overhead.
| toml = "0.9.6" | ||
| tracing = { workspace = true } | ||
| tracing-subscriber = { version = "0.3.20", default-features = false, features = ["env-filter", "fmt", "json", "nu-ansi-term"] } | ||
| ureq = { workspace = true } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify reqwest removal is complete in soar-cli.
The dependency on reqwest was removed in favor of ureq (blocking HTTP client). Confirm that:
- soar-cli doesn't still have references to reqwest in its code
- The HTTP client migration is complete (no mixed usage of reqwest and ureq)
- The async runtime is compatible with blocking ureq calls
🏁 Script executed:
#!/bin/bash
# Verify reqwest has been fully removed from soar-cli
echo "=== Checking for remaining reqwest usage in soar-cli ==="
rg -t rs "reqwest|use.*reqwest|extern.*reqwest" soar-cli/src/ || echo "No reqwest found"
echo "=== Verify ureq is actually used ==="
rg -t rs "ureq|use.*ureq" soar-cli/src/ || echo "ureq not found"Length of output: 179
I need to correct the shell script—the -t rs flag is not valid. Let me search for Rust files properly:
---
🏁 Script executed:
```shell
#!/bin/bash
# Verify reqwest has been fully removed from soar-cli with correct ripgrep syntax
echo "=== Checking for remaining reqwest usage in soar-cli ==="
rg "reqwest" soar-cli/src/ || echo "No reqwest found"
echo ""
echo "=== Checking for ureq usage in soar-cli ==="
rg "ureq" soar-cli/src/ || echo "No ureq found"
echo ""
echo "=== List Rust source files in soar-cli/src ==="
find soar-cli/src -name "*.rs" -type f
echo ""
echo "=== Check soar-cli Cargo.toml dependencies ==="
grep -A 50 "^\[dependencies\]" soar-cli/Cargo.toml | grep -E "reqwest|ureq" || echo "No reqwest/ureq in dependencies section"
Length of output: 731
Need to check the async runtime compatibility with blocking ureq:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check async runtime and ureq usage patterns
echo "=== Check soar-cli Cargo.toml for async runtime ==="
grep -E "tokio|async-std|async_runtime" soar-cli/Cargo.toml || echo "No async runtime found"
echo ""
echo "=== Check main.rs for async/await usage ==="
head -30 soar-cli/src/main.rs
echo ""
echo "=== Check actual ureq usage patterns in main.rs ==="
rg -A 3 -B 3 "ureq::" soar-cli/src/main.rs
echo ""
echo "=== Check actual ureq usage patterns in inspect.rs ==="
rg -A 3 -B 3 "ureq::" soar-cli/src/inspect.rs
Length of output: 1631
Need to verify if tokio's async runtime is actually used in a way that conflicts with blocking ureq:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check for async/await usage that might conflict with blocking ureq
echo "=== Check for #[tokio::main] macro ==="
rg "#\[tokio::main\]" soar-cli/src/ || echo "No tokio::main found"
echo ""
echo "=== Check for async fn definitions ==="
rg "async fn" soar-cli/src/ || echo "No async fn found"
echo ""
echo "=== Check for .await usage ==="
rg "\.await" soar-cli/src/ || echo "No .await found"
echo ""
echo "=== Check how tokio is used in soar-cli (if at all) ==="
rg "tokio::" soar-cli/src/ || echo "No direct tokio usage found"
echo ""
echo "=== Check soar_dl dependency for HTTP client implementation ==="
grep -A 20 "soar_dl" soar-cli/Cargo.toml || echo "soar_dl not found in direct deps"
Length of output: 8647
Need to verify if blocking ureq calls are actually being invoked from async contexts:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check actual HTTP client usage patterns
echo "=== Find where SHARED_AGENT is actually used ==="
rg "SHARED_AGENT" soar-cli/src/ -A 2 -B 2
echo ""
echo "=== Find actual HTTP calls in soar-cli (get, post, put, delete patterns) ==="
rg "\.get\(|\.post\(|\.put\(|\.delete\(" soar-cli/src/ || echo "No direct HTTP calls found"
echo ""
echo "=== Check inspect.rs more thoroughly for usage ==="
cat soar-cli/src/inspect.rs
Length of output: 7498
Critical runtime compatibility issue: Blocking ureq call in async context requires fix.
The reqwest removal is complete, but there's a critical incompatibility in the async runtime integration:
In soar-cli/src/inspect.rs, the inspect_log() function is declared as pub async fn but makes a direct blocking ureq call:
let resp = SHARED_AGENT.get(url).call()?;This blocking HTTP call inside the tokio async runtime without tokio::task::block_in_place() can cause thread pool starvation. Either wrap the call with tokio::task::block_in_place() for temporary compatibility, or refactor to use an async HTTP client throughout the stack.
🤖 Prompt for AI Agents
In soar-cli/Cargo.toml around line 42, the dependency ureq is a blocking HTTP
client but inspect_log() in soar-cli/src/inspect.rs is async and currently makes
a direct blocking call; to fix, either wrap the ureq call in
tokio::task::block_in_place() inside inspect_log() to avoid blocking the Tokio
runtime threads, or replace ureq with an async HTTP client (e.g., reqwest or
surf) and refactor the call site and any shared abstractions to use async APIs
end-to-end.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
soar-cli/src/download.rs (1)
67-90: Remove redundantprogress_callbackparameter.The
downloadfunction accepts a standaloneprogress_callbackparameter (line 73) but it's never used—all handlers accessctx.progress_callbackinstead. Either remove the parameter or use it consistently.Apply this diff:
pub async fn download( ctx: DownloadContext, links: Vec<String>, github: Vec<String>, gitlab: Vec<String>, ghcr: Vec<String>, - progress_callback: Arc<dyn Fn(Progress) + Send + Sync>, ) -> SoarResult<()> { - handle_direct_downloads(&ctx, links, ctx.output.clone(), progress_callback.clone()).await?; + handle_direct_downloads(&ctx, links, ctx.output.clone()).await?; if !github.is_empty() { handle_github_downloads(&ctx, github).await?;And update the
handle_direct_downloadssignature:pub async fn handle_direct_downloads( ctx: &DownloadContext, links: Vec<String>, output: Option<String>, - progress_callback: Arc<dyn Fn(Progress) + Send + Sync>, ) -> SoarResult<()> {
♻️ Duplicate comments (2)
soar-cli/src/download.rs (2)
381-385: Type mismatch: same issue as GitHub handler.This has the identical type mismatch as
handle_github_release(lines 312-316):assets[0]produces&&Awhile&select_asset_interactively(assets)?produces&A.Apply the same fix:
let selected_asset = if assets.len() == 1 || ctx.yes { - assets[0] + *assets[0] } else { - &select_asset_interactively(assets)? + select_asset_interactively(assets)? };
312-316: Type mismatch:assets[0]is&&Abut needs to be&A.Line 313 assigns
assets[0](which is&&Asinceassets: Vec<&A>) toselected_asset, but line 315 assigns&select_asset_interactively(assets)?(which returns&A). This creates a type mismatch—theifbranch produces&&Awhile theelseproduces&A.Apply this diff:
- let selected_asset = if assets.len() == 1 || ctx.yes { - assets[0] - } else { - &select_asset_interactively(assets)? - }; + let selected_asset = if assets.len() == 1 || ctx.yes { + *assets[0] + } else { + select_asset_interactively(assets)? + };Note: Ensure
A: Clonebound is satisfied (already present inselect_asset_interactivelysignature).
🧹 Nitpick comments (5)
soar-core/src/metadata.rs (1)
202-219: Consider making this function synchronous.The function is marked
asyncbut doesn't contain any.awaitcalls—Download::execute()appears to be synchronous. Unless you're maintaining async for API consistency with other fetch functions or future-proofing for async Download support, you could simplify the signature by removingasync.If the function should remain synchronous, apply this diff:
-pub async fn fetch_public_key<P: AsRef<Path>>(repo_path: P, pubkey_url: &str) -> SoarResult<()> { +pub fn fetch_public_key<P: AsRef<Path>>(repo_path: P, pubkey_url: &str) -> SoarResult<()> {And update the call site on line 255 to remove
.await:- fetch_public_key(&repo_path, pubkey_url).await?; + fetch_public_key(&repo_path, pubkey_url)?;crates/soar-dl/src/download.rs (2)
26-35: Consider making struct fields private.The
Downloadstruct exposes all fields aspub, which allows external code to bypass builder methods and directly mutate state. Unless there's a specific use case requiring direct field access, consider making them private and exposing only the builder API for a cleaner interface.Apply this diff if direct field access isn't required:
#[derive(Clone)] pub struct Download { - pub url: String, - pub output: Option<String>, - pub overwrite: OverwriteMode, - pub extract: bool, - pub extract_to: Option<PathBuf>, - pub on_progress: Option<Arc<dyn Fn(Progress) + Send + Sync>>, - pub ghcr_blob: bool, + url: String, + output: Option<String>, + overwrite: OverwriteMode, + extract: bool, + extract_to: Option<PathBuf>, + on_progress: Option<Arc<dyn Fn(Progress) + Send + Sync>>, + ghcr_blob: bool, }
219-226: Hardcoded path separator may break on Windows.Line 224 uses
p.ends_with("/")to detect directory hints, but Windows uses backslashes. Consider usingstd::path::MAIN_SEPARATORor checking both separators for cross-platform compatibility.Apply this diff:
let needs_head = match self.output.as_deref() { None => true, Some("-") => false, Some(p) => { let path = Path::new(p); - p.ends_with("/") || path.is_dir() + p.ends_with('/') || p.ends_with('\\') || path.is_dir() } };soar-cli/src/download.rs (2)
229-258: Consider exponential backoff for retries.The retry loop uses a fixed 5-second delay, which can amplify load during rate-limit scenarios. Implementing exponential backoff (e.g.,
2^retries * base_delay) with optional jitter would be more respectful to servers and improve success rates.Example:
} Err(err) if matches!( err, DownloadError::HttpError { status: 429, .. } | DownloadError::Network(_) ) && retries < max_retries => { retries += 1; + let delay_secs = 2u64.pow(retries.min(5)) * 2; // Cap at ~64s info!("Retrying... ({}/{})", retries, max_retries); ctx.progress_callback.clone()(Progress::Recovered); - sleep(Duration::from_secs(5)).await; + sleep(Duration::from_secs(delay_secs)).await; }
273-409: Consider abstracting GitHub and GitLab handlers to eliminate duplication.
handle_github_releaseandhandle_gitlab_releaseare nearly identical, differing only in thePlatformimplementation used. Refactoring to a generichandle_release<P: Platform>function would reduce duplication and maintenance burden.Example skeleton:
fn handle_release<P: Platform>( ctx: &DownloadContext, project: &str, tag: Option<&str>, ) -> SoarResult<()> where P::Release: Release<Asset = P::Asset>, P::Asset: Asset + Clone, { let releases = P::fetch_releases(project, tag)?; // ... rest of the common logic }Then call it as:
handle_release::<Github>(ctx, project, tag) handle_release::<GitLab>(ctx, project, tag)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/soar-dl/src/download.rs(1 hunks)crates/soar-dl/src/gitlab.rs(1 hunks)soar-cli/src/download.rs(7 hunks)soar-core/src/metadata.rs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/soar-dl/src/gitlab.rs
🧰 Additional context used
🧬 Code graph analysis (3)
soar-core/src/metadata.rs (2)
crates/soar-utils/src/fs.rs (1)
read_file_signature(217-227)crates/soar-utils/src/system.rs (1)
platform(9-16)
crates/soar-dl/src/download.rs (4)
crates/soar-utils/src/fs.rs (1)
is_elf(296-301)crates/soar-dl/src/utils.rs (3)
filename_from_header(56-69)filename_from_url(28-40)resolve_output_path(107-130)crates/soar-dl/src/xattr.rs (3)
read_resume(23-28)remove_resume(70-75)write_resume(53-55)crates/soar-dl/src/http.rs (2)
head(8-10)fetch(34-54)
soar-cli/src/download.rs (7)
soar-core/src/config.rs (2)
get_config(238-250)new(352-375)crates/soar-dl/src/oci.rs (3)
filter(224-227)new(181-193)output(240-243)soar-cli/src/utils.rs (3)
interactive_ask(30-43)select_package_interactively(74-92)response(67-67)crates/soar-dl/src/platform.rs (1)
parse(72-113)crates/soar-dl/src/gitlab.rs (8)
url(72-72)url(270-272)tag(140-142)fetch_releases(64-87)assets(215-217)name(119-121)name(232-234)size(252-254)crates/soar-dl/src/download.rs (2)
new(56-66)output(100-103)crates/soar-dl/src/filter.rs (1)
matches(39-54)
🔇 Additional comments (15)
soar-core/src/metadata.rs (6)
8-15: LGTM: Imports updated for new HTTP client pattern.The imports correctly reflect the migration to the SHARED_AGENT-based HTTP client and the Download builder pattern. All imported items are used appropriately throughout the file.
30-42: LGTM: URL validation pattern is appropriate.The function correctly validates the constructed URL via
Url::parse(line 40) while returning the string representation for downstream use. This provides format validation without tying the return type to a specific URL implementation.
82-93: Past issue resolved: Correct URL is now used.The previous review flagged that
&nest.urlwas being used instead of the transformed&url. The current code correctly uses&urlon line 83, which includes the GitHub prefix transformation fromconstruct_nest_url. The HTTP request setup with SHARED_AGENT and cache headers is implemented correctly.
95-113: LGTM: Response handling follows the new HTTP client pattern correctly.The status code checks (NOT_MODIFIED, success validation), header extraction via the new API, and body reading with
into_body().read_to_vec()are all implemented correctly for the ureq-based client.
252-252: LGTM: URL validation is consistent with the pattern used inconstruct_nest_url.The validation approach matches line 40, ensuring the URL format is valid before proceeding with the fetch operation.
258-289: LGTM: HTTP request and response handling mirrorfetch_nest_metadatacorrectly.The implementation follows the same pattern as
fetch_nest_metadatawith SHARED_AGENT, appropriate cache headers, status checks, and response body reading. The consistency across both functions is good for maintainability.crates/soar-dl/src/download.rs (5)
105-186: LGTM!The builder methods follow idiomatic Rust patterns with proper use of
Into<T>conversions and thread-safe callback wrapping viaArc<dyn Fn(Progress) + Send + Sync>. The documentation is comprehensive.
307-316: LGTM!The stdout streaming logic correctly uses
std::io::copyfor efficient transfer and properly flushes the output. The sentinel return valuePathBuf::from("-")is well-documented.
423-435: LGTM!The content-length parsing correctly prioritizes
Content-Rangefor partial responses and gracefully falls back toContent-Length, returning 0 for unknown sizes.
446-454: LGTM!The prompt logic is straightforward with proper stdout flushing and case-insensitive user input handling.
344-348: Guard against infinite recursion on resume failures.If a server repeatedly returns non-206 responses (e.g., due to misconfigurations or changing ETags), line 347's recursive call will loop indefinitely. Track whether this is a retry attempt and fail explicitly on the second pass.
Apply this diff:
fn download_to_file( &self, path: &Path, resume_info: Option<ResumeInfo>, + is_retry: bool, ) -> Result<(), DownloadError> { let (resume_from, etag) = resume_info .as_ref() .map(|r| (Some(r.downloaded), r.etag.as_deref())) .unwrap_or((None, None)); let resp = Http::fetch(&self.url, resume_from, etag, self.ghcr_blob)?; let status = resp.status(); - if resume_from.is_some() && status != 206 { + if resume_from.is_some() && status != 206 && !is_retry { // Server doesn't support resume, start from the beginning - return self.download_to_file(path, None); + return self.download_to_file(path, None, true); + } else if resume_from.is_some() && status != 206 { + return Err(DownloadError::ResumeNotSupported); }And update the call site at line 270:
- self.download_to_file(&output_path, resume_info)?; + self.download_to_file(&output_path, resume_info, false)?;Likely an incorrect or invalid review comment.
soar-cli/src/download.rs (4)
45-65: LGTM!The
DownloadContexthelper methods correctly map CLI flags to the library'sOverwriteModeandFiltertypes, providing clean separation between CLI concerns and download logic.
92-210: LGTM with one note.The direct downloads handler correctly routes URLs through platform-specific parsers and falls back to package database lookups. The redundant
progress_callbackparameter issue noted above also applies here.
411-421: LGTM!The regex pattern creation now properly propagates errors instead of panicking, addressing the previous critical issue with
.unwrap().
423-482: LGTM!The GitHub/GitLab download handlers correctly parse optional
@tagsyntax and delegate to release handlers. The interactive asset selector provides clear feedback with size information and validates user input.
Summary by CodeRabbit
New Features
Chores