fix(github): resolve rename_exe search dir for archives with bin/ subdirectory#8358
fix(github): resolve rename_exe search dir for archives with bin/ subdirectory#8358
Conversation
Summary of ChangesHello @jdx, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where rename_exe fails for archives with a bin/ subdirectory by updating the search logic in both http.rs and static_helpers.rs. The changes are functional and resolve the described problem. My main feedback is to refactor the duplicated logic for determining the search directory into a shared helper function to enhance maintainability. I have provided specific suggestions for this refactoring in the comments.
src/backend/static_helpers.rs
Outdated
| let search_dir = if let Some(bin_path_template) = lookup_with_fallback(opts, "bin_path") { | ||
| let bin_path = template_string(&bin_path_template, tv); | ||
| install_path.join(&bin_path) | ||
| } else { | ||
| install_path.clone() | ||
| let bin_dir = install_path.join("bin"); | ||
| if bin_dir.is_dir() { | ||
| bin_dir | ||
| } else { | ||
| install_path.clone() | ||
| } | ||
| }; |
There was a problem hiding this comment.
This logic for determining the search_dir is also duplicated in src/backend/http.rs. To improve maintainability and avoid code duplication, consider extracting this logic into a new helper function within static_helpers.rs.
For example, you could add this function to static_helpers.rs:
pub fn determine_search_dir(
base_path: &Path,
opts: &ToolVersionOptions,
tv: &ToolVersion,
) -> PathBuf {
if let Some(bin_path_template) = lookup_with_fallback(opts, "bin_path") {
let bin_path = template_string(&bin_path_template, tv);
base_path.join(&bin_path)
} else {
let bin_dir = base_path.join("bin");
if bin_dir.is_dir() {
bin_dir
} else {
base_path.to_path_buf()
}
}
}Then you can replace this block with a call to the new function. I've left a suggestion to use it here, but you'll need to add the function definition yourself.
let search_dir = determine_search_dir(install_path, opts, tv);| let search_dir = if let Some(bin_path_template) = get_opt(opts, "bin_path") { | ||
| let bin_path = template_string(&bin_path_template, tv); | ||
| dest.join(&bin_path) | ||
| } else { | ||
| dest.to_path_buf() | ||
| let bin_dir = dest.join("bin"); | ||
| if bin_dir.is_dir() { | ||
| bin_dir | ||
| } else { | ||
| dest.to_path_buf() | ||
| } | ||
| }; |
There was a problem hiding this comment.
As mentioned in the review of src/backend/static_helpers.rs, this logic is duplicated. After creating the determine_search_dir helper function in static_helpers.rs, you can simplify this code as well by replacing this block with a call to the new function. Using the full path crate::backend::static_helpers::determine_search_dir avoids needing to add a use statement.
let search_dir = crate::backend::static_helpers::determine_search_dir(dest, opts, tv);
Greptile SummaryFixed
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: f58108b |
Hyperfine Performance
|
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.2.21 x -- echo |
18.4 ± 0.4 | 17.2 | 20.7 | 1.00 |
mise x -- echo |
18.7 ± 0.6 | 17.6 | 26.0 | 1.02 ± 0.04 |
mise env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.2.21 env |
18.0 ± 0.5 | 17.1 | 23.2 | 1.00 |
mise env |
18.4 ± 0.4 | 17.4 | 19.8 | 1.02 ± 0.04 |
mise hook-env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.2.21 hook-env |
18.5 ± 0.4 | 17.6 | 20.3 | 1.00 |
mise hook-env |
18.7 ± 0.4 | 17.7 | 20.7 | 1.01 ± 0.03 |
mise ls
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.2.21 ls |
18.1 ± 0.4 | 17.2 | 19.3 | 1.00 |
mise ls |
18.4 ± 0.4 | 17.3 | 20.3 | 1.02 ± 0.03 |
xtasks/test/perf
| Command | mise-2026.2.21 | mise | Variance |
|---|---|---|---|
| install (cached) | 116ms | 116ms | +0% |
| ls (cached) | 70ms | 71ms | -1% |
| bin-paths (cached) | 71ms | 71ms | +0% |
| task-ls (cached) | 695ms | 704ms | -1% |
…directory When an archive extracts to a bin/ subdirectory layout (e.g., prefix/bin/binary), rename_exe was silently skipped because it searched install_path/ non-recursively instead of install_path/bin/ where the binary actually lives after auto-stripping. Now auto-detects the bin/ subdirectory as the search directory for rename_exe only, matching the same logic used by discover_bin_paths() for PATH construction. The bin= option continues to use install_path since its values are already relative to it. Fixes #8354 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f58108b to
a9a6ee6
Compare
### 🚀 Features - add `--outdated` flag to `mise plugins ls` by @jdx in [#8360](#8360) ### 🐛 Bug Fixes - **(github)** resolve rename_exe search dir for archives with bin/ subdirectory by @jdx in [#8358](#8358) - **(install)** skip tools=true env directives during backend installation by @jdx in [#8356](#8356) - **(ruby)** resolve correct Windows checksums in lockfile by @jdx in [#8357](#8357) ### 📦 Registry - switch terradozer backend to github fork by @chenrui333 in [#8365](#8365) ### Chore - **(release)** fix duplicated version prefix in release title by @jdx in [#8359](#8359) ### New Contributors - @chenrui333 made their first contribution in [#8365](#8365) ## 📦 Aqua Registry Updates #### New Packages (1) - [`huseyinbabal/taws`](https://github.com/huseyinbabal/taws) #### Updated Packages (2) - [`block/goose`](https://github.com/block/goose) - [`pre-commit/pre-commit`](https://github.com/pre-commit/pre-commit)
…directory (jdx#8358) ## Summary - When an archive extracts to a `bin/` subdirectory layout (e.g., `prefix/bin/binary`), `rename_exe` was silently skipped because it searched `install_path/` non-recursively instead of `install_path/bin/` where the binary actually lives after auto-stripping - Now auto-detects the `bin/` subdirectory as the search directory for `rename_exe`, matching the same logic used by `discover_bin_paths()` for PATH construction - Fixes both the GitHub/GitLab/Forgejo backend (`static_helpers.rs`) and the HTTP backend (`http.rs`) Fixes jdx#8354 ## Test plan - [ ] Existing `e2e/backend/test_http_rename_exe` test passes (uses explicit `bin_path` + `strip_components`) - [ ] Manual test with `github:sourcemeta/jsonschema` using `rename_exe = "sm"` without explicit `bin_path` 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Low risk, localized change to post-extraction rename logic; main impact is that `rename_exe` will now rename binaries in `bin/` layouts where it previously did nothing. > > **Overview** > Fixes archive installs where `rename_exe` was applied in the wrong directory when the extracted archive places binaries under a top-level `bin/` folder. > > Both the HTTP backend (`http.rs`) and shared installer helper (`static_helpers.rs`) now auto-select `install_path/bin` (when it exists) as the search directory when `bin_path` is not explicitly set, aligning `rename_exe` behavior with PATH discovery. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit f58108b. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
### 🚀 Features - add `--outdated` flag to `mise plugins ls` by @jdx in [jdx#8360](jdx#8360) ### 🐛 Bug Fixes - **(github)** resolve rename_exe search dir for archives with bin/ subdirectory by @jdx in [jdx#8358](jdx#8358) - **(install)** skip tools=true env directives during backend installation by @jdx in [jdx#8356](jdx#8356) - **(ruby)** resolve correct Windows checksums in lockfile by @jdx in [jdx#8357](jdx#8357) ### 📦 Registry - switch terradozer backend to github fork by @chenrui333 in [jdx#8365](jdx#8365) ### Chore - **(release)** fix duplicated version prefix in release title by @jdx in [jdx#8359](jdx#8359) ### New Contributors - @chenrui333 made their first contribution in [jdx#8365](jdx#8365) ## 📦 Aqua Registry Updates #### New Packages (1) - [`huseyinbabal/taws`](https://github.com/huseyinbabal/taws) #### Updated Packages (2) - [`block/goose`](https://github.com/block/goose) - [`pre-commit/pre-commit`](https://github.com/pre-commit/pre-commit)
Summary
bin/subdirectory layout (e.g.,prefix/bin/binary),rename_exewas silently skipped because it searchedinstall_path/non-recursively instead ofinstall_path/bin/where the binary actually lives after auto-strippingbin/subdirectory as the search directory forrename_exe, matching the same logic used bydiscover_bin_paths()for PATH constructionstatic_helpers.rs) and the HTTP backend (http.rs)Fixes #8354
Test plan
e2e/backend/test_http_rename_exetest passes (uses explicitbin_path+strip_components)github:sourcemeta/jsonschemausingrename_exe = "sm"without explicitbin_path🤖 Generated with Claude Code
Note
Medium Risk
Touches install/extraction behavior in both the GitHub-style installer helper and the HTTP backend; incorrect directory selection could cause missed or wrong renames on some archive layouts.
Overview
Fixes archive installs where
rename_exepreviously searched only the extraction root and could silently miss binaries located under an extractedbin/directory after auto-stripping.Both the HTTP backend (
src/backend/http.rs) and shared archive install path (src/backend/static_helpers.rs) now preferinstall_path/binas therename_exesearch directory whenbin_pathis not explicitly configured (while keeping explicitbin_pathbehavior unchanged), and adds an e2e regression test (e2e/backend/test_github_rename_exe) covering this layout.Written by Cursor Bugbot for commit a9a6ee6. This will update automatically on new commits. Configure here.