fix: stop resolve() from leaking foreign envs (e.g. hatch) into the venv collection#1507
Open
StellaHuang95 wants to merge 1 commit intomicrosoft:mainfrom
Open
fix: stop resolve() from leaking foreign envs (e.g. hatch) into the venv collection#1507StellaHuang95 wants to merge 1 commit intomicrosoft:mainfrom
StellaHuang95 wants to merge 1 commit intomicrosoft:mainfrom
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
@flying-sheep reported 3 related bugs, #1471, #1485 and #1491
The venvs ones both trace to the same root cause in
VenvManager.resolve(). Hatch envs structurally look like venvs (they have apyvenv.cfg), so PET classifies them asVenvand they pass throughresolveVenvPythonEnvironmentPath. Until now, every successfulresolve()call also mutatedthis.collectionviaaddEnvironment(resolved, true). So whenever anything calledresolve()for a hatch path —Python: Select Interpreter, Pylance startup, a test runner asking for execInfo, another extension querying the env — the hatch env got persisted under venv too.The original side effect dates back to commit
9363bc1(Oct 2024), when the extension only hadsystemandvenvmanagers and the author assumed any successfulresolve()was a newly discovered venv worth caching. That assumption stopped being true once the API supported third-party managers.What this PR does
One-line deletion in
src/managers/builtin/venvManager.ts(plus a small whitespace touch-up):if (resolved) { if (resolved.envId.managerId === `${PYTHON_EXTENSION_ID}:venv`) { - // This is just like finding a new environment or creating a new one. - // Add it to collection, and trigger the added event. - this.addEnvironment(resolved, true); - // We should only return the resolved env if it is a venv. return resolved; } } return undefined;resolve()is now a pure query: "given a path, are you the manager for it?" It still returns the resolved env (every consumer ofresolve()keeps working), it just stops mutating state.What this PR does not fix (follow-ups)
Three layers of bug were identified; this PR ships Layer 1 only.
resolve()side-effect leaks foreign envs into venv. Fixed.Venvkind is structural (it triggers onpyvenv.cfgalone), so cross-manager overlap is expected for any tool that producespyvenv.cfg-shaped envs (hatch inpath = ".venv"mode, tox, nox, custom scripts). Needs a PET-level fix; tracking issue to be filed upstream against microsoft/python-environment-tools.api.environments.onDidChangeActiveEnvironmentPath(the legacy global-only event), not to ourpythonEnvsApi.onDidChangeEnvironment.