feat(pacquet/package-manager): port dedupeDirectDeps setting#12024
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPorts pnpm's dedupeDirectDeps into pacquet: adds config/env/workspace plumbing, refactors SymlinkDirectDependencies to dedupe against root and public-hoist targets, precomputes hoist plans for fresh/frozen installs, threads results through install flows, updates cache-drift checks, and adds integration tests. ChangesdedupeDirectDeps Setting Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12024 +/- ##
==========================================
+ Coverage 88.50% 88.62% +0.11%
==========================================
Files 230 230
Lines 29225 29390 +165
==========================================
+ Hits 25865 26046 +181
+ Misses 3360 3344 -16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.2069384441199995,
"stddev": 0.14256881711814795,
"median": 2.1709572917199997,
"user": 2.0811899200000004,
"system": 2.49297732,
"min": 2.0241416817199998,
"max": 2.54357199872,
"times": [
2.31313398772,
2.11900041372,
2.12634068372,
2.54357199872,
2.1510742287199998,
2.0241416817199998,
2.1422056497199997,
2.26192339772,
2.1908403547199997,
2.19715204472
]
},
{
"command": "pacquet@main",
"mean": 2.23339220222,
"stddev": 0.12252718241278977,
"median": 2.23192981972,
"user": 2.1052073200000003,
"system": 2.4725641200000004,
"min": 2.05560776672,
"max": 2.40146170672,
"times": [
2.40146170672,
2.39647421272,
2.22874568472,
2.29575991672,
2.3137308657199998,
2.1115489847199997,
2.08148749072,
2.23511395472,
2.21399143872,
2.05560776672
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.77833542294,
"stddev": 0.08538308927954247,
"median": 0.7330850467400001,
"user": 0.27640232,
"system": 1.0329354800000001,
"min": 0.71305137124,
"max": 0.94176178124,
"times": [
0.94176178124,
0.71305137124,
0.73438960324,
0.73228756224,
0.91982872724,
0.73388253124,
0.81802580424,
0.73185828224,
0.72619703424,
0.73207153224
]
},
{
"command": "pacquet@main",
"mean": 0.83440240224,
"stddev": 0.12792502895374422,
"median": 0.7795657552399999,
"user": 0.28216611999999996,
"system": 1.0306199800000002,
"min": 0.71831114724,
"max": 1.04393376124,
"times": [
1.04393376124,
1.02076172024,
0.72527711524,
0.93615807324,
0.78633714424,
0.77279436624,
0.72618044324,
0.71831114724,
0.72465607724,
0.88961417424
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.2761814407,
"stddev": 0.09393720100901484,
"median": 2.2466812784,
"user": 3.0245132,
"system": 2.4116223999999997,
"min": 2.1637009469,
"max": 2.4247841639,
"times": [
2.3022299479000004,
2.1637009469,
2.1640850399000002,
2.4247841639,
2.2369421469,
2.3309684269,
2.2246967939,
2.4245922719000004,
2.2333942589,
2.2564204099
]
},
{
"command": "pacquet@main",
"mean": 2.247217844,
"stddev": 0.07587266780909302,
"median": 2.2192482229,
"user": 3.0053012000000003,
"system": 2.3242053,
"min": 2.1705165729000004,
"max": 2.3914592159000003,
"times": [
2.3645018279000003,
2.2045919869,
2.1955995439000002,
2.3914592159000003,
2.1909634919000003,
2.2745920949,
2.1945414719,
2.2515077749000003,
2.2339044589,
2.1705165729000004
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.4741427586800002,
"stddev": 0.05336264530060736,
"median": 1.44741001908,
"user": 1.34337654,
"system": 1.4506647,
"min": 1.4008945650800002,
"max": 1.54793280508,
"times": [
1.54793280508,
1.44532422808,
1.4008945650800002,
1.4400574800800001,
1.4312384310800002,
1.51448150608,
1.4494958100800002,
1.52498492808,
1.44147491708,
1.54554291608
]
},
{
"command": "pacquet@main",
"mean": 1.38700302998,
"stddev": 0.16021119382606772,
"median": 1.3028482510800001,
"user": 1.33489134,
"system": 1.4081791000000001,
"min": 1.2954805040800002,
"max": 1.79242832708,
"times": [
1.30232982908,
1.30328298508,
1.2954805040800002,
1.50952668608,
1.79242832708,
1.3024135170800002,
1.30158342008,
1.3200320040800002,
1.3020042420800002,
1.4409487850800002
]
}
]
} |
|
Bencher Report| Branch | pr/12024 |
| 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,276.18 ms(-0.64%)Baseline: 2,290.91 ms | 2,749.09 ms (82.80%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,474.14 ms(+3.27%)Baseline: 1,427.42 ms | 1,712.91 ms (86.06%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,206.94 ms(+7.20%)Baseline: 2,058.80 ms | 2,470.56 ms (89.33%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 778.34 ms(+15.03%)Baseline: 676.66 ms | 812.00 ms (95.85%) |
f34653a to
e2ddde3
Compare
Review Summary by QodoPort pnpm's dedupeDirectDeps setting with public-hoist support
WalkthroughsDescription• Port pnpm's dedupeDirectDeps setting end-to-end to pacquet - New dedupe_direct_deps: bool config field (default true) - Wired through pnpm-workspace.yaml and env override • Dedupe direct deps against workspace root by filtering non-root symlinks - Root importer's resolved targets computed upfront - Non-root importers skip symlinks matching root's targets • Extend dedupe to cover publicly-hoisted root dependencies - Pre-compute hoist plan before symlink phase - Feed public-hoist targets into dedupe map • Add comprehensive integration tests covering dedupe scenarios - Default-on behavior, opt-out flag, partial dedupe, frozen-lockfile - Public-hoist dedupe interaction via frozen-lockfile path Diagramflowchart LR
Config["Config: dedupe_direct_deps"]
YAML["pnpm-workspace.yaml"]
Env["DEDUPE_DIRECT_DEPS env"]
HoistPlan["Pre-compute HoistPlan"]
Dedupe["SymlinkDirectDependencies dedupe pass"]
RootTargets["Root importer targets"]
PublicHoist["Public-hoist targets"]
NonRootFilter["Filter non-root symlinks"]
State["WorkspaceState settings"]
YAML --> Config
Env --> Config
Config --> Dedupe
Config --> HoistPlan
HoistPlan --> PublicHoist
Dedupe --> RootTargets
PublicHoist --> Dedupe
RootTargets --> NonRootFilter
Dedupe --> State
File Changes1. pacquet/crates/cli/tests/dedupe_direct_deps.rs
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/package-manager/src/symlink_direct_dependencies.rs`:
- Around line 397-412: The dedupe currently compares raw PathBufs (in the
entries filter using root_targets.get(&entry.name_str) ... t != &entry.target)
but link targets are created with project_dir.join(candidate) and may contain
`.`/`..` so equivalent locations can compare unequal; fix by applying a lexical
path-normalization function to both sides before comparing (i.e., normalize the
link-target produced in ImporterDepVersion::Link / where
project_dir.join(candidate) is constructed and normalize entry.target and the
values stored in dedupe_against), add a small helper like
normalize_lexical_path(...) that strips `.`/`..` components (not
fs::canonicalize) and use it in the entries filter and when populating
root_targets so comparisons use normalized paths.
🪄 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: ccf6076f-9a5b-472d-a0d4-0e05d85b26c0
📒 Files selected for processing (12)
pacquet/crates/cli/tests/dedupe_direct_deps.rspacquet/crates/config/src/env_overlay.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install_frozen_lockfile.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/symlink_direct_dependencies.rspacquet/crates/package-manager/src/symlink_direct_dependencies/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). (4)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Analyze (javascript)
🧰 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/install.rspacquet/crates/config/src/env_overlay.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/symlink_direct_dependencies/tests.rspacquet/crates/cli/tests/dedupe_direct_deps.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/install_frozen_lockfile.rspacquet/crates/package-manager/src/symlink_direct_dependencies.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/dedupe_direct_deps.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/install.rspacquet/crates/config/src/env_overlay.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/symlink_direct_dependencies/tests.rspacquet/crates/cli/tests/dedupe_direct_deps.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/install_frozen_lockfile.rspacquet/crates/package-manager/src/symlink_direct_dependencies.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/install.rspacquet/crates/config/src/env_overlay.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/symlink_direct_dependencies/tests.rspacquet/crates/cli/tests/dedupe_direct_deps.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/install_frozen_lockfile.rspacquet/crates/package-manager/src/symlink_direct_dependencies.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/install.rspacquet/crates/config/src/env_overlay.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/symlink_direct_dependencies/tests.rspacquet/crates/cli/tests/dedupe_direct_deps.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/install_frozen_lockfile.rspacquet/crates/package-manager/src/symlink_direct_dependencies.rs
🔇 Additional comments (11)
pacquet/crates/config/src/lib.rs (1)
608-620: LGTM!pacquet/crates/config/src/workspace_yaml.rs (1)
158-158: LGTM!Also applies to: 416-416, 509-510
pacquet/crates/config/src/env_overlay.rs (1)
146-146: LGTM!pacquet/crates/package-manager/src/symlink_direct_dependencies/tests.rs (1)
94-94: LGTM!Also applies to: 222-222, 306-306, 372-372, 452-452, 521-521, 600-600
pacquet/crates/package-manager/src/install_frozen_lockfile.rs (1)
32-32: LGTM!Also applies to: 616-637, 647-647, 810-814, 876-932, 1205-1312
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (1)
866-870: LGTM!pacquet/crates/package-manager/src/install_package_from_registry/tests.rs (1)
55-55: LGTM!pacquet/crates/package-manager/src/install.rs (1)
1404-1405: LGTM!pacquet/crates/package-manager/src/optimistic_repeat_install.rs (1)
170-176: LGTM!Also applies to: 200-200, 261-261
pacquet/crates/package-manager/src/optimistic_repeat_install/tests.rs (1)
470-509: LGTM!Also applies to: 513-519
pacquet/crates/cli/tests/dedupe_direct_deps.rs (1)
1-419: LGTM!
|
Actionable comments posted: 0 |
…pe comparison `PathBuf::eq` is a byte-for-byte comparison, so a non-root `link:../shared` in `packages/sibling` resolved to `<ws>/packages/sibling/../shared` would not match a root `link:packages/shared` resolved to `<ws>/packages/shared` — equivalent on disk but lexically distinct. Pnpm's `linkDirectDepsAndDedupe` runs `path.relative` on stored symlink targets, and Node normalises both arguments through `path.resolve` before comparing — so pnpm dedupes these. Run the joined link target through `pacquet_fs::lexical_normalize` to bring pacquet to parity. Adds `dedupes_link_deps_resolving_to_the_same_dir_via_different_segments` exercising the case and verified to fail without the normalization. Reported by CodeRabbit on #12024. --- Written by an agent (Claude Code, claude-opus-4-7).
|
Actionable comments posted: 0 |
…pe comparison `PathBuf::eq` is a byte-for-byte comparison, so a non-root `link:../shared` in `packages/sibling` resolved to `<ws>/packages/sibling/../shared` would not match a root `link:packages/shared` resolved to `<ws>/packages/shared` — equivalent on disk but lexically distinct. Pnpm's `linkDirectDepsAndDedupe` runs `path.relative` on stored symlink targets, and Node normalises both arguments through `path.resolve` before comparing — so pnpm dedupes these. Run the joined link target through `pacquet_fs::lexical_normalize` to bring pacquet to parity. Adds `dedupes_link_deps_resolving_to_the_same_dir_via_different_segments` exercising the case and verified to fail without the normalization. Reported by CodeRabbit on #12024. --- Written by an agent (Claude Code, claude-opus-4-7).
f8a23f7 to
e79e0f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pacquet/crates/cli/tests/dedupe_direct_deps.rs (1)
64-80: ⚡ Quick winAdd diagnostic logging before non-
assert_eq!assertions in these integration tests.These tests use many non-
assert_eq!assertions without pre-assert logging, which makes CI failures harder to triage under the repo’s test style rules.As per coding guidelines, "Log before non-
assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!."Also applies to: 129-138, 190-210, 202-210, 277-287, 351-361, 435-450, 520-538
🤖 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/dedupe_direct_deps.rs` around lines 64 - 80, Add diagnostic logging immediately before each non-assert_eq! assertion in this test (and the other ranges called out) so CI failures show context; for the shown assertions, print the evaluated values for is_symlink_or_junction(...) and the Path existence check (e.g., the result of is_symlink_or_junction and workspace.join("node_modules/@pnpm.e2e/hello-world-js-bin"), and the boolean/value of workspace.join("packages/dup/node_modules").exists()). Use simple debug prints (eprintln! or dbg!) or dbg! for complex structures, placed directly above the assert! calls that reference is_symlink_or_junction and the workspace.join(...) paths, and do the same for the other listed assertion blocks (lines 129-138, 190-210, 202-210, 277-287, 351-361, 435-450, 520-538).
🤖 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/dedupe_direct_deps.rs`:
- Around line 64-80: Add diagnostic logging immediately before each
non-assert_eq! assertion in this test (and the other ranges called out) so CI
failures show context; for the shown assertions, print the evaluated values for
is_symlink_or_junction(...) and the Path existence check (e.g., the result of
is_symlink_or_junction and
workspace.join("node_modules/@pnpm.e2e/hello-world-js-bin"), and the
boolean/value of workspace.join("packages/dup/node_modules").exists()). Use
simple debug prints (eprintln! or dbg!) or dbg! for complex structures, placed
directly above the assert! calls that reference is_symlink_or_junction and the
workspace.join(...) paths, and do the same for the other listed assertion blocks
(lines 129-138, 190-210, 202-210, 277-287, 351-361, 435-450, 520-538).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fd1eda06-918c-4bcb-8103-1310078f3ca8
⛔ Files ignored due to path filters (4)
pacquet/crates/cli/tests/snapshots/add__should_install_all_dependencies.snapis excluded by!**/*.snappacquet/crates/cli/tests/snapshots/add__should_symlink_correctly.snapis excluded by!**/*.snappacquet/crates/cli/tests/snapshots/install__should_install_dependencies.snapis excluded by!**/*.snappacquet/crates/package-manager/src/install/snapshots/pacquet_package_manager__install__tests__should_install_dependencies.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
pacquet/crates/cli/tests/dedupe_direct_deps.rspacquet/crates/config/src/env_overlay.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install_frozen_lockfile.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/package-manager/src/optimistic_repeat_install/tests.rspacquet/crates/package-manager/src/symlink_direct_dependencies.rspacquet/crates/package-manager/src/symlink_direct_dependencies/tests.rs
✅ Files skipped from review due to trivial changes (1)
- pacquet/crates/package-manager/src/install.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
- pacquet/crates/package-manager/src/lib.rs
- pacquet/crates/package-manager/src/symlink_direct_dependencies/tests.rs
- pacquet/crates/config/src/lib.rs
- pacquet/crates/package-manager/src/optimistic_repeat_install/tests.rs
- pacquet/crates/package-manager/src/install_frozen_lockfile.rs
- pacquet/crates/package-manager/src/optimistic_repeat_install.rs
- pacquet/crates/package-manager/src/symlink_direct_dependencies.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: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- 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/config/src/env_overlay.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/cli/tests/dedupe_direct_deps.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/dedupe_direct_deps.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/env_overlay.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/cli/tests/dedupe_direct_deps.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/env_overlay.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/cli/tests/dedupe_direct_deps.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/env_overlay.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/cli/tests/dedupe_direct_deps.rs
🔇 Additional comments (3)
pacquet/crates/config/src/workspace_yaml.rs (1)
135-135: LGTM!Also applies to: 159-160, 414-414, 419-420, 510-510, 514-515, 518-518
pacquet/crates/config/src/env_overlay.rs (1)
141-141: LGTM!Also applies to: 147-148
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (1)
164-175: LGTM!Also applies to: 563-567, 574-574, 600-600, 605-607, 882-905, 914-914, 919-949, 1046-1046, 1120-1120
|
Actionable comments posted: 0 |
Per `CODE_STYLE_GUIDE.md`'s "Logging is almost always necessary when the assertion is not `assert_eq!`" rule: `assert!` prints the message, not the value, so a CI failure only tells you "this should be true" — not what was actually observed at the path. Bind each evaluated `is_symlink_or_junction(...)` / `.exists()` to a local, `eprintln!` it alongside the path, then assert. Spotted by CodeRabbit on #12024.
…pe comparison `PathBuf::eq` is a byte-for-byte comparison, so a non-root `link:../shared` in `packages/sibling` resolved to `<ws>/packages/sibling/../shared` would not match a root `link:packages/shared` resolved to `<ws>/packages/shared` — equivalent on disk but lexically distinct. Pnpm's `linkDirectDepsAndDedupe` runs `path.relative` on stored symlink targets, and Node normalises both arguments through `path.resolve` before comparing — so pnpm dedupes these. Run the joined link target through `pacquet_fs::lexical_normalize` to bring pacquet to parity. Adds `dedupes_link_deps_resolving_to_the_same_dir_via_different_segments` exercising the case and verified to fail without the normalization. Reported by CodeRabbit on #12024. --- Written by an agent (Claude Code, claude-opus-4-7).
Per `CODE_STYLE_GUIDE.md`'s "Logging is almost always necessary when the assertion is not `assert_eq!`" rule: `assert!` prints the message, not the value, so a CI failure only tells you "this should be true" — not what was actually observed at the path. Bind each evaluated `is_symlink_or_junction(...)` / `.exists()` to a local, `eprintln!` it alongside the path, then assert. Spotted by CodeRabbit on #12024.
cc98feb to
667c53f
Compare
Mirrors pnpm's `linkDirectDepsAndDedupe` at `installing/linking/direct-dep-linker/src/linkDirectDeps.ts:34`: when the workspace root resolves an alias to the same target as a non-root project, the non-root project skips the symlink under its `node_modules/`. A project whose direct deps are entirely covered by root ends up without a `node_modules/` directory. The dedupe runs inside `SymlinkDirectDependencies::run`: resolve every root entry's target dir, then drop any non-root entry whose alias maps to the same target. Bin linking follows because deduped entries are absent from the `dep_names` passed to `link_direct_dep_bins`. The `link_only` (hoisted) path participates too, deduping `link:` workspace siblings between root and projects. Also surfaces `dedupeDirectDeps` in `WorkspaceStateSettings` so a flag flip drives the optimistic-repeat-install fast path to reinstall. Refs #12009. --- Written by an agent (Claude Code, claude-opus-4-7).
…isted root deps Closes the publicly-hoisted half of the dedupe gap pacquet/pnpm#12009 documents. Pnpm's `linkDirectDepsAndDedupe` reads root's `node_modules/` after the hoist pass populates it, so its dedupe naturally covers both direct deps and publicly-hoisted aliases; pacquet runs `SymlinkDirectDependencies` *before* the on-disk hoist phase, so the dedupe map only saw root's direct deps. Fix: pre-compute the hoist plan once (pure BFS, no I/O) before `SymlinkDirectDependencies` and feed the publicly-hoisted alias → target-path map into the dedupe pass via a new `public_hoist_targets` field. The on-disk hoist phase reuses the same `HoistResult` instead of re-running the BFS. This makes `dedupes_direct_dep_against_publicly_hoisted_root_dep` a real test mirroring `installing/deps-installer/test/install/dedupeDirectDeps.ts:113`. The remaining `known_failures` stub (`dedupe_under_shamefully_hoist`) now blocks on a separate, broader gap: pacquet's *fresh-install* path doesn't run hoist at all yet. The upstream test does a single fresh `pnpm install`, so even with the dedupe-vs-hoist fix landed, pacquet wouldn't hoist on that path. --- Written by an agent (Claude Code, claude-opus-4-7).
Pacquet's fresh-install path previously ran no hoist phase, so a fresh `pacquet install` (no lockfile, no `--frozen-lockfile`) never produced `<vs>/node_modules/<alias>` (private hoist) or `<root>/node_modules/<alias>` (public hoist) entries. Pnpm's `linkPackages` runs hoist in both fresh and frozen paths, so this was a parity gap on the install surface and blocked the dedupe-direct-deps + shamefully-hoist case from working on a fresh install. Fix: lift the hoist plan computation + on-disk hoist phase into a pair of `pub(crate)` helpers on `install_frozen_lockfile.rs` (`compute_hoist_plan`, `collect_public_hoist_targets`, `HoistPlan`) and call them from `install_with_fresh_lockfile.rs` after `CreateVirtualStore`. The `HoistResult` is reused for the public-hoist alias map that feeds the dedupe pass inside `SymlinkDirectDependencies`, then again for `symlink_hoisted_dependencies` + private-side bin linking. Mirrors the frozen-install ordering exactly. The fresh-install `InstallWithFreshLockfileResult.hoisted_dependencies` slot now carries the real hoist map (was always empty before). This converts the last known_failures stub (`dedupe_under_shamefully_hoist`) into a real passing test, mirroring `pnpm/test/install/hoist.ts:77`. Two snapshot tests update to add the expected `node_modules/.pnpm/node_modules/<scope>/<alias>` private-hoist entries (matching pnpm's default `hoistPattern: ["*"]`). --- Written by an agent (Claude Code, claude-opus-4-7).
…pe comparison `PathBuf::eq` is a byte-for-byte comparison, so a non-root `link:../shared` in `packages/sibling` resolved to `<ws>/packages/sibling/../shared` would not match a root `link:packages/shared` resolved to `<ws>/packages/shared` — equivalent on disk but lexically distinct. Pnpm's `linkDirectDepsAndDedupe` runs `path.relative` on stored symlink targets, and Node normalises both arguments through `path.resolve` before comparing — so pnpm dedupes these. Run the joined link target through `pacquet_fs::lexical_normalize` to bring pacquet to parity. Adds `dedupes_link_deps_resolving_to_the_same_dir_via_different_segments` exercising the case and verified to fail without the normalization. Reported by CodeRabbit on #12024. --- Written by an agent (Claude Code, claude-opus-4-7).
The two snapshot tests under `pacquet/crates/cli/tests/add.rs` (`should_install_all_dependencies` + `should_symlink_correctly`) weren't updated when `aee5abc39e` taught fresh install to run hoist. They now show the same `node_modules/.pnpm/node_modules/<scope>/<alias>` private-hoist entries that the install-test snapshots already recorded — matching pnpm's default `hoistPattern: ["*"]`. --- Written by an agent (Claude Code, claude-opus-4-7).
- Doc: `[`HoistResult`]` intra-doc link in `install_frozen_lockfile.rs` is defined in `hoist.rs` and not in this module's scope. Qualify it as `[`crate::HoistResult`]` so `-D rustdoc::broken-intra-doc-links` is satisfied. - Dylint perfectionist: `single-letter-closure-param` flagged the `|t|` in the dedupe filter at `symlink_direct_dependencies.rs:411`. Rename to `|root_target|`. Neither lint is run by `just ready`, so both surfaced only in the pacquet/pnpm CI Doc + Dylint jobs.
Per `CODE_STYLE_GUIDE.md`'s "Logging is almost always necessary when the assertion is not `assert_eq!`" rule: `assert!` prints the message, not the value, so a CI failure only tells you "this should be true" — not what was actually observed at the path. Bind each evaluated `is_symlink_or_junction(...)` / `.exists()` to a local, `eprintln!` it alongside the path, then assert. Spotted by CodeRabbit on #12024.
Two Dylint perfectionist findings in the logging-additions commit: - `assert!` at `dedupe_direct_deps.rs:138` is multi-line and was missing the trailing comma after the message arg (`macro-trailing-comma`). - `eprintln!` at `dedupe_direct_deps.rs:214` is single-line and had a stray trailing comma cargo fmt left in place after I moved the argument onto its own line earlier (same lint, inverse rule). Neither shape is caught by `just ready`; only the Dylint job surfaces them.
…vate hoist Pacquet's fresh-install path now runs the hoist phase (`feat(pacquet/package-manager): run hoist on the fresh-install path`), so default `hoistPattern: ["*"]` materialises a `<vs>/.pnpm/node_modules/` private-hoist target with one symlink per hoisted alias. Empirically verified against `pnpm@11.4.0` with the equivalent fixture (`injectWorkspacePackages: true`, `autoInstallPeers: false`, `enableGlobalVirtualStore: false`) — pnpm produces the same shape: slot dirs + `lock.yaml` + `node_modules/`. The pacquet assertion of 8 entries mirrored upstream's stale `injectLocalPackages.ts:119` `toHaveLength(8)`, which predates the private-hoist dir; the ported test's "matches upstream's count exactly" comment becomes literally true again once both sides agree on 9.
8a62f03 to
15cbc93
Compare
Ports pnpm's
dedupeDirectDepssetting to pacquet end-to-end — one of the items tracked in #12009.When the workspace root resolves an alias to the same target as a non-root project, the non-root project skips the symlink under its own
node_modules/. A project whose direct deps are entirely covered by root ends up without anode_modules/at all. Matches pnpm'slinkDirectDepsAndDedupein all three regimes (direct-vs-direct, direct-vs-public-hoist, direct-vs-shamefully-hoist), on both the fresh-install and frozen-lockfile paths.Default
true, matching pnpm. SetdedupeDirectDeps: falseinpnpm-workspace.yamlto opt out.What changed
Config: new
dedupe_direct_deps: boolfield onConfig(defaulttrue), wired throughpnpm-workspace.yaml, theDEDUPE_DIRECT_DEPSenv override, andclear_workspace_only_fields.Linker:
SymlinkDirectDependencies::runcomputes the root importer's resolved targets up front and filters non-root importers against that map. Pulled out helperscollect_resolved_entries/resolve_target_path/collect_resolved_targetsso the dedupe pass and the symlink pass share one target-path computation. Thelink_only(hoisted) path participates too —link:workspace siblings dedupe consistently. Link-target paths are run throughpacquet_fs::lexical_normalizebefore equality comparison so<workspace>/packages/aand<workspace>/packages/sibling/../acorrectly dedupe (matching pnpm, whosepath.relativenormalizes viapath.resolve).Hoist + dedupe interaction: pacquet's pipeline runs
SymlinkDirectDependenciesbefore the on-disk hoist phase, so the dedupe map would have missed publicly-hoisted root entries that pnpm sees because hoist runs first there. Lifted the hoist plan computation intopub(crate)helpers (compute_hoist_plan,collect_public_hoist_targets,HoistPlan) — pure BFS, no I/O — and pass the publicly-hoisted alias → target-path map into the dedupe pass via a newpublic_hoist_targetsfield. The on-disk hoist phase reuses the sameHoistResultinstead of re-running the BFS.Fresh-install hoist: pacquet's fresh-install path previously didn't run hoist at all, so
pacquet install(no--frozen-lockfile) never produced<vs>/node_modules/<alias>(private hoist) or<root>/node_modules/<alias>(public hoist) entries — a parity gap with pnpm independent of dedupe. The same helpers are now called frominstall_with_fresh_lockfile.rsafterCreateVirtualStore. The fresh-install result'shoisted_dependenciesslot now carries the real map (was always empty before).Workspace state:
current_settingswritesdedupeDirectDeps,settings_matchparticipates in the comparison. Flipping the flag trips the optimistic-repeat-install fast-path gate.Tests
pacquet/crates/cli/tests/dedupe_direct_deps.rs(new) — 7 integration tests against the real binary + mocked registry:dedupes_direct_deps_against_workspace_root_by_default— default-on dedupes against root (sibling has nonode_modules)dedupe_direct_deps_disabled_keeps_per_project_symlinks—dedupeDirectDeps: falsekeeps per-project symlinksdedupes_only_overlapping_direct_deps— partial dedupe (one shared, one unique)dedupes_direct_deps_with_frozen_lockfile— frozen-lockfile install dedupes toodedupes_link_deps_resolving_to_the_same_dir_via_different_segments—link:packages/avslink:../adedupes via lexical normalizationdedupes_direct_dep_against_publicly_hoisted_root_dep— mirrorsinstalling/deps-installer/test/install/dedupeDirectDeps.ts:113, exercises dedupe-vs-public-hoist via the frozen pathdedupe_under_shamefully_hoist— mirrorspnpm/test/install/hoist.ts:77, exercises dedupe + shamefully-hoist on the fresh-install pathPlus:
returns_skipped_when_dedupe_direct_deps_drifts— drift test on the optimistic-repeat-install settings gate.add__should_install_all_dependencies,add__should_symlink_correctly,install__should_install_dependencies) updated to include the newnode_modules/.pnpm/node_modules/<scope>/<alias>private-hoist entries that fresh install now produces (matches pnpm's defaulthoistPattern: ["*"]).All upstream tests referencing
dedupeDirectDepsare ported.Test plan
cargo nextest run -p pacquet-cli --test dedupe_direct_deps(7/7 passing)cargo nextest run -p pacquet-package-manager(344/344 passing)cargo nextest run -p pacquet-cli --test workspace_install --test hoist --test install --test addpassingcargo nextest run -p pacquet-workspace-state -p pacquet-configpassingcargo clippy --locked --workspace --all-targets -- --deny warningscleanRUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspacecleandedupes_direct_dep_against_publicly_hoisted_root_dep; temporarily skipping the fresh-install hoist call failsdedupe_under_shamefully_hoist; removing the link-target normalization failsdedupes_link_deps_resolving_to_the_same_dir_via_different_segments.Refs #12009.
Written by an agent (Claude Code, claude-opus-4-7).