fix: record the global virtual store dir in .modules.yaml during the post-install build step#12308
Conversation
|
💖 Thanks for opening this pull request! 💖 |
Code Review by Qodo
1. Hardcoded store version
|
📝 WalkthroughWalkthroughextendBuildOptions now conditionally sets ChangesGlobal virtual store behavior
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoFix GVS build step to preserve global virtualStoreDir in .modules.yaml WalkthroughsDescription• Make post-install build options GVS-aware to avoid virtual-store mismatches. • Ensure build step records /links in .modules.yaml under GVS. • Prevent subsequent workspace installs from prompting to purge node_modules. Diagramgraph TD
A["Post-install build"] --> B["extendBuildOptions"] --> C["getContext()"] --> D["writeModulesManifest"] --> E[".modules.yaml"]
B --> F{GVS enabled?}
F -->|Yes| G["virtualStoreDir = <storeDir>/links"] --> C
F -->|No| H["default: node_modules/.pnpm"] --> C
subgraph Legend
direction LR
_proc[Process] ~~~ _fn[Function] ~~~ _dec{Decision} ~~~ _file[File]
end
High-Level AssessmentThe following are alternative approaches to this PR: 1. Patch callers to pass virtualStoreDir explicitly
2. Rely on existing manifest value when rewriting .modules.yaml
3. Move GVS virtualStoreDir derivation into getContext()
Recommendation: The chosen approach—making extendBuildOptions mirror extendInstallOptions under enableGlobalVirtualStore—is the best tradeoff. It fixes the root cause (incorrect defaulting before getContext()) with minimal scope, and automatically covers both buildProjects and buildSelectedPkgs without duplicating logic at call sites. File ChangesBug fix (1)
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
building/after-install/src/extendBuildOptions.ts (1)
112-118: ⚡ Quick winTrim the inline comment to invariant-focused intent.
This note reads like incident history. Keep only the non-obvious invariant and move the narrative to PR/issue context.
Suggested edit
- // Mirror extendInstallOptions: under a global virtual store, the virtual - // store directory is `<storeDir>/links`, not the per-project - // `node_modules/.pnpm`. Without this, getContext() in the build step - // defaults virtualStoreDir to the local `.pnpm` and writeModulesManifest - // overwrites the correct value the install step recorded — which makes the - // next install in that project detect a virtual-store mismatch and prompt - // to purge node_modules. + // Keep build-step virtualStoreDir aligned with install-step GVS resolution. + // Under GVS, default to `<storeDir>/links` (or explicit globalVirtualStoreDir).As per coding guidelines, “Do not record past implementation shape, refactor history, or removed code in comments; use git log and git blame for that information instead.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@building/after-install/src/extendBuildOptions.ts` around lines 112 - 118, The existing inline comment in extendBuildOptions.ts contains historical narrative; replace it with a concise invariant-only comment that states: under a global virtual store the virtual store directory is "<storeDir>/links" (not "node_modules/.pnpm") and therefore getContext()/writeModulesManifest must preserve the install-step virtualStoreDir to avoid virtual-store mismatches. Remove the story/history text and keep only this minimal, non-obvious invariant and its direct consequence so future readers understand why the value must be preserved.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@building/after-install/src/extendBuildOptions.ts`:
- Around line 112-118: The existing inline comment in extendBuildOptions.ts
contains historical narrative; replace it with a concise invariant-only comment
that states: under a global virtual store the virtual store directory is
"<storeDir>/links" (not "node_modules/.pnpm") and therefore
getContext()/writeModulesManifest must preserve the install-step virtualStoreDir
to avoid virtual-store mismatches. Remove the story/history text and keep only
this minimal, non-obvious invariant and its direct consequence so future readers
understand why the value must be preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 321fea68-fea6-41ca-aa3a-725a59b789f5
📒 Files selected for processing (1)
building/after-install/src/extendBuildOptions.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Compile & Lint
- GitHub Check: Analyze (javascript)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use Standard Style with trailing commas, prefer functions over classes, declare functions after they are used (relying on hoisting), limit functions to no more than two or three arguments, and use a single options object for functions needing more parameters
Follow import order: standard libraries first, then external dependencies (sorted alphabetically), then relative imports
Do not write comments that restate what the code already says; rename variables, split helpers, or move checks to more obvious places instead
Do not repeat documentation at call sites that already lives on the callee; update the JSDoc once and let every call site benefit
Use JSDoc for the function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the function body
Do not record past implementation shape, refactor history, or removed code in comments; use git log and git blame for that information instead
Write comments only when the reason for code is non-obvious, a hidden invariant exists, a workaround for a known bug is needed, or an exception to surrounding pattern is deliberate
Files:
building/after-install/src/extendBuildOptions.ts
🧠 Learnings (2)
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
building/after-install/src/extendBuildOptions.ts
📚 Learning: 2026-06-05T13:47:26.046Z
Learnt from: vsumner
Repo: pnpm/pnpm PR: 12190
File: installing/deps-installer/src/install/index.ts:2337-2343
Timestamp: 2026-06-05T13:47:26.046Z
Learning: In the pnpm/pnpm codebase, `PnpmError` automatically prefixes `err.code` with `ERR_PNPM_` when you pass a code that does not already start with `ERR_PNPM_` (it normalizes `this.code` via `code.startsWith('ERR_PNPM_') ? code : `ERR_PNPM_${code}``). Therefore, during code review you should NOT flag `new PnpmError(...)` call sites for passing a bare error code (e.g., `new PnpmError('FROZEN_STORE_INCOMPATIBLE_WITH_PNPR', ...)`); the resulting `err.code` will still be `ERR_PNPM_FROZEN_STORE_INCOMPATIBLE_WITH_PNPR`.
Applied to files:
building/after-install/src/extendBuildOptions.ts
🔇 Additional comments (1)
building/after-install/src/extendBuildOptions.ts (1)
52-52: LGTM!Also applies to: 119-121
671244f to
701a6be
Compare
|
Code review by qodo was updated up to the latest commit 701a6be |
Add an e2e regression test for the post-install build step preserving the global virtual store directory in a workspace package's .modules.yaml, and the missing changeset for the fix (pnpm#12307).
|
Code review by qodo was updated up to the latest commit 6069d24 |
Fixes #12307.
Under
enableGlobalVirtualStore, runningpnpm installin a workspace package kept prompting to remove and reinstallnode_modules(or, in a non-TTY shell, failing withERR_PNPM_ABORTED_REMOVE_MODULES_DIR_NO_TTY), even when nothing changed.Root cause
During an install, each project's
node_modules/.modules.yamlis written twice:extendInstallOptionssetsvirtualStoreDirto<storeDir>/linkswhen GVS is enabled.buildProjects/buildSelectedPkgs) callsgetContext()and rewrites.modules.yaml. Its options come fromextendBuildOptions, which — unlikeextendInstallOptions— never setvirtualStoreDirfor GVS, sogetContext()fell back to the per-projectnode_modules/.pnpmdefault and overwrote the value the install step had recorded.The next install then recomputes
<storeDir>/links, compares it to the recordednode_modules/.pnpm, fails the virtual-store check withERR_PNPM_UNEXPECTED_VIRTUAL_STORE, and — because a plain install runs withforceNewModules— prompts to purgenode_modules.Fix
Make
extendBuildOptionsGVS-aware, mirroringextendInstallOptions: whenenableGlobalVirtualStoreis set andvirtualStoreDiris unset, derive it fromglobalVirtualStoreDir(falling back to<storeDir>/links). Both build consumers flow their options throughextendBuildOptionsintogetContext, so this single change makes the build step record the same virtual-store path the install step wrote.Tests
Added an e2e regression test (
pnpm/test/install/globalVirtualStore.ts) covering a workspace package that is also its own workspace root: after the install it asserts the package's.modules.yamlrecords the global virtual store directory, and that a subsequent install is a clean no-op rather than a purge prompt.pacquet
Not affected. pacquet resolves the virtual store directory through a single
Config::effective_virtual_store_dir()helper at its one.modules.yamlwrite site, so it has no equivalent install-vs-build asymmetry (already covered by existing config and modules-yaml tests).Written by an agent (Claude Code, claude-opus-4-8).