Address inline script persistence review

Keep inline-script manager settings scoped to exact script projects and prevent stale warm validation from superseding a newer selection.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 1a9f6ba1-9bd3-4664-bc25-a0d34d7a2e91

Author: StellaHuang95

src/features/envManagers.ts

@@ -348,7 +348,11 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
         // Only persist to settings when explicitly requested
         if (shouldPersistSettings && scope) {
             const packageManager = this.getPackageManager(environment);
-            if (project && packageManager) {
+            const canPersistSettings =
+                project &&
+                packageManager &&
+                this.canPersistManagerSettingForScope(scope, manager, project);
+            if (canPersistSettings) {
                 await setAllManagerSettings([
                     {
                         project,
@@ -361,8 +365,8 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
                 `[setEnvironment] scope=${scope instanceof Uri ? scope.fsPath : scope}, ` +
                     `env=${environment?.envId?.id ?? 'undefined'}, manager=${manager.id}, ` +
                     `project=${project?.uri?.toString() ?? 'none'}, ` +
-                    `packageManager=${this.getPackageManager(environment)?.id ?? 'UNDEFINED'}, ` +
-                    `settingsPersisted=${!!(project && this.getPackageManager(environment))}`,
+                    `packageManager=${packageManager?.id ?? 'UNDEFINED'}, ` +
+                    `settingsPersisted=${!!canPersistSettings}`,
             );
         }
 
@@ -423,16 +427,19 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
                 });
                 scope.forEach((uri) => {
                     const m = this.getEnvironmentManager(uri);
+                    const project = this.pm.get(uri);
                     // Always add settings when persisting, OR when manager differs
-                    if (shouldPersistSettings || manager.id !== m?.id) {
+                    if (
+                        (shouldPersistSettings || manager.id !== m?.id) &&
+                        this.canPersistManagerSettingForScope(uri, manager, project)
+                    ) {
                         settings.push({
-                            project: this.pm.get(uri),
+                            project,
                             envManager: manager.id,
                             packageManager: manager.preferredPackageManagerId,
                         });
                     }
 
-                    const project = this.pm.get(uri);
                     const key = this.getActiveSelectionKey(uri, manager, project);
                     const oldEnv = this._activeSelection.get(key);
                     if (oldEnv?.envId.id !== environment?.envId.id) {
@@ -663,6 +670,18 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
         return `inline-script:${normalizePath(scope.fsPath)}`;
     }
 
+    private canPersistManagerSettingForScope(
+        scope: Uri,
+        manager: InternalEnvironmentManager,
+        project: PythonProject | undefined,
+    ): boolean {
+        // Inline associations are per file; never promote one to its containing project's manager setting.
+        return (
+            manager.id !== INLINE_SCRIPT_MANAGER_ID ||
+            (!!project && normalizePath(project.uri.fsPath) === normalizePath(scope.fsPath))
+        );
+    }
+
     private bumpSelectionRevision(key: string): number {
         const revision = (this._selectionRevisions.get(key) ?? 0) + 1;
         this._selectionRevisions.set(key, revision);

src/managers/builtin/inlineScript/envManager.ts

@@ -400,6 +400,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
         }
 
         const cached = this.fsPathToEnv.get(scriptPath);
+        const revision = this.associationRevisions.get(scriptPath) ?? 0;
         if (cached) {
             const validatedAt = this.cachedAssociationValidatedAt.get(scriptPath);
             if (
@@ -408,7 +409,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
             ) {
                 return cached;
             }
-            const validation = this.validateCachedAssociation(scriptPath, scriptUri, cached);
+            const validation = this.validateCachedAssociation(scriptPath, scriptUri, cached, revision);
             this.pendingRehydrations.set(scriptPath, validation);
             try {
                 return await validation;
@@ -419,7 +420,6 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
             }
         }
 
-        const revision = this.associationRevisions.get(scriptPath) ?? 0;
         const rehydration = this.rehydrateAssociation(scriptPath, scriptUri, revision);
         this.pendingRehydrations.set(scriptPath, rehydration);
         try {
@@ -444,16 +444,23 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
         scriptPath: string,
         scriptUri: Uri,
         cached: PythonEnvironment,
+        revision: number,
     ): Promise<PythonEnvironment | undefined> {
         const environmentPath = cached.environmentPath.fsPath;
         const envDirPath = path.dirname(path.dirname(environmentPath));
-        if (await this.isCacheEntryBusy(envDirPath)) {
+        const busy = await this.isCacheEntryBusy(envDirPath);
+        if (!this.isCurrentAssociationRevision(scriptPath, revision)) {
+            return this.fsPathToEnv.get(scriptPath);
+        }
+        if (busy) {
             return undefined;
         }
         try {
             const stat = await fs.stat(environmentPath);
+            if (!this.isCurrentAssociationRevision(scriptPath, revision)) {
+                return this.fsPathToEnv.get(scriptPath);
+            }
             if (stat.isFile()) {
-                const revision = this.associationRevisions.get(scriptPath) ?? 0;
                 const resolved = await resolveVenvPythonEnvironmentPath(
                     environmentPath,
                     this.nativeFinder,
@@ -491,21 +498,32 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
                 this._onDidChangeEnvironment.fire({ uri: scriptUri, old: cached, new: resolved });
                 return resolved;
             }
-            if (!(await this.isCacheEntryBusy(envDirPath))) {
+            const becameBusy = await this.isCacheEntryBusy(envDirPath);
+            if (!this.isCurrentAssociationRevision(scriptPath, revision)) {
+                return this.fsPathToEnv.get(scriptPath);
+            }
+            if (!becameBusy) {
                 await this.removeStalePersistedAssociation(
                     scriptPath,
                     environmentPath,
-                    this.associationRevisions.get(scriptPath) ?? 0,
+                    revision,
                     scriptUri,
                 );
             }
         } catch (error) {
+            if (!this.isCurrentAssociationRevision(scriptPath, revision)) {
+                return this.fsPathToEnv.get(scriptPath);
+            }
             if (this.isDefinitivelyStalePathError(error)) {
-                if (!(await this.isCacheEntryBusy(envDirPath))) {
+                const becameBusy = await this.isCacheEntryBusy(envDirPath);
+                if (!this.isCurrentAssociationRevision(scriptPath, revision)) {
+                    return this.fsPathToEnv.get(scriptPath);
+                }
+                if (!becameBusy) {
                     await this.removeStalePersistedAssociation(
                         scriptPath,
                         environmentPath,
-                        this.associationRevisions.get(scriptPath) ?? 0,
+                        revision,
                         scriptUri,
                     );
                 }

src/test/features/envManagers.lastKnown.unit.test.ts

@@ -20,16 +20,18 @@ import {
     GetEnvironmentScope,
     PythonEnvironment,
     PythonEnvironmentId,
+    PythonProject,
 } from '../../api';
 import * as extensionApis from '../../common/extension.apis';
 import { PythonEnvironmentManagers } from '../../features/envManagers';
 import * as settingHelpers from '../../features/settings/settingHelpers';
-import { PythonProjectManager } from '../../internal.api';
+import { InternalPackageManager, PythonProjectManager } from '../../internal.api';
 import { setupNonThenable } from '../mocks/helper';
 
 suite('PythonEnvironmentManagers getLastKnownEnvironment', () => {
     let envManagers: PythonEnvironmentManagers;
     let projectManager: typeMoq.IMock<PythonProjectManager>;
+    let projectsByUri: Map<string, PythonProject>;
 
     function makeEnv(id: string): PythonEnvironment {
         const envId: PythonEnvironmentId = { id, managerId: 'test-manager' };
@@ -58,8 +60,10 @@ suite('PythonEnvironmentManagers getLastKnownEnvironment', () => {
 
         projectManager = typeMoq.Mock.ofType<PythonProjectManager>();
         setupNonThenable(projectManager);
-        // No project for a scope -> refreshEnvironment/getLastKnownEnvironment use the 'global' key.
-        projectManager.setup((pm) => pm.get(typeMoq.It.isAny())).returns(() => undefined);
+        projectsByUri = new Map();
+        projectManager
+            .setup((pm) => pm.get(typeMoq.It.isAny()))
+            .returns((uri) => projectsByUri.get(uri.toString()));
 
         envManagers = new PythonEnvironmentManagers(projectManager.object);
     });
@@ -96,6 +100,13 @@ suite('PythonEnvironmentManagers getLastKnownEnvironment', () => {
         return id;
     }
 
+    function stubPackageManager(id = 'ms-python.python:pip'): void {
+        const packageManager = typeMoq.Mock.ofType<InternalPackageManager>();
+        setupNonThenable(packageManager);
+        packageManager.setup((manager) => manager.id).returns(() => id);
+        sinon.stub(envManagers, 'getPackageManager').returns(packageManager.object);
+    }
+
     test('returns undefined before any environment has been resolved', () => {
         registerManager(async () => makeEnv('env1'));
         assert.strictEqual(envManagers.getLastKnownEnvironment(undefined), undefined);
@@ -127,7 +138,7 @@ suite('PythonEnvironmentManagers getLastKnownEnvironment', () => {
     test('does not update selection, settings, or events when a registered manager rejects a selection', async () => {
         const scope = Uri.file('/workspace/script.py');
         const project = { name: 'script.py', uri: scope };
-        projectManager.setup((pm) => pm.get(scope)).returns(() => project);
+        projectsByUri.set(scope.toString(), project);
         const managerSet = sinon.stub().rejects(new Error('Inline-script environment is not an owned cache entry.'));
         const managerId = registerManager(async () => undefined, managerSet);
         const rejected = {
@@ -239,6 +250,65 @@ suite('PythonEnvironmentManagers getLastKnownEnvironment', () => {
         assert.strictEqual(envManagers.getLastKnownEnvironment(secondUri), second);
     });
 
+    test('does not persist an inline-script manager for the containing project', async () => {
+        const script = Uri.file('/workspace/project/script.py');
+        const containingProject = { name: 'project', uri: Uri.file('/workspace/project') };
+        projectsByUri.set(script.toString(), containingProject);
+        const managerId = registerManager(async () => undefined, async () => undefined, 'inline-script');
+        const environment = { ...makeEnv('inline'), envId: { id: 'inline', managerId } };
+        stubPackageManager();
+        const settings = sinon.stub(settingHelpers, 'setAllManagerSettings').resolves();
+
+        await envManagers.setEnvironment(script, environment);
+
+        assert.strictEqual(settings.callCount, 0);
+    });
+
+    test('persists an inline-script manager when the script is its own project', async () => {
+        const script = Uri.file('/workspace/script.py');
+        const scriptProject = { name: 'script.py', uri: script };
+        projectsByUri.set(script.toString(), scriptProject);
+        const managerId = registerManager(async () => undefined, async () => undefined, 'inline-script');
+        const environment = { ...makeEnv('inline'), envId: { id: 'inline', managerId } };
+        stubPackageManager();
+        const settings = sinon.stub(settingHelpers, 'setAllManagerSettings').resolves();
+
+        await envManagers.setEnvironment(script, environment);
+
+        sinon.assert.calledOnce(settings);
+        assert.deepStrictEqual(settings.firstCall.args[0], [
+            {
+                project: scriptProject,
+                envManager: managerId,
+                packageManager: 'ms-python.python:pip',
+            },
+        ]);
+    });
+
+    test('persists batch inline settings only for scripts registered as exact projects', async () => {
+        const exactScript = Uri.file('/workspace/exact.py');
+        const nestedScript = Uri.file('/workspace/project/nested.py');
+        const looseScript = Uri.file('/outside/loose.py');
+        const exactProject = { name: 'exact.py', uri: exactScript };
+        const containingProject = { name: 'project', uri: Uri.file('/workspace/project') };
+        projectsByUri.set(exactScript.toString(), exactProject);
+        projectsByUri.set(nestedScript.toString(), containingProject);
+        const managerId = registerManager(async () => undefined, async () => undefined, 'inline-script');
+        const environment = { ...makeEnv('inline'), envId: { id: 'inline', managerId } };
+        const settings = sinon.stub(settingHelpers, 'setAllManagerSettings').resolves();
+
+        await envManagers.setEnvironments([exactScript, nestedScript, looseScript], environment);
+
+        sinon.assert.calledOnce(settings);
+        assert.deepStrictEqual(settings.firstCall.args[0], [
+            {
+                project: exactProject,
+                envManager: managerId,
+                packageManager: 'ms-python.python:pip',
+            },
+        ]);
+    });
+
     test('retains an earlier successful refresh when a later refresh fails', async () => {
         const refreshed = makeEnv('refreshed');
         let resolveFirst: ((environment: PythonEnvironment) => void) | undefined;

src/test/managers/builtin/inlineScript/envManager.unit.test.ts

@@ -1754,6 +1754,48 @@ suite('InlineScriptEnvManager', () => {
             sinon.assert.calledOnceWithExactly(listener, { uri, old: environment, new: rebuilt });
         });
 
+        test('lets an explicit selection win while warm validation awaits filesystem inspection', async () => {
+            const uri = scriptUri();
+            const oldEnvironment = await createOwnedEnvironment();
+            const selectedEnvironment = await createOwnedEnvironment('fedcba9876543210');
+            await manager.set(uri, oldEnvironment);
+            const rebuiltOldEnvironment = {
+                ...oldEnvironment,
+                envId: { ...oldEnvironment.envId, id: 'rebuilt-old' },
+                version: '3.13.1',
+            };
+            resolveVenvStub.resolves(rebuiltOldEnvironment);
+            let releaseBusyCheck: (() => void) | undefined;
+            const busyCheckGate = new Promise<boolean>((resolve) => {
+                releaseBusyCheck = () => resolve(false);
+            });
+            const validationManager = manager as unknown as {
+                isCacheEntryBusy(envDirPath: string): Promise<boolean>;
+            };
+            const busyCheckStub = sinon.stub(validationManager, 'isCacheEntryBusy').callThrough();
+            busyCheckStub.onFirstCall().returns(busyCheckGate);
+            const listener = sinon.spy();
+            manager.onDidChangeEnvironment(listener);
+            clock.tick(5_000);
+
+            const pendingGet = manager.get(uri);
+            await waitForStubCall(busyCheckStub);
+            await manager.set(uri, selectedEnvironment);
+            releaseBusyCheck!();
+
+            assert.strictEqual(await pendingGet, selectedEnvironment);
+            assert.strictEqual(await manager.get(uri), selectedEnvironment);
+            assert.deepStrictEqual(persistedAssociations, {
+                [normalizePath(uri.fsPath)]: selectedEnvironment.environmentPath.fsPath,
+            });
+            assert.strictEqual(resolveVenvStub.callCount, 0);
+            sinon.assert.calledOnceWithExactly(listener, {
+                uri,
+                old: oldEnvironment,
+                new: selectedEnvironment,
+            });
+        });
+
         test('unsets a persisted association after transient rehydration failure', async () => {
             const uri = scriptUri();
             const environment = await createOwnedEnvironment();