feat(pacquet): port dedupeInjectedDeps#12023
Merged
Merged
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 dedupeInjectedDeps: adds config/env/YAML plumbing, PackageManifest helper, ImporterDepVersion::File, workspace-scoped resolve_workspace flow with injected-flag threading, dedupe_injected_deps pass that rewrites eligible ChangesImplement dedupeInjectedDeps
Sequence Diagram(s)sequenceDiagram
participant Installer as package-manager::install_with_fresh_lockfile
participant WorkspaceResolver as resolving-deps-resolver::resolve_workspace
participant ImporterResolver as resolve_importer_with_workspace
participant PeerResolver as resolve_peers_workspace
participant Dedupe as dedupe_injected_deps
participant LockfileWriter as dependencies_graph_to_lockfile
Installer->>WorkspaceResolver: call resolve_workspace(importers, opts, per_importer_options)
WorkspaceResolver->>ImporterResolver: run per-importer resolution (shared WorkspaceTreeCtx)
WorkspaceResolver->>PeerResolver: merge workspace tree and call resolve_peers_workspace(...)
PeerResolver->>Dedupe: optionally run dedupe_injected_deps(graph, direct_by_importer, importer_root_dirs, lockfile_dir)
PeerResolver->>LockfileWriter: produce lockfile importer depPaths (File or rewritten link:)
LockfileWriter->>Installer: return lockfile data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 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 |
Contributor
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.12090490948,
"stddev": 0.17179822834877997,
"median": 2.09519419638,
"user": 2.74161436,
"system": 3.2873264199999994,
"min": 1.9413638423800001,
"max": 2.4755756463800003,
"times": [
2.14542111738,
2.12160305538,
2.30844873638,
1.9413638423800001,
1.97187682538,
2.06878533738,
2.21185216838,
2.4755756463800003,
1.9883362023800002,
1.97578616338
]
},
{
"command": "pacquet@main",
"mean": 2.08963353638,
"stddev": 0.06540091634032529,
"median": 2.07436245288,
"user": 2.7980586599999997,
"system": 3.3105452200000003,
"min": 2.01188498938,
"max": 2.22949945338,
"times": [
2.22949945338,
2.04680816838,
2.07673409638,
2.03148700738,
2.1031485023800003,
2.05568673738,
2.10433279138,
2.16476280838,
2.07199080938,
2.01188498938
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6778354880399998,
"stddev": 0.018359677748251504,
"median": 0.67789392274,
"user": 0.36221654000000003,
"system": 1.36360974,
"min": 0.65435676574,
"max": 0.70988018674,
"times": [
0.67454270574,
0.70050840274,
0.68643746774,
0.68473697574,
0.65714641174,
0.65435676574,
0.66107912474,
0.66842169974,
0.68124513974,
0.70988018674
]
},
{
"command": "pacquet@main",
"mean": 0.69971063754,
"stddev": 0.030298362768841416,
"median": 0.6969548517399999,
"user": 0.36151894,
"system": 1.32736594,
"min": 0.65631473574,
"max": 0.74672345974,
"times": [
0.70754078774,
0.71632213174,
0.69657360474,
0.74672345974,
0.67440141174,
0.66298566774,
0.74233487274,
0.69719868874,
0.65631473574,
0.69671101474
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.3181172194200004,
"stddev": 0.037095559564149075,
"median": 2.32794184662,
"user": 3.8608412999999993,
"system": 2.94582272,
"min": 2.2495135396199997,
"max": 2.37302883062,
"times": [
2.3579100936199997,
2.29077892362,
2.37302883062,
2.3328632036199997,
2.2932741656199997,
2.2887653656199998,
2.3283651156199996,
2.2495135396199997,
2.32751857762,
2.33915437862
]
},
{
"command": "pacquet@main",
"mean": 2.3078489335200003,
"stddev": 0.03310909953959196,
"median": 2.30432434962,
"user": 3.8281240999999993,
"system": 3.0164404199999995,
"min": 2.25207214762,
"max": 2.36120587762,
"times": [
2.29769300662,
2.32451372862,
2.2857468436199997,
2.27474774362,
2.30067550862,
2.32693353762,
2.34692775062,
2.36120587762,
2.30797319062,
2.25207214762
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.5286036319000003,
"stddev": 0.04193151857334628,
"median": 1.5179294459000001,
"user": 1.7537511399999999,
"system": 1.7869878999999997,
"min": 1.4907931079,
"max": 1.6302616549000002,
"times": [
1.4941163459,
1.4907931079,
1.4949840329000001,
1.5111479259000002,
1.5151037669,
1.5519430309000002,
1.5234184629000003,
1.6302616549000002,
1.5535128659000002,
1.5207551249000002
]
},
{
"command": "pacquet@main",
"mean": 1.5215798192000003,
"stddev": 0.03841507815046785,
"median": 1.5146542554,
"user": 1.6890723400000003,
"system": 1.8176005000000004,
"min": 1.4693105669000002,
"max": 1.5897720809000002,
"times": [
1.5396605719,
1.4973107579,
1.5897720809000002,
1.5120840049000002,
1.5504745849000001,
1.5604007269,
1.4738152009,
1.5057451909000001,
1.4693105669000002,
1.5172245059
]
}
]
} |
Contributor
|
Bencher Report| Branch | pr/12023 |
| 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,318.12 ms(+0.75%)Baseline: 2,300.81 ms | 2,760.97 ms (83.96%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,528.60 ms(+6.47%)Baseline: 1,435.65 ms | 1,722.78 ms (88.73%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,120.90 ms(+3.09%)Baseline: 2,057.37 ms | 2,468.85 ms (85.91%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 677.84 ms(+0.07%)Baseline: 677.34 ms | 812.81 ms (83.39%) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12023 +/- ##
==========================================
+ Coverage 88.59% 88.66% +0.07%
==========================================
Files 231 233 +2
Lines 29663 30051 +388
==========================================
+ Hits 26279 26646 +367
- Misses 3384 3405 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Review Summary by QodoPort dedupeInjectedDeps feature with multi-importer resolver refactor
WalkthroughsDescription• Port pnpm's dedupeInjectedDeps feature end-to-end with config plumbing, workspace state tracking, and cross-importer dedupe pass • Thread dependenciesMeta.injected flag through resolver to enable workspace-pick and local-resolver branches for file: resolutions • Refactor resolver to multi-importer architecture with shared peer caches and single workspace-wide dedupe pass • Add end-to-end test verifying injected leaf workspace deps collapse to symlinks after dedupe Diagramflowchart LR
A["Config plumbing<br/>dedupe_injected_deps: bool"] --> B["Manifest reads<br/>dependenciesMeta.injected"]
B --> C["resolve_workspace<br/>multi-importer orchestrator"]
C --> D["Per-importer<br/>resolve_importer"]
D --> E["Merged tree<br/>+ shared caches"]
E --> F["resolve_peers_workspace<br/>cross-importer pass"]
F --> G["dedupe_injected_deps<br/>file: to link: rewrite"]
G --> H["Prune orphaned<br/>snapshots"]
H --> I["pnpm-lock.yaml<br/>with link: deps"]
File Changes1. pacquet/crates/cli/tests/dedupe_injected_deps.rs
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pacquet/crates/package-manager/src/symlink_direct_dependencies.rs (1)
460-472: 💤 Low valueConsider defensive handling for malformed
file:payloads.The
expect()on line 470 will panic ifresolved_key()returnsNone. While the invariant "File arm always produces a resolved_key" holds for resolver-produced data, a malformed lockfile could cause a panic here. Theresolved_key()implementation forFileparses"file:{payload}"asPkgVerPeer, which can fail on malformed input.This matches the existing pattern (line 284 in
dependencies_graph_to_lockfile.rshas a similar expectation), so it's consistent with the codebase. If you want to harden against malformed lockfiles, you could skip the entry with a warning instead of panicking — but that's a broader hardening effort beyond this PR's scope.🤖 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/symlink_direct_dependencies.rs` around lines 460 - 472, The code currently calls spec.version.resolved_key(name).expect(...) inside the ImporterDepVersion::File match arm which will panic on malformed `file:` payloads; change this to defensively handle a None by logging a warning and skipping this entry instead of panicking: call spec.version.resolved_key(name) and if it returns Some(dep_key) continue to use layout.slot_dir(&dep_key).join("node_modules").join(&name_str), otherwise emit a warning (via the existing logger) that the `file:` payload for the importer dep is malformed and skip building that path for this entry so execution continues safely.pacquet/crates/cli/tests/dedupe_injected_deps.rs (2)
31-64: ⚡ Quick winExtract common fixture setup to reduce duplication.
Both tests contain nearly identical workspace setup code (36 lines each). The only difference is the
pnpm-workspace.yamlcontent. Consider extracting a helper function that accepts the workspace YAML configuration to avoid repeating this fixture logic.♻️ Suggested refactor pattern
/// Helper to set up a two-package workspace where `a` injects `b`. fn setup_injected_workspace( workspace_yaml_extra: &str, ) -> (CommandTempCwd, tempfile::TempDir, mockito::ServerGuard) { let CommandTempCwd { pacquet, root, workspace, npmrc_info, .. } = CommandTempCwd::init().add_mocked_registry(); let AddMockedRegistry { mock_instance, .. } = npmrc_info; fs::write( workspace.join("package.json"), serde_json::json!({ "name": "ws-root", "version": "0.0.0", "private": true }).to_string(), ) .expect("write root package.json"); let workspace_yaml_path = workspace.join("pnpm-workspace.yaml"); let mut workspace_yaml = fs::read_to_string(&workspace_yaml_path).expect("read pnpm-workspace.yaml"); if !workspace_yaml.ends_with('\n') { workspace_yaml.push('\n'); } workspace_yaml.push_str("packages:\n - 'packages/*'\n"); workspace_yaml.push_str(workspace_yaml_extra); fs::write(&workspace_yaml_path, workspace_yaml).expect("write pnpm-workspace.yaml"); // ... rest of setup (CommandTempCwd { pacquet, root, workspace, npmrc_info }, root, mock_instance) }Then call with
setup_injected_workspace("")vssetup_injected_workspace("dedupeInjectedDeps: false\n").Also applies to: 103-136
🤖 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_injected_deps.rs` around lines 31 - 64, Extract the repeated workspace fixture into a helper function (e.g., setup_injected_workspace) that performs the CommandTempCwd::init().add_mocked_registry(), writes the root package.json, patches pnpm-workspace.yaml (appending "packages:\n - 'packages/*'\n" plus a caller-provided workspace_yaml_extra), creates packages/a and packages/b and writes their package.json files (including the injected dependency metadata). Replace the duplicated blocks in the tests with calls to setup_injected_workspace("") and setup_injected_workspace("dedupeInjectedDeps: false\n") and return the necessary values used by the tests (CommandTempCwd/ pacquet, root/workspace paths, and mock instance) so existing assertions and calls remain unchanged.
138-149: ⚡ Quick winAdd filesystem assertion to match the doc comment's claim.
The doc comment at lines 88-94 states "the on-disk layout points at the virtual store slot instead of a
link:sibling," but the test only verifies the lockfile content. The first test checks both the filesystem (symlink presence) and the lockfile; this test should verify the filesystem state as well for completeness and symmetry.📂 Suggested filesystem assertion
Add this verification before the lockfile assertions:
pacquet.with_arg("install").assert().success(); +let dep = workspace.join("packages/a/node_modules/b"); +// When dedupe is off, the injected dep should NOT be a plain symlink +// to the sibling workspace package; instead it should point through +// the virtual store (or be a real directory). Verify it exists: +assert!( + dep.exists(), + "packages/a/node_modules/b should exist when dedupe is disabled", +); + let lockfile_path = workspace.join("pnpm-lock.yaml");🤖 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_injected_deps.rs` around lines 138 - 149, The test currently reads the lockfile (variables lockfile_path and lockfile) but misses the filesystem assertion mentioned in the doc comment; add a check after pacquet.with_arg("install").assert().success() that inspects the on-disk node resolution for package b (using the test's workspace handle) — e.g., resolve the node_modules path for b (the same path pattern used in the other test) and assert it is a symlink/points into the virtual store slot (contains the virtual store marker, not a link:../b sibling), then keep the existing lockfile assertions; reference the existing variables/workflow (pacquet.with_arg("install"), workspace, lockfile_path/lockfile) to locate where to insert this filesystem assertion.
🤖 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/cli/tests/dedupe_injected_deps.rs`:
- Line 11: The test file declares an unused module with `pub mod _utils;` which
is dead code; remove the `pub mod _utils;` declaration from
dedupe_injected_deps.rs (or, if the helpers are intended to be used, replace the
declaration by importing the specific helpers via `use _utils::...` and
reference them in the test). Locate the `pub mod _utils;` line in
dedupe_injected_deps.rs and delete it unless you add actual `_utils::` uses.
In `@pacquet/crates/package-manager/src/optimistic_repeat_install.rs`:
- Line 200: The comment block describing which fields are "Deliberately not
compared" is out of date: dedupeInjectedDeps (now dedupe_injected_deps) is
actually compared in the settings-match check (see the comparison that includes
recorded.dedupe_injected_deps == live.dedupe_injected_deps), so remove the line
mentioning dedupeInjectedDeps from that comment section (or delete the whole
obsolete line) so the comment matches the actual behavior of the settings-match
comparison.
---
Nitpick comments:
In `@pacquet/crates/cli/tests/dedupe_injected_deps.rs`:
- Around line 31-64: Extract the repeated workspace fixture into a helper
function (e.g., setup_injected_workspace) that performs the
CommandTempCwd::init().add_mocked_registry(), writes the root package.json,
patches pnpm-workspace.yaml (appending "packages:\n - 'packages/*'\n" plus a
caller-provided workspace_yaml_extra), creates packages/a and packages/b and
writes their package.json files (including the injected dependency metadata).
Replace the duplicated blocks in the tests with calls to
setup_injected_workspace("") and setup_injected_workspace("dedupeInjectedDeps:
false\n") and return the necessary values used by the tests (CommandTempCwd/
pacquet, root/workspace paths, and mock instance) so existing assertions and
calls remain unchanged.
- Around line 138-149: The test currently reads the lockfile (variables
lockfile_path and lockfile) but misses the filesystem assertion mentioned in the
doc comment; add a check after pacquet.with_arg("install").assert().success()
that inspects the on-disk node resolution for package b (using the test's
workspace handle) — e.g., resolve the node_modules path for b (the same path
pattern used in the other test) and assert it is a symlink/points into the
virtual store slot (contains the virtual store marker, not a link:../b sibling),
then keep the existing lockfile assertions; reference the existing
variables/workflow (pacquet.with_arg("install"), workspace,
lockfile_path/lockfile) to locate where to insert this filesystem assertion.
In `@pacquet/crates/package-manager/src/symlink_direct_dependencies.rs`:
- Around line 460-472: The code currently calls
spec.version.resolved_key(name).expect(...) inside the ImporterDepVersion::File
match arm which will panic on malformed `file:` payloads; change this to
defensively handle a None by logging a warning and skipping this entry instead
of panicking: call spec.version.resolved_key(name) and if it returns
Some(dep_key) continue to use
layout.slot_dir(&dep_key).join("node_modules").join(&name_str), otherwise emit a
warning (via the existing logger) that the `file:` payload for the importer dep
is malformed and skip building that path for this entry so execution continues
safely.
🪄 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: 6db5da7e-341e-4a3b-9793-0a1562848571
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
pacquet/crates/cli/tests/dedupe_injected_deps.rspacquet/crates/config/src/env_overlay.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/lockfile/src/resolved_dependency.rspacquet/crates/package-manager/src/dependencies_graph_to_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-manifest/src/lib.rspacquet/crates/resolving-deps-resolver/Cargo.tomlpacquet/crates/resolving-deps-resolver/src/dedupe_injected_deps.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
💤 Files with no reviewable changes (1)
- pacquet/crates/package-manager/src/optimistic_repeat_install/tests.rs
📜 Review details
🧰 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/package-manifest/src/lib.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/cli/tests/dedupe_injected_deps.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/symlink_direct_dependencies.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/dedupe_injected_deps.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/lockfile/src/resolved_dependency.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.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_injected_deps.rs
🧠 Learnings (6)
📚 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/package-manifest/src/lib.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/cli/tests/dedupe_injected_deps.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/symlink_direct_dependencies.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/dedupe_injected_deps.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/lockfile/src/resolved_dependency.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.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/package-manifest/src/lib.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/cli/tests/dedupe_injected_deps.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/symlink_direct_dependencies.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/dedupe_injected_deps.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/lockfile/src/resolved_dependency.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.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/package-manifest/src/lib.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/cli/tests/dedupe_injected_deps.rspacquet/crates/package-manager/src/optimistic_repeat_install.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/symlink_direct_dependencies.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/dedupe_injected_deps.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/lockfile/src/resolved_dependency.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: When implementing dependency “revisit”/optional-folding in pacquet, mirror pnpm’s `resolveDependencies.ts` behavior: on revisit, update the `optional` flag only for the directly visited package, and do not automatically change `optional` for transitive descendants. pnpm corrects stale optional flags later via `copyDependencySubGraph` (a BFS in `lockfile/pruner/src/index.ts`/`copyDependencySubGraph`). Until pacquet has an equivalent pruner/copy-subgraph step, be aware that the raw `node.optional` can flow through to the lockfile/virtual store via `dependencies_graph_to_lockfile.rs` → `create_virtual_store.rs` → `installability.rs`, so review changes that affect optional flag propagation for this mismatch.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/dedupe_injected_deps.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: In pacquet’s Rust resolver layer, the behavior of `ResolvedPackage.optional` “AND-folding”/revisit updates is intentionally aligned with pnpm: when a package is directly revisited, only that package’s `optional` flag is updated; transitive descendants are not re-walked/rescored in this stage. pnpm later corrects any stale optional flags downstream via its pruner/BFS logic (`copyDependencySubGraph` + `nonOptional` reachability stamping in the lockfile pruner). Since pacquet does not yet have that pruner equivalent, review should NOT flag the downstream/stale optional propagation as a bug in pacquet PRs; treat the resolver-layer optional propagation gap as expected parity until the pruning/stamping step is ported.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/dedupe_injected_deps.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.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/resolved_dependency.rs
🔇 Additional comments (34)
pacquet/crates/config/src/lib.rs (1)
608-616: LGTM!pacquet/crates/config/src/workspace_yaml.rs (3)
158-158: LGTM!
416-416: LGTM!
509-510: LGTM!pacquet/crates/config/src/env_overlay.rs (1)
146-146: LGTM!pacquet/crates/package-manager/src/optimistic_repeat_install.rs (1)
261-261: LGTM!pacquet/crates/package-manager/src/install_package_from_registry/tests.rs (1)
55-55: LGTM!pacquet/crates/package-manifest/src/lib.rs (1)
182-198: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (8)
160-172: LGTM!
189-218: LGTM!
226-265: LGTM!
267-313: LGTM!
315-411: LGTM!
426-449: LGTM!
491-494: LGTM!Also applies to: 512-516
751-771: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (4)
127-145: LGTM!
147-177: LGTM!
196-210: LGTM!
272-295: LGTM!pacquet/crates/lockfile/src/resolved_dependency.rs (1)
44-49: LGTM!Also applies to: 61-86, 98-177, 218-243, 253-315
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs (1)
248-269: LGTM!pacquet/crates/resolving-deps-resolver/Cargo.toml (1)
27-27: LGTM!pacquet/crates/resolving-deps-resolver/src/dedupe_injected_deps.rs (2)
1-171: LGTM!
173-342: LGTM!pacquet/crates/package-manager/src/symlink_direct_dependencies.rs (1)
511-524: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs (3)
117-135: LGTM!
164-234: LGTM!
459-479: LGTM!pacquet/crates/resolving-deps-resolver/src/lib.rs (1)
1-108: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs (2)
45-80: LGTM!
90-164: LGTM!pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (2)
542-611: LGTM!
23-23: LGTM!
zkochan
added a commit
that referenced
this pull request
May 28, 2026
The dedupe e2e test was copied from `workspace_install.rs` which uses `_utils`; this test doesn't reference it, so the declaration is dead weight. Flagged in CodeRabbit review on #12023. --- Written by an agent (Claude Code, claude-opus-4-7).
|
Actionable comments posted: 0 |
zkochan
added a commit
that referenced
this pull request
May 28, 2026
The dedupe e2e test was copied from `workspace_install.rs` which uses `_utils`; this test doesn't reference it, so the declaration is dead weight. Flagged in CodeRabbit review on #12023. --- Written by an agent (Claude Code, claude-opus-4-7).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (1)
559-559: 💤 Low valueRedundant Vec clone.
dependency_groupsis already aVec<DependencyGroup>(collected at line 301). The.to_vec()allocates and copies unnecessarily — pass&dependency_groupsdirectly toresolve_workspaceinstead.♻️ Suggested fix
- let dependency_groups_slice: Vec<DependencyGroup> = dependency_groups.to_vec(); let modules_basename = config .modules_dir .file_name() .map(std::ffi::OsStr::to_os_string) .unwrap_or_else(|| std::ffi::OsString::from("node_modules")); let workspace_result = pacquet_resolving_deps_resolver::resolve_workspace( &*resolver, &workspace_importers, - &dependency_groups_slice, + &dependency_groups, workspace_opts,🤖 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/install_with_fresh_lockfile.rs` at line 559, The code is unnecessarily cloning dependency_groups into dependency_groups_slice; remove the extra allocation and pass a reference instead—replace the let binding and usages so you pass &dependency_groups (or dependency_groups.as_slice()) into resolve_workspace (the call that currently uses dependency_groups_slice), keeping the original Vec<DependencyGroup> collected earlier and avoiding the redundant to_vec() copy.
🤖 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/package-manager/src/install_with_fresh_lockfile.rs`:
- Line 559: The code is unnecessarily cloning dependency_groups into
dependency_groups_slice; remove the extra allocation and pass a reference
instead—replace the let binding and usages so you pass &dependency_groups (or
dependency_groups.as_slice()) into resolve_workspace (the call that currently
uses dependency_groups_slice), keeping the original Vec<DependencyGroup>
collected earlier and avoiding the redundant to_vec() copy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 056913b5-0401-4c19-a319-1ab188ce5250
📒 Files selected for processing (18)
pacquet/crates/cli/tests/dedupe_injected_deps.rspacquet/crates/config/src/env_overlay.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/lockfile/src/resolved_dependency.rspacquet/crates/package-manager/src/dependencies_graph_to_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-manifest/src/lib.rspacquet/crates/resolving-deps-resolver/src/dedupe_injected_deps.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
💤 Files with no reviewable changes (1)
- pacquet/crates/package-manager/src/optimistic_repeat_install/tests.rs
🚧 Files skipped from review as they are similar to previous changes (15)
- pacquet/crates/config/src/env_overlay.rs
- pacquet/crates/package-manifest/src/lib.rs
- pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
- pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
- pacquet/crates/config/src/workspace_yaml.rs
- pacquet/crates/package-manager/src/optimistic_repeat_install.rs
- pacquet/crates/resolving-deps-resolver/src/dedupe_injected_deps.rs
- pacquet/crates/cli/tests/dedupe_injected_deps.rs
- pacquet/crates/config/src/lib.rs
- pacquet/crates/package-manager/src/symlink_direct_dependencies.rs
- pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
- pacquet/crates/lockfile/src/resolved_dependency.rs
- pacquet/crates/resolving-deps-resolver/src/lib.rs
- pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
📜 Review details
🧰 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/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
🧠 Learnings (5)
📚 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_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.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_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.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_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: When implementing dependency “revisit”/optional-folding in pacquet, mirror pnpm’s `resolveDependencies.ts` behavior: on revisit, update the `optional` flag only for the directly visited package, and do not automatically change `optional` for transitive descendants. pnpm corrects stale optional flags later via `copyDependencySubGraph` (a BFS in `lockfile/pruner/src/index.ts`/`copyDependencySubGraph`). Until pacquet has an equivalent pruner/copy-subgraph step, be aware that the raw `node.optional` can flow through to the lockfile/virtual store via `dependencies_graph_to_lockfile.rs` → `create_virtual_store.rs` → `installability.rs`, so review changes that affect optional flag propagation for this mismatch.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: In pacquet’s Rust resolver layer, the behavior of `ResolvedPackage.optional` “AND-folding”/revisit updates is intentionally aligned with pnpm: when a package is directly revisited, only that package’s `optional` flag is updated; transitive descendants are not re-walked/rescored in this stage. pnpm later corrects any stale optional flags downstream via its pruner/BFS logic (`copyDependencySubGraph` + `nonOptional` reachability stamping in the lockfile pruner). Since pacquet does not yet have that pruner equivalent, review should NOT flag the downstream/stale optional propagation as a bug in pacquet PRs; treat the resolver-layer optional propagation gap as expected parity until the pruning/stamping step is ported.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
🔇 Additional comments (5)
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs (1)
1-152: LGTM!pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (4)
24-25: LGTM!Also applies to: 68-70
544-551: LGTM!
565-627: LGTM!
1042-1042: LGTM!
zkochan
added a commit
that referenced
this pull request
May 28, 2026
The dedupe e2e test was copied from `workspace_install.rs` which uses `_utils`; this test doesn't reference it, so the declaration is dead weight. Flagged in CodeRabbit review on #12023. --- Written by an agent (Claude Code, claude-opus-4-7).
|
Actionable comments posted: 0 |
zkochan
added a commit
that referenced
this pull request
May 28, 2026
`dependency_groups` is already collected into a `Vec<DependencyGroup>` at the top of `InstallWithFreshLockfile::run`; the later `dependency_groups.to_vec()` was a redundant copy. Pass the existing Vec by reference to `resolve_workspace` instead. Flagged in CodeRabbit review on #12023. --- Written by an agent (Claude Code, claude-opus-4-7).
|
Actionable comments posted: 0 |
zkochan
added a commit
that referenced
this pull request
May 28, 2026
The dedupe e2e test was copied from `workspace_install.rs` which uses `_utils`; this test doesn't reference it, so the declaration is dead weight. Flagged in CodeRabbit review on #12023. --- Written by an agent (Claude Code, claude-opus-4-7).
zkochan
added a commit
that referenced
this pull request
May 28, 2026
The dedupe e2e test was copied from `workspace_install.rs` which uses `_utils`; this test doesn't reference it, so the declaration is dead weight. Flagged in CodeRabbit review on #12023. --- Written by an agent (Claude Code, claude-opus-4-7).
zkochan
added a commit
that referenced
this pull request
May 28, 2026
The dedupe e2e test was copied from `workspace_install.rs` which uses `_utils`; this test doesn't reference it, so the declaration is dead weight. Flagged in CodeRabbit review on #12023. --- Written by an agent (Claude Code, claude-opus-4-7).
Ports pnpm's [`dedupeInjectedDeps`](https://github.com/pnpm/pnpm/blob/39101f5e37/installing/deps-resolver/src/dedupeInjectedDeps.ts) end-to-end: yaml plumbing, `Config` field (default `true`), env-var override, workspace-state round-trip, and a cross-importer pass that rewrites injected workspace deps whose children are a subset of the target project's direct deps from `file:` to `link:`, then prunes the orphaned snapshots from the merged graph. Refs #12009. --- Written by an agent (Claude Code, claude-opus-4-7).
…gh resolver The dedupeInjectedDeps consumer added in the previous commit was correct but unreachable from pacquet's install pipeline because WantedDependency.injected was never set from manifest reads. Threads dependenciesMeta[<alias>].injected: true through the importer-level wanted-dep collection in resolve_dependency_tree and resolve_importer so the npm-resolver's workspace-pick + local-resolver branches see the flag and produce a `file:<workspace>` resolution. Adds an end-to-end test (pacquet-cli) that drives `pacquet install` against a two-project workspace where `a` injects leaf `b`; with dedupe enabled the install collapses the injected dep back to `link:../b` and prunes the orphaned `file:packages/b` snapshot. Ports the spirit of pnpm's `'injected local packages are deduped'` test at installing/deps-installer/test/install/injectLocalPackages.ts. Refs #12009. --- Written by an agent (Claude Code, claude-opus-4-7).
Adds a workspace-wide `resolve_workspace` entry point that mirrors pnpm's [`resolveDependencies(importers, opts)`](https://github.com/pnpm/pnpm/blob/39101f5e37/installing/deps-resolver/src/index.ts#L128) shape: per-importer trees still come from `resolve_importer`, but the final peer-resolution + `dedupeInjectedDeps` pass runs once over a merged tree with all importers' direct deps in scope. Structural wins: - `peersCache` + `purePkgs` on the peer walker now persist across importers. An importer revisiting a `(pkgIdWithPatchHash, parent-peer-context)` pair an earlier importer already resolved short-circuits to the cached `depPath` instead of re-walking. - `dedupeInjectedDeps` moves out of the install layer and into the resolver, matching pnpm's `resolvePeers` integration. The per-importer `TreeCtx` is still constructed fresh per importer because `base_opts.project_dir` varies. Sharing `resolvedPkgsById` across importers is a separate axis — the lib.rs doc calls that out as a follow-up. Refs #12009. --- Written by an agent (Claude Code, claude-opus-4-7).
Two follow-ups on the multi-importer refactor: 1. **Share `TreeCtx` resolved-pkgs across importers.** Splits `TreeCtx` into a per-importer `TreeCtx` (`base_opts`, `patched_dependencies`) plus an `Arc<WorkspaceTreeCtx>` carrying the per-`pkgIdWithPatchHash` dedup maps (`packages`, `resolved_by_wanted`, `children_specs_by_id`, `children_by_id`) and peer-walker seed sets (`all_peer_dep_names`, `applied_patches`, `policy_violations`). `resolve_workspace` constructs one `WorkspaceTreeCtx`, hands `Arc::clone` to every per-importer `resolve_importer_with_workspace`, then snapshots the shared ctx into the merged `ResolvedTree`. Importer N's tree walk now reuses importer M's resolver hits instead of re-running the resolver chain. Closes the remaining gap with pnpm's `resolveDependencyTree`. 2. **`ImporterDepVersion::File` arm.** When `dedupeInjectedDeps: false` leaves an injected workspace dep as `file:packages/<name>` at the importer level, the lockfile writer used to panic at `importer_dep_version` (the `parse::<PkgNameVerPeer>().expect(...)` arm). Adds a `File(String)` variant to `ImporterDepVersion`, wires it through `dependencies_graph_to_lockfile` and `symlink_direct_dependencies`, and adds a new e2e test `injected_workspace_dep_with_dedupe_off_writes_file_arm` that exercises the formerly-panicking path end-to-end. Refs #12009. --- Written by an agent (Claude Code, claude-opus-4-7).
The dedupe e2e test was copied from `workspace_install.rs` which uses `_utils`; this test doesn't reference it, so the declaration is dead weight. Flagged in CodeRabbit review on #12023. --- Written by an agent (Claude Code, claude-opus-4-7).
`injected_workspace_dep_with_dedupe_off_writes_file_arm` panics on Windows in `pacquet_cmd_shim::read_manifest` trying to read `packages/a/node_modules/b/package.json` through a symlink whose target hasn't been materialised — pacquet's `create_virtual_store` doesn't copy `file:<workspace>` snapshots into the virtual store yet (broader gap tracked under #12009's `injectWorkspacePackages` line). Unix tolerates the broken link during the bin-link walk; Windows trips `ERROR_INVALID_NAME` on the mixed-separator path. The regression this test guards — `ImporterDepVersion::File` formatting in the lockfile writer — is platform-independent, so the Linux + macOS coverage remains representative. --- Written by an agent (Claude Code, claude-opus-4-7).
Two follow-ups surfaced by rebasing onto the latest main: - `injected_workspace_target` now accepts the `<name>@file:<path>` shape main's `build_pkg_id_with_patch_hash` produces once the manifest name is in scope, alongside the bare `file:<path>` form. Without this the dedupe pass missed every aliased workspace pick and the e2e tests regressed after the rebase. - Renamed the `resolve_workspace` generic parameters from `R` / `F` to `Chain` / `BuildImporterOptions` so Perfectionist's `single-letter-generic` lint stops blocking CI's Dylint job. --- Written by an agent (Claude Code, claude-opus-4-7).
The `dependencies_meta_injected_per_dep_overrides_global_off` test asserts the resolver records `project-1@file:project-1` for an injected workspace dep. With dedupeInjectedDeps defaulting to on and project-1 being a childless leaf, the file: resolution trivially dedupes back to `link:../project-1` — masking the assertion. Pin the setting off so the test exercises the resolver branch it was written for. Also apply cargo fmt to three files (CI flagged multi-line layouts that rustfmt collapses to single line).
8 tasks
This was referenced May 29, 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
Ports pnpm's
dedupeInjectedDepsto pacquet end-to-end, and restructures the resolver to match pnpm's multi-importer shape so the dedupe lives where pnpm puts it.dedupe_injected_deps: boolonConfig(defaulttrue), read frompnpm-workspace.yaml'sdedupeInjectedDepskey, overridable viaPNPM_CONFIG_DEDUPE_INJECTED_DEPS. Cleared as a workspace-only field inWorkspaceSettings::clear_workspace_only_fields.dependenciesMeta.injectedplumbing — pacquet's deps-resolver previously constructedWantedDependencywith..Default::default(), so the per-packagedependenciesMeta[<alias>].injected: trueflag never reached the npm/local resolvers and no install path produced afile:<workspace>direct dep. ReadingdependenciesMetaat the importer-level wanted-dep collection unlocks thefile:workspace-pick branch the dedupe consumer is designed to collapse.resolve_workspaceorchestrator (pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs) mirrors pnpm'sresolveDependencies(importers, opts). It constructs one sharedWorkspaceTreeCtx(resolved-pkgs dedup, children-spec cache, resolver-call memo, peer-walker seed sets), handsArc::cloneto every per-importerresolve_importer_with_workspace, then runs a singleresolve_peers_workspacepass that sharespeersCache+purePkgsacross importers. Importer N's tree walk now reuses importer M's resolver hits instead of re-running the chain.dedupeInjectedDepslives inresolve_peers_workspace(pacquet/crates/resolving-deps-resolver/src/dedupe_injected_deps.rs), matching pnpm'sresolvePeersintegration. The install layer no longer carries any dedupe wiring; it just hands importers + a per-importerResolveOptionsclosure toresolve_workspace. After dedupe, unreachablefile:<workspace>snapshots are pruned from the graph so they don't leak intopnpm-lock.yaml.ImporterDepVersion::Filearm — whendedupeInjectedDeps: falseleaves an injected workspace dep asfile:packages/<name>at the importer level, the lockfile writer used to panic atimporter_dep_version(theparse::<PkgNameVerPeer>().expect(...)arm). Adds aFile(String)variant toImporterDepVersion, wires it throughdependencies_graph_to_lockfileandsymlink_direct_dependencies, and the new e2e testinjected_workspace_dep_with_dedupe_off_writes_file_armexercises that path end-to-end.dedupe_injected_depsincurrent_settingsand adds it to thesettings_matchcomparison inoptimistic_repeat_install; drops it from the "deliberately not compared" list so settings drift now triggers a reinstall.Tracked under #12009 (one item; the rest are separate PRs).
Test plan
cargo nextest run -p pacquet-resolving-deps-resolver— 93/93 pass, including the 4 dedupe-module unit tests now under the resolver crate.cargo nextest run -p pacquet-package-manager— 340/340 pass.cargo nextest run -p pacquet-cli— 108/108 pass, including two e2e tests:dedupe_injected_deps::injected_leaf_workspace_dep_is_deduped_to_link— drivespacquet installagainst a two-project workspace usingdependenciesMeta.injected: true, asserts the resultinglink:symlink + lockfile shape (ports the spirit of pnpm's'injected local packages are deduped'test atinstalling/deps-installer/test/install/injectLocalPackages.ts:1785).dedupe_injected_deps::injected_workspace_dep_with_dedupe_off_writes_file_arm— same fixture withdedupeInjectedDeps: false, exercises theImporterDepVersion::Filearm so the formerly-panicking writer path is now covered.cargo nextest run -p pacquet-config -p pacquet-workspace-state -p pacquet-package-manifest -p pacquet-lockfile— pass.RUSTDOCFLAGS='--document-private-items -D warnings' cargo doc --no-deps -p pacquet-resolving-deps-resolver -p pacquet-package-manager -p pacquet-lockfileclean.cargo fmt --check,just check,just lintclean.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Bug Fixes
Tests