fix: detect enableGlobalVirtualStore toggle in workspace state check#12147
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 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)
📝 WalkthroughWalkthroughAdds 'enableGlobalVirtualStore' to workspace state keys, updates checkDepsStatus to compare declared workspace settings, normalizes the new key in pacquet optimistic-repeat-install (treating omitted and explicit false as equivalent), adds tests, and includes a changeset documenting the change. ChangesWorkspace State Setting Tracking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 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 |
Review Summary by QodoDetect enableGlobalVirtualStore toggle in workspace state check
WalkthroughsDescription• Add enableGlobalVirtualStore to workspace state settings detection • Ensures toggle changes trigger full dependency validation • Prevents "Already up to date" when virtual store location changes • Includes unit test for toggle detection scenario Diagramflowchart LR
A["enableGlobalVirtualStore toggle"] --> B["Added to WORKSPACE_STATE_SETTING_KEYS"]
B --> C["checkDepsStatus detects change"]
C --> D["upToDate returns false"]
D --> E["Full mutateModules path runs"]
E --> F["virtualStoreDir compatibility check executes"]
File Changes1. workspace/state/src/types.ts
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a missed workspace-state setting comparison so pnpm install doesn’t incorrectly take the optimisticRepeatInstall “Already up to date” fast path after toggling enableGlobalVirtualStore, ensuring the full install path runs and can validate/purge for the new virtual store location.
Changes:
- Add
enableGlobalVirtualStoretoWORKSPACE_STATE_SETTING_KEYSso it’s persisted and compared duringcheckDepsStatus. - Add a unit test asserting
checkDepsStatusreturnsupToDate: falsewhenenableGlobalVirtualStorechanges. - Add a changeset to ship patch releases for affected packages.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| workspace/state/src/types.ts | Record enableGlobalVirtualStore in persisted workspace state settings so status checks can detect toggles. |
| deps/status/test/checkDepsStatus.test.ts | Add coverage for checkDepsStatus detecting enableGlobalVirtualStore changes. |
| .changeset/gvs-toggle-detection.md | Publish patch changes for workspace state + deps status + pnpm. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…npm#12142) The optimisticRepeatInstall fast path bypasses validateModules entirely, so toggling enableGlobalVirtualStore off was not detected — pnpm printed "Already up to date" without reinstalling deps in the new virtual store location. Add enableGlobalVirtualStore to WORKSPACE_STATE_SETTING_KEYS so checkDepsStatus detects the change and falls through to the full mutateModules path (which includes the virtualStoreDir compatibility check and purge).
Iterate over WORKSPACE_STATE_SETTING_KEYS instead of
Object.entries(workspaceState.settings) so that settings absent from
legacy state files (e.g. enableGlobalVirtualStore) are still compared
against the current config. Removes the ad-hoc allowBuilds null-check
block since the main loop now covers it with the ?? {} coalescing.
1737198 to
20f9362
Compare
Code Review by Qodo
1. Pacquet omits workspace patterns
|
zkochan
added a commit
to aqeelat/pnpm
that referenced
this pull request
Jun 8, 2026
Port pnpm#12147 to pacquet. pnpm's checkDepsStatus now iterates the full WORKSPACE_STATE_SETTING_KEYS list, so a key pacquet omits from .pnpm-workspace-state-v1.json is read as `undefined` and compared against pnpm's resolved config. `enableGlobalVirtualStore` resolves to a concrete default (true, or false under CI), so omitting it would make pnpm reinstall after a pacquet install — and pacquet itself never detected the toggle (its own copy of pnpm#12142). Add `enable_global_virtual_store` to WorkspaceStateSettings; current_settings writes `then_some(true)` (omit when off, mirroring pnpm omitting its undefined default); settings_match coerces `None`/`Some(false)` as equal before comparing, matching the existing allow_builds/package_extensions handling.
|
Code review by qodo was updated up to the latest commit bf4aa25 |
zkochan
approved these changes
Jun 8, 2026
Contributor
Author
|
@zkochan are you working on the parquet coverage or should I do it? |
Contributor
Integrated-Benchmark Report (Linux)Each scenario reports direct installs and pnpr installs. Bencher consumes pacquet@HEAD and pnpr@HEAD. Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 9.89467198464,
"stddev": 0.09694356885521525,
"median": 9.858184751140001,
"user": 3.0514643799999996,
"system": 3.2071696199999997,
"min": 9.77029832014,
"max": 10.06726701014,
"times": [
9.940113891140001,
9.85156299514,
10.06726701014,
9.857720719140001,
9.97411202314,
9.82124833514,
9.77029832014,
9.80064391014,
10.00510385914,
9.858648783140001
]
},
{
"command": "pacquet@main",
"mean": 9.920290644940001,
"stddev": 0.1503967832578077,
"median": 9.86484465664,
"user": 3.03612708,
"system": 3.21486392,
"min": 9.792855471140001,
"max": 10.227968457140001,
"times": [
9.869198372140001,
9.792855471140001,
10.227968457140001,
9.83866504414,
9.86952219014,
10.16318253714,
9.86049094114,
9.92769059714,
9.844812366140001,
9.80852047314
]
},
{
"command": "pnpr@HEAD",
"mean": 5.36039155224,
"stddev": 0.11251790345814933,
"median": 5.32981622664,
"user": 2.51671558,
"system": 2.8766888200000005,
"min": 5.26984656014,
"max": 5.65583250714,
"times": [
5.347358472140001,
5.393948429140001,
5.32589116814,
5.273390200140001,
5.39564225414,
5.26984656014,
5.29637775714,
5.33374128514,
5.65583250714,
5.31188688914
]
},
{
"command": "pnpr@main",
"mean": 5.34908951534,
"stddev": 0.13927334165122476,
"median": 5.31235356264,
"user": 2.5377687799999995,
"system": 2.8460832199999997,
"min": 5.253317867140001,
"max": 5.734350095140001,
"times": [
5.31212747014,
5.287867851140001,
5.37237032114,
5.33488029114,
5.28129633314,
5.253317867140001,
5.31798630114,
5.31257965514,
5.734350095140001,
5.2841189681400005
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6426720350400001,
"stddev": 0.010565047036562853,
"median": 0.6406162431400001,
"user": 0.37248748,
"system": 1.3205697399999998,
"min": 0.6283327716400001,
"max": 0.6650135916400001,
"times": [
0.6514714986400001,
0.6447665596400001,
0.6650135916400001,
0.64041128964,
0.64037098964,
0.6408211966400001,
0.6484740196400001,
0.6348191946400001,
0.6322392386400001,
0.6283327716400001
]
},
{
"command": "pacquet@main",
"mean": 0.67563501384,
"stddev": 0.0689631690143954,
"median": 0.6574014366400001,
"user": 0.3660764799999999,
"system": 1.3182450399999999,
"min": 0.6372773776400001,
"max": 0.8695769656400001,
"times": [
0.6605424686400001,
0.6388188466400001,
0.6372773776400001,
0.8695769656400001,
0.66830999564,
0.6646624426400001,
0.6542604046400001,
0.66329310364,
0.64684122464,
0.6527673086400001
]
},
{
"command": "pnpr@HEAD",
"mean": 0.77597050874,
"stddev": 0.03412677215191066,
"median": 0.7655310321400002,
"user": 0.38622188,
"system": 1.3243700399999998,
"min": 0.74417738064,
"max": 0.8438679896400001,
"times": [
0.7730254596400001,
0.7517336546400001,
0.7473349876400001,
0.8070800886400001,
0.7602763896400001,
0.74417738064,
0.7707856746400001,
0.8438679896400001,
0.8135660656400001,
0.7478573966400001
]
},
{
"command": "pnpr@main",
"mean": 0.7660885945400001,
"stddev": 0.03669579120199429,
"median": 0.7551041366400001,
"user": 0.38714807999999995,
"system": 1.33142174,
"min": 0.7391695846400002,
"max": 0.8590286156400001,
"times": [
0.7568444366400001,
0.7391695846400002,
0.7454085546400001,
0.8590286156400001,
0.7575678806400001,
0.7529949606400002,
0.7404636786400001,
0.7991999606400001,
0.7567273096400001,
0.7534809636400001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 9.21005511614,
"stddev": 0.053432961516206454,
"median": 9.21339928884,
"user": 3.6303936,
"system": 3.17487162,
"min": 9.14968991584,
"max": 9.32558188384,
"times": [
9.22859615784,
9.22660242984,
9.23494795584,
9.162447475839999,
9.32558188384,
9.15729661484,
9.20019614784,
9.17361934284,
9.241573236839999,
9.14968991584
]
},
{
"command": "pacquet@main",
"mean": 9.210407914040001,
"stddev": 0.04556624776719224,
"median": 9.19598073884,
"user": 3.5894438,
"system": 3.19535032,
"min": 9.15094161784,
"max": 9.28509104584,
"times": [
9.20330770584,
9.28509104584,
9.17624846384,
9.24865784384,
9.17420219684,
9.18865377184,
9.15094161784,
9.17667734884,
9.27029592684,
9.23000321884
]
},
{
"command": "pnpr@HEAD",
"mean": 5.108432744939999,
"stddev": 0.12452737732902637,
"median": 5.05301497534,
"user": 2.3630252,
"system": 2.7313555199999997,
"min": 4.98878048684,
"max": 5.37940844084,
"times": [
5.03125225984,
5.11355336984,
4.98878048684,
5.060215212839999,
5.04581473784,
5.283951397839999,
5.095816107839999,
5.37940844084,
5.0423453478399995,
5.043190087839999
]
},
{
"command": "pnpr@main",
"mean": 5.074814851239999,
"stddev": 0.05564529414171983,
"median": 5.06439045734,
"user": 2.3871464000000002,
"system": 2.7079582199999996,
"min": 5.014970441839999,
"max": 5.21098059584,
"times": [
5.05006322584,
5.074508970839999,
5.04185602884,
5.1199040578399995,
5.21098059584,
5.073327024839999,
5.07173185984,
5.014970441839999,
5.057049054839999,
5.03375725184
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.40937626708,
"stddev": 0.015106653391288378,
"median": 1.4103490867800001,
"user": 1.51740684,
"system": 1.7968388999999998,
"min": 1.3882195637799999,
"max": 1.43890105178,
"times": [
1.42391832878,
1.41467898578,
1.39959898578,
1.39306426378,
1.41312377678,
1.40000194178,
1.41468137578,
1.43890105178,
1.40757439678,
1.3882195637799999
]
},
{
"command": "pacquet@main",
"mean": 1.4023313662799999,
"stddev": 0.022299078539003956,
"median": 1.39653653278,
"user": 1.51644234,
"system": 1.7864566999999998,
"min": 1.38332756578,
"max": 1.45819420578,
"times": [
1.45819420578,
1.40484521278,
1.40269834278,
1.4026332287799999,
1.38866088178,
1.4172969357799998,
1.3868595607799998,
1.38835789178,
1.39043983678,
1.38332756578
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6755460030800001,
"stddev": 0.021035673936795708,
"median": 0.66915793978,
"user": 0.33126034000000004,
"system": 1.2763841999999999,
"min": 0.6587076847800001,
"max": 0.72449892278,
"times": [
0.7024432417800001,
0.66109049878,
0.66937212178,
0.72449892278,
0.6587076847800001,
0.6702121567800001,
0.66894375778,
0.66706389878,
0.6702173707800001,
0.6629103767800001
]
},
{
"command": "pnpr@main",
"mean": 0.6975960978800001,
"stddev": 0.08208711182526489,
"median": 0.67251414478,
"user": 0.32674874,
"system": 1.2792863999999997,
"min": 0.6551099567800001,
"max": 0.92889950078,
"times": [
0.6721448777800001,
0.6551099567800001,
0.66892392178,
0.92889950078,
0.6653990207800001,
0.6750758717800001,
0.6624287817800001,
0.67581654578,
0.6992790897800001,
0.6728834117800001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 5.034724502120001,
"stddev": 0.036011097695373216,
"median": 5.0269384208200005,
"user": 1.69280798,
"system": 1.9921260999999997,
"min": 4.99412428032,
"max": 5.10790661232,
"times": [
4.99412428032,
5.04865817632,
4.99796688532,
5.02808364832,
5.07899846132,
5.10790661232,
5.04095387532,
5.02579319332,
5.01187636432,
5.01288352432
]
},
{
"command": "pacquet@main",
"mean": 5.00684980472,
"stddev": 0.037654634277571535,
"median": 5.00223070882,
"user": 1.6848743799999997,
"system": 1.967875,
"min": 4.94075681832,
"max": 5.07718568332,
"times": [
5.00105160932,
5.03608591432,
4.94075681832,
4.99365049732,
5.00340980832,
5.00653653432,
4.99417150632,
5.04105330032,
4.97459637532,
5.07718568332
]
},
{
"command": "pnpr@HEAD",
"mean": 0.70268173432,
"stddev": 0.015306057215695733,
"median": 0.7036407628200001,
"user": 0.33591457999999996,
"system": 1.3226501000000002,
"min": 0.6832233603200001,
"max": 0.7323828023200001,
"times": [
0.7323828023200001,
0.70417100832,
0.70502893832,
0.68390538532,
0.72007984032,
0.7073415433200001,
0.6832233603200001,
0.6922715783200001,
0.69530236932,
0.7031105173200001
]
},
{
"command": "pnpr@main",
"mean": 0.69681343102,
"stddev": 0.049922783942111686,
"median": 0.6793221513200001,
"user": 0.33850238000000005,
"system": 1.2869619,
"min": 0.6646033543200001,
"max": 0.8279659103200001,
"times": [
0.7334846673200001,
0.6646033543200001,
0.66910876332,
0.6813935843200001,
0.8279659103200001,
0.68534942332,
0.67015563332,
0.6772507183200001,
0.67700740232,
0.68181485332
]
}
]
} |
Contributor
Bencher ReportPort pnpm#12147 to pacquet. pnpm's checkDepsStatus now iterates the full WORKSPACE_STATE_SETTING_KEYS list, reading a key absent from the recorded state as `undefined`. So after a pacquet install, any key pnpm resolves to a concrete default that pacquet omits from .pnpm-workspace-state-v1.json shows up as drift, and pnpm re-runs a (no-op) install on every command instead of taking the optimistic fast path. Mirror the concrete-default keys pacquet was omitting so a pacquet-written state matches what pnpm would compute: - excludeLinksFromLockfile (pnpm default false) - minimumReleaseAge (pnpm default 1440) - minimumReleaseAgeIgnoreMissingTime (pnpm default true) enableGlobalVirtualStore is also tracked: pnpm leaves it `undefined` by default (concrete only under --global/CI), so pacquet writes `Some(true)` only when on and omits when off, matching pnpm's encoding; settings_match coerces `None`/`Some(false)` as equal (same pattern as allow_builds/package_extensions). This also fixes pacquet's own copy of pnpm#12142 (the toggle was invisible to its freshness check). Keys pnpm leaves `undefined` by default (minimumReleaseAgeStrict/Exclude, trustPolicy*) stay omitted — `undefined == undefined`. workspacePackage- Patterns is concrete for multi-package workspaces but lives in the workspace manifest, not Config; threading it into current_settings is a follow-up.
bf4aa25 to
54ac9a3
Compare
|
Code review by qodo was updated up to the latest commit 54ac9a3 |
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
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.
Problem
Closes #12142
Toggling
enableGlobalVirtualStoreoff inpnpm-workspace.yamlhas no effect —pnpm installprints "Already up to date" without reinstalling dependencies in the new virtual store location (node_modules/.pnpminstead of<storeDir>/links).Root cause
enableGlobalVirtualStorewas missing fromWORKSPACE_STATE_SETTING_KEYSinworkspace/state/src/types.ts. TheoptimisticRepeatInstallfast path (checkDepsStatus) runs beforemutateModulesand compares stored workspace state settings against current config. Since the GVS toggle was never recorded, the comparison found nothing changed and short-circuited — bypassingvalidateModules→checkCompatibilitywhich would have detected thevirtualStoreDirmismatch.Fix
'enableGlobalVirtualStore'toWORKSPACE_STATE_SETTING_KEYSsocheckDepsStatusrecords and compares it, returningupToDate: falsewhen it changes and allowing the fullmutateModulespath (virtualStoreDir compatibility check + purge) to run.checkDepsStatus: iterate the fullWORKSPACE_STATE_SETTING_KEYSlist instead of only the keys present in the recorded state. A key absent from an older state file (written before it was tracked) is now read asundefinedand compared against the live config, so a toggle is detected even on legacy state files. This also folds the formerallowBuildsnull special-case into the loop via?? {}coercion.pacquet parity
The bidirectional comparison tightens the pnpm↔pacquet workspace-state contract. pacquet writes
.pnpm-workspace-state-v1.jsontoo; with the all-keys loop, pnpm now reads any key pacquet omits asundefinedand compares it against pnpm's resolved config. Every key pnpm resolves to a concrete default that pacquet omitted therefore shows up as drift after a pacquet install, costing pnpm the optimistic fast path on every command (it falls through to a normal, idempotent install — a perf regression, not a destructive reinstall).Verified against an isolated config-reader run (the dev machine's global pnpm config otherwise pollutes the defaults), the concrete-default keys pacquet was omitting are
excludeLinksFromLockfile(false),minimumReleaseAge(1440), andminimumReleaseAgeIgnoreMissingTime(true).enableGlobalVirtualStoreisundefinedby default (concrete only under--global/CI). The rest (minimumReleaseAgeStrict/Exclude,trustPolicy*) stayundefined, so omitting them still matches.Ported to pacquet (
current_settings+settings_matchinoptimistic_repeat_install.rs, fields inworkspace-state):excludeLinksFromLockfile,minimumReleaseAge(the raw config value, not theSome(0)-disabled resolution), andminimumReleaseAgeIgnoreMissingTime— pacquet already resolves all three to pnpm's exact defaults, and consumes them (lockfile content / resolution verifier), so comparing them is correct, not spurious.enableGlobalVirtualStore: writeSome(true)only when on and omit when off (matching pnpm'sundefineddefault);settings_matchcoercesNone/Some(false)as equal (same pattern as the existingallow_builds/package_extensionshandling). This also fixes pacquet's own copy of the bug — the toggle was invisible to its freshness check.Remaining narrow gaps (documented in the carve-out comment, not fixed here):
workspacePackagePatternsis concrete for multi-package workspaces but lives in the workspace manifest rather thanConfig, so threading it intocurrent_settingsis a follow-up;minimumReleaseAgeStrictis only concrete when the user explicitly setsminimumReleaseAge.Test plan
checkDepsStatusunit tests:upToDate: falsewhenenableGlobalVirtualStoreis toggled off, and when toggled on from a legacy state file lacking the keycheckDepsStatustests pass (13/13);@pnpm/workspace.statetests pass (9/9)excludeLinksFromLockfile,minimumReleaseAge,minimumReleaseAgeIgnoreMissingTime, andenableGlobalVirtualStore; plus a pnpm-CI-writtenfalsestaying up-to-date against a pacquet off-installoptimistic_repeat_install+workspace-statetests pass (29 run);cargo check/clippy --deny warnings/fmt/doccleanWritten by an agent (opencode, glm-5.1).
The bidirectional comparison and pacquet port were added by an agent (Claude Code, claude-opus-4-8).
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
Chores