Fix inline-script routing for closed scripts, and surface its commands and names - #1761
Conversation
…ature flag Add python-envs.clearScriptEnvCache to contributes.commands and gate its Command Palette visibility with a pythonEnvsInlineScriptsEnabled context key, set at activation from the latched inline-script feature flag. Keying palette visibility off the same activation-time flag that gates the command's registration keeps the two in lockstep, so the command is never shown before a window reload has actually registered it (a live config. when-clause would reveal it early, and clicking it would fail with "command not found"). When the flag is off the command stays unregistered and hidden, so regular users are unaffected.
…ture flag Contribute python-envs.setupInlineScriptEnvs and gate its Command Palette visibility with the pythonEnvsInlineScriptsEnabled context key, which is already set at activation from the latched inline-script feature flag. No activation code is needed here: the command is already registered (only when the flag is on) via registerInlineScriptUx, so it stays hidden and unregistered for regular users. Title it "Set Up Environments for Inline Script Files" to match the QuickPick the command opens. Correct the now-stale doc comments that described the command as hidden/not-contributed, and add smoke tests mirroring the clear-cache command (contributed-but-palette-gated, and not-registered-when-the-flag-is-off).
PEP 723 inline-script environments previously surfaced their internal content-addressed cache-key hash in both the UI name and the creation progress notification, which is meaningless to users. Now the setup progress names the script being set up, and the environment shows a readable name (script env (3.12.4), short form 3.12.4 (script)) across all resolution paths: build, reuse, discovery, validation and rehydration. Implemented via an optional nameStyle/progressTitle on createWithProgress and a nameStyle argument on resolveVenvPythonEnvironmentPath; regular venv naming and progress are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7415b86e-8399-43d5-a504-071cd90592df
Aligns the Environment Managers tree label with the terse style of the other managers (venv, Conda, Global) by dropping the redundant environments suffix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7415b86e-8399-43d5-a504-071cd90592df
The routing registry was only ever populated by the lazy detector, which observes open documents. A script set up in an earlier session, or by the bulk command while it was closed, therefore had no routing metadata, could never satisfy shouldRoute, and fell through to the default env manager -- so the project view showed the wrong environment until the file was opened. Seed the registry from the saved file at the two points that already know an inline association exists: - initializePersistedAssociations, so a reload routes persisted scripts - setUpInlineScriptEnvironment, so bulk setup of closed scripts routes Both skip open documents, which the detector owns and whose metadata it deliberately withholds while the block is being edited, and both re-check the registry after the async read so they cannot race it. No metadata is invented: it is read from the saved file exactly as the detector would. Detection telemetry is unaffected because seeding bypasses the detector. The restart-reselect test now lets startup validation settle before measuring, since persisted associations are validated eagerly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e61ef1b to
6038c8b
Compare
|
🔒 Automated review in progress — Rich Chiodo (@rchiodo) is auto-reviewing this PR. |
| } | ||
| const metadata = await readInlineScriptMetadataFromFile(scriptUri); | ||
| if (metadata && !routing.getMetadata(scriptUri)) { | ||
| routing.setMetadata(scriptUri, metadata); |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
Could this re-check whether the document became open after the awaited metadata read? A user can open and edit the script while the read is pending; in that case this can publish stale on-disk metadata even though the detector intentionally withholds metadata for the edited document.
|
GitHub cannot anchor PR review comments to unchanged lines in the diff. Falling back to a general PR comment for src/managers/builtin/venvUtils.ts:L166.
The error branch returns before applying |
|
Result: 🔴 Verification detailsVerification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified. Summary: No tests ran because the verification container could not start: Docker was unavailable and local fallback was not authorized. The PR adds six routing regression cases and adds or updates three smoke checks. All relevant targeted commands are recorded as not-run, so runtime confidence is low. Additional coverage is missing for inline-script naming/progress and metadata-read races. Test runs: 5 not run
|
Rich Chiodo (rchiodo)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
5246c9b
into
microsoft:main
Five independent fixes and polish items for the PEP 723 inline-script feature, found while exercising it end to end. Every change is behind the existing internal
python-envs.inlineScripts.enabledflag (undeclared, defaults tofalse), so there is no behavior change for users who do not opt in.1. Closed inline scripts resolved to the wrong environment
Issue. With two or more scripts that have inline-script environments, the Python Projects view showed the correct environment only for the file that happened to be open. Every other script displayed the default/venv environment — and its packages — until you clicked the file open, at which point the row suddenly corrected itself.
Cause.
InlineScriptRoutingRegistry.setMetadata()had exactly one caller: the lazy detector, which only observes documents on open/save. So the registry only ever knew about open files.shouldRoute()requires metadata, so a closed script could never route;getExactProjectEnvironmentManagerdeliberately declines to use the exact project setting for inline-registered projects, so resolution fell through to the project's persistedenvManager— which is written as the default manager, not the inline one.A second instance of the same bug: the bulk setup command operates on scripts that are typically closed.
updateValidatedStateForSelectioncomparesrouting.getMetadataIdentity(uri)(undefined for a closed file) against the saved identity, so bulk setup built the environment and persisted amatchedassociation that could never become routeable. This also explains why the picker's "environment already set up" hint never appeared for closed scripts.Fix. Seed the routing registry from the saved file at the two points that already know an inline association exists —
initializePersistedAssociations(reload) andsetUpInlineScriptEnvironment(setup/bulk). No metadata is invented; it is read from disk exactly as the detector would, and it re-enters the existing validation pipeline rather than adding a parallel one. Both paths skip open documents (the detector owns those and deliberately withholds metadata while the block is being edited) and re-check the registry after the async read so they cannot race it.Detection telemetry is unaffected: seeding calls the registry directly rather than going through the detector, so
inlineScript.detectedkeeps its "files the user has shown intent in" meaning.2. Environment name leaked the internal cache-key hash
Issue. Inline-script environments live in content-addressed cache folders, so the UI showed users a meaningless hash such as
0f3a91c4d5e2b7a8 (3.12.4), and the creation progress notification readCreating virtual environment named 0f3a91c4d5e2b7a8 using python version 3.12.4.Fix. Setup progress now names the script being set up, and the environment shows
script env (3.12.4)(short form3.12.4 (script)) across every resolution path — build, reuse, discovery, validation and rehydration. Implemented as an optionalnameStyle/progressTitleoncreateWithProgressand anameStyleargument onresolveVenvPythonEnvironmentPath; regular venv naming and progress are unchanged.3. Environment Managers label was inconsistent
Improvement. Renamed the manager label from
Inline script environmentstoInline scripts, matching the terse style of the sibling managers (venv,Conda,Global).4. Clear Cache command was unreachable
Issue.
python-envs.clearScriptEnvCachewas registered but never contributed, so there was no way to invoke it.Fix. Contributed it and gated Command Palette visibility on a
pythonEnvsInlineScriptsEnabledcontext key set at activation from the same latched flag that gates registration. Keying visibility off the activation-time flag rather than a liveconfig.when-clause keeps the two in lockstep — otherwise the command would appear before the required window reload had registered it, and invoking it would fail with "command not found".5. Bulk setup command was unreachable
Issue.
python-envs.setupInlineScriptEnvswas registered byregisterInlineScriptUxbut not contributed, so the bulk flow could not be reached from the palette.Fix. Contributed it behind the same context key, titled Set Up Environments for Inline Script Files to match the QuickPick it opens. Also corrected doc comments that still described it as hidden.
Testing
tsccompile andnpm run lintacrosssrcare clean.does not rewrite or notify when a restart reselects the same persisted executable) now lets startup validation settle before measuring, because persisted associations are validated eagerly. All four of its original assertions still hold for the reselection itself.Notes for reviewers
envManager.tsshows a large diff in the final commit, but that is overwhelmingly an import-sort/Prettier reformat; the functional change is about 29 lines (seedRoutingMetadataFromSavedFile,isDocumentOpen, and one call site).getEnvironment()moments later anyway — and it runs off the activation critical path.