fix(pacquet): port pnpm's workspace-link short-circuit and depPath helpers#11944
Merged
Conversation
The previous implementation rebuilt the bare key by formatting
`{prefix}{version}` and re-parsing it through `PkgVerPeer::from_str`,
which assumed `version().to_string()` always round-trips through the
parser. That assumption holds for well-formed snapshot keys but not for
the workspace `link:<path>(<peers>)` shape that the resolver emits on
fixtures with `linkWorkspacePackages: true` and workspace packages that
carry peer dependencies (e.g. babylon's `@dev/shared-ui-components`).
`PkgNameSuffix::FromStr` splits on the first `@`, leaving the `(` in the
name slot and a suffix whose `NonSemver` version retains a `)` — when
`without_peer` reformatted that, the result no longer matched the
opening parenthesis and the `.expect(...)` panicked with
`MismatchParenthesis`.
Strip the peer through the typed fields instead. The new
`PkgVerPeer::without_peer` clones the existing `prefix` / `version`
slots and returns a `PkgVerPeer` with an empty peer string, so no
display-and-reparse round-trip runs.
Resolves #11939.
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? |
|
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:
📝 WalkthroughWalkthroughRefactors peer-suffix stripping to avoid stringify/parse cycles; adds dep-path helpers for runtime and package-id extraction; preserves patch-hash in pkg-id derivation across store and graph layers; and short-circuits workspace ChangesPeer-suffix stripping, workspace link handling, and patch-hash preservation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 |
…_peer Pacquet's `PkgNameVerPeer::without_peer` matches pnpm's `removeSuffix` semantically (it strips both the patch-hash and the peer-graph hash — [`PkgVerPeer`] doesn't model the patch-hash slot separately, so the whole parenthesised tail lives on `peer`). Port the relevant test cases from [`deps/path/test/index.ts`](https://github.com/pnpm/pnpm/blob/cc4ff817aa/deps/path/test/index.ts) so the contract is pinned next to the function: - `removeSuffix` strips patch-hash + peer to `name@version`. - `getPkgIdWithPatchHash` scoped-name leg: `@foo/bar@1.0.0(patch_hash)(peer)` strips back to `@foo/bar@1.0.0`. (The patch-hash retention divergence is tracked as a separate parity gap.) - `tryGetPackageId` nested-peer leg: a peer carrying a balanced `(…)` segment strips as a unit, not at the inner paren.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/lockfile/src/pkg_name_ver_peer/tests.rs (1)
70-79: ⚡ Quick winTrim these test doc comments to concise non-obvious rationale only.
These blocks mostly restate scenarios already captured by test names/assertions. Keep only brief “why” context (e.g., regression/parity pointer), and drop behavior narration.
As per coding guidelines: "Tests are documentation. Do not duplicate them in prose." and "Doc comments (
///,//!) document the contract... They are not a re-narration of the body."Also applies to: 89-93, 103-106
🤖 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/lockfile/src/pkg_name_ver_peer/tests.rs` around lines 70 - 79, Trim the verbose doc-comment block above the test to a short rationale noting the parity/regression intent: remove step-by-step behavior narration and keep a single sentence stating that this test ensures PkgVerPeer's without_peer yields the same bare name@version as pnpm's removeSuffix (and that patch-hash handling is a known parity gap), and apply the same concise editing to the other comment blocks covering the tests referencing PkgVerPeer and without_peer.
🤖 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 `@pacquet/crates/lockfile/src/pkg_name_ver_peer/tests.rs`:
- Around line 70-79: Trim the verbose doc-comment block above the test to a
short rationale noting the parity/regression intent: remove step-by-step
behavior narration and keep a single sentence stating that this test ensures
PkgVerPeer's without_peer yields the same bare name@version as pnpm's
removeSuffix (and that patch-hash handling is a known parity gap), and apply the
same concise editing to the other comment blocks covering the tests referencing
PkgVerPeer and without_peer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: af32341a-e64e-404c-bd38-192849e006ef
📒 Files selected for processing (1)
pacquet/crates/lockfile/src/pkg_name_ver_peer/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). (10)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Doc
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Dylint
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Analyze (javascript)
- 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 firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.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 plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas 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 manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, 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 (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.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. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/lockfile/src/pkg_name_ver_peer/tests.rs
🧠 Learnings (4)
📚 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/lockfile/src/pkg_name_ver_peer/tests.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/lockfile/src/pkg_name_ver_peer/tests.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/lockfile/src/pkg_name_ver_peer/tests.rs
📚 Learning: 2026-05-20T23:23:40.606Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11788
File: pacquet/crates/lockfile/src/snapshot_dep_ref.rs:52-56
Timestamp: 2026-05-20T23:23:40.606Z
Learning: In the pacquet lockfile crate, the link target is intentionally represented as a raw `Link(String)` (not a `LinkTarget` newtype) for both `SnapshotDepRef::Link` and `ImporterDepVersion::Link`. pnpm upstream preserves `link:` targets verbatim with only a prefix check (e.g., `refToRelative`), so do not suggest introducing a `LinkTarget` newtype in a bug-fix/targeted PR. If type branding is ever desired, it should be done together for both variants in a separate dedicated refactor PR rather than as part of a small fix.
Applied to files:
pacquet/crates/lockfile/src/pkg_name_ver_peer/tests.rs
🔇 Additional comments (1)
pacquet/crates/lockfile/src/pkg_name_ver_peer/tests.rs (1)
80-87: LGTM!Also applies to: 94-101, 107-114
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11944 +/- ##
==========================================
+ Coverage 87.83% 87.86% +0.02%
==========================================
Files 224 226 +2
Lines 27434 27587 +153
==========================================
+ Hits 24098 24239 +141
- Misses 3336 3348 +12 ☔ 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": 1.9768528632200002,
"stddev": 0.05688947995500368,
"median": 1.97111933172,
"user": 2.70709676,
"system": 3.3858524400000007,
"min": 1.9016420462199999,
"max": 2.09294001822,
"times": [
1.99839964322,
1.94869550622,
2.0306316042200003,
1.90916498422,
1.9016420462199999,
1.9554330952199999,
1.95415233822,
1.99066382822,
2.09294001822,
1.98680556822
]
},
{
"command": "pacquet@main",
"mean": 2.00244332602,
"stddev": 0.0904376487943319,
"median": 1.97445690822,
"user": 2.6978755600000004,
"system": 3.406345639999999,
"min": 1.90803912622,
"max": 2.2012951012200004,
"times": [
1.9130363292199999,
2.09030817022,
1.95970871722,
2.0393058662200003,
1.95791510722,
2.2012951012200004,
2.0236059492200003,
1.90803912622,
1.98920509922,
1.94201379422
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6471128822799999,
"stddev": 0.03407675224197739,
"median": 0.63594890228,
"user": 0.3552128600000001,
"system": 1.3538532399999998,
"min": 0.62511911578,
"max": 0.73939688378,
"times": [
0.73939688378,
0.64525412078,
0.66045977278,
0.63057903178,
0.64172753078,
0.63042360378,
0.62511911578,
0.6262709587799999,
0.63439788678,
0.63749991778
]
},
{
"command": "pacquet@main",
"mean": 0.62415417958,
"stddev": 0.014634210642216563,
"median": 0.6199212817799999,
"user": 0.34888686,
"system": 1.3482601399999998,
"min": 0.6150065057799999,
"max": 0.66394115178,
"times": [
0.66394115178,
0.61642846878,
0.6151917917799999,
0.6150065057799999,
0.61836807178,
0.62654809878,
0.62147449178,
0.62281856178,
0.61582453778,
0.62594011578
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.19564483472,
"stddev": 0.039629497277895165,
"median": 2.18765993842,
"user": 3.7312652799999997,
"system": 3.07198584,
"min": 2.1237196854200002,
"max": 2.2688748154200002,
"times": [
2.17279324242,
2.22557181542,
2.2688748154200002,
2.21733806542,
2.22261810042,
2.17631181242,
2.18559070942,
2.17390093342,
2.18972916742,
2.1237196854200002
]
},
{
"command": "pacquet@main",
"mean": 2.20253043232,
"stddev": 0.0340672122732771,
"median": 2.20316005892,
"user": 3.72374028,
"system": 3.08929204,
"min": 2.13748535542,
"max": 2.2591303594200003,
"times": [
2.23518624242,
2.18036844042,
2.20240459942,
2.2591303594200003,
2.2254049294200002,
2.17454221642,
2.21189079442,
2.13748535542,
2.19497586742,
2.20391551842
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.3535391788999998,
"stddev": 0.014876080922735105,
"median": 1.3511439894000001,
"user": 1.6431549799999998,
"system": 1.8053082800000002,
"min": 1.3300636944,
"max": 1.3869396824,
"times": [
1.3600245544,
1.3473315014,
1.3423465214,
1.3869396824,
1.3604582794,
1.3505700104,
1.3583763574,
1.3300636944,
1.3475632194,
1.3517179684
]
},
{
"command": "pacquet@main",
"mean": 1.3616457740999999,
"stddev": 0.01616815091172225,
"median": 1.3647692739,
"user": 1.6349934799999999,
"system": 1.8234720799999997,
"min": 1.3434637064,
"max": 1.3815708504,
"times": [
1.3771698803999999,
1.3442904594,
1.3812044044,
1.3704571754,
1.3629681543999999,
1.3665703934,
1.3438431284,
1.3449195884,
1.3434637064,
1.3815708504
]
}
]
} |
Contributor
|
Bencher Report| Branch | pr/11944 |
| 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 | 2,195.64 ms(-40.65%)Baseline: 3,699.74 ms | 4,439.69 ms (49.45%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,353.54 ms(-50.83%)Baseline: 2,752.60 ms | 3,303.11 ms (40.98%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 1,976.85 ms(-9.73%)Baseline: 2,190.05 ms | 2,628.06 ms (75.22%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 647.11 ms(-1.39%)Baseline: 656.25 ms | 787.50 ms (82.17%) |
Port the rest of pnpm's `deps/path/test/index.ts` cases that bear on the depPath helpers pacquet exposes. Most cover edges already pinned indirectly; the new tests pin them at the level the corresponding function lives. `pacquet-deps-path::suffix_index`: - `node@runtime:24.11.1` round-trips through both `remove_suffix` and `get_pkg_id_with_patch_hash`. - The scoped-name leg of `getPkgIdWithPatchHash`: `@foo/bar@1.0.0`, `@foo/bar@1.0.0(patch_hash=…)`, `@foo/bar@1.0.0(@peer@…)`, and the combined patch-hash + peer shape. - `tryGetPackageId` shapes that exercise nested peer parens and scope parens — the balanced-paren scan from the right correctly leaves `/@(-.-)/foo@1.0.0` intact. `pacquet-lockfile::PkgVerPeer`: - Patch-hash + peer round-trip (`1.0.0(patch_hash=0000)(<peer>)`). Pacquet doesn't model the patch-hash slot separately; it lumps the whole tail into `peer`, but the raw string still round-trips. `pacquet-lockfile::PkgNameVerPeer`: - File-protocol tarball (`tar-pkg@file:…`). - Patch-hash + peer round-trip on the full `name@version(…)` shape. - Scope-with-parens (`@(-.-)/foo@…`) — confirms parens inside the scope don't confuse either the parser or `without_peer`.
Pacquet's resolver doesn't yet implement upstream's `depth === -1` short-circuit for workspace-link tree nodes ([`resolvePeers.ts`](https://github.com/pnpm/pnpm/blob/cc4ff817aa/installing/deps-resolver/src/resolvePeers.ts#L396)). The link node still flows through peer resolution and picks up a trailing `(<peers>)` segment on its `dep_path` — pnpm never produces that shape because the link node returns empty resolved peers before `addDepPathToGraph` runs. Until the resolver-level fix lands, the lockfile-emit layer has to paper over the difference so the v9 lockfile matches pnpm: 1. `build_packages_and_snapshots` drops any node whose `dep_path` starts with `link:`. Pnpm leaves link nodes out of `lockfile.packages` entirely, and an existing test (`workspace_link_direct_dep_renders_as_importer_link`) already asserts that — but it only passed for the no-peer case because the `link:<path>(peers)` shape happened to mis-parse as a `PackageKey` instead of being skipped explicitly. The babylon fixture's `@dev/shared-ui-components` (10 peer deps) hits the wrong arm. 2. `importer_dep_version` runs `link:<path>(peers)` through `remove_suffix` before stripping `link:`, so the importer value is `link:<path>` — byte-identical to pnpm's emit. Both fixes are localized to the dep-graph-to-lockfile boundary and will become unnecessary once the resolver ports the `depth === -1` short-circuit. New regression test pins the babylon shape.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
pacquet/crates/lockfile/src/pkg_ver_peer/tests.rs (1)
258-265: ⚡ Quick winShorten this doc comment to the “why” only.
The current text narrates parsing details already enforced by the test assertions. A concise parity/regression note is enough here.
As per coding guidelines "Write code that explains itself. Comments are for the non-obvious why, not a translation of the what" and "Tests are documentation. Do not duplicate them in prose."
🤖 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/lockfile/src/pkg_ver_peer/tests.rs` around lines 258 - 265, Replace the long explanatory doc comment above the test in pacquet/crates/lockfile/src/pkg_ver_peer/tests.rs with a brief "why" note: state that this test exists as a parity/regression check for PkgVerPeer to ensure the parser preserves the raw slot string for byte-level consumers (lockfile YAML / on-disk snapshot keys), and remove the narrated parsing details that duplicate the assertions. Target the comment attached to the PkgVerPeer (suffix-only) test case so it explains only the rationale, not the implementation.pacquet/crates/lockfile/src/pkg_name_ver_peer/tests.rs (1)
37-42: ⚡ Quick winTrim test doc comments to parity intent only.
These blocks restate the test inputs/assertions in prose. Please keep only the non-obvious pnpm-compatibility rationale and link, and let the test bodies document the scenarios.
As per coding guidelines "Doc comments (
///,//!) ... are not a re-narration of the body" and "Tests are documentation. Do not duplicate them in prose."Also applies to: 50-56, 63-68
🤖 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/lockfile/src/pkg_name_ver_peer/tests.rs` around lines 37 - 42, The file's doc comments in the test module are overly verbose and repeat the test body; edit the doc comment that mentions PkgVerPeer and VersionPart::File to remove the prose that restates inputs/assertions and retain only the non-obvious pnpm-compatibility rationale and the link to the pnpm test. Apply the same trimming to the other test doc comments that repeat their test bodies (the blocks referencing the pnpm compatibility) so comments only note the compatibility rationale/link and let the test code itself document the scenario.pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs (1)
714-727: ⚡ Quick winTrim the regression-test doc block to minimal intent.
This comment block narrates behavior that the test already documents via setup/assertions. Keep a short reason (e.g.,
#11939+ parity note) and remove the step-by-step prose.As per coding guidelines: "Tests are documentation. Do not duplicate them in prose."
🤖 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/package-manager/src/dependencies_graph_to_lockfile/tests.rs` around lines 714 - 727, Replace the long regression-test doc block above the test with a minimal intent summary: keep the issue reference (e.g., `#11939`) and a short parity note about matching pnpm behavior, and remove the step-by-step narration and numbered steps; locate the comment immediately preceding the test (the block starting with "Regression for [`#11939`]" above the #[test]) and trim it to a one- or two-line explanation that conveys only the reason for the test and the parity note.pacquet/crates/deps-path/src/suffix_index.rs (1)
136-213: ⚡ Quick winReduce test doc-comment narration to non-obvious context only.
The new
///blocks mostly restate behaviors already encoded in assertions. Please keep only concise rationale (for example, issue/upstream link) and drop the scenario walkthrough text.As per coding guidelines: "Tests are documentation. Do not duplicate them in prose."
🤖 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/deps-path/src/suffix_index.rs` around lines 136 - 213, Remove the verbose narrative doc-comments above the unit tests and replace them with concise comments that only capture non-obvious context or upstream links; specifically trim the long /// blocks preceding the tests runtime_dep_path_has_no_suffix, scoped_name_without_suffix_round_trips, scoped_name_with_patch_hash_keeps_patch_hash, scoped_name_with_peer_strips_to_bare, scoped_name_with_patch_hash_and_peer_keeps_only_patch_hash, leading_slash_legacy_with_nested_peer_strips_to_bare, and scope_with_parens_does_not_confuse_suffix_scan so they no longer restate what the assertions already show—keep only a short rationale or reference (e.g., the pnpm test link or unique behavior) as per the coding guideline.
🤖 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 `@pacquet/crates/deps-path/src/suffix_index.rs`:
- Around line 136-213: Remove the verbose narrative doc-comments above the unit
tests and replace them with concise comments that only capture non-obvious
context or upstream links; specifically trim the long /// blocks preceding the
tests runtime_dep_path_has_no_suffix, scoped_name_without_suffix_round_trips,
scoped_name_with_patch_hash_keeps_patch_hash,
scoped_name_with_peer_strips_to_bare,
scoped_name_with_patch_hash_and_peer_keeps_only_patch_hash,
leading_slash_legacy_with_nested_peer_strips_to_bare, and
scope_with_parens_does_not_confuse_suffix_scan so they no longer restate what
the assertions already show—keep only a short rationale or reference (e.g., the
pnpm test link or unique behavior) as per the coding guideline.
In `@pacquet/crates/lockfile/src/pkg_name_ver_peer/tests.rs`:
- Around line 37-42: The file's doc comments in the test module are overly
verbose and repeat the test body; edit the doc comment that mentions PkgVerPeer
and VersionPart::File to remove the prose that restates inputs/assertions and
retain only the non-obvious pnpm-compatibility rationale and the link to the
pnpm test. Apply the same trimming to the other test doc comments that repeat
their test bodies (the blocks referencing the pnpm compatibility) so comments
only note the compatibility rationale/link and let the test code itself document
the scenario.
In `@pacquet/crates/lockfile/src/pkg_ver_peer/tests.rs`:
- Around line 258-265: Replace the long explanatory doc comment above the test
in pacquet/crates/lockfile/src/pkg_ver_peer/tests.rs with a brief "why" note:
state that this test exists as a parity/regression check for PkgVerPeer to
ensure the parser preserves the raw slot string for byte-level consumers
(lockfile YAML / on-disk snapshot keys), and remove the narrated parsing details
that duplicate the assertions. Target the comment attached to the PkgVerPeer
(suffix-only) test case so it explains only the rationale, not the
implementation.
In `@pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs`:
- Around line 714-727: Replace the long regression-test doc block above the test
with a minimal intent summary: keep the issue reference (e.g., `#11939`) and a
short parity note about matching pnpm behavior, and remove the step-by-step
narration and numbered steps; locate the comment immediately preceding the test
(the block starting with "Regression for [`#11939`]" above the #[test]) and trim
it to a one- or two-line explanation that conveys only the reason for the test
and the parity note.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c0642b4-ad7d-436e-b410-5eee4954d605
📒 Files selected for processing (5)
pacquet/crates/deps-path/src/suffix_index.rspacquet/crates/lockfile/src/pkg_name_ver_peer/tests.rspacquet/crates/lockfile/src/pkg_ver_peer/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/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). (7)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- 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 firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.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 plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas 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 manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, 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 (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.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. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/lockfile/src/pkg_ver_peer/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/deps-path/src/suffix_index.rspacquet/crates/lockfile/src/pkg_name_ver_peer/tests.rs
🧠 Learnings (4)
📚 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/lockfile/src/pkg_ver_peer/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/deps-path/src/suffix_index.rspacquet/crates/lockfile/src/pkg_name_ver_peer/tests.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/lockfile/src/pkg_ver_peer/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/deps-path/src/suffix_index.rspacquet/crates/lockfile/src/pkg_name_ver_peer/tests.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/lockfile/src/pkg_ver_peer/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/deps-path/src/suffix_index.rspacquet/crates/lockfile/src/pkg_name_ver_peer/tests.rs
📚 Learning: 2026-05-20T23:23:40.606Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11788
File: pacquet/crates/lockfile/src/snapshot_dep_ref.rs:52-56
Timestamp: 2026-05-20T23:23:40.606Z
Learning: In the pacquet lockfile crate, the link target is intentionally represented as a raw `Link(String)` (not a `LinkTarget` newtype) for both `SnapshotDepRef::Link` and `ImporterDepVersion::Link`. pnpm upstream preserves `link:` targets verbatim with only a prefix check (e.g., `refToRelative`), so do not suggest introducing a `LinkTarget` newtype in a bug-fix/targeted PR. If type branding is ever desired, it should be done together for both variants in a separate dedicated refactor PR rather than as part of a small fix.
Applied to files:
pacquet/crates/lockfile/src/pkg_ver_peer/tests.rspacquet/crates/lockfile/src/pkg_name_ver_peer/tests.rs
🪛 GitHub Check: Spell Check
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
[warning] 301-301:
"mis" should be "miss" or "mist".
🔇 Additional comments (1)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs (1)
16-16: LGTM!Also applies to: 237-248, 295-305
Port upstream's [`isLinkedDependency` arm of `resolveDependencies.ts`](https://github.com/pnpm/pnpm/blob/cc4ff817aa/installing/deps-resolver/src/resolveDependencies.ts#L926-L937) plus the `depth === -1` short-circuit in [`resolvePeers.ts`](https://github.com/pnpm/pnpm/blob/cc4ff817aa/installing/deps-resolver/src/resolvePeers.ts#L396). Workspace `link:` deps now: 1. Skip the child-recursion in the tree walker — `TreeChildren` is `Realized(empty)`, matching upstream's `children: {}`. The link target resolves its own deps as a separate importer; threading them under the parent's tree was already wrong. 2. Carry `depth = -1` on the tree node. Pacquet's `ResolvedTree` always set this to the caller's `depth` before; the `isLinkedDependency` branch in upstream's walker pins it at `-1` and the peer-resolution pass keys off that. 3. Carry empty `peer_dependencies` on the `ResolvedPackage`. Pnpm's linked-arm `resolvedPackage` is `{ name, version }` only — peer matching is the linked importer's concern, not the parent's. 4. Use a leaf [`NodeId`] (collapsed to one shared id per workspace path), matching `createNodeIdForLinkedLocalPkg`. In `resolve_peers.rs::resolve_node`, the existing `purePkgs` fast path now starts with the `tree_node.depth == -1` arm. The link node's depPath stays `link:<rel-path>` verbatim — no peer-graph suffix — and the node never enters `self.graph`. With the resolver-level fix in place, the two workarounds added in the previous commit to `dependencies_graph_to_lockfile.rs` go away: - `build_packages_and_snapshots`'s `if dep_path.starts_with("link:") continue` is dropped — link nodes are no longer in the graph at all. - `importer_dep_version`'s `remove_suffix` call on link depPaths is dropped — the depPath is already bare. `build_importer` now resolves the importer's `link:` entries directly from the depPath string instead of looking up a (now-missing) graph node. Regression test in `resolving-deps-resolver` exercises the babylon shape via the stub resolver — a workspace dep with peers in its manifest must produce a tree node with `depth = -1`, empty children, and zero peer_dependencies on the package.
Two depPath helpers from [`deps/path/src/index.ts`](https://github.com/pnpm/pnpm/blob/cc4ff817aa/deps/path/src/index.ts): - `is_runtime_dep_path`: matches `^(?:node|bun|deno)@runtime:` — pnpm uses this to filter the runtime-only install pass and the virtual-store iteration that pacquet hasn't ported yet. Pure byte-level prefix check, no parsing. - `try_get_package_id`: strips the peer-graph suffix and the `(patch_hash=…)` segment, then drops the leading `<name>@` prefix on URL-shaped resolution ids (`<name>@<url>`). `runtime:` entries are carved out — they keep `<name>@runtime:<version>` because the pkgId carries the engine name by design. Tests mirror pnpm's `isRuntimeDepPath` and `tryGetPackageId` cases from `deps/path/test/index.ts`, plus a few extras for the URL-shape / runtime carve-out / scoped-name branches the upstream suite doesn't exercise directly.
…p only Three sites built a [`PkgIdWithPatchHash`] from the snapshot key incorrectly. Pnpm's [`getPkgIdWithPatchHash`](https://github.com/pnpm/pnpm/blob/cc4ff817aa/deps/path/src/index.ts#L63-L70) strips only the peer-graph suffix; it keeps `(patch_hash=…)`. Pacquet was either stripping both (via `PkgNameVerPeer::without_peer`) or neither (via `to_string()` on the full snapshot key) — neither matches upstream once patches are involved. - `virtual_store_layout::lockfile_to_dep_graph` was stripping both, so the side-effects-cache key for a patched package lost its `(patch_hash=…)` segment. - `create_virtual_store`'s two `cas_paths_by_pkg_id` insert sites were stripping neither, so peer-variants of the same patched package ended up with distinct entries instead of sharing one. - `hoisted_dep_graph`'s `pkg_id_with_patch_hash` initialiser was stripping neither, with the same peer-variant divergence. All four sites now go through `pacquet_deps_path::get_pkg_id_with_patch_hash`, which does the balanced-paren scan upstream uses. Non-patched packages are unaffected (no patch_hash segment to keep or strip). Pinned by a new `virtual_store_layout::tests::full_pkg_id_keeps_patch_hash_when_present` test that constructs a patched snapshot and checks the resulting `full_pkg_id` starts with `foo@1.0.0(patch_hash=abc):` — verified to fail on the pre-fix code.
Pacquet CI runs the Perfectionist Dylint lint set, which forbids a trailing comma inside a single-line macro invocation (`macro_trailing_comma`). `cargo fmt` happily collapses multi-line macros to single-line and keeps the comma, so the violations only show up under Dylint (not `just ready`). Three asserts in the previous commits got reflowed by `cargo fmt` and tripped the lint. Drop the trailing comma so the single-line shape stays clean. CI failure: <https://github.com/pnpm/pnpm/actions/runs/26419795936/job/77771988376>.
Pacquet CI's Perfectionist Dylint lint rejects U+2026 (`…`) inside `//` line comments. `///` doc comments are accepted (the pre-existing ones in `lockfile/src/resolution.rs` and elsewhere prove the rule scopes to non-doc comments). The four `//` comments I added in the patch_hash fix carried `(patch_hash=…)` — swap them to ASCII `(patch_hash=...)`. Doc comments containing `…` are left untouched. CI failure: <https://github.com/pnpm/pnpm/actions/runs/26420118398/job/77772961966>.
CodeRabbit nitpicks on PR #11944 flagged that the test doc comments restate behavior the assertions already document. Per the repo's `CODE_STYLE_GUIDE.md` ("Doc comments document the contract... not a re-narration of the body" / "Tests are documentation. Do not duplicate them in prose"), trim each block down to the parity rationale and the upstream link. Touched: `pkg_name_ver_peer/tests.rs`, `pkg_ver_peer/tests.rs`, `deps-path/src/suffix_index.rs`. Net -36 lines.
Member
Author
|
Thanks @coderabbitai — addressed the nitpicks in b73fc41. Trimmed the test doc comments down to the parity rationale + upstream link in:
The The spell-check warning on Net -36 lines. Written by an agent (Claude Code, claude-opus-4-7). |
|
Tip For best results, initiate chat on the files or code changes.
🐇✨ |
This was referenced May 25, 2026
zkochan
added a commit
that referenced
this pull request
May 25, 2026
…writer `update_workspace_state` at the end of `Install::run` was called with the wanted lockfile loaded at the *start* of the install. On the fresh-install path (no `pnpm-lock.yaml` in the workspace root) this is `None`, so [`build_projects_map`] fell into its no-lockfile arm and recorded only the root importer's entry — even for an 87-project workspace like babylon. That broke the `optimistic_repeat_install` fast path the next install runs: [`project_structure_matches`] compares the recorded project list against the workspace projects the resolver discovers. With `len = 1` recorded and `len = 87` discovered, the check returned `false` and the install fell through to the full resolve + verify path even though the on-disk state was already a no-op. The freshly-built lockfile (`fresh_lockfile`) is in scope at the call site and carries every importer the resolver walked. Fall back to it via `lockfile.or(fresh_lockfile.as_ref())`. ## Impact Re-bench against the vlt `babylon` fixture (87-project monorepo) shows every `*+node_modules` cell collapsing from "very slow" to "faster than pnpm": | Variation | Before | After | |---|---:|---:| | `node_modules` | 37.87× | 0.09× (11× faster than pnpm) | | `lockfile+node_modules` | 25.52× | 0.07× (14× faster) | | `cache+node_modules` | 11.55× | 0.15× (6.7× faster) | | `cache+lockfile+node_modules` | 11.35× | 0.17× (5.9× faster) | The fast path only failed for workspace installs whose wanted lockfile was absent on the first install of the iteration — i.e. exactly the vlt benchmark's `node_modules` and `*+node_modules` shape. Single-project installs always recorded the root correctly because the no-lockfile fallback already emitted the root entry. Found while validating #11944's claim that the babylon DNF cells would collapse on the next pacquet release (#11902).
zkochan
added a commit
that referenced
this pull request
May 25, 2026
…, not lockfile `build_projects_map` derived workspace projects from `lockfile.importers.keys()`. On the fresh-install path the wanted lockfile is `None` (no `pnpm-lock.yaml` on disk yet), so the function fell into its no-lockfile arm and recorded only the root importer — even for an 87-project workspace like babylon. That broke the [`optimistic_repeat_install`](https://github.com/pnpm/pnpm/blob/6b3ba4d337/pacquet/crates/package-manager/src/optimistic_repeat_install.rs) fast path on the *next* install: `project_structure_matches` compared the recorded project list (`len = 1`) against the workspace projects the resolver discovered (`len = 87`), returned `false`, and the install fell through to the full resolve + verify path even though the on-disk state was already a no-op. ## Fix Upstream pnpm's [`createWorkspaceState`](https://github.com/pnpm/pnpm/blob/cc4ff817aa/workspace/state/src/createWorkspaceState.ts#L14-L27) takes `allProjects: ProjectsList` as input and emits one entry per project — the lockfile isn't involved. Pacquet already builds the matching `project_manifests: &[(PathBuf, &PackageManifest)]` at the top of `Install::run` (for the optimistic-repeat fast path); thread it through to `build_workspace_state` instead of re-deriving from the lockfile. The new `build_projects_map` is half the size of the old one and can't fall into a "lockfile missing" arm — the project list always comes from the same scan the rest of the install uses. ## Impact Re-bench against the vlt babylon fixture (87-project monorepo) shows every \`*+node_modules\` cell collapsing from "very slow" to "faster than pnpm": | Variation | Before | After | |---|---:|---:| | \`node_modules\` | 37.87x | 0.09x (11x faster than pnpm) | | \`lockfile+node_modules\` | 25.52x | 0.07x (14x faster) | | \`cache+node_modules\` | 11.55x | 0.15x (6.7x faster) | | \`cache+lockfile+node_modules\` | 11.35x | 0.17x (5.9x faster) | The fast path only failed for workspace installs whose wanted lockfile was absent on the first install of the iteration — i.e. exactly the vlt benchmark's \`node_modules\` and \`*+node_modules\` shape. Found while validating #11944's claim that the babylon DNF cells would collapse on the next pacquet release (#11902). ## Tests Ports the two scenarios from upstream's [\`createWorkspaceState.test.ts\`](https://github.com/pnpm/pnpm/blob/cc4ff817aa/workspace/state/test/createWorkspaceState.test.ts) into \`install::tests::build_workspace_state_tests\`: - \`empty_project_list_produces_empty_projects_map\` — zero projects in, empty \`projects\` map out, timestamp still populated. - \`records_every_workspace_project_keyed_by_root_dir\` — four projects in, four entries in \`projects\`, each keyed by \`root_dir\`. Verified to fail on the pre-fix shape (returned 1 entry instead of 4) when \`build_projects_map\` is temporarily restricted to the root.
zkochan
added a commit
that referenced
this pull request
May 26, 2026
…, not lockfile (#11946) `build_projects_map` derived workspace projects from `lockfile.importers.keys()`. On the fresh-install path the wanted lockfile is `None` (no `pnpm-lock.yaml` on disk yet), so the function fell into its no-lockfile arm and recorded **only the root importer** — even for an 87-project workspace like babylon. That broke the [`optimistic_repeat_install`](https://github.com/pnpm/pnpm/blob/6b3ba4d337/pacquet/crates/package-manager/src/optimistic_repeat_install.rs) fast path on the *next* install: `project_structure_matches` compared the recorded project list (`len = 1`) against the workspace projects the resolver discovered (`len = 87`), returned `false`, and the install fell through to the full resolve + verify path even though the on-disk state was already a no-op. ## Fix Upstream pnpm's [`createWorkspaceState`](https://github.com/pnpm/pnpm/blob/cc4ff817aa/workspace/state/src/createWorkspaceState.ts#L14-L27) takes `allProjects: ProjectsList` as input and emits one entry per project — the lockfile isn't involved. Pacquet already builds the matching `project_manifests: &[(PathBuf, &PackageManifest)]` at the top of `Install::run` (for the optimistic-repeat fast path); thread it through to `build_workspace_state` instead of re-deriving from the lockfile. The new `build_projects_map` is half the size of the old one and can't fall into a "lockfile missing" arm — the project list always comes from the same scan the rest of the install uses. ## Impact Re-bench against the vlt `babylon` fixture (87-project monorepo) shows every `*+node_modules` cell collapsing from "very slow" to "faster than pnpm": | Variation | Before | After | |---|---:|---:| | `node_modules` | 37.87× | **0.09×** (11× faster than pnpm) | | `lockfile+node_modules` | 25.52× | **0.07×** (14× faster) | | `cache+node_modules` | 11.55× | **0.15×** (6.7× faster) | | `cache+lockfile+node_modules` | 11.35× | **0.17×** (5.9× faster) | These were the four worst cells in the vlt benchmark chart for babylon (all flagged DNF before #11944 fixed the underlying panic). After #11944 babylon stopped DNF'ing but stayed 11-37× slower because of this workspace-state writing bug. The fast path only failed for workspace installs whose wanted lockfile was absent on the first install of the iteration — i.e. exactly the vlt benchmark's `node_modules` and `*+node_modules` shape. Single-project installs always recorded the root correctly because the no-lockfile fallback already emitted the root entry. Found while validating #11944's claim that the babylon DNF cells would collapse on the next pacquet release (#11902).
This was referenced May 27, 2026
This was referenced May 28, 2026
This was referenced Jun 8, 2026
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
pacquet installwas panicking on the babylon vlt fixture under thenode_modulesandlockfile+node_modulesvariations. The crash surfaced inPkgNameVerPeer::without_peer, but the root cause was an unported piece of pnpm's resolver: workspacelink:nodes weren't short-circuited and flowed through peer resolution, producing depPaths of the shapelink:<rel-path>(<peers>)that downstream code wasn't prepared for. This PR ports the missing short-circuit, fixes the panic, and closes the parity gaps the audit surfaced.Workspace-link short-circuit (the root fix)
Ported upstream's
isLinkedDependencyarm plus thedepth === -1short-circuit inresolvePeers.ts. Workspacelink:deps now:TreeChildren::Realized(empty)).depth = -1on the tree node.peer_dependencieson theResolvedPackage— peer matching is the linked importer's concern.NodeId] so every reference to the same workspace path shares one id.resolve_peers::resolve_nodeearly-returns fordepth == -1nodes withdep_path = pkg.id(justlink:<rel-path>). The link node never enters the graph, sopackages:/snapshots:stay clean and the importer'sversion:cell carrieslink:<rel-path>exactly.PkgNameVerPeer::without_peerno longer panicsConstruct the bare key through the typed fields rather than reformatting
{prefix}{version}and re-parsing under.expect(...). The newPkgVerPeer::without_peerclones the existingprefix/versionslots and returns aPkgVerPeerwith an empty peer string. No round-trip, no.expect(...). Defensive even after the upstream fix lands —without_peeris called on values from the lockfile, which can be hand-edited.pkgIdWithPatchHashis now strip-peer-only at every siteFour call sites built a [
PkgIdWithPatchHash] from the wrong baseline:virtual_store_layout::lockfile_to_dep_graphstripped both segments (PkgNameVerPeer::without_peer().to_string()) — patched variants of the samename@versioncollided in the dep-graph hash input.create_virtual_store's twocas_paths_by_pkg_idinserts andhoisted_dep_graph'spkg_id_with_patch_hashinitialiser stripped nothing (rawsnapshot_key.to_string()) — peer variants of the same patched package got separate CAS-paths entries instead of sharing one.All four now go through
pacquet_deps_path::get_pkg_id_with_patch_hash(the balanced-paren scan upstream'sgetPkgIdWithPatchHashuses): strip the peer-graph suffix, keep(patch_hash=…). Non-patched packages are unaffected.New helpers in
pacquet-deps-pathis_runtime_dep_path— matches^(?:node|bun|deno)@runtime:byte-level. Pnpm filters the runtime-only install pass with this.try_get_package_id— strips peer-graph + patch-hash suffix, then drops the<name>@prefix on URL-shaped resolution ids while keepingruntime:entries intact.Ported
deps/path/test/index.tscasespacquet-deps-path::suffix_indexpacquet-deps-path::is_runtime_dep_pathisRuntimeDepPathtest + carve-outspacquet-deps-path::try_get_package_idtryGetPackageIdtest + URL-shape, bare, runtime carve-outspacquet-lockfile::PkgVerPeerpacquet-lockfile::PkgNameVerPeerpacquet-resolving-deps-resolver::testsdepth = -1, empty children, no peer_dependenciespacquet-package-manager::virtual_store_layout::testsfull_pkg_idretains(patch_hash=…)Resolves #11939.
Test plan
cargo nextest run -p pacquet-deps-path -p pacquet-lockfile -p pacquet-resolving-deps-resolver -p pacquet-package-manager— 574/574 pass.cargo clippy --locked --workspace --all-targets -- --deny warnings— clean.cargo fmt --check— clean.pacquet installon the babylon fixture: the panic is gone, the install completes (exit=0),@dev/build-toolscorrectly symlinks to the workspace sibling.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
Tests