Repository navigation
fix(which): respect prefer_offline so binary lookup avoids remote fetches - #10595
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds Changesprefer_offline extension for
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes
Confidence Score: 5/5Safe to merge — the change is a single-line addition to a well-understood allow-list with no effect on any code path other than mise which offline resolution. The fix is minimal and isolated: one entry added to the prefer-offline command list, a matching docs update, and a well-constructed regression test that would have caught the original bug and guards against future regressions. The test structure mirrors the established pattern from test_hook_env_no_remote_fetch and correctly covers the fuzzy-version specifiers that trigger date-aware resolution. No files require special attention. Important Files Changed
Reviews (4): Last reviewed commit: "fix(which): respect prefer_offline so bi..." | Re-trigger Greptile |
b710d0d to
eb691de
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
e2e/cli/test_which_prefer_offline (1)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using a unique temporary path for the marker file.
The marker uses a fixed path in
$HOMEthat could theoretically conflict with other tests if they use the same name or if test isolation is imperfect. While e2e frameworks typically isolate$HOMEper test, using a unique temporary location (e.g.,mktempor$PWD/marker-$$) would be more defensive.Alternative approach
-marker="$HOME/remote-versions-fetched" +marker="$(pwd)/.remote-versions-fetched-$$"Or use a temp file:
-marker="$HOME/remote-versions-fetched" +marker="$(mktemp)"🤖 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 `@e2e/cli/test_which_prefer_offline` at line 20, The marker file path at line 20 uses a fixed location in $HOME which could cause test conflicts or isolation issues. Replace the fixed path marker="$HOME/remote-versions-fetched" with a unique temporary path by either using mktemp to generate a temporary file path, or by constructing a path with the process ID (using $$) in the current working directory (e.g., $PWD/marker-$$). Update all references to the marker variable throughout the test to use this new unique path consistently.
🤖 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 `@e2e/cli/test_which_prefer_offline`:
- Around line 22-31: The plugin scripts created with `cat >` for `list-all` and
`latest-stable` in the spy plugin directory are not executable by default, which
will cause permission errors when Mise tries to run them. After each `cat >`
command that creates these scripts, add a `chmod +x` command to set the
executable bit on the newly created files so they can be properly executed
during the test.
- Line 19: The comment at line 19 incorrectly states that cat > preserves the
executable bit from copied scripts. In reality, cat > creates files with default
permissions (typically 644) that are not executable. Correct this comment to
accurately describe that cat > does not preserve executable permissions, and
that if the scripts need to be executable, an explicit chmod +x would be
required instead.
---
Nitpick comments:
In `@e2e/cli/test_which_prefer_offline`:
- Line 20: The marker file path at line 20 uses a fixed location in $HOME which
could cause test conflicts or isolation issues. Replace the fixed path
marker="$HOME/remote-versions-fetched" with a unique temporary path by either
using mktemp to generate a temporary file path, or by constructing a path with
the process ID (using $$) in the current working directory (e.g.,
$PWD/marker-$$). Update all references to the marker variable throughout the
test to use this new unique path consistently.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 12d3a133-0e2f-4986-9c72-419f3322ad28
📒 Files selected for processing (3)
e2e/cli/test_which_prefer_offlinesettings.tomlsrc/env.rs
✅ Files skipped from review due to trivial changes (2)
- src/env.rs
- settings.toml
…ches `mise which` is a binary lookup and should not require HTTP calls when an installed version already satisfies the request. Add `which` to the prefer-offline auto-enable command list (alongside `where`, `ls`, `exec`, etc.) so version resolution prefers cached/installed data instead of fetching remote version lists -- e.g. when an explicit `minimum_release_age` cutoff is configured, which otherwise forces date-aware remote resolution. Adds a network-free e2e regression test using a marker-spy plugin. Resolves jdx#10460 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
eb691de to
c16229c
Compare
Summary
mise whichis a binary lookup, but it was missing from the prefer-offline auto-enable command list, so version resolution could make remote HTTP calls even when an installed version already satisfies the request. This addresses discussion #10460 ("a binary lookup should not require HTTP calls").Root cause
The list of commands that auto-enable
prefer_offlinelives insrc/env.rs(prefer_offline()): it containedactivate, current, direnv, env, exec, hook-env, ls, where, xbut notwhich. As a result,mise which <bin>resolved withprefer_offline = false. When an explicitminimum_release_agecutoff is configured,filters_installed_versions()is true (tool_version.rs), which forces date-aware remote resolution for non-prefer-offline commands even when an installed version matches.Fix
"which"to the prefer-offline command list insrc/env.rs.prefer_offlinedocs string insettings.tomlto mentionwhich. (The JSON schema only embeds thedescriptionfield, not thedocsprose, so no schema regeneration is needed.)Testing
e2e/cli/test_which_prefer_offline, modeled ontest_hook_env_no_remote_fetch: it copies the dummy plugin into aspyplugin whoselist-all/latest-stabletouch a marker file, setsMISE_MINIMUM_RELEASE_AGE=24h, and assertsmise which dummyresolves the installedspy@1.0.0without recreating the marker (i.e. without a remote fetch). This fails before the fix and passes after.shellcheck -xpasses on the new test.Scope note
The fix covers config-file tools (the reported scenario). An explicit
--tool=spy@1argument is aToolSource::Argumentrequest, which intentionally opts out of prefer-offline so explicit version requests resolve accurately (see #10571), consistent withmise exec/mise x. That path is deliberately left unchanged and is documented as out of scope in the test.Note
Local builds/tests were not run in the authoring sandbox (insufficient memory); compilation and e2e are left to CI.
🤖 Generated with Claude Code
Summary by CodeRabbit
mise whichfully honorsprefer_offline, resolving installed versions without triggering remote version lookups when offline preference is enabled.prefer_offlinedocumentation to listwhichalongside other commands that avoid remote version fetching.mise whichremains offline for config-file-based tool resolution (including minimum release age modes) when an installed version already satisfies the requested version.