fix(deps): keep packages the running process loaded requireable across an install that re-keys them

[zkochan]

Extracted from #10465 so the fix can land on its own.

Problem

pnpm keys a virtual-store directory by the package's peer-resolution hash, so an install that changes the dependency set gives the same name@version a new directory and deletes the one this process loaded its modules from. Node keeps the loaded module objects, but not the files — so any require/import the loaded code defers past load time resolves against the deleted directory and throws MODULE_NOT_FOUND.

Every package the process loaded out of the workspace's own virtual store is exposed to this, and an env is the worst case: @teambit/aspect defers require('./babel/babel-config') until getCompiler() is called — which the install flow itself does right after the package-manager run, when it compiles components and reloads envs. The install then dies with Cannot find module './babel/babel-config'.

Replacing the in-memory instances instead does not work (verified by trying): every reload path — reloadMovedEnvs, loading components as aspects — has to consult the registered env to do its work, and consulting it is exactly what throws. reloadMovedEnvs is additionally a no-op for these envs: it filters on env.__path/env.id, which only plugin-loaded envs carry.

Fix

Follow the rule an OS applies to a running binary's deleted files: what the process has loaded stays available for the process's lifetime.

• Before the package-manager run, snapshot which virtual-store directories back loaded modules; afterwards, restore any that vanished from their re-keyed twin — same name@version, different peer hash — whose package content is identical (same tarball; the peer set only affects the sibling dependency symlinks, which are relative and stay valid from the restored location).
• pnpmPruneModules skips directories backing loaded modules so it does not re-delete a restored one. A later command's process, which has nothing loaded from it, prunes it.
• CJS modules are found via require.cache. ESM modules live in node's ESM module map, which has no enumeration API, so aspect-loader records every file it loads through dynamic import() (the env-plugin loader and loadEsm(), the only two such call sites) in a Symbol.for-keyed global set that the pnpm side reads. A global symbol rather than a shared import because the reader lives in the pnpm package manager, which aspect-loader must not depend on — the dependency runs the other way — and Symbol.for resolves to the same key even when a module is duplicated in node_modules. Both sides document the contract and point at each other.
• Restores run sequentially: this sits right after every install, where the engine has just saturated the disk, and each restore is a recursive copy. The common case is zero removed directories and the checks stay cheap.

Commits

Cherry-picked from #10465 (-x trailers reference the originals):

• fix(deps): keep packages the running process loaded requireable across an install that re-keys them
• fix(deps): extend loaded-package preservation across installs to ESM modules
• fix(deps): restore removed loaded virtual-store dirs sequentially

#10465 also carries fix(generator): stop reporting an env-load failure as "template not found", which is not included here: it fixes getTemplateWithIdOrEnvFallback, a method that only exists on that branch, so there is nothing on master for it to apply to. It stays with #10465.

Verification

• bit test teambit.harmony/aspect-loader teambit.dependencies/pnpm — 55/55 passing (4 new specs for the ESM recorder, 6 new for the snapshot/donor/prune logic).
• npm run lint (tsc + oxlint) green.
• Behavior verified on feat(envs): remove core envs from the manifest and their sources from the workspace #10465 with deps-in-capsules.e2e.ts: the second install re-keys a dozen loaded @teambit/* slots; without the fix the suite fails on the babel-config require, with it all tests pass and the debug log shows each removed slot restored.

🤖 Generated with Claude Code

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Preserve loaded pnpm virtual-store packages across peer-hash rekeys

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Snapshot virtual-store dirs backing loaded modules before pnpm install and restore removed ones.
• Record ESM dynamic-import entry files so preservation also covers ESM-loaded envs/plugins.
• Prevent prune from deleting virtual-store dirs still used by the current process.

Diagram

graph TD
  A["PnpmPackageManager.install"] --> B["preserveLoadedVirtualStoreDirs"] --> C["pnpm install"] --> B
  D["pnpmPruneModules"] --> B
  E["AspectLoader"] --> F["recordLoadedEsmFile"] --> G[("global ESM set")]
  G --> B --> H[("node_modules/.pnpm")]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Disable virtual-store pruning for the current command

• ➕ Simpler than restoring directories
• ➕ Avoids copy cost and donor selection logic
• ➖ Leaves stale peer-hash slots behind indefinitely in long-lived processes
• ➖ Still requires special casing to avoid breaking deferred requires post-install

