fix(aqua): mirror aqua windows extension semantics#10167
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 introduces support for custom Windows extensions, conditional extension appending, and improved Windows extension completion logic, along with corresponding unit tests. The review feedback highlights three important opportunities to prevent bugs when parsing file extensions from paths containing version numbers. Specifically, the reviewer recommends updating complete_windows_dst_ext to avoid treating trailing version components (like .0) as file extensions, and modifying both os_file_ext_is_empty and path_ext_is_empty to perform version string replacements only on the filename component rather than the entire path or URL, preventing unintended side effects and path corruption.
Greptile SummaryThis PR refines aqua Windows extension handling to faithfully mirror upstream aqua semantics:
Confidence Score: 5/5Safe to merge — the change is well-tested and additive; all existing tests are adapted and a comprehensive new suite verifies the new paths. The implementation consistently mirrors documented upstream aqua behavior. No files require special attention. Important Files Changed
Reviews (5): Last reviewed commit: "fix(aqua): bump compiled registry cache ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
risu729
force-pushed
the
codex-20260531-172732-ad2d98
branch
from
b270194 to
d97f215
Compare
risu729
changed the title
risu729
force-pushed
the
codex-20260531-172732-ad2d98
branch
from
d97f215 to
5dcea01
Compare
📝 WalkthroughWalkthroughThis PR refactors Windows extension handling in the aqua registry and backend from fixed boolean configuration to optional, package-driven behavior. A new file extension utility module extracts "real" extensions while respecting version substrings. The AquaPackage type now supports optional Windows extension settings with type-based defaults, and backend helpers use this configuration to intelligently complete filenames. The cache version is bumped to invalidate old entries. ChangesWindows Extension Handling Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
risu729
force-pushed
the
codex-20260531-172732-ad2d98
branch
from
5dcea01 to
ef0691a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 615-628: In complete_windows_ext_to_asset, when handling the raw
format (the branch if self.format == "raw"), avoid unconditionally calling
complete_windows_ext(s) because that can double-append a custom Windows
extension (e.g., tool.bat -> tool.bat.bat); instead, first check whether the
rendered name already has a file extension (reuse os_file_ext_is_empty(s, v))
and only call complete_windows_ext(s) if os_file_ext_is_empty(s, v) returns
true; update the raw branch to mirror the non-raw logic that guards
complete_windows_ext behind os_file_ext_is_empty.
🪄 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: 54bc7e95-28cb-499f-b72e-312a323219ce
📒 Files selected for processing (5)
crates/aqua-registry/src/cache.rscrates/aqua-registry/src/file_ext.rscrates/aqua-registry/src/lib.rscrates/aqua-registry/src/types.rssrc/backend/aqua.rs
| fn complete_windows_ext_to_asset(&self, s: &str, v: &str, os: &str) -> Result<String> { | ||
| if os != "windows" || s.ends_with(".exe") || s.ends_with(".jar") { | ||
| return Ok(s.to_string()); | ||
| } | ||
| if self.format == "raw" { | ||
| return Ok(self.complete_windows_ext(s)); | ||
| } | ||
| if !self.format.is_empty() { | ||
| return Ok(s.to_string()); | ||
| } | ||
| if self.os_file_ext_is_empty(s, v) { | ||
| return Ok(self.complete_windows_ext(s)); | ||
| } | ||
| Ok(s.to_string()) |
There was a problem hiding this comment.
Don't double-append custom Windows extensions for raw assets/URLs.
When format == "raw", this branch appends windows_ext() without first checking whether the rendered name already has a real extension. A package that sets windows_ext: ".bat" and already renders tool.bat will become tool.bat.bat, which breaks Windows asset/url resolution.
Proposed fix
fn complete_windows_ext_to_asset(&self, s: &str, v: &str, os: &str) -> Result<String> {
if os != "windows" || s.ends_with(".exe") || s.ends_with(".jar") {
return Ok(s.to_string());
}
if self.format == "raw" {
- return Ok(self.complete_windows_ext(s));
+ return Ok(if self.os_file_ext_is_empty(s, v) {
+ self.complete_windows_ext(s)
+ } else {
+ s.to_string()
+ });
}
if !self.format.is_empty() {
return Ok(s.to_string());
}
if self.os_file_ext_is_empty(s, v) {🤖 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 615 - 628, In
complete_windows_ext_to_asset, when handling the raw format (the branch if
self.format == "raw"), avoid unconditionally calling complete_windows_ext(s)
because that can double-append a custom Windows extension (e.g., tool.bat ->
tool.bat.bat); instead, first check whether the rendered name already has a file
extension (reuse os_file_ext_is_empty(s, v)) and only call
complete_windows_ext(s) if os_file_ext_is_empty(s, v) returns true; update the
raw branch to mirror the non-raw logic that guards complete_windows_ext behind
os_file_ext_is_empty.
2 participants
Summary
complete_windows_extas tri-state instead of a defaulted booleanwindows_extandappend_extwhen rendering aqua assets, URLs, and Windows install links.exeand.jarassets as already complete, preserve existing extensions whenformatis omitted, and keep version dots from suppressing Windows completionaqua-registrycrate so the backend delegates aqua semantics instead of duplicating themAqua behavior checked
Current aqua source and upstream history checked:
windowsExt,completeWindowsExtToAsset, andcompleteWindowsExtToFileSrc,osfile.Ext, andappendExt.append_extdefaults to true. Aqua appends the configuredformatto rendered asset/URL strings when the string does not already look like an archive/compressed asset andformatis neither empty norraw;append_ext: falsedisables that behavior.complete_windows_extis tri-state. Explicittruecompletes Windows asset/URL/file source names, explicitfalseleaves them alone, and an omitted value completes by default except forgithub_contentandgithub_archivepackages.windows_extcontrols the suffix used by Windows completion. Aqua defaults to.exe, but defaults to.shforgithub_contentandgithub_archive; registry entries can override it, such as.bat..exeis hardcoded upstream as the Windows executable suffix. It started with aqua's Windows support work in aquaproj/aqua#858 for assets and aquaproj/aqua#874 for URLs/files, both under the broader Windows support request aquaproj/aqua#850..jaris also hardcoded upstream as already complete. aquaproj/aqua#4719 changedcompleteWindowsExtToAssetfrom only skipping.exeto skipping.exeor.jar, with the commit note "skip appending .exe on Windows" for JARs.formatis omitted, aqua does not blindly append a Windows extension. aquaproj/aqua#874 added the URL fallback that completes only whenfilepath.Ext(s) == "", and aquaproj/aqua#1853 added the omitted-{{.Format}}asset flow. The linked feature request aquaproj/aqua#1774 says the goal is simpler config when formats change. mise mirrors the current aqua implementation by checking the version-stripped filename extension first: existing extensions like.ps1are preserved while version dots liketool.1.0.0do not suppress completion.mise support
AquaPackagenow preservescomplete_windows_ext: Option<bool>,windows_ext, andappend_extfrom registry YAML and version overrides.append_extbefore Windows completion.windows_ext,.jarpreservation,github_content/github_archivedefaults, and version-stripped extension detection..exe.aqua-registrycrate now owns the version-aware extension detector plus install file source/destination completion helpers; the backend only adapts those string rules toPathBuffile names.Foundry / cosign note
Foundry's aqua-registry entry already declares both GitHub Artifact Attestations and a cosign Sigstore bundle at
{{.AssetWithoutExt}}.sigstore.json. mise already checks cosign bundles through the existing aqua cosign verification path; this PR is about rendering the asset names andAssetWithoutExtinputs consistently with aqua.Related mise reports
Tests
cargo test -p aqua-registrycargo test -p mise windows_extSummary by CodeRabbit
New Features
Improvements