Commit 5ea9d15
authored
fix: stop resolve() from leaking foreign envs (e.g. hatch) into the venv collection (#1507)
### Context
@flying-sheep reported three related bugs — #1471, #1485, #1491. The two
venv-related ones both traced to the same root cause in
`VenvManager.resolve()`.
Hatch envs structurally look like venvs (they have a `pyvenv.cfg`), so
until very recently PET classified them as `Venv` and they passed
through `resolveVenvPythonEnvironmentPath`. Every successful `resolve()`
call also mutated `this.collection` via `addEnvironment(resolved,
true)`. So whenever anything called `resolve()` 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.
The original side effect dates back to commit
[9363bc1](9363bc1)
(Oct 2024), when the extension only had `system` and `venv` managers and
the author assumed any successful `resolve()` was a newly discovered
venv worth caching. That assumption stopped being true once the API
supported third-party managers.
### Upstream fix has now landed in PET
As of
[microsoft/python-environment-tools#460](microsoft/python-environment-tools#460)
(commit
[d581272](microsoft/python-environment-tools@d581272)),
PET now has a dedicated `pet-hatch` locator:
- New `LocatorKind::Hatch` and `PythonEnvironmentKind::Hatch`.
- Registered **before** the `Venv` locator, so it claims hatch envs
first.
- Matches hatch's real storage layout
(`<data_dir>/env/virtual/<project_name>/<project_id>/<venv_name>`),
honors `HATCH_DATA_DIR`, and reads `[tool.hatch.dirs.env].virtual` from
`pyproject.toml` / `hatch.toml`.
That fixes the structural source of the hatch-as-venv misclassification.
### What this PR does
This PR is a complementary, defense-in-depth correction in
`src/managers/builtin/venvManager.ts`:
```diff
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 of `resolve()`
keeps working), it just stops mutating state. Cached venvs continue to
land in `this.collection` through the normal discovery and creation
paths, which is the only path that should be writing to it.
### Why keep this PR alongside the PET fix
Even with PET correctly classifying hatch envs, `VenvManager.resolve()`
mutating `this.collection` is an invariant violation worth correcting:
- **General cross-manager overlap.** Anything that produces a
`pyvenv.cfg`-shaped env (tox, nox, custom bootstrap scripts, future
tools) would re-introduce the same class of leak. With `resolve()` as a
pure query, no future overlap can silently grow the venv collection.
- **Locator-ordering regressions.** If PET's locator priority ever
shifts again, this change keeps the venv manager from quietly absorbing
whatever lands in its lap.1 parent 5e524ce commit 5ea9d15
1 file changed
Lines changed: 9 additions & 12 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
336 | 336 | | |
337 | 337 | | |
338 | 338 | | |
339 | | - | |
340 | | - | |
341 | | - | |
342 | | - | |
343 | | - | |
344 | | - | |
345 | | - | |
346 | | - | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
| 343 | + | |
| 344 | + | |
| 345 | + | |
| 346 | + | |
| 347 | + | |
347 | 348 | | |
348 | 349 | | |
349 | 350 | | |
| |||
500 | 501 | | |
501 | 502 | | |
502 | 503 | | |
503 | | - | |
504 | | - | |
505 | | - | |
506 | | - | |
507 | 504 | | |
508 | 505 | | |
509 | 506 | | |
| |||
0 commit comments