feat(real-hoist): ancestor-path-aware peer-shadow refusal (#438)#461
Conversation
Lift the previous `UnsupportedPeerDependency` upfront guard and
replace it with the real peer check from upstream's
`getNodeHoistInfo`. The hoister now accepts lockfiles whose
packages declare peer dependencies and produces the layout pnpm
would: a peer-constrained dep only floats up to the root when
doing so doesn't change which package its peers resolve to.
Mechanics:
- `HoisterResult` carries the input `peer_names` set forward.
Upstream's `HoisterResult` doesn't (the peer set lives on its
intermediate `HoisterWorkTree`); pacquet runs the algorithm on
`HoisterResult` directly, so the peer set has to ride along.
- The BFS queue now carries an ancestor `Vec<Rc<HoisterResult>>`
per entry. Each child's path is the parent's path plus the
parent itself. The ancestor chain is what `would_shadow_peer`
walks back when deciding whether a candidate hoist would
change its own peer resolution.
- New `AbsorbDecision::PeerShadow` variant alongside `Free`,
`SameNode`, `Conflict`. Fired when:
1. Root declares the candidate's own name as a peer
(root-shadow guard — vacuous today since the wrapper's
`.` root has empty `peer_names`, kept for parity).
2. For each peer name `P` the candidate declares, the closest
ancestor providing `P` does so with a different ident than
the root's `P`. Promoting the candidate would silently
re-resolve the peer.
- The previous `UnsupportedPeerDependency` error variant and
`find_first_peer_constrained` scan are gone. The `non_exhaustive`
enum tag stays so future variants can be added without breaking
callers.
DAG-vs-tree caveat: upstream's `cloneTree` duplicates the work
tree into a strict tree per parent path, so each candidate visit
has a unique ancestor chain. Pacquet preserves the DAG and
records only the path the BFS actually used. In the rare case
where the same `Rc` is reachable through both a peer-compatible
and a peer-shadowing path, pacquet refuses to hoist; the layout
is at most over-nested, never under-nested. Documented on
`would_shadow_peer`.
Tests:
- `peer_constrained_node_stays_under_parent_when_root_provides_different_ident`
— `app → widget (peer: react) + react@17`, `root → react@18`.
widget cannot hoist because hoisting changes its peer to
react@18.
- `peer_constrained_node_hoists_when_ancestor_and_root_agree` —
same shape but `app → react@18` matches root's `react@18`
(shared Rc via wrapper dedup), so widget hoists freely.
- All 10 pre-existing tests still pass; the previously-failing
`peer_dependency_in_lockfile_surfaces_unsupported` test is
replaced by the two above.
Upstream reference: `yarnpkg/berry@4287909fa6` `hoist.ts:414`
(root-shadow) and `:454-479` (ancestor-path peer check).
📝 WalkthroughWalkthroughThe hoisting algorithm moves peer handling from an upfront scan into runtime DFS evaluation. ChangesPeer-shadow hoist refusal in nm_hoist runtime
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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/real-hoist/src/lib.rs (1)
813-818: 💤 Low valuePrefer
Rc::clone(&d.0)to make the refcount bump visible.As per coding guidelines: "Prefer
Rc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site."♻️ Suggested change
let provider_rc = ancestor .dependencies .borrow() .iter() .find(|d| d.0.name == *peer_name) - .map(|d| d.0.clone()); + .map(|d| Rc::clone(&d.0));🤖 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/real-hoist/src/lib.rs` around lines 813 - 818, The code uses d.0.clone() when mapping the found dependency into provider_rc; change this to Rc::clone(&d.0) so the reference-count increment is explicit at the call site. In the closure inside ancestor.dependencies.borrow().iter().find(...).map(|d| d.0.clone()), replace the clone call with Rc::clone(&d.0) (referencing provider_rc, ancestor, dependencies, d.0 and peer_name) to follow the Rc cloning guideline.
🤖 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/real-hoist/src/lib.rs`:
- Around line 813-818: The code uses d.0.clone() when mapping the found
dependency into provider_rc; change this to Rc::clone(&d.0) so the
reference-count increment is explicit at the call site. In the closure inside
ancestor.dependencies.borrow().iter().find(...).map(|d| d.0.clone()), replace
the clone call with Rc::clone(&d.0) (referencing provider_rc, ancestor,
dependencies, d.0 and peer_name) to follow the Rc cloning guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ea8be4a3-e6f4-456b-927c-b8454bc44a7d
📒 Files selected for processing (2)
crates/real-hoist/src/lib.rscrates/real-hoist/src/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: Agent
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Dylint
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
🧰 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/real-hoist/src/tests.rscrates/real-hoist/src/lib.rs
🧠 Learnings (3)
📚 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/real-hoist/src/tests.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 this repo’s Rust tests (run via `cargo nextest`), stdout/stderr from passing tests is captured and is only shown when a test fails. As a result, `eprintln!` (and similar stderr logging) in these tests should generally not be flagged as CI “noise” on the happy path; CI output should only appear for failing tests.
Applied to files:
crates/real-hoist/src/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/real-hoist/src/tests.rscrates/real-hoist/src/lib.rs
🔇 Additional comments (10)
crates/real-hoist/src/lib.rs (7)
108-127: LGTM!
638-648: LGTM!
586-606: LGTM!
660-677: LGTM!
696-724: LGTM!Also applies to: 753-753
739-743: LGTM!
880-880: LGTM!crates/real-hoist/src/tests.rs (3)
428-452: LGTM!
454-516: LGTM!
518-570: LGTM!
There was a problem hiding this comment.
Pull request overview
Updates real-hoist to accept lockfiles containing peer dependencies by removing the previous upfront “unsupported peers” guard and implementing an upstream-style peer-shadow check during hoisting. This is intended to keep hoisting behavior aligned with pnpm/yarn by preventing hoists that would change a package’s peer resolution.
Changes:
- Extend
HoisterResultwithpeer_namesand thread peer metadata through the result graph so the hoist algorithm can consult it. - Add ancestor-path-aware peer-shadow refusal (
AbsorbDecision::PeerShadow) during BFS hoisting. - Replace the prior “peer deps are unsupported” test with two new peer-aware hoisting behavior tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/real-hoist/src/lib.rs | Carries peer metadata into HoisterResult and adds peer-shadow refusal logic to the hoist BFS. |
| crates/real-hoist/src/tests.rs | Adds peer-aware hoisting tests and removes the previous unsupported-peer error expectation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #461 +/- ##
==========================================
- Coverage 87.45% 87.34% -0.11%
==========================================
Files 113 114 +1
Lines 9303 9454 +151
==========================================
+ Hits 8136 8258 +122
- Misses 1167 1196 +29 ☔ 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.6799885100800003,
"stddev": 0.11762829364862383,
"median": 2.64555679398,
"user": 2.63183378,
"system": 3.6407142599999993,
"min": 2.56238146998,
"max": 2.9044695319800002,
"times": [
2.80508809898,
2.56238146998,
2.69266119798,
2.58730526198,
2.62667557998,
2.9044695319800002,
2.79479034198,
2.58641712498,
2.5756584849800004,
2.6644380079800003
]
},
{
"command": "pacquet@main",
"mean": 2.5942173784800002,
"stddev": 0.05529708629407338,
"median": 2.60482573198,
"user": 2.6643484799999997,
"system": 3.6710094600000005,
"min": 2.50886166198,
"max": 2.69195019098,
"times": [
2.55564898198,
2.6448331889800003,
2.50886166198,
2.69195019098,
2.60808034398,
2.55142413598,
2.62533833698,
2.60157111998,
2.53843762798,
2.6160281959800002
]
},
{
"command": "pnpm",
"mean": 6.5268591011799995,
"stddev": 0.09823614662951329,
"median": 6.52053709898,
"user": 9.71731408,
"system": 4.709697259999999,
"min": 6.35056694598,
"max": 6.65043707698,
"times": [
6.53429959498,
6.43906249398,
6.35056694598,
6.45849565598,
6.4762503029800005,
6.50677460298,
6.6044241579800005,
6.65043707698,
6.61527156398,
6.633008615980001
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7063851681200001,
"stddev": 0.03582169059804349,
"median": 0.6989327348200001,
"user": 0.38929922,
"system": 1.5069167599999997,
"min": 0.68595354582,
"max": 0.80581467582,
"times": [
0.80581467582,
0.68939321982,
0.70308457882,
0.70582455882,
0.69991923982,
0.68595354582,
0.68604154082,
0.70377228482,
0.68610180682,
0.69794622982
]
},
{
"command": "pacquet@main",
"mean": 0.7446349064200001,
"stddev": 0.0504526894100798,
"median": 0.73432671932,
"user": 0.3884857199999999,
"system": 1.5187921599999998,
"min": 0.67804199782,
"max": 0.8324078218200001,
"times": [
0.81362919382,
0.71312794982,
0.69535836482,
0.8324078218200001,
0.72439318682,
0.67804199782,
0.71181657882,
0.77480069382,
0.75851302482,
0.74426025182
]
},
{
"command": "pnpm",
"mean": 2.74835920722,
"stddev": 0.10223141326987681,
"median": 2.71924669882,
"user": 3.2712328199999994,
"system": 2.3110458599999997,
"min": 2.62046812682,
"max": 2.90941320182,
"times": [
2.8477127438200003,
2.67340994682,
2.90941320182,
2.72814926182,
2.70424605582,
2.62046812682,
2.71034413582,
2.89557315282,
2.6444142088200002,
2.7498612378200002
]
}
]
} |
…461 review) Two review findings: 1. **Stale ancestor path in BFS.** The previous loop captured each queue entry's `ancestor_path` at enqueue time. When an intermediate node was hoisted to root between being queued and being dequeued, the path baked into the queue entry still listed its former parent's siblings as ancestors. `would_shadow_peer` then walked ex-ancestors and over-refused legitimate hoists. Replace the BFS with a recursive `hoist_subtree`: the path passed into each recursion is computed *after* the parent's hoist decision has been applied. A child that was just moved to root recurses with path `[root]`; a child that stayed nested recurses with `parent_path + [parent]`. Either way the path reflects the node's current position when its own children are evaluated. 2. **Misleading comment** in `would_shadow_peer`'s "ancestor has no provider" branch — said "the BFS will visit the candidate again" but the `visited` set explicitly prevents re-visits. Reword to describe what the branch actually does (walk further up looking for the real provider; if none exists, no shadow). The `VecDeque` import drops out with the BFS body. New regression test `peer_check_uses_post_hoist_ancestor_path_not_queue_time_path` pins the fix: a deep chain (`root → app → mid → terminal`, peer: react) where `mid` hoists to root past a conflicting `app.react@17`. Under the previous BFS the stale path `[root, app, mid]` made `terminal`'s peer check trip on `app.react@17 ≠ root.react@18` and refuse the hoist; under DFS the actual path `[root, mid]` finds no provider mismatch and `terminal` correctly flattens to root. All 13 tests pass (10 pre-existing, 2 from the initial 3c push, 1 new regression). `just ready`, `cargo doc --document-private-items`, `taplo format --check`, `just dylint` clean.
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.
Summary
Lifts the previous
UnsupportedPeerDependencyupfront guard and replaces it with the real peer check from upstream'sgetNodeHoistInfo. The hoister now accepts lockfiles whose packages declare peer dependencies and produces the layout pnpm would: a peer-constrained dep only floats up to the root when doing so doesn't change which package its peers resolve to.How it works
HoisterResultcarries the inputpeer_namesset forward. Upstream'sHoisterResultdoesn't (peer info lives on its intermediateHoisterWorkTree); pacquet runs the algorithm onHoisterResultdirectly, so the peer set rides along.The hoist driver is a recursive
hoist_subtreethat walks the result graph depth-first. Each recursion receives the candidate parent's current ancestor path (Vec<Rc<HoisterResult>>exclusive of the parent). After a child's hoist decision is applied, the path passed into the child's recursion reflects its new position — a freshly-hoisted child recurses with[root], a child that stayed nested recurses withparent_path + [parent]. (An earlier version of this PR used BFS with paths captured at queue time; that path went stale whenever a node was hoisted between being queued and dequeued, leading to over-refused hoists. The recursive form was Copilot's recommendation in code review; the bug regression is pinned bypeer_check_uses_post_hoist_ancestor_path_not_queue_time_path.)would_shadow_peerwalks that path to decide whether a candidate hoist would change its own peer resolution.New
AbsorbDecision::PeerShadowvariant alongsideFree/SameNode/Conflict. Fires when:.root has emptypeer_names, kept for parity).Pthe candidate declares, the closest ancestor providingPdoes so with a different ident than the root'sP. Promoting the candidate would silently re-resolve the peer.The previous
UnsupportedPeerDependencyerror variant andfind_first_peer_constrainedupfront scan are gone. The#[non_exhaustive]tag stays so future variants can be added without breaking callers.DAG-vs-tree caveat
Upstream's
cloneTreeduplicates the work tree into a strict tree per parent path, so each candidate visit has a unique ancestor chain. Pacquet preserves the DAG (its identity-dedup is a feature) and records only the path DFS actually used to reach the candidate. In the rare case where the sameRcis reachable through both a peer-compatible and a peer-shadowing path, pacquet refuses to hoist — the layout ends up at most over-nested, never under-nested. The trade-off is documented onwould_shadow_peer.Upstream reference
hoist.ts:414— root-shadow guard.hoist.ts:454-479— ancestor-path peer check.hoist.ts:670—cloneTree, the per-path-tree shape pacquet skips.Test plan
peer_constrained_node_stays_under_parent_when_root_provides_different_ident—app → widget (peer: react) + react@17,root → react@18. widget cannot hoist because hoisting would change its peer to react@18.peer_constrained_node_hoists_when_ancestor_and_root_agree— same shape butapp → react@18matches root'sreact@18(sharedRcvia wrapper dedup), so widget hoists freely.peer_check_uses_post_hoist_ancestor_path_not_queue_time_path— regression for the BFS-stale-path bug Copilot caught.root → app → mid → terminal(peer:react) wheremidhoists past a conflictingapp.react@17. Under the previous BFS the stale path[root, app, mid]madeterminal's peer check trip onapp.react@17 ≠ root.react@18and refuse the hoist; under DFS the actual post-hoist path[root, mid]finds no provider mismatch andterminalcorrectly flattens to root.one_transitive_dep_hoists_to_root,diamond_dep_hoists_once_to_root,version_conflict_keeps_loser_at_parent,deep_chain_flattens_in_one_pass,transitive_npm_alias_resolves_target_snapshot,non_empty_hoisting_limits_surfaces_unsupported,multi_importer_lockfile_surfaces_unsupported_workspace,external_dependencies_are_stripped_from_the_result,empty_lockfile_yields_empty_root,hoist_throws_on_broken_lockfile).peer_dependency_in_lockfile_surfaces_unsupportedtest from feat(real-hoist): single-pass parent-wins hoist (#438) #452 is replaced (peers are no longer an unsupported input).just ready(836 tests pass),cargo doc --document-private-items,taplo format --check,just dylintall clean.Written by an agent (Claude Code, claude-opus-4-7).