fix(vfox): apply tools=true env to os.execute install hooks (#10282)#10432
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds Changestools=true env propagation into vfox hooks
Sequence DiagramsequenceDiagram
rect rgba(100, 180, 255, 0.5)
Note over mise install,VfoxBackend: Install-time env resolution
participant mise install
participant VfoxBackend
participant tool_val_env as Toolset::tool_val_env
participant load_post_env
mise install->>VfoxBackend: install_version_(ctx)
VfoxBackend->>tool_val_env: ctx.ts.tool_val_env(config, base_env)
tool_val_env->>load_post_env: ToolsFilter::ToolsOnlyVals
load_post_env-->>tool_val_env: KEY→value pairs
tool_val_env-->>VfoxBackend: EnvMap (no PATH)
VfoxBackend->>VfoxBackend: merge into cmd_env
end
rect rgba(180, 100, 255, 0.5)
Note over VfoxBackend,Shell: Hook execution via os.execute
participant os_execute as Lua os.execute (Rust)
participant apply_mise_env
participant Shell
VfoxBackend->>os_execute: PostInstall hook calls os.execute(cmd)
os_execute->>apply_mise_env: apply_mise_env(lua, &mut cmd)
apply_mise_env-->>os_execute: sanitized env applied
os_execute->>Shell: spawn with mise_env
Shell-->>os_execute: exit code
os_execute-->>VfoxBackend: hook result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend/mod.rs (1)
2332-2343: ⚡ Quick winAdd diagnostics when full toolset resolution is skipped.
If
config.get_toolset().awaitfails,dependency_envsilently omitstools=truevalue injection, which makes this path hard to debug.Suggested tweak
- if let Ok(ts) = config.get_toolset().await { - match ts.tool_val_env(config, &env).await { - Ok(vals) => { - for (k, v) in vals { - if k.as_str() != env::PATH_KEY.as_str() { - env.insert(k, v); - } - } - } - Err(e) => debug!("dependency_env: skipping tools=true value directives: {e:#}"), - } - } + match config.get_toolset().await { + Ok(ts) => match ts.tool_val_env(config, &env).await { + Ok(vals) => { + for (k, v) in vals { + if k.as_str() != env::PATH_KEY.as_str() { + env.insert(k, v); + } + } + } + Err(e) => debug!("dependency_env: skipping tools=true value directives: {e:#}"), + }, + Err(e) => debug!("dependency_env: failed to load full toolset for tools=true values: {e:#}"), + }🤖 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 `@src/backend/mod.rs` around lines 2332 - 2343, The code silently skips tools=true value injection when config.get_toolset().await fails without logging any diagnostic information, making debugging difficult. Add error handling for the failed case of config.get_toolset().await by adding an if let Err(e) clause alongside the existing if let Ok(ts) clause to log a debug message with the actual error details when toolset resolution fails, similar to how the error case is already handled inside the match expression for ts.tool_val_env.
🤖 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.
Nitpick comments:
In `@src/backend/mod.rs`:
- Around line 2332-2343: The code silently skips tools=true value injection when
config.get_toolset().await fails without logging any diagnostic information,
making debugging difficult. Add error handling for the failed case of
config.get_toolset().await by adding an if let Err(e) clause alongside the
existing if let Ok(ts) clause to log a debug message with the actual error
details when toolset resolution fails, similar to how the error case is already
handled inside the match expression for ts.tool_val_env.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 44e60f24-efa2-45d9-8c45-00881afdf8ba
📒 Files selected for processing (5)
crates/vfox/src/lua_mod/cmd.rse2e/backend/test_vfox_install_tools_envsrc/backend/mod.rssrc/config/env_directive/mod.rssrc/toolset/toolset_env.rs
Greptile SummaryFixes a combined
Confidence Score: 5/5Safe to merge. Changes are tightly scoped to vfox install hooks and activate only when The two-part fix is internally consistent: the No files require special attention. Important Files Changed
Reviews (5): Last reviewed commit: "fix(vfox): apply tools=true env to os.ex..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
JamBalaya56562
force-pushed
the
fix-vfox-os-execute-tools-env
branch
3 times, most recently
from
734e27e to
956f573
Compare
|
Pushed a fix for the e2e failures (they were a real deadlock introduced by this PR, not flaky CI). Root cause: the first revision injected the Fix: Verified locally on Windows with an isolated single vfox plugin: |
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 `@src/backend/vfox.rs`:
- Line 165: The comparison `k.as_str() != crate::env::PATH_KEY.as_str()` is
case-sensitive, but on Windows environment variables are case-insensitive. This
means if a user defines `path` (lowercase) in their config, it will not be
properly excluded like `PATH` would be. Replace the case-sensitive string
comparison with case-insensitive comparison using `.eq_ignore_ascii_case()`
method to ensure all variants of the PATH environment variable name are
correctly excluded regardless of case, particularly on Windows systems.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 21bfc10b-f822-453b-9597-6775774ce590
📒 Files selected for processing (5)
crates/vfox/src/lua_mod/cmd.rse2e/backend/test_vfox_install_tools_envsrc/backend/vfox.rssrc/config/env_directive/mod.rssrc/toolset/toolset_env.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/config/env_directive/mod.rs
- src/toolset/toolset_env.rs
- e2e/backend/test_vfox_install_tools_env
- crates/vfox/src/lua_mod/cmd.rs
When `mise install` installs several tools at once, a `tools = true` `[env]`
var that depends on another tool — e.g.
`CLOUDSDK_PYTHON = "{{ tools.python.path }}/bin/python3"` for a gcloud + python
setup — did not reach the dependent tool's install. Two gaps caused it:
1. vfox plugins that shell out via Lua `os.execute` (e.g. vfox-gcloud's
install.sh) inherited mise's raw process env, ignoring the sanitized
`mise_env` that cmd.exec applies. In a combined install that raw env still
carried the *stale* value rendered before the dependency was installed.
2. `dependency_env` resolved with `full_env_without_tools`, so `tools = true`
`[env]` values were never computed for the install at all.
Fix:
- crates/vfox: route Lua `os.execute` through `mise_env` (env_clear + mise_env),
matching cmd.exec; falls back to stock behavior when `mise_env` is unset, so
only mise-managed installs are affected.
- Resolve `tools = true` *value* directives against the full (installed) toolset
and merge them into `dependency_env` (best-effort). Env *modules* are skipped
via the new `ToolsFilter::ToolsOnlyVals` so modules are never run on a partial
toolset, and any resolution error falls back to the tool-less env.
Separate installs worked only because a re-activated shell re-exported the
resolved value between commands; this makes the combined install behave the same.
Tests: a vfox os.execute unit test, a ToolsOnlyVals filter unit test, and an
e2e (e2e/backend/test_vfox_install_tools_env) where a vfox plugin's PostInstall
reads a tools=true var via os.execute.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JamBalaya56562
force-pushed
the
fix-vfox-os-execute-tools-env
branch
from
956f573 to
b089cdb
Compare
2 participants
Summary
Fixes #10282. When
mise installinstalls several tools at once, atools = true[env]var that depends on another tool — e.g. a gcloud + python setup:— did not reach the dependent tool's install. Installing separately (
mise install pythonthenmise install gcloud) worked, but a combinedmise installfailed.Root cause
Two interacting gaps:
os.executebypasses the sanitized env. vfox plugins that shell out via Luaos.execute(e.g. vfox-gcloud'sinstall.sh) inherit mise's raw process env, ignoring themise_envregistry thatcmd.execapplies. During a combined install that raw env still carried the staleCLOUDSDK_PYTHONrendered before python was installed ({{ tools.python.path }}was empty →/bin/python3), soinstall.shused a nonexistent python and failed.tools = truevalues were never computed for the install.dependency_envresolves withfull_env_without_tools, sotools = true[env]values weren't applied to the install environment at all.The separate-install case only worked because the (activated) shell re-exported the now-correct
CLOUDSDK_PYTHONbetween the two commands — something a single combinedmise installprocess never does.Fix
os.executethroughmise_env(env_clear+mise_env), matchingcmd.exec. Factored the env application into a sharedapply_mise_envhelper. Whenmise_envis unset (i.e. not a mise-managed install)os.executebehaves exactly as stock, so the blast radius is limited to installs.ToolsFilter::ToolsOnlyValsselectstools = truevalue directives only.Toolset::tool_val_envresolves those against the full (installed) toolset anddependency_envmerges them in best-effort (PATH untouched; errors fall back to the tool-less env). Env modules (PythonVenv/Module/Source/File/Path) are deliberately skipped so modules are never run on a partial toolset.The full toolset (not the dependency toolset) is used for the value resolution because
dependsordering installs the dependency first, butdependency_toolsetonly tracks backend/registry-declared deps, not userdepends.Tests
crates/vfoxunit test:os.executehonorsmise_env.env_directiveunit test:ToolsOnlyValsincludestools = trueVals and excludestools = falsevars and modules.e2e/backend/test_vfox_install_tools_env: a local vfox plugin whosePostInstallreads atools = truevar viaos.execute(network-free, deterministic).Notes / scope
os.executeis rerouted;io.popen(file-handle semantics) is left as a follow-up. vfox-gcloud usesos.execute.tools = truevalue (hereCLOUDSDK_PYTHON, an absolute path). Putting a user-dependstool's bin onPATHduring install is a separate concern (dependency_toolsetdoesn't track userdepends) and not required for this report.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
os.executeso shell execution uses the same sanitizedmise_envenvironment as command execution.tools = truevalue directives are resolved, andPATHupdates are intentionally avoided.Tests
os.executehonorsmise_env, includingenv_clearbehavior for shell resolution.tools = trueenv vars are readable inside pluginPostInstallhooks.