2. Restart the process after install (fresh module graph)

• ➕ Eliminates need to preserve deleted on-disk module files
• ➖ Often infeasible for Bit flows that continue in-process after install
• ➖ Would disrupt CLI/server workflows and increase latency/complexity

3. Eagerly load known deferred-require targets pre-install

• ➕ No filesystem copying; keeps runtime state consistent
• ➖ Not general: requires knowing/maintaining a list of deferred imports/config reads
• ➖ Doesn’t help arbitrary user/env code paths and still misses ESM dynamic imports

Recommendation: Keep the PR’s approach: snapshot + best-effort restore of removed virtual-store dirs is the most general fix that preserves Node’s runtime expectations without requiring process restarts or fragile heuristics. The added ESM recording contract is a pragmatic way to cover dynamic imports given Node’s lack of ESM module-map enumeration, and prune-skipping prevents immediate re-breakage in the same process.

Files changed (8) +386 / -1

Bug fix (6) +259 / -1

pnpm-prune-modules.tsSkip pruning virtual-store dirs backing loaded modules +12/-1

Skip pruning virtual-store dirs backing loaded modules

• Updates pnpm prune logic to avoid deleting .pnpm slot directories that the current process has loaded modules from. This prevents re-deleting directories restored after a peer-hash re-key install.

scopes/dependencies/pnpm/pnpm-prune-modules.ts

pnpm.package-manager.tsSnapshot/restore loaded virtual-store dirs around pnpm install +8/-0

Snapshot/restore loaded virtual-store dirs around pnpm install

• Captures the set of virtual-store directories backing loaded modules before running pnpm. After installation completes, restores any removed loaded directories to keep deferred requires/imports working.

scopes/dependencies/pnpm/pnpm.package-manager.ts

preserve-loaded-virtual-store-dirs.tsImplement preservation of loaded .pnpm slots across re-key installs +198/-0

Implement preservation of loaded .pnpm slots across re-key installs

• Adds snapshot and restore utilities that detect virtual-store slots backing loaded modules (CJS via require.cache, ESM via a global Symbol-keyed set) and restore removed slots by copying from same-version peer-hash donors. Provides helper utilities for prune integration and donor discovery.

scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts

aspect-loader.main.runtime.tsRecord ESM loads in AspectLoader.loadEsm() +2/-0

Record ESM loads in AspectLoader.loadEsm()

• Hooks the general ESM loader path to record dynamically-imported entry files. This enables pnpm’s loaded-slot preservation logic to include ESM modules.

scopes/harmony/aspect-loader/aspect-loader.main.runtime.ts

plugins.tsRecord resolved plugin ESM entry file before dynamic import +2/-0

Record resolved plugin ESM entry file before dynamic import

• Records the resolved real path used for ESM plugin loading so preserved slots can be attributed to the correct .pnpm directory. Complements the main AspectLoader ESM recording path.

scopes/harmony/aspect-loader/plugins.ts

record-loaded-esm-file.tsAdd global ESM entry-file recorder for cross-package scanning +37/-0

Add global ESM entry-file recorder for cross-package scanning

• Implements a best-effort recorder that writes ESM entry file paths (and their realpaths) to a global Symbol.for-keyed Set. This bridges aspect-loader dynamic imports to pnpm’s preservation logic without creating a dependency edge in the wrong direction.

scopes/harmony/aspect-loader/record-loaded-esm-file.ts

Tests (2) +127 / -0

preserve-loaded-virtual-store-dirs.spec.tsAdd unit tests for donor selection and loaded-dir scanning +95/-0

Add unit tests for donor selection and loaded-dir scanning

• Introduces specs for donor directory resolution across peer-hash changes and for attributing loaded CJS/ESM module files to virtual-store slot dirs. Validates interaction with the global ESM-recording contract.

scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.spec.ts

record-loaded-esm-file.spec.tsAdd unit tests for ESM load recording contract +32/-0

Add unit tests for ESM load recording contract

• Adds tests ensuring the global Symbol-keyed Set is created, populated, and resilient to missing paths. Verifies best-effort realpath recording behavior.

