feat: support binaries with arch or OS as extension#183
Merged
Conversation
- support additional case - commented out OS not supported atm - refactor code
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReworks asset selection scoring in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
pirafrank
changed the title
pirafrank
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/selector.rs (2)
167-175: Side effect inside.map()closure works but is non-idiomatic.The pattern of mutating
scoreinside the.map()closure works correctly sincefind()short-circuits, but it's unusual in Rust. Consider separating the match detection from score modification for clarity:♻️ Optional: More idiomatic approach
- let matching_os: Option<String> = - os_aliases - .iter() - .find(|alias| item.contains(*alias)) - .map(|alias| { - score += 5; - alias.to_string() - }); + let matching_os: Option<String> = os_aliases + .iter() + .find(|alias| item.contains(*alias)) + .map(|alias| alias.to_string()); + if matching_os.is_some() { + score += 5; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/selector.rs` around lines 167 - 175, The code mutates score inside the map closure on the iterator chain (matching_os creation), which is non-idiomatic; change it to first find the match without side effects (use os_aliases.iter().find(...) to set matching_os or found_os) and then, if matching_os.is_some() (or if found_os), increment score by 5 in a separate statement; update the variables referenced (matching_os, found_os, score, os_aliases, item) so side effects are not performed inside the closure and behavior remains the same.
201-219: Mutation insidefind()predicate is non-idiomatic and error-prone.Decrementing
num_aliasesinside thefind()predicate works but mixes iteration control with side effects. The logic prioritizes earlier aliases correctly, but the code is hard to follow. Consider usingenumerate()for clearer intent:♻️ Clearer approach using enumerate
- let mut num_aliases = arch_aliases.len(); - let matching_arch: Option<String> = arch_aliases + let matching_arch: Option<(usize, String)> = arch_aliases .iter() - .find(|alias| { - // We iterate over the aliases in reverse order to give more priority to - // the more specific aliases. - num_aliases -= 1; - item.contains(*alias) - }) - .map(|alias| { - // We also add a base 5 points bonus in case of a match, like for OS matching. - // Then we add the number of aliases remaining to the score to give more priority - // to the earlier aliases in the array. - // This is to avoid false positives for assets that have multiple options for - // the same architecture, e.g. armv7, armv6 and armhf when running on armv7. - score = score + 5 + (num_aliases as i32); - alias.to_string() - }); + .enumerate() + .find(|(_, alias)| item.contains(*alias)) + .map(|(idx, alias)| { + // Base 5 points + priority bonus for earlier aliases + let priority_bonus = (arch_aliases.len() - 1 - idx) as i32; + score += 5 + priority_bonus; + (idx, alias.to_string()) + }); + let matching_arch: Option<String> = matching_arch.map(|(_, s)| s); let found_arch: bool = matching_arch.is_some();
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/selector.rs`:
- Around line 167-175: The code mutates score inside the map closure on the
iterator chain (matching_os creation), which is non-idiomatic; change it to
first find the match without side effects (use os_aliases.iter().find(...) to
set matching_os or found_os) and then, if matching_os.is_some() (or if
found_os), increment score by 5 in a separate statement; update the variables
referenced (matching_os, found_os, score, os_aliases, item) so side effects are
not performed inside the closure and behavior remains the same.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/selector.rssrc/core/selector/tests/assets/vitor-mariano@regex-tui.ronsrc/core/selector/tests/selector_tests.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #183 +/- ##
==========================================
+ Coverage 89.64% 89.84% +0.20%
==========================================
Files 34 34
Lines 3187 3231 +44
==========================================
+ Hits 2857 2903 +46
+ Misses 330 328 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/selector/tests/assets.rs (1)
400-446: Strengthen the new extension tests with uniqueness checks.These tests validate content, but not that a single best asset is selected. Add
assert_eq!(binaries.len(), 1)to catch scoring ties/regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/selector/tests/assets.rs` around lines 400 - 446, Add assertions that exactly one asset is selected to each new test to prevent ties/regressions: after unwrapping the Option from get_triple_compatible_assets (in tests test_linux_x86_64_arch_label_as_extension, test_macos_arm64_arch_label_as_extension, test_linux_x86_64_os_label_as_extension, test_macos_arm64_os_label_as_extension) insert assert_eq!(binaries.len(), 1) before the contains check so you verify the length of the binaries Vec returned for the given AssetTriple and scoring logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/selector.rs`:
- Around line 167-170: The current alias matching uses raw substring checks
(matching_os via os_aliases.iter().find(|alias| item.contains(*alias))) which is
too permissive; change it to a boundary-aware match (e.g., match whole tokens or
use word-boundary/regex checks) so aliases only match when they are separate
tokens or adjacent to common separators (-, _, ., start/end), not when embedded
in unrelated words. Update the matching logic for os_aliases -> matching_os and
the analogous arch_aliases -> matching_arch checks (the other occurrence around
the arch matching code) to use the same token/boundary-aware matching approach,
and ensure you preserve the same alias-to-string mapping (alias.to_string())
after a successful bounded match.
- Around line 231-235: The current logic grants points for any filename without
a dot (variables item and score), which is too permissive; change the condition
so that no-dot filenames only get the bonus when they are likely real
executables: implement or call a helper (e.g., is_likely_executable(item)) that
rejects common metadata/checksum names ("sha", "sha256", "checksums",
"manifest", "license", "readme", etc.) and only returns true if the base name
matches the expected executable name pattern or passes executable heuristics;
then replace the three no-dot checks (the item.contains(".") branches around the
snippets using found_arch and matching_arch at the shown location and the
similar blocks at the other occurrences) to use that helper so non-binary
metadata files no longer receive the bonus.
---
Nitpick comments:
In `@src/core/selector/tests/assets.rs`:
- Around line 400-446: Add assertions that exactly one asset is selected to each
new test to prevent ties/regressions: after unwrapping the Option from
get_triple_compatible_assets (in tests
test_linux_x86_64_arch_label_as_extension,
test_macos_arm64_arch_label_as_extension,
test_linux_x86_64_os_label_as_extension, test_macos_arm64_os_label_as_extension)
insert assert_eq!(binaries.len(), 1) before the contains check so you verify the
length of the binaries Vec returned for the given AssetTriple and scoring logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 80a3b628-a5a1-4d34-a6ee-7e2710fd42d6
📒 Files selected for processing (6)
src/core/selector.rssrc/core/selector/tests/assets.rssrc/core/selector/tests/assets/fantauser@fantarepo_ends_in_os.ronsrc/core/selector/tests/assets/pirafrank@rust_exif_renamer.ronsrc/core/selector/tests/incompatible.rssrc/core/selector/tests/mod.rs
✅ Files skipped from review due to trivial changes (1)
- src/core/selector/tests/assets/fantauser@fantarepo_ends_in_os.ron
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/files/utils.rs (1)
116-119: Handle empty aliases explicitly.At Line 116, an empty
aliascan produce boundary-dependent matches throughmatch_indices(""). Add an early return to make behavior deterministic (falsefor empty alias).Proposed change
pub fn contains_alias_token(item: &str, alias: &str) -> bool { + if alias.is_empty() { + return false; + } let item = item.to_lowercase(); let alias = alias.to_lowercase(); item.match_indices(&alias).any(|(start, _)| {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/files/utils.rs` around lines 116 - 119, The contains_alias_token function should explicitly handle empty aliases to avoid match_indices("") behavior; add an early check in contains_alias_token (e.g., if alias.is_empty() return false) before performing the lowercase/substring checks so empty alias returns false deterministically, then proceed with the existing logic that lowercases item and alias and uses match_indices to test token boundaries.src/core/selector.rs (1)
305-326: Avoid duplicate lowercasing in hot scoring path.
get_triple_scorealready normalizesitemat Line 138, thenis_checksum_file(Line 306) andis_signature_file(Line 325) lowercase again. This adds avoidable allocations per asset.Proposed change
-fn is_checksum_file(item: &str) -> bool { - let item = item.to_lowercase(); +fn is_checksum_file(item: &str) -> bool { item == "checksum.txt" || item == "checksums.txt" || item.ends_with(".sha256") @@ -fn is_signature_file(item: &str) -> bool { - let item = item.to_lowercase(); +fn is_signature_file(item: &str) -> bool { item.ends_with(".asc") || item.ends_with(".sig") || item.ends_with(".pem")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/selector.rs` around lines 305 - 326, Summary: get_triple_score already normalizes item once, so remove duplicate lowercasing inside is_checksum_file and is_signature_file to avoid extra allocations; modify these functions to assume they receive an already lowercased &str (remove the to_lowercase() calls and operate directly on the &str) and update their callers (notably get_triple_score which normalizes item at Line 138) to pass the normalized string slice into is_checksum_file and is_signature_file. Ensure function signatures remain &str inputs and only one lowercase allocation happens in get_triple_score.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/selector.rs`:
- Around line 305-326: Summary: get_triple_score already normalizes item once,
so remove duplicate lowercasing inside is_checksum_file and is_signature_file to
avoid extra allocations; modify these functions to assume they receive an
already lowercased &str (remove the to_lowercase() calls and operate directly on
the &str) and update their callers (notably get_triple_score which normalizes
item at Line 138) to pass the normalized string slice into is_checksum_file and
is_signature_file. Ensure function signatures remain &str inputs and only one
lowercase allocation happens in get_triple_score.
In `@src/files/utils.rs`:
- Around line 116-119: The contains_alias_token function should explicitly
handle empty aliases to avoid match_indices("") behavior; add an early check in
contains_alias_token (e.g., if alias.is_empty() return false) before performing
the lowercase/substring checks so empty alias returns false deterministically,
then proceed with the existing logic that lowercases item and alias and uses
match_indices to test token boundaries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3afa4d1-ea71-43cb-8962-618e68deb0d6
📒 Files selected for processing (4)
src/core/selector.rssrc/files/utils.rssrc/files/utils/tests/mod.rssrc/files/utils/tests/test_contains_alias_token.rs
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/core/selector.rs (1)
10-21: Commented-out code for unsupported OSes is acceptable.Per retrieved learnings, Windows is not supported in this project. The BSD entries are also commented out, which is fine if poof doesn't target those platforms. Consider adding a brief comment explaining why these are disabled (e.g.,
// Currently unsupported platforms) for future maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/selector.rs` around lines 10 - 21, The commented-out OS entries in the lazy_static OPERATING_SYSTEM hashmap (symbol OPERATING_SYSTEM in selector.rs) lack an explanatory note; add a brief clarifying comment above or next to the commented entries (e.g., “// Currently unsupported platforms: windows, openbsd, freebsd, netbsd”) so future readers understand these platforms are intentionally disabled rather than accidentally left commented out. Ensure the comment references the disabled platforms by name and sits near the m.insert(...) lines for visibility.src/files/utils.rs (2)
237-246: Consider whether.pemis appropriate for signature detection.
.pemfiles are typically certificates or private keys, not signatures. Including them may cause false positives for legitimate PEM-encoded assets (e.g., CA bundles, TLS certs distributed alongside tools).Common signature-only extensions are:
.asc,.sig,.minisig,.pgp,.gpg.🔧 Suggested fix
pub fn is_signature_file(item: &str) -> bool { let item = item.to_lowercase(); item.ends_with(".asc") || item.ends_with(".sig") - || item.ends_with(".pem") || item.ends_with(".minisign") || item.ends_with(".pgp") || item.ends_with(".gpg") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/files/utils.rs` around lines 237 - 246, The is_signature_file function currently treats ".pem" as a signature which causes false positives; update the function to remove the ".pem" check and ensure the signature extensions list uses the correct common names (replace ".minisign" with ".minisig" if needed) — modify the checks in pub fn is_signature_file(item: &str) to only match known signature extensions (".asc", ".sig", ".minisig" or ".minisign" consistent with project conventions, ".pgp", ".gpg") and run/update any tests that assert signature detection behavior accordingly.
218-235: Consider adding additional checksum patterns.Some projects use different naming conventions for checksum files that aren't covered:
SHASUMS,SHA256SUMS,SHA512SUMS(common in Node.js releases).b2sum,.blake2(Blake2 checksums)- Files containing "digest" in the name
This is optional and can be deferred if current coverage meets project needs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/files/utils.rs` around lines 218 - 235, The is_checksum_file function currently misses several common patterns; update is_checksum_file to also detect names like "shasums", "sha256sums", "sha512sums" (accounting for the existing to_lowercase), add checks for Blake2 extensions (e.g., ends_with(".b2sum") and ends_with(".blake2")), and consider a contains("digest") check to catch files with "digest" in the name; modify the function's boolean expressions (or refactor to a small iterator over patterns) to include these new patterns while keeping the initial to_lowercase call and existing checks (function: is_checksum_file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/selector.rs`:
- Around line 10-21: The commented-out OS entries in the lazy_static
OPERATING_SYSTEM hashmap (symbol OPERATING_SYSTEM in selector.rs) lack an
explanatory note; add a brief clarifying comment above or next to the commented
entries (e.g., “// Currently unsupported platforms: windows, openbsd, freebsd,
netbsd”) so future readers understand these platforms are intentionally disabled
rather than accidentally left commented out. Ensure the comment references the
disabled platforms by name and sits near the m.insert(...) lines for visibility.
In `@src/files/utils.rs`:
- Around line 237-246: The is_signature_file function currently treats ".pem" as
a signature which causes false positives; update the function to remove the
".pem" check and ensure the signature extensions list uses the correct common
names (replace ".minisign" with ".minisig" if needed) — modify the checks in pub
fn is_signature_file(item: &str) to only match known signature extensions
(".asc", ".sig", ".minisig" or ".minisign" consistent with project conventions,
".pgp", ".gpg") and run/update any tests that assert signature detection
behavior accordingly.
- Around line 218-235: The is_checksum_file function currently misses several
common patterns; update is_checksum_file to also detect names like "shasums",
"sha256sums", "sha512sums" (accounting for the existing to_lowercase), add
checks for Blake2 extensions (e.g., ends_with(".b2sum") and
ends_with(".blake2")), and consider a contains("digest") check to catch files
with "digest" in the name; modify the function's boolean expressions (or
refactor to a small iterator over patterns) to include these new patterns while
keeping the initial to_lowercase call and existing checks (function:
is_checksum_file).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c933696-be50-4311-b716-50380885725f
📒 Files selected for processing (6)
src/core/selector.rssrc/files/utils.rssrc/files/utils/tests/mod.rssrc/files/utils/tests/test_contains_alias_token.rssrc/files/utils/tests/test_is_checksum_file.rssrc/files/utils/tests/test_is_signature_file.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/files/utils/tests/test_contains_alias_token.rs
- src/files/utils/tests/mod.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/selector/tests/assets.rs (2)
412-458: Strengthen single-match assertions to avoid false positives.In these tests, Line 420/432/444/456 unwrap and then only check
binaries[0]. Please also assertlen() == 1so regressions that return extra matches are caught.Proposed tightening
assert!(binaries.is_some()); let binaries = binaries.unwrap(); + assert_eq!(binaries.len(), 1); assert!(binaries[0].contains("regex-tui_v0.7.0_linux.amd64")); @@ assert!(binaries.is_some()); let binaries = binaries.unwrap(); + assert_eq!(binaries.len(), 1); assert!(binaries[0].contains("regex-tui_v0.7.0_darwin.arm64")); @@ assert!(binaries.is_some()); let binaries = binaries.unwrap(); + assert_eq!(binaries.len(), 1); assert!(binaries[0].contains("fantarepo_ends_in_os_v1.0.0_amd64.linux")); @@ assert!(binaries.is_some()); let binaries = binaries.unwrap(); + assert_eq!(binaries.len(), 1); assert!(binaries[0].contains("fantarepo_ends_in_os_v1.0.0_arm64.darwin"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/selector/tests/assets.rs` around lines 412 - 458, The tests calling get_triple_compatible_assets currently unwrap the Option and only assert on binaries[0], which allows extra matches to slip by; update each test (those using AssetTriple in test_linux_x86_64_arch_label_as_extension, test_macos_arm64_arch_label_as_extension, test_linux_x86_64_os_label_as_extension, test_macos_arm64_os_label_as_extension) to assert that binaries.len() == 1 immediately after unwrap() before checking binaries[0]. This ensures exactly one match is returned and prevents false positives from additional results.
460-482: Make multi-asset checks order-independent.Line 468-469 and Line 480-481 assume stable ordering. Since these tests validate membership, prefer order-agnostic assertions to reduce brittleness.
Order-agnostic variant
assert!(!binaries.is_empty() && binaries.len() == 2); - assert!(binaries[0].contains("kubectx_v0.9.5_linux_x86_64.tar.gz")); - assert!(binaries[1].contains("kubens_v0.9.5_linux_x86_64.tar.gz")); + assert!(binaries + .iter() + .any(|b| b.contains("kubectx_v0.9.5_linux_x86_64.tar.gz"))); + assert!(binaries + .iter() + .any(|b| b.contains("kubens_v0.9.5_linux_x86_64.tar.gz"))); @@ assert!(!binaries.is_empty() && binaries.len() == 2); - assert!(binaries[0].contains("kubectx_v0.9.5_linux_armv7.tar.gz")); - assert!(binaries[1].contains("kubens_v0.9.5_linux_armv7.tar.gz")); + assert!(binaries + .iter() + .any(|b| b.contains("kubectx_v0.9.5_linux_armv7.tar.gz"))); + assert!(binaries + .iter() + .any(|b| b.contains("kubens_v0.9.5_linux_armv7.tar.gz")));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/selector/tests/assets.rs` around lines 460 - 482, The tests test_linux_x86_64_multiple_assets and test_linux_armv7_multiple_assets rely on stable ordering of the returned Vec in get_triple_compatible_assets; change the assertions to be order-independent by asserting membership of both expected filenames rather than exact positions — e.g., unwrap binaries, ensure binaries.len() == 2 (or >=2 if appropriate), then check that the collection contains "kubectx_v0.9.5_linux_x86_64.tar.gz" and "kubens_v0.9.5_linux_x86_64.tar.gz" (and the armv7 variants) using contains or by converting to a HashSet and asserting both expected strings are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/selector/tests/assets.rs`:
- Around line 412-458: The tests calling get_triple_compatible_assets currently
unwrap the Option and only assert on binaries[0], which allows extra matches to
slip by; update each test (those using AssetTriple in
test_linux_x86_64_arch_label_as_extension,
test_macos_arm64_arch_label_as_extension,
test_linux_x86_64_os_label_as_extension, test_macos_arm64_os_label_as_extension)
to assert that binaries.len() == 1 immediately after unwrap() before checking
binaries[0]. This ensures exactly one match is returned and prevents false
positives from additional results.
- Around line 460-482: The tests test_linux_x86_64_multiple_assets and
test_linux_armv7_multiple_assets rely on stable ordering of the returned Vec in
get_triple_compatible_assets; change the assertions to be order-independent by
asserting membership of both expected filenames rather than exact positions —
e.g., unwrap binaries, ensure binaries.len() == 2 (or >=2 if appropriate), then
check that the collection contains "kubectx_v0.9.5_linux_x86_64.tar.gz" and
"kubens_v0.9.5_linux_x86_64.tar.gz" (and the armv7 variants) using contains or
by converting to a HashSet and asserting both expected strings are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1f3b99d-de44-4c96-bce7-9a5169919be0
📒 Files selected for processing (1)
src/core/selector/tests/assets.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changed
Description
This pull request refactors and improves the asset selection logic in
src/core/selector.rsto make binary matching more robust and accurate, especially for executables that use OS or architecture labels as file extensions. It also adds new tests and test assets to verify these improvements.Asset Selection Logic Improvements
get_triple_scoreto use early returns as deal-breakers if the current OS or architecture is not found in their respective hashmaps, making the logic more robust for unsupported platforms. [1] [2]is_linuxhelper for clarity and correctness.OPERATING_SYSTEMmap to focus matching on relevant platforms.Test Coverage
src/core/selector/tests/assets/vitor-mariano@regex-tui.roncontaining various asset filenames with OS and architecture labels as extensions.selector_tests.rsto verify correct matching for Linux x86_64 and macOS arm64 assets that use OS/arch labels as extensions.Summary by CodeRabbit