fix(pacquet/modules-yaml): write GVS-aware virtualStoreDir to match pnpm#11896
Merged
Conversation
Under `enableGlobalVirtualStore: true`, upstream pnpm mutates `virtualStoreDir` in place at `extendInstallOptions.ts:419-422` so every consumer that reads `ctx.virtualStoreDir` — including `writeModulesManifest` and the `pnpm:context` debug log — sees the GVS-derived `<storeDir>/v11/links` path. Pacquet kept `Config::virtual_store_dir` at its project-local value (deliberately, see `apply_global_virtual_store_derivation`'s rationale) and wrote that field straight into `.modules.yaml` and the context log. With `pnpm install` delegating to pacquet via `configDependencies`, every run came back through pnpm's `checkCompatibility`, the recorded project-local `virtualStoreDir` didn't match the GVS-mutated value pnpm computed, and per-importer purges fired the "modules directories will be reinstalled from scratch" prompt on every install. Route both externally-visible consumers through a new `Config::effective_virtual_store_dir` helper that returns `global_virtual_store_dir` when GVS is on (which already encodes "user pinned or fall back to `<storeDir>/links`" via `apply_global_virtual_store_derivation`) and the project-local `virtual_store_dir` otherwise. Pacquet's internal layout consumers still read the field directly — the divergence the helper bridges is only at the parity boundary. Test pins both halves: `.modules.yaml` round-trips to `<storeDir>/v11/links` under GVS, and the `pnpm:context` event reports the same path.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent 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). (7)
🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2026-05-20T19:40:55.051ZApplied to files:
📚 Learning: 2026-05-22T00:08:44.646ZApplied to files:
📚 Learning: 2026-05-20T23:07:58.444ZApplied to files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughAdds Config::effective_virtual_store_dir(), switches install serialization/logging to use it for virtualStoreDir, and adds tests asserting ChangesGlobal Virtual Store Directory Reporting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11896 +/- ##
=======================================
Coverage 87.83% 87.84%
=======================================
Files 206 206
Lines 24520 24525 +5
=======================================
+ Hits 21538 21544 +6
+ Misses 2982 2981 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.4694117675599996,
"stddev": 0.08048096703064878,
"median": 2.4761091340599997,
"user": 2.6639608399999997,
"system": 3.5377732199999996,
"min": 2.34562417756,
"max": 2.5811444775599997,
"times": [
2.57102357156,
2.5811444775599997,
2.41165734256,
2.50395079756,
2.53370708956,
2.4704514075599997,
2.34562417756,
2.4194579955599997,
2.48176686056,
2.37533395556
]
},
{
"command": "pacquet@main",
"mean": 2.41020830836,
"stddev": 0.05971350587167937,
"median": 2.39438894556,
"user": 2.65375814,
"system": 3.5793070200000003,
"min": 2.3301536445599997,
"max": 2.51412307756,
"times": [
2.3301536445599997,
2.4023182095599998,
2.48293484656,
2.44164856056,
2.34424918156,
2.44638450656,
2.38645968156,
2.36973093956,
2.38408043556,
2.51412307756
]
},
{
"command": "pnpm",
"mean": 4.90301671406,
"stddev": 0.0405340448544853,
"median": 4.9016154285599995,
"user": 8.26207984,
"system": 4.24188952,
"min": 4.82669438756,
"max": 4.96156048156,
"times": [
4.96156048156,
4.95107520156,
4.93802477356,
4.89674871556,
4.8992682165599994,
4.87804285256,
4.90780839856,
4.86698147256,
4.82669438756,
4.90396264056
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.67328842764,
"stddev": 0.04634444249611974,
"median": 0.65522626154,
"user": 0.37704170000000004,
"system": 1.4331746,
"min": 0.64231825454,
"max": 0.7993274195400001,
"times": [
0.7993274195400001,
0.6754184885400001,
0.6535063425400001,
0.68325989554,
0.65394120154,
0.6466925975400001,
0.6736549015400001,
0.64825385354,
0.64231825454,
0.65651132154
]
},
{
"command": "pacquet@main",
"mean": 0.68626488084,
"stddev": 0.037616780069496175,
"median": 0.6739882635400001,
"user": 0.3832122,
"system": 1.4507614,
"min": 0.6551600215400001,
"max": 0.77798138054,
"times": [
0.66751310154,
0.71219303254,
0.65915694654,
0.6551600215400001,
0.66442776254,
0.7030340635400001,
0.68046342554,
0.65792503554,
0.68479403854,
0.77798138054
]
},
{
"command": "pnpm",
"mean": 2.67519293094,
"stddev": 0.06543419047216763,
"median": 2.6611197860400004,
"user": 3.3186014999999998,
"system": 2.2681955,
"min": 2.61912811554,
"max": 2.8015819515400002,
"times": [
2.64125234254,
2.62479420954,
2.8015819515400002,
2.78516435054,
2.6641221285400003,
2.61943643754,
2.61912811554,
2.66236916754,
2.67421020154,
2.6598704045400003
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.89075011138,
"stddev": 0.17312066521695407,
"median": 4.89615646838,
"user": 6.590735180000001,
"system": 3.5660129000000005,
"min": 4.58900732038,
"max": 5.12855977938,
"times": [
5.01345426438,
5.11749220138,
4.58900732038,
4.7873501993800005,
5.12855977938,
4.77137732738,
4.95586348538,
4.75191547238,
4.95603161238,
4.83644945138
]
},
{
"command": "pacquet@main",
"mean": 4.912526767779999,
"stddev": 0.2314974295539865,
"median": 4.8515618183800004,
"user": 6.605044479999999,
"system": 3.5911537000000004,
"min": 4.66451040238,
"max": 5.39898795338,
"times": [
4.66451040238,
4.7200338223800005,
4.83331013138,
4.76832143938,
4.87708935038,
5.18858145738,
4.86981350538,
5.39898795338,
5.04468914738,
4.75993046838
]
},
{
"command": "pnpm",
"mean": 6.658628504880001,
"stddev": 0.12430944920311156,
"median": 6.70365707088,
"user": 10.820982279999999,
"system": 4.4367168,
"min": 6.415849021380001,
"max": 6.799641379380001,
"times": [
6.47233332738,
6.415849021380001,
6.7091831193800004,
6.75175028738,
6.799641379380001,
6.73534050938,
6.60202009338,
6.69285316938,
6.70364248938,
6.703671652380001
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 3.7100478128800005,
"stddev": 0.05268690525337999,
"median": 3.7160912917799998,
"user": 4.0698515,
"system": 2.16070948,
"min": 3.62701513428,
"max": 3.7994858052800002,
"times": [
3.73050767428,
3.72244126828,
3.7994858052800002,
3.62701513428,
3.73050961728,
3.77114870728,
3.6579278252800003,
3.66340276328,
3.6882980182800003,
3.70974131528
]
},
{
"command": "pacquet@main",
"mean": 3.77876552858,
"stddev": 0.12946190488700438,
"median": 3.74607022728,
"user": 4.093991399999999,
"system": 2.23586568,
"min": 3.64351746028,
"max": 4.00566913228,
"times": [
3.7346298202800003,
3.65394175528,
3.6618577392800002,
3.64351746028,
3.91415845728,
3.70435423228,
3.93852150428,
3.75751063428,
3.77349455028,
4.00566913228
]
},
{
"command": "pnpm",
"mean": 4.3362929684800005,
"stddev": 0.04288091495045665,
"median": 4.32324476078,
"user": 5.508751200000001,
"system": 2.5689145799999995,
"min": 4.28950686028,
"max": 4.41597068128,
"times": [
4.30235901528,
4.28950686028,
4.31086246228,
4.3063403322800005,
4.32486978828,
4.40046891428,
4.41597068128,
4.36395191228,
4.32697998528,
4.32161973328
]
}
]
} |
Contributor
|
Bencher Report| Branch | pr/11896 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 4,890.75 ms(-1.97%)Baseline: 4,988.94 ms | 5,986.73 ms (81.69%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 3,710.05 ms(-5.57%)Baseline: 3,928.69 ms | 4,714.42 ms (78.70%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,469.41 ms(+1.23%)Baseline: 2,439.40 ms | 2,927.29 ms (84.36%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 673.29 ms(-0.79%)Baseline: 678.63 ms | 814.35 ms (82.68%) |
…oin so the test passes on Windows `modules_yaml_serialized_store_dir_carries_store_version` (added in 3209c25) hardcoded `/tmp/.pnpm-store/v11` on the right-hand side while the left-hand side flows through `StoreDir::from`'s `PathBuf::join`. On Windows that join emits `\v11`, so the test panicked with `\v11` vs `/v11` — and has been failing in the post-merge `Pacquet CI` run for #11891 since it landed. Pnpm itself uses Node's `path.join` (via `getStorePath` → `path.join(storePath, STORE_VERSION)`), which is also backslash-joined on Windows. Building the expected value through `Path::join` here mirrors that path and keeps the test asserting the real parity contract on every platform.
This was referenced Jun 10, 2026
Merged
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Under `enableGlobalVirtualStore: true`, upstream pnpm mutates `virtualStoreDir` in place at `extendInstallOptions.ts:419-422` so every consumer that reads `ctx.virtualStoreDir` — including `writeModulesManifest` and the `pnpm:context` debug log — sees the GVS-derived `/v11/links` path.
Pacquet kept `Config::virtual_store_dir` at its project-local value (deliberately, see `apply_global_virtual_store_derivation`'s rationale) and wrote that field straight into `.modules.yaml` and the context log. With `pnpm install` delegating to pacquet via `configDependencies: @pnpm/pacquet`, every run came back through pnpm's `checkCompatibility`, the recorded project-local `virtualStoreDir` didn't match the GVS-mutated value pnpm computed, and per-importer purges fired the "modules directories will be reinstalled from scratch" prompt on every install.
This was reproducible in this repo: `pnpm-workspace.yaml` declares both `enableGlobalVirtualStore: true` and `configDependencies: '@pnpm/pacquet': 0.2.7`. Aborting the prompt (e.g. in CI without a TTY) failed with `ERR_PNPM_ABORTED_REMOVE_MODULES_DIR_NO_TTY`.
Fix
Route both externally-visible consumers — `build_modules_manifest` and the `pnpm:context` emission — through a new `Config::effective_virtual_store_dir` helper that returns `global_virtual_store_dir` when GVS is on (which already encodes "user pinned or fall back to `/links`" via `apply_global_virtual_store_derivation`) and the project-local `virtual_store_dir` otherwise.
Pacquet's internal layout consumers still read the field directly — the deliberate divergence the helper documents lives only at the parity boundary.
Test plan
Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
Tests