fix(pacquet/store-dir): append STORE_VERSION to storeDir so pnpm and pacquet stay interchangeable by zkochan · Pull Request #11891 · pnpm/pnpm

zkochan · 2026-05-23T23:20:27Z

Summary

When you install with pacquet and then run pnpm in the same project (or vice versa), pnpm refuses to proceed with:

ERR_PNPM_UNEXPECTED_STORE
The dependencies at \"<…>/__typings__/node_modules\" are currently linked from the store at \"/Volumes/src/.pnpm-store\".
pnpm now wants to use the store at \"/Volumes/src/.pnpm-store/v11\" to link dependencies.

getStorePath in pnpm appends STORE_VERSION (\"v11\") to whatever the user configured, so the .modules.yaml it writes records the v11-suffixed path. pacquet stored the suffix only as an internal sub-path accessor (StoreDir::v11), which meant config.store_dir.display() — the value pacquet writes to .modules.yaml, prints from pacquet store path, and emits in the NDJSON context log — yielded the un-suffixed parent. pnpm's checkCompatibility then disagreed with the recorded value and threw.

Fix

Centralised in From<PathBuf> for StoreDir, mirroring pnpm's if (endsWith(v11)) return; else append(v11) branch:

StoreDir::root is now the STORE_VERSION-suffixed path itself, matching pnpm's storeDir.
The auto-suffix lives in one place (From<PathBuf>), so every consumer reading from StoreDir (display(), root(), files(), tmp(), links(), projects()) gets the suffixed value through one source of truth.
The previously separate StoreDir::v11() helper is gone — files/links/tmp/projects now join directly off root.
Introduced pacquet_store_dir::STORE_VERSION (\"v11\") so tests and consumers reference the same constant pnpm exports as @pnpm/constants.STORE_VERSION.

The fix is the same one already in place for the GVS path (<root>/links) — links() previously composed self.v11().join(\"links\") so it landed at <storeDir>/links matching pnpm. The bug was that display() and the field underneath it bypassed that suffix. Collapsing root into the suffixed form unifies the two paths.

On-disk store layout is unchanged. From<PathBuf> doesn't double-append when the input already ends with v11, so explicit user-set storeDir values that already include /v11 round-trip cleanly.

Test plan

typos pacquet (matches CI scope)
cargo fmt --check
just check
just lint
cargo dylint --all (catches Perfectionist lints just ready doesn't)
cargo nextest run --no-fail-fast — all 2004 tests pass
New tests in pacquet_store_dir::store_dir::tests:
- from_pathbuf_auto_appends_store_version_when_missing
- from_pathbuf_does_not_double_append_when_already_suffixed
- modules_yaml_serialized_store_dir_carries_store_version
CI green

Written by an agent (Claude Code, claude-opus-4-7).

Summary by CodeRabbit

Chores
- Store paths now consistently include a version segment for clearer organization.
- Store directory layout and path-resolution behavior updated for consistent results across platforms.
- Package store index moved to reside directly under the store root for simplified access.
- Tests and persisted store metadata updated to reflect and preserve the new versioned path layout.

…stay interchangeable pnpm's `getStorePath` appends `STORE_VERSION` (`"v11"`) to whatever the user configured, so the `.modules.yaml` it writes records the v11-suffixed path. pacquet stored the suffix only as an internal sub-path accessor (`StoreDir::v11`), which meant `config.store_dir.display()` — the value pacquet writes to `.modules.yaml`, prints from `pacquet store path`, and emits in the NDJSON `context` log — yielded the un-suffixed parent. Switching between the two CLIs in the same project tripped pnpm's `checkCompatibility` with `ERR_PNPM_UNEXPECTED_STORE`. Fix is centralised in `From<PathBuf> for StoreDir`, mirroring pnpm's `if (endsWith(v11)) return; else append(v11)` branch at store/path/src/index.ts:39-42. Every consumer reading from `StoreDir` (`display()`, `root()`, `files()`, `tmp()`, `links()`, `projects()`) now sees the v11-suffixed path through one source of truth, so the on-disk layout is unchanged and the externally-reported `storeDir` matches pnpm's exactly. Ref: https://github.com/pnpm/pnpm/blob/29a42efc3b/store/path/src/index.ts#L39-L42

qodo-code-review · 2026-05-23T23:20:31Z

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

coderabbitai · 2026-05-23T23:20:45Z

📝 Walkthrough

Walkthrough

This PR refactors how the pnpm v11 store version suffix is incorporated into store paths. StoreDir.root now includes STORE_VERSION; From auto-appends it when missing. Path helpers build from root, v11() is removed, index.db moves to root, and tests/config are updated accordingly.

Changes

Store version path layout refactor

Layer / File(s)	Summary
StoreDir path contract and helpers `pacquet/crates/store-dir/src/store_dir.rs`, `pacquet/crates/store-dir/src/store_dir/tests.rs`	`StoreDir.root` is redefined to include `STORE_VERSION`; `From<PathBuf>` conditionally appends the version when missing. Documentation and internal path helpers (`files`, `tmp`, `links`, `projects`) are updated to build directly from `self.root`. New tests validate auto-append and idempotence behavior.
Store index location migration `pacquet/crates/store-dir/src/store_index.rs`, `pacquet/crates/store-dir/src/store_index/tests.rs`	`StoreIndex` and `StoreIndexWriter` now open and check for `index.db` at `store_dir.root()` instead of `store_dir.v11()`. Writer spawn and open methods are updated, along with the corresponding test assertion.
Store path module documentation `pacquet/crates/config/src/store_path.rs`	Documentation is clarified to state the module returns an un-suffixed base store path and that `StoreDir::from` appends `STORE_VERSION` to ensure pacquet outputs match pnpm behavior.
Config store resolution with new path layout `pacquet/crates/config/src/lib.rs`	`Config::current` updates non-explicit `store_dir` resolution to compute `store_root` from the parent of the versioned `self.store_dir.root()` path, with fallback to home directory.
Config defaults and home-dir resolution tests `pacquet/crates/config/src/defaults/tests.rs`, `pacquet/crates/config/src/lib.rs`	Tests for `PNPM_HOME`, `XDG_DATA_HOME`, and fallback home-directory branches are updated to assert that resolved `store_dir` includes the `STORE_VERSION` suffix.
Project registry test setup `pacquet/crates/store-dir/src/project_registry.rs`	The `register_skips_when_store_is_inside_project` test now materializes `store_dir.root()` on disk (including any `STORE_VERSION` suffix) to ensure the canonical-path containment check behaves consistently.
CLI and install output tests `pacquet/crates/cli/tests/store.rs`, `pacquet/crates/package-manager/src/install/tests.rs`	Tests for the `pacquet store path` command, `pnpm:context` payload, and `.modules.yaml` contents are updated to expect `store_dir` to include the version suffix.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

pnpm/pnpm#11819: Changes the fresh-resolve install path to register and write projects under v11/links/v11/projects, directly aligned with this PR's refactor of store path layout.
pnpm/pnpm#11718: Modifies pacquet/crates/config defaults and tests related to default_store_dir, overlapping with this PR's config resolution changes.
pnpm/pnpm#11804: Also touches pacquet/crates/config/src/lib.rs and store-path resolution, overlapping concerns about when and how the final store path is computed.

Poem

🐰 I hopped to a path and found v11,

Root now wears the version like a den.
No sideways v11 calls in sight,
Tests and index now sleep tight.
A tidy root — the store's delight!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: appending STORE_VERSION to storeDir to maintain interchangeability between pnpm and pacquet.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix-pacquet-store-dir-v11

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2026-05-23T23:25:24Z

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.01      7.9±0.26ms   548.2 KB/sec    1.00      7.9±0.32ms   552.2 KB/sec

codecov-commenter · 2026-05-23T23:28:02Z

Codecov Report

❌ Patch coverage is 91.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.83%. Comparing base (43e06bb) to head (a2024e5).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
pacquet/crates/store-dir/src/store_dir.rs	70.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11891      +/-   ##
==========================================
- Coverage   87.84%   87.83%   -0.02%     
==========================================
  Files         206      206              
  Lines       24509    24520      +11     
==========================================
+ Hits        21531    21538       +7     
- Misses       2978     2982       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

pacquet/crates/store-dir/src/project_registry.rs (1)

457-468: ⚡ Quick win

Documentation inconsistency: comment mentions StoreDir::from but code uses StoreDir::new.

The comment on line 458 states "after [StoreDir::from] applies the suffix" but the actual code on line 468 calls StoreDir::new(&store_path). Either update the comment to reference new instead of from, or clarify that both constructors apply the suffix logic.

📝 Suggested fix for comment consistency

     /// Subdir guard: when the store lives inside the project, the
     /// function is a silent no-op — registering would otherwise create
     /// a self-referential symlink. The `STORE_VERSION` subdir
-    /// (`store_dir.root()` after [`StoreDir::from`] applies the suffix)
+    /// (`store_dir.root()` after `StoreDir::new` applies the suffix)
     /// is materialised on disk so [`path_contains`]'s canonical-form
     /// comparison sees both sides as canonical paths even on macOS,
     /// where `/tmp` symlinks to `/private/tmp` and a missing target
     /// would silently fall back to lexical comparison and miss the
     /// containment.

🤖 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 `@pacquet/crates/store-dir/src/project_registry.rs` around lines 457 - 468, The
doc comment incorrectly references StoreDir::from while the test constructs a
StoreDir via StoreDir::new; update the comment to reference StoreDir::new (or
explicitly state that both StoreDir::from and StoreDir::new apply the suffix) so
the documentation matches the usage in the test; locate the comment near the
test function register_skips_when_store_is_inside_project and change the
bracketed reference [`StoreDir::from`] to [`StoreDir::new`] or add a brief note
mentioning both constructors.

🤖 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.

Inline comments:
In `@pacquet/crates/store-dir/src/store_dir.rs`:
- Around line 29-37: The current #[serde(transparent)] on StoreDir allows Serde
to deserialize directly into StoreDir.root (PathBuf) and bypass the
normalization in impl From<PathBuf> for StoreDir, so persisted unsuffixed paths
can violate the STORE_VERSION suffix invariant; replace the derive-based
Deserialize for StoreDir with a custom impl Deserialize that deserializes a
PathBuf (or String) and constructs StoreDir via the existing From<PathBuf> (or a
normalize method) to ensure the suffix is applied, remove or keep
serde(transparent) only if compatible, and add a serde round-trip test that
deserializes an unsuffixed persisted path and asserts the resulting
StoreDir.root has the STORE_VERSION suffix (referencing StoreDir, root, and the
From<PathBuf> for locating code).

---

Nitpick comments:
In `@pacquet/crates/store-dir/src/project_registry.rs`:
- Around line 457-468: The doc comment incorrectly references StoreDir::from
while the test constructs a StoreDir via StoreDir::new; update the comment to
reference StoreDir::new (or explicitly state that both StoreDir::from and
StoreDir::new apply the suffix) so the documentation matches the usage in the
test; locate the comment near the test function
register_skips_when_store_is_inside_project and change the bracketed reference
[`StoreDir::from`] to [`StoreDir::new`] or add a brief note mentioning both
constructors.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 91a7dd80-167e-4aad-bd5b-c49cafc37e5f

📥 Commits

Reviewing files that changed from the base of the PR and between e0bd879 and aedb504.

📒 Files selected for processing (10)

pacquet/crates/cli/tests/store.rs
pacquet/crates/config/src/defaults/tests.rs
pacquet/crates/config/src/lib.rs
pacquet/crates/config/src/store_path.rs
pacquet/crates/package-manager/src/install/tests.rs
pacquet/crates/store-dir/src/project_registry.rs
pacquet/crates/store-dir/src/store_dir.rs
pacquet/crates/store-dir/src/store_dir/tests.rs
pacquet/crates/store-dir/src/store_index.rs
pacquet/crates/store-dir/src/store_index/tests.rs

📜 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). (8)

GitHub Check: Run benchmark on ubuntu-latest
GitHub Check: Lint and Test (ubuntu-latest)
GitHub Check: Lint and Test (macos-latest)
GitHub Check: Lint and Test (windows-latest)
GitHub Check: Dylint
GitHub Check: Run benchmark on ubuntu-latest
GitHub Check: Code Coverage
GitHub Check: Compile & Lint

🧰 Additional context used

📓 Path-based instructions (2)

pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plain String or &str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only via TryFrom<String> and/or FromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallible From<String> (and From<&str> when convenient).
If upstream occasionally constructs without validation, expose from_str_unchecked as an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use #[serde(try_from = "String")] for deserialization and #[serde(into = "String")] for serialization.
Use #[derive(derive_more::From)] and #[derive(derive_more::Into)] for mechanical conversion impls. Fall back to manual impl only when conversion needs custom logic.
String-literal unions should become enums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Path over &PathBuf, &str over &String) when it doesn't force extra copies.
Prefer Arc::clone(&x) / Rc::clone(&x) over x.clone() for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Write use super::{Foo, bar} instead of use super::*;. Two forms stay allowed: external-crate preludes like use rayon::prelude::*; and root-of-module re-...

Files:

pacquet/crates/config/src/store_path.rs
pacquet/crates/cli/tests/store.rs
pacquet/crates/store-dir/src/store_dir/tests.rs
pacquet/crates/store-dir/src/project_registry.rs
pacquet/crates/store-dir/src/store_index/tests.rs
pacquet/crates/package-manager/src/install/tests.rs
pacquet/crates/config/src/defaults/tests.rs
pacquet/crates/config/src/lib.rs
pacquet/crates/store-dir/src/store_index.rs
pacquet/crates/store-dir/src/store_dir.rs

pacquet/**/{tests,test}/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/{tests,test}/**/*.rs: Prefer real fixtures; reach for the dependency-injection seam only when they can't cover the branch. Most happy paths and error paths should be tested with tempfile::TempDir, the mocked registry, or an integration test that spawns the actual binary.
Use the DI seam — a capability trait on the Host provider, threaded as Sys: <Bounds> — only for branches a real fixture can't reach portably: filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths.
Log before non-assert_eq! assertions, dbg! complex structures, skip logging for simple scalar assert_eq!. Follow the test-logging guidance in CODE_STYLE_GUIDE.md.
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found. If the test needs a tool, let it panic when the tool is absent.
Prefer #[cfg_attr(target_os = "windows", ignore = "...")] (or #[cfg(unix)]) over a runtime probe-and-skip helper for platform-locked tools.
Port relevant tests from pnpm when porting behavior. Matching test coverage is the easiest way to prove behavioral parity.
When porting behavior from pnpm, consult plans/TEST_PORTING.md before adding ported tests. Use allow_known_failure! at the not-yet-implemented boundary and update checkboxes as items land.

Files:

pacquet/crates/cli/tests/store.rs

🧠 Learnings (3)

📚 Learning: 2026-05-20T19:40:55.051Z

Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.

Applied to files:

pacquet/crates/config/src/store_path.rs
pacquet/crates/cli/tests/store.rs
pacquet/crates/store-dir/src/store_dir/tests.rs
pacquet/crates/store-dir/src/project_registry.rs
pacquet/crates/store-dir/src/store_index/tests.rs
pacquet/crates/package-manager/src/install/tests.rs
pacquet/crates/config/src/defaults/tests.rs
pacquet/crates/config/src/lib.rs
pacquet/crates/store-dir/src/store_index.rs
pacquet/crates/store-dir/src/store_dir.rs

📚 Learning: 2026-05-22T00:08:44.646Z

Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.

Applied to files:

pacquet/crates/config/src/store_path.rs
pacquet/crates/cli/tests/store.rs
pacquet/crates/store-dir/src/store_dir/tests.rs
pacquet/crates/store-dir/src/project_registry.rs
pacquet/crates/store-dir/src/store_index/tests.rs
pacquet/crates/package-manager/src/install/tests.rs
pacquet/crates/config/src/defaults/tests.rs
pacquet/crates/config/src/lib.rs
pacquet/crates/store-dir/src/store_index.rs
pacquet/crates/store-dir/src/store_dir.rs

📚 Learning: 2026-05-20T23:07:58.444Z

Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.

Applied to files:

pacquet/crates/config/src/store_path.rs
pacquet/crates/cli/tests/store.rs
pacquet/crates/store-dir/src/store_dir/tests.rs
pacquet/crates/store-dir/src/project_registry.rs
pacquet/crates/store-dir/src/store_index/tests.rs
pacquet/crates/package-manager/src/install/tests.rs
pacquet/crates/config/src/defaults/tests.rs
pacquet/crates/config/src/lib.rs
pacquet/crates/store-dir/src/store_index.rs
pacquet/crates/store-dir/src/store_dir.rs

🔇 Additional comments (8)

pacquet/crates/config/src/store_path.rs (1)

35-45: LGTM!

pacquet/crates/config/src/lib.rs (1)

1225-1235: LGTM!

Also applies to: 1430-1433, 1461-1464

pacquet/crates/config/src/defaults/tests.rs (1)

7-7: LGTM!

Also applies to: 51-51, 80-83, 116-117

pacquet/crates/cli/tests/store.rs (1)

2-2: LGTM!

Also applies to: 43-47

pacquet/crates/package-manager/src/install/tests.rs (1)

15-15: LGTM!

Also applies to: 582-582, 679-679

pacquet/crates/store-dir/src/store_dir/tests.rs (1)

1-4: LGTM!

Also applies to: 23-54

pacquet/crates/store-dir/src/store_index.rs (1)

123-127: LGTM!

Also applies to: 404-447

pacquet/crates/store-dir/src/store_index/tests.rs (1)

138-138: LGTM!

…ng multi-line assert

…de(from)] CodeRabbit flagged that the previous `#[serde(transparent)]` derive on `StoreDir` deserialised straight into `StoreDir::root`, bypassing the auto-append in `impl From<PathBuf> for StoreDir`. A persisted unsuffixed path would therefore violate the [`STORE_VERSION`] invariant on the live struct until the next reconstruction. Pacquet doesn't currently deserialize `StoreDir` from any disk shape, but the type-level guarantee is part of the public contract — future serialised state must round-trip through the suffix logic. Route both directions through `PathBuf` with `#[serde(from = "PathBuf", into = "PathBuf")]`. Deserialize now flows through `From<PathBuf>` (which applies the suffix); serialize converts to `PathBuf` and back to the same wire shape `transparent` produced, so no on-disk format change. `Clone` is required by `into` and was added. Also fix CodeRabbit's doc-comment nit at project_registry::register_skips_when_store_is_inside_project — the comment referenced `StoreDir::from` while the test calls `StoreDir::new`; clarified that `new` routes through `From<PathBuf>`. Added round-trip tests in `store_dir::tests`: - `deserialize_applies_store_version_to_unsuffixed_path` - `deserialize_preserves_already_suffixed_path`

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pacquet/crates/store-dir/src/store_dir.rs (1)
177-179: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update stale helper reference in root() docs.

The doc text still lists v11 as a helper, but StoreDir::v11() was removed. Please update this list to avoid a stale API contract in generated docs.
Suggested patch
-    /// purpose-built helpers (`v11`, `tmp`, `links`, `projects`); this
+    /// purpose-built helpers (`tmp`, `links`, `projects`); this
🤖 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 `@pacquet/crates/store-dir/src/store_dir.rs` around lines 177 - 179, Update the
doc comment for the root() method to remove the stale reference to
StoreDir::v11() (which was removed); edit the sentence that lists helpers so it
only mentions the existing helper methods (e.g., tmp, links, projects) and any
other current helpers, ensuring the root() docs no longer reference v11.

🤖 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.

Outside diff comments:
In `@pacquet/crates/store-dir/src/store_dir.rs`:
- Around line 177-179: Update the doc comment for the root() method to remove
the stale reference to StoreDir::v11() (which was removed); edit the sentence
that lists helpers so it only mentions the existing helper methods (e.g., tmp,
links, projects) and any other current helpers, ensuring the root() docs no
longer reference v11.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2933edf0-0fd3-44f8-8911-fbd7eb1f77c3

📥 Commits

Reviewing files that changed from the base of the PR and between 3129b27 and a2024e5.

📒 Files selected for processing (3)

pacquet/crates/store-dir/src/project_registry.rs
pacquet/crates/store-dir/src/store_dir.rs
pacquet/crates/store-dir/src/store_dir/tests.rs

📜 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). (8)

