fix(env): uv venv in env_cache key to prevent cross directory leak#10217
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR fixes an env-cache bug where directories sharing identical configuration files would incorrectly inherit cached virtual environment state. The fix extends cache key computation to include ChangesEnv cache venv isolation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR fixes a cache-key collision in
Confidence Score: 5/5Safe to merge — the change is surgical (one new block in the key computation, one visibility change) and both failure scenarios are covered by tests. The cache-key change is minimal and correctly mirrors the existing mise.lock pattern. The condition guarding the new inputs (should_source()) is exactly the same condition used inside uv_venv() itself, so the key is always consistent with what the function would do. get_file_mtime handles directories correctly via std::fs::metadata, and the empty-vec path for non-venv directories ensures they get a distinct key without any extra bookkeeping. Both leak scenarios are exercised by the new e2e tests on Linux and Windows. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "Update src/toolset/toolset_env.rs" | Re-trigger Greptile |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
I don't have a Windows device, but it looks like it's been fixed. I'd appreciate it if someone could help me with some user testing. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR fixes env-cache key collisions when python.uv_venv_auto auto-sources a uv virtualenv, preventing VIRTUAL_ENV from leaking (or being suppressed) across sibling directories that share the same resolved config inputs.
Changes:
- Expose
uv_root()within the crate so other modules can key off the discovereduv.lockroot. - Include
uv.lock/.venv(and their mtimes) in the env cache key computation when uv auto-source is enabled. - Add Unix and Windows E2E regression tests covering both “venv leaks to sibling” and “sibling suppresses venv” scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/uv.rs | Makes uv_root() accessible to integrate uv discovery into other logic. |
| src/toolset/toolset_env.rs | Adds uv venv inputs into cache-key computation to prevent cross-directory cache collisions. |
| e2e/env/test_env_cache_venv | Adds Unix regression test asserting no uv venv leakage/suppression via env cache. |
| e2e-win/env_cache_venv.Tests.ps1 | Adds Windows regression test for the same env-cache/uv-venv interaction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub(crate) fn uv_root() -> Option<PathBuf> { | ||
| file::find_up(dirs::CWD.as_ref()?, &["uv.lock"]).map(|p| p.parent().unwrap().to_path_buf()) | ||
| } |
There was a problem hiding this comment.
find_up always returns a path ending in uv.lock, so parent() is never None
| @" | ||
| [settings] | ||
| python.uv_venv_auto = "source" | ||
| "@ | Out-File (Join-Path $script:TestRoot ".mise.toml") |
fix #4316
Problem
With
env_cacheenabled andpython.uv_venv_autoactive, a uv venv stays active after you leave the project directory:VIRTUAL_ENV(and the.venv/binentry onPATH) persists in unrelated directories. It leaks into a long-lived shell it propagates to every child process.Repro
Enable env caching with uv venv auto source (shell has
mise activate):A plain uv project:
uv.lock+.venv, no localmise.toml:VIRTUAL_ENV(andproj/.venv/binonPATH) stays active after leaving the project. Since it's exported, it then propagates to every process spawned from that shell, until the cache TTL expires.Disabling
env_cachemakes the venv deactivate correctly.Cause
compute_env_cache_key()derives the key from the resolved config files (plus siblingmise.lock), tool versions, settings and the basePATH, but not from theuv.lock/.venvthatpython.uv_venv_autodiscovers by walking upfrom the current directory (
uv_root()→find_up("uv.lock")).So two directories that resolve the same config files produce the same cache key even when only one of them contains a venv. On a cache hit mise returns the stored env and skips the
uv_venvstep entirely, so whichever directorycomputed first wins for both:
proj/(caches an env with the venv) →cdto a sibling → cache hit→
VIRTUAL_ENVleaks in;cd proj/→ cache hit → venv never activates.
cd/chpwdis not a cache-invalidation trigger, so the stale entry survives.Fix
Mirroring theexisting
mise.lockhandling, so venv and non-venv directories no longer collide on one key. Add the uv venv (uv.lock+.venv) to theenv_cachekey.Test
Add e2e test.
Summary by CodeRabbit
Bug Fixes
Tests