scopes/harmony/aspect-loader/record-loaded-esm-file.spec.ts

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Symlink breaks dir detection ✓ Resolved 🐞 Bug ≡ Correctness

Description

snapshotLoadedVirtualStoreDirs() and loadedVirtualStoreDirNames() build the .pnpm prefix with
path.resolve() and then do startsWith(prefix), but loaded module filenames are
documented/recorded as realpaths. If the workspace is accessed via a symlink, the prefix can differ
from require.cache/recorded ESM paths, so loaded directories won’t be detected and the install can
still hit MODULE_NOT_FOUND after re-key/prune.

Code

scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[R64-66]

+  const virtualStoreDir = path.join(path.resolve(rootDir), 'node_modules', '.pnpm');
+  const prefix = `${virtualStoreDir}${path.sep}`;
+  const byDirName = new Map<string, LoadedVirtualStoreDir>();

Evidence

The new code explicitly states that require.cache keys are realpaths, but it computes its .pnpm
prefix from path.resolve(rootDir) / path.resolve(virtualStoreDir), which won’t match
realpath-based module filenames when the workspace path is symlinked. The repo already documents
symlinked workspaces as a real case and uses fs.realpath() to avoid incorrect path comparisons,
and the aspect-loader plugin loader also realpaths paths to avoid cached-resolution issues.

scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[58-75]
scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[158-166]
scopes/dependencies/dependency-resolver/dependency-installer.ts[241-246]
scopes/harmony/aspect-loader/plugins.ts[58-67]
scopes/harmony/aspect-loader/record-loaded-esm-file.ts[25-33]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The preservation/prune-skip logic compares loaded module filenames to a `.pnpm` prefix derived with `path.resolve()`, which does not dereference symlinks. Because loaded module filenames are treated as realpaths, a symlinked workspace root can make the prefix mismatch and cause loaded `.pnpm` slot dirs to be missed.
## Issue Context
This repo already handles symlinked workspace spellings elsewhere by comparing realpaths to avoid misclassification. The new preservation code should mirror that behavior to reliably detect loaded modules.
## Fix Focus Areas
- scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[63-76]
- scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[158-166]
- scopes/dependencies/pnpm/pnpm-prune-modules.ts[26-43]
- scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.spec.ts[48-95]
## What to change
1. In both `snapshotLoadedVirtualStoreDirs()` and `loadedVirtualStoreDirNames()`, derive a *canonical* virtual-store dir using `fs.realpath`/`fs.realpathSync` when available, with a fallback to the unresolved spelling when the dir doesn’t exist.
2. Accept matches against both spellings (symlink path and realpath) to stay compatible with scenarios like `--preserve-symlinks`.
3. Consider normalizing path casing on Windows (e.g., compare using `path.normalize()` and, on win32, lowercasing drive letter) before `startsWith`.
4. Add a regression test that creates a symlinked `rootDir` pointing at a real temp dir containing a `.pnpm` path, inserts a `require.cache` entry using the *realpath* spelling, and asserts `snapshotLoadedVirtualStoreDirs()` still finds it.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Stale ESM load records ✓ Resolved 🐞 Bug ☼ Reliability

Description

AspectLoaderMain.loadEsm() and Plugins.loadModule() record the path before awaiting esmLoader(), so
a rejected import leaves the path in the process-global set. pnpmPruneModules() then treats the
corresponding .pnpm slot as loaded and may skip pruning it for the rest of the process lifetime even
though nothing was successfully imported.

Code

scopes/harmony/aspect-loader/aspect-loader.main.runtime.ts[R538-541]

async loadEsm(path: string) {
+    recordLoadedEsmFile(path);
 return esmLoader(path);
}

Evidence

The PR adds recording immediately before the import in both ESM load paths, while pnpm prune
consults the recorded set (via loadedVirtualStoreDirNames) to decide what to delete; this makes
failed imports leave stale “loaded” directories in-memory for the remainder of the process.

