Address PR #5749 review feedback

- Hoist regexes in depPathToFilename to module-level constants
- Unify v6/v9 branches in getDescriptionFileRootFromKey using offset
- Check specifier against packages list instead of regex heuristic
- Extract detectV9Lockfile helper (iterates with early return, no clone)
- Store version on IResolverContext when parsing lockfile keys
- Extract getStoreIndexPath helper in afterInstallAsync
- Use context.version instead of .split() on hot path
- Fix undefined in snapshot test names

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

Author: bmiddha

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

@@ -143,6 +143,40 @@ export async function afterInstallAsync(
     // Ignore
   }
 
+  /**
+   * Computes the pnpm store index file path for a given package integrity hash.
+   */
+  function getStoreIndexPath(context: IResolverContext, hash: string): string {
+    if (!useV10Store) {
+      return `${pnpmStoreDir}${hash.slice(0, 2)}/${hash.slice(2)}-index.json`;
+    }
+
+    // pnpm 10 truncates integrity hashes to 32 bytes (64 hex chars) for index paths.
+    const truncHash: string = hash.length > 64 ? hash.slice(0, 64) : hash;
+    const hashDir: string = truncHash.slice(0, 2);
+    const hashRest: string = truncHash.slice(2);
+    // pnpm 10 index path format: <hash (0-2)>/<hash (2-64)>-<name>@<version>.json
+    const pkgName: string = (context.name || '').replace(/\//g, '+');
+    const nameVer: string = context.version ? `${pkgName}@${context.version}` : pkgName;
+    let indexPath: string = `${pnpmStoreDir}${hashDir}/${hashRest}-${nameVer}.json`;
+    // For truncated/hashed folder names, nameVer from the key may be wrong.
+    // Fallback: scan the directory for a file matching the hash prefix.
+    if (!existsSync(indexPath)) {
+      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));
+        if (match) {
+          indexPath = dir + match;
+        }
+      } catch {
+        // ignore
+      }
+    }
+    return indexPath;
+  }
+
   async function afterExternalPackagesAsync(
     contexts: Map<string, IResolverContext>,
     missingOptionalDependencies: Set<string>
@@ -173,50 +207,7 @@ export async function afterInstallAsync(
       const prefixIndex: number = descriptionFileHash.indexOf('-');
       const hash: string = Buffer.from(descriptionFileHash.slice(prefixIndex + 1), 'base64').toString('hex');
 
-      // The pnpm store directory has index files of package contents at paths:
-      // pnpm 8:  <store>/v3/files/<hash (0-2)>/<hash (2-)>-index.json
-      // pnpm 10: <store>/v10/index/<hash (0-2)>/<hash (2-64)>-<name>@<version>.json
-      // See https://github.com/pnpm/pnpm/blob/f394cfccda7bc519ceee8c33fc9b68a0f4235532/store/cafs/src/getFilePathInCafs.ts#L33
-      let indexPath: string;
-      if (useV10Store) {
-        // pnpm 10 truncates integrity hashes to 32 bytes (64 hex chars) for index paths.
-        const truncHash: string = hash.length > 64 ? hash.slice(0, 64) : hash;
-        const hashDir: string = truncHash.slice(0, 2);
-        const hashRest: string = truncHash.slice(2);
-        // Build the bare name@version using context.name and version from the .pnpm folder path.
-        // The .pnpm folder name format is: <name+ver>_<peer1>_<peer2>/node_modules/<name>
-        const pkgName: string = (context.name || '').replace(/\//g, '+');
-        const pnpmSegment: string | undefined = context.descriptionFileRoot.split('/node_modules/.pnpm/')[1];
-        const folderName: string = pnpmSegment ? pnpmSegment.split('/node_modules/')[0] : '';
-        const namePrefix: string = `${pkgName}@`;
-        const nameStart: number = folderName.indexOf(namePrefix);
-        let nameVer: string = folderName;
-        if (nameStart !== -1) {
-          const afterName: string = folderName.slice(nameStart + namePrefix.length);
-          // Version ends at first _ followed by a letter/@ (peer dep separator)
-          const peerSep: number = afterName.search(/_[a-zA-Z@]/);
-          const version: string = peerSep !== -1 ? afterName.slice(0, peerSep) : afterName;
-          nameVer = `${pkgName}@${version}`;
-        }
-        indexPath = `${pnpmStoreDir}${hashDir}/${hashRest}-${nameVer}.json`;
-        // For truncated/hashed folder names, nameVer from the folder path may be wrong.
-        // Fallback: scan the directory for a file matching the hash prefix.
-        if (!existsSync(indexPath)) {
-          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));
-            if (match) {
-              indexPath = dir + match;
-            }
-          } catch {
-            // ignore
-          }
-        }
-      } else {
-        indexPath = `${pnpmStoreDir}${hash.slice(0, 2)}/${hash.slice(2)}-index.json`;
-      }
+      const indexPath: string = getStoreIndexPath(context, hash);
 
       try {
         const indexContent: string = await FileSystem.readFileAsync(indexPath);

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

@@ -152,6 +152,24 @@ function convertToSlashes(path: string): string {
   return path.replace(/\\/g, '/');
 }
 
+/**
+ * Detects whether the lockfile uses v9+ format by checking the major version
+ * 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;
+  }
+  for (const key of lockfile.packages.keys()) {
+    if (!key.startsWith('file:')) {
+      return !key.startsWith('/');
+    }
+  }
+  return false;
+}
+
 /**
  * Given a lockfile and information about the workspace and platform, computes the resolver cache file.
  * @param params - The options for computing the resolver cache
@@ -169,14 +187,7 @@ export async function computeResolverCacheFromLockfileAsync(
   const contexts: Map<string, IResolverContext> = new Map();
   const missingOptionalDependencies: Set<string> = new Set();
 
-  // Detect v9+ lockfile format by checking if the lockfile has shrinkwrapFileMajorVersion >= 9,
-  // or by checking if the first package key lacks a leading '/' (v6 keys always start with '/').
-  const isV9Lockfile: boolean =
-    (lockfile as { shrinkwrapFileMajorVersion?: number }).shrinkwrapFileMajorVersion !== undefined
-      ? (lockfile as { shrinkwrapFileMajorVersion?: number }).shrinkwrapFileMajorVersion! >= 9
-      : !Array.from(lockfile.packages.keys())
-          .find((k) => !k.startsWith('file:'))
-          ?.startsWith('/');
+  const isV9Lockfile: boolean = detectV9Lockfile(lockfile);
 
   // Enumerate external dependencies first, to simplify looping over them for store data
   for (const [key, pack] of lockfile.packages) {
@@ -191,17 +202,18 @@ export async function computeResolverCacheFromLockfileAsync(
 
     const integrity: string | undefined = pack.resolution?.integrity;
 
-    if (!name && key.startsWith('/')) {
-      const versionIndex: number = key.indexOf('@', 2);
-      name = key.slice(1, versionIndex);
-    }
-
-    if (!name) {
-      // Handle v9 lockfile keys: @scope/name@version or name@version
-      const searchStart: number = key.startsWith('@') ? key.indexOf('/') + 1 : 0;
-      const versionIndex: number = key.indexOf('@', searchStart);
-      if (versionIndex !== -1) {
-        name = key.slice(0, versionIndex);
+    // 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);
       }
     }
 
@@ -214,6 +226,7 @@ export async function computeResolverCacheFromLockfileAsync(
       descriptionFileHash: integrity,
       isProject: false,
       name,
+      version,
       deps: new Map(),
       ordinal: -1,
       optional: pack.optional
@@ -222,10 +235,10 @@ export async function computeResolverCacheFromLockfileAsync(
     contexts.set(descriptionFileRoot, context);
 
     if (pack.dependencies) {
-      resolveDependencies(workspaceRoot, pack.dependencies, context, isV9Lockfile);
+      resolveDependencies(workspaceRoot, pack.dependencies, context, isV9Lockfile, lockfile.packages);
     }
     if (pack.optionalDependencies) {
-      resolveDependencies(workspaceRoot, pack.optionalDependencies, context, isV9Lockfile);
+      resolveDependencies(workspaceRoot, pack.optionalDependencies, context, isV9Lockfile, lockfile.packages);
     }
   }
 
@@ -266,13 +279,19 @@ export async function computeResolverCacheFromLockfileAsync(
     contexts.set(descriptionFileRoot, context);
 
     if (importer.dependencies) {
-      resolveDependencies(workspaceRoot, importer.dependencies, context, isV9Lockfile);
+      resolveDependencies(workspaceRoot, importer.dependencies, context, isV9Lockfile, lockfile.packages);
     }
     if (importer.devDependencies) {
-      resolveDependencies(workspaceRoot, importer.devDependencies, context, isV9Lockfile);
+      resolveDependencies(workspaceRoot, importer.devDependencies, context, isV9Lockfile, lockfile.packages);
     }
     if (importer.optionalDependencies) {
-      resolveDependencies(workspaceRoot, importer.optionalDependencies, context, isV9Lockfile);
+      resolveDependencies(
+        workspaceRoot,
+        importer.optionalDependencies,
+        context,
+        isV9Lockfile,
+        lockfile.packages
+      );
     }
   }
 

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

@@ -13,6 +13,12 @@ const PNPM8_MAX_LENGTH_WITHOUT_HASH: number = 120 - 26 - 1;
 const PNPM10_MAX_LENGTH_WITHOUT_HASH: number = 120 - 32 - 1;
 const BASE32: string[] = 'abcdefghijklmnopqrstuvwxyz234567'.split('');
 
+const SPECIAL_CHARS_REGEX: RegExp = /[\\/:*?"<>|]/g;
+const HASH_CHAR_REGEX: RegExp = /#/g;
+const TRAILING_PAREN_REGEX: RegExp = /\)$/;
+const PNPM10_PARENS_REGEX: RegExp = /\)\(|\(|\)/g;
+const PNPM8_PARENS_REGEX: RegExp = /(\)\()|\(/g;
+
 // https://github.com/swansontec/rfc4648.js/blob/ead9c9b4b68e5d4a529f32925da02c02984e772c/src/codec.ts#L82-L118
 export function createBase32Hash(input: string): string {
   const data: Buffer = createHash('md5').update(input).digest();
@@ -53,19 +59,19 @@ export function createShortSha256Hash(input: string): string {
 
 // 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(/[\\/:*?"<>|]/g, '+');
+  let filename: string = depPathToFilenameUnescaped(depPath).replace(SPECIAL_CHARS_REGEX, '+');
   if (usePnpm10Hashing) {
     // pnpm 10 also replaces `#` and handles parentheses differently
-    filename = filename.replace(/#/g, '+');
+    filename = filename.replace(HASH_CHAR_REGEX, '+');
     if (filename.includes('(')) {
-      filename = filename.replace(/\)$/, '').replace(/\)\(|\(|\)/g, '_');
+      filename = filename.replace(TRAILING_PAREN_REGEX, '').replace(PNPM10_PARENS_REGEX, '_');
     }
     if (filename.length > 120 || (filename !== filename.toLowerCase() && !filename.startsWith('file+'))) {
       return `${filename.substring(0, PNPM10_MAX_LENGTH_WITHOUT_HASH)}_${createShortSha256Hash(filename)}`;
     }
   } else {
     if (filename.includes('(')) {
-      filename = filename.replace(/(\)\()|\(/g, '_').replace(/\)$/, '');
+      filename = filename.replace(PNPM8_PARENS_REGEX, '_').replace(TRAILING_PAREN_REGEX, '');
     }
     if (filename.length > 120 || (filename !== filename.toLowerCase() && !filename.startsWith('file+'))) {
       return `${filename.substring(0, PNPM8_MAX_LENGTH_WITHOUT_HASH)}_${createBase32Hash(filename)}`;
@@ -87,25 +93,21 @@ export function resolveDependencyKey(
   key: string,
   specifier: string,
   context: IResolverContext,
-  isV9Lockfile?: boolean
+  isV9Lockfile?: boolean,
+  packageKeys?: { has(key: string): boolean }
 ): string {
-  if (specifier.startsWith('/')) {
-    return getDescriptionFileRootFromKey(lockfileFolder, specifier);
-  } else if (specifier.startsWith('link:')) {
+  if (specifier.startsWith('link:')) {
     if (context.isProject) {
       return path.posix.join(context.descriptionFileRoot, specifier.slice(5));
     } else {
       return path.posix.join(lockfileFolder, specifier.slice(5));
     }
   } else if (specifier.startsWith('file:')) {
     return getDescriptionFileRootFromKey(lockfileFolder, specifier, key);
+  } else if (packageKeys?.has(specifier)) {
+    // The specifier is a full package key (v6: '/pkg@ver', v9: 'pkg@ver')
+    return getDescriptionFileRootFromKey(lockfileFolder, specifier);
   } else {
-    // In v9 lockfiles, aliased dependency values use the full package key format
-    // (e.g., 'string-width@4.2.3' or '@types/events@3.0.0') instead of bare versions.
-    // A bare version starts with a digit; a full key starts with a letter or @.
-    if (/^[a-zA-Z@]/.test(specifier)) {
-      return getDescriptionFileRootFromKey(lockfileFolder, specifier);
-    }
     // Construct the full dependency key from package name and version specifier.
     // v6 keys use '/' prefix; v9 keys don't.
     const fullKey: string = isV9Lockfile ? `${key}@${specifier}` : `/${key}@${specifier}`;
@@ -124,18 +126,9 @@ export function getDescriptionFileRootFromKey(lockfileFolder: string, key: strin
   // Detect lockfile version: v6 keys start with '/', v9 keys don't
   const isV9Key: boolean = !key.startsWith('/') && !key.startsWith('file:');
 
-  if (!key.startsWith('file:')) {
-    if (key.startsWith('/')) {
-      // v6 format: /name@version or /@scope/name@version
-      name = key.slice(1, key.indexOf('@', 2));
-    } else if (!name) {
-      // v9 format: name@version or @scope/name@version
-      const searchStart: number = key.startsWith('@') ? key.indexOf('/') + 1 : 0;
-      const versionIndex: number = key.indexOf('@', searchStart);
-      if (versionIndex !== -1) {
-        name = key.slice(0, versionIndex);
-      }
-    }
+  if (!key.startsWith('file:') && !name) {
+    const offset: number = key.startsWith('/') ? 1 : 0;
+    name = key.slice(offset, key.indexOf('@', offset + 1));
   }
   if (!name) {
     throw new Error(`Missing package name for ${key}`);
@@ -150,11 +143,19 @@ export function resolveDependencies(
   lockfileFolder: string,
   collection: Record<string, IDependencyEntry>,
   context: IResolverContext,
-  isV9Lockfile?: boolean
+  isV9Lockfile?: boolean,
+  packageKeys?: { has(key: string): boolean }
 ): void {
   for (const [key, value] of Object.entries(collection)) {
     const version: string = typeof value === 'string' ? value : value.version;
-    const resolved: string = resolveDependencyKey(lockfileFolder, key, version, context, isV9Lockfile);
+    const resolved: string = resolveDependencyKey(
+      lockfileFolder,
+      key,
+      version,
+      context,
+      isV9Lockfile,
+      packageKeys
+    );
 
     context.deps.set(key, resolved);
   }