This repository was archived by the owner on May 14, 2026. It is now read-only.
Conversation
Micro-Benchmark ResultsLinux |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds pnpm-style dependency hoisting to pacquet installs, including pattern matching (hoistPattern / publicHoistPattern) and persistence of hoistedDependencies into .modules.yaml, to improve compatibility with packages that rely on phantom dependencies.
Changes:
- Introduces a new
pacquet_config::matchermodule implementing pnpm’s glob/negation “first match wins” semantics (noregexdependency). - Ports pnpm’s hoist graph walk + symlink creation into
pacquet_package_manager::hoistand wires it into the frozen-lockfile install pipeline; threads the resulting map into.modules.yaml. - Adds unit + integration tests for matcher and hoist behavior; updates
plans/TEST_PORTING.mdto mark ported/stubbed upstream cases.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| plans/TEST_PORTING.md | Marks upstream hoist tests as ported or stubbed with rationale. |
| crates/package-manager/src/lib.rs | Adds and re-exports the new hoist module. |
| crates/package-manager/src/install.rs | Threads HoistedDependencies into .modules.yaml generation. |
| crates/package-manager/src/install_without_lockfile.rs | Returns an empty HoistedDependencies map for symmetry with frozen-lockfile installs. |
| crates/package-manager/src/install_package_from_registry/tests.rs | Updates tests for Option<Vec<String>> hoist patterns. |
| crates/package-manager/src/install_frozen_lockfile.rs | Runs hoist pass after linking and persists resulting hoistedDependencies. |
| crates/package-manager/src/hoist.rs | Implements hoist graph build, BFS selection, symlink creation, and private-hoist bin linking. |
| crates/package-manager/src/hoist/tests.rs | Adds algorithm unit tests (patterns, precedence, skipping, bins set, helpers). |
| crates/config/src/workspace_yaml.rs | Applies workspace yaml overrides for hoist patterns (now Option<Vec<String>>). |
| crates/config/src/matcher.rs | Adds pnpm matcher port with tests. |
| crates/config/src/lib.rs | Promotes hoist patterns to Option<Vec<String>> and exports matcher module. |
| crates/cli/tests/hoist.rs | Adds end-to-end hoist integration tests + known-failure stubs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #445 +/- ##
==========================================
+ Coverage 87.32% 88.69% +1.37%
==========================================
Files 114 116 +2
Lines 9435 9917 +482
==========================================
+ Hits 8239 8796 +557
+ Misses 1196 1121 -75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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:
📝 WalkthroughWalkthroughAdds tri-state hoist configuration and workspace YAML handling, a pnpm-compatible glob matcher with ignore semantics, a BFS-based hoist decision pass with public/private precedence, symlink/bin-link wiring, install integration persisting hoistedDependencies into .modules.yaml, plus unit and Unix-only E2E tests. ChangesHoisting Implementation
End-to-End Tests and Documentation
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@crates/config/src/workspace_yaml.rs`:
- Around line 217-231: The YAML fields collapse missing vs explicit null; change
the YAML-mapped fields to preserve explicit null by making self.hoist_pattern
and self.public_hoist_pattern use a nested Option (e.g.
Option<Option<Vec<String>>> or use serde to preserve nulls), then update the
assignment logic in the block around
config.hoist_pattern/config.public_hoist_pattern to match the outer Option and
assign the inner Option directly (e.g. match self.hoist_pattern { Some(inner) =>
config.hoist_pattern = inner, None => {} }) so that an explicit `hoistPattern:
null` sets Config.* = None while a missing field leaves the default.
🪄 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: 9a93fa2d-5055-4d9e-a0f1-00f9f99e1d92
📒 Files selected for processing (12)
crates/cli/tests/hoist.rscrates/config/src/lib.rscrates/config/src/matcher.rscrates/config/src/workspace_yaml.rscrates/package-manager/src/hoist.rscrates/package-manager/src/hoist/tests.rscrates/package-manager/src/install.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/install_package_from_registry/tests.rscrates/package-manager/src/install_without_lockfile.rscrates/package-manager/src/lib.rsplans/TEST_PORTING.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;for any glob whose target is a module you control. External-crate preludes (e.g.,use rayon::prelude::*;) and root-of-module re-exports (e.g.,pub use submodule::*;inlib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plainStringor&str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only viaTryFrom<String>and/orFromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/package-manager/src/lib.rscrates/config/src/workspace_yaml.rscrates/package-manager/src/install_without_lockfile.rscrates/package-manager/src/install_package_from_registry/tests.rscrates/config/src/lib.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/install.rscrates/package-manager/src/hoist/tests.rscrates/cli/tests/hoist.rscrates/config/src/matcher.rscrates/package-manager/src/hoist.rs
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
When citing upstream pnpm code anywhere (code comments, doc comments, Markdown docs, PR descriptions, or commit messages), link to a specific commit SHA, not a branch name. Use the first 10 hex characters of the SHA. Branch links like
github.com/<owner>/<repo>/blob/main/...are impermanent; permanent links pin the commit so the reference stays meaningful long after upstream changes. This rule applies to references to any GitHub repository, not only pnpm/pnpm.
Files:
plans/TEST_PORTING.md
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: When porting behavior from pnpm, port the relevant pnpm tests to Rust tests whenever they translate. Matching test coverage is the easiest way to prove behavioral parity.
Consult the test-porting plan inplans/TEST_PORTING.mdbefore adding ported tests. Follow the conventions expected for ports: useknown_failuresmodules, usepacquet_testing_utils::allow_known_failure!at the not-yet-implemented boundary, and temporarily break the subject under test to verify the ported test actually catches the regression. Update TEST_PORTING.md checkboxes as items land.
Follow the test-logging guidance in CODE_STYLE_GUIDE.md: log before non-assert_eq!assertions, usedbg!for complex structures, skip logging for simple scalarassert_eq!assertions.
Files:
crates/cli/tests/hoist.rs
🧠 Learnings (3)
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.
Applied to files:
crates/package-manager/src/lib.rscrates/config/src/workspace_yaml.rscrates/package-manager/src/install_without_lockfile.rscrates/package-manager/src/install_package_from_registry/tests.rscrates/config/src/lib.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/install.rscrates/package-manager/src/hoist/tests.rscrates/cli/tests/hoist.rscrates/config/src/matcher.rscrates/package-manager/src/hoist.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.
Applied to files:
crates/package-manager/src/install_package_from_registry/tests.rscrates/package-manager/src/hoist/tests.rs
📚 Learning: 2026-05-07T14:24:47.105Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 391
File: crates/cli/tests/lifecycle_scripts.rs:0-0
Timestamp: 2026-05-07T14:24:47.105Z
Learning: In pnpm/pacquet CLI lifecycle tests, note that `AutoMockInstance::load_or_init` returns an anchor to a shared singleton mock registry process. If a test spawns a secondary `CommandTempCwd::init().add_mocked_registry()` (e.g., to run a reinstall with `--frozen-lockfile`), the secondary/inner `mock_instance` may be dropped safely as long as the primary/outer `mock_instance` remains in scope (so the singleton registry stays alive). Separately retain the inner `TempDir` (e.g., via a `frozen_root` binding) so the workspace lives for the duration of the command.
Applied to files:
crates/cli/tests/hoist.rs
🔇 Additional comments (21)
plans/TEST_PORTING.md (1)
100-127: LGTM!Also applies to: 131-132
crates/config/src/matcher.rs (1)
1-409: LGTM!crates/config/src/lib.rs (1)
2-2: LGTM!Also applies to: 141-166
crates/package-manager/src/install_package_from_registry/tests.rs (1)
20-21: LGTM!crates/package-manager/src/hoist.rs (1)
1-460: LGTM!crates/package-manager/src/lib.rs (1)
11-11: LGTM!Also applies to: 35-35
crates/package-manager/src/hoist/tests.rs (1)
1-435: LGTM!crates/package-manager/src/install_frozen_lockfile.rs (4)
1-7: LGTM!Also applies to: 115-115, 395-395
72-85: LGTM!
216-265: LGTM!
126-129: LGTM!crates/package-manager/src/install_without_lockfile.rs (1)
2-3: LGTM!Also applies to: 19-19, 67-76, 239-239
crates/package-manager/src/install.rs (3)
1-6: LGTM!
167-203: LGTM!
254-289: LGTM!crates/cli/tests/hoist.rs (6)
1-34: LGTM!
36-57: LGTM!
66-134: LGTM!
142-243: LGTM!
250-367: LGTM!
369-631: LGTM!
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.67773545922,
"stddev": 0.05820957782668119,
"median": 2.6737227419200003,
"user": 2.56906316,
"system": 3.6787571800000003,
"min": 2.60146721192,
"max": 2.75912799792,
"times": [
2.68981413392,
2.63180198492,
2.71076498692,
2.64400825692,
2.6035184139200003,
2.6576313499200004,
2.7394412219200004,
2.75912799792,
2.60146721192,
2.73977903392
]
},
{
"command": "pacquet@main",
"mean": 2.5538927081200002,
"stddev": 0.07063343141682328,
"median": 2.5419252189200003,
"user": 2.55563986,
"system": 3.5011243800000003,
"min": 2.47848991492,
"max": 2.69785538692,
"times": [
2.50134348092,
2.69785538692,
2.6038465199200003,
2.5504413289200003,
2.48349964292,
2.59521588092,
2.60243945792,
2.47848991492,
2.5334091089200004,
2.49238635892
]
},
{
"command": "pnpm",
"mean": 6.165193089519999,
"stddev": 0.11391130025365838,
"median": 6.14365161642,
"user": 9.17639686,
"system": 4.410328780000001,
"min": 6.049008401919999,
"max": 6.41686195692,
"times": [
6.16457974192,
6.232049527919999,
6.049008401919999,
6.26413996592,
6.111883048919999,
6.162859613919999,
6.41686195692,
6.050562643919999,
6.12444361892,
6.0755423749199995
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.72409764744,
"stddev": 0.035296770395962344,
"median": 0.71150619684,
"user": 0.37126145999999993,
"system": 1.5794607399999996,
"min": 0.6983492988400001,
"max": 0.8190021388400001,
"times": [
0.8190021388400001,
0.7333036528400001,
0.7057664158400001,
0.70391616584,
0.7065252848400001,
0.71985212884,
0.7083599378400001,
0.71465245584,
0.6983492988400001,
0.73124899484
]
},
{
"command": "pacquet@main",
"mean": 0.7520753165400001,
"stddev": 0.027140140130620584,
"median": 0.7507104318400002,
"user": 0.35557496000000005,
"system": 1.4769795399999999,
"min": 0.7199046968400001,
"max": 0.7935854678400001,
"times": [
0.7644278348400001,
0.73006427784,
0.7611183188400001,
0.7199046968400001,
0.7786677228400001,
0.7242942828400001,
0.7403025448400001,
0.7935854678400001,
0.7818153708400001,
0.7265726478400001
]
},
{
"command": "pnpm",
"mean": 2.5824919167399996,
"stddev": 0.09976464169638383,
"median": 2.55900032984,
"user": 3.17489806,
"system": 2.14910604,
"min": 2.4991594408399997,
"max": 2.83884071184,
"times": [
2.6433046558399997,
2.57191786484,
2.55115563984,
2.83884071184,
2.4991594408399997,
2.51945165784,
2.56684501984,
2.50405790684,
2.58906871084,
2.54111755884
]
}
]
} |
zkochan
added a commit
that referenced
this pull request
May 13, 2026
… bin pass Address PR #445 review feedback. - **Link public-hoisted bins** (Copilot). Hoist now collects `publicly_hoisted_aliases_with_bins` alongside the private side. `InstallFrozenLockfile::run` runs a second `link_direct_dep_bins` pass against `<root>/node_modules` after the hoist symlinks land, so public-hoisted bins reach `<root>/node_modules/.bin` even though pacquet's `SymlinkDirectDependencies` runs *before* the hoist (upstream's order is reversed). - **Tri-state null hoist patterns** (Copilot, coderabbit). YAML fields become `Option<Option<Vec<String>>>` via a small `deserialize_double_option` helper. Now distinguishes: - missing key → outer `None` → leave config defaults in place - explicit `null` → `Some(None)` → disable that hoist side - explicit list → `Some(Some(vec))` → override Mirrors upstream's `hoistPattern != null` guard semantics. Workspace_yaml `apply_to` uses the inner option directly. - **Reuse `link_direct_dep_bins`** (Copilot). Drop the sequential `link_hoisted_bins` helper that duplicated the manifest-read + `link_bins_of_packages` flow. Both private and public hoist passes now go through the existing rayon-parallel `link_direct_dep_bins`. - **Strengthen `public_hoist_bin_is_linked_via_root_bin_dir` test** (Copilot). Now asserts `<root>/node_modules/.bin/hello-world-js-bin` exists, not just the alias symlink. Catches the bin-link gap the first round failed to detect. - Add `hoist_patterns_tri_state_round_trip` regression test in `workspace_yaml::tests` covering all three serde shapes (missing, null, list) for both pattern fields. Tests: `just ready` 671/671 pass; hoist CLI suite 35/35; matcher 6/6; hoist algorithm 12/12; new tri-state test passes.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
… bin pass Address PR #445 review feedback. - **Link public-hoisted bins** (Copilot). Hoist now collects `publicly_hoisted_aliases_with_bins` alongside the private side. `InstallFrozenLockfile::run` runs a second `link_direct_dep_bins` pass against `<root>/node_modules` after the hoist symlinks land, so public-hoisted bins reach `<root>/node_modules/.bin` even though pacquet's `SymlinkDirectDependencies` runs *before* the hoist (upstream's order is reversed). - **Tri-state null hoist patterns** (Copilot, coderabbit). YAML fields become `Option<Option<Vec<String>>>` via a small `deserialize_double_option` helper. Now distinguishes: - missing key → outer `None` → leave config defaults in place - explicit `null` → `Some(None)` → disable that hoist side - explicit list → `Some(Some(vec))` → override Mirrors upstream's `hoistPattern != null` guard semantics. Workspace_yaml `apply_to` uses the inner option directly. - **Reuse `link_direct_dep_bins`** (Copilot). Drop the sequential `link_hoisted_bins` helper that duplicated the manifest-read + `link_bins_of_packages` flow. Both private and public hoist passes now go through the existing rayon-parallel `link_direct_dep_bins`. - **Strengthen `public_hoist_bin_is_linked_via_root_bin_dir` test** (Copilot). Now asserts `<root>/node_modules/.bin/hello-world-js-bin` exists, not just the alias symlink. Catches the bin-link gap the first round failed to detect. - Add `hoist_patterns_tri_state_round_trip` regression test in `workspace_yaml::tests` covering all three serde shapes (missing, null, list) for both pattern fields. Tests: `just ready` 671/671 pass; hoist CLI suite 35/35; matcher 6/6; hoist algorithm 12/12; new tri-state test passes.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
…cleanup Address PR #445 follow-up review and the failed Doc CI job. **Doc check** (CI gate) - Drop the broken intra-doc link `crate::link_bins::link_bins_of_packages` and the dangling `link_hoisted_bins` reference (deleted in eeda321). Module-level doc now points at `crate::link_direct_dep_bins` via the call site. - Disambiguate `crate::symlink_package` (both a module and a function) with `()` to pin the function form. - Replace the `[\`deserialize_double_option\`]` link from public `WorkspaceSettings::hoist_pattern` with prose — the helper is intentionally private and `-D rustdoc::private-intra-doc-links` rejects the link from a public item. **Restrict hoist input to root importer** (Copilot) - `build_direct_deps_by_importer` now takes `IntoIterator<Item = (&String, &ProjectSnapshot)>` instead of `&HashMap<...>`. The frozen-lockfile call site filters down to the single root importer via `importers.get_key_value(ROOT_IMPORTER_KEY)`. With workspace install (#431) not yet implemented, hoisting transitives of non-root importers would have polluted the root project's `node_modules` with packages that aren't actually installed. **Correct `MatcherImpl::Mixed` doc comment** (Copilot) - Old comment said "a subsequent include does not re-take the sticky slot" — wrong. The implementation lets a later include re-take after a matching ignore clears the sticky slot, which is exactly what the `eslint-*`, `!eslint-plugin-*`, `eslint-plugin-bar` test case relies on. Comment now describes the actual "first include after the last matching ignore wins" behavior with the test case cited. **Dedicated `external_symlink_introspection` known_failure** (Copilot) - `public_hoist_preserves_existing_root_directories` was gated on `partial_install_persists_hoisted_map()` even though its doc describes the missing `resolveLinkTarget` + `isSubdir` introspection. Added a dedicated `external_symlink_introspection` reason and pointed the stub at it so the gate matches the actual blocker. Tests: `just ready` 705/705 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
… bin pass Address PR #445 review feedback. - **Link public-hoisted bins** (Copilot). Hoist now collects `publicly_hoisted_aliases_with_bins` alongside the private side. `InstallFrozenLockfile::run` runs a second `link_direct_dep_bins` pass against `<root>/node_modules` after the hoist symlinks land, so public-hoisted bins reach `<root>/node_modules/.bin` even though pacquet's `SymlinkDirectDependencies` runs *before* the hoist (upstream's order is reversed). - **Tri-state null hoist patterns** (Copilot, coderabbit). YAML fields become `Option<Option<Vec<String>>>` via a small `deserialize_double_option` helper. Now distinguishes: - missing key → outer `None` → leave config defaults in place - explicit `null` → `Some(None)` → disable that hoist side - explicit list → `Some(Some(vec))` → override Mirrors upstream's `hoistPattern != null` guard semantics. Workspace_yaml `apply_to` uses the inner option directly. - **Reuse `link_direct_dep_bins`** (Copilot). Drop the sequential `link_hoisted_bins` helper that duplicated the manifest-read + `link_bins_of_packages` flow. Both private and public hoist passes now go through the existing rayon-parallel `link_direct_dep_bins`. - **Strengthen `public_hoist_bin_is_linked_via_root_bin_dir` test** (Copilot). Now asserts `<root>/node_modules/.bin/hello-world-js-bin` exists, not just the alias symlink. Catches the bin-link gap the first round failed to detect. - Add `hoist_patterns_tri_state_round_trip` regression test in `workspace_yaml::tests` covering all three serde shapes (missing, null, list) for both pattern fields. Tests: `just ready` 671/671 pass; hoist CLI suite 35/35; matcher 6/6; hoist algorithm 12/12; new tri-state test passes.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
…cleanup Address PR #445 follow-up review and the failed Doc CI job. **Doc check** (CI gate) - Drop the broken intra-doc link `crate::link_bins::link_bins_of_packages` and the dangling `link_hoisted_bins` reference (deleted in eeda321). Module-level doc now points at `crate::link_direct_dep_bins` via the call site. - Disambiguate `crate::symlink_package` (both a module and a function) with `()` to pin the function form. - Replace the `[\`deserialize_double_option\`]` link from public `WorkspaceSettings::hoist_pattern` with prose — the helper is intentionally private and `-D rustdoc::private-intra-doc-links` rejects the link from a public item. **Restrict hoist input to root importer** (Copilot) - `build_direct_deps_by_importer` now takes `IntoIterator<Item = (&String, &ProjectSnapshot)>` instead of `&HashMap<...>`. The frozen-lockfile call site filters down to the single root importer via `importers.get_key_value(ROOT_IMPORTER_KEY)`. With workspace install (#431) not yet implemented, hoisting transitives of non-root importers would have polluted the root project's `node_modules` with packages that aren't actually installed. **Correct `MatcherImpl::Mixed` doc comment** (Copilot) - Old comment said "a subsequent include does not re-take the sticky slot" — wrong. The implementation lets a later include re-take after a matching ignore clears the sticky slot, which is exactly what the `eslint-*`, `!eslint-plugin-*`, `eslint-plugin-bar` test case relies on. Comment now describes the actual "first include after the last matching ignore wins" behavior with the test case cited. **Dedicated `external_symlink_introspection` known_failure** (Copilot) - `public_hoist_preserves_existing_root_directories` was gated on `partial_install_persists_hoisted_map()` even though its doc describes the missing `resolveLinkTarget` + `isSubdir` introspection. Added a dedicated `external_symlink_introspection` reason and pointed the stub at it so the gate matches the actual blocker. Tests: `just ready` 705/705 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Address PR #445 review feedback (round 3). - **`Config.hoist == false` now nullifies `hoist_pattern`** (Copilot). `WorkspaceSettings::apply_to` runs an `!config.hoist ⇒ config.hoist_pattern = None` post-step after assigning the per- field overrides. Mirrors upstream's [`projectConfig.ts:72-75`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/projectConfig.ts#L72-L75) `result.hoist === false ⇒ hoistPattern: undefined`. `public_hoist_pattern` is intentionally untouched (upstream doesn't nullify it either; public hoisting is governed by its own pattern + `shamefullyHoist`). Setting `hoist: false` in `pnpm-workspace.yaml` now correctly short-circuits private hoisting via the install-time `is_some() || is_some()` guard, even when the default `Some(["*"])` pattern would otherwise apply. - **`PkgName::clone(name)` → `name.clone()`** (Copilot style nit). Both forms compile identically (`Type::method(value)` is fully- qualified syntax for trait methods), but method-call form is consistent with the rest of the file. - **Fix misleading test doc comment** (Copilot). `skipped_snapshot_is_excluded`'s comment said the symlink pass "attempts and fails gracefully on the missing node" — wrong on two counts: `symlink_hoisted_dependencies` skips missing nodes via an early `let Some(_) else { continue }` (no attempt), and `symlink_package` on Unix would create a dangling symlink rather than fail anyway. Updated to describe the actual skip + cite upstream's pre-skip recording behavior. - New regression test `hoist_false_disables_private_hoist_pattern` in `workspace_yaml::tests` covers `hoist: false` alone (drops the default pattern, leaves public alone) and `hoist: false` paired with an explicit `hoistPattern` (drops the explicit value too). Tests: `just ready` 756/756 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
… bin pass Address PR #445 review feedback. - **Link public-hoisted bins** (Copilot). Hoist now collects `publicly_hoisted_aliases_with_bins` alongside the private side. `InstallFrozenLockfile::run` runs a second `link_direct_dep_bins` pass against `<root>/node_modules` after the hoist symlinks land, so public-hoisted bins reach `<root>/node_modules/.bin` even though pacquet's `SymlinkDirectDependencies` runs *before* the hoist (upstream's order is reversed). - **Tri-state null hoist patterns** (Copilot, coderabbit). YAML fields become `Option<Option<Vec<String>>>` via a small `deserialize_double_option` helper. Now distinguishes: - missing key → outer `None` → leave config defaults in place - explicit `null` → `Some(None)` → disable that hoist side - explicit list → `Some(Some(vec))` → override Mirrors upstream's `hoistPattern != null` guard semantics. Workspace_yaml `apply_to` uses the inner option directly. - **Reuse `link_direct_dep_bins`** (Copilot). Drop the sequential `link_hoisted_bins` helper that duplicated the manifest-read + `link_bins_of_packages` flow. Both private and public hoist passes now go through the existing rayon-parallel `link_direct_dep_bins`. - **Strengthen `public_hoist_bin_is_linked_via_root_bin_dir` test** (Copilot). Now asserts `<root>/node_modules/.bin/hello-world-js-bin` exists, not just the alias symlink. Catches the bin-link gap the first round failed to detect. - Add `hoist_patterns_tri_state_round_trip` regression test in `workspace_yaml::tests` covering all three serde shapes (missing, null, list) for both pattern fields. Tests: `just ready` 671/671 pass; hoist CLI suite 35/35; matcher 6/6; hoist algorithm 12/12; new tri-state test passes.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
…cleanup Address PR #445 follow-up review and the failed Doc CI job. **Doc check** (CI gate) - Drop the broken intra-doc link `crate::link_bins::link_bins_of_packages` and the dangling `link_hoisted_bins` reference (deleted in eeda321). Module-level doc now points at `crate::link_direct_dep_bins` via the call site. - Disambiguate `crate::symlink_package` (both a module and a function) with `()` to pin the function form. - Replace the `[\`deserialize_double_option\`]` link from public `WorkspaceSettings::hoist_pattern` with prose — the helper is intentionally private and `-D rustdoc::private-intra-doc-links` rejects the link from a public item. **Restrict hoist input to root importer** (Copilot) - `build_direct_deps_by_importer` now takes `IntoIterator<Item = (&String, &ProjectSnapshot)>` instead of `&HashMap<...>`. The frozen-lockfile call site filters down to the single root importer via `importers.get_key_value(ROOT_IMPORTER_KEY)`. With workspace install (#431) not yet implemented, hoisting transitives of non-root importers would have polluted the root project's `node_modules` with packages that aren't actually installed. **Correct `MatcherImpl::Mixed` doc comment** (Copilot) - Old comment said "a subsequent include does not re-take the sticky slot" — wrong. The implementation lets a later include re-take after a matching ignore clears the sticky slot, which is exactly what the `eslint-*`, `!eslint-plugin-*`, `eslint-plugin-bar` test case relies on. Comment now describes the actual "first include after the last matching ignore wins" behavior with the test case cited. **Dedicated `external_symlink_introspection` known_failure** (Copilot) - `public_hoist_preserves_existing_root_directories` was gated on `partial_install_persists_hoisted_map()` even though its doc describes the missing `resolveLinkTarget` + `isSubdir` introspection. Added a dedicated `external_symlink_introspection` reason and pointed the stub at it so the gate matches the actual blocker. Tests: `just ready` 705/705 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Address PR #445 review feedback (round 3). - **`Config.hoist == false` now nullifies `hoist_pattern`** (Copilot). `WorkspaceSettings::apply_to` runs an `!config.hoist ⇒ config.hoist_pattern = None` post-step after assigning the per- field overrides. Mirrors upstream's [`projectConfig.ts:72-75`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/projectConfig.ts#L72-L75) `result.hoist === false ⇒ hoistPattern: undefined`. `public_hoist_pattern` is intentionally untouched (upstream doesn't nullify it either; public hoisting is governed by its own pattern + `shamefullyHoist`). Setting `hoist: false` in `pnpm-workspace.yaml` now correctly short-circuits private hoisting via the install-time `is_some() || is_some()` guard, even when the default `Some(["*"])` pattern would otherwise apply. - **`PkgName::clone(name)` → `name.clone()`** (Copilot style nit). Both forms compile identically (`Type::method(value)` is fully- qualified syntax for trait methods), but method-call form is consistent with the rest of the file. - **Fix misleading test doc comment** (Copilot). `skipped_snapshot_is_excluded`'s comment said the symlink pass "attempts and fails gracefully on the missing node" — wrong on two counts: `symlink_hoisted_dependencies` skips missing nodes via an early `let Some(_) else { continue }` (no attempt), and `symlink_package` on Unix would create a dangling symlink rather than fail anyway. Updated to describe the actual skip + cite upstream's pre-skip recording behavior. - New regression test `hoist_false_disables_private_hoist_pattern` in `workspace_yaml::tests` covers `hoist: false` alone (drops the default pattern, leaves public alone) and `hoist: false` paired with an explicit `hoistPattern` (drops the explicit value too). Tests: `just ready` 756/756 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Origin/main grew a `path: Option<String>` field on `TarballResolution` (PR #451, `feat(git-fetcher): install git-hosted tarballs via preparePackage + packlist`). That PR landed after #439 (`feat(package-is-installable): platform + engine check`) and missed updating the `TarballResolution` literal in `crates/package-manager/src/installability/tests.rs`, so any PR opened against current main inherits a build break in the `package-is-installable` test build (#445 hit the same failure). My own `peer_dependency_in_lockfile_surfaces_unsupported` test in `crates/real-hoist/src/tests.rs` had the same gap. Add `path: None` to both literals so the test builds compile against the post-#451 `TarballResolution` shape. Includes the rebase of feat/438-slice-3b onto current origin/main.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
…cleanup Address PR #445 follow-up review and the failed Doc CI job. **Doc check** (CI gate) - Drop the broken intra-doc link `crate::link_bins::link_bins_of_packages` and the dangling `link_hoisted_bins` reference (deleted in eeda321). Module-level doc now points at `crate::link_direct_dep_bins` via the call site. - Disambiguate `crate::symlink_package` (both a module and a function) with `()` to pin the function form. - Replace the `[\`deserialize_double_option\`]` link from public `WorkspaceSettings::hoist_pattern` with prose — the helper is intentionally private and `-D rustdoc::private-intra-doc-links` rejects the link from a public item. **Restrict hoist input to root importer** (Copilot) - `build_direct_deps_by_importer` now takes `IntoIterator<Item = (&String, &ProjectSnapshot)>` instead of `&HashMap<...>`. The frozen-lockfile call site filters down to the single root importer via `importers.get_key_value(ROOT_IMPORTER_KEY)`. With workspace install (#431) not yet implemented, hoisting transitives of non-root importers would have polluted the root project's `node_modules` with packages that aren't actually installed. **Correct `MatcherImpl::Mixed` doc comment** (Copilot) - Old comment said "a subsequent include does not re-take the sticky slot" — wrong. The implementation lets a later include re-take after a matching ignore clears the sticky slot, which is exactly what the `eslint-*`, `!eslint-plugin-*`, `eslint-plugin-bar` test case relies on. Comment now describes the actual "first include after the last matching ignore wins" behavior with the test case cited. **Dedicated `external_symlink_introspection` known_failure** (Copilot) - `public_hoist_preserves_existing_root_directories` was gated on `partial_install_persists_hoisted_map()` even though its doc describes the missing `resolveLinkTarget` + `isSubdir` introspection. Added a dedicated `external_symlink_introspection` reason and pointed the stub at it so the gate matches the actual blocker. Tests: `just ready` 705/705 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Address PR #445 review feedback (round 3). - **`Config.hoist == false` now nullifies `hoist_pattern`** (Copilot). `WorkspaceSettings::apply_to` runs an `!config.hoist ⇒ config.hoist_pattern = None` post-step after assigning the per- field overrides. Mirrors upstream's [`projectConfig.ts:72-75`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/projectConfig.ts#L72-L75) `result.hoist === false ⇒ hoistPattern: undefined`. `public_hoist_pattern` is intentionally untouched (upstream doesn't nullify it either; public hoisting is governed by its own pattern + `shamefullyHoist`). Setting `hoist: false` in `pnpm-workspace.yaml` now correctly short-circuits private hoisting via the install-time `is_some() || is_some()` guard, even when the default `Some(["*"])` pattern would otherwise apply. - **`PkgName::clone(name)` → `name.clone()`** (Copilot style nit). Both forms compile identically (`Type::method(value)` is fully- qualified syntax for trait methods), but method-call form is consistent with the rest of the file. - **Fix misleading test doc comment** (Copilot). `skipped_snapshot_is_excluded`'s comment said the symlink pass "attempts and fails gracefully on the missing node" — wrong on two counts: `symlink_hoisted_dependencies` skips missing nodes via an early `let Some(_) else { continue }` (no attempt), and `symlink_package` on Unix would create a dangling symlink rather than fail anyway. Updated to describe the actual skip + cite upstream's pre-skip recording behavior. - New regression test `hoist_false_disables_private_hoist_pattern` in `workspace_yaml::tests` covers `hoist: false` alone (drops the default pattern, leaves public alone) and `hoist: false` paired with an explicit `hoistPattern` (drops the explicit value too). Tests: `just ready` 756/756 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
… bin pass Address PR #445 review feedback. - **Link public-hoisted bins** (Copilot). Hoist now collects `publicly_hoisted_aliases_with_bins` alongside the private side. `InstallFrozenLockfile::run` runs a second `link_direct_dep_bins` pass against `<root>/node_modules` after the hoist symlinks land, so public-hoisted bins reach `<root>/node_modules/.bin` even though pacquet's `SymlinkDirectDependencies` runs *before* the hoist (upstream's order is reversed). - **Tri-state null hoist patterns** (Copilot, coderabbit). YAML fields become `Option<Option<Vec<String>>>` via a small `deserialize_double_option` helper. Now distinguishes: - missing key → outer `None` → leave config defaults in place - explicit `null` → `Some(None)` → disable that hoist side - explicit list → `Some(Some(vec))` → override Mirrors upstream's `hoistPattern != null` guard semantics. Workspace_yaml `apply_to` uses the inner option directly. - **Reuse `link_direct_dep_bins`** (Copilot). Drop the sequential `link_hoisted_bins` helper that duplicated the manifest-read + `link_bins_of_packages` flow. Both private and public hoist passes now go through the existing rayon-parallel `link_direct_dep_bins`. - **Strengthen `public_hoist_bin_is_linked_via_root_bin_dir` test** (Copilot). Now asserts `<root>/node_modules/.bin/hello-world-js-bin` exists, not just the alias symlink. Catches the bin-link gap the first round failed to detect. - Add `hoist_patterns_tri_state_round_trip` regression test in `workspace_yaml::tests` covering all three serde shapes (missing, null, list) for both pattern fields. Tests: `just ready` 671/671 pass; hoist CLI suite 35/35; matcher 6/6; hoist algorithm 12/12; new tri-state test passes.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
…cleanup Address PR #445 follow-up review and the failed Doc CI job. **Doc check** (CI gate) - Drop the broken intra-doc link `crate::link_bins::link_bins_of_packages` and the dangling `link_hoisted_bins` reference (deleted in eeda321). Module-level doc now points at `crate::link_direct_dep_bins` via the call site. - Disambiguate `crate::symlink_package` (both a module and a function) with `()` to pin the function form. - Replace the `[\`deserialize_double_option\`]` link from public `WorkspaceSettings::hoist_pattern` with prose — the helper is intentionally private and `-D rustdoc::private-intra-doc-links` rejects the link from a public item. **Restrict hoist input to root importer** (Copilot) - `build_direct_deps_by_importer` now takes `IntoIterator<Item = (&String, &ProjectSnapshot)>` instead of `&HashMap<...>`. The frozen-lockfile call site filters down to the single root importer via `importers.get_key_value(ROOT_IMPORTER_KEY)`. With workspace install (#431) not yet implemented, hoisting transitives of non-root importers would have polluted the root project's `node_modules` with packages that aren't actually installed. **Correct `MatcherImpl::Mixed` doc comment** (Copilot) - Old comment said "a subsequent include does not re-take the sticky slot" — wrong. The implementation lets a later include re-take after a matching ignore clears the sticky slot, which is exactly what the `eslint-*`, `!eslint-plugin-*`, `eslint-plugin-bar` test case relies on. Comment now describes the actual "first include after the last matching ignore wins" behavior with the test case cited. **Dedicated `external_symlink_introspection` known_failure** (Copilot) - `public_hoist_preserves_existing_root_directories` was gated on `partial_install_persists_hoisted_map()` even though its doc describes the missing `resolveLinkTarget` + `isSubdir` introspection. Added a dedicated `external_symlink_introspection` reason and pointed the stub at it so the gate matches the actual blocker. Tests: `just ready` 705/705 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
zkochan
added a commit
that referenced
this pull request
May 13, 2026
Address PR #445 review feedback (round 3). - **`Config.hoist == false` now nullifies `hoist_pattern`** (Copilot). `WorkspaceSettings::apply_to` runs an `!config.hoist ⇒ config.hoist_pattern = None` post-step after assigning the per- field overrides. Mirrors upstream's [`projectConfig.ts:72-75`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/projectConfig.ts#L72-L75) `result.hoist === false ⇒ hoistPattern: undefined`. `public_hoist_pattern` is intentionally untouched (upstream doesn't nullify it either; public hoisting is governed by its own pattern + `shamefullyHoist`). Setting `hoist: false` in `pnpm-workspace.yaml` now correctly short-circuits private hoisting via the install-time `is_some() || is_some()` guard, even when the default `Some(["*"])` pattern would otherwise apply. - **`PkgName::clone(name)` → `name.clone()`** (Copilot style nit). Both forms compile identically (`Type::method(value)` is fully- qualified syntax for trait methods), but method-call form is consistent with the rest of the file. - **Fix misleading test doc comment** (Copilot). `skipped_snapshot_is_excluded`'s comment said the symlink pass "attempts and fails gracefully on the missing node" — wrong on two counts: `symlink_hoisted_dependencies` skips missing nodes via an early `let Some(_) else { continue }` (no attempt), and `symlink_package` on Unix would create a dangling symlink rather than fail anyway. Updated to describe the actual skip + cite upstream's pre-skip recording behavior. - New regression test `hoist_false_disables_private_hoist_pattern` in `workspace_yaml::tests` covers `hoist: false` alone (drops the default pattern, leaves public alone) and `hoist: false` paired with an explicit `hoistPattern` (drops the explicit value too). Tests: `just ready` 756/756 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
Port pnpm v11's hoist pass into pacquet's frozen-lockfile install path. Closes #435. - `pacquet_config::matcher` ports `@pnpm/config.matcher` (`*`-only glob, `!`-negation, first-match-wins; no regex dependency). - `pacquet_package_manager::hoist` ports `getHoistedDependencies` and `symlinkHoistedDependencies` from `installing/linking/hoist/src/index.ts`. The BFS, sort by (depth, nodeId), public-wins-ties, first-seen-wins-per-alias, and root-importer-direct-dep seed all mirror upstream. - Hoist patterns become `Option<Vec<String>>` on `Config` so the install-time `is_some() || is_some()` guard mirrors upstream's `!= null` check. - `InstallFrozenLockfile::run` returns `HoistedDependencies` and threads it into `.modules.yaml` via `Install::run`. The without-lockfile path returns an empty map (no graph to walk). - Symlinks land at `<vs>/node_modules/<alias>` (private) and `<root>/node_modules/<alias>` (public). Private-side bins go into `<vs>/node_modules/.bin/`; public-side bins reuse the existing direct-deps bin pass. - Out of scope per the issue: GVS (#432), workspace install (#431), partial install (#433), and `nodeLinker: hoisted`. Tests that depend on those features live in `crates/cli/tests/hoist.rs`'s `known_failures` module via `allow_known_failure!`. Tests: 6 matcher unit tests, 12 hoist algorithm unit tests, 9 CLI end-to-end tests, and 17 `known_failures` stubs porting the remainder of `installing/deps-installer/test/install/hoist.ts`. `plans/TEST_PORTING.md` updated to mark the ported entries. Upstream pinned at pnpm v11 `94240bc046`.
… bin pass Address PR #445 review feedback. - **Link public-hoisted bins** (Copilot). Hoist now collects `publicly_hoisted_aliases_with_bins` alongside the private side. `InstallFrozenLockfile::run` runs a second `link_direct_dep_bins` pass against `<root>/node_modules` after the hoist symlinks land, so public-hoisted bins reach `<root>/node_modules/.bin` even though pacquet's `SymlinkDirectDependencies` runs *before* the hoist (upstream's order is reversed). - **Tri-state null hoist patterns** (Copilot, coderabbit). YAML fields become `Option<Option<Vec<String>>>` via a small `deserialize_double_option` helper. Now distinguishes: - missing key → outer `None` → leave config defaults in place - explicit `null` → `Some(None)` → disable that hoist side - explicit list → `Some(Some(vec))` → override Mirrors upstream's `hoistPattern != null` guard semantics. Workspace_yaml `apply_to` uses the inner option directly. - **Reuse `link_direct_dep_bins`** (Copilot). Drop the sequential `link_hoisted_bins` helper that duplicated the manifest-read + `link_bins_of_packages` flow. Both private and public hoist passes now go through the existing rayon-parallel `link_direct_dep_bins`. - **Strengthen `public_hoist_bin_is_linked_via_root_bin_dir` test** (Copilot). Now asserts `<root>/node_modules/.bin/hello-world-js-bin` exists, not just the alias symlink. Catches the bin-link gap the first round failed to detect. - Add `hoist_patterns_tri_state_round_trip` regression test in `workspace_yaml::tests` covering all three serde shapes (missing, null, list) for both pattern fields. Tests: `just ready` 671/671 pass; hoist CLI suite 35/35; matcher 6/6; hoist algorithm 12/12; new tri-state test passes.
…cleanup Address PR #445 follow-up review and the failed Doc CI job. **Doc check** (CI gate) - Drop the broken intra-doc link `crate::link_bins::link_bins_of_packages` and the dangling `link_hoisted_bins` reference (deleted in eeda321). Module-level doc now points at `crate::link_direct_dep_bins` via the call site. - Disambiguate `crate::symlink_package` (both a module and a function) with `()` to pin the function form. - Replace the `[\`deserialize_double_option\`]` link from public `WorkspaceSettings::hoist_pattern` with prose — the helper is intentionally private and `-D rustdoc::private-intra-doc-links` rejects the link from a public item. **Restrict hoist input to root importer** (Copilot) - `build_direct_deps_by_importer` now takes `IntoIterator<Item = (&String, &ProjectSnapshot)>` instead of `&HashMap<...>`. The frozen-lockfile call site filters down to the single root importer via `importers.get_key_value(ROOT_IMPORTER_KEY)`. With workspace install (#431) not yet implemented, hoisting transitives of non-root importers would have polluted the root project's `node_modules` with packages that aren't actually installed. **Correct `MatcherImpl::Mixed` doc comment** (Copilot) - Old comment said "a subsequent include does not re-take the sticky slot" — wrong. The implementation lets a later include re-take after a matching ignore clears the sticky slot, which is exactly what the `eslint-*`, `!eslint-plugin-*`, `eslint-plugin-bar` test case relies on. Comment now describes the actual "first include after the last matching ignore wins" behavior with the test case cited. **Dedicated `external_symlink_introspection` known_failure** (Copilot) - `public_hoist_preserves_existing_root_directories` was gated on `partial_install_persists_hoisted_map()` even though its doc describes the missing `resolveLinkTarget` + `isSubdir` introspection. Added a dedicated `external_symlink_introspection` reason and pointed the stub at it so the gate matches the actual blocker. Tests: `just ready` 705/705 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
Address PR #445 review feedback (round 3). - **`Config.hoist == false` now nullifies `hoist_pattern`** (Copilot). `WorkspaceSettings::apply_to` runs an `!config.hoist ⇒ config.hoist_pattern = None` post-step after assigning the per- field overrides. Mirrors upstream's [`projectConfig.ts:72-75`](https://github.com/pnpm/pnpm/blob/94240bc046/config/reader/src/projectConfig.ts#L72-L75) `result.hoist === false ⇒ hoistPattern: undefined`. `public_hoist_pattern` is intentionally untouched (upstream doesn't nullify it either; public hoisting is governed by its own pattern + `shamefullyHoist`). Setting `hoist: false` in `pnpm-workspace.yaml` now correctly short-circuits private hoisting via the install-time `is_some() || is_some()` guard, even when the default `Some(["*"])` pattern would otherwise apply. - **`PkgName::clone(name)` → `name.clone()`** (Copilot style nit). Both forms compile identically (`Type::method(value)` is fully- qualified syntax for trait methods), but method-call form is consistent with the rest of the file. - **Fix misleading test doc comment** (Copilot). `skipped_snapshot_is_excluded`'s comment said the symlink pass "attempts and fails gracefully on the missing node" — wrong on two counts: `symlink_hoisted_dependencies` skips missing nodes via an early `let Some(_) else { continue }` (no attempt), and `symlink_package` on Unix would create a dangling symlink rather than fail anyway. Updated to describe the actual skip + cite upstream's pre-skip recording behavior. - New regression test `hoist_false_disables_private_hoist_pattern` in `workspace_yaml::tests` covers `hoist: false` alone (drops the default pattern, leaves public alone) and `hoist: false` paired with an explicit `hoistPattern` (drops the explicit value too). Tests: `just ready` 756/756 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
…hers
Address the ~200ms regression vs main's frozen-lockfile baseline by
trimming the hoist pass's IO and skipping work when the matchers
can't match anything.
Three changes, in order of expected impact:
- **Parallelize `symlink_hoisted_dependencies` via rayon.** The pass
now runs in three phases:
1. Walk the input once to collect every `(target, dest)` pair plus
the set of `<root>/@scope` dirs needed by scoped aliases.
2. `create_dir_all` the two hoisted-modules roots and each scope
dir — once per dir, not per symlink. The previous version
called `create_dir_all` inside `symlink_package` on every
symlink, paying ~1k redundant stats on the same handful of
parents for a typical install.
3. `pairs.par_iter().try_for_each(...)` issues `symlinkat()`
syscalls in parallel via the existing rayon thread pool. Each
pair is now a single syscall, so the only contention is the
kernel's parent-directory inode lock.
- **Static fast-path for empty pattern lists.** Add
`Matcher::is_empty()` (returns `true` only for matchers built
from `&[]`). When *both* compiled matchers are empty,
`InstallFrozenLockfile::run` short-circuits before building the
hoist graph, walking the BFS, and entering the symlink phase.
Previously these would all run and produce empty output.
- **Drop the now-unused `crate::symlink_package` import** from the
hoist module — the new path uses `pacquet_fs::symlink_dir`
directly.
Tests: `just ready` 808/808 pass; `cargo doc --workspace --no-deps`
with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean. New
`is_empty_only_for_empty_pattern_list` unit test guards the
short-circuit guard. CLI hoist integration tests (35/35) verify the
parallelized symlink phase produces the same on-disk layout.
Workspace install (#431) landed in #443. Pacquet now installs every entry in `Lockfile.importers`, not just the root — hoist needs to follow. - `InstallFrozenLockfile::run` now passes the full `importers` map to `build_direct_deps_by_importer` instead of filtering down to the root importer. Transitives unique to a workspace package now reach the shared `<vs>/node_modules` private hoist target, matching upstream's `directDepsByImporterId` shape. - New `workspace_hoist_walks_every_importer` integration test covers the basic multi-importer hoist: root + `packages/foo`, where `foo` is the only importer pulling in `@pnpm.e2e/hello-world-js-bin-parent`. Asserts the transitive `@pnpm.e2e/hello-world-js-bin` lands at `<workspace>/node_modules/.pnpm/node_modules/@pnpm.e2e/hello-world-js-bin` even though the root has no deps. Verified by temporarily reverting the importers swap — the test fails with the expected "transitive of workspace package must be privately hoisted" message. - `known_failures` reasons updated. The original `workspace_install()` blanket reason (which pointed at #431) is replaced by three more-specific reasons that match the actual blockers: - `manifest_mutation_via_pnpm_add()` for tests that call `pnpm add`-equivalent flows mid-test - `workspace_filter_selection()` for tests that use `--filter`-style project selection (separate from #431; not yet implemented) - `hoist_workspace_packages_unsupported()` for the `hoistWorkspacePackages` config which links workspace projects themselves (separate `hoistedWorkspacePackages` shape pacquet doesn't model yet) `workspace_hoist_all_to_virtual_store_node_modules` now points at `partial_install_persists_hoisted_map()` since the basic shape is covered by the new `workspace_hoist_walks_every_importer` test — only the re-install-and-preserve assertion is still gated. - Lockfile types changed: `ResolvedDependencySpec.version` is now `ImporterDepVersion` (`Regular | Link`). `build_direct_deps_by_importer` uses `as_regular()` to skip `link:` workspace siblings — they're not snapshots and aren't hoist candidates (upstream handles them via the separate `hoistedWorkspacePackages` shape). - `SymlinkDirectDependencies` field cleanup from #443: `requester` was removed (now derived per-importer from `rootDir`), `workspace_root` was added. - Updated module docs and `plans/TEST_PORTING.md` to reflect that workspace install (#431) landed; remaining workspace-related hoist tests are gated on more-specific blockers. Tests: `just ready` 882/882 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
… graph build Three more hoist optimizations to claw back the remaining ~90ms gap vs `pacquet@main`. Each one is independent and verified by the existing test suite (12 unit + 36 CLI hoist tests, 882/882 overall). - **Borrow children in `BfsEntry`** — was the biggest hotspot. Pre-change, every BFS-visited node cloned its full `HashMap<String, PackageKey>` of children into the entry list. For ~1.5k snapshots × ~3 children avg, that's ~4.5k String + PackageKey allocations per install. The BFS now stores `&'a HashMap<String, PackageKey>` borrowed from the input graph (or the importer's direct-deps map for depth=-1 pseudo-nodes); the visited set + work queue also hold `&'a PackageKey` borrowed from the graph's keyspace via `get_key_value`. - **Drop within-entry alias sort** — the inner loop was running `child_pairs.sort_by(|a, b| a.0.cmp(b.0))` on every entry's children. Children is a `HashMap<alias, _>` so within-entry alias collisions are impossible, and the on-disk `hoistedDependencies` is `BTreeMap`-sorted at write time. The inner sort produced no observable difference and cost ~entries × Vec-build + log-fanout sort. Now iterates the HashMap directly. - **Parallelize `build_hoist_graph` via rayon** — single-threaded loop over ~1.5k snapshots becomes `par_iter().filter_map(...).collect()`. Per-snapshot work (children-map build, package-metadata lookup) is independent; rayon fans it across the existing thread pool. `sort_key` on `BfsEntry` stays a `String` (one alloc per node) because `PackageKey` deliberately doesn't carry an `Ord` impl that would match the old `to_string()` lex order — switching to component-wise lex would diverge for scoped names. The single String allocation per node is cheap relative to what was a HashMap clone. Tests: `just ready` 882/882 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean. CI integrated-benchmark will measure the win on Linux.
Tail-end follow-up to the BFS-borrow + parallel-graph-build perf
work. Targets the symlink phase's per-alias PathBuf cloning, which
duplicates the (already expensive) `to_virtual_store_name()` string
on every hoist entry.
- `symlink_hoisted_dependencies` now collects `(Arc<dep_dir>, kind,
alias)` tuples instead of `(PathBuf, PathBuf)` pairs. Sharing
`dep_dir` via `Arc` avoids cloning the PathBuf — and the
`to_virtual_store_name()` String inside it — once per alias on
multi-alias nodes. Most nodes have a single hoisted alias, but
the lockfile crate itself flags `to_virtual_store_name()` as
"far from optimal" (4× `String::replace` per call), so building
it just once per node is worth the indirection.
- Scope-dir detection now reads the alias string directly
(`alias.starts_with('@') && alias.find('/')`) instead of
materialising the full `dest` PathBuf to look up its parent.
Saves one PathBuf allocation per unscoped alias in phase 1
(~90% of the workload on a typical install).
- `dest` construction moves into the parallel closure so phase 1
doesn't allocate it; phase 3 builds it per task and uses it
twice (the syscall + the error variant).
Tests: `just ready` 882/882 pass; `cargo doc --workspace --no-deps`
with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean.
Whether this wins back any measurable wall time depends on the
benchmark's noise floor (±60-130ms std-dev on this fixture)
swallowing the trim; CI will tell.
Workspace install (#431 / #443) and GVS install activation (#432 / #449) both landed since the hoist work in #435 began. The rebase needed two adjustments to keep the symlink target paths correct under GVS: - `symlink_hoisted_dependencies` now takes `&VirtualStoreLayout` instead of `virtual_store_dir: &Path`. The symlink target (the source of each hoist symlink) goes through `layout.slot_dir(key)`, which under GVS resolves to `<store_dir>/links/<scope>/<name>/<version>/<hash>/` and falls back to the legacy `<virtual_store_dir>/<key.virtual_store_name>/` flat name when GVS is off. The hoist code never has to branch on `enable_global_virtual_store` itself. - The private hoist *target dir* (where hoist symlinks live) stays `config.virtual_store_dir.join("node_modules")`. Pacquet keeps `virtual_store_dir` project-local even with GVS enabled — only `global_virtual_store_dir` carries the shared `<store_dir>/links` path (see `Config::apply_global_virtual_store_derivation`). So `<root>/node_modules/.pnpm/node_modules` is still the right placement under both modes; the GVS-rewire concern from the issue description doesn't apply to pacquet's split-field design. Updated the call site comment to record this and dropped the stale "GVS not implemented yet — tracked at #432" note. Tests: `just ready` 894/894 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean. The full hoist CLI suite (36/36) and unit suite (12/12) still pass with the GVS layout pulled into the symlink path.
Address PR #445 review feedback after the rebase pulled in #439 (installability check / `SkippedSnapshots`) + #443 (workspace install). - **Honor `SkippedSnapshots` in the hoist pass** (Copilot, real bug). `install_frozen_lockfile.rs` was passing `HashSet::new()` to `HoistInputs.skipped` even though `compute_skipped_snapshots` already produces the optional+platform-incompatible skip set earlier in the same function. Effect of the fix: hoist no longer creates symlinks to slots that were never extracted, and an alias that would have been claimed by a skipped snapshot can now be claimed by a non-skipped sibling at a deeper level. The `HoistInputs` shape stays `&HashSet<PackageKey>` so `hoist.rs` doesn't depend on `installability`; the cheap conversion happens at the call site. - **Refresh `build_direct_deps_by_importer` doc comment** (Copilot). Doc said "currently only installs the root importer" — wrong since #443 (workspace install) and the follow-up that switched the call site to pass the full `importers` map. Updated to describe the current per-importer walk and the `link:` filter inside the loop. - **Refresh `skipped_optional_deps` known-failure reason** (Copilot). The reason claimed pacquet doesn't compute skip sets — wrong since #439. The actual gap is the test fixture: upstream's `@pnpm.e2e/not-compatible-with-any-os` isn't in pacquet's mocked registry, so the end-to-end skip-then-hoist path can't be exercised yet. Reason updated to point at the fixture gap, not the (already-implemented) skip computation. Tests: `just ready` 902/902 pass; `cargo doc --workspace --no-deps` with `RUSTDOCFLAGS="-D warnings"` clean; `just dylint` clean. The 36 CLI hoist tests + 12 unit tests still pass with the real skip set threaded through. Out of scope: Copilot's comment about `satisfies_package_manifest` only validating the root importer is about main's #447 freshness check (not this PR's hoist work). Worth a follow-up issue but not addressed here.
13 tasks
KSXGitHub
pushed a commit
that referenced
this pull request
May 13, 2026
Pull in 4 commits from upstream main: - feat(lockfile): BinaryResolution + VariationsResolution (#457) - feat: hoisting support (hoistPattern + publicHoistPattern) (#445) - test(git-fetcher): port §E git-fetcher tests (#462) - feat(real-hoist): ancestor-path-aware peer-shadow refusal (#461) Conflict: `crates/config/src/lib.rs` — the hoisting PR (#445) added `pub mod matcher;` adjacent to my `mod env_replace;`. Keep both module declarations.
6 tasks
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Closes #435.
Summary
@pnpm/config.matcher(*glob,!negation, first-match-wins) into a newpacquet_config::matchermodule — noregexdependency.getHoistedDependenciesandsymlinkHoistedDependenciesfrom upstreaminstalling/linking/hoist/src/index.tsinto a newpacquet_package_manager::hoistmodule. BFS by depth + lex on nodeId, public wins ties, first-seen wins per alias, root importer's direct-dep names seed the "already hoisted" set.InstallFrozenLockfile::runbetween the per-slot bin link and theimporting_doneemit. Result is threaded throughInstall::runinto.modules.yaml'shoistedDependencies.Config.hoist_patternandConfig.public_hoist_patterntoOption<Vec<String>>so the install-time guard mirrors upstream's!= nullcheck (issue §D).Upstream pinned at pnpm v11
94240bc046.Tests
crates/config/src/matcher.rs) — 6 tests, including direct ports of upstream'smatcher()andcreateMatcherWithIndex()test cases plus regex-special-char-literal coverage.crates/package-manager/src/hoist/tests.rs) — 12 unit tests covering empty graph, default*pattern, public hoist, public-wins-ties, negation, first-seen-wins, root direct-dep blocking, skipped-snapshot exclusion, bin-set collection, and graph-builder + direct-deps-builder helpers.crates/cli/tests/hoist.rs) — 9 integration tests using pnpm to generate the lockfile and pacquet to install with--frozen-lockfile. Cover default private hoist,publicHoistPattern: '*', both-empty,shamefullyHoistlegacy, scoped-pattern filter,!-negation,.modules.yamlround-trip, and bin linking on both private and public sides.installing/deps-installer/test/install/hoist.tsintoknown_failuresmodules viapacquet_testing_utils::allow_known_failure!. Each stub names the upstream URL line + the feature blocking it (workspace install Add workspace support topacquet install --frozen-lockfile#431, partial install Partial install with--frozen-lockfile: read+writenode_modules/.pnpm/lock.yamland skip unchanged snapshots #433, skipped optional deps, direct-dep bin precedence,extendNodePath).plans/TEST_PORTING.mdupdated to mark every portedhoist.tsentry with the corresponding pacquet test name.Out of scope (per the issue)
pacquet install --frozen-lockfile#432) —privateHoistedModulesDirrewiring + force-public-empty.pacquet install --frozen-lockfile#431) — multi-importerdirectDepsByImporterId. The hoist API takes the per-importer map from day one, so wiring is a follow-up rather than a rewrite.--frozen-lockfile: read+writenode_modules/.pnpm/lock.yamland skip unchanged snapshots #433) — pattern-change detection + persisted-map preservation across re-installs.nodeLinker: hoisted— different code path entirely.Test plan
cargo test -p pacquet-config matcher— 6/6 passcargo test -p pacquet-package-manager hoist::tests— 12/12 passcargo nextest run -p pacquet-cli --test hoist— 35/35 pass (9 real + 26 known_failure stubs)just ready— 670/670 tests passtaplo format --check— cleanjust dylint— cleanMatcher::matches(always-true) — both matcher unit tests and theboth_patterns_empty_produces_no_hoist_symlinksintegration test caught it; reverted.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Tests
Documentation