scopes/harmony/aspect-loader/aspect-loader.main.runtime.ts[538-541]
scopes/harmony/aspect-loader/plugins.ts[58-75]
scopes/dependencies/pnpm/pnpm-prune-modules.ts[33-47]
scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[44-60]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`recordLoadedEsmFile()` is called before the dynamic import is known to have succeeded. If `esmLoader()` throws, the global record still contains that path, which later causes pnpm pruning logic to treat related virtual-store dirs as “loaded by this process” and skip cleanup.
### Issue Context
This PR introduces a cross-package contract via `Symbol.for('bit.loaded-esm-module-files')` so pnpm can preserve loaded packages across re-keying installs. The writer should only record successful loads.
### Fix Focus Areas
- scopes/harmony/aspect-loader/aspect-loader.main.runtime.ts[538-541]
- scopes/harmony/aspect-loader/plugins.ts[64-68]
### Suggested change
- In `AspectLoaderMain.loadEsm()`: call `recordLoadedEsmFile()` **after** `await esmLoader(...)` succeeds (or wrap with try/catch and delete the entry on failure).
- In `Plugins.loadModule()`: similarly, record only after `esmLoader(realPath, true)` succeeds (or remove on failure).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Slot pkgName misattributed ✓ Resolved 🐞 Bug ≡ Correctness

Description

snapshotLoadedVirtualStoreDirs() permanently binds a .pnpm slot dirName to the first pkgName parsed
from any loaded path under it; with symlink-preserving resolution (e.g. --preserve-symlinks), that
first path can be .pnpm/<parent-slot>/node_modules/<dep>/..., producing a pkgName that doesn't match
the slot. restoreRemovedLoadedVirtualStoreDirs() then fails to restore that removed loaded slot
because findDonorDirName() rejects dirName/pkName prefix mismatches, so deferred requires can still
throw MODULE_NOT_FOUND after a re-keyed install.

Code

scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[R112-115]

+  for (const { storeDir, segments } of loadedFilesUnderVirtualStore(virtualStoreDir)) {
+    const dirName = segments[0];
+    if (!dirName || byDirName.has(dirName)) continue;
+    const pkgName = parsePkgName(segments);

Evidence

The snapshot is currently first-hit-wins per slot dirName, even if the parsed pkgName doesn't
correspond to that slot; donor selection later hard-requires the dirName prefix to match the escaped
pkgName, so a mismatched snapshot makes restoration impossible for that removed dir.

scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[109-121]
scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[226-230]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`snapshotLoadedVirtualStoreDirs()` records the first loaded path encountered for a given `.pnpm/<dirName>` slot and stores `pkgName` parsed from that path. In symlink-preserving scenarios, the first loaded path under a slot can be inside that slot's `node_modules/<dep>` symlink tree, so `parsePkgName()` returns the dependency name, not the slot's owning package name. Later, `findDonorDirName()` requires `missingDirName` to start with the escaped `pkgName`, so restoration becomes impossible for that slot.
### Issue Context
This manifests when module filenames/URLs retain dependency-symlink spellings (not realpaths), which the code comments explicitly aims to support.
### Fix Focus Areas
- scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[109-121]
### Suggested fix
In `snapshotLoadedVirtualStoreDirs()`:
1. After `pkgName = parsePkgName(segments)`, compute `escaped = pkgName.replace(/\//g, '+')` and **only accept** this `pkgName` for `dirName` if `dirName.startsWith(`${escaped}@`)`.
2. If `byDirName` already has an entry for `dirName` but it was derived from a non-matching `pkgName`, allow replacing it when you later encounter a matching `(dirName, pkgName)` pair.
This keeps snapshotting order-independent and guarantees `findDonorDirName(dirName, pkgName, ...)` can succeed when a donor exists.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Unchecked ESM global value ✓ Resolved 🐞 Bug ☼ Reliability

Description

loadedModuleFiles() spreads globalThis[Symbol.for('bit.loaded-esm-module-files')] assuming it’s a
Set; if it’s any other truthy value, snapshotLoadedVirtualStoreDirs()/loadedVirtualStoreDirNames()
can throw and crash installs/prunes. recordLoadedEsmFile() also won’t repair an existing wrong-typed
value because it uses ??= and then calls .add().

Code

scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[R44-47]

+function loadedModuleFiles(): string[] {
+  const esmFiles = (globalThis as { [LOADED_ESM_FILES]?: Set<string> })[LOADED_ESM_FILES];
+  return esmFiles ? [...Object.keys(require.cache), ...esmFiles] : Object.keys(require.cache);
+}

Evidence

pnpm’s reader spreads the global symbol value without checking its runtime type, and aspect-loader’s
writer keeps an existing wrong-typed truthy value due to ??= and then attempts .add(). Together,
a corrupted global value can turn the preservation/prune scan into a hard failure.

scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[42-47]
scopes/harmony/aspect-loader/record-loaded-esm-file.ts[23-34]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`loadedModuleFiles()` assumes the global registry at `Symbol.for('bit.loaded-esm-module-files')` is a `Set<string>` and spreads it. If another copy/version or any other code sets a truthy non-Set value at that symbol, the spread can throw (or produce garbage), breaking installs/prunes.
### Issue Context
This is a cross-package global contract (aspect-loader writes, pnpm reads). Defensive runtime validation is needed at the boundary.
### Fix Focus Areas
- scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[44-47]
- scopes/harmony/aspect-loader/record-loaded-esm-file.ts[25-33]
### Suggested fix
- In `loadedModuleFiles()`, only use the value if it is a `Set` (or at least iterable of strings), otherwise treat it as absent.
- In `recordLoadedEsmFile()`, if the existing global value is not a `Set`, replace it with a new `Set` before calling `.add()`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View review recommended (1)

5. Donor content not verified ✓ Resolved 🐞 Bug ≡ Correctness

Description

restoreOneDir() picks a donor directory using findDonorDirName(), which matches only escaped package
name and version, but the module comment states restore should only happen when package content is
identical. If multiple same-version candidates exist with different contents, this can restore the
wrong variant and make deferred requires read mismatched on-disk files.

Code

scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[R211-214]

+  const version = missingDirName.slice(namePrefix.length).split('_')[0];
+  const exact = `${namePrefix}${version}`;
+  return currentDirs.find((dir) => dir !== missingDirName && (dir === exact || dir.startsWith(`${exact}_`)));
+}

