Address second round of PR #5749 review feedback

- Use readdirSync with { withFileTypes: true } to match only files
- Use lockfile.shrinkwrapFileMajorVersion directly (no cast needed)
- Extract extractNameAndVersionFromKey helper with unit tests
- Add comment explaining why depPathToFilename is inlined vs @pnpm/dependency-path

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

Author: bmiddha

rush-plugins/rush-resolver-cache-plugin/src/afterInstallAsync.ts

@@ -165,10 +165,12 @@ export async function afterInstallAsync(
       const dir: string = `${pnpmStoreDir}${hashDir}/`;
       const filePrefix: string = `${hashRest}-`;
       try {
-        const files: string[] = readdirSync(dir);
-        const match: string | undefined = files.find((f) => f.startsWith(filePrefix));
+        const entries: import('node:fs').Dirent[] = readdirSync(dir, { withFileTypes: true });
+        const match: import('node:fs').Dirent | undefined = entries.find(
+          (e) => e.isFile() && e.name.startsWith(filePrefix)
+        );
         if (match) {
-          indexPath = dir + match;
+          indexPath = dir + match.name;
         }
       } catch {
         // ignore

rush-plugins/rush-resolver-cache-plugin/src/computeResolverCacheFromLockfileAsync.ts

@@ -9,7 +9,12 @@ import type {
 } from '@rushstack/webpack-workspace-resolve-plugin';
 
 import type { PnpmShrinkwrapFile } from './externals';
-import { getDescriptionFileRootFromKey, resolveDependencies, createContextSerializer } from './helpers';
+import {
+  getDescriptionFileRootFromKey,
+  resolveDependencies,
+  createContextSerializer,
+  extractNameAndVersionFromKey
+} from './helpers';
 import type { IResolverContext } from './types';
 
 /**
@@ -157,11 +162,10 @@ function convertToSlashes(path: string): string {
  * or inspecting the first non-file package key.
  */
 function detectV9Lockfile(lockfile: PnpmShrinkwrapFile): boolean {
-  const majorVersion: number | undefined = (lockfile as { shrinkwrapFileMajorVersion?: number })
-    .shrinkwrapFileMajorVersion;
-  if (majorVersion !== undefined) {
-    return majorVersion >= 9;
+  if (lockfile.shrinkwrapFileMajorVersion > 0) {
+    return lockfile.shrinkwrapFileMajorVersion >= 9;
   }
+  // Fallback for lockfiles where version parsing failed: inspect the first non-file package key.
   for (const key of lockfile.packages.keys()) {
     if (!key.startsWith('file:')) {
       return !key.startsWith('/');
@@ -203,17 +207,10 @@ export async function computeResolverCacheFromLockfileAsync(
     const integrity: string | undefined = pack.resolution?.integrity;
 
     // Extract name and version from the key if not already provided
-    let version: string | undefined;
-    if (!key.startsWith('file:')) {
-      const offset: number = key.startsWith('/') ? 1 : 0;
-      const versionAtIndex: number = key.indexOf('@', offset + 1);
-      if (versionAtIndex !== -1) {
-        if (!name) {
-          name = key.slice(offset, versionAtIndex);
-        }
-        const parenIndex: number = key.indexOf('(', versionAtIndex);
-        version =
-          parenIndex !== -1 ? key.slice(versionAtIndex + 1, parenIndex) : key.slice(versionAtIndex + 1);
+    const parsed: { name: string; version: string } | undefined = extractNameAndVersionFromKey(key);
+    if (parsed) {
+      if (!name) {
+        name = parsed.name;
       }
     }
 
@@ -226,7 +223,7 @@ export async function computeResolverCacheFromLockfileAsync(
       descriptionFileHash: integrity,
       isProject: false,
       name,
-      version,
+      version: parsed?.version,
       deps: new Map(),
       ordinal: -1,
       optional: pack.optional

rush-plugins/rush-resolver-cache-plugin/src/helpers.ts

@@ -57,6 +57,9 @@ export function createShortSha256Hash(input: string): string {
   return createHash('sha256').update(input).digest('hex').substring(0, 32);
 }
 
+// Mirrors @pnpm/dependency-path depPathToFilename.
+// We inline it rather than importing two aliased versions of the package (v8 and v10)
+// since each brings transitive deps (@pnpm/crypto.hash, @pnpm/types, semver).
 // https://github.com/pnpm/pnpm/blob/f394cfccda7bc519ceee8c33fc9b68a0f4235532/packages/dependency-path/src/index.ts#L167-L189
 export function depPathToFilename(depPath: string, usePnpm10Hashing?: boolean): string {
   let filename: string = depPathToFilenameUnescaped(depPath).replace(SPECIAL_CHARS_REGEX, '+');
@@ -161,6 +164,27 @@ export function resolveDependencies(
   }
 }
 
+/**
+ * Extracts the package name and version from a lockfile package key.
+ * @param key - The lockfile package key (e.g. '/autoprefixer\@9.8.8', '\@scope/name\@1.0.0(peer\@2.0.0)')
+ * @returns The extracted name and version, or undefined for file: keys
+ */
+export function extractNameAndVersionFromKey(key: string): { name: string; version: string } | undefined {
+  if (key.startsWith('file:')) {
+    return undefined;
+  }
+  const offset: number = key.startsWith('/') ? 1 : 0;
+  const versionAtIndex: number = key.indexOf('@', offset + 1);
+  if (versionAtIndex === -1) {
+    return undefined;
+  }
+  const name: string = key.slice(offset, versionAtIndex);
+  const parenIndex: number = key.indexOf('(', versionAtIndex);
+  const version: string =
+    parenIndex !== -1 ? key.slice(versionAtIndex + 1, parenIndex) : key.slice(versionAtIndex + 1);
+  return { name, version };
+}
+
 /**
  *
  * @param depPath - The path to the dependency

rush-plugins/rush-resolver-cache-plugin/src/test/helpers.test.ts

@@ -5,7 +5,8 @@ import {
   createBase32Hash,
   createShortSha256Hash,
   depPathToFilename,
-  getDescriptionFileRootFromKey
+  getDescriptionFileRootFromKey,
+  extractNameAndVersionFromKey
 } from '../helpers';
 
 describe(createBase32Hash.name, () => {
@@ -101,3 +102,54 @@ describe(getDescriptionFileRootFromKey.name, () => {
     }
   });
 });
+
+describe(extractNameAndVersionFromKey.name, () => {
+  it('extracts name and version from v6 keys (leading /)', () => {
+    expect(extractNameAndVersionFromKey('/autoprefixer@9.8.8')).toEqual({
+      name: 'autoprefixer',
+      version: '9.8.8'
+    });
+    expect(extractNameAndVersionFromKey('/autoprefixer@10.4.18(postcss@8.4.36)')).toEqual({
+      name: 'autoprefixer',
+      version: '10.4.18'
+    });
+    expect(extractNameAndVersionFromKey('/@some/package@1.2.3(@azure/msal-browser@2.28.1)')).toEqual({
+      name: '@some/package',
+      version: '1.2.3'
+    });
+    expect(
+      extractNameAndVersionFromKey('/@typescript-eslint/utils@6.19.1(eslint@7.7.0)(typescript@5.4.2)')
+    ).toEqual({
+      name: '@typescript-eslint/utils',
+      version: '6.19.1'
+    });
+  });
+
+  it('extracts name and version from v9 keys (no leading /)', () => {
+    expect(extractNameAndVersionFromKey('autoprefixer@9.8.8')).toEqual({
+      name: 'autoprefixer',
+      version: '9.8.8'
+    });
+    expect(extractNameAndVersionFromKey('autoprefixer@10.4.18(postcss@8.4.36)')).toEqual({
+      name: 'autoprefixer',
+      version: '10.4.18'
+    });
+    expect(extractNameAndVersionFromKey('@some/package@1.2.3(@azure/msal-browser@2.28.1)')).toEqual({
+      name: '@some/package',
+      version: '1.2.3'
+    });
+    expect(
+      extractNameAndVersionFromKey('@typescript-eslint/utils@6.19.1(eslint@7.7.0)(typescript@5.4.2)')
+    ).toEqual({
+      name: '@typescript-eslint/utils',
+      version: '6.19.1'
+    });
+  });
+
+  it('returns undefined for file: keys', () => {
+    expect(extractNameAndVersionFromKey('file:../../../rigs/local-node-rig')).toBeUndefined();
+    expect(
+      extractNameAndVersionFromKey('file:../../../libraries/ts-command-line(@types/node@18.17.15)')
+    ).toBeUndefined();
+  });
+});