GitHub Check: Run benchmark on ubuntu-latest
GitHub Check: Lint and Test (windows-latest)
GitHub Check: Lint and Test (ubuntu-latest)
GitHub Check: Lint and Test (macos-latest)
GitHub Check: Doc
GitHub Check: Run benchmark on ubuntu-latest
GitHub Check: Code Coverage
GitHub Check: Compile & Lint

🧰 Additional context used

📓 Path-based instructions (1)

pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plain String or &str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only via TryFrom<String> and/or FromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallible From<String> (and From<&str> when convenient).
If upstream occasionally constructs without validation, expose from_str_unchecked as an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use #[serde(try_from = "String")] for deserialization and #[serde(into = "String")] for serialization.
Use #[derive(derive_more::From)] and #[derive(derive_more::Into)] for mechanical conversion impls. Fall back to manual impl only when conversion needs custom logic.
String-literal unions should become enums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Path over &PathBuf, &str over &String) when it doesn't force extra copies.
Prefer Arc::clone(&x) / Rc::clone(&x) over x.clone() for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Write use super::{Foo, bar} instead of use super::*;. Two forms stay allowed: external-crate preludes like use rayon::prelude::*; and root-of-module re-...

Files:

pacquet/crates/store-dir/src/project_registry.rs
pacquet/crates/store-dir/src/store_dir/tests.rs
pacquet/crates/store-dir/src/store_dir.rs

