fix(config): don't write the global config into a conf.d drop-in - #11633
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change prevents global configuration writes from targeting ChangesGlobal configuration selection
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 SummaryThe PR prevents implicit global configuration writes from selecting a
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "fix(config): don't write the global conf..." | Re-trigger Greptile |
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/config/mod.rs`:
- Around line 2228-2232: Filter out conf.d drop-in paths from the global
configuration file collection before invoking first_config_file, so its
files.first() fallback can only select eligible files such as .tool-versions.
Preserve the existing fallback behavior and add an end-to-end case covering both
a drop-in and .tool-versions, verifying that .tool-versions remains unchanged.
🪄 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: fb2f08d5-7174-42b3-a901-05c353290ab1
📒 Files selected for processing (2)
e2e/cli/test_global_config_confdsrc/config/mod.rs
88ed12d to
ef7eae2
Compare
|
The scenario does not occur, but the test it asks for is worth having, so I added that.
Measured on v2026.8.0 — i.e. without this patch — with a drop-in and # default (MISE_USE_TOML is true, so ~/.tool-versions is not a candidate)
$ mise use -g --dry-run jq@1.7.1
mise would update ~/.config/mise/conf.d/10-drop-in.toml # ← the bug this PR fixes
# MISE_USE_TOML=false, so both are candidates
$ MISE_USE_TOML=false mise use -g --dry-run jq@1.7.1
mise would update ~/.tool-versions # ← not the drop-inThe second line is the case the comment describes, and the fallback already returns What the finding did surface is that the fix leans on an ordering invariant declared ~65 lines away, with nothing guarding it. If those two inserts were ever swapped, printf 'dummy 1.0.0\n' >"$HOME/.tool-versions"
MISE_USE_TOML=false mise use -g dummy@system
assert_contains "cat $HOME/.tool-versions" "dummy"
assert_not_contains "cat $HOME/.config/mise/conf.d/10-drop-in.toml" "dummy"
assert_fail "test -f $HOME/.config/mise/config.toml"It runs before the rest of the file creates I kept the filter where it is rather than pre-filtering the set: both produce identical results in every case above, and putting it on the return value keeps the intent local to the function that documents itself as "the preferred global config file to write to". Happy to switch to pre-filtering if a reviewer prefers not depending on the insert order at all. |
Reported in #5842: with
~/.config/mise/config.tomlnot yet created andconf.d/*.tomlpresent,mise use --globaledits a drop-in instead.Measured on v2026.7.15, config dir holding only
conf.d/10-a.toml:Cause
first_config_filedeliberately skips conf.d entries while choosing, but falls back tofiles.first()when nothing else qualifies — andconfig_files_from_dirinserts conf.d entries beforeCONFIG_FILENAMES. So a config dir holding only drop-ins hands one straight back toglobal_config_path, whose own doc comment reads "the preferred global config file to write to, or the path where it should be created".That also explains the reporter's observation that it works correctly once conf.d is empty.
Change
One
.filteringlobal_config_path, so an unqualified candidate falls through toMISE_GLOBAL_CONFIG_FILEand then to<config dir>/config.toml— the namemise use --helpalready documents for--global.first_config_fileitself is left alone: itsfiles.first()fallback is what makes a.tool-versions-only global config work, and other callers rely on it. Only the global write-target resolution needed narrowing.What else this touches
All nine callers of
global_config_path()are write-target resolution, so they are all fixed by the same change and none of them wanted a drop-in:Config::global_config()— the base for settings writesresolve_target_config_path— the--globalbranch and the in-$HOMEbranchcli/unuse.rs,cli/edit.rs,cli/settings/set.rs,cli/settings/unset.rsUnchanged: reading. Drop-ins are still discovered and loaded exactly as before — the filter only decides where writes go. Also unchanged: a
.tool-versionsglobal config (reachable withMISE_USE_TOML=false), which is not a conf.d file and passes the filter.If
<config dir>/mise.tomlalready exists it is still preferred over creatingconfig.toml, sincefirst_config_filefinds it before the fallback is reached.Tests
e2e/cli/test_global_config_confd, modelled on the existingtest_global_config_file_home: with only a drop-in present,mise use -g/mise set -g/mise settings setall write toconfig.toml, the drop-in is byte-for-byte untouched, and it is still loaded before and after — the reading path is the thing most worth guarding here.No unit test:
config/mod.rs'smod testsis#[cfg(unix)]and snapshot-based, andglobal_config_files()readsdirs::CONFIGthrough aMutex-cached static that a test cannot vary.Note
@jdx said in the thread that this was "probably easier said than done" — it turned out to be one predicate, because the intent was already encoded in
first_config_file; only the fallback leaked past it.Summary by CodeRabbit
config.tomlfile instead ofconf.ddrop-in files.