fix(pacquet): drop synthetic transitive-peer entries from the importer lockfile#11877
Conversation
…r lockfile Aligns `dependencies_graph_to_lockfile` with pnpm's `addDirectDependenciesToLockfile`, which only writes manifest-declared aliases into `importers["."].dependencies` / `importers["."].specifiers`. The earlier synthetic-specifier branch (added in #11867 to keep the top-level symlinks the old recursive walker happened to produce) wrote the resolver's transitive auto-installed peers into the importer block with the resolved version as their specifier, so `pnpm-lock.yaml` ended up listing entries the on-disk `package.json` never declared. `satisfies_package_manifest` then read those entries on the next install, flagged 17 "removed" specifiers against the alotta-files fixture, and the auto-frozen fast path (`preferFrozenLockfile: true`) fell through to the full fresh-resolve pipeline — turning the bench's `withWarmCacheAndLockfile` / `withWarmModulesAndLockfile` / `repeatInstall` scenarios from sub-second no-ops into ~4.5 s re-resolutions in pacquet@0.2.5. Verified against pnpm itself (`pnpm install react-dom@18.2.0` → lockfile lists only `react-dom` in `importers["."].dependencies`, no top-level `react` symlink). Pacquet now produces the same shape: transitive auto-installed peers live in `snapshots:` / `packages:` only, consumers reach them through their parent's slot's `node_modules`, and the freshness gate stays clean across installs. Local reproduction on the alotta-files fixture: - importer.dependencies count: 126 → 109 (matches the manifest exactly) - `pacquet install --frozen-lockfile`: was failing with "17 dependencies were removed", now succeeds - `repeatInstall`: ~22 s → ~0.5 s Test updates: - `auto_install_peers_hoists_missing_peers_at_importer` keeps the `.pnpm/` virtual-store assertions (the actual hoist-loop contract) and drops the top-level `node_modules/<peer>` symlink loop, which was asserting behavior pnpm itself doesn't produce. - `should_install_dependencies` snapshot drops the three `node_modules/@pnpm/{x,y,z}` entries for the same reason — those are `@pnpm/xyz`'s transitive peers and never reach the importer's `node_modules` under pnpm. Refs #11867. --- Written by an agent (Claude Code, claude-opus-4-7).
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? |
📝 WalkthroughWalkthroughThe PR enforces that direct dependencies in the root importer must have a manifest-provided specifier to be included in ChangesAuto-installed peers specifier handling alignment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/cli/tests/install.rs (1)
321-329: ⚡ Quick winAssert the non-hoisting behavior directly.
The updated prose now documents that these peers must not appear under
node_modules/<alias>, but the test only checks that the.pnpmslots exist. Adding explicit negative assertions here would keep this regression pinned if importer-level hoisting ever sneaks back in.Suggested test addition
for peer in ["peer-a", "peer-b", "peer-c"] { // The registry's `^1.0.0` resolves to the latest 1.x; assert on // the slot prefix rather than a specific version so a registry // bump doesn't churn this test. let prefix = format!("`@pnpm.e2e`+{peer}`@1`."); assert!( entries.iter().any(|name| name.starts_with(&prefix) && !name.contains('_')), "expected {peer} to be auto-installed; .pnpm/ entries: {entries:?}", ); + assert!( + !workspace.join(format!("node_modules/@pnpm.e2e/{peer}")).exists(), + "expected {peer} to stay out of the importer root", + ); }As per coding guidelines: Port relevant tests from pnpm when porting behavior. Matching test coverage is the easiest way to prove behavioral parity.
🤖 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/cli/tests/install.rs` around lines 321 - 329, The test currently only asserts that the `.pnpm` snapshot/slots exist but does not explicitly verify importer-level hoisting is absent; update the test in install.rs to add explicit negative assertions that the transitive peer package is NOT present under the importer's node_modules/<alias> (i.e., make assertions that the path node_modules/<peer_alias> does not exist and that requiring/looking up that package from the importer resolves via the parent slot rather than importer-level node_modules), alongside the existing checks for `.pnpm` slots to lock in the non-hoisting behavior.
🤖 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/cli/tests/install.rs`:
- Around line 321-329: The test currently only asserts that the `.pnpm`
snapshot/slots exist but does not explicitly verify importer-level hoisting is
absent; update the test in install.rs to add explicit negative assertions that
the transitive peer package is NOT present under the importer's
node_modules/<alias> (i.e., make assertions that the path
node_modules/<peer_alias> does not exist and that requiring/looking up that
package from the importer resolves via the parent slot rather than
importer-level node_modules), alongside the existing checks for `.pnpm` slots to
lock in the non-hoisting behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d41af95-81c0-4822-8615-7a247e4563d4
⛔ Files ignored due to path filters (1)
pacquet/crates/package-manager/src/install/snapshots/pacquet_package_manager__install__tests__should_install_dependencies.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
pacquet/crates/cli/tests/install.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.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: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Doc
- GitHub Check: Dylint
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Analyze (javascript)
- 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 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/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/cli/tests/install.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 withtempfile::TempDir, the mocked registry, or an integration test that spawns the actual binary.
Use the DI seam — a capability trait on theHostprovider, threaded asSys: <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 scalarassert_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. Useallow_known_failure!at the not-yet-implemented boundary and update checkboxes as items land.
Files:
pacquet/crates/cli/tests/install.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/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/cli/tests/install.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/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/cli/tests/install.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/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/cli/tests/install.rs
🔇 Additional comments (1)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs (1)
119-132: LGTM!
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11877 +/- ##
==========================================
- Coverage 87.81% 87.81% -0.01%
==========================================
Files 205 205
Lines 24422 24424 +2
==========================================
+ Hits 21446 21447 +1
- Misses 2976 2977 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.50792000646,
"stddev": 0.18736234717319522,
"median": 2.42897703066,
"user": 2.7560772799999995,
"system": 3.6658071399999996,
"min": 2.3893933886600003,
"max": 2.98038094666,
"times": [
2.45580500666,
2.65829974666,
2.39279203566,
2.40376716466,
2.55037815466,
2.98038094666,
2.40478040666,
2.45317365466,
2.3893933886600003,
2.3904295596600003
]
},
{
"command": "pacquet@main",
"mean": 2.43690308716,
"stddev": 0.07428318714383675,
"median": 2.45931301366,
"user": 2.7193565800000004,
"system": 3.6428010399999997,
"min": 2.28661539066,
"max": 2.51201365766,
"times": [
2.4017957666600003,
2.40641171966,
2.44272925266,
2.49197319166,
2.28661539066,
2.35446288466,
2.4866607316600002,
2.51201365766,
2.51047150166,
2.4758967746600002
]
},
{
"command": "pnpm",
"mean": 4.960277414160001,
"stddev": 0.07467883684868314,
"median": 4.97176484866,
"user": 8.38746048,
"system": 4.285187339999999,
"min": 4.82982719566,
"max": 5.07183214266,
"times": [
4.82982719566,
4.95239999266,
4.96573991566,
4.99794499366,
5.07183214266,
4.85711182066,
4.91752043766,
5.00724912466,
4.97778978166,
5.02535873666
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6779085628400001,
"stddev": 0.021866722680765328,
"median": 0.6709617038400001,
"user": 0.38347319999999996,
"system": 1.47627192,
"min": 0.6551675478400001,
"max": 0.7261918028400001,
"times": [
0.7261918028400001,
0.69826565184,
0.6673196158400001,
0.6579814808400001,
0.6722731968400001,
0.69247811684,
0.67659295184,
0.66316505284,
0.6551675478400001,
0.6696502108400001
]
},
{
"command": "pacquet@main",
"mean": 0.69849782744,
"stddev": 0.04436103258674018,
"median": 0.6837926138400001,
"user": 0.384107,
"system": 1.47066962,
"min": 0.6575702388400001,
"max": 0.76917783484,
"times": [
0.7420835398400001,
0.6947738708400001,
0.6936700948400001,
0.6575702388400001,
0.6695300458400001,
0.76917783484,
0.6584799558400001,
0.67391513284,
0.65976430184,
0.76601325884
]
},
{
"command": "pnpm",
"mean": 2.62237617854,
"stddev": 0.11595518477568215,
"median": 2.58381368484,
"user": 3.2852796000000004,
"system": 2.2932282200000005,
"min": 2.53520231384,
"max": 2.91151005784,
"times": [
2.68857268784,
2.59523192284,
2.91151005784,
2.58276935684,
2.54942446484,
2.54268897484,
2.58485801284,
2.53520231384,
2.54610359384,
2.68740039984
]
}
]
}Scenario: Clean Install
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.9299839658,
"stddev": 0.09281557652579207,
"median": 4.9391167836,
"user": 6.6481554,
"system": 3.623268000000001,
"min": 4.7221713031000006,
"max": 5.0430965611000005,
"times": [
4.9328724181000005,
4.992534683100001,
5.0192892301,
4.8978246381,
4.8773216101,
5.0430965611000005,
4.9453611491,
4.881922927100001,
4.7221713031000006,
4.9874451381
]
},
{
"command": "pacquet@main",
"mean": 4.935078143599999,
"stddev": 0.0801722051689804,
"median": 4.9265088411,
"user": 6.6532184999999995,
"system": 3.6387232,
"min": 4.8307496681,
"max": 5.0613263501,
"times": [
5.0170048001000005,
4.8431143901,
5.039684401100001,
4.8834274981000005,
4.9312953841,
4.921722298100001,
4.8898353471,
4.8307496681,
4.9326212991,
5.0613263501
]
},
{
"command": "pnpm",
"mean": 6.533424304600001,
"stddev": 0.10886185710828748,
"median": 6.5357766626,
"user": 10.8335821,
"system": 4.5064161,
"min": 6.3242639361,
"max": 6.6766347561,
"times": [
6.4532152231,
6.5731254641,
6.498427861100001,
6.6150134531,
6.6448400391,
6.6086894761,
6.6766347561,
6.4839840821,
6.4560487551,
6.3242639361
]
}
]
}Scenario: Full Resolution
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 3.8703902309,
"stddev": 0.17819984730665855,
"median": 3.8021391971000003,
"user": 4.261956360000001,
"system": 2.1946637399999993,
"min": 3.6999006841,
"max": 4.1898728271,
"times": [
3.7085229381,
3.7726675091,
4.1552682981,
3.8071225771000003,
3.9792679001000004,
3.7433028111000004,
3.8508209471000003,
3.7971558171,
4.1898728271,
3.6999006841
]
},
{
"command": "pacquet@main",
"mean": 3.7493200646,
"stddev": 0.09328174071033918,
"median": 3.7090621756,
"user": 4.152691259999999,
"system": 2.179628739999999,
"min": 3.6469250171,
"max": 3.9197930841,
"times": [
3.8746749681000003,
3.7255335121,
3.6838142121,
3.7121483871,
3.6846343401,
3.6469250171,
3.9197930841,
3.6994616901,
3.7059759641000003,
3.8402394711000003
]
},
{
"command": "pnpm",
"mean": 4.3565003348,
"stddev": 0.04352210338009613,
"median": 4.3553820381,
"user": 5.543326859999999,
"system": 2.6109623399999995,
"min": 4.2643759291,
"max": 4.409721520100001,
"times": [
4.409721520100001,
4.349320485100001,
4.3572508481000005,
4.2643759291,
4.3919901101,
4.4035313531,
4.314409604100001,
4.3744617501,
4.3464285201,
4.353513228100001
]
}
]
} |
Summary
dependencies_graph_to_lockfilewith pnpm'saddDirectDependenciesToLockfile: transitive auto-installed peers no longer get written intoimporters["."].dependencies/specifiers. They stay insnapshots:/packages:only, matching whatpnpm installitself produces (verified locally withpnpm install react-dom@18.2.0).preferFrozenLockfile: true) for installs whose lockfile is up-to-date —withWarmCacheAndLockfile/withWarmModulesAndLockfile/repeatInstallwere ~5–25× slower in pacquet@0.2.5 than in 0.2.4 because the synthetic entries failedsatisfies_package_manifestand pushed every install onto the full fresh-resolve pipeline.Why
#11867 routed the fresh-lockfile install through
CreateVirtualStore+SymlinkDirectDependencies, which both read fromimporter.dependencies. To keep the top-levelnode_modules/<peer>symlinks the old recursive walker happened to produce, the PR also taughtbuild_root_importerto write synthetic entries for auto-installed peers — using the resolved version as their specifier.Those entries then landed in
pnpm-lock.yaml(17 of them against the alotta-files fixture:@babel/core,typescript,@types/node, …). On the next install:satisfies_package_manifestflagged them as "removed" specifierspreferFrozenLockfilesawStaleand fell through to the fresh-resolve pathpacquet install --frozen-lockfilefailed outright withERR_PNPM_OUTDATED_LOCKFILElisting all 17This is visible on the published benchmark page: 0.2.4
withWarmCacheAndLockfilewas ~720 ms, 0.2.5 jumped to ~4500 ms;withWarmModulesAndLockfilewent from ~220 ms to ~5000 ms.Pnpm itself doesn't put transitive auto-installed peers in
importer.dependencies—addDirectDependenciesToLockfileiterates only overgetAllDependenciesFromManifest(manifest). Confirmed by inspecting a realpnpm install react-dom@18.2.0lockfile: onlyreact-dominimporters["."].dependencies, no top-levelreactsymlink.Per the cardinal rule (pnpm is the source of truth, pacquet ports from it), pacquet should match. This PR restores the
continuefor aliases the manifest doesn't declare and updates the two tests that were asserting the divergent top-level-symlink behavior.Local verification on the alotta-files fixture
importer.dependenciesentry countpacquet install --frozen-lockfilepacquet installTest plan
cargo nextest run --release -p pacquet-package-manager— 298/298 passcargo nextest run --release -p pacquet-cli --test install— 10/10 pass (including the updatedauto_install_peers_hoists_missing_peers_at_importer)cargo nextest run --release -p pacquet-lockfile— 154/154 passcargo clippy --release --locked --all-targets -- -D warningsclean on the affected cratescargo fmt --checkcleanpacquet installon the alotta-files fixture — lockfile clean, second install fast,--frozen-lockfileno longer errorsRefs #11867.
Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
node_modules, and missing peer specifiers are now properly handled during the installation process.