🧠 Learnings (3)

📚 Learning: 2026-05-20T19:40:55.051Z

Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.

Applied to files:

pacquet/crates/store-dir/src/project_registry.rs
pacquet/crates/store-dir/src/store_dir/tests.rs
pacquet/crates/store-dir/src/store_dir.rs

📚 Learning: 2026-05-22T00:08:44.646Z

Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.

Applied to files:

pacquet/crates/store-dir/src/project_registry.rs
pacquet/crates/store-dir/src/store_dir/tests.rs
pacquet/crates/store-dir/src/store_dir.rs

📚 Learning: 2026-05-20T23:07:58.444Z

Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.

Applied to files:

pacquet/crates/store-dir/src/project_registry.rs
pacquet/crates/store-dir/src/store_dir/tests.rs
pacquet/crates/store-dir/src/store_dir.rs

🔇 Additional comments (3)

pacquet/crates/store-dir/src/store_dir.rs (1)

29-37: LGTM!

Also applies to: 54-57, 61-65

pacquet/crates/store-dir/src/store_dir/tests.rs (1)

56-74: LGTM!

pacquet/crates/store-dir/src/project_registry.rs (1)

458-460: LGTM!

…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.

…npm (#11896) * fix(pacquet/modules-yaml): write GVS-aware virtualStoreDir to match pnpm 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. * fix(pacquet/store-dir): build modules_yaml expected path with Path::join 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.

coderabbitai Bot requested changes May 23, 2026

View reviewed changes

Comment thread pacquet/crates/store-dir/src/store_dir.rs Outdated

zkochan added 2 commits May 24, 2026 01:30

fix(store-dir): satisfy Perfectionist macro-trailing-comma on remaini…

3129b27

…ng multi-line assert

coderabbitai Bot reviewed May 23, 2026

View reviewed changes

coderabbitai Bot approved these changes May 23, 2026

View reviewed changes

zkochan merged commit 3209c25 into main May 23, 2026
24 of 26 checks passed

zkochan deleted the fix-pacquet-store-dir-v11 branch May 23, 2026 23:48

This was referenced Jun 21, 2026

feat(pacquet): implement find-hash #12552

Merged

feat(pacquet): implement cat-index command #12551

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(pacquet/store-dir): append STORE_VERSION to storeDir so pnpm and pacquet stay interchangeable#11891

fix(pacquet/store-dir): append STORE_VERSION to storeDir so pnpm and pacquet stay interchangeable#11891
zkochan merged 3 commits into
mainfrom
fix-pacquet-store-dir-v11

zkochan commented May 23, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

qodo-code-review Bot commented May 23, 2026

Uh oh!

coderabbitai Bot commented May 23, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

github-actions Bot commented May 23, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented May 23, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

zkochan commented May 23, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fix

Test plan

Summary by CodeRabbit

Uh oh!

qodo-code-review Bot commented May 23, 2026

Qodo reviews are paused for this user.

Uh oh!

coderabbitai Bot commented May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

github-actions Bot commented May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Micro-Benchmark Results

Linux

Uh oh!

codecov-commenter commented May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zkochan commented May 23, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 23, 2026 •

edited

Loading

github-actions Bot commented May 23, 2026 •

edited

Loading

codecov-commenter commented May 23, 2026 •

edited

Loading