fix(pacquet): dedupeDirectDeps default + per-importer workspace link resolution, with defaults contract test#12128
Conversation
pacquet defaulted `dedupe_direct_deps` to `true`, but pnpm's
config-reader default is `false` (config/reader/src/index.ts:139,
`'dedupe-direct-deps': false`). With the wrong default, a direct
dependency that both the workspace root and a non-root importer
declare gets dropped from the non-root importer's `node_modules/`.
This broke the v11.5.1 release (.github#214). The release installs
the monorepo through pacquet's frozen-lockfile path, then runs
`pnpm publish` on `@pnpm/exe`, which rewrites its `workspace:*`
devDependency on `@pnpm/jest-config` by reading
`exe/node_modules/@pnpm/jest-config/package.json`. The workspace
root also depends on `@pnpm/jest-config`, so pacquet's erroneous
dedupe removed the per-importer symlink and publish failed with
`ERR_PNPM_CANNOT_RESOLVE_WORKSPACE_PROTOCOL`.
The dedupe-on integration tests already in this file relied on the
old default; they now set `dedupeDirectDeps: true` explicitly,
mirroring upstream's `testDefaults({ ..., dedupeDirectDeps: true })`.
A new test pins the default-off behavior against a pnpm-written
frozen lockfile, reproducing the release scenario.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2026-05-20T19:40:55.051ZApplied to files:
📚 Learning: 2026-05-22T00:08:44.646ZApplied to files:
📚 Learning: 2026-05-20T23:07:58.444ZApplied to files:
🔇 Additional comments (8)
📝 WalkthroughWalkthroughChange default for dedupe_direct_deps to false, partition resolver cache entries by importer for project-relative specs, and update/add workspace tests to assert importer-local link behavior and a frozen-lockfile regression. ChangesDedupe Direct Deps Default & Workspace Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate 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 |
Review Summary by QodoFix dedupe_direct_deps default to false, matching pnpm config
WalkthroughsDescription• Changed dedupe_direct_deps default from true to false to match pnpm • Fixes v11.5.1 release failure where shared workspace deps were incorrectly deduplicated • Updated existing tests to explicitly set dedupeDirectDeps: true • Added regression test for default-off behavior with frozen lockfile Diagramflowchart LR
A["pacquet config default"] -->|changed from true| B["dedupe_direct_deps = false"]
B -->|matches| C["pnpm config-reader default"]
B -->|prevents| D["incorrect dedup of shared workspace deps"]
D -->|fixes| E["v11.5.1 release failure"]
File Changes1. pacquet/crates/config/src/lib.rs
|
There was a problem hiding this comment.
Pull request overview
Aligns pacquet’s dedupeDirectDeps default with pnpm’s config-reader default (false) to prevent missing per-importer workspace symlinks during frozen installs (notably impacting the release workflow that runs pnpm publish after a pacquet frozen install).
Changes:
- Change pacquet
Config::dedupe_direct_depsdefault fromtruetofalseand update the inline documentation to reference pnpm’s default. - Update existing dedupe-on integration tests to explicitly set
dedupeDirectDeps: trueinpnpm-workspace.yaml. - Add a regression test asserting that the default-off behavior preserves a non-root importer’s workspace symlink when the root shares the same workspace dependency (frozen-lockfile replay).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pacquet/crates/config/src/lib.rs | Changes dedupe_direct_deps default to false and updates doc links to pnpm’s default/call site. |
| pacquet/crates/cli/tests/dedupe_direct_deps.rs | Makes dedupe-on tests set dedupeDirectDeps: true explicitly and adds a regression test for default-off behavior under frozen install. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Micro-Benchmark ResultsLinux |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/cli/tests/dedupe_direct_deps.rs (1)
231-350: 💤 Low valueConsider adding a log before the second assertion.
The test correctly validates the default-off behavior and reproduces the release scenario. However, the assertion at line 343 lacks a preceding log statement. While the variable is derived from the already-logged
app_link, adding an explicit log would improve debuggability.📋 Suggested logging addition
); + eprintln!("app_manifest={app_manifest:?}"); assert!( app_manifest.exists(),🤖 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 231 - 350, The second assertion that checks app_manifest.exists() lacks a preceding log; add a short diagnostic print (e.g. eprintln!) just before the assertion to show the app_manifest path and whether it exists (use the existing app_manifest and app_link variables) so test failures include that context; update the test function dedupe_off_by_default_keeps_shared_workspace_link to emit this log immediately before the assert!(app_manifest.exists(), ...) line.
🤖 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 231-350: The second assertion that checks app_manifest.exists()
lacks a preceding log; add a short diagnostic print (e.g. eprintln!) just before
the assertion to show the app_manifest path and whether it exists (use the
existing app_manifest and app_link variables) so test failures include that
context; update the test function
dedupe_off_by_default_keeps_shared_workspace_link to emit this log immediately
before the assert!(app_manifest.exists(), ...) line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 11994c51-ba3f-4bf2-964d-d70553a546aa
📒 Files selected for processing (2)
pacquet/crates/cli/tests/dedupe_direct_deps.rspacquet/crates/config/src/lib.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). (11)
- GitHub Check: Agent
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Doc
- GitHub Check: Dylint
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-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: Log emissions are part of matching pnpm — when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/config/src/lib.rspacquet/crates/cli/tests/dedupe_direct_deps.rs
pacquet/**/tests/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/tests/**/*.rs: Prefer real fixtures over dependency-injection seams — usetempfile::TempDir, the mocked registry, or integration tests spawning the actual binary for happy paths and most error paths; use the DI seam only for filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths
Follow test-logging guidance — log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found — let the test panic when a required tool is absent
Prefer#[cfg_attr(target_os = "windows", ignore = "...")](or matching#[cfg(unix)]gates) over runtime probe-and-skip helpers for platform-locked tools
Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm
Use snapshot tests withinstaand carefully review diffs when intentional changes alter snapshots; accept withcargo insta reviewonly after careful review
Tests that need the mocked registry should startpnprthroughpacquet-testing-utils;cargo test/cargo nextest runshould not require a separatejust registry-mock launchstep
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/lib.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/lib.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/lib.rspacquet/crates/cli/tests/dedupe_direct_deps.rs
🔇 Additional comments (5)
pacquet/crates/config/src/lib.rs (1)
728-733: LGTM!pacquet/crates/cli/tests/dedupe_direct_deps.rs (4)
1-18: LGTM!
30-34: LGTM!Also applies to: 57-57
184-184: LGTM!
394-394: LGTM!Also applies to: 462-462, 543-543, 567-567, 638-638
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12128 +/- ##
==========================================
+ Coverage 87.45% 87.50% +0.05%
==========================================
Files 260 261 +1
Lines 29376 29582 +206
==========================================
+ Hits 25690 25886 +196
- Misses 3686 3696 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
A non-injected workspace dependency (`workspace:*`, `link:<path>`) resolves to a `link:<path>` whose path is computed relative to the *consuming importer's* directory. pacquet's workspace-wide `resolved_by_wanted` cache keyed only `(alias, specifier, optional, injected, pick_lowest, published_by)`, so the first importer to resolve a shared workspace dep seeded the cache with its own relative path and every other importer reused it verbatim. With a root and a `packages/app` both depending on `@scope/lib`, the root resolved `link:packages/lib` and `packages/app` got that same string back — a dangling `packages/app/packages/lib`. pnpm writes `link:../lib` for `packages/app`. Add the consuming importer's `project_dir` to the cache key for project-relative specifiers (`link:` / `file:` / `workspace:`) only; registry specifiers stay importer-independent and keep sharing one cache slot across importers, preserving the cross-importer dedup.
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.05597792568,
"stddev": 0.05966570853653516,
"median": 2.04471446928,
"user": 2.5805680399999997,
"system": 3.2996187399999997,
"min": 1.96576046128,
"max": 2.13956809528,
"times": [
2.0103994482800003,
2.13956809528,
2.06192212028,
1.96576046128,
2.02750681828,
2.01174244628,
2.11933733728,
2.13664733428,
2.06791557928,
2.0189796162800002
]
},
{
"command": "pacquet@main",
"mean": 2.06366964378,
"stddev": 0.11101341751138684,
"median": 2.0316759062800003,
"user": 2.59176954,
"system": 3.3007187399999998,
"min": 1.97002900628,
"max": 2.3193753362800003,
"times": [
1.97666972128,
2.13483024628,
1.9719016992799998,
2.07003251928,
2.0823270002800003,
1.9872439472799999,
2.1309676682800003,
1.9933192932800001,
1.97002900628,
2.3193753362800003
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.63893473266,
"stddev": 0.023438081067033347,
"median": 0.6314090986600001,
"user": 0.35042303999999996,
"system": 1.2992657199999997,
"min": 0.62601674716,
"max": 0.70420367216,
"times": [
0.70420367216,
0.63065367616,
0.6290457431600001,
0.64357521716,
0.62601674716,
0.63385914216,
0.62681646216,
0.63168314516,
0.63113505216,
0.63235846916
]
},
{
"command": "pacquet@main",
"mean": 0.6404123232600001,
"stddev": 0.020000701237503235,
"median": 0.63657513516,
"user": 0.33946174,
"system": 1.3147367199999997,
"min": 0.61823322716,
"max": 0.6929929031600001,
"times": [
0.6929929031600001,
0.63298222016,
0.64069221816,
0.6273808481600001,
0.63845698616,
0.63469328416,
0.63264679516,
0.64502982716,
0.61823322716,
0.64101492316
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.2936714719599998,
"stddev": 0.058672964949918145,
"median": 2.28108554856,
"user": 3.7257128,
"system": 3.07950932,
"min": 2.2345697965599998,
"max": 2.45240063656,
"times": [
2.30062643156,
2.45240063656,
2.27400651656,
2.28154528856,
2.29076333056,
2.28062580856,
2.26486780256,
2.2345697965599998,
2.26740164956,
2.28990745856
]
},
{
"command": "pacquet@main",
"mean": 2.2824490902600005,
"stddev": 0.023521177978152574,
"median": 2.28341052306,
"user": 3.7434642000000005,
"system": 3.0843857200000007,
"min": 2.23305065656,
"max": 2.32767273956,
"times": [
2.29026134656,
2.28255028856,
2.29097568756,
2.23305065656,
2.26402820056,
2.28415969556,
2.32767273956,
2.2824647965600002,
2.28266135056,
2.28666614056
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.4594998276200002,
"stddev": 0.017689562748462864,
"median": 1.4672491924200002,
"user": 1.65091514,
"system": 1.8192971400000002,
"min": 1.43221257242,
"max": 1.4765537804200002,
"times": [
1.46720033842,
1.46729804642,
1.43672809242,
1.46977090542,
1.4765537804200002,
1.4764674474200001,
1.4564840404200001,
1.4750775444200002,
1.43221257242,
1.4372055084200002
]
},
{
"command": "pacquet@main",
"mean": 1.4664844893200002,
"stddev": 0.017031018165125325,
"median": 1.4705056329200001,
"user": 1.6594252399999998,
"system": 1.80653854,
"min": 1.4398853254200001,
"max": 1.49383030942,
"times": [
1.4499624934200002,
1.4705447964200002,
1.4475658904200002,
1.46136526842,
1.49383030942,
1.48635534842,
1.47265382142,
1.4398853254200001,
1.47046646942,
1.4722151704200002
]
}
]
} |
|
Bencher Report| Branch | pr/12128 |
| 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,293.67 ms(-1.94%)Baseline: 2,339.10 ms | 2,806.92 ms (81.71%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,459.50 ms(-3.34%)Baseline: 1,509.91 ms | 1,811.89 ms (80.55%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,055.98 ms(-0.57%)Baseline: 2,067.80 ms | 2,481.35 ms (82.86%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 638.93 ms(-2.88%)Baseline: 657.90 ms | 789.48 ms (80.93%) |
A contract test reads pnpm's `defaultOptions` literal from config/reader/src/index.ts at test time and asserts every pacquet default that maps to a pnpm setting equals pnpm's value. Drift on either side fails here — the `dedupeDirectDeps` default that broke the v11.5.1 release would have been caught. `every_pnpm_default_is_classified` partitions all 80 pnpm settings into mapped / non-literal (env, platform, CPU) / not-ported, so a new pnpm setting that's neither mapped nor skipped fails the test until someone classifies it.
What
Two pacquet fixes for workspace
link:handling, both surfaced by the v11.5.1 release failure, plus a contract test that prevents the class of bug from recurring.1.
dedupeDirectDepsdefaulttrue→false(the release cause)pnpm's config-reader default is
false(config/reader/src/index.ts:139,'dedupe-direct-deps': false); pacquet defaulted totrue.The v11.5.1 release run failed at Publish @pnpm/exe with
ERR_PNPM_CANNOT_RESOLVE_WORKSPACE_PROTOCOLfor@pnpm/jest-config:@pnpm/pacquetconfigDependency (0.2.8→0.2.2-15, a newer build despite the lower semver), engaging pacquet as the install engine for the first time (v11.5.0's release log mentions pacquet zero times).pnpm publish @pnpm/exerewrites itsworkspace:*devDependency on@pnpm/jest-configby readingexe/node_modules/@pnpm/jest-config/package.json.@pnpm/jest-config, so the erroneous dedupe removed the per-importer symlink and publish aborted.Verified on the repo: a pacquet
0.2.2-15frozen install leavespnpm/artifacts/exe/node_modules/@pnpm/jest-configmissing; the JS engine keeps it.2. Per-importer workspace
link:resolutionA non-injected workspace dep resolves to a
link:<path>computed relative to the consuming importer's directory. pacquet's workspace-wideresolved_by_wantedcache omittedproject_dirfrom its key, so the first importer to resolve a shared workspace dep seeded the cache with its own relative path and every other importer reused it verbatim — a root resolvinglink:packages/libhandedpackages/appthe same string (danglingpackages/app/packages/lib; pnpm writeslink:../lib). The fix addsproject_dirto the key only for project-relative specifiers (link:/file:/workspace:); registry specifiers stay importer-independent and keep sharing one cache slot.3. Contract test: pacquet defaults == pnpm CLI defaults
crates/config/src/pnpm_default_parity.rsreads pnpm'sdefaultOptionsliteral fromconfig/reader/src/index.tsat test time and asserts every pacquet default that maps to a pnpm setting equals pnpm's value — thededupeDirectDepsdivergence would have failed here instead of in the release pipeline. A second test partitions all 80 pnpm settings into mapped / non-literal (env·platform·CPU) / not-ported, so a new pnpm setting that's neither mapped nor skipped fails the test until someone classifies it.Tests
dedupeDirectDeps: trueexplicitly (mirroring upstream'stestDefaults({ ..., dedupeDirectDeps: true })).dedupe_off_by_default_keeps_shared_workspace_linkpins the default-off behavior against a pnpm-written frozen lockfile (reproduces the release scenario).shared_workspace_dep_link_is_relative_to_each_importerpins per-importerlink:paths on a fresh resolve.pacquet_defaults_match_pnpm_cli_defaults+every_pnpm_default_is_classified— the contract tests above.Each new behavioral test was confirmed to fail before its fix.
just-level checks run: fmt, clippy--deny warnings,cargo doc -D warnings, dylint (pre-push), typos, and the resolver + config + workspace/dedupe/lockfile-reuse suites.Note for the release
The configDependency
@pnpm/pacquet@0.2.2-15is the published binary; these source fixes only take effect once a new pacquet build is published andconfigDependencies['@pnpm/pacquet']inpnpm-workspace.yamlis bumped to it.Written by an agent (Claude Code, claude-opus-4-8).
Summary by CodeRabbit
Bug Fixes
Documentation
Tests