Fix PEP 723 cache drift and support deleting individual script environments - #1780
Conversation
1afe4bc to
4b9c138
Compare
|
🔒 Automated review in progress — Rich Chiodo (@rchiodo) is auto-reviewing this PR. |
Inline-script environments are built from a script's `# /// script` block and cached per (dependencies, base interpreter), so one entry is shared by every script that resolves to it. The extension must therefore notice when such an environment stops matching what setup installed. It previously answered that by interpreting package-change events, which required classifying every event's origin: provenance metadata on a shared notification contract, per-environment revisions and operation queues, a pre-debounce snapshot captured by the file watcher, and a provider opt-in flag so third-party package managers kept their existing contract. Several defects came from that machinery misjudging the extension's own work - most visibly, merely listing an environment's packages compared them against an empty baseline and forced a working environment through setup again. This records the installed distributions in the entry's own `.meta.json`, inside the cache-entry lock that already guards the build, and compares that record against the `*.dist-info` directories when validating. Because the record is written under the same lock as the installation it describes, the extension cannot observe its own work as an external edit, so the races the provenance machinery refereed cannot occur. An entry with no recorded inventory, or one whose inventory cannot be read, is unknown rather than modified. `compareInstalledPackages` is the only place that distinction is made; treating "no record" as "everything was added" is the mistake this must never repeat. Package management is no longer offered for inline-script environments. Editing one by hand would silently change every script sharing it, so the install, uninstall, and version-change actions are hidden; their package list stays visible. Read-only packages use a distinct context value, so the existing menu clauses simply do not match them. Package management is refused at the command resolution chokepoint as well as hidden in the tree. The palette entry can still resolve an inline-script environment from the active script, so `getPackageCommandOptions` rejects one with actionable guidance rather than letting the install proceed. Also fixed, independent of the above: - A modified environment stays unroutable until setup runs again. The check is enforced on association lookup, selection, and metadata-provenance validation, not only on creation and reuse, so saving or reopening a script cannot make a drifted environment look usable. Explicitly selecting one is rejected with actionable guidance rather than an unhandled error. - Renaming a script with a case-only change on Windows keeps its environment. The association key is unchanged, so the rename is no longer treated as a reason to clear it. Dirty documents and pending metadata reads are resolved through canonical script keys. No shared package-manager code is touched: `packageChanges`, `packageWatcher`, `internal.api`, the environment-manager event relay, the conda and poetry package managers, the pip package manager, and both views are unchanged, so other environment types and third-party providers keep their exact behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a4277008-9dd3-4beb-b9ac-4197eb085404
Environment lookup could reject a drifted entry while leaving its routing state and the other scripts sharing it marked valid. Confirmed drift now clears those cached associations and validation results, restores the setup CodeLens without a save, and notifies environment consumers. Recheck the current inventory and sidecar under the existing cache-entry lock before invalidating. Do not wait for another installer or invalidate from an unreadable confirmation. Preserve newer selections when an older lookup finishes, and keep persisted associations available for repair. Add regression coverage for get-only detection, shared and unrelated entries, cold rehydration, newer repairs and selections, lock contention, unavailable inventory, and the real cache-entry lock. No shared package-manager changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a4277008-9dd3-4beb-b9ac-4197eb085404
Support Delete Environment for an individual inline-script cache entry without a confirmation prompt. Reuse the existing cache maintenance and entry-lock checks, but limit disk deletion and association cleanup to the selected entry. Clear affected routing and notify consumers, remove the deleted environment from the discovered collection, and preserve scripts, project settings, base Python installations, unrelated environments, and opaque association records. Resolve inline Projects-view items through their owning manager rather than the file's potentially changed fallback selection. Reject missing, foreign, redirected, and busy targets. Mark an entry unusable before deletion so a partial failure cannot leave surviving files trusted as a healthy environment. Report filesystem and persistence failures, and preserve operation ordering with concurrent setup and rehydration. Add coverage for shared/unshared deletion, no confirmation, isolation, missing entries, unsafe targets, locks, partial failures, and command routing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a4277008-9dd3-4beb-b9ac-4197eb085404
4b9c138 to
1d3ea92
Compare
|
Result: 🔴 Verification detailsVerification: Isolated verification observed failures that were not classified as caused by this PR: Dependency and test discovery. The relevant tests could not be fully run in the isolated environment; this review is not fully verified. Summary: Verification could not start because the container has no Node.js executable; discovery failed with `/bin/sh: node: not found`. Consequently, TypeScript compilation and the targeted Mocha suites were not run. The PR adds extensive unit coverage for drift detection, deletion, routing, and package-management restrictions, but none could be executed here. Overall confidence is limited by the verification environment. Test runs: 1 failed, 4 not run
|
Replace the negative package-capability substring assertion with the complete expected contextValue. The test now also detects missing environment, remove, or activation markers instead of accepting an empty or undefined context. Addresses PR microsoft#1780 review comment 3993748325. No production-code changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a4277008-9dd3-4beb-b9ac-4197eb085404
|
Result: 🔴 Verification detailsVerification: Isolated verification observed failures that were not classified as caused by this PR: Dependency and test-runner preflight. The relevant tests could not be fully run in the isolated environment; this review is not fully verified. Summary: Verification could not proceed because the container lacks Node.js (`node: not found`). Consequently, compilation and the four targeted Mocha groups were not run. The PR adds extensive unit coverage for drift detection, routing, package restrictions, renames, and targeted deletion, but none could be executed here. Test runs: 1 failed, 4 not run
|
Rich Chiodo (rchiodo)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
6ad1427
into
microsoft:main
Summary
Fix PEP 723 inline-script cache and routing issues, make cached environments read-only through the package-management UI, and support deleting one cached environment without clearing the entire script cache.
The feature remains behind the internal, undeclared
python-envs.inlineScripts.enabledflag, which defaults tofalseand is latched at activation. This does not enable inline scripts by default or add automatic installation on Run/Debug.Commits and user-visible changes
64960273669ffe181d3ea92aImplementation and behavior notes
Record package inventory instead of interpreting package events
installedPackagesHashin the entry's.meta.json, under the existing cache-entry lock. The hash is computed from sorted, case-normalized*.dist-infonames, which include distribution names and versions.manuallyModifiedsidecars remain honored.Keep routing and the setup action consistent
Delete one cached environment
Testing
Local validation before the rebase/signing update (
1afe4bc9):npm run compile-tests: passed.npm run compile(webpack extension bundle): passed.npm run lint: passed.Coverage includes get-only drift detection and CodeLens recovery, shared versus unrelated entries, stale observations after newer repairs/selections, real cache-entry locks, unavailable inventory, Windows case-only renames, palette restrictions, targeted deletion without prompting, protected source/settings/base-Python files, already-missing entries, unsafe paths, partial failures, and concurrent create/delete/rehydration.
Suggested manual verification
These are reviewer checks, not a claim that a full live VS Code end-to-end pass has been completed:
python-envs.inlineScripts.enabledand reload. Use the Python Environments integration and companion builds that support per-file interpreters.job.pytoJob.py; verify the existing environment remains associated and unsaved metadata is not replaced with stale disk metadata.Scope and known follow-ups
.egg-info-only installations.site-packagesdirectory is accepted during reuse remains a follow-up; deleting the cache entry and running setup provides a recovery path.