Evidence

The module-level comment explicitly describes restoring only when content is identical, but the
actual implementation selects donors based solely on name@version and only checks that the donor
contains node_modules/. The install pipeline also passes patchedDependencies, demonstrating that
content-affecting installation variants exist in this codebase.

scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[23-27]
scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[160-175]
scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[207-214]
scopes/dependencies/pnpm/pnpm.package-manager.ts[235-241]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The code restores a removed slot dir by copying from the first donor that matches only `name@version`. This does not enforce the documented assumption that the donor has identical package contents.
### Issue Context
The codebase already supports installation options like `patchedDependencies`, and (more generally) ecosystems can contain multiple same-version variants. Restoring from the wrong donor can cause the running process to observe inconsistent disk contents vs. already-loaded module objects.
### Fix Focus Areas
- scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[112-175]
- scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[198-214]
- scopes/dependencies/pnpm/pnpm.package-manager.ts[235-241]
### Suggested fix
- Extend the snapshot to record a lightweight content fingerprint for each loaded package directory *before* install (e.g., hash of `<dirPath>/node_modules/<pkgName>/package.json` and/or another stable metadata file).
- When restoring, evaluate all donor candidates for the same `name@version` and pick the one whose fingerprint matches; if none match, skip restore and log debug (best-effort semantics preserved).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Prune scans cache always ✓ Resolved 🐞 Bug ➹ Performance

Description

pnpmPruneModules() now scans require.cache (and the global ESM-record set) even when there are no
extraneous virtual-store dirs to remove, adding avoidable O(loaded modules) work to no-op prunes.
This overhead is introduced by computing loadedVirtualStoreDirNames() unconditionally before knowing
whether difference(pkgDirs, dirsShouldBePresent) is empty.

Code

scopes/dependencies/pnpm/pnpm-prune-modules.ts[R38-42]

