Address inline manager review feedback

Centralize the pyenv manager ID, simplify inline manager data flow and release comparisons, and share filesystem not-found classification across cache components.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 39dcc6a3-0fbd-4f36-9d0f-68677de49c27

Author: StellaHuang95

src/common/constants.ts

@@ -4,6 +4,7 @@ export const ENVS_EXTENSION_ID = 'ms-python.vscode-python-envs';
 export const PYTHON_EXTENSION_ID = 'ms-python.python';
 export const CONDA_MANAGER_ID = `${PYTHON_EXTENSION_ID}:conda`;
 export const INLINE_SCRIPT_MANAGER_ID = `${PYTHON_EXTENSION_ID}:inline-script`;
+export const PYENV_MANAGER_ID = `${PYTHON_EXTENSION_ID}:pyenv`;
 export const JUPYTER_EXTENSION_ID = 'ms-toolsai.jupyter';
 export const EXTENSION_ROOT_DIR = path.dirname(__dirname);
 export const ISSUES_URL = 'https://github.com/microsoft/vscode-python-environments/issues';

src/common/inlineScriptCacheLayout.ts

@@ -8,6 +8,7 @@ import { Uri } from 'vscode';
 import type { PythonEnvironment } from '../api';
 import { INLINE_SCRIPT_MANAGER_ID } from './constants';
 import { traceWarn } from './logging';
+import { isFileNotFoundError } from './utils/filesystem';
 import { normalizePath } from './utils/pathUtils';
 import { isWindows } from './utils/platformUtils';
 import { getVenvPythonPath } from './utils/virtualEnvironment';
@@ -273,10 +274,6 @@ function parsePyvenvHome(raw: string): string | undefined {
     return undefined;
 }
 
