fix: Vfox Plugin Auto-Installation for Environment Directives#8035
fix: Vfox Plugin Auto-Installation for Environment Directives#8035
Conversation
Summary of ChangesHello @pose, 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 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 fixes a timing issue where vfox plugins referenced in the [env] section would fail on a clean installation because they weren't installed yet. The solution involves passing the config object down to mise_env and mise_path to ensure plugins are installed before use. The changes are logical and well-described. I've added a couple of suggestions to improve test coverage and code maintainability.
e2e/env/test_env_module_auto_install
Outdated
| # Test that env modules work correctly with vfox plugins when referenced | ||
| # in the [env] section, creating the plugin directory beforehand to simulate | ||
| # a plugin that's already been "installed" by other means. | ||
| # This tests the scenario where mise loads env directives with vfox plugins. |
There was a problem hiding this comment.
This test is a great start, but it doesn't fully cover the auto-installation scenario described in the issue. The test creates the plugin directory manually, which simulates a pre-existing plugin. The original bug occurred on a clean installation where the plugin directory did not exist.
To more accurately test the fix, consider adding a test case that:
- Initializes a local git repository for a mock plugin.
- Adds the plugin to
[plugins]inmise.tomlusing the local git repository URL. - Ensures the plugin directory does not exist in
$MISE_DATA_DIR/plugins. - Runs
mise envand verifies that the plugin is automatically cloned and the environment is set up correctly.
There was a problem hiding this comment.
I wasn't able to add that test since I couldn't find how to specify a local plugin in the mise.toml configuration. I tried using the file: scheme, but it was not recognizing it. I also tried using a map and changing the url of the map when using the http backend to a file, but that didn't fly either (mise.toml schema expects [plugins] entries to be strings). The following e2e test reproduces the issue but it requires a remote git repository URL which is more brittle:
#!/usr/bin/env bash
export MISE_TRUSTED_CONFIG_PATHS=""
AUTO_PLUGIN_DIR="$MISE_DATA_DIR/plugins/vfox-pulumi"
cat >mise.toml <<EOF
[env]
_.vfox-pulumi = { module_path = "." }
[tools]
"vfox-pulumi:pulumi/pulumictl" = '0.0.50'
[plugins]
vfox-pulumi = "https://github.com/pulumi/vfox-pulumi"
EOF
cat > "go.mod" <<EOF
module sample
toolchain go 1.23.4
EOF
OUTPUT=$(env mise trust && mise env -s bash 2>&1)
EXIT_CODE=$?
if [ $EXIT_CODE -ne 0 ]; then
echo "FAILURE: mise env failed with exit code $EXIT_CODE"
echo "Output: $OUTPUT"
exit 1
fi
if [ ! -d "$AUTO_PLUGIN_DIR" ]; then
echo "FAILURE: Expected plugin directory to be auto-installed"
echo "Expected directory: $AUTO_PLUGIN_DIR"
exit 1
fi
# # Verify the env variable was set correctly
if echo "$OUTPUT" | grep -q "GO_VERSION_MISE=1.23.4"; then
echo "SUCCESS: Plugin env module executed correctly"
else
echo "FAILURE: Expected GO_VERSION_MISE=1.23.4 in output"
echo "Output: $OUTPUT"
exit 1
fiI verified that in main that script reproduces the "Plugin directory not found" error, whereas using the code from this PR, it retrieves and installs the plugin successfully.
0af4690 to
5fe0839
Compare
jdx
left a comment
There was a problem hiding this comment.
Code review — the fix is correct in principle (auto-install vfox plugins before env hooks run). A few issues:
1. Double ensure_installed per module invocation
module() calls mise_env() then mise_path() sequentially on the same plugin. Both now call ensure_installed_for_env_hook(). The second call is a no-op (the directory exists by then), but it still creates a MultiProgressReport, checks config for repo URLs, etc. Consider either:
- Calling
ensure_installedonce inmodule()before both calls, or - Caching the installed state on the
VfoxPlugininstance
2. ensure_installed_for_env_hook is unnecessary indirection
async fn ensure_installed_for_env_hook(&self, config: &Arc<Config>) -> Result<()> {
self.ensure_installed(config, &MultiProgressReport::get(), false, false).await
}This is a one-liner wrapper that hardcodes MultiProgressReport::get(). It doesn't add clarity — just inline the ensure_installed call at the two call sites (or better, once in module() as suggested above).
3. E2e test doesn't test auto-install
The test name is test_env_module_auto_install but it manually creates the plugin directory with mkdir -p and populates it. It never tests the actual auto-install path (cloning from a URL when the directory doesn't exist). It's testing "env modules work with a pre-existing plugin," which is what the existing tests already cover. To test the fix, the test should define the plugin in [plugins] with a URL and not pre-create the directory.
4. E2e test style
The test uses manual if/grep/echo patterns instead of the project's assert / assert_contains helpers from e2e/assert.sh. See other e2e tests for the expected pattern.
5. Minor: PR description is very verbose
The PR body includes full function signatures, explanations of the entire call chain, and copies of the code. A concise summary of the problem + fix is sufficient — reviewers can read the diff.
Overall the core fix (passing config through and calling ensure_installed) is the right approach. The main suggestion is to call ensure_installed once in module() rather than inside each vfox method, and to fix the e2e test to actually exercise the auto-install codepath.
This review was generated by AI (Claude).
jdx
left a comment
There was a problem hiding this comment.
Code review — the fix is correct in principle (auto-install vfox plugins before env hooks run). A few issues:
1. Double ensure_installed per module invocation
module() calls mise_env() then mise_path() sequentially on the same plugin. Both now call ensure_installed_for_env_hook(). The second call is a no-op (the directory exists by then), but it still creates a MultiProgressReport, checks config for repo URLs, etc. Consider either:
- Calling
ensure_installedonce inmodule()before both calls, or - Caching the installed state on the
VfoxPlugininstance
2. ensure_installed_for_env_hook is unnecessary indirection
async fn ensure_installed_for_env_hook(&self, config: &Arc<Config>) -> Result<()> {
self.ensure_installed(config, &MultiProgressReport::get(), false, false).await
}This is a one-liner wrapper that hardcodes MultiProgressReport::get(). It doesn't add clarity — just inline the ensure_installed call at the two call sites (or better, once in module() as suggested above).
3. E2e test doesn't test auto-install
The test name is test_env_module_auto_install but it manually creates the plugin directory with mkdir -p and populates it. It never tests the actual auto-install path (cloning from a URL when the directory doesn't exist). It's testing "env modules work with a pre-existing plugin," which is what the existing tests already cover. To test the fix, the test should define the plugin in [plugins] with a URL and not pre-create the directory.
4. E2e test style
The test uses manual if/grep/echo patterns instead of the project's assert / assert_contains helpers from e2e/assert.sh. See other e2e tests for the expected pattern.
This review was generated by AI (Claude).
|
Re: point #3 — for the e2e test, you could use https://github.com/jdx/mise-env-sample as a real vfox plugin to test the auto-install path. Define it in This comment was generated by AI (Claude). |
|
good enhancement @pose! great to hear from you as well! |
4f157b9 to
db5a29e
Compare
Thanks! Great to hear from you too :) Addressed PR Feedback✅ 1. Double ensure_installed per module invocation The double-ensure issue has been addressed while preserving call-site safety and layering:
✅ 3. E2e test doesn't test auto-install: Used jdx/mise-env-sample as a real vfox plugin to test the auto-install path. Verified test effectively fails in |
jdx
left a comment
There was a problem hiding this comment.
This review is AI-generated.
Overall this looks good — real bug fix for a real problem on clean setups. Two notes:
1. env_module_ensured cache doesn't help here
VfoxPlugin is constructed fresh every time module() is called (module.rs:20):
let plugin = VfoxPlugin::new(name, path.clone());So the AsyncMutex<bool> will always start as false — the per-instance cache never survives between calls. Within a single module() call, ensure_installed gets called twice (once for mise_env, once for mise_path), but since ensure_installed already short-circuits when self.plugin_path.exists(), the second call is cheap anyway.
I'd suggest either:
- Calling
ensure_installedonce inmodule()before bothmise_env/mise_pathcalls, or - Dropping the
env_module_ensuredfield entirely and callingensure_installeddirectly in each method
Both are simpler and avoid the unused tokio::sync::Mutex dependency.
2. E2E test file mode
The new test file is 100755 (executable). Convention in this repo is that e2e tests are not chmod'd executable.
cfdba72 to
6398d85
Compare
|
Addressed, thanks for the review. Changes made
This keeps install orchestration in one place ( Note: this response was AI-generated. |
6398d85 to
2d18cf3
Compare
## Issue On a clean setup, env modules backed by vfox can fail during config loading with: `Plugin directory not found ...` This happens because `[env]` module evaluation can run before the plugin directory exists. ## Fix - Pass `config` into `VfoxPlugin::mise_env()` / `VfoxPlugin::mise_path()` so installation can be ensured at hook-execution time. - Ensure the plugin is installed before running vfox env/path hooks. - Avoid duplicate ensure overhead when `module()` calls `mise_env()` then `mise_path()`: - ensure state is cached per `VfoxPlugin` instance - second call returns early instead of repeating setup work - Keep call sites simple: `module.rs` does not need an explicit pre-init call. ## Why this approach - Prevents the clean-install failure path. - Preserves API ergonomics (no “remember to initialize first” requirement). - Keeps progress/reporting concerns in the plugin layer rather than `config/env_directive`. ## Testing - Added: `e2e/env/test_env_module_auto_install` - Verifiev-module behavior and existing related e2e coverage: - `e2e/env/test_env_module_cmd_exec_tools` - other env e2e tests via the normal suite
Ensure vfox plugins are installed once in env module resolution, remove ineffective per-instance install caching, and normalize the new e2e test file mode to match repository conventions. Co-authored-by: Cursor <cursoragent@cursor.com>
2d18cf3 to
f33d701
Compare
### 🚀 Features - **(activate)** add shims directory as fallback when auto-install is enabled by @ctaintor in [#8106](#8106) - **(env)** add `tools` variable to tera template context by @jdx in [#8108](#8108) - **(set)** add --stdin flag for multiline environment variables by @jdx in [#8110](#8110) ### 🐛 Bug Fixes - **(backend)** improve conda patchelf and dependency resolution for complex packages by @jdx in [#8087](#8087) - **(ci)** fix validate-new-tools grep pattern for test field by @jdx in [#8100](#8100) - **(config)** make MISE_OFFLINE work correctly by gracefully skipping network calls by @jdx in [#8109](#8109) - **(github)** skip v prefix for "latest" version by @jdx in [#8105](#8105) - **(gitlab)** resolve tool options from config for aliased tools by @jdx in [#8084](#8084) - **(install)** use version_expr for Flutter to fix version resolution by @jdx in [#8081](#8081) - **(registry)** add Linux support for tuist by @fortmarek in [#8102](#8102) - **(release)** write release notes to file instead of capturing stdout by @jdx in [#8086](#8086) - **(upgrade)** tools are not uninstalled properly due to outdated symlink by @roele in [#8099](#8099) - **(upgrade)** ensure uninstallation failure does not leave invalid symlinks by @roele in [#8101](#8101) - SLSA for in-toto statement with no signatures by @gerhard in [#8094](#8094) - Vfox Plugin Auto-Installation for Environment Directives by @pose in [#8035](#8035) ### 📚 Documentation - use mise activate for PowerShell in getting-started by @rileychh in [#8112](#8112) ### 📦 Registry - add conda backend for mysql by @jdx in [#8080](#8080) - add conda backends for 10 asdf-only tools by @jdx in [#8083](#8083) - added podman-tui by @tony-sol in [#8098](#8098) ### Chore - sort settings.toml alphabetically and add test by @jdx in [#8111](#8111) ### New Contributors - @ctaintor made their first contribution in [#8106](#8106) - @rileychh made their first contribution in [#8112](#8112) - @fortmarek made their first contribution in [#8102](#8102) - @pose made their first contribution in [#8035](#8035) - @gerhard made their first contribution in [#8094](#8094) ## 📦 Aqua Registry Updates #### New Packages (2) - [`entireio/cli`](https://github.com/entireio/cli) - [`rmitchellscott/reManager`](https://github.com/rmitchellscott/reManager) #### Updated Packages (1) - [`atuinsh/atuin`](https://github.com/atuinsh/atuin)
### 🚀 Features - **(activate)** add shims directory as fallback when auto-install is enabled by @ctaintor in [#8106](#8106) - **(env)** add `tools` variable to tera template context by @jdx in [#8108](#8108) - **(set)** add --stdin flag for multiline environment variables by @jdx in [#8110](#8110) ### 🐛 Bug Fixes - **(backend)** improve conda patchelf and dependency resolution for complex packages by @jdx in [#8087](#8087) - **(ci)** fix validate-new-tools grep pattern for test field by @jdx in [#8100](#8100) - **(config)** make MISE_OFFLINE work correctly by gracefully skipping network calls by @jdx in [#8109](#8109) - **(github)** skip v prefix for "latest" version by @jdx in [#8105](#8105) - **(gitlab)** resolve tool options from config for aliased tools by @jdx in [#8084](#8084) - **(install)** use version_expr for Flutter to fix version resolution by @jdx in [#8081](#8081) - **(registry)** add Linux support for tuist by @fortmarek in [#8102](#8102) - **(release)** write release notes to file instead of capturing stdout by @jdx in [#8086](#8086) - **(upgrade)** tools are not uninstalled properly due to outdated symlink by @roele in [#8099](#8099) - **(upgrade)** ensure uninstallation failure does not leave invalid symlinks by @roele in [#8101](#8101) - SLSA for in-toto statement with no signatures by @gerhard in [#8094](#8094) - Vfox Plugin Auto-Installation for Environment Directives by @pose in [#8035](#8035) ### 📚 Documentation - use mise activate for PowerShell in getting-started by @rileychh in [#8112](#8112) ### 📦 Registry - add conda backend for mysql by @jdx in [#8080](#8080) - add conda backends for 10 asdf-only tools by @jdx in [#8083](#8083) - added podman-tui by @tony-sol in [#8098](#8098) ### Chore - sort settings.toml alphabetically and add test by @jdx in [#8111](#8111) ### New Contributors - @ctaintor made their first contribution in [#8106](#8106) - @rileychh made their first contribution in [#8112](#8112) - @fortmarek made their first contribution in [#8102](#8102) - @pose made their first contribution in [#8035](#8035) - @gerhard made their first contribution in [#8094](#8094) ## 📦 Aqua Registry Updates #### New Packages (2) - [`entireio/cli`](https://github.com/entireio/cli) - [`rmitchellscott/reManager`](https://github.com/rmitchellscott/reManager) #### Updated Packages (1) - [`atuinsh/atuin`](https://github.com/atuinsh/atuin)
## Issue On a clean setup, env modules backed by vfox can fail during config loading with: `Plugin directory not found ...` This happens because `[env]` module evaluation can run before the plugin directory exists. ## Fix - Ensure plugin installation once in `EnvResults::module()` before invoking vfox hooks. - Keep `module()` flow as: - `ensure_installed(...)` - `mise_env(...)` - `mise_path(...)` - Remove ineffective per-instance install cache from `VfoxPlugin`: - dropped `env_module_ensured` - removed `ensure_installed_for_env_module` wrapper - Simplify vfox hook methods: - `VfoxPlugin::mise_env()` / `mise_path()` no longer take `config` - no install side effects inside these methods - Normalize new e2e file mode to repo convention (`100644`, non-executable). ## Why this approach - Fixes clean-install failures by guaranteeing plugin availability before hook execution. - Avoids duplicate install checks within one module resolution without introducing unnecessary caching state. - Keeps hook methods focused on hook execution and moves orchestration to module resolution. ## Testing - Added: `e2e/env/test_env_module_auto_install` (uses real plugin URL and exercises auto-install path). - Verified with: - `mise run test:e2e e2e/env/test_env_module_auto_install` - `mise run lint` --------- Co-authored-by: Cursor <cursoragent@cursor.com>
### 🚀 Features - **(activate)** add shims directory as fallback when auto-install is enabled by @ctaintor in [jdx#8106](jdx#8106) - **(env)** add `tools` variable to tera template context by @jdx in [jdx#8108](jdx#8108) - **(set)** add --stdin flag for multiline environment variables by @jdx in [jdx#8110](jdx#8110) ### 🐛 Bug Fixes - **(backend)** improve conda patchelf and dependency resolution for complex packages by @jdx in [jdx#8087](jdx#8087) - **(ci)** fix validate-new-tools grep pattern for test field by @jdx in [jdx#8100](jdx#8100) - **(config)** make MISE_OFFLINE work correctly by gracefully skipping network calls by @jdx in [jdx#8109](jdx#8109) - **(github)** skip v prefix for "latest" version by @jdx in [jdx#8105](jdx#8105) - **(gitlab)** resolve tool options from config for aliased tools by @jdx in [jdx#8084](jdx#8084) - **(install)** use version_expr for Flutter to fix version resolution by @jdx in [jdx#8081](jdx#8081) - **(registry)** add Linux support for tuist by @fortmarek in [jdx#8102](jdx#8102) - **(release)** write release notes to file instead of capturing stdout by @jdx in [jdx#8086](jdx#8086) - **(upgrade)** tools are not uninstalled properly due to outdated symlink by @roele in [jdx#8099](jdx#8099) - **(upgrade)** ensure uninstallation failure does not leave invalid symlinks by @roele in [jdx#8101](jdx#8101) - SLSA for in-toto statement with no signatures by @gerhard in [jdx#8094](jdx#8094) - Vfox Plugin Auto-Installation for Environment Directives by @pose in [jdx#8035](jdx#8035) ### 📚 Documentation - use mise activate for PowerShell in getting-started by @rileychh in [jdx#8112](jdx#8112) ### 📦 Registry - add conda backend for mysql by @jdx in [jdx#8080](jdx#8080) - add conda backends for 10 asdf-only tools by @jdx in [jdx#8083](jdx#8083) - added podman-tui by @tony-sol in [jdx#8098](jdx#8098) ### Chore - sort settings.toml alphabetically and add test by @jdx in [jdx#8111](jdx#8111) ### New Contributors - @ctaintor made their first contribution in [jdx#8106](jdx#8106) - @rileychh made their first contribution in [jdx#8112](jdx#8112) - @fortmarek made their first contribution in [jdx#8102](jdx#8102) - @pose made their first contribution in [jdx#8035](jdx#8035) - @gerhard made their first contribution in [jdx#8094](jdx#8094) ## 📦 Aqua Registry Updates #### New Packages (2) - [`entireio/cli`](https://github.com/entireio/cli) - [`rmitchellscott/reManager`](https://github.com/rmitchellscott/reManager) #### Updated Packages (1) - [`atuinsh/atuin`](https://github.com/atuinsh/atuin)
### 🚀 Features - **(activate)** add shims directory as fallback when auto-install is enabled by @ctaintor in [jdx#8106](jdx#8106) - **(env)** add `tools` variable to tera template context by @jdx in [jdx#8108](jdx#8108) - **(set)** add --stdin flag for multiline environment variables by @jdx in [jdx#8110](jdx#8110) ### 🐛 Bug Fixes - **(backend)** improve conda patchelf and dependency resolution for complex packages by @jdx in [jdx#8087](jdx#8087) - **(ci)** fix validate-new-tools grep pattern for test field by @jdx in [jdx#8100](jdx#8100) - **(config)** make MISE_OFFLINE work correctly by gracefully skipping network calls by @jdx in [jdx#8109](jdx#8109) - **(github)** skip v prefix for "latest" version by @jdx in [jdx#8105](jdx#8105) - **(gitlab)** resolve tool options from config for aliased tools by @jdx in [jdx#8084](jdx#8084) - **(install)** use version_expr for Flutter to fix version resolution by @jdx in [jdx#8081](jdx#8081) - **(registry)** add Linux support for tuist by @fortmarek in [jdx#8102](jdx#8102) - **(release)** write release notes to file instead of capturing stdout by @jdx in [jdx#8086](jdx#8086) - **(upgrade)** tools are not uninstalled properly due to outdated symlink by @roele in [jdx#8099](jdx#8099) - **(upgrade)** ensure uninstallation failure does not leave invalid symlinks by @roele in [jdx#8101](jdx#8101) - SLSA for in-toto statement with no signatures by @gerhard in [jdx#8094](jdx#8094) - Vfox Plugin Auto-Installation for Environment Directives by @pose in [jdx#8035](jdx#8035) ### 📚 Documentation - use mise activate for PowerShell in getting-started by @rileychh in [jdx#8112](jdx#8112) ### 📦 Registry - add conda backend for mysql by @jdx in [jdx#8080](jdx#8080) - add conda backends for 10 asdf-only tools by @jdx in [jdx#8083](jdx#8083) - added podman-tui by @tony-sol in [jdx#8098](jdx#8098) ### Chore - sort settings.toml alphabetically and add test by @jdx in [jdx#8111](jdx#8111) ### New Contributors - @ctaintor made their first contribution in [jdx#8106](jdx#8106) - @rileychh made their first contribution in [jdx#8112](jdx#8112) - @fortmarek made their first contribution in [jdx#8102](jdx#8102) - @pose made their first contribution in [jdx#8035](jdx#8035) - @gerhard made their first contribution in [jdx#8094](jdx#8094) ## 📦 Aqua Registry Updates #### New Packages (2) - [`entireio/cli`](https://github.com/entireio/cli) - [`rmitchellscott/reManager`](https://github.com/rmitchellscott/reManager) #### Updated Packages (1) - [`atuinsh/atuin`](https://github.com/atuinsh/atuin)
Issue
On a clean setup, env modules backed by vfox can fail during config loading with:
Plugin directory not found ...This happens because
[env]module evaluation can run before the plugin directory exists.Fix
EnvResults::module()before invoking vfox hooks.module()flow as:ensure_installed(...)mise_env(...)mise_path(...)VfoxPlugin:env_module_ensuredensure_installed_for_env_modulewrapperVfoxPlugin::mise_env()/mise_path()no longer takeconfig100644, non-executable).Why this approach
Testing
e2e/env/test_env_module_auto_install(uses real plugin URL and exercises auto-install path).mise run test:e2e e2e/env/test_env_module_auto_installmise run lint