+  const loadedByThisProcess = loadedVirtualStoreDirNames(virtualStoreDir);
+  await Promise.all(
+    difference(pkgDirs, dirsShouldBePresent)
+      .filter((dir) => !loadedByThisProcess.has(dir))
+      .map((dir) => fs.remove(path.join(virtualStoreDir, dir)))

Evidence

The prune code computes loadedVirtualStoreDirNames() before filtering deletions, and that helper
walks the loaded module file list. This means the scan happens even when `difference(pkgDirs,
dirsShouldBePresent)` is empty (no deletions).

scopes/dependencies/pnpm/pnpm-prune-modules.ts[26-43]
scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[44-47]
scopes/dependencies/pnpm/preserve-loaded-virtual-store-dirs.ts[190-195]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`pnpmPruneModules()` always scans loaded modules to compute `loadedByThisProcess`, even when there are no directories to prune.
### Issue Context
The loaded-module scan walks `Object.keys(require.cache)` (+ recorded ESM paths) and does prefix checks; this can be non-trivial in long-lived processes.
### Fix Focus Areas
- scopes/dependencies/pnpm/pnpm-prune-modules.ts[26-43]
### Suggested fix
- Compute `const extraneous = difference(pkgDirs, dirsShouldBePresent);` first.
- If `extraneous.length === 0`, return early.
- Only then compute `loadedByThisProcess` and filter `extraneous` against it.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[zkochan]

Fixed in 63ccfc1 — the finding was correct.

require.cache is keyed by the module's realpath, so comparing against a prefix built from path.resolve(rootDir) matched nothing whenever the workspace was reached through a symlink: the snapshot came back empty and the whole preservation silently became a no-op. macOS makes that the common case rather than an edge one — a temp dir under /var is really under /private/var, so every e2e workspace hits it.

Both scanners now share one walker that matches against the given spelling and its realpath (the given one kept for --preserve-symlinks), and each slot is recorded the way the file that revealed it was spelled, so the existence check and the restore address the directory the module was actually loaded from.

Two regression specs added: they fail on the previous code with an empty result and pass now.

I skipped the suggested Windows drive-letter case normalization — both sides of the comparison come from the same path APIs there, and nothing else in this repo normalizes casing, so it would be speculative.

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 63ccfc1

[zkochan]

All three addressed in 18254b4.

1. Donor content not verified — real, and I could pin down exactly when. depPathToDirName turns foo@1.0.0(patch_hash=abc) into foo@1.0.0_patch_hash=abc, i.e. a patch lands in the same _ suffix as the peer set, so foo@1.0.0_patch_hash=abc was an accepted donor for foo@1.0.0. That is the one case where the donor's files genuinely differ from what the process loaded — a peer set only changes the sibling symlinks. findDonorDirName now requires the patch_hash= segment to be equal; when it isn't, no donor is found and the restore is skipped, which is exactly the behavior this PR started from.

I did not take the pre-install fingerprint route from the suggestion. It would add a file read per loaded slot to every install to cover a case the dir name already answers for free, and the snapshot is deliberately pure in-memory so it costs nothing when there is nothing to preserve.

2. Unchecked ESM global value — agreed, and the reader was the dangerous half: it spread the value unguarded, so a non-iterable there would throw out of every install and prune. It now treats anything that isn't a set of paths as absent (CJS preservation still works) and filters non-string entries. The recorder replaces a wrong-typed value instead of losing every later add() — it was already inside a try, so it failed silently rather than crashing, but it never recovered either.

3. Prune scans cache always — fair. pnpmPruneModules now computes the extraneous set first and returns before scanning when it's empty, which is the common case.

Six new specs cover the three changes.

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 18254b4

[zkochan]

Fixed in 34c4137 — correct, and the invariant was already implied, just never enforced where it was decided.

A slot holds its dependencies as symlinks under the same node_modules, so a loaded path that kept that spelling instead of being realpathed names the dependency, not the package the slot is keyed by. findDonorDirName then rejects the pair, so that slot is never restored and the deferred requires it exists to protect still throw. The snapshot now prefers whichever loaded path names the owner, independent of arrival order.

On the second half of the suggestion — I kept a non-owner attribution as a fallback rather than dropping the entry. A slot named after something other than <pkg>@<version> (a tarball or git dependency) has no path that can match it, and dropping it would change a restore that was already impossible into a missing snapshot entry for no gain. The prefix test both call sites need is now a single helper, so they cannot drift.

Two specs: the first fails on the previous code with expected 'lodash' to equal '@teambit/aspect', the second pins the fallback.

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 34c4137

[zkochan]

Fixed in 696c727. Both call sites now record after the import() resolves.

The consequence is bounded — a slot nothing is using survives one extra prune — but the record is supposed to mean "this process has something loaded from here", and a rejected import leaves nothing in memory whose files need to stay around. The recorder's contract now says so, since it is the only thing keeping the two writers honest.

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 696c727