This repository was archived by the owner on Jul 21, 2026. It is now read-only.
feat(config,package-manager): hoistingLimits + externalDependencies knobs (#438 slice 10) - #522
Merged
Conversation
…nobs (#438 slice 10) Plumbs the two programmatic-only hoister knobs from `pnpm-workspace.yaml` through to the slice 4 walker and the slice 3 hoister. Both fields already existed on `HoistOpts`; this slice wires them end-to-end. - `Config::hoisting_limits: BTreeMap<String, BTreeSet<String>>` — per-importer block-list, locator-keyed (`'.@'` for the root). Reads `hoistingLimits: { ".@": [foo, bar] }` from yaml. Mirrors upstream's https://github.com/pnpm/pnpm/blob/94240bc046/installing/linking/real-hoist/src/index.ts#L10 programmatic-only knob, exposed as yaml for parity since the ergonomics of the locator-keyed map don't translate to a CLI flag. - `Config::external_dependencies: BTreeSet<String>` — name slots reserved at the root for an external linker (the Bit CLI is the only known consumer upstream). Reads `externalDependencies: [...]` from yaml. - `LockfileToHoistedDepGraphOptions` gains both fields and forwards them to `HoistOpts` in `build_dep_graph`. - `InstallFrozenLockfile::run` clones the two `Config` fields into the walker opts. Both knobs default to empty (no limits, no externals), matching upstream's default. Neither has any effect under `nodeLinker: isolated` — the isolated linker keeps per-importer subtrees by construction and doesn't consult the hoister. Tests: - `parses_hoisting_limits_from_yaml_and_applies` — yaml round-trip + apply_to. - `parses_external_dependencies_from_yaml_and_applies` — same. - `omitting_hoisting_limits_and_external_dependencies_keeps_defaults` — pins the apply_to skip-on-None branch so a yaml without these keys doesn't accidentally overwrite Config defaults. - `walker_forwards_external_dependencies_to_hoister` — end-to-end: the walker observes an empty graph for an externalised alias because the hoister stripped it. Pins the slice 10 plumbing.
📝 WalkthroughWalkthroughConfiguration fields for hoisting limits and external dependencies are added to ChangesHoisting Configuration Threading
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/config/src/workspace_yaml/tests.rs (1)
670-697: ⚡ Quick winImprove failure diagnostics for new non-
assert_eq!assertions.These membership checks use
assert!without printing the actual set contents, which makes failures harder to debug in CI. Please addeprintln!context (or switch toassert_eq!-style assertions with explicit expected values).♻️ Proposed change
- assert!(aliases.contains("foo") && aliases.contains("bar")); + eprintln!("hoistingLimits['.@']={aliases:?}"); + assert!(aliases.contains("foo") && aliases.contains("bar")); @@ - assert!(aliases.contains("foo") && aliases.contains("bar")); + eprintln!("config.hoisting_limits['.@']={aliases:?}"); + assert!(aliases.contains("foo") && aliases.contains("bar")); @@ - assert!(raw.contains("bit-bin") && raw.contains("some-other-external")); + eprintln!("externalDependencies(raw)={raw:?}"); + assert!(raw.contains("bit-bin") && raw.contains("some-other-external")); @@ - assert!(config.external_dependencies.contains("bit-bin")); - assert!(config.external_dependencies.contains("some-other-external")); + eprintln!("config.external_dependencies={:?}", config.external_dependencies); + assert!(config.external_dependencies.contains("bit-bin")); + assert!(config.external_dependencies.contains("some-other-external"));Based on learnings, in Rust tests non-
assert_eq!assertions should log relevant actual/expected context so failures are diagnosable without reruns.🤖 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 `@crates/config/src/workspace_yaml/tests.rs` around lines 670 - 697, The membership asserts in the test function parses_external_dependencies_from_yaml_and_applies lack diagnostic output; before the assert! checks on raw and config.external_dependencies, add contextual diagnostics (e.g., eprintln! showing the actual BTreeSet contents or switch the checks to assert_eq!/assert_ne! that include left/right values) so failures print the actual set values; update the assertions around raw.contains("bit-bin"), raw.contains("some-other-external") and config.external_dependencies.contains(...) and/or replace them with assertions that show the expected collection after calling settings.apply_to(&mut config, Path::new("/irrelevant")).
🤖 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 `@crates/config/src/workspace_yaml/tests.rs`:
- Around line 670-697: The membership asserts in the test function
parses_external_dependencies_from_yaml_and_applies lack diagnostic output;
before the assert! checks on raw and config.external_dependencies, add
contextual diagnostics (e.g., eprintln! showing the actual BTreeSet contents or
switch the checks to assert_eq!/assert_ne! that include left/right values) so
failures print the actual set values; update the assertions around
raw.contains("bit-bin"), raw.contains("some-other-external") and
config.external_dependencies.contains(...) and/or replace them with assertions
that show the expected collection after calling settings.apply_to(&mut config,
Path::new("/irrelevant")).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 42736b3a-4610-4e9f-bf5b-d315b1e452d3
📒 Files selected for processing (6)
crates/config/src/lib.rscrates/config/src/workspace_yaml.rscrates/config/src/workspace_yaml/tests.rscrates/package-manager/src/hoisted_dep_graph.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/install_package_from_registry/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Dylint
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/install_package_from_registry/tests.rscrates/config/src/lib.rscrates/package-manager/src/install_frozen_lockfile.rscrates/config/src/workspace_yaml.rscrates/config/src/workspace_yaml/tests.rscrates/package-manager/src/hoisted_dep_graph.rs
🧠 Learnings (5)
📚 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/config/src/workspace_yaml/tests.rs
📚 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/install_package_from_registry/tests.rscrates/config/src/lib.rscrates/package-manager/src/install_frozen_lockfile.rscrates/config/src/workspace_yaml.rscrates/config/src/workspace_yaml/tests.rscrates/package-manager/src/hoisted_dep_graph.rs
📚 Learning: 2026-05-13T22:52:32.579Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 506
File: crates/package-manager/src/link_bins.rs:238-241
Timestamp: 2026-05-13T22:52:32.579Z
Learning: In Rust code using `matches!` (or `match`) on a borrowed value (e.g., `meta: &PackageMetadata` and a field access like `meta.resolution`), it is safe to use wildcard/OR patterns such as `LockfileResolution::Binary(_) | LockfileResolution::Variations(_)` when the pattern does not bind the inner payload by value. `_` wildcard patterns (and similar discriminant-only patterns) should not move non-`Copy` data; they only test the variant and avoid ownership transfer. Do not flag these patterns as move-out-of-borrow/ownership compile errors. Only patterns that bind the inner value by value (e.g., `LockfileResolution::Binary(b)`) would require ownership and may trigger move errors when matching on data behind a borrow.
Applied to files:
crates/package-manager/src/install_package_from_registry/tests.rscrates/config/src/lib.rscrates/package-manager/src/install_frozen_lockfile.rscrates/config/src/workspace_yaml.rscrates/config/src/workspace_yaml/tests.rscrates/package-manager/src/hoisted_dep_graph.rs
📚 Learning: 2026-05-13T19:22:48.951Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 478
File: crates/package-manager/src/hoisted_dep_graph.rs:51-55
Timestamp: 2026-05-13T19:22:48.951Z
Learning: When reviewing this Rust codebase, avoid introducing/using a newtype like `PkgIdWithPatchHash` in only one module (e.g., `hoisted_dep_graph.rs`) if other related pacquet modules still represent the upstream pnpm “pkg id with patch hash” as a plain `String` (e.g., `virtual_store_layout`). For type consistency, either keep `pkg_id_with_patch_hash` as `String` here, or require a workspace-wide sweep that defines/extracts the `PkgIdWithPatchHash` newtype once and updates all call sites together (otherwise defer the refactor to a coordinated follow-up PR).
Applied to files:
crates/package-manager/src/install_package_from_registry/tests.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/hoisted_dep_graph.rs
📚 Learning: 2026-05-13T20:09:22.171Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 486
File: crates/package-manager/src/hoisted_dep_graph.rs:472-476
Timestamp: 2026-05-13T20:09:22.171Z
Learning: In pnpm/pacquet, when generating serialized `hoisted_locations` strings (used for `.modules.yaml`), normalize path separators to forward slashes (e.g., replace `\\` with `/`). This preserves cross-platform portability: the output must not use OS-native separators because upstream pnpm’s `path.relative` can return platform-specific separators, which would break consistency with pnpm/pacquet’s other serialized formats on Windows.
Applied to files:
crates/package-manager/src/install_package_from_registry/tests.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/hoisted_dep_graph.rs
🔇 Additional comments (6)
crates/package-manager/src/install_package_from_registry/tests.rs (1)
41-42: LGTM!crates/package-manager/src/install_frozen_lockfile.rs (1)
696-697: LGTM!crates/config/src/workspace_yaml/tests.rs (1)
653-668: LGTM!Also applies to: 700-715
crates/config/src/workspace_yaml.rs (1)
12-12: LGTM!Also applies to: 122-133, 311-311
crates/package-manager/src/hoisted_dep_graph.rs (1)
241-258: LGTM!Also applies to: 278-279, 397-398, 1859-1896
crates/config/src/lib.rs (1)
16-20: LGTM!Also applies to: 370-401
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #522 +/- ##
==========================================
+ Coverage 89.07% 89.10% +0.02%
==========================================
Files 125 125
Lines 14087 14114 +27
==========================================
+ Hits 12548 12576 +28
+ Misses 1539 1538 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.5282939314999995,
"stddev": 0.10545759101229238,
"median": 2.4843518851999997,
"user": 2.6058012999999995,
"system": 3.55969536,
"min": 2.4319826267,
"max": 2.7155283896999998,
"times": [
2.5650858946999997,
2.4611445477,
2.4319826267,
2.7155283896999998,
2.5075592227,
2.5184520506999997,
2.7127306336999997,
2.4539049556999997,
2.4556426826999997,
2.4609083107
]
},
{
"command": "pacquet@main",
"mean": 2.5422666119999997,
"stddev": 0.07338754147508435,
"median": 2.5545579772,
"user": 2.6020573,
"system": 3.5360899600000004,
"min": 2.4398658997,
"max": 2.6376514256999997,
"times": [
2.6081703776999996,
2.5444163847,
2.4572094506999997,
2.6051757366999997,
2.4757885197,
2.5646995696999997,
2.4398658997,
2.6376514256999997,
2.4799932327,
2.6096955227
]
},
{
"command": "pnpm",
"mean": 5.7497225511,
"stddev": 0.06925598940319949,
"median": 5.7343525942,
"user": 8.404968400000001,
"system": 4.272052960000001,
"min": 5.6543005917,
"max": 5.8655227367,
"times": [
5.8535318787,
5.6869506457,
5.7761074596999995,
5.6543005917,
5.7171617407,
5.7515434477,
5.7013258637,
5.7154210067,
5.8655227367,
5.7753601397
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.74645068774,
"stddev": 0.0289504034746069,
"median": 0.74641087324,
"user": 0.3600739,
"system": 1.6433985200000003,
"min": 0.71412791924,
"max": 0.80937669424,
"times": [
0.80937669424,
0.76369147124,
0.75904546724,
0.74499610524,
0.75884235924,
0.74782564124,
0.71606121424,
0.71804182024,
0.71412791924,
0.73249818524
]
},
{
"command": "pacquet@main",
"mean": 0.8126874515399999,
"stddev": 0.06806947857688721,
"median": 0.79652899824,
"user": 0.37083949999999993,
"system": 1.6339009199999999,
"min": 0.74094161224,
"max": 0.97092539124,
"times": [
0.97092539124,
0.78780426224,
0.81612160824,
0.74094161224,
0.76276698424,
0.86867275124,
0.75753929324,
0.80525373424,
0.77583552424,
0.84101335424
]
},
{
"command": "pnpm",
"mean": 2.38412747394,
"stddev": 0.11423826832993723,
"median": 2.3651313522399997,
"user": 2.8248189999999993,
"system": 2.17013412,
"min": 2.22769465924,
"max": 2.58422733124,
"times": [
2.42394841524,
2.32495019924,
2.58422733124,
2.30738417124,
2.22769465924,
2.41605135824,
2.3386081272399997,
2.2771683982399997,
2.3916545772399997,
2.5495875022399996
]
}
]
} |
This was referenced May 14, 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 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.
Summary
Plumbs the two programmatic-only hoister knobs from
pnpm-workspace.yamlthrough to the slice 4 walker and the slice 3 hoister. Both fields already existed onHoistOpts; this slice wires them end-to-end.Changes
Config::hoisting_limits: BTreeMap<String, BTreeSet<String>>— per-importer block-list, locator-keyed ('.@'for the root). ReadshoistingLimits: { \".@\": [foo, bar] }from yaml. Mirrors upstream'shoistingLimits, exposed as yaml for parity since the ergonomics of the locator-keyed map don't translate to a CLI flag.Config::external_dependencies: BTreeSet<String>— name slots reserved at the root for an external linker (the Bit CLI is the only known consumer upstream). ReadsexternalDependencies: [...]from yaml.LockfileToHoistedDepGraphOptionsgains both fields and forwards them toHoistOptsinbuild_dep_graph.InstallFrozenLockfile::runclones the twoConfigfields into the walker opts.Both knobs default to empty (no limits, no externals), matching upstream's default. Neither has any effect under
nodeLinker: isolated— the isolated linker keeps per-importer subtrees by construction and doesn't consult the hoister.Out of scope
Slice 11 — forbidden-combinations errors (
BUNDLED_DEPENDENCIES_WITHOUT_HOISTED;LOCKFILE_MISSING_DEPENDENCYalready exists in slice 3;MISSING_HOISTED_LOCATIONSdeferred untilpacquet rebuildlands).Test plan
just ready(all tests pass; lint, fmt, typos clean)just dylint(perfectionist) cleantaplo format --checkcleanRUSTDOCFLAGS=\"-D warnings\" cargo doc -p pacquet-config -p pacquet-package-manager -p pacquet-real-hoist --no-depscleanparses_hoisting_limits_from_yaml_and_applies— yaml round-trip +apply_to.parses_external_dependencies_from_yaml_and_applies— same.omitting_hoisting_limits_and_external_dependencies_keeps_defaults— pins theapply_toskip-on-None branch so a yaml without these keys doesn't accidentally overwriteConfigdefaults.walker_forwards_external_dependencies_to_hoister— end-to-end: the walker observes an empty graph for an externalised alias because the hoister stripped it.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Release Notes
hoistingLimitsconfiguration option in pnpm-workspace.yaml to control hoisting constraintsexternalDependenciesconfiguration option in pnpm-workspace.yaml for external package handling