fix(aqua): strip aqua asset formats from AssetWithoutExt#10104
fix(aqua): strip aqua asset formats from AssetWithoutExt#10104risu729 wants to merge 12 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors asset format detection and extension stripping in the aqua-registry crate, and updates the Windows extension completion logic in the aqua backend to ignore version numbers when detecting existing extensions. This prevents issues like duplicate extensions on files containing version dots. Feedback on these changes highlights opportunities to optimize string allocations in asset_without_ext, improve robustness in path_ext_ignoring_version by operating only on the filename rather than the entire path, and remove the redundant aqua_file_src helper function.
Greptile SummaryThis PR fixes the
Confidence Score: 5/5Safe to merge — the change is well-scoped, all extraction paths are covered by the new dispatch logic, and new tests directly exercise the previously broken zst strip case. The core fix (replacing the hard-coded suffix chain with AQUA_ASSET_FORMATS iteration) is straightforward and verified by unit tests. The ArchiveFormat rename is mechanical and consistent across all callers. The new format=="raw" branch correctly handles raw-format packages that the old code would have errored on. No pre-existing invariants are broken. No files require special attention. Important Files Changed
Reviews (16): Last reviewed commit: "fix(aqua-registry): document unimplement..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
risu729
force-pushed
the
fix/aqua-windows-compressed-bins
branch
2 times, most recently
from
a47734e to
eef1558
Compare
risu729
force-pushed
the
fix/aqua-windows-compressed-bins
branch
4 times, most recently
from
29619af to
d375c28
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR refactors asset format and archive extension handling in aqua-registry by introducing shared helper functions that centralize format detection and normalize known archive shorthand formats, then refactoring existing callers to use these helpers. It includes unit tests validating the new behavior and Windows E2E tests for binary installation. ChangesAsset Format and Extension Handling Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This comment was marked as outdated.
This comment was marked as outdated.
risu729
force-pushed
the
fix/aqua-windows-compressed-bins
branch
from
972b5ef to
ca30edf
Compare
risu729
changed the title
risu729
force-pushed
the
fix/aqua-windows-compressed-bins
branch
from
ca30edf to
f37e519
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aqua-registry/src/types.rs`:
- Around line 1428-1448: The test
test_remove_ext_from_asset_uses_aqua_asset_formats is missing assertions for the
shorthand tar-based suffixes; update that test to add assertions calling
remove_ext_from_asset for filenames ending with .tbr, .tlz4, and .tsz and assert
they normalize to ("name", "tar.br"), ("name", "tar.lz4"), and ("name",
"tar.zst") respectively (e.g., "tool.tbr" -> ("tool","tar.br"), "tool.tlz4" ->
("tool","tar.lz4"), "tool.tsz" -> ("tool","tar.zst")); keep the test name and
placement and follow the same assert_eq! style as the other cases so the
normalization behavior is covered.
- Around line 891-898: normalize_asset_format currently maps
"tgz","txz","tbz","tbz2" but misses other tar-based shorthands present in
AQUA_ASSET_FORMATS; update the normalize_asset_format function to also map "tbr"
-> "tar.br", "tlz4" -> "tar.lz4", and "tsz" -> "tar.sz" so downstream checks
like format.starts_with("tar") (used in install logic) correctly route these
archives to the untar handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7afc5978-0a73-4902-85d7-6cb6c06db3f0
📒 Files selected for processing (2)
crates/aqua-registry/src/types.rse2e-win/backend/aqua_bin_path.Tests.ps1
| fn normalize_asset_format(format: &'static str) -> &'static str { | ||
| match format { | ||
| "tgz" => "tar.gz", | ||
| "txz" => "tar.xz", | ||
| "tbz2" | "tbz" => "tar.bz2", | ||
| _ => format, | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing normalizations for tar-based shorthand formats.
The function normalizes tgz, txz, tbz, and tbz2 but does not normalize tbr, tlz4, or tsz to their expanded forms (tar.br, tar.lz4, tar.sz). Since these shorthand formats are explicitly included in AQUA_ASSET_FORMATS (line 869), files with these extensions will be detected but not normalized.
This will cause runtime failures in the installer: the aqua backend's install logic checks format.starts_with("tar") to route tar-based archives to the untar handler, but unnormalized formats like "tbr" won't match and will fall through to bail!("unsupported format: {}", format).
🔧 Proposed fix to normalize all tar-based shorthands
fn normalize_asset_format(format: &'static str) -> &'static str {
match format {
"tgz" => "tar.gz",
"txz" => "tar.xz",
"tbz2" | "tbz" => "tar.bz2",
+ "tbr" => "tar.br",
+ "tlz4" => "tar.lz4",
+ "tsz" => "tar.sz",
_ => format,
}
}Based on learnings from context snippet 1 (src/backend/aqua.rs:2242-2353), which shows the installer branches on format.starts_with("tar") for tar extraction.
📝 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 normalize_asset_format(format: &'static str) -> &'static str { | |
| match format { | |
| "tgz" => "tar.gz", | |
| "txz" => "tar.xz", | |
| "tbz2" | "tbz" => "tar.bz2", | |
| _ => format, | |
| } | |
| } | |
| fn normalize_asset_format(format: &'static str) -> &'static str { | |
| match format { | |
| "tgz" => "tar.gz", | |
| "txz" => "tar.xz", | |
| "tbz2" | "tbz" => "tar.bz2", | |
| "tbr" => "tar.br", | |
| "tlz4" => "tar.lz4", | |
| "tsz" => "tar.sz", | |
| _ => format, | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/aqua-registry/src/types.rs` around lines 891 - 898,
normalize_asset_format currently maps "tgz","txz","tbz","tbz2" but misses other
tar-based shorthands present in AQUA_ASSET_FORMATS; update the
normalize_asset_format function to also map "tbr" -> "tar.br", "tlz4" ->
"tar.lz4", and "tsz" -> "tar.sz" so downstream checks like
format.starts_with("tar") (used in install logic) correctly route these archives
to the untar handler.
| #[test] | ||
| fn test_remove_ext_from_asset_uses_aqua_asset_formats() { | ||
| assert_eq!( | ||
| remove_ext_from_asset("tfcmt_linux_amd64.tar.gz"), | ||
| ("tfcmt_linux_amd64", "tar.gz") | ||
| ); | ||
| assert_eq!( | ||
| remove_ext_from_asset("tfcmt_linux_amd64.tgz"), | ||
| ("tfcmt_linux_amd64", "tar.gz") | ||
| ); | ||
| assert_eq!(remove_ext_from_asset("tool.tar.br"), ("tool", "tar.br")); | ||
| assert_eq!( | ||
| remove_ext_from_asset("codex-x86_64-pc-windows-msvc.exe.zst"), | ||
| ("codex-x86_64-pc-windows-msvc.exe", "zst") | ||
| ); | ||
| assert_eq!(remove_ext_from_asset("tfcmt.js"), ("tfcmt.js", "raw")); | ||
| assert_eq!( | ||
| remove_ext_from_asset("tfcmt_windows_amd64.exe"), | ||
| ("tfcmt_windows_amd64.exe", "raw") | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add test coverage for shorthand format normalization.
The test verifies that remove_ext_from_asset strips known extensions and handles the common .tgz → "tar.gz" normalization, but does not cover the newly supported tar-based shorthands (.tbr, .tlz4, .tsz). Adding assertions for these formats will prevent regressions if the normalization logic is updated.
🧪 Suggested test additions
assert_eq!(
remove_ext_from_asset("tfcmt_linux_amd64.tgz"),
("tfcmt_linux_amd64", "tar.gz")
);
+ assert_eq!(
+ remove_ext_from_asset("tool.tbr"),
+ ("tool", "tar.br")
+ );
+ assert_eq!(
+ remove_ext_from_asset("tool.tlz4"),
+ ("tool", "tar.lz4")
+ );
+ assert_eq!(
+ remove_ext_from_asset("tool.tsz"),
+ ("tool", "tar.sz")
+ );
assert_eq!(remove_ext_from_asset("tool.tar.br"), ("tool", "tar.br"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/aqua-registry/src/types.rs` around lines 1428 - 1448, The test
test_remove_ext_from_asset_uses_aqua_asset_formats is missing assertions for the
shorthand tar-based suffixes; update that test to add assertions calling
remove_ext_from_asset for filenames ending with .tbr, .tlz4, and .tsz and assert
they normalize to ("name", "tar.br"), ("name", "tar.lz4"), and ("name",
"tar.zst") respectively (e.g., "tool.tbr" -> ("tool","tar.br"), "tool.tlz4" ->
("tool","tar.lz4"), "tool.tsz" -> ("tool","tar.zst")); keep the test name and
placement and follow the same assert_eq! style as the other cases so the
normalization behavior is covered.
|
This PR currently has merge conflicts. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
risu729
force-pushed
the
fix/aqua-windows-compressed-bins
branch
from
f37e519 to
1470420
Compare
risu729
force-pushed
the
fix/aqua-windows-compressed-bins
branch
from
1470420 to
44e4dbb
Compare
risu729
force-pushed
the
fix/aqua-windows-compressed-bins
branch
from
6163968 to
93cda74
Compare
TarFormat::from_ext handles tgz/tbz/txz aliases at extraction time.
risu729
force-pushed
the
fix/aqua-windows-compressed-bins
branch
from
93cda74 to
ccda558
Compare
risu729
force-pushed
the
fix/aqua-windows-compressed-bins
branch
from
ccda558 to
fcc0062
Compare
Call TarFormat::from_ext and unarchive directly from the aqua backend.
risu729
force-pushed
the
fix/aqua-windows-compressed-bins
branch
from
fcc0062 to
9a1dc3b
Compare
risu729
force-pushed
the
fix/aqua-windows-compressed-bins
branch
from
9a1dc3b to
8b206a2
Compare
- rename TarFormat to ArchiveFormat; from_ext returns Option - replace unarchive/ArchiveOptions with extract_archive/ExtractOptions - route aqua compressed assets through decompress_file directly
risu729
force-pushed
the
fix/aqua-windows-compressed-bins
branch
from
8b206a2 to
a9e6b11
Compare
risu729
force-pushed
the
fix/aqua-windows-compressed-bins
branch
from
a9e6b11 to
4dcbca2
Compare
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 2 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 3 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 4 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 5 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks and merge conflicts. If this continues for 7 days, it will be closed automatically. This is warning day 6 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
1 participant
Summary
{{.AssetWithoutExt}}with aqua's asset-format detector instead of a shorter hard-coded suffix strip listdetect_formatviaAQUA_ASSET_FORMATS/remove_ext_from_assetDepends on #10241 for archive extraction API changes (
extract_archive/decompress_file,ArchiveFormat::from_ext). This PR only fixes template suffix stripping; compressed Windows installs rely on #10241 routing.zstassets throughdecompress_fileto the registry bin path.Context
This addresses the remaining
AssetWithoutExtcompressed-asset behavior from GitHub Discussions #10057 / #10064.The reporter in #10064 confirmed
aqua:evilmartians/lefthookis fixed inv2026.6.0. That release includes #10167, which fixed the Windows extension/version-dot path needed for the.gzlefthook install. This PR is still needed becausev2026.6.0did not changeAquaFile::template_ctxto strip all aqua archive/compression suffixes from{{.AssetWithoutExt}}: it strips.gz, but not.zst.This deliberately does not use the version-aware executable extension detector added by #10167.
AssetWithoutExtneeds aqua archive/compression format semantics, not generic path-extension detection.Suffixes
Before this PR,
AssetWithoutExtstripped only a short hard-coded list. This PR uses the full aqua asset-format set (including.zst,.tar.br,.dmg, etc.).The
openai/codexaqua package renderscodex-x86_64-pc-windows-msvc.exe.zstand uses{{.AssetWithoutExt}}as the file source. Without this PR, mise looks for the.zstpath after extraction instead ofcodex-x86_64-pc-windows-msvc.exe.Tests
cargo test -p aqua-registry test_remove_ext_from_asset_uses_aqua_asset_formatscargo test -p aqua-registry test_aqua_file_src_asset_without_ext_strips_zste2e-win/backend/aqua_bin_path.Tests.ps1