-function isFileNotFoundError(err: unknown): boolean {
-    return typeof err === 'object' && err !== null && 'code' in err && (err as NodeJS.ErrnoException).code === 'ENOENT';
-}
-
 function isDescendantPath(rootPath: string, candidatePath: string): boolean {
     const relative = path.relative(rootPath, candidatePath);
     return (

src/common/utils/filesystem.ts

@@ -0,0 +1,11 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+export function isFileNotFoundError(error: unknown): error is NodeJS.ErrnoException {
+    return (
+        typeof error === 'object' &&
+        error !== null &&
+        'code' in error &&
+        (error as NodeJS.ErrnoException).code === 'ENOENT'
+    );
+}

src/managers/builtin/inlineScriptEnvManager.ts

@@ -21,7 +21,6 @@ import {
 } from '../../api';
 import { computeCacheKey } from '../../common/inlineScriptCacheKey';
 import {
-    InlineScriptEnvMeta,
     META_SCHEMA_VERSION,
     getBaseInterpreterStatus,
     getScriptEnvCacheRoot,
@@ -37,8 +36,9 @@ import {
     matchesPythonVersion,
     readInlineScriptMetadataFromFile,
 } from '../../common/inlineScriptMetadata';
-import { CONDA_MANAGER_ID, PYTHON_EXTENSION_ID, SYSTEM_MANAGER_ID } from '../../common/constants';
+import { CONDA_MANAGER_ID, PYENV_MANAGER_ID, SYSTEM_MANAGER_ID } from '../../common/constants';
 import { acquireFileLock, AcquiredFileLock } from '../../common/lockfile.apis';
+import { isFileNotFoundError } from '../../common/utils/filesystem';
 import { normalizePath } from '../../common/utils/pathUtils';
 import { compareReleaseSegments, parseReleaseSegments } from '../../common/utils/pep440Release';
 import { getVenvPythonPath } from '../../common/utils/virtualEnvironment';
@@ -48,12 +48,33 @@ import { createWithProgress, resolveVenvPythonEnvironmentPath } from './venvUtil
 const BASE_INTERPRETER_MANAGER_IDS = new Set([
     SYSTEM_MANAGER_ID,
     CONDA_MANAGER_ID,
-    `${PYTHON_EXTENSION_ID}:pyenv`,
+    PYENV_MANAGER_ID,
 ]);
 
 const CACHE_LOCK_TIMEOUT_MS = 5 * 60 * 1000;
 const CACHE_LOCK_RETRY_MS = 500;
 
+interface SelectedBaseInterpreter {
+    readonly environment: PythonEnvironment;
+    readonly canonicalPath: string;
+}
+
+interface CreateOrReuseEnvironmentOptions {
+    readonly cacheKey: string;
+    readonly packages: ReadonlyArray<string>;
+    readonly metadata: InlineScriptMetadata;
+    readonly selectedBase: SelectedBaseInterpreter;
+}
+
+interface BuildCacheEntryResult {
+    readonly environment?: PythonEnvironment;
+    readonly retainLock?: boolean;
+}
+
+type CacheEntryInspection =
+    | { readonly kind: 'absent' | 'stale' | 'uncertain' }
+    | { readonly kind: 'reusable'; readonly environment: PythonEnvironment };
+
 /** Manages extension-owned PEP 723 script environments. */
 export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
     private readonly pendingCreations = new Map<string, Promise<PythonEnvironment | undefined>>();
@@ -100,9 +121,10 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
                 return undefined;
             }
 
-            const packages = [...(metadata.dependencies ?? []), ...(options?.additionalPackages ?? [])].map((value) =>
-                value.trim(),
-            );
+            const packages = [
+                ...(metadata.dependencies ?? []),
+                ...(options?.additionalPackages ?? []),
+            ].map((value) => value.trim());
             if (packages.some((value) => value.length === 0)) {
                 this.log.warn(`Inline-script dependencies must not contain empty entries: ${scriptUri.fsPath}.`);
                 return undefined;
@@ -113,6 +135,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
                 this.log.warn(`No installed Python satisfies the inline-script requirements for ${scriptUri.fsPath}.`);
                 return undefined;
             }
+
             const cacheKey = computeCacheKey({
                 dependencies: packages,
                 interpreterPath: selectedBase.canonicalPath,
@@ -122,7 +145,12 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
                 return await pending;
             }
 
-            const creation = this.createOrReuseEnvironment(cacheKey, packages, metadata, selectedBase);
+            const creation = this.createOrReuseEnvironment({
+                cacheKey,
+                packages,
+                metadata,
+                selectedBase,
+            });
             this.pendingCreations.set(cacheKey, creation);
             try {
                 return await creation;
@@ -163,7 +191,8 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
     }
 
     private async selectBaseInterpreter(metadata: InlineScriptMetadata): Promise<SelectedBaseInterpreter | undefined> {
-        const reported = (await this.api.getEnvironments('global')).filter(
+        const globalEnvironments = await this.api.getEnvironments('global');
+        const reported = globalEnvironments.filter(
             (environment) =>
                 BASE_INTERPRETER_MANAGER_IDS.has(environment.envId.managerId) &&
                 (environment.envId.managerId !== CONDA_MANAGER_ID || environment.name === 'base'),
@@ -209,12 +238,12 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
         return undefined;
     }
 
-    private async createOrReuseEnvironment(
-        cacheKey: string,
-        packages: ReadonlyArray<string>,
-        metadata: InlineScriptMetadata,
-        selectedBase: SelectedBaseInterpreter,
-    ): Promise<PythonEnvironment | undefined> {
+    private async createOrReuseEnvironment({
+        cacheKey,
+        packages,
+        metadata,
+        selectedBase,
+    }: CreateOrReuseEnvironmentOptions): Promise<PythonEnvironment | undefined> {
         const cacheRoot = getScriptEnvCacheRoot(this.globalStorageUri);
         const envDir = getScriptEnvDir(this.globalStorageUri, cacheKey);
         await fs.ensureDir(cacheRoot.fsPath);
@@ -279,7 +308,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
                 return { kind: 'uncertain' };
             }
         } catch (error) {
-            return this.isFileNotFoundError(error) ? { kind: 'absent' } : { kind: 'uncertain' };
+            return isFileNotFoundError(error) ? { kind: 'absent' } : { kind: 'uncertain' };
         }
 
         let resolvedEntry: string | undefined;
@@ -329,8 +358,7 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
         if (environmentStatus !== 'expected') {
             return { kind: environmentStatus };
         }
-        const releaseComparison = this.comparePythonReleases(environment.version, selectedBase.environment.version);
-        if (releaseComparison !== 'same') {
+        if (!this.areEqualPythonReleases(environment.version, selectedBase.environment.version)) {
             return { kind: 'stale' };
         }
         const requiresPython = metadata.requiresPython?.trim();
@@ -385,22 +413,21 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
             return {};
         }
         if (
-            this.comparePythonReleases(result.environment.version, selectedBase.environment.version) !== 'same' ||
+            !this.areEqualPythonReleases(result.environment.version, selectedBase.environment.version) ||
             (await inspectOwnedCacheEntry(result.environment, cacheRoot, envDir)) !== 'expected'
         ) {
             this.log.error('Created inline-script environment does not match the requested cache entry.');
             await this.removeCacheEntry(envDir);
             return {};
         }
 
-        const sidecar: InlineScriptEnvMeta = {
-            schemaVersion: META_SCHEMA_VERSION,
-            baseInterpreterPath: selectedBase.canonicalPath,
-            baseInterpreterVersion: selectedBase.environment.version,
-            lastUsedAt: new Date().toISOString(),
-        };
         try {
-            await writeMetaJson(envDir, sidecar);
+            await writeMetaJson(envDir, {
+                schemaVersion: META_SCHEMA_VERSION,
+                baseInterpreterPath: selectedBase.canonicalPath,
+                baseInterpreterVersion: selectedBase.environment.version,
+                lastUsedAt: new Date().toISOString(),
+            });
         } catch (error) {
             this.log.error(`Failed to record inline-script cache metadata: ${this.errorMessage(error)}`);
             await this.removeCacheEntry(envDir);
@@ -420,22 +447,13 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
         }
     }
 
-    private isFileNotFoundError(error: unknown): boolean {
-        return (
-            typeof error === 'object' &&
-            error !== null &&
-            'code' in error &&
-            (error as NodeJS.ErrnoException).code === 'ENOENT'
-        );
-    }
-
-    private comparePythonReleases(actual: string, expected: string): PythonReleaseComparison {
+    private areEqualPythonReleases(actual: string, expected: string): boolean {
         const actualRelease = parseReleaseSegments(actual);
         const expectedRelease = parseReleaseSegments(expected);
         if (actualRelease === undefined || expectedRelease === undefined) {
-            return 'uncertain';
+            return false;
         }
-        return compareReleaseSegments(actualRelease, expectedRelease) === 0 ? 'same' : 'different';
+        return compareReleaseSegments(actualRelease, expectedRelease) === 0;
     }
 
     private errorMessage(error: unknown): string {
@@ -447,19 +465,3 @@ export class InlineScriptEnvManager implements EnvironmentManager, Disposable {
         this._onDidChangeEnvironment.dispose();
     }
 }
-
-interface SelectedBaseInterpreter {
-    readonly environment: PythonEnvironment;
-    readonly canonicalPath: string;
-}
-
-interface BuildCacheEntryResult {
-    readonly environment?: PythonEnvironment;
-    readonly retainLock?: boolean;
-}
-
-type CacheEntryInspection =
-    | { readonly kind: 'absent' | 'stale' | 'uncertain' }
-    | { readonly kind: 'reusable'; readonly environment: PythonEnvironment };
-
-type PythonReleaseComparison = 'same' | 'different' | 'uncertain';

src/managers/pyenv/pyenvManager.ts

@@ -15,6 +15,7 @@ import {
     ResolveEnvironmentContext,
     SetEnvironmentScope,
 } from '../../api';
+import { PYENV_MANAGER_ID } from '../../common/constants';
 import { PyenvStrings } from '../../common/localize';
 import { traceError, traceInfo } from '../../common/logging';
 import { StopWatch } from '../../common/stopWatch';
@@ -120,7 +121,7 @@ export class PyEnvManager implements EnvironmentManager, Disposable {
             if (toolSource === 'none') {
                 result = 'tool_not_found';
                 if (this.projectManager) {
-                    await notifyMissingManagerIfDefault('ms-python.python:pyenv', this.projectManager, this.api);
+                    await notifyMissingManagerIfDefault(PYENV_MANAGER_ID, this.projectManager, this.api);
                 }
             }
         } catch (ex) {

src/test/common/filesystem.unit.test.ts

@@ -0,0 +1,20 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+import assert from 'assert';
+import { isFileNotFoundError } from '../../common/utils/filesystem';
+
+suite('filesystem utilities', () => {
+    test('recognizes ENOENT errors', () => {
+        const error = Object.assign(new Error('missing'), { code: 'ENOENT' });
+
+        assert.strictEqual(isFileNotFoundError(error), true);
+    });
+
+    test('rejects other errors and non-errors', () => {
+        assert.strictEqual(isFileNotFoundError(Object.assign(new Error('not a directory'), { code: 'ENOTDIR' })), false);
+        assert.strictEqual(isFileNotFoundError(new Error('missing code')), false);
+        assert.strictEqual(isFileNotFoundError(undefined), false);
+        assert.strictEqual(isFileNotFoundError('ENOENT'), false